[PATCH v1 1/1] block/file-posix: Avoid maybe-uninitialized warning

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" 

Avoid a maybe-uninitialized warning in raw_refresh_zoned_limits()
by initializing zoned.

With GCC 14.1.0:
In function ‘raw_refresh_zoned_limits’,
inlined from ‘raw_refresh_limits’ at ../qemu/block/file-posix.c:1522:5:
../qemu/block/file-posix.c:1405:17: error: ‘zoned’ may be used uninitialized 
[-Werror=maybe-uninitialized]
 1405 | if (ret < 0 || zoned == BLK_Z_NONE) {
  | ^~
../qemu/block/file-posix.c:1401:20: note: ‘zoned’ was declared here
 1401 | BlockZoneModel zoned;
  |^
cc1: all warnings being treated as errors

Signed-off-by: Edgar E. Iglesias 
---
 block/file-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index ff928b5e85..90fa54352c 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1398,7 +1398,7 @@ static void raw_refresh_zoned_limits(BlockDriverState 
*bs, struct stat *st,
  Error **errp)
 {
 BDRVRawState *s = bs->opaque;
-BlockZoneModel zoned;
+BlockZoneModel zoned = BLK_Z_NONE;
 int ret;
 
 ret = get_sysfs_zoned_model(st, &zoned);
-- 
2.43.0

[PATCH v1 0/1] block/file-posix: Avoid maybe-uninitialized warning

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" 

Hi,

I ran into the following build-warning when building QEMU
with GCC 14.1.0:
[925/1857] Compiling C object libblock.a.p/block_file-posix.c.o
FAILED: libblock.a.p/block_file-posix.c.o 
aarch64-poky-linux-gcc -mcpu=cortex-a57+crc -mbranch-protection=standard 
-fstack-protector-strong -O2 -D_FORTIFY_SOURCE=2 -Wformat -Wformat-security 
-Werror=format-security 
--sysroot=/opt/yoxen/arm64/sysroots/cortexa57-poky-linux -Ilibblock.a.p -I. 
-I../qemu -Iqapi -Itrace -Iui -Iui/shader -Iblock 
-I/opt/yoxen/arm64/sysroots/cortexa57-poky-linux/usr/include/glib-2.0 
-I/opt/yoxen/arm64/sysroots/cortexa57-poky-linux/usr/lib/glib-2.0/include 
-fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g 
-fstack-protector-strong -Wempty-body -Wendif-labels -Wexpansion-to-defined 
-Wformat-security -Wformat-y2k -Wignored-qualifiers -Wimplicit-fallthrough=2 
-Winit-self -Wmissing-format-attribute -Wmissing-prototypes -Wnested-externs 
-Wold-style-declaration -Wold-style-definition -Wredundant-decls -Wshadow=local 
-Wstrict-prototypes -Wtype-limits -Wundef -Wvla -Wwrite-strings 
-Wno-missing-include-dirs -Wno-psabi -Wno-shift-negative-value -isystem 
/home/edgar/src/c/qemu/qemu/linux-headers -isystem linux-headers -iquote . 
-iquote /home/edgar/src/c/qemu/qemu -iquote /home/edgar/src/c/qemu/qemu/include 
-iquote /home/edgar/src/c/qemu/qemu/host/include/aarch64 -iquote 
/home/edgar/src/c/qemu/qemu/host/include/generic -D_GNU_SOURCE 
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common 
-fwrapv -ftrivial-auto-var-init=zero -fzero-call-used-regs=used-gpr -O2 -pipe 
-g -feliminate-unused-debug-types -Os -fPIE -MD -MQ 
libblock.a.p/block_file-posix.c.o -MF libblock.a.p/block_file-posix.c.o.d -o 
libblock.a.p/block_file-posix.c.o -c ../qemu/block/file-posix.c
In function ‘raw_refresh_zoned_limits’,
inlined from ‘raw_refresh_limits’ at ../qemu/block/file-posix.c:1522:5:
../qemu/block/file-posix.c:1405:17: error: ‘zoned’ may be used uninitialized 
[-Werror=maybe-uninitialized]
 1405 | if (ret < 0 || zoned == BLK_Z_NONE) {
  | ^~
../qemu/block/file-posix.c:1401:20: note: ‘zoned’ was declared here
 1401 | BlockZoneModel zoned;
  |^
cc1: all warnings being treated as errors

It looks like a false positive and this fix avoids the warning.

Cheers,
Edgar

Edgar E. Iglesias (1):
  block/file-posix: Avoid maybe-uninitialized warning

 block/file-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.43.0

Re: [PATCH v2 08/13] hw/arm/xlnx-versal-virt: Replace drive_get_next() by drive_get()

[Edgar E. Iglesias]

On Wed, Nov 17, 2021 at 05:34:04PM +0100, Markus Armbruster wrote:
> drive_get_next() is basically a bad idea.  It returns the "next" block
> backend of a certain interface type.  "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
> 
> This lets you define unit numbers implicitly by execution order.  If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break.  Hard to spot in review.
> 
> Machine "xlnx-versal-virt" connects backends with drive_get_next() in
> a counting loop.  Change it to use drive_get() directly.  This makes
> the unit numbers explicit in the code.

Acked-by: Edgar E. Iglesias 


> 
> Cc: Alistair Francis 
> Cc: "Edgar E. Iglesias" 
> Cc: Peter Maydell 
> Cc: qemu-...@nongnu.org
> Signed-off-by: Markus Armbruster 
> ---
>  hw/arm/xlnx-versal-virt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
> index d2f55e29b6..0c5edc898e 100644
> --- a/hw/arm/xlnx-versal-virt.c
> +++ b/hw/arm/xlnx-versal-virt.c
> @@ -669,7 +669,8 @@ static void versal_virt_init(MachineState *machine)
>  
>  /* Plugin SD cards.  */
>  for (i = 0; i < ARRAY_SIZE(s->soc.pmc.iou.sd); i++) {
> -sd_plugin_card(&s->soc.pmc.iou.sd[i], drive_get_next(IF_SD));
> +sd_plugin_card(&s->soc.pmc.iou.sd[i],
> +   drive_get(IF_SD, 0, i));
>  }
>  
>  s->binfo.ram_size = machine->ram_size;
> -- 
> 2.31.1
> 

Re: [PATCH v2 09/13] hw/microblaze: Replace drive_get_next() by drive_get()

[Edgar E. Iglesias]

On Wed, Nov 17, 2021 at 05:34:05PM +0100, Markus Armbruster wrote:
> drive_get_next() is basically a bad idea.  It returns the "next" block
> backend of a certain interface type.  "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
> 
> This lets you define unit numbers implicitly by execution order.  If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break.  Hard to spot in review.
> 
> Machine "petalogix-ml605" connects backends with drive_get_next() in a
> counting loop.  Change it to use drive_get() directly.  This makes the
> unit numbers explicit in the code.


Acked-by: Edgar E. Iglesias 


> 
> Cc: "Edgar E. Iglesias" 
> Signed-off-by: Markus Armbruster 
> ---
>  hw/microblaze/petalogix_ml605_mmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/microblaze/petalogix_ml605_mmu.c 
> b/hw/microblaze/petalogix_ml605_mmu.c
> index 159db6cbe2..a24fadddca 100644
> --- a/hw/microblaze/petalogix_ml605_mmu.c
> +++ b/hw/microblaze/petalogix_ml605_mmu.c
> @@ -183,7 +183,7 @@ petalogix_ml605_init(MachineState *machine)
>  spi = (SSIBus *)qdev_get_child_bus(dev, "spi");
>  
>  for (i = 0; i < NUM_SPI_FLASHES; i++) {
> -DriveInfo *dinfo = drive_get_next(IF_MTD);
> +DriveInfo *dinfo = drive_get(IF_MTD, 0, i);
>  qemu_irq cs_line;
>  
>  dev = qdev_new("n25q128");
> -- 
> 2.31.1
> 

Re: [PATCH v2 11/13] hw/arm/xilinx_zynq: Replace drive_get_next() by drive_get()

[Edgar E. Iglesias]

On Wed, Nov 17, 2021 at 05:34:07PM +0100, Markus Armbruster wrote:
> drive_get_next() is basically a bad idea.  It returns the "next" block
> backend of a certain interface type.  "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
> 
> This lets you define unit numbers implicitly by execution order.  If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break.  Hard to spot in review.
> 
> Machine "xlnx-zcu102" connects backends with drive_get_next() in two
> counting loops, one of them in a helper function.  Change it to use
> drive_get() directly.  This makes the unit numbers explicit in the
> code.


Acked-by: Edgar E. Iglesias 


> 
> Cc: "Edgar E. Iglesias" 
> Cc: Alistair Francis 
> Cc: Peter Maydell 
> Cc: qemu-...@nongnu.org
> Signed-off-by: Markus Armbruster 
> ---
>  hw/arm/xilinx_zynq.c | 16 +---
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 69c333e91b..50e7268396 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -125,9 +125,10 @@ static void gem_init(NICInfo *nd, uint32_t base, 
> qemu_irq irq)
>  sysbus_connect_irq(s, 0, irq);
>  }
>  
> -static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
> - bool is_qspi)
> +static inline int zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
> +bool is_qspi, int unit0)
>  {
> +int unit = unit0;
>  DeviceState *dev;
>  SysBusDevice *busdev;
>  SSIBus *spi;
> @@ -156,7 +157,7 @@ static inline void zynq_init_spi_flashes(uint32_t 
> base_addr, qemu_irq irq,
>  spi = (SSIBus *)qdev_get_child_bus(dev, bus_name);
>  
>  for (j = 0; j < num_ss; ++j) {
> -DriveInfo *dinfo = drive_get_next(IF_MTD);
> +DriveInfo *dinfo = drive_get(IF_MTD, 0, unit++);
>  flash_dev = qdev_new("n25q128");
>  if (dinfo) {
>  qdev_prop_set_drive_err(flash_dev, "drive",
> @@ -170,6 +171,7 @@ static inline void zynq_init_spi_flashes(uint32_t 
> base_addr, qemu_irq irq,
>  }
>  }
>  
> +return unit;
>  }
>  
>  static void zynq_init(MachineState *machine)
> @@ -247,9 +249,9 @@ static void zynq_init(MachineState *machine)
>  pic[n] = qdev_get_gpio_in(dev, n);
>  }
>  
> -zynq_init_spi_flashes(0xE0006000, pic[58-IRQ_OFFSET], false);
> -zynq_init_spi_flashes(0xE0007000, pic[81-IRQ_OFFSET], false);
> -zynq_init_spi_flashes(0xE000D000, pic[51-IRQ_OFFSET], true);
> +n = zynq_init_spi_flashes(0xE0006000, pic[58 - IRQ_OFFSET], false, 0);
> +n = zynq_init_spi_flashes(0xE0007000, pic[81 - IRQ_OFFSET], false, n);
> +n = zynq_init_spi_flashes(0xE000D000, pic[51 - IRQ_OFFSET], true, n);
>  
>  sysbus_create_simple(TYPE_CHIPIDEA, 0xE0002000, pic[53 - IRQ_OFFSET]);
>  sysbus_create_simple(TYPE_CHIPIDEA, 0xE0003000, pic[76 - IRQ_OFFSET]);
> @@ -298,7 +300,7 @@ static void zynq_init(MachineState *machine)
>  sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, hci_addr);
>  sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[hci_irq - 
> IRQ_OFFSET]);
>  
> -di = drive_get_next(IF_SD);
> +di = drive_get(IF_SD, 0, n);
>  blk = di ? blk_by_legacy_dinfo(di) : NULL;
>  carddev = qdev_new(TYPE_SD_CARD);
>  qdev_prop_set_drive_err(carddev, "drive", blk, &error_fatal);
> -- 
> 2.31.1
> 

Re: [PATCH v2 10/13] hw/arm/xlnx-zcu102: Replace drive_get_next() by drive_get()

[Edgar E. Iglesias]

On Wed, Nov 17, 2021 at 05:34:06PM +0100, Markus Armbruster wrote:
> drive_get_next() is basically a bad idea.  It returns the "next" block
> backend of a certain interface type.  "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
> 
> This lets you define unit numbers implicitly by execution order.  If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break.  Hard to spot in review.
> 
> Machine "xlnx-zcu102" connects backends with drive_get_next() in
> several counting loops.  Change it to use drive_get() directly.  This
> makes the unit numbers explicit in the code.


Acked-by: Edgar E. Iglesias 



> 
> Cc: Alistair Francis 
> Cc: "Edgar E. Iglesias" 
> Cc: Peter Maydell 
> Cc: qemu-...@nongnu.org
> Signed-off-by: Markus Armbruster 
> ---
>  hw/arm/xlnx-zcu102.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> index 3dc2b5e8ca..45eb19ab3b 100644
> --- a/hw/arm/xlnx-zcu102.c
> +++ b/hw/arm/xlnx-zcu102.c
> @@ -169,7 +169,7 @@ static void xlnx_zcu102_init(MachineState *machine)
>  /* Create and plug in the SD cards */
>  for (i = 0; i < XLNX_ZYNQMP_NUM_SDHCI; i++) {
>  BusState *bus;
> -DriveInfo *di = drive_get_next(IF_SD);
> +DriveInfo *di = drive_get(IF_SD, 0, i);
>  BlockBackend *blk = di ? blk_by_legacy_dinfo(di) : NULL;
>  DeviceState *carddev;
>  char *bus_name;
> @@ -190,7 +190,7 @@ static void xlnx_zcu102_init(MachineState *machine)
>  BusState *spi_bus;
>  DeviceState *flash_dev;
>  qemu_irq cs_line;
> -DriveInfo *dinfo = drive_get_next(IF_MTD);
> +DriveInfo *dinfo = drive_get(IF_MTD, 0, i);
>  gchar *bus_name = g_strdup_printf("spi%d", i);
>  
>  spi_bus = qdev_get_child_bus(DEVICE(&s->soc), bus_name);
> @@ -212,7 +212,7 @@ static void xlnx_zcu102_init(MachineState *machine)
>  BusState *spi_bus;
>  DeviceState *flash_dev;
>  qemu_irq cs_line;
> -DriveInfo *dinfo = drive_get_next(IF_MTD);
> +DriveInfo *dinfo = drive_get(IF_MTD, 0, XLNX_ZYNQMP_NUM_SPIS + i);
>  int bus = i / XLNX_ZYNQMP_NUM_QSPI_BUS_CS;
>  gchar *bus_name = g_strdup_printf("qspi%d", bus);
>  
> -- 
> 2.31.1
> 

Re: [RFC PATCH 15/15] arm: xlnx-versal: Add emmc to versal

[Edgar E. Iglesias]

On Fri, Feb 12, 2021 at 01:37:18PM -0800, Alistair Francis wrote:
> On Thu, Feb 11, 2021 at 12:36 AM Sai Pavan Boddu
>  wrote:
> >
> > Configuring SDHCI-0 to act as eMMC controller.
> >
> > Signed-off-by: Sai Pavan Boddu 
> 
> Reviewed-by: Alistair Francis 
> 
> Alistair



Hi Sai,

It would be great, if EMMC somehow could be made optional.
In any case, I think this is OK!

Reviewed-by: Edgar E. Iglesias 

Could you please also add an example command-line in 
docs/system/arm/xlnx-versal-virt.rst?

Thanks,
Edgar



> 
> > ---
> >  hw/arm/xlnx-versal-virt.c | 16 +++-
> >  hw/arm/xlnx-versal.c  | 14 --
> >  2 files changed, 23 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
> > index 8482cd6..18489e4 100644
> > --- a/hw/arm/xlnx-versal-virt.c
> > +++ b/hw/arm/xlnx-versal-virt.c
> > @@ -333,6 +333,13 @@ static void fdt_add_sd_nodes(VersalVirt *s)
> >  qemu_fdt_setprop_sized_cells(s->fdt, name, "reg",
> >   2, addr, 2, MM_PMC_SD0_SIZE);
> >  qemu_fdt_setprop(s->fdt, name, "compatible", compat, 
> > sizeof(compat));
> > +/*
> > + * eMMC specific properties
> > + */
> > +if (i == 0) {
> > +qemu_fdt_setprop(s->fdt, name, "non-removable", NULL, 0);
> > +qemu_fdt_setprop_sized_cells(s->fdt, name, "bus-width", 1, 8);
> > +}
> >  g_free(name);
> >  }
> >  }
> > @@ -512,7 +519,7 @@ static void create_virtio_regions(VersalVirt *s)
> >  }
> >  }
> >
> > -static void sd_plugin_card(SDHCIState *sd, DriveInfo *di)
> > +static void sd_plugin_card(SDHCIState *sd, DriveInfo *di, bool emmc)
> >  {
> >  BlockBackend *blk = di ? blk_by_legacy_dinfo(di) : NULL;
> >  DeviceState *card;
> > @@ -520,6 +527,7 @@ static void sd_plugin_card(SDHCIState *sd, DriveInfo 
> > *di)
> >  card = qdev_new(TYPE_SD_CARD);
> >  object_property_add_child(OBJECT(sd), "card[*]", OBJECT(card));
> >  qdev_prop_set_drive_err(card, "drive", blk, &error_fatal);
> > +object_property_set_bool(OBJECT(card), "emmc", emmc, &error_fatal);
> >  qdev_realize_and_unref(card, qdev_get_child_bus(DEVICE(sd), "sd-bus"),
> > &error_fatal);
> >  }
> > @@ -528,7 +536,6 @@ static void versal_virt_init(MachineState *machine)
> >  {
> >  VersalVirt *s = XLNX_VERSAL_VIRT_MACHINE(machine);
> >  int psci_conduit = QEMU_PSCI_CONDUIT_DISABLED;
> > -int i;
> >
> >  /*
> >   * If the user provides an Operating System to be loaded, we expect 
> > them
> > @@ -581,10 +588,9 @@ static void versal_virt_init(MachineState *machine)
> >  memory_region_add_subregion_overlap(get_system_memory(),
> >  0, &s->soc.fpd.apu.mr, 0);
> >
> > +sd_plugin_card(&s->soc.pmc.iou.sd[0], drive_get_next(IF_EMMC), true);
> >  /* Plugin SD cards.  */
> > -for (i = 0; i < ARRAY_SIZE(s->soc.pmc.iou.sd); i++) {
> > -sd_plugin_card(&s->soc.pmc.iou.sd[i], drive_get_next(IF_SD));
> > -}
> > +sd_plugin_card(&s->soc.pmc.iou.sd[1], drive_get_next(IF_SD), false);
> >
> >  s->binfo.ram_size = machine->ram_size;
> >  s->binfo.loader_start = 0x0;
> > diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
> > index b077716..3498dd9 100644
> > --- a/hw/arm/xlnx-versal.c
> > +++ b/hw/arm/xlnx-versal.c
> > @@ -230,9 +230,14 @@ static void versal_create_admas(Versal *s, qemu_irq 
> > *pic)
> >  }
> >
> >  #define SDHCI_CAPABILITIES  0x280737ec6481 /* Same as on ZynqMP.  */
> > +#define SDHCI0_CAPS ((SDHCI_CAPABILITIES & ~(3 << 30)) | \
> > + (1 << 30))
> > +#define SDHCI1_CAPS SDHCI_CAPABILITIES
> > +
> >  static void versal_create_sds(Versal *s, qemu_irq *pic)
> >  {
> >  int i;
> > +uint64_t caps[] = {SDHCI0_CAPS, SDHCI1_CAPS};
> >
> >  for (i = 0; i < ARRAY_SIZE(s->pmc.iou.sd); i++) {
> >  DeviceState *dev;
> > @@ -244,9 +249,14 @@ static void versal_create_sds(Versal *s, qemu_irq *pic)
> >
> >  object_property_set_uint(OBJECT(dev), "sd-spec-version", 3,
> >   &error_fatal);
> > -object_property_set_uint(OBJECT(dev), "capareg", 
> > SDHCI_CAPABILITIES,
> > +object_property_set_uint(OBJECT(dev), "capareg", caps[i],
> >   &error_fatal);
> > -object_property_set_uint(OBJECT(dev), "uhs", UHS_I, &error_fatal);
> > +/*
> > + * UHS is not applicable for eMMC
> > + */
> > +if (i == 1) {
> > +object_property_set_uint(OBJECT(dev), "uhs", UHS_I, 
> > &error_fatal);
> > +}
> >  sysbus_realize(SYS_BUS_DEVICE(dev), &error_fatal);
> >
> >  mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
> > --
> > 2.7.4
> >
> >

Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands

[Edgar E. Iglesias]

t dummy cycles as bytes.
> > > > > >
> > > > > > Yes, something like that. My concern is that since this
> functionality has been
> > > > > > in tree for while, users have found known or unknown features
> that got
> > > > > > introduced by it. By removing the functionality (and the
> known/uknown features)
> > > > > > we are riscing to brake our user's use cases (currently I'm
> aware of one
> > > > > > feature/use case but it is not unlikely that there are more).
> [1] states that
> > > > > > "In general features are intended to be supported indefinitely
> once introduced
> > > > > > into QEMU", to me that makes very much sense because the
> opposite would mean
> > > > > > that we were not reliable. So in case [1] needs to be honored it
> looks to be
> > > > > > safer to add functionality instead of removing (and riscing the
> removal of use
> > > > > > cases/features). Luckily I still believe in this case that it
> will be easier to
> > > > > > go forward (even if I also agree on what you are saying below
> about what I
> > > > > > proposed).
> > > > > >
> > > > >
> > > > > Even if the implementation is buggy and we need to keep the buggy
> > > > > implementation forever? I think that's why
> > > > > qemu/docs/system/deprecated.rst was created for deprecating such
> > > > > feature.
> > > >
> > > > With the RFC I posted all commands in m25p80 are working for both
> the case 1
> > > > controller (using a txfifo) and the case 2 controller (no txfifo, as
> GQSPI).
> > > > Because of this, I, with all respect, will have to disagree that
> this is buggy.
> > >
> > > Well, the existing m25p80 implementation that uses dummy cycle
> > > accuracy for those flashes prevents all SPI controllers that use tx
> > > fifo to work with those flashes. Hence it is buggy.
> > >
> > > >
> > > > >
> > > > > > >
> > > > > > > > >
> > > > > > > > > > don't think it is fair to call them 'seriously broken'
> (and else we should
> > > > > > > > > > probably let the maintainers know about it). Most likely
> the lack of support
> > > > > > > > >
> > > > > > > > > I called it "seriously broken" because current
> implementation only
> > > > > > > > > considered one type of SPI controllers while completely
> ignoring the
> > > > > > > > > other type.
> > > > > > > >
> > > > > > > > If we change view and see this from the perspective of
> m25p80, it models the
> > > > > > > > commands a certain way and provides an API that the SPI
> controllers need to
> > > > > > > > implement for interacting with it. It is true that there are
> SPI controllers
> > > > > > > > referred to above that do not support the portion of that
> API that corresponds
> > > > > > > > to commands with dummy clock cycles, but I don't think it is
> true that this is
> > > > > > > > broken since there is also one SPI controller that has a
> working implementation
> > > > > > > > of m25p80's full API also when transfering through a tx fifo
> (use case 1). But
> > > > > > > > as mentioned above, by doing a minor extension and
> improvement to m25p80's API
> > > > > > > > and allow for toggling the accuracy from dummy clock cycles
> to dummy bytes [1]
> > > > > > > > will still be honored as in the same time making it possible
> to have full
> > > > > > > > support for the API in the SPI controllers that currently do
> not (please reread
> > > > > > > > the proposal in my previous reply that attempts to do this).
> I myself see this
> > > > > > > > as win/win situation, also because no controller should need
> modifications.
> > > > > > > >
> > > > > > >
> > > > > > > I am afraid your proposal does not work. Your proposed new
> device
> > > > > > > property 'model_dummy_bytes' to select to convert the accurate
> dummy
> > > > > > > clock cycle count to dummy bytes inside m25p80, is hard to
> justify as
> > > > > > > a property to the flash itself, as the behavior is tightly
> coupled to
> > > > > > > how the SPI controller works.
> > > > > >
> > > > > > I agree on above. I decided though that instead of posting
> sample code in here
> > > > > > I'll post an RFC with hopefully an improved proposal. I'll cc
> you. About below,
> > > > > > Xilinx ZynqMP GQSPI should not need any modication in a first
> step.
> > > > > >
> > > > >
> > > > > Wait, (see below)
> > > > >
> > > > > > >
> > > > > > > Please take a look at the Xilinx GQSPI controller, which
> supports both
> > > > > > > use cases, that the dummy cycles can be transferred via tx
> fifo, or
> > > > > > > generated by the controller automatically. Please read the
> example
> > > > > > > given in:
> > > > > > >
> > > > > > > table 24‐22, an example of Generic FIFO Contents for Quad
> I/O Read
> > > > > > > Command (EBh)
> > > > > > >
> > > > > > > in
> https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf
> > > > > > >
> > > > > > > If you choose to set the m25p80 device property
> 'model_dummy_bytes' to
> > > > > > > true when working with the Xilinx GQSPI controller, you are
> bound to
> > > > > > > only allow guest software to use tx fifo to transfer the dummy
> cycles,
> > > > > > > and this is wrong.
> > > > > > >
> > > > >
> > > > > You missed this part. I looked at your RFC, and as I mentioned
> above
> > > > > your proposal cannot support the complicated controller like Xilinx
> > > > > GQSPI. Please read the example of table 24-22. With your RFC, you
> > > > > mandate guest software's GQSPI driver to only use hardware dummy
> cycle
> > > > > generation, which is wrong.
> > > > >
> > > >
> > > > First, thank you very much for looking into the RFC series, very much
> > > > appreciated. Secondly, about above, the GQSPI model in QEMU
> transfers from 2
> > > > locations in the file, in 1 location the transfer referred to above
> is done, in
> > > > another location the transfer through the txfifo is done. The
> location where
> > > > transfer referred to above is done will not need any modifications
> (and will
> > > > thus work equally well as it does currently).
> > >
> > > Please explain this a little bit. How does your RFC series handle
> > > cases as described in table 24-22, where the 6 dummy cycles are split
> > > into 2 transfers, with one transfer using tx fifo, and the other one
> > > using hardware dummy cycle generation?
> >
> > Sorry, I missunderstod. You are right, that won't work.
>
> +Edgar E. Iglesias
>
> So it looks by far the only way to implement dummy cycles correctly to
> work with all SPI controller models is what I proposed here in this
> patch series.
>
> Maintainers are quite silent, so I would like to hear your thoughts.
>
> @Alistair Francis @Philippe Mathieu-Daudé @Peter Maydell would you
> please share your thoughts since you are the one who reviewed the
> existing dummy implementation (based on commits history)
>
>
Francisco really knows this stuff better than me
I would tend to agree that it's unfortunate to model things in cycles, if
we could abstract things at a higher level that would be nice. Without
breaking existing use-cases.
Francisco, is it impossible to bring up the abstraction level to bytes and
keep existing use-cases?

We have a bunch of test-cases, We'll publish some of them in source code,
others we can't publish since they use proprietary SW we're not allowed to
publish at all, but we can run tests and Ack if things work.

Best regards,
Edgar

Re: [PATCH 00/13] dma: Let the DMA API take MemTxAttrs argument and propagate MemTxResult

[Edgar E. Iglesias]

On Wed, 16 Sep 2020, 15:48 Philippe Mathieu-Daudé, 
wrote:

> On 9/4/20 5:44 PM, Philippe Mathieu-Daudé wrote:
> > Salvaging cleanups patches from the RFC series "Forbid DMA write
> > accesses to MMIO regions" [*], propagating MemTxResult and
> > adding documentation.
> >
> > Philippe Mathieu-Daudé (12):
> >   dma: Let dma_memory_valid() take MemTxAttrs argument
> >   dma: Let dma_memory_set() take MemTxAttrs argument
> >   dma: Let dma_memory_rw_relaxed() take MemTxAttrs argument
> >   dma: Let dma_memory_rw() take MemTxAttrs argument
> >   dma: Let dma_memory_read/write() take MemTxAttrs argument
> >   dma: Let dma_memory_map() take MemTxAttrs argument
>
> Talking with Laszlo, he wonders if we shouldn't enforce setting
> MemTxAttrs attrs.secure = 0 in these calls.
>
> Is there a concept of "secure DMA controller" in QEMU?
>
> Thanks,
>
> Phil.
>

Hi,

Yes, we have models of secure DMA devices out of tree. Actually, on the
ZynqMP and Versal SoCs, there are secure registers that can configure any
DMA device to issue secure or non-secure transactions at runtime. We just
haven't modelled all of the control regs that allow that in upstream QEMU.

Cheers,
Edgar

Re: [PATCH 00/13] dma: Let the DMA API take MemTxAttrs argument and propagate MemTxResult

[Edgar E. Iglesias]

On Fri, Sep 04, 2020 at 05:44:26PM +0200, Philippe Mathieu-Daudé wrote:
> Salvaging cleanups patches from the RFC series "Forbid DMA write
> accesses to MMIO regions" [*], propagating MemTxResult and
> adding documentation.
> 
> [*] https://www.mail-archive.com/qemu-block@nongnu.org/msg72924.html

On the whole series:
Reviewed-by: Edgar E. Iglesias 



> 
> Klaus Jensen (1):
>   pci: pass along the return value of dma_memory_rw
> 
> Philippe Mathieu-Daudé (12):
>   docs/devel/loads-stores: Add regexp for DMA functions
>   dma: Document address_space_map/address_space_unmap() prototypes
>   dma: Let dma_memory_set() propagate MemTxResult
>   dma: Let dma_memory_rw() propagate MemTxResult
>   dma: Let dma_memory_read() propagate MemTxResult
>   dma: Let dma_memory_write() propagate MemTxResult
>   dma: Let dma_memory_valid() take MemTxAttrs argument
>   dma: Let dma_memory_set() take MemTxAttrs argument
>   dma: Let dma_memory_rw_relaxed() take MemTxAttrs argument
>   dma: Let dma_memory_rw() take MemTxAttrs argument
>   dma: Let dma_memory_read/write() take MemTxAttrs argument
>   dma: Let dma_memory_map() take MemTxAttrs argument
> 
>  docs/devel/loads-stores.rst   |   2 +
>  include/hw/pci/pci.h  |   7 +-
>  include/hw/ppc/spapr_vio.h|  11 ++-
>  include/sysemu/dma.h  | 156 +++---
>  dma-helpers.c |  16 ++--
>  hw/arm/musicpal.c |  13 +--
>  hw/arm/smmu-common.c  |   3 +-
>  hw/arm/smmuv3.c   |  14 +--
>  hw/core/generic-loader.c  |   3 +-
>  hw/display/virtio-gpu.c   |   8 +-
>  hw/dma/pl330.c|  12 ++-
>  hw/dma/sparc32_dma.c  |  16 ++--
>  hw/dma/xlnx-zynq-devcfg.c |   6 +-
>  hw/dma/xlnx_dpdma.c   |  10 ++-
>  hw/hyperv/vmbus.c |   8 +-
>  hw/i386/amd_iommu.c   |  16 ++--
>  hw/i386/intel_iommu.c |  28 +++---
>  hw/ide/ahci.c |   9 +-
>  hw/ide/macio.c|   2 +-
>  hw/intc/spapr_xive.c  |   3 +-
>  hw/intc/xive.c|   7 +-
>  hw/misc/bcm2835_property.c|   3 +-
>  hw/misc/macio/mac_dbdma.c |  10 ++-
>  hw/net/allwinner-sun8i-emac.c |  21 +++--
>  hw/net/ftgmac100.c|  25 --
>  hw/net/imx_fec.c  |  32 ---
>  hw/nvram/fw_cfg.c |  12 ++-
>  hw/pci-host/pnv_phb3.c|   5 +-
>  hw/pci-host/pnv_phb3_msi.c|   9 +-
>  hw/pci-host/pnv_phb4.c|   7 +-
>  hw/sd/allwinner-sdhost.c  |  14 +--
>  hw/sd/sdhci.c |  35 +---
>  hw/usb/hcd-dwc2.c |   8 +-
>  hw/usb/hcd-ehci.c |   6 +-
>  hw/usb/hcd-ohci.c |  28 +++---
>  hw/usb/libhw.c|   3 +-
>  hw/virtio/virtio.c|   6 +-
>  37 files changed, 385 insertions(+), 189 deletions(-)
> 
> -- 
> 2.26.2
> 

Re: [PATCH 03/13] dma: Document address_space_map/address_space_unmap() prototypes

[Edgar E. Iglesias]

On Fri, Sep 04, 2020 at 05:44:29PM +0200, Philippe Mathieu-Daudé wrote:
> Add documentation based on address_space_map / address_space_unmap.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/sysemu/dma.h | 30 +-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index 80c5bc3e02d..19bc9ad1b69 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -1,7 +1,7 @@
>  /*
>   * DMA helper functions
>   *
> - * Copyright (c) 2009 Red Hat
> + * Copyright (c) 2009, 2020 Red Hat
>   *
>   * This work is licensed under the terms of the GNU General Public License
>   * (GNU GPL), version 2 or later.
> @@ -125,6 +125,20 @@ static inline int dma_memory_write(AddressSpace *as, 
> dma_addr_t addr,
>  
>  int dma_memory_set(AddressSpace *as, dma_addr_t addr, uint8_t c, dma_addr_t 
> len);
>  
> +/**
> + * address_space_map: Map a physical memory region into a DMA controller
> + *virtual address

It may be easier to understand this if you change DMA controller virtual address
to host virtual address.

Either way:
Reviewed-by: Edgar E. Iglesias 



> + *
> + * May map a subset of the requested range, given by and returned in @plen.
> + * May return %NULL and set *@plen to zero(0), if resources needed to perform
> + * the mapping are exhausted.
> + * Use only for reads OR writes - not for read-modify-write operations.
> + *
> + * @as: #AddressSpace to be accessed
> + * @addr: address within that address space
> + * @len: pointer to length of buffer; updated on return
> + * @dir: indicates the transfer direction
> + */
>  static inline void *dma_memory_map(AddressSpace *as,
> dma_addr_t addr, dma_addr_t *len,
> DMADirection dir)
> @@ -138,6 +152,20 @@ static inline void *dma_memory_map(AddressSpace *as,
>  return p;
>  }
>  
> +/**
> + * address_space_unmap: Unmaps a memory region previously mapped
> + *  by dma_memory_map()
> + *
> + * Will also mark the memory as dirty if @dir == %DMA_DIRECTION_FROM_DEVICE.
> + * @access_len gives the amount of memory that was actually read or written
> + * by the caller.
> + *
> + * @as: #AddressSpace used
> + * @buffer: host pointer as returned by address_space_map()
> + * @len: buffer length as returned by address_space_map()
> + * @dir: indicates the transfer direction
> + * @access_len: amount of data actually transferred
> + */
>  static inline void dma_memory_unmap(AddressSpace *as,
>  void *buffer, dma_addr_t len,
>  DMADirection dir, dma_addr_t access_len)
> -- 
> 2.26.2
> 

Re: [RFC PATCH 00/12] hw: Forbid DMA write accesses to MMIO regions

[Edgar E. Iglesias]

On Thu, Sep 03, 2020 at 07:53:33PM +0200, Paolo Bonzini wrote:
> On 03/09/20 17:50, Edgar E. Iglesias wrote:
> >>> Hmm, I guess it would make sense to have a configurable option in KVM
> >>> to isolate passthrough devices so they only can DMA to guest RAM...
> >>
> >> Passthrough devices are always protected by the IOMMU, anything else
> >> would be obviously insane^H^H^Hecure. :)
> > 
> > Really? To always do that blindly seems wrong.
> > 
> > I'm refering to the passthrough device not being able to reach registers
> > of other passthrough devices within the same guest.
> 
> Ah okay; sorry, I misunderstood.  That makes more sense now!
> 
> Multiple devices are put in the same IOMMU "container" (page table
> basically), and that takes care of reaching registers of other
> passthrough devices.

Thanks, yes, that's a sane default. What I was trying to say before is that
it may make sense to allow the user to "harden" the setup by selectivly
putting certain passthrough devs on a separate group that can *only*
DMA access guest RAM (not other device regs).

Some devs need access to other device's regs but many passthrough devs don't
need DMA access to anything else but RAM (e.g an Ethernet MAC).

That could mitigate the damage caused by wild DMA pointers...

Cheers,
Edgar

Re: [RFC PATCH 12/12] dma: Assert when device writes to indirect memory (such MMIO regions)

[Edgar E. Iglesias]

On Thu, Sep 03, 2020 at 01:08:31PM +0200, Philippe Mathieu-Daudé wrote:
> Assert DMA accesses are done on direct memory (in particular
> to catch invalid accesses to MMIO regions).

Hi Philippe,

Is the motivation for this to make it easier to find DMA programming errors?
Shouldn't guest SW use the IOMMU/DMA-APIs to debug those?

There're valid use-cases where DMA devices target non-memory, in particular
in the embedded space but also on PCI systems.

Also, since guest SW programs the DMA registers, guest SW would be able to trig 
this assert at will...

Cheers,
Edgar




> 
> Example with the reproducer from LP#1886362 (see previous commit):
> 
>   qemu-system-i386: include/sysemu/dma.h:111: int dma_memory_rw(AddressSpace 
> *, dma_addr_t, void *, dma_addr_t, DMADirection, MemTxAttrs): Assertion `dir 
> == DMA_DIRECTION_TO_DEVICE || attrs.direct_access' failed.
>   (gdb) bt
>   #0  0x751d69e5 in raise () at /lib64/libc.so.6
>   #1  0x751bf895 in abort () at /lib64/libc.so.6
>   #2  0x751bf769 in _nl_load_domain.cold () at /lib64/libc.so.6
>   #3  0x751cee76 in annobin_assert.c_end () at /lib64/libc.so.6
>   #4  0x57b48a94 in dma_memory_rw (as=0x7fffddd3ca28, addr=4064, 
> buf=0x7fff7780, len=16, dir=DMA_DIRECTION_FROM_DEVICE, attrs=...) at 
> /home/phil/source/qemu/include/sysemu/dma.h:111
>   #5  0x57b487e0 in pci_dma_rw (dev=0x7fffddd3c800, addr=4064, 
> buf=0x7fff7780, len=16, dir=DMA_DIRECTION_FROM_DEVICE) at 
> /home/phil/source/qemu/include/hw/pci/pci.h:791
>   #6  0x57b47373 in pci_dma_write (dev=0x7fffddd3c800, addr=4064, 
> buf=0x7fff7780, len=16) at /home/phil/source/qemu/include/hw/pci/pci.h:804
>   #7  0x57b340b4 in e1000e_write_packet_to_guest 
> (core=0x7fffddd3f4e0, pkt=0x6116c740, rxr=0x7fff7cf0, 
> rss_info=0x7fff7d10) at hw/net/e1000e_core.c:1609
>   #8  0x57b30739 in e1000e_receive_iov (core=0x7fffddd3f4e0, 
> iov=0x61960e80, iovcnt=4) at hw/net/e1000e_core.c:1709
>   #9  0x576e2069 in e1000e_nc_receive_iov (nc=0x6140a060, 
> iov=0x61960e80, iovcnt=4) at hw/net/e1000e.c:213
>   #10 0x572a3c34 in net_tx_pkt_sendv (pkt=0x63128800, 
> nc=0x6140a060, iov=0x61960e80, iov_cnt=4) at hw/net/net_tx_pkt.c:556
>   #11 0x572a23e2 in net_tx_pkt_send (pkt=0x63128800, 
> nc=0x6140a060) at hw/net/net_tx_pkt.c:633
>   #12 0x572a4c67 in net_tx_pkt_send_loopback (pkt=0x63128800, 
> nc=0x6140a060) at hw/net/net_tx_pkt.c:646
>   #13 0x57b70b05 in e1000e_tx_pkt_send (core=0x7fffddd3f4e0, 
> tx=0x7fffddd5f748, queue_index=0) at hw/net/e1000e_core.c:664
>   #14 0x57b6eab8 in e1000e_process_tx_desc (core=0x7fffddd3f4e0, 
> tx=0x7fffddd5f748, dp=0x7fff8680, queue_index=0) at 
> hw/net/e1000e_core.c:743
>   #15 0x57b6d65d in e1000e_start_xmit (core=0x7fffddd3f4e0, 
> txr=0x7fff88a0) at hw/net/e1000e_core.c:934
>   #16 0x57b5ea38 in e1000e_set_tctl (core=0x7fffddd3f4e0, index=256, 
> val=255) at hw/net/e1000e_core.c:2431
>   #17 0x57b369ef in e1000e_core_write (core=0x7fffddd3f4e0, 
> addr=1027, val=255, size=4) at hw/net/e1000e_core.c:3265
>   #18 0x576de3be in e1000e_mmio_write (opaque=0x7fffddd3c800, 
> addr=1027, val=255, size=4) at hw/net/e1000e.c:109
>   #19 0x58e6b789 in memory_region_write_accessor (mr=0x7fffddd3f110, 
> addr=1027, value=0x7fff8eb0, size=4, shift=0, mask=4294967295, attrs=...) 
> at softmmu/memory.c:483
>   #20 0x58e6b05b in access_with_adjusted_size (addr=1027, 
> value=0x7fff8eb0, size=1, access_size_min=4, access_size_max=4, 
> access_fn= 0x58e6b120 , mr=0x7fffddd3f110, 
> attrs=...) at softmmu/memory.c:544
>   #21 0x58e69776 in memory_region_dispatch_write (mr=0x7fffddd3f110, 
> addr=1027, data=255, op=MO_8, attrs=...) at softmmu/memory.c:1465
>   #22 0x58f60462 in flatview_write_continue (fv=0x6063f9e0, 
> addr=3775005699, attrs=..., ptr=0x602e3710, len=1, addr1=1027, l=1, 
> mr=0x7fffddd3f110) at exec.c:3176
>   #23 0x58f4e38b in flatview_write (fv=0x6063f9e0, 
> addr=3775005699, attrs=..., buf=0x602e3710, len=1) at exec.c:3220
>   #24 0x58f4dd4f in address_space_write (as=0x6080baa0, 
> addr=3775005699, attrs=..., buf=0x602e3710, len=1) at exec.c:3315
>   #25 0x5916b3e0 in qtest_process_command (chr=0x5c03f300 
> , words=0x60458150) at softmmu/qtest.c:567
>   #26 0x5915f7f2 in qtest_process_inbuf (chr=0x5c03f300 
> , inbuf=0x619200e0) at softmmu/qtest.c:710
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/sysemu/dma.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index 8a7dbf0b0f3..a4ba9438a56 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -108,6 +108,8 @@ static inline int dma_memory_rw(AddressSpace *as, 
> dma_addr_t addr,
>  

Re: [RFC PATCH 00/12] hw: Forbid DMA write accesses to MMIO regions

[Edgar E. Iglesias]

On Thu, Sep 03, 2020 at 05:46:39PM +0200, Paolo Bonzini wrote:
> On 03/09/20 16:24, Edgar E. Iglesias wrote:
> >> [*] I do wonder about hardware-device-passthrough setups; I
> >> don't think I would care to pass through an arbitrary device
> >> to an untrusted guest...
> > Hmm, I guess it would make sense to have a configurable option in KVM
> > to isolate passthrough devices so they only can DMA to guest RAM...
> 
> Passthrough devices are always protected by the IOMMU, anything else
> would be obviously insane^H^H^Hecure. :)


Really? To always do that blindly seems wrong.

I'm refering to the passthrough device not being able to reach registers
of other passthrough devices within the same guest.

Obviously the IOMMU should be setup so that passthrough devices don't reach\
other guests or the host.

Cheers,
Edgar

Re: [RFC PATCH 00/12] hw: Forbid DMA write accesses to MMIO regions

[Edgar E. Iglesias]

On Thu, Sep 03, 2020 at 02:58:19PM +0100, Peter Maydell wrote:
> On Thu, 3 Sep 2020 at 14:37, Laszlo Ersek  wrote:
> > Peter mentions an approach at the end of
> >  that I believe
> > to understand, but -- according to him -- it seems too much work.
> 
> It also would only be effective for MMIO, not for qemu_irq lines...
> 
> > I don't think such chains work unto arbitrary depths on physical
> > hardware either.
> 
> Real hardware by and large doesn't get designed with this kind
> of DMA-to-self as a consideration either, but unfortunately it's
> not really very useful as a model to base QEMU's behaviour on:
> 
>  (1) real hardware is usually massively parallel, so the logic
>   that handles incoming MMIO is decoupled anyway from logic
>   that does outgoing DMA. (Arguably the "do all DMA in a
>   bottom-half" idea is kind of following the hardware design.)
>   Similarly simple "raise this outbound signal" logic just
>   works as an instantaneous action that causes the device on
>   the other end to change its state/do something parallel,
>   whereas for QEMU we need to actually call some code in the
>   device on the other end and so we serialize this stuff,
>   sandwiching a bit of "device B code" in the middle of a
>   run of "device A code". So a lot more of this stuff "just
>   happens to work" on h/w than we get with QEMU.
>  (2) if software running on real h/w does do something silly with
>   programming a device to DMA to itself then the worst case is
>   generally that they manage to wedge that device (or the whole
>   machine, if you're really unlucky), in which case the response
>   is "don't do that then". There isn't the same "guest code
>   can escape the VM" security boundary that QEMU needs to guard
>   against [*].
> 
> [*] I do wonder about hardware-device-passthrough setups; I
> don't think I would care to pass through an arbitrary device
> to an untrusted guest...

Hmm, I guess it would make sense to have a configurable option in KVM
to isolate passthrough devices so they only can DMA to guest RAM...

Cheers,
Edgar

Re: [PATCH v2 2/2] util/hexdump: Reorder qemu_hexdump() arguments

[Edgar E. Iglesias]

On Sat, Aug 22, 2020 at 08:09:50PM +0200, Philippe Mathieu-Daudé wrote:
> qemu_hexdump()'s pointer to the buffer and length of the
> buffer are closely related arguments but are widely separated
> in the argument list order (also, the format of 
> function prototypes is usually to have the FILE* argument
> coming first).
> 
> Reorder the arguments as "fp, prefix, buf, size" which is
> more logical.
> 
> Suggested-by: Peter Maydell 
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Edgar E. Iglesias 



> ---
>  include/qemu-common.h|  4 ++--
>  hw/dma/xlnx_dpdma.c  |  2 +-
>  hw/net/fsl_etsec/etsec.c |  2 +-
>  hw/net/fsl_etsec/rings.c |  2 +-
>  hw/sd/sd.c   |  2 +-
>  hw/usb/redirect.c|  2 +-
>  net/colo-compare.c   | 24 
>  net/net.c|  2 +-
>  util/hexdump.c   |  4 ++--
>  util/iov.c   |  2 +-
>  10 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 6834883822f..9cfd62669bf 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -138,8 +138,8 @@ int os_parse_cmd_args(int index, const char *optarg);
>   * Hexdump a buffer to a file. An optional string prefix is added to every 
> line
>   */
>  
> -void qemu_hexdump(const void *bufptr, FILE *fp,
> -  const char *prefix, size_t size);
> +void qemu_hexdump(FILE *fp, const char *prefix,
> +  const void *bufptr, size_t size);
>  
>  /*
>   * helper to parse debug environment variables
> diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
> index d75a8069426..967548abd31 100644
> --- a/hw/dma/xlnx_dpdma.c
> +++ b/hw/dma/xlnx_dpdma.c
> @@ -388,7 +388,7 @@ static void xlnx_dpdma_dump_descriptor(DPDMADescriptor 
> *desc)
>  {
>  if (DEBUG_DPDMA) {
>  qemu_log("DUMP DESCRIPTOR:\n");
> -qemu_hexdump(desc, stdout, "", sizeof(DPDMADescriptor));
> +qemu_hexdump(stdout, "", desc, sizeof(DPDMADescriptor));
>  }
>  }
>  
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index c817a28decd..c5edb25dc9f 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -357,7 +357,7 @@ static ssize_t etsec_receive(NetClientState *nc,
>  
>  #if defined(HEX_DUMP)
>  fprintf(stderr, "%s receive size:%zd\n", nc->name, size);
> -qemu_hexdump(buf, stderr, "", size);
> +qemu_hexdump(stderr, "", buf, size);
>  #endif
>  /* Flush is unnecessary as are already in receiving path */
>  etsec->need_flush = false;
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index 337a55fc957..628648a9c37 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -269,7 +269,7 @@ static void process_tx_bd(eTSEC *etsec,
>  
>  #if defined(HEX_DUMP)
>  qemu_log("eTSEC Send packet size:%d\n", etsec->tx_buffer_len);
> -qemu_hexdump(etsec->tx_buffer, stderr, "", etsec->tx_buffer_len);
> +qemu_hexdump(stderr, "", etsec->tx_buffer, etsec->tx_buffer_len);
>  #endif  /* ETSEC_RING_DEBUG */
>  
>  if (etsec->first_bd.flags & BD_TX_TOEUN) {
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 190e4cf1232..6508939b1f4 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1781,7 +1781,7 @@ send_response:
>  }
>  
>  #ifdef DEBUG_SD
> -qemu_hexdump(response, stderr, "Response", rsplen);
> +qemu_hexdump(stderr, "Response", response, rsplen);
>  #endif
>  
>  return rsplen;
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 09edb0d81c0..48f38d33912 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -240,7 +240,7 @@ static void usbredir_log_data(USBRedirDevice *dev, const 
> char *desc,
>  if (dev->debug < usbredirparser_debug_data) {
>  return;
>  }
> -qemu_hexdump(data, stderr, desc, len);
> +qemu_hexdump(stderr, desc, data, len);
>  }
>  
>  /*
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 550272b3baa..4a5ed642e9a 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -494,10 +494,10 @@ sec:
>  g_queue_push_head(&conn->secondary_list, spkt);
>  
>  if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> -qemu_hexdump(ppkt->data, stderr,
> -"colo-compare ppkt", ppkt->size);
> -qemu_hexdump(spkt->data, stderr,
> -  

Re: [PATCH v2 1/2] util/hexdump: Convert to take a void pointer argument

[Edgar E. Iglesias]

On Sat, Aug 22, 2020 at 08:09:49PM +0200, Philippe Mathieu-Daudé wrote:
> Most uses of qemu_hexdump() do not take an array of char
> as input, forcing use of cast. Since we can use this
> helper to dump any kind of buffer, use a pointer to void
> argument instead.
> 
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Edgar E. Iglesias 




> ---
> Since v1:
> - renamed argument 'bufptr' (Peter Maydell)
> ---
>  include/qemu-common.h|  3 ++-
>  hw/dma/xlnx_dpdma.c  |  2 +-
>  hw/net/fsl_etsec/etsec.c |  2 +-
>  hw/sd/sd.c   |  2 +-
>  hw/usb/redirect.c|  2 +-
>  net/colo-compare.c   | 12 ++--
>  net/net.c|  2 +-
>  util/hexdump.c   |  4 +++-
>  8 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index bb9496bd80f..6834883822f 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -138,7 +138,8 @@ int os_parse_cmd_args(int index, const char *optarg);
>   * Hexdump a buffer to a file. An optional string prefix is added to every 
> line
>   */
>  
> -void qemu_hexdump(const char *buf, FILE *fp, const char *prefix, size_t 
> size);
> +void qemu_hexdump(const void *bufptr, FILE *fp,
> +  const char *prefix, size_t size);
>  
>  /*
>   * helper to parse debug environment variables
> diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
> index b40c897de2c..d75a8069426 100644
> --- a/hw/dma/xlnx_dpdma.c
> +++ b/hw/dma/xlnx_dpdma.c
> @@ -388,7 +388,7 @@ static void xlnx_dpdma_dump_descriptor(DPDMADescriptor 
> *desc)
>  {
>  if (DEBUG_DPDMA) {
>  qemu_log("DUMP DESCRIPTOR:\n");
> -qemu_hexdump((char *)desc, stdout, "", sizeof(DPDMADescriptor));
> +qemu_hexdump(desc, stdout, "", sizeof(DPDMADescriptor));
>  }
>  }
>  
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index 7035cf4eb97..c817a28decd 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -357,7 +357,7 @@ static ssize_t etsec_receive(NetClientState *nc,
>  
>  #if defined(HEX_DUMP)
>  fprintf(stderr, "%s receive size:%zd\n", nc->name, size);
> -qemu_hexdump((void *)buf, stderr, "", size);
> +qemu_hexdump(buf, stderr, "", size);
>  #endif
>  /* Flush is unnecessary as are already in receiving path */
>  etsec->need_flush = false;
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index fad9cf1ee7a..190e4cf1232 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1781,7 +1781,7 @@ send_response:
>  }
>  
>  #ifdef DEBUG_SD
> -qemu_hexdump((const char *)response, stderr, "Response", rsplen);
> +qemu_hexdump(response, stderr, "Response", rsplen);
>  #endif
>  
>  return rsplen;
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 417a60a2e68..09edb0d81c0 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -240,7 +240,7 @@ static void usbredir_log_data(USBRedirDevice *dev, const 
> char *desc,
>  if (dev->debug < usbredirparser_debug_data) {
>  return;
>  }
> -qemu_hexdump((char *)data, stderr, desc, len);
> +qemu_hexdump(data, stderr, desc, len);
>  }
>  
>  /*
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 2c20de1537d..550272b3baa 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -494,9 +494,9 @@ sec:
>  g_queue_push_head(&conn->secondary_list, spkt);
>  
>  if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> -qemu_hexdump((char *)ppkt->data, stderr,
> +qemu_hexdump(ppkt->data, stderr,
>  "colo-compare ppkt", ppkt->size);
> -qemu_hexdump((char *)spkt->data, stderr,
> +qemu_hexdump(spkt->data, stderr,
>  "colo-compare spkt", spkt->size);
>  }
>  
> @@ -535,9 +535,9 @@ static int colo_packet_compare_udp(Packet *spkt, Packet 
> *ppkt)
>  trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
>  trace_colo_compare_udp_miscompare("Secondary pkt size", spkt->size);
>  if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> -qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> +qemu_hexdump(ppkt->data, stderr, "colo-compare pri pkt",
>   ppkt->size);
> -qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
&g

Re: [Qemu-block] [Qemu-devel] [qemu-s390x] [PATCH v7 33/42] exec: Replace device_endian with MemOp

[Edgar E. Iglesias]

On Mon, 19 Aug. 2019, 23:01 Richard Henderson, 
wrote:

> On 8/19/19 11:29 AM, Paolo Bonzini wrote:
> > On 19/08/19 20:28, Paolo Bonzini wrote:
> >> On 16/08/19 12:12, Thomas Huth wrote:
> >>> This patch is *huge*, more than 800kB. It keeps being stuck in the the
> >>> filter of the qemu-s390x list each time you send it. Please:
> >>>
> >>> 1) Try to break it up in more digestible pieces, e.g. change only one
> >>> subsystem at a time (this is also better reviewable by people who are
> >>> interested in one area)
> >>
> >> This is not really possible, since the patch is basically a
> >> search-and-replace.  You could perhaps use some magic
> >> ("DEVICE_MEMOP_ENDIAN" or something like that) to allow a split, but it
> >> would introduce more complication than anything else.
> >
> > I'm stupid, at this point of the series it _would_ be possible to split
> > the patch by subsystem.  Still not sure it would be actually an
> advantage.
>
> It might be easier to review if we split by symbol, one rename per patch
> over
> the entire code base.
>
>
> r~
>

Or if we review your script (I assume this wasn't a manual change). I'm not
sure it's realistic to have review on the entire patch or patches.

Best regards,
Edgar

>

Re: [Qemu-block] [PATCH] tests: Avoid non-portable 'echo -ARG'

[Edgar E. Iglesias]

On Wed, Jun 28, 2017 at 09:21:37AM -0500, Eric Blake wrote:
> POSIX says that backslashes in the arguments to 'echo', as well as
> any use of 'echo -n' and 'echo -e', are non-portable; it recommends
> people should favor 'printf' instead.  This is definitely true where
> we do not control which shell is running (such as in makefile snippets
> or in documentation examples).  But even for scripts where we
> require bash (and therefore, where echo does what we want by default),
> it is still possible to use 'shopt -s xpg_echo' to change bash's
> behavior of echo.  And setting a good example never hurts when we are
> not sure if a snippet will be copied from a bash-only script to a
> general shell script (although I don't change the use of non-portable
> \e for ESC when we know the running shell is bash).
> 
> Replace 'echo -n "..."' with 'printf "..."', and 'echo -e "..."'
> with 'printf "...\n"'.
> 
> In the qemu-iotests check script, also fix unusual shell quoting
> that would result in word-splitting if 'date' outputs a space.

Reviewed-by: Edgar E. Iglesias 



> 
> Signed-off-by: Eric Blake 
> ---
> 
> Of course, Stefan's pending patch:
> [PATCH 3/5] qemu-iotests: 068: extract _qemu() function
> also touches 068, so there may be some (obvious) merge conflicts
> to resolve there depending on what goes in first.
> 
>  qemu-options.hx |  4 ++--
>  tests/multiboot/run_test.sh | 10 +-
>  tests/qemu-iotests/051  |  7 ---
>  tests/qemu-iotests/068  |  2 +-
>  tests/qemu-iotests/142  | 48 
> ++---
>  tests/qemu-iotests/171  | 14 ++---
>  tests/qemu-iotests/check| 18 -
>  tests/rocker/all| 10 +-
>  tests/tcg/cris/Makefile |  8 
>  9 files changed, 61 insertions(+), 60 deletions(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 896ff17..c8205bb 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4351,7 +4351,7 @@ The simplest (insecure) usage is to provide the secret 
> inline
> 
>  The simplest secure usage is to provide the secret via a file
> 
> - # echo -n "letmein" > mypasswd.txt
> + # printf "letmein" > mypasswd.txt
>   # $QEMU -object secret,id=sec0,file=mypasswd.txt,format=raw
> 
>  For greater security, AES-256-CBC should be used. To illustrate usage,
> @@ -4379,7 +4379,7 @@ telling openssl to base64 encode the result, but it 
> could be left
>  as raw bytes if desired.
> 
>  @example
> - # SECRET=$(echo -n "letmein" |
> + # SECRET=$(printf "letmein" |
>  openssl enc -aes-256-cbc -a -K $KEY -iv $IV)
>  @end example
> 
> diff --git a/tests/multiboot/run_test.sh b/tests/multiboot/run_test.sh
> index 78d7edf..35bfe0e 100755
> --- a/tests/multiboot/run_test.sh
> +++ b/tests/multiboot/run_test.sh
> @@ -26,7 +26,7 @@ run_qemu() {
>  local kernel=$1
>  shift
> 
> -echo -e "\n\n=== Running test case: $kernel $@ ===\n" >> test.log
> +printf "\n\n=== Running test case: $kernel $@ ===\n\n" >> test.log
> 
>  $QEMU \
>  -kernel $kernel \
> @@ -68,21 +68,21 @@ for t in mmap modules; do
>  pass=1
> 
>  if [ $debugexit != 1 ]; then
> -echo -e "\e[31m ?? \e[0m $t (no debugexit used, exit code $ret)"
> +printf "\e[31m ?? \e[0m $t (no debugexit used, exit code $ret)\n"
>  pass=0
>  elif [ $ret != 0 ]; then
> -echo -e "\e[31mFAIL\e[0m $t (exit code $ret)"
> +printf "\e[31mFAIL\e[0m $t (exit code $ret)\n"
>  pass=0
>  fi
> 
>  if ! diff $t.out test.log > /dev/null 2>&1; then
> -echo -e "\e[31mFAIL\e[0m $t (output difference)"
> +printf "\e[31mFAIL\e[0m $t (output difference)\n"
>  diff -u $t.out test.log
>  pass=0
>  fi
> 
>  if [ $pass == 1 ]; then
> -echo -e "\e[32mPASS\e[0m $t"
> +printf "\e[32mPASS\e[0m $t\n"
>  fi
> 
>  done
> diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
> index 26c29de..322c4a8 100755
> --- a/tests/qemu-iotests/051
> +++ b/tests/qemu-iotests/051
> @@ -217,7 +217,7 @@ run_qemu -drive driver=null-co,cache=invalid_value
>  # Test 142 checks the direct=on cases
> 
>  for cache in writeback writethrough unsafe invalid_value; do
> -echo -e "info block\ninfo block file\ninfo block backing\ninfo block 
> backing-file&q