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

[PATCH 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>
---
 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();
+
+    /* 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)

base-commit: c1e667d2598b9b3ce62b8e89ed22dd38dfe9f57f
-- 
2.17.1

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

[David Hildenbrand]

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.

-- 
Thanks,

David / dhildenb

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

[Halil Pasic]

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

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

[Viktor Mihajlovski]

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

[...]
-- 
Kind Regards,
    Viktor

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

[Cornelia Huck]

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

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

[Christian Borntraeger]

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.

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

[Halil Pasic]

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

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

[Christian Borntraeger]

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.

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

[Christian Borntraeger]

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.

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

[Halil Pasic]

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