[PATCH kvm-unit-tests 0/9] s390x: vmalloc support

[PATCH kvm-unit-tests 0/9] s390x: vmalloc support

[David Hildenbrand]

This series implements
- detection of installed physical memory
- setup of the physical allocator
- setup of the MMU / page tables / DAT
- setup of the virtual allocator

The CPU now runs with DAT enabled. I added a small test to make sure
malloc() indeed works and uses virtual adresses.

While at it, fix the TEST BLOCK test on newer compilers.

Tested with upsteam QEMU TCG and KVM.

David Hildenbrand (9):
  s390x: fix TEST BLOCK tests
  s390x: use highest addresses for PGM_ADDRESSING errors
  s390x: increase the stack size
  s390x: add missing sclp definitions from QEMU
  s390x: rename sclp_setup() to sclp_ascii_setup()
  s390x: detect installed memory
  s390x: initialize the physical allocator
  s390x: add vmalloc support
  s390x: add test for (v)malloc

 lib/s390x/asm/arch_def.h |  57 ++++++++++++
 lib/s390x/asm/page.h     |  24 +++++
 lib/s390x/asm/pgtable.h  | 222 +++++++++++++++++++++++++++++++++++++++++++++++
 lib/s390x/io.c           |   3 +-
 lib/s390x/mmu.c          |  90 +++++++++++++++++++
 lib/s390x/sclp-ascii.c   |   4 +-
 lib/s390x/sclp.c         |  66 ++++++++++++++
 lib/s390x/sclp.h         | 111 +++++++++++++++++++++++-
 s390x/Makefile           |   6 ++
 s390x/cstart64.S         |   3 +-
 s390x/flat.lds           |   2 +-
 s390x/intercept.c        |  14 +--
 s390x/selftest.c         |  19 +++-
 13 files changed, 606 insertions(+), 15 deletions(-)
 create mode 100644 lib/s390x/asm/pgtable.h
 create mode 100644 lib/s390x/mmu.c
 create mode 100644 lib/s390x/sclp.c

-- 
2.14.3

[PATCH kvm-unit-tests 1/9] s390x: fix TEST BLOCK tests

[David Hildenbrand]

While new compilers like
	s390x-linux-gnu-gcc (GCC) 7.2.1 20170915 (Red Hat Cross 7.2.1-1)
Complain, that R1 is missing, old compilers like
	gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-16)
will complain that R1 is not valid.

According to the architecture, R1 is valid but ignored. No idea why this
was changed (in a way that programs no longer compile).

Anyhow, let's just specify the instruction explicitly.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 s390x/intercept.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/s390x/intercept.c b/s390x/intercept.c
index 59e5fca..99dde0d 100644
--- a/s390x/intercept.c
+++ b/s390x/intercept.c
@@ -139,7 +139,7 @@ static void test_testblock(void)
 
 	asm volatile (
 		" lghi	%%r0,0\n"
-		" tb	%1\n"
+		" .insn	rre,0xb22c0000,0,%1\n"
 		" ipm	%0\n"
 		" srl	%0,28\n"
 		: "=d" (cc)
@@ -150,12 +150,12 @@ static void test_testblock(void)
 
 	expect_pgm_int();
 	low_prot_enable();
-	asm volatile (" tb %0 " : : "r"(4096));
+	asm volatile (" .insn	rre,0xb22c0000,0,%0\n" : : "r"(4096));
 	low_prot_disable();
 	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
 
 	expect_pgm_int();
-	asm volatile (" tb %0 " : : "r"(-4096));
+	asm volatile (" .insn	rre,0xb22c0000,0,%0\n" : : "r"(-4096));
 	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
 }
 
-- 
2.14.3

Re: [PATCH kvm-unit-tests 1/9] s390x: fix TEST BLOCK tests

[Thomas Huth]

On 10.01.2018 22:53, David Hildenbrand wrote:
> While new compilers like
> 	s390x-linux-gnu-gcc (GCC) 7.2.1 20170915 (Red Hat Cross 7.2.1-1)
> Complain, that R1 is missing, old compilers like
> 	gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-16)
> will complain that R1 is not valid.
> 
> According to the architecture, R1 is valid but ignored. No idea why this
> was changed (in a way that programs no longer compile).

What a pity :-(

> Anyhow, let's just specify the instruction explicitly.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  s390x/intercept.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/s390x/intercept.c b/s390x/intercept.c
> index 59e5fca..99dde0d 100644
> --- a/s390x/intercept.c
> +++ b/s390x/intercept.c
> @@ -139,7 +139,7 @@ static void test_testblock(void)
>  
>  	asm volatile (
>  		" lghi	%%r0,0\n"
> -		" tb	%1\n"
> +		" .insn	rre,0xb22c0000,0,%1\n"
>  		" ipm	%0\n"
>  		" srl	%0,28\n"
>  		: "=d" (cc)
> @@ -150,12 +150,12 @@ static void test_testblock(void)
>  
>  	expect_pgm_int();
>  	low_prot_enable();
> -	asm volatile (" tb %0 " : : "r"(4096));
> +	asm volatile (" .insn	rre,0xb22c0000,0,%0\n" : : "r"(4096));
>  	low_prot_disable();
>  	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
>  
>  	expect_pgm_int();
> -	asm volatile (" tb %0 " : : "r"(-4096));
> +	asm volatile (" .insn	rre,0xb22c0000,0,%0\n" : : "r"(-4096));
>  	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
>  }
>  
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH kvm-unit-tests 1/9] s390x: fix TEST BLOCK tests

[Christian Borntraeger]

On 01/11/2018 09:51 AM, Thomas Huth wrote:
> On 10.01.2018 22:53, David Hildenbrand wrote:
>> While new compilers like
>> 	s390x-linux-gnu-gcc (GCC) 7.2.1 20170915 (Red Hat Cross 7.2.1-1)
>> Complain, that R1 is missing, old compilers like
>> 	gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-16)
>> will complain that R1 is not valid.
>>
>> According to the architecture, R1 is valid but ignored. No idea why this
>> was changed (in a way that programs no longer compile).
> 
> What a pity :-(

IIR, this was a bugfix. gcc is also used for TPF and they actually use TB and
the old binutils support for TB was architecturally wrong. Since TB is really
of no use for Linux this should not matter that much apart from the testcases
to test the KVM implementation.

Re: [PATCH kvm-unit-tests 1/9] s390x: fix TEST BLOCK tests

[David Hildenbrand]

On 11.01.2018 16:56, Christian Borntraeger wrote:
> 
> 
> On 01/11/2018 09:51 AM, Thomas Huth wrote:
>> On 10.01.2018 22:53, David Hildenbrand wrote:
>>> While new compilers like
>>> 	s390x-linux-gnu-gcc (GCC) 7.2.1 20170915 (Red Hat Cross 7.2.1-1)
>>> Complain, that R1 is missing, old compilers like
>>> 	gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-16)
>>> will complain that R1 is not valid.
>>>
>>> According to the architecture, R1 is valid but ignored. No idea why this
>>> was changed (in a way that programs no longer compile).
>>
>> What a pity :-(
> 
> IIR, this was a bugfix. gcc is also used for TPF and they actually use TB and
> the old binutils support for TB was architecturally wrong. Since TB is really
> of no use for Linux this should not matter that much apart from the testcases
> to test the KVM implementation.
> 

Well they managed to break code, so this was indeed a bad decision.

-- 

Thanks,

David / dhildenb

Re: [PATCH kvm-unit-tests 1/9] s390x: fix TEST BLOCK tests

[Andreas Krebbel]

On 01/11/2018 05:00 PM, David Hildenbrand wrote:
> On 11.01.2018 16:56, Christian Borntraeger wrote:
>>
>>
>> On 01/11/2018 09:51 AM, Thomas Huth wrote:
>>> On 10.01.2018 22:53, David Hildenbrand wrote:
>>>> While new compilers like
>>>> 	s390x-linux-gnu-gcc (GCC) 7.2.1 20170915 (Red Hat Cross 7.2.1-1)
>>>> Complain, that R1 is missing, old compilers like
>>>> 	gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-16)
>>>> will complain that R1 is not valid.
>>>>
>>>> According to the architecture, R1 is valid but ignored. No idea why this
>>>> was changed (in a way that programs no longer compile).
>>>
>>> What a pity :-(
>>
>> IIR, this was a bugfix. gcc is also used for TPF and they actually use TB and
>> the old binutils support for TB was architecturally wrong. Since TB is really
>> of no use for Linux this should not matter that much apart from the testcases
>> to test the KVM implementation.
>>
> 
> Well they managed to break code, so this was indeed a bad decision.

With that Binutils change we only report an error for code which was broken anyway.

When writing this testcase the right thing would have been to report a Binutils bug instead of
writing a testcase which uses an instruction which isn't part of the POP.

-Andreas-

Re: [PATCH kvm-unit-tests 1/9] s390x: fix TEST BLOCK tests

[David Hildenbrand]

>> Well they managed to break code, so this was indeed a bad decision.
> 
> With that Binutils change we only report an error for code which was broken anyway.

Well it worked, so it wasn't broken. I would just have liked this
instruction to be fixed in a backwards compatible way (e.g. warning - or
is it just a warning and my gcc flags treat them as errors?).

> 
> When writing this testcase the right thing would have been to report a Binutils bug instead of
> writing a testcase which uses an instruction which isn't part of the POP.

Huh?

PoP - "Control Instructions" - 10-176 - "TEST BLOCK". Even can find it
in the Pop from 2004.

But anyhow, we have this fixed now. Was just wondering, why our
_working_ code suddenly broke (and now we have to use .insn to make it
compile with GCC in general).

Thanks.

> 
> -Andreas-
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH kvm-unit-tests 1/9] s390x: fix TEST BLOCK tests

[Andreas Krebbel]

On 01/11/2018 08:11 PM, David Hildenbrand wrote:
> 
>>> Well they managed to break code, so this was indeed a bad decision.
>>
>> With that Binutils change we only report an error for code which was broken anyway.
> 
> Well it worked, so it wasn't broken. I would just have liked this
> instruction to be fixed in a backwards compatible way (e.g. warning - or
> is it just a warning and my gcc flags treat them as errors?).

One operand was missing. The ommitted operand was just 0 then. The instruction never did what the
user expected. That nobody complained about using it that way doesn't mean it was correct.

>> When writing this testcase the right thing would have been to report a Binutils bug instead of
>> writing a testcase which uses an instruction which isn't part of the POP.
> 
> Huh?
> 
> PoP - "Control Instructions" - 10-176 - "TEST BLOCK". Even can find it
> in the Pop from 2004.

It wasn't in the POP the way the testcase used it - with just one operand. It was a bug in Binutils
since the very beginning probably.

> But anyhow, we have this fixed now. Was just wondering, why our
> _working_ code suddenly broke (and now we have to use .insn to make it
> compile with GCC in general)
-Andreas-

[PATCH kvm-unit-tests 2/9] s390x: use highest addresses for PGM_ADDRESSING errors

[David Hildenbrand]

Without the UL, we get 32 bit addresses, resulting in different memory
addresses. This is necessary for enabling the MMU.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 s390x/intercept.c | 10 +++++-----
 s390x/selftest.c  |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/s390x/intercept.c b/s390x/intercept.c
index 99dde0d..b6027b2 100644
--- a/s390x/intercept.c
+++ b/s390x/intercept.c
@@ -46,7 +46,7 @@ static void test_stpx(void)
 	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
 
 	expect_pgm_int();
-	asm volatile(" stpx 0(%0) " : : "r"(-8));
+	asm volatile(" stpx 0(%0) " : : "r"(-8UL));
 	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
 }
 
@@ -78,7 +78,7 @@ static void test_spx(void)
 	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
 
 	expect_pgm_int();
-	asm volatile(" spx 0(%0) " : : "r"(-8));
+	asm volatile(" spx 0(%0) " : : "r"(-8UL));
 	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
 }
 
@@ -101,7 +101,7 @@ static void test_stap(void)
 	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
 
 	expect_pgm_int();
-	asm volatile ("stap 0(%0)\n" : : "r"(-8));
+	asm volatile ("stap 0(%0)\n" : : "r"(-8UL));
 	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
 }
 
@@ -126,7 +126,7 @@ static void test_stidp(void)
 	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
 
 	expect_pgm_int();
-	asm volatile ("stidp 0(%0)\n" : : "r"(-8));
+	asm volatile ("stidp 0(%0)\n" : : "r"(-8UL));
 	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
 }
 
@@ -155,7 +155,7 @@ static void test_testblock(void)
 	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
 
 	expect_pgm_int();
-	asm volatile (" .insn	rre,0xb22c0000,0,%0\n" : : "r"(-4096));
+	asm volatile (" .insn	rre,0xb22c0000,0,%0\n" : : "r"(-4096UL));
 	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
 }
 
diff --git a/s390x/selftest.c b/s390x/selftest.c
index 1c8d16a..76ed4bf 100644
--- a/s390x/selftest.c
+++ b/s390x/selftest.c
@@ -33,7 +33,7 @@ static void test_pgm_int(void)
 	check_pgm_int_code(PGM_INT_CODE_OPERATION);
 
 	expect_pgm_int();
-	asm volatile("	stg %0,0(%0)\n" : : "r"(-1));
+	asm volatile("	stg %0,0(%0)\n" : : "r"(-1UL));
 	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
 }
 
-- 
2.14.3

Re: [PATCH kvm-unit-tests 2/9] s390x: use highest addresses for PGM_ADDRESSING errors

[Thomas Huth]

On 10.01.2018 22:53, David Hildenbrand wrote:
> Without the UL, we get 32 bit addresses, resulting in different memory
> addresses. This is necessary for enabling the MMU.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  s390x/intercept.c | 10 +++++-----
>  s390x/selftest.c  |  2 +-
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/s390x/intercept.c b/s390x/intercept.c
> index 99dde0d..b6027b2 100644
> --- a/s390x/intercept.c
> +++ b/s390x/intercept.c
> @@ -46,7 +46,7 @@ static void test_stpx(void)
>  	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>  
>  	expect_pgm_int();
> -	asm volatile(" stpx 0(%0) " : : "r"(-8));
> +	asm volatile(" stpx 0(%0) " : : "r"(-8UL));

I think I'd slightly prefer just "L" as suffix instead of "UL" ... in
practice, I guess it does not matter here though, so:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH kvm-unit-tests 2/9] s390x: use highest addresses for PGM_ADDRESSING errors

[David Hildenbrand]

On 11.01.2018 09:57, Thomas Huth wrote:
> On 10.01.2018 22:53, David Hildenbrand wrote:
>> Without the UL, we get 32 bit addresses, resulting in different memory
>> addresses. This is necessary for enabling the MMU.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  s390x/intercept.c | 10 +++++-----
>>  s390x/selftest.c  |  2 +-
>>  2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/s390x/intercept.c b/s390x/intercept.c
>> index 99dde0d..b6027b2 100644
>> --- a/s390x/intercept.c
>> +++ b/s390x/intercept.c
>> @@ -46,7 +46,7 @@ static void test_stpx(void)
>>  	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>>  
>>  	expect_pgm_int();
>> -	asm volatile(" stpx 0(%0) " : : "r"(-8));
>> +	asm volatile(" stpx 0(%0) " : : "r"(-8UL));
> 
> I think I'd slightly prefer just "L" as suffix instead of "UL" ... in
> practice, I guess it does not matter here though, so:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Will change, thanks!

-- 

Thanks,

David / dhildenb

[PATCH kvm-unit-tests 3/9] s390x: increase the stack size

[David Hildenbrand]

160 bytes is a little small. 16k sounds better.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 s390x/flat.lds | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/s390x/flat.lds b/s390x/flat.lds
index b6e2172..6bd3c23 100644
--- a/s390x/flat.lds
+++ b/s390x/flat.lds
@@ -37,6 +37,6 @@ SECTIONS
 	/*
 	 * stackptr set with initial stack frame preallocated
 	 */
-	stackptr = . - 160;
+	stackptr = . - 16k;
 	stacktop = .;
 }
-- 
2.14.3

Re: [PATCH kvm-unit-tests 3/9] s390x: increase the stack size

[Thomas Huth]

On 10.01.2018 22:53, David Hildenbrand wrote:
> 160 bytes is a little small. 16k sounds better.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  s390x/flat.lds | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/s390x/flat.lds b/s390x/flat.lds
> index b6e2172..6bd3c23 100644
> --- a/s390x/flat.lds
> +++ b/s390x/flat.lds
> @@ -37,6 +37,6 @@ SECTIONS
>  	/*
>  	 * stackptr set with initial stack frame preallocated
>  	 */
> -	stackptr = . - 160;
> +	stackptr = . - 16k;
>  	stacktop = .;
>  }

Hu? Why?

The "-160" is just for the first initial stack frame. The total size of
the stack is handled with the ". += 64K;" right before the comment instead.

 Thomas

Re: [PATCH kvm-unit-tests 3/9] s390x: increase the stack size

[David Hildenbrand]

On 11.01.2018 10:19, Thomas Huth wrote:
> On 10.01.2018 22:53, David Hildenbrand wrote:
>> 160 bytes is a little small. 16k sounds better.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  s390x/flat.lds | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/s390x/flat.lds b/s390x/flat.lds
>> index b6e2172..6bd3c23 100644
>> --- a/s390x/flat.lds
>> +++ b/s390x/flat.lds
>> @@ -37,6 +37,6 @@ SECTIONS
>>  	/*
>>  	 * stackptr set with initial stack frame preallocated
>>  	 */
>> -	stackptr = . - 160;
>> +	stackptr = . - 16k;
>>  	stacktop = .;
>>  }
> 
> Hu? Why?
> 
> The "-160" is just for the first initial stack frame. The total size of
> the stack is handled with the ". += 64K;" right before the comment instead.

Indeed, got this wrong. Dropped. (and replaced by a fix for stackptr vs.
stacktop)

Thanks!

> 
>  Thomas
> 


-- 

Thanks,

David / dhildenb

[PATCH kvm-unit-tests 4/9] s390x: add missing sclp definitions from QEMU

[David Hildenbrand]

Copy all missing definitions from QEMU (include/hw/s390x/sclp.h).
We cannot simply replace the file, as some ASCII definitions are missing
in the QEMU file (defined somewhere else in QEMU).

Convert QEMU_PACKED accordingly and copy the copyright statement.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 lib/s390x/sclp.h | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 106 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
index 3f4c138..000cbb7 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -1,8 +1,13 @@
 /*
- * SCLP ASCII access driver
+ * SCLP definitions
  *
+ * Copyright IBM, Corp. 2012
  * Copyright (c) 2013 Alexander Graf <agraf@suse.de>
  *
+ * Authors:
+ *  Christian Borntraeger <borntraeger@de.ibm.com>
+ *  Alexander Graf <agraf@suse.de>$
+ *
  * This work is licensed under the terms of the GNU GPL, version 2 or (at
  * your option) any later version. See the COPYING file in the top-level
  * directory.
@@ -11,21 +16,51 @@
 #ifndef SCLP_H
 #define SCLP_H
 
+#define SCLP_CMD_CODE_MASK                      0xffff00ff
+
 /* SCLP command codes */
 #define SCLP_CMDW_READ_SCP_INFO                 0x00020001
 #define SCLP_CMDW_READ_SCP_INFO_FORCED          0x00120001
+#define SCLP_READ_STORAGE_ELEMENT_INFO          0x00040001
+#define SCLP_ATTACH_STORAGE_ELEMENT             0x00080001
+#define SCLP_ASSIGN_STORAGE                     0x000D0001
 #define SCLP_CMD_READ_EVENT_DATA                0x00770005
 #define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
 #define SCLP_CMD_READ_EVENT_DATA                0x00770005
 #define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
 #define SCLP_CMD_WRITE_EVENT_MASK               0x00780005
 
+/* SCLP Memory hotplug codes */
+#define SCLP_FC_ASSIGN_ATTACH_READ_STOR         0xE00000000000ULL
+#define SCLP_STARTING_SUBINCREMENT_ID           0x10001
+#define SCLP_INCREMENT_UNIT                     0x10000
+#define MAX_AVAIL_SLOTS                         32
+#define MAX_STORAGE_INCREMENTS                  1020
+
+/* CPU hotplug SCLP codes */
+#define SCLP_HAS_CPU_INFO                       0x0C00000000000000ULL
+#define SCLP_CMDW_READ_CPU_INFO                 0x00010001
+#define SCLP_CMDW_CONFIGURE_CPU                 0x00110001
+#define SCLP_CMDW_DECONFIGURE_CPU               0x00100001
+
+/* SCLP PCI codes */
+#define SCLP_HAS_IOA_RECONFIG                   0x0000000040000000ULL
+#define SCLP_CMDW_CONFIGURE_IOA                 0x001a0001
+#define SCLP_CMDW_DECONFIGURE_IOA               0x001b0001
+#define SCLP_RECONFIG_PCI_ATYPE                 2
+
 /* SCLP response codes */
 #define SCLP_RC_NORMAL_READ_COMPLETION          0x0010
 #define SCLP_RC_NORMAL_COMPLETION               0x0020
+#define SCLP_RC_SCCB_BOUNDARY_VIOLATION         0x0100
+#define SCLP_RC_NO_ACTION_REQUIRED              0x0120
 #define SCLP_RC_INVALID_SCLP_COMMAND            0x01f0
 #define SCLP_RC_CONTAINED_EQUIPMENT_CHECK       0x0340
 #define SCLP_RC_INSUFFICIENT_SCCB_LENGTH        0x0300
+#define SCLP_RC_STANDBY_READ_COMPLETION         0x0410
+#define SCLP_RC_ADAPTER_IN_RESERVED_STATE       0x05f0
+#define SCLP_RC_ADAPTER_TYPE_NOT_RECOGNIZED     0x06f0
+#define SCLP_RC_ADAPTER_ID_NOT_RECOGNIZED       0x09f0
 #define SCLP_RC_INVALID_FUNCTION                0x40f0
 #define SCLP_RC_NO_EVENT_BUFFERS_STORED         0x60f0
 #define SCLP_RC_INVALID_SELECTION_MASK          0x70f0
@@ -50,13 +85,83 @@ typedef struct SCCBHeader {
 } __attribute__((packed)) SCCBHeader;
 
 #define SCCB_DATA_LEN (SCCB_SIZE - sizeof(SCCBHeader))
+#define SCCB_CPU_FEATURE_LEN 6
+
+/* CPU information */
+typedef struct CPUEntry {
+    uint8_t address;
+    uint8_t reserved0;
+    uint8_t features[SCCB_CPU_FEATURE_LEN];
+    uint8_t reserved2[6];
+    uint8_t type;
+    uint8_t reserved1;
+} __attribute__((packed)) CPUEntry;
 
 typedef struct ReadInfo {
     SCCBHeader h;
     uint16_t rnmax;
     uint8_t rnsize;
+    uint8_t  _reserved1[16 - 11];       /* 11-15 */
+    uint16_t entries_cpu;               /* 16-17 */
+    uint16_t offset_cpu;                /* 18-19 */
+    uint8_t  _reserved2[24 - 20];       /* 20-23 */
+    uint8_t  loadparm[8];               /* 24-31 */
+    uint8_t  _reserved3[48 - 32];       /* 32-47 */
+    uint64_t facilities;                /* 48-55 */
+    uint8_t  _reserved0[76 - 56];       /* 56-75 */
+    uint32_t ibc_val;
+    uint8_t  conf_char[99 - 80];        /* 80-98 */
+    uint8_t mha_pow;
+    uint32_t rnsize2;
+    uint64_t rnmax2;
+    uint8_t  _reserved6[116 - 112];     /* 112-115 */
+    uint8_t  conf_char_ext[120 - 116];   /* 116-119 */
+    uint16_t highest_cpu;
+    uint8_t  _reserved5[124 - 122];     /* 122-123 */
+    uint32_t hmfai;
+    struct CPUEntry entries[0];
 } __attribute__((packed)) ReadInfo;
 
+typedef struct ReadCpuInfo {
+    SCCBHeader h;
+    uint16_t nr_configured;         /* 8-9 */
+    uint16_t offset_configured;     /* 10-11 */
+    uint16_t nr_standby;            /* 12-13 */
+    uint16_t offset_standby;        /* 14-15 */
+    uint8_t reserved0[24-16];       /* 16-23 */
+    struct CPUEntry entries[0];
+} __attribute__((packed)) ReadCpuInfo;
+
+typedef struct ReadStorageElementInfo {
+    SCCBHeader h;
+    uint16_t max_id;
+    uint16_t assigned;
+    uint16_t standby;
+    uint8_t _reserved0[16 - 14]; /* 14-15 */
+    uint32_t entries[0];
+} __attribute__((packed)) ReadStorageElementInfo;
+
+typedef struct AttachStorageElement {
+    SCCBHeader h;
+    uint8_t _reserved0[10 - 8];  /* 8-9 */
+    uint16_t assigned;
+    uint8_t _reserved1[16 - 12]; /* 12-15 */
+    uint32_t entries[0];
+} __attribute__((packed)) AttachStorageElement;
+
+typedef struct AssignStorage {
+    SCCBHeader h;
+    uint16_t rn;
+} __attribute__((packed)) AssignStorage;
+
+typedef struct IoaCfgSccb {
+    SCCBHeader header;
+    uint8_t atype;
+    uint8_t reserved1;
+    uint16_t reserved2;
+    uint32_t aid;
+} __attribute__((packed)) IoaCfgSccb;
+
 typedef struct SCCB {
     SCCBHeader h;
     char data[SCCB_DATA_LEN];
-- 
2.14.3

Re: [PATCH kvm-unit-tests 4/9] s390x: add missing sclp definitions from QEMU

[Thomas Huth]

On 10.01.2018 22:53, David Hildenbrand wrote:
> Copy all missing definitions from QEMU (include/hw/s390x/sclp.h).
> We cannot simply replace the file, as some ASCII definitions are missing
> in the QEMU file (defined somewhere else in QEMU).

So this is is now a mixture from QEMU's pc-bios/s390-ccw/sclp.h and
include/hw/s390x/sclp.h files? ... I think we should maybe state that in
the header comment...

> Convert QEMU_PACKED accordingly and copy the copyright statement.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  lib/s390x/sclp.h | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 106 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
> index 3f4c138..000cbb7 100644
> --- a/lib/s390x/sclp.h
> +++ b/lib/s390x/sclp.h
> @@ -1,8 +1,13 @@
>  /*
> - * SCLP ASCII access driver
> + * SCLP definitions
>   *
> + * Copyright IBM, Corp. 2012
>   * Copyright (c) 2013 Alexander Graf <agraf@suse.de>
>   *
> + * Authors:
> + *  Christian Borntraeger <borntraeger@de.ibm.com>
> + *  Alexander Graf <agraf@suse.de>$

The "$" is certainly wrong.

I think I'd maybe rather write it like this:

/*
 * SCLP definitions
 *
 * Based on the file pc-bios/s390-ccw/sclp.h from QEMU
 * Copyright (c) 2013 Alexander Graf <agraf@suse.de>
 *
 * and based on the file include/hw/s390x/sclp.h from QEMU
 * Copyright IBM, Corp. 2012
 * Author: Christian Borntraeger <borntraeger@de.ibm.com>
 *
 * This work is licensed under the terms of the GNU GPL, version 2 or (at
 * your option) any later version. See the COPYING file in the top-level
 * directory.
 */

 Thomas

Re: [PATCH kvm-unit-tests 4/9] s390x: add missing sclp definitions from QEMU

[David Hildenbrand]

> I think I'd maybe rather write it like this:
> 

Thanks for your help, I always get such stuff wrong!

Will change!

> /*
>  * SCLP definitions
>  *
>  * Based on the file pc-bios/s390-ccw/sclp.h from QEMU
>  * Copyright (c) 2013 Alexander Graf <agraf@suse.de>
>  *
>  * and based on the file include/hw/s390x/sclp.h from QEMU
>  * Copyright IBM, Corp. 2012
>  * Author: Christian Borntraeger <borntraeger@de.ibm.com>
>  *
>  * This work is licensed under the terms of the GNU GPL, version 2 or (at
>  * your option) any later version. See the COPYING file in the top-level
>  * directory.
>  */
> 
>  Thomas
> 


-- 

Thanks,

David / dhildenb

[PATCH kvm-unit-tests 5/9] s390x: rename sclp_setup() to sclp_ascii_setup()

[David Hildenbrand]

We'll be introducing some other sclp related files including setup
functions soon. Also allow to call sclp_service_call() from these.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 lib/s390x/io.c         | 2 +-
 lib/s390x/sclp-ascii.c | 4 ++--
 lib/s390x/sclp.h       | 3 ++-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/s390x/io.c b/lib/s390x/io.c
index 12f9d26..3121a78 100644
--- a/lib/s390x/io.c
+++ b/lib/s390x/io.c
@@ -42,7 +42,7 @@ void setup()
 {
 	setup_args_progname(ipl_args);
 	setup_facilities();
-	sclp_setup();
+	sclp_ascii_setup();
 }
 
 void exit(int code)
diff --git a/lib/s390x/sclp-ascii.c b/lib/s390x/sclp-ascii.c
index dfb9e68..9c749a0 100644
--- a/lib/s390x/sclp-ascii.c
+++ b/lib/s390x/sclp-ascii.c
@@ -16,7 +16,7 @@
 static char _sccb[PAGE_SIZE] __attribute__((__aligned__(4096)));
 
 /* Perform service call. Return 0 on success, non-zero otherwise. */
-static int sclp_service_call(unsigned int command, void *sccb)
+int sclp_service_call(unsigned int command, void *sccb)
 {
         int cc;
 
@@ -47,7 +47,7 @@ static void sclp_set_write_mask(void)
     sclp_service_call(SCLP_CMD_WRITE_EVENT_MASK, sccb);
 }
 
-void sclp_setup(void)
+void sclp_ascii_setup(void)
 {
     sclp_set_write_mask();
 }
diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
index 000cbb7..ec9dcb1 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -207,7 +207,8 @@ typedef struct ReadEventData {
     uint32_t mask;
 } __attribute__((packed)) ReadEventData;
 
-void sclp_setup(void);
+void sclp_ascii_setup(void);
 void sclp_print(const char *str);
+int sclp_service_call(unsigned int command, void *sccb);
 
 #endif /* SCLP_H */
-- 
2.14.3

Re: [PATCH kvm-unit-tests 5/9] s390x: rename sclp_setup() to sclp_ascii_setup()

[Thomas Huth]

On 10.01.2018 22:53, David Hildenbrand wrote:
> We'll be introducing some other sclp related files including setup
> functions soon. Also allow to call sclp_service_call() from these.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  lib/s390x/io.c         | 2 +-
>  lib/s390x/sclp-ascii.c | 4 ++--
>  lib/s390x/sclp.h       | 3 ++-
>  3 files changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH kvm-unit-tests 6/9] s390x: detect installed memory

[David Hildenbrand]

Unfortunately, there is no easy way to simply read out the amount
of installed memory. We have to probe (via TEST PROTECTION) for installed
memory in a given range.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 lib/s390x/asm/arch_def.h | 12 +++++++++++
 lib/s390x/io.c           |  1 +
 lib/s390x/sclp.c         | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
 lib/s390x/sclp.h         |  1 +
 s390x/Makefile           |  1 +
 5 files changed, 67 insertions(+)
 create mode 100644 lib/s390x/sclp.c

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 72e5c60..ae27ab2 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -151,4 +151,16 @@ struct cpuid {
 	uint64_t reserved : 15;
 };
 
+static inline int tprot(unsigned long addr)
+{
+	int cc;
+
+	asm volatile(
+		"	tprot	%1,0\n"
+		"	ipm	%0\n"
+		"	srl	%0,28\n"
+		: "=d" (cc) : "Q" (addr) : "cc");
+	return cc;
+}
+
 #endif
diff --git a/lib/s390x/io.c b/lib/s390x/io.c
index 3121a78..eb4d171 100644
--- a/lib/s390x/io.c
+++ b/lib/s390x/io.c
@@ -43,6 +43,7 @@ void setup()
 	setup_args_progname(ipl_args);
 	setup_facilities();
 	sclp_ascii_setup();
+	sclp_memory_setup();
 }
 
 void exit(int code)
diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
new file mode 100644
index 0000000..ee56820
--- /dev/null
+++ b/lib/s390x/sclp.c
@@ -0,0 +1,52 @@
+/*
+ * s390x SCLP driver
+ *
+ * Copyright (c) 2017 Red Hat Inc
+ *
+ * Authors:
+ *  David Hildenbrand <david@redhat.com>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Library General Public License version 2.
+ */
+
+#include <libcflat.h>
+#include <asm/page.h>
+#include <asm/arch_def.h>
+#include "sclp.h"
+
+static char _sccb[PAGE_SIZE] __attribute__((__aligned__(4096)));
+static uint64_t storage_increment_size;
+static uint64_t max_ram_size;
+static uint64_t ram_size;
+
+void sclp_memory_setup(void)
+{
+	ReadInfo *ri = (void *)_sccb;
+	uint64_t rnmax, rnsize;
+
+	ri->h.length = SCCB_SIZE;
+	sclp_service_call(SCLP_CMDW_READ_SCP_INFO_FORCED, ri);
+
+	/* calculate the storage increment size */
+	rnsize = ri->rnsize;
+	if (!rnsize) {
+		rnsize = ri->rnsize2;
+	}
+	storage_increment_size = rnsize << 20;
+
+	/* calculate the maximum memory size */
+	rnmax = ri->rnmax;
+	if (!rnmax) {
+		rnmax = ri->rnmax2;
+	}
+	max_ram_size = rnmax * storage_increment_size;
+
+	/* lowcore is always accessible, so the first increment is accessible */
+	ram_size = storage_increment_size;
+
+	/* probe for r/w memory up to max memory size */
+	while (ram_size < max_ram_size &&
+	       tprot(ram_size + storage_increment_size - 1))
+		ram_size += storage_increment_size;
+}
diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
index ec9dcb1..94ff74b 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -210,5 +210,6 @@ typedef struct ReadEventData {
 void sclp_ascii_setup(void);
 void sclp_print(const char *str);
 int sclp_service_call(unsigned int command, void *sccb);
+void sclp_memory_setup(void);
 
 #endif /* SCLP_H */
diff --git a/s390x/Makefile b/s390x/Makefile
index f585faf..ce63dd1 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -24,6 +24,7 @@ cflatobjs += lib/util.o
 cflatobjs += lib/alloc_phys.o
 cflatobjs += lib/s390x/io.o
 cflatobjs += lib/s390x/stack.o
+cflatobjs += lib/s390x/sclp.o
 cflatobjs += lib/s390x/sclp-ascii.o
 cflatobjs += lib/s390x/interrupt.o
 
-- 
2.14.3

Re: [PATCH kvm-unit-tests 6/9] s390x: detect installed memory

[Thomas Huth]

On 10.01.2018 22:53, David Hildenbrand wrote:
> Unfortunately, there is no easy way to simply read out the amount
> of installed memory. We have to probe (via TEST PROTECTION) for installed
> memory in a given range.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  lib/s390x/asm/arch_def.h | 12 +++++++++++
>  lib/s390x/io.c           |  1 +
>  lib/s390x/sclp.c         | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/s390x/sclp.h         |  1 +
>  s390x/Makefile           |  1 +
>  5 files changed, 67 insertions(+)
>  create mode 100644 lib/s390x/sclp.c
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 72e5c60..ae27ab2 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -151,4 +151,16 @@ struct cpuid {
>  	uint64_t reserved : 15;
>  };
>  
> +static inline int tprot(unsigned long addr)
> +{
> +	int cc;
> +
> +	asm volatile(
> +		"	tprot	%1,0\n"
> +		"	ipm	%0\n"
> +		"	srl	%0,28\n"
> +		: "=d" (cc) : "Q" (addr) : "cc");
> +	return cc;
> +}
> +
>  #endif
> diff --git a/lib/s390x/io.c b/lib/s390x/io.c
> index 3121a78..eb4d171 100644
> --- a/lib/s390x/io.c
> +++ b/lib/s390x/io.c
> @@ -43,6 +43,7 @@ void setup()
>  	setup_args_progname(ipl_args);
>  	setup_facilities();
>  	sclp_ascii_setup();
> +	sclp_memory_setup();
>  }
>  
>  void exit(int code)
> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> new file mode 100644
> index 0000000..ee56820
> --- /dev/null
> +++ b/lib/s390x/sclp.c
> @@ -0,0 +1,52 @@
> +/*
> + * s390x SCLP driver
> + *
> + * Copyright (c) 2017 Red Hat Inc
> + *
> + * Authors:
> + *  David Hildenbrand <david@redhat.com>
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Library General Public License version 2.
> + */
> +
> +#include <libcflat.h>
> +#include <asm/page.h>
> +#include <asm/arch_def.h>
> +#include "sclp.h"
> +
> +static char _sccb[PAGE_SIZE] __attribute__((__aligned__(4096)));

Could we maybe re-use the sccb buffer from sclp-write.c ?

Maybe it would also make sense to merge sclp-write.c with sclp.c ?

 Thomas

Re: [PATCH kvm-unit-tests 6/9] s390x: detect installed memory

[David Hildenbrand]

>> +#include <libcflat.h>
>> +#include <asm/page.h>
>> +#include <asm/arch_def.h>
>> +#include "sclp.h"
>> +
>> +static char _sccb[PAGE_SIZE] __attribute__((__aligned__(4096)));
> 
> Could we maybe re-use the sccb buffer from sclp-write.c ?

We can do that, we just have to make sure that later on with multiple
CPUs, we don't call sclp functions at the same time (we could later use
a lock or locally allocate a page).

I assume secondary CPUs (e.g. SIGP tests I am working on) will not print
to console / use SCLP.

> 
> Maybe it would also make sense to merge sclp-write.c with sclp.c ?

I want to keep this separate for now because

a) I'll be introducing CPU + feature detection via sclp soon
b) I might be introducing printing also to linemode console soon

Especially b) can also be added to the pc-bios first. As we copied the
file from QEMU this makes it easier to sync changes.

Thanks!

> 
>  Thomas
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH kvm-unit-tests 6/9] s390x: detect installed memory

[David Hildenbrand]

> +	/* probe for r/w memory up to max memory size */
> +	while (ram_size < max_ram_size &&
> +	       tprot(ram_size + storage_increment_size - 1))

btw, I messed this up while refactoring. this has to be

tprot(ram_size + storage_increment_size - 1) == 0

> +		ram_size += storage_increment_size;
> +}


-- 

Thanks,

David / dhildenb

[PATCH kvm-unit-tests 7/9] s390x: initialize the physical allocator

[David Hildenbrand]

We now have the range of free memory, let's initialize the physical
allocator. It is now possible to use alloc_page()/alloc_pages().

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 lib/s390x/sclp.c | 13 +++++++++++++
 s390x/Makefile   |  3 +++
 2 files changed, 16 insertions(+)

diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
index ee56820..c0492b8 100644
--- a/lib/s390x/sclp.c
+++ b/lib/s390x/sclp.c
@@ -14,14 +14,23 @@
 #include <asm/page.h>
 #include <asm/arch_def.h>
 #include "sclp.h"
+#include <alloc_phys.h>
+
+extern unsigned long stacktop;
 
 static char _sccb[PAGE_SIZE] __attribute__((__aligned__(4096)));
 static uint64_t storage_increment_size;
 static uint64_t max_ram_size;
 static uint64_t ram_size;
 
+static void mem_init(phys_addr_t freemem_start, phys_addr_t mem_end)
+{
+	phys_alloc_init(freemem_start, ram_size - freemem_start);
+}
+
 void sclp_memory_setup(void)
 {
+	phys_addr_t freemem_start;
 	ReadInfo *ri = (void *)_sccb;
 	uint64_t rnmax, rnsize;
 
@@ -49,4 +58,8 @@ void sclp_memory_setup(void)
 	while (ram_size < max_ram_size &&
 	       tprot(ram_size + storage_increment_size - 1))
 		ram_size += storage_increment_size;
+
+	/* leave another extra page free */
+	freemem_start = ((phys_addr_t)&stacktop + PAGE_SIZE) & PAGE_MASK;
+	mem_init(freemem_start, ram_size);
 }
diff --git a/s390x/Makefile b/s390x/Makefile
index ce63dd1..4198fdc 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -21,6 +21,9 @@ asm-offsets = lib/$(ARCH)/asm-offsets.h
 include $(SRCDIR)/scripts/asm-offsets.mak
 
 cflatobjs += lib/util.o
+cflatobjs += lib/alloc.o
+cflatobjs += lib/alloc_phys.o
+cflatobjs += lib/alloc_page.o
 cflatobjs += lib/alloc_phys.o
 cflatobjs += lib/s390x/io.o
 cflatobjs += lib/s390x/stack.o
-- 
2.14.3

Re: [PATCH kvm-unit-tests 7/9] s390x: initialize the physical allocator

[Thomas Huth]

On 10.01.2018 22:53, David Hildenbrand wrote:
> We now have the range of free memory, let's initialize the physical
> allocator. It is now possible to use alloc_page()/alloc_pages().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  lib/s390x/sclp.c | 13 +++++++++++++
>  s390x/Makefile   |  3 +++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> index ee56820..c0492b8 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -14,14 +14,23 @@
>  #include <asm/page.h>
>  #include <asm/arch_def.h>
>  #include "sclp.h"
> +#include <alloc_phys.h>
> +
> +extern unsigned long stacktop;
>  
>  static char _sccb[PAGE_SIZE] __attribute__((__aligned__(4096)));
>  static uint64_t storage_increment_size;
>  static uint64_t max_ram_size;
>  static uint64_t ram_size;
>  
> +static void mem_init(phys_addr_t freemem_start, phys_addr_t mem_end)
> +{
> +	phys_alloc_init(freemem_start, ram_size - freemem_start);
> +}
> +
>  void sclp_memory_setup(void)
>  {
> +	phys_addr_t freemem_start;
>  	ReadInfo *ri = (void *)_sccb;
>  	uint64_t rnmax, rnsize;
>  
> @@ -49,4 +58,8 @@ void sclp_memory_setup(void)
>  	while (ram_size < max_ram_size &&
>  	       tprot(ram_size + storage_increment_size - 1))
>  		ram_size += storage_increment_size;
> +
> +	/* leave another extra page free */
> +	freemem_start = ((phys_addr_t)&stacktop + PAGE_SIZE) & PAGE_MASK;

<bikeshedpainting>
I think I'd rather move that above line into mem_init() instead...
</bikeshedpainting>

 Thomas

Re: [PATCH kvm-unit-tests 7/9] s390x: initialize the physical allocator

[Paolo Bonzini]

On 11/01/2018 11:29, Thomas Huth wrote:
>> +	/* leave another extra page free */
>> +	freemem_start = ((phys_addr_t)&stacktop + PAGE_SIZE) & PAGE_MASK;
> <bikeshedpainting>
> I think I'd rather move that above line into mem_init() instead...
> </bikeshedpainting>

Also say why. :)

Paolo

Re: [PATCH kvm-unit-tests 7/9] s390x: initialize the physical allocator

[Thomas Huth]

On 11.01.2018 15:00, Paolo Bonzini wrote:
> On 11/01/2018 11:29, Thomas Huth wrote:
>>> +	/* leave another extra page free */
>>> +	freemem_start = ((phys_addr_t)&stacktop + PAGE_SIZE) & PAGE_MASK;
>> <bikeshedpainting>
>> I think I'd rather move that above line into mem_init() instead...
>> </bikeshedpainting>
> 
> Also say why. :)

The calculation does not really belong to the other stuff of
sclp_memory_setup(). And you then also you don't need that freemem_start
variable in sclp_memory_setup() anymore.

 Thomas

Re: [PATCH kvm-unit-tests 7/9] s390x: initialize the physical allocator

[Paolo Bonzini]

On 11/01/2018 16:16, Thomas Huth wrote:
> On 11.01.2018 15:00, Paolo Bonzini wrote:
>> On 11/01/2018 11:29, Thomas Huth wrote:
>>>> +	/* leave another extra page free */
>>>> +	freemem_start = ((phys_addr_t)&stacktop + PAGE_SIZE) & PAGE_MASK;
>>> <bikeshedpainting>
>>> I think I'd rather move that above line into mem_init() instead...
>>> </bikeshedpainting>
>>
>> Also say why. :)
> 
> The calculation does not really belong to the other stuff of
> sclp_memory_setup(). And you then also you don't need that freemem_start
> variable in sclp_memory_setup() anymore.

Sorry, say why another page should be free...

Paolo

Re: [PATCH kvm-unit-tests 7/9] s390x: initialize the physical allocator

[David Hildenbrand]

On 11.01.2018 16:49, Paolo Bonzini wrote:
> On 11/01/2018 16:16, Thomas Huth wrote:
>> On 11.01.2018 15:00, Paolo Bonzini wrote:
>>> On 11/01/2018 11:29, Thomas Huth wrote:
>>>>> +	/* leave another extra page free */
>>>>> +	freemem_start = ((phys_addr_t)&stacktop + PAGE_SIZE) & PAGE_MASK;
>>>> <bikeshedpainting>
>>>> I think I'd rather move that above line into mem_init() instead...
>>>> </bikeshedpainting>
>>>
>>> Also say why. :)
>>
>> The calculation does not really belong to the other stuff of
>> sclp_memory_setup(). And you then also you don't need that freemem_start
>> variable in sclp_memory_setup() anymore.
> 
> Sorry, say why another page should be free...
> 
> Paolo
> 

Of course because I was trying to hide another bug :)

... without this, I got strange hangs when running tests (like
overwriting the stack).

Turned out we are setting the initital stack to stacktop instead of
stackptr. With that fixed, it works without the extra page.

-- 

Thanks,

David / dhildenb

[PATCH kvm-unit-tests 8/9] s390x: add vmalloc support

[David Hildenbrand]

To use virtual addresses, we have to
- build page tables with identity mapping
- setup the primary ASCE in cr1
- enable DAT in the PSW

Not using the Linux definititons/implementation as they contain too much
software defined stuff / things we don't need.

Written from scratch. Tried to stick to the general Linux naming
schemes.

As we currently don't invalidate anything except page table entries, it
is suficient to only use ipte for now.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 lib/s390x/asm/arch_def.h |  45 ++++++++++
 lib/s390x/asm/page.h     |  24 +++++
 lib/s390x/asm/pgtable.h  | 222 +++++++++++++++++++++++++++++++++++++++++++++++
 lib/s390x/mmu.c          |  90 +++++++++++++++++++
 lib/s390x/sclp.c         |   1 +
 s390x/Makefile           |   2 +
 s390x/cstart64.S         |   3 +-
 7 files changed, 386 insertions(+), 1 deletion(-)
 create mode 100644 lib/s390x/asm/pgtable.h
 create mode 100644 lib/s390x/mmu.c

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index ae27ab2..416dc75 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -163,4 +163,49 @@ static inline int tprot(unsigned long addr)
 	return cc;
 }
 
+static inline void lctlg(int cr, uint64_t value)
+{
+	asm volatile(
+		"	lctlg	%1,%1,%0\n"
+		: : "Q" (value), "i" (cr));
+}
+
+static inline uint64_t stctg(int cr)
+{
+	uint64_t value;
+
+	asm volatile(
+		"	stctg	%1,%1,%0\n"
+		: "=Q" (value) : "i" (cr) : "memory");
+	return value;
+}
+
+static inline void configure_dat(int enable)
+{
+	struct psw psw = {
+		.mask = 0,
+		.addr = 0,
+	};
+
+	asm volatile(
+		/* fetch the old PSW masks */
+		"	epsw	%1,%2\n"
+		/* remove the DAT flag first */
+		"	nilh	%1,0xfbff\n"
+		"	or	%3,%3\n"
+		"	jz	disable\n"
+		/* set DAT flag if enable == true */
+		"	oilh	%1,0x0400\n"
+		"	st	%1,0(%0)\n"
+		"disable:\n"
+		"	st	%2,4(%0)\n"
+		/* load the target address for our new PSW */
+		"	larl	%1,cont\n"
+		"	stg	%1,8(%0)\n"
+		"	lpswe	0(%0)\n"
+		"cont:\n"
+		: : "a" (&psw), "r" (0), "r" (1), "r" (enable)
+		: "memory", "cc");
+}
+
 #endif
