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

[Alistair Francis]

On Tue, Aug 9, 2016 at 9:42 AM, Peter Maydell <address@hidden> wrote:
> 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?

Yep, it is. I have fixed this.

>
>> 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.

I think I was thinking of the direction from the devices. So the
out_irqs are the lines that go out from the device and the in_irq goes
into the next device (the GIC in this case).

Although that probably is somewhat confusing.

>
>> +    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.

It should be bools, I just couldn't find the VMstate macro for bools
at the time.

I have fixed this.

>
>> +    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.

Ok, I have fixed this up. It now follows the standard API.

>
>> +{
>> +    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.

Fixed.

Thanks,

Alistair

>
>> +}
>> +
>> +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