[SRU][Kinetic][PATCH 1/1] UBUNTU: SAUCE: (no-up) Add mdev_set_iommu_device() kABI.

[Tarun Gupta (SW-GPU)]

Hi Andrea,

Thanks for the review.

> I think it would be more clear to split the patch in 2: the revert + the patch with the additional changes, explaining what the additional changes are doing and why they're required.
> Otherwise, if the additional changes are required because we can't cleanly revert the commit, please mention that in the patch description, explaining a bit the required "adjustments".

I don't think a clean revert of the change would be possible as the vfio_iommu_type1 code has changed between current 5.19 HWE kernel and 5.16 kernel when the mdev_set_iommu_device() was removed.
So, I'll send out a v2 with detailed explanation in patch description about why clean revert is not possible.

> These 'Ubuntu-only' markers are not really adding any useful information,

Ok, will drop those comments.

> Moreover (minor nitpick), we're not using "(no-up)" anymore, so we can remove that from the subject.

Ok, will remove no-up from the subject.

Thanks,
Tarun

-----Original Message-----
From: Andrea Righi <andrea.righi at canonical.com> 
Sent: Tuesday, September 13, 2022 8:23 PM
To: Tarun Gupta (SW-GPU) <targupta at nvidia.com>
Cc: kernel-team at lists.ubuntu.com; ian.may at canonical.com; jose.ogando at canonical.com; billy.olsen at canonical.com; masoud.kamel at canonical.com
Subject: Re: [SRU][Kinetic][PATCH 1/1] UBUNTU: SAUCE: (no-up) Add mdev_set_iommu_device() kABI.


Hello Tarun,

some comments below.

On Tue, Sep 06, 2022 at 11:38:01PM +0530, Tarun Gupta wrote:
> With below commit present from 5.16+ upstream kernel onwards, support
> mdev_set_iommu_device() kABI has been removed from kernel due to lack 
> of in-tree vendor drivers using the kABI.
>
> fda49d97f2c4 ("vfio: remove the unused mdev iommu hook")
>
> This patch partially reverts the above commit so that
> mdev_set_iommu_device() kABI is still supported with HWE kernels for 
> Ubuntu 22.04.
>
> This kABI is used by SRIOV based Nvidia vGPU to pi all guest sysmem 
> during vGPU VM boot. With this patch, SRIOV based Nvidia vGPU will 
> continue to work with upstream kernels.
>
> BugLink : https://bugs.launchpad.net/bugs/1988806
>
> Signed-off-by: Tarun Gupta <targupta at nvidia.com>
> ---

First of all this patch seems to contain two different logical changes:
 1) the revert of fda49d97f2c4 ("vfio: remove the unused mdev iommu hook")
 2) some additional changes that are not very clear why they are needed

I think it would be more clear to split the patch in 2: the revert + the patch with the additional changes, explaining what the additional changes are doing and why they're required.

Otherwise, if the additional changes are required because we can't cleanly revert the commit, please mention that in the patch description, explaining a bit the required "adjustments".

> @@ -2180,6 +2257,25 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>       group->iommu_group = iommu_group;
>
>       if (type == VFIO_EMULATED_IOMMU) {
> +             /* Ubuntu-only - BEGIN */
> +             ret = iommu_group_for_each_dev(iommu_group, &bus, 
> + vfio_bus_type);
> +
> +             if (!ret && vfio_bus_is_mdev(bus)) {
> +                     struct device *iommu_device = NULL;
> +
> +                     group->mdev_group = true;
> +
> +                     /* Determine the isolation type */
> +                     ret = iommu_group_for_each_dev(iommu_group,
> +                                                    &iommu_device,
> +                                                    vfio_mdev_iommu_device);
> +                     if (!ret && iommu_device) {
> +                             bus = iommu_device->bus;
> +                             goto mdev_iommu_device;
> +                     }
> +             }
> +             /* Ubuntu-only - END */
> +

These 'Ubuntu-only' markers are not really adding any useful information, we can already determine that this code is Ubuntu only, because it's part of a SAUCE patch. I don't think there's any benefit duplicating this information in the code, so I would drop these comments.

Moreover (minor nitpick), we're not using "(no-up)" anymore, so we can remove that from the subject.

Thanks,
-Andrea