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

[Christian Borntraeger]

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.