|
[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,
Apart from your comments below, I encounter below scenario.
The below function vgic_irq_rank() in vgic.c is not generic. The rank is always
depends on register type it is going to access. So it cannot be
just hardcoded.
struct vgic_irq_rank *(struct vcpu *v, unsigned int irq)
{
return vgic_rank_offset(v, 8, irq, DABT_WORD);
}
This function works ok for GIC v2. But this cannot be used for
GICv3 to access registers like IROUTER which are u64. The rank
calcuation goes wrong and there by takes wrong rank lock.
This function is used in vgic_get_target_vcpu()
and vgic_vcpu_inject_irq() generic code to access
ipriority and itarget.
One option is use to move functions that use this rank locking mechanism
to v2 or v3 driver as a callback.
But if we move this to vgic-v2/v3 driver, then there are some
scenario's where these callbacks are called with already rank
locks held might fail.
Ex: vgic_v2_get_target_vcpu get called from vgic_enable_irqs()
May be we can create two version of callbacks locked & unlocked
and use appropriately?
On Mon, Sep 8, 2014 at 6:37 PM, Vijay Kilari <vijay.kilari@xxxxxxxxx> 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?
>
>>
>> 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 |