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

? 2021/6/29 ??5:49, Parav Pandit ??:
> Hi Jason,
>
>> From: Jason Wang <jasowang at redhat.com>
>> Sent: Tuesday, June 29, 2021 9:22 AM
>
>>>>> Pass the whole virtio_net_config and inform via side
channel?
>>>> That could be a method.
>>> I prefer the method to pass individual fields which has the clean
code
>> approach and full flexibility.
>>> Clean code >>> 1. no typecasting based on length
>>> 2. self-describing fields, do not depends on feature bits parsing
3.
>>> proof against structure size increases in fully backward/forward
>>> compatibility without code changes
>>
>> So I think I agree. But I think we'd better to that in the virito
uAPI
>> (include/uapi/linux/virito_xxx.h)
>>
> [..]
>
>> I think maybe we can start from inventing new virtio uAPI and see if it
>> has some contradict with netlink. Or maybe you can give me some
example?
>>
>>
>>> I am unable to convince my self to build side bitmask for config
fields, type
>> casting code in spirit of using existing structure UAPI.
>>> This creates messy code for future.
>>
>> Just a quick thought, how about simply something like:
>>
>> struct virtio_net_config_build {
>>   ??????? __u8 mac[ETH_ALEN];
>>   ??????? __virtio16 max_virtqueue_pairs;
>>   ??? ??? __virtio16 reserved[3];
>> };
> In this structure we need to add bi field flags to indicate which entry is
valid.
> And when structure layout changes, we end up with similar typecast issues,
length checks and more.
> Most of it is inbuild to the netlink.
>
> So I propose,
> (a) we pass config parameters during vdpa device create
> $ vdpa dev add name foo mgmtdev pci/0000:03:00.4 mac 00:11:22:33:44:55 maxq
10
>
> This results in adding two onenew netlink optional attributes as
VDPA_DEV_NET_MAC.
> VDPA_ATTR_DEV_MAX_VQ_SIZE is already dfined for max queues.
> NLA_POLICY_ETH_ADDR takes care to validate length size when passed.
>
>> It looks to we don't need the rest of fields in the
virtio_net_config to
>> build the config since they are all hardware attributes.
> Today it is only mac and max queues. Later on we may need to define rss
hashing as hw/device advances.
> And structure size will change.
> Hence, I propose to have each as individual attribute that doesn?t need to
cast in struct.

Ok, that should work. If Michael are fine with this, I'm also fine.

Just to clarify, if I understand this correctly, with the individual 
attribute, there's no need for the bit like xxx_is_valid?

Thanks

Hi Jason,
> From: Jason Wang <jasowang at redhat.com>
> Sent: Wednesday, June 30, 2021 10:02 AM
> >> It looks to we don't need the rest of fields in the
virtio_net_config
> >> to build the config since they are all hardware attributes.
> > Today it is only mac and max queues. Later on we may need to define
rss
> hashing as hw/device advances.
> > And structure size will change.
> > Hence, I propose to have each as individual attribute that doesn?t
need to
> cast in struct.
> 
> 
> Ok, that should work. If Michael are fine with this, I'm also fine.
> 
> 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;
+	}

? 2021/6/30 ??2:03, Parav Pandit ??:
> Hi Jason,
>
>> From: Jason Wang <jasowang at redhat.com>
>> Sent: Wednesday, June 30, 2021 10:02 AM
>>>> It looks to we don't need the rest of fields in the
virtio_net_config
>>>> to build the config since they are all hardware attributes.
>>> Today it is only mac and max queues. Later on we may need to define
rss
>> hashing as hw/device advances.
>>> And structure size will change.
>>> Hence, I propose to have each as individual attribute that doesn?t
need to
>> cast in struct.
>>
>>
>> Ok, that should work. If Michael are fine with this, I'm also fine.
>>
>> 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 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.

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.

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.

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.

Thanks