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

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

[Philippe Mathieu-Daudé]

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.

IMO it would be cleaner to add the property
at the PCI_BUS level, but we only provide qdev
properties for *devices*, not *buses*. Meanwhile
we could register the property using plain QOM
API, but at this stage I thought it might be
over-engineering, so I restricted to property
to the GPEX device. After all, there is no
concern by similar PCI_BUS_EXTENDED_CONFIG_SPACE
use.

Philippe Mathieu-Daudé (8):
  hw/pci/pci_bus: Introduce PCIBusFlags::PCI_BUS_IO_ADDR0_ALLOWED
  hw/ppc/spapr_pci: Set PCI_BUS_IO_ADDR0_ALLOWED flag in host bridge
  hw/pci-host/gpex: Allow machines to set PCI_BUS_IO_ADDR0_ALLOWED flag
  hw/arm/virt: Set PCI_BUS_IO_ADDR0_ALLOWED flag on GPEX host bridge
  hw/arm/sbsa-ref: Set PCI_BUS_IO_ADDR0_ALLOWED flag on GPEX host bridge
  hw/riscv/virt: Remove pointless GPEX_HOST() cast
  hw/riscv/virt: Set PCI_BUS_IO_ADDR0_ALLOWED flag on GPEX host bridge
  hw/pci/pci: Remove legacy MachineClass::pci_allow_0_address flag

 include/hw/boards.h        |  1 -
 include/hw/pci-host/gpex.h |  1 +
 include/hw/pci/pci_bus.h   |  6 ++++++
 hw/arm/sbsa-ref.c          |  3 ++-
 hw/arm/virt.c              |  3 ++-
 hw/pci-host/gpex.c         |  6 ++++++
 hw/pci/pci.c               |  4 +---
 hw/ppc/spapr.c             |  1 -
 hw/ppc/spapr_pci.c         |  1 +
 hw/riscv/virt.c            | 23 +++++++++++------------
 10 files changed, 30 insertions(+), 19 deletions(-)

-- 
2.45.2

[PATCH-for-10.0 1/8] hw/pci/pci_bus: Introduce PCIBusFlags::PCI_BUS_IO_ADDR0_ALLOWED

[Philippe Mathieu-Daudé]

Some machines need PCI buses to allow access at BAR0.
Introduce the PCI_BUS_IO_ADDR0_ALLOWED flag and the
pci_bus_allows_io_addr0_access() helper, so machines
can set this flag during creation, similarly to how
they do with the PCI_BUS_EXTENDED_CONFIG_SPACE flag.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/pci/pci_bus.h | 6 ++++++
 hw/pci/pci.c             | 1 +
 2 files changed, 7 insertions(+)

diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index 2261312546..5c8b0d2887 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -26,6 +26,7 @@ enum PCIBusFlags {
     PCI_BUS_EXTENDED_CONFIG_SPACE                           = 0x0002,
     /* This is a CXL Type BUS */
     PCI_BUS_CXL                                             = 0x0004,
+    PCI_BUS_IO_ADDR0_ALLOWED                                = 0x0008,
 };
 
 #define PCI_NO_PASID UINT32_MAX
@@ -72,4 +73,9 @@ static inline bool pci_bus_allows_extended_config_space(PCIBus *bus)
     return !!(bus->flags & PCI_BUS_EXTENDED_CONFIG_SPACE);
 }
 
+static inline bool pci_bus_allows_io_addr0_access(PCIBus *bus)
+{
+    return !!(bus->flags & PCI_BUS_IO_ADDR0_ALLOWED);
+}
+
 #endif /* QEMU_PCI_BUS_H */
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 1416ae202c..de3f93646f 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1475,6 +1475,7 @@ pcibus_t pci_bar_address(PCIDevice *d,
     MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
     bool allow_0_address = mc->pci_allow_0_address;
 
+    allow_0_address |= pci_bus_allows_io_addr0_access(pci_get_bus(d));
     if (type & PCI_BASE_ADDRESS_SPACE_IO) {
         if (!(cmd & PCI_COMMAND_IO)) {
             return PCI_BAR_UNMAPPED;
-- 
2.45.2

[PATCH-for-10.0 2/8] hw/ppc/spapr_pci: Set PCI_BUS_IO_ADDR0_ALLOWED flag in host bridge

[Philippe Mathieu-Daudé]

See commit e402463073 ("pci: allow 0 address for PCI IO/MEM
regions"), all sPAPR machines set this flag.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ppc/spapr.c     | 1 -
 hw/ppc/spapr_pci.c | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 5c02037c56..8af56bd68a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4600,7 +4600,6 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->default_display = "std";
     mc->kvm_type = spapr_kvm_type;
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_SPAPR_PCI_HOST_BRIDGE);
-    mc->pci_allow_0_address = true;
     assert(!mc->get_hotplug_handler);
     mc->get_hotplug_handler = spapr_get_hotplug_handler;
     hc->pre_plug = spapr_machine_device_pre_plug;
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7e24084673..16f3a18d2e 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1888,6 +1888,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
                                 PCI_DEVFN(0, 0), PCI_NUM_PINS,
                                 TYPE_PCI_BUS);
 
+    bus->flags |= PCI_BUS_IO_ADDR0_ALLOWED;
     /*
      * Despite resembling a vanilla PCI bus in most ways, the PAPR
      * para-virtualized PCI bus *does* permit PCI-E extended config
-- 
2.45.2

[PATCH-for-10.0 3/8] hw/pci-host/gpex: Allow machines to set PCI_BUS_IO_ADDR0_ALLOWED flag

[Philippe Mathieu-Daudé]

Expose the "allow-io-addr0-accesses" property so machines
using a GPEX host bridge can set this flag on the bus.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/pci-host/gpex.h | 1 +
 hw/pci-host/gpex.c         | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/include/hw/pci-host/gpex.h b/include/hw/pci-host/gpex.h
index dce883573b..8c990bff5f 100644
--- a/include/hw/pci-host/gpex.h
+++ b/include/hw/pci-host/gpex.h
@@ -64,6 +64,7 @@ struct GPEXHost {
     int irq_num[GPEX_NUM_IRQS];
 
     bool allow_unmapped_accesses;
+    bool allow_io_addr0_accesses;
 
     struct GPEXConfig gpex_cfg;
 };
diff --git a/hw/pci-host/gpex.c b/hw/pci-host/gpex.c
index e9cf455bf5..635467016f 100644
--- a/hw/pci-host/gpex.c
+++ b/hw/pci-host/gpex.c
@@ -32,6 +32,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "hw/irq.h"
+#include "hw/pci/pci_bus.h"
 #include "hw/pci-host/gpex.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
@@ -136,6 +137,9 @@ static void gpex_host_realize(DeviceState *dev, Error **errp)
     pci->bus = pci_register_root_bus(dev, "pcie.0", gpex_set_irq,
                                      pci_swizzle_map_irq_fn, s, &s->io_mmio,
                                      &s->io_ioport, 0, 4, TYPE_PCIE_BUS);
+    if (s->allow_io_addr0_accesses) {
+        pci->bus->flags |= PCI_BUS_IO_ADDR0_ALLOWED;
+    }
 
     pci_bus_set_route_irq_fn(pci->bus, gpex_route_intx_pin_to_irq);
     qdev_realize(DEVICE(&s->gpex_root), BUS(pci->bus), &error_fatal);
@@ -154,6 +158,8 @@ static Property gpex_host_properties[] = {
      */
     DEFINE_PROP_BOOL("allow-unmapped-accesses", GPEXHost,
                      allow_unmapped_accesses, true),
+    DEFINE_PROP_BOOL("allow-io-addr0-accesses", GPEXHost,
+                     allow_io_addr0_accesses, false),
     DEFINE_PROP_UINT64(PCI_HOST_ECAM_BASE, GPEXHost, gpex_cfg.ecam.base, 0),
     DEFINE_PROP_SIZE(PCI_HOST_ECAM_SIZE, GPEXHost, gpex_cfg.ecam.size, 0),
     DEFINE_PROP_UINT64(PCI_HOST_PIO_BASE, GPEXHost, gpex_cfg.pio.base, 0),
-- 
2.45.2

Re: [PATCH-for-10.0 3/8] hw/pci-host/gpex: Allow machines to set PCI_BUS_IO_ADDR0_ALLOWED flag

[Peter Maydell]

On Mon, 25 Nov 2024 at 14:06, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Expose the "allow-io-addr0-accesses" property so machines
> using a GPEX host bridge can set this flag on the bus.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/pci-host/gpex.h | 1 +
>  hw/pci-host/gpex.c         | 6 ++++++
>  2 files changed, 7 insertions(+)

Given the way the gpex controller handles its MMIO regions,
I rather suspect that none of its users really need to set
"no address 0 accesses", though of course this is a behaviour
change for some machines so would need testing.

-- PMM

[PATCH-for-10.0 4/8] hw/arm/virt: Set PCI_BUS_IO_ADDR0_ALLOWED flag on GPEX host bridge

[Philippe Mathieu-Daudé]

See commit 74de8c3568 ("hw/arm/virt: Allow zero address for PCI
IO space"), all ARM Virt machines set this flag.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/virt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 1a381e9a2b..773df5f04b 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1510,6 +1510,8 @@ static void create_pcie(VirtMachineState *vms)
     MachineClass *mc = MACHINE_GET_CLASS(ms);
 
     dev = qdev_new(TYPE_GPEX_HOST);
+    object_property_set_bool(OBJECT(dev), "allow-io-addr0-accesses",
+                             true, &error_fatal);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
     ecam_id = VIRT_ECAM_ID(vms->highmem_ecam);
@@ -3124,7 +3126,6 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
 #endif
     mc->block_default_type = IF_VIRTIO;
     mc->no_cdrom = 1;
-    mc->pci_allow_0_address = true;
     /* We know we will never create a pre-ARMv7 CPU which needs 1K pages */
     mc->minimum_page_bits = 12;
     mc->possible_cpu_arch_ids = virt_possible_cpu_arch_ids;
-- 
2.45.2

[PATCH-for-10.0 5/8] hw/arm/sbsa-ref: Set PCI_BUS_IO_ADDR0_ALLOWED flag on GPEX host bridge

[Philippe Mathieu-Daudé]

All SBSA-Ref machines set this flag in MachineClass.
Directly set it to the GPEX bridge.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/sbsa-ref.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index e3195d5449..988a0a236e 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -647,6 +647,8 @@ static void create_pcie(SBSAMachineState *sms)
     int i;
 
     dev = qdev_new(TYPE_GPEX_HOST);
+    object_property_set_bool(OBJECT(dev), "allow-io-addr0-accesses",
+                             true, &error_fatal);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
     /* Map ECAM space */
@@ -896,7 +898,6 @@ static void sbsa_ref_class_init(ObjectClass *oc, void *data)
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("neoverse-n2");
     mc->valid_cpu_types = valid_cpu_types;
     mc->max_cpus = 512;
-    mc->pci_allow_0_address = true;
     mc->minimum_page_bits = 12;
     mc->block_default_type = IF_IDE;
     mc->no_cdrom = 1;
-- 
2.45.2

[PATCH-for-10.0 6/8] hw/riscv/virt: Remove pointless GPEX_HOST() cast

[Philippe Mathieu-Daudé]

No need to QOM-cast twice, since the intermediate value
is not used.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/riscv/virt.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 45a8c4f819..2feb851f15 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1140,23 +1140,21 @@ static inline DeviceState *gpex_pcie_init(MemoryRegion *sys_mem,
     dev = qdev_new(TYPE_GPEX_HOST);
 
     /* Set GPEX object properties for the virt machine */
-    object_property_set_uint(OBJECT(GPEX_HOST(dev)), PCI_HOST_ECAM_BASE,
+    object_property_set_uint(OBJECT(dev), PCI_HOST_ECAM_BASE,
                             ecam_base, NULL);
-    object_property_set_int(OBJECT(GPEX_HOST(dev)), PCI_HOST_ECAM_SIZE,
+    object_property_set_int(OBJECT(dev), PCI_HOST_ECAM_SIZE,
                             ecam_size, NULL);
-    object_property_set_uint(OBJECT(GPEX_HOST(dev)),
-                             PCI_HOST_BELOW_4G_MMIO_BASE,
+    object_property_set_uint(OBJECT(dev), PCI_HOST_BELOW_4G_MMIO_BASE,
                              mmio_base, NULL);
-    object_property_set_int(OBJECT(GPEX_HOST(dev)), PCI_HOST_BELOW_4G_MMIO_SIZE,
+    object_property_set_int(OBJECT(dev), PCI_HOST_BELOW_4G_MMIO_SIZE,
                             mmio_size, NULL);
-    object_property_set_uint(OBJECT(GPEX_HOST(dev)),
-                             PCI_HOST_ABOVE_4G_MMIO_BASE,
+    object_property_set_uint(OBJECT(dev), PCI_HOST_ABOVE_4G_MMIO_BASE,
                              high_mmio_base, NULL);
-    object_property_set_int(OBJECT(GPEX_HOST(dev)), PCI_HOST_ABOVE_4G_MMIO_SIZE,
+    object_property_set_int(OBJECT(dev), PCI_HOST_ABOVE_4G_MMIO_SIZE,
                             high_mmio_size, NULL);
-    object_property_set_uint(OBJECT(GPEX_HOST(dev)), PCI_HOST_PIO_BASE,
+    object_property_set_uint(OBJECT(dev), PCI_HOST_PIO_BASE,
                             pio_base, NULL);
-    object_property_set_int(OBJECT(GPEX_HOST(dev)), PCI_HOST_PIO_SIZE,
+    object_property_set_int(OBJECT(dev), PCI_HOST_PIO_SIZE,
                             pio_size, NULL);
 
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
@@ -1189,7 +1187,7 @@ static inline DeviceState *gpex_pcie_init(MemoryRegion *sys_mem,
         gpex_set_irq_num(GPEX_HOST(dev), i, PCIE_IRQ + i);
     }
 
-    GPEX_HOST(dev)->gpex_cfg.bus = PCI_HOST_BRIDGE(GPEX_HOST(dev))->bus;
+    GPEX_HOST(dev)->gpex_cfg.bus = PCI_HOST_BRIDGE(dev)->bus;
     return dev;
 }
 
-- 
2.45.2

Re: [PATCH-for-10.0 6/8] hw/riscv/virt: Remove pointless GPEX_HOST() cast

[Thomas Huth]

On 25/11/2024 15.05, Philippe Mathieu-Daudé wrote:
> No need to QOM-cast twice, since the intermediate value
> is not used.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/riscv/virt.c | 20 +++++++++-----------
>   1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 45a8c4f819..2feb851f15 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1140,23 +1140,21 @@ static inline DeviceState *gpex_pcie_init(MemoryRegion *sys_mem,
>       dev = qdev_new(TYPE_GPEX_HOST);
>   
>       /* Set GPEX object properties for the virt machine */
> -    object_property_set_uint(OBJECT(GPEX_HOST(dev)), PCI_HOST_ECAM_BASE,
> +    object_property_set_uint(OBJECT(dev), PCI_HOST_ECAM_BASE,
>                               ecam_base, NULL);
> -    object_property_set_int(OBJECT(GPEX_HOST(dev)), PCI_HOST_ECAM_SIZE,
> +    object_property_set_int(OBJECT(dev), PCI_HOST_ECAM_SIZE,
>                               ecam_size, NULL);
> -    object_property_set_uint(OBJECT(GPEX_HOST(dev)),
> -                             PCI_HOST_BELOW_4G_MMIO_BASE,
> +    object_property_set_uint(OBJECT(dev), PCI_HOST_BELOW_4G_MMIO_BASE,
>                                mmio_base, NULL);
> -    object_property_set_int(OBJECT(GPEX_HOST(dev)), PCI_HOST_BELOW_4G_MMIO_SIZE,
> +    object_property_set_int(OBJECT(dev), PCI_HOST_BELOW_4G_MMIO_SIZE,
>                               mmio_size, NULL);
> -    object_property_set_uint(OBJECT(GPEX_HOST(dev)),
> -                             PCI_HOST_ABOVE_4G_MMIO_BASE,
> +    object_property_set_uint(OBJECT(dev), PCI_HOST_ABOVE_4G_MMIO_BASE,
>                                high_mmio_base, NULL);
> -    object_property_set_int(OBJECT(GPEX_HOST(dev)), PCI_HOST_ABOVE_4G_MMIO_SIZE,
> +    object_property_set_int(OBJECT(dev), PCI_HOST_ABOVE_4G_MMIO_SIZE,
>                               high_mmio_size, NULL);
> -    object_property_set_uint(OBJECT(GPEX_HOST(dev)), PCI_HOST_PIO_BASE,
> +    object_property_set_uint(OBJECT(dev), PCI_HOST_PIO_BASE,
>                               pio_base, NULL);
> -    object_property_set_int(OBJECT(GPEX_HOST(dev)), PCI_HOST_PIO_SIZE,
> +    object_property_set_int(OBJECT(dev), PCI_HOST_PIO_SIZE,
>                               pio_size, NULL);
>   
>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> @@ -1189,7 +1187,7 @@ static inline DeviceState *gpex_pcie_init(MemoryRegion *sys_mem,
>           gpex_set_irq_num(GPEX_HOST(dev), i, PCIE_IRQ + i);
>       }
>   
> -    GPEX_HOST(dev)->gpex_cfg.bus = PCI_HOST_BRIDGE(GPEX_HOST(dev))->bus;
> +    GPEX_HOST(dev)->gpex_cfg.bus = PCI_HOST_BRIDGE(dev)->bus;
>       return dev;
>   }
>   

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH-for-10.0 7/8] hw/riscv/virt: Set PCI_BUS_IO_ADDR0_ALLOWED flag on GPEX host bridge

[Philippe Mathieu-Daudé]

See commit acead54c78 ("riscv: virt: Allow PCI address 0")
all RISCV Virt machines set this flag.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/riscv/virt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 2feb851f15..4e1ce3a423 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1156,6 +1156,8 @@ static inline DeviceState *gpex_pcie_init(MemoryRegion *sys_mem,
                             pio_base, NULL);
     object_property_set_int(OBJECT(dev), PCI_HOST_PIO_SIZE,
                             pio_size, NULL);
+    object_property_set_bool(OBJECT(dev), "allow-io-addr0-accesses",
+                             true, &error_fatal);
 
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
@@ -1803,7 +1805,6 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     mc->default_cpu_type = TYPE_RISCV_CPU_BASE;
     mc->block_default_type = IF_VIRTIO;
     mc->no_cdrom = 1;
-    mc->pci_allow_0_address = true;
     mc->possible_cpu_arch_ids = riscv_numa_possible_cpu_arch_ids;
     mc->cpu_index_to_instance_props = riscv_numa_cpu_index_to_props;
     mc->get_default_cpu_node_id = riscv_numa_get_default_cpu_node_id;
-- 
2.45.2

[PATCH-for-10.0 8/8] hw/pci/pci: Remove legacy MachineClass::pci_allow_0_address flag

[Philippe Mathieu-Daudé]

All users of this flag now set the PCI_BUS_IO_ADDR0_ALLOWED
PCIBusFlags. Since there are no more users, remove the field
in MachineClass.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/boards.h | 1 -
 hw/pci/pci.c        | 5 +----
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 36fbb9b59d..66fb2eddd3 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -284,7 +284,6 @@ struct MachineClass {
         no_floppy:1,
         no_cdrom:1,
         no_sdcard:1,
-        pci_allow_0_address:1,
         legacy_fw_cfg_order:1;
     bool is_default;
     const char *default_machine_opts;
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index de3f93646f..d27166dd75 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -45,7 +45,6 @@
 #include "hw/pci/msi.h"
 #include "hw/pci/msix.h"
 #include "hw/hotplug.h"
-#include "hw/boards.h"
 #include "qapi/error.h"
 #include "qemu/cutils.h"
 #include "pci-internal.h"
@@ -1472,10 +1471,8 @@ pcibus_t pci_bar_address(PCIDevice *d,
 {
     pcibus_t new_addr, last_addr;
     uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
-    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
-    bool allow_0_address = mc->pci_allow_0_address;
+    bool allow_0_address = pci_bus_allows_io_addr0_access(pci_get_bus(d));
 
-    allow_0_address |= pci_bus_allows_io_addr0_access(pci_get_bus(d));
     if (type & PCI_BASE_ADDRESS_SPACE_IO) {
         if (!(cmd & PCI_COMMAND_IO)) {
             return PCI_BAR_UNMAPPED;
-- 
2.45.2

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

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

[Philippe Mathieu-Daudé]

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.

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