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