Re: [qemu-s390x] [Qemu-devel] [PATCH v7 05/12] s390-ccw: move auxiliary IPL data to separate location

[Viktor Mihajlovski]

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