Re: [Xen-devel] [PATCH v6a 14/17] xen/arm: calculate vgic irq rank based on register size

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 06/26/2014 06:34 AM, vijay.kilari@xxxxxxxxx wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> 
> vGIC irq rank was computed assuming the register offset is byte
> size.Use the HSR abort address size in calculating register size.
> 
> So, with this patch following are acheived

s/acheived/achieved/

>    (1)  In the code 'dabt.size != number' this number is always
>         BYTE/HALF_WORD/WORD/DOUBLE defined by hsr registers.
>         Instead of checking for hard coded values use HSR abort
>         address size values.
>    (2) The vgic_irq_rank also depends on the same HSR defined

s/vgic_irq_rank/vgic_irq_rank_offset/

>        values to calculate irq rank.
> 
> This make vgic_irq_rank generic as it takes register

Same here.

> size as parameter to calculate irq rank instead of hard coding to
> value 2 in previous patches
> 
> Also, output of REG_RANK_INDEX macro is modulo by 32 to make
> sure register index is always within irq rank

I though a bit more about this patch. You are using the value of
DABT_{WORD,...} for multiple things that have non-sense together.

While it's perfectly fine to use it to check dabt.size, the
vgic_rank_offset is using this value for shift. Assuming that DABT_*
will always contains the right shift is completely wrong.

You have either to explain it somewhere or define a new set of defines.

> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index ebc683d..22a1998 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -251,6 +251,14 @@ extern struct cpuinfo_arm cpu_data[];
>  extern u32 __cpu_logical_map[];
>  #define cpu_logical_map(cpu) __cpu_logical_map[cpu]
>  
> +/* HSR data abort size definition */
> +enum dabt_size {
> +    DABT_BYTE        = 0,
> +    DABT_HALF_WORD   = 1,
> +    DABT_WORD        = 2,
> +    DABT_DOUBLE_WORD = 3,
> +};
> +

The enum looks like pointless here as you never use the defined type...

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel