Re: [PATCH v2 1/1] virtio-pci: fix memory_region_find for VirtIOPCIRegion's MR

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Oct 09, 2024 at 05:58:27PM +0800, Gao Shiyuan wrote:
> As shown below, if a virtio PCI device is attached under a pci-bridge, the MR
> of VirtIOPCIRegion does not belong to any address space. So memory_region_find
> cannot be used to search for this MR.
> 
> Introduce the virtio-pci and pci_bridge address spaces to solve this problem.
> 
> Before:
> memory-region: pci_bridge_pci
>   0000000000000000-ffffffffffffffff (prio 0, i/o): pci_bridge_pci
>     00000000fe840000-00000000fe840fff (prio 1, i/o): virtio-net-pci-msix
>       00000000fe840000-00000000fe84003f (prio 0, i/o): msix-table
>       00000000fe840800-00000000fe840807 (prio 0, i/o): msix-pba
>     0000380000000000-0000380000003fff (prio 1, i/o): virtio-pci
>       0000380000000000-0000380000000fff (prio 0, i/o): virtio-pci-common-virtio-net
>       0000380000001000-0000380000001fff (prio 0, i/o): virtio-pci-isr-virtio-net
>       0000380000002000-0000380000002fff (prio 0, i/o): virtio-pci-device-virtio-net
>       0000380000003000-0000380000003fff (prio 0, i/o): virtio-pci-notify-virtio-net
> 
> After:
> address-space: virtio-pci-cfg-mem-as
>   0000380000000000-0000380000003fff (prio 1, i/o): virtio-pci
>     0000380000000000-0000380000000fff (prio 0, i/o): virtio-pci-common-virtio-net
>     0000380000001000-0000380000001fff (prio 0, i/o): virtio-pci-isr-virtio-net
>     0000380000002000-0000380000002fff (prio 0, i/o): virtio-pci-device-virtio-net
>     0000380000003000-0000380000003fff (prio 0, i/o): virtio-pci-notify-virtio-net
> 
> address-space: pci_bridge_pci_mem
>   0000000000000000-ffffffffffffffff (prio 0, i/o): pci_bridge_pci
>     00000000fe840000-00000000fe840fff (prio 1, i/o): virtio-net-pci-msix
>       00000000fe840000-00000000fe84003f (prio 0, i/o): msix-table
>       00000000fe840800-00000000fe840807 (prio 0, i/o): msix-pba
>     0000380000000000-0000380000003fff (prio 1, i/o): virtio-pci
>       0000380000000000-0000380000000fff (prio 0, i/o): virtio-pci-common-virtio-net
>       0000380000001000-0000380000001fff (prio 0, i/o): virtio-pci-isr-virtio-net
>       0000380000002000-0000380000002fff (prio 0, i/o): virtio-pci-device-virtio-net
>       0000380000003000-0000380000003fff (prio 0, i/o): virtio-pci-notify-virtio-net
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2576
> Fixes: ffa8a3e3b2e6 ("virtio-pci: Add lookup subregion of VirtIOPCIRegion MR")
>

no empty lines between headers pls.
 
> Signed-off-by: Gao Shiyuan <gaoshiyuan@baidu.com>
> Signed-off-by: Zuo Boqun <zuoboqun@baidu.com>
> Signed-off-by: Wang Liang <wangliang44@baidu.com>


Daniel can you pls confirm it fixes the bug you reported with bsd?

