[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240806174242.GP478300@nvidia.com>
Date: Tue, 6 Aug 2024 14:42:42 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: Nicolin Chen <nicolinc@...dia.com>
Cc: kevin.tian@...el.com, yi.l.liu@...el.com, iommu@...ts.linux.dev,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] iommufd/device: Enforce reserved IOVA also when
attached to hwpt_nested
On Mon, Aug 05, 2024 at 10:02:01PM -0700, Nicolin Chen wrote:
> Currently, device reserved regions are only enforced when the device is
> attached to an hwpt_paging. In other words, if the device gets attached
> to an hwpt_nested directly, the parent hwpt_paging of the hwpt_nested's
> would not enforce those reserved IOVAs. This works for most of reserved
> region types, but not for IOMMU_RESV_SW_MSI, which is a unique software
> defined window, required by a nesting case too to setup an MSI doorbell
> on the parent stage-2 hwpt/domain.
>
> Kevin pointed out that:
> 1) there is no usage using up closely the entire IOVA space yet,
> 2) guest may change the viommu mode to switch between nested
> and paging then VMM has to take all devices' reserved regions
> into consideration anyway, when composing the GPA space.
> Link: https://lore.kernel.org/all/BN9PR11MB5276497781C96415272E6FED8CB12@BN9PR11MB5276.namprd11.prod.outlook.com/
>
> So it would be actually convenient for us to also enforce reserved IOVA
> onto the parent hwpt_paging, when attaching a device to an hwpt_nested.
>
> Repurpose the existing attach/replace_paging helpers to attach device's
> reserved IOVAs exclusively. Allow a common hwpt input to support both a
> hwpt_paging type and a hwpt_nested type.
>
> Rework the to_hwpt_paging helper, which is only used by these reserved
> IOVA functions, to allow an IOMMUFD_OBJ_HWPT_NESTED hwpt to redirect to
> its parent hwpt_paging. And add another hwpt_to_ioas helper to get the
> IOAS pointer. Return a NULL in these two helpers for any potential new
> HWPT type, and make a NOP in those reserved IOVA functions accordingly.
>
> Suggested-by: Tian, Kevin <kevin.tian@...el.com>
> Signed-off-by: Nicolin Chen <nicolinc@...dia.com>
> ---
>
> Changelog
> v2:
> * Corrected the ioas comparisons for future hwpt type that returns
> NULL by the to_hwpt_paging helper.
> v1:
> https://lore.kernel.org/all/20240802053458.2754673-1-nicolinc@nvidia.com/
>
> drivers/iommu/iommufd/device.c | 77 ++++++++++++++-----------
> drivers/iommu/iommufd/iommufd_private.h | 15 ++++-
> 2 files changed, 56 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index 3214a4c17c6b3..949b69c9f3b2d 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -327,13 +327,18 @@ static int iommufd_group_setup_msi(struct iommufd_group *igroup,
> return 0;
> }
>
> -static int iommufd_hwpt_paging_attach(struct iommufd_hwpt_paging *hwpt_paging,
> - struct iommufd_device *idev)
> +static int
> +iommufd_device_attach_reserved_iova(struct iommufd_device *idev,
> + struct iommufd_hw_pagetable *hwpt)
> {
> + struct iommufd_hwpt_paging *hwpt_paging = to_hwpt_paging(hwpt);
> int rc;
This seems like the wrong place to put these types, a big point point of the
struct iommufd_hwpt_paging was to mark functions that should only be
operating on a paging hwpt.
The caller is expected to figure out what it is doing.
> int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
> struct iommufd_device *idev)
> {
> @@ -363,11 +380,9 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
> goto err_unlock;
> }
>
> - if (hwpt_is_paging(hwpt)) {
> - rc = iommufd_hwpt_paging_attach(to_hwpt_paging(hwpt), idev);
> - if (rc)
> - goto err_unlock;
> - }
Like we had here, so it would be a bit nicer to write it as more like:
hwpt_paging = to_hwpt_paging(hwpt);
if (hwpt_paging) {
rc = iommufd_hwpt_paging_attach(hwpt_paging, idev);
}
Then we can keep the clearer labeling of the function signatures.
> @@ -321,7 +321,20 @@ static inline bool hwpt_is_paging(struct iommufd_hw_pagetable *hwpt)
> static inline struct iommufd_hwpt_paging *
> to_hwpt_paging(struct iommufd_hw_pagetable *hwpt)
> {
> - return container_of(hwpt, struct iommufd_hwpt_paging, common);
> + switch (hwpt->obj.type) {
> + case IOMMUFD_OBJ_HWPT_PAGING:
> + return container_of(hwpt, struct iommufd_hwpt_paging, common);
> + case IOMMUFD_OBJ_HWPT_NESTED:
> + return container_of(hwpt, struct iommufd_hwpt_nested, common)->parent;
> + default:
> + return NULL;
> + }
> +}
There are alot of existing callers of this, I think it should get a
new function to do this behavior and it would only be used in a few
places.
It is not OK for a NESTING to get into most of the places that are
already calling this.
Jason
Powered by blists - more mailing lists