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

> From: Jason Wang <jasowang at 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?
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.

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.

And decoding it requires to query features too. Querying features and config are
not atomic so parsed value can be wrong.

Endianness has to be self-contained in fields return via netlink. With that
baseline, lets think further.
> We don't plan to expose a legacy or transition device, so we can simply
do
> the endian conversion in the device.
>
I am bit confused with Michael's suggestion in v1 [1].

VIRTIO_F_VERSION_1 is set today by mlx5 and ifcvf driver.
> Then we can stick to the uapi of virtio_net_config and there's no need
for the
> VDPA_ATTR_DEV_NET_CFG_XXX?
> 
[1] https://lore.kernel.org/virtualization/20210224020336-mutt-send-email-mst at
kernel.org/

? 2021/4/7 ??1:10, Parav Pandit ??:
>
>> From: Jason Wang <jasowang at 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.

>
> And decoding it requires to query features too. Querying features and
config are not atomic so parsed value can be wrong.

So I think device should maintain a stable features that will is 
returned by get_features(), otherwise a lot of things will be broken.

>
> Endianness has to be self-contained in fields return via netlink. With that
baseline, lets think further.

Then mandating LE seem self-contained.

>
>> We don't plan to expose a legacy or transition device, so we can
simply do
>> the endian conversion in the device.
>>
> I am bit confused with Michael's suggestion in v1 [1].
>
> VIRTIO_F_VERSION_1 is set today by mlx5 and ifcvf driver.

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.

Thanks

>
>> Then we can stick to the uapi of virtio_net_config and there's no
need for the
>> VDPA_ATTR_DEV_NET_CFG_XXX?
>>
> [1]
https://lore.kernel.org/virtualization/20210224020336-mutt-send-email-mst at
kernel.org/
>