thr3ads.net - Virtualization - [PATCH] vdpa: reject F_ENABLE_AFTER_DRIVER_OK if backend does not support it [Jul 2023]

If this information is useful, please help other people find it:
Share via:

Michael S. Tsirkin

2023-Jul-05 08:42 UTC

[PATCH] vdpa: reject F_ENABLE_AFTER_DRIVER_OK if backend does not support it

On Wed, Jul 05, 2023 at 03:55:23PM +0800, Jason Wang
wrote:> On Tue, Jul 4, 2023 at 6:38?PM Michael S. Tsirkin <mst at redhat.com>
wrote:
> >
> > On Tue, Jul 04, 2023 at 12:25:32PM +0200, Eugenio Perez Martin wrote:
> > > On Mon, Jul 3, 2023 at 4:52?PM Michael S. Tsirkin <mst at
redhat.com> wrote:
> > > >
> > > > On Mon, Jul 03, 2023 at 04:22:18PM +0200, Eugenio P?rez
wrote:
> > > > > With the current code it is accepted as long as
userland send it.
> > > > >
> > > > > Although userland should not set a feature flag that
has not been
> > > > > offered to it with VHOST_GET_BACKEND_FEATURES, the
current code will not
> > > > > complain for it.
> > > > >
> > > > > Since there is no specific reason for any parent to
reject that backend
> > > > > feature bit when it has been proposed, let's
control it at vdpa frontend
> > > > > level. Future patches may move this control to the
parent driver.
> > > > >
> > > > > Fixes: 967800d2d52e ("vdpa: accept
VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK backend feature")
> > > > > Signed-off-by: Eugenio P?rez <eperezma at
redhat.com>
> > > >
> > > > Please do send v3. And again, I don't want to send
"after driver ok" hack
> > > > upstream at all, I merged it in next just to give it some
testing.
> > > > We want RING_ACCESS_AFTER_KICK or some such.
> > > >
> > >
> > > Current devices do not support that semantic.
> >
> > Which devices specifically access the ring after DRIVER_OK but before
> > a kick?
> 
> Vhost-net is one example at last. It polls a socket as well, so it
> starts to access the ring immediately after DRIVER_OK.
> 
> Thanks

For sure but that is not vdpa.
> >
> > > My plan was to convert
> > > it in vp_vdpa if needed, and reuse the current vdpa ops. Sorry if
I
> > > was not explicit enough.
> > >
> > > The only solution I can see to that is to trap & emulate in
the vdpa
> > > (parent?) driver, as talked in virtio-comment. But that
complicates
> > > the architecture:
> > > * Offer VHOST_BACKEND_F_RING_ACCESS_AFTER_KICK
> > > * Store vq enable state separately, at
> > > vdpa->config->set_vq_ready(true), but not transmit that
enable to hw
> > > * Store the doorbell state separately, but do not configure it to
the
> > > device directly.
> > >
> > > But how to recover if the device cannot configure them at kick
time,
> > > for example?
> > >
> > > Maybe we can just fail if the parent driver does not support
enabling
> > > the vq after DRIVER_OK? That way no new feature flag is needed.
> > >
> > > Thanks!
> > >
> > > >
> > > > > ---
> > > > > Sent with Fixes: tag pointing to
git.kernel.org/pub/scm/linux/kernel/git/mst
> > > > > commit. Please let me know if I should send a v3 of [1]
instead.
> > > > >
> > > > > [1]
https://lore.kernel.org/lkml/20230609121244-mutt-send-email-mst at kernel.org/T/
> > > > > ---
> > > > >  drivers/vhost/vdpa.c | 7 +++++--
> > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/vhost/vdpa.c
b/drivers/vhost/vdpa.c
> > > > > index e1abf29fed5b..a7e554352351 100644
> > > > > --- a/drivers/vhost/vdpa.c
> > > > > +++ b/drivers/vhost/vdpa.c
> > > > > @@ -681,18 +681,21 @@ static long
vhost_vdpa_unlocked_ioctl(struct file *filep,
> > > > >  {
> > > > >       struct vhost_vdpa *v = filep->private_data;
> > > > >       struct vhost_dev *d = &v->vdev;
> > > > > +     const struct vdpa_config_ops *ops =
v->vdpa->config;
> > > > >       void __user *argp = (void __user *)arg;
> > > > >       u64 __user *featurep = argp;
> > > > > -     u64 features;
> > > > > +     u64 features, parent_features = 0;
> > > > >       long r = 0;
> > > > >
> > > > >       if (cmd == VHOST_SET_BACKEND_FEATURES) {
> > > > >               if (copy_from_user(&features,
featurep, sizeof(features)))
> > > > >                       return -EFAULT;
> > > > > +             if (ops->get_backend_features)
> > > > > +                     parent_features =
ops->get_backend_features(v->vdpa);
> > > > >               if (features &
~(VHOST_VDPA_BACKEND_FEATURES |
> > > > >                               
BIT_ULL(VHOST_BACKEND_F_SUSPEND) |
> > > > >                               
BIT_ULL(VHOST_BACKEND_F_RESUME) |
> > > > > -                             
BIT_ULL(VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK)))
> > > > > +                              parent_features))
> > > > >                       return -EOPNOTSUPP;
> > > > >               if ((features &
BIT_ULL(VHOST_BACKEND_F_SUSPEND)) &&
> > > > >                    !vhost_vdpa_can_suspend(v))
> > > > > --
> > > > > 2.39.3
> > > >
> >

Jason Wang

2023-Jul-06 01:56 UTC

head link

[PATCH] vdpa: reject F_ENABLE_AFTER_DRIVER_OK if backend does not support it

On Wed, Jul 5, 2023 at 4:42?PM Michael S. Tsirkin <mst at redhat.com>
wrote:>
> On Wed, Jul 05, 2023 at 03:55:23PM +0800, Jason Wang wrote:
> > On Tue, Jul 4, 2023 at 6:38?PM Michael S. Tsirkin <mst at
redhat.com> wrote:
> > >
> > > On Tue, Jul 04, 2023 at 12:25:32PM +0200, Eugenio Perez Martin
wrote:
> > > > On Mon, Jul 3, 2023 at 4:52?PM Michael S. Tsirkin <mst at
redhat.com> wrote:
> > > > >
> > > > > On Mon, Jul 03, 2023 at 04:22:18PM +0200, Eugenio P?rez
wrote:
> > > > > > With the current code it is accepted as long as
userland send it.
> > > > > >
> > > > > > Although userland should not set a feature flag
that has not been
> > > > > > offered to it with VHOST_GET_BACKEND_FEATURES, the
current code will not
> > > > > > complain for it.
> > > > > >
> > > > > > Since there is no specific reason for any parent
to reject that backend
> > > > > > feature bit when it has been proposed, let's
control it at vdpa frontend
> > > > > > level. Future patches may move this control to the
parent driver.
> > > > > >
> > > > > > Fixes: 967800d2d52e ("vdpa: accept
VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK backend feature")
> > > > > > Signed-off-by: Eugenio P?rez <eperezma at
redhat.com>
> > > > >
> > > > > Please do send v3. And again, I don't want to send
"after driver ok" hack
> > > > > upstream at all, I merged it in next just to give it
some testing.
> > > > > We want RING_ACCESS_AFTER_KICK or some such.
> > > > >
> > > >
> > > > Current devices do not support that semantic.
> > >
> > > Which devices specifically access the ring after DRIVER_OK but
before
> > > a kick?
> >
> > Vhost-net is one example at last. It polls a socket as well, so it
> > starts to access the ring immediately after DRIVER_OK.
> >
> > Thanks
>
>
> For sure but that is not vdpa.
Somehow via vp_vdpa that I'm usually testing vDPA patches.

The problem is that it's very hard to audit all vDPA devices now if it
is not defined in the spec before they are invented.

Thanks
>
> > >
> > > > My plan was to convert
> > > > it in vp_vdpa if needed, and reuse the current vdpa ops.
Sorry if I
> > > > was not explicit enough.
> > > >
> > > > The only solution I can see to that is to trap & emulate
in the vdpa
> > > > (parent?) driver, as talked in virtio-comment. But that
complicates
> > > > the architecture:
> > > > * Offer VHOST_BACKEND_F_RING_ACCESS_AFTER_KICK
> > > > * Store vq enable state separately, at
> > > > vdpa->config->set_vq_ready(true), but not transmit
that enable to hw
> > > > * Store the doorbell state separately, but do not configure
it to the
> > > > device directly.
> > > >
> > > > But how to recover if the device cannot configure them at
kick time,
> > > > for example?
> > > >
> > > > Maybe we can just fail if the parent driver does not support
enabling
> > > > the vq after DRIVER_OK? That way no new feature flag is
needed.
> > > >
> > > > Thanks!
> > > >
> > > > >
> > > > > > ---
> > > > > > Sent with Fixes: tag pointing to
git.kernel.org/pub/scm/linux/kernel/git/mst
> > > > > > commit. Please let me know if I should send a v3
of [1] instead.
> > > > > >
> > > > > > [1]
https://lore.kernel.org/lkml/20230609121244-mutt-send-email-mst at kernel.org/T/
> > > > > > ---
> > > > > >  drivers/vhost/vdpa.c | 7 +++++--
> > > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/vhost/vdpa.c
b/drivers/vhost/vdpa.c
> > > > > > index e1abf29fed5b..a7e554352351 100644
> > > > > > --- a/drivers/vhost/vdpa.c
> > > > > > +++ b/drivers/vhost/vdpa.c
> > > > > > @@ -681,18 +681,21 @@ static long
vhost_vdpa_unlocked_ioctl(struct file *filep,
> > > > > >  {
> > > > > >       struct vhost_vdpa *v =
filep->private_data;
> > > > > >       struct vhost_dev *d = &v->vdev;
> > > > > > +     const struct vdpa_config_ops *ops =
v->vdpa->config;
> > > > > >       void __user *argp = (void __user *)arg;
> > > > > >       u64 __user *featurep = argp;
> > > > > > -     u64 features;
> > > > > > +     u64 features, parent_features = 0;
> > > > > >       long r = 0;
> > > > > >
> > > > > >       if (cmd == VHOST_SET_BACKEND_FEATURES) {
> > > > > >               if (copy_from_user(&features,
featurep, sizeof(features)))
> > > > > >                       return -EFAULT;
> > > > > > +             if (ops->get_backend_features)
> > > > > > +                     parent_features =
ops->get_backend_features(v->vdpa);
> > > > > >               if (features &
~(VHOST_VDPA_BACKEND_FEATURES |
> > > > > >                               
BIT_ULL(VHOST_BACKEND_F_SUSPEND) |
> > > > > >                               
BIT_ULL(VHOST_BACKEND_F_RESUME) |
> > > > > > -                             
BIT_ULL(VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK)))
> > > > > > +                              parent_features))
> > > > > >                       return -EOPNOTSUPP;
> > > > > >               if ((features &
BIT_ULL(VHOST_BACKEND_F_SUSPEND)) &&
> > > > > >                    !vhost_vdpa_can_suspend(v))
> > > > > > --
> > > > > > 2.39.3
> > > > >
> > >
>

Virtualization - Jul 2023 - [PATCH] vdpa: reject F_ENABLE_AFTER_DRIVER_OK if backend does not support it

[PATCH] vdpa: reject F_ENABLE_AFTER_DRIVER_OK if backend does not support it

[PATCH] vdpa: reject F_ENABLE_AFTER_DRIVER_OK if backend does not support it