* [kvm-unit-tests PATCH 1/9] s390x: uv: Tolerate 0x100 query return code
2021-09-22 7:18 [kvm-unit-tests PATCH 0/9] s390x: Cleanup and maintenance 2 Janosch Frank
@ 2021-09-22 7:18 ` Janosch Frank
2021-09-22 9:12 ` Claudio Imbrenda
2021-09-22 7:18 ` [kvm-unit-tests PATCH 2/9] s390x: pfmf: Fix 1MB handling Janosch Frank
` (7 subsequent siblings)
8 siblings, 1 reply; 34+ messages in thread
From: Janosch Frank @ 2021-09-22 7:18 UTC (permalink / raw)
To: kvm; +Cc: thuth, david, linux-s390, seiden, imbrenda
RC 0x100 is not an error but a notice that we could have gotten more
data from the Ultravisor if we had asked for it. So let's tolerate
them in our tests.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
s390x/uv-guest.c | 4 ++--
s390x/uv-host.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
index f05ae4c3..e7446e03 100644
--- a/s390x/uv-guest.c
+++ b/s390x/uv-guest.c
@@ -70,8 +70,8 @@ static void test_query(void)
report(cc == 1 && uvcb.header.rc == UVC_RC_INV_LEN, "length");
uvcb.header.len = sizeof(uvcb);
- cc = uv_call(0, (u64)&uvcb);
- report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED, "successful query");
+ uv_call(0, (u64)&uvcb);
+ report(uvcb.header.rc == UVC_RC_EXECUTED || uvcb.header.rc == 0x100, "successful query");
/*
* These bits have been introduced with the very first
diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 28035707..66a11160 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -401,7 +401,7 @@ static void test_query(void)
uvcb_qui.header.len = sizeof(uvcb_qui);
uv_call(0, (uint64_t)&uvcb_qui);
- report(uvcb_qui.header.rc == UVC_RC_EXECUTED, "successful query");
+ report(uvcb_qui.header.rc == UVC_RC_EXECUTED || uvcb_qui.header.rc == 0x100, "successful query");
for (i = 0; cmds[i].name; i++)
report(uv_query_test_call(cmds[i].call_bit), "%s", cmds[i].name);
--
2.30.2
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [kvm-unit-tests PATCH 1/9] s390x: uv: Tolerate 0x100 query return code
2021-09-22 7:18 ` [kvm-unit-tests PATCH 1/9] s390x: uv: Tolerate 0x100 query return code Janosch Frank
@ 2021-09-22 9:12 ` Claudio Imbrenda
2021-09-22 11:36 ` Janosch Frank
0 siblings, 1 reply; 34+ messages in thread
From: Claudio Imbrenda @ 2021-09-22 9:12 UTC (permalink / raw)
To: Janosch Frank; +Cc: kvm, thuth, david, linux-s390, seiden
On Wed, 22 Sep 2021 07:18:03 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:
> RC 0x100 is not an error but a notice that we could have gotten more
> data from the Ultravisor if we had asked for it. So let's tolerate
> them in our tests.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> s390x/uv-guest.c | 4 ++--
> s390x/uv-host.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
> index f05ae4c3..e7446e03 100644
> --- a/s390x/uv-guest.c
> +++ b/s390x/uv-guest.c
> @@ -70,8 +70,8 @@ static void test_query(void)
> report(cc == 1 && uvcb.header.rc == UVC_RC_INV_LEN, "length");
>
> uvcb.header.len = sizeof(uvcb);
> - cc = uv_call(0, (u64)&uvcb);
> - report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED, "successful query");
> + uv_call(0, (u64)&uvcb);
> + report(uvcb.header.rc == UVC_RC_EXECUTED || uvcb.header.rc
> == 0x100, "successful query");
if you want to be even more pedantic:
report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED ||
cc == 1 && uvcb.header.rc == 0x100, ...
>
> /*
> * These bits have been introduced with the very first
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index 28035707..66a11160 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -401,7 +401,7 @@ static void test_query(void)
>
> uvcb_qui.header.len = sizeof(uvcb_qui);
> uv_call(0, (uint64_t)&uvcb_qui);
> - report(uvcb_qui.header.rc == UVC_RC_EXECUTED, "successful query");
> + report(uvcb_qui.header.rc == UVC_RC_EXECUTED || uvcb_qui.header.rc == 0x100, "successful query");
same here
>
> for (i = 0; cmds[i].name; i++)
> report(uv_query_test_call(cmds[i].call_bit), "%s", cmds[i].name);
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH 1/9] s390x: uv: Tolerate 0x100 query return code
2021-09-22 9:12 ` Claudio Imbrenda
@ 2021-09-22 11:36 ` Janosch Frank
2021-09-27 15:19 ` Thomas Huth
0 siblings, 1 reply; 34+ messages in thread
From: Janosch Frank @ 2021-09-22 11:36 UTC (permalink / raw)
To: Claudio Imbrenda; +Cc: kvm, thuth, david, linux-s390, seiden
On 9/22/21 11:12 AM, Claudio Imbrenda wrote:
> On Wed, 22 Sep 2021 07:18:03 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
>
>> RC 0x100 is not an error but a notice that we could have gotten more
>> data from the Ultravisor if we had asked for it. So let's tolerate
>> them in our tests.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> s390x/uv-guest.c | 4 ++--
>> s390x/uv-host.c | 2 +-
>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
>> index f05ae4c3..e7446e03 100644
>> --- a/s390x/uv-guest.c
>> +++ b/s390x/uv-guest.c
>> @@ -70,8 +70,8 @@ static void test_query(void)
>> report(cc == 1 && uvcb.header.rc == UVC_RC_INV_LEN, "length");
>>
>> uvcb.header.len = sizeof(uvcb);
>> - cc = uv_call(0, (u64)&uvcb);
>> - report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED, "successful query");
>> + uv_call(0, (u64)&uvcb);
>> + report(uvcb.header.rc == UVC_RC_EXECUTED || uvcb.header.rc
>> == 0x100, "successful query");
>
> if you want to be even more pedantic:
> report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED ||
> cc == 1 && uvcb.header.rc == 0x100, ...
Yeah I pondered about that but at the end I chose to drop the cc check
>
>>
>> /*
>> * These bits have been introduced with the very first
>> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
>> index 28035707..66a11160 100644
>> --- a/s390x/uv-host.c
>> +++ b/s390x/uv-host.c
>> @@ -401,7 +401,7 @@ static void test_query(void)
>>
>> uvcb_qui.header.len = sizeof(uvcb_qui);
>> uv_call(0, (uint64_t)&uvcb_qui);
>> - report(uvcb_qui.header.rc == UVC_RC_EXECUTED, "successful query");
>> + report(uvcb_qui.header.rc == UVC_RC_EXECUTED || uvcb_qui.header.rc == 0x100, "successful query");
>
> same here
>
>>
>> for (i = 0; cmds[i].name; i++)
>> report(uv_query_test_call(cmds[i].call_bit), "%s", cmds[i].name);
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH 1/9] s390x: uv: Tolerate 0x100 query return code
2021-09-22 11:36 ` Janosch Frank
@ 2021-09-27 15:19 ` Thomas Huth
0 siblings, 0 replies; 34+ messages in thread
From: Thomas Huth @ 2021-09-27 15:19 UTC (permalink / raw)
To: Janosch Frank, Claudio Imbrenda; +Cc: kvm, david, linux-s390, seiden
On 22/09/2021 13.36, Janosch Frank wrote:
> On 9/22/21 11:12 AM, Claudio Imbrenda wrote:
>> On Wed, 22 Sep 2021 07:18:03 +0000
>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>
>>> RC 0x100 is not an error but a notice that we could have gotten more
>>> data from the Ultravisor if we had asked for it. So let's tolerate
>>> them in our tests.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>> s390x/uv-guest.c | 4 ++--
>>> s390x/uv-host.c | 2 +-
>>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
>>> index f05ae4c3..e7446e03 100644
>>> --- a/s390x/uv-guest.c
>>> +++ b/s390x/uv-guest.c
>>> @@ -70,8 +70,8 @@ static void test_query(void)
>>> report(cc == 1 && uvcb.header.rc == UVC_RC_INV_LEN, "length");
>>>
>>> uvcb.header.len = sizeof(uvcb);
>>> - cc = uv_call(0, (u64)&uvcb);
>>> - report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED, "successful query");
>>> + uv_call(0, (u64)&uvcb);
>>> + report(uvcb.header.rc == UVC_RC_EXECUTED || uvcb.header.rc
>>> == 0x100, "successful query");
>>
>> if you want to be even more pedantic:
>> report(cc == 0 && uvcb.header.rc == UVC_RC_EXECUTED ||
>> cc == 1 && uvcb.header.rc == 0x100, ...
>
> Yeah I pondered about that but at the end I chose to drop the cc check
Well, but we're in the kvm-unit-tests here where we check for the exact
behavior of the system ... so I'd vote for keeping/extening the cc checking,
even if it's more code to read in the end.
Thomas
^ permalink raw reply [flat|nested] 34+ messages in thread
* [kvm-unit-tests PATCH 2/9] s390x: pfmf: Fix 1MB handling
2021-09-22 7:18 [kvm-unit-tests PATCH 0/9] s390x: Cleanup and maintenance 2 Janosch Frank
2021-09-22 7:18 ` [kvm-unit-tests PATCH 1/9] s390x: uv: Tolerate 0x100 query return code Janosch Frank
@ 2021-09-22 7:18 ` Janosch Frank
2021-09-22 9:16 ` Claudio Imbrenda
2021-09-27 15:23 ` Thomas Huth
2021-09-22 7:18 ` [kvm-unit-tests PATCH 3/9] s390x: uv-host: Fence a destroy cpu test on z15 Janosch Frank
` (6 subsequent siblings)
8 siblings, 2 replies; 34+ messages in thread
From: Janosch Frank @ 2021-09-22 7:18 UTC (permalink / raw)
To: kvm; +Cc: thuth, david, linux-s390, seiden, imbrenda
On everything larger than 4k pfmf will update the address in GR2 when
it's interrupted so we should loop on pfmf and not trust that it
doesn't get interrupted.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
s390x/pfmf.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/s390x/pfmf.c b/s390x/pfmf.c
index 2f3cb110..b0095bd7 100644
--- a/s390x/pfmf.c
+++ b/s390x/pfmf.c
@@ -54,6 +54,7 @@ static void test_1m_key(void)
bool rp = true;
union pfmf_r1 r1;
union skey skey;
+ void *addr = pagebuf;
report_prefix_push("1M");
if (test_facility(169)) {
@@ -64,7 +65,9 @@ static void test_1m_key(void)
r1.reg.sk = 1;
r1.reg.fsc = PFMF_FSC_1M;
r1.reg.key = 0x30;
- pfmf(r1.val, pagebuf);
+ do {
+ addr = pfmf(r1.val, addr);
+ } while ((uintptr_t)addr != (uintptr_t)pagebuf + HPAGE_SIZE);
for (i = 0; i < 256; i++) {
skey.val = get_storage_key(pagebuf + i * PAGE_SIZE);
skey.val &= SKEY_ACC | SKEY_FP;
@@ -99,6 +102,7 @@ static void test_1m_clear(void)
int i;
union pfmf_r1 r1;
unsigned long sum = 0;
+ void *addr = pagebuf;
r1.val = 0;
r1.reg.cf = 1;
@@ -106,7 +110,9 @@ static void test_1m_clear(void)
report_prefix_push("1M");
memset(pagebuf, 42, PAGE_SIZE * 256);
- pfmf(r1.val, pagebuf);
+ do {
+ addr = pfmf(r1.val, addr);
+ } while ((uintptr_t)addr != (uintptr_t)pagebuf + HPAGE_SIZE);
for (i = 0; i < PAGE_SIZE * 256; i++)
sum |= pagebuf[i];
report(!sum, "clear memory");
--
2.30.2
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [kvm-unit-tests PATCH 2/9] s390x: pfmf: Fix 1MB handling
2021-09-22 7:18 ` [kvm-unit-tests PATCH 2/9] s390x: pfmf: Fix 1MB handling Janosch Frank
@ 2021-09-22 9:16 ` Claudio Imbrenda
2021-09-27 15:23 ` Thomas Huth
1 sibling, 0 replies; 34+ messages in thread
From: Claudio Imbrenda @ 2021-09-22 9:16 UTC (permalink / raw)
To: Janosch Frank; +Cc: kvm, thuth, david, linux-s390, seiden
On Wed, 22 Sep 2021 07:18:04 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:
> On everything larger than 4k pfmf will update the address in GR2 when
> it's interrupted so we should loop on pfmf and not trust that it
> doesn't get interrupted.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
> s390x/pfmf.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/s390x/pfmf.c b/s390x/pfmf.c
> index 2f3cb110..b0095bd7 100644
> --- a/s390x/pfmf.c
> +++ b/s390x/pfmf.c
> @@ -54,6 +54,7 @@ static void test_1m_key(void)
> bool rp = true;
> union pfmf_r1 r1;
> union skey skey;
> + void *addr = pagebuf;
>
> report_prefix_push("1M");
> if (test_facility(169)) {
> @@ -64,7 +65,9 @@ static void test_1m_key(void)
> r1.reg.sk = 1;
> r1.reg.fsc = PFMF_FSC_1M;
> r1.reg.key = 0x30;
> - pfmf(r1.val, pagebuf);
> + do {
> + addr = pfmf(r1.val, addr);
> + } while ((uintptr_t)addr != (uintptr_t)pagebuf + HPAGE_SIZE);
> for (i = 0; i < 256; i++) {
> skey.val = get_storage_key(pagebuf + i * PAGE_SIZE);
> skey.val &= SKEY_ACC | SKEY_FP;
> @@ -99,6 +102,7 @@ static void test_1m_clear(void)
> int i;
> union pfmf_r1 r1;
> unsigned long sum = 0;
> + void *addr = pagebuf;
>
> r1.val = 0;
> r1.reg.cf = 1;
> @@ -106,7 +110,9 @@ static void test_1m_clear(void)
>
> report_prefix_push("1M");
> memset(pagebuf, 42, PAGE_SIZE * 256);
> - pfmf(r1.val, pagebuf);
> + do {
> + addr = pfmf(r1.val, addr);
> + } while ((uintptr_t)addr != (uintptr_t)pagebuf + HPAGE_SIZE);
> for (i = 0; i < PAGE_SIZE * 256; i++)
> sum |= pagebuf[i];
> report(!sum, "clear memory");
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [kvm-unit-tests PATCH 2/9] s390x: pfmf: Fix 1MB handling
2021-09-22 7:18 ` [kvm-unit-tests PATCH 2/9] s390x: pfmf: Fix 1MB handling Janosch Frank
2021-09-22 9:16 ` Claudio Imbrenda
@ 2021-09-27 15:23 ` Thomas Huth
2021-09-28 9:50 ` Janosch Frank
1 sibling, 1 reply; 34+ messages in thread
From: Thomas Huth @ 2021-09-27 15:23 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: david, linux-s390, seiden, imbrenda
On 22/09/2021 09.18, Janosch Frank wrote:
> On everything larger than 4k pfmf will update the address in GR2 when
> it's interrupted so we should loop on pfmf and not trust that it
> doesn't get interrupted.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> s390x/pfmf.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
Acked-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH 2/9] s390x: pfmf: Fix 1MB handling
2021-09-27 15:23 ` Thomas Huth
@ 2021-09-28 9:50 ` Janosch Frank
0 siblings, 0 replies; 34+ messages in thread
From: Janosch Frank @ 2021-09-28 9:50 UTC (permalink / raw)
To: Thomas Huth, kvm; +Cc: david, linux-s390, seiden, imbrenda
On 9/27/21 17:23, Thomas Huth wrote:
> On 22/09/2021 09.18, Janosch Frank wrote:
>> On everything larger than 4k pfmf will update the address in GR2 when
>> it's interrupted so we should loop on pfmf and not trust that it
>> doesn't get interrupted.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> s390x/pfmf.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> Acked-by: Thomas Huth <thuth@redhat.com>
>
I had another discussion with the people that told me to change this and
after some starring at the documentation we found out this won't fix
anything because it's not a problem. FW will update GR2 and back off the
PSW when it has been interrupted so it effectively loops itself.
So I'm dropping this patch
^ permalink raw reply [flat|nested] 34+ messages in thread
* [kvm-unit-tests PATCH 3/9] s390x: uv-host: Fence a destroy cpu test on z15
2021-09-22 7:18 [kvm-unit-tests PATCH 0/9] s390x: Cleanup and maintenance 2 Janosch Frank
2021-09-22 7:18 ` [kvm-unit-tests PATCH 1/9] s390x: uv: Tolerate 0x100 query return code Janosch Frank
2021-09-22 7:18 ` [kvm-unit-tests PATCH 2/9] s390x: pfmf: Fix 1MB handling Janosch Frank
@ 2021-09-22 7:18 ` Janosch Frank
2021-09-22 9:18 ` Claudio Imbrenda
2021-09-27 15:26 ` Thomas Huth
2021-09-22 7:18 ` [kvm-unit-tests PATCH 4/9] lib: s390x: uv: Fix share return value and print Janosch Frank
` (5 subsequent siblings)
8 siblings, 2 replies; 34+ messages in thread
From: Janosch Frank @ 2021-09-22 7:18 UTC (permalink / raw)
To: kvm; +Cc: thuth, david, linux-s390, seiden, imbrenda
Firmware will not give us the expected return code on z15 so let's
fence it for the z15 machine generation.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
lib/s390x/asm/arch_def.h | 14 ++++++++++++++
s390x/uv-host.c | 11 +++++++----
2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index aa80d840..c8d2722a 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -219,6 +219,20 @@ static inline unsigned short stap(void)
return cpu_address;
}
+#define MACHINE_Z15A 0x8561
+#define MACHINE_Z15B 0x8562
+
+static inline uint16_t get_machine_id(void)
+{
+ uint64_t cpuid;
+
+ asm volatile("stidp %0" : "=Q" (cpuid));
+ cpuid = cpuid >> 16;
+ cpuid &= 0xffff;
+
+ return cpuid;
+}
+
static inline int tprot(unsigned long addr)
{
int cc;
diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 66a11160..5e351120 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -111,6 +111,7 @@ static void test_config_destroy(void)
static void test_cpu_destroy(void)
{
int rc;
+ uint16_t machineid = get_machine_id();
struct uv_cb_nodata uvcb = {
.header.len = sizeof(uvcb),
.header.cmd = UVC_CMD_DESTROY_SEC_CPU,
@@ -125,10 +126,12 @@ static void test_cpu_destroy(void)
"hdr invalid length");
uvcb.header.len += 8;
- uvcb.handle += 1;
- rc = uv_call(0, (uint64_t)&uvcb);
- report(rc == 1 && uvcb.header.rc == UVC_RC_INV_CHANDLE, "invalid handle");
- uvcb.handle -= 1;
+ if (machineid != MACHINE_Z15A && machineid != MACHINE_Z15B) {
+ uvcb.handle += 1;
+ rc = uv_call(0, (uint64_t)&uvcb);
+ report(rc == 1 && uvcb.header.rc == UVC_RC_INV_CHANDLE, "invalid handle");
+ uvcb.handle -= 1;
+ }
rc = uv_call(0, (uint64_t)&uvcb);
report(rc == 0 && uvcb.header.rc == UVC_RC_EXECUTED, "success");
--
2.30.2
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [kvm-unit-tests PATCH 3/9] s390x: uv-host: Fence a destroy cpu test on z15
2021-09-22 7:18 ` [kvm-unit-tests PATCH 3/9] s390x: uv-host: Fence a destroy cpu test on z15 Janosch Frank
@ 2021-09-22 9:18 ` Claudio Imbrenda
2021-09-27 15:26 ` Thomas Huth
1 sibling, 0 replies; 34+ messages in thread
From: Claudio Imbrenda @ 2021-09-22 9:18 UTC (permalink / raw)
To: Janosch Frank; +Cc: kvm, thuth, david, linux-s390, seiden
On Wed, 22 Sep 2021 07:18:05 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:
> Firmware will not give us the expected return code on z15 so let's
> fence it for the z15 machine generation.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
> lib/s390x/asm/arch_def.h | 14 ++++++++++++++
> s390x/uv-host.c | 11 +++++++----
> 2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index aa80d840..c8d2722a 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -219,6 +219,20 @@ static inline unsigned short stap(void)
> return cpu_address;
> }
>
> +#define MACHINE_Z15A 0x8561
> +#define MACHINE_Z15B 0x8562
> +
> +static inline uint16_t get_machine_id(void)
> +{
> + uint64_t cpuid;
> +
> + asm volatile("stidp %0" : "=Q" (cpuid));
> + cpuid = cpuid >> 16;
> + cpuid &= 0xffff;
> +
> + return cpuid;
> +}
> +
> static inline int tprot(unsigned long addr)
> {
> int cc;
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index 66a11160..5e351120 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -111,6 +111,7 @@ static void test_config_destroy(void)
> static void test_cpu_destroy(void)
> {
> int rc;
> + uint16_t machineid = get_machine_id();
> struct uv_cb_nodata uvcb = {
> .header.len = sizeof(uvcb),
> .header.cmd = UVC_CMD_DESTROY_SEC_CPU,
> @@ -125,10 +126,12 @@ static void test_cpu_destroy(void)
> "hdr invalid length");
> uvcb.header.len += 8;
>
> - uvcb.handle += 1;
> - rc = uv_call(0, (uint64_t)&uvcb);
> - report(rc == 1 && uvcb.header.rc == UVC_RC_INV_CHANDLE, "invalid handle");
> - uvcb.handle -= 1;
> + if (machineid != MACHINE_Z15A && machineid != MACHINE_Z15B) {
> + uvcb.handle += 1;
> + rc = uv_call(0, (uint64_t)&uvcb);
> + report(rc == 1 && uvcb.header.rc == UVC_RC_INV_CHANDLE, "invalid handle");
> + uvcb.handle -= 1;
> + }
>
> rc = uv_call(0, (uint64_t)&uvcb);
> report(rc == 0 && uvcb.header.rc == UVC_RC_EXECUTED, "success");
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [kvm-unit-tests PATCH 3/9] s390x: uv-host: Fence a destroy cpu test on z15
2021-09-22 7:18 ` [kvm-unit-tests PATCH 3/9] s390x: uv-host: Fence a destroy cpu test on z15 Janosch Frank
2021-09-22 9:18 ` Claudio Imbrenda
@ 2021-09-27 15:26 ` Thomas Huth
2021-09-28 11:21 ` Janosch Frank
1 sibling, 1 reply; 34+ messages in thread
From: Thomas Huth @ 2021-09-27 15:26 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: david, linux-s390, seiden, imbrenda
On 22/09/2021 09.18, Janosch Frank wrote:
> Firmware will not give us the expected return code on z15 so let's
> fence it for the z15 machine generation.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> lib/s390x/asm/arch_def.h | 14 ++++++++++++++
> s390x/uv-host.c | 11 +++++++----
> 2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index aa80d840..c8d2722a 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -219,6 +219,20 @@ static inline unsigned short stap(void)
> return cpu_address;
> }
>
> +#define MACHINE_Z15A 0x8561
> +#define MACHINE_Z15B 0x8562
> +
> +static inline uint16_t get_machine_id(void)
> +{
> + uint64_t cpuid;
> +
> + asm volatile("stidp %0" : "=Q" (cpuid));
> + cpuid = cpuid >> 16;
> + cpuid &= 0xffff;
> +
> + return cpuid;
> +}
> +
> static inline int tprot(unsigned long addr)
> {
> int cc;
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index 66a11160..5e351120 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -111,6 +111,7 @@ static void test_config_destroy(void)
> static void test_cpu_destroy(void)
> {
> int rc;
> + uint16_t machineid = get_machine_id();
> struct uv_cb_nodata uvcb = {
> .header.len = sizeof(uvcb),
> .header.cmd = UVC_CMD_DESTROY_SEC_CPU,
> @@ -125,10 +126,12 @@ static void test_cpu_destroy(void)
> "hdr invalid length");
> uvcb.header.len += 8;
>
> - uvcb.handle += 1;
> - rc = uv_call(0, (uint64_t)&uvcb);
> - report(rc == 1 && uvcb.header.rc == UVC_RC_INV_CHANDLE, "invalid handle");
> - uvcb.handle -= 1;
> + if (machineid != MACHINE_Z15A && machineid != MACHINE_Z15B) {
> + uvcb.handle += 1;
> + rc = uv_call(0, (uint64_t)&uvcb);
> + report(rc == 1 && uvcb.header.rc == UVC_RC_INV_CHANDLE, "invalid handle");
> + uvcb.handle -= 1;
> + }
So this is a bug in the firmware? Any chance that it will still get fixed
for the z15? If so, would it make sense to turn this into a report_xfail()
instead?
Thomas
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [kvm-unit-tests PATCH 3/9] s390x: uv-host: Fence a destroy cpu test on z15
2021-09-27 15:26 ` Thomas Huth
@ 2021-09-28 11:21 ` Janosch Frank
2021-09-28 16:28 ` Thomas Huth
0 siblings, 1 reply; 34+ messages in thread
From: Janosch Frank @ 2021-09-28 11:21 UTC (permalink / raw)
To: Thomas Huth, kvm; +Cc: david, linux-s390, seiden, imbrenda
On 9/27/21 17:26, Thomas Huth wrote:
> On 22/09/2021 09.18, Janosch Frank wrote:
>> Firmware will not give us the expected return code on z15 so let's
>> fence it for the z15 machine generation.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> lib/s390x/asm/arch_def.h | 14 ++++++++++++++
>> s390x/uv-host.c | 11 +++++++----
>> 2 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>> index aa80d840..c8d2722a 100644
>> --- a/lib/s390x/asm/arch_def.h
>> +++ b/lib/s390x/asm/arch_def.h
>> @@ -219,6 +219,20 @@ static inline unsigned short stap(void)
>> return cpu_address;
>> }
>>
>> +#define MACHINE_Z15A 0x8561
>> +#define MACHINE_Z15B 0x8562
>> +
>> +static inline uint16_t get_machine_id(void)
>> +{
>> + uint64_t cpuid;
>> +
>> + asm volatile("stidp %0" : "=Q" (cpuid));
>> + cpuid = cpuid >> 16;
>> + cpuid &= 0xffff;
>> +
>> + return cpuid;
>> +}
>> +
>> static inline int tprot(unsigned long addr)
>> {
>> int cc;
>> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
>> index 66a11160..5e351120 100644
>> --- a/s390x/uv-host.c
>> +++ b/s390x/uv-host.c
>> @@ -111,6 +111,7 @@ static void test_config_destroy(void)
>> static void test_cpu_destroy(void)
>> {
>> int rc;
>> + uint16_t machineid = get_machine_id();
>> struct uv_cb_nodata uvcb = {
>> .header.len = sizeof(uvcb),
>> .header.cmd = UVC_CMD_DESTROY_SEC_CPU,
>> @@ -125,10 +126,12 @@ static void test_cpu_destroy(void)
>> "hdr invalid length");
>> uvcb.header.len += 8;
>>
>> - uvcb.handle += 1;
>> - rc = uv_call(0, (uint64_t)&uvcb);
>> - report(rc == 1 && uvcb.header.rc == UVC_RC_INV_CHANDLE, "invalid handle");
>> - uvcb.handle -= 1;
>> + if (machineid != MACHINE_Z15A && machineid != MACHINE_Z15B) {
>> + uvcb.handle += 1;
>> + rc = uv_call(0, (uint64_t)&uvcb);
>> + report(rc == 1 && uvcb.header.rc == UVC_RC_INV_CHANDLE, "invalid handle");
>> + uvcb.handle -= 1;
>> + }
>
> So this is a bug in the firmware? Any chance that it will still get fixed
> for the z15? If so, would it make sense to turn this into a report_xfail()
> instead?
>
> Thomas
>
No, a xfail will not help here.
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [kvm-unit-tests PATCH 3/9] s390x: uv-host: Fence a destroy cpu test on z15
2021-09-28 11:21 ` Janosch Frank
@ 2021-09-28 16:28 ` Thomas Huth
0 siblings, 0 replies; 34+ messages in thread
From: Thomas Huth @ 2021-09-28 16:28 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: david, linux-s390, seiden, imbrenda
On 28/09/2021 13.21, Janosch Frank wrote:
> On 9/27/21 17:26, Thomas Huth wrote:
>> On 22/09/2021 09.18, Janosch Frank wrote:
>>> Firmware will not give us the expected return code on z15 so let's
>>> fence it for the z15 machine generation.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>> lib/s390x/asm/arch_def.h | 14 ++++++++++++++
>>> s390x/uv-host.c | 11 +++++++----
>>> 2 files changed, 21 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>>> index aa80d840..c8d2722a 100644
>>> --- a/lib/s390x/asm/arch_def.h
>>> +++ b/lib/s390x/asm/arch_def.h
>>> @@ -219,6 +219,20 @@ static inline unsigned short stap(void)
>>> return cpu_address;
>>> }
>>> +#define MACHINE_Z15A 0x8561
>>> +#define MACHINE_Z15B 0x8562
>>> +
>>> +static inline uint16_t get_machine_id(void)
>>> +{
>>> + uint64_t cpuid;
>>> +
>>> + asm volatile("stidp %0" : "=Q" (cpuid));
>>> + cpuid = cpuid >> 16;
>>> + cpuid &= 0xffff;
You could skip the masking line since the function returns an uint61_t anyway.
>>> +
>>> + return cpuid;
>>> +}
>>> +
>>> static inline int tprot(unsigned long addr)
>>> {
>>> int cc;
>>> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
>>> index 66a11160..5e351120 100644
>>> --- a/s390x/uv-host.c
>>> +++ b/s390x/uv-host.c
>>> @@ -111,6 +111,7 @@ static void test_config_destroy(void)
>>> static void test_cpu_destroy(void)
>>> {
>>> int rc;
>>> + uint16_t machineid = get_machine_id();
>>> struct uv_cb_nodata uvcb = {
>>> .header.len = sizeof(uvcb),
>>> .header.cmd = UVC_CMD_DESTROY_SEC_CPU,
>>> @@ -125,10 +126,12 @@ static void test_cpu_destroy(void)
>>> "hdr invalid length");
>>> uvcb.header.len += 8;
>>> - uvcb.handle += 1;
>>> - rc = uv_call(0, (uint64_t)&uvcb);
>>> - report(rc == 1 && uvcb.header.rc == UVC_RC_INV_CHANDLE, "invalid
>>> handle");
>>> - uvcb.handle -= 1;
>>> + if (machineid != MACHINE_Z15A && machineid != MACHINE_Z15B) {
>>> + uvcb.handle += 1;
>>> + rc = uv_call(0, (uint64_t)&uvcb);
>>> + report(rc == 1 && uvcb.header.rc == UVC_RC_INV_CHANDLE, "invalid
>>> handle");
>>> + uvcb.handle -= 1;
>>> + }
>>
>> So this is a bug in the firmware? Any chance that it will still get fixed
>> for the z15? If so, would it make sense to turn this into a report_xfail()
>> instead?
>>
>> Thomas
>>
>
> No, a xfail will not help here.
Ok, fair, then I think the patch is fine:
Acked-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [kvm-unit-tests PATCH 4/9] lib: s390x: uv: Fix share return value and print
2021-09-22 7:18 [kvm-unit-tests PATCH 0/9] s390x: Cleanup and maintenance 2 Janosch Frank
` (2 preceding siblings ...)
2021-09-22 7:18 ` [kvm-unit-tests PATCH 3/9] s390x: uv-host: Fence a destroy cpu test on z15 Janosch Frank
@ 2021-09-22 7:18 ` Janosch Frank
2021-09-22 9:19 ` Claudio Imbrenda
2021-09-27 17:38 ` Thomas Huth
2021-09-22 7:18 ` [kvm-unit-tests PATCH 5/9] lib: s390x: uv: Add UVC_ERR_DEBUG switch Janosch Frank
` (4 subsequent siblings)
8 siblings, 2 replies; 34+ messages in thread
From: Janosch Frank @ 2021-09-22 7:18 UTC (permalink / raw)
To: kvm; +Cc: thuth, david, linux-s390, seiden, imbrenda
Let's only return 0/1 for success/failure respectively.
If needed we can later add rc/rrc pointers so we can check for the
reasons of cc==1 cases like we do in the kernel.
As share might also be used in snippets it's best not to use prints to
avoid linking problems so lets remove the report_info().
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
lib/s390x/asm/uv.h | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
index ec10d1c4..2f099553 100644
--- a/lib/s390x/asm/uv.h
+++ b/lib/s390x/asm/uv.h
@@ -219,15 +219,8 @@ static inline int share(unsigned long addr, u16 cmd)
.header.len = sizeof(uvcb),
.paddr = addr
};
- int cc;
- cc = uv_call(0, (u64)&uvcb);
- if (!cc && uvcb.header.rc == UVC_RC_EXECUTED)
- return 0;
-
- report_info("uv_call: cmd %04x cc %d response code: %04x", cc, cmd,
- uvcb.header.rc);
- return -1;
+ return uv_call(0, (u64)&uvcb);
}
/*
--
2.30.2
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [kvm-unit-tests PATCH 4/9] lib: s390x: uv: Fix share return value and print
2021-09-22 7:18 ` [kvm-unit-tests PATCH 4/9] lib: s390x: uv: Fix share return value and print Janosch Frank
@ 2021-09-22 9:19 ` Claudio Imbrenda
2021-09-27 17:38 ` Thomas Huth
1 sibling, 0 replies; 34+ messages in thread
From: Claudio Imbrenda @ 2021-09-22 9:19 UTC (permalink / raw)
To: Janosch Frank; +Cc: kvm, thuth, david, linux-s390, seiden
On Wed, 22 Sep 2021 07:18:06 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:
> Let's only return 0/1 for success/failure respectively.
> If needed we can later add rc/rrc pointers so we can check for the
> reasons of cc==1 cases like we do in the kernel.
>
> As share might also be used in snippets it's best not to use prints to
> avoid linking problems so lets remove the report_info().
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
> lib/s390x/asm/uv.h | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
> index ec10d1c4..2f099553 100644
> --- a/lib/s390x/asm/uv.h
> +++ b/lib/s390x/asm/uv.h
> @@ -219,15 +219,8 @@ static inline int share(unsigned long addr, u16 cmd)
> .header.len = sizeof(uvcb),
> .paddr = addr
> };
> - int cc;
>
> - cc = uv_call(0, (u64)&uvcb);
> - if (!cc && uvcb.header.rc == UVC_RC_EXECUTED)
> - return 0;
> -
> - report_info("uv_call: cmd %04x cc %d response code: %04x", cc, cmd,
> - uvcb.header.rc);
> - return -1;
> + return uv_call(0, (u64)&uvcb);
> }
>
> /*
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [kvm-unit-tests PATCH 4/9] lib: s390x: uv: Fix share return value and print
2021-09-22 7:18 ` [kvm-unit-tests PATCH 4/9] lib: s390x: uv: Fix share return value and print Janosch Frank
2021-09-22 9:19 ` Claudio Imbrenda
@ 2021-09-27 17:38 ` Thomas Huth
1 sibling, 0 replies; 34+ messages in thread
From: Thomas Huth @ 2021-09-27 17:38 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: david, linux-s390, seiden, imbrenda
On 22/09/2021 09.18, Janosch Frank wrote:
> Let's only return 0/1 for success/failure respectively.
> If needed we can later add rc/rrc pointers so we can check for the
> reasons of cc==1 cases like we do in the kernel.
>
> As share might also be used in snippets it's best not to use prints to
Maybe: s/share/share()/
... so that it is clear that you're talking about the function here...
> avoid linking problems so lets remove the report_info().
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> lib/s390x/asm/uv.h | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
> index ec10d1c4..2f099553 100644
> --- a/lib/s390x/asm/uv.h
> +++ b/lib/s390x/asm/uv.h
> @@ -219,15 +219,8 @@ static inline int share(unsigned long addr, u16 cmd)
> .header.len = sizeof(uvcb),
> .paddr = addr
> };
> - int cc;
>
> - cc = uv_call(0, (u64)&uvcb);
> - if (!cc && uvcb.header.rc == UVC_RC_EXECUTED)
> - return 0;
> -
> - report_info("uv_call: cmd %04x cc %d response code: %04x", cc, cmd,
> - uvcb.header.rc);
> - return -1;
> + return uv_call(0, (u64)&uvcb);
> }
Acked-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [kvm-unit-tests PATCH 5/9] lib: s390x: uv: Add UVC_ERR_DEBUG switch
2021-09-22 7:18 [kvm-unit-tests PATCH 0/9] s390x: Cleanup and maintenance 2 Janosch Frank
` (3 preceding siblings ...)
2021-09-22 7:18 ` [kvm-unit-tests PATCH 4/9] lib: s390x: uv: Fix share return value and print Janosch Frank
@ 2021-09-22 7:18 ` Janosch Frank
2021-09-22 9:23 ` Claudio Imbrenda
2021-09-27 17:41 ` Thomas Huth
2021-09-22 7:18 ` [kvm-unit-tests PATCH 6/9] lib: s390x: Print PGM code as hex Janosch Frank
` (3 subsequent siblings)
8 siblings, 2 replies; 34+ messages in thread
From: Janosch Frank @ 2021-09-22 7:18 UTC (permalink / raw)
To: kvm; +Cc: thuth, david, linux-s390, seiden, imbrenda
Every time something goes wrong in a way we don't expect, we need to
add debug prints to some UVC to get the unexpected return code.
Let's just put the printing behind a macro so we can enable it if
needed via a simple switch.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
lib/s390x/asm/uv.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
index 2f099553..0e958ad7 100644
--- a/lib/s390x/asm/uv.h
+++ b/lib/s390x/asm/uv.h
@@ -12,6 +12,9 @@
#ifndef _ASMS390X_UV_H_
#define _ASMS390X_UV_H_
+/* Enables printing of command code and return codes for failed UVCs */
+#define UVC_ERR_DEBUG 0
+
#define UVC_RC_EXECUTED 0x0001
#define UVC_RC_INV_CMD 0x0002
#define UVC_RC_INV_STATE 0x0003
@@ -194,6 +197,15 @@ static inline int uv_call_once(unsigned long r1, unsigned long r2)
: [cc] "=d" (cc)
: [r1] "a" (r1), [r2] "a" (r2)
: "memory", "cc");
+
+#if UVC_ERR_DEBUG
+ if (cc)
+ printf("UV call error: call %x rc %x rrc %x\n",
+ ((struct uv_cb_header *)r2)->cmd,
+ ((struct uv_cb_header *)r2)->rc,
+ ((struct uv_cb_header *)r2)->rrc);
+#endif
+
return cc;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [kvm-unit-tests PATCH 5/9] lib: s390x: uv: Add UVC_ERR_DEBUG switch
2021-09-22 7:18 ` [kvm-unit-tests PATCH 5/9] lib: s390x: uv: Add UVC_ERR_DEBUG switch Janosch Frank
@ 2021-09-22 9:23 ` Claudio Imbrenda
2021-09-22 11:37 ` Janosch Frank
2021-09-27 17:41 ` Thomas Huth
1 sibling, 1 reply; 34+ messages in thread
From: Claudio Imbrenda @ 2021-09-22 9:23 UTC (permalink / raw)
To: Janosch Frank; +Cc: kvm, thuth, david, linux-s390, seiden
On Wed, 22 Sep 2021 07:18:07 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:
> Every time something goes wrong in a way we don't expect, we need to
> add debug prints to some UVC to get the unexpected return code.
>
> Let's just put the printing behind a macro so we can enable it if
> needed via a simple switch.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
but see a nit below
> ---
> lib/s390x/asm/uv.h | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
> index 2f099553..0e958ad7 100644
> --- a/lib/s390x/asm/uv.h
> +++ b/lib/s390x/asm/uv.h
> @@ -12,6 +12,9 @@
> #ifndef _ASMS390X_UV_H_
> #define _ASMS390X_UV_H_
>
> +/* Enables printing of command code and return codes for failed UVCs
> */ +#define UVC_ERR_DEBUG 0
> +
> #define UVC_RC_EXECUTED 0x0001
> #define UVC_RC_INV_CMD 0x0002
> #define UVC_RC_INV_STATE 0x0003
> @@ -194,6 +197,15 @@ static inline int uv_call_once(unsigned long r1,
> unsigned long r2) : [cc] "=d" (cc)
> : [r1] "a" (r1), [r2] "a" (r2)
> : "memory", "cc");
> +
> +#if UVC_ERR_DEBUG
> + if (cc)
it probably looks cleaner like this:
if (UVC_ERR_DEBUG && cc)
and without the #if; the compiler should be smart enough to remove the
dead code. In practice it doesn't really matter in the end, so feel free
to ignore this comment :)
> + printf("UV call error: call %x rc %x rrc %x\n",
> + ((struct uv_cb_header *)r2)->cmd,
> + ((struct uv_cb_header *)r2)->rc,
> + ((struct uv_cb_header *)r2)->rrc);
> +#endif
> +
> return cc;
> }
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [kvm-unit-tests PATCH 5/9] lib: s390x: uv: Add UVC_ERR_DEBUG switch
2021-09-22 9:23 ` Claudio Imbrenda
@ 2021-09-22 11:37 ` Janosch Frank
0 siblings, 0 replies; 34+ messages in thread
From: Janosch Frank @ 2021-09-22 11:37 UTC (permalink / raw)
To: Claudio Imbrenda; +Cc: kvm, thuth, david, linux-s390, seiden
On 9/22/21 11:23 AM, Claudio Imbrenda wrote:
> On Wed, 22 Sep 2021 07:18:07 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
>
>> Every time something goes wrong in a way we don't expect, we need to
>> add debug prints to some UVC to get the unexpected return code.
>>
>> Let's just put the printing behind a macro so we can enable it if
>> needed via a simple switch.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>
> but see a nit below
>
>> ---
>> lib/s390x/asm/uv.h | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
>> index 2f099553..0e958ad7 100644
>> --- a/lib/s390x/asm/uv.h
>> +++ b/lib/s390x/asm/uv.h
>> @@ -12,6 +12,9 @@
>> #ifndef _ASMS390X_UV_H_
>> #define _ASMS390X_UV_H_
>>
>> +/* Enables printing of command code and return codes for failed UVCs
>> */ +#define UVC_ERR_DEBUG 0
>> +
>> #define UVC_RC_EXECUTED 0x0001
>> #define UVC_RC_INV_CMD 0x0002
>> #define UVC_RC_INV_STATE 0x0003
>> @@ -194,6 +197,15 @@ static inline int uv_call_once(unsigned long r1,
>> unsigned long r2) : [cc] "=d" (cc)
>> : [r1] "a" (r1), [r2] "a" (r2)
>> : "memory", "cc");
>> +
>> +#if UVC_ERR_DEBUG
>> + if (cc)
>
> it probably looks cleaner like this:
>
> if (UVC_ERR_DEBUG && cc)
>
> and without the #if; the compiler should be smart enough to remove the
> dead code. In practice it doesn't really matter in the end, so feel free
> to ignore this comment :)
Didn't consider that, I'll fix it up.
Thank you
>
>> + printf("UV call error: call %x rc %x rrc %x\n",
>> + ((struct uv_cb_header *)r2)->cmd,
>> + ((struct uv_cb_header *)r2)->rc,
>> + ((struct uv_cb_header *)r2)->rrc);
>> +#endif
>> +
>> return cc;
>> }
>>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH 5/9] lib: s390x: uv: Add UVC_ERR_DEBUG switch
2021-09-22 7:18 ` [kvm-unit-tests PATCH 5/9] lib: s390x: uv: Add UVC_ERR_DEBUG switch Janosch Frank
2021-09-22 9:23 ` Claudio Imbrenda
@ 2021-09-27 17:41 ` Thomas Huth
2021-09-28 10:00 ` Janosch Frank
1 sibling, 1 reply; 34+ messages in thread
From: Thomas Huth @ 2021-09-27 17:41 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: david, linux-s390, seiden, imbrenda
On 22/09/2021 09.18, Janosch Frank wrote:
> Every time something goes wrong in a way we don't expect, we need to
> add debug prints to some UVC to get the unexpected return code.
>
> Let's just put the printing behind a macro so we can enable it if
> needed via a simple switch.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> lib/s390x/asm/uv.h | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
> index 2f099553..0e958ad7 100644
> --- a/lib/s390x/asm/uv.h
> +++ b/lib/s390x/asm/uv.h
> @@ -12,6 +12,9 @@
> #ifndef _ASMS390X_UV_H_
> #define _ASMS390X_UV_H_
>
> +/* Enables printing of command code and return codes for failed UVCs */
> +#define UVC_ERR_DEBUG 0
Do we maybe want a "#ifndef UVC_ERR_DEBUG" in front of this, so that we
could also set the macro to 1 from individual *.c files (or from the Makefile)?
Thomas
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH 5/9] lib: s390x: uv: Add UVC_ERR_DEBUG switch
2021-09-27 17:41 ` Thomas Huth
@ 2021-09-28 10:00 ` Janosch Frank
0 siblings, 0 replies; 34+ messages in thread
From: Janosch Frank @ 2021-09-28 10:00 UTC (permalink / raw)
To: Thomas Huth, kvm; +Cc: david, linux-s390, seiden, imbrenda
On 9/27/21 19:41, Thomas Huth wrote:
> On 22/09/2021 09.18, Janosch Frank wrote:
>> Every time something goes wrong in a way we don't expect, we need to
>> add debug prints to some UVC to get the unexpected return code.
>>
>> Let's just put the printing behind a macro so we can enable it if
>> needed via a simple switch.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> lib/s390x/asm/uv.h | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
>> index 2f099553..0e958ad7 100644
>> --- a/lib/s390x/asm/uv.h
>> +++ b/lib/s390x/asm/uv.h
>> @@ -12,6 +12,9 @@
>> #ifndef _ASMS390X_UV_H_
>> #define _ASMS390X_UV_H_
>>
>> +/* Enables printing of command code and return codes for failed UVCs */
>> +#define UVC_ERR_DEBUG 0
>
> Do we maybe want a "#ifndef UVC_ERR_DEBUG" in front of this, so that we
> could also set the macro to 1 from individual *.c files (or from the Makefile)?
>
> Thomas
>
When testing it's the least amount of work to set this to 1 so I
implemented it this way. If you think it's useful then I'll add the ifndef.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [kvm-unit-tests PATCH 6/9] lib: s390x: Print PGM code as hex
2021-09-22 7:18 [kvm-unit-tests PATCH 0/9] s390x: Cleanup and maintenance 2 Janosch Frank
` (4 preceding siblings ...)
2021-09-22 7:18 ` [kvm-unit-tests PATCH 5/9] lib: s390x: uv: Add UVC_ERR_DEBUG switch Janosch Frank
@ 2021-09-22 7:18 ` Janosch Frank
2021-09-22 9:24 ` Claudio Imbrenda
2021-09-27 17:43 ` Thomas Huth
2021-09-22 7:18 ` [kvm-unit-tests PATCH 7/9] s390x: Makefile: Remove snippet flatlib linking Janosch Frank
` (2 subsequent siblings)
8 siblings, 2 replies; 34+ messages in thread
From: Janosch Frank @ 2021-09-22 7:18 UTC (permalink / raw)
To: kvm; +Cc: thuth, david, linux-s390, seiden, imbrenda
We have them defined as hex constants in lib/s390x/asm/arch_def.h so
why not print them as hex values?
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
lib/s390x/interrupt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 126d4c0a..27d3b767 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -169,7 +169,7 @@ static void print_pgm_info(struct stack_frame_int *stack)
lc->pgm_old_psw.addr <= (uintptr_t)sie_exit);
printf("\n");
- printf("Unexpected program interrupt %s: %d on cpu %d at %#lx, ilen %d\n",
+ printf("Unexpected program interrupt %s: %#x on cpu %d at %#lx, ilen %d\n",
in_sie ? "in SIE" : "",
lc->pgm_int_code, stap(), lc->pgm_old_psw.addr, lc->pgm_int_id);
print_int_regs(stack);
--
2.30.2
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [kvm-unit-tests PATCH 6/9] lib: s390x: Print PGM code as hex
2021-09-22 7:18 ` [kvm-unit-tests PATCH 6/9] lib: s390x: Print PGM code as hex Janosch Frank
@ 2021-09-22 9:24 ` Claudio Imbrenda
2021-09-27 17:43 ` Thomas Huth
1 sibling, 0 replies; 34+ messages in thread
From: Claudio Imbrenda @ 2021-09-22 9:24 UTC (permalink / raw)
To: Janosch Frank; +Cc: kvm, thuth, david, linux-s390, seiden
On Wed, 22 Sep 2021 07:18:08 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:
> We have them defined as hex constants in lib/s390x/asm/arch_def.h so
> why not print them as hex values?
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
> lib/s390x/interrupt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 126d4c0a..27d3b767 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -169,7 +169,7 @@ static void print_pgm_info(struct stack_frame_int *stack)
> lc->pgm_old_psw.addr <= (uintptr_t)sie_exit);
>
> printf("\n");
> - printf("Unexpected program interrupt %s: %d on cpu %d at %#lx, ilen %d\n",
> + printf("Unexpected program interrupt %s: %#x on cpu %d at %#lx, ilen %d\n",
> in_sie ? "in SIE" : "",
> lc->pgm_int_code, stap(), lc->pgm_old_psw.addr, lc->pgm_int_id);
> print_int_regs(stack);
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [kvm-unit-tests PATCH 6/9] lib: s390x: Print PGM code as hex
2021-09-22 7:18 ` [kvm-unit-tests PATCH 6/9] lib: s390x: Print PGM code as hex Janosch Frank
2021-09-22 9:24 ` Claudio Imbrenda
@ 2021-09-27 17:43 ` Thomas Huth
1 sibling, 0 replies; 34+ messages in thread
From: Thomas Huth @ 2021-09-27 17:43 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: david, linux-s390, seiden, imbrenda
On 22/09/2021 09.18, Janosch Frank wrote:
> We have them defined as hex constants in lib/s390x/asm/arch_def.h so
> why not print them as hex values?
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> lib/s390x/interrupt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 126d4c0a..27d3b767 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -169,7 +169,7 @@ static void print_pgm_info(struct stack_frame_int *stack)
> lc->pgm_old_psw.addr <= (uintptr_t)sie_exit);
>
> printf("\n");
> - printf("Unexpected program interrupt %s: %d on cpu %d at %#lx, ilen %d\n",
> + printf("Unexpected program interrupt %s: %#x on cpu %d at %#lx, ilen %d\n",
> in_sie ? "in SIE" : "",
> lc->pgm_int_code, stap(), lc->pgm_old_psw.addr, lc->pgm_int_id);
> print_int_regs(stack);
>
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [kvm-unit-tests PATCH 7/9] s390x: Makefile: Remove snippet flatlib linking
2021-09-22 7:18 [kvm-unit-tests PATCH 0/9] s390x: Cleanup and maintenance 2 Janosch Frank
` (5 preceding siblings ...)
2021-09-22 7:18 ` [kvm-unit-tests PATCH 6/9] lib: s390x: Print PGM code as hex Janosch Frank
@ 2021-09-22 7:18 ` Janosch Frank
2021-09-27 17:47 ` Thomas Huth
2021-09-22 7:18 ` [kvm-unit-tests PATCH 8/9] s390x: Add sthyi cc==0 r2+1 verification Janosch Frank
2021-09-22 7:18 ` [kvm-unit-tests PATCH 9/9] s390x: skrf: Fix tprot assembly Janosch Frank
8 siblings, 1 reply; 34+ messages in thread
From: Janosch Frank @ 2021-09-22 7:18 UTC (permalink / raw)
To: kvm; +Cc: thuth, david, linux-s390, seiden, imbrenda
We can't link the flatlib as we do not export everything that it needs
and we don't (want to) call the init functions.
In the future we might implement a tiny lib that uses select lib
object files and re-implements functions like assert() and
test_facility() to be able to use some parts of the lib.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
s390x/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/s390x/Makefile b/s390x/Makefile
index 5d1a33a0..d09c0a17 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -92,7 +92,7 @@ $(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o $(FLATLIBS)
$(OBJCOPY) -I binary -O elf64-s390 -B "s390:64-bit" $@ $@
$(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_asmlib) $(FLATLIBS)
- $(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/c/flat.lds $(patsubst %.gbin,%.o,$@) $(snippet_asmlib) $(FLATLIBS)
+ $(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/c/flat.lds $(patsubst %.gbin,%.o,$@) $(snippet_asmlib)
$(OBJCOPY) -O binary $@ $@
$(OBJCOPY) -I binary -O elf64-s390 -B "s390:64-bit" $@ $@
--
2.30.2
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [kvm-unit-tests PATCH 7/9] s390x: Makefile: Remove snippet flatlib linking
2021-09-22 7:18 ` [kvm-unit-tests PATCH 7/9] s390x: Makefile: Remove snippet flatlib linking Janosch Frank
@ 2021-09-27 17:47 ` Thomas Huth
2021-09-28 9:57 ` Janosch Frank
0 siblings, 1 reply; 34+ messages in thread
From: Thomas Huth @ 2021-09-27 17:47 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: david, linux-s390, seiden, imbrenda
On 22/09/2021 09.18, Janosch Frank wrote:
> We can't link the flatlib as we do not export everything that it needs
> and we don't (want to) call the init functions.
>
> In the future we might implement a tiny lib that uses select lib
s/select/selected/ ?
> object files and re-implements functions like assert() and
> test_facility() to be able to use some parts of the lib.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> s390x/Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 5d1a33a0..d09c0a17 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -92,7 +92,7 @@ $(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o $(FLATLIBS)
> $(OBJCOPY) -I binary -O elf64-s390 -B "s390:64-bit" $@ $@
>
> $(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_asmlib) $(FLATLIBS)
> - $(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/c/flat.lds $(patsubst %.gbin,%.o,$@) $(snippet_asmlib) $(FLATLIBS)
> + $(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/c/flat.lds $(patsubst %.gbin,%.o,$@) $(snippet_asmlib)
Don't we need memcpy() and friends in some cases? ... well, likely not,
otherwise linking would fail... So:
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH 7/9] s390x: Makefile: Remove snippet flatlib linking
2021-09-27 17:47 ` Thomas Huth
@ 2021-09-28 9:57 ` Janosch Frank
0 siblings, 0 replies; 34+ messages in thread
From: Janosch Frank @ 2021-09-28 9:57 UTC (permalink / raw)
To: Thomas Huth, kvm; +Cc: david, linux-s390, seiden, imbrenda
On 9/27/21 19:47, Thomas Huth wrote:
> On 22/09/2021 09.18, Janosch Frank wrote:
>> We can't link the flatlib as we do not export everything that it needs
>> and we don't (want to) call the init functions.
>>
>> In the future we might implement a tiny lib that uses select lib
>
> s/select/selected/ ?
Sure
>
>> object files and re-implements functions like assert() and
>> test_facility() to be able to use some parts of the lib.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> s390x/Makefile | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index 5d1a33a0..d09c0a17 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -92,7 +92,7 @@ $(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o $(FLATLIBS)
>> $(OBJCOPY) -I binary -O elf64-s390 -B "s390:64-bit" $@ $@
>>
>> $(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_asmlib) $(FLATLIBS)
>> - $(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/c/flat.lds $(patsubst %.gbin,%.o,$@) $(snippet_asmlib) $(FLATLIBS)
>> + $(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/c/flat.lds $(patsubst %.gbin,%.o,$@) $(snippet_asmlib)
>
> Don't we need memcpy() and friends in some cases? ... well, likely not,
> otherwise linking would fail... So:
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>
Currently not but we can still import header files and use static things
which is very helpful already. Fixing this will require a lot of thought
since I don't really want to write a new lib just for the snippets but
making them compatible to the flatlib will also require a lot of work.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [kvm-unit-tests PATCH 8/9] s390x: Add sthyi cc==0 r2+1 verification
2021-09-22 7:18 [kvm-unit-tests PATCH 0/9] s390x: Cleanup and maintenance 2 Janosch Frank
` (6 preceding siblings ...)
2021-09-22 7:18 ` [kvm-unit-tests PATCH 7/9] s390x: Makefile: Remove snippet flatlib linking Janosch Frank
@ 2021-09-22 7:18 ` Janosch Frank
2021-09-22 9:31 ` Claudio Imbrenda
2021-09-22 7:18 ` [kvm-unit-tests PATCH 9/9] s390x: skrf: Fix tprot assembly Janosch Frank
8 siblings, 1 reply; 34+ messages in thread
From: Janosch Frank @ 2021-09-22 7:18 UTC (permalink / raw)
To: kvm; +Cc: thuth, david, linux-s390, seiden, imbrenda
On success r2 + 1 should be 0, let's also check for that.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
s390x/sthyi.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/s390x/sthyi.c b/s390x/sthyi.c
index db90b56f..4b153bf4 100644
--- a/s390x/sthyi.c
+++ b/s390x/sthyi.c
@@ -24,16 +24,16 @@ static inline int sthyi(uint64_t vaddr, uint64_t fcode, uint64_t *rc,
{
register uint64_t code asm("0") = fcode;
register uint64_t addr asm("2") = vaddr;
- register uint64_t rc3 asm("3") = 0;
+ register uint64_t rc3 asm("3") = 42;
int cc = 0;
- asm volatile(".insn rre,0xB2560000,%[r1],%[r2]\n"
- "ipm %[cc]\n"
- "srl %[cc],28\n"
- : [cc] "=d" (cc)
- : [code] "d" (code), [addr] "a" (addr), [r1] "i" (r1),
- [r2] "i" (r2)
- : "memory", "cc", "r3");
+ asm volatile(
+ ".insn rre,0xB2560000,%[r1],%[r2]\n"
+ "ipm %[cc]\n"
+ "srl %[cc],28\n"
+ : [cc] "=d" (cc), "+d" (rc3)
+ : [code] "d" (code), [addr] "a" (addr), [r1] "i" (r1), [r2] "i" (r2)
+ : "memory", "cc");
if (rc)
*rc = rc3;
return cc;
@@ -139,16 +139,18 @@ static void test_fcode0(void)
struct sthyi_hdr_sctn *hdr;
struct sthyi_mach_sctn *mach;
struct sthyi_par_sctn *par;
+ uint64_t rc = 42;
/* Zero destination memory. */
memset(pagebuf, 0, PAGE_SIZE);
report_prefix_push("fcode 0");
- sthyi((uint64_t)pagebuf, 0, NULL, 0, 2);
+ sthyi((uint64_t)pagebuf, 0, &rc, 0, 2);
hdr = (void *)pagebuf;
mach = (void *)pagebuf + hdr->INFMOFF;
par = (void *)pagebuf + hdr->INFPOFF;
+ report(!rc, "r2 + 1 == 0");
test_fcode0_hdr(hdr);
test_fcode0_mach(mach);
test_fcode0_par(par);
--
2.30.2
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [kvm-unit-tests PATCH 8/9] s390x: Add sthyi cc==0 r2+1 verification
2021-09-22 7:18 ` [kvm-unit-tests PATCH 8/9] s390x: Add sthyi cc==0 r2+1 verification Janosch Frank
@ 2021-09-22 9:31 ` Claudio Imbrenda
0 siblings, 0 replies; 34+ messages in thread
From: Claudio Imbrenda @ 2021-09-22 9:31 UTC (permalink / raw)
To: Janosch Frank; +Cc: kvm, thuth, david, linux-s390, seiden
On Wed, 22 Sep 2021 07:18:10 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:
> On success r2 + 1 should be 0, let's also check for that.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
but see comment below
> ---
> s390x/sthyi.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/s390x/sthyi.c b/s390x/sthyi.c
> index db90b56f..4b153bf4 100644
> --- a/s390x/sthyi.c
> +++ b/s390x/sthyi.c
> @@ -24,16 +24,16 @@ static inline int sthyi(uint64_t vaddr, uint64_t fcode, uint64_t *rc,
> {
> register uint64_t code asm("0") = fcode;
> register uint64_t addr asm("2") = vaddr;
> - register uint64_t rc3 asm("3") = 0;
> + register uint64_t rc3 asm("3") = 42;
I am not a big fan of the asm constraints, in the kernel we are
trying to move away from them, but I guess as long as it works it's ok
> int cc = 0;
>
> - asm volatile(".insn rre,0xB2560000,%[r1],%[r2]\n"
> - "ipm %[cc]\n"
> - "srl %[cc],28\n"
> - : [cc] "=d" (cc)
> - : [code] "d" (code), [addr] "a" (addr), [r1] "i" (r1),
> - [r2] "i" (r2)
> - : "memory", "cc", "r3");
> + asm volatile(
> + ".insn rre,0xB2560000,%[r1],%[r2]\n"
> + "ipm %[cc]\n"
> + "srl %[cc],28\n"
> + : [cc] "=d" (cc), "+d" (rc3)
> + : [code] "d" (code), [addr] "a" (addr), [r1] "i" (r1), [r2] "i" (r2)
> + : "memory", "cc");
> if (rc)
> *rc = rc3;
> return cc;
> @@ -139,16 +139,18 @@ static void test_fcode0(void)
> struct sthyi_hdr_sctn *hdr;
> struct sthyi_mach_sctn *mach;
> struct sthyi_par_sctn *par;
> + uint64_t rc = 42;
>
> /* Zero destination memory. */
> memset(pagebuf, 0, PAGE_SIZE);
>
> report_prefix_push("fcode 0");
> - sthyi((uint64_t)pagebuf, 0, NULL, 0, 2);
> + sthyi((uint64_t)pagebuf, 0, &rc, 0, 2);
> hdr = (void *)pagebuf;
> mach = (void *)pagebuf + hdr->INFMOFF;
> par = (void *)pagebuf + hdr->INFPOFF;
>
> + report(!rc, "r2 + 1 == 0");
> test_fcode0_hdr(hdr);
> test_fcode0_mach(mach);
> test_fcode0_par(par);
^ permalink raw reply [flat|nested] 34+ messages in thread
* [kvm-unit-tests PATCH 9/9] s390x: skrf: Fix tprot assembly
2021-09-22 7:18 [kvm-unit-tests PATCH 0/9] s390x: Cleanup and maintenance 2 Janosch Frank
` (7 preceding siblings ...)
2021-09-22 7:18 ` [kvm-unit-tests PATCH 8/9] s390x: Add sthyi cc==0 r2+1 verification Janosch Frank
@ 2021-09-22 7:18 ` Janosch Frank
2021-09-22 9:34 ` Claudio Imbrenda
[not found] ` <20210922134112.174842-1-scgl@linux.ibm.com>
8 siblings, 2 replies; 34+ messages in thread
From: Janosch Frank @ 2021-09-22 7:18 UTC (permalink / raw)
To: kvm; +Cc: thuth, david, linux-s390, seiden, imbrenda
It's a base + displacement address so we need to address it via 0(%[addr]).
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
s390x/skrf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/s390x/skrf.c b/s390x/skrf.c
index 8ca7588c..84fb762c 100644
--- a/s390x/skrf.c
+++ b/s390x/skrf.c
@@ -103,7 +103,7 @@ static void test_tprot(void)
{
report_prefix_push("tprot");
expect_pgm_int();
- asm volatile("tprot %[addr],0xf0(0)\n"
+ asm volatile("tprot 0(%[addr]),0xf0(0)\n"
: : [addr] "a" (pagebuf) : );
check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
report_prefix_pop();
--
2.30.2
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [kvm-unit-tests PATCH 9/9] s390x: skrf: Fix tprot assembly
2021-09-22 7:18 ` [kvm-unit-tests PATCH 9/9] s390x: skrf: Fix tprot assembly Janosch Frank
@ 2021-09-22 9:34 ` Claudio Imbrenda
2021-09-22 11:47 ` Janosch Frank
[not found] ` <20210922134112.174842-1-scgl@linux.ibm.com>
1 sibling, 1 reply; 34+ messages in thread
From: Claudio Imbrenda @ 2021-09-22 9:34 UTC (permalink / raw)
To: Janosch Frank; +Cc: kvm, thuth, david, linux-s390, seiden
On Wed, 22 Sep 2021 07:18:11 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:
> It's a base + displacement address so we need to address it via 0(%[addr]).
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
but see comment below
> ---
> s390x/skrf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/s390x/skrf.c b/s390x/skrf.c
> index 8ca7588c..84fb762c 100644
> --- a/s390x/skrf.c
> +++ b/s390x/skrf.c
> @@ -103,7 +103,7 @@ static void test_tprot(void)
> {
> report_prefix_push("tprot");
> expect_pgm_int();
> - asm volatile("tprot %[addr],0xf0(0)\n"
> + asm volatile("tprot 0(%[addr]),0xf0(0)\n"
I think the displacement defaults to 0 if not specified?
did you get a warning, or why are you changing this now?
> : : [addr] "a" (pagebuf) : );
> check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
> report_prefix_pop();
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [kvm-unit-tests PATCH 9/9] s390x: skrf: Fix tprot assembly
2021-09-22 9:34 ` Claudio Imbrenda
@ 2021-09-22 11:47 ` Janosch Frank
0 siblings, 0 replies; 34+ messages in thread
From: Janosch Frank @ 2021-09-22 11:47 UTC (permalink / raw)
To: Claudio Imbrenda; +Cc: kvm, thuth, david, linux-s390, seiden
On 9/22/21 11:34 AM, Claudio Imbrenda wrote:
> On Wed, 22 Sep 2021 07:18:11 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
>
>> It's a base + displacement address so we need to address it via 0(%[addr]).
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>
> but see comment below
>
>> ---
>> s390x/skrf.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/s390x/skrf.c b/s390x/skrf.c
>> index 8ca7588c..84fb762c 100644
>> --- a/s390x/skrf.c
>> +++ b/s390x/skrf.c
>> @@ -103,7 +103,7 @@ static void test_tprot(void)
>> {
>> report_prefix_push("tprot");
>> expect_pgm_int();
>> - asm volatile("tprot %[addr],0xf0(0)\n"
>> + asm volatile("tprot 0(%[addr]),0xf0(0)\n"
>
> I think the displacement defaults to 0 if not specified?
>
> did you get a warning, or why are you changing this now?
It fixes one of the ~18 clang warnings and making it explicit directly
tells you it's a B+D instruction i.e. it looks cleaner to me.
>
>> : : [addr] "a" (pagebuf) : );
>> check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
>> report_prefix_pop();
>
^ permalink raw reply [flat|nested] 34+ messages in thread
[parent not found: <20210922134112.174842-1-scgl@linux.ibm.com>]
* Re: [kvm-unit-tests PATCH 9/9] s390x: skrf: Fix tprot assembly
[not found] ` <20210922134112.174842-1-scgl@linux.ibm.com>
@ 2021-09-22 13:53 ` Janosch Frank
0 siblings, 0 replies; 34+ messages in thread
From: Janosch Frank @ 2021-09-22 13:53 UTC (permalink / raw)
To: Janis Schoetterl-Glausch, kvm; +Cc: thuth, david, linux-s390, seiden, imbrenda
On 9/22/21 3:41 PM, Janis Schoetterl-Glausch wrote:
> On Wed, Sep 22, 2021 at 07:18:11AM +0000, Janosch Frank wrote:
>> It's a base + displacement address so we need to address it via 0(%[addr]).
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> s390x/skrf.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/s390x/skrf.c b/s390x/skrf.c
>> index 8ca7588c..84fb762c 100644
>> --- a/s390x/skrf.c
>> +++ b/s390x/skrf.c
>> @@ -103,7 +103,7 @@ static void test_tprot(void)
>> {
>> report_prefix_push("tprot");
>> expect_pgm_int();
>> - asm volatile("tprot %[addr],0xf0(0)\n"
>> + asm volatile("tprot 0(%[addr]),0xf0(0)\n"
>> : : [addr] "a" (pagebuf) : );
>> check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
>> report_prefix_pop();
>> --
>> 2.30.2
>>
>
> Let's add an argument to tprot instead.
Sure, LGTM
> -- >8 --
> Subject: [kvm-unit-tests PATCH] lib: s390x: Add access key argument to tprot
>
> Currently there is only one callee passing a non zero key,
> but having the argument will be useful in the future.
>
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
> lib/s390x/asm/arch_def.h | 6 +++---
> lib/s390x/sclp.c | 2 +-
> s390x/skrf.c | 3 +--
> 3 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 302ef1f..55f3124 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -206,15 +206,15 @@ static inline unsigned short stap(void)
> return cpu_address;
> }
>
> -static inline int tprot(unsigned long addr)
> +static inline int tprot(unsigned long addr, char access_key)
> {
> int cc;
>
> asm volatile(
> - " tprot 0(%1),0\n"
> + " tprot 0(%1),0(%2)\n"
> " ipm %0\n"
> " srl %0,28\n"
> - : "=d" (cc) : "a" (addr) : "cc");
> + : "=d" (cc) : "a" (addr), "a" (access_key << 4) : "cc");
> return cc;
> }
>
> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> index 9502d16..0272249 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -217,7 +217,7 @@ void sclp_memory_setup(void)
> /* probe for r/w memory up to max memory size */
> while (ram_size < max_ram_size) {
> expect_pgm_int();
> - cc = tprot(ram_size + storage_increment_size - 1);
> + cc = tprot(ram_size + storage_increment_size - 1, 0);
> /* stop once we receive an exception or have protected memory */
> if (clear_pgm_int() || cc != 0)
> break;
> diff --git a/s390x/skrf.c b/s390x/skrf.c
> index 9488c32..e0a1007 100644
> --- a/s390x/skrf.c
> +++ b/s390x/skrf.c
> @@ -102,8 +102,7 @@ static void test_tprot(void)
> {
> report_prefix_push("tprot");
> expect_pgm_int();
> - asm volatile("tprot %[addr],0xf0(0)\n"
> - : : [addr] "a" (pagebuf) : );
> + tprot((unsigned long)pagebuf, 0xf);
> check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
> report_prefix_pop();
> }
>
^ permalink raw reply [flat|nested] 34+ messages in thread