1. Patchew
  2. QEMU
  3. View series

[PATCH v2] hw/arm/smmuv3: add support for stage 1 access fault

Luc Michel posted 1 patch 1 year ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240213082211.3330400-1-luc.michel@amd.com
Maintainers: Eric Auger <eric.auger@redhat.com>, Peter Maydell <peter.maydell@linaro.org>
hw/arm/smmuv3-internal.h     |  1 +
include/hw/arm/smmu-common.h |  1 +
hw/arm/smmu-common.c         | 10 ++++++++++
hw/arm/smmuv3.c              |  1 +
4 files changed, 13 insertions(+)
[PATCH v2] hw/arm/smmuv3: add support for stage 1 access fault
Posted by Luc Michel 1 year ago 
An access fault is raised when the Access Flag is not set in the
looked-up PTE and the AFFD field is not set in the corresponding context
descriptor. This was already implemented for stage 2. Implement it for
stage 1 as well.

Signed-off-by: Luc Michel <luc.michel@amd.com>
---

v2: drop erroneous submodule modification

---

 hw/arm/smmuv3-internal.h     |  1 +
 include/hw/arm/smmu-common.h |  1 +
 hw/arm/smmu-common.c         | 10 ++++++++++
 hw/arm/smmuv3.c              |  1 +
 4 files changed, 13 insertions(+)

diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index e987bc4686b..e4dd11e1e62 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -622,10 +622,11 @@ static inline int pa_range(STE *ste)
 #define CD_TSZ(x, sel)   extract32((x)->word[0], (16 * (sel)) + 0, 6)
 #define CD_TG(x, sel)    extract32((x)->word[0], (16 * (sel)) + 6, 2)
 #define CD_EPD(x, sel)   extract32((x)->word[0], (16 * (sel)) + 14, 1)
 #define CD_ENDI(x)       extract32((x)->word[0], 15, 1)
 #define CD_IPS(x)        extract32((x)->word[1], 0 , 3)
+#define CD_AFFD(x)       extract32((x)->word[1], 3 , 1)
 #define CD_TBI(x)        extract32((x)->word[1], 6 , 2)
 #define CD_HD(x)         extract32((x)->word[1], 10 , 1)
 #define CD_HA(x)         extract32((x)->word[1], 11 , 1)
 #define CD_S(x)          extract32((x)->word[1], 12, 1)
 #define CD_R(x)          extract32((x)->word[1], 13, 1)
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index fd8d772da11..5ec2e6c1a43 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -90,10 +90,11 @@ typedef struct SMMUTransCfg {
     /* Shared fields between stage-1 and stage-2. */
     int stage;                 /* translation stage */
     bool disabled;             /* smmu is disabled */
     bool bypassed;             /* translation is bypassed */
     bool aborted;              /* translation is aborted */
+    bool affd;                 /* AF fault disable */
     uint32_t iotlb_hits;       /* counts IOTLB hits */
     uint32_t iotlb_misses;     /* counts IOTLB misses*/
     /* Used by stage-1 only. */
     bool aa64;                 /* arch64 or aarch32 translation table */
     bool record_faults;        /* record fault events */
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 9a8ac45431a..09ff72e55f5 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -362,10 +362,20 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
                                         &block_size);
             trace_smmu_ptw_block_pte(stage, level, baseaddr,
                                      pte_addr, pte, iova, gpa,
                                      block_size >> 20);
         }
+
+        /*
+         * If AFFD and PTE.AF are 0 => fault. (5.4. Context Descriptor)
+         * An Access fault takes priority over a Permission fault.
+         */
+        if (!PTE_AF(pte) && !cfg->affd) {
+            info->type = SMMU_PTW_ERR_ACCESS;
+            goto error;
+        }
+
         ap = PTE_AP(pte);
         if (is_permission_fault(ap, perm)) {
             info->type = SMMU_PTW_ERR_PERMISSION;
             goto error;
         }
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 68eeef3e1d4..c416b8c0030 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -682,10 +682,11 @@ static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event)
 
     cfg->oas = oas2bits(CD_IPS(cd));
     cfg->oas = MIN(oas2bits(SMMU_IDR5_OAS), cfg->oas);
     cfg->tbi = CD_TBI(cd);
     cfg->asid = CD_ASID(cd);
+    cfg->affd = CD_AFFD(cd);
 
     trace_smmuv3_decode_cd(cfg->oas);
 
     /* decode data dependent on TT */
     for (i = 0; i <= 1; i++) {
-- 
2.39.2
Re: [PATCH v2] hw/arm/smmuv3: add support for stage 1 access fault
Posted by Eric Auger 1 year ago 
Hi Luc,
On 2/13/24 09:22, Luc Michel wrote:
> An access fault is raised when the Access Flag is not set in the
> looked-up PTE and the AFFD field is not set in the corresponding context
> descriptor. This was already implemented for stage 2. Implement it for
> stage 1 as well.
>
> Signed-off-by: Luc Michel <luc.michel@amd.com>
> ---
>
> v2: drop erroneous submodule modification
>
> ---
>
>  hw/arm/smmuv3-internal.h     |  1 +
>  include/hw/arm/smmu-common.h |  1 +
>  hw/arm/smmu-common.c         | 10 ++++++++++
>  hw/arm/smmuv3.c              |  1 +
>  4 files changed, 13 insertions(+)
>
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index e987bc4686b..e4dd11e1e62 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -622,10 +622,11 @@ static inline int pa_range(STE *ste)
>  #define CD_TSZ(x, sel)   extract32((x)->word[0], (16 * (sel)) + 0, 6)
>  #define CD_TG(x, sel)    extract32((x)->word[0], (16 * (sel)) + 6, 2)
>  #define CD_EPD(x, sel)   extract32((x)->word[0], (16 * (sel)) + 14, 1)
>  #define CD_ENDI(x)       extract32((x)->word[0], 15, 1)
>  #define CD_IPS(x)        extract32((x)->word[1], 0 , 3)
> +#define CD_AFFD(x)       extract32((x)->word[1], 3 , 1)
>  #define CD_TBI(x)        extract32((x)->word[1], 6 , 2)
>  #define CD_HD(x)         extract32((x)->word[1], 10 , 1)
>  #define CD_HA(x)         extract32((x)->word[1], 11 , 1)
>  #define CD_S(x)          extract32((x)->word[1], 12, 1)
>  #define CD_R(x)          extract32((x)->word[1], 13, 1)
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index fd8d772da11..5ec2e6c1a43 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -90,10 +90,11 @@ typedef struct SMMUTransCfg {
>      /* Shared fields between stage-1 and stage-2. */
>      int stage;                 /* translation stage */
>      bool disabled;             /* smmu is disabled */
>      bool bypassed;             /* translation is bypassed */
>      bool aborted;              /* translation is aborted */
> +    bool affd;                 /* AF fault disable */
>      uint32_t iotlb_hits;       /* counts IOTLB hits */
>      uint32_t iotlb_misses;     /* counts IOTLB misses*/
>      /* Used by stage-1 only. */
>      bool aa64;                 /* arch64 or aarch32 translation table */
>      bool record_faults;        /* record fault events */
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 9a8ac45431a..09ff72e55f5 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -362,10 +362,20 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
>                                          &block_size);
>              trace_smmu_ptw_block_pte(stage, level, baseaddr,
>                                       pte_addr, pte, iova, gpa,
>                                       block_size >> 20);
>          }
> +
> +        /*
> +         * If AFFD and PTE.AF are 0 => fault. (5.4. Context Descriptor)
> +         * An Access fault takes priority over a Permission fault.
nit: you may precise that this holds because HTTU is not currently supported
> +         */
> +        if (!PTE_AF(pte) && !cfg->affd) {
> +            info->type = SMMU_PTW_ERR_ACCESS;
> +            goto error;
> +        }
> +
>          ap = PTE_AP(pte);
>          if (is_permission_fault(ap, perm)) {
>              info->type = SMMU_PTW_ERR_PERMISSION;
>              goto error;
>          }
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 68eeef3e1d4..c416b8c0030 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -682,10 +682,11 @@ static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event)
>  
>      cfg->oas = oas2bits(CD_IPS(cd));
>      cfg->oas = MIN(oas2bits(SMMU_IDR5_OAS), cfg->oas);
>      cfg->tbi = CD_TBI(cd);
>      cfg->asid = CD_ASID(cd);
> +    cfg->affd = CD_AFFD(cd);
>  
>      trace_smmuv3_decode_cd(cfg->oas);
>  
>      /* decode data dependent on TT */
>      for (i = 0; i <= 1; i++) {
Besides, looks good to me
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
Re: [PATCH v2] hw/arm/smmuv3: add support for stage 1 access fault
Posted by Peter Maydell 12 months ago 
On Thu, 15 Feb 2024 at 07:22, Eric Auger <eric.auger@redhat.com> wrote:
>
> Hi Luc,
> On 2/13/24 09:22, Luc Michel wrote:
> > An access fault is raised when the Access Flag is not set in the
> > looked-up PTE and the AFFD field is not set in the corresponding context
> > descriptor. This was already implemented for stage 2. Implement it for
> > stage 1 as well.
> >
> > Signed-off-by: Luc Michel <luc.michel@amd.com>
> > ---
> >
> > v2: drop erroneous submodule modification
> >
> > ---
> >
> >  hw/arm/smmuv3-internal.h     |  1 +
> >  include/hw/arm/smmu-common.h |  1 +
> >  hw/arm/smmu-common.c         | 10 ++++++++++
> >  hw/arm/smmuv3.c              |  1 +
> >  4 files changed, 13 insertions(+)
> >
> > diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> > index e987bc4686b..e4dd11e1e62 100644
> > --- a/hw/arm/smmuv3-internal.h
> > +++ b/hw/arm/smmuv3-internal.h
> > @@ -622,10 +622,11 @@ static inline int pa_range(STE *ste)
> >  #define CD_TSZ(x, sel)   extract32((x)->word[0], (16 * (sel)) + 0, 6)
> >  #define CD_TG(x, sel)    extract32((x)->word[0], (16 * (sel)) + 6, 2)
> >  #define CD_EPD(x, sel)   extract32((x)->word[0], (16 * (sel)) + 14, 1)
> >  #define CD_ENDI(x)       extract32((x)->word[0], 15, 1)
> >  #define CD_IPS(x)        extract32((x)->word[1], 0 , 3)
> > +#define CD_AFFD(x)       extract32((x)->word[1], 3 , 1)
> >  #define CD_TBI(x)        extract32((x)->word[1], 6 , 2)
> >  #define CD_HD(x)         extract32((x)->word[1], 10 , 1)
> >  #define CD_HA(x)         extract32((x)->word[1], 11 , 1)
> >  #define CD_S(x)          extract32((x)->word[1], 12, 1)
> >  #define CD_R(x)          extract32((x)->word[1], 13, 1)
> > diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> > index fd8d772da11..5ec2e6c1a43 100644
> > --- a/include/hw/arm/smmu-common.h
> > +++ b/include/hw/arm/smmu-common.h
> > @@ -90,10 +90,11 @@ typedef struct SMMUTransCfg {
> >      /* Shared fields between stage-1 and stage-2. */
> >      int stage;                 /* translation stage */
> >      bool disabled;             /* smmu is disabled */
> >      bool bypassed;             /* translation is bypassed */
> >      bool aborted;              /* translation is aborted */
> > +    bool affd;                 /* AF fault disable */
> >      uint32_t iotlb_hits;       /* counts IOTLB hits */
> >      uint32_t iotlb_misses;     /* counts IOTLB misses*/
> >      /* Used by stage-1 only. */
> >      bool aa64;                 /* arch64 or aarch32 translation table */
> >      bool record_faults;        /* record fault events */
> > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> > index 9a8ac45431a..09ff72e55f5 100644
> > --- a/hw/arm/smmu-common.c
> > +++ b/hw/arm/smmu-common.c
> > @@ -362,10 +362,20 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
> >                                          &block_size);
> >              trace_smmu_ptw_block_pte(stage, level, baseaddr,
> >                                       pte_addr, pte, iova, gpa,
> >                                       block_size >> 20);
> >          }
> > +
> > +        /*
> > +         * If AFFD and PTE.AF are 0 => fault. (5.4. Context Descriptor)
> > +         * An Access fault takes priority over a Permission fault.
> nit: you may precise that this holds because HTTU is not currently supported
> > +         */

I tweaked this comment text to read

+         * QEMU does not currently implement HTTU, so if AFFD and PTE.AF
+         * are 0 we take an Access flag fault. (5.4. Context Descriptor)
+         * An Access flag fault takes priority over a Permission fault.

and have applied the patch to target-arm.next, thanks.

-- PMM
Re: [PATCH v2] hw/arm/smmuv3: add support for stage 1 access fault
Posted by Mostafa Saleh 1 year ago 
Hi Luc,

On Tue, Feb 13, 2024 at 09:22:11AM +0100, Luc Michel wrote:
> An access fault is raised when the Access Flag is not set in the
> looked-up PTE and the AFFD field is not set in the corresponding context
> descriptor. This was already implemented for stage 2. Implement it for
> stage 1 as well.
> 

I noticed the same thing when writing PTW for stage-2, I don’t think there is
any reason this is not supported for stage-1, as SMMUv3.0-HTTU* are not
supported any SW broken will be due to an existing SW bug.

Reviewed-by: Mostafa Saleh <smostafa@google.com>
Tested-by: Mostafa Saleh <smostafa@google.com>


> Signed-off-by: Luc Michel <luc.michel@amd.com>
> ---
> 
> v2: drop erroneous submodule modification
> 
> ---
> 
>  hw/arm/smmuv3-internal.h     |  1 +
>  include/hw/arm/smmu-common.h |  1 +
>  hw/arm/smmu-common.c         | 10 ++++++++++
>  hw/arm/smmuv3.c              |  1 +
>  4 files changed, 13 insertions(+)
> 
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index e987bc4686b..e4dd11e1e62 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -622,10 +622,11 @@ static inline int pa_range(STE *ste)
>  #define CD_TSZ(x, sel)   extract32((x)->word[0], (16 * (sel)) + 0, 6)
>  #define CD_TG(x, sel)    extract32((x)->word[0], (16 * (sel)) + 6, 2)
>  #define CD_EPD(x, sel)   extract32((x)->word[0], (16 * (sel)) + 14, 1)
>  #define CD_ENDI(x)       extract32((x)->word[0], 15, 1)
>  #define CD_IPS(x)        extract32((x)->word[1], 0 , 3)
> +#define CD_AFFD(x)       extract32((x)->word[1], 3 , 1)
>  #define CD_TBI(x)        extract32((x)->word[1], 6 , 2)
>  #define CD_HD(x)         extract32((x)->word[1], 10 , 1)
>  #define CD_HA(x)         extract32((x)->word[1], 11 , 1)
>  #define CD_S(x)          extract32((x)->word[1], 12, 1)
>  #define CD_R(x)          extract32((x)->word[1], 13, 1)
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index fd8d772da11..5ec2e6c1a43 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -90,10 +90,11 @@ typedef struct SMMUTransCfg {
>      /* Shared fields between stage-1 and stage-2. */
>      int stage;                 /* translation stage */
>      bool disabled;             /* smmu is disabled */
>      bool bypassed;             /* translation is bypassed */
>      bool aborted;              /* translation is aborted */
> +    bool affd;                 /* AF fault disable */
>      uint32_t iotlb_hits;       /* counts IOTLB hits */
>      uint32_t iotlb_misses;     /* counts IOTLB misses*/
>      /* Used by stage-1 only. */
>      bool aa64;                 /* arch64 or aarch32 translation table */
>      bool record_faults;        /* record fault events */
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 9a8ac45431a..09ff72e55f5 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -362,10 +362,20 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
>                                          &block_size);
>              trace_smmu_ptw_block_pte(stage, level, baseaddr,
>                                       pte_addr, pte, iova, gpa,
>                                       block_size >> 20);
>          }
> +
> +        /*
> +         * If AFFD and PTE.AF are 0 => fault. (5.4. Context Descriptor)
> +         * An Access fault takes priority over a Permission fault.
> +         */
> +        if (!PTE_AF(pte) && !cfg->affd) {
> +            info->type = SMMU_PTW_ERR_ACCESS;
> +            goto error;
> +        }
> +
>          ap = PTE_AP(pte);
>          if (is_permission_fault(ap, perm)) {
>              info->type = SMMU_PTW_ERR_PERMISSION;
>              goto error;
>          }
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 68eeef3e1d4..c416b8c0030 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -682,10 +682,11 @@ static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event)
>  
>      cfg->oas = oas2bits(CD_IPS(cd));
>      cfg->oas = MIN(oas2bits(SMMU_IDR5_OAS), cfg->oas);
>      cfg->tbi = CD_TBI(cd);
>      cfg->asid = CD_ASID(cd);
> +    cfg->affd = CD_AFFD(cd);
>  
>      trace_smmuv3_decode_cd(cfg->oas);
>  
>      /* decode data dependent on TT */
>      for (i = 0; i <= 1; i++) {
> -- 
> 2.39.2

Thanks,
Mostafa

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.