Re: [RFC PATCH v2 0/4] kvm: Make ioeventfd usable on s390.

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Sun, 24 Feb 2013 11:56:39 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Feb 22, 2013 at 01:09:45PM +0100, Cornelia Huck wrote:
> > Here's the second attempt at implementing ioeventfd for s390.
> 
> The patchset looks fine overall.
> Minor comments and questions below.

Cool, thanks for reviewing.

> 
> > 
> > Rather than the architecture-specific functions used in v1, we
> > now try to integrate with the kvm_io_device infrastructure.
> > Calls to diagnose 500 subcode 3 are now mapped to _write.
> > These devices are created on a new KVM_CSS_BUS when using a
> > new flag KVM_IOEVENTFD_FLAG_CSS. addr and datamatch are (ab)used
> > to contain the subchannel id and the virtqueue.
> > 
> > A drawback is that this interface is not easily extendable should
> > we want to attach other hypercalls or carry more payload.
> 
> Under s390 kvm_hypercallX already uses diagnose 500 so that seems
> fine. If you want to make it more generic and support
> more subcodes, I think you'll have to pass an extra u64 field:
> to bus to both avoid overflowing int value and avoid ugly
> bus-specific hacks in generic code.
> 
> Will we ever need that? Code using subcode 3 does not yet seem
> to be upstream in 3.8 so maybe yes, but you decide.

Subcode 3 will be in 3.9.

> An alternative is to add new bus types when kvm needs to handle
> new subcodes. So e.g. KVM_BUS_S390_VIRTIO_CCW_NOTIFY and
> KVM_BUS_S390_VIRTIO_NOTIFY ?

I think I'll fall back to a new bus type should we ever need a new
notification type - the less strange hacks, the better.

> 
> You decide, I'm fine with either approach.
> 
> More minor comments and questions in response to individual patches.
> 
> > Another limitation is the limit of 1000 io devices per bus, which
> > we would hit easily with a few hundred devices, but that should
> > be fixable.
> > 
> > v1 -> v2:
> > - Move irqfd initialization from a module init function to kvm_init,
> >   eliminating the need for a second module for kvm/s390.
> > - Use kvm_io_device for s390 css devices.
> > 
> > 
> > Cornelia Huck (4):
> >   KVM: Initialize irqfd from kvm_init().
> >   KVM: Introduce KVM_CSS_BUS.
> >   KVM: ioeventfd for s390 css devices.
> >   KVM: s390: Wire up ioeventfd.
> > 
> >  Documentation/virtual/kvm/api.txt |  7 +++++++
> >  arch/s390/kvm/Kconfig             |  1 +
> >  arch/s390/kvm/Makefile            |  2 +-
> >  arch/s390/kvm/diag.c              | 25 +++++++++++++++++++++++++
> >  arch/s390/kvm/kvm-s390.c          |  1 +
> >  include/linux/kvm_host.h          | 14 ++++++++++++++
> >  include/uapi/linux/kvm.h          |  2 ++
> >  virt/kvm/eventfd.c                | 15 ++++++++-------
> >  virt/kvm/kvm_main.c               |  6 ++++++
> >  9 files changed, 65 insertions(+), 8 deletions(-)
> > 
> > -- 
> > 1.7.12.4
>