| Message ID | 20240701110241.2005222-9-smostafa@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SMMUv3 nested translation support | expand |
On Mon, Jul 01, 2024 at 11:02:30AM +0000, Mostafa Saleh wrote: > According to ARM SMMU architecture specification (ARM IHI 0070 F.b), > In "5.2 Stream Table Entry": > [51:6] S1ContextPtr > If Config[1] == 1 (stage 2 enabled), this pointer is an IPA translated by > stage 2 and the programmed value must be within the range of the IAS. > > In "5.4.1 CD notes": > The translation table walks performed from TTB0 or TTB1 are always performed > in IPA space if stage 2 translations are enabled. > > This patch implements translation of the S1 context descriptor pointer and > TTBx base addresses through the S2 stage (IPA -> PA) > > smmuv3_do_translate() is updated to have one arg which is translation > class, this is useful to: > - Decide wether a translation is stage-2 only or use the STE config. > - Populate the class in case of faults, WALK_EABT is left unchanged, > as it is always triggered from TT access so no need to use the > input class. > > In case for stage-2 only translation, used in the context of nested > translation, the stage and asid are saved and restored before and > after calling smmu_translate(). > > Translating CD or TTBx can fail for the following reasons: > 1) Large address size: This is described in > (3.4.3 Address sizes of SMMU-originated accesses) > - For CD ptr larger than IAS, for SMMUv3.1, it can trigger either > C_BAD_STE or Translation fault, we implement the latter as it > requires no extra code. > - For TTBx, if larger than the effective stage 1 output address size, it > triggers C_BAD_CD. > > 2) Faults from PTWs (7.3 Event records) > - F_ADDR_SIZE: large address size after first level causes stage 2 Address > Size fault (Also in 3.4.3 Address sizes of SMMU-originated accesses) > - F_PERMISSION: Same as an address translation. However, when > CLASS == CD, the access is implicitly Data and a read. > - F_ACCESS: Same as an address translation. > - F_TRANSLATION: Same as an address translation. > - F_WALK_EABT: Same as an address translation. > These are already implemented in the PTW logic, so no extra handling > required. > > Reviewed-by: Eric Auger <eric.auger@redhat.com> > Signed-off-by: Mostafa Saleh <smostafa@google.com> > --- > hw/arm/smmuv3.c | 91 +++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 80 insertions(+), 11 deletions(-) > > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > index 229b3c388c..86f95c1e40 100644 > --- a/hw/arm/smmuv3.c > +++ b/hw/arm/smmuv3.c > @@ -337,14 +337,35 @@ static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf, > > } > > +static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, > + SMMUTransCfg *cfg, > + SMMUEventInfo *event, > + IOMMUAccessFlags flag, > + SMMUTLBEntry **out_entry, > + SMMUTranslationClass class); > /* @ssid > 0 not supported yet */ > -static int smmu_get_cd(SMMUv3State *s, STE *ste, uint32_t ssid, > - CD *buf, SMMUEventInfo *event) > +static int smmu_get_cd(SMMUv3State *s, STE *ste, SMMUTransCfg *cfg, > + uint32_t ssid, CD *buf, SMMUEventInfo *event) > { > dma_addr_t addr = STE_CTXPTR(ste); > int ret, i; > + SMMUTranslationStatus status; > + SMMUTLBEntry *entry; > > trace_smmuv3_get_cd(addr); > + > + if (cfg->stage == SMMU_NESTED) { > + status = smmuv3_do_translate(s, addr, cfg, event, > + IOMMU_RO, &entry, SMMU_CLASS_CD); > + > + /* Same PTW faults are reported but with CLASS = CD. */ > + if (status != SMMU_TRANS_SUCCESS) { In this case I think we're reporting InputAddr as the CD address, but it should be the IOVA > + return -EINVAL; > + } > + > + addr = CACHED_ENTRY_TO_ADDR(entry, addr); > + } > + > /* TODO: guarantee 64-bit single-copy atomicity */ > ret = dma_memory_read(&address_space_memory, addr, buf, sizeof(*buf), > MEMTXATTRS_UNSPECIFIED); > @@ -659,10 +680,13 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste, > return 0; > } > > -static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event) > +static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg, > + CD *cd, SMMUEventInfo *event) > { > int ret = -EINVAL; > int i; > + SMMUTranslationStatus status; > + SMMUTLBEntry *entry; > > if (!CD_VALID(cd) || !CD_AARCH64(cd)) { > goto bad_cd; > @@ -713,9 +737,26 @@ static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event) > > tt->tsz = tsz; > tt->ttb = CD_TTB(cd, i); > + > if (tt->ttb & ~(MAKE_64BIT_MASK(0, cfg->oas))) { > goto bad_cd; > } > + > + /* Translate the TTBx, from IPA to PA if nesting is enabled. */ > + if (cfg->stage == SMMU_NESTED) { > + status = smmuv3_do_translate(s, tt->ttb, cfg, event, IOMMU_RO, > + &entry, SMMU_CLASS_TT); > + /* > + * Same PTW faults are reported but with CLASS = TT. > + * If TTBx is larger than the effective stage 1 output addres > + * size, it reports C_BAD_CD, which is handled by the above case. > + */ > + if (status != SMMU_TRANS_SUCCESS) { Here too, we should report InputAddr as the IOVA > + return -EINVAL; > + } > + tt->ttb = CACHED_ENTRY_TO_ADDR(entry, tt->ttb); > + } > + > tt->had = CD_HAD(cd, i); > trace_smmuv3_decode_cd_tt(i, tt->tsz, tt->ttb, tt->granule_sz, tt->had); > } > @@ -767,12 +808,12 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg, > return 0; > } > > - ret = smmu_get_cd(s, &ste, 0 /* ssid */, &cd, event); > + ret = smmu_get_cd(s, &ste, cfg, 0 /* ssid */, &cd, event); > if (ret) { > return ret; > } > > - return decode_cd(cfg, &cd, event); > + return decode_cd(s, cfg, &cd, event); > } > > /** > @@ -832,13 +873,40 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, > SMMUTransCfg *cfg, > SMMUEventInfo *event, > IOMMUAccessFlags flag, > - SMMUTLBEntry **out_entry) > + SMMUTLBEntry **out_entry, > + SMMUTranslationClass class) > { > SMMUPTWEventInfo ptw_info = {}; > SMMUState *bs = ARM_SMMU(s); > SMMUTLBEntry *cached_entry = NULL; > + int asid, stage; > + bool S2_only = class != SMMU_CLASS_IN; > + > + /* > + * The function uses the argument class to indentify which stage is used: identify > + * - CLASS = IN: Means an input translation, determine the stage from STE. > + * - CLASS = CD: Means the addr is an IPA of the CD, and it would be > + * tranlsated using the stage-2. translated > + * - CLASS = TT: Means the addr is an IPA of the stage-1 translation table > + * and it would be tranlsated using the stage-2. translated > + * For the last 2 cases instead of having intrusive changes in the common > + * logic, we modify the cfg to be a stage-2 translation only in case of > + * nested, and then restore it after. > + */ > + if (S2_only) { "S2_only" seems a bit confusing because in the spec "stage 2-only" means absence of nesting, which is the opposite of what we're handling here. I don't have a good alternative though, maybe "desc_s2_translation" Thanks, Jean > + asid = cfg->asid; > + stage = cfg->stage; > + cfg->asid = -1; > + cfg->stage = SMMU_STAGE_2; > + } > > cached_entry = smmu_translate(bs, cfg, addr, flag, &ptw_info); > + > + if (S2_only) { > + cfg->asid = asid; > + cfg->stage = stage; > + } > + > if (!cached_entry) { > /* All faults from PTW has S2 field. */ > event->u.f_walk_eabt.s2 = (ptw_info.stage == SMMU_STAGE_2); > @@ -855,7 +923,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, > event->type = SMMU_EVT_F_TRANSLATION; > event->u.f_translation.addr = addr; > event->u.f_translation.addr2 = ptw_info.addr; > - event->u.f_translation.class = SMMU_CLASS_IN; > + event->u.f_translation.class = class; > event->u.f_translation.rnw = flag & 0x1; > } > break; > @@ -864,7 +932,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, > event->type = SMMU_EVT_F_ADDR_SIZE; > event->u.f_addr_size.addr = addr; > event->u.f_addr_size.addr2 = ptw_info.addr; > - event->u.f_addr_size.class = SMMU_CLASS_IN; > + event->u.f_addr_size.class = class; > event->u.f_addr_size.rnw = flag & 0x1; > } > break; > @@ -873,7 +941,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, > event->type = SMMU_EVT_F_ACCESS; > event->u.f_access.addr = addr; > event->u.f_access.addr2 = ptw_info.addr; > - event->u.f_access.class = SMMU_CLASS_IN; > + event->u.f_access.class = class; > event->u.f_access.rnw = flag & 0x1; > } > break; > @@ -882,7 +950,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, > event->type = SMMU_EVT_F_PERMISSION; > event->u.f_permission.addr = addr; > event->u.f_permission.addr2 = ptw_info.addr; > - event->u.f_permission.class = SMMU_CLASS_IN; > + event->u.f_permission.class = class; > event->u.f_permission.rnw = flag & 0x1; > } > break; > @@ -943,7 +1011,8 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr, > goto epilogue; > } > > - status = smmuv3_do_translate(s, addr, cfg, &event, flag, &cached_entry); > + status = smmuv3_do_translate(s, addr, cfg, &event, flag, > + &cached_entry, SMMU_CLASS_IN); > > epilogue: > qemu_mutex_unlock(&s->mutex); > -- > 2.45.2.803.g4e1b14247a-goog >
On 7/4/24 20:08, Jean-Philippe Brucker wrote: > On Mon, Jul 01, 2024 at 11:02:30AM +0000, Mostafa Saleh wrote: >> According to ARM SMMU architecture specification (ARM IHI 0070 F.b), >> In "5.2 Stream Table Entry": >> [51:6] S1ContextPtr >> If Config[1] == 1 (stage 2 enabled), this pointer is an IPA translated by >> stage 2 and the programmed value must be within the range of the IAS. >> >> In "5.4.1 CD notes": >> The translation table walks performed from TTB0 or TTB1 are always performed >> in IPA space if stage 2 translations are enabled. >> >> This patch implements translation of the S1 context descriptor pointer and >> TTBx base addresses through the S2 stage (IPA -> PA) >> >> smmuv3_do_translate() is updated to have one arg which is translation >> class, this is useful to: >> - Decide wether a translation is stage-2 only or use the STE config. >> - Populate the class in case of faults, WALK_EABT is left unchanged, >> as it is always triggered from TT access so no need to use the >> input class. >> >> In case for stage-2 only translation, used in the context of nested >> translation, the stage and asid are saved and restored before and >> after calling smmu_translate(). >> >> Translating CD or TTBx can fail for the following reasons: >> 1) Large address size: This is described in >> (3.4.3 Address sizes of SMMU-originated accesses) >> - For CD ptr larger than IAS, for SMMUv3.1, it can trigger either >> C_BAD_STE or Translation fault, we implement the latter as it >> requires no extra code. >> - For TTBx, if larger than the effective stage 1 output address size, it >> triggers C_BAD_CD. >> >> 2) Faults from PTWs (7.3 Event records) >> - F_ADDR_SIZE: large address size after first level causes stage 2 Address >> Size fault (Also in 3.4.3 Address sizes of SMMU-originated accesses) >> - F_PERMISSION: Same as an address translation. However, when >> CLASS == CD, the access is implicitly Data and a read. >> - F_ACCESS: Same as an address translation. >> - F_TRANSLATION: Same as an address translation. >> - F_WALK_EABT: Same as an address translation. >> These are already implemented in the PTW logic, so no extra handling >> required. >> >> Reviewed-by: Eric Auger <eric.auger@redhat.com> >> Signed-off-by: Mostafa Saleh <smostafa@google.com> >> --- >> hw/arm/smmuv3.c | 91 +++++++++++++++++++++++++++++++++++++++++++------ >> 1 file changed, 80 insertions(+), 11 deletions(-) >> >> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c >> index 229b3c388c..86f95c1e40 100644 >> --- a/hw/arm/smmuv3.c >> +++ b/hw/arm/smmuv3.c >> @@ -337,14 +337,35 @@ static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf, >> >> } >> >> +static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, >> + SMMUTransCfg *cfg, >> + SMMUEventInfo *event, >> + IOMMUAccessFlags flag, >> + SMMUTLBEntry **out_entry, >> + SMMUTranslationClass class); >> /* @ssid > 0 not supported yet */ >> -static int smmu_get_cd(SMMUv3State *s, STE *ste, uint32_t ssid, >> - CD *buf, SMMUEventInfo *event) >> +static int smmu_get_cd(SMMUv3State *s, STE *ste, SMMUTransCfg *cfg, >> + uint32_t ssid, CD *buf, SMMUEventInfo *event) >> { >> dma_addr_t addr = STE_CTXPTR(ste); >> int ret, i; >> + SMMUTranslationStatus status; >> + SMMUTLBEntry *entry; >> >> trace_smmuv3_get_cd(addr); >> + >> + if (cfg->stage == SMMU_NESTED) { >> + status = smmuv3_do_translate(s, addr, cfg, event, >> + IOMMU_RO, &entry, SMMU_CLASS_CD); >> + >> + /* Same PTW faults are reported but with CLASS = CD. */ >> + if (status != SMMU_TRANS_SUCCESS) { > In this case I think we're reporting InputAddr as the CD address, but it > should be the IOVA you're right. the InputAddr is defined as The 64-bit address input to the SMMU for the transaction that led to the event. Unfortunately we don't have an easy access to the IOVA in those functions. This suggest that we need to rework structs or calls. > >> + return -EINVAL; >> + } >> + >> + addr = CACHED_ENTRY_TO_ADDR(entry, addr); >> + } >> + >> /* TODO: guarantee 64-bit single-copy atomicity */ >> ret = dma_memory_read(&address_space_memory, addr, buf, sizeof(*buf), >> MEMTXATTRS_UNSPECIFIED); >> @@ -659,10 +680,13 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste, >> return 0; >> } >> >> -static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event) >> +static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg, >> + CD *cd, SMMUEventInfo *event) >> { >> int ret = -EINVAL; >> int i; >> + SMMUTranslationStatus status; >> + SMMUTLBEntry *entry; >> >> if (!CD_VALID(cd) || !CD_AARCH64(cd)) { >> goto bad_cd; >> @@ -713,9 +737,26 @@ static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event) >> >> tt->tsz = tsz; >> tt->ttb = CD_TTB(cd, i); >> + >> if (tt->ttb & ~(MAKE_64BIT_MASK(0, cfg->oas))) { >> goto bad_cd; >> } >> + >> + /* Translate the TTBx, from IPA to PA if nesting is enabled. */ >> + if (cfg->stage == SMMU_NESTED) { >> + status = smmuv3_do_translate(s, tt->ttb, cfg, event, IOMMU_RO, >> + &entry, SMMU_CLASS_TT); >> + /* >> + * Same PTW faults are reported but with CLASS = TT. >> + * If TTBx is larger than the effective stage 1 output addres >> + * size, it reports C_BAD_CD, which is handled by the above case. >> + */ >> + if (status != SMMU_TRANS_SUCCESS) { > Here too, we should report InputAddr as the IOVA > >> + return -EINVAL; >> + } >> + tt->ttb = CACHED_ENTRY_TO_ADDR(entry, tt->ttb); >> + } >> + >> tt->had = CD_HAD(cd, i); >> trace_smmuv3_decode_cd_tt(i, tt->tsz, tt->ttb, tt->granule_sz, tt->had); >> } >> @@ -767,12 +808,12 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg, >> return 0; >> } >> >> - ret = smmu_get_cd(s, &ste, 0 /* ssid */, &cd, event); >> + ret = smmu_get_cd(s, &ste, cfg, 0 /* ssid */, &cd, event); >> if (ret) { >> return ret; >> } >> >> - return decode_cd(cfg, &cd, event); >> + return decode_cd(s, cfg, &cd, event); >> } >> >> /** >> @@ -832,13 +873,40 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, >> SMMUTransCfg *cfg, >> SMMUEventInfo *event, >> IOMMUAccessFlags flag, >> - SMMUTLBEntry **out_entry) >> + SMMUTLBEntry **out_entry, >> + SMMUTranslationClass class) >> { >> SMMUPTWEventInfo ptw_info = {}; >> SMMUState *bs = ARM_SMMU(s); >> SMMUTLBEntry *cached_entry = NULL; >> + int asid, stage; >> + bool S2_only = class != SMMU_CLASS_IN; >> + >> + /* >> + * The function uses the argument class to indentify which stage is used: > identify > >> + * - CLASS = IN: Means an input translation, determine the stage from STE. >> + * - CLASS = CD: Means the addr is an IPA of the CD, and it would be >> + * tranlsated using the stage-2. > translated > >> + * - CLASS = TT: Means the addr is an IPA of the stage-1 translation table >> + * and it would be tranlsated using the stage-2. > translated > >> + * For the last 2 cases instead of having intrusive changes in the common >> + * logic, we modify the cfg to be a stage-2 translation only in case of >> + * nested, and then restore it after. >> + */ >> + if (S2_only) { > "S2_only" seems a bit confusing because in the spec "stage 2-only" means > absence of nesting, which is the opposite of what we're handling here. > I don't have a good alternative though, maybe "desc_s2_translation" In 7.3.13 F_WALK_EABT, "S2 descriptor fetch" terminology is used. Thanks Eric > > Thanks, > Jean > >> + asid = cfg->asid; >> + stage = cfg->stage; >> + cfg->asid = -1; >> + cfg->stage = SMMU_STAGE_2; >> + } >> >> cached_entry = smmu_translate(bs, cfg, addr, flag, &ptw_info); >> + >> + if (S2_only) { >> + cfg->asid = asid; >> + cfg->stage = stage; >> + } >> + >> if (!cached_entry) { >> /* All faults from PTW has S2 field. */ >> event->u.f_walk_eabt.s2 = (ptw_info.stage == SMMU_STAGE_2); >> @@ -855,7 +923,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, >> event->type = SMMU_EVT_F_TRANSLATION; >> event->u.f_translation.addr = addr; >> event->u.f_translation.addr2 = ptw_info.addr; >> - event->u.f_translation.class = SMMU_CLASS_IN; >> + event->u.f_translation.class = class; >> event->u.f_translation.rnw = flag & 0x1; >> } >> break; >> @@ -864,7 +932,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, >> event->type = SMMU_EVT_F_ADDR_SIZE; >> event->u.f_addr_size.addr = addr; >> event->u.f_addr_size.addr2 = ptw_info.addr; >> - event->u.f_addr_size.class = SMMU_CLASS_IN; >> + event->u.f_addr_size.class = class; >> event->u.f_addr_size.rnw = flag & 0x1; >> } >> break; >> @@ -873,7 +941,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, >> event->type = SMMU_EVT_F_ACCESS; >> event->u.f_access.addr = addr; >> event->u.f_access.addr2 = ptw_info.addr; >> - event->u.f_access.class = SMMU_CLASS_IN; >> + event->u.f_access.class = class; >> event->u.f_access.rnw = flag & 0x1; >> } >> break; >> @@ -882,7 +950,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, >> event->type = SMMU_EVT_F_PERMISSION; >> event->u.f_permission.addr = addr; >> event->u.f_permission.addr2 = ptw_info.addr; >> - event->u.f_permission.class = SMMU_CLASS_IN; >> + event->u.f_permission.class = class; >> event->u.f_permission.rnw = flag & 0x1; >> } >> break; >> @@ -943,7 +1011,8 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr, >> goto epilogue; >> } >> >> - status = smmuv3_do_translate(s, addr, cfg, &event, flag, &cached_entry); >> + status = smmuv3_do_translate(s, addr, cfg, &event, flag, >> + &cached_entry, SMMU_CLASS_IN); >> >> epilogue: >> qemu_mutex_unlock(&s->mutex); >> -- >> 2.45.2.803.g4e1b14247a-goog >>
Hi Jean, On Thu, Jul 04, 2024 at 07:08:43PM +0100, Jean-Philippe Brucker wrote: > On Mon, Jul 01, 2024 at 11:02:30AM +0000, Mostafa Saleh wrote: > > According to ARM SMMU architecture specification (ARM IHI 0070 F.b), > > In "5.2 Stream Table Entry": > > [51:6] S1ContextPtr > > If Config[1] == 1 (stage 2 enabled), this pointer is an IPA translated by > > stage 2 and the programmed value must be within the range of the IAS. > > > > In "5.4.1 CD notes": > > The translation table walks performed from TTB0 or TTB1 are always performed > > in IPA space if stage 2 translations are enabled. > > > > This patch implements translation of the S1 context descriptor pointer and > > TTBx base addresses through the S2 stage (IPA -> PA) > > > > smmuv3_do_translate() is updated to have one arg which is translation > > class, this is useful to: > > - Decide wether a translation is stage-2 only or use the STE config. > > - Populate the class in case of faults, WALK_EABT is left unchanged, > > as it is always triggered from TT access so no need to use the > > input class. > > > > In case for stage-2 only translation, used in the context of nested > > translation, the stage and asid are saved and restored before and > > after calling smmu_translate(). > > > > Translating CD or TTBx can fail for the following reasons: > > 1) Large address size: This is described in > > (3.4.3 Address sizes of SMMU-originated accesses) > > - For CD ptr larger than IAS, for SMMUv3.1, it can trigger either > > C_BAD_STE or Translation fault, we implement the latter as it > > requires no extra code. > > - For TTBx, if larger than the effective stage 1 output address size, it > > triggers C_BAD_CD. > > > > 2) Faults from PTWs (7.3 Event records) > > - F_ADDR_SIZE: large address size after first level causes stage 2 Address > > Size fault (Also in 3.4.3 Address sizes of SMMU-originated accesses) > > - F_PERMISSION: Same as an address translation. However, when > > CLASS == CD, the access is implicitly Data and a read. > > - F_ACCESS: Same as an address translation. > > - F_TRANSLATION: Same as an address translation. > > - F_WALK_EABT: Same as an address translation. > > These are already implemented in the PTW logic, so no extra handling > > required. > > > > Reviewed-by: Eric Auger <eric.auger@redhat.com> > > Signed-off-by: Mostafa Saleh <smostafa@google.com> > > --- > > hw/arm/smmuv3.c | 91 +++++++++++++++++++++++++++++++++++++++++++------ > > 1 file changed, 80 insertions(+), 11 deletions(-) > > > > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > > index 229b3c388c..86f95c1e40 100644 > > --- a/hw/arm/smmuv3.c > > +++ b/hw/arm/smmuv3.c > > @@ -337,14 +337,35 @@ static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf, > > > > } > > > > +static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, > > + SMMUTransCfg *cfg, > > + SMMUEventInfo *event, > > + IOMMUAccessFlags flag, > > + SMMUTLBEntry **out_entry, > > + SMMUTranslationClass class); > > /* @ssid > 0 not supported yet */ > > -static int smmu_get_cd(SMMUv3State *s, STE *ste, uint32_t ssid, > > - CD *buf, SMMUEventInfo *event) > > +static int smmu_get_cd(SMMUv3State *s, STE *ste, SMMUTransCfg *cfg, > > + uint32_t ssid, CD *buf, SMMUEventInfo *event) > > { > > dma_addr_t addr = STE_CTXPTR(ste); > > int ret, i; > > + SMMUTranslationStatus status; > > + SMMUTLBEntry *entry; > > > > trace_smmuv3_get_cd(addr); > > + > > + if (cfg->stage == SMMU_NESTED) { > > + status = smmuv3_do_translate(s, addr, cfg, event, > > + IOMMU_RO, &entry, SMMU_CLASS_CD); > > + > > + /* Same PTW faults are reported but with CLASS = CD. */ > > + if (status != SMMU_TRANS_SUCCESS) { > > In this case I think we're reporting InputAddr as the CD address, but it > should be the IOVA As Eric mentioned this would require some rework to propagate the iova, but what I am more worried about is the readability in that case, may be we can just fixup the event after smmuv3_get_config() in case of errors, something like: /* * smmuv3_get_config() Only return translation faults in case of * nested translation, otherwise it can only return C_BAD_CD, * C_BAD_STE, C_BAD_STREAMID or F_STE_FETCH. * But in case of translation fault, we need to fixup the * InputAddr to be the IOVA of the translation as the decode * functions don't know about it. */ static void smmuv3_config_fixup_event(SMMUEventInfo *event, hwaddr iova) { switch (event->type) { case SMMU_EVT_F_WALK_EABT: case SMMU_EVT_F_TRANSLATION: case SMMU_EVT_F_ADDR_SIZE: case SMMU_EVT_F_ACCESS: case SMMU_EVT_F_PERMISSION: event->u.f_walk_eabt.addr = iova; break; default: break; } } What do you think? Thanks, Mostafa > > > + return -EINVAL; > > + } > > + > > + addr = CACHED_ENTRY_TO_ADDR(entry, addr); > > + } > > + > > /* TODO: guarantee 64-bit single-copy atomicity */ > > ret = dma_memory_read(&address_space_memory, addr, buf, sizeof(*buf), > > MEMTXATTRS_UNSPECIFIED); > > @@ -659,10 +680,13 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste, > > return 0; > > } > > > > -static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event) > > +static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg, > > + CD *cd, SMMUEventInfo *event) > > { > > int ret = -EINVAL; > > int i; > > + SMMUTranslationStatus status; > > + SMMUTLBEntry *entry; > > > > if (!CD_VALID(cd) || !CD_AARCH64(cd)) { > > goto bad_cd; > > @@ -713,9 +737,26 @@ static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event) > > > > tt->tsz = tsz; > > tt->ttb = CD_TTB(cd, i); > > + > > if (tt->ttb & ~(MAKE_64BIT_MASK(0, cfg->oas))) { > > goto bad_cd; > > } > > + > > + /* Translate the TTBx, from IPA to PA if nesting is enabled. */ > > + if (cfg->stage == SMMU_NESTED) { > > + status = smmuv3_do_translate(s, tt->ttb, cfg, event, IOMMU_RO, > > + &entry, SMMU_CLASS_TT); > > + /* > > + * Same PTW faults are reported but with CLASS = TT. > > + * If TTBx is larger than the effective stage 1 output addres > > + * size, it reports C_BAD_CD, which is handled by the above case. > > + */ > > + if (status != SMMU_TRANS_SUCCESS) { > > Here too, we should report InputAddr as the IOVA > > > + return -EINVAL; > > + } > > + tt->ttb = CACHED_ENTRY_TO_ADDR(entry, tt->ttb); > > + } > > + > > tt->had = CD_HAD(cd, i); > > trace_smmuv3_decode_cd_tt(i, tt->tsz, tt->ttb, tt->granule_sz, tt->had); > > } > > @@ -767,12 +808,12 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg, > > return 0; > > } > > > > - ret = smmu_get_cd(s, &ste, 0 /* ssid */, &cd, event); > > + ret = smmu_get_cd(s, &ste, cfg, 0 /* ssid */, &cd, event); > > if (ret) { > > return ret; > > } > > > > - return decode_cd(cfg, &cd, event); > > + return decode_cd(s, cfg, &cd, event); > > } > > > > /** > > @@ -832,13 +873,40 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, > > SMMUTransCfg *cfg, > > SMMUEventInfo *event, > > IOMMUAccessFlags flag, > > - SMMUTLBEntry **out_entry) > > + SMMUTLBEntry **out_entry, > > + SMMUTranslationClass class) > > { > > SMMUPTWEventInfo ptw_info = {}; > > SMMUState *bs = ARM_SMMU(s); > > SMMUTLBEntry *cached_entry = NULL; > > + int asid, stage; > > + bool S2_only = class != SMMU_CLASS_IN; > > + > > + /* > > + * The function uses the argument class to indentify which stage is used: > > identify > > > + * - CLASS = IN: Means an input translation, determine the stage from STE. > > + * - CLASS = CD: Means the addr is an IPA of the CD, and it would be > > + * tranlsated using the stage-2. > > translated > > > + * - CLASS = TT: Means the addr is an IPA of the stage-1 translation table > > + * and it would be tranlsated using the stage-2. > > translated > > > + * For the last 2 cases instead of having intrusive changes in the common > > + * logic, we modify the cfg to be a stage-2 translation only in case of > > + * nested, and then restore it after. > > + */ > > + if (S2_only) { > > "S2_only" seems a bit confusing because in the spec "stage 2-only" means > absence of nesting, which is the opposite of what we're handling here. > I don't have a good alternative though, maybe "desc_s2_translation" > > Thanks, > Jean > > > + asid = cfg->asid; > > + stage = cfg->stage; > > + cfg->asid = -1; > > + cfg->stage = SMMU_STAGE_2; > > + } > > > > cached_entry = smmu_translate(bs, cfg, addr, flag, &ptw_info); > > + > > + if (S2_only) { > > + cfg->asid = asid; > > + cfg->stage = stage; > > + } > > + > > if (!cached_entry) { > > /* All faults from PTW has S2 field. */ > > event->u.f_walk_eabt.s2 = (ptw_info.stage == SMMU_STAGE_2); > > @@ -855,7 +923,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, > > event->type = SMMU_EVT_F_TRANSLATION; > > event->u.f_translation.addr = addr; > > event->u.f_translation.addr2 = ptw_info.addr; > > - event->u.f_translation.class = SMMU_CLASS_IN; > > + event->u.f_translation.class = class; > > event->u.f_translation.rnw = flag & 0x1; > > } > > break; > > @@ -864,7 +932,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, > > event->type = SMMU_EVT_F_ADDR_SIZE; > > event->u.f_addr_size.addr = addr; > > event->u.f_addr_size.addr2 = ptw_info.addr; > > - event->u.f_addr_size.class = SMMU_CLASS_IN; > > + event->u.f_addr_size.class = class; > > event->u.f_addr_size.rnw = flag & 0x1; > > } > > break; > > @@ -873,7 +941,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, > > event->type = SMMU_EVT_F_ACCESS; > > event->u.f_access.addr = addr; > > event->u.f_access.addr2 = ptw_info.addr; > > - event->u.f_access.class = SMMU_CLASS_IN; > > + event->u.f_access.class = class; > > event->u.f_access.rnw = flag & 0x1; > > } > > break; > > @@ -882,7 +950,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, > > event->type = SMMU_EVT_F_PERMISSION; > > event->u.f_permission.addr = addr; > > event->u.f_permission.addr2 = ptw_info.addr; > > - event->u.f_permission.class = SMMU_CLASS_IN; > > + event->u.f_permission.class = class; > > event->u.f_permission.rnw = flag & 0x1; > > } > > break; > > @@ -943,7 +1011,8 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr, > > goto epilogue; > > } > > > > - status = smmuv3_do_translate(s, addr, cfg, &event, flag, &cached_entry); > > + status = smmuv3_do_translate(s, addr, cfg, &event, flag, > > + &cached_entry, SMMU_CLASS_IN); > > > > epilogue: > > qemu_mutex_unlock(&s->mutex); > > -- > > 2.45.2.803.g4e1b14247a-goog > >
Hi Mostafa, On Tue, Jul 09, 2024 at 07:12:59AM +0000, Mostafa Saleh wrote: > > In this case I think we're reporting InputAddr as the CD address, but it > > should be the IOVA > > As Eric mentioned this would require some rework to propagate the iova, > but what I am more worried about is the readability in that case, may be we > can just fixup the event after smmuv3_get_config() in case of errors, > something like: > > /* > * smmuv3_get_config() Only return translation faults in case of > * nested translation, otherwise it can only return C_BAD_CD, > * C_BAD_STE, C_BAD_STREAMID or F_STE_FETCH. > * But in case of translation fault, we need to fixup the > * InputAddr to be the IOVA of the translation as the decode > * functions don't know about it. > */ > static void smmuv3_config_fixup_event(SMMUEventInfo *event, hwaddr iova) > { > switch (event->type) { > case SMMU_EVT_F_WALK_EABT: > case SMMU_EVT_F_TRANSLATION: > case SMMU_EVT_F_ADDR_SIZE: > case SMMU_EVT_F_ACCESS: > case SMMU_EVT_F_PERMISSION: > event->u.f_walk_eabt.addr = iova; > break; > default: > break; > } > } > > What do you think? Yes, I think that's also what I came up with. Maybe it would be simpler to unconditionally do the fixup at the end of smmuv3_translate() and remove .addr write from smmuv3_do_translate()? A separate union field "f_common" rather than f_walk_eabt may be clearer. Thanks, Jean
Hi Jean, On Tue, Jul 09, 2024 at 02:00:04PM +0100, Jean-Philippe Brucker wrote: > Hi Mostafa, > > On Tue, Jul 09, 2024 at 07:12:59AM +0000, Mostafa Saleh wrote: > > > In this case I think we're reporting InputAddr as the CD address, but it > > > should be the IOVA > > > > As Eric mentioned this would require some rework to propagate the iova, > > but what I am more worried about is the readability in that case, may be we > > can just fixup the event after smmuv3_get_config() in case of errors, > > something like: > > > > /* > > * smmuv3_get_config() Only return translation faults in case of > > * nested translation, otherwise it can only return C_BAD_CD, > > * C_BAD_STE, C_BAD_STREAMID or F_STE_FETCH. > > * But in case of translation fault, we need to fixup the > > * InputAddr to be the IOVA of the translation as the decode > > * functions don't know about it. > > */ > > static void smmuv3_config_fixup_event(SMMUEventInfo *event, hwaddr iova) > > { > > switch (event->type) { > > case SMMU_EVT_F_WALK_EABT: > > case SMMU_EVT_F_TRANSLATION: > > case SMMU_EVT_F_ADDR_SIZE: > > case SMMU_EVT_F_ACCESS: > > case SMMU_EVT_F_PERMISSION: > > event->u.f_walk_eabt.addr = iova; > > break; > > default: > > break; > > } > > } > > > > What do you think? > > Yes, I think that's also what I came up with. Maybe it would be simpler to > unconditionally do the fixup at the end of smmuv3_translate() and remove > .addr write from smmuv3_do_translate()? I wanted to make it clear what case causes the IOVA to be missing, but I guess if we unify the setup for the InputAddr it would be easier to read and just add a comment instead. > > A separate union field "f_common" rather than f_walk_eabt may be clearer. > Makes sense, but I will not do it in this patch to avoid making it larger and harder to review, and this can be a separate cleanup as I see in other places we use eabt already (smmuv3_record_event and for s2 population). Thanks, Mostafa > Thanks, > Jean
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c index 229b3c388c..86f95c1e40 100644 --- a/hw/arm/smmuv3.c +++ b/hw/arm/smmuv3.c @@ -337,14 +337,35 @@ static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf, } +static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, + SMMUTransCfg *cfg, + SMMUEventInfo *event, + IOMMUAccessFlags flag, + SMMUTLBEntry **out_entry, + SMMUTranslationClass class); /* @ssid > 0 not supported yet */ -static int smmu_get_cd(SMMUv3State *s, STE *ste, uint32_t ssid, - CD *buf, SMMUEventInfo *event) +static int smmu_get_cd(SMMUv3State *s, STE *ste, SMMUTransCfg *cfg, + uint32_t ssid, CD *buf, SMMUEventInfo *event) { dma_addr_t addr = STE_CTXPTR(ste); int ret, i; + SMMUTranslationStatus status; + SMMUTLBEntry *entry; trace_smmuv3_get_cd(addr); + + if (cfg->stage == SMMU_NESTED) { + status = smmuv3_do_translate(s, addr, cfg, event, + IOMMU_RO, &entry, SMMU_CLASS_CD); + + /* Same PTW faults are reported but with CLASS = CD. */ + if (status != SMMU_TRANS_SUCCESS) { + return -EINVAL; + } + + addr = CACHED_ENTRY_TO_ADDR(entry, addr); + } + /* TODO: guarantee 64-bit single-copy atomicity */ ret = dma_memory_read(&address_space_memory, addr, buf, sizeof(*buf), MEMTXATTRS_UNSPECIFIED); @@ -659,10 +680,13 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste, return 0; } -static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event) +static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg, + CD *cd, SMMUEventInfo *event) { int ret = -EINVAL; int i; + SMMUTranslationStatus status; + SMMUTLBEntry *entry; if (!CD_VALID(cd) || !CD_AARCH64(cd)) { goto bad_cd; @@ -713,9 +737,26 @@ static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event) tt->tsz = tsz; tt->ttb = CD_TTB(cd, i); + if (tt->ttb & ~(MAKE_64BIT_MASK(0, cfg->oas))) { goto bad_cd; } + + /* Translate the TTBx, from IPA to PA if nesting is enabled. */ + if (cfg->stage == SMMU_NESTED) { + status = smmuv3_do_translate(s, tt->ttb, cfg, event, IOMMU_RO, + &entry, SMMU_CLASS_TT); + /* + * Same PTW faults are reported but with CLASS = TT. + * If TTBx is larger than the effective stage 1 output addres + * size, it reports C_BAD_CD, which is handled by the above case. + */ + if (status != SMMU_TRANS_SUCCESS) { + return -EINVAL; + } + tt->ttb = CACHED_ENTRY_TO_ADDR(entry, tt->ttb); + } + tt->had = CD_HAD(cd, i); trace_smmuv3_decode_cd_tt(i, tt->tsz, tt->ttb, tt->granule_sz, tt->had); } @@ -767,12 +808,12 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg, return 0; } - ret = smmu_get_cd(s, &ste, 0 /* ssid */, &cd, event); + ret = smmu_get_cd(s, &ste, cfg, 0 /* ssid */, &cd, event); if (ret) { return ret; } - return decode_cd(cfg, &cd, event); + return decode_cd(s, cfg, &cd, event); } /** @@ -832,13 +873,40 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, SMMUTransCfg *cfg, SMMUEventInfo *event, IOMMUAccessFlags flag, - SMMUTLBEntry **out_entry) + SMMUTLBEntry **out_entry, + SMMUTranslationClass class) { SMMUPTWEventInfo ptw_info = {}; SMMUState *bs = ARM_SMMU(s); SMMUTLBEntry *cached_entry = NULL; + int asid, stage; + bool S2_only = class != SMMU_CLASS_IN; + + /* + * The function uses the argument class to indentify which stage is used: + * - CLASS = IN: Means an input translation, determine the stage from STE. + * - CLASS = CD: Means the addr is an IPA of the CD, and it would be + * tranlsated using the stage-2. + * - CLASS = TT: Means the addr is an IPA of the stage-1 translation table + * and it would be tranlsated using the stage-2. + * For the last 2 cases instead of having intrusive changes in the common + * logic, we modify the cfg to be a stage-2 translation only in case of + * nested, and then restore it after. + */ + if (S2_only) { + asid = cfg->asid; + stage = cfg->stage; + cfg->asid = -1; + cfg->stage = SMMU_STAGE_2; + } cached_entry = smmu_translate(bs, cfg, addr, flag, &ptw_info); + + if (S2_only) { + cfg->asid = asid; + cfg->stage = stage; + } + if (!cached_entry) { /* All faults from PTW has S2 field. */ event->u.f_walk_eabt.s2 = (ptw_info.stage == SMMU_STAGE_2); @@ -855,7 +923,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, event->type = SMMU_EVT_F_TRANSLATION; event->u.f_translation.addr = addr; event->u.f_translation.addr2 = ptw_info.addr; - event->u.f_translation.class = SMMU_CLASS_IN; + event->u.f_translation.class = class; event->u.f_translation.rnw = flag & 0x1; } break; @@ -864,7 +932,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, event->type = SMMU_EVT_F_ADDR_SIZE; event->u.f_addr_size.addr = addr; event->u.f_addr_size.addr2 = ptw_info.addr; - event->u.f_addr_size.class = SMMU_CLASS_IN; + event->u.f_addr_size.class = class; event->u.f_addr_size.rnw = flag & 0x1; } break; @@ -873,7 +941,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, event->type = SMMU_EVT_F_ACCESS; event->u.f_access.addr = addr; event->u.f_access.addr2 = ptw_info.addr; - event->u.f_access.class = SMMU_CLASS_IN; + event->u.f_access.class = class; event->u.f_access.rnw = flag & 0x1; } break; @@ -882,7 +950,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr, event->type = SMMU_EVT_F_PERMISSION; event->u.f_permission.addr = addr; event->u.f_permission.addr2 = ptw_info.addr; - event->u.f_permission.class = SMMU_CLASS_IN; + event->u.f_permission.class = class; event->u.f_permission.rnw = flag & 0x1; } break; @@ -943,7 +1011,8 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr, goto epilogue; } - status = smmuv3_do_translate(s, addr, cfg, &event, flag, &cached_entry); + status = smmuv3_do_translate(s, addr, cfg, &event, flag, + &cached_entry, SMMU_CLASS_IN); epilogue: qemu_mutex_unlock(&s->mutex);
According to ARM SMMU architecture specification (ARM IHI 0070 F.b), In "5.2 Stream Table Entry": [51:6] S1ContextPtr If Config[1] == 1 (stage 2 enabled), this pointer is an IPA translated by stage 2 and the programmed value must be within the range of the IAS. In "5.4.1 CD notes": The translation table walks performed from TTB0 or TTB1 are always performed in IPA space if stage 2 translations are enabled. This patch implements translation of the S1 context descriptor pointer and TTBx base addresses through the S2 stage (IPA -> PA) smmuv3_do_translate() is updated to have one arg which is translation class, this is useful to: - Decide wether a translation is stage-2 only or use the STE config. - Populate the class in case of faults, WALK_EABT is left unchanged, as it is always triggered from TT access so no need to use the input class. In case for stage-2 only translation, used in the context of nested translation, the stage and asid are saved and restored before and after calling smmu_translate(). Translating CD or TTBx can fail for the following reasons: 1) Large address size: This is described in (3.4.3 Address sizes of SMMU-originated accesses) - For CD ptr larger than IAS, for SMMUv3.1, it can trigger either C_BAD_STE or Translation fault, we implement the latter as it requires no extra code. - For TTBx, if larger than the effective stage 1 output address size, it triggers C_BAD_CD. 2) Faults from PTWs (7.3 Event records) - F_ADDR_SIZE: large address size after first level causes stage 2 Address Size fault (Also in 3.4.3 Address sizes of SMMU-originated accesses) - F_PERMISSION: Same as an address translation. However, when CLASS == CD, the access is implicitly Data and a read. - F_ACCESS: Same as an address translation. - F_TRANSLATION: Same as an address translation. - F_WALK_EABT: Same as an address translation. These are already implemented in the PTW logic, so no extra handling required. Reviewed-by: Eric Auger <eric.auger@redhat.com> Signed-off-by: Mostafa Saleh <smostafa@google.com> --- hw/arm/smmuv3.c | 91 +++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 80 insertions(+), 11 deletions(-)