Re: [PATCH RESENT v4 4/6] vdpa: validate device feature provisioning against supported class

[Eugenio Perez Martin]

On Fri, Oct 10, 2025 at 6:44 AM Si-Wei Liu <si-wei.liu@xxxxxxxxxx> wrote:
>
>
>
> On 10/2/2025 12:27 AM, Eugenio Perez Martin wrote:
> > On Thu, Oct 2, 2025 at 1:27 AM Si-Wei Liu <si-wei.liu@xxxxxxxxxx> 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 <si-wei.liu@xxxxxxxxxx> 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 <si-wei.liu@xxxxxxxxxx>
> >>>> ---
> >>>>    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?