>>> On 10.05.18 at 11:08, <roger....@citrix.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
