On Mon, Feb 14, 2022 at 05:01:40PM +0000, Marc Zyngier wrote:
> Hi all,
>
> On Mon, 14 Feb 2022 16:20:13 +0000,
> Alexandru Elisei <alexandru.elisei@xxxxxxx> 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@xxxxxxx>
> > > > > > ---
> > > > > > 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).
>
OK, let's keep 32-bit arm support in kvm-unit-tests, at least as long as
we can find hardware to test it with (I still have access to a mustang).
Does kvmtool support launching AArch32 guests? If so, then I suppose we
should also test kvmtool + 32-bit arm kvm-unit-tests.
Thanks,
drew
