On 2017-09-04 10:20, Lokesh Vutla wrote:
>
>
> On Monday 04 September 2017 12:58 PM, Jan Kiszka wrote:
>> On 2017-08-30 21:00, 'Lokesh Vutla' via Jailhouse wrote:
>>> Even though 'struct sgi' already supports for passing affinity levels,
>>> gic_handle_sgir_write() looks only for target fields and triggers sgis
>>> to its respective targets. This will fail in case of armv8 with affinity
>>> routing enabled. So parse all the affinity levels in sgi before sending
>>> sgi.
>>>
>>> Suggested-by: Nikhil Devshatwar <[email protected]>
>>> Signed-off-by: Nikhil Devshatwar <[email protected]>
>>> Signed-off-by: Lokesh Vutla <[email protected]>
>>> ---
>>> hypervisor/arch/arm-common/irqchip.c | 13 +++++++++++--
>>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hypervisor/arch/arm-common/irqchip.c
>>> b/hypervisor/arch/arm-common/irqchip.c
>>> index 2019342..dc892ea 100644
>>> --- a/hypervisor/arch/arm-common/irqchip.c
>>> +++ b/hypervisor/arch/arm-common/irqchip.c
>>> @@ -131,6 +131,7 @@ void gic_handle_sgir_write(struct sgi *sgi, bool
>>> virt_input)
>>> {
>>> struct per_cpu *cpu_data = this_cpu_data();
>>> unsigned long targets = sgi->targets;
>>> + u64 mpidr, clst, sgi_clst, core;
>>> unsigned int cpu;
>>>
>>> if (sgi->routing_mode == 2) {
>>> @@ -139,14 +140,22 @@ void gic_handle_sgir_write(struct sgi *sgi, bool
>>> virt_input)
>>> sgi->targets = (1 << cpu_data->cpu_id);
>>
>> Another case for the assumption "cpu_id == aff0". However, this one
>> seems harmless as we are in routing_mode = 2, and targets are ignored
>> then. We should replace that statement with comment that explains what
>> happens.
>
> Yes, This case will never hit with affinity routing on GICV3 as it
> support only routing mode 0 and 1. Will add a comment.
I've a removal patch here already. Will end up in next later.
>
>>
>>> } else {
>>> sgi->targets = 0;
>>> + sgi_clst = (u64)sgi->aff3 << MPIDR_LEVEL_SHIFT(3) |
>>> + (u64)sgi->aff2 << MPIDR_LEVEL_SHIFT(2) |
>>> + (u64)sgi->aff1 << MPIDR_LEVEL_SHIFT(1);
>>>
>>> for_each_cpu(cpu, cpu_data->cell->cpu_set) {
>>> + mpidr = per_cpu(cpu)->mpidr;
>>> + clst = mpidr & ~0xffUL;
>>> + core = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>>> +
>>> if (sgi->routing_mode == 1) {
>>> /* Route to all (cell) CPUs but the caller. */
>>> if (cpu == cpu_data->cpu_id)
>>> continue;
>>> } else if (virt_input) {
>>> - if (!test_bit(cpu, &targets))
>>> + if (sgi_clst != clst ||
>>> + !test_bit(core, &targets))
>>> continue;
>>
>> OK, forgot that this addresses parts of my concern in the other reply.
>> We should probably rename virt_input to something like
>> "affinity_routing" because that is what happens here now.
>
> ok will rename virt_input to affinity_routing.
>
>>
>> We can still stumble into the else branch below on GICv3 on GICD_SGIR
>> accesses. Just realized - again - that this results in a nop as
>
> Just wondering will there be any chance to hit GICD_SGIR? IIUC, Affinity
> Routing is always enabled on GICV3 and GICD_SGIR is reserved with AR
> enabled. Please correct me if I am wrong. But yes, i agree a comment
> should be added.
The guest is free to use the MMIO interface of the GICD irrespective of
the routing mode. We just need to ensure that GICD_SDIR has no effect in
affinity mode, just like on real HW. That happens implicitly, due to the
empty map, but that deserves a comment as well.
Jan
>
>> gicv2_target_cpu_map is empty on GICv3. And that is the desired behavior
>> on GICD_SGIR access when affinity routing is on. Deserves some comments
>> in the code.
>>
>>> } else {
>>> /*
>>> @@ -161,7 +170,7 @@ void gic_handle_sgir_write(struct sgi *sgi, bool
>>> virt_input)
>>> }
>>>
>>> irqchip_set_pending(per_cpu(cpu), sgi->id);
>>> - sgi->targets |= (1 << cpu);
>>> + sgi->targets |= (1 << core);
>>
>> And here we should explain in a comment that aff[1..3] are taken them
>> the original request.
>
> Sure, will add a comment.
>
> Thanks and regards,
> Lokesh
>
>>
>>> }
>>> }
>>>
>>>
>>
>> Jan
>>
--
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux
--
You received this message because you are subscribed to the Google Groups
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
