On Mon, Jul 08, 2024 at 04:59:30PM +0100, Peter Maydell wrote: > On Mon, 8 Jul 2024 at 08:49, Nicholas Piggin <npig...@gmail.com> wrote: > > > > On Sun Jul 7, 2024 at 9:46 AM AEST, David Gibson wrote: > > > On Sat, Jul 06, 2024 at 11:37:08AM +0100, Peter Maydell wrote: > > > > On Fri, 5 Jul 2024 at 06:13, David Gibson <da...@gibson.dropbear.id.au> > > > > wrote: > > > > > Huh.. well I'm getting different impressions of what the problem > > > > > actually is from what I initially read versus Peter Maydell's > > > > > comments, so I don't really know what to think. > > > > > > > > > > If it's just the load then fdt32_ld() etc. already exist. Or is it > > > > > really such a hot path that unconditionally handling unaligned > > > > > accesses isn't tenable? > > > > > > > > The specific problem here is that the code as written tries to > > > > cast a not-aligned-enough pointer to uint64_t* to do the load, > > > > which is UB. > > > > > > Ah... and I'm assuming it's the cast itself which triggers the UB, not > > > just dereferencing it. > > > > Oh it's just the cast itself that is UB? Looks like that's true. > > Interesting gcc and clang don't flag it, I guess they care about > > warning on practical breakage first. > > Er, I was speaking a bit vaguely there, don't take my word for > it without going and looking at the text of the C standard.
Sure.
> What I *meant* was that the practical problem here is that we
> really do dereference a pointer for a 64-bit load when the
> pointer isn't necessarily 64-bit-aligned.
From the qemu point of view, yes. And theoretically, the fix is easy,
since libfdt provides fdt32_ld() etc. for exactly this use case. But..
> As it happens, C99 says that it is the cast that is UB:
> section 6.3.2.3 para 7 says:
> "A pointer to an object or incomplete type may be converted to
> a pointer to a different object or incomplete type. If the
> resulting pointer is not correctly aligned for the pointed-to
> type, the behavior is undefined. Otherwise, when converted back
> again, the result shall compare equal to the original pointer."
.. this makes fdt32_ld() etc. unusable by design.
> Presumably this is envisaging the possibility of a pointer cast
> being a destructive operation somehow, such that e.g. a uint64_t*
> can only represent 64-bit-aligned values. But I bet QEMU does
> a lot of casting pointers around that might fall foul of this
> rule, so I'm not particularly worried about trying to clean up
> that kind of thing (until/unless analysers start warning about
> it, in which case we have a specific set of things to clean up).
Fair enough from the qemu point of view. However, this unusable by
design interface was written by me as part of a library I maintain, so
it certainly worries *me*.
> What I care about from the point of view of this patch
> is that we fix the actually-broken-on-some-real-hardware problem
> of doing the load as a misaligned access. My vote would be for
> "take Akihiko's patch as-is, rather than gating fixing the bug
> on deciding on an improvement/change to the fdt API or our
> wrappers of it".
>
> thanks
> -- PMM
>
--
David Gibson (he or they) | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you, not the other way
| around.
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
