On 09.03.20 15:27, Halil Pasic wrote:
> On Mon, 9 Mar 2020 14:44:20 +0100
> David Hildenbrand <da...@redhat.com> wrote:
>
>> On 09.03.20 14:32, 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 <pa...@linux.ibm.com>
>>> Fixes: 7104bae9de ("hw/s390x: provide loadparm property for the machine")
>>> Reported-by: Marc Hartmayer <mhart...@linux.ibm.com>
>>> Reviewed-by: Janosch Frank <fran...@linux.ibm.com>
>>> Reviewed-by: Viktor Mihajlovski <mihaj...@linux.ibm.com>
>>> Tested-by: Marc Hartmayer <mhart...@linux.ibm.com>
>>> ---
>>> v1 --> v2:
>>> * tweaked the Fixes tag (Connie)
>>> * s/mo/machine/ (David)
>>> * We decided to not abort if the setter fails. It is not clear where
>>> do the validation logic come from in the first place. For now lets put
>>> out a warning if things go wrong.
>>> The warning we get looks something like:
>>> qemu-system-s390x: warning: LOADPARM: invalid character '?' (ASCII 0x3f)
>>> * I keept the r-b's and the tested-by as the changes are minor. Please
>>> shout at me if you object.
>>> ---
>>> hw/s390x/ipl.c | 25 +++++++++++++++++++++++++
>>> 1 file changed, 25 insertions(+)
>>>
>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>>> index 9c1ecd423c..8bd50de44c 100644
>>> --- a/hw/s390x/ipl.c
>>> +++ b/hw/s390x/ipl.c
>>> @@ -538,6 +538,30 @@ 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 *machine = qdev_get_machine();
>>> + Error *err = NULL;
>>> +
>>> + /* Sync loadparm */
>>> + if (iplb->flags & DIAG308_FLAGS_LP_VALID) {
>>> + char ascii_loadparm[8];
>>> + int i;
>>> + uint8_t *ebcdic_loadparm = iplb->loadparm;
>>
>> Nit: move this to the top
>
> Do you mean (reverse xmass tree)
Yep :)
--
Thanks,
David / dhildenb
