| Message ID | ec230c740b649bba1ca4d2ef054d90c79be4be28.1729555967.git.nicolinc@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommufd: Add vIOMMU infrastructure (Part-2: vDEVICE) | expand |
On Mon, Oct 21, 2024 at 05:20:10PM -0700, Nicolin Chen wrote: > struct iommufd_viommu_ops { > + struct iommufd_vdevice *(*vdevice_alloc)(struct iommufd_viommu *viommu, > + struct device *dev, u64 id); > + void (*vdevice_free)(struct iommufd_vdevice *vdev); ... > +#define iommufd_vdevice_alloc(ictx, drv_struct, member) \ > + ({ \ > + static_assert( \ > + __same_type(struct iommufd_vdevice, \ > + ((struct drv_struct *)NULL)->member)); \ > + static_assert(offsetof(struct drv_struct, member.obj) == 0); \ > + container_of(_iommufd_object_alloc(ictx, \ > + sizeof(struct drv_struct), \ > + IOMMUFD_OBJ_VDEVICE), \ > + struct drv_struct, member.obj); \ > + }) Per discussion in vIRQ series [1], we might not need to expose struct iommufd_vdevice. So, dropping most of the changes here, and moving iommufd_device to the private header. [1] https://lore.kernel.org/linux-iommu/ZxlGfgfwrGZGIbeF@Asurada-Nvidia/ Nicolin
On 22/10/24 11:20, Nicolin Chen wrote: > Introduce a new IOMMUFD_OBJ_VDEVICE to represent a physical device, i.e. > iommufd_device (idev) object, against an iommufd_viommu (vIOMMU) object in > the VM. This vDEVICE object (and its structure) holds all the information > and attributes in a VM, regarding the device related to the vIOMMU. > > As an initial patch, add a per-vIOMMU virtual ID. This can be: > - Virtual StreamID on a nested ARM SMMUv3, an index to a Stream Table > - Virtual DeviceID on a nested AMD IOMMU, an index to a Device Table > - Virtual ID on a nested Intel VT-D IOMMU, an index to a Context Table > Potentially, this vDEVICE structure can hold some vData for Confidential > Compute Architecture (CCA). > > Add a pair of vdevice_alloc and vdevice_free in struct iommufd_viommu_ops > to allow driver-level vDEVICE structure allocations. > > Similar to iommufd_viommu_alloc, add an iommufd_vdevice_alloc helper, so > IOMMU drivers can allocate core-embedded style structures. > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > include/linux/iommufd.h | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h > index 5c13c35952d8..5d61a1d2947a 100644 > --- a/include/linux/iommufd.h > +++ b/include/linux/iommufd.h > @@ -31,6 +31,7 @@ enum iommufd_object_type { > IOMMUFD_OBJ_ACCESS, > IOMMUFD_OBJ_FAULT, > IOMMUFD_OBJ_VIOMMU, > + IOMMUFD_OBJ_VDEVICE, > #ifdef CONFIG_IOMMUFD_TEST > IOMMUFD_OBJ_SELFTEST, > #endif > @@ -92,6 +93,14 @@ struct iommufd_viommu { > unsigned int type; > }; > > +struct iommufd_vdevice { > + struct iommufd_object obj; > + struct iommufd_ctx *ictx; > + struct iommufd_device *idev; > + struct iommufd_viommu *viommu; > + u64 id; /* per-vIOMMU virtual ID */ > +}; > + > /** > * struct iommufd_viommu_ops - vIOMMU specific operations > * @free: Free all driver-specific parts of an iommufd_viommu. The memory of the > @@ -101,12 +110,24 @@ struct iommufd_viommu { > * must be defined in include/uapi/linux/iommufd.h. > * It must fully initialize the new iommu_domain before > * returning. Upon failure, ERR_PTR must be returned. > + * @vdevice_alloc: Allocate a driver-managed iommufd_vdevice to init some driver > + * specific structure or HW procedure. Note that the core-level > + * structure is filled by the iommufd core after calling this op. > + * It is suggested to call iommufd_vdevice_alloc() helper for > + * a bundled allocation of the core and the driver structures, > + * using the ictx pointer in the given @viommu. > + * @vdevice_free: Free a driver-managed iommufd_vdevice to de-init its structure > + * or HW procedure. The memory of the vdevice will be free-ed by > + * iommufd core. > */ > struct iommufd_viommu_ops { > void (*free)(struct iommufd_viommu *viommu); > struct iommu_domain *(*domain_alloc_nested)( > struct iommufd_viommu *viommu, > const struct iommu_user_data *user_data); > + struct iommufd_vdevice *(*vdevice_alloc)(struct iommufd_viommu *viommu, > + struct device *dev, u64 id); > + void (*vdevice_free)(struct iommufd_vdevice *vdev); > }; > > #if IS_ENABLED(CONFIG_IOMMUFD) > @@ -200,4 +221,15 @@ _iommufd_object_alloc(struct iommufd_ctx *ictx, size_t size, > ret->member.ops = viommu_ops; \ > ret; \ > }) > +#define iommufd_vdevice_alloc(ictx, drv_struct, member) \ > + ({ \ > + static_assert( \ > + __same_type(struct iommufd_vdevice, \ > + ((struct drv_struct *)NULL)->member)); \ > + static_assert(offsetof(struct drv_struct, member.obj) == 0); \ > + container_of(_iommufd_object_alloc(ictx, \ > + sizeof(struct drv_struct), \ > + IOMMUFD_OBJ_VDEVICE), \ > + struct drv_struct, member.obj); \ > + }) > #endif A nit: it hurts eyes to read: mock_vdev = iommufd_vdevice_alloc(viommu->ictx, mock_vdevice, core); vs. mock_vdev = iommufd_vdevice_alloc(viommu->ictx, struct mock_vdevice, core); as for the former I go searching for a "mock_vdevice" variable and for the latter it is clear it is 1) a macro 2) which does some type checking. also, it makes it impossible to pass things like typeof(..) or a type from typedef. Thanks,
On Fri, Oct 25, 2024 at 06:53:01PM +1100, Alexey Kardashevskiy wrote: > > +#define iommufd_vdevice_alloc(ictx, drv_struct, member) \ > > + ({ \ > > + static_assert( \ > > + __same_type(struct iommufd_vdevice, \ > > + ((struct drv_struct *)NULL)->member)); \ > > + static_assert(offsetof(struct drv_struct, member.obj) == 0); \ > > + container_of(_iommufd_object_alloc(ictx, \ > > + sizeof(struct drv_struct), \ > > + IOMMUFD_OBJ_VDEVICE), \ > > + struct drv_struct, member.obj); \ > > + }) > > #endif > > A nit: it hurts eyes to read: > > mock_vdev = iommufd_vdevice_alloc(viommu->ictx, mock_vdevice, core); > > vs. > > mock_vdev = iommufd_vdevice_alloc(viommu->ictx, struct mock_vdevice, core); > > as for the former I go searching for a "mock_vdevice" variable and for the > latter it is clear it is 1) a macro 2) which does some type checking. > > also, it makes it impossible to pass things like typeof(..) or a type from > typedef. Thanks, Makes sense to me And the container_of() should not be used in these macros, the point was to avoid it to make the PTR_ERR behavior cleraer. Just put a force type cast Jason
On Fri, Oct 25, 2024 at 10:20:54AM -0300, Jason Gunthorpe wrote: > On Fri, Oct 25, 2024 at 06:53:01PM +1100, Alexey Kardashevskiy wrote: > > > +#define iommufd_vdevice_alloc(ictx, drv_struct, member) \ > > > + ({ \ > > > + static_assert( \ > > > + __same_type(struct iommufd_vdevice, \ > > > + ((struct drv_struct *)NULL)->member)); \ > > > + static_assert(offsetof(struct drv_struct, member.obj) == 0); \ > > > + container_of(_iommufd_object_alloc(ictx, \ > > > + sizeof(struct drv_struct), \ > > > + IOMMUFD_OBJ_VDEVICE), \ > > > + struct drv_struct, member.obj); \ > > > + }) > > > #endif > > > > A nit: it hurts eyes to read: > > > > mock_vdev = iommufd_vdevice_alloc(viommu->ictx, mock_vdevice, core); > > > > vs. > > > > mock_vdev = iommufd_vdevice_alloc(viommu->ictx, struct mock_vdevice, core); > > > > as for the former I go searching for a "mock_vdevice" variable and for the > > latter it is clear it is 1) a macro 2) which does some type checking. > > > > also, it makes it impossible to pass things like typeof(..) or a type from > > typedef. Thanks, > > Makes sense to me Ack. Will change accordingly. > And the container_of() should not be used in these macros, the point > was to avoid it to make the PTR_ERR behavior cleraer. Just put a force > type cast I recall that I changed it for a compiler complaint. But it seems to be gone now. Will change it back. Thanks Nicolin
diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h index 5c13c35952d8..5d61a1d2947a 100644 --- a/include/linux/iommufd.h +++ b/include/linux/iommufd.h @@ -31,6 +31,7 @@ enum iommufd_object_type { IOMMUFD_OBJ_ACCESS, IOMMUFD_OBJ_FAULT, IOMMUFD_OBJ_VIOMMU, + IOMMUFD_OBJ_VDEVICE, #ifdef CONFIG_IOMMUFD_TEST IOMMUFD_OBJ_SELFTEST, #endif @@ -92,6 +93,14 @@ struct iommufd_viommu { unsigned int type; }; +struct iommufd_vdevice { + struct iommufd_object obj; + struct iommufd_ctx *ictx; + struct iommufd_device *idev; + struct iommufd_viommu *viommu; + u64 id; /* per-vIOMMU virtual ID */ +}; + /** * struct iommufd_viommu_ops - vIOMMU specific operations * @free: Free all driver-specific parts of an iommufd_viommu. The memory of the @@ -101,12 +110,24 @@ struct iommufd_viommu { * must be defined in include/uapi/linux/iommufd.h. * It must fully initialize the new iommu_domain before * returning. Upon failure, ERR_PTR must be returned. + * @vdevice_alloc: Allocate a driver-managed iommufd_vdevice to init some driver + * specific structure or HW procedure. Note that the core-level + * structure is filled by the iommufd core after calling this op. + * It is suggested to call iommufd_vdevice_alloc() helper for + * a bundled allocation of the core and the driver structures, + * using the ictx pointer in the given @viommu. + * @vdevice_free: Free a driver-managed iommufd_vdevice to de-init its structure + * or HW procedure. The memory of the vdevice will be free-ed by + * iommufd core. */ struct iommufd_viommu_ops { void (*free)(struct iommufd_viommu *viommu); struct iommu_domain *(*domain_alloc_nested)( struct iommufd_viommu *viommu, const struct iommu_user_data *user_data); + struct iommufd_vdevice *(*vdevice_alloc)(struct iommufd_viommu *viommu, + struct device *dev, u64 id); + void (*vdevice_free)(struct iommufd_vdevice *vdev); }; #if IS_ENABLED(CONFIG_IOMMUFD) @@ -200,4 +221,15 @@ _iommufd_object_alloc(struct iommufd_ctx *ictx, size_t size, ret->member.ops = viommu_ops; \ ret; \ }) +#define iommufd_vdevice_alloc(ictx, drv_struct, member) \ + ({ \ + static_assert( \ + __same_type(struct iommufd_vdevice, \ + ((struct drv_struct *)NULL)->member)); \ + static_assert(offsetof(struct drv_struct, member.obj) == 0); \ + container_of(_iommufd_object_alloc(ictx, \ + sizeof(struct drv_struct), \ + IOMMUFD_OBJ_VDEVICE), \ + struct drv_struct, member.obj); \ + }) #endif
Introduce a new IOMMUFD_OBJ_VDEVICE to represent a physical device, i.e. iommufd_device (idev) object, against an iommufd_viommu (vIOMMU) object in the VM. This vDEVICE object (and its structure) holds all the information and attributes in a VM, regarding the device related to the vIOMMU. As an initial patch, add a per-vIOMMU virtual ID. This can be: - Virtual StreamID on a nested ARM SMMUv3, an index to a Stream Table - Virtual DeviceID on a nested AMD IOMMU, an index to a Device Table - Virtual ID on a nested Intel VT-D IOMMU, an index to a Context Table Potentially, this vDEVICE structure can hold some vData for Confidential Compute Architecture (CCA). Add a pair of vdevice_alloc and vdevice_free in struct iommufd_viommu_ops to allow driver-level vDEVICE structure allocations. Similar to iommufd_viommu_alloc, add an iommufd_vdevice_alloc helper, so IOMMU drivers can allocate core-embedded style structures. Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- include/linux/iommufd.h | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)