Re: [PATCH v4 1/2] kvm: add support for guest physical bits

[Paolo Bonzini]

On Wed, Mar 20, 2024 at 3:45 AM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
> If users pass configuration like "-cpu
> qemu64,phys-bits=52,host-phys-bits-limit=45", the cpu->guest_phys_bits
> will be set to 45. I think this is not what we want, though the usage
> seems insane.
>
> We can guard it as
>
>   if (cpu->host_phys_bits && cpu->host_phys_bits_limit &&
>       cpu->guest_phys_bits > cpu->host_phys_bits_limt)
> {
> }

> Simpler, we can guard with cpu->phys_bits like below, because
> cpu->host_phys_bits_limit is used to guard cpu->phys_bits in
> host_cpu_realizefn()
>
>   if (cpu->guest_phys_bits > cpu->phys_bits) {
>         cpu->guest_phys_bits = cpu->phys_bits;
> }
>
>
> with this resolved,
>
> Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

[oops sorry - I noticed now that this email was never sent, so I am
sending it for archival]

There are more issues:

1) for compatibility with older machine types, the GuestPhysAddrSize
should remain 0. One possibility is to have "-1" as "accelerator
default" and "0" as "show it as zero in CPUID".

2) a "guest-phys-bits is not user-configurable in 32 bit" error is
probably a good idea just like it does for cpu->phys_bits

3) I think the order of the patches makes more sense if the property
is added first and KVM is adjusted second.

I'll post a v5 myself (mostly because it has to include the creation
of 9.1 machine types).

Paolo

> > +    }
> > +}
> > +
> >   static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
> >   {
> >       X86CPU *cpu = X86_CPU(cs);
> >       CPUX86State *env = &cpu->env;
> > +    bool ret;
> >
> >       /*
> >        * The realize order is important, since x86_cpu_realize() checks if
> > @@ -50,7 +73,13 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
> >                                                      MSR_IA32_UCODE_REV);
> >           }
> >       }
> > -    return host_cpu_realizefn(cs, errp);
> > +    ret = host_cpu_realizefn(cs, errp);
> > +    if (!ret) {
> > +        return ret;
> > +    }
> > +
> > +    kvm_set_guest_phys_bits(cs);
> > +    return true;
> >   }
> >
> >   static bool lmce_supported(void)
>Ther