| Message ID | 20200224150213.21253-1-pasic@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] s390/ipl: sync back loadparm | expand |
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 <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> > --- > 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". > + > + /* 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. > +} > + > 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.
On Tue, 25 Feb 2020 10:39:40 +0100 David Hildenbrand <david@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 <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> > > --- > > 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
On 2/25/20 12:56 PM, Halil Pasic wrote: > On Tue, 25 Feb 2020 10:39:40 +0100 > David Hildenbrand <david@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 <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> >>> --- >>> 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 :-). [...]
On Tue, 25 Feb 2020 15:35:47 +0100 Viktor Mihajlovski <mihajlov@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 <david@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 <pasic@linux.ibm.com> > >>> Fixes: 7104bae9de "hw/s390x: provide loadparm property for the machine" Please use the format Fixes: <hash> ("subject") > >>> 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> > >>> --- > >>> 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 :)
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 <david@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 <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> >>>> --- >>>> 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. So we have two options: a. leave the patch as-is. This means that we replace the loadparm with an empty string b. remove the else. THis means that we leave the global loadparm unchanged if the guest does not specify one (but it specifies a new IPLB). I will double check what LPAR does.
On Thu, 5 Mar 2020 13:44:31 +0100 Christian Borntraeger <borntraeger@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 <david@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 <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> > >>>> --- > >>>> 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. > So we have two options: > a. leave the patch as-is. This means that we replace the loadparm with an empty string > b. remove the else. THis means that we leave the global loadparm unchanged if the guest does not specify one (but it specifies a new IPLB). > > I will double check what LPAR does. > Thanks! BTW my reading of the architecture and understanding how we expose it via qemu interfaces makes me to lean towards option a). In my understanding we represent invalid loadparm with an empty string in the context of the qemu property. That is we don't expose the garbage-value. Regards, Halil
On 05.03.20 15:11, Halil Pasic wrote: > On Thu, 5 Mar 2020 13:44:31 +0100 > Christian Borntraeger <borntraeger@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 <david@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 <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> >>>>>> --- >>>>>> 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.
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 <borntraeger@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 <david@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 <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> >>>>>>> --- >>>>>>> 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.
On Thu, 5 Mar 2020 17:21:44 +0100 Christian Borntraeger <borntraeger@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 <borntraeger@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 <david@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 <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> > >>>>>>> --- > >>>>>>> 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
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(); + + /* 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); + } +} + 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); } IplParameterBlock *s390_ipl_get_iplb(void)