On 17.02.2018 09:26, Thomas Huth wrote:
[...]
>> struct QemuIplParameters {
>> - uint8_t reserved1[4];
>> + uint8_t boot_menu_flags;
>> + uint8_t reserved1[3];
>> + uint32_t boot_menu_timeout;
>> uint64_t netboot_start_addr;
>> - uint8_t reserved2[16];
>> + uint8_t reserved2[12];
>> } __attribute__ ((packed));
>> typedef struct QemuIplParameters QemuIplParameters;
>
> I think Victor's original intention was to get netboot_start_addr
> aligned in the lowcore memory. Now it's rather aligned in the host
> memory. Quite confusing, but I think I'd rather prefer Victor's idea to
> keep it aligned in the lowcore (since that's the "architected" part).
>
> Maybe it's better if we do not declare this as a packed struct at all,
> and then instead of doing a memcpy of the whole struct, we set the
> fields manually one by one on the host side into the lowcore, and read
> the fields manually one by one on the guest side? That's more
> cumbersome, but avoids future confusion about the alignments here...
>
> Thomas
>
Hm ... I would prefer to keep it all together and perhaps come up with
better comments (for the fields). BTW: I think it would make sense to
reserve the last 8 bytes 'seriously': in case more global configuration
data is needed in the future, we should have the possibility to install
a pointer to an extension block in there.
Anyway, here's the follup squash-in for a qipl-free IPLB.
---
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 3c6a411..fe70008 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -222,7 +222,7 @@ static Property s390_ipl_properties[] = {
DEFINE_PROP_END_OF_LIST(),
};
-static void s390_ipl_set_boot_menu(IplParameterBlock *iplb)
+static void s390_ipl_set_boot_menu(S390IPLState *ipl)
{
QemuOptsList *plist = qemu_find_opts("boot-opts");
QemuOpts *opts = QTAILQ_FIRST(&plist->head);
@@ -231,11 +231,11 @@ static void s390_ipl_set_boot_menu(IplParameterBlock
*iplb)
const char *tmp;
unsigned long splash_time = 0;
- switch (iplb->pbt) {
+ switch (ipl->iplb.pbt) {
case S390_IPL_TYPE_CCW:
case S390_IPL_TYPE_QEMU_SCSI:
- flags = &iplb->qipl.boot_menu_flags;
- timeout = &iplb->qipl.boot_menu_timeout;
+ flags = &ipl->qipl.boot_menu_flags;
+ timeout = &ipl->qipl.boot_menu_timeout;
break;
default:
error_report("boot menu is not supported for this device type.");
@@ -482,7 +482,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
}
ipl->qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr);
}
- s390_ipl_set_boot_menu(&ipl->iplb);
+ s390_ipl_set_boot_menu(ipl);
s390_ipl_prepare_qipl(cpu);
}
--
Regards,
Viktor Mihajlovski
