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
