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@xxxxxxxx>
---
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.
Attachment:
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature