MIME-Version: 1.0
References: <20230128113532.94651-1-sven@svenpeter.dev>
In-Reply-To: <20230128113532.94651-1-sven@svenpeter.dev>
From:   Eric Curtin <ecurtin@redhat.com>
Date:   Sat, 28 Jan 2023 12:23:51 +0000
Message-ID: <CAOgh=Fw5FXPKde9PtTRRVeJ9eos8wqt72F_TLe_2XjWpKmCZ-A@mail.gmail.com>
Subject: Re: [PATCH] iommu/dart: Fix apple_dart_device_group for PCI groups
To:     Sven Peter <sven@svenpeter.dev>
Cc:     Hector Martin <marcan@marcan.st>, Joerg Roedel <joro@8bytes.org>,
        Will Deacon <will@kernel.org>,
        Alyssa Rosenzweig <alyssa@rosenzweig.io>,
        Robin Murphy <robin.murphy@arm.com>, asahi@lists.linux.dev,
        linux-arm-kernel@lists.infradead.org, iommu@lists.linux.dev,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, 28 Jan 2023 at 11:43, Sven Peter <sven@svenpeter.dev> wrote:
>
> pci_device_group() can return an already existing IOMMU group if the PCI
> device's pagetables have to be shared with another one due to bus
> toplogy, isolation features and/or DMA alias quirks.
> apple_dart_device_group() however assumes that the group has just been
> created and overwrites its iommudata which will eventually lead to
> apple_dart_release_group leaving stale entries in sid2group.
> Fix that by merging the iommudata if the returned group already exists.
>
> Fixes: f0b636804c7c ("iommu/dart: Clear sid2group entry when a group is freed")
> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> ---

Reviewed-by: Eric Curtin <ecurtin@redhat.com>

Is mise le meas/Regards,

Eric Curtin

>  drivers/iommu/apple-dart.c | 51 ++++++++++++++++++++++++++++++++------
>  1 file changed, 44 insertions(+), 7 deletions(-)
>
> This won't apply cleanly to iommu/fixes as it's based on the t8110 DART
> changes since the USB4/Thunderbolt DART itself also depends on those.
> That's not a big deal though since it's not possible to run into this
> bug without complex PCI bus topologies which can only be created using
> USB4/Thunderbolt on these SoCs and there's no support for that upstream
> yet.
>
> diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
> index a1304ba3639b..02f7a1740b14 100644
> --- a/drivers/iommu/apple-dart.c
> +++ b/drivers/iommu/apple-dart.c
> @@ -840,6 +840,29 @@ static void apple_dart_release_group(void *iommu_data)
>         mutex_unlock(&apple_dart_groups_lock);
>  }
>
> +static int apple_dart_merge_master_cfg(struct apple_dart_master_cfg *dst,
> +                                      struct apple_dart_master_cfg *src)
> +{
> +       /*
> +        * We know that this function is only called for groups returned from
> +        * pci_device_group and that all Apple Silicon platforms never spread
> +        * PCIe devices from the same bus across multiple DARTs such that we can
> +        * just assume that both src and dst only have the same single DART.
> +        */
> +       if (src->stream_maps[1].dart)
> +               return -EINVAL;
> +       if (dst->stream_maps[1].dart)
> +               return -EINVAL;
> +       if (src->stream_maps[0].dart != dst->stream_maps[0].dart)
> +               return -EINVAL;
> +
> +       bitmap_or(dst->stream_maps[0].sidmap,
> +                 dst->stream_maps[0].sidmap,
> +                 src->stream_maps[0].sidmap,
> +                 dst->stream_maps[0].dart->num_streams);
> +       return 0;
> +}
> +
>  static struct iommu_group *apple_dart_device_group(struct device *dev)
>  {
>         int i, sid;
> @@ -881,14 +904,28 @@ static struct iommu_group *apple_dart_device_group(struct device *dev)
>         if (!group)
>                 goto out;
>
> -       group_master_cfg = kmemdup(cfg, sizeof(*group_master_cfg), GFP_KERNEL);
> -       if (!group_master_cfg) {
> -               iommu_group_put(group);
> -               goto out;
> -       }
> +       group_master_cfg = iommu_group_get_iommudata(group);
> +       if (group_master_cfg) {
> +               int ret;
>
> -       iommu_group_set_iommudata(group, group_master_cfg,
> -               apple_dart_release_group);
> +               ret = apple_dart_merge_master_cfg(group_master_cfg, cfg);
> +               if (ret) {
> +                       dev_err(dev, "Failed to merge DART IOMMU grups.\n");
> +                       iommu_group_put(group);
> +                       res = ERR_PTR(ret);
> +                       goto out;
> +               }
> +       } else {
> +               group_master_cfg = kmemdup(cfg, sizeof(*group_master_cfg),
> +                                          GFP_KERNEL);
> +               if (!group_master_cfg) {
> +                       iommu_group_put(group);
> +                       goto out;
> +               }
> +
> +               iommu_group_set_iommudata(group, group_master_cfg,
> +                       apple_dart_release_group);
> +       }
>
>         for_each_stream_map(i, cfg, stream_map)
>                 for_each_set_bit(sid, stream_map->sidmap, stream_map->dart->num_streams)
> --
> 2.25.1
>
>