Re: [Qemu-devel] [PATCH] libcacard: remove useless initializers

[Alon Levy]

On 05/08/2014 08:19 PM, Michael Tokarev wrote:
> libcacard has many functions which initializes local variables
> at declaration time, which are always assigned some values later
> (often right after declaration).  Clean up these initializers.

How is this an improvement? Doesn't the compiler ignore this anyhow?

> 
> Signed-off-by: Michael Tokarev 
> ---
>  libcacard/cac.c|   14 +++---
>  libcacard/card_7816.c  |5 ++---
>  libcacard/vcard.c  |4 ++--
>  libcacard/vcard_emul_nss.c |6 +++---
>  libcacard/vreader.c|   10 +-
>  libcacard/vscclient.c  |4 ++--
>  6 files changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/libcacard/cac.c b/libcacard/cac.c
> index 122129e..d1d9ee2 100644
> --- a/libcacard/cac.c
> +++ b/libcacard/cac.c
> @@ -93,8 +93,8 @@ cac_common_process_apdu(VCard *card, VCardAPDU *apdu, 
> VCardResponse **response)
>  static VCardStatus
>  cac_applet_pki_reset(VCard *card, int channel)
>  {
> -VCardAppletPrivate *applet_private = NULL;
> -CACPKIAppletData *pki_applet = NULL;
> +VCardAppletPrivate *applet_private;
> +CACPKIAppletData *pki_applet;
>  applet_private = vcard_get_current_applet_private(card, channel);
>  assert(applet_private);
>  pki_applet = &(applet_private->u.pki_data);
> @@ -113,8 +113,8 @@ static VCardStatus
>  cac_applet_pki_process_apdu(VCard *card, VCardAPDU *apdu,
>  VCardResponse **response)
>  {
> -CACPKIAppletData *pki_applet = NULL;
> -VCardAppletPrivate *applet_private = NULL;
> +CACPKIAppletData *pki_applet;
> +VCardAppletPrivate *applet_private;
>  int size, next;
>  unsigned char *sign_buffer;
>  vcard_7816_status_t status;
> @@ -288,7 +288,7 @@ cac_applet_container_process_apdu(VCard *card, VCardAPDU 
> *apdu,
>  static void
>  cac_delete_pki_applet_private(VCardAppletPrivate *applet_private)
>  {
> -CACPKIAppletData *pki_applet_data = NULL;
> +CACPKIAppletData *pki_applet_data;
>  
>  if (applet_private == NULL) {
>  return;
> @@ -336,8 +336,8 @@ static VCardApplet *
>  cac_new_pki_applet(int i, const unsigned char *cert,
> int cert_len, VCardKey *key)
>  {
> -VCardAppletPrivate *applet_private = NULL;
> -VCardApplet *applet = NULL;
> +VCardAppletPrivate *applet_private;
> +VCardApplet *applet;
>  unsigned char pki_aid[] = { 0xa0, 0x00, 0x00, 0x00, 0x79, 0x01, 0x00 };
>  int pki_aid_len = sizeof(pki_aid);
>  
> diff --git a/libcacard/card_7816.c b/libcacard/card_7816.c
> index bca8c4a..a54f880 100644
> --- a/libcacard/card_7816.c
> +++ b/libcacard/card_7816.c
> @@ -416,7 +416,7 @@ 
> VCARD_RESPONSE_NEW_STATIC_STATUS(VCARD7816_STATUS_ERROR_GENERAL)
>  VCardResponse *
>  vcard_make_response(vcard_7816_status_t status)
>  {
> -VCardResponse *response = NULL;
> +VCardResponse *response;
>  
>  switch (status) {
>  /* known 7816 response codes */
> @@ -543,9 +543,8 @@ vcard_make_response(vcard_7816_status_t status)
>  return VCARD_RESPONSE_GET_STATIC(
>  VCARD7816_STATUS_EXC_ERROR_MEMORY_FAILURE);
>  }
> +return response;
>  }
> -assert(response);
> -return response;
>  }
>  
>  /*
> diff --git a/libcacard/vcard.c b/libcacard/vcard.c
> index 227e477..6aaf085 100644
> --- a/libcacard/vcard.c
> +++ b/libcacard/vcard.c
> @@ -166,8 +166,8 @@ vcard_reference(VCard *vcard)
>  void
>  vcard_free(VCard *vcard)
>  {
> -VCardApplet *current_applet = NULL;
> -VCardApplet *next_applet = NULL;
> +VCardApplet *current_applet;
> +VCardApplet *next_applet;
>  
>  if (vcard == NULL) {
>  return;
> diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
> index 75b9d79..3f38a4c 100644
> --- a/libcacard/vcard_emul_nss.c
> +++ b/libcacard/vcard_emul_nss.c
> @@ -367,7 +367,7 @@ vcard_7816_status_t
>  vcard_emul_login(VCard *card, unsigned char *pin, int pin_len)
>  {
>  PK11SlotInfo *slot;
> -unsigned char *pin_string = NULL;
> +unsigned char *pin_string;
>  int i;
>  SECStatus rv;
>  
> @@ -423,7 +423,7 @@ static VReader *
>  vcard_emul_find_vreader_from_slot(PK11SlotInfo *slot)
>  {
>  VReaderList *reader_list = vreader_get_reader_list();
> -VReaderListEntry *current_entry = NULL;
> +VReaderListEntry *current_entry;
>  
>  if (reader_list == NULL) {
>  return NULL;
> @@ -1050,7 +1050,7 @@ void
>  vcard_emul_replay_insertion_events(void)
>  {
>  VReaderListEntry *current_entry;
> -VReaderListEntry *next_entry = NULL;
> +VReaderListEntry *next_entry;
>  VReaderList *list = vreader_get_reader_list();
>  
>  for (current_entry = vreader_list_get_first(list); current_entry;
> diff --git a/libcacard/vreader.c b/libcacard/vreader.c
> index 9304a28..22dfe43 100644
> --- a/libcacard/vreader.c
> +++ b/libcacard/vreader.c
> @@ -341,7 +341,7 @@ void
>  vreader_list_delete(VReaderList *list)
>  

Re: [Qemu-devel] [PATCH] libcacard: g_malloc cleanups

[Alon Levy]

On 05/08/2014 06:54 PM, Michael Tokarev wrote:
> This patch replaces g_malloc() in libcacard into g_new()
> or g_new0() where appropriate (removing some init-to-zero
> surrounding code), g_malloc+memcpy into g_memdup() and the
> like.

Reviewed-by: Alon Levy 

just one comment below.

> 
> Signed-off-by: Michael Tokarev 
> ---
>  libcacard/cac.c|   11 +++
>  libcacard/card_7816.c  |   11 +--
>  libcacard/event.c  |2 +-
>  libcacard/vcard.c  |   22 +-
>  libcacard/vcard_emul_nss.c |   12 ++--
>  libcacard/vreader.c|   11 +++
>  6 files changed, 23 insertions(+), 46 deletions(-)
> 
> diff --git a/libcacard/cac.c b/libcacard/cac.c
> index 74ef3e3..122129e 100644
> --- a/libcacard/cac.c
> +++ b/libcacard/cac.c
> @@ -310,16 +310,11 @@ static VCardAppletPrivate *
>  cac_new_pki_applet_private(const unsigned char *cert,
> int cert_len, VCardKey *key)
>  {
> -CACPKIAppletData *pki_applet_data = NULL;
> -VCardAppletPrivate *applet_private = NULL;

These two lines above (and the corresponding ones beloow) seem unrelated
to this patch.

> -applet_private = (VCardAppletPrivate 
> *)g_malloc(sizeof(VCardAppletPrivate));
> +CACPKIAppletData *pki_applet_data;
> +VCardAppletPrivate *applet_private;
>  
> +applet_private = g_new0(VCardAppletPrivate, 1);
>  pki_applet_data = &(applet_private->u.pki_data);
> -pki_applet_data->cert_buffer = NULL;
> -pki_applet_data->cert_buffer_len = 0;
> -pki_applet_data->sign_buffer = NULL;
> -pki_applet_data->sign_buffer_len = 0;
> -pki_applet_data->key = NULL;
>  pki_applet_data->cert = (unsigned char *)g_malloc(cert_len+1);
>  /*
>   * if we want to support compression, then we simply change the 0 to a 1
> diff --git a/libcacard/card_7816.c b/libcacard/card_7816.c
> index c28bb60..bca8c4a 100644
> --- a/libcacard/card_7816.c
> +++ b/libcacard/card_7816.c
> @@ -51,7 +51,7 @@ vcard_response_new_data(unsigned char *buf, int len)
>  {
>  VCardResponse *new_response;
>  
> -new_response = (VCardResponse *)g_malloc(sizeof(VCardResponse));
> +new_response = g_new(VCardResponse, 1);
>  new_response->b_data = g_malloc(len + 2);
>  memcpy(new_response->b_data, buf, len);
>  new_response->b_total_len = len+2;
> @@ -132,7 +132,7 @@ vcard_response_new_status(vcard_7816_status_t status)
>  {
>  VCardResponse *new_response;
>  
> -new_response = (VCardResponse *)g_malloc(sizeof(VCardResponse));
> +new_response = g_new(VCardResponse, 1);
>  new_response->b_data = &new_response->b_sw1;
>  new_response->b_len = 0;
>  new_response->b_total_len = 2;
> @@ -149,7 +149,7 @@ vcard_response_new_status_bytes(unsigned char sw1, 
> unsigned char sw2)
>  {
>  VCardResponse *new_response;
>  
> -new_response = (VCardResponse *)g_malloc(sizeof(VCardResponse));
> +new_response = g_new(VCardResponse, 1);
>  new_response->b_data = &new_response->b_sw1;
>  new_response->b_len = 0;
>  new_response->b_total_len = 2;
> @@ -336,9 +336,8 @@ vcard_apdu_new(unsigned char *raw_apdu, int len, 
> vcard_7816_status_t *status)
>  return NULL;
>  }
>  
> -new_apdu = (VCardAPDU *)g_malloc(sizeof(VCardAPDU));
> -new_apdu->a_data = g_malloc(len);
> -memcpy(new_apdu->a_data, raw_apdu, len);
> +new_apdu = g_new(VCardAPDU, 1);
> +new_apdu->a_data = g_memdup(raw_apdu, len);
>  new_apdu->a_len = len;
>  *status = vcard_apdu_set_class(new_apdu);
>  if (*status != VCARD7816_STATUS_SUCCESS) {
> diff --git a/libcacard/event.c b/libcacard/event.c
> index 2d7500f..a2e6c7d 100644
> --- a/libcacard/event.c
> +++ b/libcacard/event.c
> @@ -17,7 +17,7 @@ vevent_new(VEventType type, VReader *reader, VCard *card)
>  {
>  VEvent *new_vevent;
>  
> -new_vevent = (VEvent *)g_malloc(sizeof(VEvent));
> +new_vevent = g_new(VEvent, 1);
>  new_vevent->next = NULL;
>  new_vevent->type = type;
>  new_vevent->reader = vreader_reference(reader);
> diff --git a/libcacard/vcard.c b/libcacard/vcard.c
> index 539177b..227e477 100644
> --- a/libcacard/vcard.c
> +++ b/libcacard/vcard.c
> @@ -37,9 +37,8 @@ vcard_buffer_response_new(unsigned char *buffer, int size)
>  {
>  VCardBufferResponse *new_buffer;
>  
> -new_buffer = (VCardBufferResponse 
> *)g_malloc(sizeof(VCardBufferResponse));
> -new_buffer->buffer = (unsigned char *)g_malloc(size);
> -memcpy(new_buffer->buffer, buffer, size);
> +new_buffer = g_new(VCardBufferResponse, 1);
> +new_buffer->buffer = (unsigned char *)g_memdup(buffer, size);
>  new_buffer->buffer_len = size;
>  new_buffer->current = new_buffer->buffer;
>  new_buffer->len = size;
> @@ -102,15 +101,11 @@ vcard_new_applet(VCardProcessAPDU 
> applet_process_function,
>  {
>  VCardApplet *applet;
>  
> -applet = (VCardApplet *)g_malloc(sizeof(VCardApplet));
> -applet->next = NULL;
> -app

Re: [Qemu-devel] [PATCH v2] Add remove_boot_device_path() function for hot-unplug device

[Marcel Apfelbaum]

On Sun, 2014-05-11 at 11:07 +0800, lijun wrote:
> On 04/22/2014 05:21 PM, Marcel Apfelbaum wrote:
> > On Wed, 2014-04-16 at 22:20 +0800, Jun Li wrote:
> >> Add remove_boot_device_path() function to remove bootindex when hot-unplug
> >> a device. This patch fixed virtio-blk/virtio-net/scsi-disk/scsi-generic 
> >> device.
> >> So it has fixed bug1086603, ref:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1086603
> >>
> >> Make some changes based on Andreas's good suggestion.
> >>
> >> Signed-off-by: Jun Li 
> >> ---
> >>   hw/block/virtio-blk.c   |  1 +
> >>   hw/net/virtio-net.c |  1 +
> >>   hw/scsi/scsi-disk.c |  6 --
> >>   hw/scsi/scsi-generic.c  |  3 +++
> >>   include/sysemu/sysemu.h |  2 ++
> >>   vl.c| 16 
> >>   6 files changed, 27 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> >> index 8a568e5..ecdd266 100644
> >> --- a/hw/block/virtio-blk.c
> >> +++ b/hw/block/virtio-blk.c
> >> @@ -752,6 +752,7 @@ static void virtio_blk_device_unrealize(DeviceState 
> >> *dev, Error **errp)
> >>   unregister_savevm(dev, "virtio-blk", s);
> >>   blockdev_mark_auto_del(s->bs);
> >>   virtio_cleanup(vdev);
> >> +remove_boot_device_path(s->conf->bootindex, dev, "/disk@0,0");
> >>   }
> >>   
> >>   static Property virtio_blk_properties[] = {
> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >> index 33bd233..520c029 100644
> >> --- a/hw/net/virtio-net.c
> >> +++ b/hw/net/virtio-net.c
> >> @@ -1633,6 +1633,7 @@ static void virtio_net_device_unrealize(DeviceState 
> >> *dev, Error **errp)
> >>   g_free(n->vqs);
> >>   qemu_del_nic(n->nic);
> >>   virtio_cleanup(vdev);
> >> +remove_boot_device_path(n->nic_conf.bootindex, dev, 
> >> "/ethernet-phy@0");
> >>   }
> >>   
> >>   static void virtio_net_instance_init(Object *obj)
> >> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> >> index 48a28ae..bb2176a 100644
> >> --- a/hw/scsi/scsi-disk.c
> >> +++ b/hw/scsi/scsi-disk.c
> >> @@ -2150,12 +2150,14 @@ static void scsi_disk_reset(DeviceState *dev)
> >>   s->tray_open = 0;
> >>   }
> >>   
> >> -static void scsi_destroy(SCSIDevice *dev)
> >> +static void scsi_destroy(SCSIDevice *sdev)
> >>   {
> >> -SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
> >> +SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, sdev);
> >> +DeviceState *dev = DEVICE(sdev);
> >>   
> >>   scsi_device_purge_requests(&s->qdev, SENSE_CODE(NO_SENSE));
> >>   blockdev_mark_auto_del(s->qdev.conf.bs);
> >> +remove_boot_device_path(s->qdev.conf.bootindex, dev, NULL);
> >>   }
> >>   
> >>   static void scsi_disk_resize_cb(void *opaque)
> >> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> >> index 8d92e0d..2531a44 100644
> >> --- a/hw/scsi/scsi-generic.c
> >> +++ b/hw/scsi/scsi-generic.c
> >> @@ -388,8 +388,11 @@ static void scsi_generic_reset(DeviceState *dev)
> >>   
> >>   static void scsi_destroy(SCSIDevice *s)
> >>   {
> >> +DeviceState *dev = DEVICE(s);
> >> +
> >>   scsi_device_purge_requests(s, SENSE_CODE(NO_SENSE));
> >>   blockdev_mark_auto_del(s->conf.bs);
> >> +remove_boot_device_path(s->conf.bootindex, dev, NULL);
> >>   }
> >>   
> >>   static int scsi_generic_initfn(SCSIDevice *s)
> >> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> >> index ba5c7f8..f7ad1e2 100644
> >> --- a/include/sysemu/sysemu.h
> >> +++ b/include/sysemu/sysemu.h
> >> @@ -193,6 +193,8 @@ void rtc_change_mon_event(struct tm *tm);
> >>   
> >>   void add_boot_device_path(int32_t bootindex, DeviceState *dev,
> >> const char *suffix);
> >> +void remove_boot_device_path(int32_t bootindex, DeviceState *dev,
> >> + const char *suffix);
> >>   char *get_boot_devices_list(size_t *size, bool ignore_suffixes);
> >>   
> >>   DeviceState *get_boot_device(uint32_t position);
> >> diff --git a/vl.c b/vl.c
> >> index 9975e5a..1713c68 100644
> >> --- a/vl.c
> >> +++ b/vl.c
> >> @@ -1184,6 +1184,22 @@ void add_boot_device_path(int32_t bootindex, 
> >> DeviceState *dev,
> >>   QTAILQ_INSERT_TAIL(&fw_boot_order, node, link);
> >>   }
> >>   
> >> +void remove_boot_device_path(int32_t bootindex, DeviceState *dev,
> >> + const char *suffix)
> >> +{
> >> +FWBootEntry *node, *next_node;
> >> +
> >> +if (bootindex == -1) {
> >> +return;
> >> +}
> >> +
> >> +QTAILQ_FOREACH_SAFE(node, &fw_boot_order, link, next_node)
> >> +if (node->bootindex == bootindex) {
> >> +QTAILQ_REMOVE(&fw_boot_order, node, link);
> > Hi Liu.
> > Sorry for the late response, but I was in vacation :) .
> > Your patch looks OK, I once wrote another one on the same issue
> > (it touched the same problem, but it was *completely* different).
> >
> > Here is the *real* problem: fw_boot_order list is not queried
> > again on guest reboot, so 'touching' the list has n

Re: [Qemu-devel] [PULL 0/4] libcacard-standalone (glib compat & libcacard) series for 2014-05-08

[Michael Tokarev]

09.05.2014 18:53, Peter Maydell пишет:
> On 8 May 2014 09:30, Michael Tokarev  wrote:
>> Here's a pull request for glib pre-2.31 compatibility layer and libcacard
>> cleanups on top of it.  It has been submitted for review previously:
>>
>> v1: http://thread.gmane.org/gmane.comp.emulators.qemu/269432
>> v2: http://thread.gmane.org/gmane.comp.emulators.qemu/270259
>>
>> Previous attempt of adding glib compat layer by Stefan Hajnoczi:
>> http://thread.gmane.org/gmane.comp.emulators.qemu/253830
>>
>> Changes since v2 submission:
>>
>>  - dropped 3 trivial changes which were applied meantime
>>  - fixed coding style (identation/spacing) in first patch
>>alevy: I hope these tiny spacing changes are okay for
>>you to keep your reviewed-by ;)
>>
>> Please consider applying.  Since this touches several areas of
>> qemu, there's no clear maintainer so I'm not really sure which
>> maintainer tree should pick this up.  And it isn't very suitable
>> for -trival either :)
> 
> I'm still a bit dubious about the approach this patchset
> takes to glib back-compatibility (ie using #defines etc
> to fake up the new glib API on older glib versions, rather
> than defining wrappers), and it sounded from the conversation
> on IRC today as if Stefan was also not completely convinced.
> So I don't think there's enough consensus to apply this
> yet, and I'd rather we had more discussion/review first.

This whole thing is almost moot really.  It is an old glib
API, and no one cares about it anymore except redhat, because
it is the only distribution nowadays which is relevant and
ships that old glib.  We're discussing adding compat layer
in qemu for thing which is almost irrelevant for everyone else
who moved on to the new API and don't care about old crap.
I tried to make old redhat glib API to be almost compatible
with current glib, _just_ to not introduce additional
version requiriment.  For an API which is frozen and wont
change anymore, and which is almost unused in whole qemu
too.  The layer which I provided actually works and is
actually tested on several different platforms.

For me, I don't care a damn about this old crap.  New glib
API works on all relevant versions of Debian for example,
so I can just clean up crap in libcacard using new API
directly - ofcourse it wont be upstreamable due to that
old glib api which is still relevant, but at least we'll
have a proper libcacard in Debian.

I just wanted to do the dirty work for everyone's benefit
without adding new complexity.  If that's not needed, no
worries from me.  The 2 remaining libcacard patches are
small and trivial enough to keep in debian tree.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH] libcacard: remove useless initializers

[Michael Tokarev]

11.05.2014 11:58, Alon Levy wrote:
> On 05/08/2014 08:19 PM, Michael Tokarev wrote:
>> libcacard has many functions which initializes local variables
>> at declaration time, which are always assigned some values later
>> (often right after declaration).  Clean up these initializers.
> 
> How is this an improvement? Doesn't the compiler ignore this anyhow?

Just less code.

To me, when I see something like

  Type *var = NULL;

in a function, it somehow "translates" to a construct like

  Type *found = NULL;

That is -- so this variable will be used either as an accumulator
or a search result, so that initial value is really important.

So when I see the same variable receives its initial value in
the next line, I start wondering what's missed in the code which
should be there.  Or why I don't read the code correctly.  Or
something like this.

So, basically, this is a cleanup patch just to avoid confusion,
it most likely not needed for current compiler who can figure
it out by its own.  And for consistency - why not initialize
other variables too?

Maybe that's just my old-scool mind works this way.

At any rate you can just ignore this patch.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH] libcacard: g_malloc cleanups

[Michael Tokarev]

11.05.2014 12:03, Alon Levy wrote:
[]
>> diff --git a/libcacard/cac.c b/libcacard/cac.c
>> index 74ef3e3..122129e 100644
>> --- a/libcacard/cac.c
>> +++ b/libcacard/cac.c
>> @@ -310,16 +310,11 @@ static VCardAppletPrivate *
>>  cac_new_pki_applet_private(const unsigned char *cert,
>> int cert_len, VCardKey *key)
>>  {
>> -CACPKIAppletData *pki_applet_data = NULL;
>> -VCardAppletPrivate *applet_private = NULL;
> 
> These two lines above (and the corresponding ones beloow) seem unrelated
> to this patch.
> 
>> -applet_private = (VCardAppletPrivate 
>> *)g_malloc(sizeof(VCardAppletPrivate));
>> +CACPKIAppletData *pki_applet_data;
>> +VCardAppletPrivate *applet_private;
>>  
>> +applet_private = g_new0(VCardAppletPrivate, 1);

Yes, that's removal of initial value setting of variables which
will be initialized on the next line.  It is like cleaning up
coding style (spacing around {} etc) in a nearby lines.  I can
mention this in the commit message, but I don't really think it
is really important.

Thanks,

/mjt

Re: [Qemu-devel] uvesafb doesn't work with seabios

[Bernhard Walle]

Am 10.05.14 17:07, schrieb Kevin O'Connor:
> 
> Can you run the original SeaVGABIOS image and provide the output with
> the following added to the qemu command line:
> 
> -chardev stdio,id=seabios -device isa-debugcon,iobase=0x402,chardev=seabios

Since it's quite much debugging info, I uploaded it to
http://bwalle.de/tmp/debugoutput-unmodified

Basically it's

  set VGA mode 12
  set VGA mode 3
  VBE mode info request: 100
  VBE mode info request: 101
  ...
  VBE mode info request: 6a
  set VGA mode 3
  VBE mode info request: 100

> Also, it looks like uvesafb can call x86emu.  Older versions of x86emu
> are known to not emulate some instructions properly, and it has caused
> problems for SeaVGABIOS.  The most recent version of SeaVGABIOS from
> SeaBIOS git has additional checks for this - can you grab the seabios
> git, set CONFIG_DEBUG_LEVEL to 8, build SeaVGABIOS (set
> CONFIG_VGA_BOCHS), and post the logs from both Linux and QEMU with the
> above options?

It doesn't change the result (that it doesn't work). It only makes the
kernel failing much faster and of course there's much more debugging
output: http://bwalle.de/tmp/debugoutput (250k).


Regards,
Bernhard

Re: [Qemu-devel] [PATCH] monitor: Add info qom-tree subcommand.

[Hani Benhabiles]

On Wed, May 07, 2014 at 01:01:12PM +0200, Paolo Bonzini wrote:
> Il 05/05/2014 20:33, Luiz Capitulino ha scritto:
> >>+data = object_property_get_qobject(obj, info->name, NULL);
> >>+if (!data) {
> >>+list = list->next;
> >>+continue;
> 
> You could use object_property_print and get rid of a relatively large amount
> of code.

Thanks for pointing that. But StringOutputVisitor doesn't have handlers for 
structs and
lists like QMP countrepart so the call segfaults when used against object
properties using that.

ie. Unless I am missing something, I should add those handlers to the string
visitor before being able to use object_property_print(), right ?

> Apart from this, the patch is definitely useful.

Re: [Qemu-devel] [PATCH] monitor: Add info qom-tree subcommand.

[Hani Benhabiles]

On Wed, May 07, 2014 at 05:39:36PM +0200, Andreas Färber wrote:
> Hi Hani,
> 
> Am 27.04.2014 12:29, schrieb Hani Benhabiles:
> > Signed-off-by: Hani Benhabiles 
> > ---
> > 
> > Not sure whether the qobject stringifying functions could fit or be of some 
> > use
> > elsewhere. Thus, I kept them as static near the only place where they are 
> > used
> > at the moment.
> 
> Your "info qom-tree" reads quite similar to my proposed qom-tree script:
> http://patchwork.ozlabs.org/patch/317224/
> 
> For HMP I had been working on a command "info qom-composition" that
> emits a trimmed-down overview-style output, such as for x86_64:
> 
> (qemu) info qom-composition
> /machine (pc-i440fx-2.1-machine)
>   /peripheral-anon (container)
>   /peripheral (container)
>   /unattached (container)
> /sysbus (System)
> /device[0] (qemu64-x86_64-cpu)
>   /apic (apic)
> /device[1] (kvmvapic)
> /device[2] (i440FX)
> /device[3] (PIIX3)
>   /isa.0 (ISA)
> /device[4] (isa-i8259)
> /device[5] (isa-i8259)
> /device[6] (cirrus-vga)
> /device[7] (hpet)
> /device[8] (mc146818rtc)
> /device[9] (isa-pit)
> /device[10] (isa-pcspk)
> /device[11] (isa-serial)
> /device[12] (isa-parallel)
> /device[13] (i8042)
> /device[14] (vmport)
> /device[15] (vmmouse)
> /device[16] (port92)
> /device[17] (isa-fdc)
> /device[18] (e1000)
> /device[19] (piix3-ide)
>   /ide.0 (IDE)
>   /ide.1 (IDE)
> /device[20] (ide-cd)
> /device[21] (PIIX4_PM)
>   /i2c (i2c-bus)
> /device[22] (smbus-eeprom)
> /device[23] (smbus-eeprom)
> /device[24] (smbus-eeprom)
> /device[25] (smbus-eeprom)
> /device[26] (smbus-eeprom)
> /device[27] (smbus-eeprom)
> /device[28] (smbus-eeprom)
> /device[29] (smbus-eeprom)
>   /icc-bridge (icc-bridge)
> /icc (icc-bus)
>   /fw_cfg (fw_cfg)
>   /i440fx (i440FX-pcihost)
> /pci.0 (PCI)
> /ioapic (ioapic)
> 
> I believe both commands can coexist. Question is how.
> 
> My tree-walking implementation is not based on QMP and thus much
> slimmer. I didn't have to care about printing properties yet - that I
> deferred to a qom-get HMP command (which based on previous feedback we
> should implement either way). Possibly we could rebase your patch onto
> mine, adding an argument for whether or not to print the properties?
>

Sure, I am fine with rebasing the patch on top of yours if that ends up with
less duplicated work.

> Compared to my script, your "info qom-tree" does not allow to change the
> root, so I believe we would need to print from "/" on (rather than
> "/machine"), for dumping RNG backends etc. as well. The alternative
> would be having arguments to the command, for specifying root and/or
> output style.

A path argument (defaulting to "/") seems like a good approach to me, but I have
no strong preference on this.

Re: [Qemu-devel] uvesafb doesn't work with seabios

[Kevin O'Connor]

On Sun, May 11, 2014 at 01:42:57PM +0200, Bernhard Walle wrote:
> Am 10.05.14 17:07, schrieb Kevin O'Connor:
> > Also, it looks like uvesafb can call x86emu.  Older versions of x86emu
> > are known to not emulate some instructions properly, and it has caused
> > problems for SeaVGABIOS.  The most recent version of SeaVGABIOS from
> > SeaBIOS git has additional checks for this - can you grab the seabios
> > git, set CONFIG_DEBUG_LEVEL to 8, build SeaVGABIOS (set
> > CONFIG_VGA_BOCHS), and post the logs from both Linux and QEMU with the
> > above options?
> 
> It doesn't change the result (that it doesn't work). It only makes the
> kernel failing much faster and of course there's much more debugging
> output: http://bwalle.de/tmp/debugoutput (250k).

Were there any Linux or userspace messages with the above?

-Kevin

[Qemu-devel] [PATCH] block/nfs: use per-object vars and make it modular

[Michael Tokarev]

nfs block module uses libnfs and uses pkg-config to determine
its build information.  Somehow it used only --libs, not --cflags,
and added those libs into global $LIBS, instead of using per-object
variable.

Use both --libs and --cflags, use them as per-object variable,
and finally make block/nfs.o to be modular.

Signed-off-by: Michael Tokarev 
---
 block/Makefile.objs |2 ++
 configure   |6 --
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/Makefile.objs b/block/Makefile.objs
index fd88c03..38ddc0e 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -35,5 +35,7 @@ gluster.o-cflags   := $(GLUSTERFS_CFLAGS)
 gluster.o-libs := $(GLUSTERFS_LIBS)
 ssh.o-cflags   := $(LIBSSH2_CFLAGS)
 ssh.o-libs := $(LIBSSH2_LIBS)
+nfs.o-cflags   := $(LIBNFS_CFLAGS)
+nfs.o-libs := $(LIBNFS_LIBS)
 qcow.o-libs:= -lz
 linux-aio.o-libs   := -laio
diff --git a/configure b/configure
index ac2fa15..3bc91f6 100755
--- a/configure
+++ b/configure
@@ -3928,7 +3928,7 @@ if test "$libnfs" != "no" ; then
   if $pkg_config --atleast-version=1.9.3 libnfs; then
 libnfs="yes"
 libnfs_libs=$($pkg_config --libs libnfs)
-LIBS="$LIBS $libnfs_libs"
+libnfs_cflags=$($pkg_config --cflags libnfs)
   else
 if test "$libnfs" = "yes" ; then
   feature_not_found "libnfs"
@@ -4534,7 +4534,9 @@ if test "$libiscsi" = "yes" ; then
 fi
 
 if test "$libnfs" = "yes" ; then
-  echo "CONFIG_LIBNFS=y" >> $config_host_mak
+  echo "CONFIG_LIBNFS=m" >> $config_host_mak
+  echo "LIBNFS_CFLAGS=$libnfs_cflags" >> $config_host_mak
+  echo "LIBNFS_LIBS=$libnfs_libs" >> $config_host_mak
 fi
 
 if test "$seccomp" = "yes"; then
-- 
1.7.10.4

Re: [Qemu-devel] My OS hangup in KVM for some reasons, how can I debug?

[Jun Koi]

On Fri, May 9, 2014 at 11:24 AM, Jun Koi  wrote:

>
>
>
> On Thu, May 8, 2014 at 4:28 PM, Jun Koi  wrote:
>
>> Hi,
>>
>> I have an weird OS that I am trying to boot in KVM. however, it just hang
>> in the middle, without a good reason.
>>
>> The same OS boots fine in physical machine, and this OS comes with no
>> source code.
>>
>> There must be a bug somewhere in KVM, so I am wondering how people debug
>> deal with such a case to find and fix bugs in KVM?
>>
>> Same thing like this happened before with some new OSes (such as Windows
>> 8), and somebody always came up with a quick fix, so I am wondering what
>> the magic is here.
>>
>>
>
Any help, please?


Thanks,
Jun

Re: [Qemu-devel] [PATCH qom-next 1/4] qom: Implement info qom-composition HMP command

[Hani Benhabiles]

On Thu, May 08, 2014 at 04:21:14PM +0200, Andreas Färber wrote:
> To complement qdev's bus-oriented info qtree, info qom-composition
> prints a hierarchical view of the machine composition tree.
> 
> Signed-off-by: Andreas Färber 
> ---
>  include/monitor/qdev.h |  1 +
>  monitor.c  |  7 +++
>  qdev-monitor.c | 35 +++
>  3 files changed, 43 insertions(+)
> 
> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
> index 8d16e11..cb5c8ee 100644
> --- a/include/monitor/qdev.h
> +++ b/include/monitor/qdev.h
> @@ -8,6 +8,7 @@
>  
>  void do_info_qtree(Monitor *mon, const QDict *qdict);
>  void do_info_qdm(Monitor *mon, const QDict *qdict);
> +void do_info_qom_composition(Monitor *mon, const QDict *dict);
>  int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  int qdev_device_help(QemuOpts *opts);
>  DeviceState *qdev_device_add(QemuOpts *opts);
> diff --git a/monitor.c b/monitor.c
> index 1266ba0..c2b9315 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2920,6 +2920,13 @@ static mon_cmd_t info_cmds[] = {
>  .mhandler.cmd = do_info_qdm,
>  },
>  {
> +.name   = "qom-composition",
> +.args_type  = "",
> +.params = "",
> +.help   = "show QOM machine composition tree",
> +.mhandler.cmd = do_info_qom_composition,
> +},
> +{
>  .name   = "roms",
>  .args_type  = "",
>  .params = "",
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 02cbe43..744144a 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -658,6 +658,41 @@ void do_info_qdm(Monitor *mon, const QDict *qdict)
>  qdev_print_devinfos(true);
>  }
>  
> +typedef struct QOMCompositionState {
> +Monitor *mon;
> +int indent;
> +} QOMCompositionState;
> +
> +static void print_qom_composition(Monitor *mon, Object *obj, int indent);
> +
> +static int print_qom_composition_child(Object *obj, void *opaque)
> +{
> +QOMCompositionState *s = opaque;
> +
> +print_qom_composition(s->mon, obj, s->indent);
> +
> +return 0;
> +}
> +
> +static void print_qom_composition(Monitor *mon, Object *obj, int indent)
> +{
> +QOMCompositionState s = {
> +.mon = mon,
> +.indent = indent + 2,
> +};
> +char *name = object_get_canonical_path_component(obj);
> +
> +monitor_printf(mon, "%*s/%s (%s)\n", indent, "", name,
> +   object_get_typename(obj));
> +g_free(name);
> +object_child_foreach(obj, print_qom_composition_child, &s);
> +}
> +
> +void do_info_qom_composition(Monitor *mon, const QDict *dict)
> +{
> +print_qom_composition(mon, qdev_get_machine(), 0);
> +}
> +
>  int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
>  Error *local_err = NULL;

Reviewed-by: Hani Benhabiles 

Re: [Qemu-devel] [PATCH 3/4] block: replace fprintf(stderr, ...) with error_report() in block/

[Le Tan]

2014-05-10 21:18 GMT+08:00 Peter Crosthwaite :
> On Sat, May 10, 2014 at 9:55 AM, Le Tan  wrote:
>> Replace fprintf(stderr,...) with error_report() in files block/*, block.c,
>> block-migration.c and blockdev.c. The trailing "\n"s of the @fmt argument
>> have been removed because @fmt of error_report() should not contain newline.
>>
>> Signed-off-by: Le Tan 
>> ---
>>  block-migration.c  |4 +-
>>  block.c|4 +-
>>  block/linux-aio.c  |4 +-
>>  block/nbd-client.h |2 +-
>>  block/qcow2-cluster.c  |4 +-
>>  block/qcow2-refcount.c |  114 
>> 
>>  block/qcow2.c  |   16 +++
>>  block/quorum.c |4 +-
>>  block/raw-posix.c  |8 ++--
>>  block/raw-win32.c  |6 +--
>>  block/ssh.c|4 +-
>>  block/vdi.c|   14 +++---
>>  block/vmdk.c   |   21 -
>>  block/vpc.c|4 +-
>>  block/vvfat.c  |  108 ++---
>>  blockdev.c |6 +--
>>  16 files changed, 160 insertions(+), 163 deletions(-)
>>
>> diff --git a/block-migration.c b/block-migration.c
>> index 56951e0..5bcf7c8 100644
>> --- a/block-migration.c
>> +++ b/block-migration.c
>> @@ -790,7 +790,7 @@ static int block_load(QEMUFile *f, void *opaque, int 
>> version_id)
>>
>>  bs = bdrv_find(device_name);
>>  if (!bs) {
>> -fprintf(stderr, "Error unknown block device %s\n",
>> +error_report("Error unknown block device %s",
>>  device_name);
>>  return -EINVAL;
>>  }
>> @@ -833,7 +833,7 @@ static int block_load(QEMUFile *f, void *opaque, int 
>> version_id)
>> (addr == 100) ? '\n' : '\r');
>>  fflush(stdout);
>>  } else if (!(flags & BLK_MIG_FLAG_EOS)) {
>> -fprintf(stderr, "Unknown block migration flags: %#x\n", flags);
>> +error_report("Unknown block migration flags: %#x", flags);
>>  return -EINVAL;
>>  }
>>  ret = qemu_file_get_error(f);
>> diff --git a/block.c b/block.c
>> index b749d31..7dc4acb 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2694,8 +2694,8 @@ static int bdrv_prwv_co(BlockDriverState *bs, int64_t 
>> offset,
>>   * if it has been enabled.
>>   */
>>  if (bs->io_limits_enabled) {
>> -fprintf(stderr, "Disabling I/O throttling on '%s' due "
>> -"to synchronous I/O.\n", bdrv_get_device_name(bs));
>> +error_report("Disabling I/O throttling on '%s' due "
>> + "to synchronous I/O.", bdrv_get_device_name(bs));
>>  bdrv_io_limits_disable(bs);
>>  }
>>
>> diff --git a/block/linux-aio.c b/block/linux-aio.c
>> index 53434e2..b706a59 100644
>> --- a/block/linux-aio.c
>> +++ b/block/linux-aio.c
>> @@ -162,8 +162,8 @@ BlockDriverAIOCB *laio_submit(BlockDriverState *bs, void 
>> *aio_ctx, int fd,
>> break;
>>  /* Currently Linux kernel does not support other operations */
>>  default:
>> -fprintf(stderr, "%s: invalid AIO request type 0x%x.\n",
>> -__func__, type);
>> +error_report("%s: invalid AIO request type 0x%x.",
>> + __func__, type);
>>  goto out_free_aiocb;
>>  }
>>  io_set_eventfd(&laiocb->iocb, event_notifier_get_fd(&s->e));
>> diff --git a/block/nbd-client.h b/block/nbd-client.h
>> index f2a6337..74178f4 100644
>> --- a/block/nbd-client.h
>> +++ b/block/nbd-client.h
>> @@ -9,7 +9,7 @@
>>
>>  #if defined(DEBUG_NBD)
>>  #define logout(fmt, ...) \
>> -fprintf(stderr, "nbd\t%-24s" fmt, __func__, ##__VA_ARGS__)
>> +error_report("nbd\t%-24s" fmt, __func__, ##__VA_ARGS__)
>
> So i'm not sure we want to convert debug printfery to error_report.
> There's very good value in converting the printfs with user
> visibility, but ones like this seem intended for developers only as
> throwaway-output. My thinking is that this is a lower level output
> than error_report. For instance, as a developer you may do something
> to break your error API yet you still want your debug printfery.
> Wouldn't matter in this location, but there may be other parts of the
> tree where we don't want error_report relinace for debug
> instrumentation and it just seems better to keep it consistent.
>
> Thinking further afield, qemu_log may ultimately be the correct
> mechanism for this instead (I think thats what I have been using for
> new code recently anyway).
>
> Thoughts from others?
>
> Regards,
> Peter
Hi! I am a novice and this is my warm-up task for GSoC. So you mean
that it's good to convert printfs to error_report where the message is
deemed to notice the user, such as some warning about wrong cmd
arguments and so on, while it is better to let them be where the
printfs are used for the developer to debug, such as:
#if defined(DEBUG_NBD)
#define logout(fmt, ...) \
   

Re: [Qemu-devel] [PATCH qom-next 2/4] qom: Implement qom-list HMP command

[Hani Benhabiles]

On Thu, May 08, 2014 at 04:21:15PM +0200, Andreas Färber wrote:
> Implement it as a wrapper for QMP qom-list, but mimic the behavior of
> scripts/qmp/qom-list in making the path argument optional and listing
> the root if absent, to hint users what kind of path to pass.
> 
> Signed-off-by: Andreas Färber 
> ---
>  hmp-commands.hx | 13 +
>  hmp.c   | 27 +++
>  hmp.h   |  1 +
>  3 files changed, 41 insertions(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 8971f1b..a0166af 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1678,6 +1678,19 @@ Add CPU with id @var{id}
>  ETEXI
>  
>  {
> +.name   = "qom-list",
> +.args_type  = "path:s?",
> +.params = "path",
> +.help   = "list QOM properties",
> +.mhandler.cmd  = hmp_qom_list,
> +},
> +
> +STEXI
> +@item qom-list [@var{path}]
> +Print QOM properties of object at location @var{path}
> +ETEXI
> +
> +{
>  .name   = "info",
>  .args_type  = "item:s?",
>  .params = "[subcommand]",
> diff --git a/hmp.c b/hmp.c
> index ca869ba..1c29c8a 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1666,3 +1666,30 @@ void hmp_object_del(Monitor *mon, const QDict *qdict)
>  qmp_object_del(id, &err);
>  hmp_handle_error(mon, &err);
>  }
> +
> +void hmp_qom_list(Monitor *mon, const QDict *qdict)
> +{
> +const char *path = qdict_get_try_str(qdict, "path");
> +ObjectPropertyInfoList *list;
> +Error *err = NULL;
> +
> +if (path == NULL) {
> +monitor_printf(mon, "/\n");
> +return;
> +}
> +list = qmp_qom_list(path, &err);
> +if (err == NULL) {

This:

> +while (list != NULL) {
> +ObjectPropertyInfoList *next = list->next;
> +
> +monitor_printf(mon, "%s (%s)\n",
> +   list->value->name, list->value->type);
> +g_free(list->value->name);
> +g_free(list->value->type);
> +g_free(list->value);
> +g_free(list);
> +list = next;
> +}

could be simplified to:
ObjectPropertyInfoList *start = list;
while (list) {
ObjectPropertyInfo *value = list->value;
monitor_printf(mon, "%s (%s)\n", value->name, value->type);
list = list->next;
}
qapi_free_ObjectPropertyInfoList(start);

Other than that, the patch seems fine to me.

> +}
> +hmp_handle_error(mon, &err);
> +}
> diff --git a/hmp.h b/hmp.h
> index 20ef454..7ab969e 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -93,6 +93,7 @@ void hmp_qemu_io(Monitor *mon, const QDict *qdict);
>  void hmp_cpu_add(Monitor *mon, const QDict *qdict);
>  void hmp_object_add(Monitor *mon, const QDict *qdict);
>  void hmp_object_del(Monitor *mon, const QDict *qdict);
> +void hmp_qom_list(Monitor *mon, const QDict *qdict);
>  void object_add_completion(ReadLineState *rs, int nb_args, const char *str);
>  void object_del_completion(ReadLineState *rs, int nb_args, const char *str);
>  void device_add_completion(ReadLineState *rs, int nb_args, const char *str);

Re: [Qemu-devel] uvesafb doesn't work with seabios

[Bernhard Walle]

Am 2014-05-11 14:37, schrieb Kevin O'Connor:

On Sun, May 11, 2014 at 01:42:57PM +0200, Bernhard Walle wrote:

Am 10.05.14 17:07, schrieb Kevin O'Connor:
> Also, it looks like uvesafb can call x86emu.  Older versions of x86emu
> are known to not emulate some instructions properly, and it has caused
> problems for SeaVGABIOS.  The most recent version of SeaVGABIOS from
> SeaBIOS git has additional checks for this - can you grab the seabios
> git, set CONFIG_DEBUG_LEVEL to 8, build SeaVGABIOS (set
> CONFIG_VGA_BOCHS), and post the logs from both Linux and QEMU with the
> above options?

It doesn't change the result (that it doesn't work). It only makes the
kernel failing much faster and of course there's much more debugging
output: http://bwalle.de/tmp/debugoutput (250k).


Were there any Linux or userspace messages with the above?



Yes:

  uvesafb: Getting VBE info block failed (eax=0x1d2, err=0)
  uvesafb: vbe_init() failed with -22
  uvesafb: probe of uvesafb.0 failed with error -22



Regards,
Bernhard

Re: [Qemu-devel] [PATCH qom-next 3/4] qom: Implement qom-get HMP command

[Hani Benhabiles]

On Thu, May 08, 2014 at 04:21:16PM +0200, Andreas Färber wrote:
> Reimplement it based on qmp_qom_get() to avoid converting QObjects back
> to strings.
> 
> Inspired-by: Paolo Bonzini 
> Signed-off-by: Andreas Färber 
> ---
>  hmp-commands.hx | 13 +
>  hmp.c   | 22 ++
>  hmp.h   |  1 +
>  3 files changed, 36 insertions(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index a0166af..c0603e9 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1691,6 +1691,19 @@ Print QOM properties of object at location @var{path}
>  ETEXI
>  
>  {
> +.name   = "qom-get",
> +.args_type  = "path:s,property:s",
> +.params = "path property",
> +.help   = "print QOM property",
> +.mhandler.cmd  = hmp_qom_get,
> +},
> +
> +STEXI
> +@item qom-get @var{path} @var{property}
> +Print QOM property @var{property} of object at location @var{path}
> +ETEXI
> +
> +{
>  .name   = "info",
>  .args_type  = "item:s?",
>  .params = "[subcommand]",
> diff --git a/hmp.c b/hmp.c
> index 1c29c8a..d7d7a98 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1693,3 +1693,25 @@ void hmp_qom_list(Monitor *mon, const QDict *qdict)
>  }
>  hmp_handle_error(mon, &err);
>  }
> +
> +void hmp_qom_get(Monitor *mon, const QDict *qdict)
> +{
> +const char *path = qdict_get_str(qdict, "path");
> +const char *property = qdict_get_str(qdict, "property");
> +Error *err = NULL;
> +Object *obj;
> +char *value;
> +
> +obj = object_resolve_path(path, NULL);
> +if (obj == NULL) {
> +error_set(&err, QERR_DEVICE_NOT_FOUND, path);
> +hmp_handle_error(mon, &err);
> +return;
> +}
> +value = object_property_print(obj, property, true, &err);
> +if (err == NULL) {
> +monitor_printf(mon, "%s\n", value);
> +g_free(value);
> +}
> +hmp_handle_error(mon, &err);
> +}
> diff --git a/hmp.h b/hmp.h
> index 7ab969e..269d99e 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -94,6 +94,7 @@ void hmp_cpu_add(Monitor *mon, const QDict *qdict);
>  void hmp_object_add(Monitor *mon, const QDict *qdict);
>  void hmp_object_del(Monitor *mon, const QDict *qdict);
>  void hmp_qom_list(Monitor *mon, const QDict *qdict);
> +void hmp_qom_get(Monitor *mon, const QDict *qdict);
>  void object_add_completion(ReadLineState *rs, int nb_args, const char *str);
>  void object_del_completion(ReadLineState *rs, int nb_args, const char *str);
>  void device_add_completion(ReadLineState *rs, int nb_args, const char *str);

This segfaults:
(qemu) qom-get /machine/unattached/device[9] date
Segmentation fault (core dumped)

And so does
(qemu) qom-get /machine/unattached/device[0] filtered-features
Segmentation fault (core dumped)

See: https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg01975.html

Re: [Qemu-devel] [PATCH qom-next 4/4] qom: Implement qom-set HMP command

[Hani Benhabiles]

On Thu, May 08, 2014 at 04:21:17PM +0200, Andreas Färber wrote:
> Re-implemented based on qmp_qom_set() to facilitate argument parsing.
> 
> Signed-off-by: Andreas Färber 
> ---
>  hmp-commands.hx | 13 +
>  hmp.c   | 18 ++
>  hmp.h   |  1 +
>  3 files changed, 32 insertions(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index c0603e9..73145b7 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1704,6 +1704,19 @@ Print QOM property @var{property} of object at 
> location @var{path}
>  ETEXI
>  
>  {
> +.name   = "qom-set",
> +.args_type  = "path:s,property:s,value:s",
> +.params = "path property value",
> +.help   = "set QOM property",
> +.mhandler.cmd  = hmp_qom_set,
> +},
> +
> +STEXI
> +@item qom-set @var{path} @var{property} @var{value}
> +Set QOM property @var{property} of object at location @var{path} to value 
> @var{value}
> +ETEXI
> +
> +{
>  .name   = "info",
>  .args_type  = "item:s?",
>  .params = "[subcommand]",
> diff --git a/hmp.c b/hmp.c
> index d7d7a98..1cc2e60 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1715,3 +1715,21 @@ void hmp_qom_get(Monitor *mon, const QDict *qdict)
>  }
>  hmp_handle_error(mon, &err);
>  }
> +
> +void hmp_qom_set(Monitor *mon, const QDict *qdict)
> +{
> +const char *path = qdict_get_str(qdict, "path");
> +const char *property = qdict_get_str(qdict, "property");
> +const char *value = qdict_get_str(qdict, "value");
> +Error *err = NULL;
> +Object *obj;
> +
> +obj = object_resolve_path(path, NULL);

Is there any consensus on whether to check for path ambiguity ?

It seems to me that it would be more important to do so here than in
qmp_qom_list() for example.

Other than that, patch looks fine for me.

> +if (obj == NULL) {
> +error_set(&err, QERR_DEVICE_NOT_FOUND, path);
> +hmp_handle_error(mon, &err);
> +return;
> +}
> +object_property_parse(obj, value, property, &err);
> +hmp_handle_error(mon, &err);
> +}
> diff --git a/hmp.h b/hmp.h
> index 269d99e..6b7b1da 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -95,6 +95,7 @@ void hmp_object_add(Monitor *mon, const QDict *qdict);
>  void hmp_object_del(Monitor *mon, const QDict *qdict);
>  void hmp_qom_list(Monitor *mon, const QDict *qdict);
>  void hmp_qom_get(Monitor *mon, const QDict *qdict);
> +void hmp_qom_set(Monitor *mon, const QDict *qdict);
>  void object_add_completion(ReadLineState *rs, int nb_args, const char *str);
>  void object_del_completion(ReadLineState *rs, int nb_args, const char *str);
>  void device_add_completion(ReadLineState *rs, int nb_args, const char *str);
> -- 
> 1.8.4.5
> 
> 

[Qemu-devel] [PATCH] kvm: Fix enable_cap helpers on older gcc

[Alexander Graf]

Commit 40f1ee27aa1 introduced handy helpers for enable_cap calls on
vcpu and vm level. Unfortunately some older gcc versions (4.7.1, 4.6)
seem to choke on signedness detection in inline created variables:

target-ppc/kvm.c: In function 'kvmppc_booke_watchdog_enable':
target-ppc/kvm.c:1302:21: error: comparison of unsigned expression < 0 is 
always false [-Werror=type-limits]
target-ppc/kvm.c: In function 'kvmppc_set_papr':
target-ppc/kvm.c:1504:21: error: comparison of unsigned expression < 0 is 
always false [-Werror=type-limits]

We can easily work around this by not doing a compare with 0, but instead
shift all values up by one. The resulting binary should look the same, as
all those ARRAY_SIZE values are compile time statically optimized.

This patch fixes compile errors on some of my systems.

Signed-off-by: Alexander Graf 
---
 include/sysemu/kvm.h | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 192fe89..989c261 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -302,9 +302,9 @@ int kvm_check_extension(KVMState *s, unsigned int 
extension);
 };   \
 uint64_t args_tmp[] = { __VA_ARGS__ };   \
 int i;   \
-for (i = 0; i < ARRAY_SIZE(args_tmp) &&  \
- i < ARRAY_SIZE(cap.args); i++) {\
-cap.args[i] = args_tmp[i];   \
+for (i = 1; i < (MIN(ARRAY_SIZE(args_tmp),   \
+ ARRAY_SIZE(cap.args)) + 1); i++) {  \
+cap.args[i - 1] = args_tmp[i - 1];   \
 }\
 kvm_vm_ioctl(s, KVM_ENABLE_CAP, &cap);   \
 })
@@ -317,9 +317,9 @@ int kvm_check_extension(KVMState *s, unsigned int 
extension);
 };   \
 uint64_t args_tmp[] = { __VA_ARGS__ };   \
 int i;   \
-for (i = 0; i < ARRAY_SIZE(args_tmp) &&  \
- i < ARRAY_SIZE(cap.args); i++) {\
-cap.args[i] = args_tmp[i];   \
+for (i = 1; i < (MIN(ARRAY_SIZE(args_tmp),   \
+ ARRAY_SIZE(cap.args)) + 1); i++) {  \
+cap.args[i - 1] = args_tmp[i - 1];   \
 }\
 kvm_vcpu_ioctl(cpu, KVM_ENABLE_CAP, &cap);   \
 })
-- 
1.8.1.4

[Qemu-devel] [Bug 1318281] [NEW] linux-user: x86_64 target fails to call sys_futex()

[Jiajie Hu]

Public bug reported:

I'm building the latest QEMU (06b4f00d53637f2c16a62c2cbaa30bffb045cf88)
on ARM to run some x86_64 executables in user mode. This is my
configuration:

./configure \
  --prefix=/root/qemu-x86_64 \
  --target-list=x86_64-linux-user \
  --disable-system \
  --disable-tools

The following program is used for testing:

https://gist.github.com/hujiajie/e8cff43b574b399c8f59#file-test-c

I compile the test program in Debian-7.5-amd64 like this:

gcc -o test `pkg-config --cflags glib-2.0` test.c `pkg-config --static
--libs glib-2.0` -static

and launch the program on ARM with

qemu-x86_64 test

The test crashes with the following message:

qemu: uncaught target signal 11 (Segmentation fault) - core dumped
Segmentation fault

The output of `strace qemu-x86_64 test` is here:

https://gist.github.com/hujiajie/88d1d5e580d432d11b2d#file-test-strace-
log

It seems that the error is caused by the failure of the futex syscall.

qemu-i386 could launch the 32-bit test perfectly, the problem only
happens on a x86_64 target.

** Affects: qemu
 Importance: Undecided
 Status: New

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

Title:
  linux-user: x86_64 target fails to call sys_futex()

Status in QEMU:
  New

Bug description:
  I'm building the latest QEMU
  (06b4f00d53637f2c16a62c2cbaa30bffb045cf88) on ARM to run some x86_64
  executables in user mode. This is my configuration:

  ./configure \
--prefix=/root/qemu-x86_64 \
--target-list=x86_64-linux-user \
--disable-system \
--disable-tools

  The following program is used for testing:

  https://gist.github.com/hujiajie/e8cff43b574b399c8f59#file-test-c

  I compile the test program in Debian-7.5-amd64 like this:

  gcc -o test `pkg-config --cflags glib-2.0` test.c `pkg-config --static
  --libs glib-2.0` -static

  and launch the program on ARM with

  qemu-x86_64 test

  The test crashes with the following message:

  qemu: uncaught target signal 11 (Segmentation fault) - core dumped
  Segmentation fault

  The output of `strace qemu-x86_64 test` is here:

  https://gist.github.com/hujiajie/88d1d5e580d432d11b2d#file-test-
  strace-log

  It seems that the error is caused by the failure of the futex syscall.

  qemu-i386 could launch the 32-bit test perfectly, the problem only
  happens on a x86_64 target.

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

[Qemu-devel] [PATCH] KVM: PPC: Don't secretly add 1T segment feature to CPU

[Alexander Graf]

When we select a CPU type that does not support 1TB segments, we should
not expose 1TB just because KVM supports 1TB segments. User configuration
always wins over feature availability.

Signed-off-by: Alexander Graf 
---
 target-ppc/kvm.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index d988055..ff319fc 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -356,6 +356,10 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu)
 /* Convert to QEMU form */
 memset(&env->sps, 0, sizeof(env->sps));
 
+/*
+ * XXX This loop should be an entry wide AND of the capabilities that
+ * the selected CPU has with the capabilities that KVM supports.
+ */
 for (ik = iq = 0; ik < KVM_PPC_PAGE_SIZES_MAX_SZ; ik++) {
 struct ppc_one_seg_page_size *qsps = &env->sps.sps[iq];
 struct kvm_ppc_one_seg_page_size *ksps = &smmu_info.sps[ik];
@@ -382,9 +386,7 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu)
 }
 }
 env->slb_nr = smmu_info.slb_size;
-if (smmu_info.flags & KVM_PPC_1T_SEGMENTS) {
-env->mmu_model |= POWERPC_MMU_1TSEG;
-} else {
+if (!(smmu_info.flags & KVM_PPC_1T_SEGMENTS)) {
 env->mmu_model &= ~POWERPC_MMU_1TSEG;
 }
 }
-- 
1.8.1.4

[Qemu-devel] [RFC v2 0/4] AMBA platform device passthrough

[Alvise Rigo]

These patches are the next iteration of "[Qemu-devel] [RFC 0/4] AMBA
platform device passthrough" and are based on the latest version of VFIO
for platform devices "[RFC PATCH v5 00/11] VFIO support for platform
devices" and the initial patch series "[Qemu-devel] [RFC v2 0/6] KVM
platform device passthrough".

- [PATCH 1/4] Addition of the exec flag to the DMA mappings. At the
  moment this is mandatory only for the ARM SMMU.
  Change since v1:
  Thanks to the new version of the VFIO API, now we can know from the
  kernel if the IOMMU requires the EXEC_FLAG.  For now, we always add it
  if supported.

- [PATCH 2/4] Possibility to bind a wider class of devices. In
  particular the patch proposes a solution to enhance a bit the device
  tree nodes generation, allowing to specify more than one compatibility
  property (handy for AMBA devices). This was required by the DMA330
  that needs "arm,pl330","arm,primecell" as compatibility string,
  together with a clock source defined in the device tree. In the future
  VFIO will be able to tell if we are dealing with an AMBA device;
  before that, we have to rely on the compatibility string to state
  that.

- [PATCH 3/4] Another approach to handle the EOI, starting from the
  assumption that we know in advance the location of the interrupt clear
  register of the device. This is of course another workaround and not
  the definitive solution until we can rely on a proper callback from
  the VGIC (something that we will see in a future version of VFIO for
  platform devices).
  Change since v1:
  The code is much simpler now and does not require the definition of an
  additional memory region which would be dma-mapped by the memory
  listener vfio_listener_region_add in hw/vfio/common.c. In this regard
  probably soon or later we will need a way to exclude some regions from
  being mapped "blindly" by the memory listener: this is for example
  what happens with the memory region of the device which is dma-mapped
  to itself.

- [PATCH 4/4] If event_notifier_init fails to create a new eventfd, it
  will fallback usign pipe/pipe2 and we end up passing to the kernel a
  wrong file descriptor. This patch force to check if eventfd has been
  used.

In order to easy the test of the VFIO API with platform devices, here:
https://github.com/virtualopensystems/vfio-host-test.git
is possible to find a handy test suite for an ARM host which will print the
devices that could be probed with VFIO and run some basic tests on a chosen
device. If the ARM DMA330 is tested, a device specific code (which was
introduced in the Virtual Open Systems guide
http://virtualopensystems.com/en/solutions/guides/vfio-on-arm) will be run to
verify even further the correct behaviour of the device. This suite has been
tested with ARM FastModels.

Alvise Rigo (4):
  Add EXEC_FLAG to VFIO DMA mappings
  Add AMBA devices support to VFIO
  MemoryRegion with EOI callbacks for VFIO Platform devices
  Always use eventfd as notifying mechanism

 hw/arm/virt.c  | 39 ---
 hw/vfio/common.c   |  9 +
 hw/vfio/platform.c | 96 +-
 hw/vfio/vfio-common.h  |  1 +
 linux-headers/linux/vfio.h |  2 +
 5 files changed, 140 insertions(+), 7 deletions(-)

-- 
1.9.1

[Qemu-devel] [RFC v2 1/4] Add EXEC_FLAG to VFIO DMA mappings

[Alvise Rigo]

The flag is mandatory for the ARM SMMU so we always add it if the MMIO
handles it.

Signed-off-by: Alvise Rigo 
---
 hw/vfio/common.c   | 9 +
 hw/vfio/vfio-common.h  | 1 +
 linux-headers/linux/vfio.h | 2 ++
 3 files changed, 12 insertions(+)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 9d1f723..a805c5d 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -107,6 +107,11 @@ static int vfio_dma_map(VFIOContainer *container, hwaddr 
iova,
 map.flags |= VFIO_DMA_MAP_FLAG_WRITE;
 }
 
+/* add exec flag */
+if (container->iommu_data.has_exec_cap) {
+map.flags |= VFIO_DMA_MAP_FLAG_EXEC;
+}
+
 /*
  * Try the mapping, if it fails with EBUSY, unmap the region and try
  * again.  This shouldn't be necessary, but we sometimes see it in
@@ -352,6 +357,10 @@ static int vfio_connect_container(VFIOGroup *group)
 return -errno;
 }
 
+if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_IOMMU_PROT_EXEC)) {
+container->iommu_data.has_exec_cap = true;
+}
+
 container->iommu_data.type1.listener = vfio_memory_listener;
 container->iommu_data.release = vfio_listener_release;
 
diff --git a/hw/vfio/vfio-common.h b/hw/vfio/vfio-common.h
index 21148ef..1abbd1a 100644
--- a/hw/vfio/vfio-common.h
+++ b/hw/vfio/vfio-common.h
@@ -35,6 +35,7 @@ typedef struct VFIOContainer {
 union {
 VFIOType1 type1;
 };
+bool has_exec_cap; /* support of exec capability by the IOMMU */
 void (*release)(struct VFIOContainer *);
 } iommu_data;
 QLIST_HEAD(, VFIOGroup) group_list;
diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
index 17c58e0..95a02c5 100644
--- a/linux-headers/linux/vfio.h
+++ b/linux-headers/linux/vfio.h
@@ -24,6 +24,7 @@
 #define VFIO_TYPE1_IOMMU   1
 #define VFIO_SPAPR_TCE_IOMMU   2
 
+#define VFIO_IOMMU_PROT_EXEC   5
 /*
  * The IOCTL interface is designed for extensibility by embedding the
  * structure length (argsz) and flags into structures passed between
@@ -392,6 +393,7 @@ struct vfio_iommu_type1_dma_map {
__u32   flags;
 #define VFIO_DMA_MAP_FLAG_READ (1 << 0)/* readable from device 
*/
 #define VFIO_DMA_MAP_FLAG_WRITE (1 << 1)   /* writable from device */
+#define VFIO_DMA_MAP_FLAG_EXEC (1 << 2)/* executable from device */
__u64   vaddr;  /* Process virtual address */
__u64   iova;   /* IO virtual address */
__u64   size;   /* Size of mapping (bytes) */
-- 
1.9.1

[Qemu-devel] [RFC v2 2/4] Add AMBA devices support to VFIO

[Alvise Rigo]

The impossibility to add more then one compatibility property to the
device tree node was not permitting to bind AMBA devices.
Now we can add an arbitrary number of compatibility property values divided by
the character ";".

If the compatibility string contains the substring "arm,primecell", a
clock property will be added to the device tree node in order to allow the AMBA
bus code to probe the device.

Signed-off-by: Alvise Rigo 
---
 hw/arm/virt.c | 39 ++-
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 1fb66ef..dadf5f0 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -344,6 +344,8 @@ static int vfio_init_func(QemuOpts *opts, void *opaque)
 size_t size;
 int i;
 uint32_t *irq_attr;
+bool is_amba = false;
+int compat_str_len;
 
 if (!driver) {
 qerror_report(QERR_MISSING_PARAMETER, "driver");
@@ -369,12 +371,31 @@ static int vfio_init_func(QemuOpts *opts, void *opaque)
 
 /*
  * process compatibility property string passed by end-user
- * replaces / by ,
- * currently a single property compatibility value is supported!
+ * replaces / by , and ; by NUL character
  */
 corrected_compat = g_strdup(*pcompat);
-char *slash = strchr(corrected_compat, '/');
-*slash = ',';
+/*
+ * the total length of the string has to include also the last
+ * NUL char.
+ */
+compat_str_len = strlen(corrected_compat) + 1;
+
+char *str_ptr = corrected_compat;
+while ((str_ptr = strchr(str_ptr, '/')) != NULL) {
+*str_ptr = ',';
+}
+
+/* check if is an AMBA device */
+str_ptr = corrected_compat;
+if (strstr(str_ptr, "arm,primecell") != NULL) {
+is_amba = true;
+}
+
+/* substitute ";" with the NUL char */
+str_ptr = corrected_compat;
+while ((str_ptr = strchr(str_ptr, ';')) != NULL) {
+*str_ptr = '\0';
+}
 
 sysbus_mmio_map(s, 0, vbi->avail_vfio_base);
 
@@ -383,11 +404,19 @@ static int vfio_init_func(QemuOpts *opts, void *opaque)
 qemu_fdt_add_subnode(vbi->fdt, nodename);
 
 qemu_fdt_setprop(vbi->fdt, nodename, "compatible",
- corrected_compat, strlen(corrected_compat));
+ corrected_compat, compat_str_len);
 
 qemu_fdt_setprop_sized_cells(vbi->fdt, nodename,
  "reg", 2, vbi->avail_vfio_base, 2, size);
 
+if (is_amba) {
+qemu_fdt_setprop_cells(vbi->fdt, nodename, "clocks",
+   vbi->clock_phandle);
+char clock_names[] = "apb_pclk";
+qemu_fdt_setprop(vbi->fdt, nodename, "clock-names", clock_names,
+   sizeof(clock_names));
+}
+
 irq_attr = g_malloc0(num_irqs*3*sizeof(uint32_t));
 for (i = 0; i < num_irqs; i++) {
 sysbus_connect_irq(s, i, vbi->pic[irq_start+i]);
-- 
1.9.1

[Qemu-devel] [RFC v2 3/4] MemoryRegion with EOI callbacks for VFIO Platform devices

[Alvise Rigo]

The user can specify the location of the memory region (register) used
by the guest driver to clear the pending interrupt; if enabled, this
mechanism will overlap to the default one (timer based).

The region is provided as command line property "intclr-region" of the
vfio-platform device. The property is a string "region_index;offset;size" where:

region_index:   is the index of the memory region where the register
lives,
offset: offset of the register in the region,
size:   size of the register.

example:
-device vfio-platform,...,intclr-region="0;0x2c;4"

Signed-off-by: Alvise Rigo 
---
 hw/vfio/platform.c | 91 --
 1 file changed, 89 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index c4a4286..ec6a29e 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -40,6 +40,7 @@
 
 #include "vfio-common.h"
 
+/*#define DEBUG_VFIO 1*/
 #ifdef DEBUG_VFIO
 #define DPRINTF(fmt, ...) \
 do { fprintf(stderr, "vfio: %s: " fmt, __func__, ## __VA_ARGS__); } \
@@ -56,6 +57,11 @@
 
 #define TYPE_VFIO_PLATFORM "vfio-platform"
 
+struct intclr_region {
+hwaddr offset;
+hwaddr size;
+};
+
 typedef struct VFIORegion {
 off_t fd_offset; /* offset of region within device fd */
 int fd; /* device fd, allows us to pass VFIORegion as opaque data */
@@ -65,6 +71,7 @@ typedef struct VFIORegion {
 size_t size;
 uint32_t flags; /* VFIO region flags (rd/wr/mmap) */
 uint8_t nr; /* cache the region number for debug */
+struct intclr_region intclr_reg;
 } VFIORegion;
 
 
@@ -99,6 +106,8 @@ typedef struct VFIODevice {
 QLIST_ENTRY(VFIODevice) next;
 struct VFIOGroup *group;
 QLIST_HEAD(, VFIOINTp) intp_list;
+char *intclr_region_str; /* clear interrupt region string */
+bool has_intclr_region;
 } VFIODevice;
 
 
@@ -120,6 +129,48 @@ void vfio_get_props(SysBusDevice *s, char **pname,
  *psize = vdev->regions[0].size;
 }
 
+static int parse_clrint_string(const char *intclr_str, uint32_t *id_reg,
+hwaddr *off_reg, uint64_t *size_reg)
+{
+char *str_ptr = g_strdup(intclr_str);
+char *idx, *off, *size;
+
+if (strlen(intclr_str) < 5) {
+return -1;
+}
+
+idx = str_ptr;
+str_ptr = strchr(str_ptr, ';');
+if (!str_ptr || idx == str_ptr) {
+return -1;
+}
+*str_ptr = '\0';
+
+off = ++str_ptr;
+str_ptr = strchr(str_ptr, ';');
+if (!str_ptr || off == str_ptr) {
+return -1;
+}
+*str_ptr = '\0';
+
+size = ++str_ptr;
+if (!*size) {
+return -1;
+}
+
+*id_reg = strtol(idx, NULL, 10);
+*off_reg = strtol(off, NULL, 0);
+*size_reg = strtol(size, NULL, 10);
+
+if (errno == EINVAL || errno == ERANGE) {
+return -1;
+}
+
+DPRINTF("intclr region - id: %u offset: 0x%"HWADDR_PRIx" size: 
%"PRIx64"\n",
+*id_reg, *off_reg, *size_reg);
+return 0;
+}
+
 static void vfio_disable_irqindex(VFIODevice *vdev, int index)
 {
 struct vfio_irq_set irq_set = {
@@ -397,6 +448,8 @@ static void vfio_region_write(void *opaque, hwaddr addr,
   uint64_t data, unsigned size)
 {
 VFIORegion *region = opaque;
+VFIODevice *vdev = NULL;
+
 union {
 uint8_t byte;
 uint16_t word;
@@ -427,13 +480,22 @@ static void vfio_region_write(void *opaque, hwaddr addr,
 DPRINTF("(region %d, addr=0x%"HWADDR_PRIx", data= 0x%"PRIx64", %d)\n",
 region->nr, addr, data, size);
 
-vfio_irq_eoi(container_of(region, VFIODevice, regions[region->nr]));
+vdev = container_of(region, VFIODevice, regions[region->nr]);
+struct intclr_region *intr = ®ion->intclr_reg;
+/* If an interrupt clear region has been specified we clear the pending
+ * intterrupts only when the memory accesse is inside the region.
+ * */
+if ((addr >= intr->offset && addr + size <= intr->offset + intr->size)
+|| !vdev->has_intclr_region) {
+vfio_irq_eoi(vdev);
+}
 
 }
 
 static uint64_t vfio_region_read(void *opaque, hwaddr addr, unsigned size)
 {
 VFIORegion *region = opaque;
+VFIODevice *vdev = NULL;
 union {
 uint8_t byte;
 uint16_t word;
@@ -466,7 +528,10 @@ static uint64_t vfio_region_read(void *opaque, hwaddr 
addr, unsigned size)
 DPRINTF("(region %d, addr= 0x%"HWADDR_PRIx", data=%d) = 0x%"PRIx64"\n",
 region->nr, addr, size, data);
 
-vfio_irq_eoi(container_of(region, VFIODevice, regions[region->nr]));
+vdev = container_of(region, VFIODevice, regions[region->nr]);
+if (!vdev->has_intclr_region) {
+vfio_irq_eoi(vdev);
+}
 
 return data;
 }
@@ -669,6 +734,17 @@ static void vfio_platform_realize(DeviceState *dev, Error 
**errp)
 }
 }
 
+hwaddr intclr_off = 0;
+uint64_t intclr_siz

[Qemu-devel] [RFC v2 4/4] Always use eventfd as notifying mechanism

[Alvise Rigo]

When eventfd is not configured the method event_notifier_init fallbacks
to the pipe/pipe2 system call, causing an error in VFIO_DEVICE_SET_IRQS
since we pass to the kernel a file descriptor which is not created by
eventfd.

Signed-off-by: Alvise Rigo 
---
 hw/vfio/platform.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index ec6a29e..40109c6 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -324,6 +324,11 @@ static int vfio_enable_intp(VFIODevice *vdev, unsigned int 
index)
 sysbus_init_irq(sbdev, &intp->qemuirq);
 
 ret = event_notifier_init(&intp->interrupt, 0);
+if (!ret && (intp->interrupt.rfd != intp->interrupt.wfd)) {
+/* event_notifier_init created a pipe instead of eventfd */
+ret = -1;
+}
+
 if (ret) {
 error_report("vfio: Error: event_notifier_init failed ");
 return ret;
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2] block/raw-posix: Try both FIEMAP and SEEK_HOLE

[Christoph Hellwig]

On Tue, May 06, 2014 at 09:00:54PM +0200, Max Reitz wrote:
> The current version of raw-posix always uses ioctl(FS_IOC_FIEMAP) if
> FIEMAP is available; lseek with SEEK_HOLE/SEEK_DATA are not even
> compiled in in this case. However, there may be implementations which
> support the latter but not the former (e.g., NFSv4.2) as well as vice
> versa.
> 
> To cover both cases, always try SEEK_HOLE/SEEK_DATA (as this will
> probably be covered by POSIX soon) and if that does not work, fall back
> to FIEMAP; and if that does not work either, treat everything as
> allocated.

Btw, while I think that SEEK_HOLE/SEEK_DATA generally is the better API
for qemu, the NFS 4.2 SEEK operation will be sufficient for a proper FIEMAP
implementation, and we'll implement it for the Linux NFS client.

[Qemu-devel] [PATCH v4 0/2] Named GPIOs

[Peter Crosthwaite]

This series implements Named GPIOs on the qdev level. Patch 1 is the
feature presentation.

Second patch I give a useful example of the feature's use (an SSI
cleanup).

Changed since v3:
Addressed PMM v2 review
Changed since v2:
Re-ordered function args (PMM review)
Changed since v1:
use QLIST (AF review)
Dropped former P1 (too far out of scope)


Peter Crosthwaite (2):
  qdev: Implement named GPIOs
  ssi: Name the CS GPIO.

 hw/arm/stellaris.c  |  7 ++--
 hw/arm/xilinx_zynq.c|  2 +-
 hw/core/qdev.c  | 82 -
 hw/microblaze/petalogix_ml605_mmu.c |  2 +-
 hw/ssi/ssi.c|  4 +-
 include/hw/qdev-core.h  | 24 +--
 include/hw/ssi.h|  2 +
 qdev-monitor.c  | 16 +---
 qtest.c | 19 ++---
 9 files changed, 127 insertions(+), 31 deletions(-)

-- 
1.9.2.1.g06c4abd

[Qemu-devel] [PATCH v4 1/2] qdev: Implement named GPIOs

[Peter Crosthwaite]

Implement named GPIOs on the Device layer. Listifies the existing GPIOs
stuff using string keys. Legacy un-named GPIOs are preserved by using
a NULL name string - they are just a single matchable element in the
name list.

Signed-off-by: Peter Crosthwaite 
---
Changed since v3 (PMM review):
Added finalize() cleanup of all allocated mem
Remove %s printf of NULL in info qtree
Restricted Qtest to unnamed GPIOs only
Changed since v2:
Reordered fn args to be name-before-number (PMM review)
changed since v1:
Use QLIST instead of RYO list implementation (AF review)
Reorder struct fields for consistency

 hw/core/qdev.c | 82 --
 include/hw/qdev-core.h | 24 ---
 qdev-monitor.c | 16 +++---
 qtest.c| 19 +---
 4 files changed, 117 insertions(+), 24 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 936eae6..8efeb04 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -312,30 +312,82 @@ BusState *qdev_get_parent_bus(DeviceState *dev)
 return dev->parent_bus;
 }
 
+static NamedGPIOList *qdev_get_named_gpio_list(DeviceState *dev,
+   const char *name)
+{
+NamedGPIOList *ngl;
+
+QLIST_FOREACH(ngl, &dev->gpios, node) {
+/* NULL is a valid and matchable name, otherwise do a normal
+ * strcmp match.
+ */
+if ((!ngl->name && !name) ||
+(name && ngl->name && !strcmp(name, ngl->name))) {
+return ngl;
+}
+}
+
+ngl = g_malloc0(sizeof(*ngl));
+ngl->name = g_strdup(name);
+QLIST_INSERT_HEAD(&dev->gpios, ngl, node);
+return ngl;
+}
+
+void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
+ const char *name, int n)
+{
+NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+
+gpio_list->in = qemu_extend_irqs(gpio_list->in, gpio_list->num_in, handler,
+ dev, n);
+gpio_list->num_in += n;
+}
+
 void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n)
 {
-dev->gpio_in = qemu_extend_irqs(dev->gpio_in, dev->num_gpio_in, handler,
-dev, n);
-dev->num_gpio_in += n;
+qdev_init_gpio_in_named(dev, handler, NULL, n);
+}
+
+void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
+  const char *name, int n)
+{
+NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+
+assert(gpio_list->num_out == 0);
+gpio_list->num_out = n;
+gpio_list->out = pins;
 }
 
 void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n)
 {
-assert(dev->num_gpio_out == 0);
-dev->num_gpio_out = n;
-dev->gpio_out = pins;
+qdev_init_gpio_out_named(dev, pins, NULL, n);
+}
+
+qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n)
+{
+NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+
+assert(n >= 0 && n < gpio_list->num_in);
+return gpio_list->in[n];
 }
 
 qemu_irq qdev_get_gpio_in(DeviceState *dev, int n)
 {
-assert(n >= 0 && n < dev->num_gpio_in);
-return dev->gpio_in[n];
+return qdev_get_gpio_in_named(dev, NULL, n);
+}
+
+void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
+ qemu_irq pin)
+{
+NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+
+assert(n >= 0 && n < gpio_list->num_out);
+gpio_list->out[n] = pin;
 }
 
 void qdev_connect_gpio_out(DeviceState * dev, int n, qemu_irq pin)
 {
-assert(n >= 0 && n < dev->num_gpio_out);
-dev->gpio_out[n] = pin;
+qdev_connect_gpio_out_named(dev, NULL, n, pin);
 }
 
 BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
@@ -844,6 +896,7 @@ static void device_initfn(Object *obj)
 object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
  (Object **)&dev->parent_bus, NULL, 0,
  &error_abort);
+QLIST_INIT(&dev->gpios);
 }
 
 static void device_post_init(Object *obj)
@@ -854,10 +907,19 @@ static void device_post_init(Object *obj)
 /* Unlink device from bus and free the structure.  */
 static void device_finalize(Object *obj)
 {
+NamedGPIOList *ngl, *next;
+
 DeviceState *dev = DEVICE(obj);
 if (dev->opts) {
 qemu_opts_del(dev->opts);
 }
+
+QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) {
+QLIST_REMOVE(ngl, node);
+qemu_free_irqs(ngl->in);
+g_free(ngl->name);
+g_free(ngl);
+}
 }
 
 static void device_class_base_init(ObjectClass *class, void *data)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index dbe473c..ae31575 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -131,6 +131,17 @@ typedef struct DeviceClass {
 const char *bus_type;
 } DeviceClass;
 
+typedef struct NamedGPIOList NamedGPIOList;
+
+struct N

[Qemu-devel] [PATCH v4 2/2] ssi: Name the CS GPIO.

[Peter Crosthwaite]

To get it out of the default GPIO list. This allows child devices to
use the un-named GPIO namespace without having to be SSI aware. That
is, there is no more need for machines to know about the obscure
policy where GPIO 0 is the SSI chip-select and GPIO 1..N are the
concrete class GPIOs (defined locally as 0..N-1).

This is most notable is stellaris, which uses a device which has both
SSI and concrete level GPIOs.

Signed-off-by: Peter Crosthwaite 
Reviewed-by: Peter Maydell 
---

 hw/arm/stellaris.c  | 7 ---
 hw/arm/xilinx_zynq.c| 2 +-
 hw/microblaze/petalogix_ml605_mmu.c | 2 +-
 hw/ssi/ssi.c| 4 ++--
 include/hw/ssi.h| 2 ++
 5 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index d6cc77b..b37669a 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -1287,9 +1287,10 @@ static void stellaris_init(const char *kernel_filename, 
const char *cpu_model,
 
 sddev = ssi_create_slave(bus, "ssi-sd");
 ssddev = ssi_create_slave(bus, "ssd0323");
-gpio_out[GPIO_D][0] = qemu_irq_split(qdev_get_gpio_in(sddev, 0),
- qdev_get_gpio_in(ssddev, 0));
-gpio_out[GPIO_C][7] = qdev_get_gpio_in(ssddev, 1);
+gpio_out[GPIO_D][0] = qemu_irq_split(
+qdev_get_gpio_in_named(sddev, SSI_GPIO_CS, 0),
+qdev_get_gpio_in_named(ssddev, SSI_GPIO_CS, 0));
+gpio_out[GPIO_C][7] = qdev_get_gpio_in(ssddev, 0);
 
 /* Make sure the select pin is high.  */
 qemu_irq_raise(gpio_out[GPIO_D][0]);
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 9ee21e7..1ce7b79 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -94,7 +94,7 @@ static inline void zynq_init_spi_flashes(uint32_t base_addr, 
qemu_irq irq,
 for (j = 0; j < num_ss; ++j) {
 flash_dev = ssi_create_slave(spi, "n25q128");
 
-cs_line = qdev_get_gpio_in(flash_dev, 0);
+cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
 sysbus_connect_irq(busdev, i * num_ss + j + 1, cs_line);
 }
 }
diff --git a/hw/microblaze/petalogix_ml605_mmu.c 
b/hw/microblaze/petalogix_ml605_mmu.c
index 40a9f5c..3d7aa6e 100644
--- a/hw/microblaze/petalogix_ml605_mmu.c
+++ b/hw/microblaze/petalogix_ml605_mmu.c
@@ -196,7 +196,7 @@ petalogix_ml605_init(QEMUMachineInitArgs *args)
 qemu_irq cs_line;
 
 dev = ssi_create_slave(spi, "n25q128");
-cs_line = qdev_get_gpio_in(dev, 0);
+cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
 sysbus_connect_irq(busdev, i+1, cs_line);
 }
 }
diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
index 017f022..47c188a 100644
--- a/hw/ssi/ssi.c
+++ b/hw/ssi/ssi.c
@@ -60,7 +60,7 @@ static int ssi_slave_init(DeviceState *dev)
 
 if (ssc->transfer_raw == ssi_transfer_raw_default &&
 ssc->cs_polarity != SSI_CS_NONE) {
-qdev_init_gpio_in(dev, ssi_cs_default, 1);
+qdev_init_gpio_in_named(dev, ssi_cs_default, SSI_GPIO_CS, 1);
 }
 
 return ssc->init(s);
@@ -156,7 +156,7 @@ static int ssi_auto_connect_slave(Object *child, void 
*opaque)
 return 0;
 }
 
-cs_line = qdev_get_gpio_in(DEVICE(dev), 0);
+cs_line = qdev_get_gpio_in_named(DEVICE(dev), SSI_GPIO_CS, 0);
 qdev_set_parent_bus(DEVICE(dev), BUS(arg->bus));
 **arg->cs_linep = cs_line;
 (*arg->cs_linep)++;
diff --git a/include/hw/ssi.h b/include/hw/ssi.h
index 6c13fb2..df0f838 100644
--- a/include/hw/ssi.h
+++ b/include/hw/ssi.h
@@ -23,6 +23,8 @@ typedef struct SSISlave SSISlave;
 #define SSI_SLAVE_GET_CLASS(obj) \
  OBJECT_GET_CLASS(SSISlaveClass, (obj), TYPE_SSI_SLAVE)
 
+#define SSI_GPIO_CS "ssi-gpio-cs"
+
 typedef enum {
 SSI_CS_NONE = 0,
 SSI_CS_LOW,
-- 
1.9.2.1.g06c4abd

Re: [Qemu-devel] [PATCH v3 1/2] qdev: Implement named GPIOs

[Peter Crosthwaite]

On Sat, May 10, 2014 at 2:52 AM, Peter Maydell  wrote:
> On 8 May 2014 07:59, Peter Crosthwaite  wrote:
>> Implement named GPIOs on the Device layer. Listifies the existing GPIOs
>> stuff using string keys. Legacy un-named GPIOs are preserved by using
>> a NULL name string - they are just a single matchable element in the
>> name list.
>>
>> Signed-off-by: Peter Crosthwaite 
>> ---
>> Changed since v2:
>> Reordered fn args to be name-before-number (PMM review)
>> changed since v1:
>> Use QLIST instead of RYO list implementation (AF review)
>> Reorder struct fields for consistency
>>
>>  hw/core/qdev.c | 70 
>> ++
>>  include/hw/qdev-core.h | 24 ++---
>>  qdev-monitor.c | 14 ++
>>  qtest.c| 15 +++
>>  4 files changed, 99 insertions(+), 24 deletions(-)
>
> So, first of all: having thought a bit about it I think we
> should go ahead with the approach this patch uses. Even
> if we later come up with some fancy QOM-object based way
> of doing GPIO lines, it should be easy enough to convert
> or put in back-compatible functions, and we'll be ahead
> if we've already done the effort of splitting single GPIO
> arrays into more sensible multiple named arrays.
>
> Some review follows.
>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 936eae6..8b54db2 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -312,30 +312,79 @@ BusState *qdev_get_parent_bus(DeviceState *dev)
>>  return dev->parent_bus;
>>  }
>>
>> +static NamedGPIOList *qdev_get_named_gpio_list(DeviceState *dev,
>> +   const char *name)
>> +{
>> +NamedGPIOList *ngl;
>> +
>> +QLIST_FOREACH(ngl, &dev->gpios, node) {
>> +if (ngl->name == name ||
>> +(name && ngl->name && !strcmp(name, ngl->name))) {
>
> This slightly odd looking check is because NULL is a
> valid name, yes? That could probably use a comment.
>

Added a comment. Also change conditional to be more self documenting
around NULL matching:

if (!(ngl->name && !name) ||

>> +return ngl;
>> +}
>> +}
>> +
>> +ngl = g_malloc0(sizeof(*ngl));
>> +ngl->name = g_strdup(name);
>
> We're going to leak these when the device is deinited.
>
> The current gpio handling code is not exactly an airtight
> leakfree design, but let's see if we can improve things
> while we're here.
>
> I think device_finalize() needs to:
>  * iterate through the gpio list array
>  ** call qemu_free_irqs(ngl->in)
>  ** don't do anything with ngl->out because the caller of
> qdev_init_gpio_out() owns that [usually embedded in their
> QOM object struct]
>  ** free ngl->name
>  ** free the ngl node itself
>

Implemented.

>> +QLIST_INSERT_HEAD(&dev->gpios, ngl, node);
>> +return ngl;
>> +}
>> +
>> +void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
>> + const char *name, int n)
>> +{
>> +NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
>> +
>> +gpio_list->in = qemu_extend_irqs(gpio_list->in, gpio_list->num_in, 
>> handler,
>> + dev, n);
>> +gpio_list->num_in += n;
>> +}
>> +
>>  void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n)
>>  {
>> -dev->gpio_in = qemu_extend_irqs(dev->gpio_in, dev->num_gpio_in, handler,
>> -dev, n);
>> -dev->num_gpio_in += n;
>> +qdev_init_gpio_in_named(dev, handler, NULL, n);
>> +}
>> +
>> +void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
>> +  const char *name, int n)
>> +{
>> +NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
>> +
>> +assert(gpio_list->num_out == 0);
>> +gpio_list->num_out = n;
>> +gpio_list->out = pins;
>>  }
>>
>>  void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n)
>>  {
>> -assert(dev->num_gpio_out == 0);
>> -dev->num_gpio_out = n;
>> -dev->gpio_out = pins;
>> +qdev_init_gpio_out_named(dev, pins, NULL, n);
>> +}
>> +
>> +qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n)
>> +{
>> +NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
>> +
>> +assert(n >= 0 && n < gpio_list->num_in);
>> +return gpio_list->in[n];
>>  }
>>
>>  qemu_irq qdev_get_gpio_in(DeviceState *dev, int n)
>>  {
>> -assert(n >= 0 && n < dev->num_gpio_in);
>> -return dev->gpio_in[n];
>> +return qdev_get_gpio_in_named(dev, NULL, n);
>> +}
>> +
>> +void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
>> + qemu_irq pin)
>> +{
>> +NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
>> +
>> +assert(n >= 0 && n < gpio_list->num_out);
>> +gpio_list->out[n] = pin;
>>  }
>>
>>  void qdev_connect_gpio_out(DeviceState * dev, int n, qemu_irq pin)
>>  {
>> -assert(n

Re: [Qemu-devel] [PATCH 3/4] block: replace fprintf(stderr, ...) with error_report() in block/

[Peter Crosthwaite]

On Sun, May 11, 2014 at 11:27 PM, Le Tan  wrote:
> 2014-05-10 21:18 GMT+08:00 Peter Crosthwaite :
>> On Sat, May 10, 2014 at 9:55 AM, Le Tan  wrote:
>>> Replace fprintf(stderr,...) with error_report() in files block/*, block.c,
>>> block-migration.c and blockdev.c. The trailing "\n"s of the @fmt argument
>>> have been removed because @fmt of error_report() should not contain newline.
>>>
>>> Signed-off-by: Le Tan 
>>> ---
>>>  block-migration.c  |4 +-
>>>  block.c|4 +-
>>>  block/linux-aio.c  |4 +-
>>>  block/nbd-client.h |2 +-
>>>  block/qcow2-cluster.c  |4 +-
>>>  block/qcow2-refcount.c |  114 
>>> 
>>>  block/qcow2.c  |   16 +++
>>>  block/quorum.c |4 +-
>>>  block/raw-posix.c  |8 ++--
>>>  block/raw-win32.c  |6 +--
>>>  block/ssh.c|4 +-
>>>  block/vdi.c|   14 +++---
>>>  block/vmdk.c   |   21 -
>>>  block/vpc.c|4 +-
>>>  block/vvfat.c  |  108 ++---
>>>  blockdev.c |6 +--
>>>  16 files changed, 160 insertions(+), 163 deletions(-)
>>>
>>> diff --git a/block-migration.c b/block-migration.c
>>> index 56951e0..5bcf7c8 100644
>>> --- a/block-migration.c
>>> +++ b/block-migration.c
>>> @@ -790,7 +790,7 @@ static int block_load(QEMUFile *f, void *opaque, int 
>>> version_id)
>>>
>>>  bs = bdrv_find(device_name);
>>>  if (!bs) {
>>> -fprintf(stderr, "Error unknown block device %s\n",
>>> +error_report("Error unknown block device %s",
>>>  device_name);
>>>  return -EINVAL;
>>>  }
>>> @@ -833,7 +833,7 @@ static int block_load(QEMUFile *f, void *opaque, int 
>>> version_id)
>>> (addr == 100) ? '\n' : '\r');
>>>  fflush(stdout);
>>>  } else if (!(flags & BLK_MIG_FLAG_EOS)) {
>>> -fprintf(stderr, "Unknown block migration flags: %#x\n", flags);
>>> +error_report("Unknown block migration flags: %#x", flags);
>>>  return -EINVAL;
>>>  }
>>>  ret = qemu_file_get_error(f);
>>> diff --git a/block.c b/block.c
>>> index b749d31..7dc4acb 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -2694,8 +2694,8 @@ static int bdrv_prwv_co(BlockDriverState *bs, int64_t 
>>> offset,
>>>   * if it has been enabled.
>>>   */
>>>  if (bs->io_limits_enabled) {
>>> -fprintf(stderr, "Disabling I/O throttling on '%s' due "
>>> -"to synchronous I/O.\n", bdrv_get_device_name(bs));
>>> +error_report("Disabling I/O throttling on '%s' due "
>>> + "to synchronous I/O.", bdrv_get_device_name(bs));
>>>  bdrv_io_limits_disable(bs);
>>>  }
>>>
>>> diff --git a/block/linux-aio.c b/block/linux-aio.c
>>> index 53434e2..b706a59 100644
>>> --- a/block/linux-aio.c
>>> +++ b/block/linux-aio.c
>>> @@ -162,8 +162,8 @@ BlockDriverAIOCB *laio_submit(BlockDriverState *bs, 
>>> void *aio_ctx, int fd,
>>> break;
>>>  /* Currently Linux kernel does not support other operations */
>>>  default:
>>> -fprintf(stderr, "%s: invalid AIO request type 0x%x.\n",
>>> -__func__, type);
>>> +error_report("%s: invalid AIO request type 0x%x.",
>>> + __func__, type);
>>>  goto out_free_aiocb;
>>>  }
>>>  io_set_eventfd(&laiocb->iocb, event_notifier_get_fd(&s->e));
>>> diff --git a/block/nbd-client.h b/block/nbd-client.h
>>> index f2a6337..74178f4 100644
>>> --- a/block/nbd-client.h
>>> +++ b/block/nbd-client.h
>>> @@ -9,7 +9,7 @@
>>>
>>>  #if defined(DEBUG_NBD)
>>>  #define logout(fmt, ...) \
>>> -fprintf(stderr, "nbd\t%-24s" fmt, __func__, ##__VA_ARGS__)
>>> +error_report("nbd\t%-24s" fmt, __func__, ##__VA_ARGS__)
>>
>> So i'm not sure we want to convert debug printfery to error_report.
>> There's very good value in converting the printfs with user
>> visibility, but ones like this seem intended for developers only as
>> throwaway-output. My thinking is that this is a lower level output
>> than error_report. For instance, as a developer you may do something
>> to break your error API yet you still want your debug printfery.
>> Wouldn't matter in this location, but there may be other parts of the
>> tree where we don't want error_report relinace for debug
>> instrumentation and it just seems better to keep it consistent.
>>
>> Thinking further afield, qemu_log may ultimately be the correct
>> mechanism for this instead (I think thats what I have been using for
>> new code recently anyway).
>>
>> Thoughts from others?
>>
>> Regards,
>> Peter
> Hi! I am a novice and this is my warm-up task for GSoC. So you mean
> that it's good to convert printfs to error_report where the message is
> deemed to notice the user, such as some warning about wrong cmd
> ar

[Qemu-devel] [PATCH v19 00/16] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD

[Fam Zheng]

v19: Rebase to master, resolve several contextual conflicts.
 Patch 05 has the only code change difference from v18 to preserve.
assert(bs->backing_hd == NULL);

v18: Address reviewing comments from Jeff and Eric. Rebased to current master.
 Side by side diff from v17: http://bit.ly/1oO2Fvt

[01/15] block: Add BlockOpType enum
Add Jeff's reviewed-by.

[02/15] block: Introduce op_blockers to BlockDriverState
Add Jeff's reviewed-by.

[03/15] block: Replace in_use with operation blocker
Add Jeff's reviewed-by.

[04/15] block: Move op_blocker check from block_job_create to its caller
Add Jeff's reviewed-by.

[05/15] block: Add bdrv_set_backing_hd()
Don't unset bs->backing_file and bs->backing_format when
backing_hd==NULL, because qcow2_close() will save these into image
header.

[08/15] block: Support dropping active in bdrv_drop_intermediate
Swap parameters for bdrv_swap:
bdrv_swap(active, base); -> bdrv_swap(base, active);
Use bdrv_set_backing_hd().

[10/15] commit: Use bdrv_drop_intermediate
New. (Jeff)

[11/15] qmp: Add command 'blockdev-backup'
Since 2.0 -> Since 2.1. (Eric)

[13/15] block: Add blockdev-backup to transaction
Comment "Since 2.1" for blockdev-backup. (Eric)

[15/15] qemu-iotests: Image fleecing test case 089
Case number 083 -> 089.


Fam Zheng (16):
  vmdk: Optimize cluster allocation
  block: Add BlockOpType enum
  block: Introduce op_blockers to BlockDriverState
  block: Replace in_use with operation blocker
  block: Move op_blocker check from block_job_create to its caller
  block: Add bdrv_set_backing_hd()
  block: Add backing_blocker in BlockDriverState
  block: Parse "backing" option to reference existing BDS
  block: Support dropping active in bdrv_drop_intermediate
  stream: Use bdrv_drop_intermediate and drop close_unused_images
  commit: Use bdrv_drop_intermediate
  qmp: Add command 'blockdev-backup'
  block: Allow backup on referenced named BlockDriverState
  block: Add blockdev-backup to transaction
  qemu-iotests: Test blockdev-backup in 055
  qemu-iotests: Image fleecing test case 089

 block-migration.c   |   7 +-
 block.c | 311 +++-
 block/backup.c  |  26 
 block/commit.c  |   2 +-
 block/mirror.c  |   9 +-
 block/stream.c  |  42 +-
 block/vmdk.c| 184 
 blockdev.c  | 122 ++--
 blockjob.c  |  14 +-
 hw/block/dataplane/virtio-blk.c |  18 ++-
 include/block/block.h   |  29 +++-
 include/block/block_int.h   |   9 +-
 include/block/blockjob.h|   3 +
 qapi-schema.json|  52 +++
 qmp-commands.hx |  44 ++
 tests/qemu-iotests/055  | 275 +--
 tests/qemu-iotests/055.out  |   4 +-
 tests/qemu-iotests/089  |  99 +
 tests/qemu-iotests/089.out  |   5 +
 tests/qemu-iotests/group|   1 +
 20 files changed, 977 insertions(+), 279 deletions(-)
 create mode 100755 tests/qemu-iotests/089
 create mode 100644 tests/qemu-iotests/089.out

-- 
1.9.2

[Qemu-devel] [PATCH v19 01/16] vmdk: Optimize cluster allocation

[Fam Zheng]

This drops the unnecessary bdrv_truncate() from, and also improves,
cluster allocation code path.

Before, when we need a new cluster, get_cluster_offset truncates the
image to bdrv_getlength() + cluster_size, and returns the offset of
added area, i.e. the image length before truncating.

This is not efficient, so it's now rewritten as:

  - Save the extent file length when opening.

  - When allocating cluster, use the saved length as cluster offset.

  - Don't truncate image, because we'll anyway write data there: just
write any data at the EOF position, in descending priority:

* New user data (cluster allocation happens in a write request).

* Filling data in the beginning and/or ending of the new cluster, if
  not covered by user data: either backing file content (COW), or
  zero for standalone images.

One major benifit of this change is, on host mounted NFS images, even
over a fast network, ftruncate is slow (see the example below). This
change significantly speeds up cluster allocation. Comparing by
converting a cirros image (296M) to VMDK on an NFS mount point, over
1Gbe LAN:

$ time qemu-img convert cirros-0.3.1.img /mnt/a.raw -O vmdk

Before:
real0m21.796s
user0m0.130s
sys 0m0.483s

After:
real0m2.017s
user0m0.047s
sys 0m0.190s

We also get rid of unchecked bdrv_getlength() and bdrv_truncate(), and
get a little more documentation in function comments.

Tested that this passes qemu-iotests for all VMDK subformats.

Signed-off-by: Fam Zheng 
---
 block/vmdk.c | 184 +++
 1 file changed, 121 insertions(+), 63 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 06a1f9f..8c34d5e 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -106,6 +106,7 @@ typedef struct VmdkExtent {
 uint32_t l2_cache_counts[L2_CACHE_SIZE];
 
 int64_t cluster_sectors;
+int64_t next_cluster_offset;
 char *type;
 } VmdkExtent;
 
@@ -397,6 +398,7 @@ static int vmdk_add_extent(BlockDriverState *bs,
 {
 VmdkExtent *extent;
 BDRVVmdkState *s = bs->opaque;
+int64_t ret;
 
 if (cluster_sectors > 0x20) {
 /* 0x20 * 512Bytes = 1GB for one cluster is unrealistic */
@@ -428,6 +430,13 @@ static int vmdk_add_extent(BlockDriverState *bs,
 extent->l2_size = l2_size;
 extent->cluster_sectors = flat ? sectors : cluster_sectors;
 
+ret = bdrv_getlength(extent->file);
+
+if (ret < 0) {
+return ret;
+}
+extent->next_cluster_offset = ROUND_UP(ret, BDRV_SECTOR_SIZE);
+
 if (s->num_extents > 1) {
 extent->end_sector = (*(extent - 1)).end_sector + extent->sectors;
 } else {
@@ -954,42 +963,77 @@ static int vmdk_refresh_limits(BlockDriverState *bs)
 return 0;
 }
 
+/**
+ * get_whole_cluster
+ *
+ * Copy backing file's cluster that covers @sector_num, otherwise write zero,
+ * to the cluster at @cluster_sector_num.
+ *
+ * If @skip_start < @skip_end, the relative range [@skip_start, @skip_end) is
+ * not copied or written, and leave it for call to write user data in the
+ * request.
+ */
 static int get_whole_cluster(BlockDriverState *bs,
-VmdkExtent *extent,
-uint64_t cluster_offset,
-uint64_t offset,
-bool allocate)
+ VmdkExtent *extent,
+ uint64_t cluster_sector_num,
+ uint64_t sector_num,
+ uint64_t skip_start, uint64_t skip_end)
 {
 int ret = VMDK_OK;
-uint8_t *whole_grain = NULL;
+int64_t cluster_bytes;
+uint8_t *whole_grain;
+
+/* For COW, align request sector_num to cluster start */
+sector_num -= sector_num % extent->cluster_sectors;
+cluster_bytes = extent->cluster_sectors << BDRV_SECTOR_BITS;
+whole_grain = qemu_blockalign(bs, cluster_bytes);
+memset(whole_grain, 0, cluster_bytes);
 
+assert(skip_end <= sector_num + extent->cluster_sectors);
 /* we will be here if it's first write on non-exist grain(cluster).
  * try to read from parent image, if exist */
-if (bs->backing_hd) {
-whole_grain =
-qemu_blockalign(bs, extent->cluster_sectors << BDRV_SECTOR_BITS);
-if (!vmdk_is_cid_valid(bs)) {
-ret = VMDK_ERROR;
-goto exit;
-}
+if (bs->backing_hd && !vmdk_is_cid_valid(bs)) {
+ret = VMDK_ERROR;
+goto exit;
+}
 
-/* floor offset to cluster */
-offset -= offset % (extent->cluster_sectors * 512);
-ret = bdrv_read(bs->backing_hd, offset >> 9, whole_grain,
-extent->cluster_sectors);
+/* Read backing data before skip range */
+if (skip_start > 0) {
+if (bs->backing_hd) {
+ret = bdrv_read(bs->backing_hd, sector_num,
+whole_grain, skip_start);
+if (ret < 0) {
+ret = VM

[Qemu-devel] [PATCH v19 04/16] block: Replace in_use with operation blocker

[Fam Zheng]

This drops BlockDriverState.in_use with op_blockers:

  - Call bdrv_op_block_all in place of bdrv_set_in_use(bs, 1).
  - Call bdrv_op_unblock_all in place of bdrv_set_in_use(bs, 0).
  - Check bdrv_op_is_blocked() in place of bdrv_in_use(bs).
The specific types are used, e.g. in place of starting block backup,
bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP, ...).
  - Check bdrv_op_blocker_is_empty() in place of assert(!bs->in_use).

Note: there is only bdrv_op_block_all and bdrv_op_unblock_all callers at
this moment. So although the checks are specific to op types, this
changes can still be seen as identical logic with previously with
in_use. The difference is error message are improved because of blocker
error info.

Signed-off-by: Fam Zheng 
Reviewed-by: Jeff Cody 
---
 block-migration.c   |  7 +--
 block.c | 24 +++-
 blockdev.c  | 19 +--
 blockjob.c  | 14 +-
 hw/block/dataplane/virtio-blk.c | 18 --
 include/block/block.h   |  2 --
 include/block/block_int.h   |  1 -
 include/block/blockjob.h|  3 +++
 8 files changed, 45 insertions(+), 43 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index 56951e0..1656270 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -59,6 +59,7 @@ typedef struct BlkMigDevState {
 unsigned long *aio_bitmap;
 int64_t completed_sectors;
 BdrvDirtyBitmap *dirty_bitmap;
+Error *blocker;
 } BlkMigDevState;
 
 typedef struct BlkMigBlock {
@@ -361,7 +362,8 @@ static void init_blk_migration_it(void *opaque, 
BlockDriverState *bs)
 bmds->completed_sectors = 0;
 bmds->shared_base = block_mig_state.shared_base;
 alloc_aio_bitmap(bmds);
-bdrv_set_in_use(bs, 1);
+error_setg(&bmds->blocker, "block device is in use by migration");
+bdrv_op_block_all(bs, bmds->blocker);
 bdrv_ref(bs);
 
 block_mig_state.total_sector_sum += sectors;
@@ -599,7 +601,8 @@ static void blk_mig_cleanup(void)
 blk_mig_lock();
 while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) {
 QSIMPLEQ_REMOVE_HEAD(&block_mig_state.bmds_list, entry);
-bdrv_set_in_use(bmds->bs, 0);
+bdrv_op_unblock_all(bmds->bs, bmds->blocker);
+error_free(bmds->blocker);
 bdrv_unref(bmds->bs);
 g_free(bmds->aio_bitmap);
 g_free(bmds);
diff --git a/block.c b/block.c
index 32338ca..0747a43 100644
--- a/block.c
+++ b/block.c
@@ -1904,7 +1904,6 @@ static void bdrv_move_feature_fields(BlockDriverState 
*bs_dest,
 bs_dest->refcnt = bs_src->refcnt;
 
 /* job */
-bs_dest->in_use = bs_src->in_use;
 bs_dest->job= bs_src->job;
 
 /* keep the same entry in bdrv_states */
@@ -1947,7 +1946,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState 
*bs_old)
 assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
 assert(bs_new->job == NULL);
 assert(bs_new->dev == NULL);
-assert(bs_new->in_use == 0);
+assert(bdrv_op_blocker_is_empty(bs_new));
 assert(bs_new->io_limits_enabled == false);
 assert(!throttle_have_timer(&bs_new->throttle_state));
 
@@ -1966,7 +1965,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState 
*bs_old)
 /* Check a few fields that should remain attached to the device */
 assert(bs_new->dev == NULL);
 assert(bs_new->job == NULL);
-assert(bs_new->in_use == 0);
+assert(bdrv_op_blocker_is_empty(bs_new));
 assert(bs_new->io_limits_enabled == false);
 assert(!throttle_have_timer(&bs_new->throttle_state));
 
@@ -2011,7 +2010,7 @@ static void bdrv_delete(BlockDriverState *bs)
 {
 assert(!bs->dev);
 assert(!bs->job);
-assert(!bs->in_use);
+assert(bdrv_op_blocker_is_empty(bs));
 assert(!bs->refcnt);
 assert(QLIST_EMPTY(&bs->dirty_bitmaps));
 
@@ -2193,7 +2192,8 @@ int bdrv_commit(BlockDriverState *bs)
 return -ENOTSUP;
 }
 
-if (bdrv_in_use(bs) || bdrv_in_use(bs->backing_hd)) {
+if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, NULL) ||
+bdrv_op_is_blocked(bs->backing_hd, BLOCK_OP_TYPE_COMMIT, NULL)) {
 return -EBUSY;
 }
 
@@ -3446,8 +3446,9 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset)
 return -ENOTSUP;
 if (bs->read_only)
 return -EACCES;
-if (bdrv_in_use(bs))
+if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, NULL)) {
 return -EBUSY;
+}
 ret = drv->bdrv_truncate(bs, offset);
 if (ret == 0) {
 ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
@@ -5344,17 +5345,6 @@ bool bdrv_op_blocker_is_empty(BlockDriverState *bs)
 return true;
 }
 
-void bdrv_set_in_use(BlockDriverState *bs, int in_use)
-{
-assert(bs->in_use != in_use);
-bs->in_use = in_use;
-}
-
-int bdrv_in_use(BlockDriverState *bs)
-{
-return bs->in_use;
-}
-
 void bdrv_iostatus_

[Qemu-devel] [PATCH v19 02/16] block: Add BlockOpType enum

[Fam Zheng]

This adds the enum of all the operations that can be taken on a block
device.

Signed-off-by: Fam Zheng 
Reviewed-by: Benoit Canet 
Reviewed-by: Jeff Cody 
---
 include/block/block.h | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/include/block/block.h b/include/block/block.h
index 467fb2b..ac3a69b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -155,6 +155,25 @@ typedef struct BDRVReopenState {
 void *opaque;
 } BDRVReopenState;
 
+/*
+ * Block operation types
+ */
+typedef enum BlockOpType {
+BLOCK_OP_TYPE_BACKUP_SOURCE,
+BLOCK_OP_TYPE_BACKUP_TARGET,
+BLOCK_OP_TYPE_CHANGE,
+BLOCK_OP_TYPE_COMMIT,
+BLOCK_OP_TYPE_DATAPLANE,
+BLOCK_OP_TYPE_DRIVE_DEL,
+BLOCK_OP_TYPE_EJECT,
+BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
+BLOCK_OP_TYPE_INTERNAL_SNAPSHOT,
+BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
+BLOCK_OP_TYPE_MIRROR,
+BLOCK_OP_TYPE_RESIZE,
+BLOCK_OP_TYPE_STREAM,
+BLOCK_OP_TYPE_MAX,
+} BlockOpType;
 
 void bdrv_iostatus_enable(BlockDriverState *bs);
 void bdrv_iostatus_reset(BlockDriverState *bs);
-- 
1.9.2

[Qemu-devel] [PATCH v19 03/16] block: Introduce op_blockers to BlockDriverState

[Fam Zheng]

BlockDriverState.op_blockers is an array of lists with BLOCK_OP_TYPE_MAX
elements. Each list is a list of blockers of an operation type
(BlockOpType), that marks this BDS as currently blocked for a certain
type of operation with reason errors stored in the list. The rule of
usage is:

 * BDS user who wants to take an operation should check if there's any
   blocker of the type with bdrv_op_is_blocked().

 * BDS user who wants to block certain types of operation, should call
   bdrv_op_block (or bdrv_op_block_all to block all types of operations,
   which is similar to the existing bdrv_set_in_use()).

 * A blocker is only referenced by op_blockers, so the lifecycle is
   managed by caller, and shouldn't be lost until unblock, so typically
   a caller does these:

   - Allocate a blocker with error_setg or similar, call bdrv_op_block()
 to block some operations.
   - Hold the blocker, do his job.
   - Unblock operations that it blocked, with the same reason pointer
 passed to bdrv_op_unblock().
   - Release the blocker with error_free().

Signed-off-by: Fam Zheng 
Reviewed-by: Benoit Canet 
Reviewed-by: Jeff Cody 
---
 block.c   | 75 +++
 include/block/block.h |  7 +
 include/block/block_int.h |  5 
 3 files changed, 87 insertions(+)

diff --git a/block.c b/block.c
index b749d31..32338ca 100644
--- a/block.c
+++ b/block.c
@@ -335,6 +335,7 @@ void bdrv_register(BlockDriver *bdrv)
 BlockDriverState *bdrv_new(const char *device_name, Error **errp)
 {
 BlockDriverState *bs;
+int i;
 
 if (bdrv_find(device_name)) {
 error_setg(errp, "Device with id '%s' already exists",
@@ -353,6 +354,9 @@ BlockDriverState *bdrv_new(const char *device_name, Error 
**errp)
 if (device_name[0] != '\0') {
 QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
 }
+for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
+QLIST_INIT(&bs->op_blockers[i]);
+}
 bdrv_iostatus_disable(bs);
 notifier_list_init(&bs->close_notifiers);
 notifier_with_return_list_init(&bs->before_write_notifiers);
@@ -1907,6 +1911,8 @@ static void bdrv_move_feature_fields(BlockDriverState 
*bs_dest,
 pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name),
 bs_src->device_name);
 bs_dest->device_list = bs_src->device_list;
+memcpy(bs_dest->op_blockers, bs_src->op_blockers,
+   sizeof(bs_dest->op_blockers));
 }
 
 /*
@@ -5269,6 +5275,75 @@ void bdrv_unref(BlockDriverState *bs)
 }
 }
 
+struct BdrvOpBlocker {
+Error *reason;
+QLIST_ENTRY(BdrvOpBlocker) list;
+};
+
+bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
+{
+BdrvOpBlocker *blocker;
+assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
+if (!QLIST_EMPTY(&bs->op_blockers[op])) {
+blocker = QLIST_FIRST(&bs->op_blockers[op]);
+if (errp) {
+*errp = error_copy(blocker->reason);
+}
+return true;
+}
+return false;
+}
+
+void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason)
+{
+BdrvOpBlocker *blocker;
+assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
+
+blocker = g_malloc0(sizeof(BdrvOpBlocker));
+blocker->reason = reason;
+QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list);
+}
+
+void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason)
+{
+BdrvOpBlocker *blocker, *next;
+assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
+QLIST_FOREACH_SAFE(blocker, &bs->op_blockers[op], list, next) {
+if (blocker->reason == reason) {
+QLIST_REMOVE(blocker, list);
+g_free(blocker);
+}
+}
+}
+
+void bdrv_op_block_all(BlockDriverState *bs, Error *reason)
+{
+int i;
+for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
+bdrv_op_block(bs, i, reason);
+}
+}
+
+void bdrv_op_unblock_all(BlockDriverState *bs, Error *reason)
+{
+int i;
+for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
+bdrv_op_unblock(bs, i, reason);
+}
+}
+
+bool bdrv_op_blocker_is_empty(BlockDriverState *bs)
+{
+int i;
+
+for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
+if (!QLIST_EMPTY(&bs->op_blockers[i])) {
+return false;
+}
+}
+return true;
+}
+
 void bdrv_set_in_use(BlockDriverState *bs, int in_use)
 {
 assert(bs->in_use != in_use);
diff --git a/include/block/block.h b/include/block/block.h
index ac3a69b..5db028d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -468,6 +468,13 @@ void bdrv_unref(BlockDriverState *bs);
 void bdrv_set_in_use(BlockDriverState *bs, int in_use);
 int bdrv_in_use(BlockDriverState *bs);
 
+bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp);
+void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason);
+void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason);
+void bdrv_op_block_all(BlockDriverState *bs, Error *reason);
+void bdrv_op_unblo

[Qemu-devel] [PATCH v19 05/16] block: Move op_blocker check from block_job_create to its caller

[Fam Zheng]

It makes no sense to check for "any" blocker on bs, we are here only
because of the mechanical conversion from in_use to op_blockers. Remove
it now, and let the callers check specific operation types. Backup and
mirror already have it, add checker to stream and commit.

Signed-off-by: Fam Zheng 
Reviewed-by: Benoit Canet 
Reviewed-by: Jeff Cody 
---
 blockdev.c | 8 
 blockjob.c | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 5d950fa..21fc55b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1850,6 +1850,10 @@ void qmp_block_stream(const char *device, bool has_base,
 return;
 }
 
+if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
+return;
+}
+
 if (base) {
 base_bs = bdrv_find_backing_image(bs, base);
 if (base_bs == NULL) {
@@ -1894,6 +1898,10 @@ void qmp_block_commit(const char *device,
 return;
 }
 
+if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
+return;
+}
+
 /* default top_bs is the active layer */
 top_bs = bs;
 
diff --git a/blockjob.c b/blockjob.c
index 60e72f5..7d84ca1 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -41,7 +41,7 @@ void *block_job_create(const BlockJobDriver *driver, 
BlockDriverState *bs,
 {
 BlockJob *job;
 
-if (bs->job || !bdrv_op_blocker_is_empty(bs)) {
+if (bs->job) {
 error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
 return NULL;
 }
-- 
1.9.2

[Qemu-devel] [PATCH v19 08/16] block: Parse "backing" option to reference existing BDS

[Fam Zheng]

Now it's safe to allow reference for backing_hd in the interface.

Signed-off-by: Fam Zheng 
---
 block.c | 27 +--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 8155e68..bb7031f 100644
--- a/block.c
+++ b/block.c
@@ -1443,12 +1443,35 @@ int bdrv_open(BlockDriverState **pbs, const char 
*filename,
 /* If there is a backing file, use it */
 if ((flags & BDRV_O_NO_BACKING) == 0) {
 QDict *backing_options;
+const char *backing_name;
+BlockDriverState *backing_hd;
 
+backing_name = qdict_get_try_str(options, "backing");
 qdict_extract_subqdict(options, &backing_options, "backing.");
-ret = bdrv_open_backing_file(bs, backing_options, &local_err);
-if (ret < 0) {
+
+if (backing_name && qdict_size(backing_options)) {
+error_setg(&local_err,
+   "Option \"backing\" and \"backing.*\" cannot be "
+   "used together");
+ret = -EINVAL;
 goto close_and_fail;
 }
+if (backing_name) {
+backing_hd = bdrv_find(backing_name);
+if (!backing_hd) {
+error_set(&local_err, QERR_DEVICE_NOT_FOUND, backing_name);
+ret = -ENOENT;
+goto close_and_fail;
+}
+qdict_del(options, "backing");
+bdrv_set_backing_hd(bs, backing_hd);
+bdrv_ref(backing_hd);
+} else {
+ret = bdrv_open_backing_file(bs, backing_options, &local_err);
+if (ret < 0) {
+goto close_and_fail;
+}
+}
 }
 
 /* For snapshot=on, create a temporary qcow2 overlay. bs points to the
-- 
1.9.2

[Qemu-devel] [PATCH v19 09/16] block: Support dropping active in bdrv_drop_intermediate

[Fam Zheng]

Dropping intermediate could be useful both for commit and stream, and
BDS refcnt plus bdrv_swap could do most of the job nicely. It also needs
to work with op blockers.

Signed-off-by: Fam Zheng 
---
 block.c| 139 -
 block/commit.c |   2 +-
 2 files changed, 70 insertions(+), 71 deletions(-)

diff --git a/block.c b/block.c
index bb7031f..ab64027 100644
--- a/block.c
+++ b/block.c
@@ -2556,115 +2556,114 @@ BlockDriverState *bdrv_find_overlay(BlockDriverState 
*active,
 return overlay;
 }
 
-typedef struct BlkIntermediateStates {
-BlockDriverState *bs;
-QSIMPLEQ_ENTRY(BlkIntermediateStates) entry;
-} BlkIntermediateStates;
-
-
 /*
- * Drops images above 'base' up to and including 'top', and sets the image
- * above 'top' to have base as its backing file.
+ * Drops images above 'base' up to and including 'top', and sets new 'base' as
+ * backing_hd of top's overlay (the image orignally has 'top' as backing file).
+ * top's overlay may be NULL if 'top' is active, no such update needed.
+ * Requires that the top's overlay to 'top' is opened r/w.
+ *
+ * 1) This will convert the following chain:
+ *
+ * ... <- base <- ... <- top <- overlay <-... <- active
  *
- * Requires that the overlay to 'top' is opened r/w, so that the backing file
- * information in 'bs' can be properly updated.
+ * to
+ *
+ * ... <- base <- overlay <- active
+ *
+ * 2) It is allowed for bottom==base, in which case it converts:
  *
- * E.g., this will convert the following chain:
- * bottom <- base <- intermediate <- top <- active
+ * base <- ... <- top <- overlay <- ... <- active
  *
  * to
  *
- * bottom <- base <- active
+ * base <- overlay <- active
  *
- * It is allowed for bottom==base, in which case it converts:
+ * 2) It also allows active==top, in which case it converts:
  *
- * base <- intermediate <- top <- active
+ * ... <- base <- ... <- top (active)
  *
  * to
  *
- * base <- active
+ * ... <- base == active == top
+ *
+ * i.e. only base and lower remains: *top == *base when return.
+ *
+ * 3) If base==NULL, it will drop all the BDS below overlay and set its
+ * backing_hd to NULL. I.e.:
  *
- * Error conditions:
- *  if active == top, that is considered an error
+ * base(NULL) <- ... <- overlay <- ... <- active
+ *
+ * to
+ *
+ * overlay <- ... <- active
  *
  */
 int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
BlockDriverState *base)
 {
-BlockDriverState *intermediate;
-BlockDriverState *base_bs = NULL;
-BlockDriverState *new_top_bs = NULL;
-BlkIntermediateStates *intermediate_state, *next;
-int ret = -EIO;
-
-QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete;
-QSIMPLEQ_INIT(&states_to_delete);
+BlockDriverState *drop_start, *overlay, *bs;
+int ret = -EINVAL;
 
-if (!top->drv || !base->drv) {
+assert(active);
+assert(top);
+/* Verify that top is in backing chain of active */
+bs = active;
+while (bs && bs != top) {
+bs = bs->backing_hd;
+}
+if (!bs) {
 goto exit;
 }
+/* Verify that base is in backing chain of top */
+if (base) {
+while (bs && bs != base) {
+bs = bs->backing_hd;
+}
+if (bs != base) {
+goto exit;
+}
+}
 
-new_top_bs = bdrv_find_overlay(active, top);
-
-if (new_top_bs == NULL) {
-/* we could not find the image above 'top', this is an error */
+if (!top->drv || (base && !base->drv)) {
 goto exit;
 }
-
-/* special case of new_top_bs->backing_hd already pointing to base - 
nothing
- * to do, no intermediate images */
-if (new_top_bs->backing_hd == base) {
+if (top == base) {
+ret = 0;
+goto exit;
+} else if (top == active) {
+assert(base);
+drop_start = active->backing_hd;
+bdrv_swap(base, active);
+bdrv_set_backing_hd(base, NULL);
+bdrv_unref(drop_start);
 ret = 0;
 goto exit;
 }
 
-intermediate = top;
-
-/* now we will go down through the list, and add each BDS we find
- * into our deletion queue, until we hit the 'base'
- */
-while (intermediate) {
-intermediate_state = g_malloc0(sizeof(BlkIntermediateStates));
-intermediate_state->bs = intermediate;
-QSIMPLEQ_INSERT_TAIL(&states_to_delete, intermediate_state, entry);
-
-if (intermediate->backing_hd == base) {
-base_bs = intermediate->backing_hd;
-break;
-}
-intermediate = intermediate->backing_hd;
-}
-if (base_bs == NULL) {
-/* something went wrong, we did not end at the base. safely
- * unravel everything, and exit with error */
+overlay = bdrv_find_overlay(active, top);
+if (!overlay) {
 goto exit;
 }
-
-/* success - we can delete the intermediate states, and

[Qemu-devel] [PATCH v19 06/16] block: Add bdrv_set_backing_hd()

[Fam Zheng]

This is the common but non-trivial steps to assign or change the
backing_hd of BDS.

Signed-off-by: Fam Zheng 
---
 block.c   | 36 +++-
 include/block/block.h |  1 +
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/block.c b/block.c
index 0747a43..ec26a2b 100644
--- a/block.c
+++ b/block.c
@@ -1094,6 +1094,21 @@ fail:
 return ret;
 }
 
+void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
+{
+
+bs->backing_hd = backing_hd;
+if (!backing_hd) {
+goto out;
+}
+bs->open_flags &= ~BDRV_O_NO_BACKING;
+pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename);
+pstrcpy(bs->backing_format, sizeof(bs->backing_format),
+backing_hd->drv ? backing_hd->drv->format_name : "");
+out:
+bdrv_refresh_limits(bs);
+}
+
 /*
  * Opens the backing file for a BlockDriverState if not yet open
  *
@@ -1107,6 +1122,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
*options, Error **errp)
 char *backing_filename = g_malloc0(PATH_MAX);
 int ret = 0;
 BlockDriver *back_drv = NULL;
+BlockDriverState *backing_hd;
 Error *local_err = NULL;
 
 if (bs->backing_hd != NULL) {
@@ -1129,27 +1145,26 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
*options, Error **errp)
 bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX);
 }
 
+backing_hd = bdrv_new("", errp);
+
 if (bs->backing_format[0] != '\0') {
 back_drv = bdrv_find_format(bs->backing_format);
 }
 
 assert(bs->backing_hd == NULL);
-ret = bdrv_open(&bs->backing_hd,
+ret = bdrv_open(&backing_hd,
 *backing_filename ? backing_filename : NULL, NULL, options,
 bdrv_backing_flags(bs->open_flags), back_drv, &local_err);
 if (ret < 0) {
-bs->backing_hd = NULL;
+bdrv_unref(backing_hd);
+backing_hd = NULL;
 bs->open_flags |= BDRV_O_NO_BACKING;
 error_setg(errp, "Could not open backing file: %s",
error_get_pretty(local_err));
 error_free(local_err);
 goto free_exit;
 }
-
-if (bs->backing_hd->file) {
-pstrcpy(bs->backing_file, sizeof(bs->backing_file),
-bs->backing_hd->file->filename);
-}
+bdrv_set_backing_hd(bs, backing_hd);
 
 /* Recalculate the BlockLimits with the backing file */
 bdrv_refresh_limits(bs);
@@ -1998,12 +2013,7 @@ void bdrv_append(BlockDriverState *bs_new, 
BlockDriverState *bs_top)
 
 /* The contents of 'tmp' will become bs_top, as we are
  * swapping bs_new and bs_top contents. */
-bs_top->backing_hd = bs_new;
-bs_top->open_flags &= ~BDRV_O_NO_BACKING;
-pstrcpy(bs_top->backing_file, sizeof(bs_top->backing_file),
-bs_new->filename);
-pstrcpy(bs_top->backing_format, sizeof(bs_top->backing_format),
-bs_new->drv ? bs_new->drv->format_name : "");
+bdrv_set_backing_hd(bs_top, bs_new);
 }
 
 static void bdrv_delete(BlockDriverState *bs)
diff --git a/include/block/block.h b/include/block/block.h
index 9ff42eb..7228881 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -209,6 +209,7 @@ int bdrv_parse_discard_flags(const char *mode, int *flags);
 int bdrv_open_image(BlockDriverState **pbs, const char *filename,
 QDict *options, const char *bdref_key, int flags,
 bool allow_none, Error **errp);
+void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
 void bdrv_append_temp_snapshot(BlockDriverState *bs, Error **errp);
 int bdrv_open(BlockDriverState **pbs, const char *filename,
-- 
1.9.2

[Qemu-devel] [PATCH v19 10/16] stream: Use bdrv_drop_intermediate and drop close_unused_images

[Fam Zheng]

This reuses the new bdrv_drop_intermediate.

Signed-off-by: Fam Zheng 
---
 block/stream.c | 42 +-
 1 file changed, 1 insertion(+), 41 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index dd0b4ac..1b348a2 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -32,7 +32,6 @@ typedef struct StreamBlockJob {
 RateLimit limit;
 BlockDriverState *base;
 BlockdevOnError on_error;
-char backing_file_id[1024];
 } StreamBlockJob;
 
 static int coroutine_fn stream_populate(BlockDriverState *bs,
@@ -51,34 +50,6 @@ static int coroutine_fn stream_populate(BlockDriverState *bs,
 return bdrv_co_copy_on_readv(bs, sector_num, nb_sectors, &qiov);
 }
 
-static void close_unused_images(BlockDriverState *top, BlockDriverState *base,
-const char *base_id)
-{
-BlockDriverState *intermediate;
-intermediate = top->backing_hd;
-
-/* Must assign before bdrv_delete() to prevent traversing dangling pointer
- * while we delete backing image instances.
- */
-top->backing_hd = base;
-
-while (intermediate) {
-BlockDriverState *unused;
-
-/* reached base */
-if (intermediate == base) {
-break;
-}
-
-unused = intermediate;
-intermediate = intermediate->backing_hd;
-unused->backing_hd = NULL;
-bdrv_unref(unused);
-}
-
-bdrv_refresh_limits(top);
-}
-
 static void coroutine_fn stream_run(void *opaque)
 {
 StreamBlockJob *s = opaque;
@@ -184,15 +155,7 @@ wait:
 ret = error;
 
 if (!block_job_is_cancelled(&s->common) && sector_num == end && ret == 0) {
-const char *base_id = NULL, *base_fmt = NULL;
-if (base) {
-base_id = s->backing_file_id;
-if (base->drv) {
-base_fmt = base->drv->format_name;
-}
-}
-ret = bdrv_change_backing_file(bs, base_id, base_fmt);
-close_unused_images(bs, base, base_id);
+ret = bdrv_drop_intermediate(bs, bs->backing_hd, base);
 }
 
 qemu_vfree(buf);
@@ -237,9 +200,6 @@ void stream_start(BlockDriverState *bs, BlockDriverState 
*base,
 }
 
 s->base = base;
-if (base_id) {
-pstrcpy(s->backing_file_id, sizeof(s->backing_file_id), base_id);
-}
 
 s->on_error = on_error;
 s->common.co = qemu_coroutine_create(stream_run);
-- 
1.9.2

[Qemu-devel] [PATCH v19 11/16] commit: Use bdrv_drop_intermediate

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 block/mirror.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 6a53d79..b4b12d0 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -493,14 +493,10 @@ immediate_exit:
 if (bdrv_get_flags(s->target) != bdrv_get_flags(s->common.bs)) {
 bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL);
 }
-bdrv_swap(s->target, s->common.bs);
 if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) {
-/* drop the bs loop chain formed by the swap: break the loop then
- * trigger the unref from the top one */
-BlockDriverState *p = s->base->backing_hd;
-s->base->backing_hd = NULL;
-bdrv_op_unblock_all(p, s->base->backing_blocker);
-bdrv_unref(p);
+ret = bdrv_drop_intermediate(s->common.bs, s->common.bs, s->base);
+} else {
+bdrv_swap(s->target, s->common.bs);
 }
 }
 bdrv_unref(s->target);
-- 
1.9.2

[Qemu-devel] [PATCH v19 12/16] qmp: Add command 'blockdev-backup'

[Fam Zheng]

Similar to drive-backup, but this command uses a device id as target
instead of creating/opening an image file.

Also add blocker on target bs, since the target is also a named device
now.

Add check and report error for bs == target which became possible but is
an illegal case with introduction of blockdev-backup.

Signed-off-by: Fam Zheng 
---
 block/backup.c   | 26 ++
 blockdev.c   | 47 +++
 qapi-schema.json | 49 +
 qmp-commands.hx  | 44 
 4 files changed, 166 insertions(+)

diff --git a/block/backup.c b/block/backup.c
index 15a2e55..ea46340 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -344,6 +344,7 @@ static void coroutine_fn backup_run(void *opaque)
 hbitmap_free(job->bitmap);
 
 bdrv_iostatus_disable(target);
+bdrv_op_unblock_all(target, job->common.blocker);
 bdrv_unref(target);
 
 block_job_completed(&job->common, ret);
@@ -362,6 +363,11 @@ void backup_start(BlockDriverState *bs, BlockDriverState 
*target,
 assert(target);
 assert(cb);
 
+if (bs == target) {
+error_setg(errp, "Source and target cannot be the same");
+return;
+}
+
 if ((on_source_error == BLOCKDEV_ON_ERROR_STOP ||
  on_source_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
 !bdrv_iostatus_is_enabled(bs)) {
@@ -369,6 +375,24 @@ void backup_start(BlockDriverState *bs, BlockDriverState 
*target,
 return;
 }
 
+if (!bdrv_is_inserted(bs)) {
+error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bs->device_name);
+return;
+}
+
+if (!bdrv_is_inserted(target)) {
+error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, target->device_name);
+return;
+}
+
+if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
+return;
+}
+
+if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_BACKUP_TARGET, errp)) {
+return;
+}
+
 len = bdrv_getlength(bs);
 if (len < 0) {
 error_setg_errno(errp, -len, "unable to get length for '%s'",
@@ -382,6 +406,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState 
*target,
 return;
 }
 
+bdrv_op_block_all(target, job->common.blocker);
+
 job->on_source_error = on_source_error;
 job->on_target_error = on_target_error;
 job->target = target;
diff --git a/blockdev.c b/blockdev.c
index 21fc55b..287a2d8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1977,6 +1977,8 @@ void qmp_drive_backup(const char *device, const char 
*target,
 return;
 }
 
+/* Although backup_run has this check too, we need to use bs->drv below, so
+ * do an early check redundantly. */
 if (!bdrv_is_inserted(bs)) {
 error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
 return;
@@ -1993,6 +1995,7 @@ void qmp_drive_backup(const char *device, const char 
*target,
 }
 }
 
+/* Early check to avoid creating target */
 if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
 return;
 }
@@ -2055,6 +2058,50 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error 
**errp)
 return bdrv_named_nodes_list();
 }
 
+void qmp_blockdev_backup(const char *device, const char *target,
+ enum MirrorSyncMode sync,
+ bool has_speed, int64_t speed,
+ bool has_on_source_error,
+ BlockdevOnError on_source_error,
+ bool has_on_target_error,
+ BlockdevOnError on_target_error,
+ Error **errp)
+{
+BlockDriverState *bs;
+BlockDriverState *target_bs;
+Error *local_err = NULL;
+
+if (!has_speed) {
+speed = 0;
+}
+if (!has_on_source_error) {
+on_source_error = BLOCKDEV_ON_ERROR_REPORT;
+}
+if (!has_on_target_error) {
+on_target_error = BLOCKDEV_ON_ERROR_REPORT;
+}
+
+bs = bdrv_find(device);
+if (!bs) {
+error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+return;
+}
+
+target_bs = bdrv_find(target);
+if (!target_bs) {
+error_set(errp, QERR_DEVICE_NOT_FOUND, target);
+return;
+}
+
+bdrv_ref(target_bs);
+backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
+ block_job_cb, bs, &local_err);
+if (local_err != NULL) {
+bdrv_unref(target_bs);
+error_propagate(errp, local_err);
+}
+}
+
 #define DEFAULT_MIRROR_BUF_SIZE   (10 << 20)
 
 void qmp_drive_mirror(const char *device, const char *target,
diff --git a/qapi-schema.json b/qapi-schema.json
index 36cb964..c6b10b9 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1953,6 +1953,40 @@
 '*on-target-error': 'BlockdevOnError' } }
 
 ##
+# @BlockdevBackup
+#
+# @device: the name of the device which should be copied.
+#
+# @target: the name of the backup target 

[Qemu-devel] [PATCH v19 07/16] block: Add backing_blocker in BlockDriverState

[Fam Zheng]

This makes use of op_blocker and blocks all the operations except for
commit target, on each BlockDriverState->backing_hd.

The asserts for op_blocker in bdrv_swap are removed because with this
change, the target of block commit has at least the backing blocker of
its child, so the assertion is not true. Callers should do their check.

Signed-off-by: Fam Zheng 
---
 block.c   | 24 
 block/mirror.c|  1 +
 include/block/block_int.h |  3 +++
 3 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index ec26a2b..8155e68 100644
--- a/block.c
+++ b/block.c
@@ -1097,14 +1097,31 @@ fail:
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
 {
 
+if (bs->backing_hd) {
+assert(error_is_set(&bs->backing_blocker));
+bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
+} else if (backing_hd) {
+error_setg(&bs->backing_blocker,
+   "device is used as backing hd of '%s'",
+   bs->device_name);
+}
+
 bs->backing_hd = backing_hd;
 if (!backing_hd) {
+if (error_is_set(&bs->backing_blocker)) {
+error_free(bs->backing_blocker);
+}
 goto out;
 }
 bs->open_flags &= ~BDRV_O_NO_BACKING;
 pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename);
 pstrcpy(bs->backing_format, sizeof(bs->backing_format),
 backing_hd->drv ? backing_hd->drv->format_name : "");
+
+bdrv_op_block_all(bs->backing_hd, bs->backing_blocker);
+/* Otherwise we won't be able to commit due to check in bdrv_commit */
+bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT,
+bs->backing_blocker);
 out:
 bdrv_refresh_limits(bs);
 }
@@ -1758,8 +1775,9 @@ void bdrv_close(BlockDriverState *bs)
 
 if (bs->drv) {
 if (bs->backing_hd) {
-bdrv_unref(bs->backing_hd);
-bs->backing_hd = NULL;
+BlockDriverState *backing_hd = bs->backing_hd;
+bdrv_set_backing_hd(bs, NULL);
+bdrv_unref(backing_hd);
 }
 bs->drv->bdrv_close(bs);
 g_free(bs->opaque);
@@ -1961,7 +1979,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState 
*bs_old)
 assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
 assert(bs_new->job == NULL);
 assert(bs_new->dev == NULL);
-assert(bdrv_op_blocker_is_empty(bs_new));
 assert(bs_new->io_limits_enabled == false);
 assert(!throttle_have_timer(&bs_new->throttle_state));
 
@@ -1980,7 +1997,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState 
*bs_old)
 /* Check a few fields that should remain attached to the device */
 assert(bs_new->dev == NULL);
 assert(bs_new->job == NULL);
-assert(bdrv_op_blocker_is_empty(bs_new));
 assert(bs_new->io_limits_enabled == false);
 assert(!throttle_have_timer(&bs_new->throttle_state));
 
diff --git a/block/mirror.c b/block/mirror.c
index 1c38aa8..6a53d79 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -499,6 +499,7 @@ immediate_exit:
  * trigger the unref from the top one */
 BlockDriverState *p = s->base->backing_hd;
 s->base->backing_hd = NULL;
+bdrv_op_unblock_all(p, s->base->backing_blocker);
 bdrv_unref(p);
 }
 }
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 3518076..49e5824 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -368,6 +368,9 @@ struct BlockDriverState {
 BlockJob *job;
 
 QDict *options;
+
+/* The error object in use for blocking operations on backing_hd */
+Error *backing_blocker;
 };
 
 int get_tmp_filename(char *filename, int size);
-- 
1.9.2

[Qemu-devel] [PATCH v19 13/16] block: Allow backup on referenced named BlockDriverState

[Fam Zheng]

Drive backup is a read only operation on source bs. We want to allow
this specific case to enable image-fleecing. Note that when
image-fleecing job starts, the job still add its blocker to source bs,
and any other operation on it will be blocked by that.

Signed-off-by: Fam Zheng 
---
 block.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block.c b/block.c
index ab64027..b84b24e 100644
--- a/block.c
+++ b/block.c
@@ -1122,6 +1122,8 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
BlockDriverState *backing_hd)
 /* Otherwise we won't be able to commit due to check in bdrv_commit */
 bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT,
 bs->backing_blocker);
+bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_BACKUP_SOURCE,
+bs->backing_blocker);
 out:
 bdrv_refresh_limits(bs);
 }
