1. Patchew
  2. QEMU
  3. [RFC PATCH v2 00/11] Add stage-2 translation for SMMUv3
  4. View patch

[RFC PATCH v2 02/11] hw/arm/smmuv3: Update translation config to hold stage-2

Mostafa Saleh posted 11 patches 2 years, 2 months ago
There is a newer version of this series
[RFC PATCH v2 02/11] hw/arm/smmuv3: Update translation config to hold stage-2
Posted by Mostafa Saleh 2 years, 2 months ago 
In preparation for adding stage-2 support, add a S2 config
struct(SMMUS2Cfg), composed of the following fields and embedded in
the main SMMUTransCfg:
 -tsz: Input range
 -sl0: start level of translation
 -affd: AF fault disable
 -granule_sz: Granule page shift
 -vmid: VMID
 -vttb: PA of translation table
 -oas: Output address size

They will be used in the next patches in stage-2 address translation.

No functional change intended.

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
Changes in v2:
-Add oas
---
 include/hw/arm/smmu-common.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 9fcff26357..2deead08d6 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -58,6 +58,16 @@ typedef struct SMMUTLBEntry {
     uint8_t granule;
 } SMMUTLBEntry;
 
+typedef struct SMMUS2Cfg {
+    uint8_t tsz;            /* Input range */
+    uint8_t sl0;            /* Start level of translation */
+    bool affd;              /* AF Fault Disable */
+    uint8_t granule_sz;     /* Granule page shift */
+    uint8_t oas;            /* Output address size */
+    uint16_t vmid;          /* Virtual machine ID */
+    uint64_t vttb;          /* PA of translation table */
+} SMMUS2Cfg;
+
 /*
  * Generic structure populated by derived SMMU devices
  * after decoding the configuration information and used as
@@ -77,6 +87,7 @@ typedef struct SMMUTransCfg {
     SMMUTransTableInfo tt[2];
     uint32_t iotlb_hits;       /* counts IOTLB hits for this asid */
     uint32_t iotlb_misses;     /* counts IOTLB misses for this asid */
+    struct SMMUS2Cfg s2cfg;
 } SMMUTransCfg;
 
 typedef struct SMMUDevice {
-- 
2.39.2.637.g21b0678d19-goog
Re: [RFC PATCH v2 02/11] hw/arm/smmuv3: Update translation config to hold stage-2
Posted by Eric Auger 2 years, 1 month ago 
Hi Mostafa,

On 2/26/23 23:06, Mostafa Saleh wrote:
> In preparation for adding stage-2 support, add a S2 config
> struct(SMMUS2Cfg), composed of the following fields and embedded in
> the main SMMUTransCfg:
>  -tsz: Input range
>  -sl0: start level of translation
>  -affd: AF fault disable
>  -granule_sz: Granule page shift
>  -vmid: VMID
>  -vttb: PA of translation table
>  -oas: Output address size
>
> They will be used in the next patches in stage-2 address translation.
>
> No functional change intended.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
> Changes in v2:
> -Add oas
> ---
>  include/hw/arm/smmu-common.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 9fcff26357..2deead08d6 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -58,6 +58,16 @@ typedef struct SMMUTLBEntry {
>      uint8_t granule;
>  } SMMUTLBEntry;
>  
> +typedef struct SMMUS2Cfg {
> +    uint8_t tsz;            /* Input range */
nit: Size of IPA input region. Called t0sz
> +    uint8_t sl0;            /* Start level of translation */
> +    bool affd;              /* AF Fault Disable */
> +    uint8_t granule_sz;     /* Granule page shift */
> +    uint8_t oas;            /* Output address size */
called s2ps. if you don't want to rename you may add this in the comment.

I am suprised to not see S2R which would match S1 record_faults.

Also without further reading we can wonder wheter the parent
iotlb_hits/misses also record S2 events?

I think we need to be/make clear what fields of the original S1 parent
struct also are relevant for the S2 only case.
Maybe tag them with a specific comment S1-only or S1 | S2. It may be
cleaner to introduce a union and common fields in the parent struct though.

Thanks

Eric
> +    uint16_t vmid;          /* Virtual machine ID */
> +    uint64_t vttb;          /* PA of translation table */
> +} SMMUS2Cfg;
> +
>  /*
>   * Generic structure populated by derived SMMU devices
>   * after decoding the configuration information and used as
> @@ -77,6 +87,7 @@ typedef struct SMMUTransCfg {
>      SMMUTransTableInfo tt[2];
>      uint32_t iotlb_hits;       /* counts IOTLB hits for this asid */
>      uint32_t iotlb_misses;     /* counts IOTLB misses for this asid */
> +    struct SMMUS2Cfg s2cfg;
>  } SMMUTransCfg;
>  
>  typedef struct SMMUDevice {
Re: [RFC PATCH v2 02/11] hw/arm/smmuv3: Update translation config to hold stage-2
Posted by Mostafa Saleh 2 years, 1 month ago 
Hi Eric,

Thanks for reviewing the patch.

On Fri, Mar 17, 2023 at 12:37:11PM +0100, Eric Auger wrote:
> Hi Mostafa,
> 
> On 2/26/23 23:06, Mostafa Saleh wrote:
> > In preparation for adding stage-2 support, add a S2 config
> > struct(SMMUS2Cfg), composed of the following fields and embedded in
> > the main SMMUTransCfg:
> >  -tsz: Input range
> >  -sl0: start level of translation
> >  -affd: AF fault disable
> >  -granule_sz: Granule page shift
> >  -vmid: VMID
> >  -vttb: PA of translation table
> >  -oas: Output address size
> >
> > They will be used in the next patches in stage-2 address translation.
> >
> > No functional change intended.
> >
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > ---
> > Changes in v2:
> > -Add oas
> > ---
> >  include/hw/arm/smmu-common.h | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> > index 9fcff26357..2deead08d6 100644
> > --- a/include/hw/arm/smmu-common.h
> > +++ b/include/hw/arm/smmu-common.h
> > @@ -58,6 +58,16 @@ typedef struct SMMUTLBEntry {
> >      uint8_t granule;
> >  } SMMUTLBEntry;
> >  
> > +typedef struct SMMUS2Cfg {
> > +    uint8_t tsz;            /* Input range */
> nit: Size of IPA input region. Called t0sz
I named it this way to be similar to stage-1, as SMMUTransTableInfo
just calls it tsz, I guess that is because it can hold either t0sz or
t1sz.
I can rename it for stage-2 to t0sz.

> > +    uint8_t sl0;            /* Start level of translation */
> > +    bool affd;              /* AF Fault Disable */
> > +    uint8_t granule_sz;     /* Granule page shift */
> > +    uint8_t oas;            /* Output address size */
> called s2ps. if you don't want to rename you may add this in the comment.
This is not the S2PS parsed from the STE, but the effective value which is
capped to SMMU_IDR5.OAS, which is used for checking output size, I can add
a clarifying comment.

> I am suprised to not see S2R which would match S1 record_faults.
I was thinking about this also, right now we piggy back on record_faults
in SMMUTransCfg.
But it makes more sense to separate this from the beginning as other
fields. I will update that by adding record_faults field in SMMUS2Cfg.

> Also without further reading we can wonder wheter the parent
> iotlb_hits/misses also record S2 events?
For iotlb_*, they are shared also. However, I think this is not important for now as
it is not architectural, and it produces the correct output as only one
stage is advertised at the moment.
When nesting is supported, TLB implementation might change and then we
can take a decision about this.

> I think we need to be/make clear what fields of the original S1 parent
> struct also are relevant for the S2 only case.
> Maybe tag them with a specific comment S1-only or S1 | S2. It may be
> cleaner to introduce a union and common fields in the parent struct though.

I agree, maybe we encapsulate stage-1 only fields in a similar struct
and leave common fields outside.

struct SMMUTransCfg:
	stage, bypassed, disabled, iotlb_* //common fields
	struct SMMUS2Cfg
	struct SMMUS1Cfg
		ttb,asid....

Or we can add SMMUS1Cfg and SMMUS2Cfg in a union for now and when nesting is supported we
separate them.


Thanks,
Mostafa
Re: [RFC PATCH v2 02/11] hw/arm/smmuv3: Update translation config to hold stage-2
Posted by Eric Auger 2 years, 1 month ago 
Hi Mostafa,

On 3/17/23 15:43, Mostafa Saleh wrote:
> Hi Eric,
>
> Thanks for reviewing the patch.
>
> On Fri, Mar 17, 2023 at 12:37:11PM +0100, Eric Auger wrote:
>> Hi Mostafa,
>>
>> On 2/26/23 23:06, Mostafa Saleh wrote:
>>> In preparation for adding stage-2 support, add a S2 config
>>> struct(SMMUS2Cfg), composed of the following fields and embedded in
>>> the main SMMUTransCfg:
>>>  -tsz: Input range
>>>  -sl0: start level of translation
>>>  -affd: AF fault disable
>>>  -granule_sz: Granule page shift
>>>  -vmid: VMID
>>>  -vttb: PA of translation table
>>>  -oas: Output address size
>>>
>>> They will be used in the next patches in stage-2 address translation.
>>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Mostafa Saleh <smostafa@google.com>
>>> ---
>>> Changes in v2:
>>> -Add oas
>>> ---
>>>  include/hw/arm/smmu-common.h | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
>>> index 9fcff26357..2deead08d6 100644
>>> --- a/include/hw/arm/smmu-common.h
>>> +++ b/include/hw/arm/smmu-common.h
>>> @@ -58,6 +58,16 @@ typedef struct SMMUTLBEntry {
>>>      uint8_t granule;
>>>  } SMMUTLBEntry;
>>>  
>>> +typedef struct SMMUS2Cfg {
>>> +    uint8_t tsz;            /* Input range */
>> nit: Size of IPA input region. Called t0sz
> I named it this way to be similar to stage-1, as SMMUTransTableInfo
> just calls it tsz, I guess that is because it can hold either t0sz or
> t1sz.
> I can rename it for stage-2 to t0sz.
no strong opinion though, you can simply add this in the comment
>
>>> +    uint8_t sl0;            /* Start level of translation */
>>> +    bool affd;              /* AF Fault Disable */
>>> +    uint8_t granule_sz;     /* Granule page shift */
>>> +    uint8_t oas;            /* Output address size */
>> called s2ps. if you don't want to rename you may add this in the comment.
> This is not the S2PS parsed from the STE, but the effective value which is
> capped to SMMU_IDR5.OAS, which is used for checking output size, I can add
> a clarifying comment.
ok
>
>> I am suprised to not see S2R which would match S1 record_faults.
> I was thinking about this also, right now we piggy back on record_faults
> in SMMUTransCfg.
> But it makes more sense to separate this from the beginning as other
> fields. I will update that by adding record_faults field in SMMUS2Cfg.
ok
>
>> Also without further reading we can wonder wheter the parent
>> iotlb_hits/misses also record S2 events?
> For iotlb_*, they are shared also. However, I think this is not important for now as
> it is not architectural, and it produces the correct output as only one
> stage is advertised at the moment.
> When nesting is supported, TLB implementation might change and then we
> can take a decision about this.
>
>> I think we need to be/make clear what fields of the original S1 parent
>> struct also are relevant for the S2 only case.
>> Maybe tag them with a specific comment S1-only or S1 | S2. It may be
>> cleaner to introduce a union and common fields in the parent struct though.
> I agree, maybe we encapsulate stage-1 only fields in a similar struct
> and leave common fields outside.
>
> struct SMMUTransCfg:
> 	stage, bypassed, disabled, iotlb_* //common fields
> 	struct SMMUS2Cfg
> 	struct SMMUS1Cfg
> 		ttb,asid....
>
> Or we can add SMMUS1Cfg and SMMUS2Cfg in a union for now and when nesting is supported we
> separate them.

yeah that would be cleaner but it is also more invasive. At least make
it clear in the comments and commit msg what is used for each stage.

Thanks

Eric
>
>
> Thanks,
> Mostafa
>

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.