On Thu, 5 Mar 2020 17:21:44 +0100 Christian Borntraeger <borntrae...@de.ibm.com> wrote:
> > > On 05.03.20 15:25, Christian Borntraeger wrote: > > > > > > On 05.03.20 15:11, Halil Pasic wrote: > >> On Thu, 5 Mar 2020 13:44:31 +0100 > >> Christian Borntraeger <borntrae...@de.ibm.com> wrote: > >> > >>> > >>> > >>> On 25.02.20 15:35, Viktor Mihajlovski wrote: > >>>> > >>>> > >>>> On 2/25/20 12:56 PM, Halil Pasic wrote: > >>>>> On Tue, 25 Feb 2020 10:39:40 +0100 > >>>>> David Hildenbrand <da...@redhat.com> wrote: > >>>>> > >>>>>> On 24.02.20 16:02, Halil Pasic wrote: > >>>>>>> We expose loadparm as a r/w machine property, but if loadparm is set > >>>>>>> by > >>>>>>> the guest via DIAG 308, we don't update the property. Having a > >>>>>>> disconnect between the guest view and the QEMU property is not nice in > >>>>>>> itself, but things get even worse for SCSI, where under certain > >>>>>>> circumstances (see 789b5a401b "s390: Ensure IPL from SCSI works as > >>>>>>> expected" for details) we call s390_gen_initial_iplb() on resets > >>>>>>> effectively overwriting the guest/user supplied loadparm with the > >>>>>>> stale > >>>>>>> value. > >>>>>>> > >>>>>>> Signed-off-by: Halil Pasic <pa...@linux.ibm.com> > >>>>>>> Fixes: 7104bae9de "hw/s390x: provide loadparm property for the > >>>>>>> machine" > >>>>>>> Reported-by: Marc Hartmayer <mhart...@linux.ibm.com> > >>>>>>> Reviewed-by: Janosch Frank <fran...@linux.ibm.com> > >>>>>>> Reviewed-by: Viktor Mihajlovski <mihaj...@linux.ibm.com> > >>>>>>> Tested-by: Marc Hartmayer <mhart...@linux.ibm.com> > >>>>>>> --- > >>>>>>> hw/s390x/ipl.c | 21 +++++++++++++++++++++ > >>>>>>> 1 file changed, 21 insertions(+) > >>>>>>> > >>>>>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > >>>> [...] > >>>>>>> + > >>>>>>> + /* Sync loadparm */ > >>>>>>> + if (iplb->flags & DIAG308_FLAGS_LP_VALID) { > >>>>>>> + char ascii_loadparm[8]; > >>>>>>> + uint8_t *ebcdic_loadparm = iplb->loadparm; > >>>>>>> + int i; > >>>>>>> + > >>>>>>> + for (i = 0; i < 8 && ebcdic_loadparm[i]; i++) { > >>>>>>> + ascii_loadparm[i] = ebcdic2ascii[(uint8_t) > >>>>>>> ebcdic_loadparm[i]]; > >>>>>>> + } > >>>>>>> + ascii_loadparm[i] = 0; > >>>>>>> + object_property_set_str(mo, ascii_loadparm, "loadparm", > >>>>>>> NULL); > >>>>>>> + } else { > >>>>>>> + object_property_set_str(mo, "", "loadparm", NULL); > >>>>>>> + } > >>>>>> > >>>>>> &error_abort instead of NULL, we certainly want to know if this would > >>>>>> ever surprisingly fail. > >>>>> > >>>>> IMHO this is a typical assert() situation where one would like to have > >>>>> a fast and obvious failure when testing, but not in production. > >>>>> > >>>>> AFAIU the guest can trigger this code at any time, and crashing the > >>>>> whole (production) system seems a bit heavy handed to me. The setter > >>>>> should only fail if something is buggy. > >>>>> > >>>>> But if the majority says &error_abort I can certainly do. Other > >>>>> opinions? > >>>>> > >>>> We might consider to return 0x0402 (invalid parameter) from the diag308 > >>>> "set", which is less drastic and would allow the OS to do whatever it > >>>> finds appropriate to deal with the failure. Not that Linux would care > >>>> about that today :-). > >>> > >>> I think it is not an error. It is perfectly fine for a guest to not set > >>> DIAG308_FLAGS_LP_VALID if the guest does not want to set it. The LOADPARM > >>> is supposed to be ignored then. > >>> > >> > >> I believe David's concern was not the else branch, but the last > >> parameter of object_property_set_str(), which tells us what to do if the > >> validation/normalization done by the setter of the loadparm qemu > >> property fails the set operation. > > > > Ah I see. I still think that the guest could provoke the an error by putting > > invalid characters in the loadparm field. So error_abort seems wrong. > > And in fact for that case, the 0x0402 proposal from Viktor seems like the > > right thing to do. > > FWIW, right now we do not check the content of the loadparm and just accept > any kind of garbage via diag308 and we return that garbage. > And I checked what LPAR does. LPAR also does not use 0x0402 and it silently > takes the garbage. > So in essence I would suggest to leave the patch as is. > Ageed. I will do the cosmetics and send out the v2. Regarding validation, I don't know where the criteria Farhan implemented come from. In the longer run we may want to do away with the validation and normalization performed in the setter, but for now I think this is pretty close to the sanest cheap fix we can do. Regards, Hali