Re: [PATCH 0/5] qmp-shell modifications for non-interactive use

[Damien Hedde]

On 2/22/22 07:10, Markus Armbruster wrote:

Damien Hedde  writes:


Hi,

The main idea of this series is to be a bit more user-friendly when
using qmp-shell in a non-interactive way: with an input redirection
from a file containing a list of commands.

I'm working on dynamic qapi config of a qemu machine, this would
be very useful to provide and reproduce small examples.


Why not use plain QMP for that?

[...]


What do you mean by plain QMP ?

--
Damien

Re: [PATCH 17/31] vdpa: adapt vhost_ops callbacks to svq

[Eugenio Perez Martin]

On Tue, Feb 22, 2022 at 4:16 AM Jason Wang  wrote:
>
> On Tue, Feb 22, 2022 at 1:23 AM Eugenio Perez Martin
>  wrote:
> >
> > On Mon, Feb 21, 2022 at 8:15 AM Jason Wang  wrote:
> > >
> > >
> > > 在 2022/2/18 上午1:13, Eugenio Perez Martin 写道:
> > > > On Tue, Feb 8, 2022 at 4:58 AM Jason Wang  wrote:
> > > >>
> > > >> 在 2022/2/1 上午2:58, Eugenio Perez Martin 写道:
> > > >>> On Sun, Jan 30, 2022 at 5:03 AM Jason Wang  
> > > >>> wrote:
> > >  在 2022/1/22 上午4:27, Eugenio Pérez 写道:
> > > > First half of the buffers forwarding part, preparing vhost-vdpa
> > > > callbacks to SVQ to offer it. QEMU cannot enable it at this moment, 
> > > > so
> > > > this is effectively dead code at the moment, but it helps to reduce
> > > > patch size.
> > > >
> > > > Signed-off-by: Eugenio Pérez 
> > > > ---
> > > > hw/virtio/vhost-shadow-virtqueue.h |   2 +-
> > > > hw/virtio/vhost-shadow-virtqueue.c |  21 -
> > > > hw/virtio/vhost-vdpa.c | 133 
> > > > ++---
> > > > 3 files changed, 143 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.h 
> > > > b/hw/virtio/vhost-shadow-virtqueue.h
> > > > index 035207a469..39aef5ffdf 100644
> > > > --- a/hw/virtio/vhost-shadow-virtqueue.h
> > > > +++ b/hw/virtio/vhost-shadow-virtqueue.h
> > > > @@ -35,7 +35,7 @@ size_t vhost_svq_device_area_size(const 
> > > > VhostShadowVirtqueue *svq);
> > > >
> > > > void vhost_svq_stop(VhostShadowVirtqueue *svq);
> > > >
> > > > -VhostShadowVirtqueue *vhost_svq_new(void);
> > > > +VhostShadowVirtqueue *vhost_svq_new(uint16_t qsize);
> > > >
> > > > void vhost_svq_free(VhostShadowVirtqueue *vq);
> > > >
> > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c 
> > > > b/hw/virtio/vhost-shadow-virtqueue.c
> > > > index f129ec8395..7c168075d7 100644
> > > > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > > > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > > > @@ -277,9 +277,17 @@ void vhost_svq_stop(VhostShadowVirtqueue *svq)
> > > > /**
> > > >  * Creates vhost shadow virtqueue, and instruct vhost device to 
> > > > use the shadow
> > > >  * methods and file descriptors.
> > > > + *
> > > > + * @qsize Shadow VirtQueue size
> > > > + *
> > > > + * Returns the new virtqueue or NULL.
> > > > + *
> > > > + * In case of error, reason is reported through error_report.
> > > >  */
> > > > -VhostShadowVirtqueue *vhost_svq_new(void)
> > > > +VhostShadowVirtqueue *vhost_svq_new(uint16_t qsize)
> > > > {
> > > > +size_t desc_size = sizeof(vring_desc_t) * qsize;
> > > > +size_t device_size, driver_size;
> > > > g_autofree VhostShadowVirtqueue *svq = 
> > > > g_new0(VhostShadowVirtqueue, 1);
> > > > int r;
> > > >
> > > > @@ -300,6 +308,15 @@ VhostShadowVirtqueue *vhost_svq_new(void)
> > > > /* Placeholder descriptor, it should be deleted at 
> > > > set_kick_fd */
> > > > event_notifier_init_fd(>svq_kick, INVALID_SVQ_KICK_FD);
> > > >
> > > > +svq->vring.num = qsize;
> > >  I wonder if this is the best. E.g some hardware can support up to 32K
> > >  queue size. So this will probably end up with:
> > > 
> > >  1) SVQ use 32K queue size
> > >  2) hardware queue uses 256
> > > 
> > > >>> In that case SVQ vring queue size will be 32K and guest's vring can
> > > >>> negotiate any number with SVQ equal or less than 32K,
> > > >>
> > > >> Sorry for being unclear what I meant is actually
> > > >>
> > > >> 1) SVQ uses 32K queue size
> > > >>
> > > >> 2) guest vq uses 256
> > > >>
> > > >> This looks like a burden that needs extra logic and may damage the
> > > >> performance.
> > > >>
> > > > Still not getting this point.
> > > >
> > > > An available guest buffer, although contiguous in GPA/GVA, can expand
> > > > in multiple buffers if it's not contiguous in qemu's VA (by the while
> > > > loop in virtqueue_map_desc [1]). In that scenario it is better to have
> > > > "plenty" of SVQ buffers.
> > >
> > >
> > > Yes, but this case should be rare. So in this case we should deal with
> > > overrun on SVQ, that is
> > >
> > > 1) SVQ is full
> > > 2) guest VQ isn't
> > >
> > > We need to
> > >
> > > 1) check the available buffer slots
> > > 2) disable guest kick and wait for the used buffers
> > >
> > > But it looks to me the current code is not ready for dealing with this 
> > > case?
> > >
> >
> > Yes it deals, that's the meaning of svq->next_guest_avail_elem.
>
> Oh right, I missed that.
>
> >
> > >
> > > >
> > > > I'm ok if we decide to put an upper limit though, or if we decide not
> > > > to handle this situation. But we would leave out valid virtio drivers.
> > > > Maybe to set a fixed upper limit (1024?)? To add another parameter
> > > > 

Re: [PATCH 28/31] vdpa: Expose VHOST_F_LOG_ALL on SVQ

[Jason Wang]

在 2022/2/17 下午4:22, Eugenio Perez Martin 写道:

On Thu, Feb 17, 2022 at 7:02 AM Jason Wang  wrote:

On Wed, Feb 16, 2022 at 11:54 PM Eugenio Perez Martin
 wrote:

On Tue, Feb 8, 2022 at 9:25 AM Jason Wang  wrote:


在 2022/2/1 下午7:45, Eugenio Perez Martin 写道:

On Sun, Jan 30, 2022 at 7:50 AM Jason Wang  wrote:

在 2022/1/22 上午4:27, Eugenio Pérez 写道:

SVQ is able to log the dirty bits by itself, so let's use it to not
block migration.

Also, ignore set and clear of VHOST_F_LOG_ALL on set_features if SVQ is
enabled. Even if the device supports it, the reports would be nonsense
because SVQ memory is in the qemu region.

The log region is still allocated. Future changes might skip that, but
this series is already long enough.

Signed-off-by: Eugenio Pérez 
---
hw/virtio/vhost-vdpa.c | 20 
1 file changed, 20 insertions(+)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index fb0a338baa..75090d65e8 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -1022,6 +1022,9 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev, 
uint64_t *features)
if (ret == 0 && v->shadow_vqs_enabled) {
/* Filter only features that SVQ can offer to guest */
vhost_svq_valid_guest_features(features);
+
+/* Add SVQ logging capabilities */
+*features |= BIT_ULL(VHOST_F_LOG_ALL);
}

return ret;
@@ -1039,8 +1042,25 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev,

if (v->shadow_vqs_enabled) {
uint64_t dev_features, svq_features, acked_features;
+uint8_t status = 0;
bool ok;

+ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, );
+if (unlikely(ret)) {
+return ret;
+}
+
+if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
+/*
+ * vhost is trying to enable or disable _F_LOG, and the device
+ * would report wrong dirty pages. SVQ handles it.
+ */

I fail to understand this comment, I'd think there's no way to disable
dirty page tracking for SVQ.


vhost_log_global_{start,stop} are called at the beginning and end of
migration. To inform the device that it should start logging, they set
or clean VHOST_F_LOG_ALL at vhost_dev_set_log.


Yes, but for SVQ, we can't disable dirty page tracking, isn't it? The
only thing is to ignore or filter out the F_LOG_ALL and pretend to be
enabled and disabled.


Yes, that's what this patch does.


While SVQ does not use VHOST_F_LOG_ALL, it exports the feature bit so
vhost does not block migration. Maybe we need to look for another way
to do this?


I'm fine with filtering since it's much more simpler, but I fail to
understand why we need to check DRIVER_OK.


Ok maybe I can make that part more clear,

Since both operations use vhost_vdpa_set_features we must just filter
the one that actually sets or removes VHOST_F_LOG_ALL, without
affecting other features.

In practice, that means to not forward the set features after
DRIVER_OK. The device is not expecting them anymore.

I wonder what happens if we don't do this.


If we simply delete the check vhost_dev_set_features will return an
error, failing the start of the migration. More on this below.



Ok.





So kernel had this check:

 /*
  * It's not allowed to change the features after they have
  * been negotiated.
  */
if (ops->get_status(vdpa) & VIRTIO_CONFIG_S_FEATURES_OK)
 return -EBUSY;

So is it FEATURES_OK actually?


Yes, FEATURES_OK seems more appropriate actually so I will switch to
it for the next version.

But it should be functionally equivalent, since
vhost.c:vhost_dev_start sets both and the setting of _F_LOG_ALL cannot
be concurrent with it.



Right.





For this patch, I wonder if the thing we need to do is to see whether
it is a enable/disable F_LOG_ALL and simply return.


Yes, that's the intention of the patch.

We have 4 cases here:
a) We're being called from vhost_dev_start, with enable_log = false
b) We're being called from vhost_dev_start, with enable_log = true



And this case makes us can't simply return without calling vhost-vdpa.



c) We're being called from vhost_dev_set_log, with enable_log = false
d) We're being called from vhost_dev_set_log, with enable_log = true

The way to tell the difference between a/b and c/d is to check if
{FEATURES,DRIVER}_OK is set. And, as you point out in previous mails,
F_LOG_ALL must be filtered unconditionally since SVQ tracks dirty
memory through the memory unmapping, so we clear the bit
unconditionally if we detect that VHOST_SET_FEATURES will be called
(cases a and b).

Another possibility is to track if features have been set with a bool
in vhost_vdpa or something like that. But it seems cleaner to me to
only store that in the actual device.



So I suggest to make sure codes match the comment:

    if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
    /*
 * vhost is trying to 

Re: [PATCH v2 2/2] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table

[Ani Sinha]

> >
> > diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> > index 68ca7e7fc2..756c69b3b0 100644
> > --- a/hw/i386/acpi-microvm.c
> > +++ b/hw/i386/acpi-microvm.c
> > @@ -189,6 +189,11 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
> >  .reset_val = ACPI_GED_RESET_VALUE,
> >  };
> >
> > +if (isa_check_device_existence("i8042")) {
> > +/* Indicates if i8042 is present or not */
> > +pmfadt.iapc_boot_arch = (1 << 1);
> > +}
> > +
> >  table_offsets = g_array_new(false, true /* clear */,
> >  sizeof(uint32_t));
>
>
> We should do the same thing for arm architecture as well?
> hw/arm/virt-acpi-build.c .

Probably not since the spec says
"These flags pertain only to IA-PC platforms. On other system
architectures, the entire field should be set
to 0."

adding qemu-arm for confirmation.

Re: [PATCH 18/31] vhost: Shadow virtqueue buffers forwarding

[Jason Wang]

在 2022/2/21 下午4:15, Eugenio Perez Martin 写道:

On Mon, Feb 21, 2022 at 8:44 AM Jason Wang  wrote:


在 2022/2/17 下午8:48, Eugenio Perez Martin 写道:

On Tue, Feb 8, 2022 at 9:16 AM Jason Wang  wrote:

在 2022/2/1 下午7:25, Eugenio Perez Martin 写道:

On Sun, Jan 30, 2022 at 7:47 AM Jason Wang  wrote:

在 2022/1/22 上午4:27, Eugenio Pérez 写道:

@@ -272,6 +590,28 @@ void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, 
int svq_kick_fd)
 void vhost_svq_stop(VhostShadowVirtqueue *svq)
 {
 event_notifier_set_handler(>svq_kick, NULL);
+g_autofree VirtQueueElement *next_avail_elem = NULL;
+
+if (!svq->vq) {
+return;
+}
+
+/* Send all pending used descriptors to guest */
+vhost_svq_flush(svq, false);

Do we need to wait for all the pending descriptors to be completed here?


No, this function does not wait, it only completes the forwarding of
the *used* descriptors.

The best example is the net rx queue in my opinion. This call will
check SVQ's vring used_idx and will forward the last used descriptors
if any, but all available descriptors will remain as available for
qemu's VQ code.

To skip it would miss those last rx descriptors in migration.

Thanks!

So it's probably to not the best place to ask. It's more about the
inflight descriptors so it should be TX instead of RX.

I can imagine the migration last phase, we should stop the vhost-vDPA
before calling vhost_svq_stop(). Then we should be fine regardless of
inflight descriptors.


I think I'm still missing something here.

To be on the same page. Regarding tx this could cause repeated tx
frames (one at source and other at destination), but never a missed
buffer not transmitted. The "stop before" could be interpreted as "SVQ
is not forwarding available buffers anymore". Would that work?


Right, but this only work if

1) a flush to make sure TX DMA for inflight descriptors are all completed

2) just mark all inflight descriptor used


It currently trusts on the reverse: Buffers not marked as used (by the
device) will be available in the destination, so expect
retransmissions.



I may miss something but I think we do migrate last_avail_idx. So there 
won't be a re-transmission, since we depend on qemu virtqueue code to 
deal with vring base?


Thanks




Thanks!


Otherwise there could be buffers that is inflight forever.

Thanks



Thanks!


Thanks



Thanks



+
+for (unsigned i = 0; i < svq->vring.num; ++i) {
+g_autofree VirtQueueElement *elem = NULL;
+elem = g_steal_pointer(>ring_id_maps[i]);
+if (elem) {
+virtqueue_detach_element(svq->vq, elem, elem->len);
+}
+}
+
+next_avail_elem = g_steal_pointer(>next_guest_avail_elem);
+if (next_avail_elem) {
+virtqueue_detach_element(svq->vq, next_avail_elem,
+ next_avail_elem->len);
+}
 }

[PATCH v3 0/2] hw/i386: OVMF table parsing fixes

[Dov Murik]

Fix missing bounds check when parsing the OVMF table.

This already had two iterations as a single patch; I decided to split it
to two patches.  The first deals only with bounds checking, and the
second is a non-functional change to clear the code according to
reviewers' suggestions.

v3:
- simplify bounds check and remove max_tot_len (thanks Dave)
- split one patch to two

v2:
- add error message example to commit description
- replace magic numbers 48 and 50 with size calculations (thanks Phil
  MD)

Dov Murik (2):
  hw/i386: Improve bounds checking in OVMF table parsing
  hw/i386: Replace magic number with field length calculation

 hw/i386/pc_sysfw_ovmf.c | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)


base-commit: 477c3b934a47adf7de285863f59d6e4503dd1a6d
-- 
2.25.1

Re: [PATCH 09/31] vhost-vdpa: Take into account SVQ in vhost_vdpa_set_vring_call

[Jason Wang]

在 2022/2/21 下午4:01, Eugenio Perez Martin 写道:

On Mon, Feb 21, 2022 at 8:39 AM Jason Wang  wrote:


在 2022/2/18 下午8:35, Eugenio Perez Martin 写道:

On Tue, Feb 8, 2022 at 4:23 AM Jason Wang  wrote:

在 2022/1/31 下午11:34, Eugenio Perez Martin 写道:

On Sat, Jan 29, 2022 at 9:06 AM Jason Wang  wrote:

在 2022/1/22 上午4:27, Eugenio Pérez 写道:

Signed-off-by: Eugenio Pérez 
---
 hw/virtio/vhost-vdpa.c | 20 ++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 18de14f0fb..029f98feee 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -687,13 +687,29 @@ static int vhost_vdpa_set_vring_kick(struct vhost_dev 
*dev,
 }
 }

-static int vhost_vdpa_set_vring_call(struct vhost_dev *dev,
-   struct vhost_vring_file *file)
+static int vhost_vdpa_set_vring_dev_call(struct vhost_dev *dev,
+ struct vhost_vring_file *file)
 {
 trace_vhost_vdpa_set_vring_call(dev, file->index, file->fd);
 return vhost_vdpa_call(dev, VHOST_SET_VRING_CALL, file);
 }

+static int vhost_vdpa_set_vring_call(struct vhost_dev *dev,
+ struct vhost_vring_file *file)
+{
+struct vhost_vdpa *v = dev->opaque;
+
+if (v->shadow_vqs_enabled) {
+int vdpa_idx = vhost_vdpa_get_vq_index(dev, file->index);
+VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, vdpa_idx);
+
+vhost_svq_set_guest_call_notifier(svq, file->fd);

Two questions here (had similar questions for vring kick):

1) Any reason that we setup the eventfd for vhost-vdpa in
vhost_vdpa_svq_setup() not here?


I'm not sure what you mean.

The guest->SVQ call and kick fds are set here and at
vhost_vdpa_set_vring_kick. The event notifier handler of the guest ->
SVQ kick_fd is set at vhost_vdpa_set_vring_kick /
vhost_svq_set_svq_kick_fd. The guest -> SVQ call fd has no event
notifier handler since we don't poll it.

On the other hand, the connection SVQ <-> device uses the same fds
from the beginning to the end, and they will not change with, for
example, call fd masking. That's why it's setup from
vhost_vdpa_svq_setup. Delaying to vhost_vdpa_set_vring_call would make
us add way more logic there.

More logic in general shadow vq code but less codes for vhost-vdpa
specific code I think.

E.g for we can move the kick set logic from vhost_vdpa_svq_set_fds() to
here.


But they are different fds. vhost_vdpa_svq_set_fds sets the
SVQ<->device. This function sets the SVQ->guest call file descriptor.

To move the logic of vhost_vdpa_svq_set_fds here would imply either:
a) Logic to know if we are receiving the first call fd or not.


Any reason for this? I guess you meant multiqueue. If yes, it should not
be much difference since we have idx as the parameter.


With "first call fd" I meant "first time we receive the call fd", so
we only set them once.

I think this is going to be easier if I prepare a patch doing your way
and we comment on it.



That would be helpful but if there's no issue with current code (see 
below), we can leave it as is and do optimization on top.






   That
code is not in the series at the moment, because setting at
vhost_vdpa_dev_start tells the difference for free. Is just adding
code, not moving.
b) Logic to set again *the same* file descriptor to device, with logic
to tell if we have missed calls. That logic is not implemented for
device->SVQ call file descriptor, because we are assuming it never
changes from vhost_vdpa_svq_set_fds. So this is again adding code.

At this moment, we have:
vhost_vdpa_svq_set_fds:
set SVQ<->device fds

vhost_vdpa_set_vring_call:
set guest<-SVQ call

vhost_vdpa_set_vring_kick:
set guest->SVQ kick.

If I understood correctly, the alternative would be something like:
vhost_vdpa_set_vring_call:
set guest<-SVQ call
if(!vq->call_set) {
  - set SVQ<-device call.
  - vq->call_set = true
}

vhost_vdpa_set_vring_kick:
set guest<-SVQ call
if(!vq->dev_kick_set) {
  - set guest->device kick.
  - vq->dev_kick_set = true
}

dev_reset / dev_stop:
for vq in vqs:
vq->dev_kick_set = vq->dev_call_set = false
...

Or have I misunderstood something?


I wonder what happens if MSI-X is masking in guest. So if I understand
correctly, we don't disable the eventfd from device? If yes, this seems
suboptinal.


We cannot disable the device's call fd unless SVQ actively poll it. As
I see it, if the guest masks the call fd, it could be because:
a) it doesn't want to receive more calls because is processing buffers
b) Is going to burn a cpu to poll it.

The masking only affects SVQ->guest call. If we also mask device->SVQ,
we're adding latency in the case a), and we're effectively disabling
forwarding in case b).



Right, so we need leave a comment to explain this, then I'm totally fine 
with this approach.





It only works if guest is 

[PATCH v3 1/2] hw/i386: Improve bounds checking in OVMF table parsing

[Dov Murik]

When pc_system_parse_ovmf_flash() parses the optional GUIDed table in
the end of the OVMF flash memory area, the table length field is checked
for sizes that are too small, but doesn't error on sizes that are too
big (bigger than the flash content itself).

Add a check for maximal size of the OVMF table, and add an error report
in case the size is invalid.  In such a case, an error like this will be
displayed during launch:

qemu-system-x86_64: OVMF table has invalid size 4047

and the table parsing is skipped.

Signed-off-by: Dov Murik 
---
 hw/i386/pc_sysfw_ovmf.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc_sysfw_ovmf.c b/hw/i386/pc_sysfw_ovmf.c
index f4dd92c588..df15c9737b 100644
--- a/hw/i386/pc_sysfw_ovmf.c
+++ b/hw/i386/pc_sysfw_ovmf.c
@@ -24,6 +24,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "hw/i386/pc.h"
 #include "cpu.h"
 
@@ -66,7 +67,13 @@ void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t 
flash_size)
 ptr -= sizeof(uint16_t);
 tot_len = le16_to_cpu(*(uint16_t *)ptr) - sizeof(guid) - sizeof(uint16_t);
 
-if (tot_len <= 0) {
+if (tot_len < 0 || tot_len > (ptr - flash_ptr)) {
+error_report("OVMF table has invalid size %d", tot_len);
+return;
+}
+
+if (tot_len == 0) {
+/* no entries in the OVMF table */
 return;
 }
 
-- 
2.25.1

[PATCH v3 2/2] hw/i386: Replace magic number with field length calculation

[Dov Murik]

Replce the literal magic number 48 with length calculation (32 bytes at
the end of the firmware after the table footer + 16 bytes of the OVMF
table footer GUID).

No functional change intended.

Signed-off-by: Dov Murik 
---
 hw/i386/pc_sysfw_ovmf.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc_sysfw_ovmf.c b/hw/i386/pc_sysfw_ovmf.c
index df15c9737b..07a4c267fa 100644
--- a/hw/i386/pc_sysfw_ovmf.c
+++ b/hw/i386/pc_sysfw_ovmf.c
@@ -30,6 +30,8 @@
 
 #define OVMF_TABLE_FOOTER_GUID "96b582de-1fb2-45f7-baea-a366c55a082d"
 
+static const int bytes_after_table_footer = 32;
+
 static bool ovmf_flash_parsed;
 static uint8_t *ovmf_table;
 static int ovmf_table_len;
@@ -53,12 +55,13 @@ void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t 
flash_size)
 
 /*
  * if this is OVMF there will be a table footer
- * guid 48 bytes before the end of the flash file.  If it's
- * not found, silently abort the flash parsing.
+ * guid 48 bytes before the end of the flash file
+ * (= 32 bytes after the table + 16 bytes the GUID itself).
+ * If it's not found, silently abort the flash parsing.
  */
 qemu_uuid_parse(OVMF_TABLE_FOOTER_GUID, );
 guid = qemu_uuid_bswap(guid); /* guids are LE */
-ptr = flash_ptr + flash_size - 48;
+ptr = flash_ptr + flash_size - (bytes_after_table_footer + sizeof(guid));
 if (!qemu_uuid_is_equal((QemuUUID *)ptr, )) {
 return;
 }
-- 
2.25.1

Re: [PATCH v2 2/2] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table

[Ani Sinha]

>  };
> +if (isa_check_device_existence("i8042")) {
> +/* Indicates if i8042 is present or not */
> +fadt.iapc_boot_arch = (1 << 1);
> +}
> +
>  *data = fadt;
>  }
>

We do these things slightly differently. how about

/*
* second bit of 16 but IAPC_BOOT_ARCH indicates presence of 8042 or
equivalent
micro controller. See table 5-10 of APCI spec version 2.0 (the earliest
acpi revision that supports this).
*/

fadt.iapc_boot_arch = isa_check_device_existence("i8042")? 0x0002:0x;

> diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> index 68ca7e7fc2..756c69b3b0 100644
> --- a/hw/i386/acpi-microvm.c
> +++ b/hw/i386/acpi-microvm.c
> @@ -189,6 +189,11 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
>  .reset_val = ACPI_GED_RESET_VALUE,
>  };
>
> +if (isa_check_device_existence("i8042")) {
> +/* Indicates if i8042 is present or not */
> +pmfadt.iapc_boot_arch = (1 << 1);
> +}
> +

Ditto.

Re: [PATCH v2 2/2] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table

[Ani Sinha]

On Mon, 21 Feb 2022, Liav Albani wrote:

> This can allow the guest OS to determine more easily if i8042 controller
> is present in the system or not, so it doesn't need to do probing of the
> controller, but just initialize it immediately, before enumerating the
> ACPI AML namespace.
>
> Signed-off-by: Liav Albani 
> ---
>  hw/acpi/aml-build.c | 7 ++-
>  hw/i386/acpi-build.c| 5 +
>  hw/i386/acpi-microvm.c  | 5 +
>  include/hw/acpi/acpi-defs.h | 1 +
>  4 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 8966e16320..ef5f4cad87 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2152,7 +2152,12 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const 
> AcpiFadtData *f,
>  build_append_int_noprefix(tbl, 0, 1); /* DAY_ALRM */
>  build_append_int_noprefix(tbl, 0, 1); /* MON_ALRM */
>  build_append_int_noprefix(tbl, f->rtc_century, 1); /* CENTURY */
> -build_append_int_noprefix(tbl, 0, 2); /* IAPC_BOOT_ARCH */
> +/* IAPC_BOOT_ARCH */
> +if (f->rev == 1) {
> +build_append_int_noprefix(tbl, 0, 2);
> +} else {
> +build_append_int_noprefix(tbl, f->iapc_boot_arch, 2);
> +}

So your change will only apply for q35 machines and not for pc types. You
should write a comment saying that this is not defined in acpi spec 1.0
where revision == 1 also applies.
I see that IAPC boot arch is defined as
old as ACPI version 2:

https://uefi.org/sites/default/files/resources/ACPI_2.pdf
Section 5.2.8

On a unrelatd note, I see FADT revision is hardcoded to 3 even as old as
ACPI version 2. *Except* in ACPI version 1b, it is hardcoded to 1 which
w2k seems to like :-) (table 5-5 in
https://uefi.org/sites/default/files/resources/ACPI_1_Errata_B.pdf) .
I will add a comment in the code related to this.


>  build_append_int_noprefix(tbl, 0, 1); /* Reserved */
>  build_append_int_noprefix(tbl, f->flags, 4); /* Flags */
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index ebd47aa26f..5dc625b8d8 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -192,6 +192,11 @@ static void init_common_fadt_data(MachineState *ms, 
> Object *o,
>  .address = object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK, 
> NULL)
>  },
>  };
> +if (isa_check_device_existence("i8042")) {
> +/* Indicates if i8042 is present or not */
> +fadt.iapc_boot_arch = (1 << 1);
> +}
> +
>  *data = fadt;
>  }
>
> diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> index 68ca7e7fc2..756c69b3b0 100644
> --- a/hw/i386/acpi-microvm.c
> +++ b/hw/i386/acpi-microvm.c
> @@ -189,6 +189,11 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
>  .reset_val = ACPI_GED_RESET_VALUE,
>  };
>
> +if (isa_check_device_existence("i8042")) {
> +/* Indicates if i8042 is present or not */
> +pmfadt.iapc_boot_arch = (1 << 1);
> +}
> +
>  table_offsets = g_array_new(false, true /* clear */,
>  sizeof(uint32_t));


We should do the same thing for arm architecture as well?
hw/arm/virt-acpi-build.c .


>  bios_linker_loader_alloc(tables->linker,
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index c97e8633ad..2b42e4192b 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -77,6 +77,7 @@ typedef struct AcpiFadtData {
>  uint16_t plvl2_lat;/* P_LVL2_LAT */
>  uint16_t plvl3_lat;/* P_LVL3_LAT */
>  uint16_t arm_boot_arch;/* ARM_BOOT_ARCH */
> +uint16_t iapc_boot_arch;   /* IAPC_BOOT_ARCH */
>  uint8_t minor_ver; /* FADT Minor Version */
>
>  /*
> --
> 2.35.1
>
>

Re: [PATCH v2] hw/i386: Improve bounds checking in OVMF table parsing

[Dov Murik]

Thanks Dave for reviewing.


On 21/02/2022 21:44, Dr. David Alan Gilbert wrote:
> * Dov Murik (dovmu...@linux.ibm.com) wrote:
>> When pc_system_parse_ovmf_flash() parses the optional GUIDed table in
>> the end of the OVMF flash memory area, the table length field is checked
>> for sizes that are too small, but doesn't error on sizes that are too
>> big (bigger than the flash content itself).
>>
>> Add a check for maximal size of the OVMF table, and add an error report
>> in case the size is invalid.  In such a case, an error like this will be
>> displayed during launch:
>>
>> qemu-system-x86_64: OVMF table has invalid size 4047
>>
>> and the table parsing is skipped.
>>
>> Signed-off-by: Dov Murik 
>>
>> ---
>>
>> v2:
>> - add error message example to commit description
>> - replace magic numbers 48 and 50 with size calculations (thanks Phil MD)
>> ---
>>  hw/i386/pc_sysfw_ovmf.c | 20 
>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/i386/pc_sysfw_ovmf.c b/hw/i386/pc_sysfw_ovmf.c
>> index f4dd92c588..1c9a16e9e6 100644
>> --- a/hw/i386/pc_sysfw_ovmf.c
>> +++ b/hw/i386/pc_sysfw_ovmf.c
>> @@ -24,11 +24,14 @@
>>   */
>>  
>>  #include "qemu/osdep.h"
>> +#include "qemu/error-report.h"
>>  #include "hw/i386/pc.h"
>>  #include "cpu.h"
>>  
>>  #define OVMF_TABLE_FOOTER_GUID "96b582de-1fb2-45f7-baea-a366c55a082d"
>>  
>> +static const int bytes_after_table_footer = 32;
>> +
>>  static bool ovmf_flash_parsed;
>>  static uint8_t *ovmf_table;
>>  static int ovmf_table_len;
>> @@ -38,6 +41,8 @@ void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t 
>> flash_size)
>>  uint8_t *ptr;
>>  QemuUUID guid;
>>  int tot_len;
>> +int max_tot_len = flash_size - (bytes_after_table_footer +
>> +sizeof(guid) + sizeof(uint16_t));
>>  
>>  /* should only be called once */
>>  if (ovmf_flash_parsed) {
>> @@ -52,12 +57,13 @@ void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, 
>> size_t flash_size)
>>  
>>  /*
>>   * if this is OVMF there will be a table footer
>> - * guid 48 bytes before the end of the flash file.  If it's
>> - * not found, silently abort the flash parsing.
>> + * guid 48 bytes before the end of the flash file
>> + * (= 32 bytes after the table + 16 bytes the GUID itself).
>> + * If it's not found, silently abort the flash parsing.
>>   */
>>  qemu_uuid_parse(OVMF_TABLE_FOOTER_GUID, );
>>  guid = qemu_uuid_bswap(guid); /* guids are LE */
>> -ptr = flash_ptr + flash_size - 48;
>> +ptr = flash_ptr + flash_size - (bytes_after_table_footer + 
>> sizeof(guid));
>>  if (!qemu_uuid_is_equal((QemuUUID *)ptr, )) {
>>  return;
>>  }


I'll probably split the two hunks above (without max_tot_len) to a
separate patch "hw/i386: Replace magic number with field length
calculation".



>> @@ -66,7 +72,13 @@ void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, 
>> size_t flash_size)
>>  ptr -= sizeof(uint16_t);
>>  tot_len = le16_to_cpu(*(uint16_t *)ptr) - sizeof(guid) - 
>> sizeof(uint16_t);
> 
> Instead of the max_tot_len calculation above, is it actually:
>max_tot_len = ptr - flash_ptr;
> 
> I think that works out the same and avoids doing the calculation in two
> places; it's also logically what you have - you can't read over the
> structure you just read.

Good call, it indeed gives the same result.


I'll change the condition below to:

if (tot_len < 0 || tot_len > (ptr - flash_ptr))

and remove the max_tot_len variable, and put this in a separate patch
that only adds this condition to solve the overflow problem.


Thanks,
-Dov



> 
> Dave
> 
>> -if (tot_len <= 0) {
>> +if (tot_len < 0 || tot_len > max_tot_len) {
>> +error_report("OVMF table has invalid size %d", tot_len);
>> +return;
>> +}
>> +
>> +if (tot_len == 0) {
>> +/* no entries in the OVMF table */
>>  return;
>>  }
>>  
>>
>> base-commit: 48033ad678ae2def43bf0d543a2c4c3d2a93feaf
>> -- 
>> 2.25.1
>>

Re: [PATCH 0/5] qmp-shell modifications for non-interactive use

[Markus Armbruster]

Damien Hedde  writes:

> Hi,
>
> The main idea of this series is to be a bit more user-friendly when
> using qmp-shell in a non-interactive way: with an input redirection
> from a file containing a list of commands.
>
> I'm working on dynamic qapi config of a qemu machine, this would
> be very useful to provide and reproduce small examples.

Why not use plain QMP for that?

[...]

Re: [PATCH 2/3] util/main-loop: Introduce the main loop into QOM

[Markus Armbruster]

Nicolas Saenz Julienne  writes:

> 'event-loop-backend' provides basic property handling for all
> 'AioContext' based event loops. So let's define a new 'MainLoopClass'
> that inherits from it. This will permit tweaking the main loop's
> properties through qapi as well as through the command line using the
> '-object' keyword[1]. Only one instance of 'MainLoopClass' might be
> created at any time.
>
> 'EventLoopBackendClass' learns a new callback, 'can_be_deleted()' so as
> to mark 'MainLoop' as non-deletable.
>
> Signed-off-by: Nicolas Saenz Julienne 
>
> [1] For example:
>   -object main-loop,id=main-loop,poll-max-ns=

QAPI schema
Acked-by: Markus Armbruster 

Re: [PATCH v8 5/5] multifd: Implement zero copy write in multifd migration (multifd-zero-copy)

[Leonardo Bras Soares Passos]

On Mon, Feb 21, 2022 at 4:41 PM Leonardo Bras Soares Passos
 wrote:
>
> Hello Juan, thanks for the feedback!
>
> On Fri, Feb 18, 2022 at 1:57 PM Juan Quintela  wrote:
> >
> > Leonardo Bras  wrote:
> > > Implement zero copy send on nocomp_send_write(), by making use of 
> > > QIOChannel
> > > writev + flags & flush interface.
> > >
> > > Change multifd_send_sync_main() so flush_zero_copy() can be called
> > > after each iteration in order to make sure all dirty pages are sent before
> > > a new iteration is started. It will also flush at the beginning and at the
> > > end of migration.
> > >
> > > Also make it return -1 if flush_zero_copy() fails, in order to cancel
> > > the migration process, and avoid resuming the guest in the target host
> > > without receiving all current RAM.
> > >
> > > This will work fine on RAM migration because the RAM pages are not 
> > > usually freed,
> > > and there is no problem on changing the pages content between 
> > > writev_zero_copy() and
> > > the actual sending of the buffer, because this change will dirty the page 
> > > and
> > > cause it to be re-sent on a next iteration anyway.
> > >
> > > A lot of locked memory may be needed in order to use multid migration
> >^^
> > multifd.
> >
> > I can fix it on the commit.
>
> No worries, fixed for v9.
>
> >
> >
> > > @@ -1479,7 +1479,16 @@ static bool 
> > > migrate_params_check(MigrationParameters *params, Error **errp)
> > >  error_prepend(errp, "Invalid mapping given for 
> > > block-bitmap-mapping: ");
> > >  return false;
> > >  }
> > > -
> > > +#ifdef CONFIG_LINUX
> > > +if (params->zero_copy_send &&
> > > +(!migrate_use_multifd() ||
> > > + params->multifd_compression != MULTIFD_COMPRESSION_NONE ||
> > > + (params->tls_creds && *params->tls_creds))) {
> > > +error_setg(errp,
> > > +   "Zero copy only available for non-compressed non-TLS 
> > > multifd migration");
> > > +return false;
> > > +}
> > > +#endif
> > >  return true;
> > >  }
> >
> > Test is long, but it is exactly what we need.  Good.
>
> Thanks!
>
>
> >
> >
> > >
> > > diff --git a/migration/multifd.c b/migration/multifd.c
> > > index 43998ad117..2d68b9cf4f 100644
> > > --- a/migration/multifd.c
> > > +++ b/migration/multifd.c
> > > @@ -568,19 +568,28 @@ void multifd_save_cleanup(void)
> > >  multifd_send_state = NULL;
> > >  }
> > >
> > > -void multifd_send_sync_main(QEMUFile *f)
> > > +int multifd_send_sync_main(QEMUFile *f)
> > >  {
> > >  int i;
> > > +bool flush_zero_copy;
> > >
> > >  if (!migrate_use_multifd()) {
> > > -return;
> > > +return 0;
> > >  }
> > >  if (multifd_send_state->pages->num) {
> > >  if (multifd_send_pages(f) < 0) {
> > >  error_report("%s: multifd_send_pages fail", __func__);
> > > -return;
> > > +return 0;
> > >  }
> > >  }
> > > +
> > > +/*
> > > + * When using zero-copy, it's necessary to flush after each 
> > > iteration to
> > > + * make sure pages from earlier iterations don't end up replacing 
> > > newer
> > > + * pages.
> > > + */
> > > +flush_zero_copy = migrate_use_zero_copy_send();
> > > +
> > >  for (i = 0; i < migrate_multifd_channels(); i++) {
> > >  MultiFDSendParams *p = _send_state->params[i];
> > >
> > > @@ -591,7 +600,7 @@ void multifd_send_sync_main(QEMUFile *f)
> > >  if (p->quit) {
> > >  error_report("%s: channel %d has already quit", __func__, i);
> > >  qemu_mutex_unlock(>mutex);
> > > -return;
> > > +return 0;
> > >  }
> > >
> > >  p->packet_num = multifd_send_state->packet_num++;
> > > @@ -602,6 +611,17 @@ void multifd_send_sync_main(QEMUFile *f)
> > >  ram_counters.transferred += p->packet_len;
> > >  qemu_mutex_unlock(>mutex);
> > >  qemu_sem_post(>sem);
> > > +
> > > +if (flush_zero_copy) {
> > > +int ret;
> > > +Error *err = NULL;
> > > +
> > > +ret = qio_channel_flush(p->c, );
> > > +if (ret < 0) {
> > > +error_report_err(err);
> > > +return -1;
> > > +}
> > > +}
> > >  }
> > >  for (i = 0; i < migrate_multifd_channels(); i++) {
> > >  MultiFDSendParams *p = _send_state->params[i];
> > > @@ -610,6 +630,8 @@ void multifd_send_sync_main(QEMUFile *f)
> > >  qemu_sem_wait(>sem_sync);
> > >  }
> > >  trace_multifd_send_sync_main(multifd_send_state->packet_num);
> > > +
> > > +return 0;
> > >  }
> >
> > We are leaving pages is flight for potentially a lot of time. I *think*
> > that we can sync shorter than that.
> >
> > >  static void *multifd_send_thread(void *opaque)
> > > @@ -668,8 +690,8 @@ static void *multifd_send_thread(void *opaque)
> > >  p->iov[0].iov_len = 

Re: [PATCH 17/31] vdpa: adapt vhost_ops callbacks to svq

[Jason Wang]

On Tue, Feb 22, 2022 at 1:23 AM Eugenio Perez Martin
 wrote:
>
> On Mon, Feb 21, 2022 at 8:15 AM Jason Wang  wrote:
> >
> >
> > 在 2022/2/18 上午1:13, Eugenio Perez Martin 写道:
> > > On Tue, Feb 8, 2022 at 4:58 AM Jason Wang  wrote:
> > >>
> > >> 在 2022/2/1 上午2:58, Eugenio Perez Martin 写道:
> > >>> On Sun, Jan 30, 2022 at 5:03 AM Jason Wang  wrote:
> >  在 2022/1/22 上午4:27, Eugenio Pérez 写道:
> > > First half of the buffers forwarding part, preparing vhost-vdpa
> > > callbacks to SVQ to offer it. QEMU cannot enable it at this moment, so
> > > this is effectively dead code at the moment, but it helps to reduce
> > > patch size.
> > >
> > > Signed-off-by: Eugenio Pérez 
> > > ---
> > > hw/virtio/vhost-shadow-virtqueue.h |   2 +-
> > > hw/virtio/vhost-shadow-virtqueue.c |  21 -
> > > hw/virtio/vhost-vdpa.c | 133 
> > > ++---
> > > 3 files changed, 143 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/hw/virtio/vhost-shadow-virtqueue.h 
> > > b/hw/virtio/vhost-shadow-virtqueue.h
> > > index 035207a469..39aef5ffdf 100644
> > > --- a/hw/virtio/vhost-shadow-virtqueue.h
> > > +++ b/hw/virtio/vhost-shadow-virtqueue.h
> > > @@ -35,7 +35,7 @@ size_t vhost_svq_device_area_size(const 
> > > VhostShadowVirtqueue *svq);
> > >
> > > void vhost_svq_stop(VhostShadowVirtqueue *svq);
> > >
> > > -VhostShadowVirtqueue *vhost_svq_new(void);
> > > +VhostShadowVirtqueue *vhost_svq_new(uint16_t qsize);
> > >
> > > void vhost_svq_free(VhostShadowVirtqueue *vq);
> > >
> > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c 
> > > b/hw/virtio/vhost-shadow-virtqueue.c
> > > index f129ec8395..7c168075d7 100644
> > > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > > @@ -277,9 +277,17 @@ void vhost_svq_stop(VhostShadowVirtqueue *svq)
> > > /**
> > >  * Creates vhost shadow virtqueue, and instruct vhost device to 
> > > use the shadow
> > >  * methods and file descriptors.
> > > + *
> > > + * @qsize Shadow VirtQueue size
> > > + *
> > > + * Returns the new virtqueue or NULL.
> > > + *
> > > + * In case of error, reason is reported through error_report.
> > >  */
> > > -VhostShadowVirtqueue *vhost_svq_new(void)
> > > +VhostShadowVirtqueue *vhost_svq_new(uint16_t qsize)
> > > {
> > > +size_t desc_size = sizeof(vring_desc_t) * qsize;
> > > +size_t device_size, driver_size;
> > > g_autofree VhostShadowVirtqueue *svq = 
> > > g_new0(VhostShadowVirtqueue, 1);
> > > int r;
> > >
> > > @@ -300,6 +308,15 @@ VhostShadowVirtqueue *vhost_svq_new(void)
> > > /* Placeholder descriptor, it should be deleted at 
> > > set_kick_fd */
> > > event_notifier_init_fd(>svq_kick, INVALID_SVQ_KICK_FD);
> > >
> > > +svq->vring.num = qsize;
> >  I wonder if this is the best. E.g some hardware can support up to 32K
> >  queue size. So this will probably end up with:
> > 
> >  1) SVQ use 32K queue size
> >  2) hardware queue uses 256
> > 
> > >>> In that case SVQ vring queue size will be 32K and guest's vring can
> > >>> negotiate any number with SVQ equal or less than 32K,
> > >>
> > >> Sorry for being unclear what I meant is actually
> > >>
> > >> 1) SVQ uses 32K queue size
> > >>
> > >> 2) guest vq uses 256
> > >>
> > >> This looks like a burden that needs extra logic and may damage the
> > >> performance.
> > >>
> > > Still not getting this point.
> > >
> > > An available guest buffer, although contiguous in GPA/GVA, can expand
> > > in multiple buffers if it's not contiguous in qemu's VA (by the while
> > > loop in virtqueue_map_desc [1]). In that scenario it is better to have
> > > "plenty" of SVQ buffers.
> >
> >
> > Yes, but this case should be rare. So in this case we should deal with
> > overrun on SVQ, that is
> >
> > 1) SVQ is full
> > 2) guest VQ isn't
> >
> > We need to
> >
> > 1) check the available buffer slots
> > 2) disable guest kick and wait for the used buffers
> >
> > But it looks to me the current code is not ready for dealing with this case?
> >
>
> Yes it deals, that's the meaning of svq->next_guest_avail_elem.

Oh right, I missed that.

>
> >
> > >
> > > I'm ok if we decide to put an upper limit though, or if we decide not
> > > to handle this situation. But we would leave out valid virtio drivers.
> > > Maybe to set a fixed upper limit (1024?)? To add another parameter
> > > (x-svq-size-n=N)?
> > >
> > > If you mean we lose performance because memory gets more sparse I
> > > think the only possibility is to limit that way.
> >
> >
> > If guest is not using 32K, having a 32K for svq may gives extra stress
> > on the cache since we will end up with a pretty large working set.
> >
>
> That might be true. My guess 

Re: [PATCH 15/20] migration: Allow migrate-recover to run multiple times

[Peter Xu]

On Mon, Feb 21, 2022 at 05:03:24PM +, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@redhat.com) wrote:
> > Previously migration didn't have an easy way to cleanup the listening
> > transport, migrate recovery only allows to execute once.  That's done with a
> > trick flag in postcopy_recover_triggered.
> > 
> > Now the facility is already there.
> > 
> > Drop postcopy_recover_triggered and instead allows a new migrate-recover to
> > release the previous listener transport.
> > 
> > Signed-off-by: Peter Xu 
> 
> OK, was that the only reason you couldn't recover twice?

We could recover twice, but we couldn't specify the listening port twice
because AFAIU previously we don't have a good way to clean the existing
listener.

IOW we could always run pause->recover->pause->recover sequence even before
this patch[set], but we can never run continuous recover->recover because
the 2nd one will fail telling that we've already setup a recovery port.

> 
> 
> Reviewed-by: Dr. David Alan Gilbert 

Thanks!

-- 
Peter Xu

RE: [PATCH] multifd: ensure multifd threads are terminated before cleanup params

[Wangxin (Alexander)]

Ping.

> 
> In multifd_save_cleanup(), we terminate all multifd threads and destroy
> the 'p->mutex', while the mutex may still be held by multifd send thread,
> this causes qemu to crash.
> 
> It's because the multifd_send_thread maybe scheduled out after setting
> 'p->running' to false. To reproduce the problem, we put
> 'multifd_send_thread' to sleep seconds before unlock 'p->mutex':
> 
> function multifd_send_thread()
> {
> ...
> qemu_mutex_lock(>mutex);
> p->running = false;
> usleep(500);
> 
> qemu_mutex_unlock(>mutex);
> ...
> }
> 
> As the 'p->running' is used to indicate whether the multifd_send/recv thread
> is created, it should be set to false after the thread terminate.
> 
> Signed-off-by: Wang Xin 
> Signed-off-by: Huangyu Zhai 
> 
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 76b57a7177..d8fc7d319e 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -526,6 +526,7 @@ void multifd_save_cleanup(void)
> 
>  if (p->running) {
>  qemu_thread_join(>thread);
> +p->running = false;
>  }
>  }
>  for (i = 0; i < migrate_multifd_channels(); i++) {
> @@ -707,10 +708,6 @@ out:
>  qemu_sem_post(_send_state->channels_ready);
>  }
> 
> -qemu_mutex_lock(>mutex);
> -p->running = false;
> -qemu_mutex_unlock(>mutex);
> -
>  rcu_unregister_thread();
>  trace_multifd_send_thread_end(p->id, p->num_packets, 
> p->total_normal_pages);
> 
> @@ -995,6 +992,7 @@ int multifd_load_cleanup(Error **errp)
>   */
>  qemu_sem_post(>sem_sync);
>  qemu_thread_join(>thread);
> +p->running = false;
>  }
>  }
>  for (i = 0; i < migrate_multifd_channels(); i++) {
> @@ -1110,9 +1108,6 @@ static void *multifd_recv_thread(void *opaque)
>  multifd_recv_terminate_threads(local_err);
>  error_free(local_err);
>  }
> -qemu_mutex_lock(>mutex);
> -p->running = false;
> -qemu_mutex_unlock(>mutex);
> 
>  rcu_unregister_thread();
>  trace_multifd_recv_thread_end(p->id, p->num_packets, 
> p->total_normal_pages);
> --
> 2.26.0.windows.1

Re: [PATCH v4 12/12] KVM: Expose KVM_MEM_PRIVATE

[Maciej S. Szmigiero]

On 17.02.2022 14:45, Chao Peng wrote:

On Tue, Jan 25, 2022 at 09:20:39PM +0100, Maciej S. Szmigiero wrote:

On 18.01.2022 14:21, Chao Peng wrote:

KVM_MEM_PRIVATE is not exposed by default but architecture code can turn
on it by implementing kvm_arch_private_memory_supported().

Also private memslot cannot be movable and the same file+offset can not
be mapped into different GFNs.

Signed-off-by: Yu Zhang 
Signed-off-by: Chao Peng 
---

(..)

   static bool kvm_check_memslot_overlap(struct kvm_memslots *slots, int id,
- gfn_t start, gfn_t end)
+ struct file *file,
+ gfn_t start, gfn_t end,
+ loff_t start_off, loff_t end_off)
   {
struct kvm_memslot_iter iter;
+   struct kvm_memory_slot *slot;
+   struct inode *inode;
+   int bkt;
kvm_for_each_memslot_in_gfn_range(, slots, start, end) {
if (iter.slot->id != id)
return true;
}
+   /* Disallow mapping the same file+offset into multiple gfns. */
+   if (file) {
+   inode = file_inode(file);
+   kvm_for_each_memslot(slot, bkt, slots) {
+   if (slot->private_file &&
+file_inode(slot->private_file) == inode &&
+!(end_off <= slot->private_offset ||
+  start_off >= slot->private_offset
++ (slot->npages >> PAGE_SHIFT)))
+   return true;
+   }
+   }


That's a linear scan of all memslots on each CREATE (and MOVE) operation
with a fd - we just spent more than a year rewriting similar linear scans
into more efficient operations in KVM.


In the last version I tried to solve this problem by using interval tree
(just like existing hva_tree), but finally we realized that in one VM we
can have multiple fds with overlapped offsets so that approach is
incorrect. See https://lkml.org/lkml/2021/12/28/480 for the discussion.


That's right, in this case a two-level structure would be necessary:
the first level matching a file, then the second level matching that
file ranges.
However, if such data is going to be used just for checking possible
overlap at memslot add or move time it is almost certainly an overkill.


So linear scan is used before I can find a better way.


Another option would be to simply not check for overlap at add or move
time, declare such configuration undefined behavior under KVM API and
make sure in MMU notifiers that nothing bad happens to the host kernel
if it turns out somebody actually set up a VM this way (it could be
inefficient in this case, since it's not supposed to ever happen
unless there is a bug somewhere in the userspace part).


Chao


Thanks,
Maciej

Re: [PATCH 1/1] vdpa: Make ncs autofree

[Laurent Vivier]

Le 14/02/2022 à 20:34, Eugenio Pérez a écrit :

Simplifying memory management.

Signed-off-by: Eugenio Pérez 
---
  net/vhost-vdpa.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 4125d13118..4befba5cc7 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -264,7 +264,8 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char 
*name,
  {
  const NetdevVhostVDPAOptions *opts;
  int vdpa_device_fd;
-NetClientState **ncs, *nc;
+g_autofree NetClientState **ncs = NULL;
+NetClientState *nc;
  int queue_pairs, i, has_cvq = 0;
  
  assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);

@@ -302,7 +303,6 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char 
*name,
  goto err;
  }
  
-g_free(ncs);

  return 0;
  
  err:

@@ -310,7 +310,6 @@ err:
  qemu_del_net_client(ncs[0]);
  }
  qemu_close(vdpa_device_fd);
-g_free(ncs);
  
  return -1;

  }


Applied to my trivial-patches branch.

Thanks,
Laurent

[RFC PATCH] gitlab: upgrade the job definition for s390x to 20.04

[Alex Bennée]

The new s390x machine has more of everything including the OS. As
18.04 will soon be going we might as well get onto something moderately
modern.

Signed-off-by: Alex Bennée 
Cc: Christian Borntraeger 
Cc: Peter Maydell 
---
 .gitlab-ci.d/custom-runners.yml   |  2 +-
 ...18.04-s390x.yml => ubuntu-20.04-s390x.yml} | 28 +--
 2 files changed, 15 insertions(+), 15 deletions(-)
 rename .gitlab-ci.d/custom-runners/{ubuntu-18.04-s390x.yml => 
ubuntu-20.04-s390x.yml} (87%)

diff --git a/.gitlab-ci.d/custom-runners.yml b/.gitlab-ci.d/custom-runners.yml
index 056c374619..3e76a2034a 100644
--- a/.gitlab-ci.d/custom-runners.yml
+++ b/.gitlab-ci.d/custom-runners.yml
@@ -14,6 +14,6 @@ variables:
   GIT_STRATEGY: clone
 
 include:
-  - local: '/.gitlab-ci.d/custom-runners/ubuntu-18.04-s390x.yml'
+  - local: '/.gitlab-ci.d/custom-runners/ubuntu-20.04-s390x.yml'
   - local: '/.gitlab-ci.d/custom-runners/ubuntu-20.04-aarch64.yml'
   - local: '/.gitlab-ci.d/custom-runners/centos-stream-8-x86_64.yml'
diff --git a/.gitlab-ci.d/custom-runners/ubuntu-18.04-s390x.yml 
b/.gitlab-ci.d/custom-runners/ubuntu-20.04-s390x.yml
similarity index 87%
rename from .gitlab-ci.d/custom-runners/ubuntu-18.04-s390x.yml
rename to .gitlab-ci.d/custom-runners/ubuntu-20.04-s390x.yml
index f39d874a1e..0333872113 100644
--- a/.gitlab-ci.d/custom-runners/ubuntu-18.04-s390x.yml
+++ b/.gitlab-ci.d/custom-runners/ubuntu-20.04-s390x.yml
@@ -1,12 +1,12 @@
-# All ubuntu-18.04 jobs should run successfully in an environment
+# All ubuntu-20.04 jobs should run successfully in an environment
 # setup by the scripts/ci/setup/build-environment.yml task
-# "Install basic packages to build QEMU on Ubuntu 18.04/20.04"
+# "Install basic packages to build QEMU on Ubuntu 20.04/20.04"
 
-ubuntu-18.04-s390x-all-linux-static:
+ubuntu-20.04-s390x-all-linux-static:
  needs: []
  stage: build
  tags:
- - ubuntu_18.04
+ - ubuntu_20.04
  - s390x
  rules:
  - if: '$CI_PROJECT_NAMESPACE == "qemu-project" && $CI_COMMIT_BRANCH =~ 
/^staging/'
@@ -21,11 +21,11 @@ ubuntu-18.04-s390x-all-linux-static:
  - make --output-sync -j`nproc` check V=1
  - make --output-sync -j`nproc` check-tcg V=1
 
-ubuntu-18.04-s390x-all:
+ubuntu-20.04-s390x-all:
  needs: []
  stage: build
  tags:
- - ubuntu_18.04
+ - ubuntu_20.04
  - s390x
  rules:
  - if: '$CI_PROJECT_NAMESPACE == "qemu-project" && $CI_COMMIT_BRANCH =~ 
/^staging/'
@@ -37,11 +37,11 @@ ubuntu-18.04-s390x-all:
  - make --output-sync -j`nproc`
  - make --output-sync -j`nproc` check V=1
 
-ubuntu-18.04-s390x-alldbg:
+ubuntu-20.04-s390x-alldbg:
  needs: []
  stage: build
  tags:
- - ubuntu_18.04
+ - ubuntu_20.04
  - s390x
  rules:
  - if: '$CI_PROJECT_NAMESPACE == "qemu-project" && $CI_COMMIT_BRANCH =~ 
/^staging/'
@@ -58,11 +58,11 @@ ubuntu-18.04-s390x-alldbg:
  - make --output-sync -j`nproc`
  - make --output-sync -j`nproc` check V=1
 
-ubuntu-18.04-s390x-clang:
+ubuntu-20.04-s390x-clang:
  needs: []
  stage: build
  tags:
- - ubuntu_18.04
+ - ubuntu_20.04
  - s390x
  rules:
  - if: '$CI_PROJECT_NAMESPACE == "qemu-project" && $CI_COMMIT_BRANCH =~ 
/^staging/'
@@ -78,11 +78,11 @@ ubuntu-18.04-s390x-clang:
  - make --output-sync -j`nproc`
  - make --output-sync -j`nproc` check V=1
 
-ubuntu-18.04-s390x-tci:
+ubuntu-20.04-s390x-tci:
  needs: []
  stage: build
  tags:
- - ubuntu_18.04
+ - ubuntu_20.04
  - s390x
  rules:
  - if: '$CI_PROJECT_NAMESPACE == "qemu-project" && $CI_COMMIT_BRANCH =~ 
/^staging/'
@@ -97,11 +97,11 @@ ubuntu-18.04-s390x-tci:
  - ../configure --disable-libssh --enable-tcg-interpreter
  - make --output-sync -j`nproc`
 
-ubuntu-18.04-s390x-notcg:
+ubuntu-20.04-s390x-notcg:
  needs: []
  stage: build
  tags:
- - ubuntu_18.04
+ - ubuntu_20.04
  - s390x
  rules:
  - if: '$CI_PROJECT_NAMESPACE == "qemu-project" && $CI_COMMIT_BRANCH =~ 
/^staging/'
-- 
2.30.2

[PATCH v2 17/18] iotests: make qemu_img_log() check log level

[John Snow]

Improve qemu_img_log() to actually check if logging is turned on. If it
isn't, revert to the behavior of qemu_img(). This is done so that there
really is no way to avoid scrutinizing qemu-ing subprocess calls by
accident. You're gonna have to work for it.

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6da6890596c..0519b2a8019 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -348,7 +348,13 @@ def qemu_img_map(*args: str) -> Any:
 return qemu_img_json('map', "--output", "json", *args)
 
 def qemu_img_log(*args: str) -> subprocess.CompletedProcess[str]:
-result = qemu_img(*args, check=False)
+"""
+Logged, unchecked variant of qemu_img() that allows non-zero exit codes.
+
+If logging is perceived to be disabled, this function will behave
+like qemu_img() and prohibit non-zero return codes.
+"""
+result = qemu_img(*args, check=not logging_enabled())
 log(result.stdout, filters=[filter_testfiles])
 return result
 
@@ -1635,6 +1641,11 @@ def activate_logging():
 test_logger.setLevel(logging.INFO)
 test_logger.propagate = False
 
+def logging_enabled() -> bool:
+"""Return True if iotest logging is active."""
+return (test_logger.hasHandlers()
+and test_logger.getEffectiveLevel() >= logging.INFO)
+
 # This is called from script-style iotests without a single point of entry
 def script_initialize(*args, **kwargs):
 """Initialize script-style tests without running any tests."""
-- 
2.34.1

Re: [PATCH 0/2] Resolve some redundant property accessors

[Bernhard Beschow]

Am 21. Februar 2022 22:28:00 UTC schrieb "Philippe Mathieu-Daudé" 
:
>On 17/2/22 23:53, Bernhard Beschow wrote:
>> The QOM API already provides appropriate accessors, so reuse them.
>> 
>> Testing done:
>> 
>>:$ make check
>>Ok: 569
>>Expected Fail:  0
>>Fail:   0
>>Unexpected Pass:0
>>Skipped:178
>>Timeout:0
>> 
>> Bernhard Beschow (2):
>>hw/vfio/pci-quirks: Resolve redundant property getters
>>hw/riscv/sifive_u: Resolve redundant property accessors
>
>Good cleanup.
>
>You might want to play with Coccinelle spatch [*] to clean all uses:

Hi Philippe,

thanks for your review!

I've manually inspected most (all?) of them and found that the remaining 
setters were non-trivial, i.e. they do some extra stuff such as checking 
boundaries or invoking some extra actions. Do you have an idea how to deal with 
that?

Regards,
Bernhard

>$ git grep object_property_add\(.*uint
>hw/acpi/ich9.c:446:object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, 
>"uint32",
>hw/i386/sgx-epc.c:47:object_property_add(obj, SGX_EPC_SIZE_PROP, 
>"uint64", sgx_epc_get_size,
>hw/intc/apic_common.c:462:object_property_add(obj, "id", "uint32",
>hw/mem/pc-dimm.c:175:object_property_add(obj, PC_DIMM_SIZE_PROP, 
>"uint64", pc_dimm_get_size,
>hw/misc/aspeed_lpc.c:420:object_property_add(obj, "idr1", "uint32", 
>aspeed_kcs_get_register_property,
>hw/misc/aspeed_lpc.c:422:object_property_add(obj, "odr1", "uint32", 
>aspeed_kcs_get_register_property,
>hw/misc/aspeed_lpc.c:424:object_property_add(obj, "str1", "uint32", 
>aspeed_kcs_get_register_property,
>hw/misc/aspeed_lpc.c:426:object_property_add(obj, "idr2", "uint32", 
>aspeed_kcs_get_register_property,
>hw/misc/aspeed_lpc.c:428:object_property_add(obj, "odr2", "uint32", 
>aspeed_kcs_get_register_property,
>hw/misc/aspeed_lpc.c:430:object_property_add(obj, "str2", "uint32", 
>aspeed_kcs_get_register_property,
>hw/misc/aspeed_lpc.c:432:object_property_add(obj, "idr3", "uint32", 
>aspeed_kcs_get_register_property,
>hw/misc/aspeed_lpc.c:434:object_property_add(obj, "odr3", "uint32", 
>aspeed_kcs_get_register_property,
>hw/misc/aspeed_lpc.c:436:object_property_add(obj, "str3", "uint32", 
>aspeed_kcs_get_register_property,
>hw/misc/aspeed_lpc.c:438:object_property_add(obj, "idr4", "uint32", 
>aspeed_kcs_get_register_property,
>hw/misc/aspeed_lpc.c:440:object_property_add(obj, "odr4", "uint32", 
>aspeed_kcs_get_register_property,
>hw/misc/aspeed_lpc.c:442:object_property_add(obj, "str4", "uint32", 
>aspeed_kcs_get_register_property,
>hw/misc/npcm7xx_mft.c:493:object_property_add(obj, "max_rpm[*]", 
>"uint32",
>hw/nvme/ctrl.c:6856:object_property_add(obj, 
>"smart_critical_warning", "uint8",
>hw/pci-host/q35.c:224:object_property_add(obj, 
>PCI_HOST_PROP_PCI_HOLE_START, "uint32",
>hw/pci-host/q35.c:228:object_property_add(obj, 
>PCI_HOST_PROP_PCI_HOLE_END, "uint32",
>hw/pci-host/q35.c:232:object_property_add(obj, 
>PCI_HOST_PROP_PCI_HOLE64_START, "uint64",
>hw/pci-host/q35.c:236:object_property_add(obj, 
>PCI_HOST_PROP_PCI_HOLE64_END, "uint64",
>hw/ppc/spapr_drc.c:584:object_property_add(obj, "index", "uint32", 
>prop_get_index,
>hw/riscv/sifive_u.c:736:object_property_add(obj, "msel", "uint32",
>hw/riscv/sifive_u.c:743:object_property_add(obj, "serial", "uint32",
>hw/sensor/adm1272.c:497:object_property_add(obj, "vin", "uint16",
>hw/sensor/adm1272.c:501:object_property_add(obj, "vout", "uint16",
>hw/sensor/adm1272.c:505:object_property_add(obj, "iout", "uint16",
>hw/sensor/adm1272.c:509:object_property_add(obj, "pin", "uint16",
>hw/sensor/max34451.c:730:object_property_add(obj, "vout[*]", 
>"uint16",
>hw/sensor/max34451.c:740:object_property_add(obj, 
>"temperature[*]", "uint16",
>hw/vfio/pci-quirks.c:1621:object_property_add(OBJECT(vdev), 
>"nvlink2-tgt", "uint64",
>hw/vfio/pci-quirks.c:1682:object_property_add(OBJECT(vdev), 
>"nvlink2-tgt", "uint64",
>hw/vfio/pci-quirks.c:1688:object_property_add(OBJECT(vdev), 
>"nvlink2-link-speed", "uint32",
>net/colo-compare.c:1390:object_property_add(obj, "compare_timeout", 
>"uint64",
>net/colo-compare.c:1394:object_property_add(obj, 
>"expired_scan_cycle", "uint32",
>net/colo-compare.c:1398:object_property_add(obj, "max_queue_size", 
>"uint32",
>softmmu/memory.c:1262:object_property_add(OBJECT(mr), "priority", 
>"uint32",
>softmmu/memory.c:1266:object_property_add(OBJECT(mr), "size", "uint64",
>target/arm/cpu64.c:863:object_property_add(obj, "sve-max-vq", 
>"uint32", cpu_max_get_sve_max_vq,
>tests/unit/test-qdev-global-props.c:155:object_property_add(obj, 
>"prop1", "uint32", prop1_accessor, prop1_accessor,
>tests/unit/test-qdev-global-props.c:157:object_property_add(obj, 
>"prop2", "uint32", prop2_accessor, prop2_accessor,
>
>[*] https://coccinelle.gitlabpages.inria.fr/website/

[PATCH v2 18/18] iotests: reimplement img_info_log in terms of qemu_img_log

[John Snow]

Now every last call to qemu_img is certifiably either checked or logged.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 0519b2a8019..473173324d6 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -347,15 +347,20 @@ def qemu_img_info(*args: str) -> Any:
 def qemu_img_map(*args: str) -> Any:
 return qemu_img_json('map', "--output", "json", *args)
 
-def qemu_img_log(*args: str) -> subprocess.CompletedProcess[str]:
+def qemu_img_log(
+*args: str,
+filters: Iterable[Callable[[str], str]] = (),
+) -> subprocess.CompletedProcess[str]:
 """
 Logged, unchecked variant of qemu_img() that allows non-zero exit codes.
 
 If logging is perceived to be disabled, this function will behave
 like qemu_img() and prohibit non-zero return codes.
+
+By default, output will be filtered through filter_testfiles().
 """
 result = qemu_img(*args, check=not logging_enabled())
-log(result.stdout, filters=[filter_testfiles])
+log(result.stdout, filters=filters or [filter_testfiles])
 return result
 
 def img_info_log(filename: str, filter_path: Optional[str] = None,
@@ -369,10 +374,11 @@ def img_info_log(filename: str, filter_path: 
Optional[str] = None,
 args += extra_args
 args.append(filename)
 
-output = qemu_img(*args, check=False).stdout
 if not filter_path:
 filter_path = filename
-log(filter_img_info(output, filter_path))
+qemu_img_log(
+*args,
+filters=[lambda output: filter_img_info(output, filter_path)])
 
 def qemu_io_wrap_args(args: Sequence[str]) -> List[str]:
 if '-f' in args or '--image-opts' in args:
-- 
2.34.1

Re: [PATCH v5 1/2] semihosting/arm-compat: replace heuristic for softmmu SYS_HEAPINFO

[Alex Bennée]

Peter Maydell  writes:

> On Mon, 21 Feb 2022 at 17:06, Alex Bennée  wrote:
>>
>>
>> Peter Maydell  writes:
>>
>> > On Thu, 10 Feb 2022 at 11:30, Alex Bennée  wrote:
>> >>
>> >> The previous numbers were a guess at best and rather arbitrary without
>> >> taking into account anything that might be loaded. Instead of using
>> >> guesses based on the state of registers implement a new function that:
>> >>
>> >>  a) scans the MemoryRegions for the largest RAM block
>> >>  b) iterates through all "ROM" blobs looking for the biggest gap
>> >>
>> >> The "ROM" blobs include all code loaded via -kernel and the various
>> >> -device loader techniques.
>> >>
>> >> Signed-off-by: Alex Bennée 
>> >> Cc: Andrew Strauss 
>> >> Cc: Keith Packard 
>> >> Message-Id: <20210601090715.22330-1-alex.ben...@linaro.org>
>> >>
>> >
>> >
>> >> +/*
>> >> + * Sort into address order. We break ties between rom-startpoints
>> >> + * and rom-endpoints in favour of the startpoint, by sorting the 0->1
>> >> + * transition before the 1->0 transition. Either way round would
>> >> + * work, but this way saves a little work later by avoiding
>> >> + * dealing with "gaps" of 0 length.
>> >> + */
>> >> +static gint sort_secs(gconstpointer a, gconstpointer b)
>> >> +{
>> >> +RomSec *ra = (RomSec *) a;
>> >> +RomSec *rb = (RomSec *) b;
>> >> +
>> >> +if (ra->base == rb->base) {
>> >> +return ra->se - rb->se;
>> >> +}
>> >> +return ra->base > rb->base ? 1 : -1;
>> >> +}
>> >
>> > This sort comparator still doesn't report the equality
>> > case as actually equal.
>>
>> When ra->se and rb->se are the same it returns 0. Is that not what you want?
>
> Oops, yes it does. I misread it because I was expecting it to be
> structured differently. (AFAIK there is no "standard" way to
> structure a comparator-function that works with multiple fields,
> so the way you have it is fine.)

The other options were all ugly ;-)

-- 
Alex Bennée

[PATCH v2 15/18] iotests: remove qemu_img_log('create', ...) calls

[John Snow]

qemu_img_log() calls into qemu_img_pipe(), which always removes output
for 'create' commands on success anyway. Replace all of these calls to
the simpler qemu_img_create(...) which doesn't log, but raises a
detailed exception object on failure instead.

Blank lines are removed from output files where appropriate.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/255 |  8 
 tests/qemu-iotests/255.out |  4 
 tests/qemu-iotests/274 | 17 -
 tests/qemu-iotests/274.out | 29 -
 tests/qemu-iotests/280 |  2 +-
 tests/qemu-iotests/280.out |  1 -
 6 files changed, 13 insertions(+), 48 deletions(-)

diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255
index 3d6d0e80cb5..f86fa851b62 100755
--- a/tests/qemu-iotests/255
+++ b/tests/qemu-iotests/255
@@ -42,8 +42,8 @@ with iotests.FilePath('t.qcow2') as disk_path, \
 size_str = str(size)
 
 iotests.create_image(base_path, size)
-iotests.qemu_img_log('create', '-f', iotests.imgfmt, mid_path, size_str)
-iotests.qemu_img_log('create', '-f', iotests.imgfmt, disk_path, size_str)
+iotests.qemu_img_create('-f', iotests.imgfmt, mid_path, size_str)
+iotests.qemu_img_create('-f', iotests.imgfmt, disk_path, size_str)
 
 # Create a backing chain like this:
 # base <- [throttled: bps-read=4096] <- mid <- overlay
@@ -92,8 +92,8 @@ with iotests.FilePath('src.qcow2') as src_path, \
 size = 128 * 1024 * 1024
 size_str = str(size)
 
-iotests.qemu_img_log('create', '-f', iotests.imgfmt, src_path, size_str)
-iotests.qemu_img_log('create', '-f', iotests.imgfmt, dst_path, size_str)
+iotests.qemu_img_create('-f', iotests.imgfmt, src_path, size_str)
+iotests.qemu_img_create('-f', iotests.imgfmt, dst_path, size_str)
 
 iotests.log(iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write 0 1M',
 src_path),
diff --git a/tests/qemu-iotests/255.out b/tests/qemu-iotests/255.out
index 11a05a5213e..2e837cbb5f3 100644
--- a/tests/qemu-iotests/255.out
+++ b/tests/qemu-iotests/255.out
@@ -3,8 +3,6 @@ Finishing a commit job with background reads
 
 === Create backing chain and start VM ===
 
-
-
 === Start background read requests ===
 
 === Run a commit job ===
@@ -21,8 +19,6 @@ Closing the VM while a job is being cancelled
 
 === Create images and start VM ===
 
-
-
 wrote 1048576/1048576 bytes at offset 0
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
diff --git a/tests/qemu-iotests/274 b/tests/qemu-iotests/274
index 080a90f10f7..2495e051a22 100755
--- a/tests/qemu-iotests/274
+++ b/tests/qemu-iotests/274
@@ -31,12 +31,11 @@ size_long = 2 * 1024 * 1024
 size_diff = size_long - size_short
 
 def create_chain() -> None:
-iotests.qemu_img_log('create', '-f', iotests.imgfmt, base,
- str(size_long))
-iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', base,
- '-F', iotests.imgfmt, mid, str(size_short))
-iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', mid,
- '-F', iotests.imgfmt, top, str(size_long))
+iotests.qemu_img_create('-f', iotests.imgfmt, base, str(size_long))
+iotests.qemu_img_create('-f', iotests.imgfmt, '-b', base,
+'-F', iotests.imgfmt, mid, str(size_short))
+iotests.qemu_img_create('-f', iotests.imgfmt, '-b', mid,
+'-F', iotests.imgfmt, top, str(size_long))
 
 iotests.qemu_io_log('-c', 'write -P 1 0 %d' % size_long, base)
 
@@ -160,9 +159,9 @@ with iotests.FilePath('base') as base, \
 ('off',  '512k', '256k', '500k', '436k')]:
 
 iotests.log('=== preallocation=%s ===' % prealloc)
-iotests.qemu_img_log('create', '-f', iotests.imgfmt, base, base_size)
-iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', base,
- '-F', iotests.imgfmt, top, top_size_old)
+iotests.qemu_img_create('-f', iotests.imgfmt, base, base_size)
+iotests.qemu_img_create('-f', iotests.imgfmt, '-b', base,
+'-F', iotests.imgfmt, top, top_size_old)
 iotests.qemu_io_log('-c', 'write -P 1 %s 64k' % off, base)
 
 # After this, top_size_old to base_size should be allocated/zeroed.
diff --git a/tests/qemu-iotests/274.out b/tests/qemu-iotests/274.out
index 1ce40d839a4..acd8b166a65 100644
--- a/tests/qemu-iotests/274.out
+++ b/tests/qemu-iotests/274.out
@@ -1,7 +1,4 @@
 == Commit tests ==
-
-
-
 wrote 2097152/2097152 bytes at offset 0
 2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
@@ -63,9 +60,6 @@ read 1048576/1048576 bytes at offset 1048576
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 === Testing HMP commit (top -> mid) ===
-
-
-
 wrote 2097152/2097152 bytes at offset 0
 2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
@@ -92,9 +86,6 @@ read 1048576/1048576 bytes at offset 1048576
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX 

[PATCH v2 14/18] iotests: move has_working_luks onto qemu_img()

[John Snow]

Admittedly a mostly lateral move; the diagnostics are slightly better on
program crash.

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 882d3e0214a..16bac5df969 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1445,20 +1445,20 @@ def has_working_luks() -> Tuple[bool, str]:
 """
 
 img_file = f'{test_dir}/luks-test.luks'
-(output, status) = \
-qemu_img_pipe_and_status('create', '-f', 'luks',
- '--object', luks_default_secret_object,
- '-o', luks_default_key_secret_opt,
- '-o', 'iter-time=10',
- img_file, '1G')
+res = qemu_img('create', '-f', 'luks',
+   '--object', luks_default_secret_object,
+   '-o', luks_default_key_secret_opt,
+   '-o', 'iter-time=10',
+   img_file, '1G',
+   check=False)
 try:
 os.remove(img_file)
 except OSError:
 pass
 
-if status != 0:
-reason = output
-for line in output.splitlines():
+if res.returncode:
+reason = res.stdout
+for line in res.stdout.splitlines():
 if img_file + ':' in line:
 reason = line.split(img_file + ':', 1)[1].strip()
 break
-- 
2.34.1

[PATCH v2 09/18] iotests: remove-bitmap-from-backing: use qemu_img_info()

[John Snow]

Remove two more usages of qemu_img_pipe().

Signed-off-by: John Snow 
---
 tests/qemu-iotests/tests/remove-bitmap-from-backing | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/tests/remove-bitmap-from-backing 
b/tests/qemu-iotests/tests/remove-bitmap-from-backing
index fee31413400..15be32dcb96 100755
--- a/tests/qemu-iotests/tests/remove-bitmap-from-backing
+++ b/tests/qemu-iotests/tests/remove-bitmap-from-backing
@@ -19,7 +19,7 @@
 #
 
 import iotests
-from iotests import log, qemu_img_create, qemu_img, qemu_img_pipe
+from iotests import log, qemu_img_create, qemu_img, qemu_img_info
 
 iotests.script_initialize(supported_fmts=['qcow2'],
   unsupported_imgopts=['compat'])
@@ -33,7 +33,7 @@ qemu_img_create('-f', iotests.imgfmt, '-b', base,
 
 qemu_img('bitmap', '--add', base, 'bitmap0')
 # Just assert that our method of checking bitmaps in the image works.
-assert 'bitmaps' in qemu_img_pipe('info', base)
+assert 'bitmaps' in qemu_img_info(base)['format-specific']['data']
 
 vm = iotests.VM().add_drive(top, 'backing.node-name=base')
 vm.launch()
@@ -68,5 +68,5 @@ if result != {'return': {}}:
 
 vm.shutdown()
 
-if 'bitmaps' in qemu_img_pipe('info', base):
+if 'bitmaps' in qemu_img_info(base)['format-specific']['data']:
 log('ERROR: Bitmap is still in the base image')
-- 
2.34.1

[PATCH v2 16/18] iotests: remove qemu_img_pipe()

[John Snow]

With the exceptional 'create' calls removed in the prior commit, change
qemu_img_log() and img_info_log() to call qemu_img() directly
instead.

In keeping with the spirit of diff-based tests, allow these calls to
qemu_img() to return an unchecked non-zero status code -- because any
error we'd see from the output is going into the log anyway.

Every last call to qemu-img is now either checked for a return code of
zero or has its output logged. It should be very hard to accidentally
ignore the return code *or* output from qemu-img now; intentional malice
remains unhandled.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 26 +++---
 1 file changed, 7 insertions(+), 19 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 16bac5df969..6da6890596c 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -240,15 +240,6 @@ def qemu_img_create_prepare_args(args: List[str]) -> 
List[str]:
 
 return result
 
-def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]:
-"""
-Run qemu-img and return both its output and its exit code
-"""
-is_create = bool(args and args[0] == 'create')
-full_args = qemu_img_args + qemu_img_create_prepare_args(list(args))
-return qemu_tool_pipe_and_status('qemu-img', full_args,
- drop_successful_output=is_create)
-
 def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True
  ) -> subprocess.CompletedProcess[str]:
 """
@@ -356,17 +347,14 @@ def qemu_img_info(*args: str) -> Any:
 def qemu_img_map(*args: str) -> Any:
 return qemu_img_json('map', "--output", "json", *args)
 
-def qemu_img_pipe(*args: str) -> str:
-'''Run qemu-img and return its output'''
-return qemu_img_pipe_and_status(*args)[0]
-
-def qemu_img_log(*args):
-result = qemu_img_pipe(*args)
-log(result, filters=[filter_testfiles])
+def qemu_img_log(*args: str) -> subprocess.CompletedProcess[str]:
+result = qemu_img(*args, check=False)
+log(result.stdout, filters=[filter_testfiles])
 return result
 
-def img_info_log(filename, filter_path=None, use_image_opts=False,
- extra_args=()):
+def img_info_log(filename: str, filter_path: Optional[str] = None,
+ use_image_opts: bool = False, extra_args: Sequence[str] = (),
+ ) -> None:
 args = ['info']
 if use_image_opts:
 args.append('--image-opts')
@@ -375,7 +363,7 @@ def img_info_log(filename, filter_path=None, 
use_image_opts=False,
 args += extra_args
 args.append(filename)
 
-output = qemu_img_pipe(*args)
+output = qemu_img(*args, check=False).stdout
 if not filter_path:
 filter_path = filename
 log(filter_img_info(output, filter_path))
-- 
2.34.1

[PATCH v2 12/18] iotests: replace unchecked calls to qemu_img_pipe()

[John Snow]

qemu_img_pipe() discards the return code in favor of returning just the
output. Some tests using this function don't save, log, or check the
output either, though. Replace those calls to a checked version as
appropriate.

Tests affected are 194, 202, 203, 234, 262, and 303.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/194 | 4 ++--
 tests/qemu-iotests/202 | 4 ++--
 tests/qemu-iotests/203 | 4 ++--
 tests/qemu-iotests/234 | 4 ++--
 tests/qemu-iotests/262 | 2 +-
 tests/qemu-iotests/303 | 2 +-
 6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index e44b8df7280..68894371f51 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -33,8 +33,8 @@ with iotests.FilePath('source.img') as source_img_path, \
  iotests.VM('dest') as dest_vm:
 
 img_size = '1G'
-iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, source_img_path, 
img_size)
-iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, dest_img_path, 
img_size)
+iotests.qemu_img_create('-f', iotests.imgfmt, source_img_path, img_size)
+iotests.qemu_img_create('-f', iotests.imgfmt, dest_img_path, img_size)
 
 iotests.log('Launching VMs...')
 (source_vm.add_drive(source_img_path)
diff --git a/tests/qemu-iotests/202 b/tests/qemu-iotests/202
index 8eb5f32d153..b784dcd791a 100755
--- a/tests/qemu-iotests/202
+++ b/tests/qemu-iotests/202
@@ -35,8 +35,8 @@ with iotests.FilePath('disk0.img') as disk0_img_path, \
  iotests.VM() as vm:
 
 img_size = '10M'
-iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk0_img_path, 
img_size)
-iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk1_img_path, 
img_size)
+iotests.qemu_img_create('-f', iotests.imgfmt, disk0_img_path, img_size)
+iotests.qemu_img_create('-f', iotests.imgfmt, disk1_img_path, img_size)
 
 iotests.log('Launching VM...')
 vm.launch()
diff --git a/tests/qemu-iotests/203 b/tests/qemu-iotests/203
index ea30e504976..ab80fd0e44a 100755
--- a/tests/qemu-iotests/203
+++ b/tests/qemu-iotests/203
@@ -33,8 +33,8 @@ with iotests.FilePath('disk0.img') as disk0_img_path, \
  iotests.VM() as vm:
 
 img_size = '10M'
-iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk0_img_path, 
img_size)
-iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk1_img_path, 
img_size)
+iotests.qemu_img_create('-f', iotests.imgfmt, disk0_img_path, img_size)
+iotests.qemu_img_create('-f', iotests.imgfmt, disk1_img_path, img_size)
 
 iotests.log('Launching VM...')
 (vm.add_object('iothread,id=iothread0')
diff --git a/tests/qemu-iotests/234 b/tests/qemu-iotests/234
index cb5f1753e08..a9f764bb2c6 100755
--- a/tests/qemu-iotests/234
+++ b/tests/qemu-iotests/234
@@ -34,8 +34,8 @@ with iotests.FilePath('img') as img_path, \
  iotests.VM(path_suffix='a') as vm_a, \
  iotests.VM(path_suffix='b') as vm_b:
 
-iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, backing_path, '64M')
-iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M')
+iotests.qemu_img_create('-f', iotests.imgfmt, backing_path, '64M')
+iotests.qemu_img_create('-f', iotests.imgfmt, img_path, '64M')
 
 os.mkfifo(fifo_a)
 os.mkfifo(fifo_b)
diff --git a/tests/qemu-iotests/262 b/tests/qemu-iotests/262
index 32d69988ef7..2294fd5ecbd 100755
--- a/tests/qemu-iotests/262
+++ b/tests/qemu-iotests/262
@@ -51,7 +51,7 @@ with iotests.FilePath('img') as img_path, \
 
 vm.add_device('virtio-blk,drive=%s,iothread=iothread0' % root)
 
-iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M')
+iotests.qemu_img_create('-f', iotests.imgfmt, img_path, '64M')
 
 os.mkfifo(fifo)
 
diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303
index 16c2e108276..93aa5ce9b7d 100755
--- a/tests/qemu-iotests/303
+++ b/tests/qemu-iotests/303
@@ -38,7 +38,7 @@ def create_bitmap(bitmap_number, disabled):
 if disabled:
 args.append('--disable')
 
-iotests.qemu_img_pipe(*args)
+iotests.qemu_img(*args)
 
 
 def write_to_disk(offset, size):
-- 
2.34.1

[PATCH v2 10/18] iotests: add qemu_img_map() function

[John Snow]

By analogy with qemu_img_{measure, check, info}. Replace calls to
qemu_img_pipe('map', '--output=json', ...) with the new function.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/041 |  5 ++---
 tests/qemu-iotests/211 |  6 +++---
 tests/qemu-iotests/iotests.py  |  3 +++
 tests/qemu-iotests/tests/block-status-cache| 11 ---
 tests/qemu-iotests/tests/parallels-read-bitmap |  6 ++
 5 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index db9f5dc540e..3e16acee567 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -24,7 +24,7 @@ import os
 import re
 import json
 import iotests
-from iotests import qemu_img, qemu_img_pipe, qemu_io
+from iotests import qemu_img, qemu_img_map, qemu_io
 
 backing_img = os.path.join(iotests.test_dir, 'backing.img')
 target_backing_img = os.path.join(iotests.test_dir, 'target-backing.img')
@@ -1360,8 +1360,7 @@ class TestFilters(iotests.QMPTestCase):
 
 self.vm.qmp('blockdev-del', node_name='target')
 
-target_map = qemu_img_pipe('map', '--output=json', target_img)
-target_map = json.loads(target_map)
+target_map = qemu_img_map(target_img)
 
 assert target_map[0]['start'] == 0
 assert target_map[0]['length'] == 512 * 1024
diff --git a/tests/qemu-iotests/211 b/tests/qemu-iotests/211
index f52cadade12..1a3b4596c80 100755
--- a/tests/qemu-iotests/211
+++ b/tests/qemu-iotests/211
@@ -59,7 +59,7 @@ with iotests.FilePath('t.vdi') as disk_path, \
 vm.shutdown()
 
 iotests.img_info_log(disk_path)
-iotests.log(iotests.qemu_img_pipe('map', '--output=json', disk_path))
+iotests.log(iotests.qemu_img_map(disk_path))
 
 #
 # Successful image creation (explicit defaults)
@@ -83,7 +83,7 @@ with iotests.FilePath('t.vdi') as disk_path, \
 vm.shutdown()
 
 iotests.img_info_log(disk_path)
-iotests.log(iotests.qemu_img_pipe('map', '--output=json', disk_path))
+iotests.log(iotests.qemu_img_map(disk_path))
 
 #
 # Successful image creation (with non-default options)
@@ -107,7 +107,7 @@ with iotests.FilePath('t.vdi') as disk_path, \
 vm.shutdown()
 
 iotests.img_info_log(disk_path)
-iotests.log(iotests.qemu_img_pipe('map', '--output=json', disk_path))
+iotests.log(iotests.qemu_img_map(disk_path))
 
 #
 # Invalid BlockdevRef
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index df143ecec73..cbb3af71523 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -353,6 +353,9 @@ def qemu_img_check(*args: str) -> Any:
 def qemu_img_info(*args: str) -> Any:
 return qemu_img_json('info', "--output", "json", *args)
 
+def qemu_img_map(*args: str) -> Any:
+return qemu_img_json('map', "--output", "json", *args)
+
 def qemu_img_pipe(*args: str) -> str:
 '''Run qemu-img and return its output'''
 return qemu_img_pipe_and_status(*args)[0]
diff --git a/tests/qemu-iotests/tests/block-status-cache 
b/tests/qemu-iotests/tests/block-status-cache
index 40e648e251a..5a7bc2c1493 100755
--- a/tests/qemu-iotests/tests/block-status-cache
+++ b/tests/qemu-iotests/tests/block-status-cache
@@ -22,7 +22,7 @@
 import os
 import signal
 import iotests
-from iotests import qemu_img_create, qemu_img_pipe, qemu_nbd
+from iotests import qemu_img_create, qemu_img_map, qemu_nbd
 
 
 image_size = 1 * 1024 * 1024
@@ -76,8 +76,7 @@ class TestBscWithNbd(iotests.QMPTestCase):
 # to allocate the first sector to facilitate alignment probing), and
 # then the rest to be zero.  The BSC will thus contain (if anything)
 # one range covering the first sector.
-map_pre = qemu_img_pipe('map', '--output=json', '--image-opts',
-nbd_img_opts)
+map_pre = qemu_img_map('--image-opts', nbd_img_opts)
 
 # qemu:allocation-depth maps for want_zero=false.
 # want_zero=false should (with the file driver, which the server is
@@ -111,14 +110,12 @@ class TestBscWithNbd(iotests.QMPTestCase):
 # never loop too many times here.
 for _ in range(2):
 # (Ignore the result, this is just to contaminate the cache)
-qemu_img_pipe('map', '--output=json', '--image-opts',
-  nbd_img_opts_alloc_depth)
+qemu_img_map('--image-opts', nbd_img_opts_alloc_depth)
 
 # Now let's see whether the cache reports everything as data, or
 # whether we get correct information (i.e. the same as we got on our
 # first attempt).
-map_post = qemu_img_pipe('map', '--output=json', '--image-opts',
- nbd_img_opts)
+map_post = qemu_img_map('--image-opts', nbd_img_opts)
 
 if map_pre != map_post:
 print('ERROR: Map information differs before and after querying ' +
diff --git 

[PATCH v2 11/18] iotests: change supports_quorum to use qemu_img

[John Snow]

Similar to other recent changes: use the qemu_img invocation that
supports throwing loud, nasty exceptions when it fails for surprising
reasons.

(Why would "--help" ever fail? I don't know, but consistency is like a
well-manicured bonsai.)

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index cbb3af71523..882d3e0214a 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1428,8 +1428,8 @@ def _verify_imgopts(unsupported: Sequence[str] = ()) -> 
None:
 notrun(f'not suitable for this imgopts: {imgopts}')
 
 
-def supports_quorum():
-return 'quorum' in qemu_img_pipe('--help')
+def supports_quorum() -> bool:
+return 'quorum' in qemu_img('--help').stdout
 
 def verify_quorum():
 '''Skip test suite if quorum support is not available'''
-- 
2.34.1

[PATCH v2 08/18] iotests: add qemu_img_info()

[John Snow]

Add qemu_img_info() by analogy with qemu_img_{measure,check}.
Modify image_size() to use this function instead.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/065|  5 ++---
 tests/qemu-iotests/242|  5 ++---
 tests/qemu-iotests/iotests.py | 12 
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
index f7c1b68dadb..9466ce7df4d 100755
--- a/tests/qemu-iotests/065
+++ b/tests/qemu-iotests/065
@@ -24,7 +24,7 @@ import os
 import re
 import json
 import iotests
-from iotests import qemu_img, qemu_img_pipe
+from iotests import qemu_img, qemu_img_info, qemu_img_pipe
 import unittest
 
 test_img = os.path.join(iotests.test_dir, 'test.img')
@@ -49,8 +49,7 @@ class TestQemuImgInfo(TestImageInfoSpecific):
 human_compare = None
 
 def test_json(self):
-data = json.loads(qemu_img_pipe('info', '--output=json', test_img))
-data = data['format-specific']
+data = qemu_img_info(test_img)['format-specific']
 self.assertEqual(data['type'], iotests.imgfmt)
 self.assertEqual(data['data'], self.json_compare)
 
diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242
index 96a30152b04..547bf382e39 100755
--- a/tests/qemu-iotests/242
+++ b/tests/qemu-iotests/242
@@ -22,7 +22,7 @@
 import iotests
 import json
 import struct
-from iotests import qemu_img_create, qemu_io, qemu_img_pipe, \
+from iotests import qemu_img_create, qemu_io, qemu_img_info, \
 file_path, img_info_log, log, filter_qemu_io
 
 iotests.script_initialize(supported_fmts=['qcow2'],
@@ -39,8 +39,7 @@ flag_offset = 0x5000f
 def print_bitmap(extra_args):
 log('qemu-img info dump:\n')
 img_info_log(disk, extra_args=extra_args)
-result = json.loads(qemu_img_pipe('info', '--force-share',
-  '--output=json', disk))
+result = qemu_img_info('--force-share', disk)
 if 'bitmaps' in result['format-specific']['data']:
 bitmaps = result['format-specific']['data']['bitmaps']
 log('The same bitmaps in JSON format:')
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index fc0d054e129..df143ecec73 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -350,6 +350,9 @@ def qemu_img_measure(*args: str) -> Any:
 def qemu_img_check(*args: str) -> Any:
 return qemu_img_json("check", "--output", "json", *args)
 
+def qemu_img_info(*args: str) -> Any:
+return qemu_img_json('info', "--output", "json", *args)
+
 def qemu_img_pipe(*args: str) -> str:
 '''Run qemu-img and return its output'''
 return qemu_img_pipe_and_status(*args)[0]
@@ -568,10 +571,11 @@ def create_image(name, size):
 file.write(sector)
 i = i + 512
 
-def image_size(img):
-'''Return image's virtual size'''
-r = qemu_img_pipe('info', '--output=json', '-f', imgfmt, img)
-return json.loads(r)['virtual-size']
+def image_size(img: str) -> int:
+"""Return image's virtual size"""
+value = qemu_img_info('-f', imgfmt, img)['virtual-size']
+assert isinstance(value, int)
+return value
 
 def is_str(val):
 return isinstance(val, str)
-- 
2.34.1

[PATCH v2 04/18] iotests: make qemu_img raise on non-zero rc by default

[John Snow]

re-write qemu_img() as a function that will by default raise a
VerboseProcessException (extended from CalledProcessException) on
non-zero return codes. This will produce a stack trace that will show
the command line arguments and return code from the failed process run.

Users that want something more flexible (there appears to be only one)
can use check=False and manage the return themselves. However, when the
return code is negative, the Exception will be raised no matter what.
This is done under the belief that there's no legitimate reason, even in
negative tests, to see a crash from a tool.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/257|  8 --
 tests/qemu-iotests/iotests.py | 53 +++
 2 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
index fb5359c581e..e7e7a2317e3 100755
--- a/tests/qemu-iotests/257
+++ b/tests/qemu-iotests/257
@@ -241,11 +241,13 @@ def compare_images(image, reference, baseimg=None, 
expected_match=True):
 expected_ret = 0 if expected_match else 1
 if baseimg:
 qemu_img("rebase", "-u", "-b", baseimg, '-F', iotests.imgfmt, image)
-ret = qemu_img("compare", image, reference)
+
+sub = qemu_img("compare", image, reference, check=False)
+
 log('qemu_img compare "{:s}" "{:s}" ==> {:s}, {:s}'.format(
 image, reference,
-"Identical" if ret == 0 else "Mismatch",
-"OK!" if ret == expected_ret else "ERROR!"),
+"Identical" if sub.returncode == 0 else "Mismatch",
+"OK!" if sub.returncode == expected_ret else "ERROR!"),
 filters=[iotests.filter_testfiles])
 
 def test_bitmap_sync(bsync_mode, msync_mode='bitmap', failure=None):
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 5617f991da7..546e5cb671b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -249,9 +249,49 @@ def qemu_img_pipe_and_status(*args: str) -> Tuple[str, 
int]:
 return qemu_tool_pipe_and_status('qemu-img', full_args,
  drop_successful_output=is_create)
 
-def qemu_img(*args: str) -> int:
-'''Run qemu-img and return the exit code'''
-return qemu_img_pipe_and_status(*args)[1]
+def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True
+ ) -> subprocess.CompletedProcess[str]:
+"""
+Run qemu_img and return the status code and console output.
+
+This function always prepends QEMU_IMG_OPTIONS and may further alter
+the args for 'create' commands.
+
+:param args: command-line arguments to qemu-img.
+:param check: Enforce a return code of zero.
+:param combine_stdio: set to False to keep stdout/stderr separated.
+
+:raise VerboseProcessError:
+When the return code is negative, or on any non-zero exit code
+when 'check=True' was provided (the default). This exception has
+'stdout', 'stderr', and 'returncode' properties that may be
+inspected to show greater detail. If this exception is not
+handled, the command-line, return code, and all console output
+will be included at the bottom of the stack trace.
+
+:return: a CompletedProcess. This object has args, returncode, and
+stdout properties. If streams are not combined, it will also
+have a stderr property.
+"""
+full_args = qemu_img_args + qemu_img_create_prepare_args(list(args))
+
+subp = subprocess.run(
+full_args,
+stdout=subprocess.PIPE,
+stderr=subprocess.STDOUT if combine_stdio else subprocess.PIPE,
+universal_newlines=True,
+check=False
+)
+
+if check and subp.returncode or (subp.returncode < 0):
+raise VerboseProcessError(
+subp.returncode, full_args,
+output=subp.stdout,
+stderr=subp.stderr,
+)
+
+return subp
+
 
 def ordered_qmp(qmsg, conv_keys=True):
 # Dictionaries are not ordered prior to 3.6, therefore:
@@ -266,7 +306,7 @@ def ordered_qmp(qmsg, conv_keys=True):
 return od
 return qmsg
 
-def qemu_img_create(*args):
+def qemu_img_create(*args: str) -> subprocess.CompletedProcess[str]:
 return qemu_img('create', *args)
 
 def qemu_img_measure(*args):
@@ -469,8 +509,9 @@ def qemu_nbd_popen(*args):
 
 def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
 '''Return True if two image files are identical'''
-return qemu_img('compare', '-f', fmt1,
-'-F', fmt2, img1, img2) == 0
+res = qemu_img('compare', '-f', fmt1,
+   '-F', fmt2, img1, img2, check=False)
+return res.returncode == 0
 
 def create_image(name, size):
 '''Create a fully-allocated raw image with sector markers'''
-- 
2.34.1

[PATCH v2 03/18] iotests: Remove explicit checks for qemu_img() == 0

[John Snow]

qemu_img() returning zero ought to be the rule, not the
exception. Remove all explicit checks against the condition in
preparation for making non-zero returns an Exception.

Signed-off-by: John Snow 
Reviewed-by: Eric Blake 
---
 tests/qemu-iotests/163 |  9 +++--
 tests/qemu-iotests/216 |  6 +++---
 tests/qemu-iotests/218 |  2 +-
 tests/qemu-iotests/224 | 11 +--
 tests/qemu-iotests/228 | 12 ++--
 tests/qemu-iotests/257 |  3 +--
 tests/qemu-iotests/258 |  4 ++--
 tests/qemu-iotests/310 | 14 +++---
 tests/qemu-iotests/tests/block-status-cache|  3 +--
 tests/qemu-iotests/tests/image-fleecing|  4 ++--
 tests/qemu-iotests/tests/mirror-ready-cancel-error |  6 ++
 tests/qemu-iotests/tests/mirror-top-perms  |  3 +--
 .../qemu-iotests/tests/remove-bitmap-from-backing  |  8 
 tests/qemu-iotests/tests/stream-error-on-reset |  4 ++--
 14 files changed, 40 insertions(+), 49 deletions(-)

diff --git a/tests/qemu-iotests/163 b/tests/qemu-iotests/163
index b8bfc95358e..e4cd4b230f3 100755
--- a/tests/qemu-iotests/163
+++ b/tests/qemu-iotests/163
@@ -107,8 +107,7 @@ class ShrinkBaseClass(iotests.QMPTestCase):
 
 if iotests.imgfmt == 'raw':
 return
-self.assertEqual(qemu_img('check', test_img), 0,
- "Verifying image corruption")
+qemu_img('check', test_img)
 
 def test_empty_image(self):
 qemu_img('resize',  '-f', iotests.imgfmt, '--shrink', test_img,
@@ -130,8 +129,7 @@ class ShrinkBaseClass(iotests.QMPTestCase):
 qemu_img('resize',  '-f', iotests.imgfmt, '--shrink', test_img,
  self.shrink_size)
 
-self.assertEqual(qemu_img("compare", test_img, check_img), 0,
- "Verifying image content")
+qemu_img("compare", test_img, check_img)
 
 self.image_verify()
 
@@ -146,8 +144,7 @@ class ShrinkBaseClass(iotests.QMPTestCase):
 qemu_img('resize',  '-f', iotests.imgfmt, '--shrink', test_img,
  self.shrink_size)
 
-self.assertEqual(qemu_img("compare", test_img, check_img), 0,
- "Verifying image content")
+qemu_img("compare", test_img, check_img)
 
 self.image_verify()
 
diff --git a/tests/qemu-iotests/216 b/tests/qemu-iotests/216
index c02f8d2880f..88b385afa30 100755
--- a/tests/qemu-iotests/216
+++ b/tests/qemu-iotests/216
@@ -51,10 +51,10 @@ with iotests.FilePath('base.img') as base_img_path, \
 log('--- Setting up images ---')
 log('')
 
-assert qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M') == 0
+qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M')
 assert qemu_io_silent(base_img_path, '-c', 'write -P 1 0M 1M') == 0
-assert qemu_img('create', '-f', iotests.imgfmt, '-b', base_img_path,
-'-F', iotests.imgfmt, top_img_path) == 0
+qemu_img('create', '-f', iotests.imgfmt, '-b', base_img_path,
+ '-F', iotests.imgfmt, top_img_path)
 assert qemu_io_silent(top_img_path,  '-c', 'write -P 2 1M 1M') == 0
 
 log('Done')
diff --git a/tests/qemu-iotests/218 b/tests/qemu-iotests/218
index 4922b4d3b6f..853ed52b349 100755
--- a/tests/qemu-iotests/218
+++ b/tests/qemu-iotests/218
@@ -145,7 +145,7 @@ log('')
 with iotests.VM() as vm, \
  iotests.FilePath('src.img') as src_img_path:
 
-assert qemu_img('create', '-f', iotests.imgfmt, src_img_path, '64M') == 0
+qemu_img('create', '-f', iotests.imgfmt, src_img_path, '64M')
 assert qemu_io_silent('-f', iotests.imgfmt, src_img_path,
   '-c', 'write -P 42 0M 64M') == 0
 
diff --git a/tests/qemu-iotests/224 b/tests/qemu-iotests/224
index 38dd1536254..c31c55b49d2 100755
--- a/tests/qemu-iotests/224
+++ b/tests/qemu-iotests/224
@@ -47,12 +47,11 @@ for filter_node_name in False, True:
  iotests.FilePath('top.img') as top_img_path, \
  iotests.VM() as vm:
 
-assert qemu_img('create', '-f', iotests.imgfmt,
-base_img_path, '64M') == 0
-assert qemu_img('create', '-f', iotests.imgfmt, '-b', base_img_path,
-'-F', iotests.imgfmt, mid_img_path) == 0
-assert qemu_img('create', '-f', iotests.imgfmt, '-b', mid_img_path,
-'-F', iotests.imgfmt, top_img_path) == 0
+qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M')
+qemu_img('create', '-f', iotests.imgfmt, '-b', base_img_path,
+ '-F', iotests.imgfmt, mid_img_path)
+qemu_img('create', '-f', iotests.imgfmt, '-b', mid_img_path,
+ '-F', iotests.imgfmt, top_img_path)
 
 # Something to commit
 assert qemu_io_silent(mid_img_path, '-c', 'write -P 1 0 1M') 

[PATCH v2 07/18] iotests: use qemu_img_json() when applicable

[John Snow]

These functions should now give better crash information when something
unexpected happens.

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 1e6947b893b..fc0d054e129 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -344,11 +344,11 @@ def qemu_img_json(*args: str) -> Any:
 json_data = json.loads(res.stdout)
 return json_data
 
-def qemu_img_measure(*args):
-return json.loads(qemu_img_pipe("measure", "--output", "json", *args))
+def qemu_img_measure(*args: str) -> Any:
+return qemu_img_json("measure", "--output", "json", *args)
 
-def qemu_img_check(*args):
-return json.loads(qemu_img_pipe("check", "--output", "json", *args))
+def qemu_img_check(*args: str) -> Any:
+return qemu_img_json("check", "--output", "json", *args)
 
 def qemu_img_pipe(*args: str) -> str:
 '''Run qemu-img and return its output'''
-- 
2.34.1

[PATCH v2 01/18] python/utils: add enboxify() text decoration utility

[John Snow]

>>> print(enboxify(msg, width=72, name="commit message"))
┏━ commit message ━┓
┃ enboxify() takes a chunk of text and wraps it in a text art box that ┃
┃  adheres to a specified width. An optional title label may be given, ┃
┃  and any of the individual glyphs used to draw the box may be┃
┃ replaced or specified as well.   ┃
┗━━┛

Signed-off-by: John Snow 
---
 python/qemu/utils/__init__.py | 58 +++
 1 file changed, 58 insertions(+)

diff --git a/python/qemu/utils/__init__.py b/python/qemu/utils/__init__.py
index 7f1a5138c4b..f785316f230 100644
--- a/python/qemu/utils/__init__.py
+++ b/python/qemu/utils/__init__.py
@@ -15,7 +15,10 @@
 # the COPYING file in the top-level directory.
 #
 
+import os
 import re
+import shutil
+import textwrap
 from typing import Optional
 
 # pylint: disable=import-error
@@ -23,6 +26,7 @@
 
 
 __all__ = (
+'enboxify',
 'get_info_usernet_hostfwd_port',
 'kvm_available',
 'list_accel',
@@ -43,3 +47,57 @@ def get_info_usernet_hostfwd_port(info_usernet_output: str) 
-> Optional[int]:
 if match is not None:
 return int(match[1])
 return None
+
+
+# pylint: disable=too-many-arguments
+def enboxify(
+content: str = '',
+width: Optional[int] = None,
+name: Optional[str] = None,
+padding: int = 1,
+upper_left: str = '┏',
+upper_right: str = '┓',
+lower_left: str = '┗',
+lower_right: str = '┛',
+horizontal: str = '━',
+vertical: str = '┃',
+) -> str:
+"""
+Wrap some text into a text art box of a given width.
+
+:param content: The text to wrap into a box.
+:param width: The number of columns (including the box itself).
+:param name: A label to apply to the upper-left of the box.
+:param padding: How many columns of padding to apply inside.
+"""
+if width is None:
+width = shutil.get_terminal_size()[0]
+prefix = vertical + (' ' * padding)
+suffix = (' ' * padding) + vertical
+lwidth = width - len(suffix)
+
+def _bar(name: Optional[str], top: bool = True) -> str:
+ret = upper_left if top else lower_left
+right = upper_right if top else lower_right
+if name is not None:
+ret += f"{horizontal} {name} "
+
+assert width is not None
+filler_len = width - len(ret) - len(right)
+ret += f"{horizontal * filler_len}{right}"
+return ret
+
+def _wrap(line: str) -> str:
+return os.linesep.join([
+wrapped_line.ljust(lwidth) + suffix
+for wrapped_line in textwrap.wrap(
+line, width=lwidth, initial_indent=prefix,
+subsequent_indent=prefix, replace_whitespace=False,
+drop_whitespace=False, break_on_hyphens=False)
+])
+
+return os.linesep.join((
+_bar(name, top=True),
+os.linesep.join(_wrap(line) for line in content.splitlines()),
+_bar(None, top=False),
+))
-- 
2.34.1

[PATCH v2 05/18] iotests: fortify compare_images() against crashes

[John Snow]

Make this helper function a little stricter about what it allows by
default. If qemu_img returns some exit code that implies it didn't
actually perform the comparison, treat that as an exceptional
circumstance and force the caller to be aware of the peril.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 21 -
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 546e5cb671b..24765de2e27 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -507,11 +507,22 @@ def qemu_nbd_popen(*args):
 p.kill()
 p.wait()
 
-def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
-'''Return True if two image files are identical'''
-res = qemu_img('compare', '-f', fmt1,
-   '-F', fmt2, img1, img2, check=False)
-return res.returncode == 0
+def compare_images(img1: str, img2: str,
+   fmt1: str = imgfmt, fmt2: str = imgfmt) -> bool:
+"""
+Compare two images with QEMU_IMG; return True if they are identical.
+
+:raise CalledProcessError:
+when qemu-img crashes or returns a status code of anything other
+than 0 (identical) or 1 (different).
+"""
+try:
+qemu_img('compare', '-f', fmt1, '-F', fmt2, img1, img2)
+return True
+except subprocess.CalledProcessError as exc:
+if exc.returncode == 1:
+return False
+raise
 
 def create_image(name, size):
 '''Create a fully-allocated raw image with sector markers'''
-- 
2.34.1

[PATCH v2 06/18] iotests: add qemu_img_json()

[John Snow]

A little helper built on top of qemu_img() that tries to pull a valid
JSON document out of the stdout stream.

In the event that the return code is negative (the program crashed), or
the code is greater than zero and did not produce valid JSON output, the
VerboseProcessError raised by qemu_img is re-raised instead.

In the event that the return code is zero but we can't parse valid JSON,
allow the JSON deserialization error to be raised.

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 24765de2e27..1e6947b893b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -309,6 +309,41 @@ def ordered_qmp(qmsg, conv_keys=True):
 def qemu_img_create(*args: str) -> subprocess.CompletedProcess[str]:
 return qemu_img('create', *args)
 
+def qemu_img_json(*args: str) -> Any:
+"""
+Run qemu-img and return its output as deserialized JSON.
+
+:raise CalledProcessError:
+When qemu-img crashes, or returns a non-zero exit code without
+producing a valid JSON document to stdout.
+:raise JSONDecoderError:
+When qemu-img returns 0, but failed to produce a valid JSON document.
+"""
+json_data = ...  # json.loads can legitimately return 'None'.
+
+try:
+res = qemu_img(*args, combine_stdio=False)
+except subprocess.CalledProcessError as exc:
+# Terminated due to signal. Don't bother.
+if exc.returncode < 0:
+raise
+
+# Commands like 'check' can return failure (exit codes 2 and 3)
+# to indicate command completion, but with errors found. For
+# multi-command flexibility, ignore the exact error codes and
+# *try* to load JSON.
+try:
+json_data = json.loads(exc.stdout)
+except json.JSONDecodeError:
+pass  # Nope. This thing is toast.
+
+if json_data is ...:
+raise
+
+if json_data is ...:
+json_data = json.loads(res.stdout)
+return json_data
+
 def qemu_img_measure(*args):
 return json.loads(qemu_img_pipe("measure", "--output", "json", *args))
 
-- 
2.34.1

[PATCH v2 02/18] iotests: add VerboseProcessError

[John Snow]

This adds an Exception that extends the Python built-in
subprocess.CalledProcessError. When this exception is raised, it will
still be caught when selecting for the stdlib variant.

The difference is that the str() method of this Exception also adds the
stdout/stderr logs. In effect, if this exception goes unhandled, Python
will print the output in a nice, highlighted box to the terminal so that
it's easy to spot.

This should save some headache from having to re-run test suites with
debugging enabled by augmenting the exceptions to print more information
in the default case.

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6ba65eb1ffe..5617f991da7 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -30,6 +30,7 @@
 import struct
 import subprocess
 import sys
+import textwrap
 import time
 from typing import (Any, Callable, Dict, Iterable, Iterator,
 List, Optional, Sequence, TextIO, Tuple, Type, TypeVar)
@@ -39,6 +40,7 @@
 
 from qemu.machine import qtest
 from qemu.qmp import QMPMessage
+from qemu.utils import enboxify
 
 # Use this logger for logging messages directly from the iotests module
 logger = logging.getLogger('qemu.iotests')
@@ -117,6 +119,38 @@
 sample_img_dir = os.environ['SAMPLE_IMG_DIR']
 
 
+class VerboseProcessError(subprocess.CalledProcessError):
+"""
+The same as CalledProcessError, but more verbose.
+
+This is useful for debugging failed calls during test executions.
+The return code, signal (if any), and terminal output will be displayed
+on unhandled exceptions.
+"""
+def summary(self) -> str:
+return super().__str__()
+
+def __str__(self) -> str:
+lmargin = '  '
+width = shutil.get_terminal_size()[0] - len(lmargin)
+sections = []
+
+name = 'output' if self.stderr is None else 'stdout'
+if self.stdout:
+sections.append(enboxify(self.stdout, width, name))
+else:
+sections.append(f"{name}: N/A")
+
+if self.stderr:
+sections.append(enboxify(self.stderr, width, 'stderr'))
+elif self.stderr is not None:
+sections.append("stderr: N/A")
+
+return os.linesep.join((
+self.summary(),
+textwrap.indent(os.linesep.join(sections), prefix=lmargin),
+))
+
 @contextmanager
 def change_log_level(
 logger_name: str, level: int = logging.CRITICAL) -> Iterator[None]:
-- 
2.34.1

[PATCH v2 00/18] iotests: add detailed tracebacks to qemu_img() failures

[John Snow]

This series does two things:

(1) Adds more detailed information to terminal output when qemu-img
crashes or returns with a non-zero exit code

(2) Ensures that every last call to qemu-img made in the iotest test
suite either returns a zero, *or* has its output logged.

This is accomplished by:

- Adding a new subprocess.CalledProcessError exception type that's more
  verbose than the standard, built-in one

- Rewriting the qemu_img() function to utilize that new Exception

- Modifying every last iotest helper that invokes qemu-img to use
  qemu_img() or a derivative thereof.

This patchset was inspired by Thomas Huth noticing that iotest 065 would
crash in a way that was largely silent, except for async QMP traces when
the VM failed to start. The root cause in that case is that iotest 065
did not tolerate zstd support being compiled out of QEMU, so the
qemu-img command fails - silently. (And subsequent test actions would
then explode nastily with confusing or misleading error messages.)

So, broadly, I rewrote qemu_img() to be a lot stricter by default, and
then rebased every other helper function that called into the qemu-img
process to use qemu_img().

RFC:

- 'enboxify()' text decoration does not support unicode too well. I
  think it's important to have some text decoration, but I haven't
  gotten around to making it smarter. I might remove it for something
  simpler to avoid having to learn more about unicode.

- At this point, every last qemu-img call is audited, but I did not
  extend the same treatment to qemu-io, qemu-nbd, etc. I intend to,
  later. Eventually, I plan to offer something like 'qemu_tool()' as a
  replacement for 'qemu_tool_pipe_and_status()', and qemu_img() will
  become a thin wrapper around qemu_tool().

V2:
 - Ensure *all* calls to qemu-img go through qemu_img()
 - Raise VerboseProcessError on negative return code,
   even when check=False
 - Check logging status for logged variants and revert to
   Exceptions when logging is off!

John Snow (18):
  python/utils: add enboxify() text decoration utility
  iotests: add VerboseProcessError
  iotests: Remove explicit checks for qemu_img() == 0
  iotests: make qemu_img raise on non-zero rc by default
  iotests: fortify compare_images() against crashes
  iotests: add qemu_img_json()
  iotests: use qemu_img_json() when applicable
  iotests: add qemu_img_info()
  iotests: remove-bitmap-from-backing: use qemu_img_info()
  iotests: add qemu_img_map() function
  iotests: change supports_quorum to use qemu_img
  iotests: replace unchecked calls to qemu_img_pipe()
  iotests: remove external calls to qemu_img_pipe()
  iotests: move has_working_luks onto qemu_img()
  iotests: remove qemu_img_log('create', ...) calls
  iotests: remove qemu_img_pipe()
  iotests: make qemu_img_log() check log level
  iotests: reimplement img_info_log in terms of qemu_img_log

 python/qemu/utils/__init__.py |  58 +
 tests/qemu-iotests/041|   5 +-
 tests/qemu-iotests/065|   7 +-
 tests/qemu-iotests/149|   8 +-
 tests/qemu-iotests/163|   9 +-
 tests/qemu-iotests/194|   4 +-
 tests/qemu-iotests/202|   4 +-
 tests/qemu-iotests/203|   4 +-
 tests/qemu-iotests/211|   6 +-
 tests/qemu-iotests/216|   6 +-
 tests/qemu-iotests/218|   2 +-
 tests/qemu-iotests/224|  11 +-
 tests/qemu-iotests/228|  12 +-
 tests/qemu-iotests/234|   4 +-
 tests/qemu-iotests/237|   3 +-
 tests/qemu-iotests/237.out|   3 -
 tests/qemu-iotests/242|   5 +-
 tests/qemu-iotests/255|   8 +-
 tests/qemu-iotests/255.out|   4 -
 tests/qemu-iotests/257|  11 +-
 tests/qemu-iotests/258|   4 +-
 tests/qemu-iotests/262|   2 +-
 tests/qemu-iotests/274|  17 +-
 tests/qemu-iotests/274.out|  29 ---
 tests/qemu-iotests/280|   2 +-
 tests/qemu-iotests/280.out|   1 -
 tests/qemu-iotests/296|  13 +-
 tests/qemu-iotests/303|   2 +-
 tests/qemu-iotests/310|  14 +-
 tests/qemu-iotests/iotests.py | 217 ++
 tests/qemu-iotests/tests/block-status-cache   |  14 +-
 tests/qemu-iotests/tests/image-fleecing   |   4 +-
 .../tests/mirror-ready-cancel-error   |   6 +-
 tests/qemu-iotests/tests/mirror-top-perms |   3 +-
 .../qemu-iotests/tests/parallels-read-bitmap  |   6 +-
 .../tests/remove-bitmap-from-backing  |  14 +-
 .../qemu-iotests/tests/stream-error-on-reset  |   4 +-
 37 files changed, 334 

[PATCH v2 13/18] iotests: remove external calls to qemu_img_pipe()

[John Snow]

Several cases here rely on the knowledge that qemu_img_pipe() suppresses
*all* output on a successful case when the command being issued is
'create'.

065: This call's output is inspected, but it appears as if it's expected
 to succeed. Replace this call with the checked qemu_img() variant
 instead to get better diagnostics if/when qemu-img itself fails.

149: If the check_cipher_support check activates, we'll skip the
 test. Otherwise, we re-raise the Exception and assert that the
 image creation works.

237: We are only testing blanks against the output.
 Remove them and use a simpler checked variant.

296: One create and one amend call. The create call is expected to
 always succeed, but it needs a print statement to keep the output
 file looking nice. The amend call is an intentional negative test;
 use check=False and log the output.

After this patch, the only uses of qemu_img_pipe are internal to
iotests.py and will be removed in subsequent patches.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/065 |  4 ++--
 tests/qemu-iotests/149 |  8 ++--
 tests/qemu-iotests/237 |  3 +--
 tests/qemu-iotests/237.out |  3 ---
 tests/qemu-iotests/296 | 13 +++--
 5 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
index 9466ce7df4d..ba94e19349b 100755
--- a/tests/qemu-iotests/065
+++ b/tests/qemu-iotests/065
@@ -24,7 +24,7 @@ import os
 import re
 import json
 import iotests
-from iotests import qemu_img, qemu_img_info, qemu_img_pipe
+from iotests import qemu_img, qemu_img_info
 import unittest
 
 test_img = os.path.join(iotests.test_dir, 'test.img')
@@ -54,7 +54,7 @@ class TestQemuImgInfo(TestImageInfoSpecific):
 self.assertEqual(data['data'], self.json_compare)
 
 def test_human(self):
-data = qemu_img_pipe('info', '--output=human', test_img).split('\n')
+data = qemu_img('info', '--output=human', test_img).stdout.split('\n')
 data = data[(data.index('Format specific information:') + 1)
 :data.index('')]
 for field in data:
diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index d49646ca60b..8b7dfb4e368 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -265,8 +265,12 @@ def qemu_img_create(config, size_mb):
 "%dM" % size_mb]
 
 iotests.log("qemu-img " + " ".join(args), 
filters=[iotests.filter_test_dir])
-iotests.log(check_cipher_support(config, iotests.qemu_img_pipe(*args)),
-filters=[iotests.filter_test_dir])
+try:
+iotests.qemu_img(*args)
+except subprocess.CalledProcessError as exc:
+iotests.log(check_cipher_support(config, exc.output),
+filters=[iotests.filter_test_dir])
+raise
 
 def qemu_io_image_args(config, dev=False):
 """Get the args for access an image or device with qemu-io"""
diff --git a/tests/qemu-iotests/237 b/tests/qemu-iotests/237
index 43dfd3bd40a..5ea13eb01fc 100755
--- a/tests/qemu-iotests/237
+++ b/tests/qemu-iotests/237
@@ -165,8 +165,7 @@ with iotests.FilePath('t.vmdk') as disk_path, \
 iotests.log("")
 
 for path in [ extent1_path, extent2_path, extent3_path ]:
-msg = iotests.qemu_img_pipe('create', '-f', imgfmt, path, '0')
-iotests.log(msg, [iotests.filter_testfiles])
+iotests.qemu_img_create('-f', imgfmt, path, '0')
 
 vm.add_blockdev('driver=file,filename=%s,node-name=ext1' % (extent1_path))
 vm.add_blockdev('driver=file,filename=%s,node-name=ext2' % (extent2_path))
diff --git a/tests/qemu-iotests/237.out b/tests/qemu-iotests/237.out
index aeb97244928..62b88656778 100644
--- a/tests/qemu-iotests/237.out
+++ b/tests/qemu-iotests/237.out
@@ -129,9 +129,6 @@ Job failed: Cannot find device='this doesn't exist' nor 
node-name='this doesn't
 
 === Other subformats ===
 
-
-
-
 == Missing extent ==
 
 {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": 
{"driver": "vmdk", "file": "node0", "size": 33554432, "subformat": 
"monolithicFlat"}}}
diff --git a/tests/qemu-iotests/296 b/tests/qemu-iotests/296
index 099a3eeaa52..f32ef037a58 100755
--- a/tests/qemu-iotests/296
+++ b/tests/qemu-iotests/296
@@ -76,7 +76,7 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
 # create the encrypted block device using qemu-img
 def createImg(self, file, secret):
 
-output = iotests.qemu_img_pipe(
+iotests.qemu_img(
 'create',
 '--object', *secret.to_cmdline_object(),
 '-f', iotests.imgfmt,
@@ -85,7 +85,7 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
 file,
 '1M')
 
-iotests.log(output, filters=[iotests.filter_test_dir])
+print('')  # Keeps 296.out prettier.
 
 # attempts to add a key using qemu-img
 def addKey(self, file, secret, new_secret):
@@ -99,7 +99,7 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Mark Cave-Ayland]

On 21/02/2022 17:11, Daniel P. Berrangé wrote:


On Sun, Feb 20, 2022 at 05:18:33PM +, Mark Cave-Ayland wrote:

On 08/02/2022 13:10, Daniel P. Berrangé wrote:


On Tue, Feb 08, 2022 at 01:06:59PM +, Mark Cave-Ayland wrote:

On 08/02/2022 12:49, Daniel P. Berrangé wrote:


I was under the impression that monitor_register_hmp_info_hrt() does all the
magic here i.e. it declares the underlying QMP command with an x- prefix and
effectively encapsulates the text field in a way that says "this is an
unreliable text opaque for humans"?


The monitor_register_hmp_info_hrt only does the HMP glue side, and
that's only needed if you must dynamically register the HMP command.
For statically registered commands set '.cmd_info_hrt' directly in
the hml-commands-info.hx for the HMP side.


If a qapi/ schema is needed could you explain what it should look like for
this example and where it should go? Looking at the existing .json files I
can't immediately see one which is the right place for this to live.


Take a look in qapi/machine.json for anyof the 'x-query-' commands
there. The QAPI bit is fairly simple.

if you want to see an illustration of what's different from a previous
pure HMP impl, look at:

 commit dd98234c059e6bdb05a52998270df6d3d990332e
 Author: Daniel P. Berrangé 
 Date:   Wed Sep 8 10:35:43 2021 +0100

   qapi: introduce x-query-roms QMP command


I see, thanks for the reference. So qapi/machine.json would be the right
place to declare the QMP part even for a specific device?

Even this approach still wouldn't work in its current form though, since as
mentioned in my previous email it seems that only the target CONFIG_*
defines and not the device CONFIG_* defines are present when processing
hmp-commands-info.hx.


Yeah, that's where the pain comes in.  While QAPI schema can be made
conditional on a few CONFIG_* parameters - basically those derived
from global configure time options, it is impossible for this to be
with with target specific options like the device CONFIG_* defines.

This is why I suggested in my othuer reply that it would need to be
done with a generic 'info dev-debug' / 'x-query-dev-debug' command
that can be registered unconditionally, and then individual devices
plug into it.


After some more experiments this afternoon I still seem to be falling
through the gaps on this one. This is based upon my understanding that all
new HMP commands should use a QMP HumanReadableText implementation and the
new command should be restricted according to target.

Currently I am working with this change to hmp-commands-info.hx and
qapi/misc-target.json:


[snip]
  

i.e. qmp_marshal_output_HumanReadableText() isn't protected by the #if
TARGET guards and since HumanReadableText is only used by the new
qmp_x_query_via() functionality then the compiler complains and aborts the
compilation.

Possibly this is an error in the QAPI generator for types hidden behind
commands using "if"? Otherwise I'm not sure what is the best way to proceed,
so I'd be grateful for some further pointers.


Yes, this is pretty much what I expect and exactly what I hit with
other target specific commands.

That's why I suggested something like a general 'x-device-debug' command
that is NOT conditionalized in QAPI, against which dev impls can register
a callback to provide detailed reporting, instead of a device type specific
command.


Ah so this is a known issue with this approach then. David mentioned earlier in the 
thread that he'd be okay with a HMP command if it was useful and restricted to the 
required targets, so would it be okay to add "info via" for now as just a (non-QMP 
wrapped) HMP info command if I can get that to work?



ATB,

Mark.

Re: [PATCH 0/2] Resolve some redundant property accessors

[Philippe Mathieu-Daudé]

On 17/2/22 23:53, Bernhard Beschow wrote:

The QOM API already provides appropriate accessors, so reuse them.

Testing done:

   :$ make check
   Ok: 569
   Expected Fail:  0
   Fail:   0
   Unexpected Pass:0
   Skipped:178
   Timeout:0

Bernhard Beschow (2):
   hw/vfio/pci-quirks: Resolve redundant property getters
   hw/riscv/sifive_u: Resolve redundant property accessors


Good cleanup.

You might want to play with Coccinelle spatch [*] to clean all uses:

$ git grep object_property_add\(.*uint
hw/acpi/ich9.c:446:object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, 
"uint32",
hw/i386/sgx-epc.c:47:object_property_add(obj, SGX_EPC_SIZE_PROP, 
"uint64", sgx_epc_get_size,

hw/intc/apic_common.c:462:object_property_add(obj, "id", "uint32",
hw/mem/pc-dimm.c:175:object_property_add(obj, PC_DIMM_SIZE_PROP, 
"uint64", pc_dimm_get_size,
hw/misc/aspeed_lpc.c:420:object_property_add(obj, "idr1", "uint32", 
aspeed_kcs_get_register_property,
hw/misc/aspeed_lpc.c:422:object_property_add(obj, "odr1", "uint32", 
aspeed_kcs_get_register_property,
hw/misc/aspeed_lpc.c:424:object_property_add(obj, "str1", "uint32", 
aspeed_kcs_get_register_property,
hw/misc/aspeed_lpc.c:426:object_property_add(obj, "idr2", "uint32", 
aspeed_kcs_get_register_property,
hw/misc/aspeed_lpc.c:428:object_property_add(obj, "odr2", "uint32", 
aspeed_kcs_get_register_property,
hw/misc/aspeed_lpc.c:430:object_property_add(obj, "str2", "uint32", 
aspeed_kcs_get_register_property,
hw/misc/aspeed_lpc.c:432:object_property_add(obj, "idr3", "uint32", 
aspeed_kcs_get_register_property,
hw/misc/aspeed_lpc.c:434:object_property_add(obj, "odr3", "uint32", 
aspeed_kcs_get_register_property,
hw/misc/aspeed_lpc.c:436:object_property_add(obj, "str3", "uint32", 
aspeed_kcs_get_register_property,
hw/misc/aspeed_lpc.c:438:object_property_add(obj, "idr4", "uint32", 
aspeed_kcs_get_register_property,
hw/misc/aspeed_lpc.c:440:object_property_add(obj, "odr4", "uint32", 
aspeed_kcs_get_register_property,
hw/misc/aspeed_lpc.c:442:object_property_add(obj, "str4", "uint32", 
aspeed_kcs_get_register_property,
hw/misc/npcm7xx_mft.c:493:object_property_add(obj, "max_rpm[*]", 
"uint32",
hw/nvme/ctrl.c:6856:object_property_add(obj, 
"smart_critical_warning", "uint8",
hw/pci-host/q35.c:224:object_property_add(obj, 
PCI_HOST_PROP_PCI_HOLE_START, "uint32",
hw/pci-host/q35.c:228:object_property_add(obj, 
PCI_HOST_PROP_PCI_HOLE_END, "uint32",
hw/pci-host/q35.c:232:object_property_add(obj, 
PCI_HOST_PROP_PCI_HOLE64_START, "uint64",
hw/pci-host/q35.c:236:object_property_add(obj, 
PCI_HOST_PROP_PCI_HOLE64_END, "uint64",
hw/ppc/spapr_drc.c:584:object_property_add(obj, "index", "uint32", 
prop_get_index,

hw/riscv/sifive_u.c:736:object_property_add(obj, "msel", "uint32",
hw/riscv/sifive_u.c:743:object_property_add(obj, "serial", "uint32",
hw/sensor/adm1272.c:497:object_property_add(obj, "vin", "uint16",
hw/sensor/adm1272.c:501:object_property_add(obj, "vout", "uint16",
hw/sensor/adm1272.c:505:object_property_add(obj, "iout", "uint16",
hw/sensor/adm1272.c:509:object_property_add(obj, "pin", "uint16",
hw/sensor/max34451.c:730:object_property_add(obj, "vout[*]", 
"uint16",
hw/sensor/max34451.c:740:object_property_add(obj, 
"temperature[*]", "uint16",
hw/vfio/pci-quirks.c:1621:object_property_add(OBJECT(vdev), 
"nvlink2-tgt", "uint64",
hw/vfio/pci-quirks.c:1682:object_property_add(OBJECT(vdev), 
"nvlink2-tgt", "uint64",
hw/vfio/pci-quirks.c:1688:object_property_add(OBJECT(vdev), 
"nvlink2-link-speed", "uint32",
net/colo-compare.c:1390:object_property_add(obj, "compare_timeout", 
"uint64",
net/colo-compare.c:1394:object_property_add(obj, 
"expired_scan_cycle", "uint32",
net/colo-compare.c:1398:object_property_add(obj, "max_queue_size", 
"uint32",
softmmu/memory.c:1262:object_property_add(OBJECT(mr), "priority", 
"uint32",

softmmu/memory.c:1266:object_property_add(OBJECT(mr), "size", "uint64",
target/arm/cpu64.c:863:object_property_add(obj, "sve-max-vq", 
"uint32", cpu_max_get_sve_max_vq,
tests/unit/test-qdev-global-props.c:155:object_property_add(obj, 
"prop1", "uint32", prop1_accessor, prop1_accessor,
tests/unit/test-qdev-global-props.c:157:object_property_add(obj, 
"prop2", "uint32", prop2_accessor, prop2_accessor,


[*] https://coccinelle.gitlabpages.inria.fr/website/

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Mark Cave-Ayland]

On 21/02/2022 12:20, Philippe Mathieu-Daudé wrote:


+Thomas

On 20/2/22 18:18, Mark Cave-Ayland wrote:

On 08/02/2022 13:10, Daniel P. Berrangé wrote:


On Tue, Feb 08, 2022 at 01:06:59PM +, Mark Cave-Ayland wrote:

On 08/02/2022 12:49, Daniel P. Berrangé wrote:


I was under the impression that monitor_register_hmp_info_hrt() does all the
magic here i.e. it declares the underlying QMP command with an x- prefix and
effectively encapsulates the text field in a way that says "this is an
unreliable text opaque for humans"?


The monitor_register_hmp_info_hrt only does the HMP glue side, and
that's only needed if you must dynamically register the HMP command.
For statically registered commands set '.cmd_info_hrt' directly in
the hml-commands-info.hx for the HMP side.


If a qapi/ schema is needed could you explain what it should look like for
this example and where it should go? Looking at the existing .json files I
can't immediately see one which is the right place for this to live.


Take a look in qapi/machine.json for anyof the 'x-query-' commands
there. The QAPI bit is fairly simple.

if you want to see an illustration of what's different from a previous
pure HMP impl, look at:

    commit dd98234c059e6bdb05a52998270df6d3d990332e
    Author: Daniel P. Berrangé 
    Date:   Wed Sep 8 10:35:43 2021 +0100

  qapi: introduce x-query-roms QMP command


I see, thanks for the reference. So qapi/machine.json would be the right
place to declare the QMP part even for a specific device?

Even this approach still wouldn't work in its current form though, since as
mentioned in my previous email it seems that only the target CONFIG_*
defines and not the device CONFIG_* defines are present when processing
hmp-commands-info.hx.


Yeah, that's where the pain comes in.  While QAPI schema can be made
conditional on a few CONFIG_* parameters - basically those derived
from global configure time options, it is impossible for this to be
with with target specific options like the device CONFIG_* defines.

This is why I suggested in my othuer reply that it would need to be
done with a generic 'info dev-debug' / 'x-query-dev-debug' command
that can be registered unconditionally, and then individual devices
plug into it.


After some more experiments this afternoon I still seem to be falling through the 
gaps on this one. This is based upon my understanding that all new HMP commands 
should use a QMP HumanReadableText implementation and the new command should be 
restricted according to target.


Currently I am working with this change to hmp-commands-info.hx and 
qapi/misc-target.json:



diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 7e6bd30395..aac86d5473 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -880,15 +880,15 @@ SRST
  Show intel SGX information.
  ERST

#if defined(TARGET_M68K) || defined(TARGET_PPC)
  {
  .name = "via",
  .args_type    = "",
  .params   = "",
  .help = "show guest mos6522 VIA devices",
  .cmd_info_hrt = qmp_x_query_via,
  },
diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index 4bc45d2474..72bf71888e 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -2,6 +2,8 @@
  # vim: filetype=python
  #

+{ 'include': 'common.json' }
+
  ##
  # @RTC_CHANGE:
  #
@@ -424,3 +426,19 @@
  #
  ##
  { 'command': 'query-sgx-capabilities', 'returns': 'SGXInfo', 'if': 
'TARGET_I386' }
+
+##
+# @x-query-via:
+#
+# Query information on the registered mos6522 VIAs
+#
+# Features:
+# @unstable: This command is meant for debugging.
+#
+# Returns: internal mos6522 information
+#
+# Since: 7.0
+##
+{ 'command': 'x-query-via',
+  'returns': 'HumanReadableText',
+  'features': [ 'unstable' ], 'if': { 'any': [ 'TARGET_M68K', 'TARGET_PPC' ] } 
}


The main problem with trying to restrict the new command to a target (or targets) 
is that the autogenerated qapi/qapi-commands-misc-target.h QAPI header cannot be 
included in a device header such as mos6522.h without getting poison errors like 
below (which does actually make sense):



In file included from ./qapi/qapi-commands-misc-target.h:17,
  from 
/home/build/src/qemu/git/qemu/include/hw/misc/mos6522.h:35,
  from ../hw/misc/mos6522.c:30:
./qapi/qapi-types-misc-target.h:19:13: error: attempt to use poisoned 
"TARGET_ALPHA"


Can be kludged by making this device target-specific:

-- >8 --
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 6dcbe044f3..65837b1dfe 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -23 +23 @@ softmmu_ss.add(when: 'CONFIG_ARM_V7M', if_true: 
files('armv7m_ras.c'))
-softmmu_ss.add(when: 'CONFIG_MOS6522', if_true: files('mos6522.c'))
+specific_ss.add(when: 'CONFIG_MOS6522', if_true: files('mos6522.c'))
---

I'd rather keep devices generic, but sometime we can't. In this case
VIA is only used on m68k so it could be acceptable.


That's not quite true though, it's 

Re: [PATCH 2/2] hw/riscv/sifive_u: Resolve redundant property accessors

[Philippe Mathieu-Daudé]

On 17/2/22 23:53, Bernhard Beschow wrote:

The QOM API already provides accessors for uint32 values, so reuse them.

Signed-off-by: Bernhard Beschow 
---
  hw/riscv/sifive_u.c | 24 
  1 file changed, 4 insertions(+), 20 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 1/2] hw/vfio/pci-quirks: Resolve redundant property getters

[Philippe Mathieu-Daudé]

On 17/2/22 23:53, Bernhard Beschow wrote:

The QOM API already provides getters for uint64 and uint32 values, so reuse
them.

Signed-off-by: Bernhard Beschow 
---
  hw/vfio/pci-quirks.c | 34 +-
  1 file changed, 9 insertions(+), 25 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [RFC PATCH v2 2/3] hw/intc: Support 32/64-bit mtimecmp and mtime accesses in RISC-V ACLINT

[Alistair Francis]

On Thu, Feb 10, 2022 at 4:22 PM  wrote:
>
> From: Frank Chang 
>
> RISC-V privilege spec defines that:
>
> * In RV32, memory-mapped writes to mtimecmp modify only one 32-bit part
>   of the register.
> * For RV64, naturally aligned 64-bit memory accesses to the mtime and
>   mtimecmp registers are additionally supported and are atomic.
>
> It's possible to perform both 32/64-bit read/write accesses to both
> mtimecmp and mtime registers.
>
> Signed-off-by: Frank Chang 
> ---
>  hw/intc/riscv_aclint.c | 26 --
>  1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
> index 3b598d8a7e..e7b103e83a 100644
> --- a/hw/intc/riscv_aclint.c
> +++ b/hw/intc/riscv_aclint.c
> @@ -126,9 +126,9 @@ static uint64_t riscv_aclint_mtimer_read(void *opaque, 
> hwaddr addr,
>  qemu_log_mask(LOG_GUEST_ERROR,
>"aclint-mtimer: invalid hartid: %zu", hartid);
>  } else if ((addr & 0x7) == 0) {
> -/* timecmp_lo */
> +/* timecmp_lo for RV32/RV64 or timecmp for RV64 */
>  uint64_t timecmp = env->timecmp;
> -return timecmp & 0x;
> +return (size == 4) ? (timecmp & 0x) : timecmp;
>  } else if ((addr & 0x7) == 4) {
>  /* timecmp_hi */
>  uint64_t timecmp = env->timecmp;
> @@ -139,8 +139,9 @@ static uint64_t riscv_aclint_mtimer_read(void *opaque, 
> hwaddr addr,
>  return 0;
>  }
>  } else if (addr == mtimer->time_base) {
> -/* time_lo */
> -return cpu_riscv_read_rtc(mtimer->timebase_freq) & 0x;
> +/* time_lo for RV32/RV64 or timecmp for RV64 */
> +uint64_t rtc = cpu_riscv_read_rtc(mtimer->timebase_freq);
> +return (size == 4) ? (rtc & 0x) : rtc;
>  } else if (addr == mtimer->time_base + 4) {
>  /* time_hi */
>  return (cpu_riscv_read_rtc(mtimer->timebase_freq) >> 32) & 
> 0x;
> @@ -167,12 +168,17 @@ static void riscv_aclint_mtimer_write(void *opaque, 
> hwaddr addr,
>  qemu_log_mask(LOG_GUEST_ERROR,
>"aclint-mtimer: invalid hartid: %zu", hartid);
>  } else if ((addr & 0x7) == 0) {
> -/* timecmp_lo */
> -uint64_t timecmp_hi = env->timecmp >> 32;
> -riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu), hartid,
> -timecmp_hi << 32 | (value & 0x),
> -mtimer->timebase_freq);
> -return;
> +if (size == 4) {
> +/* timecmp_lo for RV32/RV64 */
> +uint64_t timecmp_hi = env->timecmp >> 32;
> +riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu), 
> hartid,
> +timecmp_hi << 32 | (value & 0x),
> +mtimer->timebase_freq);
> +} else {
> +/* timecmp for RV64 */
> +riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu), 
> hartid,
> +  value, 
> mtimer->timebase_freq);
> +}
>  } else if ((addr & 0x7) == 4) {
>  /* timecmp_hi */
>  uint64_t timecmp_lo = env->timecmp;

Should we check the other accesses to make sure the size == 4?

Alistair

> --
> 2.31.1
>
>

Re: [RFC PATCH v2 3/3] hw/intc: Make RISC-V ACLINT mtime MMIO register writable

[Alistair Francis]

On Thu, Feb 10, 2022 at 4:22 PM  wrote:
>
> From: Frank Chang 
>
> RISC-V privilege spec defines that mtime is exposed as a memory-mapped
> machine-mode read-write register. However, as QEMU uses host monotonic
> timer as timer source, this makes mtime to be read-only in RISC-V
> ACLINT.
>
> This patch makes mtime to be writable by recording the time delta value
> between the mtime value to be written and the timer value at the time
> mtime is written. Time delta value is then added back whenever the timer
> value is retrieved.
>
> Signed-off-by: Frank Chang 
> ---
>  hw/intc/riscv_aclint.c | 65 ++
>  include/hw/intc/riscv_aclint.h |  1 +
>  target/riscv/cpu.h |  8 ++---
>  target/riscv/cpu_helper.c  |  4 +--
>  4 files changed, 50 insertions(+), 28 deletions(-)
>
> diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
> index e7b103e83a..2d7d7361be 100644
> --- a/hw/intc/riscv_aclint.c
> +++ b/hw/intc/riscv_aclint.c
> @@ -38,12 +38,18 @@ typedef struct riscv_aclint_mtimer_callback {
>  int num;
>  } riscv_aclint_mtimer_callback;
>
> -static uint64_t cpu_riscv_read_rtc(uint32_t timebase_freq)
> +static uint64_t cpu_riscv_read_rtc_raw(uint32_t timebase_freq)
>  {
>  return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
>  timebase_freq, NANOSECONDS_PER_SECOND);
>  }
>
> +static uint64_t cpu_riscv_read_rtc(void *opaque)
> +{
> +RISCVAclintMTimerState *mtimer = opaque;
> +return cpu_riscv_read_rtc_raw(mtimer->timebase_freq) + 
> mtimer->time_delta;
> +}
> +
>  /*
>   * Called when timecmp is written to update the QEMU timer or immediately
>   * trigger timer interrupt if mtimecmp <= current timer value.
> @@ -51,13 +57,13 @@ static uint64_t cpu_riscv_read_rtc(uint32_t timebase_freq)
>  static void riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
>RISCVCPU *cpu,
>int hartid,
> -  uint64_t value,
> -  uint32_t timebase_freq)
> +  uint64_t value)
>  {
> +uint32_t timebase_freq = mtimer->timebase_freq;
>  uint64_t next;
>  uint64_t diff;
>
> -uint64_t rtc_r = cpu_riscv_read_rtc(timebase_freq);
> +uint64_t rtc_r = cpu_riscv_read_rtc(mtimer);
>
>  cpu->env.timecmp = value;
>  if (cpu->env.timecmp <= rtc_r) {
> @@ -140,11 +146,11 @@ static uint64_t riscv_aclint_mtimer_read(void *opaque, 
> hwaddr addr,
>  }
>  } else if (addr == mtimer->time_base) {
>  /* time_lo for RV32/RV64 or timecmp for RV64 */
> -uint64_t rtc = cpu_riscv_read_rtc(mtimer->timebase_freq);
> +uint64_t rtc = cpu_riscv_read_rtc(mtimer);
>  return (size == 4) ? (rtc & 0x) : rtc;
>  } else if (addr == mtimer->time_base + 4) {
>  /* time_hi */
> -return (cpu_riscv_read_rtc(mtimer->timebase_freq) >> 32) & 
> 0x;
> +return (cpu_riscv_read_rtc(mtimer) >> 32) & 0x;
>  }
>
>  qemu_log_mask(LOG_UNIMP,
> @@ -157,6 +163,7 @@ static void riscv_aclint_mtimer_write(void *opaque, 
> hwaddr addr,
>  uint64_t value, unsigned size)
>  {
>  RISCVAclintMTimerState *mtimer = opaque;
> +int i;
>
>  if (addr >= mtimer->timecmp_base &&
>  addr < (mtimer->timecmp_base + (mtimer->num_harts << 3))) {
> @@ -172,35 +179,49 @@ static void riscv_aclint_mtimer_write(void *opaque, 
> hwaddr addr,
>  /* timecmp_lo for RV32/RV64 */
>  uint64_t timecmp_hi = env->timecmp >> 32;
>  riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu), 
> hartid,
> -timecmp_hi << 32 | (value & 0x),
> -mtimer->timebase_freq);
> +timecmp_hi << 32 | (value & 0x));
>  } else {
>  /* timecmp for RV64 */
>  riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu), 
> hartid,
> -  value, 
> mtimer->timebase_freq);
> +  value);
>  }
>  } else if ((addr & 0x7) == 4) {
>  /* timecmp_hi */
>  uint64_t timecmp_lo = env->timecmp;
>  riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu), hartid,
> -value << 32 | (timecmp_lo & 0x),
> -mtimer->timebase_freq);
> +value << 32 | (timecmp_lo & 0x));
>  } else {
>  qemu_log_mask(LOG_UNIMP,
>"aclint-mtimer: invalid timecmp write: %08x",
>(uint32_t)addr);
>  }
>  return;
> -} else if (addr == mtimer->time_base) {
> -/* time_lo */
> -qemu_log_mask(LOG_UNIMP,
> -  "aclint-mtimer: 

Re: [RFC PATCH v2 2/3] hw/intc: Support 32/64-bit mtimecmp and mtime accesses in RISC-V ACLINT

[Alistair Francis]

On Thu, Feb 10, 2022 at 4:22 PM  wrote:
>
> From: Frank Chang 
>
> RISC-V privilege spec defines that:
>
> * In RV32, memory-mapped writes to mtimecmp modify only one 32-bit part
>   of the register.
> * For RV64, naturally aligned 64-bit memory accesses to the mtime and
>   mtimecmp registers are additionally supported and are atomic.
>
> It's possible to perform both 32/64-bit read/write accesses to both
> mtimecmp and mtime registers.
>
> Signed-off-by: Frank Chang 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  hw/intc/riscv_aclint.c | 26 --
>  1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
> index 3b598d8a7e..e7b103e83a 100644
> --- a/hw/intc/riscv_aclint.c
> +++ b/hw/intc/riscv_aclint.c
> @@ -126,9 +126,9 @@ static uint64_t riscv_aclint_mtimer_read(void *opaque, 
> hwaddr addr,
>  qemu_log_mask(LOG_GUEST_ERROR,
>"aclint-mtimer: invalid hartid: %zu", hartid);
>  } else if ((addr & 0x7) == 0) {
> -/* timecmp_lo */
> +/* timecmp_lo for RV32/RV64 or timecmp for RV64 */
>  uint64_t timecmp = env->timecmp;
> -return timecmp & 0x;
> +return (size == 4) ? (timecmp & 0x) : timecmp;
>  } else if ((addr & 0x7) == 4) {
>  /* timecmp_hi */
>  uint64_t timecmp = env->timecmp;
> @@ -139,8 +139,9 @@ static uint64_t riscv_aclint_mtimer_read(void *opaque, 
> hwaddr addr,
>  return 0;
>  }
>  } else if (addr == mtimer->time_base) {
> -/* time_lo */
> -return cpu_riscv_read_rtc(mtimer->timebase_freq) & 0x;
> +/* time_lo for RV32/RV64 or timecmp for RV64 */
> +uint64_t rtc = cpu_riscv_read_rtc(mtimer->timebase_freq);
> +return (size == 4) ? (rtc & 0x) : rtc;
>  } else if (addr == mtimer->time_base + 4) {
>  /* time_hi */
>  return (cpu_riscv_read_rtc(mtimer->timebase_freq) >> 32) & 
> 0x;
> @@ -167,12 +168,17 @@ static void riscv_aclint_mtimer_write(void *opaque, 
> hwaddr addr,
>  qemu_log_mask(LOG_GUEST_ERROR,
>"aclint-mtimer: invalid hartid: %zu", hartid);
>  } else if ((addr & 0x7) == 0) {
> -/* timecmp_lo */
> -uint64_t timecmp_hi = env->timecmp >> 32;
> -riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu), hartid,
> -timecmp_hi << 32 | (value & 0x),
> -mtimer->timebase_freq);
> -return;
> +if (size == 4) {
> +/* timecmp_lo for RV32/RV64 */
> +uint64_t timecmp_hi = env->timecmp >> 32;
> +riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu), 
> hartid,
> +timecmp_hi << 32 | (value & 0x),
> +mtimer->timebase_freq);
> +} else {
> +/* timecmp for RV64 */
> +riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu), 
> hartid,
> +  value, 
> mtimer->timebase_freq);
> +}
>  } else if ((addr & 0x7) == 4) {
>  /* timecmp_hi */
>  uint64_t timecmp_lo = env->timecmp;
> --
> 2.31.1
>
>

Re: [RFC PATCH v2 1/3] hw/intc: Add .impl.[min|max]_access_size declaration in RISC-V ACLINT

[Alistair Francis]

On Thu, Feb 10, 2022 at 4:19 PM  wrote:
>
> From: Frank Chang 
>
> If device's MemoryRegion doesn't have .impl.[min|max]_access_size
> declaration, the default access_size_min would be 1 byte and
> access_size_max would be 4 bytes (see: softmmu/memory.c).
> This will cause a 64-bit memory access to ACLINT to be splitted into
> two 32-bit memory accesses.
>
> Signed-off-by: Frank Chang 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  hw/intc/riscv_aclint.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
> index f1a5d3d284..3b598d8a7e 100644
> --- a/hw/intc/riscv_aclint.c
> +++ b/hw/intc/riscv_aclint.c
> @@ -208,6 +208,10 @@ static const MemoryRegionOps riscv_aclint_mtimer_ops = {
>  .valid = {
>  .min_access_size = 4,
>  .max_access_size = 8
> +},
> +.impl = {
> +.min_access_size = 4,
> +.max_access_size = 8,
>  }
>  };
>
> --
> 2.31.1
>
>

Re: [PATCH 2/2] hw/riscv/sifive_u: Resolve redundant property accessors

[Alistair Francis]

On Fri, Feb 18, 2022 at 8:54 AM Bernhard Beschow  wrote:
>
> The QOM API already provides accessors for uint32 values, so reuse them.
>
> Signed-off-by: Bernhard Beschow 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  hw/riscv/sifive_u.c | 24 
>  1 file changed, 4 insertions(+), 20 deletions(-)
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 7fbc7dea42..747eb4ee89 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -713,36 +713,20 @@ static void sifive_u_machine_set_start_in_flash(Object 
> *obj, bool value, Error *
>  s->start_in_flash = value;
>  }
>
> -static void sifive_u_machine_get_uint32_prop(Object *obj, Visitor *v,
> - const char *name, void *opaque,
> - Error **errp)
> -{
> -visit_type_uint32(v, name, (uint32_t *)opaque, errp);
> -}
> -
> -static void sifive_u_machine_set_uint32_prop(Object *obj, Visitor *v,
> - const char *name, void *opaque,
> - Error **errp)
> -{
> -visit_type_uint32(v, name, (uint32_t *)opaque, errp);
> -}
> -
>  static void sifive_u_machine_instance_init(Object *obj)
>  {
>  SiFiveUState *s = RISCV_U_MACHINE(obj);
>
>  s->start_in_flash = false;
>  s->msel = 0;
> -object_property_add(obj, "msel", "uint32",
> -sifive_u_machine_get_uint32_prop,
> -sifive_u_machine_set_uint32_prop, NULL, >msel);
> +object_property_add_uint32_ptr(obj, "msel", >msel,
> +   OBJ_PROP_FLAG_READWRITE);
>  object_property_set_description(obj, "msel",
>  "Mode Select (MSEL[3:0]) pin state");
>
>  s->serial = OTP_SERIAL;
> -object_property_add(obj, "serial", "uint32",
> -sifive_u_machine_get_uint32_prop,
> -sifive_u_machine_set_uint32_prop, NULL, >serial);
> +object_property_add_uint32_ptr(obj, "serial", >serial,
> +   OBJ_PROP_FLAG_READWRITE);
>  object_property_set_description(obj, "serial", "Board serial number");
>  }
>
> --
> 2.35.1
>
>

Re: [PATCH v3 4/6] target/riscv: Add support for mconfigptr

[Alistair Francis]

On Sun, Feb 6, 2022 at 7:43 PM Atish Patra  wrote:
>
> RISC-V privileged specification v1.12 introduced a mconfigptr
> which will hold the physical address of a configuration data
> structure. As Qemu doesn't have a configuration data structure,
> is read as zero which is valid as per the priv spec.
>
> Signed-off-by: Atish Patra 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu_bits.h | 1 +
>  target/riscv/csr.c  | 2 ++
>  2 files changed, 3 insertions(+)
>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index f96d26399607..89440241632a 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -148,6 +148,7 @@
>  #define CSR_MARCHID 0xf12
>  #define CSR_MIMPID  0xf13
>  #define CSR_MHARTID 0xf14
> +#define CSR_MCONFIGPTR  0xf15
>
>  /* Machine Trap Setup */
>  #define CSR_MSTATUS 0x300
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 25a0df498669..18fe17b62f51 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3021,6 +3021,8 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>  [CSR_MIMPID]= { "mimpid",any,   read_zero},
>  [CSR_MHARTID]   = { "mhartid",   any,   read_mhartid },
>
> +[CSR_MCONFIGPTR]  = { "mconfigptr", any,   read_zero,
> +.min_priv_ver = PRIV_VERSION_1_12_0 
> },
>  /* Machine Trap Setup */
>  [CSR_MSTATUS] = { "mstatus",any,   read_mstatus, 
> write_mstatus, NULL,
> read_mstatus_i128 
>   },
> --
> 2.30.2
>
>

Re: [PATCH v3 3/6] target/riscv: Introduce privilege version field in the CSR ops.

[Alistair Francis]

On Sun, Feb 6, 2022 at 7:19 PM Atish Patra  wrote:
>
> To allow/disallow the CSR access based on the privilege spec, a new field
> in the csr_ops is introduced. It also adds the privileged specification
> version (v1.12) for the CSRs introduced in the v1.12. This includes the
> new ratified extensions such as Vector, Hypervisor and secconfig CSR.
>
> Signed-off-by: Atish Patra 

It might be worth mentioning that there is no enforcement in this commit

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.h |   2 +
>  target/riscv/csr.c | 103 ++---
>  2 files changed, 70 insertions(+), 35 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 60b847141db2..0741f9822cf0 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -593,6 +593,8 @@ typedef struct {
>  riscv_csr_op_fn op;
>  riscv_csr_read128_fn read128;
>  riscv_csr_write128_fn write128;
> +/* The default priv spec version should be PRIV_VERSION_1_10_0 (i.e 0) */
> +uint32_t min_priv_ver;
>  } riscv_csr_operations;
>
>  /* CSR function table constants */
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 8c63caa39245..25a0df498669 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -2981,13 +2981,20 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>  [CSR_FRM]  = { "frm",  fs, read_frm, write_frm},
>  [CSR_FCSR] = { "fcsr", fs, read_fcsr,write_fcsr   },
>  /* Vector CSRs */
> -[CSR_VSTART]   = { "vstart",   vs, read_vstart,  write_vstart },
> -[CSR_VXSAT]= { "vxsat",vs, read_vxsat,   write_vxsat  },
> -[CSR_VXRM] = { "vxrm", vs, read_vxrm,write_vxrm   },
> -[CSR_VCSR] = { "vcsr", vs, read_vcsr,write_vcsr   },
> -[CSR_VL]   = { "vl",   vs, read_vl},
> -[CSR_VTYPE]= { "vtype",vs, read_vtype },
> -[CSR_VLENB]= { "vlenb",vs, read_vlenb },
> +[CSR_VSTART]   = { "vstart",   vs,read_vstart,  write_vstart,
> +  .min_priv_ver = 
> PRIV_VERSION_1_12_0 },
> +[CSR_VXSAT]= { "vxsat",vs,read_vxsat,   write_vxsat,
> +  .min_priv_ver = 
> PRIV_VERSION_1_12_0 },
> +[CSR_VXRM] = { "vxrm", vs,read_vxrm,write_vxrm,
> +  .min_priv_ver = 
> PRIV_VERSION_1_12_0 },
> +[CSR_VCSR] = { "vcsr", vs,read_vcsr,write_vcsr,
> +  .min_priv_ver = 
> PRIV_VERSION_1_12_0 },
> +[CSR_VL]   = { "vl",   vs,read_vl,
> +  .min_priv_ver = 
> PRIV_VERSION_1_12_0 },
> +[CSR_VTYPE]= { "vtype",vs,read_vtype,
> +  .min_priv_ver = 
> PRIV_VERSION_1_12_0 },
> +[CSR_VLENB]= { "vlenb",vs,read_vlenb,
> +  .min_priv_ver = 
> PRIV_VERSION_1_12_0 },
>  /* User Timers and Counters */
>  [CSR_CYCLE]= { "cycle",ctr,read_instret  },
>  [CSR_INSTRET]  = { "instret",  ctr,read_instret  },
> @@ -3096,33 +3103,58 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>  [CSR_SIEH]   = { "sieh",   aia_smode32, NULL, NULL, rmw_sieh },
>  [CSR_SIPH]   = { "siph",   aia_smode32, NULL, NULL, rmw_siph },
>
> -[CSR_HSTATUS] = { "hstatus", hmode,   read_hstatus, 
> write_hstatus },
> -[CSR_HEDELEG] = { "hedeleg", hmode,   read_hedeleg, 
> write_hedeleg },
> -[CSR_HIDELEG] = { "hideleg", hmode,   NULL,   NULL, 
> rmw_hideleg   },
> -[CSR_HVIP]= { "hvip",hmode,   NULL,   NULL, rmw_hvip 
>  },
> -[CSR_HIP] = { "hip", hmode,   NULL,   NULL, rmw_hip  
>  },
> -[CSR_HIE] = { "hie", hmode,   NULL,   NULL, rmw_hie  
>  },
> -[CSR_HCOUNTEREN]  = { "hcounteren",  hmode,   read_hcounteren,  
> write_hcounteren  },
> -[CSR_HGEIE]   = { "hgeie",   hmode,   read_hgeie,   
> write_hgeie   },
> -[CSR_HTVAL]   = { "htval",   hmode,   read_htval,   
> write_htval   },
> -[CSR_HTINST]  = { "htinst",  hmode,   read_htinst,  
> write_htinst  },
> -[CSR_HGEIP]   = { "hgeip",   hmode,   read_hgeip,   NULL 
>  },
> -[CSR_HGATP]   = { "hgatp",   hmode,   read_hgatp,   
> write_hgatp   },
> -[CSR_HTIMEDELTA]  = { "htimedelta",  hmode,   read_htimedelta,  
> write_htimedelta  },
> -[CSR_HTIMEDELTAH] = { "htimedeltah", hmode32, read_htimedeltah, 
> write_htimedeltah },
> -
> -[CSR_VSSTATUS]= { "vsstatus",hmode,   read_vsstatus,
> write_vsstatus},
> -[CSR_VSIP]= { "vsip",

Re: [PATCH v3 6/6] target/riscv: Enable privileged spec version 1.12

[Alistair Francis]

On Sun, Feb 6, 2022 at 7:51 PM Atish Patra  wrote:
>
> Virt machine uses privileged specification version 1.12 now.
> All other machine continue to use the default one defined for that
> machine unless changed to 1.12 by the user explicitly.
>
> Signed-off-by: Atish Patra 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.c | 8 +---
>  target/riscv/csr.c | 5 +
>  2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 2668f9c358b2..1c72dfffdc61 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -150,7 +150,7 @@ static void riscv_any_cpu_init(Object *obj)
>  #elif defined(TARGET_RISCV64)
>  set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
>  #endif
> -set_priv_version(env, PRIV_VERSION_1_11_0);
> +set_priv_version(env, PRIV_VERSION_1_12_0);
>  }
>
>  #if defined(TARGET_RISCV64)
> @@ -474,7 +474,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> **errp)
>  }
>
>  if (cpu->cfg.priv_spec) {
> -if (!g_strcmp0(cpu->cfg.priv_spec, "v1.11.0")) {
> +if (!g_strcmp0(cpu->cfg.priv_spec, "v1.12.0")) {
> +priv_version = PRIV_VERSION_1_12_0;
> +} else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.11.0")) {
>  priv_version = PRIV_VERSION_1_11_0;
>  } else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.10.0")) {
>  priv_version = PRIV_VERSION_1_10_0;
> @@ -489,7 +491,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> **errp)
>  if (priv_version) {
>  set_priv_version(env, priv_version);
>  } else if (!env->priv_ver) {
> -set_priv_version(env, PRIV_VERSION_1_11_0);
> +set_priv_version(env, PRIV_VERSION_1_12_0);
>  }
>
>  if (cpu->cfg.mmu) {
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index ff7e36596447..1c70c19cf9bd 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -2886,6 +2886,7 @@ static inline RISCVException 
> riscv_csrrw_check(CPURISCVState *env,
>  {
>  /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
>  int read_only = get_field(csrno, 0xC00) == 3;
> +int csr_min_priv = csr_ops[csrno].min_priv_ver;
>  #if !defined(CONFIG_USER_ONLY)
>  int effective_priv = env->priv;
>
> @@ -2918,6 +2919,10 @@ static inline RISCVException 
> riscv_csrrw_check(CPURISCVState *env,
>  return RISCV_EXCP_ILLEGAL_INST;
>  }
>
> +if (env->priv_ver < csr_min_priv) {
> +return RISCV_EXCP_ILLEGAL_INST;
> +}
> +
>  return csr_ops[csrno].predicate(env, csrno);
>  }
>
> --
> 2.30.2
>
>

Re: [PATCH v3 2/6] target/riscv: Add the privileged spec version 1.12.0

[Alistair Francis]

On Sun, Feb 6, 2022 at 7:37 PM Atish Patra  wrote:
>
> Add the definition for ratified privileged specification version v1.12
>
> Signed-off-by: Atish Patra 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index e5ff4c134c86..60b847141db2 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -86,6 +86,7 @@ enum {
>  enum {
>  PRIV_VERSION_1_10_0 = 0,
>  PRIV_VERSION_1_11_0,
> +PRIV_VERSION_1_12_0,
>  };
>
>  #define VEXT_VERSION_1_00_0 0x0001
> --
> 2.30.2
>
>

Re: [PATCH v3 1/6] target/riscv: Define simpler privileged spec version numbering

[Alistair Francis]

On Sun, Feb 6, 2022 at 7:26 PM Atish Patra  wrote:
>
> Currently, the privileged specification version are defined in
> a complex manner for no benefit.
>
> Simplify it by changing it to a simple enum based on.
>
> Suggested-by: Richard Henderson 
> Signed-off-by: Atish Patra 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.h | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 9d24d678e98a..e5ff4c134c86 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -82,8 +82,11 @@ enum {
>  RISCV_FEATURE_AIA
>  };
>
> -#define PRIV_VERSION_1_10_0 0x00011000
> -#define PRIV_VERSION_1_11_0 0x00011100
> +/* Privileged specification version */
> +enum {
> +PRIV_VERSION_1_10_0 = 0,
> +PRIV_VERSION_1_11_0,
> +};
>
>  #define VEXT_VERSION_1_00_0 0x0001
>
> --
> 2.30.2
>
>

Re: [PATCH v3 0/3] linux-user/ppc: Deliver SIGTRAP on tw[i]/td[i]

[Matheus K. Ferst]

Ping.

All patches reviewed and the series still applies to master with no 
conflicts.


On 13/01/2022 14:04, matheus.fe...@eldorado.org.br wrote:

From: Matheus Ferst 

In the review of 66c6b40aba1, Richard Henderson suggested[1] using
"trap" instead of ".long 0x0" to generate the signal to test XER
save/restore behavior. However, linux-user aborts when a trap
exception is raised, so we kept the patch with SIGILL.

This patch series is a follow-up to remove the cpu_abort call, deliver
SIGTRAP instead (using TRAP_BRKPT as si_code), and apply the suggestion
to the signal_save_restore_xer test.

The first patch removes the "qemu: fatal: Tried to call a TRAP" reported
in issue #588[2]. The third patch is an RFC to address the other logged
messages of "Unknown privilege violation (03)".

[1] https://lists.gnu.org/archive/html/qemu-ppc/2021-10/msg00143.html
[2] https://gitlab.com/qemu-project/qemu/-/issues/588

v3:
  - RFC to address the "Unknown privilege violation (03)" in #588.

v2:
  - Based-on rth's patch to use force_sig_fault and avoid merge conflicts

Matheus Ferst (3):
   linux-user/ppc: deliver SIGTRAP on POWERPC_EXCP_TRAP
   tests/tcg/ppc64le: change signal_save_restore_xer to use SIGTRAP
   target/ppc: Fix gen_priv_exception error value in mfspr/mtspr

  linux-user/ppc/cpu_loop.c   | 3 ++-
  target/ppc/translate.c  | 8 
  tests/tcg/ppc64le/signal_save_restore_xer.c | 8 
  3 files changed, 10 insertions(+), 9 deletions(-)

Re: [PATCH v2 0/3] qapi: Move RTC_CHANGE back out of target schema

[Eric Auger]

Hi Peter,

On 2/21/22 8:21 PM, Peter Maydell wrote:
> This patchset moves RTC_CHANGE back to misc.json, effectively
> reverting commit 183e4281a30962, which moved the RTC_CHANGE event to
> the target schema.  That change was an attempt to make the event
> target-specific to improve introspection, but the event isn't really
> target-specific: it's machine or device specific.  Putting RTC_CHANGE
> in the target schema with an ifdef list reduces maintainability (by
> adding an if: list with a long list of targets that needs to be
> manually updated as architectures are added or removed or as new
> devices gain the RTC_CHANGE functionality) and increases compile time
> (by preventing RTC devices which emit the event from being "compile
> once" rather than "compile once per target", because
> qapi-events-misc-target.h uses TARGET_* ifdefs, which are poisoned in
> "compile once" files.)
> 
> Patch 2 fixes some minor documentation issues for the RTC_CHANGE
> event, noticed during development of the patchset.
> 
> Patch 3 makes the pl031 a build-once file again, which was the
> initial motivation for the series.
> 
> Changes v1->v2:
>  * patch 1 needs to change the #include line for pl031.c, now that
>the change to make that device emit RTC_CHANGE has landed upstream
>  * patch 2 now also notes in the RTC_CHANGE documentation that
>not all devices/systems will emit the event (suggested by Markus)
>  * patch 3 is new

Reviewed-by: Eric Auger 

Thanks!

Eric
> 
> My default assumption is that this series will go in via the
> qapi tree; let me know if you'd prefer me to take it via
> target-arm instead.
> 
> thanks
> -- PMM
> 
> Peter Maydell (3):
>   qapi: Move RTC_CHANGE back out of target schema
>   qapi: Document some missing details of RTC_CHANGE event
>   hw/rtc: Compile pl031 once-only
> 
>  qapi/misc-target.json | 33 -
>  qapi/misc.json| 24 
>  hw/ppc/spapr_rtc.c|  2 +-
>  hw/rtc/mc146818rtc.c  |  2 +-
>  hw/rtc/pl031.c|  2 +-
>  hw/rtc/meson.build|  2 +-
>  6 files changed, 28 insertions(+), 37 deletions(-)
> 

Re: [PATCH v2 2/2] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table

[Liav Albani]

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index ebd47aa26f..5dc625b8d8 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -192,6 +192,11 @@ static void init_common_fadt_data(MachineState *ms, Object 
*o,
  .address = object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK, 
NULL)
  },
  };
+if (isa_check_device_existence("i8042")) {
+/* Indicates if i8042 is present or not */
+fadt.iapc_boot_arch = (1 << 1);
+}
+
Looking into this, it seems I messed up with the spaces here. So, I 
could fix this and send a v3, but want to see if there are other 
comments as well so we can get them fixed and this patch merged (or at 
least in a pull request) before the soft feature freeze in 8.3.2022 :)

Re: [PATCH v8 5/5] multifd: Implement zero copy write in multifd migration (multifd-zero-copy)

[Leonardo Bras Soares Passos]

On Fri, Feb 18, 2022 at 2:36 PM Juan Quintela  wrote:
>
> Leonardo Bras Soares Passos  wrote:
> > Hello Peter, thanks for reviewing!
> >
> > On Mon, Feb 7, 2022 at 11:22 PM Peter Xu  wrote:
> >>
> >> On Tue, Feb 01, 2022 at 03:29:03AM -0300, Leonardo Bras wrote:
> >> > -void multifd_send_sync_main(QEMUFile *f)
> >> > +int multifd_send_sync_main(QEMUFile *f)
> >> >  {
> >> >  int i;
> >> > +bool flush_zero_copy;
> >> >
> >> >  if (!migrate_use_multifd()) {
> >> > -return;
> >> > +return 0;
> >> >  }
> >> >  if (multifd_send_state->pages->num) {
> >> >  if (multifd_send_pages(f) < 0) {
> >> >  error_report("%s: multifd_send_pages fail", __func__);
> >> > -return;
> >> > +return 0;
> >>
> >> I've not checked how it used to do if multifd_send_pages() failed, but.. 
> >> should
> >> it returns -1 rather than 0 when there will be a return code?
> >
> > Yeah, that makes sense.
> > The point here is that I was trying not to modify much of the current 
> > behavior.
>
> if (qatomic_read(_send_state->exiting)) {
> return -1;
> }
>
> if (p->quit) {
> error_report("%s: channel %d has already quit!", __func__, i);
> qemu_mutex_unlock(>mutex);
> return -1;
> }
>
> This are the only two cases where the current code can return one
> error.  In the 1st case we are exiting, we are already in the middle of
> finishing, so we don't really care.
> In the second one, we have already quit, and error as already quite big.
>
> But I agree with both comments:
> - we need to improve the error paths
> - leonardo changes don't affect what is already there.
>



>
> > I mean, multifd_send_sync_main() would previously return void, so any
> > other errors would not matter to the caller of this function, which
> > will continue to run as if nothing happened.
> >
> > Now, if it fails with flush_zero_copy, the operation needs to be aborted.
> >
> > Maybe, I should make it different:
> > - In any error, return -1.
> > - Create/use a specific error code in the case of a failing
> > flush_zero_copy, so I can test the return value for it on the caller
> > function and return early.
>
> We need to add the check.  It don't matter if the problem is zero_copy
> or the existing one, we are under a minor catastrophe and migration has
> to be aborted.

Ok, I will fix that so we can abort in case of any error.
Maybe it's better to do that on a separated patch, before 5/5, right?

>
> > Or alternatively, the other errors could also return early, but since
> > this will change how the code currently works, I would probably need
> > another patch for that change. (so it can be easily reverted if
> > needed)
> >
> > What do you think is better?
> >
> >
> >> >  }
> >> >  }
> >> > +
> >> > +/*
> >> > + * When using zero-copy, it's necessary to flush after each 
> >> > iteration to
> >> > + * make sure pages from earlier iterations don't end up replacing 
> >> > newer
> >> > + * pages.
> >> > + */
> >> > +flush_zero_copy = migrate_use_zero_copy_send();
> >> > +
> >> >  for (i = 0; i < migrate_multifd_channels(); i++) {
> >> >  MultiFDSendParams *p = _send_state->params[i];
> >> >
> >> > @@ -591,7 +600,7 @@ void multifd_send_sync_main(QEMUFile *f)
> >> >  if (p->quit) {
> >> >  error_report("%s: channel %d has already quit", __func__, 
> >> > i);
> >> >  qemu_mutex_unlock(>mutex);
> >> > -return;
> >> > +return 0;
> >>
> >> Same question here.
> >
> > Please see above,
> >
> >>
> >> >  }
> >>
> >> The rest looks good.  Thanks,
>
> Later, Juan.
>

Thanks for the feedback!

Best regards,
Leo

Re: [PATCH] block/curl.c: Check error return from curl_easy_setopt()

[Peter Maydell]

On Tue, 1 Feb 2022 at 11:25, Hanna Reitz  wrote:
>
> On 28.01.22 17:55, Peter Maydell wrote:
> > Coverity points out that we aren't checking the return value
> > from curl_easy_setopt() for any of the calls to it we make
> > in block/curl.c.
> >
> > Fixes: Coverity CID 1459336, 1459482, 1460331
> > Signed-off-by: Peter Maydell 
> > ---
> > Big fat disclaimer: tested only with 'make check', which I suspect
> > may not be exercising this block backend. Hints on how to test
> > more thoroughly are welcome.
> >
> >   block/curl.c | 90 +---
> >   1 file changed, 58 insertions(+), 32 deletions(-)
>
> One problem I see in general is that most of the setopt functions are
> (indirectly) called from `curl_open()`, which is supposed to return an
> error message.  Its `out` label seems to expect some error description
> in `state->errmsg`.  The error handling here doesn’t set such a description.

Ah, yes, I hadn't noticed that -- it's a pre-existing bug, where
we do this:

if (curl_init_state(s, state) < 0) {
goto out;
}

and curl_init_state() already has an error path (for when curl_easy_init()
fails) that can take that goto without setting state->errmsg...

> Then again, there are enough existing error paths that don’t set this
> description either, so it isn’t quite this patch’s duty to fix that
> situation.

...as you've already noticed :-)

> I guess it would be nice if we had a wrapper for
> `curl_easy_setopt()` with an `Error **` parameter, so we could easily
> generate error messages that describe key and value (and then
> `curl_init_state()` should have an `Error **` parameter, too).
>
> But this patch doesn’t make anything worse than it already is, so that’d
> rather be an idea for future clean-up.
>
> > diff --git a/block/curl.c b/block/curl.c
> > index 6a6cd729758..aaee1b17bef 100644
> > --- a/block/curl.c
> > +++ b/block/curl.c
>
> [...]
>
> > @@ -879,7 +902,10 @@ static void curl_setup_preadv(BlockDriverState *bs, 
> > CURLAIOCB *acb)
> >
> >   snprintf(state->range, 127, "%" PRIu64 "-%" PRIu64, start, end);
> >   trace_curl_setup_preadv(acb->bytes, start, state->range);
> > -curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
> > +if (curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range)) {
> > +curl_clean_state(state);
> > +goto out;
>
> I think we need to mark the request as failed by setting `acb->ret` to a
> negative value (and probably also clear `state->acb[0]` like the error
> path below does).

OK; or I could roll the two curl calls into the same if:

if (curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range) ||
curl_multi_add_handle(s->multi, state->curl) != CURLM_OK) {
/* existing error handling and goto-out code */
}

thanks
-- PMM

Re: [PATCH v2] hw/i386: Improve bounds checking in OVMF table parsing

[Dr. David Alan Gilbert]

* Dov Murik (dovmu...@linux.ibm.com) wrote:
> When pc_system_parse_ovmf_flash() parses the optional GUIDed table in
> the end of the OVMF flash memory area, the table length field is checked
> for sizes that are too small, but doesn't error on sizes that are too
> big (bigger than the flash content itself).
> 
> Add a check for maximal size of the OVMF table, and add an error report
> in case the size is invalid.  In such a case, an error like this will be
> displayed during launch:
> 
> qemu-system-x86_64: OVMF table has invalid size 4047
> 
> and the table parsing is skipped.
> 
> Signed-off-by: Dov Murik 
> 
> ---
> 
> v2:
> - add error message example to commit description
> - replace magic numbers 48 and 50 with size calculations (thanks Phil MD)
> ---
>  hw/i386/pc_sysfw_ovmf.c | 20 
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i386/pc_sysfw_ovmf.c b/hw/i386/pc_sysfw_ovmf.c
> index f4dd92c588..1c9a16e9e6 100644
> --- a/hw/i386/pc_sysfw_ovmf.c
> +++ b/hw/i386/pc_sysfw_ovmf.c
> @@ -24,11 +24,14 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "hw/i386/pc.h"
>  #include "cpu.h"
>  
>  #define OVMF_TABLE_FOOTER_GUID "96b582de-1fb2-45f7-baea-a366c55a082d"
>  
> +static const int bytes_after_table_footer = 32;
> +
>  static bool ovmf_flash_parsed;
>  static uint8_t *ovmf_table;
>  static int ovmf_table_len;
> @@ -38,6 +41,8 @@ void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t 
> flash_size)
>  uint8_t *ptr;
>  QemuUUID guid;
>  int tot_len;
> +int max_tot_len = flash_size - (bytes_after_table_footer +
> +sizeof(guid) + sizeof(uint16_t));
>  
>  /* should only be called once */
>  if (ovmf_flash_parsed) {
> @@ -52,12 +57,13 @@ void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, 
> size_t flash_size)
>  
>  /*
>   * if this is OVMF there will be a table footer
> - * guid 48 bytes before the end of the flash file.  If it's
> - * not found, silently abort the flash parsing.
> + * guid 48 bytes before the end of the flash file
> + * (= 32 bytes after the table + 16 bytes the GUID itself).
> + * If it's not found, silently abort the flash parsing.
>   */
>  qemu_uuid_parse(OVMF_TABLE_FOOTER_GUID, );
>  guid = qemu_uuid_bswap(guid); /* guids are LE */
> -ptr = flash_ptr + flash_size - 48;
> +ptr = flash_ptr + flash_size - (bytes_after_table_footer + sizeof(guid));
>  if (!qemu_uuid_is_equal((QemuUUID *)ptr, )) {
>  return;
>  }
> @@ -66,7 +72,13 @@ void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t 
> flash_size)
>  ptr -= sizeof(uint16_t);
>  tot_len = le16_to_cpu(*(uint16_t *)ptr) - sizeof(guid) - 
> sizeof(uint16_t);

Instead of the max_tot_len calculation above, is it actually:
   max_tot_len = ptr - flash_ptr;

I think that works out the same and avoids doing the calculation in two
places; it's also logically what you have - you can't read over the
structure you just read.

Dave

> -if (tot_len <= 0) {
> +if (tot_len < 0 || tot_len > max_tot_len) {
> +error_report("OVMF table has invalid size %d", tot_len);
> +return;
> +}
> +
> +if (tot_len == 0) {
> +/* no entries in the OVMF table */
>  return;
>  }
>  
> 
> base-commit: 48033ad678ae2def43bf0d543a2c4c3d2a93feaf
> -- 
> 2.25.1
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH v8 5/5] multifd: Implement zero copy write in multifd migration (multifd-zero-copy)

[Leonardo Bras Soares Passos]

Hello Juan, thanks for the feedback!

On Fri, Feb 18, 2022 at 1:57 PM Juan Quintela  wrote:
>
> Leonardo Bras  wrote:
> > Implement zero copy send on nocomp_send_write(), by making use of QIOChannel
> > writev + flags & flush interface.
> >
> > Change multifd_send_sync_main() so flush_zero_copy() can be called
> > after each iteration in order to make sure all dirty pages are sent before
> > a new iteration is started. It will also flush at the beginning and at the
> > end of migration.
> >
> > Also make it return -1 if flush_zero_copy() fails, in order to cancel
> > the migration process, and avoid resuming the guest in the target host
> > without receiving all current RAM.
> >
> > This will work fine on RAM migration because the RAM pages are not usually 
> > freed,
> > and there is no problem on changing the pages content between 
> > writev_zero_copy() and
> > the actual sending of the buffer, because this change will dirty the page 
> > and
> > cause it to be re-sent on a next iteration anyway.
> >
> > A lot of locked memory may be needed in order to use multid migration
>^^
> multifd.
>
> I can fix it on the commit.

No worries, fixed for v9.

>
>
> > @@ -1479,7 +1479,16 @@ static bool migrate_params_check(MigrationParameters 
> > *params, Error **errp)
> >  error_prepend(errp, "Invalid mapping given for 
> > block-bitmap-mapping: ");
> >  return false;
> >  }
> > -
> > +#ifdef CONFIG_LINUX
> > +if (params->zero_copy_send &&
> > +(!migrate_use_multifd() ||
> > + params->multifd_compression != MULTIFD_COMPRESSION_NONE ||
> > + (params->tls_creds && *params->tls_creds))) {
> > +error_setg(errp,
> > +   "Zero copy only available for non-compressed non-TLS 
> > multifd migration");
> > +return false;
> > +}
> > +#endif
> >  return true;
> >  }
>
> Test is long, but it is exactly what we need.  Good.

Thanks!


>
>
> >
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index 43998ad117..2d68b9cf4f 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -568,19 +568,28 @@ void multifd_save_cleanup(void)
> >  multifd_send_state = NULL;
> >  }
> >
> > -void multifd_send_sync_main(QEMUFile *f)
> > +int multifd_send_sync_main(QEMUFile *f)
> >  {
> >  int i;
> > +bool flush_zero_copy;
> >
> >  if (!migrate_use_multifd()) {
> > -return;
> > +return 0;
> >  }
> >  if (multifd_send_state->pages->num) {
> >  if (multifd_send_pages(f) < 0) {
> >  error_report("%s: multifd_send_pages fail", __func__);
> > -return;
> > +return 0;
> >  }
> >  }
> > +
> > +/*
> > + * When using zero-copy, it's necessary to flush after each iteration 
> > to
> > + * make sure pages from earlier iterations don't end up replacing newer
> > + * pages.
> > + */
> > +flush_zero_copy = migrate_use_zero_copy_send();
> > +
> >  for (i = 0; i < migrate_multifd_channels(); i++) {
> >  MultiFDSendParams *p = _send_state->params[i];
> >
> > @@ -591,7 +600,7 @@ void multifd_send_sync_main(QEMUFile *f)
> >  if (p->quit) {
> >  error_report("%s: channel %d has already quit", __func__, i);
> >  qemu_mutex_unlock(>mutex);
> > -return;
> > +return 0;
> >  }
> >
> >  p->packet_num = multifd_send_state->packet_num++;
> > @@ -602,6 +611,17 @@ void multifd_send_sync_main(QEMUFile *f)
> >  ram_counters.transferred += p->packet_len;
> >  qemu_mutex_unlock(>mutex);
> >  qemu_sem_post(>sem);
> > +
> > +if (flush_zero_copy) {
> > +int ret;
> > +Error *err = NULL;
> > +
> > +ret = qio_channel_flush(p->c, );
> > +if (ret < 0) {
> > +error_report_err(err);
> > +return -1;
> > +}
> > +}
> >  }
> >  for (i = 0; i < migrate_multifd_channels(); i++) {
> >  MultiFDSendParams *p = _send_state->params[i];
> > @@ -610,6 +630,8 @@ void multifd_send_sync_main(QEMUFile *f)
> >  qemu_sem_wait(>sem_sync);
> >  }
> >  trace_multifd_send_sync_main(multifd_send_state->packet_num);
> > +
> > +return 0;
> >  }
>
> We are leaving pages is flight for potentially a lot of time. I *think*
> that we can sync shorter than that.
>
> >  static void *multifd_send_thread(void *opaque)
> > @@ -668,8 +690,8 @@ static void *multifd_send_thread(void *opaque)
> >  p->iov[0].iov_len = p->packet_len;
> >  p->iov[0].iov_base = p->packet;
> >
> > -ret = qio_channel_writev_all(p->c, p->iov, p->iovs_num,
> > - _err);
> > +ret = qio_channel_writev_full_all(p->c, p->iov, p->iovs_num, 
> > NULL,
> > +  0, p->write_flags, 
> > _err);
> >   

Re: [PATCH v2 2/3] qapi: Document some missing details of RTC_CHANGE event

[Philippe Mathieu-Daudé]

On 21/2/22 20:21, Peter Maydell wrote:

The RTC_CHANGE event's documentation is missing some details:
  * the offset argument is in units of seconds
  * it isn't guaranteed that the RTC will implement the event

Signed-off-by: Peter Maydell 
---
v1->v2: add the "RTC might not implement this" note
---
  qapi/misc.json | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 1/3] qapi: Move RTC_CHANGE back out of target schema

[Philippe Mathieu-Daudé]

On 21/2/22 20:21, Peter Maydell wrote:

This commit effectively reverts commit 183e4281a30962, which moved
the RTC_CHANGE event to the target schema.  That change was an
attempt to make the event target-specific to improve introspection,
but the event isn't really target-specific: it's machine or device
specific.  Putting RTC_CHANGE in the target schema with an ifdef list
reduces maintainability (by adding an if: list with a long list of
targets that needs to be manually updated as architectures are added
or removed or as new devices gain the RTC_CHANGE functionality) and
increases compile time (by preventing RTC devices which emit the
event from being "compile once" rather than "compile once per
target", because qapi-events-misc-target.h uses TARGET_* ifdefs,
which are poisoned in "compile once" files.)

Move RTC_CHANGE back to misc.json.

Signed-off-by: Peter Maydell 
Reviewed-by: Markus Armbruster 
Acked-by: Greg Kurz 
---
  qapi/misc-target.json | 33 -
  qapi/misc.json| 22 ++
  hw/ppc/spapr_rtc.c|  2 +-
  hw/rtc/mc146818rtc.c  |  2 +-
  hw/rtc/pl031.c|  2 +-
  5 files changed, 25 insertions(+), 36 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

[PATCH v2 0/3] qapi: Move RTC_CHANGE back out of target schema

[Peter Maydell]

This patchset moves RTC_CHANGE back to misc.json, effectively
reverting commit 183e4281a30962, which moved the RTC_CHANGE event to
the target schema.  That change was an attempt to make the event
target-specific to improve introspection, but the event isn't really
target-specific: it's machine or device specific.  Putting RTC_CHANGE
in the target schema with an ifdef list reduces maintainability (by
adding an if: list with a long list of targets that needs to be
manually updated as architectures are added or removed or as new
devices gain the RTC_CHANGE functionality) and increases compile time
(by preventing RTC devices which emit the event from being "compile
once" rather than "compile once per target", because
qapi-events-misc-target.h uses TARGET_* ifdefs, which are poisoned in
"compile once" files.)

Patch 2 fixes some minor documentation issues for the RTC_CHANGE
event, noticed during development of the patchset.

Patch 3 makes the pl031 a build-once file again, which was the
initial motivation for the series.

Changes v1->v2:
 * patch 1 needs to change the #include line for pl031.c, now that
   the change to make that device emit RTC_CHANGE has landed upstream
 * patch 2 now also notes in the RTC_CHANGE documentation that
   not all devices/systems will emit the event (suggested by Markus)
 * patch 3 is new

My default assumption is that this series will go in via the
qapi tree; let me know if you'd prefer me to take it via
target-arm instead.

thanks
-- PMM

Peter Maydell (3):
  qapi: Move RTC_CHANGE back out of target schema
  qapi: Document some missing details of RTC_CHANGE event
  hw/rtc: Compile pl031 once-only

 qapi/misc-target.json | 33 -
 qapi/misc.json| 24 
 hw/ppc/spapr_rtc.c|  2 +-
 hw/rtc/mc146818rtc.c  |  2 +-
 hw/rtc/pl031.c|  2 +-
 hw/rtc/meson.build|  2 +-
 6 files changed, 28 insertions(+), 37 deletions(-)

-- 
2.25.1

Re: [PATCH v2 3/3] hw/rtc: Compile pl031 once-only

[Philippe Mathieu-Daudé]

On 21/2/22 20:21, Peter Maydell wrote:

Now that the RTC_CHANGE event is no longer target-specific,
we can move the pl031 back to a compile-once source file
rather than a compile-per-target one.

Signed-off-by: Peter Maydell 
---
  hw/rtc/meson.build | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Philippe Mathieu-Daudé 

[PATCH v2 1/3] qapi: Move RTC_CHANGE back out of target schema

[Peter Maydell]

This commit effectively reverts commit 183e4281a30962, which moved
the RTC_CHANGE event to the target schema.  That change was an
attempt to make the event target-specific to improve introspection,
but the event isn't really target-specific: it's machine or device
specific.  Putting RTC_CHANGE in the target schema with an ifdef list
reduces maintainability (by adding an if: list with a long list of
targets that needs to be manually updated as architectures are added
or removed or as new devices gain the RTC_CHANGE functionality) and
increases compile time (by preventing RTC devices which emit the
event from being "compile once" rather than "compile once per
target", because qapi-events-misc-target.h uses TARGET_* ifdefs,
which are poisoned in "compile once" files.)

Move RTC_CHANGE back to misc.json.

Signed-off-by: Peter Maydell 
Reviewed-by: Markus Armbruster 
Acked-by: Greg Kurz 
---
 qapi/misc-target.json | 33 -
 qapi/misc.json| 22 ++
 hw/ppc/spapr_rtc.c|  2 +-
 hw/rtc/mc146818rtc.c  |  2 +-
 hw/rtc/pl031.c|  2 +-
 5 files changed, 25 insertions(+), 36 deletions(-)

diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index 4bc45d24741..036c5e4a914 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -2,39 +2,6 @@
 # vim: filetype=python
 #
 
-##
-# @RTC_CHANGE:
-#
-# Emitted when the guest changes the RTC time.
-#
-# @offset: offset between base RTC clock (as specified by -rtc base), and
-#  new RTC clock value
-#
-# Note: This event is rate-limited.
-#
-# Since: 0.13
-#
-# Example:
-#
-# <-   { "event": "RTC_CHANGE",
-#"data": { "offset": 78 },
-#"timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
-#
-##
-{ 'event': 'RTC_CHANGE',
-  'data': { 'offset': 'int' },
-  'if': { 'any': [ 'TARGET_ALPHA',
-   'TARGET_ARM',
-   'TARGET_HPPA',
-   'TARGET_I386',
-   'TARGET_MIPS',
-   'TARGET_MIPS64',
-   'TARGET_PPC',
-   'TARGET_PPC64',
-   'TARGET_S390X',
-   'TARGET_SH4',
-   'TARGET_SPARC' ] } }
-
 ##
 # @rtc-reset-reinjection:
 #
diff --git a/qapi/misc.json b/qapi/misc.json
index e8054f415b2..7a70eaa3ffc 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -527,3 +527,25 @@
  'data': { '*option': 'str' },
  'returns': ['CommandLineOptionInfo'],
  'allow-preconfig': true }
+
+##
+# @RTC_CHANGE:
+#
+# Emitted when the guest changes the RTC time.
+#
+# @offset: offset between base RTC clock (as specified by -rtc base), and
+#  new RTC clock value
+#
+# Note: This event is rate-limited.
+#
+# Since: 0.13
+#
+# Example:
+#
+# <-   { "event": "RTC_CHANGE",
+#"data": { "offset": 78 },
+#"timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
+#
+##
+{ 'event': 'RTC_CHANGE',
+  'data': { 'offset': 'int' } }
diff --git a/hw/ppc/spapr_rtc.c b/hw/ppc/spapr_rtc.c
index 94a5510e4eb..79677cf5503 100644
--- a/hw/ppc/spapr_rtc.c
+++ b/hw/ppc/spapr_rtc.c
@@ -32,7 +32,7 @@
 #include "hw/ppc/spapr.h"
 #include "migration/vmstate.h"
 #include "qapi/error.h"
-#include "qapi/qapi-events-misc-target.h"
+#include "qapi/qapi-events-misc.h"
 #include "qemu/cutils.h"
 #include "qemu/module.h"
 
diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index e61a0cced4c..57c514e15c5 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -40,7 +40,7 @@
 #include "hw/rtc/mc146818rtc_regs.h"
 #include "migration/vmstate.h"
 #include "qapi/error.h"
-#include "qapi/qapi-events-misc-target.h"
+#include "qapi/qapi-events-misc.h"
 #include "qapi/visitor.h"
 #include "hw/rtc/mc146818rtc_regs.h"
 
diff --git a/hw/rtc/pl031.c b/hw/rtc/pl031.c
index 38d9d3c2f38..60167c778f2 100644
--- a/hw/rtc/pl031.c
+++ b/hw/rtc/pl031.c
@@ -24,7 +24,7 @@
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "trace.h"
-#include "qapi/qapi-events-misc-target.h"
+#include "qapi/qapi-events-misc.h"
 
 #define RTC_DR  0x00/* Data read register */
 #define RTC_MR  0x04/* Match register */
-- 
2.25.1

[PATCH v2 3/3] hw/rtc: Compile pl031 once-only

[Peter Maydell]

Now that the RTC_CHANGE event is no longer target-specific,
we can move the pl031 back to a compile-once source file
rather than a compile-per-target one.

Signed-off-by: Peter Maydell 
---
 hw/rtc/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/rtc/meson.build b/hw/rtc/meson.build
index 8fd8d8f9a71..7cecdee5ddb 100644
--- a/hw/rtc/meson.build
+++ b/hw/rtc/meson.build
@@ -2,7 +2,7 @@
 softmmu_ss.add(when: 'CONFIG_DS1338', if_true: files('ds1338.c'))
 softmmu_ss.add(when: 'CONFIG_M41T80', if_true: files('m41t80.c'))
 softmmu_ss.add(when: 'CONFIG_M48T59', if_true: files('m48t59.c'))
-specific_ss.add(when: 'CONFIG_PL031', if_true: files('pl031.c'))
+softmmu_ss.add(when: 'CONFIG_PL031', if_true: files('pl031.c'))
 softmmu_ss.add(when: 'CONFIG_TWL92230', if_true: files('twl92230.c'))
 softmmu_ss.add(when: ['CONFIG_ISA_BUS', 'CONFIG_M48T59'], if_true: 
files('m48t59-isa.c'))
 softmmu_ss.add(when: 'CONFIG_XLNX_ZYNQMP', if_true: files('xlnx-zynqmp-rtc.c'))
-- 
2.25.1

[PATCH v2 2/2] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table

[Liav Albani]

This can allow the guest OS to determine more easily if i8042 controller
is present in the system or not, so it doesn't need to do probing of the
controller, but just initialize it immediately, before enumerating the
ACPI AML namespace.

Signed-off-by: Liav Albani 
---
 hw/acpi/aml-build.c | 7 ++-
 hw/i386/acpi-build.c| 5 +
 hw/i386/acpi-microvm.c  | 5 +
 include/hw/acpi/acpi-defs.h | 1 +
 4 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 8966e16320..ef5f4cad87 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2152,7 +2152,12 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const 
AcpiFadtData *f,
 build_append_int_noprefix(tbl, 0, 1); /* DAY_ALRM */
 build_append_int_noprefix(tbl, 0, 1); /* MON_ALRM */
 build_append_int_noprefix(tbl, f->rtc_century, 1); /* CENTURY */
-build_append_int_noprefix(tbl, 0, 2); /* IAPC_BOOT_ARCH */
+/* IAPC_BOOT_ARCH */
+if (f->rev == 1) {
+build_append_int_noprefix(tbl, 0, 2);
+} else {
+build_append_int_noprefix(tbl, f->iapc_boot_arch, 2);
+}
 build_append_int_noprefix(tbl, 0, 1); /* Reserved */
 build_append_int_noprefix(tbl, f->flags, 4); /* Flags */
 
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index ebd47aa26f..5dc625b8d8 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -192,6 +192,11 @@ static void init_common_fadt_data(MachineState *ms, Object 
*o,
 .address = object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK, NULL)
 },
 };
+if (isa_check_device_existence("i8042")) {
+/* Indicates if i8042 is present or not */
+fadt.iapc_boot_arch = (1 << 1);
+}
+
 *data = fadt;
 }
 
diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
index 68ca7e7fc2..756c69b3b0 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -189,6 +189,11 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
 .reset_val = ACPI_GED_RESET_VALUE,
 };
 
+if (isa_check_device_existence("i8042")) {
+/* Indicates if i8042 is present or not */
+pmfadt.iapc_boot_arch = (1 << 1);
+}
+
 table_offsets = g_array_new(false, true /* clear */,
 sizeof(uint32_t));
 bios_linker_loader_alloc(tables->linker,
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index c97e8633ad..2b42e4192b 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -77,6 +77,7 @@ typedef struct AcpiFadtData {
 uint16_t plvl2_lat;/* P_LVL2_LAT */
 uint16_t plvl3_lat;/* P_LVL3_LAT */
 uint16_t arm_boot_arch;/* ARM_BOOT_ARCH */
+uint16_t iapc_boot_arch;   /* IAPC_BOOT_ARCH */
 uint8_t minor_ver; /* FADT Minor Version */
 
 /*
-- 
2.35.1

[PATCH v2 2/3] qapi: Document some missing details of RTC_CHANGE event

[Peter Maydell]

The RTC_CHANGE event's documentation is missing some details:
 * the offset argument is in units of seconds
 * it isn't guaranteed that the RTC will implement the event

Signed-off-by: Peter Maydell 
---
v1->v2: add the "RTC might not implement this" note
---
 qapi/misc.json | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/qapi/misc.json b/qapi/misc.json
index 7a70eaa3ffc..0ab235e41f7 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -533,10 +533,12 @@
 #
 # Emitted when the guest changes the RTC time.
 #
-# @offset: offset between base RTC clock (as specified by -rtc base), and
-#  new RTC clock value
+# @offset: offset in seconds between base RTC clock (as specified
+#  by -rtc base), and new RTC clock value
 #
 # Note: This event is rate-limited.
+#   It is not guaranteed that the RTC in the system implements
+#   this event, or even that the system has an RTC at all.
 #
 # Since: 0.13
 #
-- 
2.25.1

[PATCH v2 1/2] hw/isa: add function to check for existence of device by its type

[Liav Albani]

This function enumerates all attached ISA devices in the machine, and
tries to compare a given device type name to the enumerated devices.
For example, this can help other code to determine if a i8042 controller
exists in the machine.

Signed-off-by: Liav Albani 
---
 hw/isa/isa-bus.c | 23 +++
 include/hw/isa/isa.h |  1 +
 2 files changed, 24 insertions(+)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 6c31398dda..663aa36d29 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -222,6 +222,29 @@ void isa_build_aml(ISABus *bus, Aml *scope)
 }
 }
 
+bool isa_check_device_existence(const char *typename)
+{
+/*
+ * If there's no ISA bus, we know for sure that the checked ISA device type
+ * doesn't exist in the machine.
+ */
+if (isabus == NULL) {
+return false;
+}
+
+BusChild *kid;
+ISADevice *dev;
+
+QTAILQ_FOREACH(kid, >parent_obj.children, sibling) {
+dev = ISA_DEVICE(kid->child);
+const char *object_type = object_get_typename(OBJECT(dev));
+if (object_type && strcmp(object_type, typename) == 0) {
+return true;
+}
+}
+return false;
+}
+
 static void isabus_dev_print(Monitor *mon, DeviceState *dev, int indent)
 {
 ISADevice *d = ISA_DEVICE(dev);
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index d4417b34b6..65f0c7e28c 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -99,6 +99,7 @@ IsaDma *isa_get_dma(ISABus *bus, int nchan);
 MemoryRegion *isa_address_space(ISADevice *dev);
 MemoryRegion *isa_address_space_io(ISADevice *dev);
 ISADevice *isa_new(const char *name);
+bool isa_check_device_existence(const char *typename);
 ISADevice *isa_try_new(const char *name);
 bool isa_realize_and_unref(ISADevice *dev, ISABus *bus, Error **errp);
 ISADevice *isa_create_simple(ISABus *bus, const char *name);
-- 
2.35.1

Re: [PULL 00/10] Misc next patches

[Daniel P . Berrangé]

On Fri, Feb 18, 2022 at 08:05:12PM +, Peter Maydell wrote:
> On Thu, 17 Feb 2022 at 12:01, Daniel P. Berrangé  wrote:
> >
> > The following changes since commit ad38520bdeb2b1e0b487db317f29119e94c1c88d:
> >
> >   Merge remote-tracking branch 
> > 'remotes/stefanha-gitlab/tags/block-pull-request' into staging (2022-02-15 
> > 19:30:33 +)
> >
> > are available in the Git repository at:
> >
> >   https://gitlab.com/berrange/qemu tags/misc-next-pull-request
> >
> > for you to fetch changes up to 2720ceda0521bc43139cfdf45e3e470559e11ce3:
> >
> >   docs: expand firmware descriptor to allow flash without NVRAM (2022-02-16 
> > 18:53:26 +)
> >
> > 
> > This misc series of changes:
> >
> >  - Improves documentation of SSH fingerprint checking
> >  - Fixes SHA256 fingerprints with non-blockdev usage
> >  - Blocks the clone3, setns, unshare & execveat syscalls
> >with seccomp
> >  - Blocks process spawning via clone syscall, but allows
> >threads, with seccomp
> >  - Takes over seccomp maintainer role
> >  - Expands firmware descriptor spec to allow flash
> >without NVRAM
> 
> Hi; this series seems to cause the x64-freebsd-13-build to fail:
> https://gitlab.com/qemu-project/qemu/-/jobs/2112237501
> 
> 1/1 qemu:block / qemu-iotests qcow2 ERROR 155.99s exit status 1
> ▶ 469/707 /or1k/qmp/x-query-opcount OK
> ▶ 493/707 /ppc64/pnv-xscom/cfam_id/POWER8NVL OK
> Summary of Failures:
> 1/1 qemu:block / qemu-iotests qcow2 ERROR 155.99s exit status 1
> Ok: 0
> Expected Fail: 0
> Fail: 1
> Unexpected Pass: 0
> Skipped: 0
> Timeout: 0
> Full log written to /tmp/cirrus-ci-build/build/meson-logs/iotestslog.txt
> 
> This is an allowed-to-fail job, so I could in theory allow the
> merge, but OTOH the job was passing previously and the failure
> is block-related and this is a block-related pullreq...

AFAIK, the block jobs run in CI don't cover the SSH driver at all.

I had a CI pipeline before submitting, which covered Free BSD 13
which passed. To be sure I just rebased to git master and tried
another pipeline which passed too:

  https://gitlab.com/berrange/qemu/-/jobs/2119118096

so I'm thinking the failure you got is a transient. Could you retry
this PULL

With 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 :|

[PATCH v2 0/2] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table

[Liav Albani]

This can allow the guest OS to determine more easily if i8042 controller
is present in the system or not, so it doesn't need to do probing of the
controller, but just initialize it immediately, before enumerating the
ACPI AML namespace.

To allow "flexible" indication, I don't hardcode the bit at location 1
as on in the IA-PC boot flags, but try to search for i8042 on the ISA
bus to verify it exists in the system.

Why this is useful you might ask - this patch allows the guest OS to
probe and use the i8042 controller without decoding the ACPI AML blob
at all. For example, as a developer of the SerenityOS kernel, I might
want to allow people to not try to decode the ACPI AML namespace (for
now, we still don't support ACPI AML as it's a work in progress), but
still to not probe for the i8042 but just use it after looking in the
IA-PC boot flags in the ACPI FADT table.

A note about this version of the patch series: I changed the assertion
checking if the ISA bus exists to a if statement, because I can see how
in the future someone might want to run a x86 machine without an ISA bus
so we should not assert if someone calls the ISA check device existence
function but return FALSE gracefully.
If someone thinks this is wrong, I'm more than happy to discuss and fix
the code :)

Liav Albani (2):
  hw/isa: add function to check for existence of device by its type
  hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT
table

 hw/acpi/aml-build.c |  7 ++-
 hw/i386/acpi-build.c|  5 +
 hw/i386/acpi-microvm.c  |  5 +
 hw/isa/isa-bus.c| 23 +++
 include/hw/acpi/acpi-defs.h |  1 +
 include/hw/isa/isa.h|  1 +
 6 files changed, 41 insertions(+), 1 deletion(-)

-- 
2.35.1

Re: [PATCH v2 2/2] hw/ide: add ich6 ide controller device emulation

[Liav Albani]

On 2/21/22 13:33, Gerd Hoffmann wrote:

   Hi,


ICH6 and ICH7 IDE controllers are quite the same as far as I know. I
could change it, but then one could argue that the name ich6-ide seems
like "ich9-ide", so not sure if we can really go on this path.

I think we don't actually have ich9-ide, we only have piix3, piix4 and ahci,
the latter is used by ich9. I just said that calling this new device
ich7-ide instead of ich6-ide would make it more clear it has nothing to do
with ich9.

Well, there actually is ich9-ide in physical hardware.  And it's quite
simliar for all ich6 -> ich9 (and possibly more) physical hardware.
I know, but I based it on Intel documentation and the ICH7 machine I 
have from 2009. Also, according to libata in Linux, the enum of 
piix_controller_ids include ich5, ich6 and ich8 (and much more, these 
are more relevant in the list for this reason), and by looking into what 
ich6_sata is being used for, I can see it matches the IDE controller PCI 
ID on the ICH7 machine I use, so that's another reason to choose this name.

The hardware implements both ide and sata.  Typically the bios setup
offers to pick ide or sata mode for the storage controller, and on boot
the chipset is configured accordingly by the firmware.
Yes, and then the BIOS configures the MAP register to indicate the setup 
that it was decided by the user in the BIOS configuration.
However, setting the MAP register to zero is a valid value - it 
indicates you have only SATA connectors in use (at least that's what 
Linux thinks), according to the Intel ICH5 Serial ATA Controller RPM.

qemu never bothered to implement ide mode for q35/ich9.  When a guest OS
is so old that it doesn't come with a sata driver there is the option to
just use the 'pc' machine type.  And usually that's the better choice
anyway because these old guests tend to have problems with other q35
components too.
That's true if you care about giving emulation only for the benefits of 
the guest (so you only care about supporting what the guest OS can 
expect from standard IDE controller, not edge cases), but my approach is 
looking at a very different goal.

So I'm wondering why you implement ich{6,7,9}-ide in the first place?
What is the use case / benefit?


I talked about it in the last patch about this topic I've sent (v1 to be 
precise), but let me describe it again :)
I'm a SerenityOS developer, as you might remember or not, I've talked to 
you (Gerd) in the past about SeaBIOS topics related to the OS off-list.
As I said before in this mail, I tend to test the SerenityOS kernel on 
the ICH7 machine I have from 2009. That machine has 4 SATA ports and you 
can connect 2 PATA devices (as one parallel cable can be used to connect 
two devices at once, to one connector on the mainboard).


I've seen that the kernel struggled to use the IDE controller - the main 
problem we have is long timeouts because of some problematic pattern in 
our code. However, on regular QEMU PC and Q35 machines everything boots 
fine. When I wrote this emulation component, I saw the same problem I 
had on the bare metal machine, so it is a convenient feature for me to 
debug this problem without having to use the bare metal machine - it 
helps saving lots of time for me by avoiding the need to compile a 
kernel, put it on the SATA harddrive and try to boot it in the rapid 
compile-boot-test cycle I have here.


I thought it might be beneficial for other OS developers and hobbyists 
like me to have such component. For now, it's an IDE 
ICH5/6/7/9-compatible controller, supporting only PCI IDE native mode - 
which means you can relocate the resources to anything you want on the 
IO space, so it's a legacy-free device in the sense of PCI bus resource 
management, but still a legacy device that to use it on bare metal you 
need a machine from late 2000s.


Also, I do see a point in expanding this controller with more features. 
For example, some ICH6 IDE controllers had AHCI mode within them, so you 
could actually enable the AHCI mode and disable IDE mode if you know 
what you're doing - you will probably need to assign the IDE PCI BARs 
correctly first if you want IDE mode in such controller, or ignore it 
and go with AHCI mode instead. Also, this emulation component is only 
about PCI IDE native mode currently, but we can easily put it that you 
can switch channels between compatibility mode and native mode if wanted 
to. My ICH7 test machine has such controller - it allows you to switch 
between the two modes, so the OS can decide what to do with the IDE 
controller according to its needs.



take care,
   Gerd


Best regards,
Liav

Re: [PATCH] analyze-migration.py: Fix instance_id signedness

[Philippe Mathieu-Daudé]

On 21/2/22 18:53, Fabiano Rosas wrote:

The instance_id field is uint32_t in the migration code, however the
script currently handles it as a signed integer:

$ ./scripts/analyze-migration.py -f mig
  Traceback (most recent call last):
File "./scripts/analyze-migration.py", line 605, in 
  dump.read(dump_memory = args.memory)
File "./scripts/analyze-migration.py", line 539, in read
  classdesc = self.section_classes[section_key]
  KeyError: ('spapr_iommu', -2147483648)

Signed-off-by: Fabiano Rosas 
---
  scripts/analyze-migration.py | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 17/20] migration: Postcopy preemption preparation on channel creation

[Dr. David Alan Gilbert]

* Peter Xu (pet...@redhat.com) wrote:
> Create a new socket for postcopy to be prepared to send postcopy requested
> pages via this specific channel, so as to not get blocked by precopy pages.
> 
> A new thread is also created on dest qemu to receive data from this new 
> channel
> based on the ram_load_postcopy() routine.
> 
> The ram_load_postcopy(POSTCOPY) branch and the thread has not started to
> function, and that'll be done in follow up patches.
> 
> Cleanup the new sockets on both src/dst QEMUs, meanwhile look after the new
> thread too to make sure it'll be recycled properly.
> 
> Signed-off-by: Peter Xu 
> ---
>  migration/migration.c| 62 
>  migration/migration.h|  8 
>  migration/postcopy-ram.c | 88 ++--
>  migration/postcopy-ram.h | 10 +
>  migration/ram.c  | 25 
>  migration/ram.h  |  4 +-
>  migration/socket.c   | 22 +-
>  migration/socket.h   |  1 +
>  migration/trace-events   |  3 ++
>  9 files changed, 203 insertions(+), 20 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 4c22bad304..3d7f897b72 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -321,6 +321,12 @@ void migration_incoming_state_destroy(void)
>  mis->page_requested = NULL;
>  }
>  
> +if (mis->postcopy_qemufile_dst) {
> +migration_ioc_unregister_yank_from_file(mis->postcopy_qemufile_dst);
> +qemu_fclose(mis->postcopy_qemufile_dst);
> +mis->postcopy_qemufile_dst = NULL;
> +}
> +
>  yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>  }
>  
> @@ -714,15 +720,21 @@ void migration_fd_process_incoming(QEMUFile *f, Error 
> **errp)
>  migration_incoming_process();
>  }
>  
> +static bool migration_needs_multiple_sockets(void)
> +{
> +return migrate_use_multifd() || migrate_postcopy_preempt();
> +}
> +
>  void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
>  {
>  MigrationIncomingState *mis = migration_incoming_get_current();
>  Error *local_err = NULL;
>  bool start_migration;
> +QEMUFile *f;
>  
>  if (!mis->from_src_file) {
>  /* The first connection (multifd may have multiple) */
> -QEMUFile *f = qemu_fopen_channel_input(ioc);
> +f = qemu_fopen_channel_input(ioc);
>  
>  if (!migration_incoming_setup(f, errp)) {
>  return;
> @@ -730,13 +742,18 @@ void migration_ioc_process_incoming(QIOChannel *ioc, 
> Error **errp)
>  
>  /*
>   * Common migration only needs one channel, so we can start
> - * right now.  Multifd needs more than one channel, we wait.
> + * right now.  Some features need more than one channel, we wait.
>   */
> -start_migration = !migrate_use_multifd();
> +start_migration = !migration_needs_multiple_sockets();
>  } else {
>  /* Multiple connections */
> -assert(migrate_use_multifd());
> -start_migration = multifd_recv_new_channel(ioc, _err);
> +assert(migration_needs_multiple_sockets());
> +if (migrate_use_multifd()) {
> +start_migration = multifd_recv_new_channel(ioc, _err);
> +} else if (migrate_postcopy_preempt()) {
> +f = qemu_fopen_channel_input(ioc);
> +start_migration = postcopy_preempt_new_channel(mis, f);
> +}
>  if (local_err) {
>  error_propagate(errp, local_err);
>  return;
> @@ -761,11 +778,20 @@ void migration_ioc_process_incoming(QIOChannel *ioc, 
> Error **errp)
>  bool migration_has_all_channels(void)
>  {
>  MigrationIncomingState *mis = migration_incoming_get_current();
> -bool all_channels;
>  
> -all_channels = multifd_recv_all_channels_created();
> +if (!mis->from_src_file) {
> +return false;
> +}
> +
> +if (migrate_use_multifd()) {
> +return multifd_recv_all_channels_created();
> +}
> +
> +if (migrate_postcopy_preempt()) {
> +return mis->postcopy_qemufile_dst != NULL;
> +}
>  
> -return all_channels && mis->from_src_file != NULL;
> +return true;
>  }
>  
>  /*
> @@ -1858,6 +1884,12 @@ static void migrate_fd_cleanup(MigrationState *s)
>  qemu_fclose(tmp);
>  }
>  
> +if (s->postcopy_qemufile_src) {
> +migration_ioc_unregister_yank_from_file(s->postcopy_qemufile_src);
> +qemu_fclose(s->postcopy_qemufile_src);
> +s->postcopy_qemufile_src = NULL;
> +}
> +
>  assert(!migration_is_active(s));
>  
>  if (s->state == MIGRATION_STATUS_CANCELLING) {
> @@ -3233,6 +3265,11 @@ static void migration_completion(MigrationState *s)
>  qemu_savevm_state_complete_postcopy(s->to_dst_file);
>  qemu_mutex_unlock_iothread();
>  
> +/* Shutdown the postcopy fast path thread */
> +if (migrate_postcopy_preempt()) {
> +postcopy_preempt_shutdown_file(s);
> +

Re: [PATCH 0/2] qapi: Move RTC_CHANGE back out of target schema

[Peter Maydell]

On Sat, 25 Sept 2021 at 08:44, Markus Armbruster  wrote:
>
> Peter Maydell  writes:
>
> > This patchset moves RTC_CHANGE back to misc.json, effectively
> > reverting commit 183e4281a30962, which moved the RTC_CHANGE event to
> > the target schema.  That change was an attempt to make the event
> > target-specific to improve introspection, but the event isn't really
> > target-specific: it's machine or device specific.  Putting RTC_CHANGE
> > in the target schema with an ifdef list reduces maintainability (by
> > adding an if: list with a long list of targets that needs to be
> > manually updated as architectures are added or removed or as new
> > devices gain the RTC_CHANGE functionality) and increases compile time
> > (by preventing RTC devices which emit the event from being "compile
> > once" rather than "compile once per target", because
> > qapi-events-misc-target.h uses TARGET_* ifdefs, which are poisoned in
> > "compile once" files.)
> >
> > Patch 2 fixes a minor documentation issue that I noticed while
> > I was doing this -- we didn't document that the units used in
> > the RTC_CHANGE event are seconds.
>
> Series
> Reviewed-by: Markus Armbruster 

I realized that this patchset never got applied -- I think I was
expecting it to be picked up via a QAPI related tree, and then
it was a bit close to a release to be put in, or something.
Anyway, I'm going to resend it in a moment.

> An additional patch documenting that not all RTCs implement RTC_CHANGE
> would be nice.  Listing them would be even nicer.

I disagree that listing them would be nice -- the whole point of
the series is to avoid having lists that get out of date when we
add a new RTC implementation or fix the missing-feature in an
existing one. I can add a sentence to the patch 2 docs change:
"Note that it is not guaranteed that the RTC in a system implements
this event, or even that the system has an RTC at all."

> An additional patch adding @qom-path event argument would be nice.

I don't understand what this would involve, so I'll leave it to you
if you want it.

thanks
-- PMM

[PATCH] analyze-migration.py: Fix instance_id signedness

[Fabiano Rosas]

The instance_id field is uint32_t in the migration code, however the
script currently handles it as a signed integer:

$ ./scripts/analyze-migration.py -f mig
 Traceback (most recent call last):
   File "./scripts/analyze-migration.py", line 605, in 
 dump.read(dump_memory = args.memory)
   File "./scripts/analyze-migration.py", line 539, in read
 classdesc = self.section_classes[section_key]
 KeyError: ('spapr_iommu', -2147483648)

Signed-off-by: Fabiano Rosas 
---
 scripts/analyze-migration.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
index b82a1b0c58..713f645eea 100755
--- a/scripts/analyze-migration.py
+++ b/scripts/analyze-migration.py
@@ -40,8 +40,8 @@ def __init__(self, filename):
 def read64(self):
 return int.from_bytes(self.file.read(8), byteorder='big', signed=True)
 
-def read32(self):
-return int.from_bytes(self.file.read(4), byteorder='big', signed=True)
+def read32(self, signed=True):
+return int.from_bytes(self.file.read(4), byteorder='big', 
signed=signed)
 
 def read16(self):
 return int.from_bytes(self.file.read(2), byteorder='big', signed=True)
@@ -533,7 +533,7 @@ def read(self, desc_only = False, dump_memory = False, 
write_memory = False):
 elif section_type == self.QEMU_VM_SECTION_START or section_type == 
self.QEMU_VM_SECTION_FULL:
 section_id = file.read32()
 name = file.readstr()
-instance_id = file.read32()
+instance_id = file.read32(signed=False)
 version_id = file.read32()
 section_key = (name, instance_id)
 classdesc = self.section_classes[section_key]
-- 
2.34.1

Re: [PATCH 0/2] Adds designware i2c module and adds it to virt arm

[Chris Rauer]

Hi Phil,

> What about using virtio-gpio & bitbang I2C?
>
> - virtio-gpio
> https://lore.kernel.org/qemu-devel/20201127182917.2387-5-i...@metux.net/
>
> - bitbang I2C already in: hw/i2c/bitbang_i2c.c
Sorry for the delay.

That looks like it might be doable with a bit more work creating the ACPI
entries for the bitbang I2C.  This definitely seems more appropriate on the
ARM_VIRT platform instead of putting a specific controller in.

For my uses, I will have to stick with the designware controller since it
matches the system I'm emulating a little more closely.  We can hold back
the designware stuff until another SoC platform is interested in using this
controller (since it seems like it is a common one).  Hopefully someone
will find another use for the controller patches someday.

Thanks again for looking at our patches.

-Chris


On Wed, Jan 26, 2022 at 3:42 PM Philippe Mathieu-Daudé 
wrote:

> +Enrico Weigelt
>
> On 26/1/22 19:03, Peter Maydell wrote:
> > On Wed, 26 Jan 2022 at 17:12, Chris Rauer  wrote:
> >>
> >>> I need to see a pretty strong justification for why we should be
> >>> adding new kinds of devices to the virt machine,
> >>
> >> The designware i2c controller is a very common controller on many
> >>   ARM SoCs.  It has device tree bindings and ACPI bindings which
> >> makes it ideal for this platform.
> >
> > No, I mean, why do we need an i2c controller on the virt
> > board at all ?
> >
> >>> Forgot to mention, but my prefered approach for providing
> >>> an i2c controller on the virt board would be to have a
> >>> PCI i2c controller: that way users who do need it can plug it
> >>> in with a -device command line option, and users who don't
> >>> need it never have to worry about it.
> >
> >>> (We seem to have an ICH9-SMB PCI device already; I have no idea if
> it's suitable.)
> >> I didn't find that device suitable because it is part of the Intel
> >> Southbridge, which may have some Intel platform quirks, and
> >> we don't need all the things in that IO hub.
> >
> > That's a pity. Is there a different PCI I2C controller we could model ?
>
> What about using virtio-gpio & bitbang I2C?
>
> - virtio-gpio
> https://lore.kernel.org/qemu-devel/20201127182917.2387-5-i...@metux.net/
>
> - bitbang I2C already in: hw/i2c/bitbang_i2c.c
>
> Regards,
>
> Phil.
>

Re: [PATCH] qapi, target/i386/sev: Add cpu0-id to query-sev-capabilities

[Dov Murik]

Thanks Daniel for reviewing.


On 21/02/2022 18:24, Daniel P. Berrangé wrote:
> On Mon, Feb 21, 2022 at 04:08:50PM +, Dov Murik wrote:
>> Add a new field 'cpu0-id' to the response of query-sev-capabilities
>> QMP command.  The value of the field is the hex-encoded 64-byte unique
>> ID of the CPU0 (socket 0), which can be used to retrieve the signed CEK
>> of the CPU from AMD's Key Distribution Service (KDS).
>>
>> Signed-off-by: Dov Murik 
>> ---
>>  qapi/misc-target.json |  4 
>>  target/i386/sev.c | 43 +++
>>  2 files changed, 47 insertions(+)
>>
>> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
>> index 4bc45d2474..d9b4991c86 100644
>> --- a/qapi/misc-target.json
>> +++ b/qapi/misc-target.json
>> @@ -177,6 +177,8 @@
>>  #
>>  # @cert-chain:  PDH certificate chain (base64 encoded)
>>  #
>> +# @cpu0-id: 64-byte unique ID of CPU0 (hex encoded) (since 7.0)
> 
> For binary data in QAPI we've pretty much standardized on using
> base64 encoding. I think we should stick with that encoding.
> 

OK, I'll change that to base64.

I thought about the cpu0-id as some kind of "address string", like mac
addresses or IPv6 addresses which are usually represented as hex strings
and not as base64-encoded.  But I guess that the AMD CPU unique ID
doesn't have the same legacy (and accepted notation) as mac addresses or
IPv6 addresses, so we might as well treat it as "regular" binary data.

Going with base64 also saves some code because QEMU doesn't have a
ready-made hex_encode() function (I copied mine from a static function
in crypto/hash.c).

-Dov


>> +#
>>  # @cbitpos: C-bit location in page table entry
>>  #
>>  # @reduced-phys-bits: Number of physical Address bit reduction when SEV is
>> @@ -187,6 +189,7 @@
>>  { 'struct': 'SevCapability',
>>'data': { 'pdh': 'str',
>>  'cert-chain': 'str',
>> +'cpu0-id': 'str',
>>  'cbitpos': 'int',
>>  'reduced-phys-bits': 'int'},
>>'if': 'TARGET_I386' }
>> @@ -205,6 +208,7 @@
>>  #
>>  # -> { "execute": "query-sev-capabilities" }
>>  # <- { "return": { "pdh": "8CCDD8DDD", "cert-chain": "888CCCDDDEE",
>> +#  "cpu0-id": "5ea2e1...90ea39",
>>  #  "cbitpos": 47, "reduced-phys-bits": 5}}
>>  #
>>  ##
> 
> Regards,
> Daniel

Re: [PATCH] qemu-options: fix incorrect description for '-drive index='

[Laurent Vivier]

Le 02/02/2022 à 15:34, Laurent Vivier a écrit :

qemu-options.hx contains grammar that a native English-speaking
person would never use.

Replace "This option defines where is connected the drive" by
"This option defines where the drive is connected".

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/853
Signed-off-by: Laurent Vivier 
---
  qemu-options.hx | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index ba3ae6a42aa3..094a6c1d7c28 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1377,7 +1377,7 @@ SRST
  the bus number and the unit id.
  
  ``index=index``

-This option defines where is connected the drive by using an
+This option defines where the drive is connected by using an
  index in the list of available connectors of a given interface
  type.
  


Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [RFC PATCH 3/3] tests/tcg/ppc64le: Use vector types instead of __int128

[Matheus K. Ferst]

On 17/02/2022 09:46, Matheus K. Ferst wrote:

On 17/02/2022 05:09, Cédric Le Goater wrote:

On 2/8/22 21:31, matheus.fe...@eldorado.org.br wrote:

From: Matheus Ferst 

LLVM/Clang doesn't like inline asm with __int128, use a vector type
instead.

Signed-off-by: Matheus Ferst 
---
Alternatively, we could pass VSR values in GPR pairs, as we did in
tests/tcg/ppc64le/non_signalling_xscv.c
---
  tests/tcg/ppc64le/bcdsub.c | 92 +-
  1 file changed, 52 insertions(+), 40 deletions(-)

diff --git a/tests/tcg/ppc64le/bcdsub.c b/tests/tcg/ppc64le/bcdsub.c
index 8c188cae6d..17403daf22 100644
--- a/tests/tcg/ppc64le/bcdsub.c
+++ b/tests/tcg/ppc64le/bcdsub.c
@@ -1,6 +1,7 @@
  #include 
  #include 
  #include 
+#include 

  #define CRF_LT  (1 << 3)
  #define CRF_GT  (1 << 2)
@@ -8,6 +9,16 @@
  #define CRF_SO  (1 << 0)
  #define UNDEF   0

+#ifdef __LITTLE_ENDIAN__


Shouldn't we be using :

#if BYTE_ORDER == LITTLE_ENDIAN

instead ?



I guess it is better, I'll send a v2.



Actually, it doesn't work for LLVM and needs endian.h for GCC[1]. This 
check is also used in sigbus and sha1 tests. The first shouldn't be a 
problem (allow_fail is zero for ppc), but sha1 gives the wrong result 
for BE:


$ ./qemu-ppc64le tests/tcg/ppc64le-linux-user/sha1
SHA1=15dd99a1991e0b3826fede3deffc1feba42278e6
$ ./qemu-ppc64 tests/tcg/ppc64-linux-user/sha1
SHA1=70f1d4d65eb47309ffacc5a28ff285ad826006da

and 'make check-tcg' succeeds anyway...

[1] https://godbolt.org/z/fYbzbbexn
--
Matheus K. Ferst
Instituto de Pesquisas ELDORADO 
Analista de Software
Aviso Legal - Disclaimer 

[PATCH v2] iotests: Write test output to TEST_DIR

[Hanna Reitz]

Drop the use of OUTPUT_DIR (test/qemu-iotests under the build
directory), and instead write test output files (.out.bad, .notrun, and
.casenotrun) to TEST_DIR.

With this, the same test can be run concurrently without the separate
instances interfering, because they will need separate TEST_DIRs anyway.
Running the same test separately is useful when running the iotests with
various format/protocol combinations in parallel, or when you just want
to aggressively exercise a single test (e.g. when it fails only
sporadically).

Putting this output into TEST_DIR means that it will stick around for
inspection after the test run is done (though running the same test in
the same TEST_DIR will overwrite it, just as it used to be); but given
that TEST_DIR is a scratch directory, it should be clear that users can
delete all of its content at any point.  (And if TEST_DIR is on tmpfs,
it will just disappear on shutdown.)  Contrarily, alternative approaches
that would put these output files into OUTPUT_DIR with some prefix to
differentiate between separate test runs might easily lead to cluttering
OUTPUT_DIR.

(This change means OUTPUT_DIR is no longer written to by the iotests, so
we can drop its usage altogether.)

Signed-off-by: Hanna Reitz 
---
v1: https://lists.nongnu.org/archive/html/qemu-block/2022-02/msg00675.html

v2:
- Delete .casenotrun before running a test: Writes to this file only
  append data, so if we do not delete it before a test run, it may still
  contain stale data from a previous run
- While at it, we might as well delete .notrun, because before this
  patch, all of .out.bad, .notrun, and .casenotrun are deleted.  (Really
  no need to delete .out.bad, though, given it is immediately
  overwritten after where we delete .notrun and .casenotrun.)
---
 tests/qemu-iotests/common.rc |  6 +++---
 tests/qemu-iotests/iotests.py|  5 ++---
 tests/qemu-iotests/testenv.py|  5 +
 tests/qemu-iotests/testrunner.py | 15 +--
 4 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 9885030b43..5bde2415dc 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -726,7 +726,7 @@ _img_info()
 #
 _notrun()
 {
-echo "$*" >"$OUTPUT_DIR/$seq.notrun"
+echo "$*" >"$TEST_DIR/$seq.notrun"
 echo "$seq not run: $*"
 status=0
 exit
@@ -738,14 +738,14 @@ _notrun()
 #
 _casenotrun()
 {
-echo "[case not run] $*" >>"$OUTPUT_DIR/$seq.casenotrun"
+echo "[case not run] $*" >>"$TEST_DIR/$seq.casenotrun"
 }
 
 # just plain bail out
 #
 _fail()
 {
-echo "$*" | tee -a "$OUTPUT_DIR/$seq.full"
+echo "$*" | tee -a "$TEST_DIR/$seq.full"
 echo "(see $seq.full for details)"
 status=1
 exit 1
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6ba65eb1ff..1d157d1325 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -84,7 +84,6 @@
 
 imgfmt = os.environ.get('IMGFMT', 'raw')
 imgproto = os.environ.get('IMGPROTO', 'file')
-output_dir = os.environ.get('OUTPUT_DIR', '.')
 
 try:
 test_dir = os.environ['TEST_DIR']
@@ -1209,7 +1208,7 @@ def notrun(reason):
 # Each test in qemu-iotests has a number ("seq")
 seq = os.path.basename(sys.argv[0])
 
-with open('%s/%s.notrun' % (output_dir, seq), 'w', encoding='utf-8') \
+with open('%s/%s.notrun' % (test_dir, seq), 'w', encoding='utf-8') \
 as outfile:
 outfile.write(reason + '\n')
 logger.warning("%s not run: %s", seq, reason)
@@ -1224,7 +1223,7 @@ def case_notrun(reason):
 # Each test in qemu-iotests has a number ("seq")
 seq = os.path.basename(sys.argv[0])
 
-with open('%s/%s.casenotrun' % (output_dir, seq), 'a', encoding='utf-8') \
+with open('%s/%s.casenotrun' % (test_dir, seq), 'a', encoding='utf-8') \
 as outfile:
 outfile.write('[case not run] ' + reason + '\n')
 
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 0f32897fe8..b11e943c8a 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -66,7 +66,7 @@ class TestEnv(ContextManager['TestEnv']):
 # pylint: disable=too-many-instance-attributes
 
 env_variables = ['PYTHONPATH', 'TEST_DIR', 'SOCK_DIR', 'SAMPLE_IMG_DIR',
- 'OUTPUT_DIR', 'PYTHON', 'QEMU_PROG', 'QEMU_IMG_PROG',
+ 'PYTHON', 'QEMU_PROG', 'QEMU_IMG_PROG',
  'QEMU_IO_PROG', 'QEMU_NBD_PROG', 'QSD_PROG',
  'QEMU_OPTIONS', 'QEMU_IMG_OPTIONS',
  'QEMU_IO_OPTIONS', 'QEMU_IO_OPTIONS_NO_FMT',
@@ -106,7 +106,6 @@ def init_directories(self) -> None:
  TEST_DIR
  SOCK_DIR
  SAMPLE_IMG_DIR
- OUTPUT_DIR
 """
 
 # Path where qemu goodies live in this source tree.
@@ -134,8 +133,6 @@ def init_directories(self) -> None:
 

Re: [PATCH 2/2] iotests/303: Check for zstd support

[Thomas Huth]

On 21/02/2022 18.08, Hanna Reitz wrote:

303 runs two test cases, one of which requires zstd support.
Unfortunately, given that this is not a unittest-style test, we cannot
easily skip that single case, and instead can only skip the whole test.

(Alternatively, we could split this test into a zlib and a zstd part,
but that seems excessive, given that this test is not in auto and thus
likely only run by developers who have zstd support compiled in.)

Fixes: 677e0bae686e7c670a71d1f ("iotest 303: explicit compression type")
Signed-off-by: Hanna Reitz 
---
  tests/qemu-iotests/303 | 15 +++
  1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303
index 16c2e10827..5a3efb4ba3 100755
--- a/tests/qemu-iotests/303
+++ b/tests/qemu-iotests/303
@@ -21,7 +21,8 @@
  
  import iotests

  import subprocess
-from iotests import qemu_img_create, qemu_io, file_path, log, filter_qemu_io
+from iotests import notrun, qemu_img_pipe_and_status, qemu_io, file_path, \
+log, filter_qemu_io
  
  iotests.script_initialize(supported_fmts=['qcow2'],

unsupported_imgopts=['refcount_bits', 'compat'])
@@ -55,9 +56,15 @@ def add_bitmap(num, begin, end, disabled):
  
  
  def test(compression_type: str, json_output: bool) -> None:

-qemu_img_create('-f', iotests.imgfmt,
-'-o', f'compression_type={compression_type}',
-disk, '10M')
+opts = f'compression_type={compression_type}'
+output, status = qemu_img_pipe_and_status('create',
+  '-f', iotests.imgfmt,
+  '-o', opts,
+  disk, '10M')
+if status == 1 and \
+"'compression-type' does not accept value 'zstd'" in output:
+notrun('zstd compression not supported')
+
  add_bitmap(1, 0, 6, False)
  add_bitmap(2, 6, 8, True)
  


303 was failing on my system, too, but I did not get around to investigate 
on that one yet - so thanks for fixing this pro-actively! Now the test is 
correctly skipped on my system.


Tested-by: Thomas Huth 

Re: [PATCH v5 1/2] semihosting/arm-compat: replace heuristic for softmmu SYS_HEAPINFO

[Peter Maydell]

On Mon, 21 Feb 2022 at 17:06, Alex Bennée  wrote:
>
>
> Peter Maydell  writes:
>
> > On Thu, 10 Feb 2022 at 11:30, Alex Bennée  wrote:
> >>
> >> The previous numbers were a guess at best and rather arbitrary without
> >> taking into account anything that might be loaded. Instead of using
> >> guesses based on the state of registers implement a new function that:
> >>
> >>  a) scans the MemoryRegions for the largest RAM block
> >>  b) iterates through all "ROM" blobs looking for the biggest gap
> >>
> >> The "ROM" blobs include all code loaded via -kernel and the various
> >> -device loader techniques.
> >>
> >> Signed-off-by: Alex Bennée 
> >> Cc: Andrew Strauss 
> >> Cc: Keith Packard 
> >> Message-Id: <20210601090715.22330-1-alex.ben...@linaro.org>
> >>
> >
> >
> >> +/*
> >> + * Sort into address order. We break ties between rom-startpoints
> >> + * and rom-endpoints in favour of the startpoint, by sorting the 0->1
> >> + * transition before the 1->0 transition. Either way round would
> >> + * work, but this way saves a little work later by avoiding
> >> + * dealing with "gaps" of 0 length.
> >> + */
> >> +static gint sort_secs(gconstpointer a, gconstpointer b)
> >> +{
> >> +RomSec *ra = (RomSec *) a;
> >> +RomSec *rb = (RomSec *) b;
> >> +
> >> +if (ra->base == rb->base) {
> >> +return ra->se - rb->se;
> >> +}
> >> +return ra->base > rb->base ? 1 : -1;
> >> +}
> >
> > This sort comparator still doesn't report the equality
> > case as actually equal.
>
> When ra->se and rb->se are the same it returns 0. Is that not what you want?

Oops, yes it does. I misread it because I was expecting it to be
structured differently. (AFAIK there is no "standard" way to
structure a comparator-function that works with multiple fields,
so the way you have it is fine.)

-- PMM

Re: [PULL v2 00/25] target-arm queue

[Peter Maydell]

On Mon, 21 Feb 2022 at 13:31, Peter Maydell  wrote:
>
> v2: drop npcm7xx sdhci tests: new tests assert on some platforms.
>
> -- PMM
>
> The following changes since commit e670f6d825d4dee248b311197fd4048469d6772b:
>
>   Merge remote-tracking branch 'remotes/legoater/tags/pull-ppc-20220218' into 
> staging (2022-02-20 15:05:41 +)
>
> are available in the Git repository at:
>
>   https://git.linaro.org/people/pmaydell/qemu-arm.git 
> tags/pull-target-arm-20220221-1
>
> for you to fetch changes up to ca511604925eef8572e22ecbf0d3c758d7277924:
>
>   ui/cocoa: Fix the leak of qemu_console_get_label (2022-02-21 13:30:21 +)
>
> 
> arm, cocoa and misc:
>  * MAINTAINERS file updates
>  * Mark remaining global TypeInfo instances as const
>  * checkpatch: Ensure that TypeInfos are const
>  * arm hvf: Handle unknown ID registers as RES0
>  * Make KVM -cpu max exactly like -cpu host
>  * Fix '-cpu max' for HVF
>  * Support PAuth extension for hvf
>  * Kconfig: Add I2C_DEVICES device group
>  * Kconfig: Add 'imply I2C_DEVICES' on boards with available i2c bus
>  * hw/arm/armv7m: Handle disconnected clock inputs
>  * osdep.h: pull out various things into new header files
>  * hw/timer: fix a9gtimer vmstate
>  * hw/arm: add initial mori-bmc board
>  * ui/cocoa: Remove allowedFileTypes restriction in SavePanel
>  * ui/cocoa: Do not alert even without block devices
>  * ui/cocoa: Fix the leak of qemu_console_get_label


Applied, thanks.

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

-- PMM

Re: [PATCH 17/31] vdpa: adapt vhost_ops callbacks to svq

[Eugenio Perez Martin]

On Mon, Feb 21, 2022 at 8:15 AM Jason Wang  wrote:
>
>
> 在 2022/2/18 上午1:13, Eugenio Perez Martin 写道:
> > On Tue, Feb 8, 2022 at 4:58 AM Jason Wang  wrote:
> >>
> >> 在 2022/2/1 上午2:58, Eugenio Perez Martin 写道:
> >>> On Sun, Jan 30, 2022 at 5:03 AM Jason Wang  wrote:
>  在 2022/1/22 上午4:27, Eugenio Pérez 写道:
> > First half of the buffers forwarding part, preparing vhost-vdpa
> > callbacks to SVQ to offer it. QEMU cannot enable it at this moment, so
> > this is effectively dead code at the moment, but it helps to reduce
> > patch size.
> >
> > Signed-off-by: Eugenio Pérez 
> > ---
> > hw/virtio/vhost-shadow-virtqueue.h |   2 +-
> > hw/virtio/vhost-shadow-virtqueue.c |  21 -
> > hw/virtio/vhost-vdpa.c | 133 
> > ++---
> > 3 files changed, 143 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.h 
> > b/hw/virtio/vhost-shadow-virtqueue.h
> > index 035207a469..39aef5ffdf 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.h
> > +++ b/hw/virtio/vhost-shadow-virtqueue.h
> > @@ -35,7 +35,7 @@ size_t vhost_svq_device_area_size(const 
> > VhostShadowVirtqueue *svq);
> >
> > void vhost_svq_stop(VhostShadowVirtqueue *svq);
> >
> > -VhostShadowVirtqueue *vhost_svq_new(void);
> > +VhostShadowVirtqueue *vhost_svq_new(uint16_t qsize);
> >
> > void vhost_svq_free(VhostShadowVirtqueue *vq);
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.c 
> > b/hw/virtio/vhost-shadow-virtqueue.c
> > index f129ec8395..7c168075d7 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > @@ -277,9 +277,17 @@ void vhost_svq_stop(VhostShadowVirtqueue *svq)
> > /**
> >  * Creates vhost shadow virtqueue, and instruct vhost device to use 
> > the shadow
> >  * methods and file descriptors.
> > + *
> > + * @qsize Shadow VirtQueue size
> > + *
> > + * Returns the new virtqueue or NULL.
> > + *
> > + * In case of error, reason is reported through error_report.
> >  */
> > -VhostShadowVirtqueue *vhost_svq_new(void)
> > +VhostShadowVirtqueue *vhost_svq_new(uint16_t qsize)
> > {
> > +size_t desc_size = sizeof(vring_desc_t) * qsize;
> > +size_t device_size, driver_size;
> > g_autofree VhostShadowVirtqueue *svq = 
> > g_new0(VhostShadowVirtqueue, 1);
> > int r;
> >
> > @@ -300,6 +308,15 @@ VhostShadowVirtqueue *vhost_svq_new(void)
> > /* Placeholder descriptor, it should be deleted at set_kick_fd 
> > */
> > event_notifier_init_fd(>svq_kick, INVALID_SVQ_KICK_FD);
> >
> > +svq->vring.num = qsize;
>  I wonder if this is the best. E.g some hardware can support up to 32K
>  queue size. So this will probably end up with:
> 
>  1) SVQ use 32K queue size
>  2) hardware queue uses 256
> 
> >>> In that case SVQ vring queue size will be 32K and guest's vring can
> >>> negotiate any number with SVQ equal or less than 32K,
> >>
> >> Sorry for being unclear what I meant is actually
> >>
> >> 1) SVQ uses 32K queue size
> >>
> >> 2) guest vq uses 256
> >>
> >> This looks like a burden that needs extra logic and may damage the
> >> performance.
> >>
> > Still not getting this point.
> >
> > An available guest buffer, although contiguous in GPA/GVA, can expand
> > in multiple buffers if it's not contiguous in qemu's VA (by the while
> > loop in virtqueue_map_desc [1]). In that scenario it is better to have
> > "plenty" of SVQ buffers.
>
>
> Yes, but this case should be rare. So in this case we should deal with
> overrun on SVQ, that is
>
> 1) SVQ is full
> 2) guest VQ isn't
>
> We need to
>
> 1) check the available buffer slots
> 2) disable guest kick and wait for the used buffers
>
> But it looks to me the current code is not ready for dealing with this case?
>

Yes it deals, that's the meaning of svq->next_guest_avail_elem.

>
> >
> > I'm ok if we decide to put an upper limit though, or if we decide not
> > to handle this situation. But we would leave out valid virtio drivers.
> > Maybe to set a fixed upper limit (1024?)? To add another parameter
> > (x-svq-size-n=N)?
> >
> > If you mean we lose performance because memory gets more sparse I
> > think the only possibility is to limit that way.
>
>
> If guest is not using 32K, having a 32K for svq may gives extra stress
> on the cache since we will end up with a pretty large working set.
>

That might be true. My guess is that it should not matter, since SVQ
and the guest's vring will have the same numbers of scattered buffers
and the avail / used / packed ring will be consumed more or less
sequentially. But I haven't tested.

I think it's better to add an upper limit (either fixed or in the
qemu's backend's cmdline) later if we see that this is a 

Re: [PATCH] target/avr: Correct AVRCPUClass docstring

[Laurent Vivier]

Le 22/01/2022 à 01:10, Philippe Mathieu-Daudé via a écrit :

There is no 'vr' field in AVRCPUClass.

Likely a copy/paste typo from CRISCPUClass ;)

Signed-off-by: Philippe Mathieu-Daudé 
---
  target/avr/cpu-qom.h | 1 -
  1 file changed, 1 deletion(-)

diff --git a/target/avr/cpu-qom.h b/target/avr/cpu-qom.h
index 9fa6989c18..14e5b3ce72 100644
--- a/target/avr/cpu-qom.h
+++ b/target/avr/cpu-qom.h
@@ -33,7 +33,6 @@ OBJECT_DECLARE_TYPE(AVRCPU, AVRCPUClass,
   *  AVRCPUClass:
   *  @parent_realize: The parent class' realize handler.
   *  @parent_reset: The parent class' reset handler.
- *  @vr: Version Register value.
   *
   *  A AVR CPU model.
   */


Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [PATCH] hw/nvram: use at24 macro

[Laurent Vivier]

Le 25/01/2022 à 10:20, Paolo Bonzini a écrit :

On 1/19/22 22:43, Patrick Venture wrote:

Use the macro for going from I2CSlave to EEPROMState.

Signed-off-by: Patrick Venture 
---
  hw/nvram/eeprom_at24c.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index af6f5dbb99..da435500ba 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -54,7 +54,7 @@ struct EEPROMState {
  static
  int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
  {
-    EEPROMState *ee = container_of(s, EEPROMState, parent_obj);
+    EEPROMState *ee = AT24C_EE(s);
  switch (event) {
  case I2C_START_SEND:


Cc: qemu-triv...@nongnu.org



Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [PATCH 1/2] iotests/065: Check for zstd support

[Thomas Huth]

On 21/02/2022 18.08, Hanna Reitz wrote:

Some test cases run in iotest 065 require zstd support.  Skip them if
qemu-img reports it not to be available.

Reported-by: Thomas Huth 
Fixes: 12a936171d71f839dc907ff ("iotest 065: explicit compression type")
Signed-off-by: Hanna Reitz 
---
  tests/qemu-iotests/065 | 11 ---
  1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
index f7c1b68dad..b68df84642 100755
--- a/tests/qemu-iotests/065
+++ b/tests/qemu-iotests/065
@@ -24,7 +24,7 @@ import os
  import re
  import json
  import iotests
-from iotests import qemu_img, qemu_img_pipe
+from iotests import qemu_img_pipe, qemu_img_pipe_and_status
  import unittest
  
  test_img = os.path.join(iotests.test_dir, 'test.img')

@@ -35,8 +35,13 @@ class TestImageInfoSpecific(iotests.QMPTestCase):
  def setUp(self):
  if self.img_options is None:
  self.skipTest('Skipping abstract test class')
-qemu_img('create', '-f', iotests.imgfmt, '-o', self.img_options,
- test_img, '128K')
+output, status = qemu_img_pipe_and_status('create',
+  '-f', iotests.imgfmt,
+  '-o', self.img_options,
+  test_img, '128K')
+if status == 1 and \
+"'compression-type' does not accept value 'zstd'" in output:
+self.case_skip('zstd compression not supported')
  
  def tearDown(self):

  os.remove(test_img)


Thanks, that fixes 065 for me!

Tested-by: Thomas Huth 

Re: [PATCH v2] target/rx: Remove unused ENV_OFFSET definition

[Laurent Vivier]

Le 03/02/2022 à 01:12, Philippe Mathieu-Daudé via a écrit :

The last use of ENV_OFFSET was removed in 5e1401969b
("cpu: Move icount_decr to CPUNegativeOffsetState");
the commit of target/rx came in just afterward.

Reviewed-by: Richard Henderson 
Reviewed-by: Yoshinori Sato 
Signed-off-by: Philippe Mathieu-Daudé 
---
  target/rx/cpu.h | 2 --
  1 file changed, 2 deletions(-)

diff --git a/target/rx/cpu.h b/target/rx/cpu.h
index 657db84ef0..58adf9edf6 100644
--- a/target/rx/cpu.h
+++ b/target/rx/cpu.h
@@ -116,8 +116,6 @@ struct RXCPU {
  
  typedef RXCPU ArchCPU;
  
-#define ENV_OFFSET offsetof(RXCPU, env)

-
  #define RX_CPU_TYPE_SUFFIX "-" TYPE_RX_CPU
  #define RX_CPU_TYPE_NAME(model) model RX_CPU_TYPE_SUFFIX
  #define CPU_RESOLVING_TYPE TYPE_RX_CPU


Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [PATCH] configure: Disable capstone and slirp in the --without-default-features mode

[Laurent Vivier]

Le 21/02/2022 à 10:06, Thomas Huth a écrit :

For the users, it looks a little bit weird that capstone and slirp are
not disabled automatically if they run the configure script with the
"--without-default-features" option, so let's do that now.
Note: fdt is *not* changed accordingly since this affects the targets
that we can build, so disabling fdt automatically here might have
unexpected side-effects for the users.

Signed-off-by: Thomas Huth 
---
  I thought I sent out that patch a couple of weeks ago already, but
  I cannot find it in the archives, so I likely missed to send it
  correctly. Anyway, sorry if you've got this twice!

  configure | 9 +++--
  1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 3a29eff5cc..36d10d95bb 100755
--- a/configure
+++ b/configure
@@ -361,9 +361,14 @@ slirp_smbd="$default_feature"
  # are included in the automatically generated help message)
  
  # 1. Track which submodules are needed

-capstone="auto"
+if test "$default_feature" = no ; then
+  capstone="disabled"
+  slirp="disabled"
+else
+  capstone="auto"
+  slirp="auto"
+fi
  fdt="auto"
-slirp="auto"
  
  # 2. Support --with/--without option

  default_devices="true"


Applied to my trivial-patches branch.

Thanks,
Laurent

[PATCH 0/3] util/thread-pool: Expose minimun and maximum size

[Nicolas Saenz Julienne]

As discussed on the previous RFC[1] the thread-pool's dynamic thread
management doesn't play well with real-time and latency sensitive
systems. This series introduces a set of controls that'll permit
achieving more deterministic behaviours, for example by fixing the
pool's size.

We first introduce a new common interface to event loop configuration by
moving iothread's already available properties into an abstract class
called 'EventLooopBackend' and have both 'IOThread' and the newly
created 'MainLoop' inherit the properties from that class.

With this new configuration interface in place it's relatively simple to
introduce new options to fix the even loop's thread pool sizes. The
resulting QAPI looks like this:

-object main-loop,id=main-loop,thread-pool-min=1,thread-pool-max=1

Note that all patches are bisect friendly and pass all the tests.

[1] 
https://patchwork.ozlabs.org/project/qemu-devel/patch/20220202175234.656711-1-nsaen...@redhat.com/

---

Nicolas Saenz Julienne (3):
  util & iothread: Introduce event-loop abstract class
  util/main-loop: Introduce the main loop into QOM
  util/event-loop: Introduce options to set the thread pool size

 MAINTAINERS   |   1 +
 include/block/aio.h   |  11 +++
 include/qemu/main-loop.h  |  11 +++
 include/sysemu/iothread.h |  11 +--
 iothread.c| 171 --
 qapi/qom.json |  14 ++--
 qga/meson.build   |   2 +-
 qom/meson.build   |   1 +
 tests/unit/meson.build|  10 +--
 util/async.c  |   3 +
 util/event-loop.c | 168 +
 util/event-loop.h |  45 ++
 util/main-loop.c  |  56 +
 util/thread-pool.c|  41 -
 14 files changed, 368 insertions(+), 177 deletions(-)
 create mode 100644 util/event-loop.c
 create mode 100644 util/event-loop.h

-- 
2.35.1

[PATCH 1/3] util & iothread: Introduce event-loop abstract class

[Nicolas Saenz Julienne]

Introduce the 'event-loop-backend' abstract class, it'll hold the
properties common to all event loops and provide the necessary hooks for
their creation. Then have 'IOThread' inherit from it. The class is
defined as user creatable and provides a hook for its children to attach
themselves to the user creatable class 'complete' function.

The new 'event-loop-backend' class will live in the util directory, and
will be packed into the qom static library.

No functional changes intended.

Signed-off-by: Nicolas Saenz Julienne 
---
 MAINTAINERS   |   1 +
 include/sysemu/iothread.h |  11 +--
 iothread.c| 171 --
 qom/meson.build   |   1 +
 util/event-loop.c | 142 +++
 util/event-loop.h |  40 +
 6 files changed, 204 insertions(+), 162 deletions(-)
 create mode 100644 util/event-loop.c
 create mode 100644 util/event-loop.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 4b3ae2ab08..e5ffbea449 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2670,6 +2670,7 @@ F: include/sysemu/runstate.h
 F: include/sysemu/runstate-action.h
 F: util/main-loop.c
 F: util/qemu-timer.c
+F: util/event-loop*
 F: softmmu/vl.c
 F: softmmu/main.c
 F: softmmu/cpus.c
diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
index 7f714bd136..aafef546b5 100644
--- a/include/sysemu/iothread.h
+++ b/include/sysemu/iothread.h
@@ -17,11 +17,12 @@
 #include "block/aio.h"
 #include "qemu/thread.h"
 #include "qom/object.h"
+#include "util/event-loop.h"
 
 #define TYPE_IOTHREAD "iothread"
 
 struct IOThread {
-Object parent_obj;
+EventLoopBackend parent_obj;
 
 QemuThread thread;
 AioContext *ctx;
@@ -32,14 +33,6 @@ struct IOThread {
 bool stopping;  /* has iothread_stop() been called? */
 bool running;   /* should iothread_run() continue? */
 int thread_id;
-
-/* AioContext poll parameters */
-int64_t poll_max_ns;
-int64_t poll_grow;
-int64_t poll_shrink;
-
-/* AioContext AIO engine parameters */
-int64_t aio_max_batch;
 };
 typedef struct IOThread IOThread;
 
diff --git a/iothread.c b/iothread.c
index 0f98af0f2a..fca5ee8f4c 100644
--- a/iothread.c
+++ b/iothread.c
@@ -23,22 +23,13 @@
 #include "qemu/error-report.h"
 #include "qemu/rcu.h"
 #include "qemu/main-loop.h"
+#include "util/event-loop.h"
 
 typedef ObjectClass IOThreadClass;
 
 DECLARE_CLASS_CHECKERS(IOThreadClass, IOTHREAD,
TYPE_IOTHREAD)
 
-#ifdef CONFIG_POSIX
-/* Benchmark results from 2016 on NVMe SSD drives show max polling times around
- * 16-32 microseconds yield IOPS improvements for both iodepth=1 and iodepth=32
- * workloads.
- */
-#define IOTHREAD_POLL_MAX_NS_DEFAULT 32768ULL
-#else
-#define IOTHREAD_POLL_MAX_NS_DEFAULT 0ULL
-#endif
-
 static void *iothread_run(void *opaque)
 {
 IOThread *iothread = opaque;
@@ -105,7 +96,6 @@ static void iothread_instance_init(Object *obj)
 {
 IOThread *iothread = IOTHREAD(obj);
 
-iothread->poll_max_ns = IOTHREAD_POLL_MAX_NS_DEFAULT;
 iothread->thread_id = -1;
 qemu_sem_init(>init_done_sem, 0);
 /* By default, we don't run gcontext */
@@ -152,28 +142,24 @@ static void iothread_init_gcontext(IOThread *iothread)
 iothread->main_loop = g_main_loop_new(iothread->worker_context, TRUE);
 }
 
-static void iothread_set_aio_context_params(IOThread *iothread, Error **errp)
+static void iothread_set_aio_context_params(EventLoopBackend *bc, Error **errp)
 {
+IOThread *iothread = IOTHREAD(bc);
 ERRP_GUARD();
 
-aio_context_set_poll_params(iothread->ctx,
-iothread->poll_max_ns,
-iothread->poll_grow,
-iothread->poll_shrink,
-errp);
+aio_context_set_poll_params(iothread->ctx, bc->poll_max_ns, bc->poll_grow,
+bc->poll_shrink, errp);
 if (*errp) {
 return;
 }
 
-aio_context_set_aio_params(iothread->ctx,
-   iothread->aio_max_batch,
-   errp);
+aio_context_set_aio_params(iothread->ctx, bc->aio_max_batch, errp);
 }
 
-static void iothread_complete(UserCreatable *obj, Error **errp)
+static void iothread_init(EventLoopBackend *bc, Error **errp)
 {
 Error *local_error = NULL;
-IOThread *iothread = IOTHREAD(obj);
+IOThread *iothread = IOTHREAD(bc);
 char *thread_name;
 
 iothread->stopping = false;
@@ -189,7 +175,7 @@ static void iothread_complete(UserCreatable *obj, Error 
**errp)
  */
 iothread_init_gcontext(iothread);
 
-iothread_set_aio_context_params(iothread, _error);
+iothread_set_aio_context_params(bc, _error);
 if (local_error) {
 error_propagate(errp, local_error);
 aio_context_unref(iothread->ctx);
@@ -201,7 +187,7 @@ static void iothread_complete(UserCreatable *obj, Error 
**errp)
  * to 

[PATCH 3/3] util/event-loop: Introduce options to set the thread pool size

[Nicolas Saenz Julienne]

The thread pool regulates itself: when idle, it kills threads until
empty, when in demand, it creates new threads until full. This behaviour
doesn't play well with latency sensitive workloads where the price of
creating a new thread is too high. For example, when paired with qemu's
'-mlock', or using safety features like SafeStack, creating a new thread
has been measured take multiple milliseconds.

In order to mitigate this let's introduce a new 'EventLoopBackend'
property to set the thread pool size. The threads will be created during
the pool's initialization, remain available during its lifetime
regardless of demand, and destroyed upon freeing it. A properly
characterized workload will then be able to configure the pool to avoid
any latency spike.

Signed-off-by: Nicolas Saenz Julienne 
---
 include/block/aio.h | 11 +++
 qapi/qom.json   |  4 +++-
 util/async.c|  3 +++
 util/event-loop.c   | 15 ++-
 util/event-loop.h   |  4 
 util/main-loop.c| 13 +
 util/thread-pool.c  | 41 +
 7 files changed, 85 insertions(+), 6 deletions(-)

diff --git a/include/block/aio.h b/include/block/aio.h
index 5634173b12..331483d1d1 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -192,6 +192,8 @@ struct AioContext {
 QSLIST_HEAD(, Coroutine) scheduled_coroutines;
 QEMUBH *co_schedule_bh;
 
+int pool_min;
+int pool_max;
 /* Thread pool for performing work and receiving completion callbacks.
  * Has its own locking.
  */
@@ -769,4 +771,13 @@ void aio_context_set_poll_params(AioContext *ctx, int64_t 
max_ns,
 void aio_context_set_aio_params(AioContext *ctx, int64_t max_batch,
 Error **errp);
 
+/**
+ * aio_context_set_thread_pool_params:
+ * @ctx: the aio context
+ * @min: Thread pool's min size, 0 by default
+ * @max: Thread pool's max size, 64 by default
+ */
+void aio_context_set_thread_pool_params(AioContext *ctx, uint64_t min,
+uint64_t max, Error **errp);
+
 #endif
diff --git a/qapi/qom.json b/qapi/qom.json
index e7730ef62f..79c141b6bf 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -526,7 +526,9 @@
   'data': { '*poll-max-ns': 'int',
 '*poll-grow': 'int',
 '*poll-shrink': 'int',
-'*aio-max-batch': 'int' } }
+'*aio-max-batch': 'int',
+'*thread-pool-min': 'int',
+'*thread-pool-max': 'int' } }
 
 ##
 # @MemoryBackendProperties:
diff --git a/util/async.c b/util/async.c
index 08d25feef5..58ef2e2ee5 100644
--- a/util/async.c
+++ b/util/async.c
@@ -562,6 +562,9 @@ AioContext *aio_context_new(Error **errp)
 
 ctx->aio_max_batch = 0;
 
+ctx->pool_min = 0;
+ctx->pool_max = 64;
+
 return ctx;
 fail:
 g_source_destroy(>source);
diff --git a/util/event-loop.c b/util/event-loop.c
index c0ddd61f20..f2532e7d31 100644
--- a/util/event-loop.c
+++ b/util/event-loop.c
@@ -51,6 +51,12 @@ static EventLoopBackendParamInfo poll_shrink_info = {
 static EventLoopBackendParamInfo aio_max_batch_info = {
 "aio-max-batch", offsetof(EventLoopBackend, aio_max_batch),
 };
+static EventLoopBackendParamInfo thread_pool_min_info = {
+"thread-pool-min", offsetof(EventLoopBackend, thread_pool_min),
+};
+static EventLoopBackendParamInfo thread_pool_max_info = {
+"thread-pool-max", offsetof(EventLoopBackend, thread_pool_max),
+};
 
 static void event_loop_backend_get_param(Object *obj, Visitor *v,
 const char *name, void *opaque, Error **errp)
@@ -84,7 +90,6 @@ static void event_loop_backend_set_param(Object *obj, Visitor 
*v,
 *field = value;
 
 return;
-
 }
 
 static void
@@ -132,6 +137,14 @@ static void event_loop_backend_class_init(ObjectClass 
*klass, void *class_data)
   event_loop_backend_get_param,
   event_loop_backend_set_param,
   NULL, _max_batch_info);
+object_class_property_add(klass, "thread-pool-min", "int",
+  event_loop_backend_get_param,
+  event_loop_backend_set_param,
+  NULL, _pool_min_info);
+object_class_property_add(klass, "thread-pool-max", "int",
+  event_loop_backend_get_param,
+  event_loop_backend_set_param,
+  NULL, _pool_max_info);
 }
 
 static const TypeInfo event_loop_backend_info = {
diff --git a/util/event-loop.h b/util/event-loop.h
index 34cf9309af..0f4255ee7b 100644
--- a/util/event-loop.h
+++ b/util/event-loop.h
@@ -37,5 +37,9 @@ struct EventLoopBackend {
 
 /* AioContext AIO engine parameters */
 int64_t aio_max_batch;
+
+/* AioContext thread pool parameters */
+int64_t thread_pool_min;
+int64_t thread_pool_max;
 };
 #endif
diff --git a/util/main-loop.c b/util/main-loop.c
index 395fd9bd3e..266a9c72d8 100644
--- 

[PATCH 1/2] iotests/065: Check for zstd support

[Hanna Reitz]

Some test cases run in iotest 065 require zstd support.  Skip them if
qemu-img reports it not to be available.

Reported-by: Thomas Huth 
Fixes: 12a936171d71f839dc907ff ("iotest 065: explicit compression type")
Signed-off-by: Hanna Reitz 
---
 tests/qemu-iotests/065 | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
index f7c1b68dad..b68df84642 100755
--- a/tests/qemu-iotests/065
+++ b/tests/qemu-iotests/065
@@ -24,7 +24,7 @@ import os
 import re
 import json
 import iotests
-from iotests import qemu_img, qemu_img_pipe
+from iotests import qemu_img_pipe, qemu_img_pipe_and_status
 import unittest
 
 test_img = os.path.join(iotests.test_dir, 'test.img')
@@ -35,8 +35,13 @@ class TestImageInfoSpecific(iotests.QMPTestCase):
 def setUp(self):
 if self.img_options is None:
 self.skipTest('Skipping abstract test class')
-qemu_img('create', '-f', iotests.imgfmt, '-o', self.img_options,
- test_img, '128K')
+output, status = qemu_img_pipe_and_status('create',
+  '-f', iotests.imgfmt,
+  '-o', self.img_options,
+  test_img, '128K')
+if status == 1 and \
+"'compression-type' does not accept value 'zstd'" in output:
+self.case_skip('zstd compression not supported')
 
 def tearDown(self):
 os.remove(test_img)
-- 
2.34.1

[PATCH 2/2] iotests/303: Check for zstd support

[Hanna Reitz]

303 runs two test cases, one of which requires zstd support.
Unfortunately, given that this is not a unittest-style test, we cannot
easily skip that single case, and instead can only skip the whole test.

(Alternatively, we could split this test into a zlib and a zstd part,
but that seems excessive, given that this test is not in auto and thus
likely only run by developers who have zstd support compiled in.)

Fixes: 677e0bae686e7c670a71d1f ("iotest 303: explicit compression type")
Signed-off-by: Hanna Reitz 
---
 tests/qemu-iotests/303 | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303
index 16c2e10827..5a3efb4ba3 100755
--- a/tests/qemu-iotests/303
+++ b/tests/qemu-iotests/303
@@ -21,7 +21,8 @@
 
 import iotests
 import subprocess
-from iotests import qemu_img_create, qemu_io, file_path, log, filter_qemu_io
+from iotests import notrun, qemu_img_pipe_and_status, qemu_io, file_path, \
+log, filter_qemu_io
 
 iotests.script_initialize(supported_fmts=['qcow2'],
   unsupported_imgopts=['refcount_bits', 'compat'])
@@ -55,9 +56,15 @@ def add_bitmap(num, begin, end, disabled):
 
 
 def test(compression_type: str, json_output: bool) -> None:
-qemu_img_create('-f', iotests.imgfmt,
-'-o', f'compression_type={compression_type}',
-disk, '10M')
+opts = f'compression_type={compression_type}'
+output, status = qemu_img_pipe_and_status('create',
+  '-f', iotests.imgfmt,
+  '-o', opts,
+  disk, '10M')
+if status == 1 and \
+"'compression-type' does not accept value 'zstd'" in output:
+notrun('zstd compression not supported')
+
 add_bitmap(1, 0, 6, False)
 add_bitmap(2, 6, 8, True)
 
-- 
2.34.1

Re: [PATCH 15/20] migration: Allow migrate-recover to run multiple times

[Dr. David Alan Gilbert]

* Peter Xu (pet...@redhat.com) wrote:
> Previously migration didn't have an easy way to cleanup the listening
> transport, migrate recovery only allows to execute once.  That's done with a
> trick flag in postcopy_recover_triggered.
> 
> Now the facility is already there.
> 
> Drop postcopy_recover_triggered and instead allows a new migrate-recover to
> release the previous listener transport.
> 
> Signed-off-by: Peter Xu 

OK, was that the only reason you couldn't recover twice?


Reviewed-by: Dr. David Alan Gilbert 

> ---
>  migration/migration.c | 13 ++---
>  migration/migration.h |  1 -
>  migration/savevm.c|  3 ---
>  3 files changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 6bb321cdd3..16086897aa 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2159,11 +2159,8 @@ void qmp_migrate_recover(const char *uri, Error **errp)
>  return;
>  }
>  
> -if (qatomic_cmpxchg(>postcopy_recover_triggered,
> -   false, true) == true) {
> -error_setg(errp, "Migrate recovery is triggered already");
> -return;
> -}
> +/* If there's an existing transport, release it */
> +migration_incoming_transport_cleanup(mis);
>  
>  /*
>   * Note that this call will never start a real migration; it will
> @@ -2171,12 +2168,6 @@ void qmp_migrate_recover(const char *uri, Error **errp)
>   * to continue using that newly established channel.
>   */
>  qemu_start_incoming_migration(uri, errp);
> -
> -/* Safe to dereference with the assert above */
> -if (*errp) {
> -/* Reset the flag so user could still retry */
> -qatomic_set(>postcopy_recover_triggered, false);
> -}
>  }
>  
>  void qmp_migrate_pause(Error **errp)
> diff --git a/migration/migration.h b/migration/migration.h
> index f17ccc657c..a863032b71 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -139,7 +139,6 @@ struct MigrationIncomingState {
>  struct PostcopyBlocktimeContext *blocktime_ctx;
>  
>  /* notify PAUSED postcopy incoming migrations to try to continue */
> -bool postcopy_recover_triggered;
>  QemuSemaphore postcopy_pause_sem_dst;
>  QemuSemaphore postcopy_pause_sem_fault;
>  
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 967ff80547..254aa78234 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2589,9 +2589,6 @@ static bool 
> postcopy_pause_incoming(MigrationIncomingState *mis)
>  
>  assert(migrate_postcopy_ram());
>  
> -/* Clear the triggered bit to allow one recovery */
> -mis->postcopy_recover_triggered = false;
> -
>  /*
>   * Unregister yank with either from/to src would work, since ioc behind 
> it
>   * is the same
> -- 
> 2.32.0
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK