| Message ID | 20231121221023.419901-3-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/6] hw/xen: Set XenBackendInstance in the XenDevice before realizing it | expand |
Hi Volodymyr, On 21/11/23 23:10, Volodymyr Babchuk wrote: > was created by QEMU Please do not split lines between subject and content. Rewrite the full line. Preferably restrict the subject to 72 chars. Otherwise your patch isn't displayed correctly in git tools. Thanks, Phil. > 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 is acting as a backend (i.e. it 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> > --- > 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(-)
On 21/11/2023 22:10, Volodymyr Babchuk wrote: > was created by QEMU > > 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 is acting as a backend (i.e. it was configured by Xen Technally *all* XenDevice objects are backends. > 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. >
On Wed, 2023-11-22 at 17:03 +0000, Paul Durrant wrote: > > > This patch checks presence of xendev->backend to check if Xen PV > > device is acting as a backend (i.e. it was configured by Xen > > Technally *all* XenDevice objects are backends. Right. The key criterion is whether the backend was created by QEMU, or merely discovered by QEMU after the toolstack created it. Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.
Hi Philippe, Philippe Mathieu-Daudé <philmd@linaro.org> writes: > Hi Volodymyr, > > On 21/11/23 23:10, Volodymyr Babchuk wrote: >> was created by QEMU > > Please do not split lines between subject and content. Rewrite the > full line. Preferably restrict the subject to 72 chars. I tried to come with shorter description, but failed. I'll rewrite it in the next version.
Hi Paul, Paul Durrant <xadimgnik@gmail.com> writes: > On 21/11/2023 22:10, Volodymyr Babchuk wrote: >> was created by QEMU >> 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 is acting as a backend (i.e. it was configured by Xen > > Technally *all* XenDevice objects are backends. > Yes, you are right of course. I'll rephrase this paragraph in the next version.
On Wed, 2023-11-22 at 22:49 +0000, Volodymyr Babchuk wrote: > > > On 21/11/23 23:10, Volodymyr Babchuk wrote: > > > was created by QEMU > > > > Please do not split lines between subject and content. Rewrite the > > full line. Preferably restrict the subject to 72 chars. > > I tried to come with shorter description, but failed. I'll rewrite it in > the next version. Even if you just put those last four words *into* the subject, it's only 75 characters once the leaving [PATCH...] is stripped. xen: backends: touch some XenStore nodes only if device was created by QEMU And this would be 9 characters fewer: xen: backends: don't overwrite XenStore nodes created by toolstack
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",
was created by QEMU 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 is acting as a backend (i.e. it 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> --- 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(-)