All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH for 7.1 v1 0/5] virtio fixes (split from virtio-gpio series)
@ 2022-07-27 15:56 Alex Bennée
  2022-07-27 15:56 ` [PATCH v1 1/5] block/vhost-user-blk-server: don't expose VHOST_USER_F_PROTOCOL_FEATURES Alex Bennée
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Alex Bennée @ 2022-07-27 15:56 UTC (permalink / raw)
  To: qemu-devel; +Cc: mst, Alex Bennée

Hi,

This is just a split out series based on:

   Subject: [PATCH v3 for 7.2 00/21] virtio-gpio and various virtio cleanups
   Date: Tue, 26 Jul 2022 20:21:29 +0100
   Message-Id: <20220726192150.2435175-1-alex.bennee@linaro.org>

with the patches identified by mst:

  Right. Still I think some are fixes we should merge now.
  I am thinking patches 5, 7,8,9 ? 6 if it makes backporting
  much easier. WDYT? If you agree pls separate bugfixes in
  series I can apply. Thanks!

So here they are ;-)

Alex Bennée (5):
  block/vhost-user-blk-server: don't expose
    VHOST_USER_F_PROTOCOL_FEATURES
  hw/virtio: incorporate backend features in features
  hw/virtio: gracefully handle unset vhost_dev vdev
  hw/virtio: handle un-configured shutdown in virtio-pci
  hw/virtio: fix vhost_user_read tracepoint

 block/export/vhost-user-blk-server.c |  3 +--
 hw/virtio/vhost-user.c               | 13 ++++++++++---
 hw/virtio/vhost.c                    | 10 +++++++---
 hw/virtio/virtio-pci.c               |  9 +++++++--
 4 files changed, 25 insertions(+), 10 deletions(-)

-- 
2.30.2



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v1 1/5] block/vhost-user-blk-server: don't expose VHOST_USER_F_PROTOCOL_FEATURES
  2022-07-27 15:56 [PATCH for 7.1 v1 0/5] virtio fixes (split from virtio-gpio series) Alex Bennée
@ 2022-07-27 15:56 ` Alex Bennée
  2022-07-28  8:46   ` Kevin Wolf
  2022-07-27 15:56 ` [PATCH v1 2/5] hw/virtio: incorporate backend features in features Alex Bennée
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 8+ messages in thread
From: Alex Bennée @ 2022-07-27 15:56 UTC (permalink / raw)
  To: qemu-devel
  Cc: mst, Alex Bennée, Coiby Xu, Kevin Wolf, Hanna Reitz,
	open list:Block layer core

This bit is unused in actual VirtIO feature negotiation and should
only appear in the vhost-user messages between master and slave.

[AJB: experiment, this doesn't break the tests but I'm not super
confident of the range of tests]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20220726192150.2435175-6-alex.bennee@linaro.org>
---
 block/export/vhost-user-blk-server.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
index 3409d9e02e..d31436006d 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -125,8 +125,7 @@ static uint64_t vu_blk_get_features(VuDev *dev)
                1ull << VIRTIO_BLK_F_MQ |
                1ull << VIRTIO_F_VERSION_1 |
                1ull << VIRTIO_RING_F_INDIRECT_DESC |
-               1ull << VIRTIO_RING_F_EVENT_IDX |
-               1ull << VHOST_USER_F_PROTOCOL_FEATURES;
+               1ull << VIRTIO_RING_F_EVENT_IDX ;
 
     if (!vexp->handler.writable) {
         features |= 1ull << VIRTIO_BLK_F_RO;
-- 
2.30.2



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH  v1 2/5] hw/virtio: incorporate backend features in features
  2022-07-27 15:56 [PATCH for 7.1 v1 0/5] virtio fixes (split from virtio-gpio series) Alex Bennée
  2022-07-27 15:56 ` [PATCH v1 1/5] block/vhost-user-blk-server: don't expose VHOST_USER_F_PROTOCOL_FEATURES Alex Bennée
@ 2022-07-27 15:56 ` Alex Bennée
  2022-07-27 15:56 ` [PATCH v1 3/5] hw/virtio: gracefully handle unset vhost_dev vdev Alex Bennée
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Alex Bennée @ 2022-07-27 15:56 UTC (permalink / raw)
  To: qemu-devel; +Cc: mst, Alex Bennée

There are some extra bits used over a vhost-user connection which are
hidden from the device itself. We need to set them here to ensure we
enable things like the protocol extensions.

Currently net/vhost-user.c has it's own inscrutable way of persisting
this data but it really should live in the core vhost_user code.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20220726192150.2435175-7-alex.bennee@linaro.org>
---
 hw/virtio/vhost-user.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 75b8df21a4..1936a44e82 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1460,7 +1460,14 @@ static int vhost_user_set_features(struct vhost_dev *dev,
      */
     bool log_enabled = features & (0x1ULL << VHOST_F_LOG_ALL);
 
-    return vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES, features,
+    /*
+     * We need to include any extra backend only feature bits that
+     * might be needed by our device. Currently this includes the
+     * VHOST_USER_F_PROTOCOL_FEATURES bit for enabling protocol
+     * features.
+     */
+    return vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES,
+                              features | dev->backend_features,
                               log_enabled);
 }
 
-- 
2.30.2



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH  v1 3/5] hw/virtio: gracefully handle unset vhost_dev vdev
  2022-07-27 15:56 [PATCH for 7.1 v1 0/5] virtio fixes (split from virtio-gpio series) Alex Bennée
  2022-07-27 15:56 ` [PATCH v1 1/5] block/vhost-user-blk-server: don't expose VHOST_USER_F_PROTOCOL_FEATURES Alex Bennée
  2022-07-27 15:56 ` [PATCH v1 2/5] hw/virtio: incorporate backend features in features Alex Bennée
@ 2022-07-27 15:56 ` Alex Bennée
  2022-07-27 15:56 ` [PATCH v1 4/5] hw/virtio: handle un-configured shutdown in virtio-pci Alex Bennée
  2022-07-27 15:56 ` [PATCH v1 5/5] hw/virtio: fix vhost_user_read tracepoint Alex Bennée
  4 siblings, 0 replies; 8+ messages in thread
From: Alex Bennée @ 2022-07-27 15:56 UTC (permalink / raw)
  To: qemu-devel; +Cc: mst, Alex Bennée

I've noticed asserts firing because we query the status of vdev after
a vhost connection is closed down. Rather than faulting on the NULL
indirect just quietly reply false.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20220726192150.2435175-8-alex.bennee@linaro.org>
---
 hw/virtio/vhost.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 0827d631c0..f758f177bb 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -306,7 +306,7 @@ static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
     dev->log_size = size;
 }
 
-static int vhost_dev_has_iommu(struct vhost_dev *dev)
+static bool vhost_dev_has_iommu(struct vhost_dev *dev)
 {
     VirtIODevice *vdev = dev->vdev;
 
@@ -316,8 +316,12 @@ 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 transactions.
      */
-    return virtio_bus_device_iommu_enabled(vdev) &&
-           virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
+    if (vdev) {
+        return virtio_bus_device_iommu_enabled(vdev) &&
+            virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
+    } else {
+        return false;
+    }
 }
 
 static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
-- 
2.30.2



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v1 4/5] hw/virtio: handle un-configured shutdown in virtio-pci
  2022-07-27 15:56 [PATCH for 7.1 v1 0/5] virtio fixes (split from virtio-gpio series) Alex Bennée
                   ` (2 preceding siblings ...)
  2022-07-27 15:56 ` [PATCH v1 3/5] hw/virtio: gracefully handle unset vhost_dev vdev Alex Bennée
@ 2022-07-27 15:56 ` Alex Bennée
  2022-07-27 15:56 ` [PATCH v1 5/5] hw/virtio: fix vhost_user_read tracepoint Alex Bennée
  4 siblings, 0 replies; 8+ messages in thread
From: Alex Bennée @ 2022-07-27 15:56 UTC (permalink / raw)
  To: qemu-devel; +Cc: mst, Alex Bennée

The assert() protecting against leakage is a little aggressive and
causes needless crashes if a device is shutdown without having been
configured. In this case no descriptors are lost because none have
been assigned.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20220726192150.2435175-9-alex.bennee@linaro.org>
---
 hw/virtio/virtio-pci.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 45327f0b31..5ce61f9b45 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -996,9 +996,14 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d, int nvqs, bool assign)
 
     nvqs = MIN(nvqs, VIRTIO_QUEUE_MAX);
 
-    /* When deassigning, pass a consistent nvqs value
-     * to avoid leaking notifiers.
+    /*
+     * When deassigning, pass a consistent nvqs value to avoid leaking
+     * notifiers. But first check we've actually been configured, exit
+     * early if we haven't.
      */
+    if (!assign && !proxy->nvqs_with_notifiers) {
+        return 0;
+    }
     assert(assign || nvqs == proxy->nvqs_with_notifiers);
 
     proxy->nvqs_with_notifiers = nvqs;
-- 
2.30.2



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH  v1 5/5] hw/virtio: fix vhost_user_read tracepoint
  2022-07-27 15:56 [PATCH for 7.1 v1 0/5] virtio fixes (split from virtio-gpio series) Alex Bennée
                   ` (3 preceding siblings ...)
  2022-07-27 15:56 ` [PATCH v1 4/5] hw/virtio: handle un-configured shutdown in virtio-pci Alex Bennée
@ 2022-07-27 15:56 ` Alex Bennée
  4 siblings, 0 replies; 8+ messages in thread
From: Alex Bennée @ 2022-07-27 15:56 UTC (permalink / raw)
  To: qemu-devel; +Cc: mst, Alex Bennée

As reads happen in the callback we were never seeing them. We only
really care about the header so move the tracepoint to when the header
is complete.

Fixes: 6ca6d8ee9d (hw/virtio: add vhost_user_[read|write] trace points)
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20220726192150.2435175-10-alex.bennee@linaro.org>
---
 hw/virtio/vhost-user.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 1936a44e82..c0b50deaf2 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -295,6 +295,8 @@ static int vhost_user_read_header(struct vhost_dev *dev, VhostUserMsg *msg)
         return -EPROTO;
     }
 
+    trace_vhost_user_read(msg->hdr.request, msg->hdr.flags);
+
     return 0;
 }
 
@@ -544,8 +546,6 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
         }
     }
 
-    trace_vhost_user_read(msg.hdr.request, msg.hdr.flags);
-
     return 0;
 }
 
-- 
2.30.2



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH  v1 1/5] block/vhost-user-blk-server: don't expose VHOST_USER_F_PROTOCOL_FEATURES
  2022-07-27 15:56 ` [PATCH v1 1/5] block/vhost-user-blk-server: don't expose VHOST_USER_F_PROTOCOL_FEATURES Alex Bennée
@ 2022-07-28  8:46   ` Kevin Wolf
  2022-07-28 10:27     ` Alex Bennée
  0 siblings, 1 reply; 8+ messages in thread
From: Kevin Wolf @ 2022-07-28  8:46 UTC (permalink / raw)
  To: Alex Bennée
  Cc: qemu-devel, mst, Coiby Xu, Hanna Reitz, open list:Block layer core

Am 27.07.2022 um 17:56 hat Alex Bennée geschrieben:
> This bit is unused in actual VirtIO feature negotiation and should
> only appear in the vhost-user messages between master and slave.
> 
> [AJB: experiment, this doesn't break the tests but I'm not super
> confident of the range of tests]
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20220726192150.2435175-6-alex.bennee@linaro.org>
> ---
>  block/export/vhost-user-blk-server.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
> index 3409d9e02e..d31436006d 100644
> --- a/block/export/vhost-user-blk-server.c
> +++ b/block/export/vhost-user-blk-server.c
> @@ -125,8 +125,7 @@ static uint64_t vu_blk_get_features(VuDev *dev)
>                 1ull << VIRTIO_BLK_F_MQ |
>                 1ull << VIRTIO_F_VERSION_1 |
>                 1ull << VIRTIO_RING_F_INDIRECT_DESC |
> -               1ull << VIRTIO_RING_F_EVENT_IDX |
> -               1ull << VHOST_USER_F_PROTOCOL_FEATURES;
> +               1ull << VIRTIO_RING_F_EVENT_IDX ;

