For TDX guest, KVM doesn't allow phys_bits configuration and the
phys_bits can only be native/host value.

Add the logic to set cpu->phys_bits to host value when user doesn't
give a explicit one and error out when user desires a different one
than host value.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
Changes in v6:
 - new patches;
---
 target/i386/host-cpu.c |  2 +-
 target/i386/host-cpu.h |  1 +
 target/i386/kvm/tdx.c  | 17 +++++++++++++++++
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c
index 03b9d1b169a5..e2c59e5ae288 100644
--- a/target/i386/host-cpu.c
+++ b/target/i386/host-cpu.c
@@ -15,7 +15,7 @@
 #include "sysemu/sysemu.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 61fb1f184149..289722a129ce 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -23,6 +23,8 @@
 
 #include <linux/kvm_para.h>
 
+#include "cpu.h"
+#include "host-cpu.h"
 #include "hw/i386/e820_memory_layout.h"
 #include "hw/i386/x86.h"
 #include "hw/i386/tdvf.h"
@@ -389,6 +391,20 @@ static void tdx_cpu_instance_init(X86ConfidentialGuest *cg, CPUState *cpu)
     object_property_set_bool(OBJECT(cpu), "pmu", false, &error_abort);
 }
 
+static void tdx_cpu_realizefn(X86ConfidentialGuest *cg, CPUState *cs,
+                              Error **errp)
+{
+    X86CPU *cpu = X86_CPU(cs);
+    uint32_t host_phys_bits = host_cpu_phys_bits();
+
+    if (!cpu->phys_bits) {
+        cpu->phys_bits = host_phys_bits;
+    } else if (cpu->phys_bits != host_phys_bits) {
+        error_setg(errp, "TDX only supports host physical bits (%u)",
+                   host_phys_bits);
+    }
+}
+
 static int tdx_validate_attributes(TdxGuest *tdx, Error **errp)
 {
     if ((tdx->attributes & ~tdx_caps->supported_attrs)) {
@@ -733,4 +749,5 @@ static void tdx_guest_class_init(ObjectClass *oc, void *data)
     klass->kvm_init = tdx_kvm_init;
     x86_klass->kvm_type = tdx_kvm_type;
     x86_klass->cpu_instance_init = tdx_cpu_instance_init;
+    x86_klass->cpu_realizefn = tdx_cpu_realizefn;
 }
-- 
2.34.1

On 11/5/24 07:23, Xiaoyao Li wrote:
> +static void tdx_cpu_realizefn(X86ConfidentialGuest *cg, CPUState *cs,
> +                              Error **errp)
> +{
> +    X86CPU *cpu = X86_CPU(cs);
> +    uint32_t host_phys_bits = host_cpu_phys_bits();
> +
> +    if (!cpu->phys_bits) {
> +        cpu->phys_bits = host_phys_bits;
> +    } else if (cpu->phys_bits != host_phys_bits) {
> +        error_setg(errp, "TDX only supports host physical bits (%u)",
> +                   host_phys_bits);
> +    }
> +}

This should be already handled by host_cpu_realizefn(), which is reached 
via cpu_exec_realizefn().

Why is it needed earlier, but not as early as instance_init?  If 
absolutely needed I would do the assignment in patch 33, but I don't 
understand why it's necessary.

Either way, the check should be in tdx_check_features.

Paolo

>   static int tdx_validate_attributes(TdxGuest *tdx, Error **errp)
>   {
>       if ((tdx->attributes & ~tdx_caps->supported_attrs)) {
> @@ -733,4 +749,5 @@ static void tdx_guest_class_init(ObjectClass *oc, void *data)
>       klass->kvm_init = tdx_kvm_init;
>       x86_klass->kvm_type = tdx_kvm_type;
>       x86_klass->cpu_instance_init = tdx_cpu_instance_init;
> +    x86_klass->cpu_realizefn = tdx_cpu_realizefn;
>   }

On 11/5/2024 6:06 PM, Paolo Bonzini wrote:
> On 11/5/24 07:23, Xiaoyao Li wrote:
>> +static void tdx_cpu_realizefn(X86ConfidentialGuest *cg, CPUState *cs,
>> +                              Error **errp)
>> +{
>> +    X86CPU *cpu = X86_CPU(cs);
>> +    uint32_t host_phys_bits = host_cpu_phys_bits();
>> +
>> +    if (!cpu->phys_bits) {
>> +        cpu->phys_bits = host_phys_bits;
>> +    } else if (cpu->phys_bits != host_phys_bits) {
>> +        error_setg(errp, "TDX only supports host physical bits (%u)",
>> +                   host_phys_bits);
>> +    }
>> +}
> 
> This should be already handled by host_cpu_realizefn(), which is reached 
> via cpu_exec_realizefn().
> 
> Why is it needed earlier, but not as early as instance_init?  If 
> absolutely needed I would do the assignment in patch 33, but I don't 
> understand why it's necessary.

It's not called earlier but right after cpu_exec_realizefn().

Patch 33 adds x86_confidenetial_guest_cpu_realizefn() right after 
ecpu_exec_realizefn(). This patch implements the callback and gets 
called in x86_confidenetial_guest_cpu_realizefn() so it's called after
cpu_exec_realizefn().

The reason why host_cpu_realizefn() cannot satisfy is that for normal 
VMs, the check in cpu_exec_realizefn() is just a warning and QEMU does 
allow the user to configure the physical address bit other than host's 
value, and the configured value will be seen inside guest. i.e., "-cpu 
phys-bits=xx" where xx != host_value works for normal VMs.

But for TDX, KVM doesn't allow it and the value seen in TD guest is 
always the host value.  i.e., "-cpu phys-bits=xx" where xx != host_value 
doesn't work for TDX.

> Either way, the check should be in tdx_check_features.

Good idea. I will try to implement it in tdx_check_features()

> Paolo
> 
>>   static int tdx_validate_attributes(TdxGuest *tdx, Error **errp)
>>   {
>>       if ((tdx->attributes & ~tdx_caps->supported_attrs)) {
>> @@ -733,4 +749,5 @@ static void tdx_guest_class_init(ObjectClass *oc, 
>> void *data)
>>       klass->kvm_init = tdx_kvm_init;
>>       x86_klass->kvm_type = tdx_kvm_type;
>>       x86_klass->cpu_instance_init = tdx_cpu_instance_init;
>> +    x86_klass->cpu_realizefn = tdx_cpu_realizefn;
>>   }
> 

On 11/5/24 12:38, Xiaoyao Li wrote:
> On 11/5/2024 6:06 PM, Paolo Bonzini wrote:
>> On 11/5/24 07:23, Xiaoyao Li wrote:
>>> +static void tdx_cpu_realizefn(X86ConfidentialGuest *cg, CPUState *cs,
>>> +                              Error **errp)
>>> +{
>>> +    X86CPU *cpu = X86_CPU(cs);
>>> +    uint32_t host_phys_bits = host_cpu_phys_bits();
>>> +
>>> +    if (!cpu->phys_bits) {
>>> +        cpu->phys_bits = host_phys_bits;
>>> +    } else if (cpu->phys_bits != host_phys_bits) {
>>> +        error_setg(errp, "TDX only supports host physical bits (%u)",
>>> +                   host_phys_bits);
>>> +    }
>>> +}
>>
>> This should be already handled by host_cpu_realizefn(), which is 
>> reached via cpu_exec_realizefn().
>>
>> Why is it needed earlier, but not as early as instance_init?  If 
>> absolutely needed I would do the assignment in patch 33, but I don't 
>> understand why it's necessary.
> 
> It's not called earlier but right after cpu_exec_realizefn().
> 
> Patch 33 adds x86_confidenetial_guest_cpu_realizefn() right after 
> ecpu_exec_realizefn(). This patch implements the callback and gets 
> called in x86_confidenetial_guest_cpu_realizefn() so it's called after
> cpu_exec_realizefn().
> 
> The reason why host_cpu_realizefn() cannot satisfy is that for normal 
> VMs, the check in cpu_exec_realizefn() is just a warning and QEMU does 
> allow the user to configure the physical address bit other than host's 
> value, and the configured value will be seen inside guest. i.e., "-cpu 
> phys-bits=xx" where xx != host_value works for normal VMs.
> 
> But for TDX, KVM doesn't allow it and the value seen in TD guest is 
> always the host value.  i.e., "-cpu phys-bits=xx" where xx != host_value 
> doesn't work for TDX.
> 
>> Either way, the check should be in tdx_check_features.
> 
> Good idea. I will try to implement it in tdx_check_features()

Thanks, and I think there's no need to change cpu->phys_bits, either. 
So x86_confidenetial_guest_cpu_realizefn() should not be necessary.

Paolo