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

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 4414 bytes --]

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@arndb.de>
>>>>>> Origin: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
>>>>>> 803f4e1eab7a
>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>
>>>>> 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 #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]