... in preparation to introduce a second caller.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Ian Jackson <iwj@xenproject.org>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
CC: Frédéric Pierret <frederic.pierret@qubes-os.org>

For 4.15.  Pre-req for trying to unbreak AMD Ryzen 1800X systems.
---
 xen/arch/x86/hpet.c        | 116 ++++++++++++++++++++++++---------------------
 xen/include/asm-x86/hpet.h |   6 +++
 2 files changed, 68 insertions(+), 54 deletions(-)

diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 1ff005fb4a..c73135bb15 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -754,11 +754,70 @@ int hpet_legacy_irq_tick(void)
 }
 
 static u32 *hpet_boot_cfg;
+static u64 __initdata hpet_rate;
+
+bool __init hpet_enable_legacy_replacement_mode(void)
+{
+    unsigned int id, cfg, c0_cfg, ticks, count;
+
+    if ( !hpet_rate ||
+         !((id = hpet_read32(HPET_ID)) & HPET_ID_LEGSUP) ||
+         (cfg = hpet_read32(HPET_CFG) & HPET_CFG_LEGACY) )
+        return false;
+
+    /* Stop the main counter. */
+    hpet_write32(cfg & ~HPET_CFG_ENABLE, HPET_CFG);
+
+    /* Reconfigure channel 0 to be 32bit periodic. */
+    c0_cfg = hpet_read32(HPET_Tn_CFG(0));
+    c0_cfg |= (HPET_TN_ENABLE | HPET_TN_PERIODIC | HPET_TN_SETVAL |
+               HPET_TN_32BIT);
+    hpet_write32(c0_cfg, HPET_Tn_CFG(0));
+
+    /*
+     * The exact period doesn't have to match a legacy PIT.  All we need
+     * is an interrupt queued up via the IO-APIC to check routing.
+     *
+     * Use HZ as the frequency.
+     */
+    ticks = ((SECONDS(1) / HZ) * div_sc(hpet_rate, SECONDS(1), 32)) >> 32;
+
+    count = hpet_read32(HPET_COUNTER);
+
+    /*
+     * HPET_TN_SETVAL above is atrociously documented in the spec.
+     *
+     * Periodic HPET channels have a main comparator register, and
+     * separate "accumulator" register.  Despite being named accumulator
+     * in the spec, this is not an accurate description of its behaviour
+     * or purpose.
+     *
+     * Each time an interrupt is generated, the "accumulator" register is
+     * re-added to the comparator set up the new period.
+     *
+     * Normally, writes to the CMP register update both registers.
+     * However, under these semantics, it is impossible to set up a
+     * periodic timer correctly without the main HPET counter being at 0.
+     *
+     * Instead, HPET_TN_SETVAL is a self-clearing control bit which we can
+     * use for periodic timers to mean that the second write to CMP
+     * updates the accumulator only, and not the absolute comparator
+     * value.
+     *
+     * This lets us set a period when the main counter isn't at 0.
+     */
+    hpet_write32(count + ticks, HPET_Tn_CMP(0));
+    hpet_write32(ticks,         HPET_Tn_CMP(0));
+
+    /* Restart the main counter, and legacy mode. */
+    hpet_write32(cfg | HPET_CFG_ENABLE | HPET_CFG_LEGACY, HPET_CFG);
+
+    return true;
+}
 
 u64 __init hpet_setup(void)
 {
-    static u64 __initdata hpet_rate;
-    unsigned int hpet_id, hpet_period, hpet_cfg;
+    unsigned int hpet_id, hpet_period;
     unsigned int last, rem;
 
     if ( hpet_rate )
@@ -805,58 +864,7 @@ u64 __init hpet_setup(void)
      * Reconfigure the HPET into legacy mode to re-establish the timer
      * interrupt.
      */
-    if ( hpet_id & HPET_ID_LEGSUP &&
-         !((hpet_cfg = hpet_read32(HPET_CFG)) & HPET_CFG_LEGACY) )
-    {
-        unsigned int c0_cfg, ticks, count;
-
-        /* Stop the main counter. */
-        hpet_write32(hpet_cfg & ~HPET_CFG_ENABLE, HPET_CFG);
-
-        /* Reconfigure channel 0 to be 32bit periodic. */
-        c0_cfg = hpet_read32(HPET_Tn_CFG(0));
-        c0_cfg |= (HPET_TN_ENABLE | HPET_TN_PERIODIC | HPET_TN_SETVAL |
-                   HPET_TN_32BIT);
-        hpet_write32(c0_cfg, HPET_Tn_CFG(0));
-
-        /*
-         * The exact period doesn't have to match a legacy PIT.  All we need
-         * is an interrupt queued up via the IO-APIC to check routing.
-         *
-         * Use HZ as the frequency.
-         */
-        ticks = ((SECONDS(1) / HZ) * div_sc(hpet_rate, SECONDS(1), 32)) >> 32;
-
-        count = hpet_read32(HPET_COUNTER);
-
-        /*
-         * HPET_TN_SETVAL above is atrociously documented in the spec.
-         *
-         * Periodic HPET channels have a main comparator register, and
-         * separate "accumulator" register.  Despite being named accumulator
-         * in the spec, this is not an accurate description of its behaviour
-         * or purpose.
-         *
-         * Each time an interrupt is generated, the "accumulator" register is
-         * re-added to the comparator set up the new period.
-         *
-         * Normally, writes to the CMP register update both registers.
-         * However, under these semantics, it is impossible to set up a
-         * periodic timer correctly without the main HPET counter being at 0.
-         *
-         * Instead, HPET_TN_SETVAL is a self-clearing control bit which we can
-         * use for periodic timers to mean that the second write to CMP
-         * updates the accumulator only, and not the absolute comparator
-         * value.
-         *
-         * This lets us set a period when the main counter isn't at 0.
-         */
-        hpet_write32(count + ticks, HPET_Tn_CMP(0));
-        hpet_write32(ticks,         HPET_Tn_CMP(0));
-
-        /* Restart the main counter, and legacy mode. */
-        hpet_write32(hpet_cfg | HPET_CFG_ENABLE | HPET_CFG_LEGACY, HPET_CFG);
-    }
+    hpet_enable_legacy_replacement_mode();
 
     return hpet_rate;
 }
diff --git a/xen/include/asm-x86/hpet.h b/xen/include/asm-x86/hpet.h
index fb6bf05065..50176de3d2 100644
--- a/xen/include/asm-x86/hpet.h
+++ b/xen/include/asm-x86/hpet.h
@@ -73,6 +73,12 @@ void hpet_disable(void);
 int hpet_legacy_irq_tick(void);
 
 /*
+ * Try to enable HPET Legacy Replacement mode.  Returns a boolean indicating
+ * whether the HPET configuration was changed.
+ */
+bool hpet_enable_legacy_replacement_mode(void);
+
+/*
  * Temporarily use an HPET event counter for timer interrupt handling,
  * rather than using the LAPIC timer. Used for Cx state entry.
  */
-- 
2.11.0

On 25.03.2021 17:52, Andrew Cooper wrote:
> ... in preparation to introduce a second caller.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Generally
Reviewed-by: Jan Beulich <jbeulich@suse.com>
but I think there's one small code change needed plus I have two
nits (and I expect that this change wouldn't be committed without
patch 2, as making the function non-static isn't warranted
otherwise):

> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -754,11 +754,70 @@ int hpet_legacy_irq_tick(void)
>  }
>  
>  static u32 *hpet_boot_cfg;
> +static u64 __initdata hpet_rate;

Use uint64_t as you move this here?

> +bool __init hpet_enable_legacy_replacement_mode(void)
> +{
> +    unsigned int id, cfg, c0_cfg, ticks, count;
> +
> +    if ( !hpet_rate ||

I think you need to also honor opt_hpet here.

> +         !((id = hpet_read32(HPET_ID)) & HPET_ID_LEGSUP) ||

I don't think I see a need for the assignment and hence the local
variable. Dropping it would make the code easier to read imo.

Jan

On 26/03/2021 09:59, Jan Beulich wrote:
> On 25.03.2021 17:52, Andrew Cooper wrote:
>> ... in preparation to introduce a second caller.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Generally
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks,

> but I think there's one small code change needed plus I have two
> nits (and I expect that this change wouldn't be committed without
> patch 2, as making the function non-static isn't warranted
> otherwise):

Yeah - I intend these to go in together.

>
>> --- a/xen/arch/x86/hpet.c
>> +++ b/xen/arch/x86/hpet.c
>> @@ -754,11 +754,70 @@ int hpet_legacy_irq_tick(void)
>>  }
>>  
>>  static u32 *hpet_boot_cfg;
>> +static u64 __initdata hpet_rate;
> Use uint64_t as you move this here?

Ok.

>
>> +bool __init hpet_enable_legacy_replacement_mode(void)
>> +{
>> +    unsigned int id, cfg, c0_cfg, ticks, count;
>> +
>> +    if ( !hpet_rate ||
> I think you need to also honor opt_hpet here.

Can't (order of patches), and also no need.

When opt_hpet is introduced, hpet_rate can't become nonzero without
opt_hpet being set.

>
>> +         !((id = hpet_read32(HPET_ID)) & HPET_ID_LEGSUP) ||
> I don't think I see a need for the assignment and hence the local
> variable. Dropping it would make the code easier to read imo.

Ok.

~Andrew

On 26.03.2021 11:53, Andrew Cooper wrote:
> On 26/03/2021 09:59, Jan Beulich wrote:
>> On 25.03.2021 17:52, Andrew Cooper wrote:
>>> +bool __init hpet_enable_legacy_replacement_mode(void)
>>> +{
>>> +    unsigned int id, cfg, c0_cfg, ticks, count;
>>> +
>>> +    if ( !hpet_rate ||
>> I think you need to also honor opt_hpet here.
> 
> Can't (order of patches), and also no need.
> 
> When opt_hpet is introduced, hpet_rate can't become nonzero without
> opt_hpet being set.

Oh, right, sorry: I did mix up hpet_address and hpet_rate.

Jan