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

[David Hildenbrand]

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!
--
Cheers,

David / dhildenb