Re: [PATCH 03/55] qdev: New qdev_new(), qdev_realize(), etc.

[Paolo Bonzini]

Il lun 25 mag 2020, 08:30 Markus Armbruster  ha scritto:

> > I think we should move the whole /machine/unattached dance into
> > qdev_realize, and just assert that a device being realized already has a
> > parent.  Then the ref/unref _will_ be unnecessary.  In the meanwhile, I
> > agree to either keep it or move it inside device_set_realized.
>
> Could be done on top.  I might try for v2, if I can find the time.
>

Certainly on top.

Paolo

Re: [PATCH 51/55] qdev: Make qdev_realize() support bus-less devices

[Markus Armbruster]

Paolo Bonzini  writes:

> On 20/05/20 17:02, Markus Armbruster wrote:

 qdev_realize_and_unref() remains restricted, because its reference
 counting would become rather confusing for bus-less devices.
>>> I think it would be fine, you would just rely on the reference held by
>>> the QOM parent (via the child property).
>> I took one look at the contract I wrote for it, and balked :)
>> 
>> qdev_realize()'s contract before this patch:
>> 
>> /*
>>  * Realize @dev.
>>  * @dev must not be plugged into a bus.
>>  * Plug @dev into @bus.  This takes a reference to @dev.
>>  * If @dev has no QOM parent, make one up, taking another reference.
>>  * On success, return true.
>>  * On failure, store an error through @errp and return false.
>>  */
>> bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp)
>> 
>> Simple enough.
>> 
>> This patch merely adds "If @bus, " before "plug".  Still simple enough.
>> 
>> qdev_realize_and_unref()'s contract:
>> 
>> /*
>>  * Realize @dev and drop a reference.
>>  * This is like qdev_realize(), except it steals a reference rather
>>  * than take one to plug @dev into @bus.  On failure, it drops that
>>  * reference instead.  @bus must not be null.  Intended use:
>>  * dev = qdev_new();
>>  * [...]
>>  * qdev_realize_and_unref(dev, bus, errp);
>>  * Now @dev can go away without further ado.
>>  */
>> bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error 
>> **errp)
>> 
>> If @bus is null, who gets to hold the stolen reference?
>> 
>> You seem to suggest the QOM parent.  What if @dev already has a parent?
>
> The caller would still hold the stolen reference, and it would be
> dropped.

I read this sentence three times, and still don't get it.  Is the
reference held or is it dropped?

>   You cannot have a device that goes away at the end of
> qdev_realize_and_unref, as long as dev has a QOM parent that clings onto
> dev.  Since dev will have /machine/unattached as the parent if it didn't
> already have one before, the function is safe even without a bus.

Write me a nice function contract, and I'll update the implementation to
match it.

> Or alternatively, ignore all the stolen references stuff, and merely see
> qdev_realize_and_unref as a shortcut for qdev_realize+object_unref,
> because it's a common idiom.

Even common idioms need to make sense :)

The contract must specify exactly what happens to the reference count,
case by case.

I chose to outlaw a case I see no use for, to keep the contract simpler.

Re: [PATCH 03/55] qdev: New qdev_new(), qdev_realize(), etc.

[Markus Armbruster]

Paolo Bonzini  writes:

> On 20/05/20 16:42, Markus Armbruster wrote:
>> If something goes
>> wrong there, we execute
>> 
>> fail:
>> error_propagate(errp, local_err);
>> if (unattached_parent) {
>> /*
>>  * Beware, this doesn't just revert
>>  * object_property_add_child(), it also runs bus_remove()!
>>  */
>> object_unparent(OBJECT(dev));
>> unattached_count--;
>> }
>> 
>> and bus_remove() drops the reference count to zero.
>
> Whoa whoa...  I didn't expect this from a failure to realize.

Me neither.  But by the time I understood what's going on here, my
appetite for big, structural QOM changes was pretty much gone, so I
merely added the "Beware" comment.

> I think we should move the whole /machine/unattached dance into
> qdev_realize, and just assert that a device being realized already has a
> parent.  Then the ref/unref _will_ be unnecessary.  In the meanwhile, I
> agree to either keep it or move it inside device_set_realized.

Could be done on top.  I might try for v2, if I can find the time.

Re: [PATCH 09/24] macio: Fix to realize "mos6522-cuda" and "mos6522-pmu" devices

[Markus Armbruster]

Peter Maydell  writes:

> On Mon, 18 May 2020 at 06:12, Markus Armbruster  wrote:
>>
>> cuda_init() creates a "mos6522-cuda" device, but it's never realized.
>> Affects machines mac99 with via=cuda (default) and g3beige.
>>
>> pmu_init() creates a "mos6522-pmu" device, but it's never realized.
>> Affects machine mac99 with via=pmu and via=pmu-adb,
>>
>> I wonder how this ever worked.  If the "device becomes real only on
>> realize" thing actually works, then we've always been missing these
>> devices, yet nobody noticed.
>
> Same remark as elsewhere: the devices aren't missing, we just
> got lucky that using them in a half-initialized state happens
> to work for these devices. Could you update the commit messages
> when you reroll this series, please?

Of course.  What about something like this:

In theory, a device becomes real only on realize.  In practice, the
transition from unreal to real is a fuzzy one.  The work to make a
device real can be spread between realize methods (fine),
instance_init methods (wrong), and board code wiring up the device
(fine as long as it effectively happens on realize).  Depending on
what exactly is done where, a device can work even when we neglect
to realize it.  Nevertheless, it's a clear misuse of the interface.
Even when it works today (more or less by chance), it can break
tomorrow.

>> Fix by realizing them in cuda_realize() and pmu_realize(),
>> respectively.
>>
>> Fixes: 6dca62af95e0b7020aa00d0ca9b2c421f341
>> Cc: Laurent Vivier 
>> Signed-off-by: Markus Armbruster 
>> ---
>>  hw/misc/macio/cuda.c | 8 +++-
>>  hw/misc/macio/pmu.c  | 8 +++-
>>  2 files changed, 6 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
>> index e0cc0aac5d..6d4d135f71 100644
>> --- a/hw/misc/macio/cuda.c
>> +++ b/hw/misc/macio/cuda.c
>> @@ -523,15 +523,13 @@ static void cuda_realize(DeviceState *dev, Error 
>> **errp)
>>  {
>>  CUDAState *s = CUDA(dev);
>>  SysBusDevice *sbd;
>> -MOS6522State *ms;
>> -DeviceState *d;
>>  struct tm tm;
>>
>> +qdev_init_nofail(DEVICE(&s->mos6522_cuda));
>
> Since we init the device with sysbus_init_child_obj() and
> we're in a position here to propagate any realize error
> back up to our caller, it would be nicer to do this via
> setting the realize property rather than qdev_init_nofail().

The error handling will be unreachable.

The proper way to say "error not possible" is of course &error_abort,
not qdev_init_nofail()'s &error_fatal.

Many realize methods misuse the Error interface this way.  NULL instead
of &error_abort is also common.  Cleaning this up is yet another big
task.  But that's no excuse for making it bigger now.

Laurent, would you prefer unreachable error handling or &error_abort?

> That's what this patch from March does:
> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg04260.html
> (we seem to have let that series drop accidentally,
> probably because it was halfway through release and
> because it touches several architectures at once).

Pity.

[Bug 1880507] [NEW] VMM from Ubuntu 20.04 does not show the memory consumption

[Dvornikov Mihail]

Public bug reported:

KVM host system: Ubuntu 18.04 and 20.04, guest machines: Windows and
Ubuntu. Management through Ubuntu 20.04, vmm does not show RAM
consumption for Windows guest systems (Win7, Win2008R2), for Ubuntu
values are shown. The error is not observed in Ubuntu 18.04/vmm.

** Affects: qemu
 Importance: Undecided
 Status: New

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1880507

Title:
  VMM from Ubuntu 20.04 does not show the memory consumption

Status in QEMU:
  New

Bug description:
  KVM host system: Ubuntu 18.04 and 20.04, guest machines: Windows and
  Ubuntu. Management through Ubuntu 20.04, vmm does not show RAM
  consumption for Windows guest systems (Win7, Win2008R2), for Ubuntu
  values are shown. The error is not observed in Ubuntu 18.04/vmm.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1880507/+subscriptions

Re: [PATCH 06/24] armv7m: Bury unwanted "ARM,bitband-memory" devices

[Markus Armbruster]

Peter Maydell  writes:

> On Mon, 18 May 2020 at 06:04, Markus Armbruster  wrote:
>>
>> These devices are optional, and enabled by property "enable-bitband".
>> armv7m_instance_init() creates them unconditionally, because the
>> property has not been set then.  armv7m_realize() realizes them only
>> when the property is true.  Works, although it leaves unrealized
>> devices hanging around in the QOM composition tree.  Affects machines
>> microbit, mps2-an505, mps2-an521, musca-a, and musca-b1.
>>
>> Bury the unwanted devices by making armv7m_realize() unparent them.
>> Visible in "info qom-tree"; here's the change for microbit:
>>
>>  /machine (microbit-machine)
>>/microbit.twi (microbit.i2c)
>>  /microbit.twi[0] (qemu:memory-region)
>>/nrf51 (nrf51-soc)
>>  /armv6m (armv7m)
>>/armv7m-container[0] (qemu:memory-region)
>> -  /bitband[0] (ARM,bitband-memory)
>> -/bitband[0] (qemu:memory-region)
>> -  /bitband[1] (ARM,bitband-memory)
>> -/bitband[0] (qemu:memory-region)
>>/cpu (cortex-m0-arm-cpu)
>
> What does "bury" mean here? To me it implies "they still
> exist but we've stuck them in a hole somewhere and covered
> them up", but the qom-tree delta suggests we've actually
> really deleted them?

We really delete them now.

"They've been lying dead in the streets; give them a decent burial".

Would you like me to s/Bury/Delete/?

Re: [PATCH] cpus: Fix botched configure_icount() error API violation fix

[Markus Armbruster]

Paolo Bonzini  writes:

> On 08/05/20 12:49, Markus Armbruster wrote:
>> Fixes: abc9bf69a66a11499a801ff545b8fe7adbb3a04c
>> Fixes: Coverity CID 1428754
>> Signed-off-by: Markus Armbruster 
>> ---
>>  cpus.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/cpus.c b/cpus.c
>> index 5670c96bcf..b9275c672d 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -813,7 +813,7 @@ void configure_icount(QemuOpts *opts, Error **errp)
>>  return;
>>  }
>>  
>> -if (strcmp(option, "auto") != 0) {
>> +if (option && !strcmp(option, "auto")) {
>>  if (qemu_strtol(option, NULL, 0, &time_shift) < 0
>>  || time_shift < 0 || time_shift > MAX_ICOUNT_SHIFT) {
>>  error_setg(errp, "icount: Invalid shift value");
>> 
>
> Queued, thanks.

This one's wrong, please queue v2 instead.

Re: [PATCH] hw/pci-bridge/dec: Remove dead debug code

[David Gibson]

On Mon, May 25, 2020 at 05:39:10AM +0200, Philippe Mathieu-Daudé wrote:
> Remove debug code never used since added in commit e1c6bbabee30.
> 
> Signed-off-by: Philippe Mathieu-Daudé 

Applied to ppc-for-5.1, thanks.

> ---
>  hw/pci-bridge/dec.c | 10 --
>  1 file changed, 10 deletions(-)
> 
> diff --git a/hw/pci-bridge/dec.c b/hw/pci-bridge/dec.c
> index 3ae2f788a4..952bc71122 100644
> --- a/hw/pci-bridge/dec.c
> +++ b/hw/pci-bridge/dec.c
> @@ -32,16 +32,6 @@
>  #include "hw/pci/pci_bridge.h"
>  #include "hw/pci/pci_bus.h"
>  
> -/* debug DEC */
> -//#define DEBUG_DEC
> -
> -#ifdef DEBUG_DEC
> -#define DEC_DPRINTF(fmt, ...)   \
> -do { printf("DEC: " fmt , ## __VA_ARGS__); } while (0)
> -#else
> -#define DEC_DPRINTF(fmt, ...)
> -#endif
> -
>  #define DEC_21154(obj) OBJECT_CHECK(DECState, (obj), TYPE_DEC_21154)
>  
>  typedef struct DECState {

-- 
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: [PATCH v3] spapr: Add a new level of NUMA for GPUs

[David Gibson]

On Fri, May 22, 2020 at 02:53:33PM -0500, Reza Arbab wrote:
> NUMA nodes corresponding to GPU memory currently have the same
> affinity/distance as normal memory nodes. Add a third NUMA associativity
> reference point enabling us to give GPU nodes more distance.
> 
> This is guest visible information, which shouldn't change under a
> running guest across migration between different qemu versions, so make
> the change effective only in new (pseries > 5.0) machine types.
> 
> Before, `numactl -H` output in a guest with 4 GPUs (nodes 2-5):
> 
> node distances:
> node   0   1   2   3   4   5
>   0:  10  40  40  40  40  40
>   1:  40  10  40  40  40  40
>   2:  40  40  10  40  40  40
>   3:  40  40  40  10  40  40
>   4:  40  40  40  40  10  40
>   5:  40  40  40  40  40  10
> 
> After:
> 
> node distances:
> node   0   1   2   3   4   5
>   0:  10  40  80  80  80  80
>   1:  40  10  80  80  80  80
>   2:  80  80  10  80  80  80
>   3:  80  80  80  10  80  80
>   4:  80  80  80  80  10  80
>   5:  80  80  80  80  80  10
> 
> These are the same distances as on the host, mirroring the change made
> to host firmware in skiboot commit f845a648b8cb ("numa/associativity:
> Add a new level of NUMA for GPU's").
> 
> Signed-off-by: Reza Arbab 
> ---
> v3:
> * Squash into one patch
> * Add PHB compat property
> ---
>  hw/ppc/spapr.c  | 21 +++--
>  hw/ppc/spapr_pci.c  |  2 ++
>  hw/ppc/spapr_pci_nvlink2.c  |  7 ++-
>  include/hw/pci-host/spapr.h |  1 +
>  include/hw/ppc/spapr.h  |  1 +
>  5 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c18eab0a2305..7c304b6c389d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -889,10 +889,16 @@ static int spapr_dt_rng(void *fdt)
>  static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
>  {
>  MachineState *ms = MACHINE(spapr);
> +SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
>  int rtas;
>  GString *hypertas = g_string_sized_new(256);
>  GString *qemu_hypertas = g_string_sized_new(256);
> -uint32_t refpoints[] = { cpu_to_be32(0x4), cpu_to_be32(0x4) };
> +uint32_t refpoints[] = {
> +cpu_to_be32(0x4),
> +cpu_to_be32(0x4),
> +cpu_to_be32(0x2),
> +};
> +uint32_t nr_refpoints = 3;
>  uint64_t max_device_addr = MACHINE(spapr)->device_memory->base +
>  memory_region_size(&MACHINE(spapr)->device_memory->mr);
>  uint32_t lrdr_capacity[] = {
> @@ -944,8 +950,12 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void 
> *fdt)
>   qemu_hypertas->str, qemu_hypertas->len));
>  g_string_free(qemu_hypertas, TRUE);
>  
> +if (smc->pre_5_1_assoc_refpoints) {
> +nr_refpoints = 2;
> +}
> +
>  _FDT(fdt_setprop(fdt, rtas, "ibm,associativity-reference-points",
> - refpoints, sizeof(refpoints)));
> + refpoints, nr_refpoints * sizeof(refpoints[0])));
>  
>  _FDT(fdt_setprop(fdt, rtas, "ibm,max-associativity-domains",
>   maxdomains, sizeof(maxdomains)));
> @@ -4607,8 +4617,15 @@ DEFINE_SPAPR_MACHINE(5_1, "5.1", true);
>   */
>  static void spapr_machine_5_0_class_options(MachineClass *mc)
>  {
> +SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> +static GlobalProperty compat[] = {
> +{ TYPE_SPAPR_PCI_HOST_BRIDGE, "pre-5.1-associativity", "on" },
> +};
> +
>  spapr_machine_5_1_class_options(mc);
>  compat_props_add(mc->compat_props, hw_compat_5_0, hw_compat_5_0_len);
> +compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
> +smc->pre_5_1_assoc_refpoints = true;
>  }
>  
>  DEFINE_SPAPR_MACHINE(5_0, "5.0", false);
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 61b84a392d65..bcdf1a25ae8b 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -2092,6 +2092,8 @@ static Property spapr_phb_properties[] = {
>   pcie_ecs, true),
>  DEFINE_PROP_UINT64("gpa", SpaprPhbState, nv2_gpa_win_addr, 0),
>  DEFINE_PROP_UINT64("atsd", SpaprPhbState, nv2_atsd_win_addr, 0),
> +DEFINE_PROP_BOOL("pre-5.1-associativity", SpaprPhbState,
> + pre_5_1_assoc, false),
>  DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/ppc/spapr_pci_nvlink2.c b/hw/ppc/spapr_pci_nvlink2.c
> index 8332d5694e46..3394ac425eee 100644
> --- a/hw/ppc/spapr_pci_nvlink2.c
> +++ b/hw/ppc/spapr_pci_nvlink2.c
> @@ -362,7 +362,7 @@ void spapr_phb_nvgpu_ram_populate_dt(SpaprPhbState *sphb, 
> void *fdt)
>  uint32_t associativity[] = {
>  cpu_to_be32(0x4),
>  SPAPR_GPU_NUMA_ID,
> -SPAPR_GPU_NUMA_ID,
> +cpu_to_be32(nvslot->numa_id),
>  SPAPR_GPU_NUMA_ID,
>  cpu_to_be32(nvslot->numa_id)


This doesn't look quite right.  In the new case we'll get {
GPU_NUMA_ID, nvslot->numa_id, GPU_NUMA_ID, nvslot->numa_id }.

>  };
> @@ -374,6 +374,11 @@ voi

Re: [PATCH] hw/nvram/mac_nvram: Convert debug printf()s to trace events

[David Gibson]

On Sun, May 24, 2020 at 06:51:26PM +0200, Philippe Mathieu-Daudé wrote:
> Convert NVR_DPRINTF() to trace events and remove ifdef'ry.
> 
> Signed-off-by: Philippe Mathieu-Daudé 

Applied to ppc-for-5.1, thanks.

> ---
>  hw/nvram/mac_nvram.c  | 17 +++--
>  hw/nvram/trace-events |  4 
>  2 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/nvram/mac_nvram.c b/hw/nvram/mac_nvram.c
> index ff5db03e6b..beec1c4e4d 100644
> --- a/hw/nvram/mac_nvram.c
> +++ b/hw/nvram/mac_nvram.c
> @@ -30,18 +30,9 @@
>  #include "migration/vmstate.h"
>  #include "qemu/cutils.h"
>  #include "qemu/module.h"
> +#include "trace.h"
>  #include 
>  
> -/* debug NVR */
> -//#define DEBUG_NVR
> -
> -#ifdef DEBUG_NVR
> -#define NVR_DPRINTF(fmt, ...)   \
> -do { printf("NVR: " fmt , ## __VA_ARGS__); } while (0)
> -#else
> -#define NVR_DPRINTF(fmt, ...)
> -#endif
> -
>  #define DEF_SYSTEM_SIZE 0xc10
>  
>  /* macio style NVRAM device */
> @@ -51,9 +42,8 @@ static void macio_nvram_writeb(void *opaque, hwaddr addr,
>  MacIONVRAMState *s = opaque;
>  
>  addr = (addr >> s->it_shift) & (s->size - 1);
> +trace_macio_nvram_write(addr, value);
>  s->data[addr] = value;
> -NVR_DPRINTF("writeb addr %04" HWADDR_PRIx " val %" PRIx64 "\n",
> -addr, value);
>  }
>  
>  static uint64_t macio_nvram_readb(void *opaque, hwaddr addr,
> @@ -64,8 +54,7 @@ static uint64_t macio_nvram_readb(void *opaque, hwaddr addr,
>  
>  addr = (addr >> s->it_shift) & (s->size - 1);
>  value = s->data[addr];
> -NVR_DPRINTF("readb addr %04" HWADDR_PRIx " val %" PRIx32 "\n",
> -addr, value);
> +trace_macio_nvram_read(addr, value);
>  
>  return value;
>  }
> diff --git a/hw/nvram/trace-events b/hw/nvram/trace-events
> index 0dea9260ce..e023193295 100644
> --- a/hw/nvram/trace-events
> +++ b/hw/nvram/trace-events
> @@ -13,3 +13,7 @@ fw_cfg_add_string(uint16_t key_value, const char *key_name, 
> const char *value) "
>  fw_cfg_add_i16(uint16_t key_value, const char *key_name, uint16_t value) 
> "key 0x%04" PRIx16 " '%s', value 0x%" PRIx16
>  fw_cfg_add_i32(uint16_t key_value, const char *key_name, uint32_t value) 
> "key 0x%04" PRIx16 " '%s', value 0x%" PRIx32
>  fw_cfg_add_i64(uint16_t key_value, const char *key_name, uint64_t value) 
> "key 0x%04" PRIx16 " '%s', value 0x%" PRIx64
> +
> +# mac_nvram.c
> +macio_nvram_read(uint32_t addr, uint8_t val) "read addr=0x%04"PRIx32" 
> val=0x%02x"
> +macio_nvram_write(uint32_t addr, uint8_t val) "write addr=0x%04"PRIx32" 
> val=0x%02x"

-- 
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: [PATCH v3] spapr: Add a new level of NUMA for GPUs

[David Gibson]

On Fri, May 22, 2020 at 03:08:56PM -0500, Reza Arbab wrote:
> On Fri, May 22, 2020 at 02:53:33PM -0500, Reza Arbab wrote:
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -889,10 +889,16 @@ static int spapr_dt_rng(void *fdt)
> > static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
> > {
> > MachineState *ms = MACHINE(spapr);
> > +SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
> > int rtas;
> > GString *hypertas = g_string_sized_new(256);
> > GString *qemu_hypertas = g_string_sized_new(256);
> > -uint32_t refpoints[] = { cpu_to_be32(0x4), cpu_to_be32(0x4) };
> > +uint32_t refpoints[] = {
> > +cpu_to_be32(0x4),
> > +cpu_to_be32(0x4),
> > +cpu_to_be32(0x2),
> > +};
> > +uint32_t nr_refpoints = 3;
> 
> Gah, I soon as I hit send I realize this should be
> 
> uint32_t nr_refpoints = ARRAY_SIZE(refpoints);
> 
> Can you fixup or should I send a v4?

I had one other comment that needs addressing, so you might as well
send a v4.

> 

-- 
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: [PATCH] configure: Do not ignore malloc value

[Philippe Mathieu-Daudé]

On 5/25/20 12:12 AM, Leonid Bloch wrote:
> From: Leonid Bloch 
> 
> Not checking the value of malloc will cause a warning with GCC 10.1,
> which may result in configuration failure, with the following line in
> config.log:
> 
> config-temp/qemu-conf.c:2:18: error: ignoring return value of ‘malloc’ 
> declared with attribute ‘warn_unused_result’ [-Werror=unused-result]
> 2 | int main(void) { malloc(1); return 0; }
>   |  ^

Do we want to add -Wno-unused-result to compile_prog()?

> 
> Signed-off-by: Leonid Bloch 
> ---
>  configure | 20 +---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/configure b/configure
> index 2fc05c4465..d0220b2944 100755
> --- a/configure
> +++ b/configure
> @@ -4567,7 +4567,13 @@ fi
>  if test "$tcmalloc" = "yes" ; then
>cat > $TMPC << EOF
>  #include 
> -int main(void) { malloc(1); return 0; }
> +int main(void) {
> +void *tmp = malloc(1);
> +if (tmp != NULL) {
> +return 0;
> +}
> +return 1;
> +}
>  EOF
>  
>if compile_prog "" "-ltcmalloc" ; then
> @@ -4583,7 +4589,13 @@ fi
>  if test "$jemalloc" = "yes" ; then
>cat > $TMPC << EOF
>  #include 
> -int main(void) { malloc(1); return 0; }
> +int main(void) {
> +void *tmp = malloc(1);
> +if (tmp != NULL) {
> +return 0;
> +}
> +return 1;
> +}
>  EOF
>  
>if compile_prog "" "-ljemalloc" ; then
> @@ -6144,7 +6156,9 @@ if test "$sanitizers" = "yes" ; then
>  #include 
>  int main(void) {
>  void *tmp = malloc(10);
> -return *(int *)(tmp + 2);
> +if (tmp != NULL) {
> +return *(int *)(tmp + 2);
> +}
>  }
>  EOF
>if compile_prog "$CPU_CFLAGS -Werror -fsanitize=undefined" ""; then
> 

Re: [PATCH 3/5] vhost-user-blk: add VIRTIO_F_RING_PACKED feature bit

[Raphael Norwitz]

On Fri, May 22, 2020 at 1:20 PM Stefan Hajnoczi  wrote:
>
> Vhost devices have a list of feature bits that the device backend is
> allowed to control. The VIRTIO_F_RING_PACKED feature is a feature that
> must be negotiated through all the way to the device backend. Add it so
> the device backend can declare whether or not it supports the packed
> ring layout.
>
> Signed-off-by: Stefan Hajnoczi 

Reviewed-by: Raphael Norwitz 

> ---
>  hw/block/vhost-user-blk.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 9d8c0b3909..10e114a19a 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -44,6 +44,7 @@ static const int user_feature_bits[] = {
>  VIRTIO_BLK_F_DISCARD,
>  VIRTIO_BLK_F_WRITE_ZEROES,
>  VIRTIO_F_VERSION_1,
> +VIRTIO_F_RING_PACKED,
>  VIRTIO_RING_F_INDIRECT_DESC,
>  VIRTIO_RING_F_EVENT_IDX,
>  VIRTIO_F_NOTIFY_ON_EMPTY,
> --
> 2.25.3
>

Re: [PATCH 4/5] vhost-scsi: add VIRTIO_F_VERSION_1 and VIRTIO_F_RING_PACKED

[Raphael Norwitz]

On Fri, May 22, 2020 at 1:19 PM Stefan Hajnoczi  wrote:
>
> Let vhost-scsi and vhost-user-scsi device backends determine whether
> VIRTIO 1.0 and packed virtqueues are supported. It doesn't make sense to
> handle these feature bits in QEMU since the device backend needs to
> support them if we want to use them.
>
> Signed-off-by: Stefan Hajnoczi 

Reviewed-by: Raphael Norwitz 

> ---
>  hw/scsi/vhost-scsi.c  | 2 ++
>  hw/scsi/vhost-user-scsi.c | 2 ++
>  2 files changed, 4 insertions(+)
>
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index c1b012aea4..a7fb788af5 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -34,6 +34,8 @@
>
>  /* Features supported by host kernel. */
>  static const int kernel_feature_bits[] = {
> +VIRTIO_F_VERSION_1,
> +VIRTIO_F_RING_PACKED,
>  VIRTIO_F_NOTIFY_ON_EMPTY,
>  VIRTIO_RING_F_INDIRECT_DESC,
>  VIRTIO_RING_F_EVENT_IDX,
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index cbb5d97599..6aa0d5ded2 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -32,6 +32,8 @@
>
>  /* Features supported by the host application */
>  static const int user_feature_bits[] = {
> +VIRTIO_F_VERSION_1,
> +VIRTIO_F_RING_PACKED,
>  VIRTIO_F_NOTIFY_ON_EMPTY,
>  VIRTIO_RING_F_INDIRECT_DESC,
>  VIRTIO_RING_F_EVENT_IDX,
> --
> 2.25.3
>

Re: [PATCH v3 2/2] vhost-user-blk: delay vhost_user_blk_disconnect

[Raphael Norwitz]

I'm mostly happy with this. A couple comments.

On Wed, May 20, 2020 at 11:54 AM Dima Stepanov  wrote:
>
> A socket write during vhost-user communication may trigger a disconnect
> event, calling vhost_user_blk_disconnect() and clearing all the
> vhost_dev structures holding data that vhost-user functions expect to
> remain valid to roll back initialization correctly. Delay the cleanup to
> keep vhost_dev structure valid.
> There are two possible states to handle:
> 1. RUN_STATE_PRELAUNCH: skip bh oneshot call and perform disconnect in
> the caller routine.
> 2. RUN_STATE_RUNNING: delay by using bh
>
> BH changes are based on the similar changes for the vhost-user-net
> device:
>   commit e7c83a885f865128ae3cf1946f8cb538b63cbfba
>   "vhost-user: delay vhost_user_stop"
>
I'd also give credit to Li Feng here - he sent a similar patch:

https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg02255.html

> Signed-off-by: Dima Stepanov 
> ---
>  hw/block/vhost-user-blk.c | 49 
> +--
>  1 file changed, 43 insertions(+), 6 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 9d8c0b3..447fc9c 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -337,11 +337,6 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
>  VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>  VHostUserBlk *s = VHOST_USER_BLK(vdev);
>
> -if (!s->connected) {
> -return;
> -}
> -s->connected = false;
> -
>  if (s->dev.started) {
>  vhost_user_blk_stop(vdev);
>  }
> @@ -349,6 +344,19 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
>  vhost_dev_cleanup(&s->dev);
>  }
>
> +static void vhost_user_blk_event(void *opaque, QEMUChrEvent event);
> +
> +static void vhost_user_blk_chr_closed_bh(void *opaque)
> +{
> +DeviceState *dev = opaque;
> +VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +VHostUserBlk *s = VHOST_USER_BLK(vdev);
> +
> +vhost_user_blk_disconnect(dev);
> +qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
> +NULL, opaque, NULL, true);
> +}
> +
>  static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
>  {
>  DeviceState *dev = opaque;
> @@ -363,7 +371,28 @@ static void vhost_user_blk_event(void *opaque, 
> QEMUChrEvent event)
>  }
>  break;
>  case CHR_EVENT_CLOSED:
> -vhost_user_blk_disconnect(dev);
> +/*
> + * A close event may happen during a read/write, but vhost
> + * code assumes the vhost_dev remains setup, so delay the
> + * stop & clear. There are two possible paths to hit this
> + * disconnect event:
> + * 1. When VM is in the RUN_STATE_PRELAUNCH state. The
> + * vhost_user_blk_device_realize() is a caller.
> + * 2. In tha main loop phase after VM start.
> + *
> + * For p2 the disconnect event will be delayed. We can't
> + * do the same for p1, because we are not running the loop
> + * at this moment. So just skip this step and perform
> + * disconnect in the caller function.
> + */
> +if (s->connected && runstate_is_running()) {
> +AioContext *ctx = qemu_get_current_aio_context();
> +
> +qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
> +NULL, NULL, false);
> +aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, 
> opaque);
> +}
> +s->connected = false;
>  break;
>  case CHR_EVENT_BREAK:
>  case CHR_EVENT_MUX_IN:
> @@ -428,6 +457,14 @@ reconnect:
>
>  ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
> sizeof(struct virtio_blk_config));

I find checking s->connected before ret a little confusing. I think we
should also enforce a reconnect if s->connected is false. AFIK if
s->connected is false, ret must also be less than 0, but to be safe
I’d prefer something like:

if (ret < 0 || !s->connected) {
if (!s->connected) {
/*
 * Perform disconnect before making reconnect. More detailed
 * comment why it was delayed is in the vhost_user_blk_event()
 * routine.
 */
  vhost_user_blk_disconnect(dev);
}
if (ret < 0) {
   error_report(“vhost-user-blk: get block config failed”);
}
goto reconnect;
}

> +if (!s->connected) {
> +/*
> + * Perform disconnect before making reconnect. More detailed
> + * comment why it was delayed is in the vhost_user_blk_event()
> + * routine.
> + */
> +vhost_user_blk_disconnect(dev);
> +}
>  if (ret < 0) {
>  error_report("vhost-user-blk: get block config failed");
>  goto reconnect;
> --
> 2.7.4
>
>

[PATCH 4/4] hw/display/xlnx_dp: Replace disabled DPRINTF() by error_report()

[Philippe Mathieu-Daudé]

DPRINTF() calls are disabled by default, so when unexpected
data is used, the whole process abort without information.

Display a bit of information with error_report() before crashing.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/display/xlnx_dp.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
index 3e5fb44e06..8d940cd8d1 100644
--- a/hw/display/xlnx_dp.c
+++ b/hw/display/xlnx_dp.c
@@ -1,5 +1,5 @@
 /*
- * xlnx_dp.c
+ * Xilinx Display Port
  *
  *  Copyright (C) 2015 : GreenSocs Ltd
  *  http://www.greensocs.com/ , email: i...@greensocs.com
@@ -24,6 +24,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qemu/error-report.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "hw/display/xlnx_dp.h"
@@ -465,7 +466,7 @@ static uint8_t xlnx_dp_aux_pop_tx_fifo(XlnxDPState *s)
 uint8_t ret;
 
 if (fifo8_is_empty(&s->tx_fifo)) {
-DPRINTF("tx_fifo underflow..\n");
+error_report("%s: TX_FIFO underflow", __func__);
 abort();
 }
 ret = fifo8_pop(&s->tx_fifo);
@@ -525,6 +526,7 @@ static void xlnx_dp_aux_set_command(XlnxDPState *s, 
uint32_t value)
 qemu_log_mask(LOG_UNIMP, "xlnx_dp: Write i2c status not 
implemented\n");
 break;
 default:
+error_report("%s: invalid command: %u", __func__, cmd);
 abort();
 }
 
@@ -631,8 +633,8 @@ static void xlnx_dp_change_graphic_fmt(XlnxDPState *s)
 s->g_plane.format = PIXMAN_b8g8r8;
 break;
 default:
-DPRINTF("error: unsupported graphic format %u.\n",
-s->avbufm_registers[AV_BUF_FORMAT] & DP_GRAPHIC_MASK);
+error_report("%s: unsupported graphic format %u", __func__,
+ s->avbufm_registers[AV_BUF_FORMAT] & DP_GRAPHIC_MASK);
 abort();
 }
 
@@ -647,8 +649,8 @@ static void xlnx_dp_change_graphic_fmt(XlnxDPState *s)
 s->v_plane.format = PIXMAN_x8b8g8r8;
 break;
 default:
-DPRINTF("error: unsupported video format %u.\n",
-s->avbufm_registers[AV_BUF_FORMAT] & DP_NL_VID_FMT_MASK);
+error_report("%s: unsupported video format %u", __func__,
+ s->avbufm_registers[AV_BUF_FORMAT] & DP_NL_VID_FMT_MASK);
 abort();
 }
 
-- 
2.21.3

[PATCH 2/4] hw/display/dpcd: Fix memory region size

[Philippe Mathieu-Daudé]

The memory region size is 512K.

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

diff --git a/hw/display/dpcd.c b/hw/display/dpcd.c
index 170545c605..0c1b7b35fb 100644
--- a/hw/display/dpcd.c
+++ b/hw/display/dpcd.c
@@ -1,5 +1,5 @@
 /*
- * dpcd.c
+ * Xilinx Display Port Control Data
  *
  *  Copyright (C) 2015 : GreenSocs Ltd
  *  http://www.greensocs.com/ , email: i...@greensocs.com
@@ -137,7 +137,7 @@ static void dpcd_init(Object *obj)
 {
 DPCDState *s = DPCD(obj);
 
-memory_region_init_io(&s->iomem, obj, &aux_ops, s, TYPE_DPCD, 0x7);
+memory_region_init_io(&s->iomem, obj, &aux_ops, s, TYPE_DPCD, 0x8);
 aux_init_mmio(AUX_SLAVE(obj), &s->iomem);
 }
 
-- 
2.21.3

[PATCH 3/4] hw/display/dpcd: Convert debug printf()s to trace events

[Philippe Mathieu-Daudé]

Convert DPRINTF() to trace events and remove ifdef'ry.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/display/dpcd.c   | 16 +++-
 hw/display/trace-events |  4 
 2 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/hw/display/dpcd.c b/hw/display/dpcd.c
index 0c1b7b35fb..64463654a1 100644
--- a/hw/display/dpcd.c
+++ b/hw/display/dpcd.c
@@ -32,16 +32,7 @@
 #include "hw/misc/auxbus.h"
 #include "migration/vmstate.h"
 #include "hw/display/dpcd.h"
-
-#ifndef DEBUG_DPCD
-#define DEBUG_DPCD 0
-#endif
-
-#define DPRINTF(fmt, ...) do { 
\
-if (DEBUG_DPCD) {  
\
-qemu_log("dpcd: " fmt, ## __VA_ARGS__);
\
-}  
\
-} while (0)
+#include "trace.h"
 
 #define DPCD_READABLE_AREA  0x600
 
@@ -70,8 +61,8 @@ static uint64_t dpcd_read(void *opaque, hwaddr offset, 
unsigned size)
offset);
 ret = 0;
 }
+trace_dpcd_read(offset, ret);
 
-DPRINTF("read 0x%" PRIX8 " @0x%" HWADDR_PRIX "\n", ret, offset);
 return ret;
 }
 
@@ -80,8 +71,7 @@ static void dpcd_write(void *opaque, hwaddr offset, uint64_t 
value,
 {
 DPCDState *e = DPCD(opaque);
 
-DPRINTF("write 0x%" PRIX8 " @0x%" HWADDR_PRIX "\n", (uint8_t)value, 
offset);
-
+trace_dpcd_write(offset, value);
 if (offset < DPCD_READABLE_AREA) {
 e->dpcd_info[offset] = value;
 } else {
diff --git a/hw/display/trace-events b/hw/display/trace-events
index e6e22bef88..00543f1601 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -151,3 +151,7 @@ artist_vram_write(unsigned int size, uint64_t addr, 
uint64_t val) "%u 0x%"PRIx64
 artist_fill_window(unsigned int start_x, unsigned int start_y, unsigned int 
width, unsigned int height, uint32_t op, uint32_t ctlpln) "start=%ux%u 
length=%ux%u op=0x%08x ctlpln=0x%08x"
 artist_block_move(unsigned int start_x, unsigned int start_y, unsigned int 
dest_x, unsigned int dest_y, unsigned int width, unsigned int height) "source 
%ux%u -> dest %ux%u size %ux%u"
 artist_draw_line(unsigned int start_x, unsigned int start_y, unsigned int 
end_x, unsigned int end_y) "%ux%u %ux%u"
+
+# dpcd.c
+dpcd_read(uint32_t addr, uint8_t val) "read addr:0x%"PRIx32" val:0x%02x"
+dpcd_write(uint32_t addr, uint8_t val) "write addr:0x%"PRIx32" val:0x%02x"
-- 
2.21.3

[PATCH 1/4] hw/misc/auxbus: Use qemu_log_mask(UNIMP) instead of debug printf

[Philippe Mathieu-Daudé]

Convert the deprecated DPRINTF() call by qemu_log_mask(LOG_UNIMP).

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/misc/auxbus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
index f8e7b97971..06aabf20c5 100644
--- a/hw/misc/auxbus.c
+++ b/hw/misc/auxbus.c
@@ -196,7 +196,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t 
address,
 }
 break;
 default:
-DPRINTF("Not implemented!\n");
+qemu_log_mask(LOG_UNIMP, "AUX cmd=%u not implemented\n", cmd);
 return AUX_NACK;
 }
 
-- 
2.21.3

[PATCH 0/4] hw/display/xlnx_dp: Fix memory region size, improve logging

[Philippe Mathieu-Daudé]

A collection of cleanup patches written while fuzzing
the Xilinx Display Port device.

Philippe Mathieu-Daudé (4):
  hw/misc/auxbus: Use qemu_log_mask(UNIMP) instead of debug printf
  hw/display/dpcd: Fix memory region size
  hw/display/dpcd: Convert debug printf()s to trace events
  hw/display/xlnx_dp: Replace disabled DPRINTF() by error_report()

 hw/display/dpcd.c   | 20 +---
 hw/display/xlnx_dp.c| 14 --
 hw/misc/auxbus.c|  2 +-
 hw/display/trace-events |  4 
 4 files changed, 18 insertions(+), 22 deletions(-)

-- 
2.21.3

[PATCH] hw/pci-bridge/dec: Remove dead debug code

[Philippe Mathieu-Daudé]

Remove debug code never used since added in commit e1c6bbabee30.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/pci-bridge/dec.c | 10 --
 1 file changed, 10 deletions(-)

diff --git a/hw/pci-bridge/dec.c b/hw/pci-bridge/dec.c
index 3ae2f788a4..952bc71122 100644
--- a/hw/pci-bridge/dec.c
+++ b/hw/pci-bridge/dec.c
@@ -32,16 +32,6 @@
 #include "hw/pci/pci_bridge.h"
 #include "hw/pci/pci_bus.h"
 
-/* debug DEC */
-//#define DEBUG_DEC
-
-#ifdef DEBUG_DEC
-#define DEC_DPRINTF(fmt, ...)   \
-do { printf("DEC: " fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DEC_DPRINTF(fmt, ...)
-#endif
-
 #define DEC_21154(obj) OBJECT_CHECK(DECState, (obj), TYPE_DEC_21154)
 
 typedef struct DECState {
-- 
2.21.3

Re: [PATCH v3 2/2] vhost-user-blk: delay vhost_user_blk_disconnect

[Jason Wang]

On 2020/5/20 下午11:53, Dima Stepanov wrote:

A socket write during vhost-user communication may trigger a disconnect
event, calling vhost_user_blk_disconnect() and clearing all the
vhost_dev structures holding data that vhost-user functions expect to
remain valid to roll back initialization correctly. Delay the cleanup to
keep vhost_dev structure valid.
There are two possible states to handle:
1. RUN_STATE_PRELAUNCH: skip bh oneshot call and perform disconnect in
the caller routine.
2. RUN_STATE_RUNNING: delay by using bh

BH changes are based on the similar changes for the vhost-user-net
device:
   commit e7c83a885f865128ae3cf1946f8cb538b63cbfba
   "vhost-user: delay vhost_user_stop"



It's better to explain why we don't need to deal with case 1 in the 
vhost-user-net case.


And do we still need patch 1 if we had this patch??

Thanks




Signed-off-by: Dima Stepanov 
---
  hw/block/vhost-user-blk.c | 49 +--
  1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 9d8c0b3..447fc9c 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -337,11 +337,6 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
  VirtIODevice *vdev = VIRTIO_DEVICE(dev);
  VHostUserBlk *s = VHOST_USER_BLK(vdev);
  
-if (!s->connected) {

-return;
-}
-s->connected = false;
-
  if (s->dev.started) {
  vhost_user_blk_stop(vdev);
  }
@@ -349,6 +344,19 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
  vhost_dev_cleanup(&s->dev);
  }
  
+static void vhost_user_blk_event(void *opaque, QEMUChrEvent event);

+
+static void vhost_user_blk_chr_closed_bh(void *opaque)
+{
+DeviceState *dev = opaque;
+VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+VHostUserBlk *s = VHOST_USER_BLK(vdev);
+
+vhost_user_blk_disconnect(dev);
+qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
+NULL, opaque, NULL, true);
+}
+
  static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
  {
  DeviceState *dev = opaque;
@@ -363,7 +371,28 @@ static void vhost_user_blk_event(void *opaque, 
QEMUChrEvent event)
  }
  break;
  case CHR_EVENT_CLOSED:
-vhost_user_blk_disconnect(dev);
+/*
+ * A close event may happen during a read/write, but vhost
+ * code assumes the vhost_dev remains setup, so delay the
+ * stop & clear. There are two possible paths to hit this
+ * disconnect event:
+ * 1. When VM is in the RUN_STATE_PRELAUNCH state. The
+ * vhost_user_blk_device_realize() is a caller.
+ * 2. In tha main loop phase after VM start.
+ *
+ * For p2 the disconnect event will be delayed. We can't
+ * do the same for p1, because we are not running the loop
+ * at this moment. So just skip this step and perform
+ * disconnect in the caller function.
+ */
+if (s->connected && runstate_is_running()) {
+AioContext *ctx = qemu_get_current_aio_context();
+
+qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
+NULL, NULL, false);
+aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
+}
+s->connected = false;
  break;
  case CHR_EVENT_BREAK:
  case CHR_EVENT_MUX_IN:
@@ -428,6 +457,14 @@ reconnect:
  
  ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,

 sizeof(struct virtio_blk_config));
+if (!s->connected) {
+/*
+ * Perform disconnect before making reconnect. More detailed
+ * comment why it was delayed is in the vhost_user_blk_event()
+ * routine.
+ */
+vhost_user_blk_disconnect(dev);
+}
  if (ret < 0) {
  error_report("vhost-user-blk: get block config failed");
  goto reconnect;

Re: [PATCH] scripts/clean-includes: Mark 'qemu/qemu-plugin.h' as special header

[Emilio G. Cota]

On Sun, May 24, 2020 at 23:56:54 +0200, Philippe Mathieu-Daudé wrote:
> "qemu/qemu-plugin.h" isn't meant to be include by QEMU codebase,
> but by 3rd party plugins that QEMU can use. These plugins can be
> built out of QEMU and don't include "qemu/osdep.h".
> Mark "qemu/qemu-plugin.h" as a special header that doesn't need
> to be cleaned for "qemu/osdep.h".
> 
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Emilio G. Cota 

Thanks,
Emilio

Re: [PATCH V2 0/7] Latest COLO tree queued patches

[Jason Wang]

On 2020/5/22 下午3:53, Zhang Chen wrote:

From: Zhang Chen 

Hi Jason, this series include latest COLO related patches.
I have finish basic test and review.
If no other comments, please check and merge this series.

Derek Su (1):
   colo-compare: Fix memory leak in packet_enqueue()

Lukas Straub (6):
   net/colo-compare.c: Create event_bh with the right AioContext
   chardev/char.c: Use qemu_co_sleep_ns if in coroutine
   net/colo-compare.c: Fix deadlock in compare_chr_send
   net/colo-compare.c: Only hexdump packets if tracing is enabled
   net/colo-compare.c: Check that colo-compare is active
   net/colo-compare.c: Correct ordering in complete and finalize

  chardev/char.c |   7 +-
  net/colo-compare.c | 277 +
  net/colo.c |   7 ++
  net/colo.h |   1 +
  net/trace-events   |   1 +
  5 files changed, 222 insertions(+), 71 deletions(-)Philippe



Applied with the following tweaks:

- reword the commit log of patch 2 as  Philippe suggested
- a missing ack from Philippe in patch 7

Thanks

[Bug 1879425] Re: The thread of "CPU 0 /KVM" keeping 99.9%CPU

[cliff chen]

Add the ticket link in centos
https://bugs.centos.org/view.php?id=17385

** Bug watch added: bugs.centos.org/ #17385
   https://bugs.centos.org/view.php?id=17385

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1879425

Title:
  The thread of "CPU 0 /KVM" keeping 99.9%CPU

Status in QEMU:
  Incomplete

Bug description:
  Hi Expert:

  The VM is hung here after (2, or 3, or 5 and the longest time is 10 hours) by 
qemu-kvm.
  Notes:
  for VM:
    OS: RHEL8.1
    CPU: 1
    MEM:4G
  For qemu-kvm(host kernel RHEL7):
    1) version:
   /usr/libexec/qemu-kvm -version
   QEMU emulator version 2.10.0(qemu-kvm-ev-2.10.0-21.el7_5.4.1)
    2) once the issue is occurred, the CPU of "CPU0 /KVM" is more than 99% by 
com "top -p VM_pro_ID"
  PID  UDER   PR NI RES   S  % CPU %MEM  TIME+COMMAND
  872067   qemu   20 0  1.6g  R   99.9  0.6  37:08.87 CPU 0/KVM
    3) use "pstack 493307" and below is function trace
  Thread 1 (Thread 0x7f2572e73040 (LWP 872067)):
  #0  0x7f256cad8fcf in ppoll () from /lib64/libc.so.6
  #1  0x55ff34bdf4a9 in qemu_poll_ns ()
  #2  0x55ff34be02a8 in main_loop_wait ()
  #3  0x55ff348bfb1a in main ()
    4) use strace "strace -tt -ff -p 872067 -o cfx" and below log keep printing
  21:24:02.977833 ppoll([{fd=4, events=POLLIN}, {fd=6, events=POLLIN}, {fd=8, 
events=POLLIN}, {fd=9, events=POLLIN}, {fd=80, events=POLLIN}, {fd=82, 
events=POLLIN}, {fd=84, events=POLLIN}, {fd=115, events=POLLIN}, {fd=121, 
events=POLLIN}], 9, {0, 0}, NULL, 8) = 0 (Timeout)
  21:24:02.977918 ppoll([{fd=4, events=POLLIN}, {fd=6, events=POLLIN}, {fd=8, 
events=POLLIN}, {fd=9, events=POLLIN}, {fd=80, events=POLLIN}, {fd=82, 
events=POLLIN}, {fd=84, events=POLLIN}, {fd=115, events=POLLIN}, {fd=121, 
events=POLLIN}], 9, {0, 911447}, NULL, 8) = 0 (Timeout)
  21:24:02.978945 ppoll([{fd=4, events=POLLIN}, {fd=6, events=POLLIN}, {fd=8, 
events=POLLIN}, {fd=9, events=POLLIN}, {fd=80, events=POLLIN}, {fd=82, 
events=POLLIN}, {fd=84, events=POLLIN}, {fd=115, events=POLLIN}, {fd=121, 
events=POLLIN}], 9, {0, 0}, NULL, 8) = 0 (Timeout)
  Therefore, I think the thread "CPU 0/KVM" is in tight loop.
    5) use reset can recover this issue. however, it will reoccurred again.
  Current work around is increase one CPU for this VM, then issue is gone.

  thanks
  Cliff

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1879425/+subscriptions

Re: [PATCH v6 1/3] memory: drop guest writes to read-only ram device regions

[Yan Zhao]

On Thu, May 21, 2020 at 04:38:47PM +0200, Paolo Bonzini wrote:
> On 30/04/20 11:40, Peter Maydell wrote:
> >> This does not "drop" a write to a r/o region -- it causes it to generate
> >> whatever the guest architecture's equivalent of a bus error is (eg data
> >> abort on Arm).
> 
> 
> > More generally, this change seems a bit odd: currently we do not
> > check the mr->readonly flag here, but in general guests don't get
> > to write to ROM areas. Where is that check currently done
> 
> Writes to ROM are directed to mr->ops unassigned_mem_ops.  Because _all_
> ram-device reads and writes go through the ops, for ram-device we have
> to stick the check for mr->readonly in the ops.
> 
> On one hand, I was quite surprised to see that unassigned_mem_write does
> not return MEMTX_ERROR now that I looked at it.
> 
> On the other hand, we should use MEMTX_ERROR in patch 2 as well, if we
> decide it's the way to go.
> 
> (Sorry Yan for the late response).
> 
hi Paolo,
thanks for your reply and never mind :)

But there's one thing I just can't figure out the reason and eagerly need
your guide.

why do we have to convert all .write operations to .write_with_attrs and
return MEMTX_ERROR? because of the handling of writes to read-only region?

however, it seems that all regions have to handle this case, so ultimately
we have to convert all .write to .write_with_attrs and there would be no
.write operations any more?

Thanks
Yan

[PATCH] configure: Do not ignore malloc value

[Leonid Bloch]

From: Leonid Bloch 

Not checking the value of malloc will cause a warning with GCC 10.1,
which may result in configuration failure, with the following line in
config.log:

config-temp/qemu-conf.c:2:18: error: ignoring return value of ‘malloc’ declared 
with attribute ‘warn_unused_result’ [-Werror=unused-result]
2 | int main(void) { malloc(1); return 0; }
  |  ^

Signed-off-by: Leonid Bloch 
---
 configure | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index 2fc05c4465..d0220b2944 100755
--- a/configure
+++ b/configure
@@ -4567,7 +4567,13 @@ fi
 if test "$tcmalloc" = "yes" ; then
   cat > $TMPC << EOF
 #include 
-int main(void) { malloc(1); return 0; }
+int main(void) {
+void *tmp = malloc(1);
+if (tmp != NULL) {
+return 0;
+}
+return 1;
+}
 EOF
 
   if compile_prog "" "-ltcmalloc" ; then
@@ -4583,7 +4589,13 @@ fi
 if test "$jemalloc" = "yes" ; then
   cat > $TMPC << EOF
 #include 
-int main(void) { malloc(1); return 0; }
+int main(void) {
+void *tmp = malloc(1);
+if (tmp != NULL) {
+return 0;
+}
+return 1;
+}
 EOF
 
   if compile_prog "" "-ljemalloc" ; then
@@ -6144,7 +6156,9 @@ if test "$sanitizers" = "yes" ; then
 #include 
 int main(void) {
 void *tmp = malloc(10);
-return *(int *)(tmp + 2);
+if (tmp != NULL) {
+return *(int *)(tmp + 2);
+}
 }
 EOF
   if compile_prog "$CPU_CFLAGS -Werror -fsanitize=undefined" ""; then
-- 
2.26.2

[Bug 1810000] Re: qemu system emulator crashed with the attachment of usb-bt-dongle device

[BALATON Zoltan]

This issue has nothing to do with bluetooth rather something with xhci. I've 
got the same error while trying to pass through a usb device to a Windows VM, 
once the guest driver is loaded in the the assert fires so probably it's trying 
to send something that's not handled correctly. A similar (same?) issue is also 
mentioned in this bug tracker: 
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=849884

For me it happens with both qemu-xhci and nec-usb-xhci

Here are some debug infos I could gather, not sure if it helps:

qemu-system-x86_64: hw/usb/core.c:591: usb_packet_copy: Assertion 
`p->actual_length + bytes <= iov->size' failed.
(gdb) bt
#0  0x76b85a7a in raise () at /lib64/libc.so.6
#1  0x76b6e524 in abort () at /lib64/libc.so.6
#2  0x76b6e40f in _nl_load_domain.cold.0 () at /lib64/libc.so.6
#3  0x76b7a9a2 in  () at /lib64/libc.so.6
#4  0x55bd1c93 in usb_packet_copy (p=0x7fff680fc8f8, 
ptr=0x5772d63c, bytes=5) at hw/usb/core.c:591
#5  0x55bd10ba in usb_generic_async_ctrl_complete (s=0x5772d550, 
p=0x7fff680fc8f8) at hw/usb/core.c:332
#6  0x55c0dc54 in usb_host_req_complete_ctrl (xfer=0x7fff681a6430) at 
hw/usb/host-libusb.c:416
#7  0x77c68082 in  () at /lib64/libusb-1.0.so.0
#8  0x77c6bd1a in  () at /lib64/libusb-1.0.so.0
#9  0x77c6daa8 in  () at /lib64/libusb-1.0.so.0
#10 0x77c67a28 in  () at /lib64/libusb-1.0.so.0
#11 0x77c68b13 in libusb_handle_events_timeout_completed () at 
/lib64/libusb-1.0.so.0
#12 0x55c0d4a6 in usb_host_handle_fd (opaque=0x568eeb70) at 
hw/usb/host-libusb.c:226
#13 0x55e2099c in aio_dispatch_handler (ctx=0x56614530, 
node=0x570c06c0) at util/aio-posix.c:339
[...]
(gdb) up
#4  0x55bd1c93 in usb_packet_copy (p=0x7fff680fc8f8, 
ptr=0x5772d63c, bytes=5) at hw/usb/core.c:591
591 assert(p->actual_length + bytes <= iov->size);
(gdb) list
586 void usb_packet_copy(USBPacket *p, void *ptr, size_t bytes)
587 {
588 QEMUIOVector *iov = p->combined ? &p->combined->iov : &p->iov;
589 
590 assert(p->actual_length >= 0);
591 assert(p->actual_length + bytes <= iov->size);
(gdb) p/x *p
$3 = {pid = 0x69, id = 0x1a20f5c0, ep = 0x5772e650, stream = 0x0,
 iov = {iov = 0x7fff680fc200, niov = 0x0, {{nalloc = 0x1, local_iov = 
{iov_base = 0x0, iov_len = 0x0}}, {__pad = {
 0x1, 0x0 }, size = 0x0}}}, parameter = 0x50f000680, 
short_not_ok = 0x0, int_req = 0x1,
 status = 0x0, actual_length = 0x0, state = 0x3, combined = 0x0,
 queue = {tqe_next = 0x0, tqe_circ = {tql_next = 0x0, tql_prev = 
0x5772e668}},
 combined_entry = {tqe_next = 0x0, tqe_circ = {tql_next = 0x0, tql_prev = 
0x0}}}


** Bug watch added: Debian Bug tracker #849884
   https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=849884

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/181

Title:
  qemu system emulator crashed with the attachment of usb-bt-dongle
  device

Status in QEMU:
  New

Bug description:
  I am testing usb-bt-dongle device on xchi host controller, and found
  that the qemu crashed directly with an assertion failer.

  Here is the information to reproduce the crash:

  Qemu git revision: 9b2e891ec5ccdb4a7d583b77988848282606fdea
  System emulator: qemu-x86_64
  VM image: 
https://people.debian.org/~aurel32/qemu/amd64/debian_squeeze_amd64_desktop.qcow2
  CommandLine: qemu-system-x86_64 -M q35 -device qemu-xhci,id=xhci -enable-kvm 
-device usb-bt-dongle  -hda ./debian_wheezy_amd64_standard.qcow2

  Error message:

  qemu-system-x86_64: /build/qemu-
  Eap4uc/qemu-2.11+dfsg/hw/usb/core.c:592: usb_packet_copy: Assertion
  `p->actual_length + bytes <= iov->size' failed.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/181/+subscriptions

Re: [PATCH] qemu-plugin.h: add missing include to define size_t

[Philippe Mathieu-Daudé]

On 5/24/20 10:24 PM, Emilio G. Cota wrote:
> Signed-off-by: Emilio G. Cota 
> ---
>  include/qemu/qemu-plugin.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index 89ed579f55..bab8b0d4b3 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -12,6 +12,7 @@
>  
>  #include 
>  #include 
> +#include 
>  
>  /*
>   * For best performance, build the plugin with -fvisibility=hidden so that
> 

Reviewed-by: Philippe Mathieu-Daudé 

[PATCH] scripts/clean-includes: Mark 'qemu/qemu-plugin.h' as special header

[Philippe Mathieu-Daudé]

"qemu/qemu-plugin.h" isn't meant to be include by QEMU codebase,
but by 3rd party plugins that QEMU can use. These plugins can be
built out of QEMU and don't include "qemu/osdep.h".
Mark "qemu/qemu-plugin.h" as a special header that doesn't need
to be cleaned for "qemu/osdep.h".

Signed-off-by: Philippe Mathieu-Daudé 
---
 scripts/clean-includes | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/clean-includes b/scripts/clean-includes
index dd938daa3e..795b3bea31 100755
--- a/scripts/clean-includes
+++ b/scripts/clean-includes
@@ -123,6 +123,7 @@ for f in "$@"; do
   ;;
 *include/qemu/osdep.h | \
 *include/qemu/compiler.h | \
+*include/qemu/qemu-plugin.h | \
 *include/glib-compat.h | \
 *include/sysemu/os-posix.h | \
 *include/sysemu/os-win32.h | \
-- 
2.21.3

Re: [PATCH 0/7] hw/misc/empty_slot: Spring cleaning

[Peter Maydell]

On Sun, 24 May 2020 at 21:21, Philippe Mathieu-Daudé  wrote:
> On 5/24/20 9:37 PM, Aleksandar Markovic wrote:
> > I agree with all of your patches, they absolutely make sense to me,
> > but I would like to know Peter's opinion on such treatment of empty
> > slots.
> >
> > I am going to give r-bs and integrate mips patches as soon as Peter
> > OKs the general approach. So, Peter, is Philippe's approach to empty
> > slots fine?
>
> Thanks Aleksandar for looking at this series.
>
> I expect a neutral opinion from Peter.

Yes. In particular I cannot be the bottleneck for all design
choices in QEMU: that doesn't scale. Mostly you can assume
that if I have a strong opinion then I'll provide it, and
otherwise I prefer it if the people who care enough to
maintain code and write patches to tidy it up make the choices.

(Empty slots seemed a bit odd to me last time I looked at them,
but if they work for mips and sparc that's fine with me.)

thanks
-- PMM

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/cover.1590344541.git.lukasstra...@web.de/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

migration/qemu-file-channel.o: in function `channel_close':
/tmp/qemu-test/src/migration/qemu-file-channel.c:111: undefined reference to 
`yank_generic_iochannel'
/usr/bin/ld: /tmp/qemu-test/src/migration/qemu-file-channel.c:110: undefined 
reference to `yank_unregister_function'
clang-8: error: linker command failed with exit code 1 (use -v to see 
invocation)
make: *** [/tmp/qemu-test/src/rules.mak:124: tests/test-vmstate] Error 1
make: *** Waiting for unfinished jobs
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in 
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=18e540c0fae14ac68e571023cc096e31', '-u', 
'1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 
'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 
'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', 
'-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-92q_wrsw/src/docker-src.2020-05-24-17.15.58.27154:/var/tmp/qemu:z,ro',
 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=18e540c0fae14ac68e571023cc096e31
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-92q_wrsw/src'
make: *** [docker-run-test-debug@fedora] Error 2

real5m14.699s
user0m8.970s


The full log is available at
http://patchew.org/logs/cover.1590344541.git.lukasstra...@web.de/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/cover.1590344541.git.lukasstra...@web.de/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

migration/qemu-file-channel.o: In function `channel_close':
/tmp/qemu-test/src/migration/qemu-file-channel.c:110: undefined reference to 
`yank_generic_iochannel'
/tmp/qemu-test/src/migration/qemu-file-channel.c:110: undefined reference to 
`yank_unregister_function'
collect2: error: ld returned 1 exit status
  LINKtests/test-cutils
make: *** [tests/test-vmstate] Error 1
make: *** Waiting for unfinished jobs
  TESTiotest-qcow2: 001
  TESTiotest-qcow2: 002
---
Not run: 259
Failures: 267
Failed 1 of 119 iotests
make: *** [check-tests/check-block.sh] Error 1
make: *** wait: No child processes.  Stop.
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in 
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=db483e1a7b374ed8a9cf3528ad2357b2', '-u', 
'1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
'-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 
'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
'/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-4gmncabb/src/docker-src.2020-05-24-17.04.07.24804:/var/tmp/qemu:z,ro',
 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=db483e1a7b374ed8a9cf3528ad2357b2
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-4gmncabb/src'
make: *** [docker-run-test-quick@centos7] Error 2

real12m5.130s
user0m8.082s


The full log is available at
http://patchew.org/logs/cover.1590344541.git.lukasstra...@web.de/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

[PATCH] qemu-plugin.h: add missing include to define size_t

[Emilio G. Cota]

Signed-off-by: Emilio G. Cota 
---
 include/qemu/qemu-plugin.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 89ed579f55..bab8b0d4b3 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -12,6 +12,7 @@
 
 #include 
 #include 
+#include 
 
 /*
  * For best performance, build the plugin with -fvisibility=hidden so that
-- 
2.25.1

Re: [PATCH 0/7] hw/misc/empty_slot: Spring cleaning

[Philippe Mathieu-Daudé]

On 5/24/20 9:37 PM, Aleksandar Markovic wrote:
> нед, 24. мај 2020. у 18:58 Philippe Mathieu-Daudé 
> је написао/ла:
>>
>> ping?
>>
> 
> I agree with all of your patches, they absolutely make sense to me,
> but I would like to know Peter's opinion on such treatment of empty
> slots.
> 
> I am going to give r-bs and integrate mips patches as soon as Peter
> OKs the general approach. So, Peter, is Philippe's approach to empty
> slots fine?

Thanks Aleksandar for looking at this series.

I expect a neutral opinion from Peter.

What would be helpful is feedback from Artyom, since it authored this
device.

Artyom, do you mind Acking the series?

Thanks,

Phil.

> 
> Aleksandar
> 
>> On 5/10/20 5:28 PM, Philippe Mathieu-Daudé wrote:
>>> New Spring, new opportunity to clean this device :)
>>> (v1 was in 2018, v2 in 2019).
>>>
>>> - lower device priority
>>> - follow qdev model and use properties
>>> - convert to trace events
>>> - describe with slot name
>>> - move under hw/misc/ and cover in MAINTAINERS
>>>
>>> Peter, I hope you are OK adding it wit UNIMP device,
>>> as both are very similar, and don't have much activity.
>>>
>>> Only MIPS/SPARC32 targets use this device.
>>>
>>> v2: https://www.mail-archive.com/qemu-devel@nongnu.org/msg626498.html
>>> v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg564060.html
>>>
>>> Philippe Mathieu-Daudé (7):
>>>   hw/sparc/sun4m: Use UnimplementedDevice for I/O devices
>>>   hw/misc/empty_slot: Lower address space priority
>>>   hw/misc/empty_slot: Convert 'size' field as qdev property
>>>   hw/misc/empty_slot: Add a 'name' qdev property
>>>   hw/misc/empty_slot: Convert debug printf() to trace event
>>>   hw/misc/empty_slot: Move the 'hw/misc' and cover in MAINTAINERS
>>>   hw/misc/empty_slot: Name the slots when created
>>>
>>>  include/hw/empty_slot.h|  9 ---
>>>  include/hw/misc/empty_slot.h   | 19 ++
>>>  hw/mips/mips_malta.c   |  4 +--
>>>  hw/{core => misc}/empty_slot.c | 47 +++---
>>>  hw/sparc/sun4m.c   | 23 +++--
>>>  MAINTAINERS|  4 ++-
>>>  hw/core/Makefile.objs  |  1 -
>>>  hw/misc/Makefile.objs  |  1 +
>>>  hw/misc/trace-events   |  4 +++
>>>  9 files changed, 70 insertions(+), 42 deletions(-)
>>>  delete mode 100644 include/hw/empty_slot.h
>>>  create mode 100644 include/hw/misc/empty_slot.h
>>>  rename hw/{core => misc}/empty_slot.c (66%)
>>>
>>
> 

Re: [PATCH 0/7] hw/misc/empty_slot: Spring cleaning

[Aleksandar Markovic]

нед, 24. мај 2020. у 18:58 Philippe Mathieu-Daudé 
је написао/ла:
>
> ping?
>

I agree with all of your patches, they absolutely make sense to me,
but I would like to know Peter's opinion on such treatment of empty
slots.

I am going to give r-bs and integrate mips patches as soon as Peter
OKs the general approach. So, Peter, is Philippe's approach to empty
slots fine?

Aleksandar

> On 5/10/20 5:28 PM, Philippe Mathieu-Daudé wrote:
> > New Spring, new opportunity to clean this device :)
> > (v1 was in 2018, v2 in 2019).
> >
> > - lower device priority
> > - follow qdev model and use properties
> > - convert to trace events
> > - describe with slot name
> > - move under hw/misc/ and cover in MAINTAINERS
> >
> > Peter, I hope you are OK adding it wit UNIMP device,
> > as both are very similar, and don't have much activity.
> >
> > Only MIPS/SPARC32 targets use this device.
> >
> > v2: https://www.mail-archive.com/qemu-devel@nongnu.org/msg626498.html
> > v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg564060.html
> >
> > Philippe Mathieu-Daudé (7):
> >   hw/sparc/sun4m: Use UnimplementedDevice for I/O devices
> >   hw/misc/empty_slot: Lower address space priority
> >   hw/misc/empty_slot: Convert 'size' field as qdev property
> >   hw/misc/empty_slot: Add a 'name' qdev property
> >   hw/misc/empty_slot: Convert debug printf() to trace event
> >   hw/misc/empty_slot: Move the 'hw/misc' and cover in MAINTAINERS
> >   hw/misc/empty_slot: Name the slots when created
> >
> >  include/hw/empty_slot.h|  9 ---
> >  include/hw/misc/empty_slot.h   | 19 ++
> >  hw/mips/mips_malta.c   |  4 +--
> >  hw/{core => misc}/empty_slot.c | 47 +++---
> >  hw/sparc/sun4m.c   | 23 +++--
> >  MAINTAINERS|  4 ++-
> >  hw/core/Makefile.objs  |  1 -
> >  hw/misc/Makefile.objs  |  1 +
> >  hw/misc/trace-events   |  4 +++
> >  9 files changed, 70 insertions(+), 42 deletions(-)
> >  delete mode 100644 include/hw/empty_slot.h
> >  create mode 100644 include/hw/misc/empty_slot.h
> >  rename hw/{core => misc}/empty_slot.c (66%)
> >
>

[PATCH v3 4/4] migration: Add yank feature

[Lukas Straub]

Register yank functions on sockets to shut them down.

Signed-off-by: Lukas Straub 
---
 Makefile.objs |  1 +
 migration/channel.c   | 12 
 migration/migration.c | 18 +-
 migration/multifd.c   | 10 ++
 migration/qemu-file-channel.c |  6 ++
 5 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/Makefile.objs b/Makefile.objs
index 5582f4eda9..d2a49d3834 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -58,6 +58,7 @@ common-obj-$(CONFIG_LINUX) += fsdev/

 common-obj-y += accel/
 common-obj-y += migration/
+common-obj-y += yank.o

 common-obj-y += audio/
 common-obj-m += audio/
diff --git a/migration/channel.c b/migration/channel.c
index 20e4c8e2dc..5cb48d403a 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -18,6 +18,8 @@
 #include "trace.h"
 #include "qapi/error.h"
 #include "io/channel-tls.h"
+#include "io/channel-socket.h"
+#include "yank.h"

 /**
  * @migration_channel_process_incoming - Create new incoming migration channel
@@ -35,6 +37,11 @@ void migration_channel_process_incoming(QIOChannel *ioc)
 trace_migration_set_incoming_channel(
 ioc, object_get_typename(OBJECT(ioc)));

+if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)) {
+yank_register_function((char *) "migration", yank_generic_iochannel,
+   QIO_CHANNEL(ioc));
+}
+
 if (s->parameters.tls_creds &&
 *s->parameters.tls_creds &&
 !object_dynamic_cast(OBJECT(ioc),
@@ -67,6 +74,11 @@ void migration_channel_connect(MigrationState *s,
 ioc, object_get_typename(OBJECT(ioc)), hostname, error);

 if (!error) {
+if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)) {
+yank_register_function((char *) "migration", 
yank_generic_iochannel,
+   QIO_CHANNEL(ioc));
+}
+
 if (s->parameters.tls_creds &&
 *s->parameters.tls_creds &&
 !object_dynamic_cast(OBJECT(ioc),
diff --git a/migration/migration.c b/migration/migration.c
index 187ac0410c..c6d7119c08 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -54,6 +54,7 @@
 #include "net/announce.h"
 #include "qemu/queue.h"
 #include "multifd.h"
+#include "yank.h"

 #define MAX_THROTTLE  (32 << 20)  /* Migration transfer speed throttling */

@@ -231,6 +232,8 @@ void migration_incoming_state_destroy(void)
 qapi_free_SocketAddressList(mis->socket_address_list);
 mis->socket_address_list = NULL;
 }
+
+yank_unregister_instance((char *) "migration");
 }

 static void migrate_generate_event(int new_state)
@@ -362,7 +365,9 @@ void qemu_start_incoming_migration(const char *uri, Error 
**errp)
 const char *p;

 qapi_event_send_migration(MIGRATION_STATUS_SETUP);
+yank_register_instance((char *) "migration");
 if (!strcmp(uri, "defer")) {
+yank_unregister_instance((char *) "migration");
 deferred_incoming_migration(errp);
 } else if (strstart(uri, "tcp:", &p)) {
 tcp_start_incoming_migration(p, errp);
@@ -377,6 +382,7 @@ void qemu_start_incoming_migration(const char *uri, Error 
**errp)
 } else if (strstart(uri, "fd:", &p)) {
 fd_start_incoming_migration(p, errp);
 } else {
+yank_unregister_instance((char *) "migration");
 error_setg(errp, "unknown migration protocol: %s", uri);
 }
 }
@@ -1632,6 +1638,7 @@ static void migrate_fd_cleanup(MigrationState *s)
 }
 notifier_list_notify(&migration_state_notifiers, s);
 block_cleanup_parameters(s);
+yank_unregister_instance((char *) "migration");
 }

 static void migrate_fd_cleanup_schedule(MigrationState *s)
@@ -1899,6 +1906,7 @@ void qmp_migrate_recover(const char *uri, Error **errp)
  * only re-setup the migration stream and poke existing migration
  * to continue using that newly established channel.
  */
+yank_unregister_instance((char *) "migration");
 qemu_start_incoming_migration(uri, errp);
 }

@@ -2035,7 +2043,9 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 /* Error detected, put into errp */
 return;
 }
-
+if (!(has_resume && resume)) {
+yank_register_instance((char *) "migration");
+}
 if (strstart(uri, "tcp:", &p)) {
 tcp_start_outgoing_migration(s, p, &local_err);
 #ifdef CONFIG_RDMA
@@ -2049,6 +2059,9 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 } else if (strstart(uri, "fd:", &p)) {
 fd_start_outgoing_migration(s, p, &local_err);
 } else {
+if (!(has_resume && resume)) {
+yank_unregister_instance((char *) "migration");
+}
 error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri",
"a valid migration protocol");
 migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
@@ -2058,6 +2071,9 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 }

 if (loca

[PATCH v3 3/4] chardev/char-socket.c: Add yank feature

[Lukas Straub]

Register a yank function to shutdown the socket on yank.

Signed-off-by: Lukas Straub 
---
 Makefile.objs |  1 +
 chardev/char-socket.c | 24 
 2 files changed, 25 insertions(+)

diff --git a/Makefile.objs b/Makefile.objs
index 8e403b81f3..5582f4eda9 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -10,6 +10,7 @@ qom-obj-y = qom/
 ifeq ($(call lor,$(CONFIG_SOFTMMU),$(CONFIG_TOOLS)),y)

 chardev-obj-y = chardev/
+chardev-obj-y += yank.o

 authz-obj-y = authz/

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 185fe38dda..d5c6cd2153 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -34,6 +34,7 @@
 #include "qapi/error.h"
 #include "qapi/clone-visitor.h"
 #include "qapi/qapi-visit-sockets.h"
+#include "yank.h"

 #include "chardev/char-io.h"

@@ -69,6 +70,7 @@ typedef struct {
 size_t read_msgfds_num;
 int *write_msgfds;
 size_t write_msgfds_num;
+char *yank_name;

 SocketAddress *addr;
 bool is_listen;
@@ -409,6 +411,11 @@ static void tcp_chr_free_connection(Chardev *chr)

 tcp_set_msgfds(chr, NULL, 0);
 remove_fd_in_watch(chr);
+if (s->state == TCP_CHARDEV_STATE_CONNECTING
+|| s->state == TCP_CHARDEV_STATE_CONNECTED) {
+yank_unregister_function(s->yank_name, yank_generic_iochannel,
+ QIO_CHANNEL(s->sioc));
+}
 object_unref(OBJECT(s->sioc));
 s->sioc = NULL;
 object_unref(OBJECT(s->ioc));
@@ -912,6 +919,8 @@ static int tcp_chr_add_client(Chardev *chr, int fd)
 }
 tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
 tcp_chr_set_client_ioc_name(chr, sioc);
+yank_register_function(s->yank_name, yank_generic_iochannel,
+   QIO_CHANNEL(sioc));
 ret = tcp_chr_new_client(chr, sioc);
 object_unref(OBJECT(sioc));
 return ret;
@@ -926,6 +935,8 @@ static void tcp_chr_accept(QIONetListener *listener,

 tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
 tcp_chr_set_client_ioc_name(chr, cioc);
+yank_register_function(s->yank_name, yank_generic_iochannel,
+   QIO_CHANNEL(cioc));
 tcp_chr_new_client(chr, cioc);
 }

@@ -941,6 +952,8 @@ static int tcp_chr_connect_client_sync(Chardev *chr, Error 
**errp)
 object_unref(OBJECT(sioc));
 return -1;
 }
+yank_register_function(s->yank_name, yank_generic_iochannel,
+   QIO_CHANNEL(sioc));
 tcp_chr_new_client(chr, sioc);
 object_unref(OBJECT(sioc));
 return 0;
@@ -956,6 +969,8 @@ static void tcp_chr_accept_server_sync(Chardev *chr)
 tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
 sioc = qio_net_listener_wait_client(s->listener);
 tcp_chr_set_client_ioc_name(chr, sioc);
+yank_register_function(s->yank_name, yank_generic_iochannel,
+   QIO_CHANNEL(sioc));
 tcp_chr_new_client(chr, sioc);
 object_unref(OBJECT(sioc));
 }
@@ -1066,6 +1081,8 @@ static void char_socket_finalize(Object *obj)
 object_unref(OBJECT(s->tls_creds));
 }
 g_free(s->tls_authz);
+yank_unregister_instance(s->yank_name);
+g_free(s->yank_name);

 qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
@@ -1081,6 +1098,8 @@ static void qemu_chr_socket_connected(QIOTask *task, void 
*opaque)

 if (qio_task_propagate_error(task, &err)) {
 tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
+yank_unregister_function(s->yank_name, yank_generic_iochannel,
+ QIO_CHANNEL(sioc));
 check_report_connect_error(chr, err);
 error_free(err);
 goto cleanup;
@@ -1115,6 +1134,8 @@ static void tcp_chr_connect_client_async(Chardev *chr)
 tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
 sioc = qio_channel_socket_new();
 tcp_chr_set_client_ioc_name(chr, sioc);
+yank_register_function(s->yank_name, yank_generic_iochannel,
+   QIO_CHANNEL(sioc));
 /*
  * Normally code would use the qio_channel_socket_connect_async
  * method which uses a QIOTask + qio_task_set_error internally
@@ -1356,6 +1377,9 @@ static void qmp_chardev_open_socket(Chardev *chr,
 qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
 }

+s->yank_name = g_strconcat("chardev:", chr->label, NULL);
+yank_register_instance(s->yank_name);
+
 /* be isn't opened until we get a connection */
 *be_opened = false;

--
2.20.1



pgpj9Q39AJ5fK.pgp
Description: OpenPGP digital signature

[PATCH v3 1/4] Introduce yank feature

[Lukas Straub]

The yank feature allows to recover from hanging qemu by "yanking"
at various parts. Other qemu systems can register themselves and
multiple yank functions. Then all yank functions for selected
instances can be called by the 'yank' out-of-band qmp command.
Available instances can be queried by a 'query-yank' oob command.

Signed-off-by: Lukas Straub 
---
 qapi/misc.json |  45 +
 yank.c | 174 +
 yank.h |  67 +++
 3 files changed, 286 insertions(+)
 create mode 100644 yank.c
 create mode 100644 yank.h

diff --git a/qapi/misc.json b/qapi/misc.json
index 99b90ac80b..f5228b2502 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -1550,3 +1550,48 @@
 ##
 { 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }

+##
+# @YankInstances:
+#
+# @instances: List of yank instances.
+#
+# Yank instances are named after the following schema:
+# "blockdev:", "chardev:" and "migration"
+#
+# Since: 5.1
+##
+{ 'struct': 'YankInstances', 'data': {'instances': ['str'] } }
+
+##
+# @yank:
+#
+# Recover from hanging qemu by yanking the specified instances.
+#
+# Takes @YankInstances as argument.
+#
+# Returns: nothing.
+#
+# Example:
+#
+# -> { "execute": "yank", "arguments": { "instances": ["blockdev:nbd0"] } }
+# <- { "return": {} }
+#
+# Since: 5.1
+##
+{ 'command': 'yank', 'data': 'YankInstances', 'allow-oob': true }
+
+##
+# @query-yank:
+#
+# Query yank instances.
+#
+# Returns: @YankInstances
+#
+# Example:
+#
+# -> { "execute": "query-yank" }
+# <- { "return": { "instances": ["blockdev:nbd0"] } }
+#
+# Since: 5.1
+##
+{ 'command': 'query-yank', 'returns': 'YankInstances', 'allow-oob': true }
diff --git a/yank.c b/yank.c
new file mode 100644
index 00..36d8139d4d
--- /dev/null
+++ b/yank.c
@@ -0,0 +1,174 @@
+/*
+ * QEMU yank feature
+ *
+ * Copyright (c) Lukas Straub 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/thread.h"
+#include "qemu/queue.h"
+#include "qapi/qapi-commands-misc.h"
+#include "io/channel.h"
+#include "yank.h"
+
+struct YankFuncAndParam {
+YankFn *func;
+void *opaque;
+QLIST_ENTRY(YankFuncAndParam) next;
+};
+
+struct YankInstance {
+char *name;
+QLIST_HEAD(, YankFuncAndParam) yankfns;
+QLIST_ENTRY(YankInstance) next;
+};
+
+static QemuMutex lock;
+static QLIST_HEAD(yankinst_list, YankInstance) head
+= QLIST_HEAD_INITIALIZER(head);
+
+static struct YankInstance *yank_find_instance(char *name)
+{
+struct YankInstance *tmp, *instance;
+instance = NULL;
+QLIST_FOREACH(tmp, &head, next) {
+if (!strcmp(tmp->name, name)) {
+instance = tmp;
+}
+}
+return instance;
+}
+
+void yank_register_instance(char *instance_name)
+{
+struct YankInstance *instance;
+
+qemu_mutex_lock(&lock);
+assert(!yank_find_instance(instance_name));
+
+instance = g_slice_new(struct YankInstance);
+instance->name = g_strdup(instance_name);
+QLIST_INIT(&instance->yankfns);
+QLIST_INSERT_HEAD(&head, instance, next);
+
+qemu_mutex_unlock(&lock);
+}
+
+void yank_unregister_instance(char *instance_name)
+{
+struct YankInstance *instance;
+
+qemu_mutex_lock(&lock);
+instance = yank_find_instance(instance_name);
+assert(instance);
+
+assert(QLIST_EMPTY(&instance->yankfns));
+QLIST_REMOVE(instance, next);
+g_free(instance->name);
+g_slice_free(struct YankInstance, instance);
+
+qemu_mutex_unlock(&lock);
+}
+
+void yank_register_function(char *instance_name, YankFn *func, void *opaque)
+{
+struct YankInstance *instance;
+struct YankFuncAndParam *entry;
+
+qemu_mutex_lock(&lock);
+instance = yank_find_instance(instance_name);
+assert(instance);
+
+entry = g_slice_new(struct YankFuncAndParam);
+entry->func = func;
+entry->opaque = opaque;
+
+QLIST_INSERT_HEAD(&instance->yankfns, entry, next);
+qemu_mutex_unlock(&lock);
+}
+
+void yank_unregister_function(char *instance_name, YankFn *func, void *opaque)
+{
+struct YankInstance *instance;
+struct YankFuncAndParam *entry;
+
+qemu_mutex_lock(&lock);
+instance = yank_find_instance(instance_name);
+assert(instance);
+
+QLIST_FOREACH(entry, &instance->yankfns, next) {
+if (entry->func == func && entry->opaque == opaque) {
+QLIST_REMOVE(entry, next);
+g_slice_free(struct YankFuncAndParam, entry);
+qemu_mutex_unlock(&lock);
+return;
+}
+}
+
+abort();
+}
+
+void yank_generic_iochannel(void *opaque)
+{
+QIOChannel *ioc = QIO_CHANNEL(opaque);
+
+qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+}
+
+void qmp_yank(strList *instances, Error **errp)
+{
+strList *tmp;
+struct YankInstance *instance;
+struct YankFuncAndParam *entry;
+
+qemu_mute

[PATCH v3 2/4] block/nbd.c: Add yank feature

[Lukas Straub]

Register a yank function which shuts down the socket and sets
s->state = NBD_CLIENT_QUIT. This is the same behaviour as if an
error occured.

Signed-off-by: Lukas Straub 
---
 Makefile.objs |   1 +
 block/nbd.c   | 101 --
 2 files changed, 65 insertions(+), 37 deletions(-)

diff --git a/Makefile.objs b/Makefile.objs
index a7c967633a..8e403b81f3 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -18,6 +18,7 @@ block-obj-y += block.o blockjob.o job.o
 block-obj-y += block/ scsi/
 block-obj-y += qemu-io-cmds.o
 block-obj-$(CONFIG_REPLICATION) += replication.o
+block-obj-y += yank.o

 block-obj-m = block/

diff --git a/block/nbd.c b/block/nbd.c
index 2160859f64..3a41749f1b 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -35,6 +35,7 @@
 #include "qemu/option.h"
 #include "qemu/cutils.h"
 #include "qemu/main-loop.h"
+#include "qemu/atomic.h"

 #include "qapi/qapi-visit-sockets.h"
 #include "qapi/qmp/qstring.h"
@@ -43,6 +44,8 @@
 #include "block/nbd.h"
 #include "block/block_int.h"

+#include "yank.h"
+
 #define EN_OPTSTR ":exportname="
 #define MAX_NBD_REQUESTS16

@@ -84,6 +87,8 @@ typedef struct BDRVNBDState {
 NBDReply reply;
 BlockDriverState *bs;

+char *yank_name;
+
 /* Connection parameters */
 uint32_t reconnect_delay;
 SocketAddress *saddr;
@@ -94,6 +99,7 @@ typedef struct BDRVNBDState {
 } BDRVNBDState;

 static int nbd_client_connect(BlockDriverState *bs, Error **errp);
+static void nbd_yank(void *opaque);

 static void nbd_clear_bdrvstate(BDRVNBDState *s)
 {
@@ -106,17 +112,19 @@ static void nbd_clear_bdrvstate(BDRVNBDState *s)
 s->tlscredsid = NULL;
 g_free(s->x_dirty_bitmap);
 s->x_dirty_bitmap = NULL;
+g_free(s->yank_name);
+s->yank_name = NULL;
 }

 static void nbd_channel_error(BDRVNBDState *s, int ret)
 {
 if (ret == -EIO) {
-if (s->state == NBD_CLIENT_CONNECTED) {
+if (atomic_read(&s->state) == NBD_CLIENT_CONNECTED) {
 s->state = s->reconnect_delay ? NBD_CLIENT_CONNECTING_WAIT :
 NBD_CLIENT_CONNECTING_NOWAIT;
 }
 } else {
-if (s->state == NBD_CLIENT_CONNECTED) {
+if (atomic_read(&s->state) == NBD_CLIENT_CONNECTED) {
 qio_channel_shutdown(s->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
 }
 s->state = NBD_CLIENT_QUIT;
@@ -167,7 +175,7 @@ static void nbd_client_attach_aio_context(BlockDriverState 
*bs,
  * s->connection_co is either yielded from nbd_receive_reply or from
  * nbd_co_reconnect_loop()
  */
-if (s->state == NBD_CLIENT_CONNECTED) {
+if (atomic_read(&s->state) == NBD_CLIENT_CONNECTED) {
 qio_channel_attach_aio_context(QIO_CHANNEL(s->ioc), new_context);
 }

@@ -206,7 +214,7 @@ static void nbd_teardown_connection(BlockDriverState *bs)
 {
 BDRVNBDState *s = (BDRVNBDState *)bs->opaque;

-if (s->state == NBD_CLIENT_CONNECTED) {
+if (atomic_read(&s->state) == NBD_CLIENT_CONNECTED) {
 /* finish any pending coroutines */
 assert(s->ioc);
 qio_channel_shutdown(s->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
@@ -230,13 +238,14 @@ static void nbd_teardown_connection(BlockDriverState *bs)

 static bool nbd_client_connecting(BDRVNBDState *s)
 {
-return s->state == NBD_CLIENT_CONNECTING_WAIT ||
-s->state == NBD_CLIENT_CONNECTING_NOWAIT;
+NBDClientState state = atomic_read(&s->state);
+return state == NBD_CLIENT_CONNECTING_WAIT ||
+state == NBD_CLIENT_CONNECTING_NOWAIT;
 }

 static bool nbd_client_connecting_wait(BDRVNBDState *s)
 {
-return s->state == NBD_CLIENT_CONNECTING_WAIT;
+return atomic_read(&s->state) == NBD_CLIENT_CONNECTING_WAIT;
 }

 static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
@@ -274,6 +283,7 @@ static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState 
*s)
 /* Finalize previous connection if any */
 if (s->ioc) {
 nbd_client_detach_aio_context(s->bs);
+yank_unregister_function(s->yank_name, nbd_yank, s->bs);
 object_unref(OBJECT(s->sioc));
 s->sioc = NULL;
 object_unref(OBJECT(s->ioc));
@@ -305,7 +315,7 @@ static coroutine_fn void nbd_co_reconnect_loop(BDRVNBDState 
*s)
 nbd_reconnect_attempt(s);

 while (nbd_client_connecting(s)) {
-if (s->state == NBD_CLIENT_CONNECTING_WAIT &&
+if (atomic_read(&s->state) == NBD_CLIENT_CONNECTING_WAIT &&
 qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start_time_ns > delay_ns)
 {
 s->state = NBD_CLIENT_CONNECTING_NOWAIT;
@@ -341,7 +351,7 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
 int ret = 0;
 Error *local_err = NULL;

-while (s->state != NBD_CLIENT_QUIT) {
+while (atomic_read(&s->state) != NBD_CLIENT_QUIT) {
 /*
  * The NBD client can only really be considered idle when it has
  * yielded from qio_channel_readv_all_eof(), waiting for data. This is
@@ -356,7 +366,7

[PATCH v3 0/4] Introduce 'yank' oob qmp command to recover from hanging qemu

[Lukas Straub]

Hello Everyone,
In many cases, if qemu has a network connection (qmp, migration, chardev, etc.)
to some other server and that server dies or hangs, qemu hangs too.
These patches introduce the new 'yank' out-of-band qmp command to recover from
these kinds of hangs. The different subsystems register callbacks which get
executed with the yank command. For example the callback can shutdown() a
socket. This is intended for the colo use-case, but it can be used for other
things too of course.

Regards,
Lukas Straub

v3:
 -don't touch softmmu/vl.c, use __contructor__ attribute instead (Paolo Bonzini)
 -fix build errors
 -rewrite migration patch so it actually passes all tests

v2:
 -don't touch io/ code anymore
 -always register yank functions
 -'yank' now takes a list of instances to yank
 -'query-yank' returns a list of yankable instances

Lukas Straub (4):
  Introduce yank feature
  block/nbd.c: Add yank feature
  chardev/char-socket.c: Add yank feature
  migration: Add yank feature

 Makefile.objs |   3 +
 block/nbd.c   | 101 
 chardev/char-socket.c |  24 +
 migration/channel.c   |  12 +++
 migration/migration.c |  18 +++-
 migration/multifd.c   |  10 ++
 migration/qemu-file-channel.c |   6 ++
 qapi/misc.json|  45 +
 yank.c| 174 ++
 yank.h|  67 +
 10 files changed, 422 insertions(+), 38 deletions(-)
 create mode 100644 yank.c
 create mode 100644 yank.h

--
2.20.1


pgpD8oKCBem3g.pgp
Description: OpenPGP digital signature

Re: [PATCH RFC v3 01/11] linux-headers: Update

[Peter Maydell]

On Sun, 24 May 2020 at 15:07, Peter Xu  wrote:
>
> Hi, Peter,
>
> On Sun, May 24, 2020 at 02:27:14PM +0100, Peter Maydell wrote:
> > On Sun, 24 May 2020 at 00:21, Peter Xu  wrote:
> > >
> > > Signed-off-by: Peter Xu 
> >
> > Header updates should always include the upstream
> > kernel commit against which you ran the scripts/update-linux-headers.sh
> > script, please.
>
> This is based on a kernel series that hasn't yet been merged, so I didn't tag
> it (so this is still a RFC series).  Will do when it's merged.

Ah, cool. (It's helpful to note in the commit message for the
header-update patch if it's a not-yet-upstream set of changes.)

thanks
-- PMM

[Bug 1880424] [NEW] I/O write make imx_epit_reset() crash

[Philippe Mathieu-Daudé]

Public bug reported:

libFuzzer found:

qemu-fuzz-arm: hw/core/ptimer.c:377: void ptimer_transaction_begin(ptimer_state 
*): Assertion `!s->in_transaction' failed.
==6041== ERROR: libFuzzer: deadly signal
#8 0x7fcaba320565 in __GI___assert_fail (/lib64/libc.so.6+0x30565)
#9 0x563b46f91637 in ptimer_transaction_begin (qemu-fuzz-arm+0x1af1637)
#10 0x563b476cc4c6 in imx_epit_reset (qemu-fuzz-arm+0x222c4c6)
#11 0x563b476cd004 in imx_epit_write (qemu-fuzz-arm+0x222d004)
#12 0x563b46582377 in memory_region_write_accessor (qemu-fuzz-arm+0x10e2377)
#13 0x563b46581ee3 in access_with_adjusted_size (qemu-fuzz-arm+0x10e1ee3)
#14 0x563b46580a83 in memory_region_dispatch_write (qemu-fuzz-arm+0x10e0a83)
#15 0x563b463c5022 in flatview_write_continue (qemu-fuzz-arm+0xf25022)
#16 0x563b463b4ea2 in flatview_write (qemu-fuzz-arm+0xf14ea2)
#17 0x563b463b49d4 in address_space_write (qemu-fuzz-arm+0xf149d4)

Reproducer:

qemu-system-arm -M kzm -display none -S -qtest stdio << 'EOF'
writel 0x53f94000 0x11
EOF

qemu-system-arm: hw/core/ptimer.c:377: ptimer_transaction_begin: Assertion 
`!s->in_transaction' failed.
Aborted (core dumped)

(gdb) bt
#1  0x7f4aa4daa895 in abort () at /lib64/libc.so.6
#2  0x7f4aa4daa769 in _nl_load_domain.cold () at /lib64/libc.so.6
#3  0x7f4aa4db8566 in annobin_assert.c_end () at /lib64/libc.so.6
#4  0x55ee85400164 in ptimer_transaction_begin (s=0x55ee873bc4c0) at 
hw/core/ptimer.c:377
#5  0x55ee855c7936 in imx_epit_reset (dev=0x55ee871725c0) at 
hw/timer/imx_epit.c:111
#6  0x55ee855c7d1b in imx_epit_write (opaque=0x55ee871725c0, offset=0, 
value=1114112, size=4) at hw/timer/imx_epit.c:209
#7  0x55ee8513db85 in memory_region_write_accessor (mr=0x55ee871728f0, 
addr=0, value=0x7fff3012d6f8, size=4, shift=0, mask=4294967295, attrs=...) at 
memory.c:483
#8  0x55ee8513dd96 in access_with_adjusted_size (addr=0, 
value=0x7fff3012d6f8, size=4, access_size_min=1, access_size_max=4, access_fn=
0x55ee8513daa2 , mr=0x55ee871728f0, 
attrs=...) at memory.c:545
#9  0x55ee85140cbd in memory_region_dispatch_write (mr=0x55ee871728f0, 
addr=0, data=1114112, op=MO_32, attrs=...) at memory.c:1477
#10 0x55ee850deba5 in flatview_write_continue (fv=0x55ee87181bd0, 
addr=1408843776, attrs=..., ptr=0x7fff3012d900, len=4, addr1=0, l=4, 
mr=0x55ee871728f0) at exec.c:3147
#11 0x55ee850decf3 in flatview_write (fv=0x55ee87181bd0, addr=1408843776, 
attrs=..., buf=0x7fff3012d900, len=4) at exec.c:3190
#12 0x55ee850df05d in address_space_write (as=0x55ee8730a560, 
addr=1408843776, attrs=..., buf=0x7fff3012d900, len=4) at exec.c:3289

** Affects: qemu
 Importance: Undecided
 Status: New


** Tags: arm

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1880424

Title:
  I/O write make imx_epit_reset() crash

Status in QEMU:
  New

Bug description:
  libFuzzer found:

  qemu-fuzz-arm: hw/core/ptimer.c:377: void 
ptimer_transaction_begin(ptimer_state *): Assertion `!s->in_transaction' failed.
  ==6041== ERROR: libFuzzer: deadly signal
  #8 0x7fcaba320565 in __GI___assert_fail (/lib64/libc.so.6+0x30565)
  #9 0x563b46f91637 in ptimer_transaction_begin (qemu-fuzz-arm+0x1af1637)
  #10 0x563b476cc4c6 in imx_epit_reset (qemu-fuzz-arm+0x222c4c6)
  #11 0x563b476cd004 in imx_epit_write (qemu-fuzz-arm+0x222d004)
  #12 0x563b46582377 in memory_region_write_accessor 
(qemu-fuzz-arm+0x10e2377)
  #13 0x563b46581ee3 in access_with_adjusted_size (qemu-fuzz-arm+0x10e1ee3)
  #14 0x563b46580a83 in memory_region_dispatch_write 
(qemu-fuzz-arm+0x10e0a83)
  #15 0x563b463c5022 in flatview_write_continue (qemu-fuzz-arm+0xf25022)
  #16 0x563b463b4ea2 in flatview_write (qemu-fuzz-arm+0xf14ea2)
  #17 0x563b463b49d4 in address_space_write (qemu-fuzz-arm+0xf149d4)

  Reproducer:

  qemu-system-arm -M kzm -display none -S -qtest stdio << 'EOF'
  writel 0x53f94000 0x11
  EOF

  qemu-system-arm: hw/core/ptimer.c:377: ptimer_transaction_begin: Assertion 
`!s->in_transaction' failed.
  Aborted (core dumped)

  (gdb) bt
  #1  0x7f4aa4daa895 in abort () at /lib64/libc.so.6
  #2  0x7f4aa4daa769 in _nl_load_domain.cold () at /lib64/libc.so.6
  #3  0x7f4aa4db8566 in annobin_assert.c_end () at /lib64/libc.so.6
  #4  0x55ee85400164 in ptimer_transaction_begin (s=0x55ee873bc4c0) at 
hw/core/ptimer.c:377
  #5  0x55ee855c7936 in imx_epit_reset (dev=0x55ee871725c0) at 
hw/timer/imx_epit.c:111
  #6  0x55ee855c7d1b in imx_epit_write (opaque=0x55ee871725c0, offset=0, 
value=1114112, size=4) at hw/timer/imx_epit.c:209
  #7  0x55ee8513db85 in memory_region_write_accessor (mr=0x55ee871728f0, 
addr=0, value=0x7fff3012d6f8, size=4, shift=0, mask=4294967295, attrs=...) at 
memory.c:483
  #8  0x55ee8513dd96 in access_with_adjusted_size (addr=0, 
value=0x7fff3012d6f8, size=4, access_size_min=1, access_size_max=4, access_

[PATCH] hw/display/cg3: Convert debug printf()s to trace events

[Philippe Mathieu-Daudé]

Convert DPRINTF() to trace events and remove ifdef'ry.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/display/cg3.c| 14 --
 hw/display/trace-events |  4 
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index f7f1c199ce..f6100c0e3d 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -35,6 +35,7 @@
 #include "hw/qdev-properties.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "trace.h"
 
 /* Change to 1 to enable debugging */
 #define DEBUG_CG3 0
@@ -63,12 +64,6 @@
 #define CG3_VRAM_SIZE 0x10
 #define CG3_VRAM_OFFSET 0x80
 
-#define DPRINTF(fmt, ...) do { \
-if (DEBUG_CG3) { \
-printf("CG3: " fmt , ## __VA_ARGS__); \
-} \
-} while (0)
-
 #define TYPE_CG3 "cgthree"
 #define CG3(obj) OBJECT_CHECK(CG3State, (obj), TYPE_CG3)
 
@@ -195,7 +190,8 @@ static uint64_t cg3_reg_read(void *opaque, hwaddr addr, 
unsigned size)
 val = 0;
 break;
 }
-DPRINTF("read %02x from reg %" HWADDR_PRIx "\n", val, addr);
+trace_cg3_read(size, addr, val);
+
 return val;
 }
 
@@ -206,9 +202,7 @@ static void cg3_reg_write(void *opaque, hwaddr addr, 
uint64_t val,
 uint8_t regval;
 int i;
 
-DPRINTF("write %" PRIx64 " to reg %" HWADDR_PRIx " size %d\n",
-val, addr, size);
-
+trace_cg3_write(size, addr, val);
 switch (addr) {
 case CG3_REG_BT458_ADDR:
 s->dac_index = val;
diff --git a/hw/display/trace-events b/hw/display/trace-events
index e6e22bef88..1ebcbeea46 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -151,3 +151,7 @@ artist_vram_write(unsigned int size, uint64_t addr, 
uint64_t val) "%u 0x%"PRIx64
 artist_fill_window(unsigned int start_x, unsigned int start_y, unsigned int 
width, unsigned int height, uint32_t op, uint32_t ctlpln) "start=%ux%u 
length=%ux%u op=0x%08x ctlpln=0x%08x"
 artist_block_move(unsigned int start_x, unsigned int start_y, unsigned int 
dest_x, unsigned int dest_y, unsigned int width, unsigned int height) "source 
%ux%u -> dest %ux%u size %ux%u"
 artist_draw_line(unsigned int start_x, unsigned int start_y, unsigned int 
end_x, unsigned int end_y) "%ux%u %ux%u"
+
+# cg3.c
+cg3_read(unsigned size, uint32_t addr, uint32_t val) "read size:%u 
addr:0x%06"PRIx32" val:0x%08"PRIx32
+cg3_write(unsigned size, uint32_t addr, uint32_t val) "read write:%u 
addr:0x%06"PRIx32" val:0x%08"PRIx32
-- 
2.21.3

Re: [PATCH 0/7] hw/misc/empty_slot: Spring cleaning

[Philippe Mathieu-Daudé]

ping?

On 5/10/20 5:28 PM, Philippe Mathieu-Daudé wrote:
> New Spring, new opportunity to clean this device :)
> (v1 was in 2018, v2 in 2019).
> 
> - lower device priority
> - follow qdev model and use properties
> - convert to trace events
> - describe with slot name
> - move under hw/misc/ and cover in MAINTAINERS
> 
> Peter, I hope you are OK adding it wit UNIMP device,
> as both are very similar, and don't have much activity.
> 
> Only MIPS/SPARC32 targets use this device.
> 
> v2: https://www.mail-archive.com/qemu-devel@nongnu.org/msg626498.html
> v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg564060.html
> 
> Philippe Mathieu-Daudé (7):
>   hw/sparc/sun4m: Use UnimplementedDevice for I/O devices
>   hw/misc/empty_slot: Lower address space priority
>   hw/misc/empty_slot: Convert 'size' field as qdev property
>   hw/misc/empty_slot: Add a 'name' qdev property
>   hw/misc/empty_slot: Convert debug printf() to trace event
>   hw/misc/empty_slot: Move the 'hw/misc' and cover in MAINTAINERS
>   hw/misc/empty_slot: Name the slots when created
> 
>  include/hw/empty_slot.h|  9 ---
>  include/hw/misc/empty_slot.h   | 19 ++
>  hw/mips/mips_malta.c   |  4 +--
>  hw/{core => misc}/empty_slot.c | 47 +++---
>  hw/sparc/sun4m.c   | 23 +++--
>  MAINTAINERS|  4 ++-
>  hw/core/Makefile.objs  |  1 -
>  hw/misc/Makefile.objs  |  1 +
>  hw/misc/trace-events   |  4 +++
>  9 files changed, 70 insertions(+), 42 deletions(-)
>  delete mode 100644 include/hw/empty_slot.h
>  create mode 100644 include/hw/misc/empty_slot.h
>  rename hw/{core => misc}/empty_slot.c (66%)
> 

[PATCH] hw/nvram/mac_nvram: Convert debug printf()s to trace events

[Philippe Mathieu-Daudé]

Convert NVR_DPRINTF() to trace events and remove ifdef'ry.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/nvram/mac_nvram.c  | 17 +++--
 hw/nvram/trace-events |  4 
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/hw/nvram/mac_nvram.c b/hw/nvram/mac_nvram.c
index ff5db03e6b..beec1c4e4d 100644
--- a/hw/nvram/mac_nvram.c
+++ b/hw/nvram/mac_nvram.c
@@ -30,18 +30,9 @@
 #include "migration/vmstate.h"
 #include "qemu/cutils.h"
 #include "qemu/module.h"
+#include "trace.h"
 #include 
 
-/* debug NVR */
-//#define DEBUG_NVR
-
-#ifdef DEBUG_NVR
-#define NVR_DPRINTF(fmt, ...)   \
-do { printf("NVR: " fmt , ## __VA_ARGS__); } while (0)
-#else
-#define NVR_DPRINTF(fmt, ...)
-#endif
-
 #define DEF_SYSTEM_SIZE 0xc10
 
 /* macio style NVRAM device */
@@ -51,9 +42,8 @@ static void macio_nvram_writeb(void *opaque, hwaddr addr,
 MacIONVRAMState *s = opaque;
 
 addr = (addr >> s->it_shift) & (s->size - 1);
+trace_macio_nvram_write(addr, value);
 s->data[addr] = value;
-NVR_DPRINTF("writeb addr %04" HWADDR_PRIx " val %" PRIx64 "\n",
-addr, value);
 }
 
 static uint64_t macio_nvram_readb(void *opaque, hwaddr addr,
@@ -64,8 +54,7 @@ static uint64_t macio_nvram_readb(void *opaque, hwaddr addr,
 
 addr = (addr >> s->it_shift) & (s->size - 1);
 value = s->data[addr];
-NVR_DPRINTF("readb addr %04" HWADDR_PRIx " val %" PRIx32 "\n",
-addr, value);
+trace_macio_nvram_read(addr, value);
 
 return value;
 }
diff --git a/hw/nvram/trace-events b/hw/nvram/trace-events
index 0dea9260ce..e023193295 100644
--- a/hw/nvram/trace-events
+++ b/hw/nvram/trace-events
@@ -13,3 +13,7 @@ fw_cfg_add_string(uint16_t key_value, const char *key_name, 
const char *value) "
 fw_cfg_add_i16(uint16_t key_value, const char *key_name, uint16_t value) "key 
0x%04" PRIx16 " '%s', value 0x%" PRIx16
 fw_cfg_add_i32(uint16_t key_value, const char *key_name, uint32_t value) "key 
0x%04" PRIx16 " '%s', value 0x%" PRIx32
 fw_cfg_add_i64(uint16_t key_value, const char *key_name, uint64_t value) "key 
0x%04" PRIx16 " '%s', value 0x%" PRIx64
+
+# mac_nvram.c
+macio_nvram_read(uint32_t addr, uint8_t val) "read addr=0x%04"PRIx32" 
val=0x%02x"
+macio_nvram_write(uint32_t addr, uint8_t val) "write addr=0x%04"PRIx32" 
val=0x%02x"
-- 
2.21.3

[PATCH] hw/isa/apm: Convert debug printf()s to trace events

[Philippe Mathieu-Daudé]

Convert APM_DPRINTF() to trace events and remove ifdef'ry.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/isa/apm.c| 15 +--
 hw/isa/trace-events |  4 
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/hw/isa/apm.c b/hw/isa/apm.c
index 6300b1ba7a..bce266b957 100644
--- a/hw/isa/apm.c
+++ b/hw/isa/apm.c
@@ -24,14 +24,8 @@
 #include "hw/isa/apm.h"
 #include "hw/pci/pci.h"
 #include "migration/vmstate.h"
+#include "trace.h"
 
-//#define DEBUG
-
-#ifdef DEBUG
-# define APM_DPRINTF(format, ...)   printf(format, ## __VA_ARGS__)
-#else
-# define APM_DPRINTF(format, ...)   do { } while (0)
-#endif
 
 /* fixed I/O location */
 #define APM_STS_IOPORT  0xb3
@@ -41,8 +35,8 @@ static void apm_ioport_writeb(void *opaque, hwaddr addr, 
uint64_t val,
 {
 APMState *apm = opaque;
 addr &= 1;
-APM_DPRINTF("apm_ioport_writeb addr=0x%" HWADDR_PRIx
-" val=0x%02" PRIx64 "\n", addr, val);
+
+trace_apm_io_write(addr, val);
 if (addr == 0) {
 apm->apmc = val;
 
@@ -65,7 +59,8 @@ static uint64_t apm_ioport_readb(void *opaque, hwaddr addr, 
unsigned size)
 } else {
 val = apm->apms;
 }
-APM_DPRINTF("apm_ioport_readb addr=0x%" HWADDR_PRIx " val=0x%02x\n", addr, 
val);
+trace_apm_io_read(addr, val);
+
 return val;
 }
 
diff --git a/hw/isa/trace-events b/hw/isa/trace-events
index 202f8938e7..3544c6213c 100644
--- a/hw/isa/trace-events
+++ b/hw/isa/trace-events
@@ -9,3 +9,7 @@ superio_create_ide(int id, uint16_t base, unsigned int irq) 
"id=%d, base 0x%03x,
 # pc87312.c
 pc87312_io_read(uint32_t addr, uint32_t val) "read addr=0x%x val=0x%x"
 pc87312_io_write(uint32_t addr, uint32_t val) "write addr=0x%x val=0x%x"
+
+# apm.c
+apm_io_read(uint8_t addr, uint8_t val) "read addr=0x%x val=0x%02x"
+apm_io_write(uint8_t addr, uint8_t val) "write addr=0x%x val=0x%02x"
-- 
2.21.3

Re: [PATCH v6 00/32] Add subcluster allocation to qcow2

[no-reply]

Patchew URL: https://patchew.org/QEMU/cover.1590331741.git.be...@igalia.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC  block/parallels.o
  CC  block/blklogwrites.o
  CC  block/block-backend.o
/tmp/qemu-test/src/block/qcow2-cluster.c:1912:54: error: implicit conversion 
from enumeration type 'QCow2ClusterType' (aka 'enum QCow2ClusterType') to 
different enumeration type 'enum qcow2_discard_type' [-Werror,-Wenum-conversion]
qcow2_free_any_clusters(bs, old_l2_entry, 1, type);
~~~  ^~~~
1 error generated.
make: *** [/tmp/qemu-test/src/rules.mak:69: block/qcow2-cluster.o] Error 1
make: *** Waiting for unfinished jobs
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in 
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=7888e00ead4743f2936bfa5f2bc1cd85', '-u', 
'1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 
'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 
'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', 
'-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-a3y14gp6/src/docker-src.2020-05-24-12.43.26.11536:/var/tmp/qemu:z,ro',
 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=7888e00ead4743f2936bfa5f2bc1cd85
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-a3y14gp6/src'
make: *** [docker-run-test-debug@fedora] Error 2

real3m28.097s
user0m7.204s


The full log is available at
http://patchew.org/logs/cover.1590331741.git.be...@igalia.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

[PATCH] hw/unicore32/puv3: Use qemu_log_mask(ERROR) instead of debug printf()

[Philippe Mathieu-Daudé]

Replace some debug printf() calls by qemu_log_mask(LOG_GUEST_ERROR).

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/dma/puv3_dma.c   |  9 +++--
 hw/gpio/puv3_gpio.c | 15 +++
 hw/intc/puv3_intc.c |  9 +++--
 hw/misc/puv3_pm.c   |  9 +++--
 hw/timer/puv3_ost.c |  9 +++--
 hw/unicore32/puv3.c |  2 --
 6 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/hw/dma/puv3_dma.c b/hw/dma/puv3_dma.c
index 5488d388a9..7fa979180f 100644
--- a/hw/dma/puv3_dma.c
+++ b/hw/dma/puv3_dma.c
@@ -15,6 +15,7 @@
 #undef DEBUG_PUV3
 #include "hw/unicore32/puv3.h"
 #include "qemu/module.h"
+#include "qemu/log.h"
 
 #define PUV3_DMA_CH_NR  (6)
 #define PUV3_DMA_CH_MASK(0xff)
@@ -43,7 +44,9 @@ static uint64_t puv3_dma_read(void *opaque, hwaddr offset,
 ret = s->reg_CFG[PUV3_DMA_CH(offset)];
 break;
 default:
-DPRINTF("Bad offset 0x%x\n", offset);
+qemu_log_mask(LOG_GUEST_ERROR,
+  "%s: Bad read offset 0x%"HWADDR_PRIx"\n",
+  __func__, offset);
 }
 DPRINTF("offset 0x%x, value 0x%x\n", offset, ret);
 
@@ -62,7 +65,9 @@ static void puv3_dma_write(void *opaque, hwaddr offset,
 s->reg_CFG[PUV3_DMA_CH(offset)] = value;
 break;
 default:
-DPRINTF("Bad offset 0x%x\n", offset);
+qemu_log_mask(LOG_GUEST_ERROR,
+  "%s: Bad write offset 0x%"HWADDR_PRIx"\n",
+  __func__, offset);
 }
 DPRINTF("offset 0x%x, value 0x%x\n", offset, value);
 }
diff --git a/hw/gpio/puv3_gpio.c b/hw/gpio/puv3_gpio.c
index d19e342514..7362b6715f 100644
--- a/hw/gpio/puv3_gpio.c
+++ b/hw/gpio/puv3_gpio.c
@@ -15,6 +15,7 @@
 #undef DEBUG_PUV3
 #include "hw/unicore32/puv3.h"
 #include "qemu/module.h"
+#include "qemu/log.h"
 
 #define TYPE_PUV3_GPIO "puv3_gpio"
 #define PUV3_GPIO(obj) OBJECT_CHECK(PUV3GPIOState, (obj), TYPE_PUV3_GPIO)
@@ -47,7 +48,9 @@ static uint64_t puv3_gpio_read(void *opaque, hwaddr offset,
 ret = s->reg_GPIR;
 break;
 default:
-DPRINTF("Bad offset 0x%x\n", offset);
+qemu_log_mask(LOG_GUEST_ERROR,
+  "%s: Bad read offset 0x%"HWADDR_PRIx"\n",
+  __func__, offset);
 }
 DPRINTF("offset 0x%x, value 0x%x\n", offset, ret);
 
@@ -68,14 +71,16 @@ static void puv3_gpio_write(void *opaque, hwaddr offset,
 if (s->reg_GPDR & value) {
 s->reg_GPLR |= value;
 } else {
-DPRINTF("Write gpio input port error!");
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Write gpio input port\n",
+  __func__);
 }
 break;
 case 0x0c:
 if (s->reg_GPDR & value) {
 s->reg_GPLR &= ~value;
 } else {
-DPRINTF("Write gpio input port error!");
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Write gpio input port\n",
+  __func__);
 }
 break;
 case 0x10: /* GRER */
@@ -86,7 +91,9 @@ static void puv3_gpio_write(void *opaque, hwaddr offset,
 s->reg_GPIR = value;
 break;
 default:
-DPRINTF("Bad offset 0x%x\n", offset);
+qemu_log_mask(LOG_GUEST_ERROR,
+  "%s: Bad write offset 0x%"HWADDR_PRIx"\n",
+  __func__, offset);
 }
 }
 
diff --git a/hw/intc/puv3_intc.c b/hw/intc/puv3_intc.c
index e018955ce8..090d4839d1 100644
--- a/hw/intc/puv3_intc.c
+++ b/hw/intc/puv3_intc.c
@@ -16,6 +16,7 @@
 #undef DEBUG_PUV3
 #include "hw/unicore32/puv3.h"
 #include "qemu/module.h"
+#include "qemu/log.h"
 
 #define TYPE_PUV3_INTC "puv3_intc"
 #define PUV3_INTC(obj) OBJECT_CHECK(PUV3INTCState, (obj), TYPE_PUV3_INTC)
@@ -68,7 +69,9 @@ static uint64_t puv3_intc_read(void *opaque, hwaddr offset,
 ret = s->reg_ICPR; /* the same value with ICPR */
 break;
 default:
-DPRINTF("Bad offset %x\n", (int)offset);
+qemu_log_mask(LOG_GUEST_ERROR,
+  "%s: Bad read offset 0x%"HWADDR_PRIx"\n",
+  __func__, offset);
 }
 DPRINTF("offset 0x%x, value 0x%x\n", offset, ret);
 return ret;
@@ -88,7 +91,9 @@ static void puv3_intc_write(void *opaque, hwaddr offset,
 s->reg_ICMR = value;
 break;
 default:
-DPRINTF("Bad offset 0x%x\n", (int)offset);
+qemu_log_mask(LOG_GUEST_ERROR,
+  "%s: Bad write offset 0x%"HWADDR_PRIx"\n",
+  __func__, offset);
 return;
 }
 puv3_intc_update(s);
diff --git a/hw/misc/puv3_pm.c b/hw/misc/puv3_pm.c
index c213500d9c..8989d363cd 100644
--- a/hw/misc/puv3_pm.c
+++ b/hw/misc/puv3_pm.c
@@ -15,6 +15,7 @@
 #undef DEBUG_PUV3
 #include "hw/unicore32/puv3.h"
 #include "qemu/module.h"
+#include "qemu/log.h"
 
 #define TYPE_PUV3_PM "puv3_pm"
 #define PUV3_PM(obj) OBJECT_CHECK(PUV3PMState, (obj), TYPE_PUV3_PM)
@@ -73,7 +74,9 @@ static uint64_t puv3_pm_read(void *opaque, hwaddr offset,
 ret = 0x7;
  

Re: [PATCH] fuzz: add information about useful libFuzzer flags

[Philippe Mathieu-Daudé]

On 5/24/20 4:37 PM, Alexander Bulekov wrote:
> Signed-off-by: Alexander Bulekov 
> ---
>  docs/devel/fuzzing.txt | 33 +
>  1 file changed, 33 insertions(+)
> 
> diff --git a/docs/devel/fuzzing.txt b/docs/devel/fuzzing.txt
> index 324d2cd92b..3458763e53 100644
> --- a/docs/devel/fuzzing.txt
> +++ b/docs/devel/fuzzing.txt
> @@ -45,6 +45,39 @@ Information about these is available by passing -help=1
>  Now the only thing left to do is wait for the fuzzer to trigger potential
>  crashes.
>  
> +== Useful libFuzzer flags ==
> +
> +As mentioned above, libFuzzer accepts some arguments. Passing -help=1 will 
> list
> +the available arguments. In particular, these arguments might be helpful:
> +
> +$CORPUS_DIR/ : Specify a directory as the last argument to libFuzzer. 
> libFuzzer
> +stores each "interesting" input in this corpus directory. The next time you 
> run
> +libFuzzer, it will read all of the inputs from the corpus, and continue 
> fuzzing
> +from there.

Maybe describe we can use multiple directories, the first one is where
new inputs are written.

> +
> +-max_len=4096 : specify the maximum byte-length of the inputs libFuzzer will
> +generate.
> +
> +-close_fd_mask={1,2,3} : close, stderr, or both. Useful for targets that
> +trigger many debug/error messages, or create output on the serial console.
> +
> +-jobs=4 -workers=4 : These arguments configure libFuzzer to run 4 fuzzers in
> +parallel.

The difference between job/worker is not clear...

> Replace 4 with a number appropriate for your machine. Make sure to
> +specify a $CORPUS_DIR, which will allow the parallel fuzzers to share
> +information about the interesting inputs they find.
> +
> +-use_value_profile=1 : For each comparison operation, libFuzzer computes 
> +(caller_pc&4095) | (popcnt(Arg1 ^ Arg2) << 12) and places this in the 
> coverage
> +table. Useful for targets with "magic" constants. If Arg1 came from the 
> fuzzer's
> +input and Arg2 is a magic constant, then each time the hamming distance

"Hamming" (lastname).

Thanks!

> +between Arg1 and Arg2 decreases, libFuzzer adds the input to the corpus.
> +
> +-shrink=1 : Tries to make elements of the corpus "smaller". Might lead to
> +better coverage performance, depending on the target.
> +
> +Note that libFuzzer's exact behavior will depend on the version of
> +clang and libFuzzer used to build the device fuzzers.
> +
>  == Adding a new fuzzer ==
>  Coverage over virtual devices can be improved by adding additional fuzzers.
>  Fuzzers are kept in tests/qtest/fuzz/ and should be added to
> 

Re: [PATCH v6 00/32] Add subcluster allocation to qcow2

[no-reply]

Patchew URL: https://patchew.org/QEMU/cover.1590331741.git.be...@igalia.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC  block/snapshot.o
  CC  block/qapi.o
/tmp/qemu-test/src/block/qcow2-cluster.c: In function 'qcow2_get_host_offset':
/tmp/qemu-test/src/block/qcow2-cluster.c:473:19: error: 'expected_type' may be 
used uninitialized in this function [-Werror=maybe-uninitialized]
 } else if (type != expected_type) {
   ^
/tmp/qemu-test/src/block/qcow2-cluster.c:449:25: note: 'expected_type' was 
declared here
 QCow2SubclusterType expected_type, type;
 ^
/tmp/qemu-test/src/block/qcow2-cluster.c:475:19: error: 'check_offset' may be 
used uninitialized in this function [-Werror=maybe-uninitialized]
 } else if (check_offset) {
   ^
/tmp/qemu-test/src/block/qcow2-cluster.c:447:10: note: 'check_offset' was 
declared here
 bool check_offset;
  ^
/tmp/qemu-test/src/block/qcow2-cluster.c:476:29: error: 'expected_offset' may 
be used uninitialized in this function [-Werror=maybe-uninitialized]
 expected_offset += s->cluster_size;
 ^
/tmp/qemu-test/src/block/qcow2-cluster.c:448:14: note: 'expected_offset' was 
declared here
 uint64_t expected_offset;
  ^
cc1: all warnings being treated as errors
make: *** [block/qcow2-cluster.o] Error 1
make: *** Waiting for unfinished jobs
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in 
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=2b00ed76657046888b159ce8f43ddafe', '-u', 
'1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
'-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 
'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
'/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-8sa3xlcn/src/docker-src.2020-05-24-12.39.44.4969:/var/tmp/qemu:z,ro',
 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=2b00ed76657046888b159ce8f43ddafe
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-8sa3xlcn/src'
make: *** [docker-run-test-quick@centos7] Error 2

real2m27.867s
user0m4.586s


The full log is available at
http://patchew.org/logs/cover.1590331741.git.be...@igalia.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [PATCH RFC v3 03/11] KVM: Fixup kvm_log_clear_one_slot() ioctl return check

[Philippe Mathieu-Daudé]

On 5/24/20 1:20 AM, Peter Xu wrote:
> kvm_vm_ioctl() handles the errno trick already for ioctl() on
> returning -1 for errors.  Fix this.
> 
> Reviewed-by: Dr. David Alan Gilbert 
> Signed-off-by: Peter Xu 
> ---
>  accel/kvm/kvm-all.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index d06cc04079..6e015aa2d4 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -699,14 +699,13 @@ static int kvm_log_clear_one_slot(KVMSlot *mem, int 
> as_id, uint64_t start,
>  d.num_pages = bmap_npages;
>  d.slot = mem->slot | (as_id << 16);
>  
> -if (kvm_vm_ioctl(s, KVM_CLEAR_DIRTY_LOG, &d) == -1) {
> -ret = -errno;
> +ret = kvm_vm_ioctl(s, KVM_CLEAR_DIRTY_LOG, &d);
> +if (ret) {
>  error_report("%s: KVM_CLEAR_DIRTY_LOG failed, slot=%d, "
>   "start=0x%"PRIx64", size=0x%"PRIx32", errno=%d",
>   __func__, d.slot, (uint64_t)d.first_page,
>   (uint32_t)d.num_pages, ret);
>  } else {
> -ret = 0;
>  trace_kvm_clear_dirty_log(d.slot, d.first_page, d.num_pages);
>  }
>  
> 

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v6 00/32] Add subcluster allocation to qcow2

[no-reply]

Patchew URL: https://patchew.org/QEMU/cover.1590331741.git.be...@igalia.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC  block/qed-table.o
  CC  block/qed-cluster.o
  CC  block/qed-check.o
/tmp/qemu-test/src/block/qcow2-cluster.c:1912:54: error: implicit conversion 
from enumeration type 'QCow2ClusterType' (aka 'enum QCow2ClusterType') to 
different enumeration type 'enum qcow2_discard_type' [-Werror,-Wenum-conversion]
qcow2_free_any_clusters(bs, old_l2_entry, 1, type);
~~~  ^~~~
1 error generated.
make: *** [/tmp/qemu-test/src/rules.mak:69: block/qcow2-cluster.o] Error 1
make: *** Waiting for unfinished jobs
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in 
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=247935bf1140449991b4816970528c03', '-u', 
'1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 
'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 
'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', 
'-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-59iulxz0/src/docker-src.2020-05-24-12.30.45.31580:/var/tmp/qemu:z,ro',
 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=247935bf1140449991b4816970528c03
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-59iulxz0/src'
make: *** [docker-run-test-debug@fedora] Error 2

real3m31.674s
user0m8.521s


The full log is available at
http://patchew.org/logs/cover.1590331741.git.be...@igalia.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [PATCH v6 00/32] Add subcluster allocation to qcow2

[no-reply]

Patchew URL: https://patchew.org/QEMU/cover.1590331741.git.be...@igalia.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC  block/qed-table.o
  CC  block/qed-cluster.o
/tmp/qemu-test/src/block/qcow2-cluster.c: In function 'qcow2_get_host_offset':
/tmp/qemu-test/src/block/qcow2-cluster.c:473:19: error: 'expected_type' may be 
used uninitialized in this function [-Werror=maybe-uninitialized]
 } else if (type != expected_type) {
   ^
/tmp/qemu-test/src/block/qcow2-cluster.c:449:25: note: 'expected_type' was 
declared here
 QCow2SubclusterType expected_type, type;
 ^
/tmp/qemu-test/src/block/qcow2-cluster.c:475:19: error: 'check_offset' may be 
used uninitialized in this function [-Werror=maybe-uninitialized]
 } else if (check_offset) {
   ^
/tmp/qemu-test/src/block/qcow2-cluster.c:447:10: note: 'check_offset' was 
declared here
 bool check_offset;
  ^
/tmp/qemu-test/src/block/qcow2-cluster.c:476:29: error: 'expected_offset' may 
be used uninitialized in this function [-Werror=maybe-uninitialized]
 expected_offset += s->cluster_size;
 ^
/tmp/qemu-test/src/block/qcow2-cluster.c:448:14: note: 'expected_offset' was 
declared here
 uint64_t expected_offset;
  ^
cc1: all warnings being treated as errors
make: *** [block/qcow2-cluster.o] Error 1
make: *** Waiting for unfinished jobs
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in 
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=5d5c1a3ec20c4733b2b49cfa8c25190e', '-u', 
'1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
'-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 
'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
'/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-ke8h2yeo/src/docker-src.2020-05-24-12.27.22.23564:/var/tmp/qemu:z,ro',
 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=5d5c1a3ec20c4733b2b49cfa8c25190e
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-ke8h2yeo/src'
make: *** [docker-run-test-quick@centos7] Error 2

real2m28.041s
user0m8.588s


The full log is available at
http://patchew.org/logs/cover.1590331741.git.be...@igalia.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

[PATCH v6 32/32] iotests: Add tests for qcow2 images with extended L2 entries

[Alberto Garcia]

Signed-off-by: Alberto Garcia 
---
 tests/qemu-iotests/271 | 705 +
 tests/qemu-iotests/271.out | 603 +++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 1309 insertions(+)
 create mode 100755 tests/qemu-iotests/271
 create mode 100644 tests/qemu-iotests/271.out

diff --git a/tests/qemu-iotests/271 b/tests/qemu-iotests/271
new file mode 100755
index 00..4dd11772cf
--- /dev/null
+++ b/tests/qemu-iotests/271
@@ -0,0 +1,705 @@
+#!/bin/bash
+#
+# Test qcow2 images with extended L2 entries
+#
+# Copyright (C) 2019-2020 Igalia, S.L.
+# Author: Alberto Garcia 
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=be...@igalia.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+rm -f "$TEST_IMG.raw"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file nfs
+_supported_os Linux
+_unsupported_imgopts extended_l2 compat=0.10 cluster_size data_file
+
+l2_offset=262144 # 0x4
+
+_verify_img()
+{
+$QEMU_IMG compare "$TEST_IMG" "$TEST_IMG.raw" | grep -v 'Images are 
identical'
+$QEMU_IMG check "$TEST_IMG" | _filter_qemu_img_check | \
+grep -v 'No errors were found on the image'
+}
+
+# Compare the bitmap of an extended L2 entry against an expected value
+_verify_l2_bitmap()
+{
+entry_no="$1"# L2 entry number, starting from 0
+expected_alloc="$alloc"  # Space-separated list of allocated subcluster 
indexes
+expected_zero="$zero"# Space-separated list of zero subcluster indexes
+
+offset=$(($l2_offset + $entry_no * 16))
+entry=`peek_file_be "$TEST_IMG" $offset 8`
+offset=$(($offset + 8))
+bitmap=`peek_file_be "$TEST_IMG" $offset 8`
+
+expected_bitmap=0
+for bit in $expected_alloc; do
+expected_bitmap=$(($expected_bitmap | (1 << $bit)))
+done
+for bit in $expected_zero; do
+expected_bitmap=$(($expected_bitmap | (1 << (32 + $bit
+done
+expected_bitmap=`printf "%llu" $expected_bitmap`
+
+printf "L2 entry #%d: 0x%016lx %016lx\n" "$entry_no" "$entry" "$bitmap"
+if [ "$bitmap" != "$expected_bitmap" ]; then
+printf "ERROR: expecting bitmap   0x%016lx\n" "$expected_bitmap"
+fi
+}
+
+_test_write()
+{
+cmd="$1"
+l2_entry_idx="$2"
+[ -n "$l2_entry_idx" ] || l2_entry_idx=0
+raw_cmd=`echo $cmd | sed s/-c//` # Raw images don't support -c
+echo "$cmd"
+$QEMU_IO -c "$cmd" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "$raw_cmd" -f raw "$TEST_IMG.raw" | _filter_qemu_io
+_verify_img
+_verify_l2_bitmap "$l2_entry_idx"
+}
+
+_reset_img()
+{
+size="$1"
+$QEMU_IMG create -f raw "$TEST_IMG.raw" "$size" | _filter_img_create
+if [ "$use_backing_file" = "yes" ]; then
+$QEMU_IMG create -f raw "$TEST_IMG.base" "$size" | _filter_img_create
+$QEMU_IO -c "write -q -P 0xFF 0 $size" -f raw "$TEST_IMG.base" | 
_filter_qemu_io
+$QEMU_IO -c "write -q -P 0xFF 0 $size" -f raw "$TEST_IMG.raw" | 
_filter_qemu_io
+_make_test_img -o extended_l2=on -b "$TEST_IMG.base" "$size"
+else
+_make_test_img -o extended_l2=on "$size"
+fi
+}
+
+# Test that writing to an image with subclusters produces the expected
+# results, in images with and without backing files
+for use_backing_file in yes no; do
+echo
+echo "### Standard write tests (backing file: $use_backing_file) ###"
+echo
+_reset_img 1M
+### Write subcluster #0 (beginning of subcluster) ###
+alloc="0"; zero=""
+_test_write 'write -q -P 1 0 1k'
+
+### Write subcluster #1 (middle of subcluster) ###
+alloc="0 1"; zero=""
+_test_write 'write -q -P 2 3k 512'
+
+### Write subcluster #2 (end of subcluster) ###
+alloc="0 1 2"; zero=""
+_test_write 'write -q -P 3 5k 1k'
+
+### Write subcluster #3 (full subcluster) ###
+alloc="0 1 2 3"; zero=""
+_test_write 'write -q -P 4 6k 2k'
+
+### Write subclusters #4-6 (full subclusters) ###
+alloc="`seq 0 6`"; zero=""
+_test_write 'write -q -P 5 8k 6k'
+
+### Write subclusters #7-9 (partial subclusters) ###
+alloc="`seq 0 9`"; zero=""
+_test_write 'write -q

[PATCH v6 18/32] qcow2: Replace QCOW2_CLUSTER_* with QCOW2_SUBCLUSTER_*

[Alberto Garcia]

In order to support extended L2 entries some functions of the qcow2
driver need to start dealing with subclusters instead of clusters.

qcow2_get_host_offset() is modified to return the subcluster type
instead of the cluster type, and all callers are updated to replace
all values of QCow2ClusterType with their QCow2SubclusterType
equivalents.

This patch only changes the data types, there are no semantic changes.

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.h |  2 +-
 block/qcow2-cluster.c | 10 +++
 block/qcow2.c | 70 ++-
 3 files changed, 42 insertions(+), 40 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 9cd7cbd895..84c06d9bba 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -894,7 +894,7 @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t 
sector_num,
 
 int qcow2_get_host_offset(BlockDriverState *bs, uint64_t offset,
   unsigned int *bytes, uint64_t *host_offset,
-  QCow2ClusterType *cluster_type);
+  QCow2SubclusterType *subcluster_type);
 int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
unsigned int *bytes, uint64_t *host_offset,
QCowL2Meta **m);
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 1e5681b0c6..ed7b92dbb2 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -564,15 +564,15 @@ static int coroutine_fn 
do_perform_cow_write(BlockDriverState *bs,
  * offset that we are interested in.
  *
  * On exit, *bytes is the number of bytes starting at offset that have the same
- * cluster type and (if applicable) are stored contiguously in the image file.
- * The cluster type is stored in *cluster_type.
- * Compressed clusters are always returned one by one.
+ * subcluster type and (if applicable) are stored contiguously in the image
+ * file. The subcluster type is stored in *subcluster_type.
+ * Compressed clusters are always processed one by one.
  *
  * Returns 0 on success, -errno in error cases.
  */
 int qcow2_get_host_offset(BlockDriverState *bs, uint64_t offset,
   unsigned int *bytes, uint64_t *host_offset,
-  QCow2ClusterType *cluster_type)
+  QCow2SubclusterType *subcluster_type)
 {
 BDRVQcow2State *s = bs->opaque;
 unsigned int l2_index;
@@ -713,7 +713,7 @@ out:
 assert(bytes_available - offset_in_cluster <= UINT_MAX);
 *bytes = bytes_available - offset_in_cluster;
 
-*cluster_type = type;
+*subcluster_type = qcow2_cluster_to_subcluster_type(type);
 
 return 0;
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 2806642f68..5303b0070b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2033,7 +2033,7 @@ static int coroutine_fn 
qcow2_co_block_status(BlockDriverState *bs,
 BDRVQcow2State *s = bs->opaque;
 uint64_t host_offset;
 unsigned int bytes;
-QCow2ClusterType type;
+QCow2SubclusterType type;
 int ret, status = 0;
 
 qemu_co_mutex_lock(&s->lock);
@@ -2053,15 +2053,16 @@ static int coroutine_fn 
qcow2_co_block_status(BlockDriverState *bs,
 
 *pnum = bytes;
 
-if ((type == QCOW2_CLUSTER_NORMAL || type == QCOW2_CLUSTER_ZERO_ALLOC) &&
-!s->crypto) {
+if ((type == QCOW2_SUBCLUSTER_NORMAL ||
+ type == QCOW2_SUBCLUSTER_ZERO_ALLOC) && !s->crypto) {
 *map = host_offset;
 *file = s->data_file->bs;
 status |= BDRV_BLOCK_OFFSET_VALID;
 }
-if (type == QCOW2_CLUSTER_ZERO_PLAIN || type == QCOW2_CLUSTER_ZERO_ALLOC) {
+if (type == QCOW2_SUBCLUSTER_ZERO_PLAIN ||
+type == QCOW2_SUBCLUSTER_ZERO_ALLOC) {
 status |= BDRV_BLOCK_ZERO;
-} else if (type != QCOW2_CLUSTER_UNALLOCATED) {
+} else if (type != QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN) {
 status |= BDRV_BLOCK_DATA;
 }
 if (s->metadata_preallocation && (status & BDRV_BLOCK_DATA) &&
@@ -2158,7 +2159,7 @@ typedef struct Qcow2AioTask {
 AioTask task;
 
 BlockDriverState *bs;
-QCow2ClusterType cluster_type; /* only for read */
+QCow2SubclusterType subcluster_type; /* only for read */
 uint64_t host_offset; /* or full descriptor in compressed clusters */
 uint64_t offset;
 uint64_t bytes;
@@ -2171,7 +2172,7 @@ static coroutine_fn int 
qcow2_co_preadv_task_entry(AioTask *task);
 static coroutine_fn int qcow2_add_task(BlockDriverState *bs,
AioTaskPool *pool,
AioTaskFunc func,
-   QCow2ClusterType cluster_type,
+   QCow2SubclusterType subcluster_type,
uint64_t host_offset,
uint64_t offset,
uint64_t bytes,
@@ -2185,

[PATCH v6 07/32] qcow2: Document the Extended L2 Entries feature

[Alberto Garcia]

Subcluster allocation in qcow2 is implemented by extending the
existing L2 table entries and adding additional information to
indicate the allocation status of each subcluster.

This patch documents the changes to the qcow2 format and how they
affect the calculation of the L2 cache size.

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
---
 docs/interop/qcow2.txt | 68 --
 docs/qcow2-cache.txt   | 19 +++-
 2 files changed, 83 insertions(+), 4 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index cb723463f2..64e9345fb4 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -42,6 +42,9 @@ The first cluster of a qcow2 image contains the file header:
 as the maximum cluster size and won't be able to open 
images
 with larger cluster sizes.
 
+Note: if the image has Extended L2 Entries then 
cluster_bits
+must be at least 14 (i.e. 16384 byte clusters).
+
  24 - 31:   size
 Virtual disk size in bytes.
 
@@ -117,7 +120,12 @@ the next fields through header_length.
 clusters. The compression_type field must be
 present and not zero.
 
-Bits 4-63:  Reserved (set to 0)
+Bit 4:  Extended L2 Entries.  If this bit is set then
+L2 table entries use an extended format that
+allows subcluster-based allocation. See the
+Extended L2 Entries section for more details.
+
+Bits 5-63:  Reserved (set to 0)
 
  80 -  87:  compatible_features
 Bitmask of compatible features. An implementation can
@@ -498,7 +506,7 @@ cannot be relaxed without an incompatible layout change).
 Given an offset into the virtual disk, the offset into the image file can be
 obtained as follows:
 
-l2_entries = (cluster_size / sizeof(uint64_t))
+l2_entries = (cluster_size / sizeof(uint64_t))[*]
 
 l2_index = (offset / cluster_size) % l2_entries
 l1_index = (offset / cluster_size) / l2_entries
@@ -508,6 +516,8 @@ obtained as follows:
 
 return cluster_offset + (offset % cluster_size)
 
+[*] this changes if Extended L2 Entries are enabled, see next section
+
 L1 table entry:
 
 Bit  0 -  8:Reserved (set to 0)
@@ -548,7 +558,8 @@ Standard Cluster Descriptor:
 nor is data read from the backing file if the cluster is
 unallocated.
 
-With version 2, this is always 0.
+With version 2 or with extended L2 entries (see the next
+section), this is always 0.
 
  1 -  8:Reserved (set to 0)
 
@@ -585,6 +596,57 @@ file (except if bit 0 in the Standard Cluster Descriptor 
is set). If there is
 no backing file or the backing file is smaller than the image, they shall read
 zeros for all parts that are not covered by the backing file.
 
+== Extended L2 Entries ==
+
+An image uses Extended L2 Entries if bit 4 is set on the incompatible_features
+field of the header.
+
+In these images standard data clusters are divided into 32 subclusters of the
+same size. They are contiguous and start from the beginning of the cluster.
+Subclusters can be allocated independently and the L2 entry contains 
information
+indicating the status of each one of them. Compressed data clusters don't have
+subclusters so they are treated the same as in images without this feature.
+
+The size of an extended L2 entry is 128 bits so the number of entries per table
+is calculated using this formula:
+
+l2_entries = (cluster_size / (2 * sizeof(uint64_t)))
+
+The first 64 bits have the same format as the standard L2 table entry described
+in the previous section, with the exception of bit 0 of the standard cluster
+descriptor.
+
+The last 64 bits contain a subcluster allocation bitmap with this format:
+
+Subcluster Allocation Bitmap (for standard clusters):
+
+Bit  0 - 31:Allocation status (one bit per subcluster)
+
+1: the subcluster is allocated. In this case the
+   host cluster offset field must contain a valid
+   offset.
+0: the subcluster is not allocated. In this case
+   read requests shall go to the backing file or
+   return zeros if there is no backing file data.
+
+Bits are assigned starting from the least significant
+one (i.e. bit x is used for subcluster x).
+
+32 - 63 Subcluster reads as zeros (one bit per subcluster)
+
+1: the subcluster reads as zeros. In this case the
+   allocation status bit must be unset. The host
+   cluster offset field may or m

[PATCH v6 20/32] qcow2: Add subcluster support to calculate_l2_meta()

[Alberto Garcia]

If an image has subclusters then there are more copy-on-write
scenarios that we need to consider. Let's say we have a write request
from the middle of subcluster #3 until the end of the cluster:

1) If we are writing to a newly allocated cluster then we need
   copy-on-write. The previous contents of subclusters #0 to #3 must
   be copied to the new cluster. We can optimize this process by
   skipping all leading unallocated or zero subclusters (the status of
   those skipped subclusters will be reflected in the new L2 bitmap).

2) If we are overwriting an existing cluster:

   2.1) If subcluster #3 is unallocated or has the all-zeroes bit set
then we need copy-on-write (on subcluster #3 only).

   2.2) If subcluster #3 was already allocated then there is no need
for any copy-on-write. However we still need to update the L2
bitmap to reflect possible changes in the allocation status of
subclusters #4 to #31. Because of this, this function checks
if all the overwritten subclusters are already allocated and
in this case it returns without creating a new QCowL2Meta
structure.

After all these changes l2meta_cow_start() and l2meta_cow_end()
are not necessarily cluster-aligned anymore. We need to update the
calculation of old_start and old_end in handle_dependencies() to
guarantee that no two requests try to write on the same cluster.

Signed-off-by: Alberto Garcia 
---
 block/qcow2-cluster.c | 163 +-
 1 file changed, 131 insertions(+), 32 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index ed7b92dbb2..59dd9bda29 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -387,7 +387,6 @@ fail:
  * If the L2 entry is invalid return -errno and set @type to
  * QCOW2_SUBCLUSTER_INVALID.
  */
-G_GNUC_UNUSED
 static int qcow2_get_subcluster_range_type(BlockDriverState *bs,
uint64_t l2_entry,
uint64_t l2_bitmap,
@@ -1110,56 +1109,148 @@ void qcow2_alloc_cluster_abort(BlockDriverState *bs, 
QCowL2Meta *m)
  * If @keep_old is true it means that the clusters were already
  * allocated and will be overwritten. If false then the clusters are
  * new and we have to decrease the reference count of the old ones.
+ *
+ * Returns 0 on success, -errno on failure.
  */
-static void calculate_l2_meta(BlockDriverState *bs,
-  uint64_t host_cluster_offset,
-  uint64_t guest_offset, unsigned bytes,
-  uint64_t *l2_slice, QCowL2Meta **m, bool 
keep_old)
+static int calculate_l2_meta(BlockDriverState *bs, uint64_t 
host_cluster_offset,
+ uint64_t guest_offset, unsigned bytes,
+ uint64_t *l2_slice, QCowL2Meta **m, bool keep_old)
 {
 BDRVQcow2State *s = bs->opaque;
-int l2_index = offset_to_l2_slice_index(s, guest_offset);
-uint64_t l2_entry;
+int sc_index, l2_index = offset_to_l2_slice_index(s, guest_offset);
+uint64_t l2_entry, l2_bitmap;
 unsigned cow_start_from, cow_end_to;
 unsigned cow_start_to = offset_into_cluster(s, guest_offset);
 unsigned cow_end_from = cow_start_to + bytes;
 unsigned nb_clusters = size_to_clusters(s, cow_end_from);
 QCowL2Meta *old_m = *m;
-QCow2ClusterType type;
+QCow2SubclusterType type;
+int i;
+bool skip_cow = keep_old;
 
 assert(nb_clusters <= s->l2_slice_size - l2_index);
 
-/* Return if there's no COW (all clusters are normal and we keep them) */
-if (keep_old) {
-int i;
-for (i = 0; i < nb_clusters; i++) {
-l2_entry = get_l2_entry(s, l2_slice, l2_index + i);
-if (qcow2_get_cluster_type(bs, l2_entry) != QCOW2_CLUSTER_NORMAL) {
-break;
+/* Check the type of all affected subclusters */
+for (i = 0; i < nb_clusters; i++) {
+l2_entry = get_l2_entry(s, l2_slice, l2_index + i);
+l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index + i);
+if (skip_cow) {
+unsigned write_from = MAX(cow_start_to, i << s->cluster_bits);
+unsigned write_to = MIN(cow_end_from, (i + 1) << s->cluster_bits);
+int first_sc = offset_to_sc_index(s, write_from);
+int last_sc = offset_to_sc_index(s, write_to - 1);
+int cnt = qcow2_get_subcluster_range_type(bs, l2_entry, l2_bitmap,
+  first_sc, &type);
+/* Is any of the subclusters of type != QCOW2_SUBCLUSTER_NORMAL ? 
*/
+if (type != QCOW2_SUBCLUSTER_NORMAL || first_sc + cnt <= last_sc) {
+skip_cow = false;
 }
+} else {
+/* If we can't skip the cow we can still look for invalid entries 
*/
+type = qcow2_get_subcluster_type(bs, l2_entry, l2_bitmap, 0);
 }
-if (i == nb_clusters) {
-

[PATCH v6 04/32] qcow2: Split cluster_needs_cow() out of count_cow_clusters()

[Alberto Garcia]

We are going to need it in other places.

Signed-off-by: Alberto Garcia 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/qcow2-cluster.c | 34 +++---
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 61ad638bdc..80f9787461 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1087,6 +1087,24 @@ static void calculate_l2_meta(BlockDriverState *bs,
 QLIST_INSERT_HEAD(&s->cluster_allocs, *m, next_in_flight);
 }
 
+/* Returns true if writing to a cluster requires COW */
+static bool cluster_needs_cow(BlockDriverState *bs, uint64_t l2_entry)
+{
+switch (qcow2_get_cluster_type(bs, l2_entry)) {
+case QCOW2_CLUSTER_NORMAL:
+if (l2_entry & QCOW_OFLAG_COPIED) {
+return false;
+}
+case QCOW2_CLUSTER_UNALLOCATED:
+case QCOW2_CLUSTER_COMPRESSED:
+case QCOW2_CLUSTER_ZERO_PLAIN:
+case QCOW2_CLUSTER_ZERO_ALLOC:
+return true;
+default:
+abort();
+}
+}
+
 /*
  * Returns the number of contiguous clusters that can be used for an allocating
  * write, but require COW to be performed (this includes yet unallocated space,
@@ -1099,25 +1117,11 @@ static int count_cow_clusters(BlockDriverState *bs, int 
nb_clusters,
 
 for (i = 0; i < nb_clusters; i++) {
 uint64_t l2_entry = be64_to_cpu(l2_slice[l2_index + i]);
-QCow2ClusterType cluster_type = qcow2_get_cluster_type(bs, l2_entry);
-
-switch(cluster_type) {
-case QCOW2_CLUSTER_NORMAL:
-if (l2_entry & QCOW_OFLAG_COPIED) {
-goto out;
-}
+if (!cluster_needs_cow(bs, l2_entry)) {
 break;
-case QCOW2_CLUSTER_UNALLOCATED:
-case QCOW2_CLUSTER_COMPRESSED:
-case QCOW2_CLUSTER_ZERO_PLAIN:
-case QCOW2_CLUSTER_ZERO_ALLOC:
-break;
-default:
-abort();
 }
 }
 
-out:
 assert(i <= nb_clusters);
 return i;
 }
-- 
2.20.1

Re: [RFC v2 3/3] cpus: extract out accel-specific code to each accel

[Claudio Fontana]

On 5/22/20 7:10 PM, Claudio Fontana wrote:
> each accelerator registers a new "CpusAccelInterface"
> on initialization, providing functions for starting a vcpu,
> kicking a vcpu, and sychronizing state.
> 
> This way the code in cpus.cc is now all general softmmu code,
> nothing (or almost nothing) accelerator-specific anymore.


NB: here I _really_ wanted to make AccelClass more useful and add methods to it.

In practice though, accessing the methods becomes clunky and expensive with 
lots of indirections from cpus.c

(current_accel() call, -> accelerator, ACCEL_GET_CLASS(), -> AccelCpusInterface 
-> method);

In my first attempt I ended up with a cache of the interface in cpus.c ,
and at that point the usefulness of having the methods at all in AccelClass 
becomes nil, so that resulted in the cpus accel interface being disjointed from 
AccelClass.

Any thoughts?

Ciao,

Claudio


> 
> Signed-off-by: Claudio Fontana 
> ---
>  MAINTAINERS  |   1 +
>  accel/kvm/Makefile.objs  |   2 +
>  accel/kvm/kvm-all.c  |  15 +-
>  accel/kvm/kvm-cpus-interface.c   |  94 
>  accel/kvm/kvm-cpus-interface.h   |   8 +
>  accel/qtest.c|  82 
>  accel/stubs/kvm-stub.c   |   3 +-
>  accel/tcg/Makefile.objs  |   1 +
>  accel/tcg/tcg-all.c  |  12 +-
>  accel/tcg/tcg-cpus-interface.c   | 523 
>  accel/tcg/tcg-cpus-interface.h   |   8 +
>  cpus.c   | 911 
> +++
>  hw/core/cpu.c|   1 +
>  include/sysemu/cpus.h|  44 ++
>  include/sysemu/hvf.h |   1 -
>  include/sysemu/hw_accel.h|  57 +--
>  include/sysemu/kvm.h |   2 +-
>  stubs/Makefile.objs  |   1 +
>  stubs/cpu-synchronize-state.c|  15 +
>  target/i386/Makefile.objs|   7 +-
>  target/i386/hax-all.c|   6 +-
>  target/i386/hax-cpus-interface.c |  85 
>  target/i386/hax-cpus-interface.h |   8 +
>  target/i386/hax-i386.h   |   2 +
>  target/i386/hax-posix.c  |  12 +
>  target/i386/hax-windows.c|  20 +
>  target/i386/hvf/Makefile.objs|   2 +-
>  target/i386/hvf/hvf-cpus-interface.c |  92 
>  target/i386/hvf/hvf-cpus-interface.h |   8 +
>  target/i386/hvf/hvf.c|   5 +-
>  target/i386/whpx-all.c   |   3 +
>  target/i386/whpx-cpus-interface.c|  96 
>  target/i386/whpx-cpus-interface.h|   8 +
>  33 files changed, 1232 insertions(+), 903 deletions(-)
>  create mode 100644 accel/kvm/kvm-cpus-interface.c
>  create mode 100644 accel/kvm/kvm-cpus-interface.h
>  create mode 100644 accel/tcg/tcg-cpus-interface.c
>  create mode 100644 accel/tcg/tcg-cpus-interface.h
>  create mode 100644 stubs/cpu-synchronize-state.c
>  create mode 100644 target/i386/hax-cpus-interface.c
>  create mode 100644 target/i386/hax-cpus-interface.h
>  create mode 100644 target/i386/hvf/hvf-cpus-interface.c
>  create mode 100644 target/i386/hvf/hvf-cpus-interface.h
>  create mode 100644 target/i386/whpx-cpus-interface.c
>  create mode 100644 target/i386/whpx-cpus-interface.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d8df7130ef..d86af5188f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -426,6 +426,7 @@ WHPX CPUs
>  M: Sunil Muthuswamy 
>  S: Supported
>  F: target/i386/whpx-all.c
> +F: target/i386/whpx-cpus-interface.c
>  F: target/i386/whp-dispatch.h
>  F: accel/stubs/whpx-stub.c
>  F: include/sysemu/whpx.h
> diff --git a/accel/kvm/Makefile.objs b/accel/kvm/Makefile.objs
> index fdfa481578..4babbf7796 100644
> --- a/accel/kvm/Makefile.objs
> +++ b/accel/kvm/Makefile.objs
> @@ -1,2 +1,4 @@
>  obj-y += kvm-all.o
> +obj-y += kvm-cpus-interface.o
> +
>  obj-$(call lnot,$(CONFIG_SEV)) += sev-stub.o
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index d06cc04079..c9cbbb1184 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -45,6 +45,10 @@
>  #include "qapi/qapi-types-common.h"
>  #include "qapi/qapi-visit-common.h"
>  #include "sysemu/reset.h"
> +#include "qemu/guest-random.h"
> +
> +#include "sysemu/hw_accel.h"
> +#include "kvm-cpus-interface.h"
>  
>  #include "hw/boards.h"
>  
> @@ -329,7 +333,7 @@ err:
>  return ret;
>  }
>  
> -int kvm_destroy_vcpu(CPUState *cpu)
> +static int do_kvm_destroy_vcpu(CPUState *cpu)
>  {
>  KVMState *s = kvm_state;
>  long mmap_size;
> @@ -363,6 +367,14 @@ err:
>  return ret;
>  }
>  
> +void kvm_destroy_vcpu(CPUState *cpu)
> +{
> +if (do_kvm_destroy_vcpu(cpu) < 0) {
> +error_report("kvm_destroy_vcpu failed");
> +exit(EXIT_FAILURE);
> +}
> +}
> +
>  static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
>  {
>  struct KVMParkedVcpu *cpu;
> @@ -2146,6 +2158,7 @@ static int kvm_init(MachineState *ms)
>  qemu_balloon_inhibit(true);
>  }
>  
> +  

[PATCH v6 08/32] qcow2: Add dummy has_subclusters() function

[Alberto Garcia]

This function will be used by the qcow2 code to check if an image has
subclusters or not.

At the moment this simply returns false. Once all patches needed for
subcluster support are ready then QEMU will be able to create and
read images with subclusters and this function will return the actual
value.

Signed-off-by: Alberto Garcia 
Reviewed-by: Eric Blake 
Reviewed-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.h | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/block/qcow2.h b/block/qcow2.h
index afee84f41f..f41bfd743f 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -510,6 +510,12 @@ typedef enum QCow2MetadataOverlap {
 
 #define INV_OFFSET (-1ULL)
 
+static inline bool has_subclusters(BDRVQcow2State *s)
+{
+/* FIXME: Return false until this feature is complete */
+return false;
+}
+
 static inline uint64_t get_l2_entry(BDRVQcow2State *s, uint64_t *l2_slice,
 int idx)
 {
-- 
2.20.1

[PATCH v6 15/32] qcow2: Add qcow2_get_subcluster_range_type()

[Alberto Garcia]

There are situations in which we want to know how many contiguous
subclusters of the same type there are in a given cluster. This can be
done by simply iterating over the subclusters and repeatedly calling
qcow2_get_subcluster_type() for each one of them.

However once we determined the type of a subcluster we can check the
rest efficiently by counting the number of adjacent ones (or zeroes)
in the bitmap. This is what this function does.

Signed-off-by: Alberto Garcia 
---
 block/qcow2-cluster.c | 51 +++
 1 file changed, 51 insertions(+)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 8b2fc550b7..32dc6e75e3 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -375,6 +375,57 @@ fail:
 return ret;
 }
 
+/*
+ * For a given L2 entry, count the number of contiguous subclusters of
+ * the same type starting from @sc_from. Compressed clusters are
+ * treated as if they were divided into subclusters of size
+ * s->subcluster_size.
+ *
+ * Return the number of contiguous subclusters and set @type to the
+ * subcluster type.
+ *
+ * If the L2 entry is invalid return -errno and set @type to
+ * QCOW2_SUBCLUSTER_INVALID.
+ */
+G_GNUC_UNUSED
+static int qcow2_get_subcluster_range_type(BlockDriverState *bs,
+   uint64_t l2_entry,
+   uint64_t l2_bitmap,
+   unsigned sc_from,
+   QCow2SubclusterType *type)
+{
+BDRVQcow2State *s = bs->opaque;
+uint32_t val;
+
+*type = qcow2_get_subcluster_type(bs, l2_entry, l2_bitmap, sc_from);
+
+if (*type == QCOW2_SUBCLUSTER_INVALID) {
+return -EINVAL;
+} else if (!has_subclusters(s) || *type == QCOW2_SUBCLUSTER_COMPRESSED) {
+return s->subclusters_per_cluster - sc_from;
+}
+
+switch (*type) {
+case QCOW2_SUBCLUSTER_NORMAL:
+val = l2_bitmap | QCOW_OFLAG_SUB_ALLOC_RANGE(0, sc_from);
+return cto32(val) - sc_from;
+
+case QCOW2_SUBCLUSTER_ZERO_PLAIN:
+case QCOW2_SUBCLUSTER_ZERO_ALLOC:
+val = (l2_bitmap | QCOW_OFLAG_SUB_ZERO_RANGE(0, sc_from)) >> 32;
+return cto32(val) - sc_from;
+
+case QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN:
+case QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC:
+val = ((l2_bitmap >> 32) | l2_bitmap)
+& ~QCOW_OFLAG_SUB_ALLOC_RANGE(0, sc_from);
+return ctz32(val) - sc_from;
+
+default:
+g_assert_not_reached();
+}
+}
+
 /*
  * Checks how many clusters in a given L2 slice are contiguous in the image
  * file. As soon as one of the flags in the bitmask stop_flags changes compared
-- 
2.20.1

[PATCH v6 13/32] qcow2: Update get/set_l2_entry() and add get/set_l2_bitmap()

[Alberto Garcia]

Extended L2 entries are 128-bit wide: 64 bits for the entry itself and
64 bits for the subcluster allocation bitmap.

In order to support them correctly get/set_l2_entry() need to be
updated so they take the entry width into account in order to
calculate the correct offset.

This patch also adds the get/set_l2_bitmap() functions that are
used to access the bitmaps. For convenience we allow calling
get_l2_bitmap() on images without subclusters. In this case the
returned value is always 0 and has no meaning.

Signed-off-by: Alberto Garcia 
Reviewed-by: Eric Blake 
---
 block/qcow2.h | 21 +
 1 file changed, 21 insertions(+)

diff --git a/block/qcow2.h b/block/qcow2.h
index 95cc16a089..5c6bf48c7a 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -533,15 +533,36 @@ static inline size_t l2_entry_size(BDRVQcow2State *s)
 static inline uint64_t get_l2_entry(BDRVQcow2State *s, uint64_t *l2_slice,
 int idx)
 {
+idx *= l2_entry_size(s) / sizeof(uint64_t);
 return be64_to_cpu(l2_slice[idx]);
 }
 
+static inline uint64_t get_l2_bitmap(BDRVQcow2State *s, uint64_t *l2_slice,
+ int idx)
+{
+if (has_subclusters(s)) {
+idx *= l2_entry_size(s) / sizeof(uint64_t);
+return be64_to_cpu(l2_slice[idx + 1]);
+} else {
+return 0; /* For convenience only; this value has no meaning. */
+}
+}
+
 static inline void set_l2_entry(BDRVQcow2State *s, uint64_t *l2_slice,
 int idx, uint64_t entry)
 {
+idx *= l2_entry_size(s) / sizeof(uint64_t);
 l2_slice[idx] = cpu_to_be64(entry);
 }
 
+static inline void set_l2_bitmap(BDRVQcow2State *s, uint64_t *l2_slice,
+ int idx, uint64_t bitmap)
+{
+assert(has_subclusters(s));
+idx *= l2_entry_size(s) / sizeof(uint64_t);
+l2_slice[idx + 1] = cpu_to_be64(bitmap);
+}
+
 static inline bool has_data_file(BlockDriverState *bs)
 {
 BDRVQcow2State *s = bs->opaque;
-- 
2.20.1

[PATCH v6 21/32] qcow2: Add subcluster support to qcow2_get_host_offset()

[Alberto Garcia]

The logic of this function remains pretty much the same, except that
it uses count_contiguous_subclusters(), which combines the logic of
count_contiguous_clusters() / count_contiguous_clusters_unallocated()
and checks individual subclusters.

qcow2_cluster_to_subcluster_type() is not necessary as a separate
function anymore so it's inlined into its caller.

Signed-off-by: Alberto Garcia 
---
 block/qcow2.h |  38 ---
 block/qcow2-cluster.c | 150 ++
 2 files changed, 92 insertions(+), 96 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 84c06d9bba..32c68ead9a 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -710,29 +710,6 @@ static inline QCow2ClusterType 
qcow2_get_cluster_type(BlockDriverState *bs,
 }
 }
 
-/*
- * For an image without extended L2 entries, return the
- * QCow2SubclusterType equivalent of a given QCow2ClusterType.
- */
-static inline
-QCow2SubclusterType qcow2_cluster_to_subcluster_type(QCow2ClusterType type)
-{
-switch (type) {
-case QCOW2_CLUSTER_COMPRESSED:
-return QCOW2_SUBCLUSTER_COMPRESSED;
-case QCOW2_CLUSTER_ZERO_PLAIN:
-return QCOW2_SUBCLUSTER_ZERO_PLAIN;
-case QCOW2_CLUSTER_ZERO_ALLOC:
-return QCOW2_SUBCLUSTER_ZERO_ALLOC;
-case QCOW2_CLUSTER_NORMAL:
-return QCOW2_SUBCLUSTER_NORMAL;
-case QCOW2_CLUSTER_UNALLOCATED:
-return QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN;
-default:
-g_assert_not_reached();
-}
-}
-
 /*
  * In an image without subsclusters @l2_bitmap is ignored and
  * @sc_index must be 0.
@@ -776,7 +753,20 @@ QCow2SubclusterType 
qcow2_get_subcluster_type(BlockDriverState *bs,
 g_assert_not_reached();
 }
 } else {
-return qcow2_cluster_to_subcluster_type(type);
+switch (type) {
+case QCOW2_CLUSTER_COMPRESSED:
+return QCOW2_SUBCLUSTER_COMPRESSED;
+case QCOW2_CLUSTER_ZERO_PLAIN:
+return QCOW2_SUBCLUSTER_ZERO_PLAIN;
+case QCOW2_CLUSTER_ZERO_ALLOC:
+return QCOW2_SUBCLUSTER_ZERO_ALLOC;
+case QCOW2_CLUSTER_NORMAL:
+return QCOW2_SUBCLUSTER_NORMAL;
+case QCOW2_CLUSTER_UNALLOCATED:
+return QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN;
+default:
+g_assert_not_reached();
+}
 }
 }
 
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 59dd9bda29..2f3bd3a882 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -426,66 +426,66 @@ static int 
qcow2_get_subcluster_range_type(BlockDriverState *bs,
 }
 
 /*
- * Checks how many clusters in a given L2 slice are contiguous in the image
- * file. As soon as one of the flags in the bitmask stop_flags changes compared
- * to the first cluster, the search is stopped and the cluster is not counted
- * as contiguous. (This allows it, for example, to stop at the first compressed
- * cluster which may require a different handling)
+ * Return the number of contiguous subclusters of the exact same type
+ * in a given L2 slice, starting from cluster @l2_index, subcluster
+ * @sc_index. Allocated subclusters are required to be contiguous in
+ * the image file.
+ * At most @nb_clusters are checked (note that this means clusters,
+ * not subclusters).
+ * Compressed clusters are always processed one by one but for the
+ * purpose of this count they are treated as if they were divided into
+ * subclusters of size s->subcluster_size.
+ * On failure return -errno and update @l2_index to point to the
+ * invalid entry.
  */
-static int count_contiguous_clusters(BlockDriverState *bs, int nb_clusters,
-int cluster_size, uint64_t *l2_slice, int l2_index, uint64_t 
stop_flags)
+static int count_contiguous_subclusters(BlockDriverState *bs, int nb_clusters,
+unsigned sc_index, uint64_t *l2_slice,
+unsigned *l2_index)
 {
 BDRVQcow2State *s = bs->opaque;
-int i;
-QCow2ClusterType first_cluster_type;
-uint64_t mask = stop_flags | L2E_OFFSET_MASK | QCOW_OFLAG_COMPRESSED;
-uint64_t first_entry = get_l2_entry(s, l2_slice, l2_index);
-uint64_t offset = first_entry & mask;
+int i, count = 0;
+bool check_offset;
+uint64_t expected_offset;
+QCow2SubclusterType expected_type, type;
 
-first_cluster_type = qcow2_get_cluster_type(bs, first_entry);
-if (first_cluster_type == QCOW2_CLUSTER_UNALLOCATED) {
-return 0;
-}
-
-/* must be allocated */
-assert(first_cluster_type == QCOW2_CLUSTER_NORMAL ||
-   first_cluster_type == QCOW2_CLUSTER_ZERO_ALLOC);
+assert(*l2_index + nb_clusters <= s->l2_size);
 
 for (i = 0; i < nb_clusters; i++) {
-uint64_t l2_entry = get_l2_entry(s, l2_slice, l2_index + i) & mask;
-if (offset + (uint64_t) i * cluster_size != l2_entry) {
+unsigned first_sc = (i == 0) ? sc_index : 0;
+uint64_t l2_entry = get_l2_entry(s, l2_slice, *l2_i

[PATCH v6 11/32] qcow2: Add offset_into_subcluster() and size_to_subclusters()

[Alberto Garcia]

Like offset_into_cluster() and size_to_clusters(), but for
subclusters.

Signed-off-by: Alberto Garcia 
Reviewed-by: Eric Blake 
---
 block/qcow2.h | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/block/qcow2.h b/block/qcow2.h
index ca73ac9b67..79c4f82383 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -555,11 +555,21 @@ static inline int64_t offset_into_cluster(BDRVQcow2State 
*s, int64_t offset)
 return offset & (s->cluster_size - 1);
 }
 
+static inline int64_t offset_into_subcluster(BDRVQcow2State *s, int64_t offset)
+{
+return offset & (s->subcluster_size - 1);
+}
+
 static inline uint64_t size_to_clusters(BDRVQcow2State *s, uint64_t size)
 {
 return (size + (s->cluster_size - 1)) >> s->cluster_bits;
 }
 
+static inline uint64_t size_to_subclusters(BDRVQcow2State *s, uint64_t size)
+{
+return (size + (s->subcluster_size - 1)) >> s->subcluster_bits;
+}
+
 static inline int64_t size_to_l1(BDRVQcow2State *s, int64_t size)
 {
 int shift = s->cluster_bits + s->l2_bits;
-- 
2.20.1

[PATCH v6 25/32] qcow2: Update L2 bitmap in qcow2_alloc_cluster_link_l2()

[Alberto Garcia]

The L2 bitmap needs to be updated after each write to indicate what
new subclusters are now allocated. This needs to happen even if the
cluster was already allocated and the L2 entry was otherwise valid.

In some cases however a write operation doesn't need change the L2
bitmap (because all affected subclusters were already allocated). This
is detected in calculate_l2_meta(), and qcow2_alloc_cluster_link_l2()
is never called in those cases.

Signed-off-by: Alberto Garcia 
---
 block/qcow2-cluster.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 5e70b76c21..c8d6e16237 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1061,6 +1061,24 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, 
QCowL2Meta *m)
 assert((offset & L2E_OFFSET_MASK) == offset);
 
 set_l2_entry(s, l2_slice, l2_index + i, offset | QCOW_OFLAG_COPIED);
+
+/* Update bitmap with the subclusters that were just written */
+if (has_subclusters(s)) {
+uint64_t l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index + i);
+unsigned written_from = m->cow_start.offset;
+unsigned written_to = m->cow_end.offset + m->cow_end.nb_bytes ?:
+m->nb_clusters << s->cluster_bits;
+int first_sc, last_sc;
+/* Narrow written_from and written_to down to the current cluster 
*/
+written_from = MAX(written_from, i << s->cluster_bits);
+written_to   = MIN(written_to, (i + 1) << s->cluster_bits);
+assert(written_from < written_to);
+first_sc = offset_to_sc_index(s, written_from);
+last_sc  = offset_to_sc_index(s, written_to - 1);
+l2_bitmap |= QCOW_OFLAG_SUB_ALLOC_RANGE(first_sc, last_sc + 1);
+l2_bitmap &= ~QCOW_OFLAG_SUB_ZERO_RANGE(first_sc, last_sc + 1);
+set_l2_bitmap(s, l2_slice, l2_index + i, l2_bitmap);
+}
  }
 
 
-- 
2.20.1

[PATCH v6 27/32] qcow2: Add subcluster support to handle_alloc_space()

[Alberto Garcia]

The bdrv_co_pwrite_zeroes() call here fills complete clusters with
zeroes, but it can happen that some subclusters are not part of the
write request or the copy-on-write. This patch makes sure that only
the affected subclusters are overwritten.

A potential improvement would be to also fill with zeroes the other
subclusters if we can guarantee that we are not overwriting existing
data. However this would waste more disk space, so we should first
evaluate if it's really worth doing.

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 8126ad8fa0..430b4e423a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2411,6 +2411,9 @@ static int handle_alloc_space(BlockDriverState *bs, 
QCowL2Meta *l2meta)
 
 for (m = l2meta; m != NULL; m = m->next) {
 int ret;
+uint64_t start_offset = m->alloc_offset + m->cow_start.offset;
+unsigned nb_bytes = m->cow_end.offset + m->cow_end.nb_bytes -
+m->cow_start.offset;
 
 if (!m->cow_start.nb_bytes && !m->cow_end.nb_bytes) {
 continue;
@@ -2425,16 +2428,14 @@ static int handle_alloc_space(BlockDriverState *bs, 
QCowL2Meta *l2meta)
  * efficiently zero out the whole clusters
  */
 
-ret = qcow2_pre_write_overlap_check(bs, 0, m->alloc_offset,
-m->nb_clusters * s->cluster_size,
+ret = qcow2_pre_write_overlap_check(bs, 0, start_offset, nb_bytes,
 true);
 if (ret < 0) {
 return ret;
 }
 
 BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC_SPACE);
-ret = bdrv_co_pwrite_zeroes(s->data_file, m->alloc_offset,
-m->nb_clusters * s->cluster_size,
+ret = bdrv_co_pwrite_zeroes(s->data_file, start_offset, nb_bytes,
 BDRV_REQ_NO_FALLBACK);
 if (ret < 0) {
 if (ret != -ENOTSUP && ret != -EAGAIN) {
-- 
2.20.1

[PATCH v6 17/32] qcow2: Add cluster type parameter to qcow2_get_host_offset()

[Alberto Garcia]

This function returns an integer that can be either an error code or a
cluster type (a value from the QCow2ClusterType enum).

We are going to start using subcluster types instead of cluster types
in some functions so it's better to use the exact data types instead
of integers for clarity and in order to detect errors more easily.

This patch makes qcow2_get_host_offset() return 0 on success and
puts the returned cluster type in a separate parameter. There are no
semantic changes.

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.h |  3 ++-
 block/qcow2-cluster.c | 11 +++
 block/qcow2.c | 37 ++---
 3 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index ba7614e406..9cd7cbd895 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -893,7 +893,8 @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t 
sector_num,
   uint8_t *buf, int nb_sectors, bool enc, Error 
**errp);
 
 int qcow2_get_host_offset(BlockDriverState *bs, uint64_t offset,
-  unsigned int *bytes, uint64_t *host_offset);
+  unsigned int *bytes, uint64_t *host_offset,
+  QCow2ClusterType *cluster_type);
 int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
unsigned int *bytes, uint64_t *host_offset,
QCowL2Meta **m);
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 32dc6e75e3..1e5681b0c6 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -565,13 +565,14 @@ static int coroutine_fn 
do_perform_cow_write(BlockDriverState *bs,
  *
  * On exit, *bytes is the number of bytes starting at offset that have the same
  * cluster type and (if applicable) are stored contiguously in the image file.
+ * The cluster type is stored in *cluster_type.
  * Compressed clusters are always returned one by one.
  *
- * Returns the cluster type (QCOW2_CLUSTER_*) on success, -errno in error
- * cases.
+ * Returns 0 on success, -errno in error cases.
  */
 int qcow2_get_host_offset(BlockDriverState *bs, uint64_t offset,
-  unsigned int *bytes, uint64_t *host_offset)
+  unsigned int *bytes, uint64_t *host_offset,
+  QCow2ClusterType *cluster_type)
 {
 BDRVQcow2State *s = bs->opaque;
 unsigned int l2_index;
@@ -712,7 +713,9 @@ out:
 assert(bytes_available - offset_in_cluster <= UINT_MAX);
 *bytes = bytes_available - offset_in_cluster;
 
-return type;
+*cluster_type = type;
+
+return 0;
 
 fail:
 qcow2_cache_put(s->l2_table_cache, (void **)&l2_slice);
diff --git a/block/qcow2.c b/block/qcow2.c
index d4851e0e18..2806642f68 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2033,6 +2033,7 @@ static int coroutine_fn 
qcow2_co_block_status(BlockDriverState *bs,
 BDRVQcow2State *s = bs->opaque;
 uint64_t host_offset;
 unsigned int bytes;
+QCow2ClusterType type;
 int ret, status = 0;
 
 qemu_co_mutex_lock(&s->lock);
@@ -2044,7 +2045,7 @@ static int coroutine_fn 
qcow2_co_block_status(BlockDriverState *bs,
 }
 
 bytes = MIN(INT_MAX, count);
-ret = qcow2_get_host_offset(bs, offset, &bytes, &host_offset);
+ret = qcow2_get_host_offset(bs, offset, &bytes, &host_offset, &type);
 qemu_co_mutex_unlock(&s->lock);
 if (ret < 0) {
 return ret;
@@ -2052,15 +2053,15 @@ static int coroutine_fn 
qcow2_co_block_status(BlockDriverState *bs,
 
 *pnum = bytes;
 
-if ((ret == QCOW2_CLUSTER_NORMAL || ret == QCOW2_CLUSTER_ZERO_ALLOC) &&
+if ((type == QCOW2_CLUSTER_NORMAL || type == QCOW2_CLUSTER_ZERO_ALLOC) &&
 !s->crypto) {
 *map = host_offset;
 *file = s->data_file->bs;
 status |= BDRV_BLOCK_OFFSET_VALID;
 }
-if (ret == QCOW2_CLUSTER_ZERO_PLAIN || ret == QCOW2_CLUSTER_ZERO_ALLOC) {
+if (type == QCOW2_CLUSTER_ZERO_PLAIN || type == QCOW2_CLUSTER_ZERO_ALLOC) {
 status |= BDRV_BLOCK_ZERO;
-} else if (ret != QCOW2_CLUSTER_UNALLOCATED) {
+} else if (type != QCOW2_CLUSTER_UNALLOCATED) {
 status |= BDRV_BLOCK_DATA;
 }
 if (s->metadata_preallocation && (status & BDRV_BLOCK_DATA) &&
@@ -2269,6 +2270,7 @@ static coroutine_fn int 
qcow2_co_preadv_part(BlockDriverState *bs,
 int ret = 0;
 unsigned int cur_bytes; /* number of bytes in current iteration */
 uint64_t host_offset = 0;
+QCow2ClusterType type;
 AioTaskPool *aio = NULL;
 
 while (bytes != 0 && aio_task_pool_status(aio) == 0) {
@@ -2280,22 +2282,23 @@ static coroutine_fn int 
qcow2_co_preadv_part(BlockDriverState *bs,
 }
 
 qemu_co_mutex_lock(&s->lock);
-ret = qcow2_get_host_offset(bs, offset, &cur_bytes, &host_offset);
+ret = qcow2_get_host_offset(bs, offset, &cur_bytes,
+ 

[PATCH v6 24/32] qcow2: Add subcluster support to check_refcounts_l2()

[Alberto Garcia]

Setting the QCOW_OFLAG_ZERO bit of the L2 entry is forbidden if an
image has subclusters. Instead, the individual 'all zeroes' bits must
be used.

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
---
 block/qcow2-refcount.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 770c5dbc83..696e4dad07 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1686,8 +1686,13 @@ static int check_refcounts_l2(BlockDriverState *bs, 
BdrvCheckResult *res,
 int ign = active ? QCOW2_OL_ACTIVE_L2 :
QCOW2_OL_INACTIVE_L2;
 
-l2_entry = QCOW_OFLAG_ZERO;
-set_l2_entry(s, l2_table, i, l2_entry);
+if (has_subclusters(s)) {
+set_l2_entry(s, l2_table, i, 0);
+set_l2_bitmap(s, l2_table, i,
+  QCOW_L2_BITMAP_ALL_ZEROES);
+} else {
+set_l2_entry(s, l2_table, i, QCOW_OFLAG_ZERO);
+}
 ret = qcow2_pre_write_overlap_check(bs, ign,
 l2e_offset, l2_entry_size(s), false);
 if (ret < 0) {
-- 
2.20.1

[PATCH v6 31/32] qcow2: Assert that expand_zero_clusters_in_l1() does not support subclusters

[Alberto Garcia]

This function is only used by qcow2_expand_zero_clusters() to
downgrade a qcow2 image to a previous version. It is however not
possible to downgrade an image with extended L2 entries because older
versions of qcow2 do not have this feature.

Signed-off-by: Alberto Garcia 
Reviewed-by: Eric Blake 
---
 block/qcow2-cluster.c  | 8 +++-
 tests/qemu-iotests/061 | 6 ++
 tests/qemu-iotests/061.out | 5 +
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index b808d6ad95..608e785dd6 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -2156,6 +2156,9 @@ static int expand_zero_clusters_in_l1(BlockDriverState 
*bs, uint64_t *l1_table,
 int ret;
 int i, j;
 
+/* qcow2_downgrade() is not allowed in images with subclusters */
+assert(!has_subclusters(s));
+
 slice_size2 = s->l2_slice_size * l2_entry_size(s);
 n_slices = s->cluster_size / slice_size2;
 
@@ -2224,7 +2227,8 @@ static int expand_zero_clusters_in_l1(BlockDriverState 
*bs, uint64_t *l1_table,
 if (cluster_type == QCOW2_CLUSTER_ZERO_PLAIN) {
 if (!bs->backing) {
 /* not backed; therefore we can simply deallocate the
- * cluster */
+ * cluster. No need to call set_l2_bitmap(), this
+ * function doesn't support images with subclusters. */
 set_l2_entry(s, l2_slice, j, 0);
 l2_dirty = true;
 continue;
@@ -2295,6 +2299,8 @@ static int expand_zero_clusters_in_l1(BlockDriverState 
*bs, uint64_t *l1_table,
 } else {
 set_l2_entry(s, l2_slice, j, offset);
 }
+/* No need to call set_l2_bitmap() after set_l2_entry() because
+ * this function doesn't support images with subclusters. */
 l2_dirty = true;
 }
 
diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
index 10eb243164..23add2dfe3 100755
--- a/tests/qemu-iotests/061
+++ b/tests/qemu-iotests/061
@@ -303,6 +303,12 @@ $QEMU_IMG amend -o "compat=0.10" "$TEST_IMG"
 _img_info --format-specific
 _check_test_img
 
+echo
+echo "=== Testing version downgrade with extended L2 entries ==="
+echo
+_make_test_img -o "compat=1.1,extended_l2=on" 64M
+$QEMU_IMG amend -o "compat=0.10" "$TEST_IMG"
+
 echo
 echo "=== Try changing the external data file ==="
 echo
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index 39812d8cf8..c1acdbd751 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -528,6 +528,11 @@ Format specific information:
 extended l2: false
 No errors were found on the image.
 
+=== Testing version downgrade with extended L2 entries ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+qemu-img: Cannot downgrade an image with incompatible features 0x10 set
+
 === Try changing the external data file ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-- 
2.20.1

[PATCH v6 22/32] qcow2: Add subcluster support to zero_in_l2_slice()

[Alberto Garcia]

The QCOW_OFLAG_ZERO bit that indicates that a cluster reads as
zeroes is only used in standard L2 entries. Extended L2 entries use
individual 'all zeroes' bits for each subcluster.

This must be taken into account when updating the L2 entry and also
when deciding that an existing entry does not need to be updated.

Signed-off-by: Alberto Garcia 
---
 block/qcow2-cluster.c | 36 +++-
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 2f3bd3a882..4e59bbd545 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1956,7 +1956,6 @@ static int zero_in_l2_slice(BlockDriverState *bs, 
uint64_t offset,
 int l2_index;
 int ret;
 int i;
-bool unmap = !!(flags & BDRV_REQ_MAY_UNMAP);
 
 ret = get_cluster_table(bs, offset, &l2_slice, &l2_index);
 if (ret < 0) {
@@ -1968,28 +1967,31 @@ static int zero_in_l2_slice(BlockDriverState *bs, 
uint64_t offset,
 assert(nb_clusters <= INT_MAX);
 
 for (i = 0; i < nb_clusters; i++) {
-uint64_t old_offset;
-QCow2ClusterType cluster_type;
+uint64_t old_l2_entry = get_l2_entry(s, l2_slice, l2_index + i);
+uint64_t old_l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index + i);
+QCow2ClusterType type = qcow2_get_cluster_type(bs, old_l2_entry);
+bool unmap = (type == QCOW2_CLUSTER_COMPRESSED) ||
+((flags & BDRV_REQ_MAY_UNMAP) && qcow2_cluster_is_allocated(type));
+uint64_t new_l2_entry = unmap ? 0 : old_l2_entry;
+uint64_t new_l2_bitmap = old_l2_bitmap;
 
-old_offset = get_l2_entry(s, l2_slice, l2_index + i);
+if (has_subclusters(s)) {
+new_l2_bitmap = QCOW_L2_BITMAP_ALL_ZEROES;
+} else {
+new_l2_entry |= QCOW_OFLAG_ZERO;
+}
 
-/*
- * Minimize L2 changes if the cluster already reads back as
- * zeroes with correct allocation.
- */
-cluster_type = qcow2_get_cluster_type(bs, old_offset);
-if (cluster_type == QCOW2_CLUSTER_ZERO_PLAIN ||
-(cluster_type == QCOW2_CLUSTER_ZERO_ALLOC && !unmap)) {
+if (old_l2_entry == new_l2_entry && old_l2_bitmap == new_l2_bitmap) {
 continue;
 }
 
 qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice);
-if (cluster_type == QCOW2_CLUSTER_COMPRESSED || unmap) {
-set_l2_entry(s, l2_slice, l2_index + i, QCOW_OFLAG_ZERO);
-qcow2_free_any_clusters(bs, old_offset, 1, QCOW2_DISCARD_REQUEST);
-} else {
-uint64_t entry = get_l2_entry(s, l2_slice, l2_index + i);
-set_l2_entry(s, l2_slice, l2_index + i, entry | QCOW_OFLAG_ZERO);
+if (unmap) {
+qcow2_free_any_clusters(bs, old_l2_entry, 1, 
QCOW2_DISCARD_REQUEST);
+}
+set_l2_entry(s, l2_slice, l2_index + i, new_l2_entry);
+if (has_subclusters(s)) {
+set_l2_bitmap(s, l2_slice, l2_index + i, new_l2_bitmap);
 }
 }
 
-- 
2.20.1

[PATCH v6 19/32] qcow2: Handle QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC

[Alberto Garcia]

When dealing with subcluster types there is a new value called
QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC that has no equivalent in
QCow2ClusterType.

This patch handles that value in all places where subcluster types
are processed.

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 5303b0070b..8126ad8fa0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2054,7 +2054,8 @@ static int coroutine_fn 
qcow2_co_block_status(BlockDriverState *bs,
 *pnum = bytes;
 
 if ((type == QCOW2_SUBCLUSTER_NORMAL ||
- type == QCOW2_SUBCLUSTER_ZERO_ALLOC) && !s->crypto) {
+ type == QCOW2_SUBCLUSTER_ZERO_ALLOC ||
+ type == QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC) && !s->crypto) {
 *map = host_offset;
 *file = s->data_file->bs;
 status |= BDRV_BLOCK_OFFSET_VALID;
@@ -2062,7 +2063,8 @@ static int coroutine_fn 
qcow2_co_block_status(BlockDriverState *bs,
 if (type == QCOW2_SUBCLUSTER_ZERO_PLAIN ||
 type == QCOW2_SUBCLUSTER_ZERO_ALLOC) {
 status |= BDRV_BLOCK_ZERO;
-} else if (type != QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN) {
+} else if (type != QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN &&
+   type != QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC) {
 status |= BDRV_BLOCK_DATA;
 }
 if (s->metadata_preallocation && (status & BDRV_BLOCK_DATA) &&
@@ -2225,6 +2227,7 @@ static coroutine_fn int 
qcow2_co_preadv_task(BlockDriverState *bs,
 g_assert_not_reached();
 
 case QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN:
+case QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC:
 assert(bs->backing); /* otherwise handled in qcow2_co_preadv_part */
 
 BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
@@ -2293,7 +2296,8 @@ static coroutine_fn int 
qcow2_co_preadv_part(BlockDriverState *bs,
 
 if (type == QCOW2_SUBCLUSTER_ZERO_PLAIN ||
 type == QCOW2_SUBCLUSTER_ZERO_ALLOC ||
-(type == QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN && !bs->backing))
+(type == QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN && !bs->backing) ||
+(type == QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC && !bs->backing))
 {
 qemu_iovec_memset(qiov, qiov_offset, 0, cur_bytes);
 } else {
@@ -3865,6 +3869,7 @@ static coroutine_fn int 
qcow2_co_pwrite_zeroes(BlockDriverState *bs,
 ret = qcow2_get_host_offset(bs, offset, &nr, &off, &type);
 if (ret < 0 ||
 (type != QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN &&
+ type != QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC &&
  type != QCOW2_SUBCLUSTER_ZERO_PLAIN &&
  type != QCOW2_SUBCLUSTER_ZERO_ALLOC)) {
 qemu_co_mutex_unlock(&s->lock);
@@ -3943,6 +3948,7 @@ qcow2_co_copy_range_from(BlockDriverState *bs,
 
 switch (type) {
 case QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN:
+case QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC:
 if (bs->backing && bs->backing->bs) {
 int64_t backing_length = bdrv_getlength(bs->backing->bs);
 if (src_offset >= backing_length) {
-- 
2.20.1

[PATCH v6 28/32] qcow2: Add subcluster support to qcow2_co_pwrite_zeroes()

[Alberto Garcia]

This works now at the subcluster level and pwrite_zeroes_alignment is
updated accordingly.

qcow2_cluster_zeroize() is turned into qcow2_subcluster_zeroize() with
the following changes:

   - The request can now be subcluster-aligned.

   - The cluster-aligned body of the request is still zeroized using
 zero_in_l2_slice() as before.

   - The subcluster-aligned head and tail of the request are zeroized
 with the new zero_l2_subclusters() function.

There is just one thing to take into account for a possible future
improvement: compressed clusters cannot be partially zeroized so
zero_l2_subclusters() on the head or the tail can return -ENOTSUP.
This makes the caller repeat the *complete* request and write actual
zeroes to disk. This is sub-optimal because

   1) if the head area was compressed we would still be able to use
  the fast path for the body and possibly the tail.

   2) if the tail area was compressed we are writing zeroes to the
  head and the body areas, which are already zeroized.

Signed-off-by: Alberto Garcia 
---
 block/qcow2.h |  4 +--
 block/qcow2-cluster.c | 80 +++
 block/qcow2.c | 27 ---
 3 files changed, 90 insertions(+), 21 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 32c68ead9a..ece5f1cb5a 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -898,8 +898,8 @@ void qcow2_alloc_cluster_abort(BlockDriverState *bs, 
QCowL2Meta *m);
 int qcow2_cluster_discard(BlockDriverState *bs, uint64_t offset,
   uint64_t bytes, enum qcow2_discard_type type,
   bool full_discard);
-int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t offset,
-  uint64_t bytes, int flags);
+int qcow2_subcluster_zeroize(BlockDriverState *bs, uint64_t offset,
+ uint64_t bytes, int flags);
 
 int qcow2_expand_zero_clusters(BlockDriverState *bs,
BlockDriverAmendStatusCB *status_cb,
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 3639dc8057..b808d6ad95 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -2014,12 +2014,58 @@ static int zero_in_l2_slice(BlockDriverState *bs, 
uint64_t offset,
 return nb_clusters;
 }
 
-int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t offset,
-  uint64_t bytes, int flags)
+static int zero_l2_subclusters(BlockDriverState *bs, uint64_t offset,
+   unsigned nb_subclusters)
+{
+BDRVQcow2State *s = bs->opaque;
+uint64_t *l2_slice;
+uint64_t old_l2_bitmap, l2_bitmap;
+int l2_index, ret, sc = offset_to_sc_index(s, offset);
+
+/* For full clusters use zero_in_l2_slice() instead */
+assert(nb_subclusters > 0 && nb_subclusters < s->subclusters_per_cluster);
+assert(sc + nb_subclusters <= s->subclusters_per_cluster);
+
+ret = get_cluster_table(bs, offset, &l2_slice, &l2_index);
+if (ret < 0) {
+return ret;
+}
+
+switch (qcow2_get_cluster_type(bs, get_l2_entry(s, l2_slice, l2_index))) {
+case QCOW2_CLUSTER_COMPRESSED:
+ret = -ENOTSUP; /* We cannot partially zeroize compressed clusters */
+goto out;
+case QCOW2_CLUSTER_NORMAL:
+case QCOW2_CLUSTER_UNALLOCATED:
+break;
+default:
+g_assert_not_reached();
+}
+
+old_l2_bitmap = l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index);
+
+l2_bitmap |=  QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters);
+l2_bitmap &= ~QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + nb_subclusters);
+
+if (old_l2_bitmap != l2_bitmap) {
+set_l2_bitmap(s, l2_slice, l2_index, l2_bitmap);
+qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice);
+}
+
+ret = 0;
+out:
+qcow2_cache_put(s->l2_table_cache, (void **) &l2_slice);
+
+return ret;
+}
+
+int qcow2_subcluster_zeroize(BlockDriverState *bs, uint64_t offset,
+ uint64_t bytes, int flags)
 {
 BDRVQcow2State *s = bs->opaque;
 uint64_t end_offset = offset + bytes;
 uint64_t nb_clusters;
+unsigned head, tail;
 int64_t cleared;
 int ret;
 
@@ -2034,8 +2080,8 @@ int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t 
offset,
 }
 
 /* Caller must pass aligned values, except at image end */
-assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
-assert(QEMU_IS_ALIGNED(end_offset, s->cluster_size) ||
+assert(offset_into_subcluster(s, offset) == 0);
+assert(offset_into_subcluster(s, end_offset) == 0 ||
end_offset >= bs->total_sectors << BDRV_SECTOR_BITS);
 
 /* The zero flag is only supported by version 3 and newer */
@@ -2043,11 +2089,26 @@ int qcow2_cluster_zeroize(BlockDriverState *bs, 
uint64_t offset,
 return -ENOTSUP;
 }
 
-/* Each L2 slice is handled by its own loop iteration */
-nb_clusters = size_to_clusters(s, bytes);
+head = MIN(end_offset, ROUND_UP(offset, s->clus

[PATCH v6 02/32] qcow2: Convert qcow2_get_cluster_offset() into qcow2_get_host_offset()

[Alberto Garcia]

qcow2_get_cluster_offset() takes an (unaligned) guest offset and
returns the (aligned) offset of the corresponding cluster in the qcow2
image.

In practice none of the callers need to know where the cluster starts
so this patch makes the function calculate and return the final host
offset directly. The function is also renamed accordingly.

There is a pre-existing exception with compressed clusters: in this
case the function returns the complete cluster descriptor (containing
the offset and size of the compressed data). This does not change with
this patch but it is now documented.

Signed-off-by: Alberto Garcia 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.h |  4 ++--
 block/qcow2-cluster.c | 42 +++---
 block/qcow2.c | 24 +++-
 3 files changed, 32 insertions(+), 38 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 402e8acb1c..9d8530f1e8 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -694,8 +694,8 @@ int qcow2_write_l1_entry(BlockDriverState *bs, int 
l1_index);
 int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num,
   uint8_t *buf, int nb_sectors, bool enc, Error 
**errp);
 
-int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
- unsigned int *bytes, uint64_t *cluster_offset);
+int qcow2_get_host_offset(BlockDriverState *bs, uint64_t offset,
+  unsigned int *bytes, uint64_t *host_offset);
 int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
unsigned int *bytes, uint64_t *host_offset,
QCowL2Meta **m);
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 4b5fc8c4a7..9ab41cb728 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -496,10 +496,15 @@ static int coroutine_fn 
do_perform_cow_write(BlockDriverState *bs,
 
 
 /*
- * get_cluster_offset
+ * get_host_offset
  *
- * For a given offset of the virtual disk, find the cluster type and offset in
- * the qcow2 file. The offset is stored in *cluster_offset.
+ * For a given offset of the virtual disk find the equivalent host
+ * offset in the qcow2 file and store it in *host_offset. Neither
+ * offset needs to be aligned to a cluster boundary.
+ *
+ * If the cluster is unallocated then *host_offset will be 0.
+ * If the cluster is compressed then *host_offset will contain the
+ * complete compressed cluster descriptor.
  *
  * On entry, *bytes is the maximum number of contiguous bytes starting at
  * offset that we are interested in.
@@ -511,12 +516,12 @@ static int coroutine_fn 
do_perform_cow_write(BlockDriverState *bs,
  * Returns the cluster type (QCOW2_CLUSTER_*) on success, -errno in error
  * cases.
  */
-int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
- unsigned int *bytes, uint64_t *cluster_offset)
+int qcow2_get_host_offset(BlockDriverState *bs, uint64_t offset,
+  unsigned int *bytes, uint64_t *host_offset)
 {
 BDRVQcow2State *s = bs->opaque;
 unsigned int l2_index;
-uint64_t l1_index, l2_offset, *l2_slice;
+uint64_t l1_index, l2_offset, *l2_slice, l2_entry;
 int c;
 unsigned int offset_in_cluster;
 uint64_t bytes_available, bytes_needed, nb_clusters;
@@ -537,8 +542,6 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t 
offset,
 bytes_needed = bytes_available;
 }
 
-*cluster_offset = 0;
-
 /* seek to the l2 offset in the l1 table */
 
 l1_index = offset_to_l1_index(s, offset);
@@ -570,7 +573,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t 
offset,
 /* find the cluster offset for the given disk offset */
 
 l2_index = offset_to_l2_slice_index(s, offset);
-*cluster_offset = be64_to_cpu(l2_slice[l2_index]);
+l2_entry = be64_to_cpu(l2_slice[l2_index]);
 
 nb_clusters = size_to_clusters(s, bytes_needed);
 /* bytes_needed <= *bytes + offset_in_cluster, both of which are unsigned
@@ -578,7 +581,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t 
offset,
  * true */
 assert(nb_clusters <= INT_MAX);
 
-type = qcow2_get_cluster_type(bs, *cluster_offset);
+type = qcow2_get_cluster_type(bs, l2_entry);
 if (s->qcow_version < 3 && (type == QCOW2_CLUSTER_ZERO_PLAIN ||
 type == QCOW2_CLUSTER_ZERO_ALLOC)) {
 qcow2_signal_corruption(bs, true, -1, -1, "Zero cluster entry found"
@@ -599,42 +602,43 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, 
uint64_t offset,
 }
 /* Compressed clusters can only be processed one by one */
 c = 1;
-*cluster_offset &= L2E_COMPRESSED_OFFSET_SIZE_MASK;
+*host_offset = l2_entry & L2E_COMPRESSED_OFFSET_SIZE_MASK;
 break;
 case QCOW2_CLUSTER_ZERO_PLAIN:
 case QCOW2_CLUSTER_UNALLOCATED:
 /* how many empty clusters ? */
 

[PATCH v6 30/32] qcow2: Add the 'extended_l2' option and the QCOW2_INCOMPAT_EXTL2 bit

[Alberto Garcia]

Now that the implementation of subclusters is complete we can finally
add the necessary options to create and read images with this feature,
which we call "extended L2 entries".

Signed-off-by: Alberto Garcia 
---
 qapi/block-core.json |   7 +++
 block/qcow2.h|   8 ++-
 include/block/block_int.h|   1 +
 block/qcow2.c|  74 --
 tests/qemu-iotests/031.out   |   8 +--
 tests/qemu-iotests/036.out   |   4 +-
 tests/qemu-iotests/049.out   | 102 +++
 tests/qemu-iotests/060.out   |   1 +
 tests/qemu-iotests/061.out   |  20 +++---
 tests/qemu-iotests/065   |  12 ++--
 tests/qemu-iotests/082.out   |  48 ---
 tests/qemu-iotests/085.out   |  38 ++--
 tests/qemu-iotests/144.out   |   4 +-
 tests/qemu-iotests/182.out   |   2 +-
 tests/qemu-iotests/185.out   |   8 +--
 tests/qemu-iotests/198.out   |   2 +
 tests/qemu-iotests/206.out   |   4 ++
 tests/qemu-iotests/242.out   |   5 ++
 tests/qemu-iotests/255.out   |   8 +--
 tests/qemu-iotests/274.out   |  49 ---
 tests/qemu-iotests/280.out   |   2 +-
 tests/qemu-iotests/common.filter |   1 +
 22 files changed, 268 insertions(+), 140 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6fbacddab2..0cb80bda9b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -66,6 +66,9 @@
 # standalone (read-only) raw image without looking at qcow2
 # metadata (since: 4.0)
 #
+# @extended-l2: true if the image has extended L2 entries; only valid for
+#   compat >= 1.1 (since 5.1)
+#
 # @lazy-refcounts: on or off; only valid for compat >= 1.1
 #
 # @corrupt: true if the image has been marked corrupt; only valid for
@@ -87,6 +90,7 @@
   'compat': 'str',
   '*data-file': 'str',
   '*data-file-raw': 'bool',
+  '*extended-l2': 'bool',
   '*lazy-refcounts': 'bool',
   '*corrupt': 'bool',
   'refcount-bits': 'int',
@@ -4312,6 +4316,8 @@
 # @data-file-raw: True if the external data file must stay valid as a
 # standalone (read-only) raw image without looking at qcow2
 # metadata (default: false; since: 4.0)
+# @extended-l2  True to make the image have extended L2 entries
+#   (default: false; since 5.1)
 # @size: Size of the virtual disk in bytes
 # @version: Compatibility level (default: v3)
 # @backing-file: File name of the backing file if a backing file
@@ -4332,6 +4338,7 @@
   'data': { 'file': 'BlockdevRef',
 '*data-file':   'BlockdevRef',
 '*data-file-raw':   'bool',
+'*extended-l2': 'bool',
 'size': 'size',
 '*version': 'BlockdevQcow2Version',
 '*backing-file':'str',
diff --git a/block/qcow2.h b/block/qcow2.h
index ece5f1cb5a..0f26e39d12 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -246,15 +246,18 @@ enum {
 QCOW2_INCOMPAT_CORRUPT_BITNR= 1,
 QCOW2_INCOMPAT_DATA_FILE_BITNR  = 2,
 QCOW2_INCOMPAT_COMPRESSION_BITNR = 3,
+QCOW2_INCOMPAT_EXTL2_BITNR  = 4,
 QCOW2_INCOMPAT_DIRTY= 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
 QCOW2_INCOMPAT_CORRUPT  = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
 QCOW2_INCOMPAT_DATA_FILE= 1 << QCOW2_INCOMPAT_DATA_FILE_BITNR,
 QCOW2_INCOMPAT_COMPRESSION  = 1 << QCOW2_INCOMPAT_COMPRESSION_BITNR,
+QCOW2_INCOMPAT_EXTL2= 1 << QCOW2_INCOMPAT_EXTL2_BITNR,
 
 QCOW2_INCOMPAT_MASK = QCOW2_INCOMPAT_DIRTY
 | QCOW2_INCOMPAT_CORRUPT
 | QCOW2_INCOMPAT_DATA_FILE
-| QCOW2_INCOMPAT_COMPRESSION,
+| QCOW2_INCOMPAT_COMPRESSION
+| QCOW2_INCOMPAT_EXTL2,
 };
 
 /* Compatible feature bits */
@@ -573,8 +576,7 @@ typedef enum QCow2MetadataOverlap {
 
 static inline bool has_subclusters(BDRVQcow2State *s)
 {
-/* FIXME: Return false until this feature is complete */
-return false;
+return s->incompatible_features & QCOW2_INCOMPAT_EXTL2;
 }
 
 static inline size_t l2_entry_size(BDRVQcow2State *s)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 791de6a59c..36e1993788 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -58,6 +58,7 @@
 #define BLOCK_OPT_DATA_FILE "data_file"
 #define BLOCK_OPT_DATA_FILE_RAW "data_file_raw"
 #define BLOCK_OPT_COMPRESSION_TYPE  "compression_type"
+#define BLOCK_OPT_EXTL2 "extended_l2"
 
 #define BLOCK_PROBE_BUF_SIZE512
 
diff --git a/block/qcow2.c b/block/qcow2.c
index bc7a4a1729..6690b39510 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1438,6 +1438,12 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,

[PATCH v6 00/32] Add subcluster allocation to qcow2

[Alberto Garcia]

Hi,

here's the new version of the patches to add subcluster allocation
support to qcow2.

Please refer to the cover letter of the first version for a full
description of the patches:

   https://lists.gnu.org/archive/html/qemu-block/2019-10/msg00983.html

The big change here is that the code does not need to iterate over
every subcluster anymore. There's now a new function called
qcow2_get_subcluster_range_type() that checks a group of subclusters
in one go using cto and ctz operations.

Berto

v6:
- Patch 07: Fix indentation issues in the documentation [Eric]
- Patch 14: Use half-open intervals for the subcluster macros.
New method to detected invalid subclusters.
- Patch 15: New patch
- Patch 20: Use a faster method to check the status of a groups of
subclusters without having to iterate over each one of
them.
- Patch 21: Optimize count_contiguous_subclusters() by removing the
iteration over every subcluster.
Earlier detection of invalid subclusters.
Minor documentation updates [Eric]
- Patch 25: Update after the macro changes in patch 14
- Patch 28: Update after the macro changes in patch 14
- Patch 29: Change patch order so this goes before patch 30
- Patch 30: Fix rebase conflicts now that compression_type has been
merged.
Disable toggling extended_l2 using amend [Eric]
- Patch 32: Many new tests and refactoring

v5: https://lists.gnu.org/archive/html/qemu-block/2020-05/msg00251.html
v4: https://lists.gnu.org/archive/html/qemu-block/2020-03/msg00966.html
v3: https://lists.gnu.org/archive/html/qemu-block/2019-12/msg00587.html
v2: https://lists.gnu.org/archive/html/qemu-block/2019-10/msg01642.html
v1: https://lists.gnu.org/archive/html/qemu-block/2019-10/msg00983.html

Output of git backport-diff against v5:

Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/32:[] [--] 'qcow2: Make Qcow2AioTask store the full host offset'
002/32:[] [--] 'qcow2: Convert qcow2_get_cluster_offset() into 
qcow2_get_host_offset()'
003/32:[] [--] 'qcow2: Add calculate_l2_meta()'
004/32:[] [--] 'qcow2: Split cluster_needs_cow() out of 
count_cow_clusters()'
005/32:[] [--] 'qcow2: Process QCOW2_CLUSTER_ZERO_ALLOC clusters in 
handle_copied()'
006/32:[] [--] 'qcow2: Add get_l2_entry() and set_l2_entry()'
007/32:[0006] [FC] 'qcow2: Document the Extended L2 Entries feature'
008/32:[] [--] 'qcow2: Add dummy has_subclusters() function'
009/32:[] [--] 'qcow2: Add subcluster-related fields to BDRVQcow2State'
010/32:[] [--] 'qcow2: Add offset_to_sc_index()'
011/32:[] [--] 'qcow2: Add offset_into_subcluster() and 
size_to_subclusters()'
012/32:[] [--] 'qcow2: Add l2_entry_size()'
013/32:[] [--] 'qcow2: Update get/set_l2_entry() and add 
get/set_l2_bitmap()'
014/32:[0039] [FC] 'qcow2: Add QCow2SubclusterType and 
qcow2_get_subcluster_type()'
015/32:[down] 'qcow2: Add qcow2_get_subcluster_range_type()'
016/32:[] [--] 'qcow2: Add qcow2_cluster_is_allocated()'
017/32:[] [--] 'qcow2: Add cluster type parameter to 
qcow2_get_host_offset()'
018/32:[] [--] 'qcow2: Replace QCOW2_CLUSTER_* with QCOW2_SUBCLUSTER_*'
019/32:[] [--] 'qcow2: Handle QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC'
020/32:[0111] [FC] 'qcow2: Add subcluster support to calculate_l2_meta()'
021/32:[0091] [FC] 'qcow2: Add subcluster support to qcow2_get_host_offset()'
022/32:[] [--] 'qcow2: Add subcluster support to zero_in_l2_slice()'
023/32:[] [--] 'qcow2: Add subcluster support to discard_in_l2_slice()'
024/32:[] [--] 'qcow2: Add subcluster support to check_refcounts_l2()'
025/32:[0004] [FC] 'qcow2: Update L2 bitmap in qcow2_alloc_cluster_link_l2()'
026/32:[] [--] 'qcow2: Clear the L2 bitmap when allocating a compressed 
cluster'
027/32:[] [--] 'qcow2: Add subcluster support to handle_alloc_space()'
028/32:[0004] [FC] 'qcow2: Add subcluster support to qcow2_co_pwrite_zeroes()'
029/32:[0001] [FC] 'qcow2: Add subcluster support to qcow2_measure()'
030/32:[0549] [FC] 'qcow2: Add the 'extended_l2' option and the 
QCOW2_INCOMPAT_EXTL2 bit'
031/32:[] [--] 'qcow2: Assert that expand_zero_clusters_in_l1() does not 
support subclusters'
032/32:[0775] [FC] 'iotests: Add tests for qcow2 images with extended L2 
entries'

Alberto Garcia (32):
  qcow2: Make Qcow2AioTask store the full host offset
  qcow2: Convert qcow2_get_cluster_offset() into qcow2_get_host_offset()
  qcow2: Add calculate_l2_meta()
  qcow2: Split cluster_needs_cow() out of count_cow_clusters()
  qcow2: Process QCOW2_CLUSTER_ZERO_ALLOC clusters in handle_copied()
  qcow2: Add get_l2_entry() and set_l2_entry()
  qcow2: Document the Extended L2 Entries feature
  qcow2: Add dummy has_subclusters() function
  qcow2: Add subcluster-related fields to BDRVQcow2State
  qcow2: A

[PATCH v6 09/32] qcow2: Add subcluster-related fields to BDRVQcow2State

[Alberto Garcia]

This patch adds the following new fields to BDRVQcow2State:

- subclusters_per_cluster: Number of subclusters in a cluster
- subcluster_size: The size of each subcluster, in bytes
- subcluster_bits: No. of bits so 1 << subcluster_bits = subcluster_size

Images without subclusters are treated as if they had exactly one
subcluster per cluster (i.e. subcluster_size = cluster_size).

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.h | 5 +
 block/qcow2.c | 5 +
 2 files changed, 10 insertions(+)

diff --git a/block/qcow2.h b/block/qcow2.h
index f41bfd743f..4e9c732831 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -78,6 +78,8 @@
 /* The cluster reads as all zeros */
 #define QCOW_OFLAG_ZERO (1ULL << 0)
 
+#define QCOW_EXTL2_SUBCLUSTERS_PER_CLUSTER 32
+
 #define MIN_CLUSTER_BITS 9
 #define MAX_CLUSTER_BITS 21
 
@@ -295,6 +297,9 @@ typedef struct BDRVQcow2State {
 int cluster_bits;
 int cluster_size;
 int l2_slice_size;
+int subcluster_bits;
+int subcluster_size;
+int subclusters_per_cluster;
 int l2_bits;
 int l2_size;
 int l1_size;
diff --git a/block/qcow2.c b/block/qcow2.c
index 6562eb1590..916b6c3f92 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1433,6 +1433,11 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
 }
 }
 
+s->subclusters_per_cluster =
+has_subclusters(s) ? QCOW_EXTL2_SUBCLUSTERS_PER_CLUSTER : 1;
+s->subcluster_size = s->cluster_size / s->subclusters_per_cluster;
+s->subcluster_bits = ctz32(s->subcluster_size);
+
 /* Check support for various header values */
 if (header.refcount_order > 6) {
 error_setg(errp, "Reference count entry width too large; may not "
-- 
2.20.1

[PATCH v6 26/32] qcow2: Clear the L2 bitmap when allocating a compressed cluster

[Alberto Garcia]

Compressed clusters always have the bitmap part of the extended L2
entry set to 0.

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
---
 block/qcow2-cluster.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index c8d6e16237..3639dc8057 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -861,6 +861,9 @@ int qcow2_alloc_compressed_cluster_offset(BlockDriverState 
*bs,
 BLKDBG_EVENT(bs->file, BLKDBG_L2_UPDATE_COMPRESSED);
 qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice);
 set_l2_entry(s, l2_slice, l2_index, cluster_offset);
+if (has_subclusters(s)) {
+set_l2_bitmap(s, l2_slice, l2_index, 0);
+}
 qcow2_cache_put(s->l2_table_cache, (void **) &l2_slice);
 
 *host_offset = cluster_offset & s->cluster_offset_mask;
-- 
2.20.1

[PATCH v6 06/32] qcow2: Add get_l2_entry() and set_l2_entry()

[Alberto Garcia]

The size of an L2 entry is 64 bits, but if we want to have subclusters
we need extended L2 entries. This means that we have to access L2
tables and slices differently depending on whether an image has
extended L2 entries or not.

This patch replaces all l2_slice[] accesses with calls to
get_l2_entry() and set_l2_entry().

Signed-off-by: Alberto Garcia 
Reviewed-by: Eric Blake 
Reviewed-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.h  | 12 
 block/qcow2-cluster.c  | 63 ++
 block/qcow2-refcount.c | 17 ++--
 3 files changed, 54 insertions(+), 38 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 9d8530f1e8..afee84f41f 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -510,6 +510,18 @@ typedef enum QCow2MetadataOverlap {
 
 #define INV_OFFSET (-1ULL)
 
+static inline uint64_t get_l2_entry(BDRVQcow2State *s, uint64_t *l2_slice,
+int idx)
+{
+return be64_to_cpu(l2_slice[idx]);
+}
+
+static inline void set_l2_entry(BDRVQcow2State *s, uint64_t *l2_slice,
+int idx, uint64_t entry)
+{
+l2_slice[idx] = cpu_to_be64(entry);
+}
+
 static inline bool has_data_file(BlockDriverState *bs)
 {
 BDRVQcow2State *s = bs->opaque;
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index fce0be7a08..76fd0f3cdb 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -383,12 +383,13 @@ fail:
  * cluster which may require a different handling)
  */
 static int count_contiguous_clusters(BlockDriverState *bs, int nb_clusters,
-int cluster_size, uint64_t *l2_slice, uint64_t stop_flags)
+int cluster_size, uint64_t *l2_slice, int l2_index, uint64_t 
stop_flags)
 {
+BDRVQcow2State *s = bs->opaque;
 int i;
 QCow2ClusterType first_cluster_type;
 uint64_t mask = stop_flags | L2E_OFFSET_MASK | QCOW_OFLAG_COMPRESSED;
-uint64_t first_entry = be64_to_cpu(l2_slice[0]);
+uint64_t first_entry = get_l2_entry(s, l2_slice, l2_index);
 uint64_t offset = first_entry & mask;
 
 first_cluster_type = qcow2_get_cluster_type(bs, first_entry);
@@ -401,7 +402,7 @@ static int count_contiguous_clusters(BlockDriverState *bs, 
int nb_clusters,
first_cluster_type == QCOW2_CLUSTER_ZERO_ALLOC);
 
 for (i = 0; i < nb_clusters; i++) {
-uint64_t l2_entry = be64_to_cpu(l2_slice[i]) & mask;
+uint64_t l2_entry = get_l2_entry(s, l2_slice, l2_index + i) & mask;
 if (offset + (uint64_t) i * cluster_size != l2_entry) {
 break;
 }
@@ -417,14 +418,16 @@ static int count_contiguous_clusters(BlockDriverState 
*bs, int nb_clusters,
 static int count_contiguous_clusters_unallocated(BlockDriverState *bs,
  int nb_clusters,
  uint64_t *l2_slice,
+ int l2_index,
  QCow2ClusterType wanted_type)
 {
+BDRVQcow2State *s = bs->opaque;
 int i;
 
 assert(wanted_type == QCOW2_CLUSTER_ZERO_PLAIN ||
wanted_type == QCOW2_CLUSTER_UNALLOCATED);
 for (i = 0; i < nb_clusters; i++) {
-uint64_t entry = be64_to_cpu(l2_slice[i]);
+uint64_t entry = get_l2_entry(s, l2_slice, l2_index + i);
 QCow2ClusterType type = qcow2_get_cluster_type(bs, entry);
 
 if (type != wanted_type) {
@@ -573,7 +576,7 @@ int qcow2_get_host_offset(BlockDriverState *bs, uint64_t 
offset,
 /* find the cluster offset for the given disk offset */
 
 l2_index = offset_to_l2_slice_index(s, offset);
-l2_entry = be64_to_cpu(l2_slice[l2_index]);
+l2_entry = get_l2_entry(s, l2_slice, l2_index);
 
 nb_clusters = size_to_clusters(s, bytes_needed);
 /* bytes_needed <= *bytes + offset_in_cluster, both of which are unsigned
@@ -608,7 +611,7 @@ int qcow2_get_host_offset(BlockDriverState *bs, uint64_t 
offset,
 case QCOW2_CLUSTER_UNALLOCATED:
 /* how many empty clusters ? */
 c = count_contiguous_clusters_unallocated(bs, nb_clusters,
-  &l2_slice[l2_index], type);
+  l2_slice, l2_index, type);
 *host_offset = 0;
 break;
 case QCOW2_CLUSTER_ZERO_ALLOC:
@@ -617,7 +620,7 @@ int qcow2_get_host_offset(BlockDriverState *bs, uint64_t 
offset,
 *host_offset = host_cluster_offset + offset_in_cluster;
 /* how many allocated clusters ? */
 c = count_contiguous_clusters(bs, nb_clusters, s->cluster_size,
-  &l2_slice[l2_index], QCOW_OFLAG_ZERO);
+  l2_slice, l2_index, QCOW_OFLAG_ZERO);
 if (offset_into_cluster(s, host_cluster_offset)) {
 qcow2_signal_corruption(bs, true, -1, -1,
 "Cluster allocation offse

[PATCH v6 01/32] qcow2: Make Qcow2AioTask store the full host offset

[Alberto Garcia]

The file_cluster_offset field of Qcow2AioTask stores a cluster-aligned
host offset. In practice this is not very useful because all users(*)
of this structure need the final host offset into the cluster, which
they calculate using

   host_offset = file_cluster_offset + offset_into_cluster(s, offset)

There is no reason why Qcow2AioTask cannot store host_offset directly
and that is what this patch does.

(*) compressed clusters are the exception: in this case what
file_cluster_offset was storing was the full compressed cluster
descriptor (offset + size). This does not change with this patch
but it is documented now.

Signed-off-by: Alberto Garcia 
Reviewed-by: Eric Blake 
Reviewed-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.c  | 69 ++
 block/trace-events |  2 +-
 2 files changed, 34 insertions(+), 37 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index dfab8d2f6c..4815dc0931 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -74,7 +74,7 @@ typedef struct {
 
 static int coroutine_fn
 qcow2_co_preadv_compressed(BlockDriverState *bs,
-   uint64_t file_cluster_offset,
+   uint64_t cluster_descriptor,
uint64_t offset,
uint64_t bytes,
QEMUIOVector *qiov,
@@ -2103,7 +2103,7 @@ out:
 
 static coroutine_fn int
 qcow2_co_preadv_encrypted(BlockDriverState *bs,
-   uint64_t file_cluster_offset,
+   uint64_t host_offset,
uint64_t offset,
uint64_t bytes,
QEMUIOVector *qiov,
@@ -2130,16 +2130,12 @@ qcow2_co_preadv_encrypted(BlockDriverState *bs,
 }
 
 BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
-ret = bdrv_co_pread(s->data_file,
-file_cluster_offset + offset_into_cluster(s, offset),
-bytes, buf, 0);
+ret = bdrv_co_pread(s->data_file, host_offset, bytes, buf, 0);
 if (ret < 0) {
 goto fail;
 }
 
-if (qcow2_co_decrypt(bs,
- file_cluster_offset + offset_into_cluster(s, offset),
- offset, buf, bytes) < 0)
+if (qcow2_co_decrypt(bs, host_offset, offset, buf, bytes) < 0)
 {
 ret = -EIO;
 goto fail;
@@ -2157,7 +2153,7 @@ typedef struct Qcow2AioTask {
 
 BlockDriverState *bs;
 QCow2ClusterType cluster_type; /* only for read */
-uint64_t file_cluster_offset;
+uint64_t host_offset; /* or full descriptor in compressed clusters */
 uint64_t offset;
 uint64_t bytes;
 QEMUIOVector *qiov;
@@ -2170,7 +2166,7 @@ static coroutine_fn int qcow2_add_task(BlockDriverState 
*bs,
AioTaskPool *pool,
AioTaskFunc func,
QCow2ClusterType cluster_type,
-   uint64_t file_cluster_offset,
+   uint64_t host_offset,
uint64_t offset,
uint64_t bytes,
QEMUIOVector *qiov,
@@ -2185,7 +2181,7 @@ static coroutine_fn int qcow2_add_task(BlockDriverState 
*bs,
 .bs = bs,
 .cluster_type = cluster_type,
 .qiov = qiov,
-.file_cluster_offset = file_cluster_offset,
+.host_offset = host_offset,
 .offset = offset,
 .bytes = bytes,
 .qiov_offset = qiov_offset,
@@ -2194,7 +2190,7 @@ static coroutine_fn int qcow2_add_task(BlockDriverState 
*bs,
 
 trace_qcow2_add_task(qemu_coroutine_self(), bs, pool,
  func == qcow2_co_preadv_task_entry ? "read" : "write",
- cluster_type, file_cluster_offset, offset, bytes,
+ cluster_type, host_offset, offset, bytes,
  qiov, qiov_offset);
 
 if (!pool) {
@@ -2208,13 +2204,12 @@ static coroutine_fn int qcow2_add_task(BlockDriverState 
*bs,
 
 static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
  QCow2ClusterType cluster_type,
- uint64_t file_cluster_offset,
+ uint64_t host_offset,
  uint64_t offset, uint64_t bytes,
  QEMUIOVector *qiov,
  size_t qiov_offset)
 {
 BDRVQcow2State *s = bs->opaque;
-int offset_in_cluster = offset_into_cluster(s, offset);
 
 switch (cluster_type) {
 case QCOW2_CLUSTER_ZERO_PLAIN:
@@ -2230,19 +2225,17 @@ static coroutine_fn int 
qcow2_co_preadv_task(BlockDriverState *bs,
qiov, qiov_offs

[PATCH v6 16/32] qcow2: Add qcow2_cluster_is_allocated()

[Alberto Garcia]

This helper function tells us if a cluster is allocated (that is,
there is an associated host offset for it).

Signed-off-by: Alberto Garcia 
Reviewed-by: Eric Blake 
---
 block/qcow2.h | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/block/qcow2.h b/block/qcow2.h
index 27dbcbc502..ba7614e406 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -780,6 +780,12 @@ QCow2SubclusterType 
qcow2_get_subcluster_type(BlockDriverState *bs,
 }
 }
 
+static inline bool qcow2_cluster_is_allocated(QCow2ClusterType type)
+{
+return (type == QCOW2_CLUSTER_COMPRESSED || type == QCOW2_CLUSTER_NORMAL ||
+type == QCOW2_CLUSTER_ZERO_ALLOC);
+}
+
 /* Check whether refcounts are eager or lazy */
 static inline bool qcow2_need_accurate_refcounts(BDRVQcow2State *s)
 {
-- 
2.20.1

[PATCH v6 12/32] qcow2: Add l2_entry_size()

[Alberto Garcia]

qcow2 images with subclusters have 128-bit L2 entries. The first 64
bits contain the same information as traditional images and the last
64 bits form a bitmap with the status of each individual subcluster.

Because of that we cannot assume that L2 entries are sizeof(uint64_t)
anymore. This function returns the proper value for the image.

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
---
 block/qcow2.h  |  9 +
 block/qcow2-cluster.c  | 12 ++--
 block/qcow2-refcount.c | 14 --
 block/qcow2.c  |  8 
 4 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 79c4f82383..95cc16a089 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -80,6 +80,10 @@
 
 #define QCOW_EXTL2_SUBCLUSTERS_PER_CLUSTER 32
 
+/* Size of normal and extended L2 entries */
+#define L2E_SIZE_NORMAL   (sizeof(uint64_t))
+#define L2E_SIZE_EXTENDED (sizeof(uint64_t) * 2)
+
 #define MIN_CLUSTER_BITS 9
 #define MAX_CLUSTER_BITS 21
 
@@ -521,6 +525,11 @@ static inline bool has_subclusters(BDRVQcow2State *s)
 return false;
 }
 
+static inline size_t l2_entry_size(BDRVQcow2State *s)
+{
+return has_subclusters(s) ? L2E_SIZE_EXTENDED : L2E_SIZE_NORMAL;
+}
+
 static inline uint64_t get_l2_entry(BDRVQcow2State *s, uint64_t *l2_slice,
 int idx)
 {
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 76fd0f3cdb..8b2fc550b7 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -208,7 +208,7 @@ static int l2_load(BlockDriverState *bs, uint64_t offset,
uint64_t l2_offset, uint64_t **l2_slice)
 {
 BDRVQcow2State *s = bs->opaque;
-int start_of_slice = sizeof(uint64_t) *
+int start_of_slice = l2_entry_size(s) *
 (offset_to_l2_index(s, offset) - offset_to_l2_slice_index(s, offset));
 
 return qcow2_cache_get(bs, s->l2_table_cache, l2_offset + start_of_slice,
@@ -281,7 +281,7 @@ static int l2_allocate(BlockDriverState *bs, int l1_index)
 
 /* allocate a new l2 entry */
 
-l2_offset = qcow2_alloc_clusters(bs, s->l2_size * sizeof(uint64_t));
+l2_offset = qcow2_alloc_clusters(bs, s->l2_size * l2_entry_size(s));
 if (l2_offset < 0) {
 ret = l2_offset;
 goto fail;
@@ -305,7 +305,7 @@ static int l2_allocate(BlockDriverState *bs, int l1_index)
 
 /* allocate a new entry in the l2 cache */
 
-slice_size2 = s->l2_slice_size * sizeof(uint64_t);
+slice_size2 = s->l2_slice_size * l2_entry_size(s);
 n_slices = s->cluster_size / slice_size2;
 
 trace_qcow2_l2_allocate_get_empty(bs, l1_index);
@@ -369,7 +369,7 @@ fail:
 }
 s->l1_table[l1_index] = old_l2_offset;
 if (l2_offset > 0) {
-qcow2_free_clusters(bs, l2_offset, s->l2_size * sizeof(uint64_t),
+qcow2_free_clusters(bs, l2_offset, s->l2_size * l2_entry_size(s),
 QCOW2_DISCARD_ALWAYS);
 }
 return ret;
@@ -716,7 +716,7 @@ static int get_cluster_table(BlockDriverState *bs, uint64_t 
offset,
 
 /* Then decrease the refcount of the old table */
 if (l2_offset) {
-qcow2_free_clusters(bs, l2_offset, s->l2_size * sizeof(uint64_t),
+qcow2_free_clusters(bs, l2_offset, s->l2_size * l2_entry_size(s),
 QCOW2_DISCARD_OTHER);
 }
 
@@ -1913,7 +1913,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState 
*bs, uint64_t *l1_table,
 int ret;
 int i, j;
 
-slice_size2 = s->l2_slice_size * sizeof(uint64_t);
+slice_size2 = s->l2_slice_size * l2_entry_size(s);
 n_slices = s->cluster_size / slice_size2;
 
 if (!is_active_l1) {
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 04546838e8..770c5dbc83 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1254,7 +1254,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
 l2_slice = NULL;
 l1_table = NULL;
 l1_size2 = l1_size * sizeof(uint64_t);
-slice_size2 = s->l2_slice_size * sizeof(uint64_t);
+slice_size2 = s->l2_slice_size * l2_entry_size(s);
 n_slices = s->cluster_size / slice_size2;
 
 s->cache_discards = true;
@@ -1605,7 +1605,7 @@ static int check_refcounts_l2(BlockDriverState *bs, 
BdrvCheckResult *res,
 int i, l2_size, nb_csectors, ret;
 
 /* Read L2 table from disk */
-l2_size = s->l2_size * sizeof(uint64_t);
+l2_size = s->l2_size * l2_entry_size(s);
 l2_table = g_malloc(l2_size);
 
 ret = bdrv_pread(bs->file, l2_offset, l2_table, l2_size);
@@ -1680,15 +1680,16 @@ static int check_refcounts_l2(BlockDriverState *bs, 
BdrvCheckResult *res,
 fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR",
 offset);
 if (fix & BDRV_FIX_ERRORS) {
+int idx = i * (l2_entry_size(s) / sizeof(uint64_t));
 uint64_t l2e_offset =
-l2_offset + (uint64_t

[PATCH v6 05/32] qcow2: Process QCOW2_CLUSTER_ZERO_ALLOC clusters in handle_copied()

[Alberto Garcia]

When writing to a qcow2 file there are two functions that take a
virtual offset and return a host offset, possibly allocating new
clusters if necessary:

   - handle_copied() looks for normal data clusters that are already
 allocated and have a reference count of 1. In those clusters we
 can simply write the data and there is no need to perform any
 copy-on-write.

   - handle_alloc() looks for clusters that do need copy-on-write,
 either because they haven't been allocated yet, because their
 reference count is != 1 or because they are ZERO_ALLOC clusters.

The ZERO_ALLOC case is a bit special because those are clusters that
are already allocated and they could perfectly be dealt with in
handle_copied() (as long as copy-on-write is performed when required).

In fact, there is extra code specifically for them in handle_alloc()
that tries to reuse the existing allocation if possible and frees them
otherwise.

This patch changes the handling of ZERO_ALLOC clusters so the
semantics of these two functions are now like this:

   - handle_copied() looks for clusters that are already allocated and
 which we can overwrite (NORMAL and ZERO_ALLOC clusters with a
 reference count of 1).

   - handle_alloc() looks for clusters for which we need a new
 allocation (all other cases).

One important difference after this change is that clusters found
in handle_copied() may now require copy-on-write, but this will be
necessary anyway once we add support for subclusters.

Signed-off-by: Alberto Garcia 
Reviewed-by: Eric Blake 
---
 block/qcow2-cluster.c | 256 +++---
 1 file changed, 141 insertions(+), 115 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 80f9787461..fce0be7a08 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1039,13 +1039,18 @@ void qcow2_alloc_cluster_abort(BlockDriverState *bs, 
QCowL2Meta *m)
 
 /*
  * For a given write request, create a new QCowL2Meta structure, add
- * it to @m and the BDRVQcow2State.cluster_allocs list.
+ * it to @m and the BDRVQcow2State.cluster_allocs list. If the write
+ * request does not need copy-on-write or changes to the L2 metadata
+ * then this function does nothing.
  *
  * @host_cluster_offset points to the beginning of the first cluster.
  *
  * @guest_offset and @bytes indicate the offset and length of the
  * request.
  *
+ * @l2_slice contains the L2 entries of all clusters involved in this
+ * write request.
+ *
  * If @keep_old is true it means that the clusters were already
  * allocated and will be overwritten. If false then the clusters are
  * new and we have to decrease the reference count of the old ones.
@@ -1053,15 +1058,53 @@ void qcow2_alloc_cluster_abort(BlockDriverState *bs, 
QCowL2Meta *m)
 static void calculate_l2_meta(BlockDriverState *bs,
   uint64_t host_cluster_offset,
   uint64_t guest_offset, unsigned bytes,
-  QCowL2Meta **m, bool keep_old)
+  uint64_t *l2_slice, QCowL2Meta **m, bool 
keep_old)
 {
 BDRVQcow2State *s = bs->opaque;
-unsigned cow_start_from = 0;
+int l2_index = offset_to_l2_slice_index(s, guest_offset);
+uint64_t l2_entry;
+unsigned cow_start_from, cow_end_to;
 unsigned cow_start_to = offset_into_cluster(s, guest_offset);
 unsigned cow_end_from = cow_start_to + bytes;
-unsigned cow_end_to = ROUND_UP(cow_end_from, s->cluster_size);
 unsigned nb_clusters = size_to_clusters(s, cow_end_from);
 QCowL2Meta *old_m = *m;
+QCow2ClusterType type;
+
+assert(nb_clusters <= s->l2_slice_size - l2_index);
+
+/* Return if there's no COW (all clusters are normal and we keep them) */
+if (keep_old) {
+int i;
+for (i = 0; i < nb_clusters; i++) {
+l2_entry = be64_to_cpu(l2_slice[l2_index + i]);
+if (qcow2_get_cluster_type(bs, l2_entry) != QCOW2_CLUSTER_NORMAL) {
+break;
+}
+}
+if (i == nb_clusters) {
+return;
+}
+}
+
+/* Get the L2 entry of the first cluster */
+l2_entry = be64_to_cpu(l2_slice[l2_index]);
+type = qcow2_get_cluster_type(bs, l2_entry);
+
+if (type == QCOW2_CLUSTER_NORMAL && keep_old) {
+cow_start_from = cow_start_to;
+} else {
+cow_start_from = 0;
+}
+
+/* Get the L2 entry of the last cluster */
+l2_entry = be64_to_cpu(l2_slice[l2_index + nb_clusters - 1]);
+type = qcow2_get_cluster_type(bs, l2_entry);
+
+if (type == QCOW2_CLUSTER_NORMAL && keep_old) {
+cow_end_to = cow_end_from;
+} else {
+cow_end_to = ROUND_UP(cow_end_from, s->cluster_size);
+}
 
 *m = g_malloc0(sizeof(**m));
 **m = (QCowL2Meta) {
@@ -1087,18 +1130,22 @@ static void calculate_l2_meta(BlockDriverState *bs,
 QLIST_INSERT_HEAD(&s->cluster_allocs, *m, next_in_flight);
 }
 
-/* Returns true if writin

[PATCH v6 10/32] qcow2: Add offset_to_sc_index()

[Alberto Garcia]

For a given offset, return the subcluster number within its cluster
(i.e. with 32 subclusters per cluster it returns a number between 0
and 31).

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.h | 5 +
 1 file changed, 5 insertions(+)

diff --git a/block/qcow2.h b/block/qcow2.h
index 4e9c732831..ca73ac9b67 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -581,6 +581,11 @@ static inline int offset_to_l2_slice_index(BDRVQcow2State 
*s, int64_t offset)
 return (offset >> s->cluster_bits) & (s->l2_slice_size - 1);
 }
 
+static inline int offset_to_sc_index(BDRVQcow2State *s, int64_t offset)
+{
+return (offset >> s->subcluster_bits) & (s->subclusters_per_cluster - 1);
+}
+
 static inline int64_t qcow2_vm_state_offset(BDRVQcow2State *s)
 {
 return (int64_t)s->l1_vm_state_index << (s->cluster_bits + s->l2_bits);
-- 
2.20.1

[PATCH v6 03/32] qcow2: Add calculate_l2_meta()

[Alberto Garcia]

handle_alloc() creates a QCowL2Meta structure in order to update the
image metadata and perform the necessary copy-on-write operations.

This patch moves that code to a separate function so it can be used
from other places.

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
---
 block/qcow2-cluster.c | 77 +--
 1 file changed, 53 insertions(+), 24 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 9ab41cb728..61ad638bdc 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1037,6 +1037,56 @@ void qcow2_alloc_cluster_abort(BlockDriverState *bs, 
QCowL2Meta *m)
 }
 }
 
+/*
+ * For a given write request, create a new QCowL2Meta structure, add
+ * it to @m and the BDRVQcow2State.cluster_allocs list.
+ *
+ * @host_cluster_offset points to the beginning of the first cluster.
+ *
+ * @guest_offset and @bytes indicate the offset and length of the
+ * request.
+ *
+ * If @keep_old is true it means that the clusters were already
+ * allocated and will be overwritten. If false then the clusters are
+ * new and we have to decrease the reference count of the old ones.
+ */
+static void calculate_l2_meta(BlockDriverState *bs,
+  uint64_t host_cluster_offset,
+  uint64_t guest_offset, unsigned bytes,
+  QCowL2Meta **m, bool keep_old)
+{
+BDRVQcow2State *s = bs->opaque;
+unsigned cow_start_from = 0;
+unsigned cow_start_to = offset_into_cluster(s, guest_offset);
+unsigned cow_end_from = cow_start_to + bytes;
+unsigned cow_end_to = ROUND_UP(cow_end_from, s->cluster_size);
+unsigned nb_clusters = size_to_clusters(s, cow_end_from);
+QCowL2Meta *old_m = *m;
+
+*m = g_malloc0(sizeof(**m));
+**m = (QCowL2Meta) {
+.next   = old_m,
+
+.alloc_offset   = host_cluster_offset,
+.offset = start_of_cluster(s, guest_offset),
+.nb_clusters= nb_clusters,
+
+.keep_old_clusters = keep_old,
+
+.cow_start = {
+.offset = cow_start_from,
+.nb_bytes   = cow_start_to - cow_start_from,
+},
+.cow_end = {
+.offset = cow_end_from,
+.nb_bytes   = cow_end_to - cow_end_from,
+},
+};
+
+qemu_co_queue_init(&(*m)->dependent_requests);
+QLIST_INSERT_HEAD(&s->cluster_allocs, *m, next_in_flight);
+}
+
 /*
  * Returns the number of contiguous clusters that can be used for an allocating
  * write, but require COW to be performed (this includes yet unallocated space,
@@ -1435,35 +1485,14 @@ static int handle_alloc(BlockDriverState *bs, uint64_t 
guest_offset,
 uint64_t requested_bytes = *bytes + offset_into_cluster(s, guest_offset);
 int avail_bytes = nb_clusters << s->cluster_bits;
 int nb_bytes = MIN(requested_bytes, avail_bytes);
-QCowL2Meta *old_m = *m;
-
-*m = g_malloc0(sizeof(**m));
-
-**m = (QCowL2Meta) {
-.next   = old_m,
-
-.alloc_offset   = alloc_cluster_offset,
-.offset = start_of_cluster(s, guest_offset),
-.nb_clusters= nb_clusters,
-
-.keep_old_clusters  = keep_old_clusters,
-
-.cow_start = {
-.offset = 0,
-.nb_bytes   = offset_into_cluster(s, guest_offset),
-},
-.cow_end = {
-.offset = nb_bytes,
-.nb_bytes   = avail_bytes - nb_bytes,
-},
-};
-qemu_co_queue_init(&(*m)->dependent_requests);
-QLIST_INSERT_HEAD(&s->cluster_allocs, *m, next_in_flight);
 
 *host_offset = alloc_cluster_offset + offset_into_cluster(s, guest_offset);
 *bytes = MIN(*bytes, nb_bytes - offset_into_cluster(s, guest_offset));
 assert(*bytes != 0);
 
+calculate_l2_meta(bs, alloc_cluster_offset, guest_offset, *bytes,
+  m, keep_old_clusters);
+
 return 1;
 
 fail:
-- 
2.20.1

[PATCH v6 29/32] qcow2: Add subcluster support to qcow2_measure()

[Alberto Garcia]

Extended L2 entries are bigger than normal L2 entries so this has an
impact on the amount of metadata needed for a qcow2 file.

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
---
 block/qcow2.c | 20 +---
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 40988fff55..bc7a4a1729 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3233,28 +3233,31 @@ int64_t qcow2_refcount_metadata_size(int64_t clusters, 
size_t cluster_size,
  * @total_size: virtual disk size in bytes
  * @cluster_size: cluster size in bytes
  * @refcount_order: refcount bits power-of-2 exponent
+ * @extended_l2: true if the image has extended L2 entries
  *
  * Returns: Total number of bytes required for the fully allocated image
  * (including metadata).
  */
 static int64_t qcow2_calc_prealloc_size(int64_t total_size,
 size_t cluster_size,
-int refcount_order)
+int refcount_order,
+bool extended_l2)
 {
 int64_t meta_size = 0;
 uint64_t nl1e, nl2e;
 int64_t aligned_total_size = ROUND_UP(total_size, cluster_size);
+size_t l2e_size = extended_l2 ? L2E_SIZE_EXTENDED : L2E_SIZE_NORMAL;
 
 /* header: 1 cluster */
 meta_size += cluster_size;
 
 /* total size of L2 tables */
 nl2e = aligned_total_size / cluster_size;
-nl2e = ROUND_UP(nl2e, cluster_size / sizeof(uint64_t));
-meta_size += nl2e * sizeof(uint64_t);
+nl2e = ROUND_UP(nl2e, cluster_size / l2e_size);
+meta_size += nl2e * l2e_size;
 
 /* total size of L1 tables */
-nl1e = nl2e * sizeof(uint64_t) / cluster_size;
+nl1e = nl2e * l2e_size / cluster_size;
 nl1e = ROUND_UP(nl1e, cluster_size / sizeof(uint64_t));
 meta_size += nl1e * sizeof(uint64_t);
 
@@ -4834,6 +4837,8 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, 
BlockDriverState *in_bs,
 PreallocMode prealloc;
 bool has_backing_file;
 bool has_luks;
+bool extended_l2 = false; /* Set to false until the option is added */
+size_t l2e_size;
 
 /* Parse image creation options */
 cluster_size = qcow2_opt_get_cluster_size_del(opts, &local_err);
@@ -4899,8 +4904,9 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, 
BlockDriverState *in_bs,
 virtual_size = ROUND_UP(virtual_size, cluster_size);
 
 /* Check that virtual disk size is valid */
+l2e_size = extended_l2 ? L2E_SIZE_EXTENDED : L2E_SIZE_NORMAL;
 l2_tables = DIV_ROUND_UP(virtual_size / cluster_size,
- cluster_size / sizeof(uint64_t));
+ cluster_size / l2e_size);
 if (l2_tables * sizeof(uint64_t) > QCOW_MAX_L1_SIZE) {
 error_setg(&local_err, "The image size is too large "
"(try using a larger cluster size)");
@@ -4963,9 +4969,9 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, 
BlockDriverState *in_bs,
 }
 
 info = g_new(BlockMeasureInfo, 1);
-info->fully_allocated =
+info->fully_allocated = luks_payload_size +
 qcow2_calc_prealloc_size(virtual_size, cluster_size,
- ctz32(refcount_bits)) + luks_payload_size;
+ ctz32(refcount_bits), extended_l2);
 
 /* Remove data clusters that are not required.  This overestimates the
  * required size because metadata needed for the fully allocated file is
-- 
2.20.1

[PATCH v6 14/32] qcow2: Add QCow2SubclusterType and qcow2_get_subcluster_type()

[Alberto Garcia]

This patch adds QCow2SubclusterType, which is the subcluster-level
version of QCow2ClusterType. All QCOW2_SUBCLUSTER_* values have the
the same meaning as their QCOW2_CLUSTER_* equivalents (when they
exist). See below for details and caveats.

In images without extended L2 entries clusters are treated as having
exactly one subcluster so it is possible to replace one data type with
the other while keeping the exact same semantics.

With extended L2 entries there are new possible values, and every
subcluster in the same cluster can obviously have a different
QCow2SubclusterType so functions need to be adapted to work on the
subcluster level.

There are several things that have to be taken into account:

  a) QCOW2_SUBCLUSTER_COMPRESSED means that the whole cluster is
 compressed. We do not support compression at the subcluster
 level.

  b) There are two different values for unallocated subclusters:
 QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN which means that the whole
 cluster is unallocated, and QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC
 which means that the cluster is allocated but the subcluster is
 not. The latter can only happen in images with extended L2
 entries.

  c) QCOW2_SUBCLUSTER_INVALID is used to detect the cases where an L2
 entry has a value that violates the specification. The caller is
 responsible for handling these situations.

 To prevent compatibility problems with images that have invalid
 values but are currently being read by QEMU without causing side
 effects, QCOW2_SUBCLUSTER_INVALID is only returned for images
 with extended L2 entries.

qcow2_cluster_to_subcluster_type() is added as a separate function
from qcow2_get_subcluster_type(), but this is only temporary and both
will be merged in a subsequent patch.

Signed-off-by: Alberto Garcia 
---
 block/qcow2.h | 126 +-
 1 file changed, 125 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 5c6bf48c7a..27dbcbc502 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -80,6 +80,21 @@
 
 #define QCOW_EXTL2_SUBCLUSTERS_PER_CLUSTER 32
 
+/* The subcluster X [0..31] is allocated */
+#define QCOW_OFLAG_SUB_ALLOC(X)   (1ULL << (X))
+/* The subcluster X [0..31] reads as zeroes */
+#define QCOW_OFLAG_SUB_ZERO(X)(QCOW_OFLAG_SUB_ALLOC(X) << 32)
+/* Subclusters [X, Y) (0 <= X <= Y <= 32) are allocated */
+#define QCOW_OFLAG_SUB_ALLOC_RANGE(X, Y) \
+(QCOW_OFLAG_SUB_ALLOC(Y) - QCOW_OFLAG_SUB_ALLOC(X))
+/* Subclusters [X, Y) (0 <= X <= Y <= 32) read as zeroes */
+#define QCOW_OFLAG_SUB_ZERO_RANGE(X, Y) \
+(QCOW_OFLAG_SUB_ALLOC_RANGE(X, Y) << 32)
+/* L2 entry bitmap with all allocation bits set */
+#define QCOW_L2_BITMAP_ALL_ALLOC  (QCOW_OFLAG_SUB_ALLOC_RANGE(0, 32))
+/* L2 entry bitmap with all "read as zeroes" bits set */
+#define QCOW_L2_BITMAP_ALL_ZEROES (QCOW_OFLAG_SUB_ZERO_RANGE(0, 32))
+
 /* Size of normal and extended L2 entries */
 #define L2E_SIZE_NORMAL   (sizeof(uint64_t))
 #define L2E_SIZE_EXTENDED (sizeof(uint64_t) * 2)
@@ -462,6 +477,33 @@ typedef struct QCowL2Meta
 QLIST_ENTRY(QCowL2Meta) next_in_flight;
 } QCowL2Meta;
 
+/*
+ * In images with standard L2 entries all clusters are treated as if
+ * they had one subcluster so QCow2ClusterType and QCow2SubclusterType
+ * can be mapped to each other and have the exact same meaning
+ * (QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC cannot happen in these images).
+ *
+ * In images with extended L2 entries QCow2ClusterType refers to the
+ * complete cluster and QCow2SubclusterType to each of the individual
+ * subclusters, so there are several possible combinations:
+ *
+ * |--+---|
+ * | Cluster type | Possible subcluster types |
+ * |--+---|
+ * | UNALLOCATED  | UNALLOCATED_PLAIN |
+ * |  |ZERO_PLAIN |
+ * |--+---|
+ * | NORMAL   | UNALLOCATED_ALLOC |
+ * |  |ZERO_ALLOC |
+ * |  |NORMAL |
+ * |--+---|
+ * | COMPRESSED   |COMPRESSED |
+ * |--+---|
+ *
+ * QCOW2_SUBCLUSTER_INVALID means that the L2 entry is incorrect and
+ * the image should be marked corrupt.
+ */
+
 typedef enum QCow2ClusterType {
 QCOW2_CLUSTER_UNALLOCATED,
 QCOW2_CLUSTER_ZERO_PLAIN,
@@ -470,6 +512,16 @@ typedef enum QCow2ClusterType {
 QCOW2_CLUSTER_COMPRESSED,
 } QCow2ClusterType;
 
+typedef enum QCow2SubclusterType {
+QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN,
+QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC,
+QCOW2_SUBCLUSTER_ZERO_PLAIN,
+QCOW2_SUBCLUSTER_ZERO_ALLOC,
+QCOW2_SUBCLUSTER_NORMAL,
+QCOW2_SUBCLUSTER_COMPRESSED,
+QCOW2_SUBCLUSTER_INVALID,
+} QCow2SubclusterType;
+
 typedef enum QCow2MetadataOverlap {
 QCOW2_OL_MAIN_HEADER_B

[PATCH v6 23/32] qcow2: Add subcluster support to discard_in_l2_slice()

[Alberto Garcia]

Two things need to be taken into account here:

1) With full_discard == true the L2 entry must be cleared completely.
   This also includes the L2 bitmap if the image has extended L2
   entries.

2) With full_discard == false we have to make the discarded cluster
   read back as zeroes. With normal L2 entries this is done with the
   QCOW_OFLAG_ZERO bit, whereas with extended L2 entries this is done
   with the individual 'all zeroes' bits for each subcluster.

   Note however that QCOW_OFLAG_ZERO is not supported in v2 qcow2
   images so, if there is a backing file, discard cannot guarantee
   that the image will read back as zeroes. If this is important for
   the caller it should forbid it as qcow2_co_pdiscard() does (see
   80f5c01183 for more details).

Signed-off-by: Alberto Garcia 
---
 block/qcow2-cluster.c | 51 +++
 1 file changed, 22 insertions(+), 29 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 4e59bbd545..5e70b76c21 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1847,11 +1847,16 @@ static int discard_in_l2_slice(BlockDriverState *bs, 
uint64_t offset,
 assert(nb_clusters <= INT_MAX);
 
 for (i = 0; i < nb_clusters; i++) {
-uint64_t old_l2_entry;
-
-old_l2_entry = get_l2_entry(s, l2_slice, l2_index + i);
+uint64_t old_l2_entry = get_l2_entry(s, l2_slice, l2_index + i);
+uint64_t old_l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index + i);
+uint64_t new_l2_entry = old_l2_entry;
+uint64_t new_l2_bitmap = old_l2_bitmap;
+QCow2ClusterType type = qcow2_get_cluster_type(bs, old_l2_entry);
 
 /*
+ * If full_discard is true, the cluster should not read back as zeroes,
+ * but rather fall through to the backing file.
+ *
  * If full_discard is false, make sure that a discarded area reads back
  * as zeroes for v3 images (we cannot do it for v2 without actually
  * writing a zero-filled buffer). We can skip the operation if the
@@ -1860,40 +1865,28 @@ static int discard_in_l2_slice(BlockDriverState *bs, 
uint64_t offset,
  *
  * TODO We might want to use bdrv_block_status(bs) here, but we're
  * holding s->lock, so that doesn't work today.
- *
- * If full_discard is true, the sector should not read back as zeroes,
- * but rather fall through to the backing file.
  */
-switch (qcow2_get_cluster_type(bs, old_l2_entry)) {
-case QCOW2_CLUSTER_UNALLOCATED:
-if (full_discard || !bs->backing) {
-continue;
+if (full_discard) {
+new_l2_entry = new_l2_bitmap = 0;
+} else if (bs->backing || qcow2_cluster_is_allocated(type)) {
+if (has_subclusters(s)) {
+new_l2_entry = 0;
+new_l2_bitmap = QCOW_L2_BITMAP_ALL_ZEROES;
+} else {
+new_l2_entry = s->qcow_version >= 3 ? QCOW_OFLAG_ZERO : 0;
 }
-break;
+}
 
-case QCOW2_CLUSTER_ZERO_PLAIN:
-if (!full_discard) {
-continue;
-}
-break;
-
-case QCOW2_CLUSTER_ZERO_ALLOC:
-case QCOW2_CLUSTER_NORMAL:
-case QCOW2_CLUSTER_COMPRESSED:
-break;
-
-default:
-abort();
+if (old_l2_entry == new_l2_entry && old_l2_bitmap == new_l2_bitmap) {
+continue;
 }
 
 /* First remove L2 entries */
 qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice);
-if (!full_discard && s->qcow_version >= 3) {
-set_l2_entry(s, l2_slice, l2_index + i, QCOW_OFLAG_ZERO);
-} else {
-set_l2_entry(s, l2_slice, l2_index + i, 0);
+set_l2_entry(s, l2_slice, l2_index + i, new_l2_entry);
+if (has_subclusters(s)) {
+set_l2_bitmap(s, l2_slice, l2_index + i, new_l2_bitmap);
 }
-
 /* Then decrease the refcount */
 qcow2_free_any_clusters(bs, old_l2_entry, 1, type);
 }
-- 
2.20.1

[PATCH] fuzz: add information about useful libFuzzer flags

[Alexander Bulekov]

Signed-off-by: Alexander Bulekov 
---
 docs/devel/fuzzing.txt | 33 +
 1 file changed, 33 insertions(+)

diff --git a/docs/devel/fuzzing.txt b/docs/devel/fuzzing.txt
index 324d2cd92b..3458763e53 100644
--- a/docs/devel/fuzzing.txt
+++ b/docs/devel/fuzzing.txt
@@ -45,6 +45,39 @@ Information about these is available by passing -help=1
 Now the only thing left to do is wait for the fuzzer to trigger potential
 crashes.
 
+== Useful libFuzzer flags ==
+
+As mentioned above, libFuzzer accepts some arguments. Passing -help=1 will list
+the available arguments. In particular, these arguments might be helpful:
+
+$CORPUS_DIR/ : Specify a directory as the last argument to libFuzzer. libFuzzer
+stores each "interesting" input in this corpus directory. The next time you run
+libFuzzer, it will read all of the inputs from the corpus, and continue fuzzing
+from there.
+
+-max_len=4096 : specify the maximum byte-length of the inputs libFuzzer will
+generate.
+
+-close_fd_mask={1,2,3} : close, stderr, or both. Useful for targets that
+trigger many debug/error messages, or create output on the serial console.
+
+-jobs=4 -workers=4 : These arguments configure libFuzzer to run 4 fuzzers in
+parallel. Replace 4 with a number appropriate for your machine. Make sure to
+specify a $CORPUS_DIR, which will allow the parallel fuzzers to share
+information about the interesting inputs they find.
+
+-use_value_profile=1 : For each comparison operation, libFuzzer computes 
+(caller_pc&4095) | (popcnt(Arg1 ^ Arg2) << 12) and places this in the coverage
+table. Useful for targets with "magic" constants. If Arg1 came from the 
fuzzer's
+input and Arg2 is a magic constant, then each time the hamming distance
+between Arg1 and Arg2 decreases, libFuzzer adds the input to the corpus.
+
+-shrink=1 : Tries to make elements of the corpus "smaller". Might lead to
+better coverage performance, depending on the target.
+
+Note that libFuzzer's exact behavior will depend on the version of
+clang and libFuzzer used to build the device fuzzers.
+
 == Adding a new fuzzer ==
 Coverage over virtual devices can be improved by adding additional fuzzers.
 Fuzzers are kept in tests/qtest/fuzz/ and should be added to
-- 
2.26.2

Re: [Bug 1880355] [NEW] Length restrictions for fw_cfg_dma_transfer?

[Philippe Mathieu-Daudé]

On 5/24/20 3:40 PM, Peter Maydell wrote:
> On Sun, 24 May 2020 at 11:30, Philippe Mathieu-Daudé  
> wrote:
>> It looks to me a normal behavior for a DMA device. DMA devices have a
>> different address space view than the CPUs.
>> Also note the fw_cfg is a generic device, not restricted to the x86 arch.
> 
> In an ideal world all our DMA devices would use some kind of common
> framework or design pattern so they didn't hog all the CPU
> and/or spend minutes with the BQL held if the guest requests
> an enormous-sized DMA. In practice many of them just have
> a simple "loop until the DMA transfer is complete" implementation...

Is this framework already implemented in the hidden dma-helpers.c?

Apparently this file was written for BlockBackend, but the code seems
rather generic.

Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times

[Emilio G. Cota]

Hi Alex,

On Tue, May 12, 2020 at 21:11:46 +0100, Alex Bennée wrote:
> 
> Emilio G. Cota  writes:
> 
> > On Mon, May 11, 2020 at 18:53:19 +0300, Nikolay Igotti wrote:
> >> Attached to the mail counter.c when running with attached test.c compiled
> >> to Linux standalone binary shows failing assert, unless the patch is
> >> applied.
> >
> > I didn't get the attachment. Can you paste the code at the end of your
> > reply?
> 
> So the problem is we are recycling cpu_index:
> 
>   21:01:54 [alex@zen:~/l/q/b/all.plugin] plugins/next|●2✚3…(+8 insertions(+)) 
> 127 + ./x86_64-linux-user/qemu-x86_64 
> ./tests/tcg/x86_64-linux-user/threadcount
>   cpu_list_add: 0x5605598579f0
>   cpu_get_free_index: 0
>   cpu_list_add: 0x5605598c6010
>   cpu_get_free_index: 1
>   cpu_list_add: 0x5605598ec330
>   cpu_get_free_index: 2
>   cpu_list_add: 0x560559912a30
>   cpu_get_free_index: 3
>   cpu_list_add: 0x560559939130
>   cpu_get_free_index: 4
>   cpu_list_add: 0x56055995d360
>   cpu_get_free_index: 1
>   **
>   
> ERROR:/home/alex/lsrc/qemu.git/plugins/core.c:205:qemu_plugin_vcpu_init_hook: 
> assertion failed: (success)
>   qemu:handle_cpu_signal received signal outside vCPU context @ 
> pc=0x7fa29c702613
> 
> However I'm confused as to how. The cpu_index is really just a function
> of the number of CPUs in the cpus list and I can't see where we free
> them up. We never call cpu_common_unrealizefn so the list should never
> shrink. We should just end up with a slowly growing list of now dead CPU
> states.
> 
> I'm giving up for now but will have a look again in the morning if no
> one else has figured out what is going on by then.

I just got this fixed, only to realise that you already fixed it weeks ago :-)
(I didn't see your fixes because I'm not subscribed to the list.)

My changes are here https://github.com/cota/qemu/tree/cpu_index
Please feel free to take anything from there that you find valuable, e.g.
the use of a bitmap for tracking cpu_index'es and the addition of a unit test
(note that the test is fixed in the last commit).

Thanks,

Emilio

Re: [PATCH RFC v3 01/11] linux-headers: Update

[Peter Xu]

Hi, Peter,

On Sun, May 24, 2020 at 02:27:14PM +0100, Peter Maydell wrote:
> On Sun, 24 May 2020 at 00:21, Peter Xu  wrote:
> >
> > Signed-off-by: Peter Xu 
> 
> Header updates should always include the upstream
> kernel commit against which you ran the scripts/update-linux-headers.sh
> script, please.

This is based on a kernel series that hasn't yet been merged, so I didn't tag
it (so this is still a RFC series).  Will do when it's merged.

> 
>  linux-headers/asm-x86/kvm.h |  1 +
>  linux-headers/linux/kvm.h   | 53 +
> 
> Are these really the only files which had changes? It looks
> a suspiciously short list...

I didn't check, but I did use the script all the time..

Thanks,

-- 
Peter Xu

Re: [PATCH 0/4] hw/display/cirrus_vga: Convert debug printf() to qemu_log_mask & trace

[Philippe Mathieu-Daudé]

On 5/24/20 2:26 PM, no-re...@patchew.org wrote:
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
[...]
> 3/4 Checking commit 0b01a633fe92 (hw/display/cirrus_vga: Use 
> qemu_log_mask(ERROR) instead of debug printf)
> ERROR: suspect code indent for conditional statements (16, 12)
> #31: FILE: hw/display/cirrus_vga.c:1038:
> if (s->cirrus_blt_pixelwidth > 2) {
> +qemu_log_mask(LOG_GUEST_ERROR,

False positive. Checkpatch is confused by the mis-indented code previous
to this line.

> 
> total: 1 errors, 0 warnings, 156 lines checked
> 
> Patch 3/4 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
[...]

Re: [PATCH] hw/display/omap_dss: Replace fprintf() call by qemu_log_mask(LOG_UNIMP)

[Philippe Mathieu-Daudé]

On 5/24/20 2:41 PM, Alexander Bulekov wrote:
> On 200524 1238, Philippe Mathieu-Daudé wrote:
>> Replace fprintf() call by qemu_log_mask(LOG_UNIMP), which is
>> disabled by default. This avoid flooding the terminal when
>> fuzzing the device.
>>
> Hi Philippe,
> libfuzzer also lets you pass -close_fd_mask=[1,2,3] to close stdout,
> stderr or both. As a side-effect, In this mode, libfuzzer will write the
> crashing stacktraces to individual files ./.$PID, instead of to the
> terminal's output.

Thanks, good to know.

This information is worth adding in docs/devel/fuzzing.txt :P

> As far as I know, closing the FDs doesn't bypass all
> of printf's allocation/formatting code, so getting rid of calls can
> still be a good idea for performance, sometimes.

Certainly.

> -Alex
> 
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  hw/display/omap_dss.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/display/omap_dss.c b/hw/display/omap_dss.c
>> index 32dc0d6aa7..21fde58a26 100644
>> --- a/hw/display/omap_dss.c
>> +++ b/hw/display/omap_dss.c
>> @@ -619,7 +619,7 @@ static void omap_rfbi_transfer_start(struct omap_dss_s 
>> *s)
>>  if (s->rfbi.control & (1 << 1)) {   /* 
>> BYPASS */
>>  /* TODO: in non-Bypass mode we probably need to just assert the
>>   * DRQ and wait for DMA to write the pixels.  */
>> -fprintf(stderr, "%s: Bypass mode unimplemented\n", __func__);
>> +qemu_log_mask(LOG_UNIMP, "%s: Bypass mode unimplemented\n", 
>> __func__);
>>  return;
>>  }
>>  
>> -- 
>> 2.21.3
>>
> 

Re: [PULL 0/4] pflash-next patches for 2020-05-22

[Peter Maydell]

On Fri, 22 May 2020 at 18:47, Philippe Mathieu-Daudé  wrote:
>
> The following changes since commit d19f1ab0de8b763159513e3eaa12c5bc68122361:
>
>   Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-2020052=
> 1-1' into staging (2020-05-21 22:06:56 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/philmd/qemu.git tags/pflash-next-20200522
>
> for you to fetch changes up to 1857b9db49770590483be44eb90993c42b2a5a99:
>
>   hw/block/pflash: Check return value of blk_pwrite() (2020-05-22 19:38:14 +0=
> 200)
>
> 
>
> - Remove unused timer in CFI01 flash,
> - Clean up code documentation,
> - Silent a long-standing Coverity warning (2016-07-15).


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.1
for any user-visible changes.

-- PMM

Re: [Bug 1880355] [NEW] Length restrictions for fw_cfg_dma_transfer?

[Peter Maydell]

On Sun, 24 May 2020 at 11:30, Philippe Mathieu-Daudé  wrote:
> It looks to me a normal behavior for a DMA device. DMA devices have a
> different address space view than the CPUs.
> Also note the fw_cfg is a generic device, not restricted to the x86 arch.

In an ideal world all our DMA devices would use some kind of common
framework or design pattern so they didn't hog all the CPU
and/or spend minutes with the BQL held if the guest requests
an enormous-sized DMA. In practice many of them just have
a simple "loop until the DMA transfer is complete" implementation...

thanks
-- PMM

Re: [PATCH RFC v3 01/11] linux-headers: Update

[Peter Maydell]

On Sun, 24 May 2020 at 00:21, Peter Xu  wrote:
>
> Signed-off-by: Peter Xu 

Header updates should always include the upstream
kernel commit against which you ran the scripts/update-linux-headers.sh
script, please.

 linux-headers/asm-x86/kvm.h |  1 +
 linux-headers/linux/kvm.h   | 53 +

Are these really the only files which had changes? It looks
a suspiciously short list...

thanks
-- PMM

Re: [PATCH RFC v3 00/11] KVM: Dirty ring support (QEMU part)

[Peter Xu]

On Sat, May 23, 2020 at 07:20:24PM -0400, Peter Xu wrote:
> I kept the dirty sync in kvm_set_phys_mem() for kvmslot removals, left a
> comment on the known issue about strict dirty sync so we can fix it someday in
> the future together with dirty log and dirty ring.

Side note: patch 3,5-8 should not be RFC material at all - they either fixes
existing issues or clean code up.  Please conside to review/merge them first
even before the rest of the patches.  Thanks,

-- 
Peter Xu

Re: [PATCH] hw/display/omap_dss: Replace fprintf() call by qemu_log_mask(LOG_UNIMP)

[Alexander Bulekov]

On 200524 1238, Philippe Mathieu-Daudé wrote:
> Replace fprintf() call by qemu_log_mask(LOG_UNIMP), which is
> disabled by default. This avoid flooding the terminal when
> fuzzing the device.
> 
Hi Philippe,
libfuzzer also lets you pass -close_fd_mask=[1,2,3] to close stdout,
stderr or both. As a side-effect, In this mode, libfuzzer will write the
crashing stacktraces to individual files ./.$PID, instead of to the
terminal's output. As far as I know, closing the FDs doesn't bypass all
of printf's allocation/formatting code, so getting rid of calls can
still be a good idea for performance, sometimes.
-Alex

> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/display/omap_dss.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/display/omap_dss.c b/hw/display/omap_dss.c
> index 32dc0d6aa7..21fde58a26 100644
> --- a/hw/display/omap_dss.c
> +++ b/hw/display/omap_dss.c
> @@ -619,7 +619,7 @@ static void omap_rfbi_transfer_start(struct omap_dss_s *s)
>  if (s->rfbi.control & (1 << 1)) {/* 
> BYPASS */
>  /* TODO: in non-Bypass mode we probably need to just assert the
>   * DRQ and wait for DMA to write the pixels.  */
> -fprintf(stderr, "%s: Bypass mode unimplemented\n", __func__);
> +qemu_log_mask(LOG_UNIMP, "%s: Bypass mode unimplemented\n", 
> __func__);
>  return;
>  }
>  
> -- 
> 2.21.3
> 

Re: [PATCH 0/4] hw/display/cirrus_vga: Convert debug printf() to qemu_log_mask & trace

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200524110121.12686-1-f4...@amsat.org/



Hi,

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

Message-id: 20200524110121.12686-1-f4...@amsat.org
Subject: [PATCH 0/4] hw/display/cirrus_vga: Convert debug printf() to 
qemu_log_mask & trace
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
f2c5a76 hw/display/cirrus_vga: Convert debug printf() to trace event
0b01a63 hw/display/cirrus_vga: Use qemu_log_mask(ERROR) instead of debug printf
74d18b7 hw/display/cirrus_vga: Use qemu_log_mask(UNIMP) instead of debug printf
9000535 hw/display/cirrus_vga: Convert debug printf() to trace event

=== OUTPUT BEGIN ===
1/4 Checking commit 90005354b7d4 (hw/display/cirrus_vga: Convert debug printf() 
to trace event)
2/4 Checking commit 74d18b7ca83b (hw/display/cirrus_vga: Use 
qemu_log_mask(UNIMP) instead of debug printf)
3/4 Checking commit 0b01a633fe92 (hw/display/cirrus_vga: Use 
qemu_log_mask(ERROR) instead of debug printf)
ERROR: suspect code indent for conditional statements (16, 12)
#31: FILE: hw/display/cirrus_vga.c:1038:
if (s->cirrus_blt_pixelwidth > 2) {
+qemu_log_mask(LOG_GUEST_ERROR,

total: 1 errors, 0 warnings, 156 lines checked

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

4/4 Checking commit f2c5a7686574 (hw/display/cirrus_vga: Convert debug printf() 
to trace event)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200524110121.12686-1-f4...@amsat.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com