[PATCH v3 0/3] various: Remove unnecessary casts

[Philippe Mathieu-Daudé]

Remove unnecessary casts using coccinelle scripts.

The CPU()/OBJECT() patches don't introduce logical change,
The DEVICE() one removes various OBJECT_CHECK() calls.

Since v3:
- Fixed patch #2 description (Markus)
- Add A-b/R-b tags

Since v2:
- Reword description (Markus)
- Add A-b/R-b tags

Philippe Mathieu-Daudé (3):
  target: Remove unnecessary CPU() cast
  various: Remove unnecessary OBJECT() cast
  hw: Remove unnecessary DEVICE() cast

 hw/core/bus.c   | 2 +-
 hw/display/artist.c | 2 +-
 hw/display/cg3.c| 2 +-
 hw/display/sm501.c  | 2 +-
 hw/display/tcx.c| 4 ++--
 hw/display/vga-isa.c| 2 +-
 hw/i2c/imx_i2c.c| 2 +-
 hw/i2c/mpc_i2c.c| 2 +-
 hw/ide/ahci-allwinner.c | 2 +-
 hw/ide/piix.c   | 2 +-
 hw/ipmi/smbus_ipmi.c| 2 +-
 hw/microblaze/petalogix_ml605_mmu.c | 8 
 hw/misc/macio/pmu.c | 2 +-
 hw/net/ftgmac100.c  | 3 +--
 hw/net/imx_fec.c| 2 +-
 hw/nubus/nubus-device.c | 2 +-
 hw/pci-host/bonito.c| 2 +-
 hw/ppc/spapr.c  | 2 +-
 hw/s390x/sclp.c | 2 +-
 hw/sh4/sh_pci.c | 2 +-
 hw/xen/xen-legacy-backend.c | 2 +-
 monitor/misc.c  | 3 +--
 qom/object.c| 4 ++--
 target/ppc/mmu_helper.c | 2 +-
 24 files changed, 29 insertions(+), 31 deletions(-)

-- 
2.21.3

[PATCH v3 2/3] various: Remove unnecessary OBJECT() cast

[Philippe Mathieu-Daudé]

The OBJECT() macro is defined as:

  #define OBJECT(obj) ((Object *)(obj))

Remove the unnecessary OBJECT() casts when we already know the
pointer is of Object type.

Patch created mechanically using spatch with this script:

  @@
  typedef Object;
  Object *o;
  @@
  -   OBJECT(o)
  +   o

Acked-by: Cornelia Huck 
Acked-by: Corey Minyard 
Acked-by: John Snow 
Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/core/bus.c   | 2 +-
 hw/ide/ahci-allwinner.c | 2 +-
 hw/ipmi/smbus_ipmi.c| 2 +-
 hw/microblaze/petalogix_ml605_mmu.c | 8 
 hw/s390x/sclp.c | 2 +-
 monitor/misc.c  | 3 +--
 qom/object.c| 4 ++--
 7 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/hw/core/bus.c b/hw/core/bus.c
index 3dc0a825f0..4ea5870de8 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -25,7 +25,7 @@
 
 void qbus_set_hotplug_handler(BusState *bus, Object *handler, Error **errp)
 {
-object_property_set_link(OBJECT(bus), OBJECT(handler),
+object_property_set_link(OBJECT(bus), handler,
  QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
 }
 
diff --git a/hw/ide/ahci-allwinner.c b/hw/ide/ahci-allwinner.c
index bb8393d2b6..8536b9eb5a 100644
--- a/hw/ide/ahci-allwinner.c
+++ b/hw/ide/ahci-allwinner.c
@@ -90,7 +90,7 @@ static void allwinner_ahci_init(Object *obj)
 SysbusAHCIState *s = SYSBUS_AHCI(obj);
 AllwinnerAHCIState *a = ALLWINNER_AHCI(obj);
 
-memory_region_init_io(&a->mmio, OBJECT(obj), &allwinner_ahci_mem_ops, a,
+memory_region_init_io(&a->mmio, obj, &allwinner_ahci_mem_ops, a,
   "allwinner-ahci", ALLWINNER_AHCI_MMIO_SIZE);
 memory_region_add_subregion(&s->ahci.mem, ALLWINNER_AHCI_MMIO_OFF,
 &a->mmio);
diff --git a/hw/ipmi/smbus_ipmi.c b/hw/ipmi/smbus_ipmi.c
index 2a9470d9df..f1a0148755 100644
--- a/hw/ipmi/smbus_ipmi.c
+++ b/hw/ipmi/smbus_ipmi.c
@@ -329,7 +329,7 @@ static void smbus_ipmi_init(Object *obj)
 {
 SMBusIPMIDevice *sid = SMBUS_IPMI(obj);
 
-ipmi_bmc_find_and_link(OBJECT(obj), (Object **) &sid->bmc);
+ipmi_bmc_find_and_link(obj, (Object **) &sid->bmc);
 }
 
 static void smbus_ipmi_get_fwinfo(struct IPMIInterface *ii, IPMIFwInfo *info)
diff --git a/hw/microblaze/petalogix_ml605_mmu.c 
b/hw/microblaze/petalogix_ml605_mmu.c
index 0a2640c40b..52dcea9abd 100644
--- a/hw/microblaze/petalogix_ml605_mmu.c
+++ b/hw/microblaze/petalogix_ml605_mmu.c
@@ -150,9 +150,9 @@ petalogix_ml605_init(MachineState *machine)
 qdev_set_nic_properties(eth0, &nd_table[0]);
 qdev_prop_set_uint32(eth0, "rxmem", 0x1000);
 qdev_prop_set_uint32(eth0, "txmem", 0x1000);
-object_property_set_link(OBJECT(eth0), OBJECT(ds),
+object_property_set_link(OBJECT(eth0), ds,
  "axistream-connected", &error_abort);
-object_property_set_link(OBJECT(eth0), OBJECT(cs),
+object_property_set_link(OBJECT(eth0), cs,
  "axistream-control-connected", &error_abort);
 qdev_init_nofail(eth0);
 sysbus_mmio_map(SYS_BUS_DEVICE(eth0), 0, AXIENET_BASEADDR);
@@ -163,9 +163,9 @@ petalogix_ml605_init(MachineState *machine)
 cs = object_property_get_link(OBJECT(eth0),
   "axistream-control-connected-target", NULL);
 qdev_prop_set_uint32(dma, "freqhz", 100 * 100);
-object_property_set_link(OBJECT(dma), OBJECT(ds),
+object_property_set_link(OBJECT(dma), ds,
  "axistream-connected", &error_abort);
-object_property_set_link(OBJECT(dma), OBJECT(cs),
+object_property_set_link(OBJECT(dma), cs,
  "axistream-control-connected", &error_abort);
 qdev_init_nofail(dma);
 sysbus_mmio_map(SYS_BUS_DEVICE(dma), 0, AXIDMA_BASEADDR);
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index ede056b3ef..4132286db7 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -322,7 +322,7 @@ void s390_sclp_init(void)
 
 object_property_add_child(qdev_get_machine(), TYPE_SCLP, new,
   NULL);
-object_unref(OBJECT(new));
+object_unref(new);
 qdev_init_nofail(DEVICE(new));
 }
 
diff --git a/monitor/misc.c b/monitor/misc.c
index 9723b466cd..f5207cd242 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -1837,8 +1837,7 @@ void object_add_completion(ReadLineState *rs, int 
nb_args, const char *str)
 static int qdev_add_hotpluggable_device(Object *obj, void *opaque)
 {
 GSList **list = opaque;
-DeviceState *dev = (DeviceState *)object_dynamic_cast(OBJECT(obj),
-  TYPE_DEVICE);
+DeviceState *dev = (DeviceState *)object_dynamic_cast(obj, TYPE_DEVICE);
 
 if (dev == NULL) {
 return 0;
diff --git a/qom/object.c b/qom/object.c
index be700e831f..07c1443d0e 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -762,7 +762,7 @@ Object *ob

[PATCH v3 3/3] hw: Remove unnecessary DEVICE() cast

[Philippe Mathieu-Daudé]

The DEVICE() macro is defined as:

  #define DEVICE(obj) OBJECT_CHECK(DeviceState, (obj), TYPE_DEVICE)

which expands to:

  ((DeviceState *)object_dynamic_cast_assert((Object *)(obj), (name),
 __FILE__, __LINE__,
 __func__))

This assertion can only fail when @obj points to something other
than its stated type, i.e. when we're in undefined behavior country.

Remove the unnecessary DEVICE() casts when we already know the
pointer is of DeviceState type.

Patch created mechanically using spatch with this script:

  @@
  typedef DeviceState;
  DeviceState *s;
  @@
  -   DEVICE(s)
  +   s

Acked-by: David Gibson 
Acked-by: Paul Durrant 
Reviewed-by: Markus Armbruster 
Reviewed-by: Cédric Le Goater 
Acked-by: John Snow 
Reviewed-by: Richard Henderson 
Reviewed-by: Markus Armbruster 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/display/artist.c | 2 +-
 hw/display/cg3.c| 2 +-
 hw/display/sm501.c  | 2 +-
 hw/display/tcx.c| 4 ++--
 hw/display/vga-isa.c| 2 +-
 hw/i2c/imx_i2c.c| 2 +-
 hw/i2c/mpc_i2c.c| 2 +-
 hw/ide/piix.c   | 2 +-
 hw/misc/macio/pmu.c | 2 +-
 hw/net/ftgmac100.c  | 3 +--
 hw/net/imx_fec.c| 2 +-
 hw/nubus/nubus-device.c | 2 +-
 hw/pci-host/bonito.c| 2 +-
 hw/ppc/spapr.c  | 2 +-
 hw/sh4/sh_pci.c | 2 +-
 hw/xen/xen-legacy-backend.c | 2 +-
 16 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/hw/display/artist.c b/hw/display/artist.c
index 753dbb9a77..7e2a4556bd 100644
--- a/hw/display/artist.c
+++ b/hw/display/artist.c
@@ -1353,7 +1353,7 @@ static void artist_realizefn(DeviceState *dev, Error 
**errp)
 s->cursor_height = 32;
 s->cursor_width = 32;
 
-s->con = graphic_console_init(DEVICE(dev), 0, &artist_ops, s);
+s->con = graphic_console_init(dev, 0, &artist_ops, s);
 qemu_console_resize(s->con, s->width, s->height);
 }
 
diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index a1ede10394..f7f1c199ce 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -321,7 +321,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp)
 
 sysbus_init_irq(sbd, &s->irq);
 
-s->con = graphic_console_init(DEVICE(dev), 0, &cg3_ops, s);
+s->con = graphic_console_init(dev, 0, &cg3_ops, s);
 qemu_console_resize(s->con, s->width, s->height);
 }
 
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index de0ab9d977..2a564889bd 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1839,7 +1839,7 @@ static void sm501_init(SM501State *s, DeviceState *dev,
 &s->twoD_engine_region);
 
 /* create qemu graphic console */
-s->con = graphic_console_init(DEVICE(dev), 0, &sm501_ops, s);
+s->con = graphic_console_init(dev, 0, &sm501_ops, s);
 }
 
 static const VMStateDescription vmstate_sm501_state = {
diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 76de16e8ea..1fb45b1aab 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -868,9 +868,9 @@ static void tcx_realizefn(DeviceState *dev, Error **errp)
 sysbus_init_irq(sbd, &s->irq);
 
 if (s->depth == 8) {
-s->con = graphic_console_init(DEVICE(dev), 0, &tcx_ops, s);
+s->con = graphic_console_init(dev, 0, &tcx_ops, s);
 } else {
-s->con = graphic_console_init(DEVICE(dev), 0, &tcx24_ops, s);
+s->con = graphic_console_init(dev, 0, &tcx24_ops, s);
 }
 s->thcmisc = 0;
 
diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
index 0633ed382c..3aaeeeca1e 100644
--- a/hw/display/vga-isa.c
+++ b/hw/display/vga-isa.c
@@ -74,7 +74,7 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
 0x000a,
 vga_io_memory, 1);
 memory_region_set_coalescing(vga_io_memory);
-s->con = graphic_console_init(DEVICE(dev), 0, s->hw_ops, s);
+s->con = graphic_console_init(dev, 0, s->hw_ops, s);
 
 memory_region_add_subregion(isa_address_space(isadev),
 VBE_DISPI_LFB_PHYSICAL_ADDRESS,
diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c
index 30b9aea247..2e02e1c4fa 100644
--- a/hw/i2c/imx_i2c.c
+++ b/hw/i2c/imx_i2c.c
@@ -305,7 +305,7 @@ static void imx_i2c_realize(DeviceState *dev, Error **errp)
   IMX_I2C_MEM_SIZE);
 sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
 sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
-s->bus = i2c_init_bus(DEVICE(dev), NULL);
+s->bus = i2c_init_bus(dev, NULL);
 }
 
 static void imx_i2c_class_init(ObjectClass *klass, void *data)
diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc_i2c.c
index 0aa1be3ce7..9a724f3a3e 100644
--- a/hw/i2c/mpc_i2c.c
+++ b/hw/i2c/mpc_i2c.c
@@ -332,7 +332,7 @@ static void mpc_i2c_realize(DeviceState *dev, Error **errp)
 memory_region_init_io(&i2c->iomem, OBJECT(i2c), &i2c_ops, i2c,
   

[PATCH v3 1/3] target: Remove unnecessary CPU() cast

[Philippe Mathieu-Daudé]

The CPU() macro is defined as:

  #define CPU(obj) ((CPUState *)(obj))

which expands to:

  ((CPUState *)object_dynamic_cast_assert((Object *)(obj), (name),
  __FILE__, __LINE__, __func__))

This assertion can only fail when @obj points to something other
than its stated type, i.e. when we're in undefined behavior country.

Remove the unnecessary CPU() casts when we already know the pointer
is of CPUState type.

Patch created mechanically using spatch with this script:

  @@
  typedef CPUState;
  CPUState *s;
  @@
  -   CPU(s)
  +   s

Acked-by: David Gibson 
Reviewed-by: Cédric Le Goater 
Reviewed-by: Richard Henderson 
Reviewed-by: Markus Armbruster 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/ppc/mmu_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
index 86c667b094..8972714775 100644
--- a/target/ppc/mmu_helper.c
+++ b/target/ppc/mmu_helper.c
@@ -1820,7 +1820,7 @@ static inline void do_invalidate_BAT(CPUPPCState *env, 
target_ulong BATu,
 if (((end - base) >> TARGET_PAGE_BITS) > 1024) {
 /* Flushing 1024 4K pages is slower than a complete flush */
 LOG_BATS("Flush all BATs\n");
-tlb_flush(CPU(cs));
+tlb_flush(cs);
 LOG_BATS("Flush done\n");
 return;
 }
-- 
2.21.3

Re: [PATCH v2 0/3] qom: Few trivial patches

[Philippe Mathieu-Daudé]

ping?

On 5/4/20 1:56 PM, Philippe Mathieu-Daudé wrote:

Some QOM patches worth salvaging while doing housekeeping.

Since v1:
- Fixed test build failure (patchew)

Philippe Mathieu-Daudé (3):
   qom/object: Move Object typedef to 'qemu/typedefs.h'
   io/task: Move 'qom/object.h' header to source
   qom/object: Make reparenting error more verbose

  include/io/task.h | 2 --
  include/qemu/typedefs.h   | 1 +
  include/qom/object.h  | 2 --
  include/qom/qom-qobject.h | 2 --
  include/sysemu/sysemu.h   | 1 -
  io/task.c | 1 +
  qom/object.c  | 7 ++-
  tests/test-io-task.c  | 1 +
  8 files changed, 9 insertions(+), 8 deletions(-)

[PATCH v3 0/2] scripts: More Python fixes

[Philippe Mathieu-Daudé]

Trivial Python3 fixes, again...

Since v2:
- Remove patch updating MAINTAINERS

Since v1:
- Added Alex Bennée A-b tags
- Addressed John Snow review comments
  - Use /usr/bin/env
  - Do not modify os.path (dropped last patch)

Philippe Mathieu-Daudé (2):
  scripts/qemugdb: Remove shebang header
  scripts/qmp: Use Python 3 interpreter

 scripts/qemugdb/__init__.py  | 3 +--
 scripts/qemugdb/aio.py   | 3 +--
 scripts/qemugdb/coroutine.py | 3 +--
 scripts/qemugdb/mtree.py | 4 +---
 scripts/qemugdb/tcg.py   | 1 -
 scripts/qmp/qom-get  | 2 +-
 scripts/qmp/qom-list | 2 +-
 scripts/qmp/qom-set  | 2 +-
 scripts/qmp/qom-tree | 2 +-
 9 files changed, 8 insertions(+), 14 deletions(-)

-- 
2.21.3

[PATCH v3 1/2] scripts/qemugdb: Remove shebang header

[Philippe Mathieu-Daudé]

These scripts are loaded as plugin by GDB (and they don't
have any __main__ entry point). Remove the shebang header.

Acked-by: Alex Bennée 
Signed-off-by: Philippe Mathieu-Daudé 
---
 scripts/qemugdb/__init__.py  | 3 +--
 scripts/qemugdb/aio.py   | 3 +--
 scripts/qemugdb/coroutine.py | 3 +--
 scripts/qemugdb/mtree.py | 4 +---
 scripts/qemugdb/tcg.py   | 1 -
 5 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/scripts/qemugdb/__init__.py b/scripts/qemugdb/__init__.py
index 969f552b26..da8ff612e5 100644
--- a/scripts/qemugdb/__init__.py
+++ b/scripts/qemugdb/__init__.py
@@ -1,5 +1,4 @@
-#!/usr/bin/python
-
+#
 # GDB debugging support
 #
 # Copyright (c) 2015 Linaro Ltd
diff --git a/scripts/qemugdb/aio.py b/scripts/qemugdb/aio.py
index 2ba00c..d7c1ba0c28 100644
--- a/scripts/qemugdb/aio.py
+++ b/scripts/qemugdb/aio.py
@@ -1,5 +1,4 @@
-#!/usr/bin/python
-
+#
 # GDB debugging support: aio/iohandler debug
 #
 # Copyright (c) 2015 Red Hat, Inc.
diff --git a/scripts/qemugdb/coroutine.py b/scripts/qemugdb/coroutine.py
index 41e079d0e2..db61389022 100644
--- a/scripts/qemugdb/coroutine.py
+++ b/scripts/qemugdb/coroutine.py
@@ -1,5 +1,4 @@
-#!/usr/bin/python
-
+#
 # GDB debugging support
 #
 # Copyright 2012 Red Hat, Inc. and/or its affiliates
diff --git a/scripts/qemugdb/mtree.py b/scripts/qemugdb/mtree.py
index 3030a60d3f..8fe42c3c12 100644
--- a/scripts/qemugdb/mtree.py
+++ b/scripts/qemugdb/mtree.py
@@ -1,5 +1,4 @@
-#!/usr/bin/python
-
+#
 # GDB debugging support
 #
 # Copyright 2012 Red Hat, Inc. and/or its affiliates
@@ -84,4 +83,3 @@ def print_item(self, ptr, offset = gdb.Value(0), level = 0):
 while not isnull(subregion):
 self.print_item(subregion, addr, level)
 subregion = subregion['subregions_link']['tqe_next']
-
diff --git a/scripts/qemugdb/tcg.py b/scripts/qemugdb/tcg.py
index 18880fc9a7..16c03c06a9 100644
--- a/scripts/qemugdb/tcg.py
+++ b/scripts/qemugdb/tcg.py
@@ -1,4 +1,3 @@
-#!/usr/bin/python
 # -*- coding: utf-8 -*-
 #
 # GDB debugging support, TCG status
-- 
2.21.3

[PATCH v3 2/2] scripts/qmp: Use Python 3 interpreter

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 scripts/qmp/qom-get  | 2 +-
 scripts/qmp/qom-list | 2 +-
 scripts/qmp/qom-set  | 2 +-
 scripts/qmp/qom-tree | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/scripts/qmp/qom-get b/scripts/qmp/qom-get
index 007b4cd442..7c5ede91bb 100755
--- a/scripts/qmp/qom-get
+++ b/scripts/qmp/qom-get
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python3
 ##
 # QEMU Object Model test tools
 #
diff --git a/scripts/qmp/qom-list b/scripts/qmp/qom-list
index 03bda3446b..bb68fd65d4 100755
--- a/scripts/qmp/qom-list
+++ b/scripts/qmp/qom-list
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python3
 ##
 # QEMU Object Model test tools
 #
diff --git a/scripts/qmp/qom-set b/scripts/qmp/qom-set
index c37fe78b00..19881d85e9 100755
--- a/scripts/qmp/qom-set
+++ b/scripts/qmp/qom-set
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python3
 ##
 # QEMU Object Model test tools
 #
diff --git a/scripts/qmp/qom-tree b/scripts/qmp/qom-tree
index 1c8acf61e7..fa91147a03 100755
--- a/scripts/qmp/qom-tree
+++ b/scripts/qmp/qom-tree
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python3
 ##
 # QEMU Object Model test tools
 #
-- 
2.21.3

Re: [PATCH 1/2] hw/mips/mips_int: De-duplicate KVM interrupt delivery

[Philippe Mathieu-Daudé]

On 4/29/20 10:48 AM, chen huacai wrote:

Hi, Philippe,

On Wed, Apr 29, 2020 at 4:30 PM Philippe Mathieu-Daudé  wrote:


Refactor duplicated code in a single place.

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/mips/mips_int.c | 11 +++
  1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
index 796730b11d..4a1bf846da 100644
--- a/hw/mips/mips_int.c
+++ b/hw/mips/mips_int.c
@@ -47,17 +47,12 @@ static void cpu_mips_irq_request(void *opaque, int irq, int 
level)

  if (level) {
  env->CP0_Cause |= 1 << (irq + CP0Ca_IP);
-
-if (kvm_enabled() && irq == 2) {
-kvm_mips_set_interrupt(cpu, irq, level);
-}
-
  } else {
  env->CP0_Cause &= ~(1 << (irq + CP0Ca_IP));
+}

Since the if-else has become one line, so can we remove { and } here?


This is the QEMU coding style, see CODING_STYLE.rst:

Block structure
===

Every indented statement is braced; even if the block contains just one
statement.  The opening brace is on the line that contains the control
flow statement that introduces the new block; the closing brace is on the
same line as the else keyword, or on a line by itself if there is no else
keyword.  Example:

.. code-block:: c

if (a == 5) {
printf("a was 5.\n");
} else if (a == 6) {
printf("a was 6.\n");
} else {
printf("a was something else entirely.\n");
}

Rationale: a consistent (except for functions...) bracing style reduces
ambiguity and avoids needless churn when lines are added or removed.
Furthermore, it is the QEMU coding style.





-if (kvm_enabled() && irq == 2) {
-kvm_mips_set_interrupt(cpu, irq, level);
-}
+if (kvm_enabled() && irq == 2) {
+kvm_mips_set_interrupt(cpu, irq, level);
  }

  if (env->CP0_Cause & CP0Ca_IP_mask) {
--
2.21.1

Re: [PATCH 2/2] target/mips/kvm: Assert unreachable code is not used

[Philippe Mathieu-Daudé]

+Paolo

On 4/29/20 10:29 AM, Philippe Mathieu-Daudé wrote:

This code must not be used outside of KVM. Abort if it is.

Signed-off-by: Philippe Mathieu-Daudé 
---
  target/mips/kvm.c | 8 ++--
  1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/target/mips/kvm.c b/target/mips/kvm.c
index de3e26ef1f..050bfbd7fa 100644
--- a/target/mips/kvm.c
+++ b/target/mips/kvm.c
@@ -196,9 +196,7 @@ int kvm_mips_set_interrupt(MIPSCPU *cpu, int irq, int level)
  CPUState *cs = CPU(cpu);
  struct kvm_mips_interrupt intr;
  
-if (!kvm_enabled()) {

-return 0;
-}
+assert(kvm_enabled());
  
  intr.cpu = -1;
  
@@ -219,9 +217,7 @@ int kvm_mips_set_ipi_interrupt(MIPSCPU *cpu, int irq, int level)

  CPUState *dest_cs = CPU(cpu);
  struct kvm_mips_interrupt intr;
  
-if (!kvm_enabled()) {

-return 0;
-}
+assert(kvm_enabled());
  
  intr.cpu = dest_cs->cpu_index;
  

Re: [RFC PATCH 0/3] hw/net/tulip: Fix LP#1874539

[Philippe Mathieu-Daudé]

Hi Jason,

On 4/27/20 5:32 AM, Jason Wang wrote:

On 2020/4/26 下午3:57, Peter Maydell wrote:

On Sun, 26 Apr 2020 at 03:50, Jason Wang  wrote:


Looks good to me.

Would you please send a formal patch and cc Peter.

Consider we are about to release 5.0, it's better for him to apply the
patch directly.

I am not applying any further patches for 5.0 unless they come
with an attached rock-solid justification for why we should
delay the release again for them.

thanks
-- PMM



Ok.

I will queue that patch for 5.1.


Can you queue patches #1 and #2?



Thanks

[PATCH] i386/kvm: fix a use-after-free when vcpu plug/unplug

[Pan Nengyuan]

When we hotplug vcpus, cpu_update_state is added to vm_change_state_head
in kvm_arch_init_vcpu(). But it forgot to delete in kvm_arch_destroy_vcpu() 
after
unplug. Then it will cause a use-after-free access. This patch delete it in
kvm_arch_destroy_vcpu() to fix that.

Reproducer:
virsh setvcpus vm1 4 --live
virsh setvcpus vm1 2 --live
virsh suspend vm1
virsh resume vm1

The UAF stack:
==qemu-system-x86_64==28233==ERROR: AddressSanitizer: heap-use-after-free on 
address 0x62e2e798 at pc 0x5573c6917d9e bp 0x7fff07139e50 sp 0x7fff07139e40
WRITE of size 1 at 0x62e2e798 thread T0
#0 0x5573c6917d9d in cpu_update_state /mnt/sdb/qemu/target/i386/kvm.c:742
#1 0x5573c699121a in vm_state_notify /mnt/sdb/qemu/vl.c:1290
#2 0x5573c636287e in vm_prepare_start /mnt/sdb/qemu/cpus.c:2144
#3 0x5573c6362927 in vm_start /mnt/sdb/qemu/cpus.c:2150
#4 0x5573c71e8304 in qmp_cont /mnt/sdb/qemu/monitor/qmp-cmds.c:173
#5 0x5573c727cb1e in qmp_marshal_cont qapi/qapi-commands-misc.c:835
#6 0x5573c7694c7a in do_qmp_dispatch /mnt/sdb/qemu/qapi/qmp-dispatch.c:132
#7 0x5573c7694c7a in qmp_dispatch /mnt/sdb/qemu/qapi/qmp-dispatch.c:175
#8 0x5573c71d9110 in monitor_qmp_dispatch /mnt/sdb/qemu/monitor/qmp.c:145
#9 0x5573c71dad4f in monitor_qmp_bh_dispatcher 
/mnt/sdb/qemu/monitor/qmp.c:234

Reported-by: Euler Robot 
Signed-off-by: Pan Nengyuan 
---
 target/i386/cpu.h | 1 +
 target/i386/kvm.c | 5 -
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index e818fc712a..afbd11b7a3 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1631,6 +1631,7 @@ struct X86CPU {
 
 CPUNegativeOffsetState neg;
 CPUX86State env;
+VMChangeStateEntry *vmsentry;
 
 uint64_t ucode_rev;
 
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 4901c6dd74..ff2848357e 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1770,7 +1770,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
 }
 }
 
-qemu_add_vm_change_state_handler(cpu_update_state, env);
+cpu->vmsentry = qemu_add_vm_change_state_handler(cpu_update_state, env);
 
 c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0);
 if (c) {
@@ -1883,6 +1883,9 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
 env->nested_state = NULL;
 }
 
+qemu_del_vm_change_state_handler(cpu->vmsentry);
+cpu->vmsentry = NULL;
+
 return 0;
 }
 
-- 
2.18.2

Re: [PATCH v1 1/8] s390/sclp: remove SCLPDevice param from prepare_cpu_entries

[David Hildenbrand]

On 09.05.20 01:08, Collin Walling wrote:
> It was never used in this function, so let's remove it.
> 
> Signed-off-by: Collin Walling 
> ---
>  hw/s390x/sclp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index ede056b3ef..156ffe3223 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -49,7 +49,7 @@ static inline bool sclp_command_code_valid(uint32_t code)
>  return false;
>  }
>  
> -static void prepare_cpu_entries(SCLPDevice *sclp, CPUEntry *entry, int 
> *count)
> +static void prepare_cpu_entries(CPUEntry *entry, int *count)
>  {
>  MachineState *ms = MACHINE(qdev_get_machine());
>  uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
> @@ -77,7 +77,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>  IplParameterBlock *ipib = s390_ipl_get_iplb();
>  
>  /* CPU information */
> -prepare_cpu_entries(sclp, read_info->entries, &cpu_count);
> +prepare_cpu_entries(read_info->entries, &cpu_count);
>  read_info->entries_cpu = cpu_to_be16(cpu_count);
>  read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries));
>  read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
> @@ -135,7 +135,7 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB 
> *sccb)
>  ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
>  int cpu_count;
>  
> -prepare_cpu_entries(sclp, cpu_info->entries, &cpu_count);
> +prepare_cpu_entries(cpu_info->entries, &cpu_count);
>  cpu_info->nr_configured = cpu_to_be16(cpu_count);
>  cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, 
> entries));
>  cpu_info->nr_standby = cpu_to_be16(0);
> 

Reviewed-by: David Hildenbrand 

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 3/8] s390/sclp: rework sclp boundary and length checks

[David Hildenbrand]

On 09.05.20 01:08, Collin Walling wrote:
> Let's factor out the SCLP boundary and length checks
> into separate functions.
> 
> Signed-off-by: Collin Walling 
> ---
>  hw/s390x/sclp.c | 41 +++--
>  1 file changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index d08a291e40..470d5da7a2 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -49,6 +49,34 @@ static inline bool sclp_command_code_valid(uint32_t code)
>  return false;
>  }
>  
> +static bool check_sccb_boundary_valid(uint64_t sccb_addr, uint32_t code,
> +  SCCB *sccb)

I suggest naming this

"has_valid_sccb_boundary", then the true/false response is clearer.

> +{
> +uint64_t current_len = sccb_addr + be16_to_cpu(sccb->h.length);
> +uint64_t allowed_len = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
> +
> +switch (code & SCLP_CMD_CODE_MASK) {
> +default:
> +if (current_len <= allowed_len) {
> +return true;
> +}
> +}
> +sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> +return false;
> +}
> +
> +static bool check_sufficient_sccb_len(SCCB *sccb, int size)

"has_sufficient_sccb_len" ?

