[PATCH v2 1/1] s390/ipl: sync back loadparm

[PATCH v2 1/1] s390/ipl: sync back loadparm

[Halil Pasic]

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;
+
+        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)

base-commit: 67f17e23baca5dd545fe98b01169cc351a70fe35
-- 
2.17.1

Re: [PATCH v2 1/1] s390/ipl: sync back loadparm

[David Hildenbrand]

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>

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 1/1] s390/ipl: sync back loadparm

[Halil Pasic]

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>
> 

Re: [PATCH v2 1/1] s390/ipl: sync back loadparm

[David Hildenbrand]

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 :)


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 1/1] s390/ipl: sync back loadparm

[Christian Borntraeger]

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.