Virtualization - Apr 2022 - [PATCH v2 1/2] vdpa: add the check for id_table in struct vdpa_mgmt_dev

On Thu, Apr 28, 2022 at 11:21 AM Cindy Lu <lulu at redhat.com>
wrote:
>
> On Thu, Apr 28, 2022 at 11:07 AM Jason Wang <jasowang at redhat.com>
wrote:
> >
> > On Thu, Apr 28, 2022 at 9:56 AM Cindy Lu <lulu at redhat.com>
wrote:
> > >
> > > On Wed, Apr 27, 2022 at 12:04 PM Jason Wang <jasowang at
redhat.com> wrote:
> > > >
> > > >
> > > > ? 2022/4/27 10:01, Cindy Lu ??:
> > > > > On Mon, Apr 25, 2022 at 5:00 PM Jason Wang <jasowang
at redhat.com> wrote:
> > > > >> On Mon, Apr 25, 2022 at 2:27 PM Cindy Lu <lulu
at redhat.com> 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 at
redhat.com>
> > > > >>> ---
> > > > >>>   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
Yes.
>
> > >   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
This is not what I read from the code:

static int virtio_vdpa_probe(struct vdpa_device *vdpa)
{
...
vd_dev->vdev.id.device = ops->get_device_id(vdpa);
        if (vd_dev->vdev.id.device == 0)
                goto err;

vd_dev->vdev.id.vendor = ops->get_vendor_id(vdpa);
        ret = register_virtio_device(&vd_dev->vdev);
...
}

?

Thanks
>  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
> > > > >>>
> > > >
> > >
> >
>