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

[Mostafa Saleh]

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