Re: [Qemu-devel] [PATCH 3/3] s390-sclp: SCLP Event integration

[Andreas Färber]

Hi,

Thanks for splitting these off.

Am 20.01.2014 17:56, schrieb Jason J. Herne:
> From: "Jason J. Herne" <address@hidden>
> 
> 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 <address@hidden>
> ---
>  hw/s390x/Makefile.objs            |   1 +
>  hw/s390x/event-facility.c         |   6 ++
>  hw/s390x/sclpcpu.c                | 113 
> ++++++++++++++++++++++++++++++++++++++
>  include/hw/s390x/event-facility.h |   3 +
>  include/hw/s390x/sclp.h           |   1 +
>  5 files changed, 124 insertions(+)
>  create mode 100644 hw/s390x/sclpcpu.c
[...]
> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> index 25951a0..575d407 100644
> --- a/hw/s390x/event-facility.c
> +++ b/hw/s390x/event-facility.c
> @@ -32,6 +32,8 @@ struct SCLPEventFacility {
>      unsigned int receive_mask;
>  };
>  
> +SCLPEvent cpu_hotplug;
> +
>  /* return true if any child has event pending set */
>  static bool event_pending(SCLPEventFacility *ef)
>  {
> @@ -335,6 +337,10 @@ static int init_event_facility(S390SCLPDevice *sdev)
>      }
>      qdev_init_nofail(quiesce);
>  
> +    object_initialize(&cpu_hotplug, sizeof(cpu_hotplug), "sclp-cpu-hotplug");

Any chance we can use a TYPE_* define in event-facility.h?

> +    qdev_set_parent_bus(DEVICE(&cpu_hotplug), BUS(&event_facility->sbus));
> +    object_property_set_bool(OBJECT(&cpu_hotplug), true, "realized", NULL);

Otherwise looks okay to me, except for two minor nits below.

> +
>      return 0;
>  }
>  
> diff --git a/hw/s390x/sclpcpu.c b/hw/s390x/sclpcpu.c
> new file mode 100644
> index 0000000..7fd1f9e
> --- /dev/null
> +++ b/hw/s390x/sclpcpu.c
[...]
> +static void cpu_class_init(ObjectClass *klass, void *data)

Please use oc to avoid the deliberately misspelled klass.

> +{
> +    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->read_event_data = read_event_data;
> +    k->write_event_data = NULL;
> +}
> +
> +static const TypeInfo sclp_cpu_info = {
> +    .name          = "sclp-cpu-hotplug",
> +    .parent        = TYPE_SCLP_EVENT,
> +    .instance_size = sizeof(SCLPEvent),
> +    .class_init    = cpu_class_init,
> +    .class_size    = sizeof(SCLPEventClass),
> +};
> +
> +static void register_types(void)

Please use your unique prefix: sclp_cpu_register_types

> +{
> +    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 7ce7079..4347f06 100644
> --- a/include/hw/s390x/event-facility.h
> +++ b/include/hw/s390x/event-facility.h
> @@ -17,10 +17,12 @@
>  
>  #include <hw/qdev.h>
>  #include "qemu/thread.h"
> +#include "hw/s390x/sclp.h"
>  
>  /* SCLP event types */
>  #define SCLP_EVENT_OPRTNS_COMMAND               0x01
>  #define SCLP_EVENT_MESSAGE                      0x02
> +#define SCLP_EVENT_CONFIG_MGT_DATA              0x04
>  #define SCLP_EVENT_PMSGCMD                      0x09
>  #define SCLP_EVENT_ASCII_CONSOLE_DATA           0x1a
>  #define SCLP_EVENT_SIGNAL_QUIESCE               0x1d
> @@ -28,6 +30,7 @@
>  /* 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_EVENT_MASK_OP_CMD                  0x80000000
>  #define SCLP_EVENT_MASK_MSG                     0x40000000
>  #define SCLP_EVENT_MASK_PMSGCMD                 0x00800000
[snip]

Regards,
Andreas

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