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