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

["Tu Dinh" <ngoc-tu.dinh@xxxxxxxxxx>]

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

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