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

[Christian Borntraeger]

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.