Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation
On Tue, 2023-04-04 at 18:45 +0100, Peter Maydell wrote:
> On Tue, 4 Apr 2023 at 18:45, David Woodhouse
> wrote:
> >
> > On Tue, 2023-04-04 at 18:35 +0100, Peter Maydell wrote:
> > > On Tue, 7 Mar 2023 at 18:27, David Woodhouse
> > >
> > > wrote:
> > > >
> > > > From: Paul Durrant
> > > >
> > > > Signed-off-by: Paul Durrant
> > > > Signed-off-by: David Woodhouse
> > > > Reviewed-by: Paul Durrant
> > > > ---
> > >
> > > Hi; Coverity points out a memory leak in this code (CID 1508098):
> > >
> > > > +static struct qemu_xs_handle *libxenstore_open(void)
> > > > +{
> > > > + struct xs_handle *xsh = xs_open(0);
> > > > + struct qemu_xs_handle *h = g_new0(struct qemu_xs_handle,
> > > > 1);
> > >
> > > Here we allocate memory...
> > >
> > > > +
> > > > + if (!xsh) {
> > > > + return NULL;
> > >
> > > ...but here we can return without freeing it...
> > >
> > > > + }
> > > > +
> > > > + h = g_new0(struct qemu_xs_handle, 1);
> > >
> > > ...and here we allocate a second time and overwrite the
> > > pointer to the first allocation.
> > >
> > > Deleting the first call to g_new0() would fix both of these.
> >
> > Indeed, thanks. Do you want a
> >
> > Reviewed-by: David Woodhouse
> >
> > or would you prefer me to submit the actual patch as described?
>
> If you could submit the patch that would be easiest -- you're in
> a better position to test it.
I've been getting Paul to test the parts on actual Xen, so I'll send it
and let him test.
smime.p7s
Description: S/MIME cryptographic signature
Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation
On Tue, 4 Apr 2023 at 18:45, David Woodhouse wrote:
>
> On Tue, 2023-04-04 at 18:35 +0100, Peter Maydell wrote:
> > On Tue, 7 Mar 2023 at 18:27, David Woodhouse
> > wrote:
> > >
> > > From: Paul Durrant
> > >
> > > Signed-off-by: Paul Durrant
> > > Signed-off-by: David Woodhouse
> > > Reviewed-by: Paul Durrant
> > > ---
> >
> > Hi; Coverity points out a memory leak in this code (CID 1508098):
> >
> > > +static struct qemu_xs_handle *libxenstore_open(void)
> > > +{
> > > +struct xs_handle *xsh = xs_open(0);
> > > +struct qemu_xs_handle *h = g_new0(struct qemu_xs_handle, 1);
> >
> > Here we allocate memory...
> >
> > > +
> > > +if (!xsh) {
> > > +return NULL;
> >
> > ...but here we can return without freeing it...
> >
> > > +}
> > > +
> > > +h = g_new0(struct qemu_xs_handle, 1);
> >
> > ...and here we allocate a second time and overwrite the
> > pointer to the first allocation.
> >
> > Deleting the first call to g_new0() would fix both of these.
>
> Indeed, thanks. Do you want a
>
> Reviewed-by: David Woodhouse
>
> or would you prefer me to submit the actual patch as described?
If you could submit the patch that would be easiest -- you're in
a better position to test it.
thanks
-- PMM
Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation
On Tue, 2023-04-04 at 18:35 +0100, Peter Maydell wrote:
> On Tue, 7 Mar 2023 at 18:27, David Woodhouse
> wrote:
> >
> > From: Paul Durrant
> >
> > Signed-off-by: Paul Durrant
> > Signed-off-by: David Woodhouse
> > Reviewed-by: Paul Durrant
> > ---
>
> Hi; Coverity points out a memory leak in this code (CID 1508098):
>
> > +static struct qemu_xs_handle *libxenstore_open(void)
> > +{
> > + struct xs_handle *xsh = xs_open(0);
> > + struct qemu_xs_handle *h = g_new0(struct qemu_xs_handle, 1);
>
> Here we allocate memory...
>
> > +
> > + if (!xsh) {
> > + return NULL;
>
> ...but here we can return without freeing it...
>
> > + }
> > +
> > + h = g_new0(struct qemu_xs_handle, 1);
>
> ...and here we allocate a second time and overwrite the
> pointer to the first allocation.
>
> Deleting the first call to g_new0() would fix both of these.
Indeed, thanks. Do you want a
Reviewed-by: David Woodhouse
or would you prefer me to submit the actual patch as described?
smime.p7s
Description: S/MIME cryptographic signature
Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation
On Tue, 7 Mar 2023 at 18:27, David Woodhouse wrote:
>
> From: Paul Durrant
>
> Signed-off-by: Paul Durrant
> Signed-off-by: David Woodhouse
> Reviewed-by: Paul Durrant
> ---
Hi; Coverity points out a memory leak in this code (CID 1508098):
> +static struct qemu_xs_handle *libxenstore_open(void)
> +{
> +struct xs_handle *xsh = xs_open(0);
> +struct qemu_xs_handle *h = g_new0(struct qemu_xs_handle, 1);
Here we allocate memory...
> +
> +if (!xsh) {
> +return NULL;
...but here we can return without freeing it...
> +}
> +
> +h = g_new0(struct qemu_xs_handle, 1);
...and here we allocate a second time and overwrite the
pointer to the first allocation.
Deleting the first call to g_new0() would fix both of these.
> +h->xsh = xsh;
> +
> +notifier_list_init(>notifiers);
> +qemu_set_fd_handler(xs_fileno(h->xsh), watch_event, NULL, h);
> +
> +return h;
> +}
thanks
-- PMM
Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation
On Mon, 2023-03-13 at 19:17 -0400, Jason Andryuk wrote:
> This looks good, better than what I posted, and seems to work for both
> dm_restrict set and unset.
Thanks.
> For dm_restricted, xs_write() does fail. I verified that with a print
> statement. I think "shouldn't even try" makes sense. I'm thinking
> that xen_domid_restricted shouldn't even add the callback. Something
> like:
>
> --- a/accel/xen/xen-all.c
> +++ b/accel/xen/xen-all.c
> @@ -39,8 +39,7 @@ static void xenstore_record_dm_state(const char *state)
> * This call may fail when running restricted so don't make it fatal in
> * that case. Toolstacks should instead use QMP to listen for state
> changes.
> */
> - if (!qemu_xen_xs_write(xenstore, XBT_NULL, path, state, strlen(state)) &&
> - !xen_domid_restrict) {
> + if (!qemu_xen_xs_write(xenstore, XBT_NULL, path, state, strlen(state))) {
> error_report("error recording dm state");
> exit(1);
> }
> @@ -101,7 +100,10 @@ static int xen_init(MachineState *ms)
> xc_interface_close(xen_xc);
> return -1;
> }
> - qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
> +
> + if(!xen_domid_restrict)
> + qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
> +
> /*
> * opt out of system RAM being allocated by generic code
> */
>
> That works for both dm_restrict 0 & 1.
>
> I think you should submit your change and I can follow up with the
> above if it seems desirable.
Let's just do it in one. I'll move that comment about 'call may fail'
down to where you've made the qemu_add_vm_change_state_handler()
conditional. And QEMU style requires braces even for a one-line if().
I'll send it out and let you add your Signed-off-by: and Tested-by: to
it.
smime.p7s
Description: S/MIME cryptographic signature
Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation
Hi, David,
On Mon, Mar 13, 2023 at 4:45 AM David Woodhouse wrote:
>
> On Sun, 2023-03-12 at 15:19 -0400, Jason Andryuk wrote:
> >
> > This breaks dm_restrict=1 since the xs_open is not allowed by the
> > time
> > this is called. There are other evtchn errors before this as well:
> > # cat /var/log/xen/qemu-dm-debian.log
> > char device redirected to /dev/pts/8 (label serial0)
> > xen be core: can't open evtchn device
> > xen be core: can't open evtchn device
> > xen be core: can't open evtchn device
> > xen be core: can't open evtchn device
> > xen be core: can't open evtchn device
> > xen be core: can't open evtchn device
> > xen be core: can't open evtchn device
> > xen be core: can't open evtchn device
> > Could not contact XenStore
> >
> > Ok, those "xen be core: can't open evtchn device" were there before
> > the recent changes and seem to be non-fatal.
>
> Hm, I *think* we can just revert that part and use the global
> 'xenstore' like we did before, except via the new ops.
>
> --- a/accel/xen/xen-all.c
> +++ b/accel/xen/xen-all.c
> @@ -32,28 +32,18 @@ xendevicemodel_handle *xen_dmod;
>
> static void xenstore_record_dm_state(const char *state)
> {
> -struct xs_handle *xs;
> char path[50];
>
> -/* We now have everything we need to set the xenstore entry. */
> -xs = xs_open(0);
> -if (xs == NULL) {
> -fprintf(stderr, "Could not contact XenStore\n");
> -exit(1);
> -}
> -
> snprintf(path, sizeof (path), "device-model/%u/state", xen_domid);
> /*
> * This call may fail when running restricted so don't make it fatal in
> * that case. Toolstacks should instead use QMP to listen for state
> changes.
> */
> -if (!xs_write(xs, XBT_NULL, path, state, strlen(state)) &&
> +if (!qemu_xen_xs_write(xenstore, XBT_NULL, path, state, strlen(state)) &&
> !xen_domid_restrict) {
> error_report("error recording dm state");
> exit(1);
> }
> -
> -xs_close(xs);
> }
This looks good, better than what I posted, and seems to work for both
dm_restrict set and unset.
>
> Alternatively, that xs_write is destined to fail anyway in the
> xen_domid_restrict case, isn't it? So the xs_open() should be allowed
> to fail similarly. Or perhaps we shouldn't even *try*?
For dm_restricted, xs_write() does fail. I verified that with a print
statement. I think "shouldn't even try" makes sense. I'm thinking
that xen_domid_restricted shouldn't even add the callback. Something
like:
--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -39,8 +39,7 @@ static void xenstore_record_dm_state(const char *state)
* This call may fail when running restricted so don't make it fatal in
* that case. Toolstacks should instead use QMP to listen for
state changes.
*/
-if (!qemu_xen_xs_write(xenstore, XBT_NULL, path, state, strlen(state)) &&
-!xen_domid_restrict) {
+if (!qemu_xen_xs_write(xenstore, XBT_NULL, path, state, strlen(state))) {
error_report("error recording dm state");
exit(1);
}
@@ -101,7 +100,10 @@ static int xen_init(MachineState *ms)
xc_interface_close(xen_xc);
return -1;
}
-qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
+
+if(!xen_domid_restrict)
+qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
+
/*
* opt out of system RAM being allocated by generic code
*/
That works for both dm_restrict 0 & 1.
I think you should submit your change and I can follow up with the
above if it seems desirable.
Thanks,
Jason
Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation
On Sun, 2023-03-12 at 15:19 -0400, Jason Andryuk wrote:
>
> This breaks dm_restrict=1 since the xs_open is not allowed by the
> time
> this is called. There are other evtchn errors before this as well:
> # cat /var/log/xen/qemu-dm-debian.log
> char device redirected to /dev/pts/8 (label serial0)
> xen be core: can't open evtchn device
> xen be core: can't open evtchn device
> xen be core: can't open evtchn device
> xen be core: can't open evtchn device
> xen be core: can't open evtchn device
> xen be core: can't open evtchn device
> xen be core: can't open evtchn device
> xen be core: can't open evtchn device
> Could not contact XenStore
>
> Ok, those "xen be core: can't open evtchn device" were there before
> the recent changes and seem to be non-fatal.
Hm, I *think* we can just revert that part and use the global
'xenstore' like we did before, except via the new ops.
--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -32,28 +32,18 @@ xendevicemodel_handle *xen_dmod;
static void xenstore_record_dm_state(const char *state)
{
-struct xs_handle *xs;
char path[50];
-/* We now have everything we need to set the xenstore entry. */
-xs = xs_open(0);
-if (xs == NULL) {
-fprintf(stderr, "Could not contact XenStore\n");
-exit(1);
-}
-
snprintf(path, sizeof (path), "device-model/%u/state", xen_domid);
/*
* This call may fail when running restricted so don't make it fatal in
* that case. Toolstacks should instead use QMP to listen for state
changes.
*/
-if (!xs_write(xs, XBT_NULL, path, state, strlen(state)) &&
+if (!qemu_xen_xs_write(xenstore, XBT_NULL, path, state, strlen(state)) &&
!xen_domid_restrict) {
error_report("error recording dm state");
exit(1);
}
-
-xs_close(xs);
}
Alternatively, that xs_write is destined to fail anyway in the
xen_domid_restrict case, isn't it? So the xs_open() should be allowed
to fail similarly. Or perhaps we shouldn't even *try*?
smime.p7s
Description: S/MIME cryptographic signature
Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation
On Tue, Mar 7, 2023 at 1:29 PM David Woodhouse wrote:
>
> From: Paul Durrant
>
> Signed-off-by: Paul Durrant
> Signed-off-by: David Woodhouse
> Reviewed-by: Paul Durrant
> ---
> accel/xen/xen-all.c | 11 +-
> hw/char/xen_console.c | 2 +-
> hw/i386/kvm/xen_xenstore.c | 3 -
> hw/i386/kvm/xenstore_impl.h | 8 +-
> hw/xen/xen-bus-helper.c | 62 +++
> hw/xen/xen-bus.c| 261
> hw/xen/xen-legacy-backend.c | 119 +++--
> hw/xen/xen-operations.c | 198 +
> hw/xen/xen_devconfig.c | 4 +-
> hw/xen/xen_pt_graphics.c| 1 -
> hw/xen/xen_pvdev.c | 49 +-
> include/hw/xen/xen-bus-helper.h | 26 +--
> include/hw/xen/xen-bus.h| 17 +-
> include/hw/xen/xen-legacy-backend.h | 6 +-
> include/hw/xen/xen_backend_ops.h| 163 +
> include/hw/xen/xen_common.h | 1 -
> include/hw/xen/xen_pvdev.h | 2 +-
> softmmu/globals.c | 1 +
> 18 files changed, 525 insertions(+), 409 deletions(-)
>
> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
> index e85e4aeba5..425216230f 100644
> --- a/accel/xen/xen-all.c
> +++ b/accel/xen/xen-all.c
> @@ -90,12 +90,15 @@ void xenstore_store_pv_console_info(int i, Chardev *chr)
> }
>
>
> -static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
> +static void xenstore_record_dm_state(const char *state)
> {
> +struct xs_handle *xs;
> char path[50];
>
> +/* We now have everything we need to set the xenstore entry. */
> +xs = xs_open(0);
> if (xs == NULL) {
> -error_report("xenstore connection not initialized");
> +fprintf(stderr, "Could not contact XenStore\n");
> exit(1);
> }
This breaks dm_restrict=1 since the xs_open is not allowed by the time
this is called. There are other evtchn errors before this as well:
# cat /var/log/xen/qemu-dm-debian.log
char device redirected to /dev/pts/8 (label serial0)
xen be core: can't open evtchn device
xen be core: can't open evtchn device
xen be core: can't open evtchn device
xen be core: can't open evtchn device
xen be core: can't open evtchn device
xen be core: can't open evtchn device
xen be core: can't open evtchn device
xen be core: can't open evtchn device
Could not contact XenStore
Ok, those "xen be core: can't open evtchn device" were there before
the recent changes and seem to be non-fatal.
Regards,
Jason
[PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation
From: Paul Durrant
Signed-off-by: Paul Durrant
Signed-off-by: David Woodhouse
Reviewed-by: Paul Durrant
---
accel/xen/xen-all.c | 11 +-
hw/char/xen_console.c | 2 +-
hw/i386/kvm/xen_xenstore.c | 3 -
hw/i386/kvm/xenstore_impl.h | 8 +-
hw/xen/xen-bus-helper.c | 62 +++
hw/xen/xen-bus.c| 261
hw/xen/xen-legacy-backend.c | 119 +++--
hw/xen/xen-operations.c | 198 +
hw/xen/xen_devconfig.c | 4 +-
hw/xen/xen_pt_graphics.c| 1 -
hw/xen/xen_pvdev.c | 49 +-
include/hw/xen/xen-bus-helper.h | 26 +--
include/hw/xen/xen-bus.h| 17 +-
include/hw/xen/xen-legacy-backend.h | 6 +-
include/hw/xen/xen_backend_ops.h| 163 +
include/hw/xen/xen_common.h | 1 -
include/hw/xen/xen_pvdev.h | 2 +-
softmmu/globals.c | 1 +
18 files changed, 525 insertions(+), 409 deletions(-)
diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
index e85e4aeba5..425216230f 100644
--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -90,12 +90,15 @@ void xenstore_store_pv_console_info(int i, Chardev *chr)
}
-static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
+static void xenstore_record_dm_state(const char *state)
{
+struct xs_handle *xs;
char path[50];
+/* We now have everything we need to set the xenstore entry. */
+xs = xs_open(0);
if (xs == NULL) {
-error_report("xenstore connection not initialized");
+fprintf(stderr, "Could not contact XenStore\n");
exit(1);
}
@@ -109,6 +112,8 @@ static void xenstore_record_dm_state(struct xs_handle *xs,
const char *state)
error_report("error recording dm state");
exit(1);
}
+
+xs_close(xs);
}
@@ -117,7 +122,7 @@ static void xen_change_state_handler(void *opaque, bool
running,
{
if (running) {
/* record state running */
-xenstore_record_dm_state(xenstore, "running");
+xenstore_record_dm_state("running");
}
}
diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index e9cef3e1ef..ad8638a86d 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -181,7 +181,7 @@ static int con_init(struct XenLegacyDevice *xendev)
const char *output;
/* setup */
-dom = xs_get_domain_path(xenstore, con->xendev.dom);
+dom = qemu_xen_xs_get_domain_path(xenstore, con->xendev.dom);
if (!xendev->dev) {
snprintf(con->console, sizeof(con->console), "%s/console", dom);
} else {
diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
index fb3648a058..35898e9b37 100644
--- a/hw/i386/kvm/xen_xenstore.c
+++ b/hw/i386/kvm/xen_xenstore.c
@@ -38,9 +38,6 @@
#define TYPE_XEN_XENSTORE "xen-xenstore"
OBJECT_DECLARE_SIMPLE_TYPE(XenXenstoreState, XEN_XENSTORE)
-#define XEN_PAGE_SHIFT 12
-#define XEN_PAGE_SIZE (1ULL << XEN_PAGE_SHIFT)
-
#define ENTRIES_PER_FRAME_V1 (XEN_PAGE_SIZE / sizeof(grant_entry_v1_t))
#define ENTRIES_PER_FRAME_V2 (XEN_PAGE_SIZE / sizeof(grant_entry_v2_t))
diff --git a/hw/i386/kvm/xenstore_impl.h b/hw/i386/kvm/xenstore_impl.h
index bbe2391e2e..0df2a91aae 100644
--- a/hw/i386/kvm/xenstore_impl.h
+++ b/hw/i386/kvm/xenstore_impl.h
@@ -12,13 +12,7 @@
#ifndef QEMU_XENSTORE_IMPL_H
#define QEMU_XENSTORE_IMPL_H
-typedef uint32_t xs_transaction_t;
-
-#define XBT_NULL 0
-
-#define XS_PERM_NONE 0x00
-#define XS_PERM_READ 0x01
-#define XS_PERM_WRITE 0x02
+#include "hw/xen/xen_backend_ops.h"
typedef struct XenstoreImplState XenstoreImplState;
diff --git a/hw/xen/xen-bus-helper.c b/hw/xen/xen-bus-helper.c
index 5a1e12b374..b2b2cc9c5d 100644
--- a/hw/xen/xen-bus-helper.c
+++ b/hw/xen/xen-bus-helper.c
@@ -10,6 +10,7 @@
#include "hw/xen/xen-bus.h"
#include "hw/xen/xen-bus-helper.h"
#include "qapi/error.h"
+#include "trace.h"
#include
@@ -46,34 +47,28 @@ const char *xs_strstate(enum xenbus_state state)
return "INVALID";
}
-void xs_node_create(struct xs_handle *xsh, xs_transaction_t tid,
-const char *node, struct xs_permissions perms[],
-unsigned int nr_perms, Error **errp)
+void xs_node_create(struct qemu_xs_handle *h, xs_transaction_t tid,
+const char *node, unsigned int owner, unsigned int domid,
+unsigned int perms, Error **errp)
{
trace_xs_node_create(node);
-if (!xs_write(xsh, tid, node, "", 0)) {
+if (!qemu_xen_xs_create(h, tid, owner, domid, perms, node)) {
error_setg_errno(errp, errno, "failed to create node '%s'", node);
-return;
-}
-
-if (!xs_set_permissions(xsh, tid, node, perms, nr_perms)) {
-error_setg_errno(errp, errno, "failed to set node '%s' permissions",
- node);
}
}
-void xs_node_destroy(struct
