[PATCH] um: virt-pci: set device ready in probe()

[PATCH] um: virt-pci: set device ready in probe()

[Vincent Whitchurch]

Call virtio_device_ready() to make this driver work after commit
b4ec69d7e09 ("virtio: harden vring IRQ"), since the driver uses the
virtqueues in the probe function.  (The virtio core sets the device
ready when probe returns.)

Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ")
Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
 arch/um/drivers/virt-pci.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/um/drivers/virt-pci.c b/arch/um/drivers/virt-pci.c
index 5c092a9153ea..027847023184 100644
--- a/arch/um/drivers/virt-pci.c
+++ b/arch/um/drivers/virt-pci.c
@@ -544,6 +544,8 @@ static int um_pci_init_vqs(struct um_pci_device *dev)
 	dev->cmd_vq = vqs[0];
 	dev->irq_vq = vqs[1];
 
+	virtio_device_ready(dev->vdev);
+
 	for (i = 0; i < NUM_IRQ_MSGS; i++) {
 		void *msg = kzalloc(MAX_IRQ_MSG_SIZE, GFP_KERNEL);
 
@@ -587,7 +589,7 @@ static int um_pci_virtio_probe(struct virtio_device *vdev)
 	dev->irq = irq_alloc_desc(numa_node_id());
 	if (dev->irq < 0) {
 		err = dev->irq;
-		goto error;
+		goto err_reset;
 	}
 	um_pci_devices[free].dev = dev;
 	vdev->priv = dev;
@@ -604,6 +606,9 @@ static int um_pci_virtio_probe(struct virtio_device *vdev)
 
 	um_pci_rescan();
 	return 0;
+err_reset:
+	virtio_reset_device(vdev);
+	vdev->config->del_vqs(vdev);
 error:
 	mutex_unlock(&um_pci_mtx);
 	kfree(dev);
-- 
2.34.1

Re: [PATCH] um: virt-pci: set device ready in probe()

[Johannes Berg]

On Fri, 2022-06-10 at 17:12 +0200, Vincent Whitchurch wrote:
> Call virtio_device_ready() to make this driver work after commit
> b4ec69d7e09 ("virtio: harden vring IRQ"), since the driver uses the
> virtqueues in the probe function.  (The virtio core sets the device
> ready when probe returns.)
> 
> Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ")
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>

Thank you! I had apparently run into this a few days ago, but hadn't had
a chance to debug it yet. This patch fixes my test setup on 5.19-rc1.

Tested-by: Johannes Berg <johannes@sipsolutions.net>

johannes

Re: [PATCH] um: virt-pci: set device ready in probe()

[Michael S. Tsirkin]

On Fri, Jun 10, 2022 at 05:12:03PM +0200, Vincent Whitchurch wrote:
> Call virtio_device_ready() to make this driver work after commit
> b4ec69d7e09 ("virtio: harden vring IRQ"), since the driver uses the
> virtqueues in the probe function.  (The virtio core sets the device
> ready when probe returns.)
> 
> Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ")
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>

Also fixes this commit:

commit 68f5d3f3b6543266b29e047cfaf9842333019b4c
Author: Johannes Berg <johannes.berg@intel.com>
Date:   Fri Mar 5 13:19:58 2021 +0100

    um: add PCI over virtio emulation driver
    

BTW Johannes I think you need to spec this device and get
an ID - what's the plan for that? Current hack of punting
this to userspace isn't really any good long term.


> ---
>  arch/um/drivers/virt-pci.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/um/drivers/virt-pci.c b/arch/um/drivers/virt-pci.c
> index 5c092a9153ea..027847023184 100644
> --- a/arch/um/drivers/virt-pci.c
> +++ b/arch/um/drivers/virt-pci.c
> @@ -544,6 +544,8 @@ static int um_pci_init_vqs(struct um_pci_device *dev)
>  	dev->cmd_vq = vqs[0];
>  	dev->irq_vq = vqs[1];
>  
> +	virtio_device_ready(dev->vdev);
> +
>  	for (i = 0; i < NUM_IRQ_MSGS; i++) {
>  		void *msg = kzalloc(MAX_IRQ_MSG_SIZE, GFP_KERNEL);
>  
> @@ -587,7 +589,7 @@ static int um_pci_virtio_probe(struct virtio_device *vdev)
>  	dev->irq = irq_alloc_desc(numa_node_id());
>  	if (dev->irq < 0) {
>  		err = dev->irq;
> -		goto error;
> +		goto err_reset;
>  	}
>  	um_pci_devices[free].dev = dev;
>  	vdev->priv = dev;
> @@ -604,6 +606,9 @@ static int um_pci_virtio_probe(struct virtio_device *vdev)
>  
>  	um_pci_rescan();
>  	return 0;
> +err_reset:
> +	virtio_reset_device(vdev);
> +	vdev->config->del_vqs(vdev);
>  error:
>  	mutex_unlock(&um_pci_mtx);
>  	kfree(dev);
> -- 
> 2.34.1

Re: [PATCH] um: virt-pci: set device ready in probe()

[Johannes Berg]

On Fri, 2022-06-10 at 20:34 -0400, Michael S. Tsirkin wrote:
> 
> Also fixes this commit:
> 
> commit 68f5d3f3b6543266b29e047cfaf9842333019b4c
> Author: Johannes Berg <johannes.berg@intel.com>
> Date:   Fri Mar 5 13:19:58 2021 +0100
> 
>     um: add PCI over virtio emulation driver

Hm, why? It worked before the harden change.

> BTW Johannes I think you need to spec this device and get
> an ID - what's the plan for that? Current hack of punting
> this to userspace isn't really any good long term.

Yeah, agree, it dropped off my radar (and the process is a bit
cumbersome IMHO).

But I'm not quite sure what you mean wrt. "punting to userspace", here
in the virt-pci code I'm punting to the Kconfig :-)

Did you just mix that up, or was there some additional userspace thing
you're thinking of?

The only userspace thing I can think of it is in virtio_uml where you
have the ID on the command-line, but that's because it implements the
virtio device bus over vhost-user which doesn't have ID discoverability
in the protocol. That could also be fixed I guess, but it's a bit of a
chicken & egg problem, if you don't have the ID and discovering it were
not supported, you'd end up with an unusable device unless you specified
the ID, in which case you don't need to discover it...

johannes

Re: [PATCH] um: virt-pci: set device ready in probe()

[Michael S. Tsirkin]

On Sun, Jun 12, 2022 at 10:58:20AM +0200, Johannes Berg wrote:
> On Fri, 2022-06-10 at 20:34 -0400, Michael S. Tsirkin wrote:
> > 
> > Also fixes this commit:
> > 
> > commit 68f5d3f3b6543266b29e047cfaf9842333019b4c
> > Author: Johannes Berg <johannes.berg@intel.com>
> > Date:   Fri Mar 5 13:19:58 2021 +0100
> > 
> >     um: add PCI over virtio emulation driver
> 
> Hm, why? It worked before the harden change.

Worked on a specific hypervisor. It was out of spec though.

> > BTW Johannes I think you need to spec this device and get
> > an ID - what's the plan for that? Current hack of punting
> > this to userspace isn't really any good long term.
> 
> Yeah, agree, it dropped off my radar (and the process is a bit
> cumbersome IMHO).

Hmm.

So right the recommended way is:
post patch
open github issue
send an email requesting vote

I guess we can switch request for vote to the github issue
to streamline this a bit - do you think that will help?




> But I'm not quite sure what you mean wrt. "punting to userspace", here
> in the virt-pci code I'm punting to the Kconfig :-)
> 
> Did you just mix that up, or was there some additional userspace thing
> you're thinking of?

Right, I meant "punting it to the user".

> The only userspace thing I can think of it is in virtio_uml where you
> have the ID on the command-line, but that's because it implements the
> virtio device bus over vhost-user which doesn't have ID discoverability
> in the protocol. That could also be fixed I guess, but it's a bit of a
> chicken & egg problem, if you don't have the ID and discovering it were
> not supported, you'd end up with an unusable device unless you specified
> the ID, in which case you don't need to discover it...
> 
> johannes


So, please start by reserving an id.

-- 
MST

Re: [PATCH] um: virt-pci: set device ready in probe()

[Vincent Whitchurch]

On Sat, Jun 11, 2022 at 02:34:04AM +0200, Michael S. Tsirkin wrote:
> BTW Johannes I think you need to spec this device and get
> an ID - what's the plan for that? Current hack of punting
> this to userspace isn't really any good long term.

There is work ongoing in QEMU to add a new vfio-user protocol to support
out-of-process PCI device emulation.  Perhaps an option would be to use
that protocol instead in UML too in the future?

Re: [PATCH] um: virt-pci: set device ready in probe()

[Jason Wang]

On Fri, Jun 10, 2022 at 11:12 PM Vincent Whitchurch
<vincent.whitchurch@axis.com> wrote:
>
> Call virtio_device_ready() to make this driver work after commit
> b4ec69d7e09 ("virtio: harden vring IRQ"), since the driver uses the
> virtqueues in the probe function.  (The virtio core sets the device
> ready when probe returns.)
>
> Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ")
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> ---
>  arch/um/drivers/virt-pci.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/um/drivers/virt-pci.c b/arch/um/drivers/virt-pci.c
> index 5c092a9153ea..027847023184 100644
> --- a/arch/um/drivers/virt-pci.c
> +++ b/arch/um/drivers/virt-pci.c
> @@ -544,6 +544,8 @@ static int um_pci_init_vqs(struct um_pci_device *dev)
>         dev->cmd_vq = vqs[0];
>         dev->irq_vq = vqs[1];
>
> +       virtio_device_ready(dev->vdev);
> +
>         for (i = 0; i < NUM_IRQ_MSGS; i++) {
>                 void *msg = kzalloc(MAX_IRQ_MSG_SIZE, GFP_KERNEL);
>
> @@ -587,7 +589,7 @@ static int um_pci_virtio_probe(struct virtio_device *vdev)
>         dev->irq = irq_alloc_desc(numa_node_id());
>         if (dev->irq < 0) {
>                 err = dev->irq;
> -               goto error;
> +               goto err_reset;
>         }
>         um_pci_devices[free].dev = dev;
>         vdev->priv = dev;
> @@ -604,6 +606,9 @@ static int um_pci_virtio_probe(struct virtio_device *vdev)
>
>         um_pci_rescan();
>         return 0;
> +err_reset:
> +       virtio_reset_device(vdev);
> +       vdev->config->del_vqs(vdev);

This part seems to be an independent fix.

Thanks

>  error:
>         mutex_unlock(&um_pci_mtx);
>         kfree(dev);
> --
> 2.34.1
>

Re: [PATCH] um: virt-pci: set device ready in probe()

[Michael S. Tsirkin]

On Mon, Jun 13, 2022 at 02:58:40PM +0800, Jason Wang wrote:
> On Fri, Jun 10, 2022 at 11:12 PM Vincent Whitchurch
> <vincent.whitchurch@axis.com> wrote:
> >
> > Call virtio_device_ready() to make this driver work after commit
> > b4ec69d7e09 ("virtio: harden vring IRQ"), since the driver uses the
> > virtqueues in the probe function.  (The virtio core sets the device
> > ready when probe returns.)
> >
> > Fixes: 8b4ec69d7e09 ("virtio: harden vring IRQ")
> > Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> > ---
> >  arch/um/drivers/virt-pci.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/um/drivers/virt-pci.c b/arch/um/drivers/virt-pci.c
> > index 5c092a9153ea..027847023184 100644
> > --- a/arch/um/drivers/virt-pci.c
> > +++ b/arch/um/drivers/virt-pci.c
> > @@ -544,6 +544,8 @@ static int um_pci_init_vqs(struct um_pci_device *dev)
> >         dev->cmd_vq = vqs[0];
> >         dev->irq_vq = vqs[1];
> >
> > +       virtio_device_ready(dev->vdev);
> > +
> >         for (i = 0; i < NUM_IRQ_MSGS; i++) {
> >                 void *msg = kzalloc(MAX_IRQ_MSG_SIZE, GFP_KERNEL);
> >
> > @@ -587,7 +589,7 @@ static int um_pci_virtio_probe(struct virtio_device *vdev)
> >         dev->irq = irq_alloc_desc(numa_node_id());
> >         if (dev->irq < 0) {
> >                 err = dev->irq;
> > -               goto error;
> > +               goto err_reset;
> >         }
> >         um_pci_devices[free].dev = dev;
> >         vdev->priv = dev;
> > @@ -604,6 +606,9 @@ static int um_pci_virtio_probe(struct virtio_device *vdev)
> >
> >         um_pci_rescan();
> >         return 0;
> > +err_reset:
> > +       virtio_reset_device(vdev);
> > +       vdev->config->del_vqs(vdev);
> 
> This part seems to be an independent fix.
> 
> Thanks


Yes good point. I merged as is, oh well.

> >  error:
> >         mutex_unlock(&um_pci_mtx);
> >         kfree(dev);
> > --
> > 2.34.1
> >

Re: [PATCH] um: virt-pci: set device ready in probe()

[Vincent Whitchurch]

On Mon, Jun 13, 2022 at 08:58:40AM +0200, Jason Wang wrote:
> On Fri, Jun 10, 2022 at 11:12 PM Vincent Whitchurch <vincent.whitchurch@axis.com> wrote:
> > diff --git a/arch/um/drivers/virt-pci.c b/arch/um/drivers/virt-pci.c
> > index 5c092a9153ea..027847023184 100644
> > --- a/arch/um/drivers/virt-pci.c
> > +++ b/arch/um/drivers/virt-pci.c
> > @@ -544,6 +544,8 @@ static int um_pci_init_vqs(struct um_pci_device *dev)
> >         dev->cmd_vq = vqs[0];
> >         dev->irq_vq = vqs[1];
> >
> > +       virtio_device_ready(dev->vdev);
> > +
> >         for (i = 0; i < NUM_IRQ_MSGS; i++) {
> >                 void *msg = kzalloc(MAX_IRQ_MSG_SIZE, GFP_KERNEL);
> >
> > @@ -587,7 +589,7 @@ static int um_pci_virtio_probe(struct virtio_device *vdev)
> >         dev->irq = irq_alloc_desc(numa_node_id());
> >         if (dev->irq < 0) {
> >                 err = dev->irq;
> > -               goto error;
> > +               goto err_reset;
> >         }
> >         um_pci_devices[free].dev = dev;
> >         vdev->priv = dev;
> > @@ -604,6 +606,9 @@ static int um_pci_virtio_probe(struct virtio_device *vdev)
> >
> >         um_pci_rescan();
> >         return 0;
> > +err_reset:
> > +       virtio_reset_device(vdev);
> > +       vdev->config->del_vqs(vdev);
> 
> This part seems to be an independent fix.

Yes, I guess that last line could have been added in a separate patch.
When adding the cleanup in the error path for the virtio_device_ready()
call added by the patch I noticed that this driver wasn't deleting its
virtqueues in the error path, unlike other virtio drivers I looked at.