Re: [Xen-devel] [PATCH v3 13/15] x86/IRQ: tighten vector checks

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, May 17, 2019 at 04:52:32AM -0600, Jan Beulich wrote:
> Use valid_irq_vector() rather than "> 0".
> 
> Also replace an open-coded use of IRQ_VECTOR_UNASSIGNED.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

The question I have below is not directly related to the usage of
valid_irq_vector, but rather with the existing code.

> ---
> v3: New.
> 
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -342,7 +342,7 @@ void clear_irq_vector(int irq)
>  
>  int irq_to_vector(int irq)
>  {
> -    int vector = -1;
> +    int vector = IRQ_VECTOR_UNASSIGNED;
>  
>      BUG_ON(irq >= nr_irqs || irq < 0);
>  
> @@ -452,15 +452,18 @@ static vmask_t *irq_get_used_vector_mask
>              int vector;
>              
>              vector = irq_to_vector(irq);
> -            if ( vector > 0 )
> +            if ( valid_irq_vector(vector) )
>              {
> -                printk(XENLOG_INFO "IRQ %d already assigned vector %d\n",
> +                printk(XENLOG_INFO "IRQ%d already assigned vector %02x\n",
>                         irq, vector);
>                  
>                  ASSERT(!test_bit(vector, ret));
>  
>                  set_bit(vector, ret);
>              }
> +            else if ( vector != IRQ_VECTOR_UNASSIGNED )
> +                printk(XENLOG_WARNING "IRQ%d mapped to bogus vector %02x\n",
> +                       irq, vector);

Maybe add an assert_unreachable here? It seems really bogus to call
irq_get_used_vector_mask with an unassigned vector.

But I'm not sure I fully understand this piece of code, neither why a
vector without a IRQ assigned can have a vector assigned. Is this
covering up for the lack of cleanup elsewhere?

Thanks, Roger.

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