diff --git a/lib/s390x/asm/page.h b/lib/s390x/asm/page.h
index 141a456..e7cc16d 100644
--- a/lib/s390x/asm/page.h
+++ b/lib/s390x/asm/page.h
@@ -13,4 +13,28 @@
 
 #include <asm-generic/page.h>
 
+typedef uint64_t pgdval_t;		/* Region-1 table entry */
+typedef uint64_t p4dval_t;		/* Region-2 table entry*/
+typedef uint64_t pudval_t;		/* Region-3 table Entry */
+typedef uint64_t pmdval_t;		/* Segment table entry */
+typedef uint64_t pteval_t;		/* Page table entry */
+
+typedef struct { pgdval_t pgd; } pgd_t;
+typedef struct { p4dval_t p4d; } p4d_t;
+typedef struct { pudval_t pud; } pud_t;
+typedef struct { pmdval_t pmd; } pmd_t;
+typedef struct { pteval_t pte; } pte_t;
+
+#define pgd_val(x)	((x).pgd)
+#define p4d_val(x)	((x).p4d)
+#define pud_val(x)	((x).pud)
+#define pmd_val(x)	((x).pmd)
+#define pte_val(x)	((x).pte)
+
+#define __pgd(x)	((pgd_t) { (x) } )
+#define __p4d(x)	((p4d_t) { (x) } )
+#define __pud(x)	((pud_t) { (x) } )
+#define __pmd(x)	((pmd_t) { (x) } )
+#define __pte(x)	((pte_t) { (x) } )
+
 #endif
diff --git a/lib/s390x/asm/pgtable.h b/lib/s390x/asm/pgtable.h
new file mode 100644
index 0000000..88713b5
--- /dev/null
+++ b/lib/s390x/asm/pgtable.h
@@ -0,0 +1,222 @@
+/*
+ * s390x page table definitions and functions
+ *
+ * Copyright (c) 2017 Red Hat Inc
+ *
+ * Authors:
+ *  David Hildenbrand <david@redhat.com>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Library General Public License version 2.
+ */
+#ifndef _ASMS390X_PGTABLE_H_
+#define _ASMS390X_PGTABLE_H_
+
+#include <asm/page.h>
+#include <alloc_page.h>
+
+#define ASCE_ORIGIN			0xfffffffffffff000UL
+#define ASCE_G				0x0000000000000200UL
+#define ASCE_P				0x0000000000000100UL
+#define ASCE_S				0x0000000000000080UL
+#define ASCE_X				0x0000000000000040UL
+#define ASCE_R				0x0000000000000020UL
+#define ASCE_DT				0x000000000000000cUL
+#define ASCE_TL				0x0000000000000003UL
+
+#define ASCE_DT_REGION1			0x000000000000000cUL
+#define ASCE_DT_REGION2			0x0000000000000008UL
+#define ASCE_DT_REGION3			0x0000000000000004UL
+#define ASCE_DT_SEGMENT			0x0000000000000000UL
+
+#define REGION_TABLE_ORDER		2
+#define REGION_TABLE_ENTRIES		2048
+#define REGION_TABLE_LENGTH		3
+
+#define REGION1_SHIFT			53
+#define REGION2_SHIFT			42
+#define REGION3_SHIFT			31
+
+#define REGION_ENTRY_ORIGIN		0xfffffffffffff000UL
+#define REGION_ENTRY_P			0x0000000000000200UL
+#define REGION_ENTRY_TF			0x00000000000000c0UL
+#define REGION_ENTRY_I			0x0000000000000020UL
+#define REGION_ENTRY_TT			0x000000000000000cUL
+#define REGION_ENTRY_TL			0x0000000000000003UL
+
+#define REGION_ENTRY_TT_REGION1		0x000000000000000cUL
+#define REGION_ENTRY_TT_REGION2		0x0000000000000008UL
+#define REGION_ENTRY_TT_REGION3		0x0000000000000004UL
+
+#define REGION3_ENTRY_RFAA		0xffffffff80000000UL
+#define REGION3_ENTRY_AV		0x0000000000010000UL
+#define REGION3_ENTRY_ACC		0x000000000000f000UL
+#define REGION3_ENTRY_F			0x0000000000000800UL
+#define REGION3_ENTRY_FC		0x0000000000000400UL
+#define REGION3_ENTRY_IEP		0x0000000000000100UL
+#define REGION3_ENTRY_CR		0x0000000000000010UL
+
+#define SEGMENT_TABLE_ORDER		2
+#define SEGMENT_TABLE_ENTRIES		2048
+#define SEGMENT_TABLE_LENGTH		3
+#define SEGMENT_SHIFT			20
+
+#define SEGMENT_ENTRY_ORIGIN		0xfffffffffffff800UL
+#define SEGMENT_ENTRY_SFAA		0xfffffffffff80000UL
+#define SEGMENT_ENTRY_AV		0x0000000000010000UL
+#define SEGMENT_ENTRY_ACC		0x000000000000f000UL
+#define SEGMENT_ENTRY_F			0x0000000000000800UL
+#define SEGMENT_ENTRY_FC		0x0000000000000400UL
+#define SEGMENT_ENTRY_P			0x0000000000000200UL
+#define SEGMENT_ENTRY_IEP		0x0000000000000100UL
+#define SEGMENT_ENTRY_I			0x0000000000000020UL
+#define SEGMENT_ENTRY_CS		0x0000000000000010UL
+#define SEGMENT_ENTRY_TT		0x000000000000000cUL
+
+#define SEGMENT_ENTRY_TT_REGION1	0x000000000000000cUL
+#define SEGMENT_ENTRY_TT_REGION2	0x0000000000000008UL
+#define SEGMENT_ENTRY_TT_REGION3	0x0000000000000004UL
+#define SEGMENT_ENTRY_TT_SEGMENT	0x0000000000000000UL
+
+#define PAGE_TABLE_ORDER		0
+#define PAGE_TABLE_ENTRIES		256
+
+#define PAGE_ENTRY_I			0x000000000000400UL
+#define PAGE_ENTRY_P			0x000000000000200UL
+#define PAGE_ENTRY_IEP			0x000000000000100UL
+
+#define PTRS_PER_PGD			REGION_TABLE_ENTRIES
+#define PTRS_PER_P4D			REGION_TABLE_ENTRIES
+#define PTRS_PER_PUD			REGION_TABLE_ENTRIES
+#define PTRS_PER_PMD			SEGMENT_TABLE_ENTRIES
+#define PTRS_PER_PTE			PAGE_TABLE_ENTRIES
+
+#define PGDIR_SHIFT			REGION1_SHIFT
+#define P4D_SHIFT			REGION2_SHIFT
+#define PUD_SHIFT			REGION3_SHIFT
+#define PMD_SHIFT			SEGMENT_SHIFT
+
+#define pgd_none(entry) (pgd_val(entry) & REGION_ENTRY_I)
+#define p4d_none(entry) (p4d_val(entry) & REGION_ENTRY_I)
+#define pud_none(entry) (pud_val(entry) & REGION_ENTRY_I)
+#define pmd_none(entry) (pmd_val(entry) & SEGMENT_ENTRY_I)
+#define pte_none(entry) (pte_val(entry) & PAGE_ENTRY_I)
+
+#define pgd_addr(entry) __va(pgd_val(entry) & REGION_ENTRY_ORIGIN)
+#define p4d_addr(entry) __va(p4d_val(entry) & REGION_ENTRY_ORIGIN)
+#define pud_addr(entry) __va(pud_val(entry) & REGION_ENTRY_ORIGIN)
+#define pmd_addr(entry) __va(pmd_val(entry) & SEGMENT_ENTRY_ORIGIN)
+#define pte_addr(entry) __va(pte_val(entry) & PAGE_MASK)
+
+#define pgd_index(addr) (((addr) >> PGDIR_SHIFT) & (PTRS_PER_PGD - 1))
+#define p4d_index(addr) (((addr) >> P4D_SHIFT) & (PTRS_PER_P4D - 1))
+#define pud_index(addr) (((addr) >> PUD_SHIFT) & (PTRS_PER_PUD - 1))
+#define pmd_index(addr) (((addr) >> PMD_SHIFT) & (PTRS_PER_PMD - 1))
+#define pte_index(addr) (((addr) >> PAGE_SHIFT) & (PTRS_PER_PTE - 1))
+
+#define pgd_offset(table, addr) ((pgd_t *)(table) + pgd_index(addr))
+#define p4d_offset(pgd, addr) ((p4d_t *)pgd_addr(*(pgd)) + p4d_index(addr))
+#define pud_offset(p4d, addr) ((pud_t *)p4d_addr(*(p4d)) + pud_index(addr))
+#define pmd_offset(pud, addr) ((pmd_t *)pud_addr(*(pud)) + pmd_index(addr))
+#define pte_offset(pmd, addr) ((pte_t *)pmd_addr(*(pmd)) + pte_index(addr))
+
+static inline pgd_t *pgd_alloc_one(void)
+{
+	pgd_t *pgd = alloc_pages(REGION_TABLE_ORDER);
+	int i;
+
+	for (i = 0; i < REGION_TABLE_ENTRIES; i++)
+		pgd_val(pgd[i]) = REGION_ENTRY_TT_REGION1 | REGION_ENTRY_I;
+	return pgd;
+}
+
+static inline p4d_t *p4d_alloc_one(void)
+{
+	p4d_t *p4d = alloc_pages(REGION_TABLE_ORDER);
+	int i;
+
+	for (i = 0; i < REGION_TABLE_ENTRIES; i++)
+		p4d_val(p4d[i]) = REGION_ENTRY_TT_REGION2 | REGION_ENTRY_I;
+	return p4d;
+}
+
+static inline p4d_t *p4d_alloc(pgd_t *pgd, unsigned long addr)
+{
+	if (pgd_none(*pgd)) {
+		p4d_t *p4d = p4d_alloc_one();
+		pgd_val(*pgd) = __pa(p4d) | REGION_ENTRY_TT_REGION1 |
+				REGION_TABLE_LENGTH;
+	}
+	return p4d_offset(pgd, addr);
+}
+
+static inline pud_t *pud_alloc_one(void)
+{
+	pud_t *pud = alloc_pages(REGION_TABLE_ORDER);
+	int i;
+
+	for (i = 0; i < REGION_TABLE_ENTRIES; i++)
+		pud_val(pud[i]) = REGION_ENTRY_TT_REGION3 | REGION_ENTRY_I;
+	return pud;
+}
+
+static inline pud_t *pud_alloc(p4d_t *p4d, unsigned long addr)
+{
+	if (p4d_none(*p4d)) {
+		pud_t *pud = pud_alloc_one();
+		p4d_val(*p4d) = __pa(pud) | REGION_ENTRY_TT_REGION2 |
+				REGION_TABLE_LENGTH;
+	}
+	return pud_offset(p4d, addr);
+}
+
+static inline pmd_t *pmd_alloc_one(void)
+{
+	pmd_t *pmd = alloc_pages(SEGMENT_TABLE_ORDER);
+	int i;
+
+	for (i = 0; i < SEGMENT_TABLE_ENTRIES; i++)
+		pmd_val(pmd[i]) = SEGMENT_ENTRY_TT_SEGMENT | SEGMENT_ENTRY_I;
+	return pmd;
+}
+
+static inline pmd_t *pmd_alloc(pud_t *pud, unsigned long addr)
+{
+	if (pud_none(*pud)) {
+		pmd_t *pmd = pmd_alloc_one();
+		pud_val(*pud) = __pa(pmd) | REGION_ENTRY_TT_REGION3 |
+				REGION_TABLE_LENGTH;
+	}
+	return pmd_offset(pud, addr);
+}
+
+static inline pte_t *pte_alloc_one(void)
+{
+	pte_t *pte = alloc_pages(PAGE_TABLE_ORDER);
+	int i;
+
+	for (i = 0; i < PAGE_TABLE_ENTRIES; i++)
+		pte_val(pte[i]) = PAGE_ENTRY_I;
+	return pte;
+}
+
+static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)
+{
+	if (pmd_none(*pmd)) {
+		pte_t *pte = pte_alloc_one();
+		pmd_val(*pmd) = __pa(pte) | SEGMENT_ENTRY_TT_SEGMENT |
+				SEGMENT_TABLE_LENGTH;
+	}
+	return pte_offset(pmd, addr);
+}
+
+static inline void ipte(unsigned long vaddr, pteval_t *p_pte)
+{
+	unsigned long table_origin = (unsigned long)p_pte & PAGE_MASK;
+
+	asm volatile(
+		"	ipte %0,%1\n"
+		: : "a" (table_origin), "a" (vaddr) : "memory");
+}
+
+#endif /* _ASMS390X_PGTABLE_H_ */
diff --git a/lib/s390x/mmu.c b/lib/s390x/mmu.c
new file mode 100644
index 0000000..d4d1d06
--- /dev/null
+++ b/lib/s390x/mmu.c
@@ -0,0 +1,90 @@
+/*
+ * s390x MMU
+ *
+ * Copyright (c) 2017 Red Hat Inc
+ *
+ * Authors:
+ *  David Hildenbrand <david@redhat.com>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Library General Public License version 2.
+ */
+
+#include <libcflat.h>
+#include <asm/pgtable.h>
+#include <asm/arch_def.h>
+#include <vmalloc.h>
+
+static void mmu_enable(pgd_t *pgtable)
+{
+	const uint64_t asce = __pa(pgtable) | ASCE_DT_REGION1 |
+			      REGION_TABLE_LENGTH;
+
+	/* set primary asce */
+	lctlg(1, asce);
+	assert(stctg(1) == asce);
+
+	/* enable dat (primary == 0 set as default) */
+	configure_dat(1);
+}
+
+static pteval_t *get_pte(pgd_t *pgtable, uintptr_t vaddr)
+{
+	pgd_t *pgd = pgd_offset(pgtable, vaddr);
+	p4d_t *p4d = p4d_alloc(pgd, vaddr);
+	pud_t *pud = pud_alloc(p4d, vaddr);
+	pmd_t *pmd = pmd_alloc(pud, vaddr);
+	pte_t *pte = pte_alloc(pmd, vaddr);
+
+	return &pte_val(*pte);
+}
+
+phys_addr_t virt_to_pte_phys(pgd_t *pgtable, void *vaddr)
+{
+	return (*get_pte(pgtable, (uintptr_t)vaddr) & PAGE_MASK) +
+	       ((unsigned long)vaddr & ~PAGE_MASK);
+}
+
+pteval_t *install_page(pgd_t *pgtable, phys_addr_t phys, void *vaddr)
+{
+	pteval_t *p_pte = get_pte(pgtable, (uintptr_t)vaddr);
+
+	/* first flush the old entry (if we're replacing anything) */
+	ipte((uintptr_t)vaddr, p_pte);
+
+	*p_pte = __pa(phys);
+	return p_pte;
+}
+
+static void setup_identity(pgd_t *pgtable, phys_addr_t start_addr,
+			   phys_addr_t end_addr)
+{
+	phys_addr_t cur;
+
+	start_addr &= PAGE_MASK;
+	for (cur = start_addr; true; cur += PAGE_SIZE) {
+		if (start_addr < end_addr && cur >= end_addr)
+			break;
+		if (start_addr > end_addr && cur <= end_addr)
+			break;
+		install_page(pgtable, cur, __va(cur));
+	}
+}
+
+void *setup_mmu(phys_addr_t phys_end){
+	pgd_t *page_root;
+
+	/* allocate a region-1 table */
+	page_root = pgd_alloc_one();
+
+	/* map all physical memory 1:1 */
+	setup_identity(page_root, 0, phys_end);
+
+	/* generate 128MB of invalid adresses at the end (for testing PGM) */
+	init_alloc_vpage((void *) -(1UL << 27));
+	setup_identity(page_root, -(1UL << 27), 0);
+
+	/* finally enable DAT with the new table */
+	mmu_enable(page_root);
+	return page_root;
+}
diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
index c0492b8..522c387 100644
--- a/lib/s390x/sclp.c
+++ b/lib/s390x/sclp.c
@@ -26,6 +26,7 @@ static uint64_t ram_size;
 static void mem_init(phys_addr_t freemem_start, phys_addr_t mem_end)
 {
 	phys_alloc_init(freemem_start, ram_size - freemem_start);
+	setup_vm();
 }
 
 void sclp_memory_setup(void)
