Re: [PATCH 7/9] accel/kvm: avoid using predefined PAGE_SIZE

[Thomas Huth]

On 21/12/2020 01.53, Jiaxun Yang wrote:

As per POSIX specification of limits.h [1], OS libc may define
PAGE_SIZE in limits.h.

To prevent collosion of definition, we discard PAGE_SIZE from
defined by libc and take QEMU's variable.

[1]: https://pubs.opengroup.org/onlinepubs/7908799/xsh/limits.h.html

Signed-off-by: Jiaxun Yang 
---
  accel/kvm/kvm-all.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 389eaace72..3feb17d965 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -58,6 +58,9 @@
  /* KVM uses PAGE_SIZE in its definition of KVM_COALESCED_MMIO_MAX. We
   * need to use the real host PAGE_SIZE, as that's what KVM will use.
   */
+#ifdef PAGE_SIZE
+#undef PAGE_SIZE
+#endif
  #define PAGE_SIZE qemu_real_host_page_size


If I get that right, the PAGE_SIZE macro is only used one time in this 
file... so it's maybe easier to get rid of the macro completely and replace 
the single occurance with qemu_real_host_page_size directly?


 Thomas

Re: [PATCH 6/9] hw/block/nand: Rename PAGE_SIZE to NAND_PAGE_SIZE

[Thomas Huth]

On 21/12/2020 01.53, Jiaxun Yang wrote:

As per POSIX specification of limits.h [1], OS libc may define
PAGE_SIZE in limits.h.

To prevent collosion of definition, we rename PAGE_SIZE here.

[1]: https://pubs.opengroup.org/onlinepubs/7908799/xsh/limits.h.html

Signed-off-by: Jiaxun Yang 
---
  hw/block/nand.c | 40 
  1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/hw/block/nand.c b/hw/block/nand.c
index 1d7a48a2ec..17645667d8 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -114,24 +114,24 @@ static void mem_and(uint8_t *dest, const uint8_t *src, 
size_t n)
  # define NAND_IO
  
  # define PAGE(addr)		((addr) >> ADDR_SHIFT)

-# define PAGE_START(page)  (PAGE(page) * (PAGE_SIZE + OOB_SIZE))
+# define PAGE_START(page)  (PAGE(page) * (NAND_PAGE_SIZE + OOB_SIZE))
  # define PAGE_MASK((1 << ADDR_SHIFT) - 1)
  # define OOB_SHIFT(PAGE_SHIFT - 5)
  # define OOB_SIZE (1 << OOB_SHIFT)
  # define SECTOR(addr) ((addr) >> (9 + ADDR_SHIFT - PAGE_SHIFT))
  # define SECTOR_OFFSET(addr)  ((addr) & ((511 >> PAGE_SHIFT) << 8))
  
-# define PAGE_SIZE		256

+# define NAND_PAGE_SIZE 256
  # define PAGE_SHIFT   8
  # define PAGE_SECTORS 1
  # define ADDR_SHIFT   8
  # include "nand.c"
-# define PAGE_SIZE 512
+# define NAND_PAGE_SIZE 512
  # define PAGE_SHIFT   9
  # define PAGE_SECTORS 1
  # define ADDR_SHIFT   8
  # include "nand.c"
-# define PAGE_SIZE 2048
+# define NAND_PAGE_SIZE2048
  # define PAGE_SHIFT   11
  # define PAGE_SECTORS 4
  # define ADDR_SHIFT   16
@@ -661,7 +661,7 @@ type_init(nand_register_types)
  #else
  
  /* Program a single page */

-static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState *s)
+static void glue(nand_blk_write_, NAND_PAGE_SIZE)(NANDFlashState *s)
  {
  uint64_t off, page, sector, soff;
  uint8_t iobuf[(PAGE_SECTORS + 2) * 0x200];
@@ -681,11 +681,11 @@ static void glue(nand_blk_write_, 
PAGE_SIZE)(NANDFlashState *s)
  return;
  }
  
-mem_and(iobuf + (soff | off), s->io, MIN(s->iolen, PAGE_SIZE - off));

-if (off + s->iolen > PAGE_SIZE) {
+mem_and(iobuf + (soff | off), s->io, MIN(s->iolen, NAND_PAGE_SIZE - 
off));
+if (off + s->iolen > NAND_PAGE_SIZE) {
  page = PAGE(s->addr);
-mem_and(s->storage + (page << OOB_SHIFT), s->io + PAGE_SIZE - off,
-MIN(OOB_SIZE, off + s->iolen - PAGE_SIZE));
+mem_and(s->storage + (page << OOB_SHIFT), s->io + NAND_PAGE_SIZE - 
off,
+MIN(OOB_SIZE, off + s->iolen - NAND_PAGE_SIZE));
  }
  
  if (blk_pwrite(s->blk, sector << BDRV_SECTOR_BITS, iobuf,

@@ -713,7 +713,7 @@ static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState 
*s)
  }
  
  /* Erase a single block */

-static void glue(nand_blk_erase_, PAGE_SIZE)(NANDFlashState *s)
+static void glue(nand_blk_erase_, NAND_PAGE_SIZE)(NANDFlashState *s)
  {
  uint64_t i, page, addr;
  uint8_t iobuf[0x200] = { [0 ... 0x1ff] = 0xff, };
@@ -725,7 +725,7 @@ static void glue(nand_blk_erase_, PAGE_SIZE)(NANDFlashState 
*s)
  
  if (!s->blk) {

  memset(s->storage + PAGE_START(addr),
-0xff, (PAGE_SIZE + OOB_SIZE) << s->erase_shift);
+0xff, (NAND_PAGE_SIZE + OOB_SIZE) << s->erase_shift);
  } else if (s->mem_oob) {
  memset(s->storage + (PAGE(addr) << OOB_SHIFT),
  0xff, OOB_SIZE << s->erase_shift);
@@ -751,7 +751,7 @@ static void glue(nand_blk_erase_, PAGE_SIZE)(NANDFlashState 
*s)
  
  memset(iobuf, 0xff, 0x200);

  i = (addr & ~0x1ff) + 0x200;
-for (addr += ((PAGE_SIZE + OOB_SIZE) << s->erase_shift) - 0x200;
+for (addr += ((NAND_PAGE_SIZE + OOB_SIZE) << s->erase_shift) - 0x200;
  i < addr; i += 0x200) {
  if (blk_pwrite(s->blk, i, iobuf, BDRV_SECTOR_SIZE, 0) < 0) {
  printf("%s: write error in sector %" PRIu64 "\n",
@@ -772,7 +772,7 @@ static void glue(nand_blk_erase_, PAGE_SIZE)(NANDFlashState 
*s)
  }
  }
  
-static void glue(nand_blk_load_, PAGE_SIZE)(NANDFlashState *s,

+static void glue(nand_blk_load_, NAND_PAGE_SIZE)(NANDFlashState *s,
  uint64_t addr, int offset)
  {
  if (PAGE(addr) >= s->pages) {
@@ -786,7 +786,7 @@ static void glue(nand_blk_load_, PAGE_SIZE)(NANDFlashState 
*s,
  printf("%s: read error in sector %" PRIu64 "\n",
  __func__, SECTOR(addr));
  }
-memcpy(s->io + SECTOR_OFFSET(s->addr) + PAGE_SIZE,
+memcpy(s->io + SECTOR_OFFSET(s->addr) + NAND_PAGE_SIZE,
  s->storage + (PAGE(s->addr) << OOB_SHIFT),
  OOB_SIZE);
  s->ioaddr = s->io + SECTOR_OFFSET(s

Re: [PATCH 8/9] tests: Rename PAGE_SIZE definitions

[Thomas Huth]

On 21/12/2020 01.53, Jiaxun Yang wrote:

As per POSIX specification of limits.h [1], OS libc may define
PAGE_SIZE in limits.h.

Self defined PAGE_SIZE is frequently used in tests, to prevent
collosion of definition, we give PAGE_SIZE definitons reasonable
prefixs.

[1]: https://pubs.opengroup.org/onlinepubs/7908799/xsh/limits.h.html

Signed-off-by: Jiaxun Yang 
---
  tests/migration/stress.c| 10 ++---
  tests/qtest/libqos/malloc-pc.c  |  4 +-
  tests/qtest/libqos/malloc-spapr.c   |  4 +-
  tests/qtest/m25p80-test.c   | 54 +++---
  tests/tcg/multiarch/system/memory.c |  6 +--
  tests/test-xbzrle.c | 70 ++---
  6 files changed, 74 insertions(+), 74 deletions(-)

diff --git a/tests/migration/stress.c b/tests/migration/stress.c
index de45e8e490..b7240a15c8 100644
--- a/tests/migration/stress.c
+++ b/tests/migration/stress.c
@@ -27,7 +27,7 @@
  
  const char *argv0;
  
-#define PAGE_SIZE 4096

+#define RAM_PAGE_SIZE 4096
  
  #ifndef CONFIG_GETTID

  static int gettid(void)
@@ -158,11 +158,11 @@ static unsigned long long now(void)
  
  static void stressone(unsigned long long ramsizeMB)

  {
-size_t pagesPerMB = 1024 * 1024 / PAGE_SIZE;
+size_t pagesPerMB = 1024 * 1024 / RAM_PAGE_SIZE;
  g_autofree char *ram = g_malloc(ramsizeMB * 1024 * 1024);
  char *ramptr;
  size_t i, j, k;
-g_autofree char *data = g_malloc(PAGE_SIZE);
+g_autofree char *data = g_malloc(RAM_PAGE_SIZE);
  char *dataptr;
  size_t nMB = 0;
  unsigned long long before, after;
@@ -174,7 +174,7 @@ static void stressone(unsigned long long ramsizeMB)
   * calloc instead :-) */
  memset(ram, 0xfe, ramsizeMB * 1024 * 1024);
  
-if (random_bytes(data, PAGE_SIZE) < 0) {

+if (random_bytes(data, RAM_PAGE_SIZE) < 0) {
  return;
  }
  
@@ -186,7 +186,7 @@ static void stressone(unsigned long long ramsizeMB)

  for (i = 0; i < ramsizeMB; i++, nMB++) {
  for (j = 0; j < pagesPerMB; j++) {
  dataptr = data;
-for (k = 0; k < PAGE_SIZE; k += sizeof(long long)) {
+for (k = 0; k < RAM_PAGE_SIZE; k += sizeof(long long)) {
  ramptr += sizeof(long long);
  dataptr += sizeof(long long);
  *(unsigned long long *)ramptr ^= *(unsigned long long 
*)dataptr;
diff --git a/tests/qtest/libqos/malloc-pc.c b/tests/qtest/libqos/malloc-pc.c
index 16ff9609cc..f1e3b392a5 100644
--- a/tests/qtest/libqos/malloc-pc.c
+++ b/tests/qtest/libqos/malloc-pc.c
@@ -18,7 +18,7 @@
  
  #include "qemu-common.h"
  
-#define PAGE_SIZE (4096)

+#define ALLOC_PAGE_SIZE (4096)
  
  void pc_alloc_init(QGuestAllocator *s, QTestState *qts, QAllocOpts flags)

  {
@@ -26,7 +26,7 @@ void pc_alloc_init(QGuestAllocator *s, QTestState *qts, 
QAllocOpts flags)
  QFWCFG *fw_cfg = pc_fw_cfg_init(qts);
  
  ram_size = qfw_cfg_get_u64(fw_cfg, FW_CFG_RAM_SIZE);

-alloc_init(s, flags, 1 << 20, MIN(ram_size, 0xE000), PAGE_SIZE);
+alloc_init(s, flags, 1 << 20, MIN(ram_size, 0xE000), ALLOC_PAGE_SIZE);
  
  /* clean-up */

  pc_fw_cfg_uninit(fw_cfg);
diff --git a/tests/qtest/libqos/malloc-spapr.c 
b/tests/qtest/libqos/malloc-spapr.c
index 84862e4876..05b306c191 100644
--- a/tests/qtest/libqos/malloc-spapr.c
+++ b/tests/qtest/libqos/malloc-spapr.c
@@ -10,7 +10,7 @@
  
  #include "qemu-common.h"
  
-#define PAGE_SIZE 4096

+#define SPAPR_PAGE_SIZE 4096
  
  /* Memory must be a multiple of 256 MB,

   * so we have at least 256MB
@@ -19,5 +19,5 @@
  
  void spapr_alloc_init(QGuestAllocator *s, QTestState *qts, QAllocOpts flags)

  {
-alloc_init(s, flags, 1 << 20, SPAPR_MIN_SIZE, PAGE_SIZE);
+alloc_init(s, flags, 1 << 20, SPAPR_MIN_SIZE, SPAPR_PAGE_SIZE);
  }
diff --git a/tests/qtest/m25p80-test.c b/tests/qtest/m25p80-test.c
index 50c6b79fb3..f860cef5f0 100644
--- a/tests/qtest/m25p80-test.c
+++ b/tests/qtest/m25p80-test.c
@@ -62,7 +62,7 @@ enum {
  #define FLASH_JEDEC 0x20ba19  /* n25q256a */
  #define FLASH_SIZE  (32 * 1024 * 1024)
  
-#define PAGE_SIZE   256

+#define FLASH_PAGE_SIZE   256
  
  /*

   * Use an explicit bswap for the values read/wrote to the flash region
@@ -165,7 +165,7 @@ static void read_page(uint32_t addr, uint32_t *page)
  writel(ASPEED_FLASH_BASE, make_be32(addr));
  
  /* Continuous read are supported */

-for (i = 0; i < PAGE_SIZE / 4; i++) {
+for (i = 0; i < FLASH_PAGE_SIZE / 4; i++) {
  page[i] = make_be32(readl(ASPEED_FLASH_BASE));
  }
  spi_ctrl_stop_user();
@@ -178,15 +178,15 @@ static void read_page_mem(uint32_t addr, uint32_t *page)
  /* move out USER mode to use direct reads from the AHB bus */
  spi_ctrl_setmode(CTRL_READMODE, READ);
  
-for (i = 0; i < PAGE_SIZE / 4; i++) {

+for (i = 0; i < FLASH_PAGE_SIZE / 4; i++) {
  page[i] = make_be32(readl(ASPEED_FLASH_BASE + addr + i * 4));
  }
 

Re: [PATCH 5/9] elf2dmp: Rename PAGE_SIZE to ELF2DMP_PAGE_SIZE

[Thomas Huth]

On 21/12/2020 01.53, Jiaxun Yang wrote:

As per POSIX specification of limits.h [1], OS libc may define
PAGE_SIZE in limits.h.

To prevent collosion of definition, we rename PAGE_SIZE here.

[1]: https://pubs.opengroup.org/onlinepubs/7908799/xsh/limits.h.html

Signed-off-by: Jiaxun Yang 
---
  contrib/elf2dmp/addrspace.c |  4 ++--
  contrib/elf2dmp/addrspace.h |  6 +++---
  contrib/elf2dmp/main.c  | 18 +-
  3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/contrib/elf2dmp/addrspace.c b/contrib/elf2dmp/addrspace.c
index 8a76069cb5..53ded17061 100644
--- a/contrib/elf2dmp/addrspace.c
+++ b/contrib/elf2dmp/addrspace.c
@@ -207,8 +207,8 @@ int va_space_rw(struct va_space *vs, uint64_t addr,
  void *buf, size_t size, int is_write)
  {
  while (size) {
-uint64_t page = addr & PFN_MASK;
-size_t s = (page + PAGE_SIZE) - addr;
+uint64_t page = addr & ELF2DMP_PFN_MASK;
+size_t s = (page + ELF2DMP_PAGE_SIZE) - addr;
  void *ptr;
  
  s = (s > size) ? size : s;

diff --git a/contrib/elf2dmp/addrspace.h b/contrib/elf2dmp/addrspace.h
index d87f6a18c6..00b44c1218 100644
--- a/contrib/elf2dmp/addrspace.h
+++ b/contrib/elf2dmp/addrspace.h
@@ -10,9 +10,9 @@
  
  #include "qemu_elf.h"
  
-#define PAGE_BITS 12

-#define PAGE_SIZE (1ULL << PAGE_BITS)
-#define PFN_MASK (~(PAGE_SIZE - 1))
+#define ELF2DMP_PAGE_BITS 12
+#define ELF2DMP_PAGE_SIZE (1ULL << ELF2DMP_PAGE_BITS)
+#define ELF2DMP_PFN_MASK (~(ELF2DMP_PAGE_SIZE - 1))
  
  #define INVALID_PA  UINT64_MAX
  
diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c

index ac746e49e0..20b477d582 100644
--- a/contrib/elf2dmp/main.c
+++ b/contrib/elf2dmp/main.c
@@ -244,8 +244,8 @@ static int fill_header(WinDumpHeader64 *hdr, struct 
pa_space *ps,
  WinDumpHeader64 h;
  size_t i;
  
-QEMU_BUILD_BUG_ON(KUSD_OFFSET_SUITE_MASK >= PAGE_SIZE);

-QEMU_BUILD_BUG_ON(KUSD_OFFSET_PRODUCT_TYPE >= PAGE_SIZE);
+QEMU_BUILD_BUG_ON(KUSD_OFFSET_SUITE_MASK >= ELF2DMP_PAGE_SIZE);
+QEMU_BUILD_BUG_ON(KUSD_OFFSET_PRODUCT_TYPE >= ELF2DMP_PAGE_SIZE);
  
  if (!suite_mask || !product_type) {

  return 1;
@@ -281,14 +281,14 @@ static int fill_header(WinDumpHeader64 *hdr, struct 
pa_space *ps,
  };
  
  for (i = 0; i < ps->block_nr; i++) {

-h.PhysicalMemoryBlock.NumberOfPages += ps->block[i].size / PAGE_SIZE;
+h.PhysicalMemoryBlock.NumberOfPages += ps->block[i].size / 
ELF2DMP_PAGE_SIZE;
  h.PhysicalMemoryBlock.Run[i] = (WinDumpPhyMemRun64) {
-.BasePage = ps->block[i].paddr / PAGE_SIZE,
-.PageCount = ps->block[i].size / PAGE_SIZE,
+.BasePage = ps->block[i].paddr / ELF2DMP_PAGE_SIZE,
+.PageCount = ps->block[i].size / ELF2DMP_PAGE_SIZE,
  };
  }
  
-h.RequiredDumpSpace += h.PhysicalMemoryBlock.NumberOfPages << PAGE_BITS;

+h.RequiredDumpSpace += h.PhysicalMemoryBlock.NumberOfPages << 
ELF2DMP_PAGE_BITS;
  
  *hdr = h;
  
@@ -379,7 +379,7 @@ static int pe_get_pdb_symstore_hash(uint64_t base, void *start_addr,

  size_t pdb_name_sz;
  size_t i;
  
-QEMU_BUILD_BUG_ON(sizeof(*dos_hdr) >= PAGE_SIZE);

+QEMU_BUILD_BUG_ON(sizeof(*dos_hdr) >= ELF2DMP_PAGE_SIZE);
  
  if (memcmp(&dos_hdr->e_magic, e_magic, sizeof(e_magic))) {

  return 1;
@@ -509,10 +509,10 @@ int main(int argc, char *argv[])
  }
  printf("CPU #0 IDT[0] -> 0x%016"PRIx64"\n", 
idt_desc_addr(first_idt_desc));
  
-KernBase = idt_desc_addr(first_idt_desc) & ~(PAGE_SIZE - 1);

+KernBase = idt_desc_addr(first_idt_desc) & ~(ELF2DMP_PAGE_SIZE - 1);
  printf("Searching kernel downwards from 0x%016"PRIx64"...\n", KernBase);
  
-for (; KernBase >= 0xf780; KernBase -= PAGE_SIZE) {

+for (; KernBase >= 0xf780; KernBase -= ELF2DMP_PAGE_SIZE) {
  nt_start_addr = va_space_resolve(&vs, KernBase);
  if (!nt_start_addr) {
  continue;



Reviewed-by: Thomas Huth 

Re: [PATCH 4/9] libvhost-user: Include poll.h instead of sys/poll.h

[Thomas Huth]

On 21/12/2020 01.53, Jiaxun Yang wrote:

Musl libc complains about it's wrong usage.

In file included from ../subprojects/libvhost-user/libvhost-user.h:20,
  from ../subprojects/libvhost-user/libvhost-user-glib.h:19,
  from ../subprojects/libvhost-user/libvhost-user-glib.c:15:
/usr/include/sys/poll.h:1:2: error: #warning redirecting incorrect #include 
 to  [-Werror=cpp]
 1 | #warning redirecting incorrect #include  to 
   |  ^~~

Signed-off-by: Jiaxun Yang 
---
  subprojects/libvhost-user/libvhost-user.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/subprojects/libvhost-user/libvhost-user.h 
b/subprojects/libvhost-user/libvhost-user.h
index 7d47f1364a..3d13dfadde 100644
--- a/subprojects/libvhost-user/libvhost-user.h
+++ b/subprojects/libvhost-user/libvhost-user.h
@@ -17,7 +17,7 @@
  #include 
  #include 
  #include 
-#include 
+#include 
  #include 
  #include 
  #include "standard-headers/linux/virtio_ring.h"



Reviewed-by: Thomas Huth 

Re: [PATCH 3/9] configure/meson: Only check sys/signal.h on non-Linux

[Thomas Huth]

On 21/12/2020 01.53, Jiaxun Yang wrote:

signal.h is equlevant of sys/signal.h on Linux, musl would complain
wrong usage of sys/signal.h.

In file included from /builds/FlyGoat/qemu/include/qemu/osdep.h:108,
  from ../tests/qemu-iotests/socket_scm_helper.c:13:
/usr/include/sys/signal.h:1:2: error: #warning redirecting incorrect #include 
 to  [-Werror=cpp]
 1 | #warning redirecting incorrect #include  to 
   |  ^~~

Signed-off-by: Jiaxun Yang 
---
  meson.build | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 372576f82c..1ef8722b3a 100644
--- a/meson.build
+++ b/meson.build
@@ -841,7 +841,10 @@ config_host_data.set('HAVE_DRM_H', 
cc.has_header('libdrm/drm.h'))
  config_host_data.set('HAVE_PTY_H', cc.has_header('pty.h'))
  config_host_data.set('HAVE_SYS_IOCCOM_H', cc.has_header('sys/ioccom.h'))
  config_host_data.set('HAVE_SYS_KCOV_H', cc.has_header('sys/kcov.h'))
-config_host_data.set('HAVE_SYS_SIGNAL_H', cc.has_header('sys/signal.h'))
+if targetos != 'linux'
+  # signal.h is equlevant of sys/signal.h on Linux
+  config_host_data.set('HAVE_SYS_SIGNAL_H', cc.has_header('sys/signal.h'))
+endif


Seems like it sys/signal.h was introduced for OpenBSD once (see commit 
128ab2ff50a), so this new check should be fine.


Reviewed-by: Thomas Huth 

Re: [PATCH 2/9] configure: Add sys/timex.h to probe clk_adjtime

[Thomas Huth]

In the subject:

s/clk_adjtime/clock_adjtime/

On 21/12/2020 01.53, Jiaxun Yang wrote:

It is not a part of standard time.h. Glibc put it under
time.h however musl treat it as a sys timex extension.

Signed-off-by: Jiaxun Yang 
---
  configure | 1 +
  1 file changed, 1 insertion(+)

diff --git a/configure b/configure
index c228f7c21e..990f37e123 100755
--- a/configure
+++ b/configure
@@ -4374,6 +4374,7 @@ fi
  clock_adjtime=no
  cat > $TMPC <
+#include 
  
  int main(void)

  {



According to the man page:

 http://www.tin.org/bin/man.cgi?section=2&topic=clock_adjtime

sys/timex.h is indeed the right header here.

Reviewed-by: Thomas Huth 

Re: [PATCH] hw/scsi/megasas: check for NULL frame in megasas_command_cancelled()

[Alexander Bulekov]

Looks like one reported by OSS-Fuzz:
Here's a reproducer

cat << EOF | ./qemu-system-i386 -qtest stdio -display none \
-machine q35,accel=qtest -m 512M  -nodefaults \
-device megasas -device scsi-cd,drive=null0 \
-blockdev driver=null-co,read-zeroes=on,node-name=null0 
outl 0xcf8 0x8801
outl 0xcfc 0x1500
outl 0xcf8 0x8817
outl 0xcfc 0x1e
write 0x40 0x1 0x01
write 0x47 0x1 0x03
write 0x50 0x1 0x12
write 0x55 0x1 0x10
write 0x6a 0x1 0x20
write 0x70 0x1 0x10
write 0x7b 0x1 0x10
write 0x7f 0x1 0x10
write 0x86 0x1 0x10
write 0x8b 0x1 0x10
outb 0x1e40 0x40
write 0x1a 0x1 0x0
write 0x6a000f 0x1 0x0
outb 0x1e40 0x0
outl 0x1e40 0x0
write 0x6f1 0x1 0x00
write 0x6f9 0x1 0x00
write 0x6fd 0x1 0x01
write 0x701 0x1 0x00
write 0x705 0x1 0x06
write 0x730 0x1 0x00
write 0x738 0x1 0x00
write 0x73c 0x1 0x01
write 0x740 0x1 0x00
write 0x744 0x1 0x06
write 0x75c 0x1 0x00
write 0x760 0x1 0x01
write 0x76f 0x1 0x00
write 0x770 0x1 0x20
write 0x77c 0x1 0x20
write 0x780 0x1 0x00
write 0x79b 0x1 0x00
write 0x79f 0x1 0x01
write 0x7ae 0x1 0x00
write 0x7af 0x1 0x20
write 0x7bb 0x1 0x20
write 0x7bf 0x1 0x00
write 0x7cf 0x1 0x10
write 0x7db 0x1 0x00
write 0x7df 0x1 0x20
write 0x7ee 0x1 0x20
write 0x7ef 0x1 0x06
write 0x7fb 0x1 0x10
write 0x7ff 0x1 0x00
outb 0x1e40 0x0
outl 0x1e1f 0x4200
EOF

-Alex

On 201224 1854, Mauro Matteo Cascella wrote:
> Ensure that 'cmd->frame' is not NULL before accessing the 'header' field.
> This check prevents a potential NULL pointer dereference issue.
> 
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1910346
> Signed-off-by: Mauro Matteo Cascella 
> Reported-by: Cheolwoo Myung 
> ---
>  hw/scsi/megasas.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 1a5fc5857d..77510e120c 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -1893,7 +1893,7 @@ static void megasas_command_cancelled(SCSIRequest *req)
>  {
>  MegasasCmd *cmd = req->hba_private;
>  
> -if (!cmd) {
> +if (!cmd || !cmd->frame) {
>  return;
>  }
>  cmd->frame->header.cmd_status = MFI_STAT_SCSI_IO_FAILED;
> -- 
> 2.29.2
> 
> 

Re: [PATCH v6 3/7] qemu: add support for iOS host

[Joelle van Dyne]

On Tue, Jan 12, 2021 at 7:03 AM Peter Maydell  wrote:
>
> On Tue, 5 Jan 2021 at 02:25, Joelle van Dyne  wrote:
> >
> > This introduces support for building for iOS hosts. When the correct Xcode
> > toolchain is used, iOS host will be detected automatically.
> >
> > * block: disable features not supported by iOS sandbox
> > * slirp: disable SMB features for iOS
> > * osdep: disable system() calls for iOS
> >
> > Signed-off-by: Joelle van Dyne 
> > ---
> >  docs/devel/index.rst|  1 +
> >  docs/devel/ios.rst  | 28 +++
> >  configure   | 43 -
> >  meson.build |  2 +-
> >  include/qemu/osdep.h| 11 +++
> >  block.c |  2 +-
> >  block/file-posix.c  | 31 +
> >  net/slirp.c | 16 +++
> >  qga/commands-posix.c|  6 ++
> >  MAINTAINERS |  7 +++
> >  tests/qtest/meson.build |  7 +++
> >  11 files changed, 127 insertions(+), 27 deletions(-)
> >  create mode 100644 docs/devel/ios.rst
> >
> > diff --git a/docs/devel/index.rst b/docs/devel/index.rst
> > index f10ed77e4c..2cc8a13ebe 100644
> > --- a/docs/devel/index.rst
> > +++ b/docs/devel/index.rst
> > @@ -35,3 +35,4 @@ Contents:
> > clocks
> > qom
> > block-coroutine-wrapper
> > +   ios
> > diff --git a/docs/devel/ios.rst b/docs/devel/ios.rst
> > new file mode 100644
> > index 00..b4ab11bec1
> > --- /dev/null
> > +++ b/docs/devel/ios.rst
> > @@ -0,0 +1,28 @@
> > +===
> > +iOS Support
> > +===
> > +
> > +To run qemu on the iOS platform, some modifications were required. Most of 
> > the
>
> QEMU is upper-cased.
>
> > +modifications are conditioned on the ``CONFIG_IOS`` and configuration 
> > variable.
> > +
> > +Build support
> > +-
> > +
> > +For the code to compile, certain changes in the block driver and the slirp
> > +driver had to be made. There is no ``system()`` call, so it has been 
> > replaced
> > +with an assertion error. There should be no code path that call system() 
> > from
>
> "calls"
>
> > +iOS.
> > +
> > +``ucontext`` support is broken on iOS. The implementation from 
> > ``libucontext``
> > +is used instead.
> > +
> > +JIT support
> > +---
> > +
> > +On iOS, allocating RWX pages require special entitlements not usually 
> > granted to
>
> "requires"
>
> > +apps. However, it is possible to use `bulletproof JIT`_ with a development
> > +certificate. This means that we need to allocate one chunk of memory with 
> > RX
> > +permissions and then mirror map the same memory with RW permissions. We 
> > generate
> > +code to the mirror mapping and execute the original mapping.
> > +
> > +.. _bulletproof JIT: 
> > https://www.blackhat.com/docs/us-16/materials/us-16-Krstic.pdf
> > diff --git a/configure b/configure
> > index 744d1990be..c1a08f0171 100755
> > --- a/configure
> > +++ b/configure
> > @@ -560,6 +560,19 @@ EOF
> >compile_object
> >  }
> >
> > +check_ios() {
> > +  cat > $TMPC < > +#ifdef __APPLE__
> > +#import "TargetConditionals.h"
> > +#if !TARGET_OS_IPHONE
> > +#error TARGET_OS_IPHONE not true
> > +#endif
> > +#endif
> > +int main(void) { return 0; }
> > +EOF
> > +  compile_object
> > +}
> > +
> >  check_include() {
> >  cat > $TMPC < >  #include <$1>
> > @@ -602,7 +615,11 @@ elif check_define __DragonFly__ ; then
> >  elif check_define __NetBSD__; then
> >targetos='NetBSD'
> >  elif check_define __APPLE__; then
> > -  targetos='Darwin'
> > +  if check_ios ; then
> > +targetos='iOS'
> > +  else
> > +targetos='Darwin'
> > +  fi
> >  else
> ># This is a fatal error, but don't report it yet, because we
> ># might be going to just print the --help text, or it might
>
> So here targetos=iOS and targetos=Darwin are separate things...
>
> > @@ -6974,6 +7012,9 @@ if test "$cross_compile" = "yes"; then
> >  if test "$linux" = "yes" ; then
> >  echo "system = 'linux'" >> $cross
> >  fi
> > +if test "$darwin" = "yes" ; then
> > +echo "system = 'darwin'" >> $cross
> > +fi
>
> ...so why is this needed if we're not "darwin", but "iOS"...
iOS and macOS being treated the same works in 99% of the cases which
is why this patch is relatively small. For the 1% of time the two
systems behave differently, I added CONFIG_IOS. It's a bit of a hack,
but the alternative is to include  and check for
"TARGET_OS_IPHONE" (which is how it's usually done).

>
> >  case "$ARCH" in
> >  i386|x86_64)
> >  echo "cpu_family = 'x86'" >> $cross
> > diff --git a/meson.build b/meson.build
> > index 9a640d3407..ee333b7a94 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -181,7 +181,7 @@ if targetos == 'windows'
> >include_directories: 
> > include_directories('.'))
> >  elif targetos == 'darwin'
> >coref = dependency('appleframeworks', modules: 'CoreFoundation')
> > -  iokit = dependency('appleframew

Re: [PATCH] hw/ide/ahci: Replace fprintf() by qemu_log_mask(GUEST_ERROR)

[Laurent Vivier]

Le 12/01/2021 à 12:29, Philippe Mathieu-Daudé a écrit :
> Replace fprintf() calls by qemu_log_mask(LOG_GUEST_ERROR).
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/ide/ahci.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 4b675b9cfd8..6d50482b8d1 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -465,8 +465,9 @@ static void ahci_mem_write(void *opaque, hwaddr addr,
>  
>  /* Only aligned reads are allowed on AHCI */
>  if (addr & 3) {
> -fprintf(stderr, "ahci: Mis-aligned write to addr 0x"
> -TARGET_FMT_plx "\n", addr);
> +qemu_log_mask(LOG_GUEST_ERROR,
> +  "ahci: Mis-aligned write to addr 0x%03" HWADDR_PRIX 
> "\n",
> +  addr);
>  return;
>  }
>  
> @@ -,7 +1112,8 @@ static void process_ncq_command(AHCIState *s, int port, 
> uint8_t *cmd_fis,
>  g_assert(is_ncq(ncq_fis->command));
>  if (ncq_tfs->used) {
>  /* error - already in use */
> -fprintf(stderr, "%s: tag %d already used\n", __func__, tag);
> +qemu_log_mask(LOG_GUEST_ERROR, "%s: tag %d already used\n",
> +  __func__, tag);
>  return;
>  }
>  
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

iotest 129

[Max Reitz]

Hi,

tl;dr: I have some troubles debugging what’s wrong with iotest 129.  It 
wants to check that 'stop' does not drain a block job, but to me it 
seems like that’s exactly what’s happening with the mirror job.



For quite some time, I’ve had 129 disabled in my test branch because it 
fails all the time for me.  Now it came up again in Vladimir’s async 
backup series, and so I tried my hands at debugging it once again.


Recap: 129 writes 128M to some image, then runs a block job from there 
(while the source drive is supposedly throttled), then issues a stop 
command, and checks that the job is not drained.  I.e., still running.


(It checks the “running” state via @busy, which is probably wrong; it 
should verify that @state == 'running' (which wasn’t available back in 
2015), but that’s not really why I’m writing this mail.)


Like the last time I tried
(https://lists.nongnu.org/archive/html/qemu-block/2019-06/msg00499.html) 
I can see that block jobs completely ignore BB throttling: If you apply 
the attachment show-progress.patch, you’ll probably see some progress 
made while the test waits for five seconds.  (Here, on tmpfs, mirror and 
commit get to READY, and backup completes.)


OK, so now that block jobs don’t completely break with filters, you can 
instead use a filter node on the source (as I tried in the patch 
referenced above).  And to fully fix the test, we’d also replace the 
@busy == True check by @status == 'running'.  That’s the attachment 
filter-node-show-progress.patch.


If I apply that, I can see that now there actually is some throttling. 
After the time.sleep(5), mirror and commit get to exactly 1 MB, and 
backup gets to 1.0625 MB.  Good.


However, after the stop is issued, backup stays at 1.2 MB (good), but 
mirror and commit progress obviously without throttling to 30, 40, 50 
MB, whatever.  So it appears to me they are drained by the stop.  I.e., 
precisely what the iotest is trying to prove not to happen.



I plan to continue investigating, but I just wanted to send this mail to 
see whether perhaps someone has an idea on what’s going on.


(My main problem is that bisecting this is hard.  AFAIK the throttling 
applied in master:129 has been broken ever since blockdev throttling was 
moved to the BB.  Even without the “Deal with filters” series, you can 
use at least mirror sync=full from filter nodes, so I tried to use 
filter-node-show-progress.patch for just test_drive_mirror(), but that 
only works back until fe4f0614ef9e361d (or rather 0f12264e7a4145 if you 
prefer not to get a SIGABRT).  Before that commit, it hangs.)


Max
commit 9697ee270fc59a7a9e5b6b4a56373e38f2e50327
Author: Max Reitz 
Date:   Tue Jan 12 17:33:08 2021 +0100

iotests/129: Show job progress

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 0e13244d85..30a9612d97 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -64,9 +64,24 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
 self.assert_qmp(result, 'return', {})
 result = self.vm.qmp(cmd, **args)
 self.assert_qmp(result, 'return', {})
+
+print('---')
+print('Right after the job was started:')
+print(self.vm.qmp("query-block-jobs"))
+time.sleep(5)
+print('Five seconds later:')
+print(self.vm.qmp("query-block-jobs"))
+
 result = self.vm.qmp("stop")
 self.assert_qmp(result, 'return', {})
 result = self.vm.qmp("query-block-jobs")
+
+result = self.vm.qmp("stop")
+self.assert_qmp(result, 'return', {})
+result = self.vm.qmp("query-block-jobs")
+
+print('After stop:')
+print(result)
 self.assert_qmp(result, 'return[0]/busy', True)
 self.assert_qmp(result, 'return[0]/ready', False)
 
commit 5b09bd175481925cd60dfa959b3f1f7b7ba4a19c
Author: Max Reitz 
Date:   Mon Jan 28 20:41:16 2019 +0100

iotests/129: Use filter node, and show progress

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 0e13244d85..eae77b9da9 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -32,52 +32,46 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
 iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img,
  "-b", self.base_img, '-F', iotests.imgfmt)
 iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', self.test_img)
-self.vm = iotests.VM().add_drive(self.test_img)
+self.vm = iotests.VM()
+self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
+self.vm.add_drive(None, 'driver=throttle,throttle-group=tg0,file.driver=%s,file.file.filename=%s' % (iotests.imgfmt, self.test_img))
 self.vm.launch()
 
 def tearDown(self):
-params = {"device": "drive0",
-  "bps": 0,
-  "bps_rd": 0,
-  "bps_wr": 0,
-  "iops": 0,
-  "iops_rd": 0,
-  "iops_wr": 0,
- 

Re: [PATCH v6 10/11] iotests: rewrite check into python

[Kevin Wolf]

Am 09.01.2021 um 13:26 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Just use classes introduced in previous three commits. Behavior
> difference is described in these three commits.
> 
> Drop group file, as it becomes unused.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/check | 994 ++-
>  tests/qemu-iotests/group | 321 -
>  2 files changed, 28 insertions(+), 1287 deletions(-)
>  delete mode 100644 tests/qemu-iotests/group

> +import sys
> +import os
> +from findtests import find_tests, TestFinder
> +from testenv import TestEnv
> +from testrunner import TestRunner
> +
> +if __name__ == '__main__':
> +if len(sys.argv) == 2 and sys.argv[1] in ['-h', '--help']:
> +print('Usage: ./check [options] [testlist]')
> +print()
> +TestFinder.get_argparser().print_help()
> +print()
> +TestEnv.get_argparser().print_help()
> +print()
> +TestRunner.get_argparser().print_help()
> +exit()

+check:34:8: R1722: Consider using sys.exit() (consider-using-sys-exit)

> +
> +env = TestEnv(sys.argv[1:])
> +tests, remaining_argv = find_tests(env.remaining_argv,
> +   test_dir=env.source_iotests)
> +
> +with TestRunner(remaining_argv, env) as tr:
> +assert not tr.remaining_argv
> +tr.run_tests([os.path.join(env.source_iotests, t) for t in tests])

The assertion means that giving an unknown option results in an error
message like this:

$ build/check -T -raw
Traceback (most recent call last):
  File "/home/kwolf/source/qemu/tests/qemu-iotests/build/check", line 41, in 

assert not tr.remaining_argv
AssertionError

I think this could be a bit friendlier (especially since this doesn't
even tell you which of your options was wrong).

Kevin

Re: [PATCH v5 02/14] block: use return status of bdrv_append()

[Alberto Garcia]

On Sat 09 Jan 2021 01:57:59 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> Now bdrv_append returns status and we can drop all the local_err things
> around it.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Reviewed-by: Alberto Garcia 

Berto

Re: [PATCH v6 08/11] iotests: add testenv.py

[Kevin Wolf]

Am 09.01.2021 um 13:26 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Add TestEnv class, which will handle test environment in a new python
> iotests running framework.
> 
> Difference with current ./check interface:
> - -v (verbose) option dropped, as it is unused
> 
> - -xdiff option is dropped, until somebody complains that it is needed
> - same for -n option
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

+* Module testenv
+testenv.py:48:0: R0902: Too many instance attributes (34/7) 
(too-many-instance-attributes)
+testenv.py:212:16: R1722: Consider using sys.exit() (consider-using-sys-exit)
+testenv.py:214:16: R1722: Consider using sys.exit() (consider-using-sys-exit)
+testenv.py:324:8: R1722: Consider using sys.exit() (consider-using-sys-exit)
+testenv.py:1:0: R0801: Similar lines in 2 files
+==findtests:45
+==testenv:72
+_argparser = None
+@classmethod
+def get_argparser(cls) -> argparse.ArgumentParser:
+if cls._argparser is not None:
+return cls._argparser
+ (duplicate-code)
+testenv.py:294: error: Function is missing a type annotation for one or more 
arguments

I wonder whether we should just disable the "similar lines" check? The
instance attributes one looks kind of arbitrary, too. The rest looks
valid enough.

Kevin

Re: [PATCH v5 01/14] block: return status from bdrv_append and friends

[Alberto Garcia]

On Sat 09 Jan 2021 01:57:58 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> -void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
> +int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
>   Error **errp)

The indentation of the second line should be adjusted, shouldn't it?

>  {
> +int ret;
>  bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
>  bdrv_inherits_from_recursive(backing_hd, bs);
>  
>  if (bdrv_is_backing_chain_frozen(bs, child_bs(bs->backing), errp)) {
> -return;
> +return -EPERM;
>  }
>  
>  if (backing_hd) {
> @@ -2853,15 +2854,24 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
> BlockDriverState *backing_hd,
>
>  bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_of_bds,
>  bdrv_backing_role(bs), errp);
> +if (!bs->backing) {
> +ret = -EPERM;
> +goto out;
> +}

This is not visible in the patch, but before the bdrv_attach_child()
call there's this:

if (!backing_hd) {
goto out;
}

But in this case 'ret' is still uninitialized.

>  out:
>  bdrv_refresh_limits(bs, NULL);
> +
> +return ret;
>  }



> -static void bdrv_replace_node_common(BlockDriverState *from,
> - BlockDriverState *to,
> - bool auto_skip, Error **errp)
> +static int bdrv_replace_node_common(BlockDriverState *from,
> +BlockDriverState *to,
> +bool auto_skip, Error **errp)
>  {
>  BdrvChild *c, *next;
>  GSList *list = NULL, *p;
> @@ -4562,6 +4572,7 @@ static void bdrv_replace_node_common(BlockDriverState 
> *from,
>  goto out;
>  }
>  if (c->frozen) {
> +ret = -EPERM;
>  error_setg(errp, "Cannot change '%s' link to '%s'",
> c->name, from->node_name);
>  goto out;

Same here, you set 'ret' in the second 'goto out' but not in the first.

Berto

[PATCH v2] iotests: Add test for the regression fixed in c8bf9a9169

[Alberto Garcia]

Signed-off-by: Alberto Garcia 
Suggested-by: Maxim Levitsky 
Reviewed-by: Maxim Levitsky 
---
v2: Rebase on top of the latest master

 tests/qemu-iotests/313 | 103 +
 tests/qemu-iotests/313.out |  29 +++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 133 insertions(+)
 create mode 100755 tests/qemu-iotests/313
 create mode 100644 tests/qemu-iotests/313.out

diff --git a/tests/qemu-iotests/313 b/tests/qemu-iotests/313
new file mode 100755
index 00..0a5202ad49
--- /dev/null
+++ b/tests/qemu-iotests/313
@@ -0,0 +1,103 @@
+#!/usr/bin/env bash
+#
+# Test for the regression fixed in commit c8bf9a9169
+#
+# Copyright (C) 2020 Igalia, S.L.
+# Author: Alberto Garcia 
+# Based on a test case by Maxim Levitsky 
+#
+# 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=be...@igalia.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
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+_unsupported_imgopts cluster_size refcount_bits extended_l2 compat=0.10 
data_file
+
+# The cluster size must be at least the granularity of the mirror job (4KB)
+# Note that larger cluster sizes will produce very large images (several GBs)
+cluster_size=4096
+refcount_bits=64 # Make it equal to the L2 entry size for convenience
+options="cluster_size=${cluster_size},refcount_bits=${refcount_bits}"
+
+# Number of refcount entries per refcount blocks
+ref_entries=$(( ${cluster_size} * 8 / ${refcount_bits} ))
+
+# Number of data clusters needed to fill a refcount block
+# Equals ${ref_entries} minus two (one L2 table and one refcount block)
+data_clusters_per_refblock=$(( ${ref_entries} - 2 ))
+
+# Number of entries in the refcount cache
+ref_blocks=4
+
+# Write enough data clusters to fill the refcount cache and allocate
+# one more refcount block.
+# Subtract 3 clusters from the total: qcow2 header, refcount table, L1 table
+total_data_clusters=$(( ${data_clusters_per_refblock} * ${ref_blocks} + 1 - 3 
))
+
+# Total size to write in bytes
+total_size=$(( ${total_data_clusters} * ${cluster_size} ))
+
+echo
+echo '### Create the image'
+echo
+TEST_IMG_FILE=$TEST_IMG.base _make_test_img -o $options $total_size | 
_filter_img_create_size
+
+echo
+echo '### Write data to allocate more refcount blocks than the cache can hold'
+echo
+$QEMU_IO -c "write -P 1 0 $total_size" $TEST_IMG.base | _filter_qemu_io
+
+echo
+echo '### Create an overlay'
+echo
+_make_test_img -F $IMGFMT -b $TEST_IMG.base -o $options | 
_filter_img_create_size
+
+echo
+echo '### Fill the overlay with zeroes'
+echo
+$QEMU_IO -c "write -z 0 $total_size" $TEST_IMG | _filter_qemu_io
+
+echo
+echo '### Commit changes to the base image'
+echo
+$QEMU_IMG commit $TEST_IMG
+
+echo
+echo '### Check the base image'
+echo
+$QEMU_IMG check $TEST_IMG.base
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/313.out b/tests/qemu-iotests/313.out
new file mode 100644
index 00..adb9f7bd95
--- /dev/null
+++ b/tests/qemu-iotests/313.out
@@ -0,0 +1,29 @@
+QA output created by 313
+
+### Create the image
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=SIZE
+
+### Write data to allocate more refcount blocks than the cache can hold
+
+wrote 8347648/8347648 bytes at offset 0
+7.961 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+### Create an overlay
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=SIZE 
backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
+
+### Fill the overlay with zeroes
+
+wrote 8347648/8347648 bytes at offset 0
+7.961 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+### Commit changes to the base image
+
+Image committed.
+
+### Check the base image
+
+No errors were found on the image.
+Image end offset: 8396800
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index bc5bc324fe..26c9c67c45 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -319,3 +319,4 @@
 308 rw
 309 rw auto quick
 312 rw quick
+313 rw auto quick
-- 
2.20.1

Re: [PATCH v3 25/25] simplebench: add bench-backup.py

[Vladimir Sementsov-Ogievskiy]

12.01.2021 17:50, Max Reitz wrote:

On 26.10.20 18:18, Vladimir Sementsov-Ogievskiy wrote:

Add script to benchmark new backup architecture.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/bench-backup.py | 165 
  1 file changed, 165 insertions(+)
  create mode 100755 scripts/simplebench/bench-backup.py

Loose review, because I’m not too involved, and this is “just” a performance 
test, too.

Result: Looks good to me, two minor things below.


diff --git a/scripts/simplebench/bench-backup.py 
b/scripts/simplebench/bench-backup.py
new file mode 100755
index 00..5c62b1a7ed
--- /dev/null
+++ b/scripts/simplebench/bench-backup.py


[...]


+def bench(args):
+    test_cases = []
+
+    sources = {}
+    targets = {}
+    for d in args.dir:
+    label, path = d.split(':')


Should this be d.split(':', 1) to accommodate path names with colons in them?


Yes, it makes sense.




+    sources[label] = drv_file(path + '/test-source')
+    targets[label] = drv_file(path + '/test-target')
+
+    if args.nbd:
+    nbd = args.nbd.split(':')
+    host = nbd[0]
+    port = '10809' if len(nbd) == 1 else nbd[1]
+    drv = drv_nbd(host, port)
+    sources['nbd'] = drv
+    targets['nbd'] = drv
+
+    for t in args.test:
+    src, dst = t.split(':')
+
+    test_cases.append({
+    'id': t,
+    'source': sources[src],
+    'target': targets[dst]
+    })
+
+    binaries = []  # list of (, , [])
+    for i, q in enumerate(args.env):
+    name_path = q.split(':')


(Same here)


hmm here we can't distinguish path with ':' inside without a tag from tag:path..
but anyway, using ", 1" constantly is good. Possible failures will be more 
clear.




+    if len(name_path) == 1:
+    label = f'q{i}'
+    path_opts = name_path[0].split(',')
+    else:
+    label = name_path[0]
+    path_opts = name_path[1].split(',')
+
+    binaries.append((label, path_opts[0], path_opts[1:]))
+
+    test_envs = []
+
+    bin_paths = {}
+    for i, q in enumerate(args.env):
+    opts = q.split(',')
+    label_path = opts[0]
+    opts = opts[1:]
+
+    if ':' in label_path:
+    label, path = label_path.split(':')


(And here)


+    bin_paths[label] = path
+    elif label_path in bin_paths:
+    label = label_path
+    path = bin_paths[label]
+    else:
+    path = label_path
+    label = f'q{i}'
+    bin_paths[label] = path


[...]


+if __name__ == '__main__':
+    p = argparse.ArgumentParser('Backup benchmark', epilog='''
+ENV format
+
+    (LABEL:PATH|LABEL|PATH)[,max-workers=N][,use-copy-range=(on|off)][,mirror]
+
+    LABEL    short name for the binary
+    PATH path to the binary
+    max-workers  set x-perf.max-workers of backup job
+    use-copy-range   set x-perf.disable-copy-range of backup job


s/disable/use/?



yes


--
Best regards,
Vladimir

Re: [PATCH v6 07/11] iotests: add findtests.py

[Kevin Wolf]

Am 09.01.2021 um 13:26 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Add python script with new logic of searching for tests:
> 
> Current ./check behavior:
>  - tests are named [0-9][0-9][0-9]
>  - tests must be registered in group file (even if test doesn't belong
>to any group, like 142)
> 
> Behavior of findtests.py:
>  - group file is dropped
>  - tests are all files in tests/ subdirectory (except for .out files),
>so it's not needed more to "register the test", just create it with
>appropriate name in tests/ subdirectory. Old names like
>[0-9][0-9][0-9] (in root iotests directory) are supported too, but
>not recommended for new tests
>  - groups are parsed from '# group: ' line inside test files
>  - optional file group.local may be used to define some additional
>groups for downstreams
>  - 'disabled' group is used to temporary disable tests. So instead of
>commenting tests in old 'group' file you now can add them to
>disabled group with help of 'group.local' file
>  - selecting test ranges like 5-15 are not supported more
>(to support restarting failed ./check command from the middle of the
> process, new argument is added: --start-from)
> 
> Benefits:
>  - no rebase conflicts in group file on patch porting from branch to
>branch
>  - no conflicts in upstream, when different series want to occupy same
>test number
>  - meaningful names for test files
>For example, with digital number, when some person wants to add some
>test about block-stream, he most probably will just create a new
>test. But if there would be test-block-stream test already, he will
>at first look at it and may be just add a test-case into it.
>And anyway meaningful names are better.
> 
> This commit don't update check behavior (which will be don in further
> commit), still, the documentation changed like new behavior is already
> here.  Let's live with this small inconsistency for the following few
> commits, until final change.
> 
> The file findtests.py is self-executable and may be used for debugging
> purposes.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  docs/devel/testing.rst  |  50 ++-
>  tests/qemu-iotests/findtests.py | 229 

I extended 297 so that it also checks the newly added Python file, and
pylint and mypy report some errors:

+* Module findtests
+findtests.py:127:19: W0621: Redefining name 'tests' from outer scope (line 
226) (redefined-outer-name)
+findtests.py:224:8: R1722: Consider using sys.exit() (consider-using-sys-exit)
+findtests.py:32: error: Missing type parameters for generic type "Iterator"
+findtests.py:87: error: Function is missing a return type annotation
+findtests.py:206: error: Function is missing a type annotation for one or more 
arguments

I would suggest including a final patch in this series that adds all new
Python files to the checking in 297.

> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> index 0aa7a13bba..bfb6b65edc 100644
> --- a/docs/devel/testing.rst
> +++ b/docs/devel/testing.rst
> @@ -111,7 +111,7 @@ check-block
>  ---
>  
>  ``make check-block`` runs a subset of the block layer iotests (the tests that
> -are in the "auto" group in ``tests/qemu-iotests/group``).
> +are in the "auto" group).
>  See the "QEMU iotests" section below for more information.
>  
>  GCC gcov support
> @@ -224,6 +224,54 @@ another application on the host may have locked the 
> file, possibly leading to a
>  test failure.  If using such devices are explicitly desired, consider adding
>  ``locking=off`` option to disable image locking.
>  
> +Test case groups
> +
> +
> +Test may belong to some groups, you may define it in the comment inside the
> +test.

Maybe: "Tests may belong to one or more test groups, which are defined
in the form of a comment in the test source file."

> By convention, test groups are listed in the second line of the test
> +file, after "#!/..." line, like this:

"after the "#!/..." line"

> +
> +.. code::
> +
> +  #!/usr/bin/env python3
> +  # group: auto quick
> +  #
> +  ...
> +
> +Additional way of defining groups is creating tests/qemu-iotests/group.local

"An additional" or "Another".
"creating the ... file"

> +file. This should be used only for downstream (this file should never appear
> +in upstream). This file may be used for defining some downstream test groups
> +or for temporary disable tests, like this:

"disabling"

> +
> +.. code::
> +
> +  # groups for some company downstream process
> +  #
> +  # ci - tests to run on build
> +  # down - our downstream tests, not for upstream
> +  #
> +  # Format of each line is:
> +  # TEST_NAME TEST_GROUP [TEST_GROUP ]...
> +
> +  013 ci
> +  210 disabled
> +  215 disabled
> +  our-ugly-workaround-test down ci
> +
> +The (not exhaustive) list of groups:

Maybe just something like this?

"Note that the following group names have a special meaning:"

Re: [PATCH v14 0/7] Introduce 'yank' oob qmp command to recover from hanging qemu

[Markus Armbruster]

Queued.  Thanks for persevering!

Re: [PATCH v3 20/25] qapi: backup: disable copy_range by default

[Max Reitz]

On 12.01.21 16:44, Vladimir Sementsov-Ogievskiy wrote:

12.01.2021 17:05, Max Reitz wrote:

On 26.10.20 18:18, Vladimir Sementsov-Ogievskiy wrote:

Further commit will add a benchmark
(scripts/simplebench/bench-backup.py), which will show that backup
works better with async parallel requests (previous commit) and
disabled copy_range. So, let's disable copy_range by default.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  qapi/block-core.json | 2 +-
  blockdev.c   | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)


Uh, well, er, then why did you add it as true in patch 2?

Do you mean this patch as the basis for a discussion whether it should 
be true or false by default? 


Yes, I just kept the change in separate, to make it simpler to discuss.. 
Hmm, or seems I've tried to keep it close to the point when we are going 
to increase performance anyway (and making default false is needed for 
good final performance), when if we do the change earlier it would like 
a degradation during previous 20 commits.


Hm, yes, before this series, copy_range is effectively the default, right.

I don’t have anything to contribute, though, ergo I don’t mind either 
way.


Do you have an idea why copy offloading isn’t better?



As I understand, it may be really fast for filesystem which will avoid 
data copying, like btrfs. Why it works slower for others when we enable 
async + block-status things, I don't know. It should depend on kernel 
implementation.


I have two additional thoughts:

1. for backup we usually want to do a copy on another node, or at least 
another hard drive, so we really going to copy the data, not just create 
some metadata links in fs.


2. even if fs can just create some links to existing clusters instead of 
copying data, IMHO it is not good for backup:


Main difference of backup from snapshot is that we don't touch active 
disk: it works in the same way like without backup job running. But if 
we do create fs-metadata links in backup image to original clusters, it 
means that on next guest write fs will have to do COW operation, so 
backup file will reference old cluster from active disk, and new cluster 
will be allocated for active disk. This means, that backup job influence 
on active vm disk, increasing its fragmentation.. Possibly I'm wrong, 
I'm not really good in how filesystems works.


Both sound like good points to me.  So as far as I’m concerned, 
disabling copy offloading sounds good to me.


So now this separate patch here makes sense to me:

Reviewed-by: Max Reitz 

Perhaps it deserves some explanation in a commit message, though.  I.e., 
that originally, this defaulted to true, as it was before, but now that 
async copying is implemented, we should let it default to false.


Max

Re: [PATCH v3 20/25] qapi: backup: disable copy_range by default

[Vladimir Sementsov-Ogievskiy]

12.01.2021 17:05, Max Reitz wrote:

On 26.10.20 18:18, Vladimir Sementsov-Ogievskiy wrote:

Further commit will add a benchmark
(scripts/simplebench/bench-backup.py), which will show that backup
works better with async parallel requests (previous commit) and
disabled copy_range. So, let's disable copy_range by default.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  qapi/block-core.json | 2 +-
  blockdev.c   | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)


Uh, well, er, then why did you add it as true in patch 2?

Do you mean this patch as the basis for a discussion whether it should be true or false by default? 


Yes, I just kept the change in separate, to make it simpler to discuss.. Hmm, 
or seems I've tried to keep it close to the point when we are going to increase 
performance anyway (and making default false is needed for good final 
performance), when if we do the change earlier it would like a degradation 
during previous 20 commits.


I don’t have anything to contribute, though, ergo I don’t mind either way.

Do you have an idea why copy offloading isn’t better?



As I understand, it may be really fast for filesystem which will avoid data 
copying, like btrfs. Why it works slower for others when we enable async + 
block-status things, I don't know. It should depend on kernel implementation.

I have two additional thoughts:

1. for backup we usually want to do a copy on another node, or at least another 
hard drive, so we really going to copy the data, not just create some metadata 
links in fs.

2. even if fs can just create some links to existing clusters instead of 
copying data, IMHO it is not good for backup:

Main difference of backup from snapshot is that we don't touch active disk: it 
works in the same way like without backup job running. But if we do create 
fs-metadata links in backup image to original clusters, it means that on next 
guest write fs will have to do COW operation, so backup file will reference old 
cluster from active disk, and new cluster will be allocated for active disk. 
This means, that backup job influence on active vm disk, increasing its 
fragmentation.. Possibly I'm wrong, I'm not really good in how filesystems 
works.





diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5a21c24c1d..58eb2bcb86 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1376,7 +1376,7 @@
  # Optional parameters for backup. These parameters don't affect
  # functionality, but may significantly affect performance.
  #
-# @use-copy-range: Use copy offloading. Default true.
+# @use-copy-range: Use copy offloading. Default false.
  #
  # @max-workers: Maximum number of parallel requests for the sustained 
background
  #   copying process. Doesn't influence copy-before-write 
operations.
diff --git a/blockdev.c b/blockdev.c
index 0ed390abe0..1ac64d8ee2 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2788,7 +2788,7 @@ static BlockJob *do_backup_common(BackupCommon *backup,
  {
  BlockJob *job = NULL;
  BdrvDirtyBitmap *bmap = NULL;
-    BackupPerf perf = { .use_copy_range = true, .max_workers = 64 };
+    BackupPerf perf = { .max_workers = 64 };
  int job_flags = JOB_DEFAULT;
  if (!backup->has_speed) {






--
Best regards,
Vladimir

[PATCH v4] block: report errno when flock fcntl fails

[David Edmondson]

When a call to fcntl(2) for the purpose of manipulating file locks
fails with an error other than EAGAIN or EACCES, report the error
returned by fcntl.

EAGAIN or EACCES are elided as they are considered to be common
failures, indicating that a conflicting lock is held by another
process.

Signed-off-by: David Edmondson 
---
v3:
- Remove the now unnecessary updates to the test framework (Max).
- Elide the error detail for EAGAIN or EACCES when locking (Kevin,
  sort-of Max).
- Philippe and Vladimir sent Reviewed-by, but things have changed
  noticeably, so I didn't add them (dme).

v4:
- Really, really remove the unnecessary updates to the test framework.

 block/file-posix.c | 52 +-
 1 file changed, 42 insertions(+), 10 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 00cdaaa2d4..c5142f7ffa 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -836,7 +836,13 @@ static int raw_apply_lock_bytes(BDRVRawState *s, int fd,
 if ((perm_lock_bits & bit) && !(locked_perm & bit)) {
 ret = qemu_lock_fd(fd, off, 1, false);
 if (ret) {
-error_setg(errp, "Failed to lock byte %d", off);
+int err = -ret;
+
+if (err == EAGAIN || err == EACCES) {
+error_setg(errp, "Failed to lock byte %d", off);
+} else {
+error_setg_errno(errp, err, "Failed to lock byte %d", off);
+}
 return ret;
 } else if (s) {
 s->locked_perm |= bit;
@@ -844,7 +850,13 @@ static int raw_apply_lock_bytes(BDRVRawState *s, int fd,
 } else if (unlock && (locked_perm & bit) && !(perm_lock_bits & bit)) {
 ret = qemu_unlock_fd(fd, off, 1);
 if (ret) {
-error_setg(errp, "Failed to unlock byte %d", off);
+int err = -ret;
+
+if (err == EAGAIN || err == EACCES) {
+error_setg(errp, "Failed to lock byte %d", off);
+} else {
+error_setg_errno(errp, err, "Failed to lock byte %d", off);
+}
 return ret;
 } else if (s) {
 s->locked_perm &= ~bit;
@@ -857,7 +869,13 @@ static int raw_apply_lock_bytes(BDRVRawState *s, int fd,
 if ((shared_perm_lock_bits & bit) && !(locked_shared_perm & bit)) {
 ret = qemu_lock_fd(fd, off, 1, false);
 if (ret) {
-error_setg(errp, "Failed to lock byte %d", off);
+int err = -ret;
+
+if (err == EAGAIN || err == EACCES) {
+error_setg(errp, "Failed to lock byte %d", off);
+} else {
+error_setg_errno(errp, err, "Failed to lock byte %d", off);
+}
 return ret;
 } else if (s) {
 s->locked_shared_perm |= bit;
@@ -866,7 +884,7 @@ static int raw_apply_lock_bytes(BDRVRawState *s, int fd,
!(shared_perm_lock_bits & bit)) {
 ret = qemu_unlock_fd(fd, off, 1);
 if (ret) {
-error_setg(errp, "Failed to unlock byte %d", off);
+error_setg_errno(errp, -ret, "Failed to unlock byte %d", off);
 return ret;
 } else if (s) {
 s->locked_shared_perm &= ~bit;
@@ -890,9 +908,16 @@ static int raw_check_lock_bytes(int fd, uint64_t perm, 
uint64_t shared_perm,
 ret = qemu_lock_fd_test(fd, off, 1, true);
 if (ret) {
 char *perm_name = bdrv_perm_names(p);
-error_setg(errp,
-   "Failed to get \"%s\" lock",
-   perm_name);
+int err = -ret;
+
+if (err == EAGAIN || err == EACCES) {
+error_setg(errp, "Failed to get \"%s\" lock",
+   perm_name);
+} else {
+error_setg_errno(errp, err,
+ "Failed to get \"%s\" lock",
+ perm_name);
+}
 g_free(perm_name);
 return ret;
 }
@@ -905,9 +930,16 @@ static int raw_check_lock_bytes(int fd, uint64_t perm, 
uint64_t shared_perm,
 ret = qemu_lock_fd_test(fd, off, 1, true);
 if (ret) {
 char *perm_name = bdrv_perm_names(p);
-error_setg(errp,
-   "Failed to get shared \"%s\" lock",
-   perm_name);
+int err = -ret;
+
+if (err == EAGAIN || err == EACCES) {
+error_setg(errp, "Failed to get shared \"%s\" lock",
+   perm_name);
+} else {
+error_setg_errno(errp, err,
+ "Failed to get shared \"%s\"

Re: [PATCH v3 19/25] backup: move to block-copy

[Vladimir Sementsov-Ogievskiy]

12.01.2021 16:23, Max Reitz wrote:

On 26.10.20 18:18, Vladimir Sementsov-Ogievskiy wrote:

This brings async request handling and block-status driven chunk sizes
to backup out of the box, which improves backup performance.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/backup.c | 187 -
  1 file changed, 121 insertions(+), 66 deletions(-)


Reviewed-by: Max Reitz 


Thanks a lot!

My way to this point was started in far 2016 summer.. And final concept is 
far-far away from my first attempt:)



Irrelevant notes below.


diff --git a/block/backup.c b/block/backup.c
index 449b763ce4..0d1cd64eab 100644
--- a/block/backup.c
+++ b/block/backup.c


(Just something I noted when looking for remaining instances of “ratelimit”: 
backup.c includes ratelimit.h, which I don’t think it needs to do any longer)


agree



[...]


  static int coroutine_fn backup_loop(BackupBlockJob *job)
  {


[...]


+    if (!block_copy_call_finished(s) &&
+    job_is_cancelled(&job->common.job))
+    {


Just for the sake of clarity: If !block_copy_call_finished(), then 
job_is_cancelled() must be true, right?

(I.e., could be an assertion instead of the second part of the condition.  I 
don’t mind how it is, but then again, it did made me ask.)


Hmm, keeping in mind previous loop "while (!block_copy_call_finished(s) && 
!job_is_cancelled(&job->common.job)) {}", yes, you are right.




+    /*
+ * Note that we can't use job_yield() here, as it doesn't work for
+ * cancelled job.
+ */
+    block_copy_call_cancel(s);
+    job->wait = true;
+    qemu_coroutine_yield();
+    assert(block_copy_call_finished(s));
+    ret = 0;
+    goto out;
+    }
+
+    if (job_is_cancelled(&job->common.job) ||
+    block_copy_call_succeeded(s))
+    {
+    ret = 0;
+    goto out;
+    }
+
+    if (block_copy_call_cancelled(s)) {
+    /*
+ * Job is not cancelled but only block-copy call. This is possible
+ * after job pause. Now the pause is finished, start new block-copy
+ * iteration.
+ */
+    block_copy_call_free(s);
+    continue;


If one were to avoid all continues, we could alternatively put the whole error 
handling into a block_copy_call_failed() condition, and have the 
block_copy_call_free() common for both the cancel and the fail case.

*shrug*


Hmm. Same emotion, *shrug*:). I like my comment, describing exactly 
block_copy_call_cancelled() in corresponding if.. Being combined with failure 
case it would be less demonstrative. But I'm OK with your suggestion too.




+    }
+
+    /* The only remaining case is failed block-copy call. */
+    assert(block_copy_call_failed(s));
+
+    ret = block_copy_call_status(s, &error_is_read);
+    act = backup_error_action(job, error_is_read, -ret);
+    switch (act) {
+    case BLOCK_ERROR_ACTION_REPORT:
+    goto out;
+    case BLOCK_ERROR_ACTION_STOP:
+    /*
+ * Go to pause prior to starting new block-copy call on the next
+ * iteration.
+ */
+    job_pause_point(&job->common.job);
+    break;
+    case BLOCK_ERROR_ACTION_IGNORE:
+    /* Proceed to new block-copy call to retry. */
+    break;
+    default:
+    abort();
+    }
+
+    block_copy_call_free(s);
  }
- out:
-    bdrv_dirty_iter_free(bdbi);
+out:
+    block_copy_call_free(s);


Reads a bit weird after the block_copy_call_free(s) at the end of the while 
(true) loop, but is entirely correct, of course.  (And I can’t offer any better 
alternative.)



Yes, looks weird.. Probably we'll invent some good refactoring later. If the 
function doesn't fit into one screen, there must exist some good refactoring)

--
Best regards,
Vladimir

Re: [PATCH v6 3/7] qemu: add support for iOS host

[Peter Maydell]

On Tue, 5 Jan 2021 at 02:25, Joelle van Dyne  wrote:
>
> This introduces support for building for iOS hosts. When the correct Xcode
> toolchain is used, iOS host will be detected automatically.
>
> * block: disable features not supported by iOS sandbox
> * slirp: disable SMB features for iOS
> * osdep: disable system() calls for iOS
>
> Signed-off-by: Joelle van Dyne 
> ---
>  docs/devel/index.rst|  1 +
>  docs/devel/ios.rst  | 28 +++
>  configure   | 43 -
>  meson.build |  2 +-
>  include/qemu/osdep.h| 11 +++
>  block.c |  2 +-
>  block/file-posix.c  | 31 +
>  net/slirp.c | 16 +++
>  qga/commands-posix.c|  6 ++
>  MAINTAINERS |  7 +++
>  tests/qtest/meson.build |  7 +++
>  11 files changed, 127 insertions(+), 27 deletions(-)
>  create mode 100644 docs/devel/ios.rst
>
> diff --git a/docs/devel/index.rst b/docs/devel/index.rst
> index f10ed77e4c..2cc8a13ebe 100644
> --- a/docs/devel/index.rst
> +++ b/docs/devel/index.rst
> @@ -35,3 +35,4 @@ Contents:
> clocks
> qom
> block-coroutine-wrapper
> +   ios
> diff --git a/docs/devel/ios.rst b/docs/devel/ios.rst
> new file mode 100644
> index 00..b4ab11bec1
> --- /dev/null
> +++ b/docs/devel/ios.rst
> @@ -0,0 +1,28 @@
> +===
> +iOS Support
> +===
> +
> +To run qemu on the iOS platform, some modifications were required. Most of 
> the

QEMU is upper-cased.

> +modifications are conditioned on the ``CONFIG_IOS`` and configuration 
> variable.
> +
> +Build support
> +-
> +
> +For the code to compile, certain changes in the block driver and the slirp
> +driver had to be made. There is no ``system()`` call, so it has been replaced
> +with an assertion error. There should be no code path that call system() from

"calls"

> +iOS.
> +
> +``ucontext`` support is broken on iOS. The implementation from 
> ``libucontext``
> +is used instead.
> +
> +JIT support
> +---
> +
> +On iOS, allocating RWX pages require special entitlements not usually 
> granted to

"requires"

> +apps. However, it is possible to use `bulletproof JIT`_ with a development
> +certificate. This means that we need to allocate one chunk of memory with RX
> +permissions and then mirror map the same memory with RW permissions. We 
> generate
> +code to the mirror mapping and execute the original mapping.
> +
> +.. _bulletproof JIT: 
> https://www.blackhat.com/docs/us-16/materials/us-16-Krstic.pdf
> diff --git a/configure b/configure
> index 744d1990be..c1a08f0171 100755
> --- a/configure
> +++ b/configure
> @@ -560,6 +560,19 @@ EOF
>compile_object
>  }
>
> +check_ios() {
> +  cat > $TMPC < +#ifdef __APPLE__
> +#import "TargetConditionals.h"
> +#if !TARGET_OS_IPHONE
> +#error TARGET_OS_IPHONE not true
> +#endif
> +#endif
> +int main(void) { return 0; }
> +EOF
> +  compile_object
> +}
> +
>  check_include() {
>  cat > $TMPC <  #include <$1>
> @@ -602,7 +615,11 @@ elif check_define __DragonFly__ ; then
>  elif check_define __NetBSD__; then
>targetos='NetBSD'
>  elif check_define __APPLE__; then
> -  targetos='Darwin'
> +  if check_ios ; then
> +targetos='iOS'
> +  else
> +targetos='Darwin'
> +  fi
>  else
># This is a fatal error, but don't report it yet, because we
># might be going to just print the --help text, or it might

So here targetos=iOS and targetos=Darwin are separate things...

> @@ -6974,6 +7012,9 @@ if test "$cross_compile" = "yes"; then
>  if test "$linux" = "yes" ; then
>  echo "system = 'linux'" >> $cross
>  fi
> +if test "$darwin" = "yes" ; then
> +echo "system = 'darwin'" >> $cross
> +fi

...so why is this needed if we're not "darwin", but "iOS"...

>  case "$ARCH" in
>  i386|x86_64)
>  echo "cpu_family = 'x86'" >> $cross
> diff --git a/meson.build b/meson.build
> index 9a640d3407..ee333b7a94 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -181,7 +181,7 @@ if targetos == 'windows'
>include_directories: 
> include_directories('.'))
>  elif targetos == 'darwin'
>coref = dependency('appleframeworks', modules: 'CoreFoundation')
> -  iokit = dependency('appleframeworks', modules: 'IOKit')
> +  iokit = dependency('appleframeworks', modules: 'IOKit', required: 
> 'CONFIG_IOS' not in config_host)
>cocoa = dependency('appleframeworks', modules: 'Cocoa', required: 
> get_option('cocoa'))
>  elif targetos == 'sunos'
>socket = [cc.find_library('socket'),

...and here ios seems to be a subtype of darwin, not a different
kind of targetos. That's a bit confusing. Maybe this is Meson's fault ?

> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index f9ec8c84e9..eb8d06cbf5 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -686,4 +686,15 @@ char *qemu_get_host_name(Error **errp);
>   */

Re: [PATCH v3 25/25] simplebench: add bench-backup.py

[Max Reitz]

On 26.10.20 18:18, Vladimir Sementsov-Ogievskiy wrote:

Add script to benchmark new backup architecture.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/bench-backup.py | 165 
  1 file changed, 165 insertions(+)
  create mode 100755 scripts/simplebench/bench-backup.py
Loose review, because I’m not too involved, and this is “just” a 
performance test, too.


Result: Looks good to me, two minor things below.


diff --git a/scripts/simplebench/bench-backup.py 
b/scripts/simplebench/bench-backup.py
new file mode 100755
index 00..5c62b1a7ed
--- /dev/null
+++ b/scripts/simplebench/bench-backup.py


[...]


+def bench(args):
+test_cases = []
+
+sources = {}
+targets = {}
+for d in args.dir:
+label, path = d.split(':')


Should this be d.split(':', 1) to accommodate path names with colons in 
them?



+sources[label] = drv_file(path + '/test-source')
+targets[label] = drv_file(path + '/test-target')
+
+if args.nbd:
+nbd = args.nbd.split(':')
+host = nbd[0]
+port = '10809' if len(nbd) == 1 else nbd[1]
+drv = drv_nbd(host, port)
+sources['nbd'] = drv
+targets['nbd'] = drv
+
+for t in args.test:
+src, dst = t.split(':')
+
+test_cases.append({
+'id': t,
+'source': sources[src],
+'target': targets[dst]
+})
+
+binaries = []  # list of (, , [])
+for i, q in enumerate(args.env):
+name_path = q.split(':')


(Same here)


+if len(name_path) == 1:
+label = f'q{i}'
+path_opts = name_path[0].split(',')
+else:
+label = name_path[0]
+path_opts = name_path[1].split(',')
+
+binaries.append((label, path_opts[0], path_opts[1:]))
+
+test_envs = []
+
+bin_paths = {}
+for i, q in enumerate(args.env):
+opts = q.split(',')
+label_path = opts[0]
+opts = opts[1:]
+
+if ':' in label_path:
+label, path = label_path.split(':')


(And here)


+bin_paths[label] = path
+elif label_path in bin_paths:
+label = label_path
+path = bin_paths[label]
+else:
+path = label_path
+label = f'q{i}'
+bin_paths[label] = path


[...]


+if __name__ == '__main__':
+p = argparse.ArgumentParser('Backup benchmark', epilog='''
+ENV format
+
+(LABEL:PATH|LABEL|PATH)[,max-workers=N][,use-copy-range=(on|off)][,mirror]
+
+LABELshort name for the binary
+PATH path to the binary
+max-workers  set x-perf.max-workers of backup job
+use-copy-range   set x-perf.disable-copy-range of backup job


s/disable/use/?

Max

Re: [PATCH v14 3/7] chardev/char-socket.c: Add yank feature

[Marc-André Lureau]

On Mon, Dec 28, 2020 at 7:08 PM Lukas Straub  wrote:

> Register a yank function to shutdown the socket on yank.
>
> Signed-off-by: Lukas Straub 
> Acked-by: Stefan Hajnoczi 
>

Acked-by: Marc-André Lureau 

---
>  chardev/char-socket.c | 34 ++
>  1 file changed, 34 insertions(+)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 213a4c8dd0..8a707d766c 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -34,6 +34,7 @@
>  #include "qapi/error.h"
>  #include "qapi/clone-visitor.h"
>  #include "qapi/qapi-visit-sockets.h"
> +#include "qemu/yank.h"
>
>  #include "chardev/char-io.h"
>  #include "qom/object.h"
> @@ -70,6 +71,7 @@ struct SocketChardev {
>  size_t read_msgfds_num;
>  int *write_msgfds;
>  size_t write_msgfds_num;
> +bool registered_yank;
>
>  SocketAddress *addr;
>  bool is_listen;
> @@ -415,6 +417,12 @@ static void tcp_chr_free_connection(Chardev *chr)
>
>  tcp_set_msgfds(chr, NULL, 0);
>  remove_fd_in_watch(chr);
> +if (s->state == TCP_CHARDEV_STATE_CONNECTING
> +|| s->state == TCP_CHARDEV_STATE_CONNECTED) {
> +yank_unregister_function(CHARDEV_YANK_INSTANCE(chr->label),
> + yank_generic_iochannel,
> + QIO_CHANNEL(s->sioc));
> +}
>  object_unref(OBJECT(s->sioc));
>  s->sioc = NULL;
>  object_unref(OBJECT(s->ioc));
> @@ -932,6 +940,9 @@ static int tcp_chr_add_client(Chardev *chr, int fd)
>  }
>  tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
>  tcp_chr_set_client_ioc_name(chr, sioc);
> +yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
> +   yank_generic_iochannel,
> +   QIO_CHANNEL(sioc));
>  ret = tcp_chr_new_client(chr, sioc);
>  object_unref(OBJECT(sioc));
>  return ret;
> @@ -946,6 +957,9 @@ static void tcp_chr_accept(QIONetListener *listener,
>
>  tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
>  tcp_chr_set_client_ioc_name(chr, cioc);
> +yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
> +   yank_generic_iochannel,
> +   QIO_CHANNEL(cioc));
>  tcp_chr_new_client(chr, cioc);
>  }
>
> @@ -961,6 +975,9 @@ static int tcp_chr_connect_client_sync(Chardev *chr,
> Error **errp)
>  object_unref(OBJECT(sioc));
>  return -1;
>  }
> +yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
> +   yank_generic_iochannel,
> +   QIO_CHANNEL(sioc));
>  tcp_chr_new_client(chr, sioc);
>  object_unref(OBJECT(sioc));
>  return 0;
> @@ -976,6 +993,9 @@ static void tcp_chr_accept_server_sync(Chardev *chr)
>  tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
>  sioc = qio_net_listener_wait_client(s->listener);
>  tcp_chr_set_client_ioc_name(chr, sioc);
> +yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
> +   yank_generic_iochannel,
> +   QIO_CHANNEL(sioc));
>  tcp_chr_new_client(chr, sioc);
>  object_unref(OBJECT(sioc));
>  }
> @@ -1086,6 +1106,9 @@ static void char_socket_finalize(Object *obj)
>  object_unref(OBJECT(s->tls_creds));
>  }
>  g_free(s->tls_authz);
> +if (s->registered_yank) {
> +yank_unregister_instance(CHARDEV_YANK_INSTANCE(chr->label));
> +}
>
>  qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>  }
> @@ -1101,6 +1124,9 @@ static void qemu_chr_socket_connected(QIOTask *task,
> void *opaque)
>
>  if (qio_task_propagate_error(task, &err)) {
>  tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
> +yank_unregister_function(CHARDEV_YANK_INSTANCE(chr->label),
> + yank_generic_iochannel,
> + QIO_CHANNEL(sioc));
>  check_report_connect_error(chr, err);
>  goto cleanup;
>  }
> @@ -1134,6 +1160,9 @@ static void tcp_chr_connect_client_async(Chardev
> *chr)
>  tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
>  sioc = qio_channel_socket_new();
>  tcp_chr_set_client_ioc_name(chr, sioc);
> +yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
> +   yank_generic_iochannel,
> +   QIO_CHANNEL(sioc));
>  /*
>   * Normally code would use the qio_channel_socket_connect_async
>   * method which uses a QIOTask + qio_task_set_error internally
> @@ -1376,6 +1405,11 @@ static void qmp_chardev_open_socket(Chardev *chr,
>  qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
>  }
>
> +if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), errp))
> {
> +return;
> +}
> +s->registered_yank = true;
> +
>  /* be isn't opened until we get a connection */
>  *be_opened = false;
>
> --
> 2.29.2
>
>

-- 
Marc-André Lureau

Re: [PATCH v14 1/7] Introduce yank feature

[Marc-André Lureau]

On Tue, Jan 12, 2021 at 6:18 PM Markus Armbruster  wrote:

> Marc-André Lureau  writes:
>
> > On Mon, Jan 11, 2021 at 5:34 PM Markus Armbruster 
> wrote:
> >
> >> Marc-André Lureau  writes:
> >>
> >> > Hi
> >> >
> >> > On Mon, Dec 28, 2020 at 7:08 PM Lukas Straub 
> >> wrote:
> >> >
> >> >> The yank feature allows to recover from hanging qemu by "yanking"
> >> >> at various parts. Other qemu systems can register themselves and
> >> >> multiple yank functions. Then all yank functions for selected
> >> >> instances can be called by the 'yank' out-of-band qmp command.
> >> >> Available instances can be queried by a 'query-yank' oob command.
> >> >>
> >> >
> >> > Looking at the changes and API usage, I wonder if it wouldn't have
> been
> >> > simpler to associate the yank function directly with the YankInstance
> >> > (removing the need for register/unregister functions - tracking the
> state
> >> > left to the callback). Have you tried that approach? If not, I could
> >> check
> >> > if this idea would work.
> >>
> >> Considering we're at v14...  would it make sense to commit the current
> >> approach, then explore the alternative approach on top?
> >>
> >>
> > works for me
> >
> >> If yes, is v14 committable as is?
> >>
> >>
> > Acked-by: Marc-André Lureau 
>
> Thanks!  Does your ACK apply to the series, or just to PATCH 1?
>

I didn't look much at the rest of the series, but PATCH 3 too, I'll reply
there.


-- 
Marc-André Lureau

Re: [PATCH] tests/iotests: drop test 312 from auto group

[Alex Bennée]

Kevin Wolf  writes:

> Am 05.01.2021 um 11:04 hat Alex Bennée geschrieben:
>> The "auto" documentation states:
>> 
>>   That means they should run with every QEMU binary (also non-x86)
>> 
>> which is not the case as the check-system-fedora build which only
>> includes a rag tag group of rare and deprecated targets doesn't
>> support the virtio device required.
>> 
>> Signed-off-by: Alex Bennée 
>
> I think the better solution would be to do something like in 192 so that
> the test is still run at least for one binary:
>
> if [ "$QEMU_DEFAULT_MACHINE" != "pc" ]; then
> _notrun "Requires a PC machine"
> fi

The fix is already in so feel free to revert and fix up properly. I
wasn't quite able to follow the logic of how the qemu-system binary is
chosen it seemed a little too much to chance.

>
> Kevin


-- 
Alex Bennée

Re: [PATCH v14 1/7] Introduce yank feature

[Markus Armbruster]

Marc-André Lureau  writes:

> On Mon, Jan 11, 2021 at 5:34 PM Markus Armbruster  wrote:
>
>> Marc-André Lureau  writes:
>>
>> > Hi
>> >
>> > On Mon, Dec 28, 2020 at 7:08 PM Lukas Straub 
>> wrote:
>> >
>> >> The yank feature allows to recover from hanging qemu by "yanking"
>> >> at various parts. Other qemu systems can register themselves and
>> >> multiple yank functions. Then all yank functions for selected
>> >> instances can be called by the 'yank' out-of-band qmp command.
>> >> Available instances can be queried by a 'query-yank' oob command.
>> >>
>> >
>> > Looking at the changes and API usage, I wonder if it wouldn't have been
>> > simpler to associate the yank function directly with the YankInstance
>> > (removing the need for register/unregister functions - tracking the state
>> > left to the callback). Have you tried that approach? If not, I could
>> check
>> > if this idea would work.
>>
>> Considering we're at v14...  would it make sense to commit the current
>> approach, then explore the alternative approach on top?
>>
>>
> works for me
>
>> If yes, is v14 committable as is?
>>
>>
> Acked-by: Marc-André Lureau 

Thanks!  Does your ACK apply to the series, or just to PATCH 1?

Re: [PATCH v3 23/25] simplebench/bench_block_job: use correct shebang line with python3

[Max Reitz]

On 26.10.20 18:18, Vladimir Sementsov-Ogievskiy wrote:

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  scripts/simplebench/bench_block_job.py | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Max Reitz 

Re: [PATCH v3 21/25] block/block-copy: drop unused block_copy_set_progress_callback()

[Max Reitz]

On 26.10.20 18:18, Vladimir Sementsov-Ogievskiy wrote:

Drop unused code.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/block/block-copy.h |  6 --
  block/block-copy.c | 15 ---
  2 files changed, 21 deletions(-)


Reviewed-by: Max Reitz 

Re: [PATCH v3 20/25] qapi: backup: disable copy_range by default

[Max Reitz]

On 26.10.20 18:18, Vladimir Sementsov-Ogievskiy wrote:

Further commit will add a benchmark
(scripts/simplebench/bench-backup.py), which will show that backup
works better with async parallel requests (previous commit) and
disabled copy_range. So, let's disable copy_range by default.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  qapi/block-core.json | 2 +-
  blockdev.c   | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)


Uh, well, er, then why did you add it as true in patch 2?

Do you mean this patch as the basis for a discussion whether it should 
be true or false by default?  I don’t have anything to contribute, 
though, ergo I don’t mind either way.


Do you have an idea why copy offloading isn’t better?

Max


diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5a21c24c1d..58eb2bcb86 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1376,7 +1376,7 @@
  # Optional parameters for backup. These parameters don't affect
  # functionality, but may significantly affect performance.
  #
-# @use-copy-range: Use copy offloading. Default true.
+# @use-copy-range: Use copy offloading. Default false.
  #
  # @max-workers: Maximum number of parallel requests for the sustained 
background
  #   copying process. Doesn't influence copy-before-write 
operations.
diff --git a/blockdev.c b/blockdev.c
index 0ed390abe0..1ac64d8ee2 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2788,7 +2788,7 @@ static BlockJob *do_backup_common(BackupCommon *backup,
  {
  BlockJob *job = NULL;
  BdrvDirtyBitmap *bmap = NULL;
-BackupPerf perf = { .use_copy_range = true, .max_workers = 64 };
+BackupPerf perf = { .max_workers = 64 };
  int job_flags = JOB_DEFAULT;
  
  if (!backup->has_speed) {

Re: [PATCH v3 19/25] backup: move to block-copy

[Max Reitz]

On 26.10.20 18:18, Vladimir Sementsov-Ogievskiy wrote:

This brings async request handling and block-status driven chunk sizes
to backup out of the box, which improves backup performance.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/backup.c | 187 -
  1 file changed, 121 insertions(+), 66 deletions(-)


Reviewed-by: Max Reitz 

Irrelevant notes below.


diff --git a/block/backup.c b/block/backup.c
index 449b763ce4..0d1cd64eab 100644
--- a/block/backup.c
+++ b/block/backup.c


(Just something I noted when looking for remaining instances of 
“ratelimit”: backup.c includes ratelimit.h, which I don’t think it needs 
to do any longer)


[...]


  static int coroutine_fn backup_loop(BackupBlockJob *job)
  {


[...]


+if (!block_copy_call_finished(s) &&
+job_is_cancelled(&job->common.job))
+{


Just for the sake of clarity: If !block_copy_call_finished(), then 
job_is_cancelled() must be true, right?


(I.e., could be an assertion instead of the second part of the 
condition.  I don’t mind how it is, but then again, it did made me ask.)



+/*
+ * Note that we can't use job_yield() here, as it doesn't work for
+ * cancelled job.
+ */
+block_copy_call_cancel(s);
+job->wait = true;
+qemu_coroutine_yield();
+assert(block_copy_call_finished(s));
+ret = 0;
+goto out;
+}
+
+if (job_is_cancelled(&job->common.job) ||
+block_copy_call_succeeded(s))
+{
+ret = 0;
+goto out;
+}
+
+if (block_copy_call_cancelled(s)) {
+/*
+ * Job is not cancelled but only block-copy call. This is possible
+ * after job pause. Now the pause is finished, start new block-copy
+ * iteration.
+ */
+block_copy_call_free(s);
+continue;


If one were to avoid all continues, we could alternatively put the whole 
error handling into a block_copy_call_failed() condition, and have the 
block_copy_call_free() common for both the cancel and the fail case.


*shrug*


+}
+
+/* The only remaining case is failed block-copy call. */
+assert(block_copy_call_failed(s));
+
+ret = block_copy_call_status(s, &error_is_read);
+act = backup_error_action(job, error_is_read, -ret);
+switch (act) {
+case BLOCK_ERROR_ACTION_REPORT:
+goto out;
+case BLOCK_ERROR_ACTION_STOP:
+/*
+ * Go to pause prior to starting new block-copy call on the next
+ * iteration.
+ */
+job_pause_point(&job->common.job);
+break;
+case BLOCK_ERROR_ACTION_IGNORE:
+/* Proceed to new block-copy call to retry. */
+break;
+default:
+abort();
+}
+
+block_copy_call_free(s);
  }
  
- out:

-bdrv_dirty_iter_free(bdbi);
+out:
+block_copy_call_free(s);


Reads a bit weird after the block_copy_call_free(s) at the end of the 
while (true) loop, but is entirely correct, of course.  (And I can’t 
offer any better alternative.)


Max


+job->bg_bcs_call = NULL;
  return ret;
  }

Re: [PATCH] hw/block/nvme: fix for non-msix machines

[Philippe Mathieu-Daudé]

On 1/12/21 1:47 PM, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> Commit 1c0c2163aa08 ("hw/block/nvme: verify msix_init_exclusive_bar()
> return value") had the unintended effect of breaking support on
> several platforms not supporting MSI-X.
> 
> Still check for errors, but only report that MSI-X is unsupported
> instead of bailing out.
> 
> Reported-by: Guenter Roeck 
> Fixes: 1c0c2163aa08 ("hw/block/nvme: verify msix_init_exclusive_bar() return 
> value")
> Fixes: fbf2e5375e33 ("hw/block/nvme: Verify msix_vector_use() returned value")
> Signed-off-by: Klaus Jensen 
> ---
>  hw/block/nvme.c | 31 ++-
>  1 file changed, 22 insertions(+), 9 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

[PATCH] hw/block/nvme: fix for non-msix machines

[Klaus Jensen]

From: Klaus Jensen 

Commit 1c0c2163aa08 ("hw/block/nvme: verify msix_init_exclusive_bar()
return value") had the unintended effect of breaking support on
several platforms not supporting MSI-X.

Still check for errors, but only report that MSI-X is unsupported
instead of bailing out.

Reported-by: Guenter Roeck 
Fixes: 1c0c2163aa08 ("hw/block/nvme: verify msix_init_exclusive_bar() return 
value")
Fixes: fbf2e5375e33 ("hw/block/nvme: Verify msix_vector_use() returned value")
Signed-off-by: Klaus Jensen 
---
 hw/block/nvme.c | 31 ++-
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 0854ee307227..cf0fe28fe6eb 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2590,7 +2590,9 @@ static void nvme_free_cq(NvmeCQueue *cq, NvmeCtrl *n)
 n->cq[cq->cqid] = NULL;
 timer_del(cq->timer);
 timer_free(cq->timer);
-msix_vector_unuse(&n->parent_obj, cq->vector);
+if (msix_enabled(&n->parent_obj)) {
+msix_vector_unuse(&n->parent_obj, cq->vector);
+}
 if (cq->cqid) {
 g_free(cq);
 }
@@ -2624,8 +2626,10 @@ static void nvme_init_cq(NvmeCQueue *cq, NvmeCtrl *n, 
uint64_t dma_addr,
 {
 int ret;
 
-ret = msix_vector_use(&n->parent_obj, vector);
-assert(ret == 0);
+if (msix_enabled(&n->parent_obj)) {
+ret = msix_vector_use(&n->parent_obj, vector);
+assert(ret == 0);
+}
 cq->ctrl = n;
 cq->cqid = cqid;
 cq->size = size;
@@ -4159,9 +4163,12 @@ static void nvme_init_pmr(NvmeCtrl *n, PCIDevice 
*pci_dev)
  PCI_BASE_ADDRESS_MEM_PREFETCH, &n->pmrdev->mr);
 }
 
-static void nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
+static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
 {
 uint8_t *pci_conf = pci_dev->config;
+int ret;
+
+Error *err = NULL;
 
 pci_conf[PCI_INTERRUPT_PIN] = 1;
 pci_config_set_prog_interface(pci_conf, 0x2);
@@ -4181,8 +4188,14 @@ static void nvme_init_pci(NvmeCtrl *n, PCIDevice 
*pci_dev, Error **errp)
   n->reg_size);
 pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
  PCI_BASE_ADDRESS_MEM_TYPE_64, &n->iomem);
-if (msix_init_exclusive_bar(pci_dev, n->params.msix_qsize, 4, errp)) {
-return;
+ret = msix_init_exclusive_bar(pci_dev, n->params.msix_qsize, 4, &err);
+if (ret < 0) {
+if (ret == -ENOTSUP) {
+warn_report_err(err);
+} else {
+error_propagate(errp, err);
+return ret;
+}
 }
 
 if (n->params.cmb_size_mb) {
@@ -4190,6 +4203,8 @@ static void nvme_init_pci(NvmeCtrl *n, PCIDevice 
*pci_dev, Error **errp)
 } else if (n->pmrdev) {
 nvme_init_pmr(n, pci_dev);
 }
+
+return 0;
 }
 
 static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
@@ -4278,9 +4293,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
 &pci_dev->qdev, n->parent_obj.qdev.id);
 
 nvme_init_state(n);
-nvme_init_pci(n, pci_dev, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+if (nvme_init_pci(n, pci_dev, errp)) {
 return;
 }
 
-- 
2.30.0

Re: [PATCH v3 18/25] block/backup: drop extra gotos from backup_run()

[Max Reitz]

On 26.10.20 18:18, Vladimir Sementsov-Ogievskiy wrote:

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/backup.c | 12 +---
  1 file changed, 5 insertions(+), 7 deletions(-)


Reviewed-by: Max Reitz 

Re: [PATCH v3 17/25] block/block-copy: make progress_bytes_callback optional

[Max Reitz]

On 26.10.20 18:18, Vladimir Sementsov-Ogievskiy wrote:

We are going to stop use of this callback in the following commit.
Still the callback handling code will be dropped in a separate commit.
So, for now let's make it optional.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/block-copy.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)


Reviewed-by: Max Reitz 

Re: [PATCH v3 15/25] iotests: 219: prepare for backup over block-copy

[Max Reitz]

On 26.10.20 18:18, Vladimir Sementsov-Ogievskiy wrote:

The further change of moving backup to be a one block-copy call will
make copying chunk-size and cluster-size two separate things. So, even
with 64k cluster sized qcow2 image, default chunk would be 1M.
Test 219 depends on specified chunk-size. Update it for explicit
chunk-size for backup as for mirror.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  tests/qemu-iotests/219 | 13 +++--
  1 file changed, 7 insertions(+), 6 deletions(-)


Reviewed-by: Max Reitz 

Re: [PATCH] hw/ide/ahci: Replace fprintf() by qemu_log_mask(GUEST_ERROR)

[Thomas Huth]

On 12/01/2021 12.29, Philippe Mathieu-Daudé wrote:

Replace fprintf() calls by qemu_log_mask(LOG_GUEST_ERROR).

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/ide/ahci.c | 8 +---
  1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 4b675b9cfd8..6d50482b8d1 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -465,8 +465,9 @@ static void ahci_mem_write(void *opaque, hwaddr addr,
  
  /* Only aligned reads are allowed on AHCI */

  if (addr & 3) {
-fprintf(stderr, "ahci: Mis-aligned write to addr 0x"
-TARGET_FMT_plx "\n", addr);
+qemu_log_mask(LOG_GUEST_ERROR,
+  "ahci: Mis-aligned write to addr 0x%03" HWADDR_PRIX "\n",
+  addr);
  return;
  }
  
@@ -,7 +1112,8 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,

  g_assert(is_ncq(ncq_fis->command));
  if (ncq_tfs->used) {
  /* error - already in use */
-fprintf(stderr, "%s: tag %d already used\n", __func__, tag);
+qemu_log_mask(LOG_GUEST_ERROR, "%s: tag %d already used\n",
+  __func__, tag);
  return;
  }
  



Reviewed-by: Thomas Huth 

Re: [PATCH v3 13/25] iotests: 129: prepare for backup over block-copy

[Max Reitz]

On 26.10.20 18:18, Vladimir Sementsov-Ogievskiy wrote:

After introducing parallel async copy requests instead of plain
cluster-by-cluster copying loop, backup job may finish earlier than
final assertion in do_test_stop. Let's require slow backup explicitly
by specifying speed parameter.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  tests/qemu-iotests/129 | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 3c22f6448c..e9da0208c4 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -77,7 +77,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
  def test_drive_backup(self):
  self.do_test_stop("drive-backup", device="drive0",
target=self.target_img,
-  sync="full")
+  sync="full", speed=1024)
  
  def test_block_commit(self):

  self.do_test_stop("block-commit", device="drive0")


I still don’t like this very much.  We write 128 MB of data, 
do_test_stop throttles the source to 1 kB/s, so even “a lot of parallel 
requests” shouldn’t finish immediately.


I feel like there’s a bigger problem here.

Then again, I have 129 disabled all the time anyway because its 
throttling simply doesn’t work, so I guess


Acked-by: Max Reitz 

Re: [PATCH v3 12/25] iotests: 56: prepare for backup over block-copy

[Max Reitz]

On 26.10.20 18:18, Vladimir Sementsov-Ogievskiy wrote:

After introducing parallel async copy requests instead of plain
cluster-by-cluster copying loop, we'll have to wait for paused status,
as we need to wait for several parallel request. So, let's gently wait
instead of just asserting that job already paused.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  tests/qemu-iotests/056 | 9 +++--
  1 file changed, 7 insertions(+), 2 deletions(-)


Reviewed-by: Max Reitz 

Re: [PATCH v3 11/25] qapi: backup: add max-chunk and max-workers to x-perf struct

[Max Reitz]

On 26.10.20 18:18, Vladimir Sementsov-Ogievskiy wrote:

Add new parameters to configure future backup features. The patch
doesn't introduce aio backup requests (so we actually have only one
worker) neither requests larger than one cluster. Still, formally we
satisfy these maximums anyway, so add the parameters now, to facilitate
further patch which will really change backup job behavior.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  qapi/block-core.json | 11 ++-
  block/backup.c   | 28 +++-
  block/replication.c  |  2 +-
  blockdev.c   |  8 +++-
  4 files changed, 41 insertions(+), 8 deletions(-)


[...]


diff --git a/block/backup.c b/block/backup.c
index 09ff5a92ef..8c67d77504 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -388,6 +388,29 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
  return NULL;
  }
  
+cluster_size = backup_calculate_cluster_size(target, errp);

+if (cluster_size < 0) {
+goto error;
+}
+
+if (perf->max_workers < 1) {
+error_setg(errp, "max-worker must be greater than zero");


*max-workers

With that fixed:

Reviewed-by: Max Reitz 


+return NULL;
+}

Re: [PATCH v6 3/7] qemu: add support for iOS host

[Philippe Mathieu-Daudé]

Hi Joelle,

On 1/5/21 3:20 AM, Joelle van Dyne wrote:
> This introduces support for building for iOS hosts. When the correct Xcode
> toolchain is used, iOS host will be detected automatically.
> 
> * block: disable features not supported by iOS sandbox
> * slirp: disable SMB features for iOS
> * osdep: disable system() calls for iOS
> 
> Signed-off-by: Joelle van Dyne 
> ---
>  docs/devel/index.rst|  1 +
>  docs/devel/ios.rst  | 28 +++
>  configure   | 43 -
>  meson.build |  2 +-
>  include/qemu/osdep.h| 11 +++
>  block.c |  2 +-
>  block/file-posix.c  | 31 +
>  net/slirp.c | 16 +++
>  qga/commands-posix.c|  6 ++
>  MAINTAINERS |  7 +++
>  tests/qtest/meson.build |  7 +++
>  11 files changed, 127 insertions(+), 27 deletions(-)
>  create mode 100644 docs/devel/ios.rst

> 
> diff --git a/docs/devel/index.rst b/docs/devel/index.rst
> index f10ed77e4c..2cc8a13ebe 100644
> --- a/docs/devel/index.rst
> +++ b/docs/devel/index.rst
> @@ -35,3 +35,4 @@ Contents:
> clocks
> qom
> block-coroutine-wrapper
> +   ios
> diff --git a/docs/devel/ios.rst b/docs/devel/ios.rst
> new file mode 100644
> index 00..b4ab11bec1
> --- /dev/null
> +++ b/docs/devel/ios.rst
> @@ -0,0 +1,28 @@
> +===
> +iOS Support
> +===
> +
> +To run qemu on the iOS platform, some modifications were required. Most of 
> the
> +modifications are conditioned on the ``CONFIG_IOS`` and configuration 
> variable.
> +
> +Build support
> +-
> +
> +For the code to compile, certain changes in the block driver and the slirp
> +driver had to be made. There is no ``system()`` call, so it has been replaced
> +with an assertion error. There should be no code path that call system() from
> +iOS.
> +
> +``ucontext`` support is broken on iOS. The implementation from 
> ``libucontext``
> +is used instead.

Do you have a CI testing plan for these builds?

Is it possible to add a Gitlab-CI job? If not, on Cirrus-CI?

Thanks,

Phil.

Re: [PATCH] tests/iotests: drop test 312 from auto group

[Kevin Wolf]

Am 05.01.2021 um 11:04 hat Alex Bennée geschrieben:
> The "auto" documentation states:
> 
>   That means they should run with every QEMU binary (also non-x86)
> 
> which is not the case as the check-system-fedora build which only
> includes a rag tag group of rare and deprecated targets doesn't
> support the virtio device required.
> 
> Signed-off-by: Alex Bennée 

I think the better solution would be to do something like in 192 so that
the test is still run at least for one binary:

if [ "$QEMU_DEFAULT_MACHINE" != "pc" ]; then
_notrun "Requires a PC machine"
fi

Kevin

Re: [PATCH v3 08/25] block/block-copy: add block_copy_cancel

[Max Reitz]

On 26.10.20 18:17, Vladimir Sementsov-Ogievskiy wrote:

Add function to cancel running async block-copy call. It will be used
in backup.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/block/block-copy.h | 13 +
  block/block-copy.c | 24 +++-
  2 files changed, 32 insertions(+), 5 deletions(-)


Reviewed-by: Max Reitz 

[PATCH] hw/ide/ahci: Replace fprintf() by qemu_log_mask(GUEST_ERROR)

[Philippe Mathieu-Daudé]

Replace fprintf() calls by qemu_log_mask(LOG_GUEST_ERROR).

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/ide/ahci.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 4b675b9cfd8..6d50482b8d1 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -465,8 +465,9 @@ static void ahci_mem_write(void *opaque, hwaddr addr,
 
 /* Only aligned reads are allowed on AHCI */
 if (addr & 3) {
-fprintf(stderr, "ahci: Mis-aligned write to addr 0x"
-TARGET_FMT_plx "\n", addr);
+qemu_log_mask(LOG_GUEST_ERROR,
+  "ahci: Mis-aligned write to addr 0x%03" HWADDR_PRIX "\n",
+  addr);
 return;
 }
 
@@ -,7 +1112,8 @@ static void process_ncq_command(AHCIState *s, int port, 
uint8_t *cmd_fis,
 g_assert(is_ncq(ncq_fis->command));
 if (ncq_tfs->used) {
 /* error - already in use */
-fprintf(stderr, "%s: tag %d already used\n", __func__, tag);
+qemu_log_mask(LOG_GUEST_ERROR, "%s: tag %d already used\n",
+  __func__, tag);
 return;
 }
 
-- 
2.26.2

Re: [PATCH v3 07/25] block/block-copy: add ratelimit to block-copy

[Max Reitz]

On 26.10.20 18:17, Vladimir Sementsov-Ogievskiy wrote:

We are going to directly use one async block-copy operation for backup
job, so we need rate limiter.

We want to maintain current backup behavior: only background copying is
limited and copy-before-write operations only participate in limit
calculation. Therefore we need one rate limiter for block-copy state
and boolean flag for block-copy call state for actual limitation.

Note, that we can't just calculate each chunk in limiter after
successful copying: it will not save us from starting a lot of async
sub-requests which will exceed limit too much. Instead let's use the
following scheme on sub-request creation:
1. If at the moment limit is not exceeded, create the request and
account it immediately.
2. If at the moment limit is already exceeded, drop create sub-request
and handle limit instead (by sleep).
With this approach we'll never exceed the limit more than by one
sub-request (which pretty much matches current backup behavior).

Note also, that if there is in-flight block-copy async call,
block_copy_kick() should be used after set-speed to apply new setup
faster. For that block_copy_kick() published in this patch.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/block/block-copy.h |  5 -
  block/backup-top.c |  2 +-
  block/backup.c |  2 +-
  block/block-copy.c | 46 +-
  4 files changed, 51 insertions(+), 4 deletions(-)


Reviewed-by: Max Reitz 

Re: [PATCH v2 1/1] hw/block/nvme: add smart_critical_warning property

[Klaus Jensen]

On Jan 12 15:49, zhenwei pi wrote:
> There is a very low probability that hitting physical NVMe disk
> hardware critical warning case, it's hard to write & test a monitor
> agent service.
> 
> For debugging purposes, add a new 'smart_critical_warning' property
> to emulate this situation.
> 
> The orignal version of this change is implemented by adding a fixed
> property which could be initialized by QEMU command line. Suggested
> by Philippe & Klaus, rework like current version.
> 
> Test with this patch:
> 1, change smart_critical_warning property for a running VM:
>  #virsh qemu-monitor-command nvme-upstream '{ "execute": "qom-set",
>   "arguments": { "path": "/machine/peripheral-anon/device[0]",
>   "property": "smart_critical_warning", "value":16 } }'
> 2, run smartctl in guest
>  #smartctl -H -l error /dev/nvme0n1
> 
>   === START OF SMART DATA SECTION ===
>   SMART overall-health self-assessment test result: FAILED!
>   - volatile memory backup device has failed
> 
> Signed-off-by: zhenwei pi 

I think we also need to check the asynchronous event configuration and
issue an AEN if required like we do when the temperature threshold
changes.

Philippe, what are the locking semantics here? This runs under the big
lock?

> ---
>  hw/block/nvme.c | 28 
>  hw/block/nvme.h |  1 +
>  2 files changed, 29 insertions(+)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 27d2c72716..a98757b6a1 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1214,6 +1214,7 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t 
> rae, uint32_t buf_len,
>  }
>  
>  trans_len = MIN(sizeof(smart) - off, buf_len);
> +smart.critical_warning = n->smart_critical_warning;
>  
>  smart.data_units_read[0] = cpu_to_le64(DIV_ROUND_UP(stats.units_read,
>  1000));
> @@ -2827,6 +2828,29 @@ static Property nvme_props[] = {
>  DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +
> +static void nvme_get_smart_warning(Object *obj, Visitor *v, const char *name,
> +   void *opaque, Error **errp)
> +{
> +NvmeCtrl *s = NVME(obj);
> +uint8_t value = s->smart_critical_warning;
> +
> +visit_type_uint8(v, name, &value, errp);
> +}
> +
> +static void nvme_set_smart_warning(Object *obj, Visitor *v, const char *name,
> +   void *opaque, Error **errp)
> +{
> +NvmeCtrl *s = NVME(obj);
> +uint8_t value;
> +
> +if (!visit_type_uint8(v, name, &value, errp)) {
> +return;
> +}
> +
> +s->smart_critical_warning = value;
> +}
> +
>  static const VMStateDescription nvme_vmstate = {
>  .name = "nvme",
>  .unmigratable = 1,
> @@ -2857,6 +2881,10 @@ static void nvme_instance_init(Object *obj)
>"bootindex", "/namespace@1,0",
>DEVICE(obj));
>  }
> +
> +object_property_add(obj, "smart_critical_warning", "uint8",
> +nvme_get_smart_warning,
> +nvme_set_smart_warning, NULL, NULL);
>  }
>  
>  static const TypeInfo nvme_info = {
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index e080a2318a..64e3497244 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -139,6 +139,7 @@ typedef struct NvmeCtrl {
>  uint64_ttimestamp_set_qemu_clock_ms;/* QEMU clock time */
>  uint64_tstarttime_ms;
>  uint16_ttemperature;
> +uint8_t smart_critical_warning;
>  
>  HostMemoryBackend *pmrdev;
>  
> -- 
> 2.25.1
> 
> 

-- 
One of us - No more doubt, silence or taboo about mental illness.


signature.asc
Description: PGP signature

Re: [PATCH 0/2] sysemu: Let VMChangeStateHandler take boolean 'running' argument

[Stefan Hajnoczi]

On Mon, Jan 11, 2021 at 04:20:18PM +0100, Philippe Mathieu-Daudé wrote:
> Trivial prototype change to clarify the use of the 'running'
> argument of VMChangeStateHandler.
> 
> Green CI:
> https://gitlab.com/philmd/qemu/-/pipelines/239497352
> 
> Philippe Mathieu-Daudé (2):
>   sysemu/runstate: Let runstate_is_running() return bool
>   sysemu: Let VMChangeStateHandler take boolean 'running' argument
> 
>  include/sysemu/runstate.h   | 12 +---
>  target/arm/kvm_arm.h|  2 +-
>  target/ppc/cpu-qom.h|  2 +-
>  accel/xen/xen-all.c |  2 +-
>  audio/audio.c   |  2 +-
>  block/block-backend.c   |  2 +-
>  gdbstub.c   |  2 +-
>  hw/block/pflash_cfi01.c |  2 +-
>  hw/block/virtio-blk.c   |  2 +-
>  hw/display/qxl.c|  2 +-
>  hw/i386/kvm/clock.c |  2 +-
>  hw/i386/kvm/i8254.c |  2 +-
>  hw/i386/kvmvapic.c  |  2 +-
>  hw/i386/xen/xen-hvm.c   |  2 +-
>  hw/ide/core.c   |  2 +-
>  hw/intc/arm_gicv3_its_kvm.c |  2 +-
>  hw/intc/arm_gicv3_kvm.c |  2 +-
>  hw/intc/spapr_xive_kvm.c|  2 +-
>  hw/misc/mac_via.c   |  2 +-
>  hw/net/e1000e_core.c|  2 +-
>  hw/nvram/spapr_nvram.c  |  2 +-
>  hw/ppc/ppc.c|  2 +-
>  hw/ppc/ppc_booke.c  |  2 +-
>  hw/s390x/tod-kvm.c  |  2 +-
>  hw/scsi/scsi-bus.c  |  2 +-
>  hw/usb/hcd-ehci.c   |  2 +-
>  hw/usb/host-libusb.c|  2 +-
>  hw/usb/redirect.c   |  2 +-
>  hw/vfio/migration.c |  2 +-
>  hw/virtio/virtio-rng.c  |  2 +-
>  hw/virtio/virtio.c  |  2 +-
>  net/net.c   |  2 +-
>  softmmu/memory.c|  2 +-
>  softmmu/runstate.c  |  4 ++--
>  target/arm/kvm.c|  2 +-
>  target/i386/kvm/kvm.c   |  2 +-
>  target/i386/sev.c   |  2 +-
>  target/i386/whpx/whpx-all.c |  2 +-
>  target/mips/kvm.c   |  4 ++--
>  ui/gtk.c|  2 +-
>  ui/spice-core.c |  2 +-
>  41 files changed, 51 insertions(+), 45 deletions(-)
> 
> -- 
> 2.26.2
> 
> 

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

[PATCH] hw/block/nvme: fix zone write finalize

[Klaus Jensen]

From: Klaus Jensen 

The zone write pointer is unconditionally advanced, even for write
faults. Make sure that the zone is always transitioned to Full if the
write pointer reaches zone capacity.

Signed-off-by: Klaus Jensen 
Cc: Dmitry Fomichev 
---
 hw/block/nvme.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 0854ee307227..280b31b4459d 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1268,10 +1268,13 @@ static void nvme_finalize_zoned_write(NvmeNamespace 
*ns, NvmeRequest *req,
 nlb = le16_to_cpu(rw->nlb) + 1;
 zone = nvme_get_zone_by_slba(ns, slba);
 
+zone->d.wp += nlb;
+
 if (failed) {
 res->slba = 0;
-zone->d.wp += nlb;
-} else if (zone->w_ptr == nvme_zone_wr_boundary(zone)) {
+}
+
+if (zone->d.wp == nvme_zone_wr_boundary(zone)) {
 switch (nvme_get_zone_state(zone)) {
 case NVME_ZONE_STATE_IMPLICITLY_OPEN:
 case NVME_ZONE_STATE_EXPLICITLY_OPEN:
@@ -1288,9 +1291,6 @@ static void nvme_finalize_zoned_write(NvmeNamespace *ns, 
NvmeRequest *req,
 default:
 assert(false);
 }
-zone->d.wp = zone->w_ptr;
-} else {
-zone->d.wp += nlb;
 }
 }
 
-- 
2.30.0