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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.12.2024 12:59, Jürgen Groß wrote:
> 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.

Hmm, right.

Jan