On Wed, Apr 07, 2021 at 07:08:06PM +0200, 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_execute_callbacks(unsigned int gsi)
> > > +{
> > > + struct hvm_irq *hvm_irq = hvm_domain_irq(current->domain);
> > > + struct hvm_gsi_eoi_callback *cb;
> > > +
> > > + read_lock(&hvm_irq->gsi_callbacks_lock);
> > > + list_for_each_entry ( cb, &hvm_irq->gsi_callbacks[gsi], list )
> > > + cb->callback(gsi, cb->data);
> > > + read_unlock(&hvm_irq->gsi_callbacks_lock);
> > > +}
> >
> > Just as an observation (for now at least) - holding the lock here
> > means the callbacks cannot re-register themselves.
>
> Well, re-registering would be weird, as the callback is not
> unregistered after execution. What is likely more relevant is that the
> callback cannot unregister itself. I haven't found a need for this so
> far, so I think it's fine.
I'm afraid I was wrong here - rtc_pf_callback could attempt to
unregister the timer, and thus end up calling
hvm_gsi_unregister_callback inside of a callback.
I need to figure a way to solve this. We already run the RTC in no ack
mode (which is correct because of the flag we expose in the WAET ACPI
table), and hence I wonder if we still need to keep the code for the
strict_mode around, since it's not used at all. Would you be OK with
me removing the mode_strict related code?
Thanks, Roger.
