Re: [PATCH v4 1/5] iommu/s390: Fix duplicate domain attachments

[Niklas Schnelle]

On Thu, 2022-10-06 at 09:02 -0300, Jason Gunthorpe wrote:
> On Thu, Oct 06, 2022 at 01:52:44PM +0200, Niklas Schnelle wrote:
> 
> > One option I see would be to ignore the error return from
> > zpci_register_ioat() if it indicates case 2b. Then we would still add
> > the device to the IOMMU's devices list and return success despite
> > knowing that the device is inaccessible (DMA and MMIO blocked).
> > 
> > Then the recovery/reset code will register the new domain once the
> > device comes out of the error state. At least from an IOMMU API point
> > of view that would make the attachment always succeed for all
> > zpci_register_ioat() error cases that aren't programming bugs and can
> > conceivably be recovered from.
> 
> This is what I was thinking..
> 
> > If you agree I would propose adding this as a robustness improvement as
> > part of my upcoming series of IOMMU improvements needed for the DMA API
> > conversion. As stated above before the DMA API conversion any error
> > that would cause zpci_register_ioat() to fail while the IOMMU API is
> > being used will need a "power cycle" anyway so postponing this doesn't
> > hurt.
> 
> Yes, I think this series is fine as is
> 
> Patch 4 mostly deletes all these error cases, and the one hunk that is left:
> 
> +	if (domain->geometry.aperture_start > zdev->end_dma ||
> +	    domain->geometry.aperture_end < zdev->start_dma)
> +		return -EINVAL;
> 
> Is misplaced. If a device cannot be supported by the IOMMU, which is
> what that is really saying since it only s390 creates one aperture
> size, then it should fail to probe, not fail at attach.
> 
> So I'd change the above to a WARN_ON() for future safety and add a
> similar test to probe and then all that is left is the
> zpci_register_ioat() which you have a plan for.
> 
> Jason
>  

Sounds good will do a v5 anyway to add the map_pages()/unmap_pages().