[kvm-unit-tests,v2] s390x: try !FORCE SCLP read SCP info if FORCED is unknown
diff mbox series

Message ID 20181217101651.11723-1-david@redhat.com
State New
Headers show
Series
  • [kvm-unit-tests,v2] s390x: try !FORCE SCLP read SCP info if FORCED is unknown
Related show

Commit Message

David Hildenbrand Dec. 17, 2018, 10:16 a.m. UTC
Looking at what the kernel does, looks like we should
- Check for more errors
- Try to execute !FORCED read if the FORCED one is unknown
- Set the function code to a normal write
- Set control_mask[2] to SCLP_VARIABLE_LENGTH_RESPONSE

The kernel seems to set the function code to 0x80. Looking at where that
comes from, turns out this is a "dump indicator" to produce more
meaningful dumps. Looking for other function codes used in the kernel, I
discovered 0x40, which is used for "single increment assign" to speed up
memory hotplug. I was not able to find out what "control_mask[2] = 0x80"
means.

I guess we can just set the funcion code to 0 ("normal write). Let's
document the other bits in case anybody ever wonders what they mean.
Rename SCLP_VARIABLE_LENGTH_RESPONSE to SCLP_CM2_VARIABLE_LENGTH_RESPONSE
and move the SCLP function codes, mask bits and event buffer flags to
dedicated sections in the header.

Make sure that all other callers properly initialize the header
fully (also inititalize control_mask and function_code).

Tested-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---

v1 -> v2:
- Use define for control mask value 0x80
- Rename SCLP_VARIABLE_LENGTH_RESPONSE to SCLP_CM2_VARIABLE_LENGTH_RESPONSE
- Cleanup sclp.h header file a bit (move to sections)
- Make sure control mask and function code are in a deterministic state
  when issuing sclp_service_call

 lib/s390x/sclp-ascii.c |  7 +++++++
 lib/s390x/sclp.c       | 26 ++++++++++++++++++++++++--
 lib/s390x/sclp.h       | 13 +++++++++----
 3 files changed, 40 insertions(+), 6 deletions(-)

Comments

Janosch Frank Dec. 17, 2018, 11:19 a.m. UTC | #1
On 17.12.18 11:16, David Hildenbrand wrote:
> Looking at what the kernel does, looks like we should
> - Check for more errors
> - Try to execute !FORCED read if the FORCED one is unknown
> - Set the function code to a normal write
> - Set control_mask[2] to SCLP_VARIABLE_LENGTH_RESPONSE
> 
> The kernel seems to set the function code to 0x80. Looking at where that
> comes from, turns out this is a "dump indicator" to produce more
> meaningful dumps. Looking for other function codes used in the kernel, I
> discovered 0x40, which is used for "single increment assign" to speed up
> memory hotplug. I was not able to find out what "control_mask[2] = 0x80"
> means.

We figured out what control_mask[2] = 0x80 is, so you can drop that.
Maybe rephrase that anyway and put it to the second to last paragraph.

> 
> I guess we can just set the funcion code to 0 ("normal write). Let's

s/funcion/function/

> document the other bits in case anybody ever wonders what they mean.
> Rename SCLP_VARIABLE_LENGTH_RESPONSE to SCLP_CM2_VARIABLE_LENGTH_RESPONSE
> and move the SCLP function codes, mask bits and event buffer flags to
> dedicated sections in the header.
> 
> Make sure that all other callers properly initialize the header
> fully (also inititalize control_mask and function_code).

s/inititalize/initialize/

> 
> Tested-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> 
> v1 -> v2:
> - Use define for control mask value 0x80
> - Rename SCLP_VARIABLE_LENGTH_RESPONSE to SCLP_CM2_VARIABLE_LENGTH_RESPONSE
> - Cleanup sclp.h header file a bit (move to sections)
> - Make sure control mask and function code are in a deterministic state
>   when issuing sclp_service_call
> 
>  lib/s390x/sclp-ascii.c |  7 +++++++
>  lib/s390x/sclp.c       | 26 ++++++++++++++++++++++++--
>  lib/s390x/sclp.h       | 13 +++++++++----
>  3 files changed, 40 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/s390x/sclp-ascii.c b/lib/s390x/sclp-ascii.c
> index 893ca17..344a414 100644
> --- a/lib/s390x/sclp-ascii.c
> +++ b/lib/s390x/sclp-ascii.c
> @@ -38,6 +38,10 @@ static void sclp_set_write_mask(void)
>      WriteEventMask *sccb = (void *)_sccb;
>  
>      sccb->h.length = sizeof(WriteEventMask);
> +    sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
> +    sccb->h.control_mask[0] = 0;
> +    sccb->h.control_mask[1] = 0;
> +    sccb->h.control_mask[2] = 0;

I opted for a memset and I would prefer it here too, as I'll introduce
it anyway.

>      sccb->mask_length = sizeof(unsigned int);
>      sccb->receive_mask = SCLP_EVENT_MASK_MSG_ASCII;
>      sccb->cp_receive_mask = SCLP_EVENT_MASK_MSG_ASCII;
> @@ -59,6 +63,9 @@ void sclp_print(const char *str)
>  
>      sccb->h.length = sizeof(WriteEventData) + len;
>      sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
> +    sccb->h.control_mask[0] = 0;
> +    sccb->h.control_mask[1] = 0;
> +    sccb->h.control_mask[2] = 0;
>      sccb->ebh.length = sizeof(EventBufferHeader) + len;
>      sccb->ebh.type = SCLP_EVENT_ASCII_CONSOLE_DATA;
>      sccb->ebh.flags = 0;
> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> index 1d4a010..d04981d 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -30,14 +30,36 @@ static void mem_init(phys_addr_t mem_end)
>  	phys_alloc_init(freemem_start, mem_end - freemem_start);
>  }
>  
> +static void sclp_read_scp_info(ReadInfo *ri, int length)
> +{
> +	unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED,
> +				    SCLP_CMDW_READ_SCP_INFO };
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(commands); i++) {
> +		ri->h.length = length;
> +		ri->h.function_code = SCLP_FC_NORMAL_WRITE;
> +		ri->h.control_mask[0] = 0;
> +		ri->h.control_mask[1] = 0;
> +		ri->h.control_mask[2] = SCLP_CM2_VARIABLE_LENGTH_RESPONSE;

Same here.

> +
> +		if (sclp_service_call(commands[i], ri))
> +			break;
> +		if (ri->h.response_code == SCLP_RC_NORMAL_READ_COMPLETION)
> +			return;
> +		if (ri->h.response_code != SCLP_RC_INVALID_SCLP_COMMAND)
> +			break;
> +	}
> +	report_abort("READ_SCP_INFO failed");
> +}
> +
>  void sclp_memory_setup(void)
>  {
>  	ReadInfo *ri = (void *)_sccb;
>  	uint64_t rnmax, rnsize;
>  	int cc;
>  
> -	ri->h.length = SCCB_SIZE;
> -	sclp_service_call(SCLP_CMDW_READ_SCP_INFO_FORCED, ri);
> +	sclp_read_scp_info(ri, SCCB_SIZE);
>  
>  	/* calculate the storage increment size */
>  	rnsize = ri->rnsize;
> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
> index 21d482b..629e9e2 100644
> --- a/lib/s390x/sclp.h
> +++ b/lib/s390x/sclp.h
> @@ -68,14 +68,19 @@
>  #define SCLP_RC_EVENT_BUFFER_SYNTAX_ERROR       0x73f0
>  #define SCLP_RC_INVALID_MASK_LENGTH             0x74f0
>  
> -/* Service Call Control Block (SCCB) and its elements */
> +/* SCLP control mask bits */
> +#define SCLP_CM2_VARIABLE_LENGTH_RESPONSE       0x80
>  
> -#define SCCB_SIZE 4096
> +/* SCLP function codes */
> +#define SCLP_FC_NORMAL_WRITE                    0
> +#define SCLP_FC_SINGLE_INCREMENT_ASSIGN         0x40
> +#define SCLP_FC_DUMP_INDICATOR                  0x80
>  
> -#define SCLP_VARIABLE_LENGTH_RESPONSE           0x80
> +/* SCLP event buffer flags */
>  #define SCLP_EVENT_BUFFER_ACCEPTED              0x80
>  
> -#define SCLP_FC_NORMAL_WRITE                    0
> +/* Service Call Control Block (SCCB) and its elements */
> +#define SCCB_SIZE 4096
>  
>  typedef struct SCCBHeader {
>      uint16_t length;
>
David Hildenbrand Dec. 17, 2018, 12:43 p.m. UTC | #2
On 17.12.18 12:19, Janosch Frank wrote:
> On 17.12.18 11:16, David Hildenbrand wrote:
>> Looking at what the kernel does, looks like we should
>> - Check for more errors
>> - Try to execute !FORCED read if the FORCED one is unknown
>> - Set the function code to a normal write
>> - Set control_mask[2] to SCLP_VARIABLE_LENGTH_RESPONSE
>>
>> The kernel seems to set the function code to 0x80. Looking at where that
>> comes from, turns out this is a "dump indicator" to produce more
>> meaningful dumps. Looking for other function codes used in the kernel, I
>> discovered 0x40, which is used for "single increment assign" to speed up
>> memory hotplug. I was not able to find out what "control_mask[2] = 0x80"
>> means.
> 
> We figured out what control_mask[2] = 0x80 is, so you can drop that.
> Maybe rephrase that anyway and put it to the second to last paragraph.
> 

Does it actually make sense to set 0x80 at all or should we simply drop it?
> 
>>
>> Tested-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>
>> v1 -> v2:
>> - Use define for control mask value 0x80
>> - Rename SCLP_VARIABLE_LENGTH_RESPONSE to SCLP_CM2_VARIABLE_LENGTH_RESPONSE
>> - Cleanup sclp.h header file a bit (move to sections)
>> - Make sure control mask and function code are in a deterministic state
>>   when issuing sclp_service_call
>>
>>  lib/s390x/sclp-ascii.c |  7 +++++++
>>  lib/s390x/sclp.c       | 26 ++++++++++++++++++++++++--
>>  lib/s390x/sclp.h       | 13 +++++++++----
>>  3 files changed, 40 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/s390x/sclp-ascii.c b/lib/s390x/sclp-ascii.c
>> index 893ca17..344a414 100644
>> --- a/lib/s390x/sclp-ascii.c
>> +++ b/lib/s390x/sclp-ascii.c
>> @@ -38,6 +38,10 @@ static void sclp_set_write_mask(void)
>>      WriteEventMask *sccb = (void *)_sccb;
>>  
>>      sccb->h.length = sizeof(WriteEventMask);
>> +    sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
>> +    sccb->h.control_mask[0] = 0;
>> +    sccb->h.control_mask[1] = 0;
>> +    sccb->h.control_mask[2] = 0;
> 
> I opted for a memset and I would prefer it here too, as I'll introduce
> it anyway.

Make sense, but only for the mask and not the whole header I guess?
Janosch Frank Dec. 17, 2018, 1:10 p.m. UTC | #3
On 17.12.18 13:43, David Hildenbrand wrote:
> On 17.12.18 12:19, Janosch Frank wrote:
>> On 17.12.18 11:16, David Hildenbrand wrote:
>>> Looking at what the kernel does, looks like we should
>>> - Check for more errors
>>> - Try to execute !FORCED read if the FORCED one is unknown
>>> - Set the function code to a normal write
>>> - Set control_mask[2] to SCLP_VARIABLE_LENGTH_RESPONSE
>>>
>>> The kernel seems to set the function code to 0x80. Looking at where that
>>> comes from, turns out this is a "dump indicator" to produce more
>>> meaningful dumps. Looking for other function codes used in the kernel, I
>>> discovered 0x40, which is used for "single increment assign" to speed up
>>> memory hotplug. I was not able to find out what "control_mask[2] = 0x80"
>>> means.
>>
>> We figured out what control_mask[2] = 0x80 is, so you can drop that.
>> Maybe rephrase that anyway and put it to the second to last paragraph.
>>
> 
> Does it actually make sense to set 0x80 at all or should we simply drop it?

I don't think we need it, we were fine before without it.

>>
>>>
>>> Tested-by: Janosch Frank <frankja@linux.ibm.com>
>>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>
>>> v1 -> v2:
>>> - Use define for control mask value 0x80
>>> - Rename SCLP_VARIABLE_LENGTH_RESPONSE to SCLP_CM2_VARIABLE_LENGTH_RESPONSE
>>> - Cleanup sclp.h header file a bit (move to sections)
>>> - Make sure control mask and function code are in a deterministic state
>>>   when issuing sclp_service_call
>>>
>>>  lib/s390x/sclp-ascii.c |  7 +++++++
>>>  lib/s390x/sclp.c       | 26 ++++++++++++++++++++++++--
>>>  lib/s390x/sclp.h       | 13 +++++++++----
>>>  3 files changed, 40 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/lib/s390x/sclp-ascii.c b/lib/s390x/sclp-ascii.c
>>> index 893ca17..344a414 100644
>>> --- a/lib/s390x/sclp-ascii.c
>>> +++ b/lib/s390x/sclp-ascii.c
>>> @@ -38,6 +38,10 @@ static void sclp_set_write_mask(void)
>>>      WriteEventMask *sccb = (void *)_sccb;
>>>  
>>>      sccb->h.length = sizeof(WriteEventMask);
>>> +    sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
>>> +    sccb->h.control_mask[0] = 0;
>>> +    sccb->h.control_mask[1] = 0;
>>> +    sccb->h.control_mask[2] = 0;
>>
>> I opted for a memset and I would prefer it here too, as I'll introduce
>> it anyway.
> 
> Make sense, but only for the mask and not the whole header I guess?

I memset the whole struct.
David Hildenbrand Dec. 17, 2018, 1:20 p.m. UTC | #4
On 17.12.18 14:10, Janosch Frank wrote:
> On 17.12.18 13:43, David Hildenbrand wrote:
>> On 17.12.18 12:19, Janosch Frank wrote:
>>> On 17.12.18 11:16, David Hildenbrand wrote:
>>>> Looking at what the kernel does, looks like we should
>>>> - Check for more errors
>>>> - Try to execute !FORCED read if the FORCED one is unknown
>>>> - Set the function code to a normal write
>>>> - Set control_mask[2] to SCLP_VARIABLE_LENGTH_RESPONSE
>>>>
>>>> The kernel seems to set the function code to 0x80. Looking at where that
>>>> comes from, turns out this is a "dump indicator" to produce more
>>>> meaningful dumps. Looking for other function codes used in the kernel, I
>>>> discovered 0x40, which is used for "single increment assign" to speed up
>>>> memory hotplug. I was not able to find out what "control_mask[2] = 0x80"
>>>> means.
>>>
>>> We figured out what control_mask[2] = 0x80 is, so you can drop that.
>>> Maybe rephrase that anyway and put it to the second to last paragraph.
>>>
>>
>> Does it actually make sense to set 0x80 at all or should we simply drop it?
> 
> I don't think we need it, we were fine before without it.
> 
>>>
>>>>
>>>> Tested-by: Janosch Frank <frankja@linux.ibm.com>
>>>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>> - Use define for control mask value 0x80
>>>> - Rename SCLP_VARIABLE_LENGTH_RESPONSE to SCLP_CM2_VARIABLE_LENGTH_RESPONSE
>>>> - Cleanup sclp.h header file a bit (move to sections)
>>>> - Make sure control mask and function code are in a deterministic state
>>>>   when issuing sclp_service_call
>>>>
>>>>  lib/s390x/sclp-ascii.c |  7 +++++++
>>>>  lib/s390x/sclp.c       | 26 ++++++++++++++++++++++++--
>>>>  lib/s390x/sclp.h       | 13 +++++++++----
>>>>  3 files changed, 40 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/lib/s390x/sclp-ascii.c b/lib/s390x/sclp-ascii.c
>>>> index 893ca17..344a414 100644
>>>> --- a/lib/s390x/sclp-ascii.c
>>>> +++ b/lib/s390x/sclp-ascii.c
>>>> @@ -38,6 +38,10 @@ static void sclp_set_write_mask(void)
>>>>      WriteEventMask *sccb = (void *)_sccb;
>>>>  
>>>>      sccb->h.length = sizeof(WriteEventMask);
>>>> +    sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
>>>> +    sccb->h.control_mask[0] = 0;
>>>> +    sccb->h.control_mask[1] = 0;
>>>> +    sccb->h.control_mask[2] = 0;
>>>
>>> I opted for a memset and I would prefer it here too, as I'll introduce
>>> it anyway.
>>
>> Make sense, but only for the mask and not the whole header I guess?
> 
> I memset the whole struct.
> 

Okay, I#ll leave this up to your patches and only memset the struct to
zero in the new function I introduce.

Patch
diff mbox series

diff --git a/lib/s390x/sclp-ascii.c b/lib/s390x/sclp-ascii.c
index 893ca17..344a414 100644
--- a/lib/s390x/sclp-ascii.c
+++ b/lib/s390x/sclp-ascii.c
@@ -38,6 +38,10 @@  static void sclp_set_write_mask(void)
     WriteEventMask *sccb = (void *)_sccb;
 
     sccb->h.length = sizeof(WriteEventMask);
+    sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
+    sccb->h.control_mask[0] = 0;
+    sccb->h.control_mask[1] = 0;
+    sccb->h.control_mask[2] = 0;
     sccb->mask_length = sizeof(unsigned int);
     sccb->receive_mask = SCLP_EVENT_MASK_MSG_ASCII;
     sccb->cp_receive_mask = SCLP_EVENT_MASK_MSG_ASCII;
@@ -59,6 +63,9 @@  void sclp_print(const char *str)
 
     sccb->h.length = sizeof(WriteEventData) + len;
     sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
+    sccb->h.control_mask[0] = 0;
+    sccb->h.control_mask[1] = 0;
+    sccb->h.control_mask[2] = 0;
     sccb->ebh.length = sizeof(EventBufferHeader) + len;
     sccb->ebh.type = SCLP_EVENT_ASCII_CONSOLE_DATA;
     sccb->ebh.flags = 0;
diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
index 1d4a010..d04981d 100644
--- a/lib/s390x/sclp.c
+++ b/lib/s390x/sclp.c
@@ -30,14 +30,36 @@  static void mem_init(phys_addr_t mem_end)
 	phys_alloc_init(freemem_start, mem_end - freemem_start);
 }
 
+static void sclp_read_scp_info(ReadInfo *ri, int length)
+{
+	unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED,
+				    SCLP_CMDW_READ_SCP_INFO };
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(commands); i++) {
+		ri->h.length = length;
+		ri->h.function_code = SCLP_FC_NORMAL_WRITE;
+		ri->h.control_mask[0] = 0;
+		ri->h.control_mask[1] = 0;
+		ri->h.control_mask[2] = SCLP_CM2_VARIABLE_LENGTH_RESPONSE;
+
+		if (sclp_service_call(commands[i], ri))
+			break;
+		if (ri->h.response_code == SCLP_RC_NORMAL_READ_COMPLETION)
+			return;
+		if (ri->h.response_code != SCLP_RC_INVALID_SCLP_COMMAND)
+			break;
+	}
+	report_abort("READ_SCP_INFO failed");
+}
+
 void sclp_memory_setup(void)
 {
 	ReadInfo *ri = (void *)_sccb;
 	uint64_t rnmax, rnsize;
 	int cc;
 
-	ri->h.length = SCCB_SIZE;
-	sclp_service_call(SCLP_CMDW_READ_SCP_INFO_FORCED, ri);
+	sclp_read_scp_info(ri, SCCB_SIZE);
 
 	/* calculate the storage increment size */
 	rnsize = ri->rnsize;
diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
index 21d482b..629e9e2 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -68,14 +68,19 @@ 
 #define SCLP_RC_EVENT_BUFFER_SYNTAX_ERROR       0x73f0
 #define SCLP_RC_INVALID_MASK_LENGTH             0x74f0
 
-/* Service Call Control Block (SCCB) and its elements */
+/* SCLP control mask bits */
+#define SCLP_CM2_VARIABLE_LENGTH_RESPONSE       0x80
 
-#define SCCB_SIZE 4096
+/* SCLP function codes */
+#define SCLP_FC_NORMAL_WRITE                    0
+#define SCLP_FC_SINGLE_INCREMENT_ASSIGN         0x40
+#define SCLP_FC_DUMP_INDICATOR                  0x80
 
-#define SCLP_VARIABLE_LENGTH_RESPONSE           0x80
+/* SCLP event buffer flags */
 #define SCLP_EVENT_BUFFER_ACCEPTED              0x80
 
-#define SCLP_FC_NORMAL_WRITE                    0
+/* Service Call Control Block (SCCB) and its elements */
+#define SCCB_SIZE 4096
 
 typedef struct SCCBHeader {
     uint16_t length;