Re: [PATCH v2 8/8] iommu/arm-smmu-v3: Support IOMMU_DOMAIN_NESTED

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Sep 10, 2024 at 11:12:20AM +0000, Mostafa Saleh wrote:
> On Fri, Sep 06, 2024 at 10:34:44AM -0300, Jason Gunthorpe wrote:
> > On Fri, Sep 06, 2024 at 11:07:47AM +0000, Mostafa Saleh wrote:
> > 
> > > However, I believe the UAPI can be more clear and solid in terms of
> > > what is supported (maybe a typical struct with the CD, and some
> > > extra configs?) I will give it a think.
> > 
> > I don't think breaking up the STE into fields in another struct is
> > going to be a big improvement, it adds more code and corner cases to
> > break up and reassemble it.
> > 
> > #define STRTAB_STE_0_NESTING_ALLOWED                                         \
> > 	cpu_to_le64(STRTAB_STE_0_V | STRTAB_STE_0_CFG | STRTAB_STE_0_S1FMT | \
> > 		    STRTAB_STE_0_S1CTXPTR_MASK | STRTAB_STE_0_S1CDMAX)
> > #define STRTAB_STE_1_NESTING_ALLOWED                            \
> > 	cpu_to_le64(STRTAB_STE_1_S1DSS | STRTAB_STE_1_S1CIR |   \
> > 		    STRTAB_STE_1_S1COR | STRTAB_STE_1_S1CSH |   \
> > 		    STRTAB_STE_1_S1STALLD | STRTAB_STE_1_EATS)
> > 
> > It is 11 fields that would need to be recoded, that's alot.. Even if
> > you say the 3 cache ones are not needed it is still alot.
> 
> I was thinking of providing a higher level semantics
> (no need for caching, valid...), something like:

Well, that isn't higher level semantics, really, it is just splitting
up the existing fields.

We do need to do something with valid as well as the VM can create a
non-valid STE and we still have to wrap a nesting domain around it to
ensure that event routing can work.

> struct smmu_user_table {
> 	u64 cd_table;
> 	u32 smmu_cd_cfg;  /* linear or 2lvl,.... */
> 	u32 smmu_trans_cfg; /* Translate, bypass, abort */
> 	u32 dev_feat; /*ATS, STALL, …*/
> };
> 
> I feel that is a bit more clear for user space?

Having done these sorts of interfaces over a long time, I belive it is
not. Deviating from the native HW format and re-marshalling into
something else is error prone and can become a problem when the
transformation from the well known HW format to the intermediate
format becomes a source of confusion too.

> Instead of partially setting the STE, and it should be easier to
> extend than masking the STE.

It is not going to partially set, it is going to validate a mask from
the original vSTE and if the mask fails then it will create a
non-valid STE instead.

We can't eliminate the mask because the VMM needs to mask and check
always no matter what the kernel interface is.

One option for the vmm is to just pass the vSTE entirely to the kernel
and let it validate it. If validation fails then use a V=0 STE
instead.
x
> I’am not opposed to the vSTE, I just feel it's loosely defined,
> that's why I was asking for the docs.

The kdoc lists all the fields and it is reflected directly to HW, and
there is a bitmask above being very explicit about what bits are
allowed. Where is the loosely defined you see?

If we broaden the mask down the road then we'd need some feature bits
to inform the VMM that the kernel supports a wider vSTE mask.

Thanks,
Jason