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

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

On Tue, 2024-09-17 at 13:02 +0200, David Hildenbrand wrote:
> On 17.09.24 12:50, David Hildenbrand wrote:
> > On 17.09.24 12:45, David Hildenbrand wrote:
> > > On 12.09.24 15:22, Nina Schoetterl-Glausch wrote:
> > > > 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.
> > > 
> > > Agreed.
> > > 
> > > > 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.
> > > 
> > > You are right, agreed.
> > 
> > Ah, reading again, we have a "return 0;" after the
> > "kvm_s390_program_interrupt", so it should work as expected.
> > 
> 
> The following in should be what you suggest:

Yup, looks good.
> 
> diff --git a/hw/s390x/s390-virtio-hcall.c b/hw/s390x/s390-virtio-hcall.c
> index 4cddf69fbb..5fb78a719e 100644
> --- a/hw/s390x/s390-virtio-hcall.c
> +++ b/hw/s390x/s390-virtio-hcall.c
> @@ -57,18 +57,19 @@ static int handle_virtio_ccw_notify(uint64_t subch_id, uint64_t data)
>       return 0;
>   }
>   
> -int handle_diag_500(CPUS390XState *env)
> +void handle_diag_500(S390CPU *cpu, uintptr_t ra)

I don't see a point to passing a cpu when we only need the env, but it doesn't matter.

>   {
> +    CPUS390XState *env = &cpu->env;
>       const uint64_t subcode = env->regs[1];
>   
>       switch (subcode) {
>       case DIAG500_VIRTIO_NOTIFY:
>           env->regs[2] = handle_virtio_notify(env->regs[2]);
> -        return 0;
> +        break;
>       case DIAG500_VIRTIO_CCW_NOTIFY:
>           env->regs[2] = handle_virtio_ccw_notify(env->regs[2], env->regs[3]);
> -        return 0;
> +        break;
>       default:
> -        return -EINVAL;
> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>       }
>   }
> diff --git a/hw/s390x/s390-virtio-hcall.h b/hw/s390x/s390-virtio-hcall.h
> index e4f76aca82..dca456b926 100644
> --- a/hw/s390x/s390-virtio-hcall.h
> +++ b/hw/s390x/s390-virtio-hcall.h
> @@ -19,6 +19,6 @@
>   #define DIAG500_VIRTIO_SET_STATUS       2 /* legacy */
>   #define DIAG500_VIRTIO_CCW_NOTIFY       3 /* KVM_S390_VIRTIO_CCW_NOTIFY */
>   
> -int handle_diag_500(CPUS390XState *env);
> +void handle_diag_500(S390CPU *cpu, uintptr_t ra);
>   
>   #endif /* HW_S390_VIRTIO_HCALL_H */
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index ac292b184a..2f9a54fe04 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -1491,14 +1491,6 @@ static int handle_e3(S390CPU *cpu, struct kvm_run *run, uint8_t ipbl)
>       return r;
>   }
>   
> -static int handle_hypercall(S390CPU *cpu, struct kvm_run *run)
> -{
> -    if (handle_diag_500(&cpu->env)) {
> -        kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
> -    }
> -    return 0;
> -}
> -
>   static void kvm_handle_diag_288(S390CPU *cpu, struct kvm_run *run)
>   {
>       uint64_t r1, r3;
> @@ -1595,7 +1587,7 @@ static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb)
>           handle_diag_318(cpu, run);
>           break;
>       case DIAG_KVM_HYPERCALL:
> -        r = handle_hypercall(cpu, run);
> +        handle_diag_500(cpu, RA_IGNORED)
>           break;
>       case DIAG_KVM_BREAKPOINT:
>           r = handle_sw_breakpoint(cpu, run);
> diff --git a/target/s390x/tcg/misc_helper.c b/target/s390x/tcg/misc_helper.c
> index 58757585a2..170fe00629 100644
> --- a/target/s390x/tcg/misc_helper.c
> +++ b/target/s390x/tcg/misc_helper.c
> @@ -119,7 +119,7 @@ void HELPER(diag)(CPUS390XState *env, uint32_t r1, uint32_t r3, uint32_t num)
>       case 0x500:
>           /* QEMU/KVM hypercall */
>           bql_lock();
> -        r = handle_diag_500(env);
> +        handle_diag_500(env_archcpu(env), GETPC());
>           bql_unlock();
>           break;
>       case 0x44:
> 
>