Re: [EXTERNAL] [PATCH v3 2/5] xen: backends: don't overwrite XenStore nodes created by toolstack

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 4738 bytes --]

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?




[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]