On Thu, May 29, 2025 at 7:07 PM Paolo Abeni [off-list ref] wrote:

On 5/29/25 4:22 AM, Jason Wang wrote:

quoted

On Thu, May 29, 2025 at 12:02 AM Paolo Abeni [off-list ref] wrote:

quoted

On 5/27/25 5:04 AM, Jason Wang wrote:

quoted

On Mon, May 26, 2025 at 6:53 PM Paolo Abeni [off-list ref] wrote:

quoted

On 5/26/25 2:49 AM, Jason Wang wrote:

quoted

On Wed, May 21, 2025 at 6:33 PM Paolo Abeni [off-list ref] wrote:

quoted

The virtio specifications allows for up to 128 bits for the
device features. Soon we are going to use some of the 'extended'
bits features (above 64) for the virtio_net driver.

Extend the virtio pci modern driver to support configuring the full
virtio features range, replacing the unrolled loops reading and
writing the features space with explicit one bounded to the actual
features space size in word.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 drivers/virtio/virtio_pci_modern_dev.c | 39 +++++++++++++++++---------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
index 1d34655f6b658..e3025b6fa8540 100644
--- a/drivers/virtio/virtio_pci_modern_dev.c
+++ b/drivers/virtio/virtio_pci_modern_dev.c

@@ -396,12 +396,16 @@ EXPORT_SYMBOL_GPL(vp_modern_remove);
 virtio_features_t vp_modern_get_features(struct virtio_pci_modern_device *mdev)
 {
        struct virtio_pci_common_cfg __iomem *cfg = mdev->common;
-       virtio_features_t features;
+       virtio_features_t features = 0;
+       int i;

-       vp_iowrite32(0, &cfg->device_feature_select);
-       features = vp_ioread32(&cfg->device_feature);
-       vp_iowrite32(1, &cfg->device_feature_select);
-       features |= ((u64)vp_ioread32(&cfg->device_feature) << 32);
+       for (i = 0; i < VIRTIO_FEATURES_WORDS; i++) {
+               virtio_features_t cur;
+
+               vp_iowrite32(i, &cfg->device_feature_select);
+               cur = vp_ioread32(&cfg->device_feature);
+               features |= cur << (32 * i);
+       }

No matter if we decide to go with 128bit or not. I think at the lower
layer like this, it's time to allow arbitrary length of the features
as the spec supports.

Is that useful if the vhost interface is not going to support it?

I think so, as there are hardware virtio devices that can benefit from this.

Let me look at the question from another perspective. Let's suppose that
the virtio device supports an arbitrary wide features space, and the
uAPI allows passing to/from the kernel an arbitrary high number of features.

How could the kernel stop the above loop? AFAICS the virtio spec does
not define any way to detect the end of the features space. An arbitrary
bound is actually needed.

I think this is a good question ad we have something that could work:

1) current driver has drv->feature_table_size, so the driver knows
it's meaningless to read above the size

and

2) we can extend the spec, e.g add a transport specific field to let
the driver to know the feature size

So I guess we can postpone any additional change here until we have some
spec in place, right?

I think 1) should be sufficient. Considering we agree that
virtio_features_t will use arrays in the future, I'm fine to start
with int128.

Thanks

/P