qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/6] hw/arm/smmuv3: Correct stream ID and event address handling
@ 2019-12-16 15:15 Simon Veith
  2019-12-16 15:15 ` [PATCH v3 1/6] hw/arm/smmuv3: Apply address mask to linear strtab base address Simon Veith
                   ` (6 more replies)
  0 siblings, 7 replies; 12+ messages in thread
From: Simon Veith @ 2019-12-16 15:15 UTC (permalink / raw)
  To: qemu-devel, qemu-arm; +Cc: Simon Veith

While working on the Linux SMMUv3 driver, I noticed a few cases where the QEMU
SMMUv3 behavior relating to stream tables was inconsistent with our hardware.

Also, when debugging those differences, I found that the errors reported through
the QEMU SMMUv3 event queue contained the address fields in an incorrect
position.

These patches correct the QEMU SMMUv3 behavior to match the specification (and
the behavior that I observed in our hardware). Linux guests normally will not
notice these issues, but other SMMUv3 driver implementations might.

Changes in v2:

* New patch "hw/arm/smmuv3: Correct SMMU_BASE_ADDR_MASK value" added
* Updated patch "hw/arm/smmuv3: Check stream IDs against actual table LOG2SIZE"
* Updated patch "hw/arm/smmuv3: Align stream table base address to table size"

Changes in v3:

* No changes, but sending again to correct a patch submission mishap that
  confused Patchew

Simon Veith (6):
  hw/arm/smmuv3: Apply address mask to linear strtab base address
  hw/arm/smmuv3: Correct SMMU_BASE_ADDR_MASK value
  hw/arm/smmuv3: Check stream IDs against actual table LOG2SIZE
  hw/arm/smmuv3: Align stream table base address to table size
  hw/arm/smmuv3: Use correct bit positions in EVT_SET_ADDR2 macro
  hw/arm/smmuv3: Report F_STE_FETCH fault address in correct word
    position

 hw/arm/smmuv3-internal.h |  6 +++---
 hw/arm/smmuv3.c          | 28 +++++++++++++++++++++-------
 2 files changed, 24 insertions(+), 10 deletions(-)

-- 
2.7.4



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3 1/6] hw/arm/smmuv3: Apply address mask to linear strtab base address
  2019-12-16 15:15 [PATCH v3 0/6] hw/arm/smmuv3: Correct stream ID and event address handling Simon Veith
@ 2019-12-16 15:15 ` Simon Veith
  2019-12-16 15:15 ` [PATCH v3 2/6] hw/arm/smmuv3: Correct SMMU_BASE_ADDR_MASK value Simon Veith
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Simon Veith @ 2019-12-16 15:15 UTC (permalink / raw)
  To: qemu-devel, qemu-arm; +Cc: Simon Veith, Eric Auger

In the SMMU_STRTAB_BASE register, the stream table base address only
occupies bits [51:6]. Other bits, such as RA (bit [62]), must be masked
out to obtain the base address.

The branch for 2-level stream tables correctly applies this mask by way
of SMMU_BASE_ADDR_MASK, but the one for linear stream tables does not.

Apply the missing mask in that case as well so that the correct stream
base address is used by guests which configure a linear stream table.

Linux guests are unaffected by this change because they choose a 2-level
stream table layout for the QEMU SMMUv3, based on the size of its stream
ID space.

ref. ARM IHI 0070C, section 6.3.23.

Signed-off-by: Simon Veith <sveith@amazon.de>
Cc: Eric Auger <eric.auger@redhat.com>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Acked-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/smmuv3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index e2fbb83..eef9a18 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -429,7 +429,7 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
         }
         addr = l2ptr + l2_ste_offset * sizeof(*ste);
     } else {
-        addr = s->strtab_base + sid * sizeof(*ste);
+        addr = (s->strtab_base & SMMU_BASE_ADDR_MASK) + sid * sizeof(*ste);
     }
 
     if (smmu_get_ste(s, addr, ste, event)) {
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 2/6] hw/arm/smmuv3: Correct SMMU_BASE_ADDR_MASK value
  2019-12-16 15:15 [PATCH v3 0/6] hw/arm/smmuv3: Correct stream ID and event address handling Simon Veith
  2019-12-16 15:15 ` [PATCH v3 1/6] hw/arm/smmuv3: Apply address mask to linear strtab base address Simon Veith
@ 2019-12-16 15:15 ` Simon Veith
  2019-12-17  8:55   ` Auger Eric
  2019-12-16 15:15 ` [PATCH v3 3/6] hw/arm/smmuv3: Check stream IDs against actual table LOG2SIZE Simon Veith
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 12+ messages in thread
From: Simon Veith @ 2019-12-16 15:15 UTC (permalink / raw)
  To: qemu-devel, qemu-arm; +Cc: Simon Veith, Eric Auger

There are two issues with the current value of SMMU_BASE_ADDR_MASK:

- At the lower end, we are clearing bits [4:0]. Per the SMMUv3 spec,
  we should also be treating bit 5 as zero in the base address.
- At the upper end, we are clearing bits [63:48]. Per the SMMUv3 spec,
  only bits [63:52] must be explicitly treated as zero.

Update the SMMU_BASE_ADDR_MASK value to mask out bits [63:52] and [5:0].

ref. ARM IHI 0070C, section 6.3.23.

Signed-off-by: Simon Veith <sveith@amazon.de>
Cc: Eric Auger <eric.auger@redhat.com>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
---
 hw/arm/smmuv3-internal.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index d190181..042b435 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -99,7 +99,7 @@ REG32(GERROR_IRQ_CFG2, 0x74)
 
 #define A_STRTAB_BASE      0x80 /* 64b */
 
-#define SMMU_BASE_ADDR_MASK 0xffffffffffe0
+#define SMMU_BASE_ADDR_MASK 0xfffffffffffc0
 
 REG32(STRTAB_BASE_CFG,     0x88)
     FIELD(STRTAB_BASE_CFG, FMT,      16, 2)
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 3/6] hw/arm/smmuv3: Check stream IDs against actual table LOG2SIZE
  2019-12-16 15:15 [PATCH v3 0/6] hw/arm/smmuv3: Correct stream ID and event address handling Simon Veith
  2019-12-16 15:15 ` [PATCH v3 1/6] hw/arm/smmuv3: Apply address mask to linear strtab base address Simon Veith
  2019-12-16 15:15 ` [PATCH v3 2/6] hw/arm/smmuv3: Correct SMMU_BASE_ADDR_MASK value Simon Veith
@ 2019-12-16 15:15 ` Simon Veith
  2019-12-17  8:54   ` Auger Eric
  2019-12-16 15:15 ` [PATCH v3 4/6] hw/arm/smmuv3: Align stream table base address to table size Simon Veith
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 12+ messages in thread
From: Simon Veith @ 2019-12-16 15:15 UTC (permalink / raw)
  To: qemu-devel, qemu-arm; +Cc: Simon Veith, Eric Auger

When checking whether a stream ID is in range of the stream table, we
have so far been only checking it against our implementation limit
(SMMU_IDR1_SIDSIZE). However, the guest can program the
STRTAB_BASE_CFG.LOG2SIZE field to a size that is smaller than this
limit.

Check the stream ID against this limit as well to match the hardware
behavior of raising C_BAD_STREAMID events in case the limit is exceeded.
Also, ensure that we do not go one entry beyond the end of the table by
checking that its index is strictly smaller than the table size.

ref. ARM IHI 0070C, section 6.3.24.

Signed-off-by: Simon Veith <sveith@amazon.de>
Cc: Eric Auger <eric.auger@redhat.com>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
---
Changed in v2:

* Also check that stream ID is strictly lower than the table size

 hw/arm/smmuv3.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index eef9a18..727558b 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -377,11 +377,15 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
                          SMMUEventInfo *event)
 {
     dma_addr_t addr;
+    uint32_t log2size;
     int ret;
 
     trace_smmuv3_find_ste(sid, s->features, s->sid_split);
-    /* Check SID range */
-    if (sid > (1 << SMMU_IDR1_SIDSIZE)) {
+    log2size = FIELD_EX32(s->strtab_base_cfg, STRTAB_BASE_CFG, LOG2SIZE);
+    /*
+     * Check SID range against both guest-configured and implementation limits
+     */
+    if (sid >= (1 << MIN(log2size, SMMU_IDR1_SIDSIZE))) {
         event->type = SMMU_EVT_C_BAD_STREAMID;
         return -EINVAL;
     }
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 4/6] hw/arm/smmuv3: Align stream table base address to table size
  2019-12-16 15:15 [PATCH v3 0/6] hw/arm/smmuv3: Correct stream ID and event address handling Simon Veith
                   ` (2 preceding siblings ...)
  2019-12-16 15:15 ` [PATCH v3 3/6] hw/arm/smmuv3: Check stream IDs against actual table LOG2SIZE Simon Veith
@ 2019-12-16 15:15 ` Simon Veith
  2019-12-17  9:40   ` Auger Eric
  2019-12-16 15:15 ` [PATCH v3 5/6] hw/arm/smmuv3: Use correct bit positions in EVT_SET_ADDR2 macro Simon Veith
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 12+ messages in thread
From: Simon Veith @ 2019-12-16 15:15 UTC (permalink / raw)
  To: qemu-devel, qemu-arm; +Cc: Simon Veith, Eric Auger

Per the specification, and as observed in hardware, the SMMUv3 aligns
the SMMU_STRTAB_BASE address to the size of the table by masking out the
respective least significant bits in the ADDR field.

Apply this masking logic to our smmu_find_ste() lookup function per the
specification.

ref. ARM IHI 0070C, section 6.3.23.

Signed-off-by: Simon Veith <sveith@amazon.de>
Cc: Eric Auger <eric.auger@redhat.com>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
---
Changed in v2:

* Now using MAKE_64BIT_MASK()
* Eliminated unnecessary branches by using MAX()
* Removed unnecessary range check against DMA_ADDR_BITS

 hw/arm/smmuv3.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 727558b..31ac3ca 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -376,8 +376,9 @@ bad_ste:
 static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
                          SMMUEventInfo *event)
 {
-    dma_addr_t addr;
+    dma_addr_t addr, strtab_base;
     uint32_t log2size;
+    int strtab_size_shift;
     int ret;
 
     trace_smmuv3_find_ste(sid, s->features, s->sid_split);
@@ -391,10 +392,16 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
     }
     if (s->features & SMMU_FEATURE_2LVL_STE) {
         int l1_ste_offset, l2_ste_offset, max_l2_ste, span;
-        dma_addr_t strtab_base, l1ptr, l2ptr;
+        dma_addr_t l1ptr, l2ptr;
         STEDesc l1std;
 
-        strtab_base = s->strtab_base & SMMU_BASE_ADDR_MASK;
+        /*
+         * Align strtab base address to table size. For this purpose, assume it
+         * is not bounded by SMMU_IDR1_SIDSIZE.
+         */
+        strtab_size_shift = MAX(5, (int)log2size - s->sid_split - 1 + 3);
+        strtab_base = s->strtab_base & SMMU_BASE_ADDR_MASK &
+                      ~MAKE_64BIT_MASK(0, strtab_size_shift);
         l1_ste_offset = sid >> s->sid_split;
         l2_ste_offset = sid & ((1 << s->sid_split) - 1);
         l1ptr = (dma_addr_t)(strtab_base + l1_ste_offset * sizeof(l1std));
@@ -433,7 +440,10 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
         }
         addr = l2ptr + l2_ste_offset * sizeof(*ste);
     } else {
-        addr = (s->strtab_base & SMMU_BASE_ADDR_MASK) + sid * sizeof(*ste);
+        strtab_size_shift = log2size + 5;
+        strtab_base = s->strtab_base & SMMU_BASE_ADDR_MASK &
+                      ~MAKE_64BIT_MASK(0, strtab_size_shift);
+        addr = strtab_base + sid * sizeof(*ste);
     }
 
     if (smmu_get_ste(s, addr, ste, event)) {
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 5/6] hw/arm/smmuv3: Use correct bit positions in EVT_SET_ADDR2 macro
  2019-12-16 15:15 [PATCH v3 0/6] hw/arm/smmuv3: Correct stream ID and event address handling Simon Veith
                   ` (3 preceding siblings ...)
  2019-12-16 15:15 ` [PATCH v3 4/6] hw/arm/smmuv3: Align stream table base address to table size Simon Veith
@ 2019-12-16 15:15 ` Simon Veith
  2019-12-16 15:15 ` [PATCH v3 6/6] hw/arm/smmuv3: Report F_STE_FETCH fault address in correct word position Simon Veith
  2019-12-17 10:03 ` [PATCH v3 0/6] hw/arm/smmuv3: Correct stream ID and event address handling Auger Eric
  6 siblings, 0 replies; 12+ messages in thread
From: Simon Veith @ 2019-12-16 15:15 UTC (permalink / raw)
  To: qemu-devel, qemu-arm; +Cc: Simon Veith, Eric Auger

The bit offsets in the EVT_SET_ADDR2 macro do not match those specified
in the ARM SMMUv3 Architecture Specification. In all events that use
this macro, e.g. F_WALK_EABT, the faulting fetch address or IPA actually
occupies the 32-bit words 6 and 7 in the event record contiguously, with
the upper and lower unused bits clear due to alignment or maximum
supported address bits. How many bits are clear depends on the
individual event type.

Update the macro to write to the correct words in the event record so
that guest drivers can obtain accurate address information on events.

ref. ARM IHI 0070C, sections 7.3.12 through 7.3.16.

Signed-off-by: Simon Veith <sveith@amazon.de>
Cc: Eric Auger <eric.auger@redhat.com>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Acked-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/smmuv3-internal.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index 042b435..4112394 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -461,8 +461,8 @@ typedef struct SMMUEventInfo {
     } while (0)
 #define EVT_SET_ADDR2(x, addr)                            \
     do {                                                  \
-            (x)->word[7] = deposit32((x)->word[7], 3, 29, addr >> 16);   \
-            (x)->word[7] = deposit32((x)->word[7], 0, 16, addr & 0xffff);\
+            (x)->word[7] = (uint32_t)(addr >> 32);        \
+            (x)->word[6] = (uint32_t)(addr & 0xffffffff); \
     } while (0)
 
 void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *event);
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 6/6] hw/arm/smmuv3: Report F_STE_FETCH fault address in correct word position
  2019-12-16 15:15 [PATCH v3 0/6] hw/arm/smmuv3: Correct stream ID and event address handling Simon Veith
                   ` (4 preceding siblings ...)
  2019-12-16 15:15 ` [PATCH v3 5/6] hw/arm/smmuv3: Use correct bit positions in EVT_SET_ADDR2 macro Simon Veith
@ 2019-12-16 15:15 ` Simon Veith
  2019-12-17 10:03 ` [PATCH v3 0/6] hw/arm/smmuv3: Correct stream ID and event address handling Auger Eric
  6 siblings, 0 replies; 12+ messages in thread
From: Simon Veith @ 2019-12-16 15:15 UTC (permalink / raw)
  To: qemu-devel, qemu-arm; +Cc: Simon Veith, Eric Auger

The smmuv3_record_event() function that generates the F_STE_FETCH error
uses the EVT_SET_ADDR macro to record the fetch address, placing it in
32-bit words 4 and 5.

The correct position for this address is in words 6 and 7, per the
SMMUv3 Architecture Specification.

Update the function to use the EVT_SET_ADDR2 macro instead, which is the
macro intended for writing to these words.

ref. ARM IHI 0070C, section 7.3.4.

Signed-off-by: Simon Veith <sveith@amazon.de>
Cc: Eric Auger <eric.auger@redhat.com>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Acked-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/smmuv3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 31ac3ca..8b5f157 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -172,7 +172,7 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
     case SMMU_EVT_F_STE_FETCH:
         EVT_SET_SSID(&evt, info->u.f_ste_fetch.ssid);
         EVT_SET_SSV(&evt,  info->u.f_ste_fetch.ssv);
-        EVT_SET_ADDR(&evt, info->u.f_ste_fetch.addr);
+        EVT_SET_ADDR2(&evt, info->u.f_ste_fetch.addr);
         break;
     case SMMU_EVT_C_BAD_STE:
         EVT_SET_SSID(&evt, info->u.c_bad_ste.ssid);
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 3/6] hw/arm/smmuv3: Check stream IDs against actual table LOG2SIZE
  2019-12-16 15:15 ` [PATCH v3 3/6] hw/arm/smmuv3: Check stream IDs against actual table LOG2SIZE Simon Veith
