Re: [PATCH v2 4/7] xen: add bitmap to indicate per-domain state changes

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.12.2024 14:02, 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.
> 
> Resetting a bit will be done in a future patch.
> 
> This information is needed for Xenstore to keep track of all domains.
> 
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>

What I'm still missing is at least mention of the global-ness of all of
this, and why that's deemed okay for now.

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -138,6 +138,60 @@ bool __read_mostly vmtrace_available;
>  
>  bool __read_mostly vpmu_is_available;
>  
> +static DEFINE_SPINLOCK(dom_state_changed_lock);
> +static unsigned long *dom_state_changed;
> +
> +int domain_init_states(void)
> +{
> +    const struct domain *d;
> +    int rc = -ENOMEM;
> +
> +    spin_lock(&dom_state_changed_lock);
> +
> +    if ( dom_state_changed )
> +        bitmap_zero(dom_state_changed, DOMID_FIRST_RESERVED);
> +    else
> +    {
> +        dom_state_changed = xzalloc_array(unsigned long,
> +                                          
> BITS_TO_LONGS(DOMID_FIRST_RESERVED));

New code wants to use xvmalloc() et al.

> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -485,20 +485,27 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
> evtchn_port_t port)
>      if ( (v = domain_vcpu(d, vcpu)) == NULL )
>          return -ENOENT;
>  
> +    if ( virq == VIRQ_DOM_EXC )
> +    {
> +        rc = domain_init_states();
> +        if ( rc )
> +            goto out;
> +    }
> +
>      write_lock(&d->event_lock);
>  
>      if ( read_atomic(&v->virq_to_evtchn[virq]) )
>      {
>          rc = -EEXIST;
>          gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc);
> -        goto out;
> +        goto unlock;
>      }
>  
>      port = rc = evtchn_get_port(d, port);
>      if ( rc < 0 )
>      {
>          gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc);
> -        goto out;
> +        goto unlock;
>      }
>  
>      rc = 0;
> @@ -524,9 +531,13 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
> evtchn_port_t port)
>       */
>      write_atomic(&v->virq_to_evtchn[virq], port);
>  
> - out:
> + unlock:
>      write_unlock(&d->event_lock);
>  
> + out:
> +    if ( rc )
> +        domain_deinit_states();
> +
>      return rc;
>  }

Renaming the prior label (and hence needing to fiddle with existing goto-s)
feels a little fragile. How about keeping "out" as is and introducing "deinit"
or some such?

Jan