[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0
On 07.07.23 10:00, Oleksandr Tyshchenko wrote:
On 07.07.23 10:04, Juergen Gross wrote:
Hello Juergen
Re-reading the whole thread again ...
On 29.06.23 03:00, Stefano Stabellini wrote:
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.
In case you want virtio device passthrough, you shouldn't use a kernel
built with CONFIG_XEN_VIRTIO_FORCE_GRANT.
And supporting passing through virtio devices of the host to pv-domUs is
a security risk anyway.
We _could_ drop the requirement of the backend needing to set
VIRTIO_F_ACCESS_PLATFORM for PV guests and allow grant-less virtio
handling for all guests. For this to work xen_virtio_restricted_mem_acc()
would need to check for VIRTIO_F_ACCESS_PLATFORM and return true if set.
Maybe we'd want to enable that possibility via a boot parameter?
Maybe, yes. I don't see at the moment why this won't work.
At the same time I wonder, could we just modify xen_pv_init_platform()
to call virtio_no_restricted_mem_acc() if forcibly disabled by boot
parameter irrespective of VIRTIO_F_ACCESS_PLATFORM presence?
This wouldn't work for the case where a host virtio device is passed through
to the pv domU and at the same time another virtio device is using dom0 as a
backend. I think we should use grants if possible.
Juergen
Attachment:
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
|