@ 2019-12-17  8:54   ` Auger Eric
  0 siblings, 0 replies; 12+ messages in thread
From: Auger Eric @ 2019-12-17  8:54 UTC (permalink / raw)
  To: Simon Veith, qemu-devel, qemu-arm

Hi Simon,

On 12/16/19 4:15 PM, Simon Veith wrote:
> When checking whether a stream ID is in range of the stream table, we
> have so far been only checking it against our implementation limit
> (SMMU_IDR1_SIDSIZE). However, the guest can program the
> STRTAB_BASE_CFG.LOG2SIZE field to a size that is smaller than this
> limit.
> 
> Check the stream ID against this limit as well to match the hardware
> behavior of raising C_BAD_STREAMID events in case the limit is exceeded.
> Also, ensure that we do not go one entry beyond the end of the table by
> checking that its index is strictly smaller than the table size.
> 
> ref. ARM IHI 0070C, section 6.3.24.
> 
> Signed-off-by: Simon Veith <sveith@amazon.de>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
Acked-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
> Changed in v2:
> 
> * Also check that stream ID is strictly lower than the table size
> 
>  hw/arm/smmuv3.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index eef9a18..727558b 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -377,11 +377,15 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
>                           SMMUEventInfo *event)
>  {
>      dma_addr_t addr;
> +    uint32_t log2size;
>      int ret;
>  
>      trace_smmuv3_find_ste(sid, s->features, s->sid_split);
> -    /* Check SID range */
> -    if (sid > (1 << SMMU_IDR1_SIDSIZE)) {
> +    log2size = FIELD_EX32(s->strtab_base_cfg, STRTAB_BASE_CFG, LOG2SIZE);
> +    /*
> +     * Check SID range against both guest-configured and implementation limits
> +     */
> +    if (sid >= (1 << MIN(log2size, SMMU_IDR1_SIDSIZE))) {
>          event->type = SMMU_EVT_C_BAD_STREAMID;
>          return -EINVAL;
>      }
> 



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/6] hw/arm/smmuv3: Correct SMMU_BASE_ADDR_MASK value
  2019-12-16 15:15 ` [PATCH v3 2/6] hw/arm/smmuv3: Correct SMMU_BASE_ADDR_MASK value Simon Veith
@ 2019-12-17  8:55   ` Auger Eric
  0 siblings, 0 replies; 12+ messages in thread
From: Auger Eric @ 2019-12-17  8:55 UTC (permalink / raw)
  To: Simon Veith, qemu-devel, qemu-arm

Hi Simon,

On 12/16/19 4:15 PM, Simon Veith wrote:
> There are two issues with the current value of SMMU_BASE_ADDR_MASK:
> 
> - At the lower end, we are clearing bits [4:0]. Per the SMMUv3 spec,
>   we should also be treating bit 5 as zero in the base address.
> - At the upper end, we are clearing bits [63:48]. Per the SMMUv3 spec,
>   only bits [63:52] must be explicitly treated as zero.
> 
> Update the SMMU_BASE_ADDR_MASK value to mask out bits [63:52] and [5:0].
> 
> ref. ARM IHI 0070C, section 6.3.23.
> 
> Signed-off-by: Simon Veith <sveith@amazon.de>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
Acked-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
>  hw/arm/smmuv3-internal.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index d190181..042b435 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -99,7 +99,7 @@ REG32(GERROR_IRQ_CFG2, 0x74)
>  
>  #define A_STRTAB_BASE      0x80 /* 64b */
>  
> -#define SMMU_BASE_ADDR_MASK 0xffffffffffe0
> +#define SMMU_BASE_ADDR_MASK 0xfffffffffffc0
>  
>  REG32(STRTAB_BASE_CFG,     0x88)
>      FIELD(STRTAB_BASE_CFG, FMT,      16, 2)
> 



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 4/6] hw/arm/smmuv3: Align stream table base address to table size
  2019-12-16 15:15 ` [PATCH v3 4/6] hw/arm/smmuv3: Align stream table base address to table size Simon Veith
