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

[Halil Pasic]

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