|
[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
The only additional info that Xen provides is via the CTRL-AAA menu.
Some details on the GIC are printed but nothing as well organized as
/proc/interrupts.
On Wed, 10 Sep 2014, Vijay Kilari wrote:
> 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 |