| Message ID | 20230816121349.104436-1-yi.l.liu@intel.com |
|---|---|
| Headers | show |
| Series | iommufd: Add iommu hardware info reporting | expand |
Looks like Yi's latest code has not addressed these comments. On Thu, Aug 17, 2023 at 07:31:42AM +0000, Tian, Kevin wrote: > > From: Liu, Yi L <yi.l.liu@intel.com> > > Sent: Wednesday, August 16, 2023 8:14 PM > > > > Under nested IOMMU translation, userspace owns the stage-1 translation > > table (e.g. the stage-1 page table of Intel VT-d or the context table of > > ARM SMMUv3, and etc.). Stage-1 translation tables are vendor specific, and > > need to be compatible with the underlying IOMMU hardware. Hence, > > userspace > > should know the IOMMU hardware capability before creating and > > configuring > > the stage-1 translation table to kernel. > > > > This adds IOMMU_GET_HW_INFO ioctl to query the IOMMU hardware > > information > > (a.k.a capability) for a given device. The returned data is vendor > > specific, userspace needs to decode it with the structure by the output > > @out_data_type field. > > "The format of the returned data is vendor specific and must be decoded > according to @out_data_type field". Ack. > > +int iommufd_get_hw_info(struct iommufd_ucmd *ucmd) > > +{ > > + struct iommu_hw_info *cmd = ucmd->cmd; > > + void __user *user_ptr = u64_to_user_ptr(cmd->data_uptr); > > + const struct iommu_ops *ops; > > + struct iommufd_device *idev; > > + unsigned int data_len; > > + unsigned int copy_len; > > + void *data = NULL; [..] > > + } else { > > + cmd->out_data_type = IOMMU_HW_INFO_TYPE_NONE; > > + data_len = 0; > > + data = NULL; > > data is already initialized as NULL. Will drop. > > + > > + /* > > + * We return the length the kernel supports so userspace may know > > what > > + * the kernel capability is. It could be larger than the input buffer. > > + */ > > + cmd->data_len = data_len; > > + > > + rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd)); > > +out: > > out_free: > > > + kfree(data); > > +err_put: > > out_put: (since this is also used in the success path) Ack for both. > > + * To capture an iommu type specific hardware information data, > > @data_uptr and > > + * its length @data_len must be provided. Trailing bytes will be zeroed if the > > + * user buffer is larger than the data that kernel has. Otherwise, kernel only > > + * fills the buffer using the given length in @data_len. If the ioctl succeeds, > > + * @data_len will be updated to the length that kernel actually supports, > > + * @out_data_type will be filled to decode the data filled in the buffer > > + * pointed by @data_uptr. Input @data_len == zero is allowed, no > > information > > + * data will be filled to user, but user space could get the > > iommu_hw_info_type > > + * filled in @out_data_type and the iommu hardware information data > > length > > + * supported by kernel filled in @data_len. > > I'd just keep "Input @data_len == zero is allowed" and remove all the > trailing words which just duplicate with the former context. Will do. > Reviewed-by: Kevin Tian <kevin.tian@intel.com> Adding this. Thanks Nic
On Thu, Aug 17, 2023 at 07:24:44AM +0000, Tian, Kevin wrote: > > > From: Liu, Yi L <yi.l.liu@intel.com> > > Sent: Wednesday, August 16, 2023 8:14 PM > > > > Different IOMMU hardware would have different hardware information. So > > the > > information reported differs as well. To let the external user understand > > the difference. enum iommu_hw_info_type is defined. For the iommu > > s/difference. enum/difference, enum/ > > > + * @hw_info: IOMMU hardware information. The type of the returned data > > is > > + * marked by the output type of this op. Type is one of > > + * enum iommu_hw_info_type defined in > > include/uapi/linux/iommufd.h. > > + * The drivers that support this op should define a unique type > > + * in include/uapi/linux/iommufd.h. The data buffer returned by this > > + * op is allocated in the IOMMU driver and the caller should free it > > + * after use. Return the data buffer if success, or ERR_PTR on > > + * failure. > > simplified as: > > @hw_info: report iommu hardware information. The data buffer returned by > this op is allocated in the iommu driver and freed by the caller > after use. The information type is one of enum iommu_hw_info_type > defined in include/uapi/linux/iommufd.h. > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> Done. Thanks!
> From: Nicolin Chen <nicolinc@nvidia.com> > Sent: Friday, August 18, 2023 5:08 AM > > Looks like Yi's latest code has not addressed these comments. Yeah. Not yet. In progress to incorporate them. 😊 > > On Thu, Aug 17, 2023 at 07:31:42AM +0000, Tian, Kevin wrote: > > > > From: Liu, Yi L <yi.l.liu@intel.com> > > > Sent: Wednesday, August 16, 2023 8:14 PM > > > > > > Under nested IOMMU translation, userspace owns the stage-1 translation > > > table (e.g. the stage-1 page table of Intel VT-d or the context table of > > > ARM SMMUv3, and etc.). Stage-1 translation tables are vendor specific, and > > > need to be compatible with the underlying IOMMU hardware. Hence, > > > userspace > > > should know the IOMMU hardware capability before creating and > > > configuring > > > the stage-1 translation table to kernel. > > > > > > This adds IOMMU_GET_HW_INFO ioctl to query the IOMMU hardware > > > information > > > (a.k.a capability) for a given device. The returned data is vendor > > > specific, userspace needs to decode it with the structure by the output > > > @out_data_type field. > > > > "The format of the returned data is vendor specific and must be decoded > > according to @out_data_type field". > > Ack. > > > > +int iommufd_get_hw_info(struct iommufd_ucmd *ucmd) > > > +{ > > > + struct iommu_hw_info *cmd = ucmd->cmd; > > > + void __user *user_ptr = u64_to_user_ptr(cmd->data_uptr); > > > + const struct iommu_ops *ops; > > > + struct iommufd_device *idev; > > > + unsigned int data_len; > > > + unsigned int copy_len; > > > + void *data = NULL; > [..] > > > + } else { > > > + cmd->out_data_type = IOMMU_HW_INFO_TYPE_NONE; > > > + data_len = 0; > > > + data = NULL; > > > > data is already initialized as NULL. Probably we can set data_len = 0 and the out_data_type to _NONE is the top as well. Any preference? > > Will drop. > > > > + > > > + /* > > > + * We return the length the kernel supports so userspace may know > > > what > > > + * the kernel capability is. It could be larger than the input buffer. > > > + */ > > > + cmd->data_len = data_len; > > > + > > > + rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd)); > > > +out: > > > > out_free: > > > > > + kfree(data); > > > +err_put: > > > > out_put: (since this is also used in the success path) > > Ack for both. > > > > + * To capture an iommu type specific hardware information data, > > > @data_uptr and > > > + * its length @data_len must be provided. Trailing bytes will be zeroed if the > > > + * user buffer is larger than the data that kernel has. Otherwise, kernel only > > > + * fills the buffer using the given length in @data_len. If the ioctl succeeds, > > > + * @data_len will be updated to the length that kernel actually supports, > > > + * @out_data_type will be filled to decode the data filled in the buffer > > > + * pointed by @data_uptr. Input @data_len == zero is allowed, no > > > information > > > + * data will be filled to user, but user space could get the > > > iommu_hw_info_type > > > + * filled in @out_data_type and the iommu hardware information data > > > length > > > + * supported by kernel filled in @data_len. > > > > I'd just keep "Input @data_len == zero is allowed" and remove all the > > trailing words which just duplicate with the former context. > > Will do. > > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > > Adding this. > > Thanks > Nic
On Fri, Aug 18, 2023 at 12:04:29AM +0000, Liu, Yi L wrote: > > > > +int iommufd_get_hw_info(struct iommufd_ucmd *ucmd) > > > > +{ > > > > + struct iommu_hw_info *cmd = ucmd->cmd; > > > > + void __user *user_ptr = u64_to_user_ptr(cmd->data_uptr); > > > > + const struct iommu_ops *ops; > > > > + struct iommufd_device *idev; > > > > + unsigned int data_len; > > > > + unsigned int copy_len; > > > > + void *data = NULL; > > [..] > > > > + } else { > > > > + cmd->out_data_type = IOMMU_HW_INFO_TYPE_NONE; > > > > + data_len = 0; > > > > + data = NULL; > > > > > > data is already initialized as NULL. > > Probably we can set data_len = 0 and the out_data_type to _NONE is > the top as well. Any preference? I think it is clear to remove the variable initialization so this branch is more explicit Jason
On Thu, Aug 17, 2023 at 05:21:43PM -0700, Nicolin Chen wrote: > On Thu, Aug 17, 2023 at 05:08:34PM -0700, Nicolin Chen wrote: > > On Fri, Aug 18, 2023 at 12:04:29AM +0000, Liu, Yi L wrote: > > > > > > From: Nicolin Chen <nicolinc@nvidia.com> > > > > Sent: Friday, August 18, 2023 5:08 AM > > > > > > > > Looks like Yi's latest code has not addressed these comments. > > > > > > Yeah. Not yet. In progress to incorporate them. 😊 > > > > I fixed them all in my local tree. I'm finalizing with alloc_user. > > Let me send my branch after that, and you can edit upon :) > > https://github.com/nicolinc/iommufd/tree/wip/iommufd_alloc_user-08172023-nic > Attached two sets of git-diff for the updates that I made to the > two series. Note that I didn't make any change to the VT-d patch. > } else { > cmd->out_data_type = IOMMU_HW_INFO_TYPE_NONE; > data_len = 0; > - data = NULL; Reverted this one and removed its initialization in my remote branch, following Jason's latest email. Thanks Nic