Move the current code in get_wallclock_time() to fetch the Xen wallclock
information from the shared page when running as a guest into a separate
helper.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - New in this version.
---
 xen/arch/x86/time.c | 43 ++++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index a97d78484105..d022db4bd4a0 100644
--- 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
 
 #ifdef CONFIG_HYPERV_GUEST
@@ -1497,24 +1522,8 @@ void rtc_guest_write(unsigned int port, unsigned int data)
 
 static unsigned long get_wallclock_time(void)
 {
-#ifdef CONFIG_XEN_GUEST
     if ( xen_guest )
-    {
-        struct shared_info *sh_info = XEN_shared_info;
-        uint32_t wc_version;
-        uint64_t wc_sec;
-
-        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;
-    }
-#endif
+        return read_xen_wallclock();
 
     return get_cmos_time();
 }
-- 
2.46.0

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.

Jan

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.