[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 06/16] hw/intc/arm_gic: Add Interrupt Group R
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH v2 06/16] hw/intc/arm_gic: Add Interrupt Group Registers |
Date: |
Tue, 14 Apr 2015 20:13:31 +0100 |
On 30 October 2014 at 22:12, Greg Bellows <address@hidden> wrote:
> From: Fabian Aggeler <address@hidden>
>
> Interrupt Group Registers (previously called Interrupt Security
> Registers) as defined in GICv1 with Security Extensions or GICv2 allow
> to configure interrupts as Secure (Group0) or Non-secure (Group1).
> In GICv2 these registers are implemented independent of the existence of
> Security Extensions.
Worth mentioning in the commit that this patch only
implements the register accessors, not the functionality
that the bits control.
> Signed-off-by: Fabian Aggeler <address@hidden>
>
> ---
>
> v1 -> v2
> - Add clarifying comments to gic_dist_readb/writeb on interrupt group register
> update
> - Swap GIC_SET_GROUP0/1 macro logic. Setting the irq_state.group field for
> group 0 should clear the bit not set it. Similarly, setting the field for
> group 1 should set the bit not clear it.
> ---
> hw/intc/arm_gic.c | 49
> +++++++++++++++++++++++++++++++++++++---
> hw/intc/arm_gic_common.c | 1 +
> hw/intc/gic_internal.h | 4 ++++
> include/hw/intc/arm_gic_common.h | 1 +
> 4 files changed, 52 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index bee71a1..36ac188 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -312,8 +312,27 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr
> offset)
> if (offset < 0x08)
> return 0;
> if (offset >= 0x80) {
> - /* Interrupt Security , RAZ/WI */
> - return 0;
> + /* Interrupt Group Registers
> + *
> + * For GIC with Security Extn and Non-secure access RAZ/WI
> + * For GICv1 without Security Extn RAZ/WI
> + */
> + res = 0;
> + if (!(s->security_extn && ns_access()) &&
> + ((s->revision == 1 && s->security_extn)
> + || s->revision == 2)) {
It would probably be clearer to write this as
if (whatever) {
return 0;
}
if (whatever) {
return 0;
}
logic for registers;
rather than inverting the conditions from their more
natural and readable sense.
Also I suspect we will want some utility functions. One
that springs to mind here would be a gic_has_groups()
which returns (gic is v2 || (gic is v1 && has security extns)).
> + /* Every byte offset holds 8 group status bits */
> + irq = (offset - 0x080) * 8 + GIC_BASE_IRQ;
Better written as 0x80 I think.
> + if (irq >= s->num_irq) {
> + goto bad_reg;
> + }
> + for (i = 0; i < 8; i++) {
> + if (!GIC_TEST_GROUP0(irq + i, cm)) {
> + res |= (1 << i);
> + }
> + }
> + }
> + return res;
> }
> goto bad_reg;
> } else if (offset < 0x200) {
> @@ -457,7 +476,31 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
> } else if (offset < 4) {
> /* ignored. */
> } else if (offset >= 0x80) {
> - /* Interrupt Security Registers, RAZ/WI */
> + /* Interrupt Group Registers
> + *
> + * For GIC with Security Extn and Non-secure access RAZ/WI
> + * For GICv1 without Security Extn RAZ/WI
> + */
> + if (!(s->security_extn && ns_access()) &&
> + ((s->revision == 1 && s->security_extn)
> + || s->revision == 2)) {
> + /* Every byte offset holds 8 group status bits */
> + irq = (offset - 0x080) * 8 + GIC_BASE_IRQ;
> + if (irq >= s->num_irq) {
> + goto bad_reg;
> + }
> + for (i = 0; i < 8; i++) {
> + /* Group bits are banked for private interrupts
> (internal)*/
Missing trailing space before */.
> + int cm = (irq < GIC_INTERNAL) ? (1 << cpu) :
> ALL_CPU_MASK;
> + if (value & (1 << i)) {
> + /* Group1 (Non-secure) */
> + GIC_SET_GROUP1(irq + i, cm);
> + } else {
> + /* Group0 (Secure) */
> + GIC_SET_GROUP0(irq + i, cm);
> + }
> + }
> + }
> } else {
> goto bad_reg;
> }
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index e35049d..28f3b2a 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -52,6 +52,7 @@ static const VMStateDescription vmstate_gic_irq_state = {
> VMSTATE_UINT8(level, gic_irq_state),
> VMSTATE_BOOL(model, gic_irq_state),
> VMSTATE_BOOL(edge_trigger, gic_irq_state),
> + VMSTATE_UINT8(group, gic_irq_state),
We want to bump the vmstate version at some point in this
series, but if we're making several changes to the structure
I guess we can do that last.
> VMSTATE_END_OF_LIST()
> }
> };
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index e87ef36..f01955a 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -50,6 +50,10 @@
> s->priority1[irq][cpu] : \
> s->priority2[(irq) - GIC_INTERNAL])
> #define GIC_TARGET(irq) s->irq_target[irq]
> +#define GIC_SET_GROUP0(irq, cm) (s->irq_state[irq].group &= ~(cm))
> +#define GIC_SET_GROUP1(irq, cm) (s->irq_state[irq].group |= (cm))
> +#define GIC_TEST_GROUP0(irq, cm) ((s->irq_state[irq].group & (cm)) == 0)
This is confusing (especially the way the TEST macro
is doing an == 0 comparison). I think we should just have
GIC_SET_GROUP/GIC_CLEAR_GROUP/GIC_TEST_GROUP macros,
and not try to include the idea of "bit set means
group 1" in the macro names.
> +
>
> /* The special cases for the revision property: */
> #define REV_11MPCORE 0
> diff --git a/include/hw/intc/arm_gic_common.h
> b/include/hw/intc/arm_gic_common.h
> index 7825134..b78981e 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -42,6 +42,7 @@ typedef struct gic_irq_state {
> uint8_t level;
> bool model; /* 0 = N:N, 1 = 1:N */
> bool edge_trigger; /* true: edge-triggered, false: level-triggered */
> + uint8_t group;
> } gic_irq_state;
> typedef struct GICState {
> --
> 1.8.3.2
>
-- PMM
- Re: [Qemu-devel] [PATCH v2 06/16] hw/intc/arm_gic: Add Interrupt Group Registers,
Peter Maydell <=