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

[Cindy Lu]

On Thu, Apr 28, 2022 at 11:07 AM Jason Wang <jasowang@xxxxxxxxxx> wrote:
>
> 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.
>
sure.But I think maybe we can fix this in another patch, since it
related to 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.
>
sorry， I didn't make it clearly,  I mean  we can use the vp_vdpa device id as
 static struct virtio_device_id id_table[] = {
 { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
{ 0 },
since it now only support the net device and This will make the vp_vdpa work.

Thanks
cindy

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