On Thu, Apr 08, 2021 at 04:31:59PM +0200, Jan Beulich wrote:
> 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.
There's also pt_irq_destroy_bind which likely cares about the return
value, so let's return a value from hvm_gsi_unregister_callback and
check it in pt_irq_destroy_bind.
Thanks, Roger.
