On Mon, Jan 25, 2021 at 01:50:09PM +0000, Jonathan Cameron wrote:
> > +static int arm_smmu_master_sva_enable_iopf(struct arm_smmu_master *master)
> > +{
> > +   int ret;
> > +   struct device *dev = master->dev;
> > +
> > +   /*
> > +    * Drivers for devices supporting PRI or stall should enable IOPF first.
> > +    * Others have device-specific fault handlers and don't need IOPF.
> > +    */
> > +   if (!arm_smmu_master_iopf_supported(master))
> 
> So if we have master->iopf_enabled and this happens. Then I'm not totally sure
> what prevents the disable below running its cleanup on stuff that was never
> configured.

Since arm_smmu_dev_enable_feature() checks that the feature is supported,
iopf_enabled can only be true if arm_smmu_master_iopf_supported() is true.

What's missing is checking that drivers don't disable IOPF while SVA is
enabled - or else the disable below can leak. Another thing I broke in v10 :/

Thanks,
Jean

> 
> > +           return 0;
> > +
> > +   if (!master->iopf_enabled)
> > +           return -EINVAL;
> > +
> > +   ret = iopf_queue_add_device(master->smmu->evtq.iopf, dev);
> > +   if (ret)
> > +           return ret;
> > +
> > +   ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev);
> > +   if (ret) {
> > +           iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
> > +           return ret;
> > +   }
> > +   return 0;
> > +}
> > +
> > +static void arm_smmu_master_sva_disable_iopf(struct arm_smmu_master 
> > *master)
> > +{
> > +   struct device *dev = master->dev;
> > +
> > +   if (!master->iopf_enabled)
> > +           return;
> 
> As above, I think you need a sanity check on
> 
> !arm_smmu_master_iopf_supported(master) before clearing the following.
> 
> I may well be missing something that stops us getting here though.
> 
> Alternative is probably to sanity check iopf_enabled = true is supported
> before letting a driver set it.
> 
> 
> > +
> > +   iommu_unregister_device_fault_handler(dev);
> > +   iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
> > +}
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Reply via email to