Re: [Qemu-devel] [PATCH v5 1/4] s390x: split flic into kvm and non-kvm parts

[Alexander Graf <agraf@suse.de>]

On 05/08/2014 03:03 PM, Cornelia Huck wrote:
> Introduce a common parent class for both cases, where kvm and non-kvm
> can hook up callbacks. This will be used by follow-on patches for
> adapter registration and mapping.
>
> We now always have a flic, regardless of whether we use kvm; the
> non-kvm implementation just doesn't do anything.
>
> Reviewed-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
>   default-configs/s390x-softmmu.mak |    3 +-
>   hw/intc/Makefile.objs             |    1 +
>   hw/intc/s390_flic.c               |  318 ++++--------------------------------
>   hw/intc/s390_flic_kvm.c           |  325 +++++++++++++++++++++++++++++++++++++
>   include/hw/s390x/s390_flic.h      |   51 ++++--
>   5 files changed, 399 insertions(+), 299 deletions(-)
>   create mode 100644 hw/intc/s390_flic_kvm.c
>
> diff --git a/default-configs/s390x-softmmu.mak b/default-configs/s390x-softmmu.mak
> index d843dc0..126d88d 100644
> --- a/default-configs/s390x-softmmu.mak
> +++ b/default-configs/s390x-softmmu.mak
> @@ -1,3 +1,4 @@
>   CONFIG_VIRTIO=y
>   CONFIG_SCLPCONSOLE=y
> -CONFIG_S390_FLIC=$(CONFIG_KVM)
> +CONFIG_S390_FLIC=y
> +CONFIG_S390_FLIC_KVM=$(CONFIG_KVM)
> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> index c8a2318..843864a 100644
> --- a/hw/intc/Makefile.objs
> +++ b/hw/intc/Makefile.objs
> @@ -26,3 +26,4 @@ obj-$(CONFIG_XICS) += xics.o
>   obj-$(CONFIG_XICS_KVM) += xics_kvm.o
>   obj-$(CONFIG_ALLWINNER_A10_PIC) += allwinner-a10-pic.o
>   obj-$(CONFIG_S390_FLIC) += s390_flic.o
> +obj-$(CONFIG_S390_FLIC_KVM) += s390_flic_kvm.o
> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> index b2ef3e3..7dc8c7d 100644
> --- a/hw/intc/s390_flic.c
> +++ b/hw/intc/s390_flic.c
> @@ -1,322 +1,66 @@
>   /*
> - * QEMU S390x KVM floating interrupt controller (flic)
> + * QEMU S390x floating interrupt controller (flic)
>    *
>    * Copyright 2014 IBM Corp.
>    * Author(s): Jens Freimann <jfrei@linux.vnet.ibm.com>
> + *            Cornelia Huck <cornelia.huck@de.ibm.com>
>    *
>    * This work is licensed under the terms of the GNU GPL, version 2 or (at
>    * your option) any later version. See the COPYING file in the top-level
>    * directory.
>    */
>   
> -#include <sys/ioctl.h>
>   #include "qemu/error-report.h"
>   #include "hw/sysbus.h"
> -#include "sysemu/kvm.h"
>   #include "migration/qemu-file.h"
>   #include "hw/s390x/s390_flic.h"
>   #include "trace.h"
>   
> -#define FLIC_SAVE_INITIAL_SIZE getpagesize()
> -#define FLIC_FAILED (-1UL)
> -#define FLIC_SAVEVM_VERSION 1
> -
> -void s390_flic_init(void)
> -{
> -    DeviceState *dev;
> -    int r;
> -
> -    if (kvm_enabled()) {
> -        dev = qdev_create(NULL, "s390-flic");
> -        object_property_add_child(qdev_get_machine(), "s390-flic",
> -                                OBJECT(dev), NULL);
> -        r = qdev_init(dev);
> -        if (r) {
> -            error_report("flic: couldn't create qdev");
> -        }
> -    }
> -}
> -
> -/**
> - * flic_get_all_irqs - store all pending irqs in buffer
> - * @buf: pointer to buffer which is passed to kernel
> - * @len: length of buffer
> - * @flic: pointer to flic device state
> - *
> - * Returns: -ENOMEM if buffer is too small,
> - * -EINVAL if attr.group is invalid,
> - * -EFAULT if copying to userspace failed,
> - * on success return number of stored interrupts
> - */
> -static int flic_get_all_irqs(KVMS390FLICState *flic,
> -                             void *buf, int len)
> -{
> -    struct kvm_device_attr attr = {
> -        .group = KVM_DEV_FLIC_GET_ALL_IRQS,
> -        .addr = (uint64_t) buf,
> -        .attr = len,
> -    };
> -    int rc;
> -
> -    rc = ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr);
> -
> -    return rc == -1 ? -errno : rc;
> -}
> -
> -static void flic_enable_pfault(KVMS390FLICState *flic)
> -{
> -    struct kvm_device_attr attr = {
> -        .group = KVM_DEV_FLIC_APF_ENABLE,
> -    };
> -    int rc;
> -
> -    rc = ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
> -
> -    if (rc) {
> -        fprintf(stderr, "flic: couldn't enable pfault\n");
> -    }
> -}
> -
> -static void flic_disable_wait_pfault(KVMS390FLICState *flic)
> +S390FLICState *s390_get_flic(void)
>   {
> -    struct kvm_device_attr attr = {
> -        .group = KVM_DEV_FLIC_APF_DISABLE_WAIT,
> -    };
> -    int rc;
> -
> -    rc = ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
> +    S390FLICState *fs;
>   
> -    if (rc) {
> -        fprintf(stderr, "flic: couldn't disable pfault\n");
> +    fs = S390_FLIC_COMMON(object_resolve_path(TYPE_KVM_S390_FLIC, NULL));
> +    if (!fs) {
> +        fs = S390_FLIC_COMMON(object_resolve_path(TYPE_QEMU_S390_FLIC, NULL));
>       }
> +    return fs;
>   }
>   
> -/** flic_enqueue_irqs - returns 0 on success
> - * @buf: pointer to buffer which is passed to kernel
> - * @len: length of buffer
> - * @flic: pointer to flic device state
> - *
> - * Returns: -EINVAL if attr.group is unknown
> - */
> -static int flic_enqueue_irqs(void *buf, uint64_t len,
> -                            KVMS390FLICState *flic)
> -{
> -    int rc;
> -    struct kvm_device_attr attr = {
> -        .group = KVM_DEV_FLIC_ENQUEUE,
> -        .addr = (uint64_t) buf,
> -        .attr = len,
> -    };
> -
> -    rc = ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
> -
> -    return rc ? -errno : 0;
> -}
> -
> -/**
> - * __get_all_irqs - store all pending irqs in buffer
> - * @flic: pointer to flic device state
> - * @buf: pointer to pointer to a buffer
> - * @len: length of buffer
> - *
> - * Returns: return value of flic_get_all_irqs
> - * Note: Retry and increase buffer size until flic_get_all_irqs
> - * either returns a value >= 0 or a negative error code.
> - * -ENOMEM is an exception, which means the buffer is too small
> - * and we should try again. Other negative error codes can be
> - * -EFAULT and -EINVAL which we ignore at this point
> - */
> -static int __get_all_irqs(KVMS390FLICState *flic,
> -                          void **buf, int len)
> +void s390_flic_init(void)
>   {
> +    DeviceState *dev;
>       int r;
>   
> -    do {
> -        /* returns -ENOMEM if buffer is too small and number
> -         * of queued interrupts on success */
> -        r = flic_get_all_irqs(flic, *buf, len);
> -        if (r >= 0) {
> -            break;
> -        }
> -        len *= 2;
> -        *buf = g_try_realloc(*buf, len);
> -        if (!buf) {
> -            return -ENOMEM;
> -        }
> -    } while (r == -ENOMEM && len <= KVM_S390_FLIC_MAX_BUFFER);
> -
> -    return r;
> -}
> -
> -/**
> - * kvm_flic_save - Save pending floating interrupts
> - * @f: QEMUFile containing migration state
> - * @opaque: pointer to flic device state
> - *
> - * Note: Pass buf and len to kernel. Start with one page and
> - * increase until buffer is sufficient or maxium size is
> - * reached
> - */
> -static void kvm_flic_save(QEMUFile *f, void *opaque)
> -{
> -    KVMS390FLICState *flic = opaque;
> -    int len = FLIC_SAVE_INITIAL_SIZE;
> -    void *buf;
> -    int count;
> -
> -    flic_disable_wait_pfault((struct KVMS390FLICState *) opaque);
> -
> -    buf = g_try_malloc0(len);
> -    if (!buf) {
> -        /* Storing FLIC_FAILED into the count field here will cause the
> -         * target system to fail when attempting to load irqs from the
> -         * migration state */
> -        error_report("flic: couldn't allocate memory");
> -        qemu_put_be64(f, FLIC_FAILED);
> -        return;
> +    dev = s390_flic_kvm_create();

Why have this helper? Can't that logic live in the board file?


Alex