diff mbox series

[v3,6/8] iommu/arm-smmu: Add impl hook for inherit boot mappings

Message ID 20200904155513.282067-7-bjorn.andersson@linaro.org
State New
Headers show
Series iommu/arm-smmu: Support maintaining bootloader mappings | expand

Commit Message

Bjorn Andersson Sept. 4, 2020, 3:55 p.m. UTC
Add a new operation to allow platform implementations to inherit any
stream mappings from the boot loader.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v2:
- New patch/interface

 drivers/iommu/arm/arm-smmu/arm-smmu.c | 11 ++++++-----
 drivers/iommu/arm/arm-smmu/arm-smmu.h |  6 ++++++
 2 files changed, 12 insertions(+), 5 deletions(-)

Comments

Sai Prakash Ranjan Sept. 11, 2020, 8:27 a.m. UTC | #1
On 2020-09-04 21:25, Bjorn Andersson wrote:
> Add a new operation to allow platform implementations to inherit any
> stream mappings from the boot loader.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 

Reviewed-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
Tested-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
Robin Murphy Sept. 11, 2020, 5:13 p.m. UTC | #2
On 2020-09-04 16:55, Bjorn Andersson wrote:
> Add a new operation to allow platform implementations to inherit any
> stream mappings from the boot loader.

Is there a reason we need an explicit step for this? The aim of the 
cfg_probe hook is that the SMMU software state should all be set up by 
then, and you can mess about with it however you like before 
arm_smmu_reset() actually commits anything to hardware. I would have 
thought you could permanently steal a context bank, configure it as your 
bypass hole, read out the previous SME configuration and tweak 
smmu->smrs and smmu->s2crs appropriately all together "invisibly" at 
that point. If that can't work, I'm very curious as to what I've overlooked.

Robin.

> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v2:
> - New patch/interface
> 
>   drivers/iommu/arm/arm-smmu/arm-smmu.c | 11 ++++++-----
>   drivers/iommu/arm/arm-smmu/arm-smmu.h |  6 ++++++
>   2 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index eb5c6ca5c138..4c4d302cd747 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -85,11 +85,6 @@ static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
>   		pm_runtime_put_autosuspend(smmu->dev);
>   }
>   
> -static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
> -{
> -	return container_of(dom, struct arm_smmu_domain, domain);
> -}
> -
>   static struct platform_driver arm_smmu_driver;
>   static struct iommu_ops arm_smmu_ops;
>   
> @@ -2188,6 +2183,12 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>   	if (err)
>   		return err;
>   
> +	if (smmu->impl->inherit_mappings) {
> +		err = smmu->impl->inherit_mappings(smmu);
> +		if (err)
> +			return err;
> +	}
> +
>   	if (smmu->version == ARM_SMMU_V2) {
>   		if (smmu->num_context_banks > smmu->num_context_irqs) {
>   			dev_err(dev,
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> index 235d9a3a6ab6..f58164976e74 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> @@ -378,6 +378,11 @@ struct arm_smmu_domain {
>   	struct iommu_domain		domain;
>   };
>   
> +static inline struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
> +{
> +	return container_of(dom, struct arm_smmu_domain, domain);
> +}
> +
>   struct arm_smmu_master_cfg {
>   	struct arm_smmu_device		*smmu;
>   	s16				smendx[];
> @@ -442,6 +447,7 @@ struct arm_smmu_impl {
>   	int (*alloc_context_bank)(struct arm_smmu_domain *smmu_domain,
>   				  struct arm_smmu_device *smmu,
>   				  struct device *dev, int start);
> +	int (*inherit_mappings)(struct arm_smmu_device *smmu);
>   };
>   
>   #define INVALID_SMENDX			-1
>
Bjorn Andersson Sept. 13, 2020, 3:25 a.m. UTC | #3
On Fri 11 Sep 12:13 CDT 2020, Robin Murphy wrote:

> On 2020-09-04 16:55, Bjorn Andersson wrote:
> > Add a new operation to allow platform implementations to inherit any
> > stream mappings from the boot loader.
> 
> Is there a reason we need an explicit step for this? The aim of the
> cfg_probe hook is that the SMMU software state should all be set up by then,
> and you can mess about with it however you like before arm_smmu_reset()
> actually commits anything to hardware. I would have thought you could
> permanently steal a context bank, configure it as your bypass hole, read out
> the previous SME configuration and tweak smmu->smrs and smmu->s2crs
> appropriately all together "invisibly" at that point.

I did this because as of 6a79a5a3842b ("iommu/arm-smmu: Call
configuration impl hook before consuming features") we no longer have
setup pgsize_bitmap as we hit cfg_probe, which means that I need to
replicate this logic to set up the iommu_domain.

If I avoid setting up an iommu_domain for the identity context, as you
request in patch 8, this shouldn't be needed anymore.

> If that can't work, I'm very curious as to what I've overlooked.
> 

I believe that will work, I will rework the patches and try it out.

Thanks,
Bjorn

> Robin.
> 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > 
> > Changes since v2:
> > - New patch/interface
> > 
> >   drivers/iommu/arm/arm-smmu/arm-smmu.c | 11 ++++++-----
> >   drivers/iommu/arm/arm-smmu/arm-smmu.h |  6 ++++++
> >   2 files changed, 12 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > index eb5c6ca5c138..4c4d302cd747 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > @@ -85,11 +85,6 @@ static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
> >   		pm_runtime_put_autosuspend(smmu->dev);
> >   }
> > -static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
> > -{
> > -	return container_of(dom, struct arm_smmu_domain, domain);
> > -}
> > -
> >   static struct platform_driver arm_smmu_driver;
> >   static struct iommu_ops arm_smmu_ops;
> > @@ -2188,6 +2183,12 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
> >   	if (err)
> >   		return err;
> > +	if (smmu->impl->inherit_mappings) {
> > +		err = smmu->impl->inherit_mappings(smmu);
> > +		if (err)
> > +			return err;
> > +	}
> > +
> >   	if (smmu->version == ARM_SMMU_V2) {
> >   		if (smmu->num_context_banks > smmu->num_context_irqs) {
> >   			dev_err(dev,
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> > index 235d9a3a6ab6..f58164976e74 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> > @@ -378,6 +378,11 @@ struct arm_smmu_domain {
> >   	struct iommu_domain		domain;
> >   };
> > +static inline struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
> > +{
> > +	return container_of(dom, struct arm_smmu_domain, domain);
> > +}
> > +
> >   struct arm_smmu_master_cfg {
> >   	struct arm_smmu_device		*smmu;
> >   	s16				smendx[];
> > @@ -442,6 +447,7 @@ struct arm_smmu_impl {
> >   	int (*alloc_context_bank)(struct arm_smmu_domain *smmu_domain,
> >   				  struct arm_smmu_device *smmu,
> >   				  struct device *dev, int start);
> > +	int (*inherit_mappings)(struct arm_smmu_device *smmu);
> >   };
> >   #define INVALID_SMENDX			-1
> >
Will Deacon Sept. 21, 2020, 9:08 p.m. UTC | #4
On Sat, Sep 12, 2020 at 10:25:59PM -0500, Bjorn Andersson wrote:
> On Fri 11 Sep 12:13 CDT 2020, Robin Murphy wrote:
> > On 2020-09-04 16:55, Bjorn Andersson wrote:
> > > Add a new operation to allow platform implementations to inherit any
> > > stream mappings from the boot loader.
> > 
> > Is there a reason we need an explicit step for this? The aim of the
> > cfg_probe hook is that the SMMU software state should all be set up by then,
> > and you can mess about with it however you like before arm_smmu_reset()
> > actually commits anything to hardware. I would have thought you could
> > permanently steal a context bank, configure it as your bypass hole, read out
> > the previous SME configuration and tweak smmu->smrs and smmu->s2crs
> > appropriately all together "invisibly" at that point.
> 
> I did this because as of 6a79a5a3842b ("iommu/arm-smmu: Call
> configuration impl hook before consuming features") we no longer have
> setup pgsize_bitmap as we hit cfg_probe, which means that I need to
> replicate this logic to set up the iommu_domain.
> 
> If I avoid setting up an iommu_domain for the identity context, as you
> request in patch 8, this shouldn't be needed anymore.
> 
> > If that can't work, I'm very curious as to what I've overlooked.
> > 
> 
> I believe that will work, I will rework the patches and try it out.

Did you get a chance to rework this?

Will
Bjorn Andersson Sept. 24, 2020, 3:55 p.m. UTC | #5
On Mon 21 Sep 16:08 CDT 2020, Will Deacon wrote:

> On Sat, Sep 12, 2020 at 10:25:59PM -0500, Bjorn Andersson wrote:
> > On Fri 11 Sep 12:13 CDT 2020, Robin Murphy wrote:
> > > On 2020-09-04 16:55, Bjorn Andersson wrote:
> > > > Add a new operation to allow platform implementations to inherit any
> > > > stream mappings from the boot loader.
> > > 
> > > Is there a reason we need an explicit step for this? The aim of the
> > > cfg_probe hook is that the SMMU software state should all be set up by then,
> > > and you can mess about with it however you like before arm_smmu_reset()
> > > actually commits anything to hardware. I would have thought you could
> > > permanently steal a context bank, configure it as your bypass hole, read out
> > > the previous SME configuration and tweak smmu->smrs and smmu->s2crs
> > > appropriately all together "invisibly" at that point.
> > 
> > I did this because as of 6a79a5a3842b ("iommu/arm-smmu: Call
> > configuration impl hook before consuming features") we no longer have
> > setup pgsize_bitmap as we hit cfg_probe, which means that I need to
> > replicate this logic to set up the iommu_domain.
> > 
> > If I avoid setting up an iommu_domain for the identity context, as you
> > request in patch 8, this shouldn't be needed anymore.
> > 
> > > If that can't work, I'm very curious as to what I've overlooked.
> > > 
> > 
> > I believe that will work, I will rework the patches and try it out.
> 
> Did you get a chance to rework this?
> 

Unfortunately not, I hope to get to this shortly.

Thanks,
Bjorn
Bjorn Andersson Oct. 12, 2020, 7:31 a.m. UTC | #6
On Mon 21 Sep 23:08 CEST 2020, Will Deacon wrote:

> On Sat, Sep 12, 2020 at 10:25:59PM -0500, Bjorn Andersson wrote:
> > On Fri 11 Sep 12:13 CDT 2020, Robin Murphy wrote:
> > > On 2020-09-04 16:55, Bjorn Andersson wrote:
> > > > Add a new operation to allow platform implementations to inherit any
> > > > stream mappings from the boot loader.
> > > 
> > > Is there a reason we need an explicit step for this? The aim of the
> > > cfg_probe hook is that the SMMU software state should all be set up by then,
> > > and you can mess about with it however you like before arm_smmu_reset()
> > > actually commits anything to hardware. I would have thought you could
> > > permanently steal a context bank, configure it as your bypass hole, read out
> > > the previous SME configuration and tweak smmu->smrs and smmu->s2crs
> > > appropriately all together "invisibly" at that point.
> > 
> > I did this because as of 6a79a5a3842b ("iommu/arm-smmu: Call
> > configuration impl hook before consuming features") we no longer have
> > setup pgsize_bitmap as we hit cfg_probe, which means that I need to
> > replicate this logic to set up the iommu_domain.
> > 
> > If I avoid setting up an iommu_domain for the identity context, as you
> > request in patch 8, this shouldn't be needed anymore.
> > 
> > > If that can't work, I'm very curious as to what I've overlooked.
> > > 
> > 
> > I believe that will work, I will rework the patches and try it out.
> 
> Did you get a chance to rework this?
> 

Finally got a chance to dig through this properly.

Initial results where positive and with an implementation of cfg_probe
in qcom_smmu_impl I'm able to probe the arm-smmu driver just fine - and
display (e.g. efifb) stays alive.

Unfortunately as the display driver (drivers/gpu/drm/msm) is about to
probe a new iommu domain is created, which due to its match against
qcom_smmu_client_of_match[] becomes of type IOMMU_DOMAIN_IDENTITY.
This results in a S2CR of BYPASS type, which the firmware intercepts and
turns the stream into a type FAULT.

So while the cfg_probe looks very reasonable we're still in need of a
mechanism to use the fake identity context for the iommu domain
associated with the display controller.


The workings of the display driver is that it gets the iommu domain
setup for byass and then after that creates a translation context for
this same stream where it maps the framebuffer.

For testing purposes I made def_domain_type always return 0 in the qcom
impl and the result is that we get a few page faults while probing the
display driver, but these are handled somewhat gracefully and the
initialization did proceed and the system comes up nicely (but in the
case that the display driver would probe defer this leads to an storm of
faults as the screen continues to be refreshed).

TL;DR I think we still need to have a way to get the arm-smmu driver to
allow the qcom implementation to configure identity domains to use
translation - but we can make the setup of the identity context a detail
of the qcom driver.

Regards,
Bjorn
Robin Murphy Oct. 13, 2020, 4:47 p.m. UTC | #7
On 2020-10-12 08:31, Bjorn Andersson wrote:
> On Mon 21 Sep 23:08 CEST 2020, Will Deacon wrote:
> 
>> On Sat, Sep 12, 2020 at 10:25:59PM -0500, Bjorn Andersson wrote:
>>> On Fri 11 Sep 12:13 CDT 2020, Robin Murphy wrote:
>>>> On 2020-09-04 16:55, Bjorn Andersson wrote:
>>>>> Add a new operation to allow platform implementations to inherit any
>>>>> stream mappings from the boot loader.
>>>>
>>>> Is there a reason we need an explicit step for this? The aim of the
>>>> cfg_probe hook is that the SMMU software state should all be set up by then,
>>>> and you can mess about with it however you like before arm_smmu_reset()
>>>> actually commits anything to hardware. I would have thought you could
>>>> permanently steal a context bank, configure it as your bypass hole, read out
>>>> the previous SME configuration and tweak smmu->smrs and smmu->s2crs
>>>> appropriately all together "invisibly" at that point.
>>>
>>> I did this because as of 6a79a5a3842b ("iommu/arm-smmu: Call
>>> configuration impl hook before consuming features") we no longer have
>>> setup pgsize_bitmap as we hit cfg_probe, which means that I need to
>>> replicate this logic to set up the iommu_domain.
>>>
>>> If I avoid setting up an iommu_domain for the identity context, as you
>>> request in patch 8, this shouldn't be needed anymore.
>>>
>>>> If that can't work, I'm very curious as to what I've overlooked.
>>>>
>>>
>>> I believe that will work, I will rework the patches and try it out.
>>
>> Did you get a chance to rework this?
>>
> 
> Finally got a chance to dig through this properly.
> 
> Initial results where positive and with an implementation of cfg_probe
> in qcom_smmu_impl I'm able to probe the arm-smmu driver just fine - and
> display (e.g. efifb) stays alive.
> 
> Unfortunately as the display driver (drivers/gpu/drm/msm) is about to
> probe a new iommu domain is created, which due to its match against
> qcom_smmu_client_of_match[] becomes of type IOMMU_DOMAIN_IDENTITY.
> This results in a S2CR of BYPASS type, which the firmware intercepts and
> turns the stream into a type FAULT.
> 
> So while the cfg_probe looks very reasonable we're still in need of a
> mechanism to use the fake identity context for the iommu domain
> associated with the display controller.

Yes, we'll still need some kind of hook somewhere to make identity 
domains work at all - my point about cfg_probe was to keep the 
reservation and configuration of the special identity context, plus the 
handling of the initial SME state, simple and entirely internal to the 
impl. In terms of where said hook should be, TBH it might actually work 
out pretty clean to simply hook GR0 register accesses so you can rewrite 
between S2CR bypass entries and translation entries targeting your 
reserved context on-the-fly. Failing that, something to massage "type" 
and "cbndx" in arm_smmu_domain_add_master() would be the next best 
option, I think.

Robin.

> The workings of the display driver is that it gets the iommu domain
> setup for byass and then after that creates a translation context for
> this same stream where it maps the framebuffer.
> 
> For testing purposes I made def_domain_type always return 0 in the qcom
> impl and the result is that we get a few page faults while probing the
> display driver, but these are handled somewhat gracefully and the
> initialization did proceed and the system comes up nicely (but in the
> case that the display driver would probe defer this leads to an storm of
> faults as the screen continues to be refreshed).
> 
> TL;DR I think we still need to have a way to get the arm-smmu driver to
> allow the qcom implementation to configure identity domains to use
> translation - but we can make the setup of the identity context a detail
> of the qcom driver.
> 
> Regards,
> Bjorn
>
diff mbox series

Patch

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index eb5c6ca5c138..4c4d302cd747 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -85,11 +85,6 @@  static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
 		pm_runtime_put_autosuspend(smmu->dev);
 }
 
-static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
-{
-	return container_of(dom, struct arm_smmu_domain, domain);
-}
-
 static struct platform_driver arm_smmu_driver;
 static struct iommu_ops arm_smmu_ops;
 
@@ -2188,6 +2183,12 @@  static int arm_smmu_device_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
+	if (smmu->impl->inherit_mappings) {
+		err = smmu->impl->inherit_mappings(smmu);
+		if (err)
+			return err;
+	}
+
 	if (smmu->version == ARM_SMMU_V2) {
 		if (smmu->num_context_banks > smmu->num_context_irqs) {
 			dev_err(dev,
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
index 235d9a3a6ab6..f58164976e74 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
@@ -378,6 +378,11 @@  struct arm_smmu_domain {
 	struct iommu_domain		domain;
 };
 
+static inline struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
+{
+	return container_of(dom, struct arm_smmu_domain, domain);
+}
+
 struct arm_smmu_master_cfg {
 	struct arm_smmu_device		*smmu;
 	s16				smendx[];
@@ -442,6 +447,7 @@  struct arm_smmu_impl {
 	int (*alloc_context_bank)(struct arm_smmu_domain *smmu_domain,
 				  struct arm_smmu_device *smmu,
 				  struct device *dev, int start);
+	int (*inherit_mappings)(struct arm_smmu_device *smmu);
 };
 
 #define INVALID_SMENDX			-1