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;