| Message ID | 20231124232400.943580-3-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/5] hw/xen: Set XenBackendInstance in the XenDevice before realizing it | expand |
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?
Hi David, Thank you for the review David Woodhouse <dwmw2@infradead.org> 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@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? 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. > 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. > 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). Nope, I am fine with this approach. I'll remove this hunk in the next version. > >> 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? > 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. Xen toolstack is written in a such way, that it always provides "mac" XenStore entry and frontend driver expects that this entry is always present. So, if by any chance toolstack has not provided the MAC, then there is something wrong and it is better to fail, what xen_net_device_create() does. I am not sure that it is a good idea to try to silently fix any toolstack issues.
On Tue, 2023-11-28 at 01:20 +0000, Volodymyr Babchuk wrote: > Hi David, > > Thank you for the review > > David Woodhouse <dwmw2@infradead.org> 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@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? > > 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.
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c index c2ac9db4a2..dac519a6d3 100644 --- a/hw/block/xen-block.c +++ b/hw/block/xen-block.c @@ -390,13 +390,15 @@ static void xen_block_realize(XenDevice *xendev, Error **errp) xen_device_backend_printf(xendev, "info", "%u", blockdev->info); - xen_device_frontend_printf(xendev, "virtual-device", "%lu", - vdev->number); - xen_device_frontend_printf(xendev, "device-type", "%s", - blockdev->device_type); - - xen_device_backend_printf(xendev, "sector-size", "%u", - conf->logical_block_size); + if (!xendev->backend) { + xen_device_frontend_printf(xendev, "virtual-device", "%lu", + vdev->number); + xen_device_frontend_printf(xendev, "device-type", "%s", + blockdev->device_type); + + xen_device_backend_printf(xendev, "sector-size", "%u", + conf->logical_block_size); + } xen_block_set_size(blockdev); 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); } 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); diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c index dd0171ab98..d0f17aeb27 100644 --- a/hw/xen/xen-bus.c +++ b/hw/xen/xen-bus.c @@ -599,8 +599,10 @@ static void xen_device_backend_destroy(XenDevice *xendev) g_assert(xenbus->xsh); - xs_node_destroy(xenbus->xsh, XBT_NULL, xendev->backend_path, - &local_err); + if (!xendev->backend) { + xs_node_destroy(xenbus->xsh, XBT_NULL, xendev->backend_path, + &local_err); + } g_free(xendev->backend_path); xendev->backend_path = NULL; @@ -764,8 +766,10 @@ static void xen_device_frontend_destroy(XenDevice *xendev) g_assert(xenbus->xsh); - xs_node_destroy(xenbus->xsh, XBT_NULL, xendev->frontend_path, - &local_err); + if (!xendev->backend) { + xs_node_destroy(xenbus->xsh, XBT_NULL, xendev->frontend_path, + &local_err); + } g_free(xendev->frontend_path); xendev->frontend_path = NULL; @@ -1063,7 +1067,7 @@ static void xen_device_realize(DeviceState *dev, Error **errp) xen_device_backend_set_online(xendev, true); xen_device_backend_set_state(xendev, XenbusStateInitWait); - if (!xen_device_frontend_exists(xendev)) { + if (!xen_device_frontend_exists(xendev) && !xendev->backend) { xen_device_frontend_printf(xendev, "backend", "%s", xendev->backend_path); xen_device_frontend_printf(xendev, "backend-id", "%u",
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> --- Changes in v3: - Rephrased the commit message --- hw/block/xen-block.c | 16 +++++++++------- hw/char/xen_console.c | 2 +- hw/net/xen_nic.c | 18 ++++++++++-------- hw/xen/xen-bus.c | 14 +++++++++----- 4 files changed, 29 insertions(+), 21 deletions(-)