Re: [PATCH 09/12] pc-bios/s390-ccw: Split virtio-scsi code from virtio_blk_setup_device()

[Eric Farman]

On Tue, 2022-06-28 at 15:10 +0200, Thomas Huth wrote:
> The next patch is going to add more virtio-block specific code to
> virtio_blk_setup_device(), and if the virtio-scsi code is also in
> there, this is more cumbersome. And the calling function
> virtio_setup()
> in main.c looks at the device type already anyway, so it's more
> logical to separate the virtio-scsi stuff into a new function in
> virtio-scsi.c instead.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

I think this is a good untangling.

Reviewed-by: Eric Farman <farman@linux.ibm.com>

> ---
>  pc-bios/s390-ccw/virtio-scsi.h   |  2 +-
>  pc-bios/s390-ccw/main.c          | 24 +++++++++++++++++-------
>  pc-bios/s390-ccw/virtio-blkdev.c | 20 ++------------------
>  pc-bios/s390-ccw/virtio-scsi.c   | 19 ++++++++++++++++++-
>  4 files changed, 38 insertions(+), 27 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/virtio-scsi.h b/pc-bios/s390-
> ccw/virtio-scsi.h
> index 4b14c2c2f9..e6b6cd4815 100644
> --- a/pc-bios/s390-ccw/virtio-scsi.h
> +++ b/pc-bios/s390-ccw/virtio-scsi.h
> @@ -67,8 +67,8 @@ static inline bool virtio_scsi_response_ok(const
> VirtioScsiCmdResp *r)
>          return r->response == VIRTIO_SCSI_S_OK && r->status ==
> CDB_STATUS_GOOD;
>  }
>  
> -int virtio_scsi_setup(VDev *vdev);
>  int virtio_scsi_read_many(VDev *vdev,
>                            ulong sector, void *load_addr, int
> sec_num);
> +int virtio_scsi_setup_device(SubChannelId schid);
>  
>  #endif /* VIRTIO_SCSI_H */
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 835341457d..a2def83e82 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -14,6 +14,7 @@
>  #include "s390-ccw.h"
>  #include "cio.h"
>  #include "virtio.h"
> +#include "virtio-scsi.h"
>  #include "dasd-ipl.h"
>  
>  char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
> @@ -218,6 +219,7 @@ static int virtio_setup(void)
>  {
>      VDev *vdev = virtio_get_device();
>      QemuIplParameters *early_qipl = (QemuIplParameters
> *)QIPL_ADDRESS;
> +    int ret;
>  
>      memcpy(&qipl, early_qipl, sizeof(QemuIplParameters));
>  
> @@ -225,18 +227,26 @@ static int virtio_setup(void)
>          menu_setup();
>      }
>  
> -    if (virtio_get_device_type() == VIRTIO_ID_NET) {
> +    switch (vdev->senseid.cu_model) {
> +    case VIRTIO_ID_NET:
>          sclp_print("Network boot device detected\n");
>          vdev->netboot_start_addr = qipl.netboot_start_addr;
> -    } else {
> -        int ret = virtio_blk_setup_device(blk_schid);
> -        if (ret) {
> -            return ret;
> -        }
> +        return 0;
> +    case VIRTIO_ID_BLOCK:
> +        ret = virtio_blk_setup_device(blk_schid);
> +        break;
> +    case VIRTIO_ID_SCSI:
> +        ret = virtio_scsi_setup_device(blk_schid);
> +        break;
> +    default:
> +        panic("\n! No IPL device available !\n");
> +    }
> +
> +    if (!ret) {
>          IPL_assert(virtio_ipl_disk_is_valid(), "No valid IPL device
> detected");
>      }
>  
> -    return 0;
> +    return ret;
>  }
>  
>  static void ipl_boot_device(void)
> diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-
> ccw/virtio-blkdev.c
> index 11820754f3..a2b157b2c0 100644
> --- a/pc-bios/s390-ccw/virtio-blkdev.c
> +++ b/pc-bios/s390-ccw/virtio-blkdev.c
> @@ -222,27 +222,11 @@ uint64_t virtio_get_blocks(void)
>  int virtio_blk_setup_device(SubChannelId schid)
>  {
>      VDev *vdev = virtio_get_device();
> -    int ret = 0;
>  
>      vdev->schid = schid;
>      virtio_setup_ccw(vdev);
>  
> -    switch (vdev->senseid.cu_model) {
> -    case VIRTIO_ID_BLOCK:
> -        sclp_print("Using virtio-blk.\n");
> -        break;
> -    case VIRTIO_ID_SCSI:
> -        IPL_assert(vdev->config.scsi.sense_size ==
> VIRTIO_SCSI_SENSE_SIZE,
> -            "Config: sense size mismatch");
> -        IPL_assert(vdev->config.scsi.cdb_size ==
> VIRTIO_SCSI_CDB_SIZE,
> -            "Config: CDB size mismatch");
> +    sclp_print("Using virtio-blk.\n");
>  
> -        sclp_print("Using virtio-scsi.\n");
> -        ret = virtio_scsi_setup(vdev);
> -        break;
> -    default:
> -        panic("\n! No IPL device available !\n");
> -    }
> -
> -    return ret;
> +    return 0;
>  }
> diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-
> ccw/virtio-scsi.c
> index 2c8d0f3097..3b7069270c 100644
> --- a/pc-bios/s390-ccw/virtio-scsi.c
> +++ b/pc-bios/s390-ccw/virtio-scsi.c
> @@ -329,7 +329,7 @@ static void scsi_parse_capacity_report(void
> *data,
>      }
>  }
>  
> -int virtio_scsi_setup(VDev *vdev)
> +static int virtio_scsi_setup(VDev *vdev)
>  {
>      int retry_test_unit_ready = 3;
>      uint8_t data[256];
> @@ -430,3 +430,20 @@ int virtio_scsi_setup(VDev *vdev)
>  
>      return 0;
>  }
> +
> +int virtio_scsi_setup_device(SubChannelId schid)
> +{
> +    VDev *vdev = virtio_get_device();
> +
> +    vdev->schid = schid;
> +    virtio_setup_ccw(vdev);
> +
> +    IPL_assert(vdev->config.scsi.sense_size ==
> VIRTIO_SCSI_SENSE_SIZE,
> +               "Config: sense size mismatch");
> +    IPL_assert(vdev->config.scsi.cdb_size == VIRTIO_SCSI_CDB_SIZE,
> +               "Config: CDB size mismatch");
> +
> +    sclp_print("Using virtio-scsi.\n");
> +
> +    return virtio_scsi_setup(vdev);
> +}