Re: [PATCH v6 5/7] xen: add new domctl get_changed_domain

[Juergen Gross <jgross@xxxxxxxx>]

On 07.01.25 18:06, Jan Beulich wrote:
> On 07.01.2025 17:48, Jürgen Groß wrote:
> > On 07.01.25 17:28, Jan Beulich wrote:
> > > On 07.01.2025 11:17, Juergen Gross wrote:
> > > > --- a/xen/common/domain.c
> > > > +++ b/xen/common/domain.c
> > > > @@ -185,6 +185,76 @@ static void domain_changed_state(const struct domain *d)
> > > >        unlock_dom_exc_handler(hdl);
> > > >    }
> > > >    
> > > > +static void set_domain_state_info(struct xen_domctl_get_domain_state *info,
> > > > +                                  const struct domain *d)
> > > > +{
> > > > +    info->state = XEN_DOMCTL_GETDOMSTATE_STATE_EXIST;
> > > > +    if ( d->is_shut_down )
> > > > +        info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_SHUTDOWN;
> > > > +    if ( d->is_dying == DOMDYING_dying )
> > > > +        info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_DYING;
> > > > +    if ( d->is_dying == DOMDYING_dead )
> > > > +        info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_DEAD;
> > > > +    info->unique_id = d->unique_id;
> > > > +}
> > > > +
> > > > +int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain 
> > > > *d,
> > > > +                     domid_t *domid)
> > > > +{
> > > > +    unsigned int dom;
> > > > +    int rc = -ENOENT;
> > > > +    struct domain *hdl;
> > > > +
> > > > +    if ( info->pad0 || info->pad1 )
> > > > +        return -EINVAL;
> > > > +
> > > > +    if ( d )
> > > > +    {
> > > > +        set_domain_state_info(info, d);
> > > > +
> > > > +        return 0;
> > > > +    }
> > > > +
> > > > +    /*
> > > > +     * Only domain registered for VIRQ_DOM_EXC event is allowed to query
> > > > +     * domains having changed state.
> > > > +     */
> > > > +    if ( !domain_handles_global_virq(current->domain, VIRQ_DOM_EXC) )
> > > > +        return -EACCES;
> > > > +
> > > > +    hdl = lock_dom_exc_handler();
> > > Instead of leaving a small window for races between the if() and this
> > > function call, can't you simply compare hdl against current->domain?
> > Good idea.
> >
> > > > +    while ( dom_state_changed )
> > > > +    {
> > > > +        dom = find_first_bit(dom_state_changed, DOMID_MASK + 1);
> > > > +        if ( dom >= DOMID_FIRST_RESERVED )
> > > > +            break;
> > > > +        if ( test_and_clear_bit(dom, dom_state_changed) )
> > > As this is now running under lock, does it really need to be test-and-clear?
> > > What mechanism would allow the flag to be cleared between the find-1st and
> > > here? Plus, like for patch 4, I think it could be __clear_bit() here.
> > It is only under read_lock(), so there are concurrent calls possible.
> > I don't think we want to use write_lock() here, do we?
> Probably not; I have to admit I didn't even pay attention to this aspect.
> Then the set_bit() in domain_changed_state() also need to remain as is (in
> patch 4 I think it was).
This one needs to stay, but the one in domain_init_states() can be changed
to the non-atomic variant again.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature