Re: [PATCH v1 03/14] s390x/s390-virtio-hcall: prepare for more diag500 hypercalls

[Nina Schoetterl-Glausch]

On Tue, 2024-09-10 at 19:57 +0200, David Hildenbrand wrote:
> Let's generalize, abstracting the virtio bits. diag500 is now a generic
> hypercall to handle QEMU/KVM specific things. Explicitly specify all
> already defined subcodes, including legacy ones (so we know what we can
> use for new hypercalls).
> 
> We'll rename the files separately, so git properly detects the rename.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-virtio-hcall.c   |  8 ++++----
>  hw/s390x/s390-virtio-hcall.h   | 11 ++++++-----
>  target/s390x/kvm/kvm.c         | 10 ++--------
>  target/s390x/tcg/misc_helper.c |  4 ++--
>  4 files changed, 14 insertions(+), 19 deletions(-)
> 
[...]
> 
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 94181d9281..ac292b184a 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -1493,16 +1493,10 @@ static int handle_e3(S390CPU *cpu, struct kvm_run 
> *run, uint8_t ipbl)
>  
>  static int handle_hypercall(S390CPU *cpu, struct kvm_run *run)

Might as well make the return type void then.
More importantly, are you changing the behavior?
If I'm reading the code right, previously handle_instruction would inject an
additional PGM_OPERATION due to the negative return value, which does seem off 
to me.
If so, IMO this change should go into a separate patch.
I'm also wondering if the injection of PGM_SPECIFICATION should just go into
handle_diag_500 and handle_hypercall should just be inlined.

>  {
> -    CPUS390XState *env = &cpu->env;
> -    int ret;
> -
> -    ret = s390_virtio_hypercall(env);
> -    if (ret == -EINVAL) {
> +    if (handle_diag_500(&cpu->env)) {
>          kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
> -        return 0;
>      }
> -
> -    return ret;
> +    return 0;
>  }