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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.12.2024 12:12, Juergen Gross wrote:
> V4:
> - add __read_mostly (Jan Beulich)
> - use __set_biz() (Jan Beulich)
> - fix error handling in evtchn_bind_virq() (Jan Beulich)

I'm sorry, I should have spotted a 2nd issue already when reviewing v3 (or
even an earlier version).

> @@ -138,6 +139,60 @@ bool __read_mostly vmtrace_available;
>  
>  bool __read_mostly vpmu_is_available;
>  
> +static DEFINE_SPINLOCK(dom_state_changed_lock);
> +static unsigned long *__read_mostly 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);

This needs to not happen when ...

> @@ -485,11 +486,21 @@ 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 )
> +            return rc;
> +
> +        deinit_if_err = true;
> +    }
> +
>      write_lock(&d->event_lock);
>  
>      if ( read_atomic(&v->virq_to_evtchn[virq]) )
>      {
>          rc = -EEXIST;
> +        deinit_if_err = false;
>          gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc);
>          goto out;
>      }

... we take this error path. Which I think calls for moving the
domain_init_states() invocation ...

> @@ -527,6 +538,9 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
> evtchn_port_t port)
>   out:
>      write_unlock(&d->event_lock);
>  
> +    if ( rc && deinit_if_err )
> +        domain_deinit_states();
> +
>      return rc;
>  }

... somewhere here. It really doesn't need doing early, as the caller
may assume the bitmap was set up only when this hypercall returns
successfully.

Jan