Re: [PATCH v7 4/8] xen/arm: Use struct sgi_target instead of vcpu_mask in vgic_to_sgi

[Julien Grall <julien.grall@citrix.com>]

Hi Chen,

On 11/06/2015 09:05, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
>
> The old unsigned long type of vcpu_mask can only express 64 cpus at the
> most, which might not be enough for the guest which used vGICv3. We
> introduce a new struct sgi_target for the target cpu list of SGI, which
> holds the affinity path information. For GICv2 that has no affinity
> level, we can just set the corresponding fields to be 0.

The whole point of this patch is to add support for AFF1 in GICv3... not 
replacing vcpu_mask by sgi_target. The latter is just how you decided to 
implement AFF1.

Please ensure that your commit message/tittle reflect it.

[..]

>   static int vgic_v3_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 7b387b7..0173c8d 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -318,15 +318,14 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>       }
>   }
>
> -/* TODO: unsigned long is used to fit vcpu_mask.*/
>   int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int virq,
> -                unsigned long vcpu_mask)
> +                struct sgi_target *target)

const struct sgi_target...

[..]

> @@ -334,29 +333,40 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int
>       {
>       case SGI_TARGET_LIST:
>           perfc_incr(vgic_sgi_list);
> +        base = (target->aff1 << 4) | (target->aff2 << 12) |
> +               (target->aff3 << 20);
> +        bitmap = target->list;
> +        for_each_set_bit( i, &bitmap, sizeof(target->list)*8 )

Coding style.

sizeof(target->list) * 8

[..]

> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 463fffb..c6ef4bf 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -64,6 +64,7 @@
>   #define GICD_SGI_TARGET_SELF_VAL     (2)
>   #define GICD_SGI_TARGET_SHIFT        (16)
>   #define GICD_SGI_TARGET_MASK         (0xFFUL<<GICD_SGI_TARGET_SHIFT)
> +#define GICD_SGI_TARGET_BITS         (8)
>   #define GICD_SGI_GROUP1              (1UL<<15)
>   #define GICD_SGI_INTID_MASK          (0xFUL)
>
> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
> index 556f114..333aa56 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -152,6 +152,10 @@
>   #define ICH_SGI_IRQ_SHIFT            24
>   #define ICH_SGI_IRQ_MASK             0xf
>   #define ICH_SGI_TARGETLIST_MASK      0xffff
> +#define ICH_SGI_TARGET_BITS          16
> +#define ICH_SGI_AFFx_MASK            0xff
> +#define ICH_SGI_AFFINITY_LEVEL(x)    (16 * (x))
> +
>   #endif /* __ASM_ARM_GIC_V3_DEFS_H__ */
>
>   /*
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 6dcdf9f..cc68c27 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -98,6 +98,13 @@ struct vgic_irq_rank {
>       };
>   };
>
> +struct sgi_target {
> +    uint8_t aff3;
> +    uint8_t aff2;

I don't see why you are not using aff2 and aff3 at all the the vGICv* 
drivers. Please drop them.

> +    uint8_t aff1;
> +    uint16_t list;
> +};
> +

Regards,

-- 
Julien Grall