|
[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, 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.
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.
> >> + 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.
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.
> > - 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.
> 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 |