by Jeremi Piotrowski

[permalink] [raw]

Subject: Re: [RFC PATCH v1 3/6] x86/sev: Maintain shadow rmptable on Hyper-V

On Sun, Jan 29, 2023 at 04:37:24AM +0000, Michael Kelley (LINUX) wrote:
> From: Jeremi Piotrowski <[email protected]> Sent: Monday, January 23, 2023 8:51 AM
> >
> > Hyper-V can expose the SEV-SNP feature to guests, and manages the
> > system-wide RMP (Reverse Map) table. The SNP implementation in the
> > kernel needs access to the rmptable for tracking pages and deciding
> > when/how to issue rmpupdate/psmash. When running as a Hyper-V guest
> > with SNP support, an rmptable is allocated by the kernel during boot for
> > this purpose. Keep the table in sync with issued rmpupdate/psmash
> > instructions.
> >
> > The logic for how to update the rmptable comes from "AMD64 Architecture
> > Programmer’s Manual, Volume 3" which describes the psmash and rmpupdate
> > instructions. To ensure correctness of the SNP host code, the most
> > important fields are "assigned" and "page size".
> >
> > Signed-off-by: Jeremi Piotrowski <[email protected]>
> > ---
> > arch/x86/kernel/sev.c | 59 +++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 59 insertions(+)
> >
> > diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> > index 95404c7e5150..edec1ccb80b1 100644
> > --- a/arch/x86/kernel/sev.c
> > +++ b/arch/x86/kernel/sev.c
> > @@ -26,6 +26,7 @@
> > #include <linux/iommu.h>
> > #include <linux/amd-iommu.h>
> >
> > +#include <asm/mshyperv.h>
>
> The code in sev.c should be generic and appropriate for use with any
> hypervisor. As such, I think we want to avoid sev.c having a dependency
> on Hyper-V specific code, such as the <asm/mshyperv.h> include file
> and the ms_hyperv.nested_features variable as used below.
>
> Instead, create a boolean static variable in the sev.c module along with
> a wrapper function to set it. The logic that tests hv_no_rmp_table()
> should test this static variable instead, which would default to "false".
> Hypervisor-specific initialization code can call the wrapper function
> to set the variable to "true" based on whatever logic is appropriate for
> the hypervisor. This approach reverses the dependency and hopefully
> is usable by other hypervisors that want to offer nested SNP support.
>

ok, this makes sense. I've added a call to the wrapper to the init_mem_mapping
callback added before so that it is enabled together with allocating the
rmptable.

2023-01-30 17:25:07

by Jeremi Piotrowski

[permalink] [raw]

Subject: Re: [RFC PATCH v1 4/6] x86/amd: Configure necessary MSRs for SNP during CPU init when running as a guest

On Sun, Jan 29, 2023 at 04:44:05AM +0000, Michael Kelley (LINUX) wrote:
> From: Jeremi Piotrowski <[email protected]> Sent: Monday, January 23, 2023 8:51 AM
> >
> > Hyper-V may expose the SEV/SEV-SNP CPU features to the guest, but it is
> > up to the guest to use them. early_detect_mem_encrypt() checks
> > SYSCFG[MEM_ENCRYPT] and HWCR[SMMLOCK] and if these are not set the
> > SEV-SNP features are cleared. Check if we are running under a
> > hypervisor and if so - update SYSCFG and skip the HWCR check.
> >
> > It would be great to make this check more specific (checking for
> > Hyper-V) but this code runs before hypervisor detection on the boot cpu.
>
> Could you elaborate on why we would want this check to be Hyper-V
> specific? Per my comments on Patch 3 of this series, I would think the
> opposite. If possible, we want code like this to work on any hypervisor,
> and not have Hyper-V specific behavior in code outside of the Hyper-V
> modules. But I don't know this code well at all, so maybe there's an
> aspect I'm missing.
>
> Michael
>

This patch would work for any hypervisor, but I'm not sure every hypervisor
would chose to do things this way. Take the MSR_AMD64_SYSCFG_MEM_ENCRYPT
setting. It could be done like on baremetal with VM BIOS settings, which
wouldn't work well for Hyper-V. The VMM could also simply always return
MSR_AMD64_SYSCFG_MEM_ENCRYPT when it exposes SEV/-ES/-SNP flags to a non-SNP
guest (KVM always returns 0 in SYSCFG right now, and doesn't allow it to be
set).

But ultimately all this function does is mask off SEV/-ES/-SNP CPU flags based
on an assumption that no longer holds, so I think this approach to fixing it is
acceptable. The only thing I would check is whether it's possible to check the
coco attr here as well so that this definitely doesn't run for SNP guests
(provided this information is available at this point).

> >
> > Signed-off-by: Jeremi Piotrowski <[email protected]>
> > ---
> > arch/x86/kernel/cpu/amd.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> > index c7884198ad5b..17d91ac62937 100644
> > --- a/arch/x86/kernel/cpu/amd.c
> > +++ b/arch/x86/kernel/cpu/amd.c
> > @@ -565,6 +565,12 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
> > * don't advertise the feature under CONFIG_X86_32.
> > */
> > if (cpu_has(c, X86_FEATURE_SME) || cpu_has(c, X86_FEATURE_SEV)) {
> > + if (cpu_has(c, X86_FEATURE_HYPERVISOR)) {
> > + rdmsrl(MSR_AMD64_SYSCFG, msr);
> > + msr |= MSR_AMD64_SYSCFG_MEM_ENCRYPT;
> > + wrmsrl(MSR_AMD64_SYSCFG, msr);
> > + }
> > +
> > /* Check if memory encryption is enabled */
> > rdmsrl(MSR_AMD64_SYSCFG, msr);
> > if (!(msr & MSR_AMD64_SYSCFG_MEM_ENCRYPT))
> > @@ -584,7 +590,7 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
> > setup_clear_cpu_cap(X86_FEATURE_SME);
> >
> > rdmsrl(MSR_K7_HWCR, msr);
> > - if (!(msr & MSR_K7_HWCR_SMMLOCK))
> > + if (!(msr & MSR_K7_HWCR_SMMLOCK) && !cpu_has(c, X86_FEATURE_HYPERVISOR))
> > goto clear_sev;
> >
> > return;
> > --
> > 2.25.1