Re: [PATCH v5 17/18] s390x: Add KVM PV dump interface

[Steffen Eiden]

On 8/11/22 14:11, Janosch Frank wrote:
> Let's add a few bits of code which hide the new KVM PV dump API from
> us via new functions.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   hw/s390x/pv.c         | 51 +++++++++++++++++++++++++++++++++++++++++++
>   include/hw/s390x/pv.h |  8 +++++++
>   2 files changed, 59 insertions(+)
>
> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
> index 2b892b45e8..ce3b6ad3e9 100644
> --- a/hw/s390x/pv.c
> +++ b/hw/s390x/pv.c
> @@ -175,6 +175,57 @@ bool kvm_s390_pv_info_basic_valid(void)
>       return info_valid;
>   }
>   
> +static int s390_pv_dump_cmd(uint64_t subcmd, uint64_t uaddr, uint64_t gaddr,
> +                            uint64_t len)
> +{
> +    struct kvm_s390_pv_dmp dmp = {
> +        .subcmd = subcmd,
> +        .buff_addr = uaddr,
> +        .buff_len = len,
> +        .gaddr = gaddr,
> +    };
> +    int ret;
> +
> +    ret = s390_pv_cmd(KVM_PV_DUMP, (void *)&dmp);
> +    if (ret) {
> +        error_report("KVM DUMP command %ld failed", subcmd);
> +    }
> +    return ret;
> +}
> +
> +int kvm_s390_dump_cpu(S390CPU *cpu, void *buff)
> +{
> +    struct kvm_s390_pv_dmp dmp = {
> +        .subcmd = KVM_PV_DUMP_CPU,
> +        .buff_addr = (uint64_t)buff,
> +        .gaddr = 0,
> +        .buff_len = info_dump.dump_cpu_buffer_len,
> +    };
> +    struct kvm_pv_cmd pv = {
> +        .cmd = KVM_PV_DUMP,
> +        .data = (uint64_t)&dmp,
> +    };
> +
> +    return kvm_vcpu_ioctl(CPU(cpu), KVM_S390_PV_CPU_COMMAND, &pv);
> +}
> +
> +int kvm_s390_dump_init(void)
> +{
> +    return s390_pv_dump_cmd(KVM_PV_DUMP_INIT, 0, 0, 0);
> +}
> +

I suggest changing that into something like
 * kvm_s390_dump_mem_state
 * kvm_s390_dump_config_state

This would make the effect of that function more clear.
It is not dumping the memory, but getting (part of) the
necessary metadata to process the dump.

Additionally, I suggest to reflect the name changes in the next patch.
I mark all functions I find.
> +int kvm_s390_dump_mem(uint64_t gaddr, size_t len, void *dest)
> +{
> +    return s390_pv_dump_cmd(KVM_PV_DUMP_CONFIG_STATE, (uint64_t)dest,
> +                            gaddr, len);
> +}
> +
> +int kvm_s390_dump_complete(void *buff)
> +{
> +    return s390_pv_dump_cmd(KVM_PV_DUMP_COMPLETE, (uint64_t)buff, 0,
> +                            info_dump.dump_config_finalize_len);
> +}
> +
>   #define TYPE_S390_PV_GUEST "s390-pv-guest"
>   OBJECT_DECLARE_SIMPLE_TYPE(S390PVGuest, S390_PV_GUEST)
>   
> diff --git a/include/hw/s390x/pv.h b/include/hw/s390x/pv.h
> index 573259cf2b..02a6c06b9f 100644
> --- a/include/hw/s390x/pv.h
> +++ b/include/hw/s390x/pv.h
> @@ -51,6 +51,10 @@ uint64_t kvm_s390_pv_dmp_get_size_cpu(void);
>   uint64_t kvm_s390_pv_dmp_get_size_stor_state(void);
>   uint64_t kvm_s390_pv_dmp_get_size_complete(void);
>   bool kvm_s390_pv_info_basic_valid(void);
> +int kvm_s390_dump_init(void);
> +int kvm_s390_dump_cpu(S390CPU *cpu, void *buff);
> +int kvm_s390_dump_mem(uint64_t addr, size_t len, void *dest);
> +int kvm_s390_dump_complete(void *buff);
>   #else /* CONFIG_KVM */
>   static inline bool s390_is_pv(void) { return false; }
>   static inline int s390_pv_query_info(void) { return 0; }
> @@ -66,6 +70,10 @@ static inline uint64_t kvm_s390_pv_dmp_get_size_cpu(void) { 
> return 0; }
>   static inline uint64_t kvm_s390_pv_dmp_get_size_stor_state(void) { return 0; }
>   static inline uint64_t kvm_s390_pv_dmp_get_size_complete(void) { return 0; }
>   static inline bool kvm_s390_pv_info_basic_valid(void) { return false; }
> +static inline int kvm_s390_dump_init(void) { return 0; }
> +static inline int kvm_s390_dump_cpu(S390CPU *cpu, void *buff, size_t len) { 
> return 0; }
> +static inline int kvm_s390_dump_mem(uint64_t addr, size_t len, void *dest) { 
> return 0; }
> +static inline int kvm_s390_dump_complete(void *buff) { return 0; }
>   #endif /* CONFIG_KVM */
>   
>   int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);

Beside that nit, LGTM.

Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>