Re: [Qemu-devel] [PATCH v3 08/16] hw/intc/arm_gic: Make ICCICR/GICC_CTLR banked

[Peter Maydell]

On 27 April 2015 at 16:00, Peter Maydell <address@hidden> wrote:
> On 15 April 2015 at 17:02, Greg Bellows <address@hidden> wrote:
>> From: Fabian Aggeler <address@hidden>
>>
>> ICCICR/GICC_CTLR is banked in GICv1 implementations with Security
>> Extensions or in GICv2 in independent from Security Extensions.
>> This makes it possible to enable forwarding of interrupts from
>> the CPU interfaces to the connected processors for Group0 and Group1.
>>
>> We also allow to set additional bits like AckCtl and FIQEn by changing
>> the type from bool to uint32. Since the field does not only store the
>> enable bit anymore and since we are touching the vmstate, we use the
>> opportunity to rename the field to cpu_control.
>>
>> Signed-off-by: Fabian Aggeler <address@hidden>
>> Signed-off-by: Greg Bellows <address@hidden>
>>
>> ---
>>
>> v1 -> v2
>> - Rework gic_set_cpu_control() and gic_get_cpu_control() to close gap on
>>   handling GICv1 wihtout security extensions.
>> - Fix use of incorrect control index in update.
>> ---
>>  hw/intc/arm_gic.c                | 82 
>> +++++++++++++++++++++++++++++++++++++---
>>  hw/intc/arm_gic_common.c         |  5 ++-
>>  hw/intc/arm_gic_kvm.c            |  8 ++--
>>  hw/intc/armv7m_nvic.c            |  2 +-
>>  hw/intc/gic_internal.h           | 14 +++++++
>>  include/hw/intc/arm_gic_common.h |  2 +-
>>  6 files changed, 100 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>> index b9dfde3..b402e00 100644
>> --- a/hw/intc/arm_gic.c
>> +++ b/hw/intc/arm_gic.c
>> @@ -66,7 +66,7 @@ void gic_update(GICState *s)
>>      for (cpu = 0; cpu < NUM_CPU(s); cpu++) {
>>          cm = 1 << cpu;
>>          s->current_pending[cpu] = 1023;
>> -        if (!s->enabled || !s->cpu_enabled[cpu]) {
>> +        if (!s->enabled || !(s->cpu_control[cpu][1] & 1)) {
>
> This breaks no-security-extensions configs at this point
> in the patchseries, because here we always test
> cpu_control[cpu][1] bit 0...
>
>> +void gic_set_cpu_control(GICState *s, int cpu, uint32_t value)
>> +{
>> +    if (!s->security_extn) {
>> +        if (s->revision == 1) {
>> +            s->cpu_control[cpu][1] = value & 0x1;
>> +            DPRINTF("CPU Interface %d %sabled\n", cpu,
>> +                s->cpu_control[cpu][1] ? "En" : "Dis");
>> +        } else {
>> +            /* Write to Secure instance of the register */
>> +            s->cpu_control[cpu][0] = value & GICC_CTLR_S_MASK;
>> +            /* Synchronize EnableGrp1 alias of Non-secure copy */
>> +            s->cpu_control[cpu][1] &= ~GICC_CTLR_NS_EN_GRP1;
>> +            s->cpu_control[cpu][1] |=
>> +                (value & GICC_CTLR_S_EN_GRP1) ? GICC_CTLR_NS_EN_GRP1 : 0;
>
> ...but here we only set bit 0 of [cpu][1] if bit 1 of the value is
> set. So a guest which only sets bit 0 stops working (my testcase
> is an aarch64 linux guest on the virt board).

I think actually both parts of this code are wrong in some
way. Firstly, until we've actually fully implemented support
for grouping we want to be using the cpu_control[cpu][0] bit 0
for testing whether the CPU interface has disabled interrupts.
(When we later add the grouping support we end up not taking
this fast exit unless both group0 and group1 are disabled,
which is why the later patch unbreaks us.) Secondly, for
rev1 no-security-extns GICs we want to read/write cpu_control[cpu][0],
not [1], for the register access functions...

-- PMM