qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v7 06/20] hw/arm/smmuv3: Wired IRQ and GERROR he


From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH v7 06/20] hw/arm/smmuv3: Wired IRQ and GERROR helpers
Date: Mon, 9 Oct 2017 18:01:49 +0100

On 1 September 2017 at 18:21, Eric Auger <address@hidden> 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.
>
> Signed-off-by: Eric Auger <address@hidden>
>
> ---
>
> Is CMD_SYNC interrupt enabled somewhere?
> ---
>  hw/arm/smmuv3-internal.h | 20 ++++++++++++++++++
>  hw/arm/smmuv3.c          | 55 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/arm/trace-events      |  2 ++
>  3 files changed, 77 insertions(+)
>
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index 488acc8..2b44ee2 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -198,4 +198,24 @@ static inline int smmu_enabled(SMMUV3State *s)
>      return smmu_read32_reg(s, SMMU_REG_CR0) & SMMU_CR0_SMMU_ENABLE;
>  }
>
> +/*****************************
> + * Interrupts
> + *****************************/
> +
> +#define smmu_evt_irq_enabled(s)                   \
> +    (smmu_read64_reg(s, SMMU_REG_IRQ_CTRL) & SMMU_IRQ_CTRL_EVENT_EN)
> +#define smmu_gerror_irq_enabled(s)                  \
> +    (smmu_read64_reg(s, SMMU_REG_IRQ_CTRL) & SMMU_IRQ_CTRL_GERROR_EN)
> +#define smmu_pri_irq_enabled(s)                 \
> +    (smmu_read64_reg(s, SMMU_REG_IRQ_CTRL) & SMMU_IRQ_CTRL_PRI_EN)
> +
> +#define SMMU_PENDING_GERRORS(s) \
> +    (smmu_read32_reg(s, SMMU_REG_GERROR) ^ \
> +     smmu_read32_reg(s, SMMU_REG_GERRORN))

Hiding this XOR inside a macro is very confusing.

> +
> +#define SMMU_CMDQ_ERR(s) (SMMU_PENDING_GERRORS(s) & SMMU_GERROR_CMDQ)
> +
> +void smmuv3_irq_trigger(SMMUV3State *s, SMMUIrq irq, uint32_t gerror_val);
> +void smmuv3_write_gerrorn(SMMUV3State *s, uint32_t gerrorn);
> +
>  #endif
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 0a7cd1c..468134f 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -29,6 +29,61 @@
>  #include "hw/arm/smmuv3.h"
>  #include "smmuv3-internal.h"
>
> +/**
> + * smmuv3_irq_trigger - pulse @irq if enabled and update
> + * GERROR register in case of GERROR interrupt
> + *
> + * @irq: irq type
> + * @gerror: gerror new value, only relevant if @irq is GERROR
> + */
> +void smmuv3_irq_trigger(SMMUV3State *s, SMMUIrq irq, uint32_t gerror_val)
> +{
> +    uint32_t pending_gerrors = SMMU_PENDING_GERRORS(s);
> +    bool pulse = false;
> +
> +    switch (irq) {
> +    case SMMU_IRQ_EVTQ:
> +        pulse = smmu_evt_irq_enabled(s);
> +        break;
> +    case SMMU_IRQ_PRIQ:
> +        pulse = smmu_pri_irq_enabled(s);
> +        break;
> +    case SMMU_IRQ_CMD_SYNC:
> +        pulse = true;
> +        break;
> +    case SMMU_IRQ_GERROR:
> +    {
> +        /* don't toggle an already pending error */
> +        bool new_gerrors = ~pending_gerrors & gerror_val;
> +        uint32_t gerror = smmu_read32_reg(s, SMMU_REG_GERROR);
> +
> +        smmu_write32_reg(s, SMMU_REG_GERROR, gerror | new_gerrors);

The SMMU toggles GERROR bits to indicate that they have been
set, it doesn't just set them to 1.

> +
> +        /* pulse the GERROR irq only if all fields were acked */
> +        pulse = smmu_gerror_irq_enabled(s) && !pending_gerrors;
> +        break;
> +    }
> +    }
> +    if (pulse) {
> +            trace_smmuv3_irq_trigger(irq,
> +                                     smmu_read32_reg(s, SMMU_REG_GERROR),
> +                                     SMMU_PENDING_GERRORS(s));
> +            qemu_irq_pulse(s->irq[irq]);

qemu_irq_pulse() is very rarely the right thing for an interrupt
line, but it looks like it's right here (per spec 3.18.2).

> +    }
> +}
> +
> +void smmuv3_write_gerrorn(SMMUV3State *s, uint32_t gerrorn)
> +{
> +    uint32_t pending_gerrors = SMMU_PENDING_GERRORS(s);
> +    uint32_t sanitized;
> +
> +    /* Make sure SW does not toggle irqs that are not active */

If it does, this is CONSTRAINED UNPREDICTABLE. That is worth
remarking in either a comment or a LOG_GUEST_ERROR warning.

> +    sanitized = gerrorn & pending_gerrors;

This isn't right -- if you want to sanitize the result
being written then you need to make it only change bits
that are in pending_errors, but this expression allows
the guest to write a 0 to a field that is for a non-pending
error. (Or you could just not sanitize at all, that's allowed
by the CONSTRAINED UNPREDICTABLE.)

> +
> +    smmu_write32_reg(s, SMMU_REG_GERRORN, sanitized);
> +    trace_smmuv3_write_gerrorn(gerrorn, sanitized, SMMU_PENDING_GERRORS(s));
> +}
> +
>  static void smmuv3_init_regs(SMMUV3State *s)
>  {
>      uint32_t data =
> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index 8affbf7..c1ce8eb 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -17,3 +17,5 @@ smmu_set_translated_address(hwaddr iova, hwaddr pa) "iova = 
> 0x%"PRIx64" -> pa =
>
>  #hw/arm/smmuv3.c
>  smmuv3_read_mmio(hwaddr addr, uint64_t val, unsigned size) "addr: 
> 0x%"PRIx64" val:0x%"PRIx64" size: 0x%x"
> +smmuv3_irq_trigger(int irq, uint32_t gerror, uint32_t pending) "irq=%d 
> gerror=0x%x pending gerrors=0x%x"
> +smmuv3_write_gerrorn(uint32_t gerrorn, uint32_t sanitized, uint32_t pending) 
> "gerrorn=0x%x sanitized=0x%x pending=0x%x"
> --
> 2.5.5

thanks
-- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]