Message-ID: <3ae72579-7259-49ba-af37-a2eaba719e7e@redhat.com>
Date: Wed, 28 May 2025 17:47:53 +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 1/8] virtio: introduce virtio_features_t

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

Powered by blists - more mailing lists