RE: [PATCH linux-next v2 04/14] vdpa: Introduce query of device config layout

[Parav Pandit <parav@nvidia.com>]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, April 7, 2021 12:42 PM
> 
> 在 2021/4/7 下午1:10, Parav Pandit 写道:
> >
> >> From: Jason Wang <jasowang@redhat.com>
> >> Sent: Wednesday, April 7, 2021 9:26 AM
> > [..]
> >>>    /**
> >>>     * struct vdpa_config_ops - operations for configuring a vDPA device.
> >>>     * Note: vDPA device drivers are required to implement all of the
> >>> @@
> >>> -164,6 +200,10 @@ struct vdpa_iova_range {
> >>>     *				@buf: buffer used to write from
> >>>     *				@len: the length to write to
> >>>     *				configuration space
> >>> + * @get_ce_config:		Read device specific configuration in
> >>> + *				cpu endianness.
> >>> + *				@vdev: vdpa device
> >>> + *				@config: pointer to config buffer used to
> >> read to
> >>
> >>
> >> So I wonder what's the reason for having this? If it's only the
> >> reason of endian, we can just use get_config.
> >>
> > Didn't follow your suggestion.
> > How does in kernel management tool caller get_config  know how to do
> endianenss conversion?
> 
> 
> LE should be used, so it's the responsibility of the vDPA parent
> (driver) to do the endian conversion.
> 
> 
> > Or you are proposing to send this whole virtio_config structure as binary
> data to user space via netlink?
> > If so, it is not a practice in netlink to return struct.
> 
> 
> I don't get here, it should work as mac address I think.
> 
> 
> >
> > Also making netlink user space to understand __virtio16 doesn't sound
> correct.
> > Often orchestration is not written in C. I cannot think of returning
> virtio_net_config via netlink socket if it is your thought.
> 
> 
> I'm not sure I get here. __virtio16 is part of uapi which is defined
> virtio_types.h so did the virtio_net_config. Duplicating those through
> dedicated netlink attr looks like burden. E.g you need to introduce new
> attrs for each field of the config for every virtio devices and keep it
> up-to-date with the virtio uapis.
> 
> 
Given that only few fields are valid from the config struct, if we pass the whole struct, we additionally need to pass the bitmask to indicate what is valid.
And so I think passing individual fields via the currently defined netlink is better.
Lets continue using individual fields.
> So I think device should maintain a stable features that will is
> returned by get_features(), otherwise a lot of things will be broken.


> I've had some disucssion with Michael, the conclusion is that we should
> only advertise (or mandate) a modern device to be exposed userspace.
> Otherwise it could be a lot of burden. Qemu can still present a
> transtitonal device by doing the endian conversion when necessary in the
> middle. I'm working on the Qemu patches to do that.

Ok. enforcing this as first thing so that netlink config callbacks can assume them LE would be nice.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization