On 8/25/26 12:44 PM, Zhuoying Cai wrote:
On 8/25/26 6:29 AM, Eric Farman wrote:
On 8/24/26 1:53 PM, Zhuoying Cai wrote:
On 8/11/26 10:46 AM, [email protected] wrote:
From: Jared Rossi <[email protected]>
The IplBlockPci struct is 336 bytes in length.
The commit message is slightly misleading because IplBlockPci itself is
312 bytes in length.
Maybe reword it to something like:
"IplBlockPci is 312 bytes, so the minimum PCI IPLB length is 336 bytes."
Given that, should the #define be sizeof(IplBlockPci) + 24?
If IplBlockPci is likely to expand in the future, it might make sense to
derive this value from the structure layout. For example,
offsetof(IplParameterBlock, pci) + sizeof(IplBlockPci) could be more
accurate. Otherwise, keeping it as a constant like the other definitions
seems reasonable as well.
All of those suggestions would be valid, but I'm leaning toward keeping it
defined without using sizeof() for the sake of consistency if nothing
else. The other IPLB types are already defined as fixed numbers and for
the IplBlockQemuScsi being used later in this patch series, it actually has
a minimum value that is less than the sizeof() itself due to the same struct
servicing both PCI and CCW controllers, where the minimum size for CCW is
less than PCI.
I can envision several ways to improve the definitions and/or naming
conventions, but I think it is outside the scope of this series because the
changes should be uniformly applied to all definitions, not just PCI.
Possibly something to consider in the future.
As far as the commit message, I will update it to state more specifically
that the minimum size of the IPLB used for PCI devices is 336 (referring
to the IplBlockPci struct directly was not accurate).
And does the patch itself need to be cc'd to stable?
I think it could be a candidate, but I don't believe it really needs to be
applied to stable. There is no exploitable bug with the incorrect length
definition as it is; I only noticed it because I was checking the field
offsets against the other IPLBs. The length is still within the designated
IPLB boundaries. I'll cc stable in the next version and they can pick it
up if they feel it warrants the change, but it has no impact in practice.
Otherwise:
Reviewed-by: Zhuoying Cai <[email protected]>
Fix the S390_IPLB_MIN_PCI_LEN to reflect this, and assign a valid blk0_len
while we are at it.
Signed-off-by: Jared Rossi <[email protected]>
---
hw/s390x/ipl.c | 2 ++
hw/s390x/ipl.h | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index d5fdc3ea0f..fd6e9100f9 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -526,6 +526,8 @@ static bool s390_build_iplb(DeviceState *dev_st,
IplParameterBlock *iplb)
switch (devtype) {
case PCI_DEVTYPE_VIRTIO:
iplb->len = cpu_to_be32(S390_IPLB_MIN_PCI_LEN);
+ iplb->blk0_len =
+ cpu_to_be32(S390_IPLB_MIN_PCI_LEN - S390_IPLB_HEADER_LEN);
iplb->pbt = S390_IPL_TYPE_PCI;
iplb->pci.fid = cpu_to_be32(pbdev->fid);
break;
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index fac30763df..b4d93054a7 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -107,7 +107,7 @@ QEMU_BUILD_BUG_MSG(offsetof(S390IPLState, iplb) & 3, "alignment
of iplb wrong");
#define S390_IPLB_MIN_PV_LEN 148
#define S390_IPLB_MIN_CCW_LEN 200
#define S390_IPLB_MIN_FCP_LEN 384
-#define S390_IPLB_MIN_PCI_LEN 376
+#define S390_IPLB_MIN_PCI_LEN 336
#define S390_IPLB_MIN_QEMU_SCSI_LEN 200
static inline bool iplb_valid_len(IplParameterBlock *iplb)
