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

[Ira Weiny]

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?

> 
> 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
>