| Message ID | 20180403195934.24068-2-xypron.glpk@gmx.de |
|---|---|
| State | Accepted |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | efi_loader: armv7: enable unaligned access | expand |
On 03.04.18 21:59, Heinrich Schuchardt wrote: > The UEFI spec mandates that unaligned memory access should be enabled if > supported by the CPU architecture. > > This patch adds an empty weak function unaligned_access() that can be > overridden by an architecture specific routine. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > cmd/bootefi.c | 13 +++++++++++++ > include/asm-generic/unaligned.h | 3 +++ > 2 files changed, 16 insertions(+) > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > index ba258ac9f3..412e6519ba 100644 > --- a/cmd/bootefi.c > +++ b/cmd/bootefi.c > @@ -18,6 +18,7 @@ > #include <memalign.h> > #include <asm/global_data.h> > #include <asm-generic/sections.h> > +#include <asm-generic/unaligned.h> > #include <linux/linkage.h> > > DECLARE_GLOBAL_DATA_PTR; > @@ -89,6 +90,15 @@ out: > return ret; > } > > +/* > + * Allow unaligned memory access. > + * > + * This routine is overridden by architectures providing this feature. > + */ > +void __weak allow_unaligned(void) > +{ > +} > + I'd prefer to guard the body of the function with an #ifdef CONFIG_ARM so everyone knows why it's there. Then call straight into a function provided in the ARM core code: static void allow_unaligned(void) { /* On ARM we prohibit unaligned accesses by default, enable them here */ #if defined(CONFIG_ARM) && !defined(CONFIG_ARM64) && !defined(CONFIG_CPU_V7M) mmu_enable_unaligned(); #endif } And then in arch/arm/lib/cache-cp15.c: void mmu_enable_unaligned(void) { set_cr(get_cr() & ~CR_A); } I basically want to make sure this is as explicit as it gets :). Alex
On 04/04/2018 02:32 PM, Alexander Graf wrote: > > > On 03.04.18 21:59, Heinrich Schuchardt wrote: >> The UEFI spec mandates that unaligned memory access should be enabled if >> supported by the CPU architecture. >> >> This patch adds an empty weak function unaligned_access() that can be >> overridden by an architecture specific routine. >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> --- >> cmd/bootefi.c | 13 +++++++++++++ >> include/asm-generic/unaligned.h | 3 +++ >> 2 files changed, 16 insertions(+) >> >> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >> index ba258ac9f3..412e6519ba 100644 >> --- a/cmd/bootefi.c >> +++ b/cmd/bootefi.c >> @@ -18,6 +18,7 @@ >> #include <memalign.h> >> #include <asm/global_data.h> >> #include <asm-generic/sections.h> >> +#include <asm-generic/unaligned.h> >> #include <linux/linkage.h> >> >> DECLARE_GLOBAL_DATA_PTR; >> @@ -89,6 +90,15 @@ out: >> return ret; >> } >> >> +/* >> + * Allow unaligned memory access. >> + * >> + * This routine is overridden by architectures providing this feature. >> + */ >> +void __weak allow_unaligned(void) >> +{ >> +} >> + > > I'd prefer to guard the body of the function with an #ifdef CONFIG_ARM > so everyone knows why it's there. Then call straight into a function > provided in the ARM core code: The same visibility can be achieved with a comment. > > static void allow_unaligned(void) > { > /* On ARM we prohibit unaligned accesses by default, enable them here */ > #if defined(CONFIG_ARM) && !defined(CONFIG_ARM64) && > !defined(CONFIG_CPU_V7M) > mmu_enable_unaligned(); > #endif > } RISC-V also supports traps for unaligned access. Using architecture specific flags outside arch/ is a mess. We should not commit new sins but eliminate the existing deviations. > > And then in arch/arm/lib/cache-cp15.c: > > void mmu_enable_unaligned(void) > { > set_cr(get_cr() & ~CR_A); > } For some ARM architecture versions the bit is reserved and not used for unaligned access. No clue what this function would do in this case. That is why I used a weak function that does nothing if the CPU does not support the flag. Best regards Heinrich > > I basically want to make sure this is as explicit as it gets :). > > > Alex >
On 04.04.18 17:10, Heinrich Schuchardt wrote: > On 04/04/2018 02:32 PM, Alexander Graf wrote: >> >> >> On 03.04.18 21:59, Heinrich Schuchardt wrote: >>> The UEFI spec mandates that unaligned memory access should be enabled if >>> supported by the CPU architecture. >>> >>> This patch adds an empty weak function unaligned_access() that can be >>> overridden by an architecture specific routine. >>> >>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >>> --- >>> cmd/bootefi.c | 13 +++++++++++++ >>> include/asm-generic/unaligned.h | 3 +++ >>> 2 files changed, 16 insertions(+) >>> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >>> index ba258ac9f3..412e6519ba 100644 >>> --- a/cmd/bootefi.c >>> +++ b/cmd/bootefi.c >>> @@ -18,6 +18,7 @@ >>> #include <memalign.h> >>> #include <asm/global_data.h> >>> #include <asm-generic/sections.h> >>> +#include <asm-generic/unaligned.h> >>> #include <linux/linkage.h> >>> >>> DECLARE_GLOBAL_DATA_PTR; >>> @@ -89,6 +90,15 @@ out: >>> return ret; >>> } >>> >>> +/* >>> + * Allow unaligned memory access. >>> + * >>> + * This routine is overridden by architectures providing this feature. >>> + */ >>> +void __weak allow_unaligned(void) >>> +{ >>> +} >>> + >> >> I'd prefer to guard the body of the function with an #ifdef CONFIG_ARM >> so everyone knows why it's there. Then call straight into a function >> provided in the ARM core code: > > The same visibility can be achieved with a comment. It's not as obvious. The default really should be to map memory as cached and allow for unaligned accesses. > >> >> static void allow_unaligned(void) >> { >> /* On ARM we prohibit unaligned accesses by default, enable them here */ >> #if defined(CONFIG_ARM) && !defined(CONFIG_ARM64) && >> !defined(CONFIG_CPU_V7M) >> mmu_enable_unaligned(); >> #endif >> } > > RISC-V also supports traps for unaligned access. Just because it supports them doesn't mean we should use them. AArch64 also allows for unaligned access traps and I went through great length to refactor the mm code in U-Boot to ensure that we do not trap. > Using architecture specific flags outside arch/ is a mess. > We should not commit new sins but eliminate the existing deviations. > >> >> And then in arch/arm/lib/cache-cp15.c: >> >> void mmu_enable_unaligned(void) >> { >> set_cr(get_cr() & ~CR_A); >> } > > For some ARM architecture versions the bit is reserved and not used for > unaligned access. No clue what this function would do in this case. Do you have pointers? Anything defined in the UEFI spec should have the bit. > That is why I used a weak function that does nothing if the CPU does not > support the flag. Any platform that uses cache-cp15 also supports the CR_A bit FWIW. So it really belongs there. And again, I do not want to prettify a special hack for a broken architecture. People should see warts when they're there. The real fix IMHO is to enable aligned accesses always, like we do on any sane architecture. Alex
On 04/04/2018 06:11 PM, Alexander Graf wrote: > > > On 04.04.18 17:10, Heinrich Schuchardt wrote: >> On 04/04/2018 02:32 PM, Alexander Graf wrote: >>> >>> >>> On 03.04.18 21:59, Heinrich Schuchardt wrote: >>>> The UEFI spec mandates that unaligned memory access should be enabled if >>>> supported by the CPU architecture. >>>> >>>> This patch adds an empty weak function unaligned_access() that can be >>>> overridden by an architecture specific routine. >>>> >>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >>>> --- >>>> cmd/bootefi.c | 13 +++++++++++++ >>>> include/asm-generic/unaligned.h | 3 +++ >>>> 2 files changed, 16 insertions(+) >>>> >>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >>>> index ba258ac9f3..412e6519ba 100644 >>>> --- a/cmd/bootefi.c >>>> +++ b/cmd/bootefi.c >>>> @@ -18,6 +18,7 @@ >>>> #include <memalign.h> >>>> #include <asm/global_data.h> >>>> #include <asm-generic/sections.h> >>>> +#include <asm-generic/unaligned.h> >>>> #include <linux/linkage.h> >>>> >>>> DECLARE_GLOBAL_DATA_PTR; >>>> @@ -89,6 +90,15 @@ out: >>>> return ret; >>>> } >>>> >>>> +/* >>>> + * Allow unaligned memory access. >>>> + * >>>> + * This routine is overridden by architectures providing this feature. >>>> + */ >>>> +void __weak allow_unaligned(void) >>>> +{ >>>> +} >>>> + >>> >>> I'd prefer to guard the body of the function with an #ifdef CONFIG_ARM >>> so everyone knows why it's there. Then call straight into a function >>> provided in the ARM core code: >> >> The same visibility can be achieved with a comment. > > It's not as obvious. The default really should be to map memory as > cached and allow for unaligned accesses. > >> >>> >>> static void allow_unaligned(void) >>> { >>> /* On ARM we prohibit unaligned accesses by default, enable them here */ >>> #if defined(CONFIG_ARM) && !defined(CONFIG_ARM64) && >>> !defined(CONFIG_CPU_V7M) >>> mmu_enable_unaligned(); >>> #endif >>> } >> >> RISC-V also supports traps for unaligned access. > > Just because it supports them doesn't mean we should use them. AArch64 > also allows for unaligned access traps and I went through great length > to refactor the mm code in U-Boot to ensure that we do not trap. > >> Using architecture specific flags outside arch/ is a mess. >> We should not commit new sins but eliminate the existing deviations. >> >>> >>> And then in arch/arm/lib/cache-cp15.c: >>> >>> void mmu_enable_unaligned(void) >>> { >>> set_cr(get_cr() & ~CR_A); >>> } >> >> For some ARM architecture versions the bit is reserved and not used for >> unaligned access. No clue what this function would do in this case. > > Do you have pointers? Anything defined in the UEFI spec should have the bit. UEFI spec 2.7: <cite>2.3.5 AArch32 Platforms In addition, the invoking OSs can assume that unaligned access support is enabled if it is present in the processor.</cite> So the UEFI spec foresees processors supporting unaligned memory access and others that do not support it. > >> That is why I used a weak function that does nothing if the CPU does not >> support the flag. > > Any platform that uses cache-cp15 also supports the CR_A bit FWIW. So it > really belongs there.> > And again, I do not want to prettify a special hack for a broken > architecture. People should see warts when they're there. > > The real fix IMHO is to enable aligned accesses always, like we do on > any sane architecture. > Is this a typo: "enable aligned accesses"? Aligned access is always enabled. It is unaligned access that currently is not enabled in U-Boot. Regards Heinrich > > Alex >
On 04/04/2018 09:14 PM, Heinrich Schuchardt wrote: > On 04/04/2018 06:11 PM, Alexander Graf wrote: >> >> On 04.04.18 17:10, Heinrich Schuchardt wrote: >>> On 04/04/2018 02:32 PM, Alexander Graf wrote: >>>> >>>> On 03.04.18 21:59, Heinrich Schuchardt wrote: >>>>> The UEFI spec mandates that unaligned memory access should be enabled if >>>>> supported by the CPU architecture. >>>>> >>>>> This patch adds an empty weak function unaligned_access() that can be >>>>> overridden by an architecture specific routine. >>>>> >>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >>>>> --- >>>>> cmd/bootefi.c | 13 +++++++++++++ >>>>> include/asm-generic/unaligned.h | 3 +++ >>>>> 2 files changed, 16 insertions(+) >>>>> >>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >>>>> index ba258ac9f3..412e6519ba 100644 >>>>> --- a/cmd/bootefi.c >>>>> +++ b/cmd/bootefi.c >>>>> @@ -18,6 +18,7 @@ >>>>> #include <memalign.h> >>>>> #include <asm/global_data.h> >>>>> #include <asm-generic/sections.h> >>>>> +#include <asm-generic/unaligned.h> >>>>> #include <linux/linkage.h> >>>>> >>>>> DECLARE_GLOBAL_DATA_PTR; >>>>> @@ -89,6 +90,15 @@ out: >>>>> return ret; >>>>> } >>>>> >>>>> +/* >>>>> + * Allow unaligned memory access. >>>>> + * >>>>> + * This routine is overridden by architectures providing this feature. >>>>> + */ >>>>> +void __weak allow_unaligned(void) >>>>> +{ >>>>> +} >>>>> + >>>> I'd prefer to guard the body of the function with an #ifdef CONFIG_ARM >>>> so everyone knows why it's there. Then call straight into a function >>>> provided in the ARM core code: >>> The same visibility can be achieved with a comment. >> It's not as obvious. The default really should be to map memory as >> cached and allow for unaligned accesses. >> >>>> static void allow_unaligned(void) >>>> { >>>> /* On ARM we prohibit unaligned accesses by default, enable them here */ >>>> #if defined(CONFIG_ARM) && !defined(CONFIG_ARM64) && >>>> !defined(CONFIG_CPU_V7M) >>>> mmu_enable_unaligned(); >>>> #endif >>>> } >>> RISC-V also supports traps for unaligned access. >> Just because it supports them doesn't mean we should use them. AArch64 >> also allows for unaligned access traps and I went through great length >> to refactor the mm code in U-Boot to ensure that we do not trap. >> >>> Using architecture specific flags outside arch/ is a mess. >>> We should not commit new sins but eliminate the existing deviations. >>> >>>> And then in arch/arm/lib/cache-cp15.c: >>>> >>>> void mmu_enable_unaligned(void) >>>> { >>>> set_cr(get_cr() & ~CR_A); >>>> } >>> For some ARM architecture versions the bit is reserved and not used for >>> unaligned access. No clue what this function would do in this case. >> Do you have pointers? Anything defined in the UEFI spec should have the bit. > UEFI spec 2.7: > <cite>2.3.5 AArch32 Platforms > In addition, the invoking OSs can assume that unaligned access support > is enabled if it is present in the processor.</cite> > > So the UEFI spec foresees processors supporting unaligned memory access > and others that do not support it. I think the only corner case is Cortex-M, but that's not terribly high up on my priority list. And if that requires everything to be aligned, so be it. > >>> That is why I used a weak function that does nothing if the CPU does not >>> support the flag. >> Any platform that uses cache-cp15 also supports the CR_A bit FWIW. So it >> really belongs there.> >> And again, I do not want to prettify a special hack for a broken >> architecture. People should see warts when they're there. >> >> The real fix IMHO is to enable aligned accesses always, like we do on >> any sane architecture. >> > Is this a typo: "enable aligned accesses"? > > Aligned access is always enabled. It is unaligned access that currently > is not enabled in U-Boot. Yes, enable unaligned accesses of course :). Albert, this is your call I think. Would you be heavily opposed to Heinrich's initial patch? It really is the best option IMHO: https://patchwork.ozlabs.org/patch/893014/ Alex
On Thu, Apr 05, 2018 at 09:39:20AM +0200, Alexander Graf wrote: > On 04/04/2018 09:14 PM, Heinrich Schuchardt wrote: > >On 04/04/2018 06:11 PM, Alexander Graf wrote: > >> > >>On 04.04.18 17:10, Heinrich Schuchardt wrote: > >>>On 04/04/2018 02:32 PM, Alexander Graf wrote: > >>>> > >>>>On 03.04.18 21:59, Heinrich Schuchardt wrote: > >>>>>The UEFI spec mandates that unaligned memory access should be enabled if > >>>>>supported by the CPU architecture. > >>>>> > >>>>>This patch adds an empty weak function unaligned_access() that can be > >>>>>overridden by an architecture specific routine. > >>>>> > >>>>>Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > >>>>>--- > >>>>> cmd/bootefi.c | 13 +++++++++++++ > >>>>> include/asm-generic/unaligned.h | 3 +++ > >>>>> 2 files changed, 16 insertions(+) > >>>>> > >>>>>diff --git a/cmd/bootefi.c b/cmd/bootefi.c > >>>>>index ba258ac9f3..412e6519ba 100644 > >>>>>--- a/cmd/bootefi.c > >>>>>+++ b/cmd/bootefi.c > >>>>>@@ -18,6 +18,7 @@ > >>>>> #include <memalign.h> > >>>>> #include <asm/global_data.h> > >>>>> #include <asm-generic/sections.h> > >>>>>+#include <asm-generic/unaligned.h> > >>>>> #include <linux/linkage.h> > >>>>> DECLARE_GLOBAL_DATA_PTR; > >>>>>@@ -89,6 +90,15 @@ out: > >>>>> return ret; > >>>>> } > >>>>>+/* > >>>>>+ * Allow unaligned memory access. > >>>>>+ * > >>>>>+ * This routine is overridden by architectures providing this feature. > >>>>>+ */ > >>>>>+void __weak allow_unaligned(void) > >>>>>+{ > >>>>>+} > >>>>>+ > >>>>I'd prefer to guard the body of the function with an #ifdef CONFIG_ARM > >>>>so everyone knows why it's there. Then call straight into a function > >>>>provided in the ARM core code: > >>>The same visibility can be achieved with a comment. > >>It's not as obvious. The default really should be to map memory as > >>cached and allow for unaligned accesses. > >> > >>>>static void allow_unaligned(void) > >>>>{ > >>>>/* On ARM we prohibit unaligned accesses by default, enable them here */ > >>>>#if defined(CONFIG_ARM) && !defined(CONFIG_ARM64) && > >>>>!defined(CONFIG_CPU_V7M) > >>>> mmu_enable_unaligned(); > >>>>#endif > >>>>} > >>>RISC-V also supports traps for unaligned access. > >>Just because it supports them doesn't mean we should use them. AArch64 > >>also allows for unaligned access traps and I went through great length > >>to refactor the mm code in U-Boot to ensure that we do not trap. > >> > >>>Using architecture specific flags outside arch/ is a mess. > >>>We should not commit new sins but eliminate the existing deviations. > >>> > >>>>And then in arch/arm/lib/cache-cp15.c: > >>>> > >>>>void mmu_enable_unaligned(void) > >>>>{ > >>>> set_cr(get_cr() & ~CR_A); > >>>>} > >>>For some ARM architecture versions the bit is reserved and not used for > >>>unaligned access. No clue what this function would do in this case. > >>Do you have pointers? Anything defined in the UEFI spec should have the bit. > >UEFI spec 2.7: > ><cite>2.3.5 AArch32 Platforms > >In addition, the invoking OSs can assume that unaligned access support > >is enabled if it is present in the processor.</cite> > > > >So the UEFI spec foresees processors supporting unaligned memory access > >and others that do not support it. > > I think the only corner case is Cortex-M, but that's not terribly high up on > my priority list. And if that requires everything to be aligned, so be it. > > > > >>>That is why I used a weak function that does nothing if the CPU does not > >>>support the flag. > >>Any platform that uses cache-cp15 also supports the CR_A bit FWIW. So it > >>really belongs there.> > >>And again, I do not want to prettify a special hack for a broken > >>architecture. People should see warts when they're there. > >> > >>The real fix IMHO is to enable aligned accesses always, like we do on > >>any sane architecture. > >> > >Is this a typo: "enable aligned accesses"? > > > >Aligned access is always enabled. It is unaligned access that currently > >is not enabled in U-Boot. > > Yes, enable unaligned accesses of course :). > > Albert, this is your call I think. Would you be heavily opposed to > Heinrich's initial patch? It really is the best option IMHO: > > https://patchwork.ozlabs.org/patch/893014/ > > > Alex > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
On Thu, Apr 05, 2018 at 09:39:20AM +0200, Alexander Graf wrote: > On 04/04/2018 09:14 PM, Heinrich Schuchardt wrote: > >On 04/04/2018 06:11 PM, Alexander Graf wrote: > >> > >>On 04.04.18 17:10, Heinrich Schuchardt wrote: > >>>On 04/04/2018 02:32 PM, Alexander Graf wrote: > >>>> > >>>>On 03.04.18 21:59, Heinrich Schuchardt wrote: > >>>>>The UEFI spec mandates that unaligned memory access should be enabled if > >>>>>supported by the CPU architecture. > >>>>> > >>>>>This patch adds an empty weak function unaligned_access() that can be > >>>>>overridden by an architecture specific routine. > >>>>> > >>>>>Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > >>>>>--- > >>>>> cmd/bootefi.c | 13 +++++++++++++ > >>>>> include/asm-generic/unaligned.h | 3 +++ > >>>>> 2 files changed, 16 insertions(+) > >>>>> > >>>>>diff --git a/cmd/bootefi.c b/cmd/bootefi.c > >>>>>index ba258ac9f3..412e6519ba 100644 > >>>>>--- a/cmd/bootefi.c > >>>>>+++ b/cmd/bootefi.c > >>>>>@@ -18,6 +18,7 @@ > >>>>> #include <memalign.h> > >>>>> #include <asm/global_data.h> > >>>>> #include <asm-generic/sections.h> > >>>>>+#include <asm-generic/unaligned.h> > >>>>> #include <linux/linkage.h> > >>>>> DECLARE_GLOBAL_DATA_PTR; > >>>>>@@ -89,6 +90,15 @@ out: > >>>>> return ret; > >>>>> } > >>>>>+/* > >>>>>+ * Allow unaligned memory access. > >>>>>+ * > >>>>>+ * This routine is overridden by architectures providing this feature. > >>>>>+ */ > >>>>>+void __weak allow_unaligned(void) > >>>>>+{ > >>>>>+} > >>>>>+ > >>>>I'd prefer to guard the body of the function with an #ifdef CONFIG_ARM > >>>>so everyone knows why it's there. Then call straight into a function > >>>>provided in the ARM core code: > >>>The same visibility can be achieved with a comment. > >>It's not as obvious. The default really should be to map memory as > >>cached and allow for unaligned accesses. > >> > >>>>static void allow_unaligned(void) > >>>>{ > >>>>/* On ARM we prohibit unaligned accesses by default, enable them here */ > >>>>#if defined(CONFIG_ARM) && !defined(CONFIG_ARM64) && > >>>>!defined(CONFIG_CPU_V7M) > >>>> mmu_enable_unaligned(); > >>>>#endif > >>>>} > >>>RISC-V also supports traps for unaligned access. > >>Just because it supports them doesn't mean we should use them. AArch64 > >>also allows for unaligned access traps and I went through great length > >>to refactor the mm code in U-Boot to ensure that we do not trap. > >> > >>>Using architecture specific flags outside arch/ is a mess. > >>>We should not commit new sins but eliminate the existing deviations. > >>> > >>>>And then in arch/arm/lib/cache-cp15.c: > >>>> > >>>>void mmu_enable_unaligned(void) > >>>>{ > >>>> set_cr(get_cr() & ~CR_A); > >>>>} > >>>For some ARM architecture versions the bit is reserved and not used for > >>>unaligned access. No clue what this function would do in this case. > >>Do you have pointers? Anything defined in the UEFI spec should have the bit. > >UEFI spec 2.7: > ><cite>2.3.5 AArch32 Platforms > >In addition, the invoking OSs can assume that unaligned access support > >is enabled if it is present in the processor.</cite> > > > >So the UEFI spec foresees processors supporting unaligned memory access > >and others that do not support it. > > I think the only corner case is Cortex-M, but that's not terribly high up on > my priority list. And if that requires everything to be aligned, so be it. > > > > >>>That is why I used a weak function that does nothing if the CPU does not > >>>support the flag. > >>Any platform that uses cache-cp15 also supports the CR_A bit FWIW. So it > >>really belongs there.> > >>And again, I do not want to prettify a special hack for a broken > >>architecture. People should see warts when they're there. > >> > >>The real fix IMHO is to enable aligned accesses always, like we do on > >>any sane architecture. > >> > >Is this a typo: "enable aligned accesses"? > > > >Aligned access is always enabled. It is unaligned access that currently > >is not enabled in U-Boot. > > Yes, enable unaligned accesses of course :). > > Albert, this is your call I think. Would you be heavily opposed to > Heinrich's initial patch? It really is the best option IMHO: Let me try actually saying something this time. We had a large amount of back and forth over "unaligned access" over the last several years. Heinrich's is doing something we've expressly chosen not to do (and there's pages and pages of discussion in the archives). So the changes here to enter an EFI application in the way it expects need to be done for EFI as part of entering EFI. I would almost suggest something like a prepare_for_efi (and a matching unwind) function.
On 05/07/2018 11:53 PM, Tom Rini wrote: > On Thu, Apr 05, 2018 at 09:39:20AM +0200, Alexander Graf wrote: >> On 04/04/2018 09:14 PM, Heinrich Schuchardt wrote: >>> On 04/04/2018 06:11 PM, Alexander Graf wrote: >>>> >>>> On 04.04.18 17:10, Heinrich Schuchardt wrote: >>>>> On 04/04/2018 02:32 PM, Alexander Graf wrote: >>>>>> >>>>>> On 03.04.18 21:59, Heinrich Schuchardt wrote: >>>>>>> The UEFI spec mandates that unaligned memory access should be enabled if >>>>>>> supported by the CPU architecture. >>>>>>> >>>>>>> This patch adds an empty weak function unaligned_access() that can be >>>>>>> overridden by an architecture specific routine. >>>>>>> >>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >>>>>>> --- >>>>>>> cmd/bootefi.c | 13 +++++++++++++ >>>>>>> include/asm-generic/unaligned.h | 3 +++ >>>>>>> 2 files changed, 16 insertions(+) >>>>>>> >>>>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >>>>>>> index ba258ac9f3..412e6519ba 100644 >>>>>>> --- a/cmd/bootefi.c >>>>>>> +++ b/cmd/bootefi.c >>>>>>> @@ -18,6 +18,7 @@ >>>>>>> #include <memalign.h> >>>>>>> #include <asm/global_data.h> >>>>>>> #include <asm-generic/sections.h> >>>>>>> +#include <asm-generic/unaligned.h> >>>>>>> #include <linux/linkage.h> >>>>>>> DECLARE_GLOBAL_DATA_PTR; >>>>>>> @@ -89,6 +90,15 @@ out: >>>>>>> return ret; >>>>>>> } >>>>>>> +/* >>>>>>> + * Allow unaligned memory access. >>>>>>> + * >>>>>>> + * This routine is overridden by architectures providing this feature. >>>>>>> + */ >>>>>>> +void __weak allow_unaligned(void) >>>>>>> +{ >>>>>>> +} >>>>>>> + >>>>>> I'd prefer to guard the body of the function with an #ifdef CONFIG_ARM >>>>>> so everyone knows why it's there. Then call straight into a function >>>>>> provided in the ARM core code: >>>>> The same visibility can be achieved with a comment. >>>> It's not as obvious. The default really should be to map memory as >>>> cached and allow for unaligned accesses. >>>> >>>>>> static void allow_unaligned(void) >>>>>> { >>>>>> /* On ARM we prohibit unaligned accesses by default, enable them here */ >>>>>> #if defined(CONFIG_ARM) && !defined(CONFIG_ARM64) && >>>>>> !defined(CONFIG_CPU_V7M) >>>>>> mmu_enable_unaligned(); >>>>>> #endif >>>>>> } >>>>> RISC-V also supports traps for unaligned access. >>>> Just because it supports them doesn't mean we should use them. AArch64 >>>> also allows for unaligned access traps and I went through great length >>>> to refactor the mm code in U-Boot to ensure that we do not trap. >>>> >>>>> Using architecture specific flags outside arch/ is a mess. >>>>> We should not commit new sins but eliminate the existing deviations. >>>>> >>>>>> And then in arch/arm/lib/cache-cp15.c: >>>>>> >>>>>> void mmu_enable_unaligned(void) >>>>>> { >>>>>> set_cr(get_cr() & ~CR_A); >>>>>> } >>>>> For some ARM architecture versions the bit is reserved and not used for >>>>> unaligned access. No clue what this function would do in this case. >>>> Do you have pointers? Anything defined in the UEFI spec should have the bit. >>> UEFI spec 2.7: >>> <cite>2.3.5 AArch32 Platforms >>> In addition, the invoking OSs can assume that unaligned access support >>> is enabled if it is present in the processor.</cite> >>> >>> So the UEFI spec foresees processors supporting unaligned memory access >>> and others that do not support it. >> >> I think the only corner case is Cortex-M, but that's not terribly high up on >> my priority list. And if that requires everything to be aligned, so be it. >> >>> >>>>> That is why I used a weak function that does nothing if the CPU does not >>>>> support the flag. >>>> Any platform that uses cache-cp15 also supports the CR_A bit FWIW. So it >>>> really belongs there.> >>>> And again, I do not want to prettify a special hack for a broken >>>> architecture. People should see warts when they're there. >>>> >>>> The real fix IMHO is to enable aligned accesses always, like we do on >>>> any sane architecture. >>>> >>> Is this a typo: "enable aligned accesses"? >>> >>> Aligned access is always enabled. It is unaligned access that currently >>> is not enabled in U-Boot. >> >> Yes, enable unaligned accesses of course :). >> >> Albert, this is your call I think. Would you be heavily opposed to >> Heinrich's initial patch? It really is the best option IMHO: > > Let me try actually saying something this time. We had a large amount > of back and forth over "unaligned access" over the last several years. > Heinrich's is doing something we've expressly chosen not to do (and > there's pages and pages of discussion in the archives). So the changes > here to enter an EFI application in the way it expects need to be done > for EFI as part of entering EFI. I would almost suggest something like > a prepare_for_efi (and a matching unwind) function. > The bootefi command may serve to load an EFI driver binary which will stay active after returning to the U-Boot prompt. So we cannot disallow unaligned access after calling bootefi. So there is nothing we could "unwind". As this patch series moves allowing unaligned access to the bootefi call I suggest to procede with it. Best regards Heinrich
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index ba258ac9f3..412e6519ba 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -18,6 +18,7 @@ #include <memalign.h> #include <asm/global_data.h> #include <asm-generic/sections.h> +#include <asm-generic/unaligned.h> #include <linux/linkage.h> DECLARE_GLOBAL_DATA_PTR; @@ -89,6 +90,15 @@ out: return ret; } +/* + * Allow unaligned memory access. + * + * This routine is overridden by architectures providing this feature. + */ +void __weak allow_unaligned(void) +{ +} + /* * Set the load options of an image from an environment variable. * @@ -351,6 +361,9 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) efi_status_t r; void *fdt_addr; + /* Allow unaligned memory access */ + allow_unaligned(); + /* Initialize EFI drivers */ r = efi_init_obj_list(); if (r != EFI_SUCCESS) { diff --git a/include/asm-generic/unaligned.h b/include/asm-generic/unaligned.h index fd0255099a..3d33a5a063 100644 --- a/include/asm-generic/unaligned.h +++ b/include/asm-generic/unaligned.h @@ -20,4 +20,7 @@ #error invalid endian #endif +/* Allow unaligned memory access */ +void allow_unaligned(void); + #endif
The UEFI spec mandates that unaligned memory access should be enabled if supported by the CPU architecture. This patch adds an empty weak function unaligned_access() that can be overridden by an architecture specific routine. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- cmd/bootefi.c | 13 +++++++++++++ include/asm-generic/unaligned.h | 3 +++ 2 files changed, 16 insertions(+)