Re: [PATCH v3 06/11] x86/hvm: allowing registering EOI callbacks for GSIs

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.04.2021 14:52, Roger Pau Monné wrote:
> On Wed, Apr 07, 2021 at 05:51:14PM +0200, Jan Beulich wrote:
>> On 31.03.2021 12:32, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/hvm/irq.c
>>> +++ b/xen/arch/x86/hvm/irq.c
>>> +void hvm_gsi_unregister_callback(struct domain *d, unsigned int gsi,
>>> +                                 struct hvm_gsi_eoi_callback *cb)
>>> +{
>>> +    struct hvm_irq *hvm_irq = hvm_domain_irq(d);
>>> +    const struct list_head *tmp;
>>> +
>>> +    if ( gsi >= hvm_irq->nr_gsis )
>>> +    {
>>> +        ASSERT_UNREACHABLE();
>>> +        return;
>>> +    }
>>> +
>>> +    write_lock(&hvm_irq->gsi_callbacks_lock);
>>> +    list_for_each ( tmp, &hvm_irq->gsi_callbacks[gsi] )
>>> +        if ( tmp == &cb->list )
>>> +        {
>>> +            list_del(&cb->list);
>>> +            break;
>>> +        }
>>> +    write_unlock(&hvm_irq->gsi_callbacks_lock);
>>> +}
>>
>> Perhaps somehow flag, at least in debug builds, if the callback
>> wasn#t found?
> 
> I've added a debug printf here to warn if the callback is not found,
> but I see it triggering because hpet_set_timer will call
> destroy_periodic_time and create_periodic_time and thus two calls will
> be made to hvm_gsi_unregister_callback. This is fine, but adding a
> message there gets too verbose, so I will drop it and leave the code
> as-is.
> 
> I don't see a problem with calling destroy_periodic_time multiple
> times even if the timer was not active, and that shouldn't result in a
> message being printed.

If destroy_periodic_time() is to remain the only caller, I guess I
agree. Other (future) callers may then need this function to gain
a return value indicating whether the callback was actually found.

Jan