Re: [PATCH] x86/vhpet: Fix sanitization of legacy IRQ route

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>
>>> ---
>>>   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