Re: [PATCH v2 1/2] vdpa: add the check for id_table in struct vdpa_mgmt_dev

[Jason Wang]

On Thu, Apr 28, 2022 at 9:56 AM Cindy Lu <lulu@xxxxxxxxxx> wrote:
>
> On Wed, Apr 27, 2022 at 12:04 PM Jason Wang <jasowang@xxxxxxxxxx> wrote:
> >
> >
> > 在 2022/4/27 10:01, Cindy Lu 写道:
> > > On Mon, Apr 25, 2022 at 5:00 PM Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > >> On Mon, Apr 25, 2022 at 2:27 PM Cindy Lu <lulu@xxxxxxxxxx> wrote:
> > >>> To support the dynamic ids in vp_vdpa, we need to add the check for
> > >>> id table. If the id table is NULL, will not set the device type
> > >>>
> > >>> Signed-off-by: Cindy Lu <lulu@xxxxxxxxxx>
> > >>> ---
> > >>>   drivers/vdpa/vdpa.c | 11 +++++++----
> > >>>   1 file changed, 7 insertions(+), 4 deletions(-)
> > >>>
> > >>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > >>> index 1ea525433a5c..09edd92cede0 100644
> > >>> --- a/drivers/vdpa/vdpa.c
> > >>> +++ b/drivers/vdpa/vdpa.c
> > >>> @@ -492,10 +492,13 @@ 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 (mdev->id_table != NULL) {
> > >>> +               while (mdev->id_table[i].device) {
> > >>> +                       if (mdev->id_table[i].device <= 63)
> > >>> +                               supported_classes |=
> > >>> +                                       BIT_ULL(mdev->id_table[i].device);
> > >>> +                       i++;
> > >>> +               }
> > >>>          }
> > >> This will cause 0 to be advertised as the supported classes.
> > >>
> > >> I wonder if we can simply use VIRTIO_DEV_ANY_ID here (and need to
> > >> export it to via uAPI probably).
> > >>
> > >> Thanks
> > >>
> > > like the below one? not sure if this ok to use like this?
> > > static struct virtio_device_id vp_vdpa_id_table[] = {
> > > { VIRTIO_DEV_ANY_ID, VIRTIO_DEV_ANY_ID },
> > > { 0 },
> > > };
> >
> >
> > Something like this.
> >
> > Thanks
> >
> >
> I have checked the code, this maybe can not work, because the
> #define VIRTIO_DEV_ANY_ID 0xffffffff
>  it want't work in
>                 supported_classes |= BIT_ULL(mdev->id_table[i].device);
> if we chane to
>             supported_classes |= VIRTIO_DEV_ANY_ID;
> the vdpa dev show will be
> pci/0000:00:04.0:
>   supported_classes net block < unknown class > < unknown class > <
> unknown class > < unknown class > < unknown class > < unknow>
>   max_supported_vqs 3

That's why I suggest exporting the ANY_ID via uAPI and then we can fix
the userspace.

>   dev_features CSUM GUEST_CSUM CTRL_GUEST_OFFLOADS MAC GUEST_TSO4
> GUEST_TSO6 GUEST_ECN GUEST_UFO HOST_TSO4 HOST_TSO6 HOST_
>  I think we can use
> static struct virtio_device_id id_table[] = {
> { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
> { 0 },
> };
>  if we need to add another type of device, we can add the device id at that type
>

My point is that, we have supported any virtio devices before. But
after this change, we only support virtio-net.

So if we choose to use id arrays, let's just add all possible virtio
devices that are supported by the kernel here.

Thanks

> Thanks
> cindy
>
>
>
> > >
> > >
> > >>>          if (nla_put_u64_64bit(msg, VDPA_ATTR_MGMTDEV_SUPPORTED_CLASSES,
> > >>> --
> > >>> 2.34.1
> > >>>
> >
>