[kvm-unit-tests,v4,2/3] s390x: define UV compatible I/O allocation

Commit Message

Pierre Morel Jan. 21, 2021, 9:13 a.m. UTC

To centralize the memory allocation for I/O we define
the alloc_io_page/free_io_page functions which share the I/O
memory with the host in case the guest runs with
protected virtualization.

Signed-off-by: Pierre Morel <[e-mail]>
---
 MAINTAINERS           |  1 +
 lib/s390x/malloc_io.c | 70 +++++++++++++++++++++++++++++++++++++++++++
 lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
 s390x/Makefile        |  1 +
 4 files changed, 117 insertions(+)
 create mode 100644 lib/s390x/malloc_io.c
 create mode 100644 lib/s390x/malloc_io.h

Comments

Thomas Huth Jan. 21, 2021, 9:32 a.m. UTC | #1

On 21/01/2021 10.13, Pierre Morel wrote:
> To centralize the memory allocation for I/O we define
> the alloc_io_page/free_io_page functions which share the I/O
> memory with the host in case the guest runs with
> protected virtualization.
> 
> Signed-off-by: Pierre Morel <[e-mail]>
> ---
>   MAINTAINERS           |  1 +
>   lib/s390x/malloc_io.c | 70 +++++++++++++++++++++++++++++++++++++++++++
>   lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
>   s390x/Makefile        |  1 +
>   4 files changed, 117 insertions(+)
>   create mode 100644 lib/s390x/malloc_io.c
>   create mode 100644 lib/s390x/malloc_io.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 54124f6..89cb01e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -82,6 +82,7 @@ M: Thomas Huth <[e-mail]>
>   M: David Hildenbrand <[e-mail]>
>   M: Janosch Frank <[e-mail]>
>   R: Cornelia Huck <[e-mail]>
> +R: Pierre Morel <[e-mail]>
>   L: [e-mail]
>   L: [e-mail]
>   F: s390x/*
> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
> new file mode 100644
> index 0000000..bfe8c6a
> --- /dev/null
> +++ b/lib/s390x/malloc_io.c
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0

"GPL-2.0" is deprecated according to https://spdx.org/licenses/ ... please 
use "GPL-2.0-only" instead.

> +/*
> + * I/O page allocation
> + *
> + * Copyright (c) 2021 IBM Corp
> + *
> + * Authors:
> + *  Pierre Morel <[e-mail]>
> + *
> + * Using this interface provide host access to the allocated pages in
> + * case the guest is a secure guest.
> + * This is needed for I/O buffers.
> + *
> + */
> +#include <libcflat.h>
> +#include <asm/page.h>
> +#include <asm/uv.h>
> +#include <malloc_io.h>
> +#include <alloc_page.h>
> +#include <asm/facility.h>
> +
> +static int share_pages(void *p, int count)
> +{
> +	int i = 0;
> +
> +	for (i = 0; i < count; i++, p += PAGE_SIZE)
> +		if (uv_set_shared((unsigned long)p))
> +			return i;

Just a matter of taste, but you could replace the "return i" here also with 
a "break" since you're returning i below anyway.

> +	return i;
> +}
> +
> +static void unshare_pages(void *p, int count)
> +{
> +	int i;
> +
> +	for (i = count; i > 0; i--, p += PAGE_SIZE)
> +		uv_remove_shared((unsigned long)p);
> +}
> +
> +void *alloc_io_pages(int size, int flags)

I still think the naming or size parameter is confusing here. If I read 
something like alloc_io_pages(), I'd expect a "num_pages" parameter. So if 
you want to keep the "size" in bytes, I'd suggest to rename the function to 
"alloc_io_mem" instead.

> +{
> +	int order = (size >> PAGE_SHIFT);

I think this is wrong. According to the description of alloc_pages_flag, it 
allocates "1ull << order" pages.
So you likely want to do this instead here:

         int order = get_order(size >> PAGE_SHIFT);

> +	void *p;
> +	int n;
> +
> +	assert(size);
> +
> +	p = alloc_pages_flags(order, AREA_DMA31 | flags);
> +	if (!p || !test_facility(158))
> +		return p;
> +
> +	n = share_pages(p, 1 << order);
> +	if (n == 1 << order)
> +		return p;
> +
> +	unshare_pages(p, n);
> +	free_pages(p);
> +	return NULL;
> +}
> +
> +void free_io_pages(void *p, int size)
> +{
> +	int order = size >> PAGE_SHIFT;

dito?

> +	assert(IS_ALIGNED((uintptr_t)p, PAGE_SIZE));
> +
> +	if (test_facility(158))
> +		unshare_pages(p, 1 << order);
> +	free_pages(p);
> +}
> diff --git a/lib/s390x/malloc_io.h b/lib/s390x/malloc_io.h
> new file mode 100644
> index 0000000..494dfe9
> --- /dev/null
> +++ b/lib/s390x/malloc_io.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

GPL-2.0-only please.

> +/*
> + * I/O allocations
> + *
> + * Copyright (c) 2021 IBM Corp
> + *
> + * Authors:
> + *  Pierre Morel <[e-mail]>
> + *
> + */
> +#ifndef _S390X_MALLOC_IO_H_
> +#define _S390X_MALLOC_IO_H_
> +
> +/*
> + * Allocates a page aligned page bound range of contiguous real or
> + * absolute memory in the DMA31 region large enough to contain size
> + * bytes.
> + * If Protected Virtualization facility is present, shares the pages
> + * with the host.
> + * If all the pages for the specified size cannot be reserved,
> + * the function rewinds the partial allocation and a NULL pointer
> + * is returned.
> + *
> + * @size: the minimal size allocated in byte.
> + * @flags: the flags used for the underlying page allocator.
> + *
> + * Errors:
> + *   The allocation will assert the size parameter, will fail if the
> + *   underlying page allocator fail or in the case of protected
> + *   virtualisation if the sharing of the pages fails.

I think "virtualization" (with an z) is more common than "virtualisation".

> + *
> + * Returns a pointer to the first page in case of success, NULL otherwise.
> + */
> +void *alloc_io_pages(int size, int flags);
> +
> +/*
> + * Frees a previously memory space allocated by alloc_io_pages.
> + * If Protected Virtualization facility is present, unshares the pages
> + * with the host.
> + * The address must be aligned on a page boundary otherwise an assertion
> + * breaks the program.
> + */
> +void free_io_pages(void *p, int size);
> +
> +#endif /* _S390X_MALLOC_IO_H_ */
> diff --git a/s390x/Makefile b/s390x/Makefile
> index b079a26..4b6301c 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -63,6 +63,7 @@ cflatobjs += lib/s390x/smp.o
>   cflatobjs += lib/s390x/vm.o
>   cflatobjs += lib/s390x/css_dump.o
>   cflatobjs += lib/s390x/css_lib.o
> +cflatobjs += lib/s390x/malloc_io.o
>   
>   OBJDIRS += lib/s390x

  Thomas

Janosch Frank Jan. 21, 2021, 9:46 a.m. UTC | #2

On 1/21/21 10:13 AM, Pierre Morel wrote:
> To centralize the memory allocation for I/O we define
> the alloc_io_page/free_io_page functions which share the I/O
> memory with the host in case the guest runs with
> protected virtualization.
> 
> Signed-off-by: Pierre Morel <[e-mail]>
> ---
>  MAINTAINERS           |  1 +
>  lib/s390x/malloc_io.c | 70 +++++++++++++++++++++++++++++++++++++++++++
>  lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
>  s390x/Makefile        |  1 +
>  4 files changed, 117 insertions(+)
>  create mode 100644 lib/s390x/malloc_io.c
>  create mode 100644 lib/s390x/malloc_io.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 54124f6..89cb01e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -82,6 +82,7 @@ M: Thomas Huth <[e-mail]>
>  M: David Hildenbrand <[e-mail]>
>  M: Janosch Frank <[e-mail]>
>  R: Cornelia Huck <[e-mail]>
> +R: Pierre Morel <[e-mail]>

If you're ok with the amount of mails you'll get then go ahead.
But I think maintainer file changes should always be in a separate patch.

>  L: [e-mail]
>  L: [e-mail]
>  F: s390x/*
> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
> new file mode 100644
> index 0000000..bfe8c6a
> --- /dev/null
> +++ b/lib/s390x/malloc_io.c
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0

I think we wanted to use:
/* SPDX-License-Identifier: GPL-2.0-or-later */

> +/*
> + * I/O page allocation
> + *
> + * Copyright (c) 2021 IBM Corp
> + *
> + * Authors:
> + *  Pierre Morel <[e-mail]>
> + *
> + * Using this interface provide host access to the allocated pages in
> + * case the guest is a secure guest.
> + * This is needed for I/O buffers.
> + *
> + */
> +#include <libcflat.h>
> +#include <asm/page.h>
> +#include <asm/uv.h>
> +#include <malloc_io.h>
> +#include <alloc_page.h>
> +#include <asm/facility.h>
> +
> +static int share_pages(void *p, int count)
> +{
> +	int i = 0;
> +
> +	for (i = 0; i < count; i++, p += PAGE_SIZE)
> +		if (uv_set_shared((unsigned long)p))
> +			return i;
> +	return i;
> +}
> +
> +static void unshare_pages(void *p, int count)
> +{
> +	int i;
> +
> +	for (i = count; i > 0; i--, p += PAGE_SIZE)
> +		uv_remove_shared((unsigned long)p);
> +}
> +
> +void *alloc_io_pages(int size, int flags)
> +{
> +	int order = (size >> PAGE_SHIFT);
> +	void *p;
> +	int n;
> +
> +	assert(size);
> +
> +	p = alloc_pages_flags(order, AREA_DMA31 | flags);
> +	if (!p || !test_facility(158))
> +		return p;
> +
> +	n = share_pages(p, 1 << order);
> +	if (n == 1 << order)
> +		return p;
> +
> +	unshare_pages(p, n);
> +	free_pages(p);
> +	return NULL;
> +}
> +
> +void free_io_pages(void *p, int size)
> +{
> +	int order = size >> PAGE_SHIFT;
> +
> +	assert(IS_ALIGNED((uintptr_t)p, PAGE_SIZE));
> +
> +	if (test_facility(158))
> +		unshare_pages(p, 1 << order);

I have a lib file in the making that will let you check UV features like
test_facility(). When that's ready I'm gonna check for unshare here.

> +	free_pages(p);
> +}
> diff --git a/lib/s390x/malloc_io.h b/lib/s390x/malloc_io.h
> new file mode 100644
> index 0000000..494dfe9
> --- /dev/null
> +++ b/lib/s390x/malloc_io.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * I/O allocations
> + *
> + * Copyright (c) 2021 IBM Corp
> + *
> + * Authors:
> + *  Pierre Morel <[e-mail]>
> + *
> + */
> +#ifndef _S390X_MALLOC_IO_H_
> +#define _S390X_MALLOC_IO_H_
> +
> +/*
> + * Allocates a page aligned page bound range of contiguous real or
> + * absolute memory in the DMA31 region large enough to contain size
> + * bytes.
> + * If Protected Virtualization facility is present, shares the pages
> + * with the host.
> + * If all the pages for the specified size cannot be reserved,
> + * the function rewinds the partial allocation and a NULL pointer
> + * is returned.
> + *
> + * @size: the minimal size allocated in byte.
> + * @flags: the flags used for the underlying page allocator.
> + *
> + * Errors:
> + *   The allocation will assert the size parameter, will fail if the
> + *   underlying page allocator fail or in the case of protected
> + *   virtualisation if the sharing of the pages fails.
> + *
> + * Returns a pointer to the first page in case of success, NULL otherwise.
> + */
> +void *alloc_io_pages(int size, int flags);
> +
> +/*
> + * Frees a previously memory space allocated by alloc_io_pages.
> + * If Protected Virtualization facility is present, unshares the pages
> + * with the host.
> + * The address must be aligned on a page boundary otherwise an assertion
> + * breaks the program.
> + */
> +void free_io_pages(void *p, int size);
> +
> +#endif /* _S390X_MALLOC_IO_H_ */
> diff --git a/s390x/Makefile b/s390x/Makefile
> index b079a26..4b6301c 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -63,6 +63,7 @@ cflatobjs += lib/s390x/smp.o
>  cflatobjs += lib/s390x/vm.o
>  cflatobjs += lib/s390x/css_dump.o
>  cflatobjs += lib/s390x/css_lib.o
> +cflatobjs += lib/s390x/malloc_io.o
>  
>  OBJDIRS += lib/s390x
>  
>

Pierre Morel Jan. 21, 2021, 9:57 a.m. UTC | #3

On 1/21/21 10:46 AM, Janosch Frank wrote:
> On 1/21/21 10:13 AM, Pierre Morel wrote:
>> To centralize the memory allocation for I/O we define
>> the alloc_io_page/free_io_page functions which share the I/O
>> memory with the host in case the guest runs with
>> protected virtualization.
>>
>> Signed-off-by: Pierre Morel <[e-mail]>
>> ---
>>   MAINTAINERS           |  1 +
>>   lib/s390x/malloc_io.c | 70 +++++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
>>   s390x/Makefile        |  1 +
>>   4 files changed, 117 insertions(+)
>>   create mode 100644 lib/s390x/malloc_io.c
>>   create mode 100644 lib/s390x/malloc_io.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 54124f6..89cb01e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -82,6 +82,7 @@ M: Thomas Huth <[e-mail]>
>>   M: David Hildenbrand <[e-mail]>
>>   M: Janosch Frank <[e-mail]>
>>   R: Cornelia Huck <[e-mail]>
>> +R: Pierre Morel <[e-mail]>
> 
> If you're ok with the amount of mails you'll get then go ahead.
> But I think maintainer file changes should always be in a separate patch.

You are right it was more an attempts to bring the Linux checkpatch to 
be quiet but this would need more changes so.. we can discuss this in a 
separate patch.

> 
>>   L: [e-mail]
>>   L: [e-mail]
>>   F: s390x/*
>> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
>> new file mode 100644
>> index 0000000..bfe8c6a
>> --- /dev/null
>> +++ b/lib/s390x/malloc_io.c
>> @@ -0,0 +1,70 @@
>> +// SPDX-License-Identifier: GPL-2.0
> 
> I think we wanted to use:
> /* SPDX-License-Identifier: GPL-2.0-or-later */

yes


...snip...
>> +
>> +void free_io_pages(void *p, int size)
>> +{
>> +	int order = size >> PAGE_SHIFT;
>> +
>> +	assert(IS_ALIGNED((uintptr_t)p, PAGE_SIZE));
>> +
>> +	if (test_facility(158))
>> +		unshare_pages(p, 1 << order);
> 
> I have a lib file in the making that will let you check UV features like
> test_facility(). When that's ready I'm gonna check for unshare here.

OK

Thanks,
Pierre

Pierre Morel Jan. 21, 2021, 1:02 p.m. UTC | #4

On 1/21/21 10:46 AM, Janosch Frank wrote:
> On 1/21/21 10:13 AM, Pierre Morel wrote:
>> To centralize the memory allocation for I/O we define
>> the alloc_io_page/free_io_page functions which share the I/O
>> memory with the host in case the guest runs with
>> protected virtualization.
>>
>> Signed-off-by: Pierre Morel <[e-mail]>
>> ---
>>   MAINTAINERS           |  1 +
>>   lib/s390x/malloc_io.c | 70 +++++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
>>   s390x/Makefile        |  1 +
>>   4 files changed, 117 insertions(+)
>>   create mode 100644 lib/s390x/malloc_io.c
>>   create mode 100644 lib/s390x/malloc_io.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 54124f6..89cb01e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -82,6 +82,7 @@ M: Thomas Huth <[e-mail]>
>>   M: David Hildenbrand <[e-mail]>
>>   M: Janosch Frank <[e-mail]>
>>   R: Cornelia Huck <[e-mail]>
>> +R: Pierre Morel <[e-mail]>
> 
> If you're ok with the amount of mails you'll get then go ahead.
> But I think maintainer file changes should always be in a separate patch.
> 
>>   L: [e-mail]
>>   L: [e-mail]
>>   F: s390x/*
>> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
>> new file mode 100644
>> index 0000000..bfe8c6a
>> --- /dev/null
>> +++ b/lib/s390x/malloc_io.c
>> @@ -0,0 +1,70 @@
>> +// SPDX-License-Identifier: GPL-2.0
> 
> I think we wanted to use:

@Janosch , @Thomas

> /* SPDX-License-Identifier: GPL-2.0-or-later */

or

// SPDX-License-Identifier: GPL-2.0-only

later or only ?

/* or // ?


If both are OK, I will take the Janosch proposition which is in use in 
vm.[ch] and ignore the Linux checkpatch warning.

Just to : Why are you people not using the Linux style code completely 
instead of making new exceptions.

i.e. SPDX license and MAINTAINERS

Pierre Morel Jan. 21, 2021, 1:33 p.m. UTC | #5

On 1/21/21 10:46 AM, Janosch Frank wrote:
> On 1/21/21 10:13 AM, Pierre Morel wrote:
>> To centralize the memory allocation for I/O we define
>> the alloc_io_page/free_io_page functions which share the I/O
>> memory with the host in case the guest runs with
>> protected virtualization.
>>
>> Signed-off-by: Pierre Morel <[e-mail]>
>> ---
>>   MAINTAINERS           |  1 +
>>   lib/s390x/malloc_io.c | 70 +++++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
>>   s390x/Makefile        |  1 +
>>   4 files changed, 117 insertions(+)
>>   create mode 100644 lib/s390x/malloc_io.c
>>   create mode 100644 lib/s390x/malloc_io.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 54124f6..89cb01e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -82,6 +82,7 @@ M: Thomas Huth <[e-mail]>
>>   M: David Hildenbrand <[e-mail]>
>>   M: Janosch Frank <[e-mail]>
>>   R: Cornelia Huck <[e-mail]>
>> +R: Pierre Morel <[e-mail]>
> 
> If you're ok with the amount of mails you'll get then go ahead.
> But I think maintainer file changes should always be in a separate patch.
> 
>>   L: [e-mail]
>>   L: [e-mail]
>>   F: s390x/*
>> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
>> new file mode 100644
>> index 0000000..bfe8c6a
>> --- /dev/null
>> +++ b/lib/s390x/malloc_io.c
>> @@ -0,0 +1,70 @@
>> +// SPDX-License-Identifier: GPL-2.0
> 
> I think we wanted to use:
> /* SPDX-License-Identifier: GPL-2.0-or-later */
> 
>> +/*
>> + * I/O page allocation
>> + *
>> + * Copyright (c) 2021 IBM Corp
>> + *
>> + * Authors:
>> + *  Pierre Morel <[e-mail]>
>> + *
>> + * Using this interface provide host access to the allocated pages in
>> + * case the guest is a secure guest.
>> + * This is needed for I/O buffers.
>> + *
>> + */
>> +#include <libcflat.h>
>> +#include <asm/page.h>
>> +#include <asm/uv.h>
>> +#include <malloc_io.h>
>> +#include <alloc_page.h>
>> +#include <asm/facility.h>
>> +
>> +static int share_pages(void *p, int count)
>> +{
>> +	int i = 0;
>> +
>> +	for (i = 0; i < count; i++, p += PAGE_SIZE)
>> +		if (uv_set_shared((unsigned long)p))
>> +			return i;
>> +	return i;
>> +}
>> +
>> +static void unshare_pages(void *p, int count)
>> +{
>> +	int i;
>> +
>> +	for (i = count; i > 0; i--, p += PAGE_SIZE)
>> +		uv_remove_shared((unsigned long)p);
>> +}
>> +
>> +void *alloc_io_pages(int size, int flags)
>> +{
>> +	int order = (size >> PAGE_SHIFT);
>> +	void *p;
>> +	int n;
>> +
>> +	assert(size);
>> +
>> +	p = alloc_pages_flags(order, AREA_DMA31 | flags);
>> +	if (!p || !test_facility(158))
>> +		return p;
>> +
>> +	n = share_pages(p, 1 << order);
>> +	if (n == 1 << order)
>> +		return p;
>> +
>> +	unshare_pages(p, n);
>> +	free_pages(p);
>> +	return NULL;
>> +}
>> +
>> +void free_io_pages(void *p, int size)
>> +{
>> +	int order = size >> PAGE_SHIFT;
>> +
>> +	assert(IS_ALIGNED((uintptr_t)p, PAGE_SIZE));
>> +
>> +	if (test_facility(158))
>> +		unshare_pages(p, 1 << order);
> 
> I have a lib file in the making that will let you check UV features like
> test_facility(). When that's ready I'm gonna check for unshare here.

OK.
On share_pages the rc is enough I suppose.

Thomas Huth Jan. 21, 2021, 1:43 p.m. UTC | #6

On 21/01/2021 14.02, Pierre Morel wrote:
> 
> 
> On 1/21/21 10:46 AM, Janosch Frank wrote:
>> On 1/21/21 10:13 AM, Pierre Morel wrote:
>>> To centralize the memory allocation for I/O we define
>>> the alloc_io_page/free_io_page functions which share the I/O
>>> memory with the host in case the guest runs with
>>> protected virtualization.
>>>
>>> Signed-off-by: Pierre Morel <[e-mail]>
>>> ---
>>>   MAINTAINERS           |  1 +
>>>   lib/s390x/malloc_io.c | 70 +++++++++++++++++++++++++++++++++++++++++++
>>>   lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
>>>   s390x/Makefile        |  1 +
>>>   4 files changed, 117 insertions(+)
>>>   create mode 100644 lib/s390x/malloc_io.c
>>>   create mode 100644 lib/s390x/malloc_io.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 54124f6..89cb01e 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -82,6 +82,7 @@ M: Thomas Huth <[e-mail]>
>>>   M: David Hildenbrand <[e-mail]>
>>>   M: Janosch Frank <[e-mail]>
>>>   R: Cornelia Huck <[e-mail]>
>>> +R: Pierre Morel <[e-mail]>
>>
>> If you're ok with the amount of mails you'll get then go ahead.
>> But I think maintainer file changes should always be in a separate patch.
>>
>>>   L: [e-mail]
>>>   L: [e-mail]
>>>   F: s390x/*
>>> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
>>> new file mode 100644
>>> index 0000000..bfe8c6a
>>> --- /dev/null
>>> +++ b/lib/s390x/malloc_io.c
>>> @@ -0,0 +1,70 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>
>> I think we wanted to use:
> 
> @Janosch , @Thomas
> 
>> /* SPDX-License-Identifier: GPL-2.0-or-later */
> 
> or
> 
> // SPDX-License-Identifier: GPL-2.0-only
> 
> later or only ?

If it's a new file, it's up to the author. I personally prefer -later, but I 
think IBM's preference is normally -only instead. Please check with your 
colleagues.
Most s390x-related files in the kvm-unit-tests currently use "GPL-2.0-only", 
so that should be ok anyway.

> /* or // ?

I don't mind. // seems to be kernel style for .c files, but so far we've 
only used /* with SPDX in the kvm-unit-tests, so both should be fine, I think.

> Just to : Why are you people not using the Linux style code completely 
> instead of making new exceptions.
> 
> i.e. SPDX license and MAINTAINERS

Actually, I wonder why the Linux documentation still recommends the 
identifiers that are marked as deprecated on the SPDX website. The 
deprecated "GPL-2.0" can be rather confusing, since it IMHO can easily be 
mistaken as "GPL-2.0+", so the newer identifiers are better, indeed.

Not sure what you mean with MAINTAINERS, though.

  Thomas

Pierre Morel Jan. 21, 2021, 1:47 p.m. UTC | #7

On 1/21/21 2:43 PM, Thomas Huth wrote:
> On 21/01/2021 14.02, Pierre Morel wrote:
>>
>>
>> On 1/21/21 10:46 AM, Janosch Frank wrote:
>>> On 1/21/21 10:13 AM, Pierre Morel wrote:
>>>> To centralize the memory allocation for I/O we define
>>>> the alloc_io_page/free_io_page functions which share the I/O
>>>> memory with the host in case the guest runs with
>>>> protected virtualization.
>>>>
>>>> Signed-off-by: Pierre Morel <[e-mail]>
>>>> ---
>>>>   MAINTAINERS           |  1 +
>>>>   lib/s390x/malloc_io.c | 70 
>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>   lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
>>>>   s390x/Makefile        |  1 +
>>>>   4 files changed, 117 insertions(+)
>>>>   create mode 100644 lib/s390x/malloc_io.c
>>>>   create mode 100644 lib/s390x/malloc_io.h
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 54124f6..89cb01e 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -82,6 +82,7 @@ M: Thomas Huth <[e-mail]>
>>>>   M: David Hildenbrand <[e-mail]>
>>>>   M: Janosch Frank <[e-mail]>
>>>>   R: Cornelia Huck <[e-mail]>
>>>> +R: Pierre Morel <[e-mail]>
>>>
>>> If you're ok with the amount of mails you'll get then go ahead.
>>> But I think maintainer file changes should always be in a separate 
>>> patch.
>>>
>>>>   L: [e-mail]
>>>>   L: [e-mail]
>>>>   F: s390x/*
>>>> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
>>>> new file mode 100644
>>>> index 0000000..bfe8c6a
>>>> --- /dev/null
>>>> +++ b/lib/s390x/malloc_io.c
>>>> @@ -0,0 +1,70 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>
>>> I think we wanted to use:
>>
>> @Janosch , @Thomas
>>
>>> /* SPDX-License-Identifier: GPL-2.0-or-later */
>>
>> or
>>
>> // SPDX-License-Identifier: GPL-2.0-only
>>
>> later or only ?
> 
> If it's a new file, it's up to the author. I personally prefer -later, 
> but I think IBM's preference is normally -only instead. Please check 
> with your colleagues.
> Most s390x-related files in the kvm-unit-tests currently use 
> "GPL-2.0-only", so that should be ok anyway.
> 
>> /* or // ?
> 
> I don't mind. // seems to be kernel style for .c files, but so far we've 
> only used /* with SPDX in the kvm-unit-tests, so both should be fine, I 
> think.
> 
>> Just to : Why are you people not using the Linux style code completely 
>> instead of making new exceptions.
>>
>> i.e. SPDX license and MAINTAINERS
> 
> Actually, I wonder why the Linux documentation still recommends the 
> identifiers that are marked as deprecated on the SPDX website. The 
> deprecated "GPL-2.0" can be rather confusing, since it IMHO can easily 
> be mistaken as "GPL-2.0+", so the newer identifiers are better, indeed.
> 
> Not sure what you mean with MAINTAINERS, though.

Thanks for the explanations :)

For MAINTAINERS, the Linux kernel checkpatch warns that we should use
TABS instead of SPACES between item and names.

Pierre

Janosch Frank Jan. 21, 2021, 1:48 p.m. UTC | #8

On 1/21/21 2:02 PM, Pierre Morel wrote:
> 
> 
> On 1/21/21 10:46 AM, Janosch Frank wrote:
>> On 1/21/21 10:13 AM, Pierre Morel wrote:
>>> To centralize the memory allocation for I/O we define
>>> the alloc_io_page/free_io_page functions which share the I/O
>>> memory with the host in case the guest runs with
>>> protected virtualization.
>>>
>>> Signed-off-by: Pierre Morel <[e-mail]>
>>> ---
>>>   MAINTAINERS           |  1 +
>>>   lib/s390x/malloc_io.c | 70 +++++++++++++++++++++++++++++++++++++++++++
>>>   lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
>>>   s390x/Makefile        |  1 +
>>>   4 files changed, 117 insertions(+)
>>>   create mode 100644 lib/s390x/malloc_io.c
>>>   create mode 100644 lib/s390x/malloc_io.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 54124f6..89cb01e 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -82,6 +82,7 @@ M: Thomas Huth <[e-mail]>
>>>   M: David Hildenbrand <[e-mail]>
>>>   M: Janosch Frank <[e-mail]>
>>>   R: Cornelia Huck <[e-mail]>
>>> +R: Pierre Morel <[e-mail]>
>>
>> If you're ok with the amount of mails you'll get then go ahead.
>> But I think maintainer file changes should always be in a separate patch.
>>
>>>   L: [e-mail]
>>>   L: [e-mail]
>>>   F: s390x/*
>>> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
>>> new file mode 100644
>>> index 0000000..bfe8c6a
>>> --- /dev/null
>>> +++ b/lib/s390x/malloc_io.c
>>> @@ -0,0 +1,70 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>
>> I think we wanted to use:
> 
> @Janosch , @Thomas
> 
>> /* SPDX-License-Identifier: GPL-2.0-or-later */
> 
> or
> 
> // SPDX-License-Identifier: GPL-2.0-only
> 
> later or only ?
> 
> /* or // ?
> 
> 
> If both are OK, I will take the Janosch proposition which is in use in 
> vm.[ch] and ignore the Linux checkpatch warning.
> 
> Just to : Why are you people not using the Linux style code completely 
> instead of making new exceptions.
> 
> i.e. SPDX license and MAINTAINERS
> 

s390 also has /* */ style SPDX and GPL2.0+ statements in the kernel...

Since KUT has way less developers the style rules aren't as strict and
currently I see that as an advantage. Following checkpatch down the
cliff is a bad idea in the kernel and for unit tests. It's most often
correct, but not always.

Thomas Huth Jan. 21, 2021, 1:56 p.m. UTC | #9

On 21/01/2021 14.47, Pierre Morel wrote:
[...]
> For MAINTAINERS, the Linux kernel checkpatch warns that we should use
> TABS instead of SPACES between item and names.

Interesting, I wasn't aware of that. I guess it's simply because the 
MAINTAINERS file in kvm-unit-tests is older than the patch that changed the 
checkpatch script in the kernel, and updates to the MAINTAINRS file in k-u-t 
are so seldom that nobody really noticed afterwards.

If it bothers you, feel free to send a patch to fix k-u-t's MAINTAINERS 
file, it might be nice indeed to be aligned with the kernel here again.

  Thomas

Pierre Morel Jan. 21, 2021, 3:47 p.m. UTC | #10

On 1/21/21 2:56 PM, Thomas Huth wrote:
> On 21/01/2021 14.47, Pierre Morel wrote:
> [...]
>> For MAINTAINERS, the Linux kernel checkpatch warns that we should use
>> TABS instead of SPACES between item and names.
> 
> Interesting, I wasn't aware of that. I guess it's simply because the 
> MAINTAINERS file in kvm-unit-tests is older than the patch that changed 
> the checkpatch script in the kernel, and updates to the MAINTAINRS file 
> in k-u-t are so seldom that nobody really noticed afterwards.
> 
> If it bothers you, feel free to send a patch to fix k-u-t's MAINTAINERS 
> file, it might be nice indeed to be aligned with the kernel here again.
> 
>   Thomas
> 
> 

OK, I will do,
Thanks,
Pierre

Pierre Morel Jan. 21, 2021, 3:48 p.m. UTC | #11

On 1/21/21 2:48 PM, Janosch Frank wrote:
> On 1/21/21 2:02 PM, Pierre Morel wrote:
>>
>>
>> On 1/21/21 10:46 AM, Janosch Frank wrote:
>>> On 1/21/21 10:13 AM, Pierre Morel wrote:
>>>> To centralize the memory allocation for I/O we define
>>>> the alloc_io_page/free_io_page functions which share the I/O
>>>> memory with the host in case the guest runs with
>>>> protected virtualization.
>>>>
>>>> Signed-off-by: Pierre Morel <[e-mail]>
>>>> ---
>>>>    MAINTAINERS           |  1 +
>>>>    lib/s390x/malloc_io.c | 70 +++++++++++++++++++++++++++++++++++++++++++
>>>>    lib/s390x/malloc_io.h | 45 ++++++++++++++++++++++++++++
>>>>    s390x/Makefile        |  1 +
>>>>    4 files changed, 117 insertions(+)
>>>>    create mode 100644 lib/s390x/malloc_io.c
>>>>    create mode 100644 lib/s390x/malloc_io.h
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 54124f6..89cb01e 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -82,6 +82,7 @@ M: Thomas Huth <[e-mail]>
>>>>    M: David Hildenbrand <[e-mail]>
>>>>    M: Janosch Frank <[e-mail]>
>>>>    R: Cornelia Huck <[e-mail]>
>>>> +R: Pierre Morel <[e-mail]>
>>>
>>> If you're ok with the amount of mails you'll get then go ahead.
>>> But I think maintainer file changes should always be in a separate patch.
>>>
>>>>    L: [e-mail]
>>>>    L: [e-mail]
>>>>    F: s390x/*
>>>> diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
>>>> new file mode 100644
>>>> index 0000000..bfe8c6a
>>>> --- /dev/null
>>>> +++ b/lib/s390x/malloc_io.c
>>>> @@ -0,0 +1,70 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>
>>> I think we wanted to use:
>>
>> @Janosch , @Thomas
>>
>>> /* SPDX-License-Identifier: GPL-2.0-or-later */
>>
>> or
>>
>> // SPDX-License-Identifier: GPL-2.0-only
>>
>> later or only ?
>>
>> /* or // ?
>>
>>
>> If both are OK, I will take the Janosch proposition which is in use in
>> vm.[ch] and ignore the Linux checkpatch warning.
>>
>> Just to : Why are you people not using the Linux style code completely
>> instead of making new exceptions.
>>
>> i.e. SPDX license and MAINTAINERS
>>
> 
> s390 also has /* */ style SPDX and GPL2.0+ statements in the kernel...
> 
> Since KUT has way less developers the style rules aren't as strict and
> currently I see that as an advantage. Following checkpatch down the
> cliff is a bad idea in the kernel and for unit tests. It's most often
> correct, but not always.
> 

Oh OK,
thanks for the explanation,

Pierre

Pierre Morel Jan. 22, 2021, 7:55 a.m. UTC | #12

On 1/21/21 10:32 AM, Thomas Huth wrote:

...snip...

>> +#include <asm/facility.h>
>> +
>> +static int share_pages(void *p, int count)
>> +{
>> +    int i = 0;
>> +
>> +    for (i = 0; i < count; i++, p += PAGE_SIZE)
>> +        if (uv_set_shared((unsigned long)p))
>> +            return i;
> 
> Just a matter of taste, but you could replace the "return i" here also 
> with a "break" since you're returning i below anyway.

right a single out point is always better.

> 
>> +    return i;
>> +}
>> +
>> +static void unshare_pages(void *p, int count)
>> +{
>> +    int i;
>> +
>> +    for (i = count; i > 0; i--, p += PAGE_SIZE)
>> +        uv_remove_shared((unsigned long)p);
>> +}
>> +
>> +void *alloc_io_pages(int size, int flags)
> 
> I still think the naming or size parameter is confusing here. If I read 
> something like alloc_io_pages(), I'd expect a "num_pages" parameter. So 
> if you want to keep the "size" in bytes, I'd suggest to rename the 
> function to "alloc_io_mem" instead.

OK, I rename the function, allowing the user to keep a simple interface
without having to calculate the page order.

> 
>> +{
>> +    int order = (size >> PAGE_SHIFT);
> 
> I think this is wrong. According to the description of alloc_pages_flag, 
> it allocates "1ull << order" pages.
> So you likely want to do this instead here:
> 
>          int order = get_order(size >> PAGE_SHIFT);

you are absolutely right.

> 
>> +    void *p;
>> +    int n;
>> +
>> +    assert(size);
>> +
>> +    p = alloc_pages_flags(order, AREA_DMA31 | flags);
>> +    if (!p || !test_facility(158))
>> +        return p;
>> +
>> +    n = share_pages(p, 1 << order);
>> +    if (n == 1 << order)
>> +        return p;
>> +
>> +    unshare_pages(p, n);
>> +    free_pages(p);
>> +    return NULL;
>> +}
>> +
>> +void free_io_pages(void *p, int size)
>> +{
>> +    int order = size >> PAGE_SHIFT;
> 
> dito?

yes :(

> 
>> +    assert(IS_ALIGNED((uintptr_t)p, PAGE_SIZE));
>> +
>> +    if (test_facility(158))
>> +        unshare_pages(p, 1 << order);
>> +    free_pages(p);
>> +}
>> diff --git a/lib/s390x/malloc_io.h b/lib/s390x/malloc_io.h
>> new file mode 100644
>> index 0000000..494dfe9
>> --- /dev/null
>> +++ b/lib/s390x/malloc_io.h
>> @@ -0,0 +1,45 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
> 
> GPL-2.0-only please.

almmost... I use:
/* SPDX-License-Identifier: GPL-2.0-or-later */

as in other files updated by janosch if this is not a problem.

> 
>> +/*
>> + * I/O allocations
>> + *
>> + * Copyright (c) 2021 IBM Corp
>> + *
>> + * Authors:
>> + *  Pierre Morel <[e-mail]>
>> + *
>> + */
>> +#ifndef _S390X_MALLOC_IO_H_
>> +#define _S390X_MALLOC_IO_H_
>> +
>> +/*
>> + * Allocates a page aligned page bound range of contiguous real or
>> + * absolute memory in the DMA31 region large enough to contain size
>> + * bytes.
>> + * If Protected Virtualization facility is present, shares the pages
>> + * with the host.
>> + * If all the pages for the specified size cannot be reserved,
>> + * the function rewinds the partial allocation and a NULL pointer
>> + * is returned.
>> + *
>> + * @size: the minimal size allocated in byte.
>> + * @flags: the flags used for the underlying page allocator.
>> + *
>> + * Errors:
>> + *   The allocation will assert the size parameter, will fail if the
>> + *   underlying page allocator fail or in the case of protected
>> + *   virtualisation if the sharing of the pages fails.
> 
> I think "virtualization" (with an z) is more common than "virtualisation".

OK


Thanks,
Pierre

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 54124f6..89cb01e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -82,6 +82,7 @@  M: Thomas Huth <[e-mail]>
 M: David Hildenbrand <[e-mail]>
 M: Janosch Frank <[e-mail]>
 R: Cornelia Huck <[e-mail]>
+R: Pierre Morel <[e-mail]>
 L: [e-mail]
 L: [e-mail]
 F: s390x/*
diff --git a/lib/s390x/malloc_io.c b/lib/s390x/malloc_io.c
new file mode 100644
index 0000000..bfe8c6a
--- /dev/null
+++ b/lib/s390x/malloc_io.c
@@ -0,0 +1,70 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * I/O page allocation
+ *
+ * Copyright (c) 2021 IBM Corp
+ *
+ * Authors:
+ *  Pierre Morel <[e-mail]>
+ *
+ * Using this interface provide host access to the allocated pages in
+ * case the guest is a secure guest.
+ * This is needed for I/O buffers.
+ *
+ */
+#include <libcflat.h>
+#include <asm/page.h>
+#include <asm/uv.h>
+#include <malloc_io.h>
+#include <alloc_page.h>
+#include <asm/facility.h>
+
+static int share_pages(void *p, int count)
+{
+	int i = 0;
+
+	for (i = 0; i < count; i++, p += PAGE_SIZE)
+		if (uv_set_shared((unsigned long)p))
+			return i;
+	return i;
+}
+
+static void unshare_pages(void *p, int count)
+{
+	int i;
+
+	for (i = count; i > 0; i--, p += PAGE_SIZE)
+		uv_remove_shared((unsigned long)p);
+}
+
+void *alloc_io_pages(int size, int flags)
+{
+	int order = (size >> PAGE_SHIFT);
+	void *p;
+	int n;
+
+	assert(size);
+
+	p = alloc_pages_flags(order, AREA_DMA31 | flags);
+	if (!p || !test_facility(158))
+		return p;
+
+	n = share_pages(p, 1 << order);
+	if (n == 1 << order)
+		return p;
+
+	unshare_pages(p, n);
+	free_pages(p);
+	return NULL;
+}
+
+void free_io_pages(void *p, int size)
+{
+	int order = size >> PAGE_SHIFT;
+
+	assert(IS_ALIGNED((uintptr_t)p, PAGE_SIZE));
+
+	if (test_facility(158))
+		unshare_pages(p, 1 << order);
+	free_pages(p);
+}
diff --git a/lib/s390x/malloc_io.h b/lib/s390x/malloc_io.h
new file mode 100644
index 0000000..494dfe9
--- /dev/null
+++ b/lib/s390x/malloc_io.h
@@ -0,0 +1,45 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * I/O allocations
+ *
+ * Copyright (c) 2021 IBM Corp
+ *
+ * Authors:
+ *  Pierre Morel <[e-mail]>
+ *
+ */
+#ifndef _S390X_MALLOC_IO_H_
+#define _S390X_MALLOC_IO_H_
+
+/*
+ * Allocates a page aligned page bound range of contiguous real or
+ * absolute memory in the DMA31 region large enough to contain size
+ * bytes.
+ * If Protected Virtualization facility is present, shares the pages
+ * with the host.
+ * If all the pages for the specified size cannot be reserved,
+ * the function rewinds the partial allocation and a NULL pointer
+ * is returned.
+ *
+ * @size: the minimal size allocated in byte.
+ * @flags: the flags used for the underlying page allocator.
+ *
+ * Errors:
+ *   The allocation will assert the size parameter, will fail if the
+ *   underlying page allocator fail or in the case of protected
+ *   virtualisation if the sharing of the pages fails.
+ *
+ * Returns a pointer to the first page in case of success, NULL otherwise.
+ */
+void *alloc_io_pages(int size, int flags);
+
+/*
+ * Frees a previously memory space allocated by alloc_io_pages.
+ * If Protected Virtualization facility is present, unshares the pages
+ * with the host.
+ * The address must be aligned on a page boundary otherwise an assertion
+ * breaks the program.
+ */
+void free_io_pages(void *p, int size);
+
+#endif /* _S390X_MALLOC_IO_H_ */
diff --git a/s390x/Makefile b/s390x/Makefile
index b079a26..4b6301c 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -63,6 +63,7 @@  cflatobjs += lib/s390x/smp.o
 cflatobjs += lib/s390x/vm.o
 cflatobjs += lib/s390x/css_dump.o
 cflatobjs += lib/s390x/css_lib.o
+cflatobjs += lib/s390x/malloc_io.o
 
 OBJDIRS += lib/s390x