subject:"\[PATCH\] vdpa\: reject F_ENABLE_AFTER_DRIVER

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

2023-07-19 Thread Si-Wei Liu




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  wrote:

On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin  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  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  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 

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 p

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

2023-07-19 Thread Si-Wei Liu




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  wrote:

On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin  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  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  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 

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/20230609121244-mutt-send-email-...@kernel.org/T/
---
   drivers/vhost/vdpa.c | 7 +--
   1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index e1abf29fed5b..a7e554352351 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -681,18 +681,21 @@ static long vhost_vdpa_unlocked_ioctl(struct

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

2023-07-19 Thread Michael S. Tsirkin

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  wrote:
> > > > > On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin  
> > > > > 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 
> > > > > > >  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 
> > > > > > > > >  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 
> > > > > > > > > > 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 d

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

2023-07-07 Thread Jason Wang

On Thu, Jul 6, 2023 at 5:39 PM Eugenio Perez Martin  wrote:
>
> On Thu, Jul 6, 2023 at 10:03 AM Jason Wang  wrote:
> >
> > On Thu, Jul 6, 2023 at 3:55 PM Jason Wang  wrote:
> > >
> > > On Thu, Jul 6, 2023 at 3:06 PM Eugenio Perez Martin  
> > > wrote:
> > > >
> > > > On Thu, Jul 6, 2023 at 3:55 AM Jason Wang  wrote:
> > > > >
> > > > > On Wed, Jul 5, 2023 at 4:41 PM Michael S. Tsirkin  
> > > > > 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 
> > > > > > >  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 
> > > > > > > > >  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 
> > > > > > > > > > >  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 
> > > > > > > > > > > >
> > > > > > > > > > > > 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

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

2023-07-06 Thread Jason Wang

On Thu, Jul 6, 2023 at 3:55 PM Jason Wang  wrote:
>
> On Thu, Jul 6, 2023 at 3:06 PM Eugenio Perez Martin  
> wrote:
> >
> > On Thu, Jul 6, 2023 at 3:55 AM Jason Wang  wrote:
> > >
> > > On Wed, Jul 5, 2023 at 4:41 PM Michael S. Tsirkin  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  
> > > > > 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 
> > > > > > >  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 
> > > > > > > > >  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 
> > > > > > > > > >
> > > > > > > > > > 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. 
> > > > > > > > > So

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

2023-07-06 Thread Jason Wang

On Thu, Jul 6, 2023 at 3:06 PM Eugenio Perez Martin  wrote:
>
> On Thu, Jul 6, 2023 at 3:55 AM Jason Wang  wrote:
> >
> > On Wed, Jul 5, 2023 at 4:41 PM Michael S. Tsirkin  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  
> > > > 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 
> > > > > >  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 
> > > > > > > >  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 
> > > > > > > > >
> > > > > > > > > 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 di

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

2023-07-05 Thread Michael S. Tsirkin

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  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  wrote:
> > > > >
> > > > > On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin  
> > > > > 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 
> > > > > > >  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 
> > > > > > > > >  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 
> > > > > > > > > >
> > > > > > > > > > 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 v

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

2023-07-05 Thread Michael S. Tsirkin

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  wrote:
> > > 
> > > On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin  
> > > 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  
> > > > > 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 
> > > > > > >  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 
> > > > > > > > 
> > > > > > > > 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,

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

2023-07-05 Thread Michael S. Tsirkin

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  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  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  
> > > > > 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 
> > > > > >
> > > > > > 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/20230609121244-mutt-send-email-...@kernel.org/T/
> > > > > > > ---
> > > > > > >  drivers/vhost/vdpa.c | 7 +--
> > > > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > > > > > > index e1abf29fed5b..a7e554352351 100644
> > > > > > > --- a/drivers/vhost/vdpa.c
> > > > > > > +++ b/drivers/vhost/vdpa.c
> > > > > > > @@ -681,18 +681,21 @@ static long 
> > > > > > > vhost_vdpa_unlocked_ioctl(struct file *filep,
> > > > > > >  {
> > > > > > >   struct vhost_vdpa *v = filep->private_data;
> > > > > > >   struct vhost_dev *d = &v->vdev;
> > > > > > > + const struct vdpa_config_ops *ops = v->vdpa->config;
> > > > > > >   void __user *argp = (void __user *)arg;
> > > > > > >   u64 __user *featurep = argp;
> > > > > > > - u64 features;
> > > > > > > + u64 features, parent_features = 0;
> > > > > > >   long r = 0;
> > > > > > >
> > > > > > >   if (cmd == VHOST_SET_BACKEND_FEATURES) {
> > > > > > >   if (copy_from_user(&features, featurep, 
> > > > > > > sizeof(features)))
> > > > > > >   return -EFAULT;
> > > > > > > + if (ops->get_backend_features)
> > > > > > > + parent_features = 
> > > > > > > ops->get_backend_features(v->vdpa);
> >

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

2023-07-05 Thread Jason Wang

On Wed, Jul 5, 2023 at 4:42 PM Michael S. Tsirkin  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  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  
> > > > 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 
> > > > >
> > > > > Please do send v3. And again, I don't want to send "after driver ok" 
> > > > > hack
> > > > > upstream at all, I merged it in next just to give it some testing.
> > > > > We want RING_ACCESS_AFTER_KICK or some such.
> > > > >
> > > >
> > > > Current devices do not support that semantic.
> > >
> > > Which devices specifically access the ring after DRIVER_OK but before
> > > a kick?
> >
> > Vhost-net is one example at last. It polls a socket as well, so it
> > starts to access the ring immediately after DRIVER_OK.
> >
> > Thanks
>
>
> For sure but that is not vdpa.

Somehow via vp_vdpa that I'm usually testing vDPA patches.

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

Thanks

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

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

2023-07-05 Thread Jason Wang

On Wed, Jul 5, 2023 at 4:41 PM Michael S. Tsirkin  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  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  
> > > > 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  
> > > > > > 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 
> > > > > > >
> > > > > > > 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/20230609121244-mutt-send-email-...@kernel.org/T/
> > > > > > > > ---
> > > > > > > >  drivers/vhost/vdpa.c | 7 +--
> > > > > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > > > > > > > index e1abf29fed5b..a7e554352351 100644
> > > > > > > > --- a/drivers/vhost/vdpa.c
> > > > > > > > +++ b/drivers/vhost/vdp

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

2023-07-05 Thread Shannon Nelson via Virtualization


On 7/5/23 11:27 AM, Eugenio Perez Martin wrote:
>

On Wed, Jul 5, 2023 at 9:50 AM Jason Wang  wrote:


On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin  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  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  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 


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/20230609121244-mutt-send-email-...@kernel.org/T/
---
  drivers/vhost/vdpa.c | 7 +--
  1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index e1abf29fed5b..a7e554352351 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -681,18 +681,21 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
  {
   struct vhost_vdpa *v = filep->private_data;
   struct vhost_dev *d = &v->vdev;
+ const struct vdpa_config_ops *ops = v->vdpa->config;
   void __user *argp = (void __user *)arg;
   u64 __user *featurep = argp;
- u64 features;
+ u64 features, parent_features = 0;
   long r = 0;

   if (cmd == VHOST_SET_BACKEND_FEATURES) {
   if (copy_from_user(&features, featurep, sizeof(features)))
   return -EFAULT;
+ if (ops->get_backend_features)
+ parent_features = ops->get_backend_features(v->vdpa);
   if (features & ~(VHOST_VDPA_BACKEND_FEATURES |
BIT_ULL(VHOST_BACKEND_F_SUSPEND) |
BIT_ULL(VHOST_BACKEND_F_RESUME) |
-  BIT_ULL(VHOST_BACKEN

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

2023-07-05 Thread Michael S. Tsirkin

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  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  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 
> > > >
> > > > Please do send v3. And again, I don't want to send "after driver ok" 
> > > > hack
> > > > upstream at all, I merged it in next just to give it some testing.
> > > > We want RING_ACCESS_AFTER_KICK or some such.
> > > >
> > >
> > > Current devices do not support that semantic.
> >
> > Which devices specifically access the ring after DRIVER_OK but before
> > a kick?
> 
> Vhost-net is one example at last. It polls a socket as well, so it
> starts to access the ring immediately after DRIVER_OK.
> 
> Thanks


For sure but that is not vdpa.

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

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2023-07-05 Thread Michael S. Tsirkin

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  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  
> > > 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  
> > > > > 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 
> > > > > >
> > > > > > 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/20230609121244-mutt-send-email-...@kernel.org/T/
> > > > > > > ---
> > > > > > >  drivers/vhost/vdpa.c | 7 +--
> > > > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > > > > > > index e1abf29fed5b..a7e554352351 100644
> > > > > > > --- a/drivers/vhost/vdpa.c
> > > > > > > +++ b/drivers/vhost/vdpa.c
> > > > > > > @@ -681,18 +681,21 @@ static long 
> > > > > > > vhost_vdpa_unlocked_ioctl(struct file *filep,
> > > > > > >  {
> > > > > > >   struct vhost_vdpa *v = filep->private_data;
> > > > > > >   struct vhost_dev *d = &v->vdev;
> > > > > > > + const struct vdpa_config_ops *ops = v->vdpa->config;
> > > > > > >   void __user *argp = (void __user *)arg;
> > > > > > >   u64 __user *featurep = argp;
> > > > > > > - u64 features;
> > > > > > > + u64 features, pare

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

2023-07-05 Thread Jason Wang

On Tue, Jul 4, 2023 at 6:38 PM Michael S. Tsirkin  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  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 
> > >
> > > 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/20230609121244-mutt-send-email-...@kernel.org/T/
> > > > ---
> > > >  drivers/vhost/vdpa.c | 7 +--
> > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > > > index e1abf29fed5b..a7e554352351 100644
> > > > --- a/drivers/vhost/vdpa.c
> > > > +++ b/drivers/vhost/vdpa.c
> > > > @@ -681,18 +681,21 @@ static long vhost_vdpa_unlocked_ioctl(struct file 
> > > > *filep,
> > > >  {
> > > >   struct vhost_vdpa *v = filep->private_data;
> > > >   struct vhost_dev *d = &v->vdev;
> > > > + const struct vdpa_config_ops *ops = v->vdpa->config;
> > > >   void __user *argp = (void __user *)arg;
> > > >   u64 __user *featurep = argp;
> > > > - u64 features;
> > > > + u64 features, parent_features = 0;
> > > >   long r = 0;
> > > >
> > > >   if (cmd == VHOST_SET_BACKEND_FEATURES) {
> > > >   if (copy_from_user(&features, featurep, sizeof(features)))
> > > >   return -EFAULT;
> > > > + if (ops->get_backend_features)
> > > > + parent_features = 
> > > > ops->get_backend_features(v->vdpa);
> > > >   if (features & ~(VHOST_VDPA_BACKEND_FEATURES |
> > > >BIT_ULL(VHOST_BACKEND_F_SUSPEND) |
> > > >BIT_ULL(VHOST_BACKEND_F_RESUME) |
> > > > -  
> > > > BIT_ULL(VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK)))
> > > > +  parent_features))
> > > >   return -EOPNOTSUPP;
> > > >   if ((features & BIT_ULL(VHOST_BACKEND_F_SUSPEND)) &&
> > > >!vhost_vdpa_can_suspend(v))
> > > > --
> > > > 2.39.3
> > >
>

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2023-07-05 Thread Jason Wang

On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin  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  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  
> > > > 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 
> > > > >
> > > > > 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/20230609121244-mutt-send-email-...@kernel.org/T/
> > > > > > ---
> > > > > >  drivers/vhost/vdpa.c | 7 +--
> > > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > > > > > index e1abf29fed5b..a7e554352351 100644
> > > > > > --- a/drivers/vhost/vdpa.c
> > > > > > +++ b/drivers/vhost/vdpa.c
> > > > > > @@ -681,18 +681,21 @@ static long vhost_vdpa_unlocked_ioctl(struct 
> > > > > > file *filep,
> > > > > >  {
> > > > > >   struct vhost_vdpa *v = filep->private_data;
> > > > > >   struct vhost_dev *d = &v->vdev;
> > > > > > + const struct vdpa_config_ops *ops = v->vdpa->config;
> > > > > >   void __user *argp = (void __user *)arg;
> > > > > >   u64 __user *featurep = argp;
> > > > > > - u64 features;
> > > > > > + u64 features, parent_features = 0;
> > > > > >   long r = 0;
> > > > > >
> > > > > >   if (cmd == VHOST_SET_BACKEND_FEATURES) {
> > > > > >   if (copy_from_user(&features, featurep, 
> > > > > > sizeof(features)))
> > > > > >   return -EFAULT;
> > > > > > + if (ops->get_backend_features)
> > > > > > + parent_features = 
> > > > > > ops->get_backend_features(v->vdpa);
> > > > > >   if (features & ~(VHO

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

2023-07-04 Thread Michael S. Tsirkin

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  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  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 
> > > >
> > > > 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/20230609121244-mutt-send-email-...@kernel.org/T/
> > > > > ---
> > > > >  drivers/vhost/vdpa.c | 7 +--
> > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > > > > index e1abf29fed5b..a7e554352351 100644
> > > > > --- a/drivers/vhost/vdpa.c
> > > > > +++ b/drivers/vhost/vdpa.c
> > > > > @@ -681,18 +681,21 @@ static long vhost_vdpa_unlocked_ioctl(struct 
> > > > > file *filep,
> > > > >  {
> > > > >   struct vhost_vdpa *v = filep->private_data;
> > > > >   struct vhost_dev *d = &v->vdev;
> > > > > + const struct vdpa_config_ops *ops = v->vdpa->config;
> > > > >   void __user *argp = (void __user *)arg;
> > > > >   u64 __user *featurep = argp;
> > > > > - u64 features;
> > > > > + u64 features, parent_features = 0;
> > > > >   long r = 0;
> > > > >
> > > > >   if (cmd == VHOST_SET_BACKEND_FEATURES) {
> > > > >   if (copy_from_user(&features, featurep, 
> > > > > sizeof(features)))
> > > > >   return -EFAULT;
> > > > > + if (ops->get_backend_features)
> > > > > + parent_features = 
> > > > > ops->get_backend_features(v->vdpa);
> > > > >   if (features & ~(VHOST_VDPA_BACKEND_FEATURES |
> > > > >BIT_ULL(VHOST_BACKEND_F_SUSPEND) |
> > > > >BIT_ULL(VHOST_BACKEND_F_RESUME) |
> > > > > -  
> > > > > BIT_ULL(VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK)))
> > > > > +  parent_features))
> > > > >   return -EOPNOTSUPP;
> > > > >   if ((features & BIT_ULL(VHOST_BACKEND_F_SUSPEND)) &&
> > > > >!vhost_vdpa_can_suspend(v))
> > > > > --
> > > > > 2.

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

2023-07-04 Thread Michael S. Tsirkin

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  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 
> >
> > 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/20230609121244-mutt-send-email-...@kernel.org/T/
> > > ---
> > >  drivers/vhost/vdpa.c | 7 +--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > > index e1abf29fed5b..a7e554352351 100644
> > > --- a/drivers/vhost/vdpa.c
> > > +++ b/drivers/vhost/vdpa.c
> > > @@ -681,18 +681,21 @@ static long vhost_vdpa_unlocked_ioctl(struct file 
> > > *filep,
> > >  {
> > >   struct vhost_vdpa *v = filep->private_data;
> > >   struct vhost_dev *d = &v->vdev;
> > > + const struct vdpa_config_ops *ops = v->vdpa->config;
> > >   void __user *argp = (void __user *)arg;
> > >   u64 __user *featurep = argp;
> > > - u64 features;
> > > + u64 features, parent_features = 0;
> > >   long r = 0;
> > >
> > >   if (cmd == VHOST_SET_BACKEND_FEATURES) {
> > >   if (copy_from_user(&features, featurep, sizeof(features)))
> > >   return -EFAULT;
> > > + if (ops->get_backend_features)
> > > + parent_features = 
> > > ops->get_backend_features(v->vdpa);
> > >   if (features & ~(VHOST_VDPA_BACKEND_FEATURES |
> > >BIT_ULL(VHOST_BACKEND_F_SUSPEND) |
> > >BIT_ULL(VHOST_BACKEND_F_RESUME) |
> > > -  
> > > BIT_ULL(VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK)))
> > > +  parent_features))
> > >   return -EOPNOTSUPP;
> > >   if ((features & BIT_ULL(VHOST_BACKEND_F_SUSPEND)) &&
> > >!vhost_vdpa_can_suspend(v))
> > > --
> > > 2.39.3
> >

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2023-07-03 Thread Michael S. Tsirkin

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 

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/20230609121244-mutt-send-email-...@kernel.org/T/
> ---
>  drivers/vhost/vdpa.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index e1abf29fed5b..a7e554352351 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -681,18 +681,21 @@ static long vhost_vdpa_unlocked_ioctl(struct file 
> *filep,
>  {
>   struct vhost_vdpa *v = filep->private_data;
>   struct vhost_dev *d = &v->vdev;
> + const struct vdpa_config_ops *ops = v->vdpa->config;
>   void __user *argp = (void __user *)arg;
>   u64 __user *featurep = argp;
> - u64 features;
> + u64 features, parent_features = 0;
>   long r = 0;
>  
>   if (cmd == VHOST_SET_BACKEND_FEATURES) {
>   if (copy_from_user(&features, featurep, sizeof(features)))
>   return -EFAULT;
> + if (ops->get_backend_features)
> + parent_features = ops->get_backend_features(v->vdpa);
>   if (features & ~(VHOST_VDPA_BACKEND_FEATURES |
>BIT_ULL(VHOST_BACKEND_F_SUSPEND) |
>BIT_ULL(VHOST_BACKEND_F_RESUME) |
> -  
> BIT_ULL(VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK)))
> +  parent_features))
>   return -EOPNOTSUPP;
>   if ((features & BIT_ULL(VHOST_BACKEND_F_SUSPEND)) &&
>!vhost_vdpa_can_suspend(v))
> -- 
> 2.39.3

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

19 matches

Site Navigation

Mail list logo

Footer information