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

[Jan Beulich <jbeulich@suse.com>]

On 20.04.2021 16:07, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -46,15 +46,6 @@
>  #define epoch_year     1900
>  #define get_year(x)    (x + epoch_year)
>  
> -enum rtc_mode {
> -   rtc_mode_no_ack,
> -   rtc_mode_strict
> -};
> -
> -/* This must be in sync with how hvmloader sets the ACPI WAET flags. */
> -#define mode_is(d, m) ((void)(d), rtc_mode_##m == rtc_mode_no_ack)
> -#define rtc_mode_is(s, m) mode_is(vrtc_domain(s), m)

Leaving aside my concerns about this removal, I think some form of
reference to hvmloader and its respective behavior should remain
here, presumably in form of a (replacement) comment.

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

Jan