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

[David Hildenbrand]

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

Reviewed-by: David Hildenbrand <address@hidden>

-- 
Thanks,

David / dhildenb