Re: [Qemu-block] [PATCH] virtio-blk: Drop x-data-plane option

[Cornelia Huck]

On Mon, 7 Dec 2015 16:19:07 +0100
Paolo Bonzini  wrote:

> The solution would be to add object_property_add_child to
> virtio_blk_data_plane_create, between object_initialize and
> user_creatable_complete.  But I think this patch is ok for 2.5.

Just sent a patch that does this. If it is correct, I'd prefer it over
either removing x-data-plane or reverting the iothread patch.

Re: [Qemu-block] [PATCH RFC for-2.6 0/3] block: Add meta dirty bitmap for migration/persistence

[Vladimir Sementsov-Ogievskiy]

On 07.12.2015 08:59, Fam Zheng wrote:

Vladimir,

This is what I propose to implement meta bitmap. It's implemented in the
HBitmap level to be more efficient, and the interface slightly varies too.


What is the benefit?

Hbitmap usage:

1) BdrvDirtyBitmap - need meta
2) BackupBlockJob - doesn't need meta
3) BlockDirtyBitmapState - doesn't need meta
4) now I'm working on series for parallels format and I use HBitmap to 
mark allocated/free clusters.. - doesn't need meta

5) your meta hbitmap =) - doesn't need meta..

So, what is the benefit of moving this functionality to parent class? 
(Which is complicated without it)..


However, I'm not really against, except my comment to the first patch.

PS:
Actually I don't like HBitmap - BdrvDirtyBitmap..
- No implementation without granularity
   I need HBitmap without granularity for my needs and have to use 
granularity=0. If there was HBitmap without granularity some operations 
can be faster - for example, finding next/previous/last zeros, jumping 
by words not by bits..

- It is not sparse. Empty bitmap occupies lots of ram.
- different granularity units for HBitmap and BdrvDirtyBitmap
- different layers with/without granularity in hbitmap.c
- HBitmap with non-zero granularity doesn't know its size (only rounded 
up to granularity)

- necessity of writing wrappers like
   bdrv_dirty_bitmap_do_something(...)
   {
hbitmap_do_something(...)
   }
   -- Yes, I understand that this is inevitably, but I just don't like it..
- BdrvDirtyBitmap is defined in block.c.. I think, it should have its 
own .c file.





I'd like to use these operations to make dirty bitmap persistence more
efficient too: unchanged dirty bits don't need to be flushed to disk. So I'm
posting this as a separate series for a common base for both sides.

Posting as RFC as 2.6 dev phase is just starting, we can still tweak the
interface and/or implementation to fit the need.

Fam Zheng (3):
   HBitmap: Introduce "meta" bitmap to track bit changes
   tests: Add test code for meta bitmap
   block: Support meta dirty bitmap

  block.c| 46 ++-
  block/mirror.c |  3 +-
  blockdev.c |  3 +-
  include/block/block.h  | 11 
  include/qemu/hbitmap.h |  7 +
  migration/block.c  |  2 +-
  tests/test-hbitmap.c   | 74 ++
  util/hbitmap.c | 22 +++
  8 files changed, 164 insertions(+), 4 deletions(-)




--
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.

[Qemu-block] [PATCH for-2.5] virtio-blk/dataplane: parentize compat iothread

[Cornelia Huck]

For x-data-plane=true, we create an iothread automatically for
compatibility. Commit d21e877 ("iothread: include id in thread name")
exposed that this iothread missed correct parenthood: fix this.

Signed-off-by: Cornelia Huck 
---
 hw/block/dataplane/virtio-blk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index c42ddeb..c7e5668 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -187,6 +187,9 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, 
VirtIOBlkConf *conf,
 object_initialize(>internal_iothread_obj,
   sizeof(s->internal_iothread_obj),
   TYPE_IOTHREAD);
+object_property_add_child(OBJECT(conf), TYPE_IOTHREAD,
+  OBJECT(>internal_iothread_obj),
+  _abort);
 user_creatable_complete(OBJECT(>internal_iothread_obj), 
_abort);
 s->iothread = >internal_iothread_obj;
 }
-- 
2.3.9

Re: [Qemu-block] [PATCH RFC for-2.6 1/3] HBitmap: Introduce "meta" bitmap to track bit changes

[Vladimir Sementsov-Ogievskiy]

On 07.12.2015 08:59, Fam Zheng wrote:

The meta bitmap will have the same size and granularity as the tracked
bitmap, and upon each bit toggle, the corresponding bit in the meta
bitmap, at an identical position, will be set.


No, meta bitmap should not have same granularity. If we have 16tb 
storage, then 16kb granulated bitmap will occupy more then 128 mb. And 
additional meta bitmap of the same size and granularity is redundant 
128+ mb of RAM, when actually we need meta bitmap for blocks for example 
of 1mb and it should occupy about 128 bits.





Signed-off-by: Fam Zheng 
---
  include/qemu/hbitmap.h |  7 +++
  util/hbitmap.c | 22 ++
  2 files changed, 29 insertions(+)

diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index bb94a00..09a6b06 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -181,6 +181,13 @@ void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap 
*hb, uint64_t first);
   */
  unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi);
  
+/* hbitmap_create_meta

+ * @hb: The HBitmap to operate on.
+ *
+ * Create a "meta" hbitmap to track dirtiness of the bits in this HBitmap.
+ */
+HBitmap *hbitmap_create_meta(HBitmap *hb);
+
  /**
   * hbitmap_iter_next:
   * @hbi: HBitmapIter to operate on.
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 50b888f..3ad406e 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -81,6 +81,9 @@ struct HBitmap {
   */
  int granularity;
  
+/* A meta dirty bitmap to track the dirtiness of bits in this HBitmap. */

+HBitmap *meta;
+
  /* A number of progressively less coarse bitmaps (i.e. level 0 is the
   * coarsest).  Each bit in level N represents a word in level N+1 that
   * has a set bit, except the last level where each bit represents the
@@ -232,6 +235,7 @@ static inline bool hb_set_elem(unsigned long *elem, 
uint64_t start, uint64_t las
  /* The recursive workhorse (the depth is limited to HBITMAP_LEVELS)... */
  static void hb_set_between(HBitmap *hb, int level, uint64_t start, uint64_t 
last)
  {
+uint64_t save_start = start;
  size_t pos = start >> BITS_PER_LEVEL;
  size_t lastpos = last >> BITS_PER_LEVEL;
  bool changed = false;
@@ -252,6 +256,9 @@ static void hb_set_between(HBitmap *hb, int level, uint64_t 
start, uint64_t last
  }
  }
  changed |= hb_set_elem(>levels[level][i], start, last);
+if (hb->meta && level == HBITMAP_LEVELS - 1 && changed) {
+hbitmap_set(hb->meta, save_start, last - save_start + 1);
+}
  
  /* If there was any change in this layer, we may have to update

   * the one above.
@@ -298,6 +305,7 @@ static inline bool hb_reset_elem(unsigned long *elem, 
uint64_t start, uint64_t l
  /* The recursive workhorse (the depth is limited to HBITMAP_LEVELS)... */
  static void hb_reset_between(HBitmap *hb, int level, uint64_t start, uint64_t 
last)
  {
+uint64_t save_start = start;
  size_t pos = start >> BITS_PER_LEVEL;
  size_t lastpos = last >> BITS_PER_LEVEL;
  bool changed = false;
@@ -336,6 +344,10 @@ static void hb_reset_between(HBitmap *hb, int level, 
uint64_t start, uint64_t la
  lastpos--;
  }
  
+if (hb->meta && level == HBITMAP_LEVELS - 1 && changed) {

+hbitmap_set(hb->meta, save_start, last - save_start + 1);
+}
+
  if (level > 0 && changed) {
  hb_reset_between(hb, level - 1, pos, lastpos);
  }
@@ -384,6 +396,9 @@ void hbitmap_free(HBitmap *hb)
  for (i = HBITMAP_LEVELS; i-- > 0; ) {
  g_free(hb->levels[i]);
  }
+if (hb->meta) {
+hbitmap_free(hb->meta);
+}
  g_free(hb);
  }
  
@@ -493,3 +508,10 @@ bool hbitmap_merge(HBitmap *a, const HBitmap *b)
  
  return true;

  }
+
+HBitmap *hbitmap_create_meta(HBitmap *hb)
+{
+assert(!hb->meta);
+hb->meta = hbitmap_alloc(hb->size, hb->granularity);
+return hb->meta;
+}



--
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.

Re: [Qemu-block] [PATCH] virtio-blk: Drop x-data-plane option

[Paolo Bonzini]

On 07/12/2015 14:02, Fam Zheng wrote:
> On Mon, 12/07 12:29, Cornelia Huck wrote:
>> On Mon,  7 Dec 2015 18:59:27 +0800
>> Fam Zheng  wrote:
>>
>>> The official way of enabling dataplane is through the "iothread"
>>> property that references an iothread object created by "-object
>>> iothread".  Since the old "x-data-plane=on" way now even crashes, it's
>>> probably easier to just drop it:
>>>
>>> $ qemu-system-x86_64 -drive file=null-co://,id=d0,if=none \
>>> -device virtio-blk-pci,drive=d0,x-data-plane=on
>>>
>>> ERROR:/home/fam/work/qemu/qom/object.c:1515:
>>> object_get_canonical_path_component: assertion failed: (obj->parent != NULL)
>>> Aborted
>>
>> Do we understand yet why this crashes, btw?
> 
> I think it's because with x-data-plane=on, virtio-blk initialize an object 
> that
> doesn't have a parent, therefore it doesn't have a valid "canonical path
> component" thing, which is different from objects created with "-object" CLI.
> I'm not very familiar with the QOM semantics here.
> 
>>
>>>
>>> Signed-off-by: Fam Zheng 
>>> ---
>>>  hw/block/dataplane/virtio-blk.c | 15 ++-
>>>  hw/block/virtio-blk.c   |  1 -
>>>  include/hw/virtio/virtio-blk.h  |  1 -
>>>  3 files changed, 2 insertions(+), 15 deletions(-)
>>>
>>
>> No general objection to removing x-data-plane; but this probably wants
>> a mention on the changelog as x-data-plane has been described in
>> various howtos etc. over the years.
> 
> Yes, that is a good point.  I don't know if it's too rushing in removing it 
> for
> 2.5 (this is just posted as one option) and we'll have to count on QOM experts
> for the fix, if it is.

The solution would be to add object_property_add_child to
virtio_blk_data_plane_create, between object_initialize and
user_creatable_complete.  But I think this patch is ok for 2.5.

Paolo

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.5] virtio-blk/dataplane: parentize compat iothread

[Cornelia Huck]

On Mon, 7 Dec 2015 16:33:11 +
Peter Maydell  wrote:

> On 7 December 2015 at 16:27, Cornelia Huck  wrote:
> > On Mon,  7 Dec 2015 16:50:01 +0100
> > Cornelia Huck  wrote:
> >
> >> For x-data-plane=true, we create an iothread automatically for
> >> compatibility. Commit d21e877 ("iothread: include id in thread name")
> >> exposed that this iothread missed correct parenthood: fix this.
> >>
> >> Signed-off-by: Cornelia Huck 
> >> ---
> >>  hw/block/dataplane/virtio-blk.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/hw/block/dataplane/virtio-blk.c 
> >> b/hw/block/dataplane/virtio-blk.c
> >> index c42ddeb..c7e5668 100644
> >> --- a/hw/block/dataplane/virtio-blk.c
> >> +++ b/hw/block/dataplane/virtio-blk.c
> >> @@ -187,6 +187,9 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, 
> >> VirtIOBlkConf *conf,
> >>  object_initialize(>internal_iothread_obj,
> >>sizeof(s->internal_iothread_obj),
> >>TYPE_IOTHREAD);
> >> +object_property_add_child(OBJECT(conf), TYPE_IOTHREAD,
> >> +  OBJECT(>internal_iothread_obj),
> >> +  _abort);
> >>  user_creatable_complete(OBJECT(>internal_iothread_obj), 
> >> _abort);
> >>  s->iothread = >internal_iothread_obj;
> >>  }
> >
> > Scratch that, does not work... (I thought I was hitting the other
> > segfault in the block backend processing...)
> 
> Should I go ahead and apply Fam's "remove x-data-plane" patch then?

That's probably the quickest solution, as the iothread name patch is
useful and I haven't been able to come up with a correct patch.

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.5] virtio-blk/dataplane: parentize compat iothread

[Cornelia Huck]

On Mon,  7 Dec 2015 16:50:01 +0100
Cornelia Huck  wrote:

> For x-data-plane=true, we create an iothread automatically for
> compatibility. Commit d21e877 ("iothread: include id in thread name")
> exposed that this iothread missed correct parenthood: fix this.
> 
> Signed-off-by: Cornelia Huck 
> ---
>  hw/block/dataplane/virtio-blk.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index c42ddeb..c7e5668 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -187,6 +187,9 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, 
> VirtIOBlkConf *conf,
>  object_initialize(>internal_iothread_obj,
>sizeof(s->internal_iothread_obj),
>TYPE_IOTHREAD);
> +object_property_add_child(OBJECT(conf), TYPE_IOTHREAD,
> +  OBJECT(>internal_iothread_obj),
> +  _abort);
>  user_creatable_complete(OBJECT(>internal_iothread_obj), 
> _abort);
>  s->iothread = >internal_iothread_obj;
>  }

Scratch that, does not work... (I thought I was hitting the other
segfault in the block backend processing...)

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.5] virtio-blk/dataplane: parentize compat iothread

[Peter Maydell]

On 7 December 2015 at 16:27, Cornelia Huck  wrote:
> On Mon,  7 Dec 2015 16:50:01 +0100
> Cornelia Huck  wrote:
>
>> For x-data-plane=true, we create an iothread automatically for
>> compatibility. Commit d21e877 ("iothread: include id in thread name")
>> exposed that this iothread missed correct parenthood: fix this.
>>
>> Signed-off-by: Cornelia Huck 
>> ---
>>  hw/block/dataplane/virtio-blk.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/block/dataplane/virtio-blk.c 
>> b/hw/block/dataplane/virtio-blk.c
>> index c42ddeb..c7e5668 100644
>> --- a/hw/block/dataplane/virtio-blk.c
>> +++ b/hw/block/dataplane/virtio-blk.c
>> @@ -187,6 +187,9 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, 
>> VirtIOBlkConf *conf,
>>  object_initialize(>internal_iothread_obj,
>>sizeof(s->internal_iothread_obj),
>>TYPE_IOTHREAD);
>> +object_property_add_child(OBJECT(conf), TYPE_IOTHREAD,
>> +  OBJECT(>internal_iothread_obj),
>> +  _abort);
>>  user_creatable_complete(OBJECT(>internal_iothread_obj), 
>> _abort);
>>  s->iothread = >internal_iothread_obj;
>>  }
>
> Scratch that, does not work... (I thought I was hitting the other
> segfault in the block backend processing...)

Should I go ahead and apply Fam's "remove x-data-plane" patch then?

thanks
-- PMM

[Qemu-block] [PATCH] qcow2: always initialize specific image info

[Roman Kagan]

qcow2_get_specific_info() used to have a code path which would leave
pointer to ImageInfoSpecificQCow2 uninitialized.

We guess that it caused sporadic crashes on freeing an invalid pointer
in response to "query-block" QMP command in
visit_type_ImageInfoSpecificQCow2 with QapiDeallocVisitor.

Although we have neither a solid proof nor a reproduction scenario,
making sure the field is initialized appears a reasonable thing to do.

Signed-off-by: Roman Kagan 
---
 block/qcow2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 88f56c8..67c9d3d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2739,7 +2739,7 @@ static ImageInfoSpecific 
*qcow2_get_specific_info(BlockDriverState *bs)
 
 *spec_info = (ImageInfoSpecific){
 .type  = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
-.u.qcow2 = g_new(ImageInfoSpecificQCow2, 1),
+.u.qcow2 = g_new0(ImageInfoSpecificQCow2, 1),
 };
 if (s->qcow_version == 2) {
 *spec_info->u.qcow2 = (ImageInfoSpecificQCow2){
-- 
2.5.0

Re: [Qemu-block] [PATCH] virtio-blk: Drop x-data-plane option

[Peter Maydell]

On 7 December 2015 at 15:19, Paolo Bonzini  wrote:
>
>
> On 07/12/2015 14:02, Fam Zheng wrote:
>> On Mon, 12/07 12:29, Cornelia Huck wrote:
>>> On Mon,  7 Dec 2015 18:59:27 +0800
>>> Fam Zheng  wrote:
>>>
 The official way of enabling dataplane is through the "iothread"
 property that references an iothread object created by "-object
 iothread".  Since the old "x-data-plane=on" way now even crashes, it's
 probably easier to just drop it:

 $ qemu-system-x86_64 -drive file=null-co://,id=d0,if=none \
 -device virtio-blk-pci,drive=d0,x-data-plane=on

 ERROR:/home/fam/work/qemu/qom/object.c:1515:
 object_get_canonical_path_component: assertion failed: (obj->parent != 
 NULL)
 Aborted
>>>
>>> Do we understand yet why this crashes, btw?
>>
>> I think it's because with x-data-plane=on, virtio-blk initialize an object 
>> that
>> doesn't have a parent, therefore it doesn't have a valid "canonical path
>> component" thing, which is different from objects created with "-object" CLI.
>> I'm not very familiar with the QOM semantics here.
>>
>>>

 Signed-off-by: Fam Zheng 
 ---
  hw/block/dataplane/virtio-blk.c | 15 ++-
  hw/block/virtio-blk.c   |  1 -
  include/hw/virtio/virtio-blk.h  |  1 -
  3 files changed, 2 insertions(+), 15 deletions(-)

>>>
>>> No general objection to removing x-data-plane; but this probably wants
>>> a mention on the changelog as x-data-plane has been described in
>>> various howtos etc. over the years.
>>
>> Yes, that is a good point.  I don't know if it's too rushing in removing it 
>> for
>> 2.5 (this is just posted as one option) and we'll have to count on QOM 
>> experts
>> for the fix, if it is.
>
> The solution would be to add object_property_add_child to
> virtio_blk_data_plane_create, between object_initialize and
> user_creatable_complete.  But I think this patch is ok for 2.5.

Paolo asked me to apply this to master, so I have done so.

thanks
-- PMM

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.6 0/2] Preparation for PCI devices convert to realize()

[Markus Armbruster]

Cao jin  writes:

> Hi John
>
> On 12/05/2015 01:55 AM, John Snow wrote:
>>
>>
>> On 12/04/2015 02:47 AM, Cao jin wrote:
>>> Hi,
>>>  As you know, there are many PCI devices still using .init() as its
>>>  initialization function, I am planning to do the "convert to realize()"
>>>  work, and PCI bridge devices are chosen first.
>>>  The supporting functions should be modified first. msi_init() a 
>>> supporting
>>>  function for PCI devices.
>>>
>>>  Maybe it should be put in 2.6, as title indicated
>>>
>>> Cao jin (2):
>>>Add param Error** to msi_init()
>>>Modify callers of msi_init()
>>>
>>>   hw/audio/intel-hda.c   |  7 ++-
>>>   hw/ide/ich.c   |  2 +-
>>>   hw/net/vmxnet3.c   |  3 ++-
>>>   hw/pci-bridge/ioh3420.c|  6 +-
>>>   hw/pci-bridge/pci_bridge_dev.c |  6 +-
>>>   hw/pci-bridge/xio3130_downstream.c |  7 ++-
>>>   hw/pci-bridge/xio3130_upstream.c   |  7 ++-
>>>   hw/pci/msi.c   | 17 +
>>>   hw/scsi/megasas.c  |  2 +-
>>>   hw/scsi/vmw_pvscsi.c   |  3 ++-
>>>   hw/usb/hcd-xhci.c  |  5 -
>>>   hw/vfio/pci.c  |  3 ++-
>>>   include/hw/pci/msi.h   |  4 ++--
>>>   13 files changed, 55 insertions(+), 17 deletions(-)
>>>
>>
>> You'll need to squash these patches as the first patch will break git
>> bisect.
>>
>
> Ok, will squash it. And I have another question： what`s the benefit
> of converting to realize? Because AFAICT, doing this make the error
> reporting machanism seems clean & clear, all device-init errors are
> passed above along the call chain. I mean, besides, are there any
> other benefits?

Let's compare behavior on error:

* realize methods pass an error object to their caller

  The caller decides how to handle the error.

  Many callers simply pass it on.  Typically, errors from realize are
  passed up the call chain through qdev_device_add().

  Eventually, the caller that handles the error is reached.  If the
  error needs to be reported to the user, it decides how.  Examples:

  - device_init_func(), which does the actual work for -device, reports
the human-readable message to stderr.

  - hmp_device_add(), which does the work for HMP device_add, reports
the human-readable message to to the appropriate monitor.

  - handle_qmp_command(), which handles all QMP commands including QMP
device_add, sends a QMP error reply down the QMP connection.

  - petalogix_ml605_init(), which creates the "petalogix-ml605" board,
aborts when realizing the CPU fails.

* init methods report to stderr and return -1

  Fine when the error should be reported to stderr.  This is fine for
  some of the above examples, and wrong on others.  In particular, a
  device needs to provide a realize method to be fully work with
  device_add, unless its init method cannot fail.

Re: [Qemu-block] [PATCH v3 00/21] block: Cache mode for children etc.

[Kevin Wolf]

Am 04.12.2015 um 14:35 hat Kevin Wolf geschrieben:
> This is part three (or four, depending on whether you count the bdrv_swap
> removal) of what I had sent earlier as "[PATCH 00/34] block: Cache mode for
> children, reopen overhaul and more". Most of the patches were actually already
> reviewed in v1.
> 
> This part contains the remaining functional changes that the cover letter for
> v1 advertised, and a bit more:
> 
> - You can now use node name references for backing files
> - bdrv_reopen() works now properly for inherited options (don't exist before
>   this series; after the series the cache options)
> - bdrv_reopen() works now properly with semantically overlapping options
> - bdrv_reopen() can change child node options
> - And finally you can set cache mode options for backing files and other
>   children now (and the reopen behaviour even makes sense

Applied to block-next.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.6 0/2] Preparation for PCI devices convert to realize()

[Markus Armbruster]

Markus Armbruster  writes:

> Cao jin  writes:
>
>> Hi John
>>
>> On 12/05/2015 01:55 AM, John Snow wrote:
>>>
>>>
>>> On 12/04/2015 02:47 AM, Cao jin wrote:
 Hi,
  As you know, there are many PCI devices still using .init() as its
  initialization function, I am planning to do the "convert to 
 realize()"
  work, and PCI bridge devices are chosen first.
  The supporting functions should be modified first. msi_init() a 
 supporting
  function for PCI devices.

  Maybe it should be put in 2.6, as title indicated

 Cao jin (2):
Add param Error** to msi_init()
Modify callers of msi_init()

   hw/audio/intel-hda.c   |  7 ++-
   hw/ide/ich.c   |  2 +-
   hw/net/vmxnet3.c   |  3 ++-
   hw/pci-bridge/ioh3420.c|  6 +-
   hw/pci-bridge/pci_bridge_dev.c |  6 +-
   hw/pci-bridge/xio3130_downstream.c |  7 ++-
   hw/pci-bridge/xio3130_upstream.c   |  7 ++-
   hw/pci/msi.c   | 17 +
   hw/scsi/megasas.c  |  2 +-
   hw/scsi/vmw_pvscsi.c   |  3 ++-
   hw/usb/hcd-xhci.c  |  5 -
   hw/vfio/pci.c  |  3 ++-
   include/hw/pci/msi.h   |  4 ++--
   13 files changed, 55 insertions(+), 17 deletions(-)

>>>
>>> You'll need to squash these patches as the first patch will break git
>>> bisect.
>>>
>>
>> Ok, will squash it. And I have another question： what`s the benefit
>> of converting to realize? Because AFAICT, doing this make the error
>> reporting machanism seems clean & clear, all device-init errors are
>> passed above along the call chain. I mean, besides, are there any
>> other benefits?
>
> Let's compare behavior on error:
>
> * realize methods pass an error object to their caller
>
>   The caller decides how to handle the error.
>
>   Many callers simply pass it on.  Typically, errors from realize are
>   passed up the call chain through qdev_device_add().
>
>   Eventually, the caller that handles the error is reached.  If the
>   error needs to be reported to the user, it decides how.  Examples:
>
>   - device_init_func(), which does the actual work for -device, reports
> the human-readable message to stderr.
>
>   - hmp_device_add(), which does the work for HMP device_add, reports
> the human-readable message to to the appropriate monitor.
>
>   - handle_qmp_command(), which handles all QMP commands including QMP
> device_add, sends a QMP error reply down the QMP connection.
>
>   - petalogix_ml605_init(), which creates the "petalogix-ml605" board,
> aborts when realizing the CPU fails.
>
> * init methods report to stderr and return -1
>
>   Fine when the error should be reported to stderr.

Correction: init methods should use error_report(), which reports either
to stderr or the current monitor.

>  This is fine for
>   some of the above examples, and wrong on others.  In particular, a
>   device needs to provide a realize method to be fully work with
>   device_add, unless its init method cannot fail.

This is the case all the same.

[Qemu-block] [PATCH] virtio-blk: Drop x-data-plane option

[Fam Zheng]

The official way of enabling dataplane is through the "iothread"
property that references an iothread object created by "-object
iothread".  Since the old "x-data-plane=on" way now even crashes, it's
probably easier to just drop it:

$ qemu-system-x86_64 -drive file=null-co://,id=d0,if=none \
-device virtio-blk-pci,drive=d0,x-data-plane=on

ERROR:/home/fam/work/qemu/qom/object.c:1515:
object_get_canonical_path_component: assertion failed: (obj->parent != NULL)
Aborted

Signed-off-by: Fam Zheng 
---
 hw/block/dataplane/virtio-blk.c | 15 ++-
 hw/block/virtio-blk.c   |  1 -
 include/hw/virtio/virtio-blk.h  |  1 -
 3 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index c42ddeb..c57f293 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -45,7 +45,6 @@ struct VirtIOBlockDataPlane {
  * use it).
  */
 IOThread *iothread;
-IOThread internal_iothread_obj;
 AioContext *ctx;
 EventNotifier host_notifier;/* doorbell */
 
@@ -149,14 +148,14 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, 
VirtIOBlkConf *conf,
 
 *dataplane = NULL;
 
-if (!conf->data_plane && !conf->iothread) {
+if (!conf->iothread) {
 return;
 }
 
 /* Don't try if transport does not support notifiers. */
 if (!k->set_guest_notifiers || !k->set_host_notifier) {
 error_setg(errp,
-   "device is incompatible with x-data-plane "
+   "device is incompatible with dataplane "
"(transport does not support notifiers)");
 return;
 }
@@ -179,16 +178,6 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, 
VirtIOBlkConf *conf,
 if (conf->iothread) {
 s->iothread = conf->iothread;
 object_ref(OBJECT(s->iothread));
-} else {
-/* Create per-device IOThread if none specified.  This is for
- * x-data-plane option compatibility.  If x-data-plane is removed we
- * can drop this.
- */
-object_initialize(>internal_iothread_obj,
-  sizeof(s->internal_iothread_obj),
-  TYPE_IOTHREAD);
-user_creatable_complete(OBJECT(>internal_iothread_obj), 
_abort);
-s->iothread = >internal_iothread_obj;
 }
 s->ctx = iothread_get_aio_context(s->iothread);
 s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 756ae5c..b88b726 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -986,7 +986,6 @@ static Property virtio_blk_properties[] = {
 #endif
 DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0,
 true),
-DEFINE_PROP_BIT("x-data-plane", VirtIOBlock, conf.data_plane, 0, false),
 DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 6bf5905..ae11a63 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -37,7 +37,6 @@ struct VirtIOBlkConf
 char *serial;
 uint32_t scsi;
 uint32_t config_wce;
-uint32_t data_plane;
 uint32_t request_merging;
 };
 
-- 
2.4.3

Re: [Qemu-block] [PATCH v2 7/8] block: Move some bdrv_*_all() functions to BB

[Kevin Wolf]

Am 05.12.2015 um 00:15 hat Max Reitz geschrieben:
> On 01.12.2015 17:01, Kevin Wolf wrote:
> > Am 10.11.2015 um 04:27 hat Max Reitz geschrieben:
> >> Move bdrv_drain_all(), bdrv_commit_all(), bdrv_flush_all() and
> >> bdrv_invalidate_cache_all() to BB.
> >>
> >> The only operation left is bdrv_close_all(), which cannot be moved to
> >> the BB because it should not only close all BBs, but also all
> >> monitor-owned BDSs.
> >>
> >> Signed-off-by: Max Reitz 
> > 
> > bdrv_commit_all() and bdrv_flush_all() are relatively obvious. They are
> > meant to commit/flush changes made by a guest, so moving to the BB level
> > seems right.
> 
> OK, I wanted to just follow this comment, but since monitor_bdrv_states
> is local to blockdev.c (and I consider that correct) that would mean
> either making it global (which I wouldn't like) or keeping bdrv_states
> (which I wouldn't like either, not least because dropping bdrv_states
> allows us to drop bdrv_new_root(), which may or may not be crucial to
> the follow-up series to this one).
> 
> So, I'll be trying to defend what this patch does for now.
> 
> > I'm not so sure about bdrv_invalidate_cache_all(). Even if a BB isn't
> > attached to a BDS, we still need to invalidate the caches of any images
> > that have been opened before migration has completed, including those
> > images that are only owned by the monitor.
> 
> I consider BB-less BDSs to be in a temporary state between blockdev-add
> and binding them to a device, or between unbinding them from a device
> and blockdev-del. So I don't even know if we should allow them to be
> migrated.

If we don't want to, we should add a migration blocker while such a BDS
exists.

I don't think that would be right, though. Definitely not in the strict
way you phrased it ("binding them to a device"), but probably also not
if you interpret "device" as any kind of user, including block jobs or
NBD servers; actually, I think even the monitor would belong in this
list, but then you always have "something" attached, otherwise the
refcount becomes zero and the BDS is deleted.

If you don't include the monitor, though, you prevent entirely
reasonable use cases like opening upfront multiple images for a device
with removable media and then changing between them later on, which
leaves some BDSes unattached while another medium is inserted.

I think BDSes without a BB attached should still be first-class
citizens.

> Anyway, in my opinion, a BB-less BDS should be totally static.
> Invalidating its cache shouldn't do anything. Instead, its cache should
> be invalidated when it is detached from a BB (just like this patch adds
> a bdrv_drain() call to blk_remove_bs()).

Are you sure you remember what bdrv_invalidate_cache() is for? Its job
is dropping stale cache contents after a mere bdrv_open() when migrating
with shared storage. It is wrong to ever use this after actually using
(instead of just opening) an image.

The order is like this:

1. Start destination qemu -incoming. This opens the image and
   potentially reads in some metadata (qcow2 L1 table, etc.)
2. Source keeps writing to the image.
3. Migration completes. Source flushes the image and stops writing.
4. Destination must call bdrv_invalidate_cache() to make sure it gets
   all the updates from step 2 instead of using stale data from step 1.

Note how there is no detaching from a BB involved at all.

> > Similarly I'm concerned about bdrv_drain_all(). Anything outside the
> > block layer should certainly call blk_drain_all() because it can only be
> > interested in those images that it can see. The calls in block.c,
> > however, look like they should consider BDSes without a BB as well. (The
> > monitor, which can hold direct BDS references, may be another valid user
> > of a bdrv_drain_all that also covers BDSes without a BB.)
> 
> Similarly, in my opinion, bdrv_drain() shouldn't do anything on a
> BB-less BDS. That is why this patch adds a bdrv_drain() call to
> blk_remove_bs() (a bdrv_invalidate_cache() call is missing yet, though).
> 
> But I just remembered that block jobs don't use BBs... Did I mention
> before how wrong I think that is?
> 
> Now I'm torn between trying to make block jobs use BBs once more
> (because I really think BB-less BDSs should not have any active I/O) and
> doing some ugly things with bdrv_states and/or bdrv_monitor_states. I'll
> have to think about it.

I think we first need to allow multiple BBs per BDS, so that block jobs
can create their own BB.

And anyway, this seems to be a great start for discussing our whole BB
model in person on Friday. As I said, I think what we're currently doing
is wrong. I am concerned that introducing a model that actually makes
sense is hard to do in a compatible way, but I'm getting less and less
confident that doing it like we do now is the right conclusion from that.

> > And block.c calling blk_*() functions smells a bit fishy anyway.
> 
> I don't find it any more fishy than single-BDS 

Re: [Qemu-block] [Qemu-devel] [PATCH] virtio-blk: Drop x-data-plane option

[Cornelia Huck]

On Mon,  7 Dec 2015 18:59:27 +0800
Fam Zheng  wrote:

> The official way of enabling dataplane is through the "iothread"
> property that references an iothread object created by "-object
> iothread".  Since the old "x-data-plane=on" way now even crashes, it's
> probably easier to just drop it:
> 
> $ qemu-system-x86_64 -drive file=null-co://,id=d0,if=none \
> -device virtio-blk-pci,drive=d0,x-data-plane=on
> 
> ERROR:/home/fam/work/qemu/qom/object.c:1515:
> object_get_canonical_path_component: assertion failed: (obj->parent != NULL)
> Aborted

Do we understand yet why this crashes, btw?

> 
> Signed-off-by: Fam Zheng 
> ---
>  hw/block/dataplane/virtio-blk.c | 15 ++-
>  hw/block/virtio-blk.c   |  1 -
>  include/hw/virtio/virtio-blk.h  |  1 -
>  3 files changed, 2 insertions(+), 15 deletions(-)
> 

No general objection to removing x-data-plane; but this probably wants
a mention on the changelog as x-data-plane has been described in
various howtos etc. over the years.

Re: [Qemu-block] [Qemu-devel] [PATCH RFC for-2.6 0/3] block: Add meta dirty bitmap for migration/persistence

[John Snow]

On 12/07/2015 12:59 AM, Fam Zheng wrote:
> Vladimir,
> 
> This is what I propose to implement meta bitmap. It's implemented in the
> HBitmap level to be more efficient, and the interface slightly varies too.
> 

I missed it: What was wrong with Vladimir's approach / what are the
benefits of this approach?

> I'd like to use these operations to make dirty bitmap persistence more
> efficient too: unchanged dirty bits don't need to be flushed to disk. So I'm
> posting this as a separate series for a common base for both sides.
> 

This is a reasonable use of the meta-bitmap strategy in general.

Keep in mind Vladimir's approach to Meta bitmaps used a different
granularity such that 1 physical bit implied 1 sector needed to be
re-transmitted.

A meta-bitmap that keeps track of disk flushes may require a different
granularity than one used for migration.

> Posting as RFC as 2.6 dev phase is just starting, we can still tweak the
> interface and/or implementation to fit the need.
> 
> Fam Zheng (3):
>   HBitmap: Introduce "meta" bitmap to track bit changes
>   tests: Add test code for meta bitmap
>   block: Support meta dirty bitmap
> 
>  block.c| 46 ++-
>  block/mirror.c |  3 +-
>  blockdev.c |  3 +-
>  include/block/block.h  | 11 
>  include/qemu/hbitmap.h |  7 +
>  migration/block.c  |  2 +-
>  tests/test-hbitmap.c   | 74 
> ++
>  util/hbitmap.c | 22 +++
>  8 files changed, 164 insertions(+), 4 deletions(-)
> 

Re: [Qemu-block] [Qemu-devel] [PATCH] virtio-blk: Drop x-data-plane option

[Fam Zheng]

On Mon, 12/07 12:29, Cornelia Huck wrote:
> On Mon,  7 Dec 2015 18:59:27 +0800
> Fam Zheng  wrote:
> 
> > The official way of enabling dataplane is through the "iothread"
> > property that references an iothread object created by "-object
> > iothread".  Since the old "x-data-plane=on" way now even crashes, it's
> > probably easier to just drop it:
> > 
> > $ qemu-system-x86_64 -drive file=null-co://,id=d0,if=none \
> > -device virtio-blk-pci,drive=d0,x-data-plane=on
> > 
> > ERROR:/home/fam/work/qemu/qom/object.c:1515:
> > object_get_canonical_path_component: assertion failed: (obj->parent != NULL)
> > Aborted
> 
> Do we understand yet why this crashes, btw?

I think it's because with x-data-plane=on, virtio-blk initialize an object that
doesn't have a parent, therefore it doesn't have a valid "canonical path
component" thing, which is different from objects created with "-object" CLI.
I'm not very familiar with the QOM semantics here.

> 
> > 
> > Signed-off-by: Fam Zheng 
> > ---
> >  hw/block/dataplane/virtio-blk.c | 15 ++-
> >  hw/block/virtio-blk.c   |  1 -
> >  include/hw/virtio/virtio-blk.h  |  1 -
> >  3 files changed, 2 insertions(+), 15 deletions(-)
> > 
> 
> No general objection to removing x-data-plane; but this probably wants
> a mention on the changelog as x-data-plane has been described in
> various howtos etc. over the years.
> 

Yes, that is a good point.  I don't know if it's too rushing in removing it for
2.5 (this is just posted as one option) and we'll have to count on QOM experts
for the fix, if it is.

Fam

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.5?] qcow2: always initialize specific image info

[Max Reitz]

On 07.12.2015 19:11, Denis V. Lunev wrote:
> On 12/07/2015 08:54 PM, Eric Blake wrote:
>> On 12/07/2015 10:51 AM, Eric Blake wrote:
>>> [adding qemu-devel - ALL patches should go to qemu-devel, even if they
>>> are also going to a sub-list like qemu-block]
>>>
>>> On 12/07/2015 10:07 AM, Roman Kagan wrote:
 qcow2_get_specific_info() used to have a code path which would leave
 pointer to ImageInfoSpecificQCow2 uninitialized.

 We guess that it caused sporadic crashes on freeing an invalid pointer
 in response to "query-block" QMP command in
 visit_type_ImageInfoSpecificQCow2 with QapiDeallocVisitor.

 Although we have neither a solid proof nor a reproduction scenario,
 making sure the field is initialized appears a reasonable thing to do.

 Signed-off-by: Roman Kagan 
 ---
   block/qcow2.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
>> Oops; hit send too soon.  I added for-2.5? to the subject line,
>> because...
>>
 diff --git a/block/qcow2.c b/block/qcow2.c
 index 88f56c8..67c9d3d 100644
 --- a/block/qcow2.c
 +++ b/block/qcow2.c
 @@ -2739,7 +2739,7 @@ static ImageInfoSpecific
 *qcow2_get_specific_info(BlockDriverState *bs)
 *spec_info = (ImageInfoSpecific){
   .type  = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
 -.u.qcow2 = g_new(ImageInfoSpecificQCow2, 1),
 +.u.qcow2 = g_new0(ImageInfoSpecificQCow2, 1),
>>> NACK.  This makes no difference, except when s->qcow_version is out
>>> of spec.
>>>
   };
   if (s->qcow_version == 2) {
   *spec_info->u.qcow2 = (ImageInfoSpecificQCow2){

>>> If s->qcow_version is exactly 2, then we end up initializing all fields
>>> due to the assignment here; same if qcow_version is exactly 3.  The only
>>> time qcow2 remains uninitialized is if qcow_version is 0, 1, or > 3; but
>>> we refuse to handle qcow files with out-of-range versions.  So I don't
>>> see how you are plugging any uninitialized values; and therefore, I
>>> don't see how this is patching any crashes.
>> ...if you can prove that we aren't gracefully handling an out-of-spec
>> qcow_version, such that the uninitialized memory in that scenario is
>> indeed causing a crash, then it is worth respinning a v2 of this patch
>> with that proof, and worth considering it for 2.5 if it really is a
>> crash fixer.

More or less unfortunately, Eric is right. I chose a g_new() over
g_new0() because I was using compound literals anyway (and members not
explicitly initialized in the initializer list given for a compound
literal are initialized implicitly like object with static storage
duration, i.e. 0, generally).

s->qcow_version is always set to 2 or 3. It is only set in:
(1) qcow2_open(): Directly before it is set, we check that the version
is either 2 or 3.
(2) qcow2_downgrade(): We make sure that target_version (the value
s->qcow_version is set to) is equal to 2.
(3) qcow2_downgrade(): On error, s->qcow_version may be reset to
current_version, which is the old value of s->qcow_version which we
can inductively assume to be either 2 or 3.
(4) qcow2_amend_options(): On version upgrade, s->qcow_version is
overwritten by new_version, which is either 2, 3, or the old value
of s->qcow_version (in practice it is always 3 because it needs to
be greater than s->qcow_version in order to get here).
(5) qcow2_amend_options(): On version upgrade error, s->qcow_version is
reset to the old value of s->qcow_version, which we can again assume
to be 2 or 3.

I'd be completely fine with adding an "else { abort(); }" branch to
qcow2_get_specific_info(). But I fear that the issue you encountered is
caused by something different than the ImageInfoSpecificQCow2 object not
being fully initialized, and therefore I'm against this patch even if it
should not change anything (because it might make as feel as if we found
the issue even though there (most probably) is none here).

Also...

> Here is an info about our crash.
> 
> we have this crash under unknown conditions on RHEV version of QEMU.
> 
> Sorry, there is no much additional info. For the 

[...]

> which looks like
> 
> More specifically (expanding inlines along the stack trace):
> 
> (gdb) l *(visit_type_ImageInfoSpecificQCow2+169)
> 0x1a71e9 is in visit_type_ImageInfoSpecificQCow2 (qapi-visit.c:552).
> 548 static void visit_type_ImageInfoSpecificQCow2_fields(Visitor *m,
> ImageInfoSpecificQCow2 **obj, Error **errp)
> 549 {
> 550 Error *err = NULL;
> 551 ==> visit_type_str(m, &(*obj)->compat, "compat", );

...the compat field is always set, in either code path (both for version
2 and 3). Therefore, if this pointer is wrong, it definitely is not due
to the field not being initialized.

Without any way of reproducing, it is difficult to find the true cause
of the issue, though. valgrind would probably tell us something, but for
that we'd need something it 

[Qemu-block] [PATCH] qemu-img / curl: When fetching Content-Size use GET instead of HEAD.

[Boris Schrijver]

Hi all,

I was testing out the "qemu-img info/convert" options in combination with
"http/https" when I stumbled upon this issue. When "qemu-img info/convert" tries
to collect the file info it will first try to fetch the Content-Size of the
remote file. It does a HEAD request and after a GET request for the correct
range.

The HEAD request is an issue. Because when you've got a pre-signed url, for
example from S3, which INCLUDES the REQUEST METHOD in it's signature, you'll get
a 403 Forbidden.

It's is therefore better to use only the GET request method, and discard the
body at the first call.

Please review! I'll be ready for answers!

[PATCH] qemu-img / curl: When fetching Content-Size use GET instead of HEAD.

A server can respond different to both methods, or can block one of the two.
---
 block/curl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 8994182..2e74c32 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -594,11 +594,11 @@ static int curl_open(BlockDriverState *bs, QDict *options,
int flags,
 // Get file size
 
 s->accept_range = false;
-curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1);
+curl_easy_setopt(state->curl, CURLOPT_HTTPGET, 1);
 curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION,
  curl_header_cb);
 curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s);
-if (curl_easy_perform(state->curl))
+if (curl_easy_perform(state->curl) != 23)
 goto out;
 curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, );
 if (d)
-- 
2.1.4

-- 

Met vriendelijke groet / Kind regards,

Boris Schrijver

PCextreme B.V.

http://www.pcextreme.nl/contact
Tel direct: +31 (0) 118 700 215

[Qemu-block] [PATCH for-2.6 v2 03/10] fdc: add disk field

[John Snow]

This allows us to distinguish between the current disk type and the
current drive type. The drive is what's reported to CMOS, the disk is
whatever the pick_geometry function suspects has been inserted.

The drive field maintains the exact same meaning as it did previously,
however pick_geometry/fd_revalidate will be refactored to *never* update
the drive field, considering it frozen in-place during an earlier
initialization call.

Before this patch, pick_geometry/fd_revalidate could only change the
drive field when it was FDRIVE_DRV_NONE, which indicated that we had
not yet reported our drive type to CMOS. After we "pick one," even
though pick_geometry/fd_revalidate re-set drv->drive, it should always
be the same as the value going into these calls, so it is effectively
already static.

As of this patch, disk and drive will always be the same, but that may
not be true by the end of this series.

Disk does not need to be migrated because it is not user-visible state
nor is it currently used for any calculations. It is purely informative,
and will be rebuilt automatically via fd_revalidate on the new host.

Signed-off-by: John Snow 
---
 hw/block/fdc.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 09bb63d..13fef23 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -133,7 +133,8 @@ typedef struct FDrive {
 FDCtrl *fdctrl;
 BlockBackend *blk;
 /* Drive status */
-FDriveType drive;
+FDriveType drive; /* CMOS drive type */
+FDriveType disk;  /* Current disk type */
 uint8_t perpendicular;/* 2.88 MB access mode*/
 /* Position */
 uint8_t head;
@@ -157,6 +158,7 @@ static void fd_init(FDrive *drv)
 drv->drive = FDRIVE_DRV_NONE;
 drv->perpendicular = 0;
 /* Disk */
+drv->disk = FDRIVE_DRV_NONE;
 drv->last_sect = 0;
 drv->max_track = 0;
 }
@@ -286,6 +288,7 @@ static void pick_geometry(FDrive *drv)
 drv->max_track = parse->max_track;
 drv->last_sect = parse->last_sect;
 drv->drive = parse->drive;
+drv->disk = drv->media_inserted ? parse->drive : FDRIVE_DRV_NONE;
 drv->media_rate = parse->rate;
 
 if (drv->media_inserted) {
-- 
2.4.3

[Qemu-block] [PATCH for-2.6 v2 08/10] fdc: rework pick_geometry

[John Snow]

This one is the crazy one.

fd_revalidate currently uses pick_geometry to tell if the diskette
geometry has changed upon an eject/insert event, but it won't allow us
to insert a 1.44MB diskette into a 2.88MB drive. This is inflexible.

The new algorithm applies a new heuristic to guessing disk geometries
that allows us to switch diskette types as long as the physical size
matches before falling back to the old heuristic.

The old one is roughly:
 - If the size and type matches, choose it.
 - Fall back to the first geometry that matched our type.

The new one is:
 - If the size and type matches, choose it.
 - If the size (sectors) and physical size match, choose it.
 - If the size (sectors) matches at all, choose it begrudgingly.
 - Fall back to the first geometry that matched our type.

Signed-off-by: John Snow 
---
 hw/block/fdc.c | 63 +-
 1 file changed, 40 insertions(+), 23 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 12a2595..246bd83 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -132,7 +132,6 @@ static const FDFormat fd_formats[] = {
 { FLOPPY_DRIVE_TYPE_NONE, -1, -1, 0, 0, },
 };
 
-__attribute__((__unused__))
 static FDriveSize drive_size(FloppyDriveType drive)
 {
 switch (drive) {
@@ -287,45 +286,63 @@ static bool pick_geometry(FDrive *drv)
 BlockBackend *blk = drv->blk;
 const FDFormat *parse;
 uint64_t nb_sectors, size;
-int i, first_match, match;
+int i;
+int match, size_match, type_match;
+bool magic = drv->drive == FLOPPY_DRIVE_TYPE_AUTO;
 
 /* We can only pick a geometry if we have a diskette. */
 if (!drv->media_inserted) {
 return false;
 }
 
+/* We need to determine the likely geometry of the inserted medium.
+ * In order of preference, we look for:
+ * (1) The same drive type and number of sectors,
+ * (2) The same diskette size and number of sectors,
+ * (3) The same number of sectors,
+ * (4) The same drive type.
+ *
+ * In all cases, matches that occur higher in the drive table will take
+ * precedence over matches that occur later in the table.
+ */
 blk_get_geometry(blk, _sectors);
-match = -1;
-first_match = -1;
+match = size_match = type_match = -1;
 for (i = 0; ; i++) {
 parse = _formats[i];
 if (parse->drive == FLOPPY_DRIVE_TYPE_NONE) {
 break;
 }
-if (drv->drive == parse->drive ||
-drv->drive == FLOPPY_DRIVE_TYPE_AUTO) {
-size = (parse->max_head + 1) * parse->max_track *
-parse->last_sect;
-if (nb_sectors == size) {
-match = i;
-break;
-}
-if (first_match == -1) {
-first_match = i;
+size = (parse->max_head + 1) * parse->max_track * parse->last_sect;
+if (nb_sectors == size) {
+if (magic || parse->drive == drv->drive) {
+/* (1) perfect match */
+goto out;
+} else if (drive_size(parse->drive) == drive_size(drv->drive)) {
+/* (2) physical size match */
+match = (match == -1) ? i : match;
+} else {
+/* (3) nsectors match only */
+size_match = (size_match == -1) ? i : size_match;
 }
+} else if ((type_match == -1) &&
+   ((parse->drive == drv->drive) ||
+(magic && (parse->drive == FDRIVE_AUTO_FALLBACK {
+/* (4) type matches, or type matches the autodetect default if we
+ * are using the autodetect mechanism. */
+type_match = i;
 }
 }
+
 if (match == -1) {
-if (first_match == -1) {
-/* No entry found: drive_type was NONE or we neglected to add any
- * candidate geometries for our drive type into the fd_formats 
table
- */
-match = ARRAY_SIZE(fd_formats) - 1;
-} else {
-match = first_match;
-}
-parse = _formats[match];
+match = (size_match != -1) ? size_match : type_match;
+}
+
+if (match == -1) {
+/* No entry found: drive_type was NONE or we neglected to add any
+ * candidate geometries for our drive type into the fd_formats table. 
*/
+match = ARRAY_SIZE(fd_formats) - 1;
 }
+parse = &(fd_formats[match]);
 
  out:
 if (parse->max_head == 0) {
-- 
2.4.3

[Qemu-block] [PATCH for-2.6 v2 06/10] fdc: implement new drive type property

[John Snow]

Respect the drive type as given via the CLI.

Set the type given by the CLI during fd_init. If the type remains the
default (auto), we'll attempt to scan an inserted diskette if present
to determine a type. If auto is selected but no diskette is present,
we fall back to a predetermined default (currently 1.44MB to match
legacy QEMU behavior.)

The pick_geometry algorithm is modified to only allow matches outside
of the existing drive type for the new auto behavior. If a user specifies
the "none" type, QEMU will not report this drive to the CMOS.

Signed-off-by: John Snow 
---
 hw/block/fdc.c | 45 -
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 39e680b..9bb3021 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -161,10 +161,12 @@ typedef struct FDrive {
 bool media_inserted;  /* Is there a medium in the tray */
 } FDrive;
 
+static FloppyDriveType get_default_drive_type(FDrive *drv);
+
 static void fd_init(FDrive *drv)
 {
 /* Drive */
-drv->drive = FLOPPY_DRIVE_TYPE_NONE;
+drv->drive = get_default_drive_type(drv);
 drv->perpendicular = 0;
 /* Disk */
 drv->disk = FLOPPY_DRIVE_TYPE_NONE;
@@ -277,7 +279,7 @@ static bool pick_geometry(FDrive *drv)
 break;
 }
 if (drv->drive == parse->drive ||
-drv->drive == FLOPPY_DRIVE_TYPE_NONE) {
+drv->drive == FLOPPY_DRIVE_TYPE_AUTO) {
 size = (parse->max_head + 1) * parse->max_track *
 parse->last_sect;
 if (nb_sectors == size) {
@@ -291,13 +293,17 @@ static bool pick_geometry(FDrive *drv)
 }
 if (match == -1) {
 if (first_match == -1) {
-match = 1;
+/* No entry found: drive_type was NONE or we neglected to add any
+ * candidate geometries for our drive type into the fd_formats 
table
+ */
+match = ARRAY_SIZE(fd_formats) - 1;
 } else {
 match = first_match;
 }
 parse = _formats[match];
 }
 
+ out:
 if (parse->max_head == 0) {
 drv->flags &= ~FDISK_DBL_SIDES;
 } else {
@@ -319,8 +325,16 @@ static bool pick_geometry(FDrive *drv)
 
 static void pick_drive_type(FDrive *drv)
 {
-if (pick_geometry(drv)) {
-drv->drive = drv->disk;
+if (drv->drive != FLOPPY_DRIVE_TYPE_AUTO) {
+return;
+}
+
+if (!drv->media_inserted) {
+drv->drive = FDRIVE_AUTO_FALLBACK;
+} else {
+if (pick_geometry(drv)) {
+drv->drive = drv->disk;
+}
 }
 }
 
@@ -623,6 +637,27 @@ typedef struct FDCtrlISABus {
 int32_t bootindexB;
 } FDCtrlISABus;
 
+static FloppyDriveType get_default_drive_type(FDrive *drv)
+{
+FDCtrl *fdctrl = drv->fdctrl;
+
+g_assert_cmpint(MAX_FD, ==, 2);
+
+if (!drv->blk) {
+return FLOPPY_DRIVE_TYPE_NONE;
+}
+
+if (drv == >drives[0]) {
+return fdctrl->typeA;
+}
+
+if (drv == >drives[1]) {
+return fdctrl->typeB;
+}
+
+return FDRIVE_DEFAULT;
+}
+
 static uint32_t fdctrl_read (void *opaque, uint32_t reg)
 {
 FDCtrl *fdctrl = opaque;
-- 
2.4.3

[Qemu-block] [PATCH for-2.6 v2 05/10] fdc: do not call revalidate on eject

[John Snow]

Currently, fd_revalidate is called in two different places, with two
different expectations of behavior:

(1) On initialization, as a routine to help pick the drive type and
initial geometries as a side-effect of the pick_geometry routine

(2) On insert/eject, which either sets the geometries to a default value
or chooses new geometries based on the inserted diskette.

Break this nonsense apart by creating a new function dedicated towards
picking the drive type on initialization.

This has a few results:

(1) fd_revalidate does not get called on boot anymore for drives with no
diskette.

(2) pick_geometry will actually get called twice if we have a diskette
inserted, but this is harmless. (Once for the drive type, and once
as part of the media callback.)

Signed-off-by: John Snow 
---
 hw/block/fdc.c | 36 +---
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 498eb9c..39e680b 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -170,6 +170,7 @@ static void fd_init(FDrive *drv)
 drv->disk = FLOPPY_DRIVE_TYPE_NONE;
 drv->last_sect = 0;
 drv->max_track = 0;
+drv->ro = true;
 }
 
 #define NUM_SIDES(drv) ((drv)->flags & FDISK_DBL_SIDES ? 2 : 1)
@@ -252,13 +253,21 @@ static void fd_recalibrate(FDrive *drv)
 fd_seek(drv, 0, 0, 1, 1);
 }
 
-static void pick_geometry(FDrive *drv)
+/**
+ * Determine geometry based on inserted diskette.
+ */
+static bool pick_geometry(FDrive *drv)
 {
 BlockBackend *blk = drv->blk;
 const FDFormat *parse;
 uint64_t nb_sectors, size;
 int i, first_match, match;
 
+/* We can only pick a geometry if we have a diskette. */
+if (!drv->media_inserted) {
+return false;
+}
+
 blk_get_geometry(blk, _sectors);
 match = -1;
 first_match = -1;
@@ -296,8 +305,7 @@ static void pick_geometry(FDrive *drv)
 }
 drv->max_track = parse->max_track;
 drv->last_sect = parse->last_sect;
-drv->drive = parse->drive;
-drv->disk = drv->media_inserted ? parse->drive : FLOPPY_DRIVE_TYPE_NONE;
+drv->disk = parse->drive;
 drv->media_rate = parse->rate;
 
 if (drv->media_inserted) {
@@ -306,6 +314,14 @@ static void pick_geometry(FDrive *drv)
drv->max_track, drv->last_sect,
drv->ro ? "ro" : "rw");
 }
+return true;
+}
+
+static void pick_drive_type(FDrive *drv)
+{
+if (pick_geometry(drv)) {
+drv->drive = drv->disk;
+}
 }
 
 /* Revalidate a disk drive after a disk change */
@@ -314,15 +330,18 @@ static void fd_revalidate(FDrive *drv)
 FLOPPY_DPRINTF("revalidate\n");
 if (drv->blk != NULL) {
 drv->ro = blk_is_read_only(drv->blk);
-pick_geometry(drv);
 if (!drv->media_inserted) {
 FLOPPY_DPRINTF("No disk in drive\n");
+drv->disk = FLOPPY_DRIVE_TYPE_NONE;
+} else {
+pick_geometry(drv);
 }
 } else {
 FLOPPY_DPRINTF("No drive connected\n");
 drv->last_sect = 0;
 drv->max_track = 0;
 drv->flags &= ~FDISK_DBL_SIDES;
+drv->disk = FLOPPY_DRIVE_TYPE_NONE;
 }
 }
 
@@ -2194,9 +2213,11 @@ static void fdctrl_change_cb(void *opaque, bool load)
 FDrive *drive = opaque;
 
 drive->media_inserted = load && drive->blk && blk_is_inserted(drive->blk);
-
 drive->media_changed = 1;
-fd_revalidate(drive);
+
+if (load) {
+fd_revalidate(drive);
+}
 }
 
 static bool fdctrl_is_tray_open(void *opaque)
@@ -2232,11 +2253,12 @@ static void fdctrl_connect_drives(FDCtrl *fdctrl, Error 
**errp)
 }
 
 fd_init(drive);
-fdctrl_change_cb(drive, 0);
 if (drive->blk) {
 blk_set_dev_ops(drive->blk, _block_ops, drive);
 drive->media_inserted = blk_is_inserted(drive->blk);
+pick_drive_type(drive);
 }
+fdctrl_change_cb(drive, drive->media_inserted);
 }
 }
 
-- 
2.4.3

[Qemu-block] [PATCH for-2.6 v2 07/10] fdc: add physical disk sizes

[John Snow]

2.88MB capable drives can accept 1.44MB floppies,
for instance. To rework the pick_geometry function,
we need to know if our current drive can even accept
the type of disks we're considering.

NB: This allows us to distinguish between all of the
"total sectors" collisions between 1.20MB and 1.44MB
diskette types, by using the physical drive size as a
differentiator.

Signed-off-by: John Snow 
---
 hw/block/fdc.c | 40 
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 9bb3021..12a2595 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -59,6 +59,12 @@ typedef enum FDriveRate {
 FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
 } FDriveRate;
 
+typedef enum FDriveSize {
+FDRIVE_SIZE_UNKNOWN,
+FDRIVE_SIZE_350,
+FDRIVE_SIZE_525,
+} FDriveSize;
+
 typedef struct FDFormat {
 FloppyDriveType drive;
 uint8_t last_sect;
@@ -75,11 +81,15 @@ typedef struct FDFormat {
 #define FDRIVE_DEFAULT FLOPPY_DRIVE_TYPE_AUTO
 #define FDRIVE_AUTO_FALLBACK FLOPPY_DRIVE_TYPE_144
 
+/* In many cases, the total sector size of a format is enough to uniquely
+ * identify it. However, there are some total sector collisions between
+ * formats of different physical size, and these are noted below by
+ * highlighting the total sector size for entries with collisions. */
 static const FDFormat fd_formats[] = {
 /* First entry is default format */
 /* 1.44 MB 3"1/2 floppy disks */
-{ FLOPPY_DRIVE_TYPE_144, 18, 80, 1, FDRIVE_RATE_500K, },
-{ FLOPPY_DRIVE_TYPE_144, 20, 80, 1, FDRIVE_RATE_500K, },
+{ FLOPPY_DRIVE_TYPE_144, 18, 80, 1, FDRIVE_RATE_500K, }, /* 3.5" 2880 */
+{ FLOPPY_DRIVE_TYPE_144, 20, 80, 1, FDRIVE_RATE_500K, }, /* 3.5" 3200 */
 { FLOPPY_DRIVE_TYPE_144, 21, 80, 1, FDRIVE_RATE_500K, },
 { FLOPPY_DRIVE_TYPE_144, 21, 82, 1, FDRIVE_RATE_500K, },
 { FLOPPY_DRIVE_TYPE_144, 21, 83, 1, FDRIVE_RATE_500K, },
@@ -93,7 +103,7 @@ static const FDFormat fd_formats[] = {
 { FLOPPY_DRIVE_TYPE_288, 44, 80, 1, FDRIVE_RATE_1M, },
 { FLOPPY_DRIVE_TYPE_288, 48, 80, 1, FDRIVE_RATE_1M, },
 /* 720 kB 3"1/2 floppy disks */
-{ FLOPPY_DRIVE_TYPE_144,  9, 80, 1, FDRIVE_RATE_250K, },
+{ FLOPPY_DRIVE_TYPE_144,  9, 80, 1, FDRIVE_RATE_250K, }, /* 3.5" 1400 */
 { FLOPPY_DRIVE_TYPE_144, 10, 80, 1, FDRIVE_RATE_250K, },
 { FLOPPY_DRIVE_TYPE_144, 10, 82, 1, FDRIVE_RATE_250K, },
 { FLOPPY_DRIVE_TYPE_144, 10, 83, 1, FDRIVE_RATE_250K, },
@@ -101,15 +111,15 @@ static const FDFormat fd_formats[] = {
 { FLOPPY_DRIVE_TYPE_144, 14, 80, 1, FDRIVE_RATE_250K, },
 /* 1.2 MB 5"1/4 floppy disks */
 { FLOPPY_DRIVE_TYPE_120, 15, 80, 1, FDRIVE_RATE_500K, },
-{ FLOPPY_DRIVE_TYPE_120, 18, 80, 1, FDRIVE_RATE_500K, },
+{ FLOPPY_DRIVE_TYPE_120, 18, 80, 1, FDRIVE_RATE_500K, }, /* 5.25" 2880 */
 { FLOPPY_DRIVE_TYPE_120, 18, 82, 1, FDRIVE_RATE_500K, },
 { FLOPPY_DRIVE_TYPE_120, 18, 83, 1, FDRIVE_RATE_500K, },
-{ FLOPPY_DRIVE_TYPE_120, 20, 80, 1, FDRIVE_RATE_500K, },
+{ FLOPPY_DRIVE_TYPE_120, 20, 80, 1, FDRIVE_RATE_500K, }, /* 5.25" 3200 */
 /* 720 kB 5"1/4 floppy disks */
-{ FLOPPY_DRIVE_TYPE_120,  9, 80, 1, FDRIVE_RATE_250K, },
+{ FLOPPY_DRIVE_TYPE_120,  9, 80, 1, FDRIVE_RATE_250K, }, /* 5.25" 1440 */
 { FLOPPY_DRIVE_TYPE_120, 11, 80, 1, FDRIVE_RATE_250K, },
 /* 360 kB 5"1/4 floppy disks */
-{ FLOPPY_DRIVE_TYPE_120,  9, 40, 1, FDRIVE_RATE_300K, },
+{ FLOPPY_DRIVE_TYPE_120,  9, 40, 1, FDRIVE_RATE_300K, }, /* 5.25" 720 */
 { FLOPPY_DRIVE_TYPE_120,  9, 40, 0, FDRIVE_RATE_300K, },
 { FLOPPY_DRIVE_TYPE_120, 10, 41, 1, FDRIVE_RATE_300K, },
 { FLOPPY_DRIVE_TYPE_120, 10, 42, 1, FDRIVE_RATE_300K, },
@@ -117,11 +127,25 @@ static const FDFormat fd_formats[] = {
 { FLOPPY_DRIVE_TYPE_120,  8, 40, 1, FDRIVE_RATE_250K, },
 { FLOPPY_DRIVE_TYPE_120,  8, 40, 0, FDRIVE_RATE_250K, },
 /* 360 kB must match 5"1/4 better than 3"1/2... */
-{ FLOPPY_DRIVE_TYPE_144,  9, 80, 0, FDRIVE_RATE_250K, },
+{ FLOPPY_DRIVE_TYPE_144,  9, 80, 0, FDRIVE_RATE_250K, }, /* 3.5" 720 */
 /* end */
 { FLOPPY_DRIVE_TYPE_NONE, -1, -1, 0, 0, },
 };
 
+__attribute__((__unused__))
+static FDriveSize drive_size(FloppyDriveType drive)
+{
+switch (drive) {
+case FLOPPY_DRIVE_TYPE_120:
+return FDRIVE_SIZE_525;
+case FLOPPY_DRIVE_TYPE_144:
+case FLOPPY_DRIVE_TYPE_288:
+return FDRIVE_SIZE_350;
+default:
+return FDRIVE_SIZE_UNKNOWN;
+}
+}
+
 #define GET_CUR_DRV(fdctrl) ((fdctrl)->cur_drv)
 #define SET_CUR_DRV(fdctrl, drive) ((fdctrl)->cur_drv = (drive))
 
-- 
2.4.3

[Qemu-block] [PATCH for-2.6 v2 00/10] fdc: fix 2.88mb floppy diskette support

[John Snow]

Yes, it's been broken for ten years.
No, it's not a CVE.

The problem is that QEMU doesn't have a configuration option for the type
of floppy drive you want. It determines that based on the type of
diskette inserted at boot time.

If you don't insert one, it always chooses a 1.44MB type.

If you want to insert a 2.88MB floppy after boot, you simply cannot.

"Wow, who cares?"

Good question -- Unfortunately, the virtio-win floppy disk images that
Red Hat/fedora ship require a 2.88MB drive, so if you forgot to insert
them at boot, you'd have to change your VM configuration and try again.

For a one-shot operation, that's kind of obnoxious -- it'd be nice to
allow one to just insert the diskette on-demand.

"OK, What are you changing in this decades-old device?"

(1) Add a new property to allow users to specify what kind of drive they
want without relying on magical guessing behavior.
Choices are: 120, 144, 288, auto, and none.

120, 144 and 288 refer to 1.20MB, 1.44MB, and 2.88MB drives.
auto refers to the auto-detect behavior QEMU currently has.
none ... hides the drive. You probably don't want to use this.

(2) Add the concept of physical diskette size to QEMU, classifying
120-style diskettes as fundamentally different from 144 and 288 ones.

(3) Revamp the automatic guessing heuristic to understand that
2.88MB style drives can accept 1.44MB diskettes.

(4) Change the automatic fallback type for the automatic guessing
heuristic from 1.44MB to 2.88MB as it is a more diverse drive.

(5) A lot of code cleanup in general.

"Won't this break everything, you madman?"

No: I tested this in MS-DOS 6.22, Fedora 23 and Windows 8.1. All
seemed perfectly happy with 2.88MB drives as the default for 1.44
or 2.88MB floppy diskette images.

If any guests are discovered to be unable to cope with this default,
they are free to choose a 1.44MB drive type at boot, or insert an
appropriate diskette. By and large, this appears to improve the
diskette compatibility for most guests.



For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch fdc-default
https://github.com/jnsnow/qemu/tree/fdc-default

This version is tagged fdc-default-v2:
https://github.com/jnsnow/qemu/releases/tag/fdc-default-v2

John Snow (10):
  fdc: move pick_geometry
  fdc: refactor pick_geometry
  fdc: add disk field
  fdc: add default drive type option
  fdc: do not call revalidate on eject
  fdc: implement new drive type property
  fdc: add physical disk sizes
  fdc: rework pick_geometry
  qtest/fdc: Support for 2.88MB drives
  fdc: change auto fallback drive to 288

 hw/block/fdc.c   | 317 +--
 hw/core/qdev-properties.c|  11 ++
 hw/i386/pc.c |  17 +--
 include/hw/block/fdc.h   |   9 +-
 include/hw/qdev-properties.h |   1 +
 qapi/block.json  |  16 +++
 tests/fdc-test.c |   2 +-
 7 files changed, 255 insertions(+), 118 deletions(-)

-- 
2.4.3

[Qemu-block] [PATCH for-2.6 v2 02/10] fdc: refactor pick_geometry

[John Snow]

Modify this function to operate directly on FDrive objects instead of
unpacking and passing all of those parameters manually.

Helps reduce complexity in each caller, and reduces the number of args.

Signed-off-by: John Snow 
---
 hw/block/fdc.c | 54 +++---
 1 file changed, 23 insertions(+), 31 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 246b631..09bb63d 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -241,11 +241,9 @@ static void fd_recalibrate(FDrive *drv)
 fd_seek(drv, 0, 0, 1, 1);
 }
 
-static void pick_geometry(BlockBackend *blk, int *nb_heads,
-  int *max_track, int *last_sect,
-  FDriveType drive_in, FDriveType *drive,
-  FDriveRate *rate)
+static void pick_geometry(FDrive *drv)
 {
+BlockBackend *blk = drv->blk;
 const FDFormat *parse;
 uint64_t nb_sectors, size;
 int i, first_match, match;
@@ -258,8 +256,8 @@ static void pick_geometry(BlockBackend *blk, int *nb_heads,
 if (parse->drive == FDRIVE_DRV_NONE) {
 break;
 }
-if (drive_in == parse->drive ||
-drive_in == FDRIVE_DRV_NONE) {
+if (drv->drive == parse->drive ||
+drv->drive == FDRIVE_DRV_NONE) {
 size = (parse->max_head + 1) * parse->max_track *
 parse->last_sect;
 if (nb_sectors == size) {
@@ -279,41 +277,35 @@ static void pick_geometry(BlockBackend *blk, int 
*nb_heads,
 }
 parse = _formats[match];
 }
-*nb_heads = parse->max_head + 1;
-*max_track = parse->max_track;
-*last_sect = parse->last_sect;
-*drive = parse->drive;
-*rate = parse->rate;
+
+if (parse->max_head == 0) {
+drv->flags &= ~FDISK_DBL_SIDES;
+} else {
+drv->flags |= FDISK_DBL_SIDES;
+}
+drv->max_track = parse->max_track;
+drv->last_sect = parse->last_sect;
+drv->drive = parse->drive;
+drv->media_rate = parse->rate;
+
+if (drv->media_inserted) {
+FLOPPY_DPRINTF("Floppy disk (%d h %d t %d s) %s\n",
+   parse->max_head + 1,
+   drv->max_track, drv->last_sect,
+   drv->ro ? "ro" : "rw");
+}
 }
 
 /* Revalidate a disk drive after a disk change */
 static void fd_revalidate(FDrive *drv)
 {
-int nb_heads, max_track, last_sect, ro;
-FDriveType drive;
-FDriveRate rate;
-
 FLOPPY_DPRINTF("revalidate\n");
 if (drv->blk != NULL) {
-ro = blk_is_read_only(drv->blk);
-pick_geometry(drv->blk, _heads, _track,
-  _sect, drv->drive, , );
+drv->ro = blk_is_read_only(drv->blk);
+pick_geometry(drv);
 if (!drv->media_inserted) {
 FLOPPY_DPRINTF("No disk in drive\n");
-} else {
-FLOPPY_DPRINTF("Floppy disk (%d h %d t %d s) %s\n", nb_heads,
-   max_track, last_sect, ro ? "ro" : "rw");
 }
-if (nb_heads == 1) {
-drv->flags &= ~FDISK_DBL_SIDES;
-} else {
-drv->flags |= FDISK_DBL_SIDES;
-}
-drv->max_track = max_track;
-drv->last_sect = last_sect;
-drv->ro = ro;
-drv->drive = drive;
-drv->media_rate = rate;
 } else {
 FLOPPY_DPRINTF("No drive connected\n");
 drv->last_sect = 0;
-- 
2.4.3

[Qemu-block] [PATCH for-2.6 v2 01/10] fdc: move pick_geometry

[John Snow]

Code motion: I want to refactor this function to work with FDrive
directly, so shuffle it below that definition.

Signed-off-by: John Snow 
---
 hw/block/fdc.c | 90 +-
 1 file changed, 45 insertions(+), 45 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 4292ece..246b631 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -114,51 +114,6 @@ static const FDFormat fd_formats[] = {
 { FDRIVE_DRV_NONE, -1, -1, 0, 0, },
 };
 
-static void pick_geometry(BlockBackend *blk, int *nb_heads,
-  int *max_track, int *last_sect,
-  FDriveType drive_in, FDriveType *drive,
-  FDriveRate *rate)
-{
-const FDFormat *parse;
-uint64_t nb_sectors, size;
-int i, first_match, match;
-
-blk_get_geometry(blk, _sectors);
-match = -1;
-first_match = -1;
-for (i = 0; ; i++) {
-parse = _formats[i];
-if (parse->drive == FDRIVE_DRV_NONE) {
-break;
-}
-if (drive_in == parse->drive ||
-drive_in == FDRIVE_DRV_NONE) {
-size = (parse->max_head + 1) * parse->max_track *
-parse->last_sect;
-if (nb_sectors == size) {
-match = i;
-break;
-}
-if (first_match == -1) {
-first_match = i;
-}
-}
-}
-if (match == -1) {
-if (first_match == -1) {
-match = 1;
-} else {
-match = first_match;
-}
-parse = _formats[match];
-}
-*nb_heads = parse->max_head + 1;
-*max_track = parse->max_track;
-*last_sect = parse->last_sect;
-*drive = parse->drive;
-*rate = parse->rate;
-}
-
 #define GET_CUR_DRV(fdctrl) ((fdctrl)->cur_drv)
 #define SET_CUR_DRV(fdctrl, drive) ((fdctrl)->cur_drv = (drive))
 
@@ -286,6 +241,51 @@ static void fd_recalibrate(FDrive *drv)
 fd_seek(drv, 0, 0, 1, 1);
 }
 
+static void pick_geometry(BlockBackend *blk, int *nb_heads,
+  int *max_track, int *last_sect,
+  FDriveType drive_in, FDriveType *drive,
+  FDriveRate *rate)
+{
+const FDFormat *parse;
+uint64_t nb_sectors, size;
+int i, first_match, match;
+
+blk_get_geometry(blk, _sectors);
+match = -1;
+first_match = -1;
+for (i = 0; ; i++) {
+parse = _formats[i];
+if (parse->drive == FDRIVE_DRV_NONE) {
+break;
+}
+if (drive_in == parse->drive ||
+drive_in == FDRIVE_DRV_NONE) {
+size = (parse->max_head + 1) * parse->max_track *
+parse->last_sect;
+if (nb_sectors == size) {
+match = i;
+break;
+}
+if (first_match == -1) {
+first_match = i;
+}
+}
+}
+if (match == -1) {
+if (first_match == -1) {
+match = 1;
+} else {
+match = first_match;
+}
+parse = _formats[match];
+}
+*nb_heads = parse->max_head + 1;
+*max_track = parse->max_track;
+*last_sect = parse->last_sect;
+*drive = parse->drive;
+*rate = parse->rate;
+}
+
 /* Revalidate a disk drive after a disk change */
 static void fd_revalidate(FDrive *drv)
 {
-- 
2.4.3

[Qemu-block] [PATCH for-2.6 v2 04/10] fdc: add default drive type option

[John Snow]

This patch adds a new explicit Floppy Drive Type option. The existing
behavior in QEMU is to automatically guess a drive type based on the
media inserted, or if a diskette is not present, arbitrarily assign one.

This behavior can be described as "auto." This patch adds explicit
behaviors: 120, 144, 288, auto, and none. The new "auto" behavior
is intended to mimick current behavior, while the other types pick
one explicitly.

In a future patch, the goal is to change the FDC's default drive type
from auto (falling back to 1.44MB) to auto (falling back to 2.88MB).

In order to allow users to obtain the old behaviors, though, a mechanism
for specifying the exact type of drive we want is needed.

This patch adds the properties, but it is not acted on yet in favor of
making those changes a little more explicitly clear in standalone patches
later in this patch set.

Signed-off-by: John Snow 
---
 hw/block/fdc.c   | 108 ++-
 hw/core/qdev-properties.c|  11 +
 hw/i386/pc.c |  17 +++
 include/hw/block/fdc.h   |   9 +---
 include/hw/qdev-properties.h |   1 +
 qapi/block.json  |  16 +++
 6 files changed, 103 insertions(+), 59 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 13fef23..498eb9c 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -60,58 +60,66 @@ typedef enum FDriveRate {
 } FDriveRate;
 
 typedef struct FDFormat {
-FDriveType drive;
+FloppyDriveType drive;
 uint8_t last_sect;
 uint8_t max_track;
 uint8_t max_head;
 FDriveRate rate;
 } FDFormat;
 
+/**
+ * FDRIVE_DEFAULT: The default drive type if none specified.
+ * FDRIVE_AUTO_FALLBACK: The default drive type to assume if
+ *   no media is inserted.
+ */
+#define FDRIVE_DEFAULT FLOPPY_DRIVE_TYPE_AUTO
+#define FDRIVE_AUTO_FALLBACK FLOPPY_DRIVE_TYPE_144
+
 static const FDFormat fd_formats[] = {
 /* First entry is default format */
 /* 1.44 MB 3"1/2 floppy disks */
-{ FDRIVE_DRV_144, 18, 80, 1, FDRIVE_RATE_500K, },
-{ FDRIVE_DRV_144, 20, 80, 1, FDRIVE_RATE_500K, },
-{ FDRIVE_DRV_144, 21, 80, 1, FDRIVE_RATE_500K, },
-{ FDRIVE_DRV_144, 21, 82, 1, FDRIVE_RATE_500K, },
-{ FDRIVE_DRV_144, 21, 83, 1, FDRIVE_RATE_500K, },
-{ FDRIVE_DRV_144, 22, 80, 1, FDRIVE_RATE_500K, },
-{ FDRIVE_DRV_144, 23, 80, 1, FDRIVE_RATE_500K, },
-{ FDRIVE_DRV_144, 24, 80, 1, FDRIVE_RATE_500K, },
+{ FLOPPY_DRIVE_TYPE_144, 18, 80, 1, FDRIVE_RATE_500K, },
+{ FLOPPY_DRIVE_TYPE_144, 20, 80, 1, FDRIVE_RATE_500K, },
+{ FLOPPY_DRIVE_TYPE_144, 21, 80, 1, FDRIVE_RATE_500K, },
+{ FLOPPY_DRIVE_TYPE_144, 21, 82, 1, FDRIVE_RATE_500K, },
+{ FLOPPY_DRIVE_TYPE_144, 21, 83, 1, FDRIVE_RATE_500K, },
+{ FLOPPY_DRIVE_TYPE_144, 22, 80, 1, FDRIVE_RATE_500K, },
+{ FLOPPY_DRIVE_TYPE_144, 23, 80, 1, FDRIVE_RATE_500K, },
+{ FLOPPY_DRIVE_TYPE_144, 24, 80, 1, FDRIVE_RATE_500K, },
 /* 2.88 MB 3"1/2 floppy disks */
-{ FDRIVE_DRV_288, 36, 80, 1, FDRIVE_RATE_1M, },
-{ FDRIVE_DRV_288, 39, 80, 1, FDRIVE_RATE_1M, },
-{ FDRIVE_DRV_288, 40, 80, 1, FDRIVE_RATE_1M, },
-{ FDRIVE_DRV_288, 44, 80, 1, FDRIVE_RATE_1M, },
-{ FDRIVE_DRV_288, 48, 80, 1, FDRIVE_RATE_1M, },
+{ FLOPPY_DRIVE_TYPE_288, 36, 80, 1, FDRIVE_RATE_1M, },
+{ FLOPPY_DRIVE_TYPE_288, 39, 80, 1, FDRIVE_RATE_1M, },
+{ FLOPPY_DRIVE_TYPE_288, 40, 80, 1, FDRIVE_RATE_1M, },
+{ FLOPPY_DRIVE_TYPE_288, 44, 80, 1, FDRIVE_RATE_1M, },
+{ FLOPPY_DRIVE_TYPE_288, 48, 80, 1, FDRIVE_RATE_1M, },
 /* 720 kB 3"1/2 floppy disks */
-{ FDRIVE_DRV_144,  9, 80, 1, FDRIVE_RATE_250K, },
-{ FDRIVE_DRV_144, 10, 80, 1, FDRIVE_RATE_250K, },
-{ FDRIVE_DRV_144, 10, 82, 1, FDRIVE_RATE_250K, },
-{ FDRIVE_DRV_144, 10, 83, 1, FDRIVE_RATE_250K, },
-{ FDRIVE_DRV_144, 13, 80, 1, FDRIVE_RATE_250K, },
-{ FDRIVE_DRV_144, 14, 80, 1, FDRIVE_RATE_250K, },
+{ FLOPPY_DRIVE_TYPE_144,  9, 80, 1, FDRIVE_RATE_250K, },
+{ FLOPPY_DRIVE_TYPE_144, 10, 80, 1, FDRIVE_RATE_250K, },
+{ FLOPPY_DRIVE_TYPE_144, 10, 82, 1, FDRIVE_RATE_250K, },
+{ FLOPPY_DRIVE_TYPE_144, 10, 83, 1, FDRIVE_RATE_250K, },
+{ FLOPPY_DRIVE_TYPE_144, 13, 80, 1, FDRIVE_RATE_250K, },
+{ FLOPPY_DRIVE_TYPE_144, 14, 80, 1, FDRIVE_RATE_250K, },
 /* 1.2 MB 5"1/4 floppy disks */
-{ FDRIVE_DRV_120, 15, 80, 1, FDRIVE_RATE_500K, },
-{ FDRIVE_DRV_120, 18, 80, 1, FDRIVE_RATE_500K, },
-{ FDRIVE_DRV_120, 18, 82, 1, FDRIVE_RATE_500K, },
-{ FDRIVE_DRV_120, 18, 83, 1, FDRIVE_RATE_500K, },
-{ FDRIVE_DRV_120, 20, 80, 1, FDRIVE_RATE_500K, },
+{ FLOPPY_DRIVE_TYPE_120, 15, 80, 1, FDRIVE_RATE_500K, },
+{ FLOPPY_DRIVE_TYPE_120, 18, 80, 1, FDRIVE_RATE_500K, },
+{ FLOPPY_DRIVE_TYPE_120, 18, 82, 1, FDRIVE_RATE_500K, },
+{ FLOPPY_DRIVE_TYPE_120, 18, 83, 1, FDRIVE_RATE_500K, },
+{ FLOPPY_DRIVE_TYPE_120, 20, 80, 1, FDRIVE_RATE_500K, },
 /* 720 kB 5"1/4 floppy disks */
-{ FDRIVE_DRV_120,  9, 

[Qemu-block] [PATCH for-2.6 v2 09/10] qtest/fdc: Support for 2.88MB drives

[John Snow]

The old test assumes a 1.44MB drive.
Assert that the QEMU default drive is now either 1.44 or 2.88.

Signed-off-by: John Snow 
---
 tests/fdc-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/fdc-test.c b/tests/fdc-test.c
index b5a4696..526d459 100644
--- a/tests/fdc-test.c
+++ b/tests/fdc-test.c
@@ -267,7 +267,7 @@ static void test_cmos(void)
 uint8_t cmos;
 
 cmos = cmos_read(CMOS_FLOPPY);
-g_assert(cmos == 0x40);
+g_assert(cmos == 0x40 || cmos == 0x50);
 }
 
 static void test_no_media_on_start(void)
-- 
2.4.3

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.5?] qcow2: always initialize specific image info

[Eric Blake]

On 12/07/2015 10:51 AM, Eric Blake wrote:
> [adding qemu-devel - ALL patches should go to qemu-devel, even if they
> are also going to a sub-list like qemu-block]
> 
> On 12/07/2015 10:07 AM, Roman Kagan wrote:
>> qcow2_get_specific_info() used to have a code path which would leave
>> pointer to ImageInfoSpecificQCow2 uninitialized.
>>
>> We guess that it caused sporadic crashes on freeing an invalid pointer
>> in response to "query-block" QMP command in
>> visit_type_ImageInfoSpecificQCow2 with QapiDeallocVisitor.
>>
>> Although we have neither a solid proof nor a reproduction scenario,
>> making sure the field is initialized appears a reasonable thing to do.
>>
>> Signed-off-by: Roman Kagan 
>> ---
>>  block/qcow2.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Oops; hit send too soon.  I added for-2.5? to the subject line, because...

> 
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 88f56c8..67c9d3d 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -2739,7 +2739,7 @@ static ImageInfoSpecific 
>> *qcow2_get_specific_info(BlockDriverState *bs)
>>  
>>  *spec_info = (ImageInfoSpecific){
>>  .type  = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
>> -.u.qcow2 = g_new(ImageInfoSpecificQCow2, 1),
>> +.u.qcow2 = g_new0(ImageInfoSpecificQCow2, 1),
> 
> NACK.  This makes no difference, except when s->qcow_version is out of spec.
> 
>>  };
>>  if (s->qcow_version == 2) {
>>  *spec_info->u.qcow2 = (ImageInfoSpecificQCow2){
>>
> 
> If s->qcow_version is exactly 2, then we end up initializing all fields
> due to the assignment here; same if qcow_version is exactly 3.  The only
> time qcow2 remains uninitialized is if qcow_version is 0, 1, or > 3; but
> we refuse to handle qcow files with out-of-range versions.  So I don't
> see how you are plugging any uninitialized values; and therefore, I
> don't see how this is patching any crashes.

...if you can prove that we aren't gracefully handling an out-of-spec
qcow_version, such that the uninitialized memory in that scenario is
indeed causing a crash, then it is worth respinning a v2 of this patch
with that proof, and worth considering it for 2.5 if it really is a
crash fixer.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.5?] qcow2: always initialize specific image info

[Denis V. Lunev]

On 12/07/2015 08:54 PM, Eric Blake wrote:

On 12/07/2015 10:51 AM, Eric Blake wrote:

[adding qemu-devel - ALL patches should go to qemu-devel, even if they
are also going to a sub-list like qemu-block]

On 12/07/2015 10:07 AM, Roman Kagan wrote:

qcow2_get_specific_info() used to have a code path which would leave
pointer to ImageInfoSpecificQCow2 uninitialized.

We guess that it caused sporadic crashes on freeing an invalid pointer
in response to "query-block" QMP command in
visit_type_ImageInfoSpecificQCow2 with QapiDeallocVisitor.

Although we have neither a solid proof nor a reproduction scenario,
making sure the field is initialized appears a reasonable thing to do.

Signed-off-by: Roman Kagan 
---
  block/qcow2.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Oops; hit send too soon.  I added for-2.5? to the subject line, because...


diff --git a/block/qcow2.c b/block/qcow2.c
index 88f56c8..67c9d3d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2739,7 +2739,7 @@ static ImageInfoSpecific 
*qcow2_get_specific_info(BlockDriverState *bs)
  
  *spec_info = (ImageInfoSpecific){

  .type  = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
-.u.qcow2 = g_new(ImageInfoSpecificQCow2, 1),
+.u.qcow2 = g_new0(ImageInfoSpecificQCow2, 1),

NACK.  This makes no difference, except when s->qcow_version is out of spec.


  };
  if (s->qcow_version == 2) {
  *spec_info->u.qcow2 = (ImageInfoSpecificQCow2){


If s->qcow_version is exactly 2, then we end up initializing all fields
due to the assignment here; same if qcow_version is exactly 3.  The only
time qcow2 remains uninitialized is if qcow_version is 0, 1, or > 3; but
we refuse to handle qcow files with out-of-range versions.  So I don't
see how you are plugging any uninitialized values; and therefore, I
don't see how this is patching any crashes.

...if you can prove that we aren't gracefully handling an out-of-spec
qcow_version, such that the uninitialized memory in that scenario is
indeed causing a crash, then it is worth respinning a v2 of this patch
with that proof, and worth considering it for 2.5 if it really is a
crash fixer.


Here is an info about our crash.

we have this crash under unknown conditions on RHEV version of QEMU.

Sorry, there is no much additional info. For the time being it
has happen only once.


*** Error in `/usr/libexec/qemu-kvm': free(): invalid pointer: 
0x7f1c453757b8 ***
=== Backtrace: =
/lib64/libc.so.6(+0x7d1fd)[0x7f1c450381fd]
/lib64/libglib-2.0.so.0(g_free+0xf)[0x7f1c49b5236f]
/usr/libexec/qemu-kvm(+0x1a71e9)[0x7f1c4ca0d1e9]
/usr/libexec/qemu-kvm(+0x1a779e)[0x7f1c4ca0d79e]
/usr/libexec/qemu-kvm(+0x1a7bf3)[0x7f1c4ca0dbf3]
/usr/libexec/qemu-kvm(+0x1a8664)[0x7f1c4ca0e664]
/usr/libexec/qemu-kvm(+0x1a92dd)[0x7f1c4ca0f2dd]
/usr/libexec/qemu-kvm(+0x1a9380)[0x7f1c4ca0f380]
/usr/libexec/qemu-kvm(+0x194eb8)[0x7f1c4c9faeb8]
/usr/libexec/qemu-kvm(+0xb35d8)[0x7f1c4c9195d8]
/usr/libexec/qemu-kvm(+0x301f02)[0x7f1c4cb67f02]
/usr/libexec/qemu-kvm(+0x31483f)[0x7f1c4cb7a83f]
/usr/libexec/qemu-kvm(+0x31490e)[0x7f1c4cb7a90e]
/usr/libexec/qemu-kvm(+0xb112f)[0x7f1c4c91712f]
/usr/libexec/qemu-kvm(+0x18a460)[0x7f1c4c9f0460]
/lib64/libglib-2.0.so.0(g_main_context_dispatch+0x15a)[0x7f1c49b4c79a]
/usr/libexec/qemu-kvm(+0x2b96b8)[0x7f1c4cb1f6b8]
/usr/libexec/qemu-kvm(+0x87a4e)[0x7f1c4c8eda4e]
/lib64/libc.so.6(__libc_start_main+0xf5)[0x7f1c44fdcaf5]
/usr/libexec/qemu-kvm(+0x8c2bd)[0x7f1c4c8f22bd]
=== Memory map: 

which we have decoded as

Decoded stacktrace:

g_free+0xf
visit_type_ImageInfoSpecificQCow2+169
visit_type_ImageInfoSpecific+302
visit_type_ImageInfo+867
visit_type_BlockDeviceInfo+820
visit_type_BlockInfo+685
visit_type_BlockInfoList+128
qmp_marshal_input_query_block+232
handle_qmp_command+1992
json_message_process_token+242
json_lexer_feed_char+383
json_lexer_feed+46
monitor_control_read+31
tcp_chr_read+144
g_main_context_dispatch+0x15a
main_loop_wait+440
main+5502
__libc_start_main+0xf5
_start+41

which looks like

More specifically (expanding inlines along the stack trace):

(gdb) l *(visit_type_ImageInfoSpecificQCow2+169)
0x1a71e9 is in visit_type_ImageInfoSpecificQCow2 (qapi-visit.c:552).
548 static void visit_type_ImageInfoSpecificQCow2_fields(Visitor *m, 
ImageInfoSpecificQCow2 **obj, Error **errp)
549 {
550 Error *err = NULL;
551 ==> visit_type_str(m, &(*obj)->compat, "compat", );
552 if (err) {
553 goto out;
554 }


(gdb) l visit_type_str
238 void visit_type_str(Visitor *v, char **obj, const char *name, Error 
**errp)
239 {
240 ==> v->type_str(v, obj, name, errp);
241 }


(gdb) l qapi_dealloc_visitor_new
175 QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
176 {
177 QapiDeallocVisitor *v;
178
179 v = g_malloc0(sizeof(*v));
[...]
191 v->visitor.type_str = qapi_dealloc_type_str;
192 v->visitor.type_number = qapi_dealloc_type_number;
193 

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.6 v2 04/10] fdc: add default drive type option

[Eric Blake]

On 12/07/2015 04:34 PM, John Snow wrote:
> This patch adds a new explicit Floppy Drive Type option. The existing
> behavior in QEMU is to automatically guess a drive type based on the
> media inserted, or if a diskette is not present, arbitrarily assign one.
> 
> This behavior can be described as "auto." This patch adds explicit
> behaviors: 120, 144, 288, auto, and none. The new "auto" behavior
> is intended to mimick current behavior, while the other types pick

s/mimick/mimic/ (one of those weird 'ic' verbs where the 'k' is
necessary in past tense but not present tense)

> one explicitly.
> 
> In a future patch, the goal is to change the FDC's default drive type
> from auto (falling back to 1.44MB) to auto (falling back to 2.88MB).
> 
> In order to allow users to obtain the old behaviors, though, a mechanism
> for specifying the exact type of drive we want is needed.
> 
> This patch adds the properties, but it is not acted on yet in favor of
> making those changes a little more explicitly clear in standalone patches
> later in this patch set.
> 
> Signed-off-by: John Snow 
> ---

> +++ b/qapi/block.json
> @@ -40,6 +40,22 @@
>'data': ['auto', 'none', 'lba', 'large', 'rechs']}
>  
>  ##
> +# @FloppyDriveType
> +#
> +# Type of Floppy drive to be emulated by the Floppy Disk Controller.
> +#
> +# @144:  1.44MB 3.5" drive
> +# @288:  2.88MB 3.5" drive
> +# @120:  1.5MB 5.25" drive

Names start with a digit - not the prettiest, but also not the first
instance, so qapi handles it just fine.  And I don't have any
suggestions for a better yet still concise name.

> +# @none: No drive connected
> +# @auto: Automatically determined by inserted media at boot
> +#
> +# Since: 2.6
> +##
> +{ 'enum': 'FloppyDriveType',
> +  'data': ['144', '288', '120', 'none', 'auto']}

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH for-2.5] qcow2: always initialize specific image info

[Eric Blake]

[adding qemu-devel - ALL patches should go to qemu-devel, even if they
are also going to a sub-list like qemu-block]

On 12/07/2015 10:07 AM, Roman Kagan wrote:
> qcow2_get_specific_info() used to have a code path which would leave
> pointer to ImageInfoSpecificQCow2 uninitialized.
> 
> We guess that it caused sporadic crashes on freeing an invalid pointer
> in response to "query-block" QMP command in
> visit_type_ImageInfoSpecificQCow2 with QapiDeallocVisitor.
> 
> Although we have neither a solid proof nor a reproduction scenario,
> making sure the field is initialized appears a reasonable thing to do.
> 
> Signed-off-by: Roman Kagan 
> ---
>  block/qcow2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)


> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 88f56c8..67c9d3d 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2739,7 +2739,7 @@ static ImageInfoSpecific 
> *qcow2_get_specific_info(BlockDriverState *bs)
>  
>  *spec_info = (ImageInfoSpecific){
>  .type  = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
> -.u.qcow2 = g_new(ImageInfoSpecificQCow2, 1),
> +.u.qcow2 = g_new0(ImageInfoSpecificQCow2, 1),

NACK.  This makes no difference, except when s->qcow_version is out of spec.

>  };
>  if (s->qcow_version == 2) {
>  *spec_info->u.qcow2 = (ImageInfoSpecificQCow2){
> 

If s->qcow_version is exactly 2, then we end up initializing all fields
due to the assignment here; same if qcow_version is exactly 3.  The only
time qcow2 remains uninitialized is if qcow_version is 0, 1, or > 3; but
we refuse to handle qcow files with out-of-range versions.  So I don't
see how you are plugging any uninitialized values; and therefore, I
don't see how this is patching any crashes.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH RFC for-2.6 1/3] HBitmap: Introduce "meta" bitmap to track bit changes

[Fam Zheng]

On Mon, 12/07 16:32, Vladimir Sementsov-Ogievskiy wrote:
> On 07.12.2015 08:59, Fam Zheng wrote:
> >The meta bitmap will have the same size and granularity as the tracked
> >bitmap, and upon each bit toggle, the corresponding bit in the meta
> >bitmap, at an identical position, will be set.
> 
> No, meta bitmap should not have same granularity. If we have 16tb
> storage, then 16kb granulated bitmap will occupy more then 128 mb.
> And additional meta bitmap of the same size and granularity is
> redundant 128+ mb of RAM, when actually we need meta bitmap for
> blocks for example of 1mb and it should occupy about 128 bits.

Makes sense, do you prefer a parameterized granularity, or a fixed scaling like
one bit for 1 word?

Fam

> 
> 
> >
> >Signed-off-by: Fam Zheng 
> >---
> >  include/qemu/hbitmap.h |  7 +++
> >  util/hbitmap.c | 22 ++
> >  2 files changed, 29 insertions(+)
> >
> >diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> >index bb94a00..09a6b06 100644
> >--- a/include/qemu/hbitmap.h
> >+++ b/include/qemu/hbitmap.h
> >@@ -181,6 +181,13 @@ void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap 
> >*hb, uint64_t first);
> >   */
> >  unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi);
> >+/* hbitmap_create_meta
> >+ * @hb: The HBitmap to operate on.
> >+ *
> >+ * Create a "meta" hbitmap to track dirtiness of the bits in this HBitmap.
> >+ */
> >+HBitmap *hbitmap_create_meta(HBitmap *hb);
> >+
> >  /**
> >   * hbitmap_iter_next:
> >   * @hbi: HBitmapIter to operate on.
> >diff --git a/util/hbitmap.c b/util/hbitmap.c
> >index 50b888f..3ad406e 100644
> >--- a/util/hbitmap.c
> >+++ b/util/hbitmap.c
> >@@ -81,6 +81,9 @@ struct HBitmap {
> >   */
> >  int granularity;
> >+/* A meta dirty bitmap to track the dirtiness of bits in this HBitmap. 
> >*/
> >+HBitmap *meta;
> >+
> >  /* A number of progressively less coarse bitmaps (i.e. level 0 is the
> >   * coarsest).  Each bit in level N represents a word in level N+1 that
> >   * has a set bit, except the last level where each bit represents the
> >@@ -232,6 +235,7 @@ static inline bool hb_set_elem(unsigned long *elem, 
> >uint64_t start, uint64_t las
> >  /* The recursive workhorse (the depth is limited to HBITMAP_LEVELS)... */
> >  static void hb_set_between(HBitmap *hb, int level, uint64_t start, 
> > uint64_t last)
> >  {
> >+uint64_t save_start = start;
> >  size_t pos = start >> BITS_PER_LEVEL;
> >  size_t lastpos = last >> BITS_PER_LEVEL;
> >  bool changed = false;
> >@@ -252,6 +256,9 @@ static void hb_set_between(HBitmap *hb, int level, 
> >uint64_t start, uint64_t last
> >  }
> >  }
> >  changed |= hb_set_elem(>levels[level][i], start, last);
> >+if (hb->meta && level == HBITMAP_LEVELS - 1 && changed) {
> >+hbitmap_set(hb->meta, save_start, last - save_start + 1);
> >+}
> >  /* If there was any change in this layer, we may have to update
> >   * the one above.
> >@@ -298,6 +305,7 @@ static inline bool hb_reset_elem(unsigned long *elem, 
> >uint64_t start, uint64_t l
> >  /* The recursive workhorse (the depth is limited to HBITMAP_LEVELS)... */
> >  static void hb_reset_between(HBitmap *hb, int level, uint64_t start, 
> > uint64_t last)
> >  {
> >+uint64_t save_start = start;
> >  size_t pos = start >> BITS_PER_LEVEL;
> >  size_t lastpos = last >> BITS_PER_LEVEL;
> >  bool changed = false;
> >@@ -336,6 +344,10 @@ static void hb_reset_between(HBitmap *hb, int level, 
> >uint64_t start, uint64_t la
> >  lastpos--;
> >  }
> >+if (hb->meta && level == HBITMAP_LEVELS - 1 && changed) {
> >+hbitmap_set(hb->meta, save_start, last - save_start + 1);
> >+}
> >+
> >  if (level > 0 && changed) {
> >  hb_reset_between(hb, level - 1, pos, lastpos);
> >  }
> >@@ -384,6 +396,9 @@ void hbitmap_free(HBitmap *hb)
> >  for (i = HBITMAP_LEVELS; i-- > 0; ) {
> >  g_free(hb->levels[i]);
> >  }
> >+if (hb->meta) {
> >+hbitmap_free(hb->meta);
> >+}
> >  g_free(hb);
> >  }
> >@@ -493,3 +508,10 @@ bool hbitmap_merge(HBitmap *a, const HBitmap *b)
> >  return true;
> >  }
> >+
> >+HBitmap *hbitmap_create_meta(HBitmap *hb)
> >+{
> >+assert(!hb->meta);
> >+hb->meta = hbitmap_alloc(hb->size, hb->granularity);
> >+return hb->meta;
> >+}
> 
> 
> -- 
> Best regards,
> Vladimir
> * now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.
> 

Re: [Qemu-block] [Qemu-devel] [PATCH RFC for-2.6 0/3] block: Add meta dirty bitmap for migration/persistence

[Fam Zheng]

On Mon, 12/07 18:47, John Snow wrote:
> 
> 
> On 12/07/2015 12:59 AM, Fam Zheng wrote:
> > Vladimir,
> > 
> > This is what I propose to implement meta bitmap. It's implemented in the
> > HBitmap level to be more efficient, and the interface slightly varies too.
> > 
> 
> I missed it: What was wrong with Vladimir's approach / what are the
> benefits of this approach?

The only real difference with this series is, only actual bit toggling will
mark meta dirty. Vladimir's approach was in BdrvDirtyBitmap level which can't
tell bit toggling from repetitive bit set/unset. This is from his patch:

@@ -3390,6 +3428,9 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
 {
 assert(bdrv_dirty_bitmap_enabled(bitmap));
 hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
+if (bitmap->meta_bitmap) {
+hbitmap_set(bitmap->meta_bitmap, cur_sector, nr_sectors);
+}
 }

 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
@@ -3397,6 +3438,9 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
 {
 assert(bdrv_dirty_bitmap_enabled(bitmap));
 hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
+if (bitmap->meta_bitmap) {
+hbitmap_set(bitmap->meta_bitmap, cur_sector, nr_sectors);
+}
 }

 void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap)

> 
> > I'd like to use these operations to make dirty bitmap persistence more
> > efficient too: unchanged dirty bits don't need to be flushed to disk. So I'm
> > posting this as a separate series for a common base for both sides.
> > 
> 
> This is a reasonable use of the meta-bitmap strategy in general.
> 
> Keep in mind Vladimir's approach to Meta bitmaps used a different
> granularity such that 1 physical bit implied 1 sector needed to be
> re-transmitted.

Yes, I can fix the meta bitmap granularity.

> 
> A meta-bitmap that keeps track of disk flushes may require a different
> granularity than one used for migration.
> 
> > Posting as RFC as 2.6 dev phase is just starting, we can still tweak the
> > interface and/or implementation to fit the need.
> > 
> > Fam Zheng (3):
> >   HBitmap: Introduce "meta" bitmap to track bit changes
> >   tests: Add test code for meta bitmap
> >   block: Support meta dirty bitmap
> > 
> >  block.c| 46 ++-
> >  block/mirror.c |  3 +-
> >  blockdev.c |  3 +-
> >  include/block/block.h  | 11 
> >  include/qemu/hbitmap.h |  7 +
> >  migration/block.c  |  2 +-
> >  tests/test-hbitmap.c   | 74 
> > ++
> >  util/hbitmap.c | 22 +++
> >  8 files changed, 164 insertions(+), 4 deletions(-)
> > 

Re: [Qemu-block] [Qemu-devel] [PATCH] virtio-blk: Drop x-data-plane option

[Fam Zheng]

On Mon, 12/07 21:02, Fam Zheng wrote:
> On Mon, 12/07 12:29, Cornelia Huck wrote:
> > No general objection to removing x-data-plane; but this probably wants
> > a mention on the changelog as x-data-plane has been described in
> > various howtos etc. over the years.

Add a changelog line,

http://wiki.qemu.org/ChangeLog/2.5#Block_devices_and_tools

please review.

Fam

Re: [Qemu-block] [Qemu-devel] [PATCH 0/2] Preparation for PCI devices convert to realize()

[Cao jin]

Hi, Marcel

On 12/07/2015 09:42 PM, Marcel Apfelbaum wrote:

On 12/07/2015 10:08 AM, Cao jin wrote:

There are many PCI devices still using .init() as its initialization
function,
I am planning to do the "convert to realize()" work, and PCI bridge
devices are
chosen first. The supporting functions should be modified
first.msi_init() &
msix_init() are supporting functions for PCI devices.

Because this patchset is much different from the previous one, so, didn`t
add "V2" in the subject


Hi,

Even if the patches are different is worth mentioning V2, otherwise
the maintainer would not know which to take.



I see. Thanks for your suggestion:)


Thanks,
Marcel



--
Yours Sincerely,

Cao Jin

Re: [Qemu-block] [PATCH RFC for-2.6 0/3] block: Add meta dirty bitmap for migration/persistence

[Fam Zheng]

On Mon, 12/07 17:19, Vladimir Sementsov-Ogievskiy wrote:
> On 07.12.2015 08:59, Fam Zheng wrote:
> >Vladimir,
> >
> >This is what I propose to implement meta bitmap. It's implemented in the
> >HBitmap level to be more efficient, and the interface slightly varies too.
> 
> What is the benefit?
> 
> Hbitmap usage:
> 
> 1) BdrvDirtyBitmap - need meta
> 2) BackupBlockJob - doesn't need meta
> 3) BlockDirtyBitmapState - doesn't need meta
> 4) now I'm working on series for parallels format and I use HBitmap
> to mark allocated/free clusters.. - doesn't need meta
> 5) your meta hbitmap =) - doesn't need meta..

6) persistence dirty bitmap. - need meta

> 
> So, what is the benefit of moving this functionality to parent
> class? (Which is complicated without it)..

See my reply to John's comment on the cover letter. This is more efficient than
doing it in BdrvDirtyBitmap.

> 
> However, I'm not really against, except my comment to the first patch.
> 
> PS:
> Actually I don't like HBitmap - BdrvDirtyBitmap..
> - No implementation without granularity
>I need HBitmap without granularity for my needs and have to
> use granularity=0. If there was HBitmap without granularity some
> operations can be faster - for example, finding next/previous/last
> zeros, jumping by words not by bits..
> - It is not sparse. Empty bitmap occupies lots of ram.
> - different granularity units for HBitmap and BdrvDirtyBitmap
> - different layers with/without granularity in hbitmap.c
> - HBitmap with non-zero granularity doesn't know its size (only
> rounded up to granularity)
> - necessity of writing wrappers like
>bdrv_dirty_bitmap_do_something(...)
>{
> hbitmap_do_something(...)
>}
>-- Yes, I understand that this is inevitably, but I just don't like it..
> - BdrvDirtyBitmap is defined in block.c.. I think, it should have
> its own .c file.

Yes, I agree we should cut it out during 2.6, with a separate header.