Message ID | 20240429032403.74910-15-smostafa@google.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | SMMUv3 nested translation support | expand |
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? > - 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 >
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;
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(-)