Re: [PATCH v1 3/3] target/s390x: Return UVC cmd code, RC and RRC value when DIAG 308 Subcode 10 fails to enter secure mode

[Steffen Eiden <seiden@linux.ibm.com>]

On Fri, Apr 11, 2025 at 11:22:33AM +0200, Gautam Gala wrote:
> Extend DIAG308 subcode 10 to return the UVC RC, RRC and command code
> in bit positions 32-47, 16-31, and 0-15 of register R1 + 1 if the
> function does not complete successfully (in addition to the
> previously returned diag response code in bit position 47-63).
> 
> Signed-off-by: Gautam Gala <ggala@linux.ibm.com>
> ---
>  hw/s390x/ipl.c             | 11 ++++++----
>  hw/s390x/ipl.h             |  5 +++--
>  hw/s390x/s390-virtio-ccw.c | 24 +++++++++++++++------
>  target/s390x/kvm/pv.c      | 44 +++++++++++++++++++++++++-------------
>  target/s390x/kvm/pv.h      | 27 ++++++++++++++++-------
>  5 files changed, 76 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index ce6f6078d7..4f3e3945f1 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -676,7 +676,8 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu)
>      cpu_physical_memory_unmap(addr, len, 1, len);
>  }
>  
> -int s390_ipl_prepare_pv_header(Error **errp)
> +int s390_ipl_prepare_pv_header(Error **errp, uint16_t *pv_cmd,
> +                               uint16_t *pv_rc, uint16_t *pv_rrc)

`Error **errp` should be the last argument. (multiple locations)

...
  
> +struct diag308response {
> +    uint16_t pv_cmd;
> +    uint16_t pv_rrc;
> +    uint16_t pv_rc;
> +    uint16_t diag_rc;
> +};
> +

I think Qemu style wants you to use CamelCase here.
See e.g `390CcwMachineState` below


>  static S390CPU *s390x_new_cpu(const char *typename, uint32_t core_id,
>                                Error **errp)
>  {
> @@ -364,7 +371,10 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
>      ram_block_discard_disable(false);
>  }
>  

...

> @@ -452,6 +462,7 @@ static void s390_machine_reset(MachineState *machine, ResetType type)
>  {
>      S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
>      enum s390_reset reset_type;
> +    struct diag308response resp;

Reverse Christmas tree.

>      CPUState *cs, *t;
>      S390CPU *cpu;
>  

...