[kvm-unit-tests PATCH 0/8] s390x: Cleanup and maintenance

[kvm-unit-tests PATCH 0/8] s390x: Cleanup and maintenance

[Janosch Frank]

A bit more cleanup and some extensions before I start adding the PV
SIE support.

https://gitlab.com/frankja/kvm-unit-tests/-/tree/lib_clean_ext

Janosch Frank (8):
  s390x: lib: Extend bitops
  lib: s390x: Add 0x3d, 0x3e and 0x3f PGM constants
  lib: s390x: Print addressing related exception information
  lib: s390x: Start using bitops instead of magic constants
  s390x: uv-host: Explain why we set up the home space and remove the
    space change
  lib: s390x: Add PSW_MASK_64
  lib: s390x: Control register constant cleanup
  lib: s390x: uv: Add rc 0x100 query error handling

 lib/s390x/asm/arch_def.h |  39 +++++++++------
 lib/s390x/asm/bitops.h   | 102 +++++++++++++++++++++++++++++++++++++++
 lib/s390x/interrupt.c    |  80 +++++++++++++++++++++++++++++-
 lib/s390x/smp.c          |   4 +-
 lib/s390x/uv.c           |   4 +-
 s390x/Makefile           |   1 +
 s390x/mvpg-sie.c         |   2 +-
 s390x/sie.c              |   2 +-
 s390x/skrf.c             |   8 +--
 s390x/uv-host.c          |  11 +++--
 10 files changed, 223 insertions(+), 30 deletions(-)

-- 
2.30.2

[kvm-unit-tests PATCH 1/8] s390x: lib: Extend bitops

[Janosch Frank]

Bit setting and clearing is never bad to have.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/bitops.h | 102 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/lib/s390x/asm/bitops.h b/lib/s390x/asm/bitops.h
index 792881ec..f5612855 100644
--- a/lib/s390x/asm/bitops.h
+++ b/lib/s390x/asm/bitops.h
@@ -17,6 +17,78 @@
 
 #define BITS_PER_LONG	64
 
