Re: [PATCH] hw/sd/allwinner-sdhost: Use AddressSpace for DMA transfers

2020-08-24 Thread Peter Maydell 
On Fri, 14 Aug 2020 at 12:01, Philippe Mathieu-Daudé  wrote:
>
> Allow the device to execute the DMA transfers in a different
> AddressSpace.
>
> The A10 and H3 SoC keep using the system_memory address space,
> but via the proper dma_memory_access() API.
>
> Signed-off-by: Philippe Mathieu-Daudé 



Applied to target-arm.next, thanks.

-- PMM

Re: [PATCH] hw/sd/allwinner-sdhost: Use AddressSpace for DMA transfers

2020-08-17 Thread Niek Linnenbank 
Hi Philippe,

Nice improvement, I didnt know about this API. Makes sense to use it indeed.
The patch works fine. I tested your patches by applying the previous two
sets first, and then this one.
It ran well with the avocado tests and also with the official image
OrangePi_pc_debian_stretch_server_linux5.3.5_v1.0.img.

Tested-by: Niek Linnenbank 
Reviewed-by: Niek Linnenbank 

Regards,
Niek



On Fri, Aug 14, 2020 at 1:01 PM Philippe Mathieu-Daudé 
wrote:

> Allow the device to execute the DMA transfers in a different
> AddressSpace.
>
> The A10 and H3 SoC keep using the system_memory address space,
> but via the proper dma_memory_access() API.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> Based-on: <20200814092346.21825-1-f4...@amsat.org>
> "hw/sd: Use sdbus_read_data/sdbus_write_data for multiple bytes access"
>
> Tested with:
>   AVOCADO_ALLOW_LARGE_STORAGE=1 avocado run -t machine:orangepi-pc -t
> machine:cubieboard tests/acceptance/
> ---
>  include/hw/sd/allwinner-sdhost.h |  6 ++
>  hw/arm/allwinner-a10.c   |  2 ++
>  hw/arm/allwinner-h3.c|  2 ++
>  hw/sd/allwinner-sdhost.c | 37 ++--
>  4 files changed, 41 insertions(+), 6 deletions(-)
>
> diff --git a/include/hw/sd/allwinner-sdhost.h
> b/include/hw/sd/allwinner-sdhost.h
> index d94606a853..839732ebf3 100644
> --- a/include/hw/sd/allwinner-sdhost.h
> +++ b/include/hw/sd/allwinner-sdhost.h
> @@ -71,6 +71,12 @@ typedef struct AwSdHostState {
>  /** Interrupt output signal to notify CPU */
>  qemu_irq irq;
>
> +/** Memory region where DMA transfers are done */
> +MemoryRegion *dma_mr;
> +
> +/** Address space used internally for DMA transfers */
> +AddressSpace dma_as;
> +
>  /** Number of bytes left in current DMA transfer */
>  uint32_t transfer_cnt;
>
> diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
> index e258463747..d404f31e02 100644
> --- a/hw/arm/allwinner-a10.c
> +++ b/hw/arm/allwinner-a10.c
> @@ -155,6 +155,8 @@ static void aw_a10_realize(DeviceState *dev, Error
> **errp)
>  }
>
>  /* SD/MMC */
> +object_property_set_link(OBJECT(&s->mmc0), "dma-memory",
> + OBJECT(get_system_memory()), &error_fatal);
>  sysbus_realize(SYS_BUS_DEVICE(&s->mmc0), &error_fatal);
>  sysbus_mmio_map(SYS_BUS_DEVICE(&s->mmc0), 0, AW_A10_MMC0_BASE);
>  sysbus_connect_irq(SYS_BUS_DEVICE(&s->mmc0), 0, qdev_get_gpio_in(dev,
> 32));
> diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
> index ff92ded82c..43a8d3dc48 100644
> --- a/hw/arm/allwinner-h3.c
> +++ b/hw/arm/allwinner-h3.c
> @@ -349,6 +349,8 @@ static void allwinner_h3_realize(DeviceState *dev,
> Error **errp)
>  sysbus_mmio_map(SYS_BUS_DEVICE(&s->sid), 0, s->memmap[AW_H3_SID]);
>
>  /* SD/MMC */
> +object_property_set_link(OBJECT(&s->mmc0), "dma-memory",
> + OBJECT(get_system_memory()), &error_fatal);
>  sysbus_realize(SYS_BUS_DEVICE(&s->mmc0), &error_fatal);
>  sysbus_mmio_map(SYS_BUS_DEVICE(&s->mmc0), 0, s->memmap[AW_H3_MMC0]);
>  sysbus_connect_irq(SYS_BUS_DEVICE(&s->mmc0), 0,
> diff --git a/hw/sd/allwinner-sdhost.c b/hw/sd/allwinner-sdhost.c
> index f9eb92c09e..e82afb75eb 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,22 +334,24 @@ 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);
>  sdbus_write_data(&s->sdbus, buf, buf_bytes);
>
>  /* Read from SD bus */
>  } else {
>  sdbus_read_data(&s->sdbus, buf, buf_bytes);
> -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);
>

Re: [PATCH] hw/sd/allwinner-sdhost: Use AddressSpace for DMA transfers

2020-08-14 Thread no-reply 
Patchew URL: https://patchew.org/QEMU/20200814110057.307-1-f4...@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 ===

  CC  x86_64-softmmu/target/i386/smm_helper.o
  CC  x86_64-softmmu/target/i386/svm_helper.o
/tmp/qemu-test/src/hw/arm/allwinner-a10.c: In function 'aw_a10_realize':
/tmp/qemu-test/src/hw/arm/allwinner-a10.c:170:30: error: passing argument 2 of 
'object_property_set_link' from incompatible pointer type [-Werror]
  OBJECT(get_system_memory()), &error_fatal);
  ^
In file included from /tmp/qemu-test/src/include/exec/memory.h:28:0,
---
/tmp/qemu-test/src/include/qom/object.h:1243:6: note: expected 'struct Object 
*' but argument is of type 'const char *'
 void object_property_set_link(Object *obj, Object *value,
  ^
/tmp/qemu-test/src/hw/arm/allwinner-a10.c:170:30: error: passing argument 3 of 
'object_property_set_link' from incompatible pointer type [-Werror]
  OBJECT(get_system_memory()), &error_fatal);
  ^
In file included from /tmp/qemu-test/src/include/exec/memory.h:28:0,
---
 void object_property_set_link(Object *obj, Object *value,
  ^
cc1: all warnings being treated as errors
make[1]: *** [hw/arm/allwinner-a10.o] Error 1
make[1]: *** Waiting for unfinished jobs
  CC  x86_64-softmmu/target/i386/machine.o
  CC  x86_64-softmmu/target/i386/arch_memory_mapping.o
/tmp/qemu-test/src/hw/arm/allwinner-h3.c: In function 'allwinner_h3_realize':
/tmp/qemu-test/src/hw/arm/allwinner-h3.c:353:30: error: passing argument 2 of 
'object_property_set_link' from incompatible pointer type [-Werror]
  OBJECT(get_system_memory()), &error_fatal);
  ^
In file included from /tmp/qemu-test/src/include/exec/memory.h:28:0,
---
/tmp/qemu-test/src/include/qom/object.h:1243:6: note: expected 'struct Object 
*' but argument is of type 'const char *'
 void object_property_set_link(Object *obj, Object *value,
  ^
/tmp/qemu-test/src/hw/arm/allwinner-h3.c:353:30: error: passing argument 3 of 
'object_property_set_link' from incompatible pointer type [-Werror]
  OBJECT(get_system_memory()), &error_fatal);
  ^
In file included from /tmp/qemu-test/src/include/exec/memory.h:28:0,
---
 void object_property_set_link(Object *obj, Object *value,
  ^
cc1: all warnings being treated as errors
make[1]: *** [hw/arm/allwinner-h3.o] Error 1
  CC  x86_64-softmmu/target/i386/arch_dump.o
  CC  x86_64-softmmu/target/i386/monitor.o
  CC  x86_64-softmmu/target/i386/kvm.o
---
  CC  x86_64-softmmu/softmmu/main.o
  CC  x86_64-softmmu/gdbstub-xml.o
  CC  x86_64-softmmu/trace/generated-helpers.o
make: *** [aarch64-softmmu/all] Error 2
make: *** Waiting for unfinished jobs
  LINKx86_64-softmmu/qemu-system-x86_64
Traceback (most recent call last):
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=db66dd82f40243898ca5fc5874340ff2', '-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-_2_0qwhh/src/docker-src.2020-08-14-08.01.24.10929:/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=db66dd82f40243898ca5fc5874340ff2
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-_2_0qwhh/src'
make: *** [docker-run-test-quick@centos7] Error 2

real3m37.968s
user0m8.605s


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

Re: [PATCH] hw/sd/allwinner-sdhost: Use AddressSpace for DMA transfers

2020-08-14 Thread no-reply 
Patchew URL: https://patchew.org/QEMU/20200814110057.307-1-f4...@amsat.org/



Hi,

This series failed the docker-mingw@fedora 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
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC  aarch64-softmmu/trace/generated-helpers.o
  CC  aarch64-softmmu/target/arm/translate-sve.o
/tmp/qemu-test/src/hw/arm/allwinner-a10.c: In function 'aw_a10_realize':
/tmp/qemu-test/src/hw/arm/allwinner-a10.c:169:48: error: passing argument 2 of 
'object_property_set_link' from incompatible pointer type 
[-Werror=incompatible-pointer-types]
  169 | object_property_set_link(OBJECT(&s->mmc0), "dma-memory",
  |^~~~
  ||
---
/tmp/qemu-test/src/include/qom/object.h:1243:52: note: expected 'Object *' {aka 
'struct Object *'} but argument is of type 'const char *'
 1243 | void object_property_set_link(Object *obj, Object *value,
  |^
/tmp/qemu-test/src/include/qom/object.h:509:6: error: passing argument 3 of 
'object_property_set_link' from incompatible pointer type 
[-Werror=incompatible-pointer-types]
  509 | ((Object *)(obj))
  | ~^~~~
  |  |
---
  170 |  OBJECT(get_system_memory()), &error_fatal);
  |  ^~
/tmp/qemu-test/src/include/qom/object.h:1244:43: note: expected 'const char *' 
but argument is of type 'Object *' {aka 'struct Object *'}
 1244 |   const char *name, Error **errp);
  |   ^~~~
cc1: all warnings being treated as errors
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: hw/arm/allwinner-a10.o] Error 1
make[1]: *** Waiting for unfinished jobs
/tmp/qemu-test/src/hw/arm/allwinner-h3.c: In function 'allwinner_h3_realize':
/tmp/qemu-test/src/hw/arm/allwinner-h3.c:352:48: error: passing argument 2 of 
'object_property_set_link' from incompatible pointer type 
[-Werror=incompatible-pointer-types]
  352 | object_property_set_link(OBJECT(&s->mmc0), "dma-memory",
  |^~~~
  ||
---
/tmp/qemu-test/src/include/qom/object.h:1243:52: note: expected 'Object *' {aka 
'struct Object *'} but argument is of type 'const char *'
 1243 | void object_property_set_link(Object *obj, Object *value,
  |^
/tmp/qemu-test/src/include/qom/object.h:509:6: error: passing argument 3 of 
'object_property_set_link' from incompatible pointer type 
[-Werror=incompatible-pointer-types]
  509 | ((Object *)(obj))
  | ~^~~~
  |  |
---
  353 |  OBJECT(get_system_memory()), &error_fatal);
  |  ^~
/tmp/qemu-test/src/include/qom/object.h:1244:43: note: expected 'const char *' 
but argument is of type 'Object *' {aka 'struct Object *'}
 1244 |   const char *name, Error **errp);
  |   ^~~~
cc1: all warnings being treated as errors
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: hw/arm/allwinner-h3.o] Error 1
make: *** [Makefile:527: aarch64-softmmu/all] Error 2
make: *** Waiting for unfinished jobs
  GEN x86_64-softmmu/qemu-system-x86_64.exe
Traceback (most recent call last):
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=90cc61fdf8a54dfb83e2aee788b81fec', '-u', 
'1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
'-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 
'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
'/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-o9b1ibdl/src/docker-src.2020-08-14-07.57.27.32200:/var/tmp/qemu:z,ro',
 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=90cc61fdf8a54dfb83e2aee788b81fec
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-o9b1ibdl/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real3m17.833s
user0m8.465s


The full log is available at
http://patchew.org/logs/20200814110057.307-1-f4...@amsat.org/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

[PATCH] hw/sd/allwinner-sdhost: Use AddressSpace for DMA transfers

2020-08-14 Thread Philippe Mathieu-Daudé 
Allow the device to execute the DMA transfers in a different
AddressSpace.

The A10 and H3 SoC keep using the system_memory address space,
but via the proper dma_memory_access() API.

Signed-off-by: Philippe Mathieu-Daudé 
---
Based-on: <20200814092346.21825-1-f4...@amsat.org>
"hw/sd: Use sdbus_read_data/sdbus_write_data for multiple bytes access"

Tested with:
  AVOCADO_ALLOW_LARGE_STORAGE=1 avocado run -t machine:orangepi-pc -t 
machine:cubieboard tests/acceptance/
---
 include/hw/sd/allwinner-sdhost.h |  6 ++
 hw/arm/allwinner-a10.c   |  2 ++
 hw/arm/allwinner-h3.c|  2 ++
 hw/sd/allwinner-sdhost.c | 37 ++--
 4 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/include/hw/sd/allwinner-sdhost.h b/include/hw/sd/allwinner-sdhost.h
index d94606a853..839732ebf3 100644
--- a/include/hw/sd/allwinner-sdhost.h
+++ b/include/hw/sd/allwinner-sdhost.h
@@ -71,6 +71,12 @@ typedef struct AwSdHostState {
 /** Interrupt output signal to notify CPU */
 qemu_irq irq;
 
+/** Memory region where DMA transfers are done */
+MemoryRegion *dma_mr;
+
+/** Address space used internally for DMA transfers */
+AddressSpace dma_as;
+
 /** Number of bytes left in current DMA transfer */
 uint32_t transfer_cnt;
 
diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
index e258463747..d404f31e02 100644
--- a/hw/arm/allwinner-a10.c
+++ b/hw/arm/allwinner-a10.c
@@ -155,6 +155,8 @@ static void aw_a10_realize(DeviceState *dev, Error **errp)
 }
 
 /* SD/MMC */
+object_property_set_link(OBJECT(&s->mmc0), "dma-memory",
+ OBJECT(get_system_memory()), &error_fatal);
 sysbus_realize(SYS_BUS_DEVICE(&s->mmc0), &error_fatal);
 sysbus_mmio_map(SYS_BUS_DEVICE(&s->mmc0), 0, AW_A10_MMC0_BASE);
 sysbus_connect_irq(SYS_BUS_DEVICE(&s->mmc0), 0, qdev_get_gpio_in(dev, 32));
diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
index ff92ded82c..43a8d3dc48 100644
--- a/hw/arm/allwinner-h3.c
+++ b/hw/arm/allwinner-h3.c
@@ -349,6 +349,8 @@ static void allwinner_h3_realize(DeviceState *dev, Error 
**errp)
 sysbus_mmio_map(SYS_BUS_DEVICE(&s->sid), 0, s->memmap[AW_H3_SID]);
 
 /* SD/MMC */
+object_property_set_link(OBJECT(&s->mmc0), "dma-memory",
+ OBJECT(get_system_memory()), &error_fatal);
 sysbus_realize(SYS_BUS_DEVICE(&s->mmc0), &error_fatal);
 sysbus_mmio_map(SYS_BUS_DEVICE(&s->mmc0), 0, s->memmap[AW_H3_MMC0]);
 sysbus_connect_irq(SYS_BUS_DEVICE(&s->mmc0), 0,
diff --git a/hw/sd/allwinner-sdhost.c b/hw/sd/allwinner-sdhost.c
index f9eb92c09e..e82afb75eb 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,22 +334,24 @@ 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);
 sdbus_write_data(&s->sdbus, buf, buf_bytes);
 
 /* Read from SD bus */
 } else {
 sdbus_read_data(&s->sdbus, buf, buf_bytes);
-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;
 }
@@ -721,6 +726,12 @@ static const VMStateDescription vmstate_allwinner_sdhost = 
{
 }
 };
 
+static Property allwinner_sdhost_properties[] = {
+DEFINE_PROP_LINK("dma-memory", AwSdHostState, dma_mr,
+ TYPE_MEMORY_REGION, MemoryRegion *),
+DEFINE_PROP_END_OF_LIST(),
+};
+
 static void allwinner_sdhost_init(Object *obj)
 {
 AwSdHostState *s = A