Re: [PATCH-for-10.0 0/8] hw/boards: Remove legacy MachineClass::pci_allow_0_address flag

[Peter Maydell]

On Mon, 25 Nov 2024 at 14:49, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 25/11/24 15:14, Peter Maydell wrote:
> > On Mon, 25 Nov 2024 at 14:06, Philippe Mathieu-Daudé <philmd@linaro.org> 
> > wrote:
> >>
> >> This series aims to remove a legacy field from
> >> MachineClass.
> >>
> >> Rather than a global exposed to all machines,
> >> use a pci-bus specific flag on each machine
> >> requiering it.
> >
> > Should this be a property of the PCI controller, rather
> > than on the PCI bus? Presumably on the machines that
> > don't allow a 0 PCI BAR address this happens because the
> > PCI controller refuses to map BARs at that address.
> >
> > TBH the commit message for e402463073 suggests to me
> > that "allow address zero" should be the default and
> > either specific machines should forbid it or else we
> > should figure out what goes wrong with them, if the
> > problem is caused by some bug in QEMU. The commit message's
> > mention of "fix PCI memory priorities" suggests to me
> > that this is a QEMU bug, and that it ought to be possible
> > to have the machine set up such that you *can* map the
> > BAR at address 0, it's merely invisible to the guest because
> > some other machine devices have higher priority and are
> > visible "on top" of it instead.
>
> You are probably right, the following comment ...:
>
>   pcibus_t pci_bar_address(PCIDevice *d,
>                            int reg, uint8_t type, pcibus_t size)
>   {
>       ...
>       /* NOTE: we do not support wrapping */
>       /* XXX: as we cannot support really dynamic
>          mappings, we handle specific values as invalid
>          mappings. */
>       if (last_addr <= new_addr || last_addr == PCI_BAR_UNMAPPED ||
>           (!allow_0_address && new_addr == 0)) {
>           return PCI_BAR_UNMAPPED;
>       }
>
> ... is from 20 years ago at the beginning of PCI in QEMU, commit
> 0ac32c8375 ("PCI interrupt support - PCI BIOS interrupt remapping
> - more accurate memory mapping - 'info pci' monitor command") which
> suggest the implementation is incomplete here.

See also this thread from 2015:

https://lore.kernel.org/qemu-devel/1444683308-30543-1-git-send-email-agordeev@redhat.com/T/#u

which includes:
 * me asking why this isn't a property on the PCI controller device :-)
 * MST confirming that this setting is only for buggy machine types
   that don't get the priorities correct when the BAR is configured
   so it overlaps something else
 * me expressing disappointment that we made the default for this
   flag be "this machine type is broken" rather than "this machine
   type is not broken", because of course almost every machine added
   since has left the flag at its default value
 * a now out-of-date list of possibly affected machine types that
   might actually need to mark themselves as "broken", or at least
   be tested to see what they do

thanks
-- PMM