On 19.02.2018 09:50, Viktor Mihajlovski wrote: > On 17.02.2018 09:11, Thomas Huth wrote: > [...] >> >> I still think that the information should *not* be stored within the >> IplParameterBlock to avoid that we pass it via DIAG 0x308, too. >> If we do it like this, I'm pretty sure that we will look at this code in >> a couple of years and wonder whether we can change it again or whether >> this is an established interface between the host and the guest. So >> please, let's avoid establishing such "hidden" interfaces just out of >> current convenience. There must be a better location for this. >> Christian, do you have an idea? >> >> Thomas >> > In principle I do agree, although I think we can manage the host/guest > boundary. If we believe we can't, we have a much bigger problem (looking > at the position of the iplb smack in the middle of the s390-ipl state). > > The main reason why I didn't bother to introduce a new private field in > s390-ipl was that I expect more changes to the IPL area in the future > (earlier than in a couple of years) and am not really sure whether this > QEMU <-> BIOS interface will remain the same and how much effort to > spend on it. > > The major point of this change is to move non-standard data out of the > guest-visible IPLB to avoid compatibility problems in the future, while > still catering for features like network boot and boot menus. I have no > bias against other solutions achieving this objective. If you and > Christian think we need a new field, it's all right with me. > With below squashed in (and a subsequent fixup for the next patch), we can logically separate the QEMU IPL parameter from the IPLB. Better?
--- diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c index 31565ce..c5923b5 100644 --- a/hw/s390x/ipl.c +++ b/hw/s390x/ipl.c @@ -410,7 +410,7 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu) error_report("Cannot set QEMU IPL parameters"); return; } - memcpy(addr + QIPL_ADDRESS, &ipl->iplb.qipl, sizeof(QemuIplParameters)); + memcpy(addr + QIPL_ADDRESS, &ipl->qipl, sizeof(QemuIplParameters)); cpu_physical_memory_unmap(addr, len, 1, len); } @@ -433,7 +433,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu) error_report_err(err); vm_stop(RUN_STATE_INTERNAL_ERROR); } - ipl->iplb.qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr); + ipl->qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr); } s390_ipl_prepare_qipl(cpu); diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h index 74469b1..775d363 100644 --- a/hw/s390x/ipl.h +++ b/hw/s390x/ipl.h @@ -88,7 +88,6 @@ union IplParameterBlock { IplBlockFcp fcp; IplBlockQemuScsi scsi; }; - QemuIplParameters qipl; } QEMU_PACKED; struct { uint8_t reserved1[110]; @@ -120,6 +119,7 @@ struct S390IPLState { bool iplb_valid; bool reipl_requested; bool netboot; + QemuIplParameters qipl; /*< public >*/ char *kernel; -- Regards, Viktor Mihajlovski