> +{
> +MachineState *ms = MACHINE(qdev_get_machine());
> +int required_len = size + ms->possible_cpus->len * sizeof(CPUEntry);

Rather pass in the number of cpus instead. Looking up the machine again
in here is ugly.

> +
> +if (be16_to_cpu(sccb->h.length) < required_len) {
> +sccb->h.response_code = 
> cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +return false;
> +}
> +return true;
> +}
> +
>  static void prepare_cpu_entries(CPUEntry *entry, int *count)
>  {
>  MachineState *ms = MACHINE(qdev_get_machine());
> @@ -76,8 +104,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>  int rnsize, rnmax;
>  IplParameterBlock *ipib = s390_ipl_get_iplb();
>  
> -if (be16_to_cpu(sccb->h.length) < (sizeof(ReadInfo) + cpu_count * 
> sizeof(CPUEntry))) {
> -sccb->h.response_code = 
> cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +if (!check_sufficient_sccb_len(sccb, sizeof(ReadInfo))) {
>  return;
>  }
>  
> @@ -134,8 +161,7 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB 
> *sccb)
>  ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
>  int cpu_count;
>  
> -if (be16_to_cpu(sccb->h.length) < (sizeof(ReadCpuInfo) + cpu_count * 
> sizeof(CPUEntry))) {
> -sccb->h.response_code = 
> cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +if (!check_sufficient_sccb_len(sccb, sizeof(ReadCpuInfo))) {
>  return;
>  }
>  
> @@ -227,6 +253,10 @@ int sclp_service_call_protected(CPUS390XState *env, 
> uint64_t sccb,
>  goto out_write;
>  }
>  
> +if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) {
> +goto out_write;
> +}

This is not a "factor out". You're adding new code, this needs
justification in the patch description.

> +
>  sclp_c->execute(sclp, &work_sccb, code);
>  out_write:
>  s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
> @@ -272,8 +302,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, 
> uint32_t code)
>  goto out_write;
>  }
>  
> -if ((sccb + be16_to_cpu(work_sccb.h.length)) > ((sccb & PAGE_MASK) + 
> PAGE_SIZE)) {
> -work_sccb.h.response_code = 
> cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> +if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) {
>  goto out_write;
>  }
>  
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH v1 4/8] s390/sclp: read sccb from mem based on sccb length

[David Hildenbrand]

On 09.05.20 01:08, Collin Walling wrote:
> The header of the SCCB contains the actual length of the
> SCCB. Instead of using a static 4K size, let's allow
> for a variable size determined by the value set in the
> header. The proper checks are already in place to ensure
> the SCCB length is sufficent to store a full response,
> and that the length does not cross any explicitly-set
> boundaries.
> 
> Signed-off-by: Collin Walling 
> ---
>  hw/s390x/sclp.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 470d5da7a2..748d04a0e2 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -244,15 +244,16 @@ int sclp_service_call_protected(CPUS390XState *env, 
> uint64_t sccb,
>  SCLPDevice *sclp = get_sclp_device();
>  SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
>  SCCB work_sccb;
> -hwaddr sccb_len = sizeof(SCCB);
>  
> -s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
> +s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, 
> sizeof(SCCBHeader));
>  
>  if (!sclp_command_code_valid(code)) {
>  work_sccb.h.response_code = 
> cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>  goto out_write;
>  }
>  
> +s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, 
> work_sccb.h.length);

be16_to_cpu() necessary.

> +
>  if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) {
>  goto out_write;
>  }
> @@ -271,8 +272,6 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, 
> uint32_t code)
>  SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
>  SCCB work_sccb;
>  
> -hwaddr sccb_len = sizeof(SCCB);
> -
>  /* first some basic checks on program checks */
>  if (env->psw.mask & PSW_MASK_PSTATE) {
>  return -PGM_PRIVILEGED;
> @@ -290,13 +289,16 @@ int sclp_service_call(CPUS390XState *env, uint64_t 
> sccb, uint32_t code)
>   * from playing dirty tricks by modifying the memory content after
>   * the host has checked the values
>   */
> -cpu_physical_memory_read(sccb, &work_sccb, sccb_len);
> +cpu_physical_memory_read(sccb, &work_sccb, sizeof(SCCBHeader));
>  
>  /* Valid sccb sizes */
>  if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) {
>  return -PGM_SPECIFICATION;
>  }
>  
> +/* the header contains the actual length of the sccb */
> +cpu_physical_memory_read(sccb, &work_sccb, work_sccb.h.length);

be16_to_cpu() necessary.

> +
>  if (!sclp_command_code_valid(code)) {
>  work_sccb.h.response_code = 
> cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>  goto out_write;
> 

-- 
Thanks,

David / dhildenb

how to extend to load COFF executable image file

[xiaolei]

Hi all,
  I attempt to add DSP architecture support for some TI processor, based on 
QEMU 4.2.
  When I work on the executable file loading , I try to load COFF executable  
file. Following the ELF file processing scheme, I thought I could write a 
function similar to :
   rom_add_elf_program(label, mapped_file, data, file_size, mem_size, addr, 
as);
  But I got lost when I track down the usage to the global variable  :static 
QTAILQ_HEAD(, Rom) roms;
  I did not get where this 'roms'  is used for program loading, and how the 
loaded program get to run eventually.  Can someone give me some hints?

  Also, the COFF file format differs from the ELF, there is no program header. 
I wonder if I could reuse the 'rom' structure like loading a ELF. Or there is a 
better way to do it.

struct Rom {
char *name;
char *path;

/* datasize is the amount of memory allocated in "data". If datasize is less
 * than romsize, it means that the area from datasize to romsize is filled
 * with zeros.
 */
size_t romsize;  //?how to fill romsize for coff file 
size_t datasize;//?how to fill datasize for coff file 
uint8_t *data;  //? for coff file 
MemoryRegion *mr;
AddressSpace *as;
int isrom;
char *fw_dir;
char *fw_file;
GMappedFile *mapped_file;
bool committed;
hwaddr addr;
QTAILQ_ENTRY(Rom) next;
}; 
Any advise would be appreciated!!

regards,

xiaolei cui

Re: [PATCH v5 0/7] dwc-hsotg (aka dwc2) USB host controller emulation

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200512064900.28554-1-pauld...@gmail.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20200512064900.28554-1-pauld...@gmail.com
Subject: [PATCH v5 0/7] dwc-hsotg (aka dwc2) USB host controller emulation
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
b305ad2 raspi2 acceptance test: add test for dwc-hsotg (dwc2) USB host
65ec67a wire in the dwc-hsotg (dwc2) USB host controller emulation
c013ecb usb: add short-packet handling to usb-storage driver
b349ad3 dwc-hsotg (dwc2) USB host controller emulation
eefc2af dwc-hsotg (dwc2) USB host controller state definitions
42049bd dwc-hsotg (dwc2) USB host controller register definitions
6b6d361 raspi: add BCM2835 SOC MPHI emulation

=== OUTPUT BEGIN ===
1/7 Checking commit 6b6d3619b1cf (raspi: add BCM2835 SOC MPHI emulation)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#63: 
new file mode 100644

WARNING: line over 80 characters
#211: FILE: hw/misc/bcm2835_mphi.c:144:
+memory_region_init_io(&s->iomem, obj, &mphi_mmio_ops, s, "mphi", 
MPHI_MMIO_SIZE);

total: 0 errors, 2 warnings, 278 lines checked

Patch 1/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/7 Checking commit 42049bddb541 (dwc-hsotg (dwc2) USB host controller register 
definitions)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#18: 
new file mode 100644

WARNING: architecture specific defines should be avoided
#60: FILE: include/hw/usb/dwc2-regs.h:38:
+#ifndef __DWC2_HW_H__

ERROR: code indent should never use tabs
#63: FILE: include/hw/usb/dwc2-regs.h:41:
+#define HSOTG_REG(x)^I(x)$

ERROR: code indent should never use tabs
#65: FILE: include/hw/usb/dwc2-regs.h:43:
+#define GOTGCTL^I^I^I^IHSOTG_REG(0x000)$

ERROR: code indent should never use tabs
#66: FILE: include/hw/usb/dwc2-regs.h:44:
+#define GOTGCTL_CHIRPEN^I^I^IBIT(27)$

ERROR: code indent should never use tabs
#67: FILE: include/hw/usb/dwc2-regs.h:45:
+#define GOTGCTL_MULT_VALID_BC_MASK^I(0x1f << 22)$

ERROR: code indent should never use tabs
#68: FILE: include/hw/usb/dwc2-regs.h:46:
+#define GOTGCTL_MULT_VALID_BC_SHIFT^I22$

ERROR: code indent should never use tabs
#69: FILE: include/hw/usb/dwc2-regs.h:47:
+#define GOTGCTL_OTGVER^I^I^IBIT(20)$

ERROR: code indent should never use tabs
#70: FILE: include/hw/usb/dwc2-regs.h:48:
+#define GOTGCTL_BSESVLD^I^I^IBIT(19)$

ERROR: code indent should never use tabs
#71: FILE: include/hw/usb/dwc2-regs.h:49:
+#define GOTGCTL_ASESVLD^I^I^IBIT(18)$

ERROR: code indent should never use tabs
#72: FILE: include/hw/usb/dwc2-regs.h:50:
+#define GOTGCTL_DBNC_SHORT^I^IBIT(17)$

ERROR: code indent should never use tabs
#73: FILE: include/hw/usb/dwc2-regs.h:51:
+#define GOTGCTL_CONID_B^I^I^IBIT(16)$

ERROR: code indent should never use tabs
#74: FILE: include/hw/usb/dwc2-regs.h:52:
+#define GOTGCTL_DBNCE_FLTR_BYPASS^IBIT(15)$

ERROR: code indent should never use tabs
#75: FILE: include/hw/usb/dwc2-regs.h:53:
+#define GOTGCTL_DEVHNPEN^I^IBIT(11)$

ERROR: code indent should never use tabs
#76: FILE: include/hw/usb/dwc2-regs.h:54:
+#define GOTGCTL_HSTSETHNPEN^I^IBIT(10)$

ERROR: code indent should never use tabs
#77: FILE: include/hw/usb/dwc2-regs.h:55:
+#define GOTGCTL_HNPREQ^I^I^IBIT(9)$

ERROR: code indent should never use tabs
#78: FILE: include/hw/usb/dwc2-regs.h:56:
+#define GOTGCTL_HSTNEGSCS^I^IBIT(8)$

ERROR: code indent should never use tabs
#79: FILE: include/hw/usb/dwc2-regs.h:57:
+#define GOTGCTL_SESREQ^I^I^IBIT(1)$

ERROR: code indent should never use tabs
#80: FILE: include/hw/usb/dwc2-regs.h:58:
+#define GOTGCTL_SESREQSCS^I^IBIT(0)$

ERROR: code indent should never use tabs
#82: FILE: include/hw/usb/dwc2-regs.h:60:
+#define GOTGINT^I^I^I^IHSOTG_REG(0x004)$

ERROR: code indent should never use tabs
#83: FILE: include/hw/usb/dwc2-regs.h:61:
+#define GOTGINT_DBNCE_DONE^I^IBIT(19)$

ERROR: code indent should never use tabs
#84: FILE: include/hw/usb/dwc2-regs.h:62:
+#define GOTGINT_A_DEV_TOUT_CHG^I^IBIT(18)$

ERROR: code indent should never use tabs
#85: FILE: include/hw/usb/dwc2-regs.h:63:
+#define GOTGINT_HST_NEG_DET^I^IBIT(17)$

ERROR: code indent should never use tabs
#86: FILE: include/hw/usb/dwc2-regs.h:64:
+#define GOTGINT_HST_NEG_SUC_STS_CHNG^IBIT(9)$

ERROR: code indent should never use tabs
#87: FILE: include/hw/usb/dwc2-regs.h:65:
+#define GOTGINT_SES_REQ_SUC_STS_CHNG^IBIT(8)$

ERROR: code indent should never use tabs
#88: FILE: include/hw/usb/dwc2-regs.h:66:
+#define GOTGINT_SES_END_DET^I^IBIT(2)$

ERROR: code indent should never use tabs
#90: FILE: include/hw/usb/dwc2-regs.h:68:

Re: Qemu, VNC and non-US keymaps

[B3r3n]

Hello Daniel, all,

I am a bit confused.

Ok, RFB protocol should be the solution that solves all, sending 
scancodes rather than doing keysyms stuff. No pb for me.

So I removed my '-k fr' to my Qemu VM start as it was before.

However, reading TightVNC & noVNC docs, both are able to perform RFB.

Since these explanations, I replayed a bit:

Under my testing Debian 10, I redefined keyboard to French + No 
compose key + AltGr as CTRL_R


Under noVNC: Ctrl_R works well as alternative but when using AltGr, I 
received 29+100+7 (AltGr + 6) and keep displaying a minus as with 
AltGr was not pressed.


Under TightVNC (2.7.10) : Ctrl_R displays characters, I am still in 
QWERTY for letters, weird mapping for other characters, did not 
checked if compliant with whatever definition.
Under TightVNC (last 2.8.27, supposed to be able to RFB): Ctrl_R 
displays nothing, keys are QWERTY. Seems the same as TightVNC 2.7.10.


With the keyboard defining AltGr as AltGr, no change.

I realize that AltGr is sending 29+100 (seen via showkey), when 
CTRL_R only sends 97.

When using a remote console (iLo and iDRAC), AltGr only sends 100.

I wonder if the issue would not also be the fact AltGr sends 2 codes, 
still another one to select the character key (6 for example).


Is that normal Qemu is transforming AltGr (100) in 29+100 ?

Thanks

Brgrds

[PATCH v2] linux-user: support of semtimedop syscall

[Matus Kysel]

We should add support of semtimedop syscall as new version of
glibc 2.31 uses semop based on semtimedop (commit: 
https://gitlab.com/freedesktop-sdk/mirrors/sourceware/glibc/-/commit/765cdd0bffd77960ae852104fc4ea5edcdb8aed3
 ).

Signed-off-by: Matus Kysel 
---
 linux-user/syscall.c | 34 --
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 05f03919ff..7c6f9439e0 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1227,7 +1227,8 @@ static inline abi_long copy_to_user_timeval64(abi_ulong 
target_tv_addr,
 defined(TARGET_NR_pselect6) || defined(TARGET_NR_pselect6) || \
 defined(TARGET_NR_nanosleep) || defined(TARGET_NR_clock_settime) || \
 defined(TARGET_NR_utimensat) || defined(TARGET_NR_mq_timedsend) || \
-defined(TARGET_NR_mq_timedreceive)
+defined(TARGET_NR_mq_timedreceive) || defined(TARGET_NR_ipc) || \
+defined(TARGET_NR_semop) || defined(TARGET_NR_semtimedop)
 static inline abi_long target_to_host_timespec(struct timespec *host_ts,
abi_ulong target_addr)
 {
@@ -3875,25 +3876,39 @@ static inline abi_long target_to_host_sembuf(struct 
sembuf *host_sembuf,
 return 0;
 }

-static inline abi_long do_semop(int semid, abi_long ptr, unsigned nsops)
+#if defined(TARGET_NR_ipc) || defined(TARGET_NR_semop) || \
+defined(TARGET_NR_semtimedop)
+static inline abi_long do_semtimedop(int semid,
+ abi_long ptr,
+ unsigned nsops,
+ abi_long timeout)
 {
 struct sembuf sops[nsops];
+struct timespec ts, *pts = NULL;
 abi_long ret;

+if (timeout) {
+pts = &ts;
+if (target_to_host_timespec(pts, timeout)) {
+return -TARGET_EFAULT;
+}
+}
+
 if (target_to_host_sembuf(sops, ptr, nsops))
 return -TARGET_EFAULT;

 ret = -TARGET_ENOSYS;
 #ifdef __NR_semtimedop
-ret = get_errno(safe_semtimedop(semid, sops, nsops, NULL));
+ret = get_errno(safe_semtimedop(semid, sops, nsops, pts));
 #endif
 #ifdef __NR_ipc
 if (ret == -TARGET_ENOSYS) {
-ret = get_errno(safe_ipc(IPCOP_semtimedop, semid, nsops, 0, sops, 0));
+ret = get_errno(safe_ipc(IPCOP_semtimedop, semid, nsops, 0, sops, 
pts));
 }
 #endif
 return ret;
 }
+#endif

 struct target_msqid_ds
 {
@@ -4369,7 +4384,10 @@ static abi_long do_ipc(CPUArchState *cpu_env,

 switch (call) {
 case IPCOP_semop:
-ret = do_semop(first, ptr, second);
+ret = do_semtimedop(first, ptr, second, 0);
+break;
+case IPCOP_semtimedop:
+ret = do_semtimedop(first, ptr, second, third);
 break;

 case IPCOP_semget:
@@ -9594,7 +9612,11 @@ static abi_long do_syscall1(void *cpu_env, int num, 
abi_long arg1,
 #endif
 #ifdef TARGET_NR_semop
 case TARGET_NR_semop:
-return do_semop(arg1, arg2, arg3);
+return do_semtimedop(arg1, arg2, arg3, 0);
+#endif
+#ifdef TARGET_NR_semtimedop
+case TARGET_NR_semtimedop:
+return do_semtimedop(arg1, arg2, arg3, arg4);
 #endif
 #ifdef TARGET_NR_semctl
 case TARGET_NR_semctl:
--
2.17.1

[PATCH v2 0/3] hw: Use qdev gpio rather than qemu_allocate_irqs()

[Philippe Mathieu-Daudé]

Use a coccinelle script to convert few qemu_allocate_irqs()
calls to the qdev gpio API.

One memory leak removed in hw/openrisc/pic_cpu.c

Since v1:
- Referrenced Coverity CID (Stafford)
- Reword AHCI description (Zoltan)

Philippe Mathieu-Daudé (3):
  hw/ide/ahci: Use qdev gpio rather than qemu_allocate_irqs()
  hw/mips/mips_int: Use qdev gpio rather than qemu_allocate_irqs()
  hw/openrisc/pic_cpu: Use qdev gpio rather than qemu_allocate_irqs()

 hw/ide/ahci.c | 6 ++
 hw/mips/mips_int.c| 6 ++
 hw/openrisc/pic_cpu.c | 5 ++---
 3 files changed, 6 insertions(+), 11 deletions(-)

-- 
2.21.3

[PATCH v2 1/3] hw/ide/ahci: Use qdev gpio rather than qemu_allocate_irqs()

[Philippe Mathieu-Daudé]

When plugging/unplugging devices on a AHCI bus, we might
leak the memory returned by qemu_allocate_irqs(). We can
avoid this leak by switching to using qdev_init_gpio_in();
the base class finalize will free the irqs that this allocates
under the hood.

Patch created mechanically using spatch with this script
inspired from commit d6ef883d9d7:

  @@
  typedef qemu_irq;
  identifier irqs, handler;
  expression opaque, count, i;
  @@
  -   qemu_irq *irqs;
  ...
  -   irqs = qemu_allocate_irqs(handler, opaque, count);
  +   qdev_init_gpio_in(DEVICE(opaque), handler, count);
  <+...
  -   irqs[i]
  +   qdev_get_gpio_in(DEVICE(opaque), i)
  ...+>
  ?-  g_free(irqs);

Inspired-by: Peter Maydell 
Reviewed-by: Alistair Francis 
Reviewed-by: John Snow 
Acked-by: John Snow 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/ide/ahci.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 13d91e109a..991bb7c9c8 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1534,19 +1534,18 @@ void ahci_init(AHCIState *s, DeviceState *qdev)
 
 void ahci_realize(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports)
 {
-qemu_irq *irqs;
 int i;
 
 s->as = as;
 s->ports = ports;
 s->dev = g_new0(AHCIDevice, ports);
 ahci_reg_init(s);
-irqs = qemu_allocate_irqs(ahci_irq_set, s, s->ports);
+qdev_init_gpio_in(qdev, ahci_irq_set, s->ports);
 for (i = 0; i < s->ports; i++) {
 AHCIDevice *ad = &s->dev[i];
 
 ide_bus_new(&ad->port, sizeof(ad->port), qdev, i, 1);
-ide_init2(&ad->port, irqs[i]);
+ide_init2(&ad->port, qdev_get_gpio_in(qdev, i));
 
 ad->hba = s;
 ad->port_no = i;
@@ -1554,7 +1553,6 @@ void ahci_realize(AHCIState *s, DeviceState *qdev, 
AddressSpace *as, int ports)
 ad->port.dma->ops = &ahci_dma_ops;
 ide_register_restart_cb(&ad->port);
 }
-g_free(irqs);
 }
 
 void ahci_uninit(AHCIState *s)
-- 
2.21.3

[PATCH v2 3/3] hw/openrisc/pic_cpu: Use qdev gpio rather than qemu_allocate_irqs()

[Philippe Mathieu-Daudé]

Coverity points out (CID 1421934) that we are leaking the
memory returned by qemu_allocate_irqs(). We can avoid this
leak by switching to using qdev_init_gpio_in(); the base
class finalize will free the irqs that this allocates under
the hood.

Patch created mechanically using spatch with this script
inspired from commit d6ef883d9d7:

  @@
  typedef qemu_irq;
  identifier irqs, handler;
  expression opaque, count, i;
  @@
  -   qemu_irq *irqs;
  ...
  -   irqs = qemu_allocate_irqs(handler, opaque, count);
  +   qdev_init_gpio_in(DEVICE(opaque), handler, count);
  <+...
  -   irqs[i]
  +   qdev_get_gpio_in(DEVICE(opaque), i)
  ...+>
  ?-  g_free(irqs);

Fixes: Coverity CID 1421934 RESOURCE_LEAK
Inspired-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/openrisc/pic_cpu.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/openrisc/pic_cpu.c b/hw/openrisc/pic_cpu.c
index 36f9350830..4b0c92f842 100644
--- a/hw/openrisc/pic_cpu.c
+++ b/hw/openrisc/pic_cpu.c
@@ -52,10 +52,9 @@ static void openrisc_pic_cpu_handler(void *opaque, int irq, 
int level)
 void cpu_openrisc_pic_init(OpenRISCCPU *cpu)
 {
 int i;
-qemu_irq *qi;
-qi = qemu_allocate_irqs(openrisc_pic_cpu_handler, cpu, NR_IRQS);
+qdev_init_gpio_in(DEVICE(cpu), openrisc_pic_cpu_handler, NR_IRQS);
 
 for (i = 0; i < NR_IRQS; i++) {
-cpu->env.irq[i] = qi[i];
+cpu->env.irq[i] = qdev_get_gpio_in(DEVICE(cpu), i);
 }
 }
-- 
2.21.3

[PATCH v2 2/3] hw/mips/mips_int: Use qdev gpio rather than qemu_allocate_irqs()

[Philippe Mathieu-Daudé]

Switch to using the qdev gpio API which is preferred over
qemu_allocate_irqs(). One step to eventually deprecate and
remove qemu_allocate_irqs() one day.

Patch created mechanically using spatch with this script
inspired from commit d6ef883d9d7:

  @@
  typedef qemu_irq;
  identifier irqs, handler;
  expression opaque, count, i;
  @@
  -   qemu_irq *irqs;
  ...
  -   irqs = qemu_allocate_irqs(handler, opaque, count);
  +   qdev_init_gpio_in(DEVICE(opaque), handler, count);
  <+...
  -   irqs[i]
  +   qdev_get_gpio_in(DEVICE(opaque), i)
  ...+>
  ?-  g_free(irqs);

Fixes: Coverity CID 1421934 RESOURCE_LEAK
Inspired-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/mips/mips_int.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
index 796730b11d..91788c51a9 100644
--- a/hw/mips/mips_int.c
+++ b/hw/mips/mips_int.c
@@ -74,14 +74,12 @@ static void cpu_mips_irq_request(void *opaque, int irq, int 
level)
 void cpu_mips_irq_init_cpu(MIPSCPU *cpu)
 {
 CPUMIPSState *env = &cpu->env;
-qemu_irq *qi;
 int i;
 
-qi = qemu_allocate_irqs(cpu_mips_irq_request, cpu, 8);
+qdev_init_gpio_in(DEVICE(cpu), cpu_mips_irq_request, 8);
 for (i = 0; i < 8; i++) {
-env->irq[i] = qi[i];
+env->irq[i] = qdev_get_gpio_in(DEVICE(cpu), i);
 }
-g_free(qi);
 }
 
 void cpu_mips_soft_irq(CPUMIPSState *env, int irq, int level)
-- 
2.21.3

Re: [PATCH] i386/kvm: fix a use-after-free when vcpu plug/unplug

[Philippe Mathieu-Daudé]

On 5/12/20 3:39 PM, Pan Nengyuan wrote:

When we hotplug vcpus, cpu_update_state is added to vm_change_state_head
in kvm_arch_init_vcpu(). But it forgot to delete in kvm_arch_destroy_vcpu() 
after
unplug. Then it will cause a use-after-free access. This patch delete it in
kvm_arch_destroy_vcpu() to fix that.

Reproducer:
 virsh setvcpus vm1 4 --live
 virsh setvcpus vm1 2 --live
 virsh suspend vm1
 virsh resume vm1

The UAF stack:
==qemu-system-x86_64==28233==ERROR: AddressSanitizer: heap-use-after-free on 
address 0x62e2e798 at pc 0x5573c6917d9e bp 0x7fff07139e50 sp 0x7fff07139e40
WRITE of size 1 at 0x62e2e798 thread T0
 #0 0x5573c6917d9d in cpu_update_state /mnt/sdb/qemu/target/i386/kvm.c:742
 #1 0x5573c699121a in vm_state_notify /mnt/sdb/qemu/vl.c:1290
 #2 0x5573c636287e in vm_prepare_start /mnt/sdb/qemu/cpus.c:2144
 #3 0x5573c6362927 in vm_start /mnt/sdb/qemu/cpus.c:2150
 #4 0x5573c71e8304 in qmp_cont /mnt/sdb/qemu/monitor/qmp-cmds.c:173
 #5 0x5573c727cb1e in qmp_marshal_cont qapi/qapi-commands-misc.c:835
 #6 0x5573c7694c7a in do_qmp_dispatch /mnt/sdb/qemu/qapi/qmp-dispatch.c:132
 #7 0x5573c7694c7a in qmp_dispatch /mnt/sdb/qemu/qapi/qmp-dispatch.c:175
 #8 0x5573c71d9110 in monitor_qmp_dispatch /mnt/sdb/qemu/monitor/qmp.c:145
 #9 0x5573c71dad4f in monitor_qmp_bh_dispatcher 
/mnt/sdb/qemu/monitor/qmp.c:234

Reported-by: Euler Robot 
Signed-off-by: Pan Nengyuan 
---
  target/i386/cpu.h | 1 +
  target/i386/kvm.c | 5 -
  2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index e818fc712a..afbd11b7a3 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1631,6 +1631,7 @@ struct X86CPU {
  
  CPUNegativeOffsetState neg;

  CPUX86State env;
+VMChangeStateEntry *vmsentry;
  
  uint64_t ucode_rev;
  
diff --git a/target/i386/kvm.c b/target/i386/kvm.c

index 4901c6dd74..ff2848357e 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1770,7 +1770,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
  }
  }
  
-qemu_add_vm_change_state_handler(cpu_update_state, env);

+cpu->vmsentry = qemu_add_vm_change_state_handler(cpu_update_state, env);
  
  c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0);

  if (c) {
@@ -1883,6 +1883,9 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
  env->nested_state = NULL;
  }
  
+qemu_del_vm_change_state_handler(cpu->vmsentry);

+cpu->vmsentry = NULL;


Why set it to NULL? there is no non-NULL check.

Anyway if it matters to you, why not do it in 
qemu_del_vm_change_state_handler()?


Otherwise:
Reviewed-by: Philippe Mathieu-Daudé 


+
  return 0;
  }
  

[PATCH 1/1] NetBSD/arm build fix

[Nick Hudson]

Fix building on NetBSD/arm by extracting the FSR value from the
correct siginfo_t field.

Signed-off-by: Nick Hudson 
---
 accel/tcg/user-exec.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 52359949df..3637626456 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -517,6 +517,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
 
 #if defined(__NetBSD__)
 #include 
+#include 
 #endif
 
 int cpu_signal_handler(int host_signum, void *pinfo,
@@ -525,6 +526,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
 siginfo_t *info = pinfo;
 #if defined(__NetBSD__)
 ucontext_t *uc = puc;
+siginfo_t *si = pinfo;
 #else
 ucontext_t *uc = puc;
 #endif
@@ -539,10 +541,18 @@ int cpu_signal_handler(int host_signum, void *pinfo,
 pc = uc->uc_mcontext.arm_pc;
 #endif
 
+#if defined(__NetBSD__)
+/* siginfo_t::si_trap is the FSR value, in which bit 11 is WnR
+ * (assuming a v6 or later processor; on v5 we will always report
+ * this as a read).
+ */
+is_write = extract32(si->si_trap, 11, 1);
+#else
 /* error_code is the FSR value, in which bit 11 is WnR (assuming a v6 or
  * later processor; on v5 we will always report this as a read).
  */
 is_write = extract32(uc->uc_mcontext.error_code, 11, 1);
+#endif
 return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
 }
 
-- 
2.17.1

Re: [PATCH 1/4] fuzz: add datadir for oss-fuzz compatability

[Darren Kenny]

On Monday, 2020-05-11 at 23:01:30 -04, Alexander Bulekov wrote:
> This allows us to keep pc-bios in executable_dir/pc-bios, rather than
> executable_dir/../pc-bios, which is incompatible with oss-fuzz' file
> structure.
>
> Signed-off-by: Alexander Bulekov 

Reviewed-by: Darren Kenny 

> ---
>  include/sysemu/sysemu.h |  2 ++
>  softmmu/vl.c|  2 +-
>  tests/qtest/fuzz/fuzz.c | 15 +++
>  3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index ef81302e1a..cc96b66fc9 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -15,6 +15,8 @@ extern const char *qemu_name;
>  extern QemuUUID qemu_uuid;
>  extern bool qemu_uuid_set;
>  
> +void qemu_add_data_dir(const char *path);
> +
>  void qemu_add_exit_notifier(Notifier *notify);
>  void qemu_remove_exit_notifier(Notifier *notify);
>  
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index afd2615fb3..c71485a965 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -1993,7 +1993,7 @@ char *qemu_find_file(int type, const char *name)
>  return NULL;
>  }
>  
> -static void qemu_add_data_dir(const char *path)
> +void qemu_add_data_dir(const char *path)
>  {
>  int i;
>  
> diff --git a/tests/qtest/fuzz/fuzz.c b/tests/qtest/fuzz/fuzz.c
> index f5c923852e..33365c3782 100644
> --- a/tests/qtest/fuzz/fuzz.c
> +++ b/tests/qtest/fuzz/fuzz.c
> @@ -137,6 +137,7 @@ int LLVMFuzzerInitialize(int *argc, char ***argv, char 
> ***envp)
>  {
>  
>  char *target_name;
> +char *dir;
>  
>  /* Initialize qgraph and modules */
>  qos_graph_init();
> @@ -147,6 +148,20 @@ int LLVMFuzzerInitialize(int *argc, char ***argv, char 
> ***envp)
>  target_name = strstr(**argv, "-target-");
>  if (target_name) {/* The binary name specifies the target */
>  target_name += strlen("-target-");
> +/*
> + * With oss-fuzz, the executable is kept in the root of a directory 
> (we
> + * cannot assume the path). All data (including bios binaries) must 
> be
> + * in the same dir, or a subdir. Thus, we cannot place the pc-bios so
> + * that it would be in exec_dir/../pc-bios.
> + * As a workaround, oss-fuzz allows us to use argv[0] to get the
> + * location of the executable. Using this we add exec_dir/pc-bios to
> + * the datadirs.
> + */
> +dir = g_build_filename(g_path_get_dirname(**argv), "pc-bios", NULL);
> +if (g_file_test(dir, G_FILE_TEST_IS_DIR)) {
> +qemu_add_data_dir(dir);
> +}
> +g_free(dir);
>  } else if (*argc > 1) {  /* The target is specified as an argument */
>  target_name = (*argv)[1];
>  if (!strstr(target_name, "--fuzz-target=")) {
> -- 
> 2.26.2

Re: [PATCH 2/4] fuzz: fix typo in i440fx-qtest-reboot arguments

[Darren Kenny]

On Monday, 2020-05-11 at 23:01:31 -04, Alexander Bulekov wrote:
> Signed-off-by: Alexander Bulekov 

Reviewed-by: Darren Kenny 

> ---
>  tests/qtest/fuzz/i440fx_fuzz.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qtest/fuzz/i440fx_fuzz.c b/tests/qtest/fuzz/i440fx_fuzz.c
> index ab5f112584..90e75ffaea 100644
> --- a/tests/qtest/fuzz/i440fx_fuzz.c
> +++ b/tests/qtest/fuzz/i440fx_fuzz.c
> @@ -143,7 +143,7 @@ static void i440fx_fuzz_qos_fork(QTestState *s,
>  }
>  
>  static const char *i440fx_qtest_argv = TARGET_NAME " -machine accel=qtest"
> -   "-m 0 -display none";
> +   " -m 0 -display none";
>  static const char *i440fx_argv(FuzzTarget *t)
>  {
>  return i440fx_qtest_argv;
> -- 
> 2.26.2

Re: [PATCH 4/4] fuzz: run the main-loop in fork-server process

[Darren Kenny]

On Monday, 2020-05-11 at 23:01:33 -04, Alexander Bulekov wrote:
> Without this, the time since the last main-loop keeps increasing, as the
> fuzzer runs. The forked children need to handle all the "past-due"
> timers, slowing them down, over time. With this change, the
> parent/fork-server process runs the main-loop, while waiting on the
> child, ensuring that the timer events do not pile up, over time.
>
> Signed-off-by: Alexander Bulekov 

Reviewed-by: Darren Kenny 

> ---
>  tests/qtest/fuzz/i440fx_fuzz.c  | 1 +
>  tests/qtest/fuzz/virtio_net_fuzz.c  | 2 ++
>  tests/qtest/fuzz/virtio_scsi_fuzz.c | 2 ++
>  3 files changed, 5 insertions(+)
>
> I'm working on another series to abstract away the details of resetting
> qemu state between runs from the individual targets. That should relieve
> us from needing to add this for each new fuzzing target.
>
> diff --git a/tests/qtest/fuzz/i440fx_fuzz.c b/tests/qtest/fuzz/i440fx_fuzz.c
> index 90e75ffaea..8449f81687 100644
> --- a/tests/qtest/fuzz/i440fx_fuzz.c
> +++ b/tests/qtest/fuzz/i440fx_fuzz.c
> @@ -138,6 +138,7 @@ static void i440fx_fuzz_qos_fork(QTestState *s,
>  i440fx_fuzz_qos(s, Data, Size);
>  _Exit(0);
>  } else {
> +flush_events(s);
>  wait(NULL);
>  }
>  }
> diff --git a/tests/qtest/fuzz/virtio_net_fuzz.c 
> b/tests/qtest/fuzz/virtio_net_fuzz.c
> index d08a47e278..a33bd73067 100644
> --- a/tests/qtest/fuzz/virtio_net_fuzz.c
> +++ b/tests/qtest/fuzz/virtio_net_fuzz.c
> @@ -122,6 +122,7 @@ static void virtio_net_fork_fuzz(QTestState *s,
>  flush_events(s);
>  _Exit(0);
>  } else {
> +flush_events(s);
>  wait(NULL);
>  }
>  }
> @@ -134,6 +135,7 @@ static void virtio_net_fork_fuzz_check_used(QTestState *s,
>  flush_events(s);
>  _Exit(0);
>  } else {
> +flush_events(s);
>  wait(NULL);
>  }
>  }
> diff --git a/tests/qtest/fuzz/virtio_scsi_fuzz.c 
> b/tests/qtest/fuzz/virtio_scsi_fuzz.c
> index 3b95247f12..51dce491ab 100644
> --- a/tests/qtest/fuzz/virtio_scsi_fuzz.c
> +++ b/tests/qtest/fuzz/virtio_scsi_fuzz.c
> @@ -145,6 +145,7 @@ static void virtio_scsi_fork_fuzz(QTestState *s,
>  flush_events(s);
>  _Exit(0);
>  } else {
> +flush_events(s);
>  wait(NULL);
>  }
>  }
> @@ -164,6 +165,7 @@ static void virtio_scsi_with_flag_fuzz(QTestState *s,
>  }
>  _Exit(0);
>  } else {
> +flush_events(s);
>  wait(NULL);
>  }
>  }
> -- 
> 2.26.2

Re: [PATCH 3/4] fuzz: add mangled object name to linker script

[Darren Kenny]

On Monday, 2020-05-11 at 23:01:32 -04, Alexander Bulekov wrote:
> Previously, we relied on "FuzzerTracePC*(.bss*)" to place libfuzzer's
> fuzzer::TPC object into our contiguous shared-memory region. This does
> not work for some libfuzzer builds, so this addition identifies the
> region by its mangled name: *(.bss._ZN6fuzzer3TPCE);
>
> Signed-off-by: Alexander Bulekov 

FWIW, since I'm not really familiar with the syntax, but I understand
what the intent is:

Reviewed-by: Darren Kenny 


> ---
>  tests/qtest/fuzz/fork_fuzz.ld | 5 +
>  1 file changed, 5 insertions(+)
>
> This isn't ideal, but I looked at the libfuzzer builds packaged for
> debian, for versions 6, 7, 8, 9, 10 and 11 and this (mangled) object
> name appears consistently in the symbol tables.
>
> diff --git a/tests/qtest/fuzz/fork_fuzz.ld b/tests/qtest/fuzz/fork_fuzz.ld
> index e086bba873..bfb667ed06 100644
> --- a/tests/qtest/fuzz/fork_fuzz.ld
> +++ b/tests/qtest/fuzz/fork_fuzz.ld
> @@ -28,6 +28,11 @@ SECTIONS
>  
>/* Internal Libfuzzer TracePC object which contains the 
> ValueProfileMap */
>FuzzerTracePC*(.bss*);
> +  /*
> +   * In case the above line fails, explicitly specify the (mangled) name 
> of
> +   * the object we care about
> +   */
> +   *(.bss._ZN6fuzzer3TPCE);
>}
>.data.fuzz_end : ALIGN(4K)
>{
> -- 
> 2.26.2

Re: [PATCH v3 2/2] char-file: add test for distinct path= and pathin=

[Darren Kenny]

Hi Alex,

On Monday, 2020-05-11 at 23:47:50 -04, Alexander Bulekov wrote:
> Signed-off-by: Alexander Bulekov 

Thanks for making those changes.

Reviewed-by: Darren Kenny 

Thanks,

Darren.

> ---
>  tests/test-char.c | 96 +++
>  1 file changed, 96 insertions(+)
>
> diff --git a/tests/test-char.c b/tests/test-char.c
> index 3afc9b1b8d..6c66fae86a 100644
> --- a/tests/test-char.c
> +++ b/tests/test-char.c
> @@ -1228,6 +1228,101 @@ static void char_file_test_internal(Chardev *ext_chr, 
> const char *filepath)
>  g_free(out);
>  }
>  
> +static int file_can_read(void *opaque)
> +{
> +return 4096;
> +}
> +
> +static void file_read(void *opaque, const uint8_t *buf, int size)
> +{
> +int ret;
> +Chardev *chr = *(Chardev **)opaque;
> +g_assert_cmpint(size, <=, file_can_read(opaque));
> +
> +g_assert_cmpint(size, ==, 6);
> +g_assert(strncmp((const char *)buf, "hello!", 6) == 0);
> +ret = qemu_chr_write_all(chr, (const uint8_t *)"world!", 6);
> +g_assert_cmpint(ret, ==, 6);
> +quit = true;
> +}
> +
> +static void char_file_separate_input_file(void)
> +{
> +char *tmp_path = g_dir_make_tmp("qemu-test-char.XX", NULL);
> +char *in;
> +char *out;
> +QemuOpts *opts;
> +Chardev *chr;
> +ChardevFile file = {};
> +CharBackend be;
> +ChardevBackend backend = { .type = CHARDEV_BACKEND_KIND_FILE,
> +   .u.file.data = &file };
> +char *contents = NULL;
> +gsize length;
> +int ret;
> +time_t in_mtime;
> +GStatBuf file_stat;
> +
> +in = g_build_filename(tmp_path, "in", NULL);
> +out = g_build_filename(tmp_path, "out", NULL);
> +
> +ret = g_file_set_contents(in, "hello!", 6, NULL);
> +g_assert(ret == TRUE);
> +g_stat(in, &file_stat);
> +in_mtime = file_stat.st_mtime;
> +/*
> + * Sleep to ensure that if the following actions modify the file, the 
> mtime
> + * will be different
> + */
> +sleep(1);
> +opts = qemu_opts_create(qemu_find_opts("chardev"), "serial-id",
> +1, &error_abort);
> +qemu_opt_set(opts, "backend", "file", &error_abort);
> +qemu_opt_set(opts, "pathin", in, &error_abort);
> +qemu_opt_set(opts, "path", out, &error_abort);
> +
> +chr = qemu_chr_new_from_opts(opts, NULL, NULL);
> +qemu_chr_fe_init(&be, chr, &error_abort);
> +
> +file.has_in = true;
> +file.in = in;
> +file.out = out;
> +
> +
> +qemu_chr_fe_set_handlers(&be, file_can_read,
> + file_read,
> + NULL, NULL, &chr, NULL, true);
> +
> +chr = qemu_chardev_new(NULL, TYPE_CHARDEV_FILE, &backend,
> +   NULL, &error_abort);
> +g_assert_nonnull(chr);
> +
> +main_loop(); /* should call file_read, and copy contents of in to out */
> +
> +qemu_chr_fe_deinit(&be, true);
> +
> +/* Check that out was written to */
> +ret = g_file_get_contents(out, &contents, &length, NULL);
> +g_assert(ret == TRUE);
> +g_assert_cmpint(length, ==, 6);
> +g_assert(strncmp(contents, "world!", 6) == 0);
> +g_free(contents);
> +
> +/* Check that in hasn't been modified */
> +ret = g_file_get_contents(in, &contents, &length, NULL);
> +g_assert(ret == TRUE);
> +g_assert_cmpint(length, ==, 6);
> +g_assert(strncmp(contents, "hello!", 6) == 0);
> +g_stat(in, &file_stat);
> +g_assert(file_stat.st_mtime == in_mtime);
> +
> +g_free(contents);
> +g_rmdir(tmp_path);
> +g_free(tmp_path);
> +g_free(in);
> +g_free(out);
> +}
> +
>  static void char_file_test(void)
>  {
>  char_file_test_internal(NULL, NULL);
> @@ -1398,6 +1493,7 @@ int main(int argc, char **argv)
>  g_test_add_func("/char/pipe", char_pipe_test);
>  #endif
>  g_test_add_func("/char/file", char_file_test);
> +g_test_add_func("/char/file/pathin", char_file_separate_input_file);
>  #ifndef _WIN32
>  g_test_add_func("/char/file-fifo", char_file_fifo_test);
>  #endif
> -- 
> 2.26.2

Re: [PATCH] i386/kvm: fix a use-after-free when vcpu plug/unplug

[Pan Nengyuan]

On 5/12/2020 3:54 PM, Philippe Mathieu-Daudé wrote:
> On 5/12/20 3:39 PM, Pan Nengyuan wrote:
>> When we hotplug vcpus, cpu_update_state is added to vm_change_state_head
>> in kvm_arch_init_vcpu(). But it forgot to delete in kvm_arch_destroy_vcpu() 
>> after
>> unplug. Then it will cause a use-after-free access. This patch delete it in
>> kvm_arch_destroy_vcpu() to fix that.
>>
>> Reproducer:
>>  virsh setvcpus vm1 4 --live
>>  virsh setvcpus vm1 2 --live
>>  virsh suspend vm1
>>  virsh resume vm1
>>
>> The UAF stack:
>> ==qemu-system-x86_64==28233==ERROR: AddressSanitizer: heap-use-after-free on 
>> address 0x62e2e798 at pc 0x5573c6917d9e bp 0x7fff07139e50 sp 
>> 0x7fff07139e40
>> WRITE of size 1 at 0x62e2e798 thread T0
>>  #0 0x5573c6917d9d in cpu_update_state 
>> /mnt/sdb/qemu/target/i386/kvm.c:742
>>  #1 0x5573c699121a in vm_state_notify /mnt/sdb/qemu/vl.c:1290
>>  #2 0x5573c636287e in vm_prepare_start /mnt/sdb/qemu/cpus.c:2144
>>  #3 0x5573c6362927 in vm_start /mnt/sdb/qemu/cpus.c:2150
>>  #4 0x5573c71e8304 in qmp_cont /mnt/sdb/qemu/monitor/qmp-cmds.c:173
>>  #5 0x5573c727cb1e in qmp_marshal_cont qapi/qapi-commands-misc.c:835
>>  #6 0x5573c7694c7a in do_qmp_dispatch 
>> /mnt/sdb/qemu/qapi/qmp-dispatch.c:132
>>  #7 0x5573c7694c7a in qmp_dispatch /mnt/sdb/qemu/qapi/qmp-dispatch.c:175
>>  #8 0x5573c71d9110 in monitor_qmp_dispatch 
>> /mnt/sdb/qemu/monitor/qmp.c:145
>>  #9 0x5573c71dad4f in monitor_qmp_bh_dispatcher 
>> /mnt/sdb/qemu/monitor/qmp.c:234
>>
>> Reported-by: Euler Robot 
>> Signed-off-by: Pan Nengyuan 
>> ---
>>   target/i386/cpu.h | 1 +
>>   target/i386/kvm.c | 5 -
>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
>> index e818fc712a..afbd11b7a3 100644
>> --- a/target/i386/cpu.h
>> +++ b/target/i386/cpu.h
>> @@ -1631,6 +1631,7 @@ struct X86CPU {
>>     CPUNegativeOffsetState neg;
>>   CPUX86State env;
>> +    VMChangeStateEntry *vmsentry;
>>     uint64_t ucode_rev;
>>   diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>> index 4901c6dd74..ff2848357e 100644
>> --- a/target/i386/kvm.c
>> +++ b/target/i386/kvm.c
>> @@ -1770,7 +1770,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>   }
>>   }
>>   -    qemu_add_vm_change_state_handler(cpu_update_state, env);
>> +    cpu->vmsentry = qemu_add_vm_change_state_handler(cpu_update_state, env);
>>     c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0);
>>   if (c) {
>> @@ -1883,6 +1883,9 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
>>   env->nested_state = NULL;
>>   }
>>   +    qemu_del_vm_change_state_handler(cpu->vmsentry);
>> +    cpu->vmsentry = NULL;
> 
> Why set it to NULL? there is no non-NULL check.
> 
> Anyway if it matters to you, why not do it in 
> qemu_del_vm_change_state_handler()?

Yes, there is no non-NULL check and it will not be NULL at all.

Actually it doesn't matters to me, just habitually set to null after free.

I will remove it.

Thanks.

> 
> Otherwise:
> Reviewed-by: Philippe Mathieu-Daudé 
> 
>> +
>>   return 0;
>>   }
>>  
> 
> .

Re: [PATCH 04/12] hw/mips/fuloong2e: Fix typo in Fuloong machine name

[Philippe Mathieu-Daudé]

On 5/11/20 10:27 AM, Aleksandar Markovic wrote:

пон, 11. мај 2020. у 10:12 Aleksandar Markovic
 је написао/ла:


пон, 11. мај 2020. у 08:52 chen huacai  је написао/ла:


Hi, Philippe and Alexandar,

On Mon, May 11, 2020 at 2:38 PM Philippe Mathieu-Daudé  wrote:


On 5/11/20 8:21 AM, Aleksandar Markovic wrote:

пон, 11. мај 2020. у 03:11 chen huacai  је написао/ла:


Hi, Philippe,

On Mon, May 11, 2020 at 5:06 AM Philippe Mathieu-Daudé  wrote:


We always miswrote the Fuloong machine... Fix its name.
Add an machine alias to the previous name for backward
compatibility.

Suggested-by: Aleksandar Markovic 
Signed-off-by: Philippe Mathieu-Daudé 
---
   docs/system/target-mips.rst  |  2 +-
   default-configs/mips64el-softmmu.mak |  2 +-
   hw/isa/vt82c686.c|  2 +-
   hw/mips/{mips_fulong2e.c => fuloong2e.c} | 46 

Use mips_fuloong2e.c instead of fuloong2e.c? Other machine file names
also have a "mips_" prefix.



I would leave mips_ prefix for Fuloong, and actually add it to Boston
source file, so that we are finally consistent across all MIPS
machines.

What do you think?


These names were used years ago when all hardware was in the same hw/
directory, not sorted per target. Now new machines don't use the target
as prefix name. I'd clean the other way around, and dropping the 'mips_'
prefix. The positive side is we can 5 more characters to better describe
a patch while limited by the 72 chars in the subject :)


All having the prefix, or all dropping the prefix, are both good for
me, just keep consistency.

Huacai



Philippe, Huacai,

Prefix or not, I have mixed feelings. I had consistency more in mind
than prefix.

So it seems the prevailing opinion is slightly on the side of dropping
prefix "mips_".

Philippe, if it is not too difficult, could you perhaps make dropping
that prefix for all source file names in hw/mips a part of the this
series (not to complicate situation with a separate series) in its
follow-up version (but perhaps keep that change(s) in separate
patch(es))?


Certainly not difficult, but I won't take that as a priority.
I already spend more time trying to document better the MIPS commits, as 
it is important to you, and it also serves the community. Regarding 
unifying the file names I don't care much, so I'll not rename this file 
to stop bikeshredding on futile topics and keep focus on the technical 
changes of the patches.






Conveniently enough, most of involved files do not have checkpatch
warnings and errors:

$ ../../scripts/checkpatch.pl --strict -f ./mips_fulong2e.c
total: 0 errors, 0 warnings, 404 lines checked

./mips_fulong2e.c has no obvious style problems and is ready for submission.
$ ../../scripts/checkpatch.pl --strict -f ./mips_malta.c
ERROR: if this code is redundant consider removing it
#430: FILE: ./mips_malta.c:430:
+#if 0

ERROR: if this code is redundant consider removing it
#518: FILE: ./mips_malta.c:518:
+#if 0

total: 2 errors, 0 warnings, 1458 lines checked

./mips_malta.c has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
$ ../../scripts/checkpatch.pl --strict -f ./mips_mipssim.c
total: 0 errors, 0 warnings, 246 lines checked

./mips_mipssim.c has no obvious style problems and is ready for submission.
$ ../../scripts/checkpatch.pl --strict -f ./mips_r4k.c
total: 0 errors, 0 warnings, 318 lines checked

./mips_r4k.c has no obvious style problems and is ready for submission.


Maybe we should also finally get rid of these segments in mips_malta.c:

#if 0
 printf("malta_fpga_read: Bad register offset 0x" TARGET_FMT_lx "\n",
addr);
#endif

and

#if 0
 printf("malta_fpga_write: Bad register offset 0x" TARGET_FMT_lx "\n",
addr);
#endif

possibly replacing them with some logging?


Yes, I had it done before the checkpatch fixes on hw/mips/, now I have 
to rebase. Instead I postpone for the day I find interest in playing 
with the Malta. Remember this is a hobbyist board (the Boston being the 
corporate quality one).




Philippe?

Thanks,
Aleksandar




Sincerely,
Aleksandar





Aleksandar


Huacai

   hw/pci-host/bonito.c |  8 ++---
   tests/qtest/endianness-test.c|  2 +-
   MAINTAINERS  |  4 +--
   hw/mips/Kconfig  |  2 +-
   hw/mips/Makefile.objs|  2 +-
   9 files changed, 36 insertions(+), 34 deletions(-)
   rename hw/mips/{mips_fulong2e.c => fuloong2e.c} (91%)

diff --git a/docs/system/target-mips.rst b/docs/system/target-mips.rst
index 2736fd0509..cd2a931edf 100644
--- a/docs/system/target-mips.rst
+++ b/docs/system/target-mips.rst
@@ -74,7 +74,7 @@ The MIPS Magnum R4000 emulation supports:

   -  G364 framebuffer

-The Fulong 2E emulation supports:
+The Fuloong 2E emulation supports:

   -  Loongson 2E CPU

diff --git a/default-configs/mips64el-softmmu.m

Re: [PATCH 2/4] fuzz: fix typo in i440fx-qtest-reboot arguments

[Philippe Mathieu-Daudé]

On 5/12/20 5:01 AM, Alexander Bulekov wrote:

Signed-off-by: Alexander Bulekov 
---
  tests/qtest/fuzz/i440fx_fuzz.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/fuzz/i440fx_fuzz.c b/tests/qtest/fuzz/i440fx_fuzz.c
index ab5f112584..90e75ffaea 100644
--- a/tests/qtest/fuzz/i440fx_fuzz.c
+++ b/tests/qtest/fuzz/i440fx_fuzz.c
@@ -143,7 +143,7 @@ static void i440fx_fuzz_qos_fork(QTestState *s,
  }
  
  static const char *i440fx_qtest_argv = TARGET_NAME " -machine accel=qtest"

-   "-m 0 -display none";
+   " -m 0 -display none";
  static const char *i440fx_argv(FuzzTarget *t)
  {
  return i440fx_qtest_argv;



Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH RESEND v6 01/36] memory: alloc RAM from file at offset

[Stefan Hajnoczi]

On Wed, Apr 22, 2020 at 09:13:36PM -0700, elena.ufimts...@oracle.com wrote:
> From: Jagannathan Raman 
> 
> Allow RAM MemoryRegion to be created from an offset in a file, instead
> of allocating at offset of 0 by default. This is needed to synchronize
> RAM between QEMU & remote process.
> 
> Signed-off-by: Jagannathan Raman 
> Signed-off-by: John G Johnson 
> Signed-off-by: Elena Ufimtseva 
> Reviewed-by: Dr. David Alan Gilbert 
> ---
>  exec.c| 11 +++
>  include/exec/ram_addr.h   |  2 +-
>  include/qemu/mmap-alloc.h |  3 ++-
>  memory.c  |  2 +-
>  util/mmap-alloc.c |  7 ---
>  util/oslib-posix.c|  2 +-
>  6 files changed, 16 insertions(+), 11 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

AW: [PATCH 1/1] tricore: added AURIX TC277 D-Step TriBoard

[Konopik, Andreas (EFS-GH2)]

Hello Philippe,

thank you for your feedback, implementing the TriBoard within an SoC structure 
seems to be best practice.
We will rewrite the patch accordingly.

I also attached an elf with an empty main-function to test the Machine.

Best regards,

Andreas

> -Ursprüngliche Nachricht-
> Von: Philippe Mathieu-Daudé 
> Gesendet: Montag, 11. Mai 2020 15:48
> An: David Brenken ; qemu-devel@nongnu.org
> Cc: kbast...@mail.uni-paderborn.de; Biermanski, Lars (EFS-GH3)
> ; Hofstetter, Georg (EFS-GH2)
> ; Brenken, David (EFS-GH5)
> ; Rasche, Robert (EFS-GH2)  auto.de>; Konopik, Andreas (EFS-GH2) 
> Betreff: Re: [PATCH 1/1] tricore: added AURIX TC277 D-Step TriBoard
> 
> Hello David,
> 
> On 5/11/20 2:21 PM, David Brenken wrote:
> > From: Andreas Konopik 
> >
> > Signed-off-by: Andreas Konopik 
> > Signed-off-by: David Brenken 
> > Signed-off-by: Georg Hofstetter 
> > Signed-off-by: Robert Rasche 
> > Signed-off-by: Lars Biermanski 
> > ---
> >   hw/tricore/Makefile.objs   |   1 +
> >   hw/tricore/aurix_triboard_tc277d.c | 240
> +
> >   2 files changed, 241 insertions(+)
> >   create mode 100644 hw/tricore/aurix_triboard_tc277d.c
> >
> > diff --git a/hw/tricore/Makefile.objs b/hw/tricore/Makefile.objs index
> > 5501f6c1a8..e4a2106dd9 100644
> > --- a/hw/tricore/Makefile.objs
> > +++ b/hw/tricore/Makefile.objs
> > @@ -1 +1,2 @@
> >   obj-$(CONFIG_TRICORE) += tricore_testboard.o
> > +obj-$(CONFIG_TRICORE) += aurix_triboard_tc277d.o
> > diff --git a/hw/tricore/aurix_triboard_tc277d.c
> > b/hw/tricore/aurix_triboard_tc277d.c
> > new file mode 100644
> > index 00..86deff9a50
> > --- /dev/null
> > +++ b/hw/tricore/aurix_triboard_tc277d.c
> > @@ -0,0 +1,240 @@
> > +/*
> > + * Infineon AURIX TC277 D-Step TriBoard System emulation.
> > + *
> > + * Copyright (c) 2019 Andreas Konopik 
> > + * Copyright (c) 2019 David Brenken 
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, see 
> > .
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/units.h"
> > +#include "qapi/error.h"
> > +#include "hw/qdev-properties.h"
> > +#include "cpu.h"
> > +#include "net/net.h"
> > +#include "hw/boards.h"
> > +#include "hw/loader.h"
> > +#include "exec/address-spaces.h"
> > +#include "elf.h"
> > +#include "hw/tricore/tricore.h"
> > +#include "qemu/error-report.h"
> > +
> > +/*
> > + * The TriCore processor architecture defines their respective
> > +on-chip memory
> > + * layout. Therefore, in contrast to other common architectures, the
> > +QEMU
> > + * TriCore machine and cpu are closely coupled.
> > + *
> > + * Memory maps are aligned with the TC27x D-Step Microcontroller layout.
> > + */
> > +
> > +#define TC27XD_MEMMAP_DSPR20x5000
> > +#define TC27XD_MEMMAP_DCACHE2  0x5001E000
> > +#define TC27XD_MEMMAP_DTAG20x500C
> > +#define TC27XD_MEMMAP_PSPR20x5010
> > +#define TC27XD_MEMMAP_PCACHE2  0x50108000
> > +#define TC27XD_MEMMAP_PTAG20x501C
> > +#define TC27XD_MEMMAP_DSPR10x6000
> > +#define TC27XD_MEMMAP_DCACHE1  0x6001E000
> > +#define TC27XD_MEMMAP_DTAG10x600C
> > +#define TC27XD_MEMMAP_PSPR10x6010
> > +#define TC27XD_MEMMAP_PCACHE1  0x60108000
> > +#define TC27XD_MEMMAP_PTAG10x601C
> > +#define TC27XD_MEMMAP_DSPR00x7000
> > +#define TC27XD_MEMMAP_PSPR00x7010
> > +#define TC27XD_MEMMAP_PCACHE0  0x70106000
> > +#define TC27XD_MEMMAP_PTAG00x701C
> > +#define TC27XD_MEMMAP_PFLASH0_C0x8000
> > +#define TC27XD_MEMMAP_PFLASH1_C0x8020
> > +#define TC27XD_MEMMAP_OLDA_C   0x8FE7
> > +#define TC27XD_MEMMAP_BROM_C   0x8FFF8000
> > +#define TC27XD_MEMMAP_LMURAM_C 0x9000
> > +#define TC27XD_MEMMAP_EMEM_C   0x9F00
> > +#define TC27XD_MEMMAP_PFLASH0_U0xA000
> > +#define TC27XD_MEMMAP_PFLASH1_U0xA020
> > +#define TC27XD_MEMMAP_DFLASH0  0xAF00
> > +#define TC27XD_MEMMAP_DFLASH1  0xAF11
> > +#define TC27XD_MEMMAP_OLDA_U   0xAFE7
> > +#define TC27XD_MEMMAP_BROM_U   0xAFFF8000
> > +#define TC27XD_MEMMAP_LMURAM_U 0xB000
> > +#define TC27XD_MEMMAP_EMEM_U   0xBF00
> > +#define TC27XD_MEMMAP_PSPRX0xC000
> > +#define TC27XD_MEMMAP_DSPRX0xD000
> > +
> > +static str

Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times

[Nikolay Igotti]

--- counter.c

#include 
#include 
#include 
#include 
#include 
#include 

#include 

#include 

QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;

// Files with descriptors after this one are intercepted for instruction
counting marks.
#define CATCH_BASE 0xcafebabe

static uint64_t insn_count = 0;
static pthread_t counting = false;
static pthread_t counting_for = 0;
static bool on_every_close = false;

static void vcpu_insn_exec_before(unsigned int cpu_index, void *udata)
{
if (counting && pthread_self() == counting_for)
insn_count++;
}

static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
{
size_t n = qemu_plugin_tb_n_insns(tb);
size_t i;

for (i = 0; i < n; i++) {
struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);

// TODO: do this call only on first insn in bb.
qemu_plugin_register_vcpu_insn_exec_cb(
insn, vcpu_insn_exec_before, QEMU_PLUGIN_CB_NO_REGS, NULL);
}
}

static void print_insn_count(void) {
g_autofree gchar *out = g_strdup_printf("executed %" PRIu64 "
instructions\n", insn_count);
qemu_plugin_outs(out);
}

static void vcpu_syscall(qemu_plugin_id_t id, unsigned int vcpu_index,
int64_t num, uint64_t a1, uint64_t a2,
uint64_t a3, uint64_t a4, uint64_t a5,
uint64_t a6, uint64_t a7, uint64_t a8)
{
// We put our listener on fd reads in range [CATCH_BASE, CATCH_BASE + 1]
if (num == 0) { // sys_read
switch (a1)
{
case CATCH_BASE + 0:
counting = true;
counting_for = pthread_self();
insn_count = 0;
break;
case CATCH_BASE + 1: {
counting = false;
counting_for = 0;
if (a3 == 8) {
// In case of user emulation in QEMU, addresses are 1:1
translated, so we can tell the caller
// number of executed instructions by just writing into
the buffer argument of read.
*(uint64_t*)a2 = insn_count;
}
print_insn_count();
break;
}
default:
break;
}
}
if (num == 3 && on_every_close) { // sys_close
print_insn_count();
}
}

QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
   const qemu_info_t *info,
   int argc, char **argv)
{
int i;
for (i = 0; i < argc; i++) {
if (!strcmp(argv[i], "on_every_close")) {
on_every_close = true;
counting = true;
counting_for = pthread_self();
}
}

qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
qemu_plugin_register_vcpu_syscall_cb(id, vcpu_syscall);
return 0;
}

--- test.c

#include 
#include 
#include 
#include 
#include 

#define CATCH_BASE 0xcafebabe

static void start_counting() {
char buf;
int rv = read(CATCH_BASE, &buf, 1);
(void)rv;
}

static void end_counting() {
uint64_t counter = 0;
int rv = read(CATCH_BASE + 1, &counter, sizeof(counter));
(void)rv;
printf("We got %lld from TCG\n", counter);
}

int global = 0;

typedef struct {
int delay;
} ThreadArg;

static void* thread_fn(void* varg)  {
ThreadArg* arg = varg;
usleep(arg->delay);
free(arg);
return NULL;
}

int main(int argc, char** argv) {
int i;
int repeat = 100;
#define THREAD_NUM 10
pthread_t threads[THREAD_NUM];

if (argc > 1) {
repeat = atoi(argv[1]);
}

for (i = 0; i < THREAD_NUM; i++) {
ThreadArg* arg = calloc(sizeof(ThreadArg), 1);
arg->delay = i * 100;
pthread_create(threads + i, NULL, thread_fn, arg);
}

start_counting();
for (i = 0; i < repeat; i++) {
global += i;
}
end_counting();

for (i = 0; i < THREAD_NUM; i++) {
pthread_join(threads[i], NULL);
}

return 0;
}

On Tue, May 12, 2020 at 3:55 AM Emilio G. Cota  wrote:

> On Mon, May 11, 2020 at 18:53:19 +0300, Nikolay Igotti wrote:
> > Attached to the mail counter.c when running with attached test.c compiled
> > to Linux standalone binary shows failing assert, unless the patch is
> > applied.
>
> I didn't get the attachment. Can you paste the code at the end of your
> reply?
>
> Thanks,
> Emilio
>

Re: AW: [PATCH 1/1] tricore: added AURIX TC277 D-Step TriBoard

[Philippe Mathieu-Daudé]

On 5/12/20 10:32 AM, Konopik, Andreas (EFS-GH2) wrote:

Hello Philippe,

thank you for your feedback, implementing the TriBoard within an SoC structure 
seems to be best practice.
We will rewrite the patch accordingly.

I also attached an elf with an empty main-function to test the Machine.


Thanks, I am getting:

$ qemu-system-tricore -M AURIX_TriBoard_TC277D \
  -kernel hello_world.elf -d unimp,in_asm
qemu-system-tricore: function cpu_get_phys_page_attrs_debug not 
implemented, aborting




Best regards,

Andreas


-Ursprüngliche Nachricht-
Von: Philippe Mathieu-Daudé 
Gesendet: Montag, 11. Mai 2020 15:48
An: David Brenken ; qemu-devel@nongnu.org
Cc: kbast...@mail.uni-paderborn.de; Biermanski, Lars (EFS-GH3)
; Hofstetter, Georg (EFS-GH2)
; Brenken, David (EFS-GH5)
; Rasche, Robert (EFS-GH2) ; Konopik, Andreas (EFS-GH2) 
Betreff: Re: [PATCH 1/1] tricore: added AURIX TC277 D-Step TriBoard

Hello David,

On 5/11/20 2:21 PM, David Brenken wrote:

From: Andreas Konopik 

Signed-off-by: Andreas Konopik 
Signed-off-by: David Brenken 
Signed-off-by: Georg Hofstetter 
Signed-off-by: Robert Rasche 
Signed-off-by: Lars Biermanski 
---
   hw/tricore/Makefile.objs   |   1 +
   hw/tricore/aurix_triboard_tc277d.c | 240

+

   2 files changed, 241 insertions(+)
   create mode 100644 hw/tricore/aurix_triboard_tc277d.c

diff --git a/hw/tricore/Makefile.objs b/hw/tricore/Makefile.objs index
5501f6c1a8..e4a2106dd9 100644
--- a/hw/tricore/Makefile.objs
+++ b/hw/tricore/Makefile.objs
@@ -1 +1,2 @@
   obj-$(CONFIG_TRICORE) += tricore_testboard.o
+obj-$(CONFIG_TRICORE) += aurix_triboard_tc277d.o
diff --git a/hw/tricore/aurix_triboard_tc277d.c

[...]

Re: [PATCH RESEND v6 01/36] memory: alloc RAM from file at offset

[Daniel P . Berrangé]

On Wed, Apr 22, 2020 at 09:13:36PM -0700, elena.ufimts...@oracle.com wrote:
> From: Jagannathan Raman 
> 
> Allow RAM MemoryRegion to be created from an offset in a file, instead
> of allocating at offset of 0 by default. This is needed to synchronize
> RAM between QEMU & remote process.

Can you elaborate on why remote processes require the RAM to be offset
from zero ?

NB, I'm not objecting - I'm just curious to understand more.

> 
> Signed-off-by: Jagannathan Raman 
> Signed-off-by: John G Johnson 
> Signed-off-by: Elena Ufimtseva 
> Reviewed-by: Dr. David Alan Gilbert 
> ---
>  exec.c| 11 +++
>  include/exec/ram_addr.h   |  2 +-
>  include/qemu/mmap-alloc.h |  3 ++-
>  memory.c  |  2 +-
>  util/mmap-alloc.c |  7 ---
>  util/oslib-posix.c|  2 +-
>  6 files changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 2874bb5088..d0ac9545f4 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1801,6 +1801,7 @@ static void *file_ram_alloc(RAMBlock *block,
>  ram_addr_t memory,
>  int fd,
>  bool truncate,
> +off_t offset,
>  Error **errp)
>  {
>  void *area;
> @@ -1851,7 +1852,8 @@ static void *file_ram_alloc(RAMBlock *block,
>  }
>  
>  area = qemu_ram_mmap(fd, memory, block->mr->align,
> - block->flags & RAM_SHARED, block->flags & RAM_PMEM);
> + block->flags & RAM_SHARED, block->flags & RAM_PMEM,
> + offset);
>  if (area == MAP_FAILED) {
>  error_setg_errno(errp, errno,
>   "unable to map backing store for guest RAM");
> @@ -2283,7 +2285,7 @@ static void ram_block_add(RAMBlock *new_block, Error 
> **errp, bool shared)
>  #ifdef CONFIG_POSIX
>  RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
>   uint32_t ram_flags, int fd,
> - Error **errp)
> + off_t offset, Error **errp)
>  {
>  RAMBlock *new_block;
>  Error *local_err = NULL;
> @@ -2328,7 +2330,8 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, 
> MemoryRegion *mr,
>  new_block->used_length = size;
>  new_block->max_length = size;
>  new_block->flags = ram_flags;
> -new_block->host = file_ram_alloc(new_block, size, fd, !file_size, errp);
> +new_block->host = file_ram_alloc(new_block, size, fd, !file_size, offset,
> + errp);
>  if (!new_block->host) {
>  g_free(new_block);
>  return NULL;
> @@ -2358,7 +2361,7 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, 
> MemoryRegion *mr,
>  return NULL;
>  }
>  
> -block = qemu_ram_alloc_from_fd(size, mr, ram_flags, fd, errp);
> +block = qemu_ram_alloc_from_fd(size, mr, ram_flags, fd, 0, errp);
>  if (!block) {
>  if (created) {
>  unlink(mem_path);
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 5e59a3d8d7..1b9f489ff0 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -121,7 +121,7 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, 
> MemoryRegion *mr,
> Error **errp);
>  RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
>   uint32_t ram_flags, int fd,
> - Error **errp);
> + off_t offset, Error **errp);
>  
>  RAMBlock *qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
>MemoryRegion *mr, Error **errp);
> diff --git a/include/qemu/mmap-alloc.h b/include/qemu/mmap-alloc.h
> index e786266b92..4f579858bc 100644
> --- a/include/qemu/mmap-alloc.h
> +++ b/include/qemu/mmap-alloc.h
> @@ -25,7 +25,8 @@ void *qemu_ram_mmap(int fd,
>  size_t size,
>  size_t align,
>  bool shared,
> -bool is_pmem);
> +bool is_pmem,
> +off_t start);
>  
>  void qemu_ram_munmap(int fd, void *ptr, size_t size);
>  
> diff --git a/memory.c b/memory.c
> index 601b749906..f5fec476b7 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1596,7 +1596,7 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
>  mr->destructor = memory_region_destructor_ram;
>  mr->ram_block = qemu_ram_alloc_from_fd(size, mr,
> share ? RAM_SHARED : 0,
> -   fd, &err);
> +   fd, 0, &err);
>  mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
>  if (err) {
>  mr->size = int128_zero();
> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
> index 27dcccd8ec..a28f7025f0 100644
> -

Re: [PATCH 2/5] io/channel.c,io/channel-socket.c: Add yank feature

[Daniel P . Berrangé]

On Mon, May 11, 2020 at 10:00:42PM +0200, Lukas Straub wrote:
> On Mon, 11 May 2020 12:51:46 +0100
> Daniel P. Berrangé  wrote:
> 
> > On Mon, May 11, 2020 at 01:14:41PM +0200, Lukas Straub wrote:
> > > Add qio_channel_set_yank function to channel and to channel-socket,
> > > which will register a yank function. The yank function calls
> > > shutdown() on the socket.
> > > 
> > > Signed-off-by: Lukas Straub 
> > > ---
> > >  Makefile.objs   |  1 +
> > >  include/io/channel-socket.h |  1 +
> > >  include/io/channel.h| 12 
> > >  io/channel-socket.c | 29 +
> > >  io/channel.c|  9 +
> > >  5 files changed, 52 insertions(+)  
> > 
> > Assuming we want the yank feature (which I'm not entirely convinced
> > of), then I don't think any of this addition should exist. The
> > QIOChannel class already provides a "qio_channel_shutdown" method
> > which can be invoked. The layer above which is using the QIOChannel
> > should be calling this existing qio_channel_shutdown method in
> > response to any yank request.  The I/O layer shouldn't have any
> > direct dependancy on the yank feature.
> 
> Having the code here simplifys the code in the other places.

This introduces a depedancy from the IO channel code into the system
emulator infra which is not desired. The goal is to keep the io/ module
isolated and as self-contained as possible.

I don't think it makes a significant difference to the yank code to
keep it out of the io layer, and simply call the QIOChannel objects
via their existing public API.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH 3/5] block/nbd.c: Add yank feature

[Daniel P . Berrangé]

On Mon, May 11, 2020 at 07:05:24PM +0200, Lukas Straub wrote:
> On Mon, 11 May 2020 17:19:09 +0100
> "Dr. David Alan Gilbert"  wrote:
> 
> > * Lukas Straub (lukasstra...@web.de) wrote:
> > > Add yank option, pass it to the socket-channel and register a yank
> > > function which sets s->state = NBD_CLIENT_QUIT. This is the same
> > > behaviour as if an error occured.
> > > 
> > > Signed-off-by: Lukas Straub   
> > 
> > > +static void nbd_yank(void *opaque)
> > > +{
> > > +BlockDriverState *bs = opaque;
> > > +BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
> > > +
> > > +atomic_set(&s->state, NBD_CLIENT_QUIT);  
> > 
> > I think I was expecting a shutdown on the socket here - why doesn't it
> > have one?
> 
> For nbd, we register two yank functions: This one and we enable
> the yank feature on the qio channel (see function
> nbd_establish_connection below).

As mentioned on the earlier patch, I don't want to see any yank
code in the QIOChannel object directly. This nbd_yank function
can simply call the qio_channel_shutdown() function directly
and avoid need for modifying the QIOChannel object with yank
support.

This will make the NBD code clearer too, as we can see exactly
what actions are performed at NBD layer when a yank happens,
which is what David was not seeing clearly here.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v3 1/2] scripts/qemugdb: Remove shebang header

[Kevin Wolf]

Am 12.05.2020 um 09:06 hat Philippe Mathieu-Daudé geschrieben:
> These scripts are loaded as plugin by GDB (and they don't
> have any __main__ entry point). Remove the shebang header.
> 
> Acked-by: Alex Bennée 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  scripts/qemugdb/__init__.py  | 3 +--
>  scripts/qemugdb/aio.py   | 3 +--
>  scripts/qemugdb/coroutine.py | 3 +--
>  scripts/qemugdb/mtree.py | 4 +---
>  scripts/qemugdb/tcg.py   | 1 -

There is still a shebang line left in scripts/qemugdb/timers.py.

Kevin

Re: [PATCH RESEND v6 11/36] multi-process: define mpqemu-link object

[Stefan Hajnoczi]

On Wed, Apr 22, 2020 at 09:13:46PM -0700, elena.ufimts...@oracle.com wrote:
> From: Jagannathan Raman 
> 
> Defines mpqemu-link object which forms the communication link between
> QEMU & emulation program.
> Adds functions to configure members of mpqemu-link object instance.
> Adds functions to send and receive messages over the communication
> channel.
> Adds GMainLoop to handle events received on the communication channel.
> 
> Signed-off-by: Jagannathan Raman 
> Signed-off-by: John G Johnson 
> Signed-off-by: Elena Ufimtseva 

This will change a lot when integrated into the QEMU event loop so I've
skipped a lot of the code.

QIOChannel is probably the appropriate object to use instead of directly
accessing a file descriptor.

> +/**
> + * mpqemu_cmd_t:
> + *
> + * proc_cmd_t enum type to specify the command to be executed on the remote
> + * device.
> + */
> +typedef enum {
> +INIT = 0,
> +MAX,
> +} mpqemu_cmd_t;
> +
> +/**
> + * MPQemuMsg:
> + * @cmd: The remote command
> + * @bytestream: Indicates if the data to be shared is structured (data1)
> + *  or unstructured (data2)
> + * @size: Size of the data to be shared
> + * @data1: Structured data
> + * @fds: File descriptors to be shared with remote device
> + * @data2: Unstructured data
> + *
> + * MPQemuMsg Format of the message sent to the remote device from QEMU.
> + *
> + */
> +typedef struct {
> +mpqemu_cmd_t cmd;

Please use an int field on the wire because the C standard says:

  Each enumerated type shall be compatible with char, a signed integer
  type, or an unsigned integer type. The choice of type is
  implementation-defined, but shall be capable of representing the
  values of all the members of the enumeration.

So the compiler may make this a char field (which would introduce
padding before the bytestream field) but if a new enum constant FOO =
0x100 is added then the compiler might change the size to 16-bit.

> +int mpqemu_msg_recv(MPQemuMsg *msg, MPQemuChannel *chan)
> +{
> +int rc;
> +uint8_t *data;
> +union {
> +char control[CMSG_SPACE(REMOTE_MAX_FDS * sizeof(int))];
> +struct cmsghdr align;
> +} u;
> +struct msghdr hdr;
> +struct cmsghdr *chdr;
> +size_t fdsize;
> +int sock = chan->sock;
> +QemuMutex *lock = &chan->recv_lock;
> +
> +struct iovec iov = {
> +.iov_base = (char *) msg,
> +.iov_len = MPQEMU_MSG_HDR_SIZE,
> +};
> +
> +memset(&hdr, 0, sizeof(hdr));
> +memset(&u, 0, sizeof(u));
> +
> +hdr.msg_iov = &iov;
> +hdr.msg_iovlen = 1;
> +hdr.msg_control = &u;
> +hdr.msg_controllen = sizeof(u);
> +
> +WITH_QEMU_LOCK_GUARD(lock) {
> +do {
> +rc = recvmsg(sock, &hdr, 0);
> +} while (rc < 0 && (errno == EINTR || errno == EAGAIN));
> +
> +if (rc < 0) {

Missing rc != MPQEMU_MSG_HDR_SIZE check. If this was a short read we
should not attempt to parse uninitialized bytes in msg.

This is more defensive than relying on catching bogus input values later
on and also protects against accidentally revealing uninitialized memory
contents by observing our error handling response.

> +qemu_log_mask(LOG_REMOTE_DEBUG, "%s - recvmsg rc is %d, "
> +  "errno is %d, sock %d\n", __func__, rc, errno, 
> sock);
> +return rc;
> +}
> +
> +msg->num_fds = 0;
> +for (chdr = CMSG_FIRSTHDR(&hdr); chdr != NULL;
> + chdr = CMSG_NXTHDR(&hdr, chdr)) {
> +if ((chdr->cmsg_level == SOL_SOCKET) &&
> +(chdr->cmsg_type == SCM_RIGHTS)) {
> +fdsize = chdr->cmsg_len - CMSG_LEN(0);
> +msg->num_fds = fdsize / sizeof(int);
> +if (msg->num_fds > REMOTE_MAX_FDS) {
> +qemu_log_mask(LOG_REMOTE_DEBUG,
> +  "%s: Max FDs exceeded\n", __func__);
> +return -ERANGE;
> +}
> +
> +memcpy(msg->fds, CMSG_DATA(chdr), fdsize);
> +break;
> +}
> +}
> +
> +if (msg->bytestream) {
> +if (!msg->size) {
> +qemu_mutex_unlock(lock);

Duplicate unlock, we're already inside WITH_QEMU_LOCK_GUARD().

> +return -EINVAL;
> +}
> +
> +msg->data2 = calloc(1, msg->size);

What is the maximum message size? Please pick one and enforce it to
protect against huge allocations that cause us to run out of memory.

> +data = msg->data2;
> +} else {
> +data = (uint8_t *)&msg->data1;

Adding a uint8_t member to the union eliminates the need for a cast:

  union {
  uint64_t u64;
  uint8_t u8;
  } data1;

  ...

  data = &msg->data1.u8;

> +}
> +
> +if (msg->size) {
> +do {
> +rc = read(sock, data, msg->size);
> +} while (rc < 0 && (errno == EINTR || errno == EAGAIN));
> +}

Short reads are a

Re: [PATCH 4/5] chardev/char-socket.c: Add yank feature

[Daniel P . Berrangé]

On Mon, May 11, 2020 at 01:14:47PM +0200, Lukas Straub wrote:
> Add yank option which is passed to the socket-channel.
> 
> Signed-off-by: Lukas Straub 
> ---
>  chardev/char-socket.c | 8 
>  chardev/char.c| 3 +++
>  qapi/char.json| 5 -
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 185fe38dda..e476358941 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -65,6 +65,7 @@ typedef struct {
>  int max_size;
>  int do_telnetopt;
>  int do_nodelay;
> +int do_yank;
>  int *read_msgfds;
>  size_t read_msgfds_num;
>  int *write_msgfds;
> @@ -877,6 +878,9 @@ static int tcp_chr_new_client(Chardev *chr, 
> QIOChannelSocket *sioc)
>  if (s->do_nodelay) {
>  qio_channel_set_delay(s->ioc, false);
>  }
> +if (s->do_yank) {
> +qio_channel_set_yank(s->ioc, true);
> +}

This should call yank_register_function() to  register a local
callback, which then invokes qio_channel_shutdown() upon acting,
avoiding the need to add this qio_channel_set_yank method.

Likewise for the migration code in the next patch.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[PATCH] qemu-nbd: Close inherited stderr

[Raphael Pour]

Hello,

after e6df58a5, the inherited stderr 'old_stderr' won't get closed
anymore if 'fork_process' is false. This causes other processes relying
on EOF to infinitely block or crash.

From 47ab9b517038d13117876a8bb3ef45c53d7f2f9e Mon Sep 17 00:00:00 2001
From: "Raphael Pour" 
Date: Tue, 12 May 2020 10:18:44 +0200
Subject: [PATCH] qemu-nbd: Close inherited stderr

Close inherited stderr of the parent if fork_process is false.
Otherwise no one will close it. (introduced by e6df58a5)

Signed-off-by: Raphael Pour 
---
 qemu-nbd.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 108a51f7e..f2981e18a 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1032,8 +1032,15 @@ int main(int argc, char **argv)
 exit(EXIT_FAILURE);
 }

-/* ... close the descriptor we inherited and go on.  */
+/* ... close the descriptor we inherited and ...  */
 close(stderr_fd[1]);
+
+/* ... also close the old_stderr IF fork_process is false
otherwise
+ * it will never get closed.
+ */
+if (!fork_process) {
+  close(old_stderr);
+}
 } else {
 bool errors = false;
 char *buf;
-- 
2.25.4


-- 
Hetzner Online GmbH
Am Datacenter-Park 1
08223 Falkenstein/Vogtland
raphael.p...@hetzner.com
www.hetzner.com

Registergericht Ansbach, HRB 6089
Geschäftsführer: Martin Hetzner, Stephan Konvickova, Günther Müller



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v3 2/2] scripts/qmp: Use Python 3 interpreter

[Kevin Wolf]

Am 12.05.2020 um 09:06 hat Philippe Mathieu-Daudé geschrieben:
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Kevin Wolf 

[Bug 1877794] Re: Constant Folding on 64-bit Subtraction causes SIGILL on linux-user glxgears ppc64le to x86_64 by way of generating bad shift instruction with c=-1

[Laurent Vivier]

QEMU has been selected to have a GSoC intern to work on new ioctl():
https://wiki.qemu.org/Google_Summer_of_Code_2020#Extend_linux-user_syscalls_and_ioctls

Perhaps he can help you by working on DRM ioctl()?
Perhaps you can send an RFC series to the QEMU mailing list?

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1877794

Title:
  Constant Folding on 64-bit Subtraction causes SIGILL on linux-user
  glxgears ppc64le to x86_64 by way of generating bad shift instruction
  with c=-1

Status in QEMU:
  New

Bug description:
  Hello, I've been recently working on my own little branch of QEMU
  implementing the drm IOCTLs, when I discovered that glxgears seems to
  crash in GLXSwapBuffers(); with a SIGILL. I investigated this for
  about 2 weeks, manually trying to trace the call stack, only to find
  that we seemingly crash in a bad shift instruction. Originally
  intended to be an shr_i64 generated to an RLDICL, we end up with an
  all ones(-1) c value, which gets thrown to the macro for generating
  the MB, and replaces the instruction with mostly ones. This new
  instruction, FFE0 is invalid on ppc64le, and crashes in a host
  SIGILL in codegen_buffer. I tried to see if the output of translate.c
  had this bad instruction, but all I got were two (shr eax, cl)
  instructions, and upon creating a test program with shr (eax, cl) in
  it, nothing happened. Then figuring that there was nothing actually
  wrong with the instruction in the first place, I turned my eye to the
  optimizer, and completely disabled constant folding for arithmetic
  instructions.  This seemed to actually resolve the issue, and then I
  slowly enabled constant folding again on various instructions only to
  find that enabling not on the shifts, but on subtraction seemed to
  cause the bug to reappear. I am bewildered and frankly at this point
  I'm not sure I have a chance in hell of figuring out what causes it,
  so I'm throwing it here.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1877794/+subscriptions

Re: [PATCH 0/5] Introduce 'yank' oob qmp command to recover from hanging qemu

[Daniel P . Berrangé]

On Mon, May 11, 2020 at 08:12:18PM +0200, Lukas Straub wrote:
> On Mon, 11 May 2020 12:49:47 +0100
> Daniel P. Berrangé  wrote:
> 
> > On Mon, May 11, 2020 at 01:14:34PM +0200, Lukas Straub wrote:
> > > Hello Everyone,
> > > In many cases, if qemu has a network connection (qmp, migration, chardev, 
> > > etc.)
> > > to some other server and that server dies or hangs, qemu hangs too.  
> > 
> > If qemu as a whole hangs due to a stalled network connection, that is a
> > bug in QEMU that we should be fixing IMHO. QEMU should be doing non-blocking
> > I/O in general, such that if the network connection or remote server stalls,
> > we simply stop sending I/O - we shouldn't ever hang the QEMU process or main
> > loop.
> > 
> > There are places in QEMU code which are not well behaved in this respect,
> > but many are, and others are getting fixed where found to be important.
> > 
> > Arguably any place in QEMU code which can result in a hang of QEMU in the
> > event of a stalled network should be considered a security flaw, because
> > the network is untrusted in general.
> 
> The fact that out-of-band qmp commands exist at all shows that we have to 
> make tradeoffs of developer time vs. doing things right. Sure, the migration 
> code can be rewritten to use non-blocking i/o and finegrained locks. But as a 
> hobbyist I don't have time to fix this.
> 
> > > These patches introduce the new 'yank' out-of-band qmp command to recover 
> > > from
> > > these kinds of hangs. The different subsystems register callbacks which 
> > > get
> > > executed with the yank command. For example the callback can shutdown() a
> > > socket. This is intended for the colo use-case, but it can be used for 
> > > other
> > > things too of course.  
> > 
> > IIUC, invoking the "yank" command unconditionally kills every single
> > network connection in QEMU that has registered with the "yank" subsystem.
> > IMHO this is way too big of a hammer, even if we accept there are bugs in
> > QEMU not handling stalled networking well.
> > 
> > eg if a chardev hangs QEMU, and we tear down everything, killing the NBD
> > connection used for the guest disk, we needlessly break I/O.
> 
> Yeah, these patches are intended to solve the problems with the colo
> use-case where all external connections (migration, chardevs, nbd)
> are just for replication. In other use-cases you'd enable the yank
> feature only on the non-essential connections.

That is a pretty inflexible design for other use cases though,
as "non-essential" is not a black & white list in general. There
are varying levels of importance to the different channels. We
can afford to loose migration without any user visible effects.
If that doesn't solve it, a serial device chardev, or VNC connection
can be dropped at the inconvenience of loosing interactive console
which is end user visible impact, so may only be want to be yanked
if the migration yank didn't fix it. 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH 0/5] Introduce 'yank' oob qmp command to recover from hanging qemu

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berra...@redhat.com) wrote:
> On Mon, May 11, 2020 at 08:12:18PM +0200, Lukas Straub wrote:
> > On Mon, 11 May 2020 12:49:47 +0100
> > Daniel P. Berrangé  wrote:
> > 
> > > On Mon, May 11, 2020 at 01:14:34PM +0200, Lukas Straub wrote:
> > > > Hello Everyone,
> > > > In many cases, if qemu has a network connection (qmp, migration, 
> > > > chardev, etc.)
> > > > to some other server and that server dies or hangs, qemu hangs too.  
> > > 
> > > If qemu as a whole hangs due to a stalled network connection, that is a
> > > bug in QEMU that we should be fixing IMHO. QEMU should be doing 
> > > non-blocking
> > > I/O in general, such that if the network connection or remote server 
> > > stalls,
> > > we simply stop sending I/O - we shouldn't ever hang the QEMU process or 
> > > main
> > > loop.
> > > 
> > > There are places in QEMU code which are not well behaved in this respect,
> > > but many are, and others are getting fixed where found to be important.
> > > 
> > > Arguably any place in QEMU code which can result in a hang of QEMU in the
> > > event of a stalled network should be considered a security flaw, because
> > > the network is untrusted in general.
> > 
> > The fact that out-of-band qmp commands exist at all shows that we have to 
> > make tradeoffs of developer time vs. doing things right. Sure, the 
> > migration code can be rewritten to use non-blocking i/o and finegrained 
> > locks. But as a hobbyist I don't have time to fix this.
> > 
> > > > These patches introduce the new 'yank' out-of-band qmp command to 
> > > > recover from
> > > > these kinds of hangs. The different subsystems register callbacks which 
> > > > get
> > > > executed with the yank command. For example the callback can shutdown() 
> > > > a
> > > > socket. This is intended for the colo use-case, but it can be used for 
> > > > other
> > > > things too of course.  
> > > 
> > > IIUC, invoking the "yank" command unconditionally kills every single
> > > network connection in QEMU that has registered with the "yank" subsystem.
> > > IMHO this is way too big of a hammer, even if we accept there are bugs in
> > > QEMU not handling stalled networking well.
> > > 
> > > eg if a chardev hangs QEMU, and we tear down everything, killing the NBD
> > > connection used for the guest disk, we needlessly break I/O.
> > 
> > Yeah, these patches are intended to solve the problems with the colo
> > use-case where all external connections (migration, chardevs, nbd)
> > are just for replication. In other use-cases you'd enable the yank
> > feature only on the non-essential connections.
> 
> That is a pretty inflexible design for other use cases though,
> as "non-essential" is not a black & white list in general. There
> are varying levels of importance to the different channels. We
> can afford to loose migration without any user visible effects.
> If that doesn't solve it, a serial device chardev, or VNC connection
> can be dropped at the inconvenience of loosing interactive console
> which is end user visible impact, so may only be want to be yanked
> if the migration yank didn't fix it. 

In the case of COLO that's not the case though - here we explicitly want
to kill the migration to be able to ensure that we can recover - and
we're under time pressure to get the other member of the pair running
again. 

Dave

> Regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH v2 4/5] vhost: check vring address before calling unmap

[Dima Stepanov]

On Tue, May 12, 2020 at 11:26:11AM +0800, Jason Wang wrote:
> 
> On 2020/5/11 下午5:11, Dima Stepanov wrote:
> >On Mon, May 11, 2020 at 11:05:58AM +0800, Jason Wang wrote:
> >>On 2020/4/30 下午9:36, Dima Stepanov wrote:
> >>>Since disconnect can happen at any time during initialization not all
> >>>vring buffers (for instance used vring) can be intialized successfully.
> >>>If the buffer was not initialized then vhost_memory_unmap call will lead
> >>>to SIGSEGV. Add checks for the vring address value before calling unmap.
> >>>Also add assert() in the vhost_memory_unmap() routine.
> >>>
> >>>Signed-off-by: Dima Stepanov 
> >>>---
> >>>  hw/virtio/vhost.c | 27 +--
> >>>  1 file changed, 21 insertions(+), 6 deletions(-)
> >>>
> >>>diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >>>index ddbdc53..3ee50c4 100644
> >>>--- a/hw/virtio/vhost.c
> >>>+++ b/hw/virtio/vhost.c
> >>>@@ -314,6 +314,8 @@ static void vhost_memory_unmap(struct vhost_dev *dev, 
> >>>void *buffer,
> >>> hwaddr len, int is_write,
> >>> hwaddr access_len)
> >>>  {
> >>>+assert(buffer);
> >>>+
> >>>  if (!vhost_dev_has_iommu(dev)) {
> >>>  cpu_physical_memory_unmap(buffer, len, is_write, access_len);
> >>>  }
> >>>@@ -1132,12 +1134,25 @@ static void vhost_virtqueue_stop(struct vhost_dev 
> >>>*dev,
> >>>  vhost_vq_index);
> >>>  }
> >>>-vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, 
> >>>idx),
> >>>-   1, virtio_queue_get_used_size(vdev, idx));
> >>>-vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vdev, 
> >>>idx),
> >>>-   0, virtio_queue_get_avail_size(vdev, idx));
> >>>-vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, 
> >>>idx),
> >>>-   0, virtio_queue_get_desc_size(vdev, idx));
> >>>+/*
> >>>+ * Since the vhost-user disconnect can happen during initialization
> >>>+ * check if vring was initialized, before making unmap.
> >>>+ */
> >>>+if (vq->used) {
> >>>+vhost_memory_unmap(dev, vq->used,
> >>>+   virtio_queue_get_used_size(vdev, idx),
> >>>+   1, virtio_queue_get_used_size(vdev, idx));
> >>>+}
> >>>+if (vq->avail) {
> >>>+vhost_memory_unmap(dev, vq->avail,
> >>>+   virtio_queue_get_avail_size(vdev, idx),
> >>>+   0, virtio_queue_get_avail_size(vdev, idx));
> >>>+}
> >>>+if (vq->desc) {
> >>>+vhost_memory_unmap(dev, vq->desc,
> >>>+   virtio_queue_get_desc_size(vdev, idx),
> >>>+   0, virtio_queue_get_desc_size(vdev, idx));
> >>>+}
> >>
> >>Any reason not checking hdev->started instead? vhost_dev_start() will set it
> >>to true if virtqueues were correctly mapped.
> >>
> >>Thanks
> >Well i see it a little bit different:
> >  - vhost_dev_start() sets hdev->started to true before starting
> >virtqueues
> >  - vhost_virtqueue_start() maps all the memory
> >If we hit the vhost disconnect at the start of the
> >vhost_virtqueue_start(), for instance for this call:
> >   r = dev->vhost_ops->vhost_set_vring_base(dev, &state);
> >Then we will call vhost_user_blk_disconnect:
> >   vhost_user_blk_disconnect()->
> > vhost_user_blk_stop()->
> >   vhost_dev_stop()->
> > vhost_virtqueue_stop()
> >As a result we will come in this routine with the hdev->started still
> >set to true, but if used/avail/desc fields still uninitialized and set
> >to 0.
> 
> 
> I may miss something, but consider both vhost_dev_start() and
> vhost_user_blk_disconnect() were serialized in main loop. Can this really
> happen?
Yes, consider the case when we start the vhost-user-blk device:
  vhost_dev_start->
vhost_virtqueue_start
And we got a disconnect in the middle of vhost_virtqueue_start()
routine, for instance:
  1000 vq->num = state.num = virtio_queue_get_num(vdev, idx);
  1001 r = dev->vhost_ops->vhost_set_vring_num(dev, &state);
  1002 if (r) {
  1003 VHOST_OPS_DEBUG("vhost_set_vring_num failed");
  1004 return -errno;
  1005 }
  --> Here we got a disconnect <--
  1006 
  1007 state.num = virtio_queue_get_last_avail_idx(vdev, idx);
  1008 r = dev->vhost_ops->vhost_set_vring_base(dev, &state);
  1009 if (r) {
  1010 VHOST_OPS_DEBUG("vhost_set_vring_base failed");
  1011 return -errno;
  1012 }
As a result call to vhost_set_vring_base will call the disconnect
routine. The backtrace log for SIGSEGV is as follows:
  Thread 4 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
  [Switching to Thread 0x72ea9700 (LWP 183150)]
  0x74d60840 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
  (gdb) bt
  #0  0x74d60840 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
  #1  0x55

Re: [PATCH 0/5] Introduce 'yank' oob qmp command to recover from hanging qemu

[Dr. David Alan Gilbert]

* Lukas Straub (lukasstra...@web.de) wrote:
> On Mon, 11 May 2020 12:49:47 +0100
> Daniel P. Berrangé  wrote:
> 
> > On Mon, May 11, 2020 at 01:14:34PM +0200, Lukas Straub wrote:
> > > Hello Everyone,
> > > In many cases, if qemu has a network connection (qmp, migration, chardev, 
> > > etc.)
> > > to some other server and that server dies or hangs, qemu hangs too.  
> > 
> > If qemu as a whole hangs due to a stalled network connection, that is a
> > bug in QEMU that we should be fixing IMHO. QEMU should be doing non-blocking
> > I/O in general, such that if the network connection or remote server stalls,
> > we simply stop sending I/O - we shouldn't ever hang the QEMU process or main
> > loop.
> > 
> > There are places in QEMU code which are not well behaved in this respect,
> > but many are, and others are getting fixed where found to be important.
> > 
> > Arguably any place in QEMU code which can result in a hang of QEMU in the
> > event of a stalled network should be considered a security flaw, because
> > the network is untrusted in general.
> 
> The fact that out-of-band qmp commands exist at all shows that we have to 
> make tradeoffs of developer time vs. doing things right. Sure, the migration 
> code can be rewritten to use non-blocking i/o and finegrained locks. But as a 
> hobbyist I don't have time to fix this.

If it was just an hobbyist vs fulltime thing then I'd say that was a bad
way to make the decision; however the reality is that even those who are
paid to watch this code don't have the feeling we can make it fail
quickly/non-blocking - and for COLO you need to be absolutely sure you
nail every case, so you'd some how have to audit the whole lot, and keep
watching it.

(and thank you for taking your time to do this!)

Dave


> > > These patches introduce the new 'yank' out-of-band qmp command to recover 
> > > from
> > > these kinds of hangs. The different subsystems register callbacks which 
> > > get
> > > executed with the yank command. For example the callback can shutdown() a
> > > socket. This is intended for the colo use-case, but it can be used for 
> > > other
> > > things too of course.  
> > 
> > IIUC, invoking the "yank" command unconditionally kills every single
> > network connection in QEMU that has registered with the "yank" subsystem.
> > IMHO this is way too big of a hammer, even if we accept there are bugs in
> > QEMU not handling stalled networking well.
> > 
> > eg if a chardev hangs QEMU, and we tear down everything, killing the NBD
> > connection used for the guest disk, we needlessly break I/O.
> 
> Yeah, these patches are intended to solve the problems with the colo use-case 
> where all external connections (migration, chardevs, nbd) are just for 
> replication. In other use-cases you'd enable the yank feature only on the 
> non-essential connections.
> 
> > eg doing this in the chardev backend is not desirable, because the bugs
> > with hanging QEMU are typically caused by the way the frontend device
> > uses the chardev blocking I/O calls, instead of non-blocking I/O calls.
> > 
> > 
> > Regards,
> > Daniel
> 


--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: Qemu, VNC and non-US keymaps

[Daniel P . Berrangé]

On Tue, May 12, 2020 at 09:45:20AM +0200, B3r3n wrote:
> Hello Daniel, all,
> 
> I am a bit confused.
> 
> Ok, RFB protocol should be the solution that solves all, sending scancodes
> rather than doing keysyms stuff. No pb for me.
> So I removed my '-k fr' to my Qemu VM start as it was before.
> 
> However, reading TightVNC & noVNC docs, both are able to perform RFB.

VNC == RFB - they're two different terms of the same thing.

The core RFB/VNC protocol only knows about keysyms.

Hardware scancodes is an extension defined by QEMU, and GTK-VNC, and since
implemented by TigerVNC.

Removing the "-k" arg, merely enables use of the scancode extension.
This requires a compatible client app that knows the scancode extension.
If the client doesn't support scancodes, it will continue using keysyms,
which will then be treated as plain us keymap.

AFAIK,  TightVNC doesn't support the scancode extension, only TigerVNC.

> Since these explanations, I replayed a bit:
> 
> Under my testing Debian 10, I redefined keyboard to French + No compose key
> + AltGr as CTRL_R

This is a key example of the problems of VNC's traditional key handling.

QEMU has a single keymap "fr". It has no way of selecting compose key
on/off, or overriding AltGr to be CtrlR.  So as soon as you do that on
your local desktop, you make it impossible to QEMU VNC serve to work
correctly.

> 
> Under noVNC: Ctrl_R works well as alternative but when using AltGr, I
> received 29+100+7 (AltGr + 6) and keep displaying a minus as with AltGr was
> not pressed.
> 
> Under TightVNC (2.7.10) : Ctrl_R displays characters, I am still in QWERTY
> for letters, weird mapping for other characters, did not checked if
> compliant with whatever definition.
> Under TightVNC (last 2.8.27, supposed to be able to RFB): Ctrl_R displays
> nothing, keys are QWERTY. Seems the same as TightVNC 2.7.10.
> 
> With the keyboard defining AltGr as AltGr, no change.
> 
> I realize that AltGr is sending 29+100 (seen via showkey), when CTRL_R only
> sends 97.
> When using a remote console (iLo and iDRAC), AltGr only sends 100.
> 
> I wonder if the issue would not also be the fact AltGr sends 2 codes, still
> another one to select the character key (6 for example).
> 
> Is that normal Qemu is transforming AltGr (100) in 29+100 ?

It is hard to say without seeing debuging to see what QEMU received.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v2 5/5] vhost: add device started check in migration set log

[Dima Stepanov]

On Tue, May 12, 2020 at 11:47:34AM +0800, Li Feng wrote:
> Hi, Dima.
> 
> If vhost_migration_log return < 0, then vhost_log_global_start will
> trigger a crash.
> Does your patch have process this abort?
> If a disconnect happens in the migration stage, the correct operation
> is to stop the migration, right?
> 
>  841 static void vhost_log_global_start(MemoryListener *listener)
>  842 {
>  843 int r;
>  844
>  845 r = vhost_migration_log(listener, true);
>  846 if (r < 0) {
>  847 abort();
>  848 }
>  849 }
Yes, my patch process it by not returning an error ). That is one of the
point we've talked about with Raphael and Michael in this thread. First
of all in my patches i'm still following the same logic which has been
already in upstream ./hw/virtio/vhost.c:vhost_migration_log():
  ...
  820 if (!dev->started) {
  821 dev->log_enabled = enable;
  822 return 0;
  823 }
  ...
It means, that if device not started, then continue migration without
returning any error. So i followed the same logic, if we got a
disconnect, then it will mean that device isn't started and we can
continue migration. As a result no error is returned and assert() isn't
hit.
Also there is a question from Raphael to Michael about it you can find
it in this thread, by i will add it also:
  > Subject: Re: [PATCH v2 5/5] vhost: add device started check in
  > migration set log

  > On Wed, May 06, 2020 at 06:08:34PM -0400, Raphael Norwitz wrote:
  >> In particular, we need to decide whether a migration should be
  >> allowed to continue if a device disconnects durning the migration
  >> stage.
  >>
  >> mst, any thoughts?

  > Why not? It can't change state while disconnected, so it just makes
  > things easier.

So it looks like a correct way to handle it. Also our internal tests
passed. Some words about our tests:
  - run src VM with vhost-usr-blk daemon used
  - run fio inside it
  - perform reconnect every X seconds (just kill and restart
daemon), X is random
  - run dst VM
  - perform migration
  - fio should complete in dst VM
And we cycle this test like forever. At least for now we see no new
issues.

No other comments mixed in below.

> 
> Thanks,
> 
> Feng Li
> 
> Jason Wang  于2020年5月12日周二 上午11:33写道：
> >
> >
> > On 2020/5/11 下午5:25, Dima Stepanov wrote:
> > > On Mon, May 11, 2020 at 11:15:53AM +0800, Jason Wang wrote:
> > >> On 2020/4/30 下午9:36, Dima Stepanov wrote:
> > >>> If vhost-user daemon is used as a backend for the vhost device, then we
> > >>> should consider a possibility of disconnect at any moment. If such
> > >>> disconnect happened in the vhost_migration_log() routine the vhost
> > >>> device structure will be clean up.
> > >>> At the start of the vhost_migration_log() function there is a check:
> > >>>if (!dev->started) {
> > >>>dev->log_enabled = enable;
> > >>>return 0;
> > >>>}
> > >>> To be consistent with this check add the same check after calling the
> > >>> vhost_dev_set_log() routine. This in general help not to break a
> > >>> migration due the assert() message. But it looks like that this code
> > >>> should be revised to handle these errors more carefully.
> > >>>
> > >>> In case of vhost-user device backend the fail paths should consider the
> > >>> state of the device. In this case we should skip some function calls
> > >>> during rollback on the error paths, so not to get the NULL dereference
> > >>> errors.
> > >>>
> > >>> Signed-off-by: Dima Stepanov 
> > >>> ---
> > >>>   hw/virtio/vhost.c | 39 +++
> > >>>   1 file changed, 35 insertions(+), 4 deletions(-)
> > >>>
> > >>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > >>> index 3ee50c4..d5ab96d 100644
> > >>> --- a/hw/virtio/vhost.c
> > >>> +++ b/hw/virtio/vhost.c
> > >>> @@ -787,6 +787,17 @@ static int vhost_dev_set_features(struct vhost_dev 
> > >>> *dev,
> > >>>   static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
> > >>>   {
> > >>>   int r, i, idx;
> > >>> +
> > >>> +if (!dev->started) {
> > >>> +/*
> > >>> + * If vhost-user daemon is used as a backend for the
> > >>> + * device and the connection is broken, then the vhost_dev
> > >>> + * structure will be reset all its values to 0.
> > >>> + * Add additional check for the device state.
> > >>> + */
> > >>> +return -1;
> > >>> +}
> > >>> +
> > >>>   r = vhost_dev_set_features(dev, enable_log);
> > >>>   if (r < 0) {
> > >>>   goto err_features;
> > >>> @@ -801,12 +812,19 @@ static int vhost_dev_set_log(struct vhost_dev 
> > >>> *dev, bool enable_log)
> > >>>   }
> > >>>   return 0;
> > >>>   err_vq:
> > >>> -for (; i >= 0; --i) {
> > >>> +/*
> > >>> + * Disconnect with the vhost-user daemon can lead to the
> > >>> + * vhost_dev_cleanup() call which will clean up vhost_dev
> > >>> + * structure.
> > >>> + */
> > >>> +for (; dev->started &&

Re: how to extend to load COFF executable image file

[Peter Maydell]

On Tue, 12 May 2020 at 08:41, xiaolei <1482995...@qq.com> wrote:
>
> Hi all,
>   I attempt to add DSP architecture support for some TI processor, based on 
> QEMU 4.2.

Don't try to add new code to QEMU based on an old version.
You should always work with the head-of-git. Otherwise you'll
be dealing with stuff that's gradually drifting out of date
and you'll end up with pain when you need to rebase.

>   When I work on the executable file loading , I try to load COFF executable  
> file. Following the ELF file processing scheme, I thought I could write a 
> function similar to :
>rom_add_elf_program(label, mapped_file, data, file_size, mem_size, 
> addr, as);
>   But I got lost when I track down the usage to the global variable  :static 
> QTAILQ_HEAD(, Rom) roms;
>   I did not get where this 'roms'  is used for program loading, and how the 
> loaded program get to run eventually.  Can someone give me some hints?

You've gone down too far into the internals of the implementation there.
The way that QEMU's image loading code works is that all the data
that must be loaded is put into a set of "ROM blobs", which we
keep in a linked list. When the system is reset then the function
rom_reset() runs through the list and copies the data in each
blob into the right location in the guest memory. The simplest
way to create a blob is to use rom_add_blob(); but you could
also use rom_add_elf_program() if you wanted. That function is
a bit misnamed: it's not ELF specific and it doesn't handle
an entire program; it just has a couple of extra properties over
rom_add_blob:
 * it lets the caller provide data which only partly fills
   the blob, with the remainder to be zeroes
 * it can work with a GMappedFile so you don't need to copy
   the data out of the mapped file to pass to it

>   Also, the COFF file format differs from the ELF, there is no program 
> header. I wonder if I could reuse the 'rom' structure like loading a ELF. Or 
> there is a better way to do it.

Yes, you want to reuse the Rom struct, which isn't ELF specific
(we also use it for loading raw files and uImages). You would
need to write code which could read and parse a COFF file and
identify what parts of the input file needed to go where in
memory (and what parts of memory would need to be zeroed). Then
for each of those parts of memory you create a rom blob
(via rom_add_blob() or rom_add_elf_program()). That is, you
want the equivalent of the function "static int glue(load_elf, SZ)"
in include/hw/elf_ops.h. (That function is a bit odd because
we use the C preprocessor to create a 32 bit and a 64 bit
version of the function so we can load 32-bit ELF and 64-bit
ELF files. COFF may not need that complication.)

Consider also taking the simpler approach of just using
binutils objcopy to convert the COFF file into an ELF
and then passing that ELF file to QEMU. In particular
if you're also trying to implement an entire new architecture
you might as well skip the pain of writing a COFF file
loader for the moment.

thanks
-- PMM

Re: [PATCH 0/5] Introduce 'yank' oob qmp command to recover from hanging qemu

[Lukas Straub]

On Mon, 11 May 2020 16:46:45 +0100
"Dr. David Alan Gilbert"  wrote:

> * Daniel P. Berrangé (berra...@redhat.com) wrote: 
> > ...
> > That way if QEMU does get stuck, you can start by tearing down the
> > least distruptive channel. eg try tearing down the migration connection
> > first (which shouldn't negatively impact the guest), and only if that
> > doesn't work then, move on to tear down the NBD connection (which risks
> > data loss)  
> 
> I wonder if a different way would be to make all network connections
> register with yank, but then make yank take a list of connections to
> shutdown(2).

Good Idea. We could name the connections (/yank callbacks) in the form 
"nbd:", "chardev:" and "migration" (and add 
"netdev:...", etc. in the future). Then make yank take a list of connection 
names as you suggest and silently ignore connections that don't exist. And 
maybe even add a 'query-yank' oob command returning a list of registered 
connections so the management application can do pattern matching if it wants.

Comments?

Regards,
Lukas Straub

> Dave
> 
> > Regards,
> > Daniel
> > -- 
> > |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange 
> > :|
> > |: https://libvirt.org -o-https://fstop138.berrange.com 
> > :|
> > |: https://entangle-photo.org-o-https://www.instagram.com/dberrange 
> > :|  
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> 



pgppwyx0rTKxj.pgp
Description: OpenPGP digital signature

Re: [PATCH v2 5/5] vhost: add device started check in migration set log

[Dima Stepanov]

On Tue, May 12, 2020 at 11:32:50AM +0800, Jason Wang wrote:
> 
> On 2020/5/11 下午5:25, Dima Stepanov wrote:
> >On Mon, May 11, 2020 at 11:15:53AM +0800, Jason Wang wrote:
> >>On 2020/4/30 下午9:36, Dima Stepanov wrote:
> >>>If vhost-user daemon is used as a backend for the vhost device, then we
> >>>should consider a possibility of disconnect at any moment. If such
> >>>disconnect happened in the vhost_migration_log() routine the vhost
> >>>device structure will be clean up.
> >>>At the start of the vhost_migration_log() function there is a check:
> >>>   if (!dev->started) {
> >>>   dev->log_enabled = enable;
> >>>   return 0;
> >>>   }
> >>>To be consistent with this check add the same check after calling the
> >>>vhost_dev_set_log() routine. This in general help not to break a
> >>>migration due the assert() message. But it looks like that this code
> >>>should be revised to handle these errors more carefully.
> >>>
> >>>In case of vhost-user device backend the fail paths should consider the
> >>>state of the device. In this case we should skip some function calls
> >>>during rollback on the error paths, so not to get the NULL dereference
> >>>errors.
> >>>
> >>>Signed-off-by: Dima Stepanov 
> >>>---
> >>>  hw/virtio/vhost.c | 39 +++
> >>>  1 file changed, 35 insertions(+), 4 deletions(-)
> >>>
> >>>diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >>>index 3ee50c4..d5ab96d 100644
> >>>--- a/hw/virtio/vhost.c
> >>>+++ b/hw/virtio/vhost.c
> >>>@@ -787,6 +787,17 @@ static int vhost_dev_set_features(struct vhost_dev 
> >>>*dev,
> >>>  static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
> >>>  {
> >>>  int r, i, idx;
> >>>+
> >>>+if (!dev->started) {
> >>>+/*
> >>>+ * If vhost-user daemon is used as a backend for the
> >>>+ * device and the connection is broken, then the vhost_dev
> >>>+ * structure will be reset all its values to 0.
> >>>+ * Add additional check for the device state.
> >>>+ */
> >>>+return -1;
> >>>+}
> >>>+
> >>>  r = vhost_dev_set_features(dev, enable_log);
> >>>  if (r < 0) {
> >>>  goto err_features;
> >>>@@ -801,12 +812,19 @@ static int vhost_dev_set_log(struct vhost_dev *dev, 
> >>>bool enable_log)
> >>>  }
> >>>  return 0;
> >>>  err_vq:
> >>>-for (; i >= 0; --i) {
> >>>+/*
> >>>+ * Disconnect with the vhost-user daemon can lead to the
> >>>+ * vhost_dev_cleanup() call which will clean up vhost_dev
> >>>+ * structure.
> >>>+ */
> >>>+for (; dev->started && (i >= 0); --i) {
> >>>  idx = dev->vhost_ops->vhost_get_vq_index(
> >>
> >>Why need the check of dev->started here, can started be modified outside
> >>mainloop? If yes, I don't get the check of !dev->started in the beginning of
> >>this function.
> >>
> >No dev->started can't change outside the mainloop. The main problem is
> >only for the vhost_user_blk daemon. Consider the case when we
> >successfully pass the dev->started check at the beginning of the
> >function, but after it we hit the disconnect on the next call on the
> >second or third iteration:
> >  r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx, enable_log);
> >The unix socket backend device will call the disconnect routine for this
> >device and reset the structure. So the structure will be reset (and
> >dev->started set to false) inside this set_addr() call.
> 
> 
> I still don't get here. I think the disconnect can not happen in the middle
> of vhost_dev_set_log() since both of them were running in mainloop. And even
> if it can, we probably need other synchronization mechanism other than
> simple check here.
Disconnect isn't happened in the separate thread it is happened in this
routine inside vhost_dev_set_log. When for instance vhost_user_write()
call failed:
  vhost_user_set_log_base()
vhost_user_write()
  vhost_user_blk_disconnect()
vhost_dev_cleanup()
  vhost_user_backend_cleanup()
So the point is that if we somehow got a disconnect with the
vhost-user-blk daemon before the vhost_user_write() call then it will
continue clean up by running vhost_user_blk_disconnect() function. I
wrote a more detailed backtrace stack in the separate thread, which is
pretty similar to what we have here:
  Re: [PATCH v2 4/5] vhost: check vring address before calling unmap
The places are different but the problem is pretty similar.

So if vhost-user commands handshake then everything is fine and
reconnect will work as expected. The only problem is how to handle
reconnect properly between vhost-user command send/receive.

As i wrote we have a test:
  - run src VM with vhost-usr-blk daemon used
  - run fio inside it
  - perform reconnect every X seconds (just kill and restart daemon),
X is random
  - run dst VM
  - perform migration
  - fio should complete in dst VM
And we cycle this test like forever.
So it fails once per ~25 iteration. By adding some delays in

Re: SBSA-REF maintainer email bouncing

[Leif Lindholm]

On Tue, May 12, 2020 at 08:34:13 +0200, Philippe Mathieu-Daudé wrote:
> Hello,
> 
> Radoslaw Biernacki is listed as maintainer for the SBSA-REF board.
> 
> His radoslaw.bierna...@linaro.org email address no longer works,
> apparently "Radoslaw Biernacki no longer works for Linaro".

That was probably technically the case already when the patches went
in, but Radek was given an "associate account" for continuity.

I'm digging into what happened (although as _I_ don't work for Linaro
anymore, that might take a bit longer than it otherwise would have).

/
Leif

>   SBSA-REF
>   M: Radoslaw Biernacki 
>   M: Peter Maydell 
>   R: Leif Lindholm 
>   L: qemu-...@nongnu.org
>   S: Maintained
>   F: hw/arm/sbsa-ref.c
> 

Re: [PATCH 0/2] 9pfs: regression init_in_iov_from_pdu truncating size

[Greg Kurz]

On Sun, 10 May 2020 19:41:52 +0200
Christian Schoenebeck  wrote:

> Stefano, looks like your original patch needs some more fine tuning:
> 
> https://bugs.launchpad.net/bugs/1877688
> 
> Please check if the assumptions I made about Xen are correct, and please
> also test whether these changes still work for you with Xen as intended by
> you.
> 
> Christian Schoenebeck (2):
>   xen-9pfs: Fix log messages of reply errors
>   9pfs: fix init_in_iov_from_pdu truncating size
> 
>  hw/9pfs/virtio-9p-device.c | 35 
>  hw/9pfs/xen-9p-backend.c   | 41 --
>  2 files changed, 58 insertions(+), 18 deletions(-)
> 

Sorry, I'm off this week, not sure I'll have time to review.
So I've only applied patch 1 for now and I'll let Stefano
and you sort out what should be done for patch 2.

Cheers,

--
Greg

Re: [PATCH v2 1/5] qemu-img: remove check that cvtnum value > MAX_INT

[Eyal Moscovici]

On 07/05/2020 0:49, Eric Blake wrote:

On 5/6/20 4:34 PM, Eyal Moscovici wrote:
Following commit f46bfdbfc8f95cf65d7818ef68a801e063c40332 
(util/cutils: Change

qemu_strtosz*() from int64_t to uint64_t) which added a similar check to
cvtnum. As a result there is no need to check it separately outside 
of cvtnum.


Acked-by: Mark Kanda 
Signed-off-by: Eyal Moscovici 
---
  qemu-img.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 6a4327aaba..116a9c6349 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4307,7 +4307,7 @@ static int img_bench(int argc, char **argv)
  int64_t sval;
    sval = cvtnum(optarg);
-    if (sval < 0 || sval > INT_MAX) {
+    if (sval < 0) {
  error_report("Invalid buffer size specified");


INT_MAX is smaller than cvtnum's check for INT64_MAX.  This code 
change allows larger buffer sizes, which is probably not a good idea.
I was the most hesitant about this patch because of the size difference. 
I decided to submit it because the type is int64 which pairs better with 
the MAX_INT64 check and I couldn't find a concrete reason to cap the 
variable at MAX_INT. Do you a concrete reason? Because the max size 
should rerally come into effect on very fringe cases and if you are 
asking for a really big buffer you should know the risks.



  return 1;
  }
@@ -4320,7 +4320,7 @@ static int img_bench(int argc, char **argv)
  int64_t sval;
    sval = cvtnum(optarg);
-    if (sval < 0 || sval > INT_MAX) {
+    if (sval < 0) {
  error_report("Invalid step size specified");
  return 1;
  }
@@ -4493,7 +4493,7 @@ static int img_dd_bs(const char *arg,
    res = cvtnum(arg);
  -    if (res <= 0 || res > INT_MAX) {
+    if (res <= 0) {
  error_report("invalid number: '%s'", arg);
  return 1;
  }



NACK.

[RFC v3 for Linux] virtio_balloon: Add VIRTIO_BALLOON_VQ_INFLATE_CONT to handle THP split issue

[Hui Zhu]

The first and second version are in [1] and [2].
According to the comments from Michael, I updated the patch.
1. Removed the separate vq inflate_cont_vq and just use inflate_vq to
   transport inflate pages.
2. Add two max_pages_order and current_pages_order to virtio_balloon_config
   instead of pages_order.
   max_pages_order is set by QEMU.  It is the max order of the inflate
   pages.
   current_pages_order is set by kernel.  It is the current order of
   the inflate pages.
   When balloon inflate begin, current_pages_order is set to
   max_pages_order.
   Kernel tries to allocate current_pages_order page.  If allocation fails,
   current_pages_order will be reduced by 1 until it is 0.
   When QEMU get pfn from inflate_vq, it will release with size in
   current_pages_order.

Following is the introduction of the function.
If the guest kernel has many fragmentation pages, use virtio_balloon
will split THP of QEMU when it calls MADV_DONTNEED madvise to release
the balloon pages.
This is an example in a VM with 1G memory 1CPU:
// This is the THP number before VM execution in the host.
// None use THP.
cat /proc/meminfo | grep AnonHugePages:
AnonHugePages: 0 kB

// After VM start, use usemem
// (https://git.kernel.org/pub/scm/linux/kernel/git/wfg/vm-scalability.git)
// punch-holes function generates 400m fragmentation pages in the guest
// kernel.
usemem --punch-holes -s -1 800m &

// This is the THP number after this command in the host.
// Some THP is used by VM because usemem will access 800M memory
// in the guest.
cat /proc/meminfo | grep AnonHugePages:
AnonHugePages:978944 kB

// Connect to the QEMU monitor, setup balloon, and set it size to 600M.
(qemu) device_add virtio-balloon-pci,id=balloon1
(qemu) info balloon
balloon: actual=1024
(qemu) balloon 600
(qemu) info balloon
balloon: actual=600

// This is the THP number after inflate the balloon in the host.
cat /proc/meminfo | grep AnonHugePages:
AnonHugePages:153600 kB

// Set the size back to 1024M in the QEMU monitor.
(qemu) balloon 1024
(qemu) info balloon
balloon: actual=1024

// Use usemem to increase the memory usage of QEMU.
killall usemem
usemem 800m

// This is the THP number after this operation.
cat /proc/meminfo | grep AnonHugePages:
AnonHugePages:153600 kB

THP number decreased more than 800M after inflate the balloon in the host.
The reason is usemem with punch-holes option will free every other
page after allocation.  Then 400M free memory inside the guest kernel
is fragmentation pages.
The guest kernel will use them to inflate the balloon.  When these
fragmentation pages are freed, THP will be split.
THP number is not increased after deflate the balloon and increase
memory useage because fragmentation address affect the THP allcation
in the host.

This commit tries to handle this with add a new flag
VIRTIO_BALLOON_VQ_INFLATE_CONT.
When this flag is set, the balloon will try to use continuous pages
inflate the balloon.  And the pages default order is set to THP order.
Then THP pages will be freed together in the host.
This is an example in a VM with 1G memory 1CPU:
// This is the THP number before VM execution in the host.
// None use THP.
cat /proc/meminfo | grep AnonHugePages:
AnonHugePages: 0 kB

// After VM start, use usemem punch-holes function generates 400M
// fragmentation pages in the guest kernel.
usemem --punch-holes -s -1 800m &

// This is the THP number after this command in the host.
// Some THP is used by VM because usemem will access 800M memory
// in the guest.
cat /proc/meminfo | grep AnonHugePages:
AnonHugePages:978944 kB

// Connect to the QEMU monitor, setup balloon, and set it size to 600M.
(qemu) device_add virtio-balloon-pci,id=balloon1,cont-pages=on
(qemu) info balloon
balloon: actual=1024
(qemu) balloon 600
(qemu) info balloon
balloon: actual=600

// This is the THP number after inflate the balloon in the host.
cat /proc/meminfo | grep AnonHugePages:
AnonHugePages:612352 kB

// Set the size back to 1024M in the QEMU monitor.
(qemu) balloon 1024
(qemu) info balloon
balloon: actual=1024

// Use usemem to increase the memory usage of QEMU.
killall usemem
usemem 800m

// This is the THP number after this operation.
cat /proc/meminfo | grep AnonHugePages:
AnonHugePages:944128 kB

The THP number decreases 358M.  This shows that
VIRTIO_BALLOON_VQ_INFLATE_CONT can help handle the THP split issue.
The THP number is increased after deflate the balloon and increase
memory useage.

[1] https://lkml.org/lkml/2020/3/12/144
[2] 
https://lore.kernel.org/linux-mm/1584893097-12317-1-git-send-email-teawa...@gmail.com/

Signed-off-by: Hui Zhu 
---
 drivers/virtio/virtio_balloon.c | 98 +++--
 include/linux/balloon_compaction.h  |  9 +++-
 include/uapi/linux/virtio_balloon.h |  5 ++
 mm/balloon_compaction.c | 40 ---
 4 files changed, 129 insertions(+), 23 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/

[RFC v3 for QEMU] virtio-balloon: Add option cont-pages to set VIRTIO_BALLOON_VQ_INFLATE_CONT

[Hui Zhu]

If the guest kernel has many fragmentation pages, use virtio_balloon
will split THP of QEMU when it calls MADV_DONTNEED madvise to release
the balloon pages.
Set option cont-pages to on will open flags VIRTIO_BALLOON_VQ_INFLATE_CONT
and set default continuous pages order to THP order.
Then It will get continuous pages PFN that its order is current_pages_order
from VQ ivq use use madvise MADV_DONTNEED release the page.
This will handle the THP split issue.

Signed-off-by: Hui Zhu 
---
 hw/virtio/virtio-balloon.c  | 77 +
 include/hw/virtio/virtio-balloon.h  |  2 +
 include/standard-headers/linux/virtio_balloon.h |  5 ++
 3 files changed, 60 insertions(+), 24 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a4729f7..84d47d3 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -34,6 +34,7 @@
 #include "hw/virtio/virtio-access.h"
 
 #define BALLOON_PAGE_SIZE  (1 << VIRTIO_BALLOON_PFN_SHIFT)
+#define CONT_PAGES_ORDER   9
 
 typedef struct PartiallyBalloonedPage {
 ram_addr_t base_gpa;
@@ -72,6 +73,8 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
 RAMBlock *rb;
 size_t rb_page_size;
 int subpages;
+size_t inflate_size = BALLOON_PAGE_SIZE << balloon->current_pages_order;
+int pages_num;
 
 /* XXX is there a better way to get to the RAMBlock than via a
  * host address? */
@@ -81,7 +84,7 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
 if (rb_page_size == BALLOON_PAGE_SIZE) {
 /* Easy case */
 
-ram_block_discard_range(rb, rb_offset, rb_page_size);
+ram_block_discard_range(rb, rb_offset, inflate_size);
 /* We ignore errors from ram_block_discard_range(), because it
  * has already reported them, and failing to discard a balloon
  * page is not fatal */
@@ -99,32 +102,38 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
 
 rb_aligned_offset = QEMU_ALIGN_DOWN(rb_offset, rb_page_size);
 subpages = rb_page_size / BALLOON_PAGE_SIZE;
-base_gpa = memory_region_get_ram_addr(mr) + mr_offset -
-   (rb_offset - rb_aligned_offset);
 
-if (pbp->bitmap && !virtio_balloon_pbp_matches(pbp, base_gpa)) {
-/* We've partially ballooned part of a host page, but now
- * we're trying to balloon part of a different one.  Too hard,
- * give up on the old partial page */
-virtio_balloon_pbp_free(pbp);
-}
+for (pages_num = inflate_size / BALLOON_PAGE_SIZE;
+ pages_num > 0; pages_num--) {
+base_gpa = memory_region_get_ram_addr(mr) + mr_offset -
+   (rb_offset - rb_aligned_offset);
 
-if (!pbp->bitmap) {
-virtio_balloon_pbp_alloc(pbp, base_gpa, subpages);
-}
+if (pbp->bitmap && !virtio_balloon_pbp_matches(pbp, base_gpa)) {
+/* We've partially ballooned part of a host page, but now
+* we're trying to balloon part of a different one.  Too hard,
+* give up on the old partial page */
+virtio_balloon_pbp_free(pbp);
+}
 
-set_bit((rb_offset - rb_aligned_offset) / BALLOON_PAGE_SIZE,
-pbp->bitmap);
+if (!pbp->bitmap) {
+virtio_balloon_pbp_alloc(pbp, base_gpa, subpages);
+}
 
-if (bitmap_full(pbp->bitmap, subpages)) {
-/* We've accumulated a full host page, we can actually discard
- * it now */
+set_bit((rb_offset - rb_aligned_offset) / BALLOON_PAGE_SIZE,
+pbp->bitmap);
 
-ram_block_discard_range(rb, rb_aligned_offset, rb_page_size);
-/* We ignore errors from ram_block_discard_range(), because it
- * has already reported them, and failing to discard a balloon
- * page is not fatal */
-virtio_balloon_pbp_free(pbp);
+if (bitmap_full(pbp->bitmap, subpages)) {
+/* We've accumulated a full host page, we can actually discard
+* it now */
+
+ram_block_discard_range(rb, rb_aligned_offset, rb_page_size);
+/* We ignore errors from ram_block_discard_range(), because it
+* has already reported them, and failing to discard a balloon
+* page is not fatal */
+virtio_balloon_pbp_free(pbp);
+}
+
+mr_offset += BALLOON_PAGE_SIZE;
 }
 }
 
@@ -345,7 +354,7 @@ static void virtio_balloon_handle_output(VirtIODevice 
*vdev, VirtQueue *vq)
 offset += 4;
 
 section = memory_region_find(get_system_memory(), pa,
- BALLOON_PAGE_SIZE);
+BALLOON_PAGE_SIZE << s->current_pages_order);
 if (!section.mr) {
 trace_virtio_balloon_bad_addr(pa);
 continue;
@@ -618,9 +627,12 @@ static size_t virtio_balloon_config_size(VirtIOBalloon *s)
 if (s->qemu_4_0_config_size) {
 return sizeof(struct virtio_ba

Re: [PATCH 0/5] Introduce 'yank' oob qmp command to recover from hanging qemu

[Daniel P . Berrangé]

On Tue, May 12, 2020 at 11:32:06AM +0200, Lukas Straub wrote:
> On Mon, 11 May 2020 16:46:45 +0100
> "Dr. David Alan Gilbert"  wrote:
> 
> > * Daniel P. Berrangé (berra...@redhat.com) wrote: 
> > > ...
> > > That way if QEMU does get stuck, you can start by tearing down the
> > > least distruptive channel. eg try tearing down the migration connection
> > > first (which shouldn't negatively impact the guest), and only if that
> > > doesn't work then, move on to tear down the NBD connection (which risks
> > > data loss)  
> > 
> > I wonder if a different way would be to make all network connections
> > register with yank, but then make yank take a list of connections to
> > shutdown(2).
> 
> Good Idea. We could name the connections (/yank callbacks) in the
> form "nbd:", "chardev:" and "migration"
> (and add "netdev:...", etc. in the future). Then make yank take a
> list of connection names as you suggest and silently ignore connections
> that don't exist. And maybe even add a 'query-yank' oob command returning
> a list of registered connections so the management application can do
> pattern matching if it wants.

Yes, that would make the yank command much more flexible in how it can
be used.

As an alternative to using formatted strings like this, it could be
modelled more explicitly in QAPI

  { 'struct':  'YankChannels',
'data': { 'chardev': [ 'string' ],
  'nbd': ['string'],
  'migration': bool } }

In this example, 'chardev' would accept a list of chardev IDs which
have it enabled, 'nbd' would accept a list of block node IDs which
have it enabled, and migration is a singleton on/off.

The benefit of this modelling is that you can introspect QEMU
to discover what classes of channels support being yanked by
this QEMU build, as well as what instances are configured to
be yanked. ie you can distinguish between a QEMU that doesn't
support yanking network devices, from a QEMU that does support
yanking network devices, but doesn't have it enabled for any
network device instances.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v2 2/5] qemu_img: add error report to cvtnum

[Eyal Moscovici]

On 07/05/2020 0:59, Eric Blake wrote:

On 5/6/20 4:34 PM, Eyal Moscovici wrote:
All calls to cvtnum check the return value and print the same error 
message more
or less. And so error reporting moved to cvtnum to reduce duplicate 
code and

provide a single error message.

Acked-by: Mark Kanda 
Signed-off-by: Eyal Moscovici 
---
  qemu-img.c | 63 --
  tests/qemu-iotests/049.out |  4 +--
  2 files changed, 28 insertions(+), 39 deletions(-)





-    err = qemu_strtosz(s, NULL, &value);
-    if (err < 0) {
+    err = qemu_strtosz(arg_value, NULL, &value);
+    if (err < 0 && err != -ERANGE) {
+    error_report("Invalid %s specified! You may use "
+ "k, M, G, T, P or E suffixes for ", arg_name);
+    error_report("kilobytes, megabytes, gigabytes, terabytes, "
+ "petabytes and exabytes.");
 return err;
 }
-    if (value > INT64_MAX) {
+    if (err == -ERANGE || value > INT64_MAX) {
+    error_report("Invalid %s specified! Must be less than 8 EiB!",


Copied from our pre-existing errors, but why are we shouting at our 
user?  This would be a good time to s/!/./ to tone it down a bit.

Sure, will fix.



@@ -4491,10 +4488,12 @@ static int img_dd_bs(const char *arg,
  {
  int64_t res;
  -    res = cvtnum(arg);
+    res = cvtnum("bs", arg);
  -    if (res <= 0) {
-    error_report("invalid number: '%s'", arg);
+    if (res < 0) {
+    return 1;
+    } else if (res == 0) {
+    error_report("Invalid bs specified! It cannot be 0.");


Maybe it's worth two functions:

int64_t cvtnum_full(const char *name, const char *value, int64_t min, 
int64_t max)


and then a common helper:

int64_t cvtnum(const char *name, const char *value) {
    return cvtnum_full(name, value, 0, INT64_MAX);
}

many existing callers remain with cvtnum(), but callers like this 
could be cvtnum("bs", arg, 1, INT64_MAX).  You'd still have to 
special-case other restrictions, such as whether a number must a 
power-of-2, but that's fewer places.



Great idea, I will create two functions.

+++ b/tests/qemu-iotests/049.out
@@ -92,13 +92,13 @@ Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 
size=1649267441664 cluster_size=65536 l

  == 3. Invalid sizes ==
    qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- -1024
-qemu-img: Image size must be less than 8 EiB!
+qemu-img: Invalid image size specified! Must be less than 8 EiB!


Nice that you checked for iotest fallout.  Is this really the only 
impacted test?



Thanks, yes.

Re: [PATCH v2 5/5] qemu-img: Add --start-offset and --max-length to map

[Eyal Moscovici]

On 07/05/2020 1:04, Eric Blake wrote:

On 5/6/20 4:34 PM, Eyal Moscovici wrote:

The mapping operation of large disks especially ones stored over a
long chain of QCOW2 files can take a long time to finish.
Additionally when mapping fails there was no way recover by
restarting the mapping from the failed location.

The new options, --start-offset and --max-length allows the user to
divide these type of map operations into shorter independent tasks.

Reviewed-by: Eric Blake 


This patch has some changes from v1.  Among others,...


@@ -3041,6 +3045,18 @@ static int img_map(int argc, char **argv)
  case OPTION_OUTPUT:
  output = optarg;
  break;
+    case 's':
+    start_offset = cvtnum("start offset", optarg);
+    if (start_offset < 0) {
+    return 1;
+    }
+    break;


the new semantics of cvtnum() in this series is enough of a difference 
that I would have removed R-b to make sure the updated patch gets 
re-reviewed, if it had been me as author.  But in this case, it does 
look like the changes are all addressed to comments I suggested in v1, 
so I'm fine that you left my R-b.



Ok, got it.

[PATCH v9] audio/jack: add JACK client audiodev

[Geoffrey McRae]

This commit adds a new audiodev backend to allow QEMU to use JACK as
both an audio sink and source.

Signed-off-by: Geoffrey McRae 
---
 audio/Makefile.objs|   5 +
 audio/audio.c  |   1 +
 audio/audio_template.h |   2 +
 audio/jackaudio.c  | 667 +
 configure  |  17 ++
 qapi/audio.json|  56 +++-
 6 files changed, 746 insertions(+), 2 deletions(-)
 create mode 100644 audio/jackaudio.c

diff --git a/audio/Makefile.objs b/audio/Makefile.objs
index d7490a379f..b4a4c11f31 100644
--- a/audio/Makefile.objs
+++ b/audio/Makefile.objs
@@ -28,3 +28,8 @@ common-obj-$(CONFIG_AUDIO_SDL) += sdl.mo
 sdl.mo-objs = sdlaudio.o
 sdl.mo-cflags := $(SDL_CFLAGS)
 sdl.mo-libs := $(SDL_LIBS)
+
+# jack module
+common-obj-$(CONFIG_AUDIO_JACK) += jack.mo
+jack.mo-objs = jackaudio.o
+jack.mo-libs := $(JACK_LIBS)
diff --git a/audio/audio.c b/audio/audio.c
index 7a9e680355..95d9fb16ca 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1969,6 +1969,7 @@ void audio_create_pdos(Audiodev *dev)
 CASE(ALSA, alsa, Alsa);
 CASE(COREAUDIO, coreaudio, Coreaudio);
 CASE(DSOUND, dsound, );
+CASE(JACK, jack, Jack);
 CASE(OSS, oss, Oss);
 CASE(PA, pa, Pa);
 CASE(SDL, sdl, );
diff --git a/audio/audio_template.h b/audio/audio_template.h
index 7013d3041f..8dd48ce14e 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -330,6 +330,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_, 
TYPE)(Audiodev *dev)
 dev->u.coreaudio.TYPE);
 case AUDIODEV_DRIVER_DSOUND:
 return dev->u.dsound.TYPE;
+case AUDIODEV_DRIVER_JACK:
+return qapi_AudiodevJackPerDirectionOptions_base(dev->u.jack.TYPE);
 case AUDIODEV_DRIVER_OSS:
 return qapi_AudiodevOssPerDirectionOptions_base(dev->u.oss.TYPE);
 case AUDIODEV_DRIVER_PA:
diff --git a/audio/jackaudio.c b/audio/jackaudio.c
new file mode 100644
index 00..722ddb1dfe
--- /dev/null
+++ b/audio/jackaudio.c
@@ -0,0 +1,667 @@
+/*
+ * QEMU JACK Audio Connection Kit Client
+ *
+ * Copyright (c) 2020 Geoffrey McRae (gnif)
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu/atomic.h"
+#include "qemu-common.h"
+#include "audio.h"
+
+#define AUDIO_CAP "jack"
+#include "audio_int.h"
+
+#include 
+#include 
+
+struct QJack;
+
+typedef enum QJackState {
+QJACK_STATE_DISCONNECTED,
+QJACK_STATE_STOPPED,
+QJACK_STATE_RUNNING,
+QJACK_STATE_SHUTDOWN
+}
+QJackState;
+
+typedef struct QJackBuffer {
+int  channels;
+int  frames;
+uint32_t used;
+int  rptr, wptr;
+float  **data;
+}
+QJackBuffer;
+
+typedef struct QJackClient {
+AudiodevJackPerDirectionOptions *opt;
+
+bool out;
+bool finished;
+bool connect_ports;
+int  packets;
+
+QJackState  state;
+jack_client_t  *client;
+jack_nframes_t  freq;
+
+struct QJack   *j;
+int nchannels;
+int buffersize;
+jack_port_t   **port;
+QJackBuffer fifo;
+}
+QJackClient;
+
+typedef struct QJackOut {
+HWVoiceOut  hw;
+QJackClient c;
+}
+QJackOut;
+
+typedef struct QJackIn {
+HWVoiceIn   hw;
+QJackClient c;
+}
+QJackIn;
+
+static int qjack_client_init(QJackClient *c);
+static void qjack_client_connect_ports(QJackClient *c);
+static void qjack_client_fini(QJackClient *c);
+
+static void qjack_buffer_create(QJackBuffer *buffer, int channels, int frames)
+{
+buffer->channels = channels;
+buffer->frames   = frames;
+buffer->used = 0;
+buffer->rptr = 0;
+buffer->wptr = 0;
+buffer->data = g_malloc(channels * sizeof(float *));
+for (int i = 0; i < channels; ++i) {
+buffer->data[i] = g_malloc(frames * sizeof(float));
+}
+}
+
+static void qjack_buffer_clear(QJackBuffer *buffer)
+{
+assert(buffer->data);

Re: [PATCH v3 7/9] qcow2: Expose bitmaps' size during measure

[Nir Soffer]

On Fri, May 8, 2020 at 9:03 PM Eric Blake  wrote:
>
> It's useful to know how much space can be occupied by qcow2 persistent
> bitmaps, even though such metadata is unrelated to the guest-visible
> data.  Report this value as an additional field, present when
> measuring an existing image and the output format supports bitmaps.
> Update iotest 178 and 190 to updated output, as well as new coverage
> in 190 demonstrating non-zero values made possible with the
> recently-added qemu-img bitmap command.
>
> The addition of a new field demonstrates why we should always
> zero-initialize qapi C structs; while the qcow2 driver still fully
> populates all fields, the raw and crypto drivers had to be tweaked to
> avoid uninitialized data.
>
> See also: https://bugzilla.redhat.com/1779904
>
> Reported-by: Nir Soffer 
> Signed-off-by: Eric Blake 
> ---
>  qapi/block-core.json | 15 +++
>  block/crypto.c   |  2 +-
>  block/qcow2.c| 37 ---
>  block/raw-format.c   |  2 +-
>  qemu-img.c   |  3 +++
>  tests/qemu-iotests/178.out.qcow2 | 16 
>  tests/qemu-iotests/190   | 43 ++--
>  tests/qemu-iotests/190.out   | 23 -
>  8 files changed, 128 insertions(+), 13 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 943df1926a91..9a7a388c7ad3 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -633,18 +633,23 @@
>  # efficiently so file size may be smaller than virtual disk size.
>  #
>  # The values are upper bounds that are guaranteed to fit the new image file.
> -# Subsequent modification, such as internal snapshot or bitmap creation, may
> -# require additional space and is not covered here.
> +# Subsequent modification, such as internal snapshot or further bitmap
> +# creation, may require additional space and is not covered here.
>  #
> -# @required: Size required for a new image file, in bytes.
> +# @required: Size required for a new image file, in bytes, when copying just
> +#guest-visible contents.
>  #
>  # @fully-allocated: Image file size, in bytes, once data has been written
> -#   to all sectors.
> +#   to all sectors, when copying just guest-visible contents.

This does not break old code since previously we always reported only
guest visible content
here, but it changes the semantics, and now you cannot allocate
"required" size, you need
to allocate "required" size with "bitmaps" size. If we add a new
extension all users will have to
change the calculation again.

I think keeping the semantics that "required" and "fully-allocated"
are the size you need based
on the parameters of the command is easier to use and more consistent.
Current the measure
command contract is that invoking it with similar parameters as used
in convert will give
the right measurement needed for the convert command.

> +#
> +# @bitmaps: Additional size required for bitmap metadata in a source image,
> +#   if that bitmap metadata can be copied in addition to guest
> +#   contents. (since 5.1)

Reporting bitmaps without specifying that bitmaps are needed is less consistent
with "qemu-img convert", but has the advantage that we don't need to
check if measure
supports bitmaps. But this will work only if new versions always
report "bitmaps" even when
the value is zero.

With the current way, to measure an image we need to do:

qemu-img info --output json ...
check if image contains bitmaps
qemu-img measure --output json ...
check if bitmaps are reported.

If image has bitmaps and bitmaps are not reported, we know that we have an old
version of qemu-img that does not know how to measure bitmaps.

If bitmaps are reported in both commands we will use the value when creating
block devices.

If we always report bitmaps even when they are zero, we don't need to
run "qemu-img info".

But  my preferred interface is:

   qemu-img measure --bitmaps ...

And report bitmaps only if the user asked to get the value. In this
case the required and
fully-allocated values will include bitmaps.

To learn if qemu-img measure understand bitmaps we can check --help
output, like we did
in the past until we can require the version on all platforms.

An advantage is not having to change old tests.

Nir

>  #
>  # Since: 2.10
>  ##
>  { 'struct': 'BlockMeasureInfo',
> -  'data': {'required': 'int', 'fully-allocated': 'int'} }
> +  'data': {'required': 'int', 'fully-allocated': 'int', '*bitmaps': 'int'} }
>
>  ##
>  # @query-block:
> diff --git a/block/crypto.c b/block/crypto.c
> index 6b21d6bf6c01..eadbcb248563 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -552,7 +552,7 @@ static BlockMeasureInfo *block_crypto_measure(QemuOpts 
> *opts,
>   * Unallocated blocks are still encrypted so allocation status makes no
>   * difference to the file size.
>   */
> -info = g

[Bug 1657538] Re: qemu 2.7.x 2.8 softmmu dont work on BE machine

[Thomas Huth]

** Bug watch removed: Red Hat Bugzilla #1332449
   https://bugzilla.redhat.com/show_bug.cgi?id=1332449

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1657538

Title:
  qemu 2.7.x 2.8 softmmu dont work on BE machine

Status in QEMU:
  Incomplete

Bug description:
  Build on Be machine qemu 2.7.1 and 2.8 in pure softmmu (tgc) dont work on big 
endian hardware .
  tested with ppc-softmmu,i386-softmmu,arm-softmmu same result:

  with :
   ./qemu-system-i386 
  Gtk-Message: Failed to load module "overlay-scrollbar"
  qemu-system-i386: Trying to execute code outside RAM or ROM at 0x000a
  This usually means one of the following happened:

  (1) You told QEMU to execute a kernel for the wrong machine type, and it 
crashed on startup (eg trying to run a raspberry pi kernel on a versatilepb 
QEMU machine)
  (2) You didn't give QEMU a kernel or BIOS filename at all, and QEMU executed 
a ROM full of no-op instructions until it fell off the end
  (3) Your guest kernel has a bug and crashed by jumping off into nowhere

  This is almost always one of the first two, so check your command line and 
that you are using the right type of kernel for this machine.
  If you think option (3) is likely then you can try debugging your guest with 
the -d debug options; in particular -d guest_errors will cause the log to 
include a dump of the guest register state at this point.

  Execution cannot continue; stopping here.

  
  I try to add the -L option with ../pc-bios/bios.bin 
  and have the same result.

  note the ppc-softmmu and ppc64-softmmu work in kvm mode only emulated
  mode have issue.

  
  tested on my hardware a  Qriq P5040 and G5 4x970MP with Ubuntu Mate 16.10 
  thanks
  Luigi

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1657538/+subscriptions

Re: [PATCH RESEND v6 12/36] multi-process: add functions to synchronize proxy and remote endpoints

[Stefan Hajnoczi]

On Wed, Apr 22, 2020 at 09:13:47PM -0700, elena.ufimts...@oracle.com wrote:
> From: Jagannathan Raman 
> 
> In some cases, for example MMIO read, QEMU has to wait for the remote to
> complete a command before proceeding. An eventfd based mechanism is
> added to synchronize QEMU & remote process.

Why are temporary eventfds used instead of sending a reply message from
the remote device program back to QEMU?

> Signed-off-by: John G Johnson 
> Signed-off-by: Jagannathan Raman 
> Signed-off-by: Elena Ufimtseva 
> ---
>  include/io/mpqemu-link.h |  7 +
>  io/mpqemu-link.c | 61 
>  2 files changed, 68 insertions(+)
> 
> diff --git a/include/io/mpqemu-link.h b/include/io/mpqemu-link.h
> index af401e640c..ef95599bca 100644
> --- a/include/io/mpqemu-link.h
> +++ b/include/io/mpqemu-link.h
> @@ -124,4 +124,11 @@ void mpqemu_link_set_callback(MPQemuLinkState *s,
>  void mpqemu_start_coms(MPQemuLinkState *s, MPQemuChannel* chan);
>  bool mpqemu_msg_valid(MPQemuMsg *msg);
>  
> +#define GET_REMOTE_WAIT eventfd(0, EFD_CLOEXEC)
> +#define PUT_REMOTE_WAIT(wait) close(wait)

Hiding this in macros makes the code harder to understand.

Why is an eventfd necessary instead of a reply message? It's simpler and
probably faster to use a reply message instead of creating and passing
temporary eventfds.

> +#define PROXY_LINK_WAIT_DONE 1
> +
> +uint64_t wait_for_remote(int efd);
> +void notify_proxy(int fd, uint64_t val);
> +
>  #endif
> diff --git a/io/mpqemu-link.c b/io/mpqemu-link.c
> index 48f53a8928..cc0a7aecd4 100644
> --- a/io/mpqemu-link.c
> +++ b/io/mpqemu-link.c
> @@ -10,6 +10,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
> +#include 
>  
>  #include "qemu/module.h"
>  #include "io/mpqemu-link.h"
> @@ -204,6 +205,66 @@ int mpqemu_msg_recv(MPQemuMsg *msg, MPQemuChannel *chan)
>  return rc;
>  }
>  
> +/*
> + * wait_for_remote() Synchronizes QEMU and the remote process. The maximum
> + *   wait time is 1s, after which the wait times out.
> + *   The function alse returns a 64 bit return value after
> + *   the wait. The function uses eventfd() to do the wait
> + *   and pass the return values. eventfd() can't return a
> + *   value of '0'. Therefore, all return values are offset
> + *   by '1' at the sending end, and corrected at the
> + *   receiving end.
> + */
> +
> +uint64_t wait_for_remote(int efd)
> +{
> +struct pollfd pfd = { .fd = efd, .events = POLLIN };
> +uint64_t val;
> +int ret;
> +
> +ret = poll(&pfd, 1, 1000);

This 1 second blocking operation is not allowed in an event loop since
it will stall any other event loop activity. If locks are held then
other threads may also be stalled.

It's likely that this will need to change as part of the QEMU event loop
integration. Caller code can be kept mostly unchanged if you use
coroutines.

> +
> +switch (ret) {
> +case 0:
> +qemu_log_mask(LOG_REMOTE_DEBUG, "Error wait_for_remote: Timed 
> out\n");
> +/* TODO: Kick-off error recovery */
> +return UINT64_MAX;
> +case -1:
> +qemu_log_mask(LOG_REMOTE_DEBUG, "Poll error wait_for_remote: %s\n",
> +  strerror(errno));
> +return UINT64_MAX;
> +default:
> +if (read(efd, &val, sizeof(val)) == -1) {
> +qemu_log_mask(LOG_REMOTE_DEBUG, "Error wait_for_remote: %s\n",
> +  strerror(errno));
> +return UINT64_MAX;
> +}
> +}
> +
> +/*
> + * The remote process could write a non-zero value
> + * to the eventfd to wake QEMU up. However, the drawback of using eventfd
> + * for this purpose is that a return value of zero wouldn't wake QEMU up.
> + * Therefore, we offset the return value by one at the remote process and
> + * correct it in the QEMU end.
> + */
> +val = (val == UINT64_MAX) ? val : (val - 1);
> +
> +return val;
> +}
> +
> +void notify_proxy(int efd, uint64_t val)
> +{
> +val = (val == UINT64_MAX) ? val : (val + 1);
> +ssize_t len = -1;
> +
> +len = write(efd, &val, sizeof(val));
> +if (len == -1 || len != sizeof(val)) {
> +qemu_log_mask(LOG_REMOTE_DEBUG, "Error notify_proxy: %s\n",
> +  strerror(errno));
> +}
> +}
> +
>  static gboolean mpqemu_link_handler_prepare(GSource *gsrc, gint *timeout)
>  {
>  g_assert(timeout);
> -- 
> 2.25.GIT
> 


signature.asc
Description: PGP signature

Re: [PATCH RESEND v3 0/2] Makefile: libfdt: build only the strict necessary

[Claudio Fontana]

On 5/11/20 8:33 AM, David Gibson wrote:
> On Wed, Apr 15, 2020 at 10:16:52AM +0200, Claudio Fontana wrote:
>> On 4/14/20 4:03 AM, David Gibson wrote:
>>> On Sat, Apr 11, 2020 at 11:31:48AM +0200, Claudio Fontana wrote:
 v2 -> v3:

 * changed into a 2 patch series; in the second patch we remove the old
   compatibility gunks that were meant for removal some time after 4.1.

 * renamed the libfdt PHONY rule to dtc/all, with the intent to make
   existing working trees forward and backward compatible across the change.

 v1 -> v2:

 * fix error generated when running UNCHECKED_GOALS without prior configure,
   for example during make docker-image-fedora. Without configure, DSOSUF is
   empty, and the module pattern rule in rules.mak that uses this variable
   can match too much; provide a default in the Makefile to avoid it.

 * only attempt to build the archive when there is a non-empty list of 
 objects.
   This could be done in general for the %.a: pattern in rules.mak, but 
 maybe
   there are valid reasons to build an empty .a?

 * removed some intermediate variables that did not add much value
   (LIBFDT_srcdir, LIBFDT_archive)

 Tested locally with 3 VPATH configurations (no-, VPATH, VPATH in src 
 subdir),
 and with docker-image-fedora, docker-test-debug@fedora that failed
 before.
>>>
>>> Seems reasonable to me.  It's a bit of a shame that we can't use the
>>> dtc makefiles more simply for this.  But I don't quickly know how to
>>> fix them upstream to allow that.
>>
>>
>> Hi David,
> 
> Sorry it's taken so long to reply.
> 
>> I tried to look at dtc upstream makefiles, I don't see a perfect
>> solution at the moment.
>>
>> I came up with this idea though (not fully tested..) that _could_
>> work, ie special casing the libfdt target when it is the only goal
>> in MAKECMDGOALS.
>>
>> Any thoughts?
> 
> Bit of a hack, but still better than what we have now.  If you post
> that for dtc upstream , I'd apply
> it.
> 
> Another improvement would be to not include these files on a "make
> clean" - it's kind of annoying how a make clean will regenerate all
> these before removing them.
> 

Hi David,

seems it works well also integrated with qemu, and there should be no problem 
in make clean,
as the version_gen should not get created anymore, the overall clean rule 
should take care
of objects and libs.

While bolting it to qemu I noticed that _avoiding_ passing down CFLAGS _and_ 
QEMU_CFLAGS is
necessary to avoid compilation errors. With those changes made, the integration 
of the
modified dtc seems to work.

the diff needed in qemu (note the absence of DTC_CFLAGS passed down as CFLAGS):

From: Claudio Fontana 
Date: Tue, 12 May 2020 12:24:55 +0200
Subject: [PATCH] Makefile: dtc: build the libfdt target

call the libfdt target from the dtc Makefile, which has been
changed to not require bison, flex, etc.

scripts/ symlink and tests directory creation are not necessary,
and neither is calling the clean rule explicitly.

Signed-off-by: Claudio Fontana 
---
 Makefile  | 8 +++-
 configure | 1 -
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index 34275f57c9..3b88e5eb99 100644
--- a/Makefile
+++ b/Makefile
@@ -527,12 +527,11 @@ $(TARGET_DIRS_RULES):
$(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C $(dir $@) V="$(V)" 
TARGET_DIR="$(dir $@)" $(notdir $@),)
 
 DTC_MAKE_ARGS=-I$(SRC_PATH)/dtc VPATH=$(SRC_PATH)/dtc -C dtc V="$(V)" 
LIBFDT_srcdir=$(SRC_PATH)/dtc/libfdt
-DTC_CFLAGS=$(CFLAGS) $(QEMU_CFLAGS)
-DTC_CPPFLAGS=-I$(BUILD_DIR)/dtc -I$(SRC_PATH)/dtc -I$(SRC_PATH)/dtc/libfdt
+DTC_CPPFLAGS=-I$(SRC_PATH)/dtc/libfdt
 
 .PHONY: dtc/all
-dtc/all: .git-submodule-status dtc/libfdt dtc/tests
-   $(call quiet-command,$(MAKE) $(DTC_MAKE_ARGS) 
CPPFLAGS="$(DTC_CPPFLAGS)" CFLAGS="$(DTC_CFLAGS)" LDFLAGS="$(QEMU_LDFLAGS)" 
ARFLAGS="$(ARFLAGS)" CC="$(CC)" AR="$(AR)" LD="$(LD)" $(SUBDIR_MAKEFLAGS) 
libfdt/libfdt.a,)
+dtc/all: .git-submodule-status dtc/libfdt
+   $(call quiet-command,$(MAKE) $(DTC_MAKE_ARGS) 
CPPFLAGS="$(DTC_CPPFLAGS)" LDFLAGS="$(QEMU_LDFLAGS)" ARFLAGS="$(ARFLAGS)" 
CC="$(CC)" AR="$(AR)" LD="$(LD)" $(SUBDIR_MAKEFLAGS) libfdt,)
 
 dtc/%: .git-submodule-status
@mkdir -p $@
@@ -820,7 +819,6 @@ distclean: clean
rm -rf $$d || exit 1 ; \
 done
rm -Rf .sdk
-   if test -f dtc/version_gen.h; then $(MAKE) $(DTC_MAKE_ARGS) clean; fi
 
 KEYMAPS=da en-gb  et  fr fr-ch  is  lt  no  pt-br  sv \
 ar  de en-us  fi  fr-be  hr it  lv  nl pl  ru th \
diff --git a/configure b/configure
index 0d69c360c0..42554792ec 100755
--- a/configure
+++ b/configure
@@ -4281,7 +4281,6 @@ EOF
   mkdir -p dtc
   if [ "$pwd_is_source_path" != "y" ] ; then
   symlink "$source_path/dtc/Makefile" "dtc/Makefile"
-  symlink "$source_path/dtc/scripts" "dtc/scripts"
   fi
  

Re: [PATCH RESEND v6 13/36] multi-process: setup PCI host bridge for remote device

[Stefan Hajnoczi]

On Wed, Apr 22, 2020 at 09:13:48PM -0700, elena.ufimts...@oracle.com wrote:
> diff --git a/include/remote/pcihost.h b/include/remote/pcihost.h
> new file mode 100644
> index 00..7aca9ccaf1
> --- /dev/null
> +++ b/include/remote/pcihost.h
> @@ -0,0 +1,45 @@
> +/*
> + * PCI Host for remote device
> + *
> + * Copyright © 2018, 2020 Oracle and/or its affiliates.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef REMOTE_PCIHOST_H
> +#define REMOTE_PCIHOST_H
> +
> +#include 
> +#include 
> +
> +#include "exec/memory.h"
> +#include "hw/pci/pcie_host.h"
> +
> +#define TYPE_REMOTE_HOST_DEVICE "remote-pcihost"
> +#define REMOTE_HOST_DEVICE(obj) \
> +OBJECT_CHECK(RemPCIHost, (obj), TYPE_REMOTE_HOST_DEVICE)
> +
> +typedef struct RemPCIHost {

Hmm...this object has no state or behavior specific to remote device
emulation. Could you use an existing PCIe host instead? It's not clear
to me that a new object is needed.

> +/*< private >*/
> +PCIExpressHost parent_obj;
> +/*< public >*/
> +
> +/*
> + * Memory Controller Hub (MCH) may not be necessary for the emulation
> + * program. The two important reasons for implementing a PCI host in the
> + * emulation program are:
> + * - Provide a PCI bus for IO devices
> + * - Enable translation of guest PA to the PCI bar regions
> + *
> + * For both the above mentioned purposes, it doesn't look like we would
> + * need the MCH
> + */
> +
> +MemoryRegion *mr_pci_mem;
> +MemoryRegion *mr_sys_mem;

Unused?

> +MemoryRegion *mr_sys_io;
> +} RemPCIHost;

The name "RemotePCIHost" would be consistent with the QOM type and the
filename. It might seem trivial but when reading code that others have
written every time the naming changes you need to figure out why (just
an inconsistency or is this a different concept/abstraction?).

> +
> +#endif
> diff --git a/remote/Makefile.objs b/remote/Makefile.objs
> index a9b2256b2a..2757f5a265 100644
> --- a/remote/Makefile.objs
> +++ b/remote/Makefile.objs
> @@ -1 +1,2 @@
>  remote-pci-obj-$(CONFIG_MPQEMU) += remote-main.o
> +remote-pci-obj-$(CONFIG_MPQEMU) += pcihost.o
> diff --git a/remote/pcihost.c b/remote/pcihost.c
> new file mode 100644
> index 00..dbe081903e
> --- /dev/null
> +++ b/remote/pcihost.c
> @@ -0,0 +1,64 @@
> +/*
> + * Remote PCI host device
> + *
> + * Copyright © 2018, 2020 Oracle and/or its affiliates.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include 
> +#include 

"qemu/osdep.h" already includes these headers.

> +#include "qemu/osdep.h"
> +#include "hw/pci/pci.h"
> +#include "hw/pci/pci_host.h"
> +#include "hw/pci/pcie_host.h"
> +#include "hw/qdev-properties.h"
> +#include "remote/pcihost.h"
> +#include "exec/memory.h"
> +
> +static const char *remote_host_root_bus_path(PCIHostState *host_bridge,
> + PCIBus *rootbus)
> +{
> +return ":00";
> +}
> +
> +static void remote_host_realize(DeviceState *dev, Error **errp)
> +{
> +char *busname = g_strdup_printf("remote-pci-%ld", (unsigned 
> long)getpid());
> +PCIHostState *pci = PCI_HOST_BRIDGE(dev);
> +RemPCIHost *s = REMOTE_HOST_DEVICE(dev);
> +
> +pci->bus = pci_root_bus_new(DEVICE(s), busname,
> +s->mr_pci_mem, s->mr_sys_io,
> +0, TYPE_PCIE_BUS);
> +}
> +
> +static void remote_host_class_init(ObjectClass *klass, void *data)
> +{
> +DeviceClass *dc = DEVICE_CLASS(klass);
> +PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
> +
> +hc->root_bus_path = remote_host_root_bus_path;
> +dc->realize = remote_host_realize;
> +
> +dc->user_creatable = false;
> +set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +dc->fw_name = "pci";
> +}
> +
> +static const TypeInfo remote_host_info = {
> +.name = TYPE_REMOTE_HOST_DEVICE,
> +.parent = TYPE_PCIE_HOST_BRIDGE,
> +.instance_size = sizeof(RemPCIHost),
> +.class_init = remote_host_class_init,
> +};
> +
> +static void remote_machine_register(void)
> +{
> +type_register_static(&remote_host_info);
> +}
> +
> +type_init(remote_machine_register)

The naming in this file is inconsistent:

remote_host_root_bus_path -> remote_pcihost_root_bus_path
remote_machine_register -> remote_pcihost_register

I haven't listed all instances.


signature.asc
Description: PGP signature

[PATCH v4 0/6] scripts: More Python fixes

[Philippe Mathieu-Daudé]

Trivial Python3 fixes, again...

Since v3:
- Fixed missing scripts/qemugdb/timers.py (kwolf)
- Cover more scripts
- Check for __main__ in few scripts

Since v2:
- Remove patch updating MAINTAINERS

Since v1:
- Added Alex Bennée A-b tags
- Addressed John Snow review comments
  - Use /usr/bin/env
  - Do not modify os.path (dropped last patch)

Philippe Mathieu-Daudé (6):
  scripts/qemugdb: Remove shebang header
  scripts/qemu-gdb: Use Python 3 interpreter
  scripts/qmp: Use Python 3 interpreter
  scripts/kvm/vmxcap: Use Python 3 interpreter and add pseudo-main()
  scripts/modules/module_block: Use Python 3 interpreter & add
pseudo-main
  tests/migration/guestperf: Use Python 3 interpreter

 scripts/kvm/vmxcap |  7 ---
 scripts/modules/module_block.py| 31 +++---
 scripts/qemu-gdb.py|  4 ++--
 scripts/qemugdb/__init__.py|  3 +--
 scripts/qemugdb/aio.py |  3 +--
 scripts/qemugdb/coroutine.py   |  3 +--
 scripts/qemugdb/mtree.py   |  4 +---
 scripts/qemugdb/tcg.py |  1 -
 scripts/qemugdb/timers.py  |  1 -
 scripts/qmp/qom-get|  2 +-
 scripts/qmp/qom-list   |  2 +-
 scripts/qmp/qom-set|  2 +-
 scripts/qmp/qom-tree   |  2 +-
 tests/migration/guestperf-batch.py |  2 +-
 tests/migration/guestperf-plot.py  |  2 +-
 tests/migration/guestperf.py   |  2 +-
 16 files changed, 33 insertions(+), 38 deletions(-)

-- 
2.21.3

[PATCH v4 1/6] scripts/qemugdb: Remove shebang header

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé 

These scripts are loaded as plugin by GDB (and they don't
have any __main__ entry point). Remove the shebang header.

Acked-by: Alex Bennée 
Signed-off-by: Philippe Mathieu-Daudé 
---
 scripts/qemugdb/__init__.py  | 3 +--
 scripts/qemugdb/aio.py   | 3 +--
 scripts/qemugdb/coroutine.py | 3 +--
 scripts/qemugdb/mtree.py | 4 +---
 scripts/qemugdb/tcg.py   | 1 -
 scripts/qemugdb/timers.py| 1 -
 6 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/scripts/qemugdb/__init__.py b/scripts/qemugdb/__init__.py
index 969f552b26..da8ff612e5 100644
--- a/scripts/qemugdb/__init__.py
+++ b/scripts/qemugdb/__init__.py
@@ -1,5 +1,4 @@
-#!/usr/bin/python
-
+#
 # GDB debugging support
 #
 # Copyright (c) 2015 Linaro Ltd
diff --git a/scripts/qemugdb/aio.py b/scripts/qemugdb/aio.py
index 2ba00c..d7c1ba0c28 100644
--- a/scripts/qemugdb/aio.py
+++ b/scripts/qemugdb/aio.py
@@ -1,5 +1,4 @@
-#!/usr/bin/python
-
+#
 # GDB debugging support: aio/iohandler debug
 #
 # Copyright (c) 2015 Red Hat, Inc.
diff --git a/scripts/qemugdb/coroutine.py b/scripts/qemugdb/coroutine.py
index 41e079d0e2..db61389022 100644
--- a/scripts/qemugdb/coroutine.py
+++ b/scripts/qemugdb/coroutine.py
@@ -1,5 +1,4 @@
-#!/usr/bin/python
-
+#
 # GDB debugging support
 #
 # Copyright 2012 Red Hat, Inc. and/or its affiliates
diff --git a/scripts/qemugdb/mtree.py b/scripts/qemugdb/mtree.py
index 3030a60d3f..8fe42c3c12 100644
--- a/scripts/qemugdb/mtree.py
+++ b/scripts/qemugdb/mtree.py
@@ -1,5 +1,4 @@
-#!/usr/bin/python
-
+#
 # GDB debugging support
 #
 # Copyright 2012 Red Hat, Inc. and/or its affiliates
@@ -84,4 +83,3 @@ def print_item(self, ptr, offset = gdb.Value(0), level = 0):
 while not isnull(subregion):
 self.print_item(subregion, addr, level)
 subregion = subregion['subregions_link']['tqe_next']
-
diff --git a/scripts/qemugdb/tcg.py b/scripts/qemugdb/tcg.py
index 18880fc9a7..16c03c06a9 100644
--- a/scripts/qemugdb/tcg.py
+++ b/scripts/qemugdb/tcg.py
@@ -1,4 +1,3 @@
-#!/usr/bin/python
 # -*- coding: utf-8 -*-
 #
 # GDB debugging support, TCG status
diff --git a/scripts/qemugdb/timers.py b/scripts/qemugdb/timers.py
index f0e132d27a..46537b27cf 100644
--- a/scripts/qemugdb/timers.py
+++ b/scripts/qemugdb/timers.py
@@ -1,4 +1,3 @@
-#!/usr/bin/python
 # -*- coding: utf-8 -*-
 # GDB debugging support
 #
-- 
2.21.3

[PATCH v4 6/6] tests/migration/guestperf: Use Python 3 interpreter

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/migration/guestperf-batch.py | 2 +-
 tests/migration/guestperf-plot.py  | 2 +-
 tests/migration/guestperf.py   | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/migration/guestperf-batch.py 
b/tests/migration/guestperf-batch.py
index cb150ce804..f1e900908d 100755
--- a/tests/migration/guestperf-batch.py
+++ b/tests/migration/guestperf-batch.py
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python3
 #
 # Migration test batch comparison invokation
 #
diff --git a/tests/migration/guestperf-plot.py 
b/tests/migration/guestperf-plot.py
index d70bb7a557..907151011a 100755
--- a/tests/migration/guestperf-plot.py
+++ b/tests/migration/guestperf-plot.py
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python3
 #
 # Migration test graph plotting command
 #
diff --git a/tests/migration/guestperf.py b/tests/migration/guestperf.py
index 99b027e8ba..ba1c4bc4ca 100755
--- a/tests/migration/guestperf.py
+++ b/tests/migration/guestperf.py
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python3
 #
 # Migration test direct invokation command
 #
-- 
2.21.3

[PATCH v4 5/6] scripts/modules/module_block: Use Python 3 interpreter & add pseudo-main

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 scripts/modules/module_block.py | 31 ---
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/scripts/modules/module_block.py b/scripts/modules/module_block.py
index f23191fac1..2e7021b952 100644
--- a/scripts/modules/module_block.py
+++ b/scripts/modules/module_block.py
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python3
 #
 # Module information generator
 #
@@ -10,7 +10,6 @@
 # This work is licensed under the terms of the GNU GPL, version 2.
 # See the COPYING file in the top-level directory.
 
-import sys
 import os
 
 def get_string_struct(line):
@@ -80,19 +79,21 @@ def print_bottom(fheader):
 #endif
 ''')
 
-# First argument: output file
-# All other arguments: modules source files (.c)
-output_file = sys.argv[1]
-with open(output_file, 'w') as fheader:
-print_top(fheader)
+if __name__ == '__main__':
+import sys
+# First argument: output file
+# All other arguments: modules source files (.c)
+output_file = sys.argv[1]
+with open(output_file, 'w') as fheader:
+print_top(fheader)
 
-for filename in sys.argv[2:]:
-if os.path.isfile(filename):
-process_file(fheader, filename)
-else:
-print("File " + filename + " does not exist.", file=sys.stderr)
-sys.exit(1)
+for filename in sys.argv[2:]:
+if os.path.isfile(filename):
+process_file(fheader, filename)
+else:
+print("File " + filename + " does not exist.", file=sys.stderr)
+sys.exit(1)
 
-print_bottom(fheader)
+print_bottom(fheader)
 
-sys.exit(0)
+sys.exit(0)
-- 
2.21.3

[PATCH v4 2/6] scripts/qemu-gdb: Use Python 3 interpreter

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé 

Signed-off-by: Philippe Mathieu-Daudé 
---
 scripts/qemu-gdb.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/qemu-gdb.py b/scripts/qemu-gdb.py
index f2a305c42e..e0bfa7b5a4 100644
--- a/scripts/qemu-gdb.py
+++ b/scripts/qemu-gdb.py
@@ -1,5 +1,5 @@
-#!/usr/bin/python
-
+#!/usr/bin/env python3
+#
 # GDB debugging support
 #
 # Copyright 2012 Red Hat, Inc. and/or its affiliates
-- 
2.21.3

[PATCH v4 3/6] scripts/qmp: Use Python 3 interpreter

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé 

Signed-off-by: Philippe Mathieu-Daudé 
---
 scripts/qmp/qom-get  | 2 +-
 scripts/qmp/qom-list | 2 +-
 scripts/qmp/qom-set  | 2 +-
 scripts/qmp/qom-tree | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/scripts/qmp/qom-get b/scripts/qmp/qom-get
index 007b4cd442..7c5ede91bb 100755
--- a/scripts/qmp/qom-get
+++ b/scripts/qmp/qom-get
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python3
 ##
 # QEMU Object Model test tools
 #
diff --git a/scripts/qmp/qom-list b/scripts/qmp/qom-list
index 03bda3446b..bb68fd65d4 100755
--- a/scripts/qmp/qom-list
+++ b/scripts/qmp/qom-list
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python3
 ##
 # QEMU Object Model test tools
 #
diff --git a/scripts/qmp/qom-set b/scripts/qmp/qom-set
index c37fe78b00..19881d85e9 100755
--- a/scripts/qmp/qom-set
+++ b/scripts/qmp/qom-set
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python3
 ##
 # QEMU Object Model test tools
 #
diff --git a/scripts/qmp/qom-tree b/scripts/qmp/qom-tree
index 1c8acf61e7..fa91147a03 100755
--- a/scripts/qmp/qom-tree
+++ b/scripts/qmp/qom-tree
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python3
 ##
 # QEMU Object Model test tools
 #
-- 
2.21.3

[PATCH v4 4/6] scripts/kvm/vmxcap: Use Python 3 interpreter and add pseudo-main()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 scripts/kvm/vmxcap | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/scripts/kvm/vmxcap b/scripts/kvm/vmxcap
index 971ed0e721..6fe66d5f57 100755
--- a/scripts/kvm/vmxcap
+++ b/scripts/kvm/vmxcap
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python3
 #
 # tool for querying VMX capabilities
 #
@@ -275,5 +275,6 @@ controls = [
 ),
 ]
 
-for c in controls:
-c.show()
+if __name__ == '__main__':
+for c in controls:
+c.show()
-- 
2.21.3

Re: [PATCH v3 1/2] scripts/qemugdb: Remove shebang header

[Philippe Mathieu-Daudé]

On 5/12/20 10:55 AM, Kevin Wolf wrote:

Am 12.05.2020 um 09:06 hat Philippe Mathieu-Daudé geschrieben:

These scripts are loaded as plugin by GDB (and they don't
have any __main__ entry point). Remove the shebang header.

Acked-by: Alex Bennée 
Signed-off-by: Philippe Mathieu-Daudé 
---
  scripts/qemugdb/__init__.py  | 3 +--
  scripts/qemugdb/aio.py   | 3 +--
  scripts/qemugdb/coroutine.py | 3 +--
  scripts/qemugdb/mtree.py | 4 +---
  scripts/qemugdb/tcg.py   | 1 -


There is still a shebang line left in scripts/qemugdb/timers.py.


Oops, thanks for catching this.



Kevin

Re: [PATCH RESEND v6 14/36] multi-process: setup a machine object for remote device process

[Stefan Hajnoczi]

On Wed, Apr 22, 2020 at 09:13:49PM -0700, elena.ufimts...@oracle.com wrote:
> From: Jagannathan Raman 
> 
> remote-machine object sets up various subsystems of the remote device
> process. Instantiate PCI host bridge object and initialize RAM, IO &
> PCI memory regions.
> 
> Signed-off-by: John G Johnson 
> Signed-off-by: Jagannathan Raman 
> Signed-off-by: Elena Ufimtseva 
> ---
>  MAINTAINERS   |  2 +
>  Makefile.objs |  1 +
>  exec.c|  3 +-
>  include/exec/address-spaces.h |  2 +
>  include/remote/machine.h  | 30 +
>  remote/Makefile.objs  |  2 +
>  remote/machine.c  | 84 +++
>  remote/remote-main.c  |  7 +++

Now that the separate remote emulation program is going away I think it
makes sense to move the PCIe host and machine type into hw/:

  hw/pci-host/remote.c <-- PCIe host
  hw/i386/remote.c <-- machine type (could be moved again later if
   other architectures are supported)

> diff --git a/exec.c b/exec.c
> index d0ac9545f4..5b1e414099 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -161,7 +161,6 @@ typedef struct subpage_t {
>  #define PHYS_SECTION_UNASSIGNED 0
>  
>  static void io_mem_init(void);
> -static void memory_map_init(void);

The memory_map_init() change is unnecessary once a softmmu target is
used since it will be called from cpu_exec_init_all().

> +static void remote_machine_init(Object *obj)
> +{
> +RemMachineState *s = REMOTE_MACHINE(obj);
> +RemPCIHost *rem_host;
> +MemoryRegion *system_memory, *system_io, *pci_memory;
> +
> +Error *error_abort = NULL;
> +
> +object_property_add_child(object_get_root(), "machine", obj, 
> &error_abort);
> +if (error_abort) {

error_abort aborts the program so handling it is not necessary.

> +error_report_err(error_abort);
> +}
> +
> +memory_map_init();
> +
> +system_memory = get_system_memory();
> +system_io = get_system_io();
> +
> +pci_memory = g_new(MemoryRegion, 1);
> +memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
> +
> +rem_host = REMOTE_HOST_DEVICE(qdev_create(NULL, 
> TYPE_REMOTE_HOST_DEVICE));
> +
> +rem_host->mr_pci_mem = pci_memory;
> +rem_host->mr_sys_mem = system_memory;
> +rem_host->mr_sys_io = system_io;
> +
> +s->host = rem_host;
> +
> +object_property_add_child(OBJECT(s), "remote-device", OBJECT(rem_host),
> +  &error_abort);
> +if (error_abort) {

error_abort aborts the program so handling it is not necessary.

> +error_report_err(error_abort);
> +return;
> +}
> +
> +qemu_mutex_lock_iothread();

This will be executed with the iothread lock held. There is no need to
acquire it.

> +memory_region_add_subregion_overlap(system_memory, 0x0, pci_memory, -1);
> +qemu_mutex_unlock_iothread();
> +
> +qdev_init_nofail(DEVICE(rem_host));
> +}


signature.asc
Description: PGP signature

[PATCH] tests/guest-debug: catch hanging guests

[Alex Bennée]

If gdb never actually connected with the guest we need to catch that
and clean-up after ourselves.

Signed-off-by: Alex Bennée 
---
 tests/guest-debug/run-test.py | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/tests/guest-debug/run-test.py b/tests/guest-debug/run-test.py
index d9af9573b9e..71c55690546 100755
--- a/tests/guest-debug/run-test.py
+++ b/tests/guest-debug/run-test.py
@@ -80,4 +80,10 @@ if __name__ == '__main__':
 print("GDB crashed? SKIPPING")
 exit(0)
 
+try:
+inferior.wait(2)
+except subprocess.TimeoutExpired:
+print("GDB never connected? Killed guest")
+inferior.kill()
+
 exit(result)
-- 
2.20.1

Re: [RFC] bdrv_flush: only use fast path when in owned AioContext

[Kevin Wolf]

Am 11.05.2020 um 18:50 hat Stefan Reiter geschrieben:
> Just because we're in a coroutine doesn't imply ownership of the context
> of the flushed drive. In such a case use the slow path which explicitly
> enters bdrv_flush_co_entry in the correct AioContext.
> 
> Signed-off-by: Stefan Reiter 
> ---
> 
> We've experienced some lockups in this codepath when taking snapshots of VMs
> with drives that have IO-Threads enabled (we have an async 'savevm'
> implementation running from a coroutine).
> 
> Currently no reproducer for upstream versions I could find, but in testing 
> this
> patch fixes all issues we're seeing and I think the logic checks out.
> 
> The fast path pattern is repeated a few times in this file, so if this change
> makes sense, it's probably worth evaluating the other occurences as well.

What do you mean by "owning" the context? If it's about taking the
AioContext lock, isn't the problem more with calling bdrv_flush() from
code that doesn't take the locks?

Though I think we have some code that doesn't only rely on holding the
AioContext locks, but that actually depends on running in the right
thread, so the change looks right anyway.

Kevin

>  block/io.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index aba67f66b9..ee7310fa13 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2895,8 +2895,9 @@ int bdrv_flush(BlockDriverState *bs)
>  .ret = NOT_DONE,
>  };
>  
> -if (qemu_in_coroutine()) {
> -/* Fast-path if already in coroutine context */
> +if (qemu_in_coroutine() &&
> +bdrv_get_aio_context(bs) == qemu_get_current_aio_context()) {
> +/* Fast-path if already in coroutine and we own the drive's context 
> */
>  bdrv_flush_co_entry(&flush_co);
>  } else {
>  co = qemu_coroutine_create(bdrv_flush_co_entry, &flush_co);
> -- 
> 2.20.1
> 
> 

Re: [PATCH v3 7/9] qcow2: Expose bitmaps' size during measure

[Max Reitz]

On 12.05.20 12:17, Nir Soffer wrote:
> On Fri, May 8, 2020 at 9:03 PM Eric Blake  wrote:
>>
>> It's useful to know how much space can be occupied by qcow2 persistent
>> bitmaps, even though such metadata is unrelated to the guest-visible
>> data.  Report this value as an additional field, present when
>> measuring an existing image and the output format supports bitmaps.
>> Update iotest 178 and 190 to updated output, as well as new coverage
>> in 190 demonstrating non-zero values made possible with the
>> recently-added qemu-img bitmap command.
>>
>> The addition of a new field demonstrates why we should always
>> zero-initialize qapi C structs; while the qcow2 driver still fully
>> populates all fields, the raw and crypto drivers had to be tweaked to
>> avoid uninitialized data.
>>
>> See also: https://bugzilla.redhat.com/1779904
>>
>> Reported-by: Nir Soffer 
>> Signed-off-by: Eric Blake 
>> ---
>>  qapi/block-core.json | 15 +++
>>  block/crypto.c   |  2 +-
>>  block/qcow2.c| 37 ---
>>  block/raw-format.c   |  2 +-
>>  qemu-img.c   |  3 +++
>>  tests/qemu-iotests/178.out.qcow2 | 16 
>>  tests/qemu-iotests/190   | 43 ++--
>>  tests/qemu-iotests/190.out   | 23 -
>>  8 files changed, 128 insertions(+), 13 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 943df1926a91..9a7a388c7ad3 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -633,18 +633,23 @@
>>  # efficiently so file size may be smaller than virtual disk size.
>>  #
>>  # The values are upper bounds that are guaranteed to fit the new image file.
>> -# Subsequent modification, such as internal snapshot or bitmap creation, may
>> -# require additional space and is not covered here.
>> +# Subsequent modification, such as internal snapshot or further bitmap
>> +# creation, may require additional space and is not covered here.
>>  #
>> -# @required: Size required for a new image file, in bytes.
>> +# @required: Size required for a new image file, in bytes, when copying just
>> +#guest-visible contents.
>>  #
>>  # @fully-allocated: Image file size, in bytes, once data has been written
>> -#   to all sectors.
>> +#   to all sectors, when copying just guest-visible 
>> contents.
> 
> This does not break old code since previously we always reported only
> guest visible content
> here, but it changes the semantics, and now you cannot allocate
> "required" size, you need
> to allocate "required" size with "bitmaps" size.

Only if you copy the bitmaps, though, which requires using the --bitmaps
switch for convert.

> If we add a new
> extension all users will have to
> change the calculation again.

It was my impression that existing users won’t have to do that, because
they don’t use --bitmaps yet.

In contrast, if we included the bitmap size in @required or
@fully-allocated, then previous users that didn’t copy bitmaps to the
destination (which they couldn’t) would allocate too much space.

...revisiting this after reading more of your mail: With a --bitmaps
switch, existing users wouldn’t suffer from such compatibility problems.
 However, then users (that now want to copy bitmaps) will have to pass
the new --bitmaps flag in the command line, and I don’t see how that’s
less complicated than just adding @bitmaps to @required.

> I think keeping the semantics that "required" and "fully-allocated"
> are the size you need based
> on the parameters of the command is easier to use and more consistent.
> Current the measure
> command contract is that invoking it with similar parameters as used
> in convert will give
> the right measurement needed for the convert command.
> 
>> +#
>> +# @bitmaps: Additional size required for bitmap metadata in a source image,
>> +#   if that bitmap metadata can be copied in addition to guest
>> +#   contents. (since 5.1)
> 
> Reporting bitmaps without specifying that bitmaps are needed is less 
> consistent
> with "qemu-img convert", but has the advantage that we don't need to
> check if measure
> supports bitmaps. But this will work only if new versions always
> report "bitmaps" even when
> the value is zero.
> 
> With the current way, to measure an image we need to do:
> 
> qemu-img info --output json ...
> check if image contains bitmaps
> qemu-img measure --output json ...
> check if bitmaps are reported.
> 
> If image has bitmaps and bitmaps are not reported, we know that we have an old
> version of qemu-img that does not know how to measure bitmaps.

Well, but you’ll also see that convert --bitmaps will fail.  But I can
see that you probably want to know about that before you create the
target image.

> If bitmaps are reported in both commands we will use the value when creating
> block devices.

And otherwise?  Do you error out, or d

Re: [PATCH v5 13/15] acpi: drop build_piix4_pm()

[Gerd Hoffmann]

On Mon, May 11, 2020 at 09:37:32PM +0200, Igor Mammedov wrote:
> On Thu,  7 May 2020 15:16:38 +0200
> Gerd Hoffmann  wrote:
> 
> > The _SB.PCI0.PX13.P13C opregion (holds isa device enable bits)
> > is not used any more, remove it from DSDT.
> > 
> > Signed-off-by: Gerd Hoffmann 
> > ---
> >  hw/i386/acpi-build.c | 16 
> >  1 file changed, 16 deletions(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 765409a90eb6..c1e63cce5e8e 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1441,21 +1441,6 @@ static void build_q35_isa_bridge(Aml *table)
> >  aml_append(table, scope);
> >  }
> >  
> > -static void build_piix4_pm(Aml *table)
> > -{
> > -Aml *dev;
> > -Aml *scope;
> > -
> > -scope =  aml_scope("_SB.PCI0");
> > -dev = aml_device("PX13");
> > -aml_append(dev, aml_name_decl("_ADR", aml_int(0x00010003)));
> I agree about removing P13C but I'm not sure if it's safe to remove
> whole isa bridge

The isa bridge is _SB0.PCI0.ISA (piix4 function 0).
_SB.PCI0.PX13 is the acpi pm device (piix4 function 3).

So this does _not_ remove the isa bridge.  And given that PX13 has
nothing declared beside the opregion we are removing and I have not
found any other references to the PX13 device I assumed it is ok to drop
that one too.  Didn't notice any odd side effects in testing.

take care,
  Gerd

Re: [PATCH v4 0/6] scripts: More Python fixes

[Kevin Wolf]

Am 12.05.2020 um 12:32 hat Philippe Mathieu-Daudé geschrieben:
> Trivial Python3 fixes, again...
> 
> Since v3:
> - Fixed missing scripts/qemugdb/timers.py (kwolf)
> - Cover more scripts
> - Check for __main__ in few scripts

I'm not sure if the __main__ check actually provides anything useful in
source files of standalone tools that aren't supposed to be imported
from somewhere else. But of course, it's not wrong either.

Reviewed-by: Kevin Wolf 

Re: [PATCH 01/10] ui/win32-kbd-hook: handle AltGr in a hook procedure

[Gerd Hoffmann]

On Sun, May 10, 2020 at 08:42:55PM +0200, Volker Rümelin wrote:
> Import win32 keyboard hooking code from project spice-gtk.

> +/*
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version.  See the COPYING file in the
> + * top-level directory.
> + */

Can you add the import note to the comment too?

> +void win32_kbd_set_window(HWND hwnd);

Can we use uint32_t here (or whatever HWND actually is) so the header
can also be included on non-win32 machines?

thanks,
  Gerd

Re: [PATCH 02/10] ui/gtk: fix handling of AltGr key on Windows

[Gerd Hoffmann]

  Hi,

> +#ifdef CONFIG_WIN32
> +win32_kbd_set_grab(true);
> +#endif

Can we have stubs for these (see stubs/ directory for examples) so we
can avoid the #ifdefs?

thanks,
  Gerd

Re: [PATCH 03/10] ui/gkt: release all keys on grab-broken-event

[Gerd Hoffmann]

On Sun, May 10, 2020 at 08:42:57PM +0200, Volker Rümelin wrote:
> There is no way to grab the Ctrl-Alt-Del key combination on
> Windows. [ ... ]

> +#ifdef CONFIG_WIN32

A comment here would be good (so you can see the reason for this without
digging into the git commit log).

thanks,
  Gerd

Re: [PATCH 09/10] ui/gtk: don't pass on win keys without keyboard grab

[Gerd Hoffmann]

On Sun, May 10, 2020 at 08:43:03PM +0200, Volker Rümelin wrote:
> This patch applies commit c68f74b02e "win32: do not handle win
> keys when the keyboard is not grabbed" from project spice-gtk
> to ui/gtk.c.

Which issue does this solve?

thanks,
  Gerd

Re: [PATCH 10/10] ui/gtk: use native keyboard scancodes on Windows

[Daniel P . Berrangé]

On Sun, May 10, 2020 at 08:43:04PM +0200, Volker Rümelin wrote:
> Since GTK 3.22 the function gdk_event_get_scancode() is
> available. On Windows this function returns keyboard scancodes
> and some extended flags. These raw keyboard scancodes are much
> better suited for this use case than the half-cooked win32
> virtual-key codes because scancodes report the key position on
> the keyboard and the positions are independent of national
> language settings.
> 
> Signed-off-by: Volker Rümelin 
> ---
>  ui/gtk.c | 33 +
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/ui/gtk.c b/ui/gtk.c
> index a43fddc57f..242b378bf1 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -1016,8 +1016,13 @@ static const guint16 *gd_get_keymap(size_t *maplen)
>  #ifdef GDK_WINDOWING_WIN32
>  if (GDK_IS_WIN32_DISPLAY(dpy)) {
>  trace_gd_keymap_windowing("win32");
> +#if GTK_CHECK_VERSION(3, 22, 0)
> +*maplen = qemu_input_map_atset1_to_qcode_len;
> +return qemu_input_map_atset1_to_qcode;
> +#else
>  *maplen = qemu_input_map_win32_to_qcode_len;
>  return qemu_input_map_win32_to_qcode;
> +#endif

Our current min GTK is 3.14, which I picked here:

commit 58296cb61866195297510e946a51acc5f0b9639e
Author: Daniel P. Berrangé 
Date:   Wed Aug 22 14:15:53 2018 +0100

ui: increase min required GTK3 version to 3.14.0

Per supported platforms doc[1], the various min GTK3 on relevant distros is:

  RHEL-7.0: 3.8.8
  RHEL-7.2: 3.14.13
  RHEL-7.4: 3.22.10
  RHEL-7.5: 3.22.26
  Debian (Stretch): 3.22.11
  Debian (Jessie): 3.14.5
  OpenBSD (Ports): 3.22.30
  FreeBSD (Ports): 3.22.29
  OpenSUSE Leap 15: 3.22.30
  SLE12-SP2: Unknown
  Ubuntu (Xenial): 3.18.9
  macOS (Homebrew): 3.22.30

This suggests that a minimum GTK3 of 3.14.0 is a reasonable target,
as users are unlikely to be stuck on RHEL-7.0/7.1 still



since that time, we no longer support Debian Jessie, since Debian Buster
is now released. We also no longer support Ubuntu Xenial (16.04), since
we now only need Ubuntu Bionic (18.04) and Focal (20.04).

So we can justify moving the minium Gtk in QEMU to 3.22 at this time.

This will avoid you needing to do versioned ifdef for this new functionality.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH 2/2] 9pfs: fix init_in_iov_from_pdu truncating size

[Christian Schoenebeck]

On Dienstag, 12. Mai 2020 00:09:46 CEST Stefano Stabellini wrote:
> On Sun, 10 May 2020, Christian Schoenebeck wrote:
> > Commit SHA-1 16724a173049ac29c7b5ade741da93a0f46edff7 introduced
> > truncating the response to the currently available transport buffer
> > size, which was supposed to fix an 9pfs error on Xen boot where
> > transport buffer might still be smaller than required for response.
> > 
> > Unfortunately this change broke small reads (with less than 12
> > bytes).
> > 
> > To address both concerns, check the actual response type and only
> > truncate reply for Rreaddir responses,
> 
> I realize you mean "Rread" (not Rreaddir).

Yes, that's currently an unfortunate auto completion issue in my head due to 
having spent too much time on readdir() code lately. I'm working on it. ;-)

> Are we sure that truncation
> can only happen with Rread? I checked the spec it looks like Directories
> are pretty much like files from the spec point of view. So it seems to
> me that truncation might be possible there too.

That's the big question here: do we need to address this for other types than 
Rread? So far I only addressed this for Rread because from what you described 
you were encountering that Xen boot issue only with reads, right?

What we cannot do is simply truncating any response arbitrarily without 
looking at the precise response type. With Rread (9p2000.L) it's quite simple, 
because it is Ok (i.e. from client & guest OS perspective) to return 
*arbitrarily* less data than originally requested by client.

A problem with Rread would be protocol variant 9p2000.u, because such clients 
would also use Rread on directories. In that case client would end up with 
trash data -> undefined behaviour.

Likewise for Rreaddir (9p2000.L) it would be much more complicated, we could 
truncate the response, but we may not truncate individual directory entries of 
that response. So not only the Rreaddir header must be preserved, we also 
would have to look at the individual entries (their sizes vary for each entry 
individually) returned and only truncate at the boundaries of individual 
entries inside the response, otherwise client would receive trash data -> 
undefined behaviour.

And that's it, for all other 9P types we can't truncate response at all. For 
those types we could probably just return EAGAIN, but would it help? Probably 
would require changes on client side as well then to handle this correctly.

> > and only if truncated reply would at least return one payload byte to
> > client. Use Rreaddir's precise header size (11) for this instead of
> > P9_IOHDRSZ.
> 
> Ah! That's the underlying error isn't it? That P9_IOHDRSZ is not really
> the size of the reply header, it is bigger. Hence the check:
> 
>   if (buf_size < P9_IOHDRSZ) {
> 
> can be wrong for very small sizes.

Exactly. We need to handle this according to the precise header size of the 
respective response type. But unfortunately that's not enough, like described 
above in detail.

> > Fixes: 16724a173049ac29c7b5ade741da93a0f46edff7
> > Fixes: https://bugs.launchpad.net/bugs/1877688
> > Signed-off-by: Christian Schoenebeck 
> > ---
> > 
> >  hw/9pfs/virtio-9p-device.c | 35 +++
> >  hw/9pfs/xen-9p-backend.c   | 38 +-
> >  2 files changed, 56 insertions(+), 17 deletions(-)
> > 
> > diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> > index 536447a355..57e4d92ecb 100644
> > --- a/hw/9pfs/virtio-9p-device.c
> > +++ b/hw/9pfs/virtio-9p-device.c
> > @@ -154,15 +154,34 @@ static void virtio_init_in_iov_from_pdu(V9fsPDU
> > *pdu, struct iovec **piov,> 
> >  VirtQueueElement *elem = v->elems[pdu->idx];
> >  size_t buf_size = iov_size(elem->in_sg, elem->in_num);
> > 
> > -if (buf_size < P9_IOHDRSZ) {
> > -VirtIODevice *vdev = VIRTIO_DEVICE(v);
> > +if (pdu->id + 1 == P9_RREAD) {
> > +/* size[4] Rread tag[2] count[4] data[count] */
> 
> 4+2+4 = 10

That's 4+1+2+4 = 11

You were missing "Rread" here which identifies the (numeric) 9P response type 
and which is always 1 byte in size.

> > +const size_t hdr_size = 11;
> 
> Are you adding 1 to account for "count"?

The idea was that from client perspective a successful read() call must at 
least return 1 byte. We must not return 0 bytes, because that would indicate 
EOF for POSIX systems. For that reason the minimum Rread response size would 
be:

header_size + 1 payload_byte

and hence

11 + 1 = 12

> > +/*
> > + * If current transport buffer size is smaller than actually
> > required + * for this Rreaddir response, then truncate the
> > response to the + * currently available transport buffer size,
> > however only if it would + * at least allow to return 1 payload
> > byte to client.
> > + */
> > +if (buf_size < hdr_size + 1) {
> 
> If you have already added 1 before, why do we need to add 1 again here?

Re: [RFC] bdrv_flush: only use fast path when in owned AioContext

[Kevin Wolf]

Am 12.05.2020 um 12:57 hat Kevin Wolf geschrieben:
> Am 11.05.2020 um 18:50 hat Stefan Reiter geschrieben:
> > Just because we're in a coroutine doesn't imply ownership of the context
> > of the flushed drive. In such a case use the slow path which explicitly
> > enters bdrv_flush_co_entry in the correct AioContext.
> > 
> > Signed-off-by: Stefan Reiter 
> > ---
> > 
> > We've experienced some lockups in this codepath when taking snapshots of VMs
> > with drives that have IO-Threads enabled (we have an async 'savevm'
> > implementation running from a coroutine).
> > 
> > Currently no reproducer for upstream versions I could find, but in testing 
> > this
> > patch fixes all issues we're seeing and I think the logic checks out.
> > 
> > The fast path pattern is repeated a few times in this file, so if this 
> > change
> > makes sense, it's probably worth evaluating the other occurences as well.
> 
> What do you mean by "owning" the context? If it's about taking the
> AioContext lock, isn't the problem more with calling bdrv_flush() from
> code that doesn't take the locks?
> 
> Though I think we have some code that doesn't only rely on holding the
> AioContext locks, but that actually depends on running in the right
> thread, so the change looks right anyway.

Well, the idea is right, but the change itself isn't, of course. If
we're already in coroutine context, we must not busy wait with
BDRV_POLL_WHILE(). I'll see if I can put something together after lunch.

Kevin

Re: [PATCH v4 4/6] scripts/kvm/vmxcap: Use Python 3 interpreter and add pseudo-main()

[Paolo Bonzini]

On 12/05/20 12:32, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  scripts/kvm/vmxcap | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/kvm/vmxcap b/scripts/kvm/vmxcap
> index 971ed0e721..6fe66d5f57 100755
> --- a/scripts/kvm/vmxcap
> +++ b/scripts/kvm/vmxcap
> @@ -1,4 +1,4 @@
> -#!/usr/bin/python
> +#!/usr/bin/env python3
>  #
>  # tool for querying VMX capabilities
>  #
> @@ -275,5 +275,6 @@ controls = [
>  ),
>  ]
>  
> -for c in controls:
> -c.show()
> +if __name__ == '__main__':
> +for c in controls:
> +c.show()
> 

Acked-by: Paolo Bonzini 

Re: [PATCH 04/10] ui/gtk: remove unused code

[Philippe Mathieu-Daudé]

On 5/10/20 8:42 PM, Volker Rümelin wrote:

This code was last used before commit 2ec78706d1 "ui: convert
GTK and SDL1 frontends to keycodemapdb".

Signed-off-by: Volker Rümelin 
---
  ui/gtk.c | 9 -
  1 file changed, 9 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index 5de2a75691..c70bfc2be4 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -112,15 +112,6 @@
  # define VTE_CHECK_VERSION(a, b, c) 0
  #endif
  
-/* Some older mingw versions lack this constant or have

- * it conditionally defined */
-#ifdef _WIN32
-# ifndef MAPVK_VK_TO_VSC
-#  define MAPVK_VK_TO_VSC 0
-# endif
-#endif
-
-
  #define HOTKEY_MODIFIERS(GDK_CONTROL_MASK | GDK_MOD1_MASK)
  
  static const guint16 *keycode_map;




Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 05/10] ui/gtk: remove unused variable ignore_keys

[Philippe Mathieu-Daudé]

On 5/10/20 8:42 PM, Volker Rümelin wrote:
Since the removal of GTK2 code 


"... in commit 89d85cde7 ..."


the code around ignore_keys is
unused. See commit 1a01716a30 "gtk: Avoid accel key leakage
into guest on console switch" why it was needed before.

Signed-off-by: Volker Rümelin 


With description updated:
Reviewed-by: Philippe Mathieu-Daudé 


---
  ui/gtk.c | 9 -
  1 file changed, 9 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index c70bfc2be4..5a25e3fa4c 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -168,8 +168,6 @@ struct GtkDisplayState {
  
  bool external_pause_update;
  
-bool ignore_keys;

-
  DisplayOptions *opts;
  };
  
@@ -1085,14 +1083,8 @@ static gboolean gd_text_key_down(GtkWidget *widget,

  static gboolean gd_key_event(GtkWidget *widget, GdkEventKey *key, void 
*opaque)
  {
  VirtualConsole *vc = opaque;
-GtkDisplayState *s = vc->s;
  int qcode;
  
-if (s->ignore_keys) {

-s->ignore_keys = (key->type == GDK_KEY_PRESS);
-return TRUE;
-}
-
  #ifdef WIN32
  /* on windows, we ought to ignore the reserved key event? */
  if (key->hardware_keycode == 0xff)
@@ -1189,7 +1181,6 @@ static void gd_menu_switch_vc(GtkMenuItem *item, void 
*opaque)
  gtk_notebook_set_current_page(nb, page);
  gtk_widget_grab_focus(vc->focus);
  }
-s->ignore_keys = false;
  }
  
  static void gd_accel_switch_vc(void *opaque)

Re: [PATCH 08/10] ui/sdl2-input: use trace-events to debug key events

[Philippe Mathieu-Daudé]

On 5/10/20 8:43 PM, Volker Rümelin wrote:

Signed-off-by: Volker Rümelin 
---
  ui/sdl2-input.c | 3 +++
  ui/trace-events | 3 +++
  2 files changed, 6 insertions(+)

diff --git a/ui/sdl2-input.c b/ui/sdl2-input.c
index 1f9fe831b3..f068382209 100644
--- a/ui/sdl2-input.c
+++ b/ui/sdl2-input.c
@@ -27,6 +27,7 @@
  #include "ui/console.h"
  #include "ui/input.h"
  #include "ui/sdl2.h"
+#include "trace.h"
  
  void sdl2_process_key(struct sdl2_console *scon,

SDL_KeyboardEvent *ev)
@@ -38,6 +39,8 @@ void sdl2_process_key(struct sdl2_console *scon,
  return;
  }
  qcode = qemu_input_map_usb_to_qcode[ev->keysym.scancode];
+trace_sdl2_process_key(ev->keysym.scancode, qcode,
+   ev->type == SDL_KEYDOWN ? "down" : "up");
  qkbd_state_key_event(scon->kbd, qcode, ev->type == SDL_KEYDOWN);
  
  if (!qemu_console_is_graphic(con)) {

diff --git a/ui/trace-events b/ui/trace-events
index 0dcda393c1..5367fd3f16 100644
--- a/ui/trace-events
+++ b/ui/trace-events
@@ -75,6 +75,9 @@ input_event_abs(int conidx, const char *axis, int value) "con 
%d, axis %s, value
  input_event_sync(void) ""
  input_mouse_mode(int absolute) "absolute %d"
  
+# sdl2-input.c

+sdl2_process_key(int sdl_scancode, int qcode, const char *action) "translated SDL 
scancode %d to QKeyCode %d (%s)"
+
  # spice-display.c
  qemu_spice_add_memslot(int qid, uint32_t slot_id, unsigned long virt_start, unsigned 
long virt_end, int async) "%d %u: host virt 0x%lx - 0x%lx async=%d"
  qemu_spice_del_memslot(int qid, uint32_t gid, uint32_t slot_id) "%d gid=%u 
sid=%u"



Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH RESEND v6 01/36] memory: alloc RAM from file at offset

[Jag Raman]

> On May 12, 2020, at 4:48 AM, Daniel P. Berrangé  wrote:
> 
> On Wed, Apr 22, 2020 at 09:13:36PM -0700, elena.ufimts...@oracle.com wrote:
>> From: Jagannathan Raman 
>> 
>> Allow RAM MemoryRegion to be created from an offset in a file, instead
>> of allocating at offset of 0 by default. This is needed to synchronize
>> RAM between QEMU & remote process.
> 
> Can you elaborate on why remote processes require the RAM to be offset
> from zero ?

Hi Daniel,

As it turns out, the RAM is scattered across the physical address space
(system_memory) of QEMU. Therefore, the system memory is composed
of multiple sections of RAM, and some sections start at a non-zero RAM
offset.

As a result, the remote process needs the ability to map these RAM
sections into system_memory.

Thank you!
--
Jag

> 
> NB, I'm not objecting - I'm just curious to understand more.
> 
>> 
>> Signed-off-by: Jagannathan Raman 
>> Signed-off-by: John G Johnson 
>> Signed-off-by: Elena Ufimtseva 
>> Reviewed-by: Dr. David Alan Gilbert 
>> ---
>> exec.c| 11 +++
>> include/exec/ram_addr.h   |  2 +-
>> include/qemu/mmap-alloc.h |  3 ++-
>> memory.c  |  2 +-
>> util/mmap-alloc.c |  7 ---
>> util/oslib-posix.c|  2 +-
>> 6 files changed, 16 insertions(+), 11 deletions(-)
>> 
>> diff --git a/exec.c b/exec.c
>> index 2874bb5088..d0ac9545f4 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -1801,6 +1801,7 @@ static void *file_ram_alloc(RAMBlock *block,
>> ram_addr_t memory,
>> int fd,
>> bool truncate,
>> +off_t offset,
>> Error **errp)
>> {
>> void *area;
>> @@ -1851,7 +1852,8 @@ static void *file_ram_alloc(RAMBlock *block,
>> }
>> 
>> area = qemu_ram_mmap(fd, memory, block->mr->align,
>> - block->flags & RAM_SHARED, block->flags & 
>> RAM_PMEM);
>> + block->flags & RAM_SHARED, block->flags & RAM_PMEM,
>> + offset);
>> if (area == MAP_FAILED) {
>> error_setg_errno(errp, errno,
>>  "unable to map backing store for guest RAM");
>> @@ -2283,7 +2285,7 @@ static void ram_block_add(RAMBlock *new_block, Error 
>> **errp, bool shared)
>> #ifdef CONFIG_POSIX
>> RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
>>  uint32_t ram_flags, int fd,
>> - Error **errp)
>> + off_t offset, Error **errp)
>> {
>> RAMBlock *new_block;
>> Error *local_err = NULL;
>> @@ -2328,7 +2330,8 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, 
>> MemoryRegion *mr,
>> new_block->used_length = size;
>> new_block->max_length = size;
>> new_block->flags = ram_flags;
>> -new_block->host = file_ram_alloc(new_block, size, fd, !file_size, errp);
>> +new_block->host = file_ram_alloc(new_block, size, fd, !file_size, 
>> offset,
>> + errp);
>> if (!new_block->host) {
>> g_free(new_block);
>> return NULL;
>> @@ -2358,7 +2361,7 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, 
>> MemoryRegion *mr,
>> return NULL;
>> }
>> 
>> -block = qemu_ram_alloc_from_fd(size, mr, ram_flags, fd, errp);
>> +block = qemu_ram_alloc_from_fd(size, mr, ram_flags, fd, 0, errp);
>> if (!block) {
>> if (created) {
>> unlink(mem_path);
>> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
>> index 5e59a3d8d7..1b9f489ff0 100644
>> --- a/include/exec/ram_addr.h
>> +++ b/include/exec/ram_addr.h
>> @@ -121,7 +121,7 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, 
>> MemoryRegion *mr,
>>Error **errp);
>> RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
>>  uint32_t ram_flags, int fd,
>> - Error **errp);
>> + off_t offset, Error **errp);
>> 
>> RAMBlock *qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
>>   MemoryRegion *mr, Error **errp);
>> diff --git a/include/qemu/mmap-alloc.h b/include/qemu/mmap-alloc.h
>> index e786266b92..4f579858bc 100644
>> --- a/include/qemu/mmap-alloc.h
>> +++ b/include/qemu/mmap-alloc.h
>> @@ -25,7 +25,8 @@ void *qemu_ram_mmap(int fd,
>> size_t size,
>> size_t align,
>> bool shared,
>> -bool is_pmem);
>> +bool is_pmem,
>> +off_t start);
>> 
>> void qemu_ram_munmap(int fd, void *ptr, size_t size);
>> 
>> diff --git a/memory.c b/memory.c
>> index 601b749906..f5fec476b7 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -1596,7 +1596,7 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
>> mr->destructor = memory_

Re: [PATCH RESEND v6 11/36] multi-process: define mpqemu-link object

[Jag Raman]

> On May 12, 2020, at 4:56 AM, Stefan Hajnoczi  wrote:
> 
> On Wed, Apr 22, 2020 at 09:13:46PM -0700, elena.ufimts...@oracle.com wrote:
>> From: Jagannathan Raman 
>> 
>> Defines mpqemu-link object which forms the communication link between
>> QEMU & emulation program.
>> Adds functions to configure members of mpqemu-link object instance.
>> Adds functions to send and receive messages over the communication
>> channel.
>> Adds GMainLoop to handle events received on the communication channel.
>> 
>> Signed-off-by: Jagannathan Raman 
>> Signed-off-by: John G Johnson 
>> Signed-off-by: Elena Ufimtseva 
> 
> This will change a lot when integrated into the QEMU event loop so I've
> skipped a lot of the code.
> 
> QIOChannel is probably the appropriate object to use instead of directly
> accessing a file descriptor.

OK, got it. Thanks!

> 
>> +/**
>> + * mpqemu_cmd_t:
>> + *
>> + * proc_cmd_t enum type to specify the command to be executed on the remote
>> + * device.
>> + */
>> +typedef enum {
>> +INIT = 0,
>> +MAX,
>> +} mpqemu_cmd_t;
>> +
>> +/**
>> + * MPQemuMsg:
>> + * @cmd: The remote command
>> + * @bytestream: Indicates if the data to be shared is structured (data1)
>> + *  or unstructured (data2)
>> + * @size: Size of the data to be shared
>> + * @data1: Structured data
>> + * @fds: File descriptors to be shared with remote device
>> + * @data2: Unstructured data
>> + *
>> + * MPQemuMsg Format of the message sent to the remote device from QEMU.
>> + *
>> + */
>> +typedef struct {
>> +mpqemu_cmd_t cmd;
> 
> Please use an int field on the wire because the C standard says:
> 
>  Each enumerated type shall be compatible with char, a signed integer
>  type, or an unsigned integer type. The choice of type is
>  implementation-defined, but shall be capable of representing the
>  values of all the members of the enumeration.
> 
> So the compiler may make this a char field (which would introduce
> padding before the bytestream field) but if a new enum constant FOO =
> 0x100 is added then the compiler might change the size to 16-bit.
> 
>> +int mpqemu_msg_recv(MPQemuMsg *msg, MPQemuChannel *chan)
>> +{
>> +int rc;
>> +uint8_t *data;
>> +union {
>> +char control[CMSG_SPACE(REMOTE_MAX_FDS * sizeof(int))];
>> +struct cmsghdr align;
>> +} u;
>> +struct msghdr hdr;
>> +struct cmsghdr *chdr;
>> +size_t fdsize;
>> +int sock = chan->sock;
>> +QemuMutex *lock = &chan->recv_lock;
>> +
>> +struct iovec iov = {
>> +.iov_base = (char *) msg,
>> +.iov_len = MPQEMU_MSG_HDR_SIZE,
>> +};
>> +
>> +memset(&hdr, 0, sizeof(hdr));
>> +memset(&u, 0, sizeof(u));
>> +
>> +hdr.msg_iov = &iov;
>> +hdr.msg_iovlen = 1;
>> +hdr.msg_control = &u;
>> +hdr.msg_controllen = sizeof(u);
>> +
>> +WITH_QEMU_LOCK_GUARD(lock) {
>> +do {
>> +rc = recvmsg(sock, &hdr, 0);
>> +} while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>> +
>> +if (rc < 0) {
> 
> Missing rc != MPQEMU_MSG_HDR_SIZE check. If this was a short read we
> should not attempt to parse uninitialized bytes in msg.
> 
> This is more defensive than relying on catching bogus input values later
> on and also protects against accidentally revealing uninitialized memory
> contents by observing our error handling response.
> 
>> +qemu_log_mask(LOG_REMOTE_DEBUG, "%s - recvmsg rc is %d, "
>> +  "errno is %d, sock %d\n", __func__, rc, errno, 
>> sock);
>> +return rc;
>> +}
>> +
>> +msg->num_fds = 0;
>> +for (chdr = CMSG_FIRSTHDR(&hdr); chdr != NULL;
>> + chdr = CMSG_NXTHDR(&hdr, chdr)) {
>> +if ((chdr->cmsg_level == SOL_SOCKET) &&
>> +(chdr->cmsg_type == SCM_RIGHTS)) {
>> +fdsize = chdr->cmsg_len - CMSG_LEN(0);
>> +msg->num_fds = fdsize / sizeof(int);
>> +if (msg->num_fds > REMOTE_MAX_FDS) {
>> +qemu_log_mask(LOG_REMOTE_DEBUG,
>> +  "%s: Max FDs exceeded\n", __func__);
>> +return -ERANGE;
>> +}
>> +
>> +memcpy(msg->fds, CMSG_DATA(chdr), fdsize);
>> +break;
>> +}
>> +}
>> +
>> +if (msg->bytestream) {
>> +if (!msg->size) {
>> +qemu_mutex_unlock(lock);
> 
> Duplicate unlock, we're already inside WITH_QEMU_LOCK_GUARD().
> 
>> +return -EINVAL;
>> +}
>> +
>> +msg->data2 = calloc(1, msg->size);
> 
> What is the maximum message size? Please pick one and enforce it to
> protect against huge allocations that cause us to run out of memory.
> 
>> +data = msg->data2;
>> +} else {
>> +data = (uint8_t *)&msg->data1;
> 
> Adding a uint8_t member to the union eliminates the need for a cast:
> 
>  union {
>  uint64_t u64;
>  uint8_t u8;
>  

Re: [libcamera-devel] [virtio-dev] Re: Fwd: Qemu Support for Virtio Video V4L2 driver

[Dmitry Sepp]

Hi Laurent,

On Montag, 11. Mai 2020 16:31:36 CEST Laurent Pinchart wrote:
> 
> I don't think this would be the right level of abstraction. The V4L2 API
> is way too low-level when it comes to camera paravirtualization (and may
> not be the only API we'll have in the future). I thus recommend
> virtualizing cameras with a higher-level API, more or less on top of
> libcamera or the Android camera HAL (they both sit at the same level in
> the camera stack). Anything lower than that won't be practical.
> 

I think the the main thing to do first would be to define the logic of such 
virtio-camera device and the set of mandatory features. Host-side API is a bit 
of a side topic. But libcamera fits the best though.

Best regards,
Dmitry.

Re: [PATCH RESEND v6 15/36] multi-process: setup memory manager for remote device

[Stefan Hajnoczi]

On Wed, Apr 22, 2020 at 09:13:50PM -0700, elena.ufimts...@oracle.com wrote:
> diff --git a/exec.c b/exec.c
> index 5b1e414099..1e02e00f00 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2371,6 +2371,23 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, 
> MemoryRegion *mr,
>  
>  return block;
>  }
> +
> +void qemu_ram_init_from_fd(MemoryRegion *mr, int fd, uint64_t size,

This looks like a memory_region_init_*() function, not a qemu_ram_*()
function. Why is it being added to exec.c instead of memory.c?

> +   ram_addr_t offset, Error **errp)

qemu_ram_alloc_from_fd()'s offset argument has the off_t type. Why is
ram_addr_t used here?

> +typedef struct {
> +hwaddr gpas[REMOTE_MAX_FDS];
> +uint64_t sizes[REMOTE_MAX_FDS];
> +ram_addr_t offsets[REMOTE_MAX_FDS];

Should this be off_t because it's the file offset, not a RAMBlock
address?

> +} sync_sysmem_msg_t;

QEMU coding style would name this struct SyncSysMemMsg.

> +void remote_sysmem_reconfig(MPQemuMsg *msg, Error **errp)
> +{
> +sync_sysmem_msg_t *sysmem_info = &msg->data1.sync_sysmem;
> +MemoryRegion *sysmem, *subregion, *next;
> +Error *local_err = NULL;
> +int region;
> +
> +sysmem = get_system_memory();
> +
> +qemu_mutex_lock_iothread();
> +
> +memory_region_transaction_begin();
> +
> +QTAILQ_FOREACH_SAFE(subregion, &sysmem->subregions, subregions_link, 
> next) {
> +if (subregion->ram) {
> +memory_region_del_subregion(sysmem, subregion);
> +qemu_ram_free(subregion->ram_block);

subregion is leaked. qemu_ram_free() shouldn't be called directly. It's
normally called from the MemoryRegion->destructor function but that
wasn't set up by qemu_ram_init_from_fd().

Please check how MemoryRegion lifecycles should work, update
qemu_ram_init_from_fd(), and update this function to avoid leaks.

> +}
> +}
> +
> +for (region = 0; region < msg->num_fds; region++) {
> +subregion = g_new(MemoryRegion, 1);
> +qemu_ram_init_from_fd(subregion, msg->fds[region],
> +  sysmem_info->sizes[region],
> +  sysmem_info->offsets[region], &local_err);

Where are is msg->fds[region] closed?


signature.asc
Description: PGP signature

Re: [PATCH RESEND v6 14/36] multi-process: setup a machine object for remote device process

[Jag Raman]

> On May 12, 2020, at 6:43 AM, Stefan Hajnoczi  wrote:
> 
> On Wed, Apr 22, 2020 at 09:13:49PM -0700, elena.ufimts...@oracle.com wrote:
>> From: Jagannathan Raman 
>> 
>> remote-machine object sets up various subsystems of the remote device
>> process. Instantiate PCI host bridge object and initialize RAM, IO &
>> PCI memory regions.
>> 
>> Signed-off-by: John G Johnson 
>> Signed-off-by: Jagannathan Raman 
>> Signed-off-by: Elena Ufimtseva 
>> ---
>> MAINTAINERS   |  2 +
>> Makefile.objs |  1 +
>> exec.c|  3 +-
>> include/exec/address-spaces.h |  2 +
>> include/remote/machine.h  | 30 +
>> remote/Makefile.objs  |  2 +
>> remote/machine.c  | 84 +++
>> remote/remote-main.c  |  7 +++
> 
> Now that the separate remote emulation program is going away I think it
> makes sense to move the PCIe host and machine type into hw/:
> 
>  hw/pci-host/remote.c <-- PCIe host
>  hw/i386/remote.c <-- machine type (could be moved again later if
>   other architectures are supported)

OK, got it.

> 
>> diff --git a/exec.c b/exec.c
>> index d0ac9545f4..5b1e414099 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -161,7 +161,6 @@ typedef struct subpage_t {
>> #define PHYS_SECTION_UNASSIGNED 0
>> 
>> static void io_mem_init(void);
>> -static void memory_map_init(void);
> 
> The memory_map_init() change is unnecessary once a softmmu target is
> used since it will be called from cpu_exec_init_all().

OK.

> 
>> +static void remote_machine_init(Object *obj)
>> +{
>> +RemMachineState *s = REMOTE_MACHINE(obj);
>> +RemPCIHost *rem_host;
>> +MemoryRegion *system_memory, *system_io, *pci_memory;
>> +
>> +Error *error_abort = NULL;
>> +
>> +object_property_add_child(object_get_root(), "machine", obj, 
>> &error_abort);
>> +if (error_abort) {
> 
> error_abort aborts the program so handling it is not necessary.

OK, thanks!

> 
>> +error_report_err(error_abort);
>> +}
>> +
>> +memory_map_init();
>> +
>> +system_memory = get_system_memory();
>> +system_io = get_system_io();
>> +
>> +pci_memory = g_new(MemoryRegion, 1);
>> +memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
>> +
>> +rem_host = REMOTE_HOST_DEVICE(qdev_create(NULL, 
>> TYPE_REMOTE_HOST_DEVICE));
>> +
>> +rem_host->mr_pci_mem = pci_memory;
>> +rem_host->mr_sys_mem = system_memory;
>> +rem_host->mr_sys_io = system_io;
>> +
>> +s->host = rem_host;
>> +
>> +object_property_add_child(OBJECT(s), "remote-device", OBJECT(rem_host),
>> +  &error_abort);
>> +if (error_abort) {
> 
> error_abort aborts the program so handling it is not necessary.
> 
>> +error_report_err(error_abort);
>> +return;
>> +}
>> +
>> +qemu_mutex_lock_iothread();
> 
> This will be executed with the iothread lock held. There is no need to
> acquire it.

Yes, this wouldn’t be necessary from QEMU’s main loop.

Thanks!
--
Jag

> 
>> +memory_region_add_subregion_overlap(system_memory, 0x0, pci_memory, -1);
>> +qemu_mutex_unlock_iothread();
>> +
>> +qdev_init_nofail(DEVICE(rem_host));
>> +}

Re: [PATCH 0/2] use unsigned type for MegasasState fields

[P J P]

+-- On Thu, 7 May 2020, P J P wrote --+
| Hello,
| 
| * This series fixes an OOB access issue which may occur when a guest user
|   sets 's->reply_queue_head' field to a negative(or large positive) value,
|   via 'struct mfi_init_qinfo' object in megasas_init_firmware().
| 
| * Second patch updates other numeric fields of MegasasState to unsigned type.
| 
| Thank you.
| ---
| Prasad J Pandit (2):
|   megasas: use unsigned type for reply_queue_head
|   megasas: use unsigned type for positive numeric fields
| 
|  hw/scsi/megasas.c | 40 
|  1 file changed, 20 insertions(+), 20 deletions(-)

Ping...!
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D