Re: [PATCH 1/2] xen: make include/xen/unaligned.h usable on all architectures

[Juergen Gross <jgross@xxxxxxxx>]

On 05.12.23 14:46, Julien Grall wrote:
> On 05/12/2023 13:41, Juergen Gross wrote:
> > On 05.12.23 14:31, Julien Grall wrote:
> > > Hi Juergen,
> > >
> > > On 05/12/2023 12:39, Juergen Gross wrote:
> > > > On 05.12.23 12:53, Julien Grall wrote:
> > > > > Hi Juergen,
> > > > >
> > > > > On 05/12/2023 10:07, Juergen Gross wrote:
> > > > > > Instead of defining get_unaligned() and put_unaligned() in a way that
> > > > > > is only supporting architectures allowing unaligned accesses, use the
> > > > > > same approach as the Linux kernel and let the compiler do the
> > > > > > decision how to generate the code for probably unaligned data accesses.
> > > > > >
> > > > > > Update include/xen/unaligned.h from include/asm-generic/unaligned.h of
> > > > > > the Linux kernel.
> > > > > >
> > > > > > The generated code has been checked to be the same on x86.
> > > > > >
> > > > > > Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> > > > > > Origin: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
> > > > > > 803f4e1eab7a
> > > > > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > > > Can you outline your end goal? At least on arm32, I believe this will 
> > > > > result to abort because event if the architecture support unaligned access, 
> > > > > we are preventing them on Arm32.
> > > > I need something like that in Xen tools for supporting packed data accesses
> > > > on the 9pfs ring page, so I looked into the hypervisor for related support.
> > > Did we really introduce an ABI requiring unaligned access??? Or is this 
> > > something you are coming up with?
> > This is the 9pfs protocol (see [1]).
> Urgh :(.
>
> > > Anyway, IIRC Linux allows unaligned access. So the problem I am describing is 
> > > only for the hypervisor. Although, I would like to point out that unaligned 
> > > access has no atomicity guarantee. I assume this is not going to be a concern 
> > > for you?
> > Correct.
> >
> > > > I guess for arm32 using -mno-unaligned-access when building should avoid any
> > > > unaligned accesses?
> > > I am not sure. This is implies the compiler will be able to infer that the 
> > > access will be unaligned. Is this always the case?
> > This should happen through the "__packed" attribute on the access macros. As
> > e.g. MIPS doesn't support unaligned accesses, but is working with those access
> > macros in the Linux kernel, I suspect the attribute is doing its job.
> Someone will need to dig a bit deeper to confirm and also the impact on the rest 
> of the hypervisor.
> > > Anyway, given you don't seem to have a use-case yet, I would simply to 
> > > consider to surround the declaration with an a config which can be selected 
> > > if unaligned access is supported.
> > Like in xen/common/lzo.c et al?
> Just to clarify, I am suggesting to add in unaligned.h:
>
> #ifdef CONFIG_HAS_UNALIGNED_ACCESS
>
> your definitions
>
> #endif
>
> And then for X86, select CONFIG_HAS_UNALIGNED_ACCESS.
>
>   Those are compiled with CONFIG_X86 only
> > today,
> > but I guess other archs might need the decompressors in future, too.
> Possibly yes. But my point is that you don't have to solve the problem today. 
> Yet I don't think it is wise to allow the header to be used on arm32 until we 
> have done some investigation.
> And to clarify, I am not asking you to do the investigation.
I've done a quick verification using gcc 7.5.

Using -mno-unaligned-access for 32-bit Arm seems to do the job:

#include <xen/unaligned.h>
int tst(const unsigned short *in)
{
    return get_unaligned(in);
}

results in:

00000000 <tst>:
   0:   e52db004        push    {fp}            @ (str fp, [sp, #-4]!)
   4:   e28db000        add     fp, sp, #0
   8:   e5d03000        ldrb    r3, [r0]
   c:   e5d00001        ldrb    r0, [r0, #1]
  10:   e1830400        orr     r0, r3, r0, lsl #8
  14:   e28bd000        add     sp, fp, #0
  18:   e49db004        pop     {fp}            @ (ldr fp, [sp], #4)
  1c:   e12fff1e        bx      lr

Without the -mno-unaligned-access I'm getting:

00000000 <tst>:
   0:   e52db004        push    {fp}            @ (str fp, [sp, #-4]!)
   4:   e28db000        add     fp, sp, #0
   8:   e1d000b0        ldrh    r0, [r0]
   c:   e28bd000        add     sp, fp, #0
  10:   e49db004        pop     {fp}            @ (ldr fp, [sp], #4)
  14:   e12fff1e        bx      lr


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature