[PATCH 0/6] exec/memory: Rework some address and access size limits

[PATCH 0/6] exec/memory: Rework some address and access size limits

[Philippe Mathieu-Daudé]

These patches are extracted from a bigger series which
- remove generic ISA space, restricting it to the hw
  that really has it (mostly PCI-ISA bridges)
- allow QTest/GDB to use any address space
- make I/O address space target-specific (only X86 and
  AVR have a CPU connected to it)
- better handle Harvard architectures

Various patches only make sense if the AVR arch is merged,
so instead of waiting and keeping rebasing/testing, let's
share what is generic and might be worthwhile.

Currently the QMP/QTest commands only use the 1st CPU
address space, which has already been reported to limit
fuzzing/fault_injection/gdbstub.

I'll probably follow with the PCI-ISA bridge part, but
let's first see the feedback for this batch.

Regards,

Phil.

Philippe Mathieu-Daudé (6):
  target/s390x/mmu_helper: Use address_space_rw() in place
  hw/dma/rc4030: Use DMA address space to do DMA accesses
  hw/sd/allwinner-sdhost: Do DMA accesses via DMA address space
  exec/cpu-common: Do not restrict CPU to 32-bit memory access maximum
  exec: Restrict 32-bit CPUs to 32-bit address space
  memory: Use CPU register size as default access_size_max

 include/exec/cpu-common.h        |  4 ++--
 include/hw/sd/allwinner-sdhost.h |  4 ++++
 exec.c                           | 10 ++++++++-
 hw/dma/rc4030.c                  |  3 ++-
 hw/sd/allwinner-sdhost.c         | 36 ++++++++++++++++++++++++++------
 hw/usb/hcd-musb.c                | 12 +++++------
 memory.c                         |  2 +-
 target/s390x/mmu_helper.c        |  6 ++++--
 8 files changed, 58 insertions(+), 19 deletions(-)

-- 
2.21.3

[PATCH 1/6] target/s390x/mmu_helper: Use address_space_rw() in place

[Philippe Mathieu-Daudé]

