Re: [PATCH] scripts: Fix undefinited name 'file' error for python3

[Han Han]

On Wed, Dec 4, 2019 at 4:23 PM Thomas Huth  wrote:

> On 04/12/2019 07.48, Han Han wrote:
> > Anyone help to review it?
>
>  Hi!
>
> When sending patches to the qemu-devel mailing list, please always make
> sure to put the corresponding maintainers on CC:, otherwise your mails
> might get lost in the high traffic of the mailing list. For this case,
> it would have been good to CC: the "Migration" and "Python script"
> maintainers, see the corresponding sections of the MAINTAINERS file in
> the top most directory of the QEMU sources.
>
OK. Thanks for your advice

>
> Anyway, it seems someone else ran into the same issue already, too, and
>  it got already fixed here:
>
> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=e8d0ac5801edda91412e5
>
>   Thomas
>
>
> > On Tue, Nov 26, 2019 at 1:54 PM Han Han  > > wrote:
> >
> > ping
> >
> > On Wed, Nov 13, 2019 at 9:17 PM Han Han  > > wrote:
> >
> > In python3, 'file' is no longer a keyword for file type object.
> > So it
> > will can error when run the scripts by python3:
> >
> > $ python3 ./scripts/vmstate-static-checker.py -s 4.0.json -d
> > 4.1.json
> > Traceback (most recent call last):
> >   File "./scripts/vmstate-static-checker.py", line 431, in
> 
> > sys.exit(main())
> >   File "./scripts/vmstate-static-checker.py", line 378, in main
> > parser.add_argument('-s', '--src', type=file, required=True,
> > NameError: name 'file' is not defined
> >
> > Replace file type to argparse.FileType('r').
> >
> > Signed-off-by: Han Han mailto:h...@redhat.com
> >>
> > ---
> >  scripts/vmstate-static-checker.py | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/vmstate-static-checker.py
> > b/scripts/vmstate-static-checker.py
> > index d3467288..14f199a0 100755
> > --- a/scripts/vmstate-static-checker.py
> > +++ b/scripts/vmstate-static-checker.py
> > @@ -375,9 +375,9 @@ def main():
> >  help_text = "Parse JSON-formatted vmstate dumps from QEMU
> > in files SRC and DEST.  Checks whether migration from SRC to
> > DEST QEMU versions would break based on the VMSTATE information
> > contained within the JSON outputs.  The JSON output is created
> > from a QEMU invocation with the -dump-vmstate parameter and a
> > filename argument to it.  Other parameters to QEMU do not
> > matter, except the -M (machine type) parameter."
> >
> >  parser = argparse.ArgumentParser(description=help_text)
> > -parser.add_argument('-s', '--src', type=file, required=True,
> > +parser.add_argument('-s', '--src',
> > type=argparse.FileType('r'), required=True,
> >  help='json dump from src qemu')
> > -parser.add_argument('-d', '--dest', type=file,
> required=True,
> > +parser.add_argument('-d', '--dest',
> > type=argparse.FileType('r'), required=True,
> >  help='json dump from dest qemu')
> >  parser.add_argument('--reverse', required=False,
> default=False,
> >  action='store_true',
> > --
> > 2.23.0
> >
> >
> >
> > --
> > Best regards,
> > ---
> > Han Han
> > Quality Engineer
> > Redhat.
> >
> > Email: h...@redhat.com 
> > Phone: +861065339333
>
>

-- 
Best regards,
---
Han Han
Quality Engineer
Redhat.

Email: h...@redhat.com
Phone: +861065339333

Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[Pankaj Gupta]

> From: Pan Nengyuan 
> 
> Devices tend to maintain vq pointers, allow deleting them trough a vq
> pointer.
> 
> Signed-off-by: Michael S. Tsirkin 
> Signed-off-by: Pan Nengyuan 
> ---
> Changes v2 to v1:
> - add a new function virtio_delete_queue to cleanup vq through a vq pointer
> ---
>  hw/virtio/virtio.c | 16 +++-
>  include/hw/virtio/virtio.h |  2 ++
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 04716b5..6de3cfd 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int
> queue_size,
>  return >vq[i];
>  }
>  
> +void virtio_delete_queue(VirtQueue *vq)
> +{
> +vq->vring.num = 0;
> +vq->vring.num_default = 0;
> +vq->handle_output = NULL;
> +vq->handle_aio_output = NULL;
> +g_free(vq->used_elems);
> +vq->used_elems = NULL;
> +}
> +
>  void virtio_del_queue(VirtIODevice *vdev, int n)
>  {
>  if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
>  abort();
>  }
>  
> -vdev->vq[n].vring.num = 0;
> -vdev->vq[n].vring.num_default = 0;
> -vdev->vq[n].handle_output = NULL;
> -vdev->vq[n].handle_aio_output = NULL;
> -g_free(vdev->vq[n].used_elems);
> +virtio_delete_queue(>vq[n]);
>  }
>  
>  static void virtio_set_isr(VirtIODevice *vdev, int value)
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index c32a815..e18756d 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int
> queue_size,
>  
>  void virtio_del_queue(VirtIODevice *vdev, int n);
>  
> +void virtio_delete_queue(VirtQueue *vq);
> +
>  void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
>  unsigned int len);
>  void virtqueue_flush(VirtQueue *vq, unsigned int count);
> --
> 2.7.2.windows.1
> 
> 
Overall it ooks good to me.

Just one point: e.g in virtio_rng: "virtio_rng_device_unrealize" function
We are doing : virtio_del_queue(vdev, 0);

One can directly call "virtio_delete_queue". It can become confusing
to call multiple functions for same purpose. Instead, Can we make 
"virtio_delete_queue" static inline?

Other than that:
Reviewed-by: Pankaj Gupta 

> 
> 

Re: [PATCH v6 3/9] qdev: add clock input support to devices.

[Damien Hedde]

On 12/2/19 3:34 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde  wrote:
>>
>> Add functions to easily add input or output clocks to a device.
>> A clock objects is added as a child of the device.
> 
> "object"
> 
>> The api is very similar the gpio's one.
> 
> "API"; "to the GPIO API".
> 
>>
>> This is based on the original work of Frederic Konrad.
>>
>> Signed-off-by: Damien Hedde 
>>
> 
>> +static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char 
>> *name,
>> +bool forward)
>> +{
>> +NamedClockList *ncl;
>> +
>> +/*
>> + * The clock path will be computed by the device's realize function 
>> call.
>> + * This is required to ensure the clock's canonical path is right and 
>> log
>> + * messages are meaningfull.
> 
> "meaningful"
> 
>> + */
>> +assert(name);
>> +assert(!dev->realized);
>> +
>> +/* The ncl structure will be freed in device's finalize function call */
> 
> Do you mean "in device_finalize()", or "in the finalize method
> of the device" ?  If you mean a specific function, then it's
> good to name it, so the reader can go and check that code if
> they need to confirm that there's a matching free()/deref/etc.

Yes, it is device_finalize(). I'll update the comment.

> 
>> +ncl = g_malloc0(sizeof(*ncl));
> 
> Prefer g_new0(NamedClockList, 1).
> 
>> +ncl->name = g_strdup(name);
>> +ncl->forward = forward;
>> +
>> +QLIST_INSERT_HEAD(>clocks, ncl, node);
>> +return ncl;
>> +}
>> +
>> +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name)
>> +{
>> +NamedClockList *ncl;
>> +Object *clk;
>> +
>> +ncl = qdev_init_clocklist(dev, name, false);
>> +
>> +clk = object_new(TYPE_CLOCK_OUT);
>> +
>> +/* will fail if name already exists */
> 
> This is true but it would be more helpful to say
>  /*
>   * Trying to create a clock whose name clashes with some other
>   * clock or property is a bug in the caller and we will abort().
>   */
> 
> (assuming that's what's going on here).

You're right.

> 
>> +object_property_add_child(OBJECT(dev), name, clk, _abort);
>> +object_unref(clk); /* remove the initial ref made by object_new */
>> +
>> +ncl->out = CLOCK_OUT(clk);
>> +return ncl->out;
>> +}
>> +
>> +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name,
>> +ClockCallback *callback, void *opaque)
>> +{
>> +NamedClockList *ncl;
>> +Object *clk;
>> +
>> +ncl = qdev_init_clocklist(dev, name, false);
>> +
>> +clk = object_new(TYPE_CLOCK_IN);
>> +/*
>> + * the ref initialized by object_new will be cleared during dev 
>> finalize.
> 
> This means "in device_finalize()", I think from reading later patches ?

Yes. I'll update the comment too.
> 
>> + * It allows us to safely remove the callback.
>> + */
>> +
>> +/* will fail if name already exists */
> 
> Similar remark as for earlier comment.
> 
>> +object_property_add_child(OBJECT(dev), name, clk, _abort);
>> +
>> +ncl->in = CLOCK_IN(clk);
>> +if (callback) {
>> +clock_set_callback(ncl->in, callback, opaque);
>> +}
>> +return ncl->in;
>> +}
> 
>> +ClockIn *qdev_get_clock_in(DeviceState *dev, const char *name)
>> +{
>> +NamedClockList *ncl;
>> +
>> +assert(dev && name);
>> +
>> +ncl = qdev_get_clocklist(dev, name);
>> +return ncl ? ncl->in : NULL;
>> +}
> 
> Do we expect to want to be able to pass in the name of
> a clock that doesn't exist ? Should that be an error
> rather than returning NULL ?

I think it can be an error and we may assert the clock exists.

> 
>> +
>> +static ClockOut *qdev_get_clock_out(DeviceState *dev, const char *name)
>> +{
>> +NamedClockList *ncl;
>> +
>> +assert(dev && name);
>> +
>> +ncl = qdev_get_clocklist(dev, name);
>> +return ncl ? ncl->out : NULL;
> 
> Ditto.
> 
>> +}
>> +
>> +void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn 
>> *clk,
>> +Error **errp)
>> +{
>> +ClockOut *clkout = qdev_get_clock_out(dev, name);
>> +
>> +if (!clk) {
>> +error_setg(errp, "NULL input clock");
>> +return;
>> +}
>> +
>> +if (!clkout) {
>> +error_setg(errp, "no output clock '%s' in device", name);
>> +return;
>> +}
>> +
>> +clock_connect(clk, clkout);
> 
> Do we need to support returning an error here, or would it
> always be a programming bug to try to connect a non-existent clock?

As for above, I think it can be considered an error. Should I remove the
errp and do an assert instead ?

> 
>> --- /dev/null
>> +++ b/include/hw/qdev-clock.h
>> @@ -0,0 +1,67 @@
>> +#ifndef QDEV_CLOCK_H
>> +#define QDEV_CLOCK_H
> 
> Another missing copyright/license comment.
> 
>> +
>> +#include "hw/clock.h"
>> +
>> +/**
>> + * qdev_init_clock_in:
>> + * @dev: the device in which to add a clock
> 
> "the device to add a clock input to"
> 
>> + * @name: the name of the clock (can't be NULL).
>> + * 

Re: qom device lifecycle interaction with hotplug/hotunplug ?

[Jens Freimann]

On Tue, Dec 03, 2019 at 06:40:04PM -0300, Eduardo Habkost wrote:

+jfreimann, +mst

On Sat, Nov 30, 2019 at 11:10:19AM +, Peter Maydell wrote:

On Fri, 29 Nov 2019 at 20:05, Eduardo Habkost  wrote:
> So, to summarize the current issues:
>
> 1) realize triggers a plug operation implicitly.
> 2) unplug triggers unrealize implicitly.
>
> Do you expect to see use cases that will require us to implement
> realize-without-plug?

I don't think so, but only because of the oddity that
we put lots of devices on the 'sysbus' and claim that
that's plugging them into the bus. The common case of
'realize' is where one device (say an SoC) has a bunch of child
devices (like UARTs); the SoC's realize method realizes its child
devices. Those devices all end up plugged into the 'sysbus'
but there's no actual bus there, it's fictional and about
the only thing it matters for is reset propagation (which
we don't model right either). A few devices don't live on
buses at all.


That's my impression as well.



> Similarly, do you expect use cases that will require us to
> implement unplug-without-unrealize?

I don't know enough about hotplug to answer this one:
it's essentially what I'm hoping you'd be able to answer.
I vaguely had in mind that eg the user might be able to
create a 'disk' object, plug it into a SCSI bus, then
unplug it from the bus without the disk and all its data
evaporating, and maybe plug it back into the SCSI
bus (or some other SCSI bus) later ? But I don't know
anything about how we expose that kind of thing to the
user via QMP/HMP.


This ability isn't exposed to the user at all.  Our existing
interfaces are -device, device_add and device_del.

We do have something new that sounds suspiciously similar to
"unplugged but not unrealized", though: the new hidden device
API, added by commit f3a850565693 ("qdev/qbus: add hidden device
support").

Jens, Michael, what exactly is the difference between a "hidden"
device and a "unplugged" device?


"hidden" the way we use it for virtio-net failover is actually unplugged. But it
doesn't have to be that way. You can register a function that decides
if the device should be hidden, i.e. plugged now, or do something else
with it (in the virtio-net failover case we just save everything we
need to plug the device later).  


We did introduce a "unplugged but not unrealized" function too as part
of the failover feature. See "a99c4da9fc pci: mark devices partially
unplugged"

This was needed so we would be able to re-plug the device in case a
migration failed and we need to hotplug the primary device back to the
guest. To avoid the risk of not getting the resources the device needs
we don't unrealize but just trigger the unplug from the guest OS.

regards
Jens

Re: [PATCH v2 00/18] Error handling fixes

[David Hildenbrand]

On 04.12.19 10:36, Markus Armbruster wrote:
> v2:
> * Old PATCH 01-03 have been merged for 4.2
> * PATCH 05-15: Commit message rephrased [David], R-bys kept

Thanks!


-- 
Thanks,

David / dhildenb

[PATCH v2 03/18] io: Fix Error usage in a comment

[Markus Armbruster]

Cc: "Daniel P. Berrangé" 
Signed-off-by: Markus Armbruster 
---
 include/io/task.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/io/task.h b/include/io/task.h
index 5cb9faf9f2..1abbfb8b65 100644
--- a/include/io/task.h
+++ b/include/io/task.h
@@ -119,7 +119,7 @@ typedef void (*QIOTaskWorker)(QIOTask *task,
  *   gboolean myobject_operation_timer(gpointer opaque)
  *   {
  *  QIOTask *task = QIO_TASK(opaque);
- *  Error *err;*
+ *  Error *err = NULL;
  *
  *  ...check something important...
  *   if (err) {
-- 
2.21.0

Re: [PATCH v2 00/18] Error handling fixes

[Markus Armbruster]

Neglected to mention: I'm fine with taking care of getting the whole
series merged.  Maintainers, feel free to take "your" parts through your
tree regardless.  Whatever is left after a while I'll take through mine.

Re: [PATCH v4 14/40] target/arm: Recover 4 bits from TBFLAGs

[Alex Bennée]

Richard Henderson  writes:

> We had completely run out of TBFLAG bits.
> Split A- and M-profile bits into two overlapping buckets.
> This results in 4 free bits.
>
> Signed-off-by: Richard Henderson 
> ---
>  target/arm/cpu.h   | 52 ---
>  target/arm/helper.c| 17 ++---
>  target/arm/translate.c | 56 +++---
>  3 files changed, 70 insertions(+), 55 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 28259be733..ae9fc1ded3 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3188,38 +3188,50 @@ FIELD(TBFLAG_ANY, BE_DATA, 23, 1)
>   */
>  FIELD(TBFLAG_ANY, DEBUG_TARGET_EL, 21, 2)

I'm not sure if this visual aid helps but here you go:

 *  31  20 1916 15 10 90
 * +--++-+--+
 * |  ||   TBFLAG_A64   |
 * |  | +--+-+--+
 * | TBFLAG_ANY   | |   TBFLAG_A32   |  |
 * |  | +-+--+  TBFLAG_AM32 |
 * |  |   |TBFLAG_M32|  |
 * +--+---+--+--+

>  
> -/* Bit usage when in AArch32 state: */
> -FIELD(TBFLAG_A32, THUMB, 0, 1)  /* Not cached. */
> -FIELD(TBFLAG_A32, VECLEN, 1, 3) /* Not cached. */
> -FIELD(TBFLAG_A32, VECSTRIDE, 4, 2)  /* Not cached. */
> +/*
> + * Bit usage when in AArch32 state, both A- and M-profile.
> + */
> +FIELD(TBFLAG_AM32, CONDEXEC, 0, 8)  /* Not cached. */
> +FIELD(TBFLAG_AM32, THUMB, 8, 1) /* Not cached. */
> +
> +/*
> + * Bit usage when in AArch32 state, for A-profile only.
> + */
> +FIELD(TBFLAG_A32, VECLEN, 9, 3) /* Not cached. */
> +FIELD(TBFLAG_A32, VECSTRIDE, 12, 2) /* Not cached. */
>  /*
>   * We store the bottom two bits of the CPAR as TB flags and handle
>   * checks on the other bits at runtime. This shares the same bits as
>   * VECSTRIDE, which is OK as no XScale CPU has VFP.
>   * Not cached, because VECLEN+VECSTRIDE are not cached.
>   */
> -FIELD(TBFLAG_A32, XSCALE_CPAR, 4, 2)
> +FIELD(TBFLAG_A32, XSCALE_CPAR, 12, 2)
> +FIELD(TBFLAG_A32, VFPEN, 14, 1) /* Partially cached, minus FPEXC. */
> +FIELD(TBFLAG_A32, SCTLR_B, 15, 1)
>  /*
>   * Indicates whether cp register reads and writes by guest code should access
>   * the secure or nonsecure bank of banked registers; note that this is not
>   * the same thing as the current security state of the processor!
>   */
> -FIELD(TBFLAG_A32, NS, 6, 1)
> -FIELD(TBFLAG_A32, VFPEN, 7, 1)  /* Partially cached, minus FPEXC. */
> -FIELD(TBFLAG_A32, CONDEXEC, 8, 8)   /* Not cached. */
> -FIELD(TBFLAG_A32, SCTLR_B, 16, 1)
> -/* For M profile only, set if FPCCR.LSPACT is set */
> -FIELD(TBFLAG_A32, LSPACT, 18, 1)/* Not cached. */
> -/* For M profile only, set if we must create a new FP context */
> -FIELD(TBFLAG_A32, NEW_FP_CTXT_NEEDED, 19, 1) /* Not cached. */
> -/* For M profile only, set if FPCCR.S does not match current security state 
> */
> -FIELD(TBFLAG_A32, FPCCR_S_WRONG, 20, 1) /* Not cached. */
> -/* For M profile only, Handler (ie not Thread) mode */
> -FIELD(TBFLAG_A32, HANDLER, 21, 1)
> -/* For M profile only, whether we should generate stack-limit checks */
> -FIELD(TBFLAG_A32, STACKCHECK, 22, 1)
> +FIELD(TBFLAG_A32, NS, 16, 1)
>  
> -/* Bit usage when in AArch64 state */
> +/*
> + * Bit usage when in AArch32 state, for M-profile only.
> + */
> +/* Handler (ie not Thread) mode */
> +FIELD(TBFLAG_M32, HANDLER, 9, 1)
> +/* Whether we should generate stack-limit checks */
> +FIELD(TBFLAG_M32, STACKCHECK, 10, 1)
> +/* Set if FPCCR.LSPACT is set */
> +FIELD(TBFLAG_M32, LSPACT, 11, 1) /* Not cached. */
> +/* Set if we must create a new FP context */
> +FIELD(TBFLAG_M32, NEW_FP_CTXT_NEEDED, 12, 1) /* Not cached. */
> +/* Set if FPCCR.S does not match current security state */
> +FIELD(TBFLAG_M32, FPCCR_S_WRONG, 13, 1)  /* Not cached. */
> +
> +/*
> + * Bit usage when in AArch64 state
> + */
>  FIELD(TBFLAG_A64, TBII, 0, 2)
>  FIELD(TBFLAG_A64, SVEEXC_EL, 2, 2)
>  FIELD(TBFLAG_A64, ZCR_LEN, 4, 4)
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 5172843667..ec5c7fa325 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -11207,11 +11207,8 @@ static uint32_t rebuild_hflags_m32(CPUARMState *env, 
> int fp_el,
>  {
>  uint32_t flags = 0;
>  
> -/* v8M always enables the fpu.  */
> -flags = FIELD_DP32(flags, TBFLAG_A32, VFPEN, 1);
> -
>  if (arm_v7m_is_handler_mode(env)) {
> -flags = FIELD_DP32(flags, TBFLAG_A32, HANDLER, 1);
> +flags = FIELD_DP32(flags, TBFLAG_M32, HANDLER, 1);
>  }
>  
>  /*
> @@ -11222,7 +11219,7 @@ static uint32_t rebuild_hflags_m32(CPUARMState *env, 
> int fp_el,
>  if (arm_feature(env, ARM_FEATURE_V8) &&
>  

[PATCH] travis.yml: Drop libcap-dev

[Greg Kurz]

Commit b1553ab12fe0 converted virtfs-proxy-helper to using libcap-ng. There
aren't any users of libcap anymore. No need to install libcap-dev.

Signed-off-by: Greg Kurz 
---

Yet another follow-up to Paolo's patch to use libcap-ng instead of libcap.
Like with the docker and the gitlab CI patches, if I get an ack from Alex
I'll gladly merge this in the 9p tree and send a PR as soon as 5.0 dev
begins. I'll make sure the SHA1 for Paolo's patch remains the same.

 .travis.yml |1 -
 1 file changed, 1 deletion(-)

diff --git a/.travis.yml b/.travis.yml
index 445b0646c18a..6cb8af6fa599 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -26,7 +26,6 @@ addons:
   - libaio-dev
   - libattr1-dev
   - libbrlapi-dev
-  - libcap-dev
   - libcap-ng-dev
   - libgcc-4.8-dev
   - libgnutls28-dev

[PATCH] Revert "qemu-options.hx: Update for reboot-timeout parameter"

[Han Han]

This reverts commit bbd9e6985ff342cbe15b9cb7eb30e842796fbbe8.

In 20a1922032 we allowed reboot-timeout=-1 again, so update the doc
accordingly.
---
 qemu-options.hx | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 65c9473b73..e14d88e9b2 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -327,8 +327,8 @@ format(true color). The resolution should be supported by 
the SVGA mode, so
 the recommended is 320x240, 640x480, 800x640.
 
 A timeout could be passed to bios, guest will pause for @var{rb_timeout} ms
-when boot failed, then reboot. If @option{reboot-timeout} is not set,
-guest will not reboot by default. Currently Seabios for X86
+when boot failed, then reboot. If @var{rb_timeout} is '-1', guest will not
+reboot, qemu passes '-1' to bios by default. Currently Seabios for X86
 system support it.
 
 Do strict boot via @option{strict=on} as far as firmware/BIOS
-- 
2.24.0.rc1

[Bug 1846427] Re: 4.1.0: qcow2 corruption on savevm/quit/loadvm cycle

[Matti Hameister]

I was unable to compile the qemu-git package and I currently have not
time to investigate that. But I updated to 4.1.1. I just started my
Windows 10 VM with that and after a short time of use the image was
corrupted again. Here is my full start parameter set. Maybe there is
something wrong or I should change something?

qemu-system-x86_64 -cpu Haswell-noTSX -M q35 -enable-kvm -smp
4,cores=4,threads=1,sockets=1 -net nic,model=virtio -net
user,hostname=WindowsKVM.local -drive
if=none,id=hd,file=hdd.qcow2,discard=unmap -device virtio-scsi-
pci,id=scsi --enable-kvm -device scsi-hd,drive=hd -m 4096 -drive
if=pflash,format=raw,readonly,file=/usr/share/ovmf/x64/OVMF_CODE.fd
-drive if=pflash,format=raw,file=./OVMF_VARS.fd -drive
file=Windows10ISO/Windows.iso,index=0,media=cdrom -drive file=virtio-
win-0.1.173.iso,index=1,media=cdrom -no-quit

My Linux VM is still fine.

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

Title:
  4.1.0: qcow2 corruption on savevm/quit/loadvm cycle

Status in QEMU:
  Fix Committed

Bug description:
  I'm seeing massive corruption of qcow2 images with qemu 4.1.0 and git
  master as of 7f21573c822805a8e6be379d9bcf3ad9effef3dc after a few
  savevm/quit/loadvm cycles. I've narrowed it down to the following
  reproducer (further notes below):

  # qemu-img check debian.qcow2
  No errors were found on the image.
  251601/327680 = 76.78% allocated, 1.63% fragmented, 0.00% compressed clusters
  Image end offset: 18340446208
  # bin/qemu/bin/qemu-system-x86_64 -machine pc-q35-4.0.1,accel=kvm -m 4096 
-chardev stdio,id=charmonitor -mon chardev=charmonitor -drive 
file=debian.qcow2,id=d -S
  qemu-system-x86_64: warning: dbind: Couldn't register with accessibility bus: 
Did not receive a reply. Possible causes include: the remote application did 
not send a reply, the message bus security policy blocked the reply, the reply 
timeout expired, or the network connection was broken.
  QEMU 4.1.50 monitor - type 'help' for more information
  (qemu) loadvm foo
  (qemu) c
  (qemu) qcow2_free_clusters failed: Invalid argument
  qcow2_free_clusters failed: Invalid argument
  qcow2_free_clusters failed: Invalid argument
  qcow2_free_clusters failed: Invalid argument
  quit
  [m@nargothrond:~] qemu-img check debian.qcow2
  Leaked cluster 85179 refcount=2 reference=1
  Leaked cluster 85180 refcount=2 reference=1
  ERROR cluster 266150 refcount=0 reference=2
  [...]
  ERROR OFLAG_COPIED data cluster: l2_entry=42284 refcount=1

  9493 errors were found on the image.
  Data may be corrupted, or further writes to the image may corrupt it.

  2 leaked clusters were found on the image.
  This means waste of disk space, but no harm to data.
  259266/327680 = 79.12% allocated, 1.67% fragmented, 0.00% compressed clusters
  Image end offset: 18340446208

  This is on a x86_64 Linux 5.3.1 Gentoo host with qemu-system-x86_64
  and accel=kvm. The compiler is gcc-9.2.0 with the rest of the system
  similarly current.

  Reproduced with qemu-4.1.0 from distribution package as well as
  vanilla git checkout of tag v4.1.0 and commit
  7f21573c822805a8e6be379d9bcf3ad9effef3dc (today's master). Does not
  happen with qemu compiled from vanilla checkout of tag v4.0.0. Build
  sequence:

  ./configure --prefix=$HOME/bin/qemu-bisect --target-list=x86_64-softmmu 
--disable-werror --disable-docs
  [...]
  CFLAGS-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g
  [...] (can provide full configure output if helpful)
  make -j8 install

  The kind of guest OS does not matter: seen with Debian testing 64bit,
  Windows 7 x86/x64 BIOS and Windows 7 x64 EFI.

  The virtual storage controller does not seem to matter: seen with
  VirtIO SCSI, emulated SCSI and emulated SATA AHCI.

  Caching modes (none, directsync, writeback), aio mode (threads,
  native) or discard (ignore, unmap) or detect-zeroes (off, unmap) does
  not influence occurence either.

  Having more RAM in the guest seems to increase odds of corruption:
  With 512MB to the Debian guest problem hardly occurs at all, with 4GB
  RAM it happens almost instantly.

  An automated reproducer works as follows:

  - the guest *does* mount its root fs and swap with option discard and
  my testing leaves me with the impression that file deletion rather
  than reading is causing the issue

  - foo is a snapshot of the running Debian VM which is already running
  command

  # while true ; do dd if=/dev/zero of=foo bs=10240k count=400 ; done

  to produce some I/O to the disk (4GB file with 4GB of RAM).

  - on the host a loop continuously resumes and saves the guest state
  and quits qemu inbetween:

  # while true ; do (echo loadvm foo ; echo c ; sleep 10 ; echo stop ;
  echo savevm foo ; echo quit ) | bin/qemu-bisect/bin/qemu-system-x86_64
  -machine pc-q35-3.1,accel=kvm -m 4096 -chardev stdio,id=charmonitor
  -mon chardev=charmonitor -drive file=debian.qcow2,id=d -S 

[PATCH v2 02/18] crypto: Fix typo in QCryptoTLSSession's comment

[Markus Armbruster]

Cc: "Daniel P. Berrangé" 
Signed-off-by: Markus Armbruster 
---
 include/crypto/tlssession.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
index e01e1a9dc2..15b9cef086 100644
--- a/include/crypto/tlssession.h
+++ b/include/crypto/tlssession.h
@@ -56,7 +56,7 @@
  *
  * static int mysock_run_tls(int sockfd,
  *   QCryptoTLSCreds *creds,
- *   Error *errp)
+ *   Error **errp)
  * {
  *QCryptoTLSSession *sess;
  *
-- 
2.21.0

[PATCH v2 17/18] hw/intc/s390: Simplify error handling in kvm_s390_flic_realize()

[Markus Armbruster]

Cc: Halil Pasic 
Cc: Cornelia Huck 
Cc: Christian Borntraeger 
Signed-off-by: Markus Armbruster 
Reviewed-by: Cornelia Huck 
Acked-by: Halil Pasic 
---
 hw/intc/s390_flic_kvm.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index 30d50c2369..33ea61 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -586,16 +586,17 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error 
**errp)
 
 KVM_S390_FLIC_GET_CLASS(dev)->parent_realize(dev, );
 if (err) {
-goto fail;
+error_propagate(errp, err);
+return;
 }
 flic_state->fd = -1;
 
 cd.type = KVM_DEV_TYPE_FLIC;
 ret = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, );
 if (ret < 0) {
-error_setg_errno(, errno, "Creating the KVM device failed");
+error_setg_errno(errp, errno, "Creating the KVM device failed");
 trace_flic_create_device(errno);
-goto fail;
+return;
 }
 flic_state->fd = cd.fd;
 
@@ -603,9 +604,6 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error 
**errp)
 test_attr.group = KVM_DEV_FLIC_CLEAR_IO_IRQ;
 flic_state->clear_io_supported = !ioctl(flic_state->fd,
 KVM_HAS_DEVICE_ATTR, test_attr);
-return;
-fail:
-error_propagate(errp, err);
 }
 
 static void kvm_s390_flic_reset(DeviceState *dev)
-- 
2.21.0

[PATCH v2 15/18] s390x/cpumodel: Fix query-cpu-definitions error API violations

[Markus Armbruster]

qmp_query_cpu_definitions() passes @errp to get_max_cpu_model(), then
frees any error it gets back.  This effectively ignores errors.
Dereferencing @errp is wrong; see the big comment in error.h.  Passing
@errp is also wrong, because it works only as long as @errp is neither
@error_fatal nor @error_abort.  Introduced in commit 38cba1f4d8
"s390x: return unavailable features via query-cpu-definitions".

No caller actually passes such @errp values.

Fix anyway: simply pass NULL to get_max_cpu_model().

Cc: David Hildenbrand 
Cc: Cornelia Huck 
Signed-off-by: Markus Armbruster 
Reviewed-by: David Hildenbrand 
---
 target/s390x/cpu_models.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 3ed301b5e5..547bab8ac3 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -462,11 +462,7 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error 
**errp)
 .list = NULL,
 };
 
-list_data.model = get_max_cpu_model(errp);
-if (*errp) {
-error_free(*errp);
-*errp = NULL;
-}
+list_data.model = get_max_cpu_model(NULL);
 
 object_class_foreach(create_cpu_model_list, TYPE_S390_CPU, false,
  _data);
-- 
2.21.0

[PATCH] i386: pass CLZERO to guests with EPYC CPU model on AMD ZEN platform

[Ani Sinha]

CLZERO CPUID should be passed on to the guests that use EPYC or EPYC-IBPB CPU
model when the AMD ZEN based host supports it. This change makes it recognize
this CPUID for guests which use EPYC or EPYC-IBPB CPU model.

Signed-off-by: Ani Sinha 
---
 target/i386/cpu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 69f518a..55f0691 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3813,6 +3813,8 @@ static X86CPUDefinition builtin_x86_defs[] = {
 CPUID_EXT3_MISALIGNSSE | CPUID_EXT3_SSE4A | CPUID_EXT3_ABM |
 CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM |
 CPUID_EXT3_TOPOEXT,
+.features[FEAT_8000_0008_EBX] =
+CPUID_8000_0008_EBX_CLZERO,
 .features[FEAT_7_0_EBX] =
 CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_AVX2 |
 CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_RDSEED |
-- 
1.9.4

Re: [PULL v2 4/6] spapr: Add /chosen to FDT only at reset time to preserve kernel and initramdisk

[Laurent Vivier]

On 04/12/2019 05:40, Alexey Kardashevskiy wrote:
> 
> 
> On 04/12/2019 15:23, Alexey Kardashevskiy wrote:
>>
>>
>> On 04/12/2019 03:09, Laurent Vivier wrote:
>>>
>>> Bad reply, the problem is with
>>>
>>> "spapr: Render full FDT on ibm,client-architecture-support"
>>
>>
>> https://git.qemu.org/?p=SLOF.git;a=blob;f=board-qemu/slof/fdt.fs;h=3e4c1b34b8af2dcebde57e548c94417e5e20e1cc;hb=HEAD#l265
>>
>> A "bit ugly" became really ugly as before we were only patching
>> interrupt-map for PHB (7 cells per line) only but now we have to patch
>> (or, rather, skip) the PCI bridge interrupt-map (9 cells per line).
>>
>> Fixing now...
> 
> 
> Basically, this:
> 
> 
> diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs
> index 3e4c1b34b8af..463a2a8c0c2d 100644
> --- a/board-qemu/slof/fdt.fs
> +++ b/board-qemu/slof/fdt.fs
> @@ -300,8 +300,13 @@ fdt-claim-reserve
> \ ." Replacing in " dup node>path type cr
> >r
> s" interrupt-map" r@ get-property 0= IF
> -  ( old new prop-addr prop-len  R: node )
> -  fdt-replace-interrupt-map
> +  dup e00 = IF
> +  ( old new prop-addr prop-len  R: node )
> +  fdt-replace-interrupt-map
> +  ELSE
> + 2drop
> +  ."  no idea what this is" cr
> +  THEN
> THEN

This does not fix the problem for me.

Thanks,
Laurent

[ANNOUNCE] libslirp 4.1.0 is now available

[Marc-André Lureau]

Hi,

We are pleased to announce the availability of libslirp 4.1.0
release.

You can grab the tarball & read the changelog from the gitlab release page here:

https://gitlab.freedesktop.org/slirp/libslirp/releases

Highlights include:

 * new slirp_new() API, more flexible than slirp_init()
 * various new options needed for slirp4netns
 * tcp_emu() is now disabled by default. tcp_emu() is known to have
caused several CVEs, and not useful today in most cases. The feature
can be still enabled by setting SlirpConfig.enable_emu to true.
 * bug fixes & cleanups

Once the library installed, you may compile QEMU >= 4.0 with
--enable-slirp=system to link against it.

podman/slirp4netns should link with the system library soon
(https://github.com/rootless-containers/slirp4netns/pull/162)

Thank you to everyone involved!

-- 
Marc-André Lureau

Re: [PATCH v4 10/40] target/arm: Rename ARMMMUIdx_S1NSE* to ARMMMUIdx_Stage1_E*

[Alex Bennée]

Richard Henderson  writes:

> This is part of a reorganization to the set of mmu_idx.
> The EL1&0 regime is the only one that uses 2-stage translation.
> Spelling out Stage avoids confusion with Secure.
>
> Signed-off-by: Richard Henderson 

> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 97677f8482..a34accec20 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2992,7 +2992,8 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t 
> value,
>  bool take_exc = false;
>  
>  if (fi.s1ptw && current_el == 1 && !arm_is_secure(env)
> -&& (mmu_idx == ARMMMUIdx_S1NSE1 || mmu_idx == ARMMMUIdx_S1NSE0)) 
> {
> +&& (mmu_idx == ARMMMUIdx_Stage1_E1
> +|| mmu_idx == ARMMMUIdx_Stage1_E0)) {

Personal nit: I think ||\nfoo == scans more nicely as it lines up but
maybe that's just me.

Reviewed-by: Alex Bennée 

-- 
Alex Bennée

Re: [PATCH 00/21] Error handling fixes, may contain 4.2 material

[Markus Armbruster]

"Michael S. Tsirkin"  writes:

> On Sat, Nov 30, 2019 at 08:42:19PM +0100, Markus Armbruster wrote:
>> PATCH 2-4 fix crash bugs.  Including them would be a no-brainer at
>> -rc0.  But we're post -rc3, and even for crash bugs we require a
>> certain likelihood of users getting bitten.
>> 
>> Jens, please assess impact of PATCH 2's crash bug.
>> 
>> Kevin, please do the same for PATCH 3.
>> 
>> Daniel, please do the same for PATCH 4.
>
> virtio things:
>
> Reviewed-by: Michael S. Tsirkin 

In my haste to get this into -rc4, I lost your r-bys.  Sorry about that!

> Jason do you want to pick these?

Merged in commit 39032981fa851d25fb27527f25f046fed800e585.

Re: [PATCH] travis.yml: Drop libcap-dev

[Philippe Mathieu-Daudé]

On 12/4/19 9:01 AM, Greg Kurz wrote:

Commit b1553ab12fe0 converted virtfs-proxy-helper to using libcap-ng. There
aren't any users of libcap anymore. No need to install libcap-dev.

Signed-off-by: Greg Kurz 
---

Yet another follow-up to Paolo's patch to use libcap-ng instead of libcap.
Like with the docker and the gitlab CI patches, if I get an ack from Alex
I'll gladly merge this in the 9p tree and send a PR as soon as 5.0 dev
begins. I'll make sure the SHA1 for Paolo's patch remains the same.

  .travis.yml |1 -
  1 file changed, 1 deletion(-)

diff --git a/.travis.yml b/.travis.yml
index 445b0646c18a..6cb8af6fa599 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -26,7 +26,6 @@ addons:
- libaio-dev
- libattr1-dev
- libbrlapi-dev
-  - libcap-dev
- libcap-ng-dev
- libgcc-4.8-dev
- libgnutls28-dev




Reviewed-by: Philippe Mathieu-Daudé 

[PATCH v2 13/18] s390x/cpumodel: Fix realize() error API violations

[Markus Armbruster]

get_max_cpu_model() dereferences @errp when
kvm_s390_get_host_cpu_model() fails, apply_cpu_model() dereferences it
when kvm_s390_apply_cpu_model() fails, and s390_realize_cpu_model()
dereferences it when get_max_cpu_model() or check_compatibility()
fail.  That's wrong; see the big comment in error.h.  All three
introduced in commit 80560137cf "s390x/cpumodel: check and apply the
CPU model".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: David Hildenbrand 
Cc: Cornelia Huck 
Signed-off-by: Markus Armbruster 
Reviewed-by: David Hildenbrand 
---
 target/s390x/cpu_models.c | 37 ++---
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 6a29fd3ab1..c702e34a26 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -870,6 +870,7 @@ static void check_compatibility(const S390CPUModel 
*max_model,
 
 static S390CPUModel *get_max_cpu_model(Error **errp)
 {
+Error *err = NULL;
 static S390CPUModel max_model;
 static bool cached;
 
@@ -878,22 +879,24 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
 }
 
 if (kvm_enabled()) {
-kvm_s390_get_host_cpu_model(_model, errp);
+kvm_s390_get_host_cpu_model(_model, );
 } else {
 max_model.def = s390_find_cpu_def(QEMU_MAX_CPU_TYPE, QEMU_MAX_CPU_GEN,
   QEMU_MAX_CPU_EC_GA, NULL);
 bitmap_copy(max_model.features, qemu_max_cpu_feat, S390_FEAT_MAX);
-   }
-if (!*errp) {
-cached = true;
-return _model;
 }
-return NULL;
+if (err) {
+error_propagate(errp, err);
+return NULL;
+}
+cached = true;
+return _model;
 }
 
 static inline void apply_cpu_model(const S390CPUModel *model, Error **errp)
 {
 #ifndef CONFIG_USER_ONLY
+Error *err = NULL;
 static S390CPUModel applied_model;
 static bool applied;
 
@@ -909,20 +912,23 @@ static inline void apply_cpu_model(const S390CPUModel 
*model, Error **errp)
 }
 
 if (kvm_enabled()) {
-kvm_s390_apply_cpu_model(model, errp);
+kvm_s390_apply_cpu_model(model, );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 }
 
-if (!*errp) {
-applied = true;
-if (model) {
-applied_model = *model;
-}
+applied = true;
+if (model) {
+applied_model = *model;
 }
 #endif
 }
 
 void s390_realize_cpu_model(CPUState *cs, Error **errp)
 {
+Error *err = NULL;
 S390CPUClass *xcc = S390_CPU_GET_CLASS(cs);
 S390CPU *cpu = S390_CPU(cs);
 const S390CPUModel *max_model;
@@ -939,7 +945,7 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
 }
 
 max_model = get_max_cpu_model(errp);
-if (*errp) {
+if (!max_model) {
 error_prepend(errp, "CPU models are not available: ");
 return;
 }
@@ -951,8 +957,9 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
 cpu->model->cpu_ver = max_model->cpu_ver;
 
 check_consistency(cpu->model);
-check_compatibility(max_model, cpu->model, errp);
-if (*errp) {
+check_compatibility(max_model, cpu->model, );
+if (err) {
+error_propagate(errp, err);
 return;
 }
 
-- 
2.21.0

[PATCH v2 18/18] tests-blockjob: Use error_free_or_abort()

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 tests/test-blockjob.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index e670a20617..4eeb184caf 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -48,9 +48,8 @@ static BlockJob *mk_job(BlockBackend *blk, const char *id,
 g_assert_cmpstr(job->job.id, ==, blk_name(blk));
 }
 } else {
-g_assert_nonnull(err);
+error_free_or_abort();
 g_assert_null(job);
-error_free(err);
 }
 
 return job;
-- 
2.21.0

Re: [PATCH v2 03/18] io: Fix Error usage in a comment

[Daniel P . Berrangé]

On Wed, Dec 04, 2019 at 10:36:10AM +0100, Markus Armbruster wrote:
> Cc: "Daniel P. Berrangé" 
> Signed-off-by: Markus Armbruster 
> ---
>  include/io/task.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Daniel P. Berrangé 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v4 08/40] target/arm: Rename ARMMMUIdx*_S12NSE* to ARMMMUIdx*_E10_*

[Alex Bennée]

Richard Henderson  writes:

> This is part of a reorganization to the set of mmu_idx.
> This emphasizes that they apply to the EL1&0 regime.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  target/arm/cpu.h   |  8 
>  target/arm/internals.h |  4 ++--
>  target/arm/helper.c| 40 +++---
>  target/arm/translate-a64.c |  4 ++--
>  target/arm/translate.c |  6 +++---
>  5 files changed, 31 insertions(+), 31 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 9729e62d2c..802cddd2df 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2864,8 +2864,8 @@ static inline bool arm_excp_unmasked(CPUState *cs, 
> unsigned int excp_idx,
>  #define ARM_MMU_IDX_COREIDX_MASK 0x7
>  
>  typedef enum ARMMMUIdx {
> -ARMMMUIdx_S12NSE0 = 0 | ARM_MMU_IDX_A,
> -ARMMMUIdx_S12NSE1 = 1 | ARM_MMU_IDX_A,
> +ARMMMUIdx_EL10_0 = 0 | ARM_MMU_IDX_A,
> +ARMMMUIdx_EL10_1 = 1 | ARM_MMU_IDX_A,
>  ARMMMUIdx_S1E2 = 2 | ARM_MMU_IDX_A,
>  ARMMMUIdx_S1E3 = 3 | ARM_MMU_IDX_A,
>  ARMMMUIdx_S1SE0 = 4 | ARM_MMU_IDX_A,
> @@ -2890,8 +2890,8 @@ typedef enum ARMMMUIdx {
>   * for use when calling tlb_flush_by_mmuidx() and friends.
>   */
>  typedef enum ARMMMUIdxBit {
> -ARMMMUIdxBit_S12NSE0 = 1 << 0,
> -ARMMMUIdxBit_S12NSE1 = 1 << 1,
> +ARMMMUIdxBit_EL10_0 = 1 << 0,
> +ARMMMUIdxBit_EL10_1 = 1 << 1,
>  ARMMMUIdxBit_S1E2 = 1 << 2,
>  ARMMMUIdxBit_S1E3 = 1 << 3,
>  ARMMMUIdxBit_S1SE0 = 1 << 4,
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index f5313dd3d4..54142dd789 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -808,8 +808,8 @@ static inline void arm_call_el_change_hook(ARMCPU *cpu)
>  static inline bool regime_is_secure(CPUARMState *env, ARMMMUIdx mmu_idx)
>  {
>  switch (mmu_idx) {
> -case ARMMMUIdx_S12NSE0:
> -case ARMMMUIdx_S12NSE1:
> +case ARMMMUIdx_EL10_0:
> +case ARMMMUIdx_EL10_1:
>  case ARMMMUIdx_S1NSE0:
>  case ARMMMUIdx_S1NSE1:
>  case ARMMMUIdx_S1E2:
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 6c09cda4ea..d2b90763ca 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -670,8 +670,8 @@ static void tlbiall_nsnh_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  CPUState *cs = env_cpu(env);
>  
>  tlb_flush_by_mmuidx(cs,
> -ARMMMUIdxBit_S12NSE1 |
> -ARMMMUIdxBit_S12NSE0 |
> +ARMMMUIdxBit_EL10_1 |
> +ARMMMUIdxBit_EL10_0 |
>  ARMMMUIdxBit_S2NS);
>  }
>  
> @@ -681,8 +681,8 @@ static void tlbiall_nsnh_is_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  CPUState *cs = env_cpu(env);
>  
>  tlb_flush_by_mmuidx_all_cpus_synced(cs,
> -ARMMMUIdxBit_S12NSE1 |
> -ARMMMUIdxBit_S12NSE0 |
> +ARMMMUIdxBit_EL10_1 |
> +ARMMMUIdxBit_EL10_0 |
>  ARMMMUIdxBit_S2NS);
>  }
>  
> @@ -3068,7 +3068,7 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t 
> value,
>  format64 = arm_s1_regime_using_lpae_format(env, mmu_idx);
>  
>  if (arm_feature(env, ARM_FEATURE_EL2)) {
> -if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == 
> ARMMMUIdx_S12NSE1) {
> +if (mmu_idx == ARMMMUIdx_EL10_0 || mmu_idx == ARMMMUIdx_EL10_1) {
>  format64 |= env->cp15.hcr_el2 & (HCR_VM | HCR_DC);
>  } else {
>  format64 |= arm_current_el(env) == 2;
> @@ -3167,11 +3167,11 @@ static void ats_write(CPUARMState *env, const 
> ARMCPRegInfo *ri, uint64_t value)
>  break;
>  case 4:
>  /* stage 1+2 NonSecure PL1: ATS12NSOPR, ATS12NSOPW */
> -mmu_idx = ARMMMUIdx_S12NSE1;
> +mmu_idx = ARMMMUIdx_EL10_1;
>  break;
>  case 6:
>  /* stage 1+2 NonSecure PL0: ATS12NSOUR, ATS12NSOUW */
> -mmu_idx = ARMMMUIdx_S12NSE0;
> +mmu_idx = ARMMMUIdx_EL10_0;
>  break;
>  default:
>  g_assert_not_reached();
> @@ -3229,10 +3229,10 @@ static void ats_write64(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  mmu_idx = secure ? ARMMMUIdx_S1SE0 : ARMMMUIdx_S1NSE0;
>  break;
>  case 4: /* AT S12E1R, AT S12E1W */
> -mmu_idx = secure ? ARMMMUIdx_S1SE1 : ARMMMUIdx_S12NSE1;
> +mmu_idx = secure ? ARMMMUIdx_S1SE1 : ARMMMUIdx_EL10_1;
>  break;
>  case 6: /* AT S12E0R, AT S12E0W */
> -mmu_idx = secure ? ARMMMUIdx_S1SE0 : ARMMMUIdx_S12NSE0;
> +mmu_idx = secure ? ARMMMUIdx_S1SE0 : ARMMMUIdx_EL10_0;
>  break;
>  default:
>  g_assert_not_reached();
> @@ -3531,8 +3531,8 @@ static void vttbr_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  /* Accesses to VTTBR 

Re: [PATCH v2 3/3] virtio-serial-bus: fix memory leak while attach virtio-serial-bus

[Laurent Vivier]

On 04/12/2019 08:31, pannengy...@huawei.com wrote:
> From: Pan Nengyuan 
> 
> ivqs/ovqs/c_ivq/c_ovq is forgot to cleanup in
> virtio_serial_device_unrealize, the memory leak stack is as bellow:
> 
> Direct leak of 1290240 byte(s) in 180 object(s) allocated from:
> #0 0x7fc9bfc27560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
> #1 0x7fc9bed6f015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
> #2 0x5650e02b83e7 in virtio_add_queue hw/virtio/virtio.c:2327
> #3 0x5650e02847b5 in virtio_serial_device_realize 
> hw/char/virtio-serial-bus.c:1089
> #4 0x5650e02b56a7 in virtio_device_realize hw/virtio/virtio.c:3504
> #5 0x5650e03bf031 in device_set_realized hw/core/qdev.c:876
> #6 0x5650e0531efd in property_set_bool qom/object.c:2080
> #7 0x5650e053650e in object_property_set_qobject qom/qom-qobject.c:26
> #8 0x5650e0533e14 in object_property_set_bool qom/object.c:1338
> #9 0x5650e04c0e37 in virtio_pci_realize hw/virtio/virtio-pci.c:1801
> 
> Reported-by: Euler Robot 
> Signed-off-by: Pan Nengyuan 
> Cc: Laurent Vivier 
> Cc: Amit Shah 
> Cc: "Marc-André Lureau" 
> Cc: Paolo Bonzini 
> ---
> Changes v2 to v1:
> - use virtio_delete_queue to cleanup vq through a vq pointer (suggested by
>   Michael S. Tsirkin)
> ---
>  hw/char/virtio-serial-bus.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 3325904..e1cbce3 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -1126,9 +1126,17 @@ static void virtio_serial_device_unrealize(DeviceState 
> *dev, Error **errp)
>  {
>  VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>  VirtIOSerial *vser = VIRTIO_SERIAL(dev);
> +int i;
>  
>  QLIST_REMOVE(vser, next);
>  
> +virtio_delete_queue(vser->c_ivq);
> +virtio_delete_queue(vser->c_ovq);
> +for (i = 0; i < vser->bus.max_nr_ports; i++) {
> +virtio_delete_queue(vser->ivqs[i]);
> +virtio_delete_queue(vser->ovqs[i]);
> +}
> +
>  g_free(vser->ivqs);
>  g_free(vser->ovqs);
>  g_free(vser->ports_map);
> 

Reviewed-by: Laurent Vivier 

Re: virtiofsd: Where should it live?

[Gerd Hoffmann]

  Hi,

> > |   ...
> > +- qemu-edid
> 
> Has its own MAINTAINERS section, together with hw/display/edit* and
> include/hw/display/edid.h.  I'm not sure moving it hw/display/ is a good
> idea.  Gerd?

Sort-of makes sense.  My personal preference would be a tools/ directory
for all those small utilities though.

> > +- qemu-keymap
> 
> Not covered by MAINTAINERS.  scripts/get_maintainer.pl --git-blame
> points to Gerd.

Generates the keymaps in pc-bios/keymaps/

cheers,
  Gerd

[PATCH v2 12/18] s390x/cpumodel: Fix feature property error API violations

[Markus Armbruster]

s390x-cpu property setters set_feature() and set_feature_group()
dereference @errp when the visitor fails.  That's wrong; see the big
comment in error.h.  Introduced in commit 0754f60429 "s390x/cpumodel:
expose features and feature groups as properties".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: David Hildenbrand 
Cc: Cornelia Huck 
Signed-off-by: Markus Armbruster 
Reviewed-by: David Hildenbrand 
---
 target/s390x/cpu_models.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 7e92fb2e15..6a29fd3ab1 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -987,6 +987,7 @@ static void get_feature(Object *obj, Visitor *v, const char 
*name,
 static void set_feature(Object *obj, Visitor *v, const char *name,
 void *opaque, Error **errp)
 {
+Error *err = NULL;
 S390Feat feat = (S390Feat) opaque;
 DeviceState *dev = DEVICE(obj);
 S390CPU *cpu = S390_CPU(obj);
@@ -1002,8 +1003,9 @@ static void set_feature(Object *obj, Visitor *v, const 
char *name,
 return;
 }
 
-visit_type_bool(v, name, , errp);
-if (*errp) {
+visit_type_bool(v, name, , );
+if (err) {
+error_propagate(errp, err);
 return;
 }
 if (value) {
@@ -1043,6 +1045,7 @@ static void get_feature_group(Object *obj, Visitor *v, 
const char *name,
 static void set_feature_group(Object *obj, Visitor *v, const char *name,
   void *opaque, Error **errp)
 {
+Error *err = NULL;
 S390FeatGroup group = (S390FeatGroup) opaque;
 const S390FeatGroupDef *def = s390_feat_group_def(group);
 DeviceState *dev = DEVICE(obj);
@@ -1059,8 +1062,9 @@ static void set_feature_group(Object *obj, Visitor *v, 
const char *name,
 return;
 }
 
-visit_type_bool(v, name, , errp);
-if (*errp) {
+visit_type_bool(v, name, , );
+if (err) {
+error_propagate(errp, err);
 return;
 }
 if (value) {
-- 
2.21.0

Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[Laurent Vivier]

On 04/12/2019 08:31, pannengy...@huawei.com wrote:
> From: Pan Nengyuan 
> 
> Devices tend to maintain vq pointers, allow deleting them trough a vq pointer.
> 
> Signed-off-by: Michael S. Tsirkin 
> Signed-off-by: Pan Nengyuan 
> ---
> Changes v2 to v1:
> - add a new function virtio_delete_queue to cleanup vq through a vq pointer
> ---
>  hw/virtio/virtio.c | 16 +++-
>  include/hw/virtio/virtio.h |  2 ++
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 04716b5..6de3cfd 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int 
> queue_size,
>  return >vq[i];
>  }
>  
> +void virtio_delete_queue(VirtQueue *vq)
> +{
> +vq->vring.num = 0;
> +vq->vring.num_default = 0;
> +vq->handle_output = NULL;
> +vq->handle_aio_output = NULL;
> +g_free(vq->used_elems);
> +vq->used_elems = NULL;
> +}
> +
>  void virtio_del_queue(VirtIODevice *vdev, int n)
>  {
>  if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
>  abort();
>  }
>  
> -vdev->vq[n].vring.num = 0;
> -vdev->vq[n].vring.num_default = 0;
> -vdev->vq[n].handle_output = NULL;
> -vdev->vq[n].handle_aio_output = NULL;
> -g_free(vdev->vq[n].used_elems);
> +virtio_delete_queue(>vq[n]);
>  }
>  
>  static void virtio_set_isr(VirtIODevice *vdev, int value)
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index c32a815..e18756d 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int 
> queue_size,
>  
>  void virtio_del_queue(VirtIODevice *vdev, int n);
>  
> +void virtio_delete_queue(VirtQueue *vq);
> +
>  void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
>  unsigned int len);
>  void virtqueue_flush(VirtQueue *vq, unsigned int count);
> 

Reviewed-by: Laurent Vivier 

[PATCH v2 11/18] s390x/event-facility: Fix realize() error API violations

[Markus Armbruster]

sclp_events_bus_realize() dereferences @errp when
object_property_set_bool() fails.  That's wrong; see the big comment
in error.h.  Introduced in commit f6102c329c "s390/sclp: rework sclp
event facility initialization + device realization".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: David Hildenbrand 
Cc: Cornelia Huck 
Signed-off-by: Markus Armbruster 
Reviewed-by: David Hildenbrand 
---
 hw/s390x/event-facility.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index 66205697ae..cdcf9154c4 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -339,14 +339,16 @@ out:
 
 static void sclp_events_bus_realize(BusState *bus, Error **errp)
 {
+Error *err = NULL;
 BusChild *kid;
 
 /* TODO: recursive realization has to be done in common code */
 QTAILQ_FOREACH(kid, >children, sibling) {
 DeviceState *dev = kid->child;
 
-object_property_set_bool(OBJECT(dev), true, "realized", errp);
-if (*errp) {
+object_property_set_bool(OBJECT(dev), true, "realized", );
+if (errp) {
+error_propagate(errp, err);
 return;
 }
 }
-- 
2.21.0

Re: [PATCH v4 09/40] target/arm: Rename ARMMMUIdx_S2NS to ARMMMUIdx_Stage2

[Alex Bennée]

Richard Henderson  writes:

> The EL1&0 regime is the only one that uses 2-stage translation.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  target/arm/cpu.h   |  4 +--
>  target/arm/internals.h |  2 +-
>  target/arm/helper.c| 57 --
>  target/arm/translate-a64.c |  2 +-
>  target/arm/translate.c |  2 +-
>  5 files changed, 35 insertions(+), 32 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 802cddd2df..fdb868f2e9 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2870,7 +2870,7 @@ typedef enum ARMMMUIdx {
>  ARMMMUIdx_S1E3 = 3 | ARM_MMU_IDX_A,
>  ARMMMUIdx_S1SE0 = 4 | ARM_MMU_IDX_A,
>  ARMMMUIdx_S1SE1 = 5 | ARM_MMU_IDX_A,
> -ARMMMUIdx_S2NS = 6 | ARM_MMU_IDX_A,
> +ARMMMUIdx_Stage2 = 6 | ARM_MMU_IDX_A,
>  ARMMMUIdx_MUser = 0 | ARM_MMU_IDX_M,
>  ARMMMUIdx_MPriv = 1 | ARM_MMU_IDX_M,
>  ARMMMUIdx_MUserNegPri = 2 | ARM_MMU_IDX_M,
> @@ -2896,7 +2896,7 @@ typedef enum ARMMMUIdxBit {
>  ARMMMUIdxBit_S1E3 = 1 << 3,
>  ARMMMUIdxBit_S1SE0 = 1 << 4,
>  ARMMMUIdxBit_S1SE1 = 1 << 5,
> -ARMMMUIdxBit_S2NS = 1 << 6,
> +ARMMMUIdxBit_Stage2 = 1 << 6,
>  ARMMMUIdxBit_MUser = 1 << 0,
>  ARMMMUIdxBit_MPriv = 1 << 1,
>  ARMMMUIdxBit_MUserNegPri = 1 << 2,
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 54142dd789..ca8be78bbf 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -813,7 +813,7 @@ static inline bool regime_is_secure(CPUARMState *env, 
> ARMMMUIdx mmu_idx)
>  case ARMMMUIdx_S1NSE0:
>  case ARMMMUIdx_S1NSE1:
>  case ARMMMUIdx_S1E2:
> -case ARMMMUIdx_S2NS:
> +case ARMMMUIdx_Stage2:
>  case ARMMMUIdx_MPrivNegPri:
>  case ARMMMUIdx_MUserNegPri:
>  case ARMMMUIdx_MPriv:
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index d2b90763ca..97677f8482 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -672,7 +672,7 @@ static void tlbiall_nsnh_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  tlb_flush_by_mmuidx(cs,
>  ARMMMUIdxBit_EL10_1 |
>  ARMMMUIdxBit_EL10_0 |
> -ARMMMUIdxBit_S2NS);
> +ARMMMUIdxBit_Stage2);
>  }
>  
>  static void tlbiall_nsnh_is_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -683,7 +683,7 @@ static void tlbiall_nsnh_is_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  tlb_flush_by_mmuidx_all_cpus_synced(cs,
>  ARMMMUIdxBit_EL10_1 |
>  ARMMMUIdxBit_EL10_0 |
> -ARMMMUIdxBit_S2NS);
> +ARMMMUIdxBit_Stage2);
>  }
>  
>  static void tlbiipas2_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -704,7 +704,7 @@ static void tlbiipas2_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  
>  pageaddr = sextract64(value << 12, 0, 40);
>  
> -tlb_flush_page_by_mmuidx(cs, pageaddr, ARMMMUIdxBit_S2NS);
> +tlb_flush_page_by_mmuidx(cs, pageaddr, ARMMMUIdxBit_Stage2);
>  }
>  
>  static void tlbiipas2_is_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -720,7 +720,7 @@ static void tlbiipas2_is_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  pageaddr = sextract64(value << 12, 0, 40);
>  
>  tlb_flush_page_by_mmuidx_all_cpus_synced(cs, pageaddr,
> - ARMMMUIdxBit_S2NS);
> + ARMMMUIdxBit_Stage2);
>  }
>  
>  static void tlbiall_hyp_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -3528,12 +3528,15 @@ static void vttbr_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  ARMCPU *cpu = env_archcpu(env);
>  CPUState *cs = CPU(cpu);
>  
> -/* Accesses to VTTBR may change the VMID so we must flush the TLB.  */
> +/*
> + * A change in VMID to the stage2 page table (Stage2) invalidates
> + * the combined stage 1&2 tlbs (EL10_1 and EL10_0).
> + */
>  if (raw_read(env, ri) != value) {
>  tlb_flush_by_mmuidx(cs,
>  ARMMMUIdxBit_EL10_1 |
>  ARMMMUIdxBit_EL10_0 |
> -ARMMMUIdxBit_S2NS);
> +ARMMMUIdxBit_Stage2);
>  raw_write(env, ri, value);
>  }
>  }
> @@ -3929,7 +3932,7 @@ static int vmalle1_tlbmask(CPUARMState *env)
>  if (arm_is_secure_below_el3(env)) {
>  return ARMMMUIdxBit_S1SE1 | ARMMMUIdxBit_S1SE0;
>  } else if (arm_feature(env, ARM_FEATURE_EL2)) {
> -return ARMMMUIdxBit_EL10_1 | ARMMMUIdxBit_EL10_0 | ARMMMUIdxBit_S2NS;
> +return ARMMMUIdxBit_EL10_1 | ARMMMUIdxBit_EL10_0 | 
> ARMMMUIdxBit_Stage2;
>  } else {
>  return ARMMMUIdxBit_EL10_1 | ARMMMUIdxBit_EL10_0;
>  }
> @@ -4083,7 +4086,7 @@ static void tlbi_aa64_ipas2e1_write(CPUARMState 

Re: [PATCH] travis.yml: Drop libcap-dev

[Thomas Huth]

On 04/12/2019 09.01, Greg Kurz wrote:
> Commit b1553ab12fe0 converted virtfs-proxy-helper to using libcap-ng. There
> aren't any users of libcap anymore. No need to install libcap-dev.
> 
> Signed-off-by: Greg Kurz 
> ---
> 
> Yet another follow-up to Paolo's patch to use libcap-ng instead of libcap.
> Like with the docker and the gitlab CI patches, if I get an ack from Alex
> I'll gladly merge this in the 9p tree and send a PR as soon as 5.0 dev
> begins. I'll make sure the SHA1 for Paolo's patch remains the same.

I hope you don't have to rebase! Otherwise simply say "One of the
previous changes ..." or so in the commit message instead.

>  .travis.yml |1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 445b0646c18a..6cb8af6fa599 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -26,7 +26,6 @@ addons:
>- libaio-dev
>- libattr1-dev
>- libbrlapi-dev
> -  - libcap-dev
>- libcap-ng-dev
>- libgcc-4.8-dev
>- libgnutls28-dev

Reviewed-by: Thomas Huth 

Re: [RFC] QEMU Gating CI

[Thomas Huth]

On 03/12/2019 15.07, Alex Bennée wrote:
[...]
>> GitLab Jobs and Pipelines
>> -
>>
>> GitLab CI is built around two major concepts: jobs and pipelines.  The
>> current GitLab CI configuration in QEMU uses jobs only (or putting it
>> another way, all jobs in a single pipeline stage).

Yeah, the initial gitlab-ci.yml file was one of the very first YAML file
and one the very first CI files that I wrote, with hardly any experience
in this area ... there is definitely a lot of room for improvement here!

>>  Consider the
>> folowing job definition[9]:
>>
>>build-tci:
>> script:
>> - TARGETS="aarch64 alpha arm hppa m68k microblaze moxie ppc64 s390x 
>> x86_64"
>> - ./configure --enable-tcg-interpreter
>>  --target-list="$(for tg in $TARGETS; do echo -n ${tg}'-softmmu '; 
>> done)"
>> - make -j2
>> - make tests/boot-serial-test tests/cdrom-test tests/pxe-test
>> - for tg in $TARGETS ; do
>> export QTEST_QEMU_BINARY="${tg}-softmmu/qemu-system-${tg}" ;
>> ./tests/boot-serial-test || exit 1 ;
>> ./tests/cdrom-test || exit 1 ;
>>   done
>> - QTEST_QEMU_BINARY="x86_64-softmmu/qemu-system-x86_64" ./tests/pxe-test
>> - QTEST_QEMU_BINARY="s390x-softmmu/qemu-system-s390x" ./tests/pxe-test 
>> -m slow
>>
>> All the lines under "script" are performed sequentially.  It should be
>> clear that there's the possibility of breaking this down into multiple
>> stages, so that a build happens first, and then "common set of tests"
>> run in parallel.  Using the example above, it would look something
>> like:
>>
>>+---++
>>|  BUILD STAGE  |TEST STAGE  |
>>+---++
>>|   +---+   |  +--+  |
>>|   | build |   |  | boot-serial-test |  |
>>|   +---+   |  +--+  |
>>|   ||
>>|   |  +--+  |
>>|   |  | cdrom-test   |  |
>>|   |  +--+  |
>>|   ||
>>|   |  +--+  |
>>|   |  | x86_64-pxe-test  |  |
>>|   |  +--+  |
>>|   ||
>>|   |  +--+  |
>>|   |  | s390x-pxe-test   |  |
>>|   |  +--+  |
>>|   ||
>>+---++
>>
>> Of course it would be silly to break down that job into smaller jobs that
>> would run individual tests like "boot-serial-test" or "cdrom-test".  Still,
>> the pipeline approach is valid because:
>>
>>  * Common set of tests would run in parallel, giving a quicker result
>>turnaround

Ok, full ack for the idea to use separate pipelines for the testing
(Philippe once showed me this idea already, too, he's using it for EDK2
testing IIRC). But the example with the build-tci is quite bad. The
single steps here are basically just a subset of "check-qtest" to skip
the tests that we are not interested in here. If we don't care about
losing some minutes of testing, we can simply replace all those steps
with "make check-qtest" again.

I think what we really want to put into different pipelines are the
sub-steps of "make check", i.e.:

- check-block
- check-qapi-schema
- check-unit
- check-softfloat
- check-qtest
- check-decodetree

And of course also the other ones that are not included in "make check"
yet, e.g. "check-acceptance" etc.

> check-unit is a good candidate for parallel tests. The others depends -
> I've recently turned most make check's back to -j 1 on travis because
> it's a real pain to see what test has hung when other tests keep
> running.

If I understood correctly, it's not about running the check steps in
parallel with "make -jXX" in one pipeline, but rather about running the
different test steps in different pipelines. So you get a separate
output for each test subsystem.

>> Current limitations for a multi-stage pipeline
>> ~~
>>
>> Because it's assumed that each job will happen in an isolated and
>> independent execution environment, jobs must explicitly define the
>> resources that will be shared between stages.  GitLab will make sure
>> the same source code revision will be available on all jobs
>> automatically.  Additionaly, GitLab supports the concept of artifacts.
>> By defining artifacts in the "build" stage, jobs in the "test" stage
>> can expect to have a copy of those artifacts automatically.
>>
>> In theory, there's nothing that prevents an entire QEMU build
>> directory, to be treated as an artifact.  In practice, there are
>> predefined limits on GitLab that prevents that from being possible,
>> resulting in errors such as:
>>
>>Uploading artifacts...
>>build: found 3164 matching 

Re: [PATCH v2 01/18] crypto: Fix certificate file error handling crash bug

[Daniel P . Berrangé]

On Wed, Dec 04, 2019 at 10:36:08AM +0100, Markus Armbruster wrote:
> qcrypto_tls_creds_load_cert() passes uninitialized GError *gerr by
> reference to g_file_get_contents().  When g_file_get_contents() fails,
> it'll try to set a GError.  Unless @gerr is null by dumb luck, this
> logs a ERROR_OVERWRITTEN_WARNING warning message and leaves @gerr
> unchanged.  qcrypto_tls_creds_load_cert() then dereferences the
> uninitialized @gerr.
> 
> Fix by initializing @gerr properly.
> 
> Fixes: 9a2fd4347c40321f5cbb4ab4220e759fcbf87d03
> Cc: "Daniel P. Berrangé" 
> Signed-off-by: Markus Armbruster 
> ---
>  crypto/tlscredsx509.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Daniel P. Berrangé 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v2 03/13] s390x: protvirt: Support unpack facility

[Thomas Huth]

On 04/12/2019 12.32, Janosch Frank wrote:
> On 12/4/19 11:48 AM, Thomas Huth wrote:
>> On 29/11/2019 10.47, Janosch Frank wrote:
>>> When a guest has saved a ipib of type 5 and call diagnose308 with
>>> subcode 10, we have to setup the protected processing environment via
>>> Ultravisor calls. The calls are done by KVM and are exposed via an API.
>>>
>>> The following steps are necessary:
>>> 1. Create a VM (register it with the Ultravisor)
>>> 2. Create secure CPUs for all of our current cpus
>>> 3. Forward the secure header to the Ultravisor (has all information on
>>> how to decrypt the image and VM information)
>>> 4. Protect image pages from the host and decrypt them
>>> 5. Verify the image integrity
>>>
>>> Only after step 5 a protected VM is allowed to run.
>>>
>>> Signed-off-by: Janosch Frank 
>>> ---
>> [...]
>>> +++ b/hw/s390x/pv.c
>>> @@ -0,0 +1,118 @@
>>> +/*
>>> + * Secure execution functions
>>> + *
>>> + * Copyright IBM Corp. 2019
>>> + * Author(s):
>>> + *  Janosch Frank 
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>>> + * your option) any later version. See the COPYING file in the top-level
>>> + * directory.
>>> + */
>>> +#include "qemu/osdep.h"
>>> +#include 
>>> +
>>> +#include 
>>> +
>>> +#include "qemu/error-report.h"
>>> +#include "sysemu/kvm.h"
>>> +#include "pv.h"
>>> +
>>> +static int s390_pv_cmd(uint32_t cmd, void *data)
>>> +{
>>> +int rc;
>>> +struct kvm_pv_cmd pv_cmd = {
>>> +.cmd = cmd,
>>> +.data = (uint64_t)data,
>>> +};
>>> +
>>> +rc = kvm_vm_ioctl(kvm_state, KVM_S390_PV_COMMAND, _cmd);
>>> +if (rc) {
>>> +error_report("KVM PV command failed cmd: %d rc: %d", cmd, rc);
>>> +exit(1);
>>> +}
>>> +return rc;
>>> +}
>>> +
>>> +static int s390_pv_cmd_vcpu(CPUState *cs, uint32_t cmd, void *data)
>>> +{
>>> +int rc;
>>> +struct kvm_pv_cmd pv_cmd = {
>>> +.cmd = cmd,
>>> +.data = (uint64_t)data,
>>> +};
>>> +
>>> +rc = kvm_vcpu_ioctl(cs, KVM_S390_PV_COMMAND_VCPU, _cmd);
>>> +if (rc) {
>>> +error_report("KVM PV VCPU command failed cmd: %d rc: %d", cmd, rc);
>>> +exit(1);
>>> +}
>>> +return rc;
>>> +}
>>> +
>>> +int s390_pv_vm_create(void)
>>> +{
>>> +return s390_pv_cmd(KVM_PV_VM_CREATE, NULL);
>>> +}
>>> +
>>> +int s390_pv_vm_destroy(void)
>>> +{
>>> +return s390_pv_cmd(KVM_PV_VM_DESTROY, NULL);
>>> +}
>>> +
>>> +int s390_pv_vcpu_create(CPUState *cs)
>>> +{
>>> +return s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_CREATE, NULL);
>>> +}
>>> +
>>> +int s390_pv_vcpu_destroy(CPUState *cs)
>>> +{
>>> +S390CPU *cpu = S390_CPU(cs);
>>> +CPUS390XState *env = >env;
>>> +int rc;
>>> +
>>> +rc = s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_DESTROY, NULL);
>>> +if (!rc) {
>>> +env->pv = false;
>>> +}
>>> +return rc;
>>> +}
>>> +
>>> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
>>> +{
>>> +struct kvm_s390_pv_sec_parm args = {
>>> +.origin = origin,
>>> +.length = length,
>>> +};
>>> +
>>> +return s390_pv_cmd(KVM_PV_VM_SET_SEC_PARMS, );
>>> +}
>>> +
>>> +/*
>>> + * Called for each component in the SE type IPL parameter block 0.
>>> + */
>>> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak)
>>> +{
>>> +struct kvm_s390_pv_unp args = {
>>> +.addr = addr,
>>> +.size = size,
>>> +.tweak = tweak,
>>> +};
>>> +
>>> +return s390_pv_cmd(KVM_PV_VM_UNPACK, );
>>> +}
>>> +
>>> +int s390_pv_perf_clear_reset(void)
>>> +{
>>> +return s390_pv_cmd(KVM_PV_VM_PERF_CLEAR_RESET, NULL);
>>> +}
>>> +
>>> +int s390_pv_verify(void)
>>> +{
>>> +return s390_pv_cmd(KVM_PV_VM_VERIFY, NULL);
>>> +}
>>> +
>>> +int s390_pv_unshare(void)
>>> +{
>>> +return s390_pv_cmd(KVM_PV_VM_UNSHARE, NULL);
>>> +}
>>> diff --git a/hw/s390x/pv.h b/hw/s390x/pv.h
>>> new file mode 100644
>>> index 00..eb074e4bc9
>>> --- /dev/null
>>> +++ b/hw/s390x/pv.h
>>> @@ -0,0 +1,26 @@
>>> +/*
>>> + * Protected Virtualization header
>>> + *
>>> + * Copyright IBM Corp. 2019
>>> + * Author(s):
>>> + *  Janosch Frank 
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>>> + * your option) any later version. See the COPYING file in the top-level
>>> + * directory.
>>> + */
>>> +
>>> +#ifndef HW_S390_PV_H
>>> +#define HW_S390_PV_H
>>> +
>>> +int s390_pv_vm_create(void);
>>> +int s390_pv_vm_destroy(void);
>>> +int s390_pv_vcpu_destroy(CPUState *cs);
>>> +int s390_pv_vcpu_create(CPUState *cs);
>>> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
>>> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
>>> +int s390_pv_perf_clear_reset(void);
>>> +int s390_pv_verify(void);
>>> +int s390_pv_unshare(void);
>>
>> I still think you should make all those functions returning "void"
>> instead of "int" - since errors results in an exit() in s390_pv_cmd()
>> and s390_pv_cmd_vcpu() anyway...
> 
> Hey Thomas,
> 
> I 

Re: [PATCH v4 12/40] target/arm: Rename ARMMMUIdx*_S1E3 to ARMMMUIdx*_SE3

[Alex Bennée]

Richard Henderson  writes:

> This is part of a reorganization to the set of mmu_idx.
> The EL3 regime only has a single stage translation, and
> is always secure.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  target/arm/cpu.h   |  4 ++--
>  target/arm/internals.h |  2 +-
>  target/arm/helper.c| 14 +++---
>  target/arm/translate.c |  2 +-
>  4 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index e8ee316e05..f307de561a 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2867,7 +2867,7 @@ typedef enum ARMMMUIdx {
>  ARMMMUIdx_EL10_0 = 0 | ARM_MMU_IDX_A,
>  ARMMMUIdx_EL10_1 = 1 | ARM_MMU_IDX_A,
>  ARMMMUIdx_S1E2 = 2 | ARM_MMU_IDX_A,
> -ARMMMUIdx_S1E3 = 3 | ARM_MMU_IDX_A,
> +ARMMMUIdx_SE3 = 3 | ARM_MMU_IDX_A,
>  ARMMMUIdx_SE0 = 4 | ARM_MMU_IDX_A,
>  ARMMMUIdx_SE1 = 5 | ARM_MMU_IDX_A,
>  ARMMMUIdx_Stage2 = 6 | ARM_MMU_IDX_A,
> @@ -2893,7 +2893,7 @@ typedef enum ARMMMUIdxBit {
>  ARMMMUIdxBit_EL10_0 = 1 << 0,
>  ARMMMUIdxBit_EL10_1 = 1 << 1,
>  ARMMMUIdxBit_S1E2 = 1 << 2,
> -ARMMMUIdxBit_S1E3 = 1 << 3,
> +ARMMMUIdxBit_SE3 = 1 << 3,
>  ARMMMUIdxBit_SE0 = 1 << 4,
>  ARMMMUIdxBit_SE1 = 1 << 5,
>  ARMMMUIdxBit_Stage2 = 1 << 6,
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 3600bf9122..50d258b0e1 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -819,7 +819,7 @@ static inline bool regime_is_secure(CPUARMState *env, 
> ARMMMUIdx mmu_idx)
>  case ARMMMUIdx_MPriv:
>  case ARMMMUIdx_MUser:
>  return false;
> -case ARMMMUIdx_S1E3:
> +case ARMMMUIdx_SE3:
>  case ARMMMUIdx_SE0:
>  case ARMMMUIdx_SE1:
>  case ARMMMUIdx_MSPrivNegPri:
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 377825431a..98d00b4549 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3138,7 +3138,7 @@ static void ats_write(CPUARMState *env, const 
> ARMCPRegInfo *ri, uint64_t value)
>  /* stage 1 current state PL1: ATS1CPR, ATS1CPW */
>  switch (el) {
>  case 3:
> -mmu_idx = ARMMMUIdx_S1E3;
> +mmu_idx = ARMMMUIdx_SE3;
>  break;
>  case 2:
>  mmu_idx = ARMMMUIdx_Stage1_E1;
> @@ -3220,7 +3220,7 @@ static void ats_write64(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  mmu_idx = ARMMMUIdx_S1E2;
>  break;
>  case 6: /* AT S1E3R, AT S1E3W */
> -mmu_idx = ARMMMUIdx_S1E3;
> +mmu_idx = ARMMMUIdx_SE3;
>  break;
>  default:
>  g_assert_not_reached();
> @@ -3963,7 +3963,7 @@ static void tlbi_aa64_alle3_write(CPUARMState *env, 
> const ARMCPRegInfo *ri,
>  ARMCPU *cpu = env_archcpu(env);
>  CPUState *cs = CPU(cpu);
>  
> -tlb_flush_by_mmuidx(cs, ARMMMUIdxBit_S1E3);
> +tlb_flush_by_mmuidx(cs, ARMMMUIdxBit_SE3);
>  }
>  
>  static void tlbi_aa64_alle1is_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -3988,7 +3988,7 @@ static void tlbi_aa64_alle3is_write(CPUARMState *env, 
> const ARMCPRegInfo *ri,
>  {
>  CPUState *cs = env_cpu(env);
>  
> -tlb_flush_by_mmuidx_all_cpus_synced(cs, ARMMMUIdxBit_S1E3);
> +tlb_flush_by_mmuidx_all_cpus_synced(cs, ARMMMUIdxBit_SE3);
>  }
>  
>  static void tlbi_aa64_vae2_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -4016,7 +4016,7 @@ static void tlbi_aa64_vae3_write(CPUARMState *env, 
> const ARMCPRegInfo *ri,
>  CPUState *cs = CPU(cpu);
>  uint64_t pageaddr = sextract64(value << 12, 0, 56);
>  
> -tlb_flush_page_by_mmuidx(cs, pageaddr, ARMMMUIdxBit_S1E3);
> +tlb_flush_page_by_mmuidx(cs, pageaddr, ARMMMUIdxBit_SE3);
>  }
>  
>  static void tlbi_aa64_vae1is_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -4065,7 +4065,7 @@ static void tlbi_aa64_vae3is_write(CPUARMState *env, 
> const ARMCPRegInfo *ri,
>  uint64_t pageaddr = sextract64(value << 12, 0, 56);
>  
>  tlb_flush_page_by_mmuidx_all_cpus_synced(cs, pageaddr,
> - ARMMMUIdxBit_S1E3);
> + ARMMMUIdxBit_SE3);
>  }
>  
>  static void tlbi_aa64_ipas2e1_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -8567,7 +8567,7 @@ static inline uint32_t regime_el(CPUARMState *env, 
> ARMMMUIdx mmu_idx)
>  case ARMMMUIdx_Stage2:
>  case ARMMMUIdx_S1E2:
>  return 2;
> -case ARMMMUIdx_S1E3:
> +case ARMMMUIdx_SE3:
>  return 3;
>  case ARMMMUIdx_SE0:
>  return arm_el_is_aa64(env, 3) ? 1 : 3;
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 787e34f258..6cf2fe2806 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -156,7 +156,7 @@ static inline int get_a32_user_mem_index(DisasContext *s)
>  case ARMMMUIdx_EL10_0:
>  case ARMMMUIdx_EL10_1:
>  return arm_to_core_mmu_idx(ARMMMUIdx_EL10_0);
> - 

Re: [PATCH v4 13/40] target/arm: Rename ARMMMUIdx_S1E2 to ARMMMUIdx_E2

[Alex Bennée]

Richard Henderson  writes:

> This is part of a reorganization to the set of mmu_idx.
> The non-secure EL2 regime only has a single stage translation;
> there is no point in pointing out that the idx is for stage1.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  target/arm/cpu.h   |  4 ++--
>  target/arm/internals.h |  2 +-
>  target/arm/helper.c| 22 +++---
>  target/arm/translate.c |  2 +-
>  4 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index f307de561a..28259be733 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2866,7 +2866,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, 
> unsigned int excp_idx,
>  typedef enum ARMMMUIdx {
>  ARMMMUIdx_EL10_0 = 0 | ARM_MMU_IDX_A,
>  ARMMMUIdx_EL10_1 = 1 | ARM_MMU_IDX_A,
> -ARMMMUIdx_S1E2 = 2 | ARM_MMU_IDX_A,
> +ARMMMUIdx_E2 = 2 | ARM_MMU_IDX_A,
>  ARMMMUIdx_SE3 = 3 | ARM_MMU_IDX_A,
>  ARMMMUIdx_SE0 = 4 | ARM_MMU_IDX_A,
>  ARMMMUIdx_SE1 = 5 | ARM_MMU_IDX_A,
> @@ -2892,7 +2892,7 @@ typedef enum ARMMMUIdx {
>  typedef enum ARMMMUIdxBit {
>  ARMMMUIdxBit_EL10_0 = 1 << 0,
>  ARMMMUIdxBit_EL10_1 = 1 << 1,
> -ARMMMUIdxBit_S1E2 = 1 << 2,
> +ARMMMUIdxBit_E2 = 1 << 2,
>  ARMMMUIdxBit_SE3 = 1 << 3,
>  ARMMMUIdxBit_SE0 = 1 << 4,
>  ARMMMUIdxBit_SE1 = 1 << 5,
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 50d258b0e1..aee54dc105 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -812,7 +812,7 @@ static inline bool regime_is_secure(CPUARMState *env, 
> ARMMMUIdx mmu_idx)
>  case ARMMMUIdx_EL10_1:
>  case ARMMMUIdx_Stage1_E0:
>  case ARMMMUIdx_Stage1_E1:
> -case ARMMMUIdx_S1E2:
> +case ARMMMUIdx_E2:
>  case ARMMMUIdx_Stage2:
>  case ARMMMUIdx_MPrivNegPri:
>  case ARMMMUIdx_MUserNegPri:
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 98d00b4549..5172843667 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -728,7 +728,7 @@ static void tlbiall_hyp_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  {
>  CPUState *cs = env_cpu(env);
>  
> -tlb_flush_by_mmuidx(cs, ARMMMUIdxBit_S1E2);
> +tlb_flush_by_mmuidx(cs, ARMMMUIdxBit_E2);
>  }
>  
>  static void tlbiall_hyp_is_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -736,7 +736,7 @@ static void tlbiall_hyp_is_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  {
>  CPUState *cs = env_cpu(env);
>  
> -tlb_flush_by_mmuidx_all_cpus_synced(cs, ARMMMUIdxBit_S1E2);
> +tlb_flush_by_mmuidx_all_cpus_synced(cs, ARMMMUIdxBit_E2);
>  }
>  
>  static void tlbimva_hyp_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -745,7 +745,7 @@ static void tlbimva_hyp_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  CPUState *cs = env_cpu(env);
>  uint64_t pageaddr = value & ~MAKE_64BIT_MASK(0, 12);
>  
> -tlb_flush_page_by_mmuidx(cs, pageaddr, ARMMMUIdxBit_S1E2);
> +tlb_flush_page_by_mmuidx(cs, pageaddr, ARMMMUIdxBit_E2);
>  }
>  
>  static void tlbimva_hyp_is_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -755,7 +755,7 @@ static void tlbimva_hyp_is_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  uint64_t pageaddr = value & ~MAKE_64BIT_MASK(0, 12);
>  
>  tlb_flush_page_by_mmuidx_all_cpus_synced(cs, pageaddr,
> - ARMMMUIdxBit_S1E2);
> + ARMMMUIdxBit_E2);
>  }
>  
>  static const ARMCPRegInfo cp_reginfo[] = {
> @@ -3189,7 +3189,7 @@ static void ats1h_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  MMUAccessType access_type = ri->opc2 & 1 ? MMU_DATA_STORE : 
> MMU_DATA_LOAD;
>  uint64_t par64;
>  
> -par64 = do_ats_write(env, value, access_type, ARMMMUIdx_S1E2);
> +par64 = do_ats_write(env, value, access_type, ARMMMUIdx_E2);
>  
>  A32_BANKED_CURRENT_REG_SET(env, par, par64);
>  }
> @@ -3217,7 +3217,7 @@ static void ats_write64(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  mmu_idx = secure ? ARMMMUIdx_SE1 : ARMMMUIdx_Stage1_E1;
>  break;
>  case 4: /* AT S1E2R, AT S1E2W */
> -mmu_idx = ARMMMUIdx_S1E2;
> +mmu_idx = ARMMMUIdx_E2;
>  break;
>  case 6: /* AT S1E3R, AT S1E3W */
>  mmu_idx = ARMMMUIdx_SE3;
> @@ -3954,7 +3954,7 @@ static void tlbi_aa64_alle2_write(CPUARMState *env, 
> const ARMCPRegInfo *ri,
>  ARMCPU *cpu = env_archcpu(env);
>  CPUState *cs = CPU(cpu);
>  
> -tlb_flush_by_mmuidx(cs, ARMMMUIdxBit_S1E2);
> +tlb_flush_by_mmuidx(cs, ARMMMUIdxBit_E2);
>  }
>  
>  static void tlbi_aa64_alle3_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -3980,7 +3980,7 @@ static void tlbi_aa64_alle2is_write(CPUARMState *env, 
> const ARMCPRegInfo *ri,
>  {
>  CPUState *cs = env_cpu(env);
>  
> -tlb_flush_by_mmuidx_all_cpus_synced(cs, ARMMMUIdxBit_S1E2);
> +

Re: [PATCH v3 2/4] s390x: Add missing vcpu reset functions

[Janosch Frank]

On 12/3/19 6:44 PM, Cornelia Huck wrote:
> On Tue,  3 Dec 2019 08:28:11 -0500
> Janosch Frank  wrote:
> 
>> Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach
>> for the initial reset, and that was also called for the clear
>> reset. To be architecture compliant, we also need to clear local
>> interrupts on a normal reset.
>>
>> Because of this and the upcoming protvirt support we need to add
>> ioctls for the missing clear and normal resets.
>>
>> Signed-off-by: Janosch Frank 
>> Reviewed-by: Thomas Huth 
>> Acked-by: David Hildenbrand 
>> ---
>>  target/s390x/cpu.c   | 16 +--
>>  target/s390x/kvm-stub.c  | 10 +-
>>  target/s390x/kvm.c   | 42 
>>  target/s390x/kvm_s390x.h |  4 +++-
>>  4 files changed, 60 insertions(+), 12 deletions(-)
>>
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index 829ce6ad54..4973365d6c 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -139,8 +139,20 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type 
>> type)
>>  }
>>  
>>  /* Reset state inside the kernel that we cannot access yet from QEMU. */
> 
> For the last iteration, I asked about the 'yet' here...

I have not written those comments, I merely refuse to delete them :)
We still reset some state in the kernel, I'm not sure how much of that
is already exposed via ioctls to QEMU, so I won't remove the comment.

> 
>> -if (kvm_enabled() && type != S390_CPU_RESET_NORMAL) {
>> -kvm_s390_reset_vcpu(cpu);
>> +if (kvm_enabled()) {
>> +switch (type) {
>> +case S390_CPU_RESET_CLEAR:
>> +kvm_s390_reset_vcpu_clear(cpu);
>> +break;
>> +case S390_CPU_RESET_INITIAL:
>> +kvm_s390_reset_vcpu_initial(cpu);
>> +break;
>> +case S390_CPU_RESET_NORMAL:
>> +kvm_s390_reset_vcpu_normal(cpu);
>> +break;
>> +default:
>> +g_assert_not_reached();
>> +}
>>  }
>>  }
>>  
> 
> (...)
> 
>> @@ -403,17 +405,41 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
>>  return 0;
>>  }
>>  
>> -void kvm_s390_reset_vcpu(S390CPU *cpu)
>> +static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
>>  {
>>  CPUState *cs = CPU(cpu);
>>  
>> -/* The initial reset call is needed here to reset in-kernel
>> - * vcpu data that we can't access directly from QEMU
>> - * (i.e. with older kernels which don't support sync_regs/ONE_REG).
>> - * Before this ioctl cpu_synchronize_state() is called in common kvm
>> - * code (kvm-all) */
>> -if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
>> -error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
>> +/*
>> + * The reset call is needed here to reset in-kernel vcpu data that
>> + * we can't access directly from QEMU (i.e. with older kernels
>> + * which don't support sync_regs/ONE_REG).  Before this ioctl
> 
> ...and this reference to 'older kernels' here.
> 
> Are the comments still correct/relevant?

See above

> 
>> + * cpu_synchronize_state() is called in common kvm code
>> + * (kvm-all).
>> + */
>> +if (kvm_vcpu_ioctl(cs, type)) {
>> +error_report("CPU reset failed on CPU %i type %lx",
>> + cs->cpu_index, type);
>> +}
>> +}
> 




signature.asc
Description: OpenPGP digital signature

[PATCH v2 04/18] tests: Clean up initialization of Error *err variables

[Markus Armbruster]

Declaring a local Error *err without initializer looks suspicious.
Fuse the declaration with the initialization to avoid that.

Signed-off-by: Markus Armbruster 
---
 tests/test-qobject-output-visitor.c | 8 
 tests/test-string-output-visitor.c  | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/test-qobject-output-visitor.c 
b/tests/test-qobject-output-visitor.c
index 3e993e5ba8..d7761ebf84 100644
--- a/tests/test-qobject-output-visitor.c
+++ b/tests/test-qobject-output-visitor.c
@@ -145,10 +145,10 @@ static void 
test_visitor_out_enum_errors(TestOutputVisitorData *data,
  const void *unused)
 {
 EnumOne i, bad_values[] = { ENUM_ONE__MAX, -1 };
-Error *err;
 
 for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
-err = NULL;
+Error *err = NULL;
+
 visit_type_EnumOne(data->ov, "unused", _values[i], );
 error_free_or_abort();
 visitor_reset(data);
@@ -240,11 +240,11 @@ static void 
test_visitor_out_struct_errors(TestOutputVisitorData *data,
 EnumOne bad_values[] = { ENUM_ONE__MAX, -1 };
 UserDefOne u = {0};
 UserDefOne *pu = 
-Error *err;
 int i;
 
 for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
-err = NULL;
+Error *err = NULL;
+
 u.has_enum1 = true;
 u.enum1 = bad_values[i];
 visit_type_UserDefOne(data->ov, "unused", , );
diff --git a/tests/test-string-output-visitor.c 
b/tests/test-string-output-visitor.c
index 02766c0f65..1be1540767 100644
--- a/tests/test-string-output-visitor.c
+++ b/tests/test-string-output-visitor.c
@@ -207,10 +207,10 @@ static void 
test_visitor_out_enum_errors(TestOutputVisitorData *data,
  const void *unused)
 {
 EnumOne i, bad_values[] = { ENUM_ONE__MAX, -1 };
-Error *err;
 
 for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
-err = NULL;
+Error *err = NULL;
+
 visit_type_EnumOne(data->ov, "unused", _values[i], );
 error_free_or_abort();
 }
-- 
2.21.0

Re: [PATCH v2 2/3] virtio-balloon: fix memory leak while attach virtio-balloon device

[Laurent Vivier]

On 04/12/2019 08:31, pannengy...@huawei.com wrote:
> From: Pan Nengyuan 
> 
> ivq/dvq/svq/free_page_vq is forgot to cleanup in
> virtio_balloon_device_unrealize, the memory leak stack is as follow:
> 
> Direct leak of 14336 byte(s) in 2 object(s) allocated from:
> #0 0x7f99fd9d8560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
> #1 0x7f99fcb20015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
> #2 0x557d90638437 in virtio_add_queue hw/virtio/virtio.c:2327
> #3 0x557d9064401d in virtio_balloon_device_realize 
> hw/virtio/virtio-balloon.c:793
> #4 0x557d906356f7 in virtio_device_realize hw/virtio/virtio.c:3504
> #5 0x557d9073f081 in device_set_realized hw/core/qdev.c:876
> #6 0x557d908b1f4d in property_set_bool qom/object.c:2080
> #7 0x557d908b655e in object_property_set_qobject qom/qom-qobject.c:26
> 
> Reported-by: Euler Robot 
> Signed-off-by: Pan Nengyuan 
> ---
> Change v2 to v1:
> - use virtio_delete_queue to cleanup vq through a vq pointer (suggested by
>   Michael S. Tsirkin)
> ---
>  hw/virtio/virtio-balloon.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 40b04f5..57f3b9f 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -831,6 +831,13 @@ static void virtio_balloon_device_unrealize(DeviceState 
> *dev, Error **errp)
>  }
>  balloon_stats_destroy_timer(s);
>  qemu_remove_balloon_handler(s);
> +
> +virtio_delete_queue(s->ivq);
> +virtio_delete_queue(s->dvq);
> +virtio_delete_queue(s->svq);
> +if (s->free_page_vq) {
> +virtio_delete_queue(s->free_page_vq);
> +}
>  virtio_cleanup(vdev);
>  }
>  
> 

Reviewed-by: Laurent Vivier 

Re: [PATCH v6 5/9] qdev-clock: introduce an init array to ease the device construction

[Damien Hedde]

On 12/2/19 4:13 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde  wrote:
>>
>> Introduce a function and macro helpers to setup several clocks
>> in a device from a static array description.
>>
>> An element of the array describes the clock (name and direction) as
>> well as the related callback and an optional offset to store the
>> created object pointer in the device state structure.
>>
>> The array must be terminated by a special element QDEV_CLOCK_END.
>>
>> This is based on the original work of Frederic Konrad.
>>
>> Signed-off-by: Damien Hedde 
>> Reviewed-by: Philippe Mathieu-Daudé 
>> ---
>>  hw/core/qdev-clock.c| 26 
>>  include/hw/qdev-clock.h | 67 +
>>  2 files changed, 93 insertions(+)
>>
>> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
>> index bebdd8fa15..32ad45c061 100644
>> --- a/hw/core/qdev-clock.c
>> +++ b/hw/core/qdev-clock.c
>> @@ -153,3 +153,29 @@ void qdev_connect_clock_out(DeviceState *dev, const 
>> char *name, ClockIn *clk,
>>
>>  clock_connect(clk, clkout);
>>  }
>> +
>> +void qdev_init_clocks(DeviceState *dev, const ClockPortInitArray clocks)
>> +{
>> +const struct ClockPortInitElem *elem;
>> +
>> +assert(dev);
>> +assert(clocks);
> 
> More unnecessary asserts, I think.
> 
> 
> 
>> +/**
>> + * ClockInitElem:
>> + * @name: name of the clock (can't be NULL)
>> + * @is_output: indicates whether the clock is input or output
>> + * @callback: for inputs, optional callback to be called on clock's update
>> + * with device as opaque
>> + * @offset: optional offset to store the ClockIn or ClockOut pointer in 
>> device
>> + * state structure (0 means unused)
>> + */
>> +struct ClockPortInitElem {
>> +const char *name;
>> +bool is_output;
>> +ClockCallback *callback;
>> +size_t offset;
>> +};
>> +
>> +#define clock_offset_value(_type, _devstate, _field) \
>> +(offsetof(_devstate, _field) + \
>> + type_check(_type *, typeof_field(_devstate, _field)))
> 
> Avoid leading underscores, please.
> 
>> +
>> +#define QDEV_CLOCK(_is_output, _type, _devstate, _field, _callback) { \
>> +.name = (stringify(_field)), \
>> +.is_output = _is_output, \
>> +.callback = _callback, \
>> +.offset = clock_offset_value(_type, _devstate, _field), \
>> +}
>> +
>> +/**
>> + * QDEV_CLOCK_(IN|OUT):
>> + * @_devstate: structure type. @dev argument of qdev_init_clocks below must 
>> be
>> + * a pointer to that same type.
> 
> It's a bit unclear what "below" here is referring to. Maybe
> just have this be "@devstate: name of a C struct type"
> and then explain below...
> 
>> + * @_field: a field in @_devstate (must be ClockIn* or ClockOut*)
>> + * @_callback: (for input only) callback (or NULL) to be called with the 
>> device
>> + * state as argument
>> + *
> 
> ...here, where we can have a paragraph giving the purpose
> of the macro:
> 
> "Define an entry in a ClockPortInitArray which is intended
> to be passed to qdev_init_clocks(), which should be called
> with an @dev argument which is a pointer to the @devstate
> struct type."

Sounds good.

> 
>> + * The name of the clock will be derived from @_field
> 
> Derived how? Guessing from the stringify(_field) above that it
> will be the same as the field name ?

yes.

> 
> It makes sense to hardcode the opaque pointer for the callback to be
> the device pointer.
> 
> 
>> + */
>> +#define QDEV_CLOCK_IN(_devstate, _field, _callback) \
>> +QDEV_CLOCK(false, ClockIn, _devstate, _field, _callback)
>> +
>> +#define QDEV_CLOCK_OUT(_devstate, _field) \
>> +QDEV_CLOCK(true, ClockOut, _devstate, _field, NULL)
>> +
>> +/**
>> + * QDEV_CLOCK_IN_NOFIELD:
>> + * @_name: name of the clock
>> + * @_callback: callback (or NULL) to be called with the device state as 
>> argument
>> + */
>> +#define QDEV_CLOCK_IN_NOFIELD(_name, _callback) { \
>> +.name = _name, \
>> +.is_output = false, \
>> +.callback = _callback, \
>> +.offset = 0, \
>> +}
> 
> When would we want to use this one ?

If the callback interaction is enough, we don't need to access the clock
object directly. So we don't need the field in the device state
structure. I can remove this macro for sake of simplicity.

--
Damien

Re: [PATCH v4 11/40] target/arm: Rename ARMMMUIdx_S1SE* to ARMMMUIdx_SE*

[Alex Bennée]

Richard Henderson  writes:

> This is part of a reorganization to the set of mmu_idx.
> The Secure regimes all have a single stage translation;
> there is no point in pointing out that the idx is for stage1.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  target/arm/cpu.h   |  8 
>  target/arm/internals.h |  4 ++--
>  target/arm/translate.h |  2 +-
>  target/arm/helper.c| 26 +-
>  target/arm/translate-a64.c |  4 ++--
>  target/arm/translate.c |  6 +++---
>  6 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 0714c52176..e8ee316e05 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2868,8 +2868,8 @@ typedef enum ARMMMUIdx {
>  ARMMMUIdx_EL10_1 = 1 | ARM_MMU_IDX_A,
>  ARMMMUIdx_S1E2 = 2 | ARM_MMU_IDX_A,
>  ARMMMUIdx_S1E3 = 3 | ARM_MMU_IDX_A,
> -ARMMMUIdx_S1SE0 = 4 | ARM_MMU_IDX_A,
> -ARMMMUIdx_S1SE1 = 5 | ARM_MMU_IDX_A,
> +ARMMMUIdx_SE0 = 4 | ARM_MMU_IDX_A,
> +ARMMMUIdx_SE1 = 5 | ARM_MMU_IDX_A,
>  ARMMMUIdx_Stage2 = 6 | ARM_MMU_IDX_A,
>  ARMMMUIdx_MUser = 0 | ARM_MMU_IDX_M,
>  ARMMMUIdx_MPriv = 1 | ARM_MMU_IDX_M,
> @@ -2894,8 +2894,8 @@ typedef enum ARMMMUIdxBit {
>  ARMMMUIdxBit_EL10_1 = 1 << 1,
>  ARMMMUIdxBit_S1E2 = 1 << 2,
>  ARMMMUIdxBit_S1E3 = 1 << 3,
> -ARMMMUIdxBit_S1SE0 = 1 << 4,
> -ARMMMUIdxBit_S1SE1 = 1 << 5,
> +ARMMMUIdxBit_SE0 = 1 << 4,
> +ARMMMUIdxBit_SE1 = 1 << 5,
>  ARMMMUIdxBit_Stage2 = 1 << 6,
>  ARMMMUIdxBit_MUser = 1 << 0,
>  ARMMMUIdxBit_MPriv = 1 << 1,
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 3fd1518f3b..3600bf9122 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -820,8 +820,8 @@ static inline bool regime_is_secure(CPUARMState *env, 
> ARMMMUIdx mmu_idx)
>  case ARMMMUIdx_MUser:
>  return false;
>  case ARMMMUIdx_S1E3:
> -case ARMMMUIdx_S1SE0:
> -case ARMMMUIdx_S1SE1:
> +case ARMMMUIdx_SE0:
> +case ARMMMUIdx_SE1:
>  case ARMMMUIdx_MSPrivNegPri:
>  case ARMMMUIdx_MSUserNegPri:
>  case ARMMMUIdx_MSPriv:
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index dd24f91f26..3760159661 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -124,7 +124,7 @@ static inline int default_exception_el(DisasContext *s)
>   * exceptions can only be routed to ELs above 1, so we target the higher 
> of
>   * 1 or the current EL.
>   */
> -return (s->mmu_idx == ARMMMUIdx_S1SE0 && s->secure_routed_to_el3)
> +return (s->mmu_idx == ARMMMUIdx_SE0 && s->secure_routed_to_el3)
>  ? 3 : MAX(1, s->current_el);
>  }
>  
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index a34accec20..377825431a 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3144,7 +3144,7 @@ static void ats_write(CPUARMState *env, const 
> ARMCPRegInfo *ri, uint64_t value)
>  mmu_idx = ARMMMUIdx_Stage1_E1;
>  break;
>  case 1:
> -mmu_idx = secure ? ARMMMUIdx_S1SE1 : ARMMMUIdx_Stage1_E1;
> +mmu_idx = secure ? ARMMMUIdx_SE1 : ARMMMUIdx_Stage1_E1;
>  break;
>  default:
>  g_assert_not_reached();
> @@ -3154,13 +3154,13 @@ static void ats_write(CPUARMState *env, const 
> ARMCPRegInfo *ri, uint64_t value)
>  /* stage 1 current state PL0: ATS1CUR, ATS1CUW */
>  switch (el) {
>  case 3:
> -mmu_idx = ARMMMUIdx_S1SE0;
> +mmu_idx = ARMMMUIdx_SE0;
>  break;
>  case 2:
>  mmu_idx = ARMMMUIdx_Stage1_E0;
>  break;
>  case 1:
> -mmu_idx = secure ? ARMMMUIdx_S1SE0 : ARMMMUIdx_Stage1_E0;
> +mmu_idx = secure ? ARMMMUIdx_SE0 : ARMMMUIdx_Stage1_E0;
>  break;
>  default:
>  g_assert_not_reached();
> @@ -3214,7 +3214,7 @@ static void ats_write64(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  case 0:
>  switch (ri->opc1) {
>  case 0: /* AT S1E1R, AT S1E1W */
> -mmu_idx = secure ? ARMMMUIdx_S1SE1 : ARMMMUIdx_Stage1_E1;
> +mmu_idx = secure ? ARMMMUIdx_SE1 : ARMMMUIdx_Stage1_E1;
>  break;
>  case 4: /* AT S1E2R, AT S1E2W */
>  mmu_idx = ARMMMUIdx_S1E2;
> @@ -3227,13 +3227,13 @@ static void ats_write64(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  }
>  break;
>  case 2: /* AT S1E0R, AT S1E0W */
> -mmu_idx = secure ? ARMMMUIdx_S1SE0 : ARMMMUIdx_Stage1_E0;
> +mmu_idx = secure ? ARMMMUIdx_SE0 : ARMMMUIdx_Stage1_E0;
>  break;
>  case 4: /* AT S12E1R, AT S12E1W */
> -mmu_idx = secure ? ARMMMUIdx_S1SE1 : ARMMMUIdx_EL10_1;
> +mmu_idx = secure ? ARMMMUIdx_SE1 : ARMMMUIdx_EL10_1;
>  break;
>  case 6: /* AT S12E0R, AT S12E0W */
> -mmu_idx = secure ? 

Re: virtiofsd: Where should it live?

[Markus Armbruster]

Daniel P. Berrangé  writes:

> On Tue, Dec 03, 2019 at 11:06:44AM +, Peter Maydell wrote:
>> On Tue, 3 Dec 2019 at 10:53, Dr. David Alan Gilbert  
>> wrote:
>> >
>> > We seem to be coming to the conclusion something that:
>> >
>> >   a) It should live in the qemu tree
>> >   b) It shouldn't live under contrib
>> >   c) We'll create a new top level, i.e. 'daemons'
>> >   d) virtiofsd will be daemons/virtiofsd
>> >
>> > Now, somethings I'm less clear on:
>> >   e) What else would move into daemons?  It was suggested
>> > that if we've got virtiofsd in there, then we should move
>> > libvhost-user - which I understand, but then it's not a
>> > 'daemons'.
>> > Are there any otehr daemons that should move?
>> 
>> I like the idea of a new top level directory, but I think
>> 'daemons' is a bit too specific -- for instance it seems to
>> me that qemu-img would be sensible to move out of the root,
>> and that's not a daemon.
>
> Do we really need an extra directory level ?

+1

> IIUC, the main point against having $GIT_ROOT/virtiofsd is that
> the root of our repo is quite cluttered already.
>
> Rather than trying to create a multi-level hierarchy which adds
> a debate around naming, why not address the clutter by moving
> *ALL* the .c/.h files out of the root so that we have a flatter
> tree:
>
>   $GITROOT
> +- qemu-system
> |   +- vl.c
> |   +- ...most other files...

Sounds good to me.

> +- qemu-img
> |   +- qemu-img.c

Perhaps this one can all go into existing block/, similar to how
pr-manager-helper.c is in scsi/, and virtfs-proxy-helper.c is in fsdev/.
Up to the block maintainers, of course.

> +- qemu-nbd
> |   +- qemu-nbd.c

block/ or nbd/?

> +- qemu-io
> |   +- qemu-io.c
> |   +- qemu-io-cmds.c

block/?

> +- qemu-bridge-helper

net/?

> |   ...
> +- qemu-edid

Has its own MAINTAINERS section, together with hw/display/edit* and
include/hw/display/edid.h.  I'm not sure moving it hw/display/ is a good
idea.  Gerd?

> +- qemu-keymap

Not covered by MAINTAINERS.  scripts/get_maintainer.pl --git-blame
points to Gerd.

> +- qga  (already exists)

Yes.

> Then we can add virtiofsd and other programs at the root with no big
> issue.

We don't *have* to put each program into its own directory.  Simple ones
could also share one.  We just need a directory name.

Re: [PATCH] travis.yml: Drop libcap-dev

[Greg Kurz]

On Wed, 4 Dec 2019 09:07:42 +0100
Thomas Huth  wrote:

> On 04/12/2019 09.01, Greg Kurz wrote:
> > Commit b1553ab12fe0 converted virtfs-proxy-helper to using libcap-ng. There
> > aren't any users of libcap anymore. No need to install libcap-dev.
> > 
> > Signed-off-by: Greg Kurz 
> > ---
> > 
> > Yet another follow-up to Paolo's patch to use libcap-ng instead of libcap.
> > Like with the docker and the gitlab CI patches, if I get an ack from Alex
> > I'll gladly merge this in the 9p tree and send a PR as soon as 5.0 dev
> > begins. I'll make sure the SHA1 for Paolo's patch remains the same.
> 
> I hope you don't have to rebase! Otherwise simply say "One of the
> previous changes ..." or so in the commit message instead.
> 

I don't expect to rebase, hence the SHA1.

> >  .travis.yml |1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/.travis.yml b/.travis.yml
> > index 445b0646c18a..6cb8af6fa599 100644
> > --- a/.travis.yml
> > +++ b/.travis.yml
> > @@ -26,7 +26,6 @@ addons:
> >- libaio-dev
> >- libattr1-dev
> >- libbrlapi-dev
> > -  - libcap-dev
> >- libcap-ng-dev
> >- libgcc-4.8-dev
> >- libgnutls28-dev
> 
> Reviewed-by: Thomas Huth 
> 

Thanks.

[PATCH v3] travis.yml: Run tcg tests with tci

[Thomas Huth]

So far we only have compile coverage for tci. But since commit
2f160e0f9797c7522bfd0d09218d0c9340a5137c ("tci: Add implementation
for INDEX_op_ld16u_i64") has been included now, we can also run the
"tcg" and "qtest" tests with tci, so let's enable them in Travis now.
Since we don't gain much additional test coverage by compiling all
targets, and TCI is broken e.g. with the Sparc targets, we also limit
the target list to a reasonable subset now (which should still get us
test coverage by tests/boot-serial-test for example).

Tested-by: Stefan Weil 
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Thomas Huth 
---
 v3:
 - Add --disable-kvm option since we're only interested in TCG here

 .travis.yml | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 445b0646c1..d73e2fb744 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -215,10 +215,11 @@ matrix:
 - TEST_CMD=""
 
 
-# We manually include builds which we disable "make check" for
+# Check the TCG interpreter (TCI)
 - env:
-- CONFIG="--enable-debug --enable-tcg-interpreter"
-- TEST_CMD=""
+- CONFIG="--enable-debug --enable-tcg-interpreter --disable-kvm 
--disable-containers
+
--target-list=alpha-softmmu,arm-softmmu,hppa-softmmu,m68k-softmmu,microblaze-softmmu,moxie-softmmu,ppc-softmmu,s390x-softmmu,x86_64-softmmu"
+- TEST_CMD="make check-qtest check-tcg V=1"
 
 
 # We don't need to exercise every backend with every front-end
-- 
2.18.1

Re: [PATCH] scripts: Fix undefinited name 'file' error for python3

[Thomas Huth]

On 04/12/2019 07.48, Han Han wrote:
> Anyone help to review it?

 Hi!

When sending patches to the qemu-devel mailing list, please always make
sure to put the corresponding maintainers on CC:, otherwise your mails
might get lost in the high traffic of the mailing list. For this case,
it would have been good to CC: the "Migration" and "Python script"
maintainers, see the corresponding sections of the MAINTAINERS file in
the top most directory of the QEMU sources.

Anyway, it seems someone else ran into the same issue already, too, and
 it got already fixed here:

https://git.qemu.org/?p=qemu.git;a=commitdiff;h=e8d0ac5801edda91412e5

  Thomas


> On Tue, Nov 26, 2019 at 1:54 PM Han Han  > wrote:
> 
> ping
> 
> On Wed, Nov 13, 2019 at 9:17 PM Han Han  > wrote:
> 
> In python3, 'file' is no longer a keyword for file type object.
> So it
> will can error when run the scripts by python3:
> 
> $ python3 ./scripts/vmstate-static-checker.py -s 4.0.json -d
> 4.1.json
> Traceback (most recent call last):
>   File "./scripts/vmstate-static-checker.py", line 431, in 
>     sys.exit(main())
>   File "./scripts/vmstate-static-checker.py", line 378, in main
>     parser.add_argument('-s', '--src', type=file, required=True,
> NameError: name 'file' is not defined
> 
> Replace file type to argparse.FileType('r').
> 
> Signed-off-by: Han Han mailto:h...@redhat.com>>
> ---
>  scripts/vmstate-static-checker.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/vmstate-static-checker.py
> b/scripts/vmstate-static-checker.py
> index d3467288..14f199a0 100755
> --- a/scripts/vmstate-static-checker.py
> +++ b/scripts/vmstate-static-checker.py
> @@ -375,9 +375,9 @@ def main():
>      help_text = "Parse JSON-formatted vmstate dumps from QEMU
> in files SRC and DEST.  Checks whether migration from SRC to
> DEST QEMU versions would break based on the VMSTATE information
> contained within the JSON outputs.  The JSON output is created
> from a QEMU invocation with the -dump-vmstate parameter and a
> filename argument to it.  Other parameters to QEMU do not
> matter, except the -M (machine type) parameter."
> 
>      parser = argparse.ArgumentParser(description=help_text)
> -    parser.add_argument('-s', '--src', type=file, required=True,
> +    parser.add_argument('-s', '--src',
> type=argparse.FileType('r'), required=True,
>                          help='json dump from src qemu')
> -    parser.add_argument('-d', '--dest', type=file, required=True,
> +    parser.add_argument('-d', '--dest',
> type=argparse.FileType('r'), required=True,
>                          help='json dump from dest qemu')
>      parser.add_argument('--reverse', required=False, default=False,
>                          action='store_true',
> -- 
> 2.23.0
> 
> 
> 
> -- 
> Best regards,
> ---
> Han Han
> Quality Engineer
> Redhat.
> 
> Email: h...@redhat.com 
> Phone: +861065339333

Re: [PATCH v3 2/4] s390x: Add missing vcpu reset functions

[Cornelia Huck]

On Wed, 4 Dec 2019 10:00:45 +0100
Janosch Frank  wrote:

> On 12/3/19 6:44 PM, Cornelia Huck wrote:
> > On Tue,  3 Dec 2019 08:28:11 -0500
> > Janosch Frank  wrote:
> >   
> >> Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach
> >> for the initial reset, and that was also called for the clear
> >> reset. To be architecture compliant, we also need to clear local
> >> interrupts on a normal reset.
> >>
> >> Because of this and the upcoming protvirt support we need to add
> >> ioctls for the missing clear and normal resets.
> >>
> >> Signed-off-by: Janosch Frank 
> >> Reviewed-by: Thomas Huth 
> >> Acked-by: David Hildenbrand 
> >> ---
> >>  target/s390x/cpu.c   | 16 +--
> >>  target/s390x/kvm-stub.c  | 10 +-
> >>  target/s390x/kvm.c   | 42 
> >>  target/s390x/kvm_s390x.h |  4 +++-
> >>  4 files changed, 60 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> >> index 829ce6ad54..4973365d6c 100644
> >> --- a/target/s390x/cpu.c
> >> +++ b/target/s390x/cpu.c
> >> @@ -139,8 +139,20 @@ static void s390_cpu_reset(CPUState *s, 
> >> cpu_reset_type type)
> >>  }
> >>  
> >>  /* Reset state inside the kernel that we cannot access yet from QEMU. 
> >> */  
> > 
> > For the last iteration, I asked about the 'yet' here...  
> 
> I have not written those comments, I merely refuse to delete them :)
> We still reset some state in the kernel, I'm not sure how much of that
> is already exposed via ioctls to QEMU, so I won't remove the comment.
> 
> >   
> >> -if (kvm_enabled() && type != S390_CPU_RESET_NORMAL) {
> >> -kvm_s390_reset_vcpu(cpu);
> >> +if (kvm_enabled()) {
> >> +switch (type) {
> >> +case S390_CPU_RESET_CLEAR:
> >> +kvm_s390_reset_vcpu_clear(cpu);
> >> +break;
> >> +case S390_CPU_RESET_INITIAL:
> >> +kvm_s390_reset_vcpu_initial(cpu);
> >> +break;
> >> +case S390_CPU_RESET_NORMAL:
> >> +kvm_s390_reset_vcpu_normal(cpu);
> >> +break;
> >> +default:
> >> +g_assert_not_reached();
> >> +}
> >>  }
> >>  }
> >>
> > 
> > (...)
> >   
> >> @@ -403,17 +405,41 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
> >>  return 0;
> >>  }
> >>  
> >> -void kvm_s390_reset_vcpu(S390CPU *cpu)
> >> +static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
> >>  {
> >>  CPUState *cs = CPU(cpu);
> >>  
> >> -/* The initial reset call is needed here to reset in-kernel
> >> - * vcpu data that we can't access directly from QEMU
> >> - * (i.e. with older kernels which don't support sync_regs/ONE_REG).
> >> - * Before this ioctl cpu_synchronize_state() is called in common kvm
> >> - * code (kvm-all) */
> >> -if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
> >> -error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
> >> +/*
> >> + * The reset call is needed here to reset in-kernel vcpu data that
> >> + * we can't access directly from QEMU (i.e. with older kernels
> >> + * which don't support sync_regs/ONE_REG).  Before this ioctl  
> > 
> > ...and this reference to 'older kernels' here.
> > 
> > Are the comments still correct/relevant?  
> 
> See above

Fair enough, let's keep them for now and revisit this later.

> 
> >   
> >> + * cpu_synchronize_state() is called in common kvm code
> >> + * (kvm-all).
> >> + */
> >> +if (kvm_vcpu_ioctl(cs, type)) {
> >> +error_report("CPU reset failed on CPU %i type %lx",
> >> + cs->cpu_index, type);
> >> +}
> >> +}  
> >   
> 
> 



pgpHZKd3FGrTB.pgp
Description: OpenPGP digital signature

Re: [PATCH 02/10] hw: arm: add Xunlong Orange Pi PC machine

[Philippe Mathieu-Daudé]

On 12/3/19 8:33 PM, Niek Linnenbank wrote:

Hello Philippe,

Thanks for your quick review comments!
I'll start working on a v2 of the patches and include the changes you 
suggested.


Thanks, but I'd suggest to wait few more days to give time to others 
reviewers. Else having multiple versions of a big series reviewed at the 
same time is very confusing.
I have other minor comments on others patches, but need to find the time 
to continue reviewing.

[PATCH v2 06/18] hw/acpi: Fix legacy CPU plug error API violations

[Markus Armbruster]

legacy_acpi_cpu_plug_cb() dereferences @errp when
acpi_set_cpu_present_bit() fails.  That's wrong; see the big comment
in error.h.  Introduced in commit cc43364de7 "acpi/cpu-hotplug:
introduce helper function to keep bit setting in one place".

No caller actually passes null, and acpi_set_cpu_present_bit() can't
actually fail.

Fix anyway: drop acpi_set_cpu_present_bit()'s @errp parameter.

Cc: "Michael S. Tsirkin" 
Cc: Igor Mammedov 
Signed-off-by: Markus Armbruster 
Reviewed-by: Igor Mammedov 
---
 hw/acpi/cpu_hotplug.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
index 3ac2045a95..9c3bcc84de 100644
--- a/hw/acpi/cpu_hotplug.c
+++ b/hw/acpi/cpu_hotplug.c
@@ -55,8 +55,7 @@ static const MemoryRegionOps AcpiCpuHotplug_ops = {
 },
 };
 
-static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, CPUState *cpu,
- Error **errp)
+static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, CPUState *cpu)
 {
 CPUClass *k = CPU_GET_CLASS(cpu);
 int64_t cpu_id;
@@ -74,10 +73,7 @@ static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, 
CPUState *cpu,
 void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
  AcpiCpuHotplug *g, DeviceState *dev, Error **errp)
 {
-acpi_set_cpu_present_bit(g, CPU(dev), errp);
-if (*errp != NULL) {
-return;
-}
+acpi_set_cpu_present_bit(g, CPU(dev));
 acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS);
 }
 
@@ -92,7 +88,7 @@ void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, 
Object *owner,
 gpe_cpu->device = owner;
 
 CPU_FOREACH(cpu) {
-acpi_set_cpu_present_bit(gpe_cpu, cpu, _abort);
+acpi_set_cpu_present_bit(gpe_cpu, cpu);
 }
 }
 
-- 
2.21.0

[PATCH v2 00/18] Error handling fixes

[Markus Armbruster]

v2:
* Old PATCH 01-03 have been merged for 4.2
* PATCH 05-15: Commit message rephrased [David], R-bys kept

Cc: "Daniel P. Berrangé" 
Cc: "Michael S. Tsirkin" 
Cc: Christian Borntraeger 
Cc: Corey Minyard 
Cc: Cornelia Huck 
Cc: David Hildenbrand 
Cc: Halil Pasic 
Cc: Igor Mammedov 
Cc: Michael Roth 

Markus Armbruster (18):
  crypto: Fix certificate file error handling crash bug
  crypto: Fix typo in QCryptoTLSSession's  comment
  io: Fix Error usage in a comment 
  tests: Clean up initialization of Error *err variables
  exec: Fix file_ram_alloc() error API violations
  hw/acpi: Fix legacy CPU plug error API violations
  hw/core: Fix fit_load_fdt() error handling violations
  hw/ipmi: Fix realize() error API violations
  qga: Fix guest-get-fsinfo error API violations
  memory-device: Fix memory pre-plug error API violations
  s390x/event-facility: Fix realize() error API violations
  s390x/cpumodel: Fix feature property error API violations
  s390x/cpumodel: Fix realize() error API violations
  s390x/cpumodel: Fix query-cpu-model-FOO error API violations
  s390x/cpumodel: Fix query-cpu-definitions error API violations
  error: Clean up unusual names of Error * variables
  hw/intc/s390: Simplify error handling in kvm_s390_flic_realize()
  tests-blockjob: Use error_free_or_abort()

 include/crypto/tlssession.h |  2 +-
 include/io/task.h   |  2 +-
 crypto/tlscredsx509.c   |  2 +-
 exec.c  |  6 +-
 hw/acpi/cpu_hotplug.c   | 10 +--
 hw/core/loader-fit.c| 15 ++---
 hw/intc/s390_flic_kvm.c | 16 +++--
 hw/ipmi/isa_ipmi_bt.c   |  7 ++-
 hw/ipmi/isa_ipmi_kcs.c  |  7 ++-
 hw/ipmi/pci_ipmi_bt.c   |  6 +-
 hw/ipmi/pci_ipmi_kcs.c  |  6 +-
 hw/mem/memory-device.c  |  6 +-
 hw/ppc/spapr_pci.c  | 16 ++---
 hw/ppc/spapr_pci_nvlink2.c  | 10 +--
 hw/s390x/event-facility.c   |  6 +-
 qga/commands-posix.c|  6 +-
 target/s390x/cpu_models.c   | 98 +
 tests/test-blockjob.c   | 15 +++--
 tests/test-qobject-output-visitor.c |  8 +--
 tests/test-string-output-visitor.c  |  4 +-
 20 files changed, 139 insertions(+), 109 deletions(-)

-- 
2.21.0

[PATCH v2 10/18] memory-device: Fix memory pre-plug error API violations

[Markus Armbruster]

memory_device_get_free_addr() dereferences @errp when
memory_device_check_addable() fails.  That's wrong; see the big
comment in error.h.  Introduced in commit 1b6d6af21b "pc-dimm: factor
out capacity and slot checks into MemoryDevice".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: David Hildenbrand 
Signed-off-by: Markus Armbruster 
Reviewed-by: David Hildenbrand 
---
 hw/mem/memory-device.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index aef148c1d7..4bc9cf0917 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -99,6 +99,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
 uint64_t align, uint64_t size,
 Error **errp)
 {
+Error *err = NULL;
 GSList *list = NULL, *item;
 Range as, new = range_empty;
 
@@ -123,8 +124,9 @@ static uint64_t memory_device_get_free_addr(MachineState 
*ms,
 return 0;
 }
 
-memory_device_check_addable(ms, size, errp);
-if (*errp) {
+memory_device_check_addable(ms, size, );
+if (err) {
+error_propagate(errp, err);
 return 0;
 }
 
-- 
2.21.0

[Bug 1855072] [NEW] ARM: HCR.TVM traps are not implemented

[Julien Freche]

Public bug reported:

On AARCH64, setting HCR.TVM to 1 is supposed to trap all writes to
CTLR_EL1, TTBR0_EL1, TTBR1_EL1, TCR_EL1, ESR_EL1, FAR_EL1, AFSR0_EL1,
AFSR1_EL1, MAIR_EL1, AMAIR_EL1, and CONTEXTIDR_EL1. However, it
currently has no effect (QEMU emulator version 4.1.1).

It is also likely that TRVM will not trap, but, I didn't verify this.

** 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/1855072

Title:
  ARM: HCR.TVM traps are not implemented

Status in QEMU:
  New

Bug description:
  On AARCH64, setting HCR.TVM to 1 is supposed to trap all writes to
  CTLR_EL1, TTBR0_EL1, TTBR1_EL1, TCR_EL1, ESR_EL1, FAR_EL1, AFSR0_EL1,
  AFSR1_EL1, MAIR_EL1, AMAIR_EL1, and CONTEXTIDR_EL1. However, it
  currently has no effect (QEMU emulator version 4.1.1).

  It is also likely that TRVM will not trap, but, I didn't verify this.

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

Re: Network connection with COLO VM

[Zhang, Chen]

On 12/3/2019 9:25 PM, Dr. David Alan Gilbert wrote:

* Daniel Cho (daniel...@qnap.com) wrote:

Hi Dave,

We could use the exist interface to add netfilter and chardev, it might not
have the problem you said.

However, the netfilter and chardev on the primary at the start, that means
we could not dynamic set COLO
feature to VM?

I wasn't expecting that to be possible - I'd expect you to be able
to start in a state that looks the same as a primary+failed secondary;
but I'm not sure.


Current COLO (with Lukas's patch) can support dynamic set COLO system.

This status is same like the secondary has triggered failover, the 
primary node need to find new secondary


node to combine new COLO system.



We try to change this chardev to prevent primary VM will stuck to wait
secondary VM.

-chardev socket,id=compare1,host=127.0.0.1,port=9004,server,wait \

to

-chardev socket,id=compare1,host=127.0.0.1,port=9004,server,nowait \

But it will make primary VM's network not works. (Can't get ip), until
starting connect with secondary VM.


I think you need to check the port 9004 if already connect to the pair node.


I'm not sure of the answer to this; I've not tried doing it - I'm not
sure anyone has!
But, the colo components do track the state of tcp connections, so I'm
expecting that they have to already exist to have the state of those
connections available for when you start the secondary.


Yes, you are right.

For this status, we don't need to sync the state of tcp connections, 
because after failover


(or just solo COLO primary node), we have empty all the tcp connections 
state in COLO module.


In the processing of build new COLO pair, we will sync all the VM state 
to secondary node and re-build


new track things in COLO module.






Otherwise, the primary VM with netfileter / chardev and without netfilter /
chardev , they takes very different
booting time.
Without  netfilter / chardev : about 1 mins
With   netfilter / chardev : about 5 mins
Is this an issue?

that sounds like it needs investigating.

Dave


Yes, In previous COLO use cases, we need make primary node and secondary 
node boot in the same time.


I did't expect such a big difference on netfilter/chardev.

I think you can try without netfilter/chardev, after VM boot, re-build 
the netfilter/chardev related work with chardev server nowait.



Thanks

Zhang Chen




Best regards,
Daniel Cho


Dr. David Alan Gilbert  於 2019年12月2日 週一 下午5:58寫道：


* Daniel Cho (daniel...@qnap.com) wrote:

Hi Zhang,

We use qemu-4.1.0 release on this case.

I think we need use block mirror to sync the disk to secondary node

first,

then stop the primary VM and build COLO system.

In the stop moment, you need add some netfilter and chardev socket node

for

COLO, maybe you need re-check this part.


Our test was already follow those step. Maybe I could describe the detail
of the test flow and issues.


Step 1:

Create primary VM without any netfilter and chardev for COLO, and using
other host ping primary VM continually.


Step 2:

Create secondary VM (the same device/drive with primary VM), and do block
mirror sync ( ping to primary VM normally )


Step 3:

After block mirror sync finish, add those netfilter and chardev to

primary

VM and secondary VM for COLO ( *Can't* ping to primary VM but those

packets

will be received later )


Step 4:

Start migrate primary VM to secondary VM, and primary VM & secondary VM

are

running ( ping to primary VM works and receive those packets on step 3
status )




Between Step 3 to Step 4, it will take 10~20 seconds in our environment.

I could image this issue (delay reply packets) is because of setting COLO
proxy for temporary status,

but we thought 10~20 seconds might a little long. (If primary VM is

already

doing some jobs, it might lose the data.)


Could we reduce those time? or those delay is depends on different VM?

I think you need to set up the netfilter and chardev on the primary at
the start;  the filter contains the state of the TCP connections working
with the VM, so adding it later can't gain that state for existing
connections.

Dave


Best Regard,

Daniel Cho.



Zhang, Chen  於 2019年11月30日 週六 上午2:04寫道：





*From:* Daniel Cho 
*Sent:* Friday, November 29, 2019 10:43 AM
*To:* Zhang, Chen 
*Cc:* Dr. David Alan Gilbert ;

lukasstra...@web.de;

qemu-devel@nongnu.org
*Subject:* Re: Network connection with COLO VM



Hi David,  Zhang,



Thanks for replying my question.

We know why will occur this issue.

As you said, the COLO VM's network needs

colo-proxy to control packets, so the guest's

interface should set the filter to solve the problem.



But we found another question, when we set the

fault-tolerance feature to guest (primary VM is running,

secondary VM is pausing), the guest's network would not

responds any request for a while (in our environment

about 20~30 secs) after secondary VM runs.



Does it be a normal situation, or a known issue?



Our test is creating primary VM for a while, then 

Re: [PATCH] virtio-serial-bus: fix memory leak while attach virtio-serial-bus

[Laurent Vivier]

On 04/12/2019 04:02, pannengyuan wrote:
> 
> 
> On 2019/12/3 16:32, Laurent Vivier wrote:
>> On 03/12/2019 01:53, pannengyuan wrote:
>>>
>>>
>>> On 2019/12/2 21:58, Laurent Vivier wrote:
 On 02/12/2019 12:15, pannengy...@huawei.com wrote:
> From: PanNengyuan 
>
> ivqs/ovqs/c_ivq/c_ovq is forgot to cleanup in
> virtio_serial_device_unrealize, the memory leak stack is as bellow:
>
> Direct leak of 1290240 byte(s) in 180 object(s) allocated from:
> #0 0x7fc9bfc27560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
> #1 0x7fc9bed6f015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
> #2 0x5650e02b83e7 in virtio_add_queue 
> /mnt/sdb/qemu-4.2.0-rc0/hw/virtio/virtio.c:2327
> #3 0x5650e02847b5 in virtio_serial_device_realize 
> /mnt/sdb/qemu-4.2.0-rc0/hw/char/virtio-serial-bus.c:1089
> #4 0x5650e02b56a7 in virtio_device_realize 
> /mnt/sdb/qemu-4.2.0-rc0/hw/virtio/virtio.c:3504
> #5 0x5650e03bf031 in device_set_realized 
> /mnt/sdb/qemu-4.2.0-rc0/hw/core/qdev.c:876
> #6 0x5650e0531efd in property_set_bool 
> /mnt/sdb/qemu-4.2.0-rc0/qom/object.c:2080
> #7 0x5650e053650e in object_property_set_qobject 
> /mnt/sdb/qemu-4.2.0-rc0/qom/qom-qobject.c:26
> #8 0x5650e0533e14 in object_property_set_bool 
> /mnt/sdb/qemu-4.2.0-rc0/qom/object.c:1338
> #9 0x5650e04c0e37 in virtio_pci_realize 
> /mnt/sdb/qemu-4.2.0-rc0/hw/virtio/virtio-pci.c:1801
>
> Reported-by: Euler Robot 
> Signed-off-by: PanNengyuan 
> ---
>  hw/char/virtio-serial-bus.c | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 3325904..da9019a 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -1126,9 +1126,15 @@ static void 
> virtio_serial_device_unrealize(DeviceState *dev, Error **errp)
>  {
>  VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>  VirtIOSerial *vser = VIRTIO_SERIAL(dev);
> +int i;
>  
>  QLIST_REMOVE(vser, next);
>  
> +for (i = 0; i <= vser->bus.max_nr_ports; i++) {
> +virtio_del_queue(vdev, 2 * i);
> +virtio_del_queue(vdev, 2 * i + 1);
> +}
> +

 According to virtio_serial_device_realize() and the number of
 virtio_add_queue(), I think you have more queues to delete:

   4 + 2 * vser->bus.max_nr_ports

 (for vser->ivqs[0], vser->ovqs[0], vser->c_ivq, vser->c_ovq,
 vser->ivqs[i], vser->ovqs[i]).

 Thanks,
 Laurent


>>> Thanks, but I think the queues is correct, the queues in
>>> virtio_serial_device_realize is as follow:
>>>
>>> // here is 2
>>> vser->ivqs[0] = virtio_add_queue(vdev, 128, handle_input);
>>> vser->ovqs[0] = virtio_add_queue(vdev, 128, handle_output);
>>>
>>> // here is 2
>>> vser->c_ivq = virtio_add_queue(vdev, 32, control_in);
>>> vser->c_ovq = virtio_add_queue(vdev, 32, control_out);
>>>
>>> // here 2 * (max_nr_ports - 1)  - i is from 1 to max_nr_ports - 1
>>> for (i = 1; i < vser->bus.max_nr_ports; i++) {
>>> vser->ivqs[i] = virtio_add_queue(vdev, 128, handle_input);
>>> vser->ovqs[i] = virtio_add_queue(vdev, 128, handle_output);
>>> }
>>>
>>> so the total queues number is:  2 * (vser->bus.max_nr_ports + 1)
>>>
>>
>> Yes, you're right. A comment in the code would have helped or written
>> clearly like:
>>
>> for (i = 0; i < 2 * (vser->bus.max_nr_ports + 1); i++) {
>> virtio_del_queue(vdev, i);
>> }
>>
>> Thanks,
>> Laurent
>>
>>
> yes, it would be helpful, Michael S. Tsirkin posted another way to make
> it more clear, I will reuse it in next version.

Yes, the proposition from Michael is much more better.

Thanks,
Laurent

[PATCH v2 09/18] qga: Fix guest-get-fsinfo error API violations

[Markus Armbruster]

build_guest_fsinfo_for_virtual_device() dereferences @errp when
build_guest_fsinfo_for_device() fails.  That's wrong; see the big
comment in error.h.  Introduced in commit 46d4c5723e "qga: Add
guest-get-fsinfo command".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: Michael Roth 
Signed-off-by: Markus Armbruster 
---
 qga/commands-posix.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 1c1a165dae..0be527ccb8 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1049,6 +1049,7 @@ static void build_guest_fsinfo_for_virtual_device(char 
const *syspath,
   GuestFilesystemInfo *fs,
   Error **errp)
 {
+Error *err = NULL;
 DIR *dir;
 char *dirpath;
 struct dirent *entry;
@@ -1078,10 +1079,11 @@ static void build_guest_fsinfo_for_virtual_device(char 
const *syspath,
 
 g_debug(" slave device '%s'", entry->d_name);
 path = g_strdup_printf("%s/slaves/%s", syspath, entry->d_name);
-build_guest_fsinfo_for_device(path, fs, errp);
+build_guest_fsinfo_for_device(path, fs, );
 g_free(path);
 
-if (*errp) {
+if (err) {
+error_propagate(errp, err);
 break;
 }
 }
-- 
2.21.0

[PATCH v2 07/18] hw/core: Fix fit_load_fdt() error handling violations

[Markus Armbruster]

fit_load_fdt() passes @errp to fit_image_addr(), then recovers from
ENOENT failures.  Passing @errp is wrong, because it works only as
long as @errp is neither @error_fatal nor @error_abort.  Messed up in
commit 3eb99edb48 "loader-fit: Wean off error_printf()".

No caller actually passes such values.

Fix anyway: splice in a local Error *err, and error_propagate().

Signed-off-by: Markus Armbruster 
---
 hw/core/loader-fit.c | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
index 953b16bc82..c465921b8f 100644
--- a/hw/core/loader-fit.c
+++ b/hw/core/loader-fit.c
@@ -178,11 +178,12 @@ static int fit_load_fdt(const struct fit_loader *ldr, 
const void *itb,
 int cfg, void *opaque, const void *match_data,
 hwaddr kernel_end, Error **errp)
 {
+Error *err = NULL;
 const char *name;
 const void *data;
 const void *load_data;
 hwaddr load_addr;
-int img_off, err;
+int img_off;
 size_t sz;
 int ret;
 
@@ -197,13 +198,13 @@ static int fit_load_fdt(const struct fit_loader *ldr, 
const void *itb,
 return -EINVAL;
 }
 
-err = fit_image_addr(itb, img_off, "load", _addr, errp);
-if (err == -ENOENT) {
+ret = fit_image_addr(itb, img_off, "load", _addr, );
+if (ret == -ENOENT) {
 load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
-error_free(*errp);
-} else if (err) {
-error_prepend(errp, "unable to read FDT load address from FIT: ");
-ret = err;
+error_free(err);
+} else if (ret) {
+error_propagate_prepend(errp, err,
+"unable to read FDT load address from FIT: ");
 goto out;
 }
 
-- 
2.21.0

Re: [PATCH v2 02/18] crypto: Fix typo in QCryptoTLSSession's comment

[Daniel P . Berrangé]

On Wed, Dec 04, 2019 at 10:36:09AM +0100, Markus Armbruster wrote:
> Cc: "Daniel P. Berrangé" 
> Signed-off-by: Markus Armbruster 
> ---
>  include/crypto/tlssession.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Daniel P. Berrangé 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v6 3/9] qdev: add clock input support to devices.

[Philippe Mathieu-Daudé]

On 12/4/19 10:05 AM, Damien Hedde wrote:

On 12/2/19 3:34 PM, Peter Maydell wrote:

On Wed, 4 Sep 2019 at 13:56, Damien Hedde  wrote:



[...]

+/**
+ * qdev_pass_clock:
+ * @dev: the device to forward the clock to
+ * @name: the name of the clock to be added (can't be NULL)
+ * @container: the device which already has the clock
+ * @cont_name: the name of the clock in the container device
+ *
+ * Add a clock @name to @dev which forward to the clock @cont_name in 
@container
+ */


'container' seems odd terminology here, because I would expect
the usual use of this function to be when a 'container' object
like an SoC wants to forward a clock to one of its components;
in that case the 'container' SoC would be @dev, wouldn't it?


Yes. I agree it is confusing.
This function just allow a a device 'A' to exhibit a clock from another
device 'B' (typically a composing sub-device, inside a soc like you
said). The device A is not supposed to interact with the clock itself.
The original sub-device is the true
owner/controller of the clock and is the only one which should use the
clock API: setting a callback on it (input clock); or updating the clock
frequency (output clock).
Basically, it just add the clock into the device clock namespace without
interfering with it.


We should get this to be the same way round as qdev_pass_gpios(),
which takes "DeviceState *dev, DeviceState *container", and
passes the gpios that exist on 'dev' over to 'container' so that
'container' now has gpios which it did not before.


Ok, I'll invert the arguments.



Also, your use of 'forward to' is inconsistent: in the 'dev'
documentation you say we're forwarding the clock to 'dev',
but in the body of the documentation you say we're forwarding
the clock to the clock in 'container'.


I'll try to clarify this and make the documentation more consistent with
the comments here.



I think the way to resolve this is to stick to the terminology
in the function name itself:
  @dev: the device which has the clock
  @name: the name of the clock on @dev
  @container: the name of the device which the clock should
   be passed to
  @cont_name: the name to use for the clock on @container


I think container is confusing because depending on how we reason (clock
in a device; device in another device), we end up thinking the opposite.

Maybe we can use "exhibit" instead of "container" in the 2nd pair of
parameters, or prefix use "origin" or "owner" as a prefix for the first
pair of parameters.


@sink vs @source?


Q: if you pass a clock to another device with this function,
does it still exist to be used directly on the original
device? For qdev_pass_gpios it does not (I think), but
this is more accident of implementation than anything else.


It depends what we mean by "used by".
Original device can:
+ set the callback in case it is an input
+ update the frequency in case it is an output


Hmm here you use @input vs @output...


But since an input clock can only be connected once,
I think the logic here is that any connection should now go through the
new device. But this is not checked and using one or the other is
exactly the same.

Maybe I misunderstood the meaning of qdev_pass_gpios().

[...]

Re: [PATCH v2 03/13] s390x: protvirt: Support unpack facility

[Thomas Huth]

On 29/11/2019 10.47, Janosch Frank wrote:
> When a guest has saved a ipib of type 5 and call diagnose308 with
> subcode 10, we have to setup the protected processing environment via
> Ultravisor calls. The calls are done by KVM and are exposed via an API.
> 
> The following steps are necessary:
> 1. Create a VM (register it with the Ultravisor)
> 2. Create secure CPUs for all of our current cpus
> 3. Forward the secure header to the Ultravisor (has all information on
> how to decrypt the image and VM information)
> 4. Protect image pages from the host and decrypt them
> 5. Verify the image integrity
> 
> Only after step 5 a protected VM is allowed to run.
> 
> Signed-off-by: Janosch Frank 
> ---
[...]
> +++ b/hw/s390x/pv.c
> @@ -0,0 +1,118 @@
> +/*
> + * Secure execution functions
> + *
> + * Copyright IBM Corp. 2019
> + * Author(s):
> + *  Janosch Frank 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +#include "qemu/osdep.h"
> +#include 
> +
> +#include 
> +
> +#include "qemu/error-report.h"
> +#include "sysemu/kvm.h"
> +#include "pv.h"
> +
> +static int s390_pv_cmd(uint32_t cmd, void *data)
> +{
> +int rc;
> +struct kvm_pv_cmd pv_cmd = {
> +.cmd = cmd,
> +.data = (uint64_t)data,
> +};
> +
> +rc = kvm_vm_ioctl(kvm_state, KVM_S390_PV_COMMAND, _cmd);
> +if (rc) {
> +error_report("KVM PV command failed cmd: %d rc: %d", cmd, rc);
> +exit(1);
> +}
> +return rc;
> +}
> +
> +static int s390_pv_cmd_vcpu(CPUState *cs, uint32_t cmd, void *data)
> +{
> +int rc;
> +struct kvm_pv_cmd pv_cmd = {
> +.cmd = cmd,
> +.data = (uint64_t)data,
> +};
> +
> +rc = kvm_vcpu_ioctl(cs, KVM_S390_PV_COMMAND_VCPU, _cmd);
> +if (rc) {
> +error_report("KVM PV VCPU command failed cmd: %d rc: %d", cmd, rc);
> +exit(1);
> +}
> +return rc;
> +}
> +
> +int s390_pv_vm_create(void)
> +{
> +return s390_pv_cmd(KVM_PV_VM_CREATE, NULL);
> +}
> +
> +int s390_pv_vm_destroy(void)
> +{
> +return s390_pv_cmd(KVM_PV_VM_DESTROY, NULL);
> +}
> +
> +int s390_pv_vcpu_create(CPUState *cs)
> +{
> +return s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_CREATE, NULL);
> +}
> +
> +int s390_pv_vcpu_destroy(CPUState *cs)
> +{
> +S390CPU *cpu = S390_CPU(cs);
> +CPUS390XState *env = >env;
> +int rc;
> +
> +rc = s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_DESTROY, NULL);
> +if (!rc) {
> +env->pv = false;
> +}
> +return rc;
> +}
> +
> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
> +{
> +struct kvm_s390_pv_sec_parm args = {
> +.origin = origin,
> +.length = length,
> +};
> +
> +return s390_pv_cmd(KVM_PV_VM_SET_SEC_PARMS, );
> +}
> +
> +/*
> + * Called for each component in the SE type IPL parameter block 0.
> + */
> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak)
> +{
> +struct kvm_s390_pv_unp args = {
> +.addr = addr,
> +.size = size,
> +.tweak = tweak,
> +};
> +
> +return s390_pv_cmd(KVM_PV_VM_UNPACK, );
> +}
> +
> +int s390_pv_perf_clear_reset(void)
> +{
> +return s390_pv_cmd(KVM_PV_VM_PERF_CLEAR_RESET, NULL);
> +}
> +
> +int s390_pv_verify(void)
> +{
> +return s390_pv_cmd(KVM_PV_VM_VERIFY, NULL);
> +}
> +
> +int s390_pv_unshare(void)
> +{
> +return s390_pv_cmd(KVM_PV_VM_UNSHARE, NULL);
> +}
> diff --git a/hw/s390x/pv.h b/hw/s390x/pv.h
> new file mode 100644
> index 00..eb074e4bc9
> --- /dev/null
> +++ b/hw/s390x/pv.h
> @@ -0,0 +1,26 @@
> +/*
> + * Protected Virtualization header
> + *
> + * Copyright IBM Corp. 2019
> + * Author(s):
> + *  Janosch Frank 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#ifndef HW_S390_PV_H
> +#define HW_S390_PV_H
> +
> +int s390_pv_vm_create(void);
> +int s390_pv_vm_destroy(void);
> +int s390_pv_vcpu_destroy(CPUState *cs);
> +int s390_pv_vcpu_create(CPUState *cs);
> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
> +int s390_pv_perf_clear_reset(void);
> +int s390_pv_verify(void);
> +int s390_pv_unshare(void);

I still think you should make all those functions returning "void"
instead of "int" - since errors results in an exit() in s390_pv_cmd()
and s390_pv_cmd_vcpu() anyway...

> +
> +#endif /* HW_S390_PV_H */
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index c1d1440272..f9481ccace 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -41,6 +41,7 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/s390x/tod.h"
>  #include "sysemu/sysemu.h"
> +#include "hw/s390x/pv.h"
>  
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>  {
> @@ -322,6 +323,7 @@ static void 

[PATCH] target/i386: relax assert when old host kernels don't include msrs

[Catherine Ho]

Commit 20a78b02d315 ("target/i386: add VMX features") unconditionally
add vmx msr entry although older host kernels don't include them.

But old host kernel + newest qemu will cause a qemu crash as follows:
qemu-system-x86_64: error: failed to set MSR 0x480 to 0x0
target/i386/kvm.c:2932: kvm_put_msrs: Assertion `ret ==
cpu->kvm_msr_buf->nmsrs' failed.

This fixes it by relaxing the condition.

Cc: Paolo Bonzini 
Signed-off-by: Catherine Ho 
---
 target/i386/kvm.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index bf16556..a8c44bf 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -2936,7 +2936,7 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
  (uint32_t)e->index, (uint64_t)e->data);
 }
 
-assert(ret == cpu->kvm_msr_buf->nmsrs);
+assert(ret <= cpu->kvm_msr_buf->nmsrs);
 return 0;
 }
 
-- 
1.7.1

[PATCH v2 01/18] crypto: Fix certificate file error handling crash bug

[Markus Armbruster]

qcrypto_tls_creds_load_cert() passes uninitialized GError *gerr by
reference to g_file_get_contents().  When g_file_get_contents() fails,
it'll try to set a GError.  Unless @gerr is null by dumb luck, this
logs a ERROR_OVERWRITTEN_WARNING warning message and leaves @gerr
unchanged.  qcrypto_tls_creds_load_cert() then dereferences the
uninitialized @gerr.

Fix by initializing @gerr properly.

Fixes: 9a2fd4347c40321f5cbb4ab4220e759fcbf87d03
Cc: "Daniel P. Berrangé" 
Signed-off-by: Markus Armbruster 
---
 crypto/tlscredsx509.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index 01fc304e5d..53a4368f49 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -380,7 +380,7 @@ qcrypto_tls_creds_load_cert(QCryptoTLSCredsX509 *creds,
 gnutls_x509_crt_t cert = NULL;
 g_autofree char *buf = NULL;
 gsize buflen;
-GError *gerr;
+GError *gerr = NULL;
 int ret = -1;
 int err;
 
-- 
2.21.0

[PATCH v2 08/18] hw/ipmi: Fix realize() error API violations

[Markus Armbruster]

isa_ipmi_bt_realize(), ipmi_isa_realize(), pci_ipmi_bt_realize(), and
pci_ipmi_kcs_realize() dereference @errp when IPMIInterfaceClass
method init() fails.  That's wrong; see the big comment in error.h.
Introduced in commit 0719029c47 "ipmi: Add an ISA KCS low-level
interface", then imitated in commit a9b74079cb "ipmi: Add a BT
low-level interface" and commit 12f983c6aa "ipmi: Add PCI IPMI
interfaces".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: Corey Minyard 
Signed-off-by: Markus Armbruster 
---
 hw/ipmi/isa_ipmi_bt.c  | 7 +--
 hw/ipmi/isa_ipmi_kcs.c | 7 +--
 hw/ipmi/pci_ipmi_bt.c  | 6 --
 hw/ipmi/pci_ipmi_kcs.c | 6 --
 4 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
index 9a87ffd3f0..9fba5ed383 100644
--- a/hw/ipmi/isa_ipmi_bt.c
+++ b/hw/ipmi/isa_ipmi_bt.c
@@ -70,6 +70,7 @@ static void isa_ipmi_bt_lower_irq(IPMIBT *ib)
 
 static void isa_ipmi_bt_realize(DeviceState *dev, Error **errp)
 {
+Error *err = NULL;
 ISADevice *isadev = ISA_DEVICE(dev);
 ISAIPMIBTDevice *iib = ISA_IPMI_BT(dev);
 IPMIInterface *ii = IPMI_INTERFACE(dev);
@@ -85,9 +86,11 @@ static void isa_ipmi_bt_realize(DeviceState *dev, Error 
**errp)
 iib->bt.bmc->intf = ii;
 iib->bt.opaque = iib;
 
-iic->init(ii, 0, errp);
-if (*errp)
+iic->init(ii, 0, );
+if (err) {
+error_propagate(errp, err);
 return;
+}
 
 if (iib->isairq > 0) {
 isa_init_irq(isadev, >irq, iib->isairq);
diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
index ca3ea36a3f..cc6bd817f2 100644
--- a/hw/ipmi/isa_ipmi_kcs.c
+++ b/hw/ipmi/isa_ipmi_kcs.c
@@ -69,6 +69,7 @@ static void isa_ipmi_kcs_lower_irq(IPMIKCS *ik)
 
 static void ipmi_isa_realize(DeviceState *dev, Error **errp)
 {
+Error *err = NULL;
 ISADevice *isadev = ISA_DEVICE(dev);
 ISAIPMIKCSDevice *iik = ISA_IPMI_KCS(dev);
 IPMIInterface *ii = IPMI_INTERFACE(dev);
@@ -84,9 +85,11 @@ static void ipmi_isa_realize(DeviceState *dev, Error **errp)
 iik->kcs.bmc->intf = ii;
 iik->kcs.opaque = iik;
 
-iic->init(ii, 0, errp);
-if (*errp)
+iic->init(ii, 0, );
+if (err) {
+error_propagate(errp, err);
 return;
+}
 
 if (iik->isairq > 0) {
 isa_init_irq(isadev, >irq, iik->isairq);
diff --git a/hw/ipmi/pci_ipmi_bt.c b/hw/ipmi/pci_ipmi_bt.c
index 6ed925a665..ba9cf016b5 100644
--- a/hw/ipmi/pci_ipmi_bt.c
+++ b/hw/ipmi/pci_ipmi_bt.c
@@ -54,6 +54,7 @@ static void pci_ipmi_lower_irq(IPMIBT *ik)
 
 static void pci_ipmi_bt_realize(PCIDevice *pd, Error **errp)
 {
+Error *err = NULL;
 PCIIPMIBTDevice *pik = PCI_IPMI_BT(pd);
 IPMIInterface *ii = IPMI_INTERFACE(pd);
 IPMIInterfaceClass *iic = IPMI_INTERFACE_GET_CLASS(ii);
@@ -74,8 +75,9 @@ static void pci_ipmi_bt_realize(PCIDevice *pd, Error **errp)
 pik->bt.raise_irq = pci_ipmi_raise_irq;
 pik->bt.lower_irq = pci_ipmi_lower_irq;
 
-iic->init(ii, 8, errp);
-if (*errp) {
+iic->init(ii, 8, );
+if (err) {
+error_propagate(errp, err);
 return;
 }
 pci_register_bar(pd, 0, PCI_BASE_ADDRESS_SPACE_IO, >bt.io);
diff --git a/hw/ipmi/pci_ipmi_kcs.c b/hw/ipmi/pci_ipmi_kcs.c
index eeba63baa4..99f46152f4 100644
--- a/hw/ipmi/pci_ipmi_kcs.c
+++ b/hw/ipmi/pci_ipmi_kcs.c
@@ -54,6 +54,7 @@ static void pci_ipmi_lower_irq(IPMIKCS *ik)
 
 static void pci_ipmi_kcs_realize(PCIDevice *pd, Error **errp)
 {
+Error *err = NULL;
 PCIIPMIKCSDevice *pik = PCI_IPMI_KCS(pd);
 IPMIInterface *ii = IPMI_INTERFACE(pd);
 IPMIInterfaceClass *iic = IPMI_INTERFACE_GET_CLASS(ii);
@@ -74,8 +75,9 @@ static void pci_ipmi_kcs_realize(PCIDevice *pd, Error **errp)
 pik->kcs.raise_irq = pci_ipmi_raise_irq;
 pik->kcs.lower_irq = pci_ipmi_lower_irq;
 
-iic->init(ii, 8, errp);
-if (*errp) {
+iic->init(ii, 8, );
+if (err) {
+error_propagate(errp, err);
 return;
 }
 pci_register_bar(pd, 0, PCI_BASE_ADDRESS_SPACE_IO, >kcs.io);
-- 
2.21.0

[PATCH v2 14/18] s390x/cpumodel: Fix query-cpu-model-FOO error API violations

[Markus Armbruster]

cpu_model_from_info() is a helper for qmp_query_cpu_model_expansion(),
qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
dereferences @errp when the visitor or the QOM setter fails.  That's
wrong; see the big comment in error.h.  Introduced in commit
137974cea3 's390x/cpumodel: implement QMP interface
"query-cpu-model-expansion"'.

Its three callers have the same issue.  Introduced in commit
4e82ef0502 's390x/cpumodel: implement QMP interface
"query-cpu-model-comparison"' and commit f1a47d08ef 's390x/cpumodel:
implement QMP interface "query-cpu-model-baseline"'.

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: David Hildenbrand 
Cc: Cornelia Huck 
Signed-off-by: Markus Armbruster 
Reviewed-by: David Hildenbrand 
---
 target/s390x/cpu_models.c | 43 ---
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index c702e34a26..3ed301b5e5 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -477,6 +477,7 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error 
**errp)
 static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
 Error **errp)
 {
+Error *err = NULL;
 const QDict *qdict = NULL;
 const QDictEntry *e;
 Visitor *visitor;
@@ -513,24 +514,26 @@ static void cpu_model_from_info(S390CPUModel *model, 
const CpuModelInfo *info,
 
 if (qdict) {
 visitor = qobject_input_visitor_new(info->props);
-visit_start_struct(visitor, NULL, NULL, 0, errp);
-if (*errp) {
+visit_start_struct(visitor, NULL, NULL, 0, );
+if (err) {
+error_propagate(errp, err);
 visit_free(visitor);
 object_unref(obj);
 return;
 }
 for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
-object_property_set(obj, visitor, e->key, errp);
-if (*errp) {
+object_property_set(obj, visitor, e->key, );
+if (err) {
 break;
 }
 }
-if (!*errp) {
+if (!err) {
 visit_check_struct(visitor, errp);
 }
 visit_end_struct(visitor, NULL);
 visit_free(visitor);
-if (*errp) {
+if (err) {
+error_propagate(errp, err);
 object_unref(obj);
 return;
 }
@@ -595,13 +598,15 @@ CpuModelExpansionInfo 
*qmp_query_cpu_model_expansion(CpuModelExpansionType type,
   CpuModelInfo *model,
   Error **errp)
 {
+Error *err = NULL;
 CpuModelExpansionInfo *expansion_info = NULL;
 S390CPUModel s390_model;
 bool delta_changes = false;
 
 /* convert it to our internal representation */
-cpu_model_from_info(_model, model, errp);
-if (*errp) {
+cpu_model_from_info(_model, model, );
+if (err) {
+error_propagate(errp, err);
 return NULL;
 }
 
@@ -634,18 +639,21 @@ CpuModelCompareInfo 
*qmp_query_cpu_model_comparison(CpuModelInfo *infoa,
  CpuModelInfo *infob,
  Error **errp)
 {
+Error *err = NULL;
 CpuModelCompareResult feat_result, gen_result;
 CpuModelCompareInfo *compare_info;
 S390FeatBitmap missing, added;
 S390CPUModel modela, modelb;
 
 /* convert both models to our internal representation */
-cpu_model_from_info(, infoa, errp);
-if (*errp) {
+cpu_model_from_info(, infoa, );
+if (err) {
+error_propagate(errp, err);
 return NULL;
 }
-cpu_model_from_info(, infob, errp);
-if (*errp) {
+cpu_model_from_info(, infob, );
+if (err) {
+error_propagate(errp, err);
 return NULL;
 }
 compare_info = g_new0(CpuModelCompareInfo, 1);
@@ -707,6 +715,7 @@ CpuModelBaselineInfo 
*qmp_query_cpu_model_baseline(CpuModelInfo *infoa,
 CpuModelInfo *infob,
 Error **errp)
 {
+Error *err = NULL;
 CpuModelBaselineInfo *baseline_info;
 S390CPUModel modela, modelb, model;
 uint16_t cpu_type;
@@ -714,13 +723,15 @@ CpuModelBaselineInfo 
*qmp_query_cpu_model_baseline(CpuModelInfo *infoa,
 uint8_t max_gen;
 
 /* convert both models to our internal representation */
-cpu_model_from_info(, infoa, errp);
-if (*errp) {
+cpu_model_from_info(, infoa, );
+if (err) {
+error_propagate(errp, err);
 return NULL;
 }
 
-cpu_model_from_info(, infob, errp);
-if (*errp) {
+cpu_model_from_info(, infob, );
+if (err) {
+error_propagate(errp, err);
 return NULL;
 }
 
-- 
2.21.0

[PATCH v2 16/18] error: Clean up unusual names of Error * variables

[Markus Armbruster]

Local Error * variables are conventionally named @err or @local_err,
and Error ** parameters @errp.  Naming local variables like parameters
is confusing.  Clean that up.

Naming parameters like local variables is also confusing.  Left for
another day.

Signed-off-by: Markus Armbruster 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/intc/s390_flic_kvm.c| 10 +-
 hw/ppc/spapr_pci.c | 16 
 hw/ppc/spapr_pci_nvlink2.c | 10 +-
 tests/test-blockjob.c  | 16 
 4 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index c9ee80eaae..30d50c2369 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -582,10 +582,10 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error 
**errp)
 struct kvm_create_device cd = {0};
 struct kvm_device_attr test_attr = {0};
 int ret;
-Error *errp_local = NULL;
+Error *err = NULL;
 
-KVM_S390_FLIC_GET_CLASS(dev)->parent_realize(dev, _local);
-if (errp_local) {
+KVM_S390_FLIC_GET_CLASS(dev)->parent_realize(dev, );
+if (err) {
 goto fail;
 }
 flic_state->fd = -1;
@@ -593,7 +593,7 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error 
**errp)
 cd.type = KVM_DEV_TYPE_FLIC;
 ret = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, );
 if (ret < 0) {
-error_setg_errno(_local, errno, "Creating the KVM device failed");
+error_setg_errno(, errno, "Creating the KVM device failed");
 trace_flic_create_device(errno);
 goto fail;
 }
@@ -605,7 +605,7 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error 
**errp)
 KVM_HAS_DEVICE_ATTR, test_attr);
 return;
 fail:
-error_propagate(errp, errp_local);
+error_propagate(errp, err);
 }
 
 static void kvm_s390_flic_reset(DeviceState *dev)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index f6fbcf99ed..723373de73 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -2042,13 +2042,13 @@ void spapr_phb_dma_reset(SpaprPhbState *sphb)
 static void spapr_phb_reset(DeviceState *qdev)
 {
 SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(qdev);
-Error *errp = NULL;
+Error *err = NULL;
 
 spapr_phb_dma_reset(sphb);
 spapr_phb_nvgpu_free(sphb);
-spapr_phb_nvgpu_setup(sphb, );
-if (errp) {
-error_report_err(errp);
+spapr_phb_nvgpu_setup(sphb, );
+if (err) {
+error_report_err(err);
 }
 
 /* Reset the IOMMU state */
@@ -2326,7 +2326,7 @@ int spapr_dt_phb(SpaprMachineState *spapr, SpaprPhbState 
*phb,
 cpu_to_be32(phb->numa_node)};
 SpaprTceTable *tcet;
 SpaprDrc *drc;
-Error *errp = NULL;
+Error *err = NULL;
 
 /* Start populating the FDT */
 _FDT(bus_off = fdt_add_subnode(fdt, 0, phb->dtbusname));
@@ -2408,9 +2408,9 @@ int spapr_dt_phb(SpaprMachineState *spapr, SpaprPhbState 
*phb,
 return ret;
 }
 
-spapr_phb_nvgpu_populate_dt(phb, fdt, bus_off, );
-if (errp) {
-error_report_err(errp);
+spapr_phb_nvgpu_populate_dt(phb, fdt, bus_off, );
+if (err) {
+error_report_err(err);
 }
 spapr_phb_nvgpu_ram_populate_dt(phb, fdt);
 
diff --git a/hw/ppc/spapr_pci_nvlink2.c b/hw/ppc/spapr_pci_nvlink2.c
index 4aa89ede23..8332d5694e 100644
--- a/hw/ppc/spapr_pci_nvlink2.c
+++ b/hw/ppc/spapr_pci_nvlink2.c
@@ -57,7 +57,7 @@ struct SpaprPhbPciNvGpuConfig {
 uint64_t nv2_atsd_current;
 int num; /* number of non empty (i.e. tgt!=0) entries in slots[] */
 SpaprPhbPciNvGpuSlot slots[NVGPU_MAX_NUM];
-Error *errp;
+Error *err;
 };
 
 static SpaprPhbPciNvGpuSlot *
@@ -153,7 +153,7 @@ static void spapr_phb_pci_collect_nvgpu(PCIBus *bus, 
PCIDevice *pdev,
 spapr_pci_collect_nvnpu(nvgpus, pdev, tgt, MEMORY_REGION(mr_npu),
 _err);
 }
-error_propagate(>errp, local_err);
+error_propagate(>err, local_err);
 }
 if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
  PCI_HEADER_TYPE_BRIDGE)) {
@@ -187,9 +187,9 @@ void spapr_phb_nvgpu_setup(SpaprPhbState *sphb, Error 
**errp)
 pci_for_each_device(bus, pci_bus_num(bus),
 spapr_phb_pci_collect_nvgpu, sphb->nvgpus);
 
-if (sphb->nvgpus->errp) {
-error_propagate(errp, sphb->nvgpus->errp);
-sphb->nvgpus->errp = NULL;
+if (sphb->nvgpus->err) {
+error_propagate(errp, sphb->nvgpus->err);
+sphb->nvgpus->err = NULL;
 goto cleanup_exit;
 }
 
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index 7844c9ffcb..e670a20617 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -34,13 +34,13 @@ static BlockJob *mk_job(BlockBackend *blk, const char *id,
 int flags)
 {
 BlockJob *job;
-Error *errp = NULL;
+Error *err = NULL;
 
 job = block_job_create(id, drv, 

[Bug 1846427] Re: 4.1.0: qcow2 corruption on savevm/quit/loadvm cycle

[Kevin Wolf]

I don't see anything suspicious in that command line. My only idea for a
different configuration to test would be discard=off, which would remove
a few code paths that could contain a bug.

Anyway, I think it's pretty clear now that you're hitting a different
bug than Michael. Maybe it would be better to create a new report, too,
and to continue there.

Did you upgrade from 4.0 to 4.1 when you first hit the bug or was it
from an earlier version?

It would be perfect if you could bisect the problem like Michael did
with his, but I understand that this might not be possible soon.
Alternatively, I could also debug it myself if I had a clear reproducer
(that ideally doesn't involve Windows).

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

Title:
  4.1.0: qcow2 corruption on savevm/quit/loadvm cycle

Status in QEMU:
  Fix Committed

Bug description:
  I'm seeing massive corruption of qcow2 images with qemu 4.1.0 and git
  master as of 7f21573c822805a8e6be379d9bcf3ad9effef3dc after a few
  savevm/quit/loadvm cycles. I've narrowed it down to the following
  reproducer (further notes below):

  # qemu-img check debian.qcow2
  No errors were found on the image.
  251601/327680 = 76.78% allocated, 1.63% fragmented, 0.00% compressed clusters
  Image end offset: 18340446208
  # bin/qemu/bin/qemu-system-x86_64 -machine pc-q35-4.0.1,accel=kvm -m 4096 
-chardev stdio,id=charmonitor -mon chardev=charmonitor -drive 
file=debian.qcow2,id=d -S
  qemu-system-x86_64: warning: dbind: Couldn't register with accessibility bus: 
Did not receive a reply. Possible causes include: the remote application did 
not send a reply, the message bus security policy blocked the reply, the reply 
timeout expired, or the network connection was broken.
  QEMU 4.1.50 monitor - type 'help' for more information
  (qemu) loadvm foo
  (qemu) c
  (qemu) qcow2_free_clusters failed: Invalid argument
  qcow2_free_clusters failed: Invalid argument
  qcow2_free_clusters failed: Invalid argument
  qcow2_free_clusters failed: Invalid argument
  quit
  [m@nargothrond:~] qemu-img check debian.qcow2
  Leaked cluster 85179 refcount=2 reference=1
  Leaked cluster 85180 refcount=2 reference=1
  ERROR cluster 266150 refcount=0 reference=2
  [...]
  ERROR OFLAG_COPIED data cluster: l2_entry=42284 refcount=1

  9493 errors were found on the image.
  Data may be corrupted, or further writes to the image may corrupt it.

  2 leaked clusters were found on the image.
  This means waste of disk space, but no harm to data.
  259266/327680 = 79.12% allocated, 1.67% fragmented, 0.00% compressed clusters
  Image end offset: 18340446208

  This is on a x86_64 Linux 5.3.1 Gentoo host with qemu-system-x86_64
  and accel=kvm. The compiler is gcc-9.2.0 with the rest of the system
  similarly current.

  Reproduced with qemu-4.1.0 from distribution package as well as
  vanilla git checkout of tag v4.1.0 and commit
  7f21573c822805a8e6be379d9bcf3ad9effef3dc (today's master). Does not
  happen with qemu compiled from vanilla checkout of tag v4.0.0. Build
  sequence:

  ./configure --prefix=$HOME/bin/qemu-bisect --target-list=x86_64-softmmu 
--disable-werror --disable-docs
  [...]
  CFLAGS-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g
  [...] (can provide full configure output if helpful)
  make -j8 install

  The kind of guest OS does not matter: seen with Debian testing 64bit,
  Windows 7 x86/x64 BIOS and Windows 7 x64 EFI.

  The virtual storage controller does not seem to matter: seen with
  VirtIO SCSI, emulated SCSI and emulated SATA AHCI.

  Caching modes (none, directsync, writeback), aio mode (threads,
  native) or discard (ignore, unmap) or detect-zeroes (off, unmap) does
  not influence occurence either.

  Having more RAM in the guest seems to increase odds of corruption:
  With 512MB to the Debian guest problem hardly occurs at all, with 4GB
  RAM it happens almost instantly.

  An automated reproducer works as follows:

  - the guest *does* mount its root fs and swap with option discard and
  my testing leaves me with the impression that file deletion rather
  than reading is causing the issue

  - foo is a snapshot of the running Debian VM which is already running
  command

  # while true ; do dd if=/dev/zero of=foo bs=10240k count=400 ; done

  to produce some I/O to the disk (4GB file with 4GB of RAM).

  - on the host a loop continuously resumes and saves the guest state
  and quits qemu inbetween:

  # while true ; do (echo loadvm foo ; echo c ; sleep 10 ; echo stop ;
  echo savevm foo ; echo quit ) | bin/qemu-bisect/bin/qemu-system-x86_64
  -machine pc-q35-3.1,accel=kvm -m 4096 -chardev stdio,id=charmonitor
  -mon chardev=charmonitor -drive file=debian.qcow2,id=d -S -display
  none ; done

  - quitting qemu inbetween saves and loads seems to be necessary for
  the problem to occur. Just continusouly in one session saving and
  loading guest 

[PATCH v2 05/18] exec: Fix file_ram_alloc() error API violations

[Markus Armbruster]

When os_mem_prealloc() fails, file_ram_alloc() calls qemu_ram_munmap()
and returns null.  Except it doesn't when its @errp argument is null,
because it checks for failure with (errp && *errp).  Introduced in
commit 056b68af77 "fix qemu exit on memory hotplug when allocation
fails at prealloc time".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: Igor Mammedov 
Signed-off-by: Markus Armbruster 
Reviewed-by: Igor Mammedov 
---
 exec.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/exec.c b/exec.c
index ffdb518535..45695a5f2d 100644
--- a/exec.c
+++ b/exec.c
@@ -1841,6 +1841,7 @@ static void *file_ram_alloc(RAMBlock *block,
 bool truncate,
 Error **errp)
 {
+Error *err = NULL;
 MachineState *ms = MACHINE(qdev_get_machine());
 void *area;
 
@@ -1898,8 +1899,9 @@ static void *file_ram_alloc(RAMBlock *block,
 }
 
 if (mem_prealloc) {
-os_mem_prealloc(fd, area, memory, ms->smp.cpus, errp);
-if (errp && *errp) {
+os_mem_prealloc(fd, area, memory, ms->smp.cpus, );
+if (err) {
+error_propagate(errp, err);
 qemu_ram_munmap(fd, area, memory);
 return NULL;
 }
-- 
2.21.0

Re: [PATCH v4 15/40] target/arm: Expand TBFLAG_ANY.MMUIDX to 4 bits

[Alex Bennée]

Richard Henderson  writes:

> We are about to expand the number of mmuidx to 10, and so need 4 bits.
> For the benefit of reading the number out of -d exec, align it to the
> penultimate nibble.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  target/arm/cpu.h | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index ae9fc1ded3..5f295c7e60 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3176,17 +3176,17 @@ typedef ARMCPU ArchCPU;
>   * Unless otherwise noted, these bits are cached in env->hflags.
>   */
>  FIELD(TBFLAG_ANY, AARCH64_STATE, 31, 1)
> -FIELD(TBFLAG_ANY, MMUIDX, 28, 3)
> -FIELD(TBFLAG_ANY, SS_ACTIVE, 27, 1)
> -FIELD(TBFLAG_ANY, PSTATE_SS, 26, 1) /* Not cached. */
> +FIELD(TBFLAG_ANY, SS_ACTIVE, 30, 1)
> +FIELD(TBFLAG_ANY, PSTATE_SS, 29, 1) /* Not cached. */
> +FIELD(TBFLAG_ANY, BE_DATA, 28, 1)
> +FIELD(TBFLAG_ANY, MMUIDX, 24, 4)
>  /* Target EL if we take a floating-point-disabled exception */
> -FIELD(TBFLAG_ANY, FPEXC_EL, 24, 2)
> -FIELD(TBFLAG_ANY, BE_DATA, 23, 1)
> +FIELD(TBFLAG_ANY, FPEXC_EL, 22, 2)
>  /*
>   * For A-profile only, target EL for debug exceptions.
>   * Note that this overlaps with the M-profile-only HANDLER and STACKCHECK 
> bits.
>   */
> -FIELD(TBFLAG_ANY, DEBUG_TARGET_EL, 21, 2)
> +FIELD(TBFLAG_ANY, DEBUG_TARGET_EL, 20, 2)
>  
>  /*
>   * Bit usage when in AArch32 state, both A- and M-profile.


-- 
Alex Bennée

Re: Custom logic gates on user space emulation

[burak sarac]

Hello Alex,
 Thank you for your response, if I am on the right path I want to add
hadamard or one of pauli gate to gnu assembler then I want to run this
extended GAS via qemu using user space emulation. i.e.
https://en.wikipedia.org/wiki/Quantum_logic_gate#Hadamard_(H)_gate.
The idea is there are many quantum computer emulators i.e. here
https://www.quantiki.org/wiki/list-qc-simulators , yet I couldnt find
any one of them uses qemu. For beginning I wanted to try to port
libquantum hadamard impl.  There is https://github.com/Qiskit/openqasm
but source is not present.

Burak

burak sarac  writes:

> Hello All,
>  Currently I am studying qemu and I want to figure out how I can use
> custom logic gates on user space emulation.  I am searching very basic
> 'hello world' kind of tutorial or some resources to i.e. adding left
> or LOR : 1 | 0 = 1 but 0 | 1 = 0 to existing x86 arch
> ((https://en.wikibooks.org/wiki/X86_Assembly/Logic) ?).

It's not clear what you want to do. Are you looking to extend an
existing instruction set with additional custom instructions? Can you
explain why you want to do this?

> What I want to
> try is run this extended x86 version with qemu user space emulation.
> Do I need a custom toolchain also for this? I found this book:
> https://subscription.packtpub.com/book/hardware_and_creative/9781783289455/1/ch01lvl1sec15/generating-a-custom-toolchain-become-an-expert

For testing you don't need a custom toolchain - you can use inline
assembly with data statements to insert your custom instructions into a
program. Again it depends on what your eventual aim is here.

>
> Sorry for my ignorance in case it is totally irrelevant and I would
> appreciate any guidance! Or pseudo kind of road map for me!
>
> Thank you & have a nice day
> Burak


-- 
Alex Bennée

Re: [PATCH v6 7/9] hw/misc/zynq_slcr: add clock generation for uarts

[Damien Hedde]

On 12/2/19 4:20 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde  wrote:
>>
>> Switch the slcr to multi-phase reset and add some clocks:
>> + the main input clock (ps_clk)
>> + the reference clock outputs for each uart (uart0 & 1)
>>
>> The clock frequencies are computed using the internal pll & uart 
>> configuration
>> registers and the ps_clk frequency.
>>
>> Signed-off-by: Damien Hedde 
> 
> Review of this and the following two patches by some Xilinx
> person would be nice. I've just looked them over for general
> issues, and haven't checked against the hardware specs.
> 
>> ---
> 
> 
>> +/*
>> + * return the output frequency of a clock given:
>> + * + the frequencies in an array corresponding to mux's indexes
>> + * + the register xxx_CLK_CTRL value
>> + * + enable bit index in ctrl register
>> + *
>> + * This function make the assumption that ctrl_reg value is organized as 
>> follow:
> 
> "makes"; "that the"; "follows"
> 
>> + * + bits[13:8] clock divisor
>> + * + bits[5:4]  clock mux selector (index in array)
>> + * + bits[index] clock enable
>> + */
>> +static uint64_t zynq_slcr_compute_clock(const uint64_t mux[],
>> +uint32_t ctrl_reg,
>> +unsigned index)
>> +{
>> +uint32_t srcsel = extract32(ctrl_reg, 4, 2); /* bits [5:4] */
>> +uint32_t divisor = extract32(ctrl_reg, 8, 6); /* bits [13:8] */
>> +
>> +/* first, check if clock is enabled */
>> +if (((ctrl_reg >> index) & 1u) == 0) {
>> +return 0;
>> +}
>> +
>> +/*
>> + * according to the Zynq technical ref. manual UG585 v1.12.2 in
>> + * "Clocks" chapter, section 25.10.1 page 705" the range of the divisor
>> + * is [1;63].
> 
> Is this the range notation the spec doc uses?

The exact terms is:
"The 6-bit divider provides a divide range of 1 to 63"
At the time, I checked also the kernel sources, and this is the behavior
implemented in the driver as well (1 based timer and allowing 0 special
value for bypass). The bypass is undocumented as far as I can tell.

> 
>> + * So divide the source while avoiding division-by-zero.
>> + */
>> +return mux[srcsel] / (divisor ? divisor : 1u);
>> +}
>> +
> 
>> +static const ClockPortInitArray zynq_slcr_clocks = {
>> +QDEV_CLOCK_IN(ZynqSLCRState, ps_clk, zynq_slcr_ps_clk_callback),
>> +QDEV_CLOCK_OUT(ZynqSLCRState, uart0_ref_clk),
>> +QDEV_CLOCK_OUT(ZynqSLCRState, uart1_ref_clk),
>> +QDEV_CLOCK_END
>> +};
>> +
>>  static void zynq_slcr_init(Object *obj)
>>  {
>>  ZynqSLCRState *s = ZYNQ_SLCR(obj);
>> @@ -425,6 +559,8 @@ static void zynq_slcr_init(Object *obj)
>>  memory_region_init_io(>iomem, obj, _ops, s, "slcr",
>>ZYNQ_SLCR_MMIO_SIZE);
>>  sysbus_init_mmio(SYS_BUS_DEVICE(obj), >iomem);
>> +
>> +qdev_init_clocks(DEVICE(obj), zynq_slcr_clocks);
>>  }
>>
>>  static const VMStateDescription vmstate_zynq_slcr = {
>> @@ -440,9 +576,12 @@ static const VMStateDescription vmstate_zynq_slcr = {
>>  static void zynq_slcr_class_init(ObjectClass *klass, void *data)
>>  {
>>  DeviceClass *dc = DEVICE_CLASS(klass);
>> +ResettableClass *rc = RESETTABLE_CLASS(klass);
>>
>>  dc->vmsd = _zynq_slcr;
>> -dc->reset = zynq_slcr_reset;
>> +rc->phases.init = zynq_slcr_reset_init;
>> +rc->phases.hold = zynq_slcr_reset_hold;
>> +rc->phases.exit = zynq_slcr_reset_exit;
>>  }
> 
> We're adding an input clock, so doesn't the migration
> state struct need to be updated to migrate it ?
Yes, we can. Although this input clock is really not expected to change.

> 
> thanks
> -- PMM
> 

Re: virtiofsd: Where should it live?

[Kevin Wolf]

Am 04.12.2019 um 09:17 hat Gerd Hoffmann geschrieben:
>   Hi,
> 
> > > |   ...
> > > +- qemu-edid
> > 
> > Has its own MAINTAINERS section, together with hw/display/edit* and
> > include/hw/display/edid.h.  I'm not sure moving it hw/display/ is a good
> > idea.  Gerd?
> 
> Sort-of makes sense.  My personal preference would be a tools/ directory
> for all those small utilities though.

I think I would like that better than throwing tools into block/ where
currently mostly just block drivers live.

Or, if we want to move the tools there, we'd need another directory
level inside block/ to keep things reasonably organised.

Kevin

Re: [PATCH v6 8/9] hw/char/cadence_uart: add clock support

[Damien Hedde]

On 12/2/19 4:24 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde  wrote:
>>
>> Switch the cadence uart to multi-phase reset and add the
>> reference clock input.
>>
>> The input clock frequency is added to the migration structure.
>>
>> The reference clock controls the baudrate generation. If it disabled,
>> any input characters and events are ignored.
>>
>> If this clock remains unconnected, the uart behaves as before
>> (it default to a 50MHz ref clock).
>>
>> Signed-off-by: Damien Hedde 
> 
>>  static void uart_parameters_setup(CadenceUARTState *s)
>>  {
>>  QEMUSerialSetParams ssp;
>> -unsigned int baud_rate, packet_size;
>> +unsigned int baud_rate, packet_size, input_clk;
>> +input_clk = clock_get_frequency(s->refclk);
>>
>> -baud_rate = (s->r[R_MR] & UART_MR_CLKS) ?
>> -UART_INPUT_CLK / 8 : UART_INPUT_CLK;
>> +baud_rate = (s->r[R_MR] & UART_MR_CLKS) ? input_clk / 8 : input_clk;
>> +baud_rate /= (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
>> +trace_cadence_uart_baudrate(baud_rate);
>> +
>> +ssp.speed = baud_rate;
>>
>> -ssp.speed = baud_rate / (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
>>  packet_size = 1;
>>
>>  switch (s->r[R_MR] & UART_MR_PAR) {
>> @@ -215,6 +220,13 @@ static void uart_parameters_setup(CadenceUARTState *s)
>>  }
>>
>>  packet_size += ssp.data_bits + ssp.stop_bits;
>> +if (ssp.speed == 0) {
>> +/*
>> + * Avoid division-by-zero below.
>> + * TODO: find something better
>> + */
> 
> Any ideas what might be better? :-)

Well maybe the comment is misplaced. Because it is probably a good thing
to round up the ssp.speed in case it becomes 0 (which is very unlikely
apart from the case where the input clock is 0/disabled).

The problem is what should we do when the clock is disabled ?
Right now we:
+ set a minimal baudrate
+ ignore input characters/events
+ still forward output characters... (I just checked)

I suppose we could at least fix the last point: we can drop any output
characters. But if this happen, there is definitely a problem somewhere
(a firmware should not try to send characters to an unclocked uart). Is
there a qemu way of reporting this kind of situation ?

It would be best to somehow tell the backend we're not handling anything
anymore. So I could put that in the comment instead.

I really don't know if/how we can do that. When I looked I did not see
any way to do the opposite of qemu_chr_fe_accept_input() which is done
to start receiving stuff.

--
Damien

Re: [PATCH v3] travis.yml: Run tcg tests with tci

[Thomas Huth]

On 04/12/2019 14.48, Alex Bennée wrote:
> 
> Thomas Huth  writes:
> 
>> So far we only have compile coverage for tci. But since commit
>> 2f160e0f9797c7522bfd0d09218d0c9340a5137c ("tci: Add implementation
>> for INDEX_op_ld16u_i64") has been included now, we can also run the
>> "tcg" and "qtest" tests with tci, so let's enable them in Travis now.
>> Since we don't gain much additional test coverage by compiling all
>> targets, and TCI is broken e.g. with the Sparc targets, we also limit
>> the target list to a reasonable subset now (which should still get us
>> test coverage by tests/boot-serial-test for example).
> 
> Queued to testing/next, thanks.

Thanks! But could you maybe s/--enable-debug/--enable-debug-tcg/ as
Richard just suggested in his mail? Or want me to send a v4?

 Thomas

Re: [PATCH 0/6] Enable Travis builds on arm64, ppc64le and s390x

[Thomas Huth]

On 27/11/2019 09.50, Thomas Huth wrote:
> On 25/11/2019 11.28, Alex Bennée wrote:
>>
>> Alex Bennée  writes:
>>
>>> Thomas Huth  writes:
>>>
 Travis recently added build hosts for arm64, ppc64le and s390x, so
 this is a welcome addition to our Travis testing matrix.

 Unfortunately, the builds are running in quite restricted LXD
 containers
 there, for example it is not possible to create huge files there (even
 if they are just sparse), and certain system calls are blocked. So we
 have to change some tests first to stop them failing in such
 environments.
>>> 
    iotests: Skip test 060 if it is not possible to create large files
    iotests: Skip test 079 if it is not possible to create large files
>>>
>>> It seems like 161 is also failing:
>>>
>>>    https://travis-ci.org/stsquad/qemu/jobs/615672478
>>
>> And sometimes 249
> 
> These must be intermittent problems ... I've seen 161 failing once at
> the very beginning of my tests, but then never again, so I assumed that
> it was a quirk with the test system that got fixed later. Seems like
> that was a wrong assumption. I've never seen 249 failing so far... I'll
> try to do some more tests when I've got some spare time...

It's not intermittent, it's a problem with "dist: bionic". It seems to
work fine with "dist: xenial", as far as I can tell. So I think we
should simply stick with "dist: xenial" for the ppc64le builder unless
someone has a ppc64le bionic system at hand for debugging.

 Thomas

Re: [PATCH v4 19/40] target/arm: Add regime_has_2_ranges

[Alex Bennée]

Richard Henderson  writes:

> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  target/arm/internals.h | 16 
>  target/arm/helper.c| 23 ++-
>  target/arm/translate-a64.c |  3 +--
>  3 files changed, 23 insertions(+), 19 deletions(-)
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index d73615064c..1ca9a7cc78 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -837,6 +837,22 @@ static inline void arm_call_el_change_hook(ARMCPU *cpu)
>  }
>  }
>  
> +/* Return true if this address translation regime has two ranges.  */
> +static inline bool regime_has_2_ranges(ARMMMUIdx mmu_idx)
> +{
> +switch (mmu_idx) {
> +case ARMMMUIdx_Stage1_E0:
> +case ARMMMUIdx_Stage1_E1:
> +case ARMMMUIdx_EL10_0:
> +case ARMMMUIdx_EL10_1:
> +case ARMMMUIdx_EL20_0:
> +case ARMMMUIdx_EL20_2:
> +return true;
> +default:
> +return false;
> +}
> +}
> +
>  /* Return true if this address translation regime is secure */
>  static inline bool regime_is_secure(CPUARMState *env, ARMMMUIdx mmu_idx)
>  {
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index f86285ffbe..27adf24fa6 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -8885,15 +8885,8 @@ static int get_S1prot(CPUARMState *env, ARMMMUIdx 
> mmu_idx, bool is_aa64,
>  }
>  
>  if (is_aa64) {
> -switch (regime_el(env, mmu_idx)) {
> -case 1:
> -if (!is_user) {
> -xn = pxn || (user_rw & PAGE_WRITE);
> -}
> -break;
> -case 2:
> -case 3:
> -break;
> +if (regime_has_2_ranges(mmu_idx) && !is_user) {
> +xn = pxn || (user_rw & PAGE_WRITE);
>  }
>  } else if (arm_feature(env, ARM_FEATURE_V7)) {
>  switch (regime_el(env, mmu_idx)) {
> @@ -9427,7 +9420,6 @@ ARMVAParameters aa64_va_parameters_both(CPUARMState 
> *env, uint64_t va,
>  ARMMMUIdx mmu_idx)
>  {
>  uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr;
> -uint32_t el = regime_el(env, mmu_idx);
>  bool tbi, tbid, epd, hpd, using16k, using64k;
>  int select, tsz;
>  
> @@ -9437,7 +9429,7 @@ ARMVAParameters aa64_va_parameters_both(CPUARMState 
> *env, uint64_t va,
>   */
>  select = extract64(va, 55, 1);
>  
> -if (el > 1) {
> +if (!regime_has_2_ranges(mmu_idx)) {
>  tsz = extract32(tcr, 0, 6);
>  using64k = extract32(tcr, 14, 1);
>  using16k = extract32(tcr, 15, 1);
> @@ -9593,10 +9585,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> target_ulong address,
>  param = aa64_va_parameters(env, address, mmu_idx,
> access_type != MMU_INST_FETCH);
>  level = 0;
> -/* If we are in 64-bit EL2 or EL3 then there is no TTBR1, so mark it
> - * invalid.
> - */
> -ttbr1_valid = (el < 2);
> +ttbr1_valid = regime_has_2_ranges(mmu_idx);
>  addrsize = 64 - 8 * param.tbi;
>  inputsize = 64 - param.tsz;
>  } else {
> @@ -11306,8 +11295,8 @@ static uint32_t rebuild_hflags_a64(CPUARMState *env, 
> int el, int fp_el,
>  
>  flags = FIELD_DP32(flags, TBFLAG_ANY, AARCH64_STATE, 1);
>  
> -/* FIXME: ARMv8.1-VHE S2 translation regime.  */
> -if (regime_el(env, stage1) < 2) {
> +/* Get control bits for tagged addresses.  */
> +if (regime_has_2_ranges(mmu_idx)) {
>  ARMVAParameters p1 = aa64_va_parameters_both(env, -1, stage1);
>  tbid = (p1.tbi << 1) | p0.tbi;
>  tbii = tbid & ~((p1.tbid << 1) | p0.tbid);
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 885c99f0c9..d0b65c49e2 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -175,8 +175,7 @@ static void gen_top_byte_ignore(DisasContext *s, TCGv_i64 
> dst,
>  if (tbi == 0) {
>  /* Load unmodified address */
>  tcg_gen_mov_i64(dst, src);
> -} else if (s->current_el >= 2) {
> -/* FIXME: ARMv8.1-VHE S2 translation regime.  */
> +} else if (!regime_has_2_ranges(s->mmu_idx)) {
>  /* Force tag byte to all zero */
>  tcg_gen_extract_i64(dst, src, 0, 56);
>  } else {


-- 
Alex Bennée

Re: [PATCH v3] travis.yml: Run tcg tests with tci

[Alex Bennée]

Thomas Huth  writes:

> On 04/12/2019 14.48, Alex Bennée wrote:
>> 
>> Thomas Huth  writes:
>> 
>>> So far we only have compile coverage for tci. But since commit
>>> 2f160e0f9797c7522bfd0d09218d0c9340a5137c ("tci: Add implementation
>>> for INDEX_op_ld16u_i64") has been included now, we can also run the
>>> "tcg" and "qtest" tests with tci, so let's enable them in Travis now.
>>> Since we don't gain much additional test coverage by compiling all
>>> targets, and TCI is broken e.g. with the Sparc targets, we also limit
>>> the target list to a reasonable subset now (which should still get us
>>> test coverage by tests/boot-serial-test for example).
>> 
>> Queued to testing/next, thanks.
>
> Thanks! But could you maybe s/--enable-debug/--enable-debug-tcg/ as
> Richard just suggested in his mail? Or want me to send a v4?

I fixed it in my tree, no need to send v4

>
>  Thomas


-- 
Alex Bennée

Re: [PATCH v2 3/3] virtio-serial-bus: fix memory leak while attach virtio-serial-bus

[Eric Blake]

On 12/4/19 1:31 AM, pannengy...@huawei.com wrote:

From: Pan Nengyuan 

ivqs/ovqs/c_ivq/c_ovq is forgot to cleanup in


s/is //


virtio_serial_device_unrealize, the memory leak stack is as bellow:


below



Direct leak of 1290240 byte(s) in 180 object(s) allocated from:
 #0 0x7fc9bfc27560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
 #1 0x7fc9bed6f015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
 #2 0x5650e02b83e7 in virtio_add_queue hw/virtio/virtio.c:2327
 #3 0x5650e02847b5 in virtio_serial_device_realize 
hw/char/virtio-serial-bus.c:1089
 #4 0x5650e02b56a7 in virtio_device_realize hw/virtio/virtio.c:3504
 #5 0x5650e03bf031 in device_set_realized hw/core/qdev.c:876
 #6 0x5650e0531efd in property_set_bool qom/object.c:2080
 #7 0x5650e053650e in object_property_set_qobject qom/qom-qobject.c:26
 #8 0x5650e0533e14 in object_property_set_bool qom/object.c:1338
 #9 0x5650e04c0e37 in virtio_pci_realize hw/virtio/virtio-pci.c:1801

Reported-by: Euler Robot 
Signed-off-by: Pan Nengyuan 
Cc: Laurent Vivier 
Cc: Amit Shah 
Cc: "Marc-André Lureau" 
Cc: Paolo Bonzini 
---

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

Re: [PATCH v3 2/4] s390x: Add missing vcpu reset functions

[Cornelia Huck]

On Tue,  3 Dec 2019 08:28:11 -0500
Janosch Frank  wrote:

> Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach
> for the initial reset, and that was also called for the clear
> reset. To be architecture compliant, we also need to clear local
> interrupts on a normal reset.

Do we also need to do something like that for tcg? David?

> 
> Because of this and the upcoming protvirt support we need to add
> ioctls for the missing clear and normal resets.
> 
> Signed-off-by: Janosch Frank 
> Reviewed-by: Thomas Huth 
> Acked-by: David Hildenbrand 
> ---
>  target/s390x/cpu.c   | 16 +--
>  target/s390x/kvm-stub.c  | 10 +-
>  target/s390x/kvm.c   | 42 
>  target/s390x/kvm_s390x.h |  4 +++-
>  4 files changed, 60 insertions(+), 12 deletions(-)

Re: [PATCH v6] error: rename errp to errp_in where it is IN-argument

[Vladimir Sementsov-Ogievskiy]

04.12.2019 16:03, Markus Armbruster wrote:
> Markus Armbruster  writes:
> 
>> Vladimir Sementsov-Ogievskiy  writes:
>>
>>> 29.11.2019 17:35, Markus Armbruster wrote:
> [...]
 I pushed my fixups to git://repo.or.cz/qemu/armbru.git branch error-prep
 for your convenience.  The commit messages of the fixed up commits need
 rephrasing, of course.

>>>
>>> Looked through fixups, looks OK for me, thanks! What next?
>>
>> Let me finish my fixing incorrect dereferences of errp, and then we
>> figure out what to include in v7.
> 
> Your v6 with my fixups does not conflict with my "[PATCH v2 00/18] Error
> handling fixes", except for "hw/core/loader-fit: fix freeing errp in
> fit_load_fdt", which my "[PATCH v2 07/18] hw/core: Fix fit_load_fdt()
> error handling violations" supersedes.
> 
> Suggest you work in the fixups and post as v7.  I'll merge that in my
> tree, to give you a base for the remainder of your "auto propagated
> local_err" work.  While you work on that, I'll work on getting the base
> merged into master.  Sounds like a plan?
> 

Yes, that's good. I'll send v7 tomorrow.

What you suggest to do after it?
Send in one series a patch with macro + coccinelle +
subset of autogenerated patches, which were reviewed (but not sending half
a subsystem of course)?

-- 
Best regards,
Vladimir

Re: [PATCH v3 2/4] s390x: Add missing vcpu reset functions

[David Hildenbrand]

On 04.12.19 16:07, David Hildenbrand wrote:
> On 04.12.19 15:59, Cornelia Huck wrote:
>> On Tue,  3 Dec 2019 08:28:11 -0500
>> Janosch Frank  wrote:
>>
>>> Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach
>>> for the initial reset, and that was also called for the clear
>>> reset. To be architecture compliant, we also need to clear local
>>> interrupts on a normal reset.
>>
>> Do we also need to do something like that for tcg? David?
>>
> 
> So, we have
> 
> /* Fields up to this point are not cleared by initial CPU reset */
> struct {} start_initial_reset_fields;
> [...]
> int pending_int
> uint16_t external_call_addr;
> DECLARE_BITMAP(emergency_signals, S390_MAX_CPUS);
> [...]
> /* Fields up to this point are cleared by a CPU reset */
> struct {} end_reset_fields;
> 
> This means, local interrupts will be cleared by everything that zeroes
> "start_initial_reset_fields->end_reset_fields"
> 
> So, they will get cleared by S390_CPU_RESET_INITIAL only if I am not
> wrong. In order to clear them on S390_CPU_RESET_NORMAL, we have to
> manually set them to zero.

Sorry, by S390_CPU_RESET_INITIAL and S390_CPU_RESET_CLEAR.

> 
> (we really should wrap TCG-only fields by ifdefs)
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH v3 2/4] s390x: Add missing vcpu reset functions

[David Hildenbrand]

On 04.12.19 15:59, Cornelia Huck wrote:
> On Tue,  3 Dec 2019 08:28:11 -0500
> Janosch Frank  wrote:
> 
>> Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach
>> for the initial reset, and that was also called for the clear
>> reset. To be architecture compliant, we also need to clear local
>> interrupts on a normal reset.
> 
> Do we also need to do something like that for tcg? David?
> 

So, we have

/* Fields up to this point are not cleared by initial CPU reset */
struct {} start_initial_reset_fields;
[...]
int pending_int
uint16_t external_call_addr;
DECLARE_BITMAP(emergency_signals, S390_MAX_CPUS);
[...]
/* Fields up to this point are cleared by a CPU reset */
struct {} end_reset_fields;

This means, local interrupts will be cleared by everything that zeroes
"start_initial_reset_fields->end_reset_fields"

So, they will get cleared by S390_CPU_RESET_INITIAL only if I am not
wrong. In order to clear them on S390_CPU_RESET_NORMAL, we have to
manually set them to zero.

(we really should wrap TCG-only fields by ifdefs)

-- 
Thanks,

David / dhildenb

Re: [PATCH] target/i386: relax assert when old host kernels don't include msrs

[Catherine Ho]

Hi Paolo


On Wed, 4 Dec 2019 at 21:53, Paolo Bonzini  wrote:
>
> On 04/12/19 14:33, Catherine Ho wrote:
> > Hi Paolo
> > [sorry to resend it, seems to reply it incorrectly]
> >
> > On Wed, 4 Dec 2019 at 19:23, Paolo Bonzini  > > wrote:
> >
> > On 04/12/19 09:50, Catherine Ho wrote:
> > > Commit 20a78b02d315 ("target/i386: add VMX features") unconditionally
> > > add vmx msr entry although older host kernels don't include them.
> > >
> > > But old host kernel + newest qemu will cause a qemu crash as follows:
> > > qemu-system-x86_64: error: failed to set MSR 0x480 to 0x0
> > > target/i386/kvm.c:2932: kvm_put_msrs: Assertion `ret ==
> > > cpu->kvm_msr_buf->nmsrs' failed.
> > >
> > > This fixes it by relaxing the condition.
> >
> > This is intentional.  The VMX MSR entries should not have been added.
> > What combination of host kernel/QEMU are you using, and what QEMU
> > command line?
> >
> >
> > Host kernel: 4.15.0 (ubuntu 18.04)
> > Qemu: https://gitlab.com/virtio-fs/qemu/tree/virtio-fs-dev
> > cmdline: qemu-system-x86_64 -M pc -cpu host --enable-kvm -smp 8 \
> >   -m 4G,maxmem=4G
> >
> > But before 20a78b02d315, the older kernel + latest qemu can boot guest
> > successfully.
>
> Ok, so the problem is that some MSR didn't exist in that version.  Which
I thought in my platform, the only MSR didn't exist is MSR_IA32_VMX_BASIC
(0x480). If I remove this kvm_msr_entry_add(), everything is ok, the guest can
be boot up successfully.

> one it is?  Can you make it conditional, similar to MSR_IA32_VMX_VMFUNC?
Ok, I will. Thanks for the suggestion

Best regards
Catherine

Re: [PATCH] target/i386: relax assert when old host kernels don't include msrs

[Paolo Bonzini]

On 04/12/19 09:50, Catherine Ho wrote:
> Commit 20a78b02d315 ("target/i386: add VMX features") unconditionally
> add vmx msr entry although older host kernels don't include them.
> 
> But old host kernel + newest qemu will cause a qemu crash as follows:
> qemu-system-x86_64: error: failed to set MSR 0x480 to 0x0
> target/i386/kvm.c:2932: kvm_put_msrs: Assertion `ret ==
> cpu->kvm_msr_buf->nmsrs' failed.
> 
> This fixes it by relaxing the condition.

This is intentional.  The VMX MSR entries should not have been added.
What combination of host kernel/QEMU are you using, and what QEMU
command line?

Paolo

Re: [PATCH] Revert "qemu-options.hx: Update for reboot-timeout parameter"

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191204085628.2892-1-h...@redhat.com/



Hi,

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

Subject: [PATCH] Revert "qemu-options.hx: Update for reboot-timeout parameter"
Type: series
Message-id: 20191204085628.2892-1-h...@redhat.com

=== 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'
6803338 Revert "qemu-options.hx: Update for reboot-timeout parameter"

=== OUTPUT BEGIN ===
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 10 lines checked

Commit 680333822026 (Revert "qemu-options.hx: Update for reboot-timeout 
parameter") has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


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

Re: [PATCH] Revert "qemu-options.hx: Update for reboot-timeout parameter"

[Markus Armbruster]

Han Han  writes:

> This reverts commit bbd9e6985ff342cbe15b9cb7eb30e842796fbbe8.
>
> In 20a1922032 we allowed reboot-timeout=-1 again, so update the doc
> accordingly.
> ---
>  qemu-options.hx | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 65c9473b73..e14d88e9b2 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -327,8 +327,8 @@ format(true color). The resolution should be supported by 
> the SVGA mode, so
>  the recommended is 320x240, 640x480, 800x640.
>  
>  A timeout could be passed to bios, guest will pause for @var{rb_timeout} ms
> -when boot failed, then reboot. If @option{reboot-timeout} is not set,
> -guest will not reboot by default. Currently Seabios for X86
> +when boot failed, then reboot. If @var{rb_timeout} is '-1', guest will not
> +reboot, qemu passes '-1' to bios by default. Currently Seabios for X86
>  system support it.
>  
>  Do strict boot via @option{strict=on} as far as firmware/BIOS

Reviewed-by: Markus Armbruster 

Re: [PATCH v2 08/13] s390x: protvirt: Add new VCPU reset functions

[Janosch Frank]

On 12/4/19 12:58 PM, Thomas Huth wrote:
> On 29/11/2019 10.48, Janosch Frank wrote:
>> CPU resets for protected guests need to be done via Ultravisor
>> calls. Hence we need a way to issue these calls for each reset.
>>
>> As we formerly had only one reset function and it was called for
>> initial, as well as for the clear reset, we now need a new interface.
>>
>> Signed-off-by: Janosch Frank 
>> ---
> [...]
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index b802d02ff5..5b1ed3acb4 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -154,6 +154,7 @@ static int cap_ri;
>>  static int cap_gs;
>>  static int cap_hpage_1m;
>>  static int cap_protvirt;
>> +static int cap_vcpu_resets;
>>  
>>  static int active_cmma;
>>  
>> @@ -346,6 +347,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>>  cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
>>  cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
>>  cap_protvirt = kvm_check_extension(s, KVM_CAP_S390_PROTECTED);
>> +cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS);
>>  
>>  if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
>>  || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
>> @@ -407,20 +409,44 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
>>  return 0;
>>  }
>>  
>> -void kvm_s390_reset_vcpu(S390CPU *cpu)
>> +static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
>>  {
>>  CPUState *cs = CPU(cpu);
>>  
>> -/* The initial reset call is needed here to reset in-kernel
>> - * vcpu data that we can't access directly from QEMU
>> - * (i.e. with older kernels which don't support sync_regs/ONE_REG).
>> - * Before this ioctl cpu_synchronize_state() is called in common kvm
>> - * code (kvm-all) */
>> +/*
>> + * The reset call is needed here to reset in-kernel vcpu data that
>> + * we can't access directly from QEMU (i.e. with older kernels
>> + * which don't support sync_regs/ONE_REG).  Before this ioctl
>> + * cpu_synchronize_state() is called in common kvm code
>> + * (kvm-all).
>> + */
>> +if (cap_vcpu_resets) {
>> +if (kvm_vcpu_ioctl(cs, KVM_S390_VCPU_RESET, type)) {
>> +error_report("CPU reset type %ld failed on CPU %i",
>> + type, cs->cpu_index);
>> +}
>> +return;
>> +}
>>  if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
>>  error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
>>  }
> 
> Don't you want to check for type != KVM_S390_VCPU_RESET_NORMAL before
> doing the INITIAL_RESET here?
> 
>>  }
>>  
>> +void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
>> +{
>> +kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_INITIAL);
>> +}
>> +
>> +void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
>> +{
>> +kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_CLEAR);
>> +}
>> +
>> +void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
>> +{
>> +kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_NORMAL);
>> +}
> 
> ... otherwise this might reset more things than expected?
> 
> Or do I miss something here?

Hey Thomas I split out this patch into the "architectural compliance"
patch series. Have a look into there, this is an old version.

> 
>  Thomas
> 




signature.asc
Description: OpenPGP digital signature

Re: [PATCH 7/6] Makefile: Make Makefile depend on generated qga files, too

[Eric Blake]

On 12/4/19 12:56 AM, Markus Armbruster wrote:


+++ b/Makefile
@@ -130,6 +130,15 @@ GENERATED_QAPI_FILES += qapi/qapi-doc.texi
 generated-files-y += $(GENERATED_QAPI_FILES)
   +GENERATED_QGA_FILES := qga-qapi-types.c qga-qapi-types.h
+GENERATED_QGA_FILES += qga-qapi-visit.c qga-qapi-visit.h
+GENERATED_QGA_FILES += qga-qapi-commands.h qga-qapi-commands.c
+GENERATED_QGA_FILES += qga-qapi-init-commands.h qga-qapi-init-commands.c
+GENERATED_QGA_FILES += qga-qapi-doc.texi
+GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/, $(GENERATED_QGA_FILES))


Would it be worth using two separate variable names (maybe
GENERATED_QGA_BASEFILES for the first list) rather than exploiting the
arcane knowledge that consecutive use of := causes GNU make to rewrite
an existing variable with new contents?


Our rules.mak relies on this already.  It's full of magic, which
admittedly diminishes its suitability to serve as a good example.

Your worry might be rooted in old "=" burns.  "=" makes the variable
recursively expanded, and

 GENERATED_QGA_FILES = $(addprefix qga/qapi-generated/, 
$(GENERATED_QGA_FILES))


Indeed, but I have to refer to the manual to remind myself of whether = 
or := is what I want in a given situation.




would be an infinite loop.  ":=" makes it simply expanded; there's not
even a loop, let alone an infinite one.  The GNU Make manual explains
this clearly at
https://www.gnu.org/software/make/manual/html_node/Flavors.html

Aside: there's a reason one of the two flavors is called "simple".  It
could additionally be called "not as slow".  One of my pet makefile
peeves: unthinking use of recursively expanded variables, complicating
semantics and slowing down builds.

Back to this patch.  I had started to write the thing in longhand, but
got tired of repeating qga/qapi-generated/, so I factored that out.
Would longhand be easier to understand?


It's more verbose.  My suggestion was more:

GENERATED_QGA_BASENAMES := qga-qapi-types.c qga-qapi-types.h
GENERATED_QGA_BASENAMES += qga-qapi-visit.c qga-qapi-visit.h
...
GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/, 
$(GENERATED_QGA_BASENAMES))


to avoid the reassignment-to-self issue altogether, while still 
remaining concise compared to longhand.


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

Re: [PATCH 7/6] Makefile: Make Makefile depend on generated qga files, too

[Markus Armbruster]

Eric Blake  writes:

> On 12/4/19 12:56 AM, Markus Armbruster wrote:
>
 +++ b/Makefile
 @@ -130,6 +130,15 @@ GENERATED_QAPI_FILES += qapi/qapi-doc.texi
  generated-files-y += $(GENERATED_QAPI_FILES)
+GENERATED_QGA_FILES := qga-qapi-types.c qga-qapi-types.h
 +GENERATED_QGA_FILES += qga-qapi-visit.c qga-qapi-visit.h
 +GENERATED_QGA_FILES += qga-qapi-commands.h qga-qapi-commands.c
 +GENERATED_QGA_FILES += qga-qapi-init-commands.h qga-qapi-init-commands.c
 +GENERATED_QGA_FILES += qga-qapi-doc.texi
 +GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/, 
 $(GENERATED_QGA_FILES))
>>>
>>> Would it be worth using two separate variable names (maybe
>>> GENERATED_QGA_BASEFILES for the first list) rather than exploiting the
>>> arcane knowledge that consecutive use of := causes GNU make to rewrite
>>> an existing variable with new contents?
>>
>> Our rules.mak relies on this already.  It's full of magic, which
>> admittedly diminishes its suitability to serve as a good example.
>>
>> Your worry might be rooted in old "=" burns.  "=" makes the variable
>> recursively expanded, and
>>
>>  GENERATED_QGA_FILES = $(addprefix qga/qapi-generated/, 
>> $(GENERATED_QGA_FILES))
>
> Indeed, but I have to refer to the manual to remind myself of whether
> = or := is what I want in a given situation.

Trust me, you're either sure you want "=", or you want ":=".

On a green field, I recommend a hard rule "no = without a comment
explaining why".

>> would be an infinite loop.  ":=" makes it simply expanded; there's not
>> even a loop, let alone an infinite one.  The GNU Make manual explains
>> this clearly at
>> https://www.gnu.org/software/make/manual/html_node/Flavors.html
>>
>> Aside: there's a reason one of the two flavors is called "simple".  It
>> could additionally be called "not as slow".  One of my pet makefile
>> peeves: unthinking use of recursively expanded variables, complicating
>> semantics and slowing down builds.
>>
>> Back to this patch.  I had started to write the thing in longhand, but
>> got tired of repeating qga/qapi-generated/, so I factored that out.
>> Would longhand be easier to understand?
>
> It's more verbose.  My suggestion was more:
>
> GENERATED_QGA_BASENAMES := qga-qapi-types.c qga-qapi-types.h
> GENERATED_QGA_BASENAMES += qga-qapi-visit.c qga-qapi-visit.h
> ...
> GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/,
> $(GENERATED_QGA_BASENAMES))
>
> to avoid the reassignment-to-self issue altogether, while still
> remaining concise compared to longhand.

Either way, we use multiple assignments to build GENERATED_QGA_FILES.
The only difference is that the version using two variables would also
work with recursive expansion, due to the magic of +=.

[Bug 1852196] Re: update edk2 submodule & binaries to edk2-stable201911

[Philippe Mathieu-Daudé]

** Changed in: qemu
 Assignee: Laszlo Ersek (Red Hat) (lersek) => Philippe Mathieu-Daudé 
(philmd)

** Changed in: qemu
   Status: New => In Progress

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

Title:
  update edk2 submodule & binaries to edk2-stable201911

Status in QEMU:
  In Progress

Bug description:
  edk2-stable201911 will be tagged soon:

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-
  Release-Planning

https://github.com/tianocore/edk2/releases/tag/edk2-stable201911
[upcoming link]

  It should be picked up by QEMU, after the v4.2.0 release.

  Relevant fixes / features in edk2, since edk2-stable201905 (which is
  what QEMU bundles at the moment, from LP#1831477):

  - enable UEFI HTTPS Boot in ArmVirtQemu* platforms
https://bugzilla.tianocore.org/show_bug.cgi?id=1009
(this is from edk2-stable201908)

  - fix CVE-2019-14553 (Invalid server certificate accepted in HTTPS Boot)
https://bugzilla.tianocore.org/show_bug.cgi?id=960

  - consume OpenSSL-1.1.1d, for fixing CVE-2019-1543, CVE-2019-1552 and
CVE-2019-1563
https://bugzilla.tianocore.org/show_bug.cgi?id=2226

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

Re: [PATCH v2 04/18] tests: Clean up initialization of Error *err variables

[Philippe Mathieu-Daudé]

On 12/4/19 10:36 AM, Markus Armbruster wrote:

Declaring a local Error *err without initializer looks suspicious.
Fuse the declaration with the initialization to avoid that.

Signed-off-by: Markus Armbruster 
---
  tests/test-qobject-output-visitor.c | 8 
  tests/test-string-output-visitor.c  | 4 ++--
  2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/test-qobject-output-visitor.c 
b/tests/test-qobject-output-visitor.c
index 3e993e5ba8..d7761ebf84 100644
--- a/tests/test-qobject-output-visitor.c
+++ b/tests/test-qobject-output-visitor.c
@@ -145,10 +145,10 @@ static void 
test_visitor_out_enum_errors(TestOutputVisitorData *data,
   const void *unused)
  {
  EnumOne i, bad_values[] = { ENUM_ONE__MAX, -1 };
-Error *err;
  
  for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {

-err = NULL;
+Error *err = NULL;
+


Reviewed-by: Philippe Mathieu-Daudé 


  visit_type_EnumOne(data->ov, "unused", _values[i], );
  error_free_or_abort();
  visitor_reset(data);
@@ -240,11 +240,11 @@ static void 
test_visitor_out_struct_errors(TestOutputVisitorData *data,
  EnumOne bad_values[] = { ENUM_ONE__MAX, -1 };
  UserDefOne u = {0};
  UserDefOne *pu = 
-Error *err;
  int i;
  
  for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {

-err = NULL;
+Error *err = NULL;
+
  u.has_enum1 = true;
  u.enum1 = bad_values[i];
  visit_type_UserDefOne(data->ov, "unused", , );
diff --git a/tests/test-string-output-visitor.c 
b/tests/test-string-output-visitor.c
index 02766c0f65..1be1540767 100644
--- a/tests/test-string-output-visitor.c
+++ b/tests/test-string-output-visitor.c
@@ -207,10 +207,10 @@ static void 
test_visitor_out_enum_errors(TestOutputVisitorData *data,
   const void *unused)
  {
  EnumOne i, bad_values[] = { ENUM_ONE__MAX, -1 };
-Error *err;
  
  for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {

-err = NULL;
+Error *err = NULL;
+
  visit_type_EnumOne(data->ov, "unused", _values[i], );
  error_free_or_abort();
  }

Re: [PATCH 10/10] arm: allwinner-h3: add EMAC ethernet device

[Philippe Mathieu-Daudé]

On 12/3/19 10:33 AM, KONRAD Frederic wrote:

Le 12/2/19 à 10:09 PM, Niek Linnenbank a écrit :

The Allwinner H3 System on Chip includes an Ethernet MAC (EMAC)
which provides 10M/100M/1000M Ethernet connectivity. This commit
adds support for the Allwinner H3 EMAC, including emulation for
the following functionality:

  * DMA transfers
  * MII interface
  * Transmit CRC calculation

Signed-off-by: Niek Linnenbank 
---
  hw/arm/Kconfig |   1 +
  hw/arm/allwinner-h3.c  |  17 +
  hw/arm/orangepi.c  |   7 +
  hw/net/Kconfig |   3 +
  hw/net/Makefile.objs   |   1 +
  hw/net/allwinner-h3-emac.c | 786 +
  hw/net/trace-events    |  10 +
  include/hw/arm/allwinner-h3.h  |   2 +
  include/hw/net/allwinner-h3-emac.h |  69 +++
  9 files changed, 896 insertions(+)
  create mode 100644 hw/net/allwinner-h3-emac.c
  create mode 100644 include/hw/net/allwinner-h3-emac.h

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index ebf8d2325f..551cff3442 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -294,6 +294,7 @@ config ALLWINNER_A10
  config ALLWINNER_H3
  bool
  select ALLWINNER_A10_PIT
+    select ALLWINNER_H3_EMAC
  select SERIAL
  select ARM_TIMER
  select ARM_GIC
diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
index c2972caf88..274b8548c0 100644
--- a/hw/arm/allwinner-h3.c
+++ b/hw/arm/allwinner-h3.c
@@ -53,6 +53,9 @@ static void aw_h3_init(Object *obj)
  sysbus_init_child_obj(obj, "mmc0", >mmc0, sizeof(s->mmc0),
    TYPE_AW_H3_SDHOST);
+
+    sysbus_init_child_obj(obj, "emac", >emac, sizeof(s->emac),
+  TYPE_AW_H3_EMAC);
  }
  static void aw_h3_realize(DeviceState *dev, Error **errp)
@@ -237,6 +240,20 @@ static void aw_h3_realize(DeviceState *dev, Error 
**errp)

  return;
  }
+    /* EMAC */
+    if (nd_table[0].used) {
+    qemu_check_nic_model(_table[0], TYPE_AW_H3_EMAC);
+    qdev_set_nic_properties(DEVICE(>emac), _table[0]);
+    }
+    object_property_set_bool(OBJECT(>emac), true, "realized", );
+    if (err != NULL) {
+    error_propagate(errp, err);
+    return;
+    }
+    sysbusdev = SYS_BUS_DEVICE(>emac);
+    sysbus_mmio_map(sysbusdev, 0, AW_H3_EMAC_BASE);
+    sysbus_connect_irq(sysbusdev, 0, s->irq[AW_H3_GIC_SPI_EMAC]);
+
  /* Universal Serial Bus */
  sysbus_create_simple(TYPE_AW_H3_EHCI, AW_H3_EHCI0_BASE,
   s->irq[AW_H3_GIC_SPI_EHCI0]);
diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
index dee3efaf08..8a61eb0e69 100644
--- a/hw/arm/orangepi.c
+++ b/hw/arm/orangepi.c
@@ -61,6 +61,13 @@ static void orangepi_init(MachineState *machine)
  exit(1);
  }
+    /* Setup EMAC properties */
+    object_property_set_int(OBJECT(>h3->emac), 1, "phy-addr", );
+    if (err != NULL) {
+    error_reportf_err(err, "Couldn't set phy address: ");
+    exit(1);
+    }
+
  /* Mark H3 object realized */
  object_property_set_bool(OBJECT(s->h3), true, "realized", );
  if (err != NULL) {
diff --git a/hw/net/Kconfig b/hw/net/Kconfig
index 3856417d42..36d3923992 100644
--- a/hw/net/Kconfig
+++ b/hw/net/Kconfig
@@ -74,6 +74,9 @@ config MIPSNET
  config ALLWINNER_EMAC
  bool
+config ALLWINNER_H3_EMAC
+    bool
+
  config IMX_FEC
  bool
diff --git a/hw/net/Makefile.objs b/hw/net/Makefile.objs
index 7907d2c199..5548deb07a 100644
--- a/hw/net/Makefile.objs
+++ b/hw/net/Makefile.objs
@@ -23,6 +23,7 @@ common-obj-$(CONFIG_XGMAC) += xgmac.o
  common-obj-$(CONFIG_MIPSNET) += mipsnet.o
  common-obj-$(CONFIG_XILINX_AXI) += xilinx_axienet.o
  common-obj-$(CONFIG_ALLWINNER_EMAC) += allwinner_emac.o
+common-obj-$(CONFIG_ALLWINNER_H3_EMAC) += allwinner-h3-emac.o
  common-obj-$(CONFIG_IMX_FEC) += imx_fec.o
  common-obj-$(CONFIG_CADENCE) += cadence_gem.o
diff --git a/hw/net/allwinner-h3-emac.c b/hw/net/allwinner-h3-emac.c
new file mode 100644
index 00..37f6f44406
--- /dev/null
+++ b/hw/net/allwinner-h3-emac.c
@@ -0,0 +1,786 @@
+/*
+ * Allwinner H3 EMAC emulation
+ *
+ * Copyright (C) 2019 Niek Linnenbank 
+ *
+ * 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 .
+ */
+
+#include "qemu/osdep.h"
+#include "hw/sysbus.h"
+#include "migration/vmstate.h"
+#include "net/net.h"
+#include "hw/irq.h"
+#include "hw/qdev-properties.h"
+#include 

Re: [PATCH 10/10] arm: allwinner-h3: add EMAC ethernet device

[KONRAD Frederic]

Le 12/4/19 à 4:14 PM, Philippe Mathieu-Daudé a écrit :

On 12/3/19 10:33 AM, KONRAD Frederic wrote:

Le 12/2/19 à 10:09 PM, Niek Linnenbank a écrit :

The Allwinner H3 System on Chip includes an Ethernet MAC (EMAC)
which provides 10M/100M/1000M Ethernet connectivity. This commit
adds support for the Allwinner H3 EMAC, including emulation for
the following functionality:

  * DMA transfers
  * MII interface
  * Transmit CRC calculation

Signed-off-by: Niek Linnenbank 
---
  hw/arm/Kconfig |   1 +
  hw/arm/allwinner-h3.c  |  17 +
  hw/arm/orangepi.c  |   7 +
  hw/net/Kconfig |   3 +
  hw/net/Makefile.objs   |   1 +
  hw/net/allwinner-h3-emac.c | 786 +
  hw/net/trace-events    |  10 +
  include/hw/arm/allwinner-h3.h  |   2 +
  include/hw/net/allwinner-h3-emac.h |  69 +++
  9 files changed, 896 insertions(+)
  create mode 100644 hw/net/allwinner-h3-emac.c
  create mode 100644 include/hw/net/allwinner-h3-emac.h

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index ebf8d2325f..551cff3442 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -294,6 +294,7 @@ config ALLWINNER_A10
  config ALLWINNER_H3
  bool
  select ALLWINNER_A10_PIT
+    select ALLWINNER_H3_EMAC
  select SERIAL
  select ARM_TIMER
  select ARM_GIC
diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
index c2972caf88..274b8548c0 100644
--- a/hw/arm/allwinner-h3.c
+++ b/hw/arm/allwinner-h3.c
@@ -53,6 +53,9 @@ static void aw_h3_init(Object *obj)
  sysbus_init_child_obj(obj, "mmc0", >mmc0, sizeof(s->mmc0),
    TYPE_AW_H3_SDHOST);
+
+    sysbus_init_child_obj(obj, "emac", >emac, sizeof(s->emac),
+  TYPE_AW_H3_EMAC);
  }
  static void aw_h3_realize(DeviceState *dev, Error **errp)
@@ -237,6 +240,20 @@ static void aw_h3_realize(DeviceState *dev, Error **errp)
  return;
  }
+    /* EMAC */
+    if (nd_table[0].used) {
+    qemu_check_nic_model(_table[0], TYPE_AW_H3_EMAC);
+    qdev_set_nic_properties(DEVICE(>emac), _table[0]);
+    }
+    object_property_set_bool(OBJECT(>emac), true, "realized", );
+    if (err != NULL) {
+    error_propagate(errp, err);
+    return;
+    }
+    sysbusdev = SYS_BUS_DEVICE(>emac);
+    sysbus_mmio_map(sysbusdev, 0, AW_H3_EMAC_BASE);
+    sysbus_connect_irq(sysbusdev, 0, s->irq[AW_H3_GIC_SPI_EMAC]);
+
  /* Universal Serial Bus */
  sysbus_create_simple(TYPE_AW_H3_EHCI, AW_H3_EHCI0_BASE,
   s->irq[AW_H3_GIC_SPI_EHCI0]);
diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
index dee3efaf08..8a61eb0e69 100644
--- a/hw/arm/orangepi.c
+++ b/hw/arm/orangepi.c
@@ -61,6 +61,13 @@ static void orangepi_init(MachineState *machine)
  exit(1);
  }
+    /* Setup EMAC properties */
+    object_property_set_int(OBJECT(>h3->emac), 1, "phy-addr", );
+    if (err != NULL) {
+    error_reportf_err(err, "Couldn't set phy address: ");
+    exit(1);
+    }
+
  /* Mark H3 object realized */
  object_property_set_bool(OBJECT(s->h3), true, "realized", );
  if (err != NULL) {
diff --git a/hw/net/Kconfig b/hw/net/Kconfig
index 3856417d42..36d3923992 100644
--- a/hw/net/Kconfig
+++ b/hw/net/Kconfig
@@ -74,6 +74,9 @@ config MIPSNET
  config ALLWINNER_EMAC
  bool
+config ALLWINNER_H3_EMAC
+    bool
+
  config IMX_FEC
  bool
diff --git a/hw/net/Makefile.objs b/hw/net/Makefile.objs
index 7907d2c199..5548deb07a 100644
--- a/hw/net/Makefile.objs
+++ b/hw/net/Makefile.objs
@@ -23,6 +23,7 @@ common-obj-$(CONFIG_XGMAC) += xgmac.o
  common-obj-$(CONFIG_MIPSNET) += mipsnet.o
  common-obj-$(CONFIG_XILINX_AXI) += xilinx_axienet.o
  common-obj-$(CONFIG_ALLWINNER_EMAC) += allwinner_emac.o
+common-obj-$(CONFIG_ALLWINNER_H3_EMAC) += allwinner-h3-emac.o
  common-obj-$(CONFIG_IMX_FEC) += imx_fec.o
  common-obj-$(CONFIG_CADENCE) += cadence_gem.o
diff --git a/hw/net/allwinner-h3-emac.c b/hw/net/allwinner-h3-emac.c
new file mode 100644
index 00..37f6f44406
--- /dev/null
+++ b/hw/net/allwinner-h3-emac.c
@@ -0,0 +1,786 @@
+/*
+ * Allwinner H3 EMAC emulation
+ *
+ * Copyright (C) 2019 Niek Linnenbank 
+ *
+ * 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 .
+ */
+
+#include "qemu/osdep.h"
+#include "hw/sysbus.h"
+#include "migration/vmstate.h"
+#include "net/net.h"

[PATCH v2 2/7] iotests: Skip test 060 if it is not possible to create large files

[Thomas Huth]

Test 060 fails in the arm64, s390x and ppc64le LXD containers on Travis
(which we will hopefully enable in our CI soon). These containers
apparently do not allow large files to be created. The repair process
in test 060 creates a file of 64 GiB, so test first whether such large
files are possible and skip the test if that's not the case.

Signed-off-by: Thomas Huth 
---
 tests/qemu-iotests/060 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index b91d8321bb..d96f17a484 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -49,6 +49,9 @@ _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
 
+# The repair process will create a large file - so check for availability first
+_require_large_file 64G
+
 rt_offset=65536  # 0x1 (XXX: just an assumption)
 rb_offset=131072 # 0x2 (XXX: just an assumption)
 l1_offset=196608 # 0x3 (XXX: just an assumption)
-- 
2.18.1

Re: [PATCH v2 04/18] tests: Clean up initialization of Error *err variables

[Eric Blake]

On 12/4/19 3:36 AM, Markus Armbruster wrote:

Declaring a local Error *err without initializer looks suspicious.
Fuse the declaration with the initialization to avoid that.

Signed-off-by: Markus Armbruster 
---
  tests/test-qobject-output-visitor.c | 8 
  tests/test-string-output-visitor.c  | 4 ++--
  2 files changed, 6 insertions(+), 6 deletions(-)



Reviewed-by: Eric Blake 

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

Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[Eric Blake]

On 12/4/19 1:31 AM, pannengy...@huawei.com wrote:

From: Pan Nengyuan 

Devices tend to maintain vq pointers, allow deleting them trough a vq pointer.


through



Signed-off-by: Michael S. Tsirkin 
Signed-off-by: Pan Nengyuan 
---


Also, don't forget to send a 0/3 cover letter (any series longer than 
one patch should have a cover letter; it is possible to configure git to 
do this automatically: https://wiki.qemu.org/Contribute/SubmitAPatch has 
this tip and others)


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

Re: [PATCH 02/11] target/arm: Add arm_mmu_idx_is_stage1

[Philippe Mathieu-Daudé]

On 12/3/19 11:53 PM, Richard Henderson wrote:

Use a common predicate for querying stage1-ness.

Signed-off-by: Richard Henderson 


Reviewed-by: Philippe Mathieu-Daudé 


---
  target/arm/internals.h | 11 +++
  target/arm/helper.c|  8 +++-
  2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 49dac2a677..850f204f14 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1034,6 +1034,17 @@ static inline ARMMMUIdx arm_stage1_mmu_idx(CPUARMState 
*env)
  ARMMMUIdx arm_stage1_mmu_idx(CPUARMState *env);
  #endif
  
+static inline bool arm_mmu_idx_is_stage1(ARMMMUIdx mmu_idx)

+{
+switch (mmu_idx) {
+case ARMMMUIdx_Stage1_E0:
+case ARMMMUIdx_Stage1_E1:
+return true;
+default:
+return false;
+}
+}
+
  /*
   * Parameters of a given virtual address, as extracted from the
   * translation control register (TCR) for a given regime.
diff --git a/target/arm/helper.c b/target/arm/helper.c
index f3785d5ad6..fdb86ea427 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3212,8 +3212,7 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t 
value,
  bool take_exc = false;
  
  if (fi.s1ptw && current_el == 1 && !arm_is_secure(env)

-&& (mmu_idx == ARMMMUIdx_Stage1_E1
-|| mmu_idx == ARMMMUIdx_Stage1_E0)) {
+&& arm_mmu_idx_is_stage1(mmu_idx)) {
  /*
   * Synchronous stage 2 fault on an access made as part of the
   * translation table walk for AT S1E0* or AT S1E1* insn
@@ -9159,8 +9158,7 @@ static inline bool 
regime_translation_disabled(CPUARMState *env,
  }
  }
  
-if ((env->cp15.hcr_el2 & HCR_DC) &&

-(mmu_idx == ARMMMUIdx_Stage1_E0 || mmu_idx == ARMMMUIdx_Stage1_E1)) {
+if ((env->cp15.hcr_el2 & HCR_DC) && arm_mmu_idx_is_stage1(mmu_idx)) {
  /* HCR.DC means SCTLR_EL1.M behaves as 0 */
  return true;
  }
@@ -9469,7 +9467,7 @@ static hwaddr S1_ptw_translate(CPUARMState *env, 
ARMMMUIdx mmu_idx,
 hwaddr addr, MemTxAttrs txattrs,
 ARMMMUFaultInfo *fi)
  {
-if ((mmu_idx == ARMMMUIdx_Stage1_E0 || mmu_idx == ARMMMUIdx_Stage1_E1) &&
+if (arm_mmu_idx_is_stage1(mmu_idx) &&
  !regime_translation_disabled(env, ARMMMUIdx_Stage2)) {
  target_ulong s2size;
  hwaddr s2pa;

[PATCH v2 3/7] iotests: Skip test 079 if it is not possible to create large files

[Thomas Huth]

Test 079 fails in the arm64, s390x and ppc64le LXD containers on Travis
(which we will hopefully enable in our CI soon). These containers
apparently do not allow large files to be created. Test 079 tries to
create a 4G sparse file, which is apparently already too big for these
containers, so check first whether we can really create such files before
executing the test.

Signed-off-by: Thomas Huth 
---
 tests/qemu-iotests/079 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/079 b/tests/qemu-iotests/079
index 81f0c21f53..78536d3bbf 100755
--- a/tests/qemu-iotests/079
+++ b/tests/qemu-iotests/079
@@ -39,6 +39,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file nfs
 
+# Some containers (e.g. non-x86 on Travis) do not allow large files
+_require_large_file 4G
+
 echo "=== Check option preallocation and cluster_size ==="
 echo
 cluster_sizes="16384 32768 65536 131072 262144 524288 1048576 2097152 4194304"
-- 
2.18.1

[PATCH v2 7/7] travis.yml: Enable builds on arm64, ppc64le and s390x

[Thomas Huth]

Travis recently added the possibility to test on these architectures,
too, so let's enable them in our travis.yml file to extend our test
coverage.

Unfortunately, the libssh in this Ubuntu version (bionic) is in a pretty
unusable Frankenstein state and libspice-server-dev is not available here,
so we can not use the global list of packages to install, but have to
provide individual package lists instead.

Also, some of the iotests crash when using "dist: bionic" on arm64
and ppc64le, thus these two builders have to use "dist: xenial" until
the problem is understood / fixed.

Signed-off-by: Thomas Huth 
---
 .travis.yml | 86 +
 1 file changed, 86 insertions(+)

diff --git a/.travis.yml b/.travis.yml
index 445b0646c1..0e6458b0af 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -354,6 +354,92 @@ matrix:
 - TEST_CMD="make -j3 check-tcg V=1"
 - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-debug-tcg"
 
+- arch: arm64
+  dist: xenial
+  addons:
+apt_packages:
+  - libaio-dev
+  - libattr1-dev
+  - libbrlapi-dev
+  - libcap-ng-dev
+  - libgcrypt20-dev
+  - libgnutls28-dev
+  - libgtk-3-dev
+  - libiscsi-dev
+  - liblttng-ust-dev
+  - libncurses5-dev
+  - libnfs-dev
+  - libnss3-dev
+  - libpixman-1-dev
+  - libpng-dev
+  - librados-dev
+  - libsdl2-dev
+  - libseccomp-dev
+  - liburcu-dev
+  - libusb-1.0-0-dev
+  - libvdeplug-dev
+  - libvte-2.91-dev
+  env:
+- TEST_CMD="make check check-tcg V=1"
+- CONFIG="--disable-containers --target-list=${MAIN_SOFTMMU_TARGETS}"
+
+- arch: ppc64le
+  dist: xenial
+  addons:
+apt_packages:
+  - libaio-dev
+  - libattr1-dev
+  - libbrlapi-dev
+  - libcap-ng-dev
+  - libgcrypt20-dev
+  - libgnutls28-dev
+  - libgtk-3-dev
+  - libiscsi-dev
+  - liblttng-ust-dev
+  - libncurses5-dev
+  - libnfs-dev
+  - libnss3-dev
+  - libpixman-1-dev
+  - libpng-dev
+  - librados-dev
+  - libsdl2-dev
+  - libseccomp-dev
+  - liburcu-dev
+  - libusb-1.0-0-dev
+  - libvdeplug-dev
+  - libvte-2.91-dev
+  env:
+- TEST_CMD="make check check-tcg V=1"
+- CONFIG="--disable-containers 
--target-list=${MAIN_SOFTMMU_TARGETS},ppc64le-linux-user"
+
+- arch: s390x
+  dist: bionic
+  addons:
+apt_packages:
+  - libaio-dev
+  - libattr1-dev
+  - libbrlapi-dev
+  - libcap-ng-dev
+  - libgcrypt20-dev
+  - libgnutls28-dev
+  - libgtk-3-dev
+  - libiscsi-dev
+  - liblttng-ust-dev
+  - libncurses5-dev
+  - libnfs-dev
+  - libnss3-dev
+  - libpixman-1-dev
+  - libpng-dev
+  - librados-dev
+  - libsdl2-dev
+  - libseccomp-dev
+  - liburcu-dev
+  - libusb-1.0-0-dev
+  - libvdeplug-dev
+  - libvte-2.91-dev
+  env:
+- TEST_CMD="make check check-tcg V=1"
+- CONFIG="--disable-containers 
--target-list=${MAIN_SOFTMMU_TARGETS},s390x-linux-user"
 
 # Release builds
 # The make-release script expect a QEMU version, so our tag must start 
with a 'v'.
-- 
2.18.1

Re: [PATCH v2 12/13] s390x: protvirt: Disable address checks for PV guest IO emulation

[Thomas Huth]

On 29/11/2019 10.48, Janosch Frank wrote:
> IO instruction data is routed through SIDAD for protected guests, so
> adresses do not need to be checked, as this is kernel memory.
> 
> Signed-off-by: Janosch Frank 
> ---
>  target/s390x/ioinst.c | 26 +++---
>  1 file changed, 19 insertions(+), 7 deletions(-)

Reviewed-by: Thomas Huth 

Re: [PATCH v6] error: rename errp to errp_in where it is IN-argument

[Markus Armbruster]

Markus Armbruster  writes:

> Vladimir Sementsov-Ogievskiy  writes:
>
>> 29.11.2019 17:35, Markus Armbruster wrote:
[...]
>>> I pushed my fixups to git://repo.or.cz/qemu/armbru.git branch error-prep
>>> for your convenience.  The commit messages of the fixed up commits need
>>> rephrasing, of course.
>>> 
>>
>> Looked through fixups, looks OK for me, thanks! What next?
>
> Let me finish my fixing incorrect dereferences of errp, and then we
> figure out what to include in v7.

Your v6 with my fixups does not conflict with my "[PATCH v2 00/18] Error
handling fixes", except for "hw/core/loader-fit: fix freeing errp in
fit_load_fdt", which my "[PATCH v2 07/18] hw/core: Fix fit_load_fdt()
error handling violations" supersedes.

Suggest you work in the fixups and post as v7.  I'll merge that in my
tree, to give you a base for the remainder of your "auto propagated
local_err" work.  While you work on that, I'll work on getting the base
merged into master.  Sounds like a plan?

Re: [PATCH v4 18/40] target/arm: Reorganize ARMMMUIdx

[Alex Bennée]

Richard Henderson  writes:

> Prepare for, but do not yet implement, the EL2&0 regime.
> This involves adding the new MMUIdx enumerators and adjusting
> some of the MMUIdx related predicates to match.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  target/arm/cpu-param.h |   2 +-
>  target/arm/cpu.h   | 128 ++---
>  target/arm/internals.h |  37 +++-
>  target/arm/helper.c|  66 ++---
>  target/arm/translate.c |   1 -
>  5 files changed, 150 insertions(+), 84 deletions(-)
>
> diff --git a/target/arm/cpu-param.h b/target/arm/cpu-param.h
> index 6e6948e960..18ac562346 100644
> --- a/target/arm/cpu-param.h
> +++ b/target/arm/cpu-param.h
> @@ -29,6 +29,6 @@
>  # define TARGET_PAGE_BITS_MIN  10
>  #endif
>  
> -#define NB_MMU_MODES 8
> +#define NB_MMU_MODES 9
>  
>  #endif
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 015301e93a..bf8eb57e3a 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2778,7 +2778,9 @@ static inline bool arm_excp_unmasked(CPUState *cs, 
> unsigned int excp_idx,
>   *  + NonSecure EL1 & 0 stage 1
>   *  + NonSecure EL1 & 0 stage 2
>   *  + NonSecure EL2
> - *  + Secure EL1 & EL0
> + *  + NonSecure EL2 & 0   (ARMv8.1-VHE)
> + *  + Secure EL0
> + *  + Secure EL1
>   *  + Secure EL3
>   * If EL3 is 32-bit:
>   *  + NonSecure PL1 & 0 stage 1
> @@ -2788,8 +2790,9 @@ static inline bool arm_excp_unmasked(CPUState *cs, 
> unsigned int excp_idx,
>   * (reminder: for 32 bit EL3, Secure PL1 is *EL3*, not EL1.)
>   *
>   * For QEMU, an mmu_idx is not quite the same as a translation regime 
> because:
> - *  1. we need to split the "EL1 & 0" regimes into two mmu_idxes, because 
> they
> - * may differ in access permissions even if the VA->PA map is the same
> + *  1. we need to split the "EL1 & 0" and "EL2 & 0" regimes into two 
> mmu_idxes,
> + * because they may differ in access permissions even if the VA->PA map 
> is
> + * the same
>   *  2. we want to cache in our TLB the full VA->IPA->PA lookup for a stage 
> 1+2
>   * translation, which means that we have one mmu_idx that deals with two
>   * concatenated translation regimes [this sort of combined s1+2 TLB is
> @@ -2801,19 +2804,23 @@ static inline bool arm_excp_unmasked(CPUState *cs, 
> unsigned int excp_idx,
>   *  4. we can also safely fold together the "32 bit EL3" and "64 bit EL3"
>   * translation regimes, because they map reasonably well to each other
>   * and they can't both be active at the same time.
> - * This gives us the following list of mmu_idx values:
> + *  5. we want to be able to use the TLB for accesses done as part of a
> + * stage1 page table walk, rather than having to walk the stage2 page
> + * table over and over.
>   *
> - * NS EL0 (aka NS PL0) stage 1+2
> - * NS EL1 (aka NS PL1) stage 1+2
> + * This gives us the following list of cases:
> + *
> + * NS EL0 (aka NS PL0) EL1&0 stage 1+2
> + * NS EL1 (aka NS PL1) EL1&0 stage 1+2
> + * NS EL0 EL2&0
> + * NS EL2 EL2&0
>   * NS EL2 (aka NS PL2)
> - * S EL3 (aka S PL1)
>   * S EL0 (aka S PL0)
>   * S EL1 (not used if EL3 is 32 bit)
> - * NS EL0+1 stage 2
> + * S EL3 (aka S PL1)
> + * NS EL0&1 stage 2
>   *
> - * (The last of these is an mmu_idx because we want to be able to use the TLB
> - * for the accesses done as part of a stage 1 page table walk, rather than
> - * having to walk the stage 2 page table over and over.)
> + * for a total of 9 different mmu_idx.
>   *
>   * R profile CPUs have an MPU, but can use the same set of MMU indexes
>   * as A profile. They only need to distinguish NS EL0 and NS EL1 (and
> @@ -2851,26 +2858,47 @@ static inline bool arm_excp_unmasked(CPUState *cs, 
> unsigned int excp_idx,
>   * For M profile we arrange them to have a bit for priv, a bit for negpri
>   * and a bit for secure.
>   */
> -#define ARM_MMU_IDX_A 0x10 /* A profile */
> -#define ARM_MMU_IDX_NOTLB 0x20 /* does not have a TLB */
> -#define ARM_MMU_IDX_M 0x40 /* M profile */
> +#define ARM_MMU_IDX_A 0x10  /* A profile */
> +#define ARM_MMU_IDX_NOTLB 0x20  /* does not have a TLB */
> +#define ARM_MMU_IDX_M 0x40  /* M profile */
>  
> -/* meanings of the bits for M profile mmu idx values */
> -#define ARM_MMU_IDX_M_PRIV 0x1
> +/* Meanings of the bits for M profile mmu idx values */
> +#define ARM_MMU_IDX_M_PRIV   0x1
>  #define ARM_MMU_IDX_M_NEGPRI 0x2
> -#define ARM_MMU_IDX_M_S 0x4
> +#define ARM_MMU_IDX_M_S  0x4  /* Secure */
>  
> -#define ARM_MMU_IDX_TYPE_MASK (~0x7)
> -#define ARM_MMU_IDX_COREIDX_MASK 0x7
> +#define ARM_MMU_IDX_TYPE_MASK \
> +(ARM_MMU_IDX_A | ARM_MMU_IDX_M | ARM_MMU_IDX_NOTLB)
> +#define ARM_MMU_IDX_COREIDX_MASK 0xf
>  
>  typedef enum ARMMMUIdx {
> +/*
> + * A-profile.
> + */
>  ARMMMUIdx_EL10_0 = 0 | ARM_MMU_IDX_A,
> -ARMMMUIdx_EL10_1 = 1 | ARM_MMU_IDX_A,
> -ARMMMUIdx_E2 = 2 | ARM_MMU_IDX_A,
> -ARMMMUIdx_SE3 = 3 | ARM_MMU_IDX_A,
> -ARMMMUIdx_SE0 = 4 | 

[PATCH 3/5] hw/arm/smmuv3: Align stream table base address to table size

[Simon Veith]

Per the specification, and as observed in hardware, the SMMUv3 aligns
the SMMU_STRTAB_BASE address to the size of the table by masking out the
respective least significant bits in the ADDR field.

Apply this masking logic to our smmu_find_ste() lookup function per the
specification.

ref. ARM IHI 0070C, section 6.3.23.

Signed-off-by: Simon Veith 
Cc: Eric Auger 
Cc: qemu-devel@nongnu.org
Cc: qemu-...@nongnu.org
---
 hw/arm/smmuv3.c | 29 +
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index aad4639..2d6c275 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -376,8 +376,9 @@ bad_ste:
 static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
  SMMUEventInfo *event)
 {
-dma_addr_t addr;
+dma_addr_t addr, strtab_base;
 uint32_t log2size;
+int strtab_size_shift;
 int ret;
 
 trace_smmuv3_find_ste(sid, s->features, s->sid_split);
@@ -391,10 +392,23 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, 
STE *ste,
 }
 if (s->features & SMMU_FEATURE_2LVL_STE) {
 int l1_ste_offset, l2_ste_offset, max_l2_ste, span;
-dma_addr_t strtab_base, l1ptr, l2ptr;
+dma_addr_t l1ptr, l2ptr;
 STEDesc l1std;
 
-strtab_base = s->strtab_base & SMMU_BASE_ADDR_MASK;
+/*
+ * Align strtab base address to table size. For this purpose, assume it
+ * is not bounded by SMMU_IDR1_SIDSIZE.
+ */
+strtab_size_shift = log2size - s->sid_split - 1 + 3;
+if (strtab_size_shift < DMA_ADDR_BITS) {
+if (strtab_size_shift < 5) {
+strtab_size_shift = 5;
+}
+strtab_base = s->strtab_base & SMMU_BASE_ADDR_MASK &
+  ~((1ULL << strtab_size_shift) - 1);
+} else {
+strtab_base = 0;
+}
 l1_ste_offset = sid >> s->sid_split;
 l2_ste_offset = sid & ((1 << s->sid_split) - 1);
 l1ptr = (dma_addr_t)(strtab_base + l1_ste_offset * sizeof(l1std));
@@ -433,7 +447,14 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE 
*ste,
 }
 addr = l2ptr + l2_ste_offset * sizeof(*ste);
 } else {
-addr = (s->strtab_base & SMMU_BASE_ADDR_MASK) + sid * sizeof(*ste);
+strtab_size_shift = log2size + 5;
+if (strtab_size_shift < DMA_ADDR_BITS) {
+strtab_base = s->strtab_base & SMMU_BASE_ADDR_MASK &
+  ~((1ULL << strtab_size_shift) - 1);
+} else {
+strtab_base = 0;
+}
+addr = strtab_base + sid * sizeof(*ste);
 }
 
 if (smmu_get_ste(s, addr, ste, event)) {
-- 
2.7.4