On Mon, Oct 28, 2024 at 07:52:10AM -0700, Nicolin Chen wrote:
On Mon, Oct 28, 2024 at 10:03:09AM -0300, Jason Gunthorpe wrote:
On Mon, Oct 28, 2024 at 11:24:10AM +0800, Zhangfei Gao wrote:
+/**
- iommufd_viommu_alloc_hwpt_nested() - Get a hwpt_nested for a vIOMMU
- @viommu: vIOMMU ojbect to associate the hwpt_nested/domain with
- @user_data: user_data pointer. Must be valid
- Allocate a new IOMMU_DOMAIN_NESTED for a vIOMMU and return it as a NESTED
- hw_pagetable.
- */
+static struct iommufd_hwpt_nested * +iommufd_viommu_alloc_hwpt_nested(struct iommufd_viommu *viommu, u32 flags,
const struct iommu_user_data *user_data)+{
struct iommufd_hwpt_nested *hwpt_nested;struct iommufd_hw_pagetable *hwpt;int rc;if (flags)return ERR_PTR(-EOPNOTSUPP);This check should be removed.
When a user page fault is required, IOMMU_HWPT_FAULT_ID_VALID is set. if (cmd->flags & IOMMU_HWPT_FAULT_ID_VALID) {
It can't just be removed..
I suspect that IOMMU_HWPT_FAULT_ID_VALID should not be set on the nested domain but on the parent?
By giving another look,
In iommufd_hwpt_paging_alloc(), we reject IOMMU_HWPT_FAULT_ID_VALID: const u32 valid_flags = IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING; ... if (flags & ~valid_flags) return ERR_PTR(-EOPNOTSUPP);
In iommufd_hwpt_nested_alloc(), we mask the flag away: if ((flags & ~IOMMU_HWPT_FAULT_ID_VALID) || !user_data->len || !ops->domain_alloc_user) return ERR_PTR(-EOPNOTSUPP); ... hwpt->domain = ops->domain_alloc_user(idev->dev, flags & ~IOMMU_HWPT_FAULT_ID_VALID, parent->common.domain, user_data);
Then, in the common function it has a section of if (cmd->flags & IOMMU_HWPT_FAULT_ID_VALID) { ...
It seems that this IOMMU_HWPT_FAULT_ID_VALID is for nested domains?
So, aligning with that, here we need: if ((flags & ~IOMMU_HWPT_FAULT_ID_VALID) || !user_data->len) return ERR_PTR(-EOPNOTSUPP);
I added a TEST_F for the coverage:
+TEST_F(iommufd_viommu, viommu_alloc_nested_iopf) +{ + struct iommu_hwpt_selftest data = { + .iotlb = IOMMU_TEST_IOTLB_DEFAULT, + }; + uint32_t viommu_id = self->viommu_id; + uint32_t dev_id = self->device_id; + uint32_t iopf_hwpt_id; + uint32_t fault_id; + uint32_t fault_fd; + + if (self->device_id) { + test_ioctl_fault_alloc(&fault_id, &fault_fd); + test_err_hwpt_alloc_iopf( + ENOENT, dev_id, viommu_id, UINT32_MAX, + IOMMU_HWPT_FAULT_ID_VALID, &iopf_hwpt_id, + IOMMU_HWPT_DATA_SELFTEST, &data, sizeof(data)); + test_err_hwpt_alloc_iopf( + EOPNOTSUPP, dev_id, viommu_id, fault_id, + IOMMU_HWPT_FAULT_ID_VALID | (1 << 31), &iopf_hwpt_id, + IOMMU_HWPT_DATA_SELFTEST, &data, sizeof(data)); + test_cmd_hwpt_alloc_iopf( + dev_id, viommu_id, fault_id, IOMMU_HWPT_FAULT_ID_VALID, + &iopf_hwpt_id, IOMMU_HWPT_DATA_SELFTEST, &data, + sizeof(data)); + + test_cmd_mock_domain_replace(self->stdev_id, iopf_hwpt_id); + EXPECT_ERRNO(EBUSY, + _test_ioctl_destroy(self->fd, iopf_hwpt_id)); + test_cmd_trigger_iopf(dev_id, fault_fd); + + test_cmd_mock_domain_replace(self->stdev_id, self->ioas_id); + test_ioctl_destroy(iopf_hwpt_id); + close(fault_fd); + test_ioctl_destroy(fault_id); + } +}
Thanks Nicolin