[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [EXTERNAL] [PATCH v3 2/5] xen: backends: don't overwrite XenStore nodes created by toolstack
On Tue, 2023-11-28 at 01:20 +0000, Volodymyr Babchuk wrote: > Hi David, > > Thank you for the review > > David Woodhouse <dwmw2@xxxxxxxxxxxxx> writes: > > > [[S/MIME Signed Part:Undecided]] > > 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@xxxxxxxxx> > > > Suggested-by: David Woodhouse <dwmw@xxxxxxxxxxxx> > > > Co-Authored-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx> > > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx> > > > > Reviewed-by: David Woodhouse <dwmw@xxxxxxxxxxxx> > > > > ... 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? > > Yeah, AFAIK, console was the first PV driver, so it is a bit strange. > As I see, this frontend entry is used by "xl console" tool to find PTY > device to attach to. So yes, it should be in backend part of the > xenstore. But I don't believe we can fix this right now. Agreed. > > 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? > > Maybe it can lead to a weird situation when user in Dom-0 tries to use > "xl console" command to attach to a console that is absent in Dom-0, > because "tty" entry points to PTY in the driver domain. True, but that possibility exists the moment you have console provided in a driver domain. ... > > 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? > > > > I am not sure that I got this right. conf.maccadr can be sent in two > ways: via xen_net_device_create(), which will fail if toolstack didn't > provided a MAC address, or via qemu_macaddr_default_if_unset(), which is > the case for Xen emulation. The latter happens with hotplug under Xen too, not just Xen emulation. But the key point you make is that xen_net_device_create() will refuse to drive a device which the toolstack created, if the "mac" node isn't present. So creating it for ourselves based on 'if (!xendev->backend)' as you did in your patch is reasonable enough. Thanks. Attachment:
smime.p7s
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |