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

[Dave Hansen <dave.hansen@intel.com>]

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.