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

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

[Alexandru Elisei]

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

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

[Andrew Jones]

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

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

[Alexandru Elisei]

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
> 

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

[Andrew Jones]

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
> > 
> 

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

[Alexandru Elisei]

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
> > > 
> > 
> 

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

[Andrew Jones]

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
> > > > 
> > > 
> > 
> 

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

[Marc Zyngier]

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.

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

[Andrew Jones]

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

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

[Marc Zyngier]

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.

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

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

[Alexandru Elisei]

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

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

[Andrew Jones]

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

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

[Alexandru Elisei]

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
> 

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

[Alexandru Elisei]

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
> > 

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

[Andrew Jones]

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

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

[Andrew Jones]

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

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

[Alexandru Elisei]

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
>