date:20200303


03.03.2020 16:34, Denis Plotnikov wrote:

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

The performance test results:
Test compresses and decompresses qemu qcow2 image with just
installed rhel-7.6 guest.
Image cluster size: 64K. Image on disk size: 2.2G

The test was conducted with brd disk to reduce the influence
of disk subsystem to the test results.
The results is given in seconds.

compress cmd:
   time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
   src.img [zlib|zstd]_compressed.img
decompress cmd
   time ./qemu-img convert -O qcow2
   [zlib|zstd]_compressed.img uncompressed.img

compression   decompression
  zlib   zstd   zlib zstd

real 65.5   16.3 (-75 %)1.9  1.6 (-16 %)
user 65.0   15.85.3  2.5
sys   3.30.22.0  2.0

Both ZLIB and ZSTD gave the same compression ratio: 1.57
compressed image size in both cases: 1.4G

Signed-off-by: Denis Plotnikov 
---


[..]


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


You may want to define ZSTD_COMPRESSION_LEVEL constant instead of raw number.

anyway,
Reviewed-by: Vladimir Sementsov-Ogievskiy 




--
Best regards,
Vladimir

Re: [PATCH v4 3/5] qcow2: rework the cluster compression routine


03.03.2020 16:34, Denis Plotnikov wrote:

The patch enables processing the image compression type defined
for the image and chooses an appropriate method for image clusters
(de)compression.

Signed-off-by: Denis Plotnikov 
---
  block/qcow2-threads.c | 71 ---
  1 file changed, 60 insertions(+), 11 deletions(-)

diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index a68126f291..9bfcda6918 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -74,7 +74,9 @@ typedef struct Qcow2CompressData {
  } Qcow2CompressData;
  
  /*

- * qcow2_compress()
+ * qcow2_zlib_compress()
+ *
+ * Compress @src_size bytes of data using zlib compression method
   *
   * @dest - destination buffer, @dest_size bytes
   * @src - source buffer, @src_size bytes
@@ -83,8 +85,8 @@ typedef struct Qcow2CompressData {
   *  -ENOMEM destination buffer is not enough to store compressed data
   *  -EIOon any other error
   */
-static ssize_t qcow2_compress(void *dest, size_t dest_size,
-  const void *src, size_t src_size)
+static ssize_t qcow2_zlib_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
  {
  ssize_t ret;
  z_stream strm;
@@ -119,10 +121,10 @@ static ssize_t qcow2_compress(void *dest, size_t 
dest_size,
  }
  
  /*

- * qcow2_decompress()
+ * qcow2_zlib_decompress()
   *
   * Decompress some data (not more than @src_size bytes) to produce exactly
- * @dest_size bytes.
+ * @dest_size bytes using zlib compression method
   *
   * @dest - destination buffer, @dest_size bytes
   * @src - source buffer, @src_size bytes
@@ -130,8 +132,8 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
   * Returns: 0 on success
   *  -EIO on fail
   */
-static ssize_t qcow2_decompress(void *dest, size_t dest_size,
-const void *src, size_t src_size)
+static ssize_t qcow2_zlib_decompress(void *dest, size_t dest_size,
+ const void *src, size_t src_size)
  {
  int ret;
  z_stream strm;
@@ -191,20 +193,67 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest, 
size_t dest_size,
  return arg.ret;
  }
  
+/*

+ * qcow2_co_compress()
+ *
+ * Compress @src_size bytes of data using the compression
+ * method defined by the image compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ *  a negative error code on failure
+ */
  ssize_t coroutine_fn
  qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
const void *src, size_t src_size)
  {
-return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
-qcow2_compress);
+BDRVQcow2State *s = bs->opaque;
+Qcow2CompressFunc fn;
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+fn = qcow2_zlib_compress;
+break;
+
+default:
+abort();
+}
+
+return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
  }
  
+/*

+ * qcow2_co_decompress()
+ *
+ * Decompress some data (not more than @src_size bytes) to produce exactly
+ * @dest_size bytes using the compression method defined by the image
+ * compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: 0 on success
+ *  a negative error code on failure
+ */
  ssize_t coroutine_fn
  qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
  const void *src, size_t src_size)
  {
-return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
-qcow2_decompress);
+BDRVQcow2State *s = bs->opaque;
+Qcow2CompressFunc fn;
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+fn = qcow2_zlib_decompress;
+break;
+
+default:
+return -ENOTSUP;


abort() here, like in _compress()


+}
+
+return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
  }
  
  




--
Best regards,
Vladimir

Re: [PATCH v6 9/9] iotests: add pylintrc file

John Snow  writes:

> Repeatable results. run `pylint iotests.py` and you should get a pass.

Start your sentences with a capital letter, please.

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

Add the newline, please.

German pejorative for the too-many- and too-few- warnings: "Müsli".
Implies it's for muesli-knitters / granola-crunchers indulging their
orthorexia.

missing-docstring is not advisable for libraries.  Feels okay here.

line-too-long might be worth cleaning up.  How many of them do we have
now?

[PATCH 2/2] misc: Replace zero-length arrays with flexible array member (manual)

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

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

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

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

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

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

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

All these instances of code were found with the help of the
following command (then manual analysis):

  git grep -F '[0];'

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

Inspired-by: Gustavo A. R. Silva 
Signed-off-by: Philippe Mathieu-Daudé 
---
 docs/interop/vhost-user.rst   | 4 ++--
 block/qed.h   | 2 +-
 include/hw/acpi/acpi-defs.h   | 4 ++--
 include/hw/boards.h   | 2 +-
 include/hw/s390x/event-facility.h | 2 +-
 include/hw/s390x/sclp.h   | 8 
 block/vmdk.c  | 2 +-
 hw/char/sclpconsole-lm.c  | 2 +-
 hw/char/sclpconsole.c | 2 +-
 hw/s390x/virtio-ccw.c | 2 +-
 target/s390x/ioinst.c | 2 +-
 11 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
index 401652397c..3b1b6602c7 100644
--- a/docs/interop/vhost-user.rst
+++ b/docs/interop/vhost-user.rst
@@ -568,7 +568,7 @@ For split virtqueue, queue region can be implemented as:
   uint16_t used_idx;
 
   /* Used to track the state of each descriptor in descriptor table */
-  DescStateSplit desc[0];
+  DescStateSplit desc[];
   } QueueRegionSplit;
 
 To track inflight I/O, the queue region should be processed as follows:
@@ -690,7 +690,7 @@ For packed virtqueue, queue region can be implemented as:
   uint8_t padding[7];
 
   /* Used to track the state of each descriptor fetched from descriptor 
ring */
-  DescStatePacked desc[0];
+  DescStatePacked desc[];
   } QueueRegionPacked;
 
 To track inflight I/O, the queue region should be processed as follows:
diff --git a/block/qed.h b/block/qed.h
index 42c115d822..87428ba00e 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -103,7 +103,7 @@ typedef struct {
 } QEMU_PACKED QEDHeader;
 
 typedef struct {
-uint64_t offsets[0];/* in bytes */
+uint64_t offsets[]; /* in bytes */
 } QEDTable;
 
 /* The L2 cache is a simple write-through cache for L2 structures */
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 19f7ba7b70..c13327fa78 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -152,7 +152,7 @@ typedef struct AcpiSerialPortConsoleRedirection
  */
 struct AcpiRsdtDescriptorRev1 {
 ACPI_TABLE_HEADER_DEF   /* ACPI common table header */
-uint32_t table_offset_entry[0];  /* Array of pointers to other */
+uint32_t table_offset_entry[];  /* Array of pointers to other */
 /* ACPI tables */
 } QEMU_PACKED;
 typedef struct AcpiRsdtDescriptorRev1 AcpiRsdtDescriptorRev1;
@@ -162,7 +162,7 @@ typedef struct AcpiRsdtDescriptorRev1 
AcpiRsdtDescriptorRev1;
  */
 struct AcpiXsdtDescriptorRev2 {
 ACPI_TABLE_HEADER_DEF   /* ACPI common table header */
-uint64_t table_offset_entry[0];  /* Array of pointers to other */
+uint64_t table_offset_entry[];  /* Array of pointers to other */
 /* ACPI tables */
 } QEMU_PACKED;
 typedef struct AcpiXsdtDescriptorRev2 AcpiXsdtDescriptorRev2;
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 9bc42dfb22..c96120d15f 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -71,7 +71,7 @@ typedef struct CPUArchId {
  */
 typedef struct {
 int len;
-CPUArchId cpus[0];
+CPUArchId cpus[];
 } CPUArchIdList;
 
 /**
diff --git a/include/hw/s390x/event-facility.h 
b/include/hw/s390x/event-facility.h
index bdc32a3c09..700a610f33 100644
--- a/include/hw/s390x/event-facility.h
+++ b/include/hw/s390x/event-facility.h
@@ -122,7 +122,7 @@ typedef struct MDBO {
 
 typedef struct MDB {
 MdbHeader header;
-MDBO mdbo[0];
+MDBO mdbo[];
 } QEMU_PACKED MDB;
 
 typedef struct SclpMsg {
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index c54413b78c..cd7b24359f 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -132,7 +132,7 @@ typedef struct ReadInfo {
 uint16_t highest_cpu;
 uint8_t  _reserved5[124

[PATCH 1/2] misc: Replace zero-length arrays with flexible array member (automatic)

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

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

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

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

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

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

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

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

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

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

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

diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index 09e8aed9c7..f8bb1e5459 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -95,7 +95,7 @@ typedef struct TaskState {
 struct sigqueue *first_free; /* first free siginfo queue entry */
 int signal_pending; /* non zero if a signal may be pending */
 
-uint8_t stack[0];
+uint8_t stack[];
 } __attribute__((aligned(16))) TaskState;
 
 void init_task_state(TaskState *ts);
diff --git a/contrib/libvhost-user/libvhost-user.h 
b/contrib/libvhost-user/libvhost-user.h
index 6fc8000e99..f30394fab6 100644
--- a/contrib/libvhost-user/libvhost-user.h
+++ b/contrib/libvhost-user/libvhost-user.h
@@ -286,7 +286,7 @@ typedef struct VuVirtqInflight {
 uint16_t used_idx;
 
 /* Used to track the state of each descriptor in descriptor table */
-VuDescStateSplit desc[0];
+VuDescStateSplit desc[];
 } VuVirtqInflight;
 
 typedef struct VuVirtqInflightDesc {
diff --git a/hw/m68k/bootinfo.h b/hw/m68k/bootinfo.h
index 5f8ded2686..c954270aad 100644
--- a/hw/m68k/bootinfo.h
+++ b/hw/m68k/bootinfo.h
@@ -14,7 +14,7 @@
 struct bi_record {
 uint16_t tag;/* tag ID */
 uint16_t size;   /* size of record */
-uint32_t data[0];/* data */
+uint32_t data[]; /* data */
 };
 
 /* machine independent tags */
diff --git a/hw/scsi/srp.h b/hw/scsi/srp.h
index d27f31d2d5..54c954badd 100644
--- a/hw/scsi/srp.h
+++ b/hw/scsi/srp.h
@@ -112,7 +112,7 @@ struct srp_direct_buf {
 struct srp_indirect_buf {
 struct srp_direct_buftable_desc;
 uint32_t len;
-struct srp_direct_bufdesc_list[0];
+struct srp_direct_bufdesc_list[];
 } QEMU_PACKED;
 
 enum {
@@ -211,7 +211,7 @@ struct srp_cmd {
 uint8_treserved4;
 uint8_tadd_cdb_len;
 uint8_tcdb[16];
-uint8_tadd_data[0];
+uint8_tadd_data[];
 } QEMU_PACKED;
 
 enum {
@@ -241,7 +241,7 @@ struct srp_rsp {
 uint32_t   data_in_res_cnt;
 uint32_t   sense_data_len;
 uint32_t   resp_data_len;
-uint8_tdata[0];
+uint8_tdata[];
 } QEMU_PACKED;
 
 #endif /* SCSI_SRP_H */
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 9167bbaf6d..179775db7b 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -203,7 +203,7 @@ typedef struct XenPTMSIX {
 uint64_t mmio_

[PATCH 0/2] misc: Replace zero-length arrays with flexible array member

This is a tree-wide cleanup inspired by a Linux kernel commit
(from Gustavo A. R. Silva).

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

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

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

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

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

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

The first patch is done with the help of a coccinelle semantic
patch. However Coccinelle does not recognize:

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

but does recognize:

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

I'm not sure why, neither it is worth refactoring all QEMU
structures to use the attributes before the structure name,
so I did the 2nd patch manually.

Anyway this is annoying, because many structures are not handled
by coccinelle. Maybe this needs to be reported to upstream
coccinelle?

I used spatch 1.0.8 with:

  -I include --include-headers \
  --macro-file scripts/cocci-macro-file.h \
  --keep-comments --indent 4

Regards,

Phil.

Philippe Mathieu-Daudé (2):
  misc: Replace zero-length arrays with flexible array member
(automatic)
  misc: Replace zero-length arrays with flexible array member (manual)

 docs/interop/vhost-user.rst   |  4 ++--
 block/qed.h   |  2 +-
 bsd-user/qemu.h   |  2 +-
 contrib/libvhost-user/libvhost-user.h |  2 +-
 hw/m68k/bootinfo.h|  2 +-
 hw/scsi/srp.h |  6 +++---
 hw/xen/xen_pt.h   |  2 +-
 include/hw/acpi/acpi-defs.h   | 16 
 include/hw/arm/smmu-common.h  |  2 +-
 include/hw/boards.h   |  2 +-
 include/hw/i386/intel_iommu.h |  3 ++-
 include/hw/s390x/event-facility.h |  2 +-
 include/hw/s390x/sclp.h   |  8 
 include/hw/virtio/virtio-iommu.h  |  2 +-
 include/sysemu/cryptodev.h|  2 +-
 include/tcg/tcg.h |  2 +-
 pc-bios/s390-ccw/bootmap.h|  2 +-
 pc-bios/s390-ccw/sclp.h   |  2 +-
 tests/qtest/libqos/ahci.h |  2 +-
 block/linux-aio.c |  2 +-
 block/vmdk.c  |  2 +-
 hw/acpi/nvdimm.c  |  6 +++---
 hw/char/sclpconsole-lm.c  |  2 +-
 hw/char/sclpconsole.c |  2 +-
 hw/dma/soc_dma.c  |  2 +-
 hw/i386/x86.c |  2 +-
 hw/misc/omap_l4.c |  2 +-
 hw/nvram/eeprom93xx.c |  2 +-
 hw/rdma/vmw/pvrdma_qp_ops.c   |  4 ++--
 hw/s390x/virtio-ccw.c |  2 +-
 hw/usb/dev-network.c  |  2 +-
 hw/usb/dev-smartcard-reader.c |  4 ++--
 hw/virtio/virtio.c|  4 ++--
 net/queue.c   |  2 +-
 target/s390x/ioinst.c |  2 +-
 35 files changed, 54 insertions(+), 53 deletions(-)

-- 
2.21.1

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

2020-03-03 Thread BALATON Zoltan

On Tue, 3 Mar 2020, Mark Cave-Ayland wrote:

On 02/03/2020 21:40, BALATON Zoltan wrote:

I had a quick look at the schematics linked from the page above, and they
confirm
that the VIA IDE interface is connected directly to IRQs 14 and 15 and not to
the PCI
interrupt pins.

Where did you see that? What I got from trying to look this up in the
schematics was
that VT8231 has two pins named IRQ14 and IRQ15 (but described as Primary and
Secondary Channel Interrupt Request in doc) where the interrupt lines of the
two IDE
ports/channels are connected but how they are routed within the chip after that
was
not clear. The chip doc says that in native mode the interrupt should be
configurable
and use a single interrupt for both channels and in legacy mode they use the
usual 14
and 15 but this is not what guest drivers expect so I think not how really
works on
PegasosII. It is true however that connection to PCI interrupts aren't
mentioned so
it always uses ISA IRQ numbers, it just depends on legacy vs. native mode which
line
is raised. But that was never really a question for VT8231 and maybe only CMD646
could have such interconnection with PCI interrupts. (Proabable reason is that
via-ide is part of a southbridge chip where it has connections to ISA bus while
CMD646 is a PCI IDE controller but I could be wrong as my knowledge is limited
about
these.)

Presumably the VIA southbridge has its own internal pair of cascaded 8259s so
the IRQ
line from the drive is connected to IRQ14/15 as per an typical ISA PC. You can
see
this in the "Common Hardware Reference Platform: I/O Device Reference" PDF
section 4.1.

Yes the VIA southbridge chip integrates all usual PC devices including
PICs so these may have connection internally. I haven't checked these docs
before but now I think another chapter in the doc you referenced and its
companion "Common Hardware Reference Platform: A System Architecture" may
explain the unusal combination of PCI like io regs with legacy interrupts
the Pegasos2 seems to have. In the I/O Device Reference, chapter 11 about
IDE says that the device must be addressed only with PCI I/O addresses
where io addresses are completely relocatable and that when OpenFirmware
passes control to the OS IDE must be configured as a PCI device. So this
probably means io addresses coming from PCI BARs. But the CHRP System
Architecture, chapter 9.2 about ISA devices says that "ISA devices
included in a PCI part must route their interrupt signals to the legacy
interrupt controller" and this is what the Pegasos2 seems to do. This may
be a misinterpretation of the spec because elsewhere (I've lost the exact
reference and have no time to find it again) it also says something about
native devices must be using OpenPIC but Pegasos2 does not have OpenPIC so
that part surely cannot apply anyway.

So on that basis the best explanation as to what is happening is the
comment in the link you provided here:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/platforms/chrp/pci.c?h=v4.14.172#n353

/* Pegasos2 firmware version 20040810 configures the built-in IDE controller
* in legacy mode, but sets the PCI registers to PCI native mode.
* The chip can only operate in legacy mode, so force the PCI class into legacy
* mode as well. The same fixup must be done to the class-code property in
* the IDE node /pci@8000/ide@C,1
*/

I'm not sure that it makes much sense that the firmware configures the chip one
way
then puts info about a different way in the device tree. There could be bugs
but this
is not likely. Especially because I see in traces that the firmware does try to
configure the device in native mode. These are the first few accesses of the
firmware
to via-ide:

But that is exactly what is happening! The comment above clearly indicates the
firmware incorrectly sets the IDE controller in native mode which is in exact
agreement with the trace you provide below. And in fact if you look at

I may be reading the comment wrong but to me that says that "firmware
configures IDE in _legacy_ mode" whereas the trace shows it actually
configures it in _native_ mode which is complying to the CHRP doc above.
But since it cannot comply to the "native devices using OpenPIC" part it
probably tries to apply the "ISA devices embedded in PCI" part and locks
IRQ to 14 and 15. Or it just wants to avoid sharing IRQs as much as
possible and the designers decided they will use IRQ14 and 15 for IDE.

https://www.powerdeveloper.org/platforms/pegasos/devicetree you can see the
nvramrc
hack that was released in order to fix the device tree to boot Linux which
alters the
class-code and sets interrupts to 14 (which I believe is invalid, but seemingly
good
enough here).

Isn't this the same fixup that newer Linux kernels already include? Maybe
this was needed before Linux properly supported Pegasos2 but later kernels
will do this anyway. This does not give us any new info

Re: [PATCH v6 7/9] iotests: ignore import warnings from pylint


On 3/3/20 8:57 PM, John Snow wrote:

On 2/27/20 9:14 AM, Philippe Mathieu-Daudé wrote:

On 2/27/20 1:06 AM, John Snow wrote:

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

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


I'm tempted to NAck this and require an "installable python module"...

Let's discuss why it is that hard!



I've been tricked into this before. It's not work I am interested in
doing right now; it's WAY beyond the scope of what I am doing here.

It involves properly factoring all of our python code, deciding which
portions are meant to be installed separately from QEMU itself, coming
up with a versioning scheme, packaging code, and moving code around in
many places.

Then it involves coming up with tooling and infrastructure for creating
virtual environments, installing the right packages to it, and using it
to run our python tests.

No, that's way too invasive. I'm not doing it and I will scream loudly
if you make me.

A less invasive hack involves setting the python import path in a
consolidated spot so that python knows where it can import from. This
works, but might break the ability to run such tests as one-offs without
executing the environment setup.

Again, not work I care to do right now and so I won't. The benefit of
these patches is to provide some minimum viable CI CQA for Python where
we had none before, NOT fix every possible Python problem in one shot.


OK I guess we misunderstood each other :)

I didn't understood your comment as personal to you for this patch, but 
generic. It makes sense it is not your priority and it is obvious this 
task will take a single developer a lot of time resources. I am 
certainly NOT asking you to do it.


My question was rather community-oriented.
(Cc'ing Eduardo because we talked about this after the last KVM forum).



--js



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

diff --git a/tests/qemu-iotests/iotests.py
b/tests/qemu-iotests/iotests.py
index 60c4c7f736..214f59995e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -30,6 +30,7 @@
   from collections import OrderedDict
   from typing import Collection
   +# pylint: disable=import-error, wrong-import-position
   sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..',
'python'))
   from qemu import qtest

Re: [PATCH 5/6] qmp.py: change event_wait to use a dict




On 2/27/20 6:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> 25.02.2020 3:56, John Snow wrote:
>> It's easier to work with than a list of tuples, because we can check the
>> keys for membership.
>>
>> Signed-off-by: John Snow 
>> ---
>>   python/qemu/machine.py    | 10 +-
>>   tests/qemu-iotests/040    | 12 ++--
>>   tests/qemu-iotests/260    |  5 +++--
>>   tests/qemu-iotests/iotests.py | 16 
>>   4 files changed, 22 insertions(+), 21 deletions(-)
>>
>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>> index 183d8f3d38..748de5f322 100644
>> --- a/python/qemu/machine.py
>> +++ b/python/qemu/machine.py
>> @@ -476,21 +476,21 @@ def event_wait(self, name, timeout=60.0,
>> match=None):
>>   timeout: QEMUMonitorProtocol.pull_event timeout parameter.
>>   match: Optional match criteria. See event_match for details.
>>   """
>> -    return self.events_wait([(name, match)], timeout)
>> +    return self.events_wait({name: match}, timeout)
>>     def events_wait(self, events, timeout=60.0):
>>   """
>>   events_wait waits for and returns a named event from QMP
>> with a timeout.
>>   -    events: a sequence of (name, match_criteria) tuples.
>> +    events: a mapping containing {name: match_criteria}.
>>   The match criteria are optional and may be None.
>>   See event_match for details.
>>   timeout: QEMUMonitorProtocol.pull_event timeout parameter.
>>   """
>>   def _match(event):
>> -    for name, match in events:
>> -    if event['event'] == name and self.event_match(event,
>> match):
>> -    return True
>> +    name = event['event']
>> +    if name in events:
>> +    return self.event_match(event, events[name])
>>   return False
>>     # Search cached events
>> diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
>> index 32c82b4ec6..90b59081ff 100755
>> --- a/tests/qemu-iotests/040
>> +++ b/tests/qemu-iotests/040
>> @@ -485,12 +485,12 @@ class TestErrorHandling(iotests.QMPTestCase):
>>     def run_job(self, expected_events, error_pauses_job=False):
>>   match_device = {'data': {'device': 'job0'}}
>> -    events = [
>> -    ('BLOCK_JOB_COMPLETED', match_device),
>> -    ('BLOCK_JOB_CANCELLED', match_device),
>> -    ('BLOCK_JOB_ERROR', match_device),
>> -    ('BLOCK_JOB_READY', match_device),
>> -    ]
>> +    events = {
>> +    'BLOCK_JOB_COMPLETED': match_device,
>> +    'BLOCK_JOB_CANCELLED': match_device,
>> +    'BLOCK_JOB_ERROR': match_device,
>> +    'BLOCK_JOB_READY': match_device,
>> +    }
>>     completed = False
>>   log = []
>> diff --git a/tests/qemu-iotests/260 b/tests/qemu-iotests/260
>> index 30c0de380d..b2fb045ddd 100755
>> --- a/tests/qemu-iotests/260
>> +++ b/tests/qemu-iotests/260
>> @@ -65,8 +65,9 @@ def test(persistent, restart):
>>     vm.qmp_log('block-commit', device='drive0', top=top,
>>  filters=[iotests.filter_qmp_testfiles])
>> -    ev = vm.events_wait((('BLOCK_JOB_READY', None),
>> - ('BLOCK_JOB_COMPLETED', None)))
>> +    ev = vm.events_wait({
>> +    'BLOCK_JOB_READY': None,
>> +    'BLOCK_JOB_COMPLETED': None })
> 
> may be better to keep it 2-lines. Or, otherwise, put closing "})" on a
> separate line too.
> 

Sure.

>>   log(filter_qmp_event(ev))
>>   if (ev['event'] == 'BLOCK_JOB_COMPLETED'):
>>   vm.shutdown()
>> diff --git a/tests/qemu-iotests/iotests.py
>> b/tests/qemu-iotests/iotests.py
>> index 5d2990a0e4..3390fab021 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -604,14 +604,14 @@ def run_job(self, job, auto_finalize=True,
>> auto_dismiss=False,
>>   """
>>   match_device = {'data': {'device': job}}
>>   match_id = {'data': {'id': job}}
>> -    events = [
>> -    ('BLOCK_JOB_COMPLETED', match_device),
>> -    ('BLOCK_JOB_CANCELLED', match_device),
>> -    ('BLOCK_JOB_ERROR', match_device),
>> -    ('BLOCK_JOB_READY', match_device),
>> -    ('BLOCK_JOB_PENDING', match_id),
>> -    ('JOB_STATUS_CHANGE', match_id)
>> -    ]
>> +    events = {
>> +    'BLOCK_JOB_COMPLETED': match_device,
>> +    'BLOCK_JOB_CANCELLED': match_device,
>> +    'BLOCK_JOB_ERROR': match_device,
>> +    'BLOCK_JOB_READY': match_device,
>> +    'BLOCK_JOB_PENDING': match_id,
>> +    'JOB_STATUS_CHANGE': match_id,
>> +    }
>>   error = None
>>   while True:
>>   ev = filter_qmp_event(self.events_wait(events,
>> timeout=wait))
>>
> 
> 
> Not sure that I like new interface more (neither that it is faster), but
> it works too:
> Reviewed-by: Vladimir Sementsov-Ogi

Re: [PATCH 3/6] iotests: move bitmap helpers into their own file

On 2/27/20 5:54 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
> Clean code movement, no changes. If test passes, it should be correct :)
> 
> The only thing: I'd prefer not exporting global variables and use
> bitmaps.GROUPS instead (even then, it's not very good interface but..)
> 
> with or without it:
> Reviewed-by: Vladimir Sementsov-Ogievskiy 

Hm, yeah -- it's not great, it's definitely just a bit of a quick
shuffle instead of a more serious effort at a refactor to provide bitmap
services for all tests.

I don't think it's worth the time just yet to make a serious attempt at
making this module bigger -- but maybe there is work that can be done at
providing bitmap management services targeted for use outside of testing
code. (i.e. into the python/ folder somewhere.)

Well, I can just do this:
"
import bitmaps
from bitmaps import EmulatedBitmap
"

and bitmaps.GROUPS makes it a little more obvious in-context at the
expense of one extra import line, so I'll do that.

Re: [PATCH 2/6] qmp: expose block-dirty-bitmap-populate




On 2/27/20 5:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> 25.02.2020 3:56, John Snow wrote:
>> This is a new job-creating command.
>>
>> Signed-off-by: John Snow 
>> ---
>>   qapi/block-core.json  | 18 ++
>>   qapi/transaction.json |  2 ++
>>   blockdev.c    | 78 +++
>>   3 files changed, 98 insertions(+)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index df1797681a..a8be1fb36b 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2293,6 +2293,24 @@
>>   '*auto-finalize': 'bool',
>>   '*auto-dismiss': 'bool' } }
>>   +##
>> +# @block-dirty-bitmap-populate:
>> +#
>> +# Creates a new job that writes a pattern into a dirty bitmap.
>> +#
>> +# Since: 5.0
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "block-dirty-bitmap-populate",
>> +#  "arguments": { "node": "drive0", "target": "bitmap0",
>> +# "job-id": "job0", "pattern": "allocate-top" } }
>> +# <- { "return": {} }
>> +#
>> +##
>> +{ 'command': 'block-dirty-bitmap-populate', 'boxed': true,
>> +  'data': 'BlockDirtyBitmapPopulate' }
>> +
>>   ##
>>   # @BlockDirtyBitmapSha256:
>>   #
>> diff --git a/qapi/transaction.json b/qapi/transaction.json
>> index 04301f1be7..28521d5c7f 100644
>> --- a/qapi/transaction.json
>> +++ b/qapi/transaction.json
>> @@ -50,6 +50,7 @@
>>   # - @block-dirty-bitmap-enable: since 4.0
>>   # - @block-dirty-bitmap-disable: since 4.0
>>   # - @block-dirty-bitmap-merge: since 4.0
>> +# - @block-dirty-bitmap-populate: since 5.0
>>   # - @blockdev-backup: since 2.3
>>   # - @blockdev-snapshot: since 2.5
>>   # - @blockdev-snapshot-internal-sync: since 1.7
>> @@ -67,6 +68,7 @@
>>  'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
>>  'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
>>  'block-dirty-bitmap-merge': 'BlockDirtyBitmapMerge',
>> +   'block-dirty-bitmap-populate': 'BlockDirtyBitmapPopulate',
>>  'blockdev-backup': 'BlockdevBackup',
>>  'blockdev-snapshot': 'BlockdevSnapshot',
>>  'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
>> diff --git a/blockdev.c b/blockdev.c
>> index 011dcfec27..33c0e35399 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -2314,6 +2314,67 @@ static void
>> block_dirty_bitmap_remove_commit(BlkActionState *common)
>>   bdrv_release_dirty_bitmap(state->bitmap);
>>   }
>>   +static void block_dirty_bitmap_populate_prepare(BlkActionState
>> *common, Error **errp)
> 
> over80 line (not the only)
> 

OK, will fix all instances.

>> +{
>> +    BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState,
>> common, common);
> 
> At first glance using *Backup* looks like a mistake. May be rename it,
> or at least add a comment.
> 

You're right, it's tricky. A rename would be helpful.

>> +    BlockDirtyBitmapPopulate *bitpop;
>> +    BlockDriverState *bs;
>> +    AioContext *aio_context;
>> +    BdrvDirtyBitmap *bmap = NULL;
>> +    int job_flags = JOB_DEFAULT;
>> +
>> +    assert(common->action->type ==
>> TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_POPULATE);
>> +    bitpop = common->action->u.block_dirty_bitmap_populate.data;
>> +
>> +    bs = bdrv_lookup_bs(bitpop->node, bitpop->node, errp);
>> +    if (!bs) {
>> +    return;
>> +    }
>> +
>> +    aio_context = bdrv_get_aio_context(bs);
>> +    aio_context_acquire(aio_context);
>> +    state->bs = bs;
>> +
>> +    bmap = bdrv_find_dirty_bitmap(bs, bitpop->name);
> 
> Could we use block_dirty_bitmap_lookup ?
> 

Yes.

>> +    if (!bmap) {
>> +    error_setg(errp, "Bitmap '%s' could not be found",
>> bitpop->name);
>> +    return;
> 
> aio context lock leaked
> 

Good spot.

>> +    }
>> +
>> +    /* Paired with .clean() */
>> +    bdrv_drained_begin(state->bs);
>> +
>> +    if (!bitpop->has_on_error) {
>> +    bitpop->on_error = BLOCKDEV_ON_ERROR_REPORT;
>> +    }
>> +    if (!bitpop->has_auto_finalize) {
>> +    bitpop->auto_finalize = true;
>> +    }
>> +    if (!bitpop->has_auto_dismiss) {
>> +    bitpop->auto_dismiss = true;
>> +    }
>> +
>> +    if (!bitpop->auto_finalize) {
>> +    job_flags |= JOB_MANUAL_FINALIZE;
>> +    }
>> +    if (!bitpop->auto_dismiss) {
>> +    job_flags |= JOB_MANUAL_DISMISS;
>> +    }
>> +
>> +    state->job = bitpop_job_create(
>> +    bitpop->job_id,
>> +    bs,
>> +    bmap,
>> +    bitpop->pattern,
>> +    bitpop->on_error,
>> +    job_flags,
>> +    NULL, NULL,
>> +    common->block_job_txn,
>> +    errp);
>> +
>> +    aio_context_release(aio_context);
>> +}
>> +
>>   static void abort_prepare(BlkActionState *common, Error **errp)
>>   {
>>   error_setg(errp, "Transaction aborted using Abort action");
>> @@ -2397,6 +2458,13 @@ static const BlkActionOps actions[] = {
>>   .commit = block_dirty_bitmap_remove_commit,
>>   .abort = block_dirty_bitmap_remove_abort,
>>   },
>> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRT

Re: [PATCH 1/6] block: add bitmap-populate job

On 2/27/20 1:11 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
> Still, if user pass disabled bitmap, it will be invalid immediately after
> job finish.

Yes ... In truth, I really want to augment this job to provide a defined
point-in-time semantic so it can be fully useful in all circumstances.

At the moment, the point-in-time it provides is "at finalize", which may
or may not be the single most useful semantic. (It matches mirror --
because that was easier.)

The other option is "at start" which matches backup, but will require a
new filter and some changes to permissions. That was more invasive, so I
avoided it for now.

(In the interest of a speedy resolution for libvirt, can we add a
critical_moment=[START|FINALIZE] option to this job *later*?)

Re: [PATCH 1/2] iotests: add JobRunner class

On 2/27/20 6:44 AM, Max Reitz wrote:
>> I'll just clean up the logging series I had to do it at a more
>> fundamental level.
> OK.  So you’re looking to basically get VM.qmp() to log automatically if
> necessary?  (or maybe qmp_log() to not log unless necessary)
> 
> Max
> 

Yes. If this series looks good I will just rebase it on top of the
logging series. Then self.logging goes away and the calls to qmp_log et
al just become unconditional.

Then we can enable/disable "all QMP logging" globally instead of
individually throughout the iotests shared codebase.

--js

Re: [PATCH v6 1/9] iotests: do a light delinting




On 2/27/20 7:59 AM, Max Reitz wrote:
> On 27.02.20 01:06, John Snow wrote:
>> This doesn't fix everything in here, but it does help clean up the
>> pylint report considerably.
>>
>> This should be 100% style changes only; the intent is to make pylint
>> more useful by working on establishing a baseline for iotests that we
>> can gate against in the future. This will be important if (when?) we
>> begin adding type hints to our code base.
>>
>> Signed-off-by: John Snow 
>> ---
>>  tests/qemu-iotests/iotests.py | 88 ++-
>>  1 file changed, 45 insertions(+), 43 deletions(-)
> 
> I feel like I’m the wrongest person there is for reviewing a Python
> style-fixing patch, but here I am and so here I go:
> 

No, it's good.

>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 8815052eb5..e8a0ea14fc 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
> 
> [...]
> 
>> @@ -245,8 +243,7 @@ def qemu_nbd_early_pipe(*args):
>>' '.join(qemu_nbd_args + ['--fork'] + 
>> list(args
>>  if exitcode == 0:
>>  return exitcode, ''
>> -else:
>> -return exitcode, subp.communicate()[0]
>> +return exitcode, subp.communicate()[0]
> 
> If we want to make such a change (which I don’t doubt we want), I think
> it should be the other way around: Make the condition “exitcode != 0”,
> so the final return is the return for the successful case.  (Just
> because I think that’s how we usually do it, at least in the qemu code?)
> 
> [...]
> 

Yes, makes sense. I was behaving a little more mechanically.

>> @@ -455,10 +452,9 @@ def file_path(*names, base_dir=test_dir):
>>  def remote_filename(path):
>>  if imgproto == 'file':
>>  return path
>> -elif imgproto == 'ssh':
>> +if imgproto == 'ssh':
> 
> This seems like a weird thing to complain about for a coding style
> check, but whatever.
> 
> (As in, I prefer the elif form)
> 

Honestly, I do too. We can silence the warning instead.

This warning option doesn't like "if return else return" constructs,
preferring instead:

if x:
return 0
return 1

but I have to admit that I often like to see the branches laid out as
branches, too.

Other Pythonistas (Eduardo, Philippe, Markus?) -- strong feelings one
way or the other?

>>  return "ssh://%s@127.0.0.1:22%s" % (os.environ.get('USER'), path)
>> -else:
>> -raise Exception("Protocol %s not supported" % (imgproto))
>> +raise Exception("Protocol %s not supported" % (imgproto))
>>  
>>  class VM(qtest.QEMUQtestMachine):
>>  '''A QEMU VM'''
> 
> [...]
> 
>> @@ -756,12 +750,13 @@ def assert_block_path(self, root, path, expected_node, 
>> graph=None):
>>  assert node is not None, 'Cannot follow path %s%s' % (root, 
>> path)
>>  
>>  try:
>> -node_id = next(edge['child'] for edge in graph['edges'] \
>> - if edge['parent'] == 
>> node['id'] and
>> -edge['name'] == child_name)
>> +node_id = next(edge['child'] for edge in graph['edges']
>> +   if edge['parent'] == node['id'] and
>> +   edge['name'] == child_name)
> 
> I don’t mind the if alignment, but I do mind not aligning the third line
> to the “edge” above it (i.e. the third line is part of the condition, so
> I’d align it to the “if” condition).
> 
> But then again it’s nothing new that I like to disagree with commonly
> agreed-upon Python coding styles, so.
> 
> [...]
> 

OK, that can be addressed by highlighting the sub-expression with
parentheses:

node_id = next(edge['child'] for edge in graph['edges']
   if (edge['parent'] == node['id'] and
   edge['name'] == child_name))


>> @@ -891,13 +892,14 @@ def wait_until_completed(self, drive='drive0', 
>> check_offset=True, wait=60.0,
>>  self.assert_qmp(event, 'data/error', error)
>>  self.assert_no_active_block_jobs()
>>  return event
>> -elif event['event'] == 'JOB_STATUS_CHANGE':
>> +if event['event'] == 'JOB_STATUS_CHANGE':
>>  self.assert_qmp(event, 'data/id', drive)
>>  
>>  def wait_ready(self, drive='drive0'):
>>  '''Wait until a block job BLOCK_JOB_READY event'''
>> -f = {'data': {'type': 'mirror', 'device': drive } }
>> +f = {'data': {'type': 'mirror', 'device': drive}}
>>  event = self.vm.event_wait(name='BLOCK_JOB_READY', match=f)
>> +return event
> 
> Why not just “return self.vm.event_wait…”?
> 

Shrug. Sometimes I name my return variables when working in Python to
give some semantic clue over what exactly I'm even returning.

I can change it; but the docstring will grow to describe what it returns
to re-document the same.

--js

Re: [PATCH v6 2/9] iotests: add script_initialize




On 2/27/20 8:47 AM, Max Reitz wrote:
> On 27.02.20 01:06, John Snow wrote:
>> Like script_main, but doesn't require a single point of entry.
>> Replace all existing initialization sections with this drop-in replacement.
>>
>> This brings debug support to all existing script-style iotests.
>>
>> Signed-off-by: John Snow 
>> ---
>>  tests/qemu-iotests/149|  3 +-
>>  tests/qemu-iotests/194|  4 +-
>>  tests/qemu-iotests/202|  4 +-
>>  tests/qemu-iotests/203|  4 +-
>>  tests/qemu-iotests/206|  2 +-
>>  tests/qemu-iotests/207|  6 ++-
>>  tests/qemu-iotests/208|  2 +-
>>  tests/qemu-iotests/209|  2 +-
>>  tests/qemu-iotests/210|  6 ++-
>>  tests/qemu-iotests/211|  6 ++-
>>  tests/qemu-iotests/212|  6 ++-
>>  tests/qemu-iotests/213|  6 ++-
>>  tests/qemu-iotests/216|  4 +-
>>  tests/qemu-iotests/218|  2 +-
>>  tests/qemu-iotests/219|  2 +-
>>  tests/qemu-iotests/222|  7 ++--
>>  tests/qemu-iotests/224|  4 +-
>>  tests/qemu-iotests/228|  6 ++-
>>  tests/qemu-iotests/234|  4 +-
>>  tests/qemu-iotests/235|  4 +-
>>  tests/qemu-iotests/236|  2 +-
>>  tests/qemu-iotests/237|  2 +-
>>  tests/qemu-iotests/238|  2 +
>>  tests/qemu-iotests/242|  2 +-
>>  tests/qemu-iotests/246|  2 +-
>>  tests/qemu-iotests/248|  2 +-
>>  tests/qemu-iotests/254|  2 +-
>>  tests/qemu-iotests/255|  2 +-
>>  tests/qemu-iotests/256|  2 +-
>>  tests/qemu-iotests/258|  7 ++--
>>  tests/qemu-iotests/260|  4 +-
>>  tests/qemu-iotests/262|  4 +-
>>  tests/qemu-iotests/264|  4 +-
>>  tests/qemu-iotests/277|  2 +
>>  tests/qemu-iotests/280|  8 ++--
>>  tests/qemu-iotests/283|  4 +-
>>  tests/qemu-iotests/iotests.py | 73 +++
>>  37 files changed, 128 insertions(+), 80 deletions(-)
> 
> Reviewed-by: Max Reitz 
> 
> [...]
> 
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index e8a0ea14fc..fdcf8a940c 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
> 
> [...]
> 
>> @@ -1092,13 +1105,18 @@ def execute_unittest(output, verbosity, debug):
>>  
>>  sys.stderr.write(out)
>>  
>> -def execute_test(test_function=None,
>> - supported_fmts=[],
>> - supported_platforms=None,
>> - supported_cache_modes=[], supported_aio_modes={},
>> - unsupported_fmts=[], supported_protocols=[],
>> - unsupported_protocols=[]):
>> -"""Run either unittest or script-style tests."""
>> +def execute_setup_common(supported_fmts: Collection[str] = (),
> 
> First time I see something like this, but I suppose it means any
> collection (i.e. list or tuple in this case) that has str values?
> 
> Max
> 

Yes. Testing the waters for idiomatic type annotations. We chose our
python version to allow us to use them, so I'm pushing on that boundary.

A Collection in this case is a Python ABC that collects the Sized,
Iterable and Container ABCs.

Roughly:
Sized: provides __len__  (len(x))
Iterable: provides __iter__  ("for x in y")
Container: provides __contains__ ("if 'x' in y")

In this case, we want Iterable (to enumerate the atoms) and Sized (to
provide truth-testing that returns False when the collection has a size
of zero.) "Collection" is the nearest available type that describes all
of the desired types of duck, without becoming overly specific for
features of the type we are not using.

More information:
https://docs.python.org/3.6/library/collections.abc.html#module-collections.abc

>> + supported_platforms: Collection[str] = (),
>> + supported_cache_modes: Collection[str] = (),
>> + supported_aio_modes: Collection[str] = (),
>> + unsupported_fmts: Collection[str] = (),
>> + supported_protocols: Collection[str] = (),
>> + unsupported_protocols: Collection[str] = ()) -> 
>> bool:
>

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

2020-03-03 Thread Mark Cave-Ayland

On 02/03/2020 21:40, BALATON Zoltan wrote:

>> I had a quick look at the schematics linked from the page above, and they 
>> confirm
>> that the VIA IDE interface is connected directly to IRQs 14 and 15 and not 
>> to the PCI
>> interrupt pins.
> 
> Where did you see that? What I got from trying to look this up in the 
> schematics was
> that VT8231 has two pins named IRQ14 and IRQ15 (but described as Primary and
> Secondary Channel Interrupt Request in doc) where the interrupt lines of the 
> two IDE
> ports/channels are connected but how they are routed within the chip after 
> that was
> not clear. The chip doc says that in native mode the interrupt should be 
> configurable
> and use a single interrupt for both channels and in legacy mode they use the 
> usual 14
> and 15 but this is not what guest drivers expect so I think not how really 
> works on
> PegasosII. It is true however that connection to PCI interrupts aren't 
> mentioned so
> it always uses ISA IRQ numbers, it just depends on legacy vs. native mode 
> which line
> is raised. But that was never really a question for VT8231 and maybe only 
> CMD646
> could have such interconnection with PCI interrupts. (Proabable reason is that
> via-ide is part of a southbridge chip where it has connections to ISA bus 
> while
> CMD646 is a PCI IDE controller but I could be wrong as my knowledge is 
> limited about
> these.)

Presumably the VIA southbridge has its own internal pair of cascaded 8259s so 
the IRQ
line from the drive is connected to IRQ14/15 as per an typical ISA PC. You can 
see
this in the "Common Hardware Reference Platform: I/O Device Reference" PDF 
section 4.1.

>> So on that basis the best explanation as to what is happening is the
>> comment in the link you provided here:
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/platforms/chrp/pci.c?h=v4.14.172#n353
>>
>>
>> /* Pegasos2 firmware version 20040810 configures the built-in IDE controller
>> * in legacy mode, but sets the PCI registers to PCI native mode.
>> * The chip can only operate in legacy mode, so force the PCI class into 
>> legacy
>> * mode as well. The same fixup must be done to the class-code property in
>> * the IDE node /pci@8000/ide@C,1
>> */
> 
> I'm not sure that it makes much sense that the firmware configures the chip 
> one way
> then puts info about a different way in the device tree. There could be bugs 
> but this
> is not likely. Especially because I see in traces that the firmware does try 
> to
> configure the device in native mode. These are the first few accesses of the 
> firmware
> to via-ide:

But that is exactly what is happening! The comment above clearly indicates the
firmware incorrectly sets the IDE controller in native mode which is in exact
agreement with the trace you provide below. And in fact if you look at
https://www.powerdeveloper.org/platforms/pegasos/devicetree you can see the 
nvramrc
hack that was released in order to fix the device tree to boot Linux which 
alters the
class-code and sets interrupts to 14 (which I believe is invalid, but seemingly 
good
enough here).

> pci_cfg_write via-ide 12:1 @0x9 <- 0xf
> pci_cfg_write via-ide 12:1 @0x40 <- 0xb
> pci_cfg_write via-ide 12:1 @0x41 <- 0xf2
> pci_cfg_write via-ide 12:1 @0x43 <- 0x35
> pci_cfg_write via-ide 12:1 @0x44 <- 0x18
> pci_cfg_write via-ide 12:1 @0x45 <- 0x1c
> pci_cfg_write via-ide 12:1 @0x46 <- 0xc0
> pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717
> pci_cfg_write via-ide 12:1 @0x54 <- 0x14
> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
> pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06
> pci_cfg_read via-ide 12:1 @0xc -> 0x0
> pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4
> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
> pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080
> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
> pci_cfg_write via-ide 12:1 @0x3c <- 0x109
> 
> The very first write is to turn on native mode, so I think it's not about 
> what the
> firmware does but something about how hardware is wired on Pegasos II or the 
> VT8231
> chip itself that only allows legacy interrupts instead of 100% native mode 
> for IDE.
> 
>> Given that the DT is wrong, then we should assume that all OSs would have to
>> compensate for this in the same way as Linux, and therefore this should be 
>> handled
>> automatically.
>>
>> AFAICT this then only leaves the question: why does the firmware set
>> PCI_INTERRUPT_LINE to 9, which is presumably why you are seeing problems 
>> running
>> MorphOS under QEMU.
> 
> Linux does try to handle both true native mode and half-native mode. It only 
> uses
> half-native mode if finds IRQ14 on Pegasos, otherwise skips Pegasos specific 
> fixup
> and uses true native mode setup. I don't know what MorphOS does but I think 
> it justs
> knows that Pegasos2 has this quirk and does not look at the device tree at 
> all.
> 
>> Could it be that setting prog-if to 0x8a legacy mode also resets

Re: [PATCH v6 6/9] iotests: use python logging for iotests.log()




On 2/27/20 9:21 AM, Max Reitz wrote:
> On 27.02.20 01:06, John Snow wrote:
>> We can turn logging on/off globally instead of per-function.
>>
>> Remove use_log from run_job, and use python logging to turn on
>> diffable output when we run through a script entry point.
>>
>> iotest 245 changes output order due to buffering reasons.
>>
>> Signed-off-by: John Snow 
>> ---
>>  tests/qemu-iotests/030|  4 +--
>>  tests/qemu-iotests/245|  1 +
>>  tests/qemu-iotests/245.out| 24 -
>>  tests/qemu-iotests/iotests.py | 50 +--
>>  4 files changed, 45 insertions(+), 34 deletions(-)
> 
> [...]
> 
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index b02d7932fa..60c4c7f736 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -35,6 +35,14 @@
>>  
>>  assert sys.version_info >= (3, 6)
>>  
>> +# Use this logger for logging messages directly from the iotests module
>> +logger = logging.getLogger('qemu.iotests')
>> +logger.addHandler(logging.NullHandler())
> 
> Hm, I never see another handler added to this, so how can these messages
> actually be printed?  Will enabling debug mode somehow make all loggers
> print everything?
> 

Implicit fallback to root logger which handles them when the root logger
is configured, which happens when logging.basicConfig is called.

>> +# Use this logger for messages that ought to be used for diff output.
>> +test_logger = logging.getLogger('qemu.iotests.diff_io')
> 
> Also, why does logger get a null handler and this by default does not?
> I’m asking because test_logger makes it look like you don’t necessarily
> need a handler for output to be silently discarded.
> 
> Max
> 

It's a library trick. By adding a null handler at `qemu.iotests` I add a
default handler to everything produced by iotests. When logging is not
configured, this stops messages from being printed -- there IS a default
"nonconfigured" logging behavior and this stops it.

What I learned since the last time I wrote this patchset is that you
only need a NullHandler at some particular root, so "qemu.iotests"
suffices. "qemu.iotests.diff_io" is a child of that other logger.

>>  # This will not work if arguments contain spaces but is necessary if we
>>  # want to support the override options that ./check supports.
>>  qemu_img_args = [os.environ.get('QEMU_IMG_PROG', 'qemu-img')]
> 

-- 
—js

Re: [PATCH v6 7/9] iotests: ignore import warnings from pylint

On 2/27/20 9:14 AM, Philippe Mathieu-Daudé wrote:
> On 2/27/20 1:06 AM, John Snow wrote:
>> The right way to solve this is to come up with a virtual environment
>> infrastructure that sets all the paths correctly, and/or to create
>> installable python modules that can be imported normally.
>>
>> That's hard, so just silence this error for now.
> 
> I'm tempted to NAck this and require an "installable python module"...
> 
> Let's discuss why it is that hard!
> 

I've been tricked into this before. It's not work I am interested in
doing right now; it's WAY beyond the scope of what I am doing here.

It involves properly factoring all of our python code, deciding which
portions are meant to be installed separately from QEMU itself, coming
up with a versioning scheme, packaging code, and moving code around in
many places.

Then it involves coming up with tooling and infrastructure for creating
virtual environments, installing the right packages to it, and using it
to run our python tests.

No, that's way too invasive. I'm not doing it and I will scream loudly
if you make me.

A less invasive hack involves setting the python import path in a
consolidated spot so that python knows where it can import from. This
works, but might break the ability to run such tests as one-offs without
executing the environment setup.

Again, not work I care to do right now and so I won't. The benefit of
these patches is to provide some minimum viable CI CQA for Python where
we had none before, NOT fix every possible Python problem in one shot.

--js

>>
>> Signed-off-by: John Snow 
>> ---
>>   tests/qemu-iotests/iotests.py | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/tests/qemu-iotests/iotests.py
>> b/tests/qemu-iotests/iotests.py
>> index 60c4c7f736..214f59995e 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -30,6 +30,7 @@
>>   from collections import OrderedDict
>>   from typing import Collection
>>   +# pylint: disable=import-error, wrong-import-position
>>   sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..',
>> 'python'))
>>   from qemu import qtest
>>  
>

Re: [PATCH v6 9/9] iotests: add pylintrc file




On 2/27/20 9:11 AM, Philippe Mathieu-Daudé wrote:
> On 2/27/20 1:06 AM, John Snow wrote:
>> Repeatable results. run `pylint iotests.py` and you should get a pass.
>>
>> Signed-off-by: John Snow 
>> ---
>>   tests/qemu-iotests/pylintrc | 20 
>>   1 file changed, 20 insertions(+)
>>   create mode 100644 tests/qemu-iotests/pylintrc
>>
>> diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
>> new file mode 100644
>> index 00..feed506f75
>> --- /dev/null
>> +++ b/tests/qemu-iotests/pylintrc
>> @@ -0,0 +1,20 @@
>> +[MESSAGES CONTROL]
>> +
>> +# Disable the message, report, category or checker with the given
>> id(s). You
>> +# can either give multiple identifiers separated by comma (,) or put
>> this
>> +# option multiple times (only on the command line, not in the
>> configuration
>> +# file where it should appear only once). You can also use
>> "--disable=all" to
>> +# disable everything first and then reenable specific checks. For
>> example, if
>> +# you want to run only the similarities checker, you can use
>> "--disable=all
>> +# --enable=similarities". If you want to run only the classes
>> checker, but have
>> +# no Warning level messages displayed, use "--disable=all
>> --enable=classes
>> +# --disable=W".
>> +disable=invalid-name,
>> +    missing-docstring,
>> +    line-too-long,
>> +    too-many-lines,
>> +    too-few-public-methods,
>> +    too-many-arguments,
>> +    too-many-locals,
>> +    too-many-branches,
>> +    too-many-public-methods,
>> \ No newline at end of file
>>
> 
> Can you run it in one of the CI jobs?
> 

Tell me where to add it and I will do so.

Re: [PATCH v4 07/11] monitor/hmp: move hmp_snapshot_* to block-hmp-cmds.c hmp_snapshot_blkdev is from GPLv2 version of the hmp-cmds.c thus have to change the licence to GPLv2

Am 30.01.2020 um 13:34 hat Maxim Levitsky geschrieben:
> Signed-off-by: Maxim Levitsky 
> Reviewed-by: Dr. David Alan Gilbert 

Very long subject line. I suppose the license notice should be in the
body instead.

>  block/monitor/block-hmp-cmds.c | 56 --
>  include/block/block-hmp-cmds.h |  4 +++
>  include/monitor/hmp.h  |  3 --
>  monitor/hmp-cmds.c | 47 
>  4 files changed, 58 insertions(+), 52 deletions(-)
> 
> diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
> index 8e8288c2f1..b83687196f 100644
> --- a/block/monitor/block-hmp-cmds.c
> +++ b/block/monitor/block-hmp-cmds.c
> @@ -1,10 +1,13 @@
>  /*
>   * Blockdev HMP commands
>   *
> + *  Authors:
> + *  Anthony Liguori   
> + *
>   * Copyright (c) 2003-2008 Fabrice Bellard
>   *
> - * This work is licensed under the terms of the GNU GPL, version 2 or
> - * later.  See the COPYING file in the top-level directory.
> + * This work is licensed under the terms of the GNU GPL, version 2.
> + * See the COPYING file in the top-level directory.

Please also copy the next paragraph of the license header:

 * Contributions after 2012-01-13 are licensed under the terms of the
 * GNU GPL, version 2 or (at your option) any later version.

Kevin

Re: [PATCH v4 02/11] monitor/hmp: uninline add_init_drive

Am 30.01.2020 um 13:34 hat Maxim Levitsky geschrieben:
> This is only used by hmp_drive_add.
> The code is just a bit shorter this way.
> 
> No functional changes
> 
> Signed-off-by: Maxim Levitsky 
> Reviewed-by: Markus Armbruster 

Shouldn't the subject say "inline" rather than "uninline"?

Kevin

Re: [PATCH v2 00/20] Add qemu-storage-daemon

Am 28.02.2020 um 12:16 hat Stefan Hajnoczi geschrieben:
> On Mon, Feb 24, 2020 at 03:29:48PM +0100, 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 that are not tied to gues devices 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.
> > 
> >   The exception is --chardev, for which not clear design for a QAPIfied
> >   command line exists yet. We'll consider this interface unstable until
> >   we've figured out how to solve it. For now it just uses the same
> >   QemuOpts-based code as the system emulator.
> > 
> > * 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.
> > 
> > The plan is to merge qemu-storage-daemon as an experimental feature with
> > a reduced API stability promise in 5.0.
> > 
> > Kevin Wolf (20):
> >   qemu-storage-daemon: Add barebone tool
> >   stubs: Add arch_type
> >   block: Move system emulator QMP commands to block/qapi-sysemu.c
> >   block: Move common QMP commands to block-core QAPI module
> >   block: Move sysemu QMP commands to QAPI block module
> >   qemu-storage-daemon: Add --blockdev option
> >   qapi: Flatten object-add
> >   qemu-storage-daemon: Add --object option
> >   qemu-storage-daemon: Add --nbd-server option
> >   blockdev-nbd: Boxed argument type for nbd-server-add
> >   qemu-storage-daemon: Add --export option
> >   qemu-storage-daemon: Add main loop
> >   qemu-storage-daemon: Add --chardev option
> >   stubs: Update monitor stubs for qemu-storage-daemon
> >   qapi: Create 'pragma' module
> >   monitor: Create QAPIfied monitor_init()
> >   qmp: Fail gracefully if chardev is already in use
> >   hmp: Fail gracefully if chardev is already in use
> >   monitor: Add allow_hmp parameter to monitor_init()
> >   qemu-storage-daemon: Add --monitor option
> > 
> >  qapi/block-core.json | 730 +--
> >  qapi/block.json  | 512 +++
> >  qapi/control.json|  37 ++
> >  qapi/pragma.json |  24 +
> >  qapi/qapi-schema.json|  25 +-
> >  qapi/qom.json|  12 +-
> >  qapi/transaction.json|   2 +-
> >  configure|   2 +-
> >  include/block/nbd.h  |   1 +
> >  include/monitor/monitor.h|   6 +-
> >  include/qom/object_interfaces.h  |   7 +
> >  include/sysemu/arch_init.h   |   2 +
> >  block/qapi-sysemu.c  | 590 ++
> >  blockdev-nbd.c   |  40 +-
> >  blockdev.c   | 559 
> >  chardev/char.c   |   8 +-
> >  gdbstub.c|   2 +-
> >  hw/block/xen-block.c |  11 +-
> >  monitor/hmp-cmds.c   |  21 +-
> >  monitor/hmp.c|   8 +-
> >  monitor/misc.c   |   2 +
> >  monitor/monitor.c|  86 ++--
> >  monitor/qmp-cmds.c   |   2 +-
> >  monitor/qmp.c|  11 +-
> >  qemu-storage-daemon.c| 340 +
> >  qom/qom-qmp-cmds.c   |  42 +-
> >  stubs/arch_type.c|   4 +
> >  stubs/monitor-core.c |  21 +
> >  stubs/monitor.c  |  17 +-
> >  tests/test-util-sockets.c|   4 +-
> >  scripts/qapi/gen.py  |   5 +

Re: [PATCH v2 1/1] block/nvme: introduce PMR support from NVMe 1.4 spec

2020-03-03 Thread Andrzej Jakowski

On 2/21/20 2:23 PM, Andrzej Jakowski wrote:
> This patch introduces support for PMR that has been defined as part of NVMe 
> 1.4
> spec. User can now specify a pmr_file which will be mmap'ed into qemu address
> space and subsequently in PCI BAR 2. Guest OS can perform mmio read and writes
> to the PMR region that will stay persistent accross system reboot.
> 
> Signed-off-by: Andrzej Jakowski 
> ---
>  hw/block/nvme.c   | 165 +++-
>  hw/block/nvme.h   |   5 ++
>  hw/block/trace-events |   5 ++
>  include/block/nvme.h  | 172 ++
>  4 files changed, 346 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index d28335cbf3..ff7e74d765 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -19,10 +19,14 @@
>   *  -drive file=,if=none,id=
>   *  -device nvme,drive=,serial=,id=, \
>   *  cmb_size_mb=, \
> + *  [pmr_file=,] \
>   *  num_queues=
>   *
>   * Note cmb_size_mb denotes size of CMB in MB. CMB is assumed to be at
>   * offset 0 in BAR2 and supports only WDS, RDS and SQS for now.
> + *
> + * Either cmb or pmr - due to limitation in avaialbe BAR indexes.
> + * pmr_file file needs to be preallocated and power of two in size.
>   */
>  
>  #include "qemu/osdep.h"
> @@ -1141,6 +1145,28 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, 
> uint64_t data,
>  NVME_GUEST_ERR(nvme_ub_mmiowr_cmbsz_readonly,
> "invalid write to read only CMBSZ, ignored");
>  return;
> +#ifndef _WIN32
> +case 0xE00: /* PMRCAP */
> +NVME_GUEST_ERR(nvme_ub_mmiowr_pmrcap_readonly,
> +   "invalid write to PMRCAP register, ignored");
> +return;
> +case 0xE04: /* TODO PMRCTL */
> +break;
> +case 0xE08: /* PMRSTS */
> +NVME_GUEST_ERR(nvme_ub_mmiowr_pmrsts_readonly,
> +   "invalid write to PMRSTS register, ignored");
> +return;
> +case 0xE0C: /* PMREBS */
> +NVME_GUEST_ERR(nvme_ub_mmiowr_pmrebs_readonly,
> +   "invalid write to PMREBS register, ignored");
> +return;
> +case 0xE10: /* PMRSWTP */
> +NVME_GUEST_ERR(nvme_ub_mmiowr_pmrswtp_readonly,
> +   "invalid write to PMRSWTP register, ignored");
> +return;
> +case 0xE14: /* TODO PMRMSC */
> + break;
> +#endif /* !_WIN32 */
>  default:
>  NVME_GUEST_ERR(nvme_ub_mmiowr_invalid,
> "invalid MMIO write,"
> @@ -1169,6 +1195,22 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr 
> addr, unsigned size)
>  }
>  
>  if (addr < sizeof(n->bar)) {
> +#ifndef _WIN32
> +/*
> + * When PMRWBM bit 1 is set then read from
> + * from PMRSTS should ensure prior writes
> + * made it to persistent media
> + */
> +if (addr == 0xE08 &&
> +(NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02) >> 1) {
> +int ret;
> +ret = msync(n->pmrbuf, n->f_pmr_size, MS_SYNC);
> +if (!ret) {
> +NVME_GUEST_ERR(nvme_ub_mmiord_pmrread_barrier,
> +   "error while persisting data");
> +}
> +}
> +#endif /* !_WIN32 */
>  memcpy(&val, ptr + addr, size);
>  } else {
>  NVME_GUEST_ERR(nvme_ub_mmiord_invalid_ofs,
> @@ -1303,6 +1345,31 @@ static const MemoryRegionOps nvme_cmb_ops = {
>  },
>  };
>  
> +#ifndef _WIN32
> +static void nvme_pmr_write(void *opaque, hwaddr addr, uint64_t data,
> +unsigned size)
> +{
> +NvmeCtrl *n = (NvmeCtrl *)opaque;
> +stn_le_p(&n->pmrbuf[addr], size, data);
> +}
> +
> +static uint64_t nvme_pmr_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +NvmeCtrl *n = (NvmeCtrl *)opaque;
> +return ldn_le_p(&n->pmrbuf[addr], size);
> +}
> +
> +static const MemoryRegionOps nvme_pmr_ops = {
> +.read = nvme_pmr_read,
> +.write = nvme_pmr_write,
> +.endianness = DEVICE_LITTLE_ENDIAN,
> +.impl = {
> +.min_access_size = 1,
> +.max_access_size = 8,
> +},
> +};
> +#endif /* !_WIN32 */
> +
>  static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>  {
>  NvmeCtrl *n = NVME(pci_dev);
> @@ -1332,6 +1399,39 @@ static void nvme_realize(PCIDevice *pci_dev, Error 
> **errp)
>  error_setg(errp, "serial property not set");
>  return;
>  }
> +
> +#ifndef _WIN32
> +if (!n->cmb_size_mb && n->pmr_file) {
> +int fd;
> +
> +n->f_pmr = fopen(n->pmr_file, "r+b");
> +if (!n->f_pmr) {
> +error_setg(errp, "pmr backend file open error");
> +return;
> +}
> +
> +fseek(n->f_pmr, 0L, SEEK_END);
> +n->f_pmr_size = ftell(n->f_pmr);
> +fseek(n->f_pmr, 0L, SEEK_SET);
> +
> +/* pmr file size needs to be power of 2 in size */
> +if (!n->f_pmr_size || !is_power_of_2(n->f_pmr_size))

Re: [PATCH 0/2] block/qcow2: Fix bitmap reopen with 'auto-read-only' file child

Am 28.02.2020 um 13:44 hat Peter Krempa geschrieben:
> See patch 2/2 for explanation. Also please excuse the lack of tests
> caused by my ignorance of not figuring out where to put them.

Thanks, applied to the block branch.

A test would probably live in test/qemu-iotests/. Test case 245 is
specifically about x-blockdev-reopen, so we could consider putting it
there, or you could just create a new test case.

Kevin

Re: [PATCH v4 3/5] qcow2: rework the cluster compression routine


03.03.2020 16:34, Denis Plotnikov wrote:

The patch enables processing the image compression type defined
for the image and chooses an appropriate method for image clusters
(de)compression.

Signed-off-by: Denis Plotnikov


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH v5 0/4] qmp: Optionally run handlers in coroutines

Looks like PATCH 2 isn't necessary anymore, and PATCH 4 needs a respin.
I tentatively queued PATCH 1+3, in the hope of getting either PATCH 4 or
Marc-André's screendump patch in time for 5.0.

Pushed to branch monitor-next in git://repo.or.cz/qemu/armbru.git.
Appears to be down right now.

Thanks!

Re: [PATCH v4 4/5] qcow2: add zstd cluster compression

Denis Plotnikov  writes:

> zstd significantly reduces cluster compression time.
> It provides better compression performance maintaining
> the same level of the compression ratio in comparison with
> zlib, which, at the moment, is the only compression
> method available.
>
> The performance test results:
> Test compresses and decompresses qemu qcow2 image with just
> installed rhel-7.6 guest.
> Image cluster size: 64K. Image on disk size: 2.2G
>
> The test was conducted with brd disk to reduce the influence
> of disk subsystem to the test results.
> The results is given in seconds.
>
> compress cmd:
>   time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
>   src.img [zlib|zstd]_compressed.img
> decompress cmd
>   time ./qemu-img convert -O qcow2
>   [zlib|zstd]_compressed.img uncompressed.img
>
>compression   decompression
>  zlib   zstd   zlib zstd
> 
> real 65.5   16.3 (-75 %)1.9  1.6 (-16 %)
> user 65.0   15.85.3  2.5
> sys   3.30.22.0  2.0
>
> Both ZLIB and ZSTD gave the same compression ratio: 1.57
> compressed image size in both cases: 1.4G
>
> Signed-off-by: Denis Plotnikov 
[...]
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index a67eb8bff4..84889fb741 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4401,11 +4401,12 @@
>  # Compression type used in qcow2 image file
>  #
>  # @zlib:  zlib compression, see 
> +# @zstd:  zstd compression, see 
>  #
>  # Since: 5.0
>  ##
>  { 'enum': 'Qcow2CompressionType',
> -  'data': [ 'zlib' ] }
> +  'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
>  
>  ##
>  # @BlockdevCreateOptionsQcow2:

Like MultiFDCompression less value @none.

QAPI part:
Acked-by: Markus Armbruster 

[...]

Re: [PATCH v4 2/5] qcow2: introduce compression type feature


03.03.2020 16:34, Denis Plotnikov wrote:

The patch adds some preparation parts for incompatible compression type
feature to qcow2 allowing the use different compression methods for
image clusters (de)compressing.

It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.

The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.

The default compression is ZLIB. Images created with ZLIB compression type
are backward compatible with older qemu versions.

Adding of the compression type breaks a number of tests because now the
compression type is reported on image creation and there are some changes
in the qcow2 header in size and offsets.

The tests are fixed in the following ways:
 * filter out compression_type for all the tests
 * fix header size, feature table size and backing file offset
   affected tests: 031, 036, 061, 080
   header_size +=8: 1 byte compression type
7 bytes padding
   feature_table += 48: incompatible feture compression type
   backing_file_offset += 56 (8 + 48 -> header_change + fature_table_change)
 * add "compression type" for test output matching when it isn't filtered
   affected tests: 049, 060, 061, 065, 144, 182, 242, 255

Signed-off-by: Denis Plotnikov


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

[PATCH v4 3/5] qcow2: rework the cluster compression routine

The patch enables processing the image compression type defined
for the image and chooses an appropriate method for image clusters
(de)compression.

Signed-off-by: Denis Plotnikov 
---
 block/qcow2-threads.c | 71 ---
 1 file changed, 60 insertions(+), 11 deletions(-)

diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index a68126f291..9bfcda6918 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -74,7 +74,9 @@ typedef struct Qcow2CompressData {
 } Qcow2CompressData;
 
 /*
- * qcow2_compress()
+ * qcow2_zlib_compress()
+ *
+ * Compress @src_size bytes of data using zlib compression method
  *
  * @dest - destination buffer, @dest_size bytes
  * @src - source buffer, @src_size bytes
@@ -83,8 +85,8 @@ typedef struct Qcow2CompressData {
  *  -ENOMEM destination buffer is not enough to store compressed data
  *  -EIOon any other error
  */
-static ssize_t qcow2_compress(void *dest, size_t dest_size,
-  const void *src, size_t src_size)
+static ssize_t qcow2_zlib_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
 {
 ssize_t ret;
 z_stream strm;
@@ -119,10 +121,10 @@ static ssize_t qcow2_compress(void *dest, size_t 
dest_size,
 }
 
 /*
- * qcow2_decompress()
+ * qcow2_zlib_decompress()
  *
  * Decompress some data (not more than @src_size bytes) to produce exactly
- * @dest_size bytes.
+ * @dest_size bytes using zlib compression method
  *
  * @dest - destination buffer, @dest_size bytes
  * @src - source buffer, @src_size bytes
@@ -130,8 +132,8 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
  * Returns: 0 on success
  *  -EIO on fail
  */
-static ssize_t qcow2_decompress(void *dest, size_t dest_size,
-const void *src, size_t src_size)
+static ssize_t qcow2_zlib_decompress(void *dest, size_t dest_size,
+ const void *src, size_t src_size)
 {
 int ret;
 z_stream strm;
@@ -191,20 +193,67 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest, 
size_t dest_size,
 return arg.ret;
 }
 
+/*
+ * qcow2_co_compress()
+ *
+ * Compress @src_size bytes of data using the compression
+ * method defined by the image compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ *  a negative error code on failure
+ */
 ssize_t coroutine_fn
 qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
   const void *src, size_t src_size)
 {
-return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
-qcow2_compress);
+BDRVQcow2State *s = bs->opaque;
+Qcow2CompressFunc fn;
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+fn = qcow2_zlib_compress;
+break;
+
+default:
+abort();
+}
+
+return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
 }
 
+/*
+ * qcow2_co_decompress()
+ *
+ * Decompress some data (not more than @src_size bytes) to produce exactly
+ * @dest_size bytes using the compression method defined by the image
+ * compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: 0 on success
+ *  a negative error code on failure
+ */
 ssize_t coroutine_fn
 qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
 const void *src, size_t src_size)
 {
-return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
-qcow2_decompress);
+BDRVQcow2State *s = bs->opaque;
+Qcow2CompressFunc fn;
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+fn = qcow2_zlib_decompress;
+break;
+
+default:
+return -ENOTSUP;
+}
+
+return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
 }
 
 
-- 
2.17.0

[PATCH v4 4/5] qcow2: add zstd cluster compression

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

The performance test results:
Test compresses and decompresses qemu qcow2 image with just
installed rhel-7.6 guest.
Image cluster size: 64K. Image on disk size: 2.2G

The test was conducted with brd disk to reduce the influence
of disk subsystem to the test results.
The results is given in seconds.

compress cmd:
  time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
  src.img [zlib|zstd]_compressed.img
decompress cmd
  time ./qemu-img convert -O qcow2
  [zlib|zstd]_compressed.img uncompressed.img

   compression   decompression
 zlib   zstd   zlib zstd

real 65.5   16.3 (-75 %)1.9  1.6 (-16 %)
user 65.0   15.85.3  2.5
sys   3.30.22.0  2.0

Both ZLIB and ZSTD gave the same compression ratio: 1.57
compressed image size in both cases: 1.4G

Signed-off-by: Denis Plotnikov 
---
 docs/interop/qcow2.txt |  20 +++
 configure  |   2 +-
 qapi/block-core.json   |   3 +-
 block/qcow2-threads.c  | 123 +
 block/qcow2.c  |   7 +++
 5 files changed, 153 insertions(+), 2 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 5597e24474..9048114445 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -208,6 +208,7 @@ version 2.
 
 Available compression type values:
 0: zlib 
+1: zstd 
 
 
 === Header padding ===
@@ -575,11 +576,30 @@ Compressed Clusters Descriptor (x = 62 - (cluster_bits - 
8)):
 Another compressed cluster may map to the tail of the final
 sector used by this compressed cluster.
 
+The layout of the compressed data depends on the 
compression
+type used for the image (see compressed cluster layout).
+
 If a cluster is unallocated, read requests shall read the data from the backing
 file (except if bit 0 in the Standard Cluster Descriptor is set). If there is
 no backing file or the backing file is smaller than the image, they shall read
 zeros for all parts that are not covered by the backing file.
 
+=== Compressed Cluster Layout ===
+
+The compressed cluster data has a layout depending on the compression
+type used for the image, as follows:
+
+Compressed data layout for the available compression types:
+data_space_lenght - data chunk length available to store a compressed cluster.
+(for more details see "Compressed Clusters Descriptor")
+x = data_space_length - 1
+
+0:  (default)  zlib :
+Byte  0 -  x: the compressed data content
+  all the space provided used for compressed data
+1:  zstd :
+Byte  0 -  3: the length of compressed data in bytes
+  4 -  x: the compressed data content
 
 == Snapshots ==
 
diff --git a/configure b/configure
index caa65f5883..b2a0aa241a 100755
--- a/configure
+++ b/configure
@@ -1835,7 +1835,7 @@ disabled with --disable-FEATURE, default is enabled if 
available:
   lzfse   support of lzfse compression library
   (for reading lzfse-compressed dmg images)
   zstdsupport for zstd compression library
-  (for migration compression)
+  (for migration compression and qcow2 cluster compression)
   seccomp seccomp support
   coroutine-pool  coroutine freelist (better performance)
   glusterfs   GlusterFS backend
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a67eb8bff4..84889fb741 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4401,11 +4401,12 @@
 # Compression type used in qcow2 image file
 #
 # @zlib:  zlib compression, see 
+# @zstd:  zstd compression, see 
 #
 # Since: 5.0
 ##
 { 'enum': 'Qcow2CompressionType',
-  'data': [ 'zlib' ] }
+  'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
 
 ##
 # @BlockdevCreateOptionsQcow2:
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 9bfcda6918..0d09208d27 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -28,6 +28,11 @@
 #define ZLIB_CONST
 #include 
 
+#ifdef CONFIG_ZSTD
+#include 
+#include 
+#endif
+
 #include "qcow2.h"
 #include "block/thread-pool.h"
 #include "crypto.h"
@@ -166,6 +171,114 @@ static ssize_t qcow2_zlib_decompress(void *dest, size_t 
dest_size,
 return ret;
 }
 
+#ifdef CONFIG_ZSTD
+
+#define ZSTD_LE

[PATCH v4 0/5] qcow2: Implement zstd cluster compression method

v4:
   * the series is rebased on top of 01 "block/qcow2-threads: fix 
qcow2_decompress"
   * 01 is just a no-change resend to avoid extra dependencies. Still, it may 
be merged in separate

v3:
   * remove redundant max compression type value check [Vladimir, Eric]
 (the switch below checks everything)
   * prevent compression type changing on "qemu-img amend" [Vladimir]
   * remove zstd config setting, since it has been added already by
 "migration" patches [Vladimir]
   * change the compression type error message [Vladimir] 
   * fix alignment and 80-chars exceeding [Vladimir]

v2:
   * rework compression type setting [Vladimir]
   * squash iotest changes to the compression type introduction patch 
[Vladimir, Eric]
   * fix zstd availability checking in zstd iotest [Vladimir]
   * remove unnecessry casting [Eric]
   * remove rudundant checks [Eric]
   * fix compressed cluster layout in qcow2 spec [Vladimir]
   * fix wording [Eric, Vladimir]
   * fix compression type filtering in iotests [Eric]

v1:
   the initial series

---
zstd comression method is faster than the only available zlib.
The series adds zstd to the methods available for clusters compression.

The implementation is done with respect to the recently added compression
type additional header to the qcow2 specification.


Denis Plotnikov (4):
  qcow2: introduce compression type feature
  qcow2: rework the cluster compression routine
  qcow2: add zstd cluster compression
  iotests: 287: add qcow2 compression type test

Vladimir Sementsov-Ogievskiy (1):
  block/qcow2-threads: fix qcow2_decompress

 docs/interop/qcow2.txt   |  20 +++
 configure|   2 +-
 qapi/block-core.json |  23 +++-
 block/qcow2.h|  18 ++-
 include/block/block_int.h|   1 +
 block/qcow2-threads.c| 206 ---
 block/qcow2.c| 108 
 tests/qemu-iotests/031.out   |  14 +--
 tests/qemu-iotests/036.out   |   4 +-
 tests/qemu-iotests/049.out   | 102 +++
 tests/qemu-iotests/060.out   |   1 +
 tests/qemu-iotests/061.out   |  34 ++---
 tests/qemu-iotests/065   |  28 +++--
 tests/qemu-iotests/080   |   2 +-
 tests/qemu-iotests/144.out   |   4 +-
 tests/qemu-iotests/182.out   |   2 +-
 tests/qemu-iotests/242.out   |   5 +
 tests/qemu-iotests/255.out   |   8 +-
 tests/qemu-iotests/287   | 127 +++
 tests/qemu-iotests/287.out   |  43 +++
 tests/qemu-iotests/common.filter |   3 +-
 tests/qemu-iotests/group |   1 +
 22 files changed, 643 insertions(+), 113 deletions(-)
 create mode 100755 tests/qemu-iotests/287
 create mode 100644 tests/qemu-iotests/287.out

-- 
2.17.0

[PATCH v4 2/5] qcow2: introduce compression type feature

The patch adds some preparation parts for incompatible compression type
feature to qcow2 allowing the use different compression methods for
image clusters (de)compressing.

It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.

The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.

The default compression is ZLIB. Images created with ZLIB compression type
are backward compatible with older qemu versions.

Adding of the compression type breaks a number of tests because now the
compression type is reported on image creation and there are some changes
in the qcow2 header in size and offsets.

The tests are fixed in the following ways:
* filter out compression_type for all the tests
* fix header size, feature table size and backing file offset
  affected tests: 031, 036, 061, 080
  header_size +=8: 1 byte compression type
   7 bytes padding
  feature_table += 48: incompatible feture compression type
  backing_file_offset += 56 (8 + 48 -> header_change + fature_table_change)
* add "compression type" for test output matching when it isn't filtered
  affected tests: 049, 060, 061, 065, 144, 182, 242, 255

Signed-off-by: Denis Plotnikov 
---
 qapi/block-core.json |  22 ++-
 block/qcow2.h|  18 +-
 include/block/block_int.h|   1 +
 block/qcow2.c| 101 ++
 tests/qemu-iotests/031.out   |  14 ++---
 tests/qemu-iotests/036.out   |   4 +-
 tests/qemu-iotests/049.out   | 102 +++
 tests/qemu-iotests/060.out   |   1 +
 tests/qemu-iotests/061.out   |  34 ++-
 tests/qemu-iotests/065   |  28 ++---
 tests/qemu-iotests/080   |   2 +-
 tests/qemu-iotests/144.out   |   4 +-
 tests/qemu-iotests/182.out   |   2 +-
 tests/qemu-iotests/242.out   |   5 ++
 tests/qemu-iotests/255.out   |   8 +--
 tests/qemu-iotests/common.filter |   3 +-
 16 files changed, 253 insertions(+), 96 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 85e27bb61f..a67eb8bff4 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -78,6 +78,8 @@
 #
 # @bitmaps: A list of qcow2 bitmap details (since 4.0)
 #
+# @compression-type: the image cluster compression method (since 5.0)
+#
 # Since: 1.7
 ##
 { 'struct': 'ImageInfoSpecificQCow2',
@@ -89,7 +91,8 @@
   '*corrupt': 'bool',
   'refcount-bits': 'int',
   '*encrypt': 'ImageInfoSpecificQCow2Encryption',
-  '*bitmaps': ['Qcow2BitmapInfo']
+  '*bitmaps': ['Qcow2BitmapInfo'],
+  'compression-type': 'Qcow2CompressionType'
   } }
 
 ##
@@ -4392,6 +4395,18 @@
   'data': [ 'v2', 'v3' ] }
 
 
+##
+# @Qcow2CompressionType:
+#
+# Compression type used in qcow2 image file
+#
+# @zlib:  zlib compression, see 
+#
+# Since: 5.0
+##
+{ 'enum': 'Qcow2CompressionType',
+  'data': [ 'zlib' ] }
+
 ##
 # @BlockdevCreateOptionsQcow2:
 #
@@ -4415,6 +4430,8 @@
 # allowed values: off, falloc, full, metadata)
 # @lazy-refcounts: True if refcounts may be updated lazily (default: off)
 # @refcount-bits: Width of reference counts in bits (default: 16)
+# @compression-type: The image cluster compression method
+#(default: zlib, since 5.0)
 #
 # Since: 2.12
 ##
@@ -4430,7 +4447,8 @@
 '*cluster-size':'size',
 '*preallocation':   'PreallocMode',
 '*lazy-refcounts':  'bool',
-'*refcount-bits':   'int' } }
+'*refcount-bits':   'int',
+'*compression-type':'Qcow2CompressionType' } }
 
 ##
 # @BlockdevCreateOptionsQed:
diff --git a/block/qcow2.h b/block/qcow2.h
index 0942126232..485effcb70 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -146,6 +146,12 @@ typedef struct QCowHeader {
 
 uint32_t refcount_order;
 uint32_t header_length;
+
+/* Additional fields */
+uint8_t  compression_type;
+
+/* header must be a multiple of 8 */
+uint8_t  padding[7];
 } QEMU_PACKED QCowHeader;
 
 typedef struct QEMU_PACKED QCowSnapshotHeader {
@@ -216,13 +222,16 @@ enum {
 QCOW2_INCOMPAT_DIRTY_BITNR  = 0,
 QCOW2_INCOMPAT_CORRUPT_BITNR= 1,
 QCOW2_INCOMPAT_DATA_FILE_BITNR  = 2,
+QCOW2_INCOMPAT_COMPRESSION_BITNR = 3,
 QCOW2_INCOMPAT_DIRTY= 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
 QCOW2_INCOMPAT_CORRUPT  = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
 QCOW2_INCOMPAT_DATA_FILE= 1 << QCOW2_INCOMPAT_DATA_FILE_BITNR,
+QCOW2_INCOMPAT_COMPRESSION  = 1 << QCOW2_INCOMPAT_COMPRESSION_BITNR,
 
 QCOW2_INCOMPAT_MASK = QCOW2_INCOMPAT_DIRTY
 | QCOW2_INCOMPAT_CORRUPT

[PATCH v4 5/5] iotests: 287: add qcow2 compression type test

The test checks fulfilling qcow2 requiriements for the compression
type feature and zstd compression type operability.

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

diff --git a/tests/qemu-iotests/287 b/tests/qemu-iotests/287
new file mode 100755
index 00..39cb665c85
--- /dev/null
+++ b/tests/qemu-iotests/287
@@ -0,0 +1,127 @@
+#!/usr/bin/env bash
+#
+# Test case for an image using zstd compression
+#
+# Copyright (c) 2020 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 .
+#
+
+# creator
+owner=dplotni...@virtuozzo.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# standard environment
+. ./common.rc
+. ./common.filter
+
+# This tests qocw2-specific low-level functionality
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+# Check if we can run this test.
+IMGOPTS='compression_type=zstd'
+
+_make_test_img 64M | grep "Invalid parameter 'zstd'" 2>&1 1>/dev/null
+
+ZSTD_SUPPORTED=$?
+
+if (($ZSTD_SUPPORTED==0)); then
+_notrun "ZSTD is disabled"
+fi
+
+# Test: when compression is zlib the incompatible bit is unset
+echo
+echo "=== Testing compression type incompatible bit setting for zlib ==="
+echo
+
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+
+# Test: when compression differs from zlib the incompatible bit is set
+echo
+echo "=== Testing compression type incompatible bit setting for zstd ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+
+# Test: an image can't be openned if compression type is zlib and
+#   incompatible feature compression type is set
+echo
+echo "=== Testing zlib with incompatible bit set  ==="
+echo
+
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 3
+# to make sure the bit was actually set
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+$QEMU_IMG info "$TEST_IMG" 2>1 1>/dev/null
+if (($?==0)); then
+echo "Error: The image openned successfully. The image must not be openned"
+fi
+
+# Test: an image can't be openned if compression type is NOT zlib and
+#   incompatible feature compression type is UNSET
+echo
+echo "=== Testing zstd with incompatible bit unset  ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" set-header incompatible_features 0
+# to make sure the bit was actually unset
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+$QEMU_IMG info "$TEST_IMG" 2>1 1>/dev/null
+if (($?==0)); then
+echo "Error: The image openned successfully. The image must not be openned"
+fi
+# Test: check compression type values
+echo
+echo "=== Testing compression type values  ==="
+echo
+# zlib=0
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+od -j104 -N1 -An -vtu1 "$TEST_IMG"
+
+# zstd=1
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+od -j104 -N1 -An -vtu1 "$TEST_IMG"
+
+# Test: using zstd compression, write to and read from an image
+echo
+echo "=== Testing reading and writing with zstd ==="
+echo
+
+CLUSTER_SIZE=65536
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$QEMU_IO -c "write -c -P 0xAC 65536 64k " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -P 0xAC 65536 65536 " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -v 131070 8 " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -v 65534 8" "$TEST_IMG" | _filter_qemu_io
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/287.out b/tests/qemu-iotests/287.out
new file mode 100644
index 00..8e51c3078d
--- /dev/null
+++ b/tests/qemu-iotests/287.out
@@ -0,0 +1,43 @@
+QA output created by 287
+
+=== Testing compression type incompatible bit setting for zlib ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+incompatible_features []
+
+=== Testing compression type incom

[PATCH v4 1/5] block/qcow2-threads: fix qcow2_decompress

From: Vladimir Sementsov-Ogievskiy 

On success path we return what inflate() returns instead of 0. And it
most probably works for Z_STREAM_END as it is positive, but is
definitely broken for Z_BUF_ERROR.

While being here, switch to errno return code, to be closer to
qcow2_compress API (and usual expectations).

Revert condition in if to be more positive. Drop dead initialization of
ret.

Cc: qemu-sta...@nongnu.org # v4.0
Fixes: 341926ab83e2b
Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
---
 block/qcow2-threads.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 77bb578cdf..a68126f291 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -128,12 +128,12 @@ static ssize_t qcow2_compress(void *dest, size_t 
dest_size,
  * @src - source buffer, @src_size bytes
  *
  * Returns: 0 on success
- *  -1 on fail
+ *  -EIO on fail
  */
 static ssize_t qcow2_decompress(void *dest, size_t dest_size,
 const void *src, size_t src_size)
 {
-int ret = 0;
+int ret;
 z_stream strm;
 
 memset(&strm, 0, sizeof(strm));
@@ -144,17 +144,19 @@ static ssize_t qcow2_decompress(void *dest, size_t 
dest_size,
 
 ret = inflateInit2(&strm, -12);
 if (ret != Z_OK) {
-return -1;
+return -EIO;
 }
 
 ret = inflate(&strm, Z_FINISH);
-if ((ret != Z_STREAM_END && ret != Z_BUF_ERROR) || strm.avail_out != 0) {
+if ((ret == Z_STREAM_END || ret == Z_BUF_ERROR) && strm.avail_out == 0) {
 /*
  * We approve Z_BUF_ERROR because we need @dest buffer to be filled, 
but
  * @src buffer may be processed partly (because in qcow2 we know size 
of
  * compressed data with precision of one sector)
  */
-ret = -1;
+ret = 0;
+} else {
+ret = -EIO;
 }
 
 inflateEnd(&strm);
-- 
2.17.0

Re: [PATCH v5 1/4] qapi: Add a 'coroutine' flag for commands

Am 03.03.2020 um 09:10 hat Markus Armbruster geschrieben:
> Kevin Wolf  writes:
> 
> > This patch adds a new 'coroutine' flag to QMP command definitions that
> > tells the QMP dispatcher that the command handler is safe to be run in a
> > coroutine.
> >
> > The documentation of the new flag pretends that this flag is already
> > used as intended, which it isn't yet after this patch. We'll implement
> > this in another patch in this series.
> >
> > Signed-off-by: Kevin Wolf 
> > Reviewed-by: Marc-André Lureau 
> > Reviewed-by: Markus Armbruster 
> > ---
> >  docs/devel/qapi-code-gen.txt| 11 +++
> >  include/qapi/qmp/dispatch.h |  1 +
> >  tests/test-qmp-cmds.c   |  4 
> >  scripts/qapi/commands.py| 10 +++---
> >  scripts/qapi/doc.py |  2 +-
> >  scripts/qapi/expr.py| 10 --
> >  scripts/qapi/introspect.py  |  2 +-
> >  scripts/qapi/schema.py  |  9 ++---
> >  tests/qapi-schema/test-qapi.py  |  7 ---
> >  tests/Makefile.include  |  1 +
> >  tests/qapi-schema/oob-coroutine.err |  2 ++
> >  tests/qapi-schema/oob-coroutine.json|  2 ++
> >  tests/qapi-schema/oob-coroutine.out |  0
> >  tests/qapi-schema/qapi-schema-test.json |  1 +
> >  tests/qapi-schema/qapi-schema-test.out  |  2 ++
> >  15 files changed, 51 insertions(+), 13 deletions(-)
> >  create mode 100644 tests/qapi-schema/oob-coroutine.err
> >  create mode 100644 tests/qapi-schema/oob-coroutine.json
> >  create mode 100644 tests/qapi-schema/oob-coroutine.out
> >
> > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> > index 59d6973e1e..df01bd852b 100644
> > --- a/docs/devel/qapi-code-gen.txt
> > +++ b/docs/devel/qapi-code-gen.txt
> > @@ -457,6 +457,7 @@ Syntax:
> >  '*gen': false,
> >  '*allow-oob': true,
> >  '*allow-preconfig': true,
> > +'*coroutine': true,
> >  '*if': COND,
> >  '*features': FEATURES }
> >  
> > @@ -581,6 +582,16 @@ before the machine is built.  It defaults to false.  
> > For example:
> >  QMP is available before the machine is built only when QEMU was
> >  started with --preconfig.
> >  
> > +Member 'coroutine' tells the QMP dispatcher whether the command handler
> > +is safe to be run in a coroutine.  It defaults to false.  If it is true,
> > +the command handler is called from coroutine context and may yield while
> > +waiting for an external event (such as I/O completion) in order to avoid
> > +blocking the guest and other background operations.
> > +
> > +It is an error to specify both 'coroutine': true and 'allow-oob': true
> > +for a command.  (We don't currently have a use case for both together and
> > +without a use case, it's not entirely clear what the semantics should be.)
> 
> The parenthesis is new since v4.  I like its contents.  I'm not fond of
> putting complete sentences in parenthesis.  I'll drop them, if you don't
> mind.

You asked this already in the discussion for v4. Yes, I still agree.

> > +
> >  The optional 'if' member specifies a conditional.  See "Configuring
> >  the schema" below for more on this.
> >  
> [...]
> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > index d7a289eded..95cc006cae 100644
> > --- a/scripts/qapi/expr.py
> > +++ b/scripts/qapi/expr.py
> > @@ -89,10 +89,16 @@ def check_flags(expr, info):
> >  if key in expr and expr[key] is not False:
> >  raise QAPISemError(
> >  info, "flag '%s' may only use false value" % key)
> > -for key in ['boxed', 'allow-oob', 'allow-preconfig']:
> > +for key in ['boxed', 'allow-oob', 'allow-preconfig', 'coroutine']:
> >  if key in expr and expr[key] is not True:
> >  raise QAPISemError(
> >  info, "flag '%s' may only use true value" % key)
> > +if 'allow-oob' in expr and 'coroutine' in expr:
> > +# This is not necessarily a fundamental incompatibility, but we 
> > don't
> > +# have a use case and the desired semantics isn't obvious. The 
> > simplest
> > +# solution is to forbid it until we get a use case for it.
> 
> Appreciated.  I'll word-wrap, if you don't mind.

I still don't agree with comment line width being smaller than code line
width, and think it's in disagreement with CODING_STYLE, but if you
can't help it, adjust the formatting however you like.

> > +raise QAPISemError(info, "flags 'allow-oob' and 'coroutine' "
> > + "are incompatible")
> >  
> >  
> >  def check_if(expr, info, source):
> > @@ -344,7 +350,7 @@ def check_exprs(exprs):
> > ['command'],
> > ['data', 'returns', 'boxed', 'if', 'features',
> >  'gen', 'success-response', 'allow-oob',
> > -'allow-preconfig'])
> > +

Re: [PATCH v5 3/4] qmp: Move dispatcher to a coroutine

Am 03.03.2020 um 09:49 hat Markus Armbruster geschrieben:
> Kevin Wolf  writes:
> 
> > This moves the QMP dispatcher to a coroutine and runs all QMP command
> > handlers that declare 'coroutine': true in coroutine context so they
> > can avoid blocking the main loop while doing I/O or waiting for other
> > events.
> >
> > For commands that are not declared safe to run in a coroutine, the
> > dispatcher drops out of coroutine context by calling the QMP command
> > handler from a bottom half.
> >
> > Signed-off-by: Kevin Wolf 
> > ---
> >  include/qapi/qmp/dispatch.h |   1 +
> >  monitor/monitor-internal.h  |   6 +-
> >  monitor/monitor.c   |  55 +---
> >  monitor/qmp.c   | 122 +++-
> >  qapi/qmp-dispatch.c |  44 -
> >  qapi/qmp-registry.c |   3 +
> >  util/aio-posix.c|   7 ++-
> >  7 files changed, 196 insertions(+), 42 deletions(-)
> >
> > diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> > index d6ce9efc8e..6812e49b5f 100644
> > --- a/include/qapi/qmp/dispatch.h
> > +++ b/include/qapi/qmp/dispatch.h
> > @@ -30,6 +30,7 @@ typedef enum QmpCommandOptions
> >  typedef struct QmpCommand
> >  {
> >  const char *name;
> > +/* Runs in coroutine context if QCO_COROUTINE is set */
> >  QmpCommandFunc *fn;
> >  QmpCommandOptions options;
> >  QTAILQ_ENTRY(QmpCommand) node;
> > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> > index 3e6baba88f..f8123b151a 100644
> > --- a/monitor/monitor-internal.h
> > +++ b/monitor/monitor-internal.h
> > @@ -155,7 +155,9 @@ static inline bool monitor_is_qmp(const Monitor *mon)
> >  
> >  typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList;
> >  extern IOThread *mon_iothread;
> > -extern QEMUBH *qmp_dispatcher_bh;
> > +extern Coroutine *qmp_dispatcher_co;
> > +extern bool qmp_dispatcher_co_shutdown;
> > +extern bool qmp_dispatcher_co_busy;
> >  extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
> >  extern QemuMutex monitor_lock;
> >  extern MonitorList mon_list;
> > @@ -173,7 +175,7 @@ void monitor_fdsets_cleanup(void);
> >  
> >  void qmp_send_response(MonitorQMP *mon, const QDict *rsp);
> >  void monitor_data_destroy_qmp(MonitorQMP *mon);
> > -void monitor_qmp_bh_dispatcher(void *data);
> > +void coroutine_fn monitor_qmp_dispatcher_co(void *data);
> >  
> >  int get_monitor_def(int64_t *pval, const char *name);
> >  void help_cmd(Monitor *mon, const char *name);
> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> > index c1a6c4460f..72d57b5cd2 100644
> > --- a/monitor/monitor.c
> > +++ b/monitor/monitor.c
> > @@ -53,8 +53,32 @@ typedef struct {
> >  /* Shared monitor I/O thread */
> >  IOThread *mon_iothread;
> >  
> > -/* Bottom half to dispatch the requests received from I/O thread */
> > -QEMUBH *qmp_dispatcher_bh;
> > +/* Coroutine to dispatch the requests received from I/O thread */
> > +Coroutine *qmp_dispatcher_co;
> > +
> > +/* Set to true when the dispatcher coroutine should terminate */
> > +bool qmp_dispatcher_co_shutdown;
> > +
> > +/*
> > + * qmp_dispatcher_co_busy is used for synchronisation between the
> > + * monitor thread and the main thread to ensure that the dispatcher
> > + * coroutine never gets scheduled a second time when it's already
> > + * scheduled (scheduling the same coroutine twice is forbidden).
> > + *
> > + * It is true if the coroutine is active and processing requests.
> > + * Additional requests may then be pushed onto a mon->qmp_requests,
> > + * and @qmp_dispatcher_co_shutdown may be set without further ado.
> > + * @qmp_dispatcher_co_busy must not be woken up in this case.
> > + *
> > + * If false, you also have to set @qmp_dispatcher_co_busy to true and
> > + * wake up @qmp_dispatcher_co after pushing the new requests.
> 
> Also after setting @qmp_dispatcher_co_shutdown, right?
> 
> Happy to tweak the comment without a respin.

I understood a shutdown request as just another kind of request, but if
you want the comment to be more detailed, feel free.

Kevin

[PATCH v3 2/4] qcow2: rework the cluster compression routine

The patch enables processing the image compression type defined
for the image and chooses an appropriate method for image clusters
(de)compression.

Signed-off-by: Denis Plotnikov 
---
 block/qcow2-threads.c | 77 +++
 1 file changed, 63 insertions(+), 14 deletions(-)

diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 77bb578cdf..9288a4f852 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -74,7 +74,9 @@ typedef struct Qcow2CompressData {
 } Qcow2CompressData;
 
 /*
- * qcow2_compress()
+ * qcow2_zlib_compress()
+ *
+ * Compress @src_size bytes of data using zlib compression method
  *
  * @dest - destination buffer, @dest_size bytes
  * @src - source buffer, @src_size bytes
@@ -83,8 +85,8 @@ typedef struct Qcow2CompressData {
  *  -ENOMEM destination buffer is not enough to store compressed data
  *  -EIOon any other error
  */
-static ssize_t qcow2_compress(void *dest, size_t dest_size,
-  const void *src, size_t src_size)
+static ssize_t qcow2_zlib_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
 {
 ssize_t ret;
 z_stream strm;
@@ -119,19 +121,19 @@ static ssize_t qcow2_compress(void *dest, size_t 
dest_size,
 }
 
 /*
- * qcow2_decompress()
+ * qcow2_zlib_decompress()
  *
  * Decompress some data (not more than @src_size bytes) to produce exactly
- * @dest_size bytes.
+ * @dest_size bytes using zlib compression method
  *
  * @dest - destination buffer, @dest_size bytes
  * @src - source buffer, @src_size bytes
  *
  * Returns: 0 on success
- *  -1 on fail
+ *  -EIO on failure
  */
-static ssize_t qcow2_decompress(void *dest, size_t dest_size,
-const void *src, size_t src_size)
+static ssize_t qcow2_zlib_decompress(void *dest, size_t dest_size,
+ const void *src, size_t src_size)
 {
 int ret = 0;
 z_stream strm;
@@ -144,7 +146,7 @@ static ssize_t qcow2_decompress(void *dest, size_t 
dest_size,
 
 ret = inflateInit2(&strm, -12);
 if (ret != Z_OK) {
-return -1;
+return -EIO;
 }
 
 ret = inflate(&strm, Z_FINISH);
@@ -154,7 +156,7 @@ static ssize_t qcow2_decompress(void *dest, size_t 
dest_size,
  * @src buffer may be processed partly (because in qcow2 we know size 
of
  * compressed data with precision of one sector)
  */
-ret = -1;
+ret = -EIO;
 }
 
 inflateEnd(&strm);
@@ -189,20 +191,67 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest, 
size_t dest_size,
 return arg.ret;
 }
 
+/*
+ * qcow2_co_compress()
+ *
+ * Compress @src_size bytes of data using the compression
+ * method defined by the image compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ *  a negative error code on failure
+ */
 ssize_t coroutine_fn
 qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
   const void *src, size_t src_size)
 {
-return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
-qcow2_compress);
+BDRVQcow2State *s = bs->opaque;
+Qcow2CompressFunc fn;
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+fn = qcow2_zlib_compress;
+break;
+
+default:
+abort();
+}
+
+return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
 }
 
+/*
+ * qcow2_co_decompress()
+ *
+ * Decompress some data (not more than @src_size bytes) to produce exactly
+ * @dest_size bytes using the compression method defined by the image
+ * compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: 0 on success
+ *  a negative error code on failure
+ */
 ssize_t coroutine_fn
 qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
 const void *src, size_t src_size)
 {
-return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
-qcow2_decompress);
+BDRVQcow2State *s = bs->opaque;
+Qcow2CompressFunc fn;
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+fn = qcow2_zlib_decompress;
+break;
+
+default:
+return -ENOTSUP;
+}
+
+return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
 }
 
 
-- 
2.17.0

[PATCH v3 4/4] iotests: 287: add qcow2 compression type test

The test checks fulfilling qcow2 requiriements for the compression
type feature and zstd compression type operability.

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

diff --git a/tests/qemu-iotests/287 b/tests/qemu-iotests/287
new file mode 100755
index 00..39cb665c85
--- /dev/null
+++ b/tests/qemu-iotests/287
@@ -0,0 +1,127 @@
+#!/usr/bin/env bash
+#
+# Test case for an image using zstd compression
+#
+# Copyright (c) 2020 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 .
+#
+
+# creator
+owner=dplotni...@virtuozzo.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# standard environment
+. ./common.rc
+. ./common.filter
+
+# This tests qocw2-specific low-level functionality
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+# Check if we can run this test.
+IMGOPTS='compression_type=zstd'
+
+_make_test_img 64M | grep "Invalid parameter 'zstd'" 2>&1 1>/dev/null
+
+ZSTD_SUPPORTED=$?
+
+if (($ZSTD_SUPPORTED==0)); then
+_notrun "ZSTD is disabled"
+fi
+
+# Test: when compression is zlib the incompatible bit is unset
+echo
+echo "=== Testing compression type incompatible bit setting for zlib ==="
+echo
+
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+
+# Test: when compression differs from zlib the incompatible bit is set
+echo
+echo "=== Testing compression type incompatible bit setting for zstd ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+
+# Test: an image can't be openned if compression type is zlib and
+#   incompatible feature compression type is set
+echo
+echo "=== Testing zlib with incompatible bit set  ==="
+echo
+
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 3
+# to make sure the bit was actually set
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+$QEMU_IMG info "$TEST_IMG" 2>1 1>/dev/null
+if (($?==0)); then
+echo "Error: The image openned successfully. The image must not be openned"
+fi
+
+# Test: an image can't be openned if compression type is NOT zlib and
+#   incompatible feature compression type is UNSET
+echo
+echo "=== Testing zstd with incompatible bit unset  ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" set-header incompatible_features 0
+# to make sure the bit was actually unset
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+$QEMU_IMG info "$TEST_IMG" 2>1 1>/dev/null
+if (($?==0)); then
+echo "Error: The image openned successfully. The image must not be openned"
+fi
+# Test: check compression type values
+echo
+echo "=== Testing compression type values  ==="
+echo
+# zlib=0
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+od -j104 -N1 -An -vtu1 "$TEST_IMG"
+
+# zstd=1
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+od -j104 -N1 -An -vtu1 "$TEST_IMG"
+
+# Test: using zstd compression, write to and read from an image
+echo
+echo "=== Testing reading and writing with zstd ==="
+echo
+
+CLUSTER_SIZE=65536
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$QEMU_IO -c "write -c -P 0xAC 65536 64k " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -P 0xAC 65536 65536 " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -v 131070 8 " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -v 65534 8" "$TEST_IMG" | _filter_qemu_io
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/287.out b/tests/qemu-iotests/287.out
new file mode 100644
index 00..8e51c3078d
--- /dev/null
+++ b/tests/qemu-iotests/287.out
@@ -0,0 +1,43 @@
+QA output created by 287
+
+=== Testing compression type incompatible bit setting for zlib ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+incompatible_features []
+
+=== Testing compression type incom

[PATCH v3 0/4] qcow2: Implement zstd cluster compression method

v3:
   * remove redundant max compression type value check [Vladimir, Eric]
 (the switch below checks everything)
   * prevent compression type changing on "qemu-img amend" [Vladimir]
   * remove zstd config setting, since it has been added already by
 "migration" patches [Vladimir]
   * change the compression type error message [Vladimir] 
   * fix alignment and 80-chars exceeding [Vladimir]

v2:
   * rework compression type setting [Vladimir]
   * squash iotest changes to the compression type introduction patch 
[Vladimir, Eric]
   * fix zstd availability checking in zstd iotest [Vladimir]
   * remove unnecessry casting [Eric]
   * remove rudundant checks [Eric]
   * fix compressed cluster layout in qcow2 spec [Vladimir]
   * fix wording [Eric, Vladimir]
   * fix compression type filtering in iotests [Eric]

v1:
   the initial series

---
zstd comression method is faster than the only available zlib.
The series adds zstd to the methods available for clusters compression.

The implementation is done with respect to the recently added compression
type additional header to the qcow2 specification.

Denis Plotnikov (4):
  qcow2: introduce compression type feature
  qcow2: rework the cluster compression routine
  qcow2: add zstd cluster compression
  iotests: 287: add qcow2 compression type test

 docs/interop/qcow2.txt   |  20 
 configure|   2 +-
 qapi/block-core.json |  23 +++-
 block/qcow2.h|  18 ++-
 include/block/block_int.h|   1 +
 block/qcow2-threads.c| 200 ---
 block/qcow2.c| 108 +
 tests/qemu-iotests/031.out   |  14 +--
 tests/qemu-iotests/036.out   |   4 +-
 tests/qemu-iotests/049.out   | 102 
 tests/qemu-iotests/060.out   |   1 +
 tests/qemu-iotests/061.out   |  34 +++---
 tests/qemu-iotests/065   |  28 +++--
 tests/qemu-iotests/080   |   2 +-
 tests/qemu-iotests/144.out   |   4 +-
 tests/qemu-iotests/182.out   |   2 +-
 tests/qemu-iotests/242.out   |   5 +
 tests/qemu-iotests/255.out   |   8 +-
 tests/qemu-iotests/287   | 127 
 tests/qemu-iotests/287.out   |  43 +++
 tests/qemu-iotests/common.filter |   3 +-
 tests/qemu-iotests/group |   1 +
 22 files changed, 639 insertions(+), 111 deletions(-)
 create mode 100755 tests/qemu-iotests/287
 create mode 100644 tests/qemu-iotests/287.out

-- 
2.17.0

[PATCH v3 1/4] qcow2: introduce compression type feature

The patch adds some preparation parts for incompatible compression type
feature to qcow2 allowing the use different compression methods for
image clusters (de)compressing.

It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.

The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.

The default compression is ZLIB. Images created with ZLIB compression type
are backward compatible with older qemu versions.

Adding of the compression type breaks a number of tests because now the
compression type is reported on image creation and there are some changes
in the qcow2 header in size and offsets.

The tests are fixed in the following ways:
* filter out compression_type for all the tests
* fix header size, feature table size and backing file offset
  affected tests: 031, 036, 061, 080
  header_size +=8: 1 byte compression type
   7 bytes padding
  feature_table += 48: incompatible feture compression type
  backing_file_offset += 56 (8 + 48 -> header_change + fature_table_change)
* add "compression type" for test output matching when it isn't filtered
  affected tests: 049, 060, 061, 065, 144, 182, 242, 255

Signed-off-by: Denis Plotnikov 
---
 qapi/block-core.json |  22 ++-
 block/qcow2.h|  18 +-
 include/block/block_int.h|   1 +
 block/qcow2.c| 101 ++
 tests/qemu-iotests/031.out   |  14 ++---
 tests/qemu-iotests/036.out   |   4 +-
 tests/qemu-iotests/049.out   | 102 +++
 tests/qemu-iotests/060.out   |   1 +
 tests/qemu-iotests/061.out   |  34 ++-
 tests/qemu-iotests/065   |  28 ++---
 tests/qemu-iotests/080   |   2 +-
 tests/qemu-iotests/144.out   |   4 +-
 tests/qemu-iotests/182.out   |   2 +-
 tests/qemu-iotests/242.out   |   5 ++
 tests/qemu-iotests/255.out   |   8 +--
 tests/qemu-iotests/common.filter |   3 +-
 16 files changed, 253 insertions(+), 96 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 85e27bb61f..a67eb8bff4 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -78,6 +78,8 @@
 #
 # @bitmaps: A list of qcow2 bitmap details (since 4.0)
 #
+# @compression-type: the image cluster compression method (since 5.0)
+#
 # Since: 1.7
 ##
 { 'struct': 'ImageInfoSpecificQCow2',
@@ -89,7 +91,8 @@
   '*corrupt': 'bool',
   'refcount-bits': 'int',
   '*encrypt': 'ImageInfoSpecificQCow2Encryption',
-  '*bitmaps': ['Qcow2BitmapInfo']
+  '*bitmaps': ['Qcow2BitmapInfo'],
+  'compression-type': 'Qcow2CompressionType'
   } }
 
 ##
@@ -4392,6 +4395,18 @@
   'data': [ 'v2', 'v3' ] }
 
 
+##
+# @Qcow2CompressionType:
+#
+# Compression type used in qcow2 image file
+#
+# @zlib:  zlib compression, see 
+#
+# Since: 5.0
+##
+{ 'enum': 'Qcow2CompressionType',
+  'data': [ 'zlib' ] }
+
 ##
 # @BlockdevCreateOptionsQcow2:
 #
@@ -4415,6 +4430,8 @@
 # allowed values: off, falloc, full, metadata)
 # @lazy-refcounts: True if refcounts may be updated lazily (default: off)
 # @refcount-bits: Width of reference counts in bits (default: 16)
+# @compression-type: The image cluster compression method
+#(default: zlib, since 5.0)
 #
 # Since: 2.12
 ##
@@ -4430,7 +4447,8 @@
 '*cluster-size':'size',
 '*preallocation':   'PreallocMode',
 '*lazy-refcounts':  'bool',
-'*refcount-bits':   'int' } }
+'*refcount-bits':   'int',
+'*compression-type':'Qcow2CompressionType' } }
 
 ##
 # @BlockdevCreateOptionsQed:
diff --git a/block/qcow2.h b/block/qcow2.h
index 0942126232..485effcb70 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -146,6 +146,12 @@ typedef struct QCowHeader {
 
 uint32_t refcount_order;
 uint32_t header_length;
+
+/* Additional fields */
+uint8_t  compression_type;
+
+/* header must be a multiple of 8 */
+uint8_t  padding[7];
 } QEMU_PACKED QCowHeader;
 
 typedef struct QEMU_PACKED QCowSnapshotHeader {
@@ -216,13 +222,16 @@ enum {
 QCOW2_INCOMPAT_DIRTY_BITNR  = 0,
 QCOW2_INCOMPAT_CORRUPT_BITNR= 1,
 QCOW2_INCOMPAT_DATA_FILE_BITNR  = 2,
+QCOW2_INCOMPAT_COMPRESSION_BITNR = 3,
 QCOW2_INCOMPAT_DIRTY= 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
 QCOW2_INCOMPAT_CORRUPT  = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
 QCOW2_INCOMPAT_DATA_FILE= 1 << QCOW2_INCOMPAT_DATA_FILE_BITNR,
+QCOW2_INCOMPAT_COMPRESSION  = 1 << QCOW2_INCOMPAT_COMPRESSION_BITNR,
 
 QCOW2_INCOMPAT_MASK = QCOW2_INCOMPAT_DIRTY
 | QCOW2_INCOMPAT_CORRUPT

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

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

The performance test results:
Test compresses and decompresses qemu qcow2 image with just
installed rhel-7.6 guest.
Image cluster size: 64K. Image on disk size: 2.2G

The test was conducted with brd disk to reduce the influence
of disk subsystem to the test results.
The results is given in seconds.

compress cmd:
  time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
  src.img [zlib|zstd]_compressed.img
decompress cmd
  time ./qemu-img convert -O qcow2
  [zlib|zstd]_compressed.img uncompressed.img

   compression   decompression
 zlib   zstd   zlib zstd

real 65.5   16.3 (-75 %)1.9  1.6 (-16 %)
user 65.0   15.85.3  2.5
sys   3.30.22.0  2.0

Both ZLIB and ZSTD gave the same compression ratio: 1.57
compressed image size in both cases: 1.4G

Signed-off-by: Denis Plotnikov 
---
 docs/interop/qcow2.txt |  20 +++
 configure  |   2 +-
 qapi/block-core.json   |   3 +-
 block/qcow2-threads.c  | 123 +
 block/qcow2.c  |   7 +++
 5 files changed, 153 insertions(+), 2 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 5597e24474..9048114445 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -208,6 +208,7 @@ version 2.
 
 Available compression type values:
 0: zlib 
+1: zstd 
 
 
 === Header padding ===
@@ -575,11 +576,30 @@ Compressed Clusters Descriptor (x = 62 - (cluster_bits - 
8)):
 Another compressed cluster may map to the tail of the final
 sector used by this compressed cluster.
 
+The layout of the compressed data depends on the 
compression
+type used for the image (see compressed cluster layout).
+
 If a cluster is unallocated, read requests shall read the data from the backing
 file (except if bit 0 in the Standard Cluster Descriptor is set). If there is
 no backing file or the backing file is smaller than the image, they shall read
 zeros for all parts that are not covered by the backing file.
 
+=== Compressed Cluster Layout ===
+
+The compressed cluster data has a layout depending on the compression
+type used for the image, as follows:
+
+Compressed data layout for the available compression types:
+data_space_lenght - data chunk length available to store a compressed cluster.
+(for more details see "Compressed Clusters Descriptor")
+x = data_space_length - 1
+
+0:  (default)  zlib :
+Byte  0 -  x: the compressed data content
+  all the space provided used for compressed data
+1:  zstd :
+Byte  0 -  3: the length of compressed data in bytes
+  4 -  x: the compressed data content
 
 == Snapshots ==
 
diff --git a/configure b/configure
index caa65f5883..b2a0aa241a 100755
--- a/configure
+++ b/configure
@@ -1835,7 +1835,7 @@ disabled with --disable-FEATURE, default is enabled if 
available:
   lzfse   support of lzfse compression library
   (for reading lzfse-compressed dmg images)
   zstdsupport for zstd compression library
-  (for migration compression)
+  (for migration compression and qcow2 cluster compression)
   seccomp seccomp support
   coroutine-pool  coroutine freelist (better performance)
   glusterfs   GlusterFS backend
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a67eb8bff4..84889fb741 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4401,11 +4401,12 @@
 # Compression type used in qcow2 image file
 #
 # @zlib:  zlib compression, see 
+# @zstd:  zstd compression, see 
 #
 # Since: 5.0
 ##
 { 'enum': 'Qcow2CompressionType',
-  'data': [ 'zlib' ] }
+  'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
 
 ##
 # @BlockdevCreateOptionsQcow2:
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 9288a4f852..1020de48dd 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -28,6 +28,11 @@
 #define ZLIB_CONST
 #include 
 
+#ifdef CONFIG_ZSTD
+#include 
+#include 
+#endif
+
 #include "qcow2.h"
 #include "block/thread-pool.h"
 #include "crypto.h"
@@ -164,6 +169,114 @@ static ssize_t qcow2_zlib_decompress(void *dest, size_t 
dest_size,
 return ret;
 }
 
+#ifdef CONFIG_ZSTD
+
+#define ZSTD_LE

Re: QAPI schema for desired state of LUKS keyslots (was: [PATCH 02/13] qcrypto-luks: implement encryption key management)

2020-03-03 Thread Maxim Levitsky

On Sat, 2020-02-15 at 15:51 +0100, Markus Armbruster wrote:
> Review of this patch led to a lengthy QAPI schema design discussion.
> Let me try to condense it into a concrete proposal.
> 
> This is about the QAPI schema, and therefore about QMP.  The
> human-friendly interface is out of scope.  Not because it's not
> important (it clearly is!), only because we need to *focus* to have a
> chance at success.
> 
> I'm going to include a few design options.  I'll mark them "Option:".
> 
> The proposed "amend" interface takes a specification of desired state,
> and figures out how to get from here to there by itself.  LUKS keyslots
> are one part of desired state.
> 
> We commonly have eight LUKS keyslots.  Each keyslot is either active or
> inactive.  An active keyslot holds a secret.
> 
> Goal: a QAPI type for specifying desired state of LUKS keyslots.
> 
> Proposal:
> 
> { 'enum': 'LUKSKeyslotState',
>   'data': [ 'active', 'inactive' ] }
> 
> { 'struct': 'LUKSKeyslotActive',
>   'data': { 'secret': 'str',
> '*iter-time': 'int } }
> 
> { 'struct': 'LUKSKeyslotInactive',
>   'data': { '*old-secret': 'str' } }
> 
> { 'union': 'LUKSKeyslotAmend',
>   'base': { '*keyslot': 'int',
> 'state': 'LUKSKeyslotState' }
>   'discriminator': 'state',
>   'data': { 'active': 'LUKSKeyslotActive',
> 'inactive': 'LUKSKeyslotInactive' } }
> 
> LUKSKeyslotAmend specifies desired state for a set of keyslots.
> 
> Four cases:
> 
> * @state is "active"
> 
>   Desired state is active holding the secret given by @secret.  Optional
>   @iter-time tweaks key stretching.
> 
>   The keyslot is chosen either by the user or by the system, as follows:
> 
>   - @keyslot absent
> 
> One inactive keyslot chosen by the system.  If none exists, error.
> 
>   - @keyslot present
> 
> The keyslot given by @keyslot.
> 
> If it's already active holding @secret, no-op.  Rationale: the
> current state is the desired state.
> 
> If it's already active holding another secret, error.  Rationale:
> update in place is unsafe.
> 
> Option: delete the "already active holding @secret" case.  Feels
> inelegant to me.  Okay if it makes things substantially simpler.
> 
> * @state is "inactive"
> 
>   Desired state is inactive.
> 
>   Error if the current state has active keyslots, but the desired state
>   has none.
> 
>   The user choses the keyslot by number and/or by the secret it holds,
>   as follows:
> 
>   - @keyslot absent, @old-secret present
> 
> All active keyslots holding @old-secret.  If none exists, error.
> 
>   - @keyslot present, @old-secret absent
> 
> The keyslot given by @keyslot.
> 
> If it's already inactive, no-op.  Rationale: the current state is
> the desired state.
> 
>   - both @keyslot and @old-secret present
> 
> The keyslot given by keyslot.
> 
> If it's inactive or holds a secret other than @old-secret, error.
> 
> Option: error regardless of @old-secret, if that makes things
> simpler.
> 
>   - neither @keyslot not @old-secret present
> 
> All keyslots.  Note that this will error out due to "desired state
> has no active keyslots" unless the current state has none, either.
> 
> Option: error out unconditionally.
> 
> Note that LUKSKeyslotAmend can specify only one desired state for
> commonly just one keyslot.  Rationale: this satisfies practical needs.
> An array of LUKSKeyslotAmend could specify desired state for all
> keyslots.  However, multiple array elements could then apply to the same
> slot.  We'd have to specify how to resolve such conflicts, and we'd have
> to code up conflict detection.  Not worth it.
> 
> Examples:
> 
> * Add a secret to some free keyslot:
> 
>   { "state": "active", "secret": "CIA/GRU/MI6" }
> 
> * Deactivate all keyslots holding a secret:
> 
>   { "state": "inactive", "old-secret": "CIA/GRU/MI6" }
> 
> * Add a secret to a specific keyslot:
> 
>   { "state": "active", "secret": "CIA/GRU/MI6", "keyslot": 0 }
> 
> * Deactivate a specific keyslot:
> 
>   { "state": "inactive", "keyslot": 0 }
> 
>   Possibly less dangerous:
> 
>   { "state": "inactive", "keyslot": 0, "old-secret": "CIA/GRU/MI6" }
> 
> Option: Make use of Max's patches to support optional union tag with
> default value to let us default @state to "active".  I doubt this makes
> much of a difference in QMP.  A human-friendly interface should probably
> be higher level anyway (Daniel pointed to cryptsetup).
> 
> Option: LUKSKeyslotInactive member @old-secret could also be named
> @secret.  I don't care.
> 
> Option: delete @keyslot.  It provides low-level slot access.
> Complicates the interface.  Fine if we need lov-level slot access.  Do
> we?
> 
> I apologize for the time it has taken me to write this.
> 
> Comments?

I tried today to implement this but I hit a very unpleasant roadblock:

Since QCrypto is generic (even though it only implements currently luks for 
raw/qcow2 us

Re: [PATCH v5 3/4] qmp: Move dispatcher to a coroutine

Kevin Wolf  writes:

> This moves the QMP dispatcher to a coroutine and runs all QMP command
> handlers that declare 'coroutine': true in coroutine context so they
> can avoid blocking the main loop while doing I/O or waiting for other
> events.
>
> For commands that are not declared safe to run in a coroutine, the
> dispatcher drops out of coroutine context by calling the QMP command
> handler from a bottom half.
>
> Signed-off-by: Kevin Wolf 
> ---
>  include/qapi/qmp/dispatch.h |   1 +
>  monitor/monitor-internal.h  |   6 +-
>  monitor/monitor.c   |  55 +---
>  monitor/qmp.c   | 122 +++-
>  qapi/qmp-dispatch.c |  44 -
>  qapi/qmp-registry.c |   3 +
>  util/aio-posix.c|   7 ++-
>  7 files changed, 196 insertions(+), 42 deletions(-)
>
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index d6ce9efc8e..6812e49b5f 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.h
> @@ -30,6 +30,7 @@ typedef enum QmpCommandOptions
>  typedef struct QmpCommand
>  {
>  const char *name;
> +/* Runs in coroutine context if QCO_COROUTINE is set */
>  QmpCommandFunc *fn;
>  QmpCommandOptions options;
>  QTAILQ_ENTRY(QmpCommand) node;
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index 3e6baba88f..f8123b151a 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -155,7 +155,9 @@ static inline bool monitor_is_qmp(const Monitor *mon)
>  
>  typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList;
>  extern IOThread *mon_iothread;
> -extern QEMUBH *qmp_dispatcher_bh;
> +extern Coroutine *qmp_dispatcher_co;
> +extern bool qmp_dispatcher_co_shutdown;
> +extern bool qmp_dispatcher_co_busy;
>  extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
>  extern QemuMutex monitor_lock;
>  extern MonitorList mon_list;
> @@ -173,7 +175,7 @@ void monitor_fdsets_cleanup(void);
>  
>  void qmp_send_response(MonitorQMP *mon, const QDict *rsp);
>  void monitor_data_destroy_qmp(MonitorQMP *mon);
> -void monitor_qmp_bh_dispatcher(void *data);
> +void coroutine_fn monitor_qmp_dispatcher_co(void *data);
>  
>  int get_monitor_def(int64_t *pval, const char *name);
>  void help_cmd(Monitor *mon, const char *name);
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index c1a6c4460f..72d57b5cd2 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -53,8 +53,32 @@ typedef struct {
>  /* Shared monitor I/O thread */
>  IOThread *mon_iothread;
>  
> -/* Bottom half to dispatch the requests received from I/O thread */
> -QEMUBH *qmp_dispatcher_bh;
> +/* Coroutine to dispatch the requests received from I/O thread */
> +Coroutine *qmp_dispatcher_co;
> +
> +/* Set to true when the dispatcher coroutine should terminate */
> +bool qmp_dispatcher_co_shutdown;
> +
> +/*
> + * qmp_dispatcher_co_busy is used for synchronisation between the
> + * monitor thread and the main thread to ensure that the dispatcher
> + * coroutine never gets scheduled a second time when it's already
> + * scheduled (scheduling the same coroutine twice is forbidden).
> + *
> + * It is true if the coroutine is active and processing requests.
> + * Additional requests may then be pushed onto a mon->qmp_requests,
> + * and @qmp_dispatcher_co_shutdown may be set without further ado.
> + * @qmp_dispatcher_co_busy must not be woken up in this case.
> + *
> + * If false, you also have to set @qmp_dispatcher_co_busy to true and
> + * wake up @qmp_dispatcher_co after pushing the new requests.

Also after setting @qmp_dispatcher_co_shutdown, right?

Happy to tweak the comment without a respin.

> + *
> + * The coroutine will automatically change this variable back to false
> + * before it yields.  Nobody else may set the variable to false.
> + *
> + * Access must be atomic for thread safety.
> + */
> +bool qmp_dispatcher_co_busy;
>  
>  /* Protects mon_list, monitor_qapi_event_state, monitor_destroyed.  */
>  QemuMutex monitor_lock;
[...]

Reviewed-by: Markus Armbruster

Re: [PATCH v7 00/11] error: auto propagated local_err part I


03.03.2020 11:01, Markus Armbruster wrote:

Hi Vladimir,

I've come to rather like your ERRP_AUTO_PROPAGATE() idea.  What I
wouldn't like is a protracted conversion.

Once we're happy with PATCH 1-3, it's a matter of running Coccinelle and
reviewing its output.  I'm confident we can converge on PATCH 1-3.

It's two weeks until soft freeze.  We need to decide whether to pursue a
partial conversion for 5.0 (basically this series plus the two patches
we identified in review of PATCH 1), or delay until 5.1.  In either
case, I want the conversion to be finished in 5.1.

Please do not feel pressured to make the 5.0 deadline.

I can queue up patches for 5.1 during the freeze.

How would you like to proceed?



Hi Markus! Funny coincidence: exactly now (less than 1 hour ago), I've
started working for the next version for these series. So, I'm going to
resend today. Of course, I'd prefer to merge something to 5.0 if at all
possible.


--
Best regards,
Vladimir

Re: [PATCH v5 1/4] qapi: Add a 'coroutine' flag for commands

Kevin Wolf  writes:

> This patch adds a new 'coroutine' flag to QMP command definitions that
> tells the QMP dispatcher that the command handler is safe to be run in a
> coroutine.
>
> The documentation of the new flag pretends that this flag is already
> used as intended, which it isn't yet after this patch. We'll implement
> this in another patch in this series.
>
> Signed-off-by: Kevin Wolf 
> Reviewed-by: Marc-André Lureau 
> Reviewed-by: Markus Armbruster 
> ---
>  docs/devel/qapi-code-gen.txt| 11 +++
>  include/qapi/qmp/dispatch.h |  1 +
>  tests/test-qmp-cmds.c   |  4 
>  scripts/qapi/commands.py| 10 +++---
>  scripts/qapi/doc.py |  2 +-
>  scripts/qapi/expr.py| 10 --
>  scripts/qapi/introspect.py  |  2 +-
>  scripts/qapi/schema.py  |  9 ++---
>  tests/qapi-schema/test-qapi.py  |  7 ---
>  tests/Makefile.include  |  1 +
>  tests/qapi-schema/oob-coroutine.err |  2 ++
>  tests/qapi-schema/oob-coroutine.json|  2 ++
>  tests/qapi-schema/oob-coroutine.out |  0
>  tests/qapi-schema/qapi-schema-test.json |  1 +
>  tests/qapi-schema/qapi-schema-test.out  |  2 ++
>  15 files changed, 51 insertions(+), 13 deletions(-)
>  create mode 100644 tests/qapi-schema/oob-coroutine.err
>  create mode 100644 tests/qapi-schema/oob-coroutine.json
>  create mode 100644 tests/qapi-schema/oob-coroutine.out
>
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index 59d6973e1e..df01bd852b 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -457,6 +457,7 @@ Syntax:
>  '*gen': false,
>  '*allow-oob': true,
>  '*allow-preconfig': true,
> +'*coroutine': true,
>  '*if': COND,
>  '*features': FEATURES }
>  
> @@ -581,6 +582,16 @@ before the machine is built.  It defaults to false.  For 
> example:
>  QMP is available before the machine is built only when QEMU was
>  started with --preconfig.
>  
> +Member 'coroutine' tells the QMP dispatcher whether the command handler
> +is safe to be run in a coroutine.  It defaults to false.  If it is true,
> +the command handler is called from coroutine context and may yield while
> +waiting for an external event (such as I/O completion) in order to avoid
> +blocking the guest and other background operations.
> +
> +It is an error to specify both 'coroutine': true and 'allow-oob': true
> +for a command.  (We don't currently have a use case for both together and
> +without a use case, it's not entirely clear what the semantics should be.)

The parenthesis is new since v4.  I like its contents.  I'm not fond of
putting complete sentences in parenthesis.  I'll drop them, if you don't
mind.

> +
>  The optional 'if' member specifies a conditional.  See "Configuring
>  the schema" below for more on this.
>  
[...]
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index d7a289eded..95cc006cae 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -89,10 +89,16 @@ def check_flags(expr, info):
>  if key in expr and expr[key] is not False:
>  raise QAPISemError(
>  info, "flag '%s' may only use false value" % key)
> -for key in ['boxed', 'allow-oob', 'allow-preconfig']:
> +for key in ['boxed', 'allow-oob', 'allow-preconfig', 'coroutine']:
>  if key in expr and expr[key] is not True:
>  raise QAPISemError(
>  info, "flag '%s' may only use true value" % key)
> +if 'allow-oob' in expr and 'coroutine' in expr:
> +# This is not necessarily a fundamental incompatibility, but we don't
> +# have a use case and the desired semantics isn't obvious. The 
> simplest
> +# solution is to forbid it until we get a use case for it.

Appreciated.  I'll word-wrap, if you don't mind.

> +raise QAPISemError(info, "flags 'allow-oob' and 'coroutine' "
> + "are incompatible")
>  
>  
>  def check_if(expr, info, source):
> @@ -344,7 +350,7 @@ def check_exprs(exprs):
> ['command'],
> ['data', 'returns', 'boxed', 'if', 'features',
>  'gen', 'success-response', 'allow-oob',
> -'allow-preconfig'])
> +'allow-preconfig', 'coroutine'])
>  normalize_members(expr.get('data'))
>  check_command(expr, info)
>  elif meta == 'event':
[...]

R-by stands.

Re: [PATCH v7 00/11] error: auto propagated local_err part I