Mailing List Archive

On Tue, Jul 28, 2020 at 08:33:57AM +0000, Eslam Elnikety wrote:
> The macro timer_int_route_cap evalutes to a 64 bit value. Extend the
> size of left side of timer_int_route_valid to match.

I'm very dull with this things, so forgive me.

Isn't the left side just promoted to an unsigned 64bit value?

Also timer_int_route will strictly be <= 31, which makes the shift
safe?

I'm not opposed to switching to use unsigned long, but I think I'm not
understanding the issue.

Thanks, Roger.

Hi Roger,

On 28.07.20 10:58, Roger Pau Monné wrote:
> On Tue, Jul 28, 2020 at 08:33:57AM +0000, Eslam Elnikety wrote:
>> The macro timer_int_route_cap evalutes to a 64 bit value. Extend the
>> size of left side of timer_int_route_valid to match.
>
> I'm very dull with this things, so forgive me.
>
> Isn't the left side just promoted to an unsigned 64bit value?
>
> Also timer_int_route will strictly be <= 31, which makes the shift
> safe?

This is all true. The size mismatch is indeed benign. The patch is only
for code sanity.

>
> I'm not opposed to switching to use unsigned long, but I think I'm not
> understanding the issue.
>
> Thanks, Roger.
>

Regards,
Eslam

On 28/07/2020 09:33, Eslam Elnikety wrote:
> The macro timer_int_route_cap evalutes to a 64 bit value. Extend the
> size of left side of timer_int_route_valid to match.
>
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
>
> Signed-off-by: Eslam Elnikety <elnikety@amazon.com>
> ---
> xen/arch/x86/hvm/hpet.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
> index ca94e8b453..9afe6e6760 100644
> --- a/xen/arch/x86/hvm/hpet.c
> +++ b/xen/arch/x86/hvm/hpet.c
> @@ -66,7 +66,7 @@
> MASK_EXTR(timer_config(h, n), HPET_TN_INT_ROUTE_CAP)
>
> #define timer_int_route_valid(h, n) \
> - ((1u << timer_int_route(h, n)) & timer_int_route_cap(h, n))
> + ((1ULL << timer_int_route(h, n)) & timer_int_route_cap(h, n))
>
> static inline uint64_t hpet_read_maincounter(HPETState *h, uint64_t guest_time)
> {

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
 
 #define timer_int_route_valid(h, n) \
     ((1u << timer_int_route(h, n)) & timer_int_route_cap(h, n))
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;
+            };
+        };
         uint64_t cmp;           /* comparator */
         uint64_t fsb;           /* FSB route, not supported now */
     } timers[HPET_TIMER_NUM];

On 28.07.20 11:26, Andrew Cooper wrote:
> On 28/07/2020 09:33, Eslam Elnikety wrote:
>> The macro timer_int_route_cap evalutes to a 64 bit value. Extend the
>> size of left side of timer_int_route_valid to match.
>>
>> This bug was discovered and resolved using Coverity Static Analysis
>> Security Testing (SAST) by Synopsys, Inc.
>>
>> Signed-off-by: Eslam Elnikety <elnikety@amazon.com>
>> ---
>> xen/arch/x86/hvm/hpet.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
>> index ca94e8b453..9afe6e6760 100644
>> --- a/xen/arch/x86/hvm/hpet.c
>> +++ b/xen/arch/x86/hvm/hpet.c
>> @@ -66,7 +66,7 @@
>> MASK_EXTR(timer_config(h, n), HPET_TN_INT_ROUTE_CAP)
>>
>> #define timer_int_route_valid(h, n) \
>> - ((1u << timer_int_route(h, n)) & timer_int_route_cap(h, n))
>> + ((1ULL << timer_int_route(h, n)) & timer_int_route_cap(h, n))
>>
>> static inline uint64_t hpet_read_maincounter(HPETState *h, uint64_t guest_time)
>> {
>
> Does this work?

Yes! This is better than my fix (and I like that it clarifies the route
part of the config. Will you sign-off and send a patch?

>
> 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
>
>  #define timer_int_route_valid(h, n) \
>      ((1u << timer_int_route(h, n)) & timer_int_route_cap(h, n))
> 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;
> +            };
> +        };
>          uint64_t cmp;           /* comparator */
>          uint64_t fsb;           /* FSB route, not supported now */
>      } timers[HPET_TIMER_NUM];
>
>

On 28/07/2020 12:09, Eslam Elnikety wrote:
> On 28.07.20 11:26, Andrew Cooper wrote:
>> On 28/07/2020 09:33, Eslam Elnikety wrote:
>>> The macro timer_int_route_cap evalutes to a 64 bit value. Extend the
>>> size of left side of timer_int_route_valid to match.
>>>
>>> This bug was discovered and resolved using Coverity Static Analysis
>>> Security Testing (SAST) by Synopsys, Inc.
>>>
>>> Signed-off-by: Eslam Elnikety <elnikety@amazon.com>
>>> ---
>>>   xen/arch/x86/hvm/hpet.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
>>> index ca94e8b453..9afe6e6760 100644
>>> --- a/xen/arch/x86/hvm/hpet.c
>>> +++ b/xen/arch/x86/hvm/hpet.c
>>> @@ -66,7 +66,7 @@
>>>       MASK_EXTR(timer_config(h, n), HPET_TN_INT_ROUTE_CAP)
>>>     #define timer_int_route_valid(h, n) \
>>> -    ((1u << timer_int_route(h, n)) & timer_int_route_cap(h, n))
>>> +    ((1ULL << timer_int_route(h, n)) & timer_int_route_cap(h, n))
>>>     static inline uint64_t hpet_read_maincounter(HPETState *h,
>>> uint64_t guest_time)
>>>   {
>>
>> Does this work?
>
> Yes! This is better than my fix (and I like that it clarifies the
> route part of the config. Will you sign-off and send a patch?

Any chance I can persuade you, or someone else to do this?  Loads of the
macros can be removed by filling in proper bitfield names in place of
'_', resulting in rather better code.

~Andrew

>
>>
>> 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
>>     #define timer_int_route_valid(h, n) \
>>       ((1u << timer_int_route(h, n)) & timer_int_route_cap(h, n))
>> 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;
>> +            };
>> +        };
>>           uint64_t cmp;           /* comparator */
>>           uint64_t fsb;           /* FSB route, not supported now */
>>       } timers[HPET_TIMER_NUM];
>>
>>
>

On 28.07.20 15:46, Andrew Cooper wrote:
> On 28/07/2020 12:09, Eslam Elnikety wrote:
>> On 28.07.20 11:26, Andrew Cooper wrote:
>>> On 28/07/2020 09:33, Eslam Elnikety wrote:
>>>> The macro timer_int_route_cap evalutes to a 64 bit value. Extend the
>>>> size of left side of timer_int_route_valid to match.
>>>>
>>>> This bug was discovered and resolved using Coverity Static Analysis
>>>> Security Testing (SAST) by Synopsys, Inc.
>>>>
>>>> Signed-off-by: Eslam Elnikety <elnikety@amazon.com>
>>>> ---
>>>>   xen/arch/x86/hvm/hpet.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
>>>> index ca94e8b453..9afe6e6760 100644
>>>> --- a/xen/arch/x86/hvm/hpet.c
>>>> +++ b/xen/arch/x86/hvm/hpet.c
>>>> @@ -66,7 +66,7 @@
>>>>       MASK_EXTR(timer_config(h, n), HPET_TN_INT_ROUTE_CAP)
>>>>     #define timer_int_route_valid(h, n) \
>>>> -    ((1u << timer_int_route(h, n)) & timer_int_route_cap(h, n))
>>>> +    ((1ULL << timer_int_route(h, n)) & timer_int_route_cap(h, n))
>>>>     static inline uint64_t hpet_read_maincounter(HPETState *h,
>>>> uint64_t guest_time)
>>>>   {
>>>
>>> Does this work?
>>
>> Yes! This is better than my fix (and I like that it clarifies the
>> route part of the config. Will you sign-off and send a patch?
>
> Any chance I can persuade you, or someone else to do this?  Loads of the
> macros can be removed by filling in proper bitfield names in place of
> '_', resulting in rather better code.
>
> ~Andrew
>

Sure, I can send a patch for this one occurrence at hand right away --
and I will keep my eye on the general pattern. Since the patch will be
mostly your diff, please send your sign-off (or another tag as you see fit).

Thanks,
Eslam

>>
>>>
>>> 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
>>>     #define timer_int_route_valid(h, n) \
>>>       ((1u << timer_int_route(h, n)) & timer_int_route_cap(h, n))
>>> 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;
>>> +            };
>>> +        };
>>>           uint64_t cmp;           /* comparator */
>>>           uint64_t fsb;           /* FSB route, not supported now */
>>>       } timers[HPET_TIMER_NUM];
>>>
>>>
>>
>
>

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.

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

Jan

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
>

On 31.07.2020 10:38, Eslam Elnikety wrote:
> On 28.07.20 19:51, Jan Beulich wrote:
>> On 28.07.2020 11:26, Andrew Cooper wrote:
>>> --- 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?

Well, as already said - the problem is with old gcc not dealing
well with initializers of structs/unions with unnamed fields.

> If the latter, is that a gcc version that we still care about?

Until someone makes a (justified) proposal what the new minimum
version(s) ought to be, I'm afraid we still have to care. This
topic came up very recently in another context, and I've proposed
to put it on the agenda of the next community call.

Jan

Hi Jan,

On 31/07/2020 10:53, Jan Beulich wrote:
> On 31.07.2020 10:38, Eslam Elnikety wrote:
>> On 28.07.20 19:51, Jan Beulich wrote:
>>> On 28.07.2020 11:26, Andrew Cooper wrote:
>>>> --- 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?
>
> Well, as already said - the problem is with old gcc not dealing
> well with initializers of structs/unions with unnamed fields.

You seem to know quite well the problem. So would you mind to give us
more details on which GCC version is believed to be broken?

>
>> If the latter, is that a gcc version that we still care about?
>
> Until someone makes a (justified) proposal what the new minimum
> version(s) ought to be, I'm afraid we still have to care. This
> topic came up very recently in another context, and I've proposed
> to put it on the agenda of the next community call.

I don't think Eslam was requesting to change the limits. He was just
asking whether one of the compiler we support is affected.

Cheers,


--
Julien Grall

On 31.07.2020 14:35, Julien Grall wrote:
> Hi Jan,
>
> On 31/07/2020 10:53, Jan Beulich wrote:
>> On 31.07.2020 10:38, Eslam Elnikety wrote:
>>> On 28.07.20 19:51, Jan Beulich wrote:
>>>> On 28.07.2020 11:26, Andrew Cooper wrote:
>>>>> --- 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?
>>
>> Well, as already said - the problem is with old gcc not dealing
>> well with initializers of structs/unions with unnamed fields.
>
> You seem to know quite well the problem. So would you mind to give us
> more details on which GCC version is believed to be broken?

I don't recall for sure, but iirc anything before 4.4.

Jan