-- 
1.9.2

[Qemu-devel] [PATCH v19 15/16] qemu-iotests: Test blockdev-backup in 055

[Fam Zheng]

This applies cases on drive-backup on blockdev-backup, except cases with
target format and mode.

Also add a case to check source == target.

Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/055 | 275 ++---
 tests/qemu-iotests/055.out |   4 +-
 2 files changed, 235 insertions(+), 44 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index 451b67d..1fab088 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -1,6 +1,6 @@
 #!/usr/bin/env python
 #
-# Tests for drive-backup
+# Tests for drive-backup and blockdev-backup
 #
 # Copyright (C) 2013 Red Hat, Inc.
 #
@@ -27,6 +27,7 @@ from iotests import qemu_img, qemu_io
 
 test_img = os.path.join(iotests.test_dir, 'test.img')
 target_img = os.path.join(iotests.test_dir, 'target.img')
+blockdev_target_img = os.path.join(iotests.test_dir, 'blockdev-target.img')
 
 class TestSingleDrive(iotests.QMPTestCase):
 image_len = 64 * 1024 * 1024 # MB
@@ -38,34 +39,48 @@ class TestSingleDrive(iotests.QMPTestCase):
 qemu_io('-c', 'write -P0xd5 1M 32k', test_img)
 qemu_io('-c', 'write -P0xdc 32M 124k', test_img)
 qemu_io('-c', 'write -P0xdc 67043328 64k', test_img)
+qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, 
str(TestSingleDrive.image_len))
 
-self.vm = iotests.VM().add_drive(test_img)
+self.vm = 
iotests.VM().add_drive(test_img).add_drive(blockdev_target_img)
 self.vm.launch()
 
 def tearDown(self):
 self.vm.shutdown()
 os.remove(test_img)
+os.remove(blockdev_target_img)
 try:
 os.remove(target_img)
 except OSError:
 pass
 
-def test_cancel(self):
+def do_test_cancel(self, test_drive_backup):
 self.assert_no_active_block_jobs()
 
-result = self.vm.qmp('drive-backup', device='drive0',
- target=target_img, sync='full')
+if test_drive_backup:
+result = self.vm.qmp('drive-backup', device='drive0',
+ target=target_img, sync='full')
+else:
+result = self.vm.qmp('blockdev-backup', device='drive0',
+ target='drive1', sync='full')
 self.assert_qmp(result, 'return', {})
 
 event = self.cancel_and_wait()
 self.assert_qmp(event, 'data/type', 'backup')
 
-def test_pause(self):
+def test_cancel(self):
+self.do_test_cancel(True)
+self.do_test_cancel(False)
+
+def do_test_pause(self, test_drive_backup):
 self.assert_no_active_block_jobs()
 
 self.vm.pause_drive('drive0')
-result = self.vm.qmp('drive-backup', device='drive0',
- target=target_img, sync='full')
+if test_drive_backup:
+result = self.vm.qmp('drive-backup', device='drive0',
+ target=target_img, sync='full')
+else:
+result = self.vm.qmp('blockdev-backup', device='drive0',
+ target='drive1', sync='full')
 self.assert_qmp(result, 'return', {})
 
 result = self.vm.qmp('block-job-pause', device='drive0')
@@ -86,14 +101,28 @@ class TestSingleDrive(iotests.QMPTestCase):
 self.wait_until_completed()
 
 self.vm.shutdown()
-self.assertTrue(iotests.compare_images(test_img, target_img),
-'target image does not match source after backup')
+if test_drive_backup:
+self.assertTrue(iotests.compare_images(test_img, target_img),
+'target image does not match source after backup')
+else:
+self.assertTrue(iotests.compare_images(test_img, 
blockdev_target_img),
+'target image does not match source after backup')
+
+def test_pause_drive_backup(self):
+self.do_test_pause(True)
+
+def test_pause_blockdev_backup(self):
+self.do_test_pause(False)
 
 def test_medium_not_found(self):
 result = self.vm.qmp('drive-backup', device='ide1-cd0',
  target=target_img, sync='full')
 self.assert_qmp(result, 'error/class', 'GenericError')
 
+result = self.vm.qmp('blockdev-backup', device='ide1-cd0',
+ target='drive1', sync='full')
+self.assert_qmp(result, 'error/class', 'GenericError')
+
 def test_image_not_found(self):
 result = self.vm.qmp('drive-backup', device='drive0',
  target=target_img, sync='full', mode='existing')
@@ -110,26 +139,56 @@ class TestSingleDrive(iotests.QMPTestCase):
  target=target_img, sync='full')
 self.assert_qmp(result, 'error/class', 'DeviceNotFound')
 
+result = self.vm.qmp('blockdev-backup', device='nonexistent',
+ target='drive0', sync='full')
+self.assert_qmp(result, 

[Qemu-devel] [PATCH v19 16/16] qemu-iotests: Image fleecing test case 089

[Fam Zheng]

This tests the workflow of creating a lightweight point-in-time snapshot
with blockdev-backup command, and exporting it with built-in NBD server.

It's tested that any post-snapshot writing to the original device
doesn't change data seen in NBD target.

Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/089 | 99 ++
 tests/qemu-iotests/089.out |  5 +++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 105 insertions(+)
 create mode 100755 tests/qemu-iotests/089
 create mode 100644 tests/qemu-iotests/089.out

diff --git a/tests/qemu-iotests/089 b/tests/qemu-iotests/089
new file mode 100755
index 000..8be32d7
--- /dev/null
+++ b/tests/qemu-iotests/089
@@ -0,0 +1,99 @@
+#!/usr/bin/env python
+#
+# Tests for image fleecing (point in time snapshot export to NBD)
+#
+# Copyright (C) 2014 Red Hat, Inc.
+#
+# Based on 055.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program 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 General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import time
+import os
+import iotests
+from iotests import qemu_img, qemu_io
+
+test_img = os.path.join(iotests.test_dir, 'test.img')
+target_img = os.path.join(iotests.test_dir, 'target.img')
+nbd_sock = os.path.join(iotests.test_dir, 'nbd.sock')
+
+class TestImageFleecing(iotests.QMPTestCase):
+image_len = 64 * 1024 * 1024 # MB
+
+def setUp(self):
+# Write data to the image so we can compare later
+qemu_img('create', '-f', iotests.imgfmt, test_img, 
str(TestImageFleecing.image_len))
+self.patterns = [
+("0x5d", "0", "64k"),
+("0xd5", "1M", "64k"),
+("0xdc", "32M", "64k"),
+("0xdc", "67043328", "64k")]
+
+for p in self.patterns:
+qemu_io('-c', 'write -P%s %s %s' % p, test_img)
+
+qemu_img('create', '-f', iotests.imgfmt, target_img, 
str(TestImageFleecing.image_len))
+
+self.vm = iotests.VM().add_drive(test_img)
+self.vm.launch()
+
+self.overwrite_patterns = [
+("0xa0", "0", "64k"),
+("0x0a", "1M", "64k"),
+("0x55", "32M", "64k"),
+("0x56", "67043328", "64k")]
+
+self.nbd_uri = "nbd+unix:///drive1?socket=%s" % nbd_sock
+
+def tearDown(self):
+self.vm.shutdown()
+os.remove(test_img)
+os.remove(target_img)
+
+def verify_patterns(self):
+for p in self.patterns:
+self.assertEqual(-1, qemu_io(self.nbd_uri, '-c', 'read -P%s %s %s' 
% p).find("verification failed"),
+ "Failed to verify pattern: %s %s %s" % p)
+
+def test_image_fleecing(self):
+result = self.vm.qmp("blockdev-add", **{"options": {
+"driver": "qcow2",
+"id": "drive1",
+"file": {
+"driver": "file",
+"filename": target_img,
+},
+"backing": "drive0",
+}})
+self.assert_qmp(result, 'return', {})
+result = self.vm.qmp("nbd-server-start", **{"addr": { "type": "unix", 
"data": { "path": nbd_sock } } })
+self.assert_qmp(result, 'return', {})
+result = self.vm.qmp("blockdev-backup", device="drive0", 
target="drive1", sync="none")
+self.assert_qmp(result, 'return', {})
+result = self.vm.qmp("nbd-server-add", device="drive1")
+self.assert_qmp(result, 'return', {})
+
+self.verify_patterns()
+
+for p in self.overwrite_patterns:
+self.vm.hmp_qemu_io("drive0", "write -P%s %s %s" % p)
+
+self.verify_patterns()
+
+self.cancel_and_wait(resume=True)
+self.assert_no_active_block_jobs()
+
+if __name__ == '__main__':
+iotests.main(supported_fmts=['raw', 'qcow2'])
diff --git a/tests/qemu-iotests/089.out b/tests/qemu-iotests/089.out
new file mode 100644
index 000..ae1213e
--- /dev/null
+++ b/tests/qemu-iotests/089.out
@@ -0,0 +1,5 @@
+.
+--
+Ran 1 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index ae09663..7f4b56b 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -95,4 +95,5 @@
 086 rw auto quick
 087 rw auto
 088 rw auto
+089 rw auto quick
 090 rw auto quick
-- 
1.9.2

[Qemu-devel] [PATCH v19 14/16] block: Add blockdev-backup to transaction

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 blockdev.c   | 48 
 qapi-schema.json |  3 +++
 2 files changed, 51 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 287a2d8..1a12e24 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1411,6 +1411,49 @@ static void drive_backup_abort(BlkTransactionState 
*common)
 }
 }
 
+typedef struct BlockdevBackupState {
+BlkTransactionState common;
+BlockDriverState *bs;
+BlockJob *job;
+} BlockdevBackupState;
+
+static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
+{
+BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, 
common);
+BlockdevBackup *backup;
+Error *local_err = NULL;
+
+assert(common->action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
+backup = common->action->blockdev_backup;
+
+qmp_blockdev_backup(backup->device, backup->target,
+backup->sync,
+backup->has_speed, backup->speed,
+backup->has_on_source_error, backup->on_source_error,
+backup->has_on_target_error, backup->on_target_error,
+&local_err);
+if (error_is_set(&local_err)) {
+error_propagate(errp, local_err);
+state->bs = NULL;
+state->job = NULL;
+return;
+}
+
+state->bs = bdrv_find(backup->device);
+state->job = state->bs->job;
+}
+
+static void blockdev_backup_abort(BlkTransactionState *common)
+{
+BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, 
common);
+BlockDriverState *bs = state->bs;
+
+/* Only cancel if it's the job we started */
+if (bs && bs->job && bs->job == state->job) {
+block_job_cancel_sync(bs->job);
+}
+}
+
 static void abort_prepare(BlkTransactionState *common, Error **errp)
 {
 error_setg(errp, "Transaction aborted using Abort action");
@@ -1433,6 +1476,11 @@ static const BdrvActionOps actions[] = {
 .prepare = drive_backup_prepare,
 .abort = drive_backup_abort,
 },
+[TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = {
+.instance_size = sizeof(BlockdevBackupState),
+.prepare = blockdev_backup_prepare,
+.abort = blockdev_backup_abort,
+},
 [TRANSACTION_ACTION_KIND_ABORT] = {
 .instance_size = sizeof(BlkTransactionState),
 .prepare = abort_prepare,
diff --git a/qapi-schema.json b/qapi-schema.json
index c6b10b9..510a8f8 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2001,11 +2001,14 @@
 #
 # A discriminated record of operations that can be performed with
 # @transaction.
+#
+# Since 1.1, blockdev-backup since 2.1
 ##
 { 'union': 'TransactionAction',
   'data': {
'blockdev-snapshot-sync': 'BlockdevSnapshot',
'drive-backup': 'DriveBackup',
+   'blockdev-backup': 'BlockdevBackup',
'abort': 'Abort',
'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
} }
-- 
1.9.2

Re: [Qemu-devel] My OS hangup in KVM for some reasons, how can I debug?

[Fam Zheng]

On Thu, 05/08 16:28, Jun Koi wrote:
> Hi,
> 
> I have an weird OS that I am trying to boot in KVM. however, it just hang
> in the middle, without a good reason.
> 
> The same OS boots fine in physical machine, and this OS comes with no
> source code.

The OS boots in one physical machine doesn't mean it has no bug, and doesn't
mean it boots on ALL physical machines. So you really have to debug to tell if
it's a bug of KVM or the OS. But you have no source code, it makes the problem
really hard to work on then.

But if you can see any error message or debug output of the guest, or any
observation that may help to provide some information.

Fam

[Qemu-devel] [PATCH] rules.mak: Rewrite unnest-vars

[Fam Zheng]

The macro unnest-vars is the most important, complicated but hard to
track magic in QEMU's build system.

Rewrite it in a (hopefully) clearer way, with more comments, to make it
easier to understand and maintain.

Remove DSO_CFLAGS and module-objs-m that are not used.

A bonus fix of this version is, per object variables are properly
protected in save-objs and load-objs, before including sub-dir
Makefile.objs, just as nested variables are. So the occasional same
object name from different directory levels won't step on each other's
foot.

Signed-off-by: Fam Zheng 
---
 rules.mak | 225 +++---
 1 file changed, 156 insertions(+), 69 deletions(-)

diff --git a/rules.mak b/rules.mak
index 5c454d8..56287c6 100644
--- a/rules.mak
+++ b/rules.mak
@@ -78,7 +78,7 @@ endif
 %.o: %.dtrace
$(call quiet-command,dtrace -o $@ -G -s $<, "  GEN   $(TARGET_DIR)$@")
 
-DSO_CFLAGS := -fPIC -DBUILD_DSO
+%$(DSOSUF): CFLAGS += -fPIC -DBUILD_DSO
 %$(DSOSUF): LDFLAGS += $(LDFLAGS_SHARED)
 %$(DSOSUF): %.mo libqemustub.a
$(call LINK,$^)
@@ -161,81 +161,168 @@ clean: clean-timestamp
 # will delete the target of a rule if commands exit with a nonzero exit status
 .DELETE_ON_ERROR:
 
-# magic to descend into other directories
-
-define push-var
-$(eval save-$2-$1 = $(value $1))
-$(eval $1 :=)
-endef
-
-define pop-var
-$(eval subdir-$2-$1 := $(if $(filter $2,$(save-$2-$1)),$(addprefix $2,$($1
-$(eval $1 = $(value save-$2-$1) $$(subdir-$2-$1))
-$(eval save-$2-$1 :=)
-endef
-
-define fix-obj-vars
-$(foreach v,$($1), \
-   $(if $($v-cflags), \
-   $(eval $2$v-cflags := $($v-cflags)) \
-   $(eval $v-cflags := )) \
-   $(if $($v-libs), \
-   $(eval $2$v-libs := $($v-libs)) \
-   $(eval $v-libs := )) \
-   $(if $($v-objs), \
-   $(eval $2$v-objs := $(addprefix $2,$($v-objs))) \
-   $(eval $v-objs := )))
+# save-vars
+# Usage: $(call save-vars, vars)
+# Save each variable $v in $vars as save-vars-$v, save their object's
+# variables, then clear $v.
+define save-vars
+$(foreach v,$1,
+$(eval save-vars-$v := $(value $v))
+$(foreach o,$($v),
+$(foreach k,cflags libs objs,
+$(if $($o-$k),
+$(eval save-vars-$o-$k := $($o-$k))
+$(eval $o-$k := 
+$(eval $v := ))
 endef
 
-define unnest-dir
-$(foreach var,$(nested-vars),$(call push-var,$(var),$1/))
-$(eval obj-parent-$1 := $(obj))
-$(eval obj := $(if $(obj),$(obj)/$1,$1))
-$(eval include $(SRC_PATH)/$1/Makefile.objs)
-$(foreach v,$(nested-vars),$(call fix-obj-vars,$v,$(if $(obj),$(obj)/)))
-$(eval obj := $(obj-parent-$1))
-$(eval obj-parent-$1 := )
-$(foreach var,$(nested-vars),$(call pop-var,$(var),$1/))
+# load-vars
+# Usage: $(call load-vars, vars, add_var)
+# Load the saved value for each variable in @vars, and the per object
+# variables.
+# Append @add_var's current value to the loaded value.
+define load-vars
+   $(eval $2-new-value := $(value $2))
+$(foreach v,$1,
+$(eval $v := $(value save-vars-$v))
+$(foreach o,$($v),
+$(foreach k,cflags libs objs,
+$(if $(save-vars-$o-$k),
+$(eval $o-$k := $(save-vars-$o-$k))
+$(eval save-vars-$o-$k := 
+$(eval save-vars-$v := ))
+   $(eval $2 := $(value $2) $($2-new-value))
 endef
 
-define unnest-vars-1
-$(eval nested-dirs := $(filter-out \
-$(old-nested-dirs), \
-$(sort $(foreach var,$(nested-vars), $(filter %/, $($(var)))
-$(if $(nested-dirs),
-  $(foreach dir,$(nested-dirs),$(call unnest-dir,$(patsubst %/,%,$(dir
-  $(eval old-nested-dirs := $(old-nested-dirs) $(nested-dirs))
-  $(call unnest-vars-1))
+# fix-paths
+# Usage: $(call fix-paths, obj_path, src_path, vars)
+# Add prefix @obj_path to all objects in @vars, and add prefix @src_path to all
+# directories in @vars.
+define fix-paths
+$(foreach v,$3,
+$(foreach o,$($v),
+$(if $($o-libs),
+$(eval $1$o-libs := $(value $o-libs)))
+$(if $($o-cflags),
+$(eval $1$o-cflags := $(value $o-cflags)))
+$(if $($o-objs),
+$(eval $1$o-objs := $(addprefix $1,$(value $o-objs)
+$(eval $v := $(addprefix $1,$(filter-out %/,$(value $v))) \
+ $(addprefix $2,$(filter %/,$(value $v)
 endef
 
-define process-modules
-$(foreach o,$(filter %.o,$($1)),
-   $(eval $(patsubst %.o,%.mo,$o): $o) \
-   $(eval $(patsubst %.o,%.mo,$o)-objs := $o))
-$(foreach o,$(filter-out $(modules-m), $(patsubst %.o,%.mo,$($1))), \
-$(eval $o-objs += module-common.o)
-$(eval $o: $($o-objs))
-$(eval modules-objs-m += $($o-objs))
-$(eval modules-m += $o)
-$(eval $o:; $$(call quiet-command,touch $$@,"  GEN   $$(TARGET_DIR)$$@"))
-$(if $(CONFIG_MODULES),$(eval modules: $(patsubst %.mo,%$(DSOSUF),$o \
-$(eval m

Re: [Qemu-devel] uvesafb doesn't work with seabios

[Kevin O'Connor]

On Sun, May 11, 2014 at 04:19:55PM +0200, Bernhard Walle wrote:
> Am 2014-05-11 14:37, schrieb Kevin O'Connor:
> >On Sun, May 11, 2014 at 01:42:57PM +0200, Bernhard Walle wrote:
> >>Am 10.05.14 17:07, schrieb Kevin O'Connor:
> >>> Also, it looks like uvesafb can call x86emu.  Older versions of x86emu
> >>> are known to not emulate some instructions properly, and it has caused
> >>> problems for SeaVGABIOS.  The most recent version of SeaVGABIOS from
> >>> SeaBIOS git has additional checks for this - can you grab the seabios
> >>> git, set CONFIG_DEBUG_LEVEL to 8, build SeaVGABIOS (set
> >>> CONFIG_VGA_BOCHS), and post the logs from both Linux and QEMU with the
> >>> above options?
> >>
> >>It doesn't change the result (that it doesn't work). It only makes the
> >>kernel failing much faster and of course there's much more debugging
> >>output: http://bwalle.de/tmp/debugoutput (250k).
> >
> >Were there any Linux or userspace messages with the above?
> 
> 
> Yes:
> 
>   uvesafb: Getting VBE info block failed (eax=0x1d2, err=0)
>   uvesafb: vbe_init() failed with -22
>   uvesafb: probe of uvesafb.0 failed with error -22

It does look like the x86emu issue.  You can try applying the
SeaVGABIOS patch below to confirm it.

-Kevin


--- a/vgasrc/vgaentry.S
+++ b/vgasrc/vgaentry.S
@@ -47,7 +47,14 @@ _rom_header_signature:
 
 // Force a fault if found to be running on broken x86emu versions.
 DECLFUNC x86emu_fault
+msg:.ascii "SeaVGABIOS: x86emu leal trap!\n"
 x86emu_fault:
+#if CONFIG_DEBUG_IO
+movw %cs:DebugOutputPort, %dx
+movl $30, %ecx
+movl $msg, %esi
+rep outsb %cs:(%esi), (%dx)
+#endif
 1:  hlt
 jmp 1b
 

Re: [Qemu-devel] [PATCH] migration: cache memory region ram ptr

[Paolo Bonzini]

Il 10/05/2014 18:32, Peter Lieven ha scritto:

What about XEN?



You're right, Xen wouldn't work.  Your original patch would not break it 
just because Xen doesn't use migration (but the code would be broken).


You would have to cache qemu_get_ram_block rather than qemu_get_ram_ptr, 
move RAMBlock to memory-internal.h, and split the RAMBlock + ram_addr_t 
=> void * conversion out of qemu_get_ram_ptr and into a separate 
function (to be used by memory_region_get_ram_ptr).


I'm not sure of the benefit of your patch though.  qemu_get_ram_block 
already has a 1-item cache, are you seeing a low hit rate there?  Or any 
other profiling that shows qemu_get_ram_ptr as hot?


Paolo

Re: [Qemu-devel] [PATCH v2] cirrus_vga: adding sanity check for vram size

[Gonglei (Arei)]

Hi,

> > diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> > index d1afc76..399a2ef 100644
> > --- a/hw/display/cirrus_vga.c
> > +++ b/hw/display/cirrus_vga.c
> > @@ -2959,6 +2959,14 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
> >   PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> >   int16_t device_id = pc->device_id;
> >
> > + /* follow real hardware, cirrus card emulated has 4 MB video
> memory.
> > +   Also accept 8 MB/16 MB for backward compatibility. */
> > + if (s->vga.vram_size_mb != 4 || s->vga.vram_size_mb != 8 ||
> > + s->vga.vram_size_mb != 16) {
> 
> This condition will always be true, because a number can't be equal
> to 3 _different_ numbers at the same time.
> 
Yep, good catch. Thanks.

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2] cirrus_vga: adding sanity check for vram size

[Gonglei (Arei)]

Hi,

> > diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> > index d1afc76..399a2ef 100644
> > --- a/hw/display/cirrus_vga.c
> > +++ b/hw/display/cirrus_vga.c
> > @@ -2959,6 +2959,14 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
> >   PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> >   int16_t device_id = pc->device_id;
> >
> > + /* follow real hardware, cirrus card emulated has 4 MB video
> memory.
> > +   Also accept 8 MB/16 MB for backward compatibility. */
> > + if (s->vga.vram_size_mb != 4 || s->vga.vram_size_mb != 8 ||
> > + s->vga.vram_size_mb != 16) {
> 
> Apart from the logic bug mjt already pointed out, I note that this check
> is in the PCI initfn. Should the same restriction also apply for the ISA
> version of the device?
> 
Yes, I have noted this issue in v1. In other mail, you gave me some advices,
thanks a lot, Andreas. I will post v3.

> > + error_report("Invalid cirrus_vga ram size '%u'\n",
> > +  s->vga.vram_size_mb);
> 
> Thanks for using our new error_report(). It does not require a trailing
> \n though.
> 
Got it. Thanks.

Best regards,
-Gonglei