Virtualization - Feb 2021 - [PATCH linux-next 1/9] vdpa_sim: Consider read only supported features instead of current

On Wed, Feb 24, 2021 at 08:18:36AM +0200, Parav Pandit
wrote:
> To honor VIRTIO_F_VERSION_1 feature bit, during endianness detection,
> consider the read only supported features bit instead of current
> features bit which can be modified by the driver.
> 
> This enables vdpa_sim_net driver to invoke cpu_to_vdpasim16() early
> enough just after vdpasim device creation in subsequent patch.
> 
> Signed-off-by: Parav Pandit <parav at nvidia.com>
> Reviewed-by: Eli Cohen <elic at nvidia.com>
Well that works for legacy and modern devices but not for transitional
ones. Without transitional device support vendors are reluctant to add
modern features since that will break old guests ...  I suspect we need
to either add a new ioctl enabling modern mode, or abuse SET_FEATURES
and call it from qemu on first config space access.
> ---
>  drivers/vdpa/vdpa_sim/vdpa_sim.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h
b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> index 6d75444f9948..176d641a0939 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> @@ -11,6 +11,7 @@
>  #include <linux/virtio_byteorder.h>
>  #include <linux/vhost_iotlb.h>
>  #include <uapi/linux/virtio_config.h>
> +#include <linux/bits.h>
>  
>  #define VDPASIM_FEATURES	((1ULL << VIRTIO_F_ANY_LAYOUT) | \
>  				 (1ULL << VIRTIO_F_VERSION_1)  | \
> @@ -71,7 +72,8 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr
*attr);
>  static inline bool vdpasim_is_little_endian(struct vdpasim *vdpasim)
>  {
>  	return virtio_legacy_is_little_endian() ||
> -		(vdpasim->features & (1ULL << VIRTIO_F_VERSION_1));
> +		(vdpasim->dev_attr.supported_features &
> +		 BIT_ULL(VIRTIO_F_VERSION_1));
>  }
>  
>  static inline u16 vdpasim16_to_cpu(struct vdpasim *vdpasim, __virtio16
val)
> -- 
> 2.26.2

Hi Michael, Jason,
> From: Michael S. Tsirkin <mst at redhat.com>
> Sent: Wednesday, February 24, 2021 12:40 PM
> 
> On Wed, Feb 24, 2021 at 08:18:36AM +0200, Parav Pandit wrote:
> > To honor VIRTIO_F_VERSION_1 feature bit, during endianness detection,
> > consider the read only supported features bit instead of current
> > features bit which can be modified by the driver.
> >
> > This enables vdpa_sim_net driver to invoke cpu_to_vdpasim16() early
> > enough just after vdpasim device creation in subsequent patch.
> >
> > Signed-off-by: Parav Pandit <parav at nvidia.com>
> > Reviewed-by: Eli Cohen <elic at nvidia.com>
> 
> Well that works for legacy and modern devices but not for transitional
ones.
> Without transitional device support vendors are reluctant to add modern
> features since that will break old guests ...  I suspect we need to either
add a
> new ioctl enabling modern mode, or abuse SET_FEATURES and call it from
> qemu on first config space access.
> 
>From mgmt tool kernel point of view,
(a) config space decoding depends on current feature version bit
(b) feature get/set and config read are not atomic callbacks 

Mgmt. tool kernel code need to read them in single call from the vendor driver.
I need to add mgmt_dev->ops->get_features_config(struct
virtio_features_config*) calllback that returns value of both atomically in
structure like below.

struct virtio_features_config {
	u64 features;
	union {
		struct virtio_net_config net;
		struct virtio_block_config blk;
	} u;
}

What do you think?