Re: [PATCH v3] x86: add etc/phys-bits fw_cfg file

[Michael S. Tsirkin]

On Thu, Sep 22, 2022 at 11:37:10AM +0200, Gerd Hoffmann wrote:
> On Thu, Sep 22, 2022 at 04:55:16AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Sep 22, 2022 at 10:43:56AM +0200, Gerd Hoffmann wrote:
> > > In case phys bits are functional and can be used by the guest (aka
> > > host-phys-bits=on) add a fw_cfg file carrying the value.  This can
> > > be used by the guest firmware for address space configuration.
> > > 
> > > This is only enabled for 7.2+ machine types for live migration
> > > compatibility reasons.
> > > 
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > 
> > I'm curious why you decided to switch from a cpuid flag to fw cfg.
> 
> The kernel people didn't like the cpuid approach.
> 
> > I guess firmware reads fw cfg anyway.
> 
> Correct.
> 
> > But would the guest kernel then need to load a fw cfg driver very
> > early to detect this, too?
> 
> Nope, the guest kernel can just work with the address space layout
> created by the firmware.  The firmware can for example reserve a
> larger 64-bit mmio window in case there is enough address space for
> that.  So it programs the bridge windows etc accordingly, qemu
> generates matching acpi tables and the kernel picks up the changes
> via _CRS.
> 
> > > +void fw_cfg_phys_bits(FWCfgState *fw_cfg)
> > > +{
> > > +    X86CPU *cpu = X86_CPU(first_cpu);
> > > +    uint64_t phys_bits = cpu->phys_bits;
> > > +
> > > +    if (cpu->host_phys_bits) {
> > > +        fw_cfg_add_file(fw_cfg, "etc/phys-bits",
> > > +                        g_memdup2(&phys_bits, sizeof(phys_bits)),
> > > +                        sizeof(phys_bits));
> > > +    }
> > > +}
> > 
> > So, this allows a lot of flexibility, any phys_bits value at all can now
> > be used. Do you expect a use-case for such a flexible mechanism?  If
> > this ends up merely repeating CPUID at all times then we are just
> > creating confusion.
> 
> Yes, it'll just repeat CPUID.  Advantage is that the guest gets the
> information it needs right away.
> 
> Alternatively I could create a "etc/reliable-phys-bits" bool.
> The firmware must consult both fw_cfg and cpuid then.
> 
> take care,
>   Gerd

It might not be too bad if we actually allow these two to be different
theoretically (even if unused for now).
This is up to you. But my point is, if we do let's document what is the
expected behaviour if fw cfg and CPUID differ.



-- 
MST