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

[Jared Rossi <jrossi@linux.ibm.com>]

On 5/8/26 9:16 PM, Eric Farman wrote:
> 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...
The ssid and devno should be removed from the PCI version, it was just an
oversight to leave them there.
> 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.
Yes, it is possible.  At first I did not think it was the proper 
approach, but
thinking about it again now I can see the benefit.  I will reconsider it.

> 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...
You are correct that the offsets are wrong. At minimum I will fix the 
offsets to
match, otherwise I will rework it into a combined IPLB as described above.

Thanks for catching this error.

Regards,
Jared Rossi