In an effort to remove the cpu_physical_memory_rw() API,
update s390_cpu_virt_mem_rw() to use a more recent
address_space_rw() API.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/s390x/mmu_helper.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index 7d9f3059cd..632e8a8af4 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -529,8 +529,10 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
         /* Copy data by stepping through the area page by page */
         for (i = 0; i < nr_pages; i++) {
             currlen = MIN(len, TARGET_PAGE_SIZE - (laddr % TARGET_PAGE_SIZE));
-            cpu_physical_memory_rw(pages[i] | (laddr & ~TARGET_PAGE_MASK),
-                                   hostbuf, currlen, is_write);
+            address_space_rw(CPU(cpu)->as,
+                             pages[i] | (laddr & ~TARGET_PAGE_MASK),
+                             MEMTXATTRS_UNSPECIFIED,
+                             hostbuf, currlen, is_write);
             laddr += currlen;
             hostbuf += currlen;
             len -= currlen;
-- 
2.21.3

Re: [PATCH 1/6] target/s390x/mmu_helper: Use address_space_rw() in place

[David Hildenbrand]

On 31.05.20 19:54, Philippe Mathieu-Daudé wrote:
> In an effort to remove the cpu_physical_memory_rw() API,
> update s390_cpu_virt_mem_rw() to use a more recent
> address_space_rw() API.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/s390x/mmu_helper.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index 7d9f3059cd..632e8a8af4 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -529,8 +529,10 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
>          /* Copy data by stepping through the area page by page */
>          for (i = 0; i < nr_pages; i++) {
>              currlen = MIN(len, TARGET_PAGE_SIZE - (laddr % TARGET_PAGE_SIZE));
> -            cpu_physical_memory_rw(pages[i] | (laddr & ~TARGET_PAGE_MASK),
> -                                   hostbuf, currlen, is_write);
> +            address_space_rw(CPU(cpu)->as,
> +                             pages[i] | (laddr & ~TARGET_PAGE_MASK),
> +                             MEMTXATTRS_UNSPECIFIED,
> +                             hostbuf, currlen, is_write);
>              laddr += currlen;
>              hostbuf += currlen;
>              len -= currlen;
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH 2/6] hw/dma/rc4030: Use DMA address space to do DMA accesses

[Philippe Mathieu-Daudé]

The DMA device should not use the CPU address space
to do its operation, but its own address space.
Replace cpu_physical_memory_write() by dma_memory_read()
since we already have the DMA address space available.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/dma/rc4030.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c
index eefbabd758..c39fe2bb69 100644
--- a/hw/dma/rc4030.c
+++ b/hw/dma/rc4030.c
@@ -24,6 +24,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/units.h"
+#include "sysemu/dma.h"
 #include "hw/irq.h"
 #include "hw/mips/mips.h"
 #include "hw/sysbus.h"
@@ -301,7 +302,7 @@ static void rc4030_write(void *opaque, hwaddr addr, uint64_t data,
         if (s->cache_ltag == 0x80000001 && s->cache_bmask == 0xf0f0f0f) {
             hwaddr dest = s->cache_ptag & ~0x1;
             dest += (s->cache_maint & 0x3) << 3;
-            cpu_physical_memory_write(dest, &val, 4);
+            dma_memory_read(&s->dma_as, dest, &val, 4);
         }
         break;
     /* Remote Speed Registers */
-- 
2.21.3

[PATCH 3/6] hw/sd/allwinner-sdhost: Do DMA accesses via DMA address space

[Philippe Mathieu-Daudé]

The DMA operations should not use the CPU address space, but
the DMA address space. Add support for a DMA address space,
and replace the cpu_physical_memory API calls by equivalent
dma_memory_read/write calls.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/sd/allwinner-sdhost.h |  4 ++++
 hw/sd/allwinner-sdhost.c         | 36 ++++++++++++++++++++++++++------
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/include/hw/sd/allwinner-sdhost.h b/include/hw/sd/allwinner-sdhost.h
index d94606a853..ae1125c026 100644
--- a/include/hw/sd/allwinner-sdhost.h
+++ b/include/hw/sd/allwinner-sdhost.h
@@ -68,6 +68,10 @@ typedef struct AwSdHostState {
     /** Maps I/O registers in physical memory */
     MemoryRegion iomem;
 
+    /** DMA physical memory */
+    MemoryRegion *dma_mr;
+    AddressSpace dma_as;
+
     /** Interrupt output signal to notify CPU */
     qemu_irq irq;
 
diff --git a/hw/sd/allwinner-sdhost.c b/hw/sd/allwinner-sdhost.c
index f404e1fdb4..9a2b5fcaeb 100644
--- a/hw/sd/allwinner-sdhost.c
+++ b/hw/sd/allwinner-sdhost.c
@@ -21,7 +21,10 @@
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "qemu/units.h"
+#include "qapi/error.h"
 #include "sysemu/blockdev.h"
+#include "sysemu/dma.h"
+#include "hw/qdev-properties.h"
 #include "hw/irq.h"
 #include "hw/sd/allwinner-sdhost.h"
 #include "migration/vmstate.h"
@@ -306,7 +309,7 @@ static uint32_t allwinner_sdhost_process_desc(AwSdHostState *s,
     uint8_t buf[1024];
 
     /* Read descriptor */
-    cpu_physical_memory_read(desc_addr, desc, sizeof(*desc));
+    dma_memory_read(&s->dma_as, desc_addr, desc, sizeof(*desc));
     if (desc->size == 0) {
         desc->size = klass->max_desc_size;
     } else if (desc->size > klass->max_desc_size) {
@@ -331,8 +334,9 @@ static uint32_t allwinner_sdhost_process_desc(AwSdHostState *s,
 
         /* Write to SD bus */
         if (is_write) {
-            cpu_physical_memory_read((desc->addr & DESC_SIZE_MASK) + num_done,
-                                      buf, buf_bytes);
+            dma_memory_read(&s->dma_as,
+                            (desc->addr & DESC_SIZE_MASK) + num_done,
+                            buf, buf_bytes);
 
             for (uint32_t i = 0; i < buf_bytes; i++) {
                 sdbus_write_data(&s->sdbus, buf[i]);
@@ -343,15 +347,16 @@ static uint32_t allwinner_sdhost_process_desc(AwSdHostState *s,
             for (uint32_t i = 0; i < buf_bytes; i++) {
                 buf[i] = sdbus_read_data(&s->sdbus);
             }
-            cpu_physical_memory_write((desc->addr & DESC_SIZE_MASK) + num_done,
-                                       buf, buf_bytes);
+            dma_memory_write(&s->dma_as,
+                             (desc->addr & DESC_SIZE_MASK) + num_done,
+                             buf, buf_bytes);
         }
         num_done += buf_bytes;
     }
 
     /* Clear hold flag and flush descriptor */
     desc->status &= ~DESC_STATUS_HOLD;
-    cpu_physical_memory_write(desc_addr, desc, sizeof(*desc));
+    dma_memory_write(&s->dma_as, desc_addr, desc, sizeof(*desc));
 
     return num_done;
 }
@@ -742,6 +747,17 @@ static void allwinner_sdhost_init(Object *obj)
     sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq);
 }
 
+static void allwinner_sdhost_realize(DeviceState *dev, Error **errp)
+{
+    AwSdHostState *s = AW_SDHOST(dev);
+
+    if (!s->dma_mr) {
+        error_setg(errp, "\"dma\" property must be provided.");
+        return;
+    }
+    address_space_init(&s->dma_as, s->dma_mr, "sdhost-dma");
+}
+
 static void allwinner_sdhost_reset(DeviceState *dev)
 {
     AwSdHostState *s = AW_SDHOST(dev);
@@ -787,6 +803,12 @@ static void allwinner_sdhost_reset(DeviceState *dev)
     s->status_crc = REG_SD_CRC_STA_RST;
 }
 
+static Property allwinner_sdhost_properties[] = {
+    DEFINE_PROP_LINK("dma", AwSdHostState,
+                     dma_mr, TYPE_MEMORY_REGION, MemoryRegion *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void allwinner_sdhost_bus_class_init(ObjectClass *klass, void *data)
 {
     SDBusClass *sbc = SD_BUS_CLASS(klass);
@@ -798,7 +820,9 @@ static void allwinner_sdhost_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    device_class_set_props(dc, allwinner_sdhost_properties);
     dc->reset = allwinner_sdhost_reset;
+    dc->realize = allwinner_sdhost_realize;
     dc->vmsd = &vmstate_allwinner_sdhost;
 }
 
-- 
2.21.3

Re: [PATCH 3/6] hw/sd/allwinner-sdhost: Do DMA accesses via DMA address space

[Philippe Mathieu-Daudé]

On 5/31/20 7:54 PM, Philippe Mathieu-Daudé wrote:
> The DMA operations should not use the CPU address space, but
> the DMA address space. Add support for a DMA address space,
> and replace the cpu_physical_memory API calls by equivalent
> dma_memory_read/write calls.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/sd/allwinner-sdhost.h |  4 ++++
>  hw/sd/allwinner-sdhost.c         | 36 ++++++++++++++++++++++++++------
>  2 files changed, 34 insertions(+), 6 deletions(-)
> 
[...]> @@ -742,6 +747,17 @@ static void allwinner_sdhost_init(Object *obj)
>      sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq);
>  }
>  
> +static void allwinner_sdhost_realize(DeviceState *dev, Error **errp)
> +{
> +    AwSdHostState *s = AW_SDHOST(dev);
> +
> +    if (!s->dma_mr) {
> +        error_setg(errp, "\"dma\" property must be provided.");

Oops I forgot to include the part that sets this property in the A10/H3
SoCs.

> +        return;
> +    }
> +    address_space_init(&s->dma_as, s->dma_mr, "sdhost-dma");
> +}
> +
>  static void allwinner_sdhost_reset(DeviceState *dev)
>  {
>      AwSdHostState *s = AW_SDHOST(dev);
> @@ -787,6 +803,12 @@ static void allwinner_sdhost_reset(DeviceState *dev)
>      s->status_crc = REG_SD_CRC_STA_RST;
>  }
>  
> +static Property allwinner_sdhost_properties[] = {
> +    DEFINE_PROP_LINK("dma", AwSdHostState,
> +                     dma_mr, TYPE_MEMORY_REGION, MemoryRegion *),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void allwinner_sdhost_bus_class_init(ObjectClass *klass, void *data)
>  {
>      SDBusClass *sbc = SD_BUS_CLASS(klass);
> @@ -798,7 +820,9 @@ static void allwinner_sdhost_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
> +    device_class_set_props(dc, allwinner_sdhost_properties);
>      dc->reset = allwinner_sdhost_reset;
> +    dc->realize = allwinner_sdhost_realize;
>      dc->vmsd = &vmstate_allwinner_sdhost;
>  }
>  
> 

[PATCH 4/6] exec/cpu-common: Do not restrict CPU to 32-bit memory access maximum

[Philippe Mathieu-Daudé]

Most CPUs can do 64-bit operations. Update the CPUReadMemoryFunc
and CPUWriteMemoryFunc prototypes.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/exec/cpu-common.h |  4 ++--
 hw/usb/hcd-musb.c         | 12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index b47e5630e7..5ac766e3b6 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -43,8 +43,8 @@ extern ram_addr_t ram_size;
 
 /* memory API */
 
-typedef void CPUWriteMemoryFunc(void *opaque, hwaddr addr, uint32_t value);
-typedef uint32_t CPUReadMemoryFunc(void *opaque, hwaddr addr);
+typedef void CPUWriteMemoryFunc(void *opaque, hwaddr addr, uint64_t value);
+typedef uint64_t CPUReadMemoryFunc(void *opaque, hwaddr addr);
 
 void qemu_ram_remap(ram_addr_t addr, ram_addr_t length);
 /* This should not be used by devices.  */
diff --git a/hw/usb/hcd-musb.c b/hw/usb/hcd-musb.c
index c29fbef6fc..4063cbccf8 100644
--- a/hw/usb/hcd-musb.c
+++ b/hw/usb/hcd-musb.c
@@ -1243,7 +1243,7 @@ static void musb_ep_writeh(void *opaque, int ep, int addr, uint16_t value)
 }
 
 /* Generic control */
-static uint32_t musb_readb(void *opaque, hwaddr addr)
+static uint64_t musb_readb(void *opaque, hwaddr addr)
 {
     MUSBState *s = (MUSBState *) opaque;
     int ep, i;
@@ -1305,7 +1305,7 @@ static uint32_t musb_readb(void *opaque, hwaddr addr)
     };
 }
 
-static void musb_writeb(void *opaque, hwaddr addr, uint32_t value)
+static void musb_writeb(void *opaque, hwaddr addr, uint64_t value)
 {
     MUSBState *s = (MUSBState *) opaque;
     int ep;
@@ -1392,7 +1392,7 @@ static void musb_writeb(void *opaque, hwaddr addr, uint32_t value)
     };
 }
 
-static uint32_t musb_readh(void *opaque, hwaddr addr)
+static uint64_t musb_readh(void *opaque, hwaddr addr)
 {
     MUSBState *s = (MUSBState *) opaque;
     int ep, i;
@@ -1446,7 +1446,7 @@ static uint32_t musb_readh(void *opaque, hwaddr addr)
     };
 }
 
-static void musb_writeh(void *opaque, hwaddr addr, uint32_t value)
+static void musb_writeh(void *opaque, hwaddr addr, uint64_t value)
 {
     MUSBState *s = (MUSBState *) opaque;
     int ep;
@@ -1502,7 +1502,7 @@ static void musb_writeh(void *opaque, hwaddr addr, uint32_t value)
     };
 }
 
-static uint32_t musb_readw(void *opaque, hwaddr addr)
+static uint64_t musb_readw(void *opaque, hwaddr addr)
 {
     MUSBState *s = (MUSBState *) opaque;
     int ep;
@@ -1520,7 +1520,7 @@ static uint32_t musb_readw(void *opaque, hwaddr addr)
     };
 }
 
-static void musb_writew(void *opaque, hwaddr addr, uint32_t value)
+static void musb_writew(void *opaque, hwaddr addr, uint64_t value)
 {
     MUSBState *s = (MUSBState *) opaque;
     int ep;
-- 
2.21.3

Re: [PATCH 4/6] exec/cpu-common: Do not restrict CPU to 32-bit memory access maximum

[Peter Maydell]

On Sun, 31 May 2020 at 18:54, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Most CPUs can do 64-bit operations. Update the CPUReadMemoryFunc
> and CPUWriteMemoryFunc prototypes.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/exec/cpu-common.h |  4 ++--
>  hw/usb/hcd-musb.c         | 12 ++++++------
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index b47e5630e7..5ac766e3b6 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -43,8 +43,8 @@ extern ram_addr_t ram_size;
>
>  /* memory API */
>
> -typedef void CPUWriteMemoryFunc(void *opaque, hwaddr addr, uint32_t value);
> -typedef uint32_t CPUReadMemoryFunc(void *opaque, hwaddr addr);
> +typedef void CPUWriteMemoryFunc(void *opaque, hwaddr addr, uint64_t value);
> +typedef uint64_t CPUReadMemoryFunc(void *opaque, hwaddr addr);

I don't think the type of these functions has anything to do with the
CPU's capabilities, does it? The typedefs are a legacy remnant from before
the conversion to MemoryRegions:
 * before MemoryRegions, devices provided separate functions for
   byte/word/long reads and writes (64-bit writes were simply
   impossible with the ancient APIs, which required a 3-element
   function pointer array for read and the same for write)
 * the initial MemoryRegion conversion introduced the new-style
   "one read/write fn for all widths" APIs, but also supported
   old-style six-function devices, for ease of conversion, using
   MemoryRegionOps::old_mmio.
 * in commit 62a0db942dec6ebfe we were finally able to drop the
   old_mmio (having changed over the last devices using old-style).
   (I see I forgot to delete the now-unused MemoryRegionMmio typedef.)

The only remaining user of these typedefs is hw/usb/hcd-musb.c,
which is still not converted to QOM/qdev. It uses them to allow
its one user (hw/usb/tusb6010.c) to perform reads/writes on the
underlying musb registers.

There's no point in changing these typedefs to pass or return
a 64-bit data type, because their sole use is in the musb_read[]
and musb_write[] arrays, which only allow for 1, 2 or 4 byte
accesses, depending on which array element you use.

Possible cleanups here:
Easy:
 * delete the unused MmeoryRegionMmio
 * move these typedefs into include/hw/usb.h and rename them
   to MUSBReadFunc and MUSBWriteFunc, since that's all they're
   used for now
Tricky:
 * convert the hw/usb/hcd-musb.c code to QOM/qdev, which would
   include refactoring the current musb_read/musb_write so that
   instead of the tusb6010.c code calling function entries in these
   arrays the hcd-musb.c code exposed a MemoryRegion; the tusb6010
   code would access it via memory_region_dispatch_read/write

thanks
-- PMM

Re: [PATCH 4/6] exec/cpu-common: Do not restrict CPU to 32-bit memory access maximum

[Philippe Mathieu-Daudé]

On 5/31/20 9:41 PM, Peter Maydell wrote:
> On Sun, 31 May 2020 at 18:54, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Most CPUs can do 64-bit operations. Update the CPUReadMemoryFunc
>> and CPUWriteMemoryFunc prototypes.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  include/exec/cpu-common.h |  4 ++--
>>  hw/usb/hcd-musb.c         | 12 ++++++------
>>  2 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
>> index b47e5630e7..5ac766e3b6 100644
>> --- a/include/exec/cpu-common.h
>> +++ b/include/exec/cpu-common.h
>> @@ -43,8 +43,8 @@ extern ram_addr_t ram_size;
>>
>>  /* memory API */
>>
>> -typedef void CPUWriteMemoryFunc(void *opaque, hwaddr addr, uint32_t value);
>> -typedef uint32_t CPUReadMemoryFunc(void *opaque, hwaddr addr);
>> +typedef void CPUWriteMemoryFunc(void *opaque, hwaddr addr, uint64_t value);
>> +typedef uint64_t CPUReadMemoryFunc(void *opaque, hwaddr addr);
> 
> I don't think the type of these functions has anything to do with the
> CPU's capabilities, does it? The typedefs are a legacy remnant from before
> the conversion to MemoryRegions:
>  * before MemoryRegions, devices provided separate functions for
>    byte/word/long reads and writes (64-bit writes were simply
>    impossible with the ancient APIs, which required a 3-element
>    function pointer array for read and the same for write)
>  * the initial MemoryRegion conversion introduced the new-style
>    "one read/write fn for all widths" APIs, but also supported
>    old-style six-function devices, for ease of conversion, using
>    MemoryRegionOps::old_mmio.
>  * in commit 62a0db942dec6ebfe we were finally able to drop the
>    old_mmio (having changed over the last devices using old-style).
>    (I see I forgot to delete the now-unused MemoryRegionMmio typedef.)
> 
> The only remaining user of these typedefs is hw/usb/hcd-musb.c,
> which is still not converted to QOM/qdev. It uses them to allow
> its one user (hw/usb/tusb6010.c) to perform reads/writes on the
> underlying musb registers.

Indeed you are correct, I have been short-sighted.

> 
> There's no point in changing these typedefs to pass or return
> a 64-bit data type, because their sole use is in the musb_read[]
> and musb_write[] arrays, which only allow for 1, 2 or 4 byte
> accesses, depending on which array element you use.
> 
> Possible cleanups here:
> Easy:
>  * delete the unused MmeoryRegionMmio
>  * move these typedefs into include/hw/usb.h and rename them
>    to MUSBReadFunc and MUSBWriteFunc, since that's all they're
>    used for now

Agreed.

> Tricky:
>  * convert the hw/usb/hcd-musb.c code to QOM/qdev, which would
>    include refactoring the current musb_read/musb_write so that
>    instead of the tusb6010.c code calling function entries in these
>    arrays the hcd-musb.c code exposed a MemoryRegion; the tusb6010
>    code would access it via memory_region_dispatch_read/write

Left as TODO.

Thanks for reviewing this patch.

> 
> thanks
> -- PMM
> 

[PATCH 5/6] exec: Restrict 32-bit CPUs to 32-bit address space

[Philippe Mathieu-Daudé]

It is pointless to have 32-bit CPUs see a 64-bit address
space, when they can only address the 32 lower bits.

Only create CPU address space with a size it can address.
This makes HMP 'info mtree' command easier to understand
(on 32-bit CPUs).

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
This is particularly helpful with the AVR cores.
---
 exec.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/exec.c b/exec.c
index 5162f0d12f..d6809a9447 100644
--- a/exec.c
+++ b/exec.c
@@ -2962,9 +2962,17 @@ static void tcg_commit(MemoryListener *listener)
 
 static void memory_map_init(void)
 {
+    uint64_t system_memory_size;
+
+#if TARGET_LONG_BITS >= 64
+    system_memory_size = UINT64_MAX;
+#else
+    system_memory_size = 1ULL << TARGET_LONG_BITS;
+#endif
+
     system_memory = g_malloc(sizeof(*system_memory));
 
-    memory_region_init(system_memory, NULL, "system", UINT64_MAX);
+    memory_region_init(system_memory, NULL, "system", system_memory_size);
     address_space_init(&address_space_memory, system_memory, "memory");
 
     system_io = g_malloc(sizeof(*system_io));
-- 
2.21.3

Re: [PATCH 5/6] exec: Restrict 32-bit CPUs to 32-bit address space

[Peter Maydell]

On Sun, 31 May 2020 at 18:54, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> It is pointless to have 32-bit CPUs see a 64-bit address
> space, when they can only address the 32 lower bits.
>
> Only create CPU address space with a size it can address.
> This makes HMP 'info mtree' command easier to understand
> (on 32-bit CPUs).

> diff --git a/exec.c b/exec.c
> index 5162f0d12f..d6809a9447 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2962,9 +2962,17 @@ static void tcg_commit(MemoryListener *listener)
>
>  static void memory_map_init(void)
>  {
> +    uint64_t system_memory_size;
> +
> +#if TARGET_LONG_BITS >= 64
> +    system_memory_size = UINT64_MAX;
> +#else
> +    system_memory_size = 1ULL << TARGET_LONG_BITS;
> +#endif

TARGET_LONG_BITS is a description of the CPU's virtual
address size; but the size of the system_memory memory
region is related to the CPU's physical address size[*].
In particular, for the Arm Cortex-A15 (and any other
32-bit CPU with LPAE) TARGET_LONG_BITS is 32 but the CPU
can address more than 32 bits of physical memory.

[*] Strictly speaking, it would depend on the
maximum physical address size used by any transaction
master in the system -- in theory you could have a
32-bit-only CPU and a DMA controller that could be
programmed with 64-bit addresses. In practice the
CPU can generally address at least as much of the
physical address space as any other transaction master.

thanks
-- PMM

Re: [PATCH 5/6] exec: Restrict 32-bit CPUs to 32-bit address space

[Philippe Mathieu-Daudé]

On 5/31/20 9:09 PM, Peter Maydell wrote:
> On Sun, 31 May 2020 at 18:54, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> It is pointless to have 32-bit CPUs see a 64-bit address
>> space, when they can only address the 32 lower bits.
>>
>> Only create CPU address space with a size it can address.
>> This makes HMP 'info mtree' command easier to understand
>> (on 32-bit CPUs).
> 
>> diff --git a/exec.c b/exec.c
>> index 5162f0d12f..d6809a9447 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -2962,9 +2962,17 @@ static void tcg_commit(MemoryListener *listener)
>>
>>  static void memory_map_init(void)
>>  {
>> +    uint64_t system_memory_size;
>> +
>> +#if TARGET_LONG_BITS >= 64
>> +    system_memory_size = UINT64_MAX;
>> +#else
>> +    system_memory_size = 1ULL << TARGET_LONG_BITS;
>> +#endif
> 
> TARGET_LONG_BITS is a description of the CPU's virtual
> address size; but the size of the system_memory memory
> region is related to the CPU's physical address size[*].

OK I misunderstood it was the physical size, not virtual.

> In particular, for the Arm Cortex-A15 (and any other
> 32-bit CPU with LPAE) TARGET_LONG_BITS is 32 but the CPU
> can address more than 32 bits of physical memory.
> 
> [*] Strictly speaking, it would depend on the
> maximum physical address size used by any transaction
> master in the system -- in theory you could have a
> 32-bit-only CPU and a DMA controller that could be
> programmed with 64-bit addresses. In practice the
> CPU can generally address at least as much of the
> physical address space as any other transaction master.

Yes, I tried the Malta with 32-bit core, while the GT64120 northbridge
addresses 64-bit:

address-space: cpu-memory-0
  0000000000000000-00000000ffffffff (prio 0, i/o): system
    0000000000000000-0000000007ffffff (prio 0, ram): alias
mips_malta_low_preio.ram @mips_malta.ram 0000000000000000-0000000007ffffff
    0000000000000000-000000001fffffff (prio 0, i/o): empty-slot
    0000000010000000-0000000011ffffff (prio 0, i/o): alias pci0-io @io
0000000000000000-0000000001ffffff
    0000000012000000-0000000013ffffff (prio 0, i/o): alias pci0-mem0
@pci0-mem 0000000012000000-0000000013ffffff

address-space: gt64120_pci
  0000000000000000-ffffffffffffffff (prio 0, i/o): bus master container
    0000000000000000-00000000ffffffff (prio 0, i/o): alias bus master
@system 0000000000000000-00000000ffffffff [disabled]

So my testing is bad, because I want @system to be 64-bit wide for the
GT64120.

From "In practice the CPU can generally address at least as much of the
physical address space as any other transaction master." I understand
for QEMU @system address space must be as big as the largest transaction
a bus master can do".

I think what confuse me is what QEMU means by 'system-memory', I
understand it historically as the address space of the first CPU core.

This is more complex in the case of the raspi SoC where the DSP can
address the main bus (system memory) while the ARM cores access it via
an MMU, see this schema:
https://www.raspberrypi.org/forums/viewtopic.php?t=262747

Regards,

Phil.

Re: [PATCH 5/6] exec: Restrict 32-bit CPUs to 32-bit address space

[Peter Maydell]

On Mon, 1 Jun 2020 at 09:09, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 5/31/20 9:09 PM, Peter Maydell wrote:
> > [*] Strictly speaking, it would depend on the
> > maximum physical address size used by any transaction
> > master in the system -- in theory you could have a
> > 32-bit-only CPU and a DMA controller that could be
> > programmed with 64-bit addresses. In practice the
> > CPU can generally address at least as much of the
> > physical address space as any other transaction master.
>
> Yes, I tried the Malta with 32-bit core, while the GT64120 northbridge
> addresses 64-bit:

> From "In practice the CPU can generally address at least as much of the
> physical address space as any other transaction master." I understand
> for QEMU @system address space must be as big as the largest transaction
> a bus master can do".

That depends on what happens for transactions that are off the end
of the range, I suppose -- usually a 32-bit CPU system design will
for obvious reasons not put ram or devices over 4GB, so if the
behaviour for a DMA access past 4GB is the same whether there's
nothing mapped there or whether the access is just off-the-end then
it doesn't matter how QEMU models it. I haven't tested to see what an
off-the-end transaction does, though.

I'm inclined to say that since 'hwaddr' is always a 64-bit type we should
stick to having the system memory address space be64 bits.

> I think what confuse me is what QEMU means by 'system-memory', I
> understand it historically as the address space of the first CPU core.

Historically I think it was more "there is only one address space and
this is it": it wasn't the first CPU's address space, it was what *every*
CPU saw, and what every DMA device used, because the APIs
pre-MemoryRegion had no concept of separate address spaces at all.
So system-memory starts off as a way to continue to provide those
old semantics in an AddressSpace/MemoryRegion design, and we've
then gradually increased the degree to which different transaction
masters use different AddressSpaces. Typically system-memory
today is often "whatever's common to all CPUs" (and then you
overlay per-CPU devices etc on top of that), but it might have
less stuff than that in it (I have a feeling the arm-sse SoCs put
less stuff into system-memory than you might expect). How much
freedom you have to not put stuff into the system-memory address
space depends on things like whether the guest architecture's
target/foo code or some DMA device model on the board still uses
APIs that don't specify the address space and instead use the
system address space.

thanks
-- PMM

Re: [PATCH 5/6] exec: Restrict 32-bit CPUs to 32-bit address space

[Richard Henderson]

On 6/1/20 1:09 AM, Philippe Mathieu-Daudé wrote:
> On 5/31/20 9:09 PM, Peter Maydell wrote:
>> On Sun, 31 May 2020 at 18:54, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>
>>> It is pointless to have 32-bit CPUs see a 64-bit address
>>> space, when they can only address the 32 lower bits.
>>>
>>> Only create CPU address space with a size it can address.
>>> This makes HMP 'info mtree' command easier to understand
>>> (on 32-bit CPUs).
>>
>>> diff --git a/exec.c b/exec.c
>>> index 5162f0d12f..d6809a9447 100644
>>> --- a/exec.c
>>> +++ b/exec.c
>>> @@ -2962,9 +2962,17 @@ static void tcg_commit(MemoryListener *listener)
>>>
>>>  static void memory_map_init(void)
>>>  {
>>> +    uint64_t system_memory_size;
>>> +
>>> +#if TARGET_LONG_BITS >= 64
>>> +    system_memory_size = UINT64_MAX;
>>> +#else
>>> +    system_memory_size = 1ULL << TARGET_LONG_BITS;
>>> +#endif
>>
>> TARGET_LONG_BITS is a description of the CPU's virtual
>> address size; but the size of the system_memory memory
>> region is related to the CPU's physical address size[*].
> 
> OK I misunderstood it was the physical size, not virtual.

It is the physical size.

In the armv7 case, the lpae page table entry maps a 32-bit virtual address to a
40-bit physical address.  The i686 page table extensions do something similar.

See TARGET_PHYS_ADDR_SPACE_BITS.


r~

[RFC PATCH 6/6] memory: Use CPU register size as default access_size_max

[Philippe Mathieu-Daudé]

Do not restrict 64-bit CPU to 32-bit max access by default.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
RFC because this probably require an audit of all devices
used on 64-bit targets.
But if we find such problematic devices, they should instead
enforce their access_size_max = 4 rather than expecting the
default value to be valid...
---
 memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/memory.c b/memory.c
index fd6f3d6aca..1d6bb5cdb0 100644
--- a/memory.c
+++ b/memory.c
@@ -1370,7 +1370,7 @@ bool memory_region_access_valid(MemoryRegion *mr,
 
     access_size_max = mr->ops->valid.max_access_size;
     if (!mr->ops->valid.max_access_size) {
-        access_size_max = 4;
+        access_size_max = TARGET_LONG_SIZE;
     }
 
     access_size = MAX(MIN(size, access_size_max), access_size_min);
-- 
2.21.3

Re: [RFC PATCH 6/6] memory: Use CPU register size as default access_size_max

[Peter Maydell]

On Sun, 31 May 2020 at 18:54, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Do not restrict 64-bit CPU to 32-bit max access by default.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> RFC because this probably require an audit of all devices
> used on 64-bit targets.
> But if we find such problematic devices, they should instead
> enforce their access_size_max = 4 rather than expecting the
> default value to be valid...
> ---
>  memory.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/memory.c b/memory.c
> index fd6f3d6aca..1d6bb5cdb0 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1370,7 +1370,7 @@ bool memory_region_access_valid(MemoryRegion *mr,
>
>      access_size_max = mr->ops->valid.max_access_size;
>      if (!mr->ops->valid.max_access_size) {
> -        access_size_max = 4;
> +        access_size_max = TARGET_LONG_SIZE;
>      }

This is definitely not the right approach. TARGET_LONG_SIZE
is a property of the CPU, but memory_region_access_valid()
is testing properties of the MemoryRegion (ie the device
being addressed). One can have devices in a system with a
64-bit CPU which can only handle being accessed at 32-bit
width (indeed, it's pretty common). The behaviour of a device
shouldn't change depending on whether we happened to compile
it into a system with TARGET_LONG_SIZE=4 or 8.

(If you want to argue that we should make all our devices
explicit about the valid.max_access_size rather than relying
on "default means 4" then I wouldn't necessarily disagree.)

thanks
-- PMM

Re: [RFC PATCH 6/6] memory: Use CPU register size as default access_size_max

[Philippe Mathieu-Daudé]

On 5/31/20 9:14 PM, Peter Maydell wrote:
> On Sun, 31 May 2020 at 18:54, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Do not restrict 64-bit CPU to 32-bit max access by default.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> RFC because this probably require an audit of all devices
>> used on 64-bit targets.
>> But if we find such problematic devices, they should instead
>> enforce their access_size_max = 4 rather than expecting the
>> default value to be valid...
>> ---
>>  memory.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/memory.c b/memory.c
>> index fd6f3d6aca..1d6bb5cdb0 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -1370,7 +1370,7 @@ bool memory_region_access_valid(MemoryRegion *mr,
>>
>>      access_size_max = mr->ops->valid.max_access_size;
>>      if (!mr->ops->valid.max_access_size) {
>> -        access_size_max = 4;
>> +        access_size_max = TARGET_LONG_SIZE;
>>      }
> 
> This is definitely not the right approach. TARGET_LONG_SIZE
> is a property of the CPU, but memory_region_access_valid()
> is testing properties of the MemoryRegion (ie the device
> being addressed). One can have devices in a system with a
> 64-bit CPU which can only handle being accessed at 32-bit
> width (indeed, it's pretty common). The behaviour of a device
> shouldn't change depending on whether we happened to compile
> it into a system with TARGET_LONG_SIZE=4 or 8.

Agreed.

> 
> (If you want to argue that we should make all our devices
> explicit about the valid.max_access_size rather than relying
> on "default means 4" then I wouldn't necessarily disagree.)

Yes, I'd rather not have access_size_max set automagically, but fixing
this require a long audit, and I suppose nobody cares.
I'll drop this patch. Thanks for the review!

> 
> thanks
> -- PMM
> 

Re: [PATCH 0/6] exec/memory: Rework some address and access size limits

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200531175425.10329-1-f4bug@amsat.org/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

qemu-system-aarch64: Initialization of device allwinner-sdhost-sun4i failed: "dma" property must be provided.
Broken pipe
/tmp/qemu-test/src/tests/qtest/libqtest.c:166: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0)
ERROR - too few tests run (expected 66, got 19)
make: *** [check-qtest-aarch64] Error 1
make: *** Waiting for unfinished jobs....
Could not access KVM kernel module: No such file or directory
qemu-system-x86_64: -accel kvm: failed to initialize kvm: No such file or directory
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=2f35274b5f7c4771ba16a3557b505e20', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-gxvgqn30/src/docker-src.2020-05-31-14.18.38.23880:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=2f35274b5f7c4771ba16a3557b505e20
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-gxvgqn30/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    13m18.712s
user    0m8.733s


The full log is available at
http://patchew.org/logs/20200531175425.10329-1-f4bug@amsat.org/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com