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

[David Hildenbrand]

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:

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)
 {
+    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:


--
Cheers,

David / dhildenb