[Qemu-devel] [PATCH V3] hw/iommu: Fix problems reported by Coverity scan

[David Kiarie]

Signed-off-by: David Kiarie 
---
 hw/i386/amd_iommu.c | 22 +-
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 023de52..47b79d9 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -143,10 +143,10 @@ static void amdvi_assign_andq(AMDVIState *s, hwaddr addr, 
uint64_t val)
 
 static void amdvi_generate_msi_interrupt(AMDVIState *s)
 {
-MSIMessage msg;
-MemTxAttrs attrs;
-
-attrs.requester_id = pci_requester_id(>pci.dev);
+MSIMessage msg = {};
+MemTxAttrs attrs = {
+.requester_id = pci_requester_id(>pci.dev)
+};
 
 if (msi_enabled(>pci.dev)) {
 msg = msi_get_message(>pci.dev, 0);
@@ -185,7 +185,7 @@ static void amdvi_setevent_bits(uint64_t *buffer, uint64_t 
value, int start,
 int length)
 {
 int index = start / 64, bitpos = start % 64;
-uint64_t mask = ((1 << length) - 1) << bitpos;
+uint64_t mask = MAKE_64BIT_MASK(start, length);
 buffer[index] &= ~mask;
 buffer[index] |= (value << bitpos) & mask;
 }
@@ -333,8 +333,8 @@ static void amdvi_update_iotlb(AMDVIState *s, uint16_t 
devid,
uint64_t gpa, IOMMUTLBEntry to_cache,
uint16_t domid)
 {
-AMDVIIOTLBEntry *entry = g_malloc(sizeof(*entry));
-uint64_t *key = g_malloc(sizeof(key));
+AMDVIIOTLBEntry *entry = g_new(AMDVIIOTLBEntry, 1);
+uint64_t *key = g_new(uint64_t, 1);
 uint64_t gfn = gpa >> AMDVI_PAGE_SHIFT_4K;
 
 /* don't cache erroneous translations */
@@ -1135,6 +1135,7 @@ static void amdvi_reset(DeviceState *dev)
 
 static void amdvi_realize(DeviceState *dev, Error **err)
 {
+int ret = 0;
 AMDVIState *s = AMD_IOMMU_DEVICE(dev);
 X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev);
 PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
@@ -1147,8 +1148,11 @@ static void amdvi_realize(DeviceState *dev, Error **err)
 object_property_set_bool(OBJECT(>pci), true, "realized", err);
 s->capab_offset = pci_add_capability(>pci.dev, AMDVI_CAPAB_ID_SEC, 0,
  AMDVI_CAPAB_SIZE);
-pci_add_capability(>pci.dev, PCI_CAP_ID_MSI, 0, AMDVI_CAPAB_REG_SIZE);
-pci_add_capability(>pci.dev, PCI_CAP_ID_HT, 0, AMDVI_CAPAB_REG_SIZE);
+assert(s->capab_offset > 0);
+ret = pci_add_capability(>pci.dev, PCI_CAP_ID_MSI, 0, 
AMDVI_CAPAB_REG_SIZE);
+assert(ret > 0);
+ret = pci_add_capability(>pci.dev, PCI_CAP_ID_HT, 0, 
AMDVI_CAPAB_REG_SIZE);
+assert(ret > 0);
 
 /* set up MMIO */
 memory_region_init_io(>mmio, OBJECT(s), _mem_ops, s, "amdvi-mmio",
-- 
2.1.4

[Qemu-devel] [PATCH V3] Coverity Fix

[David Kiarie]

The following patch fixes a few issues reported by Coverity in the file 
hw/i386/amd_iommu.c

V3 includes fixes and suggestions from Paolo and Stefan.

David Kiarie (1):
  hw/iommu: Fix problems reported by Coverity scan

 hw/i386/amd_iommu.c | 22 +-
 1 file changed, 13 insertions(+), 9 deletions(-)

-- 
2.1.4

Re: [Qemu-devel] [SeaBIOS] [PATCH 5/5] [wip] sercon: initial split-output implementation

[Kevin O'Connor]

On Tue, Sep 27, 2016 at 02:00:08PM +0200, Gerd Hoffmann wrote:
> On Fr, 2016-07-15 at 10:35 -0400, Kevin O'Connor wrote:
> > On Fri, Jul 15, 2016 at 01:49:49PM +0200, Gerd Hoffmann wrote:
> > > > Finally, one high level observation is that we know there are a number
> > > > of quirks in various vgabios emulators.  For example, we know some
> > > > emulators don't handle certain 32bit instructions when in 16bit mode
> > > > (hence scripts/vgafixup.py), we know some versions of Windows use an
> > > > emulator that doesn't like some stack relative instructions (hence the
> > > > vgabios is compiled without -fomit-frame-pointer), and we know Windows
> > > > Vista doesn't like the extra stack in high ram (the skifree bug).  Any
> > > > thoughts on what happens with these quirks if the main seabios code
> > > > hooks int10?
> > > 
> > > Good question.  Do the emulators (both win, xorg) use the int10 vector
> > > set by seabios in the first place?  Or go they load the vgabios and run
> > > it, including the initialization, and use whatever entry point the init
> > > code sets up?  I suspect it is the latter.  But needs investigation and
> > > testing.
> > 
> > I think they just call the existing int10 handler.  In general, it's
> > not safe to rerun the vga init code.  Also, if they did run the init
> > it would lead to extra copies of the SeaVGABIOS version banners in the
> > debug logs, which I don't recall seeing.
> 
> Finally found the time to continue with this and ran a bunch of tests.
> RHEL-5 (known to be affected by x86emu issue) continues to work fine.
> Xorg server log looks like it goes scan memory for the vgabios instead
> of depending on the int10 vector:

Interesting.  I'm curious how the memory scan works, because I didn't
think there was any way to find the vga entry point except from the
int10 vector.

Were you looking to include this series in SeaBIOS v1.10?  We can
either delay the release or push the changes into the next release.
Also not sure where Igor's patches stand wrt inclusion into QEMU.

-Kevin

Re: [Qemu-devel] [PATCH v2 03/11] Blockjobs: Internalize user_pause logic

[John Snow]

On 10/03/2016 08:57 PM, Jeff Cody wrote:

On Fri, Sep 30, 2016 at 06:00:41PM -0400, John Snow wrote:

BlockJobs will begin hiding their state in preparation for some
refactorings anyway, so let's internalize the user_pause mechanism
instead of leaving it to callers to correctly manage.

Signed-off-by: John Snow 
---
 block/io.c   |  2 +-
 blockdev.c   | 10 --
 blockjob.c   | 16 
 include/block/blockjob.h | 11 ++-
 4 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/block/io.c b/block/io.c
index fdf7080..868b065 100644
--- a/block/io.c
+++ b/block/io.c
@@ -291,7 +291,7 @@ void bdrv_drain_all(void)
 AioContext *aio_context = blk_get_aio_context(job->blk);

 aio_context_acquire(aio_context);
-block_job_pause(job);
+block_job_pause(job, false);


Hmm.  Maybe semantically, an enum might work better here than a bool.  When
I see "block_job_pause(job, false)", it reads like you just un-paused the
job, rather than paused it with some contextual info.  Perhaps
JOB_USER_PAUSE and JOB_SYS_PAUSE (or just JOB_USER and JOB_SYS)?



Thanks for taking a look. Mostly I was trying to squish users of jobs 
internals into API accesses instead, in semi-prep for shifting things 
around a good bit later.


The real inspiration was trying to figure out if adding a new state was 
safe, and figuring out who-touched-what-and-when was easier if I split 
things into "internal" and "external" functions, then I could audit the 
external functions more carefully.


(This is a tip for auditing the safety of what I do later in this 
series...!)


To the point, you're probably right, this was a bit of a quick hack. 
I'll make this nicer tomorrow.



 aio_context_release(aio_context);
 }

diff --git a/blockdev.c b/blockdev.c
index 03200e7..268452f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3629,7 +3629,7 @@ void qmp_block_job_cancel(const char *device,
 force = false;
 }

-if (job->user_paused && !force) {
+if (block_job_paused(job) && !force) {
 error_setg(errp, "The block job for device '%s' is currently paused",
device);
 goto out;
@@ -3646,13 +3646,12 @@ void qmp_block_job_pause(const char *device, Error 
**errp)
 AioContext *aio_context;
 BlockJob *job = find_block_job(device, _context, errp);

-if (!job || job->user_paused) {
+if (!job || block_job_paused(job)) {
 return;
 }

-job->user_paused = true;
 trace_qmp_block_job_pause(job);
-block_job_pause(job);
+block_job_pause(job, true);
 aio_context_release(aio_context);
 }

@@ -3661,11 +3660,10 @@ void qmp_block_job_resume(const char *device, Error 
**errp)
 AioContext *aio_context;
 BlockJob *job = find_block_job(device, _context, errp);

-if (!job || !job->user_paused) {
+if (!job || !block_job_paused(job)) {
 return;
 }

-job->user_paused = false;
 trace_qmp_block_job_resume(job);
 block_job_iostatus_reset(job);
 block_job_resume(job);
diff --git a/blockjob.c b/blockjob.c
index 6a300ba..2a35f50 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -104,7 +104,7 @@ static void block_job_detach_aio_context(void *opaque)
 /* In case the job terminates during aio_poll()... */
 block_job_ref(job);

-block_job_pause(job);
+block_job_pause(job, false);

 if (!job->paused) {
 /* If job is !job->busy this kicks it into the next pause point. */
@@ -343,9 +343,12 @@ void block_job_complete(BlockJob *job, Error **errp)
 job->driver->complete(job, errp);
 }

-void block_job_pause(BlockJob *job)
+void block_job_pause(BlockJob *job, bool user)
 {
 job->pause_count++;
+if (user) {
+job->user_paused = true;
+}
 }

 static bool block_job_should_pause(BlockJob *job)
@@ -353,6 +356,11 @@ static bool block_job_should_pause(BlockJob *job)
 return job->pause_count > 0;
 }

+bool block_job_paused(BlockJob *job)


I think a more apt name for this would be "block_job_user_paused()", because
it doesn't tell if the job is paused per se, but only if it is paused by a
user.



Sure.


+{
+return job ? job->user_paused : 0;
+}
+
 void coroutine_fn block_job_pause_point(BlockJob *job)
 {
 if (!block_job_should_pause(job)) {
@@ -386,6 +394,7 @@ void block_job_resume(BlockJob *job)
 if (job->pause_count) {
 return;
 }
+job->user_paused = false;


At first I was thinking block_job_resume() would need a way of knowing if it
was a user resume, similar to block_job_pause().  But I guess not... if the
user paused it, then job->pause_count should always be > 0, I think.

But wait...could we ever get into a state where something like this happens:

block_job_pause(job, true);  /* user paused */

[...]

block_job_pause(job, false); /* system paused */

[...]

block_job_resume(job);   /* user resumes */

[...]   /* job->user_paused is still true, although it 

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 1/6] libqos: add PPC64 PCI support

[David Gibson]

On Mon, Oct 03, 2016 at 04:03:14PM +0200, Greg Kurz wrote:
> On Mon, 3 Oct 2016 13:23:27 +0200
> Cédric Le Goater  wrote:
> 
> > On 09/29/2016 07:27 AM, David Gibson wrote:
> > > On Wed, Sep 28, 2016 at 08:51:28PM +0200, Laurent Vivier wrote:  
> > >> Signed-off-by: Laurent Vivier 
> > >> ---
> > >>  tests/Makefile.include   |   1 +
> > >>  tests/libqos/pci-pc.c|  22 
> > >>  tests/libqos/pci-spapr.c | 280 
> > >> +++
> > >>  tests/libqos/pci-spapr.h |  17 +++
> > >>  tests/libqos/pci.c   |  22 +++-
> > >>  tests/libqos/rtas.c  |  45 
> > >>  tests/libqos/rtas.h  |   4 +
> > >>  7 files changed, 368 insertions(+), 23 deletions(-)
> > >>  create mode 100644 tests/libqos/pci-spapr.c
> > >>  create mode 100644 tests/libqos/pci-spapr.h
> > >>
> > >> diff --git a/tests/Makefile.include b/tests/Makefile.include
> > >> index 8162f6f..92c82d8 100644
> > >> --- a/tests/Makefile.include
> > >> +++ b/tests/Makefile.include
> > >> @@ -590,6 +590,7 @@ libqos-obj-y += tests/libqos/i2c.o 
> > >> tests/libqos/libqos.o
> > >>  libqos-spapr-obj-y = $(libqos-obj-y) tests/libqos/malloc-spapr.o
> > >>  libqos-spapr-obj-y += tests/libqos/libqos-spapr.o
> > >>  libqos-spapr-obj-y += tests/libqos/rtas.o
> > >> +libqos-spapr-obj-y += tests/libqos/pci-spapr.o
> > >>  libqos-pc-obj-y = $(libqos-obj-y) tests/libqos/pci-pc.o
> > >>  libqos-pc-obj-y += tests/libqos/malloc-pc.o tests/libqos/libqos-pc.o
> > >>  libqos-pc-obj-y += tests/libqos/ahci.o
> > >> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> > >> index 1ae2d37..82066b8 100644
> > >> --- a/tests/libqos/pci-pc.c
> > >> +++ b/tests/libqos/pci-pc.c
> > >> @@ -255,28 +255,6 @@ void qpci_free_pc(QPCIBus *bus)
> > >>  g_free(s);
> > >>  }
> > >>  
> > >> -void qpci_plug_device_test(const char *driver, const char *id,
> > >> -   uint8_t slot, const char *opts)
> > >> -{
> > >> -QDict *response;
> > >> -char *cmd;
> > >> -
> > >> -cmd = g_strdup_printf("{'execute': 'device_add',"
> > >> -  " 'arguments': {"
> > >> -  "   'driver': '%s',"
> > >> -  "   'addr': '%d',"
> > >> -  "   %s%s"
> > >> -  "   'id': '%s'"
> > >> -  "}}", driver, slot,
> > >> -  opts ? opts : "", opts ? "," : "",
> > >> -  id);
> > >> -response = qmp(cmd);
> > >> -g_free(cmd);
> > >> -g_assert(response);
> > >> -g_assert(!qdict_haskey(response, "error"));
> > >> -QDECREF(response);
> > >> -}
> > >> -
> > >>  void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
> > >>  {
> > >>  QDict *response;
> > >> diff --git a/tests/libqos/pci-spapr.c b/tests/libqos/pci-spapr.c
> > >> new file mode 100644
> > >> index 000..78df823
> > >> --- /dev/null
> > >> +++ b/tests/libqos/pci-spapr.c
> > >> @@ -0,0 +1,280 @@
> > >> +/*
> > >> + * libqos PCI bindings for SPAPR
> > >> + *
> > >> + * 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 "libqtest.h"
> > >> +#include "libqos/pci-spapr.h"
> > >> +#include "libqos/rtas.h"
> > >> +
> > >> +#include "hw/pci/pci_regs.h"
> > >> +
> > >> +#include "qemu-common.h"
> > >> +#include "qemu/host-utils.h"
> > >> +
> > >> +
> > >> +/* From include/hw/pci-host/spapr.h */
> > >> +
> > >> +#define SPAPR_PCI_BASE_BUID  0x8002000ULL
> > >> +
> > >> +#define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x8000ULL
> > >> +
> > >> +#define SPAPR_PCI_WINDOW_BASE0x100ULL
> > >> +#define SPAPR_PCI_WINDOW_SPACING 0x10ULL
> > >> +#define SPAPR_PCI_MMIO_WIN_OFF   0xA000
> > >> +#define SPAPR_PCI_MMIO_WIN_SIZE  (SPAPR_PCI_WINDOW_SPACING - \
> > >> + SPAPR_PCI_MEM_WIN_BUS_OFFSET)
> > >> +#define SPAPR_PCI_IO_WIN_OFF 0x8000
> > >> +#define SPAPR_PCI_IO_WIN_SIZE0x1
> > >> +
> > >> +/* index is the phb index */
> > >> +
> > >> +#define BUIDBASE(index)  (SPAPR_PCI_BASE_BUID + (index))
> > >> +#define PCIBASE(index)   (SPAPR_PCI_WINDOW_BASE + \
> > >> +  (index) * 
> > >> SPAPR_PCI_WINDOW_SPACING)
> > >> +#define IOBASE(index)(PCIBASE(index) + 
> > >> SPAPR_PCI_IO_WIN_OFF)
> > >> +#define MMIOBASE(index)  (PCIBASE(index) + 
> > >> SPAPR_PCI_MMIO_WIN_OFF)
> > >> +
> > >> +typedef struct QPCIBusSPAPR {
> > >> +QPCIBus bus;
> > >> +QGuestAllocator *alloc;
> > >> +
> > >> +uint64_t pci_hole_start;
> > >> +uint64_t pci_hole_size;
> > >> +uint64_t pci_hole_alloc;
> > >> +
> > >> +uint32_t pci_iohole_start;
> > >> +uint32_t pci_iohole_size;
> > >> +uint32_t 

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 1/6] libqos: add PPC64 PCI support

[David Gibson]

On Mon, Oct 03, 2016 at 01:23:27PM +0200, Cédric Le Goater wrote:
> On 09/29/2016 07:27 AM, David Gibson wrote:
> > On Wed, Sep 28, 2016 at 08:51:28PM +0200, Laurent Vivier wrote:
> >> Signed-off-by: Laurent Vivier 
> >> ---
> >>  tests/Makefile.include   |   1 +
> >>  tests/libqos/pci-pc.c|  22 
> >>  tests/libqos/pci-spapr.c | 280 
> >> +++
> >>  tests/libqos/pci-spapr.h |  17 +++
> >>  tests/libqos/pci.c   |  22 +++-
> >>  tests/libqos/rtas.c  |  45 
> >>  tests/libqos/rtas.h  |   4 +
> >>  7 files changed, 368 insertions(+), 23 deletions(-)
> >>  create mode 100644 tests/libqos/pci-spapr.c
> >>  create mode 100644 tests/libqos/pci-spapr.h
> >>
> >> diff --git a/tests/Makefile.include b/tests/Makefile.include
> >> index 8162f6f..92c82d8 100644
> >> --- a/tests/Makefile.include
> >> +++ b/tests/Makefile.include
> >> @@ -590,6 +590,7 @@ libqos-obj-y += tests/libqos/i2c.o 
> >> tests/libqos/libqos.o
> >>  libqos-spapr-obj-y = $(libqos-obj-y) tests/libqos/malloc-spapr.o
> >>  libqos-spapr-obj-y += tests/libqos/libqos-spapr.o
> >>  libqos-spapr-obj-y += tests/libqos/rtas.o
> >> +libqos-spapr-obj-y += tests/libqos/pci-spapr.o
> >>  libqos-pc-obj-y = $(libqos-obj-y) tests/libqos/pci-pc.o
> >>  libqos-pc-obj-y += tests/libqos/malloc-pc.o tests/libqos/libqos-pc.o
> >>  libqos-pc-obj-y += tests/libqos/ahci.o
> >> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> >> index 1ae2d37..82066b8 100644
> >> --- a/tests/libqos/pci-pc.c
> >> +++ b/tests/libqos/pci-pc.c
> >> @@ -255,28 +255,6 @@ void qpci_free_pc(QPCIBus *bus)
> >>  g_free(s);
> >>  }
> >>  
> >> -void qpci_plug_device_test(const char *driver, const char *id,
> >> -   uint8_t slot, const char *opts)
> >> -{
> >> -QDict *response;
> >> -char *cmd;
> >> -
> >> -cmd = g_strdup_printf("{'execute': 'device_add',"
> >> -  " 'arguments': {"
> >> -  "   'driver': '%s',"
> >> -  "   'addr': '%d',"
> >> -  "   %s%s"
> >> -  "   'id': '%s'"
> >> -  "}}", driver, slot,
> >> -  opts ? opts : "", opts ? "," : "",
> >> -  id);
> >> -response = qmp(cmd);
> >> -g_free(cmd);
> >> -g_assert(response);
> >> -g_assert(!qdict_haskey(response, "error"));
> >> -QDECREF(response);
> >> -}
> >> -
> >>  void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
> >>  {
> >>  QDict *response;
> >> diff --git a/tests/libqos/pci-spapr.c b/tests/libqos/pci-spapr.c
> >> new file mode 100644
> >> index 000..78df823
> >> --- /dev/null
> >> +++ b/tests/libqos/pci-spapr.c
> >> @@ -0,0 +1,280 @@
> >> +/*
> >> + * libqos PCI bindings for SPAPR
> >> + *
> >> + * 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 "libqtest.h"
> >> +#include "libqos/pci-spapr.h"
> >> +#include "libqos/rtas.h"
> >> +
> >> +#include "hw/pci/pci_regs.h"
> >> +
> >> +#include "qemu-common.h"
> >> +#include "qemu/host-utils.h"
> >> +
> >> +
> >> +/* From include/hw/pci-host/spapr.h */
> >> +
> >> +#define SPAPR_PCI_BASE_BUID  0x8002000ULL
> >> +
> >> +#define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x8000ULL
> >> +
> >> +#define SPAPR_PCI_WINDOW_BASE0x100ULL
> >> +#define SPAPR_PCI_WINDOW_SPACING 0x10ULL
> >> +#define SPAPR_PCI_MMIO_WIN_OFF   0xA000
> >> +#define SPAPR_PCI_MMIO_WIN_SIZE  (SPAPR_PCI_WINDOW_SPACING - \
> >> + SPAPR_PCI_MEM_WIN_BUS_OFFSET)
> >> +#define SPAPR_PCI_IO_WIN_OFF 0x8000
> >> +#define SPAPR_PCI_IO_WIN_SIZE0x1
> >> +
> >> +/* index is the phb index */
> >> +
> >> +#define BUIDBASE(index)  (SPAPR_PCI_BASE_BUID + (index))
> >> +#define PCIBASE(index)   (SPAPR_PCI_WINDOW_BASE + \
> >> +  (index) * SPAPR_PCI_WINDOW_SPACING)
> >> +#define IOBASE(index)(PCIBASE(index) + 
> >> SPAPR_PCI_IO_WIN_OFF)
> >> +#define MMIOBASE(index)  (PCIBASE(index) + 
> >> SPAPR_PCI_MMIO_WIN_OFF)
> >> +
> >> +typedef struct QPCIBusSPAPR {
> >> +QPCIBus bus;
> >> +QGuestAllocator *alloc;
> >> +
> >> +uint64_t pci_hole_start;
> >> +uint64_t pci_hole_size;
> >> +uint64_t pci_hole_alloc;
> >> +
> >> +uint32_t pci_iohole_start;
> >> +uint32_t pci_iohole_size;
> >> +uint32_t pci_iohole_alloc;
> >> +} QPCIBusSPAPR;
> >> +
> >> +static uint8_t qpci_spapr_io_readb(QPCIBus *bus, void *addr)
> >> +{
> >> +uint64_t port = (uint64_t)addr;
> >> +uint8_t v;
> >> +if (port < SPAPR_PCI_IO_WIN_SIZE) {
> >> +v = readb(IOBASE(0) + port);
> >> +} else {
> >> +v = readb(MMIOBASE(0) 

Re: [Qemu-devel] [PATCH] spapr: fix check of cpu alias name in spapr_get_cpu_core_type()

[David Gibson]

On Mon, Oct 03, 2016 at 08:44:22PM +0530, Bharata B Rao wrote:
> On Mon, Oct 03, 2016 at 02:13:20PM +0200, Greg Kurz wrote:
> > If the user passes an alias name and a property to -cpu, QEMU fails to
> > find the CPU definition and exits.
> > 
> > $ qemu-system-ppc64 -cpu POWER8E,compat=power7
> > qemu-system-ppc64: Unable to find sPAPR CPU Core definition
> > 
> > This happens because spapr_get_cpu_core_type() passes the full string from
> > the command line (i.e. "POWER8E,compat=power7") to ppc_cpu_lookup_alias(),
> > instead of the alias name piece only (i.e. "POWER8E").
> > 
> > The fix is to pass model_pieces[0] to ppc_cpu_lookup_alias().
> > 
> > Signed-off-by: Greg Kurz 
> 
> Reviewed-by: Bharata B Rao 

Applied to ppc-for-2.8, thanks.

> 
> > ---
> >  hw/ppc/spapr_cpu_core.c |8 
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 6f0533c34259..35d1873b9ff3 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -92,20 +92,20 @@ char *spapr_get_cpu_core_type(const char *model)
> >  gchar **model_pieces = g_strsplit(model, ",", 2);
> > 
> >  core_type = g_strdup_printf("%s-%s", model_pieces[0], 
> > TYPE_SPAPR_CPU_CORE);
> > -g_strfreev(model_pieces);
> > 
> >  /* Check whether it exists or whether we have to look up an alias name 
> > */
> >  if (!object_class_by_name(core_type)) {
> >  const char *realmodel;
> > 
> >  g_free(core_type);
> > -realmodel = ppc_cpu_lookup_alias(model);
> > +core_type = NULL;
> > +realmodel = ppc_cpu_lookup_alias(model_pieces[0]);
> >  if (realmodel) {
> > -return spapr_get_cpu_core_type(realmodel);
> > +core_type = spapr_get_cpu_core_type(realmodel);
> >  }
> > -return NULL;
> >  }
> > 
> > +g_strfreev(model_pieces);
> >  return core_type;
> >  }
> > 
> 

-- 
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: [Qemu-devel] [PATCH v2 03/11] Blockjobs: Internalize user_pause logic

[Jeff Cody]

On Fri, Sep 30, 2016 at 06:00:41PM -0400, John Snow wrote:
> BlockJobs will begin hiding their state in preparation for some
> refactorings anyway, so let's internalize the user_pause mechanism
> instead of leaving it to callers to correctly manage.
> 
> Signed-off-by: John Snow 
> ---
>  block/io.c   |  2 +-
>  blockdev.c   | 10 --
>  blockjob.c   | 16 
>  include/block/blockjob.h | 11 ++-
>  4 files changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index fdf7080..868b065 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -291,7 +291,7 @@ void bdrv_drain_all(void)
>  AioContext *aio_context = blk_get_aio_context(job->blk);
>  
>  aio_context_acquire(aio_context);
> -block_job_pause(job);
> +block_job_pause(job, false);

Hmm.  Maybe semantically, an enum might work better here than a bool.  When
I see "block_job_pause(job, false)", it reads like you just un-paused the
job, rather than paused it with some contextual info.  Perhaps
JOB_USER_PAUSE and JOB_SYS_PAUSE (or just JOB_USER and JOB_SYS)?

>  aio_context_release(aio_context);
>  }
>  
> diff --git a/blockdev.c b/blockdev.c
> index 03200e7..268452f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3629,7 +3629,7 @@ void qmp_block_job_cancel(const char *device,
>  force = false;
>  }
>  
> -if (job->user_paused && !force) {
> +if (block_job_paused(job) && !force) {
>  error_setg(errp, "The block job for device '%s' is currently paused",
> device);
>  goto out;
> @@ -3646,13 +3646,12 @@ void qmp_block_job_pause(const char *device, Error 
> **errp)
>  AioContext *aio_context;
>  BlockJob *job = find_block_job(device, _context, errp);
>  
> -if (!job || job->user_paused) {
> +if (!job || block_job_paused(job)) {
>  return;
>  }
>  
> -job->user_paused = true;
>  trace_qmp_block_job_pause(job);
> -block_job_pause(job);
> +block_job_pause(job, true);
>  aio_context_release(aio_context);
>  }
>  
> @@ -3661,11 +3660,10 @@ void qmp_block_job_resume(const char *device, Error 
> **errp)
>  AioContext *aio_context;
>  BlockJob *job = find_block_job(device, _context, errp);
>  
> -if (!job || !job->user_paused) {
> +if (!job || !block_job_paused(job)) {
>  return;
>  }
>  
> -job->user_paused = false;
>  trace_qmp_block_job_resume(job);
>  block_job_iostatus_reset(job);
>  block_job_resume(job);
> diff --git a/blockjob.c b/blockjob.c
> index 6a300ba..2a35f50 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -104,7 +104,7 @@ static void block_job_detach_aio_context(void *opaque)
>  /* In case the job terminates during aio_poll()... */
>  block_job_ref(job);
>  
> -block_job_pause(job);
> +block_job_pause(job, false);
>  
>  if (!job->paused) {
>  /* If job is !job->busy this kicks it into the next pause point. */
> @@ -343,9 +343,12 @@ void block_job_complete(BlockJob *job, Error **errp)
>  job->driver->complete(job, errp);
>  }
>  
> -void block_job_pause(BlockJob *job)
> +void block_job_pause(BlockJob *job, bool user)
>  {
>  job->pause_count++;
> +if (user) {
> +job->user_paused = true;
> +}
>  }
>  
>  static bool block_job_should_pause(BlockJob *job)
> @@ -353,6 +356,11 @@ static bool block_job_should_pause(BlockJob *job)
>  return job->pause_count > 0;
>  }
>  
> +bool block_job_paused(BlockJob *job)

I think a more apt name for this would be "block_job_user_paused()", because
it doesn't tell if the job is paused per se, but only if it is paused by a
user.

> +{
> +return job ? job->user_paused : 0;
> +}
> +
>  void coroutine_fn block_job_pause_point(BlockJob *job)
>  {
>  if (!block_job_should_pause(job)) {
> @@ -386,6 +394,7 @@ void block_job_resume(BlockJob *job)
>  if (job->pause_count) {
>  return;
>  }
> +job->user_paused = false;

At first I was thinking block_job_resume() would need a way of knowing if it
was a user resume, similar to block_job_pause().  But I guess not... if the
user paused it, then job->pause_count should always be > 0, I think.

But wait...could we ever get into a state where something like this happens:

block_job_pause(job, true);  /* user paused */

[...]

block_job_pause(job, false); /* system paused */

[...]

block_job_resume(job);   /* user resumes */

[...]   /* job->user_paused is still true, although it shouldn't be */

block_job_resume(job);   /* system resumes */

 /* now job->user_paused is false */


>  block_job_enter(job);
>  }
>  
> @@ -592,8 +601,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, 
> BlockdevOnError on_err,
>  action, _abort);
>  if (action == BLOCK_ERROR_ACTION_STOP) {
>  /* make the pause user visible, which 

[Qemu-devel] [PATCH qemu] sysemu: support up to 1024 vCPUs

[Alexey Kardashevskiy]

From: Greg Kurz 

Some systems can already provide more than 255 hardware threads.

Bumping the QEMU limit to 1024 seems reasonable:
- it has no visible overhead in top;
- the limit itself has no effect on hot paths.

Signed-off-by: Greg Kurz 
Signed-off-by: Alexey Kardashevskiy 
---
 include/sysemu/sysemu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index ef2c50b..2ec0bd8 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -173,7 +173,7 @@ extern int mem_prealloc;
  *
  * Note that cpu->get_arch_id() may be larger than MAX_CPUMASK_BITS.
  */
-#define MAX_CPUMASK_BITS 255
+#define MAX_CPUMASK_BITS 1024
 
 #define MAX_OPTION_ROMS 16
 typedef struct QEMUOptionRom {
-- 
2.5.0.rc3

Re: [Qemu-devel] [PATCH] Reducing stack frame size in stream_process_mem2s()

[Edgar E. Iglesias]

On Mon, Oct 03, 2016 at 10:32:40PM +0530, Rutuja Shah wrote:
> ++ stefan Sorry for the typo.
> Regards
> Rutuja Shah
> 
> 
> On Mon, Oct 3, 2016 at 10:26 PM,   wrote:
> > From: Rutuja Shah 
> >
> > This patch allocates memory for txbuf array on the heap rather than the 
> > stack.
> > As a result, the stack frame size is reduced.
> >
> > Signed-off-by: Rutuja Shah 
> > ---
> >  hw/dma/xilinx_axidma.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> > index b135a5f..6c63575 100644
> > --- a/hw/dma/xilinx_axidma.c
> > +++ b/hw/dma/xilinx_axidma.c
> > @@ -256,13 +256,14 @@ static void stream_process_mem2s(struct Stream *s, 
> > StreamSlave *tx_data_dev,
> >   StreamSlave *tx_control_dev)
> >  {
> >  uint32_t prev_d;
> > -unsigned char txbuf[16 * 1024];
> > +unsigned char *txbuf;
> >  unsigned int txlen;
> >
> >  if (!stream_running(s) || stream_idle(s)) {
> >  return;
> >  }
> >
> > +txbuf = g_malloc(16 * 1024);

Hi,

Two comments.

We need to move the allocation from the data-path to initialization of the DMA 
objects. (e.g put txbuf into the Stream struct)
We also need to fix up the use of sizeof txbuf.

Best regards,
Edgar


> >  while (1) {
> >  stream_desc_load(s, s->regs[R_CURDESC]);
> >
> > @@ -304,6 +305,7 @@ static void stream_process_mem2s(struct Stream *s, 
> > StreamSlave *tx_data_dev,
> >  break;
> >  }
> >  }
> > +g_free(txbuf);
> >  }
> >
> >  static size_t stream_process_s2mem(struct Stream *s, unsigned char *buf,
> > --
> > 1.9.1
> >

Re: [Qemu-devel] [PATCH v1 1/1] docs/generic-loader: Add restrictions and ToDos

[Eric Blake]

On 10/03/2016 03:42 PM, Alistair Francis wrote:

>>>  An example of loading an ELF file which CPU0 will boot is shown below:
>>>  -device loader,file=./images/boot.elf,cpu-num=0
>>> +
>>> +Restrictions and ToDos
>>> +-
>>
>> Might be worth it to have the --- line up with the text above.
> 
> It's the same length as all the other --- lines. Should I increase
> them all to the same length as the title?

It's only cosmetic, but yes, a lot of files under docs/ use
variable-length separators that act as an underline to the text above,
rather than fixed-length separators that have no relation to the text
they are emphasizing.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v1 1/1] docs/generic-loader: Add restrictions and ToDos

[Alistair Francis]

On Mon, Oct 3, 2016 at 1:35 PM, Eric Blake  wrote:
> On 10/03/2016 03:18 PM, Alistair Francis wrote:
>> Add a list of known restrictions and future work that will fix these
>> restrictions.
>>
>> Signed-off-by: Alistair Francis 
>> ---
>>
>>  docs/generic-loader.txt | 7 +++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
>> index 8fcb550..305cbc8 100644
>> --- a/docs/generic-loader.txt
>> +++ b/docs/generic-loader.txt
>> @@ -82,3 +82,10 @@ with a '0x'.
>>
>>  An example of loading an ELF file which CPU0 will boot is shown below:
>>  -device loader,file=./images/boot.elf,cpu-num=0
>> +
>> +Restrictions and ToDos
>> +-
>
> Might be worth it to have the --- line up with the text above.

It's the same length as all the other --- lines. Should I increase
them all to the same length as the title?

Thanks,

Alistair

>
>> + - At the moment it is just assumed that if you specify a cpu-num then you
>> +   want to set the PC as well. This might not always be the case. In future
>> +   the internal state 'set_pc' (which exists in the generic loader now) 
>> should
>> +   be exposed to the user so that they can choose if the PC is set or not.
>>
>
> --
> Eric Blake   eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>

Re: [Qemu-devel] [PATCH] Reducing stack frame size in stream_process_mem2s()

[Paolo Bonzini]

On 03/10/2016 18:56, rutu.shah...@gmail.com wrote:
> From: Rutuja Shah 
> 
> This patch allocates memory for txbuf array on the heap rather than the stack.
> As a result, the stack frame size is reduced.
> 
> Signed-off-by: Rutuja Shah 
> ---
>  hw/dma/xilinx_axidma.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index b135a5f..6c63575 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -256,13 +256,14 @@ static void stream_process_mem2s(struct Stream *s, 
> StreamSlave *tx_data_dev,
>   StreamSlave *tx_control_dev)
>  {
>  uint32_t prev_d;
> -unsigned char txbuf[16 * 1024];
> +unsigned char *txbuf;
>  unsigned int txlen;
>  
>  if (!stream_running(s) || stream_idle(s)) {
>  return;
>  }
>  
> +txbuf = g_malloc(16 * 1024);

Hello, please place the allocated buffer in struct Stream (see
xilinx_axidma_realize) instead.  This function is a hot path, and it's
better to keep allocations outside it.

Thanks,

Paolo


>  while (1) {
>  stream_desc_load(s, s->regs[R_CURDESC]);
>  
> @@ -304,6 +305,7 @@ static void stream_process_mem2s(struct Stream *s, 
> StreamSlave *tx_data_dev,
>  break;
>  }
>  }
> +g_free(txbuf);
>  }
>  
>  static size_t stream_process_s2mem(struct Stream *s, unsigned char *buf,
> 

Re: [Qemu-devel] [PATCH v1 1/1] docs/generic-loader: Add restrictions and ToDos

[Eric Blake]

On 10/03/2016 03:18 PM, Alistair Francis wrote:
> Add a list of known restrictions and future work that will fix these
> restrictions.
> 
> Signed-off-by: Alistair Francis 
> ---
> 
>  docs/generic-loader.txt | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
> index 8fcb550..305cbc8 100644
> --- a/docs/generic-loader.txt
> +++ b/docs/generic-loader.txt
> @@ -82,3 +82,10 @@ with a '0x'.
>  
>  An example of loading an ELF file which CPU0 will boot is shown below:
>  -device loader,file=./images/boot.elf,cpu-num=0
> +
> +Restrictions and ToDos
> +-

Might be worth it to have the --- line up with the text above.

> + - At the moment it is just assumed that if you specify a cpu-num then you
> +   want to set the PC as well. This might not always be the case. In future
> +   the internal state 'set_pc' (which exists in the generic loader now) 
> should
> +   be exposed to the user so that they can choose if the PC is set or not.
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] Baremetal Netduino2 -- cannot output on UARTs 2-4

[Seth K]

I have made a bare metal "Hello World" program for the Netduino2. I have
pushed it here:

https://github.com/skintigh/baremetal_netduino2

It should output "Test 1/4" to USART 1, "Test 2/4" to USART 2, "Test 3/4"
to USART 3 and "Test 4/4" to UART 4.

What actually happens in QEMU is only the first string is output. That may
be a command line argument error on my part, so for a sanity check I put
printf statements in the function stm32f2xx_usart_write in
qemu/hw/char/stm32f2xx_usart.c and recompiled qemu. The result is text sent
to UART1 and UART4 make is to the function (though only 1 is output), while
writes to 2 and 3 simply disappear and never make it to that function. I
assumed all writes to UARTs would go to that function.

Am I doing something dumb? Is this a bug? Any help would be greatly
appreciated.

Thanks,
Seth

[Qemu-devel] [PATCH v1 1/1] docs/generic-loader: Add restrictions and ToDos

[Alistair Francis]

Add a list of known restrictions and future work that will fix these
restrictions.

Signed-off-by: Alistair Francis 
---

 docs/generic-loader.txt | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
index 8fcb550..305cbc8 100644
--- a/docs/generic-loader.txt
+++ b/docs/generic-loader.txt
@@ -82,3 +82,10 @@ with a '0x'.
 
 An example of loading an ELF file which CPU0 will boot is shown below:
 -device loader,file=./images/boot.elf,cpu-num=0
+
+Restrictions and ToDos
+-
+ - At the moment it is just assumed that if you specify a cpu-num then you
+   want to set the PC as well. This might not always be the case. In future
+   the internal state 'set_pc' (which exists in the generic loader now) should
+   be exposed to the user so that they can choose if the PC is set or not.
-- 
2.7.4

Re: [Qemu-devel] [libvirt] QMP stubs: how to return "not implemented" errors?

[Eric Blake]

On 10/03/2016 02:04 PM, Eduardo Habkost wrote:
> Hi,
> 
> When adding new QMP commands that are implemented by
> arch-specific code, we have been adding stubs that report
> QERR_UNSUPPORTED (see stubs/arch-query-cpu-model-expansion.c for
> an example).
> 

> 3.1) Removing the command from query-commands and from the QAPI
>schema on binaries that don't implement the command.
>Needlessly complex?

Ideal goal, but we aren't there yet.

> 3.2) Removing the unimplemented command from query-commands only
>(by calling qmp_disable_command()), but keeping it on the QAPI
>schema. I am not sure it's OK to do that. If it is, this
>sounds like the simplest solution.

We already have existing commands in this category, and it is
conceptually the easiest (if query-commands doesn't list the command,
then the command doesn't work even if the introspection still shows it).
 In fact, that was part of the reason that Marc-Andre's work to remove
middle mode took so many revisions, because we were figuring out if it
was possible to get to the ideal option 3.1 (answer was not yet), then
deciding what the least disgusting hack was for sticking with option 3.2
after we dropped qemu-commands.hx (the hack currently in place, as of
qemu commit 5032a16d, is that all commands are now unconditionally
generated, but the tricky ones are now explicitly unregistered in C code
guarded by negative #ifdefs for the platforms where the command used to
be only conditionally generated from positive ifdefs in the .hx file, so
that the query-commands result is identical).


-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] QMP stubs: how to return "not implemented" errors?

[Jiri Denemark]

On Mon, Oct 03, 2016 at 16:04:42 -0300, Eduardo Habkost wrote:
> Hi,
> 
> When adding new QMP commands that are implemented by
> arch-specific code, we have been adding stubs that report
> QERR_UNSUPPORTED (see stubs/arch-query-cpu-model-expansion.c for
> an example).
> 
> But we are using GenericError for that, and this prevents clients
> from reliably checking if the command is really implemented by
> the QEMU binary.
> 
> What should be the right solution for this? Some of the options I
> have considered are:
> 
> 1) Using CommandNotFound as the error class in the stubs. This
>sounds wrong because the command exists (it is present in
>query-commands and in the QAPI schema).
> 2) Creating a CommandNotImplemented error class. Simple to do,
>but it would require clients to make two separate checks,
>before concluding that the command is available (checking
>query-commands or query-qmp-schema, and then checking for
>CommandNotImplemented errors).

Both options require an extra step to check whether a particular command
is implemented or not. It would be highly appreciated if we didn't have
to go this way since it would seriously complicate the probing process.
We'd need to run each command with some artificial, but sane enough
arguments to bypass arguments checking code:

(QEMU) query-cpu-model-expansion type=full
{"error": {"class": "GenericError", "desc": "Invalid parameter type for
'model', expected: QDict"}}

(QEMU) query-cpu-model-expansion type=full model={'name':'host'}
{"error": {"class": "GenericError", "desc": "this feature or command is
not currently supported"}}

(QEMU) query-cpu-model-expansion type=full model={'name':'Penryn'}
{"error": {"class": "GenericError", "desc": "this feature or command is
not currently supported"}}


> 3.1) Removing the command from query-commands and from the QAPI
>schema on binaries that don't implement the command.
>Needlessly complex?
> 3.2) Removing the unimplemented command from query-commands only
>(by calling qmp_disable_command()), but keeping it on the QAPI
>schema. I am not sure it's OK to do that. If it is, this
>sounds like the simplest solution.

These options are both acceptable to libvirt. So it really depends what
is acceptable to QEMU...

From my POV (which is quite ignorant to QEMU internals in this area),
there are a few other options:

- implementing a new QMP command to list unsupported commands
  (e.g., query-unsupported-commands)
- adding a flag to qmp-schema which would indicate whether a command is
  supported or not
- even a new stupid command that would take another command as an
  argument and return whether it's supported or not would be better than
  having to run each command individually with special arguments

I'm not saying these options are clean or doable, I'm just brainstorming
here.

Jirka

Re: [Qemu-devel] [PATCH v4 13/35] tcg: Add atomic helpers

[Alex Bennée]

Richard Henderson  writes:

> Add all of cmpxchg, op_fetch, fetch_op, and xchg.
> Handle both endian-ness, and sizes up to 8.
> Handle expanding non-atomically, when emulating in serial.
>
> Signed-off-by: Richard Henderson 
> ---

> diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
> index 291d50b..65e3663 100644
> --- a/tcg/tcg-op.c
> +++ b/tcg/tcg-op.c

> +void tcg_gen_atomic_cmpxchg_i32(TCGv_i32 retv, TCGv addr, TCGv_i32 cmpv,
> +TCGv_i32 newv, TCGArg idx, TCGMemOp memop)
> +{
> +memop = tcg_canonicalize_memop(memop, 0, 0);
> +
> +if (!parallel_cpus) {

This breaks the compile because parallel_cpus isn't visible to the
function. However I suspect it's because there is a missing patch in
this series (I checked my email and the archive). What happened to 06/35?

> +TCGv_i32 t1 = tcg_temp_new_i32();
> +TCGv_i32 t2 = tcg_temp_new_i32();
> +
> +tcg_gen_ext_i32(t2, cmpv, memop & MO_SIZE);
> +
> +tcg_gen_qemu_ld_i32(t1, addr, idx, memop & ~MO_SIGN);
> +tcg_gen_movcond_i32(TCG_COND_EQ, t2, t1, t2, newv, t1);
> +tcg_gen_qemu_st_i32(t2, addr, idx, memop);
> +tcg_temp_free_i32(t2);
> +
> +if (memop & MO_SIGN) {
> +tcg_gen_ext_i32(retv, t1, memop);
> +} else {
> +tcg_gen_mov_i32(retv, t1);
> +}
> +tcg_temp_free_i32(t1);
> +} else {
> +gen_atomic_cx_i32 gen;
> +
> +gen = table_cmpxchg[memop & (MO_SIZE | MO_BSWAP)];
> +tcg_debug_assert(gen != NULL);
> +
> +#ifdef CONFIG_SOFTMMU
> +{
> +TCGv_i32 oi = tcg_const_i32(make_memop_idx(memop & ~MO_SIGN, 
> idx));
> +gen(retv, tcg_ctx.tcg_env, addr, cmpv, newv, oi);
> +tcg_temp_free_i32(oi);
> +}
> +#else
> +gen(retv, tcg_ctx.tcg_env, addr, cmpv, newv);
> +#endif
> +
> +if (memop & MO_SIGN) {
> +tcg_gen_ext_i32(retv, retv, memop);
> +}
> +}
> +}
> +
> +void tcg_gen_atomic_cmpxchg_i64(TCGv_i64 retv, TCGv addr, TCGv_i64 cmpv,
> +TCGv_i64 newv, TCGArg idx, TCGMemOp memop)
> +{
> +memop = tcg_canonicalize_memop(memop, 1, 0);
> +
> +if (!parallel_cpus) {
> +TCGv_i64 t1 = tcg_temp_new_i64();
> +TCGv_i64 t2 = tcg_temp_new_i64();
> +
> +tcg_gen_ext_i64(t2, cmpv, memop & MO_SIZE);


--
Alex Bennée

Re: [Qemu-devel] [PATCH v4 00/35] cmpxchg-based emulation of atomics

[Alex Bennée]

Richard Henderson  writes:

> Rebased on top of Paolo's safe-work series, which means
> that we now have cpu_exec_step_atomic for system mode as
> well as linux-user.  This should fix the problems with
> atomic access to notdirty pages that have been reported.

Sorry it has taken me so long to get back to this series. Now Paolo's
series has been merged is it worth re-basing? I've hit a number of minor
niggly merge conflicts applying it to master.

>
> Folded in some feedback from Alex from v3.
>
> A complete tree may be found at
>
>   git://github.com/rth7680/qemu.git atomic-4
>
>
> r~
>
>
> Emilio G. Cota (18):
>   atomics: add atomic_xor
>   atomics: add atomic_op_fetch variants
>   target-i386: emulate LOCK'ed cmpxchg using cmpxchg helpers
>   target-i386: emulate LOCK'ed OP instructions using atomic helpers
>   target-i386: emulate LOCK'ed INC using atomic helper
>   target-i386: emulate LOCK'ed NOT using atomic helper
>   target-i386: emulate LOCK'ed NEG using cmpxchg helper
>   target-i386: emulate LOCK'ed XADD using atomic helper
>   target-i386: emulate LOCK'ed BTX ops using atomic helpers
>   target-i386: emulate XCHG using atomic helper
>   target-i386: remove helper_lock()
>   tests: add atomic_add-bench
>   target-arm: emulate LL/SC using cmpxchg helpers
>   target-arm: emulate SWP with atomic_xchg helper
>   target-arm: emulate aarch64's LL/SC using cmpxchg helpers
>   linux-user: remove handling of ARM's EXCP_STREX
>   linux-user: remove handling of aarch64's EXCP_STREX
>   target-arm: remove EXCP_STREX + cpu_exclusive_{test, info}
>
> Richard Henderson (17):
>   exec: Avoid direct references to Int128 parts
>   int128: Use __int128 if available
>   int128: Add int128_make128
>   tcg: Add EXCP_ATOMIC
>   HACK: Always enable parallel_cpus
>   cputlb: Replace SHIFT with DATA_SIZE
>   cputlb: Move probe_write out of softmmu_template.h
>   cputlb: Remove includes from softmmu_template.h
>   cputlb: Move most of iotlb code out of line
>   cputlb: Tidy some macros
>   tcg: Add atomic helpers
>   tcg: Add atomic128 helpers
>   tcg: Add CONFIG_ATOMIC64
>   tcg: Emit barriers with parallel_cpus
>   target-arm: Rearrange aa32 load and store functions
>   target-alpha: Introduce MMU_PHYS_IDX
>   target-alpha: Emulate LL/SC using cmpxchg helpers
>
>  Makefile.objs  |   1 -
>  Makefile.target|   1 +
>  atomic_template.h  | 211 +
>  configure  |  62 +++-
>  cpu-exec-common.c  |   6 +
>  cpu-exec.c |  30 
>  cpus.c |   2 +
>  cputlb.c   | 203 ++--
>  exec.c |   4 +-
>  include/exec/cpu-all.h |   1 +
>  include/exec/exec-all.h|   1 +
>  include/qemu-common.h  |   1 +
>  include/qemu/atomic.h  |  40 -
>  include/qemu/int128.h  | 171 +++-
>  linux-user/main.c  | 312 ++--
>  softmmu_template.h | 104 ++--
>  target-alpha/cpu.h |  22 +--
>  target-alpha/helper.c  |  14 +-
>  target-alpha/helper.h  |   9 --
>  target-alpha/machine.c |   2 -
>  target-alpha/mem_helper.c  |  73 -
>  target-alpha/translate.c   | 148 +
>  target-arm/cpu.h   |  17 +-
>  target-arm/helper-a64.c| 113 +
>  target-arm/helper-a64.h|   2 +
>  target-arm/internals.h |   4 +-
>  target-arm/translate-a64.c | 106 ++---
>  target-arm/translate.c | 342 ++-
>  target-arm/translate.h |   4 -
>  target-i386/helper.h   |   4 +-
>  target-i386/mem_helper.c   | 153 --
>  target-i386/translate.c| 386 
> +
>  tcg-runtime.c  |  74 +++--
>  tcg/tcg-op.c   | 354 +++--
>  tcg/tcg-op.h   |  44 ++
>  tcg/tcg-runtime.h  | 109 +
>  tcg/tcg.h  |  85 ++
>  tests/.gitignore   |   1 +
>  tests/Makefile.include |   4 +-
>  tests/atomic_add-bench.c   | 181 +
>  tests/test-int128.c|  22 +--
>  translate-all.c|   1 +
>  42 files changed, 2336 insertions(+), 1088 deletions(-)
>  create mode 100644 atomic_template.h
>  create mode 100644 tests/atomic_add-bench.c


--
Alex Bennée

Re: [Qemu-devel] [Bug 1626972] Re: [PATCH] util: secure memfd_create fallback mechanism

[Rafael David Tinoco]

Yes, definitely. Check this:

/**
 * @qemu_chr_fe_set_msgfds:
 *
 * For backends capable of fd passing, set an array of fds to be passed with
 * the next send operation.
 * A subsequent call to this function before calling a write function will
 * result in overwriting the fd array with the new value without being send.
 * Upon writing the message the fd array is freed.
 *
 * Returns: -1 if fd passing isn't supported.
 */
int qemu_chr_fe_set_msgfds(CharDriverState *s, int *fds, int num);



So, at least for vhost_dev_log_resize, this "interface" is being implemented:

vhost_user_set_log_base -> VhostUserMsg = VHOST_USER_SET_LOG_BASE

vhost_user_write(with the VHOST_USER_GET_LOG_BASE message):

- configures the file descriptors(... , fds, fd_num)
  qemu_chr_fe_set_msgfds

- writes them down the char driver
  qemu_chr_fe_write_all

> On Oct 03, 2016, at 15:46, Rafael David Tinoco  
> wrote:
> 
>> So you're saying that the file descriptor here is actually getting
>> passed to a different process for it to use ?

Re: [Qemu-devel] [QEMU PATCH v5 0/6] migration: ensure hotplug and migration work together

[Jianjun Duan]

I will address the style issues together with any other possible
comments. The build test failed on code not in my patches.

Thanks,
Jianjun

On 10/03/2016 11:24 AM, Jianjun Duan wrote:
> Hi all,
>The previous patches seem to get buried deep somewhere. I am sending the 
> lated rebased version. Comments are welcome.
> 
> v5: - Rebased to David's ppc-for-2.8. Previous versions are:
> 
> v4: - Introduce a way to set customized instance_id in SaveStateEntry. Use it
>   to set instance_id for DRC using its unique index to address David 
>   Gibson's concern.
> - Rename VMS_CSTM to VMS_LINKED based on Paolo Bonzini's suggestions.
> - Clean up qjson stuff in put_qtailq. 
> - Add trace for put_qtailq and get_qtailq based on David Gilbert's 
>   suggestion.
> 
> - Based on David's ppc-for-2.7. 
> 
> v3: - Simplify overall design followng discussion with Paolo. No longer need
>   metadata to migrate QTAILQ.
> - Extend VMStateInfo instead of adding similar fields to VMStateField.
> - Clean up macros in qemu/queue.h.
> (link: https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg05695.html)
> 
> v2: - Introduce a general approach to migrate QTAILQ in qemu/queue.h.
> - Migrate signalled field in the DRC state.
> - Put the newly added migrating fields in subsections so that backward 
>   migration is not broken.  
> - Set detach_cb field right after migration so that a migrated hot-unplug
>   event could finish its course.
> (link: https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg04188.html)
> 
> v1: - Inital version.
> (link: https://lists.nongnu.org/archive/html/qemu-devel/2016-04/msg02601.html)
> 
> To make guest device (PCI, CPU and memory) hotplug work together 
> with guest migration, spapr drc state needs be transmitted in
> migration. This patch defines the VMStateDescription struct for
> spapr drc state to enable it.
> 
> To fix the potential racing between hotplug events on guest and 
> guest migration, ccs_list and pending_events of spapr state need be 
> transmitted in migration. This patch also takes care of it.
> 
> Jianjun Duan (6):
>   migration: alternative way to set instance_id in SaveStateEntry
>   migration: spapr_drc: defined VMStateDescription struct
>   migration: extend VMStateInfo
>   migration: migrate QTAILQ
>   migration: spapr: migrate ccs_list in spapr state
>   migration: spapr: migrate pending_events of spapr state
> 
>  hw/net/vmxnet3.c|  18 +++--
>  hw/nvram/eeprom93xx.c   |   6 +-
>  hw/nvram/fw_cfg.c   |   6 +-
>  hw/pci/msix.c   |   6 +-
>  hw/pci/pci.c|  12 ++--
>  hw/pci/shpc.c   |   5 +-
>  hw/ppc/spapr.c  |  67 ++
>  hw/ppc/spapr_drc.c  |  69 +++
>  hw/ppc/spapr_events.c   |  22 +++---
>  hw/ppc/spapr_pci.c  |  22 ++
>  hw/scsi/scsi-bus.c  |   6 +-
>  hw/timer/twl92230.c |   6 +-
>  hw/usb/redirect.c   |  18 +++--
>  hw/virtio/virtio-pci.c  |   6 +-
>  hw/virtio/virtio.c  |   6 +-
>  include/hw/ppc/spapr.h  |   3 +-
>  include/hw/ppc/spapr_drc.h  |   9 +++
>  include/hw/qdev-core.h  |   6 ++
>  include/migration/vmstate.h |  36 --
>  include/qemu/queue.h|  32 +
>  migration/savevm.c  |  25 +--
>  migration/trace-events  |   4 ++
>  migration/vmstate.c | 161 
> ++--
>  target-alpha/machine.c  |   5 +-
>  target-arm/machine.c|  12 ++--
>  target-i386/machine.c   |  21 --
>  target-mips/machine.c   |  10 +--
>  target-ppc/machine.c|  10 +--
>  target-sparc/machine.c  |   5 +-
>  29 files changed, 505 insertions(+), 109 deletions(-)
> 

[Qemu-devel] [PULL 1/2] target-i386: Report known CPUID[EAX=0xD, ECX=0]:EAX bits as migratable

[Eduardo Habkost]

A regression was introduced by commit 96193c22a "target-i386:
Move xsave component mask to features array": all
CPUID[EAX=0xD,ECX=0]:EAX bits were being reported as unmigratable
because they don't have feature names defined. This broke
"-cpu host" because it enables only migratable features by
default.

This adds a new field to FeatureWordInfo: migratable_flags, which
will make those features be reported as migratable even if they
don't have a property name defined.

Reported-by: Wanpeng Li 
Cc: Paolo Bonzini 
Reviewed-by: Wanpeng Li 
Signed-off-by: Eduardo Habkost 
---
 target-i386/cpu.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 09b..0807e92 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -258,6 +258,7 @@ typedef struct FeatureWordInfo {
 int cpuid_reg;/* output register (R_* constant) */
 uint32_t tcg_features; /* Feature flags supported by TCG */
 uint32_t unmigratable_flags; /* Feature flags known to be unmigratable */
+uint32_t migratable_flags; /* Feature flags known to be migratable */
 } FeatureWordInfo;
 
 static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
@@ -494,6 +495,10 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 .cpuid_needs_ecx = true, .cpuid_ecx = 0,
 .cpuid_reg = R_EAX,
 .tcg_features = ~0U,
+.migratable_flags = XSTATE_FP_MASK | XSTATE_SSE_MASK |
+XSTATE_YMM_MASK | XSTATE_BNDREGS_MASK | XSTATE_BNDCSR_MASK |
+XSTATE_OPMASK_MASK | XSTATE_ZMM_Hi256_MASK | XSTATE_Hi16_ZMM_MASK |
+XSTATE_PKRU_MASK,
 },
 [FEAT_XSAVE_COMP_HI] = {
 .cpuid_eax = 0xD,
@@ -600,15 +605,13 @@ static uint32_t x86_cpu_get_migratable_flags(FeatureWord 
w)
 
 for (i = 0; i < 32; i++) {
 uint32_t f = 1U << i;
-/* If the feature name is unknown, it is not supported by QEMU yet */
-if (!wi->feat_names[i]) {
-continue;
-}
-/* Skip features known to QEMU, but explicitly marked as unmigratable 
*/
-if (wi->unmigratable_flags & f) {
-continue;
+
+/* If the feature name is known, it is implicitly considered 
migratable,
+ * unless it is explicitly set in unmigratable_flags */
+if ((wi->migratable_flags & f) ||
+(wi->feat_names[i] && !(wi->unmigratable_flags & f))) {
+r |= f;
 }
-r |= f;
 }
 return r;
 }
-- 
2.7.4

Re: [Qemu-devel] [PULL 14/20] target-i386: Move xsave component mask to features array

[Eduardo Habkost]

On Mon, Oct 03, 2016 at 05:42:36PM +0100, Daniel P. Berrange wrote:
> On Tue, Sep 27, 2016 at 05:12:24PM -0300, Eduardo Habkost wrote:
> > This will reuse the existing check/enforce logic in
> > x86_cpu_filter_features() to check the xsave component bits
> > against GET_SUPPORTED_CPUID.
> > 
> > Reviewed-by: Richard Henderson 
> > Signed-off-by: Eduardo Habkost 
> > ---
> >  target-i386/cpu.c | 42 --
> >  target-i386/cpu.h |  3 ++-
> >  2 files changed, 30 insertions(+), 15 deletions(-)
> 
> git bisect tells me that this change is responsible for breaking booting
> of a guest of mine which uses -cpu host
> 
> GRUB works, but the guest kernel hangs immediately after printing
> 
> "Probing EDD (edd=off to disable)... ok"
> 
> Removing '-cpu host' lets it work again.
[...]
> In case its relevant, QEMU prints this to stderr:
> 
> warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 0]
> warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 1]
> warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 2]

This was reported by Wanpeng Li last week. The fix ("target-i386:
Report known CPUID[EAX=0xD,ECX=0]:EAX bits as migratable") is in
the pull request I sent today.

You can also work around it by using "-cpu host,migratable=off".

-- 
Eduardo

[Qemu-devel] [PULL 2/2] target-i386: Correct family/model/stepping for Opteron_G3

[Eduardo Habkost]

From: Evgeny Yakovlev 

Current CPU definition for AMD Opteron third generation includes
features like SSE4a and LAHF_LM support in emulated CPUID. These
features are present in K8 rev.E or K10 CPUs and later. However,
current G3 family and model describe 2nd generation K8 cores instead.

This is incorrect but was considered harmless until our tests found a
problem with linux kernels >= 3.10 (and maybe earlier) which specifically
check for Opteron K8 model when parsing CPUID leaf 0x8001:
http://lxr.free-electrons.com/source/arch/x86/kernel/cpu/amd.c?v=3.16#L552
This code will disable LAHF_LM feature in /proc/cpuinfo if model number
is inconsistent.

This change sets Opteron_G3 family/model/stepping to 16/2/3 which is
a proper Opteron 3rd generation 2350 CPU.

Signed-off-by: Evgeny Yakovlev 
Signed-off-by: Denis V. Lunev 
CC: Paolo Bonzini 
CC: Richard Henderson 
CC: Eduardo Habkost 
Signed-off-by: Eduardo Habkost 
---
 include/hw/i386/pc.h | 15 +++
 target-i386/cpu.c|  6 +++---
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 47bdf10..cb2df83 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -379,6 +379,21 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 .driver   = TYPE_X86_CPU,\
 .property = "full-cpuid-auto-level",\
 .value= "off",\
+},\
+{\
+.driver   = "Opteron_G3" "-" TYPE_X86_CPU,\
+.property = "family",\
+.value= "15",\
+},\
+{\
+.driver   = "Opteron_G3" "-" TYPE_X86_CPU,\
+.property = "model",\
+.value= "6",\
+},\
+{\
+.driver   = "Opteron_G3" "-" TYPE_X86_CPU,\
+.property = "stepping",\
+.value= "1",\
 },
 
 #define PC_COMPAT_2_6 \
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0807e92..1c57fce 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1428,9 +1428,9 @@ static X86CPUDefinition builtin_x86_defs[] = {
 .name = "Opteron_G3",
 .level = 5,
 .vendor = CPUID_VENDOR_AMD,
-.family = 15,
-.model = 6,
-.stepping = 1,
+.family = 16,
+.model = 2,
+.stepping = 3,
 .features[FEAT_1_EDX] =
 CPUID_VME | CPUID_SSE2 | CPUID_SSE | CPUID_FXSR | CPUID_MMX |
 CPUID_CLFLUSH | CPUID_PSE36 | CPUID_PAT | CPUID_CMOV | CPUID_MCA |
-- 
2.7.4

[Qemu-devel] [PULL 0/2] x86 fixes

[Eduardo Habkost]

The following changes since commit c5d128ffeb5357df1ea3e6de0c13b3d6a09f6064:

  Merge remote-tracking branch 'remotes/borntraeger/tags/s390x-20160927' into 
staging (2016-09-30 23:45:56 +0100)

are available in the git repository at:

  git://github.com/ehabkost/qemu.git tags/x86-pull-request

for you to fetch changes up to 339892d758efb2d0954160d41736a0eac9875d67:

  target-i386: Correct family/model/stepping for Opteron_G3 (2016-10-03 
16:06:43 -0300)


x86 bug fixes

Fix for a XSAVE regression when using "-cpu host", and a fix on
the Opteron_G3 CPU model.



Eduardo Habkost (1):
  target-i386: Report known CPUID[EAX=0xD,ECX=0]:EAX bits as migratable

Evgeny Yakovlev (1):
  target-i386: Correct family/model/stepping for Opteron_G3

 include/hw/i386/pc.h | 15 +++
 target-i386/cpu.c| 25 ++---
 2 files changed, 29 insertions(+), 11 deletions(-)

-- 
2.7.4

Re: [Qemu-devel] [QEMU PATCH v5 0/6] migration: ensure hotplug and migration work together

[no-reply]

Hi,

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

Type: series
Message-id: 1475519097-27611-1-git-send-email-du...@linux.vnet.ibm.com
Subject: [Qemu-devel] [QEMU PATCH v5 0/6] migration: ensure hotplug and 
migration work together

=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
make J=8 docker-test-quick@centos6
make J=8 docker-test-mingw@fedora
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
d935e1f migration: spapr: migrate pending_events of spapr state
64aa229 migration: spapr: migrate ccs_list in spapr state
ecf2d6c migration: migrate QTAILQ
6f641b8 migration: extend VMStateInfo
803e704 migration: spapr_drc: defined VMStateDescription struct
08b0c64 migration: alternative way to set instance_id in SaveStateEntry

=== OUTPUT BEGIN ===
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into 'dtc'...
Submodule path 'dtc': checked out '65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf'
  BUILD centos6
  ARCHIVE qemu.tgz
  ARCHIVE dtc.tgz
  COPY RUNNER
  RUN test-quick in centos6
Packages installed:
SDL-devel-1.2.14-7.el6_7.1.x86_64
ccache-3.1.6-2.el6.x86_64
epel-release-6-8.noarch
gcc-4.4.7-17.el6.x86_64
git-1.7.1-4.el6_7.1.x86_64
glib2-devel-2.28.8-5.el6.x86_64
libfdt-devel-1.4.0-1.el6.x86_64
make-3.81-23.el6.x86_64
package g++ is not installed
pixman-devel-0.32.8-1.el6.x86_64
tar-1.23-15.el6_8.x86_64
zlib-devel-1.2.3-29.el6.x86_64

Environment variables:
PACKAGES=libfdt-devel ccache tar git make gcc g++ zlib-devel 
glib2-devel SDL-devel pixman-devel epel-release
HOSTNAME=cb3993c582fb
TERM=xterm
MAKEFLAGS= -j8
HISTSIZE=1000
J=8
USER=root
CCACHE_DIR=/var/tmp/ccache
EXTRA_CONFIGURE_OPTS=
V=
SHOW_ENV=1
MAIL=/var/spool/mail/root
PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
PWD=/
LANG=en_US.UTF-8
TARGET_LIST=
HISTCONTROL=ignoredups
SHLVL=1
HOME=/root
TEST_DIR=/tmp/qemu-test
LOGNAME=root
LESSOPEN=||/usr/bin/lesspipe.sh %s
FEATURES= dtc
DEBUG=
G_BROKEN_FILENAMES=1
CCACHE_HASHDIR=
_=/usr/bin/env

Configure options:
--enable-werror --target-list=x86_64-softmmu,aarch64-softmmu 
--prefix=/tmp/qemu-test/src/tests/docker/install
No C++ compiler available; disabling C++ specific optional code
Install prefix/tmp/qemu-test/src/tests/docker/install
BIOS directory/tmp/qemu-test/src/tests/docker/install/share/qemu
binary directory  /tmp/qemu-test/src/tests/docker/install/bin
library directory /tmp/qemu-test/src/tests/docker/install/lib
module directory  /tmp/qemu-test/src/tests/docker/install/lib/qemu
libexec directory /tmp/qemu-test/src/tests/docker/install/libexec
include directory /tmp/qemu-test/src/tests/docker/install/include
config directory  /tmp/qemu-test/src/tests/docker/install/etc
local state directory   /tmp/qemu-test/src/tests/docker/install/var
Manual directory  /tmp/qemu-test/src/tests/docker/install/share/man
ELF interp prefix /usr/gnemul/qemu-%M
Source path   /tmp/qemu-test/src
C compilercc
Host C compiler   cc
C++ compiler  
Objective-C compiler cc
ARFLAGS   rv
CFLAGS-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g 
QEMU_CFLAGS   -I/usr/include/pixman-1-pthread -I/usr/include/glib-2.0 
-I/usr/lib64/glib-2.0/include   -fPIE -DPIE -m64 -D_GNU_SOURCE 
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv  -Wendif-labels -Wmissing-include-dirs 
-Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self 
-Wignored-qualifiers -Wold-style-declaration -Wold-style-definition 
-Wtype-limits -fstack-protector-all
LDFLAGS   -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -pie -m64 -g 
make  make
install   install
pythonpython -B
smbd  /usr/sbin/smbd
module supportno
host CPU  x86_64
host big endian   no
target list   x86_64-softmmu aarch64-softmmu
tcg debug enabled no
gprof enabled no
sparse enabledno
strip binariesyes
profiler  no
static build  no
pixmansystem
SDL support   yes (1.2.14)
GTK support   no 
GTK GL supportno
VTE support   no 
TLS priority  NORMAL
GNUTLS supportno
GNUTLS rndno
libgcrypt no
libgcrypt kdf no
nettleno 
nettle kdfno
libtasn1  no
curses supportno
virgl support no
curl support  no
mingw32 support   no
Audio drivers oss
Block whitelist (rw) 
Block whitelist (ro) 
VirtFS supportno
VNC support   yes
VNC SASL support  no
VNC JPEG support  no
VNC PNG support   no
xen support   no
brlapi supportno
bluez  supportno
Documentation no
PIE   yes
vde 

Re: [Qemu-devel] Getting Started with Outreachy (Was: (no subject))

[Stefan Hajnoczi]

On Mon, Oct 3, 2016 at 6:57 PM, Shreya Shrivastava  wrote:
>>> I am interested in applying for Outreachy 2016 December- March round by
>>> contributing to  VIRTIO 1.0 support in libqos project for Qemu.
>>>
>>> Kindly let me know how to get started with this project.

Hi,
The project is about adding VIRTIO 1.0 support so you'll have to look
at the VIRTIO 1.0 specification and the source code linked from here:
http://qemu-project.org/Outreachy_2016_DecemberMarch#VIRTIO_1.0_support_in_libqos

Once you have an idea of what adding VIRTIO 1.0 support entails you
will be able to fill out the application form including a rough
project plan for the 12-week project.

Feel free to email me at stefa...@gmail.com if you have questions
about the project idea or need help understanding the links I
mentioned above.

Stefan

[Qemu-devel] QMP stubs: how to return "not implemented" errors?

[Eduardo Habkost]

Hi,

When adding new QMP commands that are implemented by
arch-specific code, we have been adding stubs that report
QERR_UNSUPPORTED (see stubs/arch-query-cpu-model-expansion.c for
an example).

But we are using GenericError for that, and this prevents clients
from reliably checking if the command is really implemented by
the QEMU binary.

What should be the right solution for this? Some of the options I
have considered are:

1) Using CommandNotFound as the error class in the stubs. This
   sounds wrong because the command exists (it is present in
   query-commands and in the QAPI schema).
2) Creating a CommandNotImplemented error class. Simple to do,
   but it would require clients to make two separate checks,
   before concluding that the command is available (checking
   query-commands or query-qmp-schema, and then checking for
   CommandNotImplemented errors).
3.1) Removing the command from query-commands and from the QAPI
   schema on binaries that don't implement the command.
   Needlessly complex?
3.2) Removing the unimplemented command from query-commands only
   (by calling qmp_disable_command()), but keeping it on the QAPI
   schema. I am not sure it's OK to do that. If it is, this
   sounds like the simplest solution.

Any ideas?

-- 
Eduardo

Re: [Qemu-devel] [Bug 1626972] Re: [PATCH] util: secure memfd_create fallback mechanism

[Rafael David Tinoco]

Hello Daniel,

> On Oct 03, 2016, at 14:55, Daniel P. Berrange  wrote:
> 
>> Well, it unlinks the file but the references are still there while the
>> descriptor isn't closed by this process, or by the one that receives the
>> descriptor (that is why is the "unlink" so early).
>> 
>> If you check vhost_dev_log_resize(), it gets *possible* new vhost log
>> (if a new size is given) and informs the vhost dev driver about the new
>> log base (vhost_ops->vhost_set_log_base).
>> 
>> For vhost_user, this means that the file descriptors for vhost logs are
>> likely going to be passed to vhost backend (fds[] in
>> vhost_user_set_log_base). This is just one example, not sure about
>> others.
>> 
>> Probably the best approach here, like what Marc-André said, is to create
>> some sort of TMPDIR, set by libvirt perhaps ?
> 
> So you're saying that the file descriptor here is actually getting
> passed to a different process for it to use ?
> 
> If so that means we definitely do not want this in TMPDIR. If we
> create a generic file in TMPDIR, then its going to have a generic
> security label. That means that the other process we're giving the
> FD to is going to have to be granted permission to access this FD
> and we certainly don't want to grant permission for it to access
> any of QEMU's other FDs. So for the SELinux integration, we'll
> need this FD to be in a specific directory, so that we can setup
> policy such that the file created gets given a specific SELinux
> label. We can then grant the other process access to only that
> particular file, and not anything else of QEMU's.
> 
> This makes me wonder about the memfd_create() code path too - we'll
> again not want that external process to be granted access to arbitrary
> FDs of QEMU's and I'm not sure of a way to get the memfd  FD to have
> a specific label. So I think it is possible that when using libvirt
> we'll want the ability to tell QEMU to *always* use an explicit file
> in a path libvirt specifies, and never use memfd even if available.

Check this execution path:

(vhost_vsock_device_realize)
  vhost_dev_init
  vhost_commit
  |- vhost_get_log_size
  |...
  |- vhost_dev_log_resize

(vhost_dev_log_resize):
  vhost_log_get -> here if the size is bigger, a new log is created
  dev->vhost_ops->vhost_set_log_base() -> kernel or user vhost driver
  vhost_log_put()



So,

* In case of the kernel mode, this is just a:

vhost in kernel mode = vhost_kernel_set_log_base
return vhost_kernel_call(dev, VHOST_SET_LOG_BASE, );

which makes an ioctl to dev->opaque file descriptor to set a new vhost
log base.

* But in the case of user mode:

vhost in user mode = vhost_user_set_log_base

which gets the log file descriptor (log->fd) and gives to
vhost_user_write. vhost_user_write will do a qemu_chr_fe_set_msgfds
passing the log file descriptors for the backend vhost driver
(CharDriverState).

If I'm reading this right.. if the backend driver is:

static int tcp_set_msgfds(CharDriverState *chr, int *fds, int num)

it would check for:

!qio_channel_has_feature(s->ioc, QIO_CHANNEL_FEATURE_FD_PASS)) {

and configure s->write_msgfds. This would be sent in:

static int tcp_chr_write(CharDriverState *chr, const uint8_t *buf, int
len)

with "io_channel_send_full" + "qio_channel_writev_full + io_writev from
QIOChannelClass.



https://www.berrange.com/posts/2016/08/16/

This, from your blog, probably confirms this behaviour:

"The migration code supports a number of different protocols besides
just “tcp:“. In particular it allows an “fd:” protocol to tell QEMU to
use a passed-in file descriptor, and an “exec:” protocol to tell QEMU to
launch an external command to tunnel the connection. It is desirable to
be able to use TLS with these protocols too, but when using TLS the
client QEMU needs to know the hostname of the target QEMU in order to
correctly validate the x509 certificate it receives. Thus, a second
“tls-hostname” parameter was added to allow QEMU to be informed of the
hostname to use for x509 certificate validation when using a non-tcp
migration protocol. This can be set on the source QEMU prior to starting
the migration using the “migrate_set_str_parameter” monitor command"

=)

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

Title:
  QEMU memfd_create fallback mechanism change for security drivers

Status in QEMU:
  In Progress

Bug description:
  And, when libvirt starts using apparmor, and creating apparmor
  profiles for every virtual machine created in the compute nodes,
  mitaka qemu (2.5 - and upstream also) uses a fallback mechanism for
  creating shared memory for live-migrations. This fall back mechanism,
  on kernels 3.13 - that don't have memfd_create() system-call, try to
  create files on /tmp/ directory and fails.. causing live-migration not
  to work.

  Trusty with kernel 3.13 + Mitaka with qemu 2.5 + apparmor capability 

Re: [Qemu-devel] [PATCH v4 05/12] block/nbd: Add nbd_has_filename_options_conflict()

[Eric Blake]

On 09/28/2016 03:55 PM, Max Reitz wrote:
> Right now, we have four possible options that conflict with specifying
> an NBD filename, and a future patch will add another one ("address").
> This future option is a nested QDict that is flattened at this point,
> requiring us to test each option whether its key has an "address."
> prefix. Therefore, we will then need to iterate through all options.

How does the plans to add 'address.' interact with Dan's patches for
auto-nesting when parsing command line options?

https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg08238.html

I'm wondering if your patches can be made a bit smaller by basing on top
of that work (allowing the command-line user to omit 'address.' and the
parser auto-nests as a result, while the QMP form is always nested).

> 
> Adding this iteration logic now will simplify adding the new option
> later. A nice side effect is that the user will not receive a long list
> of five options which are not supposed to be specified with a filename,
> but we can actually print the problematic option.

On its own, this patch looks reasonable, but I'm a bit worried that with
all the other parsing changes going in, it may result in unused code.

> 
> Signed-off-by: Max Reitz 
> ---
>  block/nbd.c | 26 --
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index c539fb5..cdab20f 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -123,6 +123,25 @@ out:
>  return ret;
>  }
>  
> +static bool nbd_has_filename_options_conflict(QDict *options, Error **errp)
> +{
> +const QDictEntry *e;
> +
> +for (e = qdict_first(options); e; e = qdict_next(options, e)) {
> +if (!strcmp(e->key, "host") ||
> +!strcmp(e->key, "port") ||
> +!strcmp(e->key, "path") ||
> +!strcmp(e->key, "export"))
> +{
> +error_setg(errp, "Option '%s' cannot be used with a file name",
> +   e->key);
> +return true;
> +}
> +}

Or put another way, while manual parsing looks fine, it's even better if
we can avoid manual parsing (and having to remember to update it when
the schema changes) by letting the schema itself automatically reject
invalid option pairs, even if the automatic rejection doesn't give quite
as nice of an error message.

> +
> +return false;
> +}
> +
>  static void nbd_parse_filename(const char *filename, QDict *options,
> Error **errp)
>  {
> @@ -131,12 +150,7 @@ static void nbd_parse_filename(const char *filename, 
> QDict *options,
>  const char *host_spec;
>  const char *unixpath;
>  
> -if (qdict_haskey(options, "host")
> -|| qdict_haskey(options, "port")
> -|| qdict_haskey(options, "path"))
> -{

Also, this patch looks like you are doing a bug fix mixed with code
motion - the old code manually checked for duplicates on only three
options, but the new code adds 'export' to the mix.

> -error_setg(errp, "host/port/path and a file name may not be 
> specified "
> - "at the same time");
> +if (nbd_has_filename_options_conflict(options, errp)) {
>  return;
>  }
>  
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [QEMU PATCH v5 0/6] migration: ensure hotplug and migration work together

[no-reply]

Hi,

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

Type: series
Message-id: 1475519097-27611-1-git-send-email-du...@linux.vnet.ibm.com
Subject: [Qemu-devel] [QEMU PATCH v5 0/6] migration: ensure hotplug and 
migration work together

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] 
patchew/1475519097-27611-1-git-send-email-du...@linux.vnet.ibm.com -> 
patchew/1475519097-27611-1-git-send-email-du...@linux.vnet.ibm.com
Switched to a new branch 'test'
d935e1f migration: spapr: migrate pending_events of spapr state
64aa229 migration: spapr: migrate ccs_list in spapr state
ecf2d6c migration: migrate QTAILQ
6f641b8 migration: extend VMStateInfo
803e704 migration: spapr_drc: defined VMStateDescription struct
08b0c64 migration: alternative way to set instance_id in SaveStateEntry

=== OUTPUT BEGIN ===
Checking PATCH 1/6: migration: alternative way to set instance_id in 
SaveStateEntry...
Checking PATCH 2/6: migration: spapr_drc: defined VMStateDescription struct...
ERROR: space required before the open parenthesis '('
#69: FILE: hw/ppc/spapr_drc.c:634:
+switch(drc->type) {

total: 1 errors, 0 warnings, 135 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/6: migration: extend VMStateInfo...
Checking PATCH 4/6: migration: migrate QTAILQ...
ERROR: spaces required around that '+' (ctx:WxV)
#108: FILE: include/qemu/queue.h:467:
+*((void **) ((char *) (head) +QTAILQ_LAST_OFFSET));
\
  ^

ERROR: spaces required around that '+' (ctx:WxV)
#109: FILE: include/qemu/queue.h:468:
+**((void ***)((char *) (head) +QTAILQ_LAST_OFFSET)) = (elm);   
\
   ^

total: 2 errors, 0 warnings, 184 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 5/6: migration: spapr: migrate ccs_list in spapr state...
ERROR: spaces required around that '*' (ctx:VxV)
#66: FILE: hw/ppc/spapr.c:1287:
+.subsections = (const VMStateDescription*[]) {
 ^

total: 1 errors, 0 warnings, 46 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 6/6: migration: spapr: migrate pending_events of spapr state...
ERROR: code indent should never use tabs
#96: FILE: hw/ppc/spapr_events.c:234:
+^I^I^I^I int data_size)$

total: 1 errors, 0 warnings, 159 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

[Qemu-devel] [QEMU PATCH v5 6/6] migration: spapr: migrate pending_events of spapr state

[Jianjun Duan]

In racing situations between hotplug events and migration operation,
a rtas hotplug event could have not yet be delivered to the source
guest when migration is started. In this case the pending_events of
spapr state need be transmitted to the target so that the hotplug
event can be finished on the target.

All the different fields of the events are encoded as defined by
PAPR. We can migrate them as uint8_t binary stream without any
concerns about data padding or endianess.

pending_events is put in a subsection in the spapr state VMSD to make
sure migration across different versions is not broken.

Signed-off-by: Jianjun Duan 
---
 hw/ppc/spapr.c | 33 +
 hw/ppc/spapr_events.c  | 22 +-
 include/hw/ppc/spapr.h |  3 ++-
 3 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 1847d35..5b57e5a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1255,12 +1255,32 @@ static bool version_before_3(void *opaque, int 
version_id)
 return version_id < 3;
 }
 
+static bool spapr_pending_events_needed(void *opaque)
+{
+sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
+return !QTAILQ_EMPTY(>pending_events);
+}
+
 static bool spapr_ccs_list_needed(void *opaque)
 {
 sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
 return !QTAILQ_EMPTY(>ccs_list);
 }
 
+static const VMStateDescription vmstate_spapr_event_entry = {
+.name = "spapreventlogentry",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_INT32(log_type, sPAPREventLogEntry),
+VMSTATE_BOOL(exception, sPAPREventLogEntry),
+VMSTATE_UINT32(data_size, sPAPREventLogEntry),
+VMSTATE_VARRAY_UINT32_ALLOC(data, sPAPREventLogEntry, data_size,
+0, vmstate_info_uint8, uint8_t),
+VMSTATE_END_OF_LIST()
+  },
+};
+
 static const VMStateDescription vmstate_spapr_ccs = {
 .name = "spaprconfigureconnectorstate",
 .version_id = 1,
@@ -1273,6 +1293,18 @@ static const VMStateDescription vmstate_spapr_ccs = {
 },
 };
 
+static const VMStateDescription vmstate_spapr_pending_events = {
+.name = "spaprpendingevents",
+.version_id = 1,
+.minimum_version_id = 1,
+.needed = spapr_pending_events_needed,
+.fields = (VMStateField[]) {
+VMSTATE_QTAILQ_V(pending_events, sPAPRMachineState, 1,
+ vmstate_spapr_event_entry, sPAPREventLogEntry, next),
+VMSTATE_END_OF_LIST()
+},
+};
+
 static const VMStateDescription vmstate_spapr_ccs_list = {
 .name = "spaprccslist",
 .version_id = 1,
@@ -1301,6 +1333,7 @@ static const VMStateDescription vmstate_spapr = {
 VMSTATE_END_OF_LIST()
 },
 .subsections = (const VMStateDescription*[]) {
+_spapr_pending_events,
 _spapr_ccs_list,
 NULL
 }
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 4c7b6ae..3f45744 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -230,7 +230,8 @@ void spapr_events_fdt_skel(void *fdt, uint32_t 
check_exception_irq)
 _FDT((fdt_end_node(fdt)));
 }
 
-static void rtas_event_log_queue(int log_type, void *data, bool exception)
+static void rtas_event_log_queue(int log_type, void *data, bool exception,
+int data_size)
 {
 sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
 sPAPREventLogEntry *entry = g_new(sPAPREventLogEntry, 1);
@@ -239,6 +240,7 @@ static void rtas_event_log_queue(int log_type, void *data, 
bool exception)
 entry->log_type = log_type;
 entry->exception = exception;
 entry->data = data;
+entry->data_size = data_size;
 QTAILQ_INSERT_TAIL(>pending_events, entry, next);
 }
 
@@ -341,6 +343,7 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
 struct rtas_event_log_v6_mainb *mainb;
 struct rtas_event_log_v6_epow *epow;
 struct epow_log_full *new_epow;
+uint32_t data_size;
 
 new_epow = g_malloc0(sizeof(*new_epow));
 hdr = _epow->hdr;
@@ -349,13 +352,13 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
 mainb = _epow->mainb;
 epow = _epow->epow;
 
+data_size = sizeof(*new_epow);
 hdr->summary = cpu_to_be32(RTAS_LOG_VERSION_6
| RTAS_LOG_SEVERITY_EVENT
| RTAS_LOG_DISPOSITION_NOT_RECOVERED
| RTAS_LOG_OPTIONAL_PART_PRESENT
| RTAS_LOG_TYPE_EPOW);
-hdr->extended_length = cpu_to_be32(sizeof(*new_epow)
-   - sizeof(new_epow->hdr));
+hdr->extended_length = cpu_to_be32(data_size - sizeof(new_epow->hdr));
 
 spapr_init_v6hdr(v6hdr);
 spapr_init_maina(maina, 3 /* Main-A, Main-B and EPOW */);
@@ -375,7 +378,7 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
 

Re: [Qemu-devel] [PATCH 3/3] target-arm: Comments to mark location of pending work for 56 bit addresses

[Tom Hanson]

On 09/30/2016 05:24 PM, Peter Maydell wrote:
> On 30 September 2016 at 15:46, Tom Hanson  wrote:
>> On 09/29/2016 07:27 PM, Peter Maydell wrote:
>> ...
 This work was not done at this time since the changes could not be tested
 with current CPU models.  Comments have been added to flag the locations
 where this will need to be fixed once a model is available.
>>>
>>> This is *not* why we haven't done this work. We haven't done it
>>> because the maximum virtual address size permitted by the
>>> architecture is less than 56 bits, and so this is a "can't happen"
>>> situation.
>>
>> But, in an earlier discussion which we had about the desire to use QEMU
>> to test potential new ARM-based architectures with large address spaces
>> I suggested that these changes be made now.  You said that the changes
>> shouldn't be made because:
>> where there is no supported guest CPU that could use
>> that code, the code shouldn't be there because it's untested
>> and untestable
>> Isn't that the same thing I said above?
> 
> That's a general statement of principle about what I think we
> should or shouldn't write code for in QEMU. In this particular case,
> it's true, but the reason it's true isn't just that we don't
> currently have any 56 bit-VA CPUs implemented, but because such
> a CPU is not permitted by the architecture. That's a stronger
> statement and I think it's worth making.
> 

Per the current spec (and v2) that's true.  But the intent was to enable 
testing of "new ARM-based architectures with large address spaces."  Vendors 
and OEMs may have difficulty in determining whether to ask for / push for / 
support a future, larger address space in the absence of a platform which is 
capable of emulating the future architecture.  

[Qemu-devel] [QEMU PATCH v5 5/6] migration: spapr: migrate ccs_list in spapr state

[Jianjun Duan]

ccs_list in spapr state maintains the device tree related
information on the rtas side for hotplugged devices. In racing
situations between hotplug events and migration operation, a rtas
hotplug event could be migrated from the source guest to target
guest, or the source guest could have not yet finished fetching
the device tree when migration is started, the target will try
to finish fetching the device tree. By migrating ccs_list, the
target can fetch the device tree properly.

ccs_list is put in a subsection in the spapr state VMSD to make
sure migration across different versions is not broken.

Signed-off-by: Jianjun Duan 
---
 hw/ppc/spapr.c | 34 ++
 1 file changed, 34 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 63b6a0d..1847d35 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1255,6 +1255,36 @@ static bool version_before_3(void *opaque, int 
version_id)
 return version_id < 3;
 }
 
+static bool spapr_ccs_list_needed(void *opaque)
+{
+sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
+return !QTAILQ_EMPTY(>ccs_list);
+}
+
+static const VMStateDescription vmstate_spapr_ccs = {
+.name = "spaprconfigureconnectorstate",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_UINT32(drc_index, sPAPRConfigureConnectorState),
+VMSTATE_INT32(fdt_offset, sPAPRConfigureConnectorState),
+VMSTATE_INT32(fdt_depth, sPAPRConfigureConnectorState),
+VMSTATE_END_OF_LIST()
+},
+};
+
+static const VMStateDescription vmstate_spapr_ccs_list = {
+.name = "spaprccslist",
+.version_id = 1,
+.minimum_version_id = 1,
+.needed = spapr_ccs_list_needed,
+.fields = (VMStateField[]) {
+VMSTATE_QTAILQ_V(ccs_list, sPAPRMachineState, 1,
+ vmstate_spapr_ccs, sPAPRConfigureConnectorState, 
next),
+VMSTATE_END_OF_LIST()
+},
+};
+
 static const VMStateDescription vmstate_spapr = {
 .name = "spapr",
 .version_id = 3,
@@ -1270,6 +1300,10 @@ static const VMStateDescription vmstate_spapr = {
 VMSTATE_PPC_TIMEBASE_V(tb, sPAPRMachineState, 2),
 VMSTATE_END_OF_LIST()
 },
+.subsections = (const VMStateDescription*[]) {
+_spapr_ccs_list,
+NULL
+}
 };
 
 static int htab_save_setup(QEMUFile *f, void *opaque)
