Re: [PATCH v3 1/7] x86/time: introduce helper to fetch Xen wallclock when running as a guest

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

On Tue, Sep 03, 2024 at 04:48:41PM +0200, Jan Beulich wrote:
> On 03.09.2024 15:02, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -785,6 +785,31 @@ static struct platform_timesource 
> > __initdata_cf_clobber plt_xen_timer =
> >      .resume = resume_xen_timer,
> >      .counter_bits = 63,
> >  };
> > +
> > +static unsigned long read_xen_wallclock(void)
> > +{
> > +    struct shared_info *sh_info = XEN_shared_info;
> > +    uint32_t wc_version;
> > +    uint64_t wc_sec;
> > +
> > +    ASSERT(xen_guest);
> > +
> > +    do {
> > +        wc_version = sh_info->wc_version & ~1;
> > +        smp_rmb();
> > +
> > +        wc_sec  = sh_info->wc_sec;
> > +        smp_rmb();
> > +    } while ( wc_version != sh_info->wc_version );
> > +
> > +    return wc_sec + read_xen_timer() / 1000000000;
> > +}
> > +#else
> > +static unsigned long read_xen_wallclock(void)
> > +{
> > +    ASSERT_UNREACHABLE();
> > +    return 0;
> > +}
> >  #endif
> 
> I understand you try to re-use an existing #ifdef here, but I wonder if I
> could talk you into not doing so and instead placing the #ifdef inside the
> (then single) function body. Less redundancy, less room for mistakes /
> oversights.

I don't mind much, I've assumed reducing the number of #ifdefs blocks
was better, but I don't have a strong opinion.  Initially I just kept
the #ifdef block in get_wallclock_time().

Thanks, Roger.