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

[Alexey Kardashevskiy]

On 23/09/17 01:45, Paolo Bonzini wrote:
> 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.

Repost without asserts?


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

What is next? Michael?



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


-- 
Alexey