On 17 February 2018 at 18:46, Eric Auger <eric.au...@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.au...@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
