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: <20250529102623-mutt-send-email-mst@kernel.org>
Date: Thu, 29 May 2025 10:28:22 -0400
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Jason Wang <jasowang@...hat.com>, 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>,
	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 Wed, May 28, 2025 at 06:02:43PM +0200, Paolo Abeni wrote:
> 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.

Well, no. Let me explain.

Only the features that are negotiated matter.

Thus, as long as the driver only knows how to handle the low 128 bits,
it has no reason at all to ever look at other bits.
Not read them, nor write them, nor store them.

Hope that helps.








> 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