Re: [Qemu-devel] [PATCH v3 20/20] arm/virt: Add support for GICv2 virtualization extensions

[Peter Maydell <peter.maydell@linaro.org>]

On 5 July 2018 at 09:46, Luc Michel <luc.michel@greensocs.com> wrote:
> On 07/05/2018 10:00 AM, Jan Kiszka wrote:
>> On 2018-07-05 08:51, Jan Kiszka wrote:
>>> But now I'm running into troubles with reading back GICD ITARGETSR.
>>> Maybe we are emulating an "early implementation" here?
>>>
>>> [from the related Jailhouse code [1]]
>>> /*
>>>  * Get the CPU interface ID for this cpu. It can be discovered by
>>>  * reading the banked value of the PPI and IPI TARGET registers
>>>  * Patch 2bb3135 in Linux explains why the probe may need to scans the
>>>  * first 8 registers: some early implementation returned 0 for the first
>>>  * ITARGETSR registers.
>>>  * Since those didn't have virtualization extensions, we can safely
>>>  * ignore that case.
>>>  */
>>>
>>> But maybe I'm just off with the configuration, checking...
>>>
>>
>> As suspected, it's a bug in QEMU, this resolves it, and I can run Linux
>> as root cell and a bare metal non-root cell:
>>
>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>> index 7d24348d96..199f953ddb 100644
>> --- a/hw/intc/arm_gic.c
>> +++ b/hw/intc/arm_gic.c
>> @@ -965,7 +965,11 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
>>              if (irq >= 29 && irq <= 31) {
>>                  res = cm;
>>              } else {
>> -                res = GIC_DIST_TARGET(irq);
>> +                if (irq < GIC_INTERNAL) {
>> +                    res = 1 << gic_get_current_cpu(s);

We already have the CPU number of the current cpu in the 'cpu'
local variable, so we don't need to call gic_get_current_cpu() again,
and we have 1 << cpu in "cm".

>> +                } else {
>> +                    res = GIC_DIST_TARGET(irq);
>> +                }
>>              }
>>          }
>>      } else if (offset < 0xf00) {
>>
>> Didn't test Linux as non-root cell (secondary guest) yet, but that
>> should work as well. I'm seeing issues in an error shutdown path, but
>> that might be the same on real hw, needs cross-checking.Hi Jan, thanks for your feedback!
>
> Yes I encountered the same issue with Xen in SMP (see my cover letter).
> Re-reading the GICv2 specs, it's now clear to me that a read to
> ITARGETSR0 to ITARGETSR7 should return "the number of the processor
> performing the read". Reading the message of commit 2bb3135 in Linux, it
> seems that older versions of the GIC exposed this value in IRQs 29, 30,
> 31, hence the
>    if (irq >= 29 && irq <= 31) { res = cm; }
> in the current QEMU implementation.
>
> I should probably add a patch to fix that. I'll have to dig in specs of
> older GIC revisions to see when this behaviour actually appeared.

The "29..31 give the current CPU and others are zero" behaviour is
specific to the 11MPCore:
http://arminfo.emea.arm.com/help/topic/com.arm.doc.ddi0360f/CCHBHJFH.html
The GICv1 spec matches the GICv2 here.

So what we want is probably to refactor this to pull the 11MPCore
code out as the top level special case (since it's weird for
uniprocessor setups too). I'll send a patch in a bit...

thanks
-- PMM