[RFC PATCH 00/16] Add stage-2 translation for SMMUv3

[RFC PATCH 00/16] Add stage-2 translation for SMMUv3

[Mostafa Saleh]

This patch series adds stage-2 translation support for SMMUv3. It is
controlled by a new system property “arm-smmuv3.stage”.
- When set to “1”: Stage-1 only would be advertised and supported (default
behavior)
- When set to “2”: Stage-2 only would be advertised and supported.
- Value “all” is reserved for nesting support. However it is not
implemented in this patch series (more about this in the end)

Features implemented in stage-2 are mostly synonymous with stage-1
- VMID16.
- Only AArch64 translation tables are supported.
- 48 bits of IPA.
- Stall is not supported.
- HTTU is not supported, SW is expected to maintain the Access flag.

To make it easy to support nesting, a new structure(SMMUS2Cfg) is
embedded within SMMUTransCfg, to hold stage-2 configuration.

TLBs were updated to support VMID, where when stage-2 is used ASID are
set to -1 and ignored and when stage-1 is used VMID is set to -1 and
ignored.
As only one stage is supported at a time at the moment, TLB will
represent IPA=>PA translation with proper attributes(granularity and
t0sz) parsed from STEs for stage-2, and will represent VA=>PA
translation with proper attributes parsed from the CDs for stage-1.

New commands where added that are used with stage-2
- CMD_TLBI_S12_VMALL: Invalidate all translations for a VMID.
- CMD_TLBI_S2_IPA: Invalidate stage-2 by VMID and IPA

SMMUv3State.features has 2 new flags (SMMU_FEATURE_STAGE1 and
SMMU_FEATURE_STAGE2): to indicate stage-1 and stage-2 support in HW.

This patch series + GBPA patch
https://lore.kernel.org/qemu-devel/20230126141120.448641-1-smostafa@google.com/
Can be used to run Linux pKVM SMMUv3 patches (currently on the list)
which controls stage-2 (from EL2) while providing a paravirtualized
interface the host(EL1)
https://lore.kernel.org/kvmarm/20230201125328.2186498-1-jean-philippe@linaro.org/

Looking forward, nesting is the next feature to go for, here are some
thoughts about this:

- TLB would require big changes for this, we can go either for a combined
implementation or per stage one. This would affect returns from PTW and
invalidation commands.

- Stage-1 data structures should be translated by stage-2 if enabled (as
context descriptors and ttb0/ttb1)

- Translated addresses from stage-1 should be translated by stage-2 if
enabled.

- Record faults should be separated between stage-1 (CD_R) and stage-2
(S2R).

- Some existing commands(as CMD_TLBI_S2_IPA, CMD_TLBI_NH_ASID …) would be
modified and some of those would be based on the design of the TLBs.

- Currently, VMID is ignored when stage-1 is used as it can’t be used with
stage-2. However when nesting is advertised VMID shouldn’t be ignored
even if stage-2 is bypassed.

Mostafa Saleh (16):
  hw/arm/smmuv3: Add missing fields for IDR0
  hw/arm/smmuv3: Update translation config to hold stage-2
  hw/arm/smmuv3: Rename smmu_ptw_64
  hw/arm/smmuv3: Add a system property to choose translation stage
  hw/arm/smmuv3: Add page table walk for stage-2
  hw/arm/smmuv3: Parse STE config for stage-2
  hw/arm/smmuv3: Check validity of stage-2 page table
  hw/arm/smmuv3: Support S2AFFD
  hw/arm/smmuv3: Don't touch CD if stage-1 is not supported.
  hw/arm/smmuv3: Make TLB lookup work for stage-2
  hw/arm/smmuv3: Read VMID from STE
  hw/arm/smmuv3: Add VMID to tlb tagging
  hw/arm/smmuv3: Add CMDs related to stage 2
  hw/arm/smmuv3: Add stage-2 support in iova notifier
  hw/arm/smmuv3: Add fault configuration for stage-2
  hw/arm/smmuv3: Enable stage-2 support

 hw/arm/smmu-common.c         | 168 +++++++++++++++++++++---
 hw/arm/smmu-internal.h       |  41 ++++++
 hw/arm/smmuv3-internal.h     |  10 ++
 hw/arm/smmuv3.c              | 247 ++++++++++++++++++++++++++++++-----
 hw/arm/trace-events          |   4 +-
 include/hw/arm/smmu-common.h |  19 ++-
 include/hw/arm/smmuv3.h      |   1 +
 7 files changed, 433 insertions(+), 57 deletions(-)

-- 
2.39.1.519.gcb327c4b5f-goog

[RFC PATCH 01/16] hw/arm/smmuv3: Add missing fields for IDR0

[Mostafa Saleh]

In preparation for adding stage-2 support.
Add IDR0 fields related to stage-2.

VMID16: 16-bit VMID supported.
S2P: Stage-2 translation supported.

They are described in 6.3.1 SMMU_IDR0.

No functional change intended.

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 hw/arm/smmuv3-internal.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index bce161870f..170e88c24a 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -34,10 +34,12 @@ typedef enum SMMUTranslationStatus {
 /* MMIO Registers */
 
 REG32(IDR0,                0x0)
+    FIELD(IDR0, S2P,         0 , 1)
     FIELD(IDR0, S1P,         1 , 1)
     FIELD(IDR0, TTF,         2 , 2)
     FIELD(IDR0, COHACC,      4 , 1)
     FIELD(IDR0, ASID16,      12, 1)
+    FIELD(IDR0, VMID16,      18, 1)
     FIELD(IDR0, TTENDIAN,    21, 2)
     FIELD(IDR0, STALL_MODEL, 24, 2)
     FIELD(IDR0, TERM_MODEL,  26, 1)
-- 
2.39.1.519.gcb327c4b5f-goog

Re: [RFC PATCH 01/16] hw/arm/smmuv3: Add missing fields for IDR0

[Richard Henderson]

On 2/4/23 23:43, Mostafa Saleh wrote:
> In preparation for adding stage-2 support.
> Add IDR0 fields related to stage-2.
> 
> VMID16: 16-bit VMID supported.
> S2P: Stage-2 translation supported.
> 
> They are described in 6.3.1 SMMU_IDR0.
> 
> No functional change intended.
> 
> Signed-off-by: Mostafa Saleh<smostafa@google.com>
> ---
>   hw/arm/smmuv3-internal.h | 2 ++
>   1 file changed, 2 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [RFC PATCH 01/16] hw/arm/smmuv3: Add missing fields for IDR0

[Eric Auger]

Hi Mostafa,

On 2/5/23 10:43, Mostafa Saleh wrote:
> In preparation for adding stage-2 support.
> Add IDR0 fields related to stage-2.
>
> VMID16: 16-bit VMID supported.
> S2P: Stage-2 translation supported.
>
> They are described in 6.3.1 SMMU_IDR0.
>
> No functional change intended.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  hw/arm/smmuv3-internal.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index bce161870f..170e88c24a 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -34,10 +34,12 @@ typedef enum SMMUTranslationStatus {
>  /* MMIO Registers */
>  
>  REG32(IDR0,                0x0)
> +    FIELD(IDR0, S2P,         0 , 1)
>      FIELD(IDR0, S1P,         1 , 1)
>      FIELD(IDR0, TTF,         2 , 2)
>      FIELD(IDR0, COHACC,      4 , 1)
>      FIELD(IDR0, ASID16,      12, 1)
> +    FIELD(IDR0, VMID16,      18, 1)
>      FIELD(IDR0, TTENDIAN,    21, 2)
>      FIELD(IDR0, STALL_MODEL, 24, 2)
>      FIELD(IDR0, TERM_MODEL,  26, 1)

[RFC PATCH 02/16] hw/arm/smmuv3: Update translation config to hold stage-2

[Mostafa Saleh]

In preparation for adding stage-2 support. Add it's configuration.

They are added as SMMUS2Cfg in SMMUTransCfg, SMMUS2Cfg hold configs
parsed from STE:
 -tsz: Input range
 -sl0: start level of translation
 -affd: AF fault disable
 -granule_sz: Granule page shift
 -vmid: VMID
 -vttb: PA of translation table

They will be used in the next patches in stage-2 address translation.

No functional change intended.

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 include/hw/arm/smmu-common.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index c5683af07d..45f74d0e93 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -60,6 +60,16 @@ typedef struct SMMUTLBEntry {
     uint8_t granule;
 } SMMUTLBEntry;
 
+typedef struct SMMUS2Cfg {
+    uint8_t tsz;            /* Input range */
+    uint8_t sl0;            /* Start level of translation */
+    bool affd;              /* AF Fault Disable */
+    uint8_t granule_sz;     /* Granule page shift */
+    uint16_t vmid;          /* Virtual machine ID */
+    uint64_t vttb;          /* PA of translation table */
+} SMMUS2Cfg;
+
+
 /*
  * Generic structure populated by derived SMMU devices
  * after decoding the configuration information and used as
@@ -79,6 +89,7 @@ typedef struct SMMUTransCfg {
     SMMUTransTableInfo tt[2];
     uint32_t iotlb_hits;       /* counts IOTLB hits for this asid */
     uint32_t iotlb_misses;     /* counts IOTLB misses for this asid */
+    struct SMMUS2Cfg s2cfg;
 } SMMUTransCfg;
 
 typedef struct SMMUDevice {
-- 
2.39.1.519.gcb327c4b5f-goog

Re: [RFC PATCH 02/16] hw/arm/smmuv3: Update translation config to hold stage-2

[Eric Auger]

hi Mostafa,

On 2/5/23 10:43, Mostafa Saleh wrote:
> In preparation for adding stage-2 support. Add it's configuration.
replace "support. Add it's " by "support, add a S2 config struct,
composed of the following fields and embedded in the main TransCfg ../.."

>
> They are added as SMMUS2Cfg in SMMUTransCfg, SMMUS2Cfg hold configs
> parsed from STE:
>  -tsz: Input range
>  -sl0: start level of translation
>  -affd: AF fault disable
>  -granule_sz: Granule page shift
>  -vmid: VMID
>  -vttb: PA of translation table
>
> They will be used in the next patches in stage-2 address translation.
>
> No functional change intended.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  include/hw/arm/smmu-common.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index c5683af07d..45f74d0e93 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -60,6 +60,16 @@ typedef struct SMMUTLBEntry {
>      uint8_t granule;
>  } SMMUTLBEntry;
>  
> +typedef struct SMMUS2Cfg {
> +    uint8_t tsz;            /* Input range */
> +    uint8_t sl0;            /* Start level of translation */
> +    bool affd;              /* AF Fault Disable */
> +    uint8_t granule_sz;     /* Granule page shift */
> +    uint16_t vmid;          /* Virtual machine ID */
> +    uint64_t vttb;          /* PA of translation table */
> +} SMMUS2Cfg;
> +
> +
>  /*
>   * Generic structure populated by derived SMMU devices
>   * after decoding the configuration information and used as
> @@ -79,6 +89,7 @@ typedef struct SMMUTransCfg {
>      SMMUTransTableInfo tt[2];
>      uint32_t iotlb_hits;       /* counts IOTLB hits for this asid */
>      uint32_t iotlb_misses;     /* counts IOTLB misses for this asid */
> +    struct SMMUS2Cfg s2cfg;
>  } SMMUTransCfg;
>  
>  typedef struct SMMUDevice {
Eric

Re: [RFC PATCH 02/16] hw/arm/smmuv3: Update translation config to hold stage-2

[Mostafa Saleh]

Hi Eric,

Thanks for taking the time to review the patches!

On Wed, Feb 15, 2023 at 07:57:59PM +0100, Eric Auger wrote:
> > In preparation for adding stage-2 support. Add it's configuration.
> replace "support. Add it's " by "support, add a S2 config struct,
> composed of the following fields and embedded in the main TransCfg ../.."

I will update it in V2.


> >
> > They are added as SMMUS2Cfg in SMMUTransCfg, SMMUS2Cfg hold configs
> > parsed from STE:
> >  -tsz: Input range
> >  -sl0: start level of translation
> >  -affd: AF fault disable
> >  -granule_sz: Granule page shift
> >  -vmid: VMID
> >  -vttb: PA of translation table
> >
> > They will be used in the next patches in stage-2 address translation.
> >
> > No functional change intended.
> >
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > ---
> >  include/hw/arm/smmu-common.h | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> > index c5683af07d..45f74d0e93 100644
> > --- a/include/hw/arm/smmu-common.h
> > +++ b/include/hw/arm/smmu-common.h
> > @@ -60,6 +60,16 @@ typedef struct SMMUTLBEntry {
> >      uint8_t granule;
> >  } SMMUTLBEntry;
> >  
> > +typedef struct SMMUS2Cfg {
> > +    uint8_t tsz;            /* Input range */
> > +    uint8_t sl0;            /* Start level of translation */
> > +    bool affd;              /* AF Fault Disable */
> > +    uint8_t granule_sz;     /* Granule page shift */
> > +    uint16_t vmid;          /* Virtual machine ID */
> > +    uint64_t vttb;          /* PA of translation table */
> > +} SMMUS2Cfg;
> > +
> > +
> >  /*
> >   * Generic structure populated by derived SMMU devices
> >   * after decoding the configuration information and used as
> > @@ -79,6 +89,7 @@ typedef struct SMMUTransCfg {
> >      SMMUTransTableInfo tt[2];
> >      uint32_t iotlb_hits;       /* counts IOTLB hits for this asid */
> >      uint32_t iotlb_misses;     /* counts IOTLB misses for this asid */
> > +    struct SMMUS2Cfg s2cfg;
> >  } SMMUTransCfg;
> >  
> >  typedef struct SMMUDevice {
> Eric

Thanks,
Mostafa

[RFC PATCH 03/16] hw/arm/smmuv3: Rename smmu_ptw_64

[Mostafa Saleh]

In preparation for adding stage-2 support. Rename smmu_ptw_64 to
smmu_ptw_64_s1.

No functional change intended.

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 hw/arm/smmu-common.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 54186f31cb..4fcbffa2f1 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -264,7 +264,7 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
 }
 
 /**
- * smmu_ptw_64 - VMSAv8-64 Walk of the page tables for a given IOVA
+ * smmu_ptw_64_s1 - VMSAv8-64 Walk of the page tables for a given IOVA
  * @cfg: translation config
  * @iova: iova to translate
  * @perm: access type
@@ -276,9 +276,9 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
  * Upon success, @tlbe is filled with translated_addr and entry
  * permission rights.
  */
-static int smmu_ptw_64(SMMUTransCfg *cfg,
-                       dma_addr_t iova, IOMMUAccessFlags perm,
-                       SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
+static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
+                          dma_addr_t iova, IOMMUAccessFlags perm,
+                          SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
 {
     dma_addr_t baseaddr, indexmask;
     int stage = cfg->stage;
@@ -384,7 +384,7 @@ int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
         g_assert_not_reached();
     }
 
-    return smmu_ptw_64(cfg, iova, perm, tlbe, info);
+    return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
 }
 
 /**
-- 
2.39.1.519.gcb327c4b5f-goog

Re: [RFC PATCH 03/16] hw/arm/smmuv3: Rename smmu_ptw_64

[Eric Auger]

On 2/5/23 10:43, Mostafa Saleh wrote:
> In preparation for adding stage-2 support. Rename smmu_ptw_64 to
> smmu_ptw_64_s1.
>
> No functional change intended.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  hw/arm/smmu-common.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 54186f31cb..4fcbffa2f1 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -264,7 +264,7 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
>  }
>  
>  /**
> - * smmu_ptw_64 - VMSAv8-64 Walk of the page tables for a given IOVA
> + * smmu_ptw_64_s1 - VMSAv8-64 Walk of the page tables for a given IOVA
>   * @cfg: translation config
>   * @iova: iova to translate
>   * @perm: access type
> @@ -276,9 +276,9 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
>   * Upon success, @tlbe is filled with translated_addr and entry
>   * permission rights.
>   */
> -static int smmu_ptw_64(SMMUTransCfg *cfg,
> -                       dma_addr_t iova, IOMMUAccessFlags perm,
> -                       SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> +static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
> +                          dma_addr_t iova, IOMMUAccessFlags perm,
> +                          SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
>  {
>      dma_addr_t baseaddr, indexmask;
>      int stage = cfg->stage;
> @@ -384,7 +384,7 @@ int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
>          g_assert_not_reached();
>      }
>  
> -    return smmu_ptw_64(cfg, iova, perm, tlbe, info);
> +    return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
May need to rename the trace points as well

Thanks

Eric
>  }
>  
>  /**

Re: [RFC PATCH 03/16] hw/arm/smmuv3: Rename smmu_ptw_64

[Mostafa Saleh]

On Wed, Feb 15, 2023 at 05:53:23PM +0100, Eric Auger wrote:
> > +                          SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> >  {
> >      dma_addr_t baseaddr, indexmask;
> >      int stage = cfg->stage;
> > @@ -384,7 +384,7 @@ int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
> >          g_assert_not_reached();
> >      }
> >  
> > -    return smmu_ptw_64(cfg, iova, perm, tlbe, info);
> > +    return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
> May need to rename the trace points as well
> 
All PTW trace points have a stage argument which is set correctly
from stage-1/stage-2 ptw functions:
trace_smmu_ptw_page_pte
trace_smmu_ptw_block_pte
trace_smmu_ptw_invalid_pte

trace_smmu_ptw_level is the only one that had no stage argument, I can
change it to be consistent with the others.

Thanks,
Mostafa

Re: [RFC PATCH 03/16] hw/arm/smmuv3: Rename smmu_ptw_64

[Eric Auger]

On 2/16/23 13:56, Mostafa Saleh wrote:
> On Wed, Feb 15, 2023 at 05:53:23PM +0100, Eric Auger wrote:
>>> +                          SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
>>>  {
>>>      dma_addr_t baseaddr, indexmask;
>>>      int stage = cfg->stage;
>>> @@ -384,7 +384,7 @@ int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
>>>          g_assert_not_reached();
>>>      }
>>>  
>>> -    return smmu_ptw_64(cfg, iova, perm, tlbe, info);
>>> +    return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
>> May need to rename the trace points as well
>>
> All PTW trace points have a stage argument which is set correctly
> from stage-1/stage-2 ptw functions:
> trace_smmu_ptw_page_pte
> trace_smmu_ptw_block_pte
> trace_smmu_ptw_invalid_pte
Ah OK. Forgot that.
>
> trace_smmu_ptw_level is the only one that had no stage argument, I can
> change it to be consistent with the others.

OK

Thanks

Eric
>
> Thanks,
> Mostafa
>

[RFC PATCH 04/16] hw/arm/smmuv3: Add a system property to choose translation stage

[Mostafa Saleh]

Add a new system property for smmuv3 to choose what translation
stages to advertise.

The property added arm-smmuv3.stage can have 3 values:
- "1": Stage-1 is only advertised.
- "2": Stage-2 is only advertised.
- "all": Stage-1 + Stage-2 are supported, which is not implemented in
this patch series.

If not passed or an unsupported value is passed, it will default to
stage-1.

The property is not used in this patch as stage-2 has not been
enabled yet.

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 hw/arm/smmuv3-internal.h |  5 +++++
 hw/arm/smmuv3.c          | 28 +++++++++++++++++++++++++++-
 include/hw/arm/smmuv3.h  |  1 +
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index 170e88c24a..ec64fb43a0 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -329,6 +329,11 @@ enum { /* Command completion notification */
         })
 
 #define SMMU_FEATURE_2LVL_STE (1 << 0)
+#define SMMU_FEATURE_STAGE1   (1 << 1)
+#define SMMU_FEATURE_STAGE2   (1 << 2)
+
+#define STAGE1_SUPPORTED(f)   (f & SMMU_FEATURE_STAGE1)
+#define STAGE2_SUPPORTED(f)   (f & SMMU_FEATURE_STAGE2)
 
 /* Events */
 
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 955b89c8d5..54dd8e5ec1 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -21,6 +21,7 @@
 #include "hw/irq.h"
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
+#include "hw/qdev-properties.h"
 #include "hw/qdev-core.h"
 #include "hw/pci/pci.h"
 #include "cpu.h"
@@ -238,6 +239,19 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
 
 static void smmuv3_init_regs(SMMUv3State *s)
 {
+    /*
+     * Based on system property, the stages supported in smmu will be advertised.
+     * At the moment "all" is not supported.
+     * Default stage is 1.
+     */
+    s->features = SMMU_FEATURE_STAGE1;
+    if (s->stage && !strcmp("2", s->stage)) {
+        s->features = SMMU_FEATURE_STAGE2;
+    } else if (s->stage && !strcmp("all", s->stage)) {
+        qemu_log_mask(LOG_UNIMP,
+                          "SMMUv3 S1 and S2 nesting not supported, defaulting to S1\n");
+    }
+
     /**
      * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
      *       multi-level stream table
@@ -276,7 +290,6 @@ static void smmuv3_init_regs(SMMUv3State *s)
     s->eventq.cons = 0;
     s->eventq.entry_size = sizeof(struct Evt);
 
-    s->features = 0;
     s->sid_split = 0;
     s->aidr = 0x1;
     s->cr[0] = 0;
@@ -1514,6 +1527,18 @@ static const VMStateDescription vmstate_smmuv3 = {
     },
 };
 
+static Property smmuv3_properties[] = {
+    /*
+     * Stages of translation advertised.
+     * "1": Stage 1
+     * "2": Stage 2
+     * "all": Stage 1 + Stage 2
+     * Defaults to stage 1
+     */
+    DEFINE_PROP_STRING("stage", SMMUv3State, stage),
+    DEFINE_PROP_END_OF_LIST()
+};
+
 static void smmuv3_instance_init(Object *obj)
 {
     /* Nothing much to do here as of now */
@@ -1530,6 +1555,7 @@ static void smmuv3_class_init(ObjectClass *klass, void *data)
                                        &c->parent_phases);
     c->parent_realize = dc->realize;
     dc->realize = smmu_realize;
+    device_class_set_props(dc, smmuv3_properties);
 }
 
 static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
index f1921fdf9e..9b9e1c7baf 100644
--- a/include/hw/arm/smmuv3.h
+++ b/include/hw/arm/smmuv3.h
@@ -62,6 +62,7 @@ struct SMMUv3State {
 
     qemu_irq     irq[4];
     QemuMutex mutex;
+    char *stage;
 };
 
 typedef enum {
-- 
2.39.1.519.gcb327c4b5f-goog

Re: [RFC PATCH 04/16] hw/arm/smmuv3: Add a system property to choose translation stage

[Eric Auger]

Hi,

On 2/5/23 10:43, Mostafa Saleh wrote:
> Add a new system property for smmuv3 to choose what translation
> stages to advertise.
>
> The property added arm-smmuv3.stage can have 3 values:
> - "1": Stage-1 is only advertised.
Stage-1 only is advertised
> - "2": Stage-2 is only advertised.
> - "all": Stage-1 + Stage-2 are supported, which is not implemented in
> this patch series.
>
> If not passed or an unsupported value is passed, it will default to
> stage-1.
>
> The property is not used in this patch as stage-2 has not been
> enabled yet.
Usually the user knob (ie. the property) is introduced at the last
stage, ie. at 16/16.

Thanks

Eric
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  hw/arm/smmuv3-internal.h |  5 +++++
>  hw/arm/smmuv3.c          | 28 +++++++++++++++++++++++++++-
>  include/hw/arm/smmuv3.h  |  1 +
>  3 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index 170e88c24a..ec64fb43a0 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -329,6 +329,11 @@ enum { /* Command completion notification */
>          })
>  
>  #define SMMU_FEATURE_2LVL_STE (1 << 0)
> +#define SMMU_FEATURE_STAGE1   (1 << 1)
> +#define SMMU_FEATURE_STAGE2   (1 << 2)
> +
> +#define STAGE1_SUPPORTED(f)   (f & SMMU_FEATURE_STAGE1)
> +#define STAGE2_SUPPORTED(f)   (f & SMMU_FEATURE_STAGE2)
>  
>  /* Events */
>  
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 955b89c8d5..54dd8e5ec1 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -21,6 +21,7 @@
>  #include "hw/irq.h"
>  #include "hw/sysbus.h"
>  #include "migration/vmstate.h"
> +#include "hw/qdev-properties.h"
>  #include "hw/qdev-core.h"
>  #include "hw/pci/pci.h"
>  #include "cpu.h"
> @@ -238,6 +239,19 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
>  
>  static void smmuv3_init_regs(SMMUv3State *s)
>  {
> +    /*
> +     * Based on system property, the stages supported in smmu will be advertised.
> +     * At the moment "all" is not supported.
> +     * Default stage is 1.
> +     */
> +    s->features = SMMU_FEATURE_STAGE1;
> +    if (s->stage && !strcmp("2", s->stage)) {
> +        s->features = SMMU_FEATURE_STAGE2;
> +    } else if (s->stage && !strcmp("all", s->stage)) {
> +        qemu_log_mask(LOG_UNIMP,
> +                          "SMMUv3 S1 and S2 nesting not supported, defaulting to S1\n");
> +    }
> +
>      /**
>       * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
>       *       multi-level stream table
> @@ -276,7 +290,6 @@ static void smmuv3_init_regs(SMMUv3State *s)
>      s->eventq.cons = 0;
>      s->eventq.entry_size = sizeof(struct Evt);
>  
> -    s->features = 0;
>      s->sid_split = 0;
>      s->aidr = 0x1;
>      s->cr[0] = 0;
> @@ -1514,6 +1527,18 @@ static const VMStateDescription vmstate_smmuv3 = {
>      },
>  };
>  
> +static Property smmuv3_properties[] = {
> +    /*
> +     * Stages of translation advertised.
> +     * "1": Stage 1
> +     * "2": Stage 2
> +     * "all": Stage 1 + Stage 2
> +     * Defaults to stage 1
> +     */
> +    DEFINE_PROP_STRING("stage", SMMUv3State, stage),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
>  static void smmuv3_instance_init(Object *obj)
>  {
>      /* Nothing much to do here as of now */
> @@ -1530,6 +1555,7 @@ static void smmuv3_class_init(ObjectClass *klass, void *data)
>                                         &c->parent_phases);
>      c->parent_realize = dc->realize;
>      dc->realize = smmu_realize;
> +    device_class_set_props(dc, smmuv3_properties);
>  }
>  
>  static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
> diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
> index f1921fdf9e..9b9e1c7baf 100644
> --- a/include/hw/arm/smmuv3.h
> +++ b/include/hw/arm/smmuv3.h
> @@ -62,6 +62,7 @@ struct SMMUv3State {
>  
>      qemu_irq     irq[4];
>      QemuMutex mutex;
> +    char *stage;
>  };
>  
>  typedef enum {

Re: [RFC PATCH 04/16] hw/arm/smmuv3: Add a system property to choose translation stage

[Mostafa Saleh]

On Wed, Feb 15, 2023 at 05:29:04PM +0100, Eric Auger wrote:
> >
> > The property added arm-smmuv3.stage can have 3 values:
> > - "1": Stage-1 is only advertised.
> Stage-1 only is advertised

I will update it.

> > - "2": Stage-2 is only advertised.
> > - "all": Stage-1 + Stage-2 are supported, which is not implemented in
> > this patch series.
> >
> > If not passed or an unsupported value is passed, it will default to
> > stage-1.
> >
> > The property is not used in this patch as stage-2 has not been
> > enabled yet.
> Usually the user knob (ie. the property) is introduced at the last
> stage, ie. at 16/16.
I will split this commit, move the knobs to the end and keep features
definition as it is used by other commits.
I think it would also make sense to merge the knobs commit with last
one([RFC PATCH 16/16] hw/arm/smmuv3: Enable stage-2 support).


Thanks,
Mostafa

Re: [RFC PATCH 04/16] hw/arm/smmuv3: Add a system property to choose translation stage

[Eric Auger]

On 2/16/23 13:58, Mostafa Saleh wrote:
> On Wed, Feb 15, 2023 at 05:29:04PM +0100, Eric Auger wrote:
>>> The property added arm-smmuv3.stage can have 3 values:
>>> - "1": Stage-1 is only advertised.
>> Stage-1 only is advertised
> I will update it.
>
>>> - "2": Stage-2 is only advertised.
>>> - "all": Stage-1 + Stage-2 are supported, which is not implemented in
>>> this patch series.
>>>
>>> If not passed or an unsupported value is passed, it will default to
>>> stage-1.
>>>
>>> The property is not used in this patch as stage-2 has not been
>>> enabled yet.
>> Usually the user knob (ie. the property) is introduced at the last
>> stage, ie. at 16/16.
> I will split this commit, move the knobs to the end and keep features
> definition as it is used by other commits.
> I think it would also make sense to merge the knobs commit with last
> one([RFC PATCH 16/16] hw/arm/smmuv3: Enable stage-2 support).

OK!

Eric
>
>
> Thanks,
> Mostafa
>

[RFC PATCH 05/16] hw/arm/smmuv3: Add page table walk for stage-2

[Mostafa Saleh]

In preparation for adding stage-2 support. Add Stage-2 PTW code.
Only Aarch64 fromat is supported as stage-1.
Max 48 bits IPA is supported.

Nesting stage-1 and stage-2 is not supported right now.

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 hw/arm/smmu-common.c         | 112 ++++++++++++++++++++++++++++++++---
 hw/arm/smmu-internal.h       |  37 ++++++++++++
 include/hw/arm/smmu-common.h |   1 +
 3 files changed, 143 insertions(+), 7 deletions(-)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 4fcbffa2f1..df0d1dc024 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -362,6 +362,99 @@ error:
     return -EINVAL;
 }
 
+/**
+ * smmu_ptw_64_s2 - VMSAv8-64 Walk of the page tables for a given IOVA
+ * for stage-2.
+ * @cfg: translation config
+ * @iova: iova to translate
+ * @perm: access type
+ * @tlbe: SMMUTLBEntry (out)
+ * @info: handle to an error info
+ *
+ * Return 0 on success, < 0 on error. In case of error, @info is filled
+ * and tlbe->perm is set to IOMMU_NONE.
+ * Upon success, @tlbe is filled with translated_addr and entry
+ * permission rights.
+ */
+
+static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
+                          dma_addr_t iova, IOMMUAccessFlags perm,
+                          SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
+{
+    const int stage = 2;
+    int granule_sz = cfg->s2cfg.granule_sz;
+    /* ARM ARM: Table D8-7. */
+    int inputsize = 64 - cfg->s2cfg.tsz;
+    int level = get_start_level(cfg->s2cfg.sl0, granule_sz);
+    int stride = granule_sz - 3;
+    int idx = pgd_idx(level, granule_sz, iova);
+    /*
+     * Get the ttb from concatenated structure.
+     * The offset is the idx * size of each ttb(number of ptes * (sizeof(pte))
+     */
+    uint64_t baseaddr = extract64(cfg->s2cfg.vttb, 0, 48) + (1 << stride) *
+                        idx * sizeof(uint64_t);
+    dma_addr_t indexmask = (1ULL << (inputsize - (stride * (4 - level)))) - 1;
+
+    baseaddr &= ~indexmask;
+
+    while (level < SMMU_MAX_LEVELS) {
+        uint64_t subpage_size = 1ULL << level_shift(level, granule_sz);
+        uint64_t mask = subpage_size - 1;
+        uint32_t offset = iova_level_offset(iova, inputsize, level, granule_sz);
+        uint64_t pte, gpa;
+        dma_addr_t pte_addr = baseaddr + offset * sizeof(pte);
+        uint8_t ap;
+
+        if (get_pte(baseaddr, offset, &pte, info)) {
+                goto error;
+        }
+        trace_smmu_ptw_level(level, iova, subpage_size,
+                             baseaddr, offset, pte);
+        if (is_invalid_pte(pte) || is_reserved_pte(pte, level)) {
+            trace_smmu_ptw_invalid_pte(stage, level, baseaddr,
+                                       pte_addr, offset, pte);
+            break;
+        }
+
+        if (is_table_pte(pte, level)) {
+            baseaddr = get_table_pte_address(pte, granule_sz);
+            level++;
+            continue;
+        } else if (is_page_pte(pte, level)) {
+            gpa = get_page_pte_address(pte, granule_sz);
+            trace_smmu_ptw_page_pte(stage, level, iova,
+                                    baseaddr, pte_addr, pte, gpa);
+        } else {
+            uint64_t block_size;
+
+            gpa = get_block_pte_address(pte, level, granule_sz,
+                                        &block_size);
+            trace_smmu_ptw_block_pte(stage, level, baseaddr,
+                                     pte_addr, pte, iova, gpa,
+                                     block_size >> 20);
+        }
+        ap = PTE_AP(pte);
+        if (is_permission_fault_s2(ap, perm)) {
+            info->type = SMMU_PTW_ERR_PERMISSION;
+            goto error;
+        }
+
+        tlbe->entry.translated_addr = gpa;
+        tlbe->entry.iova = iova & ~mask;
+        tlbe->entry.addr_mask = mask;
+        tlbe->entry.perm = ap;
+        tlbe->level = level;
+        tlbe->granule = granule_sz;
+        return 0;
+    }
+    info->type = SMMU_PTW_ERR_TRANSLATION;
+
+error:
+    tlbe->entry.perm = IOMMU_NONE;
+    return -EINVAL;
+}
+
 /**
  * smmu_ptw - Walk the page tables for an IOVA, according to @cfg
  *
@@ -376,15 +469,20 @@ error:
 int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
              SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
 {
-    if (!cfg->aa64) {
-        /*
-         * This code path is not entered as we check this while decoding
-         * the configuration data in the derived SMMU model.
-         */
-        g_assert_not_reached();
+    if (cfg->stage == 1) {
+        if (!cfg->aa64) {
+            /*
+             * This code path is not entered as we check this while decoding
+             * the configuration data in the derived SMMU model.
+             */
+            g_assert_not_reached();
+        }
+        return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
+    } else if (cfg->stage == 2) {
+        return smmu_ptw_64_s2(cfg, iova, perm, tlbe, info);
     }
 
-    return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
+    g_assert_not_reached();
 }
 
 /**
diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h
index 2d75b31953..b02c05319f 100644
--- a/hw/arm/smmu-internal.h
+++ b/hw/arm/smmu-internal.h
@@ -73,6 +73,9 @@
 #define is_permission_fault(ap, perm) \
     (((perm) & IOMMU_WO) && ((ap) & 0x2))
 
+#define is_permission_fault_s2(ap, perm) \
+    (!((ap & perm) == perm))
+
 #define PTE_AP_TO_PERM(ap) \
     (IOMMU_ACCESS_FLAG(true, !((ap) & 0x2)))
 
@@ -96,6 +99,40 @@ uint64_t iova_level_offset(uint64_t iova, int inputsize,
             MAKE_64BIT_MASK(0, gsz - 3);
 }
 
+#define SMMU_MAX_S2_CONCAT    16
+
+/*
+ * Relies on correctness of gran and sl0 from caller.
+ * FEAT_LPA2 and FEAT_TTST are not implemented.
+ */
+static inline int get_start_level(int sl0 , int gran)
+{
+    /* ARM ARM: Table D8-12. */
+    if (gran == 12) {
+        return 2 - sl0;
+    }
+    /* ARM ARM: Table D8-22 and Table D8-31. */
+    return 3 - sl0;
+}
+
+/*
+ * Index in a concatenated first level stage-2 page table.
+ * ARM ARM: D8.2.2 Concatenated translation tables.
+ */
+static inline int pgd_idx(int start_level, int granule, dma_addr_t iova)
+{
+    uint64_t ret;
+    /*
+     * Get the number of bits handled by next levels, then any extra bits in
+     * the address should index the concatenated tables. This relation can
+     * deduced from tables in ARM ARM: D8.2.7-9
+     */
+    int shift = (SMMU_MAX_LEVELS - start_level) * (granule - 3) + granule;
+
+    ret = iova >> shift;
+    return ret;
+}
+
 #define SMMU_IOTLB_ASID(key) ((key).asid)
 
 typedef struct SMMUIOTLBPageInvInfo {
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 45f74d0e93..1e666e8b6d 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -28,6 +28,7 @@
 #define SMMU_PCI_DEVFN(sid)   (sid & 0xFF)
 
 #define SMMU_MAX_VA_BITS      48
+#define SMMU_MAX_LEVELS       4
 
 /*
  * Page table walk error types
-- 
2.39.1.519.gcb327c4b5f-goog

Re: [RFC PATCH 05/16] hw/arm/smmuv3: Add page table walk for stage-2

[Eric Auger]

Hi Mostafa,

On 2/5/23 10:44, Mostafa Saleh wrote:
> In preparation for adding stage-2 support. Add Stage-2 PTW code.
> Only Aarch64 fromat is supported as stage-1.
format
> Max 48 bits IPA is supported.
>
> Nesting stage-1 and stage-2 is not supported right now.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  hw/arm/smmu-common.c         | 112 ++++++++++++++++++++++++++++++++---
>  hw/arm/smmu-internal.h       |  37 ++++++++++++
>  include/hw/arm/smmu-common.h |   1 +
>  3 files changed, 143 insertions(+), 7 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 4fcbffa2f1..df0d1dc024 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -362,6 +362,99 @@ error:
>      return -EINVAL;
>  }
>  
> +/**
> + * smmu_ptw_64_s2 - VMSAv8-64 Walk of the page tables for a given IOVA
> + * for stage-2.
> + * @cfg: translation config
> + * @iova: iova to translate
> + * @perm: access type
> + * @tlbe: SMMUTLBEntry (out)
> + * @info: handle to an error info
> + *
> + * Return 0 on success, < 0 on error. In case of error, @info is filled
> + * and tlbe->perm is set to IOMMU_NONE.
> + * Upon success, @tlbe is filled with translated_addr and entry
> + * permission rights.
> + */
> +
> +static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
> +                          dma_addr_t iova, IOMMUAccessFlags perm,
> +                          SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> +{
> +    const int stage = 2;
> +    int granule_sz = cfg->s2cfg.granule_sz;
> +    /* ARM ARM: Table D8-7. */
> +    int inputsize = 64 - cfg->s2cfg.tsz;
> +    int level = get_start_level(cfg->s2cfg.sl0, granule_sz);
> +    int stride = granule_sz - 3;
> +    int idx = pgd_idx(level, granule_sz, iova);
> +    /*
> +     * Get the ttb from concatenated structure.
> +     * The offset is the idx * size of each ttb(number of ptes * (sizeof(pte))
> +     */
> +    uint64_t baseaddr = extract64(cfg->s2cfg.vttb, 0, 48) + (1 << stride) *
> +                        idx * sizeof(uint64_t);
> +    dma_addr_t indexmask = (1ULL << (inputsize - (stride * (4 - level)))) - 1;
> +
> +    baseaddr &= ~indexmask;
> +
> +    while (level < SMMU_MAX_LEVELS) {
> +        uint64_t subpage_size = 1ULL << level_shift(level, granule_sz);
> +        uint64_t mask = subpage_size - 1;
> +        uint32_t offset = iova_level_offset(iova, inputsize, level, granule_sz);
> +        uint64_t pte, gpa;
> +        dma_addr_t pte_addr = baseaddr + offset * sizeof(pte);
> +        uint8_t ap;
> +
> +        if (get_pte(baseaddr, offset, &pte, info)) {
> +                goto error;
> +        }
> +        trace_smmu_ptw_level(level, iova, subpage_size,
> +                             baseaddr, offset, pte);
I can the trace point names should be updated as well (and
differentiated between S1/S2)
> +        if (is_invalid_pte(pte) || is_reserved_pte(pte, level)) {
> +            trace_smmu_ptw_invalid_pte(stage, level, baseaddr,
> +                                       pte_addr, offset, pte);
same for PTE's?
> +            break;
> +        }
> +
> +        if (is_table_pte(pte, level)) {
> +            baseaddr = get_table_pte_address(pte, granule_sz);
> +            level++;
> +            continue;
> +        } else if (is_page_pte(pte, level)) {
> +            gpa = get_page_pte_address(pte, granule_sz);
> +            trace_smmu_ptw_page_pte(stage, level, iova,
> +                                    baseaddr, pte_addr, pte, gpa);
> +        } else {
> +            uint64_t block_size;
> +
> +            gpa = get_block_pte_address(pte, level, granule_sz,
> +                                        &block_size);
> +            trace_smmu_ptw_block_pte(stage, level, baseaddr,
> +                                     pte_addr, pte, iova, gpa,
> +                                     block_size >> 20);
> +        }
> +        ap = PTE_AP(pte);
> +        if (is_permission_fault_s2(ap, perm)) {
> +            info->type = SMMU_PTW_ERR_PERMISSION;
don't we have to different S1 versus S2 faults?
> +            goto error;
> +        }
> +
> +        tlbe->entry.translated_addr = gpa;
> +        tlbe->entry.iova = iova & ~mask;
> +        tlbe->entry.addr_mask = mask;
> +        tlbe->entry.perm = ap;
> +        tlbe->level = level;
> +        tlbe->granule = granule_sz;
> +        return 0;
> +    }
> +    info->type = SMMU_PTW_ERR_TRANSLATION;
> +
> +error:
> +    tlbe->entry.perm = IOMMU_NONE;
> +    return -EINVAL;
> +}
> +
>  /**
>   * smmu_ptw - Walk the page tables for an IOVA, according to @cfg
>   *
> @@ -376,15 +469,20 @@ error:
>  int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
>               SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
>  {
> -    if (!cfg->aa64) {
> -        /*
> -         * This code path is not entered as we check this while decoding
> -         * the configuration data in the derived SMMU model.
> -         */
> -        g_assert_not_reached();
if that's still true for S2, maybe keep that check here upfront?
> +    if (cfg->stage == 1) {
> +        if (!cfg->aa64) {
> +            /*
> +             * This code path is not entered as we check this while decoding
> +             * the configuration data in the derived SMMU model.
> +             */
> +            g_assert_not_reached();
> +        }
> +        return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
> +    } else if (cfg->stage == 2) {
> +        return smmu_ptw_64_s2(cfg, iova, perm, tlbe, info);
>      }
>  
> -    return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
> +    g_assert_not_reached();
>  }
>  
>  /**
> diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h
> index 2d75b31953..b02c05319f 100644
> --- a/hw/arm/smmu-internal.h
> +++ b/hw/arm/smmu-internal.h
> @@ -73,6 +73,9 @@
>  #define is_permission_fault(ap, perm) \
>      (((perm) & IOMMU_WO) && ((ap) & 0x2))
>  
> +#define is_permission_fault_s2(ap, perm) \
> +    (!((ap & perm) == perm))
> +
>  #define PTE_AP_TO_PERM(ap) \
>      (IOMMU_ACCESS_FLAG(true, !((ap) & 0x2)))
>  
> @@ -96,6 +99,40 @@ uint64_t iova_level_offset(uint64_t iova, int inputsize,
>              MAKE_64BIT_MASK(0, gsz - 3);
>  }
>  
> +#define SMMU_MAX_S2_CONCAT    16
> +
> +/*
> + * Relies on correctness of gran and sl0 from caller.
> + * FEAT_LPA2 and FEAT_TTST are not implemented.
> + */
> +static inline int get_start_level(int sl0 , int gran)
> +{
> +    /* ARM ARM: Table D8-12. */
> +    if (gran == 12) {
> +        return 2 - sl0;
> +    }
> +    /* ARM ARM: Table D8-22 and Table D8-31. */
> +    return 3 - sl0;
> +}
> +
> +/*
> + * Index in a concatenated first level stage-2 page table.
> + * ARM ARM: D8.2.2 Concatenated translation tables.
> + */
> +static inline int pgd_idx(int start_level, int granule, dma_addr_t iova)
> +{
> +    uint64_t ret;
> +    /*
> +     * Get the number of bits handled by next levels, then any extra bits in
> +     * the address should index the concatenated tables. This relation can
> +     * deduced from tables in ARM ARM: D8.2.7-9
> +     */
> +    int shift = (SMMU_MAX_LEVELS - start_level) * (granule - 3) + granule;
can't we factorize anything with the S1 PTW?
indexmask = (1ULL << (inputsize - (stride * (4 - level)))) - 1;
> +
> +    ret = iova >> shift;
> +    return ret;
> +}
> +
>  #define SMMU_IOTLB_ASID(key) ((key).asid)
>  
>  typedef struct SMMUIOTLBPageInvInfo {
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 45f74d0e93..1e666e8b6d 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -28,6 +28,7 @@
>  #define SMMU_PCI_DEVFN(sid)   (sid & 0xFF)
>  
>  #define SMMU_MAX_VA_BITS      48
> +#define SMMU_MAX_LEVELS       4
can't this be reused as well with S1 PTW?
>  
>  /*
>   * Page table walk error types
Eric

Re: [RFC PATCH 05/16] hw/arm/smmuv3: Add page table walk for stage-2

[Mostafa Saleh]

Hi Eric,

On Wed, Feb 15, 2023 at 05:52:39PM +0100, Eric Auger wrote:
> > In preparation for adding stage-2 support. Add Stage-2 PTW code.
> > Only Aarch64 fromat is supported as stage-1.
> format
I will update it.

> > +        uint64_t subpage_size = 1ULL << level_shift(level, granule_sz);
> > +        uint64_t mask = subpage_size - 1;
> > +        uint32_t offset = iova_level_offset(iova, inputsize, level, granule_sz);
> > +        uint64_t pte, gpa;
> > +        dma_addr_t pte_addr = baseaddr + offset * sizeof(pte);
> > +        uint8_t ap;
> > +
> > +        if (get_pte(baseaddr, offset, &pte, info)) {
> > +                goto error;
> > +        }
> > +        trace_smmu_ptw_level(level, iova, subpage_size,
> > +                             baseaddr, offset, pte);
> I can the trace point names should be updated as well (and
> differentiated between S1/S2)
I was thinking we could leave those with stage argument, and only
update trace_smmu_ptw_level to have stage argument as the others.

> > +        if (is_permission_fault_s2(ap, perm)) {
> > +            info->type = SMMU_PTW_ERR_PERMISSION;
> don't we have to different S1 versus S2 faults?
Yes, I missed that, I see setting info->u.f_walk_eabt.s2 should be
enough, this will set the S2 field in the fault event.

> >  int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
> >               SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> >  {
> > -    if (!cfg->aa64) {
> > -        /*
> > -         * This code path is not entered as we check this while decoding
> > -         * the configuration data in the derived SMMU model.
> > -         */
> > -        g_assert_not_reached();
> if that's still true for S2, maybe keep that check here upfront?
Stage-2 is checked in STE parsing and throws BAD_STE if not aa64,
which I believe is not correct, however I think we can just call
g_assert_not_reached() during STE parsing, I don’t see added value for
saving this field in SMMUTransCfg if we don’t use it.
I am not sure why this check exists for stage-1 as it is hardcoded in
decode_cd anyway.

> > +{
> > +    uint64_t ret;
> > +    /*
> > +     * Get the number of bits handled by next levels, then any extra bits in
> > +     * the address should index the concatenated tables. This relation can
> > +     * deduced from tables in ARM ARM: D8.2.7-9
> > +     */
> > +    int shift = (SMMU_MAX_LEVELS - start_level) * (granule - 3) + granule;
> can't we factorize anything with the S1 PTW?
> indexmask = (1ULL << (inputsize - (stride * (4 - level)))) - 1;
Yes, I think we can refactor some of these in common functions/macros, I
will do that in v2.


> > @@ -28,6 +28,7 @@
> >  #define SMMU_PCI_DEVFN(sid)   (sid & 0xFF)
> >  
> >  #define SMMU_MAX_VA_BITS      48
> > +#define SMMU_MAX_LEVELS       4
> can't this be reused as well with S1 PTW?
I believe yes, I will update it.

Thanks,
Mostafa

Re: [RFC PATCH 05/16] hw/arm/smmuv3: Add page table walk for stage-2

[Eric Auger]

On 2/16/23 14:09, Mostafa Saleh wrote:
> Hi Eric,
>
> On Wed, Feb 15, 2023 at 05:52:39PM +0100, Eric Auger wrote:
>>> In preparation for adding stage-2 support. Add Stage-2 PTW code.
>>> Only Aarch64 fromat is supported as stage-1.
>> format
> I will update it.
>
>>> +        uint64_t subpage_size = 1ULL << level_shift(level, granule_sz);
>>> +        uint64_t mask = subpage_size - 1;
>>> +        uint32_t offset = iova_level_offset(iova, inputsize, level, granule_sz);
>>> +        uint64_t pte, gpa;
>>> +        dma_addr_t pte_addr = baseaddr + offset * sizeof(pte);
>>> +        uint8_t ap;
>>> +
>>> +        if (get_pte(baseaddr, offset, &pte, info)) {
>>> +                goto error;
>>> +        }
>>> +        trace_smmu_ptw_level(level, iova, subpage_size,
>>> +                             baseaddr, offset, pte);
>> I can the trace point names should be updated as well (and
>> differentiated between S1/S2)
> I was thinking we could leave those with stage argument, and only
> update trace_smmu_ptw_level to have stage argument as the others.
yes as far as the stage is properly populated that's fine.
>
>>> +        if (is_permission_fault_s2(ap, perm)) {
>>> +            info->type = SMMU_PTW_ERR_PERMISSION;
>> don't we have to different S1 versus S2 faults?
> Yes, I missed that, I see setting info->u.f_walk_eabt.s2 should be
> enough, this will set the S2 field in the fault event.
>
>>>  int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
>>>               SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
>>>  {
>>> -    if (!cfg->aa64) {
>>> -        /*
>>> -         * This code path is not entered as we check this while decoding
>>> -         * the configuration data in the derived SMMU model.
>>> -         */
>>> -        g_assert_not_reached();
>> if that's still true for S2, maybe keep that check here upfront?
> Stage-2 is checked in STE parsing and throws BAD_STE if not aa64,
> which I believe is not correct, however I think we can just call
> g_assert_not_reached() during STE parsing, I don’t see added value for
> saving this field in SMMUTransCfg if we don’t use it.
I agree. I guess we provisionned for this field in the prospect of
completing the emulation but I don't think we care.
> I am not sure why this check exists for stage-1 as it is hardcoded in
> decode_cd anyway.
>
>>> +{
>>> +    uint64_t ret;
>>> +    /*
>>> +     * Get the number of bits handled by next levels, then any extra bits in
>>> +     * the address should index the concatenated tables. This relation can
>>> +     * deduced from tables in ARM ARM: D8.2.7-9
>>> +     */
>>> +    int shift = (SMMU_MAX_LEVELS - start_level) * (granule - 3) + granule;
>> can't we factorize anything with the S1 PTW?
>> indexmask = (1ULL << (inputsize - (stride * (4 - level)))) - 1;
> Yes, I think we can refactor some of these in common functions/macros, I
> will do that in v2.
I guess that's a trade-off between beeing close enough to the spec and
maybe its pseudo-code and having both S1/S2 codes looking similar.

Eric
>
>
>>> @@ -28,6 +28,7 @@
>>>  #define SMMU_PCI_DEVFN(sid)   (sid & 0xFF)
>>>  
>>>  #define SMMU_MAX_VA_BITS      48
>>> +#define SMMU_MAX_LEVELS       4
>> can't this be reused as well with S1 PTW?
> I believe yes, I will update it.
>
> Thanks,
> Mostafa
>

[RFC PATCH 06/16] hw/arm/smmuv3: Parse STE config for stage-2

[Mostafa Saleh]

Parse stage-2 configuration and populate it in SMMUTransCfg.
Configs in this patch (s2g, ttb, tsz, sl0).
Checking validity of values added when possible.

MAX IPA supported is 48 bits and only AA64 tables are supported.

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 hw/arm/smmuv3.c              | 43 +++++++++++++++++++++++++++++++++++-
 include/hw/arm/smmu-common.h |  1 +
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 54dd8e5ec1..6633fe40fa 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -366,7 +366,48 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
         return 0;
     }
 
-    if (STE_CFG_S2_ENABLED(config)) {
+    if (STAGE2_SUPPORTED(s->features) && STE_CFG_S2_ENABLED(config)) {
+        cfg->stage = 2;
+
+        if (STE_S2AA64(ste) == 0x0) {
+            qemu_log_mask(LOG_UNIMP,
+                          "SMMUv3 AArch32 tables not supported\n");
+            goto bad_ste;
+        }
+
+        switch (STE_S2TG(ste)) {
+        case 0x0: /* 4KB */
+            cfg->s2cfg.granule_sz = 12;
+            break;
+        case 0x1: /* 64KB */
+            cfg->s2cfg.granule_sz = 16;
+            break;
+        case 0x2: /* 16KB */
+            cfg->s2cfg.granule_sz = 14;
+            break;
+        default:
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "SMMUv3 bad STE S2TG: %x\n", STE_S2TG(ste));
+            goto bad_ste;
+        }
+
+        cfg->s2cfg.vttb = STE_S2TTB(ste);
+        cfg->s2cfg.tsz = STE_S2T0SZ(ste);
+
+        if ((64 - cfg->s2cfg.tsz) > SMMU_MAX_IPA_BITS) {
+            qemu_log_mask(LOG_UNIMP, "SMMUv3 IPA too big! TS0Z = %x\n",
+                          cfg->s2cfg.tsz);
+            goto bad_ste;
+        }
+
+        cfg->s2cfg.sl0 = STE_S2SL0(ste);
+        if (cfg->s2cfg.sl0 == 0x3) {
+            qemu_log_mask(LOG_UNIMP,
+                          "SMMUv3 STE->SL0 0x3 has no meaning!\n");
+            goto bad_ste;
+        }
+
+        /* This is still here as stage 2 has not been fully enabled yet. */
         qemu_log_mask(LOG_UNIMP, "SMMUv3 does not support stage 2 yet\n");
         goto bad_ste;
     }
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 1e666e8b6d..7906e359d9 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -28,6 +28,7 @@
 #define SMMU_PCI_DEVFN(sid)   (sid & 0xFF)
 
 #define SMMU_MAX_VA_BITS      48
+#define SMMU_MAX_IPA_BITS     48
 #define SMMU_MAX_LEVELS       4
 
 /*
-- 
2.39.1.519.gcb327c4b5f-goog

Re: [RFC PATCH 06/16] hw/arm/smmuv3: Parse STE config for stage-2

[Eric Auger]

Hi Mostafa,

On 2/5/23 10:44, Mostafa Saleh wrote:
> Parse stage-2 configuration and populate it in SMMUTransCfg.
> Configs in this patch (s2g, ttb, tsz, sl0).
above 'sentence' a bit cryptic.
> Checking validity of values added when possible.
>
> MAX IPA supported is 48 bits and only AA64 tables are supported.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  hw/arm/smmuv3.c              | 43 +++++++++++++++++++++++++++++++++++-
>  include/hw/arm/smmu-common.h |  1 +
>  2 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 54dd8e5ec1..6633fe40fa 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -366,7 +366,48 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
>          return 0;
>      }
>  
> -    if (STE_CFG_S2_ENABLED(config)) {
> +    if (STAGE2_SUPPORTED(s->features) && STE_CFG_S2_ENABLED(config)) {
Don't you want to check both S1 and S2 aren't set?
> +        cfg->stage = 2;
> +
> +        if (STE_S2AA64(ste) == 0x0) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "SMMUv3 AArch32 tables not supported\n");
> +            goto bad_ste;
> +        }
> +
> +        switch (STE_S2TG(ste)) {
> +        case 0x0: /* 4KB */
> +            cfg->s2cfg.granule_sz = 12;
> +            break;
> +        case 0x1: /* 64KB */
> +            cfg->s2cfg.granule_sz = 16;
> +            break;
> +        case 0x2: /* 16KB */
> +            cfg->s2cfg.granule_sz = 14;
> +            break;
> +        default:
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "SMMUv3 bad STE S2TG: %x\n", STE_S2TG(ste));
> +            goto bad_ste;
> +        }
> +
> +        cfg->s2cfg.vttb = STE_S2TTB(ste);
> +        cfg->s2cfg.tsz = STE_S2T0SZ(ste);
What about IDR3.STT currently 0 so S2T0SZ <= 39

don't you need to check against SMMU_IDR3.STT/S2TG

• In architectures after SMMUv3.0:
– If STE.S2TG selects a 4KB or 16KB granule, the minimum valid value for
this field is MAX(16,
64-IAS).
– If STE.S2TG selects a 64KB granule, the minimum valid value for this
field is (64-IAS).
> +
> +        if ((64 - cfg->s2cfg.tsz) > SMMU_MAX_IPA_BITS) {
> +            qemu_log_mask(LOG_UNIMP, "SMMUv3 IPA too big! TS0Z = %x\n",
> +                          cfg->s2cfg.tsz);
> +            goto bad_ste;
> +        }
> +
> +        cfg->s2cfg.sl0 = STE_S2SL0(ste);
> +        if (cfg->s2cfg.sl0 == 0x3) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "SMMUv3 STE->SL0 0x3 has no meaning!\n");
> +            goto bad_ste;
what about S2PS, S2VMID?

you may either squash [RFC PATCH 11/16] hw/arm/smmuv3: Read VMID from
STE into that patch or at least mention in the commit msg that S2VMID
will be dealt with separately

Eric

> +        }
> +
> +        /* This is still here as stage 2 has not been fully enabled yet. */
>          qemu_log_mask(LOG_UNIMP, "SMMUv3 does not support stage 2 yet\n");
>          goto bad_ste;
>      }
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 1e666e8b6d..7906e359d9 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -28,6 +28,7 @@
>  #define SMMU_PCI_DEVFN(sid)   (sid & 0xFF)
>  
>  #define SMMU_MAX_VA_BITS      48
> +#define SMMU_MAX_IPA_BITS     48
>  #define SMMU_MAX_LEVELS       4
>  
>  /*

Re: [RFC PATCH 06/16] hw/arm/smmuv3: Parse STE config for stage-2

[Mostafa Saleh]

Hi Eric,

On Wed, Feb 15, 2023 at 06:47:52PM +0100, Eric Auger wrote:
> On 2/5/23 10:44, Mostafa Saleh wrote:
> > Parse stage-2 configuration and populate it in SMMUTransCfg.
> > Configs in this patch (s2g, ttb, tsz, sl0).
> above 'sentence' a bit cryptic.
I will reword it.

> > +++ b/hw/arm/smmuv3.c
> > @@ -366,7 +366,48 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
> >          return 0;
> >      }
> >  
> > -    if (STE_CFG_S2_ENABLED(config)) {
> > +    if (STAGE2_SUPPORTED(s->features) && STE_CFG_S2_ENABLED(config)) {
> Don't you want to check both S1 and S2 aren't set?
Yes, currently this is ignored, but looking at the SMMU manual it is
illegal to configure an unsupported stage, I will update it.

> > +            break;
> > +        case 0x2: /* 16KB */
> > +            cfg->s2cfg.granule_sz = 14;
> > +            break;
> > +        default:
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                          "SMMUv3 bad STE S2TG: %x\n", STE_S2TG(ste));
> > +            goto bad_ste;
> > +        }
> > +
> > +        cfg->s2cfg.vttb = STE_S2TTB(ste);
> > +        cfg->s2cfg.tsz = STE_S2T0SZ(ste);
> What about IDR3.STT currently 0 so S2T0SZ <= 39
> 
> don't you need to check against SMMU_IDR3.STT/S2TG
> 
> • In architectures after SMMUv3.0:
> – If STE.S2TG selects a 4KB or 16KB granule, the minimum valid value for
> this field is MAX(16,
> 64-IAS).
> – If STE.S2TG selects a 64KB granule, the minimum valid value for this
> field is (64-IAS).
I will add a function to validate S2T0SZ based on the behaviour after
SMMUv3.0 and checks against STT disabled. I believe it is safe just to
check S2T0SZ <= 39 without checking IDR3.STT as we don’t support it
for now.

