Re: [PATCH] x86/vhpet: Fix type size in timer_int_route_valid

[Eslam Elnikety <elnikety@amazon.com>]

On 28.07.20 19:51, Jan Beulich wrote:
> On 28.07.2020 11:26, Andrew Cooper wrote:
>> Does this work?
>>
>> diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
>> index ca94e8b453..638f6174de 100644
>> --- a/xen/arch/x86/hvm/hpet.c
>> +++ b/xen/arch/x86/hvm/hpet.c
>> @@ -62,8 +62,7 @@
>>   #define timer_int_route(h, n)    MASK_EXTR(timer_config(h, n),
>> HPET_TN_ROUTE)
>> -#define timer_int_route_cap(h, n) \
>> -    MASK_EXTR(timer_config(h, n), HPET_TN_INT_ROUTE_CAP)
>> +#define timer_int_route_cap(h, n) (h)->hpet.timers[(n)].route
> 
> Seeing that this is likely the route taken here, and hence to avoid
> an extra round trip, two remarks: Here I see no need for the
> parentheses inside the square brackets.
> 

Will take of this in v2.

>> diff --git a/xen/include/asm-x86/hvm/vpt.h 
>> b/xen/include/asm-x86/hvm/vpt.h
>> index f0e0eaec83..a41fc443cc 100644
>> --- a/xen/include/asm-x86/hvm/vpt.h
>> +++ b/xen/include/asm-x86/hvm/vpt.h
>> @@ -73,7 +73,13 @@ struct hpet_registers {
>>       uint64_t isr;               /* interrupt status reg */
>>       uint64_t mc64;              /* main counter */
>>       struct {                    /* timers */
>> -        uint64_t config;        /* configuration/cap */
>> +        union {
>> +            uint64_t config;    /* configuration/cap */
>> +            struct {
>> +                uint32_t _;
>> +                uint32_t route;
>> +            };
>> +        };
> 
> So long as there are no static initializers for this construct
> that would then suffer the old-gcc problem, this is of course a
> fine arrangement to make.
> 

I have to admit that I have no clue what the "old-gcc" problem is. I am 
curious, and I would appreciate pointers to figure out if/how to 
resolve. Is that an old, existing problem? Or a problem that was present 
in older versions of gcc? If the latter, is that a gcc version that we 
still care about? Thanks, Jan.

-- Eslam

> Jan
>