| Message ID | 20241125140535.4526-1-philmd@linaro.org |
|---|---|
| Headers | show |
| Series | hw/boards: Remove legacy MachineClass::pci_allow_0_address flag | expand |
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. thanks -- PMM
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.
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