> > +                          cfg->s2cfg.tsz);
> > +            goto bad_ste;
> > +        }
> > +
> > +        cfg->s2cfg.sl0 = STE_S2SL0(ste);
> > +        if (cfg->s2cfg.sl0 == 0x3) {
> > +            qemu_log_mask(LOG_UNIMP,
> > +                          "SMMUv3 STE->SL0 0x3 has no meaning!\n");
> > +            goto bad_ste;
> what about S2PS, S2VMID?
> 
> you may either squash [RFC PATCH 11/16] hw/arm/smmuv3: Read VMID from
> STE into that patch or at least mention in the commit msg that S2VMID
> will be dealt with separately
I will squash it with
"[RFC PATCH 11/16] hw/arm/smmuv3: Read VMID from STE"
I missed S2PS, I will add it in V2.

Thanks,
Mostafa

[RFC PATCH 07/16] hw/arm/smmuv3: Check validity of stage-2 page table

[Mostafa Saleh]

Check if with the configured start level, ia_bits and granularity we
can have a valid page table as described in ARM ARM D8.2 Translation
process.
The idea is to see for the highest possible number of IPA bits, how
many concatenated tables we would need, if it is more than 16, then
this is not possible.

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 hw/arm/smmuv3.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 6633fe40fa..c49b341287 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -341,6 +341,28 @@ static int smmu_get_cd(SMMUv3State *s, STE *ste, uint32_t ssid,
     return 0;
 }
 
+/*
+ * Return true if s2 page table config is valid.
+ * This checks with the configured start level, ia_bits and granularity we can
+ * have a valid page table as described in ARM ARM D8.2 Translation process.
+ * The idea here is to see for the highest possible number of IPA bits, how
+ * many concatenated tables we would need, if it is more than 16, then this is
+ * not possible.
+ */
+static bool s2_pgtable_config_valid(uint8_t sl0, uint8_t t0sz, uint8_t gran)
+{
+    int level = get_start_level(sl0, gran);
+    uint64_t ia_bits = 64 - t0sz;
+    uint64_t mx = (1ULL << ia_bits) - 1;
+    int nr_concat = pgd_idx(level, gran, mx) + 1;
+
+    if (nr_concat > SMMU_MAX_S2_CONCAT) {
+        return false;
+    }
+
+    return true;
+}
+
 /* Returns < 0 in case of invalid STE, 0 otherwise */
 static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
                       STE *ste, SMMUEventInfo *event)
@@ -407,6 +429,13 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
             goto bad_ste;
         }
 
+        if (!s2_pgtable_config_valid(cfg->s2cfg.sl0, cfg->s2cfg.tsz,
+                                     cfg->s2cfg.granule_sz)) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "SMMUv3 STE stage 2 config not valid!\n");
+            goto bad_ste;
+        }
+
         /* This is still here as stage 2 has not been fully enabled yet. */
         qemu_log_mask(LOG_UNIMP, "SMMUv3 does not support stage 2 yet\n");
         goto bad_ste;
-- 
2.39.1.519.gcb327c4b5f-goog

Re: [RFC PATCH 07/16] hw/arm/smmuv3: Check validity of stage-2 page table

[Eric Auger]

Hi Mostafa,
On 2/5/23 10:44, Mostafa Saleh wrote:
> Check if with the configured start level, ia_bits and granularity we
> can have a valid page table as described in ARM ARM D8.2 Translation
> process.
> The idea is to see for the highest possible number of IPA bits, how
> many concatenated tables we would need, if it is more than 16, then
> this is not possible.

This rather checks the validity and consistency of the STE S2 fields.
The patch title sounds a bit misleading to me.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  hw/arm/smmuv3.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 6633fe40fa..c49b341287 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -341,6 +341,28 @@ static int smmu_get_cd(SMMUv3State *s, STE *ste, uint32_t ssid,
>      return 0;
>  }
>  
> +/*
> + * Return true if s2 page table config is valid.
> + * This checks with the configured start level, ia_bits and granularity we can
> + * have a valid page table as described in ARM ARM D8.2 Translation process.
> + * The idea here is to see for the highest possible number of IPA bits, how
> + * many concatenated tables we would need, if it is more than 16, then this is
> + * not possible.
> + */
> +static bool s2_pgtable_config_valid(uint8_t sl0, uint8_t t0sz, uint8_t gran)
> +{
> +    int level = get_start_level(sl0, gran);
> +    uint64_t ia_bits = 64 - t0sz;
s/ia/ipa
> +    uint64_t mx = (1ULL << ia_bits) - 1;
s/mx/max_ipa
> +    int nr_concat = pgd_idx(level, gran, mx) + 1;
> +
> +    if (nr_concat > SMMU_MAX_S2_CONCAT) {
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
>  /* Returns < 0 in case of invalid STE, 0 otherwise */
>  static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
>                        STE *ste, SMMUEventInfo *event)
> @@ -407,6 +429,13 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
>              goto bad_ste;
>          }
>  
> +        if (!s2_pgtable_config_valid(cfg->s2cfg.sl0, cfg->s2cfg.tsz,
> +                                     cfg->s2cfg.granule_sz)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "SMMUv3 STE stage 2 config not valid!\n");
> +            goto bad_ste;
> +        }
> +
To me this would need to be integrated into the STE decoding patch. This
latter shall be self-contained if possible to ease the review

Thanks

Eric
>          /* This is still here as stage 2 has not been fully enabled yet. */
>          qemu_log_mask(LOG_UNIMP, "SMMUv3 does not support stage 2 yet\n");
>          goto bad_ste;

Re: [RFC PATCH 07/16] hw/arm/smmuv3: Check validity of stage-2 page table

[Mostafa Saleh]

Hi Eric,

> > can have a valid page table as described in ARM ARM D8.2 Translation
> > process.
> > The idea is to see for the highest possible number of IPA bits, how
> > many concatenated tables we would need, if it is more than 16, then
> > this is not possible.
> 
> This rather checks the validity and consistency of the STE S2 fields.
> The patch title sounds a bit misleading to me.
I will update the wording.

> > +static bool s2_pgtable_config_valid(uint8_t sl0, uint8_t t0sz, uint8_t gran)
> > +{
> > +    int level = get_start_level(sl0, gran);
> > +    uint64_t ia_bits = 64 - t0sz;
> s/ia/ipa
I will update it.

> > +    uint64_t mx = (1ULL << ia_bits) - 1;
> s/mx/max_ipa
I will update it.

> >  
> > +        if (!s2_pgtable_config_valid(cfg->s2cfg.sl0, cfg->s2cfg.tsz,
> > +                                     cfg->s2cfg.granule_sz)) {
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                          "SMMUv3 STE stage 2 config not valid!\n");
> > +            goto bad_ste;
> > +        }
> > +
> To me this would need to be integrated into the STE decoding patch. This
> latter shall be self-contained if possible to ease the review
I will squash it, I was trying to keep patches small, but it makes sense
to validate STE in the same patch parsing it.

Thanks,
Mostafa

[RFC PATCH 08/16] hw/arm/smmuv3: Support S2AFFD

[Mostafa Saleh]

Parse S2AFFD from STE and use it in stage-2 translation.

This is described in the SMMUv3 manual "5.2. Stream Table Entry" in
"[181] S2AFFD".

HTTU is not supported, SW is expected to maintain the Access flag.

This flag determines the behavior on access of a stage-2 page whose
descriptor has AF == 0:
- 0b0: An Access flag fault occurs (stall not supported).
- 0b1: An Access flag fault never occurs.

An Access fault takes priority over a Permission fault.

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 hw/arm/smmu-common.c     | 10 ++++++++++
 hw/arm/smmu-internal.h   |  2 ++
 hw/arm/smmuv3-internal.h |  1 +
 hw/arm/smmuv3.c          |  2 ++
 4 files changed, 15 insertions(+)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index df0d1dc024..541c427684 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -434,6 +434,16 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
                                      pte_addr, pte, iova, gpa,
                                      block_size >> 20);
         }
+
+        /*
+         * If S2AFFD and PTE.AF are 0 => fault. (5.2. Stream Table Entry)
+         * An Access fault takes priority over a Permission fault.
+         */
+        if (!PTE_AF(pte) && !cfg->s2cfg.affd) {
+            info->type = SMMU_PTW_ERR_ACCESS;
+            goto error;
+        }
+
         ap = PTE_AP(pte);
         if (is_permission_fault_s2(ap, perm)) {
             info->type = SMMU_PTW_ERR_PERMISSION;
diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h
index b02c05319f..7d3f76ce14 100644
--- a/hw/arm/smmu-internal.h
+++ b/hw/arm/smmu-internal.h
@@ -66,6 +66,8 @@
 #define PTE_APTABLE(pte) \
     (extract64(pte, 61, 2))
 
+#define PTE_AF(pte) \
+    (extract64(pte, 10, 1))
 /*
  * TODO: At the moment all transactions are considered as privileged (EL1)
  * as IOMMU translation callback does not pass user/priv attributes.
diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index ec64fb43a0..3ccb9d118e 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -524,6 +524,7 @@ typedef struct CD {
 #define STE_S2TG(x)        extract32((x)->word[5], 14, 2)
 #define STE_S2PS(x)        extract32((x)->word[5], 16, 3)
 #define STE_S2AA64(x)      extract32((x)->word[5], 19, 1)
+#define STE_S2AFFD(x)      extract32((x)->word[5], 21, 1)
 #define STE_S2HD(x)        extract32((x)->word[5], 24, 1)
 #define STE_S2HA(x)        extract32((x)->word[5], 25, 1)
 #define STE_S2S(x)         extract32((x)->word[5], 26, 1)
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index c49b341287..7884401475 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -436,6 +436,8 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
             goto bad_ste;
         }
 
+        cfg->s2cfg.affd = STE_S2AFFD(ste);
+
         /* This is still here as stage 2 has not been fully enabled yet. */
         qemu_log_mask(LOG_UNIMP, "SMMUv3 does not support stage 2 yet\n");
         goto bad_ste;
-- 
2.39.1.519.gcb327c4b5f-goog

Re: [RFC PATCH 08/16] hw/arm/smmuv3: Support S2AFFD

[Eric Auger]

Hi Mostafa,

On 2/5/23 10:44, Mostafa Saleh wrote:
> Parse S2AFFD from STE and use it in stage-2 translation.
>
> This is described in the SMMUv3 manual "5.2. Stream Table Entry" in
> "[181] S2AFFD".

from a patch structure pov, to me it would make more sense to add the
STE field decoding in
[RFC PATCH 06/16] hw/arm/smmuv3: Parse STE config for stage-2 and use it
in hw/arm/smmuv3: Add page table walk for stage-2
>
> HTTU is not supported, SW is expected to maintain the Access flag.
>
> This flag determines the behavior on access of a stage-2 page whose
> descriptor has AF == 0:
> - 0b0: An Access flag fault occurs (stall not supported).
> - 0b1: An Access flag fault never occurs.
>
> An Access fault takes priority over a Permission fault.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  hw/arm/smmu-common.c     | 10 ++++++++++
>  hw/arm/smmu-internal.h   |  2 ++
>  hw/arm/smmuv3-internal.h |  1 +
>  hw/arm/smmuv3.c          |  2 ++
>  4 files changed, 15 insertions(+)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index df0d1dc024..541c427684 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -434,6 +434,16 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
>                                       pte_addr, pte, iova, gpa,
>                                       block_size >> 20);
>          }
> +
> +        /*
> +         * If S2AFFD and PTE.AF are 0 => fault. (5.2. Stream Table Entry)
> +         * An Access fault takes priority over a Permission fault.
> +         */
> +        if (!PTE_AF(pte) && !cfg->s2cfg.affd) {
> +            info->type = SMMU_PTW_ERR_ACCESS;
Wondering how you are going to differentiate page faults at S1 versus
page faults at S2.
Event number #10 differentiates both and recorded fields are different.

Do you handle that somewhere?

Thanks

Eric

> +            goto error;
> +        }
> +
>          ap = PTE_AP(pte);
>          if (is_permission_fault_s2(ap, perm)) {
>              info->type = SMMU_PTW_ERR_PERMISSION;
> diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h
> index b02c05319f..7d3f76ce14 100644
> --- a/hw/arm/smmu-internal.h
> +++ b/hw/arm/smmu-internal.h
> @@ -66,6 +66,8 @@
>  #define PTE_APTABLE(pte) \
>      (extract64(pte, 61, 2))
>  
> +#define PTE_AF(pte) \
> +    (extract64(pte, 10, 1))
>  /*
>   * TODO: At the moment all transactions are considered as privileged (EL1)
>   * as IOMMU translation callback does not pass user/priv attributes.
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index ec64fb43a0..3ccb9d118e 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -524,6 +524,7 @@ typedef struct CD {
>  #define STE_S2TG(x)        extract32((x)->word[5], 14, 2)
>  #define STE_S2PS(x)        extract32((x)->word[5], 16, 3)
>  #define STE_S2AA64(x)      extract32((x)->word[5], 19, 1)
> +#define STE_S2AFFD(x)      extract32((x)->word[5], 21, 1)
>  #define STE_S2HD(x)        extract32((x)->word[5], 24, 1)
>  #define STE_S2HA(x)        extract32((x)->word[5], 25, 1)
>  #define STE_S2S(x)         extract32((x)->word[5], 26, 1)
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index c49b341287..7884401475 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -436,6 +436,8 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
>              goto bad_ste;
>          }
>  
> +        cfg->s2cfg.affd = STE_S2AFFD(ste);
> +
>          /* This is still here as stage 2 has not been fully enabled yet. */
>          qemu_log_mask(LOG_UNIMP, "SMMUv3 does not support stage 2 yet\n");
>          goto bad_ste;

Re: [RFC PATCH 08/16] hw/arm/smmuv3: Support S2AFFD

[Mostafa Saleh]

Hi Eric,

On Wed, Feb 15, 2023 at 07:37:52PM +0100, Eric Auger wrote:
> > Parse S2AFFD from STE and use it in stage-2 translation.
> >
> > This is described in the SMMUv3 manual "5.2. Stream Table Entry" in
> > "[181] S2AFFD".
> 
> from a patch structure pov, to me it would make more sense to add the
> STE field decoding in
> [RFC PATCH 06/16] hw/arm/smmuv3: Parse STE config for stage-2 and use it
> in hw/arm/smmuv3: Add page table walk for stage-2
Yes, as all STE parsing will be in the same patch, it make sense to
remove this one and AFFD in stage-2 PTW.

> > +         * An Access fault takes priority over a Permission fault.
> > +         */
> > +        if (!PTE_AF(pte) && !cfg->s2cfg.affd) {
> > +            info->type = SMMU_PTW_ERR_ACCESS;
> Wondering how you are going to differentiate page faults at S1 versus
> page faults at S2.
> Event number #10 differentiates both and recorded fields are different.
> 
> Do you handle that somewhere?
Yes, this is missing, similar to F_TRANSLATION, we can set
info->u.f_walk_eabt.s2 which would set S2[103] bit in the fault event.


Thanks,
Mostafa

[RFC PATCH 09/16] hw/arm/smmuv3: Don't touch CD if stage-1 is not supported.

[Mostafa Saleh]

If stage-1 is not supported, SW will not configure it, so don't try to
access it as it might have faulty addresses.

Signed-off-by: Mostafa Saleh <smostafa@google.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 7884401475..c18460a4ff 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -653,7 +653,7 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg,
         return ret;
     }
 
-    if (cfg->aborted || cfg->bypassed) {
+    if (cfg->aborted || cfg->bypassed || !STAGE1_SUPPORTED(s->features)) {
         return 0;
     }
 
-- 
2.39.1.519.gcb327c4b5f-goog

[RFC PATCH 10/16] hw/arm/smmuv3: Make TLB lookup work for stage-2

[Mostafa Saleh]

Right now, either stage-1 or stage-2 are supported, this simplifies
how we can deal with TLBs.
This patch makes TLB lookup work if stage-2 is enabled instead of
stage-1.
TLB lookup is done before a PTW, if a valid entry is found we won't
do the PTW.
To be able to do TLB lookup, we need the correct tagging info, as
granularity and input size, so we get this based on the supported
translation stage. The TLB entries are added correctly from each
stage PTW.

When nested translation is supported, this would need to change, for
example if we go with a combined TLB implementation, we would need to
use the min of the granularities in TLB.

As stage-2 shouldn't be tagged by ASID, it will be set to -1 if S1P
is not enabled.

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 hw/arm/smmuv3.c | 44 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 11 deletions(-)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index c18460a4ff..769c735697 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -653,6 +653,8 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg,
         return ret;
     }
 
+    /* ASID defaults to -1 if s1 is not supported. */
+    cfg->asid = -1;
     if (cfg->aborted || cfg->bypassed || !STAGE1_SUPPORTED(s->features)) {
         return 0;
     }
@@ -733,6 +735,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
     SMMUTLBEntry *cached_entry = NULL;
     SMMUTransTableInfo *tt;
     SMMUTransCfg *cfg = NULL;
+    uint8_t granule_sz, tsz;
     IOMMUTLBEntry entry = {
         .target_as = &address_space_memory,
         .iova = addr,
@@ -764,21 +767,40 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
         goto epilogue;
     }
 
-    tt = select_tt(cfg, addr);
-    if (!tt) {
-        if (cfg->record_faults) {
-            event.type = SMMU_EVT_F_TRANSLATION;
-            event.u.f_translation.addr = addr;
-            event.u.f_translation.rnw = flag & 0x1;
+    if (STAGE1_SUPPORTED(s->features)) {
+        /* Select stage1 translation table. */
+        tt = select_tt(cfg, addr);
+        if (!tt) {
+            if (cfg->record_faults) {
+                event.type = SMMU_EVT_F_TRANSLATION;
+                event.u.f_translation.addr = addr;
+                event.u.f_translation.rnw = flag & 0x1;
+            }
+            status = SMMU_TRANS_ERROR;
+            goto epilogue;
         }
-        status = SMMU_TRANS_ERROR;
-        goto epilogue;
-    }
+        granule_sz = tt->granule_sz;
+        tsz = tt->tsz;
 
-    page_mask = (1ULL << (tt->granule_sz)) - 1;
+    } else {
+        /* Stage2. */
+        granule_sz = cfg->s2cfg.granule_sz;
+        tsz = cfg->s2cfg.tsz;
+    }
+    /*
+     * TLB lookup looks for granule and input size for a translation stage,
+     * as only one stage is supported right now, choose the right values
+     * from the configuration.
+     */
+    page_mask = (1ULL << granule_sz) - 1;
     aligned_addr = addr & ~page_mask;
 
-    cached_entry = smmu_iotlb_lookup(bs, cfg, tt, aligned_addr);
+    SMMUTransTableInfo temp = {
+        .granule_sz = granule_sz,
+        .tsz = tsz,
+    };
+
+    cached_entry = smmu_iotlb_lookup(bs, cfg, &temp, aligned_addr);
     if (cached_entry) {
         if ((flag & IOMMU_WO) && !(cached_entry->entry.perm & IOMMU_WO)) {
             status = SMMU_TRANS_ERROR;
-- 
2.39.1.519.gcb327c4b5f-goog

Re: [RFC PATCH 10/16] hw/arm/smmuv3: Make TLB lookup work for stage-2

[Eric Auger]

Hi Mostafa,

On 2/5/23 10:44, Mostafa Saleh wrote:
> Right now, either stage-1 or stage-2 are supported, this simplifies
> how we can deal with TLBs.
> This patch makes TLB lookup work if stage-2 is enabled instead of
> stage-1.
> TLB lookup is done before a PTW, if a valid entry is found we won't
> do the PTW.
> To be able to do TLB lookup, we need the correct tagging info, as
> granularity and input size, so we get this based on the supported
> translation stage. The TLB entries are added correctly from each
> stage PTW.
>
> When nested translation is supported, this would need to change, for
> example if we go with a combined TLB implementation, we would need to
> use the min of the granularities in TLB.
>
> As stage-2 shouldn't be tagged by ASID, it will be set to -1 if S1P
> is not enabled.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  hw/arm/smmuv3.c | 44 +++++++++++++++++++++++++++++++++-----------
>  1 file changed, 33 insertions(+), 11 deletions(-)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index c18460a4ff..769c735697 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -653,6 +653,8 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg,
>          return ret;
>      }
>  
> +    /* ASID defaults to -1 if s1 is not supported. */
> +    cfg->asid = -1;
>      if (cfg->aborted || cfg->bypassed || !STAGE1_SUPPORTED(s->features)) {
>          return 0;
>      }
> @@ -733,6 +735,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>      SMMUTLBEntry *cached_entry = NULL;
>      SMMUTransTableInfo *tt;
>      SMMUTransCfg *cfg = NULL;
> +    uint8_t granule_sz, tsz;
>      IOMMUTLBEntry entry = {
>          .target_as = &address_space_memory,
>          .iova = addr,
> @@ -764,21 +767,40 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>          goto epilogue;
>      }
>  
> -    tt = select_tt(cfg, addr);
maybe we shall adapt select_tt for S2 instead of using temp? I know
there is a single range on S2 but well, use tt[0]?
> -    if (!tt) {
> -        if (cfg->record_faults) {
> -            event.type = SMMU_EVT_F_TRANSLATION;
> -            event.u.f_translation.addr = addr;
> -            event.u.f_translation.rnw = flag & 0x1;
> +    if (STAGE1_SUPPORTED(s->features)) {
maybe check the enable state instead.
> +        /* Select stage1 translation table. */
> +        tt = select_tt(cfg, addr);
> +        if (!tt) {
> +            if (cfg->record_faults) {
> +                event.type = SMMU_EVT_F_TRANSLATION;
> +                event.u.f_translation.addr = addr;
> +                event.u.f_translation.rnw = flag & 0x1;
> +            }
> +            status = SMMU_TRANS_ERROR;
> +            goto epilogue;
>          }
> -        status = SMMU_TRANS_ERROR;
> -        goto epilogue;
> -    }
> +        granule_sz = tt->granule_sz;
> +        tsz = tt->tsz;
>  
> -    page_mask = (1ULL << (tt->granule_sz)) - 1;
> +    } else {
> +        /* Stage2. */
> +        granule_sz = cfg->s2cfg.granule_sz;
> +        tsz = cfg->s2cfg.tsz;
> +    }
> +    /*
> +     * TLB lookup looks for granule and input size for a translation stage,
> +     * as only one stage is supported right now, choose the right values
> +     * from the configuration.
> +     */
> +    page_mask = (1ULL << granule_sz) - 1;
>      aligned_addr = addr & ~page_mask;
>  
> -    cached_entry = smmu_iotlb_lookup(bs, cfg, tt, aligned_addr);
> +    SMMUTransTableInfo temp = {
> +        .granule_sz = granule_sz,
> +        .tsz = tsz,
> +    };
> +
> +    cached_entry = smmu_iotlb_lookup(bs, cfg, &temp, aligned_addr);
>      if (cached_entry) {
>          if ((flag & IOMMU_WO) && !(cached_entry->entry.perm & IOMMU_WO)) {
>              status = SMMU_TRANS_ERROR;
Thanks

Eric

Re: [RFC PATCH 10/16] hw/arm/smmuv3: Make TLB lookup work for stage-2

[Mostafa Saleh]

Hi Eric,

On Thu, Feb 16, 2023 at 12:32:21PM +0100, Eric Auger wrote:
> >      SMMUTransTableInfo *tt;
> >      SMMUTransCfg *cfg = NULL;
> > +    uint8_t granule_sz, tsz;
> >      IOMMUTLBEntry entry = {
> >          .target_as = &address_space_memory,
> >          .iova = addr,
> > @@ -764,21 +767,40 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
> >          goto epilogue;
> >      }
> >  
> > -    tt = select_tt(cfg, addr);
> maybe we shall adapt select_tt for S2 instead of using temp? I know
> there is a single range on S2 but well, use tt[0]?
We can adapt select_tt for S2, but we would need to have an instance
of SMMUTransTableInfo in SMMUS2Cfg instead of having vttb,tsz..
inlined, as the function returns a pointer to the SMMUTransTableInfo.
I don’t think we can reuse tt[0], as this would change when we support
nesting, so I think we just isolate s1 and s2 from the beginning.


> > -    if (!tt) {
> > -        if (cfg->record_faults) {
> > -            event.type = SMMU_EVT_F_TRANSLATION;
> > -            event.u.f_translation.addr = addr;
> > -            event.u.f_translation.rnw = flag & 0x1;
> > +    if (STAGE1_SUPPORTED(s->features)) {
> maybe check the enable state instead.
Yes, if stage-1 is not enabled, tt might not be valid, I will update it.

Thanks,
Mostafa

Re: [RFC PATCH 10/16] hw/arm/smmuv3: Make TLB lookup work for stage-2

[Eric Auger]

On 2/16/23 14:49, Mostafa Saleh wrote:
> Hi Eric,
>
> On Thu, Feb 16, 2023 at 12:32:21PM +0100, Eric Auger wrote:
>>>      SMMUTransTableInfo *tt;
>>>      SMMUTransCfg *cfg = NULL;
>>> +    uint8_t granule_sz, tsz;
>>>      IOMMUTLBEntry entry = {
>>>          .target_as = &address_space_memory,
>>>          .iova = addr,
>>> @@ -764,21 +767,40 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>>>          goto epilogue;
>>>      }
>>>  
>>> -    tt = select_tt(cfg, addr);
>> maybe we shall adapt select_tt for S2 instead of using temp? I know
>> there is a single range on S2 but well, use tt[0]?
> We can adapt select_tt for S2, but we would need to have an instance
> of SMMUTransTableInfo in SMMUS2Cfg instead of having vttb,tsz..
> inlined, as the function returns a pointer to the SMMUTransTableInfo.
> I don’t think we can reuse tt[0], as this would change when we support
> nesting, so I think we just isolate s1 and s2 from the beginning.

OK fair enough, let's wait for others' feedbakcs then.

Eric
>
>
>>> -    if (!tt) {
>>> -        if (cfg->record_faults) {
>>> -            event.type = SMMU_EVT_F_TRANSLATION;
>>> -            event.u.f_translation.addr = addr;
>>> -            event.u.f_translation.rnw = flag & 0x1;
>>> +    if (STAGE1_SUPPORTED(s->features)) {
>> maybe check the enable state instead.
> Yes, if stage-1 is not enabled, tt might not be valid, I will update it.
>
> Thanks,
> Mostafa
>

[RFC PATCH 11/16] hw/arm/smmuv3: Read VMID from STE

[Mostafa Saleh]

According to SMMUv3 user manual "5.2 Stream Table Entry": All fields
with an S2 prefix (with the exception of S2VMID) are IGNORED when
stage-2 bypasses translation (Config[1] == 0).

Which means that VMID can be used(for TLB tagging) even if stage-2 is
bypassed, so we parse it unconditionally when S2P exists. Otherwise
it is set to -1.(only S1P)

Advertise 16-bit VMID is supported.

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 hw/arm/smmuv3.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 769c735697..35a0149bbf 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -260,6 +260,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
     s->idr[0] = FIELD_DP32(s->idr[0], IDR0, TTF, 2); /* AArch64 PTW only */
     s->idr[0] = FIELD_DP32(s->idr[0], IDR0, COHACC, 1); /* IO coherent */
     s->idr[0] = FIELD_DP32(s->idr[0], IDR0, ASID16, 1); /* 16-bit ASID */
+    s->idr[0] = FIELD_DP32(s->idr[0], IDR0, VMID16, 1); /* 16-bit VMID */
     s->idr[0] = FIELD_DP32(s->idr[0], IDR0, TTENDIAN, 2); /* little endian */
     s->idr[0] = FIELD_DP32(s->idr[0], IDR0, STALL_MODEL, 1); /* No stall */
     /* terminated transaction will always be aborted/error returned */
@@ -388,6 +389,14 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
         return 0;
     }
 
+    if (STAGE2_SUPPORTED(s->features)) {
+        /* VMID is considered even if s2 is disabled. */
+        cfg->s2cfg.vmid = STE_S2VMID(ste);
+    } else {
+        /* Default to -1 */
+        cfg->s2cfg.vmid = -1;
+    }
+
     if (STAGE2_SUPPORTED(s->features) && STE_CFG_S2_ENABLED(config)) {
         cfg->stage = 2;
 
-- 
2.39.1.519.gcb327c4b5f-goog

[RFC PATCH 12/16] hw/arm/smmuv3: Add VMID to tlb tagging

[Mostafa Saleh]

Allow TLB to be tagged with VMID.

If stage-1 is only supported, VMID is set to -1 and ignored from STE
and CMD_TLBI_NH* cmds.

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 hw/arm/smmu-common.c         | 24 +++++++++++++++---------
 hw/arm/smmu-internal.h       |  2 ++
 hw/arm/smmuv3.c              | 12 +++++++++---
 include/hw/arm/smmu-common.h |  5 +++--
 4 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 541c427684..028a60949a 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -56,10 +56,11 @@ static gboolean smmu_iotlb_key_equal(gconstpointer v1, gconstpointer v2)
            (k1->level == k2->level) && (k1->tg == k2->tg);
 }
 
-SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint64_t iova,
+SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova,
                                 uint8_t tg, uint8_t level)
 {
-    SMMUIOTLBKey key = {.asid = asid, .iova = iova, .tg = tg, .level = level};
+    SMMUIOTLBKey key = {.asid = asid, .vmid = vmid, .iova = iova,
+                        .tg = tg, .level = level};
 
     return key;
 }
@@ -78,7 +79,8 @@ SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
         uint64_t mask = subpage_size - 1;
         SMMUIOTLBKey key;
 
-        key = smmu_get_iotlb_key(cfg->asid, iova & ~mask, tg, level);
+        key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid,
+                                 iova & ~mask, tg, level);
         entry = g_hash_table_lookup(bs->iotlb, &key);
         if (entry) {
             break;
@@ -111,7 +113,8 @@ void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *new)
         smmu_iotlb_inv_all(bs);
     }
 
-    *key = smmu_get_iotlb_key(cfg->asid, new->entry.iova, tg, new->level);
+    *key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid, new->entry.iova,
+                              tg, new->level);
     trace_smmu_iotlb_insert(cfg->asid, new->entry.iova, tg, new->level);
     g_hash_table_insert(bs->iotlb, key, new);
 }
@@ -130,8 +133,7 @@ static gboolean smmu_hash_remove_by_asid(gpointer key, gpointer value,
 
     return SMMU_IOTLB_ASID(*iotlb_key) == asid;
 }
-
-static gboolean smmu_hash_remove_by_asid_iova(gpointer key, gpointer value,
+static gboolean smmu_hash_remove_by_asid_vmid_iova(gpointer key, gpointer value,
                                               gpointer user_data)
 {
     SMMUTLBEntry *iter = (SMMUTLBEntry *)value;
@@ -142,18 +144,21 @@ static gboolean smmu_hash_remove_by_asid_iova(gpointer key, gpointer value,
     if (info->asid >= 0 && info->asid != SMMU_IOTLB_ASID(iotlb_key)) {
         return false;
     }
+    if (info->vmid >= 0 && info->vmid != SMMU_IOTLB_VMID(iotlb_key)) {
+        return false;
+    }
     return ((info->iova & ~entry->addr_mask) == entry->iova) ||
            ((entry->iova & ~info->mask) == info->iova);
 }
 
-void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova,
+void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
                          uint8_t tg, uint64_t num_pages, uint8_t ttl)
 {
     /* if tg is not set we use 4KB range invalidation */
     uint8_t granule = tg ? tg * 2 + 10 : 12;
 
     if (ttl && (num_pages == 1) && (asid >= 0)) {
-        SMMUIOTLBKey key = smmu_get_iotlb_key(asid, iova, tg, ttl);
+        SMMUIOTLBKey key = smmu_get_iotlb_key(asid, vmid, iova, tg, ttl);
 
         if (g_hash_table_remove(s->iotlb, &key)) {
             return;
@@ -166,10 +171,11 @@ void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova,
 
     SMMUIOTLBPageInvInfo info = {
         .asid = asid, .iova = iova,
+        .vmid = vmid,
         .mask = (num_pages * 1 << granule) - 1};
 
     g_hash_table_foreach_remove(s->iotlb,
-                                smmu_hash_remove_by_asid_iova,
+                                smmu_hash_remove_by_asid_vmid_iova,
                                 &info);
 }
 
diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h
index 7d3f76ce14..3a14e5dca5 100644
--- a/hw/arm/smmu-internal.h
+++ b/hw/arm/smmu-internal.h
@@ -136,9 +136,11 @@ static inline int pgd_idx(int start_level, int granule, dma_addr_t iova)
 }
 
 #define SMMU_IOTLB_ASID(key) ((key).asid)
+#define SMMU_IOTLB_VMID(key) ((key).vmid)
 
 typedef struct SMMUIOTLBPageInvInfo {
     int asid;
+    int vmid;
     uint64_t iova;
     uint64_t mask;
 } SMMUIOTLBPageInvInfo;
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 35a0149bbf..8b070f6bb5 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -986,7 +986,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
 {
     dma_addr_t end, addr = CMD_ADDR(cmd);
     uint8_t type = CMD_TYPE(cmd);
-    uint16_t vmid = CMD_VMID(cmd);
+    int vmid = -1;
     uint8_t scale = CMD_SCALE(cmd);
     uint8_t num = CMD_NUM(cmd);
     uint8_t ttl = CMD_TTL(cmd);
@@ -995,6 +995,12 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
     uint64_t num_pages;
     uint8_t granule;
     int asid = -1;
+    SMMUv3State *smmuv3 = ARM_SMMUV3(s);
+
+    /* Only consider VMID if stage-2 is supported. */
+    if (STAGE2_SUPPORTED(smmuv3->features)) {
+        vmid = CMD_VMID(cmd);
+    }
 
     if (type == SMMU_CMD_TLBI_NH_VA) {
         asid = CMD_ASID(cmd);
@@ -1003,7 +1009,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
     if (!tg) {
         trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, 1, ttl, leaf);
         smmuv3_inv_notifiers_iova(s, asid, addr, tg, 1);
-        smmu_iotlb_inv_iova(s, asid, addr, tg, 1, ttl);
+        smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl);
         return;
     }
 
@@ -1021,7 +1027,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
         num_pages = (mask + 1) >> granule;
         trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, num_pages, ttl, leaf);
         smmuv3_inv_notifiers_iova(s, asid, addr, tg, num_pages);
-        smmu_iotlb_inv_iova(s, asid, addr, tg, num_pages, ttl);
+        smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl);
         addr += mask + 1;
     }
 }
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 7906e359d9..5cca1c17f5 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -113,6 +113,7 @@ typedef struct SMMUPciBus {
 typedef struct SMMUIOTLBKey {
     uint64_t iova;
     uint16_t asid;
+    uint16_t vmid;
     uint8_t tg;
     uint8_t level;
 } SMMUIOTLBKey;
@@ -176,11 +177,11 @@ IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid);
 SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
                                 SMMUTransTableInfo *tt, hwaddr iova);
 void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *entry);
-SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint64_t iova,
+SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova,
                                 uint8_t tg, uint8_t level);
 void smmu_iotlb_inv_all(SMMUState *s);
 void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid);
