Re: [PATCH 3/6] iommufd: Add IOMMU_DEVICE_GET_INFO

[Jason Gunthorpe]

On Fri, Feb 10, 2023 at 11:10:34AM +0000, Joao Martins wrote:
> On 10/02/2023 07:55, Tian, Kevin wrote:
> >> From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> >> Sent: Thursday, February 9, 2023 12:17 PM
> >> + * zeroed if the user buffer is larger than the data kernel has.
> >> + *
> >> + * The type specific data would be used to sync capability between the
> >> + * vIOMMU and the hardware IOMMU, also for the availabillity checking of
> >> + * iommu hardware features like dirty page tracking in I/O page table.
> > 
> > It's fine to report format information related to stage-1 page table
> > which userspace manages.
> > 
> > but IMHO this should not be an interface to report which capability is
> > supported by iommufd. Having hardware supporting dirty bit 
> > doesn't mean the underlying iommu driver provides necessary support
> > to iommufd dirty tracking.
> > 
> 
> +1
> 
> In fact this shouldn't really be a way to check any capability as we are dumping
> straight away the IOMMU hardware registers. By dumping raw IOMMU hardware data,
> forces the application to understand IOMMU hardware specific formats.  Maybe
> that's OK for hw nesting as there's a lot of info you need to know for the
> vIOMMU pgtables, pasid and etc so both are tightly coupled. But it is a bit
> disconnected from what really the software (IOMMUFD) and driver can use, without
> getting onto assumptions.
> 
> [Calling it IOMMU_DEVICE_GET_HW_INFO would be bit more obvious if you're not
> asking IOMMUFD support]
> 
> For capability checking, I think this really should be returning capabilities
> that both IOMMU driver supports ... and that IOMMUFD understands and marshalled
> on a format of its own defined by IOMMUFD. Maybe using IOMMU_CAP or some other
> thing within the kernel (now that's per-device), or even simpler. That's at
> least had written initially for the dirty tracking series.

Yes, the design of IOMMU_DEVICE_GET_INFO is already split. The HW
specific structure should only contain things related to operating the
HW specific paths (ie other HW specific structures in other APIs)

The enclosing struct should have general information about operating
the device through the abstract API - like dirty tracking.

But it may be that HW will individually report dirty tracking
capabilties related to special page table types - ie can the S2 page
table do dirty tracking

Jason