RE: [RFC PATCH] clocksource: hyper-v: Enable the tsc_page for a TDX VM in TD mode

[Dexuan Cui]

> 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