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

[Stefano Stabellini]

On Sat, 8 Jul 2023, Juergen Gross wrote:
> On 07.07.23 23:02, Stefano Stabellini wrote:
> > On Fri, 7 Jul 2023, Juergen Gross wrote:
> > > On 26.06.23 15:17, Petr Pavlu wrote:
> > > > On 6/21/23 19:58, Oleksandr Tyshchenko wrote:
> > > > > On 21.06.23 16:12, Petr Pavlu wrote:
> > > > > > 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.
> > > > 
> > > > I see, the tag should better point to 7228113d1fa0 ("xen/virtio: use
> > > > dom0 as default backend for CONFIG_XEN_VIRTIO_FORCE_GRANT") which
> > > > introduced the original logic to use dom0 as backend.
> > > > 
> > > > Commit 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma"
> > > > devices") is relevant in sense that it extended when this logic is
> > > > active by adding an OR check for xen_pv_domain().
> > > > 
> > > > > 
> > > > > 
> > > > > > 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.
> > > > 
> > > > Ok, makes sense.
> > > 
> > > I think this is okay for a fix of the current problem.
> > > 
> > > Passing through virtio devices to a PV domU is not covered by this fix,
> > > but
> > > this
> > > should be a rather rare configuration, which doesn't work today either. So
> > > the
> > > suggested patch would fix the current issue without introducing a
> > > regression.
> > > 
> > > Anything else can be done later.
> > 
> > Why do you say that passing through virtio devices to a PV domU doesn't
> > work today anyway? Also, as you know many people use Xen outside of
> > datacenter deployments (laptops, embedded etc.) where drivers domains
> > and device assignment are very common. You could assign a virtio network
> > card to a domU and use PV network to share the network with other
> > guests. Physical virtio devices, especially virtio-net devices, exist. I
> > could probably repro this problem today in a domU just installing
> > QubesOS inside QEMU. QubesOS uses network driver domains and if QEMU
> > provides a virtio-net network card, this would break even with this
> > patch.
> 
> I might be wrong, but I don't think all virtio frontends will work in that
> scenario. The main reason is the PFN/MFN difference: a frontend using guest
> consecutive memory for doing large I/Os will fail miserably. This was the
> main reason why I had to add the functionality of consecutive grants for
> large I/O buffers. The same goes for multi-page virtio ring pages.

I think for PV guests the virtio frontends would work but they might be
slow as they would have to bounce over swiotlb-xen every multi-page
buffer. But the virtio frontends should work OK for PVH and HVM guests.