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. 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. 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." 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). 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