-- 
1.9.1

[Qemu-devel] [QEMU PATCH v5 3/6] migration: extend VMStateInfo

[Jianjun Duan]

Current migration code cannot handle some data structures such as
QTAILQ in qemu/queue.h. Here we extend the signatures of put/get
in VMStateInfo so that customized handling is supported.

Signed-off-by: Jianjun Duan 
---
 hw/net/vmxnet3.c| 18 ++---
 hw/nvram/eeprom93xx.c   |  6 ++-
 hw/nvram/fw_cfg.c   |  6 ++-
 hw/pci/msix.c   |  6 ++-
 hw/pci/pci.c| 12 --
 hw/pci/shpc.c   |  5 ++-
 hw/scsi/scsi-bus.c  |  6 ++-
 hw/timer/twl92230.c |  6 ++-
 hw/usb/redirect.c   | 18 ++---
 hw/virtio/virtio-pci.c  |  6 ++-
 hw/virtio/virtio.c  |  6 ++-
 include/migration/vmstate.h | 10 +++--
 migration/savevm.c  |  5 ++-
 migration/vmstate.c | 95 -
 target-alpha/machine.c  |  5 ++-
 target-arm/machine.c| 12 --
 target-i386/machine.c   | 21 ++
 target-mips/machine.c   | 10 +++--
 target-ppc/machine.c| 10 +++--
 target-sparc/machine.c  |  5 ++-
 20 files changed, 171 insertions(+), 97 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 90f6943..943a960 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2450,7 +2450,8 @@ static void vmxnet3_put_tx_stats_to_file(QEMUFile *f,
 qemu_put_be64(f, tx_stat->pktsTxDiscard);
 }
 
