Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
On Wed, Jul 01, 2020 at 12:40:50AM -0700, Bjorn Andersson wrote: > On Wed 03 Jun 04:00 PDT 2020, Robin Murphy wrote: > > at that point I'm inclined to suggest we give up and stop trying to > > drive these things with arm-smmu. The XZR thing was bad enough, but if > > they're not even going to pretend to implement the architecture correctly > > then I'm not massively keen to continue tying the architectural driver in > > further knots if innocent things like CONFIG_IOMMU_DEFAULT_PASSTHROUGH are > > going to unexpectedly and catastrophically fail. We have qcom-iommu for > > hypervisor-mediated SMMUs, and this new hypervisor behaviour sounds to me > > more like "qcom-iommu++" with reassignable stream-to-context mappings, > > rather than a proper Arm SMMU emulation. > > > > I've been going through over and over, hoping to perhaps be able to > evolve qcom_iommu into a qcom-iommu++, but afaict the new hypervisor is > different enough that this isn't feasible. In particular, the platforms > using qcom_iommu relies entirely on the hypervisor to configure stream > mapping etc - and we can't even read most of the registers. > > On the other hand I agree with you that we're messing around quite a bit > with the arm-smmu driver, and I'm uncertain where we are on supporting > the various GPU features, so I'm adding Jordan to the thread. > > So, afaict we have the options of either shoehorning this too into the > arm-smmu driver or we essentially fork arm-smmu.c to create a > qcom-smmu.c. > > While I don't fancy the code duplication, it would allow us to revert > the Qualcomm quirks from arm-smmu and would unblock a number of > activities that we have depending on getting the SMMU enabled on various > platforms. We added the impl hooks to cater for implementation differences, so I'd still prefer to see this done as part of arm-smmu than introduce another almost-the-same-but-not-quite IOMMU driver that has the lifetime of a single SoC. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
On Wed 03 Jun 04:00 PDT 2020, Robin Murphy wrote: > On 2020-06-02 07:32, Bjorn Andersson wrote: > > On Wed 27 May 04:03 PDT 2020, Will Deacon wrote: > > > > > Hi John, Bjorn, > > > > > > On Tue, May 26, 2020 at 01:34:45PM -0700, John Stultz wrote: > > > > On Thu, May 14, 2020 at 12:34 PM wrote: > > > > > > > > > > On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote: > > > > > > > > > > Rob, Will, we're reaching the point where upstream has enough > > > > > functionality that this is becoming a critical issue for us. > > > > > > > > > > E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot > > > > > mainline with display, GPU, WiFi and audio working and the story is > > > > > similar on several devboards. > > > > > > > > > > As previously described, the only thing I want is the stream mapping > > > > > related to the display controller in place, either with the CB with > > > > > translation disabled or possibly with a way to specify the framebuffer > > > > > region (although this turns out to mess things up in the display > > > > > driver...) > > > > > > > > > > I did pick this up again recently and concluded that by omitting the > > > > > streams for the USB controllers causes an instability issue seen on > > > > > one > > > > > of the controller to disappear. So I would prefer if we somehow could > > > > > have a mechanism to only pick the display streams and the context > > > > > allocation for this. > > > > > > > > > > > > > > > Can you please share some pointers/insights/wishes for how we can > > > > > conclude on this subject? > > > > > > > > Ping? I just wanted to follow up on this discussion as this small > > > > series is crucial for booting mainline on the Dragonboard 845c > > > > devboard. It would be really valuable to be able to get some solution > > > > upstream so we can test mainline w/o adding additional patches. > > > > > > Sorry, it's been insanely busy recently and I haven't had a chance to > > > think > > > about this on top of everything else. We're also carrying a hack in > > > Android > > > for you :) > > > > > > > Thanks for taking the time to get back to us on this! > > > > > > The rest of the db845c series has been moving forward smoothly, but > > > > this set seems to be very stuck with no visible progress since Dec. > > > > > > > > Are there any pointers for what folks would prefer to see? > > > > > > I've had a chat with Robin about this. Originally, I was hoping that > > > people would all work together towards an idyllic future where firmware > > > would be able to describe arbitrary pre-existing mappings for devices, > > > irrespective of the IOMMU through which they master and Linux could > > > inherit this configuration. However, that hasn't materialised (there was > > > supposed to be an IORT update, but I don't know what happened to that) > > > and, in actual fact, the problem that you have on db845 is /far/ more > > > restricted than the general problem. > > > > > > Could you please try hacking something along the following lines and see > > > how you get on? You may need my for-joerg/arm-smmu/updates branch for > > > all the pieces: > > > > > >1. Use the ->cfg_probe() callback to reserve the SMR/S2CRs you need > > > "pinning" and configure for bypass. > > > > > >2. Use the ->def_domain_type() callback to return IOMMU_DOMAIN_IDENTITY > > > for the display controller > > > > > > I /think/ that's sufficient, but note that it differs from the current > > > approach because we don't end up reserving a CB -- bypass is configured > > > in the S2CR instead. Some invalidation might therefore be needed in > > > ->cfg_probe() after unhooking the CB. > > > > > > Thanks, and please yell if you run into problems with this approach. > > > > > > > This sounded straight forward and cleaner, so I implemented it... > > > > Unfortunately the hypervisor is playing tricks on me when writing to > > S2CR registers: > > - TRANS writes lands as requested > > - BYPASS writes ends up in the register as requested, with type FAULT > > - FAULT writes are ignored > > > > In other words, the Qualcomm firmware prevents us from relying on > > marking the relevant streams as BYPASS type. > > Sigh... I agree. > at that point I'm inclined to suggest we give up and stop trying to > drive these things with arm-smmu. The XZR thing was bad enough, but if > they're not even going to pretend to implement the architecture correctly > then I'm not massively keen to continue tying the architectural driver in > further knots if innocent things like CONFIG_IOMMU_DEFAULT_PASSTHROUGH are > going to unexpectedly and catastrophically fail. We have qcom-iommu for > hypervisor-mediated SMMUs, and this new hypervisor behaviour sounds to me > more like "qcom-iommu++" with reassignable stream-to-context mappings, > rather than a proper Arm SMMU emulation. > I've been going through over and over, hoping to perhaps be able to evolve qcom_iommu into a
Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
On Wed 03 Jun 03:24 PDT 2020, Thierry Reding wrote: > On Tue, Jun 02, 2020 at 12:32:49PM -0700, Bjorn Andersson wrote: > > On Tue 02 Jun 04:02 PDT 2020, Thierry Reding wrote: > > > > > On Wed, May 27, 2020 at 12:03:44PM +0100, Will Deacon wrote: > > > > Hi John, Bjorn, > > > > > > > > On Tue, May 26, 2020 at 01:34:45PM -0700, John Stultz wrote: > > > > > On Thu, May 14, 2020 at 12:34 PM wrote: > > > > > > > > > > > > On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote: > > > > > > > > > > > > Rob, Will, we're reaching the point where upstream has enough > > > > > > functionality that this is becoming a critical issue for us. > > > > > > > > > > > > E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot > > > > > > mainline with display, GPU, WiFi and audio working and the story is > > > > > > similar on several devboards. > > > > > > > > > > > > As previously described, the only thing I want is the stream mapping > > > > > > related to the display controller in place, either with the CB with > > > > > > translation disabled or possibly with a way to specify the > > > > > > framebuffer > > > > > > region (although this turns out to mess things up in the display > > > > > > driver...) > > > > > > > > > > > > I did pick this up again recently and concluded that by omitting the > > > > > > streams for the USB controllers causes an instability issue seen on > > > > > > one > > > > > > of the controller to disappear. So I would prefer if we somehow > > > > > > could > > > > > > have a mechanism to only pick the display streams and the context > > > > > > allocation for this. > > > > > > > > > > > > > > > > > > Can you please share some pointers/insights/wishes for how we can > > > > > > conclude on this subject? > > > > > > > > > > Ping? I just wanted to follow up on this discussion as this small > > > > > series is crucial for booting mainline on the Dragonboard 845c > > > > > devboard. It would be really valuable to be able to get some solution > > > > > upstream so we can test mainline w/o adding additional patches. > > > > > > > > Sorry, it's been insanely busy recently and I haven't had a chance to > > > > think > > > > about this on top of everything else. We're also carrying a hack in > > > > Android > > > > for you :) > > > > > > > > > The rest of the db845c series has been moving forward smoothly, but > > > > > this set seems to be very stuck with no visible progress since Dec. > > > > > > > > > > Are there any pointers for what folks would prefer to see? > > > > > > > > I've had a chat with Robin about this. Originally, I was hoping that > > > > people would all work together towards an idyllic future where firmware > > > > would be able to describe arbitrary pre-existing mappings for devices, > > > > irrespective of the IOMMU through which they master and Linux could > > > > inherit this configuration. However, that hasn't materialised (there was > > > > supposed to be an IORT update, but I don't know what happened to that) > > > > and, in actual fact, the problem that you have on db845 is /far/ more > > > > restricted than the general problem. > > > > > > It doesn't sound to me like implementing platform-specific workarounds > > > is a good long-term solution (especially since, according to Bjorn, they > > > aren't as trivial to implement as it sounds). And we already have all > > > the infrastructure in place to implement what you describe, so I don't > > > see why we shouldn't do that. This patchset uses standard device tree > > > bindings that were designed for exactly this kind of use-case. > > > > > > > I think my results would imply that we would have to end up with (at > > least) some special case of your proposal (i.e. we need a context bank > > allocated). > > I wasn't talking about implementation details, but rather about the > surrounding infrastructure. It seemed like Will was suggesting that > there's no way of conveying what memory regions to direct-map from > the firmware to the kernel. But that really isn't the problem here, > is it? What we're really looking for is how to take what we have in > device tree and use it in the ARM SMMU driver to create an early > mapping that will stay in place until a device has been properly > attached to the IOMMU domain. > I agree. We do have the iommu properties from our display node and we typically do have a reserved-memory region for the existing framebuffer that we want to associate with those properties. The one exception is on devices targeted to run Windows, where the memory region is passed to the kernel using UEFI GOP. But as you say, the information is available to the kernel already. > > > So at least for device-tree based boot firmware can already describe > > > these pre-existing mappings. If something standard materializes for ACPI > > > eventually I'm sure we can find ways to integrate that into whatever we > > > come up with now for DT. > > > > > > I think between Bjorn, John, Laurentiu and
Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
On Wed 03 Jun 04:11 PDT 2020, Will Deacon wrote: > On Mon, Jun 01, 2020 at 11:32:10PM -0700, Bjorn Andersson wrote: > > On Wed 27 May 04:03 PDT 2020, Will Deacon wrote: > > > > > Hi John, Bjorn, > > > > > > On Tue, May 26, 2020 at 01:34:45PM -0700, John Stultz wrote: > > > > On Thu, May 14, 2020 at 12:34 PM wrote: > > > > > > > > > > On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote: > > > > > > > > > > Rob, Will, we're reaching the point where upstream has enough > > > > > functionality that this is becoming a critical issue for us. > > > > > > > > > > E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot > > > > > mainline with display, GPU, WiFi and audio working and the story is > > > > > similar on several devboards. > > > > > > > > > > As previously described, the only thing I want is the stream mapping > > > > > related to the display controller in place, either with the CB with > > > > > translation disabled or possibly with a way to specify the framebuffer > > > > > region (although this turns out to mess things up in the display > > > > > driver...) > > > > > > > > > > I did pick this up again recently and concluded that by omitting the > > > > > streams for the USB controllers causes an instability issue seen on > > > > > one > > > > > of the controller to disappear. So I would prefer if we somehow could > > > > > have a mechanism to only pick the display streams and the context > > > > > allocation for this. > > > > > > > > > > > > > > > Can you please share some pointers/insights/wishes for how we can > > > > > conclude on this subject? > > > > > > > > Ping? I just wanted to follow up on this discussion as this small > > > > series is crucial for booting mainline on the Dragonboard 845c > > > > devboard. It would be really valuable to be able to get some solution > > > > upstream so we can test mainline w/o adding additional patches. > > > > > > Sorry, it's been insanely busy recently and I haven't had a chance to > > > think > > > about this on top of everything else. We're also carrying a hack in > > > Android > > > for you :) > > > > > > > Thanks for taking the time to get back to us on this! > > > > > > The rest of the db845c series has been moving forward smoothly, but > > > > this set seems to be very stuck with no visible progress since Dec. > > > > > > > > Are there any pointers for what folks would prefer to see? > > > > > > I've had a chat with Robin about this. Originally, I was hoping that > > > people would all work together towards an idyllic future where firmware > > > would be able to describe arbitrary pre-existing mappings for devices, > > > irrespective of the IOMMU through which they master and Linux could > > > inherit this configuration. However, that hasn't materialised (there was > > > supposed to be an IORT update, but I don't know what happened to that) > > > and, in actual fact, the problem that you have on db845 is /far/ more > > > restricted than the general problem. > > > > > > Could you please try hacking something along the following lines and see > > > how you get on? You may need my for-joerg/arm-smmu/updates branch for > > > all the pieces: > > > > > > 1. Use the ->cfg_probe() callback to reserve the SMR/S2CRs you need > > > "pinning" and configure for bypass. > > > > > > 2. Use the ->def_domain_type() callback to return IOMMU_DOMAIN_IDENTITY > > > for the display controller > > > > > > I /think/ that's sufficient, but note that it differs from the current > > > approach because we don't end up reserving a CB -- bypass is configured > > > in the S2CR instead. Some invalidation might therefore be needed in > > > ->cfg_probe() after unhooking the CB. > > > > > > Thanks, and please yell if you run into problems with this approach. > > > > > > > This sounded straight forward and cleaner, so I implemented it... > > > > Unfortunately the hypervisor is playing tricks on me when writing to > > S2CR registers: > > - TRANS writes lands as requested > > - BYPASS writes ends up in the register as requested, with type FAULT > > - FAULT writes are ignored > > > > In other words, the Qualcomm firmware prevents us from relying on > > marking the relevant streams as BYPASS type. > > Is this for all S2CR registers, or only the ones in use by the display > controller? Is there any scope for stopping the hypervisor from doing this? > This is for all S2CR registers. There's no chance to change this and get it deployed on all the devices people care about. As you know John need this for the RB3/db845c, where we might have a chance to modify the firmware. But I'm writing this on my Lenovo Yoga C630 and John and his colleagues are working on various phones, such as Pixel 3. None of these boots without this "workaround" and I don't expect that we can propagate a firmware modification to any of them - and definitely not all of them. > It makes it really difficult for the driver when the hardware is emulated > in a way that
Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
On Mon, Jun 01, 2020 at 11:32:10PM -0700, Bjorn Andersson wrote: > On Wed 27 May 04:03 PDT 2020, Will Deacon wrote: > > > Hi John, Bjorn, > > > > On Tue, May 26, 2020 at 01:34:45PM -0700, John Stultz wrote: > > > On Thu, May 14, 2020 at 12:34 PM wrote: > > > > > > > > On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote: > > > > > > > > Rob, Will, we're reaching the point where upstream has enough > > > > functionality that this is becoming a critical issue for us. > > > > > > > > E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot > > > > mainline with display, GPU, WiFi and audio working and the story is > > > > similar on several devboards. > > > > > > > > As previously described, the only thing I want is the stream mapping > > > > related to the display controller in place, either with the CB with > > > > translation disabled or possibly with a way to specify the framebuffer > > > > region (although this turns out to mess things up in the display > > > > driver...) > > > > > > > > I did pick this up again recently and concluded that by omitting the > > > > streams for the USB controllers causes an instability issue seen on one > > > > of the controller to disappear. So I would prefer if we somehow could > > > > have a mechanism to only pick the display streams and the context > > > > allocation for this. > > > > > > > > > > > > Can you please share some pointers/insights/wishes for how we can > > > > conclude on this subject? > > > > > > Ping? I just wanted to follow up on this discussion as this small > > > series is crucial for booting mainline on the Dragonboard 845c > > > devboard. It would be really valuable to be able to get some solution > > > upstream so we can test mainline w/o adding additional patches. > > > > Sorry, it's been insanely busy recently and I haven't had a chance to think > > about this on top of everything else. We're also carrying a hack in Android > > for you :) > > > > Thanks for taking the time to get back to us on this! > > > > The rest of the db845c series has been moving forward smoothly, but > > > this set seems to be very stuck with no visible progress since Dec. > > > > > > Are there any pointers for what folks would prefer to see? > > > > I've had a chat with Robin about this. Originally, I was hoping that > > people would all work together towards an idyllic future where firmware > > would be able to describe arbitrary pre-existing mappings for devices, > > irrespective of the IOMMU through which they master and Linux could > > inherit this configuration. However, that hasn't materialised (there was > > supposed to be an IORT update, but I don't know what happened to that) > > and, in actual fact, the problem that you have on db845 is /far/ more > > restricted than the general problem. > > > > Could you please try hacking something along the following lines and see > > how you get on? You may need my for-joerg/arm-smmu/updates branch for > > all the pieces: > > > > 1. Use the ->cfg_probe() callback to reserve the SMR/S2CRs you need > > "pinning" and configure for bypass. > > > > 2. Use the ->def_domain_type() callback to return IOMMU_DOMAIN_IDENTITY > > for the display controller > > > > I /think/ that's sufficient, but note that it differs from the current > > approach because we don't end up reserving a CB -- bypass is configured > > in the S2CR instead. Some invalidation might therefore be needed in > > ->cfg_probe() after unhooking the CB. > > > > Thanks, and please yell if you run into problems with this approach. > > > > This sounded straight forward and cleaner, so I implemented it... > > Unfortunately the hypervisor is playing tricks on me when writing to > S2CR registers: > - TRANS writes lands as requested > - BYPASS writes ends up in the register as requested, with type FAULT > - FAULT writes are ignored > > In other words, the Qualcomm firmware prevents us from relying on > marking the relevant streams as BYPASS type. Is this for all S2CR registers, or only the ones in use by the display controller? Is there any scope for stopping the hypervisor from doing this? It makes it really difficult for the driver when the hardware is emulated in a way that doesn't match the architecture... Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
On 2020-06-02 07:32, Bjorn Andersson wrote: On Wed 27 May 04:03 PDT 2020, Will Deacon wrote: Hi John, Bjorn, On Tue, May 26, 2020 at 01:34:45PM -0700, John Stultz wrote: On Thu, May 14, 2020 at 12:34 PM wrote: On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote: Rob, Will, we're reaching the point where upstream has enough functionality that this is becoming a critical issue for us. E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot mainline with display, GPU, WiFi and audio working and the story is similar on several devboards. As previously described, the only thing I want is the stream mapping related to the display controller in place, either with the CB with translation disabled or possibly with a way to specify the framebuffer region (although this turns out to mess things up in the display driver...) I did pick this up again recently and concluded that by omitting the streams for the USB controllers causes an instability issue seen on one of the controller to disappear. So I would prefer if we somehow could have a mechanism to only pick the display streams and the context allocation for this. Can you please share some pointers/insights/wishes for how we can conclude on this subject? Ping? I just wanted to follow up on this discussion as this small series is crucial for booting mainline on the Dragonboard 845c devboard. It would be really valuable to be able to get some solution upstream so we can test mainline w/o adding additional patches. Sorry, it's been insanely busy recently and I haven't had a chance to think about this on top of everything else. We're also carrying a hack in Android for you :) Thanks for taking the time to get back to us on this! The rest of the db845c series has been moving forward smoothly, but this set seems to be very stuck with no visible progress since Dec. Are there any pointers for what folks would prefer to see? I've had a chat with Robin about this. Originally, I was hoping that people would all work together towards an idyllic future where firmware would be able to describe arbitrary pre-existing mappings for devices, irrespective of the IOMMU through which they master and Linux could inherit this configuration. However, that hasn't materialised (there was supposed to be an IORT update, but I don't know what happened to that) and, in actual fact, the problem that you have on db845 is /far/ more restricted than the general problem. Could you please try hacking something along the following lines and see how you get on? You may need my for-joerg/arm-smmu/updates branch for all the pieces: 1. Use the ->cfg_probe() callback to reserve the SMR/S2CRs you need "pinning" and configure for bypass. 2. Use the ->def_domain_type() callback to return IOMMU_DOMAIN_IDENTITY for the display controller I /think/ that's sufficient, but note that it differs from the current approach because we don't end up reserving a CB -- bypass is configured in the S2CR instead. Some invalidation might therefore be needed in ->cfg_probe() after unhooking the CB. Thanks, and please yell if you run into problems with this approach. This sounded straight forward and cleaner, so I implemented it... Unfortunately the hypervisor is playing tricks on me when writing to S2CR registers: - TRANS writes lands as requested - BYPASS writes ends up in the register as requested, with type FAULT - FAULT writes are ignored In other words, the Qualcomm firmware prevents us from relying on marking the relevant streams as BYPASS type. Sigh... at that point I'm inclined to suggest we give up and stop trying to drive these things with arm-smmu. The XZR thing was bad enough, but if they're not even going to pretend to implement the architecture correctly then I'm not massively keen to continue tying the architectural driver in further knots if innocent things like CONFIG_IOMMU_DEFAULT_PASSTHROUGH are going to unexpectedly and catastrophically fail. We have qcom-iommu for hypervisor-mediated SMMUs, and this new hypervisor behaviour sounds to me more like "qcom-iommu++" with reassignable stream-to-context mappings, rather than a proper Arm SMMU emulation. Instead Qualcomm seems to implement "bypass" by setting up stream mapping, of TRANS type, pointing to a context bank without ARM_SMMU_SCTLR_M set. ...which arm-smmu specifically does not do because it's a silly waste of resources - typically context banks are even scarcer than S2CRs. Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
On Tue, Jun 02, 2020 at 12:32:49PM -0700, Bjorn Andersson wrote: > On Tue 02 Jun 04:02 PDT 2020, Thierry Reding wrote: > > > On Wed, May 27, 2020 at 12:03:44PM +0100, Will Deacon wrote: > > > Hi John, Bjorn, > > > > > > On Tue, May 26, 2020 at 01:34:45PM -0700, John Stultz wrote: > > > > On Thu, May 14, 2020 at 12:34 PM wrote: > > > > > > > > > > On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote: > > > > > > > > > > Rob, Will, we're reaching the point where upstream has enough > > > > > functionality that this is becoming a critical issue for us. > > > > > > > > > > E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot > > > > > mainline with display, GPU, WiFi and audio working and the story is > > > > > similar on several devboards. > > > > > > > > > > As previously described, the only thing I want is the stream mapping > > > > > related to the display controller in place, either with the CB with > > > > > translation disabled or possibly with a way to specify the framebuffer > > > > > region (although this turns out to mess things up in the display > > > > > driver...) > > > > > > > > > > I did pick this up again recently and concluded that by omitting the > > > > > streams for the USB controllers causes an instability issue seen on > > > > > one > > > > > of the controller to disappear. So I would prefer if we somehow could > > > > > have a mechanism to only pick the display streams and the context > > > > > allocation for this. > > > > > > > > > > > > > > > Can you please share some pointers/insights/wishes for how we can > > > > > conclude on this subject? > > > > > > > > Ping? I just wanted to follow up on this discussion as this small > > > > series is crucial for booting mainline on the Dragonboard 845c > > > > devboard. It would be really valuable to be able to get some solution > > > > upstream so we can test mainline w/o adding additional patches. > > > > > > Sorry, it's been insanely busy recently and I haven't had a chance to > > > think > > > about this on top of everything else. We're also carrying a hack in > > > Android > > > for you :) > > > > > > > The rest of the db845c series has been moving forward smoothly, but > > > > this set seems to be very stuck with no visible progress since Dec. > > > > > > > > Are there any pointers for what folks would prefer to see? > > > > > > I've had a chat with Robin about this. Originally, I was hoping that > > > people would all work together towards an idyllic future where firmware > > > would be able to describe arbitrary pre-existing mappings for devices, > > > irrespective of the IOMMU through which they master and Linux could > > > inherit this configuration. However, that hasn't materialised (there was > > > supposed to be an IORT update, but I don't know what happened to that) > > > and, in actual fact, the problem that you have on db845 is /far/ more > > > restricted than the general problem. > > > > It doesn't sound to me like implementing platform-specific workarounds > > is a good long-term solution (especially since, according to Bjorn, they > > aren't as trivial to implement as it sounds). And we already have all > > the infrastructure in place to implement what you describe, so I don't > > see why we shouldn't do that. This patchset uses standard device tree > > bindings that were designed for exactly this kind of use-case. > > > > I think my results would imply that we would have to end up with (at > least) some special case of your proposal (i.e. we need a context bank > allocated). I wasn't talking about implementation details, but rather about the surrounding infrastructure. It seemed like Will was suggesting that there's no way of conveying what memory regions to direct-map from the firmware to the kernel. But that really isn't the problem here, is it? What we're really looking for is how to take what we have in device tree and use it in the ARM SMMU driver to create an early mapping that will stay in place until a device has been properly attached to the IOMMU domain. > > So at least for device-tree based boot firmware can already describe > > these pre-existing mappings. If something standard materializes for ACPI > > eventually I'm sure we can find ways to integrate that into whatever we > > come up with now for DT. > > > > I think between Bjorn, John, Laurentiu and myself there's pretty broad > > consensus (correct me if I'm wrong, guys) that solving this via reserved > > memory regions is a good solution that works. So I think what's really > > missing is feedback on whether the changes proposed here or Laurentiu's > > updated proposal[0] are acceptable, and if not, what the preference is > > for getting something equivalent upstream. > > > > As described in my reply to your proposal, the one problem I ran into > was that I haven't figured out how to reliably "move" my display streams > from one mapping entry to another. > > With the current scheme I see that their will either be gaps in
Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
On Tue 02 Jun 04:02 PDT 2020, Thierry Reding wrote: > On Wed, May 27, 2020 at 12:03:44PM +0100, Will Deacon wrote: > > Hi John, Bjorn, > > > > On Tue, May 26, 2020 at 01:34:45PM -0700, John Stultz wrote: > > > On Thu, May 14, 2020 at 12:34 PM wrote: > > > > > > > > On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote: > > > > > > > > Rob, Will, we're reaching the point where upstream has enough > > > > functionality that this is becoming a critical issue for us. > > > > > > > > E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot > > > > mainline with display, GPU, WiFi and audio working and the story is > > > > similar on several devboards. > > > > > > > > As previously described, the only thing I want is the stream mapping > > > > related to the display controller in place, either with the CB with > > > > translation disabled or possibly with a way to specify the framebuffer > > > > region (although this turns out to mess things up in the display > > > > driver...) > > > > > > > > I did pick this up again recently and concluded that by omitting the > > > > streams for the USB controllers causes an instability issue seen on one > > > > of the controller to disappear. So I would prefer if we somehow could > > > > have a mechanism to only pick the display streams and the context > > > > allocation for this. > > > > > > > > > > > > Can you please share some pointers/insights/wishes for how we can > > > > conclude on this subject? > > > > > > Ping? I just wanted to follow up on this discussion as this small > > > series is crucial for booting mainline on the Dragonboard 845c > > > devboard. It would be really valuable to be able to get some solution > > > upstream so we can test mainline w/o adding additional patches. > > > > Sorry, it's been insanely busy recently and I haven't had a chance to think > > about this on top of everything else. We're also carrying a hack in Android > > for you :) > > > > > The rest of the db845c series has been moving forward smoothly, but > > > this set seems to be very stuck with no visible progress since Dec. > > > > > > Are there any pointers for what folks would prefer to see? > > > > I've had a chat with Robin about this. Originally, I was hoping that > > people would all work together towards an idyllic future where firmware > > would be able to describe arbitrary pre-existing mappings for devices, > > irrespective of the IOMMU through which they master and Linux could > > inherit this configuration. However, that hasn't materialised (there was > > supposed to be an IORT update, but I don't know what happened to that) > > and, in actual fact, the problem that you have on db845 is /far/ more > > restricted than the general problem. > > It doesn't sound to me like implementing platform-specific workarounds > is a good long-term solution (especially since, according to Bjorn, they > aren't as trivial to implement as it sounds). And we already have all > the infrastructure in place to implement what you describe, so I don't > see why we shouldn't do that. This patchset uses standard device tree > bindings that were designed for exactly this kind of use-case. > I think my results would imply that we would have to end up with (at least) some special case of your proposal (i.e. we need a context bank allocated). > So at least for device-tree based boot firmware can already describe > these pre-existing mappings. If something standard materializes for ACPI > eventually I'm sure we can find ways to integrate that into whatever we > come up with now for DT. > > I think between Bjorn, John, Laurentiu and myself there's pretty broad > consensus (correct me if I'm wrong, guys) that solving this via reserved > memory regions is a good solution that works. So I think what's really > missing is feedback on whether the changes proposed here or Laurentiu's > updated proposal[0] are acceptable, and if not, what the preference is > for getting something equivalent upstream. > As described in my reply to your proposal, the one problem I ran into was that I haven't figured out how to reliably "move" my display streams from one mapping entry to another. With the current scheme I see that their will either be gaps in time with no mapping for my display, or multiple mappings. The other thing I noticed in your proposal was that I have a whole bunch of DT nodes with both iommus and memory-region properties that I really don't care to set up mappings for, but I've not finalized my thoughts on this causing actual problems... > Just to highlight: the IOMMU framework already provides infrastructure > to create direct mappings (via iommu_get_resv_regions(), called from > iommu_create_device_direct_mappings()). I have patches that make use of > this on Tegra210 and earlier where a non-ARM SMMU is used and where the > IOMMU driver enables translations (and doesn't fault by default) only at > device attachment time. That works perfectly using reserved-memory > regions.
Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
On Wed, May 27, 2020 at 12:03:44PM +0100, Will Deacon wrote: > Hi John, Bjorn, > > On Tue, May 26, 2020 at 01:34:45PM -0700, John Stultz wrote: > > On Thu, May 14, 2020 at 12:34 PM wrote: > > > > > > On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote: > > > > > > Rob, Will, we're reaching the point where upstream has enough > > > functionality that this is becoming a critical issue for us. > > > > > > E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot > > > mainline with display, GPU, WiFi and audio working and the story is > > > similar on several devboards. > > > > > > As previously described, the only thing I want is the stream mapping > > > related to the display controller in place, either with the CB with > > > translation disabled or possibly with a way to specify the framebuffer > > > region (although this turns out to mess things up in the display > > > driver...) > > > > > > I did pick this up again recently and concluded that by omitting the > > > streams for the USB controllers causes an instability issue seen on one > > > of the controller to disappear. So I would prefer if we somehow could > > > have a mechanism to only pick the display streams and the context > > > allocation for this. > > > > > > > > > Can you please share some pointers/insights/wishes for how we can > > > conclude on this subject? > > > > Ping? I just wanted to follow up on this discussion as this small > > series is crucial for booting mainline on the Dragonboard 845c > > devboard. It would be really valuable to be able to get some solution > > upstream so we can test mainline w/o adding additional patches. > > Sorry, it's been insanely busy recently and I haven't had a chance to think > about this on top of everything else. We're also carrying a hack in Android > for you :) > > > The rest of the db845c series has been moving forward smoothly, but > > this set seems to be very stuck with no visible progress since Dec. > > > > Are there any pointers for what folks would prefer to see? > > I've had a chat with Robin about this. Originally, I was hoping that > people would all work together towards an idyllic future where firmware > would be able to describe arbitrary pre-existing mappings for devices, > irrespective of the IOMMU through which they master and Linux could > inherit this configuration. However, that hasn't materialised (there was > supposed to be an IORT update, but I don't know what happened to that) > and, in actual fact, the problem that you have on db845 is /far/ more > restricted than the general problem. It doesn't sound to me like implementing platform-specific workarounds is a good long-term solution (especially since, according to Bjorn, they aren't as trivial to implement as it sounds). And we already have all the infrastructure in place to implement what you describe, so I don't see why we shouldn't do that. This patchset uses standard device tree bindings that were designed for exactly this kind of use-case. So at least for device-tree based boot firmware can already describe these pre-existing mappings. If something standard materializes for ACPI eventually I'm sure we can find ways to integrate that into whatever we come up with now for DT. I think between Bjorn, John, Laurentiu and myself there's pretty broad consensus (correct me if I'm wrong, guys) that solving this via reserved memory regions is a good solution that works. So I think what's really missing is feedback on whether the changes proposed here or Laurentiu's updated proposal[0] are acceptable, and if not, what the preference is for getting something equivalent upstream. Just to highlight: the IOMMU framework already provides infrastructure to create direct mappings (via iommu_get_resv_regions(), called from iommu_create_device_direct_mappings()). I have patches that make use of this on Tegra210 and earlier where a non-ARM SMMU is used and where the IOMMU driver enables translations (and doesn't fault by default) only at device attachment time. That works perfectly using reserved-memory regions. Perhaps that infrastructure could be extended to cover the kinds of early mappings that we're discussing here. On the other hand it might be a bit premature at this point because the ARM SMMU is the only device that currently needs this, as far as I can tell. Thierry [0]: https://patchwork.ozlabs.org/project/linux-tegra/list/?series=164853 > Could you please try hacking something along the following lines and see > how you get on? You may need my for-joerg/arm-smmu/updates branch for > all the pieces: > > 1. Use the ->cfg_probe() callback to reserve the SMR/S2CRs you need > "pinning" and configure for bypass. > > 2. Use the ->def_domain_type() callback to return IOMMU_DOMAIN_IDENTITY > for the display controller > > I /think/ that's sufficient, but note that it differs from the current > approach because we don't end up reserving a CB -- bypass is configured > in the S2CR instead. Some
Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
On Wed 27 May 04:03 PDT 2020, Will Deacon wrote: > Hi John, Bjorn, > > On Tue, May 26, 2020 at 01:34:45PM -0700, John Stultz wrote: > > On Thu, May 14, 2020 at 12:34 PM wrote: > > > > > > On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote: > > > > > > Rob, Will, we're reaching the point where upstream has enough > > > functionality that this is becoming a critical issue for us. > > > > > > E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot > > > mainline with display, GPU, WiFi and audio working and the story is > > > similar on several devboards. > > > > > > As previously described, the only thing I want is the stream mapping > > > related to the display controller in place, either with the CB with > > > translation disabled or possibly with a way to specify the framebuffer > > > region (although this turns out to mess things up in the display > > > driver...) > > > > > > I did pick this up again recently and concluded that by omitting the > > > streams for the USB controllers causes an instability issue seen on one > > > of the controller to disappear. So I would prefer if we somehow could > > > have a mechanism to only pick the display streams and the context > > > allocation for this. > > > > > > > > > Can you please share some pointers/insights/wishes for how we can > > > conclude on this subject? > > > > Ping? I just wanted to follow up on this discussion as this small > > series is crucial for booting mainline on the Dragonboard 845c > > devboard. It would be really valuable to be able to get some solution > > upstream so we can test mainline w/o adding additional patches. > > Sorry, it's been insanely busy recently and I haven't had a chance to think > about this on top of everything else. We're also carrying a hack in Android > for you :) > Thanks for taking the time to get back to us on this! > > The rest of the db845c series has been moving forward smoothly, but > > this set seems to be very stuck with no visible progress since Dec. > > > > Are there any pointers for what folks would prefer to see? > > I've had a chat with Robin about this. Originally, I was hoping that > people would all work together towards an idyllic future where firmware > would be able to describe arbitrary pre-existing mappings for devices, > irrespective of the IOMMU through which they master and Linux could > inherit this configuration. However, that hasn't materialised (there was > supposed to be an IORT update, but I don't know what happened to that) > and, in actual fact, the problem that you have on db845 is /far/ more > restricted than the general problem. > > Could you please try hacking something along the following lines and see > how you get on? You may need my for-joerg/arm-smmu/updates branch for > all the pieces: > > 1. Use the ->cfg_probe() callback to reserve the SMR/S2CRs you need > "pinning" and configure for bypass. > > 2. Use the ->def_domain_type() callback to return IOMMU_DOMAIN_IDENTITY > for the display controller > > I /think/ that's sufficient, but note that it differs from the current > approach because we don't end up reserving a CB -- bypass is configured > in the S2CR instead. Some invalidation might therefore be needed in > ->cfg_probe() after unhooking the CB. > > Thanks, and please yell if you run into problems with this approach. > This sounded straight forward and cleaner, so I implemented it... Unfortunately the hypervisor is playing tricks on me when writing to S2CR registers: - TRANS writes lands as requested - BYPASS writes ends up in the register as requested, with type FAULT - FAULT writes are ignored In other words, the Qualcomm firmware prevents us from relying on marking the relevant streams as BYPASS type. Instead Qualcomm seems to implement "bypass" by setting up stream mapping, of TRANS type, pointing to a context bank without ARM_SMMU_SCTLR_M set. Regards, Bjorn ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
Hi John, Bjorn, On Tue, May 26, 2020 at 01:34:45PM -0700, John Stultz wrote: > On Thu, May 14, 2020 at 12:34 PM wrote: > > > > On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote: > > > > Rob, Will, we're reaching the point where upstream has enough > > functionality that this is becoming a critical issue for us. > > > > E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot > > mainline with display, GPU, WiFi and audio working and the story is > > similar on several devboards. > > > > As previously described, the only thing I want is the stream mapping > > related to the display controller in place, either with the CB with > > translation disabled or possibly with a way to specify the framebuffer > > region (although this turns out to mess things up in the display > > driver...) > > > > I did pick this up again recently and concluded that by omitting the > > streams for the USB controllers causes an instability issue seen on one > > of the controller to disappear. So I would prefer if we somehow could > > have a mechanism to only pick the display streams and the context > > allocation for this. > > > > > > Can you please share some pointers/insights/wishes for how we can > > conclude on this subject? > > Ping? I just wanted to follow up on this discussion as this small > series is crucial for booting mainline on the Dragonboard 845c > devboard. It would be really valuable to be able to get some solution > upstream so we can test mainline w/o adding additional patches. Sorry, it's been insanely busy recently and I haven't had a chance to think about this on top of everything else. We're also carrying a hack in Android for you :) > The rest of the db845c series has been moving forward smoothly, but > this set seems to be very stuck with no visible progress since Dec. > > Are there any pointers for what folks would prefer to see? I've had a chat with Robin about this. Originally, I was hoping that people would all work together towards an idyllic future where firmware would be able to describe arbitrary pre-existing mappings for devices, irrespective of the IOMMU through which they master and Linux could inherit this configuration. However, that hasn't materialised (there was supposed to be an IORT update, but I don't know what happened to that) and, in actual fact, the problem that you have on db845 is /far/ more restricted than the general problem. Could you please try hacking something along the following lines and see how you get on? You may need my for-joerg/arm-smmu/updates branch for all the pieces: 1. Use the ->cfg_probe() callback to reserve the SMR/S2CRs you need "pinning" and configure for bypass. 2. Use the ->def_domain_type() callback to return IOMMU_DOMAIN_IDENTITY for the display controller I /think/ that's sufficient, but note that it differs from the current approach because we don't end up reserving a CB -- bypass is configured in the S2CR instead. Some invalidation might therefore be needed in ->cfg_probe() after unhooking the CB. Thanks, and please yell if you run into problems with this approach. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
On 5/26/2020 11:34 PM, John Stultz wrote: > On Thu, May 14, 2020 at 12:34 PM wrote: >> >> On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote: >> >> Rob, Will, we're reaching the point where upstream has enough >> functionality that this is becoming a critical issue for us. >> >> E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot >> mainline with display, GPU, WiFi and audio working and the story is >> similar on several devboards. >> >> As previously described, the only thing I want is the stream mapping >> related to the display controller in place, either with the CB with >> translation disabled or possibly with a way to specify the framebuffer >> region (although this turns out to mess things up in the display >> driver...) >> >> I did pick this up again recently and concluded that by omitting the >> streams for the USB controllers causes an instability issue seen on one >> of the controller to disappear. So I would prefer if we somehow could >> have a mechanism to only pick the display streams and the context >> allocation for this. >> >> >> Can you please share some pointers/insights/wishes for how we can >> conclude on this subject? > > Ping? I just wanted to follow up on this discussion as this small > series is crucial for booting mainline on the Dragonboard 845c > devboard. It would be really valuable to be able to get some solution > upstream so we can test mainline w/o adding additional patches. +1 There are also some NXP chips that depend on this. Also, I've submitted a v2 [1] a while back that tries to address the feedback on the initial implementation. [1] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=164853 --- Best Regards, Laurentiu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
On Thu, May 14, 2020 at 12:34 PM wrote: > > On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote: > > Rob, Will, we're reaching the point where upstream has enough > functionality that this is becoming a critical issue for us. > > E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot > mainline with display, GPU, WiFi and audio working and the story is > similar on several devboards. > > As previously described, the only thing I want is the stream mapping > related to the display controller in place, either with the CB with > translation disabled or possibly with a way to specify the framebuffer > region (although this turns out to mess things up in the display > driver...) > > I did pick this up again recently and concluded that by omitting the > streams for the USB controllers causes an instability issue seen on one > of the controller to disappear. So I would prefer if we somehow could > have a mechanism to only pick the display streams and the context > allocation for this. > > > Can you please share some pointers/insights/wishes for how we can > conclude on this subject? Ping? I just wanted to follow up on this discussion as this small series is crucial for booting mainline on the Dragonboard 845c devboard. It would be really valuable to be able to get some solution upstream so we can test mainline w/o adding additional patches. The rest of the db845c series has been moving forward smoothly, but this set seems to be very stuck with no visible progress since Dec. Are there any pointers for what folks would prefer to see? thanks -john ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote: Rob, Will, we're reaching the point where upstream has enough functionality that this is becoming a critical issue for us. E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot mainline with display, GPU, WiFi and audio working and the story is similar on several devboards. As previously described, the only thing I want is the stream mapping related to the display controller in place, either with the CB with translation disabled or possibly with a way to specify the framebuffer region (although this turns out to mess things up in the display driver...) I did pick this up again recently and concluded that by omitting the streams for the USB controllers causes an instability issue seen on one of the controller to disappear. So I would prefer if we somehow could have a mechanism to only pick the display streams and the context allocation for this. Can you please share some pointers/insights/wishes for how we can conclude on this subject? PS. The list of devices specified https://lore.kernel.org/linux-arm-msm/cover.1587407458.git.saiprakash.ran...@codeaurora.org/ covers this need as well, if that matters... Thanks, Bjorn > On Mon 09 Dec 07:07 PST 2019, Thierry Reding wrote: > > > From: Thierry Reding > > > > Sorry for the slow response on this, finally got the time to go through > this in detail and try it out on some Qualcomm boards. > > > On some platforms, the firmware will setup hardware to read from a given > > region of memory. One such example is a display controller that is > > scanning out a splash screen from physical memory. > > > > This particular use case is the one that we need to figure out for > Qualcomm devices as well; on some devices it's a simple splash screen > (that on many devices can be disabled), but for others we have EFIFB > on the display and no (sane) means to disable this. > > > During Linux' boot process, the ARM SMMU will configure all contexts to > > fault by default. This means that memory accesses that happen by an SMMU > > master before its driver has had a chance to properly set up the IOMMU > > will cause a fault. This is especially annoying for something like the > > display controller scanning out a splash screen because the faults will > > result in the display controller getting bogus data (all-ones on Tegra) > > and since it repeatedly scans that framebuffer, it will keep triggering > > such faults and spam the boot log with them. > > > > As my proposed patches indicated, the Qualcomm platform boots with > stream mapping setup for the hardware used by the bootloader, but > relying on the associated context banks not being enabled. > > USFCFG in SCR0 is set and any faults resulting of this will trap into > secure world and the device will be reset. > > > In order to work around such problems, scan the device tree for IOMMU > > masters and set up a special identity domain that will map 1:1 all of > > the reserved regions associated with them. This happens before the SMMU > > is enabled, so that the mappings are already set up before translations > > begin. > > > > One thing that was pointed out earlier, and which I don't have a good > > idea on how to solve it, is that the early identity domain is not > > discarded. The assumption is that the standard direct mappings code of > > the IOMMU framework will replace the early identity domain once devices > > are properly attached to domains, but we don't have a good point in time > > when it would be safe to remove the early identity domain. > > > > One option that I can think of would be to create an early identity > > domain for each master and inherit it when that master is attached to > > the domain later on, but that seems rather complicated from an book- > > keeping point of view and tricky because we need to be careful not to > > map regions twice, etc. > > > > The one concern I ran into with this approach (after resolving below > issues) is that when the display driver probes a new domain will be > created automatically and I get a stream of "Unhandled context fault" in > the log until the driver has mapped the framebuffer in the newly > allocated context. > > This is normally not a problem, as we seem to be able to do this > initialization in a few frames, but for the cases where the display > driver probe defer this is a problem. > > But at least these devices doesn't reboot, so this is way better than the > current state. > > > Any good ideas on how to solve this? It'd also be interesting to see if > > there's a more generic way of doing this. I know that something like > > this isn't necessary on earlier Tegra SoCs with the custom Tegra SMMU > > because translations are only enabled when the devices are attached to a > > domain. I'm not sure about other IOMMUs, but in the absence of a struct > > device, I suspect that we can't really do anything really generic that > > would work across drivers. > > > > As I
Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
Hello, On 28.02.2020 04:57, Bjorn Andersson wrote: On Mon 09 Dec 07:07 PST 2019, Thierry Reding wrote: From: Thierry Reding Sorry for the slow response on this, finally got the time to go through this in detail and try it out on some Qualcomm boards. On some platforms, the firmware will setup hardware to read from a given region of memory. One such example is a display controller that is scanning out a splash screen from physical memory. This particular use case is the one that we need to figure out for Qualcomm devices as well; on some devices it's a simple splash screen (that on many devices can be disabled), but for others we have EFIFB on the display and no (sane) means to disable this. During Linux' boot process, the ARM SMMU will configure all contexts to fault by default. This means that memory accesses that happen by an SMMU master before its driver has had a chance to properly set up the IOMMU will cause a fault. This is especially annoying for something like the display controller scanning out a splash screen because the faults will result in the display controller getting bogus data (all-ones on Tegra) and since it repeatedly scans that framebuffer, it will keep triggering such faults and spam the boot log with them. As my proposed patches indicated, the Qualcomm platform boots with stream mapping setup for the hardware used by the bootloader, but relying on the associated context banks not being enabled. USFCFG in SCR0 is set and any faults resulting of this will trap into secure world and the device will be reset. In order to work around such problems, scan the device tree for IOMMU masters and set up a special identity domain that will map 1:1 all of the reserved regions associated with them. This happens before the SMMU is enabled, so that the mappings are already set up before translations begin. One thing that was pointed out earlier, and which I don't have a good idea on how to solve it, is that the early identity domain is not discarded. The assumption is that the standard direct mappings code of the IOMMU framework will replace the early identity domain once devices are properly attached to domains, but we don't have a good point in time when it would be safe to remove the early identity domain. One option that I can think of would be to create an early identity domain for each master and inherit it when that master is attached to the domain later on, but that seems rather complicated from an book- keeping point of view and tricky because we need to be careful not to map regions twice, etc. The one concern I ran into with this approach (after resolving below issues) is that when the display driver probes a new domain will be created automatically and I get a stream of "Unhandled context fault" in the log until the driver has mapped the framebuffer in the newly allocated context. This is normally not a problem, as we seem to be able to do this initialization in a few frames, but for the cases where the display driver probe defer this is a problem. Also gave this a go on one of NXP's layerscape platforms, and encountered the same issue. However, given that in our case it's not about a framebuffer device but a firmware, it cause it to crash. :-( Another apparent problem is that in the current implementation only one memory-region per device is supported. Actually it appears that this is a limitation of the DT reservation binding - it doesn't seem to allow specifying multiple regions per device. In our firmware case we would need support for multiple reserved regions (FW memory, FW i/o registers a.s.o). --- Best Regards, Laurentiu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
On Mon 09 Dec 07:07 PST 2019, Thierry Reding wrote: > From: Thierry Reding > Sorry for the slow response on this, finally got the time to go through this in detail and try it out on some Qualcomm boards. > On some platforms, the firmware will setup hardware to read from a given > region of memory. One such example is a display controller that is > scanning out a splash screen from physical memory. > This particular use case is the one that we need to figure out for Qualcomm devices as well; on some devices it's a simple splash screen (that on many devices can be disabled), but for others we have EFIFB on the display and no (sane) means to disable this. > During Linux' boot process, the ARM SMMU will configure all contexts to > fault by default. This means that memory accesses that happen by an SMMU > master before its driver has had a chance to properly set up the IOMMU > will cause a fault. This is especially annoying for something like the > display controller scanning out a splash screen because the faults will > result in the display controller getting bogus data (all-ones on Tegra) > and since it repeatedly scans that framebuffer, it will keep triggering > such faults and spam the boot log with them. > As my proposed patches indicated, the Qualcomm platform boots with stream mapping setup for the hardware used by the bootloader, but relying on the associated context banks not being enabled. USFCFG in SCR0 is set and any faults resulting of this will trap into secure world and the device will be reset. > In order to work around such problems, scan the device tree for IOMMU > masters and set up a special identity domain that will map 1:1 all of > the reserved regions associated with them. This happens before the SMMU > is enabled, so that the mappings are already set up before translations > begin. > > One thing that was pointed out earlier, and which I don't have a good > idea on how to solve it, is that the early identity domain is not > discarded. The assumption is that the standard direct mappings code of > the IOMMU framework will replace the early identity domain once devices > are properly attached to domains, but we don't have a good point in time > when it would be safe to remove the early identity domain. > > One option that I can think of would be to create an early identity > domain for each master and inherit it when that master is attached to > the domain later on, but that seems rather complicated from an book- > keeping point of view and tricky because we need to be careful not to > map regions twice, etc. > The one concern I ran into with this approach (after resolving below issues) is that when the display driver probes a new domain will be created automatically and I get a stream of "Unhandled context fault" in the log until the driver has mapped the framebuffer in the newly allocated context. This is normally not a problem, as we seem to be able to do this initialization in a few frames, but for the cases where the display driver probe defer this is a problem. But at least these devices doesn't reboot, so this is way better than the current state. > Any good ideas on how to solve this? It'd also be interesting to see if > there's a more generic way of doing this. I know that something like > this isn't necessary on earlier Tegra SoCs with the custom Tegra SMMU > because translations are only enabled when the devices are attached to a > domain. I'm not sure about other IOMMUs, but in the absence of a struct > device, I suspect that we can't really do anything really generic that > would work across drivers. > As I indicated above I managed to get this working on the boards we have that uses the arm-smmu driver. ## SDM845 Booting the SDM845 shows the following register stream mapping register content: SMR(0): 0x80080880 S2CR(0): 0x0 SMR(1): 0x80080c80 S2CR(1): 0x0 SMR(2): 0x800f00a0 S2CR(2): 0x1 SMR(3): 0x800f00c0 S2CR(3): 0x1 SMR(4): 0x800f00e0 S2CR(4): 0x2 SMR(5): 0x800f0100 S2CR(5): 0x2 SMR(6): 0x0 S2CR(6): 0x0 SMR(7): 0x0 S2CR(7): 0x0 SMR(8): 0x0 S2CR(8): 0x200ff SMR(9): 0x0 S2CR(9): 0x200ff ... Here stream 0 and 1 (SID 0x880 and 0xc80) are the display streams, the remainder are related to storage and USB - which afaict doesn't need to be maintained. As the display uses context bank 0, using this as the identity bank results in a couple of occurrences of: Unhandled context fault: fsr=0x402, iova=0x9da0, fsynr=0x370020, cbfrsynra=0x880, cb=0 Which we survive, but as we reach arm_smmu_device_reset() to flush out the new stream mapping we start by writing S2CR(0) = 0, then SMR(0) = 0x800810a0. So until SMR(4) is written we're lacking a valid stream mapping for the display, and hence if the screen does refresh in during time period the device reboots. In addition to this, the iommu_iova_to_phys() you perform in the mapping loop results in a large number of "translation fault!" printouts from arm_smmu_iova_to_phys_hard(). ##
Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
On Mon 13 Jan 14:01 PST 2020, Saravana Kannan wrote: > I added everyone from the other thread, but somehow managed to miss > the Bjorn who sent the emails! Fixing that now. > Thanks for looping me in Saravana. > On Mon, Jan 13, 2020 at 6:07 AM Thierry Reding > wrote: > > > > On Fri, Jan 10, 2020 at 08:56:39PM -0800, Saravana Kannan wrote: [..] > > In the case where you're trying to inherit the bootloader configuration > > of the SMMU, how do you solve the problem of passing the page tables to > > the kernel? You must have some way of reserving that memory in order to > > prevent the kernel from reusing it. > > Maybe "inherit" makes it sound a lot more complicated than it is? > Bjorn will know the details of what the bootloader sets up. But > AFAICT, it looks like a simple "bypass"/identity mapping too. I don't > think it actually sets up page tables. > In the Qualcomm case we have a number of stream mappings installed when the bootloader jumps to the OS, each one with SMR/S2CR referring to a CB with SMMU_CBn_SCTLR.M not set. As such the relevant hardware is able to access (without translation) DDR even with SMMU_CR0.USFCFG being set. The one case where this causes issues is that upon attaching a device to a context we'll set SMMU_CBn_SCTLR.M, so until we actually have a translation installed we do get faults - the difference is that these are not picked up as fatal faults by the secure firmware, so they are simply reported in Linux. [..] > > > > One option that I can think of would be to create an early identity > > > > domain for each master and inherit it when that master is attached to > > > > the domain later on, but that seems rather complicated from an book- > > > > keeping point of view and tricky because we need to be careful not to > > > > map regions twice, etc. > > > > > > > > Any good ideas on how to solve this? It'd also be interesting to see if > > > > there's a more generic way of doing this. I know that something like > > > > this isn't necessary on earlier Tegra SoCs with the custom Tegra SMMU > > > > because translations are only enabled when the devices are attached to a > > > > domain. > > > > > > Good foresight. As [1] shows, identity mapping doesn't solve it in a > > > generic way. > > > > I think your [1] is a special case of identity mappings where the > > mappings are already active. If you pass the information about the > > mappings via memory-region properties, then you should be able to > > reconstruct the identity mapping in the kernel before switching the > > SMMU over to the new mapping for a seamless transition. > > Ok, makes sense. But I don't have the full details here. So I'll let > Bjorn comment here. > It might be possible that we can install page tables and setup 1:1 mappings for the necessary resources, but it's not all devices with a memory-region and a iommu context defined that should have this. I will have to discuss this in more detail with the Qualcomm engineers. PS. One detail that I did notice while distilling the downstream patches is that unused mappings must have SMMU_S2CRx.CBNDX = 255 or I get odd crashes when the display (CBNDX = 0) is active. I've yet to conclude why this is. Regards, Bjorn ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
I added everyone from the other thread, but somehow managed to miss the Bjorn who sent the emails! Fixing that now. On Mon, Jan 13, 2020 at 6:07 AM Thierry Reding wrote: > > On Fri, Jan 10, 2020 at 08:56:39PM -0800, Saravana Kannan wrote: > > Hi Thierry, > > > > I happened upon this thread while looking into another thread [1]. > > > > > From: Thierry Reding > > > > > > On some platforms, the firmware will setup hardware to read from a given > > > region of memory. One such example is a display controller that is > > > scanning out a splash screen from physical memory. > > > > > > During Linux' boot process, the ARM SMMU will configure all contexts to > > > fault by default. This means that memory accesses that happen by an SMMU > > > master before its driver has had a chance to properly set up the IOMMU > > > will cause a fault. This is especially annoying for something like the > > > display controller scanning out a splash screen because the faults will > > > result in the display controller getting bogus data (all-ones on Tegra) > > > and since it repeatedly scans that framebuffer, it will keep triggering > > > such faults and spam the boot log with them. > > > > While I'm not an expert on IOMMUs, I have a decent high level > > understanding of the problem you are trying to solve. > > > > > In order to work around such problems, scan the device tree for IOMMU > > > masters and set up a special identity domain that will map 1:1 all of > > > the reserved regions associated with them. This happens before the SMMU > > > is enabled, so that the mappings are already set up before translations > > > begin. > > > > I'm not sure if this RFC will solve the splash screen issue across SoCs > > ([1] seems to have a different issue and might not have memory-regions). > > Looking at the proposed patches, they look like they're solving a > different, although related, problem. In your case you seem to have a > bootloader that already sets up the SMMU to translate for a given > master. The case that I'm trying to solve here is where the bootloader > has not yet setup the SMMU but has instead pointed some device to read > memory from a physical address. Ah, thanks for explaining your scenario. > So what this patch is trying to solve is to create the mappings that a > given device needs in order to transparently keep scanning out from an > address region that it's using, even when the kernel enables address > translation. Ok, makes sense. > In the case where you're trying to inherit the bootloader configuration > of the SMMU, how do you solve the problem of passing the page tables to > the kernel? You must have some way of reserving that memory in order to > prevent the kernel from reusing it. Maybe "inherit" makes it sound a lot more complicated than it is? Bjorn will know the details of what the bootloader sets up. But AFAICT, it looks like a simple "bypass"/identity mapping too. I don't think it actually sets up page tables. > > > One thing that was pointed out earlier, and which I don't have a good > > > idea on how to solve it, is that the early identity domain is not > > > discarded. The assumption is that the standard direct mappings code of > > > the IOMMU framework will replace the early identity domain once devices > > > are properly attached to domains, but we don't have a good point in time > > > when it would be safe to remove the early identity domain. > > > > You are in luck! I added sync_state() driver callbacks [2] exactly for > > cases like this. Heck, I even listed IOMMUs as an example use case. :) > > sync_state() works even with modules if one enables of_devlink [3] kernel > > parameter (which already supports iommus DT bindings). I'd be happy to > > answer any question you have on sync_state() and of_devlink. > > I wasn't aware of of_devlink, but I like it! It does have the drawback > that you need to reimplement a lot of the (phandle, specifier) parsing > code, but I don't think anybody was ever able to solve anyway. > > Looking at struct supplier_bindings, I think it might be possible to > share the property parsing code with the subsystems, though. But I > digress... Yeah, I don't want to digress either. But as of now, iommus are already supported. > Regarding sync_state(), I'm not sure it would be useful in my case. One > of the drivers I'm dealing with, for example, is a composite driver that > is created by tying together multiple devices. If you can give additional details about this, I can give a better answer. But with the limited info, there's one way I can think of to handle this. To make the explanation easier, let's call the device that references the IOMMU in DT as the "direct consumer device". The driver __probe function__ for the direct consumer device can add a device link from the composite device to the iommu device. This will ensure the iommu device doesn't get the sync_state() callback before the composite device probes. Keep in mind that devices do not need to be
Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
Hi Thierry, I happened upon this thread while looking into another thread [1]. > From: Thierry Reding > > On some platforms, the firmware will setup hardware to read from a given > region of memory. One such example is a display controller that is > scanning out a splash screen from physical memory. > > During Linux' boot process, the ARM SMMU will configure all contexts to > fault by default. This means that memory accesses that happen by an SMMU > master before its driver has had a chance to properly set up the IOMMU > will cause a fault. This is especially annoying for something like the > display controller scanning out a splash screen because the faults will > result in the display controller getting bogus data (all-ones on Tegra) > and since it repeatedly scans that framebuffer, it will keep triggering > such faults and spam the boot log with them. While I'm not an expert on IOMMUs, I have a decent high level understanding of the problem you are trying to solve. > In order to work around such problems, scan the device tree for IOMMU > masters and set up a special identity domain that will map 1:1 all of > the reserved regions associated with them. This happens before the SMMU > is enabled, so that the mappings are already set up before translations > begin. I'm not sure if this RFC will solve the splash screen issue across SoCs ([1] seems to have a different issue and might not have memory-regions). > One thing that was pointed out earlier, and which I don't have a good > idea on how to solve it, is that the early identity domain is not > discarded. The assumption is that the standard direct mappings code of > the IOMMU framework will replace the early identity domain once devices > are properly attached to domains, but we don't have a good point in time > when it would be safe to remove the early identity domain. You are in luck! I added sync_state() driver callbacks [2] exactly for cases like this. Heck, I even listed IOMMUs as an example use case. :) sync_state() works even with modules if one enables of_devlink [3] kernel parameter (which already supports iommus DT bindings). I'd be happy to answer any question you have on sync_state() and of_devlink. > One option that I can think of would be to create an early identity > domain for each master and inherit it when that master is attached to > the domain later on, but that seems rather complicated from an book- > keeping point of view and tricky because we need to be careful not to > map regions twice, etc. > > Any good ideas on how to solve this? It'd also be interesting to see if > there's a more generic way of doing this. I know that something like > this isn't necessary on earlier Tegra SoCs with the custom Tegra SMMU > because translations are only enabled when the devices are attached to a > domain. Good foresight. As [1] shows, identity mapping doesn't solve it in a generic way. How about actually reading the current settings/mappings and just inheriting that instead of always doing a 1:1 identity mapping? And then those "inherited" mappings can be dropped when you get a sync_state(). What's wrong with that option? Cheers, Saravana [1] https://lore.kernel.org/linux-arm-msm/20200108091641.GA15147@willie-the-truck/ [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/driver-api/driver-model/driver.rst#n172 [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/admin-guide/kernel-parameters.txt#n3239 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
