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

[Halil Pasic]

On Thu, 5 Mar 2020 17:21:44 +0100
Christian Borntraeger <address@hidden> 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 <address@hidden> 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 <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 :-).
> >>>
> >>> 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