[kvm-unit-tests PATCH 0/2] Two fixes for KVM unit tests

[kvm-unit-tests PATCH 0/2] Two fixes for KVM unit tests

[Pierre Morel]

Hello,

I am not sure about the first one, cscope, it works for me
but may be someone has a better solution.

The second one fixes a bug of mine...

Regards,
Pierre

Pierre Morel (2):
  Makefile: Fix cscope
  s390x: fixing I/O memory allocation

 Makefile              | 2 +-
 lib/s390x/malloc_io.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.25.1

[kvm-unit-tests PATCH 1/2] Makefile: Fix cscope

[Pierre Morel]

In Linux, cscope uses a wrong directory.
Simply search from the directory where the make is started.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index f7b9f28c..c8b0d74f 100644
--- a/Makefile
+++ b/Makefile
@@ -119,7 +119,7 @@ cscope: cscope_dirs = lib lib/libfdt lib/linux $(TEST_DIR) $(ARCH_LIBDIRS) lib/a
 cscope:
 	$(RM) ./cscope.*
 	find -L $(cscope_dirs) -maxdepth 1 \
-		-name '*.[chsS]' -exec realpath --relative-base=$(PWD) {} \; | sort -u > ./cscope.files
+		-name '*.[chsS]' -exec realpath --relative-base=. {} \; | sort -u > ./cscope.files
 	cscope -bk
 
 .PHONY: tags
-- 
2.25.1

Re: [kvm-unit-tests PATCH 1/2] Makefile: Fix cscope

[Thomas Huth]

On 25/08/2021 18.20, Pierre Morel wrote:
> In Linux, cscope uses a wrong directory.
> Simply search from the directory where the make is started.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   Makefile | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index f7b9f28c..c8b0d74f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -119,7 +119,7 @@ cscope: cscope_dirs = lib lib/libfdt lib/linux $(TEST_DIR) $(ARCH_LIBDIRS) lib/a
>   cscope:
>   	$(RM) ./cscope.*
>   	find -L $(cscope_dirs) -maxdepth 1 \
> -		-name '*.[chsS]' -exec realpath --relative-base=$(PWD) {} \; | sort -u > ./cscope.files
> +		-name '*.[chsS]' -exec realpath --relative-base=. {} \; | sort -u > ./cscope.files

Why is $PWD not pointing to the same location as "." ? Are you doing in-tree 
or out-of-tree builds?

  Thomas

Re: [kvm-unit-tests PATCH 1/2] Makefile: Fix cscope

[Pierre Morel]

On 8/26/21 7:07 AM, Thomas Huth wrote:
> On 25/08/2021 18.20, Pierre Morel wrote:
>> In Linux, cscope uses a wrong directory.
>> Simply search from the directory where the make is started.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   Makefile | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Makefile b/Makefile
>> index f7b9f28c..c8b0d74f 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -119,7 +119,7 @@ cscope: cscope_dirs = lib lib/libfdt lib/linux 
>> $(TEST_DIR) $(ARCH_LIBDIRS) lib/a
>>   cscope:
>>       $(RM) ./cscope.*
>>       find -L $(cscope_dirs) -maxdepth 1 \
>> -        -name '*.[chsS]' -exec realpath --relative-base=$(PWD) {} \; 
>> | sort -u > ./cscope.files
>> +        -name '*.[chsS]' -exec realpath --relative-base=. {} \; | 
>> sort -u > ./cscope.files
> 
> Why is $PWD not pointing to the same location as "." ? Are you doing 
> in-tree or out-of-tree builds?
> 
>   Thomas
> 

In tree.
That is the point, why is PWD indicating void ?
I use a bash on s390x.
inside bash PWD shows current directory
GNU Make is 4.2.1 on Ubuntu 18.04

This works on X with redhat and GNU make 3.82

This happens on s390x since:
51b8f0b1 2017-11-23 Andrew Jones Makefile: fix cscope target

So I add Andrew as CC, I did forgot to do before.


Regards,
Pierre




-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH 1/2] Makefile: Fix cscope

[Andrew Jones]

On Fri, Aug 27, 2021 at 11:26:38AM +0200, Pierre Morel wrote:
> 
> 
> On 8/26/21 7:07 AM, Thomas Huth wrote:
> > On 25/08/2021 18.20, Pierre Morel wrote:
> > > In Linux, cscope uses a wrong directory.
> > > Simply search from the directory where the make is started.
> > > 
> > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > > ---
> > >   Makefile | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/Makefile b/Makefile
> > > index f7b9f28c..c8b0d74f 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -119,7 +119,7 @@ cscope: cscope_dirs = lib lib/libfdt lib/linux
> > > $(TEST_DIR) $(ARCH_LIBDIRS) lib/a
> > >   cscope:
> > >       $(RM) ./cscope.*
> > >       find -L $(cscope_dirs) -maxdepth 1 \
> > > -        -name '*.[chsS]' -exec realpath --relative-base=$(PWD) {}
> > > \; | sort -u > ./cscope.files
> > > +        -name '*.[chsS]' -exec realpath --relative-base=. {} \; |
> > > sort -u > ./cscope.files
> > 
> > Why is $PWD not pointing to the same location as "." ? Are you doing
> > in-tree or out-of-tree builds?
> > 
> >   Thomas
> > 
> 
> In tree.
> That is the point, why is PWD indicating void ?

If you do 'env' I'll bet you'll see something like

...
PWD=
...

> I use a bash on s390x.
> inside bash PWD shows current directory
> GNU Make is 4.2.1 on Ubuntu 18.04
> 
> This works on X with redhat and GNU make 3.82
> 
> This happens on s390x since:
> 51b8f0b1 2017-11-23 Andrew Jones Makefile: fix cscope target

No surprise there, that's when the $(PWD) use was first introduced.

> 
> So I add Andrew as CC, I did forgot to do before.
>

I'll send a patch changing $(PWD) to $(shell pwd)

Thanks,
drew

Re: [kvm-unit-tests PATCH 1/2] Makefile: Fix cscope

[Paolo Bonzini]

On 27/08/21 12:22, Andrew Jones wrote:
>> 51b8f0b1 2017-11-23 Andrew Jones Makefile: fix cscope target
> No surprise there, that's when the $(PWD) use was first introduced.
> 
>> So I add Andrew as CC, I did forgot to do before.
>>
> I'll send a patch changing $(PWD) to $(shell pwd)

I could not find the patch using $(CURDIR) in my mailbox, though I found
it on spinics.net.  I fudged the following

 From 164507376abae4be15b0f65aa14d56f179198a99 Mon Sep 17 00:00:00 2001
From: Andrew Jones <drjones@redhat.com>
Date: Fri, 27 Aug 2021 12:31:15 +0200
Subject: [PATCH kvm-unit-tests] Makefile: Don't trust PWD

It's possible that PWD is already set to something which isn't
the full path of the current working directory. Let's use $(CURDIR)
instead, which is always correct.

Suggested-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

diff --git a/Makefile b/Makefile
index f7b9f28..6792b93 100644
--- a/Makefile
+++ b/Makefile
@@ -119,7 +119,7 @@ cscope: cscope_dirs = lib lib/libfdt lib/linux $(TEST_DIR) $(ARCH_LIBDIRS) lib/a
  cscope:
  	$(RM) ./cscope.*
  	find -L $(cscope_dirs) -maxdepth 1 \
-		-name '*.[chsS]' -exec realpath --relative-base=$(PWD) {} \; | sort -u > ./cscope.files
+		-name '*.[chsS]' -exec realpath --relative-base=$(CURDIR) {} \; | sort -u > ./cscope.files
  	cscope -bk
  
  .PHONY: tags

and queued it.

Paolo

Re: [kvm-unit-tests PATCH 1/2] Makefile: Fix cscope

[Andrew Jones]

On Mon, Sep 20, 2021 at 03:27:11PM +0200, Paolo Bonzini wrote:
> On 27/08/21 12:22, Andrew Jones wrote:
> > > 51b8f0b1 2017-11-23 Andrew Jones Makefile: fix cscope target
> > No surprise there, that's when the $(PWD) use was first introduced.
> > 
> > > So I add Andrew as CC, I did forgot to do before.
> > > 
> > I'll send a patch changing $(PWD) to $(shell pwd)
> 
> I could not find the patch using $(CURDIR) in my mailbox, though I found
> it on spinics.net.  I fudged the following
> 
> From 164507376abae4be15b0f65aa14d56f179198a99 Mon Sep 17 00:00:00 2001
> From: Andrew Jones <drjones@redhat.com>
> Date: Fri, 27 Aug 2021 12:31:15 +0200
> Subject: [PATCH kvm-unit-tests] Makefile: Don't trust PWD
> 
> It's possible that PWD is already set to something which isn't
> the full path of the current working directory. Let's use $(CURDIR)
> instead, which is always correct.
> 
> Suggested-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> diff --git a/Makefile b/Makefile
> index f7b9f28..6792b93 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -119,7 +119,7 @@ cscope: cscope_dirs = lib lib/libfdt lib/linux $(TEST_DIR) $(ARCH_LIBDIRS) lib/a
>  cscope:
>  	$(RM) ./cscope.*
>  	find -L $(cscope_dirs) -maxdepth 1 \
> -		-name '*.[chsS]' -exec realpath --relative-base=$(PWD) {} \; | sort -u > ./cscope.files
> +		-name '*.[chsS]' -exec realpath --relative-base=$(CURDIR) {} \; | sort -u > ./cscope.files
>  	cscope -bk
>  .PHONY: tags
> 
> and queued it.

Hi Paolo,

You'll get a conflict when you go to merge because I already did it :-)

commit 3d4eb24cb5b4de6c26f79b849fe2818d5315a691 (origin/misc/queue, misc/queue)
Author: Andrew Jones <drjones@redhat.com>
Date:   Fri Aug 27 12:25:27 2021 +0200

    Makefile: Don't trust PWD
    
    PWD comes from the environment and it's possible that it's already
    set to something which isn't the full path of the current working
    directory. Use the make variable $(CURDIR) instead.
    
    Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
    Reviewed-by: Thomas Huth <thuth@redhat.com>
    Suggested-by: Thomas Huth <thuth@redhat.com>
    Signed-off-by: Andrew Jones <drjones@redhat.com>



misc/queue is something I recently invented for stuff like this in order
to help lighten your load a bit.

Thanks,
drew

Re: [kvm-unit-tests PATCH 1/2] Makefile: Fix cscope

[Paolo Bonzini]

On 20/09/21 16:10, Andrew Jones wrote:
> Hi Paolo,
> 
> You'll get a conflict when you go to merge because I already did it :-)
> 
> commit 3d4eb24cb5b4de6c26f79b849fe2818d5315a691 (origin/misc/queue, misc/queue)
> Author: Andrew Jones <drjones@redhat.com>
> Date:   Fri Aug 27 12:25:27 2021 +0200
> 
>      Makefile: Don't trust PWD
>      
>      PWD comes from the environment and it's possible that it's already
>      set to something which isn't the full path of the current working
>      directory. Use the make variable $(CURDIR) instead.
>      
>      Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
>      Reviewed-by: Thomas Huth <thuth@redhat.com>
>      Suggested-by: Thomas Huth <thuth@redhat.com>
>      Signed-off-by: Andrew Jones <drjones@redhat.com>
> 
> 
> 
> misc/queue is something I recently invented for stuff like this in order
> to help lighten your load a bit.

Ok, are you going to create a merge request?

Re: [kvm-unit-tests PATCH 1/2] Makefile: Fix cscope

[Andrew Jones]

On Mon, Sep 20, 2021 at 08:04:02PM +0200, Paolo Bonzini wrote:
> On 20/09/21 16:10, Andrew Jones wrote:
> > Hi Paolo,
> > 
> > You'll get a conflict when you go to merge because I already did it :-)
> > 
> > commit 3d4eb24cb5b4de6c26f79b849fe2818d5315a691 (origin/misc/queue, misc/queue)
> > Author: Andrew Jones <drjones@redhat.com>
> > Date:   Fri Aug 27 12:25:27 2021 +0200
> > 
> >      Makefile: Don't trust PWD
> >      PWD comes from the environment and it's possible that it's already
> >      set to something which isn't the full path of the current working
> >      directory. Use the make variable $(CURDIR) instead.
> >      Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
> >      Reviewed-by: Thomas Huth <thuth@redhat.com>
> >      Suggested-by: Thomas Huth <thuth@redhat.com>
> >      Signed-off-by: Andrew Jones <drjones@redhat.com>
> > 
> > 
> > 
> > misc/queue is something I recently invented for stuff like this in order
> > to help lighten your load a bit.
> 
> Ok, are you going to create a merge request?
>

I did. And I merged it.

https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/merge_requests/16

Thanks,
drew 

Re: [kvm-unit-tests PATCH 1/2] Makefile: Fix cscope

[Thomas Huth]

On 27/08/2021 11.26, Pierre Morel wrote:
> 
> 
> On 8/26/21 7:07 AM, Thomas Huth wrote:
>> On 25/08/2021 18.20, Pierre Morel wrote:
>>> In Linux, cscope uses a wrong directory.
>>> Simply search from the directory where the make is started.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   Makefile | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/Makefile b/Makefile
>>> index f7b9f28c..c8b0d74f 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -119,7 +119,7 @@ cscope: cscope_dirs = lib lib/libfdt lib/linux 
>>> $(TEST_DIR) $(ARCH_LIBDIRS) lib/a
>>>   cscope:
>>>       $(RM) ./cscope.*
>>>       find -L $(cscope_dirs) -maxdepth 1 \
>>> -        -name '*.[chsS]' -exec realpath --relative-base=$(PWD) {} \; | 
>>> sort -u > ./cscope.files
>>> +        -name '*.[chsS]' -exec realpath --relative-base=. {} \; | sort 
>>> -u > ./cscope.files
>>
>> Why is $PWD not pointing to the same location as "." ? Are you doing 
>> in-tree or out-of-tree builds?
>>
>>   Thomas
>>
> 
> In tree.
> That is the point, why is PWD indicating void ?
> I use a bash on s390x.
> inside bash PWD shows current directory
> GNU Make is 4.2.1 on Ubuntu 18.04

Hmm, looking at the code twice, $(PWD) is used a a Make variable, not as a 
shell variable. For using a shell variable, it should use double $$ instead ...
So yes, this is broken since $(PWD) does not seem to be an official Make 
variable. There is $(CURDIR) which could do the job, too, but I guess "." is 
just fine as well. Andrew, what do you think?

  Thomas

[kvm-unit-tests PATCH 2/2] s390x: fixing I/O memory allocation

[Pierre Morel]

The allocator allocate pages it follows the size must be rounded
to pages before the allocation.

Fixes: b0fe3988 "s390x: define UV compatible I/O allocation"

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/malloc_io.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
index 78582eac..080fc694 100644
--- a/lib/s390x/malloc_io.c
+++ b/lib/s390x/malloc_io.c
@@ -41,7 +41,7 @@ static void unshare_pages(void *p, int count)
 
 void *alloc_io_mem(int size, int flags)
 {
-	int order = get_order(size >> PAGE_SHIFT);
+	int order = get_order(PAGE_ALIGN(size) >> PAGE_SHIFT);
 	void *p;
 	int n;
 
@@ -62,7 +62,7 @@ void *alloc_io_mem(int size, int flags)
 
 void free_io_mem(void *p, int size)
 {
-	int order = get_order(size >> PAGE_SHIFT);
+	int order = get_order(PAGE_ALIGN(size) >> PAGE_SHIFT);
 
 	assert(IS_ALIGNED((uintptr_t)p, PAGE_SIZE));
 
-- 
2.25.1

Re: [kvm-unit-tests PATCH 2/2] s390x: fixing I/O memory allocation

[Claudio Imbrenda]

On Wed, 25 Aug 2021 18:20:21 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> The allocator allocate pages it follows the size must be rounded
> to pages before the allocation.
> 
> Fixes: b0fe3988 "s390x: define UV compatible I/O allocation"
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  lib/s390x/malloc_io.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
> index 78582eac..080fc694 100644
> --- a/lib/s390x/malloc_io.c
> +++ b/lib/s390x/malloc_io.c
> @@ -41,7 +41,7 @@ static void unshare_pages(void *p, int count)
>  
>  void *alloc_io_mem(int size, int flags)
>  {
> -	int order = get_order(size >> PAGE_SHIFT);
> +	int order = get_order(PAGE_ALIGN(size) >> PAGE_SHIFT);
>  	void *p;
>  	int n;
>  
> @@ -62,7 +62,7 @@ void *alloc_io_mem(int size, int flags)
>  
>  void free_io_mem(void *p, int size)
>  {
> -	int order = get_order(size >> PAGE_SHIFT);
> +	int order = get_order(PAGE_ALIGN(size) >> PAGE_SHIFT);
>  
>  	assert(IS_ALIGNED((uintptr_t)p, PAGE_SIZE));
>  

Re: [kvm-unit-tests PATCH 2/2] s390x: fixing I/O memory allocation

[Pierre Morel]

On 8/25/21 6:31 PM, Claudio Imbrenda wrote:
> On Wed, 25 Aug 2021 18:20:21 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> The allocator allocate pages it follows the size must be rounded
>> to pages before the allocation.
>>
>> Fixes: b0fe3988 "s390x: define UV compatible I/O allocation"
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> 
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> 


Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen