RE: [PATCH v9 04/11] iommu: Add sva iommu_domain support

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
> Sent: Tuesday, June 28, 2022 7:34 PM
> 
> On 2022/6/28 16:50, Tian, Kevin wrote:
> >> From: Baolu Lu<baolu.lu@xxxxxxxxxxxxxxx>
> >> Sent: Tuesday, June 28, 2022 1:41 PM
> >>>>    struct iommu_domain {
> >>>>    	unsigned type;
> >>>>    	const struct iommu_domain_ops *ops;
> >>>>    	unsigned long pgsize_bitmap;	/* Bitmap of page sizes in use */
> >>>> -	iommu_fault_handler_t handler;
> >>>> -	void *handler_token;
> >>>>    	struct iommu_domain_geometry geometry;
> >>>>    	struct iommu_dma_cookie *iova_cookie;
> >>>> +	union {
> >>>> +		struct {	/* IOMMU_DOMAIN_DMA */
> >>>> +			iommu_fault_handler_t handler;
> >>>> +			void *handler_token;
> >>>> +		};
> >>> why is it DMA domain specific? What about unmanaged
> >>> domain? Unrecoverable fault can happen on any type
> >>> including SVA. Hence I think above should be domain type
> >>> agnostic.
> >> The report_iommu_fault() should be replaced by the new
> >> iommu_report_device_fault(). Jean has already started this work.
> >>
> >> https://lore.kernel.org/linux-iommu/Yo4Nw9QyllT1RZbd@myrica/
> >>
> >> Currently this is only for DMA domains, hence Robin suggested to make it
> >> exclude with the SVA domain things.
> >>
> >> https://lore.kernel.org/linux-iommu/f3170016-4d7f-e78e-db48-
> >> 68305f683349@xxxxxxx/
> > Then it's worthy a comment that those two fields are for
> > some legacy fault reporting stuff and DMA type only.
> 
> The iommu_fault and SVA fields are exclusive. The former is used for
> unrecoverable DMA remapping faults, while the latter is only interested
> in the recoverable page faults.
> 
> I will update the commit message with above explanation. Does this work
> for you?
> 

Not exactly. Your earlier explanation is about old vs. new API thus
leaving the existing fault handler with current only user is fine.

but this is not related to unrecoverable vs. recoverable. As I said
unrecoverable could happen on all domain types. Tying it to
DMA-only doesn't make sense and I think in the end the new
iommu_report_device_fault() will need support both. Is it not the
case?