RE: [PATCH v5 06/14] x86/ioremap: Support hypervisor specified range to map as encrypted

["Michael Kelley (LINUX)" <mikelley@microsoft.com>]

From: Dave Hansen <dave.hansen@intel.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