| Message ID | 20230920-rmtfs-mem-guard-pages-v3-0-305b37219b78@quicinc.com |
|---|---|
| Headers | show |
| Series | soc: qcom: rmtfs: Support dynamic allocation | expand |
On 9/21/23 04:37, Bjorn Andersson wrote: > In the even that Linux failed to allocate the reserved memory range > specified in the DeviceTree, the size of the reserved_mem will be 0, > which results in a oops when memory remapping is attempted. > > Detect this and report that the memory region was not found instead. > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- typo in subject: rtmfs Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad
On 9/21/23 04:37, Bjorn Andersson wrote: > In some configurations, the exact placement of the rmtfs shared memory > region isn't so strict. The DeviceTree author can then choose to use the > "size" property and rely on the OS for placement (in combination with > "alloc-ranges", if desired). > > But on some platforms the rmtfs memory region may not be allocated > adjacent to regions allocated by other clients. Add support for > discarding the first and last 4k block in the region, if > qcom,use-guard-pages is specified in DeviceTree. > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- I don't want to block this anymore, but I guess I should ask the question whether it would be valuable to add a common reserved-memory property for e.g. low-padding and high-padding Have we seen cases of that outside rmtfs? Konrad
On Wed, Sep 20, 2023 at 07:37:31PM -0700, Bjorn Andersson wrote: > In some configurations, the exact placement of the rmtfs shared memory > region isn't so strict. The DeviceTree author can then choose to use the > "size" property and rely on the OS for placement (in combination with > "alloc-ranges", if desired). > > But on some platforms the rmtfs memory region may not be allocated > adjacent to regions allocated by other clients. Add support for > discarding the first and last 4k block in the region, if > qcom,use-guard-pages is specified in DeviceTree. > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- > drivers/soc/qcom/rmtfs_mem.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c > index f83811f51175..83bba9321e72 100644 > --- a/drivers/soc/qcom/rmtfs_mem.c > +++ b/drivers/soc/qcom/rmtfs_mem.c > @@ -200,6 +200,15 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev) > rmtfs_mem->client_id = client_id; > rmtfs_mem->size = rmem->size; > > + /* > + * If requested, discard the first and last 4k block in order to ensure > + * that the rmtfs region isn't adjacent to other protected regions. > + */ > + if (of_property_present(node, "qcom,use-guard-pages")) { I think of_property_read_bool() would be more fitting here. Right now of_property_present() is just a wrapper around of_property_read_bool(). Semantically reading a bool fits better here though. :-) Feel free to fix that up while applying. FWIW I don't really have an opinion if "qcom,use-guard-pages" is a good way to describe this in the DT. For the implementation side feel free to add my Reviewed-by: Stephan Gerhold <stephan@gerhold.net> Thanks, Stephan > + rmtfs_mem->addr += SZ_4K; > + rmtfs_mem->size -= 2 * SZ_4K; > + } > + > device_initialize(&rmtfs_mem->dev); > rmtfs_mem->dev.parent = &pdev->dev; > rmtfs_mem->dev.groups = qcom_rmtfs_mem_groups; > > -- > 2.25.1 >
On Wed, Sep 20, 2023 at 07:37:32PM -0700, Bjorn Andersson wrote: > In the even that Linux failed to allocate the reserved memory range > specified in the DeviceTree, the size of the reserved_mem will be 0, > which results in a oops when memory remapping is attempted. > > Detect this and report that the memory region was not found instead. > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> I dropped these checks in my remoteproc patches because Caleb suggested maybe putting this check directly in of_reserved_mem_lookup() (or similar) given that almost none of the users verify this [1]. Do you have any opinion on that? I asked back then too but you did not reply yet [2]. :-) [1]: https://lore.kernel.org/linux-arm-msm/c3f59fb4-4dd8-f27a-d3f5-b1870006a75c@linaro.org/ [2]: https://lore.kernel.org/linux-arm-msm/ZIsld-MAdkKvdzTx@gerhold.net/ > --- > drivers/soc/qcom/rmtfs_mem.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c > index 83bba9321e72..13823abd85c2 100644 > --- a/drivers/soc/qcom/rmtfs_mem.c > +++ b/drivers/soc/qcom/rmtfs_mem.c > @@ -180,7 +180,7 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev) > int ret, i; > > rmem = of_reserved_mem_lookup(node); > - if (!rmem) { > + if (!rmem || !rmem->size) { > dev_err(&pdev->dev, "failed to acquire memory region\n"); > return -EINVAL; > } > > -- > 2.25.1 >
On Thu, Sep 21, 2023 at 08:11:23PM +0200, Stephan Gerhold wrote: > On Wed, Sep 20, 2023 at 07:37:32PM -0700, Bjorn Andersson wrote: > > In the even that Linux failed to allocate the reserved memory range > > specified in the DeviceTree, the size of the reserved_mem will be 0, > > which results in a oops when memory remapping is attempted. > > > > Detect this and report that the memory region was not found instead. > > > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > > I dropped these checks in my remoteproc patches because Caleb suggested > maybe putting this check directly in of_reserved_mem_lookup() (or > similar) given that almost none of the users verify this [1]. > > Do you have any opinion on that? I asked back then too but you did not > reply yet [2]. :-) > I'm struggling to come up with a use case where one would like to get hold of the rmem when it wasn't properly initialized. So, let's make an attempt at returning NULL from of_reserved_mem_lookup() instead. Thanks, Bjorn > [1]: https://lore.kernel.org/linux-arm-msm/c3f59fb4-4dd8-f27a-d3f5-b1870006a75c@linaro.org/ > [2]: https://lore.kernel.org/linux-arm-msm/ZIsld-MAdkKvdzTx@gerhold.net/ > > > --- > > drivers/soc/qcom/rmtfs_mem.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c > > index 83bba9321e72..13823abd85c2 100644 > > --- a/drivers/soc/qcom/rmtfs_mem.c > > +++ b/drivers/soc/qcom/rmtfs_mem.c > > @@ -180,7 +180,7 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev) > > int ret, i; > > > > rmem = of_reserved_mem_lookup(node); > > - if (!rmem) { > > + if (!rmem || !rmem->size) { > > dev_err(&pdev->dev, "failed to acquire memory region\n"); > > return -EINVAL; > > } > > > > -- > > 2.25.1 > >
On Thu, Sep 21, 2023 at 08:04:06PM +0200, Stephan Gerhold wrote: > On Wed, Sep 20, 2023 at 07:37:31PM -0700, Bjorn Andersson wrote: > > In some configurations, the exact placement of the rmtfs shared memory > > region isn't so strict. The DeviceTree author can then choose to use the > > "size" property and rely on the OS for placement (in combination with > > "alloc-ranges", if desired). > > > > But on some platforms the rmtfs memory region may not be allocated > > adjacent to regions allocated by other clients. Add support for > > discarding the first and last 4k block in the region, if > > qcom,use-guard-pages is specified in DeviceTree. > > > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > > --- > > drivers/soc/qcom/rmtfs_mem.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c > > index f83811f51175..83bba9321e72 100644 > > --- a/drivers/soc/qcom/rmtfs_mem.c > > +++ b/drivers/soc/qcom/rmtfs_mem.c > > @@ -200,6 +200,15 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev) > > rmtfs_mem->client_id = client_id; > > rmtfs_mem->size = rmem->size; > > > > + /* > > + * If requested, discard the first and last 4k block in order to ensure > > + * that the rmtfs region isn't adjacent to other protected regions. > > + */ > > + if (of_property_present(node, "qcom,use-guard-pages")) { > > I think of_property_read_bool() would be more fitting here. Right now > of_property_present() is just a wrapper around of_property_read_bool(). > Semantically reading a bool fits better here though. :-) > Are you saying that you would prefer this to be a bool, so hat you can give it a "false" value? Or you are simply saying "it walks like a boolean, quacks like a boolean, let's use the boolean accessor"? > Feel free to fix that up while applying. > > FWIW I don't really have an opinion if "qcom,use-guard-pages" is a good > way to describe this in the DT. For the implementation side feel free to > add my > Right, I don't think I commented on your suggestion to make the size of the guard page configurable. I am not aware of any current or upcoming reasons for adding such complexity, so I'd simply prefer to stick with a boolean. Should that need arise, I think this model would allow extension to express that. Regards, Bjorn > Reviewed-by: Stephan Gerhold <stephan@gerhold.net> > > Thanks, > Stephan > > > + rmtfs_mem->addr += SZ_4K; > > + rmtfs_mem->size -= 2 * SZ_4K; > > + } > > + > > device_initialize(&rmtfs_mem->dev); > > rmtfs_mem->dev.parent = &pdev->dev; > > rmtfs_mem->dev.groups = qcom_rmtfs_mem_groups; > > > > -- > > 2.25.1 > >
eOn Thu, Sep 21, 2023 at 07:51:42PM -0700, Bjorn Andersson wrote: > On Thu, Sep 21, 2023 at 08:04:06PM +0200, Stephan Gerhold wrote: > > On Wed, Sep 20, 2023 at 07:37:31PM -0700, Bjorn Andersson wrote: > > > In some configurations, the exact placement of the rmtfs shared memory > > > region isn't so strict. The DeviceTree author can then choose to use the > > > "size" property and rely on the OS for placement (in combination with > > > "alloc-ranges", if desired). > > > > > > But on some platforms the rmtfs memory region may not be allocated > > > adjacent to regions allocated by other clients. Add support for > > > discarding the first and last 4k block in the region, if > > > qcom,use-guard-pages is specified in DeviceTree. > > > > > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > > > --- > > > drivers/soc/qcom/rmtfs_mem.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c > > > index f83811f51175..83bba9321e72 100644 > > > --- a/drivers/soc/qcom/rmtfs_mem.c > > > +++ b/drivers/soc/qcom/rmtfs_mem.c > > > @@ -200,6 +200,15 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev) > > > rmtfs_mem->client_id = client_id; > > > rmtfs_mem->size = rmem->size; > > > > > > + /* > > > + * If requested, discard the first and last 4k block in order to ensure > > > + * that the rmtfs region isn't adjacent to other protected regions. > > > + */ > > > + if (of_property_present(node, "qcom,use-guard-pages")) { > > > > I think of_property_read_bool() would be more fitting here. Right now > > of_property_present() is just a wrapper around of_property_read_bool(). > > Semantically reading a bool fits better here though. :-) > > > > Are you saying that you would prefer this to be a bool, so hat you can > give it a "false" value? Or you are simply saying "it walks like a > boolean, quacks like a boolean, let's use the boolean accessor"? > The latter. I would expect that of_property_present() is used for properties which usually have a value, while of_property_read_bool() is used for pure bool values which can be present or not but must not have a value. I think a "bool" in terms of DT is simply a present or not-present property without any value? For example consider regulator-min-microvolts = <4200000000>; regulator-always-on; Then I would expect - of_property_present(..., "regulator-min-microvolts"), but - of_property_read_bool(..., "regulator-always-on") Does that make sense? :D > > Feel free to fix that up while applying. > > > > FWIW I don't really have an opinion if "qcom,use-guard-pages" is a good > > way to describe this in the DT. For the implementation side feel free to > > add my > > > > Right, I don't think I commented on your suggestion to make the size of > the guard page configurable. I am not aware of any current or upcoming > reasons for adding such complexity, so I'd simply prefer to stick with a > boolean. Should that need arise, I think this model would allow > extension to express that. > I must admit I forgot that I suggested this until now. :') I don't see a use case for a different "guard size" either so I think it's fine to have it as a bool. Thanks, Stephan
On Fri, Sep 22, 2023 at 09:35:00AM +0200, Stephan Gerhold wrote: > eOn Thu, Sep 21, 2023 at 07:51:42PM -0700, Bjorn Andersson wrote: > > On Thu, Sep 21, 2023 at 08:04:06PM +0200, Stephan Gerhold wrote: > > > On Wed, Sep 20, 2023 at 07:37:31PM -0700, Bjorn Andersson wrote: > > > > In some configurations, the exact placement of the rmtfs shared memory > > > > region isn't so strict. The DeviceTree author can then choose to use the > > > > "size" property and rely on the OS for placement (in combination with > > > > "alloc-ranges", if desired). > > > > > > > > But on some platforms the rmtfs memory region may not be allocated > > > > adjacent to regions allocated by other clients. Add support for > > > > discarding the first and last 4k block in the region, if > > > > qcom,use-guard-pages is specified in DeviceTree. > > > > > > > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > > > > --- > > > > drivers/soc/qcom/rmtfs_mem.c | 9 +++++++++ > > > > 1 file changed, 9 insertions(+) > > > > > > > > diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c > > > > index f83811f51175..83bba9321e72 100644 > > > > --- a/drivers/soc/qcom/rmtfs_mem.c > > > > +++ b/drivers/soc/qcom/rmtfs_mem.c > > > > @@ -200,6 +200,15 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev) > > > > rmtfs_mem->client_id = client_id; > > > > rmtfs_mem->size = rmem->size; > > > > > > > > + /* > > > > + * If requested, discard the first and last 4k block in order to ensure > > > > + * that the rmtfs region isn't adjacent to other protected regions. > > > > + */ > > > > + if (of_property_present(node, "qcom,use-guard-pages")) { > > > > > > I think of_property_read_bool() would be more fitting here. Right now > > > of_property_present() is just a wrapper around of_property_read_bool(). > > > Semantically reading a bool fits better here though. :-) > > > > > > > Are you saying that you would prefer this to be a bool, so hat you can > > give it a "false" value? Or you are simply saying "it walks like a > > boolean, quacks like a boolean, let's use the boolean accessor"? > > > > The latter. I would expect that of_property_present() is used for > properties which usually have a value, while of_property_read_bool() > is used for pure bool values which can be present or not but must not > have a value. I think a "bool" in terms of DT is simply a present or > not-present property without any value? > > For example consider > > regulator-min-microvolts = <4200000000>; > regulator-always-on; > > Then I would expect > > - of_property_present(..., "regulator-min-microvolts"), but > - of_property_read_bool(..., "regulator-always-on") > > Does that make sense? :D > Certainly, of_property_read_duck() it is. Thanks, Bjorn
On Wed, 20 Sep 2023 19:37:29 -0700, Bjorn Andersson wrote: > Some platforms have laxed requirements on the placement of the rmtfs > memory region, introduce support for guard pages to allow the DeviceTree > author to rely on the OS/Linux for placement of the region. > > Changes since v2: > - Rewrote DeviceTree binding description, to avoid dictating the OS > behavior. > - Adjusted addr and size before memremap(), to avoid mapping the guard > pages, and unnecessarily have to adjust the base pointer. > > [...] Applied, thanks! [1/3] dt-bindings: reserved-memory: rmtfs: Allow guard pages commit: 3ad96787949a96256931ca59aff73ea604bc0e6c [2/3] soc: qcom: rmtfs: Support discarding guard pages commit: 9265bc6bce6919c771970e5a425a66551a1c78a0 Best regards,
Some platforms have laxed requirements on the placement of the rmtfs memory region, introduce support for guard pages to allow the DeviceTree author to rely on the OS/Linux for placement of the region. Changes since v2: - Rewrote DeviceTree binding description, to avoid dictating the OS behavior. - Adjusted addr and size before memremap(), to avoid mapping the guard pages, and unnecessarily have to adjust the base pointer. Changes since v1: - Drop qcom,alloc-size in favour of using reserved-memory/size - Introduce explicit property to signal that guard pages should be carved out from this region (rather than always do it in the dynamic case). - Drop the dma_alloc_coherent() based approach and just add support for the guard pages. - Added handling of failed reserved-memory allocation (patch 3) --- Bjorn Andersson (3): dt-bindings: reserved-memory: rmtfs: Allow guard pages soc: qcom: rmtfs: Support discarding guard pages soc: qcom: rtmfs: Handle reserved-memory allocation issues .../devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml | 11 +++++++++++ drivers/soc/qcom/rmtfs_mem.c | 11 ++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) --- base-commit: 926f75c8a5ab70567eb4c2d82fbc96963313e564 change-id: 20230920-rmtfs-mem-guard-pages-d3d0ed0cb036 Best regards,