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

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

On 31.10.24 11:59, Jan Beulich wrote:
> On 23.10.2024 15:10, 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.
> That's furthering the "there can be only one consumer" model that also
> is used for VIRQ_DOM_EXC itself. I consider the existing model flawed
> (nothing keeps a 2nd party with sufficient privilege from invoking
> XEN_DOMCTL_set_virq_handler a 2nd time, taking away the notification
> from whoever had first requested it), and hence I dislike this being
> extended. Conceivably multiple parties may indeed be interested in
> this kind of information. At which point resetting state when the vIRQ
> is bound is questionable (or the data would need to become per-domain
> rather than global, or even yet more fine-grained, albeit
> ->virq_to_evtchn[] is also per-domain, when considering global vIRQ-s).
The bitmap is directly tied to the VIRQ_DOM_EXC anyway, as it is that
event which makes the consumer look into the bitmap via the new hypercall.

If we decide to allow multiple consumers of VIRQ_DOM_EXC, we'll need to
have one bitmap per consumer of the event. This is not very hard to
modify.

If you'd like that better, I can dynamically allocate the bitmap on
binding VIRQ_DOM_EXC and freeing it again when unbinding is done.

> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -138,6 +138,22 @@ bool __read_mostly vmtrace_available;
> >   
> >   bool __read_mostly vpmu_is_available;
> >   
> > +static DECLARE_BITMAP(dom_state_changed, DOMID_MASK + 1);
> While it won't alter the size of the array, I think DOMID_FIRST_RESERVED
> would be more logical to use here and ...
>
> > +void domain_reset_states(void)
> > +{
> > +    struct domain *d;
> > +
> > +    bitmap_zero(dom_state_changed, DOMID_MASK + 1);
> ... here.
Fine with me.

> > +    rcu_read_lock(&domlist_read_lock);
> > +
> > +    for_each_domain ( d )
> > +        set_bit(d->domain_id, dom_state_changed);
> d is used only here, so could be pointer-to-const?
Agreed.

> > --- a/xen/common/event_channel.c
> > +++ b/xen/common/event_channel.c
> > @@ -1296,6 +1296,8 @@ long do_event_channel_op(int cmd, 
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >           rc = evtchn_bind_virq(&bind_virq, 0);
> >           if ( !rc && __copy_to_guest(arg, &bind_virq, 1) )
> >               rc = -EFAULT; /* Cleaning up here would be a mess! */
> > +        if ( !rc && bind_virq.virq == VIRQ_DOM_EXC )
> > +            domain_reset_states();
> evtchn_bind_virq() isn't static, so callers beyond the present ones could
> appear without noticing the need for this special casing. Is there a reason
> the check can't move into the function? Doing the check in spite of the
> copy-out failing is imo still reasonable behavior.
Moving the test into evtchn_bind_virq() should work. I'll change that.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature