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 <da...@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: > >