| Message ID | 20230128113532.94651-1-sven@svenpeter.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu/dart: Fix apple_dart_device_group for PCI groups | expand |
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 > >
On Sat, Jan 28, 2023 at 12:35:32PM +0100, Sven Peter 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> > --- > drivers/iommu/apple-dart.c | 51 ++++++++++++++++++++++++++++++++------ > 1 file changed, 44 insertions(+), 7 deletions(-) Applied, thanks.
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)
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> --- 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.