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

[Jürgen Groß <jgross@xxxxxxxx>]

On 16.12.24 11:21, Jan Beulich wrote:
> On 13.12.2024 17:24, Juergen Gross wrote:
> > @@ -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 *dom_state_changed;
> __read_mostly?
Yes.

> > +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 = xvzalloc_array(unsigned long,
> > +                                           
> > BITS_TO_LONGS(DOMID_FIRST_RESERVED));
> > +        if ( !dom_state_changed )
> > +            goto unlock;
> > +    }
> > +
> > +    rcu_read_lock(&domlist_read_lock);
> > +
> > +    for_each_domain ( d )
> > +        set_bit(d->domain_id, dom_state_changed);
> Use the cheaper __set_bit() here?
Okay.

> > +static void domain_changed_state(const struct domain *d)
> > +{
> > +    spin_lock(&dom_state_changed_lock);
> > +
> > +    if ( dom_state_changed )
> > +        set_bit(d->domain_id, dom_state_changed);
> And perhaps even here, considering everything's under lock now?
True.

> > --- a/xen/common/event_channel.c
> > +++ b/xen/common/event_channel.c
> > @@ -485,6 +485,13 @@ 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 deinit;
> > +    }
> This is tied to VIRQ_DOM_EXC. How come ...
>
> > @@ -527,6 +534,10 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
> > evtchn_port_t port)
> >    out:
> >       write_unlock(&d->event_lock);
> >   
> > + deinit:
> > +    if ( rc )
> > +        domain_deinit_states();
> > +
> >       return rc;
> >   }
> ... de-init happens upon any error, regardless of vIRQ? Even checking
> the virq isn't sufficient, as we also need to gracefully deal with the
> -EEXIST path.
Good catch. Will change that.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature