netdev - Re: [PATCH net-next 2/8] virtio_pci_modern: allow setting configuring extended features

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3d5c65e0-d458-4a56-8c93-c0b5d37420b5@redhat.com>
Date: Wed, 28 May 2025 18:02:43 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jason Wang <jasowang@...hat.com>
Cc: netdev@...r.kernel.org, Willem de Bruijn
 <willemdebruijn.kernel@...il.com>, Andrew Lunn <andrew+netdev@...n.ch>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, "Michael S. Tsirkin" <mst@...hat.com>,
 Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, Eugenio Pérez
 <eperezma@...hat.com>
Subject: Re: [PATCH net-next 2/8] virtio_pci_modern: allow setting configuring
 extended features

On 5/27/25 5:04 AM, Jason Wang wrote:
> On Mon, May 26, 2025 at 6:53 PM Paolo Abeni <pabeni@...hat.com> wrote:
>> On 5/26/25 2:49 AM, Jason Wang wrote:
>>> On Wed, May 21, 2025 at 6:33 PM Paolo Abeni <pabeni@...hat.com> wrote:
>>>>
>>>> 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@...hat.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.

If 128 looks too low (why?) it can be raised to say 256 (why?). But
AFAICS the only visible effect would be slower configuration due to
larger number of unneeded I/O operations.

/P