[v6] QEMU TDX support

[PATCH v6 56/60] i386/tdx: Don't treat SYSCALL as unavailable

Posted by Xiaoyao Li 1 year, 3 months ago

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 target/i386/kvm/tdx.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
index 9cb099e160e4..05475edf72bd 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -734,6 +734,13 @@ static int tdx_check_features(X86ConfidentialGuest *cg, CPUState *cs)
 
         requested = env->features[w];
         unavailable = requested & ~actual;
+        /*
+         * Intel enumerates SYSCALL bit as 1 only when processor in 64-bit
+         * mode and before vcpu running it's not in 64-bit mode.
+         */
+        if (w == FEAT_8000_0001_EDX && unavailable & CPUID_EXT2_SYSCALL) {
+            unavailable &= ~CPUID_EXT2_SYSCALL;
+        }
         mark_unavailable_features(cpu, w, unavailable, unav_prefix);
         if (unavailable) {
             mismatch = true;
-- 
2.34.1

Re: [PATCH v6 56/60] i386/tdx: Don't treat SYSCALL as unavailable

Posted by Daniel P. Berrangé 1 year, 3 months ago

On Tue, Nov 05, 2024 at 01:24:04AM -0500, Xiaoyao Li wrote:

Preferrably explain the rationale for why this is needed in
the commit message.

> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  target/i386/kvm/tdx.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
> index 9cb099e160e4..05475edf72bd 100644
> --- a/target/i386/kvm/tdx.c
> +++ b/target/i386/kvm/tdx.c
> @@ -734,6 +734,13 @@ static int tdx_check_features(X86ConfidentialGuest *cg, CPUState *cs)
>  
>          requested = env->features[w];
>          unavailable = requested & ~actual;
> +        /*
> +         * Intel enumerates SYSCALL bit as 1 only when processor in 64-bit
> +         * mode and before vcpu running it's not in 64-bit mode.
> +         */
> +        if (w == FEAT_8000_0001_EDX && unavailable & CPUID_EXT2_SYSCALL) {
> +            unavailable &= ~CPUID_EXT2_SYSCALL;
> +        }
>          mark_unavailable_features(cpu, w, unavailable, unav_prefix);
>          if (unavailable) {
>              mismatch = true;
> -- 
> 2.34.1
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v6 56/60] i386/tdx: Don't treat SYSCALL as unavailable

Posted by Paolo Bonzini 1 year, 3 months ago

On 11/5/24 07:24, Xiaoyao Li wrote:
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>   target/i386/kvm/tdx.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
> index 9cb099e160e4..05475edf72bd 100644
> --- a/target/i386/kvm/tdx.c
> +++ b/target/i386/kvm/tdx.c
> @@ -734,6 +734,13 @@ static int tdx_check_features(X86ConfidentialGuest *cg, CPUState *cs)
>   
>           requested = env->features[w];
>           unavailable = requested & ~actual;
> +        /*
> +         * Intel enumerates SYSCALL bit as 1 only when processor in 64-bit
> +         * mode and before vcpu running it's not in 64-bit mode.
> +         */
> +        if (w == FEAT_8000_0001_EDX && unavailable & CPUID_EXT2_SYSCALL) {
> +            unavailable &= ~CPUID_EXT2_SYSCALL;
> +        }
>           mark_unavailable_features(cpu, w, unavailable, unav_prefix);
>           if (unavailable) {
>               mismatch = true;

This seems like a TDX module bug?  It's the kind of thing that I guess 
could be worked around in KVM.

If we do it in QEMU, I'd rather see it as

             actual = cpuid_entry_get_reg(entry, wi->cpuid.reg);
             switch (w) {
             case FEAT_8000_0001_EDX:
                 actual |= CPUID_EXT2_SYSCALL;
                 break;
             }
             break;

Paolo

Re: [PATCH v6 56/60] i386/tdx: Don't treat SYSCALL as unavailable

Posted by Xiaoyao Li 1 year ago

On 11/5/2024 5:59 PM, Paolo Bonzini wrote:
> On 11/5/24 07:24, Xiaoyao Li wrote:
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   target/i386/kvm/tdx.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
>> index 9cb099e160e4..05475edf72bd 100644
>> --- a/target/i386/kvm/tdx.c
>> +++ b/target/i386/kvm/tdx.c
>> @@ -734,6 +734,13 @@ static int 
>> tdx_check_features(X86ConfidentialGuest *cg, CPUState *cs)
>>           requested = env->features[w];
>>           unavailable = requested & ~actual;
>> +        /*
>> +         * Intel enumerates SYSCALL bit as 1 only when processor in 
>> 64-bit
>> +         * mode and before vcpu running it's not in 64-bit mode.
>> +         */
>> +        if (w == FEAT_8000_0001_EDX && unavailable & 
>> CPUID_EXT2_SYSCALL) {
>> +            unavailable &= ~CPUID_EXT2_SYSCALL;
>> +        }
>>           mark_unavailable_features(cpu, w, unavailable, unav_prefix);
>>           if (unavailable) {
>>               mismatch = true;
> 
> This seems like a TDX module bug?  

I don't think so. The value of CPUID_EXT2_SYSCALL depends on the mode of 
the vcpu. Per SDM, it's 0 outside 64-bit mode.

The initial state of TDX vcpu is 32-bit protected mode. At the time of 
calling KVM_TDX_GET_CPUID, vcpu hasn't started running. So the value 
should be 0.

There indeed is a TDX module. After vcpu starts running and TD guest 
switches to 64-bit mode. The value of this bit returned by TDX module 
via global metadata CPUID value still remains 0.

Off the topic, for me, it's really a bad API to return TDX's CPUID value 
via TD-scope metadata. It fits better with TD VCPU scope metadata.

> It's the kind of thing that I guess 
> could be worked around in KVM.
> 
> If we do it in QEMU, I'd rather see it as
> 
>              actual = cpuid_entry_get_reg(entry, wi->cpuid.reg);
>              switch (w) {
>              case FEAT_8000_0001_EDX:
>                  actual |= CPUID_EXT2_SYSCALL;
>                  break;
>              }
>              break;

I'll change to this way.


> Paolo
>