Re: [PATCH v3 24/34] qapi: Replace qmp_dispatch()'s TODO comment by an explanation

[Marc-André Lureau]

Hi

On Sun, Mar 15, 2020 at 3:48 PM Markus Armbruster  wrote:
>
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/qmp-dispatch.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 112d29a9ab..fb53687ce9 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -164,7 +164,11 @@ QDict *qmp_dispatch(QmpCommandList *cmds, QObject 
> *request,
>  g_assert(!ret);
>  return NULL;
>  } else if (!ret) {
> -/* TODO turn into assertion */
> +/*
> + * When the command's schema has no 'returns', cmd->fn()
> + * leaves @ret null.  The QMP spec calls for an the empty

"for an"

Can we assert that the command's schema has no 'returns' in this case?

> + * object then; supply it.
> + */
>  ret = QOBJECT(qdict_new());
>  }
>
> --
> 2.21.1
>
>


-- 
Marc-André Lureau

[PATCH v1 07/28] gdbstub: stop passing GDBState * around and use global

[Alex Bennée]

We only have one GDBState which should be allocated at the time we
process any commands. This will make further clean-up a bit easier.

Signed-off-by: Alex Bennée 
Reviewed-by: Richard Henderson 
Reviewed-by: Damien Hedde 
Reviewed-by: Philippe Mathieu-Daudé 

---
v3
  - remove final *s paramters from function calls
v4
  - a few fixups for coding style
---
 gdbstub.c | 561 +++---
 1 file changed, 278 insertions(+), 283 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 57d6e50ddfc..7243a2f7af9 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -397,21 +397,21 @@ bool gdb_has_xml;
 /* XXX: This is not thread safe.  Do we care?  */
 static int gdbserver_fd = -1;
 
-static int get_char(GDBState *s)
+static int get_char(void)
 {
 uint8_t ch;
 int ret;
 
 for(;;) {
-ret = qemu_recv(s->fd, , 1, 0);
+ret = qemu_recv(gdbserver_state.fd, , 1, 0);
 if (ret < 0) {
 if (errno == ECONNRESET)
-s->fd = -1;
+gdbserver_state.fd = -1;
 if (errno != EINTR)
 return -1;
 } else if (ret == 0) {
-close(s->fd);
-s->fd = -1;
+close(gdbserver_state.fd);
+gdbserver_state.fd = -1;
 return -1;
 } else {
 break;
@@ -449,11 +449,11 @@ int use_gdb_syscalls(void)
 }
 
 /* Resume execution.  */
-static inline void gdb_continue(GDBState *s)
+static inline void gdb_continue(void)
 {
 
 #ifdef CONFIG_USER_ONLY
-s->running_state = 1;
+gdbserver_state.running_state = 1;
 trace_gdbstub_op_continue();
 #else
 if (!runstate_needs_reset()) {
@@ -467,7 +467,7 @@ static inline void gdb_continue(GDBState *s)
  * Resume execution, per CPU actions. For user-mode emulation it's
  * equivalent to gdb_continue.
  */
-static int gdb_continue_partial(GDBState *s, char *newstates)
+static int gdb_continue_partial(char *newstates)
 {
 CPUState *cpu;
 int res = 0;
@@ -482,7 +482,7 @@ static int gdb_continue_partial(GDBState *s, char 
*newstates)
 cpu_single_step(cpu, sstep_flags);
 }
 }
-s->running_state = 1;
+gdbserver_state.running_state = 1;
 #else
 int flag = 0;
 
@@ -520,13 +520,13 @@ static int gdb_continue_partial(GDBState *s, char 
*newstates)
 return res;
 }
 
-static void put_buffer(GDBState *s, const uint8_t *buf, int len)
+static void put_buffer(const uint8_t *buf, int len)
 {
 #ifdef CONFIG_USER_ONLY
 int ret;
 
 while (len > 0) {
-ret = send(s->fd, buf, len, 0);
+ret = send(gdbserver_state.fd, buf, len, 0);
 if (ret < 0) {
 if (errno != EINTR)
 return;
@@ -538,7 +538,7 @@ static void put_buffer(GDBState *s, const uint8_t *buf, int 
len)
 #else
 /* XXX this blocks entire thread. Rewrite to use
  * qemu_chr_fe_write and background I/O callbacks */
-qemu_chr_fe_write_all(>chr, buf, len);
+qemu_chr_fe_write_all(_state.chr, buf, len);
 #endif
 }
 
@@ -620,17 +620,18 @@ static void hexdump(const char *buf, int len,
 }
 
 /* return -1 if error, 0 if OK */
-static int put_packet_binary(GDBState *s, const char *buf, int len, bool dump)
+static int put_packet_binary(const char *buf, int len, bool dump)
 {
 int csum, i;
 uint8_t *p;
+uint8_t *ps = _state.last_packet[0];
 
 if (dump && trace_event_get_state_backends(TRACE_GDBSTUB_IO_BINARYREPLY)) {
 hexdump(buf, len, trace_gdbstub_io_binaryreply);
 }
 
 for(;;) {
-p = s->last_packet;
+p = ps;
 *(p++) = '$';
 memcpy(p, buf, len);
 p += len;
@@ -642,11 +643,11 @@ static int put_packet_binary(GDBState *s, const char 
*buf, int len, bool dump)
 *(p++) = tohex((csum >> 4) & 0xf);
 *(p++) = tohex((csum) & 0xf);
 
-s->last_packet_len = p - s->last_packet;
-put_buffer(s, (uint8_t *)s->last_packet, s->last_packet_len);
+gdbserver_state.last_packet_len = p - ps;
+put_buffer(ps, gdbserver_state.last_packet_len);
 
 #ifdef CONFIG_USER_ONLY
-i = get_char(s);
+i = get_char();
 if (i < 0)
 return -1;
 if (i == '+')
@@ -659,11 +660,11 @@ static int put_packet_binary(GDBState *s, const char 
*buf, int len, bool dump)
 }
 
 /* return -1 if error, 0 if OK */
-static int put_packet(GDBState *s, const char *buf)
+static int put_packet(const char *buf)
 {
 trace_gdbstub_io_reply(buf);
 
-return put_packet_binary(s, buf, strlen(buf), false);
+return put_packet_binary(buf, strlen(buf), false);
 }
 
 /* Encode data using the encoding for 'x' packets.  */
@@ -687,37 +688,38 @@ static int memtox(char *buf, const char *mem, int len)
 return p - buf;
 }
 
-static uint32_t gdb_get_cpu_pid(const GDBState *s, CPUState *cpu)
+static uint32_t gdb_get_cpu_pid(CPUState *cpu)
 {
 /* TODO: In user mode, we should use the task state PID */
 if (cpu->cluster_index == 

Re: [PATCH v2 2/9] hw/core/qdev-properties: Use qemu_strtoul() in set_pci_host_devaddr()

[Richard Henderson]

On 3/15/20 5:11 PM, Philippe Mathieu-Daudé wrote:
> Replace strtoul() by qemu_strtoul() so checkpatch.pl won't
> complain if we move this code later. Increase the slot and
> func local variables to unsigned long so that the range check
> isn't truncated. Remove the 'e == p' test which is done in
> check_strtox_error(), called by qemu_strtoul().
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> v2: drop the e == p test, do not do modify range check (rth)
> ---
>  hw/core/qdev-properties.c | 25 -
>  1 file changed, 12 insertions(+), 13 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [PATCH v3 05/34] tests/test-qmp-cmds: Factor out qmp_dispatch() test helpers

[Eric Blake]

On 3/15/20 9:46 AM, Markus Armbruster wrote:

Checking the value of qmp_dispatch() is repetitive.  Factor out
helpers do_qmp_dispatch() and do_qmp_dispatch_error().  Without this,
the next commit would make things even more repetitive.

Signed-off-by: Markus Armbruster 
---
  tests/test-qmp-cmds.c | 72 +--
  1 file changed, 35 insertions(+), 37 deletions(-)




+
+static void do_qmp_dispatch_error(QDict *req, bool allow_oob, ErrorClass cls)
+{
+QDict *resp;
+
+resp = qmp_dispatch(_commands, QOBJECT(req), allow_oob);
+g_assert(resp && qdict_haskey(resp, "error"));
+
+qobject_unref(resp);
+}


No checking of cls?  Or is that what you hint at as coming later?

At any rate, the refactoring is sane.
Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH V2] vhost: correctly turn on VIRTIO_F_IOMMU_PLATFORM

[Peter Xu]

On Mon, Mar 16, 2020 at 05:57:37PM +0100, Halil Pasic wrote:
> On Fri, 13 Mar 2020 12:31:22 -0400
> Peter Xu  wrote:
> 
> > On Fri, Mar 13, 2020 at 11:29:59AM -0400, Michael S. Tsirkin wrote:
> > > On Fri, Mar 13, 2020 at 01:44:46PM +0100, Halil Pasic wrote:
> > > > [..]
> > > > > > 
> > > > > > CCing Tom. @Tom does vhost-vsock work for you with SEV and current 
> > > > > > qemu?
> > > > > > 
> > > > > > Also, one can specify iommu_platform=on on a device that ain't a 
> > > > > > part of
> > > > > > a secure-capable VM, just for the fun of it. And that breaks
> > > > > > vhost-vsock. Or is setting iommu_platform=on only valid if
> > > > > > qemu-system-s390x is protected virtualization capable?
> > > > > > 
> > > > > > BTW, I don't have a strong opinion on the fixes tag. We currently 
> > > > > > do not
> > > > > > recommend setting iommu_platform, and thus I don't think we care too
> > > > > > much about past qemus having problems with it.
> > > > > > 
> > > > > > Regards,
> > > > > > Halil
> > > > > 
> > > > > 
> > > > > Let's just say if we do have a Fixes: tag we want to set it correctly 
> > > > > to
> > > > > the commit that needs this fix.
> > > > > 
> > > > 
> > > > I finally did some digging regarding the performance degradation. For
> > > > s390x the performance degradation on vhost-net was introduced by commit
> > > > 076a93d797 ("exec: simplify address_space_get_iotlb_entry"). Before
> > > > IOMMUTLBEntry.addr_mask used to be based on plen, which in turn was
> > > > calculated as the rest of the memory regions size (from address), and
> > > > covered most of the guest address space. That is we didn't have a whole
> > > > lot of IOTLB API overhead.
> > > > 
> > > > With commit 076a93d797 I see IOMMUTLBEntry.addr_mask == 0xfff which 
> > > > comes
> > > > as ~TARGET_PAGE_MASK from flatview_do_translate(). To have things 
> > > > working
> > > > properly I applied 75e5b70e6, b021d1c044, and d542800d1e on the level of
> > > > 076a93d797 and 076a93d797~1.
> > > 
> > > Peter, what's your take on this one?
> > 
> > Commit 076a93d797 was one of the patchset where we want to provide
> > sensible IOTLB entries and also that should start to work with huge
> > pages.  Frankly speaking after a few years I forgot the original
> > motivation of that whole thing, but IIRC there's a patch that was
> > trying to speedup especially for vhost but I noticed it's not merged:
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg00574.html

[1]

> > 
> 
> From the looks of it, I don't think we would have seen that big
> performance degradation had this patch been included. I can give
> it a spin if you like. Shall I?
> 
> > Regarding to the current patch, I'm not sure I understand it
> > correctly, but is that performance issue only happens when (1) there's
> > no intel-iommu device, and (2) there is iommu_platform=on specified
> > for the vhost backend?
> > 
> 
> I can confirm, that your description covers my scenario. I didn't
> investigate what happens when we have an intel-iommu, because s390 does
> not do intel-iommu. I can also confirm that no performance degradation
> is observed when the virtio-net has iommu_platform=off. The property
> iommu_platform is a virtio device (and not a backend) level property.
>  
> 
> > If so, I'd confess I am not too surprised if this fails the boot with
> > vhost-vsock because after all we speicified iommu_platform=on
> > explicitly in the cmdline, so if we want it to work we can simply
> > remove that iommu_platform=on when vhost-vsock doesn't support it
> > yet...  I thougth iommu_platform=on was added for that case - when we
> > want to force IOMMU to be enabled from host side, and it should always
> > be used with a vIOMMU device.
> > 
> 
> The problem is that the virtio feature bit F_ACCESS_PLATFORM, which is
> directly controlled by the iommu_platform proprerty stands for two things
> 1) need to do IOVA translation
> 2) the access of the device to the guests RAM is restricted.
> 
> There are cases where 2) does apply and 1) does not. We need to specify
> iommu_platform=on to make the virtio implementation in the guest use
> the dma api, because we need to grant access to memory as required. But
> we don't need translation and we don't have a vIOMMU.

I see the point of this patch now.  I'm still unclear on how s390
works for DMA protection, but it seems totally different from the
IOMMU model on x86/arm.  Considering this, please ignore above patch
[1] because that's hackish in all cases to play with iotlb caches, and
current patch should be much better (and easier) IMHO.

Thanks,

-- 
Peter Xu

Re: [PATCH v5 57/60] target/riscv: vector slide instructions

[Richard Henderson]

On 3/16/20 1:04 AM, LIU Zhiwei wrote:
>> As a preference, I think you can do away with this helper.
>> Simply use the slideup helper with argument 1, and then
>> afterwards store the integer register into element 0.  You should be able to
>> re-use code from vmv.s.x for that.
> When I try it, I find it is some difficult, because  vmv.s.x will clean
> the elements (0 < index < VLEN/SEW).

Well, two things about that:

(1) The 0.8 version of vmv.s.x does *not* zero the other elements, so we'll
want to be prepared for that.

(2) We have 8 insns that, in the end come down to a direct element access,
possibly with some other processing.

So we'll want basic helper functions that can locate an element by immediate
offset and by variable offset:

/* Compute the offset of vreg[idx] relative to cpu_env.
   The index must be in range of VLMAX. */
int vec_element_ofsi(int vreg, int idx, int sew);

/* Compute a pointer to vreg[idx].
   If need_bound is true, mask idx into VLMAX,
   Otherwise we know a-priori that idx is already in bounds. */
void vec_element_ofsx(DisasContext *s, TCGv_ptr base,
  TCGv idx, int sew, bool need_bound);

/* Load idx >= VLMAX ? 0 : vreg[idx] */
void vec_element_loadi(DisasContext *s, TCGv_i64 val,
   int vreg, int idx, int sew);
void vec_element_loadx(DisasContext *s, TCGv_i64 val,
   int vreg, TCGv idx, int sew);

/* Store vreg[imm] = val.
   The index must be in range of VLMAX.  */
void vec_element_storei(DisasContext *s, int vreg, int imm,
TCGv_i64 val);
void vec_element_storex(DisasContext *s, int vreg,
TCGv idx, TCGv_i64 val);

(3) It would be handy to have TCGv cpu_vl.

Then:

vext.x.v:
If rs1 == 0,
Use vec_element_loadi(s, x[rd], vs2, 0, s->sew).
else
Use vec_element_loadx(s, x[rd], vs2, x[rs1], true).

vmv.s.x:
over = gen_new_label();
tcg_gen_brcondi_tl(TCG_COND_EQ, cpu_vl, 0, over);
For 0.7.1:
Use tcg_gen_dup8i to zero all VLMAX elements of vd.
If rs1 == 0, goto done.
Use vec_element_storei(s, vs2, 0, x[rs1]).
 done:
gen_set_label(over);

vfmv.f.s:
Use vec_element_loadi(x, f[rd], vs2, 0).
NaN-box f[rd] as necessary for SEW.

vfmv.s.f:
tcg_gen_brcondi_tl(TCG_COND_EQ, cpu_vl, 0, over);
For 0.7.1:
Use tcg_gen_dup8i to zero all VLMAX elements of vd.
Let tmp = f[rs1], nan-boxed as necessary for SEW.
Use vec_element_storei(s, vs2, 0, tmp).
gen_set_label(over);

vslide1up.vx:
Ho hum, I forgot about masking.  Some options:
(1) Call a helper just as you did in your original patch.
(2) Call a helper only for !vm, for vm as below.
(3) Call vslideup w/1.
tcg_gen_brcondi(TCG_COND_EQ, cpu_vl, 0, over);
If !vm,
// inline test for v0[0]
vec_element_loadi(s, tmp, 0, 0, MO_8);
tcg_gen_andi_i64(tmp, tmp, 1);
tcg_gen_brcondi(TCG_COND_EQ, tmp, 0, over);
Use vec_element_store(s, vd, 0, x[rs1]).
gen_set_label(over);

vslide1down.vx:
For !vm, this is complicated enough for a helper.
If using option 3 for vslide1up, then the store becomes:
tcg_gen_subi_tl(tmp, cpu_vl, 1);
vec_element_storex(s, base, tmp, x[rs1]);

vrgather.vx:
If !vm or !vl_eq_vlmax, use helper.
vec_element_loadx(s, tmp, vs2, x[rs1]);
Use tcg_gen_gvec_dup_i64 to store to tmp to vd.

vrgather.vi:
If !vm or !vl_eq_vlmax, use helper.
If imm >= vlmax,
Use tcg_gen_dup8i to zero vd;
else,
ofs = vec_element_ofsi(s, vs2, imm, s->sew);
tcg_gen_gvec_dup_mem(sew, vreg_ofs(vd),
 ofs, vlmax, vlmax);


r~

[PATCH v3 05/25] hw/arm: Use memory_region_init_rom() with read-only regions

[Philippe Mathieu-Daudé]

This commit was produced with the Coccinelle script
scripts/coccinelle/memory-region-housekeeping.cocci.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/arm/exynos4210.c | 3 +--
 hw/arm/mainstone.c  | 3 +--
 hw/arm/omap_sx1.c   | 6 ++
 hw/arm/palm.c   | 3 +--
 hw/arm/spitz.c  | 3 +--
 hw/arm/stellaris.c  | 3 +--
 hw/arm/tosa.c   | 3 +--
 7 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index 59a27bdd68..3af6502a5e 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -311,9 +311,8 @@ static void exynos4210_realize(DeviceState *socdev, Error 
**errp)
 >chipid_mem);
 
 /* Internal ROM */
