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