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

[David Hildenbrand]

On 30.09.24 13:11, Christian Borntraeger wrote:
> 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.

Right, we use it in the absence of KVM_CAP_S390_USER_INSTR0.

> 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.

It would be one additional trigger during system boot, so likely not 
really an issue. We could always add new stats for selected subcodes 
(i.e, KVM_S390_VIRTIO_CCW_NOTIFY).

> 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.

Right, it's very unlikely to be noticeable at all.

Thanks for the feedback!

--
Cheers,

David / dhildenb