Re: [Qemu-devel] [PATCH for-2.11 5/6] ppc: simplify cpu model lookup by PVR

[David Gibson]

On Wed, Aug 30, 2017 at 12:50:33PM +0200, Igor Mammedov wrote:
> On Fri, 25 Aug 2017 19:32:29 +1000
> David Gibson  wrote:
> 
> > On Fri, Aug 25, 2017 at 09:40:37AM +0200, Igor Mammedov wrote:
> > > On Fri, 25 Aug 2017 14:16:44 +1000
> > > David Gibson  wrote:
> > >   
> > > > On Thu, Aug 24, 2017 at 10:21:50AM +0200, Igor Mammedov wrote:  
> > > > > Signed-off-by: Igor Mammedov 
> > > > > ---
> > > > >  target/ppc/translate_init.c | 27 +++
> > > > >  1 file changed, 11 insertions(+), 16 deletions(-)
> > > > > 
> > > > > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> > > > > index f1a559d..ca9f1e3 100644
> > > > > --- a/target/ppc/translate_init.c
> > > > > +++ b/target/ppc/translate_init.c
> > > > > @@ -34,6 +34,7 @@
> > > > >  #include "hw/ppc/ppc.h"
> > > > >  #include "mmu-book3s-v3.h"
> > > > >  #include "sysemu/qtest.h"
> > > > > +#include "qemu/cutils.h"
> > > > >  
> > > > >  //#define PPC_DUMP_CPU
> > > > >  //#define PPC_DEBUG_SPR
> > > > > @@ -10203,22 +10204,16 @@ static ObjectClass 
> > > > > *ppc_cpu_class_by_name(const char *name)
> > > > >  char *cpu_model, *typename;
> > > > >  ObjectClass *oc;
> > > > >  const char *p;
> > > > > -int i, len;
> > > > > -
> > > > > -/* Check if the given name is a PVR */
> > > > > -len = strlen(name);
> > > > > -if (len == 10 && name[0] == '0' && name[1] == 'x') {
> > > > > -p = name + 2;
> > > > > -goto check_pvr;
> > > > > -} else if (len == 8) {
> > > > > -p = name;
> > > > > -check_pvr:
> > > > > -for (i = 0; i < 8; i++) {
> > > > > -if (!qemu_isxdigit(*p++))
> > > > > -break;
> > > > > -}
> > > > > -if (i == 8) {
> > > > > -return OBJECT_CLASS(ppc_cpu_class_by_pvr(strtoul(name, 
> > > > > NULL, 16)));
> > > > > +unsigned long pvr;
> > > > > +
> > > > > +/* Lookup by PVR if cpu_model is valid 8 digit hex number
> > > > > + * (excl: 0x prefix if present)
> > > > > + */
> > > > > +if (!qemu_strtoul(name, , 16, )) {
> > > > > +int len = p - name;
> > > > > +len = (len == 10) && (name[1] == 'x') ? len - 2 : len;
> > > > > +if (len == 8) {
> > > > > +return OBJECT_CLASS(ppc_cpu_class_by_pvr(pvr));
> > > > 
> > > > This doesn't look quite right.  A hex string of a PVR followed by
> > > > other stuff isn't a valid option, so if p (endptr) doesn't point to
> > > > the end of the string, then we shouldn't be using this path  
> > > yep, my mistake, I've lost somewhere *p == '\0' check when cleaning up 
> > > the patch  
> > 
> > Ok.
> > 
> > > >(from the
> > > > looks of qemu_strtoul() we can simply omit the endptr parameter to get
> > > > that behaviour).  I'm not sure what the messing around with len is
> > > > for, either.  If it's a valid hex string we can try this, I don't see
> > > > that we need further checks.  
> > > I've tried to keep current limitation here i.e. 8 hex symbols,
> > > but if any hex number is fine then doing
> > >  qemu_strtoul(name, NULL, 16, )
> > > is even better, so would you prefer to drop 8 symbol check altogether?  
> > 
> > Yeah, I don't see any value to the 8 character check.
> there are cpu models consisting of pure numbers => valid hex number
> so if check is dropped then it lookup will go PVR route,
> that will fail there.
> So check should be there to avoid regression.

Ah.  Good point.

> I'll fix whole string consumption check that I've missed before
> and respin with it.
> 
> 
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 1/9] s390x/css: fix cc handling for XSCH

[Thomas Huth]

On 30.08.2017 18:36, Halil Pasic wrote:
> The function ioinst_handle_xsch is presenting cc 2 when it's supposed to
> present cc 1 and the other way around, because css_do_xsch has the error
> codes mixed up. Fixing the error codes also fixes the priority.
> 
> Let us fix this.

(Nit: In case you respin, I'd suggest to remove the last sentence. You
already used "fix" two times in the previous one)

> Signed-off-by: Halil Pasic 
> Reported-by: Pierre Morel

Space missing -^

> ---
>  hw/s390x/css.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 1880b1a0ff..a50fb0727e 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1281,12 +1281,12 @@ int css_do_xsch(SubchDev *sch)
>  (!(s->ctrl &
> (SCSW_ACTL_RESUME_PEND | SCSW_ACTL_START_PEND | SCSW_ACTL_SUSP))) 
> ||
>  (s->ctrl & SCSW_ACTL_SUBCH_ACTIVE)) {
> -ret = -EINPROGRESS;
> +ret = -EBUSY;
>  goto out;
>  }
>  
>  if (s->ctrl & SCSW_CTRL_MASK_STCTL) {
> -ret = -EBUSY;
> +ret = -EINPROGRESS;
>  goto out;
>  }

Using both, EBUSY and EINPROGRESS as error codes sounds very confusing
to me here ... what's the difference between busy and in-progress? So
while you're at it, maybe you could replace the code for CC 2 ("CANCEL
SUBCHANNEL not applicable") with a different error code?

 Thomas

Re: [Qemu-devel] [PATCH for-2.11 v3 0/3] hw/ppc: CAS reset on early device hotplug

[David Gibson]

On Wed, Aug 30, 2017 at 03:21:38PM -0300, Daniel Henrique Barboza wrote:
> v3:
> - split into 3 patches, first 2 are fixes that are independent of the
> reboot code that can be applied separately:
> - patch 1: spapr_drc_needed fix
> - patch 2: clear pending_events on reboot, following David's
> suggestions on the v2 review.
> - patch 3: CAS reboot
> 
> v2:
> - rebased with ppc-for-2.11
> - function 'spapr_cas_completed' dropped
> - function 'spapr_drc_needed' made public and it's now used inside
>   'spapr_hotplugged_dev_before_cas'
> - 'spapr_drc_needed' was changed to support the migration of logical
>   DRCs with devs attached in UNUSED state
> - new function: 'spapr_clear_pending_events'. This function is used
>   inside ppc_spapr_reset to reset the pending_events QTAILQ

Applied to ppc-for-2.11.  The first two I'm also considering sending
off for the 2.10 stable branch.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v2 02/17] MAINTAINERS: add missing Versatile PB entry

[Thomas Huth]

On 30.08.2017 23:55, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b363e1b9c9..5b7891addc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -515,6 +515,7 @@ M: Peter Maydell 
>  L: qemu-...@nongnu.org
>  S: Maintained
>  F: hw/*/versatile*
> +F: hw/misc/arm_sysctl.c

I think you could also merge this into the previous patch. Anyway:

Reviewed-by: Thomas Huth 

Re: [Qemu-devel] [PATCH 1/7] serial: add serial_chr_nonnull() to use the null backend when none provided

[Thomas Huth]

On 31.08.2017 05:53, Philippe Mathieu-Daudé wrote:
> Suggested-by: Peter Maydell 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/hw/char/serial.h |  1 +
>  hw/char/serial.c | 13 +
>  2 files changed, 14 insertions(+)
> 
> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> index c4daf11a14..96bccb39e1 100644
> --- a/include/hw/char/serial.h
> +++ b/include/hw/char/serial.h
> @@ -93,6 +93,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
>  hwaddr base, int it_shift,
>  qemu_irq irq, int baudbase,
>  Chardev *chr, enum device_endian end);
> +Chardev *serial_chr_nonnull(Chardev *chr);

Why do you need the prototype? Please make the function static if
possible (otherwise please add some rationale in the patch description).

>  /* serial-isa.c */
>  #define TYPE_ISA_SERIAL "isa-serial"
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 9aec6c60d8..7a100db107 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -1025,6 +1025,19 @@ static const MemoryRegionOps serial_mm_ops[3] = {
>  },
>  };
>  
> +Chardev *serial_chr_nonnull(Chardev *chr)
> +{
> +static int serial_id;
> +char *label;
> +
> +label = g_strdup_printf("discarding-serial%d", serial_id++);
> +chr = qemu_chr_new(label, "null");

That looks wrong - you're ignoring the input parameter and always open
the "null" device? Shouldn't there be a "if (chr) return chr;" in front
of this?

> +assert(chr);
> +g_free(label);
> +
> +return chr;
> +}

 Thomas

PS: I think you should also merge the two patches together, they are
small enough.

Re: [Qemu-devel] [PATCH v2 01/17] MAINTAINERS: add missing ARM entries

[Thomas Huth]

On 30.08.2017 23:55, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  MAINTAINERS | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ccee28b12d..b363e1b9c9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -380,6 +380,7 @@ M: Peter Maydell 
>  L: qemu-...@nongnu.org
>  S: Maintained
>  F: hw/char/pl011.c
> +F: include/hw/char/pl011.h
>  F: hw/display/pl110*
>  F: hw/dma/pl080.c
>  F: hw/dma/pl330.c
> @@ -403,13 +404,15 @@ F: hw/intc/gic_internal.h
>  F: hw/misc/a9scu.c
>  F: hw/misc/arm11scu.c
>  F: hw/timer/a9gtimer*
> -F: hw/timer/arm_*
> -F: include/hw/arm/arm.h
> +F: hw/timer/arm*
> +F: include/hw/arm/arm*.h
>  F: include/hw/intc/arm*
>  F: include/hw/misc/a9scu.h
>  F: include/hw/misc/arm11scu.h
>  F: include/hw/timer/a9gtimer.h
>  F: include/hw/timer/arm_mptimer.h
> +F: include/hw/timer/armv7m_systick.h
> +F: tests/test-arm-mptimer.c
>  
>  Exynos
>  M: Igor Mitsyanko 

Reviewed-by: Thomas Huth 

Re: [Qemu-devel] [PATCH v2 00/17] add missing entries in MAINTAINERS

[Philippe Mathieu-Daudé]

Cc'ing Markus since I forgot to include him.

On 08/30/2017 06:55 PM, Philippe Mathieu-Daudé wrote:

Hi,

I tried to have a more helpful ./scripts/get_maintainer.pl output, filling
missing entries in MAINTAINERS.

Regards,

Phil.

v2:
- add R-b & A-b
- clean ARM entries (Thomas Huth)
- moved files: comment since which commit (Eric Blake)
- drop inconsistent patches (default-configs.mak related to hw/machine, no CPU)
- drop unhappy patches (include/standard-headers/linux/*)
- drop unreplied patches

Philippe Mathieu-Daudé (17):
   MAINTAINERS: add missing ARM entries
   MAINTAINERS: add missing Versatile PB entry
   MAINTAINERS: add missing STM32 entry
   MAINTAINERS: add missing entry for vhost
   MAINTAINERS: add missing VMWare entry
   MAINTAINERS: add missing Guest Agent entries
   MAINTAINERS: add missing qcow2 entry
   MAINTAINERS: add missing USB entry
   MAINTAINERS: add missing PCI entries
   MAINTAINERS: add missing SSI entries
   MAINTAINERS: add missing entries for throttling infra
   MAINTAINERS: add missing megasas test entry
   MAINTAINERS: add missing AIO entry
   MAINTAINERS: add missing entry for Generic Loader
   MAINTAINERS: add missing Cryptography entry
   MAINTAINERS: update docs/devel/ entries
   MAINTAINERS: update docs/interop/ entries

  MAINTAINERS | 44 ++--
  1 file changed, 34 insertions(+), 10 deletions(-)

[Qemu-devel] [PATCH 5/7] hw/char/exynos4210_uart: use serial_chr_nonnull()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
This ARRAY_SIZE() first surprised me but was valid :)

 hw/char/exynos4210_uart.c | 14 ++
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
index 3957e78abf..b6cdfc3006 100644
--- a/hw/char/exynos4210_uart.c
+++ b/hw/char/exynos4210_uart.c
@@ -27,6 +27,7 @@
 #include "chardev/char-serial.h"
 
 #include "hw/arm/exynos4210.h"
+#include "hw/char/serial.h"
 
 #undef DEBUG_UART
 #undef DEBUG_UART_EXTEND
@@ -589,9 +590,6 @@ DeviceState *exynos4210_uart_create(hwaddr addr,
 DeviceState  *dev;
 SysBusDevice *bus;
 
-const char chr_name[] = "serial";
-char label[ARRAY_SIZE(chr_name) + 1];
-
 dev = qdev_create(NULL, TYPE_EXYNOS4210_UART);
 
 if (!chr) {
@@ -600,15 +598,7 @@ DeviceState *exynos4210_uart_create(hwaddr addr,
  MAX_SERIAL_PORTS);
 exit(1);
 }
-chr = serial_hds[channel];
-if (!chr) {
-snprintf(label, ARRAY_SIZE(label), "%s%d", chr_name, channel);
-chr = qemu_chr_new(label, "null");
-if (!(chr)) {
-error_report("Can't assign serial port to UART%d", channel);
-exit(1);
-}
-}
+chr = serial_chr_nonnull(serial_hds[channel]);
 }
 
 qdev_prop_set_chr(dev, "chardev", chr);
-- 
2.14.1

[Qemu-devel] [PATCH 6/7] hw/char/omap_uart: serial_mm_init() already check for null chr

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/char/omap_uart.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/char/omap_uart.c b/hw/char/omap_uart.c
index 6fd1b9cf6b..1f0ac0a053 100644
--- a/hw/char/omap_uart.c
+++ b/hw/char/omap_uart.c
@@ -63,8 +63,7 @@ struct omap_uart_s *omap_uart_init(hwaddr base,
 s->irq = irq;
 s->serial = serial_mm_init(get_system_memory(), base, 2, irq,
omap_clk_getrate(fclk)/16,
-   chr ?: qemu_chr_new(label, "null"),
-   DEVICE_NATIVE_ENDIAN);
+   chr, DEVICE_NATIVE_ENDIAN);
 return s;
 }
 
@@ -183,6 +182,5 @@ void omap_uart_attach(struct omap_uart_s *s, Chardev *chr)
 /* TODO: Should reuse or destroy current s->serial */
 s->serial = serial_mm_init(get_system_memory(), s->base, 2, s->irq,
omap_clk_getrate(s->fclk) / 16,
-   chr ?: qemu_chr_new("null", "null"),
-   DEVICE_NATIVE_ENDIAN);
+   chr, DEVICE_NATIVE_ENDIAN);
 }
-- 
2.14.1

Re: [Qemu-devel] [PATCH v2 0/7] QOMify MIPS cpu

[Philippe Mathieu-Daudé]

Cc'ing Xiaoqiang Zhao, I forgot to include him.

On 08/30/2017 07:52 PM, Philippe Mathieu-Daudé wrote:

Hi,

This series is based on Igor's "complete cpu QOMification" [1] but only modify
the MIPS part. Igor posted an updated series [2], both series should apply
separately.

Igor suggested on IRC this series could enter via the Machine core tree, so
I added Eduardo and Marcel.

Regards,

Phil.

[1]: http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg04414.html
[2]: http://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg03364.html

v2:
- added Igor and James Tested-by
- squashed "!fixup mips: now than MIPSCPU is QOMified, mark it abstract"

PS: code movement somehow triggers a "binary vs unary operators" confusion
 in checkpatch: "ERROR: space prohibited after that '&' (ctx:WxW)"

Igor Mammedov (2):
   mips: MIPSCPU model subclasses
   mips: replace cpu_mips_init() with cpu_generic_init()

Philippe Mathieu-Daudé (5):
   mips: move hw/mips/cputimer.c to target/mips/
   mips: introduce internal.h and cleanup cpu.h
   mips: split cpu_mips_realize_env() out of cpu_mips_init()
   mips: call cpu_mips_realize_env() from mips_cpu_realizefn()
   mips: update mips_cpu_list() to use object_class_get_list()

  target/mips/cpu-qom.h |   1 +
  target/mips/cpu.h | 357 +-
  target/mips/internal.h| 422 ++
  hw/mips/cps.c |   2 +-
  hw/mips/mips_fulong2e.c   |   2 +-
  hw/mips/mips_jazz.c   |   2 +-
  hw/mips/mips_malta.c  |   2 +-
  hw/mips/mips_mipssim.c|   2 +-
  hw/mips/mips_r4k.c|   2 +-
  hw/mips/cputimer.c => target/mips/cp0_timer.c |   2 +-
  target/mips/cpu.c |  57 +++-
  target/mips/gdbstub.c |   1 +
  target/mips/helper.c  |  47 +++
  target/mips/kvm.c |   1 +
  target/mips/machine.c |   1 +
  target/mips/msa_helper.c  |   1 +
  target/mips/op_helper.c   |   1 +
  target/mips/translate.c   |  23 +-
  target/mips/translate_init.c  |  68 +
  hw/mips/Makefile.objs |   2 +-
  target/mips/Makefile.objs |   2 +-
  21 files changed, 549 insertions(+), 449 deletions(-)
  create mode 100644 target/mips/internal.h
  rename hw/mips/cputimer.c => target/mips/cp0_timer.c (99%)

[Qemu-devel] [PATCH 7/7] hw/xtensa: serial_mm_init() already check for null chr

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/xtensa/xtfpga.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
index 635a4d4ec3..223f396902 100644
--- a/hw/xtensa/xtfpga.c
+++ b/hw/xtensa/xtfpga.c
@@ -263,10 +263,6 @@ static void lx_init(const LxBoardDesc *board, MachineState 
*machine)
 xtensa_get_extint(env, 1), nd_table);
 }
 
-if (!serial_hds[0]) {
-serial_hds[0] = qemu_chr_new("serial0", "null");
-}
-
 serial_mm_init(system_io, 0x0d050020, 2, xtensa_get_extint(env, 0),
 115200, serial_hds[0], DEVICE_NATIVE_ENDIAN);
 
-- 
2.14.1

[Qemu-devel] [PATCH 3/7] hw/arm/fsl_imx*: use serial_chr_nonnull()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/char/imx_serial.h |  1 +
 hw/arm/fsl-imx25.c   |  9 +
 hw/arm/fsl-imx31.c   |  9 +
 hw/arm/fsl-imx6.c| 10 +-
 4 files changed, 4 insertions(+), 25 deletions(-)

diff --git a/include/hw/char/imx_serial.h b/include/hw/char/imx_serial.h
index baeec3183f..55139dc6ec 100644
--- a/include/hw/char/imx_serial.h
+++ b/include/hw/char/imx_serial.h
@@ -20,6 +20,7 @@
 
 #include "hw/sysbus.h"
 #include "chardev/char-fe.h"
+#include "hw/char/serial.h"
 
 #define TYPE_IMX_SERIAL "imx.serial"
 #define IMX_SERIAL(obj) OBJECT_CHECK(IMXSerialState, (obj), TYPE_IMX_SERIAL)
diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index 3b97eceb3c..425a9edc36 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -120,14 +120,7 @@ static void fsl_imx25_realize(DeviceState *dev, Error 
**errp)
 if (i < MAX_SERIAL_PORTS) {
 Chardev *chr;
 
-chr = serial_hds[i];
-
-if (!chr) {
-char label[20];
-snprintf(label, sizeof(label), "imx31.uart%d", i);
-chr = qemu_chr_new(label, "null");
-}
-
+chr = serial_chr_nonnull(serial_hds[i]);
 qdev_prop_set_chr(DEVICE(>uart[i]), "chardev", chr);
 }
 
diff --git a/hw/arm/fsl-imx31.c b/hw/arm/fsl-imx31.c
index 0f2ebe8161..8d4535a536 100644
--- a/hw/arm/fsl-imx31.c
+++ b/hw/arm/fsl-imx31.c
@@ -109,14 +109,7 @@ static void fsl_imx31_realize(DeviceState *dev, Error 
**errp)
 if (i < MAX_SERIAL_PORTS) {
 Chardev *chr;
 
-chr = serial_hds[i];
-
-if (!chr) {
-char label[20];
-snprintf(label, sizeof(label), "imx31.uart%d", i);
-chr = qemu_chr_new(label, "null");
-}
-
+chr = serial_chr_nonnull(serial_hds[i]);
 qdev_prop_set_chr(DEVICE(>uart[i]), "chardev", chr);
 }
 
diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index 26fd214004..7bc1aa1fbe 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -189,15 +189,7 @@ static void fsl_imx6_realize(DeviceState *dev, Error 
**errp)
 if (i < MAX_SERIAL_PORTS) {
 Chardev *chr;
 
-chr = serial_hds[i];
-
-if (!chr) {
-char *label = g_strdup_printf("imx6.uart%d", i + 1);
-chr = qemu_chr_new(label, "null");
-g_free(label);
-serial_hds[i] = chr;
-}
-
+chr = serial_chr_nonnull(serial_hds[i]);
 qdev_prop_set_chr(DEVICE(>uart[i]), "chardev", chr);
 }
 
-- 
2.14.1

[Qemu-devel] [PATCH 2/7] serial: use serial_chr_nonnull() in serial_mm_init()

[Philippe Mathieu-Daudé]

Suggested-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/char/serial.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index e4c38dc250..57e89468b4 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -1050,7 +1050,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
 s->it_shift = it_shift;
 s->irq = irq;
 s->baudbase = baudbase;
-qemu_chr_fe_init(>chr, chr, _abort);
+qemu_chr_fe_init(>chr, serial_chr_nonnull(chr), _abort);
 
 serial_realize_core(s, _fatal);
 vmstate_register(NULL, base, _serial, s);
-- 
2.14.1

[Qemu-devel] [PATCH 0/7] serial: add serial_chr_nonnull()

[Philippe Mathieu-Daudé]

Hi,

This series add the serial_chr_nonnull() which connect to the "null" chardev
backend if none is provided.

Inspired by Peter's suggestion:
http://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg05987.html
which also refers to this issue:
http://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg03546.html

Some boards still check serial_hds[x] non null before calling serial_mm_init(),
this check could now be removed on the SoC which always have an UART mapped.

This might be a follow up if this series is accepted.

Regards,

Phil.

Philippe Mathieu-Daudé (7):
  serial: add serial_chr_nonnull() to use the null backend when none provided
  serial: use serial_chr_nonnull() in serial_mm_init()
  hw/arm/fsl_imx*: use serial_chr_nonnull()
  hw/mips/malta: use serial_chr_nonnull()
  hw/char/exynos4210_uart: use serial_chr_nonnull()
  hw/char/omap_uart: serial_mm_init() already check for null chr
  hw/xtensa: serial_mm_init() already check for null chr

 include/hw/char/imx_serial.h |  1 +
 include/hw/char/serial.h |  1 +
 hw/arm/fsl-imx25.c   |  9 +
 hw/arm/fsl-imx31.c   |  9 +
 hw/arm/fsl-imx6.c| 10 +-
 hw/char/exynos4210_uart.c| 14 ++
 hw/char/omap_uart.c  |  6 ++
 hw/char/serial.c | 15 ++-
 hw/mips/mips_malta.c |  6 +-
 hw/xtensa/xtfpga.c   |  4 
 10 files changed, 24 insertions(+), 51 deletions(-)

-- 
2.14.1

[Qemu-devel] [PATCH 1/7] serial: add serial_chr_nonnull() to use the null backend when none provided

[Philippe Mathieu-Daudé]

Suggested-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/char/serial.h |  1 +
 hw/char/serial.c | 13 +
 2 files changed, 14 insertions(+)

diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index c4daf11a14..96bccb39e1 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -93,6 +93,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
 hwaddr base, int it_shift,
 qemu_irq irq, int baudbase,
 Chardev *chr, enum device_endian end);
+Chardev *serial_chr_nonnull(Chardev *chr);
 
 /* serial-isa.c */
 #define TYPE_ISA_SERIAL "isa-serial"
diff --git a/hw/char/serial.c b/hw/char/serial.c
index 9aec6c60d8..7a100db107 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -1025,6 +1025,19 @@ static const MemoryRegionOps serial_mm_ops[3] = {
 },
 };
 
+Chardev *serial_chr_nonnull(Chardev *chr)
+{
+static int serial_id;
+char *label;
+
+label = g_strdup_printf("discarding-serial%d", serial_id++);
+chr = qemu_chr_new(label, "null");
+assert(chr);
+g_free(label);
+
+return chr;
+}
+
 SerialState *serial_mm_init(MemoryRegion *address_space,
 hwaddr base, int it_shift,
 qemu_irq irq, int baudbase,
-- 
2.14.1

[Qemu-devel] [PATCH 4/7] hw/mips/malta: use serial_chr_nonnull()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/mips/mips_malta.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index af678f5784..8620e9c42c 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1035,11 +1035,7 @@ void mips_malta_init(MachineState *machine)
 
 /* Make sure the first 3 serial ports are associated with a device. */
 for(i = 0; i < 3; i++) {
-if (!serial_hds[i]) {
-char label[32];
-snprintf(label, sizeof(label), "serial%d", i);
-serial_hds[i] = qemu_chr_new(label, "null");
-}
+serial_hds[i] = serial_chr_nonnull(serial_hds[i]);
 }
 
 /* create CPU */
-- 
2.14.1

Re: [Qemu-devel] [RFC v2 0/8] monitor: allow per-monitor thread

[Peter Xu]

On Wed, Aug 30, 2017 at 11:13:11AM +0100, Daniel P. Berrange wrote:
> On Wed, Aug 30, 2017 at 09:06:20AM +0200, Markus Armbruster wrote:
> > "Daniel P. Berrange"  writes:
> > 
> > > On Wed, Aug 23, 2017 at 02:51:03PM +0800, Peter Xu wrote:
> > 
> > >> However, even with the series, it does not mean that per-monitor
> > >> threads will never hang.  One example is that we can still run "info
> > >> vcpus" in per-monitor threads during a paused postcopy (in that state,
> > >> page faults are never handled, and "info cpus" will never return since
> > >> it tries to sync every vcpus).  So to make sure it does not hang, we
> > >> not only need the per-monitor thread, the user should be careful as
> > >> well on how to use it.
> > >> 
> > >> For postcopy recovery, we may need dedicated monitor channel for
> > >> recovery.  In other words, a destination VM that supports postcopy
> > >> recovery would possibly need:
> > >> 
> > >>   -qmp MAIN_CHANNEL -qmp RECOVERY_CHANNEL
> > 
> > Where RECOVERY_CHANNEL isn't necessarily just for postcopy, but for any
> > "emergency" QMP access.  If you use it only for commands that cannot
> > hang (i.e. terminate in bounded time), then you'll always be able to get
> > commands accepted there in bounded time.
> > 
> > > I think this is a really horrible thing to expose to management 
> > > applications.
> > > They should not need to be aware of fact that QEMU is buggy and thus 
> > > requires
> > > that certain commands be run on different monitors to work around the bug.
> > 
> > These are (serious) design limitations, not bugs in the narrow sense of
> > the word.
> > 
> > However, I quite agree that the need for clients to know whether a
> > monitor command can hang is impractical for the general case.  What
> > might be practical is a QMP monitor mode that accepts only known
> > hang-free commands.  Hang-free could be introspectable.
> > 
> > In case you consider that ugly: it's best to explore the design space
> > first, and recoil from "ugly" second.
> 
> Actually you slightly mis-interpreted me there. I think it is ok for
> applications to have knowledge about whether a particular command
> may hang or not. Given that knowledge it should *not*, however, require
> that the application issue such commands on separate monitor channels.
> It is entirely possible to handle hang-free commands on the existing
> channel.
> 
> > > I'd much prefer to see the problem described handled transparently inside
> > > QEMU. One approach is have a dedicated thread in QEMU responsible for all
> > > monitor I/O. This thread should never actually execute monitor commands
> > > though, it would simply parse the command request and put data onto a 
> > > queue
> > > of pending commands, thus it could never hang. The command queue could be
> > > processed by the main thread, or by another thread that is interested.
> > > eg the migration thread could process any queued commands related to
> > > migration directly.
> > 
> > The monitor itself can't hang then, but the thread(s) dequeuing parsed
> > commands can.
> 
> If certain commands are hang-free then you can have a dedicated thread
> that only de-queues & processes the hang-free commands. The approach I
> outlined is exactly how libvirt deals with its own RPC dispatch. We have
> certain commands that are guaranteed to not hang, which are processed by
> a dedicated pool of threads. So even if all normal RPC commands have
> hung, you can still run a subset of hang-free RPC commands.
> 
> > 
> > To maintain commands' synchronous semantics, their replies need to be
> > sent in order, which of course reintroduces the hangs.
> 
> The requirement for such ordering is just an arbitrary restriction that
> QEMU currently imposes. It is reasonable to allow arbitrary ordering of
> responses (which is what libvirt does in its RPC layer). Admittedly at
> this stage though, we would likely require some "opt in" handshake when
> initializing QMP for the app to tell QEMU it can cope with out of order
> replies. It would require that each command request has a unique serial
> number, which is included in the associated reply, so apps can match
> them up. We used to have that but iirc it was then removed.
> 
> There's other ways to deal with this, such as the job starting idea you
> mention below.
> 
> The key point though is that I don't think creating multiple monitor
> servers is a desirable approach - it is just a hack to avoid dealing
> with the root cause problems. 

Yeah I kindly agree.  It's not the root problem, but AFAIU that's the
simplest way for now to solve the problem.  But I think I understand
the major problem here - an extra channel is an interface change, and
it affects users of monitors.  So I agree we'd better be patient on
choosing a good enough interface, looks like we have two:

- dedicated "hang-able" and "hang-free" channel, or,

- async command handling, then we will have one single dedicated
  command parser 

Re: [Qemu-devel] [PATCHv5 01/03] qemu-iothread: IOThread supports theGMainContext event loop

[wang.yong155]

>> From: Wang Yong 

>> 

>> IOThread uses AioContext event loop and does not run a GMainContext.

>> Therefore,chardev cannot work in IOThread,such as the chardev is

>> used for colo-compare packets reception.

>> 

>> This patch makes the IOThread run the GMainContext event loop,

>> chardev and IOThread can work together.

>> 

>> Signed-off-by: Wang Yong 

>> Signed-off-by: Wang Guang 

>> ---

>>  include/sysemu/iothread.h |  4 

>>  iothread.c| 45 +

>>  2 files changed, 49 insertions(+)

>> 

>> diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h

>> index e6da1a4..d2985b3 100644

>> --- a/include/sysemu/iothread.h

>> +++ b/include/sysemu/iothread.h

