Everything is in place, add the last missing bits:
- Handle fault checking according to the actual PTW event and not the
the translation stage.
- Consolidate parsing of STE cfg and setting translation stage.
Advertise nesting if stage requested is "nested".
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
hw/arm/smmuv3.c | 50 +++++++++++++++++++++++++++++++++----------------
1 file changed, 34 insertions(+), 16 deletions(-)
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 96d07234fe..88f6473d33 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -34,9 +34,10 @@
#include "smmuv3-internal.h"
#include "smmu-internal.h"
-#define PTW_RECORD_FAULT(cfg) (((cfg)->stage == SMMU_STAGE_1) ? \
- (cfg)->record_faults : \
- (cfg)->s2cfg.record_faults)
+#define PTW_RECORD_FAULT(ptw_info, cfg) (((ptw_info).stage == SMMU_STAGE_1 && \
+ (cfg)->record_faults) || \
+ ((ptw_info).stage == SMMU_STAGE_2 && \
+ (cfg)->s2cfg.record_faults))
/**
* smmuv3_trigger_irq - pulse @irq if enabled and update
@@ -260,6 +261,9 @@ static void smmuv3_init_regs(SMMUv3State *s)
/* Based on sys property, the stages supported in smmu will be advertised.*/
if (s->stage && !strcmp("2", s->stage)) {
s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S2P, 1);
+ } else if (s->stage && !strcmp("nested", s->stage)) {
+ s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S1P, 1);
+ s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S2P, 1);
} else {
s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S1P, 1);
}
@@ -422,8 +426,6 @@ static bool s2_pgtable_config_valid(uint8_t sl0, uint8_t t0sz, uint8_t gran)
static int decode_ste_s2_cfg(SMMUTransCfg *cfg, STE *ste)
{
- cfg->stage = SMMU_STAGE_2;
-
if (STE_S2AA64(ste) == 0x0) {
qemu_log_mask(LOG_UNIMP,
"SMMUv3 AArch32 tables not supported\n");
@@ -506,6 +508,27 @@ bad_ste:
return -EINVAL;
}
+static void decode_ste_config(SMMUTransCfg *cfg, uint32_t config)
+{
+
+ if (STE_CFG_ABORT(config)) {
+ cfg->aborted = true;
+ return;
+ }
+ if (STE_CFG_BYPASS(config)) {
+ cfg->bypassed = true;
+ return;
+ }
+
+ if (STE_CFG_S1_ENABLED(config)) {
+ cfg->stage = SMMU_STAGE_1;
+ }
+
+ if (STE_CFG_S2_ENABLED(config)) {
+ cfg->stage |= SMMU_STAGE_2;
+ }
+}
+
/* Returns < 0 in case of invalid STE, 0 otherwise */
static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
STE *ste, SMMUEventInfo *event)
@@ -522,13 +545,9 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
config = STE_CONFIG(ste);
- if (STE_CFG_ABORT(config)) {
- cfg->aborted = true;
- return 0;
- }
+ decode_ste_config(cfg, config);
- if (STE_CFG_BYPASS(config)) {
- cfg->bypassed = true;
+ if (cfg->aborted || cfg->bypassed) {
return 0;
}
@@ -701,7 +720,6 @@ static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg,
/* we support only those at the moment */
cfg->aa64 = true;
- cfg->stage = SMMU_STAGE_1;
cfg->oas = oas2bits(CD_IPS(cd));
cfg->oas = MIN(oas2bits(SMMU_IDR5_OAS), cfg->oas);
@@ -901,7 +919,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
event->u.f_walk_eabt.addr2 = ptw_info.addr;
break;
case SMMU_PTW_ERR_TRANSLATION:
- if (PTW_RECORD_FAULT(cfg)) {
+ if (PTW_RECORD_FAULT(ptw_info, cfg)) {
event->type = SMMU_EVT_F_TRANSLATION;
event->u.f_translation.addr = addr;
event->u.f_translation.addr2 = ptw_info.addr;
@@ -910,7 +928,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
}
break;
case SMMU_PTW_ERR_ADDR_SIZE:
- if (PTW_RECORD_FAULT(cfg)) {
+ if (PTW_RECORD_FAULT(ptw_info, cfg)) {
event->type = SMMU_EVT_F_ADDR_SIZE;
event->u.f_addr_size.addr = addr;
event->u.f_addr_size.addr2 = ptw_info.addr;
@@ -919,7 +937,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
}
break;
case SMMU_PTW_ERR_ACCESS:
- if (PTW_RECORD_FAULT(cfg)) {
+ if (PTW_RECORD_FAULT(ptw_info, cfg)) {
event->type = SMMU_EVT_F_ACCESS;
event->u.f_access.addr = addr;
event->u.f_access.addr2 = ptw_info.addr;
@@ -928,7 +946,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
}
break;
case SMMU_PTW_ERR_PERMISSION:
- if (PTW_RECORD_FAULT(cfg)) {
+ if (PTW_RECORD_FAULT(ptw_info, cfg)) {
event->type = SMMU_EVT_F_PERMISSION;
event->u.f_permission.addr = addr;
event->u.f_permission.addr2 = ptw_info.addr;
--
2.44.0.769.g3c40516874-googHi Mostafa,
On 4/29/24 05:23, Mostafa Saleh wrote:
> Everything is in place, add the last missing bits:
> - Handle fault checking according to the actual PTW event and not the
> the translation stage.
missing the "why". Can't it be moved in a separate patch?
> - Consolidate parsing of STE cfg and setting translation stage.
>
> Advertise nesting if stage requested is "nested".
I would move the introduction of the nested option in a separate patch
and in the associated commit msg properly document how the new option
shall be used.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
> hw/arm/smmuv3.c | 50 +++++++++++++++++++++++++++++++++----------------
> 1 file changed, 34 insertions(+), 16 deletions(-)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 96d07234fe..88f6473d33 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -34,9 +34,10 @@
> #include "smmuv3-internal.h"
> #include "smmu-internal.h"
>
> -#define PTW_RECORD_FAULT(cfg) (((cfg)->stage == SMMU_STAGE_1) ? \
> - (cfg)->record_faults : \
> - (cfg)->s2cfg.record_faults)
> +#define PTW_RECORD_FAULT(ptw_info, cfg) (((ptw_info).stage == SMMU_STAGE_1 && \
> + (cfg)->record_faults) || \
> + ((ptw_info).stage == SMMU_STAGE_2 && \
> + (cfg)->s2cfg.record_faults))
>
> /**
> * smmuv3_trigger_irq - pulse @irq if enabled and update
> @@ -260,6 +261,9 @@ static void smmuv3_init_regs(SMMUv3State *s)
> /* Based on sys property, the stages supported in smmu will be advertised.*/
> if (s->stage && !strcmp("2", s->stage)) {
> s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S2P, 1);
> + } else if (s->stage && !strcmp("nested", s->stage)) {
> + s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S1P, 1);
> + s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S2P, 1);
> } else {
> s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S1P, 1);
> }
> @@ -422,8 +426,6 @@ static bool s2_pgtable_config_valid(uint8_t sl0, uint8_t t0sz, uint8_t gran)
>
> static int decode_ste_s2_cfg(SMMUTransCfg *cfg, STE *ste)
> {
> - cfg->stage = SMMU_STAGE_2;
> -
> if (STE_S2AA64(ste) == 0x0) {
> qemu_log_mask(LOG_UNIMP,
> "SMMUv3 AArch32 tables not supported\n");
> @@ -506,6 +508,27 @@ bad_ste:
> return -EINVAL;
> }
>
> +static void decode_ste_config(SMMUTransCfg *cfg, uint32_t config)
> +{
> +
> + if (STE_CFG_ABORT(config)) {
> + cfg->aborted = true;
> + return;
> + }
> + if (STE_CFG_BYPASS(config)) {
> + cfg->bypassed = true;
> + return;
> + }
> +
> + if (STE_CFG_S1_ENABLED(config)) {
> + cfg->stage = SMMU_STAGE_1;
> + }
> +
> + if (STE_CFG_S2_ENABLED(config)) {
> + cfg->stage |= SMMU_STAGE_2;
> + }
> +}
> +
> /* Returns < 0 in case of invalid STE, 0 otherwise */
> static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
> STE *ste, SMMUEventInfo *event)
> @@ -522,13 +545,9 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
>
> config = STE_CONFIG(ste);
>
> - if (STE_CFG_ABORT(config)) {
> - cfg->aborted = true;
> - return 0;
> - }
> + decode_ste_config(cfg, config);
>
> - if (STE_CFG_BYPASS(config)) {
> - cfg->bypassed = true;
> + if (cfg->aborted || cfg->bypassed) {
> return 0;
> }
>
> @@ -701,7 +720,6 @@ static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg,
>
> /* we support only those at the moment */
> cfg->aa64 = true;
> - cfg->stage = SMMU_STAGE_1;
>
> cfg->oas = oas2bits(CD_IPS(cd));
> cfg->oas = MIN(oas2bits(SMMU_IDR5_OAS), cfg->oas);
> @@ -901,7 +919,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
> event->u.f_walk_eabt.addr2 = ptw_info.addr;
> break;
> case SMMU_PTW_ERR_TRANSLATION:
> - if (PTW_RECORD_FAULT(cfg)) {
> + if (PTW_RECORD_FAULT(ptw_info, cfg)) {
> event->type = SMMU_EVT_F_TRANSLATION;
> event->u.f_translation.addr = addr;
> event->u.f_translation.addr2 = ptw_info.addr;
> @@ -910,7 +928,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
> }
> break;
> case SMMU_PTW_ERR_ADDR_SIZE:
> - if (PTW_RECORD_FAULT(cfg)) {
> + if (PTW_RECORD_FAULT(ptw_info, cfg)) {
> event->type = SMMU_EVT_F_ADDR_SIZE;
> event->u.f_addr_size.addr = addr;
> event->u.f_addr_size.addr2 = ptw_info.addr;
> @@ -919,7 +937,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
> }
> break;
> case SMMU_PTW_ERR_ACCESS:
> - if (PTW_RECORD_FAULT(cfg)) {
> + if (PTW_RECORD_FAULT(ptw_info, cfg)) {
> event->type = SMMU_EVT_F_ACCESS;
> event->u.f_access.addr = addr;
> event->u.f_access.addr2 = ptw_info.addr;
> @@ -928,7 +946,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
> }
> break;
> case SMMU_PTW_ERR_PERMISSION:
> - if (PTW_RECORD_FAULT(cfg)) {
> + if (PTW_RECORD_FAULT(ptw_info, cfg)) {
> event->type = SMMU_EVT_F_PERMISSION;
> event->u.f_permission.addr = addr;
> event->u.f_permission.addr2 = ptw_info.addr;
Thanks
Eric
Hi Eric,
On Mon, May 20, 2024 at 03:16:40PM +0200, Eric Auger wrote:
> Hi Mostafa,
>
> On 4/29/24 05:23, Mostafa Saleh wrote:
> > Everything is in place, add the last missing bits:
> > - Handle fault checking according to the actual PTW event and not the
> > the translation stage.
> missing the "why". Can't it be moved in a separate patch?
Sure, I will split.
Thanks,
Mostafa
> > - Consolidate parsing of STE cfg and setting translation stage.
> >
> > Advertise nesting if stage requested is "nested".
> I would move the introduction of the nested option in a separate patch
> and in the associated commit msg properly document how the new option
> shall be used.
> >
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > ---
> > hw/arm/smmuv3.c | 50 +++++++++++++++++++++++++++++++++----------------
> > 1 file changed, 34 insertions(+), 16 deletions(-)
> >
> > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> > index 96d07234fe..88f6473d33 100644
> > --- a/hw/arm/smmuv3.c
> > +++ b/hw/arm/smmuv3.c
> > @@ -34,9 +34,10 @@
> > #include "smmuv3-internal.h"
> > #include "smmu-internal.h"
> >
> > -#define PTW_RECORD_FAULT(cfg) (((cfg)->stage == SMMU_STAGE_1) ? \
> > - (cfg)->record_faults : \
> > - (cfg)->s2cfg.record_faults)
> > +#define PTW_RECORD_FAULT(ptw_info, cfg) (((ptw_info).stage == SMMU_STAGE_1 && \
> > + (cfg)->record_faults) || \
> > + ((ptw_info).stage == SMMU_STAGE_2 && \
> > + (cfg)->s2cfg.record_faults))
> >
> > /**
> > * smmuv3_trigger_irq - pulse @irq if enabled and update
> > @@ -260,6 +261,9 @@ static void smmuv3_init_regs(SMMUv3State *s)
> > /* Based on sys property, the stages supported in smmu will be advertised.*/
> > if (s->stage && !strcmp("2", s->stage)) {
> > s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S2P, 1);
> > + } else if (s->stage && !strcmp("nested", s->stage)) {
> > + s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S1P, 1);
> > + s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S2P, 1);
> > } else {
> > s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S1P, 1);
> > }
> > @@ -422,8 +426,6 @@ static bool s2_pgtable_config_valid(uint8_t sl0, uint8_t t0sz, uint8_t gran)
> >
> > static int decode_ste_s2_cfg(SMMUTransCfg *cfg, STE *ste)
> > {
> > - cfg->stage = SMMU_STAGE_2;
> > -
> > if (STE_S2AA64(ste) == 0x0) {
> > qemu_log_mask(LOG_UNIMP,
> > "SMMUv3 AArch32 tables not supported\n");
> > @@ -506,6 +508,27 @@ bad_ste:
> > return -EINVAL;
> > }
> >
> > +static void decode_ste_config(SMMUTransCfg *cfg, uint32_t config)
> > +{
> > +
> > + if (STE_CFG_ABORT(config)) {
> > + cfg->aborted = true;
> > + return;
> > + }
> > + if (STE_CFG_BYPASS(config)) {
> > + cfg->bypassed = true;
> > + return;
> > + }
> > +
> > + if (STE_CFG_S1_ENABLED(config)) {
> > + cfg->stage = SMMU_STAGE_1;
> > + }
> > +
> > + if (STE_CFG_S2_ENABLED(config)) {
> > + cfg->stage |= SMMU_STAGE_2;
> > + }
> > +}
> > +
> > /* Returns < 0 in case of invalid STE, 0 otherwise */
> > static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
> > STE *ste, SMMUEventInfo *event)
> > @@ -522,13 +545,9 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
> >
> > config = STE_CONFIG(ste);
> >
> > - if (STE_CFG_ABORT(config)) {
> > - cfg->aborted = true;
> > - return 0;
> > - }
> > + decode_ste_config(cfg, config);
> >
> > - if (STE_CFG_BYPASS(config)) {
> > - cfg->bypassed = true;
> > + if (cfg->aborted || cfg->bypassed) {
> > return 0;
> > }
> >
> > @@ -701,7 +720,6 @@ static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg,
> >
> > /* we support only those at the moment */
> > cfg->aa64 = true;
> > - cfg->stage = SMMU_STAGE_1;
> >
> > cfg->oas = oas2bits(CD_IPS(cd));
> > cfg->oas = MIN(oas2bits(SMMU_IDR5_OAS), cfg->oas);
> > @@ -901,7 +919,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
> > event->u.f_walk_eabt.addr2 = ptw_info.addr;
> > break;
> > case SMMU_PTW_ERR_TRANSLATION:
> > - if (PTW_RECORD_FAULT(cfg)) {
> > + if (PTW_RECORD_FAULT(ptw_info, cfg)) {
> > event->type = SMMU_EVT_F_TRANSLATION;
> > event->u.f_translation.addr = addr;
> > event->u.f_translation.addr2 = ptw_info.addr;
> > @@ -910,7 +928,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
> > }
> > break;
> > case SMMU_PTW_ERR_ADDR_SIZE:
> > - if (PTW_RECORD_FAULT(cfg)) {
> > + if (PTW_RECORD_FAULT(ptw_info, cfg)) {
> > event->type = SMMU_EVT_F_ADDR_SIZE;
> > event->u.f_addr_size.addr = addr;
> > event->u.f_addr_size.addr2 = ptw_info.addr;
> > @@ -919,7 +937,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
> > }
> > break;
> > case SMMU_PTW_ERR_ACCESS:
> > - if (PTW_RECORD_FAULT(cfg)) {
> > + if (PTW_RECORD_FAULT(ptw_info, cfg)) {
> > event->type = SMMU_EVT_F_ACCESS;
> > event->u.f_access.addr = addr;
> > event->u.f_access.addr2 = ptw_info.addr;
> > @@ -928,7 +946,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
> > }
> > break;
> > case SMMU_PTW_ERR_PERMISSION:
> > - if (PTW_RECORD_FAULT(cfg)) {
> > + if (PTW_RECORD_FAULT(ptw_info, cfg)) {
> > event->type = SMMU_EVT_F_PERMISSION;
> > event->u.f_permission.addr = addr;
> > event->u.f_permission.addr2 = ptw_info.addr;
> Thanks
>
> Eric
>
© 2016 - 2026 Red Hat, Inc.