On Tue, 25 Feb 2020 15:35:47 +0100 Viktor Mihajlovski <mihaj...@linux.ibm.com> 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" Please use the format Fixes: <hash> ("subject") > >>> 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'm not sure if we could actually get there in any other way than via a QEMU coding error... not sure if I would trust QEMU to inject a return code if it already had a code logic fail right before that :)