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

[Jason Wang <jasowang@redhat.com>]

在 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.


>
> 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@kernel.org/
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization