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

[Viktor Mihajlovski]

On 22.02.2018 05:40, Thomas Huth wrote:
> On 21.02.2018 20:35, Collin L. Walling wrote:
>> The s390-ccw firmware needs some information in support of the
>> boot process which is not available on the native machine.
>> Examples are the netboot firmware load address and now the
>> boot menu parameters.
>>
>> While storing that data in unused fields of the IPL parameter block
>> works, that approach could create problems if the parameter block
>> definition should change in the future. Because then a guest could
>> overwrite these fields using the set IPLB diagnose.
>>
>> In fact the data in question is of more global nature and not really
>> tied to an IPL device, so separating it is rather logical.
>>
>> This commit introduces a new structure to hold firmware relevant
>> IPL parameters set by QEMU. The data is stored at location 204 (dec)
>> and can contain up to 7 32-bit words. This area is available to
>> programming in the z/Architecture Principles of Operation and
>> can thus safely be used by the firmware until the IPL has completed.
>>
>> Signed-off-by: Viktor Mihajlovski 
>> Signed-off-by: Collin L. Walling 
>> ---
>>  hw/s390x/ipl.c  | 18 +-
>>  hw/s390x/ipl.h  | 25 +++--
>>  pc-bios/s390-ccw/iplb.h | 18 --
>>  pc-bios/s390-ccw/main.c |  6 +-
>>  4 files changed, 61 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 0d06fc1..79f5a58 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -399,6 +399,21 @@ void s390_reipl_request(void)
>>  qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>>  }
>>  
>> +static void s390_ipl_prepare_qipl(S390CPU *cpu)
>> +{
>> +S390IPLState *ipl = get_ipl_device();
>> +uint8_t *addr;
>> +uint64_t len = 4096;
>> +
>> +addr = cpu_physical_memory_map(cpu->env.psa, , 1);
>> +if (!addr || len < QIPL_ADDRESS + sizeof(QemuIplParameters)) {
>> +error_report("Cannot set QEMU IPL parameters");
>> +return;
>> +}
>> +memcpy(addr + QIPL_ADDRESS, >qipl, sizeof(QemuIplParameters));
>> +cpu_physical_memory_unmap(addr, len, 1, len);
>> +}
>> +
>>  void s390_ipl_prepare_cpu(S390CPU *cpu)
>>  {
>>  S390IPLState *ipl = get_ipl_device();
>> @@ -418,8 +433,9 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
>>  error_report_err(err);
>>  vm_stop(RUN_STATE_INTERNAL_ERROR);
>>  }
>> -ipl->iplb.ccw.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);
>>  }
>>  
>>  static void s390_ipl_reset(DeviceState *dev)
>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>> index 8a705e0..08926a3 100644
>> --- a/hw/s390x/ipl.h
>> +++ b/hw/s390x/ipl.h
>> @@ -16,8 +16,7 @@
>>  #include "cpu.h"
>>  
>>  struct IplBlockCcw {
>> -uint64_t netboot_start_addr;
>> -uint8_t  reserved0[77];
>> +uint8_t  reserved0[85];
>>  uint8_t  ssid;
>>  uint16_t devno;
>>  uint8_t  vm_flags;
>> @@ -90,6 +89,27 @@ void s390_ipl_prepare_cpu(S390CPU *cpu);
>>  IplParameterBlock *s390_ipl_get_iplb(void);
>>  void s390_reipl_request(void);
>>  
>> +#define QIPL_ADDRESS  0xcc
>> +
>> +/*
>> + * The QEMU IPL Parameters will be stored at absolute address
>> + * 204 (0xcc) which means it is 32-bit word aligned but not
>> + * double-word aligned.
>> + * Placement of data fields in this area must account for
>> + * their alignment needs. E.g., netboot_start_address must
>> + * have an offset of n * 8 bytes within the struct in order
>> + * to keep it double-word aligned.
> 
> Should that rather be "4 + n * 8" instead of "n * 8" ?
I wonder if I ever get that comment right. You're correct of course.
> 
> Apart from that, patch looks good to me now, so once you've fixed the
> comment (if necessary):
> 
> Reviewed-by: Thomas Huth 
> 


-- 
Regards,
 Viktor Mihajlovski

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

[Thomas Huth]

On 21.02.2018 20:35, Collin L. Walling wrote:
> The s390-ccw firmware needs some information in support of the
> boot process which is not available on the native machine.
> Examples are the netboot firmware load address and now the
> boot menu parameters.
> 
> While storing that data in unused fields of the IPL parameter block
> works, that approach could create problems if the parameter block
> definition should change in the future. Because then a guest could
> overwrite these fields using the set IPLB diagnose.
> 
> In fact the data in question is of more global nature and not really
> tied to an IPL device, so separating it is rather logical.
> 
> This commit introduces a new structure to hold firmware relevant
> IPL parameters set by QEMU. The data is stored at location 204 (dec)
> and can contain up to 7 32-bit words. This area is available to
> programming in the z/Architecture Principles of Operation and
> can thus safely be used by the firmware until the IPL has completed.
> 
> Signed-off-by: Viktor Mihajlovski 
> Signed-off-by: Collin L. Walling 
> ---
>  hw/s390x/ipl.c  | 18 +-
>  hw/s390x/ipl.h  | 25 +++--
>  pc-bios/s390-ccw/iplb.h | 18 --
>  pc-bios/s390-ccw/main.c |  6 +-
>  4 files changed, 61 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 0d06fc1..79f5a58 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -399,6 +399,21 @@ void s390_reipl_request(void)
>  qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>  }
>  
> +static void s390_ipl_prepare_qipl(S390CPU *cpu)
> +{
> +S390IPLState *ipl = get_ipl_device();
> +uint8_t *addr;
> +uint64_t len = 4096;
> +
> +addr = cpu_physical_memory_map(cpu->env.psa, , 1);
> +if (!addr || len < QIPL_ADDRESS + sizeof(QemuIplParameters)) {
> +error_report("Cannot set QEMU IPL parameters");
> +return;
> +}
> +memcpy(addr + QIPL_ADDRESS, >qipl, sizeof(QemuIplParameters));
> +cpu_physical_memory_unmap(addr, len, 1, len);
> +}
> +
>  void s390_ipl_prepare_cpu(S390CPU *cpu)
>  {
>  S390IPLState *ipl = get_ipl_device();
> @@ -418,8 +433,9 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
>  error_report_err(err);
>  vm_stop(RUN_STATE_INTERNAL_ERROR);
>  }
> -ipl->iplb.ccw.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);
>  }
>  
>  static void s390_ipl_reset(DeviceState *dev)
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index 8a705e0..08926a3 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -16,8 +16,7 @@
>  #include "cpu.h"
>  
>  struct IplBlockCcw {
> -uint64_t netboot_start_addr;
> -uint8_t  reserved0[77];
> +uint8_t  reserved0[85];
>  uint8_t  ssid;
>  uint16_t devno;
>  uint8_t  vm_flags;
> @@ -90,6 +89,27 @@ void s390_ipl_prepare_cpu(S390CPU *cpu);
>  IplParameterBlock *s390_ipl_get_iplb(void);
>  void s390_reipl_request(void);
>  
> +#define QIPL_ADDRESS  0xcc
> +
> +/*
> + * The QEMU IPL Parameters will be stored at absolute address
> + * 204 (0xcc) which means it is 32-bit word aligned but not
> + * double-word aligned.
> + * Placement of data fields in this area must account for
> + * their alignment needs. E.g., netboot_start_address must
> + * have an offset of n * 8 bytes within the struct in order
> + * to keep it double-word aligned.

Should that rather be "4 + n * 8" instead of "n * 8" ?

Apart from that, patch looks good to me now, so once you've fixed the
comment (if necessary):

Reviewed-by: Thomas Huth