-memory_region_init_ram(>irom_mem, NULL, "exynos4210.irom",
+memory_region_init_rom(>irom_mem, NULL, "exynos4210.irom",
EXYNOS4210_IROM_SIZE, _fatal);
-memory_region_set_readonly(>irom_mem, true);
 memory_region_add_subregion(system_mem, EXYNOS4210_IROM_BASE_ADDR,
 >irom_mem);
 /* mirror of iROM */
diff --git a/hw/arm/mainstone.c b/hw/arm/mainstone.c
index 1042017086..6bc643651b 100644
--- a/hw/arm/mainstone.c
+++ b/hw/arm/mainstone.c
@@ -124,9 +124,8 @@ static void mainstone_common_init(MemoryRegion 
*address_space_mem,
 /* Setup CPU & memory */
 mpu = pxa270_init(address_space_mem, mainstone_binfo.ram_size,
   machine->cpu_type);
-memory_region_init_ram(rom, NULL, "mainstone.rom", MAINSTONE_ROM,
+memory_region_init_rom(rom, NULL, "mainstone.rom", MAINSTONE_ROM,
_fatal);
-memory_region_set_readonly(rom, true);
 memory_region_add_subregion(address_space_mem, 0, rom);
 
 /* There are two 32MiB flash devices on the board */
diff --git a/hw/arm/omap_sx1.c b/hw/arm/omap_sx1.c
index de5ff447dc..57829b3744 100644
--- a/hw/arm/omap_sx1.c
+++ b/hw/arm/omap_sx1.c
@@ -131,9 +131,8 @@ static void sx1_init(MachineState *machine, const int 
version)
 mpu = omap310_mpu_init(machine->ram, machine->cpu_type);
 
 /* External Flash (EMIFS) */
-memory_region_init_ram(flash, NULL, "omap_sx1.flash0-0", flash_size,
+memory_region_init_rom(flash, NULL, "omap_sx1.flash0-0", flash_size,
_fatal);
-memory_region_set_readonly(flash, true);
 memory_region_add_subregion(address_space, OMAP_CS0_BASE, flash);
 
 memory_region_init_io([0], NULL, _ops, ,
@@ -167,9 +166,8 @@ static void sx1_init(MachineState *machine, const int 
version)
 if ((version == 1) &&
 (dinfo = drive_get(IF_PFLASH, 0, fl_idx)) != NULL) {
 MemoryRegion *flash_1 = g_new(MemoryRegion, 1);
-memory_region_init_ram(flash_1, NULL, "omap_sx1.flash1-0",
+memory_region_init_rom(flash_1, NULL, "omap_sx1.flash1-0",
flash1_size, _fatal);
-memory_region_set_readonly(flash_1, true);
 memory_region_add_subregion(address_space, OMAP_CS1_BASE, flash_1);
 
 memory_region_init_io([1], NULL, _ops, ,
diff --git a/hw/arm/palm.c b/hw/arm/palm.c
index 99554bda19..97ca105d29 100644
--- a/hw/arm/palm.c
+++ b/hw/arm/palm.c
@@ -213,9 +213,8 @@ static void palmte_init(MachineState *machine)
 mpu = omap310_mpu_init(machine->ram, machine->cpu_type);
 
 /* External Flash (EMIFS) */
-memory_region_init_ram(flash, NULL, "palmte.flash", flash_size,
+memory_region_init_rom(flash, NULL, "palmte.flash", flash_size,
_fatal);
-memory_region_set_readonly(flash, true);
 memory_region_add_subregion(address_space_mem, OMAP_CS0_BASE, flash);
 
 memory_region_init_io([0], NULL, _ops, , "palmte-cs0",
diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index cbfa6934cf..c28d9b5ed7 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -929,8 +929,7 @@ static void spitz_common_init(MachineState *machine,
 
 sl_flash_register(mpu, (model == spitz) ? FLASH_128M : FLASH_1024M);
 
-memory_region_init_ram(rom, NULL, "spitz.rom", SPITZ_ROM, _fatal);
-memory_region_set_readonly(rom, true);
+memory_region_init_rom(rom, NULL, "spitz.rom", SPITZ_ROM, _fatal);
 memory_region_add_subregion(address_space_mem, 0, rom);
 
 /* Setup peripherals */
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 221a78674e..d136ba1a92 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -1300,9 +1300,8 @@ static void stellaris_init(MachineState *ms, 
stellaris_board_info *board)
 sram_size = ((board->dc0 >> 18) + 1) * 1024;
 
 /* Flash programming is done via the SCU, so pretend it is ROM.  */
-memory_region_init_ram(flash, NULL, "stellaris.flash", flash_size,
+memory_region_init_rom(flash, NULL, "stellaris.flash", flash_size,
_fatal);
-memory_region_set_readonly(flash, true);
 memory_region_add_subregion(system_memory, 0, flash);
 
 memory_region_init_ram(sram, NULL, "stellaris.sram", sram_size,
diff --git 

Re: [PATCH v3 23/34] qapi: Simplify how qmp_dispatch() gets the request ID

[Marc-André Lureau]

On Sun, Mar 15, 2020 at 3:51 PM Markus Armbruster  wrote:
>
> We convert the request object to a QDict twice: first in
> qmp_dispatch() to get the request ID, and then again in
> qmp_dispatch_check_obj(), which converts to QDict, then checks and
> returns it.  We can't get the request ID from the latter, because it's
> null when the qdict flunks the checks.
>
> Move getting the request ID into qmp_dispatch_check_obj().
>

I don't see this is a an improvement. qmp_dispatch_check_obj() doesn't
care about id.

And it doesn't look like it is saving cycles either.

Is that worth it?


Code change is ok otherwise,

> Signed-off-by: Markus Armbruster 
> ---
>  qapi/qmp-dispatch.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 550d1fe8d2..112d29a9ab 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -20,7 +20,7 @@
>  #include "qapi/qmp/qbool.h"
>
>  static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob,
> - Error **errp)
> + QObject **id, Error **errp)
>  {
>  const char *exec_key = NULL;
>  const QDictEntry *ent;
> @@ -30,10 +30,13 @@ static QDict *qmp_dispatch_check_obj(const QObject 
> *request, bool allow_oob,
>
>  dict = qobject_to(QDict, request);
>  if (!dict) {
> +*id = NULL;
>  error_setg(errp, "QMP input must be a JSON object");
>  return NULL;
>  }
>
> +*id = qdict_get(dict, "id");
> +
>  for (ent = qdict_first(dict); ent;
>   ent = qdict_next(dict, ent)) {
>  arg_name = qdict_entry_key(ent);
> @@ -103,12 +106,12 @@ QDict *qmp_dispatch(QmpCommandList *cmds, QObject 
> *request,
>  const char *command;
>  QDict *args;
>  QmpCommand *cmd;
> -QDict *dict = qobject_to(QDict, request);
> -QObject *id = dict ? qdict_get(dict, "id") : NULL;
> +QDict *dict;
> +QObject *id;
>  QObject *ret = NULL;
>  QDict *rsp = NULL;
>
> -dict = qmp_dispatch_check_obj(request, allow_oob, );
> +dict = qmp_dispatch_check_obj(request, allow_oob, , );
>  if (!dict) {
>  goto out;
>  }
> --
> 2.21.1
>
>


-- 
Marc-André Lureau

Re: [PATCH v3 11/34] qapi/schema: Clean up around QAPISchemaEntity.connect_doc()

[Eric Blake]

On 3/15/20 9:46 AM, Markus Armbruster wrote:

QAPISchemaEntity calls doc.connect_feature() in .check().  Improper
since commit ee1e6a1f6c8 split .connect_doc() off .check().  Move the
call.  Requires making the children call super().connect_doc() as they
should.

Signed-off-by: Markus Armbruster 
---
  scripts/qapi/schema.py | 13 +
  1 file changed, 9 insertions(+), 4 deletions(-)



Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 6/8] target/ppc: allow ppc_cpu_do_system_reset to take an alternate vector

[Cédric Le Goater]

On 3/16/20 3:26 PM, Nicholas Piggin wrote:
> Provide for an alternate delivery location, -1 defaults to the
> architected address.

I don't know what is the best approach, to override the vector addr
computed by powerpc_excp() or use a machine class handler with 
cpu->vhyp.

> Signed-off-by: Nicholas Piggin 
> ---
>  hw/ppc/spapr.c   | 2 +-
>  target/ppc/cpu.h | 2 +-
>  target/ppc/excp_helper.c | 5 -
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5f93c49706..25221d843c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3400,7 +3400,7 @@ static void spapr_machine_finalizefn(Object *obj)
>  void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg)
>  {
>  cpu_synchronize_state(cs);
> -ppc_cpu_do_system_reset(cs);
> +ppc_cpu_do_system_reset(cs, -1);
>  }
>  
>  static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 3953680534..f8c7d6f19c 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1220,7 +1220,7 @@ int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, 
> CPUState *cs,
>  int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> int cpuid, void *opaque);
>  #ifndef CONFIG_USER_ONLY
> -void ppc_cpu_do_system_reset(CPUState *cs);
> +void ppc_cpu_do_system_reset(CPUState *cs, target_ulong vector);
>  void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector);
>  extern const VMStateDescription vmstate_ppc_cpu;
>  #endif
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 7f2b5899d3..08bc885ca6 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -961,12 +961,15 @@ static void ppc_hw_interrupt(CPUPPCState *env)
>  }
>  }
>  
> -void ppc_cpu_do_system_reset(CPUState *cs)
> +void ppc_cpu_do_system_reset(CPUState *cs, target_ulong vector)
>  {
>  PowerPCCPU *cpu = POWERPC_CPU(cs);
>  CPUPPCState *env = >env;
>  
>  powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_RESET);
> +if (vector != -1) {
> +env->nip = vector;
> +}
>  }
>  
>  void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector)
> 

Re: [PATCH v2 2/2] mmap-alloc: Include osdep.h before checking CONFIG_LINUX

[Eduardo Habkost]

On Sun, Mar 15, 2020 at 05:15:46PM -0400, Michael S. Tsirkin wrote:
> On Sun, Mar 15, 2020 at 11:45:59AM -0400, Eduardo Habkost wrote:
> > On Wed, Mar 11, 2020 at 07:23:42PM -0400, Eduardo Habkost wrote:
> > > The CONFIG_LINUX check at the top of mmap-alloc.c never worked
> > > because it was done before including osdep.h.
> > > 
> > > This means MAP_SYNC and MAP_SHARED_VALIDATE would always be set
> > > to 0 at the beginning of the file.  Luckily, this didn't break
> > > when using recent glibc versions (2.28+), because those macros
> > > were redefined by glibc headers.
> > > 
> > > Move the CONFIG_LINUX check after the main include lines, so the
> > > CONFIG_LINUX check works and we actually include .
> > > This will make MAP_SYNC and MAP_SHARED_VALIDATE available even if
> > > the host has an older glibc version.
> 
> Wait a second, MAP_SHARED_VALIDATE is from
> linux-headers/linux/mman.h - it's available on all architectures.

Yes, but both MAP_SYNC and MAP_SHARED_VALIDATE aren't available
if the host is not Linux.

> 
> > > 
> > > Reported-by: Jingqi Liu 
> > > Reviewed-by: Michael S. Tsirkin 
> > > Signed-off-by: Eduardo Habkost 
> > > ---
> > > Changes v1 -> v2:
> > > * (none)
> > > ---
> > >  util/mmap-alloc.c | 7 +++
> > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
> > > index 27dcccd8ec..7c2ce98eb0 100644
> > > --- a/util/mmap-alloc.c
> > > +++ b/util/mmap-alloc.c
> > > @@ -9,6 +9,9 @@
> > >   * 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 "qemu/osdep.h"
> > > +#include "qemu/mmap-alloc.h"
> > > +#include "qemu/host-utils.h"
> > >  
> > >  #ifdef CONFIG_LINUX
> > >  #include 
> > 
> > This breaks the build on mips, because mips doesn't have MAP_SYNC
> > defined at linux/mman.h:
> > 
> > https://app.shippable.com/github/ehabkost/qemu-hacks/runs/9/9/console
> 
> 
> Oops. But that in fact means it's currently building on mips but not
> working correctly there! MAP_SHARED_VALIDATE 0x0 is especially
> problematic. I'm unsure what's the right thing to do is,
> I guess as a first step we can go back and device MAP_SYNC to 0,

Defining MAP_SYNC to 0 on MIPS would restore the existing
behavior, so it seems like a reasonable step to fix the build
failure.  But not even printing a warning when the host doesn't
have MAP_SYNC (the existing behavior on MIPS and non-Linux) seems
wrong.

-- 
Eduardo

Re: [PATCH v2 7/8] ppc/spapr: Implement FWNMI System Reset delivery

[Greg Kurz]

On Mon, 16 Mar 2020 23:05:00 +0530
Mahesh J Salgaonkar  wrote:

> On 2020-03-17 00:26:12 Tue, Nicholas Piggin wrote:
> > PAPR requires that if "ibm,nmi-register" succeeds, then the hypervisor
> > delivers all system reset and machine check exceptions to the registered
> > addresses.
> > 
> > System Resets are delivered with registers set to the architected state,
> > and with no interlock.
> > 
> > Signed-off-by: Nicholas Piggin 
> > ---
> >  hw/ppc/spapr.c | 46 --
> >  1 file changed, 44 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 25221d843c..78e649f47d 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -967,7 +967,29 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, 
> > void *fdt)
> >  _FDT(fdt_setprop(fdt, rtas, "ibm,max-associativity-domains",
> >   maxdomains, sizeof(maxdomains)));
> > 
> > -_FDT(fdt_setprop_cell(fdt, rtas, "rtas-size", RTAS_SIZE));
> > +/*
> > + * FWNMI reserves RTAS_ERROR_LOG_MAX for the machine check error log,
> > + * and 16 bytes per CPU for system reset error log plus an extra 8 
> > bytes.
> > + *
> > + * The system reset requirements are driven by existing Linux and 
> > PowerVM
> > + * implementation which (contrary to PAPR) saves r3 in the error log
> > + * structure like machine check, so Linux expects to find the saved r3
> > + * value at the address in r3 upon FWNMI-enabled sreset interrupt (and
> > + * does not look at the error value).
> > + *
> > + * System reset interrupts are not subject to interlock like machine
> > + * check, so this memory area could be corrupted if the sreset is
> > + * interrupted by a machine check (or vice versa) if it was shared. To
> > + * prevent this, system reset uses per-CPU areas for the sreset save
> > + * area. A system reset that interrupts a system reset handler could
> > + * still overwrite this area, but Linux doesn't try to recover in that
> > + * case anyway.
> > + *
> > + * The extra 8 bytes is required because Linux's FWNMI error log check
> > + * is off-by-one.
> > + */
> > +_FDT(fdt_setprop_cell(fdt, rtas, "rtas-size", RTAS_ERROR_LOG_MAX +
> > + ms->smp.max_cpus * sizeof(uint64_t)*2 + 
> > sizeof(uint64_t)));
> 
> Currently the rtas region is only of size 2048 (i.e RTAS_ERROR_LOG_MAX).
> Do we need SLOF change to increase rtas area as well ? Otherwise QEMU
> may corrupt guest memory area OR Am I wrong ?
> 

A change is pending for SLOF to use the "rtas-size" property
provided by QEMU:

https://patchwork.ozlabs.org/patch/1255264/

> Thanks,
> -Mahesh/
> 
> >  _FDT(fdt_setprop_cell(fdt, rtas, "rtas-error-log-max",
> >RTAS_ERROR_LOG_MAX));
> >  _FDT(fdt_setprop_cell(fdt, rtas, "rtas-event-scan-rate",
> > @@ -3399,8 +3421,28 @@ static void spapr_machine_finalizefn(Object *obj)
> > 
> >  void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg)
> >  {
> > +SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > +
> >  cpu_synchronize_state(cs);
> > -ppc_cpu_do_system_reset(cs, -1);
> > +/* If FWNMI is inactive, addr will be -1, which will deliver to 0x100 
> > */
> > +if (spapr->fwnmi_system_reset_addr != -1) {
> > +uint64_t rtas_addr, addr;
> > +PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +CPUPPCState *env = >env;
> > +
> > +/* get rtas addr from fdt */
> > +rtas_addr = spapr_get_rtas_addr();
> > +if (!rtas_addr) {
> > +qemu_system_guest_panicked(NULL);
> > +return;
> > +}
> > +
> > +addr = rtas_addr + RTAS_ERROR_LOG_MAX + cs->cpu_index * 
> > sizeof(uint64_t)*2;
> > +stq_be_phys(_space_memory, addr, env->gpr[3]);
> > +stq_be_phys(_space_memory, addr + sizeof(uint64_t), 0);
> > +env->gpr[3] = addr;
> > +}
> > +ppc_cpu_do_system_reset(cs, spapr->fwnmi_system_reset_addr);
> >  }
> > 
> >  static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
> > -- 
> > 2.23.0
> > 
> > 
> 

Re: [PULL 0/1] Vga 20200316 patches

[Peter Maydell]

On Mon, 16 Mar 2020 at 14:26, Gerd Hoffmann  wrote:
>
> The following changes since commit 61c265f0660ee476985808c8aa7915617c44fd53:
>
>   Merge remote-tracking branch 
> 'remotes/dgilbert/tags/pull-migration-20200313a' into staging (2020-03-13 
> 10:33:04 +)
>
> are available in the Git repository at:
>
>   git://git.kraxel.org/qemu tags/vga-20200316-pull-request
>
> for you to fetch changes up to f872c76296b991fde4db5fb87a1cfbd8d4c22c88:
>
>   stdvga+bochs-display: add dummy mmio handler (2020-03-16 12:40:47 +0100)
>
> 
> vga: stdvga/bochs mmio fix.
>
> 
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.0
for any user-visible changes.

-- PMM

Re: [PATCH v9] fixup! Fix subcode/pbt

[Cornelia Huck]

On Mon, 16 Mar 2020 16:05:03 +0100
Janosch Frank  wrote:

> On 3/16/20 3:54 PM, Cornelia Huck wrote:
> > On Mon, 16 Mar 2020 15:47:41 +0100
> > Janosch Frank  wrote:
> >   
> >> On 3/16/20 3:27 PM, Cornelia Huck wrote:  
> >>> On Fri, 13 Mar 2020 05:52:32 -0400
> >>> Janosch Frank  wrote:
> >>> 
>  Signed-off-by: Janosch Frank 
>  ---
>   hw/s390x/ipl.h  | 11 +++
>   target/s390x/diag.c |  2 +-
>   2 files changed, 8 insertions(+), 5 deletions(-)  
> > 
> >   
>  @@ -118,7 +118,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t 
>  r1, uint64_t r3, uintptr_t ra)
>   
>   cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
>   
>  -if (!iplb_valid(iplb)) {
>  +if (!iplb_valid(iplb, subcode)) {
>   env->regs[r1 + 1] = DIAG_308_RC_INVALID;
>   goto out;
>   }
> >>>
> >>> ...because you're basically checking whether you either have a valid
> >>> normal iplb, or a valid pv iplb, with the two being mutually exclusive,
> >>> IIUC. So what about introducing iplb_valid_pv and calling that for the
> >>> pv case? Would be a bit nicer to read, I think, and also matches what
> >>> you do for the STORE case.
> >>> 
> >>S390_IPL_TYPE_CCW
> >> The idea was to get rid of all of these ifs and elses and only have one
> >> iplb_valid function. Your suggestion would defeat hiding that complexity
> >> behind this function.  
> > 
> > I'd argue that this is a complexity we should not hide; for non-pv, we
> > can have several formats, for pv, only one, and we cannot use a pv iplb
> > in a non-pv context and vice versa.
> >   
> 
> Ok, then please let me split this out into a new function within diag.c.
> Something like:
> 
> static bool diag308_pbt_subcode_validity(uint8_t pbt, uint64_t subcode)
> {
>   if (subcode == DIAG308_SET) {
>   return (pbt == S390_IPL_TYPE_FCP || pbt == S390_IPL_TYPE_CCW)
>   } else if (subcode == DIAG308_PV_SET && pbt == S390_IPL_TYPE_PV) {
>   return true;
> }
> 
>   return false;
> }
> 

Sorry, you now managed to confuse me... where is that supposed to be
called?


pgpKf7qP4f23z.pgp
Description: OpenPGP digital signature

Re: [PATCH v2 5/8] ppc/spapr: Allow FWNMI on TCG

[Cédric Le Goater]

On 3/16/20 3:26 PM, Nicholas Piggin wrote:
> There should no longer be a reason to prevent TCG providing FWNMI.
> System Reset interrupts are generated to the guest with nmi monitor
> command and H_SIGNAL_SYS_RESET. Machine Checks can not be injected
> currently, but this could be implemented with the mce monitor cmd
> similarly to i386.
> 
> Signed-off-by: Nicholas Piggin 

Reviewed-by: Cédric Le Goater 

> ---
>  hw/ppc/spapr_caps.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index f626d769a0..679ae7959f 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -516,10 +516,7 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, 
> uint8_t val,
>  return; /* Disabled by default */
>  }
>  
> -if (tcg_enabled()) {
> -warn_report("Firmware Assisted Non-Maskable Interrupts(FWNMI) not "
> -"supported in TCG");
> -} else if (kvm_enabled()) {
> +if (kvm_enabled()) {
>  if (kvmppc_set_fwnmi() < 0) {
>  error_setg(errp, "Firmware Assisted Non-Maskable 
> Interrupts(FWNMI) "
>   "not supported by KVM");
> 

Re: [PATCH 2/2] block/qcow2: zero data_file child after free

[John Snow]

On 3/16/20 2:06 AM, Vladimir Sementsov-Ogievskiy wrote:
> data_file being NULL doesn't seem to be a correct state, but it's
> better than dead pointer and simpler to debug.
> 

How important is it to have correct state in the middle of teardown?

> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/qcow2.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index d44b45633d..6cdefe059f 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1758,6 +1758,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
> *bs, QDict *options,
>  g_free(s->image_data_file);
>  if (has_data_file(bs)) {
>  bdrv_unref_child(bs, s->data_file);
> +s->data_file = NULL;
>  }

Probably OK to set to NULL, since this is at the end of a failed open
where it would have been set for the first time anyway.

It's an invalid state, but resulting from a failed call. I think that's OK.

(Are there any callers of bdrv_open or qcow2_open that don't just
immediately trash this object if it failed? I don't know of any, but
there's a lot of callers to bdrv_open.)

>  g_free(s->unknown_header_fields);
>  cleanup_unknown_header_ext(bs);
> @@ -2621,6 +2622,7 @@ static void qcow2_close(BlockDriverState *bs)
>  
>  if (has_data_file(bs)) {
>  bdrv_unref_child(bs, s->data_file);
> +s->data_file = NULL;
>  }
>  

Probably fine here too. I can't imagine it's valid to use this object
after close() ... unless we open it again, and that should handle
setting this back to a realistic value.

>  qcow2_refcount_close(bs);
> 



So I think this is fine? If I understand right this just makes failures
more obvious if we do accidentally use this value after a failed open or
close, so that seems fine.

Reviewed-by: John Snow 


(As always, I'll rely on block maintainers to do more serious structural
review for cases I am not aware of)

Re: [PATCH v3 2/2] net: tulip: add .can_recieve routine

[P J P]

  Hello Stefan, Jason,

+-- On Fri, 6 Mar 2020, Stefan Hajnoczi wrote --+
| > +static int
| > +tulip_can_receive(NetClientState *nc)
| > +{
| > +TULIPState *s = qemu_get_nic_opaque(nc);
| > +
| > +if (s->rx_frame_len || tulip_rx_stopped(s)) {
| > +return false;
| > +}
| > +
| > +return true;
| > +}
| 
| Are the required qemu_flush_queued_packets() calls in place so that
| packet transfer wakes up again when .can_receive() transitions from
| false to true?

  Yes, qemu_flush_queued_packets() calls are in tulip_write(). Do we need to 
call tulip_can_receive() before each call?

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

Re: [PATCH 11/11] hw/semihosting: Make the feature depend of TCG, and allow to disable it

[Richard Henderson]

On 3/16/20 5:00 AM, Philippe Mathieu-Daudé wrote:
> The semihosting feature is only meaningful when using TCG.
> 
> So far only the ARM/MIPS/LM32 provide the semihosting feature.
> 
> Do not enable it by default, and let the few targets requiring
> it to manually select it.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/semihosting/Kconfig | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson 

r~

Re: [PATCH v3 12/34] qapi: Add feature flags to remaining definitions

[Eric Blake]

On 3/15/20 9:46 AM, Markus Armbruster wrote:

In v4.1.0, we added feature flags just to struct types (commit
6a8c0b5102^..f3ed93d545), to satisfy an immediate need (commit
c9d4070991 "file-posix: Add dynamic-auto-read-only QAPI feature").  In
v4.2.0, we added them to commands (commit 23394b4c39 "qapi: Add
feature flags to commands") to satisfy another immediate need (commit
d76744e65e "qapi: Allow introspecting fix for savevm's cooperation
with blockdev").

Add them to the remaining definitions: enumeration types, union types,
alternate types, and events.

Signed-off-by: Markus Armbruster 
---




+++ b/qapi/introspect.json
@@ -89,12 +89,18 @@
  #
  # @meta-type: the entity's meta type, inherited from @base.
  #
+# @features: names of features associated with the entity, in no
+#particular order.
+#(since 4.1 for object types, 4.2 for commands, 5.0 for
+#the rest)


Odd versioning hint, but accurate, and I don't see any way to improve it.


+#
  # Additional members depend on the value of @meta-type.
  #
  # Since: 2.5
  ##
  { 'union': 'SchemaInfo',
-  'base': { 'name': 'str', 'meta-type': 'SchemaMetaType' },
+  'base': { 'name': 'str', 'meta-type': 'SchemaMetaType',
+'*features': [ 'str' ] },
'discriminator': 'meta-type',
'data': {
'builtin': 'SchemaInfoBuiltin',
@@ -174,9 +180,6 @@
  #and may even differ from the order of the values of the
  #enum type of the @tag.
  #
-# @features: names of features associated with the type, in no particular
-#order. (since: 4.1)
-#
  # Values of this type are JSON object on the wire.
  #
  # Since: 2.5
@@ -184,8 +187,7 @@
  { 'struct': 'SchemaInfoObject',
'data': { 'members': [ 'SchemaInfoObjectMember' ],
  '*tag': 'str',
-'*variants': [ 'SchemaInfoObjectVariant' ],
-'*features': [ 'str' ] } }
+'*variants': [ 'SchemaInfoObjectVariant' ] } }


The code motion from use in some of the union branches to now being 
present in the base class of all of the branches is backwards-compatible.


The generator changes also look correct, and have enough testsuite 
coverage to make it easier to be confident about the patch.


Reviewed-by: Eric Blake 



+++ b/tests/qapi-schema/doc-good.json
@@ -53,10 +53,14 @@
  # @Enum:
  # @one: The _one_ {and only}
  #
+# Features:
+# @enum-feat: Also _one_ {and only}


All our existing public features are a single word (matching naming 
conventions elsewhere in QAPI).  Are we sure we want to allow feature 
names that include whitespace?  Of course, the fact that our testsuite 
covers it (even if we don't use it publically) means that we are sure 
that our generator can handle it, regardless of whether we decide that a 
separate patch should restrict feature names.  But I don't see it 
holding up this patch.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH v3 07/25] hw/m68k: Use memory_region_init_rom() with read-only regions

[Philippe Mathieu-Daudé]

This commit was produced with the Coccinelle script
scripts/coccinelle/memory-region-housekeeping.cocci.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/m68k/q800.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index c5699f6f3e..81749e7ec6 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -399,13 +399,12 @@ static void q800_init(MachineState *machine)
 uint8_t *ptr;
 /* allocate and load BIOS */
 rom = g_malloc(sizeof(*rom));
-memory_region_init_ram(rom, NULL, "m68k_mac.rom", MACROM_SIZE,
+memory_region_init_rom(rom, NULL, "m68k_mac.rom", MACROM_SIZE,
_abort);
 if (bios_name == NULL) {
 bios_name = MACROM_FILENAME;
 }
 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
-memory_region_set_readonly(rom, true);
 memory_region_add_subregion(get_system_memory(), MACROM_ADDR, rom);
 
 /* Load MacROM binary */
-- 
2.21.1

[PATCH v3 04/25] scripts/cocci: Patch to replace memory_region_init_{ram, readonly -> rom}

[Philippe Mathieu-Daudé]

Add a semantic patch to replace memory_region_init_ram(readonly)
by memory_region_init_rom().

Signed-off-by: Philippe Mathieu-Daudé 
---
 .../memory-region-housekeeping.cocci   | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/scripts/coccinelle/memory-region-housekeeping.cocci 
b/scripts/coccinelle/memory-region-housekeeping.cocci
index 3699c1017e..ee3923d369 100644
--- a/scripts/coccinelle/memory-region-housekeeping.cocci
+++ b/scripts/coccinelle/memory-region-housekeeping.cocci
@@ -11,6 +11,24 @@
 */
 
 
+// Replace memory_region_init_ram(readonly) by memory_region_init_rom()
+@@
+expression E1, E2, E3, E4, E5;
+symbol true;
+@@
+(
+- memory_region_init_ram(E1, E2, E3, E4, E5);
++ memory_region_init_rom(E1, E2, E3, E4, E5);
+  ... WHEN != E1
+- memory_region_set_readonly(E1, true);
+|
+- memory_region_init_ram_nomigrate(E1, E2, E3, E4, E5);
++ memory_region_init_rom_nomigrate(E1, E2, E3, E4, E5);
+  ... WHEN != E1
+- memory_region_set_readonly(E1, true);
+)
+
+
 // Replace by-hand memory_region_init_ram_nomigrate/vmstate_register_ram
 // code sequences with use of the new memory_region_init_ram function.
 // Similarly for the _rom and _rom_device functions.
-- 
2.21.1

Re: [PATCH 06/11] accel/Kconfig: Extract accel selectors into their own config

[Richard Henderson]

On 3/16/20 5:00 AM, Philippe Mathieu-Daudé wrote:
> Move the accel selectors from the global Kconfig.host to their
> own Kconfig file.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  Makefile  | 1 +
>  Kconfig.host  | 7 ---
>  accel/Kconfig | 6 ++
>  3 files changed, 7 insertions(+), 7 deletions(-)
>  create mode 100644 accel/Kconfig

Reviewed-by: Richard Henderson 

r~

Re: [PATCH v9] fixup! Fix subcode/pbt

[Cornelia Huck]

On Mon, 16 Mar 2020 16:04:00 +0100
Christian Borntraeger  wrote:

> On 16.03.20 15:54, Cornelia Huck wrote:
> > On Mon, 16 Mar 2020 15:47:41 +0100
> > Janosch Frank  wrote:
> >   
> >> On 3/16/20 3:27 PM, Cornelia Huck wrote:  
> >>> On Fri, 13 Mar 2020 05:52:32 -0400
> >>> Janosch Frank  wrote:
> >>> 
>  Signed-off-by: Janosch Frank 
>  ---
>   hw/s390x/ipl.h  | 11 +++
>   target/s390x/diag.c |  2 +-
>   2 files changed, 8 insertions(+), 5 deletions(-)  
> > 
> >   
>  @@ -118,7 +118,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t 
>  r1, uint64_t r3, uintptr_t ra)
>   
>   cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
>   
>  -if (!iplb_valid(iplb)) {
>  +if (!iplb_valid(iplb, subcode)) {
>   env->regs[r1 + 1] = DIAG_308_RC_INVALID;
>   goto out;
>   }
> >>>
> >>> ...because you're basically checking whether you either have a valid
> >>> normal iplb, or a valid pv iplb, with the two being mutually exclusive,
> >>> IIUC. So what about introducing iplb_valid_pv and calling that for the
> >>> pv case? Would be a bit nicer to read, I think, and also matches what
> >>> you do for the STORE case.
> >>> 
> >>
> >> The idea was to get rid of all of these ifs and elses and only have one
> >> iplb_valid function. Your suggestion would defeat hiding that complexity
> >> behind this function.  
> > 
> > I'd argue that this is a complexity we should not hide; for non-pv, we
> > can have several formats, for pv, only one, and we cannot use a pv iplb
> > in a non-pv context and vice versa.  
> 
> So you suggest to split these case statements?
> case DIAG308_STORE:
> case DIAG308_PV_STORE:

Why? Those cases are already done in the way I suggest for these here
as well (i.e. keep common checks, just split the iplb handling.)

Re: [PATCH 08/11] target/Kconfig: Allow targets to use Kconfig

[Richard Henderson]

On 3/16/20 5:00 AM, Philippe Mathieu-Daudé wrote:
> diff --git a/target/Kconfig b/target/Kconfig
> new file mode 100644
> index 00..8b13789179
> --- /dev/null
> +++ b/target/Kconfig
> @@ -0,0 +1 @@
> +

Does this want a

# This file intentionally left blank.

?

Otherwise,
Reviewed-by: Richard Henderson 

r~

[PATCH v3 10/25] hw/ppc: Use memory_region_init_rom() with read-only regions

[Philippe Mathieu-Daudé]

This commit was produced with the Coccinelle script
scripts/coccinelle/memory-region-housekeeping.cocci.

Acked-by: David Gibson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/ppc/mac_newworld.c | 3 +--
 hw/ppc/mac_oldworld.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index b8189bf7a4..b2ec372958 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -155,13 +155,12 @@ static void ppc_core99_init(MachineState *machine)
 memory_region_add_subregion(get_system_memory(), 0, machine->ram);
 
 /* allocate and load BIOS */
-memory_region_init_ram(bios, NULL, "ppc_core99.bios", BIOS_SIZE,
+memory_region_init_rom(bios, NULL, "ppc_core99.bios", BIOS_SIZE,
_fatal);
 
 if (bios_name == NULL)
 bios_name = PROM_FILENAME;
 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
-memory_region_set_readonly(bios, true);
 memory_region_add_subregion(get_system_memory(), PROM_ADDR, bios);
 
 /* Load OpenBIOS (ELF) */
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 440c406eb4..faaa165f3f 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -129,13 +129,12 @@ static void ppc_heathrow_init(MachineState *machine)
 memory_region_add_subregion(sysmem, 0, machine->ram);
 
 /* allocate and load BIOS */
-memory_region_init_ram(bios, NULL, "ppc_heathrow.bios", BIOS_SIZE,
+memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", BIOS_SIZE,
_fatal);
 
 if (bios_name == NULL)
 bios_name = PROM_FILENAME;
 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
-memory_region_set_readonly(bios, true);
 memory_region_add_subregion(sysmem, PROM_ADDR, bios);
 
 /* Load OpenBIOS (ELF) */
-- 
2.21.1

[PULL 07/61] WHPX: TSC get and set should be dependent on VM state

[Paolo Bonzini]

From: Sunil Muthuswamy 

Currently, TSC is set as part of the VM runtime state. Setting TSC at
runtime is heavy and additionally can have side effects on the guest,
which are not very resilient to variances in the TSC. This patch uses
the VM state to determine whether to set TSC or not. Some minor
enhancements for getting TSC values as well that considers the VM state.

Additionally, while setting the TSC, the partition is suspended to
reduce the variance in the TSC value across vCPUs.

Signed-off-by: Sunil Muthuswamy 
Message-Id: 

Signed-off-by: Paolo Bonzini 
---
 include/sysemu/whpx.h  |   7 +++
 target/i386/whp-dispatch.h |   9 
 target/i386/whpx-all.c | 103 +
 3 files changed, 110 insertions(+), 9 deletions(-)

diff --git a/include/sysemu/whpx.h b/include/sysemu/whpx.h
index 4794e8e..a84b49e 100644
--- a/include/sysemu/whpx.h
+++ b/include/sysemu/whpx.h
@@ -35,4 +35,11 @@ int whpx_enabled(void);
 
 #endif /* CONFIG_WHPX */
 
+/* state subset only touched by the VCPU itself during runtime */
+#define WHPX_SET_RUNTIME_STATE   1
+/* state subset modified during VCPU reset */
+#define WHPX_SET_RESET_STATE 2
+/* full state set, modified during initialization or on vmload */
+#define WHPX_SET_FULL_STATE  3
+
 #endif /* QEMU_WHPX_H */
diff --git a/target/i386/whp-dispatch.h b/target/i386/whp-dispatch.h
index 87d049c..e4695c3 100644
--- a/target/i386/whp-dispatch.h
+++ b/target/i386/whp-dispatch.h
@@ -23,6 +23,12 @@
   X(HRESULT, WHvGetVirtualProcessorRegisters, (WHV_PARTITION_HANDLE Partition, 
UINT32 VpIndex, const WHV_REGISTER_NAME* RegisterNames, UINT32 RegisterCount, 
WHV_REGISTER_VALUE* RegisterValues)) \
   X(HRESULT, WHvSetVirtualProcessorRegisters, (WHV_PARTITION_HANDLE Partition, 
UINT32 VpIndex, const WHV_REGISTER_NAME* RegisterNames, UINT32 RegisterCount, 
const WHV_REGISTER_VALUE* RegisterValues)) \
 
+/*
+ * These are supplemental functions that may not be present
+ * on all versions and are not critical for basic functionality.
+ */
+#define LIST_WINHVPLATFORM_FUNCTIONS_SUPPLEMENTAL(X) \
+  X(HRESULT, WHvSuspendPartitionTime, (WHV_PARTITION_HANDLE Partition)) \
 
 #define LIST_WINHVEMULATION_FUNCTIONS(X) \
   X(HRESULT, WHvEmulatorCreateEmulator, (const WHV_EMULATOR_CALLBACKS* 
Callbacks, WHV_EMULATOR_HANDLE* Emulator)) \
@@ -40,10 +46,12 @@
 /* Define function typedef */
 LIST_WINHVPLATFORM_FUNCTIONS(WHP_DEFINE_TYPE)
 LIST_WINHVEMULATION_FUNCTIONS(WHP_DEFINE_TYPE)
+LIST_WINHVPLATFORM_FUNCTIONS_SUPPLEMENTAL(WHP_DEFINE_TYPE)
 
 struct WHPDispatch {
 LIST_WINHVPLATFORM_FUNCTIONS(WHP_DECLARE_MEMBER)
 LIST_WINHVEMULATION_FUNCTIONS(WHP_DECLARE_MEMBER)
+LIST_WINHVPLATFORM_FUNCTIONS_SUPPLEMENTAL(WHP_DECLARE_MEMBER)
 };
 
 extern struct WHPDispatch whp_dispatch;
@@ -53,6 +61,7 @@ bool init_whp_dispatch(void);
 typedef enum WHPFunctionList {
 WINHV_PLATFORM_FNS_DEFAULT,
 WINHV_EMULATION_FNS_DEFAULT,
+WINHV_PLATFORM_FNS_SUPPLEMENTAL
 } WHPFunctionList;
 
 #endif /* WHP_DISPATCH_H */
diff --git a/target/i386/whpx-all.c b/target/i386/whpx-all.c
index 683d49d..b947eb1 100644
--- a/target/i386/whpx-all.c
+++ b/target/i386/whpx-all.c
@@ -114,7 +114,6 @@ static const WHV_REGISTER_NAME whpx_register_names[] = {
 WHvX64RegisterXmmControlStatus,
 
 /* X64 MSRs */
-WHvX64RegisterTsc,
 WHvX64RegisterEfer,
 #ifdef TARGET_X86_64
 WHvX64RegisterKernelGsBase,
@@ -215,7 +214,44 @@ static SegmentCache whpx_seg_h2q(const 
WHV_X64_SEGMENT_REGISTER *hs)
 return qs;
 }
 
-static void whpx_set_registers(CPUState *cpu)
+static int whpx_set_tsc(CPUState *cpu)
+{
+struct CPUX86State *env = (CPUArchState *)(cpu->env_ptr);
+WHV_REGISTER_NAME tsc_reg = WHvX64RegisterTsc;
+WHV_REGISTER_VALUE tsc_val;
+HRESULT hr;
+struct whpx_state *whpx = _global;
+
+/*
+ * Suspend the partition prior to setting the TSC to reduce the variance
+ * in TSC across vCPUs. When the first vCPU runs post suspend, the
+ * partition is automatically resumed.
+ */
+if (whp_dispatch.WHvSuspendPartitionTime) {
+
+/*
+ * Unable to suspend partition while setting TSC is not a fatal
+ * error. It just increases the likelihood of TSC variance between
+ * vCPUs and some guest OS are able to handle that just fine.
+ */
+hr = whp_dispatch.WHvSuspendPartitionTime(whpx->partition);
+if (FAILED(hr)) {
+warn_report("WHPX: Failed to suspend partition, hr=%08lx", hr);
+}
+}
+
+tsc_val.Reg64 = env->tsc;
+hr = whp_dispatch.WHvSetVirtualProcessorRegisters(
+whpx->partition, cpu->cpu_index, _reg, 1, _val);
+if (FAILED(hr)) {
+error_report("WHPX: Failed to set TSC, hr=%08lx", hr);
+return -1;
+}
+
+return 0;
+}
+
+static void whpx_set_registers(CPUState *cpu, int level)
 {
 struct whpx_state *whpx = _global;
 struct whpx_vcpu *vcpu = get_whpx_vcpu(cpu);
@@ -230,6 +266,14 @@ static void 

[PULL 09/61] MAINTAINERS: Add entry for Guest X86 HAXM CPUs

[Paolo Bonzini]

From: Colin Xu 

HAXM covers below files:
include/sysemu/hax.h
target/i386/hax-*

V2: Add HAXM github page for wiki and issue tracking.

Cc: Wenchao Wang 
Cc: Hang Yuan 
Reviewed-by: Hang Yuan 
Signed-off-by: Colin Xu 
Message-Id: <20200228012046.6629-1-colin...@intel.com>
Signed-off-by: Paolo Bonzini 
---
 MAINTAINERS | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 32867bc..a88bc28 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -436,6 +436,17 @@ F: include/hw/block/dataplane/xen*
 F: include/hw/xen/
 F: include/sysemu/xen-mapcache.h
 
+Guest CPU Cores (HAXM)
+-
+X86 HAXM CPUs
+M: Wenchao Wang 
+M: Colin Xu 
+L: haxm-t...@intel.com
+W: https://github.com/intel/haxm/issues
+S: Maintained
+F: include/sysemu/hax.h
+F: target/i386/hax-*
+
 Hosts
 -
 LINUX
-- 
1.8.3.1

[PULL 19/61] ich9: Simplify ich9_lpc_initfn

[Paolo Bonzini]

From: Felipe Franciosi 

Currently, ich9_lpc_initfn simply serves as a caller to
ich9_lpc_add_properties. This simplifies the code a bit by eliminating
ich9_lpc_add_properties altogether and executing its logic in the parent
object initialiser function.

Signed-off-by: Felipe Franciosi 
Reviewed-by: Marc-André Lureau 
Signed-off-by: Paolo Bonzini 
---
 hw/isa/lpc_ich9.c | 15 +--
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 2471463..3d0f4db 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -632,12 +632,14 @@ static void ich9_lpc_get_sci_int(Object *obj, Visitor *v, 
const char *name,
 visit_type_uint8(v, name, >sci_gsi, errp);
 }
 
-static void ich9_lpc_add_properties(ICH9LPCState *lpc)
+static void ich9_lpc_initfn(Object *obj)
 {
+ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
+
 static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
 static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
 
-object_property_add(OBJECT(lpc), ACPI_PM_PROP_SCI_INT, "uint8",
+object_property_add(obj, ACPI_PM_PROP_SCI_INT, "uint8",
 ich9_lpc_get_sci_int,
 NULL, NULL, NULL, NULL);
 object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
@@ -645,14 +647,7 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
 object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
   _disable_cmd, OBJ_PROP_FLAG_READ, NULL);
 
-ich9_pm_add_properties(OBJECT(lpc), >pm, NULL);
-}
-
-static void ich9_lpc_initfn(Object *obj)
-{
-ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
-
-ich9_lpc_add_properties(lpc);
+ich9_pm_add_properties(obj, >pm, NULL);
 }
 
 static void ich9_lpc_realize(PCIDevice *d, Error **errp)
-- 
1.8.3.1

[PULL 39/61] scripts/cocci: Rename memory-region-{init-ram -> housekeeping}

[Paolo Bonzini]

From: Philippe Mathieu-Daudé 

As we are going to add various semantic changes related to the memory
region API, rename this script to be more generic.
Add a 'usage' header, and an entry in MAINTAINERS to avoid checkpatch
warning.

Signed-off-by: Philippe Mathieu-Daudé 
---
 MAINTAINERS |  1 +
 ...gion-init-ram.cocci => memory-region-housekeeping.cocci} | 13 +
 2 files changed, 14 insertions(+)
 rename scripts/coccinelle/{memory-region-init-ram.cocci => 
memory-region-housekeeping.cocci} (84%)

diff --git a/MAINTAINERS b/MAINTAINERS
index a88bc28..770126f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2047,6 +2047,7 @@ F: include/exec/ramblock.h
 F: memory.c
 F: include/exec/memory-internal.h
 F: exec.c
+F: scripts/coccinelle/memory-region-housekeeping.cocci
 
 SPICE
 M: Gerd Hoffmann 
diff --git a/scripts/coccinelle/memory-region-init-ram.cocci 
b/scripts/coccinelle/memory-region-housekeeping.cocci
similarity index 84%
rename from scripts/coccinelle/memory-region-init-ram.cocci
rename to scripts/coccinelle/memory-region-housekeeping.cocci
index d290150..3699c10 100644
--- a/scripts/coccinelle/memory-region-init-ram.cocci
+++ b/scripts/coccinelle/memory-region-housekeeping.cocci
@@ -1,3 +1,16 @@
+/*
+  Usage:
+
+spatch \
+--macro-file scripts/cocci-macro-file.h \
+--sp-file scripts/coccinelle/memory-region-housekeeping.cocci \
+--keep-comments \
+--in-place \
+--dir .
+
+*/
+
+
 // Replace by-hand memory_region_init_ram_nomigrate/vmstate_register_ram
 // code sequences with use of the new memory_region_init_ram function.
 // Similarly for the _rom and _rom_device functions.
-- 
1.8.3.1

[PULL 32/61] Use -isystem for linux-headers dir

[Paolo Bonzini]

From: Eduardo Habkost 

glibc and Linux-provided headers are known to generate macro
redefinition warnings when used together.  For example:
 and  duplicate some macro definitions.

We normally never see those warnings because GCC suppresses
warnings generated by system headers.  We carry our own copy of
Linux header files, though, and this makes those warnings not be
suppressed when glibc headers are included before Linux headers
(e.g. if  is included before ).

Use -isystem instead of -I for linux-headers.  This makes the
compiler treat our linux-headers directory the same way it treats
system-provided Linux headers, and suppress warnings generated by
them.

Signed-off-by: Eduardo Habkost 
Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Paolo Bonzini 
---
 Makefile.target | 2 +-
 configure   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile.target b/Makefile.target
index 2d43dc5..934a9f7 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -12,7 +12,7 @@ endif
 
 $(call set-vpath, $(SRC_PATH):$(BUILD_DIR))
 ifdef CONFIG_LINUX
-QEMU_CFLAGS += -I../linux-headers
+QEMU_CFLAGS += -isystem ../linux-headers
 endif
 QEMU_CFLAGS += -iquote .. -iquote $(SRC_PATH)/target/$(TARGET_BASE_ARCH) 
-DNEED_CPU_H
 
diff --git a/configure b/configure
index 44a70cf..06fcd07 100755
--- a/configure
+++ b/configure
@@ -900,7 +900,7 @@ Linux)
   linux="yes"
   linux_user="yes"
   kvm="yes"
-  QEMU_INCLUDES="-I\$(SRC_PATH)/linux-headers -I$PWD/linux-headers 
$QEMU_INCLUDES"
+  QEMU_INCLUDES="-isystem \$(SRC_PATH)/linux-headers -isystem 
$PWD/linux-headers $QEMU_INCLUDES"
   supported_os="yes"
   libudev="yes"
 ;;
-- 
1.8.3.1

[PULL 44/61] hw/net: Use memory_region_init_rom() with read-only regions

[Paolo Bonzini]

From: Philippe Mathieu-Daudé 

This commit was produced with the Coccinelle script
scripts/coccinelle/memory-region-housekeeping.cocci.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/net/dp8393x.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 81fc13e..1563c11 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -986,13 +986,12 @@ static void dp8393x_realize(DeviceState *dev, Error 
**errp)
 
 s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
 
-memory_region_init_ram(>prom, OBJECT(dev),
-   "dp8393x-prom", SONIC_PROM_SIZE, _err);
+memory_region_init_rom(>prom, OBJECT(dev), "dp8393x-prom",
+   SONIC_PROM_SIZE, _err);
 if (local_err) {
 error_propagate(errp, local_err);
 return;
 }
-memory_region_set_readonly(>prom, true);
 prom = memory_region_get_ram_ptr(>prom);
 checksum = 0;
 for (i = 0; i < 6; i++) {
-- 
1.8.3.1

[PULL 51/61] scripts/cocci: Patch to remove unnecessary memory_region_set_readonly()

[Paolo Bonzini]

From: Philippe Mathieu-Daudé 

Add a semantic patch to remove memory_region_set_readonly() calls
on ROM memory regions.

Signed-off-by: Philippe Mathieu-Daudé 
---
 scripts/coccinelle/memory-region-housekeeping.cocci | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/scripts/coccinelle/memory-region-housekeeping.cocci 
b/scripts/coccinelle/memory-region-housekeeping.cocci
index 9cdde71..5e6b31d 100644
--- a/scripts/coccinelle/memory-region-housekeeping.cocci
+++ b/scripts/coccinelle/memory-region-housekeeping.cocci
@@ -48,6 +48,21 @@ p << possible_memory_region_init_rom.p;
 cocci.print_main("potential use of memory_region_init_rom*() in ", p)
 
 
+// Do not call memory_region_set_readonly() on ROM alias
+@@
+expression ROM, E1, E2, E3, E4;
+expression ALIAS, E5, E6, E7, E8;
+@@
+(
+  memory_region_init_rom(ROM, E1, E2, E3, E4);
+|
+  memory_region_init_rom_nomigrate(ROM, E1, E2, E3, E4);
+)
+  ...
+   memory_region_init_alias(ALIAS, E5, E6, ROM, E7, E8);
+-  memory_region_set_readonly(ALIAS, true);
+
+
 // Replace by-hand memory_region_init_ram_nomigrate/vmstate_register_ram
 // code sequences with use of the new memory_region_init_ram function.
 // Similarly for the _rom and _rom_device functions.
-- 
1.8.3.1

[PULL 41/61] hw/arm: Use memory_region_init_rom() with read-only regions

[Paolo Bonzini]

From: Philippe Mathieu-Daudé 

This commit was produced with the Coccinelle script
scripts/coccinelle/memory-region-housekeeping.cocci.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/arm/exynos4210.c | 3 +--
 hw/arm/mainstone.c  | 3 +--
 hw/arm/omap_sx1.c   | 6 ++
 hw/arm/palm.c   | 3 +--
 hw/arm/spitz.c  | 3 +--
 hw/arm/stellaris.c  | 3 +--
 hw/arm/tosa.c   | 3 +--
 7 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index 59a27bd..3af6502 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -311,9 +311,8 @@ static void exynos4210_realize(DeviceState *socdev, Error 
**errp)
 >chipid_mem);
 
 /* Internal ROM */
-memory_region_init_ram(>irom_mem, NULL, "exynos4210.irom",
+memory_region_init_rom(>irom_mem, NULL, "exynos4210.irom",
EXYNOS4210_IROM_SIZE, _fatal);
-memory_region_set_readonly(>irom_mem, true);
 memory_region_add_subregion(system_mem, EXYNOS4210_IROM_BASE_ADDR,
 >irom_mem);
 /* mirror of iROM */
diff --git a/hw/arm/mainstone.c b/hw/arm/mainstone.c
index 1042017..6bc6436 100644
--- a/hw/arm/mainstone.c
+++ b/hw/arm/mainstone.c
@@ -124,9 +124,8 @@ static void mainstone_common_init(MemoryRegion 
*address_space_mem,
 /* Setup CPU & memory */
 mpu = pxa270_init(address_space_mem, mainstone_binfo.ram_size,
   machine->cpu_type);
-memory_region_init_ram(rom, NULL, "mainstone.rom", MAINSTONE_ROM,
+memory_region_init_rom(rom, NULL, "mainstone.rom", MAINSTONE_ROM,
_fatal);
-memory_region_set_readonly(rom, true);
 memory_region_add_subregion(address_space_mem, 0, rom);
 
 /* There are two 32MiB flash devices on the board */
diff --git a/hw/arm/omap_sx1.c b/hw/arm/omap_sx1.c
index de5ff44..57829b3 100644
--- a/hw/arm/omap_sx1.c
+++ b/hw/arm/omap_sx1.c
@@ -131,9 +131,8 @@ static void sx1_init(MachineState *machine, const int 
version)
 mpu = omap310_mpu_init(machine->ram, machine->cpu_type);
 
 /* External Flash (EMIFS) */
-memory_region_init_ram(flash, NULL, "omap_sx1.flash0-0", flash_size,
+memory_region_init_rom(flash, NULL, "omap_sx1.flash0-0", flash_size,
_fatal);
-memory_region_set_readonly(flash, true);
 memory_region_add_subregion(address_space, OMAP_CS0_BASE, flash);
 
 memory_region_init_io([0], NULL, _ops, ,
@@ -167,9 +166,8 @@ static void sx1_init(MachineState *machine, const int 
version)
 if ((version == 1) &&
 (dinfo = drive_get(IF_PFLASH, 0, fl_idx)) != NULL) {
 MemoryRegion *flash_1 = g_new(MemoryRegion, 1);
-memory_region_init_ram(flash_1, NULL, "omap_sx1.flash1-0",
+memory_region_init_rom(flash_1, NULL, "omap_sx1.flash1-0",
flash1_size, _fatal);
-memory_region_set_readonly(flash_1, true);
 memory_region_add_subregion(address_space, OMAP_CS1_BASE, flash_1);
 
 memory_region_init_io([1], NULL, _ops, ,
diff --git a/hw/arm/palm.c b/hw/arm/palm.c
index 99554bd..97ca105 100644
--- a/hw/arm/palm.c
+++ b/hw/arm/palm.c
@@ -213,9 +213,8 @@ static void palmte_init(MachineState *machine)
 mpu = omap310_mpu_init(machine->ram, machine->cpu_type);
 
 /* External Flash (EMIFS) */
-memory_region_init_ram(flash, NULL, "palmte.flash", flash_size,
+memory_region_init_rom(flash, NULL, "palmte.flash", flash_size,
_fatal);
-memory_region_set_readonly(flash, true);
 memory_region_add_subregion(address_space_mem, OMAP_CS0_BASE, flash);
 
 memory_region_init_io([0], NULL, _ops, , "palmte-cs0",
diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index cbfa693..c28d9b5 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -929,8 +929,7 @@ static void spitz_common_init(MachineState *machine,
 
 sl_flash_register(mpu, (model == spitz) ? FLASH_128M : FLASH_1024M);
 
-memory_region_init_ram(rom, NULL, "spitz.rom", SPITZ_ROM, _fatal);
-memory_region_set_readonly(rom, true);
+memory_region_init_rom(rom, NULL, "spitz.rom", SPITZ_ROM, _fatal);
 memory_region_add_subregion(address_space_mem, 0, rom);
 
 /* Setup peripherals */
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 221a786..d136ba1 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -1300,9 +1300,8 @@ static void stellaris_init(MachineState *ms, 
stellaris_board_info *board)
 sram_size = ((board->dc0 >> 18) + 1) * 1024;
 
 /* Flash programming is done via the SCU, so pretend it is ROM.  */
-memory_region_init_ram(flash, NULL, "stellaris.flash", flash_size,
+memory_region_init_rom(flash, NULL, "stellaris.flash", flash_size,
_fatal);
-memory_region_set_readonly(flash, true);
 memory_region_add_subregion(system_memory, 0, flash);
 
 memory_region_init_ram(sram, NULL, "stellaris.sram", sram_size,
diff --git 

[PULL 60/61] hw/arm: Remove unnecessary memory_region_set_readonly() on ROM alias

[Paolo Bonzini]

From: Philippe Mathieu-Daudé 

This commit was produced with the Coccinelle script
scripts/coccinelle/memory-region-housekeeping.cocci.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/arm/exynos4210.c| 1 -
 hw/arm/stm32f205_soc.c | 1 -
 hw/arm/stm32f405_soc.c | 1 -
 3 files changed, 3 deletions(-)

diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index 3af6502..4e1fd7e 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -320,7 +320,6 @@ static void exynos4210_realize(DeviceState *socdev, Error 
**errp)
  >irom_mem,
  0,
  EXYNOS4210_IROM_SIZE);
-memory_region_set_readonly(>irom_alias_mem, true);
 memory_region_add_subregion(system_mem, EXYNOS4210_IROM_MIRROR_BASE_ADDR,
 >irom_alias_mem);
 
diff --git a/hw/arm/stm32f205_soc.c b/hw/arm/stm32f205_soc.c
index 2de5627..6e93726 100644
--- a/hw/arm/stm32f205_soc.c
+++ b/hw/arm/stm32f205_soc.c
@@ -97,7 +97,6 @@ static void stm32f205_soc_realize(DeviceState *dev_soc, Error 
**errp)
_fatal);
 memory_region_init_alias(flash_alias, NULL, "STM32F205.flash.alias",
  flash, 0, FLASH_SIZE);
-memory_region_set_readonly(flash_alias, true);
 
 memory_region_add_subregion(system_memory, FLASH_BASE_ADDRESS, flash);
 memory_region_add_subregion(system_memory, 0, flash_alias);
diff --git a/hw/arm/stm32f405_soc.c b/hw/arm/stm32f405_soc.c
index b8fca13..d590cd0 100644
--- a/hw/arm/stm32f405_soc.c
+++ b/hw/arm/stm32f405_soc.c
@@ -103,7 +103,6 @@ static void stm32f405_soc_realize(DeviceState *dev_soc, 
Error **errp)
 }
 memory_region_init_alias(>flash_alias, NULL, "STM32F405.flash.alias",
  >flash, 0, FLASH_SIZE);
-memory_region_set_readonly(>flash_alias, true);
 
 memory_region_add_subregion(system_memory, FLASH_BASE_ADDRESS, >flash);
 memory_region_add_subregion(system_memory, 0, >flash_alias);
-- 
1.8.3.1

Re: [PATCH v2 3/8] ppc/spapr: Add FWNMI System Reset state

[David Gibson]

On Tue, Mar 17, 2020 at 12:26:08AM +1000, Nicholas Piggin wrote:
> The FWNMI option must deliver system reset interrupts to their
> registered address, and there are a few constraints on the handler
> addresses specified in PAPR. Add the system reset address state and
> checks.
> 
> Signed-off-by: Nicholas Piggin 

Applied to ppc-for-5.0, thanks.

> ---
>  hw/ppc/spapr.c |  2 ++
>  hw/ppc/spapr_rtas.c| 14 +-
>  include/hw/ppc/spapr.h |  3 ++-
>  3 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b03b26370d..5f93c49706 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1704,6 +1704,7 @@ static void spapr_machine_reset(MachineState *machine)
>  
>  spapr->cas_reboot = false;
>  
> +spapr->fwnmi_system_reset_addr = -1;
>  spapr->fwnmi_machine_check_addr = -1;
>  spapr->fwnmi_machine_check_interlock = -1;
>  
> @@ -2023,6 +2024,7 @@ static const VMStateDescription vmstate_spapr_fwnmi = {
>  .needed = spapr_fwnmi_needed,
>  .pre_save = spapr_fwnmi_pre_save,
>  .fields = (VMStateField[]) {
> +VMSTATE_UINT64(fwnmi_system_reset_addr, SpaprMachineState),
>  VMSTATE_UINT64(fwnmi_machine_check_addr, SpaprMachineState),
>  VMSTATE_INT32(fwnmi_machine_check_interlock, SpaprMachineState),
>  VMSTATE_END_OF_LIST()
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 0b8c481593..521e6b0b72 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -414,6 +414,7 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
>uint32_t nret, target_ulong rets)
>  {
>  hwaddr rtas_addr;
> +target_ulong sreset_addr, mce_addr;
>  
>  if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI) == SPAPR_CAP_OFF) {
>  rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
> @@ -426,7 +427,18 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
>  return;
>  }
>  
> -spapr->fwnmi_machine_check_addr = rtas_ld(args, 1);
> +sreset_addr = rtas_ld(args, 0);
> +mce_addr = rtas_ld(args, 1);
> +
> +/* PAPR requires these are in the first 32M of memory and within RMA */
> +if (sreset_addr >= 32 * MiB || sreset_addr >= spapr->rma_size ||
> +   mce_addr >= 32 * MiB ||mce_addr >= spapr->rma_size) {
> +rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +return;
> +}
> +
> +spapr->fwnmi_system_reset_addr = sreset_addr;
> +spapr->fwnmi_machine_check_addr = mce_addr;
>  
>  rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>  }
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 64b83402cb..42d64a0368 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -194,9 +194,10 @@ struct SpaprMachineState {
>  
>  /* State related to FWNMI option */
>  
> -/* Machine Check Notification Routine address
> +/* System Reset and Machine Check Notification Routine addresses
>   * registered by "ibm,nmi-register" RTAS call.
>   */
> +target_ulong fwnmi_system_reset_addr;
>  target_ulong fwnmi_machine_check_addr;
>  
>  /* Machine Check FWNMI synchronization, fwnmi_machine_check_interlock is

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH v3 00/19] Support disabling TCG on ARM (part 2)

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200316160634.3386-1-phi...@redhat.com/



Hi,

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

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

missing object type 'or-irq'
Broken pipe
/tmp/qemu-test/src/tests/qtest/libqtest.c:175: kill_qemu() detected QEMU death 
from signal 6 (Aborted) (core dumped)
ERROR - too few tests run (expected 6, got 5)
make: *** [check-qtest-aarch64] Error 1
make: *** Waiting for unfinished jobs
Could not access KVM kernel module: No such file or directory
qemu-system-x86_64: -accel kvm: failed to initialize kvm: No such file or 
directory
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=d51fe73160644e0ba6a0f3cf1c1d6208', '-u', 
'1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
'-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 
'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
'/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-u8ju4ssp/src/docker-src.2020-03-16-18.57.13.21007:/var/tmp/qemu:z,ro',
 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=d51fe73160644e0ba6a0f3cf1c1d6208
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-u8ju4ssp/src'
make: *** [docker-run-test-quick@centos7] Error 2

real13m16.228s
user0m8.763s


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

Re: [PATCH v2 7/8] ppc/spapr: Implement FWNMI System Reset delivery

[David Gibson]

On Tue, Mar 17, 2020 at 12:26:12AM +1000, Nicholas Piggin wrote:
> PAPR requires that if "ibm,nmi-register" succeeds, then the hypervisor
> delivers all system reset and machine check exceptions to the registered
> addresses.
> 
> System Resets are delivered with registers set to the architected state,
> and with no interlock.
> 
> Signed-off-by: Nicholas Piggin 

Applied to ppc-for-5.0.

> ---
>  hw/ppc/spapr.c | 46 --
>  1 file changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 25221d843c..78e649f47d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -967,7 +967,29 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void 
> *fdt)
>  _FDT(fdt_setprop(fdt, rtas, "ibm,max-associativity-domains",
>   maxdomains, sizeof(maxdomains)));
>  
> -_FDT(fdt_setprop_cell(fdt, rtas, "rtas-size", RTAS_SIZE));
> +/*
> + * FWNMI reserves RTAS_ERROR_LOG_MAX for the machine check error log,
> + * and 16 bytes per CPU for system reset error log plus an extra 8 bytes.
> + *
> + * The system reset requirements are driven by existing Linux and PowerVM
> + * implementation which (contrary to PAPR) saves r3 in the error log
> + * structure like machine check, so Linux expects to find the saved r3
> + * value at the address in r3 upon FWNMI-enabled sreset interrupt (and
> + * does not look at the error value).
> + *
> + * System reset interrupts are not subject to interlock like machine
> + * check, so this memory area could be corrupted if the sreset is
> + * interrupted by a machine check (or vice versa) if it was shared. To
> + * prevent this, system reset uses per-CPU areas for the sreset save
> + * area. A system reset that interrupts a system reset handler could
> + * still overwrite this area, but Linux doesn't try to recover in that
> + * case anyway.
> + *
> + * The extra 8 bytes is required because Linux's FWNMI error log check
> + * is off-by-one.
> + */
> +_FDT(fdt_setprop_cell(fdt, rtas, "rtas-size", RTAS_ERROR_LOG_MAX +
> +   ms->smp.max_cpus * sizeof(uint64_t)*2 + 
> sizeof(uint64_t)));
>  _FDT(fdt_setprop_cell(fdt, rtas, "rtas-error-log-max",
>RTAS_ERROR_LOG_MAX));
>  _FDT(fdt_setprop_cell(fdt, rtas, "rtas-event-scan-rate",
> @@ -3399,8 +3421,28 @@ static void spapr_machine_finalizefn(Object *obj)
>  
>  void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg)
>  {
> +SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +
>  cpu_synchronize_state(cs);
> -ppc_cpu_do_system_reset(cs, -1);
> +/* If FWNMI is inactive, addr will be -1, which will deliver to 0x100 */
> +if (spapr->fwnmi_system_reset_addr != -1) {
> +uint64_t rtas_addr, addr;
> +PowerPCCPU *cpu = POWERPC_CPU(cs);
> +CPUPPCState *env = >env;
> +
> +/* get rtas addr from fdt */
> +rtas_addr = spapr_get_rtas_addr();
> +if (!rtas_addr) {
> +qemu_system_guest_panicked(NULL);
> +return;
> +}
> +
> +addr = rtas_addr + RTAS_ERROR_LOG_MAX + cs->cpu_index * 
> sizeof(uint64_t)*2;
> +stq_be_phys(_space_memory, addr, env->gpr[3]);
> +stq_be_phys(_space_memory, addr + sizeof(uint64_t), 0);
> +env->gpr[3] = addr;
> +}
> +ppc_cpu_do_system_reset(cs, spapr->fwnmi_system_reset_addr);
>  }
>  
>  static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH v2 7/8] ppc/spapr: Implement FWNMI System Reset delivery

[David Gibson]

On Mon, Mar 16, 2020 at 06:52:54PM +0100, Greg Kurz wrote:
> On Mon, 16 Mar 2020 23:05:00 +0530
> Mahesh J Salgaonkar  wrote:
> 
> > On 2020-03-17 00:26:12 Tue, Nicholas Piggin wrote:
> > > PAPR requires that if "ibm,nmi-register" succeeds, then the hypervisor
> > > delivers all system reset and machine check exceptions to the registered
> > > addresses.
> > > 
> > > System Resets are delivered with registers set to the architected state,
> > > and with no interlock.
> > > 
> > > Signed-off-by: Nicholas Piggin 
> > > ---
> > >  hw/ppc/spapr.c | 46 --
> > >  1 file changed, 44 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 25221d843c..78e649f47d 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -967,7 +967,29 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, 
> > > void *fdt)
> > >  _FDT(fdt_setprop(fdt, rtas, "ibm,max-associativity-domains",
> > >   maxdomains, sizeof(maxdomains)));
> > > 
> > > -_FDT(fdt_setprop_cell(fdt, rtas, "rtas-size", RTAS_SIZE));
> > > +/*
> > > + * FWNMI reserves RTAS_ERROR_LOG_MAX for the machine check error log,
> > > + * and 16 bytes per CPU for system reset error log plus an extra 8 
> > > bytes.
> > > + *
> > > + * The system reset requirements are driven by existing Linux and 
> > > PowerVM
> > > + * implementation which (contrary to PAPR) saves r3 in the error log
> > > + * structure like machine check, so Linux expects to find the saved 
> > > r3
> > > + * value at the address in r3 upon FWNMI-enabled sreset interrupt 
> > > (and
> > > + * does not look at the error value).
> > > + *
> > > + * System reset interrupts are not subject to interlock like machine
> > > + * check, so this memory area could be corrupted if the sreset is
> > > + * interrupted by a machine check (or vice versa) if it was shared. 
> > > To
> > > + * prevent this, system reset uses per-CPU areas for the sreset save
> > > + * area. A system reset that interrupts a system reset handler could
> > > + * still overwrite this area, but Linux doesn't try to recover in 
> > > that
> > > + * case anyway.
> > > + *
> > > + * The extra 8 bytes is required because Linux's FWNMI error log 
> > > check
> > > + * is off-by-one.
> > > + */
> > > +_FDT(fdt_setprop_cell(fdt, rtas, "rtas-size", RTAS_ERROR_LOG_MAX +
> > > +   ms->smp.max_cpus * sizeof(uint64_t)*2 + 
> > > sizeof(uint64_t)));
> > 
> > Currently the rtas region is only of size 2048 (i.e RTAS_ERROR_LOG_MAX).
> > Do we need SLOF change to increase rtas area as well ? Otherwise QEMU
> > may corrupt guest memory area OR Am I wrong ?
> > 
> 
> A change is pending for SLOF to use the "rtas-size" property
> provided by QEMU:
> 
> https://patchwork.ozlabs.org/patch/1255264/

In the meantime, this is still correct.  Because we rebuild the device
tree at CAS time, the qemu supplied value will be the one the guest
sees in the end.  We obviously want that qemu update to avoid
confusion, but we don't need it for things to work.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH v2 4/8] ppc/spapr: Fix FWNMI machine check interrupt delivery

[David Gibson]

On Tue, Mar 17, 2020 at 09:19:57AM +1000, Nicholas Piggin wrote:
> Cédric Le Goater's on March 17, 2020 3:59 am:
> > On 3/16/20 3:26 PM, Nicholas Piggin wrote:
> >> FWNMI machine check delivery misses a few things that will make it fail
> >> with TCG at least (which we would like to allow in future to improve
> >> testing).
> > 
> > I don't understand which issues are addressed in the patch.
> 
> The existing code does not compute hflags, at least.
> 
> There's a few possible other things, I didn't dig into qemu enough
> to know if they might be a problem (e.g., reservation and TLB). I
> figure it's better to keep these consistent.
> 
> Keep in mind this is a bit academic right now, because we can't
> (AFAIKS) inject an MCE from TCG. It would be good to wire that up,
> but I didn't get to it.
> 
> >> It's not nice to scatter interrupt delivery logic around the tree, so
> >> move it to excp_helper.c and share code where possible.
> > 
> > It looks correct but this is touching the ugliest routine in the QEMU 
> > PPC universe. I would split the patch in two to introduce the helper
> > powerpc_set_excp_state().
> > 
> > It does not seem to need to be an inline also.
> 
> Yeah it's all pretty ugly. I didn't yet find a nice way to do
> split things up that did not require a lot of code churn, but that
> can come later.
> 
> Inline was just because powerpc_excp is inline, I didn't want to
> change behaviour too much there (it obviously wants to do a lot of
> constant propagation but maybe only on the case statement). Anyway
> I just wanted to be minimal for now, it could be changed.

Yeah, I definitely want to get this in, so despite imperfections that
could probably be polished with time, I've applied to ppc-for-5.0.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

[PULL 27/61] hw/usb/quirks: Use smaller types to reduce .rodata by 10KiB

[Paolo Bonzini]

From: Philippe Mathieu-Daudé 

The USB descriptor sizes are specified as 16-bit for idVendor /
idProduct, and 8-bit for bInterfaceClass / bInterfaceSubClass /
bInterfaceProtocol. Doing so we reduce the usbredir_raw_serial_ids[]
and usbredir_ftdi_serial_ids[] arrays from 16KiB to 6KiB (size
reported on x86_64 host, building with --extra-cflags=-Os).

Signed-off-by: Philippe Mathieu-Daudé 
Signed-off-by: Paolo Bonzini 
---
 hw/usb/quirks.c |  4 ++--
 hw/usb/quirks.h | 22 +-
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/hw/usb/quirks.c b/hw/usb/quirks.c
index 38a9c56..23ea7a2 100644
--- a/hw/usb/quirks.c
+++ b/hw/usb/quirks.c
@@ -22,10 +22,10 @@ static bool usb_id_match(const struct usb_device_id *ids,
  uint8_t interface_protocol) {
 int i;
 
-for (i = 0; ids[i].vendor_id != -1; i++) {
+for (i = 0; ids[i].terminating_entry == 0; i++) {
 if (ids[i].vendor_id  == vendor_id &&
 ids[i].product_id == product_id &&
-(ids[i].interface_class == -1 ||
+(ids[i].interface_protocol_used == 0 ||
  (ids[i].interface_class == interface_class &&
   ids[i].interface_subclass == interface_subclass &&
   ids[i].interface_protocol == interface_protocol))) {
diff --git a/hw/usb/quirks.h b/hw/usb/quirks.h
index 89480be..50ef2f9 100644
--- a/hw/usb/quirks.h
+++ b/hw/usb/quirks.h
@@ -21,19 +21,23 @@
 #include "quirks-pl2303-ids.h"
 
 struct usb_device_id {
-int vendor_id;
-int product_id;
-int interface_class;
-int interface_subclass;
-int interface_protocol;
+uint16_t vendor_id;
+uint16_t product_id;
+uint8_t interface_class;
+uint8_t interface_subclass;
+uint8_t interface_protocol;
+uint8_t interface_protocol_used:1,
+terminating_entry:1,
+reserved:6;
 };
 
 #define USB_DEVICE(vendor, product) \
-.vendor_id = vendor, .product_id = product, .interface_class = -1,
+.vendor_id = vendor, .product_id = product, .interface_protocol_used = 0,
 
 #define USB_DEVICE_AND_INTERFACE_INFO(vend, prod, iclass, isubclass, iproto) \
 .vendor_id = vend, .product_id = prod, .interface_class = iclass, \
-.interface_subclass = isubclass, .interface_protocol = iproto
+.interface_subclass = isubclass, .interface_protocol = iproto, \
+.interface_protocol_used = 1
 
 static const struct usb_device_id usbredir_raw_serial_ids[] = {
 /*
@@ -206,7 +210,7 @@ static const struct usb_device_id usbredir_raw_serial_ids[] 
= {
 { USB_DEVICE(ADLINK_VENDOR_ID, ADLINK_ND6530_PRODUCT_ID) },
 { USB_DEVICE(SMART_VENDOR_ID, SMART_PRODUCT_ID) },
 
-{ USB_DEVICE(-1, -1) } /* Terminating Entry */
+{ .terminating_entry = 1 } /* Terminating Entry */
 };
 
 static const struct usb_device_id usbredir_ftdi_serial_ids[] = {
@@ -906,7 +910,7 @@ static const struct usb_device_id 
usbredir_ftdi_serial_ids[] = {
 { USB_DEVICE(FTDI_VID, FTDI_DISTORTEC_JTAG_LOCK_PICK_PID) },
 { USB_DEVICE(FTDI_VID, FTDI_LUMEL_PD12_PID) },
 
-{ USB_DEVICE(-1, -1) } /* Terminating Entry */
+{ .terminating_entry = 1 } /* Terminating Entry */
 };
 
 #undef USB_DEVICE
-- 
1.8.3.1

[PULL 17/61] qom/object: enable setter for uint types

[Paolo Bonzini]

From: Felipe Franciosi 

Traditionally, the uint-specific property helpers only offer getters.
When adding object (or class) uint types, one must therefore use the
generic property helper if a setter is needed (and probably duplicate
some code writing their own getters/setters).

This enhances the uint-specific property helper APIs by adding a
bitwise-or'd 'flags' field and modifying all clients of that API to set
this paramater to OBJ_PROP_FLAG_READ. This maintains the current
behaviour whilst allowing others to also set OBJ_PROP_FLAG_WRITE (or use
the more convenient OBJ_PROP_FLAG_READWRITE) in the future (which will
automatically install a setter). Other flags may be added later.

Signed-off-by: Felipe Franciosi 
Reviewed-by: Marc-André Lureau 
Signed-off-by: Paolo Bonzini 
---
 hw/acpi/ich9.c   |   4 +-
 hw/acpi/pcihp.c  |   7 +-
 hw/acpi/piix4.c  |  12 +--
 hw/isa/lpc_ich9.c|   4 +-
 hw/ppc/spapr_drc.c   |   3 +-
 include/qom/object.h |  48 ++--
 qom/object.c | 212 +--
 ui/console.c |   4 +-
 8 files changed, 246 insertions(+), 48 deletions(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 4e74284..67fe05a 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -454,12 +454,12 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs 
*pm, Error **errp)
 pm->s4_val = 2;
 
 object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
-   >pm_io_base, errp);
+   >pm_io_base, OBJ_PROP_FLAG_READ, errp);
 object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, "uint32",
 ich9_pm_get_gpe0_blk,
 NULL, NULL, pm, NULL);
 object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
-   _len, errp);
+   _len, OBJ_PROP_FLAG_READ, errp);
 object_property_add_bool(obj, "memory-hotplug-support",
  ich9_pm_get_memory_hotplug_support,
  ich9_pm_set_memory_hotplug_support,
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 8413348..4dcef37 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -80,7 +80,8 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
 
 *bus_bsel = (*bsel_alloc)++;
 object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
-   bus_bsel, _abort);
+   bus_bsel, OBJ_PROP_FLAG_READ,
+   _abort);
 }
 
 return bsel_alloc;
@@ -373,9 +374,9 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, 
PCIBus *root_bus,
 memory_region_add_subregion(address_space_io, s->io_base, >io);
 
 object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_BASE_PROP, >io_base,
-   _abort);
+   OBJ_PROP_FLAG_READ, _abort);
 object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_LEN_PROP, >io_len,
-   _abort);
+   OBJ_PROP_FLAG_READ, _abort);
 }
 
 const VMStateDescription vmstate_acpi_pcihp_pci_status = {
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index b84dbba..964d6f5 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -444,17 +444,17 @@ static void piix4_pm_add_propeties(PIIX4PMState *s)
 static const uint16_t sci_int = 9;
 
 object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_ENABLE_CMD,
-  _enable_cmd, NULL);
+  _enable_cmd, OBJ_PROP_FLAG_READ, NULL);
 object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
-  _disable_cmd, NULL);
+  _disable_cmd, OBJ_PROP_FLAG_READ, NULL);
 object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
-  _blk, NULL);
+  _blk, OBJ_PROP_FLAG_READ, NULL);
 object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
-  _blk_len, NULL);
+  _blk_len, OBJ_PROP_FLAG_READ, NULL);
 object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
-  _int, NULL);
+  _int, OBJ_PROP_FLAG_READ, NULL);
 object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
-  >io_base, NULL);
+  >io_base, OBJ_PROP_FLAG_READ, NULL);
 }
 
 static void piix4_pm_realize(PCIDevice *dev, Error **errp)
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index cb79616..d8186f5 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -643,9 +643,9 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
 ich9_lpc_get_sci_int,
 

[PULL 42/61] hw/display: Use memory_region_init_rom() with read-only regions

[Paolo Bonzini]

From: Philippe Mathieu-Daudé 

This commit was produced with the Coccinelle script
scripts/coccinelle/memory-region-housekeeping.cocci.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/display/cg3.c | 5 ++---
 hw/display/tcx.c | 5 ++---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index 4fb67c6..a1ede10 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -287,9 +287,8 @@ static void cg3_initfn(Object *obj)
 SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 CG3State *s = CG3(obj);
 
-memory_region_init_ram_nomigrate(>rom, obj, "cg3.prom", 
FCODE_MAX_ROM_SIZE,
-   _fatal);
-memory_region_set_readonly(>rom, true);
+memory_region_init_rom_nomigrate(>rom, obj, "cg3.prom",
+ FCODE_MAX_ROM_SIZE, _fatal);
 sysbus_init_mmio(sbd, >rom);
 
 memory_region_init_io(>reg, obj, _reg_ops, s, "cg3.reg",
diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index ca458f9..76de16e 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -755,9 +755,8 @@ static void tcx_initfn(Object *obj)
 SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 TCXState *s = TCX(obj);
 
-memory_region_init_ram_nomigrate(>rom, obj, "tcx.prom", 
FCODE_MAX_ROM_SIZE,
-   _fatal);
-memory_region_set_readonly(>rom, true);
+memory_region_init_rom_nomigrate(>rom, obj, "tcx.prom",
+ FCODE_MAX_ROM_SIZE, _fatal);
 sysbus_init_mmio(sbd, >rom);
 
 /* 2/STIP : Stippler */
-- 
1.8.3.1

Re: [PATCH] linux-user: Update TASK_UNMAPPED_BASE for aarch64

[Lirong Yuan]

Thanks Laurent for the prompt responses! Yes that is exactly where the
value is taken from.

For anyone interested in detailed information of the change, here is the
previous discussion thread:
http://patchwork.ozlabs.org/patch/1242370/

Regards,
Lirong


On Sat, Mar 14, 2020 at 10:34 AM Laurent Vivier  wrote:

> Le 14/03/2020 à 18:01, Aleksandar Markovic a écrit :
> > On Sat, Mar 14, 2020 at 11:45 AM Laurent Vivier 
> wrote:
> >>
> >> Le 14/03/2020 à 04:06, Aleksandar Markovic a écrit :
> >>> On Fri, Mar 13, 2020 at 1:28 AM Lirong Yuan  wrote:
> 
>  This change updates TASK_UNMAPPED_BASE (the base address for guest
> programs) for aarch64. It is needed to allow qemu to work with Thread
> Sanitizer (TSan), which has specific boundary definitions for memory
> mappings on different platforms:
> 
> https://github.com/llvm/llvm-project/blob/master/compiler-rt/lib/tsan/rtl/tsan_platform.h
> 
>  Signed-off-by: Lirong Yuan 
>  ---
>   linux-user/mmap.c | 4 
>   1 file changed, 4 insertions(+)
> 
>  diff --git a/linux-user/mmap.c b/linux-user/mmap.c
>  index 8685f02e7e..e378033797 100644
>  --- a/linux-user/mmap.c
>  +++ b/linux-user/mmap.c
>  @@ -184,7 +184,11 @@ static int mmap_frag(abi_ulong real_start,
>   }
> 
>   #if HOST_LONG_BITS == 64 && TARGET_ABI_BITS == 64
>  +#ifdef TARGET_AARCH64
>  +# define TASK_UNMAPPED_BASE  0x55
> >>>
> >>> Hi, Lirong,
> >>>
> >>> Can you point from which line of the file you linked to did you
> >>> arrive to the value 0x55?
> >>>
> >>> Second question: What about other targets?
> >>
> >> Personally, I prefer to not change the value for other targets if it is
> >> not required by someone that had some problems with the current value.
> >>
> >> It needs to be changed carefully and to be well tested after change.
> >>
> >
> > Sure, but again, from where " 0x55" comes from?
>
> The URL is in the comment, but more precisely I guess:
>
>
> https://github.com/llvm/llvm-project/blob/master/compiler-rt/lib/tsan/rtl/tsan_platform.h#L164
>
> Thanks,
> Laurent
>
>

[PULL 50/61] scripts/cocci: Patch to detect potential use of memory_region_init_rom

[Paolo Bonzini]

From: Philippe Mathieu-Daudé 

Add a semantic patch to detect potential replacement of
memory_region_init_ram(readonly) by memory_region_init_rom().

Signed-off-by: Philippe Mathieu-Daudé 
---
 scripts/coccinelle/memory-region-housekeeping.cocci | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/scripts/coccinelle/memory-region-housekeeping.cocci 
b/scripts/coccinelle/memory-region-housekeeping.cocci
index ee3923d..9cdde71 100644
--- a/scripts/coccinelle/memory-region-housekeeping.cocci
+++ b/scripts/coccinelle/memory-region-housekeeping.cocci
@@ -29,6 +29,25 @@ symbol true;
 )
 
 
+@possible_memory_region_init_rom@
+expression E1, E2, E3, E4, E5;
+position p;
+@@
+(
+  memory_region_init_ram@p(E1, E2, E3, E4, E5);
+  ...
+  memory_region_set_readonly(E1, true);
+|
+  memory_region_init_ram_nomigrate@p(E1, E2, E3, E4, E5);
+  ...
+  memory_region_set_readonly(E1, true);
+)
+@script:python@
+p << possible_memory_region_init_rom.p;
+@@
+cocci.print_main("potential use of memory_region_init_rom*() in ", p)
+
+
 // Replace by-hand memory_region_init_ram_nomigrate/vmstate_register_ram
 // code sequences with use of the new memory_region_init_ram function.
 // Similarly for the _rom and _rom_device functions.
-- 
1.8.3.1

[PULL 40/61] scripts/cocci: Patch to replace memory_region_init_{ram, readonly -> rom}

[Paolo Bonzini]

From: Philippe Mathieu-Daudé 

Add a semantic patch to replace memory_region_init_ram(readonly)
by memory_region_init_rom().

Signed-off-by: Philippe Mathieu-Daudé 
---
 scripts/coccinelle/memory-region-housekeeping.cocci | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/scripts/coccinelle/memory-region-housekeeping.cocci 
b/scripts/coccinelle/memory-region-housekeeping.cocci
index 3699c10..ee3923d 100644
--- a/scripts/coccinelle/memory-region-housekeeping.cocci
+++ b/scripts/coccinelle/memory-region-housekeeping.cocci
@@ -11,6 +11,24 @@
 */
 
 
+// Replace memory_region_init_ram(readonly) by memory_region_init_rom()
+@@
+expression E1, E2, E3, E4, E5;
+symbol true;
+@@
+(
+- memory_region_init_ram(E1, E2, E3, E4, E5);
++ memory_region_init_rom(E1, E2, E3, E4, E5);
+  ... WHEN != E1
+- memory_region_set_readonly(E1, true);
+|
+- memory_region_init_ram_nomigrate(E1, E2, E3, E4, E5);
++ memory_region_init_rom_nomigrate(E1, E2, E3, E4, E5);
+  ... WHEN != E1
+- memory_region_set_readonly(E1, true);
+)
+
+
 // Replace by-hand memory_region_init_ram_nomigrate/vmstate_register_ram
 // code sequences with use of the new memory_region_init_ram function.
 // Similarly for the _rom and _rom_device functions.
-- 
1.8.3.1

[PULL 59/61] hw/ppc/ppc405: Use memory_region_init_rom() with read-only regions

[Paolo Bonzini]

From: Philippe Mathieu-Daudé 

The scripts/coccinelle/memory-region-housekeeping.cocci reported:
* TODO 
[[view:./hw/ppc/ppc405_boards.c::face=ovl-face1::linb=195::colb=8::cole=30][potential
 use of memory_region_init_rom*() in  ./hw/ppc/ppc405_boards.c::195]]
* TODO 
[[view:./hw/ppc/ppc405_boards.c::face=ovl-face1::linb=464::colb=8::cole=30][potential
 use of memory_region_init_rom*() in  ./hw/ppc/ppc405_boards.c::464]]

We can indeed replace the memory_region_init_ram() and
memory_region_set_readonly() calls by memory_region_init_rom().

Acked-by: David Gibson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/ppc/ppc405_boards.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
index de93c40..e6bffb9 100644
--- a/hw/ppc/ppc405_boards.c
+++ b/hw/ppc/ppc405_boards.c
@@ -199,7 +199,7 @@ static void ref405ep_init(MachineState *machine)
 #endif
 {
 bios = g_new(MemoryRegion, 1);
-memory_region_init_ram(bios, NULL, "ef405ep.bios", BIOS_SIZE,
+memory_region_init_rom(bios, NULL, "ef405ep.bios", BIOS_SIZE,
_fatal);
 
 if (bios_name == NULL)
@@ -223,7 +223,6 @@ static void ref405ep_init(MachineState *machine)
 /* Avoid an uninitialized variable warning */
 bios_size = -1;
 }
-memory_region_set_readonly(bios, true);
 }
 /* Register FPGA */
 ref405ep_fpga_init(sysmem, 0xF030);
@@ -471,7 +470,7 @@ static void taihu_405ep_init(MachineState *machine)
 if (bios_name == NULL)
 bios_name = BIOS_FILENAME;
 bios = g_new(MemoryRegion, 1);
-memory_region_init_ram(bios, NULL, "taihu_405ep.bios", BIOS_SIZE,
+memory_region_init_rom(bios, NULL, "taihu_405ep.bios", BIOS_SIZE,
_fatal);
 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
 if (filename) {
@@ -489,7 +488,6 @@ static void taihu_405ep_init(MachineState *machine)
 error_report("Could not load PowerPC BIOS '%s'", bios_name);
 exit(1);
 }
-memory_region_set_readonly(bios, true);
 }
 /* Register Linux flash */
 dinfo = drive_get(IF_PFLASH, 0, fl_idx);
-- 
1.8.3.1

[PULL 03/61] misc: Replace zero-length arrays with flexible array member (automatic)

[Paolo Bonzini]

From: Philippe Mathieu-Daudé 

Description copied from Linux kernel commit from Gustavo A. R. Silva
(see [3]):

--v-- description start --v--

  The current codebase makes use of the zero-length array language
  extension to the C90 standard, but the preferred mechanism to
  declare variable-length types such as these ones is a flexible
  array member [1], introduced in C99:

  struct foo {
  int stuff;
  struct boo array[];
  };

  By making use of the mechanism above, we will get a compiler
  warning in case the flexible array does not occur last in the
  structure, which will help us prevent some kind of undefined
  behavior bugs from being unadvertenly introduced [2] to the
  Linux codebase from now on.

--^-- description end --^--

Do the similar housekeeping in the QEMU codebase (which uses
C99 since commit 7be41675f7cb).

All these instances of code were found with the help of the
following Coccinelle script:

  @@
  identifier s, m, a;
  type t, T;
  @@
   struct s {
  ...
  t m;
  -   T a[0];
  +   T a[];
  };
  @@
  identifier s, m, a;
  type t, T;
  @@
   struct s {
  ...
  t m;
  -   T a[0];
  +   T a[];
   } QEMU_PACKED;

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76497732932f
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?id=17642a2fbd2c1

Inspired-by: Gustavo A. R. Silva 
Reviewed-by: David Hildenbrand 
Signed-off-by: Philippe Mathieu-Daudé 
Signed-off-by: Paolo Bonzini 
---
 block/linux-aio.c |  2 +-
 bsd-user/qemu.h   |  2 +-
 contrib/libvhost-user/libvhost-user.h |  2 +-
 hw/acpi/nvdimm.c  |  6 +++---
 hw/dma/soc_dma.c  |  2 +-
 hw/i386/x86.c |  2 +-
 hw/m68k/bootinfo.h|  2 +-
 hw/misc/omap_l4.c |  2 +-
 hw/nvram/eeprom93xx.c |  2 +-
 hw/rdma/vmw/pvrdma_qp_ops.c   |  4 ++--
 hw/usb/dev-network.c  |  2 +-
 hw/usb/dev-smartcard-reader.c |  4 ++--
 hw/virtio/virtio.c|  4 ++--
 hw/xen/xen_pt.h   |  2 +-
 include/hw/acpi/acpi-defs.h   | 12 ++--
 include/hw/arm/smmu-common.h  |  2 +-
 include/hw/i386/intel_iommu.h |  3 ++-
 include/hw/virtio/virtio-iommu.h  |  2 +-
 include/sysemu/cryptodev.h|  2 +-
 include/tcg/tcg.h |  2 +-
 net/queue.c   |  2 +-
 pc-bios/s390-ccw/bootmap.h|  2 +-
 pc-bios/s390-ccw/sclp.h   |  2 +-
 tests/qtest/libqos/ahci.h |  2 +-
 24 files changed, 35 insertions(+), 34 deletions(-)

diff --git a/block/linux-aio.c b/block/linux-aio.c
index 91204a2..3c0527c 100644
--- a/block/linux-aio.c
+++ b/block/linux-aio.c
@@ -121,7 +121,7 @@ struct aio_ring {
 unsignedincompat_features;
 unsignedheader_length;  /* size of aio_ring */
 
-struct io_event io_events[0];
+struct io_event io_events[];
 };
 
 /**
diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index 09e8aed..f8bb1e5 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -95,7 +95,7 @@ typedef struct TaskState {
 struct sigqueue *first_free; /* first free siginfo queue entry */
 int signal_pending; /* non zero if a signal may be pending */
 
-uint8_t stack[0];
+uint8_t stack[];
 } __attribute__((aligned(16))) TaskState;
 
 void init_task_state(TaskState *ts);
diff --git a/contrib/libvhost-user/libvhost-user.h 
b/contrib/libvhost-user/libvhost-user.h
index 6fc8000..f30394f 100644
--- a/contrib/libvhost-user/libvhost-user.h
+++ b/contrib/libvhost-user/libvhost-user.h
@@ -286,7 +286,7 @@ typedef struct VuVirtqInflight {
 uint16_t used_idx;
 
 /* Used to track the state of each descriptor in descriptor table */
-VuDescStateSplit desc[0];
+VuDescStateSplit desc[];
 } VuVirtqInflight;
 
 typedef struct VuVirtqInflightDesc {
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 5219dd0..eb6a37b 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -485,7 +485,7 @@ struct NvdimmFuncGetLabelDataOut {
 /* the size of buffer filled by QEMU. */
 uint32_t len;
 uint32_t func_ret_status; /* return status code. */
-uint8_t out_buf[0]; /* the data got via Get Namesapce Label function. */
+uint8_t out_buf[]; /* the data got via Get Namesapce Label function. */
 } QEMU_PACKED;
 typedef struct NvdimmFuncGetLabelDataOut NvdimmFuncGetLabelDataOut;
 QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelDataOut) > NVDIMM_DSM_MEMORY_SIZE);
@@ -493,7 +493,7 @@ QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelDataOut) > 
NVDIMM_DSM_MEMORY_SIZE);
 struct NvdimmFuncSetLabelDataIn {
 uint32_t offset; /* the offset in the namespace label data area. */
 uint32_t length; /* the size of data is to be written via the function. */
-uint8_t in_buf[0]; /* the data written to label data 

[PULL 14/61] configure: Fix building with SASL on Windows

[Paolo Bonzini]

From: Philippe Mathieu-Daudé 

The Simple Authentication and Security Layer (SASL) library
re-defines the struct iovec on Win32 [*]. QEMU also re-defines
it in "qemu/osdep.h". The two definitions then clash on a MinGW
build.
We can avoid the SASL definition by defining STRUCT_IOVEC_DEFINED.
Since QEMU already defines 'struct iovec' if it is missing, add
the definition to vnc_sasl_cflags to avoid SASL re-defining it.

[*] 
https://github.com/cyrusimap/cyrus-sasl/blob/cyrus-sasl-2.1.27/include/sasl.h#L187

Cc: Alexey Pavlov 
Cc: Biswapriyo Nath 
Reported-by: Youry Metlitsky 
Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20200309122454.22551-2-phi...@redhat.com>
Signed-off-by: Paolo Bonzini 
---
 configure | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index a7f2c3e..44a70cf 100755
--- a/configure
+++ b/configure
@@ -3367,7 +3367,9 @@ if test "$vnc" = "yes" && test "$vnc_sasl" != "no" ; then
 int main(void) { sasl_server_init(NULL, "qemu"); return 0; }
 EOF
   # Assuming Cyrus-SASL installed in /usr prefix
-  vnc_sasl_cflags=""
+  # QEMU defines struct iovec in "qemu/osdep.h",
+  # we don't want libsasl to redefine it in .
+  vnc_sasl_cflags="-DSTRUCT_IOVEC_DEFINED"
   vnc_sasl_libs="-lsasl2"
   if compile_prog "$vnc_sasl_cflags" "$vnc_sasl_libs" ; then
 vnc_sasl=yes
-- 
1.8.3.1

[PULL 12/61] build-sys: do not make qemu-ga link with pixman

[Paolo Bonzini]

From: Marc-André Lureau 

Since commit d52c454aadcdae74506f315ebf8b58bb79a05573 ("contrib: add
vhost-user-gpu"), qemu-ga is linking with pixman.

This is because the Make-based build-system use a global namespace for
variables, and we rely on "main.o-libs" for different linking targets.

Note: this kind of variable clashing is hard to fix or prevent
currently.  meson should help, as declarations have a linear
dependency and doesn't rely so much on variables and clever tricks.

Note2: we have a lot of main.c (or other duplicated names!) in
tree. Imho, it would be annoying and a bad workaroud to rename all
those to avoid conflicts like I did here.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1811670

Signed-off-by: Marc-André Lureau 
Message-Id: <20200311160923.882474-1-marcandre.lur...@redhat.com>
Signed-off-by: Paolo Bonzini 
---
 contrib/vhost-user-gpu/Makefile.objs| 6 +++---
 contrib/vhost-user-gpu/{main.c => vhost-user-gpu.c} | 0
 2 files changed, 3 insertions(+), 3 deletions(-)
 rename contrib/vhost-user-gpu/{main.c => vhost-user-gpu.c} (100%)

diff --git a/contrib/vhost-user-gpu/Makefile.objs 
b/contrib/vhost-user-gpu/Makefile.objs
index 6170c91..0929609 100644
--- a/contrib/vhost-user-gpu/Makefile.objs
+++ b/contrib/vhost-user-gpu/Makefile.objs
@@ -1,7 +1,7 @@
-vhost-user-gpu-obj-y = main.o virgl.o vugbm.o
+vhost-user-gpu-obj-y = vhost-user-gpu.o virgl.o vugbm.o
 
-main.o-cflags := $(PIXMAN_CFLAGS) $(GBM_CFLAGS)
-main.o-libs := $(PIXMAN_LIBS)
+vhost-user-gpu.o-cflags := $(PIXMAN_CFLAGS) $(GBM_CFLAGS)
+vhost-user-gpu.o-libs := $(PIXMAN_LIBS)
 
 virgl.o-cflags := $(VIRGL_CFLAGS) $(GBM_CFLAGS)
 virgl.o-libs := $(VIRGL_LIBS)
diff --git a/contrib/vhost-user-gpu/main.c 
b/contrib/vhost-user-gpu/vhost-user-gpu.c
similarity index 100%
rename from contrib/vhost-user-gpu/main.c
rename to contrib/vhost-user-gpu/vhost-user-gpu.c
-- 
1.8.3.1

[PULL 35/61] lockable: add lock guards

[Paolo Bonzini]

From: Stefan Hajnoczi 

This patch introduces two lock guard macros that automatically unlock a
lock object (QemuMutex and others):

  void f(void) {
  QEMU_LOCK_GUARD();
  if (!may_fail()) {
  return; /* automatically unlocks mutex */
  }
  ...
  }

and:

  WITH_QEMU_LOCK_GUARD() {
  if (!may_fail()) {
  return; /* automatically unlocks mutex */
  }
  }
  /* automatically unlocks mutex here */
  ...

Convert qemu-timer.c functions that benefit from these macros as an
example.  Manual qemu_mutex_lock/unlock() callers are left unmodified in
cases where clarity would not improve by switching to the macros.

Many other QemuMutex users remain in the codebase that might benefit
from lock guards.  Over time they can be converted, if that is
desirable.

Signed-off-by: Stefan Hajnoczi 
Signed-off-by: Paolo Bonzini 
---
 include/qemu/lockable.h | 65 +
 util/qemu-timer.c   | 23 +
 2 files changed, 76 insertions(+), 12 deletions(-)

diff --git a/include/qemu/lockable.h b/include/qemu/lockable.h
index 84ea794..2b52c7c 100644
--- a/include/qemu/lockable.h
+++ b/include/qemu/lockable.h
@@ -93,4 +93,69 @@ static inline void qemu_lockable_unlock(QemuLockable *x)
 x->unlock(x->object);
 }
 
+static inline QemuLockable *qemu_lockable_auto_lock(QemuLockable *x)
+{
+qemu_lockable_lock(x);
+return x;
+}
+
+static inline void qemu_lockable_auto_unlock(QemuLockable *x)
+{
+if (x) {
+qemu_lockable_unlock(x);
+}
+}
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(QemuLockable, qemu_lockable_auto_unlock)
+
+#define WITH_QEMU_LOCK_GUARD_(x, var) \
+for (g_autoptr(QemuLockable) var = \
+qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE((x))); \
+ var; \
+ qemu_lockable_auto_unlock(var), var = NULL)
+
+/**
+ * WITH_QEMU_LOCK_GUARD - Lock a lock object for scope
+ *
+ * @x: a lock object (currently one of QemuMutex, CoMutex, QemuSpin).
+ *
+ * This macro defines a lock scope such that entering the scope takes the lock
+ * and leaving the scope releases the lock.  Return statements are allowed
+ * within the scope and release the lock.  Break and continue statements leave
+ * the scope early and release the lock.
+ *
+ *   WITH_QEMU_LOCK_GUARD() {
+ *   ...
+ *   if (error) {
+ *   return; <-- mutex is automatically unlocked
+ *   }
+ *
+ *   if (early_exit) {
+ *   break;  <-- leave this scope early
+ *   }
+ *   ...
+ *   }
+ */
+#define WITH_QEMU_LOCK_GUARD(x) \
+WITH_QEMU_LOCK_GUARD_((x), qemu_lockable_auto##__COUNTER__)
+
+/**
+ * QEMU_LOCK_GUARD - Lock an object until the end of the scope
+ *
+ * @x: a lock object (currently one of QemuMutex, CoMutex, QemuSpin).
+ *
+ * This macro takes a lock until the end of the scope.  Return statements
+ * release the lock.
+ *
+ *   ... <-- mutex not locked
+ *   QEMU_LOCK_GUARD(); <-- mutex locked from here onwards
+ *   ...
+ *   if (error) {
+ *   return; <-- mutex is automatically unlocked
+ *   }
+ */
+#define QEMU_LOCK_GUARD(x) \
+g_autoptr(QemuLockable) qemu_lockable_auto##__COUNTER__ = \
+qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE((x)))
+
 #endif
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index ef52d28..d548d3c 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu/main-loop.h"
 #include "qemu/timer.h"
+#include "qemu/lockable.h"
 #include "sysemu/replay.h"
 #include "sysemu/cpus.h"
 
@@ -186,13 +187,12 @@ bool timerlist_expired(QEMUTimerList *timer_list)
 return false;
 }
 
-qemu_mutex_lock(_list->active_timers_lock);
-if (!timer_list->active_timers) {
-qemu_mutex_unlock(_list->active_timers_lock);
-return false;
+WITH_QEMU_LOCK_GUARD(_list->active_timers_lock) {
+if (!timer_list->active_timers) {
+return false;
+}
+expire_time = timer_list->active_timers->expire_time;
 }
-expire_time = timer_list->active_timers->expire_time;
-qemu_mutex_unlock(_list->active_timers_lock);
 
 return expire_time <= qemu_clock_get_ns(timer_list->clock->type);
 }
@@ -225,13 +225,12 @@ int64_t timerlist_deadline_ns(QEMUTimerList *timer_list)
  * value but ->notify_cb() is called when the deadline changes.  Therefore
  * the caller should notice the change and there is no race condition.
  */
-qemu_mutex_lock(_list->active_timers_lock);
-if (!timer_list->active_timers) {
-qemu_mutex_unlock(_list->active_timers_lock);
-return -1;
+WITH_QEMU_LOCK_GUARD(_list->active_timers_lock) {
+if (!timer_list->active_timers) {
+return -1;
+}
+expire_time = timer_list->active_timers->expire_time;
 }
-expire_time = timer_list->active_timers->expire_time;
-qemu_mutex_unlock(_list->active_timers_lock);
 
 delta = expire_time - 

[PULL 49/61] hw/sparc: Use memory_region_init_rom() with read-only regions

[Paolo Bonzini]

From: Philippe Mathieu-Daudé 

This commit was produced with the Coccinelle script
scripts/coccinelle/memory-region-housekeeping.cocci.

Reviewed-by: KONRAD Frederic 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/sparc/leon3.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 5fa58aa..8f024da 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -255,8 +255,7 @@ static void leon3_generic_hw_init(MachineState *machine)
 
 /* Allocate BIOS */
 prom_size = 8 * MiB;
-memory_region_init_ram(prom, NULL, "Leon3.bios", prom_size, _fatal);
-memory_region_set_readonly(prom, true);
+memory_region_init_rom(prom, NULL, "Leon3.bios", prom_size, _fatal);
 memory_region_add_subregion(address_space_mem, LEON3_PROM_OFFSET, prom);
 
 /* Load boot prom */
-- 
1.8.3.1

[PULL 29/61] ui/curses: Move arrays to .heap to save 74KiB of .bss

[Paolo Bonzini]

From: Philippe Mathieu-Daudé 

We only need these arrays when using the curses display.
Move them from the .bss to the .heap (sizes reported on
x86_64 host: screen[] is 64KiB, vga_to_curses 7KiB).

Signed-off-by: Philippe Mathieu-Daudé 
Signed-off-by: Paolo Bonzini 
---
 ui/curses.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/ui/curses.c b/ui/curses.c
index 3bafc10..a59b23a 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -54,13 +54,13 @@ enum maybe_keycode {
 };
 
 static DisplayChangeListener *dcl;
-static console_ch_t screen[160 * 100];
+static console_ch_t *screen;
 static WINDOW *screenpad = NULL;
 static int width, height, gwidth, gheight, invalidate;
 static int px, py, sminx, sminy, smaxx, smaxy;
 
 static const char *font_charset = "CP437";
-static cchar_t vga_to_curses[256];
+static cchar_t *vga_to_curses;
 
 static void curses_update(DisplayChangeListener *dcl,
   int x, int y, int w, int h)
@@ -405,6 +405,8 @@ static void curses_refresh(DisplayChangeListener *dcl)
 static void curses_atexit(void)
 {
 endwin();
+g_free(vga_to_curses);
+g_free(screen);
 }
 
 /*
@@ -783,6 +785,8 @@ static void curses_display_init(DisplayState *ds, 
DisplayOptions *opts)
 if (opts->u.curses.charset) {
 font_charset = opts->u.curses.charset;
 }
+screen = g_new0(console_ch_t, 160 * 100);
+vga_to_curses = g_new0(cchar_t, 256);
 curses_setup();
 curses_keyboard_setup();
 atexit(curses_atexit);
-- 
1.8.3.1

[PULL 61/61] hw/arm: Let devices own the MemoryRegion they create

[Paolo Bonzini]

From: Philippe Mathieu-Daudé 

Avoid orphan memory regions being added in the /unattached QOM
container.

This commit was produced with the Coccinelle script
scripts/coccinelle/memory-region-housekeeping.cocci.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/arm/exynos4210.c| 12 ++--
 hw/arm/fsl-imx25.c | 10 +-
 hw/arm/fsl-imx31.c |  6 +++---
 hw/arm/fsl-imx6.c  |  6 +++---
 hw/arm/fsl-imx6ul.c|  9 +
 hw/arm/msf2-soc.c  |  6 +++---
 hw/arm/nrf51_soc.c |  2 +-
 hw/arm/stm32f205_soc.c |  8 
 hw/arm/stm32f405_soc.c |  9 +
 hw/arm/xlnx-zynqmp.c   | 11 +--
 10 files changed, 40 insertions(+), 39 deletions(-)

diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index 4e1fd7e..1f7253e 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -305,20 +305,20 @@ static void exynos4210_realize(DeviceState *socdev, Error 
**errp)
 /*** Memory ***/
 
 /* Chip-ID and OMR */
-memory_region_init_io(>chipid_mem, NULL, _chipid_and_omr_ops,
-NULL, "exynos4210.chipid", sizeof(chipid_and_omr));
+memory_region_init_io(>chipid_mem, OBJECT(socdev),
+  _chipid_and_omr_ops, NULL,
+  "exynos4210.chipid", sizeof(chipid_and_omr));
 memory_region_add_subregion(system_mem, EXYNOS4210_CHIPID_ADDR,
 >chipid_mem);
 
 /* Internal ROM */
-memory_region_init_rom(>irom_mem, NULL, "exynos4210.irom",
+memory_region_init_rom(>irom_mem, OBJECT(socdev), "exynos4210.irom",
EXYNOS4210_IROM_SIZE, _fatal);
 memory_region_add_subregion(system_mem, EXYNOS4210_IROM_BASE_ADDR,
 >irom_mem);
 /* mirror of iROM */
-memory_region_init_alias(>irom_alias_mem, NULL, "exynos4210.irom_alias",
- >irom_mem,
- 0,
+memory_region_init_alias(>irom_alias_mem, OBJECT(socdev),
+ "exynos4210.irom_alias", >irom_mem, 0,
  EXYNOS4210_IROM_SIZE);
 memory_region_add_subregion(system_mem, EXYNOS4210_IROM_MIRROR_BASE_ADDR,
 >irom_alias_mem);
diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index a3f829f..6f1a82c 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -303,16 +303,16 @@ static void fsl_imx25_realize(DeviceState *dev, Error 
**errp)
 }
 
 /* initialize 2 x 16 KB ROM */
-memory_region_init_rom(>rom[0], NULL,
-   "imx25.rom0", FSL_IMX25_ROM0_SIZE, );
+memory_region_init_rom(>rom[0], OBJECT(dev), "imx25.rom0",
+   FSL_IMX25_ROM0_SIZE, );
 if (err) {
 error_propagate(errp, err);
 return;
 }
 memory_region_add_subregion(get_system_memory(), FSL_IMX25_ROM0_ADDR,
 >rom[0]);
-memory_region_init_rom(>rom[1], NULL,
-   "imx25.rom1", FSL_IMX25_ROM1_SIZE, );
+memory_region_init_rom(>rom[1], OBJECT(dev), "imx25.rom1",
+   FSL_IMX25_ROM1_SIZE, );
 if (err) {
 error_propagate(errp, err);
 return;
@@ -331,7 +331,7 @@ static void fsl_imx25_realize(DeviceState *dev, Error 
**errp)
 >iram);
 
 /* internal RAM (128 KB) is aliased over 128 MB - 128 KB */
-memory_region_init_alias(>iram_alias, NULL, "imx25.iram_alias",
+memory_region_init_alias(>iram_alias, OBJECT(dev), "imx25.iram_alias",
  >iram, 0, FSL_IMX25_IRAM_ALIAS_SIZE);
 memory_region_add_subregion(get_system_memory(), FSL_IMX25_IRAM_ALIAS_ADDR,
 >iram_alias);
diff --git a/hw/arm/fsl-imx31.c b/hw/arm/fsl-imx31.c
index 55e90d1..8472d2e 100644
--- a/hw/arm/fsl-imx31.c
+++ b/hw/arm/fsl-imx31.c
@@ -206,7 +206,7 @@ static void fsl_imx31_realize(DeviceState *dev, Error 
**errp)
 }
 
 /* On a real system, the first 16k is a `secure boot rom' */
-memory_region_init_rom(>secure_rom, NULL, "imx31.secure_rom",
+memory_region_init_rom(>secure_rom, OBJECT(dev), "imx31.secure_rom",
FSL_IMX31_SECURE_ROM_SIZE, );
 if (err) {
 error_propagate(errp, err);
@@ -216,7 +216,7 @@ static void fsl_imx31_realize(DeviceState *dev, Error 
**errp)
 >secure_rom);
 
 /* There is also a 16k ROM */
-memory_region_init_rom(>rom, NULL, "imx31.rom",
+memory_region_init_rom(>rom, OBJECT(dev), "imx31.rom",
FSL_IMX31_ROM_SIZE, );
 if (err) {
 error_propagate(errp, err);
@@ -236,7 +236,7 @@ static void fsl_imx31_realize(DeviceState *dev, Error 
**errp)
 >iram);
 
 /* internal RAM (16 KB) is aliased over 256 MB - 16 KB */
-memory_region_init_alias(>iram_alias, NULL, "imx31.iram_alias",
+memory_region_init_alias(>iram_alias, OBJECT(dev), "imx31.iram_alias",
   

Re: [PATCH v2 4/8] ppc/spapr: Fix FWNMI machine check interrupt delivery

[Nicholas Piggin]

Cédric Le Goater's on March 17, 2020 3:59 am:
> On 3/16/20 3:26 PM, Nicholas Piggin wrote:
>> FWNMI machine check delivery misses a few things that will make it fail
>> with TCG at least (which we would like to allow in future to improve
>> testing).
> 
> I don't understand which issues are addressed in the patch.

The existing code does not compute hflags, at least.

There's a few possible other things, I didn't dig into qemu enough
to know if they might be a problem (e.g., reservation and TLB). I
figure it's better to keep these consistent.

Keep in mind this is a bit academic right now, because we can't
(AFAIKS) inject an MCE from TCG. It would be good to wire that up,
but I didn't get to it.

>> It's not nice to scatter interrupt delivery logic around the tree, so
>> move it to excp_helper.c and share code where possible.
> 
> It looks correct but this is touching the ugliest routine in the QEMU 
> PPC universe. I would split the patch in two to introduce the helper
> powerpc_set_excp_state().
> 
> It does not seem to need to be an inline also.

Yeah it's all pretty ugly. I didn't yet find a nice way to do
split things up that did not require a lot of code churn, but that
can come later.

Inline was just because powerpc_excp is inline, I didn't want to
change behaviour too much there (it obviously wants to do a lot of
constant propagation but maybe only on the case statement). Anyway
I just wanted to be minimal for now, it could be changed.

Thanks,
Nick

[PULL SUBSYSTEM qemu-pseries] pseries: Update SLOF firmware image

[Alexey Kardashevskiy]

The following changes since commit 33dead675695e596b7f32c72e6f6a20390e86d8a:

  pseries: Update SLOF firmware image (2020-03-13 17:50:44 +1100)

are available in the Git repository at:

  g...@github.com:aik/qemu.git tags/qemu-slof-20200317

for you to fetch changes up to b7cf539920376542f03df8337602c3b8974bd1a1:

  pseries: Update SLOF firmware image (2020-03-17 10:27:34 +1100)


Alexey Kardashevskiy (1):
  pseries: Update SLOF firmware image

 pc-bios/README   |   2 +-
 pc-bios/slof.bin | Bin 968848 -> 965008 bytes
 roms/SLOF|   2 +-
 3 files changed, 2 insertions(+), 2 deletions(-)


*** Note: this is not for master, this is for pseries

The only change here is moving the decision about rtas-size
to QEMU.

Re: [PATCH v2] MAINTAINERS: Mark the LatticeMico32 target as orphan

[Richard Henderson]

On 3/16/20 7:28 AM, Philippe Mathieu-Daudé wrote:
> Michael Walle expressed his desire to orphan the lm32 target [*]:
> 
>   I guess it is time to pull the plug. Mainly, because I have
>   no time for this anymore. I've always worked on this on my
>   spare time and life changed. And secondly, I guess RISC-V is
>   taking over ;) It has a far better ecosystem. Also, to my
>   knowledge the only (public) user of LM32 is milkymist and this
>   project is dead for years now..
> 
>   So time to say goodbye. It was fun and I've learned a lot -
>   technically and also how a huge open source project works.
>   Thank you everyone for that :)
> 
>   Basically everything still works and there are even TCG test
>   cases which covers all instructions the processor has.
> 
> Many thanks to Michael for his substantial contributions to QEMU,
> and for maintaining the LM32 target for various years!
> 
> [*] https://www.mail-archive.com/qemu-devel@nongnu.org/msg605024.html
> 
> Acked-by: Michael Walle 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> v2: Also orphan machines, added Michael A-b tag
> ---
>  MAINTAINERS | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [PULL 00/61] Misc patches for soft freeze

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/1584394048-44994-1-git-send-email-pbonz...@redhat.com/



Hi,

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

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

PASS 1 fdc-test /x86_64/fdc/cmos
PASS 2 fdc-test /x86_64/fdc/no_media_on_start
PASS 3 fdc-test /x86_64/fdc/read_without_media
==11429==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 4 fdc-test /x86_64/fdc/media_change
PASS 5 fdc-test /x86_64/fdc/sense_interrupt
PASS 6 fdc-test /x86_64/fdc/relative_seek
---
PASS 32 test-opts-visitor /visitor/opts/range/beyond
PASS 33 test-opts-visitor /visitor/opts/dict/unvisited
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}  
tests/test-coroutine -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl 
--test-name="test-coroutine" 
==11484==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
==11484==WARNING: ASan is ignoring requested __asan_handle_no_return: stack 
top: 0x7ffda0e6; bottom 0x7fa1c9d2; size: 0x005bd714 (394450436096)
False positive error reports may follow
For details see https://github.com/google/sanitizers/issues/189
PASS 1 test-coroutine /basic/no-dangling-access
---
PASS 12 fdc-test /x86_64/fdc/read_no_dma_19
PASS 13 fdc-test /x86_64/fdc/fuzz-registers
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}  
QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 QTEST_QEMU_IMG=qemu-img 
tests/qtest/ide-test -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl 
--test-name="ide-test" 
==11499==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 14 test-aio /aio/timer/schedule
PASS 15 test-aio /aio/coroutine/queue-chaining
PASS 16 test-aio /aio-gsource/flush
---
PASS 25 test-aio /aio-gsource/event/wait
PASS 26 test-aio /aio-gsource/event/flush
PASS 27 test-aio /aio-gsource/event/wait/no-flush-cb
==11507==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 1 ide-test /x86_64/ide/identify
==11513==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 28 test-aio /aio-gsource/timer/schedule
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}  
tests/test-aio-multithread -m=quick -k --tap < /dev/null | 
./scripts/tap-driver.pl --test-name="test-aio-multithread" 
==11520==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 1 test-aio-multithread /aio/multi/lifecycle
PASS 2 ide-test /x86_64/ide/flush
==11533==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 2 test-aio-multithread /aio/multi/schedule
PASS 3 ide-test /x86_64/ide/bmdma/simple_rw
==11544==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 4 ide-test /x86_64/ide/bmdma/trim
PASS 3 test-aio-multithread /aio/multi/mutex/contended
==11550==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 4 test-aio-multithread /aio/multi/mutex/handoff
PASS 5 test-aio-multithread /aio/multi/mutex/mcs
PASS 6 test-aio-multithread /aio/multi/mutex/pthread
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}  
tests/test-throttle -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl 
--test-name="test-throttle" 
==11572==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 1 test-throttle /throttle/leak_bucket
PASS 2 test-throttle /throttle/compute_wait
PASS 3 test-throttle /throttle/init
---
PASS 14 test-throttle /throttle/config/max
PASS 15 test-throttle /throttle/config/iops_size
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}  
tests/test-thread-pool -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl 
--test-name="test-thread-pool" 
==11576==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 1 test-thread-pool /thread-pool/submit
PASS 2 test-thread-pool /thread-pool/submit-aio
PASS 3 test-thread-pool /thread-pool/submit-co
PASS 4 test-thread-pool /thread-pool/submit-many
==11643==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 5 test-thread-pool /thread-pool/cancel
PASS 6 test-thread-pool /thread-pool/cancel-async

[PATCH v8 09/11] iotest 258: use script_main

[John Snow]

Since this one is nicely factored to use a single entry point,
use script_main to run the tests.

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/258 | 11 ---
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/258 b/tests/qemu-iotests/258
index a65151dda6..e305a1502f 100755
--- a/tests/qemu-iotests/258
+++ b/tests/qemu-iotests/258
@@ -23,12 +23,6 @@ import iotests
 from iotests import log, qemu_img, qemu_io_silent, \
 filter_qmp_testfiles, filter_qmp_imgfmt
 
-# Need backing file and change-backing-file support
-iotests.script_initialize(
-supported_fmts=['qcow2', 'qed'],
-supported_platforms=['linux'],
-)
-
 # Returns a node for blockdev-add
 def node(node_name, path, backing=None, fmt=None, throttle=None):
 if fmt is None:
@@ -161,4 +155,7 @@ def main():
 test_concurrent_finish(False)
 
 if __name__ == '__main__':
-main()
+# Need backing file and change-backing-file support
+iotests.script_main(main,
+supported_fmts=['qcow2', 'qed'],
+supported_platforms=['linux'])
-- 
2.21.1

[PATCH v8 01/11] iotests: do a light delinting

[John Snow]

This doesn't fix everything in here, but it does help clean up the
pylint report considerably.

This should be 100% style changes only; the intent is to make pylint
more useful by working on establishing a baseline for iotests that we
can gate against in the future.

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
---
 tests/qemu-iotests/iotests.py | 83 ++-
 1 file changed, 43 insertions(+), 40 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 23043baa26..c6d9ae130d 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -16,11 +16,9 @@
 # along with this program.  If not, see .
 #
 
-import errno
 import os
 import re
 import subprocess
-import string
 import unittest
 import sys
 import struct
@@ -34,7 +32,7 @@
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu import qtest
 
-assert sys.version_info >= (3,6)
+assert sys.version_info >= (3, 6)
 
 # This will not work if arguments contain spaces but is necessary if we
 # want to support the override options that ./check supports.
@@ -138,11 +136,11 @@ def qemu_img_log(*args):
 return result
 
 def img_info_log(filename, filter_path=None, imgopts=False, extra_args=[]):
-args = [ 'info' ]
+args = ['info']
 if imgopts:
 args.append('--image-opts')
 else:
-args += [ '-f', imgfmt ]
+args += ['-f', imgfmt]
 args += extra_args
 args.append(filename)
 
@@ -221,7 +219,7 @@ def cmd(self, cmd):
 # quit command is in close(), '\n' is added automatically
 assert '\n' not in cmd
 cmd = cmd.strip()
-assert cmd != 'q' and cmd != 'quit'
+assert cmd not in ('q', 'quit')
 self._p.stdin.write(cmd + '\n')
 self._p.stdin.flush()
 return self._read_output()
@@ -243,10 +241,8 @@ def qemu_nbd_early_pipe(*args):
 sys.stderr.write('qemu-nbd received signal %i: %s\n' %
  (-exitcode,
   ' '.join(qemu_nbd_args + ['--fork'] + list(args
-if exitcode == 0:
-return exitcode, ''
-else:
-return exitcode, subp.communicate()[0]
+
+return exitcode, subp.communicate()[0] if exitcode else ''
 
 def qemu_nbd_popen(*args):
 '''Run qemu-nbd in daemon mode and return the parent's exit code'''
@@ -310,7 +306,7 @@ def filter_qmp(qmsg, filter_fn):
 items = qmsg.items()
 
 for k, v in items:
-if isinstance(v, list) or isinstance(v, dict):
+if isinstance(v, (dict, list)):
 qmsg[k] = filter_qmp(v, filter_fn)
 else:
 qmsg[k] = filter_fn(k, v)
@@ -321,7 +317,7 @@ def filter_testfiles(msg):
 return msg.replace(prefix, 'TEST_DIR/PID-')
 
 def filter_qmp_testfiles(qmsg):
-def _filter(key, value):
+def _filter(_key, value):
 if is_str(value):
 return filter_testfiles(value)
 return value
@@ -347,7 +343,7 @@ def filter_imgfmt(msg):
 return msg.replace(imgfmt, 'IMGFMT')
 
 def filter_qmp_imgfmt(qmsg):
-def _filter(key, value):
+def _filter(_key, value):
 if is_str(value):
 return filter_imgfmt(value)
 return value
@@ -358,7 +354,7 @@ def log(msg, filters=[], indent=None):
 If indent is provided, JSON serializable messages are pretty-printed.'''
 for flt in filters:
 msg = flt(msg)
-if isinstance(msg, dict) or isinstance(msg, list):
+if isinstance(msg, (dict, list)):
 # Python < 3.4 needs to know not to add whitespace when 
pretty-printing:
 separators = (', ', ': ') if indent is None else (',', ': ')
 # Don't sort if it's already sorted
@@ -369,14 +365,14 @@ def log(msg, filters=[], indent=None):
 print(msg)
 
 class Timeout:
-def __init__(self, seconds, errmsg = "Timeout"):
+def __init__(self, seconds, errmsg="Timeout"):
 self.seconds = seconds
 self.errmsg = errmsg
 def __enter__(self):
 signal.signal(signal.SIGALRM, self.timeout)
 signal.setitimer(signal.ITIMER_REAL, self.seconds)
 return self
-def __exit__(self, type, value, traceback):
+def __exit__(self, exc_type, value, traceback):
 signal.setitimer(signal.ITIMER_REAL, 0)
 return False
 def timeout(self, signum, frame):
@@ -385,7 +381,7 @@ def timeout(self, signum, frame):
 def file_pattern(name):
 return "{0}-{1}".format(os.getpid(), name)
 
-class FilePaths(object):
+class FilePaths:
 """
 FilePaths is an auto-generated filename that cleans itself up.
 
@@ -532,11 +528,11 @@ def pause_drive(self, drive, event=None):
 self.pause_drive(drive, "write_aio")
 return
 self.qmp('human-monitor-command',
-command_line='qemu-io %s "break %s bp_%s"' % (drive, 
event, drive))
+ command_line='qemu-io %s "break %s bp_%s"' % 

[PATCH v8 03/11] iotests: ignore import warnings from pylint

[John Snow]

The right way to solve this is to come up with a virtual environment
infrastructure that sets all the paths correctly, and/or to create
installable python modules that can be imported normally.

That's hard, so just silence this error for now.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index a6b2889932..cb9c2cd05d 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -29,6 +29,7 @@
 import io
 from collections import OrderedDict
 
+# pylint: disable=import-error, wrong-import-position
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu import qtest
 
-- 
2.21.1

[PATCH v8 04/11] iotests: replace mutable list default args

[John Snow]

It's bad hygiene: if we modify this list, it will be modified across all
invocations.

(Remaining bad usages are fixed in a subsequent patch which changes the
function signature anyway.)

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
---
 tests/qemu-iotests/iotests.py | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index cb9c2cd05d..7cd74e7cb1 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -136,7 +136,7 @@ def qemu_img_log(*args):
 log(result, filters=[filter_testfiles])
 return result
 
-def img_info_log(filename, filter_path=None, imgopts=False, extra_args=[]):
+def img_info_log(filename, filter_path=None, imgopts=False, extra_args=()):
 args = ['info']
 if imgopts:
 args.append('--image-opts')
@@ -350,7 +350,7 @@ def _filter(_key, value):
 return value
 return filter_qmp(qmsg, _filter)
 
-def log(msg, filters=[], indent=None):
+def log(msg, filters=(), indent=None):
 '''Logs either a string message or a JSON serializable message (like QMP).
 If indent is provided, JSON serializable messages are pretty-printed.'''
 for flt in filters:
@@ -566,7 +566,7 @@ def get_qmp_events_filtered(self, wait=60.0):
 result.append(filter_qmp_event(ev))
 return result
 
-def qmp_log(self, cmd, filters=[], indent=None, **kwargs):
+def qmp_log(self, cmd, filters=(), indent=None, **kwargs):
 full_cmd = OrderedDict((
 ("execute", cmd),
 ("arguments", ordered_qmp(kwargs))
@@ -970,7 +970,7 @@ def case_notrun(reason):
 open('%s/%s.casenotrun' % (output_dir, seq), 'a').write(
 '[case not run] ' + reason + '\n')
 
-def verify_image_format(supported_fmts=[], unsupported_fmts=[]):
+def verify_image_format(supported_fmts=(), unsupported_fmts=()):
 assert not (supported_fmts and unsupported_fmts)
 
 if 'generic' in supported_fmts and \
@@ -984,7 +984,7 @@ def verify_image_format(supported_fmts=[], 
unsupported_fmts=[]):
 if not_sup or (imgfmt in unsupported_fmts):
 notrun('not suitable for this image format: %s' % imgfmt)
 
-def verify_protocol(supported=[], unsupported=[]):
+def verify_protocol(supported=(), unsupported=()):
 assert not (supported and unsupported)
 
 if 'generic' in supported:
@@ -1003,11 +1003,11 @@ def verify_platform(supported=None, unsupported=None):
 if not any((sys.platform.startswith(x) for x in supported)):
 notrun('not suitable for this OS: %s' % sys.platform)
 
-def verify_cache_mode(supported_cache_modes=[]):
+def verify_cache_mode(supported_cache_modes=()):
 if supported_cache_modes and (cachemode not in supported_cache_modes):
 notrun('not suitable for this cache mode: %s' % cachemode)
 
-def verify_aio_mode(supported_aio_modes=[]):
+def verify_aio_mode(supported_aio_modes=()):
 if supported_aio_modes and (aiomode not in supported_aio_modes):
 notrun('not suitable for this aio mode: %s' % aiomode)
 
@@ -1047,7 +1047,7 @@ def supported_formats(read_only=False):
 
 return supported_formats.formats[read_only]
 
-def skip_if_unsupported(required_formats=[], read_only=False):
+def skip_if_unsupported(required_formats=(), read_only=False):
 '''Skip Test Decorator
Runs the test if all the required formats are whitelisted'''
 def skip_test_decorator(func):
@@ -1098,11 +1098,11 @@ def execute_unittest(output, verbosity, debug):
 sys.stderr.write(out)
 
 def execute_test(test_function=None,
- supported_fmts=[],
+ supported_fmts=(),
  supported_platforms=None,
- supported_cache_modes=[], supported_aio_modes={},
- unsupported_fmts=[], supported_protocols=[],
- unsupported_protocols=[]):
+ supported_cache_modes=(), supported_aio_modes=(),
+ unsupported_fmts=(), supported_protocols=(),
+ unsupported_protocols=()):
 """Run either unittest or script-style tests."""
 
 # We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
-- 
2.21.1

[PATCH v8 00/11] iotests: use python logging

[John Snow]

This series uses python logging to enable output conditionally on
iotests.log(). We unify an initialization call (which also enables
debugging output for those tests with -d) and then make the switch
inside of iotests.

It will help alleviate the need to create logged/unlogged versions
of all the various helpers we have made.

Also, I got lost and accidentally delinted iotests while I was here.
Sorry about that.

V8:

Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/11:[] [--] 'iotests: do a light delinting'
002/11:[] [--] 'iotests: don't use 'format' for drive_add'
003/11:[] [--] 'iotests: ignore import warnings from pylint'
004/11:[] [--] 'iotests: replace mutable list default args'
005/11:[] [--] 'iotests: add pylintrc file'
006/11:[down] 'iotests: drop Python 3.4 compatibility code'
007/11:[0033] [FC] 'iotests: limit line length to 79 chars'
008/11:[] [--] 'iotests: add script_initialize'
009/11:[] [--] 'iotest 258: use script_main'
010/11:[] [--] 'iotests: Mark verify functions as private'
011/11:[0007] [FC] 'iotests: use python logging for iotests.log()'

6: Split out the little drop of Python 3.4 code. (Phil)
7: Change line continuation styles (QEMU Memorial Choir)
11: Rebase changes; remove use_log from more places, adjust test output.

V7:

- All delinting patches are now entirely front-loaded.
- Redid delinting to avoid "correcting" no-else-return statements.
- Moved more mutable list corrections into patch 4, to make it standalone.
- Moved pylintrc up to patch 5. Disabled no-else-return.
- Added patch 6 to require line length checks.
  (Some python 3.4 compatibility code is removed as a consequence.)
- Patch 7 changes slightly as a result of patch 4 changes.
- Added some logging explainer into patch 10.
  (Patch changes slightly because of patch 6.)

V6:
 - It's been so long since V5, let's just look at it anew.
 - Dropped patch 1, rebased, added more delinting.
 - I'm not touching the supported_platforms thing.
   Not interested in rehashing that debate.

V5:
 - Rebased again
 - Allow Python tests to run on any platform

V4:
 - Rebased on top of kwolf/block at the behest of mreitz

V3:
 - Rebased for 4.1+; now based on main branch.

V2:
 - Added all of the other python tests I missed to use script_initialize
 - Refactored the common setup as per Ehabkost's suggestion
 - Added protocol arguments to common initialization,
   but this isn't strictly required.

John Snow (11):
  iotests: do a light delinting
  iotests: don't use 'format' for drive_add
  iotests: ignore import warnings from pylint
  iotests: replace mutable list default args
  iotests: add pylintrc file
  iotests: drop Python 3.4 compatibility code
  iotests: limit line length to 79 chars
  iotests: add script_initialize
  iotest 258: use script_main
  iotests: Mark verify functions as private
  iotests: use python logging for iotests.log()

 tests/qemu-iotests/030|   4 +-
 tests/qemu-iotests/055|   3 +-
 tests/qemu-iotests/149|   3 +-
 tests/qemu-iotests/155|   2 +-
 tests/qemu-iotests/194|   4 +-
 tests/qemu-iotests/202|   4 +-
 tests/qemu-iotests/203|   4 +-
 tests/qemu-iotests/206|   2 +-
 tests/qemu-iotests/207|   6 +-
 tests/qemu-iotests/208|   2 +-
 tests/qemu-iotests/209|   2 +-
 tests/qemu-iotests/210|   6 +-
 tests/qemu-iotests/211|   6 +-
 tests/qemu-iotests/212|   6 +-
 tests/qemu-iotests/213|   6 +-
 tests/qemu-iotests/216|   4 +-
 tests/qemu-iotests/218|   2 +-
 tests/qemu-iotests/219|   2 +-
 tests/qemu-iotests/222|   7 +-
 tests/qemu-iotests/224|   4 +-
 tests/qemu-iotests/228|   6 +-
 tests/qemu-iotests/234|   4 +-
 tests/qemu-iotests/235|   4 +-
 tests/qemu-iotests/236|   2 +-
 tests/qemu-iotests/237|   2 +-
 tests/qemu-iotests/238|   2 +
 tests/qemu-iotests/242|   2 +-
 tests/qemu-iotests/245|   1 +
 tests/qemu-iotests/245.out|  24 +--
 tests/qemu-iotests/246|   2 +-
 tests/qemu-iotests/248|   2 +-
 tests/qemu-iotests/254|   2 +-
 tests/qemu-iotests/255|   2 +-
 tests/qemu-iotests/256|   2 +-
 tests/qemu-iotests/258|  10 +-
 tests/qemu-iotests/260|   4 +-
 tests/qemu-iotests/262|   4 +-
 tests/qemu-iotests/264|   4 +-
 tests/qemu-iotests/277|   2 +
 tests/qemu-iotests/280|   8 +-
 tests/qemu-iotests/283|   4 +-
 tests/qemu-iotests/iotests.py | 312 --
 tests/qemu-iotests/pylintrc   |  26 +++
 43 files changed, 307 insertions(+), 203 deletions(-)
 create mode 100644 tests/qemu-iotests/pylintrc

-- 
2.21.1

[PATCH v8 02/11] iotests: don't use 'format' for drive_add

[John Snow]

It shadows (with a different type) the built-in format.
Use something else.

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/055| 3 ++-
 tests/qemu-iotests/iotests.py | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index 82b9f5f47d..4175fff5e4 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -469,7 +469,8 @@ class TestDriveCompression(iotests.QMPTestCase):
 qemu_img('create', '-f', fmt, blockdev_target_img,
  str(TestDriveCompression.image_len), *args)
 if attach_target:
-self.vm.add_drive(blockdev_target_img, format=fmt, 
interface="none")
+self.vm.add_drive(blockdev_target_img,
+  img_format=fmt, interface="none")
 
 self.vm.launch()
 
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index c6d9ae130d..a6b2889932 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -482,21 +482,21 @@ def add_drive_raw(self, opts):
 self._args.append(opts)
 return self
 
-def add_drive(self, path, opts='', interface='virtio', format=imgfmt):
+def add_drive(self, path, opts='', interface='virtio', img_format=imgfmt):
 '''Add a virtio-blk drive to the VM'''
 options = ['if=%s' % interface,
'id=drive%d' % self._num_drives]
 
 if path is not None:
 options.append('file=%s' % path)
-options.append('format=%s' % format)
+options.append('format=%s' % img_format)
 options.append('cache=%s' % cachemode)
 options.append('aio=%s' % aiomode)
 
 if opts:
 options.append(opts)
 
-if format == 'luks' and 'key-secret' not in opts:
+if img_format == 'luks' and 'key-secret' not in opts:
 # default luks support
 if luks_default_secret_object not in self._args:
 self.add_object(luks_default_secret_object)
-- 
2.21.1

[PATCH v8 11/11] iotests: use python logging for iotests.log()

[John Snow]

We can turn logging on/off globally instead of per-function.

Remove use_log from run_job, and use python logging to turn on
diffable output when we run through a script entry point.

iotest 245 changes output order due to buffering reasons.


An extended note on python logging:

A NullHandler is added to `qemu.iotests` to stop output from being
generated if this code is used as a library without configuring logging.
A NullHandler is only needed at the root, so a duplicate handler is not
needed for `qemu.iotests.diff_io`.

When logging is not configured, messages at the 'WARNING' levels or
above are printed with default settings. The NullHandler stops this from
occurring, which is considered good hygiene for code used as a library.

See https://docs.python.org/3/howto/logging.html#library-config

When logging is actually enabled (always at the behest of an explicit
call by a client script), a root logger is implicitly created at the
root, which allows messages to propagate upwards and be handled/emitted
from the root logger with default settings.

When we want iotest logging, we attach a handler to the
qemu.iotests.diff_io logger and disable propagation to avoid possible
double-printing.

For more information on python logging infrastructure, I highly
recommend downloading the pip package `logging_tree`, which provides
convenient visualizations of the hierarchical logging configuration
under different circumstances.

See https://pypi.org/project/logging_tree/ for more information.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/030|  4 +--
 tests/qemu-iotests/155|  2 +-
 tests/qemu-iotests/245|  1 +
 tests/qemu-iotests/245.out| 24 
 tests/qemu-iotests/iotests.py | 53 ---
 5 files changed, 46 insertions(+), 38 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index aa911d266a..104e3cee1b 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -411,8 +411,8 @@ class TestParallelOps(iotests.QMPTestCase):
 result = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
 self.assert_qmp(result, 'return', {})
 
-self.vm.run_job(job='drive0', auto_dismiss=True, use_log=False)
-self.vm.run_job(job='node4', auto_dismiss=True, use_log=False)
+self.vm.run_job(job='drive0', auto_dismiss=True)
+self.vm.run_job(job='node4', auto_dismiss=True)
 self.assert_no_active_block_jobs()
 
 # Test a block-stream and a block-commit job in parallel
diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
index 571bce9de4..cb371d4649 100755
--- a/tests/qemu-iotests/155
+++ b/tests/qemu-iotests/155
@@ -188,7 +188,7 @@ class MirrorBaseClass(BaseClass):
 
 self.assert_qmp(result, 'return', {})
 
-self.vm.run_job('mirror-job', use_log=False, auto_finalize=False,
+self.vm.run_job('mirror-job', auto_finalize=False,
 pre_finalize=self.openBacking, auto_dismiss=True)
 
 def testFull(self):
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 1001275a44..4f5f0bb901 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1027,5 +1027,6 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 self.run_test_iothreads(None, 'iothread0')
 
 if __name__ == '__main__':
+iotests.activate_logging()
 iotests.main(supported_fmts=["qcow2"],
  supported_protocols=["file"])
diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
index 682b93394d..4b33dcaf5c 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -1,17 +1,17 @@
+{"execute": "job-finalize", "arguments": {"id": "commit0"}}
+{"return": {}}
+{"data": {"id": "commit0", "type": "commit"}, "event": "BLOCK_JOB_PENDING", 
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "commit0", "len": 3145728, "offset": 3145728, "speed": 0, 
"type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "stream0"}}
+{"return": {}}
+{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", 
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, 
"type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "stream0"}}
+{"return": {}}
+{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", 
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, 
"type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
 .
 --
 Ran 21 tests
 
 OK

[PATCH v8 10/11] iotests: Mark verify functions as private

[John Snow]

Mark the verify functions as "private" with a leading underscore, to
discourage their use.

(Also, make pending patches not yet using the new entry points fail in a
very obvious way.)

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index ec7aa42a70..7d6cc430a4 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -988,7 +988,7 @@ def case_notrun(reason):
 open('%s/%s.casenotrun' % (output_dir, seq), 'a').write(
 '[case not run] ' + reason + '\n')
 
-def verify_image_format(supported_fmts=(), unsupported_fmts=()):
+def _verify_image_format(supported_fmts=(), unsupported_fmts=()):
 assert not (supported_fmts and unsupported_fmts)
 
 if 'generic' in supported_fmts and \
@@ -1002,7 +1002,7 @@ def verify_image_format(supported_fmts=(), 
unsupported_fmts=()):
 if not_sup or (imgfmt in unsupported_fmts):
 notrun('not suitable for this image format: %s' % imgfmt)
 
-def verify_protocol(supported=(), unsupported=()):
+def _verify_protocol(supported=(), unsupported=()):
 assert not (supported and unsupported)
 
 if 'generic' in supported:
@@ -1012,7 +1012,7 @@ def verify_protocol(supported=(), unsupported=()):
 if not_sup or (imgproto in unsupported):
 notrun('not suitable for this protocol: %s' % imgproto)
 
-def verify_platform(supported=(), unsupported=()):
+def _verify_platform(supported=(), unsupported=()):
 if any((sys.platform.startswith(x) for x in unsupported)):
 notrun('not suitable for this OS: %s' % sys.platform)
 
@@ -1020,11 +1020,11 @@ def verify_platform(supported=(), unsupported=()):
 if not any((sys.platform.startswith(x) for x in supported)):
 notrun('not suitable for this OS: %s' % sys.platform)
 
-def verify_cache_mode(supported_cache_modes=()):
+def _verify_cache_mode(supported_cache_modes=()):
 if supported_cache_modes and (cachemode not in supported_cache_modes):
 notrun('not suitable for this cache mode: %s' % cachemode)
 
-def verify_aio_mode(supported_aio_modes=()):
+def _verify_aio_mode(supported_aio_modes=()):
 if supported_aio_modes and (aiomode not in supported_aio_modes):
 notrun('not suitable for this aio mode: %s' % aiomode)
 
@@ -1151,11 +1151,11 @@ def execute_setup_common(supported_fmts: 
Collection[str] = (),
 sys.stderr.write('Please run this test via the "check" script\n')
 sys.exit(os.EX_USAGE)
 
-verify_image_format(supported_fmts, unsupported_fmts)
-verify_protocol(supported_protocols, unsupported_protocols)
-verify_platform(supported=supported_platforms)
-verify_cache_mode(supported_cache_modes)
-verify_aio_mode(supported_aio_modes)
+_verify_image_format(supported_fmts, unsupported_fmts)
+_verify_protocol(supported_protocols, unsupported_protocols)
+_verify_platform(supported=supported_platforms)
+_verify_cache_mode(supported_cache_modes)
+_verify_aio_mode(supported_aio_modes)
 
 debug = '-d' in sys.argv
 if debug:
-- 
2.21.1

[PATCH v8 05/11] iotests: add pylintrc file

[John Snow]

This allows others to get repeatable results with pylint. If you run
`pylint iotests.py`, you should see a 100% pass.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/pylintrc | 22 ++
 1 file changed, 22 insertions(+)
 create mode 100644 tests/qemu-iotests/pylintrc

diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
new file mode 100644
index 00..8720b6a0de
--- /dev/null
+++ b/tests/qemu-iotests/pylintrc
@@ -0,0 +1,22 @@
+[MESSAGES CONTROL]
+
+# Disable the message, report, category or checker with the given id(s). You
+# can either give multiple identifiers separated by comma (,) or put this
+# option multiple times (only on the command line, not in the configuration
+# file where it should appear only once). You can also use "--disable=all" to
+# disable everything first and then reenable specific checks. For example, if
+# you want to run only the similarities checker, you can use "--disable=all
+# --enable=similarities". If you want to run only the classes checker, but have
+# no Warning level messages displayed, use "--disable=all --enable=classes
+# --disable=W".
+disable=invalid-name,
+no-else-return,
+too-many-lines,
+too-few-public-methods,
+too-many-arguments,
+too-many-locals,
+too-many-branches,
+too-many-public-methods,
+# These are temporary, and should be removed:
+missing-docstring,
+line-too-long,
-- 
2.21.1

[PATCH v8 06/11] iotests: drop Python 3.4 compatibility code

[John Snow]

We no longer need to accommodate 3.4, drop this code.
(Also, the line is over 79 characters, so drop it.)

Touch up the docstring a little bit while we're here.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 7cd74e7cb1..3d90fb157d 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -22,6 +22,7 @@
 import unittest
 import sys
 import struct
+from typing import Optional
 import json
 import signal
 import logging
@@ -350,18 +351,17 @@ def _filter(_key, value):
 return value
 return filter_qmp(qmsg, _filter)
 
-def log(msg, filters=(), indent=None):
-'''Logs either a string message or a JSON serializable message (like QMP).
-If indent is provided, JSON serializable messages are pretty-printed.'''
+def log(msg, filters=(), indent: Optional[int] = None) -> None:
+"""
+Logs either a string message or a JSON serializable message (like QMP).
+If indent is provided, JSON serializable messages are pretty-printed.
+"""
 for flt in filters:
 msg = flt(msg)
 if isinstance(msg, (dict, list)):
-# Python < 3.4 needs to know not to add whitespace when 
pretty-printing:
-separators = (', ', ': ') if indent is None else (',', ': ')
 # Don't sort if it's already sorted
 do_sort = not isinstance(msg, OrderedDict)
-print(json.dumps(msg, sort_keys=do_sort,
- indent=indent, separators=separators))
+print(json.dumps(msg, sort_keys=do_sort, indent=indent))
 else:
 print(msg)
 
-- 
2.21.1

[PATCH v8 08/11] iotests: add script_initialize

[John Snow]

Like script_main, but doesn't require a single point of entry.
Replace all existing initialization sections with this drop-in replacement.

This brings debug support to all existing script-style iotests.

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/149|  3 +-
 tests/qemu-iotests/194|  4 +-
 tests/qemu-iotests/202|  4 +-
 tests/qemu-iotests/203|  4 +-
 tests/qemu-iotests/206|  2 +-
 tests/qemu-iotests/207|  6 ++-
 tests/qemu-iotests/208|  2 +-
 tests/qemu-iotests/209|  2 +-
 tests/qemu-iotests/210|  6 ++-
 tests/qemu-iotests/211|  6 ++-
 tests/qemu-iotests/212|  6 ++-
 tests/qemu-iotests/213|  6 ++-
 tests/qemu-iotests/216|  4 +-
 tests/qemu-iotests/218|  2 +-
 tests/qemu-iotests/219|  2 +-
 tests/qemu-iotests/222|  7 ++--
 tests/qemu-iotests/224|  4 +-
 tests/qemu-iotests/228|  6 ++-
 tests/qemu-iotests/234|  4 +-
 tests/qemu-iotests/235|  4 +-
 tests/qemu-iotests/236|  2 +-
 tests/qemu-iotests/237|  2 +-
 tests/qemu-iotests/238|  2 +
 tests/qemu-iotests/242|  2 +-
 tests/qemu-iotests/246|  2 +-
 tests/qemu-iotests/248|  2 +-
 tests/qemu-iotests/254|  2 +-
 tests/qemu-iotests/255|  2 +-
 tests/qemu-iotests/256|  2 +-
 tests/qemu-iotests/258|  7 ++--
 tests/qemu-iotests/260|  4 +-
 tests/qemu-iotests/262|  4 +-
 tests/qemu-iotests/264|  4 +-
 tests/qemu-iotests/277|  2 +
 tests/qemu-iotests/280|  8 ++--
 tests/qemu-iotests/283|  4 +-
 tests/qemu-iotests/iotests.py | 73 +++
 37 files changed, 128 insertions(+), 80 deletions(-)

diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index b4a21bf7b7..852768f80a 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -382,8 +382,7 @@ def test_once(config, qemu_img=False):
 
 
 # Obviously we only work with the luks image format
-iotests.verify_image_format(supported_fmts=['luks'])
-iotests.verify_platform()
+iotests.script_initialize(supported_fmts=['luks'])
 
 # We need sudo in order to run cryptsetup to create
 # dm-crypt devices. This is safe to use on any
diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index 9dc1bd3510..8b1f720af4 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -21,8 +21,8 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['qcow2', 'qed', 'raw'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2', 'qed', 'raw'],
+  supported_platforms=['linux'])
 
 with iotests.FilePath('source.img') as source_img_path, \
  iotests.FilePath('dest.img') as dest_img_path, \
diff --git a/tests/qemu-iotests/202 b/tests/qemu-iotests/202
index 920a8683ef..e3900a44d1 100755
--- a/tests/qemu-iotests/202
+++ b/tests/qemu-iotests/202
@@ -24,8 +24,8 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2'],
+  supported_platforms=['linux'])
 
 with iotests.FilePath('disk0.img') as disk0_img_path, \
  iotests.FilePath('disk1.img') as disk1_img_path, \
diff --git a/tests/qemu-iotests/203 b/tests/qemu-iotests/203
index 49eff5d405..4b4bd3307d 100755
--- a/tests/qemu-iotests/203
+++ b/tests/qemu-iotests/203
@@ -24,8 +24,8 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2'],
+  supported_platforms=['linux'])
 
 with iotests.FilePath('disk0.img') as disk0_img_path, \
  iotests.FilePath('disk1.img') as disk1_img_path, \
diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
index e2b50ae24d..f42432a838 100755
--- a/tests/qemu-iotests/206
+++ b/tests/qemu-iotests/206
@@ -23,7 +23,7 @@
 import iotests
 from iotests import imgfmt
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
 
 with iotests.FilePath('t.qcow2') as disk_path, \
  iotests.FilePath('t.qcow2.base') as backing_path, \
diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
index 3d9c1208ca..a6621410da 100755
--- a/tests/qemu-iotests/207
+++ b/tests/qemu-iotests/207
@@ -24,8 +24,10 @@ import iotests
 import subprocess
 import re
 
-iotests.verify_image_format(supported_fmts=['raw'])
-iotests.verify_protocol(supported=['ssh'])
+iotests.script_initialize(
+supported_fmts=['raw'],
+supported_protocols=['ssh'],
+)
 
 def filter_hash(qmsg):
 def _filter(key, value):
diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208
index 1c3fc8c7fd..6cb642f821 100755
--- a/tests/qemu-iotests/208
+++ b/tests/qemu-iotests/208
@@ -22,7 +22,7 @@
 
 import iotests
 

[PATCH v8 07/11] iotests: limit line length to 79 chars

[John Snow]

79 is the PEP8 recommendation. This recommendation works well for
reading patch diffs in TUI email clients.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 64 +++
 tests/qemu-iotests/pylintrc   |  6 +++-
 2 files changed, 47 insertions(+), 23 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 3d90fb157d..75fd697d77 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -77,9 +77,11 @@
 def qemu_img(*args):
 '''Run qemu-img and return the exit code'''
 devnull = open('/dev/null', 'r+')
-exitcode = subprocess.call(qemu_img_args + list(args), stdin=devnull, 
stdout=devnull)
+exitcode = subprocess.call(qemu_img_args + list(args),
+   stdin=devnull, stdout=devnull)
 if exitcode < 0:
-sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
'.join(qemu_img_args + list(args
+sys.stderr.write('qemu-img received signal %i: %s\n'
+ % (-exitcode, ' '.join(qemu_img_args + list(args
 return exitcode
 
 def ordered_qmp(qmsg, conv_keys=True):
@@ -118,7 +120,8 @@ def qemu_img_verbose(*args):
 '''Run qemu-img without suppressing its output and return the exit code'''
 exitcode = subprocess.call(qemu_img_args + list(args))
 if exitcode < 0:
-sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
'.join(qemu_img_args + list(args
+sys.stderr.write('qemu-img received signal %i: %s\n'
+ % (-exitcode, ' '.join(qemu_img_args + list(args
 return exitcode
 
 def qemu_img_pipe(*args):
@@ -129,7 +132,8 @@ def qemu_img_pipe(*args):
 universal_newlines=True)
 exitcode = subp.wait()
 if exitcode < 0:
-sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
'.join(qemu_img_args + list(args
+sys.stderr.write('qemu-img received signal %i: %s\n'
+ % (-exitcode, ' '.join(qemu_img_args + list(args
 return subp.communicate()[0]
 
 def qemu_img_log(*args):
@@ -159,7 +163,8 @@ def qemu_io(*args):
 universal_newlines=True)
 exitcode = subp.wait()
 if exitcode < 0:
-sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' 
'.join(args)))
+sys.stderr.write('qemu-io received signal %i: %s\n'
+ % (-exitcode, ' '.join(args)))
 return subp.communicate()[0]
 
 def qemu_io_log(*args):
@@ -281,10 +286,13 @@ def filter_test_dir(msg):
 def filter_win32(msg):
 return win32_re.sub("", msg)
 
-qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* \([0-9\/.inf]* 
[EPTGMKiBbytes]*\/sec and [0-9\/.inf]* ops\/sec\)")
+qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* "
+r"\([0-9\/.inf]* [EPTGMKiBbytes]*\/sec "
+r"and [0-9\/.inf]* ops\/sec\)")
 def filter_qemu_io(msg):
 msg = filter_win32(msg)
-return qemu_io_re.sub("X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)", 
msg)
+return qemu_io_re.sub("X ops; XX:XX:XX.X "
+  "(XXX YYY/sec and XXX ops/sec)", msg)
 
 chown_re = re.compile(r"chown [0-9]+:[0-9]+")
 def filter_chown(msg):
@@ -336,7 +344,9 @@ def filter_img_info(output, filename):
 line = line.replace(filename, 'TEST_IMG') \
.replace(imgfmt, 'IMGFMT')
 line = re.sub('iters: [0-9]+', 'iters: XXX', line)
-line = re.sub('uuid: [-a-f0-9]+', 'uuid: 
----', line)
+line = re.sub('uuid: [-a-f0-9]+',
+  'uuid: ----',
+  line)
 line = re.sub('cid: [0-9]+', 'cid: XX', line)
 lines.append(line)
 return '\n'.join(lines)
@@ -529,11 +539,13 @@ def pause_drive(self, drive, event=None):
 self.pause_drive(drive, "write_aio")
 return
 self.qmp('human-monitor-command',
- command_line='qemu-io %s "break %s bp_%s"' % (drive, event, 
drive))
+ command_line='qemu-io %s "break %s bp_%s"'
+ % (drive, event, drive))
 
 def resume_drive(self, drive):
 self.qmp('human-monitor-command',
- command_line='qemu-io %s "remove_break bp_%s"' % (drive, 
drive))
+ command_line='qemu-io %s "remove_break bp_%s"'
+ % (drive, drive))
 
 def hmp_qemu_io(self, drive, cmd):
 '''Write to a given drive using an HMP command'''
@@ -793,16 +805,18 @@ def dictpath(self, d, path):
 idx = int(idx)
 
 if not isinstance(d, dict) or component not in d:
-self.fail('failed path traversal for "%s" in "%s"' % (path, 
str(d)))
+self.fail(f'failed path traversal for "{path}" in "{d}"')
 d = d[component]
 
 if m:

Re: [PATCH v2 6/8] target/ppc: allow ppc_cpu_do_system_reset to take an alternate vector

[David Gibson]

On Tue, Mar 17, 2020 at 09:28:24AM +1000, Nicholas Piggin wrote:
> Cédric Le Goater's on March 17, 2020 4:15 am:
> > On 3/16/20 3:26 PM, Nicholas Piggin wrote:
> >> Provide for an alternate delivery location, -1 defaults to the
> >> architected address.
> > 
> > I don't know what is the best approach, to override the vector addr
> > computed by powerpc_excp() or use a machine class handler with 
> > cpu->vhyp.
> 
> Yeah it's getting a bit ad hoc and inconsistent with machine check
> etc, I just figured get something minimal in there now. The whole
> exception delivery needs a spring clean though.

Yeah, there's a huge amount of cruft in nearly all the softmmu code.
It's such a big task that I don't really have any plans to tackle it
specifically.  Instead I've been cleaning up little pieces as they
impinge on things I actually care about.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PULL 00/38] Linux user for 5.0 patches

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200316161550.336150-1-laur...@vivier.eu/



Hi,

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

Subject: [PULL 00/38] Linux user for 5.0 patches
Message-id: 20200316161550.336150-1-laur...@vivier.eu
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'
10fafb1 linux-user, openrisc: sync syscall numbers with kernel v5.5
6782f6a linux-user, nios2: sync syscall numbers with kernel v5.5
9f6200d linux-user, aarch64: sync syscall numbers with kernel v5.5
45ee667 scripts: add a script to generate syscall_nr.h
1f2ea4d linux-user,mips: update syscall-args-o32.c.inc
d23d2a1 linux-user,mips: move content of mips_syscall_args
f41ff3c linux-user: update syscall.tbl from linux 0bf999f9c5e7
19a0e30 linux-user, scripts: add a script to update syscall.tbl
4eec500 linux-user, mips64: add syscall table generation support
aeec810 linux-user, mips: add syscall table generation support
9815b8d linux-user, x86_64: add syscall table generation support
2695951 linux-user, i386: add syscall table generation support
1459c30 linux-user, x86_64, i386: cleanup TARGET_NR_arch_prctl
4158bb5 linux-user, sparc, sparc64: add syscall table generation support
91ebd50 linux-user, s390x: add syscall table generation support
b456bac linux-user, s390x: remove syscall definitions for !TARGET_S390X
9a58453 linux-user, ppc: add syscall table generation support
01a0d3e linux-user, arm: add syscall table generation support
5c48386 linux-user, microblaze: add syscall table generation support
c908a6a linux-user, sh4: add syscall table generation support
4a37ce6 linux-user, xtensa: add syscall table generation support
e6f088c linux-user, m68k: add syscall table generation support
b3099a5 linux-user, hppa: add syscall table generation support
550b771 linux-user, alpha: add syscall table generation support
af30485 linux-user: introduce parameters to generate syscall_nr.h
136c2bf linux-user/riscv: Update the syscall_nr's to the 5.5 kernel
b2dec49 linux-user: Support futex_time64
44d988e linux-user/syscall: Add support for clock_gettime64/clock_settime64
96402ae linux-user: Protect more syscalls
8b3e85d linux-user: Update TASK_UNMAPPED_BASE for aarch64
0cbf17e linux-user: fix socket() strace
91bd410 linux-user: do prlimit selectively
403d5bc linux-user: Add AT_EXECFN auxval
06b7595 linux-user: Flush out implementation of gettimeofday
4050585 linux-user: Add x86_64 vsyscall page to /proc/self/maps
1db8bf7 linux-user/i386: Emulate x86_64 vsyscalls
f47d45b linux-user/i386: Split out gen_signal
34bb353 target/i386: Renumber EXCP_SYSCALL

=== OUTPUT BEGIN ===
1/38 Checking commit 34bb353a89ec (target/i386: Renumber EXCP_SYSCALL)
2/38 Checking commit f47d45b3bace (linux-user/i386: Split out gen_signal)
3/38 Checking commit 1db8bf765b39 (linux-user/i386: Emulate x86_64 vsyscalls)
4/38 Checking commit 405058564933 (linux-user: Add x86_64 vsyscall page to 
/proc/self/maps)
5/38 Checking commit 06b75956b14e (linux-user: Flush out implementation of 
gettimeofday)
6/38 Checking commit 403d5bca8b6d (linux-user: Add AT_EXECFN auxval)
7/38 Checking commit 91bd41044ad3 (linux-user: do prlimit selectively)
8/38 Checking commit 0cbf17e17283 (linux-user: fix socket() strace)
9/38 Checking commit 8b3e85d562b1 (linux-user: Update TASK_UNMAPPED_BASE for 
aarch64)
10/38 Checking commit 96402ae791e6 (linux-user: Protect more syscalls)
11/38 Checking commit 44d988e75f99 (linux-user/syscall: Add support for 
clock_gettime64/clock_settime64)
12/38 Checking commit b2dec497276d (linux-user: Support futex_time64)
WARNING: architecture specific defines should be avoided
#26: FILE: linux-user/syscall.c:248:
+#if defined(__NR_futex)

WARNING: architecture specific defines should be avoided
#29: FILE: linux-user/syscall.c:251:
+#if defined(__NR_futex_time64)

WARNING: architecture specific defines should be avoided
#40: FILE: linux-user/syscall.c:303:
+#if (defined(TARGET_NR_futex) && defined(__NR_futex)) || \

WARNING: architecture specific defines should be avoided
#46: FILE: linux-user/syscall.c:309:
+#if (defined(TARGET_NR_futex_time64) && defined(__NR_futex_teim64))

ERROR: space required after that ',' (ctx:VxV)
#47: FILE: linux-user/syscall.c:310:
+_syscall6(int,sys_futex_time64,int *,uaddr,int,op,int,val,
  ^

ERROR: space required after that ',' (ctx:VxV)
#47: FILE: linux-user/syscall.c:310:
+_syscall6(int,sys_futex_time64,int *,uaddr,int,op,int,val,
   ^

ERROR: space required after that ',' (ctx:OxV)
#47: FILE: linux-user/syscall.c:310:
+_syscall6(int,sys_futex_time64,int *,uaddr,int,op,int,val,
 ^

ERROR: space required after that ',' (ctx:VxV)
#47: FILE: 

Re: [PULL SUBSYSTEM qemu-pseries] pseries: Update SLOF firmware image

[David Gibson]

On Tue, Mar 17, 2020 at 10:33:06AM +1100, Alexey Kardashevskiy wrote:
> The following changes since commit 33dead675695e596b7f32c72e6f6a20390e86d8a:
> 
>   pseries: Update SLOF firmware image (2020-03-13 17:50:44 +1100)
> 
> are available in the Git repository at:
> 
>   g...@github.com:aik/qemu.git tags/qemu-slof-20200317
> 
> for you to fetch changes up to b7cf539920376542f03df8337602c3b8974bd1a1:
> 
>   pseries: Update SLOF firmware image (2020-03-17 10:27:34 +1100)
> 
> 
> Alexey Kardashevskiy (1):
>   pseries: Update SLOF firmware image
> 
>  pc-bios/README   |   2 +-
>  pc-bios/slof.bin | Bin 968848 -> 965008 bytes
>  roms/SLOF|   2 +-
>  3 files changed, 2 insertions(+), 2 deletions(-)
> 
> 
> *** Note: this is not for master, this is for pseries
> 
> The only change here is moving the decision about rtas-size
> to QEMU.

Merged to ppc-for-5.0, thanks.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

[PULL 18/61] ich9: fix getter type for sci_int property

[Paolo Bonzini]

From: Felipe Franciosi 

When QOM APIs were added to ich9 in 6f1426ab, the getter for sci_int was
written using uint32_t. However, the object property is uint8_t. This
fixes the getter for correctness.

Signed-off-by: Felipe Franciosi 
Reviewed-by: Marc-André Lureau 
Signed-off-by: Paolo Bonzini 
---
 hw/isa/lpc_ich9.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index d8186f5..2471463 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -629,9 +629,7 @@ static void ich9_lpc_get_sci_int(Object *obj, Visitor *v, 
const char *name,
  void *opaque, Error **errp)
 {
 ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
-uint32_t value = lpc->sci_gsi;
-
-visit_type_uint32(v, name, , errp);
+visit_type_uint8(v, name, >sci_gsi, errp);
 }
 
 static void ich9_lpc_add_properties(ICH9LPCState *lpc)
@@ -639,7 +637,7 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
 static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
 static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
 
-object_property_add(OBJECT(lpc), ACPI_PM_PROP_SCI_INT, "uint32",
+object_property_add(OBJECT(lpc), ACPI_PM_PROP_SCI_INT, "uint8",
 ich9_lpc_get_sci_int,
 NULL, NULL, NULL, NULL);
 object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
-- 
1.8.3.1

[PULL 25/61] hw/audio/fmopl: Move ENV_CURVE to .heap to save 32KiB of .bss

[Paolo Bonzini]

From: Philippe Mathieu-Daudé 

This buffer is only used by the adlib audio device. Move it to
the .heap to release 32KiB of .bss (size reported on x86_64 host).

Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Stefano Garzarella 
Signed-off-by: Paolo Bonzini 
---
 hw/audio/fmopl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
index 173a752..356d4df 100644
--- a/hw/audio/fmopl.c
+++ b/hw/audio/fmopl.c
@@ -186,7 +186,7 @@ static int32_t *VIB_TABLE;
 
 /* envelope output curve table */
 /* attack + decay + OFF */
-static int32_t ENV_CURVE[2*EG_ENT+1];
+static int32_t *ENV_CURVE;
 
 /* multiple table */
 #define ML 2
@@ -1090,6 +1090,7 @@ FM_OPL *OPLCreate(int clock, int rate)
OPL->clock = clock;
OPL->rate  = rate;
OPL->max_ch = max_ch;
+ENV_CURVE = g_new(int32_t, 2 * EG_ENT + 1);
/* init grobal tables */
OPL_initialize(OPL);
/* reset chip */
@@ -1127,6 +1128,7 @@ void OPLDestroy(FM_OPL *OPL)
 #endif
OPL_UnLockTable();
free(OPL);
+g_free(ENV_CURVE);
 }
 
 /* --  Option handlers --   */
-- 
1.8.3.1

[PULL 23/61] Makefile: Align 'help' target output

[Paolo Bonzini]

From: Philippe Mathieu-Daudé 

The 'help' target is displayed unaligned. Add a print-help
function and use it. Now if someone want to change the
indentation, there is a single place to modify.

Signed-off-by: Philippe Mathieu-Daudé 
Signed-off-by: Paolo Bonzini 
---
 Makefile | 44 +++-
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/Makefile b/Makefile
index 7df22fc..6ec19f8 100644
--- a/Makefile
+++ b/Makefile
@@ -1235,50 +1235,52 @@ endif
 include $(SRC_PATH)/tests/docker/Makefile.include
 include $(SRC_PATH)/tests/vm/Makefile.include
 
+print-help-run = printf "  %-30s - %s\\n" "$1" "$2"
+print-help = $(quiet-@)$(call print-help-run,$1,$2)
+
 .PHONY: help
 help:
@echo  'Generic targets:'
-   @echo  '  all - Build all'
+   $(call print-help,all,Build all)
 ifdef CONFIG_MODULES
-   @echo  '  modules - Build all modules'
+   $(call print-help,modules,Build all modules)
 endif
-   @echo  '  dir/file.o  - Build specified target only'
-   @echo  '  install - Install QEMU, documentation and tools'
-   @echo  '  ctags/TAGS  - Generate tags file for editors'
-   @echo  '  cscope  - Generate cscope index'
+   $(call print-help,dir/file.o,Build specified target only)
+   $(call print-help,install,Install QEMU, documentation and tools)
+   $(call print-help,ctags/TAGS,Generate tags file for editors)
+   $(call print-help,cscope,Generate cscope index)
@echo  ''
@$(if $(TARGET_DIRS), \
echo 'Architecture specific targets:'; \
$(foreach t, $(TARGET_DIRS), \
-   printf "  %-30s - Build for %s\\n" $(t)/all $(t);) \
+   $(call print-help-run,$(t)/all,Build for $(t));) \
echo '')
@echo  'Cleaning targets:'
-   @echo  '  clean   - Remove most generated files but keep the 
config'
+   $(call print-help,clean,Remove most generated files but keep the config)
 ifdef CONFIG_GCOV
-   @echo  '  clean-coverage  - Remove coverage files'
+   $(call print-help,clean-coverage,Remove coverage files)
 endif
-   @echo  '  distclean   - Remove all generated files'
-   @echo  '  dist- Build a distributable tarball'
+   $(call print-help,distclean,Remove all generated files)
+   $(call print-help,dist,Build a distributable tarball)
@echo  ''
@echo  'Test targets:'
-   @echo  '  check   - Run all tests (check-help for details)'
-   @echo  '  docker  - Help about targets running tests inside 
containers'
-   @echo  '  vm-help - Help about targets running tests inside VM'
+   $(call print-help,check,Run all tests (check-help for details))
+   $(call print-help,docker,Help about targets running tests inside 
containers)
+   $(call print-help,vm-help,Help about targets running tests inside VM)
@echo  ''
@echo  'Documentation targets:'
-   @echo  '  html info pdf txt'
-   @echo  '  - Build documentation in specified format'
+   $(call print-help,html info pdf txt,Build documentation in specified 
format)
 ifdef CONFIG_GCOV
-   @echo  '  coverage-report - Create code coverage report'
+   $(call print-help,coverage-report,Create code coverage report)
 endif
@echo  ''
 ifdef CONFIG_WIN32
@echo  'Windows targets:'
-   @echo  '  installer   - Build NSIS-based installer for QEMU'
+   $(call print-help,installer,Build NSIS-based installer for QEMU)
 ifdef QEMU_GA_MSI_ENABLED
-   @echo  '  msi - Build MSI-based installer for qemu-ga'
+   $(call print-help,msi,Build MSI-based installer for qemu-ga)
 endif
@echo  ''
 endif
-   @echo  '  $(MAKE) [targets]  (quiet build, default)'
-   @echo  '  $(MAKE) V=1 [targets]  (verbose build)'
+   $(call print-help,$(MAKE) [targets],(quiet build, default))
+   $(call print-help,$(MAKE) V=1 [targets],(verbose build))
-- 
1.8.3.1

[PULL 34/61] cpus: avoid pause_all_vcpus getting stuck due to race

[Paolo Bonzini]

From: Longpeng 

We found an issue when repeat reboot in guest during migration, it cause the
migration thread never be waken up again.

|
   |
LOCK BQL   |
...|
main_loop_should_exit  |
 pause_all_vcpus   |
  1. set all cpus ->stop=true  |
 and then kick |
  2. return if all cpus is paused  |
 (by '->stopped == true'), else|
  3. qemu_cond_wait [BQL UNLOCK]   |
   |LOCK BQL
   |...
   |do_vm_stop
   | pause_all_vcpus
   |  (A)set all cpus ->stop=true
   | and then kick
   |  (B)return if all cpus is paused
   | (by '->stopped == true'), else
   |  (C)qemu_cond_wait [BQL UNLOCK]
  4. be waken up and LOCK BQL  |  (D)be waken up BUT wait for  BQL
  5. goto 2.   |
 (BQL is still LOCKed) |
 resume_all_vcpus  |
  1. set all cpus ->stop=false |
 and ->stopped=false   |
...|
BQL UNLOCK |  (E)LOCK BQL
   |  (F)goto B. [but stopped is false now!]
   |Finally, sleep at step 3 forever.

resume_all_vcpus should notice this race, so we need to move the change
of runstate before pause_all_vcpus in do_vm_stop() and ignore the resume
request if runstate is not running.

Cc: Dr. David Alan Gilbert 
Cc: Richard Henderson 
Signed-off-by: Longpeng 
Suggested-by: Paolo Bonzini 
Message-Id: <20200316083732.2010-1-longpe...@huawei.com>
Signed-off-by: Paolo Bonzini 
---
 cpus.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/cpus.c b/cpus.c
index b4f8b84..ef441bd 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1026,9 +1026,9 @@ static int do_vm_stop(RunState state, bool send_stop)
 int ret = 0;
 
 if (runstate_is_running()) {
+runstate_set(state);
 cpu_disable_ticks();
 pause_all_vcpus();
-runstate_set(state);
 vm_state_notify(0, state);
 if (send_stop) {
 qapi_event_send_stop();
@@ -1899,6 +1899,10 @@ void resume_all_vcpus(void)
 {
 CPUState *cpu;
 
+if (!runstate_is_running()) {
+return;
+}
+
 qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
 CPU_FOREACH(cpu) {
 cpu_resume(cpu);
-- 
1.8.3.1

[PULL 36/61] lockable: add QemuRecMutex support

[Paolo Bonzini]

From: Stefan Hajnoczi 

The polymorphic locking macros don't support QemuRecMutex yet.  Add it
so that lock guards can be used with QemuRecMutex.

Convert TCG plugins functions that benefit from these macros.  Manual
qemu_rec_mutex_lock/unlock() callers are left unmodified in cases where
clarity would not improve by switching to the macros.

Signed-off-by: Stefan Hajnoczi 
Signed-off-by: Paolo Bonzini 
---
 include/qemu/lockable.h |  2 ++
 plugins/core.c  |  7 +++
 plugins/loader.c| 16 
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/include/qemu/lockable.h b/include/qemu/lockable.h
index 2b52c7c..44b3f4b 100644
--- a/include/qemu/lockable.h
+++ b/include/qemu/lockable.h
@@ -50,6 +50,7 @@ qemu_make_lockable(void *x, QemuLockable *lockable)
 #define QEMU_LOCK_FUNC(x) ((QemuLockUnlockFunc *)\
 QEMU_GENERIC(x,  \
  (QemuMutex *, qemu_mutex_lock), \
+ (QemuRecMutex *, qemu_rec_mutex_lock), \
  (CoMutex *, qemu_co_mutex_lock),\
  (QemuSpin *, qemu_spin_lock),   \
  unknown_lock_type))
@@ -57,6 +58,7 @@ qemu_make_lockable(void *x, QemuLockable *lockable)
 #define QEMU_UNLOCK_FUNC(x) ((QemuLockUnlockFunc *)  \
 QEMU_GENERIC(x,  \
  (QemuMutex *, qemu_mutex_unlock),   \
+ (QemuRecMutex *, qemu_rec_mutex_unlock), \
  (CoMutex *, qemu_co_mutex_unlock),  \
  (QemuSpin *, qemu_spin_unlock), \
  unknown_lock_type))
diff --git a/plugins/core.c b/plugins/core.c
index ed86301..51bfc94 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -15,6 +15,7 @@
 #include "qemu/error-report.h"
 #include "qemu/config-file.h"
 #include "qapi/error.h"
+#include "qemu/lockable.h"
 #include "qemu/option.h"
 #include "qemu/rcu_queue.h"
 #include "qemu/xxhash.h"
@@ -150,11 +151,11 @@ do_plugin_register_cb(qemu_plugin_id_t id, enum 
qemu_plugin_event ev,
 {
 struct qemu_plugin_ctx *ctx;
 
-qemu_rec_mutex_lock();
+QEMU_LOCK_GUARD();
 ctx = plugin_id_to_ctx_locked(id);
 /* if the plugin is on its way out, ignore this request */
 if (unlikely(ctx->uninstalling)) {
-goto out_unlock;
+return;
 }
 if (func) {
 struct qemu_plugin_cb *cb = ctx->callbacks[ev];
@@ -178,8 +179,6 @@ do_plugin_register_cb(qemu_plugin_id_t id, enum 
qemu_plugin_event ev,
 } else {
 plugin_unregister_cb__locked(ctx, ev);
 }
- out_unlock:
-qemu_rec_mutex_unlock();
 }
 
 void plugin_register_cb(qemu_plugin_id_t id, enum qemu_plugin_event ev,
diff --git a/plugins/loader.c b/plugins/loader.c
index 15fc7e5..685d334 100644
--- a/plugins/loader.c
+++ b/plugins/loader.c
@@ -19,6 +19,7 @@
 #include "qemu/error-report.h"
 #include "qemu/config-file.h"
 #include "qapi/error.h"
+#include "qemu/lockable.h"
 #include "qemu/option.h"
 #include "qemu/rcu_queue.h"
 #include "qemu/qht.h"
@@ -367,15 +368,14 @@ void plugin_reset_uninstall(qemu_plugin_id_t id,
 struct qemu_plugin_reset_data *data;
 struct qemu_plugin_ctx *ctx;
 
-qemu_rec_mutex_lock();
-ctx = plugin_id_to_ctx_locked(id);
-if (ctx->uninstalling || (reset && ctx->resetting)) {
-qemu_rec_mutex_unlock();
-return;
+WITH_QEMU_LOCK_GUARD() {
+ctx = plugin_id_to_ctx_locked(id);
+if (ctx->uninstalling || (reset && ctx->resetting)) {
+return;
+}
+ctx->resetting = reset;
+ctx->uninstalling = !reset;
 }
-ctx->resetting = reset;
-ctx->uninstalling = !reset;
-qemu_rec_mutex_unlock();
 
 data = g_new(struct qemu_plugin_reset_data, 1);
 data->ctx = ctx;
-- 
1.8.3.1

Re: [PULL 00/38] Linux user for 5.0 patches

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200316161550.336150-1-laur...@vivier.eu/



Hi,

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

Subject: [PULL 00/38] Linux user for 5.0 patches
Message-id: 20200316161550.336150-1-laur...@vivier.eu
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 ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
a5c430a linux-user, openrisc: sync syscall numbers with kernel v5.5
eb272b1 linux-user, nios2: sync syscall numbers with kernel v5.5
22c8c60 linux-user, aarch64: sync syscall numbers with kernel v5.5
ff2d7fb scripts: add a script to generate syscall_nr.h
e61c688 linux-user,mips: update syscall-args-o32.c.inc
2d3af3b linux-user,mips: move content of mips_syscall_args
b059ae6 linux-user: update syscall.tbl from linux 0bf999f9c5e7
e1673a3 linux-user, scripts: add a script to update syscall.tbl
66cb246 linux-user, mips64: add syscall table generation support
1a19648 linux-user, mips: add syscall table generation support
0735d9e linux-user, x86_64: add syscall table generation support
082a58f linux-user, i386: add syscall table generation support
3725dc8 linux-user, x86_64, i386: cleanup TARGET_NR_arch_prctl
c4e0a3a linux-user, sparc, sparc64: add syscall table generation support
b587fb3 linux-user, s390x: add syscall table generation support
b5216b5 linux-user, s390x: remove syscall definitions for !TARGET_S390X
8386d4c linux-user, ppc: add syscall table generation support
787b89e linux-user, arm: add syscall table generation support
f623fac linux-user, microblaze: add syscall table generation support
0257a88 linux-user, sh4: add syscall table generation support
172cbeb linux-user, xtensa: add syscall table generation support
5ec8555 linux-user, m68k: add syscall table generation support
0519335 linux-user, hppa: add syscall table generation support
586725d linux-user, alpha: add syscall table generation support
a3ebf49 linux-user: introduce parameters to generate syscall_nr.h
a129601 linux-user/riscv: Update the syscall_nr's to the 5.5 kernel
29571d4 linux-user: Support futex_time64
78be30d linux-user/syscall: Add support for clock_gettime64/clock_settime64
3781280 linux-user: Protect more syscalls
1e4fd4c linux-user: Update TASK_UNMAPPED_BASE for aarch64
ffe5816 linux-user: fix socket() strace
af7e366 linux-user: do prlimit selectively
de1e86d linux-user: Add AT_EXECFN auxval
0045e32 linux-user: Flush out implementation of gettimeofday
dcc5ae2 linux-user: Add x86_64 vsyscall page to /proc/self/maps
aca467b linux-user/i386: Emulate x86_64 vsyscalls
6828cf1 linux-user/i386: Split out gen_signal
e14979b target/i386: Renumber EXCP_SYSCALL

=== OUTPUT BEGIN ===
1/38 Checking commit e14979b2eea7 (target/i386: Renumber EXCP_SYSCALL)
2/38 Checking commit 6828cf104a6a (linux-user/i386: Split out gen_signal)
3/38 Checking commit aca467bf22c7 (linux-user/i386: Emulate x86_64 vsyscalls)
4/38 Checking commit dcc5ae296a75 (linux-user: Add x86_64 vsyscall page to 
/proc/self/maps)
5/38 Checking commit 0045e32d7be7 (linux-user: Flush out implementation of 
gettimeofday)
6/38 Checking commit de1e86dceed4 (linux-user: Add AT_EXECFN auxval)
7/38 Checking commit af7e3667c6cf (linux-user: do prlimit selectively)
8/38 Checking commit ffe5816ec43b (linux-user: fix socket() strace)
9/38 Checking commit 1e4fd4cc3de4 (linux-user: Update TASK_UNMAPPED_BASE for 
aarch64)
10/38 Checking commit 37812805e354 (linux-user: Protect more syscalls)
11/38 Checking commit 78be30d9711a (linux-user/syscall: Add support for 
clock_gettime64/clock_settime64)
12/38 Checking commit 29571d4f51fe (linux-user: Support futex_time64)
WARNING: architecture specific defines should be avoided
#26: FILE: linux-user/syscall.c:248:
+#if defined(__NR_futex)

WARNING: architecture specific defines should be avoided
#29: FILE: linux-user/syscall.c:251:
+#if defined(__NR_futex_time64)

WARNING: architecture specific defines should be avoided
#40: FILE: linux-user/syscall.c:303:
+#if (defined(TARGET_NR_futex) && defined(__NR_futex)) || \

WARNING: architecture specific defines should be avoided
#46: FILE: linux-user/syscall.c:309:
+#if (defined(TARGET_NR_futex_time64) && defined(__NR_futex_teim64))

ERROR: space required after that ',' (ctx:VxV)
#47: FILE: linux-user/syscall.c:310:
+_syscall6(int,sys_futex_time64,int *,uaddr,int,op,int,val,
  ^

ERROR: space required after that ',' (ctx:VxV)
#47: FILE: linux-user/syscall.c:310:
+_syscall6(int,sys_futex_time64,int *,uaddr,int,op,int,val,
   ^

ERROR: space required after that ',' (ctx:OxV)
#47: FILE: linux-user/syscall.c:310:
+_syscall6(int,sys_futex_time64,int *,uaddr,int,op,int,val,
 ^

ERROR: space 

[PULL 46/61] hw/ppc: Use memory_region_init_rom() with read-only regions

[Paolo Bonzini]

From: Philippe Mathieu-Daudé 

This commit was produced with the Coccinelle script
scripts/coccinelle/memory-region-housekeeping.cocci.

Acked-by: David Gibson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/ppc/mac_newworld.c | 3 +--
 hw/ppc/mac_oldworld.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index b8189bf..b2ec372 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -155,13 +155,12 @@ static void ppc_core99_init(MachineState *machine)
 memory_region_add_subregion(get_system_memory(), 0, machine->ram);
 
 /* allocate and load BIOS */
-memory_region_init_ram(bios, NULL, "ppc_core99.bios", BIOS_SIZE,
+memory_region_init_rom(bios, NULL, "ppc_core99.bios", BIOS_SIZE,
_fatal);
 
 if (bios_name == NULL)
 bios_name = PROM_FILENAME;
 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
-memory_region_set_readonly(bios, true);
 memory_region_add_subregion(get_system_memory(), PROM_ADDR, bios);
 
 /* Load OpenBIOS (ELF) */
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 440c406..faaa165 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -129,13 +129,12 @@ static void ppc_heathrow_init(MachineState *machine)
 memory_region_add_subregion(sysmem, 0, machine->ram);
 
 /* allocate and load BIOS */
-memory_region_init_ram(bios, NULL, "ppc_heathrow.bios", BIOS_SIZE,
+memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", BIOS_SIZE,
_fatal);
 
 if (bios_name == NULL)
 bios_name = PROM_FILENAME;
 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
-memory_region_set_readonly(bios, true);
 memory_region_add_subregion(sysmem, PROM_ADDR, bios);
 
 /* Load OpenBIOS (ELF) */
-- 
1.8.3.1

Re: [PATCH v2 5/8] ppc/spapr: Allow FWNMI on TCG

[Nicholas Piggin]

Greg Kurz's on March 17, 2020 4:01 am:
> On Tue, 17 Mar 2020 00:26:10 +1000
> Nicholas Piggin  wrote:
> 
>> There should no longer be a reason to prevent TCG providing FWNMI.
>> System Reset interrupts are generated to the guest with nmi monitor
>> command and H_SIGNAL_SYS_RESET. Machine Checks can not be injected
>> currently, but this could be implemented with the mce monitor cmd
>> similarly to i386.
>> 
>> Signed-off-by: Nicholas Piggin 
>> ---
>>  hw/ppc/spapr_caps.c | 5 +
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>> index f626d769a0..679ae7959f 100644
>> --- a/hw/ppc/spapr_caps.c
>> +++ b/hw/ppc/spapr_caps.c
>> @@ -516,10 +516,7 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, 
>> uint8_t val,
>>  return; /* Disabled by default */
>>  }
>>  
>> -if (tcg_enabled()) {
>> -warn_report("Firmware Assisted Non-Maskable Interrupts(FWNMI) not "
>> -"supported in TCG");
> 
> With this warning removed, we can now drop the "cap-fwnmi=off" setting
> in qtest, but this can be done as a followup.

Ah right, thanks. Would you send the patch later or should I?

Thanks,
Nick

Re: [PULL 00/38] Linux user for 5.0 patches

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200316161550.336150-1-laur...@vivier.eu/



Hi,

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

Subject: [PULL 00/38] Linux user for 5.0 patches
Message-id: 20200316161550.336150-1-laur...@vivier.eu
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'
fee33bd linux-user, openrisc: sync syscall numbers with kernel v5.5
fd2f311 linux-user, nios2: sync syscall numbers with kernel v5.5
6a5d29c linux-user, aarch64: sync syscall numbers with kernel v5.5
0f61c49 scripts: add a script to generate syscall_nr.h
ba74d13 linux-user,mips: update syscall-args-o32.c.inc
6093ee8 linux-user,mips: move content of mips_syscall_args
329d3cf linux-user: update syscall.tbl from linux 0bf999f9c5e7
fdd68d1 linux-user, scripts: add a script to update syscall.tbl
39929e8 linux-user, mips64: add syscall table generation support
cd6dd35 linux-user, mips: add syscall table generation support
890ff9d linux-user, x86_64: add syscall table generation support
353fb76 linux-user, i386: add syscall table generation support
9db49be linux-user, x86_64, i386: cleanup TARGET_NR_arch_prctl
9da00a8 linux-user, sparc, sparc64: add syscall table generation support
e7dfa49 linux-user, s390x: add syscall table generation support
07108d6 linux-user, s390x: remove syscall definitions for !TARGET_S390X
0a1acf7 linux-user, ppc: add syscall table generation support
48126a9 linux-user, arm: add syscall table generation support
13a5cdd linux-user, microblaze: add syscall table generation support
ea958ad linux-user, sh4: add syscall table generation support
2599c73 linux-user, xtensa: add syscall table generation support
9764b87 linux-user, m68k: add syscall table generation support
52d7b45 linux-user, hppa: add syscall table generation support
baeb698 linux-user, alpha: add syscall table generation support
b3d6a3d linux-user: introduce parameters to generate syscall_nr.h
c4be834 linux-user/riscv: Update the syscall_nr's to the 5.5 kernel
e9992a7 linux-user: Support futex_time64
022b88e linux-user/syscall: Add support for clock_gettime64/clock_settime64
0bf2c2d linux-user: Protect more syscalls
fc4fcbe linux-user: Update TASK_UNMAPPED_BASE for aarch64
0e97ba7 linux-user: fix socket() strace
99fda14 linux-user: do prlimit selectively
8eb735a linux-user: Add AT_EXECFN auxval
571a225 linux-user: Flush out implementation of gettimeofday
092c4cc linux-user: Add x86_64 vsyscall page to /proc/self/maps
d1a6687 linux-user/i386: Emulate x86_64 vsyscalls
169e4fb linux-user/i386: Split out gen_signal
dfc6a82 target/i386: Renumber EXCP_SYSCALL

=== OUTPUT BEGIN ===
1/38 Checking commit dfc6a82f8f0a (target/i386: Renumber EXCP_SYSCALL)
2/38 Checking commit 169e4fb16716 (linux-user/i386: Split out gen_signal)
3/38 Checking commit d1a668759f5b (linux-user/i386: Emulate x86_64 vsyscalls)
4/38 Checking commit 092c4ccea82d (linux-user: Add x86_64 vsyscall page to 
/proc/self/maps)
5/38 Checking commit 571a225e5502 (linux-user: Flush out implementation of 
gettimeofday)
6/38 Checking commit 8eb735a75f6a (linux-user: Add AT_EXECFN auxval)
7/38 Checking commit 99fda143c672 (linux-user: do prlimit selectively)
8/38 Checking commit 0e97ba7c8e8e (linux-user: fix socket() strace)
9/38 Checking commit fc4fcbe8aae4 (linux-user: Update TASK_UNMAPPED_BASE for 
aarch64)
10/38 Checking commit 0bf2c2d4158a (linux-user: Protect more syscalls)
11/38 Checking commit 022b88e1c6bd (linux-user/syscall: Add support for 
clock_gettime64/clock_settime64)
12/38 Checking commit e9992a7e4a00 (linux-user: Support futex_time64)
WARNING: architecture specific defines should be avoided
#26: FILE: linux-user/syscall.c:248:
+#if defined(__NR_futex)

WARNING: architecture specific defines should be avoided
#29: FILE: linux-user/syscall.c:251:
+#if defined(__NR_futex_time64)

WARNING: architecture specific defines should be avoided
#40: FILE: linux-user/syscall.c:303:
+#if (defined(TARGET_NR_futex) && defined(__NR_futex)) || \

WARNING: architecture specific defines should be avoided
#46: FILE: linux-user/syscall.c:309:
+#if (defined(TARGET_NR_futex_time64) && defined(__NR_futex_teim64))

ERROR: space required after that ',' (ctx:VxV)
#47: FILE: linux-user/syscall.c:310:
+_syscall6(int,sys_futex_time64,int *,uaddr,int,op,int,val,
  ^

ERROR: space required after that ',' (ctx:VxV)
#47: FILE: linux-user/syscall.c:310:
+_syscall6(int,sys_futex_time64,int *,uaddr,int,op,int,val,
   ^

ERROR: space required after that ',' (ctx:OxV)
#47: FILE: linux-user/syscall.c:310:
+_syscall6(int,sys_futex_time64,int *,uaddr,int,op,int,val,
 ^

ERROR: space required after that ',' (ctx:VxV)
#47: FILE: 

[Bug 1866892] Re: guest OS catches a page fault bug when running dotnet

[Robert Henry]

A simpler case seems to produce the same error.  See
https://bugs.launchpad.net/qemu/+bug/1824344

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

Title:
  guest OS catches a page  fault bug when running dotnet

Status in QEMU:
  New

Bug description:
  The linux guest OS catches a page fault bug when running the dotnet
  application.

  host = metal = x86_64
  host OS = ubuntu 19.10
  qemu emulation, without KVM, with "tiny code generator" tcg; no plugins; 
built from head/master
  guest emulation = x86_64
  guest OS = ubuntu 19.10
  guest app = dotnet, running any program

  qemu sha=7bc4d1980f95387c4cc921d7a066217ff4e42b70 (head/master Mar 10,
  2020)

  qemu invocation is:

  qemu/build/x86_64-softmmu/qemu-system-x86_64 \
-m size=4096 \
-smp cpus=1 \
-machine type=pc-i440fx-5.0,accel=tcg \
-cpu Skylake-Server-v1 \
-nographic \
-bios OVMF-pure-efi.fd \
-drive if=none,id=hd0,file=ubuntu-19.10-server-cloudimg-amd64.img \
-device virtio-blk,drive=hd0 \
-drive if=none,id=cloud,file=linux_cloud_config.img \
-device virtio-blk,drive=cloud \
-netdev user,id=user0,hostfwd=tcp::2223-:22 \
-device virtio-net,netdev=user0

  
  Here's the guest kernel console output:

  
  [ 2834.005449] BUG: unable to handle page fault for address: 7fffc2c0
  [ 2834.009895] #PF: supervisor read access in user mode
  [ 2834.013872] #PF: error_code(0x0001) - permissions violation
  [ 2834.018025] IDT: 0xfe00 (limit=0xfff) GDT: 0xfe001000 
(limit=0x7f)
  [ 2834.022242] LDTR: NULL
  [ 2834.026306] TR: 0x40 -- base=0xfe003000 limit=0x206f
  [ 2834.030395] PGD 8000360d0067 P4D 8000360d0067 PUD 36105067 PMD 
36193067 PTE 800076d8e867
  [ 2834.038672] Oops: 0001 [#4] SMP PTI
  [ 2834.042707] CPU: 0 PID: 13537 Comm: dotnet Tainted: G  D   
5.3.0-29-generic #31-Ubuntu
  [ 2834.050591] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
0.0.0 02/06/2015
  [ 2834.054785] RIP: 0033:0x147eaeda
  [ 2834.059017] Code: d0 00 00 00 4c 8b a7 d8 00 00 00 4c 8b af e0 00 00 00 4c 
8b b7 e8 00 00 00 4c 8b bf f0 00 00 00 48 8b bf b0 00 00 00 9d 74 02 <48> cf 48 
8d 64 24 30 5d c3 90 cc c3 66 90 55 4c 8b a7 d8 00 00 00
  [ 2834.072103] RSP: 002b:7fffc2c0 EFLAGS: 0202
  [ 2834.076507] RAX:  RBX: 1554b401af38 RCX: 
0001
  [ 2834.080832] RDX:  RSI:  RDI: 
7fffcfb0
  [ 2834.085010] RBP: 7fffd730 R08:  R09: 
7fffd1b0
  [ 2834.089184] R10: 15331dd5 R11: 153ad8d0 R12: 
0002
  [ 2834.093350] R13: 0001 R14: 0001 R15: 
1554b401d388
  [ 2834.097309] FS:  14fa5740 GS:  
  [ 2834.101131] Modules linked in: isofs nls_iso8859_1 dm_multipath 
scsi_dh_rdac scsi_dh_emc scsi_dh_alua ppdev input_leds serio_raw parport_pc 
parport sch_fq_codel ip_tables x_tables autofs4 btrfs zstd_compress raid10 
raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq 
libcrc32c raid1 raid0 multipath linear crct10dif_pclmul crc32_pclmul 
ghash_clmulni_intel aesni_intel aes_x86_64 crypto_simd cryptd glue_helper 
virtio_net psmouse net_failover failover virtio_blk floppy
  [ 2834.122539] CR2: 7fffc2c0
  [ 2834.126867] ---[ end trace dfae51f1d9432708 ]---
  [ 2834.131239] RIP: 0033:0x14d793262eda
  [ 2834.135715] Code: Bad RIP value.
  [ 2834.140243] RSP: 002b:7ffddb4e2980 EFLAGS: 0202
  [ 2834.144615] RAX:  RBX: 14d6f402acb8 RCX: 
0002
  [ 2834.148943] RDX: 01cd6950 RSI:  RDI: 
7ffddb4e3670
  [ 2834.153335] RBP: 7ffddb4e3df0 R08: 0001 R09: 
7ffddb4e3870
  [ 2834.157774] R10: 14d793da9dd5 R11: 14d793e258d0 R12: 
0002
  [ 2834.162132] R13: 0001 R14: 0001 R15: 
14d6f402d040
  [ 2834.166239] FS:  14fa5740() GS:97213ba0() 
knlGS:
  [ 2834.170529] CS:  0033 DS:  ES:  CR0: 80050033
  [ 2834.174751] CR2: 14d793262eb0 CR3: 3613 CR4: 
007406f0
  [ 2834.178892] PKRU: 5554

  I run the application from a shell with `ulimit -s unlimited`
  (unlimited stack to size).

  The application creates a number of threads, and those threads make a
  lot of calls to sigaltstack() and mprotect(); see the relevant source
  for dotnet here
  
https://github.com/dotnet/runtime/blob/15ec69e47b4dc56098e6058a11ccb6ae4d5d4fa1/src/coreclr/src/pal/src/thread/thread.cpp#L2467

  using strace -f on the app shows that no alt stacks come anywhere near
  the failing address; all alt stacks are in the heap, as expected.
  None of the mmap/mprotect/munmap syscalls were given arguments in the
  high memory 0x7fff and up.

  gdb (with default signal stop/print/pass 

[Bug 1824344] Re: x86: retf or iret pagefault sets wrong error code

[Robert Henry]

This appears to be similar to
https://bugs.launchpad.net/qemu/+bug/1866892 (and much simpler)

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

Title:
  x86: retf or iret pagefault sets wrong error code

Status in QEMU:
  New

Bug description:
  With a x86_64 or i386 guest, non-KVM, when trying to execute a
  "iret/iretq/retf" instruction in userspace with invalid stack pointer
  (under a protected mode OS, like Linux), wrong bits are set in the
  pushed error code; bit 2 is not set, indicating the error comes from
  kernel space.

  If the guest OS is using this flag to decide whether this was a kernel
  or user page fault, it will mistakenly decide a kernel has irrecoverably
  faulted, possibly causing guest OS panic.

  
  How to reproduce the problem a guest (non-KVM) Linux:
  Note, on recent Linux kernel version, this needs a CPU with SMAP support
  (eg. -cpu max)

  $ cat tst.c
  int main()
  {
  __asm__ volatile (
  "mov $0,%esp\n"
  "retf"
  );
  return 0;
  }

  $ gcc tst.c
  $ ./a.out
  Killed

  
  "dmesg" shows the kernel has in fact triggered a "BUG: unable to handle
  kernel NULL pointer dereference...", but it has "recovered" by killing
  the faulting process (see attached screenshot).

  
  Using self-compiled qemu from git:
  commit 532cc6da74ec25b5ba6893b5757c977d54582949 (HEAD -> master, tag: 
v4.0.0-rc3, origin/master, origin/HEAD)
  Author: Peter Maydell 
  Date:   Wed Apr 10 15:38:59 2019 +0100

  Update version for v4.0.0-rc3 release
  
  Signed-off-by: Peter Maydell 

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

[PULL 02/10] hbitmap: move hbitmap_iter_next_word to hbitmap.c

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

The function is definitely internal (it's not used by third party and
it has complicated interface). Move it to .c file.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
Message-id: 20200205112041.6003-3-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 include/qemu/hbitmap.h | 30 --
 util/hbitmap.c | 29 +
 2 files changed, 29 insertions(+), 30 deletions(-)

diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 1bf944ca3d..ab227b117f 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -362,34 +362,4 @@ void hbitmap_free_meta(HBitmap *hb);
  */
 int64_t hbitmap_iter_next(HBitmapIter *hbi);
 
-/**
- * hbitmap_iter_next_word:
- * @hbi: HBitmapIter to operate on.
- * @p_cur: Location where to store the next non-zero word.
- *
- * Return the index of the next nonzero word that is set in @hbi's
- * associated HBitmap, and set *p_cur to the content of that word
- * (bits before the index that was passed to hbitmap_iter_init are
- * trimmed on the first call).  Return -1, and set *p_cur to zero,
- * if all remaining words are zero.
- */
-static inline size_t hbitmap_iter_next_word(HBitmapIter *hbi, unsigned long 
*p_cur)
-{
-unsigned long cur = hbi->cur[HBITMAP_LEVELS - 1];
-
-if (cur == 0) {
-cur = hbitmap_iter_skip_words(hbi);
-if (cur == 0) {
-*p_cur = 0;
-return -1;
-}
-}
-
-/* The next call will resume work from the next word.  */
-hbi->cur[HBITMAP_LEVELS - 1] = 0;
-*p_cur = cur;
-return hbi->pos;
-}
-
-
 #endif
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 7f9b3e0cd7..a368dc5ef7 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -298,6 +298,35 @@ uint64_t hbitmap_count(const HBitmap *hb)
 return hb->count << hb->granularity;
 }
 
+/**
+ * hbitmap_iter_next_word:
+ * @hbi: HBitmapIter to operate on.
+ * @p_cur: Location where to store the next non-zero word.
+ *
+ * Return the index of the next nonzero word that is set in @hbi's
+ * associated HBitmap, and set *p_cur to the content of that word
+ * (bits before the index that was passed to hbitmap_iter_init are
+ * trimmed on the first call).  Return -1, and set *p_cur to zero,
+ * if all remaining words are zero.
+ */
+static size_t hbitmap_iter_next_word(HBitmapIter *hbi, unsigned long *p_cur)
+{
+unsigned long cur = hbi->cur[HBITMAP_LEVELS - 1];
+
+if (cur == 0) {
+cur = hbitmap_iter_skip_words(hbi);
+if (cur == 0) {
+*p_cur = 0;
+return -1;
+}
+}
+
+/* The next call will resume work from the next word.  */
+hbi->cur[HBITMAP_LEVELS - 1] = 0;
+*p_cur = cur;
+return hbi->pos;
+}
+
 /* Count the number of set bits between start and end, not accounting for
  * the granularity.  Also an example of how to use hbitmap_iter_next_word.
  */
-- 
2.21.1

[PULL 00/10] Bitmaps patches

[John Snow]

The following changes since commit 6e8a73e911f066527e775e04b98f31ebd19db600:

  Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into 
staging (2020-03-11 14:41:27 +)

are available in the Git repository at:

  https://github.com/jnsnow/qemu.git tags/bitmaps-pull-request

for you to fetch changes up to 34b456d485a4df3a88116fb5ef0c418f2f12990d:

  block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty (2020-03-12 16:36:46 
-0400)


Pull request



Vladimir Sementsov-Ogievskiy (10):
  hbitmap: assert that we don't create bitmap larger than INT64_MAX
  hbitmap: move hbitmap_iter_next_word to hbitmap.c
  hbitmap: unpublish hbitmap_iter_skip_words
  hbitmap: drop meta bitmaps as they are unused
  block/dirty-bitmap: switch _next_dirty_area and _next_zero to int64_t
  block/dirty-bitmap: add _next_dirty API
  block/dirty-bitmap: improve _next_dirty_area API
  nbd/server: introduce NBDExtentArray
  nbd/server: use bdrv_dirty_bitmap_next_dirty_area
  block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty

 include/block/dirty-bitmap.h |   9 +-
 include/qemu/hbitmap.h   |  95 +++
 block/dirty-bitmap.c |  16 +-
 block/qcow2-bitmap.c |  15 +-
 nbd/server.c | 251 ++--
 tests/test-hbitmap.c | 316 +--
 util/hbitmap.c   | 134 +--
 7 files changed, 375 insertions(+), 461 deletions(-)

-- 
2.21.1

[PULL 08/10] nbd/server: introduce NBDExtentArray

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

Introduce NBDExtentArray class, to handle extents list creation in more
controlled way and with fewer OUT parameters in functions.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Message-id: 20200205112041.6003-9-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 nbd/server.c | 210 +--
 1 file changed, 118 insertions(+), 92 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 3106aaf3b4..f90bb33a75 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1909,27 +1909,98 @@ static int coroutine_fn 
nbd_co_send_sparse_read(NBDClient *client,
 return ret;
 }
 
+typedef struct NBDExtentArray {
+NBDExtent *extents;
+unsigned int nb_alloc;
+unsigned int count;
+uint64_t total_length;
+bool can_add;
+bool converted_to_be;
+} NBDExtentArray;
+
+static NBDExtentArray *nbd_extent_array_new(unsigned int nb_alloc)
+{
+NBDExtentArray *ea = g_new0(NBDExtentArray, 1);
+
+ea->nb_alloc = nb_alloc;
+ea->extents = g_new(NBDExtent, nb_alloc);
+ea->can_add = true;
+
+return ea;
+}
+
+static void nbd_extent_array_free(NBDExtentArray *ea)
+{
+g_free(ea->extents);
+g_free(ea);
+}
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free);
+
+/* Further modifications of the array after conversion are abandoned */
+static void nbd_extent_array_convert_to_be(NBDExtentArray *ea)
+{
+int i;
+
+assert(!ea->converted_to_be);
+ea->can_add = false;
+ea->converted_to_be = true;
+
+for (i = 0; i < ea->count; i++) {
+ea->extents[i].flags = cpu_to_be32(ea->extents[i].flags);
+ea->extents[i].length = cpu_to_be32(ea->extents[i].length);
+}
+}
+
 /*
- * Populate @extents from block status. Update @bytes to be the actual
- * length encoded (which may be smaller than the original), and update
- * @nb_extents to the number of extents used.
- *
- * Returns zero on success and -errno on bdrv_block_status_above failure.
+ * Add extent to NBDExtentArray. If extent can't be added (no available space),
+ * return -1.
+ * For safety, when returning -1 for the first time, .can_add is set to false,
+ * further call to nbd_extent_array_add() will crash.
+ * (to avoid the situation, when after failing to add an extent (returned -1),
+ * user miss this failure and add another extent, which is successfully added
+ * (array is full, but new extent may be squashed into the last one), then we
+ * have invalid array with skipped extent)
  */
+static int nbd_extent_array_add(NBDExtentArray *ea,
+uint32_t length, uint32_t flags)
+{
+assert(ea->can_add);
+
+if (!length) {
+return 0;
+}
+
+/* Extend previous extent if flags are the same */
+if (ea->count > 0 && flags == ea->extents[ea->count - 1].flags) {
+uint64_t sum = (uint64_t)length + ea->extents[ea->count - 1].length;
+
+if (sum <= UINT32_MAX) {
+ea->extents[ea->count - 1].length = sum;
+ea->total_length += length;
+return 0;
+}
+}
+
+if (ea->count >= ea->nb_alloc) {
+ea->can_add = false;
+return -1;
+}
+
+ea->total_length += length;
+ea->extents[ea->count] = (NBDExtent) {.length = length, .flags = flags};
+ea->count++;
+
+return 0;
+}
+
 static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
-  uint64_t *bytes, NBDExtent *extents,
-  unsigned int *nb_extents)
+  uint64_t bytes, NBDExtentArray *ea)
 {
-uint64_t remaining_bytes = *bytes;
-NBDExtent *extent = extents, *extents_end = extents + *nb_extents;
-bool first_extent = true;
-
-assert(*nb_extents);
-while (remaining_bytes) {
+while (bytes) {
 uint32_t flags;
 int64_t num;
-int ret = bdrv_block_status_above(bs, NULL, offset, remaining_bytes,
-  , NULL, NULL);
+int ret = bdrv_block_status_above(bs, NULL, offset, bytes, ,
+  NULL, NULL);
 
 if (ret < 0) {
 return ret;
@@ -1938,60 +2009,37 @@ static int blockstatus_to_extents(BlockDriverState *bs, 
uint64_t offset,
 flags = (ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE) |
 (ret & BDRV_BLOCK_ZERO  ? NBD_STATE_ZERO : 0);
 
-if (first_extent) {
-extent->flags = flags;
-extent->length = num;
-first_extent = false;
-} else if (flags == extent->flags) {
-/* extend current extent */
-extent->length += num;
-} else {
-if (extent + 1 == extents_end) {
-break;
-}
-
-/* start new extent */
-extent++;
-extent->flags = flags;
-extent->length = num;
+if 

[PATCH v3 2/3] iotests: add JobRunner class

[John Snow]

The idea is that instead of increasing the arguments to job_run all the
time, create a more general-purpose job runner that can be subclassed to
do interesting things with.

pylint note: the 'callbacks' option guards against unused warning
arguments in functions designated as callbacks. It does not currently
guard against "no-self-use" though; hence a once-off ignore.

mypy note: QapiEvent is only a weak alias; it's fully interchangable
with the type it's declared as. In the future, we may wish to tighten
these types. For now, this communicates the rough shape of the type and
(more importantly) the intent.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/155|  15 ++-
 tests/qemu-iotests/255|   9 +-
 tests/qemu-iotests/257|  54 +
 tests/qemu-iotests/iotests.py | 201 +-
 tests/qemu-iotests/pylintrc   |  11 ++
 5 files changed, 213 insertions(+), 77 deletions(-)

diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
index cb371d4649..e2a013e774 100755
--- a/tests/qemu-iotests/155
+++ b/tests/qemu-iotests/155
@@ -163,6 +163,16 @@ class BaseClass(iotests.QMPTestCase):
 self.assert_qmp_absent(node, 'image/backing-image')
 
 
+class MirrorJob(iotests.JobRunner):
+def __init__(self, *args, test, **kwargs):
+super().__init__(*args, **kwargs)
+self.test = test
+
+def on_pending(self, event):
+self.test.openBacking()
+super().on_pending(event)
+
+
 # Class variables for controlling its behavior:
 #
 # cmd: Mirroring command to execute, either drive-mirror or blockdev-mirror
@@ -188,8 +198,9 @@ class MirrorBaseClass(BaseClass):
 
 self.assert_qmp(result, 'return', {})
 
-self.vm.run_job('mirror-job', auto_finalize=False,
-pre_finalize=self.openBacking, auto_dismiss=True)
+job = MirrorJob(self.vm, 'mirror-job', test=self,
+auto_finalize=False, auto_dismiss=True)
+job.run()
 
 def testFull(self):
 self.runMirror('full')
diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255
index 8f08f741da..e66cdfd672 100755
--- a/tests/qemu-iotests/255
+++ b/tests/qemu-iotests/255
@@ -71,8 +71,13 @@ with iotests.FilePath('t.qcow2') as disk_path, \
 result = vm.qmp_log('block-commit', job_id='job0', auto_finalize=False,
 device='overlay', top_node='mid')
 
-vm.run_job('job0', auto_finalize=False, pre_finalize=start_requests,
-auto_dismiss=True)
+class TestJobRunner(iotests.JobRunner):
+def on_pending(self, event):
+start_requests()
+super().on_pending(event)
+
+runner = TestJobRunner(vm, 'job0', auto_finalize=False, auto_dismiss=True)
+runner.run()
 
 vm.shutdown()
 
diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
index 004a433b8b..95341c330f 100755
--- a/tests/qemu-iotests/257
+++ b/tests/qemu-iotests/257
@@ -352,30 +352,40 @@ def test_bitmap_sync(bsync_mode, msync_mode='bitmap', 
failure=None):
 job = backup(drive0, 1, bsync1, msync_mode,
  bitmap="bitmap0", bitmap_mode=bsync_mode)
 
-def _callback():
-"""Issue writes while the job is open to test bitmap divergence."""
-# Note: when `failure` is 'intermediate', this isn't called.
-log('')
-bitmaps = perform_writes(drive0, 2, filter_node_name='backup-top')
-# Named bitmap (static, should be unchanged)
-ebitmap.compare(vm.get_bitmap(drive0.node, 'bitmap0',
-  bitmaps=bitmaps))
-# Anonymous bitmap (dynamic, shows new writes)
-anonymous = EmulatedBitmap()
-anonymous.dirty_group(2)
-anonymous.compare(vm.get_bitmap(drive0.node, '', recording=True,
-bitmaps=bitmaps))
 
-# Simulate the order in which this will happen:
-# group 1 gets cleared first, then group two gets written.
-if ((bsync_mode == 'on-success' and not failure) or
-(bsync_mode == 'always')):
-ebitmap.clear()
-ebitmap.dirty_group(2)
+class TestJobRunner(iotests.JobRunner):
+def on_pending(self, event):
+"""
+Issue writes while the job is open to test bitmap divergence.
+"""
+
+# Note: when `failure` is 'intermediate', this isn't called.
+log('')
+bitmaps = perform_writes(drive0, 2,
+ filter_node_name='backup-top')
+# Named bitmap (static, should be unchanged)
+ebitmap.compare(vm.get_bitmap(drive0.node, 'bitmap0',
+  bitmaps=bitmaps))
+# Anonymous bitmap (dynamic, shows new writes)
+anonymous = EmulatedBitmap()
+

[PATCH v3 3/3] iotests: modify test 040 to use JobRunner

[John Snow]

Instead of having somewhat reproduced it for itself.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/040 | 51 +-
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 90b59081ff..e2ef3bb812 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -483,34 +483,33 @@ class TestErrorHandling(iotests.QMPTestCase):
   file=('top-dbg' if top_debug else 'top-file'),
   backing='mid-fmt')
 
+
+class TestJobRunner(iotests.JobRunner):
+expected_events = ('BLOCK_JOB_COMPLETED',
+   'BLOCK_JOB_ERROR',
+   'BLOCK_JOB_READY')
+
+def __init__(self, *args, test, **kwargs):
+super().__init__(*args, **kwargs)
+self.log = []
+self.test = test
+
+def on_pause(self, event):
+super().on_pause(event)
+result = self._vm.qmp('block-job-resume', device=self._id)
+self.test.assert_qmp(result, 'return', {})
+
+def on_block_job_event(self, event):
+if event['event'] not in self.expected_events:
+self.test.fail("Unexpected event: %s" % event)
+super().on_block_job_event(event)
+self.log.append(event)
+
 def run_job(self, expected_events, error_pauses_job=False):
-match_device = {'data': {'device': 'job0'}}
-events = {
-'BLOCK_JOB_COMPLETED': match_device,
-'BLOCK_JOB_CANCELLED': match_device,
-'BLOCK_JOB_ERROR': match_device,
-'BLOCK_JOB_READY': match_device,
-}
-
-completed = False
-log = []
-while not completed:
-ev = self.vm.events_wait(events, timeout=5.0)
-if ev['event'] == 'BLOCK_JOB_COMPLETED':
-completed = True
-elif ev['event'] == 'BLOCK_JOB_ERROR':
-if error_pauses_job:
-result = self.vm.qmp('block-job-resume', device='job0')
-self.assert_qmp(result, 'return', {})
-elif ev['event'] == 'BLOCK_JOB_READY':
-result = self.vm.qmp('block-job-complete', device='job0')
-self.assert_qmp(result, 'return', {})
-else:
-self.fail("Unexpected event: %s" % ev)
-log.append(iotests.filter_qmp_event(ev))
-
+job = self.TestJobRunner(self.vm, 'job0', test=self)
+job.run()
 self.maxDiff = None
-self.assertEqual(expected_events, log)
+self.assertEqual(expected_events, job.log)
 
 def event_error(self, op, action):
 return {
-- 
2.21.1

Re: [PATCH 0/5] QEMU Gating CI

[Cleber Rosa]

- Original Message -
> From: "Peter Maydell" 
> To: "Cleber Rosa" 
> Cc: "Fam Zheng" , "Thomas Huth" , "Beraldo 
> Leal" , "Erik
> Skultety" , "Alex Bennée" , 
> "Wainer Moschetta" ,
> "QEMU Developers" , "Wainer dos Santos Moschetta" 
> , "Willian Rampazzo"
> , "Philippe Mathieu-Daudé" , "Eduardo 
> Habkost" 
> Sent: Monday, March 16, 2020 10:57:30 AM
> Subject: Re: [PATCH 0/5] QEMU Gating CI
> 
> On Mon, 16 Mar 2020 at 12:26, Cleber Rosa  wrote:
> > About the runners and the fact that the job is stuck without them,
> > the message seems straightforward enough, but I can't get to the
> > project configuration to look at the registered runners with my
> > current permissions (set as "developer").
> 
> I've moved you up to 'maintainer' status, hopefully that is
> sufficient to look at the relevant config ?
> 
> thanks
> -- PMM
> 
> 

Hi Peter,

Yes, that did the trick and I can now see the configuration.  What I can
*not* see is any "Specific Runner" configured.  So maybe:

1) The documentation I included is not clear enough about the fact that
setup steps need to be done on a machine so that it becomes a "Runner"

2) The (Ansible) playbooks (especially contrib/ci/orgs/qemu/gitlab-runner.yml)
is not working as intended

3) Some expectations misalignment on machines that would be available to run
those jobs

In any case, none of those should be big problems.  Please let me know what
you did/experienced/expected up to this point, and we can continue from there.

Regards,
- Cleber.

Re: [PATCH v3 30/34] qapi: Implement deprecated-output=hide for QMP event data

[Markus Armbruster]

Markus Armbruster  writes:

> This policy suppresses deprecated bits in output, and thus permits
> "testing the future".  Implement it for QMP event data: suppress
> deprecated members.
>
> No QMP event data is deprecated right now.
>
> Signed-off-by: Markus Armbruster 

Needs a fixup:

diff --git a/tests/test-qmp-event.c b/tests/test-qmp-event.c
index be5a2433d0..8f77485454 100644
--- a/tests/test-qmp-event.c
+++ b/tests/test-qmp-event.c
@@ -167,6 +167,8 @@ static void test_event_deprecated_data(TestEventData *data, 
const void *unused)
 qapi_event_send_test_event_features0(42);
 g_assert(data->emitted);
 
+qobject_unref(data->expect);
+
 compat_policy.deprecated_output = COMPAT_POLICY_OUTPUT_HIDE;
 data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST-EVENT-FEATURES0' 
}");
 qapi_event_send_test_event_features0(42);
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 78fa60aa8e..f03c825cc1 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -125,6 +125,8 @@ def gen_event_send(name, arg_type, features, boxed,
 visit_complete(v, );
 if (qdict_size(qobject_to(QDict, obj))) {
 qdict_put_obj(qmp, "data", obj);
+} else {
+qobject_unref(obj);
 }
 ''')
 

Re: [PATCH v3 05/34] tests/test-qmp-cmds: Factor out qmp_dispatch() test helpers

[Markus Armbruster]

Marc-André Lureau  writes:

> Hi
>
> On Sun, Mar 15, 2020 at 3:48 PM Markus Armbruster  wrote:
>>
>> Checking the value of qmp_dispatch() is repetitive.  Factor out
>> helpers do_qmp_dispatch() and do_qmp_dispatch_error().  Without this,
>> the next commit would make things even more repetitive.
>>
>> Signed-off-by: Markus Armbruster 
>> ---
>>  tests/test-qmp-cmds.c | 72 +--
>>  1 file changed, 35 insertions(+), 37 deletions(-)
>
> ASAN is unhappy:
>
> =
> ==1870336==ERROR: LeakSanitizer: detected memory leaks
>
> Indirect leak of 4120 byte(s) in 1 object(s) allocated from:
> #0 0x7fcdc9b8be56 in __interceptor_calloc (/lib64/libasan.so.5+0x10de56)
> #1 0x7fcdc998e3b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x573b0)
> #2 0x560213f56dbb in test_dispatch_cmd_io
> /home/elmarco/src/qemu/tests/test-qmp-cmds.c:238
> #3 0x7fcdc99b0a8d  (/lib64/libglib-2.0.so.0+0x79a8d)
[...]

Thanks!

Fixup to be squashed in here, with revert to be squashed into PATCH 07:

diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index fb18475c7e..b31064b064 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -279,8 +279,6 @@ static void test_dispatch_cmd_io(void)
 g_assert(qnum_get_try_int(ret3, ));
 g_assert_cmpint(val, ==, 66);
 qobject_unref(ret3);
-
-qobject_unref(req);
 }
 
 /* test generated dealloc functions for generated types */

Re: [PATCH v4 6/6] virtio-net: add migration support for RSS and hash report

[Yuri Benditovich]

On Tue, Mar 17, 2020 at 1:05 AM Michael S. Tsirkin  wrote:

> On Mon, Mar 16, 2020 at 12:09:33PM +0200, Yuri Benditovich wrote:
> > Save and restore RSS/hash report configuration.
> >
> > Signed-off-by: Yuri Benditovich 
> > ---
> >  hw/net/virtio-net.c | 26 ++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index a0614ad4e6..f343762a0f 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -2842,6 +2842,13 @@ static int virtio_net_post_load_device(void
> *opaque, int version_id)
> >  }
> >  }
> >
> > +if (n->rss_data.enabled) {
> > +trace_virtio_net_rss_enable(n->rss_data.hash_types,
> > +n->rss_data.indirections_len,
> > +sizeof(n->rss_data.key));
> > +} else {
> > +trace_virtio_net_rss_disable();
> > +}
> >  return 0;
> >  }
> >
> > @@ -3019,6 +3026,24 @@ static const VMStateDescription
> vmstate_virtio_net_has_vnet = {
> >  },
> >  };
> >
> > +static const VMStateDescription vmstate_rss = {
> > +.name  = "vmstate_rss",
> > +.fields = (VMStateField[]) {
> > +VMSTATE_BOOL(enabled, VirtioNetRssData),
> > +VMSTATE_BOOL(redirect, VirtioNetRssData),
> > +VMSTATE_BOOL(populate_hash, VirtioNetRssData),
> > +VMSTATE_UINT32(hash_types, VirtioNetRssData),
> > +VMSTATE_UINT32(indirections_len, VirtioNetRssData),
>
>
> Why is this UINT32? Shouldn't it be UINT16?
>

It is UINT32 in the _internal_ structure to use VMSTATE_VARRAY_UINT32_ALLOC.
Otherwise I need to invent additional macro for the same operation with
UINT16 length.


>
> > +VMSTATE_UINT16(default_queue, VirtioNetRssData),
> > +VMSTATE_UINT8_ARRAY(key, VirtioNetRssData,
> > +VIRTIO_NET_RSS_MAX_KEY_SIZE),
> > +VMSTATE_VARRAY_UINT32_ALLOC(indirections_table,
> VirtioNetRssData,
> > +indirections_len, 0,
> > +vmstate_info_uint16, uint16_t),
> > +VMSTATE_END_OF_LIST()
> > +},
> > +};
> > +
> >  static const VMStateDescription vmstate_virtio_net_device = {
> >  .name = "virtio-net-device",
> >  .version_id = VIRTIO_NET_VM_VERSION,
> > @@ -3067,6 +3092,7 @@ static const VMStateDescription
> vmstate_virtio_net_device = {
> >   vmstate_virtio_net_tx_waiting),
> >  VMSTATE_UINT64_TEST(curr_guest_offloads, VirtIONet,
> >  has_ctrl_guest_offloads),
> > +VMSTATE_STRUCT(rss_data, VirtIONet, 1, vmstate_rss,
> VirtioNetRssData),
> >  VMSTATE_END_OF_LIST()
> > },
> >  };
> > --
> > 2.17.1
>
>

[PATCH v2 4/8] ppc/spapr: Fix FWNMI machine check interrupt delivery

[Nicholas Piggin]

FWNMI machine check delivery misses a few things that will make it fail
with TCG at least (which we would like to allow in future to improve
testing).

It's not nice to scatter interrupt delivery logic around the tree, so
move it to excp_helper.c and share code where possible.

Signed-off-by: Nicholas Piggin 
---
 hw/ppc/spapr_events.c| 24 +++--
 target/ppc/cpu.h |  1 +
 target/ppc/excp_helper.c | 74 
 3 files changed, 57 insertions(+), 42 deletions(-)

diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 27ba8a2c19..323fcef4aa 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -785,28 +785,13 @@ static uint32_t spapr_mce_get_elog_type(PowerPCCPU *cpu, 
bool recovered,
 static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool recovered)
 {
 SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
-uint64_t rtas_addr;
+CPUState *cs = CPU(cpu);
 CPUPPCState *env = >env;
-PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
-target_ulong msr = 0;
+uint64_t rtas_addr;
 struct rtas_error_log log;
 struct mc_extended_log *ext_elog;
 uint32_t summary;
 
-/*
- * Properly set bits in MSR before we invoke the handler.
- * SRR0/1, DAR and DSISR are properly set by KVM
- */
-if (!(*pcc->interrupts_big_endian)(cpu)) {
-msr |= (1ULL << MSR_LE);
-}
-
-if (env->msr & (1ULL << MSR_SF)) {
-msr |= (1ULL << MSR_SF);
-}
-
-msr |= (1ULL << MSR_ME);
-
 ext_elog = g_malloc0(sizeof(*ext_elog));
 summary = spapr_mce_get_elog_type(cpu, recovered, ext_elog);
 
@@ -834,12 +819,11 @@ static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool 
recovered)
 cpu_physical_memory_write(rtas_addr + RTAS_ERROR_LOG_OFFSET +
   sizeof(env->gpr[3]) + sizeof(log), ext_elog,
   sizeof(*ext_elog));
+g_free(ext_elog);
 
 env->gpr[3] = rtas_addr + RTAS_ERROR_LOG_OFFSET;
-env->msr = msr;
-env->nip = spapr->fwnmi_machine_check_addr;
 
-g_free(ext_elog);
+ppc_cpu_do_fwnmi_machine_check(cs, spapr->fwnmi_machine_check_addr);
 }
 
 void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 5a55fb02bd..3953680534 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1221,6 +1221,7 @@ int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, 
CPUState *cs,
int cpuid, void *opaque);
 #ifndef CONFIG_USER_ONLY
 void ppc_cpu_do_system_reset(CPUState *cs);
+void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector);
 extern const VMStateDescription vmstate_ppc_cpu;
 #endif
 
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 027f54c0ed..7f2b5899d3 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -128,6 +128,37 @@ static uint64_t ppc_excp_vector_offset(CPUState *cs, int 
ail)
 return offset;
 }
 
+static inline void powerpc_set_excp_state(PowerPCCPU *cpu,
+  target_ulong vector, target_ulong 
msr)
+{
+CPUState *cs = CPU(cpu);
+CPUPPCState *env = >env;
+
+/*
+ * We don't use hreg_store_msr here as already have treated any
+ * special case that could occur. Just store MSR and update hflags
+ *
+ * Note: We *MUST* not use hreg_store_msr() as-is anyway because it
+ * will prevent setting of the HV bit which some exceptions might need
+ * to do.
+ */
+env->msr = msr & env->msr_mask;
+hreg_compute_hflags(env);
+env->nip = vector;
+/* Reset exception state */
+cs->exception_index = POWERPC_EXCP_NONE;
+env->error_code = 0;
+
+/* Reset the reservation */
+env->reserve_addr = -1;
+
+/*
+ * Any interrupt is context synchronizing, check if TCG TLB needs
+ * a delayed flush on ppc64
+ */
+check_tlb_flush(env, false);
+}
+
 /*
  * Note that this function should be greatly optimized when called
  * with a constant excp, from ppc_hw_interrupt
@@ -768,29 +799,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
excp_model, int excp)
 }
 }
 #endif
-/*
- * We don't use hreg_store_msr here as already have treated any
- * special case that could occur. Just store MSR and update hflags
- *
- * Note: We *MUST* not use hreg_store_msr() as-is anyway because it
- * will prevent setting of the HV bit which some exceptions might need
- * to do.
- */
-env->msr = new_msr & env->msr_mask;
-hreg_compute_hflags(env);
-env->nip = vector;
-/* Reset exception state */
-cs->exception_index = POWERPC_EXCP_NONE;
-env->error_code = 0;
 
-/* Reset the reservation */
-env->reserve_addr = -1;
-
-/*
- * Any interrupt is context synchronizing, check if TCG TLB needs
- * a delayed flush on ppc64
- */
-check_tlb_flush(env, false);
+powerpc_set_excp_state(cpu, 

Re: [PATCH v9] fixup! Fix subcode/pbt

[Cornelia Huck]

On Mon, 16 Mar 2020 15:47:41 +0100
Janosch Frank  wrote:

> On 3/16/20 3:27 PM, Cornelia Huck wrote:
> > On Fri, 13 Mar 2020 05:52:32 -0400
> > Janosch Frank  wrote:
> >   
> >> Signed-off-by: Janosch Frank 
> >> ---
> >>  hw/s390x/ipl.h  | 11 +++
> >>  target/s390x/diag.c |  2 +-
> >>  2 files changed, 8 insertions(+), 5 deletions(-)


> >> @@ -118,7 +118,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, 
> >> uint64_t r3, uintptr_t ra)
> >>  
> >>  cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
> >>  
> >> -if (!iplb_valid(iplb)) {
> >> +if (!iplb_valid(iplb, subcode)) {
> >>  env->regs[r1 + 1] = DIAG_308_RC_INVALID;
> >>  goto out;
> >>  }  
> > 
> > ...because you're basically checking whether you either have a valid
> > normal iplb, or a valid pv iplb, with the two being mutually exclusive,
> > IIUC. So what about introducing iplb_valid_pv and calling that for the
> > pv case? Would be a bit nicer to read, I think, and also matches what
> > you do for the STORE case.
> >   
> 
> The idea was to get rid of all of these ifs and elses and only have one
> iplb_valid function. Your suggestion would defeat hiding that complexity
> behind this function.

I'd argue that this is a complexity we should not hide; for non-pv, we
can have several formats, for pv, only one, and we cannot use a pv iplb
in a non-pv context and vice versa.


pgpo5viM3JqK4.pgp
Description: OpenPGP digital signature

[PATCH v2 5/8] ppc/spapr: Allow FWNMI on TCG

[Nicholas Piggin]

There should no longer be a reason to prevent TCG providing FWNMI.
System Reset interrupts are generated to the guest with nmi monitor
command and H_SIGNAL_SYS_RESET. Machine Checks can not be injected
currently, but this could be implemented with the mce monitor cmd
similarly to i386.

Signed-off-by: Nicholas Piggin 
---
 hw/ppc/spapr_caps.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index f626d769a0..679ae7959f 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -516,10 +516,7 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, 
uint8_t val,
 return; /* Disabled by default */
 }
 
-if (tcg_enabled()) {
-warn_report("Firmware Assisted Non-Maskable Interrupts(FWNMI) not "
-"supported in TCG");
-} else if (kvm_enabled()) {
+if (kvm_enabled()) {
 if (kvmppc_set_fwnmi() < 0) {
 error_setg(errp, "Firmware Assisted Non-Maskable Interrupts(FWNMI) 
"
  "not supported by KVM");
-- 
2.23.0

Re: [PATCH v9] fixup! Fix subcode/pbt

[Janosch Frank]

On 3/16/20 3:54 PM, Cornelia Huck wrote:
> On Mon, 16 Mar 2020 15:47:41 +0100
> Janosch Frank  wrote:
> 
>> On 3/16/20 3:27 PM, Cornelia Huck wrote:
>>> On Fri, 13 Mar 2020 05:52:32 -0400
>>> Janosch Frank  wrote:
>>>   
 Signed-off-by: Janosch Frank 
 ---
  hw/s390x/ipl.h  | 11 +++
  target/s390x/diag.c |  2 +-
  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> 
 @@ -118,7 +118,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, 
 uint64_t r3, uintptr_t ra)
  
  cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
  
 -if (!iplb_valid(iplb)) {
 +if (!iplb_valid(iplb, subcode)) {
  env->regs[r1 + 1] = DIAG_308_RC_INVALID;
  goto out;
  }  
>>>
>>> ...because you're basically checking whether you either have a valid
>>> normal iplb, or a valid pv iplb, with the two being mutually exclusive,
>>> IIUC. So what about introducing iplb_valid_pv and calling that for the
>>> pv case? Would be a bit nicer to read, I think, and also matches what
>>> you do for the STORE case.
>>>   
>>S390_IPL_TYPE_CCW
>> The idea was to get rid of all of these ifs and elses and only have one
>> iplb_valid function. Your suggestion would defeat hiding that complexity
>> behind this function.
> 
> I'd argue that this is a complexity we should not hide; for non-pv, we
> can have several formats, for pv, only one, and we cannot use a pv iplb
> in a non-pv context and vice versa.
> 

Ok, then please let me split this out into a new function within diag.c.
Something like:

static bool diag308_pbt_subcode_validity(uint8_t pbt, uint64_t subcode)
{
if (subcode == DIAG308_SET) {
return (pbt == S390_IPL_TYPE_FCP || pbt == S390_IPL_TYPE_CCW)
} else if (subcode == DIAG308_PV_SET && pbt == S390_IPL_TYPE_PV) {
return true;
}

return false;
}



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v6 3/4] qcow2: add zstd cluster compression

[Eric Blake]

On 3/12/20 4:22 AM, Denis Plotnikov wrote:

zstd significantly reduces cluster compression time.
It provides better compression performance maintaining
the same level of the compression ratio in comparison with
zlib, which, at the moment, is the only compression
method available.




+++ b/docs/interop/qcow2.txt
@@ -208,6 +208,7 @@ version 2.
  
  Available compression type values:

  0: zlib 
+1: zstd 
  
  
  === Header padding ===

@@ -575,11 +576,30 @@ Compressed Clusters Descriptor (x = 62 - (cluster_bits - 
8)):
  Another compressed cluster may map to the tail of the 
final
  sector used by this compressed cluster.
  
+The layout of the compressed data depends on the compression

+type used for the image (see compressed cluster layout).
+
  If a cluster is unallocated, read requests shall read the data from the 
backing
  file (except if bit 0 in the Standard Cluster Descriptor is set). If there is
  no backing file or the backing file is smaller than the image, they shall read
  zeros for all parts that are not covered by the backing file.
  
+=== Compressed Cluster Layout ===

+
+The compressed cluster data has a layout depending on the compression
+type used for the image, as follows:
+
+Compressed data layout for the available compression types:
+data_space_lenght - data chunk length available to store a compressed cluster.


length


+(for more details see "Compressed Clusters Descriptor")
+x = data_space_length - 1


If I understand correctly, data_space_length is really an upper bounds 
on the length available, because it is computed by rounding UP to the 
next 512-byte boundary (that is, the L2 descriptor lists the number of 
additional sectors used in storing the compressed data).  Which really 
means that we have the following, where + is cluster boundaries, S and E 
are the start and end of the compressed data, and D is the offset 
determined by data_space_length:


+---+---+--+
  SE...D


+
+0:  (default)  zlib :
+Byte  0 -  x: the compressed data content
+  all the space provided used for compressed data


For zlib, we have byte 0-E are compressed data, and bytes (E+1)-D (if 
any) are ignored.  There is no way to tell how many bytes between E and 
D exist, because zlib doesn't care (the compression stream itself 
ensures that decompression stops when input reaches E because the output 
reached a cluster boundary at that point).



+1:  zstd :
+Byte  0 -  3: the length of compressed data in bytes
+  4 -  x: the compressed data content


Whereas for zstd, the decompression MUST know the actual location of E, 
rather than passing in the slop between E and D; bytes 0-3 give us that 
information.


But your description is not very accurate:  if 'x' is point E, then it 
is NOT data_space_length - 1, but rather data_space_length - slop, where 
slop can be up to 511 bytes (the number of bytes from (E+1) to D).  And 
if 'x' is point E, then the real layout for zlib is:


byte 0 - E: the compressed data content
byte E+1 - x: ignored slop (E is implied solely by the compressed data)

and for zstd is:

byte 0 - 3: the length of the compressed data
byte 4 - E: the compressed data (E computed from byte 0-3)
byte E+1 - x: ignored

I'm not sure what the best way is to document this.


+++ b/block/qcow2-threads.c



+static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
+{
+size_t ret;
+
+/*
+ * steal ZSTD_LEN_BUF bytes in the very beginning of the buffer
+ * to store compressed chunk size
+ */
+char *d_buf = ((char *) dest) + ZSTD_LEN_BUF;
+
+/*
+ * sanity check that we can store the compressed data length,
+ * and there is some space left for the compressor buffer
+ */
+if (dest_size <= ZSTD_LEN_BUF) {
+return -ENOMEM;
+}
+
+dest_size -= ZSTD_LEN_BUF;
+
+ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);


Where does the magic number 5 come from?


+
+if (ZSTD_isError(ret)) {
+if (ZSTD_getErrorCode(ret) == ZSTD_error_dstSize_tooSmall) {
+return -ENOMEM;
+} else {
+return -EIO;
+}
+}
+
+/*
+ * paranoid sanity check that we can store
+ * the compressed size in the first 4 bytes
+ */
+if (ret > UINT32_MAX) {
+return -ENOMEM;
+}


The if is awkward.  I'd prefer to change this to:

/*
 * Our largest cluster is 2M, and we insist that compression
 * actually compressed things.
 */
assert(ret < UINT32_MAX);

or even tighten to assert(ret <= dest_size)


+
+/* store the compressed chunk 

[PATCH v6 16/42] nvme: make sure ncqr and nsqr is valid

[Klaus Jensen]

From: Klaus Jensen 

0x is not an allowed value for NCQR and NSQR in Set Features on
Number of Queues.

Signed-off-by: Klaus Jensen 
Acked-by: Keith Busch 
Reviewed-by: Maxim Levitsky 
---
 hw/block/nvme.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 85c7c86b35f0..e56142c4ea99 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1155,6 +1155,14 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd 
*cmd, NvmeRequest *req)
 blk_set_enable_write_cache(n->conf.blk, dw11 & 1);
 break;
 case NVME_NUMBER_OF_QUEUES:
+/*
+ * NVMe v1.3, Section 5.21.1.7: 0x is not an allowed value for NCQR
+ * and NSQR.
+ */
+if ((dw11 & 0x) == 0x || ((dw11 >> 16) & 0x) == 0x) {
+return NVME_INVALID_FIELD | NVME_DNR;
+}
+
 trace_nvme_dev_setfeat_numq((dw11 & 0x) + 1,
 ((dw11 >> 16) & 0x) + 1,
 n->params.max_ioqpairs,
-- 
2.25.1

[PATCH v6 20/42] nvme: provide the mandatory subnqn field

[Klaus Jensen]

From: Klaus Jensen 

Signed-off-by: Klaus Jensen 
---
 hw/block/nvme.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index b40d27cddc46..74061d08fd2e 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1925,6 +1925,9 @@ static void nvme_init_ctrl(NvmeCtrl *n)
 id->nn = cpu_to_le32(n->num_namespaces);
 id->oncs = cpu_to_le16(NVME_ONCS_WRITE_ZEROS | NVME_ONCS_TIMESTAMP);
 
+pstrcpy((char *) id->subnqn, sizeof(id->subnqn), "nqn.2019-08.org.qemu:");
+pstrcat((char *) id->subnqn, sizeof(id->subnqn), n->params.serial);
+
 id->psd[0].mp = cpu_to_le16(0x9c4);
 id->psd[0].enlat = cpu_to_le32(0x10);
 id->psd[0].exlat = cpu_to_le32(0x4);
-- 
2.25.1

[PATCH v6 10/42] nvme: refactor device realization

[Klaus Jensen]

From: Klaus Jensen 

This patch splits up nvme_realize into multiple individual functions,
each initializing a different subset of the device.

Signed-off-by: Klaus Jensen 
Acked-by: Keith Busch 
---
 hw/block/nvme.c | 178 ++--
 hw/block/nvme.h |  23 ++-
 2 files changed, 134 insertions(+), 67 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 7dfd8a1a392d..665485045066 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -44,6 +44,8 @@
 #include "trace.h"
 #include "nvme.h"
 
+#define NVME_CMB_BIR 2
+
 #define NVME_GUEST_ERR(trace, fmt, ...) \
 do { \
 (trace_##trace)(__VA_ARGS__); \
@@ -63,7 +65,7 @@ static inline bool nvme_addr_is_cmb(NvmeCtrl *n, hwaddr addr)
 
 static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
 {
-if (n->cmbsz && nvme_addr_is_cmb(n, addr)) {
+if (n->bar.cmbsz && nvme_addr_is_cmb(n, addr)) {
 memcpy(buf, (void *)>cmbuf[addr - n->ctrl_mem.addr], size);
 return;
 }
@@ -157,7 +159,7 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector 
*iov, uint64_t prp1,
 if (unlikely(!prp1)) {
 trace_nvme_dev_err_invalid_prp();
 return NVME_INVALID_FIELD | NVME_DNR;
-} else if (n->cmbsz && prp1 >= n->ctrl_mem.addr &&
+} else if (n->bar.cmbsz && prp1 >= n->ctrl_mem.addr &&
prp1 < n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size)) {
 qsg->nsg = 0;
 qemu_iovec_init(iov, num_prps);
@@ -1324,14 +1326,9 @@ static const MemoryRegionOps nvme_cmb_ops = {
 },
 };
 
-static void nvme_realize(PCIDevice *pci_dev, Error **errp)
+static int nvme_check_constraints(NvmeCtrl *n, Error **errp)
 {
-NvmeCtrl *n = NVME(pci_dev);
-NvmeIdCtrl *id = >id_ctrl;
-
-int i;
-int64_t bs_size;
-uint8_t *pci_conf;
+NvmeParams *params = >params;
 
 if (n->params.num_queues) {
 warn_report("nvme: num_queues is deprecated; please use max_ioqpairs "
@@ -1340,57 +1337,100 @@ static void nvme_realize(PCIDevice *pci_dev, Error 
**errp)
 n->params.max_ioqpairs = n->params.num_queues - 1;
 }
 
-if (!n->params.max_ioqpairs) {
-error_setg(errp, "max_ioqpairs can't be less than 1");
+if (params->max_ioqpairs < 1 ||
+params->max_ioqpairs > PCI_MSIX_FLAGS_QSIZE) {
+error_setg(errp, "nvme: max_ioqpairs must be ");
+return -1;
 }
 
 if (!n->conf.blk) {
-error_setg(errp, "drive property not set");
-return;
+error_setg(errp, "nvme: block backend not configured");
+return -1;
 }
 
-bs_size = blk_getlength(n->conf.blk);
-if (bs_size < 0) {
-error_setg(errp, "could not get backing file size");
-return;
+if (!params->serial) {
+error_setg(errp, "nvme: serial not configured");
+return -1;
 }
 
-if (!n->params.serial) {
-error_setg(errp, "serial property not set");
-return;
-}
+return 0;
+}
+
+static int nvme_init_blk(NvmeCtrl *n, Error **errp)
+{
 blkconf_blocksizes(>conf);
 if (!blkconf_apply_backend_options(>conf, blk_is_read_only(n->conf.blk),
false, errp)) {
-return;
+return -1;
 }
 
-pci_conf = pci_dev->config;
-pci_conf[PCI_INTERRUPT_PIN] = 1;
-pci_config_set_prog_interface(pci_dev->config, 0x2);
-pci_config_set_class(pci_dev->config, PCI_CLASS_STORAGE_EXPRESS);
-pcie_endpoint_cap_init(pci_dev, 0x80);
+return 0;
+}
 
+static void nvme_init_state(NvmeCtrl *n)
+{
 n->num_namespaces = 1;
 n->reg_size = pow2ceil(0x1008 + 2 * (n->params.max_ioqpairs) * 4);
-n->ns_size = bs_size / (uint64_t)n->num_namespaces;
-
 n->namespaces = g_new0(NvmeNamespace, n->num_namespaces);
 n->sq = g_new0(NvmeSQueue *, n->params.max_ioqpairs + 1);
 n->cq = g_new0(NvmeCQueue *, n->params.max_ioqpairs + 1);
+}
 
-memory_region_init_io(>iomem, OBJECT(n), _mmio_ops, n,
-  "nvme", n->reg_size);
-pci_register_bar(pci_dev, 0,
-PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64,
->iomem);
+static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
+{
+NVME_CMBLOC_SET_BIR(n->bar.cmbloc, NVME_CMB_BIR);
+NVME_CMBLOC_SET_OFST(n->bar.cmbloc, 0);
+
+NVME_CMBSZ_SET_SQS(n->bar.cmbsz, 1);
+NVME_CMBSZ_SET_CQS(n->bar.cmbsz, 0);
+NVME_CMBSZ_SET_LISTS(n->bar.cmbsz, 0);
+NVME_CMBSZ_SET_RDS(n->bar.cmbsz, 1);
+NVME_CMBSZ_SET_WDS(n->bar.cmbsz, 1);
+NVME_CMBSZ_SET_SZU(n->bar.cmbsz, 2);
+NVME_CMBSZ_SET_SZ(n->bar.cmbsz, n->params.cmb_size_mb);
+
+n->cmbuf = g_malloc0(NVME_CMBSZ_GETSIZE(n->bar.cmbsz));
+memory_region_init_io(>ctrl_mem, OBJECT(n), _cmb_ops, n,
+  "nvme-cmb", NVME_CMBSZ_GETSIZE(n->bar.cmbsz));
+pci_register_bar(pci_dev, NVME_CMBLOC_BIR(n->bar.cmbloc),
+ PCI_BASE_ADDRESS_SPACE_MEMORY |
+