RE: [PATCH v6 06/13] x86/hyperv: Change vTOM handling to use standard coco mechanisms

From: "Michael Kelley (LINUX)" <mikelley@microsoft.com>
To: Borislav Petkov <bp@alien8.de>
Cc: "hpa@zytor.com" <hpa@zytor.com>,
	KY Srinivasan <kys@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	"wei.liu@kernel.org" <wei.liu@kernel.org>,
	Dexuan Cui <decui@microsoft.com>,
	"luto@kernel.org" <luto@kernel.org>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"edumazet@google.com" <edumazet@google.com>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"pabeni@redhat.com" <pabeni@redhat.com>,
	"lpieralisi@kernel.org" <lpieralisi@kernel.org>,
	"robh@kernel.org" <robh@kernel.org>,
	"kw@linux.com" <kw@linux.com>,
	"bhelgaas@google.com" <bhelgaas@google.com>,
	"arnd@arndb.de" <arnd@arndb.de>, "hch@lst.de" <hch@lst.de>,
	"m.szyprowski@samsung.com" <m.szyprowski@samsung.com>,
	"robin.murphy@arm.com" <robin.murphy@arm.com>,
	"thomas.lendacky@amd.com" <thomas.lendacky@amd.com>,
	"brijesh.singh@amd.com" <brijesh.singh@amd.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	Tianyu Lan <Tianyu.Lan@microsoft.com>,
	"kirill.shutemov@linux.intel.com"
	<kirill.shutemov@linux.intel.com>,
	"sathyanarayanan.kuppuswamy@linux.intel.com" 
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	"ak@linux.intel.com" <ak@linux.intel.com>,
	"isaku.yamahata@intel.com" <isaku.yamahata@intel.com>,
	"dan.j.williams@intel.com" <dan.j.williams@intel.com>,
	"jane.chu@oracle.com" <jane.chu@oracle.com>,
	"seanjc@google.com" <seanjc@google.com>,
	"tony.luck@intel.com" <tony.luck@intel.com>,
	"x86@kernel.org" <x86@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>,
	"iommu@lists.linux.dev" <iommu@lists.linux.dev>
Subject: RE: [PATCH v6 06/13] x86/hyperv: Change vTOM handling to use standard coco mechanisms
Date: Mon, 20 Mar 2023 13:30:54 +0000	[thread overview]
Message-ID: <BYAPR21MB16880C855EDB5AD3AECA473DD7809@BYAPR21MB1688.namprd21.prod.outlook.com> (raw)
In-Reply-To: <20230320112258.GCZBhCEpNAIk0rUDnx@fat_crate.local>

From: Borislav Petkov <bp@alien8.de> Sent: Monday, March 20, 2023 4:23 AM
> 
> On Wed, Mar 08, 2023 at 06:40:07PM -0800, Michael Kelley wrote:
> > diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
> > index 49b44f8..d1c3306 100644
> > --- a/arch/x86/coco/core.c
> > +++ b/arch/x86/coco/core.c
> > @@ -88,8 +106,6 @@ bool cc_platform_has(enum cc_attr attr)
> >  		return amd_cc_platform_has(attr);
> >  	case CC_VENDOR_INTEL:
> >  		return intel_cc_platform_has(attr);
> > -	case CC_VENDOR_HYPERV:
> > -		return hyperv_cc_platform_has(attr);
> >  	default:
> >  		return false;
> >  	}
> > @@ -103,11 +119,14 @@ u64 cc_mkenc(u64 val)
> >  	 * encryption status of the page.
> >  	 *
> >  	 * - for AMD, bit *set* means the page is encrypted
> > -	 * - for Intel *clear* means encrypted.
> > +	 * - for AMD with vTOM and for Intel, *clear* means encrypted
> >  	 */
> >  	switch (vendor) {
> >  	case CC_VENDOR_AMD:
> > -		return val | cc_mask;
> > +		if (sev_status & MSR_AMD64_SNP_VTOM)
> > +			return val & ~cc_mask;
> 
> This is silly. It should simply be:
> 
> 		if (sev_status & MSR_AMD64_SNP_VTOM)
> 			return val;
> 

To be clear, cc_mask contains the vTOM bit.  It's not zero.  See the
call to cc_set_mask() further down below.  My code makes sure the
vTOM bit is *not* set for the encrypted case, just like the
CC_VENDOR_INTEL code below does for the TDX SHARED bit.

> 
> > +		else
> > +			return val | cc_mask;
> >  	case CC_VENDOR_INTEL:
> >  		return val & ~cc_mask;
> >  	default:
> > @@ -120,7 +139,10 @@ u64 cc_mkdec(u64 val)
> >  	/* See comment in cc_mkenc() */
> >  	switch (vendor) {
> >  	case CC_VENDOR_AMD:
> > -		return val & ~cc_mask;
> > +		if (sev_status & MSR_AMD64_SNP_VTOM)
> > +			return val | cc_mask;
> 
> So if you set the C-bit, that doesn't make it decrypted on AMD. cc_mask
> on VTOM is 0 so why even bother?

cc_mask is *not* zero in the vTOM case.  It contains the vTOM bit.
The C-bit is not used or set in the vTOM case.

> 
> Same as the above.
> 
> > +		else
> > +			return val & ~cc_mask;
> >  	case CC_VENDOR_INTEL:
> >  		return val | cc_mask;
> >  	default:
> 
> ...
> 
> > +void __init hv_vtom_init(void)
> > +{
> > +	/*
> > +	 * By design, a VM using vTOM doesn't see the SEV setting,
> > +	 * so SEV initialization is bypassed and sev_status isn't set.
> > +	 * Set it here to indicate a vTOM VM.
> > +	 */
> 
> This looks like a hack. The SEV status MSR cannot be intercepted so the
> guest should see vTOM. How are you running vTOM without setting it even up?!
> 

In a vTOM VM, CPUID leaf 0x8000001f is filtered so it does *not* return
Bit 1 (SEV) as set.  Consequently, sme_enable() does not read MSR_AMD64_SEV
and does not populate sev_status.  The Linux boot sequence proceeds as if
SEV-SNP (and any other memory encryption) is *not* enabled, which is the
whole point of vTOM mode.  The tricky SME/SEV code for setting the C-bit,
getting the kernel encrypted, etc. is not needed or wanted because the
hardware already encrypts all memory by default.  The bootloader runs
with memory encrypted, it loads the kernel into encrypted memory, and
so forth.

sev_status is used here only to communicate to cc_platform_has() and
cc_mkenc() and cc_mkdec() that we're in vTOM mode.  If using sev_status
to communicate is confusing, this could just as easily be some new global
variable, and sev_status could be left as all zeros.

Michael

> > +	sev_status = MSR_AMD64_SNP_VTOM;
> > +	cc_set_vendor(CC_VENDOR_AMD);
> > +	cc_set_mask(ms_hyperv.shared_gpa_boundary);
> > +	physical_mask &= ms_hyperv.shared_gpa_boundary - 1;
> > +
> > +	x86_platform.hyper.is_private_mmio = hv_is_private_mmio;
> > +	x86_platform.guest.enc_cache_flush_required = hv_vtom_cache_flush_required;
> > +	x86_platform.guest.enc_tlb_flush_required = hv_vtom_tlb_flush_required;
> > +	x86_platform.guest.enc_status_change_finish = hv_vtom_set_host_visibility;
> > +}
> > +
> > +#endif /* CONFIG_AMD_MEM_ENCRYPT */