Re: [PATCH v7 13/23] iommu/smmuv3: Implement attach/detach_pasid_table

[Robin Murphy]

On 10/05/2019 15:35, Auger Eric wrote:
> Hi Robin,
>
> On 5/8/19 4:38 PM, Robin Murphy wrote:
> > On 08/04/2019 13:19, Eric Auger wrote:
> > > On attach_pasid_table() we program STE S1 related info set
> > > by the guest into the actual physical STEs. At minimum
> > > we need to program the context descriptor GPA and compute
> > > whether the stage1 is translated/bypassed or aborted.
> > >
> > > Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
> > >
> > > ---
> > > v6 -> v7:
> > > - check versions and comment the fact we don't need to take
> > >  ÂÂ into account s1dss and s1fmt
> > > v3 -> v4:
> > > - adapt to changes in iommu_pasid_table_config
> > > - different programming convention at s1_cfg/s2_cfg/ste.abort
> > >
> > > v2 -> v3:
> > > - callback now is named set_pasid_table and struct fields
> > >  ÂÂ are laid out differently.
> > >
> > > v1 -> v2:
> > > - invalidate the STE before changing them
> > > - hold init_mutex
> > > - handle new fields
> > > ---
> > >  Â drivers/iommu/arm-smmu-v3.c | 121 ++++++++++++++++++++++++++++++++++++
> > >  Â 1 file changed, 121 insertions(+)
> > >
> > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > > index e22e944ffc05..1486baf53425 100644
> > > --- a/drivers/iommu/arm-smmu-v3.c
> > > +++ b/drivers/iommu/arm-smmu-v3.c
> > > @@ -2207,6 +2207,125 @@ static void arm_smmu_put_resv_regions(struct
> > > device *dev,
> > >  ÂÂÂÂÂÂÂÂÂ kfree(entry);
> > >  Â }
> > >  Â +static int arm_smmu_attach_pasid_table(struct iommu_domain *domain,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct iommu_pasid_table_config *cfg)
> > > +{
> > > +ÂÂÂ struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> > > +ÂÂÂ struct arm_smmu_master_data *entry;
> > > +ÂÂÂ struct arm_smmu_s1_cfg *s1_cfg;
> > > +ÂÂÂ struct arm_smmu_device *smmu;
> > > +ÂÂÂ unsigned long flags;
> > > +ÂÂÂ int ret = -EINVAL;
> > > +
> > > +ÂÂÂ if (cfg->format != IOMMU_PASID_FORMAT_SMMUV3)
> > > +ÂÂÂÂÂÂÂ return -EINVAL;
> > > +
> > > +ÂÂÂ if (cfg->version != PASID_TABLE_CFG_VERSION_1 ||
> > > +ÂÂÂÂÂÂÂ cfg->smmuv3.version != PASID_TABLE_SMMUV3_CFG_VERSION_1)
> > > +ÂÂÂÂÂÂÂ return -EINVAL;
> > > +
> > > +ÂÂÂ mutex_lock(&smmu_domain->init_mutex);
> > > +
> > > +ÂÂÂ smmu = smmu_domain->smmu;
> > > +
> > > +ÂÂÂ if (!smmu)
> > > +ÂÂÂÂÂÂÂ goto out;
> > > +
> > > +ÂÂÂ if (!((smmu->features & ARM_SMMU_FEAT_TRANS_S1) &&
> > > +ÂÂÂÂÂÂÂÂÂ (smmu->features & ARM_SMMU_FEAT_TRANS_S2))) {
> > > +ÂÂÂÂÂÂÂ dev_info(smmu_domain->smmu->dev,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂ "does not implement two stages\n");
> > > +ÂÂÂÂÂÂÂ goto out;
> > > +ÂÂÂ }
> >
> > That check is redundant (and frankly looks a little bit spammy). If the
> > one below is not enough, there is a problem elsewhere - if it's possible
> > for smmu_domain->stage to ever get set to ARM_SMMU_DOMAIN_NESTED without
> > both stages of translation present, we've already gone fundamentally wrong.
>
> Makes sense. Moved that check to arm_smmu_domain_finalise() instead and
> remove redundant ones.

Urgh, I forgot exactly how the crazy domain-allocation dance worked, 
such that we're not in a position to refuse the domain_set_attr() call 
itself, but that does sound like the best compromise for now.

Thanks,
Robin.