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>
---
Changes in v2:
- check if S1 is enabled(not supported) when reading S1 TT.
---
 hw/arm/smmuv3.c | 45 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 11 deletions(-)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index dc74a5442d..ce193e9598 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -697,6 +697,9 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg,
     STE ste;
     CD cd;
 
+    /* ASID defaults to -1 (if s1 is not supported). */
+    cfg->asid = -1;
+
     ret = smmu_find_ste(s, sid, &ste, event);
     if (ret) {
         return ret;
@@ -787,6 +790,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,
@@ -822,21 +826,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 (cfg->stage == 1) {
+        /* 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.2.637.g21b0678d19-goog

Hi Mostafa,

On 2/26/23 23:06, 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>
> ---
> Changes in v2:
> - check if S1 is enabled(not supported) when reading S1 TT.
> ---
>  hw/arm/smmuv3.c | 45 ++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index dc74a5442d..ce193e9598 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -697,6 +697,9 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg,
>      STE ste;
>      CD cd;
>  
> +    /* ASID defaults to -1 (if s1 is not supported). */
> +    cfg->asid = -1;
> +
>      ret = smmu_find_ste(s, sid, &ste, event);
>      if (ret) {
>          return ret;
> @@ -787,6 +790,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,
> @@ -822,21 +826,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 (cfg->stage == 1) {
> +        /* 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 = {
Move the declaration at the top. Also rename temp into tt to be more
explicit about what it is?
> +        .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;
Besides, looks good to me

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

Thanks

Eric

Hi Eric,

On Mon, Mar 20, 2023 at 05:05:31PM +0100, Eric Auger wrote:
> > +    /*
> > +     * 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 = {
> Move the declaration at the top. Also rename temp into tt to be more
> explicit about what it is?
I will move it to the top and remove granule_sz and tsz and just assign
values to this struct.
There is a pointer already called tt, I can call it tt_combined as
ideally this will hold the combined attributes for the TLB lookup.

Thanks,
Mostafa