| Message ID | 8bc5f6a2-275f-8e06-571d-d270deb1fa29@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19.02.2018 13:15, Viktor Mihajlovski wrote: > 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? Yes, sounds much better to me! Collin, could you please squash that in? Thanks, Thomas > --- > 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; > >
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;