Re: [PATCH v1 07/14] s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT

[Christian Borntraeger]

Am 27.09.24 um 20:05 schrieb Halil Pasic:
> On Thu, 12 Sep 2024 10:19:00 +0200
> Thomas Huth <thuth@redhat.com> wrote:
>
> > > diff --git a/hw/s390x/s390-hypercall.h b/hw/s390x/s390-hypercall.h
> > > index b7ac29f444..f0ca62bcbb 100644
> > > --- a/hw/s390x/s390-hypercall.h
> > > +++ b/hw/s390x/s390-hypercall.h
> > > @@ -18,6 +18,7 @@
> > >    #define DIAG500_VIRTIO_RESET            1 /* legacy */
> > >    #define DIAG500_VIRTIO_SET_STATUS       2 /* legacy */
> > >    #define DIAG500_VIRTIO_CCW_NOTIFY       3 /* KVM_S390_VIRTIO_CCW_NOTIFY */
> > > +#define DIAG500_STORAGE_LIMIT           4
> > >    
> > >    int handle_diag_500(CPUS390XState *env);
> >
> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> >
> > Sounds very reasonable to me - but it would be good to get an
> > Ack/Reviewed-by from IBM folks here (in case they prefer a different
> > interface)... hope they'll join the discussion!
> >
> >    Thomas
>
> According to Documentation/virt/kvm/s390/s390-diag.rst in the
> linux source tree DIAG 500 is for kvm virtio funcions.
>
> Based on the commit message this storagelimit DIAG is rather loosely
> tied to virtio if at all, so from that perspective DIAG may not be a
> perfect fit. OTOH I don't see a better fit either. I would prefer to
> have Christian's opinion on this. I have no strong opinion myself.

Some remarks:
500 with a new subcode would work, it is marked as KVM hypervisor call
in our docs. 501 was used in the past for software breakpoints.
So we could use 502 as the next free one (This is reserved for KVM).
We do have kvm_stat counters for 500, not sure if people debugging virtio
will care.
The only important question for me is, what code is generated by gcc for
the switch statement and will any variant slow down the virtio doorbell.
diag.c has
        if (!vcpu->kvm->arch.css_support ||
            (vcpu->run->s.regs.gprs[1] != KVM_S390_VIRTIO_CCW_NOTIFY))
                return -EOPNOTSUPP;

So 500+4 should probably not cause any harm apart from branch prediction
going wrong the first 2 or 3 notifies.

502 will make kvm_s390_handle_diag larger.

So I tend to go with 500+4.