[PATCH 0/5] hw/arm/smmuv3: Correct stream ID and event address handling

[PATCH 0/5] hw/arm/smmuv3: Correct stream ID and event address handling

[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.

Simon Veith (5):
  hw/arm/smmuv3: Apply address mask to linear strtab base address
  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 |  4 ++--
 hw/arm/smmuv3.c          | 39 ++++++++++++++++++++++++++++++++-------
 2 files changed, 34 insertions(+), 9 deletions(-)

-- 
2.7.4

[PATCH 1/5] hw/arm/smmuv3: Apply address mask to linear strtab base address

[Simon Veith]

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
---
 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

[PATCH 2/5] hw/arm/smmuv3: Check stream IDs against actual table LOG2SIZE

[Simon Veith]

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.

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
---
 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..aad4639 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

[PATCH 3/5] hw/arm/smmuv3: Align stream table base address to table size

[Simon Veith]

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
---
 hw/arm/smmuv3.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index aad4639..2d6c275 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,23 @@ 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 = log2size - s->sid_split - 1 + 3;
+        if (strtab_size_shift < DMA_ADDR_BITS) {
+            if (strtab_size_shift < 5) {
+                strtab_size_shift = 5;
+            }
+            strtab_base = s->strtab_base & SMMU_BASE_ADDR_MASK &
+                          ~((1ULL << strtab_size_shift) - 1);
+        } else {
+            strtab_base = 0;
+        }
         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 +447,14 @@ 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;
+        if (strtab_size_shift < DMA_ADDR_BITS) {
+            strtab_base = s->strtab_base & SMMU_BASE_ADDR_MASK &
+                          ~((1ULL << strtab_size_shift) - 1);
+        } else {
+            strtab_base = 0;
+        }
+        addr = strtab_base + sid * sizeof(*ste);
     }
 
     if (smmu_get_ste(s, addr, ste, event)) {
-- 
2.7.4

[PATCH 4/5] hw/arm/smmuv3: Use correct bit positions in EVT_SET_ADDR2 macro

[Simon Veith]

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
---
 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 d190181..eb275e2 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

[PATCH 5/5] hw/arm/smmuv3: Report F_STE_FETCH fault address in correct word position

[Simon Veith]

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
---
 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 2d6c275..125e47d 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

Re: [PATCH 4/5] hw/arm/smmuv3: Use correct bit positions in EVT_SET_ADDR2 macro

[Auger Eric]

Hi Simon,

On 12/4/19 2:55 PM, Simon Veith wrote:
> 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>
Acked-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> ---
>  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 d190181..eb275e2 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);
> 

Re: [PATCH 5/5] hw/arm/smmuv3: Report F_STE_FETCH fault address in correct word position

[Auger Eric]

Hi Simon,

On 12/4/19 2:55 PM, Simon Veith wrote:
> 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
> ---
>  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 2d6c275..125e47d 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);
> 
Acked-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

Re: [PATCH 2/5] hw/arm/smmuv3: Check stream IDs against actual table LOG2SIZE

[Auger Eric]

Hi Simon,
On 12/4/19 2:55 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.
> 
> 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
> ---
>  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..aad4639 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))) {I think this should be sid >= (1 << MIN(log2size, SMMU_IDR1_SIDSIZE))

If you agree can you fix it at the same time?
>          event->type = SMMU_EVT_C_BAD_STREAMID;
>          return -EINVAL;
>      }
> 

Thanks

Eric

Re: [PATCH 1/5] hw/arm/smmuv3: Apply address mask to linear strtab base address

[Auger Eric]

Hi Simon,

On 12/4/19 2:55 PM, Simon Veith wrote:
> 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
> ---
>  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);
Not related to this patch but I noticed SMMU_BASE_ADDR_MASK should be
0xffffffffffc0 and not 0xffffffffffe0. I can fix it separately or if you
respin, you may fix it as well?
>      }
>  
>      if (smmu_get_ste(s, addr, ste, event)) {
> 
Besides
Acked-by: Eric Auger <eric.auger@redhat.com>


Thanks

Eric

Re: [PATCH 3/5] hw/arm/smmuv3: Align stream table base address to table size

[Auger Eric]

Hi Simon,

On 12/4/19 2:55 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
> ---
>  hw/arm/smmuv3.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index aad4639..2d6c275 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,23 @@ 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 = log2size - s->sid_split - 1 + 3;
Maybe just as the spec use MAX(5, (LOG2SIZE-SPLIT-1+3))?
> +        if (strtab_size_shift < DMA_ADDR_BITS) {
> +            if (strtab_size_shift < 5) {
> +                strtab_size_shift = 5;
> +            }
> +            strtab_base = s->strtab_base & SMMU_BASE_ADDR_MASK &
> +                          ~((1ULL << strtab_size_shift) - 1);
nit: you may use ~MAKE_64BIT_MASK(0, strtab_size_shift)
> +        } else {
see below
> +            strtab_base = 0;
> +        }
>          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 +447,14 @@ 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;
> +        if (strtab_size_shift < DMA_ADDR_BITS) {
> +            strtab_base = s->strtab_base & SMMU_BASE_ADDR_MASK &
> +                          ~((1ULL << strtab_size_shift) - 1);
> +        } else {
Can it happen? I understand LOG2SIZE <= SMMU_S_IDR_5.SIDSIZE
and SIDSIZE is max 32. Same above?
> +            strtab_base = 0;
> +        }
> +        addr = strtab_base + sid * sizeof(*ste);
>      }
>  
>      if (smmu_get_ste(s, addr, ste, event)) {
> 
Thank you for those series fixes.

Best Regards

Eric

Re: [PATCH 1/5] hw/arm/smmuv3: Apply address mask to linear strtab base address

[Simon Veith]

Hello Eric,

On 05/12/2019 09:42, Auger Eric wrote:
> Not related to this patch but I noticed SMMU_BASE_ADDR_MASK should be
> 0xffffffffffc0 and not 0xffffffffffe0. I can fix it separately or if you
> respin, you may fix it as well?

Good catch, thank you. I'll fix it in the next version.

Looking at the upper end of that mask, it seems that we are currently masking out bits 48 through 63, rather than just 51 through 63.
The reference manual says that this should be done to match the system physical address size as given by SMMU_IDR5.OAS.

We define SMMU_IDR5_OAS to be 4, which selects a physical address size of 44 bits (ref. section 6.3.6). I think the mask should therefore be 0xfffffffffc0 to clear bits 44 and above. Do you agree?

Ideally, we would derive this mask from our definition of SMMU_IDR5_OAS, but I'm not sure it's okay to stuff a call to oas2bits() into the SMMU_BASE_ADDR_MASK macro.

Regards
Simon



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH 1/5] hw/arm/smmuv3: Apply address mask to linear strtab base address

[Auger Eric]

Hi Simon,

On 12/5/19 11:04 PM, Simon Veith wrote:
> Hello Eric,
> 
> On 05/12/2019 09:42, Auger Eric wrote:
>> Not related to this patch but I noticed SMMU_BASE_ADDR_MASK should be
>> 0xffffffffffc0 and not 0xffffffffffe0. I can fix it separately or if you
>> respin, you may fix it as well?
> 
> Good catch, thank you. I'll fix it in the next version.
> 
> Looking at the upper end of that mask, it seems that we are currently masking out bits 48 through 63, rather than just 51 through 63.
> The reference manual says that this should be done to match the system physical address size as given by SMMU_IDR5.OAS.
Yes you're right. This should go up to 51 as per the field range
definition. Spec says address bits and above this field range are
treated as zero.
> 
> We define SMMU_IDR5_OAS to be 4, which selects a physical address size of 44 bits (ref. section 6.3.6). I think the mask should therefore be 0xfffffffffc0 to clear bits 44 and above. Do you agree?
bits beyond the OAS are RES0. The spec does not says those fields are
treated as zero, as explicitly mentioned for bits > 51. Normally the
guest should not set them to something != 0, this would be a programming
error right? Guest is supposed to read the IDR5 and program accordingly?

> 
> Ideally, we would derive this mask from our definition of SMMU_IDR5_OAS, but I'm not sure it's okay to stuff a call to oas2bits() into the SMMU_BASE_ADDR_MASK macro.
Well I am not sure this is worth the candle. I am not sure we are
obliged to enforce this.

Thanks

Eric
> 
> Regards
> Simon
> 
> 
> 
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
> Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
> Sitz: Berlin
> Ust-ID: DE 289 237 879
> 
>