diff mbox series

[2/6] iommu/vt-d: Implement hw_info for iommu capability query

Message ID 20230209041642.9346-3-yi.l.liu@intel.com
State New
Headers
Series iommufd: Add iommu capability reporting |

Commit Message

Yi Liu Feb. 9, 2023, 4:16 a.m. UTC 
  From: Lu Baolu <baolu.lu@linux.intel.com>

To support nested translation in the userspace, it should check the
underlying hardware information for the capabilities.

Add intel_iommu_hw_info() to report cap_reg and ecap_reg information.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
---
 drivers/iommu/intel/iommu.c  | 19 +++++++++++++++++++
 drivers/iommu/intel/iommu.h  |  1 +
 include/uapi/linux/iommufd.h | 21 +++++++++++++++++++++
 3 files changed, 41 insertions(+)

Comments

Tian, Kevin Feb. 10, 2023, 7:32 a.m. UTC | #1 
> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Thursday, February 9, 2023 12:17 PM
> 
> From: Lu Baolu <baolu.lu@linux.intel.com>
> 
> To support nested translation in the userspace, it should check the
> underlying hardware information for the capabilities.

remove this sentence. that belongs to next patch.

> +
> +/**
> + * struct iommu_device_info_vtd - Intel VT-d device info
> + *
> + * @flags: Must be set to 0
> + * @__reserved: Must be 0
> + * @cap_reg: Value of Intel VT-d capability register defined in chapter
> + *	     11.4.2 of Intel VT-d spec.
> + * @ecap_reg: Value of Intel VT-d capability register defined in chapter
> + *	     11.4.3 of Intel VT-d spec.

let's be specific with section name e.g. "11.4.2 Capability Register" so if
the chapter index is changed later people can still know where to look
at.

> + *
> + * Intel hardware iommu capability.

duplicated with the first line "Intel VT-d device info"
Alex Williamson Feb. 10, 2023, 10:44 p.m. UTC | #2 
On Wed,  8 Feb 2023 20:16:38 -0800
Yi Liu <yi.l.liu@intel.com> wrote:

> From: Lu Baolu <baolu.lu@linux.intel.com>
> 
> To support nested translation in the userspace, it should check the
> underlying hardware information for the capabilities.
> 
> Add intel_iommu_hw_info() to report cap_reg and ecap_reg information.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> ---
>  drivers/iommu/intel/iommu.c  | 19 +++++++++++++++++++
>  drivers/iommu/intel/iommu.h  |  1 +
>  include/uapi/linux/iommufd.h | 21 +++++++++++++++++++++
>  3 files changed, 41 insertions(+)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 59df7e42fd53..929f600cc350 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4760,8 +4760,26 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>  	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
>  }
>  
> +static void *intel_iommu_hw_info(struct device *dev, u32 *length)
> +{
> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> +	struct intel_iommu *iommu = info->iommu;
> +	struct iommu_device_info_vtd *vtd;
> +
> +	vtd = kzalloc(sizeof(*vtd), GFP_KERNEL);
> +	if (!vtd)
> +		return ERR_PTR(-ENOMEM);
> +
> +	vtd->cap_reg = iommu->cap;
> +	vtd->ecap_reg = iommu->ecap;

Just a friendly reminder that these registers are already exposed to
userspace under /sys/class/iommu/ and each device has an iommu link
back to their iommu device there.  This series doesn't really stand on
its own without some discussion of why that interface is not sufficient
for this use case.  Thanks,

Alex
Jason Gunthorpe Feb. 11, 2023, 12:15 a.m. UTC | #3 
On Fri, Feb 10, 2023 at 03:44:10PM -0700, Alex Williamson wrote:
> On Wed,  8 Feb 2023 20:16:38 -0800
> Yi Liu <yi.l.liu@intel.com> wrote:
> 
> > From: Lu Baolu <baolu.lu@linux.intel.com>
> > 
> > To support nested translation in the userspace, it should check the
> > underlying hardware information for the capabilities.
> > 
> > Add intel_iommu_hw_info() to report cap_reg and ecap_reg information.
> > 
> > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> > ---
> >  drivers/iommu/intel/iommu.c  | 19 +++++++++++++++++++
> >  drivers/iommu/intel/iommu.h  |  1 +
> >  include/uapi/linux/iommufd.h | 21 +++++++++++++++++++++
> >  3 files changed, 41 insertions(+)
> > 
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index 59df7e42fd53..929f600cc350 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -4760,8 +4760,26 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
> >  	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
> >  }
> >  
> > +static void *intel_iommu_hw_info(struct device *dev, u32 *length)
> > +{
> > +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> > +	struct intel_iommu *iommu = info->iommu;
> > +	struct iommu_device_info_vtd *vtd;
> > +
> > +	vtd = kzalloc(sizeof(*vtd), GFP_KERNEL);
> > +	if (!vtd)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	vtd->cap_reg = iommu->cap;
> > +	vtd->ecap_reg = iommu->ecap;
> 
> Just a friendly reminder that these registers are already exposed to
> userspace under /sys/class/iommu/ and each device has an iommu link
> back to their iommu device there. 

I think in cases of mdevs w/ PASID (eg SIOV) it is not general to get
from vfio sysfs to the sysfs path for the iommu.

> This series doesn't really stand on its own without some discussion
> of why that interface is not sufficient for this use case.

IMHO I don't really like the idea of mixing iommufd with sysfs, it
should stand on its own.

In particular there is no generic way to go from a iommufd dev_id to
any sysfs path, userspace would need prior unique knowledge about the
subsystem that is being bound to iommufd first.

So, I think some of those explanations would be good in the commit
message?

I would also add explanation about what userspace is supposed to do
with these bits when it operates the nesting feature.

Jason
Baolu Lu Feb. 11, 2023, 3:45 a.m. UTC | #4 
On 2023/2/10 15:32, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Thursday, February 9, 2023 12:17 PM
>>
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>
>> To support nested translation in the userspace, it should check the
>> underlying hardware information for the capabilities.
> 
> remove this sentence. that belongs to next patch.
> 
>> +
>> +/**
>> + * struct iommu_device_info_vtd - Intel VT-d device info
>> + *
>> + * @flags: Must be set to 0
>> + * @__reserved: Must be 0
>> + * @cap_reg: Value of Intel VT-d capability register defined in chapter
>> + *	     11.4.2 of Intel VT-d spec.
>> + * @ecap_reg: Value of Intel VT-d capability register defined in chapter
>> + *	     11.4.3 of Intel VT-d spec.
> 
> let's be specific with section name e.g. "11.4.2 Capability Register" so if
> the chapter index is changed later people can still know where to look
> at.
> 
>> + *
>> + * Intel hardware iommu capability.
> 
> duplicated with the first line "Intel VT-d device info"

Above all agreed.

Best regards,
baolu
Binbin Wu Feb. 13, 2023, 3:09 a.m. UTC | #5 
On 2/9/2023 12:16 PM, Yi Liu wrote:
> From: Lu Baolu <baolu.lu@linux.intel.com>
>
> To support nested translation in the userspace, it should check the
> underlying hardware information for the capabilities.
>
> Add intel_iommu_hw_info() to report cap_reg and ecap_reg information.
>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> ---
>   drivers/iommu/intel/iommu.c  | 19 +++++++++++++++++++
>   drivers/iommu/intel/iommu.h  |  1 +
>   include/uapi/linux/iommufd.h | 21 +++++++++++++++++++++
>   3 files changed, 41 insertions(+)
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 59df7e42fd53..929f600cc350 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4760,8 +4760,26 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>   	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
>   }
>   
> +static void *intel_iommu_hw_info(struct device *dev, u32 *length)
> +{
> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> +	struct intel_iommu *iommu = info->iommu;
> +	struct iommu_device_info_vtd *vtd;
> +
> +	vtd = kzalloc(sizeof(*vtd), GFP_KERNEL);
> +	if (!vtd)
> +		return ERR_PTR(-ENOMEM);
> +
> +	vtd->cap_reg = iommu->cap;
> +	vtd->ecap_reg = iommu->ecap;
> +	*length = sizeof(*vtd);
> +
> +	return vtd;
> +}
> +
>   const struct iommu_ops intel_iommu_ops = {
>   	.capable		= intel_iommu_capable,
> +	.hw_info		= intel_iommu_hw_info,
>   	.domain_alloc		= intel_iommu_domain_alloc,
>   	.probe_device		= intel_iommu_probe_device,
>   	.probe_finalize		= intel_iommu_probe_finalize,
> @@ -4774,6 +4792,7 @@ const struct iommu_ops intel_iommu_ops = {
>   	.def_domain_type	= device_def_domain_type,
>   	.remove_dev_pasid	= intel_iommu_remove_dev_pasid,
>   	.pgsize_bitmap		= SZ_4K,
> +	.driver_type		= IOMMU_DEVICE_DATA_INTEL_VTD,
>   #ifdef CONFIG_INTEL_IOMMU_SVM
>   	.page_response		= intel_svm_page_response,
>   #endif
> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
> index 06e61e474856..2e70265d4ceb 100644
> --- a/drivers/iommu/intel/iommu.h
> +++ b/drivers/iommu/intel/iommu.h
> @@ -22,6 +22,7 @@
>   #include <linux/ioasid.h>
>   #include <linux/bitfield.h>
>   #include <linux/xarray.h>
> +#include <uapi/linux/iommufd.h>
>   
>   #include <asm/cacheflush.h>
>   #include <asm/iommu.h>
> diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
> index 2309edb55028..fda75c8450ee 100644
> --- a/include/uapi/linux/iommufd.h
> +++ b/include/uapi/linux/iommufd.h
> @@ -347,7 +347,28 @@ struct iommu_vfio_ioas {
>   
>   /**
>    * enum iommu_device_data_type - IOMMU hardware Data types
> + * @IOMMU_DEVICE_DATA_INTEL_VTD: Intel VT-d iommu data type
>    */
>   enum iommu_device_data_type {
> +	IOMMU_DEVICE_DATA_INTEL_VTD = 1,
> +};
> +
> +/**
> + * struct iommu_device_info_vtd - Intel VT-d device info
> + *
> + * @flags: Must be set to 0

Could you add more description about the usage of flags here?


> + * @__reserved: Must be 0
> + * @cap_reg: Value of Intel VT-d capability register defined in chapter
> + *	     11.4.2 of Intel VT-d spec.
> + * @ecap_reg: Value of Intel VT-d capability register defined in chapter
> + *	     11.4.3 of Intel VT-d spec.
> + *
> + * Intel hardware iommu capability.
> + */
> +struct iommu_device_info_vtd {
> +	__u32 flags;
> +	__u32 __reserved;
> +	__aligned_u64 cap_reg;
> +	__aligned_u64 ecap_reg;
>   };
>   #endif
Baolu Lu Feb. 13, 2023, 8:48 a.m. UTC | #6 
On 2023/2/13 11:09, Binbin Wu wrote:
> 
> On 2/9/2023 12:16 PM, Yi Liu wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>
>> To support nested translation in the userspace, it should check the
>> underlying hardware information for the capabilities.
>>
>> Add intel_iommu_hw_info() to report cap_reg and ecap_reg information.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c  | 19 +++++++++++++++++++
>>   drivers/iommu/intel/iommu.h  |  1 +
>>   include/uapi/linux/iommufd.h | 21 +++++++++++++++++++++
>>   3 files changed, 41 insertions(+)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 59df7e42fd53..929f600cc350 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -4760,8 +4760,26 @@ static void intel_iommu_remove_dev_pasid(struct 
>> device *dev, ioasid_t pasid)
>>       intel_pasid_tear_down_entry(iommu, dev, pasid, false);
>>   }
>> +static void *intel_iommu_hw_info(struct device *dev, u32 *length)
>> +{
>> +    struct device_domain_info *info = dev_iommu_priv_get(dev);
>> +    struct intel_iommu *iommu = info->iommu;
>> +    struct iommu_device_info_vtd *vtd;
>> +
>> +    vtd = kzalloc(sizeof(*vtd), GFP_KERNEL);
>> +    if (!vtd)
>> +        return ERR_PTR(-ENOMEM);
>> +
>> +    vtd->cap_reg = iommu->cap;
>> +    vtd->ecap_reg = iommu->ecap;
>> +    *length = sizeof(*vtd);
>> +
>> +    return vtd;
>> +}
>> +
>>   const struct iommu_ops intel_iommu_ops = {
>>       .capable        = intel_iommu_capable,
>> +    .hw_info        = intel_iommu_hw_info,
>>       .domain_alloc        = intel_iommu_domain_alloc,
>>       .probe_device        = intel_iommu_probe_device,
>>       .probe_finalize        = intel_iommu_probe_finalize,
>> @@ -4774,6 +4792,7 @@ const struct iommu_ops intel_iommu_ops = {
>>       .def_domain_type    = device_def_domain_type,
>>       .remove_dev_pasid    = intel_iommu_remove_dev_pasid,
>>       .pgsize_bitmap        = SZ_4K,
>> +    .driver_type        = IOMMU_DEVICE_DATA_INTEL_VTD,
>>   #ifdef CONFIG_INTEL_IOMMU_SVM
>>       .page_response        = intel_svm_page_response,
>>   #endif
>> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
>> index 06e61e474856..2e70265d4ceb 100644
>> --- a/drivers/iommu/intel/iommu.h
>> +++ b/drivers/iommu/intel/iommu.h
>> @@ -22,6 +22,7 @@
>>   #include <linux/ioasid.h>
>>   #include <linux/bitfield.h>
>>   #include <linux/xarray.h>
>> +#include <uapi/linux/iommufd.h>
>>   #include <asm/cacheflush.h>
>>   #include <asm/iommu.h>
>> diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
>> index 2309edb55028..fda75c8450ee 100644
>> --- a/include/uapi/linux/iommufd.h
>> +++ b/include/uapi/linux/iommufd.h
>> @@ -347,7 +347,28 @@ struct iommu_vfio_ioas {
>>   /**
>>    * enum iommu_device_data_type - IOMMU hardware Data types
>> + * @IOMMU_DEVICE_DATA_INTEL_VTD: Intel VT-d iommu data type
>>    */
>>   enum iommu_device_data_type {
>> +    IOMMU_DEVICE_DATA_INTEL_VTD = 1,
>> +};
>> +
>> +/**
>> + * struct iommu_device_info_vtd - Intel VT-d device info
>> + *
>> + * @flags: Must be set to 0
> 
> Could you add more description about the usage of flags here?

This is a Reserved 0 fields for other coming features.

Best regards,
baolu
Jason Gunthorpe Feb. 13, 2023, 2:51 p.m. UTC | #7 
On Wed, Feb 08, 2023 at 08:16:38PM -0800, Yi Liu wrote:
> From: Lu Baolu <baolu.lu@linux.intel.com>
> 
> To support nested translation in the userspace, it should check the
> underlying hardware information for the capabilities.
> 
> Add intel_iommu_hw_info() to report cap_reg and ecap_reg information.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> ---
>  drivers/iommu/intel/iommu.c  | 19 +++++++++++++++++++
>  drivers/iommu/intel/iommu.h  |  1 +
>  include/uapi/linux/iommufd.h | 21 +++++++++++++++++++++
>  3 files changed, 41 insertions(+)

This should come after the next patch in the series

Jason
diff mbox series

Patch

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 59df7e42fd53..929f600cc350 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4760,8 +4760,26 @@  static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
 	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
 }
 
+static void *intel_iommu_hw_info(struct device *dev, u32 *length)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct intel_iommu *iommu = info->iommu;
+	struct iommu_device_info_vtd *vtd;
+
+	vtd = kzalloc(sizeof(*vtd), GFP_KERNEL);
+	if (!vtd)
+		return ERR_PTR(-ENOMEM);
+
+	vtd->cap_reg = iommu->cap;
+	vtd->ecap_reg = iommu->ecap;
+	*length = sizeof(*vtd);
+
+	return vtd;
+}
+
 const struct iommu_ops intel_iommu_ops = {
 	.capable		= intel_iommu_capable,
+	.hw_info		= intel_iommu_hw_info,
 	.domain_alloc		= intel_iommu_domain_alloc,
 	.probe_device		= intel_iommu_probe_device,
 	.probe_finalize		= intel_iommu_probe_finalize,
@@ -4774,6 +4792,7 @@  const struct iommu_ops intel_iommu_ops = {
 	.def_domain_type	= device_def_domain_type,
 	.remove_dev_pasid	= intel_iommu_remove_dev_pasid,
 	.pgsize_bitmap		= SZ_4K,
+	.driver_type		= IOMMU_DEVICE_DATA_INTEL_VTD,
 #ifdef CONFIG_INTEL_IOMMU_SVM
 	.page_response		= intel_svm_page_response,
 #endif
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 06e61e474856..2e70265d4ceb 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -22,6 +22,7 @@ 
 #include <linux/ioasid.h>
 #include <linux/bitfield.h>
 #include <linux/xarray.h>
+#include <uapi/linux/iommufd.h>
 
 #include <asm/cacheflush.h>
 #include <asm/iommu.h>
diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
index 2309edb55028..fda75c8450ee 100644
--- a/include/uapi/linux/iommufd.h
+++ b/include/uapi/linux/iommufd.h
@@ -347,7 +347,28 @@  struct iommu_vfio_ioas {
 
 /**
  * enum iommu_device_data_type - IOMMU hardware Data types
+ * @IOMMU_DEVICE_DATA_INTEL_VTD: Intel VT-d iommu data type
  */
 enum iommu_device_data_type {
+	IOMMU_DEVICE_DATA_INTEL_VTD = 1,
+};
+
+/**
+ * struct iommu_device_info_vtd - Intel VT-d device info
+ *
+ * @flags: Must be set to 0
+ * @__reserved: Must be 0
+ * @cap_reg: Value of Intel VT-d capability register defined in chapter
+ *	     11.4.2 of Intel VT-d spec.
+ * @ecap_reg: Value of Intel VT-d capability register defined in chapter
+ *	     11.4.3 of Intel VT-d spec.
+ *
+ * Intel hardware iommu capability.
+ */
+struct iommu_device_info_vtd {
+	__u32 flags;
+	__u32 __reserved;
+	__aligned_u64 cap_reg;
+	__aligned_u64 ecap_reg;
 };
 #endif