| Message ID | 20230920-rmtfs-mem-guard-pages-v3-2-305b37219b78@quicinc.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 9265bc6bce6919c771970e5a425a66551a1c78a0 |
| Headers | show |
| Series | soc: qcom: rmtfs: Support dynamic allocation | expand |
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 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
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")) { + 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;
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(+)