[PATCH RFC v3 06/13] virtio-pci: implement support for extended features

Paolo Abeni posted 13 patches 3 months, 4 weeks ago
Maintainers: Dmitry Fleytman <dmitry.fleytman@gmail.com>, Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>, Jason Wang <jasowang@redhat.com>, Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>, "Michael S. Tsirkin" <mst@redhat.com>, Stefano Garzarella <sgarzare@redhat.com>, Cornelia Huck <cohuck@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Luigi Rizzo <rizzo@iet.unipi.it>, Giuseppe Lettieri <g.lettieri@iet.unipi.it>, Vincenzo Maffione <v.maffione@gmail.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>
There is a newer version of this series
[PATCH RFC v3 06/13] virtio-pci: implement support for extended features
Posted by Paolo Abeni 3 months, 4 weeks ago
Extend the features configuration space to 128 bits, and allow the
common read/write operation to access all of it.

On migration, save the 128 bit version of the features only if the
upper bits are non zero. Relay reset to clear all the feature
space before load.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v2 -> v3:
  - drop the pre_load/post_load trickery and relay on reset zeroing
    the features instead.
  - avoid union usage, just increase guest_features size and use
    SUB_ARRAY.
  - drop unneeded '!!'
  - _array -> _ex

v1 -> v2:
  - use separate VMStateDescription and pre/post load to avoid breaking
    migration
  - clear proxy features on device reset
---
 hw/virtio/virtio-pci.c         | 54 +++++++++++++++++++++++++++++-----
 include/hw/virtio/virtio-pci.h |  2 +-
 2 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 767216d795..6800c80eed 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -109,6 +109,29 @@ static const VMStateDescription vmstate_virtio_pci_modern_queue_state = {
     }
 };
 
+static bool virtio_pci_modern_state_features128_needed(void *opaque)
+{
+    VirtIOPCIProxy *proxy = opaque;
+    uint32_t features = 0;
+    int i;
+
+    for (i = 2; i < ARRAY_SIZE(proxy->guest_features); ++i) {
+        features |= proxy->guest_features[i];
+    }
+    return features;
+}
+
+static const VMStateDescription vmstate_virtio_pci_modern_state_features128 = {
+    .name = "virtio_pci/modern_state/features128",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = &virtio_pci_modern_state_features128_needed,
+    .fields = (const VMStateField[]) {
+        VMSTATE_UINT32_SUB_ARRAY(guest_features, VirtIOPCIProxy, 2, 2),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static bool virtio_pci_modern_state_needed(void *opaque)
 {
     VirtIOPCIProxy *proxy = opaque;
@@ -124,11 +147,15 @@ static const VMStateDescription vmstate_virtio_pci_modern_state_sub = {
     .fields = (const VMStateField[]) {
         VMSTATE_UINT32(dfselect, VirtIOPCIProxy),
         VMSTATE_UINT32(gfselect, VirtIOPCIProxy),
-        VMSTATE_UINT32_ARRAY(guest_features, VirtIOPCIProxy, 2),
+        VMSTATE_UINT32_SUB_ARRAY(guest_features, VirtIOPCIProxy, 0, 2),
         VMSTATE_STRUCT_ARRAY(vqs, VirtIOPCIProxy, VIRTIO_QUEUE_MAX, 0,
                              vmstate_virtio_pci_modern_queue_state,
                              VirtIOPCIQueue),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription * const []) {
+        &vmstate_virtio_pci_modern_state_features128,
+        NULL
     }
 };
 
@@ -1494,11 +1521,14 @@ static uint64_t virtio_pci_common_read(void *opaque, hwaddr addr,
         val = proxy->dfselect;
         break;
     case VIRTIO_PCI_COMMON_DF:
-        if (proxy->dfselect <= 1) {
+        if (proxy->dfselect < VIRTIO_FEATURES_WORDS) {
             VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
 
-            val = (vdev->host_features & ~vdc->legacy_features) >>
-                (32 * proxy->dfselect);
+            val = vdev->host_features_ex[proxy->dfselect >> 1] >>
+                  (32 * (proxy->dfselect & 1));
+            if (proxy->dfselect <= 1) {
+                val &= (~vdc->legacy_features) >> (32 * proxy->dfselect);
+            }
         }
         break;
     case VIRTIO_PCI_COMMON_GFSELECT:
@@ -1589,10 +1619,17 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
         break;
     case VIRTIO_PCI_COMMON_GF:
         if (proxy->gfselect < ARRAY_SIZE(proxy->guest_features)) {
+            uint64_t features[VIRTIO_FEATURES_DWORDS];
+            int i;
+
             proxy->guest_features[proxy->gfselect] = val;
-            virtio_set_features(vdev,
-                                (((uint64_t)proxy->guest_features[1]) << 32) |
-                                proxy->guest_features[0]);
+            virtio_features_clear(features);
+            for (i = 0; i < ARRAY_SIZE(proxy->guest_features); ++i) {
+                uint64_t cur = proxy->guest_features[i];
+
+                features[i >> 1] |= cur << ((i & 1) * 32);
+            }
+            virtio_set_features_ex(vdev, features);
         }
         break;
     case VIRTIO_PCI_COMMON_MSIX:
@@ -2311,6 +2348,9 @@ static void virtio_pci_reset(DeviceState *qdev)
     virtio_bus_reset(bus);
     msix_unuse_all_vectors(&proxy->pci_dev);
 
+    QEMU_BUILD_BUG_ON(ARRAY_SIZE(proxy->guest_features) != 4);
+    memset(proxy->guest_features, 0, sizeof(proxy->guest_features));
+
     for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
         proxy->vqs[i].enabled = 0;
         proxy->vqs[i].reset = 0;
diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
index eab5394898..eae0fcdec3 100644
--- a/include/hw/virtio/virtio-pci.h
+++ b/include/hw/virtio/virtio-pci.h
@@ -158,7 +158,7 @@ struct VirtIOPCIProxy {
     uint32_t nvectors;
     uint32_t dfselect;
     uint32_t gfselect;
-    uint32_t guest_features[2];
+    uint32_t guest_features[4];
     VirtIOPCIQueue vqs[VIRTIO_QUEUE_MAX];
 
     VirtIOIRQFD *vector_irqfd;
-- 
2.50.0
Re: [PATCH RFC v3 06/13] virtio-pci: implement support for extended features
Posted by Jason Wang 3 months, 3 weeks ago
On Fri, Jul 18, 2025 at 4:53 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> Extend the features configuration space to 128 bits, and allow the
> common read/write operation to access all of it.
>
> On migration, save the 128 bit version of the features only if the
> upper bits are non zero. Relay reset to clear all the feature
> space before load.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

This is a guest noticeable behaviour change. I wonder if we need a
command line option to enable and disable this feature for migration
compatibility.

Thanks
Re: [PATCH RFC v3 06/13] virtio-pci: implement support for extended features
Posted by Paolo Abeni 3 months, 3 weeks ago
On 7/22/25 5:28 AM, Jason Wang wrote:
> On Fri, Jul 18, 2025 at 4:53 PM Paolo Abeni <pabeni@redhat.com> wrote:
>>
>> Extend the features configuration space to 128 bits, and allow the
>> common read/write operation to access all of it.
>>
>> On migration, save the 128 bit version of the features only if the
>> upper bits are non zero. Relay reset to clear all the feature
>> space before load.
>>
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> 
> This is a guest noticeable behaviour change. I wonder if we need a
> command line option to enable and disable this feature for migration
> compatibility.

This point is not clear to me, could you please elaborate a bit more? do
you mean we need i.e. a DEFINE_PROP_BOOL() or the like to enable the 128
bit space usage?

Thanks,

Paolo


Re: [PATCH RFC v3 06/13] virtio-pci: implement support for extended features
Posted by Jason Wang 3 months, 3 weeks ago
On Tue, Jul 22, 2025 at 3:37 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On 7/22/25 5:28 AM, Jason Wang wrote:
> > On Fri, Jul 18, 2025 at 4:53 PM Paolo Abeni <pabeni@redhat.com> wrote:
> >>
> >> Extend the features configuration space to 128 bits, and allow the
> >> common read/write operation to access all of it.
> >>
> >> On migration, save the 128 bit version of the features only if the
> >> upper bits are non zero. Relay reset to clear all the feature
> >> space before load.
> >>
> >> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> >
> > This is a guest noticeable behaviour change. I wonder if we need a
> > command line option to enable and disable this feature for migration
> > compatibility.
>
> This point is not clear to me, could you please elaborate a bit more? do
> you mean we need i.e. a DEFINE_PROP_BOOL() or the like to enable the 128
> bit space usage?

Yes, or maybe have a way to let the device to enable it automatically
E.g when UDP GSO is enabled for virtio-net.

Thanks

>
> Thanks,
>
> Paolo
>
Re: [PATCH RFC v3 06/13] virtio-pci: implement support for extended features
Posted by Paolo Abeni 3 months, 3 weeks ago
On 7/23/25 7:47 AM, Jason Wang wrote:
> On Tue, Jul 22, 2025 at 3:37 PM Paolo Abeni <pabeni@redhat.com> wrote:
>>
>> On 7/22/25 5:28 AM, Jason Wang wrote:
>>> On Fri, Jul 18, 2025 at 4:53 PM Paolo Abeni <pabeni@redhat.com> wrote:
>>>>
>>>> Extend the features configuration space to 128 bits, and allow the
>>>> common read/write operation to access all of it.
>>>>
>>>> On migration, save the 128 bit version of the features only if the
>>>> upper bits are non zero. Relay reset to clear all the feature
>>>> space before load.
>>>>
>>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>>
>>> This is a guest noticeable behaviour change. I wonder if we need a
>>> command line option to enable and disable this feature for migration
>>> compatibility.
>>
>> This point is not clear to me, could you please elaborate a bit more? do
>> you mean we need i.e. a DEFINE_PROP_BOOL() or the like to enable the 128
>> bit space usage?
> 
> Yes, or maybe have a way to let the device to enable it automatically
> E.g when UDP GSO is enabled for virtio-net.

I think we can safely enable the extended space access (read/write) if
virtio_features_use_ex(vdev->host_features_ex). That should cover also
any eventual future addition of more extended features - even of
different devices.

/P