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

[Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>]

On 07.07.23 11:11, Juergen Gross wrote:

Hello Juergen


> 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.

Indeed, I missed that possible scenario. I agree with the explanations, 
thanks.


> 
> 
> Juergen
>