Virtualization - Jul 2021 - [PATCH linux-next v3 2/6] vdpa: Introduce query of device config layout

? 2021/7/1 ??3:00, Parav Pandit ??:
>> From: Jason Wang <jasowang at redhat.com>
>> Sent: Thursday, July 1, 2021 9:04 AM
>
>>>> Just to clarify, if I understand this correctly, with the
individual
>>>> attribute, there's no need for the bit like xxx_is_valid?
>>> xxx_is_valid is not present in the get calls.
>>> It is also not present in UAPI set calls.
>>> It is not a UAPI.
>>> It is an internal between vdpa.c and vendor driver to tell which
fields to use
>> as there are optional.
>>> If we want to get rid of those valid flags below code will move to
vendor
>> driver where we pass nl_attr, during device add callback.
>>>
>>> +	if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
>>> +		macaddr >>
nla_data(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]);
>>> +		memcpy(config.net.mac, macaddr, sizeof(config.net.mac));
>>> +		config.net_mask.mac_valid = true;
>>> +	}
>>> +	if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]) {
>>> +		config.net.mtu >>> +
>> 	nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]);
>>> +		config.net_mask.mtu_valid = true;
>>> +	}
>>
>> Have a hard thought on this. I still think re-invent (duplicate) the
virtio-net
>> config filed is not a good choice (e.g for block we need to duplicate
more
>> than 20 attributes).
> We are re-inventing by defining a new structure below.

Actually it depends on what attributes is required for building the config.

We can simply reuse the existing virtio_net_config, if most of the 
fields are required.

struct virtio_net_config_set {
 ??? ??? __virtio64 features;
 ??? ??? union {
 ??? ??? ??? struct virtio_net_config;
 ??? ??? ??? __virtio64 reserved[64];
 ??? ??? }
};

If only few of the is required, we can just pick them and use another 
structure.

Actually, I think just pass the whole config with the device_features 
during device creation is a good choice that can simplify a lot of things.

We can define what is needed and ignore the others in the virtio spec. 
Then there's no need to worry about any other things. vDPA core can just 
do santiy test like checking size vs features.

> Instead of doing them as individual netlink attributes, its lumped together
in a struct of arbitrary length. :-)

I think not? We want to have a fixed length of the structure which never 
grow.

So the different is:

1) using netlink dedicated fields

if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR])

2) using netlink as transport

if (features & VIRTIO_NET_F_MAC)

>
> I notice several fields of the vduse device is setup via ioctl, which I
think should be setup via this vdpa device add interface.
>
> Also we can always wrap above nl_attr code in a helper API so that drivers
to not hand-code it.

Then it would be still more like 2) above (wrap netlink back to 
something like virtio_net_config)?

>
>> We may meet similar issue when provision VF/SF instance at the hardware
>> level. So I think we may need something in the virtio spec in the near
future.
> Do you mean in a virtio vf and virtio sf?

Yes.

> If so, probably yes.
> Given that we have the ability to transport individual fields, we don't
need to attach the U->K UAPI to a undefined and evolving structure.

I don't object but it needs to be done in virtio uAPI instead of 
netlink, since it's the device ABI.

>
>> So assuming we don't want a single attributes to be modified and we
want to
>> let user to specify all the attributes at one time during creation.
>>
>> Maybe we can tweak virtio_net_config_set a little bit:
>>
>> struct virtio_net_config_set {
>>   ??? ??? __virtio64 features;
>>   ??????? __u8 mac[ETH_ALEN];
>>   ??????? __virtio16 max_virtqueue_pairs;
>>   ??? ??? __virtio16 mtu;
>>   ??? ??? __virtio16 reserved[62];
>> }
>>
>> So we have:
>>
>> - both features and config fields, we're self contained
>> - reserved fields which should be sufficient for the next 10 years, so
we don't
>> need to care about the growing.
> This is the reverse of netlink which offers to not reserve any arbitrary
size structure.

It's not arbitrary but with fixed length.

>   Though I agree that it may not grow.
>
>> Or actually it also allows per field modification.
>>
>> E.g if we don't specify VIRTIO_NET_F_MAC, it means mac field is
invalid.
>> So did for qps and mtu.
>>
>> The advantage is that we can standardize this in the virtio spec which
could
>> be used for SF/VF provisioning.
> Virtio spec can be still standardized about which fields of config space
should be setup.
> To do so, we don't need to lump them in one structure.

Yes, agree.

>
>> For get, we probably need more work:
>>
>> struct virtio_net_config_get {
>>   ??? ??? __virtio64 features;
>>   ??? ??? union {
>>   ??? ??? ??? ??? struct virtio_net_config;
>>   ??? ??? ??? ??? __virtio64 reserved[16];
>>   ??? ??? }
>> }
>>
>> Or just follow how it is work today, simply pass the config plus the
>> device_features.
> If we go with individual attribute get and add both sorted out neatly,
expandable.

It may only work for netlink (with some duplication with the existing 
virtio uAPI). If we can solve it at general virtio layer, it would be 
better. Otherwise we need to invent them again in the virtio spec.

E.g virito is expected to support something similar to SF, it requires 
the SF to be created/provisioned via the admin virtqueue in the PF.

