Re: [PATCH v4 01/12] x86/rtc: drop code related to strict mode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.05.2021 17:28, Roger Pau Monné wrote:
> On Mon, May 03, 2021 at 04:58:07PM +0200, Jan Beulich wrote:
>> On 03.05.2021 16:47, Roger Pau Monné wrote:
>>> On Mon, May 03, 2021 at 02:26:51PM +0200, Jan Beulich wrote:
>>>> On 03.05.2021 11:28, Roger Pau Monné wrote:
>>>>> On Thu, Apr 29, 2021 at 04:53:07PM +0200, Jan Beulich wrote:
>>>>>> On 20.04.2021 16:07, Roger Pau Monne wrote:
>>>>>>> @@ -337,8 +336,7 @@ int pt_update_irq(struct vcpu *v)
>>>>>>>      {
>>>>>>>          if ( pt->pending_intr_nr )
>>>>>>>          {
>>>>>>> -            /* RTC code takes care of disabling the timer itself. */
>>>>>>> -            if ( (pt->irq != RTC_IRQ || !pt->priv) && 
>>>>>>> pt_irq_masked(pt) &&
>>>>>>> +            if ( pt_irq_masked(pt) &&
>>>>>>>                   /* Level interrupts should be asserted even if 
>>>>>>> masked. */
>>>>>>>                   !pt->level )
>>>>>>>              {
>>>>>>
>>>>>> I'm struggling to relate this to any other part of the patch. In
>>>>>> particular I can't find the case where a periodic timer would be
>>>>>> registered with RTC_IRQ and a NULL private pointer. The only use
>>>>>> I can find is with a non-NULL pointer, which would mean the "else"
>>>>>> path is always taken at present for the RTC case (which you now
>>>>>> change).
>>>>>
>>>>> Right, the else case was always taken because as the comment noted RTC
>>>>> would take care of disabling itself (by calling destroy_periodic_time
>>>>> from the callback when using strict_mode). When no_ack mode was
>>>>> implemented this wasn't taken into account AFAICT, and thus the RTC
>>>>> was never removed from the list even when masked.
>>>>>
>>>>> I think with no_ack mode the RTC shouldn't have this specific handling
>>>>> in pt_update_irq, as it should behave like any other virtual timer.
>>>>> I could try to split this as a separate bugfix, but then I would have
>>>>> to teach pt_update_irq to differentiate between strict_mode and no_ack
>>>>> mode.
>>>>
>>>> A fair part of my confusion was about "&& !pt->priv".
>>>
>>> I think you meant "|| !pt->priv"?
>>
>> Oops, indeed.
>>
>>>> I've looked back
>>>> at 9607327abbd3 ("x86/HVM: properly handle RTC periodic timer even when
>>>> !RTC_PIE"), where this was added. It was, afaict, to cover for
>>>> hpet_set_timer() passing NULL with RTC_IRQ.
>>>
>>> That's tricky, as hpet_set_timer hardcodes 8 instead of using RTC_IRQ
>>> which makes it really easy to miss.
>>>
>>>> Which makes me suspect that
>>>> be07023be115 ("x86/vhpet: add support for level triggered interrupts")
>>>> may have subtly broken things.
>>>
>>> Right - as that would have made the RTC irq when generated from the
>>> HPET no longer be suspended when masked (as pt->priv would no longer
>>> be NULL). Could be fixed with:
>>>
>>> diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
>>> index ca94e8b4538..f2cbd12f400 100644
>>> --- a/xen/arch/x86/hvm/hpet.c
>>> +++ b/xen/arch/x86/hvm/hpet.c
>>> @@ -318,7 +318,8 @@ static void hpet_set_timer(HPETState *h, unsigned int 
>>> tn,
>>>                           hpet_tick_to_ns(h, diff),
>>>                           oneshot ? 0 : hpet_tick_to_ns(h, 
>>> h->hpet.period[tn]),
>>>                           irq, timer_level(h, tn) ? hpet_timer_fired : NULL,
>>> -                         (void *)(unsigned long)tn, timer_level(h, tn));
>>> +                         timer_level(h, tn) ? (void *)(unsigned long)tn : 
>>> NULL,
>>> +                         timer_level(h, tn));
>>>  }
>>>  
>>>  static inline uint64_t hpet_fixup_reg(
>>>
>>> Passing again NULL as the callback private data for edge triggered
>>> interrupts.
>>
>> Right, plus perhaps at the same time replacing the hardcoded 8.
> 
> Right, but if you agree to take this patch and remove strict_mode then
> the emulated RTC won't disable itself anymore, and hence needs to be
> handled as any other virtual timer?

I'm still trying to become convinced, both of the removal of the mode
in general and the particular part of the change I've been struggling
with.

Jan