diff mbox series

[2/2] hw/arm/smmuv3: fix eventq recording and IRQ triggerring

Message ID 20180921070138.10114-3-eric.auger@redhat.com (mailing list archive)
State New, archived
Headers show
Series ARM SMMUv3: Fix event queue handling and memory region names | expand

Commit Message

Eric Auger Sept. 21, 2018, 7:01 a.m. UTC 
The event queue management is broken today. Event records
are not properly written as EVT_SET_* macro was not updating
the actual event record. Also the event queue interrupt
is not correctly triggered.

Fixes: bb981004eaf4 ("hw/arm/smmuv3: Event queue recording helper")
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/smmuv3-internal.h | 26 +++++++++++++-------------
 hw/arm/smmuv3.c          |  2 +-
 2 files changed, 14 insertions(+), 14 deletions(-)

Comments

Peter Maydell Sept. 25, 2018, 10:25 a.m. UTC | #1 
On 21 September 2018 at 08:01, Eric Auger <eric.auger@redhat.com> wrote:
> The event queue management is broken today. Event records
> are not properly written as EVT_SET_* macro was not updating
> the actual event record. Also the event queue interrupt
> is not correctly triggered.
>
> Fixes: bb981004eaf4 ("hw/arm/smmuv3: Event queue recording helper")
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/smmuv3-internal.h | 26 +++++++++++++-------------
>  hw/arm/smmuv3.c          |  2 +-
>  2 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index bab25d640e..19540f8f41 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -442,17 +442,17 @@ typedef struct SMMUEventInfo {
>
>  #define EVT_Q_OVERFLOW        (1 << 31)
>
> -#define EVT_SET_TYPE(x, v)              deposit32((x)->word[0], 0 , 8 , v)
> -#define EVT_SET_SSV(x, v)               deposit32((x)->word[0], 11, 1 , v)
> -#define EVT_SET_SSID(x, v)              deposit32((x)->word[0], 12, 20, v)
> -#define EVT_SET_SID(x, v)               ((x)->word[1] = v)
> -#define EVT_SET_STAG(x, v)              deposit32((x)->word[2], 0 , 16, v)
> -#define EVT_SET_STALL(x, v)             deposit32((x)->word[2], 31, 1 , v)
> -#define EVT_SET_PNU(x, v)               deposit32((x)->word[3], 1 , 1 , v)
> -#define EVT_SET_IND(x, v)               deposit32((x)->word[3], 2 , 1 , v)
> -#define EVT_SET_RNW(x, v)               deposit32((x)->word[3], 3 , 1 , v)
> -#define EVT_SET_S2(x, v)                deposit32((x)->word[3], 7 , 1 , v)
> -#define EVT_SET_CLASS(x, v)             deposit32((x)->word[3], 8 , 2 , v)
> +#define EVT_SET_TYPE(x, v)  ((x)->word[0] = deposit32((x)->word[0], 0 , 8 , v))
> +#define EVT_SET_SSV(x, v)   ((x)->word[0] = deposit32((x)->word[0], 11, 1 , v))
> +#define EVT_SET_SSID(x, v)  ((x)->word[0] = deposit32((x)->word[0], 12, 20, v))
> +#define EVT_SET_SID(x, v)   ((x)->word[1] = v)
> +#define EVT_SET_STAG(x, v)  ((x)->word[2] = deposit32((x)->word[2], 0 , 16, v))
> +#define EVT_SET_STALL(x, v) ((x)->word[2] = deposit32((x)->word[2], 31, 1 , v))
> +#define EVT_SET_PNU(x, v)   ((x)->word[3] = deposit32((x)->word[3], 1 , 1 , v))
> +#define EVT_SET_IND(x, v)   ((x)->word[3] = deposit32((x)->word[3], 2 , 1 , v))
> +#define EVT_SET_RNW(x, v)   ((x)->word[3] = deposit32((x)->word[3], 3 , 1 , v))
> +#define EVT_SET_S2(x, v)    ((x)->word[3] = deposit32((x)->word[3], 7 , 1 , v))
> +#define EVT_SET_CLASS(x, v) ((x)->word[3] = deposit32((x)->word[3], 8 , 2 , v))


Oops. I'm a bit embarrassed I didn't notice that in code review...

thanks
-- PMM
Eric Auger Sept. 25, 2018, 12:07 p.m. UTC | #2 
Hi Peter,

On 9/25/18 12:25 PM, Peter Maydell wrote:
> On 21 September 2018 at 08:01, Eric Auger <eric.auger@redhat.com> wrote:
>> The event queue management is broken today. Event records
>> are not properly written as EVT_SET_* macro was not updating
>> the actual event record. Also the event queue interrupt
>> is not correctly triggered.
>>
>> Fixes: bb981004eaf4 ("hw/arm/smmuv3: Event queue recording helper")
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/arm/smmuv3-internal.h | 26 +++++++++++++-------------
>>  hw/arm/smmuv3.c          |  2 +-
>>  2 files changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
>> index bab25d640e..19540f8f41 100644
>> --- a/hw/arm/smmuv3-internal.h
>> +++ b/hw/arm/smmuv3-internal.h
>> @@ -442,17 +442,17 @@ typedef struct SMMUEventInfo {
>>
>>  #define EVT_Q_OVERFLOW        (1 << 31)
>>
>> -#define EVT_SET_TYPE(x, v)              deposit32((x)->word[0], 0 , 8 , v)
>> -#define EVT_SET_SSV(x, v)               deposit32((x)->word[0], 11, 1 , v)
>> -#define EVT_SET_SSID(x, v)              deposit32((x)->word[0], 12, 20, v)
>> -#define EVT_SET_SID(x, v)               ((x)->word[1] = v)
>> -#define EVT_SET_STAG(x, v)              deposit32((x)->word[2], 0 , 16, v)
>> -#define EVT_SET_STALL(x, v)             deposit32((x)->word[2], 31, 1 , v)
>> -#define EVT_SET_PNU(x, v)               deposit32((x)->word[3], 1 , 1 , v)
>> -#define EVT_SET_IND(x, v)               deposit32((x)->word[3], 2 , 1 , v)
>> -#define EVT_SET_RNW(x, v)               deposit32((x)->word[3], 3 , 1 , v)
>> -#define EVT_SET_S2(x, v)                deposit32((x)->word[3], 7 , 1 , v)
>> -#define EVT_SET_CLASS(x, v)             deposit32((x)->word[3], 8 , 2 , v)
>> +#define EVT_SET_TYPE(x, v)  ((x)->word[0] = deposit32((x)->word[0], 0 , 8 , v))
>> +#define EVT_SET_SSV(x, v)   ((x)->word[0] = deposit32((x)->word[0], 11, 1 , v))
>> +#define EVT_SET_SSID(x, v)  ((x)->word[0] = deposit32((x)->word[0], 12, 20, v))
>> +#define EVT_SET_SID(x, v)   ((x)->word[1] = v)
>> +#define EVT_SET_STAG(x, v)  ((x)->word[2] = deposit32((x)->word[2], 0 , 16, v))
>> +#define EVT_SET_STALL(x, v) ((x)->word[2] = deposit32((x)->word[2], 31, 1 , v))
>> +#define EVT_SET_PNU(x, v)   ((x)->word[3] = deposit32((x)->word[3], 1 , 1 , v))
>> +#define EVT_SET_IND(x, v)   ((x)->word[3] = deposit32((x)->word[3], 2 , 1 , v))
>> +#define EVT_SET_RNW(x, v)   ((x)->word[3] = deposit32((x)->word[3], 3 , 1 , v))
>> +#define EVT_SET_S2(x, v)    ((x)->word[3] = deposit32((x)->word[3], 7 , 1 , v))
>> +#define EVT_SET_CLASS(x, v) ((x)->word[3] = deposit32((x)->word[3], 8 , 2 , v))
> 
> 
> Oops. I'm a bit embarrassed I didn't notice that in code review...
Well I am even more embarrassed than you ;-) Usually there are no
faults. When I got some, mostly due to emulation code bugs, I mostly
focused on QEMU guest_errors logs. I noticed the code was broken when
attempting to report faults from host smmu driver. Sorry for the
inconvenience.

Thanks

Eric
> 
> thanks
> -- PMM
>
diff mbox series

Patch

diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index bab25d640e..19540f8f41 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -442,17 +442,17 @@  typedef struct SMMUEventInfo {
 
 #define EVT_Q_OVERFLOW        (1 << 31)
 
-#define EVT_SET_TYPE(x, v)              deposit32((x)->word[0], 0 , 8 , v)
-#define EVT_SET_SSV(x, v)               deposit32((x)->word[0], 11, 1 , v)
-#define EVT_SET_SSID(x, v)              deposit32((x)->word[0], 12, 20, v)
-#define EVT_SET_SID(x, v)               ((x)->word[1] = v)
-#define EVT_SET_STAG(x, v)              deposit32((x)->word[2], 0 , 16, v)
-#define EVT_SET_STALL(x, v)             deposit32((x)->word[2], 31, 1 , v)
-#define EVT_SET_PNU(x, v)               deposit32((x)->word[3], 1 , 1 , v)
-#define EVT_SET_IND(x, v)               deposit32((x)->word[3], 2 , 1 , v)
-#define EVT_SET_RNW(x, v)               deposit32((x)->word[3], 3 , 1 , v)
-#define EVT_SET_S2(x, v)                deposit32((x)->word[3], 7 , 1 , v)
-#define EVT_SET_CLASS(x, v)             deposit32((x)->word[3], 8 , 2 , v)
+#define EVT_SET_TYPE(x, v)  ((x)->word[0] = deposit32((x)->word[0], 0 , 8 , v))
+#define EVT_SET_SSV(x, v)   ((x)->word[0] = deposit32((x)->word[0], 11, 1 , v))
+#define EVT_SET_SSID(x, v)  ((x)->word[0] = deposit32((x)->word[0], 12, 20, v))
+#define EVT_SET_SID(x, v)   ((x)->word[1] = v)
+#define EVT_SET_STAG(x, v)  ((x)->word[2] = deposit32((x)->word[2], 0 , 16, v))
+#define EVT_SET_STALL(x, v) ((x)->word[2] = deposit32((x)->word[2], 31, 1 , v))
+#define EVT_SET_PNU(x, v)   ((x)->word[3] = deposit32((x)->word[3], 1 , 1 , v))
+#define EVT_SET_IND(x, v)   ((x)->word[3] = deposit32((x)->word[3], 2 , 1 , v))
+#define EVT_SET_RNW(x, v)   ((x)->word[3] = deposit32((x)->word[3], 3 , 1 , v))
+#define EVT_SET_S2(x, v)    ((x)->word[3] = deposit32((x)->word[3], 7 , 1 , v))
+#define EVT_SET_CLASS(x, v) ((x)->word[3] = deposit32((x)->word[3], 8 , 2 , v))
 #define EVT_SET_ADDR(x, addr)                             \
     do {                                                  \
             (x)->word[5] = (uint32_t)(addr >> 32);        \
@@ -460,8 +460,8 @@  typedef struct SMMUEventInfo {
     } while (0)
 #define EVT_SET_ADDR2(x, addr)                            \
     do {                                                  \
-            deposit32((x)->word[7], 3, 29, addr >> 16);   \
-            deposit32((x)->word[7], 0, 16, addr & 0xffff);\
+            (x)->word[7] = deposit32((x)->word[7], 3, 29, addr >> 16);   \
+            (x)->word[7] = deposit32((x)->word[7], 0, 16, addr & 0xffff);\
     } while (0)
 
 void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *event);
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index bb6a24e9b8..8c4e99fecc 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -136,7 +136,7 @@  static MemTxResult smmuv3_write_eventq(SMMUv3State *s, Evt *evt)
         return r;
     }
 
-    if (smmuv3_q_empty(q)) {
+    if (!smmuv3_q_empty(q)) {
         smmuv3_trigger_irq(s, SMMU_IRQ_EVTQ, 0);
     }
     return MEMTX_OK;