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? 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. Also, why is this limited to only net devices? 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? > + > err = mdev->ops->dev_add(mdev, name, &config); > err: > up_write(&vdpa_dev_lock); > -- > 1.8.3.1 >
