Re: [PATCH v2 01/40] iommu: Introduce Shared Virtual Addressing API

Christian König
2018-09-06T11:12:42Z

Am 06.09.2018 um 13:09 schrieb Jean-Philippe Brucker:
> Hi Eric,
>
> Thanks for reviewing
>
> On 05/09/2018 12:29, Auger Eric wrote:
>>> +int iommu_sva_device_init(struct device *dev, unsigned long features,
>>> + unsigned int max_pasid)
>> what about min_pasid?
> No one asked for it... The max_pasid parameter is here for drivers that
> have vendor-specific PASID size limits, such as AMD KFD (see
> kfd_iommu_device_init and
> https://patchwork.kernel.org/patch/9989307/#21389571). But in most cases
> the PASID size will only depend on the PCI PASID capability and the
> IOMMU limits, both known by the IOMMU driver, so device drivers won't
> have to set max_pasid.
>
> IOMMU drivers need to set min_pasid in the sva_device_init callback
> because it may be either 1 (e.g. Arm where PASID #0 is reserved) or 0
> (Intel Vt-d rev2), but at the moment I can't see a reason for device
> drivers to override min_pasid

Sorry to ruin your day, but if I'm not completely mistaken PASID zero is
reserved in the AMD KFD as well.

Regards,
Christian.

>
>>> + /*
>>> + * IOMMU driver updates the limits depending on the IOMMU and device
>>> + * capabilities.
>>> + */
>>> + ret = domain->ops->sva_device_init(dev, param);
>>> + if (ret)
>>> + goto err_free_param;
>> So you are likely to call sva_device_init even if it was already called
>> (ie. dev->iommu_param->sva_param is already set). Can't you test whether
>> dev->iommu_param->sva_param is already set first?
> Right, that's probably better
>
>>> +/**
>>> + * iommu_sva_device_shutdown() - Shutdown Shared Virtual Addressing for a device
>>> + * @dev: the device
>>> + *
>>> + * Disable SVA. Device driver should ensure that the device isn't performing any
>>> + * DMA while this function is running.
>>> + */
>>> +int iommu_sva_device_shutdown(struct device *dev)
>> Not sure the returned value is required for a shutdown operation.
> I don't know either. I like them because they help me debug, but are
> otherwise rather useless if we don't describe precise semantics. The
> caller cannot do anything with it. Given that the corresponding IOMMU op
> is already void, I can change this function to void as well
>
>>> +struct iommu_sva_param {
>> What are the feature values?
> At the moment only IOMMU_SVA_FEAT_IOPF, introduced by patch 09
>
>>> + unsigned long features;
>>> + unsigned int min_pasid;
>>> + unsigned int max_pasid;
>>> +};
>>> +
>>> /**
>>> * struct iommu_ops - iommu ops and capabilities
>>> * @capable: check capability
>>> @@ -219,6 +225,8 @@ struct page_response_msg {
>>> * @domain_free: free iommu domain
>>> * @attach_dev: attach device to an iommu domain
>>> * @detach_dev: detach device from an iommu domain
>>> + * @sva_device_init: initialize Shared Virtual Adressing for a device
>> Addressing
>>> + * @sva_device_shutdown: shutdown Shared Virtual Adressing for a device
>> Addressing
> Nice catch
>
> Thanks,
> Jean