| Message ID | 20250210-locked-dart-v1-5-5d97fe247f35@rosenzweig.io (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iommu: apple-dart: Support locked DARTs | expand |
On 2025-02-10 7:39 pm, Alyssa Rosenzweig wrote: > Configuration is only possible and needed for non-locked DARTs and will > fail for locked DARTs. We cannot try -- assert that we do not. Except now we absolutely will - if a locked DART and its client device are advertised to Linux, instead of gracefully refusing to touch it, we'll now attach the client to a DMA domain, firing a barrage of multiple WARNs in the process, and give it DMA ops which still cannot work. I'm not really convinced this series on its own leaves us in a better position than we're already in now... :/ How hideous is the rest of what's required to actually make this usable? Thanks, Robin. > Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io> > --- > drivers/iommu/apple-dart.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c > index 29b627b38e8c37afd2b6a72865f43d24b633834a..87eb87bb2f5158d000a2c2fc801b722a2262c941 100644 > --- a/drivers/iommu/apple-dart.c > +++ b/drivers/iommu/apple-dart.c > @@ -309,6 +309,7 @@ apple_dart_hw_enable_translation(struct apple_dart_stream_map *stream_map) > struct apple_dart *dart = stream_map->dart; > int sid; > > + WARN_ON(stream_map->dart->locked); > for_each_set_bit(sid, stream_map->sidmap, dart->num_streams) > writel(dart->hw->tcr_enabled, dart->regs + DART_TCR(dart, sid)); > } > @@ -318,6 +319,7 @@ static void apple_dart_hw_disable_dma(struct apple_dart_stream_map *stream_map) > struct apple_dart *dart = stream_map->dart; > int sid; > > + WARN_ON(stream_map->dart->locked); > for_each_set_bit(sid, stream_map->sidmap, dart->num_streams) > writel(dart->hw->tcr_disabled, dart->regs + DART_TCR(dart, sid)); > } > @@ -328,7 +330,7 @@ apple_dart_hw_enable_bypass(struct apple_dart_stream_map *stream_map) > struct apple_dart *dart = stream_map->dart; > int sid; > > - WARN_ON(!stream_map->dart->supports_bypass); > + WARN_ON(stream_map->dart->locked || !stream_map->dart->supports_bypass); > for_each_set_bit(sid, stream_map->sidmap, dart->num_streams) > writel(dart->hw->tcr_bypass, > dart->regs + DART_TCR(dart, sid)); > @@ -340,6 +342,7 @@ static void apple_dart_hw_set_ttbr(struct apple_dart_stream_map *stream_map, > struct apple_dart *dart = stream_map->dart; > int sid; > > + WARN_ON(stream_map->dart->locked); > WARN_ON(paddr & ((1 << dart->hw->ttbr_shift) - 1)); > for_each_set_bit(sid, stream_map->sidmap, dart->num_streams) > writel(dart->hw->ttbr_valid | > @@ -353,6 +356,7 @@ static void apple_dart_hw_clear_ttbr(struct apple_dart_stream_map *stream_map, > struct apple_dart *dart = stream_map->dart; > int sid; > > + WARN_ON(stream_map->dart->locked); > for_each_set_bit(sid, stream_map->sidmap, dart->num_streams) > writel(0, dart->regs + DART_TTBR(dart, sid, idx)); > } >
> > Configuration is only possible and needed for non-locked DARTs and will > > fail for locked DARTs. We cannot try -- assert that we do not. > > Except now we absolutely will - if a locked DART and its client device are > advertised to Linux, instead of gracefully refusing to touch it, we'll now > attach the client to a DMA domain, firing a barrage of multiple WARNs in the > process, and give it DMA ops which still cannot work. I'm not really > convinced this series on its own leaves us in a better position than we're > already in now... :/ Fair point, thanks for raising that. "Fortunately" the upstream DTs don't describe any locked DARTs yet. > How hideous is the rest of what's required to actually make this usable? It isn't... pretty, and it's going to be ugly no matter how we slice it. Unfortunately the display controller DARTs really are locked so our hands are tied here. When I originally wrote these patches I had some hideous hack in the shared page table code. I'm thrilled to see that Janne rewrote that code to be local to apple-dart.c, at least: https://github.com/AsahiLinux/linux/commit/d90cc3590ea460e1c574b4b7c47fdafb2794af6a I'll include that patch with v2, which makes the locked DARTs actually usable, and restructure the series so we only probe after that commit is there.
On Tue, Feb 11, 2025 at 06:41:00PM +0000, Robin Murphy wrote: > On 2025-02-10 7:39 pm, Alyssa Rosenzweig wrote: > > Configuration is only possible and needed for non-locked DARTs and will > > fail for locked DARTs. We cannot try -- assert that we do not. > > Except now we absolutely will - if a locked DART and its client device > are advertised to Linux, instead of gracefully refusing to touch it, > we'll now attach the client to a DMA domain, firing a barrage of > multiple WARNs in the process, and give it DMA ops which still cannot > work. I'm not really convinced this series on its own leaves us in a > better position than we're already in now... :/ > > How hideous is the rest of what's required to actually make this usable? The TTBR can not be changed but the preset first level table can modified at will. The driver keeps a shadow first label table and syncs that to the preset 1st level table on flush_tbl(). It gets more complicated by the fact that the iommu for the display coprocessor is locked and mappings for its firmware and boot framebuffer are preinstalled and have to be maintained or restored on initialization. This is handled via reserved memory with translation. Downstream change to handle this is in https://github.com/AsahiLinux/linux/commit/d90cc3590ea460e1c574b4b7c47fdafb2794af6a not including the change to parse / handle reserved memory with translation in iommu core. Janne
On 2025-02-11 7:21 pm, Janne Grunau wrote: > On Tue, Feb 11, 2025 at 06:41:00PM +0000, Robin Murphy wrote: >> On 2025-02-10 7:39 pm, Alyssa Rosenzweig wrote: >>> Configuration is only possible and needed for non-locked DARTs and will >>> fail for locked DARTs. We cannot try -- assert that we do not. >> >> Except now we absolutely will - if a locked DART and its client device >> are advertised to Linux, instead of gracefully refusing to touch it, >> we'll now attach the client to a DMA domain, firing a barrage of >> multiple WARNs in the process, and give it DMA ops which still cannot >> work. I'm not really convinced this series on its own leaves us in a >> better position than we're already in now... :/ >> >> How hideous is the rest of what's required to actually make this usable? > > The TTBR can not be changed but the preset first level table can > modified at will. The driver keeps a shadow first label table and syncs > that to the preset 1st level table on flush_tbl(). > It gets more complicated by the fact that the iommu for the display > coprocessor is locked and mappings for its firmware and boot framebuffer > are preinstalled and have to be maintained or restored on > initialization. > This is handled via reserved memory with translation. > > Downstream change to handle this is in > https://github.com/AsahiLinux/linux/commit/d90cc3590ea460e1c574b4b7c47fdafb2794af6a > not including the change to parse / handle reserved memory with > translation in iommu core. Oh, if we handwave away the reserved region stuff for now, it doesn't seem *too* terrible, so definitely worth trying to land the bones of it along with this prep work, I reckon. From a quick look I think it might possibly be even cleaner as an io-pgtable quirk, to essentially skip allocating/freeing L1 and have some mechanism to fill in data->pgd with the remap afterwards (possible super cheeky version - also prepopulate cfg->apple_dart_cfg.ttbr and have alloc/free handle the remapping/unmapping themselves...). I'm not 100% sure off-hand, but since you avoid the DMA API and don't seem to have any other dependency on data->pgd having a linear map VA (other than the virt_to_phys() in the normal alloc path which you'd skip anyway), it feels like it ought to work out. I guess to support multiple domains you do still end up having to save/restore the L1 contents at the driver level when attaching, so some kind of shadow table notion isn't entirely unavoidable... oh well, it's a thought, at least. Thanks, Robin.
diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c index 29b627b38e8c37afd2b6a72865f43d24b633834a..87eb87bb2f5158d000a2c2fc801b722a2262c941 100644 --- a/drivers/iommu/apple-dart.c +++ b/drivers/iommu/apple-dart.c @@ -309,6 +309,7 @@ apple_dart_hw_enable_translation(struct apple_dart_stream_map *stream_map) struct apple_dart *dart = stream_map->dart; int sid; + WARN_ON(stream_map->dart->locked); for_each_set_bit(sid, stream_map->sidmap, dart->num_streams) writel(dart->hw->tcr_enabled, dart->regs + DART_TCR(dart, sid)); } @@ -318,6 +319,7 @@ static void apple_dart_hw_disable_dma(struct apple_dart_stream_map *stream_map) struct apple_dart *dart = stream_map->dart; int sid; + WARN_ON(stream_map->dart->locked); for_each_set_bit(sid, stream_map->sidmap, dart->num_streams) writel(dart->hw->tcr_disabled, dart->regs + DART_TCR(dart, sid)); } @@ -328,7 +330,7 @@ apple_dart_hw_enable_bypass(struct apple_dart_stream_map *stream_map) struct apple_dart *dart = stream_map->dart; int sid; - WARN_ON(!stream_map->dart->supports_bypass); + WARN_ON(stream_map->dart->locked || !stream_map->dart->supports_bypass); for_each_set_bit(sid, stream_map->sidmap, dart->num_streams) writel(dart->hw->tcr_bypass, dart->regs + DART_TCR(dart, sid)); @@ -340,6 +342,7 @@ static void apple_dart_hw_set_ttbr(struct apple_dart_stream_map *stream_map, struct apple_dart *dart = stream_map->dart; int sid; + WARN_ON(stream_map->dart->locked); WARN_ON(paddr & ((1 << dart->hw->ttbr_shift) - 1)); for_each_set_bit(sid, stream_map->sidmap, dart->num_streams) writel(dart->hw->ttbr_valid | @@ -353,6 +356,7 @@ static void apple_dart_hw_clear_ttbr(struct apple_dart_stream_map *stream_map, struct apple_dart *dart = stream_map->dart; int sid; + WARN_ON(stream_map->dart->locked); for_each_set_bit(sid, stream_map->sidmap, dart->num_streams) writel(0, dart->regs + DART_TTBR(dart, sid, idx)); }
Configuration is only possible and needed for non-locked DARTs and will fail for locked DARTs. We cannot try -- assert that we do not. Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io> --- drivers/iommu/apple-dart.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)