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

[Jan Beulich <jbeulich@xxxxxxxx>]

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. 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 ...

Jan