[PATCH v3 for-4.21 0/9] x86/HPET: broadcast IRQ and other improvements

[PATCH v3 for-4.21 0/9] x86/HPET: broadcast IRQ and other improvements

[Jan Beulich]

While 1db7829e5657 ("x86/hpet: do local APIC EOI after interrupt processing")
helped quite a bit, nested interrupts could still occur. First and foremost
as a result of IRQ migration (where we don't have any control over the vectors
chosen). Hence besides reducing the number of IRQs that can be raised (first
patch), the main goal here is to eliminate the potential for nested IRQs
(patch 2). These patches are imo 4.21 candidates. Patches 3 and onwards likely
aren't important enough anymore at this point of the release cycle, even if
those with a Fixes: tag may end up being backported later on.

v3 has again a fair bit of rework of (not only) the "main" patch. See
individual patches for details.

1: deal with unused channels
2: use single, global, low-priority vector for broadcast IRQ
3: replace handle_hpet_broadcast()'s on-stack cpumask_t
4: avoid indirect call to event handler
5: make another channel flags update atomic
6: move legacy tick IRQ count adjustment
7: reduce hpet_next_event() call sites
8: drop "long timeout" handling from reprogram_hpet_evt_channel()
9: simplify "expire" check a little in reprogram_hpet_evt_channel()

Jan

[PATCH v3 for-4.21 1/9] x86/HPET: deal with unused channels

[Jan Beulich]

Keeping channels enabled when they're unused is only causing problems:
Extra interrupts harm performance, and extra nested interrupts could even
have caused worse problems. However, on all Intel hardware I looked at
closely, a 0->1 transition of the enable bit causes an immediate IRQ.
Hence disabling channels isn't a good idea there. Set a "long" timeout
instead.

Along with that also "clear" the channel's "next event", for it to be
properly written by whatever the next user is going to want (possibly
avoiding too early an IRQ).

Further, along the same lines, don't enable channels early when starting
up an IRQ. This doesn't need to happen earlier than from
set_channel_irq_affinity() (once a channel goes into use the very first
time). This eliminates a single instance of

(XEN) [VT-D]INTR-REMAP: Request device [0000:00:1f.0] fault index 0
(XEN) [VT-D]INTR-REMAP: reason 25 - Blocked a compatibility format interrupt request

during boot. (Why exactly there's only one instance, when we use multiple
counters and hence multiple IRQs, I can't tell. My understanding would be
that this was due to __hpet_setup_msi_irq() being called only after
request_irq() [and hence the .startup handler], yet that should have
affected all channels.)

Fixes: 3ba523ff957c ("CPUIDLE: enable MSI capable HPET for timer broadcast")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Release-Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
---
A window still remains for IRQs to be caused by stale comparator values:
hpet_attach_channel() is called ahead of reprogram_hpet_evt_channel().
Should we also write the comparator to "far into the future"?

Furthermore this prolongues the window until "old" vectors may be released
again, as this way we potentially (and intentionally) delay the ocurrence
of the next IRQ for the channel in question. (This issue will disappear
once we switch to a fixed, global vector.)
---
v3: Don't disable channels, only set long timeouts.

--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -295,12 +295,6 @@ static int hpet_msi_write(struct hpet_ev
     return 0;
 }
 
-static unsigned int cf_check hpet_msi_startup(struct irq_desc *desc)
-{
-    hpet_msi_unmask(desc);
-    return 0;
-}
-
 #define hpet_msi_shutdown hpet_msi_mask
 
 static void cf_check hpet_msi_set_affinity(
@@ -326,7 +320,7 @@ static void cf_check hpet_msi_set_affini
  */
 static hw_irq_controller hpet_msi_type = {
     .typename   = "HPET-MSI",
-    .startup    = hpet_msi_startup,
+    .startup    = irq_startup_none,
     .shutdown   = hpet_msi_shutdown,
     .enable	    = hpet_msi_unmask,
     .disable    = hpet_msi_mask,
@@ -526,6 +520,8 @@ static void hpet_detach_channel(unsigned
         spin_unlock_irq(&ch->lock);
     else if ( (next = cpumask_first(ch->cpumask)) >= nr_cpu_ids )
     {
+        hpet_write32(hpet_read32(HPET_COUNTER), HPET_Tn_CMP(ch->idx));
+        ch->next_event = STIME_MAX;
         ch->cpu = -1;
         clear_bit(HPET_EVT_USED_BIT, &ch->flags);
         spin_unlock_irq(&ch->lock);

Re: [PATCH v3 for-4.21 1/9] x86/HPET: deal with unused channels

[Roger Pau Monné]

On Thu, Oct 23, 2025 at 05:49:57PM +0200, Jan Beulich wrote:
> Keeping channels enabled when they're unused is only causing problems:
> Extra interrupts harm performance, and extra nested interrupts could even
> have caused worse problems. However, on all Intel hardware I looked at
> closely, a 0->1 transition of the enable bit causes an immediate IRQ.
> Hence disabling channels isn't a good idea there. Set a "long" timeout
> instead.
> 
> Along with that also "clear" the channel's "next event", for it to be
> properly written by whatever the next user is going to want (possibly
> avoiding too early an IRQ).
> 
> Further, along the same lines, don't enable channels early when starting
> up an IRQ. This doesn't need to happen earlier than from
> set_channel_irq_affinity() (once a channel goes into use the very first
> time). This eliminates a single instance of
> 
> (XEN) [VT-D]INTR-REMAP: Request device [0000:00:1f.0] fault index 0
> (XEN) [VT-D]INTR-REMAP: reason 25 - Blocked a compatibility format interrupt request
> 
> during boot. (Why exactly there's only one instance, when we use multiple
> counters and hence multiple IRQs, I can't tell. My understanding would be
> that this was due to __hpet_setup_msi_irq() being called only after
> request_irq() [and hence the .startup handler], yet that should have
> affected all channels.)
> 
> Fixes: 3ba523ff957c ("CPUIDLE: enable MSI capable HPET for timer broadcast")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Release-Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

[PATCH v3 for-4.21 2/9] x86/HPET: use single, global, low-priority vector for broadcast IRQ

[Jan Beulich]

Using dynamically allocated / maintained vectors has several downsides:
- possible nesting of IRQs due to the effects of IRQ migration,
- reduction of vectors available for devices,
- IRQs not moving as intended if there's shortage of vectors,
- higher runtime overhead.

As the vector also doesn't need to be of any priority (first and foremost
it really shouldn't be of higher or same priority as the timer IRQ, as
that raises TIMER_SOFTIRQ anyway), simply use the lowest one above the
legacy range. The vector needs reserving early, until it is known whether
it actually is used. If it isn't, it's made available for general use.

With a fixed vector, less updating is now necessary in
set_channel_irq_affinity(); in particular channels don't need transiently
masking anymore, as the necessary update is now atomic. To fully leverage
this, however, we want to stop using hpet_msi_set_affinity() there. With
the transient masking dropped, we're no longer at risk of missing events.

Fixes: 996576b965cc ("xen: allow up to 16383 cpus")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Release-Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
---
This is an alternative proposal to
https://lists.xen.org/archives/html/xen-devel/2014-03/msg00399.html.

Should we keep hpet_msi_set_affinity() at all? We'd better not have the
generic IRQ subsystem play with our IRQs' affinities, and fixup_irqs()
isn't relevant here. (If so, this likely would want to be a separate
patch, though.)

The hpet_enable_channel() call could in principle be made (effectively)
conditional, at the price of introducing a check in there. However, as
much as eliminating the masking didn't help with the many excess (early)
IRQs I'm observing on Intel hardware, doing so doesn't help either.

The Fixes: tag indicates where the problem got signficantly worse; in
principle it was there already before (crashing at perhaps 6 or 7 levels
of nested IRQs).
---
v3: Switch to using vector 0x30, to unbreak AMD, including an adjustment
    to AMD IOMMU intremap logic. Adjust condition around assertions in
    set_channel_irq_affinity().
v2: Re-work set_channel_irq_affinity() intensively. Re-base over the
    dropping of another patch. Drop setup_vector_irq() change.

--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -9,17 +9,19 @@
 #include <xen/timer.h>
 #include <xen/smp.h>
 #include <xen/softirq.h>
+#include <xen/cpuidle.h>
 #include <xen/irq.h>
 #include <xen/numa.h>
 #include <xen/param.h>
 #include <xen/sched.h>
 
 #include <asm/apic.h>
-#include <asm/fixmap.h>
 #include <asm/div64.h>
+#include <asm/fixmap.h>
+#include <asm/genapic.h>
 #include <asm/hpet.h>
+#include <asm/irq-vectors.h>
 #include <asm/msi.h>
-#include <xen/cpuidle.h>
 
 #define MAX_DELTA_NS MILLISECS(10*1000)
 #define MIN_DELTA_NS MICROSECS(20)
@@ -251,10 +253,9 @@ static void cf_check hpet_interrupt_hand
     ch->event_handler(ch);
 }
 
-static void cf_check hpet_msi_unmask(struct irq_desc *desc)
+static void hpet_enable_channel(struct hpet_event_channel *ch)
 {
     u32 cfg;
-    struct hpet_event_channel *ch = desc->action->dev_id;
 
     cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
     cfg |= HPET_TN_ENABLE;
@@ -262,6 +263,11 @@ static void cf_check hpet_msi_unmask(str
     ch->msi.msi_attrib.host_masked = 0;
 }
 
+static void cf_check hpet_msi_unmask(struct irq_desc *desc)
+{
+    hpet_enable_channel(desc->action->dev_id);
+}
+
 static void cf_check hpet_msi_mask(struct irq_desc *desc)
 {
     u32 cfg;
@@ -303,15 +309,13 @@ static void cf_check hpet_msi_set_affini
     struct hpet_event_channel *ch = desc->action->dev_id;
     struct msi_msg msg = ch->msi.msg;
 
-    msg.dest32 = set_desc_affinity(desc, mask);
-    if ( msg.dest32 == BAD_APICID )
-        return;
+    /* This really is only for dump_irqs(). */
+    cpumask_copy(desc->arch.cpu_mask, mask);
 
-    msg.data &= ~MSI_DATA_VECTOR_MASK;
-    msg.data |= MSI_DATA_VECTOR(desc->arch.vector);
+    msg.dest32 = cpu_mask_to_apicid(mask);
     msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
     msg.address_lo |= MSI_ADDR_DEST_ID(msg.dest32);
-    if ( msg.data != ch->msi.msg.data || msg.dest32 != ch->msi.msg.dest32 )
+    if ( msg.dest32 != ch->msi.msg.dest32 )
         hpet_msi_write(ch, &msg);
 }
 
@@ -324,7 +328,7 @@ static hw_irq_controller hpet_msi_type =
     .shutdown   = hpet_msi_shutdown,
     .enable	    = hpet_msi_unmask,
     .disable    = hpet_msi_mask,
-    .ack        = ack_nonmaskable_msi_irq,
+    .ack        = irq_actor_none,
     .end        = end_nonmaskable_irq,
     .set_affinity   = hpet_msi_set_affinity,
 };
@@ -343,6 +347,12 @@ static int __init hpet_setup_msi_irq(str
     u32 cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
     irq_desc_t *desc = irq_to_desc(ch->msi.irq);
 
+    clear_irq_vector(ch->msi.irq);
+    ret = bind_irq_vector(ch->msi.irq, HPET_BROADCAST_VECTOR, &cpu_online_map);
+    if ( ret )
+        return ret;
+    cpumask_setall(desc->affinity);
+
     if ( iommu_intremap != iommu_intremap_off )
     {
         ch->msi.hpet_id = hpet_blockid;
@@ -472,19 +482,50 @@ static struct hpet_event_channel *hpet_g
 static void set_channel_irq_affinity(struct hpet_event_channel *ch)
 {
     struct irq_desc *desc = irq_to_desc(ch->msi.irq);
+    struct msi_msg msg = ch->msi.msg;
 
     ASSERT(!local_irq_is_enabled());
     spin_lock(&desc->lock);
-    hpet_msi_mask(desc);
-    hpet_msi_set_affinity(desc, cpumask_of(ch->cpu));
-    hpet_msi_unmask(desc);
+
+    per_cpu(vector_irq, ch->cpu)[HPET_BROADCAST_VECTOR] = ch->msi.irq;
+
+    /*
+     * Open-coding a reduced form of hpet_msi_set_affinity() here.  With the
+     * actual update below (either of the IRTE or of [just] message address;
+     * with interrupt remapping message address/data don't change) now being
+     * atomic, we can avoid masking the IRQ around the update.  As a result
+     * we're no longer at risk of missing IRQs (provided hpet_broadcast_enter()
+     * keeps setting the new deadline only afterwards).
+     */
+    cpumask_copy(desc->arch.cpu_mask, cpumask_of(ch->cpu));
+
     spin_unlock(&desc->lock);
 
-    spin_unlock(&ch->lock);
+    msg.dest32 = cpu_physical_id(ch->cpu);
+    msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
+    msg.address_lo |= MSI_ADDR_DEST_ID(msg.dest32);
+    if ( msg.dest32 != ch->msi.msg.dest32 )
+    {
+        ch->msi.msg = msg;
 
-    /* We may have missed an interrupt due to the temporary masking. */
-    if ( ch->event_handler && ch->next_event < NOW() )
-        ch->event_handler(ch);
+        if ( iommu_intremap != iommu_intremap_off )
+        {
+            int rc = iommu_update_ire_from_msi(&ch->msi, &msg);
+
+            ASSERT(rc <= 0);
+            if ( rc >= 0 )
+            {
+                ASSERT(msg.data == hpet_read32(HPET_Tn_ROUTE(ch->idx)));
+                ASSERT(msg.address_lo ==
+                       hpet_read32(HPET_Tn_ROUTE(ch->idx) + 4));
+            }
+        }
+        else
+            hpet_write32(msg.address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
+    }
+
+    hpet_enable_channel(ch);
+    spin_unlock(&ch->lock);
 }
 
 static void hpet_attach_channel(unsigned int cpu,
@@ -622,6 +663,12 @@ void __init hpet_broadcast_init(void)
         hpet_events->flags = HPET_EVT_LEGACY;
 }
 
+void __init hpet_broadcast_late_init(void)
+{
+    if ( !num_hpets_used )
+        free_lopriority_vector(HPET_BROADCAST_VECTOR);
+}
+
 void hpet_broadcast_resume(void)
 {
     u32 cfg;
--- a/xen/arch/x86/include/asm/hpet.h
+++ b/xen/arch/x86/include/asm/hpet.h
@@ -90,6 +90,7 @@ void hpet_disable_legacy_replacement_mod
  * rather than using the LAPIC timer. Used for Cx state entry.
  */
 void hpet_broadcast_init(void);
+void hpet_broadcast_late_init(void);
 void hpet_broadcast_resume(void);
 void cf_check hpet_broadcast_enter(void);
 void cf_check hpet_broadcast_exit(void);
--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -116,6 +116,7 @@ void cf_check call_function_interrupt(vo
 void cf_check irq_move_cleanup_interrupt(void);
 
 uint8_t alloc_hipriority_vector(void);
+void free_lopriority_vector(uint8_t vector);
 
 void set_direct_apic_vector(uint8_t vector, void (*handler)(void));
 void alloc_direct_apic_vector(uint8_t *vector, void (*handler)(void));
--- a/xen/arch/x86/include/asm/irq-vectors.h
+++ b/xen/arch/x86/include/asm/irq-vectors.h
@@ -22,6 +22,9 @@
 #define FIRST_LEGACY_VECTOR     FIRST_DYNAMIC_VECTOR
 #define LAST_LEGACY_VECTOR      (FIRST_LEGACY_VECTOR + 0xf)
 
+/* HPET broadcast is statically allocated and wants to be low priority. */
+#define HPET_BROADCAST_VECTOR   (LAST_LEGACY_VECTOR + 1)
+
 #ifdef CONFIG_PV32
 #define HYPERCALL_VECTOR        0x82
 #endif
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -468,6 +468,12 @@ int __init init_irq_data(void)
           vector++ )
         __set_bit(vector, used_vectors);
 
+    /*
+     * Prevent the HPET broadcast vector from being used, until it is known
+     * whether it's actually needed.
+     */
+    __set_bit(HPET_BROADCAST_VECTOR, used_vectors);
+
     return 0;
 }
 
@@ -991,6 +997,13 @@ void alloc_direct_apic_vector(uint8_t *v
     spin_unlock(&lock);
 }
 
+/* This could free any vectors, but is needed only for low-prio ones. */
+void __init free_lopriority_vector(uint8_t vector)
+{
+    ASSERT(vector < FIRST_HIPRIORITY_VECTOR);
+    clear_bit(vector, used_vectors);
+}
+
 static void cf_check irq_ratelimit_timer_fn(void *data)
 {
     struct irq_desc *desc, *tmp;
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -2675,6 +2675,8 @@ static int __init cf_check disable_pit_i
                "Force enable with 'cpuidle'.\n");
     }
 
+    hpet_broadcast_late_init();
+
     return 0;
 }
 __initcall(disable_pit_irq);
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -551,6 +551,13 @@ int cf_check amd_iommu_msi_msg_update_ir
         for ( i = 1; i < nr; ++i )
             msi_desc[i].remap_index = msi_desc->remap_index + i;
         msg->data = data;
+        /*
+         * While the low address bits don't matter, "canonicalize" the address
+         * by zapping the bits that were transferred to the IRTE.  This way
+         * callers can check for there actually needing to be an update to
+         * wherever the address is put.
+         */
+        msg->address_lo &= ~(MSI_ADDR_DESTMODE_MASK | MSI_ADDR_DEST_ID_MASK);
     }
 
     return rc;

Re: [PATCH v3 for-4.21 2/9] x86/HPET: use single, global, low-priority vector for broadcast IRQ

[Roger Pau Monné]

On Thu, Oct 23, 2025 at 05:50:17PM +0200, Jan Beulich wrote:
> Using dynamically allocated / maintained vectors has several downsides:
> - possible nesting of IRQs due to the effects of IRQ migration,
> - reduction of vectors available for devices,
> - IRQs not moving as intended if there's shortage of vectors,
> - higher runtime overhead.
> 
> As the vector also doesn't need to be of any priority (first and foremost
> it really shouldn't be of higher or same priority as the timer IRQ, as
> that raises TIMER_SOFTIRQ anyway), simply use the lowest one above the
> legacy range. The vector needs reserving early, until it is known whether
> it actually is used. If it isn't, it's made available for general use.
> 
> With a fixed vector, less updating is now necessary in
> set_channel_irq_affinity(); in particular channels don't need transiently
> masking anymore, as the necessary update is now atomic. To fully leverage
> this, however, we want to stop using hpet_msi_set_affinity() there. With
> the transient masking dropped, we're no longer at risk of missing events.
> 
> Fixes: 996576b965cc ("xen: allow up to 16383 cpus")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Release-Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
                                    ^ space?

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I've got some comments below, but they shouldn't affect functionality.
I leave it up to you whether you think some of those might be
beneficial.

> ---
> This is an alternative proposal to
> https://lists.xen.org/archives/html/xen-devel/2014-03/msg00399.html.
> 
> Should we keep hpet_msi_set_affinity() at all? We'd better not have the
> generic IRQ subsystem play with our IRQs' affinities, and fixup_irqs()
> isn't relevant here. (If so, this likely would want to be a separate
> patch, though.)
> 
> The hpet_enable_channel() call could in principle be made (effectively)
> conditional, at the price of introducing a check in there. However, as
> much as eliminating the masking didn't help with the many excess (early)
> IRQs I'm observing on Intel hardware, doing so doesn't help either.
> 
> The Fixes: tag indicates where the problem got signficantly worse; in
> principle it was there already before (crashing at perhaps 6 or 7 levels
> of nested IRQs).
> ---
> v3: Switch to using vector 0x30, to unbreak AMD, including an adjustment
>     to AMD IOMMU intremap logic. Adjust condition around assertions in
>     set_channel_irq_affinity().
> v2: Re-work set_channel_irq_affinity() intensively. Re-base over the
>     dropping of another patch. Drop setup_vector_irq() change.
> 
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -9,17 +9,19 @@
>  #include <xen/timer.h>
>  #include <xen/smp.h>
>  #include <xen/softirq.h>
> +#include <xen/cpuidle.h>
>  #include <xen/irq.h>
>  #include <xen/numa.h>
>  #include <xen/param.h>
>  #include <xen/sched.h>
>  
>  #include <asm/apic.h>
> -#include <asm/fixmap.h>
>  #include <asm/div64.h>
> +#include <asm/fixmap.h>
> +#include <asm/genapic.h>
>  #include <asm/hpet.h>
> +#include <asm/irq-vectors.h>
>  #include <asm/msi.h>
> -#include <xen/cpuidle.h>
>  
>  #define MAX_DELTA_NS MILLISECS(10*1000)
>  #define MIN_DELTA_NS MICROSECS(20)
> @@ -251,10 +253,9 @@ static void cf_check hpet_interrupt_hand
>      ch->event_handler(ch);
>  }
>  
> -static void cf_check hpet_msi_unmask(struct irq_desc *desc)
> +static void hpet_enable_channel(struct hpet_event_channel *ch)
>  {
>      u32 cfg;
> -    struct hpet_event_channel *ch = desc->action->dev_id;
>  
>      cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
>      cfg |= HPET_TN_ENABLE;
> @@ -262,6 +263,11 @@ static void cf_check hpet_msi_unmask(str
>      ch->msi.msi_attrib.host_masked = 0;
>  }
>  
> +static void cf_check hpet_msi_unmask(struct irq_desc *desc)
> +{
> +    hpet_enable_channel(desc->action->dev_id);
> +}
> +
>  static void cf_check hpet_msi_mask(struct irq_desc *desc)
>  {
>      u32 cfg;
> @@ -303,15 +309,13 @@ static void cf_check hpet_msi_set_affini
>      struct hpet_event_channel *ch = desc->action->dev_id;
>      struct msi_msg msg = ch->msi.msg;
>  
> -    msg.dest32 = set_desc_affinity(desc, mask);
> -    if ( msg.dest32 == BAD_APICID )
> -        return;
> +    /* This really is only for dump_irqs(). */
> +    cpumask_copy(desc->arch.cpu_mask, mask);

To add some extra checks here for correctness, do you think it would
be helpful to add:

ASSERT(cpumask_weight(mask) == 1);
ASSERT(cpumask_intersects(mask, &cpu_online_mask));

Or that's too pedantic?

>  
> -    msg.data &= ~MSI_DATA_VECTOR_MASK;
> -    msg.data |= MSI_DATA_VECTOR(desc->arch.vector);
> +    msg.dest32 = cpu_mask_to_apicid(mask);
>      msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
>      msg.address_lo |= MSI_ADDR_DEST_ID(msg.dest32);
> -    if ( msg.data != ch->msi.msg.data || msg.dest32 != ch->msi.msg.dest32 )
> +    if ( msg.dest32 != ch->msi.msg.dest32 )
>          hpet_msi_write(ch, &msg);
>  }
>  
> @@ -324,7 +328,7 @@ static hw_irq_controller hpet_msi_type =
>      .shutdown   = hpet_msi_shutdown,
>      .enable	    = hpet_msi_unmask,
>      .disable    = hpet_msi_mask,
> -    .ack        = ack_nonmaskable_msi_irq,
> +    .ack        = irq_actor_none,
>      .end        = end_nonmaskable_irq,
>      .set_affinity   = hpet_msi_set_affinity,
>  };
> @@ -343,6 +347,12 @@ static int __init hpet_setup_msi_irq(str
>      u32 cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
>      irq_desc_t *desc = irq_to_desc(ch->msi.irq);
>  
> +    clear_irq_vector(ch->msi.irq);
> +    ret = bind_irq_vector(ch->msi.irq, HPET_BROADCAST_VECTOR, &cpu_online_map);

By passing cpu_online_map here, it leads to _bind_irq_vector() doing:

cpumask_copy(desc->arch.cpu_mask, &cpu_online_map);

Which strictly speaking is wrong.  However this is just a cosmetic
issue until the irq is used for the first time, at which point it will
be assigned to a concrete CPU.

You could do:

cpumask_clear(desc->arch.cpu_mask);
cpumask_set_cpu(cpumask_any(&cpu_online_map), desc->arch.cpu_mask);

(Or equivalent)

To assign the interrupt to a concrete CPU and reflex it on the
cpu_mask after the bind_irq_vector() call, but I can live with it
being like this.  I have patches to adjust _bind_irq_vector() myself,
which I hope I will be able to post soon.

> +    if ( ret )
> +        return ret;
> +    cpumask_setall(desc->affinity);
> +
>      if ( iommu_intremap != iommu_intremap_off )
>      {
>          ch->msi.hpet_id = hpet_blockid;
> @@ -472,19 +482,50 @@ static struct hpet_event_channel *hpet_g
>  static void set_channel_irq_affinity(struct hpet_event_channel *ch)
>  {
>      struct irq_desc *desc = irq_to_desc(ch->msi.irq);
> +    struct msi_msg msg = ch->msi.msg;
>  
>      ASSERT(!local_irq_is_enabled());
>      spin_lock(&desc->lock);
> -    hpet_msi_mask(desc);
> -    hpet_msi_set_affinity(desc, cpumask_of(ch->cpu));
> -    hpet_msi_unmask(desc);
> +
> +    per_cpu(vector_irq, ch->cpu)[HPET_BROADCAST_VECTOR] = ch->msi.irq;
> +
> +    /*
> +     * Open-coding a reduced form of hpet_msi_set_affinity() here.  With the
> +     * actual update below (either of the IRTE or of [just] message address;
> +     * with interrupt remapping message address/data don't change) now being
> +     * atomic, we can avoid masking the IRQ around the update.  As a result
> +     * we're no longer at risk of missing IRQs (provided hpet_broadcast_enter()
> +     * keeps setting the new deadline only afterwards).
> +     */
> +    cpumask_copy(desc->arch.cpu_mask, cpumask_of(ch->cpu));
> +
>      spin_unlock(&desc->lock);
>  
> -    spin_unlock(&ch->lock);
> +    msg.dest32 = cpu_physical_id(ch->cpu);
> +    msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
> +    msg.address_lo |= MSI_ADDR_DEST_ID(msg.dest32);
> +    if ( msg.dest32 != ch->msi.msg.dest32 )
> +    {
> +        ch->msi.msg = msg;
>  
> -    /* We may have missed an interrupt due to the temporary masking. */
> -    if ( ch->event_handler && ch->next_event < NOW() )
> -        ch->event_handler(ch);
> +        if ( iommu_intremap != iommu_intremap_off )
> +        {
> +            int rc = iommu_update_ire_from_msi(&ch->msi, &msg);
> +
> +            ASSERT(rc <= 0);
> +            if ( rc >= 0 )

I don't think the rc > 0 part of this check is meaningful, as any rc
value > 0 will trigger the ASSERT(rc <= 0) ahead of it.  The code
inside of the if block itself only contains ASSERTs, so it's only
relevant for debug=y builds that will also have the rc <= 0 ASSERT.

You could possibly use:

ASSERT(rc <= 0);
if ( !rc )
{
    ASSERT(...

And achieve the same result?

> +            {
> +                ASSERT(msg.data == hpet_read32(HPET_Tn_ROUTE(ch->idx)));
> +                ASSERT(msg.address_lo ==
> +                       hpet_read32(HPET_Tn_ROUTE(ch->idx) + 4));
> +            }
> +        }
> +        else
> +            hpet_write32(msg.address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
> +    }
> +
> +    hpet_enable_channel(ch);
> +    spin_unlock(&ch->lock);
>  }
>  
>  static void hpet_attach_channel(unsigned int cpu,
> @@ -622,6 +663,12 @@ void __init hpet_broadcast_init(void)
>          hpet_events->flags = HPET_EVT_LEGACY;
>  }
>  
> +void __init hpet_broadcast_late_init(void)
> +{
> +    if ( !num_hpets_used )
> +        free_lopriority_vector(HPET_BROADCAST_VECTOR);
> +}
> +
>  void hpet_broadcast_resume(void)
>  {
>      u32 cfg;
> --- a/xen/arch/x86/include/asm/hpet.h
> +++ b/xen/arch/x86/include/asm/hpet.h
> @@ -90,6 +90,7 @@ void hpet_disable_legacy_replacement_mod
>   * rather than using the LAPIC timer. Used for Cx state entry.
>   */
>  void hpet_broadcast_init(void);
> +void hpet_broadcast_late_init(void);
>  void hpet_broadcast_resume(void);
>  void cf_check hpet_broadcast_enter(void);
>  void cf_check hpet_broadcast_exit(void);
> --- a/xen/arch/x86/include/asm/irq.h
> +++ b/xen/arch/x86/include/asm/irq.h
> @@ -116,6 +116,7 @@ void cf_check call_function_interrupt(vo
>  void cf_check irq_move_cleanup_interrupt(void);
>  
>  uint8_t alloc_hipriority_vector(void);
> +void free_lopriority_vector(uint8_t vector);
>  
>  void set_direct_apic_vector(uint8_t vector, void (*handler)(void));
>  void alloc_direct_apic_vector(uint8_t *vector, void (*handler)(void));
> --- a/xen/arch/x86/include/asm/irq-vectors.h
> +++ b/xen/arch/x86/include/asm/irq-vectors.h
> @@ -22,6 +22,9 @@
>  #define FIRST_LEGACY_VECTOR     FIRST_DYNAMIC_VECTOR
>  #define LAST_LEGACY_VECTOR      (FIRST_LEGACY_VECTOR + 0xf)
>  
> +/* HPET broadcast is statically allocated and wants to be low priority. */
> +#define HPET_BROADCAST_VECTOR   (LAST_LEGACY_VECTOR + 1)
> +
>  #ifdef CONFIG_PV32
>  #define HYPERCALL_VECTOR        0x82
>  #endif
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -468,6 +468,12 @@ int __init init_irq_data(void)
>            vector++ )
>          __set_bit(vector, used_vectors);
>  
> +    /*
> +     * Prevent the HPET broadcast vector from being used, until it is known
> +     * whether it's actually needed.
> +     */
> +    __set_bit(HPET_BROADCAST_VECTOR, used_vectors);
> +
>      return 0;
>  }
>  
> @@ -991,6 +997,13 @@ void alloc_direct_apic_vector(uint8_t *v
>      spin_unlock(&lock);
>  }
>  
> +/* This could free any vectors, but is needed only for low-prio ones. */
> +void __init free_lopriority_vector(uint8_t vector)
> +{
> +    ASSERT(vector < FIRST_HIPRIORITY_VECTOR);
> +    clear_bit(vector, used_vectors);
> +}

I'm undecided whether we want to have such helper.  This is all very
specific to the single use by the HPET vector, and hence might be best
to simply put the clear_bit() inside of hpet_broadcast_late_init()
itself.

I could see for example other callers wanting to use this also
requiring cleanup of the per cpu vector_irq arrays.  Given it's (so
far) very limited usage it might be clearer to open-code the
clear_bit().

> +
>  static void cf_check irq_ratelimit_timer_fn(void *data)
>  {
>      struct irq_desc *desc, *tmp;
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -2675,6 +2675,8 @@ static int __init cf_check disable_pit_i
>                 "Force enable with 'cpuidle'.\n");
>      }
>  
> +    hpet_broadcast_late_init();
> +
>      return 0;
>  }
>  __initcall(disable_pit_irq);
> --- a/xen/drivers/passthrough/amd/iommu_intr.c
> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> @@ -551,6 +551,13 @@ int cf_check amd_iommu_msi_msg_update_ir
>          for ( i = 1; i < nr; ++i )
>              msi_desc[i].remap_index = msi_desc->remap_index + i;
>          msg->data = data;
> +        /*
> +         * While the low address bits don't matter, "canonicalize" the address
> +         * by zapping the bits that were transferred to the IRTE.  This way
> +         * callers can check for there actually needing to be an update to
> +         * wherever the address is put.
> +         */
> +        msg->address_lo &= ~(MSI_ADDR_DESTMODE_MASK | MSI_ADDR_DEST_ID_MASK);

You might want to mention this change on the commit message also, as
it could look unrelated to the rest of the code?

Thanks, Roger.

Re: [PATCH v3 for-4.21 2/9] x86/HPET: use single, global, low-priority vector for broadcast IRQ

[Jan Beulich]

On 24.10.2025 15:24, Roger Pau Monné wrote:
> On Thu, Oct 23, 2025 at 05:50:17PM +0200, Jan Beulich wrote:
>> Using dynamically allocated / maintained vectors has several downsides:
>> - possible nesting of IRQs due to the effects of IRQ migration,
>> - reduction of vectors available for devices,
>> - IRQs not moving as intended if there's shortage of vectors,
>> - higher runtime overhead.
>>
>> As the vector also doesn't need to be of any priority (first and foremost
>> it really shouldn't be of higher or same priority as the timer IRQ, as
>> that raises TIMER_SOFTIRQ anyway), simply use the lowest one above the
>> legacy range. The vector needs reserving early, until it is known whether
>> it actually is used. If it isn't, it's made available for general use.
>>
>> With a fixed vector, less updating is now necessary in
>> set_channel_irq_affinity(); in particular channels don't need transiently
>> masking anymore, as the necessary update is now atomic. To fully leverage
>> this, however, we want to stop using hpet_msi_set_affinity() there. With
>> the transient masking dropped, we're no longer at risk of missing events.
>>
>> Fixes: 996576b965cc ("xen: allow up to 16383 cpus")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Release-Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
>                                     ^ space?

Looks like I simply took what was provided; I've added the blank now (also
in patch 1).

> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> @@ -303,15 +309,13 @@ static void cf_check hpet_msi_set_affini
>>      struct hpet_event_channel *ch = desc->action->dev_id;
>>      struct msi_msg msg = ch->msi.msg;
>>  
>> -    msg.dest32 = set_desc_affinity(desc, mask);
>> -    if ( msg.dest32 == BAD_APICID )
>> -        return;
>> +    /* This really is only for dump_irqs(). */
>> +    cpumask_copy(desc->arch.cpu_mask, mask);
> 
> To add some extra checks here for correctness, do you think it would
> be helpful to add:
> 
> ASSERT(cpumask_weight(mask) == 1);
> ASSERT(cpumask_intersects(mask, &cpu_online_mask));
> 
> Or that's too pedantic?

Imo that would be pretty pointless in particular since the function is to go
away anyway.

>> @@ -343,6 +347,12 @@ static int __init hpet_setup_msi_irq(str
>>      u32 cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
>>      irq_desc_t *desc = irq_to_desc(ch->msi.irq);
>>  
>> +    clear_irq_vector(ch->msi.irq);
>> +    ret = bind_irq_vector(ch->msi.irq, HPET_BROADCAST_VECTOR, &cpu_online_map);
> 
> By passing cpu_online_map here, it leads to _bind_irq_vector() doing:
> 
> cpumask_copy(desc->arch.cpu_mask, &cpu_online_map);
> 
> Which strictly speaking is wrong.  However this is just a cosmetic
> issue until the irq is used for the first time, at which point it will
> be assigned to a concrete CPU.
> 
> You could do:
> 
> cpumask_clear(desc->arch.cpu_mask);
> cpumask_set_cpu(cpumask_any(&cpu_online_map), desc->arch.cpu_mask);
> 
> (Or equivalent)
> 
> To assign the interrupt to a concrete CPU and reflex it on the
> cpu_mask after the bind_irq_vector() call, but I can live with it
> being like this.  I have patches to adjust _bind_irq_vector() myself,
> which I hope I will be able to post soon.

Hmm, I wrongly memorized hpet_broadcast_init() as being pre-SMP-init only.
It has three call sites:
- mwait_idle_init(), called from cpuidle_presmp_init(),
- amd_cpuidle_init(), calling in only when invoked the very first time,
  which is again from cpuidle_presmp_init(),
- _disable_pit_irq(), called from the regular initcall disable_pit_irq().
I.e. for the latter you're right that the CPU mask is too broad (in only a
cosmetic way though). Would be you okay if I used cpumask_of(0) in place
of &cpu_online_map?

>> @@ -472,19 +482,50 @@ static struct hpet_event_channel *hpet_g
>>  static void set_channel_irq_affinity(struct hpet_event_channel *ch)
>>  {
>>      struct irq_desc *desc = irq_to_desc(ch->msi.irq);
>> +    struct msi_msg msg = ch->msi.msg;
>>  
>>      ASSERT(!local_irq_is_enabled());
>>      spin_lock(&desc->lock);
>> -    hpet_msi_mask(desc);
>> -    hpet_msi_set_affinity(desc, cpumask_of(ch->cpu));
>> -    hpet_msi_unmask(desc);
>> +
>> +    per_cpu(vector_irq, ch->cpu)[HPET_BROADCAST_VECTOR] = ch->msi.irq;
>> +
>> +    /*
>> +     * Open-coding a reduced form of hpet_msi_set_affinity() here.  With the
>> +     * actual update below (either of the IRTE or of [just] message address;
>> +     * with interrupt remapping message address/data don't change) now being
>> +     * atomic, we can avoid masking the IRQ around the update.  As a result
>> +     * we're no longer at risk of missing IRQs (provided hpet_broadcast_enter()
>> +     * keeps setting the new deadline only afterwards).
>> +     */
>> +    cpumask_copy(desc->arch.cpu_mask, cpumask_of(ch->cpu));
>> +
>>      spin_unlock(&desc->lock);
>>  
>> -    spin_unlock(&ch->lock);
>> +    msg.dest32 = cpu_physical_id(ch->cpu);
>> +    msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
>> +    msg.address_lo |= MSI_ADDR_DEST_ID(msg.dest32);
>> +    if ( msg.dest32 != ch->msi.msg.dest32 )
>> +    {
>> +        ch->msi.msg = msg;
>>  
>> -    /* We may have missed an interrupt due to the temporary masking. */
>> -    if ( ch->event_handler && ch->next_event < NOW() )
>> -        ch->event_handler(ch);
>> +        if ( iommu_intremap != iommu_intremap_off )
>> +        {
>> +            int rc = iommu_update_ire_from_msi(&ch->msi, &msg);
>> +
>> +            ASSERT(rc <= 0);
>> +            if ( rc >= 0 )
> 
> I don't think the rc > 0 part of this check is meaningful, as any rc
> value > 0 will trigger the ASSERT(rc <= 0) ahead of it.  The code
> inside of the if block itself only contains ASSERTs, so it's only
> relevant for debug=y builds that will also have the rc <= 0 ASSERT.
> 
> You could possibly use:
> 
> ASSERT(rc <= 0);
> if ( !rc )
> {
>     ASSERT(...
> 
> And achieve the same result?

Yes, except that I'd like to keep the >= to cover the case if the first
assertion was dropped / commented out, as well as to have a doc effect.

>> @@ -991,6 +997,13 @@ void alloc_direct_apic_vector(uint8_t *v
>>      spin_unlock(&lock);
>>  }
>>  
>> +/* This could free any vectors, but is needed only for low-prio ones. */
>> +void __init free_lopriority_vector(uint8_t vector)
>> +{
>> +    ASSERT(vector < FIRST_HIPRIORITY_VECTOR);
>> +    clear_bit(vector, used_vectors);
>> +}
> 
> I'm undecided whether we want to have such helper.  This is all very
> specific to the single use by the HPET vector, and hence might be best
> to simply put the clear_bit() inside of hpet_broadcast_late_init()
> itself.

I wanted to avoid making used_vectors non-static.

> I could see for example other callers wanting to use this also
> requiring cleanup of the per cpu vector_irq arrays.  Given it's (so
> far) very limited usage it might be clearer to open-code the
> clear_bit().

Dealing with vector_irq[] is a separate thing, though, isn't it?

>> --- a/xen/drivers/passthrough/amd/iommu_intr.c
>> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
>> @@ -551,6 +551,13 @@ int cf_check amd_iommu_msi_msg_update_ir
>>          for ( i = 1; i < nr; ++i )
>>              msi_desc[i].remap_index = msi_desc->remap_index + i;
>>          msg->data = data;
>> +        /*
>> +         * While the low address bits don't matter, "canonicalize" the address
>> +         * by zapping the bits that were transferred to the IRTE.  This way
>> +         * callers can check for there actually needing to be an update to
>> +         * wherever the address is put.
>> +         */
>> +        msg->address_lo &= ~(MSI_ADDR_DESTMODE_MASK | MSI_ADDR_DEST_ID_MASK);
> 
> You might want to mention this change on the commit message also, as
> it could look unrelated to the rest of the code?

I thought the comment here provided enough context and detail. I've added
"AMD interrupt remapping code so far didn't "return" a consistent MSI
 address when translating an MSI message. Clear respective fields there, to
 keep the respective assertion in set_channel_irq_affinity() from
 triggering."

Jan

Re: [PATCH v3 for-4.21 2/9] x86/HPET: use single, global, low-priority vector for broadcast IRQ

[Roger Pau Monné]

On Mon, Oct 27, 2025 at 11:23:58AM +0100, Jan Beulich wrote:
> On 24.10.2025 15:24, Roger Pau Monné wrote:
> > On Thu, Oct 23, 2025 at 05:50:17PM +0200, Jan Beulich wrote:
> >> @@ -343,6 +347,12 @@ static int __init hpet_setup_msi_irq(str
> >>      u32 cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
> >>      irq_desc_t *desc = irq_to_desc(ch->msi.irq);
> >>  
> >> +    clear_irq_vector(ch->msi.irq);
> >> +    ret = bind_irq_vector(ch->msi.irq, HPET_BROADCAST_VECTOR, &cpu_online_map);
> > 
> > By passing cpu_online_map here, it leads to _bind_irq_vector() doing:
> > 
> > cpumask_copy(desc->arch.cpu_mask, &cpu_online_map);
> > 
> > Which strictly speaking is wrong.  However this is just a cosmetic
> > issue until the irq is used for the first time, at which point it will
> > be assigned to a concrete CPU.
> > 
> > You could do:
> > 
> > cpumask_clear(desc->arch.cpu_mask);
> > cpumask_set_cpu(cpumask_any(&cpu_online_map), desc->arch.cpu_mask);
> > 
> > (Or equivalent)
> > 
> > To assign the interrupt to a concrete CPU and reflex it on the
> > cpu_mask after the bind_irq_vector() call, but I can live with it
> > being like this.  I have patches to adjust _bind_irq_vector() myself,
> > which I hope I will be able to post soon.
> 
> Hmm, I wrongly memorized hpet_broadcast_init() as being pre-SMP-init only.
> It has three call sites:
> - mwait_idle_init(), called from cpuidle_presmp_init(),
> - amd_cpuidle_init(), calling in only when invoked the very first time,
>   which is again from cpuidle_presmp_init(),
> - _disable_pit_irq(), called from the regular initcall disable_pit_irq().
> I.e. for the latter you're right that the CPU mask is too broad (in only a
> cosmetic way though). Would be you okay if I used cpumask_of(0) in place
> of &cpu_online_map?

Using cpumask_of(0) would be OK, as the per-cpu vector_irq array will
be updated ahead of assigning the interrupt to a CPU, and hence it
doesn't need to be done for all possible online CPUs in
_bind_irq_vector().

In the context here it would be more accurate to provide an empty CPU
mask, as the interrupt is not yet targeting any CPU.  Using CPU 0
would be a placeholder, which seems fine for the purpose.

> >> @@ -472,19 +482,50 @@ static struct hpet_event_channel *hpet_g
> >>  static void set_channel_irq_affinity(struct hpet_event_channel *ch)
> >>  {
> >>      struct irq_desc *desc = irq_to_desc(ch->msi.irq);
> >> +    struct msi_msg msg = ch->msi.msg;
> >>  
> >>      ASSERT(!local_irq_is_enabled());
> >>      spin_lock(&desc->lock);
> >> -    hpet_msi_mask(desc);
> >> -    hpet_msi_set_affinity(desc, cpumask_of(ch->cpu));
> >> -    hpet_msi_unmask(desc);
> >> +
> >> +    per_cpu(vector_irq, ch->cpu)[HPET_BROADCAST_VECTOR] = ch->msi.irq;
> >> +
> >> +    /*
> >> +     * Open-coding a reduced form of hpet_msi_set_affinity() here.  With the
> >> +     * actual update below (either of the IRTE or of [just] message address;
> >> +     * with interrupt remapping message address/data don't change) now being
> >> +     * atomic, we can avoid masking the IRQ around the update.  As a result
> >> +     * we're no longer at risk of missing IRQs (provided hpet_broadcast_enter()
> >> +     * keeps setting the new deadline only afterwards).
> >> +     */
> >> +    cpumask_copy(desc->arch.cpu_mask, cpumask_of(ch->cpu));
> >> +
> >>      spin_unlock(&desc->lock);
> >>  
> >> -    spin_unlock(&ch->lock);
> >> +    msg.dest32 = cpu_physical_id(ch->cpu);
> >> +    msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
> >> +    msg.address_lo |= MSI_ADDR_DEST_ID(msg.dest32);
> >> +    if ( msg.dest32 != ch->msi.msg.dest32 )
> >> +    {
> >> +        ch->msi.msg = msg;
> >>  
> >> -    /* We may have missed an interrupt due to the temporary masking. */
> >> -    if ( ch->event_handler && ch->next_event < NOW() )
> >> -        ch->event_handler(ch);
> >> +        if ( iommu_intremap != iommu_intremap_off )
> >> +        {
> >> +            int rc = iommu_update_ire_from_msi(&ch->msi, &msg);
> >> +
> >> +            ASSERT(rc <= 0);
> >> +            if ( rc >= 0 )
> > 
> > I don't think the rc > 0 part of this check is meaningful, as any rc
> > value > 0 will trigger the ASSERT(rc <= 0) ahead of it.  The code
> > inside of the if block itself only contains ASSERTs, so it's only
> > relevant for debug=y builds that will also have the rc <= 0 ASSERT.
> > 
> > You could possibly use:
> > 
> > ASSERT(rc <= 0);
> > if ( !rc )
> > {
> >     ASSERT(...
> > 
> > And achieve the same result?
> 
> Yes, except that I'd like to keep the >= to cover the case if the first
> assertion was dropped / commented out, as well as to have a doc effect.

Oh, OK.  Fair enough, I wasn't taking into account that this could be
done in case code is modified.

> >> @@ -991,6 +997,13 @@ void alloc_direct_apic_vector(uint8_t *v
> >>      spin_unlock(&lock);
> >>  }
> >>  
> >> +/* This could free any vectors, but is needed only for low-prio ones. */
> >> +void __init free_lopriority_vector(uint8_t vector)
> >> +{
> >> +    ASSERT(vector < FIRST_HIPRIORITY_VECTOR);
> >> +    clear_bit(vector, used_vectors);
> >> +}
> > 
> > I'm undecided whether we want to have such helper.  This is all very
> > specific to the single use by the HPET vector, and hence might be best
> > to simply put the clear_bit() inside of hpet_broadcast_late_init()
> > itself.
> 
> I wanted to avoid making used_vectors non-static.
> 
> > I could see for example other callers wanting to use this also
> > requiring cleanup of the per cpu vector_irq arrays.  Given it's (so
> > far) very limited usage it might be clearer to open-code the
> > clear_bit().
> 
> Dealing with vector_irq[] is a separate thing, though, isn't it?

Possibly, that's part of the binding, rather than the allocation
itself (which is what you cover here).

> >> --- a/xen/drivers/passthrough/amd/iommu_intr.c
> >> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> >> @@ -551,6 +551,13 @@ int cf_check amd_iommu_msi_msg_update_ir
> >>          for ( i = 1; i < nr; ++i )
> >>              msi_desc[i].remap_index = msi_desc->remap_index + i;
> >>          msg->data = data;
> >> +        /*
> >> +         * While the low address bits don't matter, "canonicalize" the address
> >> +         * by zapping the bits that were transferred to the IRTE.  This way
> >> +         * callers can check for there actually needing to be an update to
> >> +         * wherever the address is put.
> >> +         */
> >> +        msg->address_lo &= ~(MSI_ADDR_DESTMODE_MASK | MSI_ADDR_DEST_ID_MASK);
> > 
> > You might want to mention this change on the commit message also, as
> > it could look unrelated to the rest of the code?
> 
> I thought the comment here provided enough context and detail. I've added
> "AMD interrupt remapping code so far didn't "return" a consistent MSI
>  address when translating an MSI message. Clear respective fields there, to
>  keep the respective assertion in set_channel_irq_affinity() from
>  triggering."

LGTM, I would possibly remove the last "respective" for being
repetitive given the previous one in the sentence.

Thanks, Roger.

Re: [PATCH v3 for-4.21 2/9] x86/HPET: use single, global, low-priority vector for broadcast IRQ

[Jan Beulich]

On 27.10.2025 12:33, Roger Pau Monné wrote:
> On Mon, Oct 27, 2025 at 11:23:58AM +0100, Jan Beulich wrote:
>> On 24.10.2025 15:24, Roger Pau Monné wrote:
>>> On Thu, Oct 23, 2025 at 05:50:17PM +0200, Jan Beulich wrote:
>>>> @@ -343,6 +347,12 @@ static int __init hpet_setup_msi_irq(str
>>>>      u32 cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
>>>>      irq_desc_t *desc = irq_to_desc(ch->msi.irq);
>>>>  
>>>> +    clear_irq_vector(ch->msi.irq);
>>>> +    ret = bind_irq_vector(ch->msi.irq, HPET_BROADCAST_VECTOR, &cpu_online_map);
>>>
>>> By passing cpu_online_map here, it leads to _bind_irq_vector() doing:
>>>
>>> cpumask_copy(desc->arch.cpu_mask, &cpu_online_map);
>>>
>>> Which strictly speaking is wrong.  However this is just a cosmetic
>>> issue until the irq is used for the first time, at which point it will
>>> be assigned to a concrete CPU.
>>>
>>> You could do:
>>>
>>> cpumask_clear(desc->arch.cpu_mask);
>>> cpumask_set_cpu(cpumask_any(&cpu_online_map), desc->arch.cpu_mask);
>>>
>>> (Or equivalent)
>>>
>>> To assign the interrupt to a concrete CPU and reflex it on the
>>> cpu_mask after the bind_irq_vector() call, but I can live with it
>>> being like this.  I have patches to adjust _bind_irq_vector() myself,
>>> which I hope I will be able to post soon.
>>
>> Hmm, I wrongly memorized hpet_broadcast_init() as being pre-SMP-init only.
>> It has three call sites:
>> - mwait_idle_init(), called from cpuidle_presmp_init(),
>> - amd_cpuidle_init(), calling in only when invoked the very first time,
>>   which is again from cpuidle_presmp_init(),
>> - _disable_pit_irq(), called from the regular initcall disable_pit_irq().
>> I.e. for the latter you're right that the CPU mask is too broad (in only a
>> cosmetic way though). Would be you okay if I used cpumask_of(0) in place
>> of &cpu_online_map?
> 
> Using cpumask_of(0) would be OK, as the per-cpu vector_irq array will
> be updated ahead of assigning the interrupt to a CPU, and hence it
> doesn't need to be done for all possible online CPUs in
> _bind_irq_vector().
> 
> In the context here it would be more accurate to provide an empty CPU
> mask, as the interrupt is not yet targeting any CPU.  Using CPU 0
> would be a placeholder, which seems fine for the purpose.

Putting an empty mask there, while indeed logically correct, would (I fear)
again put us at risk with other code making various assumptions. I'll go
with cpumask_of(0).

>>>> --- a/xen/drivers/passthrough/amd/iommu_intr.c
>>>> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
>>>> @@ -551,6 +551,13 @@ int cf_check amd_iommu_msi_msg_update_ir
>>>>          for ( i = 1; i < nr; ++i )
>>>>              msi_desc[i].remap_index = msi_desc->remap_index + i;
>>>>          msg->data = data;
>>>> +        /*
>>>> +         * While the low address bits don't matter, "canonicalize" the address
>>>> +         * by zapping the bits that were transferred to the IRTE.  This way
>>>> +         * callers can check for there actually needing to be an update to
>>>> +         * wherever the address is put.
>>>> +         */
>>>> +        msg->address_lo &= ~(MSI_ADDR_DESTMODE_MASK | MSI_ADDR_DEST_ID_MASK);
>>>
>>> You might want to mention this change on the commit message also, as
>>> it could look unrelated to the rest of the code?
>>
>> I thought the comment here provided enough context and detail. I've added
>> "AMD interrupt remapping code so far didn't "return" a consistent MSI
>>  address when translating an MSI message. Clear respective fields there, to
>>  keep the respective assertion in set_channel_irq_affinity() from
>>  triggering."
> 
> LGTM, I would possibly remove the last "respective" for being
> repetitive given the previous one in the sentence.

Oh, indeed. Replaced it by "related" rather than dropping it completely.

Jan

Re: [PATCH v3 for-4.21 2/9] x86/HPET: use single, global, low-priority vector for broadcast IRQ

[Jan Beulich]

On 27.10.2025 12:53, Jan Beulich wrote:
> On 27.10.2025 12:33, Roger Pau Monné wrote:
>> On Mon, Oct 27, 2025 at 11:23:58AM +0100, Jan Beulich wrote:
>>> On 24.10.2025 15:24, Roger Pau Monné wrote:
>>>> On Thu, Oct 23, 2025 at 05:50:17PM +0200, Jan Beulich wrote:
>>>>> @@ -343,6 +347,12 @@ static int __init hpet_setup_msi_irq(str
>>>>>      u32 cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
>>>>>      irq_desc_t *desc = irq_to_desc(ch->msi.irq);
>>>>>  
>>>>> +    clear_irq_vector(ch->msi.irq);
>>>>> +    ret = bind_irq_vector(ch->msi.irq, HPET_BROADCAST_VECTOR, &cpu_online_map);
>>>>
>>>> By passing cpu_online_map here, it leads to _bind_irq_vector() doing:
>>>>
>>>> cpumask_copy(desc->arch.cpu_mask, &cpu_online_map);
>>>>
>>>> Which strictly speaking is wrong.  However this is just a cosmetic
>>>> issue until the irq is used for the first time, at which point it will
>>>> be assigned to a concrete CPU.
>>>>
>>>> You could do:
>>>>
>>>> cpumask_clear(desc->arch.cpu_mask);
>>>> cpumask_set_cpu(cpumask_any(&cpu_online_map), desc->arch.cpu_mask);
>>>>
>>>> (Or equivalent)
>>>>
>>>> To assign the interrupt to a concrete CPU and reflex it on the
>>>> cpu_mask after the bind_irq_vector() call, but I can live with it
>>>> being like this.  I have patches to adjust _bind_irq_vector() myself,
>>>> which I hope I will be able to post soon.
>>>
>>> Hmm, I wrongly memorized hpet_broadcast_init() as being pre-SMP-init only.
>>> It has three call sites:
>>> - mwait_idle_init(), called from cpuidle_presmp_init(),
>>> - amd_cpuidle_init(), calling in only when invoked the very first time,
>>>   which is again from cpuidle_presmp_init(),
>>> - _disable_pit_irq(), called from the regular initcall disable_pit_irq().
>>> I.e. for the latter you're right that the CPU mask is too broad (in only a
>>> cosmetic way though). Would be you okay if I used cpumask_of(0) in place
>>> of &cpu_online_map?
>>
>> Using cpumask_of(0) would be OK, as the per-cpu vector_irq array will
>> be updated ahead of assigning the interrupt to a CPU, and hence it
>> doesn't need to be done for all possible online CPUs in
>> _bind_irq_vector().
>>
>> In the context here it would be more accurate to provide an empty CPU
>> mask, as the interrupt is not yet targeting any CPU.  Using CPU 0
>> would be a placeholder, which seems fine for the purpose.
> 
> Putting an empty mask there, while indeed logically correct, would (I fear)
> again put us at risk with other code making various assumptions.

And indeed: _bind_irq_vector() would reject an empty mask.

Jan

Re: [PATCH v3 for-4.21 2/9] x86/HPET: use single, global, low-priority vector for broadcast IRQ

[Roger Pau Monné]

On Mon, Oct 27, 2025 at 12:53:34PM +0100, Jan Beulich wrote:
> On 27.10.2025 12:33, Roger Pau Monné wrote:
> > On Mon, Oct 27, 2025 at 11:23:58AM +0100, Jan Beulich wrote:
> >> On 24.10.2025 15:24, Roger Pau Monné wrote:
> >>> On Thu, Oct 23, 2025 at 05:50:17PM +0200, Jan Beulich wrote:
> >>>> @@ -343,6 +347,12 @@ static int __init hpet_setup_msi_irq(str
> >>>>      u32 cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
> >>>>      irq_desc_t *desc = irq_to_desc(ch->msi.irq);
> >>>>  
> >>>> +    clear_irq_vector(ch->msi.irq);
> >>>> +    ret = bind_irq_vector(ch->msi.irq, HPET_BROADCAST_VECTOR, &cpu_online_map);
> >>>
> >>> By passing cpu_online_map here, it leads to _bind_irq_vector() doing:
> >>>
> >>> cpumask_copy(desc->arch.cpu_mask, &cpu_online_map);
> >>>
> >>> Which strictly speaking is wrong.  However this is just a cosmetic
> >>> issue until the irq is used for the first time, at which point it will
> >>> be assigned to a concrete CPU.
> >>>
> >>> You could do:
> >>>
> >>> cpumask_clear(desc->arch.cpu_mask);
> >>> cpumask_set_cpu(cpumask_any(&cpu_online_map), desc->arch.cpu_mask);
> >>>
> >>> (Or equivalent)
> >>>
> >>> To assign the interrupt to a concrete CPU and reflex it on the
> >>> cpu_mask after the bind_irq_vector() call, but I can live with it
> >>> being like this.  I have patches to adjust _bind_irq_vector() myself,
> >>> which I hope I will be able to post soon.
> >>
> >> Hmm, I wrongly memorized hpet_broadcast_init() as being pre-SMP-init only.
> >> It has three call sites:
> >> - mwait_idle_init(), called from cpuidle_presmp_init(),
> >> - amd_cpuidle_init(), calling in only when invoked the very first time,
> >>   which is again from cpuidle_presmp_init(),
> >> - _disable_pit_irq(), called from the regular initcall disable_pit_irq().
> >> I.e. for the latter you're right that the CPU mask is too broad (in only a
> >> cosmetic way though). Would be you okay if I used cpumask_of(0) in place
> >> of &cpu_online_map?
> > 
> > Using cpumask_of(0) would be OK, as the per-cpu vector_irq array will
> > be updated ahead of assigning the interrupt to a CPU, and hence it
> > doesn't need to be done for all possible online CPUs in
> > _bind_irq_vector().
> > 
> > In the context here it would be more accurate to provide an empty CPU
> > mask, as the interrupt is not yet targeting any CPU.  Using CPU 0
> > would be a placeholder, which seems fine for the purpose.
> 
> Putting an empty mask there, while indeed logically correct, would (I fear)
> again put us at risk with other code making various assumptions. I'll go
> with cpumask_of(0).

Yeah, that's what I fear also.  It's in principle possible for
the cpu_mask to be empty, but since this targets 4.21 I think it's too
risky.  Using cpumask_of(0) is fine.  Please keep my RB.

Thanks, Roger.

[PATCH v3 for-4.21 3/9] x86/HPET: replace handle_hpet_broadcast()'s on-stack cpumask_t

[Jan Beulich]

With large NR_CPUS on-stack cpumask_t variables are problematic. Now that
the IRQ handler can't be invoked in a nested manner anymore, we can
instead use a per-CPU variable. While we can't use scratch_cpumask in code
invoked from IRQ handlers, simply amend that one with a HPET-special form.
(Note that only one of the two IRQ handling functions can come into play
at any one time.)

Fixes: 996576b965cc ("xen: allow up to 16383 cpus")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
While doing this I noticed that this and all pre-existing uses of
DEFINE_PER_CPU_READ_MOSTLY() aren't quite right: When the type is
cpumask_var_t, the variable is read-mostly only when NR_CPUS <=
2 * BITS_PER_LONG. That'll want sorting separately, though.

It is important for this to not be moved ahead of "x86/HPET: use single,
global, low-priority vector for broadcast IRQ", as the original call here
from set_channel_irq_affinity() may not use the new variable (it would
need to use scratch_cpumask instead).
---
v2: New.

--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -196,7 +196,7 @@ static void evt_do_broadcast(cpumask_t *
 
 static void cf_check handle_hpet_broadcast(struct hpet_event_channel *ch)
 {
-    cpumask_t mask;
+    cpumask_t *scratch = this_cpu(hpet_scratch_cpumask);
     s_time_t now, next_event;
     unsigned int cpu;
     unsigned long flags;
@@ -209,7 +209,7 @@ again:
     spin_unlock_irqrestore(&ch->lock, flags);
 
     next_event = STIME_MAX;
-    cpumask_clear(&mask);
+    cpumask_clear(scratch);
     now = NOW();
 
     /* find all expired events */
@@ -218,13 +218,13 @@ again:
         s_time_t deadline = ACCESS_ONCE(per_cpu(timer_deadline, cpu));
 
         if ( deadline <= now )
-            __cpumask_set_cpu(cpu, &mask);
+            __cpumask_set_cpu(cpu, scratch);
         else if ( deadline < next_event )
             next_event = deadline;
     }
 
     /* wakeup the cpus which have an expired event. */
-    evt_do_broadcast(&mask);
+    evt_do_broadcast(scratch);
 
     if ( next_event != STIME_MAX )
     {
--- a/xen/arch/x86/include/asm/smp.h
+++ b/xen/arch/x86/include/asm/smp.h
@@ -22,6 +22,7 @@
 DECLARE_PER_CPU(cpumask_var_t, cpu_sibling_mask);
 DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask);
 DECLARE_PER_CPU(cpumask_var_t, scratch_cpumask);
+DECLARE_PER_CPU(cpumask_var_t, hpet_scratch_cpumask);
 DECLARE_PER_CPU(cpumask_var_t, send_ipi_cpumask);
 
 /*
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -55,6 +55,9 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t
 DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, scratch_cpumask);
 static cpumask_t scratch_cpu0mask;
 
+DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, hpet_scratch_cpumask);
+static cpumask_t hpet_scratch_cpu0mask;
+
 DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, send_ipi_cpumask);
 static cpumask_t send_ipi_cpu0mask;
 
@@ -976,6 +979,8 @@ static void cpu_smpboot_free(unsigned in
         FREE_CPUMASK_VAR(per_cpu(cpu_core_mask, cpu));
         if ( per_cpu(scratch_cpumask, cpu) != &scratch_cpu0mask )
             FREE_CPUMASK_VAR(per_cpu(scratch_cpumask, cpu));
+        if ( per_cpu(hpet_scratch_cpumask, cpu) != &hpet_scratch_cpu0mask )
+            FREE_CPUMASK_VAR(per_cpu(hpet_scratch_cpumask, cpu));
         if ( per_cpu(send_ipi_cpumask, cpu) != &send_ipi_cpu0mask )
             FREE_CPUMASK_VAR(per_cpu(send_ipi_cpumask, cpu));
     }
@@ -1112,6 +1117,7 @@ static int cpu_smpboot_alloc(unsigned in
     if ( !(cond_zalloc_cpumask_var(&per_cpu(cpu_sibling_mask, cpu)) &&
            cond_zalloc_cpumask_var(&per_cpu(cpu_core_mask, cpu)) &&
            cond_alloc_cpumask_var(&per_cpu(scratch_cpumask, cpu)) &&
+           cond_alloc_cpumask_var(&per_cpu(hpet_scratch_cpumask, cpu)) &&
            cond_alloc_cpumask_var(&per_cpu(send_ipi_cpumask, cpu))) )
         goto out;
 
@@ -1239,6 +1245,7 @@ void __init smp_prepare_boot_cpu(void)
     cpumask_set_cpu(cpu, &cpu_present_map);
 #if NR_CPUS > 2 * BITS_PER_LONG
     per_cpu(scratch_cpumask, cpu) = &scratch_cpu0mask;
+    per_cpu(hpet_scratch_cpumask, cpu) = &hpet_scratch_cpu0mask;
     per_cpu(send_ipi_cpumask, cpu) = &send_ipi_cpu0mask;
 #endif
 

Re: [PATCH v3 for-4.21 3/9] x86/HPET: replace handle_hpet_broadcast()'s on-stack cpumask_t

[Jan Beulich]

On 23.10.2025 17:51, Jan Beulich wrote:
> With large NR_CPUS on-stack cpumask_t variables are problematic. Now that
> the IRQ handler can't be invoked in a nested manner anymore, we can
> instead use a per-CPU variable. While we can't use scratch_cpumask in code
> invoked from IRQ handlers, simply amend that one with a HPET-special form.
> (Note that only one of the two IRQ handling functions can come into play
> at any one time.)
> 
> Fixes: 996576b965cc ("xen: allow up to 16383 cpus")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Views towards 4.21?

Jan

Re: [PATCH v3 for-4.21 3/9] x86/HPET: replace handle_hpet_broadcast()'s on-stack cpumask_t

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]


On 10/27/25 1:25 PM, Jan Beulich wrote:
> On 23.10.2025 17:51, Jan Beulich wrote:
>> With large NR_CPUS on-stack cpumask_t variables are problematic. Now that
>> the IRQ handler can't be invoked in a nested manner anymore, we can
>> instead use a per-CPU variable. While we can't use scratch_cpumask in code
>> invoked from IRQ handlers, simply amend that one with a HPET-special form.
>> (Note that only one of the two IRQ handling functions can come into play
>> at any one time.)
>>
>> Fixes: 996576b965cc ("xen: allow up to 16383 cpus")
>> Signed-off-by: Jan Beulich<jbeulich@suse.com>
>> Reviewed-by: Roger Pau Monné<roger.pau@citrix.com>
> Views towards 4.21?

Release-Acked-By: Oleksii Kurochko<oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 1489 bytes --]

[PATCH v3 4/9] x86/HPET: avoid indirect call to event handler

[Jan Beulich]

It's only ever handle_hpet_broadcast() that's used. While we now don't
enable IRQs right away, still play safe and convert the function pointer
to a boolean, to make sure no calls occur too early.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Re-base over changes earlier in the series.

--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -40,7 +40,7 @@ struct hpet_event_channel
     s_time_t      next_event;
     cpumask_var_t cpumask;
     spinlock_t    lock;
-    void          (*event_handler)(struct hpet_event_channel *ch);
+    bool          event_handler;
 
     unsigned int idx;   /* physical channel idx */
     unsigned int cpu;   /* msi target */
@@ -194,7 +194,7 @@ static void evt_do_broadcast(cpumask_t *
        cpumask_raise_softirq(mask, TIMER_SOFTIRQ);
 }
 
-static void cf_check handle_hpet_broadcast(struct hpet_event_channel *ch)
+static void handle_hpet_broadcast(struct hpet_event_channel *ch)
 {
     cpumask_t *scratch = this_cpu(hpet_scratch_cpumask);
     s_time_t now, next_event;
@@ -250,7 +250,7 @@ static void cf_check hpet_interrupt_hand
         return;
     }
 
-    ch->event_handler(ch);
+    handle_hpet_broadcast(ch);
 }
 
 static void hpet_enable_channel(struct hpet_event_channel *ch)
@@ -653,7 +653,7 @@ void __init hpet_broadcast_init(void)
         hpet_events[i].next_event = STIME_MAX;
         spin_lock_init(&hpet_events[i].lock);
         smp_wmb();
-        hpet_events[i].event_handler = handle_hpet_broadcast;
+        hpet_events[i].event_handler = true;
 
         hpet_events[i].msi.msi_attrib.maskbit = 1;
         hpet_events[i].msi.msi_attrib.pos = MSI_TYPE_HPET;
@@ -810,7 +810,9 @@ int hpet_legacy_irq_tick(void)
          (hpet_events->flags & (HPET_EVT_DISABLE|HPET_EVT_LEGACY)) !=
          HPET_EVT_LEGACY )
         return 0;
-    hpet_events->event_handler(hpet_events);
+
+    handle_hpet_broadcast(hpet_events);
+
     return 1;
 }
 

[PATCH v3 5/9] x86/HPET: make another channel flags update atomic

[Jan Beulich]

Unlike the setting of HPET_EVT_LEGACY in hpet_broadcast_init(), the
setting of HPET_EVT_DISABLE in hpet_disable_legacy_broadcast() isn't init-
only and hence can race other flag manipulation (not all of which occur
while holding the channel's lock). While possibly any such updates would
only ever occur when HPET_EVT_LEGACY isn't set in the first place, this
doesn't look straightforward to prove, so better be on the safe side.

Fixes: d09486dba36a ("cpuidle: Enable hpet broadcast by default")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -725,7 +725,7 @@ void hpet_disable_legacy_broadcast(void)
 
     spin_lock_irqsave(&hpet_events->lock, flags);
 
-    hpet_events->flags |= HPET_EVT_DISABLE;
+    set_bit(HPET_EVT_DISABLE_BIT, &hpet_events->flags);
 
     /* disable HPET T0 */
     cfg = hpet_read32(HPET_Tn_CFG(0));

[PATCH v3 6/9] x86/HPET: move legacy tick IRQ count adjustment

[Jan Beulich]

If already we play with the IRQ count, we should do so only if we actually
"consume" the interrupt; normal timer IRQs should not have any adjustment
done.

Fixes: 353533232730 ("cpuidle: fix the menu governor to enhance IO performance")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
_Why_ we do these adjustments (also elsewhere) I don't reeally know.

--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -804,13 +804,13 @@ int hpet_broadcast_is_available(void)
 
 int hpet_legacy_irq_tick(void)
 {
-    this_cpu(irq_count)--;
-
     if ( !hpet_events ||
          (hpet_events->flags & (HPET_EVT_DISABLE|HPET_EVT_LEGACY)) !=
          HPET_EVT_LEGACY )
         return 0;
 
+    this_cpu(irq_count)--;
+
     handle_hpet_broadcast(hpet_events);
 
     return 1;

[PATCH v3 7/9] x86/HPET: reduce hpet_next_event() call sites

[Jan Beulich]

I'm surprised gcc doesn't manage to do that: At least in debug builds two
call sites exist, just like source code has it. That's not necessary
though - by using do/while we can reduce this to a single call site. Then
the function will be inlined.

While improving code gen, also switch the function's 2nd parameter to
unsigned.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Oddly enough the CDQE is replaced by an entirely unnecessary 32-bit MOV of
a register to itself (i.e. zero-extending to 64 bits), as that's
immediately preceded by a 32-bit ADD targeting the same register.

--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -124,7 +124,7 @@ static inline unsigned long ns2ticks(uns
     return (unsigned long) tmp;
 }
 
-static int hpet_next_event(unsigned long delta, int timer)
+static int hpet_next_event(unsigned long delta, unsigned int timer)
 {
     uint32_t cnt, cmp;
     unsigned long flags;
@@ -173,12 +173,10 @@ static int reprogram_hpet_evt_channel(
     delta = max_t(int64_t, delta, MIN_DELTA_NS);
     delta = ns2ticks(delta, ch->shift, ch->mult);
 
-    ret = hpet_next_event(delta, ch->idx);
-    while ( ret && force )
-    {
-        delta += delta;
+    do {
         ret = hpet_next_event(delta, ch->idx);
-    }
+        delta += delta;
+    } while ( ret && force );
 
     return ret;
 }

[PATCH v3 8/9] x86/HPET: drop "long timeout" handling from reprogram_hpet_evt_channel()

[Jan Beulich]

Neither caller passes STIME_MAX, so (bogusly) handling the case isn't
necessary.

"Bogusly" because with 32-bit counters, writing 0 means on average half
the wrapping period until an interrupt would be raised, while of course
in extreme cases an interrupt would be raised almost right away.

Amends: aa42fc0e9cd9 ("cpuidle: remove hpet access in hpet_broadcast_exit")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: Drop the code instead of adjusting it.

--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -162,13 +162,6 @@ static int reprogram_hpet_evt_channel(
 
     ch->next_event = expire;
 
-    if ( expire == STIME_MAX )
-    {
-        /* We assume it will take a long time for the timer to wrap. */
-        hpet_write32(0, HPET_Tn_CMP(ch->idx));
-        return 0;
-    }
-
     delta = min_t(int64_t, delta, MAX_DELTA_NS);
     delta = max_t(int64_t, delta, MIN_DELTA_NS);
     delta = ns2ticks(delta, ch->shift, ch->mult);

[PATCH v3 9/9] x86/HPET: simplify "expire" check a little in reprogram_hpet_evt_channel()

[Jan Beulich]

When this was added, the log message was updated correctly, but the zero
case was needlessly checked separately: hpet_broadcast_enter() had a zero
check added at the same time, while handle_hpet_broadcast() can't possibly
pass 0 here anyway.

Fixes: 7145897cfb81 ("cpuidle: Fix for timer_deadline==0 case")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -147,10 +147,10 @@ static int reprogram_hpet_evt_channel(
     int64_t delta;
     int ret;
 
-    if ( (ch->flags & HPET_EVT_DISABLE) || (expire == 0) )
+    if ( ch->flags & HPET_EVT_DISABLE )
         return 0;
 
-    if ( unlikely(expire < 0) )
+    if ( unlikely(expire <= 0) )
     {
         printk(KERN_DEBUG "reprogram: expire <= 0\n");
         return -ETIME;