Re: [PATCH 2/6] xen: add bitmap to indicate per-domain state changes

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.10.2024 15:10, Juergen Gross wrote:
> Add a bitmap with one bit per possible domid indicating the respective
> domain has changed its state (created, deleted, dying, crashed,
> shutdown).
> 
> Registering the VIRQ_DOM_EXC event will result in setting the bits for
> all existing domains and resetting all other bits.

That's furthering the "there can be only one consumer" model that also
is used for VIRQ_DOM_EXC itself. I consider the existing model flawed
(nothing keeps a 2nd party with sufficient privilege from invoking
XEN_DOMCTL_set_virq_handler a 2nd time, taking away the notification
from whoever had first requested it), and hence I dislike this being
extended. Conceivably multiple parties may indeed be interested in
this kind of information. At which point resetting state when the vIRQ
is bound is questionable (or the data would need to become per-domain
rather than global, or even yet more fine-grained, albeit
->virq_to_evtchn[] is also per-domain, when considering global vIRQ-s).

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -138,6 +138,22 @@ bool __read_mostly vmtrace_available;
>  
>  bool __read_mostly vpmu_is_available;
>  
> +static DECLARE_BITMAP(dom_state_changed, DOMID_MASK + 1);

While it won't alter the size of the array, I think DOMID_FIRST_RESERVED
would be more logical to use here and ...

> +void domain_reset_states(void)
> +{
> +    struct domain *d;
> +
> +    bitmap_zero(dom_state_changed, DOMID_MASK + 1);

... here.

> +    rcu_read_lock(&domlist_read_lock);
> +
> +    for_each_domain ( d )
> +        set_bit(d->domain_id, dom_state_changed);

d is used only here, so could be pointer-to-const?

> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -1296,6 +1296,8 @@ long do_event_channel_op(int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          rc = evtchn_bind_virq(&bind_virq, 0);
>          if ( !rc && __copy_to_guest(arg, &bind_virq, 1) )
>              rc = -EFAULT; /* Cleaning up here would be a mess! */
> +        if ( !rc && bind_virq.virq == VIRQ_DOM_EXC )
> +            domain_reset_states();

evtchn_bind_virq() isn't static, so callers beyond the present ones could
appear without noticing the need for this special casing. Is there a reason
the check can't move into the function? Doing the check in spite of the
copy-out failing is imo still reasonable behavior.

Jan