>> @@ -24,6 +24,9 @@ typedef struct {

>>  

>>  QemuThread thread

>>  AioContext *ctx

>> +GMainContext *worker_context

>> +GMainLoop *main_loop

>> +GOnce once

>>  QemuMutex init_done_lock

>>  QemuCond init_done_cond/* is thread initialization done? */

>>  bool stopping

>> @@ -41,5 +44,6 @@ typedef struct {

>>  char *iothread_get_id(IOThread *iothread)

>>  AioContext *iothread_get_aio_context(IOThread *iothread)

>>  void iothread_stop_all(void)

>> +GMainContext *iothread_get_g_main_context(IOThread *iothread)

>>  

>>  #endif /* IOTHREAD_H */

>> diff --git a/iothread.c b/iothread.c

>> index beeb870..44c8944 100644

>> --- a/iothread.c

>> +++ b/iothread.c

>> @@ -57,6 +57,23 @@ static void *iothread_run(void *opaque)

>>  

>>  while (!atomic_read(>stopping)) {

>>  aio_poll(iothread->ctx, true)

>> +

>> +if (atomic_read(>worker_context)) {

>> +GMainLoop *loop

>> +

>> +g_main_context_push_thread_default(iothread->worker_context)

>> +iothread->main_loop =

>> +g_main_loop_new(iothread->worker_context, TRUE)

>> +loop = iothread->main_loop

>> +

>> +g_main_loop_run(iothread->main_loop)

>> +iothread->main_loop = NULL

>> +g_main_loop_unref(loop)

>> +

>> +g_main_context_pop_thread_default(iothread->worker_context)

>> +g_main_context_unref(iothread->worker_context)

>> +iothread->worker_context = NULL

>> +}

>>  }

>>  

>>  rcu_unregister_thread()

>> @@ -73,6 +90,9 @@ static int iothread_stop(Object *object, void *opaque)

>>  }

>>  iothread->stopping = true

>>  aio_notify(iothread->ctx)

>> +if (atomic_read(>main_loop)) {

>> +g_main_loop_quit(iothread->main_loop)

>> +}

>>  qemu_thread_join(>thread)

>>  return 0

>>  }

>> @@ -125,6 +145,7 @@ static void iothread_complete(UserCreatable *obj, Error 
>> **errp)

>>  

>>  qemu_mutex_init(>init_done_lock)

>>  qemu_cond_init(>init_done_cond)

>> +iothread->once = (GOnce) G_ONCE_INIT

>

>In last review I suggested removing this type cast, otherwise looks good. Drop

>it and please add

>

>Reviewed-by: Fam Zheng 




Sorry, There's something wrong with our company's mail format.

Please ignore my last reply mail.




Hi Fam,

Here, iothread->once can't be initialized with G_ONCE_INIT directly, must use 
type cast.

if  remove type cast , we will get an error.




iothread.c: In function ‘iothread_complete’:

/usr/include/glib-2.0/glib/gthread.h:101:21: error: expected expression before 
‘{’ token

 #define G_ONCE_INIT { G_ONCE_STATUS_NOTCALLED, NULL }

 ^

iothread.c:149:22: note: in expansion of macro ‘G_ONCE_INIT’

 iothread->once = G_ONCE_INIT




Thanks

>

>>  

>>  /* This assumes we are called from a thread with useful CPU affinity 
>> for us

>>   * to inherit.

>> @@ -309,3 +330,27 @@ void iothread_stop_all(void)

>>  

>>  object_child_foreach(container, iothread_stop, NULL)

>>  }

>> +

>> +static gpointer iothread_g_main_context_init(gpointer opaque)

>> +{

>> +AioContext *ctx

>> +IOThread *iothread = opaque

>> +GSource *source

>> +

>> +iothread->worker_context = g_main_context_new()

>> +

>> +ctx = iothread_get_aio_context(iothread)

>> +source = aio_get_g_source(ctx)

>> +g_source_attach(source, iothread->worker_context)

>> +g_source_unref(source)

>> +

>> +aio_notify(iothread->ctx)

>> +return NULL

>> +}

>> +

>> +GMainContext *iothread_get_g_main_context(IOThread *iothread)

>> +{

>> +g_once(>once, iothread_g_main_context_init, iothread)

>> +

>> +return iothread->worker_context

>> +}

>> -- 

>> 1.8.3.1

>> 

>> 



原始邮件



发件人： 
收件人：王勇10170530
抄送人：    
 王广10165992 
 
日 期 ：2017年08月31日 10:18
主 题 ：Re: [PATCHv5 01/03] qemu-iothread: IOThread supports 

Re: [Qemu-devel] [PATCHv5 01/03] qemu-iothread: IOThread supports theGMainContext event loop

[wang.yong155]

>> From: Wang Yong >> >> IOThread uses AioContext 
>> event loop and does not run a GMainContext.>> Therefore,chardev cannot work 
>> in IOThread,such as the chardev is>> used for colo-compare packets 
>> reception.>> >> This patch makes the IOThread run the GMainContext event 
>> loop,>> chardev and IOThread can work together.>> >> Signed-off-by: Wang 
>> Yong >> Signed-off-by: Wang Guang 
>> >> --->>  include/sysemu/iothread.h |  4 >>  
>> iothread.c| 45 
>> +>>  2 files changed, 49 
>> insertions(+)>> >> diff --git a/include/sysemu/iothread.h 
>> b/include/sysemu/iothread.h>> index e6da1a4..d2985b3 100644>> --- 
>> a/include/sysemu/iothread.h>> +++ b/include/sysemu/iothread.h>> @@ -24,6 
>> +24,9 @@ typedef struct {>>  >>  QemuThread thread>>  AioContext 
>> *ctx>> +GMainContext *worker_context>> +GMainLoop *main_loop>> +
>> GOnce once>>  QemuMutex init_done_lock>>  QemuCond init_done_cond
>> /* is thread initialization done? */>>  bool stopping>> @@ -41,5 +44,6 
>> @@ typedef struct {>>  char *iothread_get_id(IOThread *iothread)>>  
>> AioContext *iothread_get_aio_context(IOThread *iothread)>>  void 
>> iothread_stop_all(void)>> +GMainContext 
>> *iothread_get_g_main_context(IOThread *iothread)>>  >>  #endif /* IOTHREAD_H 
>> */>> diff --git a/iothread.c b/iothread.c>> index beeb870..44c8944 100644>> 
>> --- a/iothread.c>> +++ b/iothread.c>> @@ -57,6 +57,23 @@ static void 
>> *iothread_run(void *opaque)>>  >>  while 
>> (!atomic_read(>stopping)) {>>  aio_poll(iothread->ctx, 
>> true)>> +>> +if (atomic_read(>worker_context)) {>> +   
>>  GMainLoop *loop>> +>> +
>> g_main_context_push_thread_default(iothread->worker_context)>> +
>> iothread->main_loop =>> +
>> g_main_loop_new(iothread->worker_context, TRUE)>> +loop = 
>> iothread->main_loop>> +>> +
>> g_main_loop_run(iothread->main_loop)>> +iothread->main_loop = 
>> NULL>> +g_main_loop_unref(loop)>> +>> +
>> g_main_context_pop_thread_default(iothread->worker_context)>> +
>> g_main_context_unref(iothread->worker_context)>> +
>> iothread->worker_context = NULL>> +}>>  }>>  >>  
>> rcu_unregister_thread()>> @@ -73,6 +90,9 @@ static int iothread_stop(Object 
>> *object, void *opaque)>>  }>>  iothread->stopping = true>>  
>> aio_notify(iothread->ctx)>> +if (atomic_read(>main_loop)) {>> 
>> +g_main_loop_quit(iothread->main_loop)>> +}>>  
>> qemu_thread_join(>thread)>>  return 0>>  }>> @@ -125,6 +145,7 
>> @@ static void iothread_complete(UserCreatable *obj, Error **errp)>>  >> 
>>  qemu_mutex_init(>init_done_lock)>>  
>> qemu_cond_init(>init_done_cond)>> +iothread->once = (GOnce) 
>> G_ONCE_INIT>>In last review I suggested removing this type cast, otherwise 
>> looks good. Drop>it and please add>>Reviewed-by: Fam Zheng >

Hi Fam,

Here, iothread->once can't be initialized with G_ONCE_INIT directly, must use 
type cast.

if  remove type cast , we will get an error.




iothread.c: In function ‘iothread_complete’:

/usr/include/glib-2.0/glib/gthread.h:101:21: error: expected expression before 
‘{’ token

 #define G_ONCE_INIT { G_ONCE_STATUS_NOTCALLED, NULL }

 ^

iothread.c:149:22: note: in expansion of macro ‘G_ONCE_INIT’

 iothread->once = G_ONCE_INIT




Thanks




>>  >>  /* This assumes we are called from a thread with useful CPU 
>> affinity for us>>   * to inherit.>> @@ -309,3 +330,27 @@ void 
>> iothread_stop_all(void)>>  >>  object_child_foreach(container, 
>> iothread_stop, NULL)>>  }>> +>> +static gpointer 
>> iothread_g_main_context_init(gpointer opaque)>> +{>> +AioContext *ctx>> 
>> +IOThread *iothread = opaque>> +GSource *source>> +>> +
>> iothread->worker_context = g_main_context_new()>> +>> +ctx = 
>> iothread_get_aio_context(iothread)>> +source = aio_get_g_source(ctx)>> + 
>>g_source_attach(source, iothread->worker_context)>> +
>> g_source_unref(source)>> +>> +aio_notify(iothread->ctx)>> +return 
>> NULL>> +}>> +>> +GMainContext *iothread_get_g_main_context(IOThread 
>> *iothread)>> +{>> +g_once(>once, iothread_g_main_context_init, 
>> iothread)>> +>> +return iothread->worker_context>> +}>> -- >> 1.8.3.1>> 
>> >> 



原始邮件



发件人： 
收件人：王勇10170530
抄送人：    
 王广10165992 
 
日 期 ：2017年08月31日 10:18
主 题 ：Re: [PATCHv5 01/03] qemu-iothread: IOThread supports theGMainContext event 
loop





On Tue, 08/29 15:22, Wang yong wrote:
> From: Wang Yong 
> 
> 

Re: [Qemu-devel] [PATCHv5 01/03] qemu-iothread: IOThread supports the GMainContext event loop

[Fam Zheng]

On Tue, 08/29 15:22, Wang yong wrote:
> From: Wang Yong 
> 
> IOThread uses AioContext event loop and does not run a GMainContext.
> Therefore,chardev cannot work in IOThread,such as the chardev is
> used for colo-compare packets reception.
> 
> This patch makes the IOThread run the GMainContext event loop,
> chardev and IOThread can work together.
> 
> Signed-off-by: Wang Yong 
> Signed-off-by: Wang Guang 
> ---
>  include/sysemu/iothread.h |  4 
>  iothread.c| 45 +
>  2 files changed, 49 insertions(+)
> 
> diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
> index e6da1a4..d2985b3 100644
> --- a/include/sysemu/iothread.h
> +++ b/include/sysemu/iothread.h
> @@ -24,6 +24,9 @@ typedef struct {
>  
>  QemuThread thread;
>  AioContext *ctx;
> +GMainContext *worker_context;
> +GMainLoop *main_loop;
> +GOnce once;
>  QemuMutex init_done_lock;
>  QemuCond init_done_cond;/* is thread initialization done? */
>  bool stopping;
> @@ -41,5 +44,6 @@ typedef struct {
>  char *iothread_get_id(IOThread *iothread);
>  AioContext *iothread_get_aio_context(IOThread *iothread);
>  void iothread_stop_all(void);
> +GMainContext *iothread_get_g_main_context(IOThread *iothread);
>  
>  #endif /* IOTHREAD_H */
> diff --git a/iothread.c b/iothread.c
> index beeb870..44c8944 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -57,6 +57,23 @@ static void *iothread_run(void *opaque)
>  
>  while (!atomic_read(>stopping)) {
>  aio_poll(iothread->ctx, true);
> +
> +if (atomic_read(>worker_context)) {
> +GMainLoop *loop;
> +
> +g_main_context_push_thread_default(iothread->worker_context);
> +iothread->main_loop =
> +g_main_loop_new(iothread->worker_context, TRUE);
> +loop = iothread->main_loop;
> +
> +g_main_loop_run(iothread->main_loop);
> +iothread->main_loop = NULL;
> +g_main_loop_unref(loop);
> +
> +g_main_context_pop_thread_default(iothread->worker_context);
> +g_main_context_unref(iothread->worker_context);
> +iothread->worker_context = NULL;
> +}
>  }
>  
>  rcu_unregister_thread();
> @@ -73,6 +90,9 @@ static int iothread_stop(Object *object, void *opaque)
>  }
>  iothread->stopping = true;
>  aio_notify(iothread->ctx);
> +if (atomic_read(>main_loop)) {
> +g_main_loop_quit(iothread->main_loop);
> +}
>  qemu_thread_join(>thread);
>  return 0;
>  }
> @@ -125,6 +145,7 @@ static void iothread_complete(UserCreatable *obj, Error 
> **errp)
>  
>  qemu_mutex_init(>init_done_lock);
>  qemu_cond_init(>init_done_cond);
> +iothread->once = (GOnce) G_ONCE_INIT;

In last review I suggested removing this type cast, otherwise looks good. Drop
it and please add

Reviewed-by: Fam Zheng 

>  
>  /* This assumes we are called from a thread with useful CPU affinity for 
> us
>   * to inherit.
> @@ -309,3 +330,27 @@ void iothread_stop_all(void)
>  
>  object_child_foreach(container, iothread_stop, NULL);
>  }
> +
> +static gpointer iothread_g_main_context_init(gpointer opaque)
> +{
> +AioContext *ctx;
> +IOThread *iothread = opaque;
> +GSource *source;
> +
> +iothread->worker_context = g_main_context_new();
> +
> +ctx = iothread_get_aio_context(iothread);
> +source = aio_get_g_source(ctx);
> +g_source_attach(source, iothread->worker_context);
> +g_source_unref(source);
> +
> +aio_notify(iothread->ctx);
> +return NULL;
> +}
> +
> +GMainContext *iothread_get_g_main_context(IOThread *iothread)
> +{
> +g_once(>once, iothread_g_main_context_init, iothread);
> +
> +return iothread->worker_context;
> +}
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 3/3] Backup Tool: Test for Incremental Backup

[Fam Zheng]

On Thu, 08/31 00:45, Ishani Chugh wrote:
> This patch is the test for incremental backup implementation in Backup tool.
> The test employs two basic subtests:
> 1) Backing up an empty guest and comparing it with base image.
> 2) Writing a pattern to the guest, creating backup, writing
>a pattern again, creating backup and comparing with base image.
> 
> Signed-off-by: Ishani Chugh 
> ---
>  tests/qemu-iotests/193 | 86 
> ++
>  tests/qemu-iotests/193.out | 34 ++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 121 insertions(+)
>  create mode 100755 tests/qemu-iotests/193
>  create mode 100644 tests/qemu-iotests/193.out
> 
> diff --git a/tests/qemu-iotests/193 b/tests/qemu-iotests/193
> new file mode 100755
> index 000..500e5df
> --- /dev/null
> +++ b/tests/qemu-iotests/193
> @@ -0,0 +1,86 @@
> +#!/bin/bash
> +#
> +# Test Incremental backup functionality of qemu-backup tool
> +#
> +# Copyright (C) 2009 Red Hat, Inc.

Year is off, probably the copyright holder too?

> +#
> +# 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=chugh.ish...@research.iiit.ac.in
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +status=1 # failure is the default!

s/\t/ /

> +
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +. ./common.qemu
> +
> +_supported_fmt generic
> +_supported_proto generic
> +_supported_os Linux
> +
> +
> +CONFIG_FILE=$TEST_DIR/backup-config
> +SOCKET=unix:$TEST_DIR/backup_socket
> +size=128M
> +
> +_make_test_img $size
> +export QEMU_BACKUP_CONFIG=$CONFIG_FILE
> +qemu_comm_method="monitor"
> +echo
> +_launch_qemu -drive if=virtio,file=$TEST_IMG -qmp $SOCKET,server,nowait
> +$PYTHON ../../contrib/backup/qemu-backup.py guest add --guest adad --qmp 
> $SOCKET
> +$PYTHON ../../contrib/backup/qemu-backup.py drive add --id virtio0 --guest 
> adad --target $TEST_DIR/virtio0
> +echo
> +echo "== Creating backup =="
> +$PYTHON ../../contrib/backup/qemu-backup.py backup --inc --guest adad
> +_send_qemu_cmd $QEMU_HANDLE 'quit' ''
> +wait=1 _cleanup_qemu
> +echo
> +echo "== Comparing images =="
> +$QEMU_IMG compare $TEST_DIR/virtio0_inc_0 $TEST_IMG
> +
> +rm $TEST_DIR/virtio0_inc_0
> +
> +_launch_qemu -drive if=virtio,id=virtio0,file=$TEST_IMG -qmp 
> $SOCKET,server,nowait
> +echo
> +echo "== Writing Pattern =="
> +_send_qemu_cmd $QEMU_HANDLE 'qemu-io virtio0 "write -P 0x22 0 1M"' "(qemu)" 
> | _filter_qemu_io
> +echo
> +
> +echo "== Creating backup =="
> +$PYTHON ../../contrib/backup/qemu-backup.py backup --inc --guest adad
> +
> +echo "== Writing Pattern =="
> +_send_qemu_cmd $QEMU_HANDLE 'qemu-io virtio0 "write -P 0x22 0 1M"' "(qemu)" 
> | _filter_qemu_io
> +echo
> +$PYTHON ../../contrib/backup/qemu-backup.py backup --inc --guest adad
> +_send_qemu_cmd $QEMU_HANDLE 'quit' ''
> +wait=1 _cleanup_qemu
> +echo
> +echo "== Comparing images =="
> +$QEMU_IMG compare $TEST_DIR/virtio0_inc_1 $TEST_IMG
> +rm $TEST_DIR/virtio0_inc_0
> +rm $TEST_DIR/virtio0_inc_1
> +rm $CONFIG_FILE
> +
> +echo "*** done"
> +status=0
> \ No newline at end of file
> diff --git a/tests/qemu-iotests/193.out b/tests/qemu-iotests/193.out
> new file mode 100644
> index 000..3a836c2
> --- /dev/null
> +++ b/tests/qemu-iotests/193.out
> @@ -0,0 +1,34 @@
> +QA output created by 193
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> +
> +Successfully Added Guest
> +Successfully Added Drive
> +
> +== Creating backup ==
> +QEMU X.Y.Z monitor - type 'help' for more information
> +(qemu) Formatting 'TEST_DIR/virtio0_inc_0', fmt=qcow2 size=134217728 
> cluster_size=65536 lazy_refcounts=off refcount_bits=16
> +quit
> +
> +== Comparing images ==
> +Images are identical.
> +
> +== Writing Pattern ==
> +QEMU X.Y.Z monitor - type 'help' for more information
> +(qemu) qemu-io virtio0 "write -P 0x22 0 1M"
> +
> +== Creating backup ==
> +Initial Backup does not exist
> +== Writing Pattern ==
> +wrote 1048576/1048576 bytes at offset 0
> +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +(qemu) Formatting 'TEST_DIR/virtio0_inc_0', fmt=qcow2 size=134217728 
> cluster_size=65536 lazy_refcounts=off refcount_bits=16
> +
> +Formatting 
> '/home/ishani/Desktop/opw/qemu/tests/qemu-iotests/scratch/virtio0_inc_1', 
> fmt=qcow2 

Re: [Qemu-devel] [PATCH 1/3] Backup Tool: Manpage for Incremental Backup

[Fam Zheng]

On Thu, 08/31 00:45, Ishani Chugh wrote:
> Adds command description to perform incremental backup and
> a full example for the same.
> 
> Signed-off-by: Ishani Chugh 
> ---
>  contrib/backup/qemu-backup.texi | 18 ++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/contrib/backup/qemu-backup.texi b/contrib/backup/qemu-backup.texi
> index 7ad266c..8e5cb86 100644
> --- a/contrib/backup/qemu-backup.texi
> +++ b/contrib/backup/qemu-backup.texi
> @@ -58,6 +58,7 @@ qemu-backup command [command options].
>  @item qemu-backup restore --guest guestname
>  @item qemu-backup guest remove --guest guestname
>  @item qemu-backup drive remove --guest guestname --id driveid
> +@item qemu-backup backup --inc --guest guestname
>  @end itemize
>  @node  Command Parameters
>  @chapter  Command Parameters
> @@ -67,6 +68,7 @@ qemu-backup command [command options].
>  @item --id: id of guest or drive.
>  @item --qmp: Path of qmp socket.
>  @item --target: Destination path on which you want your backup to be made.
> +@item --inc: incremental backup (Optional).
>  @end itemize
>  
>  @node  Command Descriptions
> @@ -119,6 +121,11 @@ This command helps remove a drive which is set for 
> backup in configuration of gi
>  
>  example: drive remove --guest=fedora --id=root
>  
> +@item qemu-backup backup --inc --guest guestname
> +This command is used for making incremental backup.
> +
> +example: qemu-backup backup --inc --guest fedora

Could you document the output file name generation?

Fam

> +
>  @item A full backup can be performed by following the given steps:
>  
>  Perform a full backup of 'vm001', which has one drive:
> @@ -129,6 +136,17 @@ qemu-backup add --id drive0 --guest vm001 --target 
> /backups/vm001-drive0.img
>  
>  qemu-backup backup --guest vm001
>  
> +@item An incremental backup can be performed by following the given steps:
> +
> +Perform an incremental backup of 'vm001', which has one drive:
> +
> +qemu-backup guest add --guest vm001 --qmp /path/to/vm001.sock
> +
> +qemu-backup add --id drive0 --guest vm001 --target /backups/vm001-drive0.img
> +
> +qemu-backup backup --inc --guest vm001
> +
> +qemu-backup backup --inc --guest vm001
>  
>  @end itemize
>  
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 2/3] Backup Tool: Support for Incremental Backup

[Fam Zheng]

On Thu, 08/31 00:45, Ishani Chugh wrote:
> Adds incremental backup functionality.
> 
> Signed-off-by: Ishani Chugh 
> ---
>  contrib/backup/qemu-backup.py | 101 
> +-
>  1 file changed, 99 insertions(+), 2 deletions(-)
> 
> diff --git a/contrib/backup/qemu-backup.py b/contrib/backup/qemu-backup.py
> index 248ca9f..7a3077a 100644
> --- a/contrib/backup/qemu-backup.py
> +++ b/contrib/backup/qemu-backup.py
> @@ -24,11 +24,13 @@ from __future__ import print_function
>  from argparse import ArgumentParser
>  import os
>  import errno
> +from string import Template
>  from socket import error as socket_error
>  try:
>  import configparser
>  except ImportError:
>  import ConfigParser as configparser
> +from configparser import NoOptionError
>  import sys
>  sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..',
>   'scripts', 'qmp'))
> @@ -41,7 +43,6 @@ class BackupTool(object):
>   '/.config/qemu/qemu-backup-config'):
>  if "QEMU_BACKUP_CONFIG" in os.environ:
>  self.config_file = os.environ["QEMU_BACKUP_CONFIG"]
> -
>  else:
>  self.config_file = config_file
>  try:
> @@ -129,6 +130,97 @@ class BackupTool(object):
>  drive_list.remove(event['data']['device'])
>  print("Backup Complete")
>  
> +def _inc_backup(self, guest_name):
> +"""
> +Performs Incremental backup
> +"""
> +if guest_name not in self.config.sections():
> +print ("Cannot find specified guest", file=sys.stderr)

s/print (/print(/

> +exit(1)
> +
> +self.verify_guest_running(guest_name)
> +connection = QEMUMonitorProtocol(
> + self.get_socket_address(
> + self.config[guest_name]['qmp']))
> +connection.connect()
> +backup_cmd = {"execute": "transaction",
> +  "arguments": {"actions": [], "properties":
> +{"completion-mode": "grouped"}}}
> +bitmap_cmd = {"execute": "transaction", "arguments": {"actions": []}}
> +for key in self.config[guest_name]:

>From here on the indentation level is launched straight into outer space. 
>Please
either extract code blocks into functions, or at least try to rearrange it like:

> +if key.startswith("drive_"):

   if not key.startswith("drive_"):
   continue

   drive = ...
   ...
> +drive = key[len('drive_'):]
> +target = self.config.get(guest_name, key).rsplit('/', 1)[0]
> +inc_backup_pattern = Template('${drive}_inc_${N}')
> +bitmap = 'qemu_backup_'+guest_name

Whitespaces missing around +.

> +try:
> +query_block_cmd = {'execute': 'query-block'}
> +returned_json = connection.cmd_obj(query_block_cmd)
> +device_present = False
> +for device in returned_json['return']:
> +if device['device'] == drive:

   if device['device'] != drive:
   continue
   device_present = True
   ...
> +device_present = True
> +bitmap_present = False
> +for bitmaps in device['dirty-bitmaps']:
> +if bitmap == bitmaps['name']:

   if bitmap != bitmaps['name']:
   continue
   bitmap_present = True
   ...
> +bitmap_present = True
> +if os.path.isfile(self.config.get(
> +  guest_name,
> +  'inc_'+drive)) is 
> False:
> +print("Initial Backup does not 
> exist")
> +bitmap_remove = {"execute":
> + "block-dirty" +
> + "-bitmap-remove",

Please fix the indentation level and join the string: 
"block-dirty-bitmap-remove".

Even if you cannot control the indentation level, long line is still better than
cutting it into halves. It makes the code hard to read and unfriendly to grep.

> + "arguments":
> + {"node": drive,
> +  "name":
> +

Re: [Qemu-devel] [PATCH v7 07/11] qemu.py: include debug information on launch error

[Fam Zheng]

On Wed, 08/30 11:55, Cleber Rosa wrote:
> 
> 
> On 08/18/2017 01:05 PM, Amador Pahim wrote:
> > When launching a VM, if an exception happens and the VM is not
> > initiated, it might be useful to see the qemu command line and
> > the qemu command output.
> > 
> > This patch creates that message. Notice that self._iolog needs to be
> > cleaned up in the beginning of the launch() to make sure we will not
> > expose the qemu log from a previous launch if the current one fails.
> > 
> > Signed-off-by: Amador Pahim 
> > ---
> >  scripts/qemu.py | 8 
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/scripts/qemu.py b/scripts/qemu.py
> > index 0bcec4b3b1..29fd2469f9 100644
> > --- a/scripts/qemu.py
> > +++ b/scripts/qemu.py
> > @@ -147,6 +147,7 @@ class QEMUMachine(object):
> >  
> >  def launch(self):
> >  '''Launch the VM and establish a QMP connection'''
> > +self._iolog = None
> >  self._qemu_full_args = None
> >  devnull = open(os.path.devnull, 'rb')
> >  qemulog = open(self._qemu_log_path, 'wb')
> > @@ -162,6 +163,13 @@ class QEMUMachine(object):
> >  self._post_launch()
> >  except:
> >  self.shutdown()
> > +
> > +LOG.debug('Error launching VM')
> > +if self._qemu_full_args:
> > +LOG.debug('Command: %r', ' '.join(self._qemu_full_args))
> > +if self._iolog:
> > +LOG.debug('Output: %r', self._iolog)
> > +
> 
> Based on Fam's comment about the signal message being a warning (worth
> showing by default), I also think this deserves more than a "debug"
> classification.

Yes, LOG.error, please.

Fam

Re: [Qemu-devel] [PATCH] block: Cleanup BMDS in bdrv_close_all

[Fam Zheng]

On Wed, 08/30 16:04, Juan Quintela wrote:
> Fam Zheng  wrote:
> > On Wed, 08/30 13:49, Juan Quintela wrote:
> >> Fam Zheng  wrote:
> >> > This fixes the assertion due to op blockers added by BMDS:
> >> >
> >> > block.c:3248: bdrv_delete: Assertion `bdrv_op_blocker_is_empty(bs)' 
> >> > failed.
> >> >
> >> > Reproducer: simply start block migration and quit QEMU before it ends.
> >> >
> >> > Cc: qemu-sta...@nongnu.org
> >> > Signed-off-by: Fam Zheng 
> >> 
> >> No need for one stub, see later.
> >> 
> >> 
> >> > ---
> >> >  block.c | 2 ++
> >> >  migration/block.c   | 2 +-
> >> >  migration/block.h   | 1 +
> >> >  stubs/Makefile.objs | 1 +
> >> >  stubs/block-migration.c | 6 ++
> >> >  5 files changed, 11 insertions(+), 1 deletion(-)
> >> >  create mode 100644 stubs/block-migration.c
> >> >
> >> > diff --git a/block.c b/block.c
> >> > index 3308814bba..508a57274d 100644
> >> > --- a/block.c
> >> > +++ b/block.c
> >> > @@ -43,6 +43,7 @@
> >> >  #include "qemu/cutils.h"
> >> >  #include "qemu/id.h"
> >> >  #include "qapi/util.h"
> >> > +#include "migration/block.h"
> >> 
> >> this should be misc.h
> >> 
> >> >  
> >> >  #ifdef CONFIG_BSD
> >> >  #include 
> >> > @@ -3111,6 +3112,7 @@ static void bdrv_close(BlockDriverState *bs)
> >> >  
> >> >  void bdrv_close_all(void)
> >> >  {
> >> > +block_migration_cleanup_bmds();
> >> >  block_job_cancel_sync_all();
> >> >  nbd_export_close_all();
> >> >  
> >> 
> >> > diff --git a/migration/block.h b/migration/block.h
> >> > index 22ebe94259..8bae1cf55a 100644
> >> > --- a/migration/block.h
> >> > +++ b/migration/block.h
> >> > @@ -42,4 +42,5 @@ static inline uint64_t blk_mig_bytes_total(void)
> >> >  #endif /* CONFIG_LIVE_BLOCK_MIGRATION */
> >> >  
> >> >  void migrate_set_block_enabled(bool value, Error **errp);
> >> > +void block_migration_cleanup_bmds(void);
> >> >  #endif /* MIGRATION_BLOCK_H */
> >> > diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> >> > index e69c217aff..7540913767 100644
> >> > --- a/stubs/Makefile.objs
> >> > +++ b/stubs/Makefile.objs
> >> > @@ -19,6 +19,7 @@ stub-obj-y += is-daemonized.o
> >> >  stub-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
> >> >  stub-obj-y += machine-init-done.o
> >> >  stub-obj-y += migr-blocker.o
> >> > +stub-obj-y += block-migration.o
> >> >  stub-obj-y += change-state-handler.o
> >> >  stub-obj-y += monitor.o
> >> >  stub-obj-y += notify-event.o
> >> > diff --git a/stubs/block-migration.c b/stubs/block-migration.c
> >> > new file mode 100644
> >> > index 00..855f15c757
> >> > --- /dev/null
> >> > +++ b/stubs/block-migration.c
> >> > @@ -0,0 +1,6 @@
> >> > +#include "qemu/osdep.h"
> >> > +#include "migration/block.h"
> >> > +
> >> > +void block_migration_cleanup_bmds(void)
> >> > +{
> >> > +}
> >> 
> >> You can add this inside include/migration/misc.h
> >> 
> >> #ifdef CONFIG_LIVE_BLOCK_MIGRATION
> >> void blk_mig_init(void);
> >> #else
> >> static inline void blk_mig_init(void) {}
> >> 
> >> // And then you add the stub here?
> >
> > This doesn't work.  The function is not stubbed for 
> > !CONFIG_LIVE_BLOCK_MIGRATION
> > configs, but for tools that don't link to common-obj-y. For example with 
> > your
> > proposed change, I get:
> >
> >   LINKqemu-nbd
> > block.o: In function `bdrv_close_all':
> > /home/fam/work/qemu/block.c:3115: undefined reference to
> > `block_migration_cleanup_bmds'
> > collect2: error: ld returned 1 exit status
> > make: *** [/home/fam/work/qemu/rules.mak:121: qemu-nbd] Error 1
> > make: Leaving directory '/home/fam/work/q/build'
> 
> 
> This works for me, for both CONFIG_LIVE_BLOCK_MIGRATION enabled and not.
> For qemu-system-x86_64 and qemu-nbd.  Could you test?

I get the same error:

  LINKqemu-nbd
block.o: In function `bdrv_close_all':
/home/fam/work/qemu/block.c:3115: undefined reference to 
`block_migration_cleanup_bmds'
collect2: error: ld returned 1 exit status
make: *** [/home/fam/work/qemu/rules.mak:121: qemu-nbd] Error 1
make: Leaving directory '/home/fam/work/q/build'

(also applies to qemu-img etc.)

Fam

---

$ cat config.status 
#!/bin/sh
# Generated by configure.
# Run this file to recreate the current configuration.
# Compiler output produced by configure, useful for debugging
# configure, is in config.log if it exists.
exec '/home/fam/work/qemu/configure' 
'--prefix=/home/fam/work/q/install/bdrv_close_all-bmds' '--enable-debug' 
'--extra-cflags=-Wno-error=format-truncation' '--target-list=x86_64-softmmu' 
"$@"

$ grep CONFIG_LIVE_BLOCK_MIGRATION config-host.h
#define CONFIG_LIVE_BLOCK_MIGRATION 1

$ make qemu-nbd V=1
(cd /home/fam/work/qemu; printf '#define QEMU_PKGVERSION '; if test -n ""; then 
printf '""\n'; else if test -d .git; then printf '" ('; git describe --match 
'v*' 2>/dev/null | tr -d '\n'; if ! git diff-index --quiet HEAD &>/dev/null; 
then printf -- '-dirty'; fi; printf ')"\n'; else printf '""\n'; fi; fi) > 
qemu-version.h.tmp
if ! cmp -s 

Re: [Qemu-devel] [PATCH v3 0/3] QEMU Backup Tool

[Fam Zheng]

On Wed, 08/30 22:44, Ishani Chugh wrote:
> This patch series is intended to introduce QEMU Backup tool.
> qemu-backup will be a command-line tool for performing full and
> incremental disk backups on running VMs. It is intended as a
> reference implementation for management stack and backup developers
> to see QEMU's backup features in action.
> This patch series contains three patches,
>1) QEMU Backup command line tool.
>2) Test for full backup.
>3) Manpage for the tool.
> v3:
> * Added versioning in config file
> * Replace location by qemu-img convert in restore
> * Removed incremental backup documentation from manpage

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PATCH] qcow2: allocate cluster_cache/cluster_data on demand

[Alexey Kardashevskiy]

On 31/08/17 03:20, Stefan Hajnoczi wrote:
> On Tue, Aug 22, 2017 at 02:56:00PM +1000, Alexey Kardashevskiy wrote:
>> On 19/08/17 12:46, Alexey Kardashevskiy wrote:
>>> On 19/08/17 01:18, Eric Blake wrote:
 On 08/18/2017 08:31 AM, Stefan Hajnoczi wrote:
> Most qcow2 files are uncompressed so it is wasteful to allocate (32 + 1)
> * cluster_size + 512 bytes upfront.  Allocate s->cluster_cache and
> s->cluster_data when the first read operation is performance on a
> compressed cluster.
>
> The buffers are freed in .bdrv_close().  .bdrv_open() no longer has any
> code paths that can allocate these buffers, so remove the free functions
> in the error code path.
>
> Reported-by: Alexey Kardashevskiy 
> Cc: Kevin Wolf 
> Signed-off-by: Stefan Hajnoczi 
> ---
> Alexey: Does this improve your memory profiling results?

 Is this a regression from earlier versions? 
>>>
>>> Hm, I have not thought about this.
>>>
>>> So. I did bisect and this started happening from
>>> 9a4c0e220d8a4f82b5665d0ee95ef94d8e1509d5
>>> "hw/virtio-pci: fix virtio behaviour"
>>>
>>> Before that, the very same command line would take less than 1GB of
>>> resident memory. That thing basically enforces virtio-1.0 for QEMU <=2.6
>>> which means that upstream with "-machine pseries-2.6" works fine (less than
>>> 1GB), "-machine pseries-2.7" does not (close to 7GB, sometime even 9GB).
>>>
>>> Then I tried bisecting again, with
>>> "scsi=off,disable-modern=off,disable-legacy=on" on my 150 virtio-block
>>> devices, started from
>>> e266d421490e0 "virtio-pci: add flags to enable/disable legacy/modern" (it
>>> added the disable-modern switch) which uses 2GB of memory.
>>>
>>> I ended up with ada434cd0b44 "virtio-pci: implement cfg capability".
>>>
>>> Then I removed proxy->modern_as on v2.10.0-rc3 (see below) and got 1.5GB of
>>> used memory (yay!)
>>>
>>> I do not really know how to reinterpret all of this, do you?
>>
>>
>> Anyone, ping? Should I move the conversation to the original thread? Any
>> hacks to try with libc?
> 
> I suggest a new top-level thread with Michael Tsirkin CCed.


I am continuing in the original "Memory use with >100 virtio devices"
thread and the problem is more generic than virtio, it is just easier to
reproduce it with virtio, that's all.



-- 
Alexey

Re: [Qemu-devel] [PATCH v2 7/9] AHCI: Rework IRQ constants

[John Snow]

On 08/29/2017 11:54 PM, Philippe Mathieu-Daudé wrote:
> On 08/29/2017 05:49 PM, John Snow wrote:
>> Create a new enum so that we can name the IRQ bits, which will make
>> debugging
>> them a little nicer if we can print them out. Not handled in this
>> patch, but
>> this will make it possible to get a nice debug printf detailing
>> exactly which
>> status bits are set, as it can be multiple at any given time.
>>
>> As a consequence of this patch, it is no longer possible to set
>> multiple IRQ
>> codes at once, but nothing was utilizing this ability anyway.
>>
>> Signed-off-by: John Snow 
>> ---
>>   hw/ide/ahci.c  | 49
>> ++---
>>   hw/ide/ahci_internal.h | 44
>> +++-
>>   hw/ide/trace-events|  2 +-
>>   3 files changed, 74 insertions(+), 21 deletions(-)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index c60a000..a0a4dd6 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -56,6 +56,27 @@ static bool ahci_map_fis_address(AHCIDevice *ad);
>>   static void ahci_unmap_clb_address(AHCIDevice *ad);
>>   static void ahci_unmap_fis_address(AHCIDevice *ad);
>>   +static const char *AHCIPortIRQ_lookup[AHCI_PORT_IRQ__END] = {
>> +[AHCI_PORT_IRQ_BIT_DHRS] = "DHRS",
>> +[AHCI_PORT_IRQ_BIT_PSS]  = "PSS",
>> +[AHCI_PORT_IRQ_BIT_DSS]  = "DSS",
>> +[AHCI_PORT_IRQ_BIT_SDBS] = "SDBS",
>> +[AHCI_PORT_IRQ_BIT_UFS]  = "UFS",
>> +[AHCI_PORT_IRQ_BIT_DPS]  = "DPS",
>> +[AHCI_PORT_IRQ_BIT_PCS]  = "PCS",
>> +[AHCI_PORT_IRQ_BIT_DMPS] = "DMPS",
>> +[8 ... 21]   = "RESERVED",
>> +[AHCI_PORT_IRQ_BIT_PRCS] = "PRCS",
>> +[AHCI_PORT_IRQ_BIT_IPMS] = "IPMS",
>> +[AHCI_PORT_IRQ_BIT_OFS]  = "OFS",
>> +[25] = "RESERVED",
>> +[AHCI_PORT_IRQ_BIT_INFS] = "INFS",
>> +[AHCI_PORT_IRQ_BIT_IFS]  = "IFS",
>> +[AHCI_PORT_IRQ_BIT_HBDS] = "HBDS",
>> +[AHCI_PORT_IRQ_BIT_HBFS] = "HBFS",
>> +[AHCI_PORT_IRQ_BIT_TFES] = "TFES",
>> +[AHCI_PORT_IRQ_BIT_CPDS] = "CPDS"
>> +};
>> static uint32_t  ahci_port_read(AHCIState *s, int port, int offset)
>>   {
>> @@ -170,12 +191,18 @@ static void ahci_check_irq(AHCIState *s)
>>   }
>> static void ahci_trigger_irq(AHCIState *s, AHCIDevice *d,
>> - int irq_type)
>> + enum AHCIPortIRQ irqbit)
>>   {
>> -DPRINTF(d->port_no, "trigger irq %#x -> %x\n",
>> -irq_type, d->port_regs.irq_mask & irq_type);
>> +g_assert(irqbit >= 0 && irqbit < 32);
> 
> I still think this assert is superfluous, anyway (and having hard time
> reading C99 statement before declarations - I need to grow):
> 
> Reviewed-by: Philippe Mathieu-Daudé 
> 

Left in because of my distrust of compilers as explained in my reply to
#05. We'll get to the bottom of it ;)

Thank you for the reviews.

--js

Re: [Qemu-devel] [PATCH v2 5/9] IDE: replace DEBUG_AIO with trace events

[John Snow]

CCing Laszlo Ersek literally just for laughs, as he is the most
entertaining language lawyer I know of.

Laszlo, please feel free to ignore this if you don't care :P

On 08/29/2017 11:14 PM, Philippe Mathieu-Daudé wrote:
> Hi John,
> 
> On 08/29/2017 05:49 PM, John Snow wrote:
>> Signed-off-by: John Snow 
>> ---
>>   hw/ide/atapi.c|  5 +
>>   hw/ide/core.c | 17 ++---
>>   hw/ide/trace-events   |  3 +++
>>   include/hw/ide/internal.h |  6 --
>>   4 files changed, 18 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
>> index 37fa699..b8fc51e 100644
>> --- a/hw/ide/atapi.c
>> +++ b/hw/ide/atapi.c
>> @@ -416,10 +416,7 @@ static void ide_atapi_cmd_read_dma_cb(void
>> *opaque, int ret)
>>   s->io_buffer_size = n * 2048;
>>   data_offset = 0;
>>   }
>> -#ifdef DEBUG_AIO
>> -printf("aio_read_cd: lba=%u n=%d\n", s->lba, n);
>> -#endif
>> -
>> +trace_ide_atapi_cmd_read_dma_cb_aio(s, s->lba, n);
>>   s->bus->dma->iov.iov_base = (void *)(s->io_buffer + data_offset);
>>   s->bus->dma->iov.iov_len = n * ATAPI_SECTOR_SIZE;
>>   qemu_iovec_init_external(>bus->dma->qiov, >bus->dma->iov, 1);
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 82a19b1..a1c90e9 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -58,6 +58,13 @@ static const int smart_attributes[][12] = {
>>   { 190,  0x03, 0x00, 0x45, 0x45, 0x1f, 0x00, 0x1f, 0x1f, 0x00,
>> 0x00, 0x32},
>>   };
>>   +const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT] = {
>> +[IDE_DMA_READ] = "DMA READ",
>> +[IDE_DMA_WRITE] = "DMA WRITE",
>> +[IDE_DMA_TRIM] = "DMA TRIM",
>> +[IDE_DMA_ATAPI] = "DMA ATAPI"
>> +};
>> +
>>   static void ide_dummy_transfer_stop(IDEState *s);
>> static void padstr(char *str, const char *src, int len)
>> @@ -860,10 +867,8 @@ static void ide_dma_cb(void *opaque, int ret)
>>   goto eot;
>>   }
>>   -#ifdef DEBUG_AIO
>> -printf("ide_dma_cb: sector_num=%" PRId64 " n=%d, cmd_cmd=%d\n",
>> -   sector_num, n, s->dma_cmd);
>> -#endif
>> +trace_ide_dma_cb(s, sector_num, n,
>> + IDE_DMA_CMD_lookup[s->dma_cmd]);
>> if ((s->dma_cmd == IDE_DMA_READ || s->dma_cmd ==
>> IDE_DMA_WRITE) &&
>>   !ide_sect_range_ok(s, sector_num, n)) {
>> @@ -2391,9 +2396,7 @@ void ide_bus_reset(IDEBus *bus)
>> /* pending async DMA */
>>   if (bus->dma->aiocb) {
>> -#ifdef DEBUG_AIO
>> -printf("aio_cancel\n");
>> -#endif
>> +trace_ide_bus_reset_aio();
>>   blk_aio_cancel(bus->dma->aiocb);
>>   bus->dma->aiocb = NULL;
>>   }
>> diff --git a/hw/ide/trace-events b/hw/ide/trace-events
>> index 8c79a6c..cc8949c 100644
>> --- a/hw/ide/trace-events
>> +++ b/hw/ide/trace-events
>> @@ -18,6 +18,8 @@ ide_cancel_dma_sync_remaining(void) "draining all
>> remaining requests"
>>   ide_sector_read(int64_t sector_num, int nsectors) "sector=%"PRId64"
>> nsectors=%d"
>>   ide_sector_write(int64_t sector_num, int nsectors) "sector=%"PRId64"
>> nsectors=%d"
>>   ide_reset(void *s) "IDEstate %p"
>> +ide_bus_reset_aio(void) "aio_cancel"
>> +ide_dma_cb(void *s, int64_t sector_num, int n, const char *dma)
>> "IDEState %p; sector_num=%"PRId64" n=%d cmd=%s"
>> # BMDMA HBAs:
>>   @@ -51,5 +53,6 @@ ide_atapi_cmd_reply_end_new(void *s, int status)
>> "IDEState: %p; new transfer sta
>>   ide_atapi_cmd_check_status(void *s) "IDEState: %p"
>>   ide_atapi_cmd_read(void *s, const char *method, int lba, int
>> nb_sectors) "IDEState: %p; read %s: LBA=%d nb_sectors=%d"
>>   ide_atapi_cmd(void *s, uint8_t cmd) "IDEState: %p; cmd: 0x%02x"
>> +ide_atapi_cmd_read_dma_cb_aio(void *s, int lba, int n) "IDEState: %p;
>> aio read: lba=%d n=%d"
>>   # Warning: Verbose
>>   ide_atapi_cmd_packet(void *s, uint16_t limit, const char *packet)
>> "IDEState: %p; limit=0x%x packet: %s"
>> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
>> index 74efe8a..db9fde0 100644
>> --- a/include/hw/ide/internal.h
>> +++ b/include/hw/ide/internal.h
>> @@ -14,7 +14,6 @@
>>   #include "block/scsi.h"
>> /* debug IDE devices */
>> -//#define DEBUG_AIO
>>   #define USE_DMA_CDROM
>> typedef struct IDEBus IDEBus;
>> @@ -333,12 +332,15 @@ struct unreported_events {
>>   };
>> enum ide_dma_cmd {
>> -IDE_DMA_READ,
>> +IDE_DMA_READ = 0,
>>   IDE_DMA_WRITE,
>>   IDE_DMA_TRIM,
>>   IDE_DMA_ATAPI,
>> +IDE_DMA__COUNT
>>   };
>>   +extern const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT];
> 
> I recommend you to avoid this declaring extern const array with size, I
> remember some compilers (old GCC?) ignoring array size in extern. Eric
> will correct me!
> 
> It is much safer to use a getter:
> 

Well, whether or not the compiler ignores it, you're right that it's
safer to use a getter. I don't think the width being declared HURTS any
compiler though, does it?

> const char *IDE_DMA_CMD_lookup(enum ide_dma_cmd cmd)
> {
> static const 

Re: [Qemu-devel] [PATCH v2 10/17] MAINTAINERS: add missing SSI entries

[Alistair Francis]

On Wed, Aug 30, 2017 at 2:55 PM, Philippe Mathieu-Daudé  wrote:
> Alistair Francis volunteered :)
>
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Alistair Francis 

Thanks,
Alistair

> ---
>  MAINTAINERS | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bbe1191883..0a3a50aa25 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -986,10 +986,13 @@ F: hw/scsi/lsi53c895a.c
>
>  SSI
>  M: Peter Crosthwaite 
> +M: Alistair Francis 
>  S: Maintained
>  F: hw/ssi/*
>  F: hw/block/m25p80.c
> +F: include/hw/ssi/ssi.h
>  X: hw/ssi/xilinx_*
> +F: tests/m25p80-test.c
>
>  Xilinx SPI
>  M: Alistair Francis 
> --
> 2.14.1
>
>

Re: [Qemu-devel] [PATCH v2 14/17] MAINTAINERS: add missing entry for Generic Loader

[Alistair Francis]

On Wed, Aug 30, 2017 at 2:55 PM, Philippe Mathieu-Daudé  wrote:
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Alistair Francis 

Thanks,
Alistair

> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a48f633cad..0b77590dc8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1164,6 +1164,7 @@ M: Alistair Francis 
>  S: Maintained
>  F: hw/core/generic-loader.c
>  F: include/hw/core/generic-loader.h
> +F: docs/generic-loader.txt
>
>  CHRP NVRAM
>  M: Thomas Huth 
> --
> 2.14.1
>
>

Re: [Qemu-devel] [PATCH v2 0/7] QOMify MIPS cpu

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20170830225225.27925-1-f4...@amsat.org
Subject: [Qemu-devel] [PATCH v2 0/7] QOMify MIPS cpu
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]   patchew/20170830225225.27925-1-f4...@amsat.org -> 
patchew/20170830225225.27925-1-f4...@amsat.org
 t [tag update]patchew/cover.1504111803.git.jc...@redhat.com -> 
patchew/cover.1504111803.git.jc...@redhat.com
Switched to a new branch 'test'
2eba874055 mips: update mips_cpu_list() to use object_class_get_list()
c9170c27df mips: replace cpu_mips_init() with cpu_generic_init()
bd1c0e43a2 mips: MIPSCPU model subclasses
e77d1d74ea mips: call cpu_mips_realize_env() from mips_cpu_realizefn()
4f1073d8be mips: split cpu_mips_realize_env() out of cpu_mips_init()
22f783eb50 mips: introduce internal.h and cleanup cpu.h
f846c7bfc6 mips: move hw/mips/cputimer.c to target/mips/

=== OUTPUT BEGIN ===
Checking PATCH 1/7: mips: move hw/mips/cputimer.c to target/mips/...
Checking PATCH 2/7: mips: introduce internal.h and cleanup cpu.h...
ERROR: space prohibited after that '&' (ctx:WxW)
#727: FILE: target/mips/internal.h:230:
+if ((env->CP0_VPControl >> CP0VPCtl_DIS) & 1) {
  ^

ERROR: space prohibited after that '&' (ctx:WxW)
#735: FILE: target/mips/internal.h:238:
+((other_cpu->env.CP0_VPControl >> CP0VPCtl_DIS) & 1)) {
 ^

ERROR: space prohibited after that '&' (ctx:WxW)
#755: FILE: target/mips/internal.h:258:
+env->hflags |= (env->CP0_Status >> CP0St_KSU) & MIPS_HFLAG_KSU;
   ^

total: 3 errors, 0 warnings, 842 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/7: mips: split cpu_mips_realize_env() out of cpu_mips_init()...
Checking PATCH 4/7: mips: call cpu_mips_realize_env() from 
mips_cpu_realizefn()...
Checking PATCH 5/7: mips: MIPSCPU model subclasses...
Checking PATCH 6/7: mips: replace cpu_mips_init() with cpu_generic_init()...
Checking PATCH 7/7: mips: update mips_cpu_list() to use 
object_class_get_list()...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

[Qemu-devel] [PATCH v2 6/7] mips: replace cpu_mips_init() with cpu_generic_init()

[Philippe Mathieu-Daudé]

From: Igor Mammedov 

now cpu_mips_init() reimplements subset of cpu_generic_init()
tasks, so just drop it and use cpu_generic_init() directly.

Signed-off-by: Igor Mammedov 
Reviewed-by: Hervé Poussineau 
Signed-off-by: Philippe Mathieu-Daudé 
[PMD: use internal.h instead of cpu.h]
Tested-by: James Hogan 
---
 target/mips/cpu.h   |  3 +--
 hw/mips/cps.c   |  2 +-
 hw/mips/mips_fulong2e.c |  2 +-
 hw/mips/mips_jazz.c |  2 +-
 hw/mips/mips_malta.c|  2 +-
 hw/mips/mips_mipssim.c  |  2 +-
 hw/mips/mips_r4k.c  |  2 +-
 target/mips/translate.c | 17 -
 8 files changed, 7 insertions(+), 25 deletions(-)

diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 2f81e0f950..66265e4eb6 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -737,10 +737,9 @@ enum {
  */
 #define CPU_INTERRUPT_WAKE CPU_INTERRUPT_TGT_INT_0
 
-MIPSCPU *cpu_mips_init(const char *cpu_model);
 int cpu_mips_signal_handler(int host_signum, void *pinfo, void *puc);
 
-#define cpu_init(cpu_model) CPU(cpu_mips_init(cpu_model))
+#define cpu_init(cpu_model) cpu_generic_init(TYPE_MIPS_CPU, cpu_model)
 bool cpu_supports_cps_smp(const char *cpu_model);
 bool cpu_supports_isa(const char *cpu_model, unsigned int isa);
 void cpu_set_exception_base(int vp_index, target_ulong address);
diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index 4ef337d5c4..708899cf92 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -71,7 +71,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
 bool itu_present = false;
 
 for (i = 0; i < s->num_vp; i++) {
-cpu = cpu_mips_init(s->cpu_model);
+cpu = MIPS_CPU(cpu_generic_init(TYPE_MIPS_CPU, s->cpu_model));
 if (cpu == NULL) {
 error_setg(errp, "%s: CPU initialization failed",  __func__);
 return;
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 3532399a13..5d9462ec35 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -280,7 +280,7 @@ static void mips_fulong2e_init(MachineState *machine)
 if (cpu_model == NULL) {
 cpu_model = "Loongson-2E";
 }
-cpu = cpu_mips_init(cpu_model);
+cpu = MIPS_CPU(cpu_generic_init(TYPE_MIPS_CPU, cpu_model));
 if (cpu == NULL) {
 fprintf(stderr, "Unable to find CPU definition\n");
 exit(1);
diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c
index df2262a2a8..c1402de1ce 100644
--- a/hw/mips/mips_jazz.c
+++ b/hw/mips/mips_jazz.c
@@ -151,7 +151,7 @@ static void mips_jazz_init(MachineState *machine,
 if (cpu_model == NULL) {
 cpu_model = "R4000";
 }
-cpu = cpu_mips_init(cpu_model);
+cpu = MIPS_CPU(cpu_generic_init(TYPE_MIPS_CPU, cpu_model));
 if (cpu == NULL) {
 fprintf(stderr, "Unable to find CPU definition\n");
 exit(1);
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index af678f5784..9ecdc818b1 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -931,7 +931,7 @@ static void create_cpu_without_cps(const char *cpu_model,
 int i;
 
 for (i = 0; i < smp_cpus; i++) {
-cpu = cpu_mips_init(cpu_model);
+cpu = MIPS_CPU(cpu_generic_init(TYPE_MIPS_CPU, cpu_model));
 if (cpu == NULL) {
 fprintf(stderr, "Unable to find CPU definition\n");
 exit(1);
diff --git a/hw/mips/mips_mipssim.c b/hw/mips/mips_mipssim.c
index 07fc4c2300..1166834f54 100644
--- a/hw/mips/mips_mipssim.c
+++ b/hw/mips/mips_mipssim.c
@@ -163,7 +163,7 @@ mips_mipssim_init(MachineState *machine)
 cpu_model = "24Kf";
 #endif
 }
-cpu = cpu_mips_init(cpu_model);
+cpu = MIPS_CPU(cpu_generic_init(TYPE_MIPS_CPU, cpu_model));
 if (cpu == NULL) {
 fprintf(stderr, "Unable to find CPU definition\n");
 exit(1);
diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c
index 2f5ced7409..de212f5c13 100644
--- a/hw/mips/mips_r4k.c
+++ b/hw/mips/mips_r4k.c
@@ -193,7 +193,7 @@ void mips_r4k_init(MachineState *machine)
 cpu_model = "24Kf";
 #endif
 }
-cpu = cpu_mips_init(cpu_model);
+cpu = MIPS_CPU(cpu_generic_init(TYPE_MIPS_CPU, cpu_model));
 if (cpu == NULL) {
 fprintf(stderr, "Unable to find CPU definition\n");
 exit(1);
diff --git a/target/mips/translate.c b/target/mips/translate.c
index f7128bc91d..d16d879df7 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -20523,23 +20523,6 @@ void cpu_mips_realize_env(CPUMIPSState *env)
 mvp_init(env, env->cpu_model);
 }
 
-MIPSCPU *cpu_mips_init(const char *cpu_model)
-{
-ObjectClass *oc;
-MIPSCPU *cpu;
-
-oc = cpu_class_by_name(TYPE_MIPS_CPU, cpu_model);
-if (oc == NULL) {
-return NULL;
-}
-
-cpu = MIPS_CPU(object_new(object_class_get_name(oc)));
-
-object_property_set_bool(OBJECT(cpu), true, "realized", NULL);
-
-return cpu;
-}
-
 bool cpu_supports_cps_smp(const char 

[Qemu-devel] [PATCH v2 7/7] mips: update mips_cpu_list() to use object_class_get_list()

[Philippe Mathieu-Daudé]

while here, move it from translate_init.c to helper.c

Signed-off-by: Philippe Mathieu-Daudé 
Tested-by: Igor Mammedov 
Tested-by: James Hogan 
---
 target/mips/helper.c | 46 
 target/mips/translate_init.c | 10 --
 2 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/target/mips/helper.c b/target/mips/helper.c
index ea076261af..8d12b0088a 100644
--- a/target/mips/helper.c
+++ b/target/mips/helper.c
@@ -1093,3 +1093,49 @@ void QEMU_NORETURN do_raise_exception_err(CPUMIPSState 
*env,
 
 cpu_loop_exit_restore(cs, pc);
 }
+
+/* Sort alphabetically by type name, except for "any". */
+static gint mips_cpu_list_compare(gconstpointer a, gconstpointer b)
+{
+ObjectClass *class_a = (ObjectClass *)a;
+ObjectClass *class_b = (ObjectClass *)b;
+const char *name_a, *name_b;
+
+name_a = object_class_get_name(class_a);
+name_b = object_class_get_name(class_b);
+if (strcmp(name_a, "any-" TYPE_MIPS_CPU) == 0) {
+return 1;
+} else if (strcmp(name_b, "any-" TYPE_MIPS_CPU) == 0) {
+return -1;
+} else {
+return strcmp(name_a, name_b);
+}
+}
+
+static void mips_cpu_list_entry(gpointer data, gpointer user_data)
+{
+ObjectClass *oc = data;
+CPUListState *s = user_data;
+const char *typename;
+char *name;
+
+typename = object_class_get_name(oc);
+name = g_strndup(typename, strlen(typename) - strlen("-" TYPE_MIPS_CPU));
+(*s->cpu_fprintf)(s->file, "  %s\n", name);
+g_free(name);
+}
+
+void mips_cpu_list(FILE *f, fprintf_function cpu_fprintf)
+{
+CPUListState s = {
+.file = f,
+.cpu_fprintf = cpu_fprintf,
+};
+GSList *list;
+
+list = object_class_get_list(TYPE_MIPS_CPU, false);
+list = g_slist_sort(list, mips_cpu_list_compare);
+(*cpu_fprintf)(f, "Available CPUs:\n");
+g_slist_foreach(list, mips_cpu_list_entry, );
+g_slist_free(list);
+}
diff --git a/target/mips/translate_init.c b/target/mips/translate_init.c
index 8bbded46c4..b75f4c9065 100644
--- a/target/mips/translate_init.c
+++ b/target/mips/translate_init.c
@@ -767,16 +767,6 @@ static const mips_def_t *cpu_mips_find_by_name (const char 
*name)
 return NULL;
 }
 
-void mips_cpu_list (FILE *f, fprintf_function cpu_fprintf)
-{
-int i;
-
-for (i = 0; i < ARRAY_SIZE(mips_defs); i++) {
-(*cpu_fprintf)(f, "MIPS '%s'\n",
-   mips_defs[i].name);
-}
-}
-
 #ifndef CONFIG_USER_ONLY
 static void no_mmu_init (CPUMIPSState *env, const mips_def_t *def)
 {
-- 
2.14.1

[Qemu-devel] [PATCH v2 2/7] mips: introduce internal.h and cleanup cpu.h

[Philippe Mathieu-Daudé]

no logical change, only code movement (and fix a comment typo).

Signed-off-by: Philippe Mathieu-Daudé 
Tested-by: Igor Mammedov 
Tested-by: James Hogan 
---

This patch triggers 3 positive falses from checkpatch:

ERROR: space prohibited after that '&' (ctx:WxW)
#664: FILE: target/mips/internal.h:230:
+if ((env->CP0_VPControl >> CP0VPCtl_DIS) & 1) {
  ^
#672: FILE: target/mips/internal.h:238:
+((other_cpu->env.CP0_VPControl >> CP0VPCtl_DIS) & 1)) {
 ^
#692: FILE: target/mips/internal.h:258:
+env->hflags |= (env->CP0_Status >> CP0St_KSU) & MIPS_HFLAG_KSU;
   ^
total: 3 errors, 0 warnings, 842 lines checked

This is a "binary vs unary operators" confusion.

 target/mips/cpu.h| 354 +
 target/mips/internal.h   | 362 +++
 target/mips/cp0_timer.c  |   1 +
 target/mips/cpu.c|   1 +
 target/mips/gdbstub.c|   1 +
 target/mips/helper.c |   1 +
 target/mips/kvm.c|   1 +
 target/mips/machine.c|   1 +
 target/mips/msa_helper.c |   1 +
 target/mips/op_helper.c  |   1 +
 target/mips/translate.c  |   1 +
 11 files changed, 372 insertions(+), 353 deletions(-)
 create mode 100644 target/mips/internal.h

diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 74f6a5b098..2f81e0f950 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -1,8 +1,6 @@
 #ifndef MIPS_CPU_H
 #define MIPS_CPU_H
 
-//#define DEBUG_OP
-
 #define ALIGNED_ONLY
 
 #define CPUArchState struct CPUMIPSState
@@ -15,56 +13,11 @@
 
 struct CPUMIPSState;
 
-typedef struct r4k_tlb_t r4k_tlb_t;
-struct r4k_tlb_t {
-target_ulong VPN;
-uint32_t PageMask;
-uint16_t ASID;
-unsigned int G:1;
-unsigned int C0:3;
-unsigned int C1:3;
-unsigned int V0:1;
-unsigned int V1:1;
-unsigned int D0:1;
-unsigned int D1:1;
-unsigned int XI0:1;
-unsigned int XI1:1;
-unsigned int RI0:1;
-unsigned int RI1:1;
-unsigned int EHINV:1;
-uint64_t PFN[2];
-};
-
-#if !defined(CONFIG_USER_ONLY)
 typedef struct CPUMIPSTLBContext CPUMIPSTLBContext;
-struct CPUMIPSTLBContext {
-uint32_t nb_tlb;
-uint32_t tlb_in_use;
-int (*map_address) (struct CPUMIPSState *env, hwaddr *physical, int *prot, 
target_ulong address, int rw, int access_type);
-void (*helper_tlbwi)(struct CPUMIPSState *env);
-void (*helper_tlbwr)(struct CPUMIPSState *env);
-void (*helper_tlbp)(struct CPUMIPSState *env);
-void (*helper_tlbr)(struct CPUMIPSState *env);
-void (*helper_tlbinv)(struct CPUMIPSState *env);
-void (*helper_tlbinvf)(struct CPUMIPSState *env);
-union {
-struct {
-r4k_tlb_t tlb[MIPS_TLB_MAX];
-} r4k;
-} mmu;
-};
-#endif
 
 /* MSA Context */
 #define MSA_WRLEN (128)
 
-enum CPUMIPSMSADataFormat {
-DF_BYTE = 0,
-DF_HALF,
-DF_WORD,
-DF_DOUBLE
-};
-
 typedef union wr_t wr_t;
 union wr_t {
 int8_t  b[MSA_WRLEN/8];
@@ -682,40 +635,6 @@ static inline MIPSCPU *mips_env_get_cpu(CPUMIPSState *env)
 
 #define ENV_OFFSET offsetof(MIPSCPU, env)
 
-#ifndef CONFIG_USER_ONLY
-extern const struct VMStateDescription vmstate_mips_cpu;
-#endif
-
-void mips_cpu_do_interrupt(CPUState *cpu);
-bool mips_cpu_exec_interrupt(CPUState *cpu, int int_req);
-void mips_cpu_dump_state(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf,
- int flags);
-hwaddr mips_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
-int mips_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
-int mips_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
-void mips_cpu_do_unaligned_access(CPUState *cpu, vaddr addr,
-  MMUAccessType access_type,
-  int mmu_idx, uintptr_t retaddr);
-
-#if !defined(CONFIG_USER_ONLY)
-int no_mmu_map_address (CPUMIPSState *env, hwaddr *physical, int *prot,
-target_ulong address, int rw, int access_type);
-int fixed_mmu_map_address (CPUMIPSState *env, hwaddr *physical, int *prot,
-   target_ulong address, int rw, int access_type);
-int r4k_map_address (CPUMIPSState *env, hwaddr *physical, int *prot,
- target_ulong address, int rw, int access_type);
-void r4k_helper_tlbwi(CPUMIPSState *env);
-void r4k_helper_tlbwr(CPUMIPSState *env);
-void r4k_helper_tlbp(CPUMIPSState *env);
-void r4k_helper_tlbr(CPUMIPSState *env);
-void r4k_helper_tlbinv(CPUMIPSState *env);
-void r4k_helper_tlbinvf(CPUMIPSState *env);
-
-void mips_cpu_unassigned_access(CPUState *cpu, hwaddr addr,
-bool is_write, bool is_exec, int unused,
-unsigned size);
-#endif
-
 void mips_cpu_list (FILE *f, fprintf_function 

[Qemu-devel] [PATCH v2 3/7] mips: split cpu_mips_realize_env() out of cpu_mips_init()

[Philippe Mathieu-Daudé]

so it can be used in mips_cpu_realizefn() in the next commit

Signed-off-by: Philippe Mathieu-Daudé 
Tested-by: Igor Mammedov 
Tested-by: James Hogan 
---
 target/mips/internal.h  |  1 +
 target/mips/translate.c | 19 ---
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/target/mips/internal.h b/target/mips/internal.h
index 91c2df4537..cf4c9db427 100644
--- a/target/mips/internal.h
+++ b/target/mips/internal.h
@@ -132,6 +132,7 @@ void mips_tcg_init(void);
 
 /* TODO QOM'ify CPU reset and remove */
 void cpu_state_reset(CPUMIPSState *s);
+void cpu_mips_realize_env(CPUMIPSState *env);
 
 /* cp0_timer.c */
 uint32_t cpu_mips_get_random(CPUMIPSState *env);
diff --git a/target/mips/translate.c b/target/mips/translate.c
index f0febaf1b2..5fc7979ac5 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -20512,6 +20512,17 @@ void mips_tcg_init(void)
 
 #include "translate_init.c"
 
+void cpu_mips_realize_env(CPUMIPSState *env)
+{
+env->exception_base = (int32_t)0xBFC0;
+
+#ifndef CONFIG_USER_ONLY
+mmu_init(env, env->cpu_model);
+#endif
+fpu_init(env, env->cpu_model);
+mvp_init(env, env->cpu_model);
+}
+
 MIPSCPU *cpu_mips_init(const char *cpu_model)
 {
 MIPSCPU *cpu;
@@ -20524,13 +20535,7 @@ MIPSCPU *cpu_mips_init(const char *cpu_model)
 cpu = MIPS_CPU(object_new(TYPE_MIPS_CPU));
 env = >env;
 env->cpu_model = def;
-env->exception_base = (int32_t)0xBFC0;
-
-#ifndef CONFIG_USER_ONLY
-mmu_init(env, def);
-#endif
-fpu_init(env, def);
-mvp_init(env, def);
+cpu_mips_realize_env(env);
 
 object_property_set_bool(OBJECT(cpu), true, "realized", NULL);
 
-- 
2.14.1

[Qemu-devel] [PATCH v2 1/7] mips: move hw/mips/cputimer.c to target/mips/

[Philippe Mathieu-Daudé]

This timer is a required part of the MIPS32/MIPS64 System Control coprocessor
(CP0). Moving it with the other architecture related files will allow an opaque
use of CPUMIPSState* in the next commit (introduce "internal.h").

also remove it from 'user' targets, remove an unnecessary include.

Signed-off-by: Philippe Mathieu-Daudé 
Tested-by: Igor Mammedov 
Tested-by: James Hogan 
---
 hw/mips/cputimer.c => target/mips/cp0_timer.c | 1 -
 hw/mips/Makefile.objs | 2 +-
 target/mips/Makefile.objs | 2 +-
 3 files changed, 2 insertions(+), 3 deletions(-)
 rename hw/mips/cputimer.c => target/mips/cp0_timer.c (99%)

diff --git a/hw/mips/cputimer.c b/target/mips/cp0_timer.c
similarity index 99%
rename from hw/mips/cputimer.c
rename to target/mips/cp0_timer.c
index 8a166b3ea7..a9a58c5604 100644
--- a/hw/mips/cputimer.c
+++ b/target/mips/cp0_timer.c
@@ -21,7 +21,6 @@
  */
 
 #include "qemu/osdep.h"
-#include "hw/hw.h"
 #include "hw/mips/cpudevs.h"
 #include "qemu/timer.h"
 #include "sysemu/kvm.h"
diff --git a/hw/mips/Makefile.objs b/hw/mips/Makefile.objs
index 48cd2ef50e..17a311aaba 100644
--- a/hw/mips/Makefile.objs
+++ b/hw/mips/Makefile.objs
@@ -1,5 +1,5 @@
 obj-y += mips_r4k.o mips_malta.o mips_mipssim.o
-obj-y += addr.o cputimer.o mips_int.o
+obj-y += addr.o mips_int.o
 obj-$(CONFIG_JAZZ) += mips_jazz.o
 obj-$(CONFIG_FULONG) += mips_fulong2e.o
 obj-y += gt64xxx_pci.o
diff --git a/target/mips/Makefile.objs b/target/mips/Makefile.objs
index bc5ed8511f..651f36f517 100644
--- a/target/mips/Makefile.objs
+++ b/target/mips/Makefile.objs
@@ -1,4 +1,4 @@
 obj-y += translate.o dsp_helper.o op_helper.o lmi_helper.o helper.o cpu.o
 obj-y += gdbstub.o msa_helper.o mips-semi.o
-obj-$(CONFIG_SOFTMMU) += machine.o
+obj-$(CONFIG_SOFTMMU) += machine.o cp0_timer.o
 obj-$(CONFIG_KVM) += kvm.o
-- 
2.14.1

[Qemu-devel] [PATCH v2 4/7] mips: call cpu_mips_realize_env() from mips_cpu_realizefn()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
Tested-by: Igor Mammedov 
Tested-by: James Hogan 
---
 target/mips/cpu.c   | 3 +++
 target/mips/translate.c | 1 -
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 68bf423e9d..e3ef835599 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -123,6 +123,7 @@ static void mips_cpu_disas_set_info(CPUState *s, 
disassemble_info *info) {
 static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
 {
 CPUState *cs = CPU(dev);
+MIPSCPU *cpu = MIPS_CPU(dev);
 MIPSCPUClass *mcc = MIPS_CPU_GET_CLASS(dev);
 Error *local_err = NULL;
 
@@ -132,6 +133,8 @@ static void mips_cpu_realizefn(DeviceState *dev, Error 
**errp)
 return;
 }
 
+cpu_mips_realize_env(>env);
+
 cpu_reset(cs);
 qemu_init_vcpu(cs);
 
diff --git a/target/mips/translate.c b/target/mips/translate.c
index 5fc7979ac5..94c38e8755 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -20535,7 +20535,6 @@ MIPSCPU *cpu_mips_init(const char *cpu_model)
 cpu = MIPS_CPU(object_new(TYPE_MIPS_CPU));
 env = >env;
 env->cpu_model = def;
-cpu_mips_realize_env(env);
 
 object_property_set_bool(OBJECT(cpu), true, "realized", NULL);
 
-- 
2.14.1

[Qemu-devel] [PATCH v2 5/7] mips: MIPSCPU model subclasses

[Philippe Mathieu-Daudé]

From: Igor Mammedov 

Register separate QOM types for each mips cpu model,
so it would be possible to reuse generic CPU creation
routines.

Signed-off-by: Igor Mammedov 
Signed-off-by: Philippe Mathieu-Daudé 
[PMD: use internal.h, use void* to hold cpu_def in MIPSCPUClass,
 mark MIPSCPU abstract]
Tested-by: James Hogan 
---
 target/mips/cpu-qom.h|  1 +
 target/mips/internal.h   | 59 
 target/mips/cpu.c| 53 ++-
 target/mips/translate.c  | 13 +-
 target/mips/translate_init.c | 58 ++-
 5 files changed, 120 insertions(+), 64 deletions(-)

diff --git a/target/mips/cpu-qom.h b/target/mips/cpu-qom.h
index 3f5bf23823..085711d8f9 100644
--- a/target/mips/cpu-qom.h
+++ b/target/mips/cpu-qom.h
@@ -49,6 +49,7 @@ typedef struct MIPSCPUClass {
 
 DeviceRealize parent_realize;
 void (*parent_reset)(CPUState *cpu);
+const void *cpu_def;
 } MIPSCPUClass;
 
 typedef struct MIPSCPU MIPSCPU;
diff --git a/target/mips/internal.h b/target/mips/internal.h
index cf4c9db427..45ded3484c 100644
--- a/target/mips/internal.h
+++ b/target/mips/internal.h
@@ -7,6 +7,65 @@
 #ifndef MIPS_INTERNAL_H
 #define MIPS_INTERNAL_H
 
+
+/* MMU types, the first four entries have the same layout as the
+   CP0C0_MT field.  */
+enum mips_mmu_types {
+MMU_TYPE_NONE,
+MMU_TYPE_R4000,
+MMU_TYPE_RESERVED,
+MMU_TYPE_FMT,
+MMU_TYPE_R3000,
+MMU_TYPE_R6000,
+MMU_TYPE_R8000
+};
+
+struct mips_def_t {
+const char *name;
+int32_t CP0_PRid;
+int32_t CP0_Config0;
+int32_t CP0_Config1;
+int32_t CP0_Config2;
+int32_t CP0_Config3;
+int32_t CP0_Config4;
+int32_t CP0_Config4_rw_bitmask;
+int32_t CP0_Config5;
+int32_t CP0_Config5_rw_bitmask;
+int32_t CP0_Config6;
+int32_t CP0_Config7;
+target_ulong CP0_LLAddr_rw_bitmask;
+int CP0_LLAddr_shift;
+int32_t SYNCI_Step;
+int32_t CCRes;
+int32_t CP0_Status_rw_bitmask;
+int32_t CP0_TCStatus_rw_bitmask;
+int32_t CP0_SRSCtl;
+int32_t CP1_fcr0;
+int32_t CP1_fcr31_rw_bitmask;
+int32_t CP1_fcr31;
+int32_t MSAIR;
+int32_t SEGBITS;
+int32_t PABITS;
+int32_t CP0_SRSConf0_rw_bitmask;
+int32_t CP0_SRSConf0;
+int32_t CP0_SRSConf1_rw_bitmask;
+int32_t CP0_SRSConf1;
+int32_t CP0_SRSConf2_rw_bitmask;
+int32_t CP0_SRSConf2;
+int32_t CP0_SRSConf3_rw_bitmask;
+int32_t CP0_SRSConf3;
+int32_t CP0_SRSConf4_rw_bitmask;
+int32_t CP0_SRSConf4;
+int32_t CP0_PageGrain_rw_bitmask;
+int32_t CP0_PageGrain;
+target_ulong CP0_EBaseWG_rw_bitmask;
+int insn_flags;
+enum mips_mmu_types mmu_type;
+};
+
+extern const struct mips_def_t mips_defs[];
+extern const int mips_defs_number;
+
 enum CPUMIPSMSADataFormat {
 DF_BYTE = 0,
 DF_HALF,
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index e3ef835599..84b6f8bf68 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -146,12 +146,37 @@ static void mips_cpu_initfn(Object *obj)
 CPUState *cs = CPU(obj);
 MIPSCPU *cpu = MIPS_CPU(obj);
 CPUMIPSState *env = >env;
+MIPSCPUClass *mcc = MIPS_CPU_GET_CLASS(obj);
 
 cs->env_ptr = env;
 
 if (tcg_enabled()) {
 mips_tcg_init();
 }
+
+if (mcc->cpu_def) {
+env->cpu_model = mcc->cpu_def;
+}
+}
+
+static char *mips_cpu_type_name(const char *cpu_model)
+{
+return g_strdup_printf("%s-" TYPE_MIPS_CPU, cpu_model);
+}
+
+static ObjectClass *mips_cpu_class_by_name(const char *cpu_model)
+{
+ObjectClass *oc;
+char *typename;
+
+if (cpu_model == NULL) {
+return NULL;
+}
+
+typename = mips_cpu_type_name(cpu_model);
+oc = object_class_by_name(typename);
+g_free(typename);
+return oc;
 }
 
 static void mips_cpu_class_init(ObjectClass *c, void *data)
@@ -166,6 +191,7 @@ static void mips_cpu_class_init(ObjectClass *c, void *data)
 mcc->parent_reset = cc->reset;
 cc->reset = mips_cpu_reset;
 
+cc->class_by_name = mips_cpu_class_by_name;
 cc->has_work = mips_cpu_has_work;
 cc->do_interrupt = mips_cpu_do_interrupt;
 cc->cpu_exec_interrupt = mips_cpu_exec_interrupt;
@@ -193,14 +219,39 @@ static const TypeInfo mips_cpu_type_info = {
 .parent = TYPE_CPU,
 .instance_size = sizeof(MIPSCPU),
 .instance_init = mips_cpu_initfn,
-.abstract = false,
+.abstract = true,
 .class_size = sizeof(MIPSCPUClass),
 .class_init = mips_cpu_class_init,
 };
 
+static void mips_cpu_cpudef_class_init(ObjectClass *oc, void *data)
+{
+MIPSCPUClass *mcc = MIPS_CPU_CLASS(oc);
+mcc->cpu_def = data;
+}
+
+static void mips_register_cpudef_type(const struct mips_def_t *def)
+{
+char *typename = mips_cpu_type_name(def->name);
+TypeInfo ti = {
+.name = typename,
+.parent = TYPE_MIPS_CPU,
+.class_init 

[Qemu-devel] [PATCH v2 0/7] QOMify MIPS cpu

[Philippe Mathieu-Daudé]

Hi,

This series is based on Igor's "complete cpu QOMification" [1] but only modify
the MIPS part. Igor posted an updated series [2], both series should apply
separately.

Igor suggested on IRC this series could enter via the Machine core tree, so
I added Eduardo and Marcel.

Regards,

Phil.

[1]: http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg04414.html
[2]: http://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg03364.html

v2: 
- added Igor and James Tested-by
- squashed "!fixup mips: now than MIPSCPU is QOMified, mark it abstract"

PS: code movement somehow triggers a "binary vs unary operators" confusion
in checkpatch: "ERROR: space prohibited after that '&' (ctx:WxW)"

Igor Mammedov (2):
  mips: MIPSCPU model subclasses
  mips: replace cpu_mips_init() with cpu_generic_init()

Philippe Mathieu-Daudé (5):
  mips: move hw/mips/cputimer.c to target/mips/
  mips: introduce internal.h and cleanup cpu.h
  mips: split cpu_mips_realize_env() out of cpu_mips_init()
  mips: call cpu_mips_realize_env() from mips_cpu_realizefn()
  mips: update mips_cpu_list() to use object_class_get_list()

 target/mips/cpu-qom.h |   1 +
 target/mips/cpu.h | 357 +-
 target/mips/internal.h| 422 ++
 hw/mips/cps.c |   2 +-
 hw/mips/mips_fulong2e.c   |   2 +-
 hw/mips/mips_jazz.c   |   2 +-
 hw/mips/mips_malta.c  |   2 +-
 hw/mips/mips_mipssim.c|   2 +-
 hw/mips/mips_r4k.c|   2 +-
 hw/mips/cputimer.c => target/mips/cp0_timer.c |   2 +-
 target/mips/cpu.c |  57 +++-
 target/mips/gdbstub.c |   1 +
 target/mips/helper.c  |  47 +++
 target/mips/kvm.c |   1 +
 target/mips/machine.c |   1 +
 target/mips/msa_helper.c  |   1 +
 target/mips/op_helper.c   |   1 +
 target/mips/translate.c   |  23 +-
 target/mips/translate_init.c  |  68 +
 hw/mips/Makefile.objs |   2 +-
 target/mips/Makefile.objs |   2 +-
 21 files changed, 549 insertions(+), 449 deletions(-)
 create mode 100644 target/mips/internal.h
 rename hw/mips/cputimer.c => target/mips/cp0_timer.c (99%)

-- 
2.14.1

Re: [Qemu-devel] [Qemu-block] [PATCH v2 8/9] AHCI: pretty-print FIS to buffer instead of stderr

[John Snow]

On 08/30/2017 05:17 AM, Stefan Hajnoczi wrote:
> On Tue, Aug 29, 2017 at 04:49:33PM -0400, John Snow wrote:
>> The current FIS printing routines dump the FIS to screen. adjust this
>> such that it dumps to buffer instead, then use this ability to have
>> FIS dump mechanisms via trace-events instead of compiled defines.
>>
>> Signed-off-by: John Snow 
>> ---
>>  hw/ide/ahci.c   | 54 
>> +++--
>>  hw/ide/trace-events |  4 
>>  2 files changed, 48 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index a0a4dd6..2e75f9b 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -644,20 +644,45 @@ static void ahci_reset_port(AHCIState *s, int port)
>>  ahci_init_d2h(d);
>>  }
>>  
>> -static void debug_print_fis(uint8_t *fis, int cmd_len)
>> +/* Buffer pretty output based on a raw FIS structure. */
>> +static void ahci_pretty_buffer_fis(uint8_t *fis, int cmd_len, char **out)
> 
> Simplified function using GString:
> 
>   static char *ahci_pretty_buffer_fis(const uint8_t *fis, int cmd_len)
>   {
>   GString *s = g_string_new("FIS:");
>   int i;
> 
>   for (i = 0; i < cmd_len; i++) {
>   if (i % 16 == 0) {
>   g_string_append_printf(s, "\n0x%02x:", i);
> }
> 
>   g_string_append_printf(s, " %02x", fis[i]);
>   }
> 
>   g_string_append_c('\n');
>   return g_string_free(s, FALSE);
>   }
> 
> It's less efficient due to extra mallocs but a lot easier to read.
> 

eeeea I guess I don't need to be bleedingly efficient with debug
printfs...

And in this example I don't need to worry about the math being precisely
correct. I'll make the change :(

>>  {
>> -#if DEBUG_AHCI
>> +size_t bufsize;
>> +char *pbuf;
>> +char *pptr;
>> +size_t lines = DIV_ROUND_UP(cmd_len, 16);
>> +const char *preamble = "FIS:";
>>  int i;
>>  
>> -fprintf(stderr, "fis:");
>> +/* Total amount of memory to store FISes in HBA memory */
>> +g_assert_cmpint(cmd_len, <=, 0x100);
>> +g_assert(out);
>> +
>> +/* Printed like:
>> + * FIS:\n
>> + * 0x00: 00 11 22 33 44 55 66 77 88 99 aa bb cc dd ee \n
>> + * 0x10: ff \n
>> + * \0
>> + *
>> + * Four bytes for the preamble, seven for each line prefix (including a
>> + * newline to start a new line), three bytes for each source byte,
>> + * a trailing newline and a terminal null byte.
>> + */
>> +
>> +bufsize = strlen(preamble) + ((6 + 1) * lines) + (3 * cmd_len) + 1 + 1;
>> +pbuf = g_malloc(bufsize);
>> +pptr = pbuf;
>> +pptr += sprintf(pptr, "%s", preamble);
>>  for (i = 0; i < cmd_len; i++) {
>>  if ((i & 0xf) == 0) {
>> -fprintf(stderr, "\n%02x:",i);
>> +pptr += sprintf(pptr, "\n0x%02x: ", i);
>>  }
>> -fprintf(stderr, "%02x ",fis[i]);
>> +pptr += sprintf(pptr, "%02x ", fis[i]);
>>  }
>> -fprintf(stderr, "\n");
>> -#endif
>> +pptr += sprintf(pptr, "\n");
>> +pptr += 1; /* \0 */
>> +g_assert(pbuf + bufsize == pptr);
>> +*out = pbuf;
>>  }
>>  
>>  static bool ahci_map_fis_address(AHCIDevice *ad)
>> @@ -1201,7 +1226,12 @@ static void handle_reg_h2d_fis(AHCIState *s, int port,
>>   * table to ide_state->io_buffer */
>>  if (opts & AHCI_CMD_ATAPI) {
>>  memcpy(ide_state->io_buffer, _fis[AHCI_COMMAND_TABLE_ACMD], 
>> 0x10);
>> -debug_print_fis(ide_state->io_buffer, 0x10);
>> +if (TRACE_HANDLE_REG_H2D_FIS_DUMP_ENABLED) {
> 
> This should probably be:
> 
>   if (trace_event_get_state_backends(TRACE_HANDLE_REG_H2D_FIS_DUMP)) {
> 
> The difference is that TRACE_HANDLE_REG_H2D_FIS_DUMP_ENABLED is set at
> compile time while trace_event_get_state_backends() checks if the event
> is enabled at run-time.
> 
> Therefore TRACE_HANDLE_REG_H2D_FIS_DUMP_ENABLED causes the trace event
> to fire even when the user hasn't enabled the trace event yet.  That
> would be a waste of CPU.
> 

Oh, cool! Nice tip!

>> +char *pretty_fis;
>> +ahci_pretty_buffer_fis(ide_state->io_buffer, 0x10, _fis);
>> +trace_handle_reg_h2d_fis_dump(s, port, pretty_fis);
>> +g_free(pretty_fis);
>> +}
>>  s->dev[port].done_atapi_packet = false;
>>  /* XXX send PIO setup FIS */
>>  }
>> @@ -1256,8 +1286,12 @@ static int handle_cmd(AHCIState *s, int port, uint8_t 
>> slot)
>>  trace_handle_cmd_badmap(s, port, cmd_len);
>>  goto out;
>>  }
>> -debug_print_fis(cmd_fis, 0x80);
>> -
>> +if (TRACE_HANDLE_CMD_FIS_DUMP_ENABLED) {
> 
> Same here.
> 

Sure thing. There's probably a block in ATAPI that needs this treatment,
too.

>> +char *pretty_fis;
>> +ahci_pretty_buffer_fis(cmd_fis, 0x80, _fis);
>> +trace_handle_cmd_fis_dump(s, port, pretty_fis);
>> +g_free(pretty_fis);
>> +}
>>  switch (cmd_fis[0]) {
>>  case 

Re: [Qemu-devel] [PATCH v6 00/18] make dirty-bitmap byte-based

[John Snow]

On 08/30/2017 05:05 PM, Eric Blake wrote:
> There are patches floating around to add NBD_CMD_BLOCK_STATUS,
> but NBD wants to report status on byte granularity (even if the
> reporting will probably be naturally aligned to sectors or even
> much higher levels).  I've therefore started the task of
> converting our block status code to report at a byte granularity
> rather than sectors.
> 
> Now that 2.11 is open, I'm rebasing/reposting the remaining patches.
> 
> The overall conversion currently looks like:
> part 1: bdrv_is_allocated (merged in 2.10, commit 51b0a488)
> part 2: dirty-bitmap (this series, v5 was here [1])
> part 3: bdrv_get_block_status (v3 is posted [2] and is mostly reviewed, but
> needs a rebase)
> part 4: .bdrv_co_block_status (v2 is posted [3], but needs a rebase)
> 
> Available as a tag at:
> git fetch git://repo.or.cz/qemu/ericb.git nbd-byte-dirty-v6
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg03512.html
> [2] https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg03853.html
> [3] https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg04370.html
> 
> Diff from v5:
> - add another patch (more for ease of bookkeeping, as it was previously
> posted independently)
> - drop bug fixes that were hoisted into 2.10 (v5 1/18, plus 14/18)
> 
> 001/18:[down] 'block: Make bdrv_img_create() size selection easier to read'
> 002/18:[] [--] 'hbitmap: Rename serialization_granularity to 
> serialization_align'
> 003/18:[] [--] 'qcow2: Ensure bitmap serialization is aligned'
> 004/18:[] [--] 'dirty-bitmap: Drop unused functions'
> 005/18:[] [--] 'dirty-bitmap: Change bdrv_dirty_bitmap_size() to report 
> bytes'
> 006/18:[] [--] 'dirty-bitmap: Change bdrv_dirty_bitmap_*serialize*() to 
> take bytes'
> 007/18:[] [--] 'qcow2: Switch sectors_covered_by_bitmap_cluster() to 
> byte-based'
> 008/18:[] [--] 'dirty-bitmap: Set iterator start by offset, not sector'
> 009/18:[] [--] 'dirty-bitmap: Change bdrv_dirty_iter_next() to report 
> byte offset'
> 010/18:[] [--] 'dirty-bitmap: Change bdrv_get_dirty_count() to report 
> bytes'
> 011/18:[] [--] 'dirty-bitmap: Change bdrv_get_dirty_locked() to take 
> bytes'
> 012/18:[] [--] 'dirty-bitmap: Change bdrv_[re]set_dirty_bitmap() to use 
> bytes'
> 013/18:[] [--] 'mirror: Switch mirror_dirty_init() to byte-based 
> iteration'
> 014/18:[0004] [FC] 'qcow2: Switch qcow2_measure() to byte-based iteration'
> 015/18:[] [--] 'qcow2: Switch load_bitmap_data() to byte-based iteration'
> 016/18:[] [--] 'qcow2: Switch store_bitmap_data() to byte-based iteration'
> 017/18:[] [--] 'dirty-bitmap: Switch bdrv_set_dirty() to bytes'
> 018/18:[] [--] 'dirty-bitmap: Convert internal hbitmap size/granularity'
> 
> Eric Blake (18):
>   block: Make bdrv_img_create() size selection easier to read
>   hbitmap: Rename serialization_granularity to serialization_align
>   qcow2: Ensure bitmap serialization is aligned
>   dirty-bitmap: Drop unused functions
>   dirty-bitmap: Change bdrv_dirty_bitmap_size() to report bytes
>   dirty-bitmap: Change bdrv_dirty_bitmap_*serialize*() to take bytes
>   qcow2: Switch sectors_covered_by_bitmap_cluster() to byte-based
>   dirty-bitmap: Set iterator start by offset, not sector
>   dirty-bitmap: Change bdrv_dirty_iter_next() to report byte offset
>   dirty-bitmap: Change bdrv_get_dirty_count() to report bytes
>   dirty-bitmap: Change bdrv_get_dirty_locked() to take bytes
>   dirty-bitmap: Change bdrv_[re]set_dirty_bitmap() to use bytes
>   mirror: Switch mirror_dirty_init() to byte-based iteration
>   qcow2: Switch qcow2_measure() to byte-based iteration
>   qcow2: Switch load_bitmap_data() to byte-based iteration
>   qcow2: Switch store_bitmap_data() to byte-based iteration
>   dirty-bitmap: Switch bdrv_set_dirty() to bytes
>   dirty-bitmap: Convert internal hbitmap size/granularity
> 
>  include/block/block_int.h|   2 +-
>  include/block/dirty-bitmap.h |  41 +-
>  include/qemu/hbitmap.h   |   8 +--
>  block/io.c   |   6 +-
>  block.c  |   2 +-
>  block/backup.c   |   7 +--
>  block/dirty-bitmap.c | 130 
> ++-
>  block/mirror.c   |  76 +++--
>  block/qcow2-bitmap.c |  57 +--
>  block/qcow2.c|  22 
>  migration/block.c|  12 ++--
>  tests/test-hbitmap.c |  10 ++--
>  util/hbitmap.c   |   8 +--
>  13 files changed, 154 insertions(+), 227 deletions(-)
> 

Should this go through the bitmap tree, or since it's touching qcow2,
I'll let Kevin/Max/Stefan stage it?

--js

Re: [Qemu-devel] [PATCH v3 1/5] qemu-iotests: set TEST_DIR to a unique dir for each test

[Jeff Cody]

On Wed, Aug 30, 2017 at 06:15:05PM -0400, John Snow wrote:
> 
> 
> On 08/30/2017 12:52 PM, Jeff Cody wrote:
> > Right now, all qemu-iotests output data into the same scratch directory,
> > and so each test needs to be responsible for cleaning up its own files.
> > 
> > Have each test use 'scratch/$seq' as its temp directory, so the check
> > script can do simple cleanup of removing the whole temporary directory.
> > 
> > Reviewed-by: Eric Blake 
> > Signed-off-by: Jeff Cody 
> > ---
> >  tests/qemu-iotests/check | 21 +
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> > index d504b6e..f6ca85d 100755
> > --- a/tests/qemu-iotests/check
> > +++ b/tests/qemu-iotests/check
> > @@ -243,6 +243,7 @@ seq="check"
> >  
> >  for seq in $list
> >  do
> > +TEST_DIR_SEQ=$TEST_DIR/$seq
> >  err=false
> >  printf %s "$seq"
> >  if [ -n "$TESTS_REMAINING_LOG" ] ; then
> > @@ -289,13 +290,23 @@ do
> >  fi
> >  export OUTPUT_DIR=$PWD
> >  if $debug; then
> > -(cd "$source_iotests";
> > +(
> > +export TEST_DIR=$TEST_DIR_SEQ
> > +. "$source_iotests/common.config"
> > +. "$source_iotests/common.rc"
> 
> What purpose do these serve?
>

This is setting $TEST_DIR according to the $seq number (test # being run) in
the bash subshell that the tests are being run from.  So that all the other
variables that are based on the $TEST_DIR are set appropriately, this also
sources them in the subshell prior to running the test.  That way their
environment is with $TEST_DIR_SEQ rather than the original base $TEST_DIR.

> > +cd "$source_iotests" &&
> >  MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(($RANDOM % 255 + 1))} \
> > -$run_command -d 2>&1 | tee $tmp.out)
> > +$run_command -d 2>&1 | tee $tmp.out
> > +)
> >  else
> > -(cd "$source_iotests";
> > +(
> > +export TEST_DIR=$TEST_DIR_SEQ
> > +. "$source_iotests/common.config"
> > +. "$source_iotests/common.rc"
> > + cd "$source_iotests" &&
> >  MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(($RANDOM % 255 + 1))} \
> > -$run_command >$tmp.out 2>&1)
> > +$run_command >$tmp.out 2>&1
> > +)
> >  fi
> >  sts=$?
> >  $timestamp && _timestamp
> > @@ -359,6 +370,8 @@ do
> >  fi
> >  fi
> >  
> > +rm -rf "$TEST_DIR_SEQ"
> > +
> >  fi
> >  
> >  # come here for each test, except when $showme is true
> > 
> 
> Seems OK to me, though I am not able to answer all doubts about exactly
> how this may effect the strange pipe/subshell arrangements that occur
> deeper in the bowels of the included files for launching QEMU and so
> on.. I suppose that might be related to the inclusion of those
> common.XYZ files?
> 
> Tested-by: John Snow 
> Reviewed-by: John Snow 

Re: [Qemu-devel] [PATCH v3 4/5] qemu-iotests: make python tests attempt to leave intermediate files

[John Snow]

On 08/30/2017 06:35 PM, Eric Blake wrote:
> On 08/30/2017 05:28 PM, John Snow wrote:
> 
>> I'm a little iffy on this patch; I know that ./check can take care of
>> our temp files for us now, but because each python test is itself a
>> little mini-harness, I'm a little leery of moving the teardown to setup
>> and trying to pre-clean the confetti before the test begins.
>>
>> What's the benefit? We still have to clean up these files per-test, but
>> now it's slightly more error-prone and in a weird place.
>>
>> If we want to try to preserve the most-recent-failure-files, perhaps we
>> can define a setting in the python test-runner that allows us to
>> globally skip file cleanup.
> 
> On the other hand, since each test is a mini-harness, globally skipping
> cleanup will make a two-part test fail on the second because of garbage
> left behind by the first.
> 

subtext was to have per-subtest files.

> Patch 5 adds a comment with another possible solution: teach the python
> mini-harness to either clean all files in the directory, or to relocate
> the directory according to test name, so that each mini-test starts with
> a fresh location, and cleanup is then handled by the harness rather than
> spaghetti pre-cleanup.  But any solution is better than our current
> situation of nothing, so that's why I'm still okay with this patch as-is
> as offering more (even if not perfect) than before.
> 

I guess where I am unsure is really if this is better than what we
currently do, which is to (try) to clean up after each test as best as
we can. I don't see it as too different from trying to clean up before
each test.

It does give us the ability to leave behind a little detritus after a
failed run, but it's so imperfect that I wonder if it's worth shifting
this code around to change not much.

I won't die on this hill, it just strikes me a slightly less intuitive
use of the python unittest framework.

--js

Re: [Qemu-devel] [PATCH v3 5/5] qemu-iotests: add option to save temp files on error

[John Snow]

On 08/30/2017 12:52 PM, Jeff Cody wrote:
> Now that ./check takes care of cleaning up after each tests, it
> can also selectively not clean up.  Add option to leave all output from
> tests intact if that test encountered an error.
> 
> Note: this currently only works for bash tests, as the python tests
> still clean up after themselves manually.
> 
> Signed-off-by: Jeff Cody 
> ---
>  tests/qemu-iotests/check  | 10 +-
>  tests/qemu-iotests/common |  6 ++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index f6ca85d..8a5fc0d 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -370,7 +370,15 @@ do
>  fi
>  fi
>  
> -rm -rf "$TEST_DIR_SEQ"
> +#TODO: There is some intial work to save intermediate files
> +#  in python tests, but it is imperfect.  Having each
> +#  test record its test name, and the tearDown function
> +#  just move intermediate images to a subdirectory with
> +#  the test name may prove more useful.
> +if [ "$save_on_err" != "true" ] || [ "$err" != "true" ]
> +then
> +rm -rf "$TEST_DIR_SEQ"
> +fi
>  
>  fi
>  
> diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
> index d34c11c..d08b233 100644
> --- a/tests/qemu-iotests/common
> +++ b/tests/qemu-iotests/common
> @@ -42,6 +42,7 @@ expunge=true
>  have_test_arg=false
>  randomize=false
>  cachemode=false
> +save_on_err=false
>  rm -f $tmp.list $tmp.tmp $tmp.sed
>  
>  export IMGFMT=raw
> @@ -172,6 +173,7 @@ other options
>  -T  output timestamps
>  -r  randomize test order
>  -c mode cache mode
> +-s  save test scratch directory on test failure
>  
>  testlist options
>  -g group[,group...]include tests from these groups
> @@ -349,6 +351,10 @@ testlist options
>  xgroup=true
>  xpand=false
>  ;;
> +-s)
> +save_on_err=true
> +xpand=false
> +;;
>  '[0-9][0-9][0-9] [0-9][0-9][0-9][0-9]')
>  echo "No tests?"
>  status=1
> 

This, however, is definitely awesome.

Tested-by: John Snow 
Reviewed-by: John Snow 

Re: [Qemu-devel] [PATCH v3 4/5] qemu-iotests: make python tests attempt to leave intermediate files

[Eric Blake]

On 08/30/2017 05:28 PM, John Snow wrote:

> I'm a little iffy on this patch; I know that ./check can take care of
> our temp files for us now, but because each python test is itself a
> little mini-harness, I'm a little leery of moving the teardown to setup
> and trying to pre-clean the confetti before the test begins.
> 
> What's the benefit? We still have to clean up these files per-test, but
> now it's slightly more error-prone and in a weird place.
> 
> If we want to try to preserve the most-recent-failure-files, perhaps we
> can define a setting in the python test-runner that allows us to
> globally skip file cleanup.

On the other hand, since each test is a mini-harness, globally skipping
cleanup will make a two-part test fail on the second because of garbage
left behind by the first.

Patch 5 adds a comment with another possible solution: teach the python
mini-harness to either clean all files in the directory, or to relocate
the directory according to test name, so that each mini-test starts with
a fresh location, and cleanup is then handled by the harness rather than
spaghetti pre-cleanup.  But any solution is better than our current
situation of nothing, so that's why I'm still okay with this patch as-is
as offering more (even if not perfect) than before.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v3 4/5] qemu-iotests: make python tests attempt to leave intermediate files

[John Snow]

On 08/30/2017 02:33 PM, Eric Blake wrote:
> On 08/30/2017 11:52 AM, Jeff Cody wrote:
>> Now that 'check' will clean up after tests, try and make python
>> tests leave intermediate files so that they might be inspectable
>> on failure.
>>
>> This isn't perfect; the python unittest framework runs multiple
>> tests, even if previous tests failed.  So we need to make sure that
>> each test still begins with a "clean" slate, to prevent false
>> positives or tainted test runs.
>>
>> Rather than delete images in the unittest tearDown, invert this
>> and delete images to be used in that test at the beginning of the
>> setUp.  This is to make sure that the test run is not inadvertently
>> using file droppings from previous runs.  We must use 'blind_remove'
>> then for these, as the files might not exist yet, but we don't want
>> to throw an error for that.
>>
>> Signed-off-by: Jeff Cody 
>> ---
> 
>> +++ b/tests/qemu-iotests/030
>> @@ -21,7 +21,7 @@
>>  import time
>>  import os
>>  import iotests
>> -from iotests import qemu_img, qemu_io
>> +from iotests import qemu_img, qemu_io, blind_remove
>>  
>>  backing_img = os.path.join(iotests.test_dir, 'backing.img')
>>  mid_img = os.path.join(iotests.test_dir, 'mid.img')
>> @@ -31,6 +31,9 @@ class TestSingleDrive(iotests.QMPTestCase):
>>  image_len = 1 * 1024 * 1024 # MB
>>  
>>  def setUp(self):
>> +blind_remove(test_img)
>> +blind_remove(mid_img)
>> +blind_remove(backing_img)
> 
> Would it be any more pythonic to have support for:
> 
> blind_remove(test_img, mid_img, backing_img)
> 
> built into the previous patch?
> 

It should probably either take an iterable, or an arbitrary number of
arguments, or both, I dunno. I'm not a python.

>>  def tearDown(self):
>>  self.vm.shutdown()
>> -os.remove(self.test_img)
>> -os.remove(self.mid_img_abs)
>> -os.remove(self.backing_img_abs)
>> -try:
>> -os.rmdir(os.path.join(iotests.test_dir, self.dir1))
>> -os.rmdir(os.path.join(iotests.test_dir, self.dir3))
>> -os.rmdir(os.path.join(iotests.test_dir, self.dir2))
>> -except OSError as exception:
>> -if exception.errno != errno.EEXIST and exception.errno != 
>> errno.ENOTEMPTY:
>> -raise
> 
> The code removed here is using a syntax that differs from what you used
> in 3/5 when defining blind_remove; does that matter for 3/5?
> 
>> +++ b/tests/qemu-iotests/041
> 
>> +blind_remove(target_img)
>>  iotests.create_image(backing_img, self.image_len)
>>  qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
>> backing_img, test_img)
>>  self.vm = iotests.VM().add_drive(test_img, 
>> "node-name=top,backing.node-name=base")
>> @@ -49,12 +52,6 @@ class TestSingleDrive(iotests.QMPTestCase):
>>  
>>  def tearDown(self):
>>  self.vm.shutdown()
>> -os.remove(test_img)
>> -os.remove(backing_img)
>> -try:
>> -os.remove(target_img)
>> -except OSError:
>> -pass
> 
> You're changing failures other than ENOENT from ignored to explicit -
> nice little bug-fix along the way :)  I notice this pattern in multiple
> tests; is it worth mentioning in the commit message as intentional?
> 
>> @@ -797,6 +788,9 @@ class TestRepairQuorum(iotests.QMPTestCase):
>>  IMAGES = [ quorum_img1, quorum_img2, quorum_img3 ]
>>  
>>  def setUp(self):
>> +for i in self.IMAGES + [ quorum_repair_img, quorum_snapshot_file ]:
>> +blind_remove(i)
> 
> Again, would it be more pythonic if blind_remove() could take a list and
> automatically work on each element of the list, rather than having to
> make the caller iterate?
> 
>> +++ b/tests/qemu-iotests/057
>> @@ -23,7 +23,7 @@
>>  import time
>>  import os
>>  import iotests
>> -from iotests import qemu_img, qemu_io
>> +from iotests import qemu_img, qemu_io, blind_remove
>>  
>>  test_drv_base_name = 'drive'
>>  
>> @@ -36,6 +36,8 @@ class ImageSnapshotTestCase(iotests.QMPTestCase):
>>  
>>  def _setUp(self, test_img_base_name, image_num):
>>  self.vm = iotests.VM()
>> +for dev_expect in self.expect:
>> +blind_remove(dev_expect['image'])
> 
> Another place where python magic could make the caller nicer?
> 
>> +++ b/tests/qemu-iotests/118
> 
>> @@ -411,16 +411,16 @@ class TestFloppyInitiallyEmpty(TestInitiallyEmpty):
>>  
>>  class TestChangeReadOnly(ChangeBaseClass):
>>  def setUp(self):
>> -qemu_img('create', '-f', iotests.imgfmt, old_img, '1440k')
>> -qemu_img('create', '-f', iotests.imgfmt, new_img, '1440k')
>> -self.vm = iotests.VM()
>> -
>> -def tearDown(self):
>> -self.vm.shutdown()
>>  os.chmod(old_img, 0666)
>>  os.chmod(new_img, 0666)
>> -os.remove(old_img)
>> -os.remove(new_img)
>> +blind_remove(old_img)
>> +blind_remove(new_img)
>> +

Re: [Qemu-devel] [PATCH v3 3/5] qemu-iotests: add 'blind_remove' for python tests

[John Snow]

On 08/30/2017 02:13 PM, Eric Blake wrote:
> On 08/30/2017 11:52 AM, Jeff Cody wrote:
>> Add a function to attempt to 'blindly' remove a file, without
>> throwing an error if the file doesn't exist.
>>
>> Signed-off-by: Jeff Cody 
>> ---
>>  tests/qemu-iotests/iotests.py | 7 +++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 7233983..a2088c7 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -57,6 +57,13 @@ qemu_default_machine = 
>> os.environ.get('QEMU_DEFAULT_MACHINE')
>>  socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper')
>>  debug = False
>>  
>> +def blind_remove(filename):
>> +try:
>> +os.remove(filename)
>> +except OSError, error:
> 
> I'm assuming this works for both python 2 and 3?
> 

Appears to be python2 specific syntax, actually. using "as error"
appears to work in both 2.7 and 3.whatever, and according to
http://python3porting.com/differences.html will work in 2.6 too.

>> +if error.errno != errno.ENOENT:
>> +raise
>> +
> 
> Weak, since I'm not the strongest at python, but you can add:
> Reviewed-by: Eric Blake 
> 

Re: [Qemu-devel] [PATCH v3 2/5] qemu-iotests: remove file cleanup from bash tests

[John Snow]

On 08/30/2017 12:52 PM, Jeff Cody wrote:
> All files for a given test are now self-contained in a subdirectory,
> and therefore the "./check" script can do all file-related cleanup
> without any help.
> 
> This removes file cleanups from the bash tests.  The only cleanup left
> is whatever is needed to kill any spawned processes; e.g. _cleanup_qemu.
> 
> Reviewed-by: Eric Blake 
> Signed-off-by: Jeff Cody 
> ---
>  tests/qemu-iotests/001 |  6 --
>  tests/qemu-iotests/002 |  6 --
>  tests/qemu-iotests/003 |  6 --
>  tests/qemu-iotests/004 |  6 --
>  tests/qemu-iotests/005 |  6 --
>  tests/qemu-iotests/007 |  7 ---
>  tests/qemu-iotests/008 |  6 --
>  tests/qemu-iotests/009 |  6 --
>  tests/qemu-iotests/010 |  6 --
>  tests/qemu-iotests/011 |  6 --
>  tests/qemu-iotests/012 |  6 --
>  tests/qemu-iotests/013 |  6 --
>  tests/qemu-iotests/014 |  6 --
>  tests/qemu-iotests/015 |  7 ---
>  tests/qemu-iotests/017 |  6 --
>  tests/qemu-iotests/018 |  6 --
>  tests/qemu-iotests/019 |  8 
>  tests/qemu-iotests/020 |  8 
>  tests/qemu-iotests/021 |  6 --
>  tests/qemu-iotests/022 |  6 --
>  tests/qemu-iotests/023 |  6 --
>  tests/qemu-iotests/024 |  8 
>  tests/qemu-iotests/025 |  6 --
>  tests/qemu-iotests/026 |  7 ---
>  tests/qemu-iotests/027 |  6 --
>  tests/qemu-iotests/028 |  8 
>  tests/qemu-iotests/029 |  7 ---
>  tests/qemu-iotests/031 |  6 --
>  tests/qemu-iotests/032 |  6 --
>  tests/qemu-iotests/033 |  6 --
>  tests/qemu-iotests/034 |  6 --
>  tests/qemu-iotests/035 |  6 --
>  tests/qemu-iotests/036 |  6 --
>  tests/qemu-iotests/037 |  6 --
>  tests/qemu-iotests/038 |  6 --
>  tests/qemu-iotests/039 |  6 --
>  tests/qemu-iotests/042 |  6 --
>  tests/qemu-iotests/043 |  7 ---
>  tests/qemu-iotests/046 |  6 --
>  tests/qemu-iotests/047 |  6 --
>  tests/qemu-iotests/048 |  8 
>  tests/qemu-iotests/048.out |  1 -
>  tests/qemu-iotests/049 |  6 --
>  tests/qemu-iotests/050 |  8 
>  tests/qemu-iotests/051 |  6 --
>  tests/qemu-iotests/052 |  6 --
>  tests/qemu-iotests/053 |  7 ---
>  tests/qemu-iotests/054 |  6 --
>  tests/qemu-iotests/058 |  8 +---
>  tests/qemu-iotests/059 |  7 ---
>  tests/qemu-iotests/060 |  6 --
>  tests/qemu-iotests/061 |  6 --
>  tests/qemu-iotests/062 |  6 --
>  tests/qemu-iotests/063 |  7 ---
>  tests/qemu-iotests/064 |  6 --
>  tests/qemu-iotests/066 |  6 --
>  tests/qemu-iotests/068 |  6 --
>  tests/qemu-iotests/069 |  6 --
>  tests/qemu-iotests/070 |  6 --
>  tests/qemu-iotests/071 |  6 --
>  tests/qemu-iotests/072 |  6 --
>  tests/qemu-iotests/073 |  6 --
>  tests/qemu-iotests/074 |  9 -
>  tests/qemu-iotests/074.out |  1 -
>  tests/qemu-iotests/075 |  6 --
>  tests/qemu-iotests/076 |  6 --
>  tests/qemu-iotests/077 |  6 --
>  tests/qemu-iotests/078 |  6 --
>  tests/qemu-iotests/079 |  6 --
>  tests/qemu-iotests/080 |  7 ---
>  tests/qemu-iotests/081 |  8 
>  tests/qemu-iotests/082 |  6 --
>  tests/qemu-iotests/084 |  6 --
>  tests/qemu-iotests/085 | 13 +
>  tests/qemu-iotests/086 |  6 --
>  tests/qemu-iotests/088 |  7 ---
>  tests/qemu-iotests/089 |  6 --
>  tests/qemu-iotests/090 |  6 --
>  tests/qemu-iotests/091 |  8 +---
>  tests/qemu-iotests/092 |  7 ---
>  tests/qemu-iotests/094 |  9 +
>  tests/qemu-iotests/095 |  8 +---
>  tests/qemu-iotests/097 |  7 ---
>  tests/qemu-iotests/098 |  7 ---
>  tests/qemu-iotests/099 |  6 --
>  tests/qemu-iotests/101 |  6 --
>  tests/qemu-iotests/102 |  7 +--
>  tests/qemu-iotests/103 |  6 --
>  tests/qemu-iotests/104 |  2 --
>  tests/qemu-iotests/105 |  6 --
>  tests/qemu-iotests/106 |  6 --
>  tests/qemu-iotests/107 |  6 --
>  tests/qemu-iotests/108 |  6 --
>  tests/qemu-iotests/109 |  8 +---
>  tests/qemu-iotests/110 |  6 --
>  tests/qemu-iotests/111 |  6 --
>  tests/qemu-iotests/112 |  6 --
>  tests/qemu-iotests/113 |  6 --
>  tests/qemu-iotests/114 |  6 --
>  tests/qemu-iotests/115 |  6 --
>  tests/qemu-iotests/116 |  6 --
>  tests/qemu-iotests/117 |  7 +--
>  tests/qemu-iotests/119 |  6 --
>  tests/qemu-iotests/120 |  6 --
>  tests/qemu-iotests/121 |  6 --
>  tests/qemu-iotests/122 |  7 ---
>  tests/qemu-iotests/123 |  7 ---
>  

Re: [Qemu-devel] [PATCH v3 1/5] qemu-iotests: set TEST_DIR to a unique dir for each test

[John Snow]

On 08/30/2017 12:52 PM, Jeff Cody wrote:
> Right now, all qemu-iotests output data into the same scratch directory,
> and so each test needs to be responsible for cleaning up its own files.
> 
> Have each test use 'scratch/$seq' as its temp directory, so the check
> script can do simple cleanup of removing the whole temporary directory.
> 
> Reviewed-by: Eric Blake 
> Signed-off-by: Jeff Cody 
> ---
>  tests/qemu-iotests/check | 21 +
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index d504b6e..f6ca85d 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -243,6 +243,7 @@ seq="check"
>  
>  for seq in $list
>  do
> +TEST_DIR_SEQ=$TEST_DIR/$seq
>  err=false
>  printf %s "$seq"
>  if [ -n "$TESTS_REMAINING_LOG" ] ; then
> @@ -289,13 +290,23 @@ do
>  fi
>  export OUTPUT_DIR=$PWD
>  if $debug; then
> -(cd "$source_iotests";
> +(
> +export TEST_DIR=$TEST_DIR_SEQ
> +. "$source_iotests/common.config"
> +. "$source_iotests/common.rc"

What purpose do these serve?

> +cd "$source_iotests" &&
>  MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(($RANDOM % 255 + 1))} \
> -$run_command -d 2>&1 | tee $tmp.out)
> +$run_command -d 2>&1 | tee $tmp.out
> +)
>  else
> -(cd "$source_iotests";
> +(
> +export TEST_DIR=$TEST_DIR_SEQ
> +. "$source_iotests/common.config"
> +. "$source_iotests/common.rc"
> + cd "$source_iotests" &&
>  MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(($RANDOM % 255 + 1))} \
> -$run_command >$tmp.out 2>&1)
> +$run_command >$tmp.out 2>&1
> +)
>  fi
>  sts=$?
>  $timestamp && _timestamp
> @@ -359,6 +370,8 @@ do
>  fi
>  fi
>  
> +rm -rf "$TEST_DIR_SEQ"
> +
>  fi
>  
>  # come here for each test, except when $showme is true
> 

Seems OK to me, though I am not able to answer all doubts about exactly
how this may effect the strange pipe/subshell arrangements that occur
deeper in the bowels of the included files for launching QEMU and so
on.. I suppose that might be related to the inclusion of those
common.XYZ files?

Tested-by: John Snow 
Reviewed-by: John Snow 

Re: [Qemu-devel] [PATCH v1 1/1] target/xtensa: Use the pre-defined MEMTXATTRS_UNSPECIFIED macro

[Peter Maydell]

On 30 August 2017 at 19:02, Alistair Francis
 wrote:
> Instead of using the hardcoded (MemTxAttrs){0} for no memory attributes
> let's use the already defined MEMTXATTRS_UNSPECIFIED macro instead.
>
> Signed-off-by: Alistair Francis 
> ---
>
>  target/xtensa/op_helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/xtensa/op_helper.c b/target/xtensa/op_helper.c
> index 519fbeddd6..3d990c0caa 100644
> --- a/target/xtensa/op_helper.c
> +++ b/target/xtensa/op_helper.c
> @@ -1025,11 +1025,11 @@ void HELPER(ule_s)(CPUXtensaState *env, uint32_t br, 
> float32 a, float32 b)
>  uint32_t HELPER(rer)(CPUXtensaState *env, uint32_t addr)
>  {
>  return address_space_ldl(env->address_space_er, addr,
> - (MemTxAttrs){0}, NULL);
> + MEMTXATTRS_UNSPECIFIED, NULL);
>  }
>
>  void HELPER(wer)(CPUXtensaState *env, uint32_t data, uint32_t addr)
>  {
>  address_space_stl(env->address_space_er, addr, data,
> -  (MemTxAttrs){0}, NULL);
> +  MEMTXATTRS_UNSPECIFIED, NULL);
>  }

Might be worth noting in the commit that this is technically
a change of behaviour, because MEMTXATTRS_UNSPECIFIED
sets the 'unspecified' field to 1 whereas {0} doesn't.
I don't think anything actually checks that field, though.

thanks
-- PMM

[Qemu-devel] [PATCH v2 14/17] MAINTAINERS: add missing entry for Generic Loader

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index a48f633cad..0b77590dc8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1164,6 +1164,7 @@ M: Alistair Francis 
 S: Maintained
 F: hw/core/generic-loader.c
 F: include/hw/core/generic-loader.h
+F: docs/generic-loader.txt
 
 CHRP NVRAM
 M: Thomas Huth 
-- 
2.14.1

[Qemu-devel] [PATCH v2 13/17] MAINTAINERS: add missing AIO entry

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
Acked-by: Fam Zheng 
Reviewed-by: Stefan Hajnoczi 
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 20d65dca73..a48f633cad 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1225,6 +1225,7 @@ F: util/aio-*.c
 F: block/io.c
 F: migration/block*
 F: include/block/aio.h
+F: scripts/qemugdb/aio.py
 T: git git://github.com/stefanha/qemu.git block
 
 Block Jobs
-- 
2.14.1

[Qemu-devel] [PATCH v2 15/17] MAINTAINERS: add missing Cryptography entry

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0b77590dc8..7a0c00550e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1526,6 +1526,7 @@ S: Maintained
 F: crypto/
 F: include/crypto/
 F: tests/test-crypto-*
+F: tests/benchmark-crypto-*
 F: qemu.sasl
 
 Coroutines
-- 
2.14.1

[Qemu-devel] [PATCH v2 12/17] MAINTAINERS: add missing megasas test entry

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index fa74b7254b..20d65dca73 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1118,6 +1118,7 @@ L: qemu-bl...@nongnu.org
 S: Supported
 F: hw/scsi/megasas.c
 F: hw/scsi/mfi.h
+F: tests/megasas-test.c
 
 Network packet abstractions
 M: Dmitry Fleytman 
-- 
2.14.1

[Qemu-devel] [PATCH v2 07/17] MAINTAINERS: add missing qcow2 entry

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
Acked-by: Kevin Wolf 
Reviewed-by: Stefan Hajnoczi 
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index f4e07173c8..eb20365fbb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1841,6 +1841,7 @@ M: Max Reitz 
 L: qemu-bl...@nongnu.org
 S: Supported
 F: block/qcow2*
+F: docs/interop/qcow2.txt
 
 qcow
 M: Kevin Wolf 
-- 
2.14.1

[Qemu-devel] [PATCH v2 17/17] MAINTAINERS: update docs/interop/ entries

[Philippe Mathieu-Daudé]

moved in commit 7746cf8aab68

Signed-off-by: Philippe Mathieu-Daudé 
Acked-by: Fam Zheng 
Acked-by: John Snow 
---
 MAINTAINERS | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 452ccd71b4..833a7a6778 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1259,7 +1259,7 @@ F: block/dirty-bitmap.c
 F: include/qemu/hbitmap.h
 F: include/block/dirty-bitmap.h
 F: tests/test-hbitmap.c
-F: docs/bitmaps.md
+F: docs/interop/bitmaps.rst
 T: git git://github.com/famz/qemu.git bitmaps
 T: git git://github.com/jnsnow/qemu.git bitmaps
 
@@ -1828,7 +1828,7 @@ M: Denis V. Lunev 
 L: qemu-bl...@nongnu.org
 S: Supported
 F: block/parallels.c
-F: docs/specs/parallels.txt
+F: docs/interop/parallels.txt
 
 qed
 M: Stefan Hajnoczi 
-- 
2.14.1

[Qemu-devel] [PATCH v2 05/17] MAINTAINERS: add missing VMWare entry

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Thomas Huth 
Reviewed-by: Dmitry Fleytman 
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 223591c231..aa185c1edd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1126,6 +1126,7 @@ M: Dmitry Fleytman 
 S: Maintained
 F: hw/net/vmxnet*
 F: hw/scsi/vmw_pvscsi*
+F: tests/vmxnet3-test.c
 
 Rocker
 M: Jiri Pirko 
-- 
2.14.1

[Qemu-devel] [PATCH v2 16/17] MAINTAINERS: update docs/devel/ entries

[Philippe Mathieu-Daudé]

moved in commit ac06724a7158

Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Eric Blake 
---
 MAINTAINERS | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7a0c00550e..452ccd71b4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1414,7 +1414,7 @@ F: tests/test-qapi-*.c
 F: tests/test-qmp-*.c
 F: tests/test-visitor-serialization.c
 F: scripts/qapi*
-F: docs/qapi*
+F: docs/devel/qapi*
 T: git git://repo.or.cz/qemu/armbru.git qapi-next
 
 QAPI Schema
@@ -1466,7 +1466,7 @@ M: Markus Armbruster 
 S: Supported
 F: qmp.c
 F: monitor.c
-F: docs/*qmp-*
+F: docs/devel/*qmp-*
 F: scripts/qmp/
 F: tests/qmp-test.c
 T: git git://repo.or.cz/qemu/armbru.git qapi-next
@@ -1497,7 +1497,7 @@ S: Maintained
 F: trace/
 F: scripts/tracetool.py
 F: scripts/tracetool/
-F: docs/tracing.txt
+F: docs/devel/tracing.txt
 T: git git://github.com/stefanha/qemu.git tracing
 
 Checkpatch
@@ -1512,7 +1512,7 @@ F: include/migration/
 F: migration/
 F: scripts/vmstate-static-checker.py
 F: tests/vmstate-static-checker-data/
-F: docs/migration.txt
+F: docs/devel/migration.txt
 
 Seccomp
 M: Eduardo Otubo 
@@ -1914,5 +1914,5 @@ Documentation
 Build system architecture
 M: Daniel P. Berrange 
 S: Odd Fixes
-F: docs/build-system.txt
+F: docs/devel/build-system.txt
 
-- 
2.14.1

[Qemu-devel] [PATCH v2 11/17] MAINTAINERS: add missing entries for throttling infra

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Stefan Hajnoczi 
---
 MAINTAINERS | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0a3a50aa25..fa74b7254b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1558,8 +1558,10 @@ M: Alberto Garcia 
 S: Supported
 F: block/throttle-groups.c
 F: include/block/throttle-groups.h
-F: include/qemu/throttle.h
+F: include/qemu/throttle*.h
 F: util/throttle.c
+F: docs/throttle.txt
+F: tests/test-throttle.c
 L: qemu-bl...@nongnu.org
 
 UUID
-- 
2.14.1

[Qemu-devel] [PATCH v2 10/17] MAINTAINERS: add missing SSI entries

[Philippe Mathieu-Daudé]

Alistair Francis volunteered :)

Signed-off-by: Philippe Mathieu-Daudé 
---
 MAINTAINERS | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index bbe1191883..0a3a50aa25 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -986,10 +986,13 @@ F: hw/scsi/lsi53c895a.c
 
 SSI
 M: Peter Crosthwaite 
+M: Alistair Francis 
 S: Maintained
 F: hw/ssi/*
 F: hw/block/m25p80.c
+F: include/hw/ssi/ssi.h
 X: hw/ssi/xilinx_*
+F: tests/m25p80-test.c
 
 Xilinx SPI
 M: Alistair Francis 
-- 
2.14.1

[Qemu-devel] [PATCH v2 08/17] MAINTAINERS: add missing USB entry

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Thomas Huth 
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index eb20365fbb..dd5e2c2b6b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1004,6 +1004,7 @@ F: docs/usb2.txt
 F: docs/usb-storage.txt
 F: include/hw/usb.h
 F: include/hw/usb/
+F: default-configs/usb.mak
 
 USB (serial adapter)
 M: Gerd Hoffmann 
-- 
2.14.1

[Qemu-devel] [PATCH v2 02/17] MAINTAINERS: add missing Versatile PB entry

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index b363e1b9c9..5b7891addc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -515,6 +515,7 @@ M: Peter Maydell 
 L: qemu-...@nongnu.org
 S: Maintained
 F: hw/*/versatile*
+F: hw/misc/arm_sysctl.c
 
 Xilinx Zynq
 M: Edgar E. Iglesias 
-- 
2.14.1

[Qemu-devel] [PATCH v2 09/17] MAINTAINERS: add missing PCI entries

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index dd5e2c2b6b..bbe1191883 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -928,6 +928,8 @@ F: include/hw/pci/*
 F: hw/misc/pci-testdev.c
 F: hw/pci/*
 F: hw/pci-bridge/*
+F: docs/pci*
+F: docs/specs/*pci*
 
 ACPI/SMBIOS
 M: Michael S. Tsirkin 
-- 
2.14.1

[Qemu-devel] [PATCH v2 06/17] MAINTAINERS: add missing Guest Agent entries

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 MAINTAINERS | 4 
 1 file changed, 4 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index aa185c1edd..f4e07173c8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1434,6 +1434,10 @@ QEMU Guest Agent
 M: Michael Roth 
 S: Maintained
 F: qga/
+F: qemu-ga.texi
+F: scripts/qemu-guest-agent/
+F: tests/test-qga.c
+F: docs/interop/qemu-ga-ref.texi
 T: git git://github.com/mdroth/qemu.git qga
 
 QOM
-- 
2.14.1

[Qemu-devel] [PATCH v2 03/17] MAINTAINERS: add missing STM32 entry

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Thomas Huth 
Reviewed-by: Alistair Francis 
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5b7891addc..8c75ce477a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -552,6 +552,7 @@ F: hw/char/stm32f2xx_usart.c
 F: hw/timer/stm32f2xx_timer.c
 F: hw/adc/*
 F: hw/ssi/stm32f2xx_spi.c
+F: include/hw/*/stm32*.h
 
 Netduino 2
 M: Alistair Francis 
-- 
2.14.1

[Qemu-devel] [PATCH v2 04/17] MAINTAINERS: add missing entry for vhost

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 8c75ce477a..223591c231 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1029,6 +1029,7 @@ vhost
 M: Michael S. Tsirkin 
 S: Supported
 F: hw/*/*vhost*
+F: docs/interop/vhost-user.txt
 
 virtio
 M: Michael S. Tsirkin 
-- 
2.14.1

[Qemu-devel] [PATCH v2 00/17] add missing entries in MAINTAINERS

[Philippe Mathieu-Daudé]

Hi,

I tried to have a more helpful ./scripts/get_maintainer.pl output, filling
missing entries in MAINTAINERS.

Regards,

Phil.

v2:
- add R-b & A-b
- clean ARM entries (Thomas Huth)
- moved files: comment since which commit (Eric Blake)
- drop inconsistent patches (default-configs.mak related to hw/machine, no CPU)
- drop unhappy patches (include/standard-headers/linux/*)
- drop unreplied patches

Philippe Mathieu-Daudé (17):
  MAINTAINERS: add missing ARM entries
  MAINTAINERS: add missing Versatile PB entry
  MAINTAINERS: add missing STM32 entry
  MAINTAINERS: add missing entry for vhost
  MAINTAINERS: add missing VMWare entry
  MAINTAINERS: add missing Guest Agent entries
  MAINTAINERS: add missing qcow2 entry
  MAINTAINERS: add missing USB entry
  MAINTAINERS: add missing PCI entries
  MAINTAINERS: add missing SSI entries
  MAINTAINERS: add missing entries for throttling infra
  MAINTAINERS: add missing megasas test entry
  MAINTAINERS: add missing AIO entry
  MAINTAINERS: add missing entry for Generic Loader
  MAINTAINERS: add missing Cryptography entry
  MAINTAINERS: update docs/devel/ entries
  MAINTAINERS: update docs/interop/ entries

 MAINTAINERS | 44 ++--
 1 file changed, 34 insertions(+), 10 deletions(-)

-- 
2.14.1

[Qemu-devel] [PATCH v2 01/17] MAINTAINERS: add missing ARM entries

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 MAINTAINERS | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index ccee28b12d..b363e1b9c9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -380,6 +380,7 @@ M: Peter Maydell 
 L: qemu-...@nongnu.org
 S: Maintained
 F: hw/char/pl011.c
+F: include/hw/char/pl011.h
 F: hw/display/pl110*
 F: hw/dma/pl080.c
 F: hw/dma/pl330.c
@@ -403,13 +404,15 @@ F: hw/intc/gic_internal.h
 F: hw/misc/a9scu.c
 F: hw/misc/arm11scu.c
 F: hw/timer/a9gtimer*
-F: hw/timer/arm_*
-F: include/hw/arm/arm.h
+F: hw/timer/arm*
+F: include/hw/arm/arm*.h
 F: include/hw/intc/arm*
 F: include/hw/misc/a9scu.h
 F: include/hw/misc/arm11scu.h
 F: include/hw/timer/a9gtimer.h
 F: include/hw/timer/arm_mptimer.h
+F: include/hw/timer/armv7m_systick.h
+F: tests/test-arm-mptimer.c
 
 Exynos
 M: Igor Mitsyanko 
-- 
2.14.1

Re: [Qemu-devel] [PATCH 3/6] tests: Enable the drive_del test also on s390x

[Cleber Rosa]

On 08/17/2017 02:25 AM, Thomas Huth wrote:
> By using the "virtio-xxx" device name aliases instead of the
> "virtio-xxx-pci" names, we can use this test on s390x, too,
> to check that adding and deleting also works fine with the
> virtio-ccw bus.
> 
> Signed-off-by: Thomas Huth 
> ---
>  tests/Makefile.include |  1 +
>  tests/drive_del-test.c | 13 +++--
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 0bb18b3..ff2a551 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -363,6 +363,7 @@ check-qtest-s390x-y = tests/boot-serial-test$(EXESUF)
>  check-qtest-s390x-$(CONFIG_SLIRP) += tests/test-netfilter$(EXESUF)
>  check-qtest-s390x-$(CONFIG_POSIX) += tests/test-filter-mirror$(EXESUF)
>  check-qtest-s390x-$(CONFIG_POSIX) += tests/test-filter-redirector$(EXESUF)
> +check-qtest-s390x-y += tests/drive_del-test$(EXESUF)
>  
>  check-qtest-generic-y += tests/qom-test$(EXESUF)
>  check-qtest-generic-y += tests/test-hmp$(EXESUF)
> diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c
> index 2175139..efceb31 100644
> --- a/tests/drive_del-test.c
> +++ b/tests/drive_del-test.c
> @@ -65,12 +65,12 @@ static void test_after_failed_device_add(void)
>  
>  qtest_start("-drive if=none,id=drive0");
>  
> -/* Make device_add fail.  If this leaks the virtio-blk-pci device then a
> +/* Make device_add fail.  If this leaks the virtio-blk device then a
>   * reference to drive0 will also be held (via qdev properties).
>   */
>  response = qmp("{'execute': 'device_add',"
> " 'arguments': {"
> -   "   'driver': 'virtio-blk-pci',"
> +   "   'driver': 'virtio-blk',"
> "   'drive': 'drive0'"
> "}}");
>  g_assert(response);
> @@ -82,7 +82,7 @@ static void test_after_failed_device_add(void)
>  drive_del();
>  
>  /* Try to re-add the drive.  This fails with duplicate IDs if a leaked
> - * virtio-blk-pci exists that holds a reference to the old drive0.
> + * virtio-blk exists that holds a reference to the old drive0.
>   */
>  drive_add();
>  
> @@ -93,7 +93,7 @@ static void test_drive_del_device_del(void)
>  {
>  /* Start with a drive used by a device that unplugs instantaneously */
>  qtest_start("-drive if=none,id=drive0,file=null-co://,format=raw"
> -" -device virtio-scsi-pci"
> +" -device virtio-scsi"
>  " -device scsi-hd,drive=drive0,id=dev0");
>  
>  /*
> @@ -114,9 +114,10 @@ int main(int argc, char **argv)
>  
>  qtest_add_func("/drive_del/without-dev", test_drive_without_dev);
>  
> -/* TODO I guess any arch with PCI would do */
> +/* TODO I guess any arch with a hot-pluggable virtio bus would do */
>  if (!strcmp(arch, "i386") || !strcmp(arch, "x86_64") ||
> -!strcmp(arch, "ppc") || !strcmp(arch, "ppc64")) {
> +!strcmp(arch, "ppc") || !strcmp(arch, "ppc64") ||
> +!strcmp(arch, "s390x")) {
>  qtest_add_func("/drive_del/after_failed_device_add",
> test_after_failed_device_add);
>  qtest_add_func("/blockdev/drive_del_device_del",
> 

I honestly can't comment on the device to be used on this test (as
others have commented).  Putting that aside, and given the fact  that I
went through the lengths of testing it on the s390x target:

Tested-by: Cleber Rosa 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [Qemu-block] Persistent bitmaps for non-qcow2 formats

[John Snow]

On 08/30/2017 09:45 AM, Daniel P. Berrange wrote:
> On Wed, Aug 30, 2017 at 02:36:11PM +0100, Stefan Hajnoczi wrote:
>> On Tue, Aug 22, 2017 at 03:07:04PM -0400, John Snow wrote:
>>> (3) Add either a new flag that turns qcow2's backing file into a full
>>> R/W backing file, or add a new extension to qcow2 entirely (bypassing
>>> the traditional backing file mechanism to avoid confusion for older
>>> tooling) that adds a new read-write backing file field.
>>>
>>> This RW backing file field will be used for all reads AND writes; the
>>> qcow2 in question becomes a metadata container on top of the BDS chain.
>>> We can re-use Vladimir's bitmap persistence extension to save bitmaps in
>>> a qcow2 shell.
>>>
>>> The qcow2 becomes effectively a metadata cache for a new (essentially)
>>> filter node that handles features such as bitmaps. This could also be
>>> used to provide allocation map data for RAW files and other goodies down
>>> the road.
>>>
>>> Hopefully this achieves our desire to not create new formats AND our
>>> desire to concentrate features (and debugging, testing, etc) into qcow2,
>>> while allowing users to "have bitmaps with raw files."
>>>
>>> Of course, in this scenario, users now have two files: a qcow2 wrapper
>>> and the actual raw file in question; but regardless of how we were going
>>> to solve this, a raw file necessitates an external file of some sort,
>>> else we give up the idea that it was a raw file.
>>
>> There is some complexity here for management tools:
>>
>> If the underlying image is resized, who resizes the qcow2 and how do
>> they know to do it?
>>
>> If QEMU's resize/truncate command it used, does first try to resize the
>> underlying image and then resize the qcow2?  This is probably the sanest
>> approach.
>>
>> If the underlying image is moved to a new location, does the qcow2 file
>> need to be modified and who does that?
>>
>> Management tools need to figure out how to represent manage this extra
>> qcow2 file.  The easiest solution is to punt it to the user and treat it
>> as part of a backing file chain.  If the management tool wants to
>> automatically manage the qcow2 so the user just specifies the underlying
>> image and enables the persistent bitmap checkbox, then it becomes more
>> complicated.
> 
> Indeed, I don't think it is practical to have libvirt / QEMU automagically
> create a qcow2 overlay on disk. Something has to decide where this would
> be stored. You might say just put it alongside the raw file, but it might
> not be a local file at all, it could be a NBD, or RBD raw "file". So do
> we create  local qcow2 file, or store a qcow2 file inside another RBD
> volume to hold the persistent bitmap. This kind of decision needs to be
> made by the mgmt app since only it knows about its storage mgmt model.

Oh, you mean to say mgmt app like VMM or something even above libvirt,
yes? Who currently makes the decision for where snapshot files and the
like goes, does libvirt not decide that, but the app using it?

> At this point you might as well just let the mgmt app take care of it
> all and not try to do anything magical with qcow2 overlays in libvirt/QEMU
> 

Might be the sanest, yes -- but say I don't offer an "automagic" way to
add a bitmap to a raw file to a running QEMU instance but rather offer a
way to qcow2ify an existing node:

We could create a wrapper:

qemu-img create -f qcow2 -o wrapper wrapper.qcow2

Then add a node:

blockdev_add driver=qcow2 node-name=foo file.driver=file
file.filename=wrapper.qcow2

(The size of the drive could perhaps remain as zero temporarily here)

Then some magic to make it the active target of whatever blockbackend
was using the old image, perhaps?:

blockdev_magic target=virtioblk0 node-name=foo

At this point, virtioblk0 is now backed by our new magic qcow2 node
which provides bitmap features for whatever kind of backing storage
virtioblk0 had, and nothing automagic happened -- we passed explicit
file references.

Any magic that we wish to provide can happen in libvirt or above.

--js

Re: [Qemu-devel] Persistent bitmaps for non-qcow2 formats

[John Snow]

On 08/30/2017 08:58 AM, Yaniv Lavi (Dary) wrote:
> 
> 
> We had no reason to switch to anything else so far and I'm sure this
> option was not available when we started supporting raw format. 
>  
> 

Yeah, they don't exist yet...! I've looped you in to see if the proposal
being discussed would alleviate the need for bitmaps for "other formats"
if we can offer a "raw mode qcow2."

At the moment I am going to still try to add bitmaps to other formats
(through the use of a qcow2 wrapper) but it sounds like a raw-layout
qcow2 might provide some benefits too.

-js

Re: [Qemu-devel] [PATCH 02/47] MAINTAINERS: add missing ARM entries

[Philippe Mathieu-Daudé]

On 07/28/2017 03:55 AM, Thomas Huth wrote:

On 28.07.2017 07:35, Philippe Mathieu-Daudé wrote:

Signed-off-by: Philippe Mathieu-Daudé 
---
  MAINTAINERS | 8 
  1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 972118e70b..795f89f709 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -120,6 +120,8 @@ F: include/hw/cpu/a*mpcore.h
  F: disas/arm.c
  F: disas/arm-a64.cc
  F: disas/libvixl/
+F: default-configs/arm-softmmu.mak
+F: default-configs/aarch64-softmmu.mak


You've added this to the TCG CPU core section, but strictly speaking
these files are also used for the machine emulation in general (and also
for KVM). So not sure whether this is a good fit here ... up to Peter to
decide.


You are right.




  CRIS
  M: Edgar E. Iglesias 
@@ -380,6 +382,7 @@ M: Peter Maydell 
  L: qemu-...@nongnu.org
  S: Maintained
  F: hw/char/pl011.c
+F: include/hw/char/pl011.h
  F: hw/display/pl110*
  F: hw/dma/pl080.c
  F: hw/dma/pl330.c
@@ -402,14 +405,19 @@ F: hw/intc/arm*
  F: hw/intc/gic_internal.h
  F: hw/misc/a9scu.c
  F: hw/misc/arm11scu.c
+F: hw/misc/arm_sysctl.c


According to a comment in that file, it is about RealView/Versatile
boards instead, so this is the wrong section here?


Wrong section indeed.




  F: hw/timer/a9gtimer*
  F: hw/timer/arm_*
+F: hw/timer/armv7m_systick.c


How about rather removing the underscore in the previous wildcard entry?


I also wondered, because I'm a slowly working branch where I try to boot 
some Marvell SoC, and I named the timer "armada_timer.c" following the 
Linux device-tree naming:


https://www.kernel.org/doc/Documentation/devicetree/bindings/timer/marvell%2Carmada-370-xp-timer.txt

But if I ever finish it I can add an exclude entry, so I'll follow your 
advice.





  F: include/hw/arm/arm.h
+F: include/hw/arm/armv7m*.h
  F: include/hw/intc/arm*
  F: include/hw/misc/a9scu.h
  F: include/hw/misc/arm11scu.h
  F: include/hw/timer/a9gtimer.h
  F: include/hw/timer/arm_mptimer.h
+F: include/hw/timer/armv7m_systick.h
+F: tests/test-arm-mptimer.c
  
  Exynos

  M: Igor Mitsyanko 



  Thomas

[Qemu-devel] [PATCH v6 17/18] dirty-bitmap: Switch bdrv_set_dirty() to bytes

[Eric Blake]

Both callers already had bytes available, but were scaling to
sectors.  Move the scaling to internal code.  In the case of
bdrv_aligned_pwritev(), we are now passing the exact offset
rather than a rounded sector-aligned value, but that's okay
as long as dirty bitmap widens start/bytes to granularity
boundaries.

Signed-off-by: Eric Blake 
Reviewed-by: John Snow 

---
v4: only context changes
v3: rebase to lock context changes, R-b kept
v2: no change
---
 include/block/block_int.h | 2 +-
 block/io.c| 6 ++
 block/dirty-bitmap.c  | 7 ---
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 7571c0aaaf..4d01dc3fa6 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -969,7 +969,7 @@ void blk_dev_eject_request(BlockBackend *blk, bool force);
 bool blk_dev_is_tray_open(BlockBackend *blk);
 bool blk_dev_is_medium_locked(BlockBackend *blk);

-void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int64_t nr_sect);
+void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes);
 bool bdrv_requests_pending(BlockDriverState *bs);

 void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out);
diff --git a/block/io.c b/block/io.c
index 26003814eb..926beebc8f 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1334,7 +1334,6 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild 
*child,
 bool waited;
 int ret;

-int64_t start_sector = offset >> BDRV_SECTOR_BITS;
 int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
 uint64_t bytes_remaining = bytes;
 int max_transfer;
@@ -1409,7 +1408,7 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild 
*child,
 bdrv_debug_event(bs, BLKDBG_PWRITEV_DONE);

 atomic_inc(>write_gen);
-bdrv_set_dirty(bs, start_sector, end_sector - start_sector);
+bdrv_set_dirty(bs, offset, bytes);

 stat64_max(>wr_highest_offset, offset + bytes);

@@ -2412,8 +2411,7 @@ int coroutine_fn bdrv_co_pdiscard(BlockDriverState *bs, 
int64_t offset,
 ret = 0;
 out:
 atomic_inc(>write_gen);
-bdrv_set_dirty(bs, req.offset >> BDRV_SECTOR_BITS,
-   req.bytes >> BDRV_SECTOR_BITS);
+bdrv_set_dirty(bs, req.offset, req.bytes);
 tracked_request_end();
 bdrv_dec_in_flight(bs);
 return ret;
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 9821225523..b54eed46e4 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -629,10 +629,10 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap 
*bitmap)
 hbitmap_deserialize_finish(bitmap->bitmap);
 }

-void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
-int64_t nr_sectors)
+void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes)
 {
 BdrvDirtyBitmap *bitmap;
+int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);

 if (QLIST_EMPTY(>dirty_bitmaps)) {
 return;
@@ -644,7 +644,8 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t 
cur_sector,
 continue;
 }
 assert(!bdrv_dirty_bitmap_readonly(bitmap));
-hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
+hbitmap_set(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
+end_sector - (offset >> BDRV_SECTOR_BITS));
 }
 bdrv_dirty_bitmaps_unlock(bs);
 }
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 1/2] tests: Use real size for iov tests

[Eric Blake]

On 08/30/2017 06:33 AM, Juan Quintela wrote:
> We were using -1 instead of the real size because the functions check
> what is bigger, size in bytes or the size of the iov.  Recent gcc's
> barf at this.
> 
> Signed-off-by: Juan Quintela 
> 
> --
> 
> Remove comments about this feature.
> Fix missing -1.
> ---
>  include/qemu/iov.h |  6 --
>  tests/test-iov.c   | 10 +-
>  2 files changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/include/qemu/iov.h b/include/qemu/iov.h
> index bd9fd55b0a..72d4c559b4 100644
> --- a/include/qemu/iov.h
> +++ b/include/qemu/iov.h
> @@ -31,11 +31,6 @@ size_t iov_size(const struct iovec *iov, const unsigned 
> int iov_cnt);
>   * Number of bytes actually copied will be returned, which is
>   *  min(bytes, iov_size(iov)-offset)
>   * `Offset' must point to the inside of iovec.
> - * It is okay to use very large value for `bytes' since we're
> - * limited by the size of the iovec anyway, provided that the
> - * buffer pointed to by buf has enough space.

Is this part of the comment still okay?

>  One possible
> - * such "large" value is -1 (sinice size_t is unsigned),

Nice that we're losing the typo in the process :)

> - * so specifying `-1' as `bytes' means 'up to the end of iovec'.

I agree with dropping this though.  As mentioned elsewhere, we had
crossed mails between v1 reviews and v2 submission, so we'll probably
need a v3 anyways.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH v6 13/18] mirror: Switch mirror_dirty_init() to byte-based iteration

[Eric Blake]

Now that we have adjusted the majority of the calls this function
makes to be byte-based, it is easier to read the code if it makes
passes over the image using bytes rather than sectors.

Signed-off-by: Eric Blake 
Reviewed-by: John Snow 

---
v6: no change
v5: rebase to earlier changes
v2-v4: no change
---
 block/mirror.c | 38 ++
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 18172a56a3..87d9857475 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -613,15 +613,13 @@ static void mirror_throttle(MirrorBlockJob *s)

 static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
 {
-int64_t sector_num, end;
+int64_t offset;
 BlockDriverState *base = s->base;
 BlockDriverState *bs = s->source;
 BlockDriverState *target_bs = blk_bs(s->target);
-int ret, n;
+int ret;
 int64_t count;

-end = s->bdev_length / BDRV_SECTOR_SIZE;
-
 if (base == NULL && !bdrv_has_zero_init(target_bs)) {
 if (!bdrv_can_write_zeroes_with_unmap(target_bs)) {
 bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, s->bdev_length);
@@ -629,9 +627,9 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
 }

 s->initial_zeroing_ongoing = true;
-for (sector_num = 0; sector_num < end; ) {
-int nb_sectors = MIN(end - sector_num,
-QEMU_ALIGN_DOWN(INT_MAX, s->granularity) >> BDRV_SECTOR_BITS);
+for (offset = 0; offset < s->bdev_length; ) {
+int bytes = MIN(s->bdev_length - offset,
+QEMU_ALIGN_DOWN(INT_MAX, s->granularity));

 mirror_throttle(s);

@@ -647,9 +645,8 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
 continue;
 }

-mirror_do_zero_or_discard(s, sector_num * BDRV_SECTOR_SIZE,
-  nb_sectors * BDRV_SECTOR_SIZE, false);
-sector_num += nb_sectors;
+mirror_do_zero_or_discard(s, offset, bytes, false);
+offset += bytes;
 }

 mirror_wait_for_all_io(s);
@@ -657,10 +654,10 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob 
*s)
 }

 /* First part, loop on the sectors and initialize the dirty bitmap.  */
-for (sector_num = 0; sector_num < end; ) {
+for (offset = 0; offset < s->bdev_length; ) {
 /* Just to make sure we are not exceeding int limit. */
-int nb_sectors = MIN(INT_MAX >> BDRV_SECTOR_BITS,
- end - sector_num);
+int bytes = MIN(s->bdev_length - offset,
+QEMU_ALIGN_DOWN(INT_MAX, s->granularity));

 mirror_throttle(s);

@@ -668,23 +665,16 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob 
*s)
 return 0;
 }

-ret = bdrv_is_allocated_above(bs, base, sector_num * BDRV_SECTOR_SIZE,
-  nb_sectors * BDRV_SECTOR_SIZE, );
+ret = bdrv_is_allocated_above(bs, base, offset, bytes, );
 if (ret < 0) {
 return ret;
 }

-/* TODO: Relax this once bdrv_is_allocated_above and dirty
- * bitmaps no longer require sector alignment. */
-assert(QEMU_IS_ALIGNED(count, BDRV_SECTOR_SIZE));
-n = count >> BDRV_SECTOR_BITS;
-assert(n > 0);
+assert(count);
 if (ret == 1) {
-bdrv_set_dirty_bitmap(s->dirty_bitmap,
-  sector_num * BDRV_SECTOR_SIZE,
-  n * BDRV_SECTOR_SIZE);
+bdrv_set_dirty_bitmap(s->dirty_bitmap, offset, count);
 }
-sector_num += n;
+offset += count;
 }
 return 0;
 }
-- 
2.13.5

Re: [Qemu-devel] [PATCH 00/47] add missing entries in MAINTAINERS

[Philippe Mathieu-Daudé]

Hi Markus,

On 08/28/2017 08:28 AM, Markus Armbruster wrote:

Where do we stand with this series?


I was giving time to eventual adoptive maintainers until 2.11 opening :)

Regards,

Phil.

Re: [Qemu-devel] [PATCH v6 00/10] qemu.py: Pylint/style fixes

[Eduardo Habkost]

On Fri, Aug 18, 2017 at 04:26:03PM +0200, Lukáš Doktor wrote:
> Hello guys,
> 
> I'm reading the available python modules to exercise qemu and while reading 
> them
> I fixed some issues that caught my attention. It usually starts with a simple
> pylint/docstring fixes and slowly graduates to more controversial ones so I'm
> open to suggestion to remove some of them.
> 
> Kind regards,
> Lukáš

I'm queueing this series on a python-next branch at:

  https://github.com/ehabkost/qemu.git python-next

-- 
Eduardo

[Qemu-devel] [PATCH v6 12/18] dirty-bitmap: Change bdrv_[re]set_dirty_bitmap() to use bytes

[Eric Blake]

Some of the callers were already scaling bytes to sectors; others
can be easily converted to pass byte offsets, all in our shift
towards a consistent byte interface everywhere.  Making the change
will also make it easier to write the hold-out callers to use byte
rather than sectors for their iterations; it also makes it easier
for a future dirty-bitmap patch to offload scaling over to the
internal hbitmap.  Although all callers happen to pass
sector-aligned values, make the internal scaling robust to any
sub-sector requests.

Signed-off-by: Eric Blake 
Reviewed-by: John Snow 

---
v5: only context change
v4: only context change, due to rebasing to persistent bitmaps
v3: rebase to addition of _locked interfaces; complex enough that I
dropped R-b
v2: no change
---
 include/block/dirty-bitmap.h |  8 
 block/dirty-bitmap.c | 22 ++
 block/mirror.c   | 16 
 migration/block.c|  7 +--
 4 files changed, 31 insertions(+), 22 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 1dddcd320b..0341a605d7 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -40,9 +40,9 @@ const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap 
*bitmap);
 int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap);
 DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
 void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
-   int64_t cur_sector, int64_t nr_sectors);
+   int64_t offset, int64_t bytes);
 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
- int64_t cur_sector, int64_t nr_sectors);
+ int64_t offset, int64_t bytes);
 BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
 BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
@@ -75,9 +75,9 @@ void bdrv_dirty_bitmap_unlock(BdrvDirtyBitmap *bitmap);
 bool bdrv_get_dirty_locked(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t offset);
 void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
-  int64_t cur_sector, int64_t nr_sectors);
+  int64_t offset, int64_t bytes);
 void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
-int64_t cur_sector, int64_t nr_sectors);
+int64_t offset, int64_t bytes);
 int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
 void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t offset);
 int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 8b3c0221c6..9821225523 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -510,35 +510,41 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)

 /* Called within bdrv_dirty_bitmap_lock..unlock */
 void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
-  int64_t cur_sector, int64_t nr_sectors)
+  int64_t offset, int64_t bytes)
 {
+int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
+
 assert(bdrv_dirty_bitmap_enabled(bitmap));
 assert(!bdrv_dirty_bitmap_readonly(bitmap));
-hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
+hbitmap_set(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
+end_sector - (offset >> BDRV_SECTOR_BITS));
 }

 void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
-   int64_t cur_sector, int64_t nr_sectors)
+   int64_t offset, int64_t bytes)
 {
 bdrv_dirty_bitmap_lock(bitmap);
-bdrv_set_dirty_bitmap_locked(bitmap, cur_sector, nr_sectors);
+bdrv_set_dirty_bitmap_locked(bitmap, offset, bytes);
 bdrv_dirty_bitmap_unlock(bitmap);
 }

 /* Called within bdrv_dirty_bitmap_lock..unlock */
 void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
-int64_t cur_sector, int64_t nr_sectors)
+int64_t offset, int64_t bytes)
 {
+int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
+
 assert(bdrv_dirty_bitmap_enabled(bitmap));
 assert(!bdrv_dirty_bitmap_readonly(bitmap));
-hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
+hbitmap_reset(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
+  end_sector - (offset >> BDRV_SECTOR_BITS));
 }

 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
- int64_t cur_sector, int64_t nr_sectors)
+ int64_t offset, int64_t bytes)
 {
 bdrv_dirty_bitmap_lock(bitmap);
-bdrv_reset_dirty_bitmap_locked(bitmap, cur_sector, nr_sectors);
+

Re: [Qemu-devel] [RFC PATCH 01/47] MAINTAINERS: add missing entry for documentation

[Philippe Mathieu-Daudé]

On 07/28/2017 03:47 AM, Thomas Huth wrote:

On 28.07.2017 07:35, Philippe Mathieu-Daudé wrote:

Signed-off-by: Philippe Mathieu-Daudé 
---

because having only a "Build system architecture" entry in Documentation seems
odd to me, so RFC.

  MAINTAINERS | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5ea273f899..972118e70b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1886,6 +1886,9 @@ W: http://patchew.org/QEMU/
  
  Documentation

  -
+Overall
+F: docs/


But it also does not make sense to add such an entry without a "M:"
line, does it?


Indeed, dropped.

Re: [Qemu-devel] [PATCH v7 04/11] qemu.py: use poll() instead of 'returncode'

[Eduardo Habkost]

On Fri, Aug 18, 2017 at 07:05:19PM +0200, Amador Pahim wrote:
> The 'returncode' Popen attribute is not guaranteed to be updated. It
> actually depends on a call to either poll(), wait() or communicate().
> 
> On the other hand, poll() will: "Check if child process has terminated.
> Set and return returncode attribute."
> 
> Let's use the poll() to check whether the process is running and also
> to get the updated process exit code, when the process is finished.
> 
> As consequence, the shutdown() calls to self._load_io_log()
> self._post_shutdown() are now always executed to make sure we cleanup
> even if VM is not running when the shutdown() is called.

I suggest moving the shutdown() changes to a separate patch.

> 
> Signed-off-by: Amador Pahim 
> ---
>  scripts/qemu.py | 19 +--
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 51545f7f97..774f971f69 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -98,12 +98,12 @@ class QEMUMachine(object):
>  raise
>  
>  def is_running(self):
> -return self._popen is not None and self._popen.returncode is None
> +return self._popen is not None and self._popen.poll() is None
>  
>  def exitcode(self):
>  if self._popen is None:
>  return None
> -return self._popen.returncode
> +return self._popen.poll()
>  
>  def get_pid(self):
>  if not self.is_running():
> @@ -111,8 +111,15 @@ class QEMUMachine(object):
>  return self._popen.pid
>  
>  def _load_io_log(self):
> -with open(self._qemu_log_path, "r") as fh:
> -self._iolog = fh.read()
> +'''
> +Load the Qemu log file content (if available) into the proper
> +instance variable
> +'''
> +try:
> +with open(self._qemu_log_path, "r") as fh:
> +self._iolog = fh.read()
> +except IOError:
> +pass

Is this really supposed to be included in this patch?  This
change in the _load_io_log() behavior is the most dangerous part
of this patch, and not described in the commit message at all.

I don't think it's a good idea to remove errors unconditionally
here.  The launch() code controls of the creation of the log file
completely, we don't even need to try to load the file if we know
it was not created yet.


>  
>  def _base_args(self):
>  if isinstance(self._monitor_address, tuple):
> @@ -168,8 +175,8 @@ class QEMUMachine(object):
>  if exitcode < 0:
>  LOG.debug('qemu received signal %i: %s', -exitcode,
>' '.join(self._args))
> -self._load_io_log()
> -self._post_shutdown()
> +self._load_io_log()
> +self._post_shutdown()
>  
>  underscore_to_dash = string.maketrans('_', '-')
>  def qmp(self, cmd, conv_keys=True, **args):
> -- 
> 2.13.5
> 
> 

-- 
Eduardo

[Qemu-devel] [PATCH v6 08/18] dirty-bitmap: Set iterator start by offset, not sector

[Eric Blake]

All callers to bdrv_dirty_iter_new() passed 0 for their initial
starting point, drop that parameter.

Most callers to bdrv_set_dirty_iter() were scaling a byte offset to
a sector number; the exception qcow2-bitmap will be converted later
to use byte rather than sector iteration.  Move the scaling to occur
internally to dirty bitmap code instead, so that callers now pass
in bytes.

Signed-off-by: Eric Blake 
Reviewed-by: John Snow 

---
v5: no change
v4: rebase to persistent bitmaps
v3: no change
v2: no change
---
 include/block/dirty-bitmap.h | 5 ++---
 block/backup.c   | 5 ++---
 block/dirty-bitmap.c | 9 -
 block/mirror.c   | 4 ++--
 block/qcow2-bitmap.c | 4 ++--
 5 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index f4ccd3f33f..ece28e1edc 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -44,8 +44,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
  int64_t cur_sector, int64_t nr_sectors);
 BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
-BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
- uint64_t first_sector);
+BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);

 uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap,
@@ -80,7 +79,7 @@ void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
 void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
 int64_t cur_sector, int64_t nr_sectors);
 int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
-void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);
+void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t offset);
 int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
 int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
diff --git a/block/backup.c b/block/backup.c
index 504a089150..1a6ec7d079 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -372,7 +372,7 @@ static int coroutine_fn 
backup_run_incremental(BackupBlockJob *job)

 granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
 clusters_per_iter = MAX((granularity / job->cluster_size), 1);
-dbi = bdrv_dirty_iter_new(job->sync_bitmap, 0);
+dbi = bdrv_dirty_iter_new(job->sync_bitmap);

 /* Find the next dirty sector(s) */
 while ((offset = bdrv_dirty_iter_next(dbi) * BDRV_SECTOR_SIZE) >= 0) {
@@ -403,8 +403,7 @@ static int coroutine_fn 
backup_run_incremental(BackupBlockJob *job)
 /* If the bitmap granularity is smaller than the backup granularity,
  * we need to advance the iterator pointer to the next cluster. */
 if (granularity < job->cluster_size) {
-bdrv_set_dirty_iter(dbi,
-cluster * job->cluster_size / 
BDRV_SECTOR_SIZE);
+bdrv_set_dirty_iter(dbi, cluster * job->cluster_size);
 }

 last_cluster = cluster - 1;
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 034effc8cd..c091f0c7ba 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -475,11 +475,10 @@ uint32_t bdrv_dirty_bitmap_granularity(const 
BdrvDirtyBitmap *bitmap)
 return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
 }

-BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
- uint64_t first_sector)
+BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap)
 {
 BdrvDirtyBitmapIter *iter = g_new(BdrvDirtyBitmapIter, 1);
-hbitmap_iter_init(>hbi, bitmap->bitmap, first_sector);
+hbitmap_iter_init(>hbi, bitmap->bitmap, 0);
 iter->bitmap = bitmap;
 bitmap->active_iterators++;
 return iter;
@@ -647,9 +646,9 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t 
cur_sector,
 /**
  * Advance a BdrvDirtyBitmapIter to an arbitrary offset.
  */
-void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t sector_num)
+void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t offset)
 {
-hbitmap_iter_init(>hbi, iter->hbi.hb, sector_num);
+hbitmap_iter_init(>hbi, iter->hbi.hb, offset >> BDRV_SECTOR_BITS);
 }

 int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
diff --git a/block/mirror.c b/block/mirror.c
index 429751b9fe..51824750ac 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -373,7 +373,7 @@ static uint64_t coroutine_fn 
mirror_iteration(MirrorBlockJob *s)
 next_dirty = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
 if (next_dirty > next_offset || next_dirty < 0) {
 /* The bitmap iterator's cache is stale, refresh it */
-bdrv_set_dirty_iter(s->dbi, next_offset >> 

[Qemu-devel] [PATCH v6 11/18] dirty-bitmap: Change bdrv_get_dirty_locked() to take bytes

[Eric Blake]

Half the callers were already scaling bytes to sectors; the other
half can eventually be simplified to use byte iteration.  Both
callers were already using the result as a bool, so make that
explicit.  Making the change also makes it easier for a future
dirty-bitmap patch to offload scaling over to the internal hbitmap.

Remember, asking whether a byte is dirty is effectively asking
whether the entire granularity containing the byte is dirty, since
we only track dirtiness by granularity.

Signed-off-by: Eric Blake 
Reviewed-by: John Snow 
Reviewed-by: Juan Quintela 

---
v4: only context change
v3: rebase to _locked rename was straightforward enough that R-b kept
v2: tweak commit message, no code change
---
 include/block/dirty-bitmap.h | 4 ++--
 block/dirty-bitmap.c | 8 
 block/mirror.c   | 3 +--
 migration/block.c| 3 ++-
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index ece28e1edc..1dddcd320b 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -72,8 +72,8 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap 
*bitmap,
 /* Functions that require manual locking.  */
 void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_unlock(BdrvDirtyBitmap *bitmap);
-int bdrv_get_dirty_locked(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
-  int64_t sector);
+bool bdrv_get_dirty_locked(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+   int64_t offset);
 void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
   int64_t cur_sector, int64_t nr_sectors);
 void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index f983d99def..8b3c0221c6 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -440,13 +440,13 @@ BlockDirtyInfoList 
*bdrv_query_dirty_bitmaps(BlockDriverState *bs)
 }

 /* Called within bdrv_dirty_bitmap_lock..unlock */
-int bdrv_get_dirty_locked(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
-  int64_t sector)
+bool bdrv_get_dirty_locked(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+   int64_t offset)
 {
 if (bitmap) {
-return hbitmap_get(bitmap->bitmap, sector);
+return hbitmap_get(bitmap->bitmap, offset >> BDRV_SECTOR_BITS);
 } else {
-return 0;
+return false;
 }
 }

diff --git a/block/mirror.c b/block/mirror.c
index cc47e21814..42888ebd04 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -362,8 +362,7 @@ static uint64_t coroutine_fn 
mirror_iteration(MirrorBlockJob *s)
 int64_t next_offset = offset + nb_chunks * s->granularity;
 int64_t next_chunk = next_offset / s->granularity;
 if (next_offset >= s->bdev_length ||
-!bdrv_get_dirty_locked(source, s->dirty_bitmap,
-   next_offset >> BDRV_SECTOR_BITS)) {
+!bdrv_get_dirty_locked(source, s->dirty_bitmap, next_offset)) {
 break;
 }
 if (test_bit(next_chunk, s->in_flight_bitmap)) {
diff --git a/migration/block.c b/migration/block.c
index a3512945da..b618869661 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -530,7 +530,8 @@ static int mig_save_device_dirty(QEMUFile *f, 
BlkMigDevState *bmds,
 blk_mig_unlock();
 }
 bdrv_dirty_bitmap_lock(bmds->dirty_bitmap);
-if (bdrv_get_dirty_locked(bs, bmds->dirty_bitmap, sector)) {
+if (bdrv_get_dirty_locked(bs, bmds->dirty_bitmap,
+  sector * BDRV_SECTOR_SIZE)) {
 if (total_sectors - sector < BDRV_SECTORS_PER_DIRTY_CHUNK) {
 nr_sectors = total_sectors - sector;
 } else {
-- 
2.13.5

[Qemu-devel] [PATCH v6 18/18] dirty-bitmap: Convert internal hbitmap size/granularity

[Eric Blake]

Now that all callers are using byte-based interfaces, there's no
reason for our internal hbitmap to remain with sector-based
granularity.  It also simplifies our internal scaling, since we
already know that hbitmap widens requests out to granularity
boundaries.

Signed-off-by: Eric Blake 
Reviewed-by: John Snow 

---
v6: no change
v5: fix bdrv_dirty_bitmap_truncate [John]
v4: rebase to earlier changes, include serialization, R-b dropped
v3: no change
v2: no change
---
 block/dirty-bitmap.c | 61 
 1 file changed, 18 insertions(+), 43 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index b54eed46e4..0b349f0b5a 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -38,7 +38,7 @@
  */
 struct BdrvDirtyBitmap {
 QemuMutex *mutex;
-HBitmap *bitmap;/* Dirty sector bitmap implementation */
+HBitmap *bitmap;/* Dirty bitmap implementation */
 HBitmap *meta;  /* Meta dirty bitmap */
 BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
 char *name; /* Optional non-empty unique ID */
@@ -130,12 +130,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState 
*bs,
 }
 bitmap = g_new0(BdrvDirtyBitmap, 1);
 bitmap->mutex = >dirty_bitmap_mutex;
-/*
- * TODO - let hbitmap track full granularity. For now, it is tracking
- * only sector granularity, as a shortcut for our iterators.
- */
-bitmap->bitmap = hbitmap_alloc(DIV_ROUND_UP(bitmap_size, BDRV_SECTOR_SIZE),
-   ctz32(granularity) - BDRV_SECTOR_BITS);
+bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(granularity));
 bitmap->size = bitmap_size;
 bitmap->name = g_strdup(name);
 bitmap->disabled = false;
@@ -314,7 +309,7 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
 QLIST_FOREACH(bitmap, >dirty_bitmaps, list) {
 assert(!bdrv_dirty_bitmap_frozen(bitmap));
 assert(!bitmap->active_iterators);
-hbitmap_truncate(bitmap->bitmap, DIV_ROUND_UP(size, BDRV_SECTOR_SIZE));
+hbitmap_truncate(bitmap->bitmap, size);
 bitmap->size = size;
 }
 bdrv_dirty_bitmaps_unlock(bs);
@@ -444,7 +439,7 @@ bool bdrv_get_dirty_locked(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap,
int64_t offset)
 {
 if (bitmap) {
-return hbitmap_get(bitmap->bitmap, offset >> BDRV_SECTOR_BITS);
+return hbitmap_get(bitmap->bitmap, offset);
 } else {
 return false;
 }
@@ -472,7 +467,7 @@ uint32_t 
bdrv_get_default_bitmap_granularity(BlockDriverState *bs)

 uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap)
 {
-return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
+return 1U << hbitmap_granularity(bitmap->bitmap);
 }

 BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap)
@@ -505,19 +500,16 @@ void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter)

 int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
 {
-return hbitmap_iter_next(>hbi) * BDRV_SECTOR_SIZE;
+return hbitmap_iter_next(>hbi);
 }

 /* Called within bdrv_dirty_bitmap_lock..unlock */
 void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
   int64_t offset, int64_t bytes)
 {
-int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
-
 assert(bdrv_dirty_bitmap_enabled(bitmap));
 assert(!bdrv_dirty_bitmap_readonly(bitmap));
-hbitmap_set(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
-end_sector - (offset >> BDRV_SECTOR_BITS));
+hbitmap_set(bitmap->bitmap, offset, bytes);
 }

 void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
@@ -532,12 +524,9 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
 void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
 int64_t offset, int64_t bytes)
 {
-int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
-
 assert(bdrv_dirty_bitmap_enabled(bitmap));
 assert(!bdrv_dirty_bitmap_readonly(bitmap));
-hbitmap_reset(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
-  end_sector - (offset >> BDRV_SECTOR_BITS));
+hbitmap_reset(bitmap->bitmap, offset, bytes);
 }

 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
@@ -557,8 +546,7 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, 
HBitmap **out)
 hbitmap_reset_all(bitmap->bitmap);
 } else {
 HBitmap *backup = bitmap->bitmap;
-bitmap->bitmap = hbitmap_alloc(DIV_ROUND_UP(bitmap->size,
-BDRV_SECTOR_SIZE),
+bitmap->bitmap = hbitmap_alloc(bitmap->size,
hbitmap_granularity(backup));
 *out = backup;
 }
@@ -577,51 +565,40 @@ void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap 
*bitmap, HBitmap *in)
 

[Qemu-devel] [PATCH v6 15/18] qcow2: Switch load_bitmap_data() to byte-based iteration

[Eric Blake]

Now that we have adjusted the majority of the calls this function
makes to be byte-based, it is easier to read the code if it makes
passes over the image using bytes rather than sectors.

Signed-off-by: Eric Blake 
Reviewed-by: John Snow 
Reviewed-by: Vladimir Sementsov-Ogievskiy 

---
v5: no change
v4: new patch
---
 block/qcow2-bitmap.c | 22 --
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index c7c60dfca2..b807298484 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -291,9 +291,8 @@ static int load_bitmap_data(BlockDriverState *bs,
 {
 int ret = 0;
 BDRVQcow2State *s = bs->opaque;
-uint64_t sector, limit, sbc;
+uint64_t offset, limit;
 uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
-uint64_t bm_sectors = DIV_ROUND_UP(bm_size, BDRV_SECTOR_SIZE);
 uint8_t *buf = NULL;
 uint64_t i, tab_size =
 size_to_clusters(s,
@@ -305,32 +304,27 @@ static int load_bitmap_data(BlockDriverState *bs,

 buf = g_malloc(s->cluster_size);
 limit = bytes_covered_by_bitmap_cluster(s, bitmap);
-sbc = limit >> BDRV_SECTOR_BITS;
-for (i = 0, sector = 0; i < tab_size; ++i, sector += sbc) {
-uint64_t count = MIN(bm_sectors - sector, sbc);
+for (i = 0, offset = 0; i < tab_size; ++i, offset += limit) {
+uint64_t count = MIN(bm_size - offset, limit);
 uint64_t entry = bitmap_table[i];
-uint64_t offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
+uint64_t data_offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;

 assert(check_table_entry(entry, s->cluster_size) == 0);

-if (offset == 0) {
+if (data_offset == 0) {
 if (entry & BME_TABLE_ENTRY_FLAG_ALL_ONES) {
-bdrv_dirty_bitmap_deserialize_ones(bitmap,
-   sector * BDRV_SECTOR_SIZE,
-   count * BDRV_SECTOR_SIZE,
+bdrv_dirty_bitmap_deserialize_ones(bitmap, offset, count,
false);
 } else {
 /* No need to deserialize zeros because the dirty bitmap is
  * already cleared */
 }
 } else {
-ret = bdrv_pread(bs->file, offset, buf, s->cluster_size);
+ret = bdrv_pread(bs->file, data_offset, buf, s->cluster_size);
 if (ret < 0) {
 goto finish;
 }
-bdrv_dirty_bitmap_deserialize_part(bitmap, buf,
-   sector * BDRV_SECTOR_SIZE,
-   count * BDRV_SECTOR_SIZE,
+bdrv_dirty_bitmap_deserialize_part(bitmap, buf, offset, count,
false);
 }
 }
-- 
2.13.5

[Qemu-devel] [PATCH v6 09/18] dirty-bitmap: Change bdrv_dirty_iter_next() to report byte offset

[Eric Blake]

Thanks to recent cleanups, most callers were scaling a return value
of sectors into bytes (the exception, in qcow2-bitmap, will be
converted to byte-based iteration later).  Update the interface to
do the scaling internally instead.

Signed-off-by: Eric Blake 
Reviewed-By: John Snow 

---
v5: no change
v4: rebase to persistent bitmap
v3: no change
v2: no change
---
 block/backup.c   | 2 +-
 block/dirty-bitmap.c | 2 +-
 block/mirror.c   | 8 
 block/qcow2-bitmap.c | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 1a6ec7d079..f53cde90d6 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -375,7 +375,7 @@ static int coroutine_fn 
backup_run_incremental(BackupBlockJob *job)
 dbi = bdrv_dirty_iter_new(job->sync_bitmap);

 /* Find the next dirty sector(s) */
-while ((offset = bdrv_dirty_iter_next(dbi) * BDRV_SECTOR_SIZE) >= 0) {
+while ((offset = bdrv_dirty_iter_next(dbi)) >= 0) {
 cluster = offset / job->cluster_size;

 /* Fake progress updates for any clusters we skipped */
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index c091f0c7ba..20f230867d 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -505,7 +505,7 @@ void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter)

 int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
 {
-return hbitmap_iter_next(>hbi);
+return hbitmap_iter_next(>hbi) * BDRV_SECTOR_SIZE;
 }

 /* Called within bdrv_dirty_bitmap_lock..unlock */
diff --git a/block/mirror.c b/block/mirror.c
index 51824750ac..af13f5d658 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -336,10 +336,10 @@ static uint64_t coroutine_fn 
mirror_iteration(MirrorBlockJob *s)
 int max_io_bytes = MAX(s->buf_size / MAX_IN_FLIGHT, MAX_IO_BYTES);

 bdrv_dirty_bitmap_lock(s->dirty_bitmap);
-offset = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
+offset = bdrv_dirty_iter_next(s->dbi);
 if (offset < 0) {
 bdrv_set_dirty_iter(s->dbi, 0);
-offset = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
+offset = bdrv_dirty_iter_next(s->dbi);
 trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap) *
   BDRV_SECTOR_SIZE);
 assert(offset >= 0);
@@ -370,11 +370,11 @@ static uint64_t coroutine_fn 
mirror_iteration(MirrorBlockJob *s)
 break;
 }

-next_dirty = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
+next_dirty = bdrv_dirty_iter_next(s->dbi);
 if (next_dirty > next_offset || next_dirty < 0) {
 /* The bitmap iterator's cache is stale, refresh it */
 bdrv_set_dirty_iter(s->dbi, next_offset);
-next_dirty = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
+next_dirty = bdrv_dirty_iter_next(s->dbi);
 }
 assert(next_dirty == next_offset);
 nb_chunks++;
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 44329fc74f..c7c60dfca2 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1109,7 +1109,7 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
 sbc = limit >> BDRV_SECTOR_BITS;
 assert(DIV_ROUND_UP(bm_size, limit) == tb_size);

-while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
+while ((sector = bdrv_dirty_iter_next(dbi) >> BDRV_SECTOR_BITS) != -1) {
 uint64_t cluster = sector / sbc;
 uint64_t end, write_size;
 int64_t off;
-- 
2.13.5

[Qemu-devel] [PATCH v6 10/18] dirty-bitmap: Change bdrv_get_dirty_count() to report bytes

[Eric Blake]

Thanks to recent cleanups, all callers were scaling a return value
of sectors into bytes; do the scaling internally instead.

Signed-off-by: Eric Blake 
Reviewed-by: John Snow 
Reviewed-by: Juan Quintela 

---
v4: no change
v3: no change, add R-b
v2: no change
---
 block/dirty-bitmap.c |  4 ++--
 block/mirror.c   | 13 +
 migration/block.c|  2 +-
 3 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 20f230867d..f983d99def 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -425,7 +425,7 @@ BlockDirtyInfoList 
*bdrv_query_dirty_bitmaps(BlockDriverState *bs)
 QLIST_FOREACH(bm, >dirty_bitmaps, list) {
 BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
 BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
-info->count = bdrv_get_dirty_count(bm) << BDRV_SECTOR_BITS;
+info->count = bdrv_get_dirty_count(bm);
 info->granularity = bdrv_dirty_bitmap_granularity(bm);
 info->has_name = !!bm->name;
 info->name = g_strdup(bm->name);
@@ -653,7 +653,7 @@ void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t 
offset)

 int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
 {
-return hbitmap_count(bitmap->bitmap);
+return hbitmap_count(bitmap->bitmap) << BDRV_SECTOR_BITS;
 }

 int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
diff --git a/block/mirror.c b/block/mirror.c
index af13f5d658..cc47e21814 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -811,11 +811,10 @@ static void coroutine_fn mirror_run(void *opaque)

 cnt = bdrv_get_dirty_count(s->dirty_bitmap);
 /* s->common.offset contains the number of bytes already processed so
- * far, cnt is the number of dirty sectors remaining and
+ * far, cnt is the number of dirty bytes remaining and
  * s->bytes_in_flight is the number of bytes currently being
  * processed; together those are the current total operation length */
-s->common.len = s->common.offset + s->bytes_in_flight +
-cnt * BDRV_SECTOR_SIZE;
+s->common.len = s->common.offset + s->bytes_in_flight + cnt;

 /* Note that even when no rate limit is applied we need to yield
  * periodically with no pending I/O so that bdrv_drain_all() returns.
@@ -827,8 +826,7 @@ static void coroutine_fn mirror_run(void *opaque)
 s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
 if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
 (cnt == 0 && s->in_flight > 0)) {
-trace_mirror_yield(s, cnt * BDRV_SECTOR_SIZE,
-   s->buf_free_count, s->in_flight);
+trace_mirror_yield(s, cnt, s->buf_free_count, s->in_flight);
 mirror_wait_for_io(s);
 continue;
 } else if (cnt != 0) {
@@ -869,7 +867,7 @@ static void coroutine_fn mirror_run(void *opaque)
  * whether to switch to target check one last time if I/O has
  * come in the meanwhile, and if not flush the data to disk.
  */
-trace_mirror_before_drain(s, cnt * BDRV_SECTOR_SIZE);
+trace_mirror_before_drain(s, cnt);

 bdrv_drained_begin(bs);
 cnt = bdrv_get_dirty_count(s->dirty_bitmap);
@@ -888,8 +886,7 @@ static void coroutine_fn mirror_run(void *opaque)
 }

 ret = 0;
-trace_mirror_before_sleep(s, cnt * BDRV_SECTOR_SIZE,
-  s->synced, delay_ns);
+trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
 if (!s->synced) {
 block_job_sleep_ns(>common, QEMU_CLOCK_REALTIME, delay_ns);
 if (block_job_is_cancelled(>common)) {
diff --git a/migration/block.c b/migration/block.c
index 9171f60028..a3512945da 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -667,7 +667,7 @@ static int64_t get_remaining_dirty(void)
 aio_context_release(blk_get_aio_context(bmds->blk));
 }

-return dirty << BDRV_SECTOR_BITS;
+return dirty;
 }


-- 
2.13.5

[Qemu-devel] [PATCH v6 16/18] qcow2: Switch store_bitmap_data() to byte-based iteration

[Eric Blake]

Now that we have adjusted the majority of the calls this function
makes to be byte-based, it is easier to read the code if it makes
passes over the image using bytes rather than sectors.

Signed-off-by: Eric Blake 
Reviewed-by: John Snow 
Reviewed-by: Vladimir Sementsov-Ogievskiy 

---
v5: no change
v4: new patch
---
 block/qcow2-bitmap.c | 26 +++---
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b807298484..63d845e35f 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1072,10 +1072,9 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
 {
 int ret;
 BDRVQcow2State *s = bs->opaque;
-int64_t sector;
-uint64_t limit, sbc;
+int64_t offset;
+uint64_t limit;
 uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
-uint64_t bm_sectors = DIV_ROUND_UP(bm_size, BDRV_SECTOR_SIZE);
 const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
 uint8_t *buf = NULL;
 BdrvDirtyBitmapIter *dbi;
@@ -1100,18 +1099,17 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
 dbi = bdrv_dirty_iter_new(bitmap);
 buf = g_malloc(s->cluster_size);
 limit = bytes_covered_by_bitmap_cluster(s, bitmap);
-sbc = limit >> BDRV_SECTOR_BITS;
 assert(DIV_ROUND_UP(bm_size, limit) == tb_size);

-while ((sector = bdrv_dirty_iter_next(dbi) >> BDRV_SECTOR_BITS) != -1) {
-uint64_t cluster = sector / sbc;
+while ((offset = bdrv_dirty_iter_next(dbi)) != -1) {
+uint64_t cluster = offset / limit;
 uint64_t end, write_size;
 int64_t off;

-sector = cluster * sbc;
-end = MIN(bm_sectors, sector + sbc);
-write_size = bdrv_dirty_bitmap_serialization_size(bitmap,
-sector * BDRV_SECTOR_SIZE, (end - sector) * BDRV_SECTOR_SIZE);
+offset = cluster * limit;
+end = MIN(bm_size, offset + limit);
+write_size = bdrv_dirty_bitmap_serialization_size(bitmap, offset,
+  end - offset);
 assert(write_size <= s->cluster_size);

 off = qcow2_alloc_clusters(bs, s->cluster_size);
@@ -1123,9 +1121,7 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
 }
 tb[cluster] = off;

-bdrv_dirty_bitmap_serialize_part(bitmap, buf,
- sector * BDRV_SECTOR_SIZE,
- (end - sector) * BDRV_SECTOR_SIZE);
+bdrv_dirty_bitmap_serialize_part(bitmap, buf, offset, end - offset);
 if (write_size < s->cluster_size) {
 memset(buf + write_size, 0, s->cluster_size - write_size);
 }
@@ -1143,11 +1139,11 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
 goto fail;
 }

-if (end >= bm_sectors) {
+if (end >= bm_size) {
 break;
 }

-bdrv_set_dirty_iter(dbi, end * BDRV_SECTOR_SIZE);
+bdrv_set_dirty_iter(dbi, end);
 }

 *bitmap_table_size = tb_size;
-- 
2.13.5

[Qemu-devel] [PATCH v6 04/18] dirty-bitmap: Drop unused functions

[Eric Blake]

We had several functions that no one is currently using, and which
use sector-based interfaces.  I'm trying to convert towards byte-based
interfaces, so it's easier to just drop the unused functions:

bdrv_dirty_bitmap_get_meta
bdrv_dirty_bitmap_get_meta_locked
bdrv_dirty_bitmap_reset_meta
bdrv_dirty_bitmap_meta_granularity

Signed-off-by: Eric Blake 
Reviewed-by: John Snow 

---
v5: no change
v4: rebase to Vladimir's persistent bitmaps (bdrv_dirty_bitmap_size now
in use), dropped R-b
v3: rebase to upstream changes (bdrv_dirty_bitmap_get_meta_locked was
added in b64bd51e with no clients), kept R-b
v2: tweak commit message based on review, no code change
---
 include/block/dirty-bitmap.h | 10 --
 block/dirty-bitmap.c | 44 
 2 files changed, 54 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index a79a58d2c3..8fd842eac9 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -34,7 +34,6 @@ void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
 uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
 uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap);
-uint32_t bdrv_dirty_bitmap_meta_granularity(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
 const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap);
@@ -44,15 +43,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int64_t nr_sectors);
 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
  int64_t cur_sector, int64_t nr_sectors);
-int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
-   BdrvDirtyBitmap *bitmap, int64_t sector,
-   int nb_sectors);
-int bdrv_dirty_bitmap_get_meta_locked(BlockDriverState *bs,
-  BdrvDirtyBitmap *bitmap, int64_t sector,
-  int nb_sectors);
-void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs,
-  BdrvDirtyBitmap *bitmap, int64_t sector,
-  int nb_sectors);
 BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
 BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
  uint64_t first_sector);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 0490ca3aff..42a55e4a4b 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -173,45 +173,6 @@ void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap 
*bitmap)
 qemu_mutex_unlock(bitmap->mutex);
 }

-int bdrv_dirty_bitmap_get_meta_locked(BlockDriverState *bs,
-  BdrvDirtyBitmap *bitmap, int64_t sector,
-  int nb_sectors)
-{
-uint64_t i;
-int sectors_per_bit = 1 << hbitmap_granularity(bitmap->meta);
-
-/* To optimize: we can make hbitmap to internally check the range in a
- * coarse level, or at least do it word by word. */
-for (i = sector; i < sector + nb_sectors; i += sectors_per_bit) {
-if (hbitmap_get(bitmap->meta, i)) {
-return true;
-}
-}
-return false;
-}
-
-int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
-   BdrvDirtyBitmap *bitmap, int64_t sector,
-   int nb_sectors)
-{
-bool dirty;
-
-qemu_mutex_lock(bitmap->mutex);
-dirty = bdrv_dirty_bitmap_get_meta_locked(bs, bitmap, sector, nb_sectors);
-qemu_mutex_unlock(bitmap->mutex);
-
-return dirty;
-}
-
-void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs,
-  BdrvDirtyBitmap *bitmap, int64_t sector,
-  int nb_sectors)
-{
-qemu_mutex_lock(bitmap->mutex);
-hbitmap_reset(bitmap->meta, sector, nb_sectors);
-qemu_mutex_unlock(bitmap->mutex);
-}
-
 int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap)
 {
 return bitmap->size;
@@ -511,11 +472,6 @@ uint32_t bdrv_dirty_bitmap_granularity(const 
BdrvDirtyBitmap *bitmap)
 return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
 }

-uint32_t bdrv_dirty_bitmap_meta_granularity(BdrvDirtyBitmap *bitmap)
-{
-return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->meta);
-}
-
 BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
  uint64_t first_sector)
 {
-- 
2.13.5

[Qemu-devel] [PATCH v6 06/18] dirty-bitmap: Change bdrv_dirty_bitmap_*serialize*() to take bytes

[Eric Blake]

Right now, the dirty-bitmap code exposes the fact that we use
a scale of sector granularity in the underlying hbitmap to anything
that wants to serialize a dirty bitmap.  It's nicer to uniformly
expose bytes as our dirty-bitmap interface, matching the previous
change to bitmap size.  The only caller to serialization is currently
qcow2-cluster.c, which becomes a bit more verbose because it is still
tracking sectors for other reasons, but a later patch will fix that
to more uniformly use byte offsets everywhere.  Likewise, within
dirty-bitmap, we have to add more assertions that we are not
truncating incorrectly, which can go away once the internal hbitmap
is byte-based rather than sector-based.

Signed-off-by: Eric Blake 
Reviewed-by: John Snow 

---
v5: no change
v4: new patch
---
 include/block/dirty-bitmap.h | 14 +++---
 block/dirty-bitmap.c | 37 -
 block/qcow2-bitmap.c | 22 ++
 3 files changed, 45 insertions(+), 28 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 8fd842eac9..f4ccd3f33f 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -49,19 +49,19 @@ BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap 
*bitmap,
 void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);

 uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap,
-  uint64_t start, uint64_t count);
+  uint64_t offset, uint64_t bytes);
 uint64_t bdrv_dirty_bitmap_serialization_align(const BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_serialize_part(const BdrvDirtyBitmap *bitmap,
-  uint8_t *buf, uint64_t start,
-  uint64_t count);
+  uint8_t *buf, uint64_t offset,
+  uint64_t bytes);
 void bdrv_dirty_bitmap_deserialize_part(BdrvDirtyBitmap *bitmap,
-uint8_t *buf, uint64_t start,
-uint64_t count, bool finish);
+uint8_t *buf, uint64_t offset,
+uint64_t bytes, bool finish);
 void bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap *bitmap,
-  uint64_t start, uint64_t count,
+  uint64_t offset, uint64_t bytes,
   bool finish);
 void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
-uint64_t start, uint64_t count,
+uint64_t offset, uint64_t bytes,
 bool finish);
 void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index e65ec4f7ec..034effc8cd 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -570,42 +570,53 @@ void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap 
*bitmap, HBitmap *in)
 }

 uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap,
-  uint64_t start, uint64_t count)
+  uint64_t offset, uint64_t bytes)
 {
-return hbitmap_serialization_size(bitmap->bitmap, start, count);
+assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
+return hbitmap_serialization_size(bitmap->bitmap,
+  offset >> BDRV_SECTOR_BITS,
+  bytes >> BDRV_SECTOR_BITS);
 }

 uint64_t bdrv_dirty_bitmap_serialization_align(const BdrvDirtyBitmap *bitmap)
 {
-return hbitmap_serialization_align(bitmap->bitmap);
+return hbitmap_serialization_align(bitmap->bitmap) * BDRV_SECTOR_SIZE;
 }

 void bdrv_dirty_bitmap_serialize_part(const BdrvDirtyBitmap *bitmap,
-  uint8_t *buf, uint64_t start,
-  uint64_t count)
+  uint8_t *buf, uint64_t offset,
+  uint64_t bytes)
 {
-hbitmap_serialize_part(bitmap->bitmap, buf, start, count);
+assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
+hbitmap_serialize_part(bitmap->bitmap, buf, offset >> BDRV_SECTOR_BITS,
+   bytes >> BDRV_SECTOR_BITS);
 }

 void bdrv_dirty_bitmap_deserialize_part(BdrvDirtyBitmap *bitmap,
-uint8_t *buf, uint64_t start,
-uint64_t count, bool finish)
+uint8_t *buf, uint64_t offset,
+uint64_t bytes, bool finish)
 {
-

[Qemu-devel] [PATCH v6 14/18] qcow2: Switch qcow2_measure() to byte-based iteration

[Eric Blake]

This is new code, but it is easier to read if it makes passes over
the image using bytes rather than sectors (and will get easier in
the future when bdrv_get_block_status is converted to byte-based).

Signed-off-by: Eric Blake 
Reviewed-by: John Snow 

---
v6: separate bug fix to earlier patch
v5: new patch
---
 block/qcow2.c | 22 ++
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 40ba26c111..57e3c5e7d5 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3666,20 +3666,19 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, 
BlockDriverState *in_bs,
  */
 required = virtual_size;
 } else {
-int cluster_sectors = cluster_size / BDRV_SECTOR_SIZE;
-int64_t sector_num;
+int64_t offset;
 int pnum = 0;

-for (sector_num = 0;
- sector_num < ssize / BDRV_SECTOR_SIZE;
- sector_num += pnum) {
-int nb_sectors = MIN(ssize / BDRV_SECTOR_SIZE - sector_num,
- BDRV_REQUEST_MAX_SECTORS);
+for (offset = 0; offset < ssize;
+ offset += pnum * BDRV_SECTOR_SIZE) {
+int nb_sectors = MIN(ssize - offset,
+ INT_MAX) / BDRV_SECTOR_SIZE;
 BlockDriverState *file;
 int64_t ret;

 ret = bdrv_get_block_status_above(in_bs, NULL,
-  sector_num, nb_sectors,
+  offset >> BDRV_SECTOR_BITS,
+  nb_sectors,
   , );
 if (ret < 0) {
 error_setg_errno(_err, -ret,
@@ -3692,12 +3691,11 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, 
BlockDriverState *in_bs,
 } else if ((ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) ==
(BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) {
 /* Extend pnum to end of cluster for next iteration */
-pnum = ROUND_UP(sector_num + pnum, cluster_sectors) -
-   sector_num;
+pnum = (ROUND_UP(offset + pnum * BDRV_SECTOR_SIZE,
+ cluster_size) - offset) >> BDRV_SECTOR_BITS;

 /* Count clusters we've seen */
-required += (sector_num % cluster_sectors + pnum) *
-BDRV_SECTOR_SIZE;
+required += offset % cluster_size + pnum * 
BDRV_SECTOR_SIZE;
 }
 }
 }
-- 
2.13.5

[Qemu-devel] [PATCH v6 07/18] qcow2: Switch sectors_covered_by_bitmap_cluster() to byte-based

[Eric Blake]

We are gradually converting to byte-based interfaces, as they are
easier to reason about than sector-based.  Change the qcow2 bitmap
helper function sectors_covered_by_bitmap_cluster(), renaming it
to bytes_covered_by_bitmap_cluster() in the process.

Signed-off-by: Eric Blake 
Reviewed-by: John Snow 

---
v5: no change
v4: new patch
---
 block/qcow2-bitmap.c | 28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 92098bfa49..4475273d8c 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -269,18 +269,16 @@ static int free_bitmap_clusters(BlockDriverState *bs, 
Qcow2BitmapTable *tb)
 return 0;
 }

-/* This function returns the number of disk sectors covered by a single qcow2
- * cluster of bitmap data. */
-static uint64_t sectors_covered_by_bitmap_cluster(const BDRVQcow2State *s,
-  const BdrvDirtyBitmap 
*bitmap)
+/* Return the disk size covered by a single qcow2 cluster of bitmap data. */
+static uint64_t bytes_covered_by_bitmap_cluster(const BDRVQcow2State *s,
+const BdrvDirtyBitmap *bitmap)
 {
-uint64_t sector_granularity =
-bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
-uint64_t sbc = sector_granularity * (s->cluster_size << 3);
+uint64_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
+uint64_t limit = granularity * (s->cluster_size << 3);

-assert(QEMU_IS_ALIGNED(sbc << BDRV_SECTOR_BITS,
+assert(QEMU_IS_ALIGNED(limit,
bdrv_dirty_bitmap_serialization_align(bitmap)));
-return sbc;
+return limit;
 }

 /* load_bitmap_data
@@ -293,7 +291,7 @@ static int load_bitmap_data(BlockDriverState *bs,
 {
 int ret = 0;
 BDRVQcow2State *s = bs->opaque;
-uint64_t sector, sbc;
+uint64_t sector, limit, sbc;
 uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
 uint64_t bm_sectors = DIV_ROUND_UP(bm_size, BDRV_SECTOR_SIZE);
 uint8_t *buf = NULL;
@@ -306,7 +304,8 @@ static int load_bitmap_data(BlockDriverState *bs,
 }

 buf = g_malloc(s->cluster_size);
-sbc = sectors_covered_by_bitmap_cluster(s, bitmap);
+limit = bytes_covered_by_bitmap_cluster(s, bitmap);
+sbc = limit >> BDRV_SECTOR_BITS;
 for (i = 0, sector = 0; i < tab_size; ++i, sector += sbc) {
 uint64_t count = MIN(bm_sectors - sector, sbc);
 uint64_t entry = bitmap_table[i];
@@ -1080,7 +1079,7 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
 int ret;
 BDRVQcow2State *s = bs->opaque;
 int64_t sector;
-uint64_t sbc;
+uint64_t limit, sbc;
 uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
 uint64_t bm_sectors = DIV_ROUND_UP(bm_size, BDRV_SECTOR_SIZE);
 const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
@@ -1106,8 +1105,9 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,

 dbi = bdrv_dirty_iter_new(bitmap, 0);
 buf = g_malloc(s->cluster_size);
-sbc = sectors_covered_by_bitmap_cluster(s, bitmap);
-assert(DIV_ROUND_UP(bm_sectors, sbc) == tb_size);
+limit = bytes_covered_by_bitmap_cluster(s, bitmap);
+sbc = limit >> BDRV_SECTOR_BITS;
+assert(DIV_ROUND_UP(bm_size, limit) == tb_size);

 while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
 uint64_t cluster = sector / sbc;
-- 
2.13.5

[Qemu-devel] [PATCH v6 03/18] qcow2: Ensure bitmap serialization is aligned

[Eric Blake]

When subdividing a bitmap serialization, the code in hbitmap.c
enforces that start/count parameters are aligned (except that
count can end early at end-of-bitmap).  We exposed this required
alignment through bdrv_dirty_bitmap_serialization_align(), but
forgot to actually check that we comply with it.

Fortunately, qcow2 is never dividing bitmap serialization smaller
than one cluster (which is a minimum of 512 bytes); so we are
always compliant with the serialization alignment (which insists
that we partition at least 64 bits per chunk) because we are doing
at least 4k bits per chunk.

Still, it's safer to add an assertion (for the unlikely case that
we'd ever support a cluster smaller than 512 bytes, or if the
hbitmap implementation changes what it considers to be aligned),
rather than leaving bdrv_dirty_bitmap_serialization_align()
without a caller.

Signed-off-by: Eric Blake 
Reviewed-by: John Snow 

---
v5: no change
v4: new patch
---
 block/qcow2-bitmap.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index e8d3bdbd6e..b3ee4c794a 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -274,10 +274,13 @@ static int free_bitmap_clusters(BlockDriverState *bs, 
Qcow2BitmapTable *tb)
 static uint64_t sectors_covered_by_bitmap_cluster(const BDRVQcow2State *s,
   const BdrvDirtyBitmap 
*bitmap)
 {
-uint32_t sector_granularity =
+uint64_t sector_granularity =
 bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
+uint64_t sbc = sector_granularity * (s->cluster_size << 3);

-return (uint64_t)sector_granularity * (s->cluster_size << 3);
+assert(QEMU_IS_ALIGNED(sbc,
+   bdrv_dirty_bitmap_serialization_align(bitmap)));
+return sbc;
 }

 /* load_bitmap_data
-- 
2.13.5

[Qemu-devel] [PATCH v6 02/18] hbitmap: Rename serialization_granularity to serialization_align

[Eric Blake]

The only client of hbitmap_serialization_granularity() is dirty-bitmap's
bdrv_dirty_bitmap_serialization_align().  Keeping the two names consistent
is worthwhile, and the shorter name is more representative of what the
function returns (the required alignment to be used for start/count of
other serialization functions, where violating the alignment causes
assertion failures).

Signed-off-by: Eric Blake 
Reviewed-by: John Snow 

---
v5: no change
v4: new patch
---
 include/qemu/hbitmap.h |  8 
 block/dirty-bitmap.c   |  2 +-
 tests/test-hbitmap.c   | 10 +-
 util/hbitmap.c |  8 
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index d3a74a21fc..81e78043d1 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -159,16 +159,16 @@ bool hbitmap_get(const HBitmap *hb, uint64_t item);
 bool hbitmap_is_serializable(const HBitmap *hb);

 /**
- * hbitmap_serialization_granularity:
+ * hbitmap_serialization_align:
  * @hb: HBitmap to operate on.
  *
- * Granularity of serialization chunks, used by other serialization functions.
- * For every chunk:
+ * Required alignment of serialization chunks, used by other serialization
+ * functions. For every chunk:
  * 1. Chunk start should be aligned to this granularity.
  * 2. Chunk size should be aligned too, except for last chunk (for which
  *  start + count == hb->size)
  */
-uint64_t hbitmap_serialization_granularity(const HBitmap *hb);
+uint64_t hbitmap_serialization_align(const HBitmap *hb);

 /**
  * hbitmap_serialization_size:
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 30462d4f9a..0490ca3aff 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -617,7 +617,7 @@ uint64_t bdrv_dirty_bitmap_serialization_size(const 
BdrvDirtyBitmap *bitmap,

 uint64_t bdrv_dirty_bitmap_serialization_align(const BdrvDirtyBitmap *bitmap)
 {
-return hbitmap_serialization_granularity(bitmap->bitmap);
+return hbitmap_serialization_align(bitmap->bitmap);
 }

 void bdrv_dirty_bitmap_serialize_part(const BdrvDirtyBitmap *bitmap,
diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index 1acb353889..af41642346 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -738,15 +738,15 @@ static void test_hbitmap_meta_one(TestHBitmapData *data, 
const void *unused)
 }
 }

-static void test_hbitmap_serialize_granularity(TestHBitmapData *data,
-   const void *unused)
+static void test_hbitmap_serialize_align(TestHBitmapData *data,
+ const void *unused)
 {
 int r;

 hbitmap_test_init(data, L3 * 2, 3);
 g_assert(hbitmap_is_serializable(data->hb));

-r = hbitmap_serialization_granularity(data->hb);
+r = hbitmap_serialization_align(data->hb);
 g_assert_cmpint(r, ==, 64 << 3);
 }

@@ -974,8 +974,8 @@ int main(int argc, char **argv)
 hbitmap_test_add("/hbitmap/meta/word", test_hbitmap_meta_word);
 hbitmap_test_add("/hbitmap/meta/sector", test_hbitmap_meta_sector);

-hbitmap_test_add("/hbitmap/serialize/granularity",
- test_hbitmap_serialize_granularity);
+hbitmap_test_add("/hbitmap/serialize/align",
+ test_hbitmap_serialize_align);
 hbitmap_test_add("/hbitmap/serialize/basic",
  test_hbitmap_serialize_basic);
 hbitmap_test_add("/hbitmap/serialize/part",
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 21535cc90b..2f9d0fdbd0 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -413,14 +413,14 @@ bool hbitmap_is_serializable(const HBitmap *hb)
 {
 /* Every serialized chunk must be aligned to 64 bits so that endianness
  * requirements can be fulfilled on both 64 bit and 32 bit hosts.
- * We have hbitmap_serialization_granularity() which converts this
+ * We have hbitmap_serialization_align() which converts this
  * alignment requirement from bitmap bits to items covered (e.g. sectors).
  * That value is:
  *64 << hb->granularity
  * Since this value must not exceed UINT64_MAX, hb->granularity must be
  * less than 58 (== 64 - 6, where 6 is ld(64), i.e. 1 << 6 == 64).
  *
- * In order for hbitmap_serialization_granularity() to always return a
+ * In order for hbitmap_serialization_align() to always return a
  * meaningful value, bitmaps that are to be serialized must have a
  * granularity of less than 58. */

@@ -437,7 +437,7 @@ bool hbitmap_get(const HBitmap *hb, uint64_t item)
 return (hb->levels[HBITMAP_LEVELS - 1][pos >> BITS_PER_LEVEL] & bit) != 0;
 }

-uint64_t hbitmap_serialization_granularity(const HBitmap *hb)
+uint64_t hbitmap_serialization_align(const HBitmap *hb)
 {
 assert(hbitmap_is_serializable(hb));

@@ -454,7 +454,7 @@ static void serialization_chunk(const HBitmap *hb,
 unsigned long **first_el, uint64_t 

[Qemu-devel] [PATCH v6 05/18] dirty-bitmap: Change bdrv_dirty_bitmap_size() to report bytes

[Eric Blake]

We are still using an internal hbitmap that tracks a size in sectors,
with the granularity scaled down accordingly, because it lets us
use a shortcut for our iterators which are currently sector-based.
But there's no reason we can't track the dirty bitmap size in bytes,
since it is (mostly) an internal-only variable (remember, the size
is how many bytes are covered by the bitmap, not how many bytes the
bitmap occupies).  Furthermore, we're already reporting bytes for
bdrv_dirty_bitmap_granularity(); mixing bytes and sectors in our
return values is a recipe for confusion.  A later cleanup will
convert dirty bitmap internals to be entirely byte-based,
eliminating the intermediate sector rounding added here; and
technically, since bdrv_getlength() already rounds up to sectors,
our use of DIV_ROUND_UP is more for theoretical completeness than
for any actual rounding.

The only external caller in qcow2-bitmap.c is temporarily more verbose
(because it is still using sector-based math), but will later be
switched to track progress by bytes instead of sectors.

Use is_power_of_2() while at it, instead of open-coding that, and
add an assertion where bdrv_getlength() should not fail.

Signed-off-by: Eric Blake 
Reviewed-by: John Snow 

---
v6: no change
v5: fix bdrv_dirty_bitmap_truncate [John], drop R-b
v4: retitle from "Track size in bytes", rebase to persistent bitmaps,
round up when converting bytes to sectors
v3: no change
v2: tweak commit message, no code change
---
 block/dirty-bitmap.c | 26 +++---
 block/qcow2-bitmap.c | 14 --
 2 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 42a55e4a4b..e65ec4f7ec 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -1,7 +1,7 @@
 /*
  * Block Dirty Bitmap
  *
- * Copyright (c) 2016 Red Hat. Inc
+ * Copyright (c) 2016-2017 Red Hat. Inc
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to 
deal
@@ -42,7 +42,7 @@ struct BdrvDirtyBitmap {
 HBitmap *meta;  /* Meta dirty bitmap */
 BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
 char *name; /* Optional non-empty unique ID */
-int64_t size;   /* Size of the bitmap (Number of sectors) */
+int64_t size;   /* Size of the bitmap, in bytes */
 bool disabled;  /* Bitmap is disabled. It ignores all writes to
the device */
 int active_iterators;   /* How many iterators are active */
@@ -115,17 +115,14 @@ BdrvDirtyBitmap 
*bdrv_create_dirty_bitmap(BlockDriverState *bs,
 {
 int64_t bitmap_size;
 BdrvDirtyBitmap *bitmap;
-uint32_t sector_granularity;

-assert((granularity & (granularity - 1)) == 0);
+assert(is_power_of_2(granularity) && granularity >= BDRV_SECTOR_SIZE);

 if (name && bdrv_find_dirty_bitmap(bs, name)) {
 error_setg(errp, "Bitmap already exists: %s", name);
 return NULL;
 }
-sector_granularity = granularity >> BDRV_SECTOR_BITS;
-assert(sector_granularity);
-bitmap_size = bdrv_nb_sectors(bs);
+bitmap_size = bdrv_getlength(bs);
 if (bitmap_size < 0) {
 error_setg_errno(errp, -bitmap_size, "could not get length of device");
 errno = -bitmap_size;
@@ -133,7 +130,12 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState 
*bs,
 }
 bitmap = g_new0(BdrvDirtyBitmap, 1);
 bitmap->mutex = >dirty_bitmap_mutex;
-bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(sector_granularity));
+/*
+ * TODO - let hbitmap track full granularity. For now, it is tracking
+ * only sector granularity, as a shortcut for our iterators.
+ */
+bitmap->bitmap = hbitmap_alloc(DIV_ROUND_UP(bitmap_size, BDRV_SECTOR_SIZE),
+   ctz32(granularity) - BDRV_SECTOR_BITS);
 bitmap->size = bitmap_size;
 bitmap->name = g_strdup(name);
 bitmap->disabled = false;
@@ -305,13 +307,14 @@ BdrvDirtyBitmap 
*bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
 void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
 {
 BdrvDirtyBitmap *bitmap;
-uint64_t size = bdrv_nb_sectors(bs);
+int64_t size = bdrv_getlength(bs);

+assert(size >= 0);
 bdrv_dirty_bitmaps_lock(bs);
 QLIST_FOREACH(bitmap, >dirty_bitmaps, list) {
 assert(!bdrv_dirty_bitmap_frozen(bitmap));
 assert(!bitmap->active_iterators);
-hbitmap_truncate(bitmap->bitmap, size);
+hbitmap_truncate(bitmap->bitmap, DIV_ROUND_UP(size, BDRV_SECTOR_SIZE));
 bitmap->size = size;
 }
 bdrv_dirty_bitmaps_unlock(bs);
@@ -549,7 +552,8 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, 
HBitmap **out)
 hbitmap_reset_all(bitmap->bitmap);
 } else {
 HBitmap *backup = bitmap->bitmap;
- 

[Qemu-devel] [PATCH v6 00/18] make dirty-bitmap byte-based

[Eric Blake]

There are patches floating around to add NBD_CMD_BLOCK_STATUS,
but NBD wants to report status on byte granularity (even if the
reporting will probably be naturally aligned to sectors or even
much higher levels).  I've therefore started the task of
converting our block status code to report at a byte granularity
rather than sectors.

Now that 2.11 is open, I'm rebasing/reposting the remaining patches.

The overall conversion currently looks like:
part 1: bdrv_is_allocated (merged in 2.10, commit 51b0a488)
part 2: dirty-bitmap (this series, v5 was here [1])
part 3: bdrv_get_block_status (v3 is posted [2] and is mostly reviewed, but
needs a rebase)
part 4: .bdrv_co_block_status (v2 is posted [3], but needs a rebase)

Available as a tag at:
git fetch git://repo.or.cz/qemu/ericb.git nbd-byte-dirty-v6

[1] https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg03512.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg03853.html
[3] https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg04370.html

Diff from v5:
- add another patch (more for ease of bookkeeping, as it was previously
posted independently)
- drop bug fixes that were hoisted into 2.10 (v5 1/18, plus 14/18)

001/18:[down] 'block: Make bdrv_img_create() size selection easier to read'
002/18:[] [--] 'hbitmap: Rename serialization_granularity to 
serialization_align'
003/18:[] [--] 'qcow2: Ensure bitmap serialization is aligned'
004/18:[] [--] 'dirty-bitmap: Drop unused functions'
005/18:[] [--] 'dirty-bitmap: Change bdrv_dirty_bitmap_size() to report 
bytes'
006/18:[] [--] 'dirty-bitmap: Change bdrv_dirty_bitmap_*serialize*() to 
take bytes'
007/18:[] [--] 'qcow2: Switch sectors_covered_by_bitmap_cluster() to 
byte-based'
008/18:[] [--] 'dirty-bitmap: Set iterator start by offset, not sector'
009/18:[] [--] 'dirty-bitmap: Change bdrv_dirty_iter_next() to report byte 
offset'
010/18:[] [--] 'dirty-bitmap: Change bdrv_get_dirty_count() to report bytes'
011/18:[] [--] 'dirty-bitmap: Change bdrv_get_dirty_locked() to take bytes'
012/18:[] [--] 'dirty-bitmap: Change bdrv_[re]set_dirty_bitmap() to use 
bytes'
013/18:[] [--] 'mirror: Switch mirror_dirty_init() to byte-based iteration'
014/18:[0004] [FC] 'qcow2: Switch qcow2_measure() to byte-based iteration'
015/18:[] [--] 'qcow2: Switch load_bitmap_data() to byte-based iteration'
016/18:[] [--] 'qcow2: Switch store_bitmap_data() to byte-based iteration'
017/18:[] [--] 'dirty-bitmap: Switch bdrv_set_dirty() to bytes'
018/18:[] [--] 'dirty-bitmap: Convert internal hbitmap size/granularity'

Eric Blake (18):
  block: Make bdrv_img_create() size selection easier to read
  hbitmap: Rename serialization_granularity to serialization_align
  qcow2: Ensure bitmap serialization is aligned
  dirty-bitmap: Drop unused functions
  dirty-bitmap: Change bdrv_dirty_bitmap_size() to report bytes
  dirty-bitmap: Change bdrv_dirty_bitmap_*serialize*() to take bytes
  qcow2: Switch sectors_covered_by_bitmap_cluster() to byte-based
  dirty-bitmap: Set iterator start by offset, not sector
  dirty-bitmap: Change bdrv_dirty_iter_next() to report byte offset
  dirty-bitmap: Change bdrv_get_dirty_count() to report bytes
  dirty-bitmap: Change bdrv_get_dirty_locked() to take bytes
  dirty-bitmap: Change bdrv_[re]set_dirty_bitmap() to use bytes
  mirror: Switch mirror_dirty_init() to byte-based iteration
  qcow2: Switch qcow2_measure() to byte-based iteration
  qcow2: Switch load_bitmap_data() to byte-based iteration
  qcow2: Switch store_bitmap_data() to byte-based iteration
  dirty-bitmap: Switch bdrv_set_dirty() to bytes
  dirty-bitmap: Convert internal hbitmap size/granularity

 include/block/block_int.h|   2 +-
 include/block/dirty-bitmap.h |  41 +-
 include/qemu/hbitmap.h   |   8 +--
 block/io.c   |   6 +-
 block.c  |   2 +-
 block/backup.c   |   7 +--
 block/dirty-bitmap.c | 130 ++-
 block/mirror.c   |  76 +++--
 block/qcow2-bitmap.c |  57 +--
 block/qcow2.c|  22 
 migration/block.c|  12 ++--
 tests/test-hbitmap.c |  10 ++--
 util/hbitmap.c   |   8 +--
 13 files changed, 154 insertions(+), 227 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH v6 01/18] block: Make bdrv_img_create() size selection easier to read

[Eric Blake]

All callers of bdrv_img_create() pass in a size, or -1 to read the
size from the backing file.  We then set that size as the QemuOpt
default, which means we will reuse that default rather than the
final parameter to qemu_opt_get_size() several lines later.  But
it is rather confusing to read subsequent checks of 'size == -1'
when it looks (without seeing the full context) like size defaults
to 0; it also doesn't help that a size of 0 is valid (for some
formats).

Rework the logic to make things more legible.

Signed-off-by: Eric Blake 
Reviewed-by: John Snow 

---
v6: Combine into a series rather than being a standalone patch (more for
ease of tracking than for being on topic)
---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 3308814bba..3f84d141a7 100644
--- a/block.c
+++ b/block.c
@@ -4392,7 +4392,7 @@ void bdrv_img_create(const char *filename, const char 
*fmt,

 /* The size for the image must always be specified, unless we have a 
backing
  * file and we have not been forbidden from opening it. */
-size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
+size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, img_size);
 if (backing_file && !(flags & BDRV_O_NO_BACKING)) {
 BlockDriverState *bs;
 char *full_backing = g_new0(char, PATH_MAX);
-- 
2.13.5

Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state

[Thomas Huth]

On 30.08.2017 19:05, David Hildenbrand wrote:
> Let's avoid global variables. While at it, move both functions using it,
> so we won't have to temporarily add includes (we'll be getting rid of
> s390-virtio.c soon).
> 
> Signed-off-by: David Hildenbrand 
> ---
>  hw/s390x/s390-virtio-ccw.c | 39 
> ++
>  hw/s390x/s390-virtio.c | 38 -
>  hw/s390x/s390-virtio.h |  1 -
>  include/hw/s390x/s390-virtio-ccw.h |  3 +++
>  4 files changed, 42 insertions(+), 39 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index dd504dd5ae..ffd56af834 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -32,6 +32,45 @@
>  #include "migration/register.h"
>  #include "cpu_models.h"
>  
> +S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
> +{
> +S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
> +
> +if (cpu_addr >= max_cpus) {
> +return NULL;
> +}
> +
> +/* Fast lookup via CPU ID */
> +return ms->cpus[cpu_addr];
> +}

I wonder whether that function should rather go into a file in
target/s390x/ instead, since it is also used there and its prototype is
in cpu.h ?

[...]
> diff --git a/include/hw/s390x/s390-virtio-ccw.h 
> b/include/hw/s390x/s390-virtio-ccw.h
> index 41a9d2862b..4bef28ec39 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -21,11 +21,14 @@
>  #define S390_MACHINE_CLASS(klass) \
>  OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE)
>  
> +struct S390CPU;

You define a "struct S390CPU" here ...

>  typedef struct S390CcwMachineState {
>  /*< private >*/
>  MachineState parent_obj;
>  
>  /*< public >*/
> +S390CPU **cpus;

... but use the typedef'ed S390CPU here ... looks somewhat suspicious, I
wonder whether the typedef is really in the right place?

>  bool aes_key_wrap;
>  bool dea_key_wrap;
>  uint8_t loadparm[8];

Anyway, that were just nits, I'm also fine with the patch as it is, so:

Reviewed-by: Thomas Huth 

Re: [Qemu-devel] [PATCH v4 1/2] hw/acpi-build: Fix SRAT memory building when there is no memory in node0

[Eduardo Habkost]

On Tue, Aug 22, 2017 at 11:24:09AM +0800, Dou Liyang wrote:
> Currently, Using the fisrt node without memory on the machine makes
> QEMU unhappy. With this example command line:
>   ... \
>   -m 1024M,slots=4,maxmem=32G \
>   -numa node,nodeid=0 \
>   -numa node,mem=1024M,nodeid=1 \
>   -numa node,nodeid=2 \
>   -numa node,nodeid=3 \
> Guest reports "No NUMA configuration found" and the NUMA topology is
> wrong.
> 
> This is because when QEMU builds ACPI SRAT, it regards node0 as the
> default node to deal with the memory hole(640K-1M). this means the
> node0 must have some memory(>1M), but, actually it can have no
> memory.
> 
> Fix this problem by replace the node0 with the first node which has
> memory on it. Add a new function for each node. Also do some cleanup.
> 
> Signed-off-by: Dou Liyang 
> ---
>  hw/i386/acpi-build.c | 78 
> +---
>  1 file changed, 50 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 98dd424..f93d712 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2318,15 +2318,43 @@ build_tpm2(GArray *table_data, BIOSLinker *linker)
>   (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
>  }
>  
> +static uint64_t
> +build_srat_node_entry(GArray *table_data, PCMachineState *pcms,
> +int i, uint64_t mem_base, uint64_t mem_len)
> +{
> +AcpiSratMemoryAffinity *numamem;
> +uint64_t next_base;
> +
> +next_base = mem_base + mem_len;
> +
> +/* Cut out the ACPI_PCI hole */
> +if (mem_base <= pcms->below_4g_mem_size &&
> +next_base > pcms->below_4g_mem_size) {
> +mem_len -= next_base - pcms->below_4g_mem_size;
> +if (mem_len > 0) {
> +numamem = acpi_data_push(table_data, sizeof *numamem);
> +build_srat_memory(numamem, mem_base, mem_len, i,
> +  MEM_AFFINITY_ENABLED);
> +}
> +mem_base = 1ULL << 32;
> +mem_len = next_base - pcms->below_4g_mem_size;
> +next_base += (1ULL << 32) - pcms->below_4g_mem_size;
> +}
> +numamem = acpi_data_push(table_data, sizeof *numamem);
> +build_srat_memory(numamem, mem_base, mem_len, i,
> +  MEM_AFFINITY_ENABLED);
> +return next_base;
> +}
> +
>  static void
>  build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
>  {
>  AcpiSystemResourceAffinityTable *srat;
>  AcpiSratMemoryAffinity *numamem;
>  
> -int i;
> +int i, node;
>  int srat_start, numa_start, slots;
> -uint64_t mem_len, mem_base, next_base;
> +uint64_t mem_len, mem_base;
>  MachineClass *mc = MACHINE_GET_CLASS(machine);
>  const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
>  PCMachineState *pcms = PC_MACHINE(machine);
> @@ -2370,36 +2398,30 @@ build_srat(GArray *table_data, BIOSLinker *linker, 
> MachineState *machine)
>  /* the memory map is a bit tricky, it contains at least one hole
>   * from 640k-1M and possibly another one from 3.5G-4G.
>   */
> -next_base = 0;
> +
>  numa_start = table_data->len;
>  
> -numamem = acpi_data_push(table_data, sizeof *numamem);
> -build_srat_memory(numamem, 0, 640 * 1024, 0, MEM_AFFINITY_ENABLED);
> -next_base = 1024 * 1024;
> -for (i = 1; i < pcms->numa_nodes + 1; ++i) {
> -mem_base = next_base;
> -mem_len = pcms->node_mem[i - 1];
> -if (i == 1) {
> -mem_len -= 1024 * 1024;
> +/* get the first node which has memory and map the hole from 640K-1M */
> +for (node = 0; node < pcms->numa_nodes; node++) {
> +if (pcms->node_mem[node] != 0) {
> +break;
>  }
> -next_base = mem_base + mem_len;
> -
> -/* Cut out the ACPI_PCI hole */
> -if (mem_base <= pcms->below_4g_mem_size &&
> -next_base > pcms->below_4g_mem_size) {
> -mem_len -= next_base - pcms->below_4g_mem_size;
> -if (mem_len > 0) {
> -numamem = acpi_data_push(table_data, sizeof *numamem);
> -build_srat_memory(numamem, mem_base, mem_len, i - 1,
> -  MEM_AFFINITY_ENABLED);
> -}
> -mem_base = 1ULL << 32;
> -mem_len = next_base - pcms->below_4g_mem_size;
> -next_base += (1ULL << 32) - pcms->below_4g_mem_size;
> +}
> +numamem = acpi_data_push(table_data, sizeof *numamem);
> +build_srat_memory(numamem, 0, 640 * 1024, node, MEM_AFFINITY_ENABLED);
> +
> +/* map the rest of memory from 1M */
> +mem_base = 1024 * 1024;
> +mem_len = pcms->node_mem[node] - mem_base;
> +mem_base = build_srat_node_entry(table_data, pcms, node,
> +mem_base, mem_len);
> +
> +for (i = 0; i < pcms->numa_nodes; i++) {
> +if (i == node) {
> +continue;
>  

Re: [Qemu-devel] [PATCH 0/8] QOMify MIPS cpu

[James Hogan]

On Wed, Aug 30, 2017 at 04:52:04PM -0300, Philippe Mathieu-Daudé wrote:
> Hi James,
> 
> On 08/30/2017 03:19 PM, James Hogan wrote:
> [...]
> >> git://github.com/philmd/qemu.git tags/mips-qomify-20170830
> > 
> > A sanity check of your branch doesn't reveal any obvious regressions
> > when booting a Malta KVM guest kernel with KVM.
> 
> Thank you for your time to test this!
> 
> Does that mean I can add your Tested-by: to the series?

Sure,

Cheers
James