| Message ID | 20240910175809.2135596-4-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: virtio-mem support | expand |
On 10/09/2024 19.57, 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(-) Reviewed-by: Thomas Huth <thuth@redhat.com>
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; > }
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. > I'm also wondering if the injection of PGM_SPECIFICATION should just go into > handle_diag_500 and handle_hypercall should just be inlined. > I had exactly that, and used s390_program_interrupt(env, PGM_SPECIFICATION, ra); let me revive that. Thanks!
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.
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:
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: > >
diff --git a/hw/s390x/s390-virtio-hcall.c b/hw/s390x/s390-virtio-hcall.c index ca49e3cd22..4cddf69fbb 100644 --- a/hw/s390x/s390-virtio-hcall.c +++ b/hw/s390x/s390-virtio-hcall.c @@ -1,5 +1,5 @@ /* - * Support for virtio hypercalls on s390 + * Support for QEMU/KVM hypercalls on s390 * * Copyright 2012 IBM Corp. * Author(s): Cornelia Huck <cornelia.huck@de.ibm.com> @@ -57,15 +57,15 @@ static int handle_virtio_ccw_notify(uint64_t subch_id, uint64_t data) return 0; } -int s390_virtio_hypercall(CPUS390XState *env) +int handle_diag_500(CPUS390XState *env) { const uint64_t subcode = env->regs[1]; switch (subcode) { - case KVM_S390_VIRTIO_NOTIFY: + case DIAG500_VIRTIO_NOTIFY: env->regs[2] = handle_virtio_notify(env->regs[2]); return 0; - case KVM_S390_VIRTIO_CCW_NOTIFY: + case DIAG500_VIRTIO_CCW_NOTIFY: env->regs[2] = handle_virtio_ccw_notify(env->regs[2], env->regs[3]); return 0; default: diff --git a/hw/s390x/s390-virtio-hcall.h b/hw/s390x/s390-virtio-hcall.h index 3d9fe147d2..e4f76aca82 100644 --- a/hw/s390x/s390-virtio-hcall.h +++ b/hw/s390x/s390-virtio-hcall.h @@ -1,5 +1,5 @@ /* - * Support for virtio hypercalls on s390x + * Support for QEMU/KVM hypercalls on s390x * * Copyright IBM Corp. 2012, 2017 * Author(s): Cornelia Huck <cornelia.huck@de.ibm.com> @@ -12,12 +12,13 @@ #ifndef HW_S390_VIRTIO_HCALL_H #define HW_S390_VIRTIO_HCALL_H -#include "standard-headers/asm-s390/virtio-ccw.h" #include "cpu.h" -/* The only thing that we need from the old kvm_virtio.h file */ -#define KVM_S390_VIRTIO_NOTIFY 0 +#define DIAG500_VIRTIO_NOTIFY 0 /* legacy, implemented as a NOP */ +#define DIAG500_VIRTIO_RESET 1 /* legacy */ +#define DIAG500_VIRTIO_SET_STATUS 2 /* legacy */ +#define DIAG500_VIRTIO_CCW_NOTIFY 3 /* KVM_S390_VIRTIO_CCW_NOTIFY */ -int s390_virtio_hypercall(CPUS390XState *env); +int handle_diag_500(CPUS390XState *env); #endif /* HW_S390_VIRTIO_HCALL_H */ 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) { - 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; } static void kvm_handle_diag_288(S390CPU *cpu, struct kvm_run *run) diff --git a/target/s390x/tcg/misc_helper.c b/target/s390x/tcg/misc_helper.c index 303f86d363..58757585a2 100644 --- a/target/s390x/tcg/misc_helper.c +++ b/target/s390x/tcg/misc_helper.c @@ -117,9 +117,9 @@ void HELPER(diag)(CPUS390XState *env, uint32_t r1, uint32_t r3, uint32_t num) switch (num) { case 0x500: - /* KVM hypercall */ + /* QEMU/KVM hypercall */ bql_lock(); - r = s390_virtio_hypercall(env); + r = handle_diag_500(env); bql_unlock(); break; case 0x44:
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(-)