I didn't see this series yet when I replied to the other series this is
split off from, but of course, my comments are still relevant for this
one.

I asked for a changed commit message (the "experiment" part should
probably go away if we're merging it; instead, it should explain that
in vu_get_features_exec(), libvhost-user adds the vhost-user protocol
level VHOST_USER_F_PROTOCOL_FEATURES flag anyway and the device is the
wrong layer to add it, but the behaviour doesn't change with this patch)
and noted the extra space before the semicolon.

Kevin



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH  v1 1/5] block/vhost-user-blk-server: don't expose VHOST_USER_F_PROTOCOL_FEATURES
  2022-07-28  8:46   ` Kevin Wolf
@ 2022-07-28 10:27     ` Alex Bennée
  0 siblings, 0 replies; 8+ messages in thread
From: Alex Bennée @ 2022-07-28 10:27 UTC (permalink / raw)
  To: Kevin Wolf
  Cc: qemu-devel, mst, Coiby Xu, Hanna Reitz, open list:Block layer core


Kevin Wolf <kwolf@redhat.com> writes:

> Am 27.07.2022 um 17:56 hat Alex Bennée geschrieben:
>> This bit is unused in actual VirtIO feature negotiation and should
>> only appear in the vhost-user messages between master and slave.
>> 
>> [AJB: experiment, this doesn't break the tests but I'm not super
>> confident of the range of tests]
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Message-Id: <20220726192150.2435175-6-alex.bennee@linaro.org>
>> ---
>>  block/export/vhost-user-blk-server.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>> 
>> diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
>> index 3409d9e02e..d31436006d 100644
>> --- a/block/export/vhost-user-blk-server.c
>> +++ b/block/export/vhost-user-blk-server.c
>> @@ -125,8 +125,7 @@ static uint64_t vu_blk_get_features(VuDev *dev)
>>                 1ull << VIRTIO_BLK_F_MQ |
>>                 1ull << VIRTIO_F_VERSION_1 |
>>                 1ull << VIRTIO_RING_F_INDIRECT_DESC |
>> -               1ull << VIRTIO_RING_F_EVENT_IDX |
>> -               1ull << VHOST_USER_F_PROTOCOL_FEATURES;
>> +               1ull << VIRTIO_RING_F_EVENT_IDX ;
>
> I didn't see this series yet when I replied to the other series this is
> split off from, but of course, my comments are still relevant for this
> one.
>
> I asked for a changed commit message (the "experiment" part should
> probably go away if we're merging it; instead, it should explain that
> in vu_get_features_exec(), libvhost-user adds the vhost-user protocol
> level VHOST_USER_F_PROTOCOL_FEATURES flag anyway and the device is the
> wrong layer to add it, but the behaviour doesn't change with this patch)
> and noted the extra space before the semicolon.

OK - mst do you want me to respin or can you tweak the commit?

>
> Kevin


-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-07-28 10:32 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-27 15:56 [PATCH for 7.1 v1 0/5] virtio fixes (split from virtio-gpio series) Alex Bennée
2022-07-27 15:56 ` [PATCH v1 1/5] block/vhost-user-blk-server: don't expose VHOST_USER_F_PROTOCOL_FEATURES Alex Bennée
2022-07-28  8:46   ` Kevin Wolf
2022-07-28 10:27     ` Alex Bennée
2022-07-27 15:56 ` [PATCH v1 2/5] hw/virtio: incorporate backend features in features Alex Bennée
2022-07-27 15:56 ` [PATCH v1 3/5] hw/virtio: gracefully handle unset vhost_dev vdev Alex Bennée
2022-07-27 15:56 ` [PATCH v1 4/5] hw/virtio: handle un-configured shutdown in virtio-pci Alex Bennée
2022-07-27 15:56 ` [PATCH v1 5/5] hw/virtio: fix vhost_user_read tracepoint Alex Bennée

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.