[Qemu-devel] [PATCH 00/15] target-arm: Add GICv1/SecExt and GICv2/Grouping

[Qemu-devel] [PATCH 00/15] target-arm: Add GICv1/SecExt and GICv2/Grouping

[Fabian Aggeler]

Hi,

this series adds GICv1 Security Extensions (secure/non-secure interrupts)
and interrupt grouping of the GICv2 specification. The patches use the 
terminology introduced by GICv2 (Group0 instead of secure). 
The series first adds FIQ lines from the GIC to the CPUs and then adds 
the Security Extensions. As the Security Extensions for CPUs are not 
upstream yet a ns_access() stub is used, which can be replaced in a 
follow-up with the actual implementation to determine the security state
of a read/write access (needed for banking).

Any feedback is highly appreciated!

Thanks,
Fabian

Fabian Aggeler (15):
  hw/intc/arm_gic: Request FIQ sources
  hw/arm/vexpress.c: Wire FIQ between CPU <> GIC
  hw/intc/arm_gic: Add Security Extensions property
  hw/intc/arm_gic: Add ns_access() function
  hw/intc/arm_gic: Add Interrupt Group Registers
  hw/intc/arm_gic: Make ICDDCR/GICD_CTLR banked
  hw/intc/arm_gic: Make ICCICR/GICC_CTLR banked
  hw/intc/arm_gic: Make ICCBPR/GICC_BPR banked
  hw/intc/arm_gic: Implement Non-secure view of RPR
  hw/intc/arm_gic: Handle grouping for GICC_HPPIR
  hw/intc/arm_gic: Change behavior of EOIR writes
  hw/intc/arm_gic: Change behavior of IAR writes
  hw/intc/arm_gic: Restrict priority view
  hw/intc/arm_gic: Break out gic_update() function
  hw/intc/arm_gic: add gic_update() for grouping

 hw/arm/vexpress.c                |   2 +
 hw/intc/arm_gic.c                | 422 ++++++++++++++++++++++++++++++++++++---
 hw/intc/arm_gic_common.c         |   9 +-
 hw/intc/arm_gic_kvm.c            |   8 +-
 hw/intc/armv7m_nvic.c            |   2 +-
 hw/intc/gic_internal.h           |  25 +++
 include/hw/intc/arm_gic_common.h |  20 +-
 7 files changed, 447 insertions(+), 41 deletions(-)

-- 
1.8.3.2

[Qemu-devel] [PATCH 01/15] hw/intc/arm_gic: Request FIQ sources

[Fabian Aggeler]

Preparing for FIQ lines from GIC to CPUs, which is needed for GIC
Security Extensions.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
---
 hw/intc/arm_gic.c                | 3 +++
 include/hw/intc/arm_gic_common.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 1532ef9..b27bd0e 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -786,6 +786,9 @@ void gic_init_irqs_and_distributor(GICState *s, int num_irq)
     for (i = 0; i < NUM_CPU(s); i++) {
         sysbus_init_irq(sbd, &s->parent_irq[i]);
     }
+    for (i = 0; i < NUM_CPU(s); i++) {
+        sysbus_init_irq(sbd, &s->parent_fiq[i]);
+    }
     memory_region_init_io(&s->iomem, OBJECT(s), &gic_dist_ops, s,
                           "gic_dist", 0x1000);
 }
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index f6887ed..01c6f24 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -50,6 +50,7 @@ typedef struct GICState {
     /*< public >*/
 
     qemu_irq parent_irq[GIC_NCPU];
+    qemu_irq parent_fiq[GIC_NCPU];
     bool enabled;
     bool cpu_enabled[GIC_NCPU];
 
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 01/15] hw/intc/arm_gic: Request FIQ sources

[Sergey Fedorov]

On 22.08.2014 14:29, Fabian Aggeler wrote:
> Preparing for FIQ lines from GIC to CPUs, which is needed for GIC
> Security Extensions.
>
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> ---
>  hw/intc/arm_gic.c                | 3 +++
>  include/hw/intc/arm_gic_common.h | 1 +
>  2 files changed, 4 insertions(+)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 1532ef9..b27bd0e 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -786,6 +786,9 @@ void gic_init_irqs_and_distributor(GICState *s, int num_irq)
>      for (i = 0; i < NUM_CPU(s); i++) {
>          sysbus_init_irq(sbd, &s->parent_irq[i]);
>      }
> +    for (i = 0; i < NUM_CPU(s); i++) {
> +        sysbus_init_irq(sbd, &s->parent_fiq[i]);
> +    }

Hi Fabian,

I would suggest to provide a way to get a sysbus IRQ/FIQ number for each
processor, e.g. a dedicated macro. Maybe it could be easier to
accomplish this by initializing IRQ and FIQ interleaved or by always
initializing GIC_NCPU IRQs/FIQs.

Regards,
Sergey

>      memory_region_init_io(&s->iomem, OBJECT(s), &gic_dist_ops, s,
>                            "gic_dist", 0x1000);
>  }
> diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
> index f6887ed..01c6f24 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -50,6 +50,7 @@ typedef struct GICState {
>      /*< public >*/
>  
>      qemu_irq parent_irq[GIC_NCPU];
> +    qemu_irq parent_fiq[GIC_NCPU];
>      bool enabled;
>      bool cpu_enabled[GIC_NCPU];
>  

Re: [Qemu-devel] [PATCH 01/15] hw/intc/arm_gic: Request FIQ sources

[Peter Maydell]

On 25 August 2014 10:16, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> On 22.08.2014 14:29, Fabian Aggeler wrote:
>> Preparing for FIQ lines from GIC to CPUs, which is needed for GIC
>> Security Extensions.
>>
>> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
>> ---
>>  hw/intc/arm_gic.c                | 3 +++
>>  include/hw/intc/arm_gic_common.h | 1 +
>>  2 files changed, 4 insertions(+)
>>
>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>> index 1532ef9..b27bd0e 100644
>> --- a/hw/intc/arm_gic.c
>> +++ b/hw/intc/arm_gic.c
>> @@ -786,6 +786,9 @@ void gic_init_irqs_and_distributor(GICState *s, int num_irq)
>>      for (i = 0; i < NUM_CPU(s); i++) {
>>          sysbus_init_irq(sbd, &s->parent_irq[i]);
>>      }
>> +    for (i = 0; i < NUM_CPU(s); i++) {
>> +        sysbus_init_irq(sbd, &s->parent_fiq[i]);
>> +    }
>
> Hi Fabian,
>
> I would suggest to provide a way to get a sysbus IRQ/FIQ number for each
> processor, e.g. a dedicated macro. Maybe it could be easier to
> accomplish this by initializing IRQ and FIQ interleaved or by always
> initializing GIC_NCPU IRQs/FIQs.

Using named GPIO registers is the way to go here,
or at least it will be once Peter C's patchset to make
sysbus IRQs just be legacy syntax for GPIOs goes in.

-- PMM

[Qemu-devel] [PATCH 02/15] hw/arm/vexpress.c: Wire FIQ between CPU <> GIC

[Fabian Aggeler]

Connect FIQ output of the GIC CPU interfaces to the CPUs.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
---
 hw/arm/vexpress.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index a88732c..bafe8d2 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -229,6 +229,8 @@ static void init_cpus(const char *cpu_model, const char *privdev,
         DeviceState *cpudev = DEVICE(qemu_get_cpu(n));
 
         sysbus_connect_irq(busdev, n, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
+        sysbus_connect_irq(busdev, n+smp_cpus,
+                                      qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
     }
 }
 
-- 
1.8.3.2

[Qemu-devel] [PATCH 03/15] hw/intc/arm_gic: Add Security Extensions property

[Fabian Aggeler]

The existing implementation does not support Security Extensions mentioned
in the GICv1 and GICv2 architecture specification. Security Extensions are
not available on all GICs. This property makes it possible to enable Security Extensions.

It also makes GICD_TYPER/ICDICTR.SecurityExtn RAO for GICs which implement
Security Extensions.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
---
 hw/intc/arm_gic.c                | 5 ++++-
 hw/intc/arm_gic_common.c         | 1 +
 include/hw/intc/arm_gic_common.h | 1 +
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index b27bd0e..75b5121 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -297,7 +297,10 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
         if (offset == 0)
             return s->enabled;
         if (offset == 4)
-            return ((s->num_irq / 32) - 1) | ((NUM_CPU(s) - 1) << 5);
+            /* Interrupt Controller Type Register */
+            return ((s->num_irq / 32) - 1)
+                    | ((NUM_CPU(s) - 1) << 5)
+                    | (s->security_extn << 10);
         if (offset < 0x08)
             return 0;
         if (offset >= 0x80) {
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 6d884ec..302a056 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -149,6 +149,7 @@ static Property arm_gic_common_properties[] = {
      * (Internally, 0xffffffff also indicates "not a GIC but an NVIC".)
      */
     DEFINE_PROP_UINT32("revision", GICState, revision, 1),
+    DEFINE_PROP_UINT8("security-extn", GICState, security_extn, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index 01c6f24..4e25017 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -105,6 +105,7 @@ typedef struct GICState {
     MemoryRegion cpuiomem[GIC_NCPU + 1]; /* CPU interfaces */
     uint32_t num_irq;
     uint32_t revision;
+    uint8_t security_extn;
     int dev_fd; /* kvm device fd if backed by kvm vgic support */
 } GICState;
 
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 03/15] hw/intc/arm_gic: Add Security Extensions property

[Sergey Fedorov]

On 22.08.2014 14:29, Fabian Aggeler wrote:
> The existing implementation does not support Security Extensions mentioned
> in the GICv1 and GICv2 architecture specification. Security Extensions are
> not available on all GICs. This property makes it possible to enable Security Extensions.
>
> It also makes GICD_TYPER/ICDICTR.SecurityExtn RAO for GICs which implement
> Security Extensions.
>
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> ---
>  hw/intc/arm_gic.c                | 5 ++++-
>  hw/intc/arm_gic_common.c         | 1 +
>  include/hw/intc/arm_gic_common.h | 1 +
>  3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index b27bd0e..75b5121 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -297,7 +297,10 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
>          if (offset == 0)
>              return s->enabled;
>          if (offset == 4)
> -            return ((s->num_irq / 32) - 1) | ((NUM_CPU(s) - 1) << 5);
> +            /* Interrupt Controller Type Register */
> +            return ((s->num_irq / 32) - 1)
> +                    | ((NUM_CPU(s) - 1) << 5)
> +                    | (s->security_extn << 10);
>          if (offset < 0x08)
>              return 0;
>          if (offset >= 0x80) {
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index 6d884ec..302a056 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -149,6 +149,7 @@ static Property arm_gic_common_properties[] = {
>       * (Internally, 0xffffffff also indicates "not a GIC but an NVIC".)
>       */
>      DEFINE_PROP_UINT32("revision", GICState, revision, 1),
> +    DEFINE_PROP_UINT8("security-extn", GICState, security_extn, 0),

Why don't use bool type and DEFINE_PROP_BOOL for this field?

Regards,
Sergey.

>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
> index 01c6f24..4e25017 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -105,6 +105,7 @@ typedef struct GICState {
>      MemoryRegion cpuiomem[GIC_NCPU + 1]; /* CPU interfaces */
>      uint32_t num_irq;
>      uint32_t revision;
> +    uint8_t security_extn;
>      int dev_fd; /* kvm device fd if backed by kvm vgic support */
>  } GICState;
>  

Re: [Qemu-devel] [PATCH 03/15] hw/intc/arm_gic: Add Security Extensions property

[Aggeler Fabian]

On 25 Aug 2014, at 11:20, Sergey Fedorov <serge.fdrv@gmail.com> wrote:

> On 22.08.2014 14:29, Fabian Aggeler wrote:
>> The existing implementation does not support Security Extensions mentioned
>> in the GICv1 and GICv2 architecture specification. Security Extensions are
>> not available on all GICs. This property makes it possible to enable Security Extensions.
>> 
>> It also makes GICD_TYPER/ICDICTR.SecurityExtn RAO for GICs which implement
>> Security Extensions.
>> 
>> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
>> ---
>> hw/intc/arm_gic.c                | 5 ++++-
>> hw/intc/arm_gic_common.c         | 1 +
>> include/hw/intc/arm_gic_common.h | 1 +
>> 3 files changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>> index b27bd0e..75b5121 100644
>> --- a/hw/intc/arm_gic.c
>> +++ b/hw/intc/arm_gic.c
>> @@ -297,7 +297,10 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
>>         if (offset == 0)
>>             return s->enabled;
>>         if (offset == 4)
>> -            return ((s->num_irq / 32) - 1) | ((NUM_CPU(s) - 1) << 5);
>> +            /* Interrupt Controller Type Register */
>> +            return ((s->num_irq / 32) - 1)
>> +                    | ((NUM_CPU(s) - 1) << 5)
>> +                    | (s->security_extn << 10);
>>         if (offset < 0x08)
>>             return 0;
>>         if (offset >= 0x80) {
>> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
>> index 6d884ec..302a056 100644
>> --- a/hw/intc/arm_gic_common.c
>> +++ b/hw/intc/arm_gic_common.c
>> @@ -149,6 +149,7 @@ static Property arm_gic_common_properties[] = {
>>      * (Internally, 0xffffffff also indicates "not a GIC but an NVIC".)
>>      */
>>     DEFINE_PROP_UINT32("revision", GICState, revision, 1),
>> +    DEFINE_PROP_UINT8("security-extn", GICState, security_extn, 0),
> 
> Why don't use bool type and DEFINE_PROP_BOOL for this field?
> 
> Regards,
> Sergey.

I used an uint8 to be able to shift the security_extn field in the return value of the
Interrupt Controller Type Register (see above). 

@Edgar: how did you add Virtualization Extensions to the GIC implementation? 
Does it make sense to combine the extensions into one QOM property?

Best,
Fabian

> 
>>     DEFINE_PROP_END_OF_LIST(),
>> };
>> 
>> diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
>> index 01c6f24..4e25017 100644
>> --- a/include/hw/intc/arm_gic_common.h
>> +++ b/include/hw/intc/arm_gic_common.h
>> @@ -105,6 +105,7 @@ typedef struct GICState {
>>     MemoryRegion cpuiomem[GIC_NCPU + 1]; /* CPU interfaces */
>>     uint32_t num_irq;
>>     uint32_t revision;
>> +    uint8_t security_extn;
>>     int dev_fd; /* kvm device fd if backed by kvm vgic support */
>> } GICState;

Re: [Qemu-devel] [PATCH 03/15] hw/intc/arm_gic: Add Security Extensions property

[Sergey Fedorov]

On 25.08.2014 13:39, Aggeler Fabian wrote:
> On 25 Aug 2014, at 11:20, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>
>> On 22.08.2014 14:29, Fabian Aggeler wrote:
>>> The existing implementation does not support Security Extensions mentioned
>>> in the GICv1 and GICv2 architecture specification. Security Extensions are
>>> not available on all GICs. This property makes it possible to enable Security Extensions.
>>>
>>> It also makes GICD_TYPER/ICDICTR.SecurityExtn RAO for GICs which implement
>>> Security Extensions.
>>>
>>> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
>>> ---
>>> hw/intc/arm_gic.c                | 5 ++++-
>>> hw/intc/arm_gic_common.c         | 1 +
>>> include/hw/intc/arm_gic_common.h | 1 +
>>> 3 files changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>>> index b27bd0e..75b5121 100644
>>> --- a/hw/intc/arm_gic.c
>>> +++ b/hw/intc/arm_gic.c
>>> @@ -297,7 +297,10 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
>>>         if (offset == 0)
>>>             return s->enabled;
>>>         if (offset == 4)
>>> -            return ((s->num_irq / 32) - 1) | ((NUM_CPU(s) - 1) << 5);
>>> +            /* Interrupt Controller Type Register */
>>> +            return ((s->num_irq / 32) - 1)
>>> +                    | ((NUM_CPU(s) - 1) << 5)
>>> +                    | (s->security_extn << 10);
>>>         if (offset < 0x08)
>>>             return 0;
>>>         if (offset >= 0x80) {
>>> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
>>> index 6d884ec..302a056 100644
>>> --- a/hw/intc/arm_gic_common.c
>>> +++ b/hw/intc/arm_gic_common.c
>>> @@ -149,6 +149,7 @@ static Property arm_gic_common_properties[] = {
>>>      * (Internally, 0xffffffff also indicates "not a GIC but an NVIC".)
>>>      */
>>>     DEFINE_PROP_UINT32("revision", GICState, revision, 1),
>>> +    DEFINE_PROP_UINT8("security-extn", GICState, security_extn, 0),
>> Why don't use bool type and DEFINE_PROP_BOOL for this field?
>>
>> Regards,
>> Sergey.
> I used an uint8 to be able to shift the security_extn field in the return value of the
> Interrupt Controller Type Register (see above). 

But uint8 type wouldn't be enough to shift 10 bits left, isn't it?
AFAIU, integer promotion should convert bool to int in this case.

Best,
Sergey

>
> @Edgar: how did you add Virtualization Extensions to the GIC implementation? 
> Does it make sense to combine the extensions into one QOM property?
>
> Best,
> Fabian
>
>>>     DEFINE_PROP_END_OF_LIST(),
>>> };
>>>
>>> diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
>>> index 01c6f24..4e25017 100644
>>> --- a/include/hw/intc/arm_gic_common.h
>>> +++ b/include/hw/intc/arm_gic_common.h
>>> @@ -105,6 +105,7 @@ typedef struct GICState {
>>>     MemoryRegion cpuiomem[GIC_NCPU + 1]; /* CPU interfaces */
>>>     uint32_t num_irq;
>>>     uint32_t revision;
>>> +    uint8_t security_extn;
>>>     int dev_fd; /* kvm device fd if backed by kvm vgic support */
>>> } GICState;

[Qemu-devel] [PATCH 04/15] hw/intc/arm_gic: Add ns_access() function

[Fabian Aggeler]

Security Extensions for GICv1 and GICv2 use register banking
to provide transparent access to seperate Secure and Non-secure
copies of GIC configuration registers. This function will later
be replaced by code determining the security state of a read/write
access to a register.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
---
 hw/intc/arm_gic.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 75b5121..9b83af0 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -45,6 +45,13 @@ static inline int gic_get_current_cpu(GICState *s)
     return 0;
 }
 
+/* Security state of a read / write access */
+static inline bool ns_access(void)
+{
+    /* TODO: use actual security state */
+    return true;
+}
+
 /* TODO: Many places that call this routine could be optimized.  */
 /* Update interrupt status after enabled or pending bits have been changed.  */
 void gic_update(GICState *s)
-- 
1.8.3.2

[Qemu-devel] [PATCH 05/15] hw/intc/arm_gic: Add Interrupt Group Registers

[Fabian Aggeler]

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.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
---
 hw/intc/arm_gic.c                | 47 +++++++++++++++++++++++++++++++++++++---
 hw/intc/arm_gic_common.c         |  1 +
 hw/intc/gic_internal.h           |  4 ++++
 include/hw/intc/arm_gic_common.h |  1 +
 4 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 9b83af0..a972942 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -311,8 +311,26 @@ 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)) {
+                irq = (offset - 0x080) * 8 + GIC_BASE_IRQ;
+                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) {
@@ -456,7 +474,30 @@ 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)) {
+                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)*/
+                    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 302a056..f74175d 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),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index 48a58d7..b0430ff 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)
+
 
 /* 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 4e25017..a61e52e 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

[Qemu-devel] [PATCH 06/15] hw/intc/arm_gic: Make ICDDCR/GICD_CTLR banked

[Fabian Aggeler]

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 <aggelerf@ethz.ch>
---
 hw/intc/arm_gic.c                | 34 ++++++++++++++++++++++++++++++----
 hw/intc/arm_gic_common.c         |  2 +-
 include/hw/intc/arm_gic_common.h |  7 ++++++-
 3 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index a972942..c78b301 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -301,8 +301,18 @@ 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) {
+            res = 0;
+            if ((s->revision == 2 && !s->security_extn)
+                    || (s->security_extn && !ns_access())) {
+                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;
+            }
+        }
         if (offset == 4)
             /* Interrupt Controller Type Register */
             return ((s->num_irq / 32) - 1)
@@ -469,8 +479,24 @@ 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. */
+                s->enabled_grp[1] = value & (1U << 1); /* EnableGrp1 */
+                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()) {
+                s->enabled_grp[1] = (value & 1U);
+                DPRINTF("Group1 distribution %sabled\n",
+                        s->enabled_grp[1] ? "En" : "Dis");
+            } else {
+                /* Neither GICv2 nor Security Extensions present */
+                s->enabled = (value & 1U);
+                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 f74175d..7652754 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 a61e52e..a39b066 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

Re: [Qemu-devel] [PATCH 06/15] hw/intc/arm_gic: Make ICDDCR/GICD_CTLR banked

[Sergey Fedorov]

On 22.08.2014 14:29, Fabian Aggeler wrote:
> 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 <aggelerf@ethz.ch>
> ---
>  hw/intc/arm_gic.c                | 34 ++++++++++++++++++++++++++++++----
>  hw/intc/arm_gic_common.c         |  2 +-
>  include/hw/intc/arm_gic_common.h |  7 ++++++-
>  3 files changed, 37 insertions(+), 6 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index a972942..c78b301 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -301,8 +301,18 @@ 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) {
> +            res = 0;
> +            if ((s->revision == 2 && !s->security_extn)
> +                    || (s->security_extn && !ns_access())) {
> +                res = (s->enabled_grp[1] << 1) | s->enabled_grp[0];

Seems that (s->enabled_grp[1] << 1) can give wrong value, namely 4
instead of 2. See below.

> +            } else if (s->security_extn && ns_access()) {
> +                res = s->enabled_grp[1];
> +            } else {
> +                /* Neither GICv2 nor Security Extensions present */
> +                res = s->enabled;
> +            }
> +        }
>          if (offset == 4)
>              /* Interrupt Controller Type Register */
>              return ((s->num_irq / 32) - 1)
> @@ -469,8 +479,24 @@ 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. */
> +                s->enabled_grp[1] = value & (1U << 1); /* EnableGrp1 */

Here s->enabled_grp[1] can get value of 2. That can give wrong result as
noted above.

Best,
Sergey

> +                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()) {
> +                s->enabled_grp[1] = (value & 1U);
> +                DPRINTF("Group1 distribution %sabled\n",
> +                        s->enabled_grp[1] ? "En" : "Dis");
> +            } else {
> +                /* Neither GICv2 nor Security Extensions present */
> +                s->enabled = (value & 1U);
> +                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 f74175d..7652754 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 a61e52e..a39b066 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];

Re: [Qemu-devel] [PATCH 06/15] hw/intc/arm_gic: Make ICDDCR/GICD_CTLR banked

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 5938 bytes --]

On 26 August 2014 06:47, Sergey Fedorov <serge.fdrv@gmail.com> wrote:

> On 22.08.2014 14:29, Fabian Aggeler wrote:
> > 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 <aggelerf@ethz.ch>
> > ---
> >  hw/intc/arm_gic.c                | 34 ++++++++++++++++++++++++++++++----
> >  hw/intc/arm_gic_common.c         |  2 +-
> >  include/hw/intc/arm_gic_common.h |  7 ++++++-
> >  3 files changed, 37 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> > index a972942..c78b301 100644
> > --- a/hw/intc/arm_gic.c
> > +++ b/hw/intc/arm_gic.c
> > @@ -301,8 +301,18 @@ 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) {
> > +            res = 0;
> > +            if ((s->revision == 2 && !s->security_extn)
> > +                    || (s->security_extn && !ns_access())) {
> > +                res = (s->enabled_grp[1] << 1) | s->enabled_grp[0];
>
> Seems that (s->enabled_grp[1] << 1) can give wrong value, namely 4
> instead of 2. See below.
>

I agree, this needs to be fixed.  I'll fix in the next version.


>
> > +            } else if (s->security_extn && ns_access()) {
> > +                res = s->enabled_grp[1];
> > +            } else {
> > +                /* Neither GICv2 nor Security Extensions present */
> > +                res = s->enabled;
> > +            }
> > +        }
> >          if (offset == 4)
> >              /* Interrupt Controller Type Register */
> >              return ((s->num_irq / 32) - 1)
> > @@ -469,8 +479,24 @@ 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. */
> > +                s->enabled_grp[1] = value & (1U << 1); /* EnableGrp1 */
>
> Here s->enabled_grp[1] can get value of 2. That can give wrong result as
> noted above.
>
>
I don't believe this is properly handling banking.  There should be two
copies of the mask that get decided upon based on the condition criteria,
but their only appears to be a single bank.


> Best,
> Sergey
>
> > +                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()) {
> > +                s->enabled_grp[1] = (value & 1U);
> > +                DPRINTF("Group1 distribution %sabled\n",
> > +                        s->enabled_grp[1] ? "En" : "Dis");
> > +            } else {
> > +                /* Neither GICv2 nor Security Extensions present */
> > +                s->enabled = (value & 1U);
> > +                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 f74175d..7652754 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 a61e52e..a39b066 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
> */
>

I'm confused why this is an array based on the group number?  The register
is banked so I can see why we need two copies, but this appears to be
separate registers for the different groups.  I think it would be cleaner
to have distinct s/ns copies where the content/definition depends on the
condition.

> +    };
> >      bool cpu_enabled[GIC_NCPU];
> >
> >      gic_irq_state irq_state[GIC_MAXIRQ];
>
>

[-- Attachment #2: Type: text/html, Size: 8263 bytes --]

[Qemu-devel] [PATCH 07/15] hw/intc/arm_gic: Make ICCICR/GICC_CTLR banked

[Fabian Aggeler]

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 <aggelerf@ethz.ch>
---
 hw/intc/arm_gic.c                | 54 ++++++++++++++++++++++++++++++++++++----
 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, 72 insertions(+), 13 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index c78b301..7f7fac3 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][0] & 1)) {
             qemu_irq_lower(s->parent_irq[cpu]);
             return;
         }
@@ -239,6 +239,52 @@ void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val)
     }
 }
 
+uint32_t gic_get_cpu_control(GICState *s, int cpu)
+{
+    if ((s->revision >= 2 && !s->security_extn)
+            || (s->security_extn && !ns_access())) {
+        return s->cpu_control[cpu][0];
+    } else if (s->security_extn && ns_access()) {
+        return s->cpu_control[cpu][1];
+    } else {
+        return s->cpu_control[cpu][0];
+    }
+}
+
+void gic_set_cpu_control(GICState *s, int cpu, uint32_t value)
+{
+    /* CPU Interface Control is banked for GICv2 and GICv1 with Security Extn */
+    if ((s->revision >= 2 && !s->security_extn)
+            || (s->security_extn && !ns_access())) {
+        /* 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;
+
+        DPRINTF("CPU Interface %d: Group0 Interrupts %sabled, "
+                "Group1 Interrupts %sabled\n", cpu,
+                (s->cpu_control[cpu][0] & GICC_CTLR_S_EN_GRP0) ? "En" : "Dis",
+                (s->cpu_control[cpu][0] & GICC_CTLR_S_EN_GRP1) ? "En" : "Dis");
+    } else if (s->security_extn && ns_access()) {
+        /* Write to Non-secure instance of the register */
+        s->cpu_control[cpu][1] = (value & GICC_CTLR_NS_MASK);
+        /* Synchronize EnableGrp1 alias of Secure copy */
+        s->cpu_control[cpu][0] &= ~GICC_CTLR_S_EN_GRP1;
+        s->cpu_control[cpu][0] |= (value & GICC_CTLR_NS_EN_GRP1) ?
+                GICC_CTLR_S_EN_GRP1 : 0;
+
+        DPRINTF("CPU Interface %d: Group1 Interrupts %sabled\n", cpu,
+                (s->cpu_control[cpu][1] & GICC_CTLR_NS_EN_GRP1) ? "En" : "Dis");
+    } else {
+        s->cpu_control[cpu][0] = (value & 1);
+
+        DPRINTF("CPU Interface %d %sabled\n", cpu,
+                s->cpu_control[cpu][0] ? "En" : "Dis");
+    }
+}
+
 void gic_complete_irq(GICState *s, int cpu, int irq)
 {
     int update = 0;
@@ -742,7 +788,7 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
 {
     switch (offset) {
     case 0x00: /* Control */
-        return s->cpu_enabled[cpu];
+        return gic_get_cpu_control(s, cpu);
     case 0x04: /* Priority mask */
         return s->priority_mask[cpu];
     case 0x08: /* Binary Point */
@@ -768,9 +814,7 @@ static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value)
 {
     switch (offset) {
     case 0x00: /* Control */
-        s->cpu_enabled[cpu] = (value & 1);
-        DPRINTF("CPU %d %sabled\n", cpu, s->cpu_enabled[cpu] ? "En" : "Dis");
-        break;
+        return gic_set_cpu_control(s, cpu, value);
     case 0x04: /* Priority mask */
         s->priority_mask[cpu] = (value & 0xff);
         break;
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 7652754..c873f7b 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -65,7 +65,7 @@ static const VMStateDescription vmstate_gic = {
     .post_load = gic_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT8_ARRAY(enabled_grp, GICState, GIC_NR_GROUP),
-        VMSTATE_BOOL_ARRAY(cpu_enabled, GICState, GIC_NCPU),
+        VMSTATE_UINT32_2DARRAY(cpu_control, GICState, GIC_NCPU, GIC_NR_GROUP),
         VMSTATE_STRUCT_ARRAY(irq_state, GICState, GIC_MAXIRQ, 1,
                              vmstate_gic_irq_state, gic_irq_state),
         VMSTATE_UINT8_ARRAY(irq_target, GICState, GIC_MAXIRQ),
@@ -127,7 +127,8 @@ static void arm_gic_common_reset(DeviceState *dev)
         s->current_pending[i] = 1023;
         s->running_irq[i] = 1023;
         s->running_priority[i] = 0x100;
-        s->cpu_enabled[i] = false;
+        s->cpu_control[i][0] = false;
+        s->cpu_control[i][1] = false;
     }
     for (i = 0; i < 16; i++) {
         GIC_SET_ENABLED(i, ALL_CPU_MASK);
diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index 5038885..9a6a2bb 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -379,8 +379,8 @@ static void kvm_arm_gic_put(GICState *s)
      */
 
     for (cpu = 0; cpu < s->num_cpu; cpu++) {
-        /* s->cpu_enabled[cpu] -> GICC_CTLR */
-        reg = s->cpu_enabled[cpu];
+        /* s->cpu_enabled[cpu][0] -> GICC_CTLR */
+        reg = s->cpu_control[cpu];
         kvm_gicc_access(s, 0x00, cpu, &reg, true);
 
         /* s->priority_mask[cpu] -> GICC_PMR */
@@ -478,9 +478,9 @@ static void kvm_arm_gic_get(GICState *s)
      */
 
     for (cpu = 0; cpu < s->num_cpu; cpu++) {
-        /* GICC_CTLR -> s->cpu_enabled[cpu] */
+        /* GICC_CTLR -> s->cpu_control[cpu][0] */
         kvm_gicc_access(s, 0x00, cpu, &reg, false);
-        s->cpu_enabled[cpu] = (reg & 1);
+        s->cpu_control[cpu][0] = reg;
 
         /* GICC_PMR -> s->priority_mask[cpu] */
         kvm_gicc_access(s, 0x04, cpu, &reg, false);
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 1a7af45..57486fc 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -465,7 +465,7 @@ static void armv7m_nvic_reset(DeviceState *dev)
      * as enabled by default, and with a priority mask which allows
      * all interrupts through.
      */
-    s->gic.cpu_enabled[0] = true;
+    s->gic.cpu_control[0][0] = true;
     s->gic.priority_mask[0] = 0x100;
     /* The NVIC as a whole is always enabled. */
     s->gic.enabled = true;
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index b0430ff..e9814f4 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -54,6 +54,17 @@
 #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)
 
+#define GICC_CTLR_S_EN_GRP0   (1U << 0)
+#define GICC_CTLR_S_EN_GRP1   (1U << 1)
+#define GICC_CTLR_S_ACK_CTL       (1U << 2)
+#define GICC_CTLR_S_FIQ_EN        (1U << 3)
+#define GICC_CTLR_S_CBPR          (1U << 4) /* GICv1: SBPR */
+
+#define GICC_CTLR_S_MASK          0x7ff
+
+#define GICC_CTLR_NS_EN_GRP1  (1U << 0)
+#define GICC_CTLR_NS_MASK         (1 | 3 << 5 | 1 << 9)
+
 
 /* The special cases for the revision property: */
 #define REV_11MPCORE 0
@@ -65,6 +76,9 @@ void gic_complete_irq(GICState *s, int cpu, int irq);
 void gic_update(GICState *s);
 void gic_init_irqs_and_distributor(GICState *s, int num_irq);
 void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val);
+uint32_t gic_get_cpu_control(GICState *s, int cpu);
+void gic_set_cpu_control(GICState *s, int cpu, uint32_t value);
+
 
 static inline bool gic_test_pending(GICState *s, int irq, int cm)
 {
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index a39b066..a912972 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -58,7 +58,7 @@ typedef struct GICState {
         uint8_t enabled;
         uint8_t enabled_grp[GIC_NR_GROUP]; /* EnableGrp0 and EnableGrp1 */
     };
-    bool cpu_enabled[GIC_NCPU];
+    uint32_t cpu_control[GIC_NCPU][GIC_NR_GROUP];
 
     gic_irq_state irq_state[GIC_MAXIRQ];
     uint8_t irq_target[GIC_MAXIRQ];
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 07/15] hw/intc/arm_gic: Make ICCICR/GICC_CTLR banked

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 9660 bytes --]

On 22 August 2014 05:29, Fabian Aggeler <aggelerf@ethz.ch> wrote:

> 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 <aggelerf@ethz.ch>
> ---
>  hw/intc/arm_gic.c                | 54
> ++++++++++++++++++++++++++++++++++++----
>  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, 72 insertions(+), 13 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index c78b301..7f7fac3 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][0] & 1)) {
>              qemu_irq_lower(s->parent_irq[cpu]);
>              return;
>          }
> @@ -239,6 +239,52 @@ void gic_set_priority(GICState *s, int cpu, int irq,
> uint8_t val)
>      }
>  }
>
> +uint32_t gic_get_cpu_control(GICState *s, int cpu)
> +{
> +    if ((s->revision >= 2 && !s->security_extn)
> +            || (s->security_extn && !ns_access())) {
> +        return s->cpu_control[cpu][0];
> +    } else if (s->security_extn && ns_access()) {
> +        return s->cpu_control[cpu][1];
> +    } else {
> +        return s->cpu_control[cpu][0];
> +    }
> +}
>

This conditional is not sufficient.  In the case of GICv1 without the
security extension, we fall through to the else case which returns [0]
instead of a masked version of [1].

This may be better:

if (!s->security_extn) {

if (s->revision == 1) {

res = s->cpu_control[cpu][1];

res &= 0x1;

} else {

res = s->cpu_control[cpu][0];

}

} else {

if (ns_access()) {

res = s->cpu_control[cpu][1];

if (s->revision == 1) {

res &= 0x1;

}

} else {

res = s->cpu_control[cpu][0];

}

}

Similar may apply below.

+
> +void gic_set_cpu_control(GICState *s, int cpu, uint32_t value)
> +{
> +    /* CPU Interface Control is banked for GICv2 and GICv1 with Security
> Extn */
> +    if ((s->revision >= 2 && !s->security_extn)
> +            || (s->security_extn && !ns_access())) {
> +        /* 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;
> +
> +        DPRINTF("CPU Interface %d: Group0 Interrupts %sabled, "
> +                "Group1 Interrupts %sabled\n", cpu,
> +                (s->cpu_control[cpu][0] & GICC_CTLR_S_EN_GRP0) ? "En" :
> "Dis",
> +                (s->cpu_control[cpu][0] & GICC_CTLR_S_EN_GRP1) ? "En" :
> "Dis");
> +    } else if (s->security_extn && ns_access()) {
> +        /* Write to Non-secure instance of the register */
> +        s->cpu_control[cpu][1] = (value & GICC_CTLR_NS_MASK);
> +        /* Synchronize EnableGrp1 alias of Secure copy */
> +        s->cpu_control[cpu][0] &= ~GICC_CTLR_S_EN_GRP1;
> +        s->cpu_control[cpu][0] |= (value & GICC_CTLR_NS_EN_GRP1) ?
> +                GICC_CTLR_S_EN_GRP1 : 0;
> +
> +        DPRINTF("CPU Interface %d: Group1 Interrupts %sabled\n", cpu,
> +                (s->cpu_control[cpu][1] & GICC_CTLR_NS_EN_GRP1) ? "En" :
> "Dis");
> +    } else {
> +        s->cpu_control[cpu][0] = (value & 1);
> +
> +        DPRINTF("CPU Interface %d %sabled\n", cpu,
> +                s->cpu_control[cpu][0] ? "En" : "Dis");
> +    }
> +}
> +
>  void gic_complete_irq(GICState *s, int cpu, int irq)
>  {
>      int update = 0;
> @@ -742,7 +788,7 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int
> offset)
>  {
>      switch (offset) {
>      case 0x00: /* Control */
> -        return s->cpu_enabled[cpu];
> +        return gic_get_cpu_control(s, cpu);
>      case 0x04: /* Priority mask */
>          return s->priority_mask[cpu];
>      case 0x08: /* Binary Point */
> @@ -768,9 +814,7 @@ static void gic_cpu_write(GICState *s, int cpu, int
> offset, uint32_t value)
>  {
>      switch (offset) {
>      case 0x00: /* Control */
> -        s->cpu_enabled[cpu] = (value & 1);
> -        DPRINTF("CPU %d %sabled\n", cpu, s->cpu_enabled[cpu] ? "En" :
> "Dis");
> -        break;
> +        return gic_set_cpu_control(s, cpu, value);
>      case 0x04: /* Priority mask */
>          s->priority_mask[cpu] = (value & 0xff);
>          break;
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index 7652754..c873f7b 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -65,7 +65,7 @@ static const VMStateDescription vmstate_gic = {
>      .post_load = gic_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT8_ARRAY(enabled_grp, GICState, GIC_NR_GROUP),
> -        VMSTATE_BOOL_ARRAY(cpu_enabled, GICState, GIC_NCPU),
> +        VMSTATE_UINT32_2DARRAY(cpu_control, GICState, GIC_NCPU,
> GIC_NR_GROUP),
>          VMSTATE_STRUCT_ARRAY(irq_state, GICState, GIC_MAXIRQ, 1,
>                               vmstate_gic_irq_state, gic_irq_state),
>          VMSTATE_UINT8_ARRAY(irq_target, GICState, GIC_MAXIRQ),
> @@ -127,7 +127,8 @@ static void arm_gic_common_reset(DeviceState *dev)
>          s->current_pending[i] = 1023;
>          s->running_irq[i] = 1023;
>          s->running_priority[i] = 0x100;
> -        s->cpu_enabled[i] = false;
> +        s->cpu_control[i][0] = false;
> +        s->cpu_control[i][1] = false;
>      }
>      for (i = 0; i < 16; i++) {
>          GIC_SET_ENABLED(i, ALL_CPU_MASK);
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index 5038885..9a6a2bb 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -379,8 +379,8 @@ static void kvm_arm_gic_put(GICState *s)
>       */
>
>      for (cpu = 0; cpu < s->num_cpu; cpu++) {
> -        /* s->cpu_enabled[cpu] -> GICC_CTLR */
> -        reg = s->cpu_enabled[cpu];
> +        /* s->cpu_enabled[cpu][0] -> GICC_CTLR */
> +        reg = s->cpu_control[cpu];
>          kvm_gicc_access(s, 0x00, cpu, &reg, true);
>
>          /* s->priority_mask[cpu] -> GICC_PMR */
> @@ -478,9 +478,9 @@ static void kvm_arm_gic_get(GICState *s)
>       */
>
>      for (cpu = 0; cpu < s->num_cpu; cpu++) {
> -        /* GICC_CTLR -> s->cpu_enabled[cpu] */
> +        /* GICC_CTLR -> s->cpu_control[cpu][0] */
>          kvm_gicc_access(s, 0x00, cpu, &reg, false);
> -        s->cpu_enabled[cpu] = (reg & 1);
> +        s->cpu_control[cpu][0] = reg;
>
>          /* GICC_PMR -> s->priority_mask[cpu] */
>          kvm_gicc_access(s, 0x04, cpu, &reg, false);
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 1a7af45..57486fc 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -465,7 +465,7 @@ static void armv7m_nvic_reset(DeviceState *dev)
>       * as enabled by default, and with a priority mask which allows
>       * all interrupts through.
>       */
> -    s->gic.cpu_enabled[0] = true;
> +    s->gic.cpu_control[0][0] = true;
>      s->gic.priority_mask[0] = 0x100;
>      /* The NVIC as a whole is always enabled. */
>      s->gic.enabled = true;
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index b0430ff..e9814f4 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -54,6 +54,17 @@
>  #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)
>
> +#define GICC_CTLR_S_EN_GRP0   (1U << 0)
> +#define GICC_CTLR_S_EN_GRP1   (1U << 1)
> +#define GICC_CTLR_S_ACK_CTL       (1U << 2)
> +#define GICC_CTLR_S_FIQ_EN        (1U << 3)
> +#define GICC_CTLR_S_CBPR          (1U << 4) /* GICv1: SBPR */
> +
> +#define GICC_CTLR_S_MASK          0x7ff
> +
> +#define GICC_CTLR_NS_EN_GRP1  (1U << 0)
> +#define GICC_CTLR_NS_MASK         (1 | 3 << 5 | 1 << 9)
> +
>
>  /* The special cases for the revision property: */
>  #define REV_11MPCORE 0
> @@ -65,6 +76,9 @@ void gic_complete_irq(GICState *s, int cpu, int irq);
>  void gic_update(GICState *s);
>  void gic_init_irqs_and_distributor(GICState *s, int num_irq);
>  void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val);
> +uint32_t gic_get_cpu_control(GICState *s, int cpu);
> +void gic_set_cpu_control(GICState *s, int cpu, uint32_t value);
> +
>
>  static inline bool gic_test_pending(GICState *s, int irq, int cm)
>  {
> diff --git a/include/hw/intc/arm_gic_common.h
> b/include/hw/intc/arm_gic_common.h
> index a39b066..a912972 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -58,7 +58,7 @@ typedef struct GICState {
>          uint8_t enabled;
>          uint8_t enabled_grp[GIC_NR_GROUP]; /* EnableGrp0 and EnableGrp1 */
>      };
> -    bool cpu_enabled[GIC_NCPU];
> +    uint32_t cpu_control[GIC_NCPU][GIC_NR_GROUP];
>
>      gic_irq_state irq_state[GIC_MAXIRQ];
>      uint8_t irq_target[GIC_MAXIRQ];
> --
> 1.8.3.2
>
>

[-- Attachment #2: Type: text/html, Size: 15000 bytes --]

[Qemu-devel] [PATCH 08/15] hw/intc/arm_gic: Make ICCBPR/GICC_BPR banked

[Fabian Aggeler]

This register is banked in GICs with Security Extensions. Storing the
non-secure copy of BPR in the abpr, which is an alias to the non-secure
copy for secure access. ABPR itself is only accessible from secure state
if the GIC implements Security Extensions.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
---
 hw/intc/arm_gic.c                | 25 +++++++++++++++++++++----
 include/hw/intc/arm_gic_common.h |  8 +++++---
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 7f7fac3..57021fd 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -792,7 +792,12 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
     case 0x04: /* Priority mask */
         return s->priority_mask[cpu];
     case 0x08: /* Binary Point */
-        return s->bpr[cpu];
+        if (s->security_extn && ns_access()) {
+            /* BPR is banked. Non-secure copy stored in ABPR. */
+            return s->abpr[cpu];
+        } else {
+            return s->bpr[cpu];
+        }
     case 0x0c: /* Acknowledge */
         return gic_acknowledge_irq(s, cpu);
     case 0x14: /* Running Priority */
@@ -800,7 +805,14 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
     case 0x18: /* Highest Pending Interrupt */
         return s->current_pending[cpu];
     case 0x1c: /* Aliased Binary Point */
-        return s->abpr[cpu];
+        if ((s->security_extn && ns_access())) {
+            /* If Security Extensions are present ABPR is a secure register,
+             * only accessible from secure state.
+             */
+            return 0;
+        } else {
+            return s->abpr[cpu];
+        }
     case 0xd0: case 0xd4: case 0xd8: case 0xdc:
         return s->apr[(offset - 0xd0) / 4][cpu];
     default:
@@ -819,12 +831,17 @@ static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value)
         s->priority_mask[cpu] = (value & 0xff);
         break;
     case 0x08: /* Binary Point */
-        s->bpr[cpu] = (value & 0x7);
+        if (s->security_extn && ns_access()) {
+            /* BPR is banked. Non-secure copy stored in ABPR. */
+            s->abpr[cpu] = (value & 0x7);
+        } else {
+            s->bpr[cpu] = (value & 0x7);
+        }
         break;
     case 0x10: /* End Of Interrupt */
         return gic_complete_irq(s, cpu, value & 0x3ff);
     case 0x1c: /* Aliased Binary Point */
-        if (s->revision >= 2) {
+        if (s->revision >= 2 && !(s->security_extn && ns_access())) {
             s->abpr[cpu] = (value & 0x7);
         }
         break;
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index a912972..c547418 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -78,9 +78,11 @@ typedef struct GICState {
     uint16_t running_priority[GIC_NCPU];
     uint16_t current_pending[GIC_NCPU];
 
-    /* We present the GICv2 without security extensions to a guest and
-     * therefore the guest can configure the GICC_CTLR to configure group 1
-     * binary point in the abpr.
+    /* If we present the GICv2 without security extensions to a guest,
+     * the guest can configure the GICC_CTLR to configure group 1 binary point
+     * in the abpr.
+     * For a GIC with Security Extensions we use use bpr for the
+     * secure copy and abpr as storage for the non-secure copy of the register.
      */
     uint8_t  bpr[GIC_NCPU];
     uint8_t  abpr[GIC_NCPU];
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 08/15] hw/intc/arm_gic: Make ICCBPR/GICC_BPR banked

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 4201 bytes --]

On 22 August 2014 05:29, Fabian Aggeler <aggelerf@ethz.ch> wrote:

> This register is banked in GICs with Security Extensions. Storing the
> non-secure copy of BPR in the abpr, which is an alias to the non-secure
> copy for secure access. ABPR itself is only accessible from secure state
> if the GIC implements Security Extensions.
>
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> ---
>  hw/intc/arm_gic.c                | 25 +++++++++++++++++++++----
>  include/hw/intc/arm_gic_common.h |  8 +++++---
>  2 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 7f7fac3..57021fd 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -792,7 +792,12 @@ static uint32_t gic_cpu_read(GICState *s, int cpu,
> int offset)
>      case 0x04: /* Priority mask */
>          return s->priority_mask[cpu];
>      case 0x08: /* Binary Point */
> -        return s->bpr[cpu];
> +        if (s->security_extn && ns_access()) {
> +            /* BPR is banked. Non-secure copy stored in ABPR. */
> +            return s->abpr[cpu];
> +        } else {
> +            return s->bpr[cpu];
> +        }
>      case 0x0c: /* Acknowledge */
>          return gic_acknowledge_irq(s, cpu);
>      case 0x14: /* Running Priority */
> @@ -800,7 +805,14 @@ static uint32_t gic_cpu_read(GICState *s, int cpu,
> int offset)
>      case 0x18: /* Highest Pending Interrupt */
>          return s->current_pending[cpu];
>      case 0x1c: /* Aliased Binary Point */
> -        return s->abpr[cpu];
> +        if ((s->security_extn && ns_access())) {
> +            /* If Security Extensions are present ABPR is a secure
> register,
> +             * only accessible from secure state.
> +             */
> +            return 0;
> +        } else {
> +            return s->abpr[cpu];
> +        }
>      case 0xd0: case 0xd4: case 0xd8: case 0xdc:
>          return s->apr[(offset - 0xd0) / 4][cpu];
>      default:
> @@ -819,12 +831,17 @@ static void gic_cpu_write(GICState *s, int cpu, int
> offset, uint32_t value)
>          s->priority_mask[cpu] = (value & 0xff);
>          break;
>      case 0x08: /* Binary Point */
> -        s->bpr[cpu] = (value & 0x7);
> +        if (s->security_extn && ns_access()) {
> +            /* BPR is banked. Non-secure copy stored in ABPR. */
> +            s->abpr[cpu] = (value & 0x7);
> +        } else {
> +            s->bpr[cpu] = (value & 0x7);
> +        }
>          break;
>      case 0x10: /* End Of Interrupt */
>          return gic_complete_irq(s, cpu, value & 0x3ff);
>      case 0x1c: /* Aliased Binary Point */
> -        if (s->revision >= 2) {
> +        if (s->revision >= 2 && !(s->security_extn && ns_access())) {
>

According to to the v2 spec, this register is present in GICv1 if the
security extensions are present but always in GICv2 (reason for the
previous condition).  I think this needs to be rewritten to be :

if ((s->revision >= 2 && !s->security_extn) || (s->security_extn &&
!ns_access()) {

s->abpr[cpu] = (value & 0x7);

}


>              s->abpr[cpu] = (value & 0x7);
>          }
>          break;
> diff --git a/include/hw/intc/arm_gic_common.h
> b/include/hw/intc/arm_gic_common.h
> index a912972..c547418 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -78,9 +78,11 @@ typedef struct GICState {
>      uint16_t running_priority[GIC_NCPU];
>      uint16_t current_pending[GIC_NCPU];
>
> -    /* We present the GICv2 without security extensions to a guest and
> -     * therefore the guest can configure the GICC_CTLR to configure group
> 1
> -     * binary point in the abpr.
> +    /* If we present the GICv2 without security extensions to a guest,
> +     * the guest can configure the GICC_CTLR to configure group 1 binary
> point
> +     * in the abpr.
> +     * For a GIC with Security Extensions we use use bpr for the
> +     * secure copy and abpr as storage for the non-secure copy of the
> register.
>       */
>      uint8_t  bpr[GIC_NCPU];
>      uint8_t  abpr[GIC_NCPU];
> --
> 1.8.3.2
>
>
Not sure if it occurs elsewhere, but these changes don't account for the
GICC_CTLR.CBPR settings which affects the read value.

[-- Attachment #2: Type: text/html, Size: 5823 bytes --]

[Qemu-devel] [PATCH 09/15] hw/intc/arm_gic: Implement Non-secure view of RPR

[Fabian Aggeler]

For GICs with Security Extensions Non-secure reads have a restricted
view on the current running priority.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
---
 hw/intc/arm_gic.c      | 17 ++++++++++++++++-
 hw/intc/gic_internal.h |  1 +
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 57021fd..473b8f4 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -285,6 +285,21 @@ void gic_set_cpu_control(GICState *s, int cpu, uint32_t value)
     }
 }
 
+uint8_t gic_get_running_priority(GICState *s, int cpu)
+{
+    if (s->security_extn && ns_access()) {
+        if (s->running_priority[cpu] & 0x80) {
+            /* Running priority in upper half, return Non-secure view */
+            return s->running_priority[cpu] << 1;
+        } else {
+            /* Running priority in lower half, RAZ */
+            return 0;
+        }
+    } else {
+        return s->running_priority[cpu];
+    }
+}
+
 void gic_complete_irq(GICState *s, int cpu, int irq)
 {
     int update = 0;
@@ -801,7 +816,7 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
     case 0x0c: /* Acknowledge */
         return gic_acknowledge_irq(s, cpu);
     case 0x14: /* Running Priority */
-        return s->running_priority[cpu];
+        return gic_get_running_priority(s, cpu);
     case 0x18: /* Highest Pending Interrupt */
         return s->current_pending[cpu];
     case 0x1c: /* Aliased Binary Point */
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index e9814f4..433d75e 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -78,6 +78,7 @@ void gic_init_irqs_and_distributor(GICState *s, int num_irq);
 void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val);
 uint32_t gic_get_cpu_control(GICState *s, int cpu);
 void gic_set_cpu_control(GICState *s, int cpu, uint32_t value);
+uint8_t gic_get_running_priority(GICState *s, int cpu);
 
 
 static inline bool gic_test_pending(GICState *s, int irq, int cm)
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 09/15] hw/intc/arm_gic: Implement Non-secure view of RPR

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 2290 bytes --]

On 22 August 2014 05:29, Fabian Aggeler <aggelerf@ethz.ch> wrote:

> For GICs with Security Extensions Non-secure reads have a restricted
> view on the current running priority.
>
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> ---
>  hw/intc/arm_gic.c      | 17 ++++++++++++++++-
>  hw/intc/gic_internal.h |  1 +
>  2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 57021fd..473b8f4 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -285,6 +285,21 @@ void gic_set_cpu_control(GICState *s, int cpu,
> uint32_t value)
>      }
>  }
>
> +uint8_t gic_get_running_priority(GICState *s, int cpu)
> +{
> +    if (s->security_extn && ns_access()) {
> +        if (s->running_priority[cpu] & 0x80) {

+            /* Running priority in upper half, return Non-secure view */
> +            return s->running_priority[cpu] << 1;
>

This needs to be masked otherwise it could return a value with rsvd bits
set.


> +        } else {
> +            /* Running priority in lower half, RAZ */
> +            return 0;
> +        }
> +    } else {
> +        return s->running_priority[cpu];
> +    }
> +}
> +
>  void gic_complete_irq(GICState *s, int cpu, int irq)
>  {
>      int update = 0;
> @@ -801,7 +816,7 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int
> offset)
>      case 0x0c: /* Acknowledge */
>          return gic_acknowledge_irq(s, cpu);
>      case 0x14: /* Running Priority */
> -        return s->running_priority[cpu];
> +        return gic_get_running_priority(s, cpu);
>      case 0x18: /* Highest Pending Interrupt */
>          return s->current_pending[cpu];
>      case 0x1c: /* Aliased Binary Point */
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index e9814f4..433d75e 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -78,6 +78,7 @@ void gic_init_irqs_and_distributor(GICState *s, int
> num_irq);
>  void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val);
>  uint32_t gic_get_cpu_control(GICState *s, int cpu);
>  void gic_set_cpu_control(GICState *s, int cpu, uint32_t value);
> +uint8_t gic_get_running_priority(GICState *s, int cpu);
>
>
>  static inline bool gic_test_pending(GICState *s, int irq, int cm)
> --
> 1.8.3.2
>
>

[-- Attachment #2: Type: text/html, Size: 3256 bytes --]

[Qemu-devel] [PATCH 10/15] hw/intc/arm_gic: Handle grouping for GICC_HPPIR

[Fabian Aggeler]

Grouping (GICv2) and Security Extensions change the behaviour of reads
of the highest priority pending interrupt register (ICCHPIR/GICC_HPPIR).

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
---
 hw/intc/arm_gic.c      | 29 ++++++++++++++++++++++++++++-
 hw/intc/gic_internal.h |  1 +
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 473b8f4..78efae1 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -300,6 +300,33 @@ uint8_t gic_get_running_priority(GICState *s, int cpu)
     }
 }
 
+uint16_t gic_get_current_pending_irq(GICState *s, int cpu)
+{
+    bool isGrp0;
+    uint16_t pendingId = s->current_pending[cpu];
+
+    if (pendingId < GIC_MAXIRQ && (s->revision >= 2 || s->security_extn)) {
+        isGrp0 = GIC_TEST_GROUP0(pendingId, (1 << cpu));
+        if ((isGrp0 && !s->enabled_grp[0])
+                || (!isGrp0 && !s->enabled_grp[1])) {
+            return 1023;
+        }
+        if (s->security_extn) {
+            if (isGrp0 && ns_access()) {
+                /* Group0 interrupts hidden from Non-secure access */
+                return 1023;
+            }
+            if (!isGrp0 && !ns_access()
+                    && !(s->cpu_control[cpu][0] & GICC_CTLR_S_ACK_CTL)) {
+                /* Group1 interrupts only seen by Secure access if
+                 * AckCtl bit set. */
+                return 1022;
+            }
+        }
+    }
+    return pendingId;
+}
+
 void gic_complete_irq(GICState *s, int cpu, int irq)
 {
     int update = 0;
@@ -818,7 +845,7 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
     case 0x14: /* Running Priority */
         return gic_get_running_priority(s, cpu);
     case 0x18: /* Highest Pending Interrupt */
-        return s->current_pending[cpu];
+        return gic_get_current_pending_irq(s, cpu);
     case 0x1c: /* Aliased Binary Point */
         if ((s->security_extn && ns_access())) {
             /* If Security Extensions are present ABPR is a secure register,
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index 433d75e..17632c1 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -79,6 +79,7 @@ void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val);
 uint32_t gic_get_cpu_control(GICState *s, int cpu);
 void gic_set_cpu_control(GICState *s, int cpu, uint32_t value);
 uint8_t gic_get_running_priority(GICState *s, int cpu);
+uint16_t gic_get_current_pending_irq(GICState *s, int cpu);
 
 
 static inline bool gic_test_pending(GICState *s, int irq, int cm)
-- 
1.8.3.2

[Qemu-devel] [PATCH 11/15] hw/intc/arm_gic: Change behavior of EOIR writes

[Fabian Aggeler]

Grouping (GICv2) and Security Extensions change the behavior of EOIR
writes. Completing Group0 interrupts is only allowed from Secure state
and completing Group1 interrupts from Secure state is only allowed if
AckCtl bit is set.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
---
 hw/intc/arm_gic.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 78efae1..a96f4a2 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -355,6 +355,21 @@ void gic_complete_irq(GICState *s, int cpu, int irq)
             GIC_SET_PENDING(irq, cm);
             update = 1;
         }
+    } else if ((s->revision >= 2 && !s->security_extn)
+                 || (s->security_extn && !ns_access())) {
+        /* Handle GICv2 without Security Extensions or GIC with Security
+         * Extensions and a secure write.
+         */
+        if (!GIC_TEST_GROUP0(irq, cm)
+                && !(s->cpu_control[cpu][0] & GICC_CTLR_S_ACK_CTL)) {
+            /* Unpredictable. We choose to ignore. */
+            DPRINTF("EOI for Group1 interrupt %d ignored "
+                    "(AckCtl disabled)\n", irq);
+            return;
+        }
+    } else if (s->security_extn && ns_access() && GIC_TEST_GROUP0(irq, cm)) {
+        DPRINTF("Non-secure EOI for Group0 interrupt %d ignored\n", irq);
+        return;
     }
 
     if (irq != s->running_irq[cpu]) {
-- 
1.8.3.2

[Qemu-devel] [PATCH 12/15] hw/intc/arm_gic: Change behavior of IAR writes

[Fabian Aggeler]

Grouping (GICv2) and Security Extensions change the behavior of IAR
reads. Acknowledging Group0 interrupts is only allowed from Secure
state and acknowledging Group1 interrupts from Secure state is only
allowed if AckCtl bit is set.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
---
 hw/intc/arm_gic.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index a96f4a2..cddad45 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -189,11 +189,35 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu)
     int ret, irq, src;
     int cm = 1 << cpu;
     irq = s->current_pending[cpu];
+    bool isGrp0;
     if (irq == 1023
             || GIC_GET_PRIORITY(irq, cpu) >= s->running_priority[cpu]) {
         DPRINTF("ACK no pending IRQ\n");
         return 1023;
     }
+
+    if (s->revision >= 2 || s->security_extn) {
+        isGrp0 = GIC_TEST_GROUP0(irq, (1 << cpu));
+        if ((isGrp0 && (!s->enabled_grp[0]
+                    || !(s->cpu_control[cpu][0] & GICC_CTLR_S_EN_GRP0)))
+           || (!isGrp0 && (!s->enabled_grp[1]
+                    || !(s->cpu_control[cpu][1] & GICC_CTLR_NS_EN_GRP1)))) {
+            return 1023;
+        }
+
+        if ((s->revision >= 2 && !s->security_extn)
+                || (s->security_extn && !ns_access())) {
+            if (!isGrp0 && !(s->cpu_control[cpu][0] & GICC_CTLR_S_ACK_CTL)) {
+                DPRINTF("Read of IAR ignored for Group1 interrupt %d "
+                        "(AckCtl disabled)\n", irq);
+                return 1022;
+            }
+        } else if (s->security_extn && ns_access() && isGrp0) {
+            DPRINTF("Non-secure read of IAR ignored for Group0 interrupt %d\n",
+                    irq);
+            return 1023;
+        }
+    }
     s->last_active[irq][cpu] = s->running_irq[cpu];
 
     if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
-- 
1.8.3.2

[Qemu-devel] [PATCH 13/15] hw/intc/arm_gic: Restrict priority view

[Fabian Aggeler]

GICs with Security Extensions restrict the non-secure view of the
interrupt priority and priority mask registers.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
---
 hw/intc/arm_gic.c      | 66 +++++++++++++++++++++++++++++++++++++++++++++-----
 hw/intc/gic_internal.h |  3 +++
 2 files changed, 63 insertions(+), 6 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index cddad45..3fe5f09 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -256,11 +256,66 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu)
 
 void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val)
 {
+    uint8_t prio = val;
+
+    if (s->security_extn && ns_access()) {
+        if (GIC_TEST_GROUP0(irq, (1 << cpu))) {
+            return; /* Ignore Non-secure access of Group0 IRQ */
+        }
+        prio = 0x80 | (prio >> 1); /* Non-secure view */
+    }
+
     if (irq < GIC_INTERNAL) {
-        s->priority1[irq][cpu] = val;
+        s->priority1[irq][cpu] = prio;
     } else {
-        s->priority2[(irq) - GIC_INTERNAL] = val;
+        s->priority2[(irq) - GIC_INTERNAL] = prio;
+    }
+}
+
+uint32_t gic_get_priority(GICState *s, int cpu, int irq)
+{
+    uint32_t prio = GIC_GET_PRIORITY(irq, cpu);
+
+    if (s->security_extn && ns_access()) {
+        if (GIC_TEST_GROUP0(irq, (1 << cpu))) {
+            return 0; /* Non-secure access cannot read priority of Group0 IRQ */
+        }
+        prio = (prio << 1); /* Non-secure view */
     }
+    return prio;
+}
+
+void gic_set_priority_mask(GICState *s, int cpu, uint8_t val)
+{
+    uint8_t pmask = (val & 0xff);
+
+    if (s->security_extn && ns_access()) {
+        if (s->priority_mask[cpu] & 0x80) {
+            /* Priority Mask in upper half */
+            pmask = 0x80 | (pmask >> 1);
+        } else {
+            /* Non-secure write ignored if priority mask is in lower half */
+            return;
+        }
+    }
+    s->priority_mask[cpu] = pmask;
+}
+
+uint32_t gic_get_priority_mask(GICState *s, int cpu)
+{
+    uint32_t pmask = s->priority_mask[cpu];
+
+    if (s->security_extn && ns_access()) {
+        if (pmask & 0x80) {
+            /* Priority Mask in upper half, return Non-secure view */
+            pmask = (pmask << 1);
+        } else {
+            /* Priority Mask in lower half, RAZ */
+            pmask = 0;
+        }
+    }
+    return pmask;
+
 }
 
 uint32_t gic_get_cpu_control(GICState *s, int cpu)
@@ -518,7 +573,7 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
         irq = (offset - 0x400) + GIC_BASE_IRQ;
         if (irq >= s->num_irq)
             goto bad_reg;
-        res = GIC_GET_PRIORITY(irq, cpu);
+        res = gic_get_priority(s, cpu, irq);
     } else if (offset < 0xc00) {
         /* Interrupt CPU Target.  */
         if (s->num_cpu == 1 && s->revision != REV_11MPCORE) {
@@ -871,7 +926,7 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
     case 0x00: /* Control */
         return gic_get_cpu_control(s, cpu);
     case 0x04: /* Priority mask */
-        return s->priority_mask[cpu];
+        return gic_get_priority_mask(s, cpu);
     case 0x08: /* Binary Point */
         if (s->security_extn && ns_access()) {
             /* BPR is banked. Non-secure copy stored in ABPR. */
@@ -909,8 +964,7 @@ static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value)
     case 0x00: /* Control */
         return gic_set_cpu_control(s, cpu, value);
     case 0x04: /* Priority mask */
-        s->priority_mask[cpu] = (value & 0xff);
-        break;
+        return gic_set_priority_mask(s, cpu, value);
     case 0x08: /* Binary Point */
         if (s->security_extn && ns_access()) {
             /* BPR is banked. Non-secure copy stored in ABPR. */
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index 17632c1..8d951cc 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -76,6 +76,9 @@ void gic_complete_irq(GICState *s, int cpu, int irq);
 void gic_update(GICState *s);
 void gic_init_irqs_and_distributor(GICState *s, int num_irq);
 void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val);
+uint32_t gic_get_priority(GICState *s, int cpu, int irq);
+void gic_set_priority_mask(GICState *s, int cpu, uint8_t val);
+uint32_t gic_get_priority_mask(GICState *s, int cpu);
 uint32_t gic_get_cpu_control(GICState *s, int cpu);
 void gic_set_cpu_control(GICState *s, int cpu, uint32_t value);
 uint8_t gic_get_running_priority(GICState *s, int cpu);
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 13/15] hw/intc/arm_gic: Restrict priority view

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 5017 bytes --]

On 22 August 2014 05:29, Fabian Aggeler <aggelerf@ethz.ch> wrote:

> GICs with Security Extensions restrict the non-secure view of the
> interrupt priority and priority mask registers.
>
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> ---
>  hw/intc/arm_gic.c      | 66
> +++++++++++++++++++++++++++++++++++++++++++++-----
>  hw/intc/gic_internal.h |  3 +++
>  2 files changed, 63 insertions(+), 6 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index cddad45..3fe5f09 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -256,11 +256,66 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu)
>
>  void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val)
>  {
> +    uint8_t prio = val;
> +
> +    if (s->security_extn && ns_access()) {
> +        if (GIC_TEST_GROUP0(irq, (1 << cpu))) {
> +            return; /* Ignore Non-secure access of Group0 IRQ */
> +        }
> +        prio = 0x80 | (prio >> 1); /* Non-secure view */
> +    }
> +
>      if (irq < GIC_INTERNAL) {
> -        s->priority1[irq][cpu] = val;
> +        s->priority1[irq][cpu] = prio;
>      } else {
> -        s->priority2[(irq) - GIC_INTERNAL] = val;
> +        s->priority2[(irq) - GIC_INTERNAL] = prio;
> +    }
> +}
> +
> +uint32_t gic_get_priority(GICState *s, int cpu, int irq)
> +{
> +    uint32_t prio = GIC_GET_PRIORITY(irq, cpu);
> +
> +    if (s->security_extn && ns_access()) {
> +        if (GIC_TEST_GROUP0(irq, (1 << cpu))) {
> +            return 0; /* Non-secure access cannot read priority of Group0
> IRQ */
> +        }
> +        prio = (prio << 1); /* Non-secure view */
>

This should probably be masked to avoid a return containing reserved bits
or only return a uint8_t.


>      }
> +    return prio;
> +}
> +
> +void gic_set_priority_mask(GICState *s, int cpu, uint8_t val)
> +{
> +    uint8_t pmask = (val & 0xff);
> +
> +    if (s->security_extn && ns_access()) {
> +        if (s->priority_mask[cpu] & 0x80) {
> +            /* Priority Mask in upper half */
> +            pmask = 0x80 | (pmask >> 1);
> +        } else {
> +            /* Non-secure write ignored if priority mask is in lower half
> */
> +            return;
> +        }
> +    }
> +    s->priority_mask[cpu] = pmask;
> +}
> +
> +uint32_t gic_get_priority_mask(GICState *s, int cpu)
> +{
> +    uint32_t pmask = s->priority_mask[cpu];
> +
> +    if (s->security_extn && ns_access()) {
> +        if (pmask & 0x80) {
> +            /* Priority Mask in upper half, return Non-secure view */
> +            pmask = (pmask << 1);
> +        } else {
> +            /* Priority Mask in lower half, RAZ */
> +            pmask = 0;
>

Again this should probably be masked to prevent returning a value with
reserved bits set.


> +        }
> +    }
> +    return pmask;
> +
>  }
>
>  uint32_t gic_get_cpu_control(GICState *s, int cpu)
> @@ -518,7 +573,7 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr
> offset)
>          irq = (offset - 0x400) + GIC_BASE_IRQ;
>          if (irq >= s->num_irq)
>              goto bad_reg;
> -        res = GIC_GET_PRIORITY(irq, cpu);
> +        res = gic_get_priority(s, cpu, irq);
>      } else if (offset < 0xc00) {
>          /* Interrupt CPU Target.  */
>          if (s->num_cpu == 1 && s->revision != REV_11MPCORE) {
> @@ -871,7 +926,7 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int
> offset)
>      case 0x00: /* Control */
>          return gic_get_cpu_control(s, cpu);
>      case 0x04: /* Priority mask */
> -        return s->priority_mask[cpu];
> +        return gic_get_priority_mask(s, cpu);
>      case 0x08: /* Binary Point */
>          if (s->security_extn && ns_access()) {
>              /* BPR is banked. Non-secure copy stored in ABPR. */
> @@ -909,8 +964,7 @@ static void gic_cpu_write(GICState *s, int cpu, int
> offset, uint32_t value)
>      case 0x00: /* Control */
>          return gic_set_cpu_control(s, cpu, value);
>      case 0x04: /* Priority mask */
> -        s->priority_mask[cpu] = (value & 0xff);
> -        break;
> +        return gic_set_priority_mask(s, cpu, value);
>      case 0x08: /* Binary Point */
>          if (s->security_extn && ns_access()) {
>              /* BPR is banked. Non-secure copy stored in ABPR. */
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index 17632c1..8d951cc 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -76,6 +76,9 @@ void gic_complete_irq(GICState *s, int cpu, int irq);
>  void gic_update(GICState *s);
>  void gic_init_irqs_and_distributor(GICState *s, int num_irq);
>  void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val);
> +uint32_t gic_get_priority(GICState *s, int cpu, int irq);
> +void gic_set_priority_mask(GICState *s, int cpu, uint8_t val);
> +uint32_t gic_get_priority_mask(GICState *s, int cpu);
>  uint32_t gic_get_cpu_control(GICState *s, int cpu);
>  void gic_set_cpu_control(GICState *s, int cpu, uint32_t value);
>  uint8_t gic_get_running_priority(GICState *s, int cpu);
> --
> 1.8.3.2
>
>

[-- Attachment #2: Type: text/html, Size: 6546 bytes --]

[Qemu-devel] [PATCH 14/15] hw/intc/arm_gic: Break out gic_update() function

[Fabian Aggeler]

Prepare to split gic_update() in two functions, one for GICs with
interrupt grouping and one without grouping (existing).

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
---
 hw/intc/arm_gic.c      | 11 ++++++++---
 hw/intc/gic_internal.h |  1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 3fe5f09..4fe3555 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -52,9 +52,7 @@ static inline bool ns_access(void)
     return true;
 }
 
-/* TODO: Many places that call this routine could be optimized.  */
-/* Update interrupt status after enabled or pending bits have been changed.  */
-void gic_update(GICState *s)
+inline void gic_update_no_grouping(GICState *s)
 {
     int best_irq;
     int best_prio;
@@ -92,6 +90,13 @@ void gic_update(GICState *s)
     }
 }
 
+/* TODO: Many places that call this routine could be optimized.  */
+/* Update interrupt status after enabled or pending bits have been changed.  */
+void gic_update(GICState *s)
+{
+    gic_update_no_grouping(s);
+}
+
 void gic_set_pending_private(GICState *s, int cpu, int irq)
 {
     int cm = 1 << cpu;
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index 8d951cc..150f867 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -73,6 +73,7 @@
 void gic_set_pending_private(GICState *s, int cpu, int irq);
 uint32_t gic_acknowledge_irq(GICState *s, int cpu);
 void gic_complete_irq(GICState *s, int cpu, int irq);
+inline void gic_update_no_grouping(GICState *s);
 void gic_update(GICState *s);
 void gic_init_irqs_and_distributor(GICState *s, int num_irq);
 void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val);
-- 
1.8.3.2

[Qemu-devel] [PATCH 15/15] hw/intc/arm_gic: add gic_update() for grouping

[Fabian Aggeler]

GICs with grouping (GICv2 or GICv1 with Security Extensions) have a
different exception generation model which is more complicated than
without interrupt grouping. We add a new function to handle this model.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
---
 hw/intc/arm_gic.c      | 87 +++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/intc/gic_internal.h |  1 +
 2 files changed, 87 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 4fe3555..f4492f4 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -52,6 +52,87 @@ static inline bool ns_access(void)
     return true;
 }
 
+inline void gic_update_with_grouping(GICState *s)
+{
+    int best_irq;
+    int best_prio;
+    int irq;
+    int irq_level;
+    int fiq_level;
+    int cpu;
+    int cm;
+    bool next_int;
+    bool next_grp0;
+    bool gicc_grp0_enabled;
+    bool gicc_grp1_enabled;
+
+    for (cpu = 0; cpu < NUM_CPU(s); cpu++) {
+        cm = 1 << cpu;
+        gicc_grp0_enabled = s->cpu_control[cpu][0] & GICC_CTLR_S_EN_GRP0;
+        gicc_grp1_enabled = s->cpu_control[cpu][1] & GICC_CTLR_S_EN_GRP1;
+        next_int = 0;
+        next_grp0 = 0;
+
+        s->current_pending[cpu] = 1023;
+        if ((!s->enabled_grp[0] && !s->enabled_grp[1])
+                || (!gicc_grp0_enabled && !gicc_grp1_enabled)) {
+            qemu_irq_lower(s->parent_irq[cpu]);
+            qemu_irq_lower(s->parent_fiq[cpu]);
+            return;
+        }
+
+        /* Determine highest priority pending interrupt */
+        best_prio = 0x100;
+        best_irq = 1023;
+        for (irq = 0; irq < s->num_irq; irq++) {
+            if (GIC_TEST_ENABLED(irq, cm) && gic_test_pending(s, irq, cm)) {
+                if (GIC_GET_PRIORITY(irq, cpu) < best_prio) {
+                    best_prio = GIC_GET_PRIORITY(irq, cpu);
+                    best_irq = irq;
+                }
+            }
+        }
+
+        /* Priority of IRQ higher than priority mask? */
+        if (best_prio < s->priority_mask[cpu]) {
+            s->current_pending[cpu] = best_irq;
+            if (GIC_TEST_GROUP0(best_irq, cm) && s->enabled_grp[0]) {
+                /* TODO: Add subpriority handling (binary point register) */
+                if (best_prio < s->running_priority[cpu]) {
+                    next_int = true;
+                    next_grp0 = true;
+                }
+            } else if (!GIC_TEST_GROUP0(best_irq, cm) && s->enabled_grp[1]) {
+                /* TODO: Add subpriority handling (binary point register) */
+                if (best_prio < s->running_priority[cpu]) {
+                    next_int = true;
+                    next_grp0 = false;
+                }
+            }
+        }
+
+        fiq_level = 0;
+        irq_level = 0;
+        if (next_int) {
+            if (next_grp0 && (s->cpu_control[cpu][0] & GICC_CTLR_S_FIQ_EN)) {
+                if (gicc_grp0_enabled) {
+                    fiq_level = 1;
+                    DPRINTF("Raised pending FIQ %d (cpu %d)\n", best_irq, cpu);
+                }
+            } else {
+                if ((next_grp0 && gicc_grp0_enabled)
+                     || (!next_grp0 && gicc_grp1_enabled)) {
+                    irq_level = 1;
+                    DPRINTF("Raised pending IRQ %d (cpu %d)\n", best_irq, cpu);
+                }
+            }
+        }
+        /* Set IRQ/FIQ signal */
+        qemu_set_irq(s->parent_irq[cpu], irq_level);
+        qemu_set_irq(s->parent_fiq[cpu], fiq_level);
+    }
+}
+
 inline void gic_update_no_grouping(GICState *s)
 {
     int best_irq;
@@ -94,7 +175,11 @@ inline void gic_update_no_grouping(GICState *s)
 /* Update interrupt status after enabled or pending bits have been changed.  */
 void gic_update(GICState *s)
 {
-    gic_update_no_grouping(s);
+    if (s->revision >= 2 || s->security_extn) {
+        gic_update_with_grouping(s);
+    } else {
+        gic_update_no_grouping(s);
+    }
 }
 
 void gic_set_pending_private(GICState *s, int cpu, int irq)
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index 150f867..6f67a09 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -73,6 +73,7 @@
 void gic_set_pending_private(GICState *s, int cpu, int irq);
 uint32_t gic_acknowledge_irq(GICState *s, int cpu);
 void gic_complete_irq(GICState *s, int cpu, int irq);
+inline void gic_update_with_grouping(GICState *s);
 inline void gic_update_no_grouping(GICState *s);
 void gic_update(GICState *s);
 void gic_init_irqs_and_distributor(GICState *s, int num_irq);
-- 
1.8.3.2