| Message ID | 20181214100614.11049-1-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests,v1] s390x: try !FORCE SCLP read SCP info if FORCED is unknown | expand |
On 14.12.18 11:06, 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 0x80 (no idea what that means) It's great that they set that and never actually check for its effects... It's (drum roll): 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. > Also, let's just set the other bits in the mask to 0, not sure if they > will actually be used, but this is definetly not wrong. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- I rebased my series on top of this patch and it works under LPAR: Tested-by: Janosch Frank <frankja@linux.ibm.com> Please update the description, but apart from that: Reviewed-by: Janosch Frank <frankja@linux.ibm.com> [...] > +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] = 0x80; We could test for the SCLP_RC_INSUFFICIENT_SCCB_LENGTH and redrive with a longer response space because we set SCLP_VARIABLE_LENGTH_RESPONSE but 4k should be enough for tests. I do not expect this to be executed with a few hundred VCPUs anyway. > + > + 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; [...] > diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h > index 21d482b..131813e 100644 > --- a/lib/s390x/sclp.h > +++ b/lib/s390x/sclp.h > @@ -76,6 +76,8 @@ > #define SCLP_EVENT_BUFFER_ACCEPTED 0x80 > > #define SCLP_FC_NORMAL_WRITE 0 > +#define SCLP_FC_SINGLE_INCREMENT_ASSIGN 0x40 > +#define SCLP_FC_DUMP_INDICATOR 0x80 If you want I could take that into my cleanups. > > typedef struct SCCBHeader { > uint16_t length; >
On 14.12.18 18:24, Janosch Frank wrote: > On 14.12.18 11:06, 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 0x80 (no idea what that means) > > It's great that they set that and never actually check for its effects... > > It's (drum roll): > SCLP_VARIABLE_LENGTH_RESPONSE Right, it's used for that purpose in QEMU. I'll do some changes in this patch - Rename SCLP_VARIABLE_LENGTH_RESPONSE to SCLP_CM2_VARIABLE_LENGTH_RESPONSE and use it - Move the SCLP_FC_* to a dedicated section in the header - Set the control_mask and function_code for every sclp_service_call() user to deterinistic values (0) > >> >> 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. >> Also, let's just set the other bits in the mask to 0, not sure if they >> will actually be used, but this is definetly not wrong. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- > > I rebased my series on top of this patch and it works under LPAR: > Tested-by: Janosch Frank <frankja@linux.ibm.com> > > Please update the description, but apart from that: > Reviewed-by: Janosch Frank <frankja@linux.ibm.com> Thanks, I'll resend with the mentioned changes. We can than decide if you want to carry this along or if Thomas wants to queue this right away. > > [...] >> +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] = 0x80; > > We could test for the SCLP_RC_INSUFFICIENT_SCCB_LENGTH and redrive with > a longer response space because we set SCLP_VARIABLE_LENGTH_RESPONSE but > 4k should be enough for tests. I do not expect this to be executed with > a few hundred VCPUs anyway. Indeed, I think we can ignore that for now. Thanks!
diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c index 1d4a010..8224553 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] = 0x80; + + 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..131813e 100644 --- a/lib/s390x/sclp.h +++ b/lib/s390x/sclp.h @@ -76,6 +76,8 @@ #define SCLP_EVENT_BUFFER_ACCEPTED 0x80 #define SCLP_FC_NORMAL_WRITE 0 +#define SCLP_FC_SINGLE_INCREMENT_ASSIGN 0x40 +#define SCLP_FC_DUMP_INDICATOR 0x80 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 0x80 (no idea what that means) 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. Also, let's just set the other bits in the mask to 0, not sure if they will actually be used, but this is definetly not wrong. Signed-off-by: David Hildenbrand <david@redhat.com> --- Quickly tested under kvm (on top of z/VM) and under tcg. lib/s390x/sclp.c | 26 ++++++++++++++++++++++++-- lib/s390x/sclp.h | 2 ++ 2 files changed, 26 insertions(+), 2 deletions(-)