Re: [Qemu-devel] [PATCH 2/2] KVM: s390: add floating irq controller

[Heiko Carstens]

On Wed, Jul 31, 2013 at 11:08:15AM +0200, Cornelia Huck wrote:
> On Mon, 29 Jul 2013 15:59:53 +0200
> Jens Freimann <address@hidden> wrote:
> 
> > This patch adds a floating irq controller as a kvm_device.
> > It will be necesary for migration of floating interrupts as well
> > as for hardening the reset code by allowing user space to explicitly
> > remove all pending floating interrupts.
> > 
> > Signed-off-by: Jens Freimann <address@hidden>
> > ---
> >  arch/s390/include/uapi/asm/kvm.h |   5 +
> >  arch/s390/kvm/interrupt.c        | 210 
> > +++++++++++++++++++++++++++++++--------
> >  arch/s390/kvm/kvm-s390.c         |   1 +
> >  include/linux/kvm_host.h         |   1 +
> >  include/uapi/linux/kvm.h         |   1 +
> >  virt/kvm/kvm_main.c              |   3 +
> >  6 files changed, 181 insertions(+), 40 deletions(-)
> > 
> 
> > +static int flic_set_attr(struct kvm_device *dev, struct kvm_device_attr 
> > *attr)
> > +{
> > +   int r;
> > +   struct kvm_s390_irq *s390irq;
> > +   struct kvm_s390_interrupt_info *inti;
> > +
> > +   switch (attr->group) {
> > +   case KVM_DEV_FLIC_ENQUEUE:
> > +           inti = kzalloc(sizeof(*inti), GFP_KERNEL);
> > +           if (!inti) {
> > +                   r = -ENOMEM;
> > +                   goto out;
> > +           }
> > +           s390irq = kzalloc(sizeof(*s390irq), GFP_KERNEL);
> > +           if (!s390irq) {
> > +                   r = -ENOMEM;
> > +                   goto out_free_inti;
> > +           }
> > +           if (copy_from_user(s390irq, (u64 __user *)attr->addr,
> > +                             sizeof(s390irq))) {
> > +                   r = -EFAULT;
> > +                   goto out_free_s390irq;
> > +           }
> 
> Can't you just copy into inti->irq here? You'd get rid of allocating
> two structures that way.
> 
> > +           switch(s390irq->type) {
> > +           case KVM_S390_INT_VIRTIO:
> > +           case KVM_S390_INT_SERVICE:
> > +           case KVM_S390_INT_IO_MIN...KVM_S390_INT_IO_MAX:
> > +           case KVM_S390_MCHK:
> > +                   inti->irq = *s390irq;
> > +                   __inject_vm(dev->kvm, inti);
> > +           default:
> > +                   r = -EINVAL;
> > +           }
> > +           break;

And, due to missing break statement, it will always return -EINVAL ;)