Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
MIME-Version: 1.0
References: <1697880319-4937-1-git-send-email-si-wei.liu@oracle.com>
 <1697880319-4937-6-git-send-email-si-wei.liu@oracle.com> <CACGkMEun7pnq5zMBH=_X+H9p_=HK6+5WHRp9A2prRe2Na-qmvQ@mail.gmail.com>
 <0ad99060-119b-4465-b611-868f693aea02@oracle.com> <d71dc008-9920-463d-948a-0375c5754810@oracle.com>
In-Reply-To: <d71dc008-9920-463d-948a-0375c5754810@oracle.com>
From:   Jason Wang <jasowang@redhat.com>
Date:   Wed, 25 Oct 2023 09:15:52 +0800
Message-ID: <CACGkMEtXOzqP_FY_UmmZZoDnOBg4MXS-EoE-vKC_=MM3cgKnmQ@mail.gmail.com>
Subject: Re: [PATCH v4 5/7] vhost-vdpa: clean iotlb map during reset for older userspace
To:     Si-Wei Liu <si-wei.liu@oracle.com>
Cc:     mst@redhat.com, eperezma@redhat.com, sgarzare@redhat.com,
        dtatulea@nvidia.com, virtualization@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Oct 25, 2023 at 12:25=E2=80=AFAM Si-Wei Liu <si-wei.liu@oracle.com>=
 wrote:
>
>
>
> On 10/24/2023 9:21 AM, Si-Wei Liu wrote:
> >
> >
> > On 10/23/2023 10:45 PM, Jason Wang wrote:
> >> On Sat, Oct 21, 2023 at 5:28=E2=80=AFPM Si-Wei Liu <si-wei.liu@oracle.=
com>
> >> wrote:
> >>> Using .compat_reset op from the previous patch, the buggy .reset
> >>> behaviour can be kept as-is on older userspace apps, which don't ack
> >>> the
> >>> IOTLB_PERSIST backend feature. As this compatibility quirk is
> >>> limited to
> >>> those drivers that used to be buggy in the past, it won't affect chan=
ge
> >>> the behaviour or affect ABI on the setups with API compliant driver.
> >>>
> >>> The separation of .compat_reset from the regular .reset allows
> >>> vhost-vdpa able to know which driver had broken behaviour before, so =
it
> >>> can apply the corresponding compatibility quirk to the individual
> >>> driver
> >>> whenever needed.  Compared to overloading the existing .reset with
> >>> flags, .compat_reset won't cause any extra burden to the implementati=
on
> >>> of every compliant driver.
> >>>
> >>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> >>> ---
> >>>   drivers/vhost/vdpa.c         | 17 +++++++++++++----
> >>>   drivers/virtio/virtio_vdpa.c |  2 +-
> >>>   include/linux/vdpa.h         |  7 +++++--
> >>>   3 files changed, 19 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> >>> index acc7c74ba7d6..9ce40003793b 100644
> >>> --- a/drivers/vhost/vdpa.c
> >>> +++ b/drivers/vhost/vdpa.c
> >>> @@ -227,13 +227,22 @@ static void vhost_vdpa_unsetup_vq_irq(struct
> >>> vhost_vdpa *v, u16 qid)
> >>> irq_bypass_unregister_producer(&vq->call_ctx.producer);
> >>>   }
> >>>
> >>> -static int vhost_vdpa_reset(struct vhost_vdpa *v)
> >>> +static int _compat_vdpa_reset(struct vhost_vdpa *v)
> >>>   {
> >>>          struct vdpa_device *vdpa =3D v->vdpa;
> >>> +       u32 flags =3D 0;
> >>>
> >>> -       v->in_batch =3D 0;
> >>> +       flags |=3D !vhost_backend_has_feature(v->vdev.vqs[0],
> >>> + VHOST_BACKEND_F_IOTLB_PERSIST) ?
> >>> +                VDPA_RESET_F_CLEAN_MAP : 0;
> >>> +
> >>> +       return vdpa_reset(vdpa, flags);
> >>> +}
> >>>
> >>> -       return vdpa_reset(vdpa);
> >>> +static int vhost_vdpa_reset(struct vhost_vdpa *v)
> >>> +{
> >>> +       v->in_batch =3D 0;
> >>> +       return _compat_vdpa_reset(v);
> >>>   }
> >>>
> >>>   static long vhost_vdpa_bind_mm(struct vhost_vdpa *v)
> >>> @@ -312,7 +321,7 @@ static long vhost_vdpa_set_status(struct
> >>> vhost_vdpa *v, u8 __user *statusp)
> >>>                          vhost_vdpa_unsetup_vq_irq(v, i);
> >>>
> >>>          if (status =3D=3D 0) {
> >>> -               ret =3D vdpa_reset(vdpa);
> >>> +               ret =3D _compat_vdpa_reset(v);
> >>>                  if (ret)
> >>>                          return ret;
> >>>          } else
> >>> diff --git a/drivers/virtio/virtio_vdpa.c
> >>> b/drivers/virtio/virtio_vdpa.c
> >>> index 06ce6d8c2e00..8d63e5923d24 100644
> >>> --- a/drivers/virtio/virtio_vdpa.c
> >>> +++ b/drivers/virtio/virtio_vdpa.c
> >>> @@ -100,7 +100,7 @@ static void virtio_vdpa_reset(struct
> >>> virtio_device *vdev)
> >>>   {
> >>>          struct vdpa_device *vdpa =3D vd_get_vdpa(vdev);
> >>>
> >>> -       vdpa_reset(vdpa);
> >>> +       vdpa_reset(vdpa, 0);
> >>>   }
> >>>
> >>>   static bool virtio_vdpa_notify(struct virtqueue *vq)
> >>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> >>> index 6b8cbf75712d..db15ac07f8a6 100644
> >>> --- a/include/linux/vdpa.h
> >>> +++ b/include/linux/vdpa.h
> >>> @@ -519,14 +519,17 @@ static inline struct device
> >>> *vdpa_get_dma_dev(struct vdpa_device *vdev)
> >>>          return vdev->dma_dev;
> >>>   }
> >>>
> >>> -static inline int vdpa_reset(struct vdpa_device *vdev)
> >>> +static inline int vdpa_reset(struct vdpa_device *vdev, u32 flags)
> >>>   {
> >>>          const struct vdpa_config_ops *ops =3D vdev->config;
> >>>          int ret;
> >>>
> >>>          down_write(&vdev->cf_lock);
> >>>          vdev->features_valid =3D false;
> >>> -       ret =3D ops->reset(vdev);
> >>> +       if (ops->compat_reset && flags)
> >>> +               ret =3D ops->compat_reset(vdev, flags);
> >>> +       else
> >>> +               ret =3D ops->reset(vdev);
> >> Instead of inventing a new API that carries the flags. Tweak the
> >> existing one seems to be simpler and better?
> > Well, as indicated in the commit message, this allows vhost-vdpa be
> > able to know which driver had broken behavior before, so it
> > can apply the corresponding compatibility quirk to the individual
> > driver when it's really necessary. If sending all flags
> > unconditionally down to every driver,

It depends on whether IOTLB_PERSIST is set.

> it's hard for driver writers to
> > distinguish which are compatibility quirks that they can safely ignore
> > and which are feature flags that are encouraged to implement. In that
> > sense, gating features from being polluted by compatibility quirks
> > with an implicit op
> s/implicit/explicit/
> > would be better.

Both of us have the points, we can listen to Michael or Eugenio for sure.

Thanks

> >
> > Regards,
> > -Siwei
> >>
> >> As compat_reset(vdev, 0) =3D=3D reset(vdev)
> >>
> >> Then you don't need the switch in the parent as well
> >>
> >> +static int vdpasim_reset(struct vdpa_device *vdpa)
> >> +{
> >> +       return vdpasim_compat_reset(vdpa, 0);
> >> +}
> >>
> >> Thanks
> >>
> >>
> >>> up_write(&vdev->cf_lock);
> >>>          return ret;
> >>>   }
> >>> --
> >>> 2.39.3
> >>>
> >
>