> From: Dave Hansen <dave.hansen@xxxxxxxxx>
> Sent: Thursday, May 23, 2024 7:26 AM
> [...]
> On 5/22/24 19:24, Dexuan Cui wrote:
> ...
> > +static bool noinstr intel_cc_platform_td_l2(enum cc_attr attr)
> > +{
> > + switch (attr) {
> > + case CC_ATTR_GUEST_MEM_ENCRYPT:
> > + case CC_ATTR_MEM_ENCRYPT:
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > static bool noinstr intel_cc_platform_has(enum cc_attr attr)
> > {
> > + if (tdx_partitioned_td_l2)
> > + return intel_cc_platform_td_l2(attr);
> > +
> > switch (attr) {
> > case CC_ATTR_GUEST_UNROLL_STRING_IO:
> > case CC_ATTR_HOTPLUG_DISABLED:
>
> On its face, this _looks_ rather troubling. It just hijacks all of the
> attributes. It totally bifurcates the code. Anything that gets added
> to intel_cc_platform_has() now needs to be considered for addition to
> intel_cc_platform_td_l2().
Maybe the bifurcation is necessary? TD mode is different from
Partitioned TD mode (L2), after all. Another reason for the bifurcation
is: currently online/offline'ing is disallowed for a TD VM, but actually
Hyper-V is able to support CPU online/offline'ing for a TD VM in
Partitioned TD mode (L2) -- how can we allow online/offline'ing for such
a VM?
BTW, the bifurcation code is copied from amd_cc_platform_has(), where
an AMD SNP VM may run in the vTOM mode.
> > --- a/arch/x86/mm/mem_encrypt_amd.c
> > +++ b/arch/x86/mm/mem_encrypt_amd.c
> ...
> > @@ -529,7 +530,7 @@ void __init
> mem_encrypt_free_decrypted_mem(void)
> > * CC_ATTR_MEM_ENCRYPT, aren't necessarily equivalent in a
> Hyper-V VM
> > * using vTOM, where sme_me_mask is always zero.
> > */
> > - if (sme_me_mask) {
> > + if (sme_me_mask || (cc_vendor == CC_VENDOR_INTEL
> && !tdx_partitioned_td_l2)) {
> > r = set_memory_encrypted(vaddr, npages);
> > if (r) {
> > pr_warn("failed to free unused decrypted
> pages\n");
>
> If _ever_ there were a place for a new CC_ attribute, this would be it.
Not sure how to add a new CC attribute for the __bss_decrypted support.
For the cpu online/offline'ing support, I'm not sure how to add a new
CC attribute and not introduce the bifurcation.
> It's also a bit concerning that now we've got a (cc_vendor ==
> CC_VENDOR_INTEL) check in an amd.c file.
I agree my change here is ugly...
Currently the __bss_decrypted support is only used for SNP.
Not sure if we should get it to work for TDX as well.
> So all of that on top of Kirill's "why do we need this in the first
> place" questions leave me really scratching my head on this one.
Probably I'll just use local APIC timer in such a VM or delay enabling
Hyper-V TSC page to a later place where set_memory_decrypted()
works for me. However, I still would like to find out how to allow
CPU online/offline'ing for a TDX VM in Partitioned TD mode (L2).
Thanks,
Dexuan