In this case, we still need to define what is required it create a 
virtio "SF". Netlink can't be used in this context.

I think even for the current mlx5e vDPA it would be better, otherwise we 
may have:

vDPA tool -> [netlink specific vDPA attributes(1)] -> vDPA core ->
[vDPA
core specific VDPA attributes(2)] -> mlx5e_vDPA -> [mlx5e specific vDPA 
attributes(3)] -> mlx5e_core

We need to use a single and unified virtio structure in all the (1), (2) 
and (3).

>
> You already explained that there isn't one to one mapping of features
to config fields for other device types too.

Yes, but features + config is self contained. That is to say, it's 
sufficient to explain a specific filed if we had device features.

Thanks

> Netlink already enables us to avoid non symmetric u64 reserved[16] in get
and u16 reserved[16] in set.

> From: Jason Wang <jasowang at redhat.com>
> Sent: Thursday, July 1, 2021 1:13 PM
> 
> 
> Actually it depends on what attributes is required for building the config.
> 
> We can simply reuse the existing virtio_net_config, if most of the fields
are
> required.
> 
> struct virtio_net_config_set {
>  ??? ??? __virtio64 features;
>  ??? ??? union {
>  ??? ??? ??? struct virtio_net_config;
>  ??? ??? ??? __virtio64 reserved[64];
>  ??? ??? }
> };
> 
> If only few of the is required, we can just pick them and use another
> structure.
The point is we define structure based on current fields. Tomorrow a new RSS or
rx scaling scheme appears, and structure size might need change.
And it demands us to go back to length based typecasting code.
and to avoid some length check we pick some arbitrary size reserved words.
And I do not know what network research group will come up for new rss algorithm
and needed plumbing.
> 
> Actually, I think just pass the whole config with the device_features
during
> device creation is a good choice that can simplify a lot of things.
Yes. I totally agree to this.
> 
> We can define what is needed and ignore the others in the virtio spec.
> Then there's no need to worry about any other things. vDPA core can
just do
> santiy test like checking size vs features.
Yes, we are trying to have code that avoids such sanity checks based on
structure size, length etc fields. :-)
> 
> 
> > Instead of doing them as individual netlink attributes, its lumped
together
> in a struct of arbitrary length. :-)
> 
> 
> I think not? We want to have a fixed length of the structure which never
> grow.
> 
I am not sure defining that future now is right choice, at least for me.
> So the different is:
> 
> 1) using netlink dedicated fields
> 
> if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR])
> 
> 2) using netlink as transport
> 
> if (features & VIRTIO_NET_F_MAC)
> 
> 
> >
> > I notice several fields of the vduse device is setup via ioctl, which
I think
> should be setup via this vdpa device add interface.
> >
> > Also we can always wrap above nl_attr code in a helper API so that
drivers
> to not hand-code it.
> 
> 
> Then it would be still more like 2) above (wrap netlink back to
> something like virtio_net_config)?
> 
> 
> >
> >> We may meet similar issue when provision VF/SF instance at the
> hardware
> >> level. So I think we may need something in the virtio spec in the
near
> future.
Given the device config is not spelled out in the virtio spec, may be we can
wait for it to define virtio management interface.
> 
> I don't object but it needs to be done in virtio uAPI instead of
> netlink, since it's the device ABI.
Device config can surely be part of the virtio uAPI.
We need not have put that in UAPI.
More below.
> > This is the reverse of netlink which offers to not reserve any
arbitrary size
> structure.
> 
> 
> It's not arbitrary but with fixed length.
Its fixed, but decided arbitrarily large in anticipation that we likely need to
grow.
And sometimes that fall short when next research comes up with more creative
thoughts.
> 
> It may only work for netlink (with some duplication with the existing
> virtio uAPI). If we can solve it at general virtio layer, it would be
> better. Otherwise we need to invent them again in the virtio spec.
> 
Virtio spec will likely define what should be config fields to program and its
layout.
Kernel can always fill up the format that virtio spec demands.
> I think even for the current mlx5e vDPA it would be better, otherwise we
> may have:
> 
> vDPA tool -> [netlink specific vDPA attributes(1)] -> vDPA core ->
[vDPA
> core specific VDPA attributes(2)] -> mlx5e_vDPA -> [mlx5e specific
vDPA
> attributes(3)] -> mlx5e_core
> 
> We need to use a single and unified virtio structure in all the (1), (2)
> and (3).
This is where I differ.
Its only vdpa tool -> vdpa core -> vendor_driver

Vdpa tool -> vdpa core = netlink attribute
Vdpa core -> vendor driver = struct_foo. (internal inside the linux kernel)

If tomorrow virtio spec defines struct_foo to be something else, kernel can
always upgrade to struct_bar without upgrading UAPI netlink attributes.
Netlink attributes addition will be needed only when struct_foo has newer
fields.
This will be still forward/backward compatible.

An exact example of this is drivers/net/vxlan.c
vxlan_nl2conf().
A vxlan device needs VNI, src ip, dst ip, tos, and more.
Instead of putting all in single structure vxlan_config as UAPI, those optional
fields are netlink attributes.
And vxlan driver internally fills up the config structure.

I am very much convinced with the above vxlan approach that enables all
functionality needed without typecasting code and without defining arbitrary
length structs.