Re: [PATCH v7 2/9] iommu/arm-smmu-v3: Make CD programming use arm_smmu_write_entry()

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Apr 18, 2024 at 02:01:31PM +0100, Robin Murphy wrote:
> On 16/04/2024 8:28 pm, Jason Gunthorpe wrote:
> > CD table entries and STE's have the same essential programming sequence,
> > just with different types.
> > 
> > Have arm_smmu_write_ctx_desc() generate a target CD and call
> > arm_smmu_write_entry() to do the programming. Due to the way the target CD
> > is generated by modifying the existing CD this alone is not enough for the
> > CD callers to be freed of the ordering requirements.
> > 
> > The following patches will make the rest of the CD flow mirror the STE
> > flow with precise CD contents generated in all cases.
> > 
> > Signed-off-by: Michael Shavit <mshavit@google.com>
> > Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> > Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > Reviewed-by: Moritz Fischer <moritzf@google.com>
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 94 ++++++++++++++++-----
> >   1 file changed, 74 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index bf105e914d38b1..3983de90c2fa01 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -56,6 +56,7 @@ struct arm_smmu_entry_writer_ops {
> >   #define NUM_ENTRY_QWORDS 8
> >   static_assert(sizeof(struct arm_smmu_ste) == NUM_ENTRY_QWORDS * sizeof(u64));
> > +static_assert(sizeof(struct arm_smmu_cd) == NUM_ENTRY_QWORDS * sizeof(u64));
> >   static phys_addr_t arm_smmu_msi_cfg[ARM_SMMU_MAX_MSIS][3] = {
> >   	[EVTQ_MSI_INDEX] = {
> > @@ -1231,6 +1232,67 @@ static struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master,
> >   	return &l1_desc->l2ptr[idx];
> >   }
> > +struct arm_smmu_cd_writer {
> > +	struct arm_smmu_entry_writer writer;
> > +	unsigned int ssid;
> > +};
> > +
> > +static void arm_smmu_get_cd_used(const __le64 *ent, __le64 *used_bits)
> > +{
> > +	used_bits[0] = cpu_to_le64(CTXDESC_CD_0_V);
> > +	if (!(ent[0] & cpu_to_le64(CTXDESC_CD_0_V)))
> > +		return;
> > +	memset(used_bits, 0xFF, sizeof(struct arm_smmu_cd));
> > +
> > +	/* EPD0 means T0SZ/TG0/IR0/OR0/SH0/TTB0 are IGNORED */
> 
> They're ignored if the *effective value* of EPD0 is 1, which means you also
> need to account for when EPD0 itself is ignored, or all this complication is
> essentially meaningless.

Do you mean this?

 Consistent with Armv8-A translation, the EPD0 and EPD1 fields are
 IGNORED (and their effective value is 0) if this CD is located from
 an STE with StreamWorld of any-EL2 or EL3. It is only possible for an
 EL1 (Secure or non-secure) or any-EL2-E2H stream to disable
 translation table walk sing EPD0 or EPD1.

Regardless, part of the design is that the make functions don't set
IGNORED bits and get_used only has to process what the make functions
build, not the universe of all descriptors.

In this case the make function sets EPD0 and constructs a CD that is
only valid if EPD0 is available. It also zeros the TTB0/etc values
because they are expected to be IGNORED and the code has no valid
value to provide anyhow.

The comment was intened to be read as: if EPD0 is set [by the make
function] then TTB0/etc will be IGNORED.

I will update the comment for clarity.

The complexity you are talking about must be delt with by the make
side. If we do need to support something where EPD0 doesn't work then
make functions must never set it.

Do we have a problem here? Can SVA activate and EPD0 will be ignored?
That would be a security bug.

Thanks,
Jason