| Message ID | 20230920-rmtfs-mem-guard-pages-v3-0-305b37219b78@quicinc.com |
|---|---|
| Headers | show |
| Series | soc: qcom: rmtfs: Support dynamic allocation | expand |
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 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
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,