Re: [Xen-devel] [PATCH] vhpet: check that the set interrupt route is valid

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.05.18 at 11:08, <roger.pau@xxxxxxxxxx> wrote:
> @@ -304,6 +313,30 @@ static inline uint64_t hpet_fixup_reg(
>      return new;
>  }
>  
> +static void timer_sanitize_int(HPETState *h, unsigned int tn)

"int" as sort of a suffix here might be misleading (could be viewed as some
sort of integer as well) - how about "route" (or "int_route") instead?

> +{
> +    unsigned int irq;
> +
> +    if ( timer_int_valid(h, tn) )
> +        return;
> +
> +    h->hpet.timers[tn].config &= ~HPET_TN_ROUTE;

Please don't open-code timer_config() (also below), despite there being
quite a few examples in pre-existing code.

> +    if ( !timer_enabled(h, tn) )
> +        return;
> +
> +    /*
> +     * If the requested interrupt is not valid and the timer is
> +     * enabled pick the first irq.
> +     */
> +    irq = ffs(timer_int_route_cap(h, tn));
> +    if ( !irq )
> +    {
> +        ASSERT_UNREACHABLE();
> +        return;
> +    }
> +    h->hpet.timers[tn].config |= (irq - 1) << HPET_TN_ROUTE_SHIFT;

I think I would prefer find_first_set_bit() instead of ffs() here, to avoid the
subtraction of 1. Also please use MASK_INSR() here - HPET_TN_ROUTE_SHIFT
is a #define that should actually go away.

I'm also unconvinced the assertion is really useful here - the field in question
is r/o, and we initialize it unconditionally in hpet_set().

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel