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 > > index 7773499d7f..97a279c1a5 100644 > > --- a/hw/s390x/ipl.c > > +++ b/hw/s390x/ipl.c > > @@ -538,6 +538,26 @@ static bool is_virtio_scsi_device(IplParameterBlock > > *iplb) > > return is_virtio_ccw_device_of_type(iplb, VIRTIO_ID_SCSI); > > } > > > > +static void update_machine_ipl_properties(IplParameterBlock *iplb) > > +{ > > + Object *mo = qdev_get_machine(); > > I'd just call this "machine". > I can change that. > > + > > + /* 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? > > > +} > > + > > void s390_ipl_update_diag308(IplParameterBlock *iplb) > > { > > S390IPLState *ipl = get_ipl_device(); > > @@ -545,6 +565,7 @@ void s390_ipl_update_diag308(IplParameterBlock *iplb) > > ipl->iplb = *iplb; > > ipl->iplb_valid = true; > > ipl->netboot = is_virtio_net_device(iplb); > > + update_machine_ipl_properties(iplb); > > } > > > > Somewhat I dislike this manual syncing (and converting back and forth), > but there seems to be no easy way around it. > I share your sentiment. Regards, Halil