| Message ID | 1407056027-7522-3-git-send-email-adam@os.inf.tu-dresden.de |
|---|---|
| State | New |
| Headers | show |
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 > >
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
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
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
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) {
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(-)