|
[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
Hi Stefano,
On Sat, Sep 6, 2014 at 12:14 AM, Stefano Stabellini
<stefano.stabellini@xxxxxxxxxxxxx> wrote:
> On Fri, 5 Sep 2014, Vijay Kilari wrote:
>> 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
>
> Given that at the moment AFF0 is just set to vcpu_id (see
> vcpu_initialise), I would simply do a direct calculation, adding a TODO
> comment so that we remember to fix this when we implement smarter mpidr
> schemes.
Instead of storing vcpu_id in AFF0 of vgic irouter[], we could rather
store vcpu_id in irouter[]
similar to v2. One bit per vcpu, though in v3 always only one bit is set.
The conversion function irouter_to_vcpu & vcpu_to_irouter will manage
mmio_{read,write}
of IROUTER regs.
Is this OK?
>
> Even better you could introduce two simple functions that convert vmpidr
> to vcpu_id and vice versa, so that we keep all the hacks in one place.
>
>
>> >> 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.
>
> That would be OK, except that vgicv3_get_target_vcpu is actually
> returning vcpu0. We should try to match physical and virtual irq
> delivery as much as possible. So if the physical irq is delivered to
> pcpu1 (or whatever is the pcpu currently running vcpu1), then we should
> try hard to deliver the corresponding virq to vcpu1.
>
> Calling vgic_migrate_irq changes the physical irq affinity to the pcpu
> running the destination vcpu. So step 1 above moves physical irq
> delivery to pcpu1 (assuming that pcpu1 is running vcpu1 for simplicity).
> After the guest kernel sets the irq as GICD_IROUTER_SPI_MODE_ANY, even
> though we could deliver the irq to any vcpu, we should actually delivery
> the irq to the vcpu running on the pcpu that received the interrupt. In
> my example it would still be vcpu1. Therefore we need to return vcpu1
> from vgicv3_get_target_vcpu.
>
> Instead as it is now, we would cause an SGI to be sent from pcpu1 to
> pcpu0 in order to inject the irq to vcpu0 (assuming that vcpu0 is
> running on pcpu0 for simplicity).
>
>
>> 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.
>
> Could you please tell me which one is the relevant chapter of the GICv3
> spec? I couldn't find the statement you mentioned. Maybe my version of
> the spec is outdated.
See 20.0 version section 5.3.4
"When this bit is written from zero to one, the Affinity fields become UNKNOWN."
>
>
>> > - 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?
>
> It is simpler and safer to avoid having any wrong irouter values in the
> first place. So for example we could write cpu0 to irouter in
> vgic_v3_distr_mmio_write if/when we catch an invalid guest write.
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |