Re: [PATCH v1 6/8] xen/arm: vgic: Optimize the way to store the target vCPU in the rank

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
> Xen is currently directly storing the value of register GICD_ITARGETSR
> (for GICv2) and GICD_IROUTER (for GICv3) in the rank. This makes the
> emulation of the registers access very simple but makes the code to get
> the target vCPU for a given IRQ more complex.
> 
> While the target vCPU of an IRQ is retrieved everytime an IRQ is
> injected to the guest, the access to the register occurs less often.
> 
> So the data structure should be optimized for the most common case
> rather than the inverse.
> 
> This patch introduce the usage of an array to store the target vCPU for
> every interrupt in the rank. This will make the code to get the target
> very quick. The emulation code will now have to generate the
> GICD_ITARGETSR
> and GICD_IROUTER register for read access and split it to store in a
> convenient way.
> 
> Note that with these changes, any read to those registers will list only
> the target vCPU used by Xen. This is fine because the GIC spec doesn't
> require to return exactly the value written and it can be seen as if we
> decide to implement the register read-only.

I think this is probably OK, but skirting round what the spec actually says
a fair bit.

> Finally take the opportunity to fix coding style in section we are
> modifying.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
> ---
>     The real reason is I'd like to drop vgic_byte_* helpers in favor of
> more
>     generic access helpers. Although it would make the code to retrieve
> the
>     priority more complex. So reworking the code to get the target vCPU
>     was the best solution.
> 
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/vgic-v2.c     | 172 ++++++++++++++++++++++++++-------------
> ------
>  xen/arch/arm/vgic-v3.c     | 121 +++++++++++++++++--------------
>  xen/arch/arm/vgic.c        |  28 ++++++--
>  xen/include/asm-arm/vgic.h |  13 ++--
>  4 files changed, 197 insertions(+), 137 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 23d1982..b6db64f 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -50,6 +50,90 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase,
> paddr_t vbase)
>      vgic_v2_hw.vbase = vbase;
>  }
>  
> +#define NR_TARGET_PER_REG 4U
> +#define NR_BIT_PER_TARGET 8U
> +
> +/*
> + * Generate the associated ITARGETSR register based on the offset from
> + * ITARGETSR0. Only one vCPU will be listed for a given IRQ.
> + *
> + * Note the offset will be round down to match a real HW register.

"rounded down".

Although I'm not 100% sure what that comment is trying to say. From the
code I think you mean aligned to the appropriate 4 byte boundary?

> + */
> +static uint32_t vgic_generate_itargetsr(struct vgic_irq_rank *rank,

For all these why not s/generate/read/?

Or since you use "store" for the other half "fetch".

("generate" is a bit of an implementation detail, the caller doesn't care
where or how the result is achieved)

> +                                        unsigned int offset)
> +{
> +    uint32_t reg = 0;
> +    unsigned int i;
> +
> +    ASSERT(spin_is_locked(&rank->lock));
> +
> +    offset &= INTERRUPT_RANK_MASK;
> +    offset &= ~(NR_TARGET_PER_REG - 1);
> +
> +    for ( i = 0; i < NR_TARGET_PER_REG; i++, offset++ )
> +        reg |= (1 << rank->vcpu[offset]) << (i * NR_BIT_PER_TARGET);
> +
> +    return reg;
> +}
> +
> +/*
> + * Store a ITARGETSR register in a convenient way and migrate the IRQ
> + * if necessary.
> + * Note the offset will be round down to match a real HW register.

As above.

> + */
> +static void vgic_store_itargetsr(struct domain *d, struct vgic_irq_rank
> *rank,
> +                                 unsigned int offset, uint32_t
> itargetsr)
> +{
> +    unsigned int i, rank_idx;
> +
> +    ASSERT(spin_is_locked(&rank->lock));
> +
> +    rank_idx = offset / NR_INTERRUPT_PER_RANK;
> +
> +    offset &= INTERRUPT_RANK_MASK;
> +    offset &= ~(NR_TARGET_PER_REG - 1);
> +
> +    for ( i = 0; i < NR_TARGET_PER_REG; i++, offset++ )
> +    {
> +        unsigned int new_target, old_target;
> +
> +        /*
> +         * SPI are using the 1-N model (see 1.4.3 in ARM IHI 0048B).
> +         * While the interrupt could be set pending to all the vCPUs in
> +         * target list, it's not guarantee by the spec.
> +         * For simplicity, always route the IRQ to the first interrupt
> +         * in the target list
> +         */

I don't see anything which is handling the PPI and SGI special case (which
is that they are r/o).

Likewise on read they are supposed to always reflect the value of the CPU
doing the read.

Are both of those handled elsewhere in some way I'm missing?

[...]
> + * Note the offset will be round down to match a real HW register

As earlier.

> + * Note the offset will be round down to match a real HW register.

Again.

> + */
> +static void vgic_store_irouter(struct domain *d, struct vgic_irq_rank
> *rank,
> +                               unsigned int offset, uint64_t irouter)
> +{
> +    struct vcpu *new_vcpu, *old_vcpu;
> +    unsigned int rank_idx, irq;
> +
> +
> +    /* There is 1 IRQ per IROUTER */
> +    irq = offset / NR_BYTE_PER_IROUTER;
> +
> +    rank_idx = irq / NR_INTERRUPT_PER_RANK;
> +
> +    /* Get the index in the rank */
> +    offset &= irq & INTERRUPT_RANK_MASK;
> +
> +    new_vcpu = vgic_v3_irouter_to_vcpu(d, irouter);
> +    old_vcpu = d->vcpu[rank->vcpu[offset]];
> +
> +    /*
> +     * From the spec (see 8.9.13 in IHI 0069A), any write with an
> +     * invalid vCPU will lead to ignore the interrupt.

"to the interrupt being ignored"

>  static struct vcpu *vgic_v3_get_target_vcpu(struct vcpu *v, unsigned int
> irq)
>  {
> -    struct vcpu *v_target;
>      struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
>  
>      ASSERT(spin_is_locked(&rank->lock));
>  
> -    v_target = vgic_v3_irouter_to_vcpu(v->domain, rank->v3.irouter[irq %
> 32]);
> -
> -    ASSERT(v_target != NULL);
> -
> -    return v_target;
> +    return v->domain->vcpu[rank->vcpu[irq & INTERRUPT_RANK_MASK]];


Looks like there isn't much call for this to be specific to a given
revision of the gic any more?

Are there sufficient checks for the range of rank->vcpu[...] elsewhere that
we needn't worry about running of domain->vcpu[] here?

Ian.