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
