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)
> Signed-off-by: Tu Dinh <[email protected]>
> ---
> 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
