|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH] xen/arm: Deliver interrupts to vcpu specified in IROUTER
On Fri, Sep 5, 2014 at 4:59 AM, Stefano Stabellini
<stefano.stabellini@xxxxxxxxxxxxx> wrote:
> On Thu, 4 Sep 2014, vijay.kilari@xxxxxxxxx wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>>
>> In GICv3 use IROUTER register contents to
>> deliver irq to specified vcpu's.
>>
>> In error scenario fallback to vcpu 0.
>>
>> This patch is similar to Stefano's commit
>> 5b3a817ea33b891caf7d7d788da9ce6deffa82a1 for GICv2
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>> ---
>> xen/arch/arm/vgic-v3.c | 115
>> ++++++++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 106 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> index ecda672..b01a201 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -45,6 +45,35 @@
>> #define GICV3_GICR_PIDR2 GICV3_GICD_PIDR2
>> #define GICV3_GICR_PIDR4 GICV3_GICD_PIDR4
>>
>> +static struct vcpu *vgicv3_irouter_to_vcpu(struct vcpu *v,
>> + uint64_t irouter_aff)
>> +{
>> + int i;
>> + uint64_t cpu_affinity;
>> + struct vcpu *v_target = NULL;
>> +
>> + /* If routing mode is set. Fallback to vcpu0 */
>> + if ( irouter_aff & GICD_IROUTER_SPI_MODE_ANY )
>> + return v->domain->vcpu[0];
>> +
>> + for ( i = 0; i < v->domain->max_vcpus; i++ )
>> + {
>> + v_target = v->domain->vcpu[i];
>> + cpu_affinity = (MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 3) <<
>> 32 |
>> + MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 2) <<
>> 16 |
>> + MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 1) << 8
>> |
>> + MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 0));
>> +
>> + if ( irouter_aff == cpu_affinity )
>> + return v_target;
>> + }
>
> Using a loop is pretty bad. Couldn't you just calculate the lowest vcpu
> number from irouter_aff (maybe using find_first_bit)?
IMO, If GICD_IROUTER_SPI_MODE_ANY bit is set, then it specifies any cpu.
If GICD_IROUTER_SPI_MODE_ANY is not set, it register specifies only
one cpu number.
So we cannot use find_first_bit() unlike GICv2 where ITARGET specifies
irq affinity
to more than one CPU.
If at all we want to avoid this for loop, then we have to maintain one
more variable for
every IROUTERn and corresponding vpcu number, which is updated on
IROUTERn update
>
>
>> + gdprintk(XENLOG_WARNING,
>> + "d%d: No vcpu match found for irouter 0x%lx\n",
>> + v->domain->domain_id, irouter_aff);
>> + /* XXX: Fallback to vcpu0? */
>> + return v->domain->vcpu[0];
>> +}
>> +
>> static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
>> uint32_t gicr_reg)
>> {
>> @@ -347,9 +376,9 @@ static int __vgic_v3_distr_common_mmio_write(struct vcpu
>> *v, mmio_info_t *info,
>> vgic_lock_rank(v, rank, flags);
>> tr = rank->ienable;
>> rank->ienable |= *r;
>> - vgic_unlock_rank(v, rank, flags);
>> /* The irq number is extracted from offset. so shift by register
>> size */
>> vgic_enable_irqs(v, (*r) & (~tr), (reg - GICD_ISENABLER) >>
>> DABT_WORD);
>> + vgic_unlock_rank(v, rank, flags);
>> return 1;
>> case GICD_ICENABLER ... GICD_ICENABLERN:
>> if ( dabt.size != DABT_WORD ) goto bad_width;
>> @@ -358,9 +387,9 @@ static int __vgic_v3_distr_common_mmio_write(struct vcpu
>> *v, mmio_info_t *info,
>> vgic_lock_rank(v, rank, flags);
>> tr = rank->ienable;
>> rank->ienable &= ~*r;
>> - vgic_unlock_rank(v, rank, flags);
>> /* The irq number is extracted from offset. so shift by register
>> size */
>> vgic_disable_irqs(v, (*r) & tr, (reg - GICD_ICENABLER) >>
>> DABT_WORD);
>> + vgic_unlock_rank(v, rank, flags);
>> return 1;
>> case GICD_ISPENDR ... GICD_ISPENDRN:
>> if ( dabt.size != DABT_WORD ) goto bad_width;
>> @@ -749,6 +778,9 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v,
>> mmio_info_t *info)
>> register_t *r = select_user_reg(regs, dabt.reg);
>> struct vgic_irq_rank *rank;
>> unsigned long flags;
>> + uint64_t new_target, old_target;
>> + struct vcpu *old_vcpu, *new_vcpu;
>> + int irq;
>> int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
>>
>> switch ( gicd_reg )
>> @@ -804,16 +836,42 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v,
>> mmio_info_t *info)
>> if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
>> rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER,
>> DABT_DOUBLE_WORD);
>> if ( rank == NULL) goto write_ignore_64;
>> - if ( *r )
>> + new_target = *r;
>> + vgic_lock_rank(v, rank, flags);
>> + old_target = rank->v3.irouter[REG_RANK_INDEX(64,
>> + (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)];
>> + if ( *r & GICD_IROUTER_SPI_MODE_ANY )
>> {
>> - /* TODO: Ignored. We don't support irq delivery for vcpu != 0 */
>> - gdprintk(XENLOG_DEBUG,
>> - "SPI delivery to secondary cpus not supported\n");
>> - goto write_ignore_64;
>> + /*
>> + * IRQ routing mode set. Route any one processor in the entire
>> + * system. We chose vcpu 0
>> + */
>> + rank->v3.irouter[REG_RANK_INDEX(64,
>> + (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = *r;
>> + vgic_unlock_rank(v, rank, flags);
>> + return 1;
>> }
>> - vgic_lock_rank(v, rank, flags);
>> + irq = (gicd_reg - GICD_IROUTER)/8;
>> + /* IROUTER now specifies only one cpu affinity for this irq */
>> + /* Migrate from any core(vcpu0) to new_target */
>> + if ( (old_target & GICD_IROUTER_SPI_MODE_ANY) &&
>> + !(new_target & GICD_IROUTER_SPI_MODE_ANY) )
>> + {
>> + new_vcpu = vgicv3_irouter_to_vcpu(v, new_target);
>> + vgic_migrate_irq(v->domain->vcpu[0], new_vcpu, irq);
>> + }
>> + else
>> + {
>> + if ( old_target != new_target )
>> + {
>> + old_vcpu = vgicv3_irouter_to_vcpu(v, old_target);
>> + new_vcpu = vgicv3_irouter_to_vcpu(v, new_target);
>> + vgic_migrate_irq(old_vcpu, new_vcpu, irq);
>> + }
>> + }
>
> The issue is that if irouter is GICD_IROUTER_SPI_MODE_ANY, you always
> return vcpu0 below.
> So if the kernel:
>
> 1) move the irq to vcpu1
> 2) set the irq as GICD_IROUTER_SPI_MODE_ANY
>
> you now have migrated the irq to vcpu1 but you are returning vcpu0 from
> vgicv3_get_target_vcpu.
>
> You can either:
> - vgic_migrate_irq the irq back to vcpu0 when the kernel sets the irq as
> GICD_IROUTER_SPI_MODE_ANY
When irq is set to vcpu1 and later when GICD_IROUTER_SPI_MODE_ANY
then I assume that vcpu1 is also valid for the moment if irq is pending.
So no need to do any migration.
Also, as per GICv3 spec, when GICD_IROUTER_SPI_MODE_ANY is set which is
previous not set ( 0 -> 1 ), the affinity value in IROUTER is invalid/unknown.
So later when vgicv3_get_target_vcpu() is called, it always returns vcpu0.
> - return the current vcpu for the irq rather than vcpu0 if
> GICD_IROUTER_SPI_MODE_ANY
>
> Either way it would work. Maybe the second option might be better.
>
>
>> rank->v3.irouter[REG_RANK_INDEX(64,
>> - (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] = *r;
>> + (gicd_reg - GICD_IROUTER), DABT_DOUBLE_WORD)] =
>> new_target;
>> vgic_unlock_rank(v, rank, flags);
>> return 1;
>> case GICD_NSACR ... GICD_NSACRN:
>> @@ -928,6 +986,44 @@ static const struct mmio_handler_ops
>> vgic_distr_mmio_handler = {
>> .write_handler = vgic_v3_distr_mmio_write,
>> };
>>
>> +static struct vcpu *vgicv3_get_target_vcpu(struct vcpu *v, unsigned int irq)
>
> for consistency I would call it vgic_v3_get_target_vcpu
>
>
>> +{
>> + int i;
>> + uint64_t irq_affinity, cpu_affinity;
>> + struct vcpu *v_target;
>> + struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
>> +
>> + ASSERT(spin_is_locked(&rank->lock));
>> +
>> + irq_affinity = rank->v3.irouter[irq % 32];
>> + if ( irq_affinity & GICD_IROUTER_SPI_MODE_ANY )
>> + {
>> + /*
>> + * IRQ routing mode set. Route any one processor in the entire
>> + * system. We chose vcpu 0
>> + */
>> + return v->domain->vcpu[0];
>> + }
>> +
>> + for ( i = 0; i < v->domain->max_vcpus; i++ )
>> + {
>> + v_target = v->domain->vcpu[i];
>> + cpu_affinity = (MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 3) <<
>> 32 |
>> + MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 2) <<
>> 16 |
>> + MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 1) << 8
>> |
>> + MPIDR_AFFINITY_LEVEL(v_target->arch.vmpidr, 0));
>> +
>> + if ( irq_affinity == cpu_affinity )
>> + return v_target;
>> + }
>
> Same comment about the loop.
>
>
>> + gdprintk(XENLOG_WARNING,
>> + "d%d: No vcpu match found for irq %d with irouter 0x%lx\n",
>> + v->domain->domain_id, irq, irq_affinity );
>> +
>> + /* XXX:Fallback to vcpu0 ? */
>> + return v->domain->vcpu[0];
>
> This is wrong: only valid values should be allowed in rank->v3.irouter.
> If the kernel writes a bad value to IROUTER we should catch it in
> vgic_v3_distr_mmio_write and avoid saving it.
0x0 is also valid value. I found that VMPIDR for cpu0 is 0x80000000.
Bit[31] is RES1 bit which is set. If we ignore bit[31], 0x0 is also valid
value.
Question: Shouldn't recover with wrong irouter value by choosing vcpu0?
Note: In next version, I will try to move common code in
vgicv3_get_target_vcpu()
and vgicv3_irouter_to_vcpu() to one single function
>
>
>> +}
>> +
>> static int vgicv3_vcpu_init(struct vcpu *v)
>> {
>> int i;
>> @@ -968,6 +1064,7 @@ static int vgicv3_domain_init(struct domain *d)
>> static const struct vgic_ops v3_ops = {
>> .vcpu_init = vgicv3_vcpu_init,
>> .domain_init = vgicv3_domain_init,
>> + .get_target_vcpu = vgicv3_get_target_vcpu,
>> .emulate_sysreg = vgicv3_emulate_sysreg,
>> };
>>
>> --
>> 1.7.9.5
>>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |