Re: [PATCH v6 34/60] i386/tdx: implement tdx_cpu_realizefn()

Xiaoyao Li
2025-01-14T08:52:37Z

On 12/13/2024 6:04 AM, Ira Weiny wrote:
> On Tue, Nov 05, 2024 at 12:53:25PM +0100, Paolo Bonzini wrote:
>> 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()
>
> Is there any reason the TDX code can't just force cpu->host_phys_bits to true?

That doesn't work for all the cases. e.g., when user set
"host-phys-bits-limit" to a smaller value. For this case, QEMU still
needs to validate the final cpu->phys_bits.

Of course, we can force host_phys_bits to true for TDX, and warn and
exit when user set "host-phys-bits-limit" to a smaller value than host
value.

But I prefer the current direction to check cpu->phys_bits directly,
which is straightforward.

>>
>> Thanks, and I think there's no need to change cpu->phys_bits, either. So
>> x86_confidenetial_guest_cpu_realizefn() should not be necessary.
>
> I was going to comment that patch 33 should be squashed here but better to just
> drop it.
>
> Ira
>
>>
>> Paolo
>>