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

[Halil Pasic <pasic@linux.ibm.com>]

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