Re: [PATCH v3 3/7] x86/time: split CMOS read and probe logic into function

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

On Tue, Sep 03, 2024 at 05:16:44PM +0200, Jan Beulich wrote:
> On 03.09.2024 15:02, Roger Pau Monne wrote:
> > The current logic to probe for the CMOS RTC is open-coded in 
> > get_cmos_time(),
> > move it to a separate function that both serves the purpose of testing for 
> > the
> > CMOS RTC existence and returning its value.
> > 
> > The goal is to be able to split the probing and the reading logic into 
> > separate
> > helpers, and putting the current logic in a separate function helps 
> > simplifying
> > further changes.
> > 
> > A transient *rtc_p variable is introduced as a parameter to the function, 
> > that
> > will be removed by further changes.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> This looks like a straight transformation, except - as noted before - for ...
> 
> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -1292,45 +1292,32 @@ static bool __get_cmos_time(struct rtc_time *rtc)
> >      return t1 <= SECONDS(1) && t2 < MILLISECS(3);
> >  }
> >  
> > -static unsigned long get_cmos_time(void)
> > +static bool cmos_probe(struct rtc_time *rtc_p, bool cmos_rtc_probe)
> >  {
> > -    unsigned long res;
> > -    struct rtc_time rtc;
> >      unsigned int seconds = 60;
> > -    static bool __read_mostly cmos_rtc_probe;
> > -    boolean_param("cmos-rtc-probe", cmos_rtc_probe);
> > -
> > -    if ( efi_enabled(EFI_RS) )
> > -    {
> > -        res = efi_get_time();
> > -        if ( res )
> > -            return res;
> > -    }
> > -
> > -    if ( likely(!(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC)) )
> > -        cmos_rtc_probe = false;
> > -    else if ( system_state < SYS_STATE_smp_boot && !cmos_rtc_probe )
> > -        panic("System with no CMOS RTC advertised must be booted from EFI"
> > -              " (or with command line option \"cmos-rtc-probe\")\n");
> >  
> >      for ( ; ; )
> >      {
> > -        bool success = __get_cmos_time(&rtc);
> > +        bool success = __get_cmos_time(rtc_p);
> > +        struct rtc_time rtc = *rtc_p;
> >  
> > -        if ( likely(!cmos_rtc_probe) || !success ||
> > +        if ( likely(!cmos_rtc_probe) )
> > +            return true;
> > +
> > +        if ( !success ||
> >               rtc.sec >= 60 || rtc.min >= 60 || rtc.hour >= 24 ||
> >               !rtc.day || rtc.day > 31 ||
> >               !rtc.mon || rtc.mon > 12 )
> > -            break;
> > +            return false;
> >  
> >          if ( seconds < 60 )
> >          {
> >              if ( rtc.sec != seconds )
> >              {
> > -                cmos_rtc_probe = false;
> 
> ... the removal of this line. As asked for before, can the somewhat 
> sub-optimal
> new behavior (with the static, which now lives in another function, being
> cleared only the next time round) please be at least mentioned in the
> description?

Sure, sorry I've failed to do this here.  Such weird behavior is
transient, and will go away wit the next patch IIRC, once the probing
is properly split from the run-time reading of the CMOS RTC.

> >                  acpi_gbl_FADT.boot_flags &= ~ACPI_FADT_NO_CMOS_RTC;
> > +                return true;
> >              }
> > -            break;
> > +            return false;
> >          }
> >  
> >          process_pending_softirqs();
> > @@ -1338,7 +1325,31 @@ static unsigned long get_cmos_time(void)
> >          seconds = rtc.sec;
> >      }
> >  
> > -    if ( unlikely(cmos_rtc_probe) )
> > +    ASSERT_UNREACHABLE();
> > +    return false;
> > +}
> > +
> > +static unsigned long get_cmos_time(void)
> > +{
> > +    unsigned long res;
> > +    struct rtc_time rtc;
> > +    static bool __read_mostly cmos_rtc_probe;
> > +    boolean_param("cmos-rtc-probe", cmos_rtc_probe);
> > +
> > +    if ( efi_enabled(EFI_RS) )
> > +    {
> > +        res = efi_get_time();
> > +        if ( res )
> > +            return res;
> > +    }
> > +
> > +    if ( likely(!(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC)) )
> > +        cmos_rtc_probe = false;
> > +    else if ( system_state < SYS_STATE_smp_boot && !cmos_rtc_probe )
> > +        panic("System with no CMOS RTC advertised must be booted from EFI"
> > +              " (or with command line option \"cmos-rtc-probe\")\n");
> > +
> > +    if ( !cmos_probe(&rtc, cmos_rtc_probe) )
> >          panic("No CMOS RTC found - system must be booted from EFI\n");
> >  
> >      return mktime(rtc.year, rtc.mon, rtc.day, rtc.hour, rtc.min, rtc.sec);
> 
> Having seen the series up to here (and already by the previous patch) I think
> I see now where we disagree about the conditional-ness of the probing: I
> suppose you deem only the 2nd and possible subsequent iterations of the loop
> in (now) cmos_probe() as "probing", whereas I consider all of what that
> function now contains as exactly that.
> 
> The difference is becoming more pronounced with the subsequent change of
> preferring CMOS over EFI time: With EFI (with or without ACPI) there never
> was a guarantee that a CMOS clock would exist. Prior to the introduction of
> the ACPI_FADT_NO_CMOS_RTC flag the was a de-facto guarantee that PC-like
> systems would have one. And vendors abusing the flag made us probe, despite
> the port accesses being problematic until we know there actually is a CMOS
> (RTC) there. Hence why I was suggesting that there be a way to bypass the
> CMOS accesses altogether at least when booted from EFI (as is the case
> right now, just without the need for any user override).

With further patches you could use wallclock=efi in order to bypass
any probing of the CMOS.

Also note that the current logic here still keeps the reading of the
CMOS limited to when ACPI_FADT_NO_CMOS_RTC is not set or when
cmos-rtc-probe is present on the command line.

Thanks, Roger.