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

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

On 17.12.24 12:30, Jan Beulich wrote:
> On 17.12.2024 12:12, Juergen Gross wrote:
> > V4:
> > - add __read_mostly (Jan Beulich)
> > - use __set_biz() (Jan Beulich)
> > - fix error handling in evtchn_bind_virq() (Jan Beulich)
> I'm sorry, I should have spotted a 2nd issue already when reviewing v3 (or
> even an earlier version).
>
> > @@ -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 *__read_mostly 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);
> This needs to not happen when ...
>
> > @@ -485,11 +486,21 @@ 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 )
> > +            return rc;
> > +
> > +        deinit_if_err = true;
> > +    }
> > +
> >       write_lock(&d->event_lock);
> >   
> >       if ( read_atomic(&v->virq_to_evtchn[virq]) )
> >       {
> >           rc = -EEXIST;
> > +        deinit_if_err = false;
> >           gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc);
> >           goto out;
> >       }
> ... we take this error path. Which I think calls for moving the
> domain_init_states() invocation ...
>
> > @@ -527,6 +538,9 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
> > evtchn_port_t port)
> >    out:
> >       write_unlock(&d->event_lock);
> >   
> > +    if ( rc && deinit_if_err )
> > +        domain_deinit_states();
> > +
> >       return rc;
> >   }
> ... somewhere here. It really doesn't need doing early, as the caller
> may assume the bitmap was set up only when this hypercall returns
> successfully.
OTOH this will require undoing the binding of the virq in case of an
error returned by domain_init_states().

It would probably be best to place the call of domain_init_states()
after the -EEXIST case.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature