| Message ID | 1518893216-9983-6-git-send-email-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ARM SMMUv3 Emulation Support | expand |
On 17 February 2018 at 18:46, Eric Auger <eric.auger@redhat.com> wrote: > We introduce some helpers to handle wired IRQs and especially > GERROR interrupt. SMMU writes GERROR register on GERROR event > and SW acks GERROR interrupts by setting GERRORn. > > The Wired interrupts are edge sensitive hence the pulse usage. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > --- > > v7 -> v8: > - remove SMMU_PENDING_GERRORS macro > - properly toggle gerror > - properly sanitize gerrorn write > --- > hw/arm/smmuv3-internal.h | 10 ++++++++ > hw/arm/smmuv3.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ > hw/arm/trace-events | 3 +++ > 3 files changed, 77 insertions(+) > > diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h > index 5be8303..40b39a1 100644 > --- a/hw/arm/smmuv3-internal.h > +++ b/hw/arm/smmuv3-internal.h > @@ -152,4 +152,14 @@ static inline uint64_t smmu_read64(uint64_t r, unsigned offset, > return extract64(r, offset << 3, 32); > } > > +/* Interrupts */ > + > +#define smmuv3_eventq_irq_enabled(s) \ > + (FIELD_EX32(s->irq_ctrl, IRQ_CTRL, EVENTQ_IRQEN)) > +#define smmuv3_gerror_irq_enabled(s) \ > + (FIELD_EX32(s->irq_ctrl, IRQ_CTRL, GERROR_IRQEN)) These are only ever used in smmuv3.c, so you can just move them to there (and make them inline functions, ideally). > + > +void smmuv3_trigger_irq(SMMUv3State *s, SMMUIrq irq, uint32_t gerror_mask); > +void smmuv3_write_gerrorn(SMMUv3State *s, uint32_t gerrorn); I guess these are global to avoid the compiler complaining about unused static functions at this point? If so, add a comment saying so, and flip them back to being static functions when their callers get added. (Or just add the callers here, if they're not too complicated.) > + > #endif > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > index dc03c9e..8779d3f 100644 > --- a/hw/arm/smmuv3.c > +++ b/hw/arm/smmuv3.c > @@ -30,6 +30,70 @@ > #include "hw/arm/smmuv3.h" > #include "smmuv3-internal.h" > > +/** > + * smmuv3_trigger_irq - pulse @irq if enabled and update > + * GERROR register in case of GERROR interrupt > + * > + * @irq: irq type > + * @gerror_mask: mask of gerrors to toggle (relevant if @irq is GERROR) > + */ > +void smmuv3_trigger_irq(SMMUv3State *s, SMMUIrq irq, uint32_t gerror_mask) > +{ > + > + bool pulse = false; > + > + switch (irq) { > + case SMMU_IRQ_EVTQ: > + pulse = smmuv3_eventq_irq_enabled(s); > + break; > + case SMMU_IRQ_PRIQ: > + error_setg(&error_fatal, "PRI not supported"); This should either assert() if it would be a bug in the rest of the smmu code, or LOG_UNIMP if the guest can trigger it. > + break; > + case SMMU_IRQ_CMD_SYNC: > + pulse = true; > + break; > + case SMMU_IRQ_GERROR: > + { > + uint32_t pending = s->gerror ^ s->gerrorn; > + uint32_t new_gerrors = ~pending & gerror_mask; > + > + if (!new_gerrors) { > + /* only toggle non pending errors */ > + return; > + } > + s->gerror ^= new_gerrors; > + trace_smmuv3_write_gerror(new_gerrors, s->gerror); > + > + /* pulse the GERROR irq only if all previous gerrors were acked */ It's not entirely clear to me that this is correct; should we generate only one pulse if the implementation raises error A, and then later raises error B before software acknowledges A ? There's some language in 3.18 about the SMMU implementation being able to coalesce events and identical interrupts, but I think that would mean that we could skip raising the first pulse for error A if error B arrived sufficiently quickly after it. (Not something we're going to care about for a s/w model.) I think the right behaviour is probably that we should pulse the interrupt if there are any new gerrors, which is to say to drop this !pending test: > + pulse = smmuv3_gerror_irq_enabled(s) && !pending; > + break; > + } > + } > + if (pulse) { > + trace_smmuv3_trigger_irq(irq); > + qemu_irq_pulse(s->irq[irq]); > + } > +} > + > +void smmuv3_write_gerrorn(SMMUv3State *s, uint32_t new_gerrorn) > +{ > + uint32_t pending = s->gerror ^ s->gerrorn; > + uint32_t toggled = s->gerrorn ^ new_gerrorn; > + uint32_t acked; > + > + if (toggled & ~pending) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "guest toggles non pending errors = 0x%x\n", > + toggled & ~pending); > + } > + > + /* Make sure SW does not toggle irqs that are not active */ > + acked = toggled & pending; > + s->gerrorn ^= acked; > + I don't think this behaviour is correct. From the hardware's perspective, we should just take the value the user writes to SMMU_GERRORN and put it in the register (and update the status of the irq accordingly). It is CONSTRAINED UNPREDICTABLE whether we actually raise an interrupt if the guest toggles a field that corresponds to an inactive error, so we should just do whatever is easiest. > + trace_smmuv3_write_gerrorn(acked, s->gerrorn); > +} > + > static void smmuv3_init_regs(SMMUv3State *s) > { > /** > diff --git a/hw/arm/trace-events b/hw/arm/trace-events > index 64d2b9b..2ddae40 100644 > --- a/hw/arm/trace-events > +++ b/hw/arm/trace-events > @@ -15,3 +15,6 @@ smmu_get_pte(uint64_t baseaddr, int index, uint64_t pteaddr, uint64_t pte) "base > > #hw/arm/smmuv3.c > smmuv3_read_mmio(hwaddr addr, uint64_t val, unsigned size) "addr: 0x%"PRIx64" val:0x%"PRIx64" size: 0x%x" > +smmuv3_trigger_irq(int irq) "irq=%d" > +smmuv3_write_gerror(uint32_t toggled, uint32_t gerror) "toggled=0x%x, new gerror=0x%x" > +smmuv3_write_gerrorn(uint32_t acked, uint32_t gerrorn) "acked=0x%x, new gerrorn=0x%x" Capitalizing names of registers like GERROR in trace messages would make them match the convention in the SMMUv3 spec. > -- > 2.5.5 > thanks -- PMM
Hi Peter, On 08/03/18 18:49, Peter Maydell wrote: > On 17 February 2018 at 18:46, Eric Auger <eric.auger@redhat.com> wrote: >> We introduce some helpers to handle wired IRQs and especially >> GERROR interrupt. SMMU writes GERROR register on GERROR event >> and SW acks GERROR interrupts by setting GERRORn. >> >> The Wired interrupts are edge sensitive hence the pulse usage. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> >> --- >> >> v7 -> v8: >> - remove SMMU_PENDING_GERRORS macro >> - properly toggle gerror >> - properly sanitize gerrorn write >> --- >> hw/arm/smmuv3-internal.h | 10 ++++++++ >> hw/arm/smmuv3.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ >> hw/arm/trace-events | 3 +++ >> 3 files changed, 77 insertions(+) >> >> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h >> index 5be8303..40b39a1 100644 >> --- a/hw/arm/smmuv3-internal.h >> +++ b/hw/arm/smmuv3-internal.h >> @@ -152,4 +152,14 @@ static inline uint64_t smmu_read64(uint64_t r, unsigned offset, >> return extract64(r, offset << 3, 32); >> } >> >> +/* Interrupts */ >> + >> +#define smmuv3_eventq_irq_enabled(s) \ >> + (FIELD_EX32(s->irq_ctrl, IRQ_CTRL, EVENTQ_IRQEN)) >> +#define smmuv3_gerror_irq_enabled(s) \ >> + (FIELD_EX32(s->irq_ctrl, IRQ_CTRL, GERROR_IRQEN)) > > These are only ever used in smmuv3.c, so you can just move them > to there (and make them inline functions, ideally). smmuv3-internal.h contains helpers, macros which are only used by smmuv3.c . I though this could avoid putting too much code in smmuv3.c and would help in the readability. what is the exact benefit of transforming those into inline functions instead of macros. Not meaning I don't want to take this into account but to improve my coding style ;-) > >> + >> +void smmuv3_trigger_irq(SMMUv3State *s, SMMUIrq irq, uint32_t gerror_mask); >> +void smmuv3_write_gerrorn(SMMUv3State *s, uint32_t gerrorn); > > I guess these are global to avoid the compiler complaining about > unused static functions at this point? If so, add a comment > saying so, and flip them back to being static functions when > their callers get added. (Or just add the callers here, if they're > not too complicated.) Yes they becomes static in "hw/arm/smmuv3: Implement MMIO write operations". Added a comment > >> + >> #endif >> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c >> index dc03c9e..8779d3f 100644 >> --- a/hw/arm/smmuv3.c >> +++ b/hw/arm/smmuv3.c >> @@ -30,6 +30,70 @@ >> #include "hw/arm/smmuv3.h" >> #include "smmuv3-internal.h" >> >> +/** >> + * smmuv3_trigger_irq - pulse @irq if enabled and update >> + * GERROR register in case of GERROR interrupt >> + * >> + * @irq: irq type >> + * @gerror_mask: mask of gerrors to toggle (relevant if @irq is GERROR) >> + */ >> +void smmuv3_trigger_irq(SMMUv3State *s, SMMUIrq irq, uint32_t gerror_mask) >> +{ >> + >> + bool pulse = false; >> + >> + switch (irq) { >> + case SMMU_IRQ_EVTQ: >> + pulse = smmuv3_eventq_irq_enabled(s); >> + break; >> + case SMMU_IRQ_PRIQ: >> + error_setg(&error_fatal, "PRI not supported"); > > This should either assert() if it would be a bug in the rest > of the smmu code, or LOG_UNIMP if the guest can trigger it. replaced by LOG_UNIMP > >> + break; >> + case SMMU_IRQ_CMD_SYNC: >> + pulse = true; >> + break; >> + case SMMU_IRQ_GERROR: >> + { >> + uint32_t pending = s->gerror ^ s->gerrorn; >> + uint32_t new_gerrors = ~pending & gerror_mask; >> + >> + if (!new_gerrors) { >> + /* only toggle non pending errors */ >> + return; >> + } >> + s->gerror ^= new_gerrors; >> + trace_smmuv3_write_gerror(new_gerrors, s->gerror); >> + >> + /* pulse the GERROR irq only if all previous gerrors were acked */ > > It's not entirely clear to me that this is correct; should > we generate only one pulse if the implementation raises error A, > and then later raises error B before software acknowledges A ? > There's some language in 3.18 about the SMMU implementation > being able to coalesce events and identical interrupts, but > I think that would mean that we could skip raising the first > pulse for error A if error B arrived sufficiently quickly after it. > (Not something we're going to care about for a s/w model.) I don't implement event merging atm. > > I think the right behaviour is probably that we should pulse > the interrupt if there are any new gerrors, which is to > say to drop this !pending test: I agree with your interpretation. > >> + pulse = smmuv3_gerror_irq_enabled(s) && !pending; >> + break; >> + } >> + } >> + if (pulse) { >> + trace_smmuv3_trigger_irq(irq); >> + qemu_irq_pulse(s->irq[irq]); >> + } >> +} >> + >> +void smmuv3_write_gerrorn(SMMUv3State *s, uint32_t new_gerrorn) >> +{ >> + uint32_t pending = s->gerror ^ s->gerrorn; >> + uint32_t toggled = s->gerrorn ^ new_gerrorn; >> + uint32_t acked; >> + >> + if (toggled & ~pending) { >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "guest toggles non pending errors = 0x%x\n", >> + toggled & ~pending); >> + } >> + >> + /* Make sure SW does not toggle irqs that are not active */ >> + acked = toggled & pending; >> + s->gerrorn ^= acked; >> + > > I don't think this behaviour is correct. From the hardware's > perspective, we should just take the value the user writes > to SMMU_GERRORN and put it in the register (and update the > status of the irq accordingly). OK > > It is CONSTRAINED UNPREDICTABLE whether we actually raise an > interrupt if the guest toggles a field that corresponds to an > inactive error, so we should just do whatever is easiest. OK: nothing except reporting a LOG_GUEST_ERROR ;-) Thanks Eric > >> + trace_smmuv3_write_gerrorn(acked, s->gerrorn); >> +} >> + >> static void smmuv3_init_regs(SMMUv3State *s) >> { >> /** >> diff --git a/hw/arm/trace-events b/hw/arm/trace-events >> index 64d2b9b..2ddae40 100644 >> --- a/hw/arm/trace-events >> +++ b/hw/arm/trace-events >> @@ -15,3 +15,6 @@ smmu_get_pte(uint64_t baseaddr, int index, uint64_t pteaddr, uint64_t pte) "base >> >> #hw/arm/smmuv3.c >> smmuv3_read_mmio(hwaddr addr, uint64_t val, unsigned size) "addr: 0x%"PRIx64" val:0x%"PRIx64" size: 0x%x" >> +smmuv3_trigger_irq(int irq) "irq=%d" >> +smmuv3_write_gerror(uint32_t toggled, uint32_t gerror) "toggled=0x%x, new gerror=0x%x" >> +smmuv3_write_gerrorn(uint32_t acked, uint32_t gerrorn) "acked=0x%x, new gerrorn=0x%x" > > Capitalizing names of registers like GERROR in trace messages would > make them match the convention in the SMMUv3 spec. done Thanks Eric > >> -- >> 2.5.5 >> > > thanks > -- PMM >
On 9 March 2018 at 14:03, Auger Eric <eric.auger@redhat.com> wrote: > On 08/03/18 18:49, Peter Maydell wrote: >>> +#define smmuv3_eventq_irq_enabled(s) \ >>> + (FIELD_EX32(s->irq_ctrl, IRQ_CTRL, EVENTQ_IRQEN)) >>> +#define smmuv3_gerror_irq_enabled(s) \ >>> + (FIELD_EX32(s->irq_ctrl, IRQ_CTRL, GERROR_IRQEN)) >> >> These are only ever used in smmuv3.c, so you can just move them >> to there (and make them inline functions, ideally). > smmuv3-internal.h contains helpers, macros which are only used by > smmuv3.c . I though this could avoid putting too much code in smmuv3.c > and would help in the readability. > > what is the exact benefit of transforming those into inline functions > instead of macros. Not meaning I don't want to take this into account > but to improve my coding style ;-) You get the benefit of type checking (and it self-documents that the macros want to be passed an SMMUv3State*). You don't have to worry about trying to write your macro to not evaluate arguments multiple times. These are one-liners so they're fairly easy to read, but when you get to 3 or 4 lines you end up with a lot of '\' lines and the inline function is I think more clearly better. I prefer to save macros for cases where you really need a macro and can't get the same effect with a function. thanks -- PMM
Hi Peter, On 09/03/18 15:18, Peter Maydell wrote: > On 9 March 2018 at 14:03, Auger Eric <eric.auger@redhat.com> wrote: >> On 08/03/18 18:49, Peter Maydell wrote: >>>> +#define smmuv3_eventq_irq_enabled(s) \ >>>> + (FIELD_EX32(s->irq_ctrl, IRQ_CTRL, EVENTQ_IRQEN)) >>>> +#define smmuv3_gerror_irq_enabled(s) \ >>>> + (FIELD_EX32(s->irq_ctrl, IRQ_CTRL, GERROR_IRQEN)) >>> >>> These are only ever used in smmuv3.c, so you can just move them >>> to there (and make them inline functions, ideally). >> smmuv3-internal.h contains helpers, macros which are only used by >> smmuv3.c . I though this could avoid putting too much code in smmuv3.c >> and would help in the readability. >> >> what is the exact benefit of transforming those into inline functions >> instead of macros. Not meaning I don't want to take this into account >> but to improve my coding style ;-) > > You get the benefit of type checking (and it self-documents that > the macros want to be passed an SMMUv3State*). You don't have to > worry about trying to write your macro to not evaluate arguments multiple > times. These are one-liners so they're fairly easy to read, > but when you get to 3 or 4 lines you end up with a lot of '\' > lines and the inline function is I think more clearly better. > I prefer to save macros for cases where you really need a macro > and can't get the same effect with a function. OK. Thank you for the explanation Eric > > thanks > -- PMM >
diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h index 5be8303..40b39a1 100644 --- a/hw/arm/smmuv3-internal.h +++ b/hw/arm/smmuv3-internal.h @@ -152,4 +152,14 @@ static inline uint64_t smmu_read64(uint64_t r, unsigned offset, return extract64(r, offset << 3, 32); } +/* Interrupts */ + +#define smmuv3_eventq_irq_enabled(s) \ + (FIELD_EX32(s->irq_ctrl, IRQ_CTRL, EVENTQ_IRQEN)) +#define smmuv3_gerror_irq_enabled(s) \ + (FIELD_EX32(s->irq_ctrl, IRQ_CTRL, GERROR_IRQEN)) + +void smmuv3_trigger_irq(SMMUv3State *s, SMMUIrq irq, uint32_t gerror_mask); +void smmuv3_write_gerrorn(SMMUv3State *s, uint32_t gerrorn); + #endif diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c index dc03c9e..8779d3f 100644 --- a/hw/arm/smmuv3.c +++ b/hw/arm/smmuv3.c @@ -30,6 +30,70 @@ #include "hw/arm/smmuv3.h" #include "smmuv3-internal.h" +/** + * smmuv3_trigger_irq - pulse @irq if enabled and update + * GERROR register in case of GERROR interrupt + * + * @irq: irq type + * @gerror_mask: mask of gerrors to toggle (relevant if @irq is GERROR) + */ +void smmuv3_trigger_irq(SMMUv3State *s, SMMUIrq irq, uint32_t gerror_mask) +{ + + bool pulse = false; + + switch (irq) { + case SMMU_IRQ_EVTQ: + pulse = smmuv3_eventq_irq_enabled(s); + break; + case SMMU_IRQ_PRIQ: + error_setg(&error_fatal, "PRI not supported"); + break; + case SMMU_IRQ_CMD_SYNC: + pulse = true; + break; + case SMMU_IRQ_GERROR: + { + uint32_t pending = s->gerror ^ s->gerrorn; + uint32_t new_gerrors = ~pending & gerror_mask; + + if (!new_gerrors) { + /* only toggle non pending errors */ + return; + } + s->gerror ^= new_gerrors; + trace_smmuv3_write_gerror(new_gerrors, s->gerror); + + /* pulse the GERROR irq only if all previous gerrors were acked */ + pulse = smmuv3_gerror_irq_enabled(s) && !pending; + break; + } + } + if (pulse) { + trace_smmuv3_trigger_irq(irq); + qemu_irq_pulse(s->irq[irq]); + } +} + +void smmuv3_write_gerrorn(SMMUv3State *s, uint32_t new_gerrorn) +{ + uint32_t pending = s->gerror ^ s->gerrorn; + uint32_t toggled = s->gerrorn ^ new_gerrorn; + uint32_t acked; + + if (toggled & ~pending) { + qemu_log_mask(LOG_GUEST_ERROR, + "guest toggles non pending errors = 0x%x\n", + toggled & ~pending); + } + + /* Make sure SW does not toggle irqs that are not active */ + acked = toggled & pending; + s->gerrorn ^= acked; + + trace_smmuv3_write_gerrorn(acked, s->gerrorn); +} + static void smmuv3_init_regs(SMMUv3State *s) { /** diff --git a/hw/arm/trace-events b/hw/arm/trace-events index 64d2b9b..2ddae40 100644 --- a/hw/arm/trace-events +++ b/hw/arm/trace-events @@ -15,3 +15,6 @@ smmu_get_pte(uint64_t baseaddr, int index, uint64_t pteaddr, uint64_t pte) "base #hw/arm/smmuv3.c smmuv3_read_mmio(hwaddr addr, uint64_t val, unsigned size) "addr: 0x%"PRIx64" val:0x%"PRIx64" size: 0x%x" +smmuv3_trigger_irq(int irq) "irq=%d" +smmuv3_write_gerror(uint32_t toggled, uint32_t gerror) "toggled=0x%x, new gerror=0x%x" +smmuv3_write_gerrorn(uint32_t acked, uint32_t gerrorn) "acked=0x%x, new gerrorn=0x%x"
We introduce some helpers to handle wired IRQs and especially GERROR interrupt. SMMU writes GERROR register on GERROR event and SW acks GERROR interrupts by setting GERRORn. The Wired interrupts are edge sensitive hence the pulse usage. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- v7 -> v8: - remove SMMU_PENDING_GERRORS macro - properly toggle gerror - properly sanitize gerrorn write --- hw/arm/smmuv3-internal.h | 10 ++++++++ hw/arm/smmuv3.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ hw/arm/trace-events | 3 +++ 3 files changed, 77 insertions(+)