linux-kernel - Re: [PATCH v2 1/2] vdpa: add the check for id_table in struct vdpa_mgmt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 28 Apr 2022 11:45:53 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Cindy Lu <lulu@...hat.com>
Cc:     mst <mst@...hat.com>,
        virtualization <virtualization@...ts.linux-foundation.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [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@...hat.com> wrote:
>
> On Thu, Apr 28, 2022 at 11:07 AM Jason Wang <jasowang@...hat.com> wrote:
> >
> > On Thu, Apr 28, 2022 at 9:56 AM Cindy Lu <lulu@...hat.com> wrote:
> > >
> > > On Wed, Apr 27, 2022 at 12:04 PM Jason Wang <jasowang@...hat.com> wrote:
> > > >
> > > >
> > > > 在 2022/4/27 10:01, Cindy Lu 写道:
> > > > > On Mon, Apr 25, 2022 at 5:00 PM Jason Wang <jasowang@...hat.com> wrote:
> > > > >> On Mon, Apr 25, 2022 at 2:27 PM Cindy Lu <lulu@...hat.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@...hat.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
> > > > >>>
> > > >
> > >
> >
>