| Message ID | 20250130072100.27297-5-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Fix page permission on arm64 architectures | expand |
On 1/30/25 07:20, Ilias Apalodimas wrote: > Now that we have everything in place switch the page permissions for > .rodata, .text and .data just after we relocate everything in top of the > RAM. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > common/board_r.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/common/board_r.c b/common/board_r.c > index 179259b00de8..38944f600fd6 100644 > --- a/common/board_r.c > +++ b/common/board_r.c > @@ -170,6 +170,13 @@ static int initr_reloc_global_data(void) > efi_save_gd(); > > efi_runtime_relocate(gd->relocaddr, NULL); > + > +#if (IS_ENABLED(CONFIG_ARM64)) '#if' should be replaced by 'if (CONFIG_IS_ENABLED())'. Shall we make mmu_set_attrs() a weak function which each architecture can override? > + mmu_set_attrs((u64)(__start_rodata), (u64)(__end_rodata - __start_rodata), 1); > + mmu_set_attrs((u64)(__start_data), (u64)(__end_data - __start_data), 3); > + mmu_set_attrs((u64)(__text_start), (u64)(__text_end - __text_start), 2); Please, replace 1, 2, 3 by constants both here and in mmu_set_attrs, e.g. 1 -> MMU_ATTR_RO 2 -> MMU_ATTR_RX 3 -> MMU_ATTR_RW Best regards Heinrich > +#endif > + > #endif > > return 0;
Hi Heinrich, On Thu, 30 Jan 2025 at 12:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 1/30/25 07:20, Ilias Apalodimas wrote: > > Now that we have everything in place switch the page permissions for > > .rodata, .text and .data just after we relocate everything in top of the > > RAM. > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > --- > > common/board_r.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/common/board_r.c b/common/board_r.c > > index 179259b00de8..38944f600fd6 100644 > > --- a/common/board_r.c > > +++ b/common/board_r.c > > @@ -170,6 +170,13 @@ static int initr_reloc_global_data(void) > > efi_save_gd(); > > > > efi_runtime_relocate(gd->relocaddr, NULL); > > + > > +#if (IS_ENABLED(CONFIG_ARM64)) > > '#if' should be replaced by 'if (CONFIG_IS_ENABLED())'. > > Shall we make mmu_set_attrs() a weak function which each architecture > can override? In order to do the IS_ENABLED as you asked, we need to define a weak function, otherwise the linker will complain. I can add a generic mmu_set_attrs() in the next version. The right way to do this in the future is to have a proper MMU API. > > > + mmu_set_attrs((u64)(__start_rodata), (u64)(__end_rodata - __start_rodata), 1); > > + mmu_set_attrs((u64)(__start_data), (u64)(__end_data - __start_data), 3); > > + mmu_set_attrs((u64)(__text_start), (u64)(__text_end - __text_start), 2); > > Please, replace 1, 2, 3 by constants both here and in mmu_set_attrs, e.g. > > 1 -> MMU_ATTR_RO > 2 -> MMU_ATTR_RX > 3 -> MMU_ATTR_RW Yea I am explaining the same thing in the cover letter Thanks /Ilias > > Best regards > > Heinrich > > > +#endif > > + > > #endif > > > > return 0; >
On 1/30/25 10:29, Ilias Apalodimas wrote: > Hi Heinrich, > > On Thu, 30 Jan 2025 at 12:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> On 1/30/25 07:20, Ilias Apalodimas wrote: >>> Now that we have everything in place switch the page permissions for >>> .rodata, .text and .data just after we relocate everything in top of the >>> RAM. >>> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> >>> --- >>> common/board_r.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/common/board_r.c b/common/board_r.c >>> index 179259b00de8..38944f600fd6 100644 >>> --- a/common/board_r.c >>> +++ b/common/board_r.c >>> @@ -170,6 +170,13 @@ static int initr_reloc_global_data(void) >>> efi_save_gd(); >>> >>> efi_runtime_relocate(gd->relocaddr, NULL); >>> + >>> +#if (IS_ENABLED(CONFIG_ARM64)) >> >> '#if' should be replaced by 'if (CONFIG_IS_ENABLED())'. >> >> Shall we make mmu_set_attrs() a weak function which each architecture >> can override? > > In order to do the IS_ENABLED as you asked, we need to define a weak > function, otherwise the linker will complain. > I can add a generic mmu_set_attrs() in the next version. The right way > to do this in the future is to have a proper MMU API. For IS_ENABLED you only need the definition in the header file and the implementation where it is configured. With the weak function no 'if' is needed. Best regards Heinrich > >> >>> + mmu_set_attrs((u64)(__start_rodata), (u64)(__end_rodata - __start_rodata), 1); >>> + mmu_set_attrs((u64)(__start_data), (u64)(__end_data - __start_data), 3); >>> + mmu_set_attrs((u64)(__text_start), (u64)(__text_end - __text_start), 2); >> >> Please, replace 1, 2, 3 by constants both here and in mmu_set_attrs, e.g. >> >> 1 -> MMU_ATTR_RO >> 2 -> MMU_ATTR_RX >> 3 -> MMU_ATTR_RW > > Yea I am explaining the same thing in the cover letter > > Thanks > /Ilias >> >> Best regards >> >> Heinrich >> >>> +#endif >>> + >>> #endif >>> >>> return 0; >>
diff --git a/common/board_r.c b/common/board_r.c index 179259b00de8..38944f600fd6 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -170,6 +170,13 @@ static int initr_reloc_global_data(void) efi_save_gd(); efi_runtime_relocate(gd->relocaddr, NULL); + +#if (IS_ENABLED(CONFIG_ARM64)) + mmu_set_attrs((u64)(__start_rodata), (u64)(__end_rodata - __start_rodata), 1); + mmu_set_attrs((u64)(__start_data), (u64)(__end_data - __start_data), 3); + mmu_set_attrs((u64)(__text_start), (u64)(__text_end - __text_start), 2); +#endif + #endif return 0;
Now that we have everything in place switch the page permissions for .rodata, .text and .data just after we relocate everything in top of the RAM. Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- common/board_r.c | 7 +++++++ 1 file changed, 7 insertions(+)