[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [EXTERNAL] [PATCH v3 2/5] xen: backends: don't overwrite XenStore no
From: |
David Woodhouse |
Subject: |
Re: [EXTERNAL] [PATCH v3 2/5] xen: backends: don't overwrite XenStore nodes created by toolstack |
Date: |
Mon, 27 Nov 2023 08:35:42 +0000 |
User-agent: |
Evolution 3.44.4-0ubuntu2 |
On Fri, 2023-11-24 at 23:24 +0000, Volodymyr Babchuk wrote:
> Xen PV devices in QEMU can be created in two ways: either by QEMU
> itself, if they were passed via command line, or by Xen toolstack. In
> the latter case, QEMU scans XenStore entries and configures devices
> accordingly.
>
> In the second case we don't want QEMU to write/delete front-end
> entries for two reasons: it might have no access to those entries if
> it is running in un-privileged domain and it is just incorrect to
> overwrite entries already provided by Xen toolstack, because
> toolstack
> manages those nodes. For example, it might read backend- or frontend-
> state to be sure that they are both disconnected and it is safe to
> destroy a domain.
>
> This patch checks presence of xendev->backend to check if Xen PV
> device was configured by Xen toolstack to decide if it should touch
> frontend entries in XenStore. Also, when we need to remove XenStore
> entries during device teardown only if they weren't created by Xen
> toolstack. If they were created by toolstack, then it is toolstack's
> job to do proper clean-up.
>
> Suggested-by: Paul Durrant <xadimgnik@gmail.com>
> Suggested-by: David Woodhouse <dwmw@amazon.co.uk>
> Co-Authored-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
... albeit with a couple of suggestions...
> diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> index bef8a3a621..b52ddddabf 100644
> --- a/hw/char/xen_console.c
> +++ b/hw/char/xen_console.c
> @@ -450,7 +450,7 @@ static void xen_console_realize(XenDevice *xendev, Error
> **errp)
>
> trace_xen_console_realize(con->dev, object_get_typename(OBJECT(cs)));
>
> - if (CHARDEV_IS_PTY(cs)) {
> + if (CHARDEV_IS_PTY(cs) && !xendev->backend) {
> /* Strip the leading 'pty:' */
> xen_device_frontend_printf(xendev, "tty", "%s", cs->filename + 4);
> }
It's kind of weird that that one is a frontend node at all; surely it
should have been a backend node? But it is known only to QEMU once it
actually opens /dev/ptmx and creates a new pty. It can't be populated
by the toolstack in advance.
So shouldn't the toolstack have made it writable by the driver domain?
I think we should attempt to write this and just gracefully handle the
failure if we can't. (In fact, xen_device_frontend_printf() will just
use error_report_err() which is probably OK unless you feel strongly
about silencing it).
> diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
> index afa10c96e8..27442bef38 100644
> --- a/hw/net/xen_nic.c
> +++ b/hw/net/xen_nic.c
> @@ -315,14 +315,16 @@ static void xen_netdev_realize(XenDevice *xendev, Error
> **errp)
>
> qemu_macaddr_default_if_unset(&netdev->conf.macaddr);
>
> - xen_device_frontend_printf(xendev, "mac",
> "%02x:%02x:%02x:%02x:%02x:%02x",
> - netdev->conf.macaddr.a[0],
> - netdev->conf.macaddr.a[1],
> - netdev->conf.macaddr.a[2],
> - netdev->conf.macaddr.a[3],
> - netdev->conf.macaddr.a[4],
> - netdev->conf.macaddr.a[5]);
> -
> + if (!xendev->backend) {
> + xen_device_frontend_printf(xendev, "mac",
> + "%02x:%02x:%02x:%02x:%02x:%02x",
> + netdev->conf.macaddr.a[0],
> + netdev->conf.macaddr.a[1],
> + netdev->conf.macaddr.a[2],
> + netdev->conf.macaddr.a[3],
> + netdev->conf.macaddr.a[4],
> + netdev->conf.macaddr.a[5]);
> + }
> netdev->nic = qemu_new_nic(&net_xen_info, &netdev->conf,
> object_get_typename(OBJECT(xendev)),
> DEVICE(xendev)->id, netdev);
Perhaps here you should create the "mac" node if it doesn't exist (or
is that "if it doesn't match netdev->conf.macaddr"?) and just
gracefully accept failure too?
smime.p7s
Description: S/MIME cryptographic signature