| Message ID | 20200309133223.100491-1-pasic@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/1] s390/ipl: sync back loadparm | expand |
On 09.03.20 14:32, 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 <pasic@linux.ibm.com> > Fixes: 7104bae9de ("hw/s390x: provide loadparm property for the machine") > Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com> > Reviewed-by: Janosch Frank <frankja@linux.ibm.com> > Reviewed-by: Viktor Mihajlovski <mihajlov@linux.ibm.com> > Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com> > --- > v1 --> v2: > * tweaked the Fixes tag (Connie) > * s/mo/machine/ (David) > * We decided to not abort if the setter fails. It is not clear where > do the validation logic come from in the first place. For now lets put > out a warning if things go wrong. > The warning we get looks something like: > qemu-system-s390x: warning: LOADPARM: invalid character '?' (ASCII 0x3f) > * I keept the r-b's and the tested-by as the changes are minor. Please > shout at me if you object. > --- > hw/s390x/ipl.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index 9c1ecd423c..8bd50de44c 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -538,6 +538,30 @@ 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 *machine = qdev_get_machine(); > + Error *err = NULL; > + > + /* Sync loadparm */ > + if (iplb->flags & DIAG308_FLAGS_LP_VALID) { > + char ascii_loadparm[8]; > + int i; > + uint8_t *ebcdic_loadparm = iplb->loadparm; Nit: move this to the top Reviewed-by: David Hildenbrand <david@redhat.com>
On Mon, 9 Mar 2020 14:44:20 +0100 David Hildenbrand <david@redhat.com> wrote: > On 09.03.20 14:32, 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 <pasic@linux.ibm.com> > > Fixes: 7104bae9de ("hw/s390x: provide loadparm property for the machine") > > Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com> > > Reviewed-by: Janosch Frank <frankja@linux.ibm.com> > > Reviewed-by: Viktor Mihajlovski <mihajlov@linux.ibm.com> > > Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com> > > --- > > v1 --> v2: > > * tweaked the Fixes tag (Connie) > > * s/mo/machine/ (David) > > * We decided to not abort if the setter fails. It is not clear where > > do the validation logic come from in the first place. For now lets put > > out a warning if things go wrong. > > The warning we get looks something like: > > qemu-system-s390x: warning: LOADPARM: invalid character '?' (ASCII 0x3f) > > * I keept the r-b's and the tested-by as the changes are minor. Please > > shout at me if you object. > > --- > > hw/s390x/ipl.c | 25 +++++++++++++++++++++++++ > > 1 file changed, 25 insertions(+) > > > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > > index 9c1ecd423c..8bd50de44c 100644 > > --- a/hw/s390x/ipl.c > > +++ b/hw/s390x/ipl.c > > @@ -538,6 +538,30 @@ 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 *machine = qdev_get_machine(); > > + Error *err = NULL; > > + > > + /* Sync loadparm */ > > + if (iplb->flags & DIAG308_FLAGS_LP_VALID) { > > + char ascii_loadparm[8]; > > + int i; > > + uint8_t *ebcdic_loadparm = iplb->loadparm; > > Nit: move this to the top Do you mean (reverse xmass tree) + if (iplb->flags & DIAG308_FLAGS_LP_VALID) { + uint8_t *ebcdic_loadparm = iplb->loadparm; + char ascii_loadparm[8]; + int i; or do you mean I should make the declarations function scope instead of block scope? Regards, Halil > > Reviewed-by: David Hildenbrand <david@redhat.com> >
On 09.03.20 15:27, Halil Pasic wrote: > On Mon, 9 Mar 2020 14:44:20 +0100 > David Hildenbrand <david@redhat.com> wrote: > >> On 09.03.20 14:32, 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 <pasic@linux.ibm.com> >>> Fixes: 7104bae9de ("hw/s390x: provide loadparm property for the machine") >>> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com> >>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com> >>> Reviewed-by: Viktor Mihajlovski <mihajlov@linux.ibm.com> >>> Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com> >>> --- >>> v1 --> v2: >>> * tweaked the Fixes tag (Connie) >>> * s/mo/machine/ (David) >>> * We decided to not abort if the setter fails. It is not clear where >>> do the validation logic come from in the first place. For now lets put >>> out a warning if things go wrong. >>> The warning we get looks something like: >>> qemu-system-s390x: warning: LOADPARM: invalid character '?' (ASCII 0x3f) >>> * I keept the r-b's and the tested-by as the changes are minor. Please >>> shout at me if you object. >>> --- >>> hw/s390x/ipl.c | 25 +++++++++++++++++++++++++ >>> 1 file changed, 25 insertions(+) >>> >>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >>> index 9c1ecd423c..8bd50de44c 100644 >>> --- a/hw/s390x/ipl.c >>> +++ b/hw/s390x/ipl.c >>> @@ -538,6 +538,30 @@ 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 *machine = qdev_get_machine(); >>> + Error *err = NULL; >>> + >>> + /* Sync loadparm */ >>> + if (iplb->flags & DIAG308_FLAGS_LP_VALID) { >>> + char ascii_loadparm[8]; >>> + int i; >>> + uint8_t *ebcdic_loadparm = iplb->loadparm; >> >> Nit: move this to the top > > Do you mean (reverse xmass tree) Yep :)
On 09.03.20 14:32, 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 <pasic@linux.ibm.com> > Fixes: 7104bae9de ("hw/s390x: provide loadparm property for the machine") > Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com> > Reviewed-by: Janosch Frank <frankja@linux.ibm.com> > Reviewed-by: Viktor Mihajlovski <mihajlov@linux.ibm.com> > Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com> Thanks applied with the reverse xmas tree change.
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c index 9c1ecd423c..8bd50de44c 100644 --- a/hw/s390x/ipl.c +++ b/hw/s390x/ipl.c @@ -538,6 +538,30 @@ 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 *machine = qdev_get_machine(); + Error *err = NULL; + + /* Sync loadparm */ + if (iplb->flags & DIAG308_FLAGS_LP_VALID) { + char ascii_loadparm[8]; + int i; + uint8_t *ebcdic_loadparm = iplb->loadparm; + + 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(machine, ascii_loadparm, "loadparm", &err); + } else { + object_property_set_str(machine, "", "loadparm", &err); + } + if (err) { + warn_report_err(err); + } +} + void s390_ipl_update_diag308(IplParameterBlock *iplb) { S390IPLState *ipl = get_ipl_device(); @@ -545,6 +569,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); } IplParameterBlock *s390_ipl_get_iplb(void)