diff --git a/s390x/Makefile b/s390x/Makefile
index 4198fdc..d9bef37 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -24,12 +24,14 @@ cflatobjs += lib/util.o
 cflatobjs += lib/alloc.o
 cflatobjs += lib/alloc_phys.o
 cflatobjs += lib/alloc_page.o
+cflatobjs += lib/vmalloc.o
 cflatobjs += lib/alloc_phys.o
 cflatobjs += lib/s390x/io.o
 cflatobjs += lib/s390x/stack.o
 cflatobjs += lib/s390x/sclp.o
 cflatobjs += lib/s390x/sclp-ascii.o
 cflatobjs += lib/s390x/interrupt.o
+cflatobjs += lib/s390x/mmu.o
 
 OBJDIRS += lib/s390x
 
diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index 4d0c877..1f837e9 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -93,7 +93,8 @@ pgm_int:
 initital_psw:
 	.quad	0x0000000180000000, init_psw_cont
 pgm_int_psw:
-	.quad	0x0000000180000000, pgm_int
+	/* once we test PGM interrupts, we already have DAT configured */
+	.quad	0x0400000180000000, pgm_int
 initital_cr0:
 	/* enable AFP-register control, so FP regs (+BFP instr) can be used */
 	.quad	0x0000000000040000
-- 
2.14.3

Re: [PATCH kvm-unit-tests 8/9] s390x: add vmalloc support

[Thomas Huth]

On 10.01.2018 22:53, David Hildenbrand wrote:
> To use virtual addresses, we have to
> - build page tables with identity mapping
> - setup the primary ASCE in cr1
> - enable DAT in the PSW
> 
> Not using the Linux definititons/implementation as they contain too much

s/definititons/definitions/

> software defined stuff / things we don't need.
> 
> Written from scratch. Tried to stick to the general Linux naming
> schemes.
> 
> As we currently don't invalidate anything except page table entries, it
> is suficient to only use ipte for now.

s/suficient/sufficient/

> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  lib/s390x/asm/arch_def.h |  45 ++++++++++
>  lib/s390x/asm/page.h     |  24 +++++
>  lib/s390x/asm/pgtable.h  | 222 +++++++++++++++++++++++++++++++++++++++++++++++
>  lib/s390x/mmu.c          |  90 +++++++++++++++++++
>  lib/s390x/sclp.c         |   1 +
>  s390x/Makefile           |   2 +
>  s390x/cstart64.S         |   3 +-
>  7 files changed, 386 insertions(+), 1 deletion(-)
>  create mode 100644 lib/s390x/asm/pgtable.h
>  create mode 100644 lib/s390x/mmu.c
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index ae27ab2..416dc75 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -163,4 +163,49 @@ static inline int tprot(unsigned long addr)
>  	return cc;
>  }
>  
> +static inline void lctlg(int cr, uint64_t value)
> +{
> +	asm volatile(
> +		"	lctlg	%1,%1,%0\n"
> +		: : "Q" (value), "i" (cr));

Doesn't the compiler complain here? I though that "i" is only possible
with constants? I guess the compiler is smart enough to replace it with
the right constants, since this is an inline function. But maybe better
play safe and turn this into a macro instead.

> +}
> +
> +static inline uint64_t stctg(int cr)
> +{
> +	uint64_t value;
> +
> +	asm volatile(
> +		"	stctg	%1,%1,%0\n"
> +		: "=Q" (value) : "i" (cr) : "memory");

dito

> +	return value;
> +}
> +
> +static inline void configure_dat(int enable)
> +{
> +	struct psw psw = {
> +		.mask = 0,
> +		.addr = 0,
> +	};
> +
> +	asm volatile(
> +		/* fetch the old PSW masks */
> +		"	epsw	%1,%2\n"
> +		/* remove the DAT flag first */
> +		"	nilh	%1,0xfbff\n"
> +		"	or	%3,%3\n"
> +		"	jz	disable\n"
> +		/* set DAT flag if enable == true */
> +		"	oilh	%1,0x0400\n"
> +		"	st	%1,0(%0)\n"
> +		"disable:\n"

Shouldn't the "disable" label be placed before the "st" ?

> +		"	st	%2,4(%0)\n"
> +		/* load the target address for our new PSW */
> +		"	larl	%1,cont\n"
> +		"	stg	%1,8(%0)\n"
> +		"	lpswe	0(%0)\n"
> +		"cont:\n"
> +		: : "a" (&psw), "r" (0), "r" (1), "r" (enable)

The usage of "r" (0), "r" (1) looks wrong. I think you should rather put
them into the output list instead, since they are modified within the
assembler code. Or maybe even better, use fixed registers in the
assembler code and mark the corresponding registers in the clobber list.

> +		: "memory", "cc");
> +}

Why is this an inline function in this header? It seems to only be used
in mmu.c, so I'd suggest to move it there (or even in a separate
assembler file).

>  #endif
> diff --git a/lib/s390x/asm/page.h b/lib/s390x/asm/page.h
> index 141a456..e7cc16d 100644
> --- a/lib/s390x/asm/page.h
> +++ b/lib/s390x/asm/page.h
[...]

I'll review this part later...

> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> index c0492b8..522c387 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -26,6 +26,7 @@ static uint64_t ram_size;
>  static void mem_init(phys_addr_t freemem_start, phys_addr_t mem_end)
>  {
>  	phys_alloc_init(freemem_start, ram_size - freemem_start);
> +	setup_vm();

Do we really want to enable VM uncoditionally for all tests? x86 also
calls setup_vm() only for some specific test. I think we should keep the
possibility to run some tests in real mode, too, shouldn't we?

>  }
>  

 Thomas

Re: [PATCH kvm-unit-tests 8/9] s390x: add vmalloc support

[David Hildenbrand]

>>  
>> +static inline void lctlg(int cr, uint64_t value)
>> +{
>> +	asm volatile(
>> +		"	lctlg	%1,%1,%0\n"
>> +		: : "Q" (value), "i" (cr));
> 
> Doesn't the compiler complain here? I though that "i" is only possible
> with constants? I guess the compiler is smart enough to replace it with
> the right constants, since this is an inline function. But maybe better
> play safe and turn this into a macro instead.

This works as it gets inlined. asm macros is frowned upon, no?

> 
>> +	return value;
>> +}
>> +
>> +static inline void configure_dat(int enable)
>> +{
>> +	struct psw psw = {
>> +		.mask = 0,
>> +		.addr = 0,
>> +	};
>> +
>> +	asm volatile(
>> +		/* fetch the old PSW masks */
>> +		"	epsw	%1,%2\n"
>> +		/* remove the DAT flag first */
>> +		"	nilh	%1,0xfbff\n"
>> +		"	or	%3,%3\n"
>> +		"	jz	disable\n"
>> +		/* set DAT flag if enable == true */
>> +		"	oilh	%1,0x0400\n"
>> +		"	st	%1,0(%0)\n"
>> +		"disable:\n"
> 
> Shouldn't the "disable" label be placed before the "st" ?

yes indeed
> 
>> +		"	st	%2,4(%0)\n"
>> +		/* load the target address for our new PSW */
>> +		"	larl	%1,cont\n"
>> +		"	stg	%1,8(%0)\n"
>> +		"	lpswe	0(%0)\n"
>> +		"cont:\n"
>> +		: : "a" (&psw), "r" (0), "r" (1), "r" (enable)
> 
> The usage of "r" (0), "r" (1) looks wrong. I think you should rather put
> them into the output list instead, since they are modified within the
> assembler code. Or maybe even better, use fixed registers in the
> assembler code and mark the corresponding registers in the clobber list.

I tried to use the output list way and it would not let me specify
immediates (0). So I have to introduce local variables. Or is there
another way?

> 
>> +		: "memory", "cc");
>> +}
> 
> Why is this an inline function in this header? It seems to only be used
> in mmu.c, so I'd suggest to move it there (or even in a separate
> assembler file).

I will move it to mmu.c but export the function, because ...

> 
>>  #endif
>> diff --git a/lib/s390x/asm/page.h b/lib/s390x/asm/page.h
>> index 141a456..e7cc16d 100644
>> --- a/lib/s390x/asm/page.h
>> +++ b/lib/s390x/asm/page.h
> [...]
> 
> I'll review this part later...
> 
>> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
>> index c0492b8..522c387 100644
>> --- a/lib/s390x/sclp.c
>> +++ b/lib/s390x/sclp.c
>> @@ -26,6 +26,7 @@ static uint64_t ram_size;
>>  static void mem_init(phys_addr_t freemem_start, phys_addr_t mem_end)
>>  {
>>  	phys_alloc_init(freemem_start, ram_size - freemem_start);
>> +	setup_vm();
> 
> Do we really want to enable VM uncoditionally for all tests? x86 also
> calls setup_vm() only for some specific test. I think we should keep the
> possibility to run some tests in real mode, too, shouldn't we?

A future test framework will most likely rely on malloc to work (that's
why we are implementing it). So I enable it as default. Each test can
simply disable dat for a certain time and reenable it then via
configure_dat().

(will also use this in sieve.c to get physical memory access)

> 
>>  }
>>  
> 
>  Thomas
> 

Thanks!

-- 

Thanks,

David / dhildenb

Re: [PATCH kvm-unit-tests 8/9] s390x: add vmalloc support

[Thomas Huth]

On 11.01.2018 13:07, David Hildenbrand wrote:
> 
>>>  
>>> +static inline void lctlg(int cr, uint64_t value)
>>> +{
>>> +	asm volatile(
>>> +		"	lctlg	%1,%1,%0\n"
>>> +		: : "Q" (value), "i" (cr));
>>
>> Doesn't the compiler complain here? I though that "i" is only possible
>> with constants? I guess the compiler is smart enough to replace it with
>> the right constants, since this is an inline function. But maybe better
>> play safe and turn this into a macro instead.
> 
> This works as it gets inlined. asm macros is frowned upon, no?

I'm just afraid that it is specific to the compiler version whether this
works with an inline function or not. But ok, let's keep it this way
first, and if we later hit a compiler where this does not work, we still
can replace it with a macro instead.

>>> +	asm volatile(
>>> +		/* fetch the old PSW masks */
>>> +		"	epsw	%1,%2\n"
>>> +		/* remove the DAT flag first */
>>> +		"	nilh	%1,0xfbff\n"
>>> +		"	or	%3,%3\n"
>>> +		"	jz	disable\n"
>>> +		/* set DAT flag if enable == true */
>>> +		"	oilh	%1,0x0400\n"
>>> +		"	st	%1,0(%0)\n"
>>> +		"disable:\n"
>>
>> Shouldn't the "disable" label be placed before the "st" ?
> 
> yes indeed
>>
>>> +		"	st	%2,4(%0)\n"
>>> +		/* load the target address for our new PSW */
>>> +		"	larl	%1,cont\n"
>>> +		"	stg	%1,8(%0)\n"
>>> +		"	lpswe	0(%0)\n"
>>> +		"cont:\n"
>>> +		: : "a" (&psw), "r" (0), "r" (1), "r" (enable)
>>
>> The usage of "r" (0), "r" (1) looks wrong. I think you should rather put
>> them into the output list instead, since they are modified within the
>> assembler code. Or maybe even better, use fixed registers in the
>> assembler code and mark the corresponding registers in the clobber list.
> 
> I tried to use the output list way and it would not let me specify
> immediates (0). So I have to introduce local variables. Or is there
> another way?

Use "+r"(0) in the output list? ... if that does not work, I think I'd
rather use the clobber list instead.

 Thomas

Re: [PATCH kvm-unit-tests 8/9] s390x: add vmalloc support

[Paolo Bonzini]

On 11/01/2018 13:24, Thomas Huth wrote:
>>>> +		"	st	%2,4(%0)\n"
>>>> +		/* load the target address for our new PSW */
>>>> +		"	larl	%1,cont\n"
>>>> +		"	stg	%1,8(%0)\n"
>>>> +		"	lpswe	0(%0)\n"
>>>> +		"cont:\n"
>>>> +		: : "a" (&psw), "r" (0), "r" (1), "r" (enable)
>>> The usage of "r" (0), "r" (1) looks wrong. I think you should rather put
>>> them into the output list instead, since they are modified within the
>>> assembler code. Or maybe even better, use fixed registers in the
>>> assembler code and mark the corresponding registers in the clobber list.
>> I tried to use the output list way and it would not let me specify
>> immediates (0). So I have to introduce local variables. Or is there
>> another way?
> Use "+r"(0) in the output list? ... if that does not work, I think I'd
> rather use the clobber list instead.

You'd have to introduce local variables, yes.  I think it's fine,
perhaps also more self-documenting than "0" and "1" to s390-impaired me.

Paolo

Re: [PATCH kvm-unit-tests 8/9] s390x: add vmalloc support

[Paolo Bonzini]

On 11/01/2018 13:07, David Hildenbrand wrote:
>>>  
>>> +static inline void lctlg(int cr, uint64_t value)
>>> +{
>>> +	asm volatile(
>>> +		"	lctlg	%1,%1,%0\n"
>>> +		: : "Q" (value), "i" (cr));
>> Doesn't the compiler complain here? I though that "i" is only possible
>> with constants? I guess the compiler is smart enough to replace it with
>> the right constants, since this is an inline function. But maybe better
>> play safe and turn this into a macro instead.
> This works as it gets inlined. asm macros is frowned upon, no?
> 

Then please use always_inline.

Paolo

Re: [PATCH kvm-unit-tests 8/9] s390x: add vmalloc support

[David Hildenbrand]

On 11.01.2018 15:02, Paolo Bonzini wrote:
> On 11/01/2018 13:07, David Hildenbrand wrote:
>>>>  
>>>> +static inline void lctlg(int cr, uint64_t value)
>>>> +{
>>>> +	asm volatile(
>>>> +		"	lctlg	%1,%1,%0\n"
>>>> +		: : "Q" (value), "i" (cr));
>>> Doesn't the compiler complain here? I though that "i" is only possible
>>> with constants? I guess the compiler is smart enough to replace it with
>>> the right constants, since this is an inline function. But maybe better
>>> play safe and turn this into a macro instead.
>> This works as it gets inlined. asm macros is frowned upon, no?
>>
> 
> Then please use always_inline.

FYI, we already do the same in lib/s390x/asm/cpacf.h "opcode" without
always_inline and that is copied from the kernel sources.

Thanks

> 
> Paolo
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH kvm-unit-tests 8/9] s390x: add vmalloc support

[Paolo Bonzini]

On 10/01/2018 22:53, David Hildenbrand wrote:
>  
> +typedef uint64_t pgdval_t;		/* Region-1 table entry */
> +typedef uint64_t p4dval_t;		/* Region-2 table entry*/
> +typedef uint64_t pudval_t;		/* Region-3 table Entry */
> +typedef uint64_t pmdval_t;		/* Segment table entry */
> +typedef uint64_t pteval_t;		/* Page table entry */
> +
> +typedef struct { pgdval_t pgd; } pgd_t;
> +typedef struct { p4dval_t p4d; } p4d_t;
> +typedef struct { pudval_t pud; } pud_t;
> +typedef struct { pmdval_t pmd; } pmd_t;
> +typedef struct { pteval_t pte; } pte_t;
> +
> +#define pgd_val(x)	((x).pgd)
> +#define p4d_val(x)	((x).p4d)
> +#define pud_val(x)	((x).pud)
> +#define pmd_val(x)	((x).pmd)
> +#define pte_val(x)	((x).pte)
> +
> +#define __pgd(x)	((pgd_t) { (x) } )
> +#define __p4d(x)	((p4d_t) { (x) } )
> +#define __pud(x)	((pud_t) { (x) } )
> +#define __pmd(x)	((pmd_t) { (x) } )
> +#define __pte(x)	((pte_t) { (x) } )
> +

You don't need to do this.  Using recursive functions as in x86 is
perfectly okay (just use pte_t as an opaque struct for type safety) if
you prefer.

Paolo

Re: [PATCH kvm-unit-tests 8/9] s390x: add vmalloc support

[David Hildenbrand]

On 11.01.2018 15:01, Paolo Bonzini wrote:
> On 10/01/2018 22:53, David Hildenbrand wrote:
>>  
>> +typedef uint64_t pgdval_t;		/* Region-1 table entry */
>> +typedef uint64_t p4dval_t;		/* Region-2 table entry*/
>> +typedef uint64_t pudval_t;		/* Region-3 table Entry */
>> +typedef uint64_t pmdval_t;		/* Segment table entry */
>> +typedef uint64_t pteval_t;		/* Page table entry */
>> +
>> +typedef struct { pgdval_t pgd; } pgd_t;
>> +typedef struct { p4dval_t p4d; } p4d_t;
>> +typedef struct { pudval_t pud; } pud_t;
>> +typedef struct { pmdval_t pmd; } pmd_t;
>> +typedef struct { pteval_t pte; } pte_t;
>> +
>> +#define pgd_val(x)	((x).pgd)
>> +#define p4d_val(x)	((x).p4d)
>> +#define pud_val(x)	((x).pud)
>> +#define pmd_val(x)	((x).pmd)
>> +#define pte_val(x)	((x).pte)
>> +
>> +#define __pgd(x)	((pgd_t) { (x) } )
>> +#define __p4d(x)	((p4d_t) { (x) } )
>> +#define __pud(x)	((pud_t) { (x) } )
>> +#define __pmd(x)	((pmd_t) { (x) } )
>> +#define __pte(x)	((pte_t) { (x) } )
>> +
> 
> You don't need to do this.  Using recursive functions as in x86 is
> perfectly okay (just use pte_t as an opaque struct for type safety) if
> you prefer.

I sticked to what arm does in their page.h

I assume you reference from Linux
	arch/x86/include/asm/pgtable_64_types.h

Which only have
	typedef struct { pteval_t pte; } pte_t;

I'll look into it, thanks!

> 
> Paolo
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH kvm-unit-tests 8/9] s390x: add vmalloc support

[David Hildenbrand]

On 11.01.2018 15:01, Paolo Bonzini wrote:
> On 10/01/2018 22:53, David Hildenbrand wrote:
>>  
>> +typedef uint64_t pgdval_t;		/* Region-1 table entry */
>> +typedef uint64_t p4dval_t;		/* Region-2 table entry*/
>> +typedef uint64_t pudval_t;		/* Region-3 table Entry */
>> +typedef uint64_t pmdval_t;		/* Segment table entry */
>> +typedef uint64_t pteval_t;		/* Page table entry */
>> +
>> +typedef struct { pgdval_t pgd; } pgd_t;
>> +typedef struct { p4dval_t p4d; } p4d_t;
>> +typedef struct { pudval_t pud; } pud_t;
>> +typedef struct { pmdval_t pmd; } pmd_t;
>> +typedef struct { pteval_t pte; } pte_t;
>> +
>> +#define pgd_val(x)	((x).pgd)
>> +#define p4d_val(x)	((x).p4d)
>> +#define pud_val(x)	((x).pud)
>> +#define pmd_val(x)	((x).pmd)
>> +#define pte_val(x)	((x).pte)
>> +
>> +#define __pgd(x)	((pgd_t) { (x) } )
>> +#define __p4d(x)	((p4d_t) { (x) } )
>> +#define __pud(x)	((pud_t) { (x) } )
>> +#define __pmd(x)	((pmd_t) { (x) } )
>> +#define __pte(x)	((pte_t) { (x) } )
>> +
> 
> You don't need to do this.  Using recursive functions as in x86 is
> perfectly okay (just use pte_t as an opaque struct for type safety) if
> you prefer.

Okay, looking at kvm-unit-tests, I think you were talking about struct
pte_search and find_pte_level().

I'd prefer to keep it as is for now, closer to what we do in the kernel
(and might come in handy once we want to construct broken page tables to
test the virtual address translation in QEMU/KVM).

Thanks!

> 
> Paolo
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH kvm-unit-tests 8/9] s390x: add vmalloc support

[Paolo Bonzini]

On 10/01/2018 22:53, David Hildenbrand wrote:
> @@ -26,6 +26,7 @@ static uint64_t ram_size;
>  static void mem_init(phys_addr_t freemem_start, phys_addr_t mem_end)
>  {
>  	phys_alloc_init(freemem_start, ram_size - freemem_start);
> +	setup_vm();
>  }
>  
>  void sclp_memory_setup(void)

I'd leave setup_vm() to tests if not strictly necessary.

Paolo

Re: [PATCH kvm-unit-tests 8/9] s390x: add vmalloc support

[David Hildenbrand]

On 12.01.2018 11:10, Paolo Bonzini wrote:
> On 10/01/2018 22:53, David Hildenbrand wrote:
>> @@ -26,6 +26,7 @@ static uint64_t ram_size;
>>  static void mem_init(phys_addr_t freemem_start, phys_addr_t mem_end)
>>  {
>>  	phys_alloc_init(freemem_start, ram_size - freemem_start);
>> +	setup_vm();
>>  }
>>  
>>  void sclp_memory_setup(void)
> 
> I'd leave setup_vm() to tests if not strictly necessary.
> 
> Paolo
> 

Makes handling of program exceptions unnecessary complicated. (which
functions we're allowed to call etc.) And DAT can be easily disabled in
tests that require it instead.

So as long as there is no good reason for it, I prefer to keep it simple
and always set it up.

-- 

Thanks,

David / dhildenb

Re: [PATCH kvm-unit-tests 8/9] s390x: add vmalloc support

[Andrew Jones]

On Fri, Jan 12, 2018 at 11:33:55AM +0100, David Hildenbrand wrote:
> On 12.01.2018 11:10, Paolo Bonzini wrote:
> > On 10/01/2018 22:53, David Hildenbrand wrote:
> >> @@ -26,6 +26,7 @@ static uint64_t ram_size;
> >>  static void mem_init(phys_addr_t freemem_start, phys_addr_t mem_end)
> >>  {
> >>  	phys_alloc_init(freemem_start, ram_size - freemem_start);
> >> +	setup_vm();
> >>  }
> >>  
> >>  void sclp_memory_setup(void)
> > 
> > I'd leave setup_vm() to tests if not strictly necessary.
> > 
> > Paolo
> > 
> 
> Makes handling of program exceptions unnecessary complicated. (which
> functions we're allowed to call etc.) And DAT can be easily disabled in
> tests that require it instead.
> 
> So as long as there is no good reason for it, I prefer to keep it simple
> and always set it up.
>

I've been considering some changes for ARM that allow setup_vm() to be
conditionally skipped for unit tests that require that. I kicked around
several ideas as to what the condition should use. Currently I have an
auxinfo flag (added to a newly introduced flags field) which is set at
build time with a Makefile rule prototyped for it.

Long story, short; I think the way David is doing it now, which is like
ARM, is a fine start, and then if necessary we can extend the framework
to allow this auxinfo flag, or whatever, to give some flexibility to
unit tests that need it.

Thanks,
drew

Re: [PATCH kvm-unit-tests 8/9] s390x: add vmalloc support

[Paolo Bonzini]

On 12/01/2018 13:17, Andrew Jones wrote:
> On Fri, Jan 12, 2018 at 11:33:55AM +0100, David Hildenbrand wrote:
>> On 12.01.2018 11:10, Paolo Bonzini wrote:
>>> On 10/01/2018 22:53, David Hildenbrand wrote:
>>>> @@ -26,6 +26,7 @@ static uint64_t ram_size;
>>>>  static void mem_init(phys_addr_t freemem_start, phys_addr_t mem_end)
>>>>  {
>>>>  	phys_alloc_init(freemem_start, ram_size - freemem_start);
>>>> +	setup_vm();
>>>>  }
>>>>  
>>>>  void sclp_memory_setup(void)
>>>
>>> I'd leave setup_vm() to tests if not strictly necessary.
>>>
>>> Paolo
>>>
>>
>> Makes handling of program exceptions unnecessary complicated. (which
>> functions we're allowed to call etc.) And DAT can be easily disabled in
>> tests that require it instead.
>>
>> So as long as there is no good reason for it, I prefer to keep it simple
>> and always set it up.
>>
> 
> I've been considering some changes for ARM that allow setup_vm() to be
> conditionally skipped for unit tests that require that. I kicked around
> several ideas as to what the condition should use. Currently I have an
> auxinfo flag (added to a newly introduced flags field) which is set at
> build time with a Makefile rule prototyped for it.
> 
> Long story, short; I think the way David is doing it now, which is like
> ARM, is a fine start, and then if necessary we can extend the framework
> to allow this auxinfo flag, or whatever, to give some flexibility to
> unit tests that need it.

Yeah, that's okay.

Most x86 tests probably can use setup_vm unconditionally too.

Paolo

[PATCH kvm-unit-tests 9/9] s390x: add test for (v)malloc

[David Hildenbrand]

Let's test if basic allocation works and we get virtual addresses.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 s390x/selftest.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/s390x/selftest.c b/s390x/selftest.c
index 76ed4bf..bf72d32 100644
--- a/s390x/selftest.c
+++ b/s390x/selftest.c
@@ -10,6 +10,7 @@
  */
 #include <libcflat.h>
 #include <util.h>
+#include <alloc.h>
 #include <asm/interrupt.h>
 
 static void test_fp(void)
@@ -37,6 +38,21 @@ static void test_pgm_int(void)
 	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
 }
 
+static void test_malloc(void)
+{
+	int *tmp = malloc(sizeof(int));
+	int *tmp2 = malloc(sizeof(int));
+
+	report("malloc: got vaddr", (uintptr_t)tmp & 0xffffffff00000000ul);
+	report("malloc: access works", (*tmp = 123456789));
+	report("malloc: got 2nd vaddr", (uintptr_t)tmp2 & 0xffffffff00000000ul);
+	report("malloc: access works", (*tmp2 = 123456789));
+	report("malloc: addresses differ", tmp != tmp2);
+
+	free(tmp);
+	free(tmp2);
+}
+
 int main(int argc, char**argv)
 {
 	report_prefix_push("selftest");
@@ -49,6 +65,7 @@ int main(int argc, char**argv)
 
 	test_fp();
 	test_pgm_int();
+	test_malloc();
 
 	return report_summary();
 }
-- 
2.14.3

Re: [PATCH kvm-unit-tests 9/9] s390x: add test for (v)malloc

[Thomas Huth]

On 10.01.2018 22:53, David Hildenbrand wrote:
> Let's test if basic allocation works and we get virtual addresses.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  s390x/selftest.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/s390x/selftest.c b/s390x/selftest.c
> index 76ed4bf..bf72d32 100644
> --- a/s390x/selftest.c
> +++ b/s390x/selftest.c
> @@ -10,6 +10,7 @@
>   */
>  #include <libcflat.h>
>  #include <util.h>
> +#include <alloc.h>
>  #include <asm/interrupt.h>
>  
>  static void test_fp(void)
> @@ -37,6 +38,21 @@ static void test_pgm_int(void)
>  	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
>  }
>  
> +static void test_malloc(void)
> +{
> +	int *tmp = malloc(sizeof(int));
> +	int *tmp2 = malloc(sizeof(int));
> +
> +	report("malloc: got vaddr", (uintptr_t)tmp & 0xffffffff00000000ul);
> +	report("malloc: access works", (*tmp = 123456789));

Using the *tmp = 123456789 as condition for report() here looks somewhat
weird. What about:

	*tmp = 123456789;
	mb();
	report("malloc: access works", (*tmp == 123456789));

?

> +	report("malloc: got 2nd vaddr", (uintptr_t)tmp2 & 0xffffffff00000000ul);
> +	report("malloc: access works", (*tmp2 = 123456789));
> +	report("malloc: addresses differ", tmp != tmp2);

Would it make sense to use the LOAD REAL ADDRESS instruction here to
check that the VA != PA ?

 Thomas

Re: [PATCH kvm-unit-tests 9/9] s390x: add test for (v)malloc

[David Hildenbrand]

On 11.01.2018 13:09, Thomas Huth wrote:
> On 10.01.2018 22:53, David Hildenbrand wrote:
>> Let's test if basic allocation works and we get virtual addresses.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  s390x/selftest.c | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/s390x/selftest.c b/s390x/selftest.c
>> index 76ed4bf..bf72d32 100644
>> --- a/s390x/selftest.c
>> +++ b/s390x/selftest.c
>> @@ -10,6 +10,7 @@
>>   */
>>  #include <libcflat.h>
>>  #include <util.h>
>> +#include <alloc.h>
>>  #include <asm/interrupt.h>
>>  
>>  static void test_fp(void)
>> @@ -37,6 +38,21 @@ static void test_pgm_int(void)
>>  	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
>>  }
>>  
>> +static void test_malloc(void)
>> +{
>> +	int *tmp = malloc(sizeof(int));
>> +	int *tmp2 = malloc(sizeof(int));
>> +
>> +	report("malloc: got vaddr", (uintptr_t)tmp & 0xffffffff00000000ul);
>> +	report("malloc: access works", (*tmp = 123456789));
> 
> Using the *tmp = 123456789 as condition for report() here looks somewhat
> weird. What about:

Can do that.

> 
> 	*tmp = 123456789;
> 	mb();
> 	report("malloc: access works", (*tmp == 123456789));
> 
> ?
> 
>> +	report("malloc: got 2nd vaddr", (uintptr_t)tmp2 & 0xffffffff00000000ul);
>> +	report("malloc: access works", (*tmp2 = 123456789));
>> +	report("malloc: addresses differ", tmp != tmp2);
> 
> Would it make sense to use the LOAD REAL ADDRESS instruction here to
> check that the VA != PA ?

As an alternative, I can disable DAT an make sure that I no longer can
access the value (PGM ADRESSING).

Thanks!

> 
>  Thomas
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH kvm-unit-tests 0/9] s390x: vmalloc support

[Paolo Bonzini]

On 10/01/2018 22:53, David Hildenbrand wrote:
> This series implements
> - detection of installed physical memory
> - setup of the physical allocator
> - setup of the MMU / page tables / DAT
> - setup of the virtual allocator
> 
> The CPU now runs with DAT enabled. I added a small test to make sure
> malloc() indeed works and uses virtual adresses.

Thanks!  Does sieve.flat work too? :)

Paolo

> 
> While at it, fix the TEST BLOCK test on newer compilers.
> 
> Tested with upsteam QEMU TCG and KVM.
> 
> David Hildenbrand (9):
>   s390x: fix TEST BLOCK tests
>   s390x: use highest addresses for PGM_ADDRESSING errors
>   s390x: increase the stack size
>   s390x: add missing sclp definitions from QEMU
>   s390x: rename sclp_setup() to sclp_ascii_setup()
>   s390x: detect installed memory
>   s390x: initialize the physical allocator
>   s390x: add vmalloc support
>   s390x: add test for (v)malloc
> 
>  lib/s390x/asm/arch_def.h |  57 ++++++++++++
>  lib/s390x/asm/page.h     |  24 +++++
>  lib/s390x/asm/pgtable.h  | 222 +++++++++++++++++++++++++++++++++++++++++++++++
>  lib/s390x/io.c           |   3 +-
>  lib/s390x/mmu.c          |  90 +++++++++++++++++++
>  lib/s390x/sclp-ascii.c   |   4 +-
>  lib/s390x/sclp.c         |  66 ++++++++++++++
>  lib/s390x/sclp.h         | 111 +++++++++++++++++++++++-
>  s390x/Makefile           |   6 ++
>  s390x/cstart64.S         |   3 +-
>  s390x/flat.lds           |   2 +-
>  s390x/intercept.c        |  14 +--
>  s390x/selftest.c         |  19 +++-
>  13 files changed, 606 insertions(+), 15 deletions(-)
>  create mode 100644 lib/s390x/asm/pgtable.h
>  create mode 100644 lib/s390x/mmu.c
>  create mode 100644 lib/s390x/sclp.c
> 

Re: [PATCH kvm-unit-tests 0/9] s390x: vmalloc support

[David Hildenbrand]

On 11.01.2018 11:43, Paolo Bonzini wrote:
> On 10/01/2018 22:53, David Hildenbrand wrote:
>> This series implements
>> - detection of installed physical memory
>> - setup of the physical allocator
>> - setup of the MMU / page tables / DAT
>> - setup of the virtual allocator
>>
>> The CPU now runs with DAT enabled. I added a small test to make sure
>> malloc() indeed works and uses virtual adresses.
> 
> Thanks!  Does sieve.flat work too? :)

Works with minor modifications (instead of setup_vm(), which is already
done, I temporarily disable DAT).

Found a QEMU TCG bug, trying to disable DAT :)


-- 

Thanks,

David / dhildenb