[PATCH v2] hw/display/xlnx_dp: fix an out-of-bounds read in xlnx_dp_read

[Qiang Liu]

xlnx_dp_read allows an out-of-bounds read at its default branch because
of an improper index.

According to
https://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers.html
(DP Module), registers 0x3A4/0x3A4/0x3AC are allowed.

DP_INT_MASK 0x03A4  32  mixed   0xF03F  Interrupt Mask 
Register for intrN.
DP_INT_EN   0x03A8  32  mixed   0x  Interrupt 
Enable Register.
DP_INT_DS   0x03AC  32  mixed   0x  Interrupt 
Disable Register.

In xlnx_dp_write, when the offset is 0x3A8 and 0x3AC, the virtual device
will write s->core_registers[0x3A4
>> 2]. That is to say, the maxize of s->core_registers could be ((0x3A4
>> 2) + 1). However, the current size of s->core_registers is (0x3AF >>
>> 2), that is ((0x3A4 >> 2) + 2), which is out of the range.
In xlxn_dp_read, the access to offset 0x3A8 or 0x3AC will be directed to
the offset 0x3A8 (incorrect functionality) or 0x3AC (out-of-bounds read)
rather than 0x3A4.

This patch enforces the read access to offset 0x3A8 and 0x3AC to 0x3A4,
but does not adjust the size of s->core_registers to avoid breaking
migration.

Fixes: 58ac482a66de ("introduce xlnx-dp")
Signed-off-by: Qiang Liu 
---
v2:
  - not change DP_CORE_REG_ARRAY_SIZE
  - add a qtest reproducer
  - update the code style

I have a question about the QTest reproducer. Before patching xlnx-dp,
(0x3ac >> 2) will exceed the right bound of s->core_registers.  However,
this is allowed by the assertion. There is no warning and this
reproducer will pass. Is the reprodocer OK?

 hw/display/xlnx_dp.c|  6 +-
 tests/qtest/fuzz-xlnx-dp-test.c | 33 +
 tests/qtest/meson.build |  1 +
 3 files changed, 39 insertions(+), 1 deletion(-)
 create mode 100644 tests/qtest/fuzz-xlnx-dp-test.c

diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
index 7bcbb13..747df6e 100644
--- a/hw/display/xlnx_dp.c
+++ b/hw/display/xlnx_dp.c
@@ -714,7 +714,11 @@ static uint64_t xlnx_dp_read(void *opaque, hwaddr offset, 
unsigned size)
 break;
 default:
 assert(offset <= (0x3AC >> 2));
-ret = s->core_registers[offset];
+if (offset == (0x3A8 >> 2) || offset == (0x3AC >> 2)) {
+ret = s->core_registers[DP_INT_MASK];
+} else {
+ret = s->core_registers[offset];
+}
 break;
 }
 
diff --git a/tests/qtest/fuzz-xlnx-dp-test.c b/tests/qtest/fuzz-xlnx-dp-test.c
new file mode 100644
index 000..69eb6c0
--- /dev/null
+++ b/tests/qtest/fuzz-xlnx-dp-test.c
@@ -0,0 +1,33 @@
+/*
+ * QTest fuzzer-generated testcase for xlnx-dp display device
+ *
+ * Copyright (c) 2021 Qiang Liu 
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "libqos/libqtest.h"
+
+/*
+ * This used to trigger the out-of-bounds read in xlnx_dp_read
+ */
+static void test_fuzz_xlnx_dp_0x3ac(void)
+{
+QTestState *s = qtest_init("-M xlnx-zcu102 -display none ");
+qtest_readl(s, 0xfd4a03ac);
+qtest_quit(s);
+}
+
+int main(int argc, char **argv)
+{
+const char *arch = qtest_get_arch();
+
+g_test_init(&argc, &argv, NULL);
+
+   if (strcmp(arch, "aarch64") == 0) {
+qtest_add_func("fuzz/test_fuzz_xlnx_dp/3ac", test_fuzz_xlnx_dp_0x3ac);
+   }
+
+   return g_test_run();
+}
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 83ad237..6fd6b0e 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -185,6 +185,7 @@ qtests_aarch64 = \
'numa-test',
'boot-serial-test',
'xlnx-can-test',
+   'fuzz-xlnx-dp-test',
'migration-test']
 
 qtests_s390x = \
-- 
2.7.4

[Bug 1908626] Re: Atomic test-and-set instruction does not work on qemu-user

[taos]

Thanks. Tested, the problem is gone.

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

Title:
  Atomic test-and-set instruction does not work on qemu-user

Status in QEMU:
  Expired

Bug description:
  I try to compile and run PostgreSQL/Greenplum database inside docker 
container/qemu-aarch64-static:
  ```
   host: CentOS7 x86_64
   container: centos:centos7.9.2009 --platform linux/arm64/v8
   qemu-user-static: https://github.com/multiarch/qemu-user-static/releases/
  ```

  However, GP/PG's spinlock always gets stuck and reports PANIC errors. It 
seems its spinlock
  has something wrong.
  ```
  https://github.com/greenplum-db/gpdb/blob/master/src/include/storage/s_lock.h
  
https://github.com/greenplum-db/gpdb/blob/master/src/backend/storage/lmgr/s_lock.c
  ```

  So I extract its spinlock implementation into one test C source file (see 
attachment file),
  and get reprodcued:

  ```
  $ gcc spinlock_qemu.c
  $ ./a.out 
  C -- slock inited, lock value is: 0
  parent 139642, child 139645
  P -- slock lock before, lock value is: 0
  P -- slock locked, lock value is: 1
  P -- slock unlock after, lock value is: 0
  C -- slock lock before, lock value is: 1
  P -- slock lock before, lock value is: 1
  C -- slock locked, lock value is: 1
  C -- slock unlock after, lock value is: 0
  C -- slock lock before, lock value is: 1
  P -- slock locked, lock value is: 1
  P -- slock unlock after, lock value is: 0
  P -- slock lock before, lock value is: 1
  C -- slock locked, lock value is: 1
  C -- slock unlock after, lock value is: 0
  P -- slock locked, lock value is: 1
  C -- slock lock before, lock value is: 1
  P -- slock unlock after, lock value is: 0
  C -- slock locked, lock value is: 1
  P -- slock lock before, lock value is: 1
  C -- slock unlock after, lock value is: 0
  P -- slock locked, lock value is: 1
  C -- slock lock before, lock value is: 1
  P -- slock unlock after, lock value is: 0
  C -- slock locked, lock value is: 1
  P -- slock lock before, lock value is: 1
  C -- slock unlock after, lock value is: 0
  P -- slock locked, lock value is: 1
  C -- slock lock before, lock value is: 1
  P -- slock unlock after, lock value is: 0
  P -- slock lock before, lock value is: 1
  spin timeout, lock value is 1 (pid 139642)
  spin timeout, lock value is 1 (pid 139645)
  spin timeout, lock value is 1 (pid 139645)
  spin timeout, lock value is 1 (pid 139642)
  spin timeout, lock value is 1 (pid 139645)
  spin timeout, lock value is 1 (pid 139642)
  ...
  ...
  ...
  ```

  NOTE: this code always works on PHYSICAL ARM64 server.

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

Re: [RFC v3] virtio/vsock: add two more queues for datagram types

[Stefano Garzarella]

On Wed, Aug 4, 2021 at 8:41 AM Stefano Garzarella  
wrote:
>
> On Tue, Aug 03, 2021 at 11:58:27AM -0700, Jiang Wang . wrote:
> >On Wed, Jul 7, 2021 at 10:27 AM Stefano Garzarella  
> >wrote:
> >> On Wed, Jul 07, 2021 at 09:52:46AM -0700, Jiang Wang . wrote:
> >> >On Wed, Jul 7, 2021 at 1:33 AM Stefano Garzarella  
> >> >wrote:
> >> >> On Tue, Jul 06, 2021 at 10:26:07PM +, Jiang Wang wrote:
>
> [...]
>
> >> >> >+
> >> >> >+if (nvqs < 0)
> >> >> >+nvqs = MAX_VQS_WITHOUT_DGRAM;
> >> >> >+
> >> >> >+if (nvqs == MAX_VQS_WITH_DGRAM) {
> >> >> >+vvc->dgram_recv_vq = virtio_add_queue(vdev, 
> >> >> >VHOST_VSOCK_QUEUE_SIZE,
> >> >> >+  
> >> >> >vhost_vsock_common_handle_output);
> >> >> >+vvc->dgram_trans_vq = virtio_add_queue(vdev, 
> >> >> >VHOST_VSOCK_QUEUE_SIZE,
> >> >> >+   
> >> >> >vhost_vsock_common_handle_output);
> >> >> >+}
> >> >> >+
> >> >> > /* The event queue belongs to QEMU */
> >> >> > vvc->event_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> >> >> >
> >> >> > vhost_vsock_common_handle_output);
> >> >>
> >> >> Did you do a test with a guest that doesn't support datagram with QEMU
> >> >> and hosts that do?
> >> >>
> >> >Yes, and it works.
> >> >
> >> >> I repost my thoughts that I had on v2:
> >> >>
> >> >>  What happen if the guest doesn't support dgram?
> >> >>
> >> >>  I think we should dynamically use the 3rd queue or the 5th queue 
> >> >> for
> >> >>  the events at runtime after the guest acked the features.
> >> >>
> >> >>  Maybe better to switch to an array of VirtQueue.
> >> >>
> >> >I think in current V3, it  already dynamically use 3rd or 5th queue
> >> >depending
> >> >on the feature bit.
> >>
> >> I'm not sure. IIUC when vhost_vsock_common_realize() is called, we don't
> >> know the features acked by the guest, so how can it be dynamic?
> >>
> >> Here we should know only if the host kernel supports it.
> >>
> >> Maybe it works, because in QEMU we use the event queue only after a
> >> migration to send a reset event, so you can try to migrate a guest to
> >> check this path.
> >>
> >I tried VM migration and didn't see any problems. The migration looks fine
> >and vsock dgram still works after migration. Is there any more specific test
> >you want to run to check for this code path?
> >
>
> I meant a migration of a guest from QEMU without this patch to a QEMU
> with this patch. Of course in that case testing a socket stream.
>

Sorry, I meant the opposite.

You should try to migrate a guest that does not support dgrams starting 
from a QEMU with this patch (and kernel that supports dgram, so qemu 
uses the 5th queue for event), to a QEMU without this patch.

I still think the event queue should remain the third and those dgrams 
the next 2.

Thanks,
Stefano

Re: [RFC v3] virtio/vsock: add two more queues for datagram types

[Stefano Garzarella]

On Tue, Aug 03, 2021 at 11:58:27AM -0700, Jiang Wang . wrote:

On Wed, Jul 7, 2021 at 10:27 AM Stefano Garzarella  wrote:

On Wed, Jul 07, 2021 at 09:52:46AM -0700, Jiang Wang . wrote:
>On Wed, Jul 7, 2021 at 1:33 AM Stefano Garzarella  wrote:
>> On Tue, Jul 06, 2021 at 10:26:07PM +, Jiang Wang wrote:


[...]


>> >+
>> >+if (nvqs < 0)
>> >+nvqs = MAX_VQS_WITHOUT_DGRAM;
>> >+
>> >+if (nvqs == MAX_VQS_WITH_DGRAM) {
>> >+vvc->dgram_recv_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
>> >+  
vhost_vsock_common_handle_output);
>> >+vvc->dgram_trans_vq = virtio_add_queue(vdev, 
VHOST_VSOCK_QUEUE_SIZE,
>> >+   
vhost_vsock_common_handle_output);
>> >+}
>> >+
>> > /* The event queue belongs to QEMU */
>> > vvc->event_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
>> >vhost_vsock_common_handle_output);
>>
>> Did you do a test with a guest that doesn't support datagram with QEMU
>> and hosts that do?
>>
>Yes, and it works.
>
>> I repost my thoughts that I had on v2:
>>
>>  What happen if the guest doesn't support dgram?
>>
>>  I think we should dynamically use the 3rd queue or the 5th queue for
>>  the events at runtime after the guest acked the features.
>>
>>  Maybe better to switch to an array of VirtQueue.
>>
>I think in current V3, it  already dynamically use 3rd or 5th queue
>depending
>on the feature bit.

I'm not sure. IIUC when vhost_vsock_common_realize() is called, we don't
know the features acked by the guest, so how can it be dynamic?

Here we should know only if the host kernel supports it.

Maybe it works, because in QEMU we use the event queue only after a
migration to send a reset event, so you can try to migrate a guest to
check this path.


I tried VM migration and didn't see any problems. The migration looks fine
and vsock dgram still works after migration. Is there any more specific test
you want to run to check for this code path?



I meant a migration of a guest from QEMU without this patch to a QEMU 
with this patch. Of course in that case testing a socket stream.


btw, I will address the rest of the comments and send a new version 
soon.




Great!

Thanks,
Stefano

Re: [PATCH 0/3] Disable vhost device IOTLB is IOMMU is not enabled

[Chao Gao]

On Wed, Aug 04, 2021 at 11:48:00AM +0800, Jason Wang wrote:
>Hi:
>
>We currently try to enable device IOTLB when iommu_platform is
>set. This may lead unnecessary trasnsactions between qemu and vhost
>when vIOMMU is not used (which is the typical case for the encrypted
>VM).
>
>So patch tries to use transport specific method to detect the enalbing
>of vIOMMU and enable the device IOTLB only if vIOMMU is enalbed.
>
>Please review.

Tested-by: Chao Gao 

Tested with TDX; this series fixes the performance issue we saw in a TD
when vhost was enabled.

Thanks
Chao

>
>Thanks
>
>Jason Wang (3):
>  virtio-bus: introduce iommu_enabled()
>  virtio-pci: implement iommu_enabled()
>  vhost: correctly detect the enabling IOMMU
>
> hw/virtio/vhost.c  |  2 +-
> hw/virtio/virtio-bus.c | 14 ++
> hw/virtio/virtio-pci.c | 14 ++
> include/hw/virtio/virtio-bus.h |  4 +++-
> 4 files changed, 32 insertions(+), 2 deletions(-)
>
>-- 
>2.25.1
>

Re: [Question] Reduce the msix_load cost for VFIO device

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

在 2021/8/3 22:19, Alex Williamson 写道:
> On Tue, 3 Aug 2021 16:43:07 +0800
> "Longpeng (Mike, Cloud Infrastructure Service Product Dept.)"
>  wrote:
> 
>> Hi Alex,
>>
>> We found that the msix_load() will cost 40~50ms if the VF has 60+ interrupts,
>> the following code cost too much for each interrupt:
>>
>> msix_load:
>>   for (vector = 0; vector < 60; vector++)
>> msix_handle_mask_update
>>   vfio_msix_vector_do_use
>> vfio_add_kvm_msi_virq
>>   kvm_irqchip_add_msi_route
>> kvm_irqchip_commit_routes <-- cost 0.8ms each time
>>
>> In irq remapping mode, the VF interrupts are not routed through KVM irqchip
> 
> I'm not sure what this means.  Your MSIX interrupts are going through
> QEMU anyway?  Why?
> 

Um ... I made a mistake, the KVM irqchip commit operation can not be skip, it
cause the VF interrupts pass to the QEMU, it's harmful for the performance.

>> in fact, so maybe we can reduce this cost by "x-no-kvm-msix=ture", right?
>> Are there any risks if we do in this way ?
> 
> You're obviously free to configure your device this way, but the
> expectation is that any sort of startup latency is more than offset by
> improved runtime latency through the KVM route.  This option is usually
> reserved for debugging, when we want to see all interaction with the
> device in QEMU.
> 
> If there's a case where we're not detecting that a KVM route is
> ineffective, then we should figure out how to detect that and skip this
> code, but again the expectation is that the KVM route is worthwhile.
> 
> If this is specifically about kvm_irqchip_commit_routes(), maybe the
> setup path needs a way to batch multiple routes and defer the commit,
> if that's possible.  Thanks,

Agree.

I had wrote a draft that defer to commit, it works. In addition, the
vfio_disable_irqindex/vfio_enable_vectors are called N times in msix_load path,
they cost much more than kvm_irqchip_commit, it's also worth to optimize.

> 
> Alex
> 
> .
> 

[PATCH 3/3] vhost: correctly detect the enabling IOMMU

[Jason Wang]

Vhost used to compare the dma_as against the address_space_memory to
detect whether the IOMMU is enabled or not. This might not work well
since the virito-bus may call get_dma_as if VIRTIO_F_IOMMU_PLATFORM is
set without an actual IOMMU enabled when device is plugged. In the
case of PCI where pci_get_address_space() is used, the bus master as
is returned. So vhost actually tries to enable device IOTLB even if
the IOMMU is not enabled. This will lead a lots of unnecessary
transactions between vhost and Qemu and will introduce a huge drop of
the performance.

For PCI, an ideal approach is to use pci_device_iommu_address_space()
just for get_dma_as. But Qemu may choose to initialize the IOMMU after
the virtio-pci which lead a wrong address space is returned during
device plugged. So this patch switch to use transport specific way via
iommu_enabled() to detect the IOMMU during vhost start. In this case,
we are fine since we know the IOMMU is initialized correctly.

Signed-off-by: Jason Wang 
---
 hw/virtio/vhost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 7b7bde7657..d2097b7423 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -286,7 +286,7 @@ static int vhost_dev_has_iommu(struct vhost_dev *dev)
  * does not have IOMMU, there's no need to enable this feature
  * which may cause unnecessary IOTLB miss/update trnasactions.
  */
-return vdev->dma_as != &address_space_memory &&
+return virtio_bus_device_iommu_enabled(vdev) &&
virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
 }
 
-- 
2.25.1

[PATCH 2/3] virtio-pci: implement iommu_enabled()

[Jason Wang]

This patch implements the PCI transport version of iommu_enabled. This
is done by comparing the address space returned by
pci_device_iommu_address_space() against address_space_memory.

Note that an ideal approach is to use pci_device_iommu_address_space()
in get_dma_as(), but it might not work well since the IOMMU could be
initialized after the virtio-pci device is initialized.

Signed-off-by: Jason Wang 
---
 hw/virtio/virtio-pci.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index b321604d9b..050af2517b 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1110,6 +1110,19 @@ static AddressSpace *virtio_pci_get_dma_as(DeviceState 
*d)
 return pci_get_address_space(dev);
 }
 
+static bool virtio_pci_iommu_enabled(DeviceState *d)
+{
+VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
+PCIDevice *dev = &proxy->pci_dev;
+AddressSpace *dma_as = pci_device_iommu_address_space(dev);
+
+if (dma_as == &address_space_memory) {
+return false;
+}
+
+return true;
+}
+
 static bool virtio_pci_queue_enabled(DeviceState *d, int n)
 {
 VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
@@ -2173,6 +2186,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, 
void *data)
 k->ioeventfd_enabled = virtio_pci_ioeventfd_enabled;
 k->ioeventfd_assign = virtio_pci_ioeventfd_assign;
 k->get_dma_as = virtio_pci_get_dma_as;
+k->iommu_enabled = virtio_pci_iommu_enabled;
 k->queue_enabled = virtio_pci_queue_enabled;
 }
 
-- 
2.25.1

[PATCH 1/3] virtio-bus: introduce iommu_enabled()

[Jason Wang]

This patch introduce a new method for the virtio-bus for the transport
to report whether or not the IOMMU is enabled for the device.

Signed-off-by: Jason Wang 
---
 hw/virtio/virtio-bus.c | 14 ++
 include/hw/virtio/virtio-bus.h |  4 +++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 859978d248..d23db98c56 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -325,6 +325,20 @@ static char *virtio_bus_get_fw_dev_path(DeviceState *dev)
 return NULL;
 }
 
+bool virtio_bus_device_iommu_enabled(VirtIODevice *vdev)
+{
+DeviceState *qdev = DEVICE(vdev);
+BusState *qbus = BUS(qdev_get_parent_bus(qdev));
+VirtioBusState *bus = VIRTIO_BUS(qbus);
+VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus);
+
+if (!klass->iommu_enabled) {
+return false;
+}
+
+return klass->iommu_enabled(qbus->parent);
+}
+
 static void virtio_bus_class_init(ObjectClass *klass, void *data)
 {
 BusClass *bus_class = BUS_CLASS(klass);
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index ef8abe49c5..7ab8c9dab0 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -93,6 +93,7 @@ struct VirtioBusClass {
  */
 bool has_variable_vring_alignment;
 AddressSpace *(*get_dma_as)(DeviceState *d);
+bool (*iommu_enabled)(DeviceState *d);
 };
 
 struct VirtioBusState {
@@ -154,5 +155,6 @@ void virtio_bus_release_ioeventfd(VirtioBusState *bus);
 int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign);
 /* Tell the bus that the ioeventfd handler is no longer required. */
 void virtio_bus_cleanup_host_notifier(VirtioBusState *bus, int n);
-
+/* Whether the IOMMU is enabled for this device */
+bool virtio_bus_device_iommu_enabled(VirtIODevice *vdev);
 #endif /* VIRTIO_BUS_H */
-- 
2.25.1

[PATCH 0/3] Disable vhost device IOTLB is IOMMU is not enabled

[Jason Wang]

Hi:

We currently try to enable device IOTLB when iommu_platform is
set. This may lead unnecessary trasnsactions between qemu and vhost
when vIOMMU is not used (which is the typical case for the encrypted
VM).

So patch tries to use transport specific method to detect the enalbing
of vIOMMU and enable the device IOTLB only if vIOMMU is enalbed.

Please review.

Thanks

Jason Wang (3):
  virtio-bus: introduce iommu_enabled()
  virtio-pci: implement iommu_enabled()
  vhost: correctly detect the enabling IOMMU

 hw/virtio/vhost.c  |  2 +-
 hw/virtio/virtio-bus.c | 14 ++
 hw/virtio/virtio-pci.c | 14 ++
 include/hw/virtio/virtio-bus.h |  4 +++-
 4 files changed, 32 insertions(+), 2 deletions(-)

-- 
2.25.1

Re: [PATCH] vhost: use large iotlb entry if no IOMMU translation is needed

[Jason Wang]

On Wed, Aug 4, 2021 at 5:11 AM Peter Xu  wrote:
>
> On Tue, Aug 03, 2021 at 04:14:57PM +0800, Jason Wang wrote:
> >
> > 在 2021/8/3 下午1:51, Chao Gao 写道:
> > > On Tue, Aug 03, 2021 at 12:43:58PM +0800, Jason Wang wrote:
> > > > 在 2021/8/3 下午12:29, Chao Gao 写道:
> > > > > Ping. Could someone help to review this patch?
> > > > >
> > > > > Thanks
> > > > > Chao
> > > > >
> > > > > On Wed, Jul 21, 2021 at 03:54:02PM +0800, Chao Gao wrote:
> > > > > > If guest enables IOMMU_PLATFORM for virtio-net, severe network
> > > > > > performance drop is observed even if there is no IOMMU.
> > > >
> > > > We see such reports internally and we're testing a patch series to 
> > > > disable
> > > > vhost IOTLB in this case.
> > > >
> > > > Will post a patch soon.
>
> [1]
>
> > > OK. put me in the CC list. I would like to test with TDX to ensure your 
> > > patch
> > > fix the performance issue I am facing.
> >
> >
> > Sure.
> >
> >
> > >
> > > >
> > > >
> > > > > >And disabling
> > > > > > vhost can mitigate the perf issue. Finally, we found the culprit is
> > > > > > frequent iotlb misses: kernel vhost-net has 2048 entries and each
> > > > > > entry is 4K (qemu uses 4K for i386 if no IOMMU); vhost-net can cache
> > > > > > translations for up to 8M (i.e. 4K*2048) IOVAs. If guest uses >8M
> > > > > > memory for DMA, there are some iotlb misses.
> > > > > >
> > > > > > If there is no IOMMU or IOMMU is disabled or IOMMU works in 
> > > > > > pass-thru
> > > > > > mode, we can optimistically use large, unaligned iotlb entries 
> > > > > > instead
> > > > > > of 4K-aligned entries to reduce iotlb pressure.
> > > >
> > > > Instead of introducing new general facilities like unaligned IOTLB 
> > > > entry. I
> > > > wonder if we optimize the vtd_iommu_translate() to use e.g 1G instead?
> > > using 1G iotlb entry looks feasible.
> >
> >
> > Want to send a patch?
> >
> >
> > >
> > > >  } else {
> > > >  /* DMAR disabled, passthrough, use 4k-page*/
> > > >  iotlb.iova = addr & VTD_PAGE_MASK_4K;
> > > >  iotlb.translated_addr = addr & VTD_PAGE_MASK_4K;
> > > >  iotlb.addr_mask = ~VTD_PAGE_MASK_4K;
> > > >  iotlb.perm = IOMMU_RW;
> > > >  success = true;
> > > >  }
> > > >
> > > >
> > > > > >Actually, vhost-net
> > > > > > in kernel supports unaligned iotlb entry. The alignment requirement 
> > > > > > is
> > > > > > imposed by address_space_get_iotlb_entry() and 
> > > > > > flatview_do_translate().
> > > >
> > > > For the passthrough case, is there anyway to detect them and then 
> > > > disable
> > > > device IOTLB in those case?
> > > yes. I guess so; qemu knows the presence and status of iommu. Currently,
> > > in flatview_do_translate(), memory_region_get_iommu() tells whether a 
> > > memory
> > > region is behind an iommu.
> >
> >
> > The issues are:
> >
> > 1) how to know the passthrough mode is enabled (note that passthrough mode
> > doesn't mean it doesn't sit behind IOMMU)
>
> memory_region_get_iommu() should return NULL if it's passthrough-ed?

Do you mean something like memory_region_get_iommu(as->root)?

Could it be possible that the iommu was attached to a specified mr but not root.

In [1], I originally try to use pci_device_iommu_address_space() in
virtio_pci_get_dma_as().

But I suffer from the issue that virtio-pci might be initialized
before the e.g intel-iommu, which you try to solve at [2].

Then I switch to introduce a iommu_enabled that compares the as
returned by pci_device_iommu_address_space() against
address_space_memory.

And the iommu_enalbed will be called during vhost start where
intel-iommu is guaranteed to be initialized.

This seems to work. Let me post the patch and let's start there.

>
> > 2) can passthrough mode be disabled on the fly? If yes, we need to deal with
> > them
>
> I don't think it happens in reality; e.g. when iommu=pt is set it's set until
> the next guest reboot.  However I don't know whether there's limitation from
> spec-wise.

Yes, that's what I worry about. Even if it's not limited by the Intel
spec, we might suffer from this in another iommu.

>  Also I don't know whether there's special cases, for example when
> we kexec.
>
> I've two questions..
>
> Jason, when you mentioned the "fix" above [1], shouldn't that also fix the 
> same
> issue, and in a better way? Because ideally I think if we know vhost does not
> need a translation for either iommu_platform=off, or passthrough, dev-iotlb
> layer seems an overhead with no real use.

Yes, see above. Let me post the patch.

>
> The other question is I'm also wondering why we care about iommu_platform=on
> when there's no vIOMMU at all - it's about why we bother setting the flag at
> all?  Or is it a valid use case?

Encrypted VM like SEV or TDX.

In those cases, swiotlb needs to be used in the guest since the
swiotlb pages were not encrypted.

And the iommu_platform=on is the only way to let the guest driver use
DMA API (swiotlb).

(The name iommu_platform is confusing 

RE: Question about delaying dirty log stop to next vm_start

[Zhoujian (jay)]

Hi Yi,

> -Original Message-
> From: Yi Sun [mailto:yi.y@linux.intel.com]
> Sent: Monday, August 2, 2021 3:54 PM
> To: Zhoujian (jay) 
> Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; sanjay.k.ku...@intel.com;
> yi.l@intel.com; yi.y@linux.intel.com
> Subject: Question about delaying dirty log stop to next vm_start
> 
> Hi, Jay,
> 
> We are testing some live migration cases recently. We found your below patch
> delays the dirty log stop to the next vm_start.
> 
> commit 1931076077254a2886daa7c830c7838ebd1f81ef
> Author: Jay Zhou 
> Date:   Fri Jul 28 18:28:53 2017 +0800
> 
> migration: optimize the downtime
> 
> We understand this commit can optimize the downtime. But we observed that
> the dirty log stop cannot be executed if user inputs "quit" in qemu monitor 
> after
> migration completes. Have you considered such case before?

If the migration is successfully finished, the src VM should be destroyed. 

If user inputs "quit" in qemu monitor after migration completes, the VM is still
paused(vcpu is stopped), dirty log takes no effect; if the migration failed, 
the dirty log
stop can be executed after src VM started.

Thanks,
Jay Zhou

Re: [PATCH] hw/net: Discard overly fragmented packets

[Jason Wang]

在 2021/8/3 下午5:51, Philippe Mathieu-Daudé 写道:

On 8/3/21 11:33 AM, Thomas Huth wrote:

On 05/07/2021 10.40, Philippe Mathieu-Daudé wrote:

Our infrastructure can handle fragmented packets up to
NET_MAX_FRAG_SG_LIST (64) pieces. This hard limit has
been proven enough in production for years. If it is
reached, it is likely an evil crafted packet. Discard it.

Include the qtest reproducer provided by Alexander Bulekov:

    $ make check-qtest-i386
    ...
    Running test qtest-i386/fuzz-vmxnet3-test
    qemu-system-i386: net/eth.c:334: void
eth_setup_ip4_fragmentation(const void *, size_t, void *, size_t,
size_t, size_t, _Bool):
    Assertion `frag_offset % IP_FRAG_UNIT_SIZE == 0' failed.

Cc: qemu-sta...@nongnu.org
Reported-by: OSS-Fuzz (Issue 35799)
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/460
Signed-off-by: Philippe Mathieu-Daudé 
---
   hw/net/net_tx_pkt.c |   8 ++
   tests/qtest/fuzz-vmxnet3-test.c | 195 
   MAINTAINERS |   1 +
   tests/qtest/meson.build |   1 +
   4 files changed, 205 insertions(+)
   create mode 100644 tests/qtest/fuzz-vmxnet3-test.c

Reviewed-by: Thomas Huth 

Jason, I think this would even still qualify for QEMU v6.1 ?

Yes, easy one for 6.1.



Yes, this will be included for rc3.

Thanks

[PATCH 6/6] docs/devel: create "Miscellaneous Topics" subsection

[John Snow]

The hallmark of any truly great taxonomical reorganization: the bin of
leftover bits and pieces that didn't neatly fit elsewhere.

Signed-off-by: John Snow 
---
 docs/devel/index.rst|  9 +
 docs/devel/section-misc.rst | 15 +++
 2 files changed, 16 insertions(+), 8 deletions(-)
 create mode 100644 docs/devel/section-misc.rst

diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index da579a7b666..ef14f3302e1 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -14,11 +14,4 @@ modifying QEMU's source code.
section-testing-debugging
section-tcg
section-subsystems
-   control-flow-integrity
-   decodetree
-   s390-dasd-ipl
-   qom
-   block-coroutine-wrapper
-   multi-process
-   ebpf_rss
-   vfio-migration
+   section-misc
diff --git a/docs/devel/section-misc.rst b/docs/devel/section-misc.rst
new file mode 100644
index 000..804852ff61c
--- /dev/null
+++ b/docs/devel/section-misc.rst
@@ -0,0 +1,15 @@
+Miscellaneous Topics
+
+
+.. toctree::
+   :maxdepth: 2
+   :includehidden:
+
+   control-flow-integrity
+   decodetree
+   s390-dasd-ipl
+   qom
+   block-coroutine-wrapper
+   multi-process
+   ebpf_rss
+   vfio-migration
-- 
2.31.1

[PATCH 5/6] docs/devel: create "Subsystem APIs" subsection

[John Snow]

Signed-off-by: John Snow 

---

I tried to grab documents that appeared to document literal internal
interfaces of QEMU (that weren't better described by some other
subsection) and put them here in this section.

Name isn't perfect, feel free to suggest alternatives.

Signed-off-by: John Snow 
---
 docs/devel/index.rst  | 10 +-
 docs/devel/section-subsystems.rst | 16 
 2 files changed, 17 insertions(+), 9 deletions(-)
 create mode 100644 docs/devel/section-subsystems.rst

diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index 71ed48881ef..da579a7b666 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -13,19 +13,11 @@ modifying QEMU's source code.
section-development
section-testing-debugging
section-tcg
+   section-subsystems
control-flow-integrity
-   loads-stores
-   memory
-   migration
-   atomics
decodetree
-   bitops
-   ui
-   reset
s390-dasd-ipl
-   clocks
qom
-   modules
block-coroutine-wrapper
multi-process
ebpf_rss
diff --git a/docs/devel/section-subsystems.rst 
b/docs/devel/section-subsystems.rst
new file mode 100644
index 000..fbe21f85adf
--- /dev/null
+++ b/docs/devel/section-subsystems.rst
@@ -0,0 +1,16 @@
+Subsystem APIs
+==
+
+.. toctree::
+   :maxdepth: 2
+   :includehidden:
+
+   atomics
+   bitops
+   loads-stores
+   clocks
+   memory
+   ui
+   migration
+   reset
+   modules
-- 
2.31.1

Re: [PATCH v2 1/4] hw/intc: Rename sifive_clint sources to riscv_aclint sources

[Alistair Francis]

On Sat, Jul 24, 2021 at 10:24 PM Anup Patel  wrote:
>
> We will be upgrading SiFive CLINT implementation into RISC-V ACLINT
> implementation so let's first rename the sources.
>
> Signed-off-by: Anup Patel 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  hw/intc/Kconfig|  2 +-
>  hw/intc/meson.build|  2 +-
>  hw/intc/{sifive_clint.c => riscv_aclint.c} |  2 +-
>  hw/riscv/Kconfig   | 12 ++--
>  hw/riscv/microchip_pfsoc.c |  2 +-
>  hw/riscv/shakti_c.c|  2 +-
>  hw/riscv/sifive_e.c|  2 +-
>  hw/riscv/sifive_u.c|  2 +-
>  hw/riscv/spike.c   |  2 +-
>  hw/riscv/virt.c|  2 +-
>  include/hw/intc/{sifive_clint.h => riscv_aclint.h} |  0
>  11 files changed, 15 insertions(+), 15 deletions(-)
>  rename hw/intc/{sifive_clint.c => riscv_aclint.c} (99%)
>  rename include/hw/intc/{sifive_clint.h => riscv_aclint.h} (100%)
>
> diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
> index f4694088a4..78aed93c45 100644
> --- a/hw/intc/Kconfig
> +++ b/hw/intc/Kconfig
> @@ -62,7 +62,7 @@ config RX_ICU
>  config LOONGSON_LIOINTC
>  bool
>
> -config SIFIVE_CLINT
> +config RISCV_ACLINT
>  bool
>
>  config SIFIVE_PLIC
> diff --git a/hw/intc/meson.build b/hw/intc/meson.build
> index 6e52a166e3..9c9338a9e4 100644
> --- a/hw/intc/meson.build
> +++ b/hw/intc/meson.build
> @@ -46,7 +46,7 @@ specific_ss.add(when: 'CONFIG_RX_ICU', if_true: 
> files('rx_icu.c'))
>  specific_ss.add(when: 'CONFIG_S390_FLIC', if_true: files('s390_flic.c'))
>  specific_ss.add(when: 'CONFIG_S390_FLIC_KVM', if_true: 
> files('s390_flic_kvm.c'))
>  specific_ss.add(when: 'CONFIG_SH_INTC', if_true: files('sh_intc.c'))
> -specific_ss.add(when: 'CONFIG_SIFIVE_CLINT', if_true: 
> files('sifive_clint.c'))
> +specific_ss.add(when: 'CONFIG_RISCV_ACLINT', if_true: 
> files('riscv_aclint.c'))
>  specific_ss.add(when: 'CONFIG_SIFIVE_PLIC', if_true: files('sifive_plic.c'))
>  specific_ss.add(when: 'CONFIG_XICS', if_true: files('xics.c'))
>  specific_ss.add(when: ['CONFIG_KVM', 'CONFIG_XICS'],
> diff --git a/hw/intc/sifive_clint.c b/hw/intc/riscv_aclint.c
> similarity index 99%
> rename from hw/intc/sifive_clint.c
> rename to hw/intc/riscv_aclint.c
> index 8a460fdf00..0f940e332b 100644
> --- a/hw/intc/sifive_clint.c
> +++ b/hw/intc/riscv_aclint.c
> @@ -26,7 +26,7 @@
>  #include "hw/sysbus.h"
>  #include "target/riscv/cpu.h"
>  #include "hw/qdev-properties.h"
> -#include "hw/intc/sifive_clint.h"
> +#include "hw/intc/riscv_aclint.h"
>  #include "qemu/timer.h"
>  #include "hw/irq.h"
>
> diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> index 86957ec7b0..bfa46694b7 100644
> --- a/hw/riscv/Kconfig
> +++ b/hw/riscv/Kconfig
> @@ -9,7 +9,7 @@ config MICROCHIP_PFSOC
>  select MCHP_PFSOC_MMUART
>  select MCHP_PFSOC_SYSREG
>  select MSI_NONBROKEN
> -select SIFIVE_CLINT
> +select RISCV_ACLINT
>  select SIFIVE_PDMA
>  select SIFIVE_PLIC
>  select UNIMP
> @@ -26,7 +26,7 @@ config SHAKTI_C
>  bool
>  select UNIMP
>  select SHAKTI
> -select SIFIVE_CLINT
> +select RISCV_ACLINT
>  select SIFIVE_PLIC
>
>  config RISCV_VIRT
> @@ -40,7 +40,7 @@ config RISCV_VIRT
>  select PCI_EXPRESS_GENERIC_BRIDGE
>  select PFLASH_CFI01
>  select SERIAL
> -select SIFIVE_CLINT
> +select RISCV_ACLINT
>  select SIFIVE_PLIC
>  select SIFIVE_TEST
>  select VIRTIO_MMIO
> @@ -49,7 +49,7 @@ config RISCV_VIRT
>  config SIFIVE_E
>  bool
>  select MSI_NONBROKEN
> -select SIFIVE_CLINT
> +select RISCV_ACLINT
>  select SIFIVE_GPIO
>  select SIFIVE_PLIC
>  select SIFIVE_UART
> @@ -60,7 +60,7 @@ config SIFIVE_U
>  bool
>  select CADENCE
>  select MSI_NONBROKEN
> -select SIFIVE_CLINT
> +select RISCV_ACLINT
>  select SIFIVE_GPIO
>  select SIFIVE_PDMA
>  select SIFIVE_PLIC
> @@ -76,5 +76,5 @@ config SPIKE
>  bool
>  select HTIF
>  select MSI_NONBROKEN
> -select SIFIVE_CLINT
> +select RISCV_ACLINT
>  select SIFIVE_PLIC
> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> index eef55f69fd..eed9e81355 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -49,7 +49,7 @@
>  #include "hw/riscv/boot.h"
>  #include "hw/riscv/riscv_hart.h"
>  #include "hw/riscv/microchip_pfsoc.h"
> -#include "hw/intc/sifive_clint.h"
> +#include "hw/intc/riscv_aclint.h"
>  #include "hw/intc/sifive_plic.h"
>  #include "sysemu/device_tree.h"
>  #include "sysemu/sysemu.h"
> diff --git a/hw/riscv/shakti_c.c b/hw/riscv/shakti_c.c
> index 09d4e1433e..f9f0a45651 100644
> --- a/hw/riscv/shakti_c.c
> +++ b/hw/riscv/shakti_c.c
> @@ -21,7 +21,7 @@
>  #include "hw/riscv/shakti_c.h"
>  #include "qapi/error.h"
>  #include "hw/intc/sifive_plic.h"
> -

[PATCH 3/6] docs/devel: create "Testing & Debugging" subsection

[John Snow]

Signed-off-by: John Snow 
---
 docs/devel/index.rst |  6 +-
 docs/devel/section-testing-debugging.rst | 12 
 2 files changed, 13 insertions(+), 5 deletions(-)
 create mode 100644 docs/devel/section-testing-debugging.rst

diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index 3cdfb786f1a..c560cc78497 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -11,19 +11,15 @@ modifying QEMU's source code.
 
section-community-governance
section-development
-   testing
-   fuzzing
+   section-testing-debugging
control-flow-integrity
loads-stores
memory
migration
atomics
-   ci
-   qtest
decodetree
tcg
tcg-icount
-   tracing
multi-thread-tcg
tcg-plugins
bitops
diff --git a/docs/devel/section-testing-debugging.rst 
b/docs/devel/section-testing-debugging.rst
new file mode 100644
index 000..e59ddab4cf5
--- /dev/null
+++ b/docs/devel/section-testing-debugging.rst
@@ -0,0 +1,12 @@
+Testing & Debugging
+===
+
+.. toctree::
+   :maxdepth: 2
+   :includehidden:
+
+   ci
+   fuzzing
+   qtest
+   testing
+   tracing
-- 
2.31.1

[PATCH 4/6] docs/devel: create TCG subsection

[John Snow]

Signed-off-by: John Snow 
---
 docs/devel/index.rst   |  5 +
 docs/devel/section-tcg.rst | 11 +++
 2 files changed, 12 insertions(+), 4 deletions(-)
 create mode 100644 docs/devel/section-tcg.rst

diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index c560cc78497..71ed48881ef 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -12,16 +12,13 @@ modifying QEMU's source code.
section-community-governance
section-development
section-testing-debugging
+   section-tcg
control-flow-integrity
loads-stores
memory
migration
atomics
decodetree
-   tcg
-   tcg-icount
-   multi-thread-tcg
-   tcg-plugins
bitops
ui
reset
diff --git a/docs/devel/section-tcg.rst b/docs/devel/section-tcg.rst
new file mode 100644
index 000..ff046983742
--- /dev/null
+++ b/docs/devel/section-tcg.rst
@@ -0,0 +1,11 @@
+TCG - Tiny Code Generator
+=
+
+.. toctree::
+   :maxdepth: 2
+   :includehidden:
+
+   tcg
+   tcg-icount
+   multi-thread-tcg
+   tcg-plugins
-- 
2.31.1

[PATCH 0/6] docs/devel: Organize devel manual into further subsections

[John Snow]

It's a bit cluttered. On my way to converting the QAPI/QMP documents to
ReST I thought it could do with another organizational level to help
make sense of things a bit more quickly.

John Snow (6):
  docs/devel: create "Community & Governance" subsection
  docs/devel: create "Developing QEMU" subsection
  docs/devel: create "Testing & Debugging" subsection
  docs/devel: create TCG subsection
  docs/devel: create "Subsystem APIs" subsection
  docs/devel: create "Miscellaneous Topics" subsection

 docs/devel/index.rst| 39 -
 docs/devel/section-community-governance.rst |  9 +
 docs/devel/section-development.rst  | 12 +++
 docs/devel/section-misc.rst | 15 
 docs/devel/section-subsystems.rst   | 16 +
 docs/devel/section-tcg.rst  | 11 ++
 docs/devel/section-testing-debugging.rst| 12 +++
 7 files changed, 81 insertions(+), 33 deletions(-)
 create mode 100644 docs/devel/section-community-governance.rst
 create mode 100644 docs/devel/section-development.rst
 create mode 100644 docs/devel/section-misc.rst
 create mode 100644 docs/devel/section-subsystems.rst
 create mode 100644 docs/devel/section-tcg.rst
 create mode 100644 docs/devel/section-testing-debugging.rst

-- 
2.31.1

[PATCH 1/6] docs/devel: create "Community & Governance" subsection

[John Snow]

Plonk the Code of Conduct and Conflict Resolution Policy guides into a
new "Community & Governance" subsection.

Signed-off-by: John Snow 
---
 docs/devel/index.rst| 3 +--
 docs/devel/section-community-governance.rst | 9 +
 2 files changed, 10 insertions(+), 2 deletions(-)
 create mode 100644 docs/devel/section-community-governance.rst

diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index 153979caf4b..f32883da8ed 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -9,8 +9,7 @@ modifying QEMU's source code.
:maxdepth: 2
:includehidden:
 
-   code-of-conduct
-   conflict-resolution
+   section-community-governance
build-system
style
kconfig
diff --git a/docs/devel/section-community-governance.rst 
b/docs/devel/section-community-governance.rst
new file mode 100644
index 000..2c7e07257d1
--- /dev/null
+++ b/docs/devel/section-community-governance.rst
@@ -0,0 +1,9 @@
+Community & Governance
+==
+
+.. toctree::
+   :maxdepth: 2
+   :includehidden:
+
+   code-of-conduct
+   conflict-resolution
-- 
2.31.1

[PATCH 2/6] docs/devel: create "Developing QEMU" subsection

[John Snow]

Signed-off-by: John Snow 
---
 docs/devel/index.rst   |  6 +-
 docs/devel/section-development.rst | 12 
 2 files changed, 13 insertions(+), 5 deletions(-)
 create mode 100644 docs/devel/section-development.rst

diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index f32883da8ed..3cdfb786f1a 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -10,9 +10,7 @@ modifying QEMU's source code.
:includehidden:
 
section-community-governance
-   build-system
-   style
-   kconfig
+   section-development
testing
fuzzing
control-flow-integrity
@@ -20,11 +18,9 @@ modifying QEMU's source code.
memory
migration
atomics
-   stable-process
ci
qtest
decodetree
-   secure-coding-practices
tcg
tcg-icount
tracing
diff --git a/docs/devel/section-development.rst 
b/docs/devel/section-development.rst
new file mode 100644
index 000..bba4fea30cb
--- /dev/null
+++ b/docs/devel/section-development.rst
@@ -0,0 +1,12 @@
+Developing QEMU
+===
+
+.. toctree::
+   :maxdepth: 2
+   :includehidden:
+
+   build-system
+   kconfig
+   style
+   secure-coding-practices
+   stable-process
-- 
2.31.1

Re: [PATCH] Makefile: Fix cscope issues on MacOS and soft links

[Peter Xu]

Hi, Alex,

On Tue, Aug 03, 2021 at 11:18:36PM +0100, Alex Bennée wrote:
> 
> Peter Xu  writes:
> 
> > This patch fixes actually two issues with 'make cscope'.
> >
> > Firstly, it fixes the command for MacOS "find" command as MacOS will append 
> > the
> > full path of "$(SRC_PATH)/" before each found entry, then after the final 
> > "./"
> > replacement trick it'll look like (e.g., "qapi/qmp-dispatch.c"):
> >
> >   /qapi/qmp-dispatch.c
> >
> > Which will point to the root directory instead.
> >
> > Fix it by simply remove the "/" in "$(SRC_PATH)/" of "find-src-path", then
> > it'll work for at least both Linux and MacOS.
> >
> > The other OS-independent issue is to start proactively ignoring soft links 
> > when
> > generating tags, otherwise by default on master branch we'll see this error
> > when "make cscope":
> >
> > cscope: cannot find file subprojects/libvhost-user/include/atomic.h
> >
> > This patch should fix the two issues altogether.
> >
> > Signed-off-by: Peter Xu 
> > ---
> >  Makefile | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 401c623a65..5562a9b464 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -229,7 +229,8 @@ distclean: clean
> > rm -f linux-headers/asm
> > rm -Rf .sdk
> >  
> > -find-src-path = find "$(SRC_PATH)/" -path "$(SRC_PATH)/meson" -prune -o \( 
> > -name "*.[chsS]" -o -name "*.[ch].inc" \)
> > +find-src-path = find "$(SRC_PATH)" -path "$(SRC_PATH)/meson" -prune -o \
> > +   -type l -prune -o \( -name "*.[chsS]" -o -name "*.[ch].inc" \)
> 
> The second half of the change causes my "make gtags" to descend down
> build directories and complain about unindexed files.

Would this help?

---8<---
diff --git a/Makefile b/Makefile
index 5562a9b464..eeb21f0e6a 100644
--- a/Makefile
+++ b/Makefile
@@ -251,7 +251,7 @@ gtags:
"GTAGS", "Remove old $@ files")
$(call quiet-command,   \
(cd $(SRC_PATH) &&  \
-$(find-src-path) | gtags -f -),\
+$(find-src-path) -print | gtags -f -), \
"GTAGS", "Re-index $(SRC_PATH)")
 
 .PHONY: TAGS
---8<---

For some reason, "make gtags" didn't use "-print" as the last expression.  My
understanding is when expression is not specified, then it's default will be
"-print".  However for some reason it acts like that when "-print" is not there
all the "-prone" expressions are not really behaving.  Above does work for me,
but frankly I don't really know enough on how "find" works here.

If you agree, I can repost a v2 with that squashed.

Thanks,

-- 
Peter Xu

Re: [PATCH v2] accel/tcg/user-exec: Fix read-modify-write of code on s390 hosts

[Richard Henderson]

On 8/3/21 12:16 PM, Ilya Leoshkevich wrote:

x86_64 dotnet/runtime uses cmpxchg for code patching. When running it
under s390x qemu-linux user, cpu_signal_handler() does not recognize
this as a write and does not restore PAGE_WRITE cleared by
tb_page_add(), incorrectly forwarding the signal to the guest code.

Signed-off-by: Ilya Leoshkevich
---

v1:https://lists.nongnu.org/archive/html/qemu-devel/2021-08/msg00464.html
v1 -> v2: Fix comment style, fix CSST detection (Richard).

  accel/tcg/user-exec.c | 48 ---
  1 file changed, 41 insertions(+), 7 deletions(-)


Reviewed-by: Richard Henderson 

r~

[PATCH v4] virtio/vsock: add two more queues for datagram types

[Jiang Wang]

Datagram sockets are connectionless and unreliable.
The sender does not know the capacity of the receiver
and may send more packets than the receiver can handle.

Add two more dedicate virtqueues for datagram sockets,
so that it will not unfairly steal resources from
stream and future connection-oriented sockets.

Signed-off-by: Jiang Wang 
---
v1 -> v2: use qemu cmd option to control number of queues,
removed configuration settings for dgram.
v2 -> v3: use ioctl to get features and decide number of
virt queues, instead of qemu cmd option.
v3 -> v4: change DGRAM feature bit value to 2. Add an argument
in vhost_vsock_common_realize to indicate dgram is supported or not.

 hw/virtio/vhost-user-vsock.c  |  2 +-
 hw/virtio/vhost-vsock-common.c| 58 ++-
 hw/virtio/vhost-vsock.c   |  5 +-
 include/hw/virtio/vhost-vsock-common.h|  6 +-
 include/hw/virtio/vhost-vsock.h   |  4 ++
 include/standard-headers/linux/virtio_vsock.h |  1 +
 6 files changed, 69 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
index 6095ed7349..e9ec0e1c00 100644
--- a/hw/virtio/vhost-user-vsock.c
+++ b/hw/virtio/vhost-user-vsock.c
@@ -105,7 +105,7 @@ static void vuv_device_realize(DeviceState *dev, Error 
**errp)
 return;
 }
 
-vhost_vsock_common_realize(vdev, "vhost-user-vsock");
+vhost_vsock_common_realize(vdev, "vhost-user-vsock", false);
 
 vhost_dev_set_config_notifier(&vvc->vhost_dev, &vsock_ops);
 
diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
index 4ad6e234ad..c78536911a 100644
--- a/hw/virtio/vhost-vsock-common.c
+++ b/hw/virtio/vhost-vsock-common.c
@@ -17,6 +17,8 @@
 #include "hw/virtio/vhost-vsock.h"
 #include "qemu/iov.h"
 #include "monitor/monitor.h"
+#include 
+#include 
 
 int vhost_vsock_common_start(VirtIODevice *vdev)
 {
@@ -196,9 +198,39 @@ int vhost_vsock_common_post_load(void *opaque, int 
version_id)
 return 0;
 }
 
-void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name)
+static int vhost_vsock_get_max_qps(bool enable_dgram)
+{
+uint64_t features;
+int ret;
+int fd = -1;
+
+if (!enable_dgram)
+return MAX_VQS_WITHOUT_DGRAM;
+
+fd = qemu_open_old("/dev/vhost-vsock", O_RDONLY);
+if (fd == -1) {
+error_report("vhost-vsock: failed to open device. %s", 
strerror(errno));
+return -1;
+}
+
+ret = ioctl(fd, VHOST_GET_FEATURES, &features);
+if (ret) {
+error_report("vhost-vsock: failed to read  device. %s", 
strerror(errno));
+qemu_close(fd);
+return ret;
+}
+
+qemu_close(fd);
+if (features & (1 << VIRTIO_VSOCK_F_DGRAM))
+return MAX_VQS_WITH_DGRAM;
+
+return MAX_VQS_WITHOUT_DGRAM;
+}
+
+void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name, bool 
enable_dgram)
 {
 VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
+int nvqs = MAX_VQS_WITHOUT_DGRAM;
 
 virtio_init(vdev, name, VIRTIO_ID_VSOCK,
 sizeof(struct virtio_vsock_config));
@@ -209,12 +241,24 @@ void vhost_vsock_common_realize(VirtIODevice *vdev, const 
char *name)
 vvc->trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
vhost_vsock_common_handle_output);
 
+nvqs = vhost_vsock_get_max_qps(enable_dgram);
+
+if (nvqs < 0)
+nvqs = MAX_VQS_WITHOUT_DGRAM;
+
+if (nvqs == MAX_VQS_WITH_DGRAM) {
+vvc->dgram_recv_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
+  
vhost_vsock_common_handle_output);
+vvc->dgram_trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
+   
vhost_vsock_common_handle_output);
+}
+
 /* The event queue belongs to QEMU */
 vvc->event_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
vhost_vsock_common_handle_output);
 
-vvc->vhost_dev.nvqs = ARRAY_SIZE(vvc->vhost_vqs);
-vvc->vhost_dev.vqs = vvc->vhost_vqs;
+vvc->vhost_dev.nvqs = nvqs;
+vvc->vhost_dev.vqs = g_new0(struct vhost_virtqueue, vvc->vhost_dev.nvqs);
 
 vvc->post_load_timer = NULL;
 }
@@ -227,6 +271,14 @@ void vhost_vsock_common_unrealize(VirtIODevice *vdev)
 
 virtio_delete_queue(vvc->recv_vq);
 virtio_delete_queue(vvc->trans_vq);
+if (vvc->vhost_dev.nvqs == MAX_VQS_WITH_DGRAM) {
+virtio_delete_queue(vvc->dgram_recv_vq);
+virtio_delete_queue(vvc->dgram_trans_vq);
+}
+
+if (vvc->vhost_dev.vqs)
+g_free(vvc->vhost_dev.vqs);
+
 virtio_delete_queue(vvc->event_vq);
 virtio_cleanup(vdev);
 }
diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index 1b1a5c70ed..f73523afaf 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -23,6 +23,7 @@
 
 const int feature_bits[] = {
 VIRTIO_VSOCK_F_SEQPAC

Re: [PATCH] Makefile: Fix cscope issues on MacOS and soft links

[Alex Bennée]

Peter Xu  writes:

> This patch fixes actually two issues with 'make cscope'.
>
> Firstly, it fixes the command for MacOS "find" command as MacOS will append 
> the
> full path of "$(SRC_PATH)/" before each found entry, then after the final "./"
> replacement trick it'll look like (e.g., "qapi/qmp-dispatch.c"):
>
>   /qapi/qmp-dispatch.c
>
> Which will point to the root directory instead.
>
> Fix it by simply remove the "/" in "$(SRC_PATH)/" of "find-src-path", then
> it'll work for at least both Linux and MacOS.
>
> The other OS-independent issue is to start proactively ignoring soft links 
> when
> generating tags, otherwise by default on master branch we'll see this error
> when "make cscope":
>
> cscope: cannot find file subprojects/libvhost-user/include/atomic.h
>
> This patch should fix the two issues altogether.
>
> Signed-off-by: Peter Xu 
> ---
>  Makefile | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index 401c623a65..5562a9b464 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -229,7 +229,8 @@ distclean: clean
>   rm -f linux-headers/asm
>   rm -Rf .sdk
>  
> -find-src-path = find "$(SRC_PATH)/" -path "$(SRC_PATH)/meson" -prune -o \( 
> -name "*.[chsS]" -o -name "*.[ch].inc" \)
> +find-src-path = find "$(SRC_PATH)" -path "$(SRC_PATH)/meson" -prune -o \
> + -type l -prune -o \( -name "*.[chsS]" -o -name "*.[ch].inc" \)

The second half of the change causes my "make gtags" to descend down
build directories and complain about unindexed files.

>  .PHONY: ctags
>  ctags:


-- 
Alex Bennée

[PATCH v2] accel/tcg/user-exec: Fix read-modify-write of code on s390 hosts

[Ilya Leoshkevich]

x86_64 dotnet/runtime uses cmpxchg for code patching. When running it
under s390x qemu-linux user, cpu_signal_handler() does not recognize
this as a write and does not restore PAGE_WRITE cleared by
tb_page_add(), incorrectly forwarding the signal to the guest code.

Signed-off-by: Ilya Leoshkevich 
---

v1: https://lists.nongnu.org/archive/html/qemu-devel/2021-08/msg00464.html
v1 -> v2: Fix comment style, fix CSST detection (Richard).

 accel/tcg/user-exec.c | 48 ---
 1 file changed, 41 insertions(+), 7 deletions(-)

diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 90d1a2d327..8fed542622 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -680,18 +680,26 @@ int cpu_signal_handler(int host_signum, void *pinfo,
 
 pc = uc->uc_mcontext.psw.addr;
 
-/* ??? On linux, the non-rt signal handler has 4 (!) arguments instead
-   of the normal 2 arguments.  The 3rd argument contains the "int_code"
-   from the hardware which does in fact contain the is_write value.
-   The rt signal handler, as far as I can tell, does not give this value
-   at all.  Not that we could get to it from here even if it were.  */
-/* ??? This is not even close to complete, since it ignores all
-   of the read-modify-write instructions.  */
+/*
+ * ??? On linux, the non-rt signal handler has 4 (!) arguments instead
+ * of the normal 2 arguments.  The 4th argument contains the "Translation-
+ * Exception Identification for DAT Exceptions" from the hardware (aka
+ * "int_parm_long"), which does in fact contain the is_write value.
+ * The rt signal handler, as far as I can tell, does not give this value
+ * at all.  Not that we could get to it from here even if it were.
+ * So fall back to parsing instructions.  Treat read-modify-write ones as
+ * writes, which is not fully correct, but for tracking self-modifying code
+ * this is better than treating them as reads.  Checking si_addr page flags
+ * might be a viable improvement, albeit a racy one.
+ */
+/* ??? This is not even close to complete.  */
 pinsn = (uint16_t *)pc;
 switch (pinsn[0] >> 8) {
 case 0x50: /* ST */
 case 0x42: /* STC */
 case 0x40: /* STH */
+case 0xba: /* CS */
+case 0xbb: /* CDS */
 is_write = 1;
 break;
 case 0xc4: /* RIL format insns */
@@ -702,6 +710,12 @@ int cpu_signal_handler(int host_signum, void *pinfo,
 is_write = 1;
 }
 break;
+case 0xc8: /* SSF format insns */
+switch (pinsn[0] & 0xf) {
+case 0x2: /* CSST */
+is_write = 1;
+}
+break;
 case 0xe3: /* RXY format insns */
 switch (pinsn[2] & 0xff) {
 case 0x50: /* STY */
@@ -715,7 +729,27 @@ int cpu_signal_handler(int host_signum, void *pinfo,
 is_write = 1;
 }
 break;
+case 0xeb: /* RSY format insns */
+switch (pinsn[2] & 0xff) {
+case 0x14: /* CSY */
+case 0x30: /* CSG */
+case 0x31: /* CDSY */
+case 0x3e: /* CDSG */
+case 0xe4: /* LANG */
+case 0xe6: /* LAOG */
+case 0xe7: /* LAXG */
+case 0xe8: /* LAAG */
+case 0xea: /* LAALG */
+case 0xf4: /* LAN */
+case 0xf6: /* LAO */
+case 0xf7: /* LAX */
+case 0xfa: /* LAAL */
+case 0xf8: /* LAA */
+is_write = 1;
+}
+break;
 }
+
 return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
 }
 
-- 
2.31.1

Re: [PATCH 0/3] Gitlab-CI improvements

[Alex Bennée]

Thomas Huth  writes:

> Here are three patches for some small issues that I noticed in our
> gitlab-CI files recently...

Queued to for-6.1/misc-fixes-for-rc2, thanks.

>
> Thomas Huth (3):
>   gitlab-ci: Merge "build-disabled" with "build-without-default-features"
>   gitlab-ci: Remove superfluous "dnf install" statement
>   gitlab-ci: Fix ..._RUNNER_AVAILABLE variables and document them
>
>  .gitlab-ci.d/buildtest.yml  | 99 +
>  .gitlab-ci.d/custom-runners.yml | 12 ++--
>  docs/devel/ci.rst   | 13 +
>  3 files changed, 32 insertions(+), 92 deletions(-)


-- 
Alex Bennée

[PATCH for-6.1?] util: Suppress -Wstringop-overflow in qemu_thread_start

[Richard Henderson]

This seems to be either a glibc or gcc bug, but the code
appears to be fine with the warning suppressed.

Signed-off-by: Richard Henderson 
---

The host is running Centos 7.9, so technically, this is out-of-support. 
But this is a gcc compile farm machine, so I'm stuck with it.  The rest
of qemu is still working fine with a locally refreshed compiler.


r~

---
 util/qemu-thread-posix.c | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index fd9d714038..6c5004220d 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -537,9 +537,28 @@ static void *qemu_thread_start(void *args)
 QEMU_TSAN_ANNOTATE_THREAD_NAME(qemu_thread_args->name);
 g_free(qemu_thread_args->name);
 g_free(qemu_thread_args);
+
+/*
+ * GCC 11 with glibc 2.17 on PowerPC reports
+ *
+ * qemu-thread-posix.c:540:5: error: ‘__sigsetjmp’ accessing 656 bytes
+ *   in a region of size 528 [-Werror=stringop-overflow=]
+ * 540 | pthread_cleanup_push(qemu_thread_atexit_notify, NULL);
+ * | ^~~~
+ *
+ * which is clearly nonsense.
+ */
+#pragma GCC diagnostic push
+#ifndef __clang__
+#pragma GCC diagnostic ignored "-Wstringop-overflow"
+#endif
+
 pthread_cleanup_push(qemu_thread_atexit_notify, NULL);
 r = start_routine(arg);
 pthread_cleanup_pop(1);
+
+#pragma GCC diagnostic pop
+
 return r;
 }
 
-- 
2.25.1

Re: [PATCH] accel/tcg/user-exec: Fix read-modify-write of code on s390 hosts

[Richard Henderson]

On 8/3/21 9:54 AM, Ilya Leoshkevich wrote:

  /* ??? On linux, the non-rt signal handler has 4 (!) arguments instead
-   of the normal 2 arguments.  The 3rd argument contains the "int_code"
-   from the hardware which does in fact contain the is_write value.
+   of the normal 2 arguments.  The 4th argument contains the "Translation-
+   Exception Identification for DAT Exceptions" from the hardware (aka
+   "int_parm_long"), which does in fact contain the is_write value.
 The rt signal handler, as far as I can tell, does not give this value
-   at all.  Not that we could get to it from here even if it were.  */
-/* ??? This is not even close to complete, since it ignores all
-   of the read-modify-write instructions.  */
+   at all.  Not that we could get to it from here even if it were.
+   So fall back to parsing instructions.  Treat read-modify-write ones as
+   writes, which is not fully correct, but for tracking self-modifying code
+   this is better than treating them as reads.  Checking si_addr page flags
+   might be a viable improvement, albeit a racy one.  */
+/* ??? This is not even close to complete.  */


You should have gotten a checkpatch warning here.
Just convert the comment to

  /*
   * this style
   */


  pinsn = (uint16_t *)pc;
  switch (pinsn[0] >> 8) {
  case 0x50: /* ST */
  case 0x42: /* STC */
  case 0x40: /* STH */
+case 0xba: /* CS */
+case 0xbb: /* CDS */
+case 0xc8: /* CSST */


CSST is format SSF; you're not checking enough bits to distinguish from LOAD 
PAIR DISJOINT.

Otherwise, looks good.

r~

Re: [PATCH v1] softmmu/physmem: fix wrong assertion in qemu_ram_alloc_internal()

[Peter Xu]

On Mon, Aug 02, 2021 at 05:22:38PM +0200, David Hildenbrand wrote:
> When adding RAM_NORESERVE, we forgot to remove the old assertion when
> adding the updated one, most probably when reworking the patches or
> rebasing. We can easily crash QEMU by adding
>   -object memory-backend-ram,id=mem0,size=500G,reserve=off
> to the QEMU cmdline:
>   qemu-system-x86_64: ../softmmu/physmem.c:2146: qemu_ram_alloc_internal:
>   Assertion `(ram_flags & ~(RAM_SHARED | RAM_RESIZEABLE | RAM_PREALLOC))
>   == 0' failed.
> 
> Fix it by removing the old assertion.
> 
> Fixes: 8dbe22c6868b ("memory: Introduce RAM_NORESERVE and wire it up in 
> qemu_ram_mmap()")
> Cc: Paolo Bonzini 
> Cc: Peter Xu 
> Cc: Philippe Mathieu-Daudé 
> Signed-off-by: David Hildenbrand 

Reviewed-by: Peter Xu 

-- 
Peter Xu

Re: [PATCH] vhost: use large iotlb entry if no IOMMU translation is needed

[Peter Xu]

On Tue, Aug 03, 2021 at 04:14:57PM +0800, Jason Wang wrote:
> 
> 在 2021/8/3 下午1:51, Chao Gao 写道:
> > On Tue, Aug 03, 2021 at 12:43:58PM +0800, Jason Wang wrote:
> > > 在 2021/8/3 下午12:29, Chao Gao 写道:
> > > > Ping. Could someone help to review this patch?
> > > > 
> > > > Thanks
> > > > Chao
> > > > 
> > > > On Wed, Jul 21, 2021 at 03:54:02PM +0800, Chao Gao wrote:
> > > > > If guest enables IOMMU_PLATFORM for virtio-net, severe network
> > > > > performance drop is observed even if there is no IOMMU.
> > > 
> > > We see such reports internally and we're testing a patch series to disable
> > > vhost IOTLB in this case.
> > > 
> > > Will post a patch soon.

[1]

> > OK. put me in the CC list. I would like to test with TDX to ensure your 
> > patch
> > fix the performance issue I am facing.
> 
> 
> Sure.
> 
> 
> > 
> > > 
> > > 
> > > > >And disabling
> > > > > vhost can mitigate the perf issue. Finally, we found the culprit is
> > > > > frequent iotlb misses: kernel vhost-net has 2048 entries and each
> > > > > entry is 4K (qemu uses 4K for i386 if no IOMMU); vhost-net can cache
> > > > > translations for up to 8M (i.e. 4K*2048) IOVAs. If guest uses >8M
> > > > > memory for DMA, there are some iotlb misses.
> > > > > 
> > > > > If there is no IOMMU or IOMMU is disabled or IOMMU works in pass-thru
> > > > > mode, we can optimistically use large, unaligned iotlb entries instead
> > > > > of 4K-aligned entries to reduce iotlb pressure.
> > > 
> > > Instead of introducing new general facilities like unaligned IOTLB entry. 
> > > I
> > > wonder if we optimize the vtd_iommu_translate() to use e.g 1G instead?
> > using 1G iotlb entry looks feasible.
> 
> 
> Want to send a patch?
> 
> 
> > 
> > >      } else {
> > >      /* DMAR disabled, passthrough, use 4k-page*/
> > >      iotlb.iova = addr & VTD_PAGE_MASK_4K;
> > >      iotlb.translated_addr = addr & VTD_PAGE_MASK_4K;
> > >      iotlb.addr_mask = ~VTD_PAGE_MASK_4K;
> > >      iotlb.perm = IOMMU_RW;
> > >      success = true;
> > >      }
> > > 
> > > 
> > > > >Actually, vhost-net
> > > > > in kernel supports unaligned iotlb entry. The alignment requirement is
> > > > > imposed by address_space_get_iotlb_entry() and 
> > > > > flatview_do_translate().
> > > 
> > > For the passthrough case, is there anyway to detect them and then disable
> > > device IOTLB in those case?
> > yes. I guess so; qemu knows the presence and status of iommu. Currently,
> > in flatview_do_translate(), memory_region_get_iommu() tells whether a memory
> > region is behind an iommu.
> 
> 
> The issues are:
> 
> 1) how to know the passthrough mode is enabled (note that passthrough mode
> doesn't mean it doesn't sit behind IOMMU)

memory_region_get_iommu() should return NULL if it's passthrough-ed?

> 2) can passthrough mode be disabled on the fly? If yes, we need to deal with
> them

I don't think it happens in reality; e.g. when iommu=pt is set it's set until
the next guest reboot.  However I don't know whether there's limitation from
spec-wise.  Also I don't know whether there's special cases, for example when
we kexec.

I've two questions..

Jason, when you mentioned the "fix" above [1], shouldn't that also fix the same
issue, and in a better way? Because ideally I think if we know vhost does not
need a translation for either iommu_platform=off, or passthrough, dev-iotlb
layer seems an overhead with no real use.

The other question is I'm also wondering why we care about iommu_platform=on
when there's no vIOMMU at all - it's about why we bother setting the flag at
all?  Or is it a valid use case?

Thanks,

-- 
Peter Xu

Re: [PATCH v5 0/2] plugins/cache: multicore cache modelling

[Alex Bennée]

Mahmoud Mandour  writes:

> Hello,
>
> This series introduce multicore cache modelling in contrib/plugins/cache.c
>
> Multi-core cache modelling is handled such that for full-system
> emulation, a private L1 cache is maintained to each core available to
> the system. For multi-threaded userspace emulation, a static number of
> cores is maintained for the overall system, and every memory access go
> through one of these, even if the number of fired threads is more than
> that number.

Queued to plugins/next, thanks.

-- 
Alex Bennée

Re: [PATCH] linux-user: fix guest/host address mixup in i386 setup_rt_frame()

[Richard Henderson]

On 8/3/21 7:18 AM, Ilya Leoshkevich wrote:

setup_rt_frame() passes siginfo and ucontext host addresses to guest
signal handlers, causing problems when e.g. emulating x86_64 on s390x.

Signed-off-by: Ilya Leoshkevich
---
  linux-user/i386/signal.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)


Reviewed-by: Richard Henderson 

r~

[PULL 1/5] hw/pcie-root-port: Fix hotplug for PCI devices requiring IO

[Michael S. Tsirkin]

From: Marcel Apfelbaum 

Q35 has now ACPI hotplug enabled by default for PCI(e) devices.
As opposed to native PCIe hotplug, guests like Fedora 34
will not assign IO range to pcie-root-ports not supporting
native hotplug, resulting into a regression.

Reproduce by:
qemu-bin -M q35 -device pcie-root-port,id=p1 -monitor stdio
device_add e1000,bus=p1
In the Guest OS the respective pcie-root-port will have the IO range
disabled.

Fix it by setting the "reserve-io" hint capability of the
pcie-root-ports so the firmware will allocate the IO range instead.

Acked-by: Igor Mammedov 
Signed-off-by: Marcel Apfelbaum 
Message-Id: <20210802090057.1709775-1-mar...@redhat.com>
Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Michael S. Tsirkin 
---
 hw/pci-bridge/gen_pcie_root_port.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/hw/pci-bridge/gen_pcie_root_port.c 
b/hw/pci-bridge/gen_pcie_root_port.c
index ec9907917e..20099a8ae3 100644
--- a/hw/pci-bridge/gen_pcie_root_port.c
+++ b/hw/pci-bridge/gen_pcie_root_port.c
@@ -28,6 +28,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(GenPCIERootPort, 
GEN_PCIE_ROOT_PORT)
 (GEN_PCIE_ROOT_PORT_AER_OFFSET + PCI_ERR_SIZEOF)
 
 #define GEN_PCIE_ROOT_PORT_MSIX_NR_VECTOR   1
+#define GEN_PCIE_ROOT_DEFAULT_IO_RANGE  4096
 
 struct GenPCIERootPort {
 /*< private >*/
@@ -75,6 +76,7 @@ static bool gen_rp_test_migrate_msix(void *opaque, int 
version_id)
 static void gen_rp_realize(DeviceState *dev, Error **errp)
 {
 PCIDevice *d = PCI_DEVICE(dev);
+PCIESlot *s = PCIE_SLOT(d);
 GenPCIERootPort *grp = GEN_PCIE_ROOT_PORT(d);
 PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(d);
 Error *local_err = NULL;
@@ -85,6 +87,9 @@ static void gen_rp_realize(DeviceState *dev, Error **errp)
 return;
 }
 
+if (grp->res_reserve.io == -1 && s->hotplug && !s->native_hotplug) {
+grp->res_reserve.io = GEN_PCIE_ROOT_DEFAULT_IO_RANGE;
+}
 int rc = pci_bridge_qemu_reserve_cap_init(d, 0,
   grp->res_reserve, errp);
 
-- 
MST

[PULL 5/5] Drop _DSM 5 from expected DSDTs on ARM

[Michael S. Tsirkin]

diff -rup /tmp/old/tests/data/acpi/microvm/DSDT.pcie.dsl 
/tmp/new/tests/data/acpi/microvm/DSDT.pcie.dsl
--- /tmp/old/tests/data/acpi/microvm/DSDT.pcie.dsl  2021-08-03 
16:22:52.289295442 -0400
+++ /tmp/new/tests/data/acpi/microvm/DSDT.pcie.dsl  2021-08-03 
16:22:40.102286317 -0400
@@ -1302,14 +1302,9 @@ DefinitionBlock ("", "DSDT", 2, "BOCHS "
 {
 Return (Buffer (One)
 {
- 0x21 
// !
+ 0x01 
// .
 })
 }
-
-If ((Arg2 == 0x05))
-{
-Return (Zero)
-}
 }

 Return (Buffer (One)

Signed-off-by: Michael S. Tsirkin 
---
 tests/qtest/bios-tables-test-allowed-diff.h |   5 -
 tests/data/acpi/microvm/DSDT.pcie   | Bin 3031 -> 3023 bytes
 tests/data/acpi/virt/DSDT   | Bin 5204 -> 5196 bytes
 tests/data/acpi/virt/DSDT.memhp | Bin 6565 -> 6557 bytes
 tests/data/acpi/virt/DSDT.numamem   | Bin 5204 -> 5196 bytes
 tests/data/acpi/virt/DSDT.pxb   | Bin 7695 -> 7679 bytes
 6 files changed, 5 deletions(-)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h 
b/tests/qtest/bios-tables-test-allowed-diff.h
index 8a7fe463c5..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,6 +1 @@
 /* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/virt/DSDT",
-"tests/data/acpi/virt/DSDT.memhp",
-"tests/data/acpi/virt/DSDT.numamem",
-"tests/data/acpi/virt/DSDT.pxb",
-"tests/data/acpi/microvm/DSDT.pcie",
diff --git a/tests/data/acpi/microvm/DSDT.pcie 
b/tests/data/acpi/microvm/DSDT.pcie
index 
3fb373fd970f0a33f30f57b1917720754396f0e9..765f14ef3d1e54d3cadccbf0a880f8adb73b3f1f
 100644
GIT binary patch
delta 51
zcmcaEeqNl*CD#U6GCFLIXMD`bp&RcK?8~x1ak3Y;JR{@e
HBJNZGj^hr8

delta 59
zcmX>veqEf)CDA8`aGj89g?~Gd||zFpYN!_GMY1IoXR_o>OrF
P`{XPx)+G#+v$#_M_<|6J

diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
index 
134d8ae5b602e0aaade6756b99c9abca45279284..c47503990715d389914fdf9c8bccb510761741ac
 100644
GIT binary patch
delta 50
zcmcbjaYlp7CD?thI$T+!B
G_%Q%n84Z^J

delta 58
zcmX@3aYcj6CD

diff --git a/tests/data/acpi/virt/DSDT.memhp b/tests/data/acpi/virt/DSDT.memhp
index 
140976b23ebea792bec12435a2a547ac5f5cd8f9..bae36cdd397473afe3923c52f030641a5ab19d5d
 100644
GIT binary patch
delta 52
zcmZ2#JlB}ZCDyi;GWjiE?8wQ*p&RcK?8~x1ak8hdJR{@g
ILSYj&0ETJ})&Kwi

delta 60
zcmbPhywsS>CDOrF
Q`{XPx)+G#^Glfmq0PR!{)&Kwi

diff --git a/tests/data/acpi/virt/DSDT.numamem 
b/tests/data/acpi/virt/DSDT.numamem
index 
134d8ae5b602e0aaade6756b99c9abca45279284..c47503990715d389914fdf9c8bccb510761741ac
 100644
GIT binary patch
delta 50
zcmcbjaYlp7CD?thI$T+!B
G_%Q%n84Z^J

delta 58
zcmX@3aYcj6CD

diff --git a/tests/data/acpi/virt/DSDT.pxb b/tests/data/acpi/virt/DSDT.pxb
index 
46b9c4cad5d65cb1b578410fb3168b70a05021be..fbd78f44c4785d19759daea909fe6d6f9a6e6b01
 100644
GIT binary patch
delta 78
zcmeCT`ESkT66_N4UzUM^Y5znn8OFOC)g?F?9XC60Hgj_5#=8XjvMf-Xd|F7Ji*bn{
YGb2NEli%{ie}uSD&QL^0Gn_Z9smFU

delta 94
zcmexw-EYI?66_MfFUP>ZG;bo84CB3x>Jprco|_#wn>jg5<6VM%Sr%wc{v#tVq_}{6
hauyfs5{4y$%!~}tO>Qd|e-YwBQNsyWGg(IVF#z_a8;k$|

-- 
MST

[PULL 3/5] arm/acpi: allow DSDT changes

[Michael S. Tsirkin]

We are going to commit ccee1a8140 ("acpi: Update _DSM method in expected 
files").
Allow changes to DSDT on ARM. Only configs with pci are
affected thus all virt variants but for microvm only the pcie variant.

Signed-off-by: Michael S. Tsirkin 
---
 tests/qtest/bios-tables-test-allowed-diff.h | 5 +
 1 file changed, 5 insertions(+)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h 
b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..8a7fe463c5 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,6 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/virt/DSDT",
+"tests/data/acpi/virt/DSDT.memhp",
+"tests/data/acpi/virt/DSDT.numamem",
+"tests/data/acpi/virt/DSDT.pxb",
+"tests/data/acpi/microvm/DSDT.pcie",
-- 
MST

Re: migration-test hang, s390x host, aarch64 guest

[Peter Xu]

On Mon, Aug 02, 2021 at 09:55:55AM +0100, Dr. David Alan Gilbert wrote:
> * Peter Maydell (peter.mayd...@linaro.org) wrote:
> > migration-test hung again during 'make check'.
> 
> ccing in Peter Xu
> 
> > Process tree:
> > 
> > ubuntu   42067  0.0  0.0   5460  3156 ?S13:55   0:00
> >\_ bash -o pipefail -c echo
> > 'MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> > QTEST_QEMU_IMG=./qemu-img
> > G_TEST_DBUS_DAEMON=/home/ubuntu/qemu/tests/dbus-vmstate-daemon.sh
> > QTEST_QEMU_BINARY=./qemu-system-aarch64
> > QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon
> > tests/qtest/migration-test --tap -k' &&
> > MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> > QTEST_QEMU_IMG=./qemu-img
> > G_TEST_DBUS_DAEMON=/home/ubuntu/qemu/tests/dbus-vmstate-daemon.sh
> > QTEST_QEMU_BINARY=./qemu-system-aarch64
> > QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon
> > tests/qtest/migration-test --tap -k -m quick < /dev/null |
> > ./scripts/tap-driver.pl --test-name="qtest-aarch64/migration-test"
> > ubuntu   42070  0.0  0.0  78844  3184 ?Sl   13:55   0:01
> >\_ tests/qtest/migration-test --tap -k -m quick
> > ubuntu   43248  0.0  4.1 1401368 160852 ?  Sl   13:55   0:03
> >|   \_ ./qemu-system-aarch64 -qtest
> > unix:/tmp/qtest-42070.sock -qtest-log /dev/null -chardev
> > socket,path=/tmp/qtest-42070.qmp,id=char0 -mon
> > chardev=char0,mode=control -display none -accel kvm -accel tcg
> > -machine virt,gic-version=max -name source,debug-threads=on -m 150M
> > -serial file:/tmp/migration-test-SL7EkN/src_serial -cpu max -kernel
> > /tmp/migration-test-SL7EkN/bootsect -accel qtest
> > ubuntu   43256  0.0  1.4 1394208 57648 ?   Sl   13:55   0:00
> >|   \_ ./qemu-system-aarch64 -qtest
> > unix:/tmp/qtest-42070.sock -qtest-log /dev/null -chardev
> > socket,path=/tmp/qtest-42070.qmp,id=char0 -mon
> > chardev=char0,mode=control -display none -accel kvm -accel tcg
> > -machine virt,gic-version=max -name target,debug-threads=on -m 150M
> > -serial file:/tmp/migration-test-SL7EkN/dest_serial -incoming
> > unix:/tmp/migration-test-SL7EkN/migsocket -cpu max -kernel
> > /tmp/migration-test-SL7EkN/bootsect -accel qtest
> > ubuntu   42071  0.0  0.2  14116 11372 ?S13:55   0:00
> >\_ perl ./scripts/tap-driver.pl
> > --test-name=qtest-aarch64/migration-test
> > 
> > Backtrace, migration-test process:
> > 
> > Thread 2 (Thread 0x3ff8ff7f910 (LWP 42075)):
> > #0  syscall () at ../sysdeps/unix/sysv/linux/s390/s390-64/syscall.S:53
> > #1  0x02aa1d0d1c1c in qemu_futex_wait (val=,
> > f=) at /home/ubuntu/qemu/include/qemu/futex.h:29
> > #2  qemu_event_wait (ev=ev@entry=0x2aa1d0fda58 )
> > at ../../util/qemu-thread-posix.c:480
> > #3  0x02aa1d0d0884 in call_rcu_thread (opaque=opaque@entry=0x0) at
> > ../../util/rcu.c:258
> > #4  0x02aa1d0d0c32 in qemu_thread_start (args=) at
> > ../../util/qemu-thread-posix.c:541
> > #5  0x03ff90207aa8 in start_thread (arg=0x3ff8ff7f910) at
> > pthread_create.c:463
> > #6  0x03ff900fa11e in thread_start () at
> > ../sysdeps/unix/sysv/linux/s390/s390-64/clone.S:65
> > 
> > Thread 1 (Thread 0x3ff905f7c00 (LWP 42070)):
> > #0  0x03ff90212978 in __waitpid (pid=,
> > stat_loc=stat_loc@entry=0x2aa1d5e06bc, options=options@entry=0)
> > at ../sysdeps/unix/sysv/linux/waitpid.c:30
> > #1  0x02aa1d0a2176 in qtest_kill_qemu (s=0x2aa1d5e06b0) at
> > ../../tests/qtest/libqtest.c:144
> > #2  0x03ff903c0de6 in g_hook_list_invoke () from
> > /usr/lib/s390x-linux-gnu/libglib-2.0.so.0
> > #3  
> > #4  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
> > #5  0x03ff9003f3ca in __GI_abort () at abort.c:79
> > #6  0x03ff903fcbb2 in g_assertion_message () from
> > /usr/lib/s390x-linux-gnu/libglib-2.0.so.0
> > #7  0x03ff903fd2b4 in g_assertion_message_cmpstr () from
> > /usr/lib/s390x-linux-gnu/libglib-2.0.so.0
> > #8  0x02aa1d0a10f6 in check_migration_status
> > (ungoals=0x3dfe198, goal=0x2aa1d0d75c0 "postcopy-paused",
> > who=0x2aa1d5dee90)
> 
> g_assert_cmpstr(current_status, !=,  *ungoal);
> 
> > at ../../tests/qtest/migration-helpers.c:146
> > #9  wait_for_migration_status (who=0x2aa1d5dee90, goal= > out>, ungoals=0x3dfe198)
> > at ../../tests/qtest/migration-helpers.c:156
> > #10 0x02aa1d09f610 in test_postcopy_recovery () at
> > ../../tests/qtest/migration-test.c:773
> 
> wait_for_migration_status(from, "postcopy-paused",
>   (const char * []) { "failed", "active",
>   "completed", NULL });
> 
> so I think that means it hit one of failed/active/completed rather than
> the postcopy-paused it expected (shame we can't tell which)
> 
> > #11 0x03ff903fc70c in ?? () from 
> > /usr/lib/s390x-linux-gnu/libglib-2.0.so.0
> > #12 0x03ff903fc648 in ?? () from 
> > /usr/lib

[PULL 4/5] Revert "acpi/gpex: Inform os to keep firmware resource map"

[Michael S. Tsirkin]

This reverts commit 0cf8882fd06ba0aeb1e90fa6f23fce85504d7e14.

Which this commit, with aarch64 when using efi PCI devices with IO ports
do not work.  The reason is that EFI creates I/O port mappings below
0x1000 (in fact, at 0). However Linux, for legacy reasons, does not
support I/O ports <= 0x1000 on PCI, so the I/O assignment created by EFI
is rejected.

EFI creates the mappings primarily for itself, and up until DSM #5
started to be enforced, all PCI resource allocations that existed at
boot were ignored by Linux and recreated from scratch.

Also, the commit in question looks dubious - it seems unlikely that
Linux would fail to create a resource tree. What does
happen is that BARs get moved around, which may cause trouble in some
cases: for instance, Linux had to add special code to the EFI framebuffer
driver to copy with framebuffer BARs being relocated.

DSM #5 has a long history of debate and misinterpretation.

Link: https://lore.kernel.org/r/20210724185234.ga2265...@roeck-us.net/
Fixes: 0cf8882fd06 ("acpi/gpex: Inform os to keep firmware resource map")
Reported-by: Guenter Roeck 
Suggested-by: Ard Biesheuvel 
Acked-by: Ard Biesheuvel 
Tested-by: Guenter Roeck 
Signed-off-by: Michael S. Tsirkin 
---
 hw/pci-host/gpex-acpi.c | 20 ++--
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
index 0f01f13a6e..e7e162a00a 100644
--- a/hw/pci-host/gpex-acpi.c
+++ b/hw/pci-host/gpex-acpi.c
@@ -112,26 +112,10 @@ static void acpi_dsdt_add_pci_osc(Aml *dev)
 UUID = aml_touuid("E5C937D0-3553-4D7A-9117-EA4D19C3434D");
 ifctx = aml_if(aml_equal(aml_arg(0), UUID));
 ifctx1 = aml_if(aml_equal(aml_arg(2), aml_int(0)));
-uint8_t byte_list[] = {
-0x1 << 0 /* support for functions other than function 0 */ |
-0x1 << 5 /* support for function 5 */
-};
-buf = aml_buffer(ARRAY_SIZE(byte_list), byte_list);
+uint8_t byte_list[1] = {1};
+buf = aml_buffer(1, byte_list);
 aml_append(ifctx1, aml_return(buf));
 aml_append(ifctx, ifctx1);
-
-/*
- * PCI Firmware Specification 3.1
- * 4.6.5. _DSM for Ignoring PCI Boot Configurations
- */
-/* Arg2: Function Index: 5 */
-ifctx1 = aml_if(aml_equal(aml_arg(2), aml_int(5)));
-/*
- * 0 - The operating system must not ignore the PCI configuration that
- * firmware has done at boot time.
- */
-aml_append(ifctx1, aml_return(aml_int(0)));
-aml_append(ifctx, ifctx1);
 aml_append(method, ifctx);
 
 byte_list[0] = 0;
-- 
MST

[PULL 2/5] acpi: x86: pcihp: add support hotplug on multifunction bridges

[Michael S. Tsirkin]

From: Igor Mammedov 

Commit [1] switched PCI hotplug from native to ACPI one by default.

That however breaks hotplug on following CLI that used to work:
   -nodefaults -machine q35 \
   -device 
pcie-root-port,id=pcie-root-port-0,multifunction=on,bus=pcie.0,addr=0x1,chassis=1
 \
   -device 
pcie-root-port,id=pcie-root-port-1,port=0x1,addr=0x1.0x1,bus=pcie.0,chassis=2

where PCI device is hotplugged to pcie-root-port-1 with error on guest side:

  ACPI BIOS Error (bug): Could not resolve symbol [^S0B.PCNT], AE_NOT_FOUND 
(20201113/psargs-330)
  ACPI Error: Aborting method \_SB.PCI0.PCNT due to previous error 
(AE_NOT_FOUND) (20201113/psparse-531)
  ACPI Error: Aborting method \_GPE._E01 due to previous error (AE_NOT_FOUND) 
(20201113/psparse-531)
  ACPI Error: AE_NOT_FOUND, while evaluating GPE method [_E01] 
(20201113/evgpe-515)

cause is that QEMU's ACPI hotplug never supported functions other then 0
and due to bug it was generating notification entries for not described
functions.

Technically there is no reason not to describe cold-plugged bridges
(root ports) on functions other then 0, as they similarly to bridge
on function 0 are unpluggable.

So since we need to describe multifunction devices iterate over
fuctions as well. But describe only cold-plugged bridges[root ports]
on functions other than 0 as well.

1)
Fixes: 17858a169508609ca9063c544833e5a1adeb7b52 (hw/acpi/ich9: Set ACPI PCI 
hot-plug as default on Q35)
Signed-off-by: Igor Mammedov 
Reported-by: Laurent Vivier 
Message-Id: <20210723090424.2092226-1-imamm...@redhat.com>
Fixes: 17858a169508609ca9063c544833e5a1adeb7b52 (hw/acpi/ich9: Set ACPI PCI 
hot-plug as default on Q35)
Signed-off-by: Igor Mammedov imamm...@redhat.com>
Reported-by: Laurent Vivier lviv...@redhat.com>
Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Michael S. Tsirkin 
---
 hw/i386/acpi-build.c | 44 ++--
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 17836149fe..a33ac8b91e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -374,7 +374,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, 
PCIBus *bus,
 Aml *dev, *notify_method = NULL, *method;
 QObject *bsel;
 PCIBus *sec;
-int i;
+int devfn;
 
 bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, 
NULL);
 if (bsel) {
@@ -384,23 +384,31 @@ static void build_append_pci_bus_devices(Aml 
*parent_scope, PCIBus *bus,
 notify_method = aml_method("DVNT", 2, AML_NOTSERIALIZED);
 }
 
-for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
+for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {
 DeviceClass *dc;
 PCIDeviceClass *pc;
-PCIDevice *pdev = bus->devices[i];
-int slot = PCI_SLOT(i);
+PCIDevice *pdev = bus->devices[devfn];
+int slot = PCI_SLOT(devfn);
+int func = PCI_FUNC(devfn);
+/* ACPI spec: 1.0b: Table 6-2 _ADR Object Bus Types, PCI type */
+int adr = slot << 16 | func;
 bool hotplug_enabled_dev;
 bool bridge_in_acpi;
 bool cold_plugged_bridge;
 
 if (!pdev) {
-if (bsel) { /* add hotplug slots for non present devices */
+/*
+ * add hotplug slots for non present devices.
+ * hotplug is supported only for non-multifunction device
+ * so generate device description only for function 0
+ */
+if (bsel && !func) {
 if (pci_bus_is_express(bus) && slot > 0) {
 break;
 }
-dev = aml_device("S%.02X", PCI_DEVFN(slot, 0));
+dev = aml_device("S%.02X", devfn);
 aml_append(dev, aml_name_decl("_SUN", aml_int(slot)));
-aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16)));
+aml_append(dev, aml_name_decl("_ADR", aml_int(adr)));
 method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
 aml_append(method,
 aml_call2("PCEJ", aml_name("BSEL"), aml_name("_SUN"))
@@ -436,9 +444,18 @@ static void build_append_pci_bus_devices(Aml 
*parent_scope, PCIBus *bus,
 continue;
 }
 
-/* start to compose PCI slot descriptor */
-dev = aml_device("S%.02X", PCI_DEVFN(slot, 0));
-aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16)));
+/*
+ * allow describing coldplugged bridges in ACPI even if they are not
+ * on function 0, as they are not unpluggable, for all other devices
+ * generate description only for function 0 per slot
+ */
+if (func && !bridge_in_acpi) {
+continue;
+}
+
+/* start to compose PCI device descriptor */
+dev = aml_device("S%.02X", devfn);
+  

[PULL 0/5] pc,pci: bugfixes

[Michael S. Tsirkin]

The following changes since commit f2da205cb4142259d9bc6b9d4596ebbe2426fe49:

  Update version for v6.1.0-rc1 release (2021-07-27 18:07:52 +0100)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream

for you to fetch changes up to 62a4db5522cbbd8b5309a2949c22f00e5b0138e3:

  Drop _DSM 5 from expected DSDTs on ARM (2021-08-03 16:32:35 -0400)


pc,pci: bugfixes

Small bugfixes all over the place.

Signed-off-by: Michael S. Tsirkin 


Igor Mammedov (1):
  acpi: x86: pcihp: add support hotplug on multifunction bridges

Marcel Apfelbaum (1):
  hw/pcie-root-port: Fix hotplug for PCI devices requiring IO

Michael S. Tsirkin (3):
  arm/acpi: allow DSDT changes
  Revert "acpi/gpex: Inform os to keep firmware resource map"
  Drop _DSM 5 from expected DSDTs on ARM

 hw/i386/acpi-build.c   |  44 +
 hw/pci-bridge/gen_pcie_root_port.c |   5 +
 hw/pci-host/gpex-acpi.c|  20 ++---
 tests/data/acpi/microvm/DSDT.pcie  | Bin 3031 -> 3023 bytes
 tests/data/acpi/virt/DSDT  | Bin 5204 -> 5196 bytes
 tests/data/acpi/virt/DSDT.memhp| Bin 6565 -> 6557 bytes
 tests/data/acpi/virt/DSDT.numamem  | Bin 5204 -> 5196 bytes
 tests/data/acpi/virt/DSDT.pxb  | Bin 7695 -> 7679 bytes
 8 files changed, 37 insertions(+), 32 deletions(-)

Re: [PATCH 2/2] Acceptance Tests: updates to the MAINTAINERS file

[Willian Rampazzo]

On Tue, Aug 3, 2021 at 4:35 PM Cleber Rosa  wrote:
>
> The tests/acceptance directory is currently lacking a maintainer
> entry, even though I've been performing that role (of course with help
> from many others).  Thus, its status is, even more now, Maintained.
>
> This also removes the currently broken Trello board link, which was
> make unavailable unintentionally by a third party.
>
> Signed-off-by: Cleber Rosa 
> ---
>  MAINTAINERS | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>

Reviewed-by: Willian Rampazzo 

Re: [PATCH 1/2] Acceptance Tests: add standard clean up at test tearDown()

[Willian Rampazzo]

On Tue, Aug 3, 2021 at 4:35 PM Cleber Rosa  wrote:
>
> The avocado.Test class, used as the basis of the avocado_qemu.Test
> class, performs a clean of temporary directories up as part of its own
> tearDown() implementation.
>
> But the avocado_qemu.Test class is currently missing the same clean
> up, as it implemented its own tearDown() method without resorting to
> the upper class behavior.
>
> This brings avocado_qemu.Test behavior in sync with the standard
> avocado.Test behavior and prevents temporary directories from
> cluttering the test results directory (unless instructed to do so with
> Avocado's "--keep-tmp" option).
>
> Reported-by: Peter Maydell 
> Signed-off-by: Cleber Rosa 
> ---
>  tests/acceptance/avocado_qemu/__init__.py | 1 +
>  1 file changed, 1 insertion(+)
>

Reviewed-by: Willian Rampazzo 

[PATCH] accel/tcg/user-exec: Fix read-modify-write of code on s390 hosts

[Ilya Leoshkevich]

x86_64 dotnet/runtime uses cmpxchg for code patching. When running it
under s390x qemu-linux user, cpu_signal_handler() does not recognize
this as a write and does not restore PAGE_WRITE cleared by
tb_page_add(), incorrectly forwarding the signal to the guest code.

Signed-off-by: Ilya Leoshkevich 
---
 accel/tcg/user-exec.c | 37 -
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 90d1a2d327..587a0f1ef9 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -681,17 +681,24 @@ int cpu_signal_handler(int host_signum, void *pinfo,
 pc = uc->uc_mcontext.psw.addr;
 
 /* ??? On linux, the non-rt signal handler has 4 (!) arguments instead
-   of the normal 2 arguments.  The 3rd argument contains the "int_code"
-   from the hardware which does in fact contain the is_write value.
+   of the normal 2 arguments.  The 4th argument contains the "Translation-
+   Exception Identification for DAT Exceptions" from the hardware (aka
+   "int_parm_long"), which does in fact contain the is_write value.
The rt signal handler, as far as I can tell, does not give this value
-   at all.  Not that we could get to it from here even if it were.  */
-/* ??? This is not even close to complete, since it ignores all
-   of the read-modify-write instructions.  */
+   at all.  Not that we could get to it from here even if it were.
+   So fall back to parsing instructions.  Treat read-modify-write ones as
+   writes, which is not fully correct, but for tracking self-modifying code
+   this is better than treating them as reads.  Checking si_addr page flags
+   might be a viable improvement, albeit a racy one.  */
+/* ??? This is not even close to complete.  */
 pinsn = (uint16_t *)pc;
 switch (pinsn[0] >> 8) {
 case 0x50: /* ST */
 case 0x42: /* STC */
 case 0x40: /* STH */
+case 0xba: /* CS */
+case 0xbb: /* CDS */
+case 0xc8: /* CSST */
 is_write = 1;
 break;
 case 0xc4: /* RIL format insns */
@@ -715,7 +722,27 @@ int cpu_signal_handler(int host_signum, void *pinfo,
 is_write = 1;
 }
 break;
+case 0xeb: /* RSY format insns */
+switch (pinsn[2] & 0xff) {
+case 0x14: /* CSY */
+case 0x30: /* CSG */
+case 0x31: /* CDSY */
+case 0x3e: /* CDSG */
+case 0xe4: /* LANG */
+case 0xe6: /* LAOG */
+case 0xe7: /* LAXG */
+case 0xe8: /* LAAG */
+case 0xea: /* LAALG */
+case 0xf4: /* LAN */
+case 0xf6: /* LAO */
+case 0xf7: /* LAX */
+case 0xfa: /* LAAL */
+case 0xf8: /* LAA */
+is_write = 1;
+}
+break;
 }
+
 return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
 }
 
-- 
2.31.1

[PATCH v7] tests/tcg/s390x: Test SIGILL and SIGSEGV handling

[Ilya Leoshkevich]

Verify that s390x-specific uc_mcontext.psw.addr is reported correctly
and that signal handling interacts properly with debugging.

Signed-off-by: Ilya Leoshkevich 
---

v6: https://lists.nongnu.org/archive/html/qemu-devel/2021-07/msg00873.html
v6 -> v7: Rebased.

 tests/tcg/s390x/Makefile.target   |  16 +-
 tests/tcg/s390x/gdbstub/test-signals-s390x.py |  76 
 tests/tcg/s390x/signals-s390x.c   | 165 ++
 3 files changed, 256 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/s390x/gdbstub/test-signals-s390x.py
 create mode 100644 tests/tcg/s390x/signals-s390x.c

diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index 5d3de1b27a..5d984294f6 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -1,4 +1,5 @@
-VPATH+=$(SRC_PATH)/tests/tcg/s390x
+S390X_SRC=$(SRC_PATH)/tests/tcg/s390x
+VPATH+=$(S390X_SRC)
 CFLAGS+=-march=zEC12 -m64
 TESTS+=hello-s390x
 TESTS+=csst
@@ -8,4 +9,17 @@ TESTS+=exrl-trtr
 TESTS+=pack
 TESTS+=mvo
 TESTS+=mvc
+TESTS+=signals-s390x
 
+ifneq ($(HAVE_GDB_BIN),)
+GDB_SCRIPT=$(SRC_PATH)/tests/guest-debug/run-test.py
+
+run-gdbstub-signals-s390x: signals-s390x
+   $(call run-test, $@, $(GDB_SCRIPT) \
+   --gdb $(HAVE_GDB_BIN) \
+   --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
+   --bin $< --test $(S390X_SRC)/gdbstub/test-signals-s390x.py, \
+   "mixing signals and debugging on s390x")
+
+EXTRA_RUNS += run-gdbstub-signals-s390x
+endif
diff --git a/tests/tcg/s390x/gdbstub/test-signals-s390x.py 
b/tests/tcg/s390x/gdbstub/test-signals-s390x.py
new file mode 100644
index 00..80a284b475
--- /dev/null
+++ b/tests/tcg/s390x/gdbstub/test-signals-s390x.py
@@ -0,0 +1,76 @@
+from __future__ import print_function
+
+#
+# Test that signals and debugging mix well together on s390x.
+#
+# This is launched via tests/guest-debug/run-test.py
+#
+
+import gdb
+import sys
+
+failcount = 0
+
+
+def report(cond, msg):
+"""Report success/fail of test"""
+if cond:
+print("PASS: %s" % (msg))
+else:
+print("FAIL: %s" % (msg))
+global failcount
+failcount += 1
+
+
+def run_test():
+"""Run through the tests one by one"""
+illegal_op = gdb.Breakpoint("illegal_op")
+stg = gdb.Breakpoint("stg")
+mvc_8 = gdb.Breakpoint("mvc_8")
+
+# Expect the following events:
+# 1x illegal_op breakpoint
+# 2x stg breakpoint, segv, breakpoint
+# 2x mvc_8 breakpoint, segv, breakpoint
+for _ in range(14):
+gdb.execute("c")
+report(illegal_op.hit_count == 1, "illegal_op.hit_count == 1")
+report(stg.hit_count == 4, "stg.hit_count == 4")
+report(mvc_8.hit_count == 4, "mvc_8.hit_count == 4")
+
+# The test must succeed.
+gdb.Breakpoint("_exit")
+gdb.execute("c")
+status = int(gdb.parse_and_eval("$r2"))
+report(status == 0, "status == 0");
+
+
+#
+# This runs as the script it sourced (via -x, via run-test.py)
+#
+try:
+inferior = gdb.selected_inferior()
+arch = inferior.architecture()
+print("ATTACHED: %s" % arch.name())
+except (gdb.error, AttributeError):
+print("SKIPPING (not connected)", file=sys.stderr)
+exit(0)
+
+if gdb.parse_and_eval("$pc") == 0:
+print("SKIP: PC not set")
+exit(0)
+
+try:
+# These are not very useful in scripts
+gdb.execute("set pagination off")
+gdb.execute("set confirm off")
+
+# Run the actual tests
+run_test()
+except (gdb.error):
+print("GDB Exception: %s" % (sys.exc_info()[0]))
+failcount += 1
+pass
+
+print("All tests complete: %d failures" % failcount)
+exit(failcount)
diff --git a/tests/tcg/s390x/signals-s390x.c b/tests/tcg/s390x/signals-s390x.c
new file mode 100644
index 00..dc2f8ee59a
--- /dev/null
+++ b/tests/tcg/s390x/signals-s390x.c
@@ -0,0 +1,165 @@
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/*
+ * Various instructions that generate SIGILL and SIGSEGV. They could have been
+ * defined in a separate .s file, but this would complicate the build, so the
+ * inline asm is used instead.
+ */
+
+void illegal_op(void);
+void after_illegal_op(void);
+asm(".globl\tillegal_op\n"
+"illegal_op:\t.byte\t0x00,0x00\n"
+"\t.globl\tafter_illegal_op\n"
+"after_illegal_op:\tbr\t%r14");
+
+void stg(void *dst, unsigned long src);
+asm(".globl\tstg\n"
+"stg:\tstg\t%r3,0(%r2)\n"
+"\tbr\t%r14");
+
+void mvc_8(void *dst, void *src);
+asm(".globl\tmvc_8\n"
+"mvc_8:\tmvc\t0(8,%r2),0(%r3)\n"
+"\tbr\t%r14");
+
+static void safe_puts(const char *s)
+{
+write(0, s, strlen(s));
+write(0, "\n", 1);
+}
+
+enum exception {
+exception_operation,
+exception_translation,
+exception_protection,
+};
+
+static struct {
+int sig;
+void *addr;
+unsigned long psw_addr;
+enum exception exception;
+} expected;
+
+static void handle_signal(int sig, siginfo_t *info, void *ucontext)
+{
+void *page;
+int err;
+
+i

[PATCH 2/2] Acceptance Tests: updates to the MAINTAINERS file

[Cleber Rosa]

The tests/acceptance directory is currently lacking a maintainer
entry, even though I've been performing that role (of course with help
from many others).  Thus, its status is, even more now, Maintained.

This also removes the currently broken Trello board link, which was
make unavailable unintentionally by a third party.

Signed-off-by: Cleber Rosa 
---
 MAINTAINERS | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 37b1a8e442..d35b948e8d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3418,11 +3418,11 @@ F: tests/tcg/Makefile
 F: tests/tcg/Makefile.include
 
 Acceptance (Integration) Testing with the Avocado framework
-W: https://trello.com/b/6Qi1pxVn/avocado-qemu
-R: Cleber Rosa 
+M: Cleber Rosa 
 R: Philippe Mathieu-Daudé 
 R: Wainer dos Santos Moschetta 
-S: Odd Fixes
+S: Maintained
+F: tests/Makefile.include
 F: tests/acceptance/
 
 Documentation
-- 
2.31.1

[PATCH 0/2] Acceptance Tests: clean up of temporary dirs and MAINTAINERS entry

[Cleber Rosa]

This is a reply to an issue[1] reported by Peter, and while at it, an
update of the MAINTAINERS entry so other people now the status and
where to go for help with regards to the acceptance tests'
infrastructure.

[1] https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg00349.html

Cleber Rosa (2):
  Acceptance Tests: add standard clean up at test tearDown()
  Acceptance Tests: updates to the MAINTAINERS file

 MAINTAINERS   | 6 +++---
 tests/acceptance/avocado_qemu/__init__.py | 1 +
 2 files changed, 4 insertions(+), 3 deletions(-)

-- 
2.31.1

[PATCH 1/2] Acceptance Tests: add standard clean up at test tearDown()

[Cleber Rosa]

The avocado.Test class, used as the basis of the avocado_qemu.Test
class, performs a clean of temporary directories up as part of its own
tearDown() implementation.

But the avocado_qemu.Test class is currently missing the same clean
up, as it implemented its own tearDown() method without resorting to
the upper class behavior.

This brings avocado_qemu.Test behavior in sync with the standard
avocado.Test behavior and prevents temporary directories from
cluttering the test results directory (unless instructed to do so with
Avocado's "--keep-tmp" option).

Reported-by: Peter Maydell 
Signed-off-by: Cleber Rosa 
---
 tests/acceptance/avocado_qemu/__init__.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/acceptance/avocado_qemu/__init__.py 
b/tests/acceptance/avocado_qemu/__init__.py
index 2c4fef3e14..1e807e2e55 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -276,6 +276,7 @@ def tearDown(self):
 for vm in self._vms.values():
 vm.shutdown()
 self._sd = None
+super(Test, self).tearDown()
 
 def fetch_asset(self, name,
 asset_hash=None, algorithm=None,
-- 
2.31.1

Re: [PATCH v2 0/1] Add remote I2C device to support external I2C device

[Corey Minyard]

On Mon, Aug 02, 2021 at 11:03:22PM +, Shengtan Mao wrote:
> This patch implements the remote I2C device.
> The remote I2C device allows an external I2C device to communicate with the 
> I2C controller in QEMU through the remote I2C protocol.
> Users no longer have to directly modify QEMU to add new I2C devices and can 
> instead implement the emulated device externally and connect it to the remote 
> I2C device.

I apologise, I haven't had time to look at this, and I'm going to be
really busy for a little while.

I looked it over a bit, and is there some description of the protocol?
Could you add a reference to it in the code?

-corey

> 
> Previous work by Wolfram Sang 
> (https://git.kernel.org/pub/scm/virt/qemu/wsa/qemu.git/commit/?h=i2c-passthrough)
>  was referenced.
> It shares the similar idea of redirecting the actual I2C device 
> functionalities, but Sang focuses on physical devices, and we focus on 
> emulated devices.
> The work by Sang mainly utilizes file descriptors while ours utilizes 
> character devices, which offers better support for emulated devices.
> The work by Sang is not meant to offer full I2C device support; it only 
> implements the receive functionality.
> Our work implements full support for I2C devices: send, recv, and event 
> (match_and_add is not applicable for external devices).
> 
> Shengtan Mao (1):
>   hw/i2c: add remote I2C device
> 
>  hw/arm/Kconfig|   1 +
>  hw/i2c/Kconfig|   4 +
>  hw/i2c/meson.build|   1 +
>  hw/i2c/remote-i2c.c   | 117 ++
>  tests/qtest/meson.build   |   1 +
>  tests/qtest/remote-i2c-test.c | 216 ++
>  6 files changed, 340 insertions(+)
>  create mode 100644 hw/i2c/remote-i2c.c
>  create mode 100644 tests/qtest/remote-i2c-test.c
> 
> -- 
> 2.32.0.554.ge1b32706d8-goog
> 

Re: Re: [RFC v3] virtio/vsock: add two more queues for datagram types

[Jiang Wang .]

On Wed, Jul 7, 2021 at 10:27 AM Stefano Garzarella  wrote:
>
> On Wed, Jul 07, 2021 at 09:52:46AM -0700, Jiang Wang . wrote:
> >On Wed, Jul 7, 2021 at 1:33 AM Stefano Garzarella  
> >wrote:
> >>
> >> On Tue, Jul 06, 2021 at 10:26:07PM +, Jiang Wang wrote:
> >> >Datagram sockets are connectionless and unreliable.
> >> >The sender does not know the capacity of the receiver
> >> >and may send more packets than the receiver can handle.
> >> >
> >> >Add two more dedicate virtqueues for datagram sockets,
> >> >so that it will not unfairly steal resources from
> >> >stream and future connection-oriented sockets.
> >> >---
> >> >v1 -> v2: use qemu cmd option to control number of queues,
> >> >removed configuration settings for dgram.
> >> >v2 -> v3: use ioctl to get features and decie numbr of
> >> >   virt queues, instead of qemu cmd option.
> >> >
> >> >btw: this patch is based on Arseny's SEQPACKET patch.
> >> >
> >> > hw/virtio/vhost-vsock-common.c| 53 
> >> > ++-
> >> > hw/virtio/vhost-vsock.c   |  3 ++
> >> > include/hw/virtio/vhost-vsock-common.h|  3 +-
> >> > include/hw/virtio/vhost-vsock.h   |  4 ++
> >> > include/standard-headers/linux/virtio_vsock.h |  3 ++
> >> > 5 files changed, 63 insertions(+), 3 deletions(-)
> >> >
> >> >diff --git a/hw/virtio/vhost-vsock-common.c 
> >> >b/hw/virtio/vhost-vsock-common.c
> >> >index 4ad6e234ad..8164e09445 100644
> >> >--- a/hw/virtio/vhost-vsock-common.c
> >> >+++ b/hw/virtio/vhost-vsock-common.c
> >> >@@ -17,6 +17,8 @@
> >> > #include "hw/virtio/vhost-vsock.h"
> >> > #include "qemu/iov.h"
> >> > #include "monitor/monitor.h"
> >> >+#include 
> >> >+#include 
> >> >
> >> > int vhost_vsock_common_start(VirtIODevice *vdev)
> >> > {
> >> >@@ -196,9 +198,36 @@ int vhost_vsock_common_post_load(void *opaque, int 
> >> >version_id)
> >> > return 0;
> >> > }
> >> >
> >> >+static int vhost_vsock_get_max_qps(void)
> >> >+{
> >> >+uint64_t features;
> >> >+int ret;
> >> >+int fd = -1;
> >> >+
> >> >+fd = qemu_open_old("/dev/vhost-vsock", O_RDONLY);
> >> >+if (fd == -1) {
> >> >+error_report("vhost-vsock: failed to open device. %s", 
> >> >strerror(errno));
> >> >+return -1;
> >> >+}
> >>
> >> You should use the `vhostfd` already opened in
> >> vhost_vsock_device_realize(), since QEMU may not have permission to
> >> access to the device, and the file descriptor can be passed from the
> >> management layer.
> >>
> >Sure. Will do.
> >
> >> >+
> >> >+ret = ioctl(fd, VHOST_GET_FEATURES, &features);
> >> >+if (ret) {
> >> >+error_report("vhost-vsock: failed to read  device. %s", 
> >> >strerror(errno));
> >> >+qemu_close(fd);
> >> >+return ret;
> >> >+}
> >> >+
> >> >+qemu_close(fd);
> >> >+if (features & (1 << VIRTIO_VSOCK_F_DGRAM))
> >> >+return MAX_VQS_WITH_DGRAM;
> >> >+
> >> >+return MAX_VQS_WITHOUT_DGRAM;
> >> >+}
> >> >+
> >> > void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name)
> >> > {
> >> > VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> >> >+int nvqs = MAX_VQS_WITHOUT_DGRAM;
> >> >
> >> > virtio_init(vdev, name, VIRTIO_ID_VSOCK,
> >> > sizeof(struct virtio_vsock_config));
> >> >@@ -209,12 +238,24 @@ void vhost_vsock_common_realize(VirtIODevice
> >> >*vdev, const char *name)
> >> > vvc->trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> >> >vhost_vsock_common_handle_output);
> >> >
> >> >+nvqs = vhost_vsock_get_max_qps();
> >>
> >> You can't do this here, since the vhost-vsock-common.c functions are
> >> used also by vhost-user-vsock, that doesn't use the /dev/vhost-vsock
> >> device since the device is emulated in a separate user process.
> >>
> >> I think you can use something similar to what you did in v2, where you
> >> passed a parameter to vhost_vsock_common_realize() to enable or not the
> >> datagram queues.
> >>
> >Just to make sure, the qemu parameter will only be used for vhost-user-vsock,
> >right? I think for the vhost-vsock kernel module, we will use ioctl and 
> >ignore
> >the qemu parameter?
>
> No, I mean a function parameter in vhost_vsock_common_realize() that we
> set to true when datagram is supported by the backend.
>
> You can move the vhost_vsock_get_max_qps() call in
> vhost_vsock_device_realize(), just before call
> vhost_vsock_common_realize() where we can pass a parameter to specify if
> datagram is supported or not.
>
> For now in vhost-user-vsock you can always pass `false`. When we will
> support it, we can add something similar to discover the features.
>
> Just to be clear, we don't need any QEMU command line parameter.
>
> >
> >> >+
> >> >+if (nvqs < 0)
> >> >+nvqs = MAX_VQS_WITHOUT_DGRAM;
> >> >+
> >> >+if (nvqs == MAX_VQS_WITH_DGRAM) {
> >> >+vvc->dgram_recv_vq = virtio_add_queue(vdev, 
> >> >VHOST_VSOCK_QUEUE_SIZE,

Re: [PULL v4 0/1] Libslirp patches

[Peter Maydell]

On Tue, 3 Aug 2021 at 15:18,  wrote:
>
> From: Marc-André Lureau 
>
> The following changes since commit 7f1cab9c628a798ae2607940993771e6300e9e00:
>
>   Merge remote-tracking branch 'remotes/bonzini-gitlab/tags/for-upstream' 
> into staging (2021-08-02 17:21:50 +0100)
>
> are available in the Git repository at:
>
>   g...@github.com:elmarco/qemu.git tags/libslirp-pull-request
>
> for you to fetch changes up to 43f547b73dfaf108c9aaacb0b36200e2e7a200f1:
>
>   Update libslirp to v4.6.1 (2021-08-03 16:07:22 +0400)
>
> 
> Update libslirp
>
> Hi,
>
> v4:
>  - drop subproject patch
>  - fix OSX linking issue
>
> v3:
>  - rebased
>  - (checked compilation with P. Maydell extra-cflags reported failure & 
> gitlab CI)
>
> v2:
>  - fix unused variables on macos
>  - fork_exec_child_setup: improve signal handling
>


Applied, thanks.

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

-- PMM

[PATCH v3 24/25] python: bump avocado to v90.0

[John Snow]

Avocado v90 includes improved support for running async unit tests. The
workaround that existed prior to v90 causes the unit tests to fail
afterwards, however, so upgrade our minimum version pin to the very
latest and greatest.

Signed-off-by: John Snow 
---
 python/Pipfile.lock | 8 
 python/setup.cfg| 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 8ab41a3f606..457f5c3fe87 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -1,7 +1,7 @@
 {
 "_meta": {
 "hash": {
-"sha256": 
"eff562a688ebc6f3ffe67494dbb804b883e2159ad81c4d55d96da9f7aec13e91"
+"sha256": 
"784b327272db32403d5a488507853b5afba850ba26a5948e5b6a90c1baef2d9c"
 },
 "pipfile-spec": 6,
 "requires": {
@@ -39,11 +39,11 @@
 },
 "avocado-framework": {
 "hashes": [
-
"sha256:3fca7226d7d164f124af8a741e7fa658ff4345a0738ddc32907631fd688b38ed",
-
"sha256:48ac254c0ae2ef0c0ceeb38e3d3df0388718eda8f48b3ab55b30b252839f42b1"
+
"sha256:244cb569f8eb4e50a22ac82e1a2b2bba2458999f4281efbe2651bd415d59c65b",
+
"sha256:6f15998b67ecd0e7dde790c4de4dd249d6df52dfe6d5cc4e2dd6596df51c3583"
 ],
 "index": "pypi",
-"version": "==87.0"
+"version": "==90.0"
 },
 "distlib": {
 "hashes": [
diff --git a/python/setup.cfg b/python/setup.cfg
index 2573cd7bfb3..077395f96e1 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -37,7 +37,7 @@ packages =
 # version, use e.g. "pipenv install --dev pylint==3.0.0".
 # Subsequently, edit 'Pipfile' to remove e.g. 'pylint = "==3.0.0'.
 devel =
-avocado-framework >= 87.0
+avocado-framework >= 90.0
 flake8 >= 3.6.0
 fusepy >= 2.0.4
 isort >= 5.1.2
-- 
2.31.1

[PATCH v3 23/25] python/aqmp: add scary message

[John Snow]

Add a warning whenever AQMP is used to steer people gently away from
using it for the time-being.

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

diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index 4b7df53e006..ab1782999cf 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -21,6 +21,8 @@
 # This work is licensed under the terms of the GNU GPL, version 2.  See
 # the COPYING file in the top-level directory.
 
+import warnings
+
 from .error import AQMPError
 from .events import EventListener
 from .message import Message
@@ -28,6 +30,18 @@
 from .qmp_client import ExecInterruptedError, ExecuteError, QMPClient
 
 
+_WMSG = """
+
+The Asynchronous QMP library is currently in development and its API
+should be considered highly fluid and subject to change. It should
+not be used by any other scripts checked into the QEMU tree.
+
+Proceed with caution!
+"""
+
+warnings.warn(_WMSG, FutureWarning)
+
+
 # The order of these fields impact the Sphinx documentation order.
 __all__ = (
 # Classes, most to least important
-- 
2.31.1

[PATCH v3 22/25] python/aqmp: add asyncio_run compatibility wrapper

[John Snow]

As a convenience. It isn't used by the library itself, but it is used by
the test suite. It will also come in handy for users of the library
still on Python 3.6.

Signed-off-by: John Snow 
---
 python/qemu/aqmp/util.py | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/python/qemu/aqmp/util.py b/python/qemu/aqmp/util.py
index 52a15321889..eaa5fc7d5f9 100644
--- a/python/qemu/aqmp/util.py
+++ b/python/qemu/aqmp/util.py
@@ -147,6 +147,25 @@ async def wait_closed(writer: asyncio.StreamWriter) -> 
None:
 await asyncio.sleep(0)
 
 
+def asyncio_run(coro: Coroutine[Any, Any, T], *, debug: bool = False) -> T:
+"""
+Python 3.6-compatible `asyncio.run` wrapper.
+
+:param coro: A coroutine to execute now.
+:return: The return value from the coroutine.
+"""
+if sys.version_info >= (3, 7):
+return asyncio.run(coro, debug=debug)
+
+# Python 3.6
+loop = asyncio.get_event_loop()
+loop.set_debug(debug)
+ret = loop.run_until_complete(coro)
+loop.close()
+
+return ret
+
+
 # 
 # Section: Logging & Debugging
 # 
-- 
2.31.1

[PATCH v3 21/25] python/aqmp: add _raw() execution interface

[John Snow]

This is added in anticipation of wanting it for a synchronous wrapper
for the iotest interface. Normally, execute() and execute_msg() both
raise QMP errors in the form of Python exceptions.

Many iotests expect the entire reply as-is. To reduce churn there, add a
private execution interface that will ease transition churn. However, I
do not wish to encourage its use, so it will remain a private interface.

Signed-off-by: John Snow 
---
 python/qemu/aqmp/qmp_client.py | 51 ++
 1 file changed, 51 insertions(+)

diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
index 879348feaaa..82e9dab124c 100644
--- a/python/qemu/aqmp/qmp_client.py
+++ b/python/qemu/aqmp/qmp_client.py
@@ -484,6 +484,57 @@ async def _execute(self, msg: Message, assign_id: bool = 
True) -> Message:
 exec_id = await self._issue(msg)
 return await self._reply(exec_id)
 
+@upper_half
+@require(Runstate.RUNNING)
+async def _raw(
+self,
+msg: Union[Message, Mapping[str, object], bytes],
+assign_id: bool = True,
+) -> Message:
+"""
+Issue a raw `Message` to the QMP server and await a reply.
+
+:param msg:
+A Message to send to the server. It may be a `Message`, any
+Mapping (including Dict), or raw bytes.
+:param assign_id:
+Assign an arbitrary execution ID to this message. If
+`False`, the existing id must either be absent (and no other
+such pending execution may omit an ID) or a string. If it is
+a string, it must not start with '__aqmp#' and no other such
+pending execution may currently be using that ID.
+
+:return: Execution reply from the server.
+
+:raise ExecInterruptedError:
+When the reply could not be retrieved because the connection
+was lost, or some other problem.
+:raise TypeError:
+When assign_id is `False`, an ID is given, and it is not a string.
+:raise ValueError:
+When assign_id is `False`, but the ID is not usable;
+Either because it starts with '__aqmp#' or it is already in-use.
+"""
+# 1. convert generic Mapping or bytes to a QMP Message
+# 2. copy Message objects so that we assign an ID only to the copy.
+msg = Message(msg)
+
+exec_id = msg.get('id')
+if not assign_id and 'id' in msg:
+if not isinstance(exec_id, str):
+raise TypeError(f"ID ('{exec_id}') must be a string.")
+if exec_id.startswith('__aqmp#'):
+raise ValueError(
+f"ID ('{exec_id}') must not start with '__aqmp#'."
+)
+
+if not assign_id and exec_id in self._pending:
+raise ValueError(
+f"ID '{exec_id}' is in-use and cannot be used."
+)
+
+return await self._execute(msg, assign_id=assign_id)
+
 @upper_half
 @require(Runstate.RUNNING)
 async def execute_msg(self, msg: Message) -> object:
-- 
2.31.1

[PATCH v3 14/25] python/aqmp: add well-known QMP object models

[John Snow]

The QMP spec doesn't define very many objects that are iron-clad in
their format, but there are a few. This module makes it trivial to
validate them without relying on an external third-party library.

Signed-off-by: John Snow 
---
 python/qemu/aqmp/models.py | 133 +
 1 file changed, 133 insertions(+)
 create mode 100644 python/qemu/aqmp/models.py

diff --git a/python/qemu/aqmp/models.py b/python/qemu/aqmp/models.py
new file mode 100644
index 000..24c94123ac0
--- /dev/null
+++ b/python/qemu/aqmp/models.py
@@ -0,0 +1,133 @@
+"""
+QMP Data Models
+
+This module provides simplistic data classes that represent the few
+structures that the QMP spec mandates; they are used to verify incoming
+data to make sure it conforms to spec.
+"""
+# pylint: disable=too-few-public-methods
+
+from collections import abc
+from typing import (
+Any,
+Mapping,
+Optional,
+Sequence,
+)
+
+
+class Model:
+"""
+Abstract data model, representing some QMP object of some kind.
+
+:param raw: The raw object to be validated.
+:raise KeyError: If any required fields are absent.
+:raise TypeError: If any required fields have the wrong type.
+"""
+def __init__(self, raw: Mapping[str, Any]):
+self._raw = raw
+
+def _check_key(self, key: str) -> None:
+if key not in self._raw:
+raise KeyError(f"'{self._name}' object requires '{key}' member")
+
+def _check_value(self, key: str, type_: type, typestr: str) -> None:
+assert key in self._raw
+if not isinstance(self._raw[key], type_):
+raise TypeError(
+f"'{self._name}' member '{key}' must be a {typestr}"
+)
+
+def _check_member(self, key: str, type_: type, typestr: str) -> None:
+self._check_key(key)
+self._check_value(key, type_, typestr)
+
+@property
+def _name(self) -> str:
+return type(self).__name__
+
+def __repr__(self) -> str:
+return f"{self._name}({self._raw!r})"
+
+
+class Greeting(Model):
+"""
+Defined in qmp-spec.txt, section 2.2, "Server Greeting".
+
+:param raw: The raw Greeting object.
+:raise KeyError: If any required fields are absent.
+:raise TypeError: If any required fields have the wrong type.
+"""
+def __init__(self, raw: Mapping[str, Any]):
+super().__init__(raw)
+#: 'QMP' member
+self.QMP: QMPGreeting  # pylint: disable=invalid-name
+
+self._check_member('QMP', abc.Mapping, "JSON object")
+self.QMP = QMPGreeting(self._raw['QMP'])
+
+
+class QMPGreeting(Model):
+"""
+Defined in qmp-spec.txt, section 2.2, "Server Greeting".
+
+:param raw: The raw QMPGreeting object.
+:raise KeyError: If any required fields are absent.
+:raise TypeError: If any required fields have the wrong type.
+"""
+def __init__(self, raw: Mapping[str, Any]):
+super().__init__(raw)
+#: 'version' member
+self.version: Mapping[str, object]
+#: 'capabilities' member
+self.capabilities: Sequence[object]
+
+self._check_member('version', abc.Mapping, "JSON object")
+self.version = self._raw['version']
+
+self._check_member('capabilities', abc.Sequence, "JSON array")
+self.capabilities = self._raw['capabilities']
+
+
+class ErrorResponse(Model):
+"""
+Defined in qmp-spec.txt, section 2.4.2, "error".
+
+:param raw: The raw ErrorResponse object.
+:raise KeyError: If any required fields are absent.
+:raise TypeError: If any required fields have the wrong type.
+"""
+def __init__(self, raw: Mapping[str, Any]):
+super().__init__(raw)
+#: 'error' member
+self.error: ErrorInfo
+#: 'id' member
+self.id: Optional[object] = None  # pylint: disable=invalid-name
+
+self._check_member('error', abc.Mapping, "JSON object")
+self.error = ErrorInfo(self._raw['error'])
+
+if 'id' in raw:
+self.id = raw['id']
+
+
+class ErrorInfo(Model):
+"""
+Defined in qmp-spec.txt, section 2.4.2, "error".
+
+:param raw: The raw ErrorInfo object.
+:raise KeyError: If any required fields are absent.
+:raise TypeError: If any required fields have the wrong type.
+"""
+def __init__(self, raw: Mapping[str, Any]):
+super().__init__(raw)
+#: 'class' member, with an underscore to avoid conflicts in Python.
+self.class_: str
+#: 'desc' member
+self.desc: str
+
+self._check_member('class', str, "string")
+self.class_ = self._raw['class']
+
+self._check_member('desc', str, "string")
+self.desc = self._raw['desc']
-- 
2.31.1

[PATCH v3 20/25] python/aqmp: add execute() interfaces

[John Snow]

Add execute() and execute_msg().

_execute() is split into _issue() and _reply() halves so that
hypothetical subclasses of QMP that want to support different execution
paradigms can do so.

I anticipate a synchronous interface may have need of separating the
send/reply phases. However, I do not wish to expose that interface here
and want to actively discourage it, so they remain private interfaces.

Signed-off-by: John Snow 
---
 python/qemu/aqmp/__init__.py   |   4 +-
 python/qemu/aqmp/qmp_client.py | 202 +++--
 2 files changed, 198 insertions(+), 8 deletions(-)

diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index d975c752eaa..4b7df53e006 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -25,7 +25,7 @@
 from .events import EventListener
 from .message import Message
 from .protocol import ConnectError, Runstate, StateError
-from .qmp_client import QMPClient
+from .qmp_client import ExecInterruptedError, ExecuteError, QMPClient
 
 
 # The order of these fields impact the Sphinx documentation order.
@@ -40,4 +40,6 @@
 'AQMPError',
 'StateError',
 'ConnectError',
+'ExecuteError',
+'ExecInterruptedError',
 )
diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
index fa0cc7c5ae5..879348feaaa 100644
--- a/python/qemu/aqmp/qmp_client.py
+++ b/python/qemu/aqmp/qmp_client.py
@@ -7,8 +7,7 @@
 accept an incoming connection from that server.
 """
 
-# The import workarounds here are fixed in the next commit.
-import asyncio  # pylint: disable=unused-import # noqa
+import asyncio
 import logging
 from typing import (
 Dict,
@@ -22,8 +21,8 @@
 from .error import AQMPError, ProtocolError
 from .events import Events
 from .message import Message
-from .models import Greeting
-from .protocol import AsyncProtocol
+from .models import ErrorResponse, Greeting
+from .protocol import AsyncProtocol, Runstate, require
 from .util import (
 bottom_half,
 exception_summary,
@@ -65,11 +64,32 @@ class NegotiationError(_WrappedProtocolError):
 """
 
 
+class ExecuteError(AQMPError):
+"""
+Exception raised by `QMPClient.execute()` on RPC failure.
+
+:param error_response: The RPC error response object.
+:param sent: The sent RPC message that caused the failure.
+:param received: The raw RPC error reply received.
+"""
+def __init__(self, error_response: ErrorResponse,
+ sent: Message, received: Message):
+super().__init__(error_response.error.desc)
+#: The sent `Message` that caused the failure
+self.sent: Message = sent
+#: The received `Message` that indicated failure
+self.received: Message = received
+#: The parsed error response
+self.error: ErrorResponse = error_response
+#: The QMP error class
+self.error_class: str = error_response.error.class_
+
+
 class ExecInterruptedError(AQMPError):
 """
-Exception raised when an RPC is interrupted.
+Exception raised by `execute()` (et al) when an RPC is interrupted.
 
-This error is raised when an execute() statement could not be
+This error is raised when an `execute()` statement could not be
 completed.  This can occur because the connection itself was
 terminated before a reply was received.
 
@@ -112,6 +132,27 @@ class ServerParseError(_MsgProtocolError):
 """
 
 
+class BadReplyError(_MsgProtocolError):
+"""
+An execution reply was successfully routed, but not understood.
+
+If a QMP message is received with an 'id' field to allow it to be
+routed, but is otherwise malformed, this exception will be raised.
+
+A reply message is malformed if it is missing either the 'return' or
+'error' keys, or if the 'error' value has missing keys or members of
+the wrong type.
+
+:param error_message: Human-readable string describing the error.
+:param msg: The malformed reply that was received.
+:param sent: The message that was sent that prompted the error.
+"""
+def __init__(self, error_message: str, msg: Message, sent: Message):
+super().__init__(error_message, msg)
+#: The sent `Message` that caused the failure
+self.sent = sent
+
+
 class QMPClient(AsyncProtocol[Message], Events):
 """
 Implements a QMP client connection.
@@ -174,6 +215,9 @@ def __init__(self, name: Optional[str] = None) -> None:
 # Cached Greeting, if one was awaited.
 self._greeting: Optional[Greeting] = None
 
+# Command ID counter
+self._execute_id = 0
+
 # Incoming RPC reply messages.
 self._pending: Dict[
 Union[str, None],
@@ -363,12 +407,135 @@ def _do_send(self, msg: Message) -> None:
 assert self._writer is not None
 self._writer.write(bytes(msg))
 
+@upper_half
+def _get_exec_id(self) -> str:
+exec_id = f"__aqmp#{self._execute_id:05d}"
+self._execute_

[PATCH v3 16/25] python/pylint: disable too-many-function-args

[John Snow]

too-many-function-args seems prone to failure when considering
things like Method Resolution Order, which mypy gets correct. When
dealing with multiple inheritance, pylint doesn't seem to understand
which method will actually get called, while mypy does.

Remove the less powerful, redundant check.

Signed-off-by: John Snow 
---
 python/setup.cfg | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/python/setup.cfg b/python/setup.cfg
index f87e32177ab..19d5e154630 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -88,7 +88,7 @@ ignore_missing_imports = True
 # --enable=similarities". If you want to run only the classes checker, but have
 # no Warning level messages displayed, use "--disable=all --enable=classes
 # --disable=W".
-disable=
+disable=too-many-function-args,  # mypy handles this with less false positives.
 
 [pylint.basic]
 # Good variable names which should always be accepted, separated by a comma.
-- 
2.31.1

[PATCH v3 13/25] python/aqmp: add QMP Message format

[John Snow]

The Message class is here primarily to serve as a solid type to use for
mypy static typing for unambiguous annotation and documentation.

We can also stuff JSON serialization and deserialization into this class
itself so it can be re-used even outside this infrastructure.

Signed-off-by: John Snow 
---
 python/qemu/aqmp/__init__.py |   4 +-
 python/qemu/aqmp/message.py  | 209 +++
 2 files changed, 212 insertions(+), 1 deletion(-)
 create mode 100644 python/qemu/aqmp/message.py

diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index 88ead4c0238..96fff1e5f3e 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -22,12 +22,14 @@
 # the COPYING file in the top-level directory.
 
 from .error import AQMPError
+from .message import Message
 from .protocol import ConnectError, Runstate, StateError
 
 
 # The order of these fields impact the Sphinx documentation order.
 __all__ = (
-# Classes
+# Classes, most to least important
+'Message',
 'Runstate',
 
 # Exceptions, most generic to most explicit
diff --git a/python/qemu/aqmp/message.py b/python/qemu/aqmp/message.py
new file mode 100644
index 000..f76ccc90746
--- /dev/null
+++ b/python/qemu/aqmp/message.py
@@ -0,0 +1,209 @@
+"""
+QMP Message Format
+
+This module provides the `Message` class, which represents a single QMP
+message sent to or from the server.
+"""
+
+import json
+from json import JSONDecodeError
+from typing import (
+Dict,
+Iterator,
+Mapping,
+MutableMapping,
+Optional,
+Union,
+)
+
+from .error import ProtocolError
+
+
+class Message(MutableMapping[str, object]):
+"""
+Represents a single QMP protocol message.
+
+QMP uses JSON objects as its basic communicative unit; so this
+Python object is a :py:obj:`~collections.abc.MutableMapping`. It may
+be instantiated from either another mapping (like a `dict`), or from
+raw `bytes` that still need to be deserialized.
+
+Once instantiated, it may be treated like any other MutableMapping::
+
+>>> msg = Message(b'{"hello": "world"}')
+>>> assert msg['hello'] == 'world'
+>>> msg['id'] = 'foobar'
+>>> print(msg)
+{
+  "hello": "world",
+  "id": "foobar"
+}
+
+It can be converted to `bytes`::
+
+>>> msg = Message({"hello": "world"})
+>>> print(bytes(msg))
+b'{"hello":"world","id":"foobar"}'
+
+Or back into a garden-variety `dict`::
+
+   >>> dict(msg)
+   {'hello': 'world'}
+
+
+:param value: Initial value, if any.
+:param eager:
+When `True`, attempt to serialize or deserialize the initial value
+immediately, so that conversion exceptions are raised during
+the call to ``__init__()``.
+"""
+# pylint: disable=too-many-ancestors
+
+def __init__(self,
+ value: Union[bytes, Mapping[str, object]] = b'{}', *,
+ eager: bool = True):
+self._data: Optional[bytes] = None
+self._obj: Optional[Dict[str, object]] = None
+
+if isinstance(value, bytes):
+self._data = value
+if eager:
+self._obj = self._deserialize(self._data)
+else:
+self._obj = dict(value)
+if eager:
+self._data = self._serialize(self._obj)
+
+# Methods necessary to implement the MutableMapping interface, see:
+# 
https://docs.python.org/3/library/collections.abc.html#collections.abc.MutableMapping
+
+# We get pop, popitem, clear, update, setdefault, __contains__,
+# keys, items, values, get, __eq__ and __ne__ for free.
+
+def __getitem__(self, key: str) -> object:
+return self._object[key]
+
+def __setitem__(self, key: str, value: object) -> None:
+self._object[key] = value
+self._data = None
+
+def __delitem__(self, key: str) -> None:
+del self._object[key]
+self._data = None
+
+def __iter__(self) -> Iterator[str]:
+return iter(self._object)
+
+def __len__(self) -> int:
+return len(self._object)
+
+# Dunder methods not related to MutableMapping:
+
+def __repr__(self) -> str:
+if self._obj is not None:
+return f"Message({self._object!r})"
+return f"Message({bytes(self)!r})"
+
+def __str__(self) -> str:
+"""Pretty-printed representation of this QMP message."""
+return json.dumps(self._object, indent=2)
+
+def __bytes__(self) -> bytes:
+"""bytes representing this QMP message."""
+if self._data is None:
+self._data = self._serialize(self._obj or {})
+return self._data
+
+# Conversion Methods
+
+@property
+def _object(self) -> Dict[str, object]:
+"""
+A `dict` representing this QMP message.
+
+Generated on-demand, if required. This property is private
+because it returns an object t

[PATCH v3 17/25] python/aqmp: add QMP protocol support

[John Snow]

The star of our show!

Add most of the QMP protocol, sans support for actually executing
commands. No problem, that happens in the next several commits.

Signed-off-by: John Snow 
---
 python/qemu/aqmp/__init__.py   |   2 +
 python/qemu/aqmp/qmp_client.py | 264 +
 2 files changed, 266 insertions(+)
 create mode 100644 python/qemu/aqmp/qmp_client.py

diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index 829166a2e2e..d975c752eaa 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -25,11 +25,13 @@
 from .events import EventListener
 from .message import Message
 from .protocol import ConnectError, Runstate, StateError
+from .qmp_client import QMPClient
 
 
 # The order of these fields impact the Sphinx documentation order.
 __all__ = (
 # Classes, most to least important
+'QMPClient',
 'Message',
 'EventListener',
 'Runstate',
diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
new file mode 100644
index 000..000ff59c7a7
--- /dev/null
+++ b/python/qemu/aqmp/qmp_client.py
@@ -0,0 +1,264 @@
+"""
+QMP Protocol Implementation
+
+This module provides the `QMPClient` class, which can be used to connect
+and send commands to a QMP server such as QEMU. The QMP class can be
+used to either connect to a listening server, or used to listen and
+accept an incoming connection from that server.
+"""
+
+import logging
+from typing import (
+Dict,
+List,
+Mapping,
+Optional,
+)
+
+from .error import ProtocolError
+from .events import Events
+from .message import Message
+from .models import Greeting
+from .protocol import AsyncProtocol
+from .util import (
+bottom_half,
+exception_summary,
+pretty_traceback,
+upper_half,
+)
+
+
+class _WrappedProtocolError(ProtocolError):
+"""
+Abstract exception class for Protocol errors that wrap an Exception.
+
+:param error_message: Human-readable string describing the error.
+:param exc: The root-cause exception.
+"""
+def __init__(self, error_message: str, exc: Exception):
+super().__init__(error_message)
+self.exc = exc
+
+def __str__(self) -> str:
+return f"{self.error_message}: {self.exc!s}"
+
+
+class GreetingError(_WrappedProtocolError):
+"""
+An exception occurred during the Greeting phase.
+
+:param error_message: Human-readable string describing the error.
+:param exc: The root-cause exception.
+"""
+
+
+class NegotiationError(_WrappedProtocolError):
+"""
+An exception occurred during the Negotiation phase.
+
+:param error_message: Human-readable string describing the error.
+:param exc: The root-cause exception.
+"""
+
+
+class QMPClient(AsyncProtocol[Message], Events):
+"""
+Implements a QMP client connection.
+
+QMP can be used to establish a connection as either the transport
+client or server, though this class always acts as the QMP client.
+
+:param name: Optional nickname for the connection, used for logging.
+
+Basic script-style usage looks like this::
+
+  qmp = QMPClient('my_virtual_machine_name')
+  await qmp.connect(('127.0.0.1', 1234))
+  ...
+  res = await qmp.execute('block-query')
+  ...
+  await qmp.disconnect()
+
+Basic async client-style usage looks like this::
+
+  class Client:
+  def __init__(self, name: str):
+  self.qmp = QMPClient(name)
+
+  async def watch_events(self):
+  try:
+  async for event in self.qmp.events:
+  print(f"Event: {event['event']}")
+  except asyncio.CancelledError:
+  return
+
+  async def run(self, address='/tmp/qemu.socket'):
+  await self.qmp.connect(address)
+  asyncio.create_task(self.watch_events())
+  await self.qmp.runstate_changed.wait()
+  await self.disconnect()
+
+See `aqmp.events` for more detail on event handling patterns.
+"""
+#: Logger object used for debugging messages.
+logger = logging.getLogger(__name__)
+
+# Read buffer limit; large enough to accept query-qmp-schema
+_limit = (256 * 1024)
+
+def __init__(self, name: Optional[str] = None) -> None:
+super().__init__(name)
+Events.__init__(self)
+
+#: Whether or not to await a greeting after establishing a connection.
+self.await_greeting: bool = True
+
+#: Whether or not to perform capabilities negotiation upon connection.
+#: Implies `await_greeting`.
+self.negotiate: bool = True
+
+# Cached Greeting, if one was awaited.
+self._greeting: Optional[Greeting] = None
+
+@upper_half
+async def _establish_session(self) -> None:
+"""
+Initiate the QMP session.
+
+Wait for the QMP greeting and perform capabilities negotiation.
+
+:raise GreetingError: When the 

[PATCH v3 12/25] python/aqmp: add AsyncProtocol._readline() method

[John Snow]

This is added as a courtesy: many protocols are line-based, including
QMP. Putting it in AsyncProtocol lets us keep the QMP class
implementation just a pinch more abstract.

(And, if we decide to add a QTEST implementation later, it will need
this, too. (Yes, I have a QTEST implementation.))

Signed-off-by: John Snow 
---
 python/qemu/aqmp/protocol.py | 29 +
 1 file changed, 29 insertions(+)

diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 86aede5d7af..066d52dccf4 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -794,6 +794,35 @@ def _cb_inbound(self, msg: T) -> T:
 self.logger.debug("<-- %s", str(msg))
 return msg
 
+@upper_half
+@bottom_half
+async def _readline(self) -> bytes:
+"""
+Wait for a newline from the incoming reader.
+
+This method is provided as a convenience for upper-layer
+protocols, as many are line-based.
+
+This method *may* return a sequence of bytes without a trailing
+newline if EOF occurs, but *some* bytes were received. In this
+case, the next call will raise `EOFError`. It is assumed that
+the layer 5 protocol will decide if there is anything meaningful
+to be done with a partial message.
+
+:raise OSError: For stream-related errors.
+:raise EOFError:
+If the reader stream is at EOF and there are no bytes to return.
+:return: bytes, including the newline.
+"""
+assert self._reader is not None
+msg_bytes = await self._reader.readline()
+
+if not msg_bytes:
+if self._reader.at_eof():
+raise EOFError
+
+return msg_bytes
+
 @upper_half
 @bottom_half
 async def _do_recv(self) -> T:
-- 
2.31.1

[PATCH v3 15/25] python/aqmp: add QMP event support

[John Snow]

This class was designed as a "mix-in" primarily so that the feature
could be given its own treatment in its own python module.

It gets quite a bit too long otherwise.

Signed-off-by: John Snow 
---
 python/qemu/aqmp/__init__.py |   2 +
 python/qemu/aqmp/events.py   | 706 +++
 2 files changed, 708 insertions(+)
 create mode 100644 python/qemu/aqmp/events.py

diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index 96fff1e5f3e..829166a2e2e 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -22,6 +22,7 @@
 # the COPYING file in the top-level directory.
 
 from .error import AQMPError
+from .events import EventListener
 from .message import Message
 from .protocol import ConnectError, Runstate, StateError
 
@@ -30,6 +31,7 @@
 __all__ = (
 # Classes, most to least important
 'Message',
+'EventListener',
 'Runstate',
 
 # Exceptions, most generic to most explicit
diff --git a/python/qemu/aqmp/events.py b/python/qemu/aqmp/events.py
new file mode 100644
index 000..fb81d216102
--- /dev/null
+++ b/python/qemu/aqmp/events.py
@@ -0,0 +1,706 @@
+"""
+AQMP Events and EventListeners
+
+Asynchronous QMP uses `EventListener` objects to listen for events. An
+`EventListener` is a FIFO event queue that can be pre-filtered to listen
+for only specific events. Each `EventListener` instance receives its own
+copy of events that it hears, so events may be consumed without fear or
+worry for depriving other listeners of events they need to hear.
+
+
+EventListener Tutorial
+--
+
+In all of the following examples, we assume that we have a `QMPClient`
+instantiated named ``qmp`` that is already connected.
+
+
+`listener()` context blocks with one name
+~
+
+The most basic usage is by using the `listener()` context manager to
+construct them:
+
+.. code:: python
+
+   with qmp.listener('STOP') as listener:
+   await qmp.execute('stop')
+   await listener.get()
+
+The listener is active only for the duration of the ‘with’ block. This
+instance listens only for ‘STOP’ events.
+
+
+`listener()` context blocks with two or more names
+~~
+
+Multiple events can be selected for by providing any ``Iterable[str]``:
+
+.. code:: python
+
+   with qmp.listener(('STOP', 'RESUME')) as listener:
+   await qmp.execute('stop')
+   event = await listener.get()
+   assert event['event'] == 'STOP'
+
+   await qmp.execute('cont')
+   event = await listener.get()
+   assert event['event'] == 'RESUME'
+
+
+`listener()` context blocks with no names
+~
+
+By omitting names entirely, you can listen to ALL events.
+
+.. code:: python
+
+   with qmp.listener() as listener:
+   await qmp.execute('stop')
+   event = await listener.get()
+   assert event['event'] == 'STOP'
+
+This isn’t a very good use case for this feature: In a non-trivial
+running system, we may not know what event will arrive next. Grabbing
+the top of a FIFO queue returning multiple kinds of events may be prone
+to error.
+
+
+Using async iterators to retrieve events
+
+
+If you’d like to simply watch what events happen to arrive, you can use
+the listener as an async iterator:
+
+.. code:: python
+
+   with qmp.listener() as listener:
+   async for event in listener:
+   print(f"Event arrived: {event['event']}")
+
+This is analogous to the following code:
+
+.. code:: python
+
+   with qmp.listener() as listener:
+   while True:
+   event = listener.get()
+   print(f"Event arrived: {event['event']}")
+
+This event stream will never end, so these blocks will never terminate.
+
+
+Using asyncio.Task to concurrently retrieve events
+~~
+
+Since a listener’s event stream will never terminate, it is not likely
+useful to use that form in a script. For longer-running clients, we can
+create event handlers by using `asyncio.Task` to create concurrent
+coroutines:
+
+.. code:: python
+
+   async def print_events(listener):
+   try:
+   async for event in listener:
+   print(f"Event arrived: {event['event']}")
+   except asyncio.CancelledError:
+   return
+
+   with qmp.listener() as listener:
+   task = asyncio.Task(print_events(listener))
+   await qmp.execute('stop')
+   await qmp.execute('cont')
+   task.cancel()
+   await task
+
+However, there is no guarantee that these events will be received by the
+time we leave this context block. Once the context block is exited, the
+listener will cease to hear any new events, and becomes inert.
+
+Be mindful of the timing: the above example will *probably*– but does
+not *guarantee*– that both STOP/RESUMED events will be printed. The
+example below outlines how to use listeners out

[PATCH v3 11/25] python/aqmp: add _cb_inbound and _cb_outbound logging hooks

[John Snow]

Add hooks designed to log/filter incoming/outgoing messages. The primary
intent for these is to be able to support iotests which may want to log
messages with specific filters for reproducible output.

Another use is for plugging into Urwid frameworks; all messages in/out
can be automatically added to a rendering list for the purposes of a
qmp-shell like tool.

Signed-off-by: John Snow 
---
 python/qemu/aqmp/protocol.py | 50 +---
 1 file changed, 46 insertions(+), 4 deletions(-)

diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 28f025928c0..86aede5d7af 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -177,6 +177,11 @@ class AsyncProtocol(Generic[T]):
  can be written after the super() call.
  - `_on_message`:
  Actions to be performed when a message is received.
+ - `_cb_outbound`:
+ Logging/Filtering hook for all outbound messages.
+ - `_cb_inbound`:
+ Logging/Filtering hook for all inbound messages.
+ This hook runs *before* `_on_message()`.
 
 :param name:
 Name used for logging messages, if any. By default, messages
@@ -752,6 +757,43 @@ async def _bh_recv_message(self) -> None:
 # Section: Message I/O
 # 
 
+@upper_half
+@bottom_half
+def _cb_outbound(self, msg: T) -> T:
+"""
+Callback: outbound message hook.
+
+This is intended for subclasses to be able to add arbitrary
+hooks to filter or manipulate outgoing messages. The base
+implementation does nothing but log the message without any
+manipulation of the message.
+
+:param msg: raw outbound message
+:return: final outbound message
+"""
+self.logger.debug("--> %s", str(msg))
+return msg
+
+@upper_half
+@bottom_half
+def _cb_inbound(self, msg: T) -> T:
+"""
+Callback: inbound message hook.
+
+This is intended for subclasses to be able to add arbitrary
+hooks to filter or manipulate incoming messages. The base
+implementation does nothing but log the message without any
+manipulation of the message.
+
+This method does not "handle" incoming messages; it is a filter.
+The actual "endpoint" for incoming messages is `_on_message()`.
+
+:param msg: raw inbound message
+:return: processed inbound message
+"""
+self.logger.debug("<-- %s", str(msg))
+return msg
+
 @upper_half
 @bottom_half
 async def _do_recv(self) -> T:
@@ -780,8 +822,8 @@ async def _recv(self) -> T:
 
 :return: A single (filtered, processed) protocol message.
 """
-# A forthcoming commit makes this method less trivial.
-return await self._do_recv()
+message = await self._do_recv()
+return self._cb_inbound(message)
 
 @upper_half
 @bottom_half
@@ -811,7 +853,7 @@ async def _send(self, msg: T) -> None:
 
 :raise OSError: For problems with the underlying stream.
 """
-# A forthcoming commit makes this method less trivial.
+msg = self._cb_outbound(msg)
 self._do_send(msg)
 
 @bottom_half
@@ -826,6 +868,6 @@ async def _on_message(self, msg: T) -> None:
 directly cause the loop to halt, so logic may be best-kept
 to a minimum if at all possible.
 
-:param msg: The incoming message
+:param msg: The incoming message, already logged/filtered.
 """
 # Nothing to do in the abstract case.
-- 
2.31.1

[PATCH v3 25/25] python/aqmp: add AsyncProtocol unit tests

[John Snow]

This tests most of protocol.py -- From a hacked up Coverage.py run, it's
at about 86%. There's a few error cases that aren't very well tested
yet, they're hard to induce artificially so far. I'm working on it.

Signed-off-by: John Snow 
---
 python/tests/null_proto.py |  70 +
 python/tests/protocol.py   | 535 +
 2 files changed, 605 insertions(+)
 create mode 100644 python/tests/null_proto.py
 create mode 100644 python/tests/protocol.py

diff --git a/python/tests/null_proto.py b/python/tests/null_proto.py
new file mode 100644
index 000..c8cedea5942
--- /dev/null
+++ b/python/tests/null_proto.py
@@ -0,0 +1,70 @@
+import asyncio
+
+from qemu.aqmp.protocol import AsyncProtocol
+
+
+class NullProtocol(AsyncProtocol[None]):
+"""
+NullProtocol is a test mockup of an AsyncProtocol implementation.
+
+It adds a fake_session instance variable that enables a code path
+that bypasses the actual connection logic, but still allows the
+reader/writers to start.
+
+Because the message type is defined as None, an asyncio.Event named
+'trigger_input' is created that prohibits the reader from
+incessantly being able to yield None; this input can be poked to
+simulate an incoming message.
+
+For testing symmetry with do_recv, an interface is added to "send" a
+Null message.
+
+For testing purposes, a "simulate_disconnection" method is also
+added which allows us to trigger a bottom half disconnect without
+injecting any real errors into the reader/writer loops; in essence
+it performs exactly half of what disconnect() normally does.
+"""
+def __init__(self, name=None):
+self.fake_session = False
+self.trigger_input: asyncio.Event
+super().__init__(name)
+
+async def _establish_session(self):
+self.trigger_input = asyncio.Event()
+await super()._establish_session()
+
+async def _do_accept(self, address, ssl=None):
+if not self.fake_session:
+await super()._do_accept(address, ssl)
+
+async def _do_connect(self, address, ssl=None):
+if not self.fake_session:
+await super()._do_connect(address, ssl)
+
+async def _do_recv(self) -> None:
+await self.trigger_input.wait()
+self.trigger_input.clear()
+
+def _do_send(self, msg: None) -> None:
+pass
+
+async def send_msg(self) -> None:
+await self._outgoing.put(None)
+
+async def simulate_disconnect(self) -> None:
+"""
+Simulates a bottom-half disconnect.
+
+This method schedules a disconnection but does not wait for it
+to complete. This is used to put the loop into the DISCONNECTING
+state without fully quiescing it back to IDLE. This is normally
+something you cannot coax AsyncProtocol to do on purpose, but it
+will be similar to what happens with an unhandled Exception in
+the reader/writer.
+
+Under normal circumstances, the library design requires you to
+await on disconnect(), which awaits the disconnect task and
+returns bottom half errors as a pre-condition to allowing the
+loop to return back to IDLE.
+"""
+self._schedule_disconnect()
diff --git a/python/tests/protocol.py b/python/tests/protocol.py
new file mode 100644
index 000..f0682d29ce5
--- /dev/null
+++ b/python/tests/protocol.py
@@ -0,0 +1,535 @@
+import asyncio
+from contextlib import contextmanager
+import os
+import socket
+from tempfile import TemporaryDirectory
+
+import avocado
+
+from qemu.aqmp import ConnectError, Runstate
+from qemu.aqmp.protocol import AsyncProtocol, StateError
+from qemu.aqmp.util import asyncio_run, create_task
+
+
+class NullProtocol(AsyncProtocol[None]):
+"""
+NullProtocol is a test mockup of an AsyncProtocol implementation.
+
+It adds a fake_session instance variable that enables a code path
+that bypasses the actual connection logic, but still allows the
+reader/writers to start.
+
+Because the message type is defined as None, an asyncio.Event named
+'trigger_input' is created that prohibits the reader from
+incessantly being able to yield None; this event can be poked to
+simulate an incoming message.
+
+For testing symmetry with do_recv, an interface is added to "send" a
+Null message.
+
+For testing purposes, a "simulate_disconnection" method is also
+added which allows us to trigger a bottom half disconnect without
+injecting any real errors into the reader/writer loops; in essence
+it performs exactly half of what disconnect() normally does.
+"""
+def __init__(self, name=None):
+self.fake_session = False
+self.trigger_input: asyncio.Event
+super().__init__(name)
+
+async def _establish_session(self):
+self.trigger_input = asyncio.Event()
+await super()._establish_session()
+
+async def _do_accept(sel

[PATCH v3 07/25] python/aqmp: Add logging utility helpers

[John Snow]

Signed-off-by: John Snow 
---
 python/qemu/aqmp/util.py | 56 
 1 file changed, 56 insertions(+)

diff --git a/python/qemu/aqmp/util.py b/python/qemu/aqmp/util.py
index 5b8f968969d..52a15321889 100644
--- a/python/qemu/aqmp/util.py
+++ b/python/qemu/aqmp/util.py
@@ -4,10 +4,15 @@
 This module provides asyncio utilities and compatibility wrappers for
 Python 3.6 to provide some features that otherwise become available in
 Python 3.7+.
+
+Various logging and debugging utilities are also provided, such as
+`exception_summary()` and `pretty_traceback()`, used primarily for
+adding information into the logging stream.
 """
 
 import asyncio
 import sys
+import traceback
 from typing import (
 Any,
 Coroutine,
@@ -140,3 +145,54 @@ async def wait_closed(writer: asyncio.StreamWriter) -> 
None:
 
 while sock.fileno() != -1:
 await asyncio.sleep(0)
+
+
+# 
+# Section: Logging & Debugging
+# 
+
+
+def exception_summary(exc: BaseException) -> str:
+"""
+Return a summary string of an arbitrary exception.
+
+It will be of the form "ExceptionType: Error Message", if the error
+string is non-empty, and just "ExceptionType" otherwise.
+"""
+name = type(exc).__qualname__
+smod = type(exc).__module__
+if smod not in ("__main__", "builtins"):
+name = smod + '.' + name
+
+error = str(exc)
+if error:
+return f"{name}: {error}"
+return name
+
+
+def pretty_traceback(prefix: str = "  | ") -> str:
+"""
+Formats the current traceback, indented to provide visual distinction.
+
+This is useful for printing a traceback within a traceback for
+debugging purposes when encapsulating errors to deliver them up the
+stack; when those errors are printed, this helps provide a nice
+visual grouping to quickly identify the parts of the error that
+belong to the inner exception.
+
+:param prefix: The prefix to append to each line of the traceback.
+:return: A string, formatted something like the following::
+
+  | Traceback (most recent call last):
+  |   File "foobar.py", line 42, in arbitrary_example
+  | foo.baz()
+  | ArbitraryError: [Errno 42] Something bad happened!
+"""
+output = "".join(traceback.format_exception(*sys.exc_info()))
+
+exc_lines = []
+for line in output.split('\n'):
+exc_lines.append(prefix + line)
+
+# The last line is always empty, omit it
+return "\n".join(exc_lines[:-1])
-- 
2.31.1

[PATCH v3 04/25] python/aqmp: add asyncio compatibility wrappers

[John Snow]

Python 3.6 does not have all of the goodies that Python 3.7 does, and we
need to support both. Add some compatibility wrappers needed for this
purpose.

(Note: Python 3.6 is EOL December 2021.)

Signed-off-by: John Snow 
---
 python/qemu/aqmp/util.py | 89 
 1 file changed, 89 insertions(+)
 create mode 100644 python/qemu/aqmp/util.py

diff --git a/python/qemu/aqmp/util.py b/python/qemu/aqmp/util.py
new file mode 100644
index 000..28acd995dbf
--- /dev/null
+++ b/python/qemu/aqmp/util.py
@@ -0,0 +1,89 @@
+"""
+Miscellaneous Utilities
+
+This module provides asyncio utilities and compatibility wrappers for
+Python 3.6 to provide some features that otherwise become available in
+Python 3.7+.
+"""
+
+import asyncio
+import sys
+from typing import (
+Any,
+Coroutine,
+Optional,
+TypeVar,
+)
+
+
+T = TypeVar('T')
+
+
+# ---
+# Section: Compatibility Wrappers
+# ---
+
+
+def create_task(coro: Coroutine[Any, Any, T],
+loop: Optional[asyncio.AbstractEventLoop] = None
+) -> 'asyncio.Future[T]':
+"""
+Python 3.6-compatible `asyncio.create_task` wrapper.
+
+:param coro: The coroutine to execute in a task.
+:param loop: Optionally, the loop to create the task in.
+
+:return: An `asyncio.Future` object.
+"""
+if sys.version_info >= (3, 7):
+if loop is not None:
+return loop.create_task(coro)
+return asyncio.create_task(coro)  # pylint: disable=no-member
+
+# Python 3.6:
+return asyncio.ensure_future(coro, loop=loop)
+
+
+def is_closing(writer: asyncio.StreamWriter) -> bool:
+"""
+Python 3.6-compatible `asyncio.StreamWriter.is_closing` wrapper.
+
+:param writer: The `asyncio.StreamWriter` object.
+:return: `True` if the writer is closing, or closed.
+"""
+if sys.version_info >= (3, 7):
+return writer.is_closing()
+
+# Python 3.6:
+transport = writer.transport
+assert isinstance(transport, asyncio.WriteTransport)
+return transport.is_closing()
+
+
+async def wait_closed(writer: asyncio.StreamWriter) -> None:
+"""
+Python 3.6-compatible `asyncio.StreamWriter.wait_closed` wrapper.
+
+:param writer: The `asyncio.StreamWriter` to wait on.
+"""
+if sys.version_info >= (3, 7):
+await writer.wait_closed()
+return
+
+# Python 3.6
+transport = writer.transport
+assert isinstance(transport, asyncio.WriteTransport)
+
+while not transport.is_closing():
+await asyncio.sleep(0)
+
+# This is an ugly workaround, but it's the best I can come up with.
+sock = transport.get_extra_info('socket')
+
+if sock is None:
+# Our transport doesn't have a socket? ...
+# Nothing we can reasonably do.
+return
+
+while sock.fileno() != -1:
+await asyncio.sleep(0)
-- 
2.31.1

Re: [PATCH] linux-user/elfload: byteswap i386 registers when dumping core

[Peter Maydell]

On Tue, 3 Aug 2021 at 18:21, Ilya Leoshkevich  wrote:
>
> Core dumps from emulating x86_64 on big-endian hosts contain incorrect
> register values.
>
> Signed-off-by: Ilya Leoshkevich 


Looks like these two were the only two guest arch versions of this
function that were missing the tswapreg calls...

Reviewed-by: Peter Maydell 

thanks
-- PMM

[PATCH v3 09/25] python/aqmp: add AsyncProtocol.accept() method

[John Snow]

It's a little messier than connect, because it wasn't designed to accept
*precisely one* connection. Such is life.

Signed-off-by: John Snow 
---
 python/qemu/aqmp/protocol.py | 89 ++--
 1 file changed, 85 insertions(+), 4 deletions(-)

diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 77b330627b3..7eca65aa265 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -243,6 +243,24 @@ async def runstate_changed(self) -> Runstate:
 await self._runstate_event.wait()
 return self.runstate
 
+@upper_half
+@require(Runstate.IDLE)
+async def accept(self, address: Union[str, Tuple[str, int]],
+ ssl: Optional[SSLContext] = None) -> None:
+"""
+Accept a connection and begin processing message queues.
+
+If this call fails, `runstate` is guaranteed to be set back to `IDLE`.
+
+:param address:
+Address to listen to; UNIX socket path or TCP address/port.
+:param ssl: SSL context to use, if any.
+
+:raise StateError: When the `Runstate` is not `IDLE`.
+:raise ConnectError: If a connection could not be accepted.
+"""
+await self._new_session(address, ssl, accept=True)
+
 @upper_half
 @require(Runstate.IDLE)
 async def connect(self, address: Union[str, Tuple[str, int]],
@@ -308,7 +326,8 @@ def _set_state(self, state: Runstate) -> None:
 @upper_half
 async def _new_session(self,
address: Union[str, Tuple[str, int]],
-   ssl: Optional[SSLContext] = None) -> None:
+   ssl: Optional[SSLContext] = None,
+   accept: bool = False) -> None:
 """
 Establish a new connection and initialize the session.
 
@@ -317,9 +336,10 @@ async def _new_session(self,
 to be set back to `IDLE`.
 
 :param address:
-Address to connect to;
+Address to connect to/listen on;
 UNIX socket path or TCP address/port.
 :param ssl: SSL context to use, if any.
+:param accept: Accept a connection instead of connecting when `True`.
 
 :raise ConnectError:
 When a connection or session cannot be established.
@@ -333,7 +353,7 @@ async def _new_session(self,
 
 try:
 phase = "connection"
-await self._establish_connection(address, ssl)
+await self._establish_connection(address, ssl, accept)
 
 phase = "session"
 await self._establish_session()
@@ -367,6 +387,7 @@ async def _establish_connection(
 self,
 address: Union[str, Tuple[str, int]],
 ssl: Optional[SSLContext] = None,
+accept: bool = False
 ) -> None:
 """
 Establish a new connection.
@@ -375,6 +396,7 @@ async def _establish_connection(
 Address to connect to/listen on;
 UNIX socket path or TCP address/port.
 :param ssl: SSL context to use, if any.
+:param accept: Accept a connection instead of connecting when `True`.
 """
 assert self.runstate == Runstate.IDLE
 self._set_state(Runstate.CONNECTING)
@@ -384,7 +406,66 @@ async def _establish_connection(
 # otherwise yield.
 await asyncio.sleep(0)
 
-await self._do_connect(address, ssl)
+if accept:
+await self._do_accept(address, ssl)
+else:
+await self._do_connect(address, ssl)
+
+@upper_half
+async def _do_accept(self, address: Union[str, Tuple[str, int]],
+ ssl: Optional[SSLContext] = None) -> None:
+"""
+Acting as the transport server, accept a single connection.
+
+:param address:
+Address to listen on; UNIX socket path or TCP address/port.
+:param ssl: SSL context to use, if any.
+
+:raise OSError: For stream-related errors.
+"""
+self.logger.debug("Awaiting connection on %s ...", address)
+connected = asyncio.Event()
+server: Optional[asyncio.AbstractServer] = None
+
+async def _client_connected_cb(reader: asyncio.StreamReader,
+   writer: asyncio.StreamWriter) -> None:
+"""Used to accept a single incoming connection, see below."""
+nonlocal server
+nonlocal connected
+
+# A connection has been accepted; stop listening for new ones.
+assert server is not None
+server.close()
+await server.wait_closed()
+server = None
+
+# Register this client as being connected
+self._reader, self._writer = (reader, writer)
+
+# Signal back: We've accepted a client!
+connected.set()
+
+if isinstance(address, tuple):
+coro = asyncio.start_server(
+   

[PATCH v3 10/25] python/aqmp: add configurable read buffer limit

[John Snow]

QMP can transmit some pretty big messages, and the default limit of 64KB
isn't sufficient. Make sure that we can configure it.

Reported-by: G S Niteesh Babu 
Signed-off-by: John Snow 
---
 python/qemu/aqmp/protocol.py | 18 --
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 7eca65aa265..28f025928c0 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -189,6 +189,9 @@ class AsyncProtocol(Generic[T]):
 #: Logger object for debugging messages from this connection.
 logger = logging.getLogger(__name__)
 
+# Maximum allowable size of read buffer
+_limit = (64 * 1024)
+
 # -
 # Section: Public interface
 # -
@@ -452,6 +455,7 @@ async def _client_connected_cb(reader: asyncio.StreamReader,
 port=address[1],
 ssl=ssl,
 backlog=1,
+limit=self._limit,
 )
 else:
 coro = asyncio.start_unix_server(
@@ -459,6 +463,7 @@ async def _client_connected_cb(reader: asyncio.StreamReader,
 path=address,
 ssl=ssl,
 backlog=1,
+limit=self._limit,
 )
 
 server = await coro # Starts listening
@@ -482,9 +487,18 @@ async def _do_connect(self, address: Union[str, Tuple[str, 
int]],
 self.logger.debug("Connecting to %s ...", address)
 
 if isinstance(address, tuple):
-connect = asyncio.open_connection(address[0], address[1], ssl=ssl)
+connect = asyncio.open_connection(
+address[0],
+address[1],
+ssl=ssl,
+limit=self._limit,
+)
 else:
-connect = asyncio.open_unix_connection(path=address, ssl=ssl)
+connect = asyncio.open_unix_connection(
+path=address,
+ssl=ssl,
+limit=self._limit,
+)
 self._reader, self._writer = await connect
 
 self.logger.debug("Connected.")
-- 
2.31.1

[PATCH v3 06/25] python/aqmp: add runstate state machine to AsyncProtocol

[John Snow]

This serves a few purposes:

1. Protect interfaces when it's not safe to call them (via @require)

2. Add an interface by which an async client can determine if the state
has changed, for the purposes of connection management.

Signed-off-by: John Snow 
---
 python/qemu/aqmp/__init__.py |   6 +-
 python/qemu/aqmp/protocol.py | 159 ++-
 2 files changed, 160 insertions(+), 5 deletions(-)

diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index 5c0de72672d..88ead4c0238 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -22,12 +22,16 @@
 # the COPYING file in the top-level directory.
 
 from .error import AQMPError
-from .protocol import ConnectError
+from .protocol import ConnectError, Runstate, StateError
 
 
 # The order of these fields impact the Sphinx documentation order.
 __all__ = (
+# Classes
+'Runstate',
+
 # Exceptions, most generic to most explicit
 'AQMPError',
+'StateError',
 'ConnectError',
 )
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 2a93da791e2..3a4703d49dc 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -12,11 +12,10 @@
 
 import asyncio
 from asyncio import StreamReader, StreamWriter
+from enum import Enum
+from functools import wraps
 from ssl import SSLContext
-# import exceptions will be removed in a forthcoming commit.
-# The problem stems from pylint/flake8 believing that 'Any'
-# is unused because of its only use in a string-quoted type.
-from typing import (  # pylint: disable=unused-import # noqa
+from typing import (
 Any,
 Awaitable,
 Callable,
@@ -26,6 +25,7 @@
 Tuple,
 TypeVar,
 Union,
+cast,
 )
 
 from .error import AQMPError
@@ -44,6 +44,20 @@
 _FutureT = TypeVar('_FutureT', bound=Optional['asyncio.Future[Any]'])
 
 
+class Runstate(Enum):
+"""Protocol session runstate."""
+
+#: Fully quiesced and disconnected.
+IDLE = 0
+#: In the process of connecting or establishing a session.
+CONNECTING = 1
+#: Fully connected and active session.
+RUNNING = 2
+#: In the process of disconnecting.
+#: Runstate may be returned to `IDLE` by calling `disconnect()`.
+DISCONNECTING = 3
+
+
 class ConnectError(AQMPError):
 """
 Raised when the initial connection process has failed.
@@ -65,6 +79,76 @@ def __str__(self) -> str:
 return f"{self.error_message}: {self.exc!s}"
 
 
+class StateError(AQMPError):
+"""
+An API command (connect, execute, etc) was issued at an inappropriate time.
+
+This error is raised when a command like
+:py:meth:`~AsyncProtocol.connect()` is issued at an inappropriate
+time.
+
+:param error_message: Human-readable string describing the state violation.
+:param state: The actual `Runstate` seen at the time of the violation.
+:param required: The `Runstate` required to process this command.
+"""
+def __init__(self, error_message: str,
+ state: Runstate, required: Runstate):
+super().__init__(error_message)
+self.error_message = error_message
+self.state = state
+self.required = required
+
+
+F = TypeVar('F', bound=Callable[..., Any])  # pylint: disable=invalid-name
+
+
+# Don't Panic.
+def require(required_state: Runstate) -> Callable[[F], F]:
+"""
+Decorator: protect a method so it can only be run in a certain `Runstate`.
+
+:param required_state: The `Runstate` required to invoke this method.
+:raise StateError: When the required `Runstate` is not met.
+"""
+def _decorator(func: F) -> F:
+# _decorator is the decorator that is built by calling the
+# require() decorator factory; e.g.:
+#
+# @require(Runstate.IDLE) def # foo(): ...
+# will replace 'foo' with the result of '_decorator(foo)'.
+
+@wraps(func)
+def _wrapper(proto: 'AsyncProtocol[Any]',
+ *args: Any, **kwargs: Any) -> Any:
+# _wrapper is the function that gets executed prior to the
+# decorated method.
+
+name = type(proto).__name__
+
+if proto.runstate != required_state:
+if proto.runstate == Runstate.CONNECTING:
+emsg = f"{name} is currently connecting."
+elif proto.runstate == Runstate.DISCONNECTING:
+emsg = (f"{name} is disconnecting."
+" Call disconnect() to return to IDLE state.")
+elif proto.runstate == Runstate.RUNNING:
+emsg = f"{name} is already connected and running."
+elif proto.runstate == Runstate.IDLE:
+emsg = f"{name} is disconnected and idle."
+else:
+assert False
+raise StateError(emsg, proto.runstate, required_state)
+# No StateError, so call the wrapped method.
+   

[PATCH v3 19/25] python/aqmp: Add message routing to QMP protocol

[John Snow]

Add the ability to handle and route messages in qmp_protocol.py. The
interface for actually sending anything still isn't added until next
commit.

Signed-off-by: John Snow 
---
 python/qemu/aqmp/qmp_client.py | 122 -
 1 file changed, 120 insertions(+), 2 deletions(-)

diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
index 000ff59c7a7..fa0cc7c5ae5 100644
--- a/python/qemu/aqmp/qmp_client.py
+++ b/python/qemu/aqmp/qmp_client.py
@@ -7,15 +7,19 @@
 accept an incoming connection from that server.
 """
 
+# The import workarounds here are fixed in the next commit.
+import asyncio  # pylint: disable=unused-import # noqa
 import logging
 from typing import (
 Dict,
 List,
 Mapping,
 Optional,
+Union,
+cast,
 )
 
-from .error import ProtocolError
+from .error import AQMPError, ProtocolError
 from .events import Events
 from .message import Message
 from .models import Greeting
@@ -61,6 +65,53 @@ class NegotiationError(_WrappedProtocolError):
 """
 
 
+class ExecInterruptedError(AQMPError):
+"""
+Exception raised when an RPC is interrupted.
+
+This error is raised when an execute() statement could not be
+completed.  This can occur because the connection itself was
+terminated before a reply was received.
+
+The true cause of the interruption will be available via `disconnect()`.
+"""
+
+
+class _MsgProtocolError(ProtocolError):
+"""
+Abstract error class for protocol errors that have a `Message` object.
+
+This Exception class is used for protocol errors where the `Message`
+was mechanically understood, but was found to be inappropriate or
+malformed.
+
+:param error_message: Human-readable string describing the error.
+:param msg: The QMP `Message` that caused the error.
+"""
+def __init__(self, error_message: str, msg: Message):
+super().__init__(error_message)
+#: The received `Message` that caused the error.
+self.msg: Message = msg
+
+def __str__(self) -> str:
+return "\n".join([
+super().__str__(),
+f"  Message was: {str(self.msg)}\n",
+])
+
+
+class ServerParseError(_MsgProtocolError):
+"""
+The Server sent a `Message` indicating parsing failure.
+
+i.e. A reply has arrived from the server, but it is missing the "ID"
+field, indicating a parsing error.
+
+:param error_message: Human-readable string describing the error.
+:param msg: The QMP `Message` that caused the error.
+"""
+
+
 class QMPClient(AsyncProtocol[Message], Events):
 """
 Implements a QMP client connection.
@@ -106,6 +157,9 @@ async def run(self, address='/tmp/qemu.socket'):
 # Read buffer limit; large enough to accept query-qmp-schema
 _limit = (256 * 1024)
 
+# Type alias for pending execute() result items
+_PendingT = Union[Message, ExecInterruptedError]
+
 def __init__(self, name: Optional[str] = None) -> None:
 super().__init__(name)
 Events.__init__(self)
@@ -120,6 +174,12 @@ def __init__(self, name: Optional[str] = None) -> None:
 # Cached Greeting, if one was awaited.
 self._greeting: Optional[Greeting] = None
 
+# Incoming RPC reply messages.
+self._pending: Dict[
+Union[str, None],
+'asyncio.Queue[QMPClient._PendingT]'
+] = {}
+
 @upper_half
 async def _establish_session(self) -> None:
 """
@@ -132,6 +192,9 @@ async def _establish_session(self) -> None:
 :raise EOFError: When the server unexpectedly hangs up.
 :raise OSError: For underlying stream errors.
 """
+self._greeting = None
+self._pending = {}
+
 if self.await_greeting or self.negotiate:
 self._greeting = await self._get_greeting()
 
@@ -203,10 +266,33 @@ async def _negotiate(self) -> None:
 self.logger.debug("%s:\n%s\n", emsg, pretty_traceback())
 raise
 
+@bottom_half
+async def _bh_disconnect(self) -> None:
+try:
+await super()._bh_disconnect()
+finally:
+if self._pending:
+self.logger.debug("Cancelling pending executions")
+keys = self._pending.keys()
+for key in keys:
+self.logger.debug("Cancelling execution '%s'", key)
+self._pending[key].put_nowait(
+ExecInterruptedError("Disconnected")
+)
+
+self.logger.debug("QMP Disconnected.")
+
+@upper_half
+def _cleanup(self) -> None:
+super()._cleanup()
+assert not self._pending
+
 @bottom_half
 async def _on_message(self, msg: Message) -> None:
 """
 Add an incoming message to the appropriate queue/handler.
+
+:raise ServerParseError: When Message indicates server parse failure.
 """
 # Incoming messages are not fully parsed/validated here;
   

[PATCH v3 00/25] python: introduce Asynchronous QMP package

[John Snow]

GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-async-qmp-aqmp
CI: https://gitlab.com/jsnow/qemu/-/pipelines/347375602
Docs: https://people.redhat.com/~jsnow/sphinx/html/qemu.aqmp.html

Hi!

This patch series adds an Asynchronous QMP package to the Python
library. It offers a few improvements over the previous library:

- out-of-band support
- true asynchronous event support
- avoids undocumented interfaces abusing non-blocking sockets
- unit tests!
- documentation!

This library serves as the basis for a new qmp-shell program that will
offer improved reconnection support, true asynchronous display of
events, VM and job status update notifiers, and so on.

My intent is to eventually publish this library directly to PyPI as a
standalone package. I would like to phase out our usage of the old QMP
library over time; eventually replacing it entirely with this
one. (Since v2 of this series, I have authored a compatibility shim not
included in this series that can be used to run all of iotests on this
new library successfully with very minimal churn.)

This series looks big by line count, but it's *mostly*
docstrings. Seriously!

This package has *no* external dependencies whatsoever.

Notes & Design
==

Here are some notes on the design of how the library works, to serve as
a primer for review; however I also **highly recommend** browsing the
generated Sphinx documentation for this series.

Here's that link again:
https://people.redhat.com/~jsnow/sphinx/html/qemu.aqmp.html

The core machinery is split between the AsyncProtocol and QMPClient
classes. AsyncProtocol provides the generic machinery, while QMPClient
provides the QMP-specific details.

The design uses two independent coroutines that act as the "bottom
half", a writer task and a reader task. These tasks run for the duration
of the connection and independently send and receive messages,
respectively.

A third task, disconnect, is scheduled asynchronously whenever an
unrecoverable error occurs and facilitates coalescing of the other two
tasks.

This diagram for how execute() operates may be helpful for understanding
how AsyncProtocol is laid out. The arrows indicate the direction of a
QMP message; the long horizontal dash indicates the separation between
the upper and lower halves of the event loop. The queue mechanisms
between both dashes serve as the intermediaries between the upper and
lower halves.

   +-+
   | caller  |
   +-+
   ^ |
   | v
   +-+
 +---> |execute()| ---+
 | +-+|
 ||
[---]
 ||
 |v
++--++++--+---+
| ExecQueue || EventListeners ||Outbound Queue|
++--+++---++--+---+
 ^^   |
 ||   |
[---]
 ||   |
 ||   v
  +--++---+   +---+---+
  | Reader Task/Coroutine |   | Writer Task/Coroutine |
  +---+---+   +---+---+
  ^   |
  |   v
+-+--+  +-+--+
|StreamReader|  |StreamWriter|
++  ++

The caller will invoke execute(), which in turn will deposit a message
in the outbound send queue. This will wake up the writer task, which
well send the message over the wire.

The execute() method will then yield to wait for a reply delivered to an
execution queue created solely for that execute statement.

When a message arrives, the Reader task will unblock and route the
message either to the EventListener subsystem, or place it in the
appropriate pending execution queue.

Once a message is placed in the pending execution queue, execute() will
unblock and the execution will conclude, returning the result of the RPC
call to the caller.

Patch Layout


Patches 1-4   add tiny pre-requisites, utilities, etc.
Patches 5-12  add a generic async message-based protocol class,
  AsyncProtocol. They are split fairly small and should
  be reasonably self-contained.
Patches 13-15 check in more QMP-centric components.
Patches 16-21 add qmp_client.py, with a new 'QMPClient()' class.
  They're split into reasonably tiny pieces here.
Patches 22-23 add a few finishing touches, they are small patches.
Patches 24-25 adds unit tests. They're a little messy still, b

[PATCH v3 05/25] python/aqmp: add generic async message-based protocol support

[John Snow]

This is the bare minimum that you need to establish a full-duplex async
message-based protocol with Python's asyncio.

The features to be added in forthcoming commits are:

- Runstate tracking
- Logging
- Support for incoming connections via accept()
- _cb_outbound, _cb_inbound message hooks
- _readline() method

Signed-off-by: John Snow 
---
 python/qemu/aqmp/__init__.py |   4 +-
 python/qemu/aqmp/protocol.py | 521 +++
 python/qemu/aqmp/util.py |  53 
 3 files changed, 577 insertions(+), 1 deletion(-)
 create mode 100644 python/qemu/aqmp/protocol.py

diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index c97be950bf4..5c0de72672d 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -22,10 +22,12 @@
 # the COPYING file in the top-level directory.
 
 from .error import AQMPError
+from .protocol import ConnectError
 
 
 # The order of these fields impact the Sphinx documentation order.
 __all__ = (
-# Exceptions
+# Exceptions, most generic to most explicit
 'AQMPError',
+'ConnectError',
 )
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
new file mode 100644
index 000..2a93da791e2
--- /dev/null
+++ b/python/qemu/aqmp/protocol.py
@@ -0,0 +1,521 @@
+"""
+Generic Asynchronous Message-based Protocol Support
+
+This module provides a generic framework for sending and receiving
+messages over an asyncio stream. `AsyncProtocol` is an abstract class
+that implements the core mechanisms of a simple send/receive protocol,
+and is designed to be extended.
+
+In this package, it is used as the implementation for the `QMPClient`
+class.
+"""
+
+import asyncio
+from asyncio import StreamReader, StreamWriter
+from ssl import SSLContext
+# import exceptions will be removed in a forthcoming commit.
+# The problem stems from pylint/flake8 believing that 'Any'
+# is unused because of its only use in a string-quoted type.
+from typing import (  # pylint: disable=unused-import # noqa
+Any,
+Awaitable,
+Callable,
+Generic,
+List,
+Optional,
+Tuple,
+TypeVar,
+Union,
+)
+
+from .error import AQMPError
+from .util import (
+bottom_half,
+create_task,
+flush,
+is_closing,
+upper_half,
+wait_closed,
+)
+
+
+T = TypeVar('T')
+_TaskFN = Callable[[], Awaitable[None]]  # aka ``async def func() -> None``
+_FutureT = TypeVar('_FutureT', bound=Optional['asyncio.Future[Any]'])
+
+
+class ConnectError(AQMPError):
+"""
+Raised when the initial connection process has failed.
+
+This Exception always wraps a "root cause" exception that can be
+interrogated for additional information.
+
+:param error_message: Human-readable string describing the error.
+:param exc: The root-cause exception.
+"""
+def __init__(self, error_message: str, exc: Exception):
+super().__init__(error_message)
+#: Human-readable error string
+self.error_message: str = error_message
+#: Wrapped root cause exception
+self.exc: Exception = exc
+
+def __str__(self) -> str:
+return f"{self.error_message}: {self.exc!s}"
+
+
+class AsyncProtocol(Generic[T]):
+"""
+AsyncProtocol implements a generic async message-based protocol.
+
+This protocol assumes the basic unit of information transfer between
+client and server is a "message", the details of which are left up
+to the implementation. It assumes the sending and receiving of these
+messages is full-duplex and not necessarily correlated; i.e. it
+supports asynchronous inbound messages.
+
+It is designed to be extended by a specific protocol which provides
+the implementations for how to read and send messages. These must be
+defined in `_do_recv()` and `_do_send()`, respectively.
+
+Other callbacks have a default implementation, but are intended to be
+either extended or overridden:
+
+ - `_establish_session`:
+ The base implementation starts the reader/writer tasks.
+ A protocol implementation can override this call, inserting
+ actions to be taken prior to starting the reader/writer tasks
+ before the super() call; actions needing to occur afterwards
+ can be written after the super() call.
+ - `_on_message`:
+ Actions to be performed when a message is received.
+"""
+# pylint: disable=too-many-instance-attributes
+
+# -
+# Section: Public interface
+# -
+
+def __init__(self) -> None:
+# stream I/O
+self._reader: Optional[StreamReader] = None
+self._writer: Optional[StreamWriter] = None
+
+# Outbound Message queue
+self._outgoing: asyncio.Queue[T]
+
+# Special, long-running tasks:
+self._reader_task: Optional[asyncio.Future[None]] = None
+self._writer_task: Optional[asyncio.Future[None]] = None
+
+# Aggreg

[PATCH v3 08/25] python/aqmp: add logging to AsyncProtocol

[John Snow]

Give the connection and the reader/writer tasks nicknames, and add
logging statements throughout.

Signed-off-by: John Snow 
---
 python/qemu/aqmp/protocol.py | 82 
 1 file changed, 73 insertions(+), 9 deletions(-)

diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 3a4703d49dc..77b330627b3 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -14,6 +14,7 @@
 from asyncio import StreamReader, StreamWriter
 from enum import Enum
 from functools import wraps
+import logging
 from ssl import SSLContext
 from typing import (
 Any,
@@ -32,8 +33,10 @@
 from .util import (
 bottom_half,
 create_task,
+exception_summary,
 flush,
 is_closing,
+pretty_traceback,
 upper_half,
 wait_closed,
 )
@@ -174,14 +177,28 @@ class AsyncProtocol(Generic[T]):
  can be written after the super() call.
  - `_on_message`:
  Actions to be performed when a message is received.
+
+:param name:
+Name used for logging messages, if any. By default, messages
+will log to 'qemu.aqmp.protocol', but each individual connection
+can be given its own logger by giving it a name; messages will
+then log to 'qemu.aqmp.protocol.${name}'.
 """
 # pylint: disable=too-many-instance-attributes
 
+#: Logger object for debugging messages from this connection.
+logger = logging.getLogger(__name__)
+
 # -
 # Section: Public interface
 # -
 
-def __init__(self) -> None:
+def __init__(self, name: Optional[str] = None) -> None:
+#: The nickname for this connection, if any.
+self.name: Optional[str] = name
+if self.name is not None:
+self.logger = self.logger.getChild(self.name)
+
 # stream I/O
 self._reader: Optional[StreamReader] = None
 self._writer: Optional[StreamWriter] = None
@@ -205,6 +222,14 @@ def __init__(self) -> None:
 self._runstate = Runstate.IDLE
 self._runstate_changed: Optional[asyncio.Event] = None
 
+def __repr__(self) -> str:
+cls_name = type(self).__name__
+tokens = []
+if self.name is not None:
+tokens.append(f"name={self.name!r}")
+tokens.append(f"runstate={self.runstate.name}")
+return f"<{cls_name} {' '.join(tokens)}>"
+
 @property  # @upper_half
 def runstate(self) -> Runstate:
 """The current `Runstate` of the connection."""
@@ -246,6 +271,7 @@ async def disconnect(self) -> None:
 
 :raise Exception: When the reader or writer terminate unexpectedly.
 """
+self.logger.debug("disconnect() called.")
 self._schedule_disconnect()
 await self._wait_disconnect()
 
@@ -273,6 +299,8 @@ def _set_state(self, state: Runstate) -> None:
 if state == self._runstate:
 return
 
+self.logger.debug("Transitioning from '%s' to '%s'.",
+  str(self._runstate), str(state))
 self._runstate = state
 self._runstate_event.set()
 self._runstate_event.clear()
@@ -312,8 +340,15 @@ async def _new_session(self,
 
 except BaseException as err:
 emsg = f"Failed to establish {phase}"
-# Reset from CONNECTING back to IDLE.
-await self.disconnect()
+self.logger.error("%s: %s", emsg, exception_summary(err))
+self.logger.debug("%s:\n%s\n", emsg, pretty_traceback())
+try:
+# Reset from CONNECTING back to IDLE.
+await self.disconnect()
+except:
+emsg = "Unexpected bottom half exception"
+self.logger.critical("%s:\n%s\n", emsg, pretty_traceback())
+raise
 
 # NB: CancelledError is not a BaseException before Python 3.8
 if isinstance(err, asyncio.CancelledError):
@@ -363,12 +398,16 @@ async def _do_connect(self, address: Union[str, 
Tuple[str, int]],
 
 :raise OSError: For stream-related errors.
 """
+self.logger.debug("Connecting to %s ...", address)
+
 if isinstance(address, tuple):
 connect = asyncio.open_connection(address[0], address[1], ssl=ssl)
 else:
 connect = asyncio.open_unix_connection(path=address, ssl=ssl)
 self._reader, self._writer = await connect
 
+self.logger.debug("Connected.")
+
 @upper_half
 async def _establish_session(self) -> None:
 """
@@ -382,8 +421,8 @@ async def _establish_session(self) -> None:
 
 self._outgoing = asyncio.Queue()
 
-reader_coro = self._bh_loop_forever(self._bh_recv_message)
-writer_coro = self._bh_loop_forever(self._bh_send_message)
+reader_coro = self._bh_loop_forever(self._bh_recv_message, 'Reader')
+writer_coro = self._bh_loop_forever(self._bh_send_message, 'Writer')
 
 self

[PATCH v3 18/25] python/pylint: disable no-member check

[John Snow]

mypy handles this better -- but we only need the workaround because
pylint under Python 3.6 does not understand that a MutableMapping really
does have a .get() method attached.

We could remove this again once 3.7 is our minimum.

Signed-off-by: John Snow 
---
 python/setup.cfg | 1 +
 1 file changed, 1 insertion(+)

diff --git a/python/setup.cfg b/python/setup.cfg
index 19d5e154630..2573cd7bfb3 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -89,6 +89,7 @@ ignore_missing_imports = True
 # no Warning level messages displayed, use "--disable=all --enable=classes
 # --disable=W".
 disable=too-many-function-args,  # mypy handles this with less false positives.
+no-member,  # mypy also handles this better.
 
 [pylint.basic]
 # Good variable names which should always be accepted, separated by a comma.
-- 
2.31.1

[PATCH v3 01/25] python/aqmp: add asynchronous QMP (AQMP) subpackage

[John Snow]

For now, it's empty! Soon, it won't be.

Signed-off-by: John Snow 
Reviewed-by: Eric Blake 
---
 python/qemu/aqmp/__init__.py | 27 +++
 python/qemu/aqmp/py.typed|  0
 python/setup.cfg |  1 +
 3 files changed, 28 insertions(+)
 create mode 100644 python/qemu/aqmp/__init__.py
 create mode 100644 python/qemu/aqmp/py.typed

diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
new file mode 100644
index 000..391141c9484
--- /dev/null
+++ b/python/qemu/aqmp/__init__.py
@@ -0,0 +1,27 @@
+"""
+QEMU Monitor Protocol (QMP) development library & tooling.
+
+This package provides a fairly low-level class for communicating
+asynchronously with QMP protocol servers, as implemented by QEMU, the
+QEMU Guest Agent, and the QEMU Storage Daemon.
+
+`QMPClient` provides the main functionality of this package. All errors
+raised by this library dervive from `AQMPError`, see `aqmp.error` for
+additional detail. See `aqmp.events` for an in-depth tutorial on
+managing QMP events.
+"""
+
+# Copyright (C) 2020, 2021 John Snow for Red Hat, Inc.
+#
+# Authors:
+#  John Snow 
+#
+# Based on earlier work by Luiz Capitulino .
+#
+# This work is licensed under the terms of the GNU GPL, version 2.  See
+# the COPYING file in the top-level directory.
+
+
+# The order of these fields impact the Sphinx documentation order.
+__all__ = (
+)
diff --git a/python/qemu/aqmp/py.typed b/python/qemu/aqmp/py.typed
new file mode 100644
index 000..e69de29bb2d
diff --git a/python/setup.cfg b/python/setup.cfg
index 14bab902883..ffb754fa9e5 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -27,6 +27,7 @@ packages =
 qemu.qmp
 qemu.machine
 qemu.utils
+qemu.aqmp
 
 [options.package_data]
 * = py.typed
-- 
2.31.1

[PATCH v3 03/25] python/pylint: Add exception for TypeVar names ('T')

[John Snow]

'T' is a common TypeVar name, allow its use.

See also https://github.com/PyCQA/pylint/issues/3401 -- In the future,
we might be able to have a separate list of acceptable names for
TypeVars exclusively.

Signed-off-by: John Snow 
Reviewed-by: Eric Blake 
---
 python/setup.cfg | 1 +
 1 file changed, 1 insertion(+)

diff --git a/python/setup.cfg b/python/setup.cfg
index ffb754fa9e5..f87e32177ab 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -101,6 +101,7 @@ good-names=i,
fh,  # fh = open(...)
fd,  # fd = os.open(...)
c,   # for c in string: ...
+   T,   # for TypeVars. See pylint#3401
 
 [pylint.similarities]
 # Ignore imports when computing similarities.
-- 
2.31.1

[PATCH v3 02/25] python/aqmp: add error classes

[John Snow]

Signed-off-by: John Snow 
Reviewed-by: Eric Blake 
---
 python/qemu/aqmp/__init__.py |  4 +++
 python/qemu/aqmp/error.py| 50 
 2 files changed, 54 insertions(+)
 create mode 100644 python/qemu/aqmp/error.py

diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index 391141c9484..c97be950bf4 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -21,7 +21,11 @@
 # This work is licensed under the terms of the GNU GPL, version 2.  See
 # the COPYING file in the top-level directory.
 
+from .error import AQMPError
+
 
 # The order of these fields impact the Sphinx documentation order.
 __all__ = (
+# Exceptions
+'AQMPError',
 )
diff --git a/python/qemu/aqmp/error.py b/python/qemu/aqmp/error.py
new file mode 100644
index 000..781f49b0087
--- /dev/null
+++ b/python/qemu/aqmp/error.py
@@ -0,0 +1,50 @@
+"""
+AQMP Error Classes
+
+This package seeks to provide semantic error classes that are intended
+to be used directly by clients when they would like to handle particular
+semantic failures (e.g. "failed to connect") without needing to know the
+enumeration of possible reasons for that failure.
+
+AQMPError serves as the ancestor for all exceptions raised by this
+package, and is suitable for use in handling semantic errors from this
+library. In most cases, individual public methods will attempt to catch
+and re-encapsulate various exceptions to provide a semantic
+error-handling interface.
+
+.. admonition:: AQMP Exception Hierarchy Reference
+
+ |   `Exception`
+ |+-- `AQMPError`
+ | +-- `ConnectError`
+ | +-- `StateError`
+ | +-- `ExecInterruptedError`
+ | +-- `ExecuteError`
+ | +-- `ListenerError`
+ | +-- `ProtocolError`
+ |  +-- `DeserializationError`
+ |  +-- `UnexpectedTypeError`
+ |  +-- `ServerParseError`
+ |  +-- `BadReplyError`
+ |  +-- `GreetingError`
+ |  +-- `NegotiationError`
+"""
+
+
+class AQMPError(Exception):
+"""Abstract error class for all errors originating from this package."""
+
+
+class ProtocolError(AQMPError):
+"""
+Abstract error class for protocol failures.
+
+Semantically, these errors are generally the fault of either the
+protocol server or as a result of a bug in this library.
+
+:param error_message: Human-readable string describing the error.
+"""
+def __init__(self, error_message: str):
+super().__init__(error_message)
+#: Human-readable error message, without any prefix.
+self.error_message: str = error_message
-- 
2.31.1

Re: [PATCH v2 01/55] hw/core: Make do_unaligned_access noreturn

[Richard Henderson]

On 8/3/21 5:47 AM, Alex Bennée wrote:

These trailing QEMU_NORETURN's seem to be fairly uncommon in the
existing code.


Showing my age, I suppose.  Once upon a time it was the only place you *could* put it in a 
declaration (as opposed to definition).



r~

Re: [PATCH v2 02/24] python/aqmp: add error classes

[John Snow]

On Tue, Aug 3, 2021 at 1:40 PM Eric Blake  wrote:

> On Tue, Aug 03, 2021 at 01:34:32PM -0400, John Snow wrote:
> > Got it. I was *just* about to send a refreshed version of this patchset
> > because I found a new bug while on my way to making a sync compatibility
> > shim for iotests -- Do you have more feedback cooking, or should I hit
> the
> > send button?
>
> I spotted another typo while browsing the web page (disconnect() "If
> there were was an exception"), but I'm fine if you re-send, and I'll
>

Thanks for spotting that. Your proofreading ability is admired and
appreciated :)


> resume looking at the series on the updated v3.  For 1-6, you can add:
>
> Reviewed-by: Eric Blake 
>
> although my python is weak enough that you may want another set of
> eyes as well.
>
>
Thanks! Review on overall design, documentation, layout, organization and
presentation is plenty helpful even if you aren't necessarily eagle-eyed on
minutiae of Python. (Maybe especially if?)
I've written quite a few tests and have used this library to run our entire
iotests suite, plus Niteesh has been banging the bits pretty hard while
working on aqmp-shell, so I am not too fearful of mechanical errors.

Re: [PATCH v6 02/11] qapi: wrap Sequence[str] in an object

[John Snow]

On Mon, Aug 2, 2021 at 5:21 AM Markus Armbruster  wrote:

> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau 
> >
> > Except for the special casing assert in _make_implicit_object_type(),
> > which needs to handle schema objects, it's a mechanical change.
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >  docs/sphinx/qapidoc.py | 10 +++---
> >  scripts/qapi/commands.py   |  4 +--
> >  scripts/qapi/events.py |  5 +--
> >  scripts/qapi/gen.py| 14 
> >  scripts/qapi/introspect.py | 26 +++---
> >  scripts/qapi/schema.py | 66 +-
> >  scripts/qapi/types.py  | 33 -
> >  scripts/qapi/visit.py  | 23 ++--
> >  tests/qapi-schema/test-qapi.py |  4 +--
> >  9 files changed, 103 insertions(+), 82 deletions(-)
> >
> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> > index 87c67ab23f..0eac3308b2 100644
> > --- a/docs/sphinx/qapidoc.py
> > +++ b/docs/sphinx/qapidoc.py
> > @@ -116,7 +116,7 @@ def _nodes_for_ifcond(self, ifcond, with_if=True):
> >  the conditions are in literal-text and the commas are not.
> >  If with_if is False, we don't return the "(If: " and ")".
> >  """
> > -condlist = intersperse([nodes.literal('', c) for c in ifcond],
> > +condlist = intersperse([nodes.literal('', c) for c in
> ifcond.ifcond],
>
> Mechanical pattern #1: ifcond becomes ifcond.ifcond to peel off the new
> wrapper.  ifcond.ifcond is ugly, but almost all instances go away in
> this series.  I'm okay with the remainder.
>
> > nodes.Text(', '))
> >  if not with_if:
> >  return condlist
> > @@ -139,7 +139,7 @@ def _nodes_for_one_member(self, member):
> >  term.append(nodes.literal('', member.type.doc_type()))
> >  if member.optional:
> >  term.append(nodes.Text(' (optional)'))
> > -if member.ifcond:
> > +if member.ifcond.ifcond:
> >  term.extend(self._nodes_for_ifcond(member.ifcond))
> >  return term
> >
> > @@ -154,7 +154,7 @@ def _nodes_for_variant_when(self, variants, variant):
> >  nodes.literal('', variants.tag_member.name),
> >  nodes.Text(' is '),
> >  nodes.literal('', '"%s"' % variant.name)]
> > -if variant.ifcond:
> > +if variant.ifcond.ifcond:
> >  term.extend(self._nodes_for_ifcond(variant.ifcond))
> >  return term
> >
> > @@ -209,7 +209,7 @@ def _nodes_for_enum_values(self, doc):
> >  dlnode = nodes.definition_list()
> >  for section in doc.args.values():
> >  termtext = [nodes.literal('', section.member.name)]
> > -if section.member.ifcond:
> > +if section.member.ifcond.ifcond:
> >
> termtext.extend(self._nodes_for_ifcond(section.member.ifcond))
> >  # TODO drop fallbacks when undocumented members are outlawed
> >  if section.text:
> > @@ -277,7 +277,7 @@ def _nodes_for_sections(self, doc):
> >  def _nodes_for_if_section(self, ifcond):
> >  """Return list of doctree nodes for the "If" section"""
> >  nodelist = []
> > -if ifcond:
> > +if ifcond.ifcond:
> >  snode = self._make_section('If')
> >  snode += nodes.paragraph(
> >  '', '', *self._nodes_for_ifcond(ifcond, with_if=False)
> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> > index 0e13d51054..3654825968 100644
> > --- a/scripts/qapi/commands.py
> > +++ b/scripts/qapi/commands.py
> > @@ -17,7 +17,6 @@
> >  Dict,
> >  List,
> >  Optional,
> > -Sequence,
> >  Set,
> >  )
> >
> > @@ -31,6 +30,7 @@
> >  from .schema import (
> >  QAPISchema,
> >  QAPISchemaFeature,
> > +QAPISchemaIfCond,
> >  QAPISchemaObjectType,
> >  QAPISchemaType,
> >  )
> > @@ -301,7 +301,7 @@ def visit_end(self) -> None:
> >  def visit_command(self,
> >name: str,
> >info: Optional[QAPISourceInfo],
> > -  ifcond: Sequence[str],
> > +  ifcond: QAPISchemaIfCond,
> >features: List[QAPISchemaFeature],
> >arg_type: Optional[QAPISchemaObjectType],
> >ret_type: Optional[QAPISchemaType],
>
> Mechanical pattern #2: Sequence[str] becomes QAPISchemaIfCond.  Also
> obvious import adjustments.
>
> > diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> > index fee8c671e7..82475e84ec 100644
> > --- a/scripts/qapi/events.py
> > +++ b/scripts/qapi/events.py
> > @@ -12,7 +12,7 @@
> >  See the COPYING file in the top-level directory.
> >  """
> >
> > -from typing import List, Optional, Sequence
> > +from typing import List, Optional
> >
> >  from .common import c_enum_const, c_name, mcgen
> >  from .gen import QAPISchemaModularCVisitor, b

[PULL 2/2] hw/sd/sdcard: Fix assertion accessing out-of-range addresses with CMD30

[Philippe Mathieu-Daudé]

OSS-Fuzz found sending illegal addresses when querying the write
protection bits triggers the assertion added in commit 84816fb63e5
("hw/sd/sdcard: Assert if accessing an illegal group"):

  qemu-fuzz-i386-target-generic-fuzz-sdhci-v3: ../hw/sd/sd.c:824: uint32_t 
sd_wpbits(SDState *, uint64_t):
  Assertion `wpnum < sd->wpgrps_size' failed.
  #3 0x7f62a8b22c91 in __assert_fail
  #4 0x5569adcec405 in sd_wpbits hw/sd/sd.c:824:9
  #5 0x5569adce5f6d in sd_normal_command hw/sd/sd.c:1389:38
  #6 0x5569adce3870 in sd_do_command hw/sd/sd.c:1737:17
  #7 0x5569adcf1566 in sdbus_do_command hw/sd/core.c:100:16
  #8 0x5569adcfc192 in sdhci_send_command hw/sd/sdhci.c:337:12
  #9 0x5569adcfa3a3 in sdhci_write hw/sd/sdhci.c:1186:9
  #10 0x5569adfb3447 in memory_region_write_accessor softmmu/memory.c:492:5

It is legal for the CMD30 to query for out-of-range addresses.
Such invalid addresses are simply ignored in the response (write
protection bits set to 0).

In commit 84816fb63e5 ("hw/sd/sdcard: Assert if accessing an illegal
group") we misplaced the assertion *before* we test the address is
in range. Move it *after*.

Include the qtest reproducer provided by Alexander Bulekov:

  $ make check-qtest-i386
  ...
  Running test qtest-i386/fuzz-sdcard-test
  qemu-system-i386: ../hw/sd/sd.c:824: sd_wpbits: Assertion `wpnum < 
sd->wpgrps_size' failed.

Cc: qemu-sta...@nongnu.org
Reported-by: OSS-Fuzz (Issue 29225)
Suggested-by: Peter Maydell 
Fixes: 84816fb63e5 ("hw/sd/sdcard: Assert if accessing an illegal group")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/495
Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20210802235524.3417739-3-f4...@amsat.org>
Reviewed-by: Peter Maydell 
Tested-by: Alexander Bulekov 
---
 hw/sd/sd.c |  2 +-
 tests/qtest/fuzz-sdcard-test.c | 36 ++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 707dcc12a14..bb5dbff68c0 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -821,7 +821,6 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
 wpnum = sd_addr_to_wpnum(addr);
 
 for (i = 0; i < 32; i++, wpnum++, addr += WPGROUP_SIZE) {
-assert(wpnum < sd->wpgrps_size);
 if (addr >= sd->size) {
 /*
  * If the addresses of the last groups are outside the valid range,
@@ -829,6 +828,7 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
  */
 continue;
 }
+assert(wpnum < sd->wpgrps_size);
 if (test_bit(wpnum, sd->wp_groups)) {
 ret |= (1 << i);
 }
diff --git a/tests/qtest/fuzz-sdcard-test.c b/tests/qtest/fuzz-sdcard-test.c
index 96602eac7e5..ae14305344a 100644
--- a/tests/qtest/fuzz-sdcard-test.c
+++ b/tests/qtest/fuzz-sdcard-test.c
@@ -52,6 +52,41 @@ static void oss_fuzz_29225(void)
 qtest_quit(s);
 }
 
+/*
+ * https://gitlab.com/qemu-project/qemu/-/issues/495
+ * Used to trigger:
+ *  Assertion `wpnum < sd->wpgrps_size' failed.
+ */
+static void oss_fuzz_36217(void)
+{
+QTestState *s;
+
+s = qtest_init(" -display none -m 32 -nodefaults -nographic"
+   " -device sdhci-pci,sd-spec-version=3 "
+   "-device sd-card,drive=d0 "
+   "-drive if=none,index=0,file=null-co://,format=raw,id=d0");
+
+qtest_outl(s, 0xcf8, 0x80001010);
+qtest_outl(s, 0xcfc, 0xe000);
+qtest_outl(s, 0xcf8, 0x80001004);
+qtest_outw(s, 0xcfc, 0x02);
+qtest_bufwrite(s, 0xe02c, "\x05", 0x1);
+qtest_bufwrite(s, 0xe00f, "\x37", 0x1);
+qtest_bufwrite(s, 0xe00a, "\x01", 0x1);
+qtest_bufwrite(s, 0xe00f, "\x29", 0x1);
+qtest_bufwrite(s, 0xe00f, "\x02", 0x1);
+qtest_bufwrite(s, 0xe00f, "\x03", 0x1);
+qtest_bufwrite(s, 0xe005, "\x01", 0x1);
+qtest_bufwrite(s, 0xe00f, "\x06", 0x1);
+qtest_bufwrite(s, 0xe00c, "\x05", 0x1);
+qtest_bufwrite(s, 0xe00e, "\x20", 0x1);
+qtest_bufwrite(s, 0xe00f, "\x08", 0x1);
+qtest_bufwrite(s, 0xe00b, "\x3d", 0x1);
+qtest_bufwrite(s, 0xe00f, "\x1e", 0x1);
+
+qtest_quit(s);
+}
+
 int main(int argc, char **argv)
 {
 const char *arch = qtest_get_arch();
@@ -60,6 +95,7 @@ int main(int argc, char **argv)
 
if (strcmp(arch, "i386") == 0) {
 qtest_add_func("fuzz/sdcard/oss_fuzz_29225", oss_fuzz_29225);
+qtest_add_func("fuzz/sdcard/oss_fuzz_36217", oss_fuzz_36217);
}
 
return g_test_run();
-- 
2.31.1

[PULL 1/2] hw/sd/sdcard: Document out-of-range addresses for SEND_WRITE_PROT

[Philippe Mathieu-Daudé]

Per the 'Physical Layer Simplified Specification Version 3.01',
Table 4-22: 'Block Oriented Write Protection Commands'

  SEND_WRITE_PROT (CMD30)

  If the card provides write protection features, this command asks
  the card to send the status of the write protection bits [1].

  [1] 32 write protection bits (representing 32 write protect groups
  starting at the specified address) [...]
  The last (least significant) bit of the protection bits corresponds
  to the first addressed group. If the addresses of the last groups
  are outside the valid range, then the corresponding write protection
  bits shall be set to 0.

Split the if() statement (without changing the behaviour of the code)
to better position the description comment.

Reviewed-by: Alexander Bulekov 
Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20210802235524.3417739-2-f4...@amsat.org>
Reviewed-by: Peter Maydell 
Tested-by: Alexander Bulekov 
---
 hw/sd/sd.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 1f964e022b1..707dcc12a14 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -822,7 +822,14 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
 
 for (i = 0; i < 32; i++, wpnum++, addr += WPGROUP_SIZE) {
 assert(wpnum < sd->wpgrps_size);
-if (addr < sd->size && test_bit(wpnum, sd->wp_groups)) {
+if (addr >= sd->size) {
+/*
+ * If the addresses of the last groups are outside the valid range,
+ * then the corresponding write protection bits shall be set to 0.
+ */
+continue;
+}
+if (test_bit(wpnum, sd->wp_groups)) {
 ret |= (1 << i);
 }
 }
-- 
2.31.1

Re: [PATCH v2 02/24] python/aqmp: add error classes

[Eric Blake]

On Tue, Aug 03, 2021 at 01:34:32PM -0400, John Snow wrote:
> Got it. I was *just* about to send a refreshed version of this patchset
> because I found a new bug while on my way to making a sync compatibility
> shim for iotests -- Do you have more feedback cooking, or should I hit the
> send button?

I spotted another typo while browsing the web page (disconnect() "If
there were was an exception"), but I'm fine if you re-send, and I'll
resume looking at the series on the updated v3.  For 1-6, you can add:

Reviewed-by: Eric Blake 

although my python is weak enough that you may want another set of
eyes as well.

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

[PULL 0/2] SD/MMC patches for 2021-08-03

[Philippe Mathieu-Daudé]

The following changes since commit acf8200722251a0a995cfa75fe5c15aea0886418:

  Merge remote-tracking branch 
'remotes/mdroth/tags/qga-pull-2021-08-03-pull-tag' into staging (2021-08-03 
14:48:57 +0100)

are available in the Git repository at:

  https://github.com/philmd/qemu.git tags/sdmmc-20210803

for you to fetch changes up to 4ac0b72bae85cf94ae0e5153b9c2c288c71667d4:

  hw/sd/sdcard: Fix assertion accessing out-of-range addresses with CMD30 
(2021-08-03 19:34:51 +0200)


SD/MMC patches queue

- sdcard: Fix assertion accessing out-of-range addresses
  with SEND_WRITE_PROT (CMD30)



Philippe Mathieu-Daudé (2):
  hw/sd/sdcard: Document out-of-range addresses for SEND_WRITE_PROT
  hw/sd/sdcard: Fix assertion accessing out-of-range addresses with
CMD30

 hw/sd/sd.c |  9 -
 tests/qtest/fuzz-sdcard-test.c | 36 ++
 2 files changed, 44 insertions(+), 1 deletion(-)

-- 
2.31.1

Re: [PATCH v2 02/24] python/aqmp: add error classes

[John Snow]

Got it. I was *just* about to send a refreshed version of this patchset
because I found a new bug while on my way to making a sync compatibility
shim for iotests -- Do you have more feedback cooking, or should I hit the
send button?

--js

On Tue, Aug 3, 2021 at 12:02 PM Eric Blake  wrote:

> On Fri, Jul 16, 2021 at 08:32:31PM -0400, John Snow wrote:
> > Signed-off-by: John Snow 
> > ---
> >  python/qemu/aqmp/__init__.py |  4 +++
> >  python/qemu/aqmp/error.py| 50 
> >  2 files changed, 54 insertions(+)
> >  create mode 100644 python/qemu/aqmp/error.py
>
> > +++ b/python/qemu/aqmp/error.py
> > @@ -0,0 +1,50 @@
>
> > +
> > +class ProtocolError(AQMPError):
> > +"""
> > +Abstract error class for protocol failures.
> > +
> > +Semantically, these errors are generally the fault of either the
> > +protocol server or as a result of a bug in this this library.
>
> duplicate 'this'
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.   +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
>
>

Re: [PATCH-for-6.1 v2 0/2] hw/sd/sdcard: Fix assertion accessing out-of-range addresses with CMD30

[Philippe Mathieu-Daudé]

On 8/3/21 3:46 PM, Alexander Bulekov wrote:
> On 210803 0155, Philippe Mathieu-Daudé wrote:
>> Fix an assertion reported by OSS-Fuzz, add corresponding qtest.
>>
>> The change is (now) simple enough for the next rc.
>>
>> Since v1:
>> - Simplified/corrected following Peter's suggestion
>>
>> Philippe Mathieu-Daudé (2):
>>   hw/sd/sdcard: Document out-of-range addresses for SEND_WRITE_PROT
>>   hw/sd/sdcard: Fix assertion accessing out-of-range addresses with
>> CMD30
>>
> 
> Fuzzed this for 20 mins, based on the OSS-Fuzz corpus, without finding
> anything.
> 
> ./qemu-fuzz-i386 --fuzz-target=generic-fuzz-sdhci-v3 -jobs=4 -workers=4 \
> -focus_function=sd_wpbits \
> ~/oss-fuzz/qemu_qemu-fuzz-i386-target-generic-fuzz-sdhci-v3/  
> 
> Tested-by: Alexander Bulekov 

Thanks both!

Queued on sdmmc-fixes.

[PATCH] linux-user/elfload: byteswap i386 registers when dumping core

[Ilya Leoshkevich]

Core dumps from emulating x86_64 on big-endian hosts contain incorrect
register values.

Signed-off-by: Ilya Leoshkevich 
---
 linux-user/elfload.c | 88 ++--
 1 file changed, 44 insertions(+), 44 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 42ef2a1148..01e9a833fb 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -172,33 +172,33 @@ typedef target_elf_greg_t  target_elf_gregset_t[ELF_NREG];
  */
 static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUX86State 
*env)
 {
-(*regs)[0] = env->regs[15];
-(*regs)[1] = env->regs[14];
-(*regs)[2] = env->regs[13];
-(*regs)[3] = env->regs[12];
-(*regs)[4] = env->regs[R_EBP];
-(*regs)[5] = env->regs[R_EBX];
-(*regs)[6] = env->regs[11];
-(*regs)[7] = env->regs[10];
-(*regs)[8] = env->regs[9];
-(*regs)[9] = env->regs[8];
-(*regs)[10] = env->regs[R_EAX];
-(*regs)[11] = env->regs[R_ECX];
-(*regs)[12] = env->regs[R_EDX];
-(*regs)[13] = env->regs[R_ESI];
-(*regs)[14] = env->regs[R_EDI];
-(*regs)[15] = env->regs[R_EAX]; /* XXX */
-(*regs)[16] = env->eip;
-(*regs)[17] = env->segs[R_CS].selector & 0x;
-(*regs)[18] = env->eflags;
-(*regs)[19] = env->regs[R_ESP];
-(*regs)[20] = env->segs[R_SS].selector & 0x;
-(*regs)[21] = env->segs[R_FS].selector & 0x;
-(*regs)[22] = env->segs[R_GS].selector & 0x;
-(*regs)[23] = env->segs[R_DS].selector & 0x;
-(*regs)[24] = env->segs[R_ES].selector & 0x;
-(*regs)[25] = env->segs[R_FS].selector & 0x;
-(*regs)[26] = env->segs[R_GS].selector & 0x;
+(*regs)[0] = tswapreg(env->regs[15]);
+(*regs)[1] = tswapreg(env->regs[14]);
+(*regs)[2] = tswapreg(env->regs[13]);
+(*regs)[3] = tswapreg(env->regs[12]);
+(*regs)[4] = tswapreg(env->regs[R_EBP]);
+(*regs)[5] = tswapreg(env->regs[R_EBX]);
+(*regs)[6] = tswapreg(env->regs[11]);
+(*regs)[7] = tswapreg(env->regs[10]);
+(*regs)[8] = tswapreg(env->regs[9]);
+(*regs)[9] = tswapreg(env->regs[8]);
+(*regs)[10] = tswapreg(env->regs[R_EAX]);
+(*regs)[11] = tswapreg(env->regs[R_ECX]);
+(*regs)[12] = tswapreg(env->regs[R_EDX]);
+(*regs)[13] = tswapreg(env->regs[R_ESI]);
+(*regs)[14] = tswapreg(env->regs[R_EDI]);
+(*regs)[15] = tswapreg(env->regs[R_EAX]); /* XXX */
+(*regs)[16] = tswapreg(env->eip);
+(*regs)[17] = tswapreg(env->segs[R_CS].selector & 0x);
+(*regs)[18] = tswapreg(env->eflags);
+(*regs)[19] = tswapreg(env->regs[R_ESP]);
+(*regs)[20] = tswapreg(env->segs[R_SS].selector & 0x);
+(*regs)[21] = tswapreg(env->segs[R_FS].selector & 0x);
+(*regs)[22] = tswapreg(env->segs[R_GS].selector & 0x);
+(*regs)[23] = tswapreg(env->segs[R_DS].selector & 0x);
+(*regs)[24] = tswapreg(env->segs[R_ES].selector & 0x);
+(*regs)[25] = tswapreg(env->segs[R_FS].selector & 0x);
+(*regs)[26] = tswapreg(env->segs[R_GS].selector & 0x);
 }
 
 #else
@@ -244,23 +244,23 @@ typedef target_elf_greg_t  target_elf_gregset_t[ELF_NREG];
  */
 static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUX86State 
*env)
 {
-(*regs)[0] = env->regs[R_EBX];
-(*regs)[1] = env->regs[R_ECX];
-(*regs)[2] = env->regs[R_EDX];
-(*regs)[3] = env->regs[R_ESI];
-(*regs)[4] = env->regs[R_EDI];
-(*regs)[5] = env->regs[R_EBP];
-(*regs)[6] = env->regs[R_EAX];
-(*regs)[7] = env->segs[R_DS].selector & 0x;
-(*regs)[8] = env->segs[R_ES].selector & 0x;
-(*regs)[9] = env->segs[R_FS].selector & 0x;
-(*regs)[10] = env->segs[R_GS].selector & 0x;
-(*regs)[11] = env->regs[R_EAX]; /* XXX */
-(*regs)[12] = env->eip;
-(*regs)[13] = env->segs[R_CS].selector & 0x;
-(*regs)[14] = env->eflags;
-(*regs)[15] = env->regs[R_ESP];
-(*regs)[16] = env->segs[R_SS].selector & 0x;
+(*regs)[0] = tswapreg(env->regs[R_EBX]);
+(*regs)[1] = tswapreg(env->regs[R_ECX]);
+(*regs)[2] = tswapreg(env->regs[R_EDX]);
+(*regs)[3] = tswapreg(env->regs[R_ESI]);
+(*regs)[4] = tswapreg(env->regs[R_EDI]);
+(*regs)[5] = tswapreg(env->regs[R_EBP]);
+(*regs)[6] = tswapreg(env->regs[R_EAX]);
+(*regs)[7] = tswapreg(env->segs[R_DS].selector & 0x);
+(*regs)[8] = tswapreg(env->segs[R_ES].selector & 0x);
+(*regs)[9] = tswapreg(env->segs[R_FS].selector & 0x);
+(*regs)[10] = tswapreg(env->segs[R_GS].selector & 0x);
+(*regs)[11] = tswapreg(env->regs[R_EAX]); /* XXX */
+(*regs)[12] = tswapreg(env->eip);
+(*regs)[13] = tswapreg(env->segs[R_CS].selector & 0x);
+(*regs)[14] = tswapreg(env->eflags);
+(*regs)[15] = tswapreg(env->regs[R_ESP]);
+(*regs)[16] = tswapreg(env->segs[R_SS].selector & 0x);
 }
 #endif
 
-- 
2.31.1

[PATCH] linux-user: fix guest/host address mixup in i386 setup_rt_frame()

[Ilya Leoshkevich]

setup_rt_frame() passes siginfo and ucontext host addresses to guest
signal handlers, causing problems when e.g. emulating x86_64 on s390x.

Signed-off-by: Ilya Leoshkevich 
---
 linux-user/i386/signal.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/linux-user/i386/signal.c b/linux-user/i386/signal.c
index 8701774e37..841cd19651 100644
--- a/linux-user/i386/signal.c
+++ b/linux-user/i386/signal.c
@@ -436,13 +436,13 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
 
 #ifndef TARGET_X86_64
 env->regs[R_EAX] = sig;
-env->regs[R_EDX] = (unsigned long)&frame->info;
-env->regs[R_ECX] = (unsigned long)&frame->uc;
+env->regs[R_EDX] = frame_addr + offsetof(struct rt_sigframe, info);
+env->regs[R_ECX] = frame_addr + offsetof(struct rt_sigframe, uc);
 #else
 env->regs[R_EAX] = 0;
 env->regs[R_EDI] = sig;
-env->regs[R_ESI] = (unsigned long)&frame->info;
-env->regs[R_EDX] = (unsigned long)&frame->uc;
+env->regs[R_ESI] = frame_addr + offsetof(struct rt_sigframe, info);
+env->regs[R_EDX] = frame_addr + offsetof(struct rt_sigframe, uc);
 #endif
 
 cpu_x86_load_seg(env, R_DS, __USER_DS);
-- 
2.31.1

Re: Failing iotest 206

[Kevin Wolf]

Am 20.07.2021 um 10:32 hat Daniel P. Berrangé geschrieben:
> On Mon, Jul 19, 2021 at 08:12:58PM -0500, Eric Blake wrote:
> > On Mon, Jul 19, 2021 at 10:06:01AM +0200, Thomas Huth wrote:
> > >  Hi,
> > > 
> > > iotest 206 fails for me with:
> > > 
> > 
> > > --- 206.out
> > > +++ 206.out.bad
> > > @@ -99,55 +99,19 @@
> > > 
> > >  {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options":
> > > {"driver": "qcow2", "encrypt": {"cipher-alg": "twofish-128", 
> > > "cipher-mode":
> > > "ctr", "format": "luks", "hash-alg": "sha1", "iter-time": 10, "ivgen-alg":
> > > "plain64", "ivgen-hash-alg": "md5", "key-secret": "keysec0"}, "file":
> > > {"driver": "file", "filename": "TEST_DIR/PID-t.qcow2"}, "size": 
> > > 33554432}}}
> > >  {"return": {}}
> > > +Job failed: Unsupported cipher algorithm twofish-128 with ctr mode
> > >  {"execute": "job-dismiss", "arguments": {"id": "job0"}}
> > >  {"return": {}}
> > 
> > > 
> > > Looks like it is missing a check for the availability of the corresponding
> > > crypto stuff? Does anybody got a clue how to fix this?
> > 
> > What system is this on? Which crypto library versions are installed?
> > I suspect this is related to Dan's effort to speed up crypto by
> > favoring gnutls over nettle, where the switch in favored libraries
> > failed to account for whether twofish-128 is supported?
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2021-07/msg03886.html
> 
> Yes, the gnutls provider doesn't support twofish. This doesn't matter
> in real world usage because no one is seriously going to ask for twofish
> instead of AES for luks encryption.
> 
> I guess that test suite was simply trying to ask for some non-default
> values though.

Do we already have a patch somewhere that makes it use a different
value? Or if not, which value would be most likely to work everywhere?

Kevin

Re: [PULL for-6.1 0/6] qemu-ga patch queue for hard-freeze

[Peter Maydell]

On Tue, 3 Aug 2021 at 14:26, Michael Roth  wrote:
>
> Hi Peter,
>
> Sorry for the late submission. These patches affect only the w32 build of
> qemu-ga. A number of these patches I've had queued for some time, but a bug
> in the MSI installer that was just fixed was blocking testing. Now that that
> is working again I am hoping to get these in along with a couple of other
> fixes that have come in since then.
>
> The following changes since commit 7f1cab9c628a798ae2607940993771e6300e9e00:
>
>   Merge remote-tracking branch 'remotes/bonzini-gitlab/tags/for-upstream' 
> into staging (2021-08-02 17:21:50 +0100)
>
> are available in the Git repository at:
>
>   git://github.com/mdroth/qemu.git tags/qga-pull-2021-08-03-pull-tag
>
> for you to fetch changes up to e300858ed4a6d0cbd52b7fb5082d3c69cc371965:
>
>   qga-win/msi: fix missing libstdc++-6 DLL in MSI installer (2021-08-03 
> 07:01:36 -0500)
>
> 
> qemu-ga patch queue for hard-freeze
>
> * w32: Fix missing/incorrect DLLs in MSI installer
> * w32: Fix memory leaks in guest-get-osinfo/guest-get-fsinfo
> * w32: Increase timeout for guest-fsfreeze-freeze
>
> 


Applied, thanks.

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

-- PMM

[PATCH v3 1/1] hw/i2c: add remote I2C device

[Shengtan Mao]

This patch adds the remote I2C device, which supports the usage of
external I2C devices.
Signed-off-by: Shengtan Mao 
---
 hw/arm/Kconfig|   1 +
 hw/i2c/Kconfig|   4 +
 hw/i2c/meson.build|   1 +
 hw/i2c/remote-i2c.c   | 117 ++
 tests/qtest/meson.build   |   1 +
 tests/qtest/remote-i2c-test.c | 216 ++
 6 files changed, 340 insertions(+)
 create mode 100644 hw/i2c/remote-i2c.c
 create mode 100644 tests/qtest/remote-i2c-test.c

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 90b19c0861..58fdfab90d 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -392,6 +392,7 @@ config NPCM7XX
 select MAX34451
 select PL310  # cache controller
 select PMBUS
+select REMOTE_I2C
 select SERIAL
 select SSI
 select UNIMP
diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
index 8217cb5041..278156991d 100644
--- a/hw/i2c/Kconfig
+++ b/hw/i2c/Kconfig
@@ -1,6 +1,10 @@
 config I2C
 bool
 
+config REMOTE_I2C
+bool
+select I2C
+
 config SMBUS
 bool
 select I2C
diff --git a/hw/i2c/meson.build b/hw/i2c/meson.build
index d3df273251..ba0215db61 100644
--- a/hw/i2c/meson.build
+++ b/hw/i2c/meson.build
@@ -6,6 +6,7 @@ i2c_ss.add(when: 'CONFIG_ACPI_X86_ICH', if_true: 
files('smbus_ich9.c'))
 i2c_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_i2c.c'))
 i2c_ss.add(when: 'CONFIG_BITBANG_I2C', if_true: files('bitbang_i2c.c'))
 i2c_ss.add(when: 'CONFIG_EXYNOS4', if_true: files('exynos4210_i2c.c'))
+i2c_ss.add(when: 'CONFIG_REMOTE_I2C', if_true: files('remote-i2c.c'))
 i2c_ss.add(when: 'CONFIG_IMX_I2C', if_true: files('imx_i2c.c'))
 i2c_ss.add(when: 'CONFIG_MPC_I2C', if_true: files('mpc_i2c.c'))
 i2c_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('microbit_i2c.c'))
diff --git a/hw/i2c/remote-i2c.c b/hw/i2c/remote-i2c.c
new file mode 100644
index 00..083eaf2210
--- /dev/null
+++ b/hw/i2c/remote-i2c.c
@@ -0,0 +1,117 @@
+/*
+ * Remote I2C Device
+ *
+ * Copyright (c) 2021 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "qemu/osdep.h"
+
+#include "chardev/char-fe.h"
+#include "hw/i2c/i2c.h"
+#include "hw/qdev-properties-system.h"
+
+#define TYPE_REMOTE_I2C "remote-i2c"
+#define REMOTE_I2C(obj) OBJECT_CHECK(RemoteI2CState, (obj), TYPE_REMOTE_I2C)
+#define ONE_BYTE 1
+
+typedef struct {
+I2CSlave parent_obj;
+CharBackend chr;
+} RemoteI2CState;
+
+typedef enum {
+REMOTE_I2C_START_RECV = 0,
+REMOTE_I2C_START_SEND = 1,
+REMOTE_I2C_FINISH = 2,
+REMOTE_I2C_NACK = 3,
+REMOTE_I2C_RECV = 4,
+REMOTE_I2C_SEND = 5,
+} RemoteI2CCommand;
+
+static uint8_t remote_i2c_recv(I2CSlave *s)
+{
+RemoteI2CState *i2c = REMOTE_I2C(s);
+uint8_t resp = 0;
+uint8_t type = REMOTE_I2C_RECV;
+qemu_chr_fe_write_all(&i2c->chr, &type, ONE_BYTE);
+
+qemu_chr_fe_read_all(&i2c->chr, &resp, ONE_BYTE);
+return resp;
+}
+
+static int remote_i2c_send(I2CSlave *s, uint8_t data)
+{
+RemoteI2CState *i2c = REMOTE_I2C(s);
+uint8_t type = REMOTE_I2C_SEND;
+uint8_t resp = 1;
+qemu_chr_fe_write_all(&i2c->chr, &type, ONE_BYTE);
+qemu_chr_fe_write_all(&i2c->chr, &data, ONE_BYTE);
+
+qemu_chr_fe_read_all(&i2c->chr, &resp, ONE_BYTE);
+return resp ? -1 : 0;
+}
+
+/* Returns non-zero when no response from the device. */
+static int remote_i2c_event(I2CSlave *s, enum i2c_event event)
+{
+RemoteI2CState *i2c = REMOTE_I2C(s);
+uint8_t type;
+uint8_t resp = 1;
+switch (event) {
+case I2C_START_RECV:
+type = REMOTE_I2C_START_RECV;
+break;
+case I2C_START_SEND:
+type = REMOTE_I2C_START_SEND;
+break;
+case I2C_FINISH:
+type = REMOTE_I2C_FINISH;
+break;
+case I2C_NACK:
+type = REMOTE_I2C_NACK;
+}
+qemu_chr_fe_write_all(&i2c->chr, &type, ONE_BYTE);
+qemu_chr_fe_read_all(&i2c->chr, &resp, ONE_BYTE);
+return resp ? -1 : 0;
+}
+
+static Property remote_i2c_props[] = {
+DEFINE_PROP_CHR("chardev", RemoteI2CState, chr),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void remote_i2c_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
+
+k->recv = &remote_i2c_recv;
+k->send = &remote_i2c_send;
+k->event = &remote_i2c_event;
+device_class_set_props(dc, remote_i2c_props);
+}
+
+static const TypeInfo remote_i2c_type = {
+.name = TYPE_REMOTE_I2C,
+.parent = TYPE_I2C_SLAVE,
+.ins

[PATCH v3 0/1] Add remote I2C device to support external I2C device

[Shengtan Mao]

This patch implements the remote I2C device.
The remote I2C device allows an external I2C device to communicate with the I2C 
controller in QEMU through the remote I2C protocol.
Users no longer have to directly modify QEMU to add new I2C devices and can 
instead implement the emulated device externally and connect it to the remote 
I2C device.

Previous work by Wolfram Sang 
(https://git.kernel.org/pub/scm/virt/qemu/wsa/qemu.git/commit/?h=i2c-passthrough)
 was referenced.
It shares the similar idea of redirecting the actual I2C device 
functionalities, but Sang focuses on physical devices, and we focus on emulated 
devices.
The work by Sang mainly utilizes file descriptors while ours utilizes character 
devices, which offers better support for emulated devices.
The work by Sang is not meant to offer full I2C device support; it only 
implements the receive functionality.
Our work implements full support for I2C devices: send, recv, and event 
(match_and_add is not applicable for external devices).

v1 -> v2
fixed terminology errors in the description comments.

v2 -> v3
corrected patch set emailing errors.

Shengtan Mao (1):
  hw/i2c: add remote I2C device

 hw/arm/Kconfig|   1 +
 hw/i2c/Kconfig|   4 +
 hw/i2c/meson.build|   1 +
 hw/i2c/remote-i2c.c   | 117 ++
 tests/qtest/meson.build   |   1 +
 tests/qtest/remote-i2c-test.c | 216 ++
 6 files changed, 340 insertions(+)
 create mode 100644 hw/i2c/remote-i2c.c
 create mode 100644 tests/qtest/remote-i2c-test.c

-- 
2.32.0.554.ge1b32706d8-goog

Re: [PATCH v2 02/24] python/aqmp: add error classes

[Eric Blake]

On Fri, Jul 16, 2021 at 08:32:31PM -0400, John Snow wrote:
> Signed-off-by: John Snow 
> ---
>  python/qemu/aqmp/__init__.py |  4 +++
>  python/qemu/aqmp/error.py| 50 
>  2 files changed, 54 insertions(+)
>  create mode 100644 python/qemu/aqmp/error.py

> +++ b/python/qemu/aqmp/error.py
> @@ -0,0 +1,50 @@

> +
> +class ProtocolError(AQMPError):
> +"""
> +Abstract error class for protocol failures.
> +
> +Semantically, these errors are generally the fault of either the
> +protocol server or as a result of a bug in this this library.

duplicate 'this'

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

Re: [PATCH v2 21/55] accel/tcg: Drop signness in tracing in cputlb.c

[Alex Bennée]

Richard Henderson  writes:

> We are already inconsistent about whether or not
> MO_SIGN is set in trace_mem_get_info.  Dropping it
> entirely allows some simplification.

I think once 6.2 opens up we should just drop all the trace_mem stuff:

  Subject: [PATCH  v1 5/7] docs: mark intention to deprecate TCG tracing 
functionality
  Date: Wed,  5 May 2021 10:22:57 +0100
  Message-Id: <20210505092259.8202-6-alex.ben...@linaro.org>

>
> Signed-off-by: Richard Henderson 
> ---
>  accel/tcg/cputlb.c| 10 +++---
>  accel/tcg/user-exec.c | 45 ++-
>  2 files changed, 9 insertions(+), 46 deletions(-)
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 116b289907..acdd20b1bc 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -2110,7 +2110,6 @@ static inline uint64_t cpu_load_helper(CPUArchState 
> *env, abi_ptr addr,
>  meminfo = trace_mem_get_info(op, mmu_idx, false);
>  trace_guest_mem_before_exec(env_cpu(env), addr, meminfo);
>  
> -op &= ~MO_SIGN;
>  oi = make_memop_idx(op, mmu_idx);
>  ret = full_load(env, addr, oi, retaddr);
>  
> @@ -2128,8 +2127,7 @@ uint32_t cpu_ldub_mmuidx_ra(CPUArchState *env, abi_ptr 
> addr,
>  int cpu_ldsb_mmuidx_ra(CPUArchState *env, abi_ptr addr,
> int mmu_idx, uintptr_t ra)
>  {
> -return (int8_t)cpu_load_helper(env, addr, mmu_idx, ra, MO_SB,
> -   full_ldub_mmu);
> +return (int8_t)cpu_ldub_mmuidx_ra(env, addr, mmu_idx, ra);
>  }
>  
>  uint32_t cpu_lduw_be_mmuidx_ra(CPUArchState *env, abi_ptr addr,
> @@ -2141,8 +2139,7 @@ uint32_t cpu_lduw_be_mmuidx_ra(CPUArchState *env, 
> abi_ptr addr,
>  int cpu_ldsw_be_mmuidx_ra(CPUArchState *env, abi_ptr addr,
>int mmu_idx, uintptr_t ra)
>  {
> -return (int16_t)cpu_load_helper(env, addr, mmu_idx, ra, MO_BESW,
> -full_be_lduw_mmu);
> +return (int16_t)cpu_lduw_be_mmuidx_ra(env, addr, mmu_idx, ra);
>  }
>  
>  uint32_t cpu_ldl_be_mmuidx_ra(CPUArchState *env, abi_ptr addr,
> @@ -2166,8 +2163,7 @@ uint32_t cpu_lduw_le_mmuidx_ra(CPUArchState *env, 
> abi_ptr addr,
>  int cpu_ldsw_le_mmuidx_ra(CPUArchState *env, abi_ptr addr,
>int mmu_idx, uintptr_t ra)
>  {
> -return (int16_t)cpu_load_helper(env, addr, mmu_idx, ra, MO_LESW,
> -full_le_lduw_mmu);
> +return (int16_t)cpu_lduw_le_mmuidx_ra(env, addr, mmu_idx, ra);
>  }
>  
>  uint32_t cpu_ldl_le_mmuidx_ra(CPUArchState *env, abi_ptr addr,
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index 5ad808a25a..e687b9652e 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -866,13 +866,7 @@ uint32_t cpu_ldub_data(CPUArchState *env, abi_ptr ptr)
>  
>  int cpu_ldsb_data(CPUArchState *env, abi_ptr ptr)
>  {
> -int ret;
> -uint16_t meminfo = trace_mem_get_info(MO_SB, MMU_USER_IDX, false);
> -
> -trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
> -ret = ldsb_p(g2h(env_cpu(env), ptr));
> -qemu_plugin_vcpu_mem_cb(env_cpu(env), ptr, meminfo);
> -return ret;
> +return (int8_t)cpu_ldub_data(env, ptr);
>  }
>  
>  uint32_t cpu_lduw_be_data(CPUArchState *env, abi_ptr ptr)
> @@ -888,13 +882,7 @@ uint32_t cpu_lduw_be_data(CPUArchState *env, abi_ptr ptr)
>  
>  int cpu_ldsw_be_data(CPUArchState *env, abi_ptr ptr)
>  {
> -int ret;
> -uint16_t meminfo = trace_mem_get_info(MO_BESW, MMU_USER_IDX, false);
> -
> -trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
> -ret = ldsw_be_p(g2h(env_cpu(env), ptr));
> -qemu_plugin_vcpu_mem_cb(env_cpu(env), ptr, meminfo);
> -return ret;
> +return (int16_t)cpu_lduw_be_data(env, ptr);
>  }
>  
>  uint32_t cpu_ldl_be_data(CPUArchState *env, abi_ptr ptr)
> @@ -932,13 +920,7 @@ uint32_t cpu_lduw_le_data(CPUArchState *env, abi_ptr ptr)
>  
>  int cpu_ldsw_le_data(CPUArchState *env, abi_ptr ptr)
>  {
> -int ret;
> -uint16_t meminfo = trace_mem_get_info(MO_LESW, MMU_USER_IDX, false);
> -
> -trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
> -ret = ldsw_le_p(g2h(env_cpu(env), ptr));
> -qemu_plugin_vcpu_mem_cb(env_cpu(env), ptr, meminfo);
> -return ret;
> +return (int16_t)cpu_lduw_le_data(env, ptr);
>  }
>  
>  uint32_t cpu_ldl_le_data(CPUArchState *env, abi_ptr ptr)
> @@ -975,12 +957,7 @@ uint32_t cpu_ldub_data_ra(CPUArchState *env, abi_ptr 
> ptr, uintptr_t retaddr)
>  
>  int cpu_ldsb_data_ra(CPUArchState *env, abi_ptr ptr, uintptr_t retaddr)
>  {
> -int ret;
> -
> -set_helper_retaddr(retaddr);
> -ret = cpu_ldsb_data(env, ptr);
> -clear_helper_retaddr();
> -return ret;
> +return (int8_t)cpu_ldub_data_ra(env, ptr, retaddr);
>  }
>  
>  uint32_t cpu_lduw_be_data_ra(CPUArchState *env, abi_ptr ptr, uintptr_t 
> retaddr)
> @@ -995,12 +972,7 @@ uint32_t cpu_lduw_be_data_ra(CPUArchState *env, abi_ptr 
> ptr, uintptr_t retaddr)
>  
>  int cpu_lds

Re: [PATCH v2 20/55] accel/tcg: Report unaligned atomics for user-only

[Alex Bennée]

Richard Henderson  writes:

> Use the newly exposed cpu_unaligned_access for atomic_mmu_lookup,
> which has access to complete alignment info from the TCGMemOpIdx arg.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 


> -void *ret = g2h(env_cpu(env), addr);
> +
> +ret = g2h(env_cpu(env), addr);
>  set_helper_retaddr(retaddr);
>  return ret;
>  }


-- 
Alex Bennée

Re: [PATCH v2 02/55] hw/core: Make do_unaligned_access available to user-only

[Alex Bennée]

Richard Henderson  writes:

> We shouldn't be ignoring SIGBUS for user-only.
>
> Move our existing TCGCPUOps hook out from CONFIG_SOFTMMU.
> Move the wrapper, cpu_unaligned_access, to cpu-exec-common.c.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

-- 
Alex Bennée

Re: [PATCH 2/2] doc: Remove trailing spaces

[Peter Maydell]

On Tue, 3 Aug 2021 at 16:24, Axel Heider  wrote:
> Then please drop the patch 2/2 for now and just keep 1/2. I will run
> more tests about this and see if I can find a way to avoid the need
> for having traling spaces in the file. They are a bit dangerous as
> most editors are set up to remove them, whch creates annoying changes
> all the time then.

You should configure your editor not to delete trailing spaces
or otherwise make whole-file changes like that. Otherwise
you'll constantly be making changes you didn't intend...

I agree that if we can avoid the trailing spaces that would be
the best thing -- they're a pretty ugly and fragile workaround
for a rST awkwardness.

-- PMM

Re: [PATCH v2 01/55] hw/core: Make do_unaligned_access noreturn

[Alex Bennée]

Richard Henderson  writes:

> While we may have had some thought of allowing system-mode
> to return from this hook, we have no guests that require this.
>
> Signed-off-by: Richard Henderson 
> ---
>  include/hw/core/tcg-cpu-ops.h  | 3 ++-
>  target/alpha/cpu.h | 4 ++--
>  target/arm/internals.h | 3 ++-
>  target/microblaze/cpu.h| 2 +-
>  target/mips/tcg/tcg-internal.h | 4 ++--
>  target/nios2/cpu.h | 4 ++--
>  target/ppc/internal.h  | 4 ++--
>  target/riscv/cpu.h | 2 +-
>  target/s390x/s390x-internal.h  | 4 ++--
>  target/sh4/cpu.h   | 4 ++--
>  target/xtensa/cpu.h| 4 ++--
>  target/hppa/cpu.c  | 7 ---
>  12 files changed, 24 insertions(+), 21 deletions(-)
>
> diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
> index eab27d0c03..ee0795def4 100644
> --- a/include/hw/core/tcg-cpu-ops.h
> +++ b/include/hw/core/tcg-cpu-ops.h
> @@ -72,10 +72,11 @@ struct TCGCPUOps {
>MemTxResult response, uintptr_t retaddr);
>  /**
>   * @do_unaligned_access: Callback for unaligned access handling
> + * The callback must exit via raising an exception.
>   */
>  void (*do_unaligned_access)(CPUState *cpu, vaddr addr,
>  MMUAccessType access_type,
> -int mmu_idx, uintptr_t retaddr);
> +int mmu_idx, uintptr_t retaddr) 
> QEMU_NORETURN;
>  
>  /**
>   * @adjust_watchpoint_address: hack for cpu_check_watchpoint used by ARM
> diff --git a/target/alpha/cpu.h b/target/alpha/cpu.h
> index 82df108967..6eb3fcc63e 100644
> --- a/target/alpha/cpu.h
> +++ b/target/alpha/cpu.h
> @@ -283,8 +283,8 @@ hwaddr alpha_cpu_get_phys_page_debug(CPUState *cpu, vaddr 
> addr);
>  int alpha_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
>  int alpha_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
>  void alpha_cpu_do_unaligned_access(CPUState *cpu, vaddr addr,
> -   MMUAccessType access_type,
> -   int mmu_idx, uintptr_t retaddr);
> +   MMUAccessType access_type, int mmu_idx,
> +   uintptr_t retaddr) QEMU_NORETURN;

These trailing QEMU_NORETURN's seem to be fairly uncommon in the
existing code. Indeed I'd glanced at this code and was about to suggest
one was added. IMHO is scans better when your reading the return type
for a function and you can always do:

  void QEMU_NORETURN
  foo_function(bar args);

if you are worried about over indentation. Anyway:

Reviewed-by: Alex Bennée 

-- 
Alex Bennée

Re: [PATCH] plugins/execlog: removed unintended "s" at the end of log lines.

[Alex Bennée]

Mahmoud Mandour  writes:

> Signed-off-by: Mahmoud Mandour 

Queued to for-6.1/misc-fixes-for-rc2, thanks.

-- 
Alex Bennée

Re: [PATCH for-6.1] hw/arm/boot: Report error if there is no fw_cfg device in the machine

[Peter Maydell]

On Tue, 3 Aug 2021 at 15:29, Leif Lindholm  wrote:
>
> On Mon, Jul 26, 2021 at 17:33:51 +0100, Peter Maydell wrote:
> > If the user provides both a BIOS/firmware image and also a guest
> > kernel filename, arm_setup_firmware_boot() will pass the
> > kernel image to the firmware via the fw_cfg device. However we
> > weren't checking whether there really was a fw_cfg device present,
> > and if there wasn't we would crash.
> >
> > This crash can be provoked with a command line such as
> >  qemu-system-aarch64 -M raspi3 -kernel /dev/null -bios /dev/null -display 
> > none
> >
> > It is currently only possible on the raspi3 machine, because unless
> > the machine sets info->firmware_loaded we won't call
> > arm_setup_firmware_boot(), and the only machines which set that are:
> >  * virt (has a fw-cfg device)
> >  * sbsa-ref (checks itself for kernel_filename && firmware_loaded)
> >  * raspi3 (crashes)
> >
> > But this is an unfortunate beartrap to leave for future machine
> > model implementors, so we should handle this situation in boot.c.
> >
> > Check in arm_setup_firmware_boot() whether the fw-cfg device exists
> > before trying to load files into it, and if it doesn't exist then
> > exit with a hopefully helpful error message.
> >
> > Because we now handle this check in a machine-agnostic way, we
> > can remove the check from sbsa-ref.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/503
> > Signed-off-by: Peter Maydell 
>
> Reviewed-by: Leif Lindholm 
>
> However, the subject line threw me slightly. How about:?
> "Report error if trying to load kernel with no fw_cfg"

Yeah, in retrospect that would have been a better subject.
However, the commit is in master already (dae257394ae5) so
it is what it is :-/

thanks
-- PMM