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

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

On 07.01.25 17:14, Jan Beulich wrote:
> On 07.01.2025 11:17, 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.
> >
> > 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.
> >
> > 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>
> > ---
> > V2:
> > - use DOMID_FIRST_RESERVED instead of DOMID_MASK + 1 (Jan Beulich)
> > - use const (Jan Beulich)
> > - move call of domain_reset_states() into evtchn_bind_virq() (Jan Beulich)
> > - dynamically allocate dom_state_changed bitmap (Jan Beulich)
> > V3:
> > - use xvzalloc_array() (Jan Beulich)
> > - don't rename existing label (Jan Beulich)
> > V4:
> > - add __read_mostly (Jan Beulich)
> > - use __set_bit() (Jan Beulich)
> This change looks to have been lost, ...
>
> > - fix error handling in evtchn_bind_virq() (Jan Beulich)
> > V5:
> > - domain_init_states() may be called only if evtchn_bind_virq() has been
> >    called validly (Jan Beulich)
> > V6:
> > - guard dom_state_changed bitmap with d->event_lock (Jan Beulich)
> ... without it being mentioned anywhere, and without it becoming clear why
> it would have needed undoing.
Oh, thanks for spotting. I changed that by accident and missed to undo
this change.

> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -35,6 +35,7 @@
> >   #include <xen/irq.h>
> >   #include <xen/argo.h>
> >   #include <xen/llc-coloring.h>
> > +#include <xen/xvmalloc.h>
> >   #include <asm/p2m.h>
> >   #include <asm/processor.h>
> >   #include <public/sched.h>
> > @@ -139,6 +140,51 @@ bool __read_mostly vmtrace_available;
> >   
> >   bool __read_mostly vpmu_is_available;
> >   
> > +static unsigned long *__read_mostly dom_state_changed;
> > +
> > +int domain_init_states(void)
> > +{
> > +    const struct domain *d;
> > +
> > +    ASSERT(!dom_state_changed);
> > +    ASSERT(rw_is_write_locked(&current->domain->event_lock));
> rw_is_write_locked_by_me()?
Yes, probably better.

> > +    dom_state_changed = xvzalloc_array(unsigned long,
> > +                                       BITS_TO_LONGS(DOMID_FIRST_RESERVED));
> > +    if ( !dom_state_changed )
> > +        return -ENOMEM;
> > +
> > +    rcu_read_lock(&domlist_read_lock);
> > +
> > +    for_each_domain ( d )
> > +        set_bit(d->domain_id, dom_state_changed);
> > +
> > +    rcu_read_unlock(&domlist_read_lock);
> > +
> > +    return 0;
> > +}
> > +
> > +void domain_deinit_states(const struct domain *d)
> > +{
> > +    ASSERT(rw_is_write_locked(&d->event_lock));
> Again.
Yes.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature