xen/arch/x86/hvm/hpet.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
When setting a timer's config register, timer_sanitize_int_route will
always reset the IRQ route value to what's valid corresponding to the
!HPET_CFG_LEGACY case. This is applied even if the HPET is set to
HPET_CFG_LEGACY.
When some operating systems (e.g. Windows) try to write to a timer
config, they will verify and rewrite the register if the values don't
match what they expect. This causes an unnecessary write to HPET_Tn_CFG.
Note, the HPET specification states that for the Tn_INT_ROUTE_CNF field:
"If the value is not supported by this prarticular timer, then the value
read back will not match what is written. [...] If the LegacyReplacement
Route bit is set, then Timers 0 and 1 will have a different routing, and
this bit field has no effect for those two timers."
Therefore, Xen should not reset timer_int_route if legacy mode is
enabled, regardless of what's in there.
Signed-off-by: Tu Dinh <ngoc-tu.dinh@vates.tech>
---
xen/arch/x86/hvm/hpet.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index f0e5f877f4..fb2f4f94aa 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -48,6 +48,8 @@
#define timer_is_32bit(h, n) (timer_config(h, n) & HPET_TN_32BIT)
#define hpet_enabled(h) ((h)->hpet.config & HPET_CFG_ENABLE)
#define timer_level(h, n) (timer_config(h, n) & HPET_TN_LEVEL)
+#define timer_is_legacy(h, n) \
+ (((n) <= 1) && ((h)->hpet.config & HPET_CFG_LEGACY))
#define timer_int_route(h, n) MASK_EXTR(timer_config(h, n), HPET_TN_ROUTE)
@@ -244,7 +246,7 @@ static void hpet_set_timer(HPETState *h, unsigned int tn,
(timer_level(h, tn) && test_bit(tn, &h->hpet.isr)) )
return;
- if ( !timer_int_route_valid(h, tn) )
+ if ( !timer_is_legacy(h, tn) && !timer_int_route_valid(h, tn) )
{
ASSERT_UNREACHABLE();
return;
@@ -275,7 +277,7 @@ static void hpet_set_timer(HPETState *h, unsigned int tn,
? (uint32_t)diff : 0;
destroy_periodic_time(&h->pt[tn]);
- if ( (tn <= 1) && (h->hpet.config & HPET_CFG_LEGACY) )
+ if ( timer_is_legacy(h, tn) )
{
/* if LegacyReplacementRoute bit is set, HPET specification requires
timer0 be routed to IRQ0 in NON-APIC or IRQ2 in the I/O APIC,
@@ -323,7 +325,7 @@ static inline uint64_t hpet_fixup_reg(
static void timer_sanitize_int_route(HPETState *h, unsigned int tn)
{
- if ( timer_int_route_valid(h, tn) )
+ if ( timer_is_legacy(h, tn) || timer_int_route_valid(h, tn) )
return;
timer_config(h, tn) &= ~HPET_TN_ROUTE;
@@ -379,6 +381,9 @@ static int cf_check hpet_write(
h->hpet.config = hpet_fixup_reg(new_val, old_val,
HPET_CFG_ENABLE | HPET_CFG_LEGACY);
+ for ( i = 0; i < HPET_TIMER_NUM; i++ )
+ timer_sanitize_int_route(h, i);
+
if ( !(old_val & HPET_CFG_ENABLE) && (new_val & HPET_CFG_ENABLE) )
{
/* Enable main counter and interrupt generation. */
--
2.43.0
--
Ngoc Tu Dinh | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.techOn 24.11.2025 14:43, Tu Dinh wrote:
> When setting a timer's config register, timer_sanitize_int_route will
> always reset the IRQ route value to what's valid corresponding to the
> !HPET_CFG_LEGACY case. This is applied even if the HPET is set to
> HPET_CFG_LEGACY.
>
> When some operating systems (e.g. Windows) try to write to a timer
> config, they will verify and rewrite the register if the values don't
> match what they expect. This causes an unnecessary write to HPET_Tn_CFG.
>
> Note, the HPET specification states that for the Tn_INT_ROUTE_CNF field:
>
> "If the value is not supported by this prarticular timer, then the value
> read back will not match what is written. [...] If the LegacyReplacement
> Route bit is set, then Timers 0 and 1 will have a different routing, and
> this bit field has no effect for those two timers."
>
> Therefore, Xen should not reset timer_int_route if legacy mode is
> enabled, regardless of what's in there.
Fixes: ec40d3fe2147 ("x86/vhpet: check that the set interrupt route is valid")
(I think)
> Signed-off-by: Tu Dinh <ngoc-tu.dinh@vates.tech>
> ---
> xen/arch/x86/hvm/hpet.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> --- a/xen/arch/x86/hvm/hpet.c
> +++ b/xen/arch/x86/hvm/hpet.c
> @@ -48,6 +48,8 @@
> #define timer_is_32bit(h, n) (timer_config(h, n) & HPET_TN_32BIT)
> #define hpet_enabled(h) ((h)->hpet.config & HPET_CFG_ENABLE)
> #define timer_level(h, n) (timer_config(h, n) & HPET_TN_LEVEL)
> +#define timer_is_legacy(h, n) \
> + (((n) <= 1) && ((h)->hpet.config & HPET_CFG_LEGACY))
>
> #define timer_int_route(h, n) MASK_EXTR(timer_config(h, n), HPET_TN_ROUTE)
>
> @@ -244,7 +246,7 @@ static void hpet_set_timer(HPETState *h, unsigned int tn,
> (timer_level(h, tn) && test_bit(tn, &h->hpet.isr)) )
> return;
>
> - if ( !timer_int_route_valid(h, tn) )
> + if ( !timer_is_legacy(h, tn) && !timer_int_route_valid(h, tn) )
Seeing this and the other use together with timer_int_route_valid(),
wouldn't timer_int_route_valid() better itself invoke the new macro?
> @@ -379,6 +381,9 @@ static int cf_check hpet_write(
> h->hpet.config = hpet_fixup_reg(new_val, old_val,
> HPET_CFG_ENABLE | HPET_CFG_LEGACY);
>
> + for ( i = 0; i < HPET_TIMER_NUM; i++ )
> + timer_sanitize_int_route(h, i);
Strictly speaking this is needed only when HPET_CFG_LEGACY went from
1 to 0. Plus it's needed only on the first 2 channels, as it's only
there where timer_sanitize_int_route() changes behavior. I'm not going
to insist to limit it like this, but if you don't, then I'd like to ask
for a comment here clarifying that excess work is done for simplicity's
sake.
Jan
On 24/11/2025 15:53, Jan Beulich wrote:
> On 24.11.2025 14:43, Tu Dinh wrote:
>> When setting a timer's config register, timer_sanitize_int_route will
>> always reset the IRQ route value to what's valid corresponding to the
>> !HPET_CFG_LEGACY case. This is applied even if the HPET is set to
>> HPET_CFG_LEGACY.
>>
>> When some operating systems (e.g. Windows) try to write to a timer
>> config, they will verify and rewrite the register if the values don't
>> match what they expect. This causes an unnecessary write to HPET_Tn_CFG.
>>
>> Note, the HPET specification states that for the Tn_INT_ROUTE_CNF field:
>>
>> "If the value is not supported by this prarticular timer, then the value
>> read back will not match what is written. [...] If the LegacyReplacement
>> Route bit is set, then Timers 0 and 1 will have a different routing, and
>> this bit field has no effect for those two timers."
>>
>> Therefore, Xen should not reset timer_int_route if legacy mode is
>> enabled, regardless of what's in there.
>
> Fixes: ec40d3fe2147 ("x86/vhpet: check that the set interrupt route is valid")
> (I think)
Yes, thanks.
>
>> Signed-off-by: Tu Dinh <ngoc-tu.dinh@vates.tech>
>> ---
>> xen/arch/x86/hvm/hpet.c | 11 ++++++++---
>> 1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> --- a/xen/arch/x86/hvm/hpet.c
>> +++ b/xen/arch/x86/hvm/hpet.c
>> @@ -48,6 +48,8 @@
>> #define timer_is_32bit(h, n) (timer_config(h, n) & HPET_TN_32BIT)
>> #define hpet_enabled(h) ((h)->hpet.config & HPET_CFG_ENABLE)
>> #define timer_level(h, n) (timer_config(h, n) & HPET_TN_LEVEL)
>> +#define timer_is_legacy(h, n) \
>> + (((n) <= 1) && ((h)->hpet.config & HPET_CFG_LEGACY))
>>
>> #define timer_int_route(h, n) MASK_EXTR(timer_config(h, n), HPET_TN_ROUTE)
>>
>> @@ -244,7 +246,7 @@ static void hpet_set_timer(HPETState *h, unsigned int tn,
>> (timer_level(h, tn) && test_bit(tn, &h->hpet.isr)) )
>> return;
>>
>> - if ( !timer_int_route_valid(h, tn) )
>> + if ( !timer_is_legacy(h, tn) && !timer_int_route_valid(h, tn) )
>
> Seeing this and the other use together with timer_int_route_valid(),
> wouldn't timer_int_route_valid() better itself invoke the new macro?
I thought about it, but I found that it was overloading the definition
of timer_int_route_valid a little too much. Since timer_is_legacy() is
being used by itself later anyway, I figured it'd be better to just
separate the two.
>
>> @@ -379,6 +381,9 @@ static int cf_check hpet_write(
>> h->hpet.config = hpet_fixup_reg(new_val, old_val,
>> HPET_CFG_ENABLE | HPET_CFG_LEGACY);
>>
>> + for ( i = 0; i < HPET_TIMER_NUM; i++ )
>> + timer_sanitize_int_route(h, i);
>
> Strictly speaking this is needed only when HPET_CFG_LEGACY went from
> 1 to 0. Plus it's needed only on the first 2 channels, as it's only
> there where timer_sanitize_int_route() changes behavior. I'm not going
> to insist to limit it like this, but if you don't, then I'd like to ask
> for a comment here clarifying that excess work is done for simplicity's
> sake.
Agreed, I can limit it to i <= 1.
I'll prepare a v2.
>
> Jan
>
--
Ngoc Tu Dinh | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
On 24.11.2025 16:02, Tu Dinh wrote:
> On 24/11/2025 15:53, Jan Beulich wrote:
>> On 24.11.2025 14:43, Tu Dinh wrote:
>>> When setting a timer's config register, timer_sanitize_int_route will
>>> always reset the IRQ route value to what's valid corresponding to the
>>> !HPET_CFG_LEGACY case. This is applied even if the HPET is set to
>>> HPET_CFG_LEGACY.
>>>
>>> When some operating systems (e.g. Windows) try to write to a timer
>>> config, they will verify and rewrite the register if the values don't
>>> match what they expect. This causes an unnecessary write to HPET_Tn_CFG.
>>>
>>> Note, the HPET specification states that for the Tn_INT_ROUTE_CNF field:
>>>
>>> "If the value is not supported by this prarticular timer, then the value
>>> read back will not match what is written. [...] If the LegacyReplacement
>>> Route bit is set, then Timers 0 and 1 will have a different routing, and
>>> this bit field has no effect for those two timers."
According to this, ...
>>> Therefore, Xen should not reset timer_int_route if legacy mode is
>>> enabled, regardless of what's in there.
>>
>> Fixes: ec40d3fe2147 ("x86/vhpet: check that the set interrupt route is valid")
>> (I think)
>
> Yes, thanks.
>
>>
>>> Signed-off-by: Tu Dinh <ngoc-tu.dinh@vates.tech>
>>> ---
>>> xen/arch/x86/hvm/hpet.c | 11 ++++++++---
>>> 1 file changed, 8 insertions(+), 3 deletions(-)
>>>
>>> --- a/xen/arch/x86/hvm/hpet.c
>>> +++ b/xen/arch/x86/hvm/hpet.c
>>> @@ -48,6 +48,8 @@
>>> #define timer_is_32bit(h, n) (timer_config(h, n) & HPET_TN_32BIT)
>>> #define hpet_enabled(h) ((h)->hpet.config & HPET_CFG_ENABLE)
>>> #define timer_level(h, n) (timer_config(h, n) & HPET_TN_LEVEL)
>>> +#define timer_is_legacy(h, n) \
>>> + (((n) <= 1) && ((h)->hpet.config & HPET_CFG_LEGACY))
>>>
>>> #define timer_int_route(h, n) MASK_EXTR(timer_config(h, n), HPET_TN_ROUTE)
>>>
>>> @@ -244,7 +246,7 @@ static void hpet_set_timer(HPETState *h, unsigned int tn,
>>> (timer_level(h, tn) && test_bit(tn, &h->hpet.isr)) )
>>> return;
>>>
>>> - if ( !timer_int_route_valid(h, tn) )
>>> + if ( !timer_is_legacy(h, tn) && !timer_int_route_valid(h, tn) )
>>
>> Seeing this and the other use together with timer_int_route_valid(),
>> wouldn't timer_int_route_valid() better itself invoke the new macro?
>
> I thought about it, but I found that it was overloading the definition
> of timer_int_route_valid a little too much. Since timer_is_legacy() is
> being used by itself later anyway, I figured it'd be better to just
> separate the two.
... the route setting is valid (because of being ignored) when in legacy
mode. Hence why I think the check wants integrating there.
>>> @@ -379,6 +381,9 @@ static int cf_check hpet_write(
>>> h->hpet.config = hpet_fixup_reg(new_val, old_val,
>>> HPET_CFG_ENABLE | HPET_CFG_LEGACY);
>>>
>>> + for ( i = 0; i < HPET_TIMER_NUM; i++ )
>>> + timer_sanitize_int_route(h, i);
>>
>> Strictly speaking this is needed only when HPET_CFG_LEGACY went from
>> 1 to 0. Plus it's needed only on the first 2 channels, as it's only
>> there where timer_sanitize_int_route() changes behavior. I'm not going
>> to insist to limit it like this, but if you don't, then I'd like to ask
>> for a comment here clarifying that excess work is done for simplicity's
>> sake.
>
> Agreed, I can limit it to i <= 1.
May I ask that you avoid such open-coding and use timer_is_legacy(h, i) as
the loop continuation expression instead?
Jan
On 24/11/2025 16:09, Jan Beulich wrote:
> On 24.11.2025 16:02, Tu Dinh wrote:
>> On 24/11/2025 15:53, Jan Beulich wrote:
>>> On 24.11.2025 14:43, Tu Dinh wrote:
>>>> When setting a timer's config register, timer_sanitize_int_route will
>>>> always reset the IRQ route value to what's valid corresponding to the
>>>> !HPET_CFG_LEGACY case. This is applied even if the HPET is set to
>>>> HPET_CFG_LEGACY.
>>>>
>>>> When some operating systems (e.g. Windows) try to write to a timer
>>>> config, they will verify and rewrite the register if the values don't
>>>> match what they expect. This causes an unnecessary write to HPET_Tn_CFG.
>>>>
>>>> Note, the HPET specification states that for the Tn_INT_ROUTE_CNF field:
>>>>
>>>> "If the value is not supported by this prarticular timer, then the value
>>>> read back will not match what is written. [...] If the LegacyReplacement
>>>> Route bit is set, then Timers 0 and 1 will have a different routing, and
>>>> this bit field has no effect for those two timers."
>
> According to this, ...
>
>>>> Therefore, Xen should not reset timer_int_route if legacy mode is
>>>> enabled, regardless of what's in there.
>>>
>>> Fixes: ec40d3fe2147 ("x86/vhpet: check that the set interrupt route is valid")
>>> (I think)
>>
>> Yes, thanks.
>>
>>>
>>>> Signed-off-by: Tu Dinh <ngoc-tu.dinh@vates.tech>
>>>> ---
>>>> xen/arch/x86/hvm/hpet.c | 11 ++++++++---
>>>> 1 file changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> --- a/xen/arch/x86/hvm/hpet.c
>>>> +++ b/xen/arch/x86/hvm/hpet.c
>>>> @@ -48,6 +48,8 @@
>>>> #define timer_is_32bit(h, n) (timer_config(h, n) & HPET_TN_32BIT)
>>>> #define hpet_enabled(h) ((h)->hpet.config & HPET_CFG_ENABLE)
>>>> #define timer_level(h, n) (timer_config(h, n) & HPET_TN_LEVEL)
>>>> +#define timer_is_legacy(h, n) \
>>>> + (((n) <= 1) && ((h)->hpet.config & HPET_CFG_LEGACY))
>>>>
>>>> #define timer_int_route(h, n) MASK_EXTR(timer_config(h, n), HPET_TN_ROUTE)
>>>>
>>>> @@ -244,7 +246,7 @@ static void hpet_set_timer(HPETState *h, unsigned int tn,
>>>> (timer_level(h, tn) && test_bit(tn, &h->hpet.isr)) )
>>>> return;
>>>>
>>>> - if ( !timer_int_route_valid(h, tn) )
>>>> + if ( !timer_is_legacy(h, tn) && !timer_int_route_valid(h, tn) )
>>>
>>> Seeing this and the other use together with timer_int_route_valid(),
>>> wouldn't timer_int_route_valid() better itself invoke the new macro?
>>
>> I thought about it, but I found that it was overloading the definition
>> of timer_int_route_valid a little too much. Since timer_is_legacy() is
>> being used by itself later anyway, I figured it'd be better to just
>> separate the two.
>
> ... the route setting is valid (because of being ignored) when in legacy
> mode. Hence why I think the check wants integrating there.
Okay, I can offload the check there.
>
>>>> @@ -379,6 +381,9 @@ static int cf_check hpet_write(
>>>> h->hpet.config = hpet_fixup_reg(new_val, old_val,
>>>> HPET_CFG_ENABLE | HPET_CFG_LEGACY);
>>>>
>>>> + for ( i = 0; i < HPET_TIMER_NUM; i++ )
>>>> + timer_sanitize_int_route(h, i);
>>>
>>> Strictly speaking this is needed only when HPET_CFG_LEGACY went from
>>> 1 to 0. Plus it's needed only on the first 2 channels, as it's only
>>> there where timer_sanitize_int_route() changes behavior. I'm not going
>>> to insist to limit it like this, but if you don't, then I'd like to ask
>>> for a comment here clarifying that excess work is done for simplicity's
>>> sake.
>>
>> Agreed, I can limit it to i <= 1.
>
> May I ask that you avoid such open-coding and use timer_is_legacy(h, i) as
> the loop continuation expression instead?
That wouldn't work as timer_is_legacy would check for hpet.config &
HPET_CFG_LEGACY, whereas we want to sanitize in the opposite case, i.e.
!(hpet.config & HPET_CFG_LEGACY).
It's probably better to use i < HPET_TIMER_NUM and add a comment as you
suggested.
>
> Jan
--
Ngoc Tu Dinh | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
© 2016 - 2025 Red Hat, Inc.