1. Patchew
  2. Xen
  3. View series

[PATCH] x86/HPET: channel handling in hpet_broadcast_resume()

Jan Beulich posted 1 patch 5 days, 7 hours ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/a77822d8-08f4-4c4f-b291-cc44a213cf9f@suse.com
[PATCH] x86/HPET: channel handling in hpet_broadcast_resume()
Posted by Jan Beulich 5 days, 7 hours ago 
The per-channel ENABLE bit is to solely be driven by hpet_enable_channel()
and hpet_msi_{,un}mask(). It doesn't need setting immediately. Except for
the (possible) channel put in legacy mode we don't do so during boot
either.

Instead reset ->arch.cpu_mask, to avoid msi_compose_msg() yielding an
all-zero message (when the passed in CPU mask has no online CPUs). Nothing
would later call msi_compose_msg() / hpet_msi_write(), and hence nothing
would later produce a well-formed message template in
hpet_events[].msi.msg.

Fixes: 15aa6c67486c ("amd iommu: use base platform MSI implementation")
Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
As to the Fixes: tag: The issue for the HPET resume case is the
cpumask_intersects(desc->arch.cpu_mask, &cpu_online_map) check in
msi_compose_msg(). The earlier cpumask_empty() wasn't a problem, as
cpu_mask_to_apicid() returning a bogus (offline) value didn't have any bad
effect: Before use, a valid destination would have been put in place, but
other parts of .msg were properly set up. Furthermore we also didn't clear
the entire message prior to that change.

Many thanks got to Marek for tirelessly trying out various debugging
suggestions.

--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -685,12 +685,18 @@ void hpet_broadcast_resume(void)
     for ( i = 0; i < n; i++ )
     {
         if ( hpet_events[i].msi.irq >= 0 )
+        {
+            struct irq_desc *desc = irq_to_desc(hpet_events[i].msi.irq);
+
+            cpumask_copy(desc->arch.cpu_mask, cpumask_of(smp_processor_id()));
+
             __hpet_setup_msi_irq(irq_to_desc(hpet_events[i].msi.irq));
+        }
 
         /* set HPET Tn as oneshot */
         cfg = hpet_read32(HPET_Tn_CFG(hpet_events[i].idx));
         cfg &= ~(HPET_TN_LEVEL | HPET_TN_PERIODIC);
-        cfg |= HPET_TN_ENABLE | HPET_TN_32BIT;
+        cfg |= HPET_TN_32BIT;
         if ( !(hpet_events[i].flags & HPET_EVT_LEGACY) )
             cfg |= HPET_TN_FSB;
         hpet_write32(cfg, HPET_Tn_CFG(hpet_events[i].idx));
Re: [PATCH] x86/HPET: channel handling in hpet_broadcast_resume()
Posted by Teddy Astie 5 days, 6 hours ago 
Le 07/04/2026 à 15:35, Jan Beulich a écrit :
> The per-channel ENABLE bit is to solely be driven by hpet_enable_channel()
> and hpet_msi_{,un}mask(). It doesn't need setting immediately. Except for
> the (possible) channel put in legacy mode we don't do so during boot
> either.
> 
> Instead reset ->arch.cpu_mask, to avoid msi_compose_msg() yielding an
> all-zero message (when the passed in CPU mask has no online CPUs). Nothing
> would later call msi_compose_msg() / hpet_msi_write(), and hence nothing
> would later produce a well-formed message template in
> hpet_events[].msi.msg.
> 
> Fixes: 15aa6c67486c ("amd iommu: use base platform MSI implementation")
> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> As to the Fixes: tag: The issue for the HPET resume case is the
> cpumask_intersects(desc->arch.cpu_mask, &cpu_online_map) check in
> msi_compose_msg(). The earlier cpumask_empty() wasn't a problem, as
> cpu_mask_to_apicid() returning a bogus (offline) value didn't have any bad
> effect: Before use, a valid destination would have been put in place, but
> other parts of .msg were properly set up. Furthermore we also didn't clear
> the entire message prior to that change.
> 
> Many thanks got to Marek for tirelessly trying out various debugging
> suggestions.
> 
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -685,12 +685,18 @@ void hpet_broadcast_resume(void)
>       for ( i = 0; i < n; i++ )
>       {
>           if ( hpet_events[i].msi.irq >= 0 )
> +        {
> +            struct irq_desc *desc = irq_to_desc(hpet_events[i].msi.irq);
> +
> +            cpumask_copy(desc->arch.cpu_mask, cpumask_of(smp_processor_id()));
> +
>               __hpet_setup_msi_irq(irq_to_desc(hpet_events[i].msi.irq));

We can directly reuse "desc" here since irq_to_desc(...) isn't supposed 
to change value with cpumask_copy().

i.e `__hpet_setup_msi_irq(desc);`

> +        }
>   
>           /* set HPET Tn as oneshot */
>           cfg = hpet_read32(HPET_Tn_CFG(hpet_events[i].idx));
>           cfg &= ~(HPET_TN_LEVEL | HPET_TN_PERIODIC);
> -        cfg |= HPET_TN_ENABLE | HPET_TN_32BIT;
> +        cfg |= HPET_TN_32BIT;
>           if ( !(hpet_events[i].flags & HPET_EVT_LEGACY) )
>               cfg |= HPET_TN_FSB;
>           hpet_write32(cfg, HPET_Tn_CFG(hpet_events[i].idx));
> 

Teddy


--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech
Re: [PATCH] x86/HPET: channel handling in hpet_broadcast_resume()
Posted by Jan Beulich 5 days, 5 hours ago 
On 07.04.2026 16:28, Teddy Astie wrote:
> Le 07/04/2026 à 15:35, Jan Beulich a écrit :
>> The per-channel ENABLE bit is to solely be driven by hpet_enable_channel()
>> and hpet_msi_{,un}mask(). It doesn't need setting immediately. Except for
>> the (possible) channel put in legacy mode we don't do so during boot
>> either.
>>
>> Instead reset ->arch.cpu_mask, to avoid msi_compose_msg() yielding an
>> all-zero message (when the passed in CPU mask has no online CPUs). Nothing
>> would later call msi_compose_msg() / hpet_msi_write(), and hence nothing
>> would later produce a well-formed message template in
>> hpet_events[].msi.msg.
>>
>> Fixes: 15aa6c67486c ("amd iommu: use base platform MSI implementation")
>> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>> ---
>> As to the Fixes: tag: The issue for the HPET resume case is the
>> cpumask_intersects(desc->arch.cpu_mask, &cpu_online_map) check in
>> msi_compose_msg(). The earlier cpumask_empty() wasn't a problem, as
>> cpu_mask_to_apicid() returning a bogus (offline) value didn't have any bad
>> effect: Before use, a valid destination would have been put in place, but
>> other parts of .msg were properly set up. Furthermore we also didn't clear
>> the entire message prior to that change.
>>
>> Many thanks got to Marek for tirelessly trying out various debugging
>> suggestions.
>>
>> --- a/xen/arch/x86/hpet.c
>> +++ b/xen/arch/x86/hpet.c
>> @@ -685,12 +685,18 @@ void hpet_broadcast_resume(void)
>>       for ( i = 0; i < n; i++ )
>>       {
>>           if ( hpet_events[i].msi.irq >= 0 )
>> +        {
>> +            struct irq_desc *desc = irq_to_desc(hpet_events[i].msi.irq);
>> +
>> +            cpumask_copy(desc->arch.cpu_mask, cpumask_of(smp_processor_id()));
>> +
>>               __hpet_setup_msi_irq(irq_to_desc(hpet_events[i].msi.irq));
> 
> We can directly reuse "desc" here since irq_to_desc(...) isn't supposed 
> to change value with cpumask_copy().
> 
> i.e `__hpet_setup_msi_irq(desc);`

Oh, indeed - how did I not spot this?

Jan

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.