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

From: Julien Grall <julien@xen.org>
To: Arnd Bergmann <arnd@arndb.de>, Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Juergen Gross <jgross@suse.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH 1/2] xen: make include/xen/unaligned.h usable on all architectures
Date: Tue, 5 Dec 2023 14:19:09 +0000	[thread overview]
Message-ID: <fcdab8c1-b0ea-4d7a-943e-2180b13234fd@xen.org> (raw)
In-Reply-To: <5a05e194-28d1-409d-9f45-ca4de0fc78c8@app.fastmail.com>

Hi Arnd,

Thanks for the answer.

On 05/12/2023 14:10, Arnd Bergmann wrote:
> On Tue, Dec 5, 2023, at 15:01, Julien Grall wrote:
>> On 05/12/2023 13:59, Jan Beulich wrote:
>>> On 05.12.2023 14:46, Julien Grall wrote:
>>>> On 05/12/2023 13:41, Juergen Gross wrote:
>>>>> On 05.12.23 14:31, Julien Grall wrote:
>>>>>> 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
>>>
>>> But that would be wrong: HAS_UNALIGNED_ACCESS would be there to indicate
>>> one does _not_ need any special accessors.
>>
>> I am guessing you are disagreeing on the name rather than the concept?
>> If so, what about CONFIG_UNALIGNED_ACCESS_ALLOWED?
> 
> This would repeat the mistake that we had in Linux in the
> past (and still do in some drivers): Simply dereferencing
> a misaligned pointer is always a bug, even on architectures
> that have efficient unaligned load/store instructions,
> because C makes this undefined behavior and gcc has
> optimizations that assume e.g. 'int *' to never have
> the lower two bits set [1].

Just to clarify, I haven't suggested to use 'int *'. My point was more 
that I don't think that the helpers would work as-is on arm32 because
even if the ISA allows a direct access, we are setting the bit in SCTLR 
to disable unaligned access.

As Juergen is proposing a common header, then I could ask him to do the 
work to confirm that the helpers properly work on arm32. But I think 
this is unfair.

I also don't have the time to chase to look at it and this is not yet 
used in code reached by Arm. Hence my suggestion for now to protect the 
code so if someone tries to use them, then they know that it doesn't 
(yet) work on Arm.

Cheers,

-- 
Julien Grall