diff mbox series

[v7,51/52] i386/tdx: Validate phys_bits against host value

Message ID 20250124132048.3229049-52-xiaoyao.li@intel.com (mailing list archive)
State New
Headers show
Series QEMU TDX support | expand

Commit Message

Xiaoyao Li Jan. 24, 2025, 1:20 p.m. UTC 
For TDX guest, the phys_bits is not configurable and can only be
host/native value.

Validate phys_bits inside tdx_check_features().

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 target/i386/host-cpu.c | 2 +-
 target/i386/host-cpu.h | 1 +
 target/i386/kvm/tdx.c  | 8 ++++++++
 3 files changed, 10 insertions(+), 1 deletion(-)

Comments

Paolo Bonzini Jan. 31, 2025, 6:27 p.m. UTC | #1 
On Fri, Jan 24, 2025 at 2:40 PM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>
> For TDX guest, the phys_bits is not configurable and can only be
> host/native value.
>
> Validate phys_bits inside tdx_check_features().

Hi Xiaoyao,

to avoid

qemu-kvm: TDX requires guest CPU physical bits (48) to match host CPU
physical bits (52)

I need options like

-cpu host,phys-bits=52,guest-phys-bits=52,host-phys-bits-limit=52,-kvm-asyncpf-int

to start a TDX guest, is that intentional?

Thanks,

Paolo

> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  target/i386/host-cpu.c | 2 +-
>  target/i386/host-cpu.h | 1 +
>  target/i386/kvm/tdx.c  | 8 ++++++++
>  3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c
> index 3e4e85e729c8..8a15af458b05 100644
> --- a/target/i386/host-cpu.c
> +++ b/target/i386/host-cpu.c
> @@ -15,7 +15,7 @@
>  #include "system/system.h"
>
>  /* Note: Only safe for use on x86(-64) hosts */
> -static uint32_t host_cpu_phys_bits(void)
> +uint32_t host_cpu_phys_bits(void)
>  {
>      uint32_t eax;
>      uint32_t host_phys_bits;
> diff --git a/target/i386/host-cpu.h b/target/i386/host-cpu.h
> index 6a9bc918baa4..b97ec01c9bec 100644
> --- a/target/i386/host-cpu.h
> +++ b/target/i386/host-cpu.h
> @@ -10,6 +10,7 @@
>  #ifndef HOST_CPU_H
>  #define HOST_CPU_H
>
> +uint32_t host_cpu_phys_bits(void);
>  void host_cpu_instance_init(X86CPU *cpu);
>  void host_cpu_max_instance_init(X86CPU *cpu);
>  bool host_cpu_realizefn(CPUState *cs, Error **errp);
> diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
> index bb75eb06dad9..c906a76c4c0e 100644
> --- a/target/i386/kvm/tdx.c
> +++ b/target/i386/kvm/tdx.c
> @@ -24,6 +24,7 @@
>
>  #include "cpu.h"
>  #include "cpu-internal.h"
> +#include "host-cpu.h"
>  #include "hw/i386/e820_memory_layout.h"
>  #include "hw/i386/x86.h"
>  #include "hw/i386/tdvf.h"
> @@ -838,6 +839,13 @@ static int tdx_check_features(X86ConfidentialGuest *cg, CPUState *cs)
>          return -1;
>      }
>
> +    if (cpu->phys_bits != host_cpu_phys_bits()) {
> +        error_report("TDX requires guest CPU physical bits (%u) "
> +                     "to match host CPU physical bits (%u)",
> +                     cpu->phys_bits, host_cpu_phys_bits());
> +        exit(1);
> +    }
> +
>      return 0;
>  }
>
> --
> 2.34.1
>
Xiaoyao Li Feb. 2, 2025, 2:39 p.m. UTC | #2 
On 2/1/2025 2:27 AM, Paolo Bonzini wrote:
> On Fri, Jan 24, 2025 at 2:40 PM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>>
>> For TDX guest, the phys_bits is not configurable and can only be
>> host/native value.
>>
>> Validate phys_bits inside tdx_check_features().
> 
> Hi Xiaoyao,
> 
> to avoid
> 
> qemu-kvm: TDX requires guest CPU physical bits (48) to match host CPU
> physical bits (52)
> 
> I need options like
> 
> -cpu host,phys-bits=52,guest-phys-bits=52,host-phys-bits-limit=52,-kvm-asyncpf-int
> 
> to start a TDX guest, is that intentional?

"-cpu host" should be sufficient and should not hit the error.

why did you get "guest CPU physical bits (48)"?

> Thanks,
> 
> Paolo
> 
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   target/i386/host-cpu.c | 2 +-
>>   target/i386/host-cpu.h | 1 +
>>   target/i386/kvm/tdx.c  | 8 ++++++++
>>   3 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c
>> index 3e4e85e729c8..8a15af458b05 100644
>> --- a/target/i386/host-cpu.c
>> +++ b/target/i386/host-cpu.c
>> @@ -15,7 +15,7 @@
>>   #include "system/system.h"
>>
>>   /* Note: Only safe for use on x86(-64) hosts */
>> -static uint32_t host_cpu_phys_bits(void)
>> +uint32_t host_cpu_phys_bits(void)
>>   {
>>       uint32_t eax;
>>       uint32_t host_phys_bits;
>> diff --git a/target/i386/host-cpu.h b/target/i386/host-cpu.h
>> index 6a9bc918baa4..b97ec01c9bec 100644
>> --- a/target/i386/host-cpu.h
>> +++ b/target/i386/host-cpu.h
>> @@ -10,6 +10,7 @@
>>   #ifndef HOST_CPU_H
>>   #define HOST_CPU_H
>>
>> +uint32_t host_cpu_phys_bits(void);
>>   void host_cpu_instance_init(X86CPU *cpu);
>>   void host_cpu_max_instance_init(X86CPU *cpu);
>>   bool host_cpu_realizefn(CPUState *cs, Error **errp);
>> diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
>> index bb75eb06dad9..c906a76c4c0e 100644
>> --- a/target/i386/kvm/tdx.c
>> +++ b/target/i386/kvm/tdx.c
>> @@ -24,6 +24,7 @@
>>
>>   #include "cpu.h"
>>   #include "cpu-internal.h"
>> +#include "host-cpu.h"
>>   #include "hw/i386/e820_memory_layout.h"
>>   #include "hw/i386/x86.h"
>>   #include "hw/i386/tdvf.h"
>> @@ -838,6 +839,13 @@ static int tdx_check_features(X86ConfidentialGuest *cg, CPUState *cs)
>>           return -1;
>>       }
>>
>> +    if (cpu->phys_bits != host_cpu_phys_bits()) {
>> +        error_report("TDX requires guest CPU physical bits (%u) "
>> +                     "to match host CPU physical bits (%u)",
>> +                     cpu->phys_bits, host_cpu_phys_bits());
>> +        exit(1);
>> +    }
>> +
>>       return 0;
>>   }
>>
>> --
>> 2.34.1
>>
> 
>
Paolo Bonzini Feb. 3, 2025, 6:15 p.m. UTC | #3 
On 2/2/25 15:39, Xiaoyao Li wrote:
> On 2/1/2025 2:27 AM, Paolo Bonzini wrote:
>> On Fri, Jan 24, 2025 at 2:40 PM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>>>
>>> For TDX guest, the phys_bits is not configurable and can only be
>>> host/native value.
>>>
>>> Validate phys_bits inside tdx_check_features().
>>
>> Hi Xiaoyao,
>>
>> to avoid
>>
>> qemu-kvm: TDX requires guest CPU physical bits (48) to match host CPU
>> physical bits (52)
>>
>> I need options like
>>
>> -cpu host,phys-bits=52,guest-phys-bits=52,host-phys-bits-limit=52,- 
>> kvm-asyncpf-int
>>
>> to start a TDX guest, is that intentional?
> 
> "-cpu host" should be sufficient and should not hit the error.
> 
> why did you get "guest CPU physical bits (48)"?

Nevermind - I got it from the RHEL machine types.  The fix (which does not have
to be included upstream, though I would not complain) is

diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
index e0ab41bcb7b..7aca8219405 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -444,6 +444,7 @@ static void tdx_cpu_instance_init(X86ConfidentialGuest *cg, CPUState *cpu)
      X86CPU *x86cpu = X86_CPU(cpu);
  
      object_property_set_bool(OBJECT(cpu), "pmu", false, &error_abort);
+    object_property_set_bool(OBJECT(cpu), "host-phys-bits-limit", 0, &error_abort);
  
      /* invtsc is fixed1 for TD guest */
      object_property_set_bool(OBJECT(cpu), "invtsc", true, &error_abort);

but it also needs the patch at
https://lore.kernel.org/qemu-devel/20250203114132.259155-1-pbonzini@redhat.com/T/.

With these two changes, QEMU accepts "-cpu host" just fine.

Paolo
diff mbox series

Patch

diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c
index 3e4e85e729c8..8a15af458b05 100644
--- a/target/i386/host-cpu.c
+++ b/target/i386/host-cpu.c
@@ -15,7 +15,7 @@ 
 #include "system/system.h"
 
 /* Note: Only safe for use on x86(-64) hosts */
-static uint32_t host_cpu_phys_bits(void)
+uint32_t host_cpu_phys_bits(void)
 {
     uint32_t eax;
     uint32_t host_phys_bits;
diff --git a/target/i386/host-cpu.h b/target/i386/host-cpu.h
index 6a9bc918baa4..b97ec01c9bec 100644
--- a/target/i386/host-cpu.h
+++ b/target/i386/host-cpu.h
@@ -10,6 +10,7 @@ 
 #ifndef HOST_CPU_H
 #define HOST_CPU_H
 
+uint32_t host_cpu_phys_bits(void);
 void host_cpu_instance_init(X86CPU *cpu);
 void host_cpu_max_instance_init(X86CPU *cpu);
 bool host_cpu_realizefn(CPUState *cs, Error **errp);
diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
index bb75eb06dad9..c906a76c4c0e 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -24,6 +24,7 @@ 
 
 #include "cpu.h"
 #include "cpu-internal.h"
+#include "host-cpu.h"
 #include "hw/i386/e820_memory_layout.h"
 #include "hw/i386/x86.h"
 #include "hw/i386/tdvf.h"
@@ -838,6 +839,13 @@  static int tdx_check_features(X86ConfidentialGuest *cg, CPUState *cs)
         return -1;
     }
 
+    if (cpu->phys_bits != host_cpu_phys_bits()) {
+        error_report("TDX requires guest CPU physical bits (%u) "
+                     "to match host CPU physical bits (%u)",
+                     cpu->phys_bits, host_cpu_phys_bits());
+        exit(1);
+    }
+
     return 0;
 }