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

[Jeremi Piotrowski]

On Sun, Jan 29, 2023 at 04:44:05AM +0000, Michael Kelley (LINUX) wrote:
> From: Jeremi Piotrowski <jpiotrowski@xxxxxxxxxxxxxxxxxxx> 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 <jpiotrowski@xxxxxxxxxxxxxxxxxxx>
> > ---
> >  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