Re: [PATCH] LoongArch: Add unaligned access support

[Huacai Chen <chenhuacai@kernel.org>]

Hi, Arnd,

On Mon, Oct 17, 2022 at 3:12 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Sun, Oct 16, 2022, at 3:34 PM, Huacai Chen wrote:
> > Loongson-2 series (Loongson-2K500, Loongson-2K1000) don't support
> > unaligned access in hardware, while Loongson-3 series (Loongson-3A5000,
> > Loongson-3C5000) are configurable whether support unaligned access in
> > hardware. This patch add unaligned access emulation for those LoongArch
> > processors without hardware support.
> >
> > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
>
> What does the Loongarch ELF ABI say about this? On most architectures,
> C compilers are not allowed to produce unaligned accesses for standard
> compliant source code, the only way you'd get this is when casting
> a an unaligned (e.g. char*) pointer to another type with higher alignment
> requirement.
Some unaligned accesses are observed from the kernel network stack, it
seems related to whether the packet aligns to IP header or MAC header.
And, gcc has a -mstrict-align parameter, if without this, there are
unaligned instructions.

>
> > +/* sysctl hooks */
> > +int unaligned_enabled __read_mostly = 1;     /* Enabled by default */
> > +int no_unaligned_warning __read_mostly = 1;  /* Only 1 warning by default */
>
> The comment says 'sysctl', the implementation has a debugfs interface.
Originally "enabled", "warning" and "counters" are all debugfs
interfaces, then you told me to use sysctl. Now in this version
"enabled" and "warning" are converted to sysctl, but there are no
existing "counters" sysctl.

Huacai

>
> > +#ifdef CONFIG_DEBUG_FS
> > +static int __init debugfs_unaligned(void)
> > +{
> > +     struct dentry *d;
> > +
> > +     d = debugfs_create_dir("loongarch", NULL);
> > +     if (!d)
> > +             return -ENOMEM;
> > +
> > +     debugfs_create_u32("unaligned_instructions_user",
> > +                             S_IRUGO, d, &unaligned_instructions_user);
> > +     debugfs_create_u32("unaligned_instructions_kernel",
> > +                             S_IRUGO, d, &unaligned_instructions_kernel);
> > +
> > +     return 0;
> > +}
> > +arch_initcall(debugfs_unaligned);
> > +#endif
>
> The debugfs interface does not sound like a good way to do this.
> Overall, my feeling is that for a new architecture we should not
> introduce this at all but instead provide a way to diagnose and
> fix user space, since we do not have to keep compatibility with
> broken binaries that worked in the past.
>
> If the ELF ABI actually allows compilers to produce unaligned
> accesses for correct code, there should at least be a more generic
> way of enabling this that follows what other architectures do.
> We are already somewhat inconsistent there between architectures,
> but I don't think anything else uses debugfs here.
>
>      Arnd