Re: [PATCH 2/7] pc-bios/s390-ccw: Split virtio-ccw and generic virtio

[Thomas Huth <thuth@redhat.com>]

  Hi Jared!

On 20/10/2025 18.20, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> Separate the CCW specific virtio routines and create generic wrappers for easier
> reuse of existing virtio functions with non-CCW devices.
> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
>   hw/s390x/ipl.h                   |   5 -
>   include/hw/s390x/ipl/qipl.h      |   6 +
>   pc-bios/s390-ccw/iplb.h          |   4 -
>   pc-bios/s390-ccw/virtio-ccw.h    |  25 ++++
>   pc-bios/s390-ccw/virtio-scsi.h   |   2 +-
>   pc-bios/s390-ccw/virtio.h        |  11 +-
>   pc-bios/s390-ccw/main.c          |  13 +-
>   pc-bios/s390-ccw/virtio-blkdev.c |  57 +++++---
>   pc-bios/s390-ccw/virtio-ccw.c    | 240 +++++++++++++++++++++++++++++++
>   pc-bios/s390-ccw/virtio-net.c    |   5 +-
>   pc-bios/s390-ccw/virtio-scsi.c   |   7 +-
>   pc-bios/s390-ccw/virtio.c        | 209 +++++----------------------
>   pc-bios/s390-ccw/Makefile        |   3 +-
>   13 files changed, 367 insertions(+), 220 deletions(-)
>   create mode 100644 pc-bios/s390-ccw/virtio-ccw.h
>   create mode 100644 pc-bios/s390-ccw/virtio-ccw.c
> 
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index 505cded490..aec2244321 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -103,11 +103,6 @@ QEMU_BUILD_BUG_MSG(offsetof(S390IPLState, iplb) & 3, "alignment of iplb wrong");
>   #define DIAG308_PV_STORE            9
>   #define DIAG308_PV_START            10
>   
> -#define S390_IPL_TYPE_FCP 0x00
> -#define S390_IPL_TYPE_CCW 0x02
> -#define S390_IPL_TYPE_PV 0x05
> -#define S390_IPL_TYPE_QEMU_SCSI 0xff
> -
>   #define S390_IPLB_HEADER_LEN 8
>   #define S390_IPLB_MIN_PV_LEN 148
>   #define S390_IPLB_MIN_CCW_LEN 200
> diff --git a/include/hw/s390x/ipl/qipl.h b/include/hw/s390x/ipl/qipl.h
> index 6824391111..aadab87c2e 100644
> --- a/include/hw/s390x/ipl/qipl.h
> +++ b/include/hw/s390x/ipl/qipl.h
> @@ -20,6 +20,12 @@
>   #define LOADPARM_LEN    8
>   #define NO_LOADPARM "\0\0\0\0\0\0\0\0"
>   
> +#define S390_IPL_TYPE_FCP 0x00
> +#define S390_IPL_TYPE_CCW 0x02
> +#define S390_IPL_TYPE_PCI 0x04
> +#define S390_IPL_TYPE_PV 0x05
> +#define S390_IPL_TYPE_QEMU_SCSI 0xff
> +
>   /*
>    * The QEMU IPL Parameters will be stored at absolute address
>    * 204 (0xcc) which means it is 32-bit word aligned but not
> diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
> index 08f259ff31..926e8eed5d 100644
> --- a/pc-bios/s390-ccw/iplb.h
> +++ b/pc-bios/s390-ccw/iplb.h
> @@ -23,10 +23,6 @@ extern QemuIplParameters qipl;
>   extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
>   extern bool have_iplb;
>   
> -#define S390_IPL_TYPE_FCP 0x00
> -#define S390_IPL_TYPE_CCW 0x02
> -#define S390_IPL_TYPE_QEMU_SCSI 0xff

This patch is doing quite a bit of different changes at once, making it hard 
to review ... It would be nice if you could at least move the 
"S390_IPL_TYPE_*" movement into a separate patch.

> diff --git a/pc-bios/s390-ccw/virtio-ccw.h b/pc-bios/s390-ccw/virtio-ccw.h
> new file mode 100644
> index 0000000000..366c4812af
> --- /dev/null
> +++ b/pc-bios/s390-ccw/virtio-ccw.h
> @@ -0,0 +1,25 @@
> +/*
> + * Virtio definitions for CCW devices
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Jared Rossi <jrossi@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef VIRTIO_CCW_H
> +#define VIRTIO_CCW_H
> +
> +/* main.c */
> +extern SubChannelId blk_schid;
> +
> +/* virtio-ccw.c */
> +int run_ccw(VDev *vdev, int cmd, void *ptr, int len, bool sli);
> +int drain_irqs_ccw(SubChannelId schid);
> +bool virtio_ccw_is_supported(SubChannelId schid);
> +int virtio_ccw_run(VDev *vdev, int vqid, VirtioCmd *cmd);
> +long virtio_ccw_notify(SubChannelId schid, int vq_idx, long cookie);
> +int virtio_ccw_setup(VDev *vdev);
> +int virtio_ccw_reset(VDev *vdev);
> +
> +#endif
> diff --git a/pc-bios/s390-ccw/virtio-scsi.h b/pc-bios/s390-ccw/virtio-scsi.h
> index c5612e16a2..7a37f8b45a 100644
> --- a/pc-bios/s390-ccw/virtio-scsi.h
> +++ b/pc-bios/s390-ccw/virtio-scsi.h
> @@ -69,6 +69,6 @@ static inline bool virtio_scsi_response_ok(const VirtioScsiCmdResp *r)
>   
>   int virtio_scsi_read_many(VDev *vdev,
>                             unsigned long sector, void *load_addr, int sec_num);
> -int virtio_scsi_setup_device(SubChannelId schid);
> +int virtio_scsi_setup_device(void);
>   
>   #endif /* VIRTIO_SCSI_H */
> diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
> index 597bd42358..1c1017a0db 100644
> --- a/pc-bios/s390-ccw/virtio.h
> +++ b/pc-bios/s390-ccw/virtio.h
> @@ -109,6 +109,8 @@ struct VRing {
>   };
>   typedef struct VRing VRing;
>   
> +char *virtio_get_ring_area(void);
> +
>   
>   /***********************************************
>    *               Virtio block                  *
> @@ -138,6 +140,9 @@ typedef struct VRing VRing;
>   /* Barrier before this op. */
>   #define VIRTIO_BLK_T_BARRIER    0x80000000
>   
> +/* For device bus switches */
> +extern int ipl_type;
> +
>   /* This is the first element of the read scatter-gather list. */
>   struct VirtioBlkOuthdr {
>           /* VIRTIO_BLK_T* */
> @@ -236,6 +241,7 @@ struct VDev {
>       int cmd_vr_idx;
>       void *ring_area;
>       long wait_reply_timeout;
> +    VirtioDevType type;
>       VirtioGDN guessed_disk_nature;
>       SubChannelId schid;
>       SenseId senseid;
> @@ -268,8 +274,9 @@ struct VirtioCmd {
>   };
>   typedef struct VirtioCmd VirtioCmd;
>   
> +void vring_init(VRing *vr, VqInfo *info);
>   bool vring_notify(VRing *vr);
> -int drain_irqs(SubChannelId schid);
> +int drain_irqs(VRing *vr);
>   void vring_send_buf(VRing *vr, void *p, int len, int flags);
>   int vr_poll(VRing *vr);
>   int vring_wait_reply(void);
> @@ -282,7 +289,7 @@ int virtio_net_init(void *mac_addr);
>   void virtio_net_deinit(void);
>   
>   /* virtio-blkdev.c */
> -int virtio_blk_setup_device(SubChannelId schid);
> +int virtio_blk_setup_device(void);
>   int virtio_read(unsigned long sector, void *load_addr);
>   unsigned long virtio_load_direct(unsigned long rec_list1, unsigned long rec_list2,
>                                    void *load_addr);
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 76bf743900..7299b8911f 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -19,11 +19,12 @@
>   #include "virtio-scsi.h"
>   #include "dasd-ipl.h"
>   
> -static SubChannelId blk_schid = { .one = 1 };
> +SubChannelId blk_schid = { .one = 1 };
>   static char loadparm_str[LOADPARM_LEN + 1];
>   QemuIplParameters qipl;
>   IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
>   bool have_iplb;
> +int ipl_type;

Having two new global variables to pass around information is somewhat ugly...

>   static uint16_t cutype;
>   LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
>   
> @@ -216,7 +217,7 @@ static bool find_boot_device(void)
>       VDev *vdev = virtio_get_device();
>       bool found = false;
>   
> -    switch (iplb.pbt) {
> +    switch (ipl_type) {
>       case S390_IPL_TYPE_CCW:
>           vdev->scsi_device_selected = false;

... could we maybe set up vdev->schid here already (or in virtio_setup 
below) ...

>           debug_print_int("device no. ", iplb.ccw.devno);
> @@ -245,15 +246,15 @@ static int virtio_setup(void)
>       vdev->is_cdrom = false;
>       int ret;
>   
> -    switch (vdev->senseid.cu_model) {
> +    switch (vdev->type) {
>       case VIRTIO_ID_NET:
>           puts("Network boot device detected");
>           return 0;
>       case VIRTIO_ID_BLOCK:
> -        ret = virtio_blk_setup_device(blk_schid);
> +        ret = virtio_blk_setup_device();

... and then simply pass vdev as parameter to virtio_blk_setup_device() here ...

>           break;
>       case VIRTIO_ID_SCSI:
> -        ret = virtio_scsi_setup_device(blk_schid);
> +        ret = virtio_scsi_setup_device();

... and here.

Then we don't have to make blk_schid global.
(Please also make this as a separate patch first, since this one here is 
already quite complicated).

Would it also be possible to store ipl_type in the VDev, so we don't have to 
make this variable global?

>           break;
>       default:
>           puts("\n! No IPL device available !\n");
> @@ -316,11 +317,13 @@ void main(void)
>       css_setup();
>       have_iplb = store_iplb(&iplb);
>       if (!have_iplb) {
> +        ipl_type = S390_IPL_TYPE_CCW; /* Assume CCW for probing */
>           boot_setup();
>           probe_boot_device();
>       }
>   
>       while (have_iplb) {
> +        ipl_type = iplb.pbt;
>           boot_setup();
>           if (have_iplb && find_boot_device()) {
>               ipl_boot_device();
> diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
> index 4b819dd80f..df6a6d5b70 100644
> --- a/pc-bios/s390-ccw/virtio-blkdev.c
> +++ b/pc-bios/s390-ccw/virtio-blkdev.c
> @@ -11,6 +11,7 @@
>   #include <stdio.h>
>   #include "s390-ccw.h"
>   #include "virtio.h"
> +#include "virtio-ccw.h"
>   #include "virtio-scsi.h"
>   
>   #define VIRTIO_BLK_F_GEOMETRY   (1 << 4)
> @@ -40,9 +41,10 @@ static int virtio_blk_read_many(VDev *vdev, unsigned long sector, void *load_add
>                      VRING_DESC_F_WRITE | VRING_HIDDEN_IS_CHAIN);
>   
>       /* Now we can tell the host to read */
> +    vring_notify(vr);

Why is there suddenly a new call to vring_notify() popping up here? ... if 
it's necessary now, this deserves a separate patch, I think.

>       vring_wait_reply();
>   
> -    if (drain_irqs(vr->schid)) {
> +    if (drain_irqs(vr)) {
>           /* Well, whatever status is supposed to contain... */
>           status = 1;
>       }
> @@ -53,14 +55,14 @@ int virtio_read_many(unsigned long sector, void *load_addr, int sec_num)
>   {
>       VDev *vdev = virtio_get_device();
>   
> -    switch (vdev->senseid.cu_model) {
> +    switch (vdev->type) {
>       case VIRTIO_ID_BLOCK:
>           return virtio_blk_read_many(vdev, sector, load_addr, sec_num);
>       case VIRTIO_ID_SCSI:
>           return virtio_scsi_read_many(vdev, sector, load_addr, sec_num);
> +    default:
> +        return -1;
>       }
> -
> -    return -1;
>   }
>   
>   unsigned long virtio_load_direct(unsigned long rec_list1, unsigned long rec_list2,
> @@ -119,7 +121,7 @@ void virtio_assume_iso9660(void)
>   {
>       VDev *vdev = virtio_get_device();
>   
> -    switch (vdev->senseid.cu_model) {
> +    switch (vdev->type) {
>       case VIRTIO_ID_BLOCK:
>           vdev->guessed_disk_nature = VIRTIO_GDN_SCSI;
>           vdev->config.blk.blk_size = VIRTIO_ISO_BLOCK_SIZE;
> @@ -129,6 +131,8 @@ void virtio_assume_iso9660(void)
>       case VIRTIO_ID_SCSI:
>           vdev->scsi_block_size = VIRTIO_ISO_BLOCK_SIZE;
>           break;
> +    default:
> +        panic("Virtio device type mismatch for iso9660 IPL");
>       }
>   }
>   
> @@ -139,13 +143,15 @@ void virtio_assume_eckd(void)
>       vdev->guessed_disk_nature = VIRTIO_GDN_DASD;
>       vdev->blk_factor = 1;
>       vdev->config.blk.physical_block_exp = 0;
> -    switch (vdev->senseid.cu_model) {
> +    switch (vdev->type) {
>       case VIRTIO_ID_BLOCK:
>           vdev->config.blk.blk_size = VIRTIO_DASD_DEFAULT_BLOCK_SIZE;
>           break;
>       case VIRTIO_ID_SCSI:
>           vdev->config.blk.blk_size = vdev->scsi_block_size;
>           break;
> +    default:
> +        panic("Virtio device type mismatch for ECKD IPL");
>       }
>       vdev->config.blk.geometry.heads = 15;
>       vdev->config.blk.geometry.sectors =
> @@ -162,8 +168,7 @@ bool virtio_ipl_disk_is_valid(void)
>           return true;
>       }
>   
> -    return (vdev->senseid.cu_model == VIRTIO_ID_BLOCK ||
> -            vdev->senseid.cu_model == VIRTIO_ID_SCSI) &&
> +    return (vdev->type == VIRTIO_ID_BLOCK || vdev->type == VIRTIO_ID_SCSI) &&
>              blksize >= 512 && blksize <= 4096;
>   }
>   
> @@ -171,41 +176,44 @@ int virtio_get_block_size(void)
>   {
>       VDev *vdev = virtio_get_device();
>   
> -    switch (vdev->senseid.cu_model) {
> +    switch (vdev->type) {
>       case VIRTIO_ID_BLOCK:
>           return vdev->config.blk.blk_size;
>       case VIRTIO_ID_SCSI:
>           return vdev->scsi_block_size;
> +    default:
> +        return 0;
>       }
> -    return 0;
>   }
>   
>   uint8_t virtio_get_heads(void)
>   {
>       VDev *vdev = virtio_get_device();
>   
> -    switch (vdev->senseid.cu_model) {
> +    switch (vdev->type) {
>       case VIRTIO_ID_BLOCK:
>           return vdev->config.blk.geometry.heads;
>       case VIRTIO_ID_SCSI:
>           return vdev->guessed_disk_nature == VIRTIO_GDN_DASD
>                  ? vdev->config.blk.geometry.heads : 255;
> +    default:
> +        return 0;
>       }
> -    return 0;
>   }
>   
>   uint8_t virtio_get_sectors(void)
>   {
>       VDev *vdev = virtio_get_device();
>   
> -    switch (vdev->senseid.cu_model) {
> +    switch (vdev->type) {
>       case VIRTIO_ID_BLOCK:
>           return vdev->config.blk.geometry.sectors;
>       case VIRTIO_ID_SCSI:
>           return vdev->guessed_disk_nature == VIRTIO_GDN_DASD
>                  ? vdev->config.blk.geometry.sectors : 63;
> +    default:
> +        return 0;
>       }
> -    return 0;
>   }
>   
>   uint64_t virtio_get_blocks(void)
> @@ -213,24 +221,29 @@ uint64_t virtio_get_blocks(void)
>       VDev *vdev = virtio_get_device();
>       const uint64_t factor = virtio_get_block_size() / VIRTIO_SECTOR_SIZE;
>   
> -    switch (vdev->senseid.cu_model) {
> +    switch (vdev->type) {
>       case VIRTIO_ID_BLOCK:
>           return vdev->config.blk.capacity / factor;
>       case VIRTIO_ID_SCSI:
>           return vdev->scsi_last_block / factor;
> +    default:
> +        return 0;
>       }
> -    return 0;
>   }
>   
> -int virtio_blk_setup_device(SubChannelId schid)
> +int virtio_blk_setup_device()
>   {
>       VDev *vdev = virtio_get_device();
>   
> -    vdev->guest_features[0] = VIRTIO_BLK_F_GEOMETRY | VIRTIO_BLK_F_BLK_SIZE;
> -    vdev->schid = schid;
> -    virtio_setup_ccw(vdev);
> -
>       puts("Using virtio-blk.");
>   
> -    return 0;
> +    vdev->guest_features[0] = VIRTIO_BLK_F_GEOMETRY | VIRTIO_BLK_F_BLK_SIZE;
> +    switch (ipl_type) {
> +    case S390_IPL_TYPE_QEMU_SCSI:
> +    case S390_IPL_TYPE_CCW:
> +        vdev->schid = blk_schid;
> +        return virtio_ccw_setup(vdev);
> +    }
> +
> +    return 1;
>   }
> diff --git a/pc-bios/s390-ccw/virtio-ccw.c b/pc-bios/s390-ccw/virtio-ccw.c
> new file mode 100644
> index 0000000000..1d6e8532f6
> --- /dev/null
> +++ b/pc-bios/s390-ccw/virtio-ccw.c
> @@ -0,0 +1,240 @@
> +/*
> + * Virtio functionality for CCW devices
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Jared Rossi <jrossi@linux.ibm.com>

Since you're copying over functions from another file, I think it would make 
sense to add the copyright statement from the other file here, too.

> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include <string.h>
> +#include "s390-ccw.h"
> +#include "cio.h"
> +#include "virtio.h"
> +#include "virtio-ccw.h"
> +#include "virtio-scsi.h"
> +#include "bswap.h"
> +#include "helper.h"
> +#include "s390-time.h"
> +
> +/* virtio spec v1.0 para 4.3.3.2 */
> +static long kvm_hypercall(unsigned long nr, unsigned long param1,
> +                          unsigned long param2, unsigned long param3)
> +{
> +    register unsigned long r_nr asm("1") = nr;
> +    register unsigned long r_param1 asm("2") = param1;
> +    register unsigned long r_param2 asm("3") = param2;
> +    register unsigned long r_param3 asm("4") = param3;
> +    register long retval asm("2");
> +
> +    asm volatile ("diag %%r2,%%r4,0x500"
> +                  : "=d" (retval)
> +                  : "d" (r_nr), "0" (r_param1), "r"(r_param2), "d"(r_param3)
> +                  : "memory", "cc");
> +
> +    return retval;
> +}
> +
> +bool virtio_ccw_is_supported(SubChannelId schid)
> +{
> +    VDev *vdev = virtio_get_device();
> +    vdev->schid = schid;
> +    memset(&vdev->senseid, 0, sizeof(vdev->senseid));
> +
> +    /*
> +     * Run sense id command.
> +     * The size of the senseid data differs between devices (notably,
> +     * between virtio devices and dasds), so specify the largest possible
> +     * size and suppress the incorrect length indication for smaller sizes.
> +     */
> +    if (run_ccw(vdev, CCW_CMD_SENSE_ID, &vdev->senseid, sizeof(vdev->senseid),
> +                true)) {
> +        return false;
> +    }
> +
> +    vdev->type = vdev->senseid.cu_model;
> +
> +    if (vdev->senseid.cu_type == 0x3832) {
> +        switch (vdev->type) {
> +        case VIRTIO_ID_BLOCK:
> +        case VIRTIO_ID_SCSI:
> +        case VIRTIO_ID_NET:
> +            return true;
> +        default:
> +            return false;
> +        }
> +    }
> +    return false;
> +}

You copied the is_supported() function to the new file, but also kept the 
original. As far as I can see, virtio_ccw_is_supported() is not used at all 
in your new code, so you could remove that here again? Or remove the 
original function and only use this one here?

> +int drain_irqs_ccw(SubChannelId schid)
> +{
> +    Irb irb = {};
> +    int r = 0;
> +
> +    while (1) {
> +        /* FIXME: make use of TPI, for that enable subchannel and isc */
> +        if (tsch(schid, &irb)) {
> +            /* Might want to differentiate error codes later on. */
> +            if (irb.scsw.cstat) {
> +                r = -EIO;
> +            } else if (irb.scsw.dstat != 0xc) {
> +                r = -EIO;
> +            }
> +            return r;
> +        }
> +    }
> +}
> +
> +long virtio_ccw_notify(SubChannelId schid, int vq_idx, long cookie)
> +{
> +    return kvm_hypercall(KVM_S390_VIRTIO_CCW_NOTIFY, *(u32 *)&schid,
> +                         vq_idx, cookie);
> +}
> +
> +int virtio_ccw_run(VDev *vdev, int vqid, VirtioCmd *cmd)
> +{
> +    VRing *vr = &vdev->vrings[vqid];
> +    int i = 0;
> +
> +    do {
> +        vring_send_buf(vr, cmd[i].data, cmd[i].size,
> +                       cmd[i].flags | (i ? VRING_HIDDEN_IS_CHAIN : 0));
> +    } while (cmd[i++].flags & VRING_DESC_F_NEXT);
> +
> +    vring_wait_reply();
> +    if (drain_irqs(vr)) {
> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +int run_ccw(VDev *vdev, int cmd, void *ptr, int len, bool sli)
> +{
> +    Ccw1 ccw = {};
> +
> +    ccw.cmd_code = cmd;
> +    ccw.cda = (long)ptr;
> +    ccw.count = len;
> +
> +    if (sli) {
> +        ccw.flags |= CCW_FLAG_SLI;
> +    }
> +
> +    return do_cio(vdev->schid, vdev->senseid.cu_type, ptr2u32(&ccw), CCW_FMT1);
> +}

As far as I can see, run_ccw() is now only used in the new file, so you 
could make this function "static".

  Thomas