Re: [PATCH 3/9] iommu: Introduce iommu do invalidate API function

[Jean-Philippe Brucker <jean-philippe.brucker@arm.com>]

On 28/06/17 17:09, Jacob Pan wrote:
> On Wed, 28 Jun 2017 12:08:23 +0200
> Joerg Roedel <joro@8bytes.org> wrote:
> 
>> On Tue, Jun 27, 2017 at 12:47:57PM -0700, Jacob Pan wrote:
>>> From: "Liu, Yi L" <yi.l.liu@linux.intel.com>
>>>
>>> When a SVM capable device is assigned to a guest, the first level
>>> page tables are owned by the guest and the guest PASID table
>>> pointer is linked to the device context entry of the physical IOMMU.
>>>
>>> Host IOMMU driver has no knowledge of caching structure updates
>>> unless the guest invalidation activities are passed down to the
>>> host. The primary usage is derived from emulated IOMMU in the
>>> guest, where QEMU can trap invalidation activities before pass them
>>> down the host/physical IOMMU. There are IOMMU architectural
>>> specific actions need to be taken which requires the generic APIs
>>> introduced in this patch to have opaque data in the
>>> tlb_invalidate_info argument.  
>>
>> Which "IOMMU architectural specific actions" are you thinking of?
>>
> construction of queued invalidation descriptors, then submit them to
> the IOMMU QI interface.
>>> +int iommu_invalidate(struct iommu_domain *domain,
>>> +		struct device *dev, struct tlb_invalidate_info
>>> *inv_info) +{
>>> +	int ret = 0;
>>> +
>>> +	if (unlikely(!domain->ops->invalidate))
>>> +		return -ENODEV;
>>> +
>>> +	ret = domain->ops->invalidate(domain, dev, inv_info);
>>> +
>>> +	return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(iommu_invalidate);  
>>
>> [...]
>>
>>> +struct tlb_invalidate_info {
>>> +	__u32	model;
>>> +	__u32	length;
>>> +	__u8	opaque[];
>>> +};  
>>
>> This interface is aweful. It requires the user of a generic api to
>> know details about the implementation behind to do anything useful.
>>
>> Please explain in more detail why this is needed. My feeling is that
>> we can make this more generic with a small set of invalidation
>> functions in the iommu-api.
>>
> My thinking was that via configuration control, there will be unlikely
> any mixed IOMMU models between pIOMMU and vIOMMU. We could have just
> model specific data pass through layers of SW (QEMU, VFIO) for
> performance reasons. We do have an earlier hybrid version that has
> generic data and opaque raw data. Would the below work for all IOMMU
> models?

For reference, this was also discussed in the initial posting of the series:
https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg03452.html

At least for ARM SMMUv2 and v3, I think the invalidation format you
propose should be sufficient, although "device_selective" should probably
be "domain_selective". And maybe a flag field could contain relatively
generic hints such as "only invalidate leaf table when page_selective".

Thanks,
Jean

> https://www.spinics.net/lists/kvm/msg148798.html
> 
> struct tlb_invalidate_info
> {
>         __u32   model;  /* Vendor number */
>         __u8 granularity
> #define DEVICE_SELECTVIE_INV    (1 << 0)
> #define PAGE_SELECTIVE_INV      (1 << 0)
> #define PASID_SELECTIVE_INV     (1 << 1)
>         __u32 pasid;
>         __u64 addr;
>         __u64 size;
> 
>         /* Since IOMMU format has already been validated for this table,
>            the IOMMU driver knows that the following structure is in a
>            format it knows */
>         __u8 opaque[];
> };
> 
>>
>>
>> 	Joerg
>>
> 
> [Jacob Pan]
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>