| Message ID | 20240213082211.3330400-1-luc.michel@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] hw/arm/smmuv3: add support for stage 1 access fault | expand |
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
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
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
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++) {
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(+)