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

[Eslam Elnikety <elnikety@xxxxxxxxxx>]

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