+static inline unsigned long *bitops_word(unsigned long nr,
+					 const volatile unsigned long *ptr)
+{
+	unsigned long addr;
+
+	addr = (unsigned long)ptr + ((nr ^ (nr & (BITS_PER_LONG - 1))) >> 3);
+	return (unsigned long *)addr;
+}
+
+static inline unsigned long bitops_mask(unsigned long nr)
+{
+	return 1UL << (nr & (BITS_PER_LONG - 1));
+}
+
+static inline uint64_t laog(volatile unsigned long *ptr, uint64_t mask)
+{
+	uint64_t old;
+
+	/* load and or 64bit concurrent and interlocked */
+	asm volatile(
+		"	laog	%[old],%[mask],%[ptr]\n"
+		: [old] "=d" (old), [ptr] "+Q" (*ptr)
+		: [mask] "d" (mask)
+		: "memory", "cc" );
+	return old;
+}
+
+static inline uint64_t lang(volatile unsigned long *ptr, uint64_t mask)
+{
+	uint64_t old;
+
+	/* load and and 64bit concurrent and interlocked */
+	asm volatile(
+		"	lang	%[old],%[mask],%[ptr]\n"
+		: [old] "=d" (old), [ptr] "+Q" (*ptr)
+		: [mask] "d" (mask)
+		: "memory", "cc" );
+	return old;
+}
+
+static inline void set_bit(unsigned long nr,
+			   const volatile unsigned long *ptr)
+{
+	uint64_t mask = bitops_mask(nr);
+	uint64_t *addr = bitops_word(nr, ptr);
+
+	laog(addr, mask);
+}
+
+static inline void set_bit_inv(unsigned long nr,
+			       const volatile unsigned long *ptr)
+{
+	return set_bit(nr ^ (BITS_PER_LONG - 1), ptr);
+}
+
+static inline void clear_bit(unsigned long nr,
+			     const volatile unsigned long *ptr)
+{
+	uint64_t mask = bitops_mask(nr);
+	uint64_t *addr = bitops_word(nr, ptr);
+
+	lang(addr, ~mask);
+}
+
+static inline void clear_bit_inv(unsigned long nr,
+				 const volatile unsigned long *ptr)
+{
+	return clear_bit(nr ^ (BITS_PER_LONG - 1), ptr);
+}
+
+/* non-atomic bit manipulation functions */
+
 static inline bool test_bit(unsigned long nr,
 			    const volatile unsigned long *ptr)
 {
@@ -33,4 +105,34 @@ static inline bool test_bit_inv(unsigned long nr,
 	return test_bit(nr ^ (BITS_PER_LONG - 1), ptr);
 }
 
+static inline void __set_bit(unsigned long nr,
+			     const volatile unsigned long *ptr)
+{
+	uint64_t mask = bitops_mask(nr);
+	uint64_t *addr = bitops_word(nr, ptr);
+
+	*addr |= mask;
+}
+
+static inline void __set_bit_inv(unsigned long nr,
+				 const volatile unsigned long *ptr)
+{
+	return __set_bit(nr ^ (BITS_PER_LONG - 1), ptr);
+}
+
+static inline void __clear_bit(unsigned long nr,
+			       const volatile unsigned long *ptr)
+{
+	uint64_t mask = bitops_mask(nr);
+	uint64_t *addr = bitops_word(nr, ptr);
+
+	*addr &= ~mask;
+}
+
+static inline void __clear_bit_inv(unsigned long nr,
+				   const volatile unsigned long *ptr)
+{
+	return __clear_bit(nr ^ (BITS_PER_LONG - 1), ptr);
+}
+
 #endif
-- 
2.30.2

Re: [kvm-unit-tests PATCH 1/8] s390x: lib: Extend bitops

[Claudio Imbrenda]

On Fri, 13 Aug 2021 07:36:08 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Bit setting and clearing is never bad to have.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/asm/bitops.h | 102
> +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 102
> insertions(+)
> 
> diff --git a/lib/s390x/asm/bitops.h b/lib/s390x/asm/bitops.h
> index 792881ec..f5612855 100644
> --- a/lib/s390x/asm/bitops.h
> +++ b/lib/s390x/asm/bitops.h
> @@ -17,6 +17,78 @@
>  
>  #define BITS_PER_LONG	64
>  
> +static inline unsigned long *bitops_word(unsigned long nr,
> +					 const volatile unsigned
> long *ptr) +{
> +	unsigned long addr;
> +
> +	addr = (unsigned long)ptr + ((nr ^ (nr & (BITS_PER_LONG -
> 1))) >> 3);
> +	return (unsigned long *)addr;

why not just 

return ptr + (nr / BITS_PER_LONG);

> +}
> +
> +static inline unsigned long bitops_mask(unsigned long nr)
> +{
> +	return 1UL << (nr & (BITS_PER_LONG - 1));
> +}
> +
> +static inline uint64_t laog(volatile unsigned long *ptr, uint64_t
> mask) +{
> +	uint64_t old;
> +
> +	/* load and or 64bit concurrent and interlocked */
> +	asm volatile(
> +		"	laog	%[old],%[mask],%[ptr]\n"
> +		: [old] "=d" (old), [ptr] "+Q" (*ptr)
> +		: [mask] "d" (mask)
> +		: "memory", "cc" );
> +	return old;
> +}

do we really need the artillery (asm) here?
is there a reason why we can't do this in C?

> +static inline uint64_t lang(volatile unsigned long *ptr, uint64_t
> mask) +{
> +	uint64_t old;
> +
> +	/* load and and 64bit concurrent and interlocked */
> +	asm volatile(
> +		"	lang	%[old],%[mask],%[ptr]\n"
> +		: [old] "=d" (old), [ptr] "+Q" (*ptr)
> +		: [mask] "d" (mask)
> +		: "memory", "cc" );
> +	return old;
> +}

(same here as above)

> +
> +static inline void set_bit(unsigned long nr,
> +			   const volatile unsigned long *ptr)
> +{
> +	uint64_t mask = bitops_mask(nr);
> +	uint64_t *addr = bitops_word(nr, ptr);
> +
> +	laog(addr, mask);
> +}
> +
> +static inline void set_bit_inv(unsigned long nr,
> +			       const volatile unsigned long *ptr)
> +{
> +	return set_bit(nr ^ (BITS_PER_LONG - 1), ptr);
> +}
> +
> +static inline void clear_bit(unsigned long nr,
> +			     const volatile unsigned long *ptr)
> +{
> +	uint64_t mask = bitops_mask(nr);
> +	uint64_t *addr = bitops_word(nr, ptr);
> +
> +	lang(addr, ~mask);
> +}
> +
> +static inline void clear_bit_inv(unsigned long nr,
> +				 const volatile unsigned long *ptr)
> +{
> +	return clear_bit(nr ^ (BITS_PER_LONG - 1), ptr);
> +}
> +
> +/* non-atomic bit manipulation functions */
> +
>  static inline bool test_bit(unsigned long nr,
>  			    const volatile unsigned long *ptr)
>  {
> @@ -33,4 +105,34 @@ static inline bool test_bit_inv(unsigned long nr,
>  	return test_bit(nr ^ (BITS_PER_LONG - 1), ptr);
>  }
>  
> +static inline void __set_bit(unsigned long nr,
> +			     const volatile unsigned long *ptr)
> +{
> +	uint64_t mask = bitops_mask(nr);
> +	uint64_t *addr = bitops_word(nr, ptr);
> +
> +	*addr |= mask;
> +}
> +
> +static inline void __set_bit_inv(unsigned long nr,
> +				 const volatile unsigned long *ptr)
> +{
> +	return __set_bit(nr ^ (BITS_PER_LONG - 1), ptr);
> +}
> +
> +static inline void __clear_bit(unsigned long nr,
> +			       const volatile unsigned long *ptr)
> +{
> +	uint64_t mask = bitops_mask(nr);
> +	uint64_t *addr = bitops_word(nr, ptr);
> +
> +	*addr &= ~mask;
> +}
> +
> +static inline void __clear_bit_inv(unsigned long nr,
> +				   const volatile unsigned long *ptr)
> +{
> +	return __clear_bit(nr ^ (BITS_PER_LONG - 1), ptr);
> +}
> +
>  #endif

Re: [kvm-unit-tests PATCH 1/8] s390x: lib: Extend bitops

[Janosch Frank]

On 8/13/21 10:32 AM, Claudio Imbrenda wrote:
> On Fri, 13 Aug 2021 07:36:08 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Bit setting and clearing is never bad to have.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  lib/s390x/asm/bitops.h | 102
>> +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 102
>> insertions(+)
>>
>> diff --git a/lib/s390x/asm/bitops.h b/lib/s390x/asm/bitops.h
>> index 792881ec..f5612855 100644
>> --- a/lib/s390x/asm/bitops.h
>> +++ b/lib/s390x/asm/bitops.h
>> @@ -17,6 +17,78 @@
>>  
>>  #define BITS_PER_LONG	64
>>  
>> +static inline unsigned long *bitops_word(unsigned long nr,
>> +					 const volatile unsigned
>> long *ptr) +{
>> +	unsigned long addr;
>> +
>> +	addr = (unsigned long)ptr + ((nr ^ (nr & (BITS_PER_LONG -
>> 1))) >> 3);
>> +	return (unsigned long *)addr;
> 
> why not just 
> 
> return ptr + (nr / BITS_PER_LONG);
> 
>> +}
>> +
>> +static inline unsigned long bitops_mask(unsigned long nr)
>> +{
>> +	return 1UL << (nr & (BITS_PER_LONG - 1));
>> +}
>> +
>> +static inline uint64_t laog(volatile unsigned long *ptr, uint64_t
>> mask) +{
>> +	uint64_t old;
>> +
>> +	/* load and or 64bit concurrent and interlocked */
>> +	asm volatile(
>> +		"	laog	%[old],%[mask],%[ptr]\n"
>> +		: [old] "=d" (old), [ptr] "+Q" (*ptr)
>> +		: [mask] "d" (mask)
>> +		: "memory", "cc" );
>> +	return old;
>> +}
> 
> do we really need the artillery (asm) here?
> is there a reason why we can't do this in C?

Those are the interlocked/atomic instructions and even though we don't
exactly need them right now I wanted to add them for completeness.
We might be able to achieve the same via compiler functionality but this
is not my expertise. Maybe Thomas or David have a few pointers for me?

> 
>> +static inline uint64_t lang(volatile unsigned long *ptr, uint64_t
>> mask) +{
>> +	uint64_t old;
>> +
>> +	/* load and and 64bit concurrent and interlocked */
>> +	asm volatile(
>> +		"	lang	%[old],%[mask],%[ptr]\n"
>> +		: [old] "=d" (old), [ptr] "+Q" (*ptr)
>> +		: [mask] "d" (mask)
>> +		: "memory", "cc" );
>> +	return old;
>> +}
> 
> (same here as above)
> 
>> +
>> +static inline void set_bit(unsigned long nr,
>> +			   const volatile unsigned long *ptr)
>> +{
>> +	uint64_t mask = bitops_mask(nr);
>> +	uint64_t *addr = bitops_word(nr, ptr);
>> +
>> +	laog(addr, mask);
>> +}
>> +
>> +static inline void set_bit_inv(unsigned long nr,
>> +			       const volatile unsigned long *ptr)
>> +{
>> +	return set_bit(nr ^ (BITS_PER_LONG - 1), ptr);
>> +}
>> +
>> +static inline void clear_bit(unsigned long nr,
>> +			     const volatile unsigned long *ptr)
>> +{
>> +	uint64_t mask = bitops_mask(nr);
>> +	uint64_t *addr = bitops_word(nr, ptr);
>> +
>> +	lang(addr, ~mask);
>> +}
>> +
>> +static inline void clear_bit_inv(unsigned long nr,
>> +				 const volatile unsigned long *ptr)
>> +{
>> +	return clear_bit(nr ^ (BITS_PER_LONG - 1), ptr);
>> +}
>> +
>> +/* non-atomic bit manipulation functions */
>> +
>>  static inline bool test_bit(unsigned long nr,
>>  			    const volatile unsigned long *ptr)
>>  {
>> @@ -33,4 +105,34 @@ static inline bool test_bit_inv(unsigned long nr,
>>  	return test_bit(nr ^ (BITS_PER_LONG - 1), ptr);
>>  }
>>  
>> +static inline void __set_bit(unsigned long nr,
>> +			     const volatile unsigned long *ptr)
>> +{
>> +	uint64_t mask = bitops_mask(nr);
>> +	uint64_t *addr = bitops_word(nr, ptr);
>> +
>> +	*addr |= mask;
>> +}
>> +
>> +static inline void __set_bit_inv(unsigned long nr,
>> +				 const volatile unsigned long *ptr)
>> +{
>> +	return __set_bit(nr ^ (BITS_PER_LONG - 1), ptr);
>> +}
>> +
>> +static inline void __clear_bit(unsigned long nr,
>> +			       const volatile unsigned long *ptr)
>> +{
>> +	uint64_t mask = bitops_mask(nr);
>> +	uint64_t *addr = bitops_word(nr, ptr);
>> +
>> +	*addr &= ~mask;
>> +}
>> +
>> +static inline void __clear_bit_inv(unsigned long nr,
>> +				   const volatile unsigned long *ptr)
>> +{
>> +	return __clear_bit(nr ^ (BITS_PER_LONG - 1), ptr);
>> +}
>> +
>>  #endif
> 

Re: [kvm-unit-tests PATCH 1/8] s390x: lib: Extend bitops

[Thomas Huth]

On 13/08/2021 13.31, Janosch Frank wrote:
> On 8/13/21 10:32 AM, Claudio Imbrenda wrote:
>> On Fri, 13 Aug 2021 07:36:08 +0000
>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>
>>> Bit setting and clearing is never bad to have.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>   lib/s390x/asm/bitops.h | 102
>>> +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 102
>>> insertions(+)
>>>
>>> diff --git a/lib/s390x/asm/bitops.h b/lib/s390x/asm/bitops.h
>>> index 792881ec..f5612855 100644
>>> --- a/lib/s390x/asm/bitops.h
>>> +++ b/lib/s390x/asm/bitops.h
>>> @@ -17,6 +17,78 @@
>>>   
>>>   #define BITS_PER_LONG	64
>>>   
>>> +static inline unsigned long *bitops_word(unsigned long nr,
>>> +					 const volatile unsigned
>>> long *ptr) +{
>>> +	unsigned long addr;
>>> +
>>> +	addr = (unsigned long)ptr + ((nr ^ (nr & (BITS_PER_LONG -
>>> 1))) >> 3);
>>> +	return (unsigned long *)addr;
>>
>> why not just
>>
>> return ptr + (nr / BITS_PER_LONG);
>>
>>> +}
>>> +
>>> +static inline unsigned long bitops_mask(unsigned long nr)
>>> +{
>>> +	return 1UL << (nr & (BITS_PER_LONG - 1));
>>> +}
>>> +
>>> +static inline uint64_t laog(volatile unsigned long *ptr, uint64_t
>>> mask) +{
>>> +	uint64_t old;
>>> +
>>> +	/* load and or 64bit concurrent and interlocked */
>>> +	asm volatile(
>>> +		"	laog	%[old],%[mask],%[ptr]\n"
>>> +		: [old] "=d" (old), [ptr] "+Q" (*ptr)
>>> +		: [mask] "d" (mask)
>>> +		: "memory", "cc" );
>>> +	return old;
>>> +}
>>
>> do we really need the artillery (asm) here?
>> is there a reason why we can't do this in C?
> 
> Those are the interlocked/atomic instructions and even though we don't
> exactly need them right now I wanted to add them for completeness.

I think I agree with Claudio - unless we really need them, we should not 
clog the sources with arbitrary inline assembly functions.

> We might be able to achieve the same via compiler functionality but this
> is not my expertise. Maybe Thomas or David have a few pointers for me?

I'm not an expert with atomic builtins either, but what's the point of this 
at all? Loading a value and OR-ing something into the value in one go? 
What's that good for?

  Thomas

Re: [kvm-unit-tests PATCH 1/8] s390x: lib: Extend bitops

[Janosch Frank]

On 8/18/21 10:20 AM, Thomas Huth wrote:
> On 13/08/2021 13.31, Janosch Frank wrote:
>> On 8/13/21 10:32 AM, Claudio Imbrenda wrote:
>>> On Fri, 13 Aug 2021 07:36:08 +0000
>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>
>>>> Bit setting and clearing is never bad to have.
>>>>
>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>> ---
>>>>   lib/s390x/asm/bitops.h | 102
>>>> +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 102
>>>> insertions(+)
>>>>
>>>> diff --git a/lib/s390x/asm/bitops.h b/lib/s390x/asm/bitops.h
>>>> index 792881ec..f5612855 100644
>>>> --- a/lib/s390x/asm/bitops.h
>>>> +++ b/lib/s390x/asm/bitops.h
>>>> @@ -17,6 +17,78 @@
>>>>   
>>>>   #define BITS_PER_LONG	64
>>>>   
>>>> +static inline unsigned long *bitops_word(unsigned long nr,
>>>> +					 const volatile unsigned
>>>> long *ptr) +{
>>>> +	unsigned long addr;
>>>> +
>>>> +	addr = (unsigned long)ptr + ((nr ^ (nr & (BITS_PER_LONG -
>>>> 1))) >> 3);
>>>> +	return (unsigned long *)addr;
>>>
>>> why not just
>>>
>>> return ptr + (nr / BITS_PER_LONG);
>>>
>>>> +}
>>>> +
>>>> +static inline unsigned long bitops_mask(unsigned long nr)
>>>> +{
>>>> +	return 1UL << (nr & (BITS_PER_LONG - 1));
>>>> +}
>>>> +
>>>> +static inline uint64_t laog(volatile unsigned long *ptr, uint64_t
>>>> mask) +{
>>>> +	uint64_t old;
>>>> +
>>>> +	/* load and or 64bit concurrent and interlocked */
>>>> +	asm volatile(
>>>> +		"	laog	%[old],%[mask],%[ptr]\n"
>>>> +		: [old] "=d" (old), [ptr] "+Q" (*ptr)
>>>> +		: [mask] "d" (mask)
>>>> +		: "memory", "cc" );
>>>> +	return old;
>>>> +}
>>>
>>> do we really need the artillery (asm) here?
>>> is there a reason why we can't do this in C?
>>
>> Those are the interlocked/atomic instructions and even though we don't
>> exactly need them right now I wanted to add them for completeness.
> 
> I think I agree with Claudio - unless we really need them, we should not 
> clog the sources with arbitrary inline assembly functions.

Alright I can trim it down

> 
>> We might be able to achieve the same via compiler functionality but this
>> is not my expertise. Maybe Thomas or David have a few pointers for me?
> 
> I'm not an expert with atomic builtins either, but what's the point of this 
> at all? Loading a value and OR-ing something into the value in one go? 
> What's that good for?

Well it's a block-concurrent interlocked-update load, or and store.
I.e. it loads the data from the ptr and copies it into [old] then ors
the mask and stores it back to the ptr address.

The instruction name "load and or" does not represent the full actions
of the instruction.

> 
>   Thomas
> 

Re: [kvm-unit-tests PATCH 1/8] s390x: lib: Extend bitops

[Thomas Huth]

On 18/08/2021 10.39, Janosch Frank wrote:
> On 8/18/21 10:20 AM, Thomas Huth wrote:
>> On 13/08/2021 13.31, Janosch Frank wrote:
>>> On 8/13/21 10:32 AM, Claudio Imbrenda wrote:
>>>> On Fri, 13 Aug 2021 07:36:08 +0000
>>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>>
>>>>> Bit setting and clearing is never bad to have.
>>>>>
>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>>> ---
>>>>>    lib/s390x/asm/bitops.h | 102
>>>>> +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 102
>>>>> insertions(+)
>>>>>
>>>>> diff --git a/lib/s390x/asm/bitops.h b/lib/s390x/asm/bitops.h
>>>>> index 792881ec..f5612855 100644
>>>>> --- a/lib/s390x/asm/bitops.h
>>>>> +++ b/lib/s390x/asm/bitops.h
>>>>> @@ -17,6 +17,78 @@
>>>>>    
>>>>>    #define BITS_PER_LONG	64
>>>>>    
>>>>> +static inline unsigned long *bitops_word(unsigned long nr,
>>>>> +					 const volatile unsigned
>>>>> long *ptr) +{
>>>>> +	unsigned long addr;
>>>>> +
>>>>> +	addr = (unsigned long)ptr + ((nr ^ (nr & (BITS_PER_LONG -
>>>>> 1))) >> 3);
>>>>> +	return (unsigned long *)addr;
>>>>
>>>> why not just
>>>>
>>>> return ptr + (nr / BITS_PER_LONG);
>>>>
>>>>> +}
>>>>> +
>>>>> +static inline unsigned long bitops_mask(unsigned long nr)
>>>>> +{
>>>>> +	return 1UL << (nr & (BITS_PER_LONG - 1));
>>>>> +}
>>>>> +
>>>>> +static inline uint64_t laog(volatile unsigned long *ptr, uint64_t
>>>>> mask) +{
>>>>> +	uint64_t old;
>>>>> +
>>>>> +	/* load and or 64bit concurrent and interlocked */
>>>>> +	asm volatile(
>>>>> +		"	laog	%[old],%[mask],%[ptr]\n"
>>>>> +		: [old] "=d" (old), [ptr] "+Q" (*ptr)
>>>>> +		: [mask] "d" (mask)
>>>>> +		: "memory", "cc" );
>>>>> +	return old;
>>>>> +}
>>>>
>>>> do we really need the artillery (asm) here?
>>>> is there a reason why we can't do this in C?
>>>
>>> Those are the interlocked/atomic instructions and even though we don't
>>> exactly need them right now I wanted to add them for completeness.
>>
>> I think I agree with Claudio - unless we really need them, we should not
>> clog the sources with arbitrary inline assembly functions.
> 
> Alright I can trim it down
> 
>>
>>> We might be able to achieve the same via compiler functionality but this
>>> is not my expertise. Maybe Thomas or David have a few pointers for me?
>>
>> I'm not an expert with atomic builtins either, but what's the point of this
>> at all? Loading a value and OR-ing something into the value in one go?
>> What's that good for?
> 
> Well it's a block-concurrent interlocked-update load, or and store.
> I.e. it loads the data from the ptr and copies it into [old] then ors
> the mask and stores it back to the ptr address.
> 
> The instruction name "load and or" does not represent the full actions
> of the instruction.

Ok, thanks, that makes more sense now, but you could at least have mentioned 
this in the comment that you added in front of it :-)

Anyway, I guess it's easier to use the builtin atomic functions like 
__atomic_or_fetch() for stuff like this in case we ever need it.

  Thomas

[kvm-unit-tests PATCH 2/8] lib: s390x: Add 0x3d, 0x3e and 0x3f PGM constants

[Janosch Frank]

For UV and format 4 SIE tests we need to handle the following PGM exceptions:
0x3d Secure Storage Access (non-secure CPU accesses secure storage)
0x3e Non-Secure Storage Access (secure CPU accesses non-secure storage)
0x3f Mapping of secure guest is wrong

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/arch_def.h | 3 +++
 lib/s390x/interrupt.c    | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 15cf7d48..4ca02c1d 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -177,6 +177,9 @@ _Static_assert(sizeof(struct lowcore) == 0x1900, "Lowcore size");
 #define PGM_INT_CODE_REGION_FIRST_TRANS		0x39
 #define PGM_INT_CODE_REGION_SECOND_TRANS	0x3a
 #define PGM_INT_CODE_REGION_THIRD_TRANS		0x3b
+#define PGM_INT_CODE_SECURE_STOR_ACCESS		0x3d
+#define PGM_INT_CODE_NON_SECURE_STOR_ACCESS	0x3e
+#define PGM_INT_CODE_SECURE_STOR_VIOLATION	0x3f
 #define PGM_INT_CODE_MONITOR_EVENT		0x40
 #define PGM_INT_CODE_PER			0x80
 #define PGM_INT_CODE_CRYPTO_OPERATION		0x119
diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 785b7355..01ded49d 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -115,6 +115,9 @@ static void fixup_pgm_int(struct stack_frame_int *stack)
 	case PGM_INT_CODE_REGION_THIRD_TRANS:
 	case PGM_INT_CODE_PER:
 	case PGM_INT_CODE_CRYPTO_OPERATION:
+	case PGM_INT_CODE_SECURE_STOR_ACCESS:
+	case PGM_INT_CODE_NON_SECURE_STOR_ACCESS:
+	case PGM_INT_CODE_SECURE_STOR_VIOLATION:
 		/* The interrupt was nullified, the old PSW points at the
 		 * responsible instruction. Forward the PSW so we don't loop.
 		 */
-- 
2.30.2

Re: [kvm-unit-tests PATCH 2/8] lib: s390x: Add 0x3d, 0x3e and 0x3f PGM constants

[Claudio Imbrenda]

On Fri, 13 Aug 2021 07:36:09 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> For UV and format 4 SIE tests we need to handle the following PGM
> exceptions: 0x3d Secure Storage Access (non-secure CPU accesses
> secure storage) 0x3e Non-Secure Storage Access (secure CPU accesses
> non-secure storage) 0x3f Mapping of secure guest is wrong
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

> ---
>  lib/s390x/asm/arch_def.h | 3 +++
>  lib/s390x/interrupt.c    | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 15cf7d48..4ca02c1d 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -177,6 +177,9 @@ _Static_assert(sizeof(struct lowcore) == 0x1900,
> "Lowcore size"); #define PGM_INT_CODE_REGION_FIRST_TRANS
> 	0x39 #define PGM_INT_CODE_REGION_SECOND_TRANS	0x3a
>  #define PGM_INT_CODE_REGION_THIRD_TRANS		0x3b
> +#define PGM_INT_CODE_SECURE_STOR_ACCESS		0x3d
> +#define PGM_INT_CODE_NON_SECURE_STOR_ACCESS	0x3e
> +#define PGM_INT_CODE_SECURE_STOR_VIOLATION	0x3f
>  #define PGM_INT_CODE_MONITOR_EVENT		0x40
>  #define PGM_INT_CODE_PER			0x80
>  #define PGM_INT_CODE_CRYPTO_OPERATION		0x119
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 785b7355..01ded49d 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -115,6 +115,9 @@ static void fixup_pgm_int(struct stack_frame_int
> *stack) case PGM_INT_CODE_REGION_THIRD_TRANS:
>  	case PGM_INT_CODE_PER:
>  	case PGM_INT_CODE_CRYPTO_OPERATION:
> +	case PGM_INT_CODE_SECURE_STOR_ACCESS:
> +	case PGM_INT_CODE_NON_SECURE_STOR_ACCESS:
> +	case PGM_INT_CODE_SECURE_STOR_VIOLATION:
>  		/* The interrupt was nullified, the old PSW points
> at the
>  		 * responsible instruction. Forward the PSW so we
> don't loop. */

[kvm-unit-tests PATCH 3/8] lib: s390x: Print addressing related exception information

[Janosch Frank]

Right now we only get told the kind of program exception as well as
the PSW at the point where it happened.

For addressing exceptions the PSW is not always enough so let's print
the TEID which contains the failing address and flags that tell us
more about the kind of address exception.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/arch_def.h |  4 +++
 lib/s390x/interrupt.c    | 72 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 4ca02c1d..39c5ba99 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -41,6 +41,10 @@ struct psw {
 	uint64_t	addr;
 };
 
+/* Let's ignore spaces we don't expect to use for now. */
+#define AS_PRIM				0
+#define AS_HOME				3
+
 #define PSW_MASK_EXT			0x0100000000000000UL
 #define PSW_MASK_IO			0x0200000000000000UL
 #define PSW_MASK_DAT			0x0400000000000000UL
diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 01ded49d..1248bceb 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -12,6 +12,7 @@
 #include <sclp.h>
 #include <interrupt.h>
 #include <sie.h>
+#include <asm/page.h>
 
 static bool pgm_int_expected;
 static bool ext_int_expected;
@@ -126,6 +127,73 @@ static void fixup_pgm_int(struct stack_frame_int *stack)
 	/* suppressed/terminated/completed point already at the next address */
 }
 
+static void decode_pgm_prot(uint64_t teid)
+{
+	/* Low-address protection exception, 100 */
+	if (test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) && !test_bit_inv(61, &teid)) {
+		printf("Type: LAP\n");
+		return;
+	}
+
+	/* Instruction execution prevention, i.e. no-execute, 101 */
+	if (test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) && test_bit_inv(61, &teid)) {
+		printf("Type: IEP\n");
+		return;
+	}
+
+	/* Standard DAT exception, 001 */
+	if (!test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) && test_bit_inv(61, &teid)) {
+		printf("Type: DAT\n");
+		return;
+	}
+}
+
+static void decode_teid(uint64_t teid)
+{
+	int asce_id = lc->trans_exc_id & 3;
+	bool dat = lc->pgm_old_psw.mask & PSW_MASK_DAT;
+
+	printf("Memory exception information:\n");
+	printf("TEID: %lx\n", teid);
+	printf("DAT: %s\n", dat ? "on" : "off");
+	printf("AS: %s\n", asce_id == AS_PRIM ? "Primary" : "Home");
+
+	if (lc->pgm_int_code == PGM_INT_CODE_PROTECTION)
+		decode_pgm_prot(teid);
+
+	/*
+	 * If teid bit 61 is off for these two exception the reported
+	 * address is unpredictable.
+	 */
+	if ((lc->pgm_int_code == PGM_INT_CODE_SECURE_STOR_ACCESS ||
+	     lc->pgm_int_code == PGM_INT_CODE_SECURE_STOR_VIOLATION) &&
+	    !test_bit_inv(61, &teid)) {
+		printf("Address: %lx, unpredictable\n ", teid & PAGE_MASK);
+		return;
+	}
+	printf("Address: %lx\n\n", teid & PAGE_MASK);
+}
+
+static void print_storage_exception_information(void)
+{
+	switch (lc->pgm_int_code) {
+	case PGM_INT_CODE_PROTECTION:
+	case PGM_INT_CODE_PAGE_TRANSLATION:
+	case PGM_INT_CODE_SEGMENT_TRANSLATION:
+	case PGM_INT_CODE_ASCE_TYPE:
+	case PGM_INT_CODE_REGION_FIRST_TRANS:
+	case PGM_INT_CODE_REGION_SECOND_TRANS:
+	case PGM_INT_CODE_REGION_THIRD_TRANS:
+	case PGM_INT_CODE_SECURE_STOR_ACCESS:
+	case PGM_INT_CODE_NON_SECURE_STOR_ACCESS:
+	case PGM_INT_CODE_SECURE_STOR_VIOLATION:
+		decode_teid(lc->trans_exc_id);
+		break;
+	default:
+		return;
+	}
+}
+
 static void print_int_regs(struct stack_frame_int *stack)
 {
 	printf("\n");
@@ -155,6 +223,10 @@ static void print_pgm_info(struct stack_frame_int *stack)
 	       lc->pgm_int_code, stap(), lc->pgm_old_psw.addr, lc->pgm_int_id);
 	print_int_regs(stack);
 	dump_stack();
+
+	/* Dump stack doesn't end with a \n so we add it here instead */
+	printf("\n");
+	print_storage_exception_information();
 	report_summary();
 	abort();
 }
-- 
2.30.2

Re: [kvm-unit-tests PATCH 3/8] lib: s390x: Print addressing related exception information

[Claudio Imbrenda]

On Fri, 13 Aug 2021 07:36:10 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Right now we only get told the kind of program exception as well as
> the PSW at the point where it happened.
> 
> For addressing exceptions the PSW is not always enough so let's print
> the TEID which contains the failing address and flags that tell us
> more about the kind of address exception.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/asm/arch_def.h |  4 +++
>  lib/s390x/interrupt.c    | 72
> ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76
> insertions(+)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 4ca02c1d..39c5ba99 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -41,6 +41,10 @@ struct psw {
>  	uint64_t	addr;
>  };
>  
> +/* Let's ignore spaces we don't expect to use for now. */
> +#define AS_PRIM				0
> +#define AS_HOME				3
> +
>  #define PSW_MASK_EXT			0x0100000000000000UL
>  #define PSW_MASK_IO			0x0200000000000000UL
>  #define PSW_MASK_DAT			0x0400000000000000UL
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 01ded49d..1248bceb 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -12,6 +12,7 @@
>  #include <sclp.h>
>  #include <interrupt.h>
>  #include <sie.h>
> +#include <asm/page.h>
>  
>  static bool pgm_int_expected;
>  static bool ext_int_expected;
> @@ -126,6 +127,73 @@ static void fixup_pgm_int(struct stack_frame_int
> *stack) /* suppressed/terminated/completed point already at the next
> address */ }
>  
> +static void decode_pgm_prot(uint64_t teid)

it is actually more complicated than this.

if you don't want to add all the possibilities because they are
unlikely and/or not relevant, maybe add a comment

> +{
> +	/* Low-address protection exception, 100 */
> +	if (test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) &&
> !test_bit_inv(61, &teid)) {
> +		printf("Type: LAP\n");
> +		return;
> +	}
> +
> +	/* Instruction execution prevention, i.e. no-execute, 101 */
> +	if (test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) &&
> test_bit_inv(61, &teid)) {
> +		printf("Type: IEP\n");
> +		return;
> +	}
> +
> +	/* Standard DAT exception, 001 */
> +	if (!test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) &&
> test_bit_inv(61, &teid)) {
> +		printf("Type: DAT\n");
> +		return;
> +	}
> +}
> +
> +static void decode_teid(uint64_t teid)
> +{
> +	int asce_id = lc->trans_exc_id & 3;
> +	bool dat = lc->pgm_old_psw.mask & PSW_MASK_DAT;
> +
> +	printf("Memory exception information:\n");
> +	printf("TEID: %lx\n", teid);
> +	printf("DAT: %s\n", dat ? "on" : "off");
> +	printf("AS: %s\n", asce_id == AS_PRIM ? "Primary" : "Home");
> +
> +	if (lc->pgm_int_code == PGM_INT_CODE_PROTECTION)
> +		decode_pgm_prot(teid);
> +
> +	/*
> +	 * If teid bit 61 is off for these two exception the reported
> +	 * address is unpredictable.
> +	 */
> +	if ((lc->pgm_int_code == PGM_INT_CODE_SECURE_STOR_ACCESS ||
> +	     lc->pgm_int_code == PGM_INT_CODE_SECURE_STOR_VIOLATION)
> &&
> +	    !test_bit_inv(61, &teid)) {
> +		printf("Address: %lx, unpredictable\n ", teid &
> PAGE_MASK);
> +		return;
> +	}
> +	printf("Address: %lx\n\n", teid & PAGE_MASK);
> +}
> +
> +static void print_storage_exception_information(void)
> +{
> +	switch (lc->pgm_int_code) {
> +	case PGM_INT_CODE_PROTECTION:
> +	case PGM_INT_CODE_PAGE_TRANSLATION:
> +	case PGM_INT_CODE_SEGMENT_TRANSLATION:
> +	case PGM_INT_CODE_ASCE_TYPE:
> +	case PGM_INT_CODE_REGION_FIRST_TRANS:
> +	case PGM_INT_CODE_REGION_SECOND_TRANS:
> +	case PGM_INT_CODE_REGION_THIRD_TRANS:
> +	case PGM_INT_CODE_SECURE_STOR_ACCESS:
> +	case PGM_INT_CODE_NON_SECURE_STOR_ACCESS:
> +	case PGM_INT_CODE_SECURE_STOR_VIOLATION:
> +		decode_teid(lc->trans_exc_id);
> +		break;
> +	default:
> +		return;
> +	}
> +}
> +
>  static void print_int_regs(struct stack_frame_int *stack)
>  {
>  	printf("\n");
> @@ -155,6 +223,10 @@ static void print_pgm_info(struct
> stack_frame_int *stack) lc->pgm_int_code, stap(),
> lc->pgm_old_psw.addr, lc->pgm_int_id); print_int_regs(stack);
>  	dump_stack();
> +
> +	/* Dump stack doesn't end with a \n so we add it here
> instead */
> +	printf("\n");
> +	print_storage_exception_information();
>  	report_summary();
>  	abort();
>  }

Re: [kvm-unit-tests PATCH 3/8] lib: s390x: Print addressing related exception information

[Janosch Frank]

On 8/13/21 10:40 AM, Claudio Imbrenda wrote:
> On Fri, 13 Aug 2021 07:36:10 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Right now we only get told the kind of program exception as well as
>> the PSW at the point where it happened.
>>
>> For addressing exceptions the PSW is not always enough so let's print
>> the TEID which contains the failing address and flags that tell us
>> more about the kind of address exception.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  lib/s390x/asm/arch_def.h |  4 +++
>>  lib/s390x/interrupt.c    | 72
>> ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76
>> insertions(+)
>>
>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>> index 4ca02c1d..39c5ba99 100644
>> --- a/lib/s390x/asm/arch_def.h
>> +++ b/lib/s390x/asm/arch_def.h
>> @@ -41,6 +41,10 @@ struct psw {
>>  	uint64_t	addr;
>>  };
>>  
>> +/* Let's ignore spaces we don't expect to use for now. */
>> +#define AS_PRIM				0
>> +#define AS_HOME				3
>> +
>>  #define PSW_MASK_EXT			0x0100000000000000UL
>>  #define PSW_MASK_IO			0x0200000000000000UL
>>  #define PSW_MASK_DAT			0x0400000000000000UL
>> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
>> index 01ded49d..1248bceb 100644
>> --- a/lib/s390x/interrupt.c
>> +++ b/lib/s390x/interrupt.c
>> @@ -12,6 +12,7 @@
>>  #include <sclp.h>
>>  #include <interrupt.h>
>>  #include <sie.h>
>> +#include <asm/page.h>
>>  
>>  static bool pgm_int_expected;
>>  static bool ext_int_expected;
>> @@ -126,6 +127,73 @@ static void fixup_pgm_int(struct stack_frame_int
>> *stack) /* suppressed/terminated/completed point already at the next
>> address */ }
>>  
>> +static void decode_pgm_prot(uint64_t teid)
> 
> it is actually more complicated than this.

I know it hurts to look at the spec :)

> 
> if you don't want to add all the possibilities because they are
> unlikely and/or not relevant, maybe add a comment

Will do!

> 
>> +{
>> +	/* Low-address protection exception, 100 */
>> +	if (test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) &&
>> !test_bit_inv(61, &teid)) {
>> +		printf("Type: LAP\n");
>> +		return;
>> +	}
>> +
>> +	/* Instruction execution prevention, i.e. no-execute, 101 */
>> +	if (test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) &&
>> test_bit_inv(61, &teid)) {
>> +		printf("Type: IEP\n");
>> +		return;
>> +	}
>> +
>> +	/* Standard DAT exception, 001 */
>> +	if (!test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) &&
>> test_bit_inv(61, &teid)) {
>> +		printf("Type: DAT\n");
>> +		return;
>> +	}
>> +}
>> +
>> +static void decode_teid(uint64_t teid)
>> +{
>> +	int asce_id = lc->trans_exc_id & 3;
>> +	bool dat = lc->pgm_old_psw.mask & PSW_MASK_DAT;
>> +
>> +	printf("Memory exception information:\n");
>> +	printf("TEID: %lx\n", teid);
>> +	printf("DAT: %s\n", dat ? "on" : "off");
>> +	printf("AS: %s\n", asce_id == AS_PRIM ? "Primary" : "Home");
>> +
>> +	if (lc->pgm_int_code == PGM_INT_CODE_PROTECTION)
>> +		decode_pgm_prot(teid);
>> +
>> +	/*
>> +	 * If teid bit 61 is off for these two exception the reported
>> +	 * address is unpredictable.
>> +	 */
>> +	if ((lc->pgm_int_code == PGM_INT_CODE_SECURE_STOR_ACCESS ||
>> +	     lc->pgm_int_code == PGM_INT_CODE_SECURE_STOR_VIOLATION)
>> &&
>> +	    !test_bit_inv(61, &teid)) {
>> +		printf("Address: %lx, unpredictable\n ", teid &
>> PAGE_MASK);
>> +		return;
>> +	}
>> +	printf("Address: %lx\n\n", teid & PAGE_MASK);
>> +}
>> +
>> +static void print_storage_exception_information(void)
>> +{
>> +	switch (lc->pgm_int_code) {
>> +	case PGM_INT_CODE_PROTECTION:
>> +	case PGM_INT_CODE_PAGE_TRANSLATION:
>> +	case PGM_INT_CODE_SEGMENT_TRANSLATION:
>> +	case PGM_INT_CODE_ASCE_TYPE:
>> +	case PGM_INT_CODE_REGION_FIRST_TRANS:
>> +	case PGM_INT_CODE_REGION_SECOND_TRANS:
>> +	case PGM_INT_CODE_REGION_THIRD_TRANS:
>> +	case PGM_INT_CODE_SECURE_STOR_ACCESS:
>> +	case PGM_INT_CODE_NON_SECURE_STOR_ACCESS:
>> +	case PGM_INT_CODE_SECURE_STOR_VIOLATION:
>> +		decode_teid(lc->trans_exc_id);
>> +		break;
>> +	default:
>> +		return;
>> +	}
>> +}
>> +
>>  static void print_int_regs(struct stack_frame_int *stack)
>>  {
>>  	printf("\n");
>> @@ -155,6 +223,10 @@ static void print_pgm_info(struct
>> stack_frame_int *stack) lc->pgm_int_code, stap(),
>> lc->pgm_old_psw.addr, lc->pgm_int_id); print_int_regs(stack);
>>  	dump_stack();
>> +
>> +	/* Dump stack doesn't end with a \n so we add it here
>> instead */
>> +	printf("\n");
>> +	print_storage_exception_information();
>>  	report_summary();
>>  	abort();
>>  }
> 

Re: [kvm-unit-tests PATCH 3/8] lib: s390x: Print addressing related exception information

[Thomas Huth]

On 13/08/2021 09.36, Janosch Frank wrote:
> Right now we only get told the kind of program exception as well as
> the PSW at the point where it happened.
> 
> For addressing exceptions the PSW is not always enough so let's print
> the TEID which contains the failing address and flags that tell us
> more about the kind of address exception.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   lib/s390x/asm/arch_def.h |  4 +++
>   lib/s390x/interrupt.c    | 72 ++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 76 insertions(+)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 4ca02c1d..39c5ba99 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -41,6 +41,10 @@ struct psw {
>   	uint64_t	addr;
>   };
>   
> +/* Let's ignore spaces we don't expect to use for now. */
> +#define AS_PRIM				0
> +#define AS_HOME				3
> +
>   #define PSW_MASK_EXT			0x0100000000000000UL
>   #define PSW_MASK_IO			0x0200000000000000UL
>   #define PSW_MASK_DAT			0x0400000000000000UL
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 01ded49d..1248bceb 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -12,6 +12,7 @@
>   #include <sclp.h>
>   #include <interrupt.h>
>   #include <sie.h>
> +#include <asm/page.h>
>   
>   static bool pgm_int_expected;
>   static bool ext_int_expected;
> @@ -126,6 +127,73 @@ static void fixup_pgm_int(struct stack_frame_int *stack)
>   	/* suppressed/terminated/completed point already at the next address */
>   }
>   
> +static void decode_pgm_prot(uint64_t teid)
> +{
> +	/* Low-address protection exception, 100 */
> +	if (test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) && !test_bit_inv(61, &teid)) {

Likely just a matter of taste, but I'd prefer something like:

	if ((teid & 0x8c) == 0x80) {

> +		printf("Type: LAP\n");
> +		return;
> +	}
> +
> +	/* Instruction execution prevention, i.e. no-execute, 101 */
> +	if (test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) && test_bit_inv(61, &teid)) {
> +		printf("Type: IEP\n");
> +		return;
> +	}
> +
> +	/* Standard DAT exception, 001 */
> +	if (!test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) && test_bit_inv(61, &teid)) {
> +		printf("Type: DAT\n");
> +		return;
> +	}

What about 010 (key controlled protection) and 011 (access-list controlled 
protection)? Even if we do not trigger those yet, it might make sense to add 
them right from the start, too?

> +}
> +
> +static void decode_teid(uint64_t teid)
> +{
> +	int asce_id = lc->trans_exc_id & 3;

Why are you referencing the lc->trans_exc_id here again? It's already passed 
as "teid" parameter.

> +	bool dat = lc->pgm_old_psw.mask & PSW_MASK_DAT;
> +
> +	printf("Memory exception information:\n");
> +	printf("TEID: %lx\n", teid);
> +	printf("DAT: %s\n", dat ? "on" : "off");
> +	printf("AS: %s\n", asce_id == AS_PRIM ? "Primary" : "Home");

Could "secondary" or "AR" mode really never happen here? I'd rather like to 
see a switch-case statement here that is able to print all four modes, just 
to avoid confusion.

> +	if (lc->pgm_int_code == PGM_INT_CODE_PROTECTION)
> +		decode_pgm_prot(teid);
> +
> +	/*
> +	 * If teid bit 61 is off for these two exception the reported
> +	 * address is unpredictable.
> +	 */
> +	if ((lc->pgm_int_code == PGM_INT_CODE_SECURE_STOR_ACCESS ||
> +	     lc->pgm_int_code == PGM_INT_CODE_SECURE_STOR_VIOLATION) &&
> +	    !test_bit_inv(61, &teid)) {
> +		printf("Address: %lx, unpredictable\n ", teid & PAGE_MASK);
> +		return;
> +	}
> +	printf("Address: %lx\n\n", teid & PAGE_MASK);
> +}
> +
> +static void print_storage_exception_information(void)
> +{
> +	switch (lc->pgm_int_code) {
> +	case PGM_INT_CODE_PROTECTION:
> +	case PGM_INT_CODE_PAGE_TRANSLATION:
> +	case PGM_INT_CODE_SEGMENT_TRANSLATION:
> +	case PGM_INT_CODE_ASCE_TYPE:
> +	case PGM_INT_CODE_REGION_FIRST_TRANS:
> +	case PGM_INT_CODE_REGION_SECOND_TRANS:
> +	case PGM_INT_CODE_REGION_THIRD_TRANS:
> +	case PGM_INT_CODE_SECURE_STOR_ACCESS:
> +	case PGM_INT_CODE_NON_SECURE_STOR_ACCESS:
> +	case PGM_INT_CODE_SECURE_STOR_VIOLATION:
> +		decode_teid(lc->trans_exc_id);
> +		break;
> +	default:
> +		return;

I think you could drop that default case.

> +	}
> +}
> +
>   static void print_int_regs(struct stack_frame_int *stack)
>   {
>   	printf("\n");
> @@ -155,6 +223,10 @@ static void print_pgm_info(struct stack_frame_int *stack)
>   	       lc->pgm_int_code, stap(), lc->pgm_old_psw.addr, lc->pgm_int_id);
>   	print_int_regs(stack);
>   	dump_stack();
> +
> +	/* Dump stack doesn't end with a \n so we add it here instead */
> +	printf("\n");
> +	print_storage_exception_information();
>   	report_summary();
>   	abort();
>   }
> 

Re: [kvm-unit-tests PATCH 3/8] lib: s390x: Print addressing related exception information

[Claudio Imbrenda]

On Wed, 18 Aug 2021 11:12:57 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 13/08/2021 09.36, Janosch Frank wrote:
> > Right now we only get told the kind of program exception as well as
> > the PSW at the point where it happened.
> > 
> > For addressing exceptions the PSW is not always enough so let's
> > print the TEID which contains the failing address and flags that
> > tell us more about the kind of address exception.
> > 
> > Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> > ---
> >   lib/s390x/asm/arch_def.h |  4 +++
> >   lib/s390x/interrupt.c    | 72
> > ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76
> > insertions(+)
> > 
> > diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> > index 4ca02c1d..39c5ba99 100644
> > --- a/lib/s390x/asm/arch_def.h
> > +++ b/lib/s390x/asm/arch_def.h
> > @@ -41,6 +41,10 @@ struct psw {
> >   	uint64_t	addr;
> >   };
> >   
> > +/* Let's ignore spaces we don't expect to use for now. */
> > +#define AS_PRIM				0
> > +#define AS_HOME				3
> > +
> >   #define PSW_MASK_EXT			0x0100000000000000UL
> >   #define PSW_MASK_IO			0x0200000000000000UL
> >   #define PSW_MASK_DAT			0x0400000000000000UL
> > diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> > index 01ded49d..1248bceb 100644
> > --- a/lib/s390x/interrupt.c
> > +++ b/lib/s390x/interrupt.c
> > @@ -12,6 +12,7 @@
> >   #include <sclp.h>
> >   #include <interrupt.h>
> >   #include <sie.h>
> > +#include <asm/page.h>
> >   
> >   static bool pgm_int_expected;
> >   static bool ext_int_expected;
> > @@ -126,6 +127,73 @@ static void fixup_pgm_int(struct
> > stack_frame_int *stack) /* suppressed/terminated/completed point
> > already at the next address */ }
> >   
> > +static void decode_pgm_prot(uint64_t teid)
> > +{
> > +	/* Low-address protection exception, 100 */
> > +	if (test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) &&
> > !test_bit_inv(61, &teid)) {  
> 
> Likely just a matter of taste, but I'd prefer something like:
> 
> 	if ((teid & 0x8c) == 0x80) {

or even better:

	switch (teid & TEID_MASK) {

> 
> > +		printf("Type: LAP\n");
> > +		return;
> > +	}
> > +
> > +	/* Instruction execution prevention, i.e. no-execute, 101
> > */
> > +	if (test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) &&
> > test_bit_inv(61, &teid)) {
> > +		printf("Type: IEP\n");
> > +		return;
> > +	}
> > +
> > +	/* Standard DAT exception, 001 */
> > +	if (!test_bit_inv(56, &teid) && !test_bit_inv(60, &teid)
> > && test_bit_inv(61, &teid)) {
> > +		printf("Type: DAT\n");
> > +		return;
> > +	}  
> 
> What about 010 (key controlled protection) and 011 (access-list
> controlled protection)? Even if we do not trigger those yet, it might
> make sense to add them right from the start, too?
> 
> > +}
> > +
> > +static void decode_teid(uint64_t teid)
> > +{
> > +	int asce_id = lc->trans_exc_id & 3;  
> 
> Why are you referencing the lc->trans_exc_id here again? It's already
> passed as "teid" parameter.
> 
> > +	bool dat = lc->pgm_old_psw.mask & PSW_MASK_DAT;
> > +
> > +	printf("Memory exception information:\n");
> > +	printf("TEID: %lx\n", teid);
> > +	printf("DAT: %s\n", dat ? "on" : "off");
> > +	printf("AS: %s\n", asce_id == AS_PRIM ? "Primary" :
> > "Home");  
> 
> Could "secondary" or "AR" mode really never happen here? I'd rather
> like to see a switch-case statement here that is able to print all
> four modes, just to avoid confusion.
> 
> > +	if (lc->pgm_int_code == PGM_INT_CODE_PROTECTION)
> > +		decode_pgm_prot(teid);
> > +
> > +	/*
> > +	 * If teid bit 61 is off for these two exception the
> > reported
> > +	 * address is unpredictable.
> > +	 */
> > +	if ((lc->pgm_int_code == PGM_INT_CODE_SECURE_STOR_ACCESS ||
> > +	     lc->pgm_int_code ==
> > PGM_INT_CODE_SECURE_STOR_VIOLATION) &&
> > +	    !test_bit_inv(61, &teid)) {
> > +		printf("Address: %lx, unpredictable\n ", teid &
> > PAGE_MASK);
> > +		return;
> > +	}
> > +	printf("Address: %lx\n\n", teid & PAGE_MASK);
> > +}
> > +
> > +static void print_storage_exception_information(void)
> > +{
> > +	switch (lc->pgm_int_code) {
> > +	case PGM_INT_CODE_PROTECTION:
> > +	case PGM_INT_CODE_PAGE_TRANSLATION:
> > +	case PGM_INT_CODE_SEGMENT_TRANSLATION:
> > +	case PGM_INT_CODE_ASCE_TYPE:
> > +	case PGM_INT_CODE_REGION_FIRST_TRANS:
> > +	case PGM_INT_CODE_REGION_SECOND_TRANS:
> > +	case PGM_INT_CODE_REGION_THIRD_TRANS:
> > +	case PGM_INT_CODE_SECURE_STOR_ACCESS:
> > +	case PGM_INT_CODE_NON_SECURE_STOR_ACCESS:
> > +	case PGM_INT_CODE_SECURE_STOR_VIOLATION:
> > +		decode_teid(lc->trans_exc_id);
> > +		break;
> > +	default:
> > +		return;  
> 
> I think you could drop that default case.
> 
> > +	}
> > +}
> > +
> >   static void print_int_regs(struct stack_frame_int *stack)
> >   {
> >   	printf("\n");
> > @@ -155,6 +223,10 @@ static void print_pgm_info(struct
> > stack_frame_int *stack) lc->pgm_int_code, stap(),
> > lc->pgm_old_psw.addr, lc->pgm_int_id); print_int_regs(stack);
> >   	dump_stack();
> > +
> > +	/* Dump stack doesn't end with a \n so we add it here
> > instead */
> > +	printf("\n");
> > +	print_storage_exception_information();
> >   	report_summary();
> >   	abort();
> >   }
> >   
> 

Re: [kvm-unit-tests PATCH 3/8] lib: s390x: Print addressing related exception information

[Janosch Frank]

On 8/18/21 11:12 AM, Thomas Huth wrote:
> On 13/08/2021 09.36, Janosch Frank wrote:
>> Right now we only get told the kind of program exception as well as
>> the PSW at the point where it happened.
>>
>> For addressing exceptions the PSW is not always enough so let's print
>> the TEID which contains the failing address and flags that tell us
>> more about the kind of address exception.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   lib/s390x/asm/arch_def.h |  4 +++
>>   lib/s390x/interrupt.c    | 72 ++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 76 insertions(+)
>>
>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>> index 4ca02c1d..39c5ba99 100644
>> --- a/lib/s390x/asm/arch_def.h
>> +++ b/lib/s390x/asm/arch_def.h
>> @@ -41,6 +41,10 @@ struct psw {
>>   	uint64_t	addr;
>>   };
>>   
>> +/* Let's ignore spaces we don't expect to use for now. */
>> +#define AS_PRIM				0
>> +#define AS_HOME				3
>> +
>>   #define PSW_MASK_EXT			0x0100000000000000UL
>>   #define PSW_MASK_IO			0x0200000000000000UL
>>   #define PSW_MASK_DAT			0x0400000000000000UL
>> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
>> index 01ded49d..1248bceb 100644
>> --- a/lib/s390x/interrupt.c
>> +++ b/lib/s390x/interrupt.c
>> @@ -12,6 +12,7 @@
>>   #include <sclp.h>
>>   #include <interrupt.h>
>>   #include <sie.h>
>> +#include <asm/page.h>
>>   
>>   static bool pgm_int_expected;
>>   static bool ext_int_expected;
>> @@ -126,6 +127,73 @@ static void fixup_pgm_int(struct stack_frame_int *stack)
>>   	/* suppressed/terminated/completed point already at the next address */
>>   }
>>   
>> +static void decode_pgm_prot(uint64_t teid)
>> +{
>> +	/* Low-address protection exception, 100 */
>> +	if (test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) && !test_bit_inv(61, &teid)) {
> 
> Likely just a matter of taste, but I'd prefer something like:
> 
> 	if ((teid & 0x8c) == 0x80) {

The POP states these as bits when you have a look at the ESOP section
and I'd like to keep it the same here for easier comparison.

The test_bits() are as explicit as it gets and I value that.

> 
>> +		printf("Type: LAP\n");
>> +		return;
>> +	}
>> +
>> +	/* Instruction execution prevention, i.e. no-execute, 101 */
>> +	if (test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) && test_bit_inv(61, &teid)) {
>> +		printf("Type: IEP\n");
>> +		return;
>> +	}
>> +
>> +	/* Standard DAT exception, 001 */
>> +	if (!test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) && test_bit_inv(61, &teid)) {
>> +		printf("Type: DAT\n");
>> +		return;
>> +	}
> 
> What about 010 (key controlled protection) and 011 (access-list controlled 
> protection)? Even if we do not trigger those yet, it might make sense to add 
> them right from the start, too?

If I do that then I can start a whole new file "fault.c" and move these
changes there (which I'll do now anyway). My intentions were a small
change that covers 90% of our current exceptions (especially PV
exceptions) to make my life easier in LPAR.

If people add skey/ar code they can also add the decoding here, no? :-)

> 
>> +}
>> +
>> +static void decode_teid(uint64_t teid)
>> +{
>> +	int asce_id = lc->trans_exc_id & 3;
> 
> Why are you referencing the lc->trans_exc_id here again? It's already passed 
> as "teid" parameter.

Forgot to remove that

> 
>> +	bool dat = lc->pgm_old_psw.mask & PSW_MASK_DAT;
>> +
>> +	printf("Memory exception information:\n");
>> +	printf("TEID: %lx\n", teid);
>> +	printf("DAT: %s\n", dat ? "on" : "off");
>> +	printf("AS: %s\n", asce_id == AS_PRIM ? "Primary" : "Home");
> 
> Could "secondary" or "AR" mode really never happen here? I'd rather like to 
> see a switch-case statement here that is able to print all four modes, just 
> to avoid confusion.

Right now we ONLY use primary space.

> 
>> +	if (lc->pgm_int_code == PGM_INT_CODE_PROTECTION)
>> +		decode_pgm_prot(teid);
>> +
>> +	/*
>> +	 * If teid bit 61 is off for these two exception the reported
>> +	 * address is unpredictable.
>> +	 */
>> +	if ((lc->pgm_int_code == PGM_INT_CODE_SECURE_STOR_ACCESS ||
>> +	     lc->pgm_int_code == PGM_INT_CODE_SECURE_STOR_VIOLATION) &&
>> +	    !test_bit_inv(61, &teid)) {
>> +		printf("Address: %lx, unpredictable\n ", teid & PAGE_MASK);
>> +		return;
>> +	}
>> +	printf("Address: %lx\n\n", teid & PAGE_MASK);
>> +}
>> +
>> +static void print_storage_exception_information(void)
>> +{
>> +	switch (lc->pgm_int_code) {
>> +	case PGM_INT_CODE_PROTECTION:
>> +	case PGM_INT_CODE_PAGE_TRANSLATION:
>> +	case PGM_INT_CODE_SEGMENT_TRANSLATION:
>> +	case PGM_INT_CODE_ASCE_TYPE:
>> +	case PGM_INT_CODE_REGION_FIRST_TRANS:
>> +	case PGM_INT_CODE_REGION_SECOND_TRANS:
>> +	case PGM_INT_CODE_REGION_THIRD_TRANS:
>> +	case PGM_INT_CODE_SECURE_STOR_ACCESS:
>> +	case PGM_INT_CODE_NON_SECURE_STOR_ACCESS:
>> +	case PGM_INT_CODE_SECURE_STOR_VIOLATION:
>> +		decode_teid(lc->trans_exc_id);
>> +		break;
>> +	default:
>> +		return;
> 
> I think you could drop that default case.

Yes

> 
>> +	}
>> +}
>> +
>>   static void print_int_regs(struct stack_frame_int *stack)
>>   {
>>   	printf("\n");
>> @@ -155,6 +223,10 @@ static void print_pgm_info(struct stack_frame_int *stack)
>>   	       lc->pgm_int_code, stap(), lc->pgm_old_psw.addr, lc->pgm_int_id);
>>   	print_int_regs(stack);
>>   	dump_stack();
>> +
>> +	/* Dump stack doesn't end with a \n so we add it here instead */
>> +	printf("\n");
>> +	print_storage_exception_information();
>>   	report_summary();
>>   	abort();
>>   }
>>
> 

[kvm-unit-tests PATCH 4/8] lib: s390x: Start using bitops instead of magic constants

[Janosch Frank]

TEID data is specified in the Principles of Operation as bits so it
makes more sens to test the bits instead of anding the mask.

We need to set -Wno-address-of-packed-member since for test bit we
take an address of a struct lowcore member.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/interrupt.c | 5 +++--
 s390x/Makefile        | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 1248bceb..e05c212e 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -8,6 +8,7 @@
  *  David Hildenbrand <david@redhat.com>
  */
 #include <libcflat.h>
+#include <bitops.h>
 #include <asm/barrier.h>
 #include <sclp.h>
 #include <interrupt.h>
@@ -77,8 +78,8 @@ static void fixup_pgm_int(struct stack_frame_int *stack)
 		break;
 	case PGM_INT_CODE_PROTECTION:
 		/* Handling for iep.c test case. */
-		if (lc->trans_exc_id & 0x80UL && lc->trans_exc_id & 0x04UL &&
-		    !(lc->trans_exc_id & 0x08UL))
+		if (test_bit_inv(56, &lc->trans_exc_id) && test_bit_inv(61, &lc->trans_exc_id) &&
+		    !test_bit_inv(60, &lc->trans_exc_id))
 			/*
 			 * We branched to the instruction that caused
 			 * the exception so we can use the return
diff --git a/s390x/Makefile b/s390x/Makefile
index ef8041a6..d260b336 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -45,6 +45,7 @@ CFLAGS += -O2
 CFLAGS += -march=zEC12
 CFLAGS += -mbackchain
 CFLAGS += -fno-delete-null-pointer-checks
+CFLAGS += -Wno-address-of-packed-member
 LDFLAGS += -nostdlib -Wl,--build-id=none
 
 # We want to keep intermediate files
-- 
2.30.2

Re: [kvm-unit-tests PATCH 4/8] lib: s390x: Start using bitops instead of magic constants

[Claudio Imbrenda]

On Fri, 13 Aug 2021 07:36:11 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> TEID data is specified in the Principles of Operation as bits so it
> makes more sens to test the bits instead of anding the mask.
> 
> We need to set -Wno-address-of-packed-member since for test bit we
> take an address of a struct lowcore member.

and s390x has no alignment requirements

> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

> ---
>  lib/s390x/interrupt.c | 5 +++--
>  s390x/Makefile        | 1 +
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 1248bceb..e05c212e 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -8,6 +8,7 @@
>   *  David Hildenbrand <david@redhat.com>
>   */
>  #include <libcflat.h>
> +#include <bitops.h>
>  #include <asm/barrier.h>
>  #include <sclp.h>
>  #include <interrupt.h>
> @@ -77,8 +78,8 @@ static void fixup_pgm_int(struct stack_frame_int
> *stack) break;
>  	case PGM_INT_CODE_PROTECTION:
>  		/* Handling for iep.c test case. */
> -		if (lc->trans_exc_id & 0x80UL && lc->trans_exc_id &
> 0x04UL &&
> -		    !(lc->trans_exc_id & 0x08UL))
> +		if (test_bit_inv(56, &lc->trans_exc_id) &&
> test_bit_inv(61, &lc->trans_exc_id) &&
> +		    !test_bit_inv(60, &lc->trans_exc_id))
>  			/*
>  			 * We branched to the instruction that caused
>  			 * the exception so we can use the return
> diff --git a/s390x/Makefile b/s390x/Makefile
> index ef8041a6..d260b336 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -45,6 +45,7 @@ CFLAGS += -O2
>  CFLAGS += -march=zEC12
>  CFLAGS += -mbackchain
>  CFLAGS += -fno-delete-null-pointer-checks
> +CFLAGS += -Wno-address-of-packed-member
>  LDFLAGS += -nostdlib -Wl,--build-id=none
>  
>  # We want to keep intermediate files

Re: [kvm-unit-tests PATCH 4/8] lib: s390x: Start using bitops instead of magic constants

[Thomas Huth]

On 13/08/2021 09.36, Janosch Frank wrote:
> TEID data is specified in the Principles of Operation as bits so it
> makes more sens to test the bits instead of anding the mask.
> 
> We need to set -Wno-address-of-packed-member since for test bit we
> take an address of a struct lowcore member.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   lib/s390x/interrupt.c | 5 +++--
>   s390x/Makefile        | 1 +
>   2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 1248bceb..e05c212e 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -8,6 +8,7 @@
>    *  David Hildenbrand <david@redhat.com>
>    */
>   #include <libcflat.h>
> +#include <bitops.h>
>   #include <asm/barrier.h>
>   #include <sclp.h>
>   #include <interrupt.h>
> @@ -77,8 +78,8 @@ static void fixup_pgm_int(struct stack_frame_int *stack)
>   		break;
>   	case PGM_INT_CODE_PROTECTION:
>   		/* Handling for iep.c test case. */
> -		if (lc->trans_exc_id & 0x80UL && lc->trans_exc_id & 0x04UL &&
> -		    !(lc->trans_exc_id & 0x08UL))
> +		if (test_bit_inv(56, &lc->trans_exc_id) && test_bit_inv(61, &lc->trans_exc_id) &&
> +		    !test_bit_inv(60, &lc->trans_exc_id))

I'd rather prefer:

	if ((lc->trans_exc_id & 0x8c) == 0x84)

... or maybe you could add a helper function for these checks a la:

bool check_teid_prot_cause(uint64_t teid, bool bit56, bool bit60,
                            bool bit61)

then you could replace the if statement with:

	if (check_teid_prot_cause(lc->trans_exc_id, 1, 0, 1))

which would be way more readable, IMHO.

>   			/*
>   			 * We branched to the instruction that caused
>   			 * the exception so we can use the return
> diff --git a/s390x/Makefile b/s390x/Makefile
> index ef8041a6..d260b336 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -45,6 +45,7 @@ CFLAGS += -O2
>   CFLAGS += -march=zEC12
>   CFLAGS += -mbackchain
>   CFLAGS += -fno-delete-null-pointer-checks
> +CFLAGS += -Wno-address-of-packed-member

I think we should avoid this since this also affects the common code, 
doesn't it? And in common code, we might need to deal with this.

  Thomas

[kvm-unit-tests PATCH 5/8] s390x: uv-host: Explain why we set up the home space and remove the space change

[Janosch Frank]

UV home addresses don't require us to be in home space but we need to
have it set up so hw/fw can use the home asce to translate home
virtual addresses.

Hence we add a comment why we're setting up the home asce and remove
the address space since it's unneeded.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/uv-host.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 426a67f6..28035707 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -444,13 +444,18 @@ static void test_clear(void)
 
 static void setup_vmem(void)
 {
-	uint64_t asce, mask;
+	uint64_t asce;
 
 	setup_mmu(get_max_ram_size(), NULL);
+	/*
+	 * setup_mmu() will enable DAT and set the primary address
+	 * space but we need to have a valid home space since UV calls
+	 * take home space virtual addresses.
+	 *
+	 * Hence we just copy the primary asce into the home space.
+	 */
 	asce = stctg(1);
 	lctlg(13, asce);
-	mask = extract_psw_mask() | 0x0000C00000000000UL;
-	load_psw_mask(mask);
 }
 
 int main(void)
-- 
2.30.2

Re: [kvm-unit-tests PATCH 5/8] s390x: uv-host: Explain why we set up the home space and remove the space change

[Claudio Imbrenda]

On Fri, 13 Aug 2021 07:36:12 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> UV home addresses don't require us to be in home space but we need to
> have it set up so hw/fw can use the home asce to translate home
> virtual addresses.
> 
> Hence we add a comment why we're setting up the home asce and remove
> the address space since it's unneeded.

oh, we actually never use it?

> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

> ---
>  s390x/uv-host.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index 426a67f6..28035707 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -444,13 +444,18 @@ static void test_clear(void)
>  
>  static void setup_vmem(void)
>  {
> -	uint64_t asce, mask;
> +	uint64_t asce;
>  
>  	setup_mmu(get_max_ram_size(), NULL);
> +	/*
> +	 * setup_mmu() will enable DAT and set the primary address
> +	 * space but we need to have a valid home space since UV
> calls
> +	 * take home space virtual addresses.
> +	 *
> +	 * Hence we just copy the primary asce into the home space.
> +	 */
>  	asce = stctg(1);
>  	lctlg(13, asce);
> -	mask = extract_psw_mask() | 0x0000C00000000000UL;
> -	load_psw_mask(mask);
>  }
>  
>  int main(void)

Re: [kvm-unit-tests PATCH 5/8] s390x: uv-host: Explain why we set up the home space and remove the space change

[Janosch Frank]

On 8/13/21 10:45 AM, Claudio Imbrenda wrote:
> On Fri, 13 Aug 2021 07:36:12 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> UV home addresses don't require us to be in home space but we need to
>> have it set up so hw/fw can use the home asce to translate home
>> virtual addresses.
>>
>> Hence we add a comment why we're setting up the home asce and remove
>> the address space since it's unneeded.
> 
> oh, we actually never use it?

Yes, as I said, those addresses are not relative to your PSW DAT
settings, they are defined to be home space addresses.

>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> 
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Thanks!

> 
>> ---
>>  s390x/uv-host.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
>> index 426a67f6..28035707 100644
>> --- a/s390x/uv-host.c
>> +++ b/s390x/uv-host.c
>> @@ -444,13 +444,18 @@ static void test_clear(void)
>>  
>>  static void setup_vmem(void)
>>  {
>> -	uint64_t asce, mask;
>> +	uint64_t asce;
>>  
>>  	setup_mmu(get_max_ram_size(), NULL);
>> +	/*
>> +	 * setup_mmu() will enable DAT and set the primary address
>> +	 * space but we need to have a valid home space since UV
>> calls
>> +	 * take home space virtual addresses.
>> +	 *
>> +	 * Hence we just copy the primary asce into the home space.
>> +	 */
>>  	asce = stctg(1);
>>  	lctlg(13, asce);
>> -	mask = extract_psw_mask() | 0x0000C00000000000UL;
>> -	load_psw_mask(mask);
>>  }
>>  
>>  int main(void)
> 

[kvm-unit-tests PATCH 6/8] lib: s390x: Add PSW_MASK_64

[Janosch Frank]

Let's replace the magic 0x0000000180000000ULL numeric constants with
PSW_MASK_64 as it's used more often since the introduction of smp and
sie.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/arch_def.h | 3 +++
 lib/s390x/smp.c          | 2 +-
 s390x/mvpg-sie.c         | 2 +-
 s390x/sie.c              | 2 +-
 s390x/skrf.c             | 6 +++---
 5 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 39c5ba99..245453c3 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -50,6 +50,9 @@ struct psw {
 #define PSW_MASK_DAT			0x0400000000000000UL
 #define PSW_MASK_WAIT			0x0002000000000000UL
 #define PSW_MASK_PSTATE			0x0001000000000000UL
+#define PSW_MASK_EA			0x0000000100000000UL
+#define PSW_MASK_BA			0x0000000080000000UL
+#define PSW_MASK_64			PSW_MASK_BA | PSW_MASK_EA;
 
 #define CR0_EXTM_SCLP			0x0000000000000200UL
 #define CR0_EXTM_EXTC			0x0000000000002000UL
diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index ee68d676..228fe667 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -202,7 +202,7 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
 	cpu->lowcore->sw_int_psw.addr = psw.addr;
 	cpu->lowcore->sw_int_grs[14] = psw.addr;
 	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack + (PAGE_SIZE * 4);
-	lc->restart_new_psw.mask = 0x0000000180000000UL;
+	lc->restart_new_psw.mask = PSW_MASK_64;
 	lc->restart_new_psw.addr = (uint64_t)smp_cpu_setup_state;
 	lc->sw_int_crs[0] = 0x0000000000040000UL;
 
diff --git a/s390x/mvpg-sie.c b/s390x/mvpg-sie.c
index 70d2fcfa..ccc273b4 100644
--- a/s390x/mvpg-sie.c
+++ b/s390x/mvpg-sie.c
@@ -100,7 +100,7 @@ static void setup_guest(void)
 	sie_guest_create(&vm, (uint64_t)guest, HPAGE_SIZE);
 
 	vm.sblk->gpsw.addr = PAGE_SIZE * 4;
-	vm.sblk->gpsw.mask = 0x0000000180000000ULL;
+	vm.sblk->gpsw.mask = PSW_MASK_64;
 	vm.sblk->ictl = ICTL_OPEREXC | ICTL_PINT;
 	/* Enable MVPG interpretation as we want to test KVM and not ourselves */
 	vm.sblk->eca = ECA_MVPGI;
diff --git a/s390x/sie.c b/s390x/sie.c
index ed2c3263..87575b29 100644
--- a/s390x/sie.c
+++ b/s390x/sie.c
@@ -27,7 +27,7 @@ static struct vm vm;
 static void test_diag(u32 instr)
 {
 	vm.sblk->gpsw.addr = PAGE_SIZE * 2;
-	vm.sblk->gpsw.mask = 0x0000000180000000ULL;
+	vm.sblk->gpsw.mask = PSW_MASK_64;
 
 	memset(guest_instr, 0, PAGE_SIZE);
 	memcpy(guest_instr, &instr, 4);
diff --git a/s390x/skrf.c b/s390x/skrf.c
index 94e906a6..9488c32b 100644
--- a/s390x/skrf.c
+++ b/s390x/skrf.c
@@ -125,15 +125,15 @@ static void ecall_cleanup(void)
 {
 	struct lowcore *lc = (void *)0x0;
 
-	lc->ext_new_psw.mask = 0x0000000180000000UL;
 	lc->sw_int_crs[0] = 0x0000000000040000;
+	lc->ext_new_psw.mask = PSW_MASK_64;
 
 	/*
 	 * PGM old contains the ext new PSW, we need to clean it up,
 	 * so we don't get a special operation exception on the lpswe
 	 * of pgm old.
 	 */
-	lc->pgm_old_psw.mask = 0x0000000180000000UL;
+	lc->pgm_old_psw.mask = PSW_MASK_64;
 
 	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
 	set_flag(1);
@@ -148,7 +148,7 @@ static void ecall_setup(void)
 	register_pgm_cleanup_func(ecall_cleanup);
 	expect_pgm_int();
 	/* Put a skey into the ext new psw */
-	lc->ext_new_psw.mask = 0x00F0000180000000UL;
+	lc->ext_new_psw.mask = 0x00F0000000000000UL | PSW_MASK_64;
 	/* Open up ext masks */
 	ctl_set_bit(0, CTL0_EXTERNAL_CALL);
 	mask = extract_psw_mask();
-- 
2.30.2

Re: [kvm-unit-tests PATCH 6/8] lib: s390x: Add PSW_MASK_64

[Claudio Imbrenda]

On Fri, 13 Aug 2021 07:36:13 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's replace the magic 0x0000000180000000ULL numeric constants with
> PSW_MASK_64 as it's used more often since the introduction of smp and
> sie.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

> ---
>  lib/s390x/asm/arch_def.h | 3 +++
>  lib/s390x/smp.c          | 2 +-
>  s390x/mvpg-sie.c         | 2 +-
>  s390x/sie.c              | 2 +-
>  s390x/skrf.c             | 6 +++---
>  5 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 39c5ba99..245453c3 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -50,6 +50,9 @@ struct psw {
>  #define PSW_MASK_DAT			0x0400000000000000UL
>  #define PSW_MASK_WAIT			0x0002000000000000UL
>  #define PSW_MASK_PSTATE			0x0001000000000000UL
> +#define PSW_MASK_EA			0x0000000100000000UL
> +#define PSW_MASK_BA			0x0000000080000000UL
> +#define PSW_MASK_64			PSW_MASK_BA | PSW_MASK_EA;
>  
>  #define CR0_EXTM_SCLP			0x0000000000000200UL
>  #define CR0_EXTM_EXTC			0x0000000000002000UL
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index ee68d676..228fe667 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -202,7 +202,7 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
>  	cpu->lowcore->sw_int_psw.addr = psw.addr;
>  	cpu->lowcore->sw_int_grs[14] = psw.addr;
>  	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack +
> (PAGE_SIZE * 4);
> -	lc->restart_new_psw.mask = 0x0000000180000000UL;
> +	lc->restart_new_psw.mask = PSW_MASK_64;
>  	lc->restart_new_psw.addr = (uint64_t)smp_cpu_setup_state;
>  	lc->sw_int_crs[0] = 0x0000000000040000UL;
>  
> diff --git a/s390x/mvpg-sie.c b/s390x/mvpg-sie.c
> index 70d2fcfa..ccc273b4 100644
> --- a/s390x/mvpg-sie.c
> +++ b/s390x/mvpg-sie.c
> @@ -100,7 +100,7 @@ static void setup_guest(void)
>  	sie_guest_create(&vm, (uint64_t)guest, HPAGE_SIZE);
>  
>  	vm.sblk->gpsw.addr = PAGE_SIZE * 4;
> -	vm.sblk->gpsw.mask = 0x0000000180000000ULL;
> +	vm.sblk->gpsw.mask = PSW_MASK_64;
>  	vm.sblk->ictl = ICTL_OPEREXC | ICTL_PINT;
>  	/* Enable MVPG interpretation as we want to test KVM and not
> ourselves */ vm.sblk->eca = ECA_MVPGI;
> diff --git a/s390x/sie.c b/s390x/sie.c
> index ed2c3263..87575b29 100644
> --- a/s390x/sie.c
> +++ b/s390x/sie.c
> @@ -27,7 +27,7 @@ static struct vm vm;
>  static void test_diag(u32 instr)
>  {
>  	vm.sblk->gpsw.addr = PAGE_SIZE * 2;
> -	vm.sblk->gpsw.mask = 0x0000000180000000ULL;
> +	vm.sblk->gpsw.mask = PSW_MASK_64;
>  
>  	memset(guest_instr, 0, PAGE_SIZE);
>  	memcpy(guest_instr, &instr, 4);
> diff --git a/s390x/skrf.c b/s390x/skrf.c
> index 94e906a6..9488c32b 100644
> --- a/s390x/skrf.c
> +++ b/s390x/skrf.c
> @@ -125,15 +125,15 @@ static void ecall_cleanup(void)
>  {
>  	struct lowcore *lc = (void *)0x0;
>  
> -	lc->ext_new_psw.mask = 0x0000000180000000UL;
>  	lc->sw_int_crs[0] = 0x0000000000040000;
> +	lc->ext_new_psw.mask = PSW_MASK_64;
>  
>  	/*
>  	 * PGM old contains the ext new PSW, we need to clean it up,
>  	 * so we don't get a special operation exception on the lpswe
>  	 * of pgm old.
>  	 */
> -	lc->pgm_old_psw.mask = 0x0000000180000000UL;
> +	lc->pgm_old_psw.mask = PSW_MASK_64;
>  
>  	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>  	set_flag(1);
> @@ -148,7 +148,7 @@ static void ecall_setup(void)
>  	register_pgm_cleanup_func(ecall_cleanup);
>  	expect_pgm_int();
>  	/* Put a skey into the ext new psw */
> -	lc->ext_new_psw.mask = 0x00F0000180000000UL;
> +	lc->ext_new_psw.mask = 0x00F0000000000000UL | PSW_MASK_64;
>  	/* Open up ext masks */
>  	ctl_set_bit(0, CTL0_EXTERNAL_CALL);
>  	mask = extract_psw_mask();

Re: [kvm-unit-tests PATCH 6/8] lib: s390x: Add PSW_MASK_64

[Thomas Huth]

On 13/08/2021 09.36, Janosch Frank wrote:
> Let's replace the magic 0x0000000180000000ULL numeric constants with
> PSW_MASK_64 as it's used more often since the introduction of smp and
> sie.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   lib/s390x/asm/arch_def.h | 3 +++
>   lib/s390x/smp.c          | 2 +-
>   s390x/mvpg-sie.c         | 2 +-
>   s390x/sie.c              | 2 +-
>   s390x/skrf.c             | 6 +++---
>   5 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 39c5ba99..245453c3 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -50,6 +50,9 @@ struct psw {
>   #define PSW_MASK_DAT			0x0400000000000000UL
>   #define PSW_MASK_WAIT			0x0002000000000000UL
>   #define PSW_MASK_PSTATE			0x0001000000000000UL
> +#define PSW_MASK_EA			0x0000000100000000UL
> +#define PSW_MASK_BA			0x0000000080000000UL
> +#define PSW_MASK_64			PSW_MASK_BA | PSW_MASK_EA;

Please put some parentheses around PSW_MASK_BA | PSW_MASK_EA and remove the 
semicolon at the end.

  Thomas

[kvm-unit-tests PATCH 7/8] lib: s390x: Control register constant cleanup

[Janosch Frank]

We had bits and masks defined and don't necessarily need both.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/arch_def.h | 29 +++++++++++++----------------
 lib/s390x/smp.c          |  2 +-
 s390x/skrf.c             |  2 +-
 3 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 245453c3..4574a166 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -54,10 +54,19 @@ struct psw {
 #define PSW_MASK_BA			0x0000000080000000UL
 #define PSW_MASK_64			PSW_MASK_BA | PSW_MASK_EA;
 
-#define CR0_EXTM_SCLP			0x0000000000000200UL
-#define CR0_EXTM_EXTC			0x0000000000002000UL
-#define CR0_EXTM_EMGC			0x0000000000004000UL
-#define CR0_EXTM_MASK			0x0000000000006200UL
+#define CTL0_LOW_ADDR_PROT		(63 - 35)
+#define CTL0_EDAT			(63 - 40)
+#define CTL0_IEP			(63 - 43)
+#define CTL0_AFP			(63 - 45)
+#define CTL0_VECTOR			(63 - 46)
+#define CTL0_EMERGENCY_SIGNAL		(63 - 49)
+#define CTL0_EXTERNAL_CALL		(63 - 50)
+#define CTL0_CLOCK_COMPARATOR		(63 - 52)
+#define CTL0_SERVICE_SIGNAL		(63 - 54)
+#define CR0_EXTM_MASK			0x0000000000006200UL /* Combined external masks */
+#define BIT_TO_MASK64(x)		1UL << x
+
+#define CTL2_GUARDED_STORAGE		(63 - 59)
 
 struct lowcore {
 	uint8_t		pad_0x0000[0x0080 - 0x0000];	/* 0x0000 */
@@ -239,18 +248,6 @@ static inline uint64_t stctg(int cr)
 	return value;
 }
 
-#define CTL0_LOW_ADDR_PROT	(63 - 35)
-#define CTL0_EDAT		(63 - 40)
-#define CTL0_IEP		(63 - 43)
-#define CTL0_AFP		(63 - 45)
-#define CTL0_VECTOR		(63 - 46)
-#define CTL0_EMERGENCY_SIGNAL	(63 - 49)
-#define CTL0_EXTERNAL_CALL	(63 - 50)
-#define CTL0_CLOCK_COMPARATOR	(63 - 52)
-#define CTL0_SERVICE_SIGNAL	(63 - 54)
-
-#define CTL2_GUARDED_STORAGE	(63 - 59)
-
 static inline void ctl_set_bit(int cr, unsigned int bit)
 {
         uint64_t reg;
diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index 228fe667..c2c6ffec 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -204,7 +204,7 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
 	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack + (PAGE_SIZE * 4);
 	lc->restart_new_psw.mask = PSW_MASK_64;
 	lc->restart_new_psw.addr = (uint64_t)smp_cpu_setup_state;
-	lc->sw_int_crs[0] = 0x0000000000040000UL;
+	lc->sw_int_crs[0] = BIT_TO_MASK64(CTL0_AFP);
 
 	/* Start processing */
 	smp_cpu_restart_nolock(addr, NULL);
diff --git a/s390x/skrf.c b/s390x/skrf.c
index 9488c32b..a350ada6 100644
--- a/s390x/skrf.c
+++ b/s390x/skrf.c
@@ -125,8 +125,8 @@ static void ecall_cleanup(void)
 {
 	struct lowcore *lc = (void *)0x0;
 
-	lc->sw_int_crs[0] = 0x0000000000040000;
 	lc->ext_new_psw.mask = PSW_MASK_64;
+	lc->sw_int_crs[0] = BIT_TO_MASK64(CTL0_AFP);
 
 	/*
 	 * PGM old contains the ext new PSW, we need to clean it up,
-- 
2.30.2

Re: [kvm-unit-tests PATCH 7/8] lib: s390x: Control register constant cleanup

[Claudio Imbrenda]

On Fri, 13 Aug 2021 07:36:14 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> We had bits and masks defined and don't necessarily need both.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/asm/arch_def.h | 29 +++++++++++++----------------
>  lib/s390x/smp.c          |  2 +-
>  s390x/skrf.c             |  2 +-
>  3 files changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 245453c3..4574a166 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -54,10 +54,19 @@ struct psw {
>  #define PSW_MASK_BA			0x0000000080000000UL
>  #define PSW_MASK_64			PSW_MASK_BA | PSW_MASK_EA;
>  
> -#define CR0_EXTM_SCLP			0x0000000000000200UL
> -#define CR0_EXTM_EXTC			0x0000000000002000UL
> -#define CR0_EXTM_EMGC			0x0000000000004000UL
> -#define CR0_EXTM_MASK			0x0000000000006200UL
> +#define CTL0_LOW_ADDR_PROT		(63 - 35)
> +#define CTL0_EDAT			(63 - 40)
> +#define CTL0_IEP			(63 - 43)
> +#define CTL0_AFP			(63 - 45)
> +#define CTL0_VECTOR			(63 - 46)
> +#define CTL0_EMERGENCY_SIGNAL		(63 - 49)
> +#define CTL0_EXTERNAL_CALL		(63 - 50)
> +#define CTL0_CLOCK_COMPARATOR		(63 - 52)
> +#define CTL0_SERVICE_SIGNAL		(63 - 54)
> +#define CR0_EXTM_MASK			0x0000000000006200UL /*
> Combined external masks */
> +#define BIT_TO_MASK64(x)		1UL << x

don't we already have BIT and BIT_ULL?

> +#define CTL2_GUARDED_STORAGE		(63 - 59)
>  
>  struct lowcore {
>  	uint8_t		pad_0x0000[0x0080 - 0x0000];
> /* 0x0000 */ @@ -239,18 +248,6 @@ static inline uint64_t stctg(int cr)
>  	return value;
>  }
>  
> -#define CTL0_LOW_ADDR_PROT	(63 - 35)
> -#define CTL0_EDAT		(63 - 40)
> -#define CTL0_IEP		(63 - 43)
> -#define CTL0_AFP		(63 - 45)
> -#define CTL0_VECTOR		(63 - 46)
> -#define CTL0_EMERGENCY_SIGNAL	(63 - 49)
> -#define CTL0_EXTERNAL_CALL	(63 - 50)
> -#define CTL0_CLOCK_COMPARATOR	(63 - 52)
> -#define CTL0_SERVICE_SIGNAL	(63 - 54)
> -
> -#define CTL2_GUARDED_STORAGE	(63 - 59)
> -
>  static inline void ctl_set_bit(int cr, unsigned int bit)
>  {
>          uint64_t reg;
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index 228fe667..c2c6ffec 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -204,7 +204,7 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
>  	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack +
> (PAGE_SIZE * 4); lc->restart_new_psw.mask = PSW_MASK_64;
>  	lc->restart_new_psw.addr = (uint64_t)smp_cpu_setup_state;
> -	lc->sw_int_crs[0] = 0x0000000000040000UL;
> +	lc->sw_int_crs[0] = BIT_TO_MASK64(CTL0_AFP);
>  
>  	/* Start processing */
>  	smp_cpu_restart_nolock(addr, NULL);
> diff --git a/s390x/skrf.c b/s390x/skrf.c
> index 9488c32b..a350ada6 100644
> --- a/s390x/skrf.c
> +++ b/s390x/skrf.c
> @@ -125,8 +125,8 @@ static void ecall_cleanup(void)
>  {
>  	struct lowcore *lc = (void *)0x0;
>  
> -	lc->sw_int_crs[0] = 0x0000000000040000;
>  	lc->ext_new_psw.mask = PSW_MASK_64;
> +	lc->sw_int_crs[0] = BIT_TO_MASK64(CTL0_AFP);
>  
>  	/*
>  	 * PGM old contains the ext new PSW, we need to clean it up,

Re: [kvm-unit-tests PATCH 7/8] lib: s390x: Control register constant cleanup

[Janosch Frank]

On 8/13/21 10:49 AM, Claudio Imbrenda wrote:
> On Fri, 13 Aug 2021 07:36:14 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> We had bits and masks defined and don't necessarily need both.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  lib/s390x/asm/arch_def.h | 29 +++++++++++++----------------
>>  lib/s390x/smp.c          |  2 +-
>>  s390x/skrf.c             |  2 +-
>>  3 files changed, 15 insertions(+), 18 deletions(-)
>>
>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>> index 245453c3..4574a166 100644
>> --- a/lib/s390x/asm/arch_def.h
>> +++ b/lib/s390x/asm/arch_def.h
>> @@ -54,10 +54,19 @@ struct psw {
>>  #define PSW_MASK_BA			0x0000000080000000UL
>>  #define PSW_MASK_64			PSW_MASK_BA | PSW_MASK_EA;
>>  
>> -#define CR0_EXTM_SCLP			0x0000000000000200UL
>> -#define CR0_EXTM_EXTC			0x0000000000002000UL
>> -#define CR0_EXTM_EMGC			0x0000000000004000UL
>> -#define CR0_EXTM_MASK			0x0000000000006200UL
>> +#define CTL0_LOW_ADDR_PROT		(63 - 35)
>> +#define CTL0_EDAT			(63 - 40)
>> +#define CTL0_IEP			(63 - 43)
>> +#define CTL0_AFP			(63 - 45)
>> +#define CTL0_VECTOR			(63 - 46)
>> +#define CTL0_EMERGENCY_SIGNAL		(63 - 49)
>> +#define CTL0_EXTERNAL_CALL		(63 - 50)
>> +#define CTL0_CLOCK_COMPARATOR		(63 - 52)
>> +#define CTL0_SERVICE_SIGNAL		(63 - 54)
>> +#define CR0_EXTM_MASK			0x0000000000006200UL /*
>> Combined external masks */
>> +#define BIT_TO_MASK64(x)		1UL << x
> 
> don't we already have BIT and BIT_ULL?

Right, I should have looked first...
Will fix!

> 
>> +#define CTL2_GUARDED_STORAGE		(63 - 59)
>>  
>>  struct lowcore {
>>  	uint8_t		pad_0x0000[0x0080 - 0x0000];
>> /* 0x0000 */ @@ -239,18 +248,6 @@ static inline uint64_t stctg(int cr)
>>  	return value;
>>  }
>>  
>> -#define CTL0_LOW_ADDR_PROT	(63 - 35)
>> -#define CTL0_EDAT		(63 - 40)
>> -#define CTL0_IEP		(63 - 43)
>> -#define CTL0_AFP		(63 - 45)
>> -#define CTL0_VECTOR		(63 - 46)
>> -#define CTL0_EMERGENCY_SIGNAL	(63 - 49)
>> -#define CTL0_EXTERNAL_CALL	(63 - 50)
>> -#define CTL0_CLOCK_COMPARATOR	(63 - 52)
>> -#define CTL0_SERVICE_SIGNAL	(63 - 54)
>> -
>> -#define CTL2_GUARDED_STORAGE	(63 - 59)
>> -
>>  static inline void ctl_set_bit(int cr, unsigned int bit)
>>  {
>>          uint64_t reg;
>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
>> index 228fe667..c2c6ffec 100644
>> --- a/lib/s390x/smp.c
>> +++ b/lib/s390x/smp.c
>> @@ -204,7 +204,7 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
>>  	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack +
>> (PAGE_SIZE * 4); lc->restart_new_psw.mask = PSW_MASK_64;
>>  	lc->restart_new_psw.addr = (uint64_t)smp_cpu_setup_state;
>> -	lc->sw_int_crs[0] = 0x0000000000040000UL;
>> +	lc->sw_int_crs[0] = BIT_TO_MASK64(CTL0_AFP);
>>  
>>  	/* Start processing */
>>  	smp_cpu_restart_nolock(addr, NULL);
>> diff --git a/s390x/skrf.c b/s390x/skrf.c
>> index 9488c32b..a350ada6 100644
>> --- a/s390x/skrf.c
>> +++ b/s390x/skrf.c
>> @@ -125,8 +125,8 @@ static void ecall_cleanup(void)
>>  {
>>  	struct lowcore *lc = (void *)0x0;
>>  
>> -	lc->sw_int_crs[0] = 0x0000000000040000;
>>  	lc->ext_new_psw.mask = PSW_MASK_64;
>> +	lc->sw_int_crs[0] = BIT_TO_MASK64(CTL0_AFP);
>>  
>>  	/*
>>  	 * PGM old contains the ext new PSW, we need to clean it up,
> 

[kvm-unit-tests PATCH 8/8] lib: s390x: uv: Add rc 0x100 query error handling

[Janosch Frank]

Let's not get bitten by an extension of the query struct and handle
the rc 0x100 error properly which does indicate that the UV has more
data for us.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/uv.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/uv.c b/lib/s390x/uv.c
index fd9de944..c5c69c47 100644
--- a/lib/s390x/uv.c
+++ b/lib/s390x/uv.c
@@ -49,6 +49,8 @@ int uv_setup(void)
 	if (!test_facility(158))
 		return 0;
 
-	assert(!uv_call(0, (u64)&uvcb_qui));
+	uv_call(0, (u64)&uvcb_qui);
+
+	assert(uvcb_qui.header.rc == 1 || uvcb_qui.header.rc == 0x100);
 	return 1;
 }
-- 
2.30.2

Re: [kvm-unit-tests PATCH 8/8] lib: s390x: uv: Add rc 0x100 query error handling

[Claudio Imbrenda]

On Fri, 13 Aug 2021 07:36:15 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's not get bitten by an extension of the query struct and handle
> the rc 0x100 error properly which does indicate that the UV has more
> data for us.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

> ---
>  lib/s390x/uv.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/uv.c b/lib/s390x/uv.c
> index fd9de944..c5c69c47 100644
> --- a/lib/s390x/uv.c
> +++ b/lib/s390x/uv.c
> @@ -49,6 +49,8 @@ int uv_setup(void)
>  	if (!test_facility(158))
>  		return 0;
>  
> -	assert(!uv_call(0, (u64)&uvcb_qui));
> +	uv_call(0, (u64)&uvcb_qui);
> +
> +	assert(uvcb_qui.header.rc == 1 || uvcb_qui.header.rc ==
> 0x100); return 1;
>  }

Re: [kvm-unit-tests PATCH 8/8] lib: s390x: uv: Add rc 0x100 query error handling

[Thomas Huth]

On 13/08/2021 09.36, Janosch Frank wrote:
> Let's not get bitten by an extension of the query struct and handle
> the rc 0x100 error properly which does indicate that the UV has more
> data for us.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   lib/s390x/uv.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/uv.c b/lib/s390x/uv.c
> index fd9de944..c5c69c47 100644
> --- a/lib/s390x/uv.c
> +++ b/lib/s390x/uv.c
> @@ -49,6 +49,8 @@ int uv_setup(void)
>   	if (!test_facility(158))
>   		return 0;
>   
> -	assert(!uv_call(0, (u64)&uvcb_qui));
> +	uv_call(0, (u64)&uvcb_qui);
> +
> +	assert(uvcb_qui.header.rc == 1 || uvcb_qui.header.rc == 0x100);

Don't you want to continue to check the return code of the uv_call() function?

  Thomas

Re: [kvm-unit-tests PATCH 8/8] lib: s390x: uv: Add rc 0x100 query error handling

[Janosch Frank]

On 8/18/21 11:30 AM, Thomas Huth wrote:
> On 13/08/2021 09.36, Janosch Frank wrote:
>> Let's not get bitten by an extension of the query struct and handle
>> the rc 0x100 error properly which does indicate that the UV has more
>> data for us.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   lib/s390x/uv.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/s390x/uv.c b/lib/s390x/uv.c
>> index fd9de944..c5c69c47 100644
>> --- a/lib/s390x/uv.c
>> +++ b/lib/s390x/uv.c
>> @@ -49,6 +49,8 @@ int uv_setup(void)
>>   	if (!test_facility(158))
>>   		return 0;
>>   
>> -	assert(!uv_call(0, (u64)&uvcb_qui));
>> +	uv_call(0, (u64)&uvcb_qui);
>> +
>> +	assert(uvcb_qui.header.rc == 1 || uvcb_qui.header.rc == 0x100);
> 
> Don't you want to continue to check the return code of the uv_call() function?
> 
>   Thomas
> 

The rc==0x100 case is a cc==1 and the rc==1 is a cc==0 so I had to
delete the check.

Those smaller patches are already upstream btw.