Re: [kvm-unit-tests PATCH] lib/devicetree: Support 64 bit addresses for the initrd

[Marc Zyngier <maz@kernel.org>]

Hi all,

On Mon, 14 Feb 2022 16:20:13 +0000,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi Drew,
> 
> (CC'ing Marc, he know more about 32 bit guest support than me)
> 
> On Mon, Feb 14, 2022 at 03:24:44PM +0100, Andrew Jones wrote:
> > On Mon, Feb 14, 2022 at 02:06:04PM +0000, Alexandru Elisei wrote:
> > > Hi Drew,
> > > 
> > > On Mon, Feb 14, 2022 at 02:52:26PM +0100, Andrew Jones wrote:
> > > > On Mon, Feb 14, 2022 at 12:05:06PM +0000, Alexandru Elisei wrote:
> > > > > The "linux,initrd-start" and "linux,initrd-end" properties encode the start
> > > > > and end address of the initrd. The size of the address is encoded in the
> > > > > root node #address-cells property and can be 1 cell (32 bits) or 2 cells
> > > > > (64 bits). Add support for parsing a 64 bit address.
> > > > > 
> > > > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > > > > ---
> > > > >  lib/devicetree.c | 18 +++++++++++++-----
> > > > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/lib/devicetree.c b/lib/devicetree.c
> > > > > index 409d18bedbba..7cf64309a912 100644
> > > > > --- a/lib/devicetree.c
> > > > > +++ b/lib/devicetree.c
> > > > > @@ -288,7 +288,7 @@ int dt_get_default_console_node(void)
> > > > >  int dt_get_initrd(const char **initrd, u32 *size)
> > > > >  {
> > > > >  	const struct fdt_property *prop;
> > > > > -	const char *start, *end;
> > > > > +	u64 start, end;
> > > > >  	int node, len;
> > > > >  	u32 *data;
> > > > >  
> > > > > @@ -303,7 +303,11 @@ int dt_get_initrd(const char **initrd, u32 *size)
> > > > >  	if (!prop)
> > > > >  		return len;
> > > > >  	data = (u32 *)prop->data;
> > > > > -	start = (const char *)(unsigned long)fdt32_to_cpu(*data);
> > > > > +	start = fdt32_to_cpu(*data);
> > > > > +	if (len == 8) {
> > > > > +		data++;
> > > > > +		start = (start << 32) | fdt32_to_cpu(*data);
> > > > > +	}
> > > > >  
> > > > >  	prop = fdt_get_property(fdt, node, "linux,initrd-end", &len);
> > > > >  	if (!prop) {
> > > > > @@ -311,10 +315,14 @@ int dt_get_initrd(const char **initrd, u32 *size)
> > > > >  		return len;
> > > > >  	}
> > > > >  	data = (u32 *)prop->data;
> > > > > -	end = (const char *)(unsigned long)fdt32_to_cpu(*data);
> > > > > +	end = fdt32_to_cpu(*data);
> > > > > +	if (len == 8) {
> > > > > +		data++;
> > > > > +		end = (end << 32) | fdt32_to_cpu(*data);
> > > > > +	}
> > > > >  
> > > > > -	*initrd = start;
> > > > > -	*size = (unsigned long)end - (unsigned long)start;
> > > > > +	*initrd = (char *)start;
> > > > > +	*size = end - start;
> > > > >  
> > > > >  	return 0;
> > > > >  }
> > > > > -- 
> > > > > 2.35.1
> > > > >
> > > > 
> > > > I added this patch on
> > > 
> > > Thanks for the quick reply!
> > > 
> > > > 
> > > > diff --git a/lib/devicetree.c b/lib/devicetree.c
> > > > index 7cf64309a912..fa8399a7513d 100644
> > > > --- a/lib/devicetree.c
> > > > +++ b/lib/devicetree.c
> > > > @@ -305,6 +305,7 @@ int dt_get_initrd(const char **initrd, u32 *size)
> > > >         data = (u32 *)prop->data;
> > > >         start = fdt32_to_cpu(*data);
> > > >         if (len == 8) {
> > > > +               assert(sizeof(long) == 8);
> > > 
> > > I'm sketchy about arm with LPAE, but wouldn't it be legal to have here a 64
> > > bit address, even if the architecture is 32 bits? Or was the assert added
> > > more because kvm-unit-tests doesn't support LPAE on arm?
> > 
> > It's possible, but only if we choose to manage it. We're (I'm) lazy and
> > require physical addresses to fit in the pointers, at least for the test
> > framework. Of course a unit test can feel free to play around with larger
> > physical addresses if it wants to.
> > 
> > > 
> > > >                 data++;
> > > >                 start = (start << 32) | fdt32_to_cpu(*data);
> > > >         }
> > > > @@ -321,7 +322,7 @@ int dt_get_initrd(const char **initrd, u32 *size)
> > > >                 end = (end << 32) | fdt32_to_cpu(*data);
> > > >         }
> > > >  
> > > > -       *initrd = (char *)start;
> > > > +       *initrd = (char *)(unsigned long)start;
> > > 
> > > My bad here, I forgot to test on arm. Tested your fix and the compilation
> > > error goes away.
> > 
> > I'm actually kicking myself a bit for the hasty fix, because the assert
> > would be better done at the end and written something like this
> > 
> >  assert(sizeof(long) == 8 || !(end >> 32));
> > 
> > I'm not sure it's worth adding another patch on top for that now, though.
> > By the lack of new 32-bit arm unit tests getting submitted, I'm not even
> > sure it's worth maintaining 32-bit arm at all...
> 
> As far as I know, 32 bit guests are still very much supported and
> maintained for KVM, so I think it would still be very useful to have the
> tests.

I can't force people to write additional tests (or even start writing
the first one), but I'd like to reaffirm that AArch32 support still is
a first class citizen when it comes to KVM/arm64.

It has been tremendously useful even in the very recent past to debug
issues that were plaguing bare metal Linux, and i don't plan to get
rid of it anytime soon (TBH, it is too small to even be noticeable).

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.