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

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

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.

Attachment: smime.p7s
Description: S/MIME cryptographic signature