RE: [Patch v3 07/14] x86/hyperv: Change vTOM handling to use standard coco mechanisms

[Michael Kelley (LINUX)]

From: Dave Hansen <dave.hansen@xxxxxxxxx> Sent: Tuesday, November 22, 2022 10:30 AM
> 
> On 11/22/22 10:22, Michael Kelley (LINUX) wrote:
> > Anyway, that's where I think this should go. Does it make sense?
> > Other thoughts?
> 
> I think hard-coding the C-bit behavior and/or position to a vendor was
> probably a bad idea.  Even the comment:
> 
> u64 cc_mkenc(u64 val)
> {
>         /*
>          * Both AMD and Intel use a bit in the page table to indicate
>          * encryption status of the page.
>          *
>          * - for AMD, bit *set* means the page is encrypted
>          * - for Intel *clear* means encrypted.
>          */
> 
> doesn't make a lot of sense now.  Maybe we should just have a:
> 
> 	CC_ATTR_ENC_SET
> 
> which gets set for the "AMD" behavior and is clear for the "Intel"
> behavior.  Hyper-V code running on AMD can set the attribute to get teh
> "Intel" behavior.
> 
> That sure beats having a Hyper-V vendor.

To me, figuring out the encryption bit polarity and position isn't
the hard part to set up.  We've got three technologies: TDX,
AMD "C-bit", and arguably, AMD vTOM.  Future silicon and architectural
enhancements will most likely be variations and improvements on
these rather than something completely new (not that I'm necessarily
aware of what the processor vendors may have planned).   The AMD
"C-bit" technology already has sub-cases (SME, SEV, SEV-ES, SEV-SNP)
because of the architectural history.  Any of the technologies may
get additional subcases over time.  Whether those subcases are
represented as new CC_VENDOR_* options, or CC_ATTR_* options
on an existing CC_VENDOR_* should be driven by the level of
commonality with what already exists.  There's no hard-and-fast
rule.  Just do whatever makes the most sense.

I'm proposing AMD vTOM as a separate CC_VENDOR_AMD_VTOM
because it is rather different from CC_VENDOR_AMD, and not just
in the polarity and position of the encryption bit.  But if we really
wanted to just make it a sub-case of CC_VENDOR_AMD, I could
probably be convinced.  The key is cleanly handling all the other
attributes like CC_ATTR_GUEST_STATE_ENCRYPT,
CC_ATTR_ACCESS_IOAPIC_ENCRYPTED (that I add in this patch set),
CC_ATTR_GUEST_UNROLL_STRING_IO, etc. where we want to avoid
too many hacky special cases.  The current code structure where the
CC_VENDOR_* selection determines the CC_ATTR_* values seems
to work OK.

Anyway, that's my thinking.  CC_VENDOR_HYPERV goes away, but
the behavior is still essentially the same when replaced by
CC_VENDOR_AMD_VTOM.

Michael