[kvm-unit-tests PATCH v1 0/2] Fix some compilation issues on 32bit

[kvm-unit-tests PATCH v1 0/2] Fix some compilation issues on 32bit

[Claudio Imbrenda]

Two small patches to fix compilation issues on 32bit:

one for a typo in x86/cstart

one for a thinko in lib/alloc_page

notice that there is another patch for the lib/alloc_page issue floating
around, this patch is an alternative to that one

Claudio Imbrenda (2):
  x86/cstart: Fix compilation issue in 32 bit mode
  lib/alloc_page: Fix compilation issue on 32bit archs

 lib/alloc_page.c | 5 +++--
 x86/cstart.S     | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

-- 
2.26.2

[kvm-unit-tests PATCH v1 1/2] x86/cstart: Fix compilation issue in 32 bit mode

[Claudio Imbrenda]

Fix a typo in x86/cstart.S so that 32bit code can be compiled again on x86.

Fixes: d86ef58519645 ("cstart: do not assume CR4 starts as zero")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 x86/cstart.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x86/cstart.S b/x86/cstart.S
index e63e4e2..c0efc5f 100644
--- a/x86/cstart.S
+++ b/x86/cstart.S
@@ -125,7 +125,7 @@ start:
         jmpl $8, $start32
 
 prepare_32:
-	mov %(1 << 4), %eax // pse
+	mov $(1 << 4), %eax // pse
 	mov %eax, %cr4
 
 	mov $pt, %eax
-- 
2.26.2

[kvm-unit-tests PATCH v1 2/2] lib/alloc_page: Fix compilation issue on 32bit archs

[Claudio Imbrenda]

The assert in lib/alloc_page is hardcoded to long, and size_t is just
an int on 32 bit architectures.

Adding a cast makes the compiler happy.

Fixes: 73f4b202beb39 ("lib/alloc_page: change some parameter types")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/alloc_page.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index fa3c527..617b003 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -29,11 +29,12 @@ void free_pages(void *mem, size_t size)
 	assert_msg((unsigned long) mem % PAGE_SIZE == 0,
 		   "mem not page aligned: %p", mem);
 
-	assert_msg(size % PAGE_SIZE == 0, "size not page aligned: %#lx", size);
+	assert_msg(size % PAGE_SIZE == 0, "size not page aligned: %#lx",
+		(unsigned long)size);
 
 	assert_msg(size == 0 || (uintptr_t)mem == -size ||
 		   (uintptr_t)mem + size > (uintptr_t)mem,
-		   "mem + size overflow: %p + %#lx", mem, size);
+		   "mem + size overflow: %p + %#lx", mem, (unsigned long)size);
 
 	if (size == 0) {
 		freelist = NULL;
-- 
2.26.2

Re: [kvm-unit-tests PATCH v1 2/2] lib/alloc_page: Fix compilation issue on 32bit archs

[Thomas Huth]

On 14/07/2020 13.09, Claudio Imbrenda wrote:
> The assert in lib/alloc_page is hardcoded to long, and size_t is just
> an int on 32 bit architectures.
> 
> Adding a cast makes the compiler happy.
> 
> Fixes: 73f4b202beb39 ("lib/alloc_page: change some parameter types")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  lib/alloc_page.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> index fa3c527..617b003 100644
> --- a/lib/alloc_page.c
> +++ b/lib/alloc_page.c
> @@ -29,11 +29,12 @@ void free_pages(void *mem, size_t size)
>  	assert_msg((unsigned long) mem % PAGE_SIZE == 0,
>  		   "mem not page aligned: %p", mem);
>  
> -	assert_msg(size % PAGE_SIZE == 0, "size not page aligned: %#lx", size);
> +	assert_msg(size % PAGE_SIZE == 0, "size not page aligned: %#lx",
> +		(unsigned long)size);
>  
>  	assert_msg(size == 0 || (uintptr_t)mem == -size ||
>  		   (uintptr_t)mem + size > (uintptr_t)mem,
> -		   "mem + size overflow: %p + %#lx", mem, size);
> +		   "mem + size overflow: %p + %#lx", mem, (unsigned long)size);

Looking at lib/printf.c, it seems like it also supports %z ... have you
tried?

 Thomas

Re: [kvm-unit-tests PATCH v1 2/2] lib/alloc_page: Fix compilation issue on 32bit archs

[Claudio Imbrenda]

On Tue, 14 Jul 2020 13:20:16 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 14/07/2020 13.09, Claudio Imbrenda wrote:
> > The assert in lib/alloc_page is hardcoded to long, and size_t is
> > just an int on 32 bit architectures.
> > 
> > Adding a cast makes the compiler happy.
> > 
> > Fixes: 73f4b202beb39 ("lib/alloc_page: change some parameter types")
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > ---
> >  lib/alloc_page.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> > index fa3c527..617b003 100644
> > --- a/lib/alloc_page.c
> > +++ b/lib/alloc_page.c
> > @@ -29,11 +29,12 @@ void free_pages(void *mem, size_t size)
> >  	assert_msg((unsigned long) mem % PAGE_SIZE == 0,
> >  		   "mem not page aligned: %p", mem);
> >  
> > -	assert_msg(size % PAGE_SIZE == 0, "size not page aligned:
> > %#lx", size);
> > +	assert_msg(size % PAGE_SIZE == 0, "size not page aligned:
> > %#lx",
> > +		(unsigned long)size);
> >  
> >  	assert_msg(size == 0 || (uintptr_t)mem == -size ||
> >  		   (uintptr_t)mem + size > (uintptr_t)mem,
> > -		   "mem + size overflow: %p + %#lx", mem, size);
> > +		   "mem + size overflow: %p + %#lx", mem,
> > (unsigned long)size);  
> 
> Looking at lib/printf.c, it seems like it also supports %z ... have
> you tried?

no, but in hindsight I should have. It's probably a much cleaner
solution. I'll try and respin.

Re: [kvm-unit-tests PATCH v1 2/2] lib/alloc_page: Fix compilation issue on 32bit archs

[Sean Christopherson]

On Tue, Jul 14, 2020 at 01:41:23PM +0200, Claudio Imbrenda wrote:
> On Tue, 14 Jul 2020 13:20:16 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
> > On 14/07/2020 13.09, Claudio Imbrenda wrote:
> > > The assert in lib/alloc_page is hardcoded to long, and size_t is
> > > just an int on 32 bit architectures.
> > > 
> > > Adding a cast makes the compiler happy.
> > > 
> > > Fixes: 73f4b202beb39 ("lib/alloc_page: change some parameter types")
> > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > > ---
> > >  lib/alloc_page.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> > > index fa3c527..617b003 100644
> > > --- a/lib/alloc_page.c
> > > +++ b/lib/alloc_page.c
> > > @@ -29,11 +29,12 @@ void free_pages(void *mem, size_t size)
> > >  	assert_msg((unsigned long) mem % PAGE_SIZE == 0,
> > >  		   "mem not page aligned: %p", mem);
> > >  
> > > -	assert_msg(size % PAGE_SIZE == 0, "size not page aligned:
> > > %#lx", size);
> > > +	assert_msg(size % PAGE_SIZE == 0, "size not page aligned:
> > > %#lx",
> > > +		(unsigned long)size);
> > >  
> > >  	assert_msg(size == 0 || (uintptr_t)mem == -size ||
> > >  		   (uintptr_t)mem + size > (uintptr_t)mem,
> > > -		   "mem + size overflow: %p + %#lx", mem, size);
> > > +		   "mem + size overflow: %p + %#lx", mem,
> > > (unsigned long)size);  
> > 
> > Looking at lib/printf.c, it seems like it also supports %z ... have
> > you tried?
> 
> no, but in hindsight I should have. It's probably a much cleaner
> solution. I'll try and respin.

I'm not opposed to using size_t, but if we go that route then the entirety
of alloc_page.c should be converted to size_t.  As is, there is code like:

	void free_pages_by_order(void *mem, unsigned int order)
	{
        	free_pages(mem, 1ul << (order + PAGE_SHIFT));
	}

and

	void *alloc_pages(unsigned int order)
	{
		...

		/* Looking for a run of length (1 << order). */
		unsigned long run = 0;
		const unsigned long n = 1ul << order;
		const unsigned long align_mask = (n << PAGE_SHIFT) - 1;
		void *run_start = NULL;
		void *run_prev = NULL;
		unsigned long run_next_pa = 0;
		unsigned long pa;

		assert(order < sizeof(unsigned long) * 8);

		...
	}

that very explicitly uses 'unsigned long' for the size.

Re: [kvm-unit-tests PATCH v1 2/2] lib/alloc_page: Fix compilation issue on 32bit archs

[Claudio Imbrenda]

On Tue, 14 Jul 2020 07:05:34 -0700
Sean Christopherson <sean.j.christopherson@intel.com> wrote:

> On Tue, Jul 14, 2020 at 01:41:23PM +0200, Claudio Imbrenda wrote:
> > On Tue, 14 Jul 2020 13:20:16 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> > > On 14/07/2020 13.09, Claudio Imbrenda wrote:  
> > > > The assert in lib/alloc_page is hardcoded to long, and size_t is
> > > > just an int on 32 bit architectures.
> > > > 
> > > > Adding a cast makes the compiler happy.
> > > > 
> > > > Fixes: 73f4b202beb39 ("lib/alloc_page: change some parameter
> > > > types") Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > > > ---
> > > >  lib/alloc_page.c | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> > > > index fa3c527..617b003 100644
> > > > --- a/lib/alloc_page.c
> > > > +++ b/lib/alloc_page.c
> > > > @@ -29,11 +29,12 @@ void free_pages(void *mem, size_t size)
> > > >  	assert_msg((unsigned long) mem % PAGE_SIZE == 0,
> > > >  		   "mem not page aligned: %p", mem);
> > > >  
> > > > -	assert_msg(size % PAGE_SIZE == 0, "size not page
> > > > aligned: %#lx", size);
> > > > +	assert_msg(size % PAGE_SIZE == 0, "size not page
> > > > aligned: %#lx",
> > > > +		(unsigned long)size);
> > > >  
> > > >  	assert_msg(size == 0 || (uintptr_t)mem == -size ||
> > > >  		   (uintptr_t)mem + size > (uintptr_t)mem,
> > > > -		   "mem + size overflow: %p + %#lx", mem,
> > > > size);
> > > > +		   "mem + size overflow: %p + %#lx", mem,
> > > > (unsigned long)size);    
> > > 
> > > Looking at lib/printf.c, it seems like it also supports %z ...
> > > have you tried?  
> > 
> > no, but in hindsight I should have. It's probably a much cleaner
> > solution. I'll try and respin.  
> 
> I'm not opposed to using size_t, but if we go that route then the
> entirety of alloc_page.c should be converted to size_t.  As is, there
> is code like:
> 
> 	void free_pages_by_order(void *mem, unsigned int order)
> 	{
>         	free_pages(mem, 1ul << (order + PAGE_SHIFT));
> 	}
> 
> and
> 
> 	void *alloc_pages(unsigned int order)
> 	{
> 		...
> 
> 		/* Looking for a run of length (1 << order). */
> 		unsigned long run = 0;
> 		const unsigned long n = 1ul << order;
> 		const unsigned long align_mask = (n << PAGE_SHIFT) -
> 1; void *run_start = NULL;
> 		void *run_prev = NULL;
> 		unsigned long run_next_pa = 0;
> 		unsigned long pa;
> 
> 		assert(order < sizeof(unsigned long) * 8);
> 
> 		...
> 	}
> 
> that very explicitly uses 'unsigned long' for the size.

don't worry, those won't stay there for long :)

once this patch series has stabilized, I'm going to send a more radical
rewrite of the allocators