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

[David Hildenbrand <david@redhat.com>]

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 <pasic@linux.ibm.com>
> Fixes: 7104bae9de "hw/s390x: provide loadparm property for the machine"
> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Viktor Mihajlovski <mihajlov@linux.ibm.com>
> Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>  hw/s390x/ipl.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 7773499d7f..97a279c1a5 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -538,6 +538,26 @@ 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 *mo = qdev_get_machine();

I'd just call this "machine".

> +
> +    /* 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.

> +}
> +
>  void s390_ipl_update_diag308(IplParameterBlock *iplb)
>  {
>      S390IPLState *ipl = get_ipl_device();
> @@ -545,6 +565,7 @@ void s390_ipl_update_diag308(IplParameterBlock *iplb)
>      ipl->iplb = *iplb;
>      ipl->iplb_valid = true;
>      ipl->netboot = is_virtio_net_device(iplb);
> +    update_machine_ipl_properties(iplb);
>  }
>  

Somewhat I dislike this manual syncing (and converting back and forth),
but there seems to be no easy way around it.

-- 
Thanks,

David / dhildenb