-static int vmxnet3_get_txq_descr(QEMUFile *f, void *pv, size_t size)
+static int vmxnet3_get_txq_descr(QEMUFile *f, void *pv, size_t size,
+VMStateField *field)
 {
 Vmxnet3TxqDescr *r = pv;
 
@@ -2464,7 +2465,8 @@ static int vmxnet3_get_txq_descr(QEMUFile *f, void *pv, 
size_t size)
 return 0;
 }
 
-static void vmxnet3_put_txq_descr(QEMUFile *f, void *pv, size_t size)
+static void vmxnet3_put_txq_descr(QEMUFile *f, void *pv, size_t size,
+VMStateField *field, QJSON *vmdesc)
 {
 Vmxnet3TxqDescr *r = pv;
 
@@ -2511,7 +2513,8 @@ static void vmxnet3_put_rx_stats_to_file(QEMUFile *f,
 qemu_put_be64(f, rx_stat->pktsRxError);
 }
 
-static int vmxnet3_get_rxq_descr(QEMUFile *f, void *pv, size_t size)
+static int vmxnet3_get_rxq_descr(QEMUFile *f, void *pv, size_t size,
+VMStateField *field)
 {
 Vmxnet3RxqDescr *r = pv;
 int i;
@@ -2529,7 +2532,8 @@ static int vmxnet3_get_rxq_descr(QEMUFile *f, void *pv, 
size_t size)
 return 0;
 }
 
-static void vmxnet3_put_rxq_descr(QEMUFile *f, void *pv, size_t size)
+static void vmxnet3_put_rxq_descr(QEMUFile *f, void *pv, size_t size,
+VMStateField *field, QJSON *vmdesc)
 {
 Vmxnet3RxqDescr *r = pv;
 int i;
@@ -2574,7 +2578,8 @@ static const VMStateInfo rxq_descr_info = {
 .put = vmxnet3_put_rxq_descr
 };
 
-static int vmxnet3_get_int_state(QEMUFile *f, void *pv, size_t size)
+static int vmxnet3_get_int_state(QEMUFile *f, void *pv, size_t size,
+VMStateField *field)
 {
 Vmxnet3IntState *r = pv;
 
@@ -2585,7 +2590,8 @@ static int vmxnet3_get_int_state(QEMUFile *f, void *pv, 
size_t size)
 return 0;
 }
 
-static void vmxnet3_put_int_state(QEMUFile *f, void *pv, size_t size)
+static void vmxnet3_put_int_state(QEMUFile *f, void *pv, size_t size,
+VMStateField *field, QJSON *vmdesc)
 {
 Vmxnet3IntState *r = pv;
 
diff --git a/hw/nvram/eeprom93xx.c b/hw/nvram/eeprom93xx.c
index 2c16fc2..76d5f41 100644
--- a/hw/nvram/eeprom93xx.c
+++ b/hw/nvram/eeprom93xx.c
@@ -94,14 +94,16 @@ struct _eeprom_t {
This is a Big hack, but it is how the old state did it.
  */
 
-static int get_uint16_from_uint8(QEMUFile *f, void *pv, size_t size)
+static int get_uint16_from_uint8(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
 {
 uint16_t *v = pv;
 *v = qemu_get_ubyte(f);
 return 0;
 }
 
-static void put_unused(QEMUFile *f, void *pv, size_t size)
+static void put_unused(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+   QJSON *vmdesc)
 {
 fprintf(stderr, "uint16_from_uint8 is used only for backwards 
compatibility.\n");
 fprintf(stderr, "Never should be used to write a new state.\n");
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 92aa563..a8a4a7a 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -524,14 +524,16 @@ static void fw_cfg_reset(DeviceState *d)
Or we broke compatibility in the state, or we can't use struct tm
  */
 
-static int get_uint32_as_uint16(QEMUFile *f, void *pv, size_t size)
+static int get_uint32_as_uint16(QEMUFile *f, void *pv, size_t size,
+VMStateField *field)
 {
 uint32_t *v = pv;
 *v = qemu_get_be16(f);
 return 0;
 }
 
-static void put_unused(QEMUFile *f, void *pv, size_t size)
+static void put_unused(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+   QJSON *vmdesc)
 {
 fprintf(stderr, "uint32_as_uint16 is only used for backward 
compatibility.\n");
 fprintf(stderr, "This functions shouldn't be called.\n");
diff --git a/hw/pci/msix.c 

[Qemu-devel] [QEMU PATCH v5 1/6] migration: alternative way to set instance_id in SaveStateEntry

[Jianjun Duan]

In QOM(QEMU Object Model) migrated objects are identified with instance_id
which is calculated automatically using their path in the QOM composition
tree. For some objects, this path could change from source to target in
migration. To migrate such objects, we need to make sure the instance_id does
not change from source to target. We add a hook in DeviceClass to do customized
instance_id calculation in such cases.

As a result, in these cases compat will not be set in the concerned
SaveStateEntry. This will prevent the inconsistent idstr to be sent over in
migration. We could have set alias_id in a similar way. But that will be
overloading the purpose of alias_id.

The first application will be setting instance_id for DRC using its unique
index. Doing this makes the instance_id of DRC to be consistent across migration
and supports flexible management of DRC objects in migration.

Signed-off-by: Jianjun Duan 
---
 include/hw/qdev-core.h |  6 ++
 migration/savevm.c | 20 ++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 2c97347..a012e8e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -139,6 +139,12 @@ typedef struct DeviceClass {
 qdev_initfn init; /* TODO remove, once users are converted to realize */
 qdev_event exit; /* TODO remove, once users are converted to unrealize */
 const char *bus_type;
+
+/* When this field is set, qemu will use it to get an unique instance_id
+ * instead of calculating an auto idstr and instanc_id for the relevant
+ * SaveStateEntry
+ */
+int (*dev_get_instance_id)(DeviceState *dev);
 } DeviceClass;
 
 typedef struct NamedGPIOList NamedGPIOList;
diff --git a/migration/savevm.c b/migration/savevm.c
index 33a2911..ef5c3d1 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -495,6 +495,11 @@ int register_savevm_live(DeviceState *dev,
  void *opaque)
 {
 SaveStateEntry *se;
+/* when it is a device and it provides a way to get instance_id,
+ * we will use it and skip setting idstr and compat.
+ */
+bool flag = (dev != NULL) &&
+(DEVICE_GET_CLASS(dev)->dev_get_instance_id != NULL);
 
 se = g_new0(SaveStateEntry, 1);
 se->version_id = version_id;
@@ -507,7 +512,7 @@ int register_savevm_live(DeviceState *dev,
 se->is_ram = 1;
 }
 
-if (dev) {
+if (dev && !flag) {
 char *id = qdev_get_dev_path(dev);
 if (id) {
 pstrcpy(se->idstr, sizeof(se->idstr), id);
@@ -523,6 +528,9 @@ int register_savevm_live(DeviceState *dev,
 }
 pstrcat(se->idstr, sizeof(se->idstr), idstr);
 
+if (flag) {
+instance_id = DEVICE_GET_CLASS(dev)->dev_get_instance_id(dev);
+}
 if (instance_id == -1) {
 se->instance_id = calculate_new_instance_id(se->idstr);
 } else {
@@ -580,6 +588,11 @@ int vmstate_register_with_alias_id(DeviceState *dev, int 
instance_id,
int required_for_version)
 {
 SaveStateEntry *se;
+/* when it is a device and it provides a way to get instance_id,
+ * we will use it and skip setting idstr and compat.
+ */
+bool flag = (dev != NULL) &&
+(DEVICE_GET_CLASS(dev)->dev_get_instance_id != NULL);
 
 /* If this triggers, alias support can be dropped for the vmsd. */
 assert(alias_id == -1 || required_for_version >= vmsd->minimum_version_id);
@@ -591,7 +604,7 @@ int vmstate_register_with_alias_id(DeviceState *dev, int 
instance_id,
 se->vmsd = vmsd;
 se->alias_id = alias_id;
 
-if (dev) {
+if (dev && !flag) {
 char *id = qdev_get_dev_path(dev);
 if (id) {
 pstrcpy(se->idstr, sizeof(se->idstr), id);
@@ -607,6 +620,9 @@ int vmstate_register_with_alias_id(DeviceState *dev, int 
instance_id,
 }
 pstrcat(se->idstr, sizeof(se->idstr), vmsd->name);
 
+if (flag) {
+instance_id = DEVICE_GET_CLASS(dev)->dev_get_instance_id(dev);
+}
 if (instance_id == -1) {
 se->instance_id = calculate_new_instance_id(se->idstr);
 } else {
-- 
1.9.1

[Qemu-devel] [QEMU PATCH v5 0/6] migration: ensure hotplug and migration work together

[Jianjun Duan]

Hi all,
   The previous patches seem to get buried deep somewhere. I am sending the 
lated rebased version. Comments are welcome.

v5: - Rebased to David's ppc-for-2.8. Previous versions are:

v4: - Introduce a way to set customized instance_id in SaveStateEntry. Use it
  to set instance_id for DRC using its unique index to address David 
  Gibson's concern.
- Rename VMS_CSTM to VMS_LINKED based on Paolo Bonzini's suggestions.
- Clean up qjson stuff in put_qtailq. 
- Add trace for put_qtailq and get_qtailq based on David Gilbert's 
  suggestion.

- Based on David's ppc-for-2.7. 

v3: - Simplify overall design followng discussion with Paolo. No longer need
  metadata to migrate QTAILQ.
- Extend VMStateInfo instead of adding similar fields to VMStateField.
- Clean up macros in qemu/queue.h.
(link: https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg05695.html)

v2: - Introduce a general approach to migrate QTAILQ in qemu/queue.h.
- Migrate signalled field in the DRC state.
- Put the newly added migrating fields in subsections so that backward 
  migration is not broken.  
- Set detach_cb field right after migration so that a migrated hot-unplug
  event could finish its course.
(link: https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg04188.html)

v1: - Inital version.
(link: https://lists.nongnu.org/archive/html/qemu-devel/2016-04/msg02601.html)

To make guest device (PCI, CPU and memory) hotplug work together 
with guest migration, spapr drc state needs be transmitted in
migration. This patch defines the VMStateDescription struct for
spapr drc state to enable it.

To fix the potential racing between hotplug events on guest and 
guest migration, ccs_list and pending_events of spapr state need be 
transmitted in migration. This patch also takes care of it.

Jianjun Duan (6):
  migration: alternative way to set instance_id in SaveStateEntry
  migration: spapr_drc: defined VMStateDescription struct
  migration: extend VMStateInfo
  migration: migrate QTAILQ
  migration: spapr: migrate ccs_list in spapr state
  migration: spapr: migrate pending_events of spapr state

 hw/net/vmxnet3.c|  18 +++--
 hw/nvram/eeprom93xx.c   |   6 +-
 hw/nvram/fw_cfg.c   |   6 +-
 hw/pci/msix.c   |   6 +-
 hw/pci/pci.c|  12 ++--
 hw/pci/shpc.c   |   5 +-
 hw/ppc/spapr.c  |  67 ++
 hw/ppc/spapr_drc.c  |  69 +++
 hw/ppc/spapr_events.c   |  22 +++---
 hw/ppc/spapr_pci.c  |  22 ++
 hw/scsi/scsi-bus.c  |   6 +-
 hw/timer/twl92230.c |   6 +-
 hw/usb/redirect.c   |  18 +++--
 hw/virtio/virtio-pci.c  |   6 +-
 hw/virtio/virtio.c  |   6 +-
 include/hw/ppc/spapr.h  |   3 +-
 include/hw/ppc/spapr_drc.h  |   9 +++
 include/hw/qdev-core.h  |   6 ++
 include/migration/vmstate.h |  36 --
 include/qemu/queue.h|  32 +
 migration/savevm.c  |  25 +--
 migration/trace-events  |   4 ++
 migration/vmstate.c | 161 ++--
 target-alpha/machine.c  |   5 +-
 target-arm/machine.c|  12 ++--
 target-i386/machine.c   |  21 --
 target-mips/machine.c   |  10 +--
 target-ppc/machine.c|  10 +--
 target-sparc/machine.c  |   5 +-
 29 files changed, 505 insertions(+), 109 deletions(-)

-- 
1.9.1

[Qemu-devel] [QEMU PATCH v5 2/6] migration: spapr_drc: defined VMStateDescription struct

[Jianjun Duan]

To manage hotplug/unplug of dynamic resources such as PCI cards,
memory, and CPU on sPAPR guests, a firmware abstraction known as
a Dynamic Resource Connector (DRC) is used to assign a particular
dynamic resource to the guest, and provide an interface for the
guest to manage configuration/removal of the resource associated
with it.

To migrate the hotplugged resources in migration, the
associated DRC state need be migrated. To migrate the DRC state,
we defined the VMStateDescription struct for spapr_drc to enable
the transmission of spapr_drc state in migration.

Not all the elements in the DRC state are migrated. Only those
ones modifiable or needed by guest actions or device add/remove
operation are migrated. From the perspective of device
hotplugging, if we hotplug a device on the source, we need to
"coldplug" it on the target. The states across two hosts for the
same device are not the same. Ideally we want the states be same
after migration so that the device would function as hotplugged
on the target. For example we can unplug it. The minimum DRC
state we need to transfer should cover all the pieces changed by
hotplugging. Out of the elements of the DRC state, isolation_state,
allocation_sate, and configured are involved in the DR state
transition diagram from PAPR+ 2.7, 13.4. configured and signalled
are needed in attaching and detaching devices. indicator_state
provides users with hardware state information. These 6 elements
are migrated.

detach_cb in the DRC state is a function pointer that cannot be
migrated. We set it right after DRC state is migrated so that
a migrated hot-unplug event could finish its work.

The instance_id is used to identify objects in migration. We set
instance_id of DRC using the unique index so that it is the same
across migration.

Signed-off-by: Jianjun Duan 
---
 hw/ppc/spapr_drc.c | 69 ++
 hw/ppc/spapr_pci.c | 22 +++
 include/hw/ppc/spapr_drc.h |  9 ++
 3 files changed, 100 insertions(+)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 6e54fd4..369ec02 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -615,6 +615,71 @@ static void spapr_dr_connector_instance_init(Object *obj)
 NULL, NULL, NULL, NULL);
 }
 
+static bool spapr_drc_needed(void *opaque)
+{
+sPAPRDRConnector *drc = (sPAPRDRConnector *)opaque;
+sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+bool rc = false;
+sPAPRDREntitySense value;
+
+drck->entity_sense(drc, );
+/* If no dev is plugged in there is no need to migrate the DRC state */
+if (value != SPAPR_DR_ENTITY_SENSE_PRESENT) {
+return false;
+}
+/*
+ * If there is dev plugged in, we need to migrate the DRC state when
+ * it is different from cold-plugged state
+ */
+switch(drc->type) {
+/* for PCI type */
+case SPAPR_DR_CONNECTOR_TYPE_PCI:
+rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) &&
+   (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) &&
+   drc->configured && drc->signalled && !drc->awaiting_release);
+break;
+/* for LMB type */
+case SPAPR_DR_CONNECTOR_TYPE_LMB:
+rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
+   (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) &&
+   drc->configured && drc->signalled && !drc->awaiting_release);
+break;
+default:
+;
+}
+
+return rc;
+}
+
+/* detach_cb needs be set since it is not migrated */
+static void postmigrate_set_detach_cb(sPAPRDRConnector *drc,
+  spapr_drc_detach_cb *detach_cb)
+{
+drc->detach_cb = detach_cb;
+}
+
+/* return the unique drc index as instance_id for qom interfaces*/
+static int get_instance_id(DeviceState *dev)
+{
+return (int)get_index(SPAPR_DR_CONNECTOR(OBJECT(dev)));
+}
+
+static const VMStateDescription vmstate_spapr_drc = {
+.name = "spapr_drc",
+.version_id = 1,
+.minimum_version_id = 1,
+.needed = spapr_drc_needed,
+.fields  = (VMStateField []) {
+VMSTATE_UINT32(isolation_state, sPAPRDRConnector),
+VMSTATE_UINT32(allocation_state, sPAPRDRConnector),
+VMSTATE_UINT32(indicator_state, sPAPRDRConnector),
+VMSTATE_BOOL(configured, sPAPRDRConnector),
+VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
+VMSTATE_BOOL(signalled, sPAPRDRConnector),
+VMSTATE_END_OF_LIST()
+}
+};
+
 static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
 {
 DeviceClass *dk = DEVICE_CLASS(k);
@@ -623,6 +688,8 @@ static void spapr_dr_connector_class_init(ObjectClass *k, 
void *data)
 dk->reset = reset;
 dk->realize = realize;
 dk->unrealize = unrealize;
+dk->vmsd = _spapr_drc;
+dk->dev_get_instance_id = get_instance_id;
 

[Qemu-devel] [QEMU PATCH v5 4/6] migration: migrate QTAILQ

[Jianjun Duan]

Currently we cannot directly transfer a QTAILQ instance because of the
limitation in the migration code. Here we introduce an approach to
transfer such structures. In our approach such a structure is tagged
with VMS_LINKED. We then modified vmstate_save_state and vmstate_load_state
so that when VMS_LINKED is encountered, put and get from VMStateInfo are
called respectively. We created VMStateInfo vmstate_info_qtailq for QTAILQ.
Similar VMStateInfo can be created for other data structures such as list.
This approach will be used to transfer pending_events and ccs_list in spapr
state.

We also create some macros in qemu/queue.h to access a QTAILQ using pointer
arithmetic. This ensures that we do not depend on the implementation
details about QTAILQ in the migration code.

Signed-off-by: Jianjun Duan 
---
 include/migration/vmstate.h | 26 ++
 include/qemu/queue.h| 32 ++
 migration/trace-events  |  4 +++
 migration/vmstate.c | 66 +
 4 files changed, 128 insertions(+)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 459dd4a..e60c994 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -186,6 +186,12 @@ enum VMStateFlags {
  * to determine the number of entries in the array. Only valid in
  * combination with one of VMS_VARRAY*. */
 VMS_MULTIPLY_ELEMENTS = 0x4000,
+/* For fields which need customized handling, such as QTAILQ in queue.h.
+ * When this flag is set in VMStateField, info->get/put will
+ * be used in vmstate_load/save_state instead of recursive call.
+ * User should implement set info to handle the concerned data structure.
+ */
+VMS_LINKED= 0x8000,
 };
 
 struct VMStateField {
@@ -246,6 +252,7 @@ extern const VMStateInfo vmstate_info_timer;
 extern const VMStateInfo vmstate_info_buffer;
 extern const VMStateInfo vmstate_info_unused_buffer;
 extern const VMStateInfo vmstate_info_bitmap;
+extern const VMStateInfo vmstate_info_qtailq;
 
 #define type_check_2darray(t1,t2,n,m) ((t1(*)[n][m])0 - (t2*)0)
 #define type_check_array(t1,t2,n) ((t1(*)[n])0 - (t2*)0)
@@ -657,6 +664,25 @@ extern const VMStateInfo vmstate_info_bitmap;
 .offset   = offsetof(_state, _field),\
 }
 
+/* For QTAILQ that need customized handling
+ * _type: type of QTAILQ element
+ * _next: name of QTAILQ entry field in QTAILQ element
+ * _vmsd: VMSD for QTAILQ element
+ * size: size of QTAILQ element
+ * start: offset of QTAILQ entry in QTAILQ element
+ */
+#define VMSTATE_QTAILQ_V(_field, _state, _version, _vmsd, _type, _next)  \
+{\
+.name = (stringify(_field)), \
+.version_id   = (_version),  \
+.vmsd = &(_vmsd),\
+.size = sizeof(_type),   \
+.info = _info_qtailq,\
+.flags= VMS_LINKED,  \
+.offset   = offsetof(_state, _field),\
+.start= offsetof(_type, _next),  \
+}
+
 /* _f : field name
_f_n : num of elements field_name
_n : num of elements
diff --git a/include/qemu/queue.h b/include/qemu/queue.h
index 342073f..12c3f80 100644
--- a/include/qemu/queue.h
+++ b/include/qemu/queue.h
@@ -438,4 +438,36 @@ struct {   
 \
 #define QTAILQ_PREV(elm, headname, field) \
 (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last))
 
+/*
+ * Offsets of layout of a tail queue head.
+ */
+#define QTAILQ_FIRST_OFFSET 0
+#define QTAILQ_LAST_OFFSET (sizeof(void *))
+
+/*
+ * Offsets of layout of a tail queue element.
+ */
+#define QTAILQ_NEXT_OFFSET 0
+#define QTAILQ_PREV_OFFSET (sizeof(void *))
+
+/*
+ * Tail queue tranversal using pointer arithmetic.
+ */
+#define QTAILQ_RAW_FOREACH(elm, head, entry)   
\
+for ((elm) = *((void **) ((char *) (head) + QTAILQ_FIRST_OFFSET)); 
\
+ (elm);
\
+ (elm) =   
\
+ *((void **) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET)))
+/*
+ * Tail queue insertion using pointer arithmetic.
+ */
+#define QTAILQ_RAW_INSERT_TAIL(head, elm, entry) do {  
\
+*((void **) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET)) = NULL;   
\
+*((void **) ((char *) (elm) + (entry) + QTAILQ_PREV_OFFSET)) = 
\
+*((void **) ((char *) (head) +QTAILQ_LAST_OFFSET));
\
+**((void ***)((char *) (head) 

Re: [Qemu-devel] backup notifier fail policy

[John Snow]

On 10/03/2016 09:11 AM, Stefan Hajnoczi wrote:

On Fri, Sep 30, 2016 at 09:59:16PM +0300, Vladimir Sementsov-Ogievskiy wrote:

On 30.09.2016 20:11, Vladimir Sementsov-Ogievskiy wrote:

Hi all!

Please, can somebody explain me, why we fail guest request in case of io
error in write notifier? I think guest consistency is more important
than success of unfinished backup. Or, what am I missing?

I'm saying about this code:

static int coroutine_fn backup_before_write_notify(
NotifierWithReturn *notifier,
void *opaque)
{
BackupBlockJob *job = container_of(notifier, BackupBlockJob,
before_write);
BdrvTrackedRequest *req = opaque;
int64_t sector_num = req->offset >> BDRV_SECTOR_BITS;
int nb_sectors = req->bytes >> BDRV_SECTOR_BITS;

assert(req->bs == blk_bs(job->common.blk));
assert((req->offset & (BDRV_SECTOR_SIZE - 1)) == 0);
assert((req->bytes & (BDRV_SECTOR_SIZE - 1)) == 0);

return backup_do_cow(job, sector_num, nb_sectors, NULL, true);
}

So, what about something like

ret = backup_do_cow(job, ...
if (ret < 0 && job->notif_ret == 0) {
   job->notif_ret = ret;
}

return 0;

and fail block job if notif_ret < 0 in other places of backup code?



And second question about notifiers in backup block job. If block job is
paused, notifiers still works and can copy data. Is it ok? So, user thinks
that job is paused, so he can do something with target disk.. But really,
this 'something' will race with write-notifiers. So, what assumptions may
user actually have about paused backup job? Is there any agreements? Also,
on query-block-jobs we will see job.busy = false, when actually
copy-on-write may be in flight..


I agree that the job should fail and the guest continues running.

The backup job cannot do the usual ENOSPC stop/resume error handling
since we lose snapshot consistency once guest writes are allowed to
proceed.  Backup errors need to be fatal, resuming is usually not
possible.  The user will have to retry the backup operation.

Stefan



If we fail and intercept the error for the backup write and HALT at that 
point, why would we lose consistency? If the backup write failed before 
we allowed the guest write to proceed, that data should still be there 
on disk, no?


I guess it is a little messier than the usual STOP case, but it doesn't 
seem inherently impossible to me...


Eh, regardless: If we're not using a STOP policy, it seems like the 
right thing to do is definitely to just fail the backup instead of 
failing the write.


As for paused guarantees... good point. If you want to truly pause a 
backup job, I think you necessarily begin accruing a backlog of data 
that needs to get written back out. Maybe it's not easily possible to 
truly pause a backup block job.


I'm not exactly sure what we should do about it, though I do know that 
eventually we want to replace write notifiers with block filters, but 
even those would likely remain operating during a pause.


'busy' means something very specific within QEMU, but perhaps the query 
function can be adjusted to return 'true' for busy as long as either the 
job is running OR it has latent portions still running (write notifiers, 
block filters, etc.)


--js

[Qemu-devel] VNC - broken initial resolution

[Gerhard Wiesinger]

Hello,

Since the last update (git log 3b71ec8..c5d128f) it looks like that the 
inital resolution via VNC is broken.


E.g. after connecting seabios screen is not the typical text resolution 
but a lot larger than it should be.


Any ideas?

Anyone can reproduce it?

Thnx.


Ciao,

Gerhard


-- https://www.wiesinger.com/

Re: [Qemu-devel] [Bug 1626972] Re: [PATCH] util: secure memfd_create fallback mechanism

[Daniel P. Berrange]

On Mon, Oct 03, 2016 at 03:41:10PM -, Rafael David Tinoco wrote:
> Sorry, I was only able to come back to this today.
> 
> > On Sep 27, 2016, at 09:18, Daniel Berrange <1626...@bugs.launchpad.net> 
> > wrote:
> > 
> >> There are numerous people relying on older kernels in openstack 
> >> deployments - sometimes with specific drivers (ovswitch, dpdk, 
> >> infiniband) holding kernel upgrades - but still in need of upgrading 
> >> userland (e.g. newer releases). Having a fallback mechanism seems 
> >> appropriate for those cases.
> > 
> > I'm not against some kind of fallback - just about the way it
> > silently creates files in /tmp.
> > 
> 
> That is why memfd_create is used here I suppose: To allow anonymous-
> backed-pages to have a descriptor and to be sealed. When falling back
> this mechanism I don't see any other way other than creating a temporary
> file. Of course one way would be something like:
> 
> http://paste.ubuntu.com/23270379/
> 
> But this is pretty much the same, just solving the "where to place the
> temporary file" (non configurable for this usage).
> 
> >> 
> >> Note that the filename, per se, is not as important as other files, 
> >> since qemu won't provide it for being accessed by external programs, and,
> >> deletes the file, while keeping the descriptor, right after its creation
> >> (due to its nature, that is probably why it was created in /tmp).
> > 
> > If it doesn't shared with other processes, and is deleted immediately,
> > why does the file need to be on disk at all ?
> 
> Well, it unlinks the file but the references are still there while the
> descriptor isn't closed by this process, or by the one that receives the
> descriptor (that is why is the "unlink" so early).
> 
> If you check vhost_dev_log_resize(), it gets *possible* new vhost log
> (if a new size is given) and informs the vhost dev driver about the new
> log base (vhost_ops->vhost_set_log_base).
> 
> For vhost_user, this means that the file descriptors for vhost logs are
> likely going to be passed to vhost backend (fds[] in
> vhost_user_set_log_base). This is just one example, not sure about
> others.
>
> Probably the best approach here, like what Marc-André said, is to create
> some sort of TMPDIR, set by libvirt perhaps ?

So you're saying that the file descriptor here is actually getting
passed to a different process for it to use ?

If so that means we definitely do not want this in TMPDIR. If we
create a generic file in TMPDIR, then its going to have a generic
security label. That means that the other process we're giving the
FD to is going to have to be granted permission to access this FD
and we certainly don't want to grant permission for it to access
any of QEMU's other FDs. So for the SELinux integration, we'll
need this FD to be in a specific directory, so that we can setup
policy such that the file created gets given a specific SELinux
label. We can then grant the other process access to only that
particular file, and not anything else of QEMU's.

This makes me wonder about the memfd_create() code path too - we'll
again not want that external process to be granted access to arbitrary
FDs of QEMU's and I'm not sure of a way to get the memfd  FD to have
a specific label. So I think it is possible that when using libvirt
we'll want the ability to tell QEMU to *always* use an explicit file
in a path libvirt specifies, and never use memfd even if available.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

[Qemu-devel] Aspirant for Outreachy 2016 December- Marc

[Shreya Shrivastava]

HI ,

I am interested in applying for Outreachy 2016 December- March round by
contributing to  VIRTIO 1.0 support in libqos project for Qemu.

Kindly let me know how to get started with this project.

Shreya Shrivastava

[Qemu-devel] Getting Started with Outreachy (Was: (no subject))

[John Snow]

Please actually write a subject!

On 10/02/2016 02:47 AM, Shreya Shrivastava wrote:

HI ,

I am interested in applying for Outreachy 2016 December- March round by
contributing to  VIRTIO 1.0 support in libqos project for Qemu.

Kindly let me know how to get started with this project.

Shreya Shrivastava




Hi Shreya,

As far as I understand it, the Outreachy project either has or had a 
requirement that you contribute at least one small patch as part of the 
application process.


We've got a list of tiny things people can do called Bite Sized Tasks: 
http://qemu-project.org/BiteSizedTasks These are often good first stops 
for Outreachy/GSoC candidates.


Some of those ideas may have been implemented already or are at least 
"in progress," so we usually recommend people take a look at the devel 
archives and make sure nobody else is working on them: 
https://lists.nongnu.org/archive/html/qemu-devel/


From there, take a look at our previous Outreachy project list: 
http://wiki.qemu.org/Outreachy_2015_MayAugust Some of the projects 
listed here may have not been attempted and may still be available for 
future applicants if the mentors are still willing.


You are also always welcome to dream up your own ideas and propose them.

Stefan may have additional information more relevant to this Outreachy 
"season."


Good luck,
--js

Re: [Qemu-devel] [PATCH] util: secure memfd_create fallback mechanism

[Rafael David Tinoco]

Hello Marc, 

> On Sep 27, 2016, at 08:13, Marc-André Lureau  wrote:
> 
>>> On Tue, Sep 27, 2016 at 03:06:21AM +, Rafael David Tinoco wrote:
>>> We should not have QEMU creating unpredictabile filenames in the
>>> first place - any filenames should be determined by libvirt
>>> explicitly.
>> 
>> Note that the filename, per se, is not as important as other files,
>> since qemu won't provide it for being accessed by external programs, and,
>> deletes the file, while keeping the descriptor, right after its creation
>> (due to its nature, that is probably why it was created in /tmp).
>> 
>> Having libvirt to define a filename that would not be used for recent
>> kernels (> 3.17) and would exist for a fraction of second doesn't seem
>> right to me.
>> 
> 
> There are other parts of qemu that rely on creating temporary files, and this 
> seems to lack a bit of uniformity. Would it make sense to define a place 
> where qemu could create those? Or setting TMPDIR should help too. Could 
> libvirt set a per-vm TMPDIR with appropriate security rules?

Best move I can see. Only problem is that if we do that, we would have to 
create a fallback mechanism for when TMPDIR is not set. It would go back to 
/tmp ? 

In my particular case (for 1 vhost log file):

-netdev tap,fd=26,id=hostnet0,vhost=on,vhostfd=28 -device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=fa:16:3e:5c:10:f2,bus=pci.0,addr=0x3

I could have something similar to:

-netdev tap,fd=26,id=hostnet0,vhost=on,vhostfd=28 -device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=fa:16:3e:5c:10:f2,bus=pci.0,addr=0x3,vhostpath=/var/lib///
 

and put mkstemp() files (one per vhost device) in there. 

Even so, what to do when "vhostpath" is not informed ? 

I'm worried that, right now there are security drivers either blocking the live 
migration entirely or allowing all instances to be able to read 
/tmp/memfd-. 

Don't you think we could push the first patch until we come up with a better 
approach for the tmp (and default tmp) files & directories ? The patch is not 
worse than what was committed already. 

Tks

Rafael

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 1/6] libqos: add PPC64 PCI support

[Cédric Le Goater]

On 10/03/2016 04:03 PM, Greg Kurz wrote:
> On Mon, 3 Oct 2016 13:23:27 +0200
> Cédric Le Goater  wrote:
> 
>> On 09/29/2016 07:27 AM, David Gibson wrote:
>>> On Wed, Sep 28, 2016 at 08:51:28PM +0200, Laurent Vivier wrote:  
 Signed-off-by: Laurent Vivier 
 ---
  tests/Makefile.include   |   1 +
  tests/libqos/pci-pc.c|  22 
  tests/libqos/pci-spapr.c | 280 
 +++
  tests/libqos/pci-spapr.h |  17 +++
  tests/libqos/pci.c   |  22 +++-
  tests/libqos/rtas.c  |  45 
  tests/libqos/rtas.h  |   4 +
  7 files changed, 368 insertions(+), 23 deletions(-)
  create mode 100644 tests/libqos/pci-spapr.c
  create mode 100644 tests/libqos/pci-spapr.h

 diff --git a/tests/Makefile.include b/tests/Makefile.include
 index 8162f6f..92c82d8 100644
 --- a/tests/Makefile.include
 +++ b/tests/Makefile.include
 @@ -590,6 +590,7 @@ libqos-obj-y += tests/libqos/i2c.o 
 tests/libqos/libqos.o
  libqos-spapr-obj-y = $(libqos-obj-y) tests/libqos/malloc-spapr.o
  libqos-spapr-obj-y += tests/libqos/libqos-spapr.o
  libqos-spapr-obj-y += tests/libqos/rtas.o
 +libqos-spapr-obj-y += tests/libqos/pci-spapr.o
  libqos-pc-obj-y = $(libqos-obj-y) tests/libqos/pci-pc.o
  libqos-pc-obj-y += tests/libqos/malloc-pc.o tests/libqos/libqos-pc.o
  libqos-pc-obj-y += tests/libqos/ahci.o
 diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
 index 1ae2d37..82066b8 100644
 --- a/tests/libqos/pci-pc.c
 +++ b/tests/libqos/pci-pc.c
 @@ -255,28 +255,6 @@ void qpci_free_pc(QPCIBus *bus)
  g_free(s);
  }
  
 -void qpci_plug_device_test(const char *driver, const char *id,
 -   uint8_t slot, const char *opts)
 -{
 -QDict *response;
 -char *cmd;
 -
 -cmd = g_strdup_printf("{'execute': 'device_add',"
 -  " 'arguments': {"
 -  "   'driver': '%s',"
 -  "   'addr': '%d',"
 -  "   %s%s"
 -  "   'id': '%s'"
 -  "}}", driver, slot,
 -  opts ? opts : "", opts ? "," : "",
 -  id);
 -response = qmp(cmd);
 -g_free(cmd);
 -g_assert(response);
 -g_assert(!qdict_haskey(response, "error"));
 -QDECREF(response);
 -}
 -
  void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
  {
  QDict *response;
 diff --git a/tests/libqos/pci-spapr.c b/tests/libqos/pci-spapr.c
 new file mode 100644
 index 000..78df823
 --- /dev/null
 +++ b/tests/libqos/pci-spapr.c
 @@ -0,0 +1,280 @@
 +/*
 + * libqos PCI bindings for SPAPR
 + *
 + * 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 "libqtest.h"
 +#include "libqos/pci-spapr.h"
 +#include "libqos/rtas.h"
 +
 +#include "hw/pci/pci_regs.h"
 +
 +#include "qemu-common.h"
 +#include "qemu/host-utils.h"
 +
 +
 +/* From include/hw/pci-host/spapr.h */
 +
 +#define SPAPR_PCI_BASE_BUID  0x8002000ULL
 +
 +#define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x8000ULL
 +
 +#define SPAPR_PCI_WINDOW_BASE0x100ULL
 +#define SPAPR_PCI_WINDOW_SPACING 0x10ULL
 +#define SPAPR_PCI_MMIO_WIN_OFF   0xA000
 +#define SPAPR_PCI_MMIO_WIN_SIZE  (SPAPR_PCI_WINDOW_SPACING - \
 + SPAPR_PCI_MEM_WIN_BUS_OFFSET)
 +#define SPAPR_PCI_IO_WIN_OFF 0x8000
 +#define SPAPR_PCI_IO_WIN_SIZE0x1
 +
 +/* index is the phb index */
 +
 +#define BUIDBASE(index)  (SPAPR_PCI_BASE_BUID + (index))
 +#define PCIBASE(index)   (SPAPR_PCI_WINDOW_BASE + \
 +  (index) * SPAPR_PCI_WINDOW_SPACING)
 +#define IOBASE(index)(PCIBASE(index) + 
 SPAPR_PCI_IO_WIN_OFF)
 +#define MMIOBASE(index)  (PCIBASE(index) + 
 SPAPR_PCI_MMIO_WIN_OFF)
 +
 +typedef struct QPCIBusSPAPR {
 +QPCIBus bus;
 +QGuestAllocator *alloc;
 +
 +uint64_t pci_hole_start;
 +uint64_t pci_hole_size;
 +uint64_t pci_hole_alloc;
 +
 +uint32_t pci_iohole_start;
 +uint32_t pci_iohole_size;
 +uint32_t pci_iohole_alloc;
 +} QPCIBusSPAPR;
 +
 +static uint8_t qpci_spapr_io_readb(QPCIBus *bus, void *addr)
 +{
 +uint64_t port = (uint64_t)addr;
 +uint8_t v;
 +if (port < SPAPR_PCI_IO_WIN_SIZE) {
 +v = readb(IOBASE(0) + 

Re: [Qemu-devel] [PATCH v12 2/2] docs: Add a generic loader explanation document

[Alistair Francis]

On Thu, Sep 29, 2016 at 10:36 PM, Markus Armbruster  wrote:
> Alistair Francis  writes:
>
>> On Thu, Sep 29, 2016 at 2:24 AM, Markus Armbruster  wrote:
>>> Alistair Francis  writes:
>>>
 Signed-off-by: Alistair Francis 
 Reviewed-by: Peter Maydell 
 ---
 V11:
  - Fix corrections
 V10:
  - Split the data loading and PC setting
 V9:
  - Clarify the image loading options
 V8:
  - Improve documentation
 V6:
  - Fixup documentation
 V4:
  - Re-write to be more comprehensive

  docs/generic-loader.txt | 81 
 +
  1 file changed, 81 insertions(+)
  create mode 100644 docs/generic-loader.txt

 diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
 new file mode 100644
 index 000..d1f8ce3
 --- /dev/null
 +++ b/docs/generic-loader.txt
 @@ -0,0 +1,81 @@
 +Copyright (c) 2016 Xilinx Inc.
 +
 +This work is licensed under the terms of the GNU GPL, version 2 or later. 
  See
 +the COPYING file in the top-level directory.
 +
 +
 +The 'loader' device allows the user to load multiple images or values into
 +QEMU at startup.
 +
 +Loading Data into Memory Values
 +-
 +The loader device allows memory values to be set from the command line. 
 This
 +can be done by following the syntax below:
 +
 + -device loader,addr=,data=,data-len=
 +   [,data-be=][,cpu-num=]
 +
 +  - The address to store the data in.
 +  - The value to be written to the address. The maximum 
 size of
 +  the data is 8 bytes.
 +  - The length of the data in bytes. This argument must be
 +  included if the data argument is.
 +   - Set to true if the data to be stored on the guest 
 should be
 +  written as big endian data. The default is to write 
 little
 +  endian data.
 +   - The number of the CPU's address space where the data 
 should
 +  be loaded. If not specified the address space of the 
 first
 +  CPU is used.
 +
 +For all values both hex and decimal values are allowed. By default the 
 values
 +will be parsed as decimal. To use hex values the user should prefix the 
 number
 +with a '0x'.
>>>
>>> Unless you bypassed QemuOpts number parsing somehow, octal works as
>>> well.  In case you did bypass: don't!  Command line consistency matters.
>>> Follow-up patch reverting the bypass would be required.
>>>
>>> Not sure we want to document QemuOpts number syntax everywhere we
>>> explain how a certain feature uses the command line.  A pointer to the
>>> canonical place could be better.  Anyway, not something that needs
>>> fixing before we commit.
>>
>> I didn't bypass it, octal should work as well. I have clarified that a
>> bit in the doc.
>
> Thanks.
>
 +
 +An example of loading value 0x800e to address 0xfd1a0104 is:
 +-device loader,addr=0xfd1a0104,data=0x800e,data-len=4
 +
 +Setting a CPU's Program Counter
 +-
 +The loader device allows the CPU's PC to be set from the command line. 
 This
 +can be done by following the syntax below:
 +
 + -device loader,addr=,cpu-num=
 +
 +  - The value to use as the CPU's PC.
 +   - The number of the CPU whose PC should be set to the
 +  specified value.
 +
 +For all values both hex and decimal values are allowed. By default the 
 values
 +will be parsed as decimal. To use hex values the user should prefix the 
 number
 +with a '0x'.
 +
 +An example of setting CPU 0's PC to 0x8000 is:
 +-device loader,addr=0x8000,cpu-num=0
 +
 +Loading Files
 +-
 +The loader device also allows files to be loaded into memory. This can be 
 done
 +similarly to setting memory values. The syntax is shown below:
 +
 +-device 
 loader,file=[,addr=][,cpu-num=][,force-raw=]
 +
 +  - A file to be loaded into memory
 +  - The addr in memory that the file should be loaded. This 
 is
 +  ignored if you are using an ELF (unless force-raw is 
 true).
 +  This is required if you aren't loading an ELF.
 +   - This specifies the CPU that should be used. This is an
 +  optional argument and will cause the CPU's PC to be set 
 to
 +  where the image is stored or in the case of an ELF file 
 to
 +  the value in the header. This option should only be 

Re: [Qemu-devel] [PATCH] Reducing stack frame size in stream_process_mem2s()

[Rutuja Shah]

++ stefan Sorry for the typo.
Regards
Rutuja Shah


On Mon, Oct 3, 2016 at 10:26 PM,   wrote:
> From: Rutuja Shah 
>
> This patch allocates memory for txbuf array on the heap rather than the stack.
> As a result, the stack frame size is reduced.
>
> Signed-off-by: Rutuja Shah 
> ---
>  hw/dma/xilinx_axidma.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index b135a5f..6c63575 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -256,13 +256,14 @@ static void stream_process_mem2s(struct Stream *s, 
> StreamSlave *tx_data_dev,
>   StreamSlave *tx_control_dev)
>  {
>  uint32_t prev_d;
> -unsigned char txbuf[16 * 1024];
> +unsigned char *txbuf;
>  unsigned int txlen;
>
>  if (!stream_running(s) || stream_idle(s)) {
>  return;
>  }
>
> +txbuf = g_malloc(16 * 1024);
>  while (1) {
>  stream_desc_load(s, s->regs[R_CURDESC]);
>
> @@ -304,6 +305,7 @@ static void stream_process_mem2s(struct Stream *s, 
> StreamSlave *tx_data_dev,
>  break;
>  }
>  }
> +g_free(txbuf);
>  }
>
>  static size_t stream_process_s2mem(struct Stream *s, unsigned char *buf,
> --
> 1.9.1
>

Re: [Qemu-devel] [PATCH 3/3] target-arm: Comments to mark location of pending work for 56 bit addresses

[Tom Hanson]

On 09/30/2016 05:24 PM, Peter Maydell wrote:
 3 comments added in same file to identify cases in a switch.
>>>
>>> This should be a separate patch, because it is unrelated to the
>>> tagged address stuff.
>>
>> As part of that same conversation you suggested adding these
>> comments rather than making the changes:
>> If we can assert, or failing that have a comment in the place
>> that would be modified anyway for 56 bit addresses then that
>> ought to catch the future case I think.
> 
> Yes, I still think this. What does it have to do with adding
> "SVC", "HVC", etc comments to the switch cases? Those have
> nothing to do with tagged addresses or 56 bit VAs, and should
> not be in this patch (though I don't object to them inherently).
> 
> thanks
> -- PMM

Sorry, moving too fast and didn't look at which comments you were referring to. 
 I'll drop them.

-Tom

[Qemu-devel] [PATCH] Reducing stack frame size in stream_process_mem2s()

[rutu . shah . 26]

From: Rutuja Shah 

This patch allocates memory for txbuf array on the heap rather than the stack.
As a result, the stack frame size is reduced.

Signed-off-by: Rutuja Shah 
---
 hw/dma/xilinx_axidma.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index b135a5f..6c63575 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -256,13 +256,14 @@ static void stream_process_mem2s(struct Stream *s, 
StreamSlave *tx_data_dev,
  StreamSlave *tx_control_dev)
 {
 uint32_t prev_d;
-unsigned char txbuf[16 * 1024];
+unsigned char *txbuf;
 unsigned int txlen;
 
 if (!stream_running(s) || stream_idle(s)) {
 return;
 }
 
+txbuf = g_malloc(16 * 1024);
 while (1) {
 stream_desc_load(s, s->regs[R_CURDESC]);
 
@@ -304,6 +305,7 @@ static void stream_process_mem2s(struct Stream *s, 
StreamSlave *tx_data_dev,
 break;
 }
 }
+g_free(txbuf);
 }
 
 static size_t stream_process_s2mem(struct Stream *s, unsigned char *buf,
-- 
1.9.1

Re: [Qemu-devel] [PULL 14/20] target-i386: Move xsave component mask to features array

[Daniel P. Berrange]

On Tue, Sep 27, 2016 at 05:12:24PM -0300, Eduardo Habkost wrote:
> This will reuse the existing check/enforce logic in
> x86_cpu_filter_features() to check the xsave component bits
> against GET_SUPPORTED_CPUID.
> 
> Reviewed-by: Richard Henderson 
> Signed-off-by: Eduardo Habkost 
> ---
>  target-i386/cpu.c | 42 --
>  target-i386/cpu.h |  3 ++-
>  2 files changed, 30 insertions(+), 15 deletions(-)

git bisect tells me that this change is responsible for breaking booting
of a guest of mine which uses -cpu host

GRUB works, but the guest kernel hangs immediately after printing

"Probing EDD (edd=off to disable)... ok"

Removing '-cpu host' lets it work again.

My command line is:

/home/berrange/usr/qemu-git/bin/qemu-system-x86_64
-name guest=f23x86_64,debug-threads=on
-S
-object 
secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-18-f23x86_64/master-key.aes
-machine pc-q35-2.6,accel=kvm,usb=off,vmport=off
-cpu host
-m 8000
-realtime mlock=off
-smp 8,sockets=8,cores=1,threads=1
-numa node,nodeid=0,cpus=0-3,mem=4000
-numa node,nodeid=1,cpus=4-5,mem=2000
-numa node,nodeid=2,cpus=6-7,mem=2000
-uuid ac4c9e05-6137-4bde-a33a-5c3623f44fb2
-no-user-config
-nodefaults
-chardev 
socket,id=charmonitor,path=/var/lib/libvirt/qemu/domain-18-f23x86_64/monitor.sock,server,nowait
-mon chardev=charmonitor,id=monitor,mode=control
-rtc base=utc,driftfix=slew
-global kvm-pit.lost_tick_policy=discard
-no-hpet
-no-shutdown
-global ICH9-LPC.disable_s3=1
-global ICH9-LPC.disable_s4=1
-boot strict=on
-device intel-iommu
-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e
-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0
-device pxb-pcie,bus_nr=214,id=pci.3,numa_node=0,bus=pcie.0,addr=0x2
-device pxb-pcie,bus_nr=234,id=pci.4,numa_node=1,bus=pcie.0,addr=0x3
-device pxb-pcie,bus_nr=254,id=pci.5,numa_node=2,bus=pcie.0,addr=0x4
-device i82801b11-bridge,id=pci.6,bus=pci.3,addr=0x0
-device i82801b11-bridge,id=pci.7,bus=pci.4,addr=0x0
-device i82801b11-bridge,id=pci.8,bus=pci.5,addr=0x0
-device ich9-usb-ehci1,id=usb,bus=pcie.0,addr=0x1d.0x7
-device 
ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pcie.0,multifunction=on,addr=0x1d
-device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pcie.0,addr=0x1d.0x1
-device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pcie.0,addr=0x1d.0x2
-device virtio-serial-pci,id=virtio-serial0,bus=pci.2,addr=0x3
-drive 
file=/var/lib/libvirt/images/f23x86_64.img,format=qcow2,if=none,id=drive-virtio-disk0
-device 
virtio-blk-pci,scsi=off,bus=pci.2,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
-netdev tap,fd=26,id=hostnet0,vhost=on,vhostfd=28
-device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:9f:58:3c,bus=pci.2,addr=0x1
-netdev user,id=hostnet1
-device e1000,netdev=hostnet1,id=net1,mac=52:54:00:7e:6e:c6,bus=pci.6,addr=0x1
-netdev user,id=hostnet2
-device e1000,netdev=hostnet2,id=net2,mac=52:54:00:7e:6e:c7,bus=pci.6,addr=0x2
-netdev user,id=hostnet3
-device e1000,netdev=hostnet3,id=net3,mac=52:54:00:7e:6e:c8,bus=pci.6,addr=0x3
-netdev user,id=hostnet4
-device e1000,netdev=hostnet4,id=net4,mac=52:54:00:7e:6e:d6,bus=pci.7,addr=0x1
-netdev user,id=hostnet5
-device e1000,netdev=hostnet5,id=net5,mac=52:54:00:7e:6e:d7,bus=pci.7,addr=0x2
-netdev user,id=hostnet6
-device e1000,netdev=hostnet6,id=net6,mac=52:54:00:7e:6e:d8,bus=pci.7,addr=0x3
-netdev user,id=hostnet7
-device e1000,netdev=hostnet7,id=net7,mac=52:54:00:7e:6e:e6,bus=pci.8,addr=0x1
-netdev user,id=hostnet8
-device e1000,netdev=hostnet8,id=net8,mac=52:54:00:7e:6e:e7,bus=pci.8,addr=0x2
-netdev user,id=hostnet9
-device e1000,netdev=hostnet9,id=net9,mac=52:54:00:7e:6e:e8,bus=pci.8,addr=0x3
-chardev pty,id=charserial0
-device isa-serial,chardev=charserial0,id=serial0
-chardev 
socket,id=charchannel0,path=/var/lib/libvirt/qemu/channel/target/domain-18-f23x86_64/org.qemu.guest_agent.0,server,nowait
-device 
virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=org.qemu.guest_agent.0
-device usb-tablet,id=input0,bus=usb.0,port=1
-vnc 127.0.0.1:0
-device 
qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,vram64_size_mb=0,vgamem_mb=16,max_outputs=1,bus=pcie.0,addr=0x1
-device intel-hda,id=sound0,bus=pci.2,addr=0x2
-device hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0
-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x5
-msg timestamp=on


In case its relevant, QEMU prints this to stderr:

warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 0]
warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 1]
warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 2]
warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 0]
warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 1]
warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 2]
warning: host doesn't support requested feature: CPUID.0DH:EAX [bit 0]
warning: host doesn't support 

[Qemu-devel] [PATCH v2 2/4] tests/docker: test-build script

[Alex Bennée]

Much like test-quick but only builds. This is useful for some of the
build targets like ThreadSanitizer that don't yet pass "make check".

Signed-off-by: Alex Bennée 
---
 tests/docker/test-build | 18 ++
 1 file changed, 18 insertions(+)
 create mode 100755 tests/docker/test-build

diff --git a/tests/docker/test-build b/tests/docker/test-build
new file mode 100755
index 000..d237ead
--- /dev/null
+++ b/tests/docker/test-build
@@ -0,0 +1,18 @@
+#!/bin/bash -e
+#
+# Quick compiling test that everyone already does. But why not automate it?
+#
+# Copyright (c) 2016 Red Hat Inc.
+#
+# Authors:
+#  Fam Zheng 
+#
+# This work is licensed under the terms of the GNU GPL, version 2
+# or (at your option) any later version. See the COPYING file in
+# the top-level directory.
+
+. common.rc
+
+DEF_TARGET_LIST="$(echo {x86_64,aarch64}-softmmu)"
+TARGET_LIST=${TARGET_LIST:-$DEF_TARGET_LIST} \
+build_qemu
-- 
2.9.3

Re: [Qemu-devel] [PATCH v4 04/13] cryptodev: introduce a new cryptodev backend

[Stefan Hajnoczi]

On Wed, Sep 28, 2016 at 04:25:43PM +0800, Gonglei wrote:
> +/* Max number of symetrical sessions */

s/symetrical/symmetric/

But why does the comment say "symetrical" when the constant name
MAX_NUM_SESSIONS seems to be a global limit for *all* sessions (not just
symmetric)?

> +#define MAX_NUM_SESSIONS 256

The guest can only have 256 sessions open?

What are the limits of real crypto libraries and accelerators?

> +
> +
> +struct QCryptoCryptoDevBackendBuiltin {
> +QCryptoCryptoDevBackend parent_obj;
> +
> +QCryptoCryptoDevBackendBuiltinSession *sessions[MAX_NUM_SESSIONS];
> +};
> +
> +static void qcrypto_cryptodev_backend_builtin_init(
> + QCryptoCryptoDevBackend *backend, Error **errp)
> +{
> +/* Only support one queue */
> +int queues = MAX(backend->conf.peers.queues, 1);
> +size_t i;
> +QCryptoCryptoDevBackendClientState *cc;
> +
> +for (i = 0; i < queues; i++) {
> +cc = qcrypto_cryptodev_backend_new_client(
> +  "cryptodev-builtin", NULL);
> +snprintf(cc->info_str, sizeof(cc->info_str),
> + "cryptodev-builtin%lu", i);
> +cc->queue_index = i;
> +
> +backend->conf.peers.ccs[i] = cc;
> +}
> +
> +backend->conf.crypto_services =
> + 1u << VIRTIO_CRYPTO_SERVICE_CIPHER |
> + 1u << VIRTIO_CRYPTO_SERVICE_HASH |
> + 1u << VIRTIO_CRYPTO_SERVICE_MAC;
> +backend->conf.cipher_algo_l = 1u << VIRTIO_CRYPTO_CIPHER_AES_CBC;
> +backend->conf.hash_algo = 1u << VIRTIO_CRYPTO_HASH_SHA1;
> +}
> +
> +static int
> +qcrypto_cryptodev_backend_builtin_get_unused_session_index(
> +  QCryptoCryptoDevBackendBuiltin *builtin)
> +{
> +size_t i;
> +
> +for (i = 0; i < MAX_NUM_SESSIONS; i++) {
> +if (builtin->sessions[i] == NULL) {
> +return i;
> +}
> +}
> +
> +return -1;
> +}
> +
> +static int
> +qcrypto_cryptodev_backend_builtin_get_algo(uint32_t key_len,
> +   Error **errp)
> +{
> +int algo;
> +
> +if (key_len == 128 / 8) {
> +algo = QCRYPTO_CIPHER_ALG_AES_128;
> +} else if (key_len == 192 / 8) {
> +algo = QCRYPTO_CIPHER_ALG_AES_192;
> +} else if (key_len == 256 / 8) {
> +algo = QCRYPTO_CIPHER_ALG_AES_256;
> +} else {
> +error_setg(errp, "unsupported key length :%u", key_len);
> +return -1;
> +}
> +
> +return algo;
> +}
> +
> +static int qcrypto_cryptodev_backend_builtin_create_cipher_session(
> +QCryptoCryptoDevBackendBuiltin *builtin,
> +QCryptoCryptoDevBackendSymSessionInfo *sess_info,
> +Error **errp)
> +{
> +int algo;
> +int mode;
> +QCryptoCipher *cipher;
> +int index;
> +QCryptoCryptoDevBackendBuiltinSession *sess;
> +
> +if (sess_info->op_type != VIRTIO_CRYPTO_SYM_OP_CIPHER) {
> +error_setg(errp, "unsupported optype :%u", sess_info->op_type);
> +return -1;
> +}
> +
> +index = 
> qcrypto_cryptodev_backend_builtin_get_unused_session_index(builtin);

Feel free to omit the function name prefix for static functions.  These
names are very long.

> +if (index < 0) {
> +error_setg(errp, "the total number of created session exceed %u",

"Total number of sessions created exceeds %u"

> +  MAX_NUM_SESSIONS);
> +return -1;
> +}
> +
> +switch (sess_info->cipher_alg) {
> +case VIRTIO_CRYPTO_CIPHER_AES_ECB:
> +algo = qcrypto_cryptodev_backend_builtin_get_algo(sess_info->key_len,
> +  errp);
> +if (algo < 0)  {
> +return -1;
> +}
> +mode = QCRYPTO_CIPHER_MODE_ECB;
> +break;
> +case VIRTIO_CRYPTO_CIPHER_AES_CBC:
> +algo = qcrypto_cryptodev_backend_builtin_get_algo(sess_info->key_len,
> +  errp);
> +if (algo < 0)  {
> +return -1;
> +}
> +mode = QCRYPTO_CIPHER_MODE_CBC;
> +break;
> +case VIRTIO_CRYPTO_CIPHER_AES_CTR:
> +algo = qcrypto_cryptodev_backend_builtin_get_algo(sess_info->key_len,
> +  errp);
> +if (algo < 0)  {
> +return -1;
> +}
> +mode = QCRYPTO_CIPHER_MODE_CTR;
> +break;
> +default:
> +error_setg(errp, "unsupported cipher alg :%u",
> +   sess_info->cipher_alg);
> +return -1;
> +}

Code duplication can be eliminated:

switch (sess_info->cipher_alg) {
case VIRTIO_CRYPTO_CIPHER_AES_ECB:
mode = QCRYPTO_CIPHER_MODE_ECB;
break;
...
}

algo = qcrypto_cryptodev_backend_builtin_get_algo(sess_info->key_len,
  errp);
if (algo < 0) {
return -1;
}

> +static void qcrypto_cryptodev_backend_builtin_cleanup(
> + 

[Qemu-devel] [PATCH v2 3/4] tests/docker: make test-mingw honour TARGET_LIST

[Alex Bennée]

The other builders honour this variable, so should the mingw build.

Signed-off-by: Alex Bennée 
---
 tests/docker/test-mingw | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/docker/test-mingw b/tests/docker/test-mingw
index c03757a..51e9c0b 100755
--- a/tests/docker/test-mingw
+++ b/tests/docker/test-mingw
@@ -15,8 +15,10 @@
 
 requires mingw dtc
 
+DEF_TARGET_LIST="x86_64-softmmu,aarch64-softmmu"
+
 for prefix in x86_64-w64-mingw32- i686-w64-mingw32-; do
-TARGET_LIST=x86_64-softmmu,aarch64-softmmu \
+TARGET_LIST=${TARGET_LIST:-$DEF_TARGET_LIST} \
 build_qemu --cross-prefix=$prefix \
 --enable-trace-backends=simple \
 --enable-debug \
-- 
2.9.3

[Qemu-devel] [PATCH v2 4/4] tests/docker/Makefile.include: add a generic docker-run target

[Alex Bennée]

This re-factors the docker makefile to include a docker-run target which
can be controlled entirely from environment variables specified on the
make command line. This allows us to run against any given docker image
we may have in our repository, for example:

make docker-run TEST="test-quick" IMAGE="debian:arm64" \
 EXECUTABLE=./aarch64-linux-user/qemu-aarch64

The existing docker-foo@bar targets still work but the inline
verification has been shunted into other target prerequisites before a
sub-make is invoked for the docker-run target.

Signed-off-by: Alex Bennée 

---
NB: I dropped the awk magic that verifies the image exists before
running. I couldn't get the thing to work in my shell so wasn't quite
sure what it was doing.

v2
 - fix spelling on arbitrary
 - document docker-run in help
 - add note on docker.py update
 - reduce noise on verifying other images
 - revert back to using filter
 - fix merge conflict
---
 tests/docker/Makefile.include | 89 ---
 1 file changed, 66 insertions(+), 23 deletions(-)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 2fcc3c6..87735d8 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -78,6 +78,7 @@ docker:
@echo ' "IMAGE" is one of the listed container 
name."'
@echo 'docker-image:Build all images.'
@echo 'docker-image-IMAGE:  Build image "IMAGE".'
+   @echo 'docker-run:  For manually running a "TEST" with 
"IMAGE"'
@echo
@echo 'Available container images:'
@echo '$(DOCKER_IMAGES)'
@@ -101,31 +102,73 @@ docker:
@echo 'NOCACHE=1Ignore cache when build images.'
@echo 'EXECUTABLE=Include executable in image.'
 
-docker-run-%: CMD = $(shell echo '$@' | sed -e 
's/docker-run-\([^@]*\)@\(.*\)/\1/')
-docker-run-%: IMAGE = $(shell echo '$@' | sed -e 
's/docker-run-\([^@]*\)@\(.*\)/\2/')
-docker-run-%: docker-qemu-src
+# This rule if for directly running against an arbitrary docker target.
+# It is called by the expanded docker targets (e.g. make
+# docker-test-foo@bar) which will do additional verification.
+#
+# For example: make docker-run TEST="test-quick" IMAGE="debian:arm64" 
EXECUTABLE=./aarch64-linux-user/qemu-aarch64
+#
+docker-run: docker-qemu-src
@mkdir -p "$(DOCKER_CCACHE_DIR)"
-   @if test -z "$(IMAGE)" || test -z "$(CMD)"; \
-   then echo "Invalid target"; exit 1; \
+   @if test -z "$(IMAGE)" || test -z "$(TEST)"; \
+   then echo "Invalid target $(IMAGE)/$(TEST)"; exit 1; \
+   fi
+   $(if $(EXECUTABLE), \
+   $(call quiet-command,   \
+   $(SRC_PATH)/tests/docker/docker.py update   \
+   $(IMAGE) $(EXECUTABLE), \
+   "  COPYING $(EXECUTABLE) to $(IMAGE)"))
+   $(call quiet-command,   \
+   $(SRC_PATH)/tests/docker/docker.py run  \
+   -t  \
+   $(if $V,,--rm)  \
+   $(if $(DEBUG),-i,--net=none)\
+   -e TARGET_LIST=$(TARGET_LIST)   \
+   -e EXTRA_CONFIGURE_OPTS="$(EXTRA_CONFIGURE_OPTS)" \
+   -e V=$V -e J=$J -e DEBUG=$(DEBUG)   \
+   -e SHOW_ENV=$(SHOW_ENV) \
+   -e CCACHE_DIR=/var/tmp/ccache   \
+   -v $$(readlink -e 
$(DOCKER_SRC_COPY)):/var/tmp/qemu:z$(COMMA)ro \
+   -v $(DOCKER_CCACHE_DIR):/var/tmp/ccache:z   \
+   $(IMAGE)\
+   /var/tmp/qemu/run   \
+   $(TEST), "  RUN $(TEST) in ${IMAGE}")
+
+#
+# Verification targets
+#
+# These targets help verify the test (CMD) and docker tag (IMAGE) are
+# part of the built in set of tests and images. You can still call the
+# docker-run target directly for testsing against arbitary images.
+#
+
+docker-verify-image-%: IMAGE = $(shell echo '$@' | sed -e 
's/docker-verify-image-\([^@]*\)@\(.*\)/\2/')
+docker-verify-image-%:
+   @if test -z "$(IMAGE)"; \
+   then echo "Invalid image"; exit 1;  \
fi
-   $(if $(filter $(TESTS),$(CMD)),$(if $(filter $(IMAGES),$(IMAGE)), \
-   $(call quiet-command,\
-   if $(SRC_PATH)/tests/docker/docker.py images | \
-   awk '$$1=="qemu" && $$2=="$(IMAGE)"{found=1} 
END{exit(!found)}'; then \
-   

[Qemu-devel] [PATCH v2 0/4] generic docker run patches

[Alex Bennée]

Hi Fam,

I've re-based the series for the generic run target. The aim being to
allow a developer to run tests against any generic docker target even
if it is not in the list of approved targets:

  make docker-run TEST=test-quick IMAGE=debian:arm64 \
EXECUTABLE=./aarch64-linux-user/qemu-aarch64 J=9 SHOW_ENV=1

The existing auto targets still work but have been converted to use
the generic runner once they have done the validation.

Since last post:
  - review comments on the main patch
  - added a travis docker file
  - minor tweak to the test-mingw script

Alex Bennée (4):
  tests/docker: add travis dockerfile
  tests/docker: test-build script
  tests/docker: make test-mingw honour TARGET_LIST
  tests/docker/Makefile.include: add a generic docker-run target

 tests/docker/Makefile.include  | 89 +-
 tests/docker/dockerfiles/travis.docker |  6 +++
 tests/docker/test-build| 18 +++
 tests/docker/test-mingw|  4 +-
 4 files changed, 93 insertions(+), 24 deletions(-)
 create mode 100644 tests/docker/dockerfiles/travis.docker
 create mode 100755 tests/docker/test-build

-- 
2.9.3

[Qemu-devel] [PATCH v2 1/4] tests/docker: add travis dockerfile

[Alex Bennée]

This target grabs the latest Travis containers from their repository at
quay.io and then installs QEMU's build dependencies. With this it is
possible to run on broadly the same setup as they have on travis-ci.org.

Signed-off-by: Alex Bennée 
---
 tests/docker/dockerfiles/travis.docker | 6 ++
 1 file changed, 6 insertions(+)
 create mode 100644 tests/docker/dockerfiles/travis.docker

diff --git a/tests/docker/dockerfiles/travis.docker 
b/tests/docker/dockerfiles/travis.docker
new file mode 100644
index 000..e4983ae
--- /dev/null
+++ b/tests/docker/dockerfiles/travis.docker
@@ -0,0 +1,6 @@
+FROM quay.io/travisci/travis-ruby
+RUN apt-get update
+RUN apt-get -y build-dep qemu
+RUN apt-get -y build-dep device-tree-compiler
+RUN apt-get -y install python2.7 dh-autoreconf
+ENV FEATURES pyyaml
-- 
2.9.3

Re: [Qemu-devel] [PATCH v3 03/15] exec-all.h: revert tb_page_addr_t to target_ulong

[Alex Bennée]

Emilio G. Cota  writes:

> On Mon, Oct 03, 2016 at 10:32:55 +0100, Alex Bennée wrote:
> (snip)
>> However the series as a whole does have value. As you can see from the
>> other patches there are some real races being picked up by the sanitizer
>> which only really become visible when a) you remove the noise of the
>> "false" positives and b) run the test many many times. For example this
>> one:
>>
>> ==
>> WARNING: ThreadSanitizer: data race (pid=24906)
>>   Read of size 8 at 0x7db4000261f0 by thread T3 (mutexes: write M8203):
>> #0 do_tb_flush /home/alex/lsrc/qemu/qemu.git/translate-all.c:872 
>> (qemu-arm+0x6000ce68)
>> #1 process_queued_cpu_work 
>> /home/alex/lsrc/qemu/qemu.git/cpus-common.c:337 (qemu-arm+0x60116712)
>> #2 cpu_loop /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:654 
>> (qemu-arm+0x60052213)
>> #3 clone_func /home/alex/lsrc/qemu/qemu.git/linux-user/syscall.c:6070 
>> (qemu-arm+0x600686fb)
>> #4   (libtsan.so.0+0x000230d9)
>>
>>   Previous write of size 8 at 0x7db4000261f0 by main thread (mutexes: write 
>> M8):
>> #0 cpu_list_add /home/alex/lsrc/qemu/qemu.git/cpus-common.c:87 
>> (qemu-arm+0x60115b7a)
>> #1 cpu_exec_init /home/alex/lsrc/qemu/qemu.git/exec.c:641 
>> (qemu-arm+0x60009900)
>> #2 arm_cpu_initfn /home/alex/lsrc/qemu/qemu.git/target-arm/cpu.c:447 
>> (qemu-arm+0x600f833b)
> [..]
>
> Nice! Which patch fixes this--patch 10? It would be cool to have this
> report in the corresponding commit message.

This particular one only actually showed up after I sent the last
series - I'd kicked off 1000 repeating tests just before I boarded my
flight back home :-)

However patch 10 fixes another rare case which is in the class of races
caused by creating or destroying a thread just as we flush.

>
> Thanks,
>
>   Emilio


--
Alex Bennée

Re: [Qemu-devel] [PATCH v4 03/13] virtio-crypto: introduce virtio_crypto.h

[Stefan Hajnoczi]

On Wed, Sep 28, 2016 at 04:25:42PM +0800, Gonglei wrote:
> Introdue the virtio_crypto.h which follows

s/Introdue/Introduce/


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH 1/2] async: add aio_bh_schedule_oneshot

[Paolo Bonzini]

qemu_bh_delete is already clearing bh->scheduled at the same time
as it's setting bh->deleted.  Since it's not using any memory
barriers, there is no synchronization going on for bh->deleted,
and this makes the bh->deleted checks superfluous in aio_compute_timeout,
aio_bh_poll and aio_ctx_check.

Just remove them, and put the (bh->scheduled && bh->deleted) combo
to work in a new function aio_bh_schedule_oneshot.  The new function
removes the need to save the QEMUBH pointer between the creation
and the execution of the bottom half.

Signed-off-by: Paolo Bonzini 
---
 async.c | 27 +++
 include/block/aio.h | 10 ++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/async.c b/async.c
index 3bca9b0..f30d011 100644
--- a/async.c
+++ b/async.c
@@ -44,6 +44,25 @@ struct QEMUBH {
 bool deleted;
 };
 
+void aio_bh_schedule_oneshot(AioContext *ctx, QEMUBHFunc *cb, void *opaque)
+{
+QEMUBH *bh;
+bh = g_new(QEMUBH, 1);
+*bh = (QEMUBH){
+.ctx = ctx,
+.cb = cb,
+.opaque = opaque,
+};
+qemu_mutex_lock(>bh_lock);
+bh->next = ctx->first_bh;
+bh->scheduled = 1;
+bh->deleted = 1;
+/* Make sure that the members are ready before putting bh into list */
+smp_wmb();
+ctx->first_bh = bh;
+qemu_mutex_unlock(>bh_lock);
+}
+
 QEMUBH *aio_bh_new(AioContext *ctx, QEMUBHFunc *cb, void *opaque)
 {
 QEMUBH *bh;
@@ -86,7 +105,7 @@ int aio_bh_poll(AioContext *ctx)
  * thread sees the zero before bh->cb has run, and thus will call
  * aio_notify again if necessary.
  */
-if (!bh->deleted && atomic_xchg(>scheduled, 0)) {
+if (atomic_xchg(>scheduled, 0)) {
 /* Idle BHs and the notify BH don't count as progress */
 if (!bh->idle && bh != ctx->notify_dummy_bh) {
 ret = 1;
@@ -104,7 +123,7 @@ int aio_bh_poll(AioContext *ctx)
 bhp = >first_bh;
 while (*bhp) {
 bh = *bhp;
-if (bh->deleted) {
+if (bh->deleted && !bh->scheduled) {
 *bhp = bh->next;
 g_free(bh);
 } else {
@@ -168,7 +187,7 @@ aio_compute_timeout(AioContext *ctx)
 QEMUBH *bh;
 
 for (bh = ctx->first_bh; bh; bh = bh->next) {
-if (!bh->deleted && bh->scheduled) {
+if (bh->scheduled) {
 if (bh->idle) {
 /* idle bottom halves will be polled at least
  * every 10ms */
@@ -216,7 +235,7 @@ aio_ctx_check(GSource *source)
 aio_notify_accept(ctx);
 
 for (bh = ctx->first_bh; bh; bh = bh->next) {
-if (!bh->deleted && bh->scheduled) {
+if (bh->scheduled) {
 return true;
 }
 }
diff --git a/include/block/aio.h b/include/block/aio.h
index 173c1ed..be7dd3b 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -181,6 +181,16 @@ void aio_context_acquire(AioContext *ctx);
 void aio_context_release(AioContext *ctx);
 
 /**
+ * aio_bh_schedule_oneshot: Allocate a new bottom half structure that will run
+ * only once and as soon as possible.
+ *
+ * Bottom halves are lightweight callbacks whose invocation is guaranteed
+ * to be wait-free, thread-safe and signal-safe.  The #QEMUBH structure
+ * is opaque and must be allocated prior to its use.
+ */
+void aio_bh_schedule_oneshot(AioContext *ctx, QEMUBHFunc *cb, void *opaque);
+
+/**
  * aio_bh_new: Allocate a new bottom half structure.
  *
  * Bottom halves are lightweight callbacks whose invocation is guaranteed
-- 
2.7.4

[Qemu-devel] [PATCH 2/2] block: use aio_bh_schedule_oneshot

[Paolo Bonzini]

This simplifies bottom half handlers by removing calls to qemu_bh_delete and
thus removing the need to stash the bottom half pointer in the opaque
datum.

Signed-off-by: Paolo Bonzini 
---
 block/archipelago.c   |  5 +
 block/blkdebug.c  |  7 +--
 block/blkverify.c |  8 ++--
 block/block-backend.c | 23 +++
 block/curl.c  |  7 +--
 block/gluster.c   |  6 +-
 block/io.c| 11 +++
 block/iscsi.c |  7 ++-
 block/nfs.c   |  7 ++-
 block/null.c  |  5 +
 block/qed.c   |  6 ++
 block/qed.h   |  1 -
 block/rbd.c   |  8 ++--
 blockjob.c|  7 ++-
 14 files changed, 27 insertions(+), 81 deletions(-)

diff --git a/block/archipelago.c b/block/archipelago.c
index 37b8aca..2449cfc 100644
--- a/block/archipelago.c
+++ b/block/archipelago.c
@@ -87,7 +87,6 @@ typedef enum {
 
 typedef struct ArchipelagoAIOCB {
 BlockAIOCB common;
-QEMUBH *bh;
 struct BDRVArchipelagoState *s;
 QEMUIOVector *qiov;
 ARCHIPCmd cmd;
@@ -154,11 +153,10 @@ static void archipelago_finish_aiocb(AIORequestData 
*reqdata)
 } else if (reqdata->aio_cb->ret == reqdata->segreq->total) {
 reqdata->aio_cb->ret = 0;
 }
-reqdata->aio_cb->bh = aio_bh_new(
+aio_bh_schedule_oneshot(
 bdrv_get_aio_context(reqdata->aio_cb->common.bs),
 qemu_archipelago_complete_aio, reqdata
 );
-qemu_bh_schedule(reqdata->aio_cb->bh);
 }
 
 static int wait_reply(struct xseg *xseg, xport srcport, struct xseg_port *port,
@@ -313,7 +311,6 @@ static void qemu_archipelago_complete_aio(void *opaque)
 AIORequestData *reqdata = (AIORequestData *) opaque;
 ArchipelagoAIOCB *aio_cb = (ArchipelagoAIOCB *) reqdata->aio_cb;
 
-qemu_bh_delete(aio_cb->bh);
 aio_cb->common.cb(aio_cb->common.opaque, aio_cb->ret);
 aio_cb->status = 0;
 
diff --git a/block/blkdebug.c b/block/blkdebug.c
index d5db166..4127571 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -49,7 +49,6 @@ typedef struct BDRVBlkdebugState {
 
 typedef struct BlkdebugAIOCB {
 BlockAIOCB common;
-QEMUBH *bh;
 int ret;
 } BlkdebugAIOCB;
 
@@ -410,7 +409,6 @@ out:
 static void error_callback_bh(void *opaque)
 {
 struct BlkdebugAIOCB *acb = opaque;
-qemu_bh_delete(acb->bh);
 acb->common.cb(acb->common.opaque, acb->ret);
 qemu_aio_unref(acb);
 }
@@ -421,7 +419,6 @@ static BlockAIOCB *inject_error(BlockDriverState *bs,
 BDRVBlkdebugState *s = bs->opaque;
 int error = rule->options.inject.error;
 struct BlkdebugAIOCB *acb;
-QEMUBH *bh;
 bool immediately = rule->options.inject.immediately;
 
 if (rule->options.inject.once) {
@@ -436,9 +433,7 @@ static BlockAIOCB *inject_error(BlockDriverState *bs,
 acb = qemu_aio_get(_aiocb_info, bs, cb, opaque);
 acb->ret = -error;
 
-bh = aio_bh_new(bdrv_get_aio_context(bs), error_callback_bh, acb);
-acb->bh = bh;
-qemu_bh_schedule(bh);
+aio_bh_schedule_oneshot(bdrv_get_aio_context(bs), error_callback_bh, acb);
 
 return >common;
 }
diff --git a/block/blkverify.c b/block/blkverify.c
index da62d75..28f9af6 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -22,7 +22,6 @@ typedef struct {
 typedef struct BlkverifyAIOCB BlkverifyAIOCB;
 struct BlkverifyAIOCB {
 BlockAIOCB common;
-QEMUBH *bh;
 
 /* Request metadata */
 bool is_write;
@@ -175,7 +174,6 @@ static BlkverifyAIOCB *blkverify_aio_get(BlockDriverState 
*bs, bool is_write,
 {
 BlkverifyAIOCB *acb = qemu_aio_get(_aiocb_info, bs, cb, opaque);
 
-acb->bh = NULL;
 acb->is_write = is_write;
 acb->sector_num = sector_num;
 acb->nb_sectors = nb_sectors;
@@ -191,7 +189,6 @@ static void blkverify_aio_bh(void *opaque)
 {
 BlkverifyAIOCB *acb = opaque;
 
-qemu_bh_delete(acb->bh);
 if (acb->buf) {
 qemu_iovec_destroy(>raw_qiov);
 qemu_vfree(acb->buf);
@@ -218,9 +215,8 @@ static void blkverify_aio_cb(void *opaque, int ret)
 acb->verify(acb);
 }
 
-acb->bh = aio_bh_new(bdrv_get_aio_context(acb->common.bs),
- blkverify_aio_bh, acb);
-qemu_bh_schedule(acb->bh);
+aio_bh_schedule_oneshot(bdrv_get_aio_context(acb->common.bs),
+blkverify_aio_bh, acb);
 break;
 }
 }
diff --git a/block/block-backend.c b/block/block-backend.c
index 0bd19ab..37595d2 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -65,7 +65,6 @@ struct BlockBackend {
 
 typedef struct BlockBackendAIOCB {
 BlockAIOCB common;
-QEMUBH *bh;
 BlockBackend *blk;
 int ret;
 } BlockBackendAIOCB;
@@ -898,7 +897,6 @@ int blk_make_zero(BlockBackend *blk, BdrvRequestFlags flags)
 static void error_callback_bh(void *opaque)
 {
 struct BlockBackendAIOCB *acb = opaque;
-qemu_bh_delete(acb->bh);
 

Re: [Qemu-devel] [PATCH v4 01/13] cryptodev: introduce cryptodev backend interface

[Daniel P. Berrange]

On Mon, Oct 03, 2016 at 05:10:48PM +0100, Stefan Hajnoczi wrote:
> On Wed, Sep 28, 2016 at 04:25:40PM +0800, Gonglei wrote:
> > diff --git a/backends/cryptodev.c b/backends/cryptodev.c
> > new file mode 100644
> > index 000..a15904b
> > --- /dev/null
> > +++ b/backends/cryptodev.c
> > @@ -0,0 +1,175 @@
> > +/*
> > + * QEMU Crypto Device Implement
> 
> s/Implement/Implementation/
> 
> > diff --git a/include/sysemu/cryptodev.h b/include/sysemu/cryptodev.h
> > new file mode 100644
> > index 000..cc3c3be
> > --- /dev/null
> > +++ b/include/sysemu/cryptodev.h
> > @@ -0,0 +1,145 @@
> > +/*
> > + * QEMU Crypto Device Implement
> 
> s/Implement/Implementation/
> 
> > + *
> > + * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
> > + *
> > + * Authors:
> > + *Gonglei 
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, see 
> > .
> > + *
> > + */
> > +#ifndef QCRYPTO_CRYPTODEV_H
> > +#define QCRYPTO_CRYPTODEV_H
> > +
> > +#include "qom/object.h"
> > +#include "qemu-common.h"
> > +
> > +/**
> > + * QCryptoCryptoDevBackend:
> > + *
> > + * The QCryptoCryptoDevBackend object is an interface
> > + * for different cryptodev backends, which provides crypto
> > + * operation wrapper.
> 
> I suggest calling it CryptoDevBackend since that's shorter and doesn't
> repeat any information.  I'm not sure why "QCrypto" is necessary.

I suggested that naming when we had it under the crypto/ directory,
since that's the standard for code there. We've moved this into
the backends/ directory now, so we don't need that name prefix
anymore.




Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

[Qemu-devel] [PATCH 0/2] block: introduce and use aio_bh_schedule_oneshot

[Paolo Bonzini]

This simplifies a bit using the bottom half API in the common
case of one-shot bottom halves, that are created once per usage.
This patch comes from the multiqueue series.

Paolo

Paolo Bonzini (2):
  async: add aio_bh_schedule_oneshot
  block: use aio_bh_schedule_oneshot

 async.c   | 27 +++
 block/archipelago.c   |  5 +
 block/blkdebug.c  |  7 +--
 block/blkverify.c |  8 ++--
 block/block-backend.c | 23 +++
 block/curl.c  |  7 +--
 block/gluster.c   |  6 +-
 block/io.c| 11 +++
 block/iscsi.c |  7 ++-
 block/nfs.c   |  7 ++-
 block/null.c  |  5 +
 block/qed.c   |  6 ++
 block/qed.h   |  1 -
 block/rbd.c   |  8 ++--
 blockjob.c|  7 ++-
 include/block/aio.h   | 10 ++
 16 files changed, 60 insertions(+), 85 deletions(-)

-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 02/13] cryptodev: add symmetric algorithm operation stuff

[Stefan Hajnoczi]

On Wed, Sep 28, 2016 at 04:25:41PM +0800, Gonglei wrote:
> This patch add session operation and crypto operation

s/add/adds/

> stuff in the cryptodev backend, including function
> pointers and correpsonding structures.

s/correpsonding/corresponding/

> +/**
> + * QCryptoCryptoDevBackendSymOpInfo:
> + *
> + * @session_id: session index which was previously
> + *  created by qcrypto_cryptodev_backend_sym_create_session()
> + * @aad_len: byte length of additional authenticated data
> + * @iv_len: byte length of initialization vector or counter
> + * @src_len: byte length of source data
> + * @dst_len: byte length of destination data, which is equal to
> + *   src_len + hash_result_len if HASH alg configured
> + * @op_type: operation type (refer to virtio_crypto.h)
> + * @iv: pointer to the initialization vector or counter
> + * @src: pointer to the source data
> + * @dst: pointer to the destination data
> + * @dst: pointer to the additional authenticated data

s/dst/aad_data/


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v4 01/13] cryptodev: introduce cryptodev backend interface

[Stefan Hajnoczi]

On Wed, Sep 28, 2016 at 04:25:40PM +0800, Gonglei wrote:
> diff --git a/backends/cryptodev.c b/backends/cryptodev.c
> new file mode 100644
> index 000..a15904b
> --- /dev/null
> +++ b/backends/cryptodev.c
> @@ -0,0 +1,175 @@
> +/*
> + * QEMU Crypto Device Implement

s/Implement/Implementation/

> diff --git a/include/sysemu/cryptodev.h b/include/sysemu/cryptodev.h
> new file mode 100644
> index 000..cc3c3be
> --- /dev/null
> +++ b/include/sysemu/cryptodev.h
> @@ -0,0 +1,145 @@
> +/*
> + * QEMU Crypto Device Implement

s/Implement/Implementation/

> + *
> + * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
> + *
> + * Authors:
> + *Gonglei 
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> .
> + *
> + */
> +#ifndef QCRYPTO_CRYPTODEV_H
> +#define QCRYPTO_CRYPTODEV_H
> +
> +#include "qom/object.h"
> +#include "qemu-common.h"
> +
> +/**
> + * QCryptoCryptoDevBackend:
> + *
> + * The QCryptoCryptoDevBackend object is an interface
> + * for different cryptodev backends, which provides crypto
> + * operation wrapper.

I suggest calling it CryptoDevBackend since that's shorter and doesn't
repeat any information.  I'm not sure why "QCrypto" is necessary.

> + *
> + */
> +
> +#define TYPE_QCRYPTO_CRYPTODEV_BACKEND "cryptodev-backend"
> +
> +#define QCRYPTO_CRYPTODEV_BACKEND(obj) \
> +OBJECT_CHECK(QCryptoCryptoDevBackend, \
> + (obj), TYPE_QCRYPTO_CRYPTODEV_BACKEND)
> +#define QCRYPTO_CRYPTODEV_BACKEND_GET_CLASS(obj) \
> +OBJECT_GET_CLASS(QCryptoCryptoDevBackendClass, \
> + (obj), TYPE_QCRYPTO_CRYPTODEV_BACKEND)
> +#define QCRYPTO_CRYPTODEV_BACKEND_CLASS(klass) \
> +OBJECT_CLASS_CHECK(QCryptoCryptoDevBackendClass, \
> +(klass), TYPE_QCRYPTO_CRYPTODEV_BACKEND)
> +
> +
> +#define MAX_CRYPTO_QUEUE_NUM  64
> +
> +typedef struct QCryptoCryptoDevBackendConf QCryptoCryptoDevBackendConf;
> +typedef struct QCryptoCryptoDevBackendPeers QCryptoCryptoDevBackendPeers;
> +typedef struct QCryptoCryptoDevBackendClientState
> + QCryptoCryptoDevBackendClientState;
> +typedef struct QCryptoCryptoDevBackend QCryptoCryptoDevBackend;
> +
> +
> +typedef struct QCryptoCryptoDevBackendClass {
> +ObjectClass parent_class;
> +
> +void (*init)(QCryptoCryptoDevBackend *backend, Error **errp);
> +void (*cleanup)(QCryptoCryptoDevBackend *backend, Error **errp);
> +} QCryptoCryptoDevBackendClass;
> +
> +
> +struct QCryptoCryptoDevBackendClientState {

The naming could be simplified: CryptoDevClient.

> +char *model;
> +char *name;
> +char info_str[128];

This should probably be char *info_str unless there is a specific reason
to use a fixed-size buffer.

> +unsigned int queue_index;
> +QTAILQ_ENTRY(QCryptoCryptoDevBackendClientState) next;
> +};
> +
> +struct QCryptoCryptoDevBackendPeers {
> +QCryptoCryptoDevBackendClientState *ccs[MAX_CRYPTO_QUEUE_NUM];
> +uint32_t queues;
> +};
> +
> +struct QCryptoCryptoDevBackendConf {
> +QCryptoCryptoDevBackendPeers peers;
> +
> +/* Supported service mask */
> +uint32_t crypto_services;
> +
> +/* Detailed algorithms mask */
> +uint32_t cipher_algo_l;
> +uint32_t cipher_algo_h;
> +uint32_t hash_algo;
> +uint32_t mac_algo_l;
> +uint32_t mac_algo_h;
> +uint32_t asym_algo;
> +uint32_t kdf_algo;
> +uint32_t aead_algo;
> +uint32_t primitive_algo;
> +};
> +
> +struct QCryptoCryptoDevBackend {
> +Object parent_obj;
> +
> +int ready;

Please use bool.  That way it's clear the field only takes true and
false values.

> +QCryptoCryptoDevBackendConf conf;
> +};
> +
> +/**
> + * qcrypto_cryptodev_backend_new_client:
> + * @model: the cryptodev backend model
> + * @name: the cryptodev backend name, can be NULL
> + *
> + * Creates a new cryptodev backend client object
> + * with the @name in the mode @mode.

s/mode/model/

> + *
> + * The returned object must be released with
> + * qcrypto_cryptodev_backend_free_client() when no
> + * longer required
> + *
> + * Returns: a new cryptodev backend client object
> + */
> +QCryptoCryptoDevBackendClientState *
> +qcrypto_cryptodev_backend_new_client(const char *model,
> +const char *name);
> +/**
> + * qcrypto_cryptodev_backend_free_client:
> + * @cc: the cryptodev backend client 

Re: [Qemu-devel] [PATCH v3 1/8] apic: add global apic_get_class()

[Eduardo Habkost]

On Fri, Sep 30, 2016 at 06:10:06PM +0200, Radim Krčmář wrote:
> Every configuration has only up to one APIC class and we'll be extending
> the class with a function that can be called without an instanced
> object, so a direct access to the class is convenient.
> 
> This patch will break compilation if some code uses apic_get_class()
> with CONFIG_USER_ONLY.
> 
> Suggested-by: Eduardo Habkost 
> Signed-off-by: Radim Krčmář 
> ---
> v2: assert() instead of error_report() and exit() [Peter]
> v3: completely rewrite the mechanism [Eduardo]
> 
> It still looks horrible, so I'll be glad for any advice.
> And what is CONFIG_USER_ONLY?
> ---
>  hw/intc/apic_common.c   |  1 +
>  include/hw/i386/apic_internal.h |  2 ++
>  target-i386/cpu.c   | 14 +++---
>  3 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index 14ac43c18666..8d01c9c8750e 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -18,6 +18,7 @@
>   * License along with this library; if not, see 
> 
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "qemu-common.h"
>  #include "cpu.h"
> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> index 06c4e9f6f95b..286684857e9f 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -222,4 +222,6 @@ static inline int apic_get_bit(uint32_t *tab, int index)
>  return !!(tab[i] & mask);
>  }
>  
> +APICCommonClass *apic_get_class(void);
> +
>  #endif /* QEMU_APIC_INTERNAL_H */
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 09b9a70e..6acf9c3c2372 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2845,9 +2845,8 @@ static void mce_init(X86CPU *cpu)
>  }
>  
>  #ifndef CONFIG_USER_ONLY
> -static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
> +APICCommonClass *apic_get_class(void)
>  {
> -APICCommonState *apic;
>  const char *apic_type = "apic";
>  
>  if (kvm_apic_in_kernel()) {
> @@ -2856,7 +2855,16 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error 
> **errp)
>  apic_type = "xen-apic";
>  }
>  
> -cpu->apic_state = DEVICE(object_new(apic_type));
> +return APIC_COMMON_CLASS(object_class_by_name(apic_type));
> +}
> +
> +static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
> +{
> +APICCommonState *apic;
> +ObjectClass *apic_object_class = OBJECT_CLASS(apic_get_class());
> +
> +assert(apic_object_class);
> +cpu->apic_state = DEVICE(object_new_with_type(apic_object_class->type));

ObjectClass::type is private. I believe the common idiom is
object_new(object_class_get_name(c)).

Except for that, I believe the interface is OK and matches the
existing logic. We can always make it better later, if
appropriate.

(e.g. I wonder if we could have a container object for all APICs
(icc-bus?), and move the send_msi() method and the
apic_get_class() logic to it).

>  
>  object_property_add_child(OBJECT(cpu), "lapic",
>OBJECT(cpu->apic_state), _abort);
> -- 
> 2.10.0
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v11 1/2] virtio-crypto: Add virtio crypto device specification

[Stefan Hajnoczi]

On Wed, Sep 28, 2016 at 05:08:24PM +0800, Gonglei wrote:
> +For scatter/gather list support, a buffer can be represented by 
> virtio_crypto_iovec structure.
> +
> +The structure is defined as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_iovec {
> +/* Guest physical address */
> +le64 addr;
> +/* Length of guest physical address */
> +le32 len;
> +/* This marks a buffer as continuing via the next field */
> +#define VIRTIO_CRYPTO_IOVEC_F_NEXT 1
> +/* The flags as indicated above VIRTIO_CRYPTO_IOVEC_F_*. */
> +le32 flags;
> +/* Pointer to next struct virtio_crypto_iovec if flags & NEXT */
> +le64 next_iovec;
> +};
> +\end{lstlisting}

The vring already provides scatter-gather I/O.  It is usually not
necessary to define scatter-gather I/O at the device level.  Addresses
can be translated by the virtio transport (PCI, MMIO, CCW).  For example
PCI bus addresses with IO-MMU.  Therefore it's messy to use guest
physical addresses in the device specification.

> +Each data request uses virtio_crypto_hash_data_req structure to store 
> information
> +used to run the HASH operations. The request only occupies one entry
> +in the Vring Descriptor Table in the virtio crypto device's dataq, which 
> improves
> +the throughput of data transmitted for the HASH service, so that the virtio 
> crypto
> +device can be better accelerated.

Indirect vrings also only require one entry in the descriptor table.  I
don't understand why you are reinventing scatter-gather I/O.

Am I missing something?

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v2 1/1] target-i386: Correct family/model/stepping for Opteron_G3

[Eduardo Habkost]

On Mon, Oct 03, 2016 at 02:50:02PM +0300, Denis V. Lunev wrote:
> From: Evgeny Yakovlev 
> 
> Current CPU definition for AMD Opteron third generation includes
> features like SSE4a and LAHF_LM support in emulated CPUID. These
> features are present in K8 rev.E or K10 CPUs and later. However,
> current G3 family and model describe 2nd generation K8 cores instead.
> 
> This is incorrect but was considered harmless until our tests found a
> problem with linux kernels >= 3.10 (and maybe earlier) which specifically
> check for Opteron K8 model when parsing CPUID leaf 0x8001:
> http://lxr.free-electrons.com/source/arch/x86/kernel/cpu/amd.c?v=3.16#L552
> This code will disable LAHF_LM feature in /proc/cpuinfo if model number
> is inconsistent.
> 
> This change sets Opteron_G3 family/model/stepping to 16/2/3 which is
> a proper Opteron 3rd generation 2350 CPU.
> 
> Signed-off-by: Evgeny Yakovlev 
> Signed-off-by: Denis V. Lunev 
> CC: Paolo Bonzini 
> CC: Richard Henderson 
> CC: Eduardo Habkost 

Applied to x86-next. Thanks.

-- 
Eduardo

Re: [Qemu-devel] [Bug 1626972] Re: [PATCH] util: secure memfd_create fallback mechanism

[Rafael David Tinoco]

Sorry, I was only able to come back to this today.

> On Sep 27, 2016, at 09:18, Daniel Berrange <1626...@bugs.launchpad.net> wrote:
> 
>> There are numerous people relying on older kernels in openstack 
>> deployments - sometimes with specific drivers (ovswitch, dpdk, 
>> infiniband) holding kernel upgrades - but still in need of upgrading 
>> userland (e.g. newer releases). Having a fallback mechanism seems 
>> appropriate for those cases.
> 
> I'm not against some kind of fallback - just about the way it
> silently creates files in /tmp.
> 

That is why memfd_create is used here I suppose: To allow 
anonymous-backed-pages to have a descriptor and to be sealed. When falling back 
this mechanism I don't see any other way other than creating a temporary file. 
Of course one way would be something like:

http://paste.ubuntu.com/23270379/

But this is pretty much the same, just solving the "where to place the 
temporary file" (non configurable for this usage). 

>> 
>> Note that the filename, per se, is not as important as other files, 
>> since qemu won't provide it for being accessed by external programs, and,
>> deletes the file, while keeping the descriptor, right after its creation
>> (due to its nature, that is probably why it was created in /tmp).
> 
> If it doesn't shared with other processes, and is deleted immediately,
> why does the file need to be on disk at all ?

Well, it unlinks the file but the references are still there while the 
descriptor isn't closed by this process, or by the one that receives the 
descriptor (that is why is the "unlink" so early). 

If you check vhost_dev_log_resize(), it gets *possible* new vhost log (if a new 
size is given) and informs the vhost dev driver about the new log base 
(vhost_ops->vhost_set_log_base). 

For vhost_user, this means that the file descriptors for vhost logs are likely 
going to be passed to vhost backend (fds[] in vhost_user_set_log_base). This is 
just one example, not sure about others. 

Probably the best approach here, like what Marc-André said, is to create some 
sort of TMPDIR, set by libvirt perhaps ?

> 
> Regards,
> Daniel

Re: [Qemu-devel] [PATCH v4 04/12] block/nbd: Use qdict_put()

[Eric Blake]

On 09/28/2016 03:55 PM, Max Reitz wrote:
> Instead of inlining this nice macro (i.e. resorting to
> qdict_put_obj(..., QOBJECT(...))), use it.
> 
> Signed-off-by: Max Reitz 
> ---
>  block/nbd.c | 13 ++---
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 

Reviewed-by: Eric Blake 

Hmm - should we have a Coccinelle script to automate this?

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v3 03/15] exec-all.h: revert tb_page_addr_t to target_ulong

[Emilio G. Cota]

On Mon, Oct 03, 2016 at 10:32:55 +0100, Alex Bennée wrote:
(snip)
> However the series as a whole does have value. As you can see from the
> other patches there are some real races being picked up by the sanitizer
> which only really become visible when a) you remove the noise of the
> "false" positives and b) run the test many many times. For example this
> one:
> 
> ==
> WARNING: ThreadSanitizer: data race (pid=24906)
>   Read of size 8 at 0x7db4000261f0 by thread T3 (mutexes: write M8203):
> #0 do_tb_flush /home/alex/lsrc/qemu/qemu.git/translate-all.c:872 
> (qemu-arm+0x6000ce68)
> #1 process_queued_cpu_work 
> /home/alex/lsrc/qemu/qemu.git/cpus-common.c:337 (qemu-arm+0x60116712)
> #2 cpu_loop /home/alex/lsrc/qemu/qemu.git/linux-user/main.c:654 
> (qemu-arm+0x60052213)
> #3 clone_func /home/alex/lsrc/qemu/qemu.git/linux-user/syscall.c:6070 
> (qemu-arm+0x600686fb)
> #4   (libtsan.so.0+0x000230d9)
> 
>   Previous write of size 8 at 0x7db4000261f0 by main thread (mutexes: write 
> M8):
> #0 cpu_list_add /home/alex/lsrc/qemu/qemu.git/cpus-common.c:87 
> (qemu-arm+0x60115b7a)
> #1 cpu_exec_init /home/alex/lsrc/qemu/qemu.git/exec.c:641 
> (qemu-arm+0x60009900)
> #2 arm_cpu_initfn /home/alex/lsrc/qemu/qemu.git/target-arm/cpu.c:447 
> (qemu-arm+0x600f833b)
[..]

Nice! Which patch fixes this--patch 10? It would be cool to have this
report in the corresponding commit message.

Thanks,

Emilio

Re: [Qemu-devel] [PATCH] raw-posix: add 'offset' and 'size' options

[Eric Blake]

On 10/02/2016 02:13 PM, Tomáš Golembiovský wrote:
> Added two new options 'offset' and 'size'. This makes it possible to use
> only part of the file as a device. This can be used e.g. to limit the
> access only to single partition in a disk image or use a disk inside a
> tar archive (like OVA).
> 
> For now this is only possible for files in read-only mode. It should be
> possible to extend it later to allow read-write mode, but would probably
> require that the size of the device is kept constant (i.e. no resizing).
> 
> Signed-off-by: Tomáš Golembiovský 
> ---
>  block/raw-posix.c | 97 
> +++
>  1 file changed, 91 insertions(+), 6 deletions(-)
> 

> +s->offset = qemu_opt_get_size(opts, "offset", 0);
> +s->assumed_size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
> +
> +if (((bs->drv != _file) || !bs->read_only) &&
> +((s->offset > 0) || (s->assumed_size > 0))) {

Over-parenthesized.

> @@ -443,6 +468,23 @@ static int raw_open_common(BlockDriverState *bs, QDict 
> *options,
>  }
>  s->fd = fd;
>  
> +/* Check size and offset */
> +real_size = raw_getlength_real(bs);
> +if (real_size < (s->offset + s->assumed_size)) {
> +if (s->assumed_size == 0) {
> +error_setg(errp, "The offset has to be smaller than actual size "
> + "of the containing file (%ld) ",
> + real_size);

Won't compile on 32-bit platforms.  %ld is not necessarily synonymous
with int64_t; you need to use PRId64 instead.  Multiple times through
your patch.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro

[Paolo Bonzini]

On 03/10/2016 15:34, Halil Pasic wrote:
> Hi Paolo,
> 
> I'm sorry, but I do not get it quite yet, or more exactly I have the
> feeling I did not manage to bring my point over. So I will try with
> more details.
> 
> On 10/03/2016 01:29 PM, Paolo Bonzini wrote:
>>
>>
>> On 03/10/2016 12:36, Halil Pasic wrote:
> #define VMSTATE_PCI_DEVICE(_field, _state) {
> 
> This was probably supposed to be VMSTATE_VIRTIO_DEVICE.

Yes.

> .name   = (stringify(_field)), \
> .size   = sizeof(VirtIODevice),\
> .vmsd   = _virtio_device,  \
> 
> This one does not exist at least very tricky to write because of the 
> peculiarities
> of virtio migration. This one would need to migrate the transport stuff too. 
> And
> the specialized device to, which is not normal.

This is my own typo - this should be .info.  Sorry.

>> Regarding VMSTATE_VIRTIO_FIELD, it's just a matter of not doing things
>> differently for no particular reason.  Your macro is already doing
>> exactly the same as other VMSTATE_* macros, just with different
>> conventions for the arguments.  I don't see any advantage in changing that.
> 
> In my opinion it is not the same. In the case of VMSTATE_PCI_DEVICE we have
> (a self contained) parent (in sense of inheritance) device, which is embedded
> as _field in the specialized device and is migrated by the vmstatedescription
> of the parent. The rest of the specialized devices state is represented by
> the other fields.
> 
> VMSTATE_VIRTIO_FIELD is however just there to make sure virtio_load and
> virtio_save are called at the right time with the right arguments. The 
> specialized
> device is then migrated by the save/load callbacks of the device class, or
> the vmsd residing in the device class. VMSTATE_VIRTIO_FIELD is supposed
> to be the only field, if the virtio device adheres to the virtio-migration
> document. VMSTATE_VIRTIO_FIELD has no arguments because it is
> a virtual field and does not depend on the offset stuff.
> 
> To summarize currently I have no idea how to write up the vmstate
> field definition macro VMSTATE_VIRTIO_DEVICE so that it meets your
> expectations. 

As above.

>> Having everything hidden behind a magic macro makes
>> things harder to follow than other vmstate definitions.  
> 
> Again in my opinion the virtio migration is different that the rest of the
> vmstate migration stuff, and it's ugly to some extent. So the idea was
> to make it look different and hide the ugliness behind the macro. 
> If you insist i will create a version where the macros are expanded so
> it's easier to say if this improves or worsens the readability.

I agree it is not exactly the same as the other devices.  But in my
opinion it's not different-enough to do everything completely more, and
in the future we should aim at making it less different.

> I think this is matter of taste, and your taste matters more ;). I do 
> agree that the variadic for the massaging functions is more complicated
> that the two function pointers taken by VMSTATE_VIRTIO_DEVICE. My idea
> was that we end up with more readable code on the caller-side, but if you
> prefer function pointers and NULLs if no callback is needed needed 
> (most cases), I can live with that.

Well, the third possibility would be expanding the VMStateDescription
definition, :) where .post_load becomes just yet another initializer.

Paolo

Re: [Qemu-devel] [PATCH 2/6] nbd: set name for all I/O channels created

[Stefan Hajnoczi]

On Fri, Sep 30, 2016 at 04:16:56PM +0100, Daniel P. Berrange wrote:
> Ensure that all I/O channels created for NBD are given names
> to distinguish their respective roles.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  block/nbd.c| 1 +
>  blockdev-nbd.c | 3 +++
>  nbd/client.c   | 1 +
>  nbd/server.c   | 1 +
>  4 files changed, 6 insertions(+)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 4/6] migration: set name for all I/O channels created

[Stefan Hajnoczi]

On Fri, Sep 30, 2016 at 04:16:58PM +0100, Daniel P. Berrange wrote:
> Ensure that all I/O channels created for migration are given names
> to distinguish their respective roles.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  migration/exec.c  | 2 ++
>  migration/fd.c| 2 ++
>  migration/migration.c | 1 +
>  migration/savevm.c| 3 +++
>  migration/socket.c| 5 +
>  migration/tls.c   | 2 ++
>  6 files changed, 15 insertions(+)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 5/6] vnc: set name for all I/O channels created

[Stefan Hajnoczi]

On Fri, Sep 30, 2016 at 04:16:59PM +0100, Daniel P. Berrange wrote:
> Ensure that all I/O channels created for VNC are given names
> to distinguish their respective roles.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  ui/vnc-auth-vencrypt.c | 1 +
>  ui/vnc-ws.c| 3 +++
>  ui/vnc.c   | 7 +++
>  3 files changed, 11 insertions(+)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 6/6] main: set names for main loop sources created

[Stefan Hajnoczi]

On Fri, Sep 30, 2016 at 04:17:00PM +0100, Daniel P. Berrange wrote:
> The main loop creates two generic sources for the AIO
> and IO handler systems.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  main-loop.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 3/6] char: set name for all I/O channels created

[Stefan Hajnoczi]

On Fri, Sep 30, 2016 at 04:16:57PM +0100, Daniel P. Berrange wrote:
> Ensure that all I/O channels created for character devices
> are given names to distinguish their respective roles.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  qemu-char.c | 77 
> +++--
>  1 file changed, 70 insertions(+), 7 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH] spapr: fix check of cpu alias name in spapr_get_cpu_core_type()

[Bharata B Rao]

On Mon, Oct 03, 2016 at 02:13:20PM +0200, Greg Kurz wrote:
> If the user passes an alias name and a property to -cpu, QEMU fails to
> find the CPU definition and exits.
> 
> $ qemu-system-ppc64 -cpu POWER8E,compat=power7
> qemu-system-ppc64: Unable to find sPAPR CPU Core definition
> 
> This happens because spapr_get_cpu_core_type() passes the full string from
> the command line (i.e. "POWER8E,compat=power7") to ppc_cpu_lookup_alias(),
> instead of the alias name piece only (i.e. "POWER8E").
> 
> The fix is to pass model_pieces[0] to ppc_cpu_lookup_alias().
> 
> Signed-off-by: Greg Kurz 

Reviewed-by: Bharata B Rao 

> ---
>  hw/ppc/spapr_cpu_core.c |8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 6f0533c34259..35d1873b9ff3 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -92,20 +92,20 @@ char *spapr_get_cpu_core_type(const char *model)
>  gchar **model_pieces = g_strsplit(model, ",", 2);
> 
>  core_type = g_strdup_printf("%s-%s", model_pieces[0], 
> TYPE_SPAPR_CPU_CORE);
> -g_strfreev(model_pieces);
> 
>  /* Check whether it exists or whether we have to look up an alias name */
>  if (!object_class_by_name(core_type)) {
>  const char *realmodel;
> 
>  g_free(core_type);
> -realmodel = ppc_cpu_lookup_alias(model);
> +core_type = NULL;
> +realmodel = ppc_cpu_lookup_alias(model_pieces[0]);
>  if (realmodel) {
> -return spapr_get_cpu_core_type(realmodel);
> +core_type = spapr_get_cpu_core_type(realmodel);
>  }
> -return NULL;
>  }
> 
> +g_strfreev(model_pieces);
>  return core_type;
>  }
> 

Re: [Qemu-devel] [PATCH 1/2] qemu-nbd: Shrink image size by specified offset

[Eric Blake]

On 10/03/2016 08:50 AM, Tomáš Golembiovský wrote:
>> Additional context:
>>
>> off_t dev_offset = 0;
>>
>> off_t fd_size;
>>
>>>  
>>> +if (dev_offset >= fd_size) {
>>> +error_report("Offset (%lu) has to be smaller than the image size 
>>> (%lu)",
>>> + dev_offset, fd_size);  
>>
>> Whoops, this fails to compile on 32-bit platforms.  %lu is not
>> necessarily synonymous with off_t values.
> 
> After some digging I figured off_t is in fact signed type. That makes
> the formatting wrong everywhere. Unfortunately I didn't find any good
> definition of the type. Any suggestion what format flag should I use? Or
> should I just use a temporary variable of known size for that?

Easiest is probably casting to a type with an easier format flag, as in
either of:

error_report("offset %lld ...", (long long) dev_offset)
error_report("offset %jd ...", (intmax_t) dev_offset)

off_t is particularly problematic because there is no magic % sequence
reserved for it, nothing in  for it, and there are 32-bit
compilation environments where it is still 32 bits (although qemu
prefers to explicitly request large-file compilation so that off_t is
always 64 bits)

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v5 00/20] Refactor trace to allow modular build

[Stefan Hajnoczi]

On Wed, Sep 28, 2016 at 02:08:03PM +0100, Daniel P. Berrange wrote:
> These patches were previously posted as part of my giant
> trace events modular build series
> 
>   v1: https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01714.html
>   v2: https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg03335.html
>   v3: https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg04282.html
>   v4: https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg05467.html
> 
> This series does all the refactoring required to support a fully
> modular build of the trace probe points, but does not actually
> convert anything to use it. The 40+ patches to convert each subdir
> to use modular build will only be posted again, once the refactoring
> is reviewed & queued, in order to avoid patch-bombing the list
> more than is needed. The full series is visible at
> 
>https://github.com/berrange/qemu/tree/trace-events-3
> 
> The key problem being tackled by this series is the assumption
> that there is a single statically declared enum which provides
> globally unique event IDs. Inside QEMU the event IDs were used
> as list indexes into the 'trace_events' array, while the event
> VCPU IDs were used as bitmap indexes in trace_dstate against
> the CPUState struct.  Externally to QEMU, the event IDs were
> also written in the simpletrace binary data format and used
> to lookup the entry in the trace-events file afterwards.
> 
> Inside QEMU the refactoring work managed to remove all need
> for event IDs for purposes of 'trace_events' array lookups.
> Instead we now have global variables per-event which can be
> referenced directly.
> 
> When QEMU starts up and the various event groups are registered,
> we now dynamically assign event IDs and VCPU IDs to each event.
> This removes the limitation in the v1 posting that all vCPU
> events had to be in one file. We also removed the limitation
> on the total number of vCPU events. So there is no regression
> in functionality of VCPU event support compared to current
> GIT master.
> Since the event IDs are allocated dynamically at runtime,
> the simpletrace.py script cannot assume they map directly
> to the 'trace-events' file entries. Thus, the simpletrace
> binary format is extended to include a record type that
> maps trace event IDs to trace event names. While it would
> be possible to take this even further and make the
> simpletrace binary format 100% self-describing this is left
> as an exercise for future developers, as it is not a
> pre-requisite for this modular build.
> 
> While some of the intermediate patches may seem pointless
> on their own, they exist in order to facilitate the review
> of later patches by ensuring each patch does the minimum
> possible refactoring work.
> 
> Changed in v5:
> 
>  - Use single '_' instead of double/triple '_' in
>constants (Lluís)
>  - Use more pythonic loop iterator (Lluís)
>  - Misc typos (Lluís/Stefan)
>  - Fix filtering of QMP trace events (Lluís)
>  - Fix some new mistakes in trace-events in master

Great, this looks very close.  There are only a few comments from Lluís
and me.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v5 20/20] trace: introduce a formal group name for trace events

[Stefan Hajnoczi]

On Wed, Sep 28, 2016 at 02:08:23PM +0100, Daniel P. Berrange wrote:
> The declarations in the generated-tracers.h file are
> assuming there's only ever going to be one instance
> of this header, as they are not namespaced. When we
> have one header per event group, if a single source
> file needs to include multiple sets of trace events,
> the symbols will all clash.
> 
> This change thus introduces a '--group NAME' arg to the
> 'tracetool' program. This will cause all the symbols in
> the generated header files to be given a unique namespace.
> 
> If no group is given, the group name 'common' is used,
> which is suitable for the current usage where there is
> only one global trace-events file used for code generation.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  scripts/tracetool.py | 15 ++-
>  scripts/tracetool/__init__.py|  6 --
>  scripts/tracetool/backend/__init__.py| 12 ++--
>  scripts/tracetool/backend/dtrace.py  |  4 ++--
>  scripts/tracetool/backend/ftrace.py  |  5 +++--
>  scripts/tracetool/backend/log.py |  4 ++--
>  scripts/tracetool/backend/simple.py  |  8 
>  scripts/tracetool/backend/syslog.py  |  4 ++--
>  scripts/tracetool/backend/ust.py |  4 ++--
>  scripts/tracetool/format/__init__.py |  4 ++--
>  scripts/tracetool/format/c.py| 18 ++
>  scripts/tracetool/format/d.py|  2 +-
>  scripts/tracetool/format/h.py| 14 +++---
>  scripts/tracetool/format/simpletrace_stap.py |  2 +-
>  scripts/tracetool/format/stap.py |  2 +-
>  scripts/tracetool/format/tcg_h.py|  2 +-
>  scripts/tracetool/format/tcg_helper_c.py |  2 +-
>  scripts/tracetool/format/tcg_helper_h.py |  2 +-
>  scripts/tracetool/format/tcg_helper_wrapper_h.py |  2 +-
>  scripts/tracetool/format/ust_events_c.py |  2 +-
>  scripts/tracetool/format/ust_events_h.py |  9 +
>  21 files changed, 71 insertions(+), 52 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v5 19/20] trace: pass trace-events to tracetool as a positional param

[Stefan Hajnoczi]

On Wed, Sep 28, 2016 at 02:08:22PM +0100, Daniel P. Berrange wrote:
> Instead of reading the contents of 'trace-events' from stdin,
> accept the filename as a positional parameter. This also
> allows for reading from multiple files, though this facility
> is not used at this time.
> 
> Reviewed-by: Lluís Vilanova 
> Signed-off-by: Daniel P. Berrange 
> ---
>  Makefile.target  |  6 +++---
>  scripts/tracetool.py |  5 -
>  trace/Makefile.objs  | 18 +-
>  3 files changed, 16 insertions(+), 13 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v5 18/20] trace: push reading of events up a level to tracetool main

[Stefan Hajnoczi]

On Wed, Sep 28, 2016 at 02:08:21PM +0100, Daniel P. Berrange wrote:
> Move the reading of events out of the 'tracetool.generate'
> method and into tracetool.main, so that the latter is not
> tied to generating from a single source of events.
> 
> Reviewed-by: Lluís Vilanova 
> Signed-off-by: Daniel P. Berrange 
> ---
>  scripts/tracetool.py  | 4 +++-
>  scripts/tracetool/__init__.py | 8 +++-
>  2 files changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v5 17/20] trace: rename _read_events to read_events

[Stefan Hajnoczi]

On Wed, Sep 28, 2016 at 02:08:20PM +0100, Daniel P. Berrange wrote:
> The _read_events method is used by callers outside of
> its module, so should be a public method, not private.
> 
> Reviewed-by: Lluís Vilanova 
> Signed-off-by: Daniel P. Berrange 
> ---
>  scripts/simpletrace.py|  6 +++---
>  scripts/tracetool/__init__.py | 14 --
>  2 files changed, 15 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v5 16/20] trace: get rid of generated-events.h/generated-events.c

[Stefan Hajnoczi]

On Wed, Sep 28, 2016 at 02:08:19PM +0100, Daniel P. Berrange wrote:
> Currently the generated-events.[ch] files contain the
> event dstates, constants and TraceEvent structs, while the
> generated-tracers.[ch] files contain the actual trace
> probe logic. With the removal of usage of the event enums
> from the API there is no longer any compelling reason for
> the separation between these files. The generated-events.h
> content is only ever needed from the generated-tracers.[ch]
> files.
> 
> The enums/constants/structs from generated-events.[ch] are
> thus moved into the generated-tracers.[ch], so that there
> is one less file to be generated.
> 
> Reviewed-by: Lluís Vilanova 
> Signed-off-by: Daniel P. Berrange 
> ---
>  Makefile |  3 --
>  include/trace-tcg.h  |  1 -
>  include/trace.h  |  1 -
>  scripts/tracetool/format/c.py| 50 ++---
>  scripts/tracetool/format/events_c.py | 62 
> 
>  scripts/tracetool/format/events_h.py | 49 
>  scripts/tracetool/format/h.py| 20 
>  trace/Makefile.objs  | 28 +++-
>  trace/control.h  |  2 +-
>  trace/simple.h   |  4 ---
>  10 files changed, 70 insertions(+), 150 deletions(-)
>  delete mode 100644 scripts/tracetool/format/events_c.py
>  delete mode 100644 scripts/tracetool/format/events_h.py

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v5 15/20] trace: dynamically allocate event IDs at runtime

[Stefan Hajnoczi]

On Wed, Sep 28, 2016 at 02:08:18PM +0100, Daniel P. Berrange wrote:
> Instead of having the code generator assign event IDs and
> event VCPU IDs, assign them when the events are registered
> at runtime. This will allow code to be generated from
> individual trace-events without having to figure out
> globally unique numbering at build time.
> 
> Reviewed-by: Lluís Vilanova 
> Signed-off-by: Daniel P. Berrange 
> ---
>  scripts/tracetool/format/events_c.py | 10 ++
>  scripts/tracetool/format/events_h.py |  4 
>  trace/control.c  | 11 ++-
>  3 files changed, 12 insertions(+), 13 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 1/2] vhost: enable any layout feature

[Maxime Coquelin]

On 09/29/2016 10:21 PM, Michael S. Tsirkin wrote:

On Thu, Sep 29, 2016 at 10:05:22PM +0200, Maxime Coquelin wrote:

>
>
> On 09/29/2016 07:57 PM, Michael S. Tsirkin wrote:

> > On Thu, Sep 29, 2016 at 05:30:53PM +0200, Maxime Coquelin wrote:

> ...

> > >
> > > Before enabling anything by default, we should first optimize the 1 slot
> > > case. Indeed, micro-benchmark using testpmd in txonly[0] shows ~17%
> > > perf regression for 64 bytes case:
> > >  - 2 descs per packet: 11.6Mpps
> > >  - 1 desc per packet: 9.6Mpps
> > >
> > > This is due to the virtio header clearing in virtqueue_enqueue_xmit().
> > > Removing it, we get better results than with 2 descs (1.20Mpps).
> > > Since the Virtio PMD doesn't support offloads, I wonder whether we can
> > > just drop the memset?

> >
> > What will happen? Will the header be uninitialized?

> Yes..
> I didn't look closely at the spec, but just looked at DPDK's and Linux
> vhost implementations. IIUC, the header is just skipped in the two
> implementations.

In linux guest skbs are initialized AFAIK. See virtio_net_hdr_from_skb
first thing it does is
memset(hdr, 0, sizeof(*hdr));




> >
> > The spec says:
> >   The driver can send a completely checksummed packet. In this case, 
flags
> >   will be zero, and gso_type
> >   will be VIRTIO_NET_HDR_GSO_NONE.
> >
> > and
> >   The driver MUST set num_buffers to zero.
> >   If VIRTIO_NET_F_CSUM is not negotiated, the driver MUST set flags to
> >   zero and SHOULD supply a fully
> >   checksummed packet to the device.
> >
> > and
> >   If none of the VIRTIO_NET_F_HOST_TSO4, TSO6 or UFO options have been
> >   negotiated, the driver MUST
> >   set gso_type to VIRTIO_NET_HDR_GSO_NONE.
> >
> > so doing this unconditionally would be a spec violation, but if you see
> > value in this, we can add a feature bit.

> Right it would be a spec violation, so it should be done conditionally.
> If a feature bit is to be added, what about VIRTIO_NET_F_NO_TX_HEADER?
> It would imply VIRTIO_NET_F_CSUM not set, and no GSO features set.
> If negotiated, we wouldn't need to prepend a header.

Yes but two points.

1. why is this memset expensive? Is the test completely skipping looking
   at the packet otherwise?

2. As long as we are doing this, see
Alignment vs. Networking

in Documentation/unaligned-memory-access.txt


This change will not have an impact on the IP header alignment,
as is offset in the mbuf will not change.

Regards,
Maxime

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 1/6] libqos: add PPC64 PCI support

[Cédric Le Goater]

On 10/03/2016 04:03 PM, Greg Kurz wrote:
> On Mon, 3 Oct 2016 13:23:27 +0200
> Cédric Le Goater  wrote:
> 
>> On 09/29/2016 07:27 AM, David Gibson wrote:
>>> On Wed, Sep 28, 2016 at 08:51:28PM +0200, Laurent Vivier wrote:  
 Signed-off-by: Laurent Vivier 
 ---
  tests/Makefile.include   |   1 +
  tests/libqos/pci-pc.c|  22 
  tests/libqos/pci-spapr.c | 280 
 +++
  tests/libqos/pci-spapr.h |  17 +++
  tests/libqos/pci.c   |  22 +++-
  tests/libqos/rtas.c  |  45 
  tests/libqos/rtas.h  |   4 +
  7 files changed, 368 insertions(+), 23 deletions(-)
  create mode 100644 tests/libqos/pci-spapr.c
  create mode 100644 tests/libqos/pci-spapr.h

 diff --git a/tests/Makefile.include b/tests/Makefile.include
 index 8162f6f..92c82d8 100644
 --- a/tests/Makefile.include
 +++ b/tests/Makefile.include
 @@ -590,6 +590,7 @@ libqos-obj-y += tests/libqos/i2c.o 
 tests/libqos/libqos.o
  libqos-spapr-obj-y = $(libqos-obj-y) tests/libqos/malloc-spapr.o
  libqos-spapr-obj-y += tests/libqos/libqos-spapr.o
  libqos-spapr-obj-y += tests/libqos/rtas.o
 +libqos-spapr-obj-y += tests/libqos/pci-spapr.o
  libqos-pc-obj-y = $(libqos-obj-y) tests/libqos/pci-pc.o
  libqos-pc-obj-y += tests/libqos/malloc-pc.o tests/libqos/libqos-pc.o
  libqos-pc-obj-y += tests/libqos/ahci.o
 diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
 index 1ae2d37..82066b8 100644
 --- a/tests/libqos/pci-pc.c
 +++ b/tests/libqos/pci-pc.c
 @@ -255,28 +255,6 @@ void qpci_free_pc(QPCIBus *bus)
  g_free(s);
  }
  
 -void qpci_plug_device_test(const char *driver, const char *id,
 -   uint8_t slot, const char *opts)
 -{
 -QDict *response;
 -char *cmd;
 -
 -cmd = g_strdup_printf("{'execute': 'device_add',"
 -  " 'arguments': {"
 -  "   'driver': '%s',"
 -  "   'addr': '%d',"
 -  "   %s%s"
 -  "   'id': '%s'"
 -  "}}", driver, slot,
 -  opts ? opts : "", opts ? "," : "",
 -  id);
 -response = qmp(cmd);
 -g_free(cmd);
 -g_assert(response);
 -g_assert(!qdict_haskey(response, "error"));
 -QDECREF(response);
 -}
 -
  void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
  {
  QDict *response;
 diff --git a/tests/libqos/pci-spapr.c b/tests/libqos/pci-spapr.c
 new file mode 100644
 index 000..78df823
 --- /dev/null
 +++ b/tests/libqos/pci-spapr.c
 @@ -0,0 +1,280 @@
 +/*
 + * libqos PCI bindings for SPAPR
 + *
 + * 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 "libqtest.h"
 +#include "libqos/pci-spapr.h"
 +#include "libqos/rtas.h"
 +
 +#include "hw/pci/pci_regs.h"
 +
 +#include "qemu-common.h"
 +#include "qemu/host-utils.h"
 +
 +
 +/* From include/hw/pci-host/spapr.h */
 +
 +#define SPAPR_PCI_BASE_BUID  0x8002000ULL
 +
 +#define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x8000ULL
 +
 +#define SPAPR_PCI_WINDOW_BASE0x100ULL
 +#define SPAPR_PCI_WINDOW_SPACING 0x10ULL
 +#define SPAPR_PCI_MMIO_WIN_OFF   0xA000
 +#define SPAPR_PCI_MMIO_WIN_SIZE  (SPAPR_PCI_WINDOW_SPACING - \
 + SPAPR_PCI_MEM_WIN_BUS_OFFSET)
 +#define SPAPR_PCI_IO_WIN_OFF 0x8000
 +#define SPAPR_PCI_IO_WIN_SIZE0x1
 +
 +/* index is the phb index */
 +
 +#define BUIDBASE(index)  (SPAPR_PCI_BASE_BUID + (index))
 +#define PCIBASE(index)   (SPAPR_PCI_WINDOW_BASE + \
 +  (index) * SPAPR_PCI_WINDOW_SPACING)
 +#define IOBASE(index)(PCIBASE(index) + 
 SPAPR_PCI_IO_WIN_OFF)
 +#define MMIOBASE(index)  (PCIBASE(index) + 
 SPAPR_PCI_MMIO_WIN_OFF)
 +
 +typedef struct QPCIBusSPAPR {
 +QPCIBus bus;
 +QGuestAllocator *alloc;
 +
 +uint64_t pci_hole_start;
 +uint64_t pci_hole_size;
 +uint64_t pci_hole_alloc;
 +
 +uint32_t pci_iohole_start;
 +uint32_t pci_iohole_size;
 +uint32_t pci_iohole_alloc;
 +} QPCIBusSPAPR;
 +
 +static uint8_t qpci_spapr_io_readb(QPCIBus *bus, void *addr)
 +{
 +uint64_t port = (uint64_t)addr;
 +uint8_t v;
 +if (port < SPAPR_PCI_IO_WIN_SIZE) {
 +v = readb(IOBASE(0) + 

Re: [Qemu-devel] [PATCH v7 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

[Stefan Hajnoczi]

On Sat, Oct 1, 2016 at 4:10 AM, ashish mittal  wrote:
>> If you make these changes then all multi-threading in libqnio and the
>> QEMU block driver can be dropped.  There will be less code and it will
>> be simpler.

You'll get a lot of basic tests for free if you add vxhs support to
the existing tests/qemu-iotests/ framework.

A vxhs server is required so qemu-iotests has something to run
against.  I saw some server code in libqnio but haven't investigated
how complete it is.  The main thing is to support read/write/flush.

Stefan

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 1/6] libqos: add PPC64 PCI support

[Greg Kurz]

On Mon, 3 Oct 2016 13:23:27 +0200
Cédric Le Goater  wrote:

> On 09/29/2016 07:27 AM, David Gibson wrote:
> > On Wed, Sep 28, 2016 at 08:51:28PM +0200, Laurent Vivier wrote:  
> >> Signed-off-by: Laurent Vivier 
> >> ---
> >>  tests/Makefile.include   |   1 +
> >>  tests/libqos/pci-pc.c|  22 
> >>  tests/libqos/pci-spapr.c | 280 
> >> +++
> >>  tests/libqos/pci-spapr.h |  17 +++
> >>  tests/libqos/pci.c   |  22 +++-
> >>  tests/libqos/rtas.c  |  45 
> >>  tests/libqos/rtas.h  |   4 +
> >>  7 files changed, 368 insertions(+), 23 deletions(-)
> >>  create mode 100644 tests/libqos/pci-spapr.c
> >>  create mode 100644 tests/libqos/pci-spapr.h
> >>
> >> diff --git a/tests/Makefile.include b/tests/Makefile.include
> >> index 8162f6f..92c82d8 100644
> >> --- a/tests/Makefile.include
> >> +++ b/tests/Makefile.include
> >> @@ -590,6 +590,7 @@ libqos-obj-y += tests/libqos/i2c.o 
> >> tests/libqos/libqos.o
> >>  libqos-spapr-obj-y = $(libqos-obj-y) tests/libqos/malloc-spapr.o
> >>  libqos-spapr-obj-y += tests/libqos/libqos-spapr.o
> >>  libqos-spapr-obj-y += tests/libqos/rtas.o
> >> +libqos-spapr-obj-y += tests/libqos/pci-spapr.o
> >>  libqos-pc-obj-y = $(libqos-obj-y) tests/libqos/pci-pc.o
> >>  libqos-pc-obj-y += tests/libqos/malloc-pc.o tests/libqos/libqos-pc.o
> >>  libqos-pc-obj-y += tests/libqos/ahci.o
> >> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> >> index 1ae2d37..82066b8 100644
> >> --- a/tests/libqos/pci-pc.c
> >> +++ b/tests/libqos/pci-pc.c
> >> @@ -255,28 +255,6 @@ void qpci_free_pc(QPCIBus *bus)
> >>  g_free(s);
> >>  }
> >>  
> >> -void qpci_plug_device_test(const char *driver, const char *id,
> >> -   uint8_t slot, const char *opts)
> >> -{
> >> -QDict *response;
> >> -char *cmd;
> >> -
> >> -cmd = g_strdup_printf("{'execute': 'device_add',"
> >> -  " 'arguments': {"
> >> -  "   'driver': '%s',"
> >> -  "   'addr': '%d',"
> >> -  "   %s%s"
> >> -  "   'id': '%s'"
> >> -  "}}", driver, slot,
> >> -  opts ? opts : "", opts ? "," : "",
> >> -  id);
> >> -response = qmp(cmd);
> >> -g_free(cmd);
> >> -g_assert(response);
> >> -g_assert(!qdict_haskey(response, "error"));
> >> -QDECREF(response);
> >> -}
> >> -
> >>  void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
> >>  {
> >>  QDict *response;
> >> diff --git a/tests/libqos/pci-spapr.c b/tests/libqos/pci-spapr.c
> >> new file mode 100644
> >> index 000..78df823
> >> --- /dev/null
> >> +++ b/tests/libqos/pci-spapr.c
> >> @@ -0,0 +1,280 @@
> >> +/*
> >> + * libqos PCI bindings for SPAPR
> >> + *
> >> + * 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 "libqtest.h"
> >> +#include "libqos/pci-spapr.h"
> >> +#include "libqos/rtas.h"
> >> +
> >> +#include "hw/pci/pci_regs.h"
> >> +
> >> +#include "qemu-common.h"
> >> +#include "qemu/host-utils.h"
> >> +
> >> +
> >> +/* From include/hw/pci-host/spapr.h */
> >> +
> >> +#define SPAPR_PCI_BASE_BUID  0x8002000ULL
> >> +
> >> +#define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x8000ULL
> >> +
> >> +#define SPAPR_PCI_WINDOW_BASE0x100ULL
> >> +#define SPAPR_PCI_WINDOW_SPACING 0x10ULL
> >> +#define SPAPR_PCI_MMIO_WIN_OFF   0xA000
> >> +#define SPAPR_PCI_MMIO_WIN_SIZE  (SPAPR_PCI_WINDOW_SPACING - \
> >> + SPAPR_PCI_MEM_WIN_BUS_OFFSET)
> >> +#define SPAPR_PCI_IO_WIN_OFF 0x8000
> >> +#define SPAPR_PCI_IO_WIN_SIZE0x1
> >> +
> >> +/* index is the phb index */
> >> +
> >> +#define BUIDBASE(index)  (SPAPR_PCI_BASE_BUID + (index))
> >> +#define PCIBASE(index)   (SPAPR_PCI_WINDOW_BASE + \
> >> +  (index) * SPAPR_PCI_WINDOW_SPACING)
> >> +#define IOBASE(index)(PCIBASE(index) + 
> >> SPAPR_PCI_IO_WIN_OFF)
> >> +#define MMIOBASE(index)  (PCIBASE(index) + 
> >> SPAPR_PCI_MMIO_WIN_OFF)
> >> +
> >> +typedef struct QPCIBusSPAPR {
> >> +QPCIBus bus;
> >> +QGuestAllocator *alloc;
> >> +
> >> +uint64_t pci_hole_start;
> >> +uint64_t pci_hole_size;
> >> +uint64_t pci_hole_alloc;
> >> +
> >> +uint32_t pci_iohole_start;
> >> +uint32_t pci_iohole_size;
> >> +uint32_t pci_iohole_alloc;
> >> +} QPCIBusSPAPR;
> >> +
> >> +static uint8_t qpci_spapr_io_readb(QPCIBus *bus, void *addr)
> >> +{
> >> +uint64_t port = (uint64_t)addr;
> >> +uint8_t v;
> >> +if (port < SPAPR_PCI_IO_WIN_SIZE) {
> >> +v = readb(IOBASE(0) + port);
> >> +} else {
> >> +v = 

Re: [Qemu-devel] [PATCH v5 14/20] trace: dynamically allocate trace_dstate in CPUState

[Stefan Hajnoczi]

On Wed, Sep 28, 2016 at 02:08:17PM +0100, Daniel P. Berrange wrote:
> The CPUState struct has a bitmap tracking which VCPU
> events are currently active. This is indexed based on
> the event ID values, and sized according the maximum
> TraceEventVCPUID enum value.
> 
> When we start dynamically assigning IDs at runtime,
> we can't statically declare a bitmap without making
> an assumption about the max event count. This problem
> can be solved by dynamically allocating the per-CPU
> dstate bitmap.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  include/qom/cpu.h | 9 ++---
>  qom/cpu.c | 8 ++--
>  trace/control.c   | 5 +
>  trace/control.h   | 7 +++
>  4 files changed, 24 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v5 13/20] trace: provide mechanism for registering trace events

[Stefan Hajnoczi]

On Wed, Sep 28, 2016 at 02:08:16PM +0100, Daniel P. Berrange wrote:
> Remove the notion of there being a single global array
> of trace events, by introducing a method for registering
> groups of events.
> 
> The module_call_init() needs to be invoked at the start
> of any program that wants to make use of the trace
> support. Currently this covers system emulators qemu-nbd,
> qemu-img and qemu-io.
> 
> Reviewed-by: Lluís Vilanova 
> Signed-off-by: Daniel P. Berrange 
> ---
>  include/qemu/module.h|  2 ++
>  qemu-img.c   |  1 +
>  qemu-io.c|  1 +
>  qemu-nbd.c   |  1 +
>  scripts/tracetool/format/events_c.py |  6 ++
>  trace/control-internal.h |  4 +++-
>  trace/control.c  | 25 +++--
>  trace/control.h  |  1 +
>  vl.c |  2 ++
>  9 files changed, 40 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH v4 1/1] qga: minimal support for fstrim for Windows guests

[Denis V. Lunev]

Unfortunately, there is no public Windows API to start trimming the
filesystem. The only viable way here is to call 'defrag.exe /L' for
each volume.

This is working since Win8 and Win2k12.

Signed-off-by: Denis V. Lunev 
Signed-off-by: Denis Plotnikov 
CC: Michael Roth 
CC: Stefan Weil 
CC: Marc-André Lureau 
---
 qga/commands-win32.c | 97 ++--
 1 file changed, 94 insertions(+), 3 deletions(-)

Changes from v3:
- fixed memory leak on error path for FindFirstVolumeW
- replaced g_malloc0 with g_malloc for uc_path. g_malloc is better as we are
  allocating string, not an object

Changes from v1, v2:
- next attempt to fix error handling on error in FindFirstVolumeW

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 9c9be12..cebf4cc 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -840,8 +840,99 @@ static void guest_fsfreeze_cleanup(void)
 GuestFilesystemTrimResponse *
 qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
 {
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
+GuestFilesystemTrimResponse *resp;
+HANDLE handle;
+WCHAR guid[MAX_PATH] = L"";
+
+handle = FindFirstVolumeW(guid, ARRAYSIZE(guid));
+if (handle == INVALID_HANDLE_VALUE) {
+error_setg_win32(errp, GetLastError(), "failed to find any volume");
+return NULL;
+}
+
+resp = g_new0(GuestFilesystemTrimResponse, 1);
+
+do {
+GuestFilesystemTrimResult *res;
+GuestFilesystemTrimResultList *list;
+PWCHAR uc_path;
+DWORD char_count = 0;
+char *path, *out;
+GError *gerr = NULL;
+gchar * argv[4];
+
+GetVolumePathNamesForVolumeNameW(guid, NULL, 0, _count);
+
+if (GetLastError() != ERROR_MORE_DATA) {
+continue;
+}
+if (GetDriveTypeW(guid) != DRIVE_FIXED) {
+continue;
+}
+
+uc_path = g_malloc(sizeof(WCHAR) * char_count);
+if (!GetVolumePathNamesForVolumeNameW(guid, uc_path, char_count,
+  _count) || !*uc_path) {
+/* strange, but this condition could be faced even with size == 2 
*/
+g_free(uc_path);
+continue;
+}
+
+res = g_new0(GuestFilesystemTrimResult, 1);
+
+path = g_utf16_to_utf8(uc_path, char_count, NULL, NULL, );
+
+g_free(uc_path);
+
+if (gerr != NULL && gerr->code) {
+res->has_error = true;
+res->error = g_strdup(gerr->message);
+g_error_free(gerr);
+break;
+}
+
+res->path = path;
+
+list = g_new0(GuestFilesystemTrimResultList, 1);
+list->value = res;
+list->next = resp->paths;
+
+resp->paths = list;
+
+memset(argv, 0, sizeof(argv));
+argv[0] = (gchar *)"defrag.exe";
+argv[1] = (gchar *)"/L";
+argv[2] = path;
+
+if (!g_spawn_sync(NULL, argv, NULL, G_SPAWN_SEARCH_PATH, NULL, NULL,
+   /* stdout */, NULL /* stdin */,
+  NULL, )) {
+res->has_error = true;
+res->error = g_strdup(gerr->message);
+g_error_free(gerr);
+} else {
+/* defrag.exe is UGLY. Exit code is ALWAYS zero.
+   Error is reported in the output with something like
+   (x8920) etc code in the stdout */
+
+int i;
+gchar **lines = g_strsplit(out, "\r\n", 0);
+g_free(out);
+
+for (i = 0; lines[i] != NULL; i++) {
+if (g_strstr_len(lines[i], -1, "(0x") == NULL) {
+continue;
+}
+res->has_error = true;
+res->error = g_strdup(lines[i]);
+break;
+}
+g_strfreev(lines);
+}
+} while (FindNextVolumeW(handle, guid, ARRAYSIZE(guid)));
+
+FindVolumeClose(handle);
+return resp;
 }
 
 typedef enum {
@@ -1416,7 +1507,7 @@ GList *ga_command_blacklist_init(GList *blacklist)
 "guest-get-memory-blocks", "guest-set-memory-blocks",
 "guest-get-memory-block-size",
 "guest-fsfreeze-freeze-list",
-"guest-fstrim", NULL};
+NULL};
 char **p = (char **)list_unsupported;
 
 while (*p) {
-- 
2.7.4

Re: [Qemu-devel] [PATCH v5 12/20] trace: don't abort qemu if ftrace can't be initialized

[Stefan Hajnoczi]

On Wed, Sep 28, 2016 at 02:08:15PM +0100, Daniel P. Berrange wrote:
> If the ftrace backend is compiled into QEMU, any attempt
> to start QEMU while non-root will fail due to the
> inability to open /sys/kernel/debug/tracing/trace_on.

s/trace_on/tracing_on/

> 
> Add a fallback into the code so that it connects up the
> trace_marker_fd variable to /dev/null when setting
> EACCESS on the 'trace_on' file. This allows QEMU to
> run, with ftrace turned into a no-op.

I wonder whether a warning is appropriate.  It depends on the situation
because maybe QEMU was compiled with multiple backends and we don't care
about ftrace when it fails to initialize...

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v5 11/20] trace: emit name <-> ID mapping in simpletrace header

[Stefan Hajnoczi]

On Wed, Sep 28, 2016 at 02:08:14PM +0100, Daniel P. Berrange wrote:
>  def read_trace_records(edict, fobj):
>  """Deserialize trace records from a file, yielding record tuples 
> (event_num, timestamp, pid, arg1, ..., arg6)."""
> +idtoname = {
> +dropped_event_id: "dropped"
> +}
>  while True:
> -rec = read_record(edict, fobj)
> -if rec is None:
> +t = fobj.read(8)
> +if len(t) == 0:
>  break
>  
> -yield rec
> +(rectype, ) = struct.unpack('=Q', t)
> +if rectype == record_type_mapping:
> +mapping = get_mapping(fobj)
> +idtoname[mapping[0]] = mapping[1]

Unpacking the tuple makes the code easier to read:

event_id, name = get_mapping(fobj)
idtoname[event_id] = name

> diff --git a/scripts/tracetool/format/simpletrace_stap.py 
> b/scripts/tracetool/format/simpletrace_stap.py
> index 7e44bc1..ac3580f 100644
> --- a/scripts/tracetool/format/simpletrace_stap.py
> +++ b/scripts/tracetool/format/simpletrace_stap.py
> @@ -21,6 +21,25 @@ from tracetool.format.stap import stap_escape
>  
>  def generate(events, backend):
>  out('/* This file is autogenerated by tracetool, do not edit. */',
> +'',
> +'global event_name_to_id_map',
> +'global event_next_id',
> +'function simple_trace_map_event(name)',
> +'',
> +'{',
> +'if (!([name] in event_name_to_id_map)) {',
> +'event_name_to_id_map[name] = event_next_id',
> +'name_len = strlen(name)',
> +'printf("%%8b%%8b%%4b%%.*s", 0, ',
> +'   event_next_id, name_len, name_len, name)',
> +'event_next_id = event_next_id + 1',
> +'}',
> +'return event_name_to_id_map[name]',
> +'}',
> +'probe begin',
> +'{',
> +'printf("%%8b%%8b%%8b", 0x, 0xf2b177cb0aa429b4, 
> 4)',

The reason the SystemTap script doesn't emit a header is because it was
designed for trace flight-recorder mode.  There's probably no harm in
emitting the header.  Flight-recorder users will still have to use
--no-header though.


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 1/2] qemu-nbd: Shrink image size by specified offset

[Tomáš Golembiovský]

Whops, somehow I completely forgot about this.

On Tue, 20 Sep 2016 09:09:59 -0500
Eric Blake  wrote:

> On 09/20/2016 04:37 AM, Tomáš Golembiovský wrote:
> 
> [meta-comment]: Your series came through without any threading (you sent
> three threads, instead of patch 1 and 2 being marked In-Reply-To the 0/2
> cover letter).

Thanks for the comment. Unfortunately it was my email client
interfering. It should be better next time.


> > When --offset is set the apparent device size has to be adjusted
> > accordingly. Otherwise client may request read/write beyond the file end
> > which would fail.
> > 
> > Signed-off-by: Tomáš Golembiovský 
> > ---
> >  qemu-nbd.c | 7 +++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/qemu-nbd.c b/qemu-nbd.c
> > index 99297a5..629bce1 100644
> > --- a/qemu-nbd.c
> > +++ b/qemu-nbd.c
> > @@ -901,6 +901,13 @@ int main(int argc, char **argv)
> >  exit(EXIT_FAILURE);
> >  }  
> 
> Additional context:
> 
> off_t dev_offset = 0;
> 
> off_t fd_size;
> 
> >  
> > +if (dev_offset >= fd_size) {
> > +error_report("Offset (%lu) has to be smaller than the image size 
> > (%lu)",
> > + dev_offset, fd_size);  
> 
> Whoops, this fails to compile on 32-bit platforms.  %lu is not
> necessarily synonymous with off_t values.

After some digging I figured off_t is in fact signed type. That makes
the formatting wrong everywhere. Unfortunately I didn't find any good
definition of the type. Any suggestion what format flag should I use? Or
should I just use a temporary variable of known size for that?


Thanks,

Tomas


-- 
Tomáš Golembiovský 

Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support

[Igor Mammedov]

On Fri, 12 Aug 2016 14:54:02 +0800
Xiao Guangrong  wrote:

General design issue in this series is regenerating
_FIT data every time inside of _FIT read loop.

The issue here is that if FIT data doesn't fit in one page
RFIT would be called several times resulting in calling
nvdimm_dsm_func_read_fit() N-times, and in between 
these N exits generated FIT data could change
(for example if another NVDIMM has been hotpluged in between)
resulting in corrupted concatenated FIT buffer returned by
_FIT method.
So one need either to block follow up hotplug or make sure
that FIT data regenerated/updated only once per _FIT
invocation and stay immutable until _FIT is completed
(maybe RCU protected buffer).

Regenerating/updating inside qemu could also be shared
with NFIT table creation path so that both cold/hotplug
would reuse the same data which are updated only when
a NVDIMM is added/removed.
---
I guess I'm done with v2 review at this point.

PS:
Not related to this series but still existing NVDIMM
codebase issues:

1:
OperationRegion (NRAM, SystemMemory, MEMA, 0x1000)
...
Name (MEMA, 0x7000)

is not valid ASL, and most certainly would make Windows BSOD.

Check spec for 
 RegionOffset := TermArg => Integer

Named object is not a TermArg.
I'd suggest to make that OperationRegion dynamic i.e
put its definition into sole user NCAL() and use

Store(MEMA, LocalX)
OperationRegion (NRAM, SystemMemory, LocalX, 0x1000)


2:
Field (NRAM, DWordAcc, NoLock, Preserve)
{
   ...
   ARG3,   32672
}
...
NCAL()
...
Store (Local3, ARG3) /* \_SB_.NVDR.ARG3 */

Using ARG3 name is confusing at best and is wrong as ARG3
is reserved name and probably it won't compile back to valid AML.

Suggest s/ARG3/FARG/
with comment at declaration point
/* Package that contains function-specific arguments _DSM(..., Arg3) */

3:
Method (NCAL, 5, Serialized) {
...
Concatenate (Buffer (Zero) {}, OBUF, Arg6)
Return (Arg6)

it's wrong to use Arg6 here as function has only 5 arguments,
use LocalX instead.


4:
if method creates/access named fields it should be serialized.

Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro

[Halil Pasic]

Hi Paolo,

I'm sorry, but I do not get it quite yet, or more exactly I have the
feeling I did not manage to bring my point over. So I will try with
more details.

On 10/03/2016 01:29 PM, Paolo Bonzini wrote:
> 
> 
> On 03/10/2016 12:36, Halil Pasic wrote:
 #define VMSTATE_PCI_DEVICE(_field, _state) {

This was probably supposed to be VMSTATE_VIRTIO_DEVICE.
   \
 .name   = (stringify(_field)), \
 .size   = sizeof(VirtIODevice),\
 .vmsd   = _virtio_device,  \

This one does not exist at least very tricky to write because of the 
peculiarities
of virtio migration. This one would need to migrate the transport stuff too. And
the specialized device to, which is not normal.

 .flags  = VMS_SINGLE,  \
 .offset = vmstate_offset_value(_state, _field, VirtIODevice),  \

This is useless if we keep virtio_save and virtio_load.

 }


>> I'm not sure if I understand where are you coming from, but if I do, I
>> think I have to disagree here. I think you are coming from the normal 
>> device inheritance direction, where you have the parent storage object
>> (that is its instance data( embedded into the child, and the corresponding
>> parent state is supposed to get migrated as a (vmstate) field of the child.
>>
>> Now if you look at the documentation of virtio migration (or the code) it is
>> pretty obvious that the situation is very different for virtio. Virtio 
>> migration
>> is kind of a template method approach (with virtio_load and virtio_save being
>> the template methods), and the parent (core, transport) and the child (device
>> specific) data are intermingled (the device data is in the middle). We can
>> not change this for compatibility reasons (sadly).
> 
> Regarding VMSTATE_VIRTIO_FIELD, it's just a matter of not doing things
> differently for no particular reason.  Your macro is already doing
> exactly the same as other VMSTATE_* macros, just with different
> conventions for the arguments.  I don't see any advantage in changing that.

In my opinion it is not the same. In the case of VMSTATE_PCI_DEVICE we have
(a self contained) parent (in sense of inheritance) device, which is embedded
as _field in the specialized device and is migrated by the vmstatedescription
of the parent. The rest of the specialized devices state is represented by
the other fields.

VMSTATE_VIRTIO_FIELD is however just there to make sure virtio_load and
virtio_save are called at the right time with the right arguments. The 
specialized
device is then migrated by the save/load callbacks of the device class, or
the vmsd residing in the device class. VMSTATE_VIRTIO_FIELD is supposed
to be the only field, if the virtio device adheres to the virtio-migration
document. VMSTATE_VIRTIO_FIELD has no arguments because it is
a virtual field and does not depend on the offset stuff.

To summarize currently I have no idea how to write up the vmstate
field definition macro VMSTATE_VIRTIO_DEVICE so that it meets your
expectations. 
> 
> Regarding VIRTIO_DEF_DEVICE_VMSD, it's true that virtio migration is
> currently done differently and we will definitely have to do things
> somewhat different due to compatibility, but we can at least evolve
> towards having a normal VMStateDescription (virtio-gpu is already more
> similar to this).  

Virtio-gpu does not adhere to the virtio-migration document because it saves
the specialized device state after the virtio_save is done (that is
after the common virtio subsections). This is however more like the normal
approach -- first you save the parent, then the child.


> Having everything hidden behind a magic macro makes
> things harder to follow than other vmstate definitions.  

Again in my opinion the virtio migration is different that the rest of the
vmstate migration stuff, and it's ugly to some extent. So the idea was
to make it look different and hide the ugliness behind the macro. 
If you insist i will create a version where the macros are expanded so
it's easier to say if this improves or worsens the readability.

> It's okay when
> you have a very small veneer as in commit 5943124cc, but in my opinion
> having field definitions inside macro arguments is already too much.

I think this is matter of taste, and your taste matters more ;). I do 
agree that the variadic for the massaging functions is more complicated
that the two function pointers taken by VMSTATE_VIRTIO_DEVICE. My idea
was that we end up with more readable code on the caller-side, but if you
prefer function pointers and NULLs if no callback is needed needed 
(most cases), I can live with that.

Sorry again for my thick head. 

Cheers,
Halil