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

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

On 09.12.24 17:52, Jan Beulich wrote:
> 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.
I'll add:

  As the usage of this bitmap is tightly coupled with the VIRQ_DOM_EXC event,
  it is meant to be used only by a single consumer in the system, just like
  the VIRQ_DOM_EXC event.

> > --- 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.
Okay.

> > --- 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?
Fine with me.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature