* [PATCH v2] hw/arm/smmuv3: add support for stage 1 access fault
@ 2024-02-13 8:22 Luc Michel
2024-02-13 10:12 ` Mostafa Saleh
2024-02-15 7:22 ` Eric Auger
0 siblings, 2 replies; 4+ messages in thread
From: Luc Michel @ 2024-02-13 8:22 UTC (permalink / raw)
To: qemu-devel, qemu-arm
Cc: Luc Michel, Eric Auger, Peter Maydell, Francisco Iglesias
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] hw/arm/smmuv3: add support for stage 1 access fault
2024-02-13 8:22 [PATCH v2] hw/arm/smmuv3: add support for stage 1 access fault Luc Michel
@ 2024-02-13 10:12 ` Mostafa Saleh
2024-02-15 7:22 ` Eric Auger
1 sibling, 0 replies; 4+ messages in thread
From: Mostafa Saleh @ 2024-02-13 10:12 UTC (permalink / raw)
To: Luc Michel
Cc: qemu-devel, qemu-arm, Eric Auger, Peter Maydell,
Francisco Iglesias
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] hw/arm/smmuv3: add support for stage 1 access fault
2024-02-13 8:22 [PATCH v2] hw/arm/smmuv3: add support for stage 1 access fault Luc Michel
2024-02-13 10:12 ` Mostafa Saleh
@ 2024-02-15 7:22 ` Eric Auger
2024-02-15 13:44 ` Peter Maydell
1 sibling, 1 reply; 4+ messages in thread
From: Eric Auger @ 2024-02-15 7:22 UTC (permalink / raw)
To: Luc Michel, qemu-devel, qemu-arm; +Cc: Peter Maydell, Francisco Iglesias
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] hw/arm/smmuv3: add support for stage 1 access fault
2024-02-15 7:22 ` Eric Auger
@ 2024-02-15 13:44 ` Peter Maydell
0 siblings, 0 replies; 4+ messages in thread
From: Peter Maydell @ 2024-02-15 13:44 UTC (permalink / raw)
To: eric.auger; +Cc: Luc Michel, qemu-devel, qemu-arm, Francisco Iglesias
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-02-15 13:45 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-13 8:22 [PATCH v2] hw/arm/smmuv3: add support for stage 1 access fault Luc Michel
2024-02-13 10:12 ` Mostafa Saleh
2024-02-15 7:22 ` Eric Auger
2024-02-15 13:44 ` Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).