Re: [PATCH v4 16/19] s390x: Rebuild IPLB for SCSI device directly from DIAG308

[Thomas Huth <thuth@redhat.com>]

On 17/10/2024 03.47, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> Because virtio-scsi type devices use a non-architected IPLB pbt code they cannot
> be set and stored normally. Instead, the IPLB must be rebuilt during re-ipl.
> 
> As s390x does not natively support multiple boot devices, the devno field is
> used to store the position in the boot order for the device.
> 
> Handling the rebuild as part of DIAG308 removes the need to check the devices
> for invalid IPLBs later in the IPL.
> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
>   hw/s390x/ipl.h              | 11 ++++--
>   include/hw/s390x/ipl/qipl.h |  3 +-
>   hw/s390x/ipl.c              | 75 +++++++------------------------------
>   pc-bios/s390-ccw/jump2ipl.c | 11 ++++--
>   target/s390x/diag.c         |  9 ++++-
>   5 files changed, 39 insertions(+), 70 deletions(-)
> 
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index 54eb48fd6e..aead1d6ae5 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -24,6 +24,7 @@
>   
>   void s390_ipl_convert_loadparm(char *ascii_lp, uint8_t *ebcdic_lp);
>   void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp);
> +void s390_rebuild_iplb(uint16_t index, IplParameterBlock *iplb);
>   void s390_ipl_update_diag308(IplParameterBlock *iplb);
>   int s390_ipl_prepare_pv_header(Error **errp);
>   int s390_ipl_pv_unpack(void);
> @@ -65,7 +66,8 @@ struct S390IPLState {
>       bool enforce_bios;
>       bool iplb_valid;
>       bool iplb_valid_pv;
> -    bool netboot;
> +    bool rebuilt_iplb;
> +    uint16_t iplb_index;
>       /* reset related properties don't have to be migrated or reset */
>       enum s390_reset reset_type;
>       int reset_cpu_index;
> @@ -172,11 +174,14 @@ static inline bool iplb_valid_pv(IplParameterBlock *iplb)
>   
>   static inline bool iplb_valid(IplParameterBlock *iplb)
>   {
> +    uint32_t len = be32_to_cpu(iplb->len);
> +
>       switch (iplb->pbt) {
>       case S390_IPL_TYPE_FCP:
> -        return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN;
> +        return len >= S390_IPLB_MIN_FCP_LEN;
>       case S390_IPL_TYPE_CCW:
> -        return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN;
> +        return (len >= S390_IPLB_MIN_CCW_LEN);

Style inconsistency: In the FCP case, you did the return statement without 
parentheses, so I'd suggest to remove them here, too.

> +    case S390_IPL_TYPE_QEMU_SCSI:
>       default:
>           return false;
>       }
> diff --git a/include/hw/s390x/ipl/qipl.h b/include/hw/s390x/ipl/qipl.h
> index e56b181298..451f3b1684 100644
> --- a/include/hw/s390x/ipl/qipl.h
> +++ b/include/hw/s390x/ipl/qipl.h
> @@ -28,7 +28,8 @@
>    */
>   struct QemuIplParameters {
>       uint8_t  qipl_flags;
> -    uint8_t  reserved1[3];
> +    uint8_t  index;
> +    uint8_t  reserved1[2];
>       uint64_t reserved2;
>       uint32_t boot_menu_timeout;
>       uint8_t  reserved3[2];
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index aec79f41a8..50fde05b67 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -448,7 +448,6 @@ void s390_ipl_convert_loadparm(char *ascii_lp, uint8_t *ebcdic_lp)
>   
>   static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
>   {
> -    S390IPLState *ipl = get_ipl_device();
>       CcwDevice *ccw_dev = NULL;
>       SCSIDevice *sd;
>       int devtype;
> @@ -481,9 +480,6 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
>               iplb->ccw.ssid = ccw_dev->sch->ssid & 3;
>               break;
>           case CCW_DEVTYPE_VIRTIO_NET:
> -            /* The S390IPLState netboot is true if ANY IPLB may use netboot */
> -            ipl->netboot = true;
> -            /* Fall through to CCW_DEVTYPE_VIRTIO case */
>           case CCW_DEVTYPE_VIRTIO:
>               iplb->len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN);
>               iplb->blk0_len =
> @@ -508,6 +504,17 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
>       return false;
>   }
>   
> +

Remove the additonal empty line, please.

> +void s390_rebuild_iplb(uint16_t dev_index, IplParameterBlock *iplb)
> +{
> +    S390IPLState *ipl = get_ipl_device();
> +    uint16_t index;
> +    index = ipl->rebuilt_iplb ? ipl->iplb_index : dev_index;
> +
> +    ipl->rebuilt_iplb = s390_build_iplb(get_boot_device(index), iplb);
> +    ipl->iplb_index = index;
> +}

With the nits fixed:
Acked-by: Thomas Huth <thuth@redhat.com>


  Thomas