Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] x86/vhpet: Fix type size in timer_int_route_valid
@ 2020-07-28  8:33 Eslam Elnikety
  2020-07-28  8:58 ` Roger Pau Monné
  2020-07-28  9:26 ` Andrew Cooper
  0 siblings, 2 replies; 12+ messages in thread
From: Eslam Elnikety @ 2020-07-28  8:33 UTC (permalink / raw)
  To: xen-devel
  Cc: Wei Liu, Andrew Cooper, Eslam Elnikety, Paul Durrant,
	Jan Beulich, Roger Pau Monné

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)
 {
-- 
2.16.6



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/vhpet: Fix type size in timer_int_route_valid
  2020-07-28  8:33 [PATCH] x86/vhpet: Fix type size in timer_int_route_valid Eslam Elnikety
@ 2020-07-28  8:58 ` Roger Pau Monné
  2020-07-28  9:14   ` Eslam Elnikety
  2020-07-28  9:26 ` Andrew Cooper
  1 sibling, 1 reply; 12+ messages in thread
From: Roger Pau Monné @ 2020-07-28  8:58 UTC (permalink / raw)
  To: Eslam Elnikety
  Cc: xen-devel, Andrew Cooper, Wei Liu, Jan Beulich, Paul Durrant

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.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/vhpet: Fix type size in timer_int_route_valid
  2020-07-28  8:58 ` Roger Pau Monné
@ 2020-07-28  9:14   ` Eslam Elnikety
  0 siblings, 0 replies; 12+ messages in thread
From: Eslam Elnikety @ 2020-07-28  9:14 UTC (permalink / raw)
  To: Roger Pau Monné, Eslam Elnikety
  Cc: xen-devel, Paul Durrant, Jan Beulich, Wei Liu, Andrew Cooper

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/vhpet: Fix type size in timer_int_route_valid
  2020-07-28  8:33 [PATCH] x86/vhpet: Fix type size in timer_int_route_valid Eslam Elnikety
  2020-07-28  8:58 ` Roger Pau Monné
@ 2020-07-28  9:26 ` Andrew Cooper
  2020-07-28 11:09   ` Eslam Elnikety
  2020-07-28 17:51   ` Jan Beulich
  1 sibling, 2 replies; 12+ messages in thread
From: Andrew Cooper @ 2020-07-28  9:26 UTC (permalink / raw)
  To: Eslam Elnikety, xen-devel
  Cc: Roger Pau Monné, Wei Liu, Jan Beulich, Paul Durrant

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];



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/vhpet: Fix type size in timer_int_route_valid
  2020-07-28  9:26 ` Andrew Cooper
@ 2020-07-28 11:09   ` Eslam Elnikety
  2020-07-28 13:46     ` Andrew Cooper
  2020-07-28 17:51   ` Jan Beulich
  1 sibling, 1 reply; 12+ messages in thread
From: Eslam Elnikety @ 2020-07-28 11:09 UTC (permalink / raw)
  To: Andrew Cooper, Eslam Elnikety, xen-devel
  Cc: Paul Durrant, Jan Beulich, Wei Liu, Roger Pau Monné

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];
> 
> 



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/vhpet: Fix type size in timer_int_route_valid
  2020-07-28 11:09   ` Eslam Elnikety
@ 2020-07-28 13:46     ` Andrew Cooper
  2020-07-28 13:55       ` Eslam Elnikety
  0 siblings, 1 reply; 12+ messages in thread
From: Andrew Cooper @ 2020-07-28 13:46 UTC (permalink / raw)
  To: Eslam Elnikety, xen-devel
  Cc: Paul Durrant, Jan Beulich, Wei Liu, Roger Pau Monné

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];
>>
>>
>



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/vhpet: Fix type size in timer_int_route_valid
  2020-07-28 13:46     ` Andrew Cooper
@ 2020-07-28 13:55       ` Eslam Elnikety
  0 siblings, 0 replies; 12+ messages in thread
From: Eslam Elnikety @ 2020-07-28 13:55 UTC (permalink / raw)
  To: Andrew Cooper, Eslam Elnikety, xen-devel
  Cc: Roger Pau Monné, Wei Liu, Jan Beulich, Paul Durrant

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];
>>>
>>>
>>
> 
> 



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/vhpet: Fix type size in timer_int_route_valid
  2020-07-28  9:26 ` Andrew Cooper
  2020-07-28 11:09   ` Eslam Elnikety
@ 2020-07-28 17:51   ` Jan Beulich
  2020-07-31  8:38     ` Eslam Elnikety
  1 sibling, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2020-07-28 17:51 UTC (permalink / raw)
  To: Andrew Cooper, Eslam Elnikety
  Cc: xen-devel, Roger Pau Monné, Wei Liu, Paul Durrant

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/vhpet: Fix type size in timer_int_route_valid
  2020-07-28 17:51   ` Jan Beulich
@ 2020-07-31  8:38     ` Eslam Elnikety
  2020-07-31  9:53       ` Jan Beulich
  0 siblings, 1 reply; 12+ messages in thread
From: Eslam Elnikety @ 2020-07-31  8:38 UTC (permalink / raw)
  To: Jan Beulich, Andrew Cooper, Eslam Elnikety
  Cc: xen-devel, Paul Durrant, Wei Liu, Roger Pau Monné

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
> 



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/vhpet: Fix type size in timer_int_route_valid
  2020-07-31  8:38     ` Eslam Elnikety
@ 2020-07-31  9:53       ` Jan Beulich
  2020-07-31 12:35         ` Julien Grall
  0 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2020-07-31  9:53 UTC (permalink / raw)
  To: Eslam Elnikety
  Cc: Andrew Cooper, Roger Pau Monné, Paul Durrant, Wei Liu, xen-devel

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/vhpet: Fix type size in timer_int_route_valid
  2020-07-31  9:53       ` Jan Beulich
@ 2020-07-31 12:35         ` Julien Grall
  2020-07-31 12:38           ` Jan Beulich
  0 siblings, 1 reply; 12+ messages in thread
From: Julien Grall @ 2020-07-31 12:35 UTC (permalink / raw)
  To: Jan Beulich, Eslam Elnikety
  Cc: Andrew Cooper, Paul Durrant, xen-devel, Wei Liu, Roger Pau Monné

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/vhpet: Fix type size in timer_int_route_valid
  2020-07-31 12:35         ` Julien Grall
@ 2020-07-31 12:38           ` Jan Beulich
  0 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2020-07-31 12:38 UTC (permalink / raw)
  To: Julien Grall
  Cc: Wei Liu, Andrew Cooper, Eslam Elnikety, Paul Durrant, xen-devel,
	Roger Pau Monné

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-28  8:33 [PATCH] x86/vhpet: Fix type size in timer_int_route_valid Eslam Elnikety
2020-07-28  8:58 ` Roger Pau Monné
2020-07-28  9:14   ` Eslam Elnikety
2020-07-28  9:26 ` Andrew Cooper
2020-07-28 11:09   ` Eslam Elnikety
2020-07-28 13:46     ` Andrew Cooper
2020-07-28 13:55       ` Eslam Elnikety
2020-07-28 17:51   ` Jan Beulich
2020-07-31  8:38     ` Eslam Elnikety
2020-07-31  9:53       ` Jan Beulich
2020-07-31 12:35         ` Julien Grall
2020-07-31 12:38           ` Jan Beulich

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git