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

[PATCH v2 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.

v2 has 3 patches dropped and 2 new ones introduced, plus in particular a fair
bit of rework of the "main" patch. See individual patches for details.

1: disable 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: don't use hardcoded 0 for "long timeout"
9: simplify "expire" check a little in reprogram_hpet_evt_channel()

Jan

[PATCH v2 for-4.21 1/9] x86/HPET: disable 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.

Note that no explicit "enable" is necessary - that's implicitly done by
set_channel_irq_affinity() once the channel goes into use again.

Along with disabling the counter, 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 similarly should happen no earlier than from
set_channel_irq_affinity() (here: 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>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.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.)

--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -262,10 +262,9 @@ static void cf_check hpet_msi_unmask(str
     ch->msi.msi_attrib.host_masked = 0;
 }
 
-static void cf_check hpet_msi_mask(struct irq_desc *desc)
+static void hpet_disable_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;
@@ -273,6 +272,11 @@ static void cf_check hpet_msi_mask(struc
     ch->msi.msi_attrib.host_masked = 1;
 }
 
+static void cf_check hpet_msi_mask(struct irq_desc *desc)
+{
+    hpet_disable_channel(desc->action->dev_id);
+}
+
 static int hpet_msi_write(struct hpet_event_channel *ch, struct msi_msg *msg)
 {
     ch->msi.msg = *msg;
@@ -295,12 +299,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 +324,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 +524,8 @@ static void hpet_detach_channel(unsigned
         spin_unlock_irq(&ch->lock);
     else if ( (next = cpumask_first(ch->cpumask)) >= nr_cpu_ids )
     {
+        hpet_disable_channel(ch);
+        ch->next_event = STIME_MAX;
         ch->cpu = -1;
         clear_bit(HPET_EVT_USED_BIT, &ch->flags);
         spin_unlock_irq(&ch->lock);

Re: [PATCH v2 for-4.21 1/9] x86/HPET: disable unused channels

[Jan Beulich]

On 20.10.2025 13:18, Jan Beulich wrote:
> @@ -526,6 +524,8 @@ static void hpet_detach_channel(unsigned
>          spin_unlock_irq(&ch->lock);
>      else if ( (next = cpumask_first(ch->cpumask)) >= nr_cpu_ids )
>      {
> +        hpet_disable_channel(ch);
> +        ch->next_event = STIME_MAX;
>          ch->cpu = -1;
>          clear_bit(HPET_EVT_USED_BIT, &ch->flags);
>          spin_unlock_irq(&ch->lock);

Now that I have everything else working, I thought I'd look into where the
excess IRQs come from on Intel hardware. Since my earlier experiment with
making conditional the write in hpet_enable_channel() / hpet_msi_unmask()
had failed, I was misguided into assuming some more complex logic in their
HPETs. It now appears to be as simple as I initially suspected: It's 0 -> 1
transitions of the ENABLE bit which cause immediate IRQs. And the reason
why the removal of the mask/unmask pairs in patch 2 weren't (sufficiently)
helpful is above: We better wouldn't disable the channels, to avoid said
(mis-)feature. Instead I'm now intending to merely write a long timeout
value here, along the lines of what I did in this version (code there fully
dropped in v3 though) in patch 8.

Jan

[PATCH v2 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), avoid any "ordinary" vectors altogther
and use a vector from the 0x10...0x1f exception vector space. Exception vs
interrupt can easily be distinguished by checking for the presence of an
error code.

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.

In principle a change to setup_vector_irq() would be necessary, but only
if we used low-prio vectors as direct-APIC ones. Since the change would be
at best benign here, it is being omitted.

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 ... (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 hpet_enable_channel().
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).
---
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 hpet_disable_channel(struct hpet_event_channel *ch)
 {
     u32 cfg;
@@ -307,15 +313,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);
 }
 
@@ -328,7 +332,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,
 };
@@ -347,6 +351,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;
@@ -476,19 +486,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;
+
+        if ( iommu_intremap != iommu_intremap_off )
+        {
+            int rc = iommu_update_ire_from_msi(&ch->msi, &msg);
 
-    /* We may have missed an interrupt due to the temporary masking. */
-    if ( ch->event_handler && ch->next_event < NOW() )
-        ch->event_handler(ch);
+            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,
--- a/xen/arch/x86/include/asm/irq-vectors.h
+++ b/xen/arch/x86/include/asm/irq-vectors.h
@@ -18,6 +18,15 @@
 /* IRQ0 (timer) is statically allocated but must be high priority. */
 #define IRQ0_VECTOR             0xf0
 
+/*
+ * Low-priority (for now statically allocated) vectors, sharing entry
+ * points with exceptions in the 0x10 ... 0x1f range, as long as the
+ * respective exception has an error code.
+ */
+#define FIRST_LOPRIORITY_VECTOR 0x10
+#define HPET_BROADCAST_VECTOR   X86_EXC_AC
+#define LAST_LOPRIORITY_VECTOR  0x1f
+
 /* Legacy PIC uses vectors 0x20-0x2f. */
 #define FIRST_LEGACY_VECTOR     FIRST_DYNAMIC_VECTOR
 #define LAST_LEGACY_VECTOR      (FIRST_LEGACY_VECTOR + 0xf)
@@ -40,7 +49,7 @@
 /* There's no IRQ2 at the PIC. */
 #define IRQ_MOVE_CLEANUP_VECTOR (FIRST_LEGACY_VECTOR + 2)
 
-#define FIRST_IRQ_VECTOR        FIRST_DYNAMIC_VECTOR
+#define FIRST_IRQ_VECTOR        FIRST_LOPRIORITY_VECTOR
 #define LAST_IRQ_VECTOR         LAST_HIPRIORITY_VECTOR
 
 #endif /* _ASM_IRQ_VECTORS_H */
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -158,7 +158,7 @@ void msi_compose_msg(unsigned vector, co
 {
     memset(msg, 0, sizeof(*msg));
 
-    if ( vector < FIRST_DYNAMIC_VECTOR )
+    if ( vector < FIRST_LOPRIORITY_VECTOR )
         return;
 
     if ( cpu_mask )
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -1045,7 +1045,13 @@ END(entry_GP)
 
 FUNC(entry_AC)
         ENDBR64
+        /* #AC shares its entry point with the HPET broadcast interrupt. */
+        test  $8, %spl
+        jz    .Lac
+        push  $0
+.Lac:
         movb  $X86_EXC_AC, EFRAME_entry_vector(%rsp)
+        jnz   common_interrupt
         jmp   handle_exception
 END(entry_AC)
 

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

[Roger Pau Monné]

On Mon, Oct 20, 2025 at 01:18:34PM +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), avoid any "ordinary" vectors altogther
> and use a vector from the 0x10...0x1f exception vector space. Exception vs
> interrupt can easily be distinguished by checking for the presence of an
> error code.
> 
> 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.
> 
> In principle a change to setup_vector_irq() would be necessary, but only
> if we used low-prio vectors as direct-APIC ones. Since the change would be
> at best benign here, it is being omitted.
> 
> 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 ... (If so, this
> likely would want to be a separate patch, though.)

I think that needs to become a no-op, with possibly an ASSERT?  Is it
possibly for dom0 to try to balance this IRQ?  I would think not.

> The hpet_enable_channel() call could in principle be made (effectively)
> conditional, at the price of introducing a check in hpet_enable_channel().
> 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.

Let's go for the current approach.

> 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).
> ---
> 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 hpet_disable_channel(struct hpet_event_channel *ch)
>  {
>      u32 cfg;
> @@ -307,15 +313,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);
>  }
>  
> @@ -328,7 +332,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,
>  };
> @@ -347,6 +351,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;
> @@ -476,19 +486,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;
> +
> +        if ( iommu_intremap != iommu_intremap_off )
> +        {
> +            int rc = iommu_update_ire_from_msi(&ch->msi, &msg);
>  
> -    /* We may have missed an interrupt due to the temporary masking. */
> -    if ( ch->event_handler && ch->next_event < NOW() )
> -        ch->event_handler(ch);
> +            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));
> +            }

The sequence of asserts seem wrong here, the asserts inside of the rc
> 0 check will never trigger, because there's an ASSERT(rc <= 0)
ahead of them?

> +        }
> +        else
> +            hpet_write32(msg.address_lo, HPET_Tn_ROUTE(ch->idx) + 4);

If you avoid the HPET register update here you possibly need to make
sure that both fields are unconditionally written on the first call
after resume from suspension.  hpet_resume() needs to somehow taint
the channels to signal that a re-write of the address and data fields
is mandatory regardless of what iommu_update_ire_from_msi() has
returned.

Thanks, Roger.

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

[Jan Beulich]

On 20.10.2025 18:22, Roger Pau Monné wrote:
> On Mon, Oct 20, 2025 at 01:18:34PM +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), avoid any "ordinary" vectors altogther
>> and use a vector from the 0x10...0x1f exception vector space. Exception vs
>> interrupt can easily be distinguished by checking for the presence of an
>> error code.
>>
>> 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.
>>
>> In principle a change to setup_vector_irq() would be necessary, but only
>> if we used low-prio vectors as direct-APIC ones. Since the change would be
>> at best benign here, it is being omitted.
>>
>> 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 ... (If so, this
>> likely would want to be a separate patch, though.)
> 
> I think that needs to become a no-op, with possibly an ASSERT?  Is it
> possibly for dom0 to try to balance this IRQ?  I would think not.

I'd consider it an error if that was possible. But then the same goes for
other Xen-internal IRQs, like the IOMMU ones. They all implement a
.set_affinity hook ...

>> @@ -476,19 +486,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;
>> +
>> +        if ( iommu_intremap != iommu_intremap_off )
>> +        {
>> +            int rc = iommu_update_ire_from_msi(&ch->msi, &msg);
>>  
>> -    /* We may have missed an interrupt due to the temporary masking. */
>> -    if ( ch->event_handler && ch->next_event < NOW() )
>> -        ch->event_handler(ch);
>> +            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));
>> +            }
> 
> The sequence of asserts seem wrong here, the asserts inside of the rc
> > 0 check will never trigger, because there's an ASSERT(rc <= 0)
> ahead of them?

Hmm. My way of thinking was that if we get back 1 (which we shouldn't),
we ought to check (and presumably fail on) data or address having changed.
Whereas when we get back 0, we're told "no change" anyway, and hence
checking isn't even needed. Did I misunderstand the purpose of the zero
vs positive return value here?

Of could I could switch to using "rc >= 0" anyway; I actually had it that
way first, but then decided the extra checks would be redundant in the 0
case.

>> +        }
>> +        else
>> +            hpet_write32(msg.address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
> 
> If you avoid the HPET register update here you possibly need to make
> sure that both fields are unconditionally written on the first call
> after resume from suspension.  hpet_resume() needs to somehow taint
> the channels to signal that a re-write of the address and data fields
> is mandatory regardless of what iommu_update_ire_from_msi() has
> returned.

hpet_broadcast_resume() calls __hpet_setup_msi_irq() (and hence
hpet_msi_write()), which I thought is enough?

Jan

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

[Roger Pau Monné]

On Tue, Oct 21, 2025 at 08:42:13AM +0200, Jan Beulich wrote:
> On 20.10.2025 18:22, Roger Pau Monné wrote:
> > On Mon, Oct 20, 2025 at 01:18:34PM +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), avoid any "ordinary" vectors altogther
> >> and use a vector from the 0x10...0x1f exception vector space. Exception vs
> >> interrupt can easily be distinguished by checking for the presence of an
> >> error code.
> >>
> >> 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.
> >>
> >> In principle a change to setup_vector_irq() would be necessary, but only
> >> if we used low-prio vectors as direct-APIC ones. Since the change would be
> >> at best benign here, it is being omitted.
> >>
> >> 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 ... (If so, this
> >> likely would want to be a separate patch, though.)
> > 
> > I think that needs to become a no-op, with possibly an ASSERT?  Is it
> > possibly for dom0 to try to balance this IRQ?  I would think not.
> 
> I'd consider it an error if that was possible. But then the same goes for
> other Xen-internal IRQs, like the IOMMU ones. They all implement a
> .set_affinity hook ...

We need such hook for fixup_irqs() at least, so that interrupts can be
evacuated when an AP goes offline.  However movement of Xen owned IRQs
should be limited to Xen (if it's not already).

> >> @@ -476,19 +486,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;
> >> +
> >> +        if ( iommu_intremap != iommu_intremap_off )
> >> +        {
> >> +            int rc = iommu_update_ire_from_msi(&ch->msi, &msg);
> >>  
> >> -    /* We may have missed an interrupt due to the temporary masking. */
> >> -    if ( ch->event_handler && ch->next_event < NOW() )
> >> -        ch->event_handler(ch);
> >> +            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));
> >> +            }
> > 
> > The sequence of asserts seem wrong here, the asserts inside of the rc
> > > 0 check will never trigger, because there's an ASSERT(rc <= 0)
> > ahead of them?
> 
> Hmm. My way of thinking was that if we get back 1 (which we shouldn't),
> we ought to check (and presumably fail on) data or address having changed.

Right, but the ASSERT(rc <= 0) will prevent reaching any of the
followup ASSERTs if rc == 1?  IOW, we possibly want:

            if ( rc > 0 )
            {
                dprintk(XENLOG_ERR,
                        "Unexpected HPET MSI setup returned: data: %#x address: %#lx expected data %#x address %#lx\n",
                        msg.data, msg.address,
                        ch->msi.msg.data, ch->msi.msg.address);
                ASSERT_UNREACHABLE();
                hpet_msi_mask(desc);
                hpet_write32(msg.data, HPET_Tn_ROUTE(ch->idx));
                hpet_write32(msg.address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
                hpet_msi_unmask(desc);
            }
            ASSERT(!rc);

I'm unsure about attempting to propagate the returned values on release
builds, I guess it's slightly better than possibly using an outdated
RTE entry?  Albeit this should never happen.

Also, should the desc->arch.cpu_mask update only be done after the MSI
fields have correctly updated, so that in case of failure of
iommu_update_ire_from_msi(9 we could return early form the function
and avoid changing desc->arch.cpu_mask?

> Whereas when we get back 0, we're told "no change" anyway, and hence
> checking isn't even needed. Did I misunderstand the purpose of the zero
> vs positive return value here?
> 
> Of could I could switch to using "rc >= 0" anyway; I actually had it that
> way first, but then decided the extra checks would be redundant in the 0
> case.
> 
> >> +        }
> >> +        else
> >> +            hpet_write32(msg.address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
> > 
> > If you avoid the HPET register update here you possibly need to make
> > sure that both fields are unconditionally written on the first call
> > after resume from suspension.  hpet_resume() needs to somehow taint
> > the channels to signal that a re-write of the address and data fields
> > is mandatory regardless of what iommu_update_ire_from_msi() has
> > returned.
> 
> hpet_broadcast_resume() calls __hpet_setup_msi_irq() (and hence
> hpet_msi_write()), which I thought is enough?

Oh, sorry, I was looking at hpet_resume(), not
hpet_broadcast_resume().

Thanks, Roger.

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

[Jan Beulich]

On 21.10.2025 15:49, Roger Pau Monné wrote:
> On Tue, Oct 21, 2025 at 08:42:13AM +0200, Jan Beulich wrote:
>> On 20.10.2025 18:22, Roger Pau Monné wrote:
>>> On Mon, Oct 20, 2025 at 01:18:34PM +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), avoid any "ordinary" vectors altogther
>>>> and use a vector from the 0x10...0x1f exception vector space. Exception vs
>>>> interrupt can easily be distinguished by checking for the presence of an
>>>> error code.
>>>>
>>>> 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.
>>>>
>>>> In principle a change to setup_vector_irq() would be necessary, but only
>>>> if we used low-prio vectors as direct-APIC ones. Since the change would be
>>>> at best benign here, it is being omitted.
>>>>
>>>> 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 ... (If so, this
>>>> likely would want to be a separate patch, though.)
>>>
>>> I think that needs to become a no-op, with possibly an ASSERT?  Is it
>>> possibly for dom0 to try to balance this IRQ?  I would think not.
>>
>> I'd consider it an error if that was possible. But then the same goes for
>> other Xen-internal IRQs, like the IOMMU ones. They all implement a
>> .set_affinity hook ...
> 
> We need such hook for fixup_irqs() at least, so that interrupts can be
> evacuated when an AP goes offline.

Hmm, yes. Just not here.

>>>> @@ -476,19 +486,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;
>>>> +
>>>> +        if ( iommu_intremap != iommu_intremap_off )
>>>> +        {
>>>> +            int rc = iommu_update_ire_from_msi(&ch->msi, &msg);
>>>>  
>>>> -    /* We may have missed an interrupt due to the temporary masking. */
>>>> -    if ( ch->event_handler && ch->next_event < NOW() )
>>>> -        ch->event_handler(ch);
>>>> +            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));
>>>> +            }
>>>
>>> The sequence of asserts seem wrong here, the asserts inside of the rc
>>>> 0 check will never trigger, because there's an ASSERT(rc <= 0)
>>> ahead of them?
>>
>> Hmm. My way of thinking was that if we get back 1 (which we shouldn't),
>> we ought to check (and presumably fail on) data or address having changed.
> 
> Right, but the ASSERT(rc <= 0) will prevent reaching any of the
> followup ASSERTs if rc == 1?

Which is no problem, as we'd be dead already anyway if the first assertion
triggered. Nevertheless I've switched the if() to >= 0 (which then pointed
out a necessary change in AMD IOMMU code).

>  IOW, we possibly want:
> 
>             if ( rc > 0 )
>             {
>                 dprintk(XENLOG_ERR,
>                         "Unexpected HPET MSI setup returned: data: %#x address: %#lx expected data %#x address %#lx\n",
>                         msg.data, msg.address,
>                         ch->msi.msg.data, ch->msi.msg.address);
>                 ASSERT_UNREACHABLE();
>                 hpet_msi_mask(desc);
>                 hpet_write32(msg.data, HPET_Tn_ROUTE(ch->idx));
>                 hpet_write32(msg.address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
>                 hpet_msi_unmask(desc);
>             }
>             ASSERT(!rc);

To be honest, for my taste this goes too far as to what follows an
ASSERT_UNREACHABLE().

> I'm unsure about attempting to propagate the returned values on release
> builds, I guess it's slightly better than possibly using an outdated
> RTE entry?  Albeit this should never happen.

Yes to the last remark; I don't actually see what you would want to do
with the propagated return value.

> Also, should the desc->arch.cpu_mask update only be done after the MSI
> fields have correctly updated, so that in case of failure of
> iommu_update_ire_from_msi(9 we could return early form the function
> and avoid changing desc->arch.cpu_mask?

Hmm, yes, I could do that, but then also in hpet_msi_set_affinity().
However, as this needs doing under the IRQ descriptor lock, I'd have to
either extend the locked region here (again), or re-acquire the lock
later. Neither looks very attractive to me.

Jan

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

[Jan Beulich]

On 22.10.2025 11:21, Jan Beulich wrote:
> On 21.10.2025 15:49, Roger Pau Monné wrote:
>> On Tue, Oct 21, 2025 at 08:42:13AM +0200, Jan Beulich wrote:
>>> On 20.10.2025 18:22, Roger Pau Monné wrote:
>>>> On Mon, Oct 20, 2025 at 01:18:34PM +0200, Jan Beulich wrote:
>>>>> @@ -476,19 +486,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;
>>>>> +
>>>>> +        if ( iommu_intremap != iommu_intremap_off )
>>>>> +        {
>>>>> +            int rc = iommu_update_ire_from_msi(&ch->msi, &msg);
>>>>>  
>>>>> -    /* We may have missed an interrupt due to the temporary masking. */
>>>>> -    if ( ch->event_handler && ch->next_event < NOW() )
>>>>> -        ch->event_handler(ch);
>>>>> +            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));
>>>>> +            }
>>>>
>>>> The sequence of asserts seem wrong here, the asserts inside of the rc
>>>>> 0 check will never trigger, because there's an ASSERT(rc <= 0)
>>>> ahead of them?
>>>
>>> Hmm. My way of thinking was that if we get back 1 (which we shouldn't),
>>> we ought to check (and presumably fail on) data or address having changed.
>>
>> Right, but the ASSERT(rc <= 0) will prevent reaching any of the
>> followup ASSERTs if rc == 1?
> 
> Which is no problem, as we'd be dead already anyway if the first assertion
> triggered. Nevertheless I've switched the if() to >= 0 (which then pointed
> out a necessary change in AMD IOMMU code).
> 
>>  IOW, we possibly want:
>>
>>             if ( rc > 0 )
>>             {
>>                 dprintk(XENLOG_ERR,
>>                         "Unexpected HPET MSI setup returned: data: %#x address: %#lx expected data %#x address %#lx\n",
>>                         msg.data, msg.address,
>>                         ch->msi.msg.data, ch->msi.msg.address);
>>                 ASSERT_UNREACHABLE();
>>                 hpet_msi_mask(desc);
>>                 hpet_write32(msg.data, HPET_Tn_ROUTE(ch->idx));
>>                 hpet_write32(msg.address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
>>                 hpet_msi_unmask(desc);
>>             }
>>             ASSERT(!rc);
> 
> To be honest, for my taste this goes too far as to what follows an
> ASSERT_UNREACHABLE().

And it's insufficient: If we suspected the need for a non-atomic update,
the channel would need disabling before the IOMMU update.

Jan

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

[Roger Pau Monné]

On Wed, Oct 22, 2025 at 11:21:15AM +0200, Jan Beulich wrote:
> On 21.10.2025 15:49, Roger Pau Monné wrote:
> > On Tue, Oct 21, 2025 at 08:42:13AM +0200, Jan Beulich wrote:
> >> On 20.10.2025 18:22, Roger Pau Monné wrote:
> >>> On Mon, Oct 20, 2025 at 01:18:34PM +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), avoid any "ordinary" vectors altogther
> >>>> and use a vector from the 0x10...0x1f exception vector space. Exception vs
> >>>> interrupt can easily be distinguished by checking for the presence of an
> >>>> error code.
> >>>>
> >>>> 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.
> >>>>
> >>>> In principle a change to setup_vector_irq() would be necessary, but only
> >>>> if we used low-prio vectors as direct-APIC ones. Since the change would be
> >>>> at best benign here, it is being omitted.
> >>>>
> >>>> 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 ... (If so, this
> >>>> likely would want to be a separate patch, though.)
> >>>
> >>> I think that needs to become a no-op, with possibly an ASSERT?  Is it
> >>> possibly for dom0 to try to balance this IRQ?  I would think not.
> >>
> >> I'd consider it an error if that was possible. But then the same goes for
> >> other Xen-internal IRQs, like the IOMMU ones. They all implement a
> >> .set_affinity hook ...
> > 
> > We need such hook for fixup_irqs() at least, so that interrupts can be
> > evacuated when an AP goes offline.
> 
> Hmm, yes. Just not here.
> 
> >>>> @@ -476,19 +486,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;
> >>>> +
> >>>> +        if ( iommu_intremap != iommu_intremap_off )
> >>>> +        {
> >>>> +            int rc = iommu_update_ire_from_msi(&ch->msi, &msg);
> >>>>  
> >>>> -    /* We may have missed an interrupt due to the temporary masking. */
> >>>> -    if ( ch->event_handler && ch->next_event < NOW() )
> >>>> -        ch->event_handler(ch);
> >>>> +            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));
> >>>> +            }
> >>>
> >>> The sequence of asserts seem wrong here, the asserts inside of the rc
> >>>> 0 check will never trigger, because there's an ASSERT(rc <= 0)
> >>> ahead of them?
> >>
> >> Hmm. My way of thinking was that if we get back 1 (which we shouldn't),
> >> we ought to check (and presumably fail on) data or address having changed.
> > 
> > Right, but the ASSERT(rc <= 0) will prevent reaching any of the
> > followup ASSERTs if rc == 1?
> 
> Which is no problem, as we'd be dead already anyway if the first assertion
> triggered. Nevertheless I've switched the if() to >= 0 (which then pointed
> out a necessary change in AMD IOMMU code).

Right, so and adjusted if condition plus an ASSERT_UNREACHABLE() at
the end of the if code block?

> >  IOW, we possibly want:
> > 
> >             if ( rc > 0 )
> >             {
> >                 dprintk(XENLOG_ERR,
> >                         "Unexpected HPET MSI setup returned: data: %#x address: %#lx expected data %#x address %#lx\n",
> >                         msg.data, msg.address,
> >                         ch->msi.msg.data, ch->msi.msg.address);
> >                 ASSERT_UNREACHABLE();
> >                 hpet_msi_mask(desc);
> >                 hpet_write32(msg.data, HPET_Tn_ROUTE(ch->idx));
> >                 hpet_write32(msg.address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
> >                 hpet_msi_unmask(desc);
> >             }
> >             ASSERT(!rc);
> 
> To be honest, for my taste this goes too far as to what follows an
> ASSERT_UNREACHABLE().

I can understand that.  It's the best way I've come up with attempting
to recover from a possible error in the release case, but I don't
particularly like it either.

> > I'm unsure about attempting to propagate the returned values on release
> > builds, I guess it's slightly better than possibly using an outdated
> > RTE entry?  Albeit this should never happen.
> 
> Yes to the last remark; I don't actually see what you would want to do
> with the propagated return value.

OK, I can this this not being clear.  By propagate here I mean
propagate to the hardware registers, not to the function caller.

> > Also, should the desc->arch.cpu_mask update only be done after the MSI
> > fields have correctly updated, so that in case of failure of
> > iommu_update_ire_from_msi(9 we could return early form the function
> > and avoid changing desc->arch.cpu_mask?
> 
> Hmm, yes, I could do that, but then also in hpet_msi_set_affinity().
> However, as this needs doing under the IRQ descriptor lock, I'd have to
> either extend the locked region here (again), or re-acquire the lock
> later. Neither looks very attractive to me.

Hm, I guess given the point in the release we can leave it as-is.  It
would be nice, but this change is big enough as it is.

Thanks, Roger.

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

[Jan Beulich]

On 23.10.2025 10:39, Roger Pau Monné wrote:
> On Wed, Oct 22, 2025 at 11:21:15AM +0200, Jan Beulich wrote:
>> On 21.10.2025 15:49, Roger Pau Monné wrote:
>>> On Tue, Oct 21, 2025 at 08:42:13AM +0200, Jan Beulich wrote:
>>>> On 20.10.2025 18:22, Roger Pau Monné wrote:
>>>>> On Mon, Oct 20, 2025 at 01:18:34PM +0200, Jan Beulich wrote:
>>>>>> @@ -476,19 +486,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;
>>>>>> +
>>>>>> +        if ( iommu_intremap != iommu_intremap_off )
>>>>>> +        {
>>>>>> +            int rc = iommu_update_ire_from_msi(&ch->msi, &msg);
>>>>>>  
>>>>>> -    /* We may have missed an interrupt due to the temporary masking. */
>>>>>> -    if ( ch->event_handler && ch->next_event < NOW() )
>>>>>> -        ch->event_handler(ch);
>>>>>> +            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));
>>>>>> +            }
>>>>>
>>>>> The sequence of asserts seem wrong here, the asserts inside of the rc
>>>>>> 0 check will never trigger, because there's an ASSERT(rc <= 0)
>>>>> ahead of them?
>>>>
>>>> Hmm. My way of thinking was that if we get back 1 (which we shouldn't),
>>>> we ought to check (and presumably fail on) data or address having changed.
>>>
>>> Right, but the ASSERT(rc <= 0) will prevent reaching any of the
>>> followup ASSERTs if rc == 1?
>>
>> Which is no problem, as we'd be dead already anyway if the first assertion
>> triggered. Nevertheless I've switched the if() to >= 0 (which then pointed
>> out a necessary change in AMD IOMMU code).
> 
> Right, so and adjusted if condition plus an ASSERT_UNREACHABLE() at
> the end of the if code block?

That is, instead of

            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));
            }

you'd prefer

            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));
                ASSERT_UNREACHABLE();
            }

? That's wrong though (for rc == 0), i.e. I fear I don't see what you mean.

>>>  IOW, we possibly want:
>>>
>>>             if ( rc > 0 )
>>>             {
>>>                 dprintk(XENLOG_ERR,
>>>                         "Unexpected HPET MSI setup returned: data: %#x address: %#lx expected data %#x address %#lx\n",
>>>                         msg.data, msg.address,
>>>                         ch->msi.msg.data, ch->msi.msg.address);
>>>                 ASSERT_UNREACHABLE();
>>>                 hpet_msi_mask(desc);
>>>                 hpet_write32(msg.data, HPET_Tn_ROUTE(ch->idx));
>>>                 hpet_write32(msg.address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
>>>                 hpet_msi_unmask(desc);
>>>             }
>>>             ASSERT(!rc);
>>
>> To be honest, for my taste this goes too far as to what follows an
>> ASSERT_UNREACHABLE().
> 
> I can understand that.  It's the best way I've come up with attempting
> to recover from a possible error in the release case, but I don't
> particularly like it either.
> 
>>> I'm unsure about attempting to propagate the returned values on release
>>> builds, I guess it's slightly better than possibly using an outdated
>>> RTE entry?  Albeit this should never happen.
>>
>> Yes to the last remark; I don't actually see what you would want to do
>> with the propagated return value.
> 
> OK, I can this this not being clear.  By propagate here I mean
> propagate to the hardware registers, not to the function caller.

I.e. you still think adding the two hpet_write32() is going to be useful?
The mask/unmask, as I did say in another reply to your comments, isn't
useful here anyway (for already not being atomic), so I wouldn't see much
sense in having them. Plus of course we'd want to avoid the writes on
release builds if the values actually match, i.e. the construct would then
rather end up as two if-mismatch-then-write-else-assert-unreachable ones.

Just to mention - apart from this I have a working v3 ready to post.

Jan

Re: [PATCH v2 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 12:37:22PM +0200, Jan Beulich wrote:
> On 23.10.2025 10:39, Roger Pau Monné wrote:
> > On Wed, Oct 22, 2025 at 11:21:15AM +0200, Jan Beulich wrote:
> >> On 21.10.2025 15:49, Roger Pau Monné wrote:
> >>> On Tue, Oct 21, 2025 at 08:42:13AM +0200, Jan Beulich wrote:
> >>>> On 20.10.2025 18:22, Roger Pau Monné wrote:
> >>>>> On Mon, Oct 20, 2025 at 01:18:34PM +0200, Jan Beulich wrote:
> >>>>>> @@ -476,19 +486,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;
> >>>>>> +
> >>>>>> +        if ( iommu_intremap != iommu_intremap_off )
> >>>>>> +        {
> >>>>>> +            int rc = iommu_update_ire_from_msi(&ch->msi, &msg);
> >>>>>>  
> >>>>>> -    /* We may have missed an interrupt due to the temporary masking. */
> >>>>>> -    if ( ch->event_handler && ch->next_event < NOW() )
> >>>>>> -        ch->event_handler(ch);
> >>>>>> +            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));
> >>>>>> +            }
> >>>>>
> >>>>> The sequence of asserts seem wrong here, the asserts inside of the rc
> >>>>>> 0 check will never trigger, because there's an ASSERT(rc <= 0)
> >>>>> ahead of them?
> >>>>
> >>>> Hmm. My way of thinking was that if we get back 1 (which we shouldn't),
> >>>> we ought to check (and presumably fail on) data or address having changed.
> >>>
> >>> Right, but the ASSERT(rc <= 0) will prevent reaching any of the
> >>> followup ASSERTs if rc == 1?
> >>
> >> Which is no problem, as we'd be dead already anyway if the first assertion
> >> triggered. Nevertheless I've switched the if() to >= 0 (which then pointed
> >> out a necessary change in AMD IOMMU code).
> > 
> > Right, so and adjusted if condition plus an ASSERT_UNREACHABLE() at
> > the end of the if code block?
> 
> That is, instead of
> 
>             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));
>             }
> 
> you'd prefer
> 
>             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));
>                 ASSERT_UNREACHABLE();
>             }
> 
> ? That's wrong though (for rc == 0), i.e. I fear I don't see what you mean.

Oh, I see, sorry for the suggestions, it's indeed wrong.  FTAOD, what
do you plan to use then here?

You could replace the ASSERT_UNREACHABLE() for ASSERT(rc == 0) in my
suggestion I think?

Or maybe just do:

ASSERT(rc <= 0);
if ( !rc )
{
    ASSERT(msg.data == hpet_read32(HPET_Tn_ROUTE(ch->idx)));
    ASSERT(msg.address_lo ==
           hpet_read32(HPET_Tn_ROUTE(ch->idx) + 4));
}

Was your original intention with those checks to ensure that for the
rc == 0 case the message fields remained unchanged?

> >>>  IOW, we possibly want:
> >>>
> >>>             if ( rc > 0 )
> >>>             {
> >>>                 dprintk(XENLOG_ERR,
> >>>                         "Unexpected HPET MSI setup returned: data: %#x address: %#lx expected data %#x address %#lx\n",
> >>>                         msg.data, msg.address,
> >>>                         ch->msi.msg.data, ch->msi.msg.address);
> >>>                 ASSERT_UNREACHABLE();
> >>>                 hpet_msi_mask(desc);
> >>>                 hpet_write32(msg.data, HPET_Tn_ROUTE(ch->idx));
> >>>                 hpet_write32(msg.address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
> >>>                 hpet_msi_unmask(desc);
> >>>             }
> >>>             ASSERT(!rc);
> >>
> >> To be honest, for my taste this goes too far as to what follows an
> >> ASSERT_UNREACHABLE().
> > 
> > I can understand that.  It's the best way I've come up with attempting
> > to recover from a possible error in the release case, but I don't
> > particularly like it either.
> > 
> >>> I'm unsure about attempting to propagate the returned values on release
> >>> builds, I guess it's slightly better than possibly using an outdated
> >>> RTE entry?  Albeit this should never happen.
> >>
> >> Yes to the last remark; I don't actually see what you would want to do
> >> with the propagated return value.
> > 
> > OK, I can this this not being clear.  By propagate here I mean
> > propagate to the hardware registers, not to the function caller.
> 
> I.e. you still think adding the two hpet_write32() is going to be useful?
> The mask/unmask, as I did say in another reply to your comments, isn't
> useful here anyway (for already not being atomic), so I wouldn't see much
> sense in having them.

Right, for it to be correct the masking would need to include the
iommu_update_ire_from_msi() call also.

> Plus of course we'd want to avoid the writes on
> release builds if the values actually match, i.e. the construct would then
> rather end up as two if-mismatch-then-write-else-assert-unreachable ones.

My concern would be that after this change we won't cope anymore with
iommu_update_ire_from_msi() returning a value > 1.  Which might be
fine, as it's in theory not possible, but seems less robust than it
was before the change.  I guess it's the price to pay for avoiding the
masking (unless you have other options).

Looking at the existing code is likely no worse than when
iommu_update_ire_from_msi() returning an error, and that case is
already silently ignored by hpet_msi_set_affinity().  So I think
silently ignoring > 0 is not that different, and doesn't make the
current handling much worse.  It would be nice to handle those better,
but can be done separately.

Thanks, Roger.

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

[Jan Beulich]

On 23.10.2025 14:49, Roger Pau Monné wrote:
> On Thu, Oct 23, 2025 at 12:37:22PM +0200, Jan Beulich wrote:
>> On 23.10.2025 10:39, Roger Pau Monné wrote:
>>> On Wed, Oct 22, 2025 at 11:21:15AM +0200, Jan Beulich wrote:
>>>> On 21.10.2025 15:49, Roger Pau Monné wrote:
>>>>> On Tue, Oct 21, 2025 at 08:42:13AM +0200, Jan Beulich wrote:
>>>>>> On 20.10.2025 18:22, Roger Pau Monné wrote:
>>>>>>> On Mon, Oct 20, 2025 at 01:18:34PM +0200, Jan Beulich wrote:
>>>>>>>> @@ -476,19 +486,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;
>>>>>>>> +
>>>>>>>> +        if ( iommu_intremap != iommu_intremap_off )
>>>>>>>> +        {
>>>>>>>> +            int rc = iommu_update_ire_from_msi(&ch->msi, &msg);
>>>>>>>>  
>>>>>>>> -    /* We may have missed an interrupt due to the temporary masking. */
>>>>>>>> -    if ( ch->event_handler && ch->next_event < NOW() )
>>>>>>>> -        ch->event_handler(ch);
>>>>>>>> +            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));
>>>>>>>> +            }
>>>>>>>
>>>>>>> The sequence of asserts seem wrong here, the asserts inside of the rc
>>>>>>>> 0 check will never trigger, because there's an ASSERT(rc <= 0)
>>>>>>> ahead of them?
>>>>>>
>>>>>> Hmm. My way of thinking was that if we get back 1 (which we shouldn't),
>>>>>> we ought to check (and presumably fail on) data or address having changed.
>>>>>
>>>>> Right, but the ASSERT(rc <= 0) will prevent reaching any of the
>>>>> followup ASSERTs if rc == 1?
>>>>
>>>> Which is no problem, as we'd be dead already anyway if the first assertion
>>>> triggered. Nevertheless I've switched the if() to >= 0 (which then pointed
>>>> out a necessary change in AMD IOMMU code).
>>>
>>> Right, so and adjusted if condition plus an ASSERT_UNREACHABLE() at
>>> the end of the if code block?
>>
>> That is, instead of
>>
>>             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));
>>             }
>>
>> you'd prefer
>>
>>             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));
>>                 ASSERT_UNREACHABLE();
>>             }
>>
>> ? That's wrong though (for rc == 0), i.e. I fear I don't see what you mean.
> 
> Oh, I see, sorry for the suggestions, it's indeed wrong.  FTAOD, what
> do you plan to use then here?

The earlier of the two code fragments presented above.

> You could replace the ASSERT_UNREACHABLE() for ASSERT(rc == 0) in my
> suggestion I think?
> 
> Or maybe just do:
> 
> ASSERT(rc <= 0);
> if ( !rc )
> {
>     ASSERT(msg.data == hpet_read32(HPET_Tn_ROUTE(ch->idx)));
>     ASSERT(msg.address_lo ==
>            hpet_read32(HPET_Tn_ROUTE(ch->idx) + 4));
> }
> 
> Was your original intention with those checks to ensure that for the
> rc == 0 case the message fields remained unchanged?

Well, originally the condition was "rc > 0", mainly to document the expectations.
Now that I changed it to "rc >= 0" (helping to find an issue in AMD IOMMU code
that needed addressing), I'm indeed (also) after checking for "no change" in the
"rc == 0" case. Hence why I've ended up with the code fragment above.

I guess it might be best if I post v3 before leaving today, and we take it from
there.

Jan

[PATCH v2 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>
---
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 v2 for-4.21 3/9] x86/HPET: replace handle_hpet_broadcast()'s on-stack cpumask_t

[Jan Beulich]

On 20.10.2025 13:19, 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>

While the cover letter (intentionally) says otherwise, I decided to leave the
4.21 tag in place here: We may want to consider this one, as making the
original problem less severe (leaving patch 2 out of the picture).

Jan

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

[Roger Pau Monné]

On Mon, Oct 20, 2025 at 01:19:20PM +0200, 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>

> ---
> 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);

Should this be declared in the hpet.h header?

>  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;

And then this defined in hpet.c.

Just thinking whether someone would like to introduce support for
build time disabling HPET in the future.

Can always be moved at a later time anyway.

Thanks, Roger.

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

[Jan Beulich]

On 21.10.2025 16:08, Roger Pau Monné wrote:
> On Mon, Oct 20, 2025 at 01:19:20PM +0200, 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>

Thanks.

>> --- 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);
> 
> Should this be declared in the hpet.h header?

Imo not without also ...

>> --- 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;
> 
> And then this defined in hpet.c.

... moving this. Which in turn I specifically avoided putting in hpet.c,
as otherwise the change would further grow, as would the risk of things
going out of sync.

Jan

[PATCH v2 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)
@@ -657,7 +657,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;
@@ -808,7 +808,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 v2 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
@@ -723,7 +723,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 v2 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
@@ -802,13 +802,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 v2 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 v2 for-4.21 8/9] x86/HPET: don't use hardcoded 0 for "long timeout"

[Jan Beulich]

With 32-bit counters, writing 0 means on average half the wrapping period
until an interrupt would be raised. Yet of course in extreme cases an
interrupt would be raised almost right away. Write the present counter
value instead, to make the timeout predicatbly a full wrapping period.

Fixes: e862b83e8433 ("CPUIDLE: Avoid remnant HPET intr while force hpetbroadcast")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Actually - can't we just disable the channel instead of setting a "long"
timeout? Of course we'd then also need to enable it a few lines down. Yet
in turn we could then remove the enabling from set_channel_irq_affinity().

--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -165,7 +165,7 @@ static int reprogram_hpet_evt_channel(
     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));
+        hpet_write32(hpet_read32(HPET_COUNTER), HPET_Tn_CMP(ch->idx));
         return 0;
     }
 

Re: [PATCH v2 for-4.21 8/9] x86/HPET: don't use hardcoded 0 for "long timeout"

[Jan Beulich]

On 20.10.2025 13:21, Jan Beulich wrote:
> With 32-bit counters, writing 0 means on average half the wrapping period
> until an interrupt would be raised. Yet of course in extreme cases an
> interrupt would be raised almost right away. Write the present counter
> value instead, to make the timeout predicatbly a full wrapping period.
> 
> Fixes: e862b83e8433 ("CPUIDLE: Avoid remnant HPET intr while force hpetbroadcast")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Whereas here I'm sorry: I screwed up and left the 4.21 tag in place.

Jan

Re: [PATCH v2 for-4.21 8/9] x86/HPET: don't use hardcoded 0 for "long timeout"

[Roger Pau Monné]

On Mon, Oct 20, 2025 at 01:21:18PM +0200, Jan Beulich wrote:
> With 32-bit counters, writing 0 means on average half the wrapping period
> until an interrupt would be raised. Yet of course in extreme cases an
> interrupt would be raised almost right away. Write the present counter
> value instead, to make the timeout predicatbly a full wrapping period.
> 
> Fixes: e862b83e8433 ("CPUIDLE: Avoid remnant HPET intr while force hpetbroadcast")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Maybe I'm missing a path, but are there any callers that still pass
STIME_MAX to reprogram_hpet_evt_channel()?

hpet_broadcast_exit() no longer explicitly passes STIME_MAX like it
did in e862b83e8433, and then the per-cpu timer_deadline variable is
not set to STIME_MAX either in timer_softirq_action().

Thanks, Roger.

Re: [PATCH v2 for-4.21 8/9] x86/HPET: don't use hardcoded 0 for "long timeout"

[Jan Beulich]

On 21.10.2025 16:21, Roger Pau Monné wrote:
> On Mon, Oct 20, 2025 at 01:21:18PM +0200, Jan Beulich wrote:
>> With 32-bit counters, writing 0 means on average half the wrapping period
>> until an interrupt would be raised. Yet of course in extreme cases an
>> interrupt would be raised almost right away. Write the present counter
>> value instead, to make the timeout predicatbly a full wrapping period.
>>
>> Fixes: e862b83e8433 ("CPUIDLE: Avoid remnant HPET intr while force hpetbroadcast")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Maybe I'm missing a path, but are there any callers that still pass
> STIME_MAX to reprogram_hpet_evt_channel()?
> 
> hpet_broadcast_exit() no longer explicitly passes STIME_MAX like it
> did in e862b83e8433, and then the per-cpu timer_deadline variable is
> not set to STIME_MAX either in timer_softirq_action().

Good point. It was me missing the fact that "expire" is an unaltered function
argument; I (blindly) assumed it would be calculated there. So yes, that
check can simply go away.

Jan

[PATCH v2 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;