Re: [PATCH 1/3] introduce unaligned.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.01.2021 11:45, Julien Grall wrote:
> On 18/01/2021 09:33, Jan Beulich wrote:
>> On 15.01.2021 12:27, Jan Beulich wrote:
>>> On 15.01.2021 12:13, Andrew Cooper wrote:
>>>> On 15/01/2021 10:05, Jan Beulich wrote:
>>>>> Rather than open-coding commonly used constructs in yet more places when
>>>>> pulling in zstd decompression support (and its xxhash prereq), pull out
>>>>> the custom bits into a commonly used header (for the hypervisor build;
>>>>> the tool stack and stubdom builds of libxenguest will still remain in
>>>>> need of similarly taking care of). For now this is limited to x86, where
>>>>> custom logic isn't needed (considering this is going to be used in init
>>>>> code only, even using alternatives patching to use MOVBE doesn't seem
>>>>> worthwhile).
>>>>>
>>>>> No change in generated code.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>> ---
>>>>> Iirc use of include/asm-generic/ was disliked, hence the generic header
>>>>> goes into include/xen/.
>>>>
>>>> Really?  I think its going to be the only sane way of fixing up some of
>>>> our header tangle.
>>>>
>>>> This series probably isn't the right place to fix this argument, so
>>>> Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>
>>> Thanks.
>>>
>>>> However, presumably we're going to want an ARM side of this imminently?
>>>
>>> Why? It's only used (and going to be further used) by code not
>>> built for Arm. So while it certainly would be nice for such a
>>> header to also appear there (and the x86-special casing going
>>> away in patch 2), it's not a strict requirement at this point.
>>> Therefore I'd prefer to leave this to the Arm maintainers (and
>>> probably for 4.16).
>>
>> I was wrong here, when it comes to an Arm64 build with ACPI
>> support enabled. xen/arch/arm/efi/efi-dom0.c has
>>
>> #define XZ_EXTERN STATIC
>> #include "../../../common/xz/crc32.c"
>>
>> in order to later do
>>
>>      xz_crc32_init();
>>      efi_sys_tbl->Hdr.CRC32 = xz_crc32((uint8_t *)efi_sys_tbl,
>>                                        efi_sys_tbl->Hdr.HeaderSize, 0);
>>
>> Urgh. Why in the world does xz code get re-used like this?
>> If we need generic crc32 support, such should imo live in
>> xen/lib/.
> 
> I suspect this was in order to make the EFI stub completely independent 
> to Xen (this is the case for Linux Arm). It turns out we now have an 
> hybrid model as we re-use pass some information in the DT and other via 
> variables.
> 
>>
>> So we have two possible courses of action: Eliminate this
>> unsuitable re-use of code, or introduce asm/unaligned.h
>> for Arm (or at least Arm64, in case it makes a difference)
>> right away.
> 
> EFI stub is only supported for Arm64. So it should be sufficient to 
> introduce the asm/unaligned.h on Arm64.
> 
> Note that on Arm32 we forbid unaligned access. So we may need two set of 
> helpers (I haven't looked at what the header does).

IOW it might be possible that Arm64 could re-use the x86 header.
Unless the price of unaligned accesses is much higher there. But
anyway, I'm about to commit the series with the issues addressed
using the 3rd approach, outlined earlier in a separate reply.

Jan