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

[Thomas Huth]

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;
> 
>