Otherwise, it gets warnings like below when number of vcpus > 1:

  warning: TDX enforces set the feature: CPUID.01H:EDX.ht [bit 28]

This is because x86_confidential_guest_check_features() checks
env->features[] instead of the cpuid date set up by cpu_x86_cpuid()

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

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 472ab206d8fe..214a1b00a815 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6571,7 +6571,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         *edx = env->features[FEAT_1_EDX];
         if (threads_per_pkg > 1) {
             *ebx |= threads_per_pkg << 16;
-            *edx |= CPUID_HT;
         }
         if (!cpu->enable_pmu) {
             *ecx &= ~CPUID_EXT_PDCM;
@@ -7784,6 +7783,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     Error *local_err = NULL;
     unsigned requested_lbr_fmt;
 
+    qemu_early_init_vcpu(cs);
+
 #if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
     /* Use pc-relative instructions in system-mode */
     tcg_cflags_set(cs, CF_PCREL);
@@ -7851,6 +7852,14 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         }
     }
 
+    /*
+     * It needs to called after feature filter because KVM doesn't report HT
+     * as supported
+     */
+    if (cs->nr_cores * cs->nr_threads > 1) {
+        env->features[FEAT_1_EDX] |= CPUID_HT;
+    }
+
     /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
      * CPUID[1].EDX.
      */
-- 
2.34.1

On 11/5/24 07:24, Xiaoyao Li wrote:
> Otherwise, it gets warnings like below when number of vcpus > 1:
> 
>    warning: TDX enforces set the feature: CPUID.01H:EDX.ht [bit 28]
> 
> This is because x86_confidential_guest_check_features() checks
> env->features[] instead of the cpuid date set up by cpu_x86_cpuid()
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>   target/i386/cpu.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 472ab206d8fe..214a1b00a815 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6571,7 +6571,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>           *edx = env->features[FEAT_1_EDX];
>           if (threads_per_pkg > 1) {
>               *ebx |= threads_per_pkg << 16;
> -            *edx |= CPUID_HT;
>           }
>           if (!cpu->enable_pmu) {
>               *ecx &= ~CPUID_EXT_PDCM;
> @@ -7784,6 +7783,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>       Error *local_err = NULL;
>       unsigned requested_lbr_fmt;
>   
> +    qemu_early_init_vcpu(cs);
> +
>   #if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
>       /* Use pc-relative instructions in system-mode */
>       tcg_cflags_set(cs, CF_PCREL);
> @@ -7851,6 +7852,14 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>           }
>       }
>   
> +    /*
> +     * It needs to called after feature filter because KVM doesn't report HT
> +     * as supported

Does it, since kvm_arch_get_supported_cpuid() has the following line?

     if (function == 1 && reg == R_EDX) {
         ...
         /* KVM never reports CPUID_HT but QEMU can support when vcpus > 1 */
         ret |= CPUID_HT;

?

Paolo

> +     */
> +    if (cs->nr_cores * cs->nr_threads > 1) {
> +        env->features[FEAT_1_EDX] |= CPUID_HT;
> +    }

On 11/5/2024 5:12 PM, Paolo Bonzini wrote:
> On 11/5/24 07:24, Xiaoyao Li wrote:
>> Otherwise, it gets warnings like below when number of vcpus > 1:
>>
>>    warning: TDX enforces set the feature: CPUID.01H:EDX.ht [bit 28]
>>
>> This is because x86_confidential_guest_check_features() checks
>> env->features[] instead of the cpuid date set up by cpu_x86_cpuid()
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   target/i386/cpu.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 472ab206d8fe..214a1b00a815 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -6571,7 +6571,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t 
>> index, uint32_t count,
>>           *edx = env->features[FEAT_1_EDX];
>>           if (threads_per_pkg > 1) {
>>               *ebx |= threads_per_pkg << 16;
>> -            *edx |= CPUID_HT;
>>           }
>>           if (!cpu->enable_pmu) {
>>               *ecx &= ~CPUID_EXT_PDCM;
>> @@ -7784,6 +7783,8 @@ static void x86_cpu_realizefn(DeviceState *dev, 
>> Error **errp)
>>       Error *local_err = NULL;
>>       unsigned requested_lbr_fmt;
>> +    qemu_early_init_vcpu(cs);
>> +
>>   #if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
>>       /* Use pc-relative instructions in system-mode */
>>       tcg_cflags_set(cs, CF_PCREL);
>> @@ -7851,6 +7852,14 @@ static void x86_cpu_realizefn(DeviceState *dev, 
>> Error **errp)
>>           }
>>       }
>> +    /*
>> +     * It needs to called after feature filter because KVM doesn't 
>> report HT
>> +     * as supported
> 
> Does it, since kvm_arch_get_supported_cpuid() has the following line?
> 
>      if (function == 1 && reg == R_EDX) {
>          ...
>          /* KVM never reports CPUID_HT but QEMU can support when vcpus > 
> 1 */
>          ret |= CPUID_HT;
> 
> ?

It seems I mixed it up with no_autoenable_flags. /faceplam

CPUID_HT doesn't get enabled by x86_cpu_expand_features() for "-cpu 
host/max". It won't be filtered by x86_cpu_filter_features() either 
because QEMU sets it in kvm_arch_get_supported_cpuid().

yes, the comment is wrong and comment needs to be dropped. The code can 
be move up to just below x86_cpu_expand_features() or inside it?

> Paolo
> 
>> +     */
>> +    if (cs->nr_cores * cs->nr_threads > 1) {
>> +        env->features[FEAT_1_EDX] |= CPUID_HT;
>> +    }
> 

On Tue, Nov 5, 2024 at 10:33 AM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>
> On 11/5/2024 5:12 PM, Paolo Bonzini wrote:
> > On 11/5/24 07:24, Xiaoyao Li wrote:
> >> Otherwise, it gets warnings like below when number of vcpus > 1:
> >>
> >>    warning: TDX enforces set the feature: CPUID.01H:EDX.ht [bit 28]
> >>
> >> This is because x86_confidential_guest_check_features() checks
> >> env->features[] instead of the cpuid date set up by cpu_x86_cpuid()
> >>
>
> It seems I mixed it up with no_autoenable_flags. /faceplam
>
> CPUID_HT doesn't get enabled by x86_cpu_expand_features() for "-cpu
> host/max". It won't be filtered by x86_cpu_filter_features() either
> because QEMU sets it in kvm_arch_get_supported_cpuid().
>
> yes, the comment is wrong and comment needs to be dropped. The code can
> be move up to just below x86_cpu_expand_features() or inside it?

Inside it seems okay, and you can then remove it from cpu_x86_cpuid().

However, let's also add qemu_early_init_vcpu() to the realize function
from all targets, and remove

    MachineState *ms = MACHINE(qdev_get_machine());

   cpu->nr_cores = machine_topo_get_cores_per_socket(ms);
    cpu->nr_threads =  ms->smp.threads;

from qemu_init_vcpu(). You can resend patches 58 and 59 separately
from the TDX series.

Paolo