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

[Eslam Elnikety <elnikety@xxxxxxxxxx>]

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