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

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 29/09/15 14:07, Ian Campbell wrote:
> 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.

Well, nothing in the spec clearly explain the behavior of a read access
on the register. An implementation could decide to make some bits RO or
even not store everything.

FWIW, KVM is using the same trick.

>> Finally take the opportunity to fix coding style in section we are
>> modifying.
>>
>> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>

[...]

>> 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?

Yes. What about: "Note the offset will be 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)

I will rename to vgic_fetch_itargetsr(...).


[...]

>> + */
>> +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?

A write to those registers is ignored and handled by a separate case
(GICD_ITARGETSR ... GICD_ITARGETSR + 7).

[...]


>>  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?

Correct, although I keep them as it is because I wasn't sure how this
would fit with the ITS support.

Though we could add LPIs specific helpers if necessary. So I will add a
patch to drop the callbacks get_target_vcpu and get_irq_priority.

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

The only places where rank->vcpu[...] is set are vgic_store_* and at the
initialization time (always routed to vCPU 0).
Before the new value is stored, we check the validity of the vCPU the
value should always be valid here.

Regards,

-- 
Julien Grall

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