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 <[email protected]>
---
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/[email protected]/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
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 <[email protected]>
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.
> ---
> 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/[email protected]/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
On Mon, Jul 3, 2023 at 4:52 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]>
>
> 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. 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/[email protected]/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
>
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 <[email protected]> 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 <[email protected]>
> >
> > 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?
> 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/[email protected]/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
> >
On Tue, Jul 4, 2023 at 12:38 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > >
> > > 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?
>
Previous versions of the QEMU LM series did a spurious kick to start
traffic at the LM destination [1]. When it was proposed, that spurious
kick was removed from the series because to check for descriptors
after driver_ok, even without a kick, was considered work of the
parent driver.
I'm ok to go back to this spurious kick, but I'm not sure if the hw
will read the ring before the kick actually. I can ask.
Thanks!
[1] https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02775.html
> > 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/[email protected]/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
> > >
>
On Tue, Jul 04, 2023 at 01:36:11PM +0200, Eugenio Perez Martin wrote:
> On Tue, Jul 4, 2023 at 12:38 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > >
> > > > 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?
> >
>
> Previous versions of the QEMU LM series did a spurious kick to start
> traffic at the LM destination [1]. When it was proposed, that spurious
> kick was removed from the series because to check for descriptors
> after driver_ok, even without a kick, was considered work of the
> parent driver.
>
> I'm ok to go back to this spurious kick, but I'm not sure if the hw
> will read the ring before the kick actually. I can ask.
>
> Thanks!
>
> [1] https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02775.html
Let's find out. We need to check for ENABLE_AFTER_DRIVER_OK too, no?
> > > 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/[email protected]/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
> > > >
> >
On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin <[email protected]> wrote:
>
> On Tue, Jul 04, 2023 at 01:36:11PM +0200, Eugenio Perez Martin wrote:
> > On Tue, Jul 4, 2023 at 12:38 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > >
> > > > > 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?
> > >
> >
> > Previous versions of the QEMU LM series did a spurious kick to start
> > traffic at the LM destination [1]. When it was proposed, that spurious
> > kick was removed from the series because to check for descriptors
> > after driver_ok, even without a kick, was considered work of the
> > parent driver.
> >
> > I'm ok to go back to this spurious kick, but I'm not sure if the hw
> > will read the ring before the kick actually. I can ask.
> >
> > Thanks!
> >
> > [1] https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02775.html
>
> Let's find out. We need to check for ENABLE_AFTER_DRIVER_OK too, no?
My understanding is [1] assuming ACCESS_AFTER_KICK. This seems
sub-optimal than assuming ENABLE_AFTER_DRIVER_OK.
But this reminds me one thing, as the thread is going too long, I
wonder if we simply assume ENABLE_AFTER_DRIVER_OK if RING_RESET is
supported?
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/[email protected]/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
> > > > >
> > >
>
On Tue, Jul 4, 2023 at 6:38 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > >
> > > 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
>
> > 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/[email protected]/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
> > >
>
On Wed, Jul 05, 2023 at 03:49:58PM +0800, Jason Wang wrote:
> On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin <[email protected]> wrote:
> >
> > On Tue, Jul 04, 2023 at 01:36:11PM +0200, Eugenio Perez Martin wrote:
> > > On Tue, Jul 4, 2023 at 12:38 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > >
> > > > > > 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?
> > > >
> > >
> > > Previous versions of the QEMU LM series did a spurious kick to start
> > > traffic at the LM destination [1]. When it was proposed, that spurious
> > > kick was removed from the series because to check for descriptors
> > > after driver_ok, even without a kick, was considered work of the
> > > parent driver.
> > >
> > > I'm ok to go back to this spurious kick, but I'm not sure if the hw
> > > will read the ring before the kick actually. I can ask.
> > >
> > > Thanks!
> > >
> > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02775.html
> >
> > Let's find out. We need to check for ENABLE_AFTER_DRIVER_OK too, no?
>
> My understanding is [1] assuming ACCESS_AFTER_KICK. This seems
> sub-optimal than assuming ENABLE_AFTER_DRIVER_OK.
>
> But this reminds me one thing, as the thread is going too long, I
> wonder if we simply assume ENABLE_AFTER_DRIVER_OK if RING_RESET is
> supported?
>
> Thanks
I don't see what does one have to do with another ...
I think with RING_RESET we had another solution, enable rings
mapping them to a zero page, then reset and re-enable later.
> >
> >
> >
> > > > > 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/[email protected]/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
> > > > > >
> > > >
> >
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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > >
> > > > 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/[email protected]/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
> > > >
> >
On Wed, Jul 5, 2023 at 9:50 AM Jason Wang <[email protected]> wrote:
>
> On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin <[email protected]> wrote:
> >
> > On Tue, Jul 04, 2023 at 01:36:11PM +0200, Eugenio Perez Martin wrote:
> > > On Tue, Jul 4, 2023 at 12:38 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > >
> > > > > > 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?
> > > >
> > >
> > > Previous versions of the QEMU LM series did a spurious kick to start
> > > traffic at the LM destination [1]. When it was proposed, that spurious
> > > kick was removed from the series because to check for descriptors
> > > after driver_ok, even without a kick, was considered work of the
> > > parent driver.
> > >
> > > I'm ok to go back to this spurious kick, but I'm not sure if the hw
> > > will read the ring before the kick actually. I can ask.
> > >
> > > Thanks!
> > >
> > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02775.html
> >
> > Let's find out. We need to check for ENABLE_AFTER_DRIVER_OK too, no?
>
> My understanding is [1] assuming ACCESS_AFTER_KICK. This seems
> sub-optimal than assuming ENABLE_AFTER_DRIVER_OK.
>
> But this reminds me one thing, as the thread is going too long, I
> wonder if we simply assume ENABLE_AFTER_DRIVER_OK if RING_RESET is
> supported?
>
The problem with that is that the device needs to support all
RING_RESET, like to be able to change vq address etc after DRIVER_OK.
Not all HW support it.
We just need the subset of having the dataplane freezed until all CVQ
commands have been consumed. I'm sure current vDPA code already
supports it in some devices, like MLX and PSD.
Thanks!
> 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/[email protected]/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
> > > > > >
> > > >
> >
>
On 7/5/23 11:27 AM, Eugenio Perez Martin wrote:
>
> On Wed, Jul 5, 2023 at 9:50 AM Jason Wang <[email protected]> wrote:
>>
>> On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin <[email protected]> wrote:
>>>
>>> On Tue, Jul 04, 2023 at 01:36:11PM +0200, Eugenio Perez Martin wrote:
>>>> On Tue, Jul 4, 2023 at 12:38 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>>>>
>>>>>>> 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?
The PDS vdpa device can deal with a call to .set_vq_ready after
DRIVER_OK is set. And I'm told that our VQ activity should start
without a kick.
Our vdpa device FW doesn't currently have support for
VIRTIO_F_RING_RESET, but I believe it could be added without too much
trouble.
sln
>>>>>
>>>>
>>>> Previous versions of the QEMU LM series did a spurious kick to start
>>>> traffic at the LM destination [1]. When it was proposed, that spurious
>>>> kick was removed from the series because to check for descriptors
>>>> after driver_ok, even without a kick, was considered work of the
>>>> parent driver.
>>>>
>>>> I'm ok to go back to this spurious kick, but I'm not sure if the hw
>>>> will read the ring before the kick actually. I can ask.
>>>>
>>>> Thanks!
>>>>
>>>> [1] https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02775.html
>>>
>>> Let's find out. We need to check for ENABLE_AFTER_DRIVER_OK too, no?
>>
>> My understanding is [1] assuming ACCESS_AFTER_KICK. This seems
>> sub-optimal than assuming ENABLE_AFTER_DRIVER_OK.
>>
>> But this reminds me one thing, as the thread is going too long, I
>> wonder if we simply assume ENABLE_AFTER_DRIVER_OK if RING_RESET is
>> supported?
>>
>
> The problem with that is that the device needs to support all
> RING_RESET, like to be able to change vq address etc after DRIVER_OK.
> Not all HW support it.
>
> We just need the subset of having the dataplane freezed until all CVQ
> commands have been consumed. I'm sure current vDPA code already
> supports it in some devices, like MLX and PSD.
>
> Thanks!
>
>> 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/[email protected]/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
>>>>>>>
>>>>>
>>>
>>
>
On Wed, Jul 5, 2023 at 4:42 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > >
> > > > > 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/[email protected]/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
> > > > >
> > >
>
On Wed, Jul 5, 2023 at 4:41 PM Michael S. Tsirkin <[email protected]> wrote:
>
> On Wed, Jul 05, 2023 at 03:49:58PM +0800, Jason Wang wrote:
> > On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin <[email protected]> wrote:
> > >
> > > On Tue, Jul 04, 2023 at 01:36:11PM +0200, Eugenio Perez Martin wrote:
> > > > On Tue, Jul 4, 2023 at 12:38 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > >
> > > > > > > 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?
> > > > >
> > > >
> > > > Previous versions of the QEMU LM series did a spurious kick to start
> > > > traffic at the LM destination [1]. When it was proposed, that spurious
> > > > kick was removed from the series because to check for descriptors
> > > > after driver_ok, even without a kick, was considered work of the
> > > > parent driver.
> > > >
> > > > I'm ok to go back to this spurious kick, but I'm not sure if the hw
> > > > will read the ring before the kick actually. I can ask.
> > > >
> > > > Thanks!
> > > >
> > > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02775.html
> > >
> > > Let's find out. We need to check for ENABLE_AFTER_DRIVER_OK too, no?
> >
> > My understanding is [1] assuming ACCESS_AFTER_KICK. This seems
> > sub-optimal than assuming ENABLE_AFTER_DRIVER_OK.
> >
> > But this reminds me one thing, as the thread is going too long, I
> > wonder if we simply assume ENABLE_AFTER_DRIVER_OK if RING_RESET is
> > supported?
> >
> > Thanks
>
> I don't see what does one have to do with another ...
>
> I think with RING_RESET we had another solution, enable rings
> mapping them to a zero page, then reset and re-enable later.
As discussed before, this seems to have some problems:
1) The behaviour is not clarified in the document
2) zero is a valid IOVA
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/[email protected]/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
> > > > > > >
> > > > >
> > >
>
On Thu, Jul 06, 2023 at 09:56:29AM +0800, Jason Wang wrote:
> On Wed, Jul 5, 2023 at 4:42 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > >
> > > > > > 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
vp_vdpa is exactly the part that bothers me. If on the host it is backed
by e.g. vhost-user then it does not work. And you don't know what is
backing it.
OTOH it supports RING_RESET ...
So, proposal: include both this solution and for drivers
vp_vdpa the RING_RESET trick.
Hmm?
> >
> > > >
> > > > > 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/[email protected]/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
> > > > > >
> > > >
> >
On Wed, Jul 05, 2023 at 05:07:11PM -0700, Shannon Nelson wrote:
> On 7/5/23 11:27 AM, Eugenio Perez Martin wrote:
> >
> > On Wed, Jul 5, 2023 at 9:50 AM Jason Wang <[email protected]> wrote:
> > >
> > > On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin <[email protected]> wrote:
> > > >
> > > > On Tue, Jul 04, 2023 at 01:36:11PM +0200, Eugenio Perez Martin wrote:
> > > > > On Tue, Jul 4, 2023 at 12:38 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > > >
> > > > > > > > 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?
>
> The PDS vdpa device can deal with a call to .set_vq_ready after DRIVER_OK is
> set. And I'm told that our VQ activity should start without a kick.
>
> Our vdpa device FW doesn't currently have support for VIRTIO_F_RING_RESET,
> but I believe it could be added without too much trouble.
>
> sln
>
OK it seems clear at least in the current version pds needs
VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK.
However can we also code up the RING_RESET path as the default?
Then down the road vendors can choose what to do.
> > > > > >
> > > > >
> > > > > Previous versions of the QEMU LM series did a spurious kick to start
> > > > > traffic at the LM destination [1]. When it was proposed, that spurious
> > > > > kick was removed from the series because to check for descriptors
> > > > > after driver_ok, even without a kick, was considered work of the
> > > > > parent driver.
> > > > >
> > > > > I'm ok to go back to this spurious kick, but I'm not sure if the hw
> > > > > will read the ring before the kick actually. I can ask.
> > > > >
> > > > > Thanks!
> > > > >
> > > > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02775.html
> > > >
> > > > Let's find out. We need to check for ENABLE_AFTER_DRIVER_OK too, no?
> > >
> > > My understanding is [1] assuming ACCESS_AFTER_KICK. This seems
> > > sub-optimal than assuming ENABLE_AFTER_DRIVER_OK.
> > >
> > > But this reminds me one thing, as the thread is going too long, I
> > > wonder if we simply assume ENABLE_AFTER_DRIVER_OK if RING_RESET is
> > > supported?
> > >
> >
> > The problem with that is that the device needs to support all
> > RING_RESET, like to be able to change vq address etc after DRIVER_OK.
> > Not all HW support it.
> >
> > We just need the subset of having the dataplane freezed until all CVQ
> > commands have been consumed. I'm sure current vDPA code already
> > supports it in some devices, like MLX and PSD.
> >
> > Thanks!
> >
> > > 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/[email protected]/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
> > > > > > > >
> > > > > >
> > > >
> > >
> >
On Thu, Jul 06, 2023 at 08:35:30AM +0200, Eugenio Perez Martin wrote:
> On Thu, Jul 6, 2023 at 8:07 AM Michael S. Tsirkin <[email protected]> wrote:
> >
> > On Wed, Jul 05, 2023 at 05:07:11PM -0700, Shannon Nelson wrote:
> > > On 7/5/23 11:27 AM, Eugenio Perez Martin wrote:
> > > >
> > > > On Wed, Jul 5, 2023 at 9:50 AM Jason Wang <[email protected]> wrote:
> > > > >
> > > > > On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > > >
> > > > > > On Tue, Jul 04, 2023 at 01:36:11PM +0200, Eugenio Perez Martin wrote:
> > > > > > > On Tue, Jul 4, 2023 at 12:38 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > > > > >
> > > > > > > > > > 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?
> > >
> > > The PDS vdpa device can deal with a call to .set_vq_ready after DRIVER_OK is
> > > set. And I'm told that our VQ activity should start without a kick.
> > >
> > > Our vdpa device FW doesn't currently have support for VIRTIO_F_RING_RESET,
> > > but I believe it could be added without too much trouble.
> > >
> > > sln
> > >
> >
> > OK it seems clear at least in the current version pds needs
> > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK.
> > However can we also code up the RING_RESET path as the default?
> > Then down the road vendors can choose what to do.
> >
>
> Yes, the RING_RESET path can be coded & tested for vp_vdpa, for
> example. I'm ok with making it the default, and let
> _F_ENABLE_AFTER_DRIVER_OK as a fallback.
Sounds good.
> >
> >
> >
> >
> > > > > > > >
> > > > > > >
> > > > > > > Previous versions of the QEMU LM series did a spurious kick to start
> > > > > > > traffic at the LM destination [1]. When it was proposed, that spurious
> > > > > > > kick was removed from the series because to check for descriptors
> > > > > > > after driver_ok, even without a kick, was considered work of the
> > > > > > > parent driver.
> > > > > > >
> > > > > > > I'm ok to go back to this spurious kick, but I'm not sure if the hw
> > > > > > > will read the ring before the kick actually. I can ask.
> > > > > > >
> > > > > > > Thanks!
> > > > > > >
> > > > > > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02775.html
> > > > > >
> > > > > > Let's find out. We need to check for ENABLE_AFTER_DRIVER_OK too, no?
> > > > >
> > > > > My understanding is [1] assuming ACCESS_AFTER_KICK. This seems
> > > > > sub-optimal than assuming ENABLE_AFTER_DRIVER_OK.
> > > > >
> > > > > But this reminds me one thing, as the thread is going too long, I
> > > > > wonder if we simply assume ENABLE_AFTER_DRIVER_OK if RING_RESET is
> > > > > supported?
> > > > >
> > > >
> > > > The problem with that is that the device needs to support all
> > > > RING_RESET, like to be able to change vq address etc after DRIVER_OK.
> > > > Not all HW support it.
> > > >
> > > > We just need the subset of having the dataplane freezed until all CVQ
> > > > commands have been consumed. I'm sure current vDPA code already
> > > > supports it in some devices, like MLX and PSD.
> > > >
> > > > Thanks!
> > > >
> > > > > 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/[email protected]/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
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > > >
> > > >
> >
On Thu, Jul 6, 2023 at 8:07 AM Michael S. Tsirkin <[email protected]> wrote:
>
> On Wed, Jul 05, 2023 at 05:07:11PM -0700, Shannon Nelson wrote:
> > On 7/5/23 11:27 AM, Eugenio Perez Martin wrote:
> > >
> > > On Wed, Jul 5, 2023 at 9:50 AM Jason Wang <[email protected]> wrote:
> > > >
> > > > On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > >
> > > > > On Tue, Jul 04, 2023 at 01:36:11PM +0200, Eugenio Perez Martin wrote:
> > > > > > On Tue, Jul 4, 2023 at 12:38 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > > > >
> > > > > > > > > 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?
> >
> > The PDS vdpa device can deal with a call to .set_vq_ready after DRIVER_OK is
> > set. And I'm told that our VQ activity should start without a kick.
> >
> > Our vdpa device FW doesn't currently have support for VIRTIO_F_RING_RESET,
> > but I believe it could be added without too much trouble.
> >
> > sln
> >
>
> OK it seems clear at least in the current version pds needs
> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK.
> However can we also code up the RING_RESET path as the default?
> Then down the road vendors can choose what to do.
>
Yes, the RING_RESET path can be coded & tested for vp_vdpa, for
example. I'm ok with making it the default, and let
_F_ENABLE_AFTER_DRIVER_OK as a fallback.
>
>
>
>
> > > > > > >
> > > > > >
> > > > > > Previous versions of the QEMU LM series did a spurious kick to start
> > > > > > traffic at the LM destination [1]. When it was proposed, that spurious
> > > > > > kick was removed from the series because to check for descriptors
> > > > > > after driver_ok, even without a kick, was considered work of the
> > > > > > parent driver.
> > > > > >
> > > > > > I'm ok to go back to this spurious kick, but I'm not sure if the hw
> > > > > > will read the ring before the kick actually. I can ask.
> > > > > >
> > > > > > Thanks!
> > > > > >
> > > > > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02775.html
> > > > >
> > > > > Let's find out. We need to check for ENABLE_AFTER_DRIVER_OK too, no?
> > > >
> > > > My understanding is [1] assuming ACCESS_AFTER_KICK. This seems
> > > > sub-optimal than assuming ENABLE_AFTER_DRIVER_OK.
> > > >
> > > > But this reminds me one thing, as the thread is going too long, I
> > > > wonder if we simply assume ENABLE_AFTER_DRIVER_OK if RING_RESET is
> > > > supported?
> > > >
> > >
> > > The problem with that is that the device needs to support all
> > > RING_RESET, like to be able to change vq address etc after DRIVER_OK.
> > > Not all HW support it.
> > >
> > > We just need the subset of having the dataplane freezed until all CVQ
> > > commands have been consumed. I'm sure current vDPA code already
> > > supports it in some devices, like MLX and PSD.
> > >
> > > Thanks!
> > >
> > > > 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/[email protected]/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
> > > > > > > > >
> > > > > > >
> > > > >
> > > >
> > >
>
On Thu, Jul 6, 2023 at 3:55 AM Jason Wang <[email protected]> wrote:
>
> On Wed, Jul 5, 2023 at 4:41 PM Michael S. Tsirkin <[email protected]> wrote:
> >
> > On Wed, Jul 05, 2023 at 03:49:58PM +0800, Jason Wang wrote:
> > > On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin <[email protected]> wrote:
> > > >
> > > > On Tue, Jul 04, 2023 at 01:36:11PM +0200, Eugenio Perez Martin wrote:
> > > > > On Tue, Jul 4, 2023 at 12:38 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > > >
> > > > > > > > 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?
> > > > > >
> > > > >
> > > > > Previous versions of the QEMU LM series did a spurious kick to start
> > > > > traffic at the LM destination [1]. When it was proposed, that spurious
> > > > > kick was removed from the series because to check for descriptors
> > > > > after driver_ok, even without a kick, was considered work of the
> > > > > parent driver.
> > > > >
> > > > > I'm ok to go back to this spurious kick, but I'm not sure if the hw
> > > > > will read the ring before the kick actually. I can ask.
> > > > >
> > > > > Thanks!
> > > > >
> > > > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02775.html
> > > >
> > > > Let's find out. We need to check for ENABLE_AFTER_DRIVER_OK too, no?
> > >
> > > My understanding is [1] assuming ACCESS_AFTER_KICK. This seems
> > > sub-optimal than assuming ENABLE_AFTER_DRIVER_OK.
> > >
> > > But this reminds me one thing, as the thread is going too long, I
> > > wonder if we simply assume ENABLE_AFTER_DRIVER_OK if RING_RESET is
> > > supported?
> > >
> > > Thanks
> >
> > I don't see what does one have to do with another ...
> >
> > I think with RING_RESET we had another solution, enable rings
> > mapping them to a zero page, then reset and re-enable later.
>
> As discussed before, this seems to have some problems:
>
> 1) The behaviour is not clarified in the document
> 2) zero is a valid IOVA
>
I think we're not on the same page here.
As I understood, rings mapped to a zero page means essentially an
avail ring whose avail_idx is always 0, offered to the device instead
of the guest's ring. Once all CVQ commands are processed, we use
RING_RESET to switch to the right ring, being guest's or SVQ vring.
> 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/[email protected]/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
> > > > > > > >
> > > > > >
> > > >
> >
>
On Thu, Jul 6, 2023 at 3:06 PM Eugenio Perez Martin <[email protected]> wrote:
>
> On Thu, Jul 6, 2023 at 3:55 AM Jason Wang <[email protected]> wrote:
> >
> > On Wed, Jul 5, 2023 at 4:41 PM Michael S. Tsirkin <[email protected]> wrote:
> > >
> > > On Wed, Jul 05, 2023 at 03:49:58PM +0800, Jason Wang wrote:
> > > > On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > >
> > > > > On Tue, Jul 04, 2023 at 01:36:11PM +0200, Eugenio Perez Martin wrote:
> > > > > > On Tue, Jul 4, 2023 at 12:38 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > > > >
> > > > > > > > > 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?
> > > > > > >
> > > > > >
> > > > > > Previous versions of the QEMU LM series did a spurious kick to start
> > > > > > traffic at the LM destination [1]. When it was proposed, that spurious
> > > > > > kick was removed from the series because to check for descriptors
> > > > > > after driver_ok, even without a kick, was considered work of the
> > > > > > parent driver.
> > > > > >
> > > > > > I'm ok to go back to this spurious kick, but I'm not sure if the hw
> > > > > > will read the ring before the kick actually. I can ask.
> > > > > >
> > > > > > Thanks!
> > > > > >
> > > > > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02775.html
> > > > >
> > > > > Let's find out. We need to check for ENABLE_AFTER_DRIVER_OK too, no?
> > > >
> > > > My understanding is [1] assuming ACCESS_AFTER_KICK. This seems
> > > > sub-optimal than assuming ENABLE_AFTER_DRIVER_OK.
> > > >
> > > > But this reminds me one thing, as the thread is going too long, I
> > > > wonder if we simply assume ENABLE_AFTER_DRIVER_OK if RING_RESET is
> > > > supported?
> > > >
> > > > Thanks
> > >
> > > I don't see what does one have to do with another ...
> > >
> > > I think with RING_RESET we had another solution, enable rings
> > > mapping them to a zero page, then reset and re-enable later.
> >
> > As discussed before, this seems to have some problems:
> >
> > 1) The behaviour is not clarified in the document
> > 2) zero is a valid IOVA
> >
>
> I think we're not on the same page here.
>
> As I understood, rings mapped to a zero page means essentially an
> avail ring whose avail_idx is always 0, offered to the device instead
> of the guest's ring. Once all CVQ commands are processed, we use
> RING_RESET to switch to the right ring, being guest's or SVQ vring.
I get this. This seems more complicated in the destination: shadow vq + ASID?
Thanks
>
>
>
> > 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/[email protected]/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
> > > > > > > > >
> > > > > > >
> > > > >
> > >
> >
>
On Thu, Jul 6, 2023 at 3:55 PM Jason Wang <[email protected]> wrote:
>
> On Thu, Jul 6, 2023 at 3:06 PM Eugenio Perez Martin <[email protected]> wrote:
> >
> > On Thu, Jul 6, 2023 at 3:55 AM Jason Wang <[email protected]> wrote:
> > >
> > > On Wed, Jul 5, 2023 at 4:41 PM Michael S. Tsirkin <[email protected]> wrote:
> > > >
> > > > On Wed, Jul 05, 2023 at 03:49:58PM +0800, Jason Wang wrote:
> > > > > On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > > >
> > > > > > On Tue, Jul 04, 2023 at 01:36:11PM +0200, Eugenio Perez Martin wrote:
> > > > > > > On Tue, Jul 4, 2023 at 12:38 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > > > > >
> > > > > > > > > > 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?
> > > > > > > >
> > > > > > >
> > > > > > > Previous versions of the QEMU LM series did a spurious kick to start
> > > > > > > traffic at the LM destination [1]. When it was proposed, that spurious
> > > > > > > kick was removed from the series because to check for descriptors
> > > > > > > after driver_ok, even without a kick, was considered work of the
> > > > > > > parent driver.
> > > > > > >
> > > > > > > I'm ok to go back to this spurious kick, but I'm not sure if the hw
> > > > > > > will read the ring before the kick actually. I can ask.
> > > > > > >
> > > > > > > Thanks!
> > > > > > >
> > > > > > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02775.html
> > > > > >
> > > > > > Let's find out. We need to check for ENABLE_AFTER_DRIVER_OK too, no?
> > > > >
> > > > > My understanding is [1] assuming ACCESS_AFTER_KICK. This seems
> > > > > sub-optimal than assuming ENABLE_AFTER_DRIVER_OK.
> > > > >
> > > > > But this reminds me one thing, as the thread is going too long, I
> > > > > wonder if we simply assume ENABLE_AFTER_DRIVER_OK if RING_RESET is
> > > > > supported?
> > > > >
> > > > > Thanks
> > > >
> > > > I don't see what does one have to do with another ...
> > > >
> > > > I think with RING_RESET we had another solution, enable rings
> > > > mapping them to a zero page, then reset and re-enable later.
> > >
> > > As discussed before, this seems to have some problems:
> > >
> > > 1) The behaviour is not clarified in the document
> > > 2) zero is a valid IOVA
> > >
> >
> > I think we're not on the same page here.
> >
> > As I understood, rings mapped to a zero page means essentially an
> > avail ring whose avail_idx is always 0, offered to the device instead
> > of the guest's ring. Once all CVQ commands are processed, we use
> > RING_RESET to switch to the right ring, being guest's or SVQ vring.
>
> I get this. This seems more complicated in the destination: shadow vq + ASID?
So it's something like:
1) set all vq ASID to shadow virtqueue
2) do not add any bufs to data qp (stick 0 as avail index)
3) start to restore states via cvq
4) ring_rest for dataqp
5) set_vq_state for dataqp
6) re-initialize dataqp address etc
7) set data QP ASID to guest
8) set queue_enable
?
Thanks
>
> Thanks
>
> >
> >
> >
> > > 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/[email protected]/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
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> > >
> >
On Thu, Jul 6, 2023 at 10:03 AM Jason Wang <[email protected]> wrote:
>
> On Thu, Jul 6, 2023 at 3:55 PM Jason Wang <[email protected]> wrote:
> >
> > On Thu, Jul 6, 2023 at 3:06 PM Eugenio Perez Martin <[email protected]> wrote:
> > >
> > > On Thu, Jul 6, 2023 at 3:55 AM Jason Wang <[email protected]> wrote:
> > > >
> > > > On Wed, Jul 5, 2023 at 4:41 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > >
> > > > > On Wed, Jul 05, 2023 at 03:49:58PM +0800, Jason Wang wrote:
> > > > > > On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > > > >
> > > > > > > On Tue, Jul 04, 2023 at 01:36:11PM +0200, Eugenio Perez Martin wrote:
> > > > > > > > On Tue, Jul 4, 2023 at 12:38 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > > > > > >
> > > > > > > > > > > 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?
> > > > > > > > >
> > > > > > > >
> > > > > > > > Previous versions of the QEMU LM series did a spurious kick to start
> > > > > > > > traffic at the LM destination [1]. When it was proposed, that spurious
> > > > > > > > kick was removed from the series because to check for descriptors
> > > > > > > > after driver_ok, even without a kick, was considered work of the
> > > > > > > > parent driver.
> > > > > > > >
> > > > > > > > I'm ok to go back to this spurious kick, but I'm not sure if the hw
> > > > > > > > will read the ring before the kick actually. I can ask.
> > > > > > > >
> > > > > > > > Thanks!
> > > > > > > >
> > > > > > > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02775.html
> > > > > > >
> > > > > > > Let's find out. We need to check for ENABLE_AFTER_DRIVER_OK too, no?
> > > > > >
> > > > > > My understanding is [1] assuming ACCESS_AFTER_KICK. This seems
> > > > > > sub-optimal than assuming ENABLE_AFTER_DRIVER_OK.
> > > > > >
> > > > > > But this reminds me one thing, as the thread is going too long, I
> > > > > > wonder if we simply assume ENABLE_AFTER_DRIVER_OK if RING_RESET is
> > > > > > supported?
> > > > > >
> > > > > > Thanks
> > > > >
> > > > > I don't see what does one have to do with another ...
> > > > >
> > > > > I think with RING_RESET we had another solution, enable rings
> > > > > mapping them to a zero page, then reset and re-enable later.
> > > >
> > > > As discussed before, this seems to have some problems:
> > > >
> > > > 1) The behaviour is not clarified in the document
> > > > 2) zero is a valid IOVA
> > > >
> > >
> > > I think we're not on the same page here.
> > >
> > > As I understood, rings mapped to a zero page means essentially an
> > > avail ring whose avail_idx is always 0, offered to the device instead
> > > of the guest's ring. Once all CVQ commands are processed, we use
> > > RING_RESET to switch to the right ring, being guest's or SVQ vring.
> >
> > I get this. This seems more complicated in the destination: shadow vq + ASID?
>
> So it's something like:
>
> 1) set all vq ASID to shadow virtqueue
> 2) do not add any bufs to data qp (stick 0 as avail index)
> 3) start to restore states via cvq
> 4) ring_rest for dataqp
> 5) set_vq_state for dataqp
> 6) re-initialize dataqp address etc
> 7) set data QP ASID to guest
> 8) set queue_enable
>
> ?
>
I think the change of ASID is not needed, as the guest cannot access
the device in that timeframe anyway. Moreover, it may require HW
support. So steps 1 and 7 are not needed.
Apart from that, the process is right.
> Thanks
>
> >
> > Thanks
> >
> > >
> > >
> > >
> > > > 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/[email protected]/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
> > > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > >
> > > >
> > >
>
On Thu, Jul 6, 2023 at 5:39 PM Eugenio Perez Martin <[email protected]> wrote:
>
> On Thu, Jul 6, 2023 at 10:03 AM Jason Wang <[email protected]> wrote:
> >
> > On Thu, Jul 6, 2023 at 3:55 PM Jason Wang <[email protected]> wrote:
> > >
> > > On Thu, Jul 6, 2023 at 3:06 PM Eugenio Perez Martin <[email protected]> wrote:
> > > >
> > > > On Thu, Jul 6, 2023 at 3:55 AM Jason Wang <[email protected]> wrote:
> > > > >
> > > > > On Wed, Jul 5, 2023 at 4:41 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > > >
> > > > > > On Wed, Jul 05, 2023 at 03:49:58PM +0800, Jason Wang wrote:
> > > > > > > On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Tue, Jul 04, 2023 at 01:36:11PM +0200, Eugenio Perez Martin wrote:
> > > > > > > > > On Tue, Jul 4, 2023 at 12:38 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > > > > > > >
> > > > > > > > > > > > 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?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Previous versions of the QEMU LM series did a spurious kick to start
> > > > > > > > > traffic at the LM destination [1]. When it was proposed, that spurious
> > > > > > > > > kick was removed from the series because to check for descriptors
> > > > > > > > > after driver_ok, even without a kick, was considered work of the
> > > > > > > > > parent driver.
> > > > > > > > >
> > > > > > > > > I'm ok to go back to this spurious kick, but I'm not sure if the hw
> > > > > > > > > will read the ring before the kick actually. I can ask.
> > > > > > > > >
> > > > > > > > > Thanks!
> > > > > > > > >
> > > > > > > > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02775.html
> > > > > > > >
> > > > > > > > Let's find out. We need to check for ENABLE_AFTER_DRIVER_OK too, no?
> > > > > > >
> > > > > > > My understanding is [1] assuming ACCESS_AFTER_KICK. This seems
> > > > > > > sub-optimal than assuming ENABLE_AFTER_DRIVER_OK.
> > > > > > >
> > > > > > > But this reminds me one thing, as the thread is going too long, I
> > > > > > > wonder if we simply assume ENABLE_AFTER_DRIVER_OK if RING_RESET is
> > > > > > > supported?
> > > > > > >
> > > > > > > Thanks
> > > > > >
> > > > > > I don't see what does one have to do with another ...
> > > > > >
> > > > > > I think with RING_RESET we had another solution, enable rings
> > > > > > mapping them to a zero page, then reset and re-enable later.
> > > > >
> > > > > As discussed before, this seems to have some problems:
> > > > >
> > > > > 1) The behaviour is not clarified in the document
> > > > > 2) zero is a valid IOVA
> > > > >
> > > >
> > > > I think we're not on the same page here.
> > > >
> > > > As I understood, rings mapped to a zero page means essentially an
> > > > avail ring whose avail_idx is always 0, offered to the device instead
> > > > of the guest's ring. Once all CVQ commands are processed, we use
> > > > RING_RESET to switch to the right ring, being guest's or SVQ vring.
> > >
> > > I get this. This seems more complicated in the destination: shadow vq + ASID?
> >
> > So it's something like:
> >
> > 1) set all vq ASID to shadow virtqueue
> > 2) do not add any bufs to data qp (stick 0 as avail index)
> > 3) start to restore states via cvq
> > 4) ring_rest for dataqp
> > 5) set_vq_state for dataqp
> > 6) re-initialize dataqp address etc
> > 7) set data QP ASID to guest
> > 8) set queue_enable
> >
> > ?
> >
>
> I think the change of ASID is not needed, as the guest cannot access
> the device in that timeframe anyway.
Yes but after the restore, we still want to shadow cvq, so ASID is still needed?
Thanks
> Moreover, it may require HW
> support. So steps 1 and 7 are not needed.
>
> Apart from that, the process is right.
>
>
> > Thanks
> >
> > >
> > > Thanks
> > >
> > > >
> > > >
> > > >
> > > > > 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/[email protected]/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
> > > > > > > > > > > >
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > > >
> > > >
> >
>
On Fri, Jul 7, 2023 at 9:57 AM Jason Wang <[email protected]> wrote:
>
> On Thu, Jul 6, 2023 at 5:39 PM Eugenio Perez Martin <[email protected]> wrote:
> >
> > On Thu, Jul 6, 2023 at 10:03 AM Jason Wang <[email protected]> wrote:
> > >
> > > On Thu, Jul 6, 2023 at 3:55 PM Jason Wang <[email protected]> wrote:
> > > >
> > > > On Thu, Jul 6, 2023 at 3:06 PM Eugenio Perez Martin <[email protected]> wrote:
> > > > >
> > > > > On Thu, Jul 6, 2023 at 3:55 AM Jason Wang <[email protected]> wrote:
> > > > > >
> > > > > > On Wed, Jul 5, 2023 at 4:41 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > > > >
> > > > > > > On Wed, Jul 05, 2023 at 03:49:58PM +0800, Jason Wang wrote:
> > > > > > > > On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, Jul 04, 2023 at 01:36:11PM +0200, Eugenio Perez Martin wrote:
> > > > > > > > > > On Tue, Jul 4, 2023 at 12:38 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > > > > > > > >
> > > > > > > > > > > > > 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?
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Previous versions of the QEMU LM series did a spurious kick to start
> > > > > > > > > > traffic at the LM destination [1]. When it was proposed, that spurious
> > > > > > > > > > kick was removed from the series because to check for descriptors
> > > > > > > > > > after driver_ok, even without a kick, was considered work of the
> > > > > > > > > > parent driver.
> > > > > > > > > >
> > > > > > > > > > I'm ok to go back to this spurious kick, but I'm not sure if the hw
> > > > > > > > > > will read the ring before the kick actually. I can ask.
> > > > > > > > > >
> > > > > > > > > > Thanks!
> > > > > > > > > >
> > > > > > > > > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02775.html
> > > > > > > > >
> > > > > > > > > Let's find out. We need to check for ENABLE_AFTER_DRIVER_OK too, no?
> > > > > > > >
> > > > > > > > My understanding is [1] assuming ACCESS_AFTER_KICK. This seems
> > > > > > > > sub-optimal than assuming ENABLE_AFTER_DRIVER_OK.
> > > > > > > >
> > > > > > > > But this reminds me one thing, as the thread is going too long, I
> > > > > > > > wonder if we simply assume ENABLE_AFTER_DRIVER_OK if RING_RESET is
> > > > > > > > supported?
> > > > > > > >
> > > > > > > > Thanks
> > > > > > >
> > > > > > > I don't see what does one have to do with another ...
> > > > > > >
> > > > > > > I think with RING_RESET we had another solution, enable rings
> > > > > > > mapping them to a zero page, then reset and re-enable later.
> > > > > >
> > > > > > As discussed before, this seems to have some problems:
> > > > > >
> > > > > > 1) The behaviour is not clarified in the document
> > > > > > 2) zero is a valid IOVA
> > > > > >
> > > > >
> > > > > I think we're not on the same page here.
> > > > >
> > > > > As I understood, rings mapped to a zero page means essentially an
> > > > > avail ring whose avail_idx is always 0, offered to the device instead
> > > > > of the guest's ring. Once all CVQ commands are processed, we use
> > > > > RING_RESET to switch to the right ring, being guest's or SVQ vring.
> > > >
> > > > I get this. This seems more complicated in the destination: shadow vq + ASID?
> > >
> > > So it's something like:
> > >
> > > 1) set all vq ASID to shadow virtqueue
> > > 2) do not add any bufs to data qp (stick 0 as avail index)
> > > 3) start to restore states via cvq
> > > 4) ring_rest for dataqp
> > > 5) set_vq_state for dataqp
> > > 6) re-initialize dataqp address etc
> > > 7) set data QP ASID to guest
> > > 8) set queue_enable
> > >
> > > ?
> > >
> >
> > I think the change of ASID is not needed, as the guest cannot access
> > the device in that timeframe anyway.
>
> Yes but after the restore, we still want to shadow cvq, so ASID is still needed?
>
Device or parent driver support for ASID is needed to shadow only CVQ.
Bue the device may not support the switch of ASID after DRIVER_OK.
Since dataplane can go in passthrough ASID all the time, we don't need
to switch it after DRIVER_OK.
> Thanks
>
> > Moreover, it may require HW
> > support. So steps 1 and 7 are not needed.
> >
> > Apart from that, the process is right.
> >
> >
> > > Thanks
> > >
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > >
> > > > >
> > > > > > 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/[email protected]/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
> > > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > >
> >
>
On 7/5/2023 11:07 PM, Michael S. Tsirkin wrote:
> On Wed, Jul 05, 2023 at 05:07:11PM -0700, Shannon Nelson wrote:
>> On 7/5/23 11:27 AM, Eugenio Perez Martin wrote:
>>> On Wed, Jul 5, 2023 at 9:50 AM Jason Wang <[email protected]> wrote:
>>>> On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin <[email protected]> wrote:
>>>>> On Tue, Jul 04, 2023 at 01:36:11PM +0200, Eugenio Perez Martin wrote:
>>>>>> On Tue, Jul 4, 2023 at 12:38 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>>>>>> 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?
>> The PDS vdpa device can deal with a call to .set_vq_ready after DRIVER_OK is
>> set. And I'm told that our VQ activity should start without a kick.
>>
>> Our vdpa device FW doesn't currently have support for VIRTIO_F_RING_RESET,
>> but I believe it could be added without too much trouble.
>>
>> sln
>>
> OK it seems clear at least in the current version pds needs
> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK.
> However can we also code up the RING_RESET path as the default?
What's the rationale of making RING_RESET path the default? Noted this
is on a performance critical path (for live migration downtime), did we
ever get consensus from every or most hardware vendors that RING_RESET
has lower cost in terms of latency overall than ENABLE_AFTER_DRIVER_OK?
I think (RING)RESET in general falls on the slow path for hardware,
while I assume either RING_RESET or ENABLE_AFTER_DRIVER_OK doesn't
matters much on software backed vdpa e.g. vp_vdpa. Maybe should make
ENABLE_AFTER_DRIVER_OK as the default?
-Siwei
> Then down the road vendors can choose what to do.
>
>
>
>
>
>>>>>> Previous versions of the QEMU LM series did a spurious kick to start
>>>>>> traffic at the LM destination [1]. When it was proposed, that spurious
>>>>>> kick was removed from the series because to check for descriptors
>>>>>> after driver_ok, even without a kick, was considered work of the
>>>>>> parent driver.
>>>>>>
>>>>>> I'm ok to go back to this spurious kick, but I'm not sure if the hw
>>>>>> will read the ring before the kick actually. I can ask.
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> [1] https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02775.html
>>>>> Let's find out. We need to check for ENABLE_AFTER_DRIVER_OK too, no?
>>>> My understanding is [1] assuming ACCESS_AFTER_KICK. This seems
>>>> sub-optimal than assuming ENABLE_AFTER_DRIVER_OK.
>>>>
>>>> But this reminds me one thing, as the thread is going too long, I
>>>> wonder if we simply assume ENABLE_AFTER_DRIVER_OK if RING_RESET is
>>>> supported?
>>>>
>>> The problem with that is that the device needs to support all
>>> RING_RESET, like to be able to change vq address etc after DRIVER_OK.
>>> Not all HW support it.
>>>
>>> We just need the subset of having the dataplane freezed until all CVQ
>>> commands have been consumed. I'm sure current vDPA code already
>>> supports it in some devices, like MLX and PSD.
>>>
>>> Thanks!
>>>
>>>> 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/[email protected]/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 mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On Wed, Jul 19, 2023 at 03:20:03PM -0700, Si-Wei Liu wrote:
>
>
> On 7/5/2023 11:07 PM, Michael S. Tsirkin wrote:
> > On Wed, Jul 05, 2023 at 05:07:11PM -0700, Shannon Nelson wrote:
> > > On 7/5/23 11:27 AM, Eugenio Perez Martin wrote:
> > > > On Wed, Jul 5, 2023 at 9:50 AM Jason Wang <[email protected]> wrote:
> > > > > On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > > > On Tue, Jul 04, 2023 at 01:36:11PM +0200, Eugenio Perez Martin wrote:
> > > > > > > On Tue, Jul 4, 2023 at 12:38 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > > > > > 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?
> > > The PDS vdpa device can deal with a call to .set_vq_ready after DRIVER_OK is
> > > set. And I'm told that our VQ activity should start without a kick.
> > >
> > > Our vdpa device FW doesn't currently have support for VIRTIO_F_RING_RESET,
> > > but I believe it could be added without too much trouble.
> > >
> > > sln
> > >
> > OK it seems clear at least in the current version pds needs
> > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK.
> > However can we also code up the RING_RESET path as the default?
> What's the rationale of making RING_RESET path the default? Noted this is on
> a performance critical path (for live migration downtime), did we ever get
> consensus from every or most hardware vendors that RING_RESET has lower cost
> in terms of latency overall than ENABLE_AFTER_DRIVER_OK? I think (RING)RESET
> in general falls on the slow path for hardware, while I assume either
> RING_RESET or ENABLE_AFTER_DRIVER_OK doesn't matters much on software backed
> vdpa e.g. vp_vdpa. Maybe should make ENABLE_AFTER_DRIVER_OK as the default?
>
> -Siwei
Coming from the spec RING_RESET has clearer semantics.
As long as we support it it is not critical which one
is the default though.
> > Then down the road vendors can choose what to do.
> >
> >
> >
> >
> >
> > > > > > > Previous versions of the QEMU LM series did a spurious kick to start
> > > > > > > traffic at the LM destination [1]. When it was proposed, that spurious
> > > > > > > kick was removed from the series because to check for descriptors
> > > > > > > after driver_ok, even without a kick, was considered work of the
> > > > > > > parent driver.
> > > > > > >
> > > > > > > I'm ok to go back to this spurious kick, but I'm not sure if the hw
> > > > > > > will read the ring before the kick actually. I can ask.
> > > > > > >
> > > > > > > Thanks!
> > > > > > >
> > > > > > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02775.html
> > > > > > Let's find out. We need to check for ENABLE_AFTER_DRIVER_OK too, no?
> > > > > My understanding is [1] assuming ACCESS_AFTER_KICK. This seems
> > > > > sub-optimal than assuming ENABLE_AFTER_DRIVER_OK.
> > > > >
> > > > > But this reminds me one thing, as the thread is going too long, I
> > > > > wonder if we simply assume ENABLE_AFTER_DRIVER_OK if RING_RESET is
> > > > > supported?
> > > > >
> > > > The problem with that is that the device needs to support all
> > > > RING_RESET, like to be able to change vq address etc after DRIVER_OK.
> > > > Not all HW support it.
> > > >
> > > > We just need the subset of having the dataplane freezed until all CVQ
> > > > commands have been consumed. I'm sure current vDPA code already
> > > > supports it in some devices, like MLX and PSD.
> > > >
> > > > Thanks!
> > > >
> > > > > 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/[email protected]/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 mailing list
> > [email protected]
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On 7/19/2023 3:26 PM, Michael S. Tsirkin wrote:
> On Wed, Jul 19, 2023 at 03:20:03PM -0700, Si-Wei Liu wrote:
>>
>> On 7/5/2023 11:07 PM, Michael S. Tsirkin wrote:
>>> On Wed, Jul 05, 2023 at 05:07:11PM -0700, Shannon Nelson wrote:
>>>> On 7/5/23 11:27 AM, Eugenio Perez Martin wrote:
>>>>> On Wed, Jul 5, 2023 at 9:50 AM Jason Wang <[email protected]> wrote:
>>>>>> On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin <[email protected]> wrote:
>>>>>>> On Tue, Jul 04, 2023 at 01:36:11PM +0200, Eugenio Perez Martin wrote:
>>>>>>>> On Tue, Jul 4, 2023 at 12:38 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>>>>>>>> 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?
>>>> The PDS vdpa device can deal with a call to .set_vq_ready after DRIVER_OK is
>>>> set. And I'm told that our VQ activity should start without a kick.
>>>>
>>>> Our vdpa device FW doesn't currently have support for VIRTIO_F_RING_RESET,
>>>> but I believe it could be added without too much trouble.
>>>>
>>>> sln
>>>>
>>> OK it seems clear at least in the current version pds needs
>>> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK.
>>> However can we also code up the RING_RESET path as the default?
>> What's the rationale of making RING_RESET path the default? Noted this is on
>> a performance critical path (for live migration downtime), did we ever get
>> consensus from every or most hardware vendors that RING_RESET has lower cost
>> in terms of latency overall than ENABLE_AFTER_DRIVER_OK? I think (RING)RESET
>> in general falls on the slow path for hardware, while I assume either
>> RING_RESET or ENABLE_AFTER_DRIVER_OK doesn't matters much on software backed
>> vdpa e.g. vp_vdpa. Maybe should make ENABLE_AFTER_DRIVER_OK as the default?
>>
>> -Siwei
> Coming from the spec RING_RESET has clearer semantics.
Spec doesn't have clearer semantics on vdpa specifics - such as how does
RING_RESET interoperate with ASID?
> As long as we support it it is not critical which one
> is the default though.
The point is vdpa vendor drivers may implement RING_RESET for a
different purpose than live migration. In case they support both I don't
see a reason why it has to fallback to a slower path given there's a
faster path. May we should leave this to vendor driver to decide, but I
am not sure.
-Siwei
>
>
>>> Then down the road vendors can choose what to do.
>>>
>>>
>>>
>>>
>>>
>>>>>>>> Previous versions of the QEMU LM series did a spurious kick to start
>>>>>>>> traffic at the LM destination [1]. When it was proposed, that spurious
>>>>>>>> kick was removed from the series because to check for descriptors
>>>>>>>> after driver_ok, even without a kick, was considered work of the
>>>>>>>> parent driver.
>>>>>>>>
>>>>>>>> I'm ok to go back to this spurious kick, but I'm not sure if the hw
>>>>>>>> will read the ring before the kick actually. I can ask.
>>>>>>>>
>>>>>>>> Thanks!
>>>>>>>>
>>>>>>>> [1] https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02775.html
>>>>>>> Let's find out. We need to check for ENABLE_AFTER_DRIVER_OK too, no?
>>>>>> My understanding is [1] assuming ACCESS_AFTER_KICK. This seems
>>>>>> sub-optimal than assuming ENABLE_AFTER_DRIVER_OK.
>>>>>>
>>>>>> But this reminds me one thing, as the thread is going too long, I
>>>>>> wonder if we simply assume ENABLE_AFTER_DRIVER_OK if RING_RESET is
>>>>>> supported?
>>>>>>
>>>>> The problem with that is that the device needs to support all
>>>>> RING_RESET, like to be able to change vq address etc after DRIVER_OK.
>>>>> Not all HW support it.
>>>>>
>>>>> We just need the subset of having the dataplane freezed until all CVQ
>>>>> commands have been consumed. I'm sure current vDPA code already
>>>>> supports it in some devices, like MLX and PSD.
>>>>>
>>>>> Thanks!
>>>>>
>>>>>> 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/[email protected]/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 mailing list
>>> [email protected]
>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization