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

[Cornelia Huck <cohuck@redhat.com>]

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