Messages in this thread | | | Date | Tue, 4 Jul 2023 08:25:21 +0200 | Subject | Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0 | From | Juergen Gross <> |
| |
On 30.06.23 00:44, Stefano Stabellini wrote: > On Thu, 29 Jun 2023, Oleksandr Tyshchenko wrote: >> On 29.06.23 04:00, Stefano Stabellini wrote: >> >> Hello Stefano >> >>> On Wed, 21 Jun 2023, Oleksandr Tyshchenko wrote: >>>> On 21.06.23 16:12, Petr Pavlu wrote: >>>> >>>> >>>> Hello Petr >>>> >>>> >>>>> When attempting to run Xen on a QEMU/KVM virtual machine with virtio >>>>> devices (all x86_64), dom0 tries to establish a grant for itself which >>>>> eventually results in a hang during the boot. >>>>> >>>>> The backtrace looks as follows, the while loop in __send_control_msg() >>>>> makes no progress: >>>>> >>>>> #0 virtqueue_get_buf_ctx (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94, ctx=ctx@entry=0x0 <fixed_percpu_data>) at ../drivers/virtio/virtio_ring.c:2326 >>>>> #1 0xffffffff817086b7 in virtqueue_get_buf (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94) at ../drivers/virtio/virtio_ring.c:2333 >>>>> #2 0xffffffff8175f6b2 in __send_control_msg (portdev=<optimized out>, port_id=0xffffffff, event=0x0, value=0x1) at ../drivers/char/virtio_console.c:562 >>>>> #3 0xffffffff8175f6ee in __send_control_msg (portdev=<optimized out>, port_id=<optimized out>, event=<optimized out>, value=<optimized out>) at ../drivers/char/virtio_console.c:569 >>>>> #4 0xffffffff817618b1 in virtcons_probe (vdev=0xffff88800585e800) at ../drivers/char/virtio_console.c:2098 >>>>> #5 0xffffffff81707117 in virtio_dev_probe (_d=0xffff88800585e810) at ../drivers/virtio/virtio.c:305 >>>>> #6 0xffffffff8198e348 in call_driver_probe (drv=0xffffffff82be40c0 <virtio_console>, drv=0xffffffff82be40c0 <virtio_console>, dev=0xffff88800585e810) at ../drivers/base/dd.c:579 >>>>> #7 really_probe (dev=dev@entry=0xffff88800585e810, drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:658 >>>>> #8 0xffffffff8198e58f in __driver_probe_device (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:800 >>>>> #9 0xffffffff8198e65a in driver_probe_device (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:830 >>>>> #10 0xffffffff8198e832 in __driver_attach (dev=0xffff88800585e810, data=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1216 >>>>> #11 0xffffffff8198bfb2 in bus_for_each_dev (bus=<optimized out>, start=start@entry=0x0 <fixed_percpu_data>, data=data@entry=0xffffffff82be40c0 <virtio_console>, >>>>> fn=fn@entry=0xffffffff8198e7b0 <__driver_attach>) at ../drivers/base/bus.c:368 >>>>> #12 0xffffffff8198db65 in driver_attach (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1233 >>>>> #13 0xffffffff8198d207 in bus_add_driver (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/bus.c:673 >>>>> #14 0xffffffff8198f550 in driver_register (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/driver.c:246 >>>>> #15 0xffffffff81706b47 in register_virtio_driver (driver=driver@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/virtio/virtio.c:357 >>>>> #16 0xffffffff832cd34b in virtio_console_init () at ../drivers/char/virtio_console.c:2258 >>>>> #17 0xffffffff8100105c in do_one_initcall (fn=0xffffffff832cd2e0 <virtio_console_init>) at ../init/main.c:1246 >>>>> #18 0xffffffff83277293 in do_initcall_level (command_line=0xffff888003e2f900 "root", level=0x6) at ../init/main.c:1319 >>>>> #19 do_initcalls () at ../init/main.c:1335 >>>>> #20 do_basic_setup () at ../init/main.c:1354 >>>>> #21 kernel_init_freeable () at ../init/main.c:1571 >>>>> #22 0xffffffff81f64be1 in kernel_init (unused=<optimized out>) at ../init/main.c:1462 >>>>> #23 0xffffffff81001f49 in ret_from_fork () at ../arch/x86/entry/entry_64.S:308 >>>>> #24 0x0000000000000000 in ?? () >>>>> >>>>> Fix the problem by preventing xen_grant_init_backend_domid() from >>>>> setting dom0 as a backend when running in dom0. >>>>> >>>>> Fixes: 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma" devices") >>>> >>>> >>>> I am not 100% sure whether the Fixes tag points to precise commit. If I >>>> am not mistaken, the said commit just moves the code in the context >>>> without changing the logic of CONFIG_XEN_VIRTIO_FORCE_GRANT, this was >>>> introduced before. >>>> >>>> >>>>> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com> >>>>> --- >>>>> drivers/xen/grant-dma-ops.c | 4 +++- >>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c >>>>> index 76f6f26265a3..29ed27ac450e 100644 >>>>> --- a/drivers/xen/grant-dma-ops.c >>>>> +++ b/drivers/xen/grant-dma-ops.c >>>>> @@ -362,7 +362,9 @@ static int xen_grant_init_backend_domid(struct device *dev, >>>>> if (np) { >>>>> ret = xen_dt_grant_init_backend_domid(dev, np, backend_domid); >>>>> of_node_put(np); >>>>> - } else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || xen_pv_domain()) { >>>>> + } else if ((IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || >>>>> + xen_pv_domain()) && >>>>> + !xen_initial_domain()) { >>>> >>>> The commit lgtm, just one note: >>>> >>>> >>>> I would even bail out early in xen_virtio_restricted_mem_acc() instead, >>>> as I assume the same issue could happen on Arm with DT (although there >>>> we don't guess the backend's domid, we read it from DT and quite >>>> unlikely we get Dom0 being in Dom0 with correct DT). >>>> >>>> Something like: >>>> >>>> @@ -416,6 +421,10 @@ bool xen_virtio_restricted_mem_acc(struct >>>> virtio_device *dev) >>>> { >>>> domid_t backend_domid; >>>> >>>> + /* Xen grant DMA ops are not used when running as initial domain */ >>>> + if (xen_initial_domain()) >>>> + return false; >>>> + >>>> if (!xen_grant_init_backend_domid(dev->dev.parent, >>>> &backend_domid)) { >>>> xen_grant_setup_dma_ops(dev->dev.parent, backend_domid); >>>> return true; >>>> (END) >>>> >>>> >>>> >>>> If so, that commit subject would need to be updated accordingly. >>>> >>>> Let's see what other reviewers will say. >>> >>> This doesn't work in all cases. Imagine using PCI Passthrough to assign >>> a "physical" virtio device to a domU. The domU will run into the same >>> error, right? >>> >>> The problem is that we need a way for the virtio backend to advertise >>> its ability of handling grants. Right now we only have a way to do with >>> that with device tree on ARM. On x86, we only have >>> CONFIG_XEN_VIRTIO_FORCE_GRANT, and if we take >>> CONFIG_XEN_VIRTIO_FORCE_GRANT at face value, it also enables grants for >>> "physical" virtio devices. Note that in this case we are fixing a >>> nested-virtualization bug, but there are actually physical >>> virtio-compatible devices out there. CONFIG_XEN_VIRTIO_FORCE_GRANT will >>> break those too. >> >> >> If these "physical" virtio devices are also spawned by >> drivers/virtio/virtio.c:virtio_dev_probe(), then yes, otherwise I don't >> see how this could even be possible, but I might miss something here. > > Yes, I would imagine virtio_dev_probe() would be called for them too > > > >> xen_virtio_restricted_mem_acc() gets called indirectly from >> virtio_dev_probe()->virtio_features_ok()-> >> virtio_check_mem_acc_cb(). So the Xen grant DMA ops are only installed >> for those. >> >> >>> >>> I think we need to add a second way? It could be anything that can help >>> us distinguish between a non-grants-capable virtio backend and a >>> grants-capable virtio backend, such as: >>> - a string on xenstore >>> - a xen param >>> - a special PCI configuration register value >>> - something in the ACPI tables >>> - the QEMU machine type >> >> >> Yes, I remember there was a discussion regarding that. The point is to >> choose a solution to be functional for both PV and HVM *and* to be able >> to support a hotplug. IIRC, the xenstore could be a possible candidate. > > xenstore would be among the easiest to make work. The only downside is > the dependency on xenstore which otherwise virtio+grants doesn't have.
I'm in favor of using xenstore. Especially the hotplug support would be much easier using xenstore (the alternative would be ACPI, which I don't think we want for PV guests or many Arm configurations).
Juergen [unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature] | |