[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to
From: |
Jason Andryuk |
Subject: |
Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation |
Date: |
Mon, 13 Mar 2023 19:17:17 -0400 |
Hi, David,
On Mon, Mar 13, 2023 at 4:45 AM David Woodhouse <dwmw2@infradead.org> 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
- [PULL 00/27] Enable PV backends with Xen/KVM emulation, David Woodhouse, 2023/03/07
- [PULL 22/27] hw/xen: Add emulated implementation of XenStore operations, David Woodhouse, 2023/03/07
- [PULL 01/27] hw/xen: Add xenstore wire implementation and implementation stubs, David Woodhouse, 2023/03/07
- [PULL 10/27] hw/xen: Add gnttab operations to allow redirection to internal emulation, David Woodhouse, 2023/03/07
- [PULL 05/27] hw/xen: Watches on XenStore transactions, David Woodhouse, 2023/03/07
- [PULL 02/27] hw/xen: Add basic XenStore tree walk and write/read/directory support, David Woodhouse, 2023/03/07
- [PULL 11/27] hw/xen: Pass grant ref to gnttab unmap operation, David Woodhouse, 2023/03/07
- [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation, David Woodhouse, 2023/03/07
- Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation, Jason Andryuk, 2023/03/12
- Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation, David Woodhouse, 2023/03/13
- Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation,
Jason Andryuk <=
- Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation, David Woodhouse, 2023/03/14
- [PATCH] accel/xen: Fix DM state change notification in dm_restrict mode, David Woodhouse, 2023/03/14
- Re: [PATCH] accel/xen: Fix DM state change notification in dm_restrict mode, Paul Durrant, 2023/03/14
- Re: [PATCH] accel/xen: Fix DM state change notification in dm_restrict mode, Jason Andryuk, 2023/03/14
[PULL 03/27] hw/xen: Implement XenStore watches, David Woodhouse, 2023/03/07
[PULL 09/27] hw/xen: Add evtchn operations to allow redirection to internal emulation, David Woodhouse, 2023/03/07
[PULL 20/27] hw/xen: Hook up emulated implementation for event channel operations, David Woodhouse, 2023/03/07
[PULL 19/27] hw/xen: Only advertise ring-page-order for xen-block if gnttab supports it, David Woodhouse, 2023/03/07
[PULL 24/27] hw/xen: Implement soft reset for emulated gnttab, David Woodhouse, 2023/03/07
[PULL 07/27] hw/xen: Implement core serialize/deserialize methods for xenstore_impl, David Woodhouse, 2023/03/07