| Message ID | 20181217101651.11723-1-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests,v2] s390x: try !FORCE SCLP read SCP info if FORCED is unknown | expand |
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; >
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?
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.
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.
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;
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(-)