Re: [PATCH 4/6] s390x: Introduce PCI SCSI IPLB and boot type

[Eric Farman <farman@linux.ibm.com>]

On Mon, 2026-05-04 at 18:16 -0400, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> Define a new IPLB format for SCSI PCI devices and add cases to handle it.
> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
>  include/hw/s390x/ipl/qipl.h    | 15 +++++++++++++++
>  pc-bios/s390-ccw/main.c        | 11 ++++++++++-
>  pc-bios/s390-ccw/virtio-pci.c  | 29 +++++++++++++++++++++++++++++
>  pc-bios/s390-ccw/virtio-scsi.c | 14 +++++++++++++-
>  pc-bios/s390-ccw/virtio.c      |  5 +++++
>  5 files changed, 72 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/s390x/ipl/qipl.h b/include/hw/s390x/ipl/qipl.h
> index 67db54c964..951c5e2d8e 100644
> --- a/include/hw/s390x/ipl/qipl.h
> +++ b/include/hw/s390x/ipl/qipl.h
> @@ -25,6 +25,7 @@ enum S390IplType {
>      S390_IPL_TYPE_CCW = 0x02,
>      S390_IPL_TYPE_PCI = 0x04,
>      S390_IPL_TYPE_PV = 0x05,
> +    S390_IPL_TYPE_PCI_SCSI = 0xfe,
>      S390_IPL_TYPE_CCW_SCSI = 0xff
>  };
>  typedef enum S390IplType S390IplType;
> @@ -117,6 +118,19 @@ struct IplBlockPci {
>  } QEMU_PACKED;
>  typedef struct IplBlockPci IplBlockPci;
>  
> +struct IplBlockPciScsi {
> +    uint32_t lun;
> +    uint16_t target;
> +    uint16_t channel;
> +    uint8_t  reserved0[74];
> +    uint8_t  opt;
> +    uint8_t  reserved1[3];
> +    uint32_t fid;
> +    uint8_t  ssid;
> +    uint16_t devno;
> +} QEMU_PACKED;
> +typedef struct IplBlockPciScsi IplBlockPciScsi;

Now that I get to the point where I see why you renamed the ...QemuScsi struct in patch one, I'm
struggling.

This combines the PCI stuff in IplBlockPci, with the CCW (!) and SCSI stuff in IplBlockCcwScsi (nee
IplBlockQemuScsi). Some of the stuff (ssid, devno) isn't needed for PCI, and I find myself wondering
what the "right" struct should look like...

Can we add the PCI stuff to a (generalized) struct that's just "IplBlockScsi" and have fields that
are specific to -ccw or -pci? That's basically what IplBlockPciScsi is, even though it's not used
that way. Or is it better that there's overlap between Ccw/CcwScsi (e.g., offsetof(Ccw.ssid) ==
offsetof(CcwScsi.ssid)) and Pci/PciScsi? If the latter, there's quite a bit of difference in their
layouts, e.g., the location of the opt and fid fields. That worries me...

> +
>  union IplParameterBlock {
>      struct {
>          uint32_t len;
> @@ -133,6 +147,7 @@ union IplParameterBlock {
>              IPLBlockPV pv;
>              IplBlockCcwScsi ccw_scsi;
>              IplBlockPci pci;
> +            IplBlockPciScsi pci_scsi;
>          };
>      } QEMU_PACKED;
>      struct {
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 1a9579beab..b2bc35962a 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -257,7 +257,15 @@ static bool find_boot_device(void)
>          blk_schid.ssid = iplb.ccw_scsi.ssid & 0x3;
>          found = find_subch(iplb.ccw_scsi.devno);
>          break;
> +     case S390_IPL_TYPE_PCI_SCSI:
> +        vdev->scsi_device_selected = true;
> +        vdev->selected_scsi_device.channel = iplb.pci_scsi.channel;
> +        vdev->selected_scsi_device.target = iplb.pci_scsi.target;
> +        vdev->selected_scsi_device.lun = iplb.pci_scsi.lun;
> +        found = find_fid(iplb.pci_scsi.fid);

...because of this. I don't see iplb.pci_scsi.fid set anywhere. Patch 5 sets ipl.pci.fid, but that's
at a different offset within the respective structs in that union.

> +        break;
>       case S390_IPL_TYPE_PCI:
> +        vdev->scsi_device_selected = false;
>          found = find_fid(iplb.pci.fid);
>          break;
>      default:
> @@ -318,13 +326,13 @@ static void ipl_pci_device(void)
>  {
>      VDev *vdev = virtio_get_device();
>      vdev->is_cdrom = false;
> -    vdev->scsi_device_selected = false;
>  
>      if (virtio_pci_setup_device()) {
>          return;
>      }
>  
>      switch (vdev->dev_type) {
> +    case VIRTIO_ID_SCSI:
>      case VIRTIO_ID_BLOCK:
>          if (virtio_setup() == 0) {
>              zipl_load(); /* only return on error */
> @@ -343,6 +351,7 @@ static void ipl_boot_device(void)
>      case S390_IPL_TYPE_CCW:
>          ipl_ccw_device();
>          break;
> +    case S390_IPL_TYPE_PCI_SCSI:
>      case S390_IPL_TYPE_PCI:
>          ipl_pci_device();
>          break;
> diff --git a/pc-bios/s390-ccw/virtio-pci.c b/pc-bios/s390-ccw/virtio-pci.c
> index 736869f4f5..01a1401ef2 100644
> --- a/pc-bios/s390-ccw/virtio-pci.c
> +++ b/pc-bios/s390-ccw/virtio-pci.c
> @@ -53,6 +53,10 @@ void virtio_pci_id2type(VDev *vdev, uint16_t device_id)
>      case 0x1001:
>          vdev->dev_type = VIRTIO_ID_BLOCK;
>          break;
> +    case 0x1048:
> +    case 0x1004:
> +        vdev->dev_type = VIRTIO_ID_SCSI;
> +        break;
>      default:
>          vdev->dev_type = 0;
>      }
> @@ -200,6 +204,26 @@ static int virtio_pci_get_blk_config(void)
>      return rc;
>  }
>  
> +static int virtio_pci_get_scsi_config(void)
> +{
> +    VirtioScsiConfig *cfg = &virtio_get_device()->config.scsi;
> +    int rc = vpci_read_flex(d_cap.off, d_cap.bar, cfg, sizeof(VirtioScsiConfig));
> +
> +    /* all fields of scsi config must be byte swapped */
> +    cfg->num_queues = bswap32(cfg->num_queues);
> +    cfg->seg_max = bswap32(cfg->seg_max);
> +    cfg->max_sectors = bswap32(cfg->max_sectors);
> +    cfg->cmd_per_lun = bswap32(cfg->cmd_per_lun);
> +    cfg->event_info_size = bswap32(cfg->event_info_size);
> +    cfg->sense_size = bswap32(cfg->sense_size);
> +    cfg->cdb_size = bswap32(cfg->cdb_size);
> +    cfg->max_channel = bswap16(cfg->max_channel);
> +    cfg->max_target = bswap16(cfg->max_target);
> +    cfg->max_lun = bswap32(cfg->max_lun);
> +
> +    return rc;
> +}
> +
>  static int virtio_pci_negotiate(void)
>  {
>      int i, rc;
> @@ -367,6 +391,11 @@ int virtio_pci_setup(VDev *vdev)
>          vdev->cmd_vr_idx = 0;
>          virtio_pci_get_blk_config();
>          break;
> +    case VIRTIO_ID_SCSI:
> +        vdev->nr_vqs = 3;
> +        vdev->cmd_vr_idx = 2;
> +        virtio_pci_get_scsi_config();
> +        break;
>      default:
>          puts("Unsupported virtio device");
>          return -ENODEV;
> diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
> index 9ea00c6fe6..09d2caa879 100644
> --- a/pc-bios/s390-ccw/virtio-scsi.c
> +++ b/pc-bios/s390-ccw/virtio-scsi.c
> @@ -16,6 +16,7 @@
>  #include "scsi.h"
>  #include "virtio-scsi.h"
>  #include "virtio-ccw.h"
> +#include "virtio-pci.h"
>  #include "s390-time.h"
>  #include "helper.h"
>  
> @@ -479,7 +480,18 @@ static int virtio_scsi_setup(VDev *vdev)
>  
>  int virtio_scsi_setup_device(VDev *vdev)
>  {
> -    virtio_ccw_setup(vdev);
> +    switch (vdev->ipl_type) {
> +    case S390_IPL_TYPE_CCW_SCSI:
> +    case S390_IPL_TYPE_CCW:
> +        virtio_ccw_setup(vdev);
> +        break;
> +    case S390_IPL_TYPE_PCI_SCSI:
> +    case S390_IPL_TYPE_PCI:
> +        virtio_pci_setup(vdev);
> +        break;
> +    default:
> +        return 1;
> +    }
>  
>      if (vdev->config.scsi.sense_size != VIRTIO_SCSI_SENSE_SIZE) {
>          puts("Config: sense size mismatch");
> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
> index 79dccdcbfc..598768d12b 100644
> --- a/pc-bios/s390-ccw/virtio.c
> +++ b/pc-bios/s390-ccw/virtio.c
> @@ -89,6 +89,8 @@ static int virtio_do_run(VDev *vdev, int vqid, VirtioCmd *cmd)
>  int virtio_run(VDev *vdev, int vqid, VirtioCmd *cmd)
>  {
>      switch (vdev->ipl_type) {
> +    case S390_IPL_TYPE_PCI_SCSI:
> +    case S390_IPL_TYPE_PCI:
>      case S390_IPL_TYPE_CCW_SCSI:
>      case S390_IPL_TYPE_CCW:
>          return virtio_do_run(vdev, vqid, cmd);
> @@ -130,6 +132,7 @@ bool vring_notify(VRing *vr)
>      case S390_IPL_TYPE_CCW:
>          vr->cookie = virtio_ccw_notify(vdev.schid, vr->id, vr->cookie);
>          break;
> +    case S390_IPL_TYPE_PCI_SCSI:
>      case S390_IPL_TYPE_PCI:
>          vr->cookie = virtio_pci_notify(vr);
>          break;
> @@ -150,6 +153,7 @@ bool be_ipl(void)
>      case S390_IPL_TYPE_CCW_SCSI:
>      case S390_IPL_TYPE_CCW:
>          return true;
> +    case S390_IPL_TYPE_PCI_SCSI:
>      case S390_IPL_TYPE_PCI:
>          return false;
>      default:
> @@ -252,6 +256,7 @@ int virtio_reset(VDev *vdev)
>      case S390_IPL_TYPE_CCW_SCSI:
>      case S390_IPL_TYPE_CCW:
>          return virtio_ccw_reset(vdev);
> +    case S390_IPL_TYPE_PCI_SCSI:
>      case S390_IPL_TYPE_PCI:
>          return virtio_pci_reset(vdev);
>      default: