Re: [Qemu-devel] [PATCH 3/8] [PATCH RFC v3] s390-qemu: cpu hotplug - SCLP Event integration

["Andreas Färber" <afaerber@suse.de>]

Am 01.08.2013 16:12, schrieb Jason J. Herne:
> From: "Jason J. Herne" <jjherne@us.ibm.com>
> 
> Add an sclp event for "cpu was hot plugged".  This allows Qemu to deliver an
> SCLP interrupt to the guest stating that the requested cpu hotplug was
> completed.
> 
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> ---
>  hw/s390x/Makefile.objs            |    2 +-
>  hw/s390x/event-facility.c         |    7 +++
>  hw/s390x/sclpcpu.c                |  120 +++++++++++++++++++++++++++++++++++++
>  include/hw/s390x/event-facility.h |    3 +
>  include/hw/s390x/sclp.h           |    1 +
>  5 files changed, 132 insertions(+), 1 deletion(-)
>  create mode 100644 hw/s390x/sclpcpu.c
> 
> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> index 77e1218..104ae8e 100644
> --- a/hw/s390x/Makefile.objs
> +++ b/hw/s390x/Makefile.objs
> @@ -2,7 +2,7 @@ obj-y = s390-virtio-bus.o s390-virtio.o
>  obj-y += s390-virtio-hcall.o
>  obj-y += sclp.o
>  obj-y += event-facility.o
> -obj-y += sclpquiesce.o
> +obj-y += sclpquiesce.o sclpcpu.o

On a line of its own for consistency and to avoid '-' line?

>  obj-y += ipl.o
>  obj-y += css.o
>  obj-y += s390-virtio-ccw.o
> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> index 0faade0..aec35cb 100644
> --- a/hw/s390x/event-facility.c
> +++ b/hw/s390x/event-facility.c
> @@ -317,6 +317,7 @@ static int init_event_facility(S390SCLPDevice *sdev)
>  {
>      SCLPEventFacility *event_facility;
>      DeviceState *quiesce;
> +    DeviceState *cpu_hotplug;
>  
>      event_facility = g_malloc0(sizeof(SCLPEventFacility));
>      sdev->ef = event_facility;
> @@ -335,6 +336,12 @@ static int init_event_facility(S390SCLPDevice *sdev)
>      }
>      qdev_init_nofail(quiesce);
>  
> +    cpu_hotplug = qdev_create(&event_facility->sbus.qbus, "sclpcpuhotplug");

Please don't create devices in such an init function. Also don't access
.qbus please.

Instead, please use object_initialize() followed by
qdev_set_parent_bus() in an instance_init function.

Conversion of the initfn to a realizefn will be a bit more involved so I
won't ask that for this series, but if there were volunteers among your
colleagues that would be appreciated. The effect would be to propagate
errors to the caller of the realizefn rather than asserting here.

> +    if (!cpu_hotplug) {
> +        return -1;
> +    }
> +    qdev_init_nofail(cpu_hotplug);
> +
>      return 0;
>  }
>  
> diff --git a/hw/s390x/sclpcpu.c b/hw/s390x/sclpcpu.c
> new file mode 100644
> index 0000000..5b4139e
> --- /dev/null
> +++ b/hw/s390x/sclpcpu.c
> @@ -0,0 +1,120 @@
> +/*
> + * SCLP event type
> + *    Signal CPU - Trigger SCLP interrupt for system CPU configure or
> + *    de-configure
> + *
> + * Copyright IBM, Corp. 2013
> + *
> + * Authors:
> + *  Thang Pham <thang.pham@us.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 <hw/qdev.h>

"hw/qdev.h", but I'm guessing that's redundant with either sclp.h or
event-facility.h?

> +#include "sysemu/sysemu.h"
> +#include "hw/s390x/sclp.h"
> +#include "hw/s390x/event-facility.h"
> +#include "cpu.h"
> +#include "sysemu/cpus.h"
> +#include "sysemu/kvm.h"
> +
> +typedef struct ConfigMgtData {
> +    EventBufferHeader ebh;
> +    uint8_t reserved;
> +    uint8_t event_qualifier;
> +} QEMU_PACKED ConfigMgtData;
> +
> +static qemu_irq irq_cpu_hotplug; /* Only used in this file */
> +
> +#define EVENT_QUAL_CPU_CHANGE  1
> +
> +void raise_irq_cpu_hotplug(void)
> +{
> +    qemu_irq_raise(irq_cpu_hotplug);
> +}
> +
> +static int event_type(void)
> +{
> +    return SCLP_EVENT_CONFIG_MGT_DATA;
> +}
> +
> +static unsigned int send_mask(void)
> +{
> +    return SCLP_EVENT_MASK_CONFIG_MGT_DATA;
> +}
> +
> +static unsigned int receive_mask(void)
> +{
> +    return 0;
> +}
> +
> +static int read_event_data(SCLPEvent *event, EventBufferHeader *evt_buf_hdr,
> +                           int *slen)
> +{
> +    ConfigMgtData *cdata = (ConfigMgtData *) evt_buf_hdr;
> +    if (*slen < sizeof(ConfigMgtData)) {
> +        return 0;
> +    }
> +
> +    /* Event is no longer pending */
> +    if (!event->event_pending) {
> +        return 0;
> +    }
> +    event->event_pending = false;
> +
> +    /* Event header data */
> +    cdata->ebh.length = cpu_to_be16(sizeof(ConfigMgtData));
> +    cdata->ebh.type = SCLP_EVENT_CONFIG_MGT_DATA;
> +    cdata->ebh.flags |= SCLP_EVENT_BUFFER_ACCEPTED;
> +
> +    /* Trigger a rescan of CPUs by setting event qualifier */
> +    cdata->event_qualifier = EVENT_QUAL_CPU_CHANGE;
> +    *slen -= sizeof(ConfigMgtData);
> +
> +    return 1;
> +}
> +
> +static void trigger_signal(void *opaque, int n, int level)
> +{
> +    SCLPEvent *event = opaque;
> +    event->event_pending = true;
> +
> +    /* Trigger SCLP read operation */
> +    sclp_service_interrupt(0);
> +}
> +
> +static int irq_cpu_hotplug_init(SCLPEvent *event)
> +{
> +    irq_cpu_hotplug = *qemu_allocate_irqs(trigger_signal, event, 1);
> +    return 0;
> +}
> +
> +static void cpu_class_init(ObjectClass *klass, void *data)
> +{
> +    SCLPEventClass *k = SCLP_EVENT_CLASS(klass);
> +
> +    k->init = irq_cpu_hotplug_init;
> +    k->get_send_mask = send_mask;
> +    k->get_receive_mask = receive_mask;
> +    k->event_type = event_type;
> +    k->read_event_data = read_event_data;
> +    k->write_event_data = NULL;
> +}
> +
> +static TypeInfo sclp_cpu_info = {

static const

> +    .name          = "sclpcpuhotplug",

Please use dashes rather than concatenation.

> +    .parent        = TYPE_SCLP_EVENT,
> +    .instance_size = sizeof(SCLPEvent),
> +    .class_init    = cpu_class_init,
> +    .class_size    = sizeof(SCLPEventClass),
> +};
> +
> +static void register_types(void)
> +{
> +    type_register_static(&sclp_cpu_info);
> +}
> +
> +type_init(register_types)
> +

Trailing white line.

> diff --git a/include/hw/s390x/event-facility.h b/include/hw/s390x/event-facility.h
> index 791ab2a..d63969f 100644
> --- a/include/hw/s390x/event-facility.h
> +++ b/include/hw/s390x/event-facility.h
> @@ -17,14 +17,17 @@
>  
>  #include <hw/qdev.h>

Ah, copy&paste and answer to the above. ;)

>  #include "qemu/thread.h"
> +#include "hw/s390x/sclp.h"
>  
>  /* SCLP event types */
> +#define SCLP_EVENT_CONFIG_MGT_DATA              0x04
>  #define SCLP_EVENT_ASCII_CONSOLE_DATA           0x1a
>  #define SCLP_EVENT_SIGNAL_QUIESCE               0x1d
>  
>  /* SCLP event masks */
>  #define SCLP_EVENT_MASK_SIGNAL_QUIESCE          0x00000008
>  #define SCLP_EVENT_MASK_MSG_ASCII               0x00000040
> +#define SCLP_EVENT_MASK_CONFIG_MGT_DATA         0x10000000
>  
>  #define SCLP_UNCONDITIONAL_READ                 0x00
>  #define SCLP_SELECTIVE_READ                     0x01
> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
> index 89ae7d1..7728ad8 100644
> --- a/include/hw/s390x/sclp.h
> +++ b/include/hw/s390x/sclp.h
> @@ -154,5 +154,6 @@ typedef struct S390SCLPDeviceClass {
>  
>  void s390_sclp_init(void);
>  void sclp_service_interrupt(uint32_t sccb);
> +void raise_irq_cpu_hotplug(void);
>  
>  #endif
> 

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg