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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Apr 16, 2021 at 09:29:26AM +0200, Jan Beulich wrote:
> On 15.04.2021 18:04, Roger Pau Monné wrote:
> > 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?
> 
> Not sure, to be honest. Years ago I did submit a patch correcting this
> ("x86/HVM: tie RTC emulation mode to enabling of Viridian emulation"),
> as we shouldn't assume all guests to even know of WAET.

It's very likely guest that don't even know about WAET to continue
working fine even in the no_ack mode. In fact the current code for
strict_mode will inject 10 interrupts without REG_C being read, as
there's no check for the value of REG_C before injecting the
interrupt.

> Hence running
> uniformly in rtc_mode_no_ack isn't really correct. I'm still carrying
> this patch, as Tim (iirc) had asked not to tie the behavior to the
> Viridian param, but give it its own one. Which I still didn't get to.
> 
> Of course, if we decided to drop mode_strict support, I could also
> drop that patch ...

AFAICT the no_ack mode it's been used since Xen 4.3, and so far we had
no complains, so I think it's safe to just remove the code for
strict_mode. It can always be fetched from the repository history if
there's a need to support strict_mode in the future.

Thanks, Roger.