Re: [qemu-s390x] [Qemu-devel] [PATCH v7 06/12] s390-ccw: parse and set boot menu options

[Viktor Mihajlovski]

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