Re: [PATCH net-next 1/8] virtio: introduce virtio_features_t

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, May 28, 2025 at 05:47:53PM +0200, Paolo Abeni wrote:
> On 5/27/25 5:51 AM, Jason Wang wrote:
> > On Mon, May 26, 2025 at 3:20 PM Paolo Abeni <pabeni@redhat.com> wrote:
> >> On 5/26/25 2:43 AM, Jason Wang wrote:
> >>> On Wed, May 21, 2025 at 6:33 PM Paolo Abeni <pabeni@redhat.com> wrote:
> >>>> diff --git a/include/linux/virtio_features.h b/include/linux/virtio_features.h
> >>>> new file mode 100644
> >>>> index 0000000000000..2f742eeb45a29
> >>>> --- /dev/null
> >>>> +++ b/include/linux/virtio_features.h
> >>>> @@ -0,0 +1,23 @@
> >>>> +/* SPDX-License-Identifier: GPL-2.0 */
> >>>> +#ifndef _LINUX_VIRTIO_FEATURES_H
> >>>> +#define _LINUX_VIRTIO_FEATURES_H
> >>>> +
> >>>> +#include <linux/bits.h>
> >>>> +
> >>>> +#if IS_ENABLED(CONFIG_ARCH_SUPPORTS_INT128)
> >>>> +#define VIRTIO_HAS_EXTENDED_FEATURES
> >>>> +#define VIRTIO_FEATURES_MAX    128
> >>>> +#define VIRTIO_FEATURES_WORDS  4
> >>>> +#define VIRTIO_BIT(b)          _BIT128(b)
> >>>> +
> >>>> +typedef __uint128_t            virtio_features_t;
> >>>
> >>> Consider:
> >>>
> >>> 1) need the trick for arch that doesn't support 128bit
> >>> 2) some transport (e.g PCI) allows much more than just 128 bit features
> >>>
> >>>  I wonder if it's better to just use arrays here.
> >>
> >> I considered that, it has been discussed both on the virtio ML and
> >> privatelly, and I tried a resonable attempt with such implementation.
> >>
> >> The diffstat would be horrible, touching a lot of the virtio/vhost code.
> > 
> > Let's start with the driver. For example, driver had already used
> > array for features:
> > 
> >         const unsigned int *feature_table;
> >         unsigned int feature_table_size;
> > 
> > For vhost, we need new ioctls anyhow:
> > 
> > /* Features bitmask for forward compatibility.  Transport bits are used for
> >  * vhost specific features. */
> > #define VHOST_GET_FEATURES      _IOR(VHOST_VIRTIO, 0x00, __u64)
> > #define VHOST_SET_FEATURES      _IOW(VHOST_VIRTIO, 0x00, __u64)
> > 
> > As we can't change uAPI for existing ioctls.
> > 
> >> Such approach will block any progress for a long time (more likely
> >> forever, since I will not have the capacity to complete it).
> >>
> > 
> > Well, could we at least start from using u64[2] for virtio_features_t?
> > 
> >> Also the benefit are AFAICS marginal, as 32 bits platform with huge
> >> virtualization deployments on top of it (that could benefit from GSO
> >> over UDP tunnel) are IMHO unlikely,
> > 
> > I think it's better to not have those architecture specific assumptions since:
> > 
> > 1) need to prove the assumption is correct or
> > 2) we may also create blockers for 64 bit archs that don't support
> > ARCH_SUPPORTS_INT128.
> > 
> >> and transport features space
> >> exhaustion is AFAIK far from being reached (also thanks to reserved
> >> features availables).
> > 
> > I wouldn't be worried if a straightforward switch to int128 worked,
> > but it looks like that is not the case:
> > 
> > 1) ARCH_SUPPORTS_INT128 dependency
> > 2) new uAPI
> > 3) we might want a new virtio config ops as well as most of transport
> > can only return 64 bit now
> >>
> >> TL;DR: if you consider a generic implementation for an arbitrary wide
> >> features space blocking, please LMK, because any other consideration
> >> would be likely irrelevant otherwise.
> 
> I read your comments above as the only way forward is abandoning the
> uint128_t usage. Could you please confirm that?
> 
> Side note: new uAPI will be required by every implementation of
> feature-space extension, as the current ones are 64-bits bound.
> 
> Thanks,
> 
> Paolo


Jason, I think what Paolo's doing is a step in the right direction, we
can do this, then gradually transfer all drivers, devices and transports
to use virtio_features_t, then make virtio_features_t an array if we want.

If instead you jump to an array straight away, it's a huge change that
can not be split up cleanly.


-- 
MST