> ---
>  hw/pci/pci_bridge.c            | 4 ++++
>  hw/virtio/virtio-pci.c         | 5 +++++
>  include/hw/pci/pci_bridge.h    | 2 ++
>  include/hw/virtio/virtio-pci.h | 3 +++
>  4 files changed, 14 insertions(+)
> 
> v1 -> v2:
> * modify commit message
> * add address space for port I/O notification config region
> 
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index 6a4e38856d..099ea9e24e 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -380,9 +380,11 @@ void pci_bridge_initfn(PCIDevice *dev, const char *typename)
>      sec_bus->map_irq = br->map_irq ? br->map_irq : pci_swizzle_map_irq_fn;
>      sec_bus->address_space_mem = &br->address_space_mem;
>      memory_region_init(&br->address_space_mem, OBJECT(br), "pci_bridge_pci", UINT64_MAX);
> +    address_space_init(&br->as_mem, &br->address_space_mem, "pci_bridge_pci_mem");
>      sec_bus->address_space_io = &br->address_space_io;
>      memory_region_init(&br->address_space_io, OBJECT(br), "pci_bridge_io",
>                         4 * GiB);
> +    address_space_init(&br->as_io, &br->address_space_io, "pci_bridge_pci_io");
>      pci_bridge_region_init(br);
>      QLIST_INIT(&sec_bus->child);
>      QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);
> @@ -399,6 +401,8 @@ void pci_bridge_exitfn(PCIDevice *pci_dev)
>      PCIBridge *s = PCI_BRIDGE(pci_dev);
>      assert(QLIST_EMPTY(&s->sec_bus.child));
>      QLIST_REMOVE(&s->sec_bus, sibling);
> +    address_space_destroy(&s->as_mem);
> +    address_space_destroy(&s->as_io);
>      pci_bridge_region_del(s, &s->windows);
>      pci_bridge_region_cleanup(s, &s->windows);
>      /* object_unparent() is called automatically during device deletion */
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 4d832fe845..1e862dd0df 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2057,6 +2057,7 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>          if (modern_pio) {
>              memory_region_init(&proxy->io_bar, OBJECT(proxy),
>                                 "virtio-pci-io", 0x4);
> +            address_space_init(&proxy->modern_cfg_io_as, &proxy->io_bar, "virtio-pci-cfg-io-as");
>  
>              pci_register_bar(&proxy->pci_dev, proxy->modern_io_bar_idx,
>                               PCI_BASE_ADDRESS_SPACE_IO, &proxy->io_bar);
> @@ -2180,6 +2181,8 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
>                         /* PCI BAR regions must be powers of 2 */
>                         pow2ceil(proxy->notify.offset + proxy->notify.size));
>  
> +    address_space_init(&proxy->modern_cfg_mem_as, &proxy->modern_bar, "virtio-pci-cfg-mem-as");
> +
>      if (proxy->disable_legacy == ON_OFF_AUTO_AUTO) {
>          proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>      }
> @@ -2275,6 +2278,8 @@ static void virtio_pci_exit(PCIDevice *pci_dev)
>          pci_is_express(pci_dev)) {
>          pcie_aer_exit(pci_dev);
>      }
> +    address_space_destroy(&proxy->modern_cfg_mem_as);
> +    address_space_destroy(&proxy->modern_cfg_io_as);
>  }
>  
>  static void virtio_pci_reset(DeviceState *qdev)
> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
> index 5cd452115a..bd12fbe4ef 100644
> --- a/include/hw/pci/pci_bridge.h
> +++ b/include/hw/pci/pci_bridge.h
> @@ -72,6 +72,8 @@ struct PCIBridge {
>       */
>      MemoryRegion address_space_mem;
>      MemoryRegion address_space_io;
> +    AddressSpace as_mem;
> +    AddressSpace as_io;
>  
>      PCIBridgeWindows windows;
>  
> diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
> index 9e67ba38c7..971c5fabd4 100644
> --- a/include/hw/virtio/virtio-pci.h
> +++ b/include/hw/virtio/virtio-pci.h
> @@ -147,6 +147,9 @@ struct VirtIOPCIProxy {
>      };
>      MemoryRegion modern_bar;
>      MemoryRegion io_bar;
> +    /* address space for VirtIOPCIRegions */
> +    AddressSpace modern_cfg_mem_as;
> +    AddressSpace modern_cfg_io_as;
>      uint32_t legacy_io_bar_idx;
>      uint32_t msix_bar_idx;
>      uint32_t modern_io_bar_idx;
> -- 
> 2.34.1