| Message ID | 20230530124056.18332-3-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixing infinite loop on SCLP READ SCP INFO error | expand |
On 5/30/23 14:40, Pierre Morel wrote: > If SCLP_CMDW_READ_SCP_INFO fails due to a short buffer, retry > with a greater buffer. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> You've been testing using all possible cpus, haven't you? > } > > -static void sclp_read_scp_info(ReadInfo *ri, int length) > +static bool sclp_read_scp_info_extended(unsigned int command, ReadInfo *ri) > +{ > + int cc; > + > + if (!test_facility(140)) { > + report_abort("S390_FEAT_EXTENDED_LENGTH_SCCB missing"); That's the QEMU name for the facility, isn't it? "extended-length-SCCB facility is missing" might be better since that's the name that the architecture specifies for that feature. > + return false; > + } > + if (ri->h.length > (2 * PAGE_SIZE)) { sizeof() would reduce the locations that we have to touch if we ever want to increase the buffer in the future. > + report_abort("SCLP_READ_INFO expected size too big"); > + return false; > + } > + > + sclp_mark_busy(); > + memset(&ri->h, 0, sizeof(ri->h)); > + ri->h.length = 2 * PAGE_SIZE; Same here > + > + cc = sclp_service_call(command, ri); > + if (cc) { > + report_abort("SCLP_READ_INFO error"); > + return false; > + } > + if (ri->h.response_code != SCLP_RC_NORMAL_READ_COMPLETION) { > + report_abort("SCLP_READ_INFO error %02x", ri->h.response_code); > + return false; > + } > + > + return true; > +} > + > +static void sclp_read_scp_info(ReadInfo *ri) > { > unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED, > SCLP_CMDW_READ_SCP_INFO }; > + int length = PAGE_SIZE; > int i, cc; > > for (i = 0; i < ARRAY_SIZE(commands); i++) { > @@ -101,19 +133,29 @@ static void sclp_read_scp_info(ReadInfo *ri, int length) > ri->h.length = length; > > cc = sclp_service_call(commands[i], ri); > - if (cc) > - break; > - if (ri->h.response_code == SCLP_RC_NORMAL_READ_COMPLETION) > + if (cc) { > + report_abort("SCLP_READ_INFO error"); > return; > - if (ri->h.response_code != SCLP_RC_INVALID_SCLP_COMMAND) > + } > + > + switch (ri->h.response_code) { > + case SCLP_RC_NORMAL_READ_COMPLETION: > + return; > + case SCLP_RC_INVALID_SCLP_COMMAND: > break; > + case SCLP_RC_INSUFFICIENT_SCCB_LENGTH: > + sclp_read_scp_info_extended(commands[i], ri); > + return; > + default: > + report_abort("READ_SCP_INFO failed"); > + return; > + } > } > - report_abort("READ_SCP_INFO failed"); > } > > void sclp_read_info(void) > { > - sclp_read_scp_info((void *)_read_info, SCCB_SIZE); Why did you remove that? You could have re-tried with the extended-length in sclp_read_scp_info(). Or you could return the rc and introduce a tiny function that tries both lengths depending on the rc. > + sclp_read_scp_info((void *)_read_info); > read_info = (ReadInfo *)_read_info; > } >
On 6/1/23 10:03, Janosch Frank wrote: > On 5/30/23 14:40, Pierre Morel wrote: >> If SCLP_CMDW_READ_SCP_INFO fails due to a short buffer, retry >> with a greater buffer. >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > > You've been testing using all possible cpus, haven't you? yes up to 248 > >> } >> -static void sclp_read_scp_info(ReadInfo *ri, int length) >> +static bool sclp_read_scp_info_extended(unsigned int command, >> ReadInfo *ri) >> +{ >> + int cc; >> + >> + if (!test_facility(140)) { >> + report_abort("S390_FEAT_EXTENDED_LENGTH_SCCB missing"); > > That's the QEMU name for the facility, isn't it? > "extended-length-SCCB facility is missing" might be better since > that's the name that the architecture specifies for that feature. yes > >> + return false; >> + } >> + if (ri->h.length > (2 * PAGE_SIZE)) { > > sizeof() would reduce the locations that we have to touch if we ever > want to increase the buffer in the future. yes > >> + report_abort("SCLP_READ_INFO expected size too big"); >> + return false; >> + } >> + >> + sclp_mark_busy(); >> + memset(&ri->h, 0, sizeof(ri->h)); >> + ri->h.length = 2 * PAGE_SIZE; > > Same here OK > >> + >> + cc = sclp_service_call(command, ri); >> + if (cc) { >> + report_abort("SCLP_READ_INFO error"); >> + return false; >> + } >> + if (ri->h.response_code != SCLP_RC_NORMAL_READ_COMPLETION) { >> + report_abort("SCLP_READ_INFO error %02x", ri->h.response_code); >> + return false; >> + } >> + >> + return true; >> +} >> + >> +static void sclp_read_scp_info(ReadInfo *ri) >> { >> unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED, >> SCLP_CMDW_READ_SCP_INFO }; >> + int length = PAGE_SIZE; >> int i, cc; >> for (i = 0; i < ARRAY_SIZE(commands); i++) { >> @@ -101,19 +133,29 @@ static void sclp_read_scp_info(ReadInfo *ri, >> int length) >> ri->h.length = length; >> cc = sclp_service_call(commands[i], ri); >> - if (cc) >> - break; >> - if (ri->h.response_code == SCLP_RC_NORMAL_READ_COMPLETION) >> + if (cc) { >> + report_abort("SCLP_READ_INFO error"); >> return; >> - if (ri->h.response_code != SCLP_RC_INVALID_SCLP_COMMAND) >> + } >> + >> + switch (ri->h.response_code) { >> + case SCLP_RC_NORMAL_READ_COMPLETION: >> + return; >> + case SCLP_RC_INVALID_SCLP_COMMAND: >> break; >> + case SCLP_RC_INSUFFICIENT_SCCB_LENGTH: >> + sclp_read_scp_info_extended(commands[i], ri); >> + return; >> + default: >> + report_abort("READ_SCP_INFO failed"); >> + return; >> + } >> } >> - report_abort("READ_SCP_INFO failed"); >> } >> void sclp_read_info(void) >> { >> - sclp_read_scp_info((void *)_read_info, SCCB_SIZE); > > Why did you remove that? > You could have re-tried with the extended-length in > sclp_read_scp_info(). Or you could return the rc and introduce a tiny > function that tries both lengths depending on the rc. Yes, I can let it here. I found it has little sense to give the length as parameter. Retrying with extended length in sclp_read_scp_info() is what is done isn'it? It does not change a lot to let the first used size here so I will let it here. > >> + sclp_read_scp_info((void *)_read_info); >> read_info = (ReadInfo *)_read_info; >> } >
Quoting Janosch Frank (2023-06-01 10:03:06) > On 5/30/23 14:40, Pierre Morel wrote: > > If SCLP_CMDW_READ_SCP_INFO fails due to a short buffer, retry > > with a greater buffer. > > > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> Janosch, I think it makes sense if Pierre picks up Claudios suggestion from here: https://lore.kernel.org/all/20230530173544.378a63c6@p-imbrenda/ Do you agree?
On 6/1/23 13:59, Nico Boehr wrote: > Quoting Janosch Frank (2023-06-01 10:03:06) >> On 5/30/23 14:40, Pierre Morel wrote: >>> If SCLP_CMDW_READ_SCP_INFO fails due to a short buffer, retry >>> with a greater buffer. >>> >>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > Janosch, I think it makes sense if Pierre picks up Claudios suggestion from here: > https://lore.kernel.org/all/20230530173544.378a63c6@p-imbrenda/ > > Do you agree? from my side: It simplifies greatly the code and tested without problem. The documentation says the SCCB length is "at least"... so we can use a greater size from the beginning.
On 6/1/23 14:55, Pierre Morel wrote: > > On 6/1/23 13:59, Nico Boehr wrote: >> Quoting Janosch Frank (2023-06-01 10:03:06) >>> On 5/30/23 14:40, Pierre Morel wrote: >>>> If SCLP_CMDW_READ_SCP_INFO fails due to a short buffer, retry >>>> with a greater buffer. >>>> >>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> Janosch, I think it makes sense if Pierre picks up Claudios suggestion from here: >> https://lore.kernel.org/all/20230530173544.378a63c6@p-imbrenda/ >> >> Do you agree? > > from my side: > > It simplifies greatly the code and tested without problem. > > The documentation says the SCCB length is "at least"... so we can use a > greater size from the beginning. > > Sounds good
diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c index 34a31da..9d51ca4 100644 --- a/lib/s390x/sclp.c +++ b/lib/s390x/sclp.c @@ -17,13 +17,14 @@ #include "sclp.h" #include <alloc_phys.h> #include <alloc_page.h> +#include <asm/facility.h> extern unsigned long stacktop; static uint64_t storage_increment_size; static uint64_t max_ram_size; static uint64_t ram_size; -char _read_info[PAGE_SIZE] __attribute__((__aligned__(PAGE_SIZE))); +char _read_info[2 * PAGE_SIZE] __attribute__((__aligned__(PAGE_SIZE))); static ReadInfo *read_info; struct sclp_facilities sclp_facilities; @@ -89,10 +90,41 @@ void sclp_clear_busy(void) spin_unlock(&sclp_lock); } -static void sclp_read_scp_info(ReadInfo *ri, int length) +static bool sclp_read_scp_info_extended(unsigned int command, ReadInfo *ri) +{ + int cc; + + if (!test_facility(140)) { + report_abort("S390_FEAT_EXTENDED_LENGTH_SCCB missing"); + return false; + } + if (ri->h.length > (2 * PAGE_SIZE)) { + report_abort("SCLP_READ_INFO expected size too big"); + return false; + } + + sclp_mark_busy(); + memset(&ri->h, 0, sizeof(ri->h)); + ri->h.length = 2 * PAGE_SIZE; + + cc = sclp_service_call(command, ri); + if (cc) { + report_abort("SCLP_READ_INFO error"); + return false; + } + if (ri->h.response_code != SCLP_RC_NORMAL_READ_COMPLETION) { + report_abort("SCLP_READ_INFO error %02x", ri->h.response_code); + return false; + } + + return true; +} + +static void sclp_read_scp_info(ReadInfo *ri) { unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED, SCLP_CMDW_READ_SCP_INFO }; + int length = PAGE_SIZE; int i, cc; for (i = 0; i < ARRAY_SIZE(commands); i++) { @@ -101,19 +133,29 @@ static void sclp_read_scp_info(ReadInfo *ri, int length) ri->h.length = length; cc = sclp_service_call(commands[i], ri); - if (cc) - break; - if (ri->h.response_code == SCLP_RC_NORMAL_READ_COMPLETION) + if (cc) { + report_abort("SCLP_READ_INFO error"); return; - if (ri->h.response_code != SCLP_RC_INVALID_SCLP_COMMAND) + } + + switch (ri->h.response_code) { + case SCLP_RC_NORMAL_READ_COMPLETION: + return; + case SCLP_RC_INVALID_SCLP_COMMAND: break; + case SCLP_RC_INSUFFICIENT_SCCB_LENGTH: + sclp_read_scp_info_extended(commands[i], ri); + return; + default: + report_abort("READ_SCP_INFO failed"); + return; + } } - report_abort("READ_SCP_INFO failed"); } void sclp_read_info(void) { - sclp_read_scp_info((void *)_read_info, SCCB_SIZE); + sclp_read_scp_info((void *)_read_info); read_info = (ReadInfo *)_read_info; }
If SCLP_CMDW_READ_SCP_INFO fails due to a short buffer, retry with a greater buffer. Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> --- lib/s390x/sclp.c | 58 +++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 50 insertions(+), 8 deletions(-)