Re: [Qemu-devel] [PATCH v6 5/8] irq: Add a new irq device that allows the ORing of lines

[Peter Maydell]

On 3 August 2016 at 04:10, Alistair Francis <address@hidden> wrote:
> Signed-off-by: Alistair Francis <address@hidden>
> ---
> As the migration framework is not included in user mode this needs to be a
> new file.
>
> V6:
>  - Make the OR IRQ device a TYPE_DEVICE
>  - Add vmstate
>
>  hw/core/Makefile.objs |   1 +
>  hw/core/irq.c         |   1 +
>  hw/core/or-irq.c      | 102 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/irq.h      |  15 ++++++++
>  4 files changed, 119 insertions(+)
>  create mode 100644 hw/core/or-irq.c
>
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index cfd4840..b47241b 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -16,4 +16,5 @@ common-obj-$(CONFIG_SOFTMMU) += null-machine.o
>  common-obj-$(CONFIG_SOFTMMU) += loader.o
>  common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
>  common-obj-$(CONFIG_SOFTMMU) += register.o
> +common-obj-$(CONFIG_SOFTMMU) += or-irq.o
>  common-obj-$(CONFIG_PLATFORM_BUS) += platform-bus.o
> diff --git a/hw/core/irq.c b/hw/core/irq.c
> index 49ff2e6..dc874cc 100644
> --- a/hw/core/irq.c
> +++ b/hw/core/irq.c
> @@ -24,6 +24,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "hw/irq.h"
> +#include "hw/sysbus.h"
>  #include "qom/object.h"
>
>  #define IRQ(obj) OBJECT_CHECK(struct IRQState, (obj), TYPE_IRQ)

We don't change anything else in this file so I think this added include
is leftover from development?

> diff --git a/hw/core/or-irq.c b/hw/core/or-irq.c
> new file mode 100644
> index 0000000..2bd181b
> --- /dev/null
> +++ b/hw/core/or-irq.c
> @@ -0,0 +1,102 @@
> +/*
> + * QEMU IRQ/GPIO common code.
> + *
> + * Copyright (c) 2016 Alistair Francis <address@hidden>.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "hw/irq.h"
> +#include "hw/sysbus.h"
> +#include "qom/object.h"
> +
> +#define OR_IRQ(obj) OBJECT_CHECK(qemu_or_irq, (obj), TYPE_OR_IRQ)
> +
> +struct OrIRQState {
> +    Object parent_obj;
> +
> +    qemu_irq in_irq;
> +    qemu_irq *out_irqs;

Aren't these names backwards? An OR gate has multiple inputs
and one output.

> +    int16_t levels[MAX_OR_LINES];

Why is this an int16_t array? I was expecting an array of bool
(or whatever the equivalent is for vmstate), or alternatively
a bitmap.h bitmap with one bit per input.

> +    int n;
> +};
> +
> +static void or_irq_handler(void *opaque, int n, int level)
> +{
> +    qemu_or_irq *or_irq = OR_IRQ(opaque);
> +    int or_level = 0;
> +    int i;
> +
> +    or_irq->levels[n] = level;
> +
> +    for (i = 0; i < or_irq->n; i++) {
> +        or_level |= or_irq->levels[i];
> +    }
> +
> +    qemu_set_irq(or_irq->in_irq, or_level);
> +}
> +
> +qemu_irq *qemu_allocate_or_irqs(qemu_irq in_irq, int n)

This API is awkward because it means that there's no way for
the caller to arrange for the OR_IRQ object to be freed
(for instance if you want to use it in a hotpluggable device),
unless I'm missing something.

I was sort of expecting this to be just-another-device that
you would wire up with the existing sysbus gpio APIs.

> +{
> +    qemu_or_irq *or_irq;
> +
> +    assert(n < MAX_OR_LINES);
> +
> +    or_irq = OR_IRQ(object_new(TYPE_OR_IRQ));
> +    object_initialize(or_irq, sizeof(qemu_or_irq),
> +                      TYPE_OR_IRQ);
> +
> +    or_irq->out_irqs = qemu_allocate_irqs(or_irq_handler, or_irq, n);
> +    or_irq->in_irq = in_irq;
> +    or_irq->n = n;
> +
> +    return or_irq->out_irqs;
> +}
> +
> +static const VMStateDescription vmstate_or_irq = {
> +    .name = TYPE_OR_IRQ,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_INT16_ARRAY(levels, qemu_or_irq, MAX_OR_LINES),
> +        VMSTATE_END_OF_LIST(),
> +    }
> +};
> +
> +static void or_irq_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->vmsd = &vmstate_or_irq;

I think this also needs a reset method that just zeroes
the levels array.

> +}
> +
> +static const TypeInfo or_irq_type_info = {
> +   .name = TYPE_OR_IRQ,
> +   .parent = TYPE_DEVICE,
> +   .instance_size = sizeof(qemu_or_irq),
> +   .class_init = or_irq_class_init,
> +};
> +
> +static void or_irq_register_types(void)
> +{
> +    type_register_static(&or_irq_type_info);
> +}
> +
> +type_init(or_irq_register_types)
> diff --git a/include/hw/irq.h b/include/hw/irq.h
> index 4c4c2ea..5e8a3b6 100644
> --- a/include/hw/irq.h
> +++ b/include/hw/irq.h
> @@ -4,8 +4,12 @@
>  /* Generic IRQ/GPIO pin infrastructure.  */
>
>  #define TYPE_IRQ "irq"
> +#define TYPE_OR_IRQ "or-irq"
> +
> +#define MAX_OR_LINES      16
>
>  typedef struct IRQState *qemu_irq;
> +typedef struct OrIRQState qemu_or_irq;
>
>  typedef void (*qemu_irq_handler)(void *opaque, int n, int level);
>
> @@ -38,6 +42,17 @@ qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, 
> void *opaque, int n);
>   */
>  qemu_irq qemu_allocate_irq(qemu_irq_handler handler, void *opaque, int n);
>
> +/*
> + * qemu_allocate_or_irqs
> + * @in_irq: An input IRQ. It will be the result of the @out_irqs ORed 
> together
> + * @n: The number of interrupt lines that should be ORed together
> + *
> + * returns: An array of interrupts that should be ORed together
> + *
> + * OR all of the interrupts returned in the array into a single @in_irq.
> + */
> +qemu_irq *qemu_allocate_or_irqs(qemu_irq in_irq, int n);
> +
>  /* Extends an Array of IRQs. Old IRQs have their handlers and opaque data
>   * preserved. New IRQs are assigned the argument handler and opaque data.
>   */
> --
> 2.7.4
>

thanks
-- PMM