[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 07/16] hw/intc/arm_gic: Make ICDDCR/GICD_CTLR
From: |
Edgar E. Iglesias |
Subject: |
Re: [Qemu-devel] [PATCH v3 07/16] hw/intc/arm_gic: Make ICDDCR/GICD_CTLR banked |
Date: |
Tue, 21 Apr 2015 11:28:46 +1000 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Wed, Apr 15, 2015 at 11:02:13AM -0500, Greg Bellows wrote:
> From: Fabian Aggeler <address@hidden>
>
> ICDDCR/GICD_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
> Distributor to the CPU interfaces for Group0 and Group1.
>
> EnableGroup0 (Bit [1]) in GICv1 is IMPDEF. Since this bit (Enable
> Non-secure) is present in the integrated IC of the Cortex-A9 MPCore,
> which implements the GICv1 profile, we support this bit in GICv1 too.
>
> Signed-off-by: Fabian Aggeler <address@hidden>
> Signed-off-by: Greg Bellows <address@hidden>
>
> ---
> v2 -> v3
> - Added missing return in gic_dist_readb()
> - Fixed typos
>
> v1 -> v2
> - Fix gic_dist_writeb() update of GICD_CTRL to only use bit[0] of the
> EnableGrp1 field not bit[1].
> - Add clarifying comments
> ---
> hw/intc/arm_gic.c | 50
> ++++++++++++++++++++++++++++++++++++----
> hw/intc/arm_gic_common.c | 2 +-
> include/hw/intc/arm_gic_common.h | 7 +++++-
> 3 files changed, 53 insertions(+), 6 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index aa4402e..b9dfde3 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -302,8 +302,26 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr
> offset)
> cpu = gic_get_current_cpu(s);
> cm = 1 << cpu;
> if (offset < 0x100) {
> - if (offset == 0)
> - return s->enabled;
> + if (offset == 0) { /* GICD_CTLR */
> + res = 0;
> + if ((s->revision == 2 && !s->security_extn)
> + || (s->security_extn && !ns_access())) {
> + /* In this case the GICD_CTLR contains both a group0 and
> group1
> + * enable bit, so we create the resuling value by aggregating
> + * the bits from the two enable values.
> + * The group0 enable bit is only visible to secure accesses.
> + * The group1 enable bit (bit[1]) is an alias of bit[0] in
> + * the non-secure copy (enabled_grp[1]).
> + */
> + res = (s->enabled_grp[1] << 1) | s->enabled_grp[0];
> + } else if (s->security_extn && ns_access()) {
> + res = s->enabled_grp[1];
> + } else {
> + /* Neither GICv2 nor Security Extensions present */
> + res = s->enabled;
> + }
> + return res;
> + }
> if (offset == 4)
> /* Interrupt Controller Type Register */
> return ((s->num_irq / 32) - 1)
> @@ -471,8 +489,32 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
> cpu = gic_get_current_cpu(s);
> if (offset < 0x100) {
> if (offset == 0) {
> - s->enabled = (value & 1);
> - DPRINTF("Distribution %sabled\n", s->enabled ? "En" : "Dis");
> + if ((s->revision == 2 && !s->security_extn)
> + || (s->security_extn && !ns_access())) {
> + s->enabled_grp[0] = value & (1U << 0); /* EnableGrp0 */
> + /* For a GICv1 with Security Extn "EnableGrp1" is IMPDEF. */
> + /* We only use the first bit of the enabled_grp vars to
> + * indicate enabled or disabled. In this case we have to
> shift
> + * the incoming value down to the low bit because the group1
> + * enabled bit is bit[1] in the secure/GICv2 GICD_CTLR.
> + */
> + s->enabled_grp[1] = (value >> 1) & 0x1; /* EnableGrp1 */
I would personally prefer just:
s->enabled_grp[0] = value & 1;
s->enabled_grp[1] = value & 2;
> + DPRINTF("Group0 distribution %sabled\n"
> + "Group1 distribution %sabled\n",
> + s->enabled_grp[0] ? "En" : "Dis",
> + s->enabled_grp[1] ? "En" : "Dis");
> + } else if (s->security_extn && ns_access()) {
> + /* If we are non-secure only the group1 enable bit is visible
> + * as bit[0] in the GICD_CTLR.
> + */
> + s->enabled_grp[1] = (value & 0x1);
Can we remove the parenthesis?
> + DPRINTF("Group1 distribution %sabled\n",
> + s->enabled_grp[1] ? "En" : "Dis");
> + } else {
> + /* Neither GICv2 nor Security Extensions present */
> + s->enabled = (value & 0x1);
> + DPRINTF("Distribution %sabled\n", s->enabled ? "En" : "Dis");
> + }
> } else if (offset < 4) {
> /* ignored. */
> } else if (offset >= 0x80) {
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index 28f3b2a..c44050d 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -64,7 +64,7 @@ static const VMStateDescription vmstate_gic = {
> .pre_save = gic_pre_save,
> .post_load = gic_post_load,
> .fields = (VMStateField[]) {
> - VMSTATE_BOOL(enabled, GICState),
> + VMSTATE_UINT8_ARRAY(enabled_grp, GICState, GIC_NR_GROUP),
> VMSTATE_BOOL_ARRAY(cpu_enabled, GICState, GIC_NCPU),
> VMSTATE_STRUCT_ARRAY(irq_state, GICState, GIC_MAXIRQ, 1,
> vmstate_gic_irq_state, gic_irq_state),
> diff --git a/include/hw/intc/arm_gic_common.h
> b/include/hw/intc/arm_gic_common.h
> index b78981e..16e193d 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -30,6 +30,8 @@
> #define GIC_NR_SGIS 16
> /* Maximum number of possible CPU interfaces, determined by GIC architecture
> */
> #define GIC_NCPU 8
> +/* Number of Groups (Group0 [Secure], Group1 [Non-secure]) */
> +#define GIC_NR_GROUP 2
>
> #define MAX_NR_GROUP_PRIO 128
> #define GIC_NR_APRS (MAX_NR_GROUP_PRIO / 32)
> @@ -52,7 +54,10 @@ typedef struct GICState {
>
> qemu_irq parent_irq[GIC_NCPU];
> qemu_irq parent_fiq[GIC_NCPU];
> - bool enabled;
> + union {
> + uint8_t enabled;
> + uint8_t enabled_grp[GIC_NR_GROUP]; /* EnableGrp0 and EnableGrp1 */
> + };
> bool cpu_enabled[GIC_NCPU];
>
> gic_irq_state irq_state[GIC_MAXIRQ];
> --
> 1.8.3.2
>
>
- [Qemu-devel] [PATCH v3 10/16] hw/intc/arm_gic: Implement Non-secure view of RPR, (continued)
- [Qemu-devel] [PATCH v3 10/16] hw/intc/arm_gic: Implement Non-secure view of RPR, Greg Bellows, 2015/04/15
- [Qemu-devel] [PATCH v3 09/16] hw/intc/arm_gic: Make ICCBPR/GICC_BPR banked, Greg Bellows, 2015/04/15
- [Qemu-devel] [PATCH v3 03/16] hw/arm/virt.c: Wire FIQ between CPU <> GIC, Greg Bellows, 2015/04/15
- [Qemu-devel] [PATCH v3 02/16] hw/arm/vexpress.c: Wire FIQ between CPU <> GIC, Greg Bellows, 2015/04/15
- [Qemu-devel] [PATCH v3 04/16] hw/intc/arm_gic: Add Security Extensions property, Greg Bellows, 2015/04/15
- [Qemu-devel] [PATCH v3 06/16] hw/intc/arm_gic: Add Interrupt Group Registers, Greg Bellows, 2015/04/15
- [Qemu-devel] [PATCH v3 01/16] hw/intc/arm_gic: Request FIQ sources, Greg Bellows, 2015/04/15
- [Qemu-devel] [PATCH v3 08/16] hw/intc/arm_gic: Make ICCICR/GICC_CTLR banked, Greg Bellows, 2015/04/15
- [Qemu-devel] [PATCH v3 07/16] hw/intc/arm_gic: Make ICDDCR/GICD_CTLR banked, Greg Bellows, 2015/04/15
- Re: [Qemu-devel] [PATCH v3 07/16] hw/intc/arm_gic: Make ICDDCR/GICD_CTLR banked,
Edgar E. Iglesias <=
- [Qemu-devel] [PATCH v3 11/16] hw/intc/arm_gic: Handle grouping for GICC_HPPIR, Greg Bellows, 2015/04/15
- [Qemu-devel] [PATCH v3 05/16] hw/intc/arm_gic: Add ns_access() function, Greg Bellows, 2015/04/15
- [Qemu-devel] [PATCH v3 14/16] hw/intc/arm_gic: Restrict priority view, Greg Bellows, 2015/04/15
- [Qemu-devel] [PATCH v3 15/16] hw/intc/arm_gic: Break out gic_update() function, Greg Bellows, 2015/04/15
- [Qemu-devel] [PATCH v3 16/16] hw/intc/arm_gic: add gic_update() for grouping, Greg Bellows, 2015/04/15
- [Qemu-devel] [PATCH v3 12/16] hw/intc/arm_gic: Change behavior of EOIR writes, Greg Bellows, 2015/04/15