Re: [PATCH v1 1/4] virtio: protect non-modern devices from too big virtqueue size setting

[Denis Plotnikov]

On 05.11.2019 23:56, Michael S. Tsirkin wrote:
> On Tue, Nov 05, 2019 at 07:11:02PM +0300, Denis Plotnikov wrote:
>> The patch protects from creating illegal virtio device configuration
>> via direct virtqueue size property setting.
>>
>> Signed-off-by: Denis Plotnikov 
>> ---
>>   hw/virtio/virtio-blk-pci.c  |  9 +
>>   hw/virtio/virtio-scsi-pci.c | 10 ++
>>   2 files changed, 19 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-blk-pci.c b/hw/virtio/virtio-blk-pci.c
>> index 60c9185c39..6177ff1df8 100644
>> --- a/hw/virtio/virtio-blk-pci.c
>> +++ b/hw/virtio/virtio-blk-pci.c
>> @@ -48,6 +48,15 @@ static void virtio_blk_pci_realize(VirtIOPCIProxy 
>> *vpci_dev, Error **errp)
>>   {
>>   VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(vpci_dev);
>>   DeviceState *vdev = DEVICE(>vdev);
>> +bool modern = virtio_pci_modern(vpci_dev);
>> +uint32_t queue_size = dev->vdev.conf.queue_size;
>> +
>> +if (!modern && queue_size > 128) {
>> +error_setg(errp,
>> +   "too big queue size (%u, max: 128) "
>> +   "for non-modern virtio device", queue_size);
>> +return;
>> +}
>
> this enables for transitional so still visible to legacy
> interface. I am guessing you want to check whether
> device is accessed through the modern interface instead.

My goal is to not break something when I'm setting the queue size > 128 
(taking into account the current seabios queue size restriction to 128). 
I'm not quite sure what to check. Could I ask why one want to the check 
whether accessing through the modern interface and how it could be checked?

Thanks!

Denis

>>   if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
>>   vpci_dev->nvectors = dev->vdev.conf.num_queues + 1;
>> diff --git a/hw/virtio/virtio-scsi-pci.c b/hw/virtio/virtio-scsi-pci.c
>> index 2830849729..6e6790fda5 100644
>> --- a/hw/virtio/virtio-scsi-pci.c
>> +++ b/hw/virtio/virtio-scsi-pci.c
>> @@ -17,6 +17,7 @@
>>   
>>   #include "hw/virtio/virtio-scsi.h"
>>   #include "virtio-pci.h"
>> +#include "qapi/error.h"
>>   
>>   typedef struct VirtIOSCSIPCI VirtIOSCSIPCI;
>>   
>> @@ -47,6 +48,15 @@ static void virtio_scsi_pci_realize(VirtIOPCIProxy 
>> *vpci_dev, Error **errp)
>>   VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
>>   DeviceState *proxy = DEVICE(vpci_dev);
>>   char *bus_name;
>> +bool modern = virtio_pci_modern(vpci_dev);
>> +uint32_t virtqueue_size = vs->conf.virtqueue_size;
>> +
>> +if (!modern && virtqueue_size > 128) {
>> +error_setg(errp,
>> +   "too big virtqueue size (%u, max: 128) "
>> +   "for non-modern virtio device", virtqueue_size);
>> +return;
>> +}
> why? what is illegal about 256 for legacy?
>
>>   
>>   if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
>>   vpci_dev->nvectors = vs->conf.num_queues + 3;
>> -- 
>> 2.17.0

Re: [PATCH v1 1/4] virtio: protect non-modern devices from too big virtqueue size setting

[Michael S. Tsirkin]

On Tue, Nov 05, 2019 at 07:11:02PM +0300, Denis Plotnikov wrote:
> The patch protects from creating illegal virtio device configuration
> via direct virtqueue size property setting.
> 
> Signed-off-by: Denis Plotnikov 
> ---
>  hw/virtio/virtio-blk-pci.c  |  9 +
>  hw/virtio/virtio-scsi-pci.c | 10 ++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/hw/virtio/virtio-blk-pci.c b/hw/virtio/virtio-blk-pci.c
> index 60c9185c39..6177ff1df8 100644
> --- a/hw/virtio/virtio-blk-pci.c
> +++ b/hw/virtio/virtio-blk-pci.c
> @@ -48,6 +48,15 @@ static void virtio_blk_pci_realize(VirtIOPCIProxy 
> *vpci_dev, Error **errp)
>  {
>  VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(vpci_dev);
>  DeviceState *vdev = DEVICE(>vdev);
> +bool modern = virtio_pci_modern(vpci_dev);
> +uint32_t queue_size = dev->vdev.conf.queue_size;
> +
> +if (!modern && queue_size > 128) {
> +error_setg(errp,
> +   "too big queue size (%u, max: 128) "
> +   "for non-modern virtio device", queue_size);
> +return;
> +}


this enables for transitional so still visible to legacy
interface. I am guessing you want to check whether
device is accessed through the modern interface instead.

>  if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
>  vpci_dev->nvectors = dev->vdev.conf.num_queues + 1;

> diff --git a/hw/virtio/virtio-scsi-pci.c b/hw/virtio/virtio-scsi-pci.c
> index 2830849729..6e6790fda5 100644
> --- a/hw/virtio/virtio-scsi-pci.c
> +++ b/hw/virtio/virtio-scsi-pci.c
> @@ -17,6 +17,7 @@
>  
>  #include "hw/virtio/virtio-scsi.h"
>  #include "virtio-pci.h"
> +#include "qapi/error.h"
>  
>  typedef struct VirtIOSCSIPCI VirtIOSCSIPCI;
>  
> @@ -47,6 +48,15 @@ static void virtio_scsi_pci_realize(VirtIOPCIProxy 
> *vpci_dev, Error **errp)
>  VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
>  DeviceState *proxy = DEVICE(vpci_dev);
>  char *bus_name;
> +bool modern = virtio_pci_modern(vpci_dev);
> +uint32_t virtqueue_size = vs->conf.virtqueue_size;
> +
> +if (!modern && virtqueue_size > 128) {
> +error_setg(errp,
> +   "too big virtqueue size (%u, max: 128) "
> +   "for non-modern virtio device", virtqueue_size);
> +return;
> +}

why? what is illegal about 256 for legacy?

>  
>  if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
>  vpci_dev->nvectors = vs->conf.num_queues + 3;
> -- 
> 2.17.0

Re: [PATCH v1 3/4] virtio: increase virtuqueue sizes in new machine types

[Michael S. Tsirkin]

On Tue, Nov 05, 2019 at 07:11:04PM +0300, Denis Plotnikov wrote:
> Linux guests submit IO requests no longer than PAGE_SIZE * max_seg
> field reported by SCSI controler. Thus typical sequential read with
> 1 MB size results in the following pattern of the IO from the guest:
>   8,16   115754 2.766095122  2071  D   R 2095104 + 1008 [dd]
>   8,16   115755 2.766108785  2071  D   R 2096112 + 1008 [dd]
>   8,16   115756 2.766113486  2071  D   R 2097120 + 32 [dd]
>   8,16   115757 2.767668961 0  C   R 2095104 + 1008 [0]
>   8,16   115758 2.768534315 0  C   R 2096112 + 1008 [0]
>   8,16   115759 2.768539782 0  C   R 2097120 + 32 [0]
> The IO was generated by
>   dd if=/dev/sda of=/dev/null bs=1024 iflag=direct
> 
> This effectively means that on rotational disks we will observe 3 IOPS
> for each 2 MBs processed. This definitely negatively affects both
> guest and host IO performance.
> 
> The cure is relatively simple - we should report lengthy scatter-gather
> ability of the SCSI controller. Fortunately the situation here is very
> good. VirtIO transport layer can accomodate 1024 items in one request
> while we are using only 128. This situation is present since almost
> very beginning. 2 items are dedicated for request metadata thus we
> should publish VIRTQUEUE_MAX_SIZE - 2 as max_seg.
> 
> The following pattern is observed after the patch:
>   8,16   1 9921 2.662721340  2063  D   R 2095104 + 1024 [dd]
>   8,16   1 9922 2.662737585  2063  D   R 2096128 + 1024 [dd]
>   8,16   1 9923 2.665188167 0  C   R 2095104 + 1024 [0]
>   8,16   1 9924 2.665198777 0  C   R 2096128 + 1024 [0]
> which is much better.
> 
> To fix this particular case, the patch adds new machine types with
> extended virtqueue sizes to 256 which also increases max_seg to 254
> implicitly.
> 
> Suggested-by: Denis V. Lunev 
> Signed-off-by: Denis Plotnikov 
> ---



the way we normally do this is change the defaults to 256.
what is wrong with doing that?


>  hw/core/machine.c   | 14 ++
>  hw/i386/pc_piix.c   | 16 +---
>  hw/i386/pc_q35.c| 14 --
>  include/hw/boards.h |  6 ++
>  4 files changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 55b08f1466..28013a0e3f 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -24,6 +24,13 @@
>  #include "hw/pci/pci.h"
>  #include "hw/mem/nvdimm.h"
>  
> +GlobalProperty hw_compat_4_0_1[] = {
> +{ "virtio-blk-device", "queue-size", "128" },
> +{ "virtio-scsi-device", "virtqueue_size", "128" },
> +{ "vhost-scsi-device", "virtqueue_size", "128" },
> +};
> +const size_t hw_compat_4_0_1_len = G_N_ELEMENTS(hw_compat_4_0_1);
> +
>  GlobalProperty hw_compat_4_0[] = {
>  { "virtio-balloon-device", "qemu-4-0-config-size", "true" },
>  };
> @@ -157,6 +164,13 @@ GlobalProperty hw_compat_2_1[] = {
>  };
>  const size_t hw_compat_2_1_len = G_N_ELEMENTS(hw_compat_2_1);
>  
> +GlobalProperty hw_compat[] = {
> +{ "virtio-blk-device", "queue-size", "256" },
> +{ "virtio-scsi-device", "virtqueue_size", "256" },
> +{ "vhost-scsi-device", "virtqueue_size", "256" },
> +};
> +const size_t hw_compat_len = G_N_ELEMENTS(hw_compat);
> +
>  static char *machine_get_accel(Object *obj, Error **errp)
>  {
>  MachineState *ms = MACHINE(obj);
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 8ad8e885c6..2260a61b1b 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -426,15 +426,27 @@ static void pc_i440fx_machine_options(MachineClass *m)
>  m->default_machine_opts = "firmware=bios-256k.bin";
>  m->default_display = "std";
>  machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
> +compat_props_add(m->compat_props, hw_compat, hw_compat_len);
>  }
>  
> -static void pc_i440fx_4_0_machine_options(MachineClass *m)
> +static void pc_i440fx_4_0_2_machine_options(MachineClass *m)
>  {
>  pc_i440fx_machine_options(m);
>  m->alias = "pc";
>  m->is_default = 1;
>  }
>  
> +DEFINE_I440FX_MACHINE(v4_0_2, "pc-i440fx-4.0.2", NULL,
> +  pc_i440fx_4_0_2_machine_options);
> +
> +static void pc_i440fx_4_0_machine_options(MachineClass *m)
> +{
> +pc_i440fx_4_0_2_machine_options(m);
> +m->alias = NULL;
> +m->is_default = 0;
> +compat_props_add(m->compat_props, hw_compat_4_0_1, hw_compat_4_0_1_len);
> +}
> +
>  DEFINE_I440FX_MACHINE(v4_0, "pc-i440fx-4.0", NULL,
>pc_i440fx_4_0_machine_options);
>  
> @@ -443,9 +455,7 @@ static void pc_i440fx_3_1_machine_options(MachineClass *m)
>  PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>  
>  pc_i440fx_4_0_machine_options(m);
> -m->is_default = 0;
>  m->smbus_no_migration_support = true;
> -m->alias = NULL;
>  pcmc->pvh_enabled = false;
>  compat_props_add(m->compat_props, hw_compat_3_1, hw_compat_3_1_len);
>  

Re: [PATCH v1 2/4] virtio: make seg_max virtqueue size dependent

[Michael S. Tsirkin]

On Tue, Nov 05, 2019 at 07:11:03PM +0300, Denis Plotnikov wrote:
> seg_max has a restriction to be less or equal to virtqueue size
> according to Virtio 1.0 specification
> 
> Although seg_max can't be set directly, it's worth to express this
> dependancy directly in the code for sanity purpose.
> 
> Signed-off-by: Denis Plotnikov 

This is guest visible so needs to be machine type dependent, right?

> ---
>  hw/block/virtio-blk.c | 2 +-
>  hw/scsi/virtio-scsi.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 06e57a4d39..21530304cf 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -903,7 +903,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
> uint8_t *config)
>  blk_get_geometry(s->blk, );
>  memset(, 0, sizeof(blkcfg));
>  virtio_stq_p(vdev, , capacity);
> -virtio_stl_p(vdev, _max, 128 - 2);
> +virtio_stl_p(vdev, _max, s->conf.queue_size - 2);
>  virtio_stw_p(vdev, , conf->cyls);
>  virtio_stl_p(vdev, _size, blk_size);
>  virtio_stw_p(vdev, _io_size, conf->min_io_size / blk_size);
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 839f120256..f7e5533cd5 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -650,7 +650,7 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
>  VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
>  
>  virtio_stl_p(vdev, >num_queues, s->conf.num_queues);
> -virtio_stl_p(vdev, >seg_max, 128 - 2);
> +virtio_stl_p(vdev, >seg_max, s->conf.virtqueue_size - 2);
>  virtio_stl_p(vdev, >max_sectors, s->conf.max_sectors);
>  virtio_stl_p(vdev, >cmd_per_lun, s->conf.cmd_per_lun);
>  virtio_stl_p(vdev, >event_info_size, sizeof(VirtIOSCSIEvent));
> -- 
> 2.17.0

[PATCH] virtio-blk: advertise F_WCE (F_FLUSH) if F_CONFIG_WCE is advertised

[Evgeny Yakovlev]

Virtio spec 1.1 (and earlier), 5.2.5.2 Driver Requirements: Device
Initialization:

"Devices SHOULD always offer VIRTIO_BLK_F_FLUSH, and MUST offer it if
they offer VIRTIO_BLK_F_CONFIG_WCE"

Currently F_CONFIG_WCE and F_WCE are not connected to each other.
Qemu will advertise F_CONFIG_WCE if config-wce argument is
set for virtio-blk device. And F_WCE is advertised only if
underlying block backend actually has it's caching enabled.

Fix this by advertising F_WCE if F_CONFIG_WCE is also advertised.

To preserve backwards compatibility with newer machine types make this
behaviour governed by "x-enable-wce-if-config-wce" virtio-blk-device
property and introduce hw_compat_4_2 with new property being off by
default for all machine types <= 4.2 (but don't introduce 4.3
machine type itself yet).

Signed-off-by: Evgeny Yakovlev 
---
 hw/arm/virt.c  | 1 +
 hw/block/virtio-blk.c  | 6 +-
 hw/core/machine.c  | 5 +
 hw/i386/pc_piix.c  | 1 +
 hw/i386/pc_q35.c   | 1 +
 hw/ppc/spapr.c | 2 +-
 hw/s390x/s390-virtio-ccw.c | 1 +
 include/hw/boards.h| 3 +++
 include/hw/virtio/virtio-blk.h | 1 +
 9 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d4bedc2..bf4b1cb 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2149,6 +2149,7 @@ type_init(machvirt_machine_init);
 
 static void virt_machine_4_2_options(MachineClass *mc)
 {
+compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
 }
 DEFINE_VIRT_MACHINE_AS_LATEST(4, 2)
 
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 4c357d2..d62e637 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -991,7 +991,9 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, 
uint64_t features,
 virtio_add_feature(, VIRTIO_BLK_F_SCSI);
 }
 
-if (blk_enable_write_cache(s->blk)) {
+if (blk_enable_write_cache(s->blk) ||
+(s->conf.x_enable_wce_if_config_wce &&
+ virtio_has_feature(features, VIRTIO_BLK_F_CONFIG_WCE))) {
 virtio_add_feature(, VIRTIO_BLK_F_WCE);
 }
 if (blk_is_read_only(s->blk)) {
@@ -1270,6 +1272,8 @@ static Property virtio_blk_properties[] = {
conf.max_discard_sectors, BDRV_REQUEST_MAX_SECTORS),
 DEFINE_PROP_UINT32("max-write-zeroes-sectors", VirtIOBlock,
conf.max_write_zeroes_sectors, 
BDRV_REQUEST_MAX_SECTORS),
+DEFINE_PROP_BOOL("x-enable-wce-if-config-wce", VirtIOBlock,
+ conf.x_enable_wce_if_config_wce, true),
 DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1689ad3..023548b 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -27,6 +27,11 @@
 #include "hw/pci/pci.h"
 #include "hw/mem/nvdimm.h"
 
+GlobalProperty hw_compat_4_2[] = {
+{ "virtio-blk-device", "x-enable-wce-if-config-wce", "off" },
+};
+const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2);
+
 GlobalProperty hw_compat_4_1[] = {
 { "virtio-pci", "x-pcie-flr-init", "off" },
 };
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index c15929a..5e9a8de 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -423,6 +423,7 @@ static void pc_i440fx_4_2_machine_options(MachineClass *m)
 m->alias = "pc";
 m->is_default = 1;
 pcmc->default_cpu_version = 1;
+compat_props_add(m->compat_props, hw_compat_4_2, hw_compat_4_2_len);
 }
 
 DEFINE_I440FX_MACHINE(v4_2, "pc-i440fx-4.2", NULL,
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index d51f524..6008d5a 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -354,6 +354,7 @@ static void pc_q35_4_2_machine_options(MachineClass *m)
 pc_q35_machine_options(m);
 m->alias = "q35";
 pcmc->default_cpu_version = 1;
+compat_props_add(m->compat_props, hw_compat_4_2, hw_compat_4_2_len);
 }
 
 DEFINE_Q35_MACHINE(v4_2, "pc-q35-4.2", NULL,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 94f9d27..c06a0cf 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4491,7 +4491,7 @@ static const TypeInfo spapr_machine_info = {
  */
 static void spapr_machine_4_2_class_options(MachineClass *mc)
 {
-/* Defaults for the latest behaviour inherited from the base class */
+compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
 }
 
 DEFINE_SPAPR_MACHINE(4_2, "4.2", true);
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index d3edeef..cb5fe4c 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -645,6 +645,7 @@ static void ccw_machine_4_2_instance_options(MachineState 
*machine)
 
 static void ccw_machine_4_2_class_options(MachineClass *mc)
 {
+compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
 }
 DEFINE_CCW_MACHINE(4_2, "4.2", true);
 
diff --git a/include/hw/boards.h b/include/hw/boards.h
index de45087..24cbeec 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -329,6 +329,9 @@ 

Re: [PATCH v4] qcow2-bitmap: Fix uint64_t left-shift overflow

[Max Reitz]

On 01.11.19 08:37, Tuguoyi wrote:
> There are two issues in In check_constraints_on_bitmap(),
> 1) The sanity check on the granularity will cause uint64_t
> integer left-shift overflow when cluster_size is 2M and the
> granularity is BIGGER than 32K.
> 2) The way to calculate image size that the maximum bitmap
> supported can map to is a bit incorrect.
> This patch fix it by add a helper function to calculate the
> number of bytes needed by a normal bitmap in image and compare
> it to the maximum bitmap bytes supported by qemu.
> 
> Fixes: 5f72826e7fc62167cf3a
> Signed-off-by: Guoyi Tu 
> ---
>  block/qcow2-bitmap.c | 14 +++---
>  1 file changed, 11 insertions(+), 3 deletions(-)

Thanks, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max



signature.asc
Description: OpenPGP digital signature

[PATCH v1 2/4] virtio: make seg_max virtqueue size dependent

[Denis Plotnikov]

seg_max has a restriction to be less or equal to virtqueue size
according to Virtio 1.0 specification

Although seg_max can't be set directly, it's worth to express this
dependancy directly in the code for sanity purpose.

Signed-off-by: Denis Plotnikov 
---
 hw/block/virtio-blk.c | 2 +-
 hw/scsi/virtio-scsi.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 06e57a4d39..21530304cf 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -903,7 +903,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
uint8_t *config)
 blk_get_geometry(s->blk, );
 memset(, 0, sizeof(blkcfg));
 virtio_stq_p(vdev, , capacity);
-virtio_stl_p(vdev, _max, 128 - 2);
+virtio_stl_p(vdev, _max, s->conf.queue_size - 2);
 virtio_stw_p(vdev, , conf->cyls);
 virtio_stl_p(vdev, _size, blk_size);
 virtio_stw_p(vdev, _io_size, conf->min_io_size / blk_size);
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 839f120256..f7e5533cd5 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -650,7 +650,7 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
 VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
 
 virtio_stl_p(vdev, >num_queues, s->conf.num_queues);
-virtio_stl_p(vdev, >seg_max, 128 - 2);
+virtio_stl_p(vdev, >seg_max, s->conf.virtqueue_size - 2);
 virtio_stl_p(vdev, >max_sectors, s->conf.max_sectors);
 virtio_stl_p(vdev, >cmd_per_lun, s->conf.cmd_per_lun);
 virtio_stl_p(vdev, >event_info_size, sizeof(VirtIOSCSIEvent));
-- 
2.17.0

[PATCH v1 3/4] virtio: increase virtuqueue sizes in new machine types

[Denis Plotnikov]

Linux guests submit IO requests no longer than PAGE_SIZE * max_seg
field reported by SCSI controler. Thus typical sequential read with
1 MB size results in the following pattern of the IO from the guest:
  8,16   115754 2.766095122  2071  D   R 2095104 + 1008 [dd]
  8,16   115755 2.766108785  2071  D   R 2096112 + 1008 [dd]
  8,16   115756 2.766113486  2071  D   R 2097120 + 32 [dd]
  8,16   115757 2.767668961 0  C   R 2095104 + 1008 [0]
  8,16   115758 2.768534315 0  C   R 2096112 + 1008 [0]
  8,16   115759 2.768539782 0  C   R 2097120 + 32 [0]
The IO was generated by
  dd if=/dev/sda of=/dev/null bs=1024 iflag=direct

This effectively means that on rotational disks we will observe 3 IOPS
for each 2 MBs processed. This definitely negatively affects both
guest and host IO performance.

The cure is relatively simple - we should report lengthy scatter-gather
ability of the SCSI controller. Fortunately the situation here is very
good. VirtIO transport layer can accomodate 1024 items in one request
while we are using only 128. This situation is present since almost
very beginning. 2 items are dedicated for request metadata thus we
should publish VIRTQUEUE_MAX_SIZE - 2 as max_seg.

The following pattern is observed after the patch:
  8,16   1 9921 2.662721340  2063  D   R 2095104 + 1024 [dd]
  8,16   1 9922 2.662737585  2063  D   R 2096128 + 1024 [dd]
  8,16   1 9923 2.665188167 0  C   R 2095104 + 1024 [0]
  8,16   1 9924 2.665198777 0  C   R 2096128 + 1024 [0]
which is much better.

To fix this particular case, the patch adds new machine types with
extended virtqueue sizes to 256 which also increases max_seg to 254
implicitly.

Suggested-by: Denis V. Lunev 
Signed-off-by: Denis Plotnikov 
---
 hw/core/machine.c   | 14 ++
 hw/i386/pc_piix.c   | 16 +---
 hw/i386/pc_q35.c| 14 --
 include/hw/boards.h |  6 ++
 4 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 55b08f1466..28013a0e3f 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -24,6 +24,13 @@
 #include "hw/pci/pci.h"
 #include "hw/mem/nvdimm.h"
 
+GlobalProperty hw_compat_4_0_1[] = {
+{ "virtio-blk-device", "queue-size", "128" },
+{ "virtio-scsi-device", "virtqueue_size", "128" },
+{ "vhost-scsi-device", "virtqueue_size", "128" },
+};
+const size_t hw_compat_4_0_1_len = G_N_ELEMENTS(hw_compat_4_0_1);
+
 GlobalProperty hw_compat_4_0[] = {
 { "virtio-balloon-device", "qemu-4-0-config-size", "true" },
 };
@@ -157,6 +164,13 @@ GlobalProperty hw_compat_2_1[] = {
 };
 const size_t hw_compat_2_1_len = G_N_ELEMENTS(hw_compat_2_1);
 
+GlobalProperty hw_compat[] = {
+{ "virtio-blk-device", "queue-size", "256" },
+{ "virtio-scsi-device", "virtqueue_size", "256" },
+{ "vhost-scsi-device", "virtqueue_size", "256" },
+};
+const size_t hw_compat_len = G_N_ELEMENTS(hw_compat);
+
 static char *machine_get_accel(Object *obj, Error **errp)
 {
 MachineState *ms = MACHINE(obj);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 8ad8e885c6..2260a61b1b 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -426,15 +426,27 @@ static void pc_i440fx_machine_options(MachineClass *m)
 m->default_machine_opts = "firmware=bios-256k.bin";
 m->default_display = "std";
 machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
+compat_props_add(m->compat_props, hw_compat, hw_compat_len);
 }
 
-static void pc_i440fx_4_0_machine_options(MachineClass *m)
+static void pc_i440fx_4_0_2_machine_options(MachineClass *m)
 {
 pc_i440fx_machine_options(m);
 m->alias = "pc";
 m->is_default = 1;
 }
 
+DEFINE_I440FX_MACHINE(v4_0_2, "pc-i440fx-4.0.2", NULL,
+  pc_i440fx_4_0_2_machine_options);
+
+static void pc_i440fx_4_0_machine_options(MachineClass *m)
+{
+pc_i440fx_4_0_2_machine_options(m);
+m->alias = NULL;
+m->is_default = 0;
+compat_props_add(m->compat_props, hw_compat_4_0_1, hw_compat_4_0_1_len);
+}
+
 DEFINE_I440FX_MACHINE(v4_0, "pc-i440fx-4.0", NULL,
   pc_i440fx_4_0_machine_options);
 
@@ -443,9 +455,7 @@ static void pc_i440fx_3_1_machine_options(MachineClass *m)
 PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
 
 pc_i440fx_4_0_machine_options(m);
-m->is_default = 0;
 m->smbus_no_migration_support = true;
-m->alias = NULL;
 pcmc->pvh_enabled = false;
 compat_props_add(m->compat_props, hw_compat_3_1, hw_compat_3_1_len);
 compat_props_add(m->compat_props, pc_compat_3_1, pc_compat_3_1_len);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 45cc29d1ad..50ccd9ebcf 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -363,14 +363,25 @@ static void pc_q35_machine_options(MachineClass *m)
 machine_class_allow_dynamic_sysbus_dev(m, TYPE_INTEL_IOMMU_DEVICE);
 machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
 

[PATCH v1 0/4] virtio: fix IO request length in virtio SCSI/block

[Denis Plotnikov]

v1:
  * make seg_max size dependent on virtuqueue size
  * don't expose seg_max as property
  * add new machine types with increased queue size
  * add test to check the new machine types
  * check queue size for non-modern virtio devices
---
From: "Denis V. Lunev" 

Linux guests submit IO requests no longer than PAGE_SIZE * max_seg
field reported by SCSI controler. Thus typical sequential read with
1 MB size results in the following pattern of the IO from the guest:
  8,16   115754 2.766095122  2071  D   R 2095104 + 1008 [dd]
  8,16   115755 2.766108785  2071  D   R 2096112 + 1008 [dd]
  8,16   115756 2.766113486  2071  D   R 2097120 + 32 [dd]
  8,16   115757 2.767668961 0  C   R 2095104 + 1008 [0]
  8,16   115758 2.768534315 0  C   R 2096112 + 1008 [0]
  8,16   115759 2.768539782 0  C   R 2097120 + 32 [0]
The IO was generated by
  dd if=/dev/sda of=/dev/null bs=1024 iflag=direct

This effectively means that on rotational disks we will observe 3 IOPS
for each 2 MBs processed. This definitely negatively affects both
guest and host IO performance.

The cure is relatively simple - we should report lengthy scatter-gather
ability of the SCSI controller. Fortunately the situation here is very
good. VirtIO transport layer can accomodate 1024 items in one request
while we are using only 128. This situation is present since almost
very beginning. 2 items are dedicated for request metadata thus we
should publish VIRTQUEUE_MAX_SIZE - 2 as max_seg.

The following pattern is observed after the patch:
  8,16   1 9921 2.662721340  2063  D   R 2095104 + 1024 [dd]
  8,16   1 9922 2.662737585  2063  D   R 2096128 + 1024 [dd]
  8,16   1 9923 2.665188167 0  C   R 2095104 + 1024 [0]
  8,16   1 9924 2.665198777 0  C   R 2096128 + 1024 [0]
which is much better.

The dark side of this patch is that we are tweaking guest visible
parameter, though this should be relatively safe as above transport
layer support is present in QEMU/host Linux for a very long time.
The patch adds configurable property for VirtIO SCSI with a new default
and hardcode option for VirtBlock which does not provide good
configurable framework.

Unfortunately the commit can not be applied as is. For the real cure we
need guest to be fixed to accomodate that queue length, which is done
only in the latest 4.14 kernel. Thus we are going to expose the property
and tweak it on machine type level.

The problem with the old kernels is that they have
max_segments <= virtqueue_size restriction which cause the guest
crashing in the case of violation.
To fix the case described above in the old kernels we can increase
virtqueue_size to 256 and max_segments to 254. The pitfall here is
that seabios allows the virtqueue_size-s < 128, however, the seabios
patch extending that value to 256 is pending.

Denis Plotnikov (4):
  virtio: protect non-modern devices from too big virtqueue size setting
  virtio: make seg_max virtqueue size dependent
  virtio: increase virtuqueue sizes in new machine types
  iotests: add test for virtio-scsi and virtio-blk machine type settings

 hw/block/virtio-blk.c   |   2 +-
 hw/core/machine.c   |  14 
 hw/i386/pc_piix.c   |  16 +++-
 hw/i386/pc_q35.c|  14 +++-
 hw/scsi/virtio-scsi.c   |   2 +-
 hw/virtio/virtio-blk-pci.c  |   9 +++
 hw/virtio/virtio-scsi-pci.c |  10 +++
 include/hw/boards.h |   6 ++
 tests/qemu-iotests/267  | 154 
 tests/qemu-iotests/267.out  |   1 +
 tests/qemu-iotests/group|   1 +
 11 files changed, 222 insertions(+), 7 deletions(-)
 create mode 100755 tests/qemu-iotests/267
 create mode 100644 tests/qemu-iotests/267.out

-- 
2.17.0

[PATCH v1 4/4] iotests: add test for virtio-scsi and virtio-blk machine type settings

[Denis Plotnikov]

It tests proper queue size settings for all available machine types.

Signed-off-by: Denis Plotnikov 
---
 tests/qemu-iotests/267 | 154 +
 tests/qemu-iotests/267.out |   1 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 156 insertions(+)
 create mode 100755 tests/qemu-iotests/267
 create mode 100644 tests/qemu-iotests/267.out

diff --git a/tests/qemu-iotests/267 b/tests/qemu-iotests/267
new file mode 100755
index 00..6d3cc574b3
--- /dev/null
+++ b/tests/qemu-iotests/267
@@ -0,0 +1,154 @@
+#!/usr/bin/env python
+#
+# Test virtio-scsi and virtio-blk queue settings for all machine types
+#
+# Copyright (c) 2019 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import sys
+import os
+import re
+import iotests
+
+sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts'))
+import qemu
+
+# list of device types and virtqueue properties to test
+# this is a generalized approach
+# for now we just check queue length
+# more properties can be added in each list
+virtio_scsi_props = {'vq_size': 'virtqueue_size'}
+virtio_blk_props = {'vq_size': 'queue-size'}
+
+dev_types = {'virtio-scsi-pci': virtio_scsi_props,
+ 'virtio-blk-pci': virtio_blk_props}
+
+vm_dev_params = {'virtio-scsi-pci': ['-device', 'virtio-scsi-pci,id=scsi0'],
+ 'virtio-blk-pci': ['-device',
+'virtio-blk-pci,id=scsi0,drive=drive0',
+'-drive',
+'driver=null-co,id=drive0,if=none']}
+
+def make_pattern(props):
+ pattern_items = ['{0} = \d+'.format(prop) for prop in props]
+ return '|'.join(pattern_items)
+
+
+def query_virtqueue_props(vm, dev_type_name):
+output = vm.qmp('human-monitor-command', command_line='info qtree')
+output = output['return']
+
+props_list = dev_types[dev_type_name].values();
+
+pattern = make_pattern(props_list)
+
+res = re.findall(pattern, output)
+
+if len(res) != len(props_list):
+not_found = props_list.difference(set(res))
+
+ret = (0, '({0}): The following properties not found: {1}'
+  .format(dev_type_name, ', '.join(not_found)))
+else:
+props = dict()
+for prop in res:
+p = prop.split(' = ')
+props[p[0]] = int(p[1])
+ret = (1, props)
+
+return ret
+
+
+def check_mt(mt, dev_type_name):
+vm_params = ['-machine', mt['name']] + vm_dev_params[dev_type_name]
+
+vm = qemu.QEMUMachine(iotests.qemu_prog, vm_params)
+vm.launch()
+ret = query_virtqueue_props(vm, dev_type_name)
+vm.shutdown()
+
+if ret[0] == 0:
+print('Error ({0}): {1}'.format(mt['name'], ret[1]))
+return 1
+
+errors = 0
+props = ret[1]
+
+for prop_name, prop_val in props.items():
+if mt[prop_name] != prop_val:
+print('Error [{0}, {1}]: {2}={3} (expected {4})'.
+  format(mt['name'], dev_type_name, prop_name, prop_val,
+ mt[prop_name]))
+errors += 1
+
+return errors
+
+def is_256_virtqueue_size_mt(mt):
+mt = mt.split("-")
+
+# machine types like pc-x.x
+if len(mt) == 2:
+return False
+
+# machine types like pc--x.x[.x]
+ver = mt[2]
+
+ver = ver.split(".");
+
+# all versions greater than 4.0.1 goes with 256 queue size
+if int(ver[0]) >= 4:
+major = int(ver[1])
+minor = 0
+if len(ver) > 2:
+minor = int(ver[2])
+
+if major > 0 or minor > 1:
+ return True
+
+return False
+
+
+# collect all machine types except 'none'
+vm = iotests.VM()
+vm.launch()
+machines = [m['name'] for m in vm.qmp('query-machines')['return']]
+vm.shutdown()
+machines.remove('none')
+machines.remove('isapc')
+
+failed = 0
+
+for dev_type in dev_types:
+# create a list of machine types and their parameters
+# machine types vz8.X.X must have virtqueue_length=256
+# others must have virtqueue_length=128
+mtypes = list()
+for m in machines:
+if is_256_virtqueue_size_mt(m):
+vq_size = 256
+else:
+vq_size = 128
+
+mtypes.append({'name': m, dev_types[dev_type]['vq_size']: vq_size})
+
+# test each machine type
+for mt in mtypes:
+failed += check_mt(mt, 

[PATCH v1 1/4] virtio: protect non-modern devices from too big virtqueue size setting

[Denis Plotnikov]

The patch protects from creating illegal virtio device configuration
via direct virtqueue size property setting.

Signed-off-by: Denis Plotnikov 
---
 hw/virtio/virtio-blk-pci.c  |  9 +
 hw/virtio/virtio-scsi-pci.c | 10 ++
 2 files changed, 19 insertions(+)

diff --git a/hw/virtio/virtio-blk-pci.c b/hw/virtio/virtio-blk-pci.c
index 60c9185c39..6177ff1df8 100644
--- a/hw/virtio/virtio-blk-pci.c
+++ b/hw/virtio/virtio-blk-pci.c
@@ -48,6 +48,15 @@ static void virtio_blk_pci_realize(VirtIOPCIProxy *vpci_dev, 
Error **errp)
 {
 VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(vpci_dev);
 DeviceState *vdev = DEVICE(>vdev);
+bool modern = virtio_pci_modern(vpci_dev);
+uint32_t queue_size = dev->vdev.conf.queue_size;
+
+if (!modern && queue_size > 128) {
+error_setg(errp,
+   "too big queue size (%u, max: 128) "
+   "for non-modern virtio device", queue_size);
+return;
+}
 
 if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
 vpci_dev->nvectors = dev->vdev.conf.num_queues + 1;
diff --git a/hw/virtio/virtio-scsi-pci.c b/hw/virtio/virtio-scsi-pci.c
index 2830849729..6e6790fda5 100644
--- a/hw/virtio/virtio-scsi-pci.c
+++ b/hw/virtio/virtio-scsi-pci.c
@@ -17,6 +17,7 @@
 
 #include "hw/virtio/virtio-scsi.h"
 #include "virtio-pci.h"
+#include "qapi/error.h"
 
 typedef struct VirtIOSCSIPCI VirtIOSCSIPCI;
 
@@ -47,6 +48,15 @@ static void virtio_scsi_pci_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
 VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
 DeviceState *proxy = DEVICE(vpci_dev);
 char *bus_name;
+bool modern = virtio_pci_modern(vpci_dev);
+uint32_t virtqueue_size = vs->conf.virtqueue_size;
+
+if (!modern && virtqueue_size > 128) {
+error_setg(errp,
+   "too big virtqueue size (%u, max: 128) "
+   "for non-modern virtio device", virtqueue_size);
+return;
+}
 
 if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
 vpci_dev->nvectors = vs->conf.num_queues + 3;
-- 
2.17.0

Re: [RFC PATCH 00/18] Add qemu-storage-daemon

[Stefan Hajnoczi]

On Thu, Oct 17, 2019 at 03:01:46PM +0200, Kevin Wolf wrote:
> This series adds a new tool 'qemu-storage-daemon', which can be used to
> export and perform operations on block devices. There is some overlap
> between qemu-img/qemu-nbd and the new qemu-storage-daemon, but there are
> a few important differences:
> 
> * The qemu-storage-daemon has QMP support. The command set is obviously
>   restricted compared to the system emulator because there is no guest,
>   but all of the block operations are present.
> 
>   This means that it can access advanced options or operations that the
>   qemu-img command line doesn't expose. For example, blockdev-create is
>   a lot more powerful than 'qemu-img create', and qemu-storage-daemon
>   allows to execute it without starting a guest.
> 
>   Compared to qemu-nbd it means that, for example, block jobs can now be
>   executed on the server side, and backing chains shared by multiple VMs
>   can be modified this way.
> 
> * The existing tools all have a separately invented one-off syntax for
>   the job at hand, which usually comes with restrictions compared to the
>   system emulator. qemu-storage-daemon shares the same syntax with the
>   system emulator for most options and prefers QAPI based interfaces
>   where possible (such as --blockdev), so it should be easy to make use
>   of in libvirt.
> 
> * While this series implements only NBD exports, the storage daemon is
>   intended to serve multiple protocols and its syntax reflects this. In
>   the past, we had proposals to add new one-off tools for exporting over
>   new protocols like FUSE or TCMU.
> 
>   With a generic storage daemon, additional export methods have a home
>   without adding a new tool for each of them.

No comments on the command-line, QAPI, or monitor aspects, but the
general idea of having a qemu-storage-daemon is likely to be useful.

Acked-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

[PULL 10/11] image-fuzzer: Use errors parameter of subprocess.Popen()

[Stefan Hajnoczi]

From: Eduardo Habkost 

Instead of manually encoding stderr and stdout output, use
`errors` parameter of subprocess.Popen().  This will make
process.communicate() return unicode strings instead of bytes
objects.

Signed-off-by: Eduardo Habkost 
Reviewed-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20191016192430.25098-11-ehabk...@redhat.com
Message-Id: <20191016192430.25098-11-ehabk...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 tests/image-fuzzer/runner.py | 11 ---
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/tests/image-fuzzer/runner.py b/tests/image-fuzzer/runner.py
index 0793234815..4ba5c79e13 100755
--- a/tests/image-fuzzer/runner.py
+++ b/tests/image-fuzzer/runner.py
@@ -79,16 +79,13 @@ def run_app(fd, q_args):
 devnull = open('/dev/null', 'r+')
 process = subprocess.Popen(q_args, stdin=devnull,
stdout=subprocess.PIPE,
-   stderr=subprocess.PIPE)
+   stderr=subprocess.PIPE,
+   errors='replace')
 try:
 out, err = process.communicate()
 signal.alarm(0)
-# fd is a text file, so we need to decode the process output before
-# writing to it.
-# We could be simply using the `errors` parameter of 
subprocess.Popen(),
-# but this will be possible only after migrating to Python 3
-fd.write(out.decode(errors='replace'))
-fd.write(err.decode(errors='replace'))
+fd.write(out)
+fd.write(err)
 fd.flush()
 return process.returncode
 
-- 
2.23.0

[PULL 07/11] image-fuzzer: Use bytes constant for field values

[Stefan Hajnoczi]

From: Eduardo Habkost 

Field values are supposed to be bytes objects, not unicode
strings.  Change two constants that were declared as strings.

Signed-off-by: Eduardo Habkost 
Reviewed-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20191016192430.25098-8-ehabk...@redhat.com
Message-Id: <20191016192430.25098-8-ehabk...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 tests/image-fuzzer/qcow2/layout.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/image-fuzzer/qcow2/layout.py 
b/tests/image-fuzzer/qcow2/layout.py
index 0adcbd448d..a0fd53c7ad 100644
--- a/tests/image-fuzzer/qcow2/layout.py
+++ b/tests/image-fuzzer/qcow2/layout.py
@@ -122,7 +122,7 @@ class Image(object):
 def create_header(self, cluster_bits, backing_file_name=None):
 """Generate a random valid header."""
 meta_header = [
-['>4s', 0, "QFI\xfb", 'magic'],
+['>4s', 0, b"QFI\xfb", 'magic'],
 ['>I', 4, random.randint(2, 3), 'version'],
 ['>Q', 8, 0, 'backing_file_offset'],
 ['>I', 16, 0, 'backing_file_size'],
@@ -231,7 +231,7 @@ class Image(object):
 feature_tables = []
 feature_ids = []
 inner_offset = self.ext_offset + ext_header_len
-feat_name = 'some cool feature'
+feat_name = b'some cool feature'
 while len(feature_tables) < num_fnt_entries * 3:
 feat_type, feat_bit = gen_feat_ids()
 # Remove duplicates
-- 
2.23.0

[PULL 08/11] image-fuzzer: Encode file name and file format to bytes

[Stefan Hajnoczi]

From: Eduardo Habkost 

Callers of create_image() will pass strings as arguments, but the
Image class will expect bytes objects to be provided.  Encode
them inside create_image().

Signed-off-by: Eduardo Habkost 
Reviewed-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20191016192430.25098-9-ehabk...@redhat.com
Message-Id: <20191016192430.25098-9-ehabk...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 tests/image-fuzzer/qcow2/layout.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/image-fuzzer/qcow2/layout.py 
b/tests/image-fuzzer/qcow2/layout.py
index a0fd53c7ad..01bff4d05e 100644
--- a/tests/image-fuzzer/qcow2/layout.py
+++ b/tests/image-fuzzer/qcow2/layout.py
@@ -602,8 +602,8 @@ class Image(object):
 def create_image(test_img_path, backing_file_name=None, backing_file_fmt=None,
  fields_to_fuzz=None):
 """Create a fuzzed image and write it to the specified file."""
-image = Image(backing_file_name)
-image.set_backing_file_format(backing_file_fmt)
+image = Image(backing_file_name.encode())
+image.set_backing_file_format(backing_file_fmt.encode())
 image.create_feature_name_table()
 image.set_end_of_extension_area()
 image.create_l_structures()
-- 
2.23.0

[PULL 03/11] image-fuzzer: Explicitly use integer division operator

[Stefan Hajnoczi]

From: Eduardo Habkost 

Most of the division expressions in image-fuzzer assume integer
division.  Use the // operator to keep the same behavior when we
move to Python 3.

Signed-off-by: Eduardo Habkost 
Reviewed-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20191016192430.25098-4-ehabk...@redhat.com
Message-Id: <20191016192430.25098-4-ehabk...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 tests/image-fuzzer/qcow2/fuzz.py   | 12 -
 tests/image-fuzzer/qcow2/layout.py | 40 +++---
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/tests/image-fuzzer/qcow2/fuzz.py b/tests/image-fuzzer/qcow2/fuzz.py
index abc4f0635d..154dc06cc0 100644
--- a/tests/image-fuzzer/qcow2/fuzz.py
+++ b/tests/image-fuzzer/qcow2/fuzz.py
@@ -27,14 +27,14 @@ UINT64 = 0x
 UINT32_M = 31
 UINT64_M = 63
 # Fuzz vectors
-UINT8_V = [0, 0x10, UINT8/4, UINT8/2 - 1, UINT8/2, UINT8/2 + 1, UINT8 - 1,
+UINT8_V = [0, 0x10, UINT8//4, UINT8//2 - 1, UINT8//2, UINT8//2 + 1, UINT8 - 1,
UINT8]
-UINT16_V = [0, 0x100, 0x1000, UINT16/4, UINT16/2 - 1, UINT16/2, UINT16/2 + 1,
+UINT16_V = [0, 0x100, 0x1000, UINT16//4, UINT16//2 - 1, UINT16//2, UINT16//2 + 
1,
 UINT16 - 1, UINT16]
-UINT32_V = [0, 0x100, 0x1000, 0x1, 0x10, UINT32/4, UINT32/2 - 1,
-UINT32/2, UINT32/2 + 1, UINT32 - 1, UINT32]
-UINT64_V = UINT32_V + [0x100, 0x1000, 0x1, UINT64/4,
-   UINT64/2 - 1, UINT64/2, UINT64/2 + 1, UINT64 - 1,
+UINT32_V = [0, 0x100, 0x1000, 0x1, 0x10, UINT32//4, UINT32//2 - 1,
+UINT32//2, UINT32//2 + 1, UINT32 - 1, UINT32]
+UINT64_V = UINT32_V + [0x100, 0x1000, 0x1, UINT64//4,
+   UINT64//2 - 1, UINT64//2, UINT64//2 + 1, UINT64 - 1,
UINT64]
 STRING_V = ['%s%p%x%d', '.1024d', '%.2049d', '%p%p%p%p', '%x%x%x%x',
 '%d%d%d%d', '%s%s%s%s', '%999s', '%08x', '%%20d', '%%20n',
diff --git a/tests/image-fuzzer/qcow2/layout.py 
b/tests/image-fuzzer/qcow2/layout.py
index fe273d4143..6501c9fd4b 100644
--- a/tests/image-fuzzer/qcow2/layout.py
+++ b/tests/image-fuzzer/qcow2/layout.py
@@ -253,7 +253,7 @@ class Image(object):
 ['>I', self.ext_offset, 0x6803f857, 'ext_magic'],
 # One feature table contains 3 fields and takes 48 bytes
 ['>I', self.ext_offset + UINT32_S,
- len(feature_tables) / 3 * 48, 'ext_length']
+ len(feature_tables) // 3 * 48, 'ext_length']
 ] + feature_tables)
 self.ext_offset = inner_offset
 
@@ -271,7 +271,7 @@ class Image(object):
 def create_l2_entry(host, guest, l2_cluster):
 """Generate one L2 entry."""
 offset = l2_cluster * self.cluster_size
-l2_size = self.cluster_size / UINT64_S
+l2_size = self.cluster_size // UINT64_S
 entry_offset = offset + UINT64_S * (guest % l2_size)
 cluster_descriptor = host * self.cluster_size
 if not self.header['version'][0].value == 2:
@@ -283,8 +283,8 @@ class Image(object):
 
 def create_l1_entry(l2_cluster, l1_offset, guest):
 """Generate one L1 entry."""
-l2_size = self.cluster_size / UINT64_S
-entry_offset = l1_offset + UINT64_S * (guest / l2_size)
+l2_size = self.cluster_size // UINT64_S
+entry_offset = l1_offset + UINT64_S * (guest // l2_size)
 # While snapshots are not supported bit #63 = 1
 entry_val = (1 << 63) + l2_cluster * self.cluster_size
 return ['>Q', entry_offset, entry_val, 'l1_entry']
@@ -298,11 +298,11 @@ class Image(object):
 l2 = []
 else:
 meta_data = self._get_metadata()
-guest_clusters = random.sample(range(self.image_size /
+guest_clusters = random.sample(range(self.image_size //
  self.cluster_size),
len(self.data_clusters))
 # Number of entries in a L1/L2 table
-l_size = self.cluster_size / UINT64_S
+l_size = self.cluster_size // UINT64_S
 # Number of clusters necessary for L1 table
 l1_size = int(ceil((max(guest_clusters) + 1) / float(l_size**2)))
 l1_start = self._get_adjacent_clusters(self.data_clusters |
@@ -318,7 +318,7 @@ class Image(object):
 # L2 entries
 l2 = []
 for host, guest in zip(self.data_clusters, guest_clusters):
-l2_id = guest / l_size
+l2_id = guest // l_size
 if l2_id not in l2_ids:
 l2_ids.append(l2_id)
 l2_clusters.append(self._get_adjacent_clusters(
@@ -339,14 +339,14 @@ class Image(object):
 def allocate_rfc_blocks(data, size):
 """Return indices of clusters allocated for 

[PULL 09/11] image-fuzzer: Run using python3

[Stefan Hajnoczi]

From: Eduardo Habkost 

image-fuzzer is now supposed to be ready to run using Python 3.
Remove the __future__ imports and change the interpreter line to
"#!/usr/bin/env python3".

Signed-off-by: Eduardo Habkost 
Reviewed-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20191016192430.25098-10-ehabk...@redhat.com
Message-Id: <20191016192430.25098-10-ehabk...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 tests/image-fuzzer/qcow2/__init__.py | 1 -
 tests/image-fuzzer/qcow2/layout.py   | 1 -
 tests/image-fuzzer/runner.py | 3 +--
 3 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/tests/image-fuzzer/qcow2/__init__.py 
b/tests/image-fuzzer/qcow2/__init__.py
index 09ef59821b..ed3af5da86 100644
--- a/tests/image-fuzzer/qcow2/__init__.py
+++ b/tests/image-fuzzer/qcow2/__init__.py
@@ -1,2 +1 @@
-from __future__ import absolute_import
 from .layout import create_image
diff --git a/tests/image-fuzzer/qcow2/layout.py 
b/tests/image-fuzzer/qcow2/layout.py
index 01bff4d05e..57ebe86e9a 100644
--- a/tests/image-fuzzer/qcow2/layout.py
+++ b/tests/image-fuzzer/qcow2/layout.py
@@ -16,7 +16,6 @@
 # along with this program.  If not, see .
 #
 
-from __future__ import absolute_import
 import random
 import struct
 from . import fuzz
diff --git a/tests/image-fuzzer/runner.py b/tests/image-fuzzer/runner.py
index 94cab5bd93..0793234815 100755
--- a/tests/image-fuzzer/runner.py
+++ b/tests/image-fuzzer/runner.py
@@ -1,4 +1,4 @@
-#!/usr/bin/env python
+#!/usr/bin/env python3
 
 # Tool for running fuzz tests
 #
@@ -18,7 +18,6 @@
 # along with this program.  If not, see .
 #
 
-from __future__ import print_function
 import sys
 import os
 import signal
-- 
2.23.0

[PULL 06/11] image-fuzzer: Return bytes objects on string fuzzing functions

[Stefan Hajnoczi]

From: Eduardo Habkost 

No caller of fuzzer functions is interested in unicode string values,
so replace them with bytes sequences.

Signed-off-by: Eduardo Habkost 
Reviewed-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20191016192430.25098-7-ehabk...@redhat.com
Message-Id: <20191016192430.25098-7-ehabk...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 tests/image-fuzzer/qcow2/fuzz.py | 42 
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/tests/image-fuzzer/qcow2/fuzz.py b/tests/image-fuzzer/qcow2/fuzz.py
index 154dc06cc0..c58bf11005 100644
--- a/tests/image-fuzzer/qcow2/fuzz.py
+++ b/tests/image-fuzzer/qcow2/fuzz.py
@@ -36,11 +36,11 @@ UINT32_V = [0, 0x100, 0x1000, 0x1, 0x10, UINT32//4, 
UINT32//2 - 1,
 UINT64_V = UINT32_V + [0x100, 0x1000, 0x1, UINT64//4,
UINT64//2 - 1, UINT64//2, UINT64//2 + 1, UINT64 - 1,
UINT64]
-STRING_V = ['%s%p%x%d', '.1024d', '%.2049d', '%p%p%p%p', '%x%x%x%x',
-'%d%d%d%d', '%s%s%s%s', '%999s', '%08x', '%%20d', '%%20n',
-'%%20x', '%%20s', '%s%s%s%s%s%s%s%s%s%s', '%p%p%p%p%p%p%p%p%p%p',
-'%#0123456x%08x%x%s%p%d%n%o%u%c%h%l%q%j%z%Z%t%i%e%g%f%a%C%S%08x%%',
-'%s x 129', '%x x 257']
+BYTES_V = [b'%s%p%x%d', b'.1024d', b'%.2049d', b'%p%p%p%p', b'%x%x%x%x',
+   b'%d%d%d%d', b'%s%s%s%s', b'%999s', b'%08x', b'%%20d', 
b'%%20n',
+   b'%%20x', b'%%20s', b'%s%s%s%s%s%s%s%s%s%s', 
b'%p%p%p%p%p%p%p%p%p%p',
+   b'%#0123456x%08x%x%s%p%d%n%o%u%c%h%l%q%j%z%Z%t%i%e%g%f%a%C%S%08x%%',
+   b'%s x 129', b'%x x 257']
 
 
 def random_from_intervals(intervals):
@@ -76,12 +76,12 @@ def random_bits(bit_ranges):
 return val
 
 
-def truncate_string(strings, length):
-"""Return strings truncated to specified length."""
-if type(strings) == list:
-return [s[:length] for s in strings]
+def truncate_bytes(sequences, length):
+"""Return sequences truncated to specified length."""
+if type(sequences) == list:
+return [s[:length] for s in sequences]
 else:
-return strings[:length]
+return sequences[:length]
 
 
 def validator(current, pick, choices):
@@ -110,12 +110,12 @@ def bit_validator(current, bit_ranges):
 return validator(current, random_bits, bit_ranges)
 
 
-def string_validator(current, strings):
-"""Return a random string value from the list not equal to the current.
+def bytes_validator(current, sequences):
+"""Return a random bytes value from the list not equal to the current.
 
 This function is useful for selection from valid values except current one.
 """
-return validator(current, random.choice, strings)
+return validator(current, random.choice, sequences)
 
 
 def selector(current, constraints, validate=int_validator):
@@ -283,9 +283,9 @@ def header_length(current):
 def bf_name(current):
 """Fuzz the backing file name."""
 constraints = [
-truncate_string(STRING_V, len(current))
+truncate_bytes(BYTES_V, len(current))
 ]
-return selector(current, constraints, string_validator)
+return selector(current, constraints, bytes_validator)
 
 
 def ext_magic(current):
@@ -303,10 +303,10 @@ def ext_length(current):
 def bf_format(current):
 """Fuzz backing file format in the corresponding header extension."""
 constraints = [
-truncate_string(STRING_V, len(current)),
-truncate_string(STRING_V, (len(current) + 7) & ~7)  # Fuzz padding
+truncate_bytes(BYTES_V, len(current)),
+truncate_bytes(BYTES_V, (len(current) + 7) & ~7)  # Fuzz padding
 ]
-return selector(current, constraints, string_validator)
+return selector(current, constraints, bytes_validator)
 
 
 def feature_type(current):
@@ -324,10 +324,10 @@ def feature_bit_number(current):
 def feature_name(current):
 """Fuzz feature name field of a feature name table header extension."""
 constraints = [
-truncate_string(STRING_V, len(current)),
-truncate_string(STRING_V, 46)  # Fuzz padding (field length = 46)
+truncate_bytes(BYTES_V, len(current)),
+truncate_bytes(BYTES_V, 46)  # Fuzz padding (field length = 46)
 ]
-return selector(current, constraints, string_validator)
+return selector(current, constraints, bytes_validator)
 
 
 def l1_entry(current):
-- 
2.23.0

[PULL 11/11] image-fuzzer: Use OSerror.strerror instead of tuple subscript

[Stefan Hajnoczi]

From: Eduardo Habkost 

OSError can't be used like a tuple on Python 3, so change the
code to use `e.sterror` instead of `e[1]`.

Reported-by: John Snow 
Signed-off-by: Eduardo Habkost 
Reviewed-by: John Snow 
Message-id: 20191021214117.18091-1-ehabk...@redhat.com
Message-Id: <20191021214117.18091-1-ehabk...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 tests/image-fuzzer/runner.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/image-fuzzer/runner.py b/tests/image-fuzzer/runner.py
index 4ba5c79e13..2fc010fd9d 100755
--- a/tests/image-fuzzer/runner.py
+++ b/tests/image-fuzzer/runner.py
@@ -159,7 +159,7 @@ class TestEnv(object):
 os.makedirs(self.current_dir)
 except OSError as e:
 print("Error: The working directory '%s' cannot be used. Reason: 
%s"\
-% (self.work_dir, e[1]), file=sys.stderr)
+% (self.work_dir, e.strerror), file=sys.stderr)
 raise TestException
 self.log = open(os.path.join(self.current_dir, "test.log"), "w")
 self.parent_log = open(run_log, "a")
@@ -246,7 +246,7 @@ class TestEnv(object):
 except OSError as e:
 multilog("%sError: Start of '%s' failed. Reason: %s\n\n"
  % (test_summary, os.path.basename(current_cmd[0]),
-e[1]),
+e.strerror),
  sys.stderr, self.log, self.parent_log)
 raise TestException
 
-- 
2.23.0

[PULL 04/11] image-fuzzer: Use io.StringIO

[Stefan Hajnoczi]

From: Eduardo Habkost 

StringIO.StringIO is not available on Python 3, but io.StringIO
is available on both Python 2 and 3.  io.StringIO is slightly
different from the Python 2 StringIO module, though, so we need
bytes coming from subprocess.Popen() to be explicitly decoded.

Signed-off-by: Eduardo Habkost 
Reviewed-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20191016192430.25098-5-ehabk...@redhat.com
Message-Id: <20191016192430.25098-5-ehabk...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 tests/image-fuzzer/runner.py | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tests/image-fuzzer/runner.py b/tests/image-fuzzer/runner.py
index 95d84f38f3..94cab5bd93 100755
--- a/tests/image-fuzzer/runner.py
+++ b/tests/image-fuzzer/runner.py
@@ -28,7 +28,7 @@ import shutil
 from itertools import count
 import time
 import getopt
-import StringIO
+import io
 import resource
 
 try:
@@ -84,8 +84,12 @@ def run_app(fd, q_args):
 try:
 out, err = process.communicate()
 signal.alarm(0)
-fd.write(out)
-fd.write(err)
+# fd is a text file, so we need to decode the process output before
+# writing to it.
+# We could be simply using the `errors` parameter of 
subprocess.Popen(),
+# but this will be possible only after migrating to Python 3
+fd.write(out.decode(errors='replace'))
+fd.write(err.decode(errors='replace'))
 fd.flush()
 return process.returncode
 
@@ -183,7 +187,7 @@ class TestEnv(object):
MAX_BACKING_FILE_SIZE) * (1 << 20)
 cmd = self.qemu_img + ['create', '-f', backing_file_fmt,
backing_file_name, str(backing_file_size)]
-temp_log = StringIO.StringIO()
+temp_log = io.StringIO()
 retcode = run_app(temp_log, cmd)
 if retcode == 0:
 temp_log.close()
@@ -240,7 +244,7 @@ class TestEnv(object):
"Backing file: %s\n" \
% (self.seed, " ".join(current_cmd),
   self.current_dir, backing_file_name)
-temp_log = StringIO.StringIO()
+temp_log = io.StringIO()
 try:
 retcode = run_app(temp_log, current_cmd)
 except OSError as e:
-- 
2.23.0

[PULL 02/11] image-fuzzer: Write bytes instead of string to image file

[Stefan Hajnoczi]

From: Eduardo Habkost 

This is necessary for Python 3 compatibility.

Signed-off-by: Eduardo Habkost 
Reviewed-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20191016192430.25098-3-ehabk...@redhat.com
Message-Id: <20191016192430.25098-3-ehabk...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 tests/image-fuzzer/qcow2/layout.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/image-fuzzer/qcow2/layout.py 
b/tests/image-fuzzer/qcow2/layout.py
index c57418fa15..fe273d4143 100644
--- a/tests/image-fuzzer/qcow2/layout.py
+++ b/tests/image-fuzzer/qcow2/layout.py
@@ -518,7 +518,7 @@ class Image(object):
 rounded = (size + self.cluster_size - 1) & ~(self.cluster_size - 1)
 if rounded > size:
 image_file.seek(rounded - 1)
-image_file.write("\0")
+image_file.write(b'\x00')
 image_file.close()
 
 @staticmethod
-- 
2.23.0

[PULL 00/11] Block patches

[Stefan Hajnoczi]

The following changes since commit 36609b4fa36f0ac934874371874416f7533a5408:

  Merge remote-tracking branch 'remotes/palmer/tags/palmer-for-master-4.2-sf1' 
into staging (2019-11-02 17:59:03 +)

are available in the Git repository at:

  https://github.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to 9fdd7860adec188ed50d2530e9a819e8d953f9bb:

  image-fuzzer: Use OSerror.strerror instead of tuple subscript (2019-11-05 
16:36:11 +0100)


Pull request

Let's get the image fuzzer Python 3 changes merged in QEMU 4.2.



Eduardo Habkost (11):
  image-fuzzer: Open image files in binary mode
  image-fuzzer: Write bytes instead of string to image file
  image-fuzzer: Explicitly use integer division operator
  image-fuzzer: Use io.StringIO
  image-fuzzer: Use %r for all fiels at Field.__repr__()
  image-fuzzer: Return bytes objects on string fuzzing functions
  image-fuzzer: Use bytes constant for field values
  image-fuzzer: Encode file name and file format to bytes
  image-fuzzer: Run using python3
  image-fuzzer: Use errors parameter of subprocess.Popen()
  image-fuzzer: Use OSerror.strerror instead of tuple subscript

 tests/image-fuzzer/qcow2/__init__.py |  1 -
 tests/image-fuzzer/qcow2/fuzz.py | 54 +-
 tests/image-fuzzer/qcow2/layout.py   | 57 ++--
 tests/image-fuzzer/runner.py | 16 
 4 files changed, 63 insertions(+), 65 deletions(-)

-- 
2.23.0

[PULL 01/11] image-fuzzer: Open image files in binary mode

[Stefan Hajnoczi]

From: Eduardo Habkost 

This probably never caused problems because on Linux there's no
actual newline conversion happening, but on Python 3 the
binary/text distinction is stronger and we must explicitly open
the image file in binary mode.

Signed-off-by: Eduardo Habkost 
Reviewed-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20191016192430.25098-2-ehabk...@redhat.com
Message-Id: <20191016192430.25098-2-ehabk...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 tests/image-fuzzer/qcow2/layout.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/image-fuzzer/qcow2/layout.py 
b/tests/image-fuzzer/qcow2/layout.py
index 675877da96..c57418fa15 100644
--- a/tests/image-fuzzer/qcow2/layout.py
+++ b/tests/image-fuzzer/qcow2/layout.py
@@ -503,7 +503,7 @@ class Image(object):
 
 def write(self, filename):
 """Write an entire image to the file."""
-image_file = open(filename, 'w')
+image_file = open(filename, 'wb')
 for field in self:
 image_file.seek(field.offset)
 image_file.write(struct.pack(field.fmt, field.value))
-- 
2.23.0

[PULL 05/11] image-fuzzer: Use %r for all fiels at Field.__repr__()

[Stefan Hajnoczi]

From: Eduardo Habkost 

This makes the formatting code simpler, and safer if we change
the type of self.value from str to bytes.

Signed-off-by: Eduardo Habkost 
Reviewed-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20191016192430.25098-6-ehabk...@redhat.com
Message-Id: <20191016192430.25098-6-ehabk...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 tests/image-fuzzer/qcow2/layout.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/image-fuzzer/qcow2/layout.py 
b/tests/image-fuzzer/qcow2/layout.py
index 6501c9fd4b..0adcbd448d 100644
--- a/tests/image-fuzzer/qcow2/layout.py
+++ b/tests/image-fuzzer/qcow2/layout.py
@@ -53,8 +53,8 @@ class Field(object):
 return iter([self.fmt, self.offset, self.value, self.name])
 
 def __repr__(self):
-return "Field(fmt='%s', offset=%d, value=%s, name=%s)" % \
-(self.fmt, self.offset, str(self.value), self.name)
+return "Field(fmt=%r, offset=%r, value=%r, name=%r)" % \
+(self.fmt, self.offset, self.value, self.name)
 
 
 class FieldsList(object):
-- 
2.23.0

Re: [PATCH] image-fuzzer: Use OSerror.strerror instead of tuple subscript

[Stefan Hajnoczi]

On Mon, Oct 21, 2019 at 06:41:17PM -0300, Eduardo Habkost wrote:
> OSError can't be used like a tuple on Python 3, so change the
> code to use `e.sterror` instead of `e[1]`.
> 
> Reported-by: John Snow 
> Signed-off-by: Eduardo Habkost 
> ---
>  tests/image-fuzzer/runner.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan


signature.asc
Description: PGP signature

Re: [PATCH 00/10] image-fuzzer: Port to Python 3

[Stefan Hajnoczi]

On Wed, Oct 16, 2019 at 04:24:20PM -0300, Eduardo Habkost wrote:
> This series ports image-fuzzer to Python 3.
> 
> Eduardo Habkost (10):
>   image-fuzzer: Open image files in binary mode
>   image-fuzzer: Write bytes instead of string to image file
>   image-fuzzer: Explicitly use integer division operator
>   image-fuzzer: Use io.StringIO
>   image-fuzzer: Use %r for all fiels at Field.__repr__()
>   image-fuzzer: Return bytes objects on string fuzzing functions
>   image-fuzzer: Use bytes constant for field values
>   image-fuzzer: Encode file name and file format to bytes
>   image-fuzzer: Run using python3
>   image-fuzzer: Use errors parameter of subprocess.Popen()
> 
>  tests/image-fuzzer/qcow2/__init__.py |  1 -
>  tests/image-fuzzer/qcow2/fuzz.py | 54 +-
>  tests/image-fuzzer/qcow2/layout.py   | 57 ++--
>  tests/image-fuzzer/runner.py | 12 +++---
>  4 files changed, 61 insertions(+), 63 deletions(-)
> 
> -- 
> 2.21.0
> 
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan


signature.asc
Description: PGP signature

Re: [RFC PATCH v2 00/26] Add subcluster allocation to qcow2

[Max Reitz]

On 26.10.19 23:25, Alberto Garcia wrote:
> Hi,
> 
> here's the new version of the patches to add subcluster allocation
> support to qcow2.
> 
> Please refer to the cover letter of the first version for a full
> description of the patches:
> 
>https://lists.gnu.org/archive/html/qemu-block/2019-10/msg00983.html
> 
> This version includes a few tests, but I'm planning to add more for
> the next revision.

I think what would help most with testing is if it were possible to
simply run the iotests with -o extended_l2=on.

In general, the RFC looks OK to me.  The one thing I dislike is that I
feel that it is a bit, well, uncourageous.  Now, after looking at the
series, I don’t know whether you really changed everything that needs to
be changed so it can deal with subclusters.

To me it feels like this is because you tried to keep everything as it
is and only do minimal changes.  That is usually a good thing, but here
I don’t know, because this way we can’t simply grep for places that need
fixing (because they use /\

signature.asc
Description: OpenPGP digital signature

Re: [RFC PATCH v2 25/26] qcow2: Allow preallocation and backing files if extended_l2 is set

[Max Reitz]

On 26.10.19 23:25, Alberto Garcia wrote:
> Traditional qcow2 images don't allow preallocation if a backing file
> is set. This is because once a cluster is allocated there is no way to
> tell that its data should be read from the backing file.
> 
> Extended L2 entries have individual allocation bits for each
> subcluster, and therefore it is perfectly possible to have an
> allocated cluster with all its subclusters unallocated.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block/qcow2.c  | 7 ---
>  tests/qemu-iotests/206.out | 2 +-
>  2 files changed, 5 insertions(+), 4 deletions(-)

But it doesn’t work, because qcow2_alloc_cluster_offset() always
allocates the whole cluster, so the backing file content isn’t visible:

$ ./qemu-img create -f qcow2 base.qcow2 64M
Formatting 'base.qcow2', fmt=qcow2 size=67108864 cluster_size=65536
lazy_refcounts=off extended_l2=off refcount_bits=16

$ ./qemu-io -c 'write -P 42 0 64M' base.qcow2
wrote 67108864/67108864 bytes at offset 0
64 MiB, 1 ops; 00.21 sec (307.344 MiB/sec and 4.8022 ops/sec)

$ ./qemu-img create -f qcow2 -o preallocation=metadata,extended_l2=on \
top.qcow2
Formatting 'top.qcow2', fmt=qcow2 size=67108864 backing_file=base.qcow2
cluster_size=65536 preallocation=metadata lazy_refcounts=off
extended_l2=on refcount_bits=16

$ ./qemu-io -c 'read -P 42 0 64M' top.qcow2
Pattern verification failed at offset 0, 67108864 bytes
read 67108864/67108864 bytes at offset 0
64 MiB, 1 ops; 00.03 sec (2.498 GiB/sec and 39.9725 ops/sec)

Max



signature.asc
Description: OpenPGP digital signature

Re: [RFC PATCH v2 24/26] qcow2: Add the 'extended_l2' option and the QCOW2_INCOMPAT_EXTL2 bit

[Max Reitz]

On 26.10.19 23:25, Alberto Garcia wrote:
> Now that the implementation of subclusters is complete we can finally
> add the necessary options to create and read images with this feature,
> which we call "extended L2 entries".
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block/qcow2.c|  46 ++
>  block/qcow2.h|   8 ++-
>  include/block/block_int.h|   1 +
>  qapi/block-core.json |   6 ++
>  tests/qemu-iotests/031.out   |   8 +--
>  tests/qemu-iotests/036.out   |   4 +-
>  tests/qemu-iotests/049.out   | 102 +++
>  tests/qemu-iotests/060.out   |   1 +
>  tests/qemu-iotests/061.out   |  20 +++---
>  tests/qemu-iotests/065   |  18 --
>  tests/qemu-iotests/082.out   |  48 ---
>  tests/qemu-iotests/085.out   |  38 ++--
>  tests/qemu-iotests/144.out   |   4 +-
>  tests/qemu-iotests/182.out   |   2 +-
>  tests/qemu-iotests/185.out   |   8 +--
>  tests/qemu-iotests/198.out   |   2 +
>  tests/qemu-iotests/206.out   |   4 ++
>  tests/qemu-iotests/242.out   |   5 ++
>  tests/qemu-iotests/255.out   |   8 +--
>  tests/qemu-iotests/common.filter |   1 +
>  20 files changed, 224 insertions(+), 110 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 537569ce88..b1fa7ab5da 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1347,6 +1347,12 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
> *bs, QDict *options,
>  s->subcluster_size = s->cluster_size / s->subclusters_per_cluster;
>  s->subcluster_bits = ctz32(s->subcluster_size);
>  
> +if (s->subcluster_size < (1 << MIN_CLUSTER_BITS)) {
> +error_setg(errp, "Unsupported subcluster size: %d", 
> s->subcluster_size);
> +ret = -EINVAL;
> +goto fail;
> +}
> +
>  /* Check support for various header values */
>  if (header.refcount_order > 6) {
>  error_setg(errp, "Reference count entry width too large; may not "

It feels like this hunk should be part of the patch that added the
s->subcluster_size assignment.

> @@ -2806,6 +2812,11 @@ int qcow2_update_header(BlockDriverState *bs)
>  .bit  = QCOW2_COMPAT_LAZY_REFCOUNTS_BITNR,
>  .name = "lazy refcounts",
>  },
> +{
> +.type = QCOW2_FEAT_TYPE_INCOMPATIBLE,
> +.bit  = QCOW2_INCOMPAT_EXTL2_BITNR,
> +.name = "extended L2 entries",
> +},
>  };
>  
>  ret = header_ext_add(buf, QCOW2_EXT_MAGIC_FEATURE_TABLE,
> @@ -3271,6 +3282,27 @@ qcow2_co_create(BlockdevCreateOptions *create_options, 
> Error **errp)
>  goto out;
>  }
>  
> +if (!qcow2_opts->has_extended_l2) {
> +qcow2_opts->extended_l2 = false;
> +}
> +if (qcow2_opts->extended_l2) {
> +unsigned min_cluster_size =
> +(1 << MIN_CLUSTER_BITS) * QCOW_MAX_SUBCLUSTERS_PER_CLUSTER;
> +if (version < 3) {
> +error_setg(errp, "Extended L2 entries are only supported with "
> +   "compatibility level 1.1 and above (use version=v3 or 
> "

Pre-existing, but old-style creation doesn’t allow version=v3.  I
suppose someone(tm) should fix that...

> +   "greater)");
> +ret = -EINVAL;
> +goto out;
> +}
> +if (cluster_size < min_cluster_size) {
> +error_setg(errp, "Extended L2 entries are only supported with "
> +   "cluster sizes of at least %u bytes", 
> min_cluster_size);
> +ret = -EINVAL;
> +goto out;
> +}
> +}
> +
>  if (!qcow2_opts->has_preallocation) {
>  qcow2_opts->preallocation = PREALLOC_MODE_OFF;
>  }

[...]

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index aa97ee2641..6e4c81ae1c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -66,6 +66,8 @@
>  # standalone (read-only) raw image without looking at qcow2
>  # metadata (since: 4.0)
>  #
> +# @extended-l2: true if the image has extended L2 entries (since 4.2)
> +#

I think there should be the same note here as for lazy-refcounts.  So
this field is not present for compat=0.10, and present otherwise.
(Without that note, it looks as if this field maybe is only present if
it’s true.)

(Personally, I’d also prefer it to be an “iff”, but who cares.)

>  # @lazy-refcounts: on or off; only valid for compat >= 1.1
>  #
>  # @corrupt: true if the image has been marked corrupt; only valid for
> @@ -85,6 +87,7 @@
>'compat': 'str',
>'*data-file': 'str',
>'*data-file-raw': 'bool',
> +  '*extended-l2': 'bool',
>'*lazy-refcounts': 'bool',
>'*corrupt': 'bool',
>'refcount-bits': 'int',
> @@ -4372,6 +4375,8 @@
>  # @data-file-rawTrue if the external data file must stay valid as a
>  #   

Re: [RFC PATCH v2 22/26] qcow2: Add subcluster support to handle_alloc_space()

[Max Reitz]

On 26.10.19 23:25, Alberto Garcia wrote:
> The bdrv_co_pwrite_zeroes() call here fills complete clusters with
> zeroes, but it can happen that some subclusters are not part of the
> write request or the copy-on-write. This patch makes sure that only
> the affected subclusters are overwritten.
> 
> A potential improvement would be to also fill with zeroes the other
> subclusters if we can guarantee that we are not overwriting existing
> data. However this would waste more disk space, so we should first
> evaluate if it's really worth doing.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block/qcow2.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 0261e87709..01322ca449 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2304,6 +2304,9 @@ static int handle_alloc_space(BlockDriverState *bs, 
> QCowL2Meta *l2meta)
>  
>  for (m = l2meta; m != NULL; m = m->next) {
>  int ret;
> +uint64_t start_offset = m->alloc_offset + m->cow_start.offset;
> +uint64_t nb_bytes = m->cow_end.offset + m->cow_end.nb_bytes -
> +m->cow_start.offset;

It might be more honest to make nb_bytes an unsigned.  (There shouldn’t
be any overflows here, because the total size is limited to INT64_MAX by
handle_alloc().)

Max

>  if (!m->cow_start.nb_bytes && !m->cow_end.nb_bytes) {
>  continue;



signature.asc
Description: OpenPGP digital signature

Re: [RFC PATCH v2 20/26] qcow2: Update L2 bitmap in qcow2_alloc_cluster_link_l2()

[Max Reitz]

On 26.10.19 23:25, Alberto Garcia wrote:
> The L2 bitmap needs to be updated after each write to indicate what
> new subclusters are now allocated.
> 
> This needs to happen even if the cluster was already allocated and the
> L2 entry was otherwise valid.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block/qcow2-cluster.c | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index fb6cf8df17..acb7226e03 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -980,6 +980,22 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, 
> QCowL2Meta *m)
>  
>  set_l2_entry(s, l2_slice, l2_index + i, QCOW_OFLAG_COPIED |
>   (cluster_offset + (i << s->cluster_bits)));
> +
> +/* Update bitmap with the subclusters that were just written */
> +if (has_subclusters(s)) {
> +uint64_t written_from = m->cow_start.offset;
> +uint64_t written_to = m->cow_end.offset + m->cow_end.nb_bytes;

It’s a bit strange to make these uint64_t when all the fields queried
are of type unsigned, but more on that below.

> +uint64_t l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index + i);
> +int sc;
> +for (sc = 0; sc < s->subclusters_per_cluster; sc++) {
> +uint64_t sc_off = i * s->cluster_size + sc * 
> s->subcluster_size;

It’s weird to give this a uint64_t type when all the variables in the
term are of type int.

I’m not sure whether it can overflow.  handle_alloc() limits everything
to INT_MAX, but I’m not sure about handle_copied().

Speaking of handle_copied(); both elements of Qcow2COWRegion are of type
unsigned.  handle_copied() doesn’t look like it takes any precautions to
limit the range to even UINT_MAX (and it should probably limit it to
INT_MAX).

Max

> +if (sc_off >= written_from && sc_off < written_to) {
> +l2_bitmap |= QCOW_OFLAG_SUB_ALLOC(sc);
> +l2_bitmap &= ~QCOW_OFLAG_SUB_ZERO(sc);
> +}
> +}
> +set_l2_bitmap(s, l2_slice, l2_index + i, l2_bitmap);
> +}
>   }
>  
>  
> 




signature.asc
Description: OpenPGP digital signature

Re: [RFC PATCH v2 18/26] qcow2: Add subcluster support to expand_zero_clusters_in_l1()

[Max Reitz]

On 26.10.19 23:25, Alberto Garcia wrote:
> Two changes are needed in order to add subcluster support to this
> function: deallocated clusters must have their bitmaps cleared, and
> expanded clusters must have all the "subcluster allocated" bits set.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block/qcow2-cluster.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index aa3eb727a5..62f2a9fcc0 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -2036,6 +2036,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState 
> *bs, uint64_t *l1_table,
>  /* not backed; therefore we can simply deallocate the
>   * cluster */
>  set_l2_entry(s, l2_slice, j, 0);
> +set_l2_bitmap(s, l2_slice, j, 0);
>  l2_dirty = true;
>  continue;
>  }
> @@ -2102,6 +2103,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState 
> *bs, uint64_t *l1_table,
>  } else {
>  set_l2_entry(s, l2_slice, j, offset);
>  }
> +set_l2_bitmap(s, l2_slice, j, QCOW_L2_BITMAP_ALL_ALLOC);
>  l2_dirty = true;
>  }

Technically this isn’t needed because this function is only called when
downgrading v3 to v2 images (which can’t have subclusters), but of
course it won’t hurt.

Max



signature.asc
Description: OpenPGP digital signature