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

[Julien Grall <julien@xxxxxxx>]

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.

Cheers,

--
Julien Grall