|
[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,
Is there way to see irq stats at vcpu level for a domain similar to
/proc/interrupts?
Regards
Vijay
On Tue, Sep 9, 2014 at 4:52 AM, Stefano Stabellini
<stefano.stabellini@xxxxxxxxxxxxx> wrote:
> On Mon, 8 Sep 2014, Vijay Kilari wrote:
>> 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?
>
> Sounds good in principle, but of course I would need to see the code to
> get a better idea.
>
>
>> >
>> > 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."
>
> Yeah, my version is outdated :-/
>
> In any case I would still recommend always keeping a valid value in
> irouter. Maybe we just want to revert to vcpu0, but still it should be a
> valid configuration.
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |