On Fri, Oct 10, 2025 at 6:44 AM Si-Wei Liu <[email protected]> wrote: > > > > On 10/2/2025 12:27 AM, Eugenio Perez Martin wrote: > > On Thu, Oct 2, 2025 at 1:27 AM Si-Wei Liu <[email protected]> wrote: > >> Hi Eugenio, > >> > >> On 10/1/2025 6:26 AM, Eugenio Perez Martin wrote: > >>> On Tue, Feb 7, 2023 at 12:15 AM Si-Wei Liu <[email protected]> wrote: > >>>> Today when device features are explicitly provisioned, the features > >>>> user supplied may contain device class specific features that are > >>>> not supported by the parent management device. On the other hand, > >>>> when parent management device supports more than one class, the > >>>> device features to provision may be ambiguous if none of the class > >>>> specific attributes is provided at the same time. Validate these > >>>> cases and prompt appropriate user errors accordingly. > >>>> > >>>> Signed-off-by: Si-Wei Liu <[email protected]> > >>>> --- > >>>> drivers/vdpa/vdpa.c | 59 > >>>> +++++++++++++++++++++++++++++++++++++++++++++-------- > >>>> 1 file changed, 50 insertions(+), 9 deletions(-) > >>>> > >>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c > >>>> index 1eba978..8da5120 100644 > >>>> --- a/drivers/vdpa/vdpa.c > >>>> +++ b/drivers/vdpa/vdpa.c > >>>> @@ -460,12 +460,28 @@ static int vdpa_nl_mgmtdev_handle_fill(struct > >>>> sk_buff *msg, const struct vdpa_mg > >>>> return 0; > >>>> } > >>>> > >>>> +static u64 vdpa_mgmtdev_get_classes(const struct vdpa_mgmt_dev *mdev, > >>>> + unsigned int *nclasses) > >>>> +{ > >>>> + u64 supported_classes = 0; > >>>> + unsigned int n = 0; > >>>> + > >>>> + for (int i = 0; mdev->id_table[i].device; i++) { > >>>> + if (mdev->id_table[i].device > 63) > >>>> + continue; > >>>> + supported_classes |= BIT_ULL(mdev->id_table[i].device); > >>>> + n++; > >>>> + } > >>>> + if (nclasses) > >>>> + *nclasses = n; > >>>> + > >>>> + return supported_classes; > >>>> +} > >>>> + > >>>> static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct > >>>> sk_buff *msg, > >>>> u32 portid, u32 seq, int flags) > >>>> { > >>>> - u64 supported_classes = 0; > >>>> void *hdr; > >>>> - int i = 0; > >>>> int err; > >>>> > >>>> hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags, > >>>> VDPA_CMD_MGMTDEV_NEW); > >>>> @@ -475,14 +491,9 @@ static int vdpa_mgmtdev_fill(const struct > >>>> vdpa_mgmt_dev *mdev, struct sk_buff *m > >>>> if (err) > >>>> goto msg_err; > >>>> > >>>> - while (mdev->id_table[i].device) { > >>>> - if (mdev->id_table[i].device <= 63) > >>>> - supported_classes |= > >>>> BIT_ULL(mdev->id_table[i].device); > >>>> - i++; > >>>> - } > >>>> - > >>>> if (nla_put_u64_64bit(msg, VDPA_ATTR_MGMTDEV_SUPPORTED_CLASSES, > >>>> - supported_classes, VDPA_ATTR_UNSPEC)) { > >>>> + vdpa_mgmtdev_get_classes(mdev, NULL), > >>>> + VDPA_ATTR_UNSPEC)) { > >>>> err = -EMSGSIZE; > >>>> goto msg_err; > >>>> } > >>>> @@ -566,13 +577,25 @@ static int vdpa_nl_cmd_mgmtdev_get_doit(struct > >>>> sk_buff *skb, struct genl_info *i > >>>> BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU) > >>>> | \ > >>>> > >>>> BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) > >>>> > >>>> +/* > >>>> + * Bitmask for all per-device features: feature bits > >>>> VIRTIO_TRANSPORT_F_START > >>>> + * through VIRTIO_TRANSPORT_F_END are unset, i.e. 0xfffffc000fffffff for > >>>> + * all 64bit features. If the features are extended beyond 64 bits, or > >>>> new > >>>> + * "holes" are reserved for other type of features than per-device, this > >>>> + * macro would have to be updated. > >>>> + */ > >>>> +#define VIRTIO_DEVICE_F_MASK (~0ULL << (VIRTIO_TRANSPORT_F_END + 1) | \ > >>>> + ((1ULL << VIRTIO_TRANSPORT_F_START) - 1)) > >>>> + > >>>> static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct > >>>> genl_info *info) > >>>> { > >>>> struct vdpa_dev_set_config config = {}; > >>>> struct nlattr **nl_attrs = info->attrs; > >>>> struct vdpa_mgmt_dev *mdev; > >>>> + unsigned int ncls = 0; > >>>> const u8 *macaddr; > >>>> const char *name; > >>>> + u64 classes; > >>>> int err = 0; > >>>> > >>>> if (!info->attrs[VDPA_ATTR_DEV_NAME]) > >>>> @@ -649,6 +672,24 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct > >>>> sk_buff *skb, struct genl_info *i > >>>> goto err; > >>>> } > >>>> > >>>> + classes = vdpa_mgmtdev_get_classes(mdev, &ncls); > >>>> + if (config.mask & VDPA_DEV_NET_ATTRS_MASK && > >>>> + !(classes & BIT_ULL(VIRTIO_ID_NET))) { > >>>> + NL_SET_ERR_MSG_MOD(info->extack, > >>>> + "Network class attributes provided on > >>>> unsupported management device"); > >>>> + err = -EINVAL; > >>>> + goto err; > >>>> + } > >>>> + if (!(config.mask & VDPA_DEV_NET_ATTRS_MASK) && > >>>> + config.mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES) && > >>>> + classes & BIT_ULL(VIRTIO_ID_NET) && ncls > 1 && > >>>> + config.device_features & VIRTIO_DEVICE_F_MASK) { > >>>> + NL_SET_ERR_MSG_MOD(info->extack, > >>>> + "Management device supports > >>>> multi-class while device features specified are ambiguous"); > >>>> + err = -EINVAL; > >>>> + goto err; > >>>> + } > >>> Hi! I need to question this last if() :). What's the point of error > >>> when we specify features device-specific, from net or blk? > >> Because device specific features belong to different feature space, for > >> instance, VIRTIO_BLK_F_SIZE_MAX (1) on block device and > >> VIRTIO_NET_F_GUEST_CSUM (1) on network device both use same feature bit > >> value of (1<<1)ULL, but they belong to different type of devices. > >> > >>> In the VDUSE case both blk and net are supported. I want to use > >>> device_features to limit the net features that the VDUSE device > >>> exports. > >> Then we have to extend to the vdpa CLI to add "class ..." attribute to > >> explicitly indicate which type of device the creation has to be, so > >> eliminate the ambiguity entirely. > >> > >>> Also, why is this limited to only net devices? > >> Actually, this is not limited to only net I think, we can even remove the > >> > >> classes & BIT_ULL(VIRTIO_ID_NET) > >> > >> conditional if mgmtdev and vdpa dev instance is 1:1 bound. But at the > >> point when this code was written, it's not clear to me how multi-class > >> can be supported - such that does it limit to one vdpa instance > >> supporting one single class 1:1, or it is even possible to support both > >> or multiple classes (multi-facets) per vdpa instance i.e. 1:N. > >> > >>> does this part: > >>> > >>> classes & BIT_ULL(VIRTIO_ID_NET) && ncls > 1 > >>> > >>> Means that it is ok to specify more than one class as long as the set > >>> does not contain net? > >> Exactly, that's why it is coded in that odd way. For instance, if a > >> multi-facet vdpa instance needs to be provisioned with respective > >> feature bits for both block and iSCSI device types at the same time, we > >> may have to extend the CLI usage to support that. > >> > > Right I get the algorithm, but I still don't get what this is trying > > to solve :). > Well the code was written at a time before any vdpa block device was > added to vdpa, and it was anticipated to expand the same to other type > of devices. The feature bit provisioning at this mgmtdev layer is > expected to be only meaningful when you know which type of device you > want to create, or it may have to defer to the device specific way to > provision features. vduse is the latter example where it has the freedom > to defer device class binding and have its client drive the feature > provisioning. >
Does it mean it is ok for you to move this code to the backend itself? Should we do any change in any current backend in that case?