@ 2019-12-17  9:40   ` Auger Eric
  0 siblings, 0 replies; 12+ messages in thread
From: Auger Eric @ 2019-12-17  9:40 UTC (permalink / raw)
  To: Simon Veith, qemu-devel, qemu-arm

Hi Simon,

On 12/16/19 4:15 PM, Simon Veith wrote:
> Per the specification, and as observed in hardware, the SMMUv3 aligns
> the SMMU_STRTAB_BASE address to the size of the table by masking out the
> respective least significant bits in the ADDR field.
> 
> Apply this masking logic to our smmu_find_ste() lookup function per the
> specification.
> 
> ref. ARM IHI 0070C, section 6.3.23.
> 
> Signed-off-by: Simon Veith <sveith@amazon.de>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
Looks good to me.

Acked-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
> Changed in v2:
> 
> * Now using MAKE_64BIT_MASK()
> * Eliminated unnecessary branches by using MAX()
> * Removed unnecessary range check against DMA_ADDR_BITS
> 
>  hw/arm/smmuv3.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 727558b..31ac3ca 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -376,8 +376,9 @@ bad_ste:
>  static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
>                           SMMUEventInfo *event)
>  {
> -    dma_addr_t addr;
> +    dma_addr_t addr, strtab_base;
>      uint32_t log2size;
> +    int strtab_size_shift;
>      int ret;
>  
>      trace_smmuv3_find_ste(sid, s->features, s->sid_split);
> @@ -391,10 +392,16 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
>      }
>      if (s->features & SMMU_FEATURE_2LVL_STE) {
>          int l1_ste_offset, l2_ste_offset, max_l2_ste, span;
> -        dma_addr_t strtab_base, l1ptr, l2ptr;
> +        dma_addr_t l1ptr, l2ptr;
>          STEDesc l1std;
>  
> -        strtab_base = s->strtab_base & SMMU_BASE_ADDR_MASK;
> +        /*
> +         * Align strtab base address to table size. For this purpose, assume it
> +         * is not bounded by SMMU_IDR1_SIDSIZE.
> +         */
> +        strtab_size_shift = MAX(5, (int)log2size - s->sid_split - 1 + 3);
> +        strtab_base = s->strtab_base & SMMU_BASE_ADDR_MASK &
> +                      ~MAKE_64BIT_MASK(0, strtab_size_shift);
>          l1_ste_offset = sid >> s->sid_split;
>          l2_ste_offset = sid & ((1 << s->sid_split) - 1);
>          l1ptr = (dma_addr_t)(strtab_base + l1_ste_offset * sizeof(l1std));
> @@ -433,7 +440,10 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
>          }
>          addr = l2ptr + l2_ste_offset * sizeof(*ste);
>      } else {
> -        addr = (s->strtab_base & SMMU_BASE_ADDR_MASK) + sid * sizeof(*ste);
> +        strtab_size_shift = log2size + 5;
> +        strtab_base = s->strtab_base & SMMU_BASE_ADDR_MASK &
> +                      ~MAKE_64BIT_MASK(0, strtab_size_shift);
> +        addr = strtab_base + sid * sizeof(*ste);
>      }
>  
>      if (smmu_get_ste(s, addr, ste, event)) {
> 



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 0/6] hw/arm/smmuv3: Correct stream ID and event address handling
  2019-12-16 15:15 [PATCH v3 0/6] hw/arm/smmuv3: Correct stream ID and event address handling Simon Veith
                   ` (5 preceding siblings ...)
  2019-12-16 15:15 ` [PATCH v3 6/6] hw/arm/smmuv3: Report F_STE_FETCH fault address in correct word position Simon Veith
@ 2019-12-17 10:03 ` Auger Eric
  2019-12-17 15:25   ` Peter Maydell
  6 siblings, 1 reply; 12+ messages in thread
From: Auger Eric @ 2019-12-17 10:03 UTC (permalink / raw)
  To: Simon Veith, qemu-devel, qemu-arm

Hi,

On 12/16/19 4:15 PM, Simon Veith wrote:
> While working on the Linux SMMUv3 driver, I noticed a few cases where the QEMU
> SMMUv3 behavior relating to stream tables was inconsistent with our hardware.
> 
> Also, when debugging those differences, I found that the errors reported through
> the QEMU SMMUv3 event queue contained the address fields in an incorrect
> position.
> 
> These patches correct the QEMU SMMUv3 behavior to match the specification (and
> the behavior that I observed in our hardware). Linux guests normally will not
> notice these issues, but other SMMUv3 driver implementations might.
> 
> Changes in v2:
> 
> * New patch "hw/arm/smmuv3: Correct SMMU_BASE_ADDR_MASK value" added
> * Updated patch "hw/arm/smmuv3: Check stream IDs against actual table LOG2SIZE"
> * Updated patch "hw/arm/smmuv3: Align stream table base address to table size"
> 
> Changes in v3:
> 
> * No changes, but sending again to correct a patch submission mishap that
>   confused Patchew
> 
> Simon Veith (6):
>   hw/arm/smmuv3: Apply address mask to linear strtab base address
>   hw/arm/smmuv3: Correct SMMU_BASE_ADDR_MASK value
>   hw/arm/smmuv3: Check stream IDs against actual table LOG2SIZE
>   hw/arm/smmuv3: Align stream table base address to table size
>   hw/arm/smmuv3: Use correct bit positions in EVT_SET_ADDR2 macro
>   hw/arm/smmuv3: Report F_STE_FETCH fault address in correct word
>     position

The series looks good to me. Also tested against non regression.

Tested-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric


> 
>  hw/arm/smmuv3-internal.h |  6 +++---
>  hw/arm/smmuv3.c          | 28 +++++++++++++++++++++-------
>  2 files changed, 24 insertions(+), 10 deletions(-)
> 



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 0/6] hw/arm/smmuv3: Correct stream ID and event address handling
  2019-12-17 10:03 ` [PATCH v3 0/6] hw/arm/smmuv3: Correct stream ID and event address handling Auger Eric
@ 2019-12-17 15:25   ` Peter Maydell
  0 siblings, 0 replies; 12+ messages in thread
From: Peter Maydell @ 2019-12-17 15:25 UTC (permalink / raw)
  To: Auger Eric; +Cc: Simon Veith, qemu-arm, QEMU Developers

On Tue, 17 Dec 2019 at 10:04, Auger Eric <eric.auger@redhat.com> wrote:
>
> Hi,
>
> On 12/16/19 4:15 PM, Simon Veith wrote:
> > While working on the Linux SMMUv3 driver, I noticed a few cases where the QEMU
> > SMMUv3 behavior relating to stream tables was inconsistent with our hardware.
> >
> > Also, when debugging those differences, I found that the errors reported through
> > the QEMU SMMUv3 event queue contained the address fields in an incorrect
> > position.
> >
> > These patches correct the QEMU SMMUv3 behavior to match the specification (and
> > the behavior that I observed in our hardware). Linux guests normally will not
> > notice these issues, but other SMMUv3 driver implementations might.
> >
> > Changes in v2:
> >
> > * New patch "hw/arm/smmuv3: Correct SMMU_BASE_ADDR_MASK value" added
> > * Updated patch "hw/arm/smmuv3: Check stream IDs against actual table LOG2SIZE"
> > * Updated patch "hw/arm/smmuv3: Align stream table base address to table size"
> >
> > Changes in v3:
> >
> > * No changes, but sending again to correct a patch submission mishap that
> >   confused Patchew
> >
> > Simon Veith (6):
> >   hw/arm/smmuv3: Apply address mask to linear strtab base address
> >   hw/arm/smmuv3: Correct SMMU_BASE_ADDR_MASK value
> >   hw/arm/smmuv3: Check stream IDs against actual table LOG2SIZE
> >   hw/arm/smmuv3: Align stream table base address to table size
> >   hw/arm/smmuv3: Use correct bit positions in EVT_SET_ADDR2 macro
> >   hw/arm/smmuv3: Report F_STE_FETCH fault address in correct word
> >     position
>
> The series looks good to me. Also tested against non regression.
>
> Tested-by: Eric Auger <eric.auger@redhat.com>

Applied to target-arm.next, thanks.

-- PMM


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-12-17 15:26 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-16 15:15 [PATCH v3 0/6] hw/arm/smmuv3: Correct stream ID and event address handling Simon Veith
2019-12-16 15:15 ` [PATCH v3 1/6] hw/arm/smmuv3: Apply address mask to linear strtab base address Simon Veith
2019-12-16 15:15 ` [PATCH v3 2/6] hw/arm/smmuv3: Correct SMMU_BASE_ADDR_MASK value Simon Veith
2019-12-17  8:55   ` Auger Eric
2019-12-16 15:15 ` [PATCH v3 3/6] hw/arm/smmuv3: Check stream IDs against actual table LOG2SIZE Simon Veith
2019-12-17  8:54   ` Auger Eric
2019-12-16 15:15 ` [PATCH v3 4/6] hw/arm/smmuv3: Align stream table base address to table size Simon Veith
2019-12-17  9:40   ` Auger Eric
2019-12-16 15:15 ` [PATCH v3 5/6] hw/arm/smmuv3: Use correct bit positions in EVT_SET_ADDR2 macro Simon Veith
2019-12-16 15:15 ` [PATCH v3 6/6] hw/arm/smmuv3: Report F_STE_FETCH fault address in correct word position Simon Veith
2019-12-17 10:03 ` [PATCH v3 0/6] hw/arm/smmuv3: Correct stream ID and event address handling Auger Eric
2019-12-17 15:25   ` Peter Maydell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).