Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation

[Jason Andryuk <jandryuk@xxxxxxxxx>]

Hi, David,

On Mon, Mar 13, 2023 at 4:45 AM David Woodhouse <dwmw2@xxxxxxxxxxxxx> 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