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

Alexey Kardashevskiy posted 1 patch 6 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170922151208.24590-1-aik@ozlabs.ru
Test checkpatch passed
Test docker passed
Test s390x passed
There is a newer version of this series
hw/virtio/virtio-pci.h | 17 +++++++-----
hw/virtio/virtio-pci.c | 71 ++++++++++++++++++++++++++++----------------------
2 files changed, 50 insertions(+), 38 deletions(-)
[Qemu-devel] [RFC PATCH qemu] virtio-pci: Replace modern_as with direct access to modern_bar
Posted by Alexey Kardashevskiy 6 years, 6 months ago
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 <aik@ozlabs.ru>
---

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?

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


Re: [Qemu-devel] [RFC PATCH qemu] virtio-pci: Replace modern_as with direct access to modern_bar
Posted by Paolo Bonzini 6 years, 6 months ago
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 <aik@ozlabs.ru>
> ---

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


Re: [Qemu-devel] [RFC PATCH qemu] virtio-pci: Replace modern_as with direct access to modern_bar
Posted by Alexey Kardashevskiy 6 years, 6 months ago
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 <aik@ozlabs.ru>
>> ---
> 
> 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

Re: [Qemu-devel] [RFC PATCH qemu] virtio-pci: Replace modern_as with direct access to modern_bar
Posted by Paolo Bonzini 6 years, 6 months ago
On 03/10/2017 04:11, Alexey Kardashevskiy wrote:
>>> 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?

Yes, I guess so.

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

Yes, I was going to ping him about this patch too.

Paolo

Re: [Qemu-devel] [RFC PATCH qemu] virtio-pci: Replace modern_as with direct access to modern_bar
Posted by Alexey Kardashevskiy 6 years, 6 months ago
On 06/10/17 23:58, Paolo Bonzini wrote:
> On 03/10/2017 04:11, Alexey Kardashevskiy wrote:
>>>> 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?
> 
> Yes, I guess so.

easy, done :)

> 
>>> I haven't thought much about the endian switches, but it looks good
>>> apart from that.
>> What is next? Michael?
> 
> Yes, I was going to ping him about this patch too.
> 
> Paolo


-- 
Alexey