Re: [Qemu-devel] [RFC PATCH qemu] virtio-pci: Replace modern_as with direct access to modern_bar

[Paolo Bonzini]

On 22/09/2017 17:12, Alexey Kardashevskiy wrote:
> The modern bar is accessed now via yet another address space created just
> for that purpose and it does not really need FlatView and dispatch tree
> as it has a single memory region so it is just a waste of memory. Things
> get even worse when there are dozens or hundreds of virtio-pci devices -
> since these address spaces are global, changing any of them triggers
> rebuilding all address spaces.
> 
> This replaces indirect accesses to the modern BAR with a simple lookup
> and direct calls to memory_region_dispatch_read/write.
> 
> This is expected to save lots of memory at boot time after applying:
> [Qemu-devel] [PULL 00/32] Misc changes for 2017-09-22
> 
> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> ---

There used to be this patch:
https://marc.info/?l=linux-virtualization&m=142363659531278&q=raw

> What is the easiest way to test it? Git history suggests that only
> SeaBIOS supports this, is this still correct?
> 
> What is best to do if MR is not found (and is it possible in practice)?
> assert or redirect to unassigned memory?

Do nothing at all, I think.

I haven't thought much about the endian switches, but it looks good
apart from that.

Paolo

> ---
>  hw/virtio/virtio-pci.h | 17 +++++++-----
>  hw/virtio/virtio-pci.c | 71 
> ++++++++++++++++++++++++++++----------------------
>  2 files changed, 50 insertions(+), 38 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index 69f5959623..12d3a90686 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -155,15 +155,18 @@ typedef struct VirtIOPCIQueue {
>  struct VirtIOPCIProxy {
>      PCIDevice pci_dev;
>      MemoryRegion bar;
> -    VirtIOPCIRegion common;
> -    VirtIOPCIRegion isr;
> -    VirtIOPCIRegion device;
> -    VirtIOPCIRegion notify;
> -    VirtIOPCIRegion notify_pio;
> +    union {
> +        struct {
> +            VirtIOPCIRegion common;
> +            VirtIOPCIRegion isr;
> +            VirtIOPCIRegion device;
> +            VirtIOPCIRegion notify;
> +            VirtIOPCIRegion notify_pio;
> +        };
> +        VirtIOPCIRegion regs[5];
> +    };
>      MemoryRegion modern_bar;
>      MemoryRegion io_bar;
> -    MemoryRegion modern_cfg;
> -    AddressSpace modern_as;
>      uint32_t legacy_io_bar_idx;
>      uint32_t msix_bar_idx;
>      uint32_t modern_io_bar_idx;
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 8b0d6b69cd..0fcb8589f7 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -545,6 +545,24 @@ static const MemoryRegionOps virtio_pci_config_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> +static MemoryRegion *virtio_address_space_lookup(VirtIOPCIProxy *proxy,
> +                                                 hwaddr *off, int len)
> +{
> +    int i;
> +    VirtIOPCIRegion *reg;
> +
> +    for (i = 0; i < ARRAY_SIZE(proxy->regs); ++i) {
> +        reg = &proxy->regs[i];
> +        if (*off >= reg->offset &&
> +            *off + len <= reg->offset + reg->size) {
> +            *off -= reg->offset;
> +            return &reg->mr;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
>  /* Below are generic functions to do memcpy from/to an address space,
>   * without byteswaps, with input validation.
>   *
> @@ -558,63 +576,68 @@ static const MemoryRegionOps virtio_pci_config_ops = {
>   * Note: host pointer must be aligned.
>   */
>  static
> -void virtio_address_space_write(AddressSpace *as, hwaddr addr,
> +void virtio_address_space_write(VirtIOPCIProxy *proxy, hwaddr addr,
>                                  const uint8_t *buf, int len)
>  {
> -    uint32_t val;
> +    uint64_t val;
> +    MemoryRegion *mr;
>  
>      /* address_space_* APIs assume an aligned address.
>       * As address is under guest control, handle illegal values.
>       */
>      addr &= ~(len - 1);
>  
> +    mr = virtio_address_space_lookup(proxy, &addr, len);
> +    assert(mr);
> +
>      /* Make sure caller aligned buf properly */
>      assert(!(((uintptr_t)buf) & (len - 1)));
>  
>      switch (len) {
>      case 1:
>          val = pci_get_byte(buf);
> -        address_space_stb(as, addr, val, MEMTXATTRS_UNSPECIFIED, NULL);
>          break;
>      case 2:
> -        val = pci_get_word(buf);
> -        address_space_stw_le(as, addr, val, MEMTXATTRS_UNSPECIFIED, NULL);
> +        val = cpu_to_le16(pci_get_word(buf));
>          break;
>      case 4:
> -        val = pci_get_long(buf);
> -        address_space_stl_le(as, addr, val, MEMTXATTRS_UNSPECIFIED, NULL);
> +        val = cpu_to_le32(pci_get_long(buf));
>          break;
>      default:
>          /* As length is under guest control, handle illegal values. */
> -        break;
> +        return;
>      }
> +    memory_region_dispatch_write(mr, addr, val, len, MEMTXATTRS_UNSPECIFIED);
>  }
>  
>  static void
> -virtio_address_space_read(AddressSpace *as, hwaddr addr, uint8_t *buf, int 
> len)
> +virtio_address_space_read(VirtIOPCIProxy *proxy, hwaddr addr,
> +                          uint8_t *buf, int len)
>  {
> -    uint32_t val;
> +    uint64_t val;
> +    MemoryRegion *mr;
>  
>      /* address_space_* APIs assume an aligned address.
>       * As address is under guest control, handle illegal values.
>       */
>      addr &= ~(len - 1);
>  
> +    mr = virtio_address_space_lookup(proxy, &addr, len);
> +    assert(mr);
> +
>      /* Make sure caller aligned buf properly */
>      assert(!(((uintptr_t)buf) & (len - 1)));
>  
> +    memory_region_dispatch_read(mr, addr, &val, len, MEMTXATTRS_UNSPECIFIED);
>      switch (len) {
>      case 1:
> -        val = address_space_ldub(as, addr, MEMTXATTRS_UNSPECIFIED, NULL);
>          pci_set_byte(buf, val);
>          break;
>      case 2:
> -        val = address_space_lduw_le(as, addr, MEMTXATTRS_UNSPECIFIED, NULL);
> -        pci_set_word(buf, val);
> +        pci_set_word(buf, le16_to_cpu(val));
>          break;
>      case 4:
> -        val = address_space_ldl_le(as, addr, MEMTXATTRS_UNSPECIFIED, NULL);
> -        pci_set_long(buf, val);
> +        pci_set_long(buf, le32_to_cpu(val));
>          break;
>      default:
>          /* As length is under guest control, handle illegal values. */
> @@ -650,8 +673,7 @@ static void virtio_write_config(PCIDevice *pci_dev, 
> uint32_t address,
>  
>          if (len == 1 || len == 2 || len == 4) {
>              assert(len <= sizeof cfg->pci_cfg_data);
> -            virtio_address_space_write(&proxy->modern_as, off,
> -                                       cfg->pci_cfg_data, len);
> +            virtio_address_space_write(proxy, off, cfg->pci_cfg_data, len);
>          }
>      }
>  }
> @@ -675,8 +697,7 @@ static uint32_t virtio_read_config(PCIDevice *pci_dev,
>  
>          if (len == 1 || len == 2 || len == 4) {
>              assert(len <= sizeof cfg->pci_cfg_data);
> -            virtio_address_space_read(&proxy->modern_as, off,
> -                                      cfg->pci_cfg_data, len);
> +            virtio_address_space_read(proxy, off, cfg->pci_cfg_data, len);
>          }
>      }
>  
> @@ -1783,15 +1804,6 @@ 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));
>  
> -    memory_region_init_alias(&proxy->modern_cfg,
> -                             OBJECT(proxy),
> -                             "virtio-pci-cfg",
> -                             &proxy->modern_bar,
> -                             0,
> -                             memory_region_size(&proxy->modern_bar));
> -
> -    address_space_init(&proxy->modern_as, &proxy->modern_cfg, 
> "virtio-pci-cfg-as");
> -
>      if (proxy->disable_legacy == ON_OFF_AUTO_AUTO) {
>          proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>      }
> @@ -1860,10 +1872,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev, 
> Error **errp)
>  
>  static void virtio_pci_exit(PCIDevice *pci_dev)
>  {
> -    VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
> -
>      msix_uninit_exclusive_bar(pci_dev);
> -    address_space_destroy(&proxy->modern_as);
>  }
>  
>  static void virtio_pci_reset(DeviceState *qdev)
>