Hi Drew,
On Tue, Feb 15, 2022 at 08:32:12AM +0100, Andrew Jones wrote:
> 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.
It does indeed support AArch32 guests (via the --aarch32 command line option,
like Marc said). I usually run the 32 bit tests with kvmtool when testing.
I've started working on the next iteration of the kvmtool test
runner support series, I'll do my best to make sure kvmtool wll be able to run
the tests when kvm-unit-tests has been configured with --arch=arm.
Thanks,
Alex
