RE: [PATCH v2 8/9] x86/hyperv: Use TDX GHCI to access some MSRs in a TDX VM with the paravisor

[Dexuan Cui]

> From: Michael Kelley (LINUX) <mikelley@xxxxxxxxxxxxx>
> Sent: Monday, August 21, 2023 12:33 PM
> > [...]
> > @@ -186,7 +186,49 @@ bool hv_ghcb_negotiate_protocol(void)
> >  	return true;
> >  }
> >
> > -void hv_ghcb_msr_write(u64 msr, u64 value)
> > +#define EXIT_REASON_MSR_READ		31
> > +#define EXIT_REASON_MSR_WRITE		32
> 
> These exit reasons are defined in arch/x86/include/uapi/asm/vmx.h.
> Are they conceptually the same thing and should be reused?

There is no VM Exit here, but I think we can use the header file.

I'll add
#include <uapi/asm/vmx.h>
and remove the 2 defines I added in this file.

> > +static void hv_tdx_read_msr(u64 msr, u64 *val)
> 
> Could you make the function name be
> hv_tdx_msr_read() so it matches hv_ghcb_msr_read()
> and hv_ivm_msr_read()?  :-)

Will do.
I'll also move the new functions around so that the new functions
won't get interleaved among the hv_ghcb_* functions.

I'll remove
EXPORT_SYMBOL_GPL(hv_ivm_msr_write);
EXPORT_SYMBOL_GPL(hv_ivm_msr_read);
because we never really used hv_ghcb_msr_write() and
hv_ghcb_msr_read() in any module.

The changelog is updated accordingly.

> > +{
> > +	struct tdx_hypercall_args args = {
> > +		.r10 = TDX_HYPERCALL_STANDARD,
> > +		.r11 = EXIT_REASON_MSR_READ,
> > +		.r12 = msr,
> > +	};
> > +
> > +#ifdef CONFIG_INTEL_TDX_GUEST
> > +	u64 ret = __tdx_hypercall_ret(&args);
> > +#else
> > +	u64 ret = HV_STATUS_INVALID_PARAMETER;
> > +#endif
> > +
> > +	if (WARN_ONCE(ret, "Failed to emulate MSR read: %lld\n", ret))
> > +		*val = 0;
> > +	else
> > +		*val = args.r11;
> > +}
> > +
> > +static void hv_tdx_write_msr(u64 msr, u64 val)
> 
> Same here on the function name.

Will fix.

> >  #ifdef CONFIG_AMD_MEM_ENCRYPT
> > -void hv_ghcb_msr_write(u64 msr, u64 value);
> > -void hv_ghcb_msr_read(u64 msr, u64 *value);
> > +void hv_ivm_msr_write(u64 msr, u64 value);
> > +void hv_ivm_msr_read(u64 msr, u64 *value);
> 
> These declarations are under CONFIG_AMD_MEM_ENCRYPT, which
> is problematic for TDX if the kernel is built with CONFIG_INTEL_TDX_GUEST
> but not CONFIG_AMD_MEM_ENCRYPT.  Presumably we want to make
> sure that combination builds and works correctly.
> 
> I think there's a bigger problem in that arch/x86/hyperv/ivm.c has
> a big #ifdef CONFIG_AMD_MEM_ENCRYPT in it, and TDX with paravisor
> wants to use the "vtom" functions that are under that #ifdef.

I worked out a new version of this patch:
https://github.com/dcui/tdx/commit/17b065e175082497907563297083b13dc86d84a9

I updated arch/x86/include/asm/mshyperv.h and arch/x86/hyperv/ivm.c
so that the kernel can still buid if CONFIG_AMD_MEM_ENCRYPT or
CONFIG_INTEL_TDX_GUEST is not set, or neither is set.