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 <address@hidden> 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 <address@hidden>
> > > Fixes: 7104bae9de "hw/s390x: provide loadparm property for the machine"
> > > Reported-by: Marc Hartmayer <address@hidden>
> > > Reviewed-by: Janosch Frank <address@hidden>
> > > Reviewed-by: Viktor Mihajlovski <address@hidden>
> > > Tested-by: Marc Hartmayer <address@hidden>
> > > ---
> > >   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