Jason Wang
2021-Jul-01 03:34 UTC
[PATCH linux-next v3 2/6] vdpa: Introduce query of device config layout
? 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
Parav Pandit
2021-Jul-01 07:00 UTC
[PATCH linux-next v3 2/6] vdpa: Introduce query of device config layout
> 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. Instead of doing them as individual netlink attributes, its lumped together in a struct of arbitrary length. :-) 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.> > 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? 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.> > 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. 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.> > 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. You already explained that there isn't one to one mapping of features to config fields for other device types too. Netlink already enables us to avoid non symmetric u64 reserved[16] in get and u16 reserved[16] in set.