On Mon, Feb 14, 2022 at 04:20:13PM +0000, Alexandru Elisei 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.
But we don't really have that many tests and, while people have started
submitting more tests now, which is great, they're only submitting them
for AArch64. I wonder how much kvm-unit-tests is helping with AArch32
guest support, if at all.
Thanks,
drew
>
> Thanks,
> Alex
>
> >
> > Thanks,
> > drew
> >
> > >
> > > Thanks,
> > > Alex
> > >
> > > > *size = end - start;
> > > >
> > > > return 0;
> > > >
> > > >
> > > > To fix compilation on 32-bit arm.
> > > >
> > > >
> > > > And now merged through misc/queue.
> > > >
> > > > Thanks,
> > > > drew
> > > >
> > >
> >
>
