Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[Peter Maydell]

On Sat, 29 Jun 2024 at 04:17, David Gibson <david@gibson.dropbear.id.au> wrote:
>
> On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki <akihiko.odaki@daynix.com> 
> > wrote:
> > >
> > > FDT properties are aligned by 4 bytes, not 8 bytes.
> > >
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > ---
> > >  hw/ppc/vof.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> > > index e3b430a81f4f..b5b6514d79fc 100644
> > > --- a/hw/ppc/vof.c
> > > +++ b/hw/ppc/vof.c
> > > @@ -646,7 +646,7 @@ static void vof_dt_memory_available(void *fdt, GArray 
> > > *claimed, uint64_t base)
> > >      mem0_reg = fdt_getprop(fdt, offset, "reg", &proplen);
> > >      g_assert(mem0_reg && proplen == sizeof(uint32_t) * (ac + sc));
> > >      if (sc == 2) {
> > > -        mem0_end = be64_to_cpu(*(uint64_t *)(mem0_reg + sizeof(uint32_t) 
> > > * ac));
> > > +        mem0_end = ldq_be_p(mem0_reg + sizeof(uint32_t) * ac);
> > >      } else {
> > >          mem0_end = be32_to_cpu(*(uint32_t *)(mem0_reg + sizeof(uint32_t) 
> > > * ac));
> > >      }
> >
> > I did wonder if there was a better way to do what this is doing,
> > but neither we (in system/device_tree.c) nor libfdt seem to
> > provide one.
>
> libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> not an automatic aligned-or-unaligned helper.   Maybe we should add that?

fdt32_ld() and friends only do the "load from this bit of memory"
part, which we already have QEMU utility functions for (and which
are this patch uses).

This particular bit of code is dealing with an fdt property ("memory")
that is an array of (address, size) tuples where address and size
can independently be either 32 or 64 bits, and it wants the
size value of tuple 0. So the missing functionality is something at
a higher level than fdt32_ld() which would let you say "give me
tuple N field X" with some way to specify the tuple layout. (Which
is an awkward kind of API to write in C.)

Slightly less general, but for this case we could perhaps have
something like the getprop equivalent of qemu_fdt_setprop_sized_cells():

  uint64_t value_array[2];
  qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", &value_array,
                               ac, sc);
  /*
   * fills in value_array[0] with address, value_array[1] with size,
   * probably barfs if the varargs-list of cell-sizes doesn't
   * cover the whole property, similar to the current assert on
   * proplen.
   */
  mem0_end = value_array[0];

thanks
-- PMM