Message-ID: <BYAPR21MB16886A07302A09B96FFFAE84D7D99@BYAPR21MB1688.namprd21.prod.outlook.com>
Date:   Thu, 9 Feb 2023 17:29:13 +0000
From:   "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
To:     Dave Hansen <dave.hansen@...el.com>,
        Borislav Petkov <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>
CC:     "hpa@...or.com" <hpa@...or.com>, KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        Dexuan Cui <decui@...rosoft.com>,
        "luto@...nel.org" <luto@...nel.org>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "lpieralisi@...nel.org" <lpieralisi@...nel.org>,
        "robh@...nel.org" <robh@...nel.org>, "kw@...ux.com" <kw@...ux.com>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "arnd@...db.de" <arnd@...db.de>, "hch@....de" <hch@....de>,
        "m.szyprowski@...sung.com" <m.szyprowski@...sung.com>,
        "robin.murphy@....com" <robin.murphy@....com>,
        "thomas.lendacky@....com" <thomas.lendacky@....com>,
        "brijesh.singh@....com" <brijesh.singh@....com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        Tianyu Lan <Tianyu.Lan@...rosoft.com>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "sathyanarayanan.kuppuswamy@...ux.intel.com" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        "ak@...ux.intel.com" <ak@...ux.intel.com>,
        "isaku.yamahata@...el.com" <isaku.yamahata@...el.com>,
        "dan.j.williams@...el.com" <dan.j.williams@...el.com>,
        "jane.chu@...cle.com" <jane.chu@...cle.com>,
        "seanjc@...gle.com" <seanjc@...gle.com>,
        "tony.luck@...el.com" <tony.luck@...el.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>
Subject: RE: [PATCH v5 06/14] x86/ioremap: Support hypervisor specified range
 to map as encrypted

From: Dave Hansen <dave.hansen@...el.com> Sent: Wednesday, February 8, 2023 7:10 AM
> 
> On 2/7/23 16:18, Michael Kelley (LINUX) wrote:
> > In v2 of this patch series, you had concerns about CC_ATTR_PARAVISOR being too
> > generic. [1]   After some back-and-forth discussion in this thread, Boris is back to
> > preferring it.   Can you live with CC_ATTR_PARAVISOR?  Just trying to reach
> > consensus ...
> 
> I still think it's too generic.  Even the comment was trying to be too
> generic:
> 
> > +	/**
> > +	 * @CC_ATTR_HAS_PARAVISOR: Guest VM is running with a paravisor
> > +	 *
> > +	 * The platform/OS is running as a guest/virtual machine with
> > +	 * a paravisor in VMPL0. Having a paravisor affects things
> > +	 * like whether the I/O APIC is emulated and operates in the
> > +	 * encrypted or decrypted portion of the guest physical address
> > +	 * space.
> > +	 *
> > +	 * Examples include Hyper-V SEV-SNP guests using vTOM.
> > +	 */
> > +	CC_ATTR_HAS_PARAVISOR,
> 
> This doesn't help me figure out when I should use CC_ATTR_HAS_PARAVISOR
> really at all.  It "operates in the encrypted or decrypted portion..."
> Which one is it?  Should I be adding or removing encryption on the
> mappings for paravisors?
> 
> That's opposed to:
> 
> > +	/**
> > +	 * @CC_ATTR_ACCESS_IOAPIC_ENCRYPTED: Guest VM IO-APIC is encrypted
> > +	 *
> > +	 * The platform/OS is running as a guest/virtual machine with
> > +	 * an IO-APIC that is emulated by a paravisor running in the
> > +	 * guest VM context. As such, the IO-APIC is accessed in the
> > +	 * encrypted portion of the guest physical address space.
> > +	 *
> > +	 * Examples include Hyper-V SEV-SNP guests using vTOM.
> > +	 */
> > +	CC_ATTR_ACCESS_IOAPIC_ENCRYPTED,
> 
> Which makes this code almost stupidly obvious:
> 
> > -	flags = pgprot_decrypted(flags);
> > +	if (!cc_platform_has(CC_ATTR_ACCESS_IOAPIC_ENCRYPTED))
> > +		flags = pgprot_decrypted(flags);
> 
> "Oh, if it's access is not encrypted, then get the decrypted version of
> the flags."
> 
> Compare that to:
> 
> 	if (!cc_platform_has(CC_ATTR_PARAVISOR))
> 		flags = pgprot_decrypted(flags);
> 
> Which is a big fat WTF.  Because a paravisor "operates in the encrypted
> or decrypted portion..."  So is this if() condition correct or inverted?
> It's utterly impossible to tell because of how generic the option is.
> 
> The only way to make sense of the generic thing is to do:
> 
> 	/* Paravisors have a decrypted IO-APIC mapping: */
> 	if (!cc_platform_has(CC_ATTR_PARAVISOR))
> 		flags = pgprot_decrypted(flags);
> 
> at every site to state the assumption and make the connection between
> paravisors and the behavior.  If you want to go do _that_, then fine by
> me.  But, at that point, the naming is pretty worthless because you
> could also have said "goldfish" instead of "paravisor" and it makes an
> equal amount of sense:
> 
> 	/* Goldfish have a decrypted IO-APIC mapping: */
> 	if (!cc_platform_has(CC_ATTR_GOLDFISH))
> 		flags = pgprot_decrypted(flags);
> 
> I get it, naming is hard.

Boris --

Any further comments?  Trying to reach consensus.  A
solution aligned with Dave's arguments would keep the current
CC_ATTR_ACCESS_IOAPIC_ENCRYPTED, and add
CC_ATTR_ACCESS_TPM_ENCRYPTED to cover the TPM case,
which decouples the two.

Yes, naming is hard.  Reaching consensus on naming is even
harder.  :-)

Michael 

Powered by blists - more mailing lists