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.
