| Message ID | 20220610151203.3492541-1-vincent.whitchurch@axis.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | um: virt-pci: set device ready in probe() | expand |
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
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
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
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.
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?
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 >
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 > >
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.
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);
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(-)