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

[Viktor Mihajlovski]

On 16.02.2018 23:07, Collin L. Walling wrote:
[...]
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index 8a705e0..74469b1 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;
> @@ -59,6 +58,21 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
> 
>  #define DIAG308_FLAGS_LP_VALID 0x80
> 
> +#define QIPL_ADDRESS  0xcc
> +
> +/*
> + * The QEMU IPL Parameters will be stored 32-bit word aligned.
> + * Placement of data fields in this area must account for
> + * their alignment needs.
> + * The entire structure must not be larger than 28 bytes.
> + */
I realize that my initial suggestion was flawed. Hopefully better:
/*
 * 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.
 * The total size of the struct must never exceed 28 bytes.
 * This definition must be kept in sync with the defininition
 * in pc-bios/s390-ccw/iplb.h.
 */
> +struct QemuIplParameters {
> +    uint8_t  reserved1[4];
> +    uint64_t netboot_start_addr;
> +    uint8_t  reserved2[16];
> +} QEMU_PACKED;
> +typedef struct QemuIplParameters QemuIplParameters;
> +
>  union IplParameterBlock {
>      struct {
>          uint32_t len;
[...]
> diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
> index 890aed9..a23237e 100644
> --- a/pc-bios/s390-ccw/iplb.h
> +++ b/pc-bios/s390-ccw/iplb.h
> @@ -13,8 +13,7 @@
>  #define IPLB_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;
> @@ -73,6 +72,23 @@ typedef struct IplParameterBlock IplParameterBlock;
> 
>  extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
> 
> +#define QIPL_ADDRESS  0xcc
> +
> +/*
> + * The QEMU IPL Parameters will be stored 32-bit word aligned.
> + * Placement of data fields in this area must account for
> + * their alignment needs.
> + * The entire structure must not be larger than 28 bytes.
> + */
The comment can probably be a bit more terse, avoiding text duplication
and potential inconsistencies arising thereof.
/*
 * This definition must be kept in sync with the defininition
 * in hw/s390x/ipl.h
 */
> +struct QemuIplParameters {
> +    uint8_t  reserved1[4];
> +    uint64_t netboot_start_addr;
> +    uint8_t  reserved2[16];
> +} __attribute__ ((packed));
> +typedef struct QemuIplParameters QemuIplParameters;
> +
> +extern QemuIplParameters qipl;
> +
>  #define S390_IPL_TYPE_FCP 0x00
>  #define S390_IPL_TYPE_CCW 0x02
>  #define S390_IPL_TYPE_QEMU_SCSI 0xff
[...]

-- 
Regards,
 Viktor Mihajlovski