Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Oleksandr Tyshchenko <Oleksandr_Tyshchenko@epam.com>]

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.





>   		dev_info(dev, "Using dom0 as backend\n");
>   		*backend_domid = 0;
>   		ret = 0;