From: Michael Kelley <mhklinux@outlook.com>
To: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>,
"linux-coco@lists.linux.dev" <linux-coco@lists.linux.dev>,
"ardb@kernel.org" <ardb@kernel.org>,
"Lutomirski, Andy" <luto@kernel.org>,
"hch@infradead.org" <hch@infradead.org>,
"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
"thomas.lendacky@amd.com" <thomas.lendacky@amd.com>,
"haiyangz@microsoft.com" <haiyangz@microsoft.com>,
"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
"kirill.shutemov@linux.intel.com"
<kirill.shutemov@linux.intel.com>,
"mingo@redhat.com" <mingo@redhat.com>,
"seanjc@google.com" <seanjc@google.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"kys@microsoft.com" <kys@microsoft.com>,
"Cui, Dexuan" <decui@microsoft.com>,
"linux-mm@kvack.org" <linux-mm@kvack.org>,
"urezki@gmail.com" <urezki@gmail.com>,
"tglx@linutronix.de" <tglx@linutronix.de>,
"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
"peterz@infradead.org" <peterz@infradead.org>,
"hpa@zytor.com" <hpa@zytor.com>, "bp@alien8.de" <bp@alien8.de>,
"wei.liu@kernel.org" <wei.liu@kernel.org>,
"Rodel, Jorg" <jroedel@suse.de>,
"sathyanarayanan.kuppuswamy@linux.intel.com"
<sathyanarayanan.kuppuswamy@linux.intel.com>,
"lstoakes@gmail.com" <lstoakes@gmail.com>,
"x86@kernel.org" <x86@kernel.org>
Subject: RE: [PATCH v2 4/8] x86/sev: Enable PVALIDATE for PFNs without a valid virtual address
Date: Tue, 12 Dec 2023 18:35:18 +0000 [thread overview]
Message-ID: <SN6PR02MB4157AEA1E3B271D5C2CFA3F8D48EA@SN6PR02MB4157.namprd02.prod.outlook.com> (raw)
In-Reply-To: <00222b634b4ce443f8d1a793c4f8fe69b7ad39d0.camel@intel.com>
From: Edgecombe, Rick P <rick.p.edgecombe@intel.com> Sent: Tuesday, November 28, 2023 10:59 AM
>
> On Tue, 2023-11-28 at 18:08 +0000, Michael Kelley wrote:
> > >
> > > Sort of separately, if those vmalloc objections can't be worked
> > > through, did you consider doing something like text_poke() does
> > > (create
> > > the temporary mapping in a temporary MM) for pvalidate purposes? I
> > > don't know enough about what kind of special exceptions might popup
> > > during that operation though, might be playing with fire...
> >
> > Interesting idea. But from a quick glance at the text_poke() code,
> > such an approach seems somewhat complex, and I suspect it will have
> > the same perf issues (or worse) as creating a new vmalloc area for
> > each PVALIDATE invocation.
>
> Using new vmalloc area's will eventually result in a kernel shootdown,
> but usually have no flushes. text_poke will always result in a local-
> only flush. So at least whatever slowdown there is would only affect
> the calling thread.
>
> As for complexity, I think it might be simple to implement actually.
> What kind of special exceptions could come out of pvalidate, I'm not so
> sure. But the kernel terminates the VM on failure anyway, so maybe it's
> not an issue?
Sorry for the delay in getting back to this topic.
OK, I see now what you are suggesting. For each page that needs to
be PVALIDATE'd, use __text_poke() to create the temp mapping and
run PVALIDATE. However, there are some problems. __text_poke()
runs vmalloc_to_page() for addresses that aren't core kernel text,
and vmalloc_to_page() will fail if the PTE "present" bit has been
cleared. That could be easily addressed by changing it to use
slow_virt_to_phys(). But PVALIDATE also needs to be able to return
the PVALIDATE_FAIL_SIZEMISMATCH error code, which is tested for
in pvalidate_pages() and does not terminate the VM. __text_poke()
doesn’t have the machinery to return such an error code, and
that's harder to fix.
There's also the conceptual issue. The PVALIDATE use case isn't
working with a text area, so that case would be abusing "text_poke"
a bit. I could imagine __text_poke() having code to verify that it's
working on a text area, even if that code isn't there now.
To get a sense of performance, I hacked the equivalent of text_poke()
to work with PVALIDATE. The biggest case of transitioning pages from
encrypted to decrypted is the swiotlb area, which is 1 Gbyte for a
VM with 16 Gbytes or more of memory. In a Hyper-V CoCo VM, current
code takes about 270 milliseconds to transition that 1 Gbyte swiotlb area.
With my initial approach using vmap_pages_range(), that 270 ms went to
319 ms, which is fairly negligible in the overall VM boot time. Using the
text_poke() approach increased the time to 368 ms, which is bigger but
still probably not a show-stopper. It's definitely faster than creating a
new vmalloc area for each page that needs to be PVALIDATE'd, which
adds about 6 seconds to the boot time.
All-in-all, I'm back to my Plan B, which is to mark the pages "not
present" only in configurations where the hypervisor callbacks operate
on physical addresses instead of virtual addresses. Since SEV-SNP
needs virtual addresses, it will need to handle exceptions generated
by load_unaligned_zeropad() and do the appropriate fixup.
I'll try to get my "Plan B" patch set posted in a new few days.
Michael
>
> diff --git a/arch/x86/kernel/alternative.c
> b/arch/x86/kernel/alternative.c
> index 73be3931e4f0..a13293564eeb 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -1905,6 +1905,16 @@ void *text_poke(void *addr, const void *opcode,
> size_t len)
> return __text_poke(text_poke_memcpy, addr, opcode, len);
> }
>
> +static void text_poke_pvalidate(void *dst, const void *src, size_t
> len)
> +{
> + pvalidate(dst, len, true); // if fail, terminate
> +}
> +
> +void *pvalidated_poke(void *addr)
> +{
> + return __text_poke(text_poke_pvalidate, addr, NULL, PAGE_SIZE);
> +}
> +
> /**
> * text_poke_kgdb - Update instructions on a live kernel by kgdb
> * @addr: address to modify
>
>
>
> >
> > At this point, the complexity of creating the temp mapping for
> > PVALIDATE is seeming excessive. On balance it seems simpler to
> > revert to an approach where the use of set_memory_np() and
> > set_memory_p() is conditional. It would be necessary when #VC
> > and #VE exceptions are directed to a paravisor. (This assumes the
> > paravisor interface in the hypervisor callbacks does the natural thing
> > of working with physical addresses, so there's no need for a temp
> > mapping.)
> >
> > Optionally, the set_memory_np()/set_memory_p() approach could
> > be used in other cases where the hypervisor callbacks work with
> > physical addresses. But it can't be used with cases where the
> > hypervisor callbacks need valid virtual addresses.
> >
> > So on net, set_memory_np()/set_memory_p() would be used in
> > the Hyper-V cases of TDX and SEV-SNP with a paravisor. It could
> > optionally be used with TDX with no paravisor, but my sense is
> > that Kirill wants to keep TDX "as is" and let the exception handlers
> > do the load_unaligned_zeropad() fixup.
> >
> > It could not be used with SEV-SNP with no paravisor. Additional fixes
> > may be needed on the SEV-SNP side to properly fixup
> > load_unaligned_zeropad() accesses to a page that's in transition
> > between encrypted and decrypted.
> >
>
> Yea, I don't know about this paravisor/exception stuff.
next prev parent reply other threads:[~2023-12-12 18:35 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-21 21:20 [PATCH v2 0/8] x86/coco: Mark CoCo VM pages not present when changing encrypted state mhkelley58
2023-11-21 21:20 ` [PATCH v2 1/8] x86/coco: Use slow_virt_to_phys() in page transition hypervisor callbacks mhkelley58
2023-11-21 21:20 ` [PATCH v2 2/8] x86/mm: Don't do a TLB flush if changing a PTE that isn't marked present mhkelley58
2023-11-27 22:21 ` Edgecombe, Rick P
2023-11-28 17:34 ` Michael Kelley
2023-11-21 21:20 ` [PATCH v2 3/8] x86/mm: Remove "static" from vmap_pages_range() mhkelley58
2023-11-22 6:26 ` Christoph Hellwig
2023-11-23 0:24 ` Michael Kelley
2023-11-23 7:32 ` Christoph Hellwig
2023-11-27 1:06 ` Michael Kelley
2023-11-21 21:20 ` [PATCH v2 4/8] x86/sev: Enable PVALIDATE for PFNs without a valid virtual address mhkelley58
2023-11-27 21:38 ` Edgecombe, Rick P
2023-11-28 18:08 ` Michael Kelley
2023-11-28 18:59 ` Edgecombe, Rick P
2023-12-12 18:35 ` Michael Kelley [this message]
2023-11-21 21:20 ` [PATCH v2 5/8] x86/mm: Mark CoCo VM pages not present while changing encrypted state mhkelley58
2023-11-21 21:20 ` [PATCH v2 6/8] x86/mm: Merge CoCo prepare and finish hypervisor callbacks mhkelley58
2023-11-21 21:20 ` [PATCH v2 7/8] x86/mm: Remove unnecessary call layer for __set_memory_enc_pgtable() mhkelley58
2023-11-21 21:20 ` [PATCH v2 8/8] x86/mm: Add comments about errors in set_memory_decrypted()/encrypted() mhkelley58
2023-11-24 10:06 ` [PATCH v2 0/8] x86/coco: Mark CoCo VM pages not present when changing encrypted state kirill.shutemov
2023-11-28 19:12 ` Michael Kelley
2023-11-29 15:10 ` kirill.shutemov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=SN6PR02MB4157AEA1E3B271D5C2CFA3F8D48EA@SN6PR02MB4157.namprd02.prod.outlook.com \
--to=mhklinux@outlook.com \
--cc=akpm@linux-foundation.org \
--cc=ardb@kernel.org \
--cc=bp@alien8.de \
--cc=dave.hansen@linux.intel.com \
--cc=decui@microsoft.com \
--cc=haiyangz@microsoft.com \
--cc=hch@infradead.org \
--cc=hpa@zytor.com \
--cc=jroedel@suse.de \
--cc=kirill.shutemov@linux.intel.com \
--cc=kys@microsoft.com \
--cc=linux-coco@lists.linux.dev \
--cc=linux-hyperv@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=lstoakes@gmail.com \
--cc=luto@kernel.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=rick.p.edgecombe@intel.com \
--cc=sathyanarayanan.kuppuswamy@linux.intel.com \
--cc=seanjc@google.com \
--cc=tglx@linutronix.de \
--cc=thomas.lendacky@amd.com \
--cc=urezki@gmail.com \
--cc=wei.liu@kernel.org \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).