diff mbox series

[RFC,v6,090/104] KVM: TDX: Handle TDX PV CPUID hypercall

Message ID 98939c0ec83a109c8f49045e82096d6cdd5dafa3.1651774251.git.isaku.yamahata@intel.com (mailing list archive)
State New, archived
Headers show
Series KVM TDX basic feature support | expand

Commit Message

Isaku Yamahata May 5, 2022, 6:15 p.m. UTC
From: Isaku Yamahata <isaku.yamahata@intel.com>

Wire up TDX PV CPUID hypercall to the KVM backend function.

Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
---
 arch/x86/kvm/vmx/tdx.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Sagi Shahar June 14, 2022, 6:15 p.m. UTC | #1
On Thu, May 5, 2022 at 11:16 AM <isaku.yamahata@intel.com> wrote:
>
> From: Isaku Yamahata <isaku.yamahata@intel.com>
>
> Wire up TDX PV CPUID hypercall to the KVM backend function.
>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> ---
>  arch/x86/kvm/vmx/tdx.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 9c712f661a7c..c7cdfee397ec 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -946,12 +946,34 @@ static int tdx_emulate_vmcall(struct kvm_vcpu *vcpu)
>         return 1;
>  }
>
> +static int tdx_emulate_cpuid(struct kvm_vcpu *vcpu)
> +{
> +       u32 eax, ebx, ecx, edx;
> +
> +       /* EAX and ECX for cpuid is stored in R12 and R13. */
> +       eax = tdvmcall_a0_read(vcpu);
> +       ecx = tdvmcall_a1_read(vcpu);
> +
> +       kvm_cpuid(vcpu, &eax, &ebx, &ecx, &edx, true);

According to the GHCI spec section 3.6
(TDG.VP.VMCALL<Instruction.CPUID>) we should return
VMCALL_INVALID_OPERAND if an invalid CPUID is requested.

kvm_cpuid already returns false in this case so we should use that
return value to set the tdvmcall return code in case of invalid leaf.
> +
> +       tdvmcall_a0_write(vcpu, eax);
> +       tdvmcall_a1_write(vcpu, ebx);
> +       tdvmcall_a2_write(vcpu, ecx);
> +       tdvmcall_a3_write(vcpu, edx);
> +
> +       tdvmcall_set_return_code(vcpu, TDG_VP_VMCALL_SUCCESS);
> +
> +       return 1;
> +}
> +
>  static int handle_tdvmcall(struct kvm_vcpu *vcpu)
>  {
>         if (tdvmcall_exit_type(vcpu))
>                 return tdx_emulate_vmcall(vcpu);
>
>         switch (tdvmcall_leaf(vcpu)) {
> +       case EXIT_REASON_CPUID:
> +               return tdx_emulate_cpuid(vcpu);
>         default:
>                 break;
>         }
> --
> 2.25.1
>

Sagi
Isaku Yamahata June 29, 2022, 10:13 a.m. UTC | #2
On Tue, Jun 14, 2022 at 11:15:00AM -0700,
Sagi Shahar <sagis@google.com> wrote:

> On Thu, May 5, 2022 at 11:16 AM <isaku.yamahata@intel.com> wrote:
> >
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> >
> > Wire up TDX PV CPUID hypercall to the KVM backend function.
> >
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > ---
> >  arch/x86/kvm/vmx/tdx.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 9c712f661a7c..c7cdfee397ec 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -946,12 +946,34 @@ static int tdx_emulate_vmcall(struct kvm_vcpu *vcpu)
> >         return 1;
> >  }
> >
> > +static int tdx_emulate_cpuid(struct kvm_vcpu *vcpu)
> > +{
> > +       u32 eax, ebx, ecx, edx;
> > +
> > +       /* EAX and ECX for cpuid is stored in R12 and R13. */
> > +       eax = tdvmcall_a0_read(vcpu);
> > +       ecx = tdvmcall_a1_read(vcpu);
> > +
> > +       kvm_cpuid(vcpu, &eax, &ebx, &ecx, &edx, true);
> 
> According to the GHCI spec section 3.6
> (TDG.VP.VMCALL<Instruction.CPUID>) we should return
> VMCALL_INVALID_OPERAND if an invalid CPUID is requested.
> 
> kvm_cpuid already returns false in this case so we should use that
> return value to set the tdvmcall return code in case of invalid leaf.

Based on CPUID instruction, cpuid results in #UD when lock prefix is used or
earlier CPU that doesn't support cpuid instruction.
So I'm not sure what CPUID input result in INVALID_OPERAND error.
Does the following make sense for you?

--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -1347,7 +1347,7 @@ static int tdx_emulate_cpuid(struct kvm_vcpu *vcpu)
        eax = tdvmcall_a0_read(vcpu);
        ecx = tdvmcall_a1_read(vcpu);

-       kvm_cpuid(vcpu, &eax, &ebx, &ecx, &edx, true);
+       kvm_cpuid(vcpu, &eax, &ebx, &ecx, &edx, false);

        tdvmcall_a0_write(vcpu, eax);
        tdvmcall_a1_write(vcpu, ebx);

thanks,
Sagi Shahar July 18, 2022, 10:37 p.m. UTC | #3
On Wed, Jun 29, 2022 at 3:14 AM Isaku Yamahata <isaku.yamahata@gmail.com> wrote:
>
> On Tue, Jun 14, 2022 at 11:15:00AM -0700,
> Sagi Shahar <sagis@google.com> wrote:
>
> > On Thu, May 5, 2022 at 11:16 AM <isaku.yamahata@intel.com> wrote:
> > >
> > > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > >
> > > Wire up TDX PV CPUID hypercall to the KVM backend function.
> > >
> > > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > > ---
> > >  arch/x86/kvm/vmx/tdx.c | 22 ++++++++++++++++++++++
> > >  1 file changed, 22 insertions(+)
> > >
> > > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > > index 9c712f661a7c..c7cdfee397ec 100644
> > > --- a/arch/x86/kvm/vmx/tdx.c
> > > +++ b/arch/x86/kvm/vmx/tdx.c
> > > @@ -946,12 +946,34 @@ static int tdx_emulate_vmcall(struct kvm_vcpu *vcpu)
> > >         return 1;
> > >  }
> > >
> > > +static int tdx_emulate_cpuid(struct kvm_vcpu *vcpu)
> > > +{
> > > +       u32 eax, ebx, ecx, edx;
> > > +
> > > +       /* EAX and ECX for cpuid is stored in R12 and R13. */
> > > +       eax = tdvmcall_a0_read(vcpu);
> > > +       ecx = tdvmcall_a1_read(vcpu);
> > > +
> > > +       kvm_cpuid(vcpu, &eax, &ebx, &ecx, &edx, true);
> >
> > According to the GHCI spec section 3.6
> > (TDG.VP.VMCALL<Instruction.CPUID>) we should return
> > VMCALL_INVALID_OPERAND if an invalid CPUID is requested.
> >
> > kvm_cpuid already returns false in this case so we should use that
> > return value to set the tdvmcall return code in case of invalid leaf.
>
> Based on CPUID instruction, cpuid results in #UD when lock prefix is used or
> earlier CPU that doesn't support cpuid instruction.
> So I'm not sure what CPUID input result in INVALID_OPERAND error.
> Does the following make sense for you?
>
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -1347,7 +1347,7 @@ static int tdx_emulate_cpuid(struct kvm_vcpu *vcpu)
>         eax = tdvmcall_a0_read(vcpu);
>         ecx = tdvmcall_a1_read(vcpu);
>
> -       kvm_cpuid(vcpu, &eax, &ebx, &ecx, &edx, true);
> +       kvm_cpuid(vcpu, &eax, &ebx, &ecx, &edx, false);
>
>         tdvmcall_a0_write(vcpu, eax);
>         tdvmcall_a1_write(vcpu, ebx);
>
> thanks,

If any CPUID request is considered valid, then perhaps the spec itself
needs to be updated.

Right now it clearly states that TDG.VP.VMCALL_INVALID_OPERAND is
returned if "Invalid CPUID requested" which I understood as a
non-existing leaf. But if you say that a non-existing leaf is still a
valid CPUID request than I'm not sure what "Invalid CPUID requested"
means in the spec itself.

>
> --
> Isaku Yamahata <isaku.yamahata@gmail.com>
Sean Christopherson July 19, 2022, 7:23 p.m. UTC | #4
On Mon, Jul 18, 2022, Sagi Shahar wrote:
> On Wed, Jun 29, 2022 at 3:14 AM Isaku Yamahata <isaku.yamahata@gmail.com> wrote:
> >
> > On Tue, Jun 14, 2022 at 11:15:00AM -0700,
> > Sagi Shahar <sagis@google.com> wrote:
> >
> > > On Thu, May 5, 2022 at 11:16 AM <isaku.yamahata@intel.com> wrote:
> > > >
> > > > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > > >
> > > > Wire up TDX PV CPUID hypercall to the KVM backend function.
> > > >
> > > > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > > > ---
> > > >  arch/x86/kvm/vmx/tdx.c | 22 ++++++++++++++++++++++
> > > >  1 file changed, 22 insertions(+)
> > > >
> > > > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > > > index 9c712f661a7c..c7cdfee397ec 100644
> > > > --- a/arch/x86/kvm/vmx/tdx.c
> > > > +++ b/arch/x86/kvm/vmx/tdx.c
> > > > @@ -946,12 +946,34 @@ static int tdx_emulate_vmcall(struct kvm_vcpu *vcpu)
> > > >         return 1;
> > > >  }
> > > >
> > > > +static int tdx_emulate_cpuid(struct kvm_vcpu *vcpu)
> > > > +{
> > > > +       u32 eax, ebx, ecx, edx;
> > > > +
> > > > +       /* EAX and ECX for cpuid is stored in R12 and R13. */
> > > > +       eax = tdvmcall_a0_read(vcpu);
> > > > +       ecx = tdvmcall_a1_read(vcpu);
> > > > +
> > > > +       kvm_cpuid(vcpu, &eax, &ebx, &ecx, &edx, true);
> > >
> > > According to the GHCI spec section 3.6
> > > (TDG.VP.VMCALL<Instruction.CPUID>) we should return
> > > VMCALL_INVALID_OPERAND if an invalid CPUID is requested.
> > >
> > > kvm_cpuid already returns false in this case so we should use that
> > > return value to set the tdvmcall return code in case of invalid leaf.
> >
> > Based on CPUID instruction, cpuid results in #UD when lock prefix is used or
> > earlier CPU that doesn't support cpuid instruction.
> > So I'm not sure what CPUID input result in INVALID_OPERAND error.
> > Does the following make sense for you?
> >
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -1347,7 +1347,7 @@ static int tdx_emulate_cpuid(struct kvm_vcpu *vcpu)
> >         eax = tdvmcall_a0_read(vcpu);
> >         ecx = tdvmcall_a1_read(vcpu);
> >
> > -       kvm_cpuid(vcpu, &eax, &ebx, &ecx, &edx, true);
> > +       kvm_cpuid(vcpu, &eax, &ebx, &ecx, &edx, false);

Barring new verbiage in the GHCI, this change is correct.  CPUID behavior is
different for AMD vs. Intel, but the behavior is architectural and well-defined
for each vendor.  KVM handles this by emulating the AMD vs. Intel behavior based
on the configured vCPU model.

Forcing an exact match would make TDX follow AMD behavior, not Intel behavior.

> >         tdvmcall_a0_write(vcpu, eax);
> >         tdvmcall_a1_write(vcpu, ebx);
> >
> > thanks,
> 
> If any CPUID request is considered valid, then perhaps the spec itself
> needs to be updated.

Agreed.  Documenting that "Invalid CPUID requested" is a legal return value implies
that TDX is creating a _third_ variant of CPUID behavior.  Dropping the error return
seems like the simplest approach, unless it was added for a specific reason?
diff mbox series

Patch

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 9c712f661a7c..c7cdfee397ec 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -946,12 +946,34 @@  static int tdx_emulate_vmcall(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+static int tdx_emulate_cpuid(struct kvm_vcpu *vcpu)
+{
+	u32 eax, ebx, ecx, edx;
+
+	/* EAX and ECX for cpuid is stored in R12 and R13. */
+	eax = tdvmcall_a0_read(vcpu);
+	ecx = tdvmcall_a1_read(vcpu);
+
+	kvm_cpuid(vcpu, &eax, &ebx, &ecx, &edx, true);
+
+	tdvmcall_a0_write(vcpu, eax);
+	tdvmcall_a1_write(vcpu, ebx);
+	tdvmcall_a2_write(vcpu, ecx);
+	tdvmcall_a3_write(vcpu, edx);
+
+	tdvmcall_set_return_code(vcpu, TDG_VP_VMCALL_SUCCESS);
+
+	return 1;
+}
+
 static int handle_tdvmcall(struct kvm_vcpu *vcpu)
 {
 	if (tdvmcall_exit_type(vcpu))
 		return tdx_emulate_vmcall(vcpu);
 
 	switch (tdvmcall_leaf(vcpu)) {
+	case EXIT_REASON_CPUID:
+		return tdx_emulate_cpuid(vcpu);
 	default:
 		break;
 	}