-void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova,
+void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
                          uint8_t tg, uint64_t num_pages, uint8_t ttl);
 
 /* Unmap the range of all the notifiers registered to any IOMMU mr */
-- 
2.39.1.519.gcb327c4b5f-goog

Re: [RFC PATCH 12/16] hw/arm/smmuv3: Add VMID to tlb tagging

[Jean-Philippe Brucker]

Hi Mostafa,

On Sun, Feb 05, 2023 at 09:44:07AM +0000, Mostafa Saleh wrote:
> Allow TLB to be tagged with VMID.
> 
> If stage-1 is only supported, VMID is set to -1 and ignored from STE
> and CMD_TLBI_NH* cmds.
> 
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  hw/arm/smmu-common.c         | 24 +++++++++++++++---------
>  hw/arm/smmu-internal.h       |  2 ++
>  hw/arm/smmuv3.c              | 12 +++++++++---
>  include/hw/arm/smmu-common.h |  5 +++--
>  4 files changed, 29 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 541c427684..028a60949a 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -56,10 +56,11 @@ static gboolean smmu_iotlb_key_equal(gconstpointer v1, gconstpointer v2)
>             (k1->level == k2->level) && (k1->tg == k2->tg);

I'm getting some aliasing in the TLB, because smmu_iotlb_key_equal() is
missing the VMID comparison. With that fixed my handful of tests pass

Thanks,
Jean

Re: [RFC PATCH 12/16] hw/arm/smmuv3: Add VMID to tlb tagging

[Mostafa Saleh]

Hi Jean,

Thanks for taking the time to check the patches.

On Wed, Feb 15, 2023 at 07:47:18PM +0000, Jean-Philippe Brucker wrote:
> > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> > index 541c427684..028a60949a 100644
> > --- a/hw/arm/smmu-common.c
> > +++ b/hw/arm/smmu-common.c
> > @@ -56,10 +56,11 @@ static gboolean smmu_iotlb_key_equal(gconstpointer v1, gconstpointer v2)
> >             (k1->level == k2->level) && (k1->tg == k2->tg);
> 
> I'm getting some aliasing in the TLB, because smmu_iotlb_key_equal() is
> missing the VMID comparison. With that fixed my handful of tests pass
> 
Oh, I missed that, I will update it in v2.

Thanks,
Mostafa

Re: [RFC PATCH 12/16] hw/arm/smmuv3: Add VMID to tlb tagging

[Eric Auger]

Hi,

On 2/5/23 10:44, Mostafa Saleh wrote:
> Allow TLB to be tagged with VMID.
>
> If stage-1 is only supported, VMID is set to -1 and ignored from STE
> and CMD_TLBI_NH* cmds.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  hw/arm/smmu-common.c         | 24 +++++++++++++++---------
>  hw/arm/smmu-internal.h       |  2 ++
>  hw/arm/smmuv3.c              | 12 +++++++++---
>  include/hw/arm/smmu-common.h |  5 +++--
>  4 files changed, 29 insertions(+), 14 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 541c427684..028a60949a 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -56,10 +56,11 @@ static gboolean smmu_iotlb_key_equal(gconstpointer v1, gconstpointer v2)
>             (k1->level == k2->level) && (k1->tg == k2->tg);
>  }
>  
> -SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint64_t iova,
> +SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova,
>                                  uint8_t tg, uint8_t level)
>  {
> -    SMMUIOTLBKey key = {.asid = asid, .iova = iova, .tg = tg, .level = level};
> +    SMMUIOTLBKey key = {.asid = asid, .vmid = vmid, .iova = iova,
> +                        .tg = tg, .level = level};
>  
>      return key;
>  }
> @@ -78,7 +79,8 @@ SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
>          uint64_t mask = subpage_size - 1;
>          SMMUIOTLBKey key;
>  
> -        key = smmu_get_iotlb_key(cfg->asid, iova & ~mask, tg, level);
> +        key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid,
> +                                 iova & ~mask, tg, level);
>          entry = g_hash_table_lookup(bs->iotlb, &key);
>          if (entry) {
>              break;
> @@ -111,7 +113,8 @@ void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *new)
>          smmu_iotlb_inv_all(bs);
>      }
>  
> -    *key = smmu_get_iotlb_key(cfg->asid, new->entry.iova, tg, new->level);
> +    *key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid, new->entry.iova,
> +                              tg, new->level);
>      trace_smmu_iotlb_insert(cfg->asid, new->entry.iova, tg, new->level);
you may update the trace point as well

Thanks

Erric
>      g_hash_table_insert(bs->iotlb, key, new);
>  }
> @@ -130,8 +133,7 @@ static gboolean smmu_hash_remove_by_asid(gpointer key, gpointer value,
>  
>      return SMMU_IOTLB_ASID(*iotlb_key) == asid;
>  }
> -
> -static gboolean smmu_hash_remove_by_asid_iova(gpointer key, gpointer value,
> +static gboolean smmu_hash_remove_by_asid_vmid_iova(gpointer key, gpointer value,
>                                                gpointer user_data)
>  {
>      SMMUTLBEntry *iter = (SMMUTLBEntry *)value;
> @@ -142,18 +144,21 @@ static gboolean smmu_hash_remove_by_asid_iova(gpointer key, gpointer value,
>      if (info->asid >= 0 && info->asid != SMMU_IOTLB_ASID(iotlb_key)) {
>          return false;
>      }
> +    if (info->vmid >= 0 && info->vmid != SMMU_IOTLB_VMID(iotlb_key)) {
> +        return false;
> +    }
>      return ((info->iova & ~entry->addr_mask) == entry->iova) ||
>             ((entry->iova & ~info->mask) == info->iova);
>  }
>  
> -void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova,
> +void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
>                           uint8_t tg, uint64_t num_pages, uint8_t ttl)
>  {
>      /* if tg is not set we use 4KB range invalidation */
>      uint8_t granule = tg ? tg * 2 + 10 : 12;
>  
>      if (ttl && (num_pages == 1) && (asid >= 0)) {
> -        SMMUIOTLBKey key = smmu_get_iotlb_key(asid, iova, tg, ttl);
> +        SMMUIOTLBKey key = smmu_get_iotlb_key(asid, vmid, iova, tg, ttl);
>  
>          if (g_hash_table_remove(s->iotlb, &key)) {
>              return;
> @@ -166,10 +171,11 @@ void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova,
>  
>      SMMUIOTLBPageInvInfo info = {
>          .asid = asid, .iova = iova,
> +        .vmid = vmid,
>          .mask = (num_pages * 1 << granule) - 1};
>  
>      g_hash_table_foreach_remove(s->iotlb,
> -                                smmu_hash_remove_by_asid_iova,
> +                                smmu_hash_remove_by_asid_vmid_iova,
>                                  &info);
>  }
>  
> diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h
> index 7d3f76ce14..3a14e5dca5 100644
> --- a/hw/arm/smmu-internal.h
> +++ b/hw/arm/smmu-internal.h
> @@ -136,9 +136,11 @@ static inline int pgd_idx(int start_level, int granule, dma_addr_t iova)
>  }
>  
>  #define SMMU_IOTLB_ASID(key) ((key).asid)
> +#define SMMU_IOTLB_VMID(key) ((key).vmid)
>  
>  typedef struct SMMUIOTLBPageInvInfo {
>      int asid;
> +    int vmid;
>      uint64_t iova;
>      uint64_t mask;
>  } SMMUIOTLBPageInvInfo;
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 35a0149bbf..8b070f6bb5 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -986,7 +986,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
>  {
>      dma_addr_t end, addr = CMD_ADDR(cmd);
>      uint8_t type = CMD_TYPE(cmd);
> -    uint16_t vmid = CMD_VMID(cmd);
> +    int vmid = -1;
>      uint8_t scale = CMD_SCALE(cmd);
>      uint8_t num = CMD_NUM(cmd);
>      uint8_t ttl = CMD_TTL(cmd);
> @@ -995,6 +995,12 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
>      uint64_t num_pages;
>      uint8_t granule;
>      int asid = -1;
> +    SMMUv3State *smmuv3 = ARM_SMMUV3(s);
> +
> +    /* Only consider VMID if stage-2 is supported. */
> +    if (STAGE2_SUPPORTED(smmuv3->features)) {
> +        vmid = CMD_VMID(cmd);
> +    }
>  
>      if (type == SMMU_CMD_TLBI_NH_VA) {
>          asid = CMD_ASID(cmd);
> @@ -1003,7 +1009,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
>      if (!tg) {
>          trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, 1, ttl, leaf);
>          smmuv3_inv_notifiers_iova(s, asid, addr, tg, 1);
> -        smmu_iotlb_inv_iova(s, asid, addr, tg, 1, ttl);
> +        smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl);
>          return;
>      }
>  
> @@ -1021,7 +1027,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
>          num_pages = (mask + 1) >> granule;
>          trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, num_pages, ttl, leaf);
>          smmuv3_inv_notifiers_iova(s, asid, addr, tg, num_pages);
> -        smmu_iotlb_inv_iova(s, asid, addr, tg, num_pages, ttl);
> +        smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl);
>          addr += mask + 1;
>      }
>  }
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 7906e359d9..5cca1c17f5 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -113,6 +113,7 @@ typedef struct SMMUPciBus {
>  typedef struct SMMUIOTLBKey {
>      uint64_t iova;
>      uint16_t asid;
> +    uint16_t vmid;
>      uint8_t tg;
>      uint8_t level;
>  } SMMUIOTLBKey;
> @@ -176,11 +177,11 @@ IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid);
>  SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
>                                  SMMUTransTableInfo *tt, hwaddr iova);
>  void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *entry);
> -SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint64_t iova,
> +SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova,
>                                  uint8_t tg, uint8_t level);
>  void smmu_iotlb_inv_all(SMMUState *s);
>  void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid);
> -void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova,
> +void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
>                           uint8_t tg, uint64_t num_pages, uint8_t ttl);
>  
>  /* Unmap the range of all the notifiers registered to any IOMMU mr */

Re: [RFC PATCH 12/16] hw/arm/smmuv3: Add VMID to tlb tagging

[Mostafa Saleh]

Hi Eric,

On Thu, Feb 16, 2023 at 11:17:34AM +0100, Eric Auger wrote:
> >      }
> >  
> > -    *key = smmu_get_iotlb_key(cfg->asid, new->entry.iova, tg, new->level);
> > +    *key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid, new->entry.iova,
> > +                              tg, new->level);
> >      trace_smmu_iotlb_insert(cfg->asid, new->entry.iova, tg, new->level);
> you may update the trace point as well
I can add a stage argument here, however I don’t think it is necessary
now as TLBs are either used for stage-1 or stage-2, not both.

Thanks,
Mostafa

Re: [RFC PATCH 12/16] hw/arm/smmuv3: Add VMID to tlb tagging

[Eric Auger]

On 2/16/23 14:53, Mostafa Saleh wrote:
> Hi Eric,
>
> On Thu, Feb 16, 2023 at 11:17:34AM +0100, Eric Auger wrote:
>>>      }
>>>  
>>> -    *key = smmu_get_iotlb_key(cfg->asid, new->entry.iova, tg, new->level);
>>> +    *key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid, new->entry.iova,
>>> +                              tg, new->level);
>>>      trace_smmu_iotlb_insert(cfg->asid, new->entry.iova, tg, new->level);
>> you may update the trace point as well
> I can add a stage argument here, however I don’t think it is necessary
> now as TLBs are either used for stage-1 or stage-2, not both.
no I meant adding the vmid value there

Eric
>
> Thanks,
> Mostafa
>

[RFC PATCH 13/16] hw/arm/smmuv3: Add CMDs related to stage 2

[Mostafa Saleh]

CMD_TLBI_S2_IPA: As S1+S2 is not enabled, for now this can be the
same as CMD_TLBI_NH_VAA.

CMD_TLBI_S12_VMALL: Added new function to invalidate TLB by VMID.

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 hw/arm/smmu-common.c         | 16 ++++++++++++++++
 hw/arm/smmuv3.c              | 25 +++++++++++++++++++++++--
 hw/arm/trace-events          |  2 ++
 include/hw/arm/smmu-common.h |  1 +
 4 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 028a60949a..28089d94a6 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -133,6 +133,16 @@ static gboolean smmu_hash_remove_by_asid(gpointer key, gpointer value,
 
     return SMMU_IOTLB_ASID(*iotlb_key) == asid;
 }
+
+static gboolean smmu_hash_remove_by_vmid(gpointer key, gpointer value,
+                                         gpointer user_data)
+{
+    uint16_t vmid = *(uint16_t *)user_data;
+    SMMUIOTLBKey *iotlb_key = (SMMUIOTLBKey *)key;
+
+    return SMMU_IOTLB_VMID(*iotlb_key) == vmid;
+}
+
 static gboolean smmu_hash_remove_by_asid_vmid_iova(gpointer key, gpointer value,
                                               gpointer user_data)
 {
@@ -185,6 +195,12 @@ void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid)
     g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_asid, &asid);
 }
 
+inline void smmu_iotlb_inv_vmid(SMMUState *s, uint16_t vmid)
+{
+    trace_smmu_iotlb_inv_vmid(vmid);
+    g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_vmid, &vmid);
+}
+
 /* VMSAv8-64 Translation */
 
 /**
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 8b070f6bb5..2b563a5b1b 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -1174,14 +1174,35 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
         case SMMU_CMD_TLBI_NH_VA:
             smmuv3_s1_range_inval(bs, &cmd);
             break;
+        case SMMU_CMD_TLBI_S12_VMALL:
+            uint16_t vmid = CMD_VMID(&cmd);
+
+            if (!STAGE2_SUPPORTED(s->features)) {
+                cmd_error = SMMU_CERROR_ILL;
+                break;
+            }
+
+            trace_smmuv3_cmdq_tlbi_s12_vmid(vmid);
+            smmu_inv_notifiers_all(&s->smmu_state);
+            smmu_iotlb_inv_vmid(bs, vmid);
+            break;
+        case SMMU_CMD_TLBI_S2_IPA:
+            if (!STAGE2_SUPPORTED(s->features)) {
+                cmd_error = SMMU_CERROR_ILL;
+                break;
+            }
+            /*
+             * As currently only either s1 or s2 are supported
+             * we can reuse same function for s2.
+             */
+            smmuv3_s1_range_inval(bs, &cmd);
+            break;
         case SMMU_CMD_TLBI_EL3_ALL:
         case SMMU_CMD_TLBI_EL3_VA:
         case SMMU_CMD_TLBI_EL2_ALL:
         case SMMU_CMD_TLBI_EL2_ASID:
         case SMMU_CMD_TLBI_EL2_VA:
         case SMMU_CMD_TLBI_EL2_VAA:
-        case SMMU_CMD_TLBI_S12_VMALL:
-        case SMMU_CMD_TLBI_S2_IPA:
         case SMMU_CMD_ATC_INV:
         case SMMU_CMD_PRI_RESP:
         case SMMU_CMD_RESUME:
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index 2dee296c8f..61e2ffade5 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -12,6 +12,7 @@ smmu_ptw_block_pte(int stage, int level, uint64_t baseaddr, uint64_t pteaddr, ui
 smmu_get_pte(uint64_t baseaddr, int index, uint64_t pteaddr, uint64_t pte) "baseaddr=0x%"PRIx64" index=0x%x, pteaddr=0x%"PRIx64", pte=0x%"PRIx64
 smmu_iotlb_inv_all(void) "IOTLB invalidate all"
 smmu_iotlb_inv_asid(uint16_t asid) "IOTLB invalidate asid=%d"
+smmu_iotlb_inv_vmid(uint16_t vmid) "IOTLB invalidate vmid=%d"
 smmu_iotlb_inv_iova(uint16_t asid, uint64_t addr) "IOTLB invalidate asid=%d addr=0x%"PRIx64
 smmu_inv_notifiers_mr(const char *name) "iommu mr=%s"
 smmu_iotlb_lookup_hit(uint16_t asid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache HIT asid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
@@ -48,6 +49,7 @@ smmuv3_config_cache_miss(uint32_t sid, uint32_t hits, uint32_t misses, uint32_t
 smmuv3_s1_range_inval(int vmid, int asid, uint64_t addr, uint8_t tg, uint64_t num_pages, uint8_t ttl, bool leaf) "vmid=%d asid=%d addr=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64" ttl=%d leaf=%d"
 smmuv3_cmdq_tlbi_nh(void) ""
 smmuv3_cmdq_tlbi_nh_asid(uint16_t asid) "asid=%d"
+smmuv3_cmdq_tlbi_s12_vmid(uint16_t vmid) "vmid=%d"
 smmuv3_config_cache_inv(uint32_t sid) "Config cache INV for sid=0x%x"
 smmuv3_notify_flag_add(const char *iommu) "ADD SMMUNotifier node for iommu mr=%s"
 smmuv3_notify_flag_del(const char *iommu) "DEL SMMUNotifier node for iommu mr=%s"
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 5cca1c17f5..46ba1f6329 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -181,6 +181,7 @@ SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova,
                                 uint8_t tg, uint8_t level);
 void smmu_iotlb_inv_all(SMMUState *s);
 void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid);
+void smmu_iotlb_inv_vmid(SMMUState *s, uint16_t vmid);
 void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
                          uint8_t tg, uint64_t num_pages, uint8_t ttl);
 
-- 
2.39.1.519.gcb327c4b5f-goog

Re: [RFC PATCH 13/16] hw/arm/smmuv3: Add CMDs related to stage 2

[Eric Auger]

Hi Mostafa,

On 2/5/23 10:44, Mostafa Saleh wrote:
> CMD_TLBI_S2_IPA: As S1+S2 is not enabled, for now this can be the
> same as CMD_TLBI_NH_VAA.
>
> CMD_TLBI_S12_VMALL: Added new function to invalidate TLB by VMID.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  hw/arm/smmu-common.c         | 16 ++++++++++++++++
>  hw/arm/smmuv3.c              | 25 +++++++++++++++++++++++--
>  hw/arm/trace-events          |  2 ++
>  include/hw/arm/smmu-common.h |  1 +
>  4 files changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 028a60949a..28089d94a6 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -133,6 +133,16 @@ static gboolean smmu_hash_remove_by_asid(gpointer key, gpointer value,
>  
>      return SMMU_IOTLB_ASID(*iotlb_key) == asid;
>  }
> +
> +static gboolean smmu_hash_remove_by_vmid(gpointer key, gpointer value,
> +                                         gpointer user_data)
> +{
> +    uint16_t vmid = *(uint16_t *)user_data;
> +    SMMUIOTLBKey *iotlb_key = (SMMUIOTLBKey *)key;
> +
> +    return SMMU_IOTLB_VMID(*iotlb_key) == vmid;
> +}
> +
>  static gboolean smmu_hash_remove_by_asid_vmid_iova(gpointer key, gpointer value,
>                                                gpointer user_data)
>  {
> @@ -185,6 +195,12 @@ void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid)
>      g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_asid, &asid);
>  }
>  
> +inline void smmu_iotlb_inv_vmid(SMMUState *s, uint16_t vmid)
> +{
> +    trace_smmu_iotlb_inv_vmid(vmid);
> +    g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_vmid, &vmid);
> +}
> +
>  /* VMSAv8-64 Translation */
>  
>  /**
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 8b070f6bb5..2b563a5b1b 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1174,14 +1174,35 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
>          case SMMU_CMD_TLBI_NH_VA:
>              smmuv3_s1_range_inval(bs, &cmd);
>              break;
> +        case SMMU_CMD_TLBI_S12_VMALL:
> +            uint16_t vmid = CMD_VMID(&cmd);
> +
> +            if (!STAGE2_SUPPORTED(s->features)) {
if you add such checks for S2, may you should consider adding similar
ones for existing S1?
> +                cmd_error = SMMU_CERROR_ILL;
> +                break;
> +            }
> +
> +            trace_smmuv3_cmdq_tlbi_s12_vmid(vmid);
> +            smmu_inv_notifiers_all(&s->smmu_state);
> +            smmu_iotlb_inv_vmid(bs, vmid);
> +            break;
> +        case SMMU_CMD_TLBI_S2_IPA:
> +            if (!STAGE2_SUPPORTED(s->features)) {
> +                cmd_error = SMMU_CERROR_ILL;
> +                break;
> +            }
> +            /*
> +             * As currently only either s1 or s2 are supported
> +             * we can reuse same function for s2.
> +             */
> +            smmuv3_s1_range_inval(bs, &cmd);
Shouldn't we rename the function then?

Eric
> +            break;
>          case SMMU_CMD_TLBI_EL3_ALL:
>          case SMMU_CMD_TLBI_EL3_VA:
>          case SMMU_CMD_TLBI_EL2_ALL:
>          case SMMU_CMD_TLBI_EL2_ASID:
>          case SMMU_CMD_TLBI_EL2_VA:
>          case SMMU_CMD_TLBI_EL2_VAA:
> -        case SMMU_CMD_TLBI_S12_VMALL:
> -        case SMMU_CMD_TLBI_S2_IPA:
>          case SMMU_CMD_ATC_INV:
>          case SMMU_CMD_PRI_RESP:
>          case SMMU_CMD_RESUME:
> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index 2dee296c8f..61e2ffade5 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -12,6 +12,7 @@ smmu_ptw_block_pte(int stage, int level, uint64_t baseaddr, uint64_t pteaddr, ui
>  smmu_get_pte(uint64_t baseaddr, int index, uint64_t pteaddr, uint64_t pte) "baseaddr=0x%"PRIx64" index=0x%x, pteaddr=0x%"PRIx64", pte=0x%"PRIx64
>  smmu_iotlb_inv_all(void) "IOTLB invalidate all"
>  smmu_iotlb_inv_asid(uint16_t asid) "IOTLB invalidate asid=%d"
> +smmu_iotlb_inv_vmid(uint16_t vmid) "IOTLB invalidate vmid=%d"
>  smmu_iotlb_inv_iova(uint16_t asid, uint64_t addr) "IOTLB invalidate asid=%d addr=0x%"PRIx64
>  smmu_inv_notifiers_mr(const char *name) "iommu mr=%s"
>  smmu_iotlb_lookup_hit(uint16_t asid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache HIT asid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
> @@ -48,6 +49,7 @@ smmuv3_config_cache_miss(uint32_t sid, uint32_t hits, uint32_t misses, uint32_t
>  smmuv3_s1_range_inval(int vmid, int asid, uint64_t addr, uint8_t tg, uint64_t num_pages, uint8_t ttl, bool leaf) "vmid=%d asid=%d addr=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64" ttl=%d leaf=%d"
>  smmuv3_cmdq_tlbi_nh(void) ""
>  smmuv3_cmdq_tlbi_nh_asid(uint16_t asid) "asid=%d"
> +smmuv3_cmdq_tlbi_s12_vmid(uint16_t vmid) "vmid=%d"
>  smmuv3_config_cache_inv(uint32_t sid) "Config cache INV for sid=0x%x"
>  smmuv3_notify_flag_add(const char *iommu) "ADD SMMUNotifier node for iommu mr=%s"
>  smmuv3_notify_flag_del(const char *iommu) "DEL SMMUNotifier node for iommu mr=%s"
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 5cca1c17f5..46ba1f6329 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -181,6 +181,7 @@ SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova,
>                                  uint8_t tg, uint8_t level);
>  void smmu_iotlb_inv_all(SMMUState *s);
>  void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid);
> +void smmu_iotlb_inv_vmid(SMMUState *s, uint16_t vmid);
>  void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
>                           uint8_t tg, uint64_t num_pages, uint8_t ttl);
>  

Re: [RFC PATCH 13/16] hw/arm/smmuv3: Add CMDs related to stage 2

[Mostafa Saleh]

Hi Eric,

On Thu, Feb 16, 2023 at 12:56:52PM +0100, Eric Auger wrote:
> > @@ -1174,14 +1174,35 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> >          case SMMU_CMD_TLBI_NH_VA:
> >              smmuv3_s1_range_inval(bs, &cmd);
> >              break;
> > +        case SMMU_CMD_TLBI_S12_VMALL:
> > +            uint16_t vmid = CMD_VMID(&cmd);
> > +
> > +            if (!STAGE2_SUPPORTED(s->features)) {
> if you add such checks for S2, may you should consider adding similar
> ones for existing S1?
Yes, I will go through the other commands and do the same for stage-1
only commands.

> > +            smmu_inv_notifiers_all(&s->smmu_state);
> > +            smmu_iotlb_inv_vmid(bs, vmid);
> > +            break;
> > +        case SMMU_CMD_TLBI_S2_IPA:
> > +            if (!STAGE2_SUPPORTED(s->features)) {
> > +                cmd_error = SMMU_CERROR_ILL;
> > +                break;
> > +            }
> > +            /*
> > +             * As currently only either s1 or s2 are supported
> > +             * we can reuse same function for s2.
> > +             */
> > +            smmuv3_s1_range_inval(bs, &cmd);
> Shouldn't we rename the function then?
I guess we can rename it smmuv3_s1_s2_range_inval, we will have to
revisit this when nesting is supported.

Thanks,
Mostafa

Re: [RFC PATCH 13/16] hw/arm/smmuv3: Add CMDs related to stage 2

[Eric Auger]

On 2/16/23 14:58, Mostafa Saleh wrote:
> Hi Eric,
>
> On Thu, Feb 16, 2023 at 12:56:52PM +0100, Eric Auger wrote:
>>> @@ -1174,14 +1174,35 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
>>>          case SMMU_CMD_TLBI_NH_VA:
>>>              smmuv3_s1_range_inval(bs, &cmd);
>>>              break;
>>> +        case SMMU_CMD_TLBI_S12_VMALL:
>>> +            uint16_t vmid = CMD_VMID(&cmd);
>>> +
>>> +            if (!STAGE2_SUPPORTED(s->features)) {
>> if you add such checks for S2, may you should consider adding similar
>> ones for existing S1?
> Yes, I will go through the other commands and do the same for stage-1
> only commands.
>
>>> +            smmu_inv_notifiers_all(&s->smmu_state);
>>> +            smmu_iotlb_inv_vmid(bs, vmid);
>>> +            break;
>>> +        case SMMU_CMD_TLBI_S2_IPA:
>>> +            if (!STAGE2_SUPPORTED(s->features)) {
>>> +                cmd_error = SMMU_CERROR_ILL;
>>> +                break;
>>> +            }
>>> +            /*
>>> +             * As currently only either s1 or s2 are supported
>>> +             * we can reuse same function for s2.
>>> +             */
>>> +            smmuv3_s1_range_inval(bs, &cmd);
>> Shouldn't we rename the function then?
> I guess we can rename it smmuv3_s1_s2_range_inval, we will have to
> revisit this when nesting is supported.
or simply smmuv3_range_inval, adding a comment specifying its is usable
for both stages

Eric
>
> Thanks,
> Mostafa
>

[RFC PATCH 14/16] hw/arm/smmuv3: Add stage-2 support in iova notifier

[Mostafa Saleh]

In smmuv3_notify_iova, read the granule based on translation stage
and use VMID if valid value is sent.

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 hw/arm/smmuv3.c     | 39 ++++++++++++++++++++++++++-------------
 hw/arm/trace-events |  2 +-
 2 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 2b563a5b1b..e0976ac236 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -919,18 +919,21 @@ epilogue:
  * @mr: IOMMU mr region handle
  * @n: notifier to be called
  * @asid: address space ID or negative value if we don't care
+ * @vmid: virtual machine ID or negative value if we don't care
  * @iova: iova
  * @tg: translation granule (if communicated through range invalidation)
  * @num_pages: number of @granule sized pages (if tg != 0), otherwise 1
  */
 static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
                                IOMMUNotifier *n,
-                               int asid, dma_addr_t iova,
-                               uint8_t tg, uint64_t num_pages)
+                               int asid, int vmid,
+                               dma_addr_t iova, uint8_t tg,
+                               uint64_t num_pages)
 {
     SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu);
     IOMMUTLBEvent event;
     uint8_t granule;
+    SMMUv3State *s = sdev->smmu;
 
     if (!tg) {
         SMMUEventInfo event = {.inval_ste_allowed = true};
@@ -945,11 +948,20 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
             return;
         }
 
-        tt = select_tt(cfg, iova);
-        if (!tt) {
+        if (vmid >= 0 && cfg->s2cfg.vmid != vmid) {
             return;
         }
-        granule = tt->granule_sz;
+
+        if (STAGE1_SUPPORTED(s->features)) {
+            tt = select_tt(cfg, iova);
+            if (!tt) {
+                return;
+            }
+            granule = tt->granule_sz;
+        } else {
+            granule = cfg->s2cfg.granule_sz;
+        }
+
     } else {
         granule = tg * 2 + 10;
     }
@@ -963,9 +975,10 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
     memory_region_notify_iommu_one(n, &event);
 }
 
-/* invalidate an asid/iova range tuple in all mr's */
-static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, dma_addr_t iova,
-                                      uint8_t tg, uint64_t num_pages)
+/* invalidate an asid/vmid/iova range tuple in all mr's */
+static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, int vmid,
+                                      dma_addr_t iova, uint8_t tg,
+                                      uint64_t num_pages)
 {
     SMMUDevice *sdev;
 
@@ -973,11 +986,11 @@ static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, dma_addr_t iova,
         IOMMUMemoryRegion *mr = &sdev->iommu;
         IOMMUNotifier *n;
 
-        trace_smmuv3_inv_notifiers_iova(mr->parent_obj.name, asid, iova,
-                                        tg, num_pages);
+        trace_smmuv3_inv_notifiers_iova(mr->parent_obj.name, asid, vmid,
+                                        iova, tg, num_pages);
 
         IOMMU_NOTIFIER_FOREACH(n, mr) {
-            smmuv3_notify_iova(mr, n, asid, iova, tg, num_pages);
+            smmuv3_notify_iova(mr, n, asid, vmid, iova, tg, num_pages);
         }
     }
 }
@@ -1008,7 +1021,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
 
     if (!tg) {
         trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, 1, ttl, leaf);
-        smmuv3_inv_notifiers_iova(s, asid, addr, tg, 1);
+        smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, 1);
         smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl);
         return;
     }
@@ -1026,7 +1039,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
 
         num_pages = (mask + 1) >> granule;
         trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, num_pages, ttl, leaf);
-        smmuv3_inv_notifiers_iova(s, asid, addr, tg, num_pages);
+        smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, num_pages);
         smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl);
         addr += mask + 1;
     }
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index 61e2ffade5..625cf16f69 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -53,5 +53,5 @@ smmuv3_cmdq_tlbi_s12_vmid(uint16_t vmid) "vmid=%d"
 smmuv3_config_cache_inv(uint32_t sid) "Config cache INV for sid=0x%x"
 smmuv3_notify_flag_add(const char *iommu) "ADD SMMUNotifier node for iommu mr=%s"
 smmuv3_notify_flag_del(const char *iommu) "DEL SMMUNotifier node for iommu mr=%s"
-smmuv3_inv_notifiers_iova(const char *name, uint16_t asid, uint64_t iova, uint8_t tg, uint64_t num_pages) "iommu mr=%s asid=%d iova=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64
+smmuv3_inv_notifiers_iova(const char *name, uint16_t asid, uint16_t vmid, uint64_t iova, uint8_t tg, uint64_t num_pages) "iommu mr=%s asid=%d vmid=%d iova=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64
 
-- 
2.39.1.519.gcb327c4b5f-goog

[RFC PATCH 15/16] hw/arm/smmuv3: Add fault configuration for stage-2

[Mostafa Saleh]

As stall is not supported, if S2S is set the translation would abort.
For S2R, we reuse the same code used for stage-1 with flag
record_faults. However when nested translation is supported we would
need to separate stage-1 and stage-2 faults.

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 hw/arm/smmuv3-internal.h | 2 ++
 hw/arm/smmuv3.c          | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index 3ccb9d118e..ccdae81db8 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -528,6 +528,8 @@ typedef struct CD {
 #define STE_S2HD(x)        extract32((x)->word[5], 24, 1)
 #define STE_S2HA(x)        extract32((x)->word[5], 25, 1)
 #define STE_S2S(x)         extract32((x)->word[5], 26, 1)
+#define STE_S2R(x)         extract32((x)->word[5], 27, 1)
+
 #define STE_CTXPTR(x)                                           \
     ({                                                          \
         unsigned long addr;                                     \
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index e0976ac236..5f792d96ab 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -446,6 +446,11 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
         }
 
         cfg->s2cfg.affd = STE_S2AFFD(ste);
+        cfg->record_faults = STE_S2R(ste);
+        if (STE_S2S(ste)) {
+            qemu_log_mask(LOG_UNIMP, "SMMUv3 Stall not implemented!\n");
+            goto bad_ste;
+        }
 
         /* This is still here as stage 2 has not been fully enabled yet. */
         qemu_log_mask(LOG_UNIMP, "SMMUv3 does not support stage 2 yet\n");
-- 
2.39.1.519.gcb327c4b5f-goog

Re: [RFC PATCH 15/16] hw/arm/smmuv3: Add fault configuration for stage-2

[Eric Auger]

On 2/5/23 10:44, Mostafa Saleh wrote:
> As stall is not supported, if S2S is set the translation would abort.
> For S2R, we reuse the same code used for stage-1 with flag
> record_faults. However when nested translation is supported we would
> need to separate stage-1 and stage-2 faults.
same here, please squash that code in the STE decoding and possible add
those above comments in the commit msg

Thanks

Eric
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  hw/arm/smmuv3-internal.h | 2 ++
>  hw/arm/smmuv3.c          | 5 +++++
>  2 files changed, 7 insertions(+)
>
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index 3ccb9d118e..ccdae81db8 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -528,6 +528,8 @@ typedef struct CD {
>  #define STE_S2HD(x)        extract32((x)->word[5], 24, 1)
>  #define STE_S2HA(x)        extract32((x)->word[5], 25, 1)
>  #define STE_S2S(x)         extract32((x)->word[5], 26, 1)
> +#define STE_S2R(x)         extract32((x)->word[5], 27, 1)
> +
>  #define STE_CTXPTR(x)                                           \
>      ({                                                          \
>          unsigned long addr;                                     \
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index e0976ac236..5f792d96ab 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -446,6 +446,11 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
>          }
>  
>          cfg->s2cfg.affd = STE_S2AFFD(ste);
> +        cfg->record_faults = STE_S2R(ste);
> +        if (STE_S2S(ste)) {
> +            qemu_log_mask(LOG_UNIMP, "SMMUv3 Stall not implemented!\n");
> +            goto bad_ste;
> +        }
>  
>          /* This is still here as stage 2 has not been fully enabled yet. */
>          qemu_log_mask(LOG_UNIMP, "SMMUv3 does not support stage 2 yet\n");

Re: [RFC PATCH 15/16] hw/arm/smmuv3: Add fault configuration for stage-2

[Mostafa Saleh]

On Wed, Feb 15, 2023 at 07:55:27PM +0100, Eric Auger wrote:
> > record_faults. However when nested translation is supported we would
> > need to separate stage-1 and stage-2 faults.
> same here, please squash that code in the STE decoding and possible add
> those above comments in the commit msg
I will update it in V2.

Thanks,
Mostafa

[RFC PATCH 16/16] hw/arm/smmuv3: Enable stage-2 support

[Mostafa Saleh]

As everything is in place, we can use the new system property to
advertise which stage is supported and remove bad_ste from STE
stage2 config.

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 hw/arm/smmuv3.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 5f792d96ab..4b66760104 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -256,7 +256,14 @@ static void smmuv3_init_regs(SMMUv3State *s)
      * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
      *       multi-level stream table
      */
-    s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S1P, 1); /* stage 1 supported */
+    if (STAGE1_SUPPORTED(s->features)) {
+        s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S1P, 1);
+    }
+
+    if (STAGE2_SUPPORTED(s->features)) {
+        s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S2P, 1);
+    }
+
     s->idr[0] = FIELD_DP32(s->idr[0], IDR0, TTF, 2); /* AArch64 PTW only */
     s->idr[0] = FIELD_DP32(s->idr[0], IDR0, COHACC, 1); /* IO coherent */
     s->idr[0] = FIELD_DP32(s->idr[0], IDR0, ASID16, 1); /* 16-bit ASID */
@@ -451,10 +458,6 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
             qemu_log_mask(LOG_UNIMP, "SMMUv3 Stall not implemented!\n");
             goto bad_ste;
         }
-
-        /* This is still here as stage 2 has not been fully enabled yet. */
-        qemu_log_mask(LOG_UNIMP, "SMMUv3 does not support stage 2 yet\n");
-        goto bad_ste;
     }
 
     if (STE_S1CDMAX(ste) != 0) {
-- 
2.39.1.519.gcb327c4b5f-goog