netdev - Re: [PATCH net-next 1/8] virtio: introduce virtio_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: <CACGkMEsHn7q8BvfkaiknQTW9=WONLC_eB9DV0bcqL=oLa62Dxg@mail.gmail.com>
Date: Tue, 27 May 2025 11:51:00 +0800
From: Jason Wang <jasowang@...hat.com>
To: Paolo Abeni <pabeni@...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 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.
>
> /P
>

Thanks