On 19 May 2016 at 10:36, Shannon Zhao <zhaoshengl...@huawei.com> wrote:
>
>
> On 2016/5/10 1:29, Peter Maydell wrote:
>> From: Pavel Fedin <p.fe...@samsung.com>
>>
>> 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 <p.fe...@samsung.com>
>> [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 <peter.mayd...@linaro.org>
>> ---
>> 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
