Re: [Qemu-devel] [PATCH 06/23] hw/intc/arm_gicv3: Add state information

[Peter Maydell]

On 19 May 2016 at 10:36, Shannon Zhao <address@hidden> wrote:
>
>
> On 2016/5/10 1:29, Peter Maydell wrote:
>> From: Pavel Fedin <address@hidden>
>>
>> Add state information to GICv3 object structure and implement
>> arm_gicv3_common_reset().
>>
>> This commit includes accessor functions for the fields which are
>> stored as bitmaps in uint32_t arrays.
>>
>> Signed-off-by: Pavel Fedin <address@hidden>
>> [PMM: significantly overhauled:
>>  * Add missing qom/cpu.h include
>>  * Remove legacy-only state fields (we can add them later if/when we add
>>    legacy emulation)
>>  * Use arrays of uint32_t to store the various distributor bitmaps,
>>    and provide accessor functions for the various set/test/etc operations
>>  * Add various missing register offset #defines
>>  * Accessor macros which combine distributor and redistributor behaviour
>>    removed
>>  * Fields in state structures renamed to match architectural register names
>>  * Corrected the reset value for GICR_IENABLER0 since we don't support
>>    legacy mode
>>  * Added ARM_LINUX_BOOT_IF interface for "we are directly booting a kernel in
>>    non-secure" so that we can fake up the firmware-mandated reconfiguration
>>    only when we need it
>> ]
>> Signed-off-by: Peter Maydell <address@hidden>
>> ---
>>  hw/intc/arm_gicv3_common.c         | 140 ++++++++++++++++++++++++++-
>>  hw/intc/gicv3_internal.h           | 172 +++++++++++++++++++++++++++++++++
>>  include/hw/intc/arm_gicv3_common.h | 189 
>> ++++++++++++++++++++++++++++++++++++-
>>  3 files changed, 496 insertions(+), 5 deletions(-)
>>  create mode 100644 hw/intc/gicv3_internal.h
>>
>> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
>> index b9d3824..9ee4412 100644
>> --- a/hw/intc/arm_gicv3_common.c
>> +++ b/hw/intc/arm_gicv3_common.c
>> @@ -3,8 +3,9 @@
>>   *
>>   * Copyright (c) 2012 Linaro Limited
>>   * Copyright (c) 2015 Huawei.
>> + * Copyright (c) 2015 Samsung Electronics Co., Ltd.
>>   * Written by Peter Maydell
>> - * Extended to 64 cores by Shlomo Pongratz
>> + * Reworked for GICv3 by Shlomo Pongratz and Pavel Fedin
>>   *
>>   * This program is free software; you can redistribute it and/or modify
>>   * it under the terms of the GNU General Public License as published by
>> @@ -22,7 +23,10 @@
>>
>>  #include "qemu/osdep.h"
>>  #include "qapi/error.h"
>> +#include "qom/cpu.h"
>>  #include "hw/intc/arm_gicv3_common.h"
>> +#include "gicv3_internal.h"
>> +#include "hw/arm/linux-boot-if.h"
>>
>>  static void gicv3_pre_save(void *opaque)
>>  {
>> @@ -90,6 +94,7 @@ void gicv3_init_irqs_and_mmio(GICv3State *s, 
>> qemu_irq_handler handler,
>>  static void arm_gicv3_common_realize(DeviceState *dev, Error **errp)
>>  {
>>      GICv3State *s = ARM_GICV3_COMMON(dev);
>> +    int i;
>>
>>      /* revision property is actually reserved and currently used only in 
>> order
>>       * to keep the interface compatible with GICv2 code, avoiding extra
>> @@ -100,11 +105,136 @@ static void arm_gicv3_common_realize(DeviceState 
>> *dev, Error **errp)
>>          error_setg(errp, "unsupported GIC revision %d", s->revision);
>>          return;
>>      }
>> +
>> +    if (s->num_irq > GICV3_MAXIRQ) {
>> +        error_setg(errp,
>> +                   "requested %u interrupt lines exceeds GIC maximum %d",
>> +                   s->num_irq, GICV3_MAXIRQ);
>> +        return;
>> +    }
>> +
> Does it need to check if s->num_irq is less than 32?
>
>> +    s->cpu = g_new0(GICv3CPUState, s->num_cpu);
>> +
> And check if s->num_cpu is greater than 0?

Yes, we should probably use the same set of checks as the gicv2
common realize code does.


>> +    /* For our implementation affinity routing is always enabled */
>> +    if (s->security_extn) {
>> +        s->gicd_ctlr = GICD_CTLR_ARE_S | GICD_CTLR_ARE_NS;
>> +    } else {
>> +        s->gicd_ctlr = GICD_CTLR_DS | GICD_CTLR_ARE;
>> +    }
>> +
> I'm a little confused with the no security support case, and in that
> case, this GICv3 implementation supports only a single Security state,
> right? If so, the SPEC says the DS bit is "Disable Security. This field
> is RAO/WI." So why do you set the DS bit here?

We set the bit here exactly because it is RAO/WI: the bit
is set to 1 here, and we forbid changing it via register writes,
so it always reads as 1. Then all the rest of the GIC code[*] checks
(s->gicd_ctlr & GICD_CTLR_DS), which works whether this is a
no-security GIC or a with-security GIC that the guest has
configured to disable security.

[*] there are one or two config register values that architecturally
need to care about s->security_extn rather than the CTLR_DS bit.

>> +#define GICD_IROUTER         0x6000

> This should be 0x6100 or gicd_irouter[GICV3_MAXSPI] should use
> GIC_MAXIRQ in struct GICv3State. Otherwise gicd_read_irouter() will be
> wrong because s->gicd_irouter[irq] will be off normal upper if irq is
> e.g. GIC_MAXIRQ - 1.

I think we should leave this offset as 0x6000, and have
gicd_read_irouter() and other places that use the array subtract
GIC_INTERNAL from the irq number. This would then be in line
with how we handle gicd_ipriority[] (and with the bitmap regs).

>> +#define GICR_PROPBASER_OUTER_CACHEABILITY_MASK (7ULL << 56)
>> +#define GICR_PROPBASER_ADDR_MASK               (0xfffffffffULL << 12)
>> +#define GICR_PROPBASER_SHAREABILITY_MASK       (3U << 10)
>> +#define GICR_PROPBASER_CACHEABILITY_MASK       (7U << 7)
>> +#define GICR_PROPBASER_IDBITS_MASK             (0x1f)
> Use (0x1f << 0) to keep consistent?

Sure.

thanks
-- PMM