[Qemu-devel] [PATCH 0/3] arm_gic: Improve handling of GICD_ICFGR

[Qemu-devel] [PATCH 0/3] arm_gic: Improve handling of GICD_ICFGR

[Adam Lackorzynski]

Hi,

the following three patches address the behavior of the GICD_ICFGR register
in the ARM GIC.

Adam Lackorzynski (3):
  arm_gic: Fix read of GICD_ICFGR
  arm_gic: SGIs for GICD_ICFGR are WI
  arm_gic: GICD_ICFGR: Do not force edge-triggered PPIs

 hw/intc/arm_gic.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

-- 
2.0.1

[Qemu-devel] [PATCH 1/3] arm_gic: Fix read of GICD_ICFGR

[Adam Lackorzynski]

The GICD_ICFGR register covers 4 interrupts per byte.

Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
---
 hw/intc/arm_gic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 1532ef9..d2b1aaf 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -372,7 +372,7 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
         }
     } else if (offset < 0xf00) {
         /* Interrupt Configuration.  */
-        irq = (offset - 0xc00) * 2 + GIC_BASE_IRQ;
+        irq = (offset - 0xc00) * 4 + GIC_BASE_IRQ;
         if (irq >= s->num_irq)
             goto bad_reg;
         res = 0;
-- 
2.0.1

Re: [Qemu-devel] [PATCH 1/3] arm_gic: Fix read of GICD_ICFGR

[Christoffer Dall]

On Sun, Aug 03, 2014 at 10:53:45AM +0200, Adam Lackorzynski wrote:
> The GICD_ICFGR register covers 4 interrupts per byte.
> 
> Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
> ---
>  hw/intc/arm_gic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 1532ef9..d2b1aaf 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -372,7 +372,7 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
>          }
>      } else if (offset < 0xf00) {
>          /* Interrupt Configuration.  */
> -        irq = (offset - 0xc00) * 2 + GIC_BASE_IRQ;
> +        irq = (offset - 0xc00) * 4 + GIC_BASE_IRQ;
>          if (irq >= s->num_irq)
>              goto bad_reg;
>          res = 0;
> -- 
> 2.0.1
> 
> 
I guess we should consider only getting/setting the LSB of each field if
we're emulating a GICv1, but:

Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

[Qemu-devel] [PATCH 2/3] arm_gic: SGIs for GICD_ICFGR are WI

[Adam Lackorzynski]

Writes to SGIs for GICD_ICFGR register must be ignored.

Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
---
 hw/intc/arm_gic.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index d2b1aaf..cd6e6ea 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -566,10 +566,13 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
             } else {
                 GIC_CLEAR_MODEL(irq + i);
             }
-            if (value & (2 << (i * 2))) {
-                GIC_SET_EDGE_TRIGGER(irq + i);
-            } else {
-                GIC_CLEAR_EDGE_TRIGGER(irq + i);
+            /* SGIs are WI */
+            if (irq >= 16) {
+                if (value & (2 << (i * 2))) {
+                    GIC_SET_EDGE_TRIGGER(irq + i);
+                } else {
+                    GIC_CLEAR_EDGE_TRIGGER(irq + i);
+                }
             }
         }
     } else if (offset < 0xf10) {
-- 
2.0.1

Re: [Qemu-devel] [PATCH 2/3] arm_gic: SGIs for GICD_ICFGR are WI

[Christoffer Dall]

On Sun, Aug 03, 2014 at 10:53:46AM +0200, Adam Lackorzynski wrote:
> Writes to SGIs for GICD_ICFGR register must be ignored.
> 
> Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
> ---
>  hw/intc/arm_gic.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index d2b1aaf..cd6e6ea 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -566,10 +566,13 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>              } else {
>                  GIC_CLEAR_MODEL(irq + i);
>              }
> -            if (value & (2 << (i * 2))) {
> -                GIC_SET_EDGE_TRIGGER(irq + i);
> -            } else {
> -                GIC_CLEAR_EDGE_TRIGGER(irq + i);
> +            /* SGIs are WI */

They're actually WI/RAO, so we should set them to edge-triggered
somewhere or always return 1 for reads of these values as well as part
of this fix.

> +            if (irq >= 16) {
> +                if (value & (2 << (i * 2))) {
> +                    GIC_SET_EDGE_TRIGGER(irq + i);
> +                } else {
> +                    GIC_CLEAR_EDGE_TRIGGER(irq + i);
> +                }
>              }
>          }
>      } else if (offset < 0xf10) {
> -- 
> 2.0.1
> 
> 

Re: [Qemu-devel] [PATCH 2/3] arm_gic: SGIs for GICD_ICFGR are WI

[Adam Lackorzynski]

On Fri Aug 15, 2014 at 14:07:14 +0200, Christoffer Dall wrote:
> On Sun, Aug 03, 2014 at 10:53:46AM +0200, Adam Lackorzynski wrote:
> > Writes to SGIs for GICD_ICFGR register must be ignored.
> > 
> > Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
> > ---
> >  hw/intc/arm_gic.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> > index d2b1aaf..cd6e6ea 100644
> > --- a/hw/intc/arm_gic.c
> > +++ b/hw/intc/arm_gic.c
> > @@ -566,10 +566,13 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
> >              } else {
> >                  GIC_CLEAR_MODEL(irq + i);
> >              }
> > -            if (value & (2 << (i * 2))) {
> > -                GIC_SET_EDGE_TRIGGER(irq + i);
> > -            } else {
> > -                GIC_CLEAR_EDGE_TRIGGER(irq + i);
> > +            /* SGIs are WI */
> 
> They're actually WI/RAO, so we should set them to edge-triggered
> somewhere or always return 1 for reads of these values as well as part
> of this fix.

SGIs are initialized to edge triggered in arm_gic_common_reset(), i.e.
this is already the case.
 
> > +            if (irq >= 16) {
> > +                if (value & (2 << (i * 2))) {
> > +                    GIC_SET_EDGE_TRIGGER(irq + i);
> > +                } else {
> > +                    GIC_CLEAR_EDGE_TRIGGER(irq + i);
> > +                }
> >              }
> >          }
> >      } else if (offset < 0xf10) {
> > -- 
> > 2.0.1
> > 
> > 

Adam
-- 
Adam                 adam@os.inf.tu-dresden.de
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

Re: [Qemu-devel] [PATCH 2/3] arm_gic: SGIs for GICD_ICFGR are WI

[Christoffer Dall]

On Sun, Aug 03, 2014 at 10:53:46AM +0200, Adam Lackorzynski wrote:
> Writes to SGIs for GICD_ICFGR register must be ignored.
> 
> Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
> ---
>  hw/intc/arm_gic.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index d2b1aaf..cd6e6ea 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -566,10 +566,13 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>              } else {
>                  GIC_CLEAR_MODEL(irq + i);
>              }
> -            if (value & (2 << (i * 2))) {
> -                GIC_SET_EDGE_TRIGGER(irq + i);
> -            } else {
> -                GIC_CLEAR_EDGE_TRIGGER(irq + i);
> +            /* SGIs are WI */
> +            if (irq >= 16) {
> +                if (value & (2 << (i * 2))) {
> +                    GIC_SET_EDGE_TRIGGER(irq + i);
> +                } else {
> +                    GIC_CLEAR_EDGE_TRIGGER(irq + i);
> +                }
>              }
>          }
>      } else if (offset < 0xf10) {

Actually, this looks a bit weird given that you do set the model bit,
which should probably be treated as WI/RAZ for a GICv2 emulation, but
you don't set the edge trigger bit for them.

I think a cleaner fix might be to to just change the existing check from
(irq < GIC_INTERNAL) to (irq < GIT_NR_SGIS), then you also don't need
the next patch.

-Christoffer

Re: [Qemu-devel] [PATCH 2/3] arm_gic: SGIs for GICD_ICFGR are WI

[Adam Lackorzynski]

On Fri Aug 15, 2014 at 14:12:17 +0200, Christoffer Dall wrote:
> On Sun, Aug 03, 2014 at 10:53:46AM +0200, Adam Lackorzynski wrote:
> > Writes to SGIs for GICD_ICFGR register must be ignored.
> > 
> > Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
> > ---
> >  hw/intc/arm_gic.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> > index d2b1aaf..cd6e6ea 100644
> > --- a/hw/intc/arm_gic.c
> > +++ b/hw/intc/arm_gic.c
> > @@ -566,10 +566,13 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
> >              } else {
> >                  GIC_CLEAR_MODEL(irq + i);
> >              }
> > -            if (value & (2 << (i * 2))) {
> > -                GIC_SET_EDGE_TRIGGER(irq + i);
> > -            } else {
> > -                GIC_CLEAR_EDGE_TRIGGER(irq + i);
> > +            /* SGIs are WI */
> > +            if (irq >= 16) {
> > +                if (value & (2 << (i * 2))) {
> > +                    GIC_SET_EDGE_TRIGGER(irq + i);
> > +                } else {
> > +                    GIC_CLEAR_EDGE_TRIGGER(irq + i);
> > +                }
> >              }
> >          }
> >      } else if (offset < 0xf10) {
> 
> Actually, this looks a bit weird given that you do set the model bit,
> which should probably be treated as WI/RAZ for a GICv2 emulation, but
> you don't set the edge trigger bit for them.

I've addressed that in a separate patch now. However, I'm not sure got
the revision check right. Comments appreciated!

> I think a cleaner fix might be to to just change the existing check from
> (irq < GIC_INTERNAL) to (irq < GIT_NR_SGIS), then you also don't need
> the next patch.

Ok, new series sent out.



Adam
-- 
Adam                 adam@os.inf.tu-dresden.de
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

[Qemu-devel] [PATCH 3/3] arm_gic: GICD_ICFGR: Do not force edge-triggered PPIs

[Adam Lackorzynski]

Using GICD_ICFGR for PPIs forces PPIs to edge-triggered mode, although they
have been initialised to level-triggered. This affects all interrupts
covered by the write access. Change the handling of PPIs to not force a
specific mode. It is implementation defined whether setting the mode of PPIs
is supported.

Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
---
 hw/intc/arm_gic.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index cd6e6ea..066a7f2 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -558,8 +558,6 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
         irq = (offset - 0xc00) * 4 + GIC_BASE_IRQ;
         if (irq >= s->num_irq)
             goto bad_reg;
-        if (irq < GIC_INTERNAL)
-            value |= 0xaa;
         for (i = 0; i < 4; i++) {
             if (value & (1 << (i * 2))) {
                 GIC_SET_MODEL(irq + i);
-- 
2.0.1

Re: [Qemu-devel] [PATCH 0/3] arm_gic: Improve handling of GICD_ICFGR

[Peter Maydell]

On 3 August 2014 09:53, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
> Hi,
>
> the following three patches address the behavior of the GICD_ICFGR register
> in the ARM GIC.
>
> Adam Lackorzynski (3):
>   arm_gic: Fix read of GICD_ICFGR
>   arm_gic: SGIs for GICD_ICFGR are WI
>   arm_gic: GICD_ICFGR: Do not force edge-triggered PPIs
>
>  hw/intc/arm_gic.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)

Christoffer, did you want to review these? (I'll have a look through
them too shortly.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/3] arm_gic: Improve handling of GICD_ICFGR

[Christoffer Dall]

On 3 August 2014 15:21, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 August 2014 09:53, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
>> Hi,
>>
>> the following three patches address the behavior of the GICD_ICFGR register
>> in the ARM GIC.
>>
>> Adam Lackorzynski (3):
>>   arm_gic: Fix read of GICD_ICFGR
>>   arm_gic: SGIs for GICD_ICFGR are WI
>>   arm_gic: GICD_ICFGR: Do not force edge-triggered PPIs
>>
>>  hw/intc/arm_gic.c | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> Christoffer, did you want to review these? (I'll have a look through
> them too shortly.)
>
Yeah, I'll have a look some time this week if that's timely enough?

-Christoffer

Re: [Qemu-devel] [PATCH 0/3] arm_gic: Improve handling of GICD_ICFGR

[Adam Lackorzynski]

Hi,

On Sun Aug 03, 2014 at 21:36:21 +0200, Christoffer Dall wrote:
> On 3 August 2014 15:21, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 3 August 2014 09:53, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
> >> Hi,
> >>
> >> the following three patches address the behavior of the GICD_ICFGR register
> >> in the ARM GIC.
> >>
> >> Adam Lackorzynski (3):
> >>   arm_gic: Fix read of GICD_ICFGR
> >>   arm_gic: SGIs for GICD_ICFGR are WI
> >>   arm_gic: GICD_ICFGR: Do not force edge-triggered PPIs
> >>
> >>  hw/intc/arm_gic.c | 15 ++++++++-------
> >>  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > Christoffer, did you want to review these? (I'll have a look through
> > them too shortly.)
> >
> Yeah, I'll have a look some time this week if that's timely enough?

Any comment appreciated.



Thanks,
Adam
-- 
Adam                 adam@os.inf.tu-dresden.de
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

Re: [Qemu-devel] [PATCH 0/3] arm_gic: Improve handling of GICD_ICFGR

[Christoffer Dall]

On Mon, Aug 11, 2014 at 7:26 PM, Adam Lackorzynski
<adam@os.inf.tu-dresden.de> wrote:
> Hi,
>
> On Sun Aug 03, 2014 at 21:36:21 +0200, Christoffer Dall wrote:
>> On 3 August 2014 15:21, Peter Maydell <peter.maydell@linaro.org> wrote:
>> > On 3 August 2014 09:53, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
>> >> Hi,
>> >>
>> >> the following three patches address the behavior of the GICD_ICFGR register
>> >> in the ARM GIC.
>> >>
>> >> Adam Lackorzynski (3):
>> >>   arm_gic: Fix read of GICD_ICFGR
>> >>   arm_gic: SGIs for GICD_ICFGR are WI
>> >>   arm_gic: GICD_ICFGR: Do not force edge-triggered PPIs
>> >>
>> >>  hw/intc/arm_gic.c | 15 ++++++++-------
>> >>  1 file changed, 8 insertions(+), 7 deletions(-)
>> >
>> > Christoffer, did you want to review these? (I'll have a look through
>> > them too shortly.)
>> >
>> Yeah, I'll have a look some time this week if that's timely enough?
>
> Any comment appreciated.
>
>
Hi Adam,

I'll try to take a look tomorrow or Wednesday.

-Christoffer