* [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 related [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 related [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, other threads:[~2020-07-31 12:39 UTC | newest] 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).