| Message ID | 20240715084519.1189624-4-smostafa@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SMMUv3 nested translation support | expand |
Hi Jean, On 7/15/24 10:45, Mostafa Saleh wrote: > The SMMUv3 spec (ARM IHI 0070 F.b - 7.3 Event records) defines the > class of events faults as: > > CLASS: The class of the operation that caused the fault: > - 0b00: CD, CD fetch. > - 0b01: TTD, Stage 1 translation table fetch. > - 0b10: IN, Input address > > However, this value was not set and left as 0 which means CD and not > IN (0b10). > > Another problem was that stage-2 class is considered IN not TT for > EABT, according to the spec: > Translation of an IPA after successful stage 1 translation (or, > in stage 2-only configuration, an input IPA) > - S2 == 1 (stage 2), CLASS == IN (Input to stage) > > This would change soon when nested translations are supported. > > While at it, add an enum for class as it would be used for nesting. > However, at the moment stage-1 and stage-2 use the same class values, > except for EABT. > > Fixes: 9bde7f0674 “hw/arm/smmuv3: Implement translate callback” > Signed-off-by: Mostafa Saleh <smostafa@google.com> > --- > hw/arm/smmuv3-internal.h | 6 ++++++ > hw/arm/smmuv3.c | 8 +++++++- > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h > index e4dd11e1e6..0f3ecec804 100644 > --- a/hw/arm/smmuv3-internal.h > +++ b/hw/arm/smmuv3-internal.h > @@ -32,6 +32,12 @@ typedef enum SMMUTranslationStatus { > SMMU_TRANS_SUCCESS, > } SMMUTranslationStatus; > > +typedef enum SMMUTranslationClass { > + SMMU_CLASS_CD, > + SMMU_CLASS_TT, > + SMMU_CLASS_IN, > +} SMMUTranslationClass; > + > /* MMIO Registers */ > > REG32(IDR0, 0x0) > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > index 9dd3ea48e4..3d214c9f57 100644 > --- a/hw/arm/smmuv3.c > +++ b/hw/arm/smmuv3.c > @@ -942,7 +942,9 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr, > event.type = SMMU_EVT_F_WALK_EABT; > event.u.f_walk_eabt.addr = addr; > event.u.f_walk_eabt.rnw = flag & 0x1; > - event.u.f_walk_eabt.class = 0x1; > + /* Stage-2 (only) is class IN while stage-1 is class TT */ > + event.u.f_walk_eabt.class = (ptw_info.stage == 2) ? > + SMMU_CLASS_IN : SMMU_CLASS_TT; does it match your expectations. While reading your previous comment I have the impression what you had in mind was more complicated than that * s2 walk that encounters EABT on S2 descriptor while translating non-descriptor IPA is reported as class=IN, even when doing s2-only. Thanks Eric > event.u.f_walk_eabt.addr2 = ptw_info.addr; > break; > case SMMU_PTW_ERR_TRANSLATION: > @@ -950,6 +952,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, 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.rnw = flag & 0x1; > } > break; > @@ -958,6 +961,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, 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_translation.class = SMMU_CLASS_IN; > event.u.f_addr_size.rnw = flag & 0x1; > } > break; > @@ -966,6 +970,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, 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_translation.class = SMMU_CLASS_IN; > event.u.f_access.rnw = flag & 0x1; > } > break; > @@ -974,6 +979,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, 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_translation.class = SMMU_CLASS_IN; > event.u.f_permission.rnw = flag & 0x1; > } > break;
Hi Eric, On Wed, Jul 17, 2024 at 05:07:57PM +0200, Eric Auger wrote: > Hi Jean, > > On 7/15/24 10:45, Mostafa Saleh wrote: > > The SMMUv3 spec (ARM IHI 0070 F.b - 7.3 Event records) defines the > > class of events faults as: > > > > CLASS: The class of the operation that caused the fault: > > - 0b00: CD, CD fetch. > > - 0b01: TTD, Stage 1 translation table fetch. > > - 0b10: IN, Input address > > > > However, this value was not set and left as 0 which means CD and not > > IN (0b10). > > > > Another problem was that stage-2 class is considered IN not TT for > > EABT, according to the spec: > > Translation of an IPA after successful stage 1 translation (or, > > in stage 2-only configuration, an input IPA) > > - S2 == 1 (stage 2), CLASS == IN (Input to stage) > > > > This would change soon when nested translations are supported. > > > > While at it, add an enum for class as it would be used for nesting. > > However, at the moment stage-1 and stage-2 use the same class values, > > except for EABT. > > > > Fixes: 9bde7f0674 “hw/arm/smmuv3: Implement translate callback” > > Signed-off-by: Mostafa Saleh <smostafa@google.com> > > --- > > hw/arm/smmuv3-internal.h | 6 ++++++ > > hw/arm/smmuv3.c | 8 +++++++- > > 2 files changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h > > index e4dd11e1e6..0f3ecec804 100644 > > --- a/hw/arm/smmuv3-internal.h > > +++ b/hw/arm/smmuv3-internal.h > > @@ -32,6 +32,12 @@ typedef enum SMMUTranslationStatus { > > SMMU_TRANS_SUCCESS, > > } SMMUTranslationStatus; > > > > +typedef enum SMMUTranslationClass { > > + SMMU_CLASS_CD, > > + SMMU_CLASS_TT, > > + SMMU_CLASS_IN, > > +} SMMUTranslationClass; > > + > > /* MMIO Registers */ > > > > REG32(IDR0, 0x0) > > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > > index 9dd3ea48e4..3d214c9f57 100644 > > --- a/hw/arm/smmuv3.c > > +++ b/hw/arm/smmuv3.c > > @@ -942,7 +942,9 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr, > > event.type = SMMU_EVT_F_WALK_EABT; > > event.u.f_walk_eabt.addr = addr; > > event.u.f_walk_eabt.rnw = flag & 0x1; > > - event.u.f_walk_eabt.class = 0x1; > > + /* Stage-2 (only) is class IN while stage-1 is class TT */ > > + event.u.f_walk_eabt.class = (ptw_info.stage == 2) ? > > + SMMU_CLASS_IN : SMMU_CLASS_TT; > does it match your expectations. While reading your previous comment I > have the impression what you had in mind was more complicated than that > > * s2 walk that encounters EABT on S2 descriptor while translating > non-descriptor IPA is reported as class=IN, even when doing s2-only. At this point we only support single-stage, so I believe this is correct: "A stage 1-only table walk that encounters EABT ... CLASS == TT" "translation of ... in stage 2-only configuration, an input IPA... CLASS == IN" Later in the series this code changes in order to support nesting, but I think it's still correct, because the EABT class works similarly to translation errors, except for stage-1 faults which have CLASS==TT Thanks, Jean > > Thanks > > Eric > > > event.u.f_walk_eabt.addr2 = ptw_info.addr; > > break; > > case SMMU_PTW_ERR_TRANSLATION: > > @@ -950,6 +952,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, 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.rnw = flag & 0x1; > > } > > break; > > @@ -958,6 +961,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, 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_translation.class = SMMU_CLASS_IN; > > event.u.f_addr_size.rnw = flag & 0x1; > > } > > break; > > @@ -966,6 +970,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, 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_translation.class = SMMU_CLASS_IN; > > event.u.f_access.rnw = flag & 0x1; > > } > > break; > > @@ -974,6 +979,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, 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_translation.class = SMMU_CLASS_IN; > > event.u.f_permission.rnw = flag & 0x1; > > } > > break; >
On 7/17/24 17:58, Jean-Philippe Brucker wrote: > Hi Eric, > > On Wed, Jul 17, 2024 at 05:07:57PM +0200, Eric Auger wrote: >> Hi Jean, >> >> On 7/15/24 10:45, Mostafa Saleh wrote: >>> The SMMUv3 spec (ARM IHI 0070 F.b - 7.3 Event records) defines the >>> class of events faults as: >>> >>> CLASS: The class of the operation that caused the fault: >>> - 0b00: CD, CD fetch. >>> - 0b01: TTD, Stage 1 translation table fetch. >>> - 0b10: IN, Input address >>> >>> However, this value was not set and left as 0 which means CD and not >>> IN (0b10). >>> >>> Another problem was that stage-2 class is considered IN not TT for >>> EABT, according to the spec: >>> Translation of an IPA after successful stage 1 translation (or, >>> in stage 2-only configuration, an input IPA) >>> - S2 == 1 (stage 2), CLASS == IN (Input to stage) >>> >>> This would change soon when nested translations are supported. >>> >>> While at it, add an enum for class as it would be used for nesting. >>> However, at the moment stage-1 and stage-2 use the same class values, >>> except for EABT. >>> >>> Fixes: 9bde7f0674 “hw/arm/smmuv3: Implement translate callback” >>> Signed-off-by: Mostafa Saleh <smostafa@google.com> >>> --- >>> hw/arm/smmuv3-internal.h | 6 ++++++ >>> hw/arm/smmuv3.c | 8 +++++++- >>> 2 files changed, 13 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h >>> index e4dd11e1e6..0f3ecec804 100644 >>> --- a/hw/arm/smmuv3-internal.h >>> +++ b/hw/arm/smmuv3-internal.h >>> @@ -32,6 +32,12 @@ typedef enum SMMUTranslationStatus { >>> SMMU_TRANS_SUCCESS, >>> } SMMUTranslationStatus; >>> >>> +typedef enum SMMUTranslationClass { >>> + SMMU_CLASS_CD, >>> + SMMU_CLASS_TT, >>> + SMMU_CLASS_IN, >>> +} SMMUTranslationClass; >>> + >>> /* MMIO Registers */ >>> >>> REG32(IDR0, 0x0) >>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c >>> index 9dd3ea48e4..3d214c9f57 100644 >>> --- a/hw/arm/smmuv3.c >>> +++ b/hw/arm/smmuv3.c >>> @@ -942,7 +942,9 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr, >>> event.type = SMMU_EVT_F_WALK_EABT; >>> event.u.f_walk_eabt.addr = addr; >>> event.u.f_walk_eabt.rnw = flag & 0x1; >>> - event.u.f_walk_eabt.class = 0x1; >>> + /* Stage-2 (only) is class IN while stage-1 is class TT */ >>> + event.u.f_walk_eabt.class = (ptw_info.stage == 2) ? >>> + SMMU_CLASS_IN : SMMU_CLASS_TT; >> does it match your expectations. While reading your previous comment I >> have the impression what you had in mind was more complicated than that >> >> * s2 walk that encounters EABT on S2 descriptor while translating >> non-descriptor IPA is reported as class=IN, even when doing s2-only. > At this point we only support single-stage, so I believe this is correct: > "A stage 1-only table walk that encounters EABT ... CLASS == TT" > "translation of ... in stage 2-only configuration, an input IPA... > CLASS == IN" > > Later in the series this code changes in order to support nesting, but I > think it's still correct, because the EABT class works similarly to > translation errors, except for stage-1 faults which have CLASS==TT OK thank you for the confirmation! Eric > > Thanks, > Jean > >> Thanks >> >> Eric >> >>> event.u.f_walk_eabt.addr2 = ptw_info.addr; >>> break; >>> case SMMU_PTW_ERR_TRANSLATION: >>> @@ -950,6 +952,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, 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.rnw = flag & 0x1; >>> } >>> break; >>> @@ -958,6 +961,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, 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_translation.class = SMMU_CLASS_IN; >>> event.u.f_addr_size.rnw = flag & 0x1; >>> } >>> break; >>> @@ -966,6 +970,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, 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_translation.class = SMMU_CLASS_IN; >>> event.u.f_access.rnw = flag & 0x1; >>> } >>> break; >>> @@ -974,6 +979,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, 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_translation.class = SMMU_CLASS_IN; >>> event.u.f_permission.rnw = flag & 0x1; >>> } >>> break;
diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h index e4dd11e1e6..0f3ecec804 100644 --- a/hw/arm/smmuv3-internal.h +++ b/hw/arm/smmuv3-internal.h @@ -32,6 +32,12 @@ typedef enum SMMUTranslationStatus { SMMU_TRANS_SUCCESS, } SMMUTranslationStatus; +typedef enum SMMUTranslationClass { + SMMU_CLASS_CD, + SMMU_CLASS_TT, + SMMU_CLASS_IN, +} SMMUTranslationClass; + /* MMIO Registers */ REG32(IDR0, 0x0) diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c index 9dd3ea48e4..3d214c9f57 100644 --- a/hw/arm/smmuv3.c +++ b/hw/arm/smmuv3.c @@ -942,7 +942,9 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr, event.type = SMMU_EVT_F_WALK_EABT; event.u.f_walk_eabt.addr = addr; event.u.f_walk_eabt.rnw = flag & 0x1; - event.u.f_walk_eabt.class = 0x1; + /* Stage-2 (only) is class IN while stage-1 is class TT */ + event.u.f_walk_eabt.class = (ptw_info.stage == 2) ? + SMMU_CLASS_IN : SMMU_CLASS_TT; event.u.f_walk_eabt.addr2 = ptw_info.addr; break; case SMMU_PTW_ERR_TRANSLATION: @@ -950,6 +952,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, 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.rnw = flag & 0x1; } break; @@ -958,6 +961,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, 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_translation.class = SMMU_CLASS_IN; event.u.f_addr_size.rnw = flag & 0x1; } break; @@ -966,6 +970,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, 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_translation.class = SMMU_CLASS_IN; event.u.f_access.rnw = flag & 0x1; } break; @@ -974,6 +979,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, 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_translation.class = SMMU_CLASS_IN; event.u.f_permission.rnw = flag & 0x1; } break;
The SMMUv3 spec (ARM IHI 0070 F.b - 7.3 Event records) defines the class of events faults as: CLASS: The class of the operation that caused the fault: - 0b00: CD, CD fetch. - 0b01: TTD, Stage 1 translation table fetch. - 0b10: IN, Input address However, this value was not set and left as 0 which means CD and not IN (0b10). Another problem was that stage-2 class is considered IN not TT for EABT, according to the spec: Translation of an IPA after successful stage 1 translation (or, in stage 2-only configuration, an input IPA) - S2 == 1 (stage 2), CLASS == IN (Input to stage) This would change soon when nested translations are supported. While at it, add an enum for class as it would be used for nesting. However, at the moment stage-1 and stage-2 use the same class values, except for EABT. Fixes: 9bde7f0674 “hw/arm/smmuv3: Implement translate callback” Signed-off-by: Mostafa Saleh <smostafa@google.com> --- hw/arm/smmuv3-internal.h | 6 ++++++ hw/arm/smmuv3.c | 8 +++++++- 2 files changed, 13 insertions(+), 1 deletion(-)