| Message ID | 20220214120506.30617-1-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] lib/devicetree: Support 64 bit addresses for the initrd | expand |
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 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); 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; *size = end - start; return 0; To fix compilation on 32-bit arm. And now merged through misc/queue. Thanks, drew
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? > 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. Thanks, Alex > *size = end - start; > > return 0; > > > To fix compilation on 32-bit arm. > > > And now merged through misc/queue. > > Thanks, > drew >
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... Thanks, drew > > Thanks, > Alex > > > *size = end - start; > > > > return 0; > > > > > > To fix compilation on 32-bit arm. > > > > > > And now merged through misc/queue. > > > > Thanks, > > drew > > >
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. 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 > > > > > >
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@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. 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 > > > > > > > > > >
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.
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@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). > 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
On Tue, 15 Feb 2022 07:32:12 +0000, Andrew Jones <drjones@redhat.com> 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@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: > > > > 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). That HW will be with us for a *very* long time, given how popular things like A53 and A72 have been (and still are). > Does kvmtool support launching AArch32 guests? If so, then I suppose we > should also test kvmtool + 32-bit arm kvm-unit-tests. It does. Passing --aarch32 as a parameter results in AArch32 EL1 being selected. The only thing that doesn't work with kvmtool and this flag is the kvmtool-provided init when booting a 32bit kernel directly (we only carry a 64bit version). This should not affect unit tests, which are standalone. Thanks, M.
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@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). > > > > 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
On Tue, Feb 15, 2022 at 10:07:16AM +0000, Alexandru Elisei wrote: > > 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. > Excellent! BTW, I went ahead an pushed a patch to misc/queue to improve the initrd address stuff https://gitlab.com/rhdrjones/kvm-unit-tests/-/commit/6f8f74ed2d9953830a3c74669f25439d9ad68dec It may be necessary for you if kvmtool shares its fdt creation between aarch64 and aarch32 guests, emitting 8 byte initrd addresses for both, even though the aarch32 guest puts the fdt below 4G. Thanks, drew
Hi Drew, On Tue, Feb 15, 2022 at 01:53:00PM +0100, Andrew Jones wrote: > On Tue, Feb 15, 2022 at 10:07:16AM +0000, Alexandru Elisei wrote: > > > > 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. > > > > Excellent! > > BTW, I went ahead an pushed a patch to misc/queue to improve the initrd > address stuff > > https://gitlab.com/rhdrjones/kvm-unit-tests/-/commit/6f8f74ed2d9953830a3c74669f25439d9ad68dec > > It may be necessary for you if kvmtool shares its fdt creation between > aarch64 and aarch32 guests, emitting 8 byte initrd addresses for both, > even though the aarch32 guest puts the fdt below 4G. While trying your patch (it works, but with the caveat below), I remembered that kvmtool is not able to run kvm-unit-tests for arm. That's because the code is not relocatable (like it is for arm64) and the text address is hardcoded in the makefile. In past, to run the arm tests with kvmtool, I was doing this change: diff --git a/arm/Makefile.arm b/arm/Makefile.arm index 3a4cc6b26234..6c580b067413 100644 --- a/arm/Makefile.arm +++ b/arm/Makefile.arm @@ -14,7 +14,7 @@ CFLAGS += $(machine) CFLAGS += -mcpu=$(PROCESSOR) CFLAGS += -mno-unaligned-access -arch_LDFLAGS = -Ttext=40010000 +arch_LDFLAGS = -Ttext=80008000 define arch_elf_check = endef Any suggestions how to fix that? One way would be to change the LDFLAGS based on $TARGET. Another way would be to make the arm tests relocatable, I tried to do that in the past but I couldn't make any progress. Separate from that, I also tried to run the 32 bit arm tests with run_tests.sh. The runner uses qemu-system-arm (because $ARCH_NAME is arm in scripts/arch-run.bash::search_qemu_binary()), but uses kvm as the accelerator (because /dev/kvm exists in scrips/arch-run.bash::kvm_available()). This fails with the error: qemu-system-arm: -accel kvm: invalid accelerator kvm I don't think that's supposed to happen, as kvm_available() specifically returns true if $HOST = aarch64 and $ARCH = arm. Any suggestions? Thanks, Alex > > Thanks, > drew >
Hi Drew, On Tue, Feb 15, 2022 at 02:16:32PM +0000, Alexandru Elisei wrote: > Hi Drew, > > On Tue, Feb 15, 2022 at 01:53:00PM +0100, Andrew Jones wrote: > > On Tue, Feb 15, 2022 at 10:07:16AM +0000, Alexandru Elisei wrote: > > > > > > 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. > > > > > > > Excellent! > > > > BTW, I went ahead an pushed a patch to misc/queue to improve the initrd > > address stuff > > > > https://gitlab.com/rhdrjones/kvm-unit-tests/-/commit/6f8f74ed2d9953830a3c74669f25439d9ad68dec > > > > It may be necessary for you if kvmtool shares its fdt creation between > > aarch64 and aarch32 guests, emitting 8 byte initrd addresses for both, > > even though the aarch32 guest puts the fdt below 4G. > > While trying your patch (it works, but with the caveat below), I remembered that > kvmtool is not able to run kvm-unit-tests for arm. That's because the code is > not relocatable (like it is for arm64) and the text address is hardcoded in the > makefile. > > In past, to run the arm tests with kvmtool, I was doing this change: > > diff --git a/arm/Makefile.arm b/arm/Makefile.arm > index 3a4cc6b26234..6c580b067413 100644 > --- a/arm/Makefile.arm > +++ b/arm/Makefile.arm > @@ -14,7 +14,7 @@ CFLAGS += $(machine) > CFLAGS += -mcpu=$(PROCESSOR) > CFLAGS += -mno-unaligned-access > > -arch_LDFLAGS = -Ttext=40010000 > +arch_LDFLAGS = -Ttext=80008000 > > define arch_elf_check = > endef > > Any suggestions how to fix that? One way would be to change the LDFLAGS based on > $TARGET. Another way would be to make the arm tests relocatable, I tried to do > that in the past but I couldn't make any progress. > > Separate from that, I also tried to run the 32 bit arm tests with run_tests.sh. > The runner uses qemu-system-arm (because $ARCH_NAME is arm in > scripts/arch-run.bash::search_qemu_binary()), but uses kvm as the accelerator > (because /dev/kvm exists in scrips/arch-run.bash::kvm_available()). This fails > with the error: > > qemu-system-arm: -accel kvm: invalid accelerator kvm > > I don't think that's supposed to happen, as kvm_available() specifically returns > true if $HOST = aarch64 and $ARCH = arm. Any suggestions? I managed to run the arm tests on an arm64 machine using qemu-system-aarch64 (instead of qemu-system-arm) and passing -cpu host,aarch64=off. I'll try to make this into a patch. Thanks, Alex > > Thanks, > Alex > > > > > Thanks, > > drew > >
On Tue, Feb 15, 2022 at 02:16:32PM +0000, Alexandru Elisei wrote: > Hi Drew, > > On Tue, Feb 15, 2022 at 01:53:00PM +0100, Andrew Jones wrote: > > On Tue, Feb 15, 2022 at 10:07:16AM +0000, Alexandru Elisei wrote: > > > > > > 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. > > > > > > > Excellent! > > > > BTW, I went ahead an pushed a patch to misc/queue to improve the initrd > > address stuff > > > > https://gitlab.com/rhdrjones/kvm-unit-tests/-/commit/6f8f74ed2d9953830a3c74669f25439d9ad68dec > > > > It may be necessary for you if kvmtool shares its fdt creation between > > aarch64 and aarch32 guests, emitting 8 byte initrd addresses for both, > > even though the aarch32 guest puts the fdt below 4G. > > While trying your patch (it works, but with the caveat below), I remembered that > kvmtool is not able to run kvm-unit-tests for arm. That's because the code is > not relocatable (like it is for arm64) and the text address is hardcoded in the > makefile. > > In past, to run the arm tests with kvmtool, I was doing this change: > > diff --git a/arm/Makefile.arm b/arm/Makefile.arm > index 3a4cc6b26234..6c580b067413 100644 > --- a/arm/Makefile.arm > +++ b/arm/Makefile.arm > @@ -14,7 +14,7 @@ CFLAGS += $(machine) > CFLAGS += -mcpu=$(PROCESSOR) > CFLAGS += -mno-unaligned-access > > -arch_LDFLAGS = -Ttext=40010000 > +arch_LDFLAGS = -Ttext=80008000 > > define arch_elf_check = > endef > > Any suggestions how to fix that? One way would be to change the LDFLAGS based on > $TARGET. Another way would be to make the arm tests relocatable, I tried to do > that in the past but I couldn't make any progress. Ideally we'd eventually make it relocatable, but it's not worth moving mountains, so I'm OK with the LDFLAGS approach. > > Separate from that, I also tried to run the 32 bit arm tests with run_tests.sh. > The runner uses qemu-system-arm (because $ARCH_NAME is arm in > scripts/arch-run.bash::search_qemu_binary()), but uses kvm as the accelerator > (because /dev/kvm exists in scrips/arch-run.bash::kvm_available()). This fails > with the error: > > qemu-system-arm: -accel kvm: invalid accelerator kvm > > I don't think that's supposed to happen, as kvm_available() specifically returns > true if $HOST = aarch64 and $ARCH = arm. Any suggestions? > Ah, that's a kvm-unit-tests bug. Typically qemu-system-$ARCH_NAME is correct and, with TCG, qemu-system-arm runs the arm built unit tests fine. But, when KVM is enabled, the QEMU used should be qemu-system-$HOST. I don't think we want to change search_qemu_binary(), though, as that would require ACCEL being set first and may not apply to all architectures. Probably the best thing to do is fix this up in arm/run diff --git a/arm/run b/arm/run index 2153bd320751..ff18def43403 100755 --- a/arm/run +++ b/arm/run @@ -13,6 +13,10 @@ processor="$PROCESSOR" ACCEL=$(get_qemu_accelerator) || exit $? +if [ "$ACCEL" = "kvm" ] && [ -z "$QEMU" ]; then + QEMU=qemu-system-$HOST +fi + qemu=$(search_qemu_binary) || exit $? Thanks, drew
On Tue, Feb 15, 2022 at 03:22:00PM +0000, Alexandru Elisei wrote: > Hi Drew, > > On Tue, Feb 15, 2022 at 02:16:32PM +0000, Alexandru Elisei wrote: > > Hi Drew, > > > > On Tue, Feb 15, 2022 at 01:53:00PM +0100, Andrew Jones wrote: > > > On Tue, Feb 15, 2022 at 10:07:16AM +0000, Alexandru Elisei wrote: > > > > > > > > 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. > > > > > > > > > > Excellent! > > > > > > BTW, I went ahead an pushed a patch to misc/queue to improve the initrd > > > address stuff > > > > > > https://gitlab.com/rhdrjones/kvm-unit-tests/-/commit/6f8f74ed2d9953830a3c74669f25439d9ad68dec > > > > > > It may be necessary for you if kvmtool shares its fdt creation between > > > aarch64 and aarch32 guests, emitting 8 byte initrd addresses for both, > > > even though the aarch32 guest puts the fdt below 4G. > > > > While trying your patch (it works, but with the caveat below), I remembered that > > kvmtool is not able to run kvm-unit-tests for arm. That's because the code is > > not relocatable (like it is for arm64) and the text address is hardcoded in the > > makefile. > > > > In past, to run the arm tests with kvmtool, I was doing this change: > > > > diff --git a/arm/Makefile.arm b/arm/Makefile.arm > > index 3a4cc6b26234..6c580b067413 100644 > > --- a/arm/Makefile.arm > > +++ b/arm/Makefile.arm > > @@ -14,7 +14,7 @@ CFLAGS += $(machine) > > CFLAGS += -mcpu=$(PROCESSOR) > > CFLAGS += -mno-unaligned-access > > > > -arch_LDFLAGS = -Ttext=40010000 > > +arch_LDFLAGS = -Ttext=80008000 > > > > define arch_elf_check = > > endef > > > > Any suggestions how to fix that? One way would be to change the LDFLAGS based on > > $TARGET. Another way would be to make the arm tests relocatable, I tried to do > > that in the past but I couldn't make any progress. > > > > Separate from that, I also tried to run the 32 bit arm tests with run_tests.sh. > > The runner uses qemu-system-arm (because $ARCH_NAME is arm in > > scripts/arch-run.bash::search_qemu_binary()), but uses kvm as the accelerator > > (because /dev/kvm exists in scrips/arch-run.bash::kvm_available()). This fails > > with the error: > > > > qemu-system-arm: -accel kvm: invalid accelerator kvm > > > > I don't think that's supposed to happen, as kvm_available() specifically returns > > true if $HOST = aarch64 and $ARCH = arm. Any suggestions? > > I managed to run the arm tests on an arm64 machine using qemu-system-aarch64 > (instead of qemu-system-arm) Yup > and passing -cpu host,aarch64=off. I'll try to make > this into a patch. The '-cpu host,aarch64=off' part should already be getting added for you by the arm/run script. Hmm, it just occurred to me that your kvmtool work may not be using arm/run. Anyway, arm/run may serve as inspiration. Thanks, drew
Hi Drew, On Tue, Feb 15, 2022 at 04:50:37PM +0100, Andrew Jones wrote: > On Tue, Feb 15, 2022 at 02:16:32PM +0000, Alexandru Elisei wrote: > > Hi Drew, > > > > On Tue, Feb 15, 2022 at 01:53:00PM +0100, Andrew Jones wrote: > > > On Tue, Feb 15, 2022 at 10:07:16AM +0000, Alexandru Elisei wrote: > > > > > > > > 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. > > > > > > > > > > Excellent! > > > > > > BTW, I went ahead an pushed a patch to misc/queue to improve the initrd > > > address stuff > > > > > > https://gitlab.com/rhdrjones/kvm-unit-tests/-/commit/6f8f74ed2d9953830a3c74669f25439d9ad68dec > > > > > > It may be necessary for you if kvmtool shares its fdt creation between > > > aarch64 and aarch32 guests, emitting 8 byte initrd addresses for both, > > > even though the aarch32 guest puts the fdt below 4G. > > > > While trying your patch (it works, but with the caveat below), I remembered that > > kvmtool is not able to run kvm-unit-tests for arm. That's because the code is > > not relocatable (like it is for arm64) and the text address is hardcoded in the > > makefile. > > > > In past, to run the arm tests with kvmtool, I was doing this change: > > > > diff --git a/arm/Makefile.arm b/arm/Makefile.arm > > index 3a4cc6b26234..6c580b067413 100644 > > --- a/arm/Makefile.arm > > +++ b/arm/Makefile.arm > > @@ -14,7 +14,7 @@ CFLAGS += $(machine) > > CFLAGS += -mcpu=$(PROCESSOR) > > CFLAGS += -mno-unaligned-access > > > > -arch_LDFLAGS = -Ttext=40010000 > > +arch_LDFLAGS = -Ttext=80008000 > > > > define arch_elf_check = > > endef > > > > Any suggestions how to fix that? One way would be to change the LDFLAGS based on > > $TARGET. Another way would be to make the arm tests relocatable, I tried to do > > that in the past but I couldn't make any progress. > > Ideally we'd eventually make it relocatable, but it's not worth moving > mountains, so I'm OK with the LDFLAGS approach. > > > > > Separate from that, I also tried to run the 32 bit arm tests with run_tests.sh. > > The runner uses qemu-system-arm (because $ARCH_NAME is arm in > > scripts/arch-run.bash::search_qemu_binary()), but uses kvm as the accelerator > > (because /dev/kvm exists in scrips/arch-run.bash::kvm_available()). This fails > > with the error: > > > > qemu-system-arm: -accel kvm: invalid accelerator kvm > > > > I don't think that's supposed to happen, as kvm_available() specifically returns > > true if $HOST = aarch64 and $ARCH = arm. Any suggestions? > > > > Ah, that's a kvm-unit-tests bug. Typically qemu-system-$ARCH_NAME is > correct and, with TCG, qemu-system-arm runs the arm built unit tests fine. > But, when KVM is enabled, the QEMU used should be qemu-system-$HOST. > > I don't think we want to change search_qemu_binary(), though, as that > would require ACCEL being set first and may not apply to all > architectures. Probably the best thing to do is fix this up in arm/run Definitely, this should go in arm/run. > > diff --git a/arm/run b/arm/run > index 2153bd320751..ff18def43403 100755 > --- a/arm/run > +++ b/arm/run > @@ -13,6 +13,10 @@ processor="$PROCESSOR" > ACCEL=$(get_qemu_accelerator) || > exit $? > > +if [ "$ACCEL" = "kvm" ] && [ -z "$QEMU" ]; then > + QEMU=qemu-system-$HOST > +fi Looking at get_qemu_accelerator, I think it returns true when /dev/kvm exists, which means that get_qemu_accelerator can return "kvm" when trying to run the arm64 tests on an arm machine with KVM. But in this case, it's impossible to run the tests using KVM. And I also think there should be some checking that aarch64=off works, as there are arm64 CPUs which don't support AArch32 (like Cortex-A510). Thanks, Alex > + > qemu=$(search_qemu_binary) || > exit $? > > > Thanks, > drew >
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; }
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(-)