[2/3] arm_gicv2m: Add GICv2m widget to support MSIs

The ARM GICv2m widget is a little device that handle MSI interrupt
writes to a trigger register and ties them to a range of interrupt lines
wires to the GIC.  It has a few status/id registers and the interrupt wires,
and that's about it.

A board instantiates the device by setting the base SPI number and
number SPIs for the frame.  The base-spi parameter is indexed in the SPI
number space only, so base-spi == 0, means IRQ number 32.  When a device
(the PCI host controller) writes to the trigger register, the payload is
the GIC IRQ number, so we have to subtract 32 from that and then index
into our frame of SPIs.

When instantiating a GICv2m device, tell PCI that we have instantiated
something that can deal with MSIs.  We rely on the board actually wiring
up the GICv2m to the PCI host controller.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 hw/intc/Makefile.objs |   1 +
 hw/intc/arm_gicv2m.c  | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 181 insertions(+)
 create mode 100644 hw/intc/arm_gicv2m.c

Hi Christoffer,
On 04/08/2015 11:20 PM, Christoffer Dall wrote:
> The ARM GICv2m widget is a little device that handle MSI interrupt
> writes to a trigger register and ties them to a range of interrupt lines
> wires to the GIC.  It has a few status/id registers and the interrupt wires,
> and that's about it.
> 
> A board instantiates the device by setting the base SPI number and
> number SPIs for the frame.  The base-spi parameter is indexed in the SPI
> number space only, so base-spi == 0, means IRQ number 32.  When a device
> (the PCI host controller) writes to the trigger register, the payload is
> the GIC IRQ number, so we have to subtract 32 from that and then index
> into our frame of SPIs.
> 
> When instantiating a GICv2m device, tell PCI that we have instantiated
> something that can deal with MSIs.  We rely on the board actually wiring
> up the GICv2m to the PCI host controller.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  hw/intc/Makefile.objs |   1 +
>  hw/intc/arm_gicv2m.c  | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 181 insertions(+)
>  create mode 100644 hw/intc/arm_gicv2m.c
> 
> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> index 843864a..6b63dfc 100644
> --- a/hw/intc/Makefile.objs
> +++ b/hw/intc/Makefile.objs
> @@ -15,6 +15,7 @@ common-obj-$(CONFIG_OPENPIC) += openpic.o
>  
>  obj-$(CONFIG_APIC) += apic.o apic_common.o
>  obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
> +obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o
we could put this above close to the other common-obj-$(CONFIG_ARM_GIC)
objects?
>  obj-$(CONFIG_STELLARIS) += armv7m_nvic.o
>  obj-$(CONFIG_EXYNOS4) += exynos4210_gic.o exynos4210_combiner.o
>  obj-$(CONFIG_GRLIB) += grlib_irqmp.o
> diff --git a/hw/intc/arm_gicv2m.c b/hw/intc/arm_gicv2m.c
> new file mode 100644
> index 0000000..a80a16d
> --- /dev/null
> +++ b/hw/intc/arm_gicv2m.c
> @@ -0,0 +1,180 @@
> +/*
> + *  GICv2m extension for MSI/MSI-x support with a GICv2-based system
> + *
> + * Copyright (C) 2015 Linaro, All rights reserved.
> + *
> + * Author: Christoffer Dall <christoffer.dall@linaro.org>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "hw/sysbus.h"
> +#include "hw/pci/msi.h"
> +
> +#define TYPE_GICV2M "gicv2m"
> +#define GICV2M(obj) OBJECT_CHECK(GICv2mState, (obj), TYPE_GICV2M)
> +
> +#define GICV2M_NUM_SPI_MAX 128
> +
Maybe we could add a comment that the model supports a single non secure
MSI register frame
> +#define V2M_MSI_TYPER           0x008
> +#define V2M_MSI_SETSPI_NS       0x040
> +#define V2M_MSI_IIDR            0xFCC
> +#define V2M_IIDR0               0xFD0
> +
> +#define PRODUCT_ID_QEMU         0x51 /* ASCII code Q */
> +#define IMPLEMENTER_ARM         0x43b
> +
> +typedef struct GICv2mState {
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion iomem;
> +    qemu_irq spi[GICV2M_NUM_SPI_MAX];
> +
/* first absolute SPI index */, to avoid mixing with ID?
> +    uint32_t base_spi;
> +    uint32_t num_spi;
> +} GICv2mState;
> +
> +static void gicv2m_set_irq(void *opaque, int irq)
> +{
> +    GICv2mState *s = (GICv2mState *)opaque;
> +
> +    qemu_irq_pulse(s->spi[irq]);
> +}
> +
> +static uint64_t gicv2m_read(void *opaque, hwaddr offset,
> +                            unsigned size)
> +{
> +    GICv2mState *s = (GICv2mState *)opaque;
> +    uint32_t val;
> +
> +    if (size != 4) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_read: bad size %u\n", size);
> +        return 0;
> +    }
> +
> +    switch (offset) {
> +    case V2M_MSI_TYPER:
> +        val = (s->base_spi + 32) << 16;
> +        val |= s->num_spi;
> +        return val;
> +    case V2M_MSI_IIDR:
> +        return (PRODUCT_ID_QEMU << 20) | IMPLEMENTER_ARM;
I guess there is a single arch revision (0?), [19:16]
> +    default:
> +        if (offset > V2M_IIDR0 && offset <= 0xFFC) {
/* Peripheral ID2 reg */?
> +            return 0;
> +        }
> +
/* reserved */?
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "gicv2m_read: Bad offset %x\n", (int)offset);
> +        return 0;
> +    }
> +}
> +
> +static void gicv2m_write(void *opaque, hwaddr offset,
> +                        uint64_t value, unsigned size)
> +{
> +    GICv2mState *s = (GICv2mState *)opaque;
> +
> +    if (size != 4) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_write: bad size %u\n", size);
> +        return;
> +    }
> +
> +    switch (offset) {
> +    case V2M_MSI_SETSPI_NS: {
> +        int spi;
> +
> +        spi = (value & 0x3ff) - (s->base_spi + 32);
> +        if (spi < s->num_spi) {
> +            gicv2m_set_irq(s, spi);
> +        }
shouldn't we print an error msg in case spi is not within the frame range?
also shouldn't we check spi >= 0?

Best Regards
Eric
> +        return;
> +    }
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "gicv2m_write: Bad offset %x\n", (int)offset);
> +        return;
> +    }
> +}
> +
> +static const MemoryRegionOps gicv2m_ops = {
> +    .read = gicv2m_read,
> +    .write = gicv2m_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void gicv2m_realize(DeviceState *dev, Error **errp)
> +{
> +    GICv2mState *s = GICV2M(dev);
> +    int i;
> +
> +    if (s->num_spi > GICV2M_NUM_SPI_MAX) {
> +        error_setg(errp,
> +                   "requested %u SPIs exceeds GICv2m frame maximum %d",
> +                   s->num_spi, GICV2M_NUM_SPI_MAX);
> +        return;
> +    }
> +
> +    if (s->base_spi + 32 > 1020 - s->num_spi) {
> +        error_setg(errp,
> +                   "requested base SPI %u+%u exceeds max. number 1020",
> +                   s->base_spi + 32, s->num_spi);
> +        return;
> +    }
> +
> +    for (i = 0; i < s->num_spi; i++) {
> +        sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->spi[i]);
> +    }
> +
> +    msi_supported = true;
> +}
> +
> +static void gicv2m_init(Object *obj)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    GICv2mState *s = GICV2M(obj);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &gicv2m_ops, s,
> +                          "gicv2m", 0x1000);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +}
> +
> +static Property gicv2m_properties[] = {
> +    DEFINE_PROP_UINT32("base-spi", GICv2mState, base_spi, 0),
> +    DEFINE_PROP_UINT32("num-spi", GICv2mState, num_spi, 64),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void gicv2m_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->props = gicv2m_properties;
> +    dc->realize = gicv2m_realize;
> +}
> +
> +static const TypeInfo gicv2m_info = {
> +    .name          = TYPE_GICV2M,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(GICv2mState),
> +    .instance_init = gicv2m_init,
> +    .class_init    = gicv2m_class_init,
> +};
> +
> +static void gicv2m_register_types(void)
> +{
> +    type_register_static(&gicv2m_info);
> +}
> +
> +type_init(gicv2m_register_types)
>

On Fri, Apr 10, 2015 at 11:16:57AM +0200, Eric Auger wrote:
> Hi Christoffer,
> On 04/08/2015 11:20 PM, Christoffer Dall wrote:
> > The ARM GICv2m widget is a little device that handle MSI interrupt
> > writes to a trigger register and ties them to a range of interrupt lines
> > wires to the GIC.  It has a few status/id registers and the interrupt wires,
> > and that's about it.
> > 
> > A board instantiates the device by setting the base SPI number and
> > number SPIs for the frame.  The base-spi parameter is indexed in the SPI
> > number space only, so base-spi == 0, means IRQ number 32.  When a device
> > (the PCI host controller) writes to the trigger register, the payload is
> > the GIC IRQ number, so we have to subtract 32 from that and then index
> > into our frame of SPIs.
> > 
> > When instantiating a GICv2m device, tell PCI that we have instantiated
> > something that can deal with MSIs.  We rely on the board actually wiring
> > up the GICv2m to the PCI host controller.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> >  hw/intc/Makefile.objs |   1 +
> >  hw/intc/arm_gicv2m.c  | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 181 insertions(+)
> >  create mode 100644 hw/intc/arm_gicv2m.c
> > 
> > diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> > index 843864a..6b63dfc 100644
> > --- a/hw/intc/Makefile.objs
> > +++ b/hw/intc/Makefile.objs
> > @@ -15,6 +15,7 @@ common-obj-$(CONFIG_OPENPIC) += openpic.o
> >  
> >  obj-$(CONFIG_APIC) += apic.o apic_common.o
> >  obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
> > +obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o
> we could put this above close to the other common-obj-$(CONFIG_ARM_GIC)
> objects?

I'm honestly not quite sure what the difference between common-obj-y and
obj-y is?

> >  obj-$(CONFIG_STELLARIS) += armv7m_nvic.o
> >  obj-$(CONFIG_EXYNOS4) += exynos4210_gic.o exynos4210_combiner.o
> >  obj-$(CONFIG_GRLIB) += grlib_irqmp.o
> > diff --git a/hw/intc/arm_gicv2m.c b/hw/intc/arm_gicv2m.c
> > new file mode 100644
> > index 0000000..a80a16d
> > --- /dev/null
> > +++ b/hw/intc/arm_gicv2m.c
> > @@ -0,0 +1,180 @@
> > +/*
> > + *  GICv2m extension for MSI/MSI-x support with a GICv2-based system
> > + *
> > + * Copyright (C) 2015 Linaro, All rights reserved.
> > + *
> > + * Author: Christoffer Dall <christoffer.dall@linaro.org>
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include "hw/sysbus.h"
> > +#include "hw/pci/msi.h"
> > +
> > +#define TYPE_GICV2M "gicv2m"
> > +#define GICV2M(obj) OBJECT_CHECK(GICv2mState, (obj), TYPE_GICV2M)
> > +
> > +#define GICV2M_NUM_SPI_MAX 128
> > +
> Maybe we could add a comment that the model supports a single non secure
> MSI register frame

Isn't that already part of the GICv2m specification and hence implied
for emulating a gicv2m?

> > +#define V2M_MSI_TYPER           0x008
> > +#define V2M_MSI_SETSPI_NS       0x040
> > +#define V2M_MSI_IIDR            0xFCC
> > +#define V2M_IIDR0               0xFD0
> > +
> > +#define PRODUCT_ID_QEMU         0x51 /* ASCII code Q */
> > +#define IMPLEMENTER_ARM         0x43b
> > +
> > +typedef struct GICv2mState {
> > +    SysBusDevice parent_obj;
> > +
> > +    MemoryRegion iomem;
> > +    qemu_irq spi[GICV2M_NUM_SPI_MAX];
> > +
> /* first absolute SPI index */, to avoid mixing with ID?

not sure this comment helps, I think reading the code is actually more
clear, unless you can think of an even more clear wording for the
comment?

> > +    uint32_t base_spi;
> > +    uint32_t num_spi;
> > +} GICv2mState;
> > +
> > +static void gicv2m_set_irq(void *opaque, int irq)
> > +{
> > +    GICv2mState *s = (GICv2mState *)opaque;
> > +
> > +    qemu_irq_pulse(s->spi[irq]);
> > +}
> > +
> > +static uint64_t gicv2m_read(void *opaque, hwaddr offset,
> > +                            unsigned size)
> > +{
> > +    GICv2mState *s = (GICv2mState *)opaque;
> > +    uint32_t val;
> > +
> > +    if (size != 4) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_read: bad size %u\n", size);
> > +        return 0;
> > +    }
> > +
> > +    switch (offset) {
> > +    case V2M_MSI_TYPER:
> > +        val = (s->base_spi + 32) << 16;
> > +        val |= s->num_spi;
> > +        return val;
> > +    case V2M_MSI_IIDR:
> > +        return (PRODUCT_ID_QEMU << 20) | IMPLEMENTER_ARM;
> I guess there is a single arch revision (0?), [19:16]

yes, see the spec.

> > +    default:
> > +        if (offset > V2M_IIDR0 && offset <= 0xFFC) {
> /* Peripheral ID2 reg */?
> > +            return 0;
> > +        }
> > +
> /* reserved */?
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "gicv2m_read: Bad offset %x\n", (int)offset);
> > +        return 0;
> > +    }
> > +}
> > +
> > +static void gicv2m_write(void *opaque, hwaddr offset,
> > +                        uint64_t value, unsigned size)
> > +{
> > +    GICv2mState *s = (GICv2mState *)opaque;
> > +
> > +    if (size != 4) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_write: bad size %u\n", size);
> > +        return;
> > +    }
> > +
> > +    switch (offset) {
> > +    case V2M_MSI_SETSPI_NS: {
> > +        int spi;
> > +
> > +        spi = (value & 0x3ff) - (s->base_spi + 32);
> > +        if (spi < s->num_spi) {
> > +            gicv2m_set_irq(s, spi);
> > +        }
> shouldn't we print an error msg in case spi is not within the frame range?
> also shouldn't we check spi >= 0?

no, we should just emulate the behavior of the device, which clearly
states: "If the resulting value does not identify an SPI that is
allocated to this frame then the write has no effect." - so no effect is
what I'm going for :)


Thanks for the review!

-Christoffer

On 04/10/2015 11:58 AM, Christoffer Dall wrote:
> On Fri, Apr 10, 2015 at 11:16:57AM +0200, Eric Auger wrote:
>> Hi Christoffer,
>> On 04/08/2015 11:20 PM, Christoffer Dall wrote:
>>> The ARM GICv2m widget is a little device that handle MSI interrupt
>>> writes to a trigger register and ties them to a range of interrupt lines
>>> wires to the GIC.  It has a few status/id registers and the interrupt wires,
>>> and that's about it.
>>>
>>> A board instantiates the device by setting the base SPI number and
>>> number SPIs for the frame.  The base-spi parameter is indexed in the SPI
>>> number space only, so base-spi == 0, means IRQ number 32.  When a device
>>> (the PCI host controller) writes to the trigger register, the payload is
>>> the GIC IRQ number, so we have to subtract 32 from that and then index
>>> into our frame of SPIs.
>>>
>>> When instantiating a GICv2m device, tell PCI that we have instantiated
>>> something that can deal with MSIs.  We rely on the board actually wiring
>>> up the GICv2m to the PCI host controller.
>>>
>>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>>> ---
>>>  hw/intc/Makefile.objs |   1 +
>>>  hw/intc/arm_gicv2m.c  | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 181 insertions(+)
>>>  create mode 100644 hw/intc/arm_gicv2m.c
>>>
>>> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
>>> index 843864a..6b63dfc 100644
>>> --- a/hw/intc/Makefile.objs
>>> +++ b/hw/intc/Makefile.objs
>>> @@ -15,6 +15,7 @@ common-obj-$(CONFIG_OPENPIC) += openpic.o
>>>  
>>>  obj-$(CONFIG_APIC) += apic.o apic_common.o
>>>  obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
>>> +obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o
>> we could put this above close to the other common-obj-$(CONFIG_ARM_GIC)
>> objects?
> 
> I'm honestly not quite sure what the difference between common-obj-y and
> obj-y is?
> 
>>>  obj-$(CONFIG_STELLARIS) += armv7m_nvic.o
>>>  obj-$(CONFIG_EXYNOS4) += exynos4210_gic.o exynos4210_combiner.o
>>>  obj-$(CONFIG_GRLIB) += grlib_irqmp.o
>>> diff --git a/hw/intc/arm_gicv2m.c b/hw/intc/arm_gicv2m.c
>>> new file mode 100644
>>> index 0000000..a80a16d
>>> --- /dev/null
>>> +++ b/hw/intc/arm_gicv2m.c
>>> @@ -0,0 +1,180 @@
>>> +/*
>>> + *  GICv2m extension for MSI/MSI-x support with a GICv2-based system
>>> + *
>>> + * Copyright (C) 2015 Linaro, All rights reserved.
>>> + *
>>> + * Author: Christoffer Dall <christoffer.dall@linaro.org>
>>> + *
>>> + * This library is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU Lesser General Public
>>> + * License as published by the Free Software Foundation; either
>>> + * version 2 of the License, or (at your option) any later version.
>>> + *
>>> + * This library is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> + * Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General Public
>>> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#include "hw/sysbus.h"
>>> +#include "hw/pci/msi.h"
>>> +
>>> +#define TYPE_GICV2M "gicv2m"
>>> +#define GICV2M(obj) OBJECT_CHECK(GICv2mState, (obj), TYPE_GICV2M)
>>> +
>>> +#define GICV2M_NUM_SPI_MAX 128
>>> +
>> Maybe we could add a comment that the model supports a single non secure
>> MSI register frame
> 
> Isn't that already part of the GICv2m specification and hence implied
> for emulating a gicv2m?
> 
>>> +#define V2M_MSI_TYPER           0x008
>>> +#define V2M_MSI_SETSPI_NS       0x040
>>> +#define V2M_MSI_IIDR            0xFCC
>>> +#define V2M_IIDR0               0xFD0
>>> +
>>> +#define PRODUCT_ID_QEMU         0x51 /* ASCII code Q */
>>> +#define IMPLEMENTER_ARM         0x43b
>>> +
>>> +typedef struct GICv2mState {
>>> +    SysBusDevice parent_obj;
>>> +
>>> +    MemoryRegion iomem;
>>> +    qemu_irq spi[GICV2M_NUM_SPI_MAX];
>>> +
>> /* first absolute SPI index */, to avoid mixing with ID?
> 
> not sure this comment helps, I think reading the code is actually more
> clear, unless you can think of an even more clear wording for the
> comment?
> 
>>> +    uint32_t base_spi;
>>> +    uint32_t num_spi;
>>> +} GICv2mState;
>>> +
>>> +static void gicv2m_set_irq(void *opaque, int irq)
>>> +{
>>> +    GICv2mState *s = (GICv2mState *)opaque;
>>> +
>>> +    qemu_irq_pulse(s->spi[irq]);
>>> +}
>>> +
>>> +static uint64_t gicv2m_read(void *opaque, hwaddr offset,
>>> +                            unsigned size)
>>> +{
>>> +    GICv2mState *s = (GICv2mState *)opaque;
>>> +    uint32_t val;
>>> +
>>> +    if (size != 4) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_read: bad size %u\n", size);
>>> +        return 0;
>>> +    }
>>> +
>>> +    switch (offset) {
>>> +    case V2M_MSI_TYPER:
>>> +        val = (s->base_spi + 32) << 16;
>>> +        val |= s->num_spi;
>>> +        return val;
>>> +    case V2M_MSI_IIDR:
>>> +        return (PRODUCT_ID_QEMU << 20) | IMPLEMENTER_ARM;
>> I guess there is a single arch revision (0?), [19:16]
> 
> yes, see the spec.
> 
>>> +    default:
>>> +        if (offset > V2M_IIDR0 && offset <= 0xFFC) {
>> /* Peripheral ID2 reg */?
>>> +            return 0;
>>> +        }
>>> +
>> /* reserved */?
>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>> +                      "gicv2m_read: Bad offset %x\n", (int)offset);
>>> +        return 0;
>>> +    }
>>> +}
>>> +
>>> +static void gicv2m_write(void *opaque, hwaddr offset,
>>> +                        uint64_t value, unsigned size)
>>> +{
>>> +    GICv2mState *s = (GICv2mState *)opaque;
>>> +
>>> +    if (size != 4) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_write: bad size %u\n", size);
>>> +        return;
>>> +    }
>>> +
>>> +    switch (offset) {
>>> +    case V2M_MSI_SETSPI_NS: {
>>> +        int spi;
>>> +
>>> +        spi = (value & 0x3ff) - (s->base_spi + 32);
>>> +        if (spi < s->num_spi) {
>>> +            gicv2m_set_irq(s, spi);
>>> +        }
>> shouldn't we print an error msg in case spi is not within the frame range?
>> also shouldn't we check spi >= 0?
> 
> no, we should just emulate the behavior of the device, which clearly
> states: "If the resulting value does not identify an SPI that is
> allocated to this frame then the write has no effect." - so no effect is
> what I'm going for :)
OK,

and about spi>= 0?

Eric
> 
> 
> Thanks for the review!
> 
> -Christoffer
>

On Fri, Apr 10, 2015 at 12:34 PM, Eric Auger <eric.auger@linaro.org> wrote:
> On 04/10/2015 11:58 AM, Christoffer Dall wrote:
>> On Fri, Apr 10, 2015 at 11:16:57AM +0200, Eric Auger wrote:
>>> Hi Christoffer,
>>> On 04/08/2015 11:20 PM, Christoffer Dall wrote:
>>>> The ARM GICv2m widget is a little device that handle MSI interrupt
>>>> writes to a trigger register and ties them to a range of interrupt lines
>>>> wires to the GIC.  It has a few status/id registers and the interrupt wires,
>>>> and that's about it.
>>>>
>>>> A board instantiates the device by setting the base SPI number and
>>>> number SPIs for the frame.  The base-spi parameter is indexed in the SPI
>>>> number space only, so base-spi == 0, means IRQ number 32.  When a device
>>>> (the PCI host controller) writes to the trigger register, the payload is
>>>> the GIC IRQ number, so we have to subtract 32 from that and then index
>>>> into our frame of SPIs.
>>>>
>>>> When instantiating a GICv2m device, tell PCI that we have instantiated
>>>> something that can deal with MSIs.  We rely on the board actually wiring
>>>> up the GICv2m to the PCI host controller.
>>>>
>>>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>>>> ---
>>>>  hw/intc/Makefile.objs |   1 +
>>>>  hw/intc/arm_gicv2m.c  | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 181 insertions(+)
>>>>  create mode 100644 hw/intc/arm_gicv2m.c
>>>>
>>>> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
>>>> index 843864a..6b63dfc 100644
>>>> --- a/hw/intc/Makefile.objs
>>>> +++ b/hw/intc/Makefile.objs
>>>> @@ -15,6 +15,7 @@ common-obj-$(CONFIG_OPENPIC) += openpic.o
>>>>
>>>>  obj-$(CONFIG_APIC) += apic.o apic_common.o
>>>>  obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
>>>> +obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o
>>> we could put this above close to the other common-obj-$(CONFIG_ARM_GIC)
>>> objects?
>>
>> I'm honestly not quite sure what the difference between common-obj-y and
>> obj-y is?
>>
>>>>  obj-$(CONFIG_STELLARIS) += armv7m_nvic.o
>>>>  obj-$(CONFIG_EXYNOS4) += exynos4210_gic.o exynos4210_combiner.o
>>>>  obj-$(CONFIG_GRLIB) += grlib_irqmp.o
>>>> diff --git a/hw/intc/arm_gicv2m.c b/hw/intc/arm_gicv2m.c
>>>> new file mode 100644
>>>> index 0000000..a80a16d
>>>> --- /dev/null
>>>> +++ b/hw/intc/arm_gicv2m.c
>>>> @@ -0,0 +1,180 @@
>>>> +/*
>>>> + *  GICv2m extension for MSI/MSI-x support with a GICv2-based system
>>>> + *
>>>> + * Copyright (C) 2015 Linaro, All rights reserved.
>>>> + *
>>>> + * Author: Christoffer Dall <christoffer.dall@linaro.org>
>>>> + *
>>>> + * This library is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU Lesser General Public
>>>> + * License as published by the Free Software Foundation; either
>>>> + * version 2 of the License, or (at your option) any later version.
>>>> + *
>>>> + * This library is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>>> + * Lesser General Public License for more details.
>>>> + *
>>>> + * You should have received a copy of the GNU Lesser General Public
>>>> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>>>> + */
>>>> +
>>>> +#include "hw/sysbus.h"
>>>> +#include "hw/pci/msi.h"
>>>> +
>>>> +#define TYPE_GICV2M "gicv2m"
>>>> +#define GICV2M(obj) OBJECT_CHECK(GICv2mState, (obj), TYPE_GICV2M)
>>>> +
>>>> +#define GICV2M_NUM_SPI_MAX 128
>>>> +
>>> Maybe we could add a comment that the model supports a single non secure
>>> MSI register frame
>>
>> Isn't that already part of the GICv2m specification and hence implied
>> for emulating a gicv2m?
>>
>>>> +#define V2M_MSI_TYPER           0x008
>>>> +#define V2M_MSI_SETSPI_NS       0x040
>>>> +#define V2M_MSI_IIDR            0xFCC
>>>> +#define V2M_IIDR0               0xFD0
>>>> +
>>>> +#define PRODUCT_ID_QEMU         0x51 /* ASCII code Q */
>>>> +#define IMPLEMENTER_ARM         0x43b
>>>> +
>>>> +typedef struct GICv2mState {
>>>> +    SysBusDevice parent_obj;
>>>> +
>>>> +    MemoryRegion iomem;
>>>> +    qemu_irq spi[GICV2M_NUM_SPI_MAX];
>>>> +
>>> /* first absolute SPI index */, to avoid mixing with ID?
>>
>> not sure this comment helps, I think reading the code is actually more
>> clear, unless you can think of an even more clear wording for the
>> comment?
>>
>>>> +    uint32_t base_spi;
>>>> +    uint32_t num_spi;
>>>> +} GICv2mState;
>>>> +
>>>> +static void gicv2m_set_irq(void *opaque, int irq)
>>>> +{
>>>> +    GICv2mState *s = (GICv2mState *)opaque;
>>>> +
>>>> +    qemu_irq_pulse(s->spi[irq]);
>>>> +}
>>>> +
>>>> +static uint64_t gicv2m_read(void *opaque, hwaddr offset,
>>>> +                            unsigned size)
>>>> +{
>>>> +    GICv2mState *s = (GICv2mState *)opaque;
>>>> +    uint32_t val;
>>>> +
>>>> +    if (size != 4) {
>>>> +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_read: bad size %u\n", size);
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    switch (offset) {
>>>> +    case V2M_MSI_TYPER:
>>>> +        val = (s->base_spi + 32) << 16;
>>>> +        val |= s->num_spi;
>>>> +        return val;
>>>> +    case V2M_MSI_IIDR:
>>>> +        return (PRODUCT_ID_QEMU << 20) | IMPLEMENTER_ARM;
>>> I guess there is a single arch revision (0?), [19:16]
>>
>> yes, see the spec.
>>
>>>> +    default:
>>>> +        if (offset > V2M_IIDR0 && offset <= 0xFFC) {
>>> /* Peripheral ID2 reg */?
>>>> +            return 0;
>>>> +        }
>>>> +
>>> /* reserved */?
>>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>>> +                      "gicv2m_read: Bad offset %x\n", (int)offset);
>>>> +        return 0;
>>>> +    }
>>>> +}
>>>> +
>>>> +static void gicv2m_write(void *opaque, hwaddr offset,
>>>> +                        uint64_t value, unsigned size)
>>>> +{
>>>> +    GICv2mState *s = (GICv2mState *)opaque;
>>>> +
>>>> +    if (size != 4) {
>>>> +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_write: bad size %u\n", size);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    switch (offset) {
>>>> +    case V2M_MSI_SETSPI_NS: {
>>>> +        int spi;
>>>> +
>>>> +        spi = (value & 0x3ff) - (s->base_spi + 32);
>>>> +        if (spi < s->num_spi) {
>>>> +            gicv2m_set_irq(s, spi);
>>>> +        }
>>> shouldn't we print an error msg in case spi is not within the frame range?
>>> also shouldn't we check spi >= 0?
>>
>> no, we should just emulate the behavior of the device, which clearly
>> states: "If the resulting value does not identify an SPI that is
>> allocated to this frame then the write has no effect." - so no effect is
>> what I'm going for :)
> OK,
>
> and about spi>= 0?
>
oh, right, good point, I need to check the lower bound too.  Will address in v2.

Waiting for Peter's review and I will incorporate your fixes.

-Christoffer

On 10 April 2015 at 10:58, Christoffer Dall <christoffer.dall@linaro.org>
wrote:
> On Fri, Apr 10, 2015 at 11:16:57AM +0200, Eric Auger wrote:
>> Hi Christoffer,
>> On 04/08/2015 11:20 PM, Christoffer Dall wrote:
>> > The ARM GICv2m widget is a little device that handle MSI interrupt
>> > writes to a trigger register and ties them to a range of interrupt
lines
>> > wires to the GIC.  It has a few status/id registers and the interrupt
wires,
>> > and that's about it.
>> >
>> > A board instantiates the device by setting the base SPI number and
>> > number SPIs for the frame.  The base-spi parameter is indexed in the
SPI
>> > number space only, so base-spi == 0, means IRQ number 32.  When a
device
>> > (the PCI host controller) writes to the trigger register, the payload
is
>> > the GIC IRQ number, so we have to subtract 32 from that and then index
>> > into our frame of SPIs.
>> >
>> > When instantiating a GICv2m device, tell PCI that we have instantiated
>> > something that can deal with MSIs.  We rely on the board actually
wiring
>> > up the GICv2m to the PCI host controller.
>> >
>> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>> > ---
>> >  hw/intc/Makefile.objs |   1 +
>> >  hw/intc/arm_gicv2m.c  | 180
++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 181 insertions(+)
>> >  create mode 100644 hw/intc/arm_gicv2m.c
>> >
>> > diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
>> > index 843864a..6b63dfc 100644
>> > --- a/hw/intc/Makefile.objs
>> > +++ b/hw/intc/Makefile.objs
>> > @@ -15,6 +15,7 @@ common-obj-$(CONFIG_OPENPIC) += openpic.o
>> >
>> >  obj-$(CONFIG_APIC) += apic.o apic_common.o
>> >  obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
>> > +obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o
>> we could put this above close to the other common-obj-$(CONFIG_ARM_GIC)
>> objects?
>
> I'm honestly not quite sure what the difference between common-obj-y and
> obj-y is?

common-obj-y is for object files which can be built once and
then linked into any qemu-*-softmmu binary, ie they have no
dependencies on details of the target CPU. obj-y is for
objects which get built for every target CPU they're going
to be used for (in this case that would mean separate .o files
for arm-softmmu and aarch64-softmmu).

common-obj-y is preferred unless there's something that
absolutely requires the code to be built per-cpu. In this
case there probably isn't anything like that.

-- PMM

On 8 April 2015 at 22:20, Christoffer Dall <christoffer.dall@linaro.org>
wrote:

> The ARM GICv2m widget is a little device that handle MSI interrupt
>

"handles"


> writes to a trigger register and ties them to a range of interrupt lines
> wires to the GIC.  It has a few status/id registers and the interrupt
> wires,
> and that's about it.
>
> A board instantiates the device by setting the base SPI number and
> number SPIs for the frame.  The base-spi parameter is indexed in the SPI
> number space only, so base-spi == 0, means IRQ number 32.  When a device
> (the PCI host controller) writes to the trigger register, the payload is
> the GIC IRQ number, so we have to subtract 32 from that and then index
> into our frame of SPIs.
>
> When instantiating a GICv2m device, tell PCI that we have instantiated
> something that can deal with MSIs.  We rely on the board actually wiring
> up the GICv2m to the PCI host controller.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  hw/intc/Makefile.objs |   1 +
>  hw/intc/arm_gicv2m.c  | 180
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 181 insertions(+)
>  create mode 100644 hw/intc/arm_gicv2m.c
>
> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> index 843864a..6b63dfc 100644
> --- a/hw/intc/Makefile.objs
> +++ b/hw/intc/Makefile.objs
> @@ -15,6 +15,7 @@ common-obj-$(CONFIG_OPENPIC) += openpic.o
>
>  obj-$(CONFIG_APIC) += apic.o apic_common.o
>  obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
> +obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o
>

Should be common-obj-, as per other email.


>  obj-$(CONFIG_STELLARIS) += armv7m_nvic.o
>  obj-$(CONFIG_EXYNOS4) += exynos4210_gic.o exynos4210_combiner.o
>  obj-$(CONFIG_GRLIB) += grlib_irqmp.o
> diff --git a/hw/intc/arm_gicv2m.c b/hw/intc/arm_gicv2m.c
> new file mode 100644
> index 0000000..a80a16d
> --- /dev/null
> +++ b/hw/intc/arm_gicv2m.c
> @@ -0,0 +1,180 @@
> +/*
> + *  GICv2m extension for MSI/MSI-x support with a GICv2-based system
> + *
> + * Copyright (C) 2015 Linaro, All rights reserved.
> + *
> + * Author: Christoffer Dall <christoffer.dall@linaro.org>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <
> http://www.gnu.org/licenses/>.
> + */
>

A reference to the document defining the GICv2M would be nice here
(it's the SBSA unless I'm confused).



> +
> +#include "hw/sysbus.h"
> +#include "hw/pci/msi.h"
> +
> +#define TYPE_GICV2M "gicv2m"
>

I think this should be
#define TYPE_ARM_GICV2M "arm-gicv2m"


> +#define GICV2M(obj) OBJECT_CHECK(GICv2mState, (obj), TYPE_GICV2M)
>

and  ARM_GICV2M(obj)


> +
> +#define GICV2M_NUM_SPI_MAX 128
> +
> +#define V2M_MSI_TYPER           0x008
> +#define V2M_MSI_SETSPI_NS       0x040
> +#define V2M_MSI_IIDR            0xFCC
> +#define V2M_IIDR0               0xFD0
> +
> +#define PRODUCT_ID_QEMU         0x51 /* ASCII code Q */
> +#define IMPLEMENTER_ARM         0x43b
> +
> +typedef struct GICv2mState {
>

ARMGICv2mState

+    SysBusDevice parent_obj;
> +
> +    MemoryRegion iomem;
> +    qemu_irq spi[GICV2M_NUM_SPI_MAX];
> +
> +    uint32_t base_spi;
> +    uint32_t num_spi;
> +} GICv2mState;
> +
> +static void gicv2m_set_irq(void *opaque, int irq)
> +{
> +    GICv2mState *s = (GICv2mState *)opaque;
> +
> +    qemu_irq_pulse(s->spi[irq]);
> +}
> +
> +static uint64_t gicv2m_read(void *opaque, hwaddr offset,
> +                            unsigned size)
> +{
> +    GICv2mState *s = (GICv2mState *)opaque;
> +    uint32_t val;
> +
> +    if (size != 4) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_read: bad size %u\n",
> size);
> +        return 0;
> +    }
> +
> +    switch (offset) {
> +    case V2M_MSI_TYPER:
> +        val = (s->base_spi + 32) << 16;
> +        val |= s->num_spi;
> +        return val;
> +    case V2M_MSI_IIDR:
> +        return (PRODUCT_ID_QEMU << 20) | IMPLEMENTER_ARM;
>

This choice of implementer field value is a little hard to justify :-)


> +    default:
> +        if (offset > V2M_IIDR0 && offset <= 0xFFC) {
>

Why not just have a "case 0xFD0..0xFFC" ?
Could do with a comment about what the ID regs are
(ie that they're mostly impdef and we choose to read as zero).

+            return 0;
> +        }
> +
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "gicv2m_read: Bad offset %x\n", (int)offset);
> +        return 0;
> +    }
> +}
> +
> +static void gicv2m_write(void *opaque, hwaddr offset,
> +                        uint64_t value, unsigned size)
> +{
> +    GICv2mState *s = (GICv2mState *)opaque;
> +
> +    if (size != 4) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_write: bad size %u\n",
> size);
> +        return;
> +    }
>

"The MSI_SETSPI_S and MSI_SETSPI_NS registers must also support
16 bit writes to bits [15:0]", so you can't insist on 32 bit
accesses only here.


> +
> +    switch (offset) {
> +    case V2M_MSI_SETSPI_NS: {
> +        int spi;
> +
> +        spi = (value & 0x3ff) - (s->base_spi + 32);
> +        if (spi < s->num_spi) {
> +            gicv2m_set_irq(s, spi);
> +        }
> +        return;
> +    }
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "gicv2m_write: Bad offset %x\n", (int)offset);
> +        return;
> +    }
> +}
> +
> +static const MemoryRegionOps gicv2m_ops = {
> +    .read = gicv2m_read,
> +    .write = gicv2m_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void gicv2m_realize(DeviceState *dev, Error **errp)
> +{
> +    GICv2mState *s = GICV2M(dev);
> +    int i;
> +
> +    if (s->num_spi > GICV2M_NUM_SPI_MAX) {
> +        error_setg(errp,
> +                   "requested %u SPIs exceeds GICv2m frame maximum %d",
> +                   s->num_spi, GICV2M_NUM_SPI_MAX);
> +        return;
> +    }
> +
> +    if (s->base_spi + 32 > 1020 - s->num_spi) {
> +        error_setg(errp,
> +                   "requested base SPI %u+%u exceeds max. number 1020",
> +                   s->base_spi + 32, s->num_spi);
> +        return;
> +    }
> +
> +    for (i = 0; i < s->num_spi; i++) {
> +        sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->spi[i]);
> +    }
> +
> +    msi_supported = true;
> +}
> +
> +static void gicv2m_init(Object *obj)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    GICv2mState *s = GICV2M(obj);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &gicv2m_ops, s,
> +                          "gicv2m", 0x1000);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +}
> +
> +static Property gicv2m_properties[] = {
> +    DEFINE_PROP_UINT32("base-spi", GICv2mState, base_spi, 0),
> +    DEFINE_PROP_UINT32("num-spi", GICv2mState, num_spi, 64),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void gicv2m_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->props = gicv2m_properties;
> +    dc->realize = gicv2m_realize;
> +}
> +
> +static const TypeInfo gicv2m_info = {
> +    .name          = TYPE_GICV2M,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(GICv2mState),
> +    .instance_init = gicv2m_init,
> +    .class_init    = gicv2m_class_init,
> +};
> +
> +static void gicv2m_register_types(void)
> +{
> +    type_register_static(&gicv2m_info);
> +}
> +
> +type_init(gicv2m_register_types)
> --
> 2.1.2.330.g565301e.dirty


Are we ever going to want to support the security extensions for
gicv2m? If we do can we backwards-compatibly add it later?
(I think the answer to that is probably 'yes' but I haven't
thought through the details...)

thanks
-- PMM

On Tue, Apr 21, 2015 at 03:40:51PM +0100, Peter Maydell wrote:
> On 8 April 2015 at 22:20, Christoffer Dall <christoffer.dall@linaro.org>
> wrote:
> 
> > The ARM GICv2m widget is a little device that handle MSI interrupt
> >
> 
> "handles"
> 
> 
> > writes to a trigger register and ties them to a range of interrupt lines
> > wires to the GIC.  It has a few status/id registers and the interrupt
> > wires,
> > and that's about it.
> >
> > A board instantiates the device by setting the base SPI number and
> > number SPIs for the frame.  The base-spi parameter is indexed in the SPI
> > number space only, so base-spi == 0, means IRQ number 32.  When a device
> > (the PCI host controller) writes to the trigger register, the payload is
> > the GIC IRQ number, so we have to subtract 32 from that and then index
> > into our frame of SPIs.
> >
> > When instantiating a GICv2m device, tell PCI that we have instantiated
> > something that can deal with MSIs.  We rely on the board actually wiring
> > up the GICv2m to the PCI host controller.
> >
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> >  hw/intc/Makefile.objs |   1 +
> >  hw/intc/arm_gicv2m.c  | 180
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 181 insertions(+)
> >  create mode 100644 hw/intc/arm_gicv2m.c
> >
> > diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> > index 843864a..6b63dfc 100644
> > --- a/hw/intc/Makefile.objs
> > +++ b/hw/intc/Makefile.objs
> > @@ -15,6 +15,7 @@ common-obj-$(CONFIG_OPENPIC) += openpic.o
> >
> >  obj-$(CONFIG_APIC) += apic.o apic_common.o
> >  obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
> > +obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o
> >
> 
> Should be common-obj-, as per other email.
> 
> 
> >  obj-$(CONFIG_STELLARIS) += armv7m_nvic.o
> >  obj-$(CONFIG_EXYNOS4) += exynos4210_gic.o exynos4210_combiner.o
> >  obj-$(CONFIG_GRLIB) += grlib_irqmp.o
> > diff --git a/hw/intc/arm_gicv2m.c b/hw/intc/arm_gicv2m.c
> > new file mode 100644
> > index 0000000..a80a16d
> > --- /dev/null
> > +++ b/hw/intc/arm_gicv2m.c
> > @@ -0,0 +1,180 @@
> > +/*
> > + *  GICv2m extension for MSI/MSI-x support with a GICv2-based system
> > + *
> > + * Copyright (C) 2015 Linaro, All rights reserved.
> > + *
> > + * Author: Christoffer Dall <christoffer.dall@linaro.org>
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, see <
> > http://www.gnu.org/licenses/>.
> > + */
> >
> 
> A reference to the document defining the GICv2M would be nice here
> (it's the SBSA unless I'm confused).
> 
> 
> 
> > +
> > +#include "hw/sysbus.h"
> > +#include "hw/pci/msi.h"
> > +
> > +#define TYPE_GICV2M "gicv2m"
> >
> 
> I think this should be
> #define TYPE_ARM_GICV2M "arm-gicv2m"
> 
> 
> > +#define GICV2M(obj) OBJECT_CHECK(GICv2mState, (obj), TYPE_GICV2M)
> >
> 
> and  ARM_GICV2M(obj)
> 
> 
> > +
> > +#define GICV2M_NUM_SPI_MAX 128
> > +
> > +#define V2M_MSI_TYPER           0x008
> > +#define V2M_MSI_SETSPI_NS       0x040
> > +#define V2M_MSI_IIDR            0xFCC
> > +#define V2M_IIDR0               0xFD0
> > +
> > +#define PRODUCT_ID_QEMU         0x51 /* ASCII code Q */
> > +#define IMPLEMENTER_ARM         0x43b
> > +
> > +typedef struct GICv2mState {
> >
> 
> ARMGICv2mState
> 
> +    SysBusDevice parent_obj;
> > +
> > +    MemoryRegion iomem;
> > +    qemu_irq spi[GICV2M_NUM_SPI_MAX];
> > +
> > +    uint32_t base_spi;
> > +    uint32_t num_spi;
> > +} GICv2mState;
> > +
> > +static void gicv2m_set_irq(void *opaque, int irq)
> > +{
> > +    GICv2mState *s = (GICv2mState *)opaque;
> > +
> > +    qemu_irq_pulse(s->spi[irq]);
> > +}
> > +
> > +static uint64_t gicv2m_read(void *opaque, hwaddr offset,
> > +                            unsigned size)
> > +{
> > +    GICv2mState *s = (GICv2mState *)opaque;
> > +    uint32_t val;
> > +
> > +    if (size != 4) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_read: bad size %u\n",
> > size);
> > +        return 0;
> > +    }
> > +
> > +    switch (offset) {
> > +    case V2M_MSI_TYPER:
> > +        val = (s->base_spi + 32) << 16;
> > +        val |= s->num_spi;
> > +        return val;
> > +    case V2M_MSI_IIDR:
> > +        return (PRODUCT_ID_QEMU << 20) | IMPLEMENTER_ARM;
> >
> 
> This choice of implementer field value is a little hard to justify :-)
> 
> 
> > +    default:
> > +        if (offset > V2M_IIDR0 && offset <= 0xFFC) {
> >
> 
> Why not just have a "case 0xFD0..0xFFC" ?
> Could do with a comment about what the ID regs are
> (ie that they're mostly impdef and we choose to read as zero).
> 
> +            return 0;
> > +        }
> > +
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "gicv2m_read: Bad offset %x\n", (int)offset);
> > +        return 0;
> > +    }
> > +}
> > +
> > +static void gicv2m_write(void *opaque, hwaddr offset,
> > +                        uint64_t value, unsigned size)
> > +{
> > +    GICv2mState *s = (GICv2mState *)opaque;
> > +
> > +    if (size != 4) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "gicv2m_write: bad size %u\n",
> > size);
> > +        return;
> > +    }
> >
> 
> "The MSI_SETSPI_S and MSI_SETSPI_NS registers must also support
> 16 bit writes to bits [15:0]", so you can't insist on 32 bit
> accesses only here.
> 
> 
> > +
> > +    switch (offset) {
> > +    case V2M_MSI_SETSPI_NS: {
> > +        int spi;
> > +
> > +        spi = (value & 0x3ff) - (s->base_spi + 32);
> > +        if (spi < s->num_spi) {
> > +            gicv2m_set_irq(s, spi);
> > +        }
> > +        return;
> > +    }
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "gicv2m_write: Bad offset %x\n", (int)offset);
> > +        return;
> > +    }
> > +}
> > +
> > +static const MemoryRegionOps gicv2m_ops = {
> > +    .read = gicv2m_read,
> > +    .write = gicv2m_write,
> > +    .endianness = DEVICE_LITTLE_ENDIAN,
> > +};
> > +
> > +static void gicv2m_realize(DeviceState *dev, Error **errp)
> > +{
> > +    GICv2mState *s = GICV2M(dev);
> > +    int i;
> > +
> > +    if (s->num_spi > GICV2M_NUM_SPI_MAX) {
> > +        error_setg(errp,
> > +                   "requested %u SPIs exceeds GICv2m frame maximum %d",
> > +                   s->num_spi, GICV2M_NUM_SPI_MAX);
> > +        return;
> > +    }
> > +
> > +    if (s->base_spi + 32 > 1020 - s->num_spi) {
> > +        error_setg(errp,
> > +                   "requested base SPI %u+%u exceeds max. number 1020",
> > +                   s->base_spi + 32, s->num_spi);
> > +        return;
> > +    }
> > +
> > +    for (i = 0; i < s->num_spi; i++) {
> > +        sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->spi[i]);
> > +    }
> > +
> > +    msi_supported = true;
> > +}
> > +
> > +static void gicv2m_init(Object *obj)
> > +{
> > +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> > +    GICv2mState *s = GICV2M(obj);
> > +
> > +    memory_region_init_io(&s->iomem, OBJECT(s), &gicv2m_ops, s,
> > +                          "gicv2m", 0x1000);
> > +    sysbus_init_mmio(sbd, &s->iomem);
> > +}
> > +
> > +static Property gicv2m_properties[] = {
> > +    DEFINE_PROP_UINT32("base-spi", GICv2mState, base_spi, 0),
> > +    DEFINE_PROP_UINT32("num-spi", GICv2mState, num_spi, 64),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void gicv2m_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > +    dc->props = gicv2m_properties;
> > +    dc->realize = gicv2m_realize;
> > +}
> > +
> > +static const TypeInfo gicv2m_info = {
> > +    .name          = TYPE_GICV2M,
> > +    .parent        = TYPE_SYS_BUS_DEVICE,
> > +    .instance_size = sizeof(GICv2mState),
> > +    .instance_init = gicv2m_init,
> > +    .class_init    = gicv2m_class_init,
> > +};
> > +
> > +static void gicv2m_register_types(void)
> > +{
> > +    type_register_static(&gicv2m_info);
> > +}
> > +
> > +type_init(gicv2m_register_types)
> > --
> > 2.1.2.330.g565301e.dirty
> 
> 
> Are we ever going to want to support the security extensions for
> gicv2m? If we do can we backwards-compatibly add it later?
> (I think the answer to that is probably 'yes' but I haven't
> thought through the details...)
> 

Thanks for the review!

I've tried to address all your comments.

Regarding adding support for the security extensions later, I assume the
QEMU-specifics will be to add a flag to the device instantiation from
the containing board activating security support, which would grow the
IO region size of this device from 4K to 8K for an adjacent secure
register frame and adjust the offsets.  I cannot think of a reason why
that wouldn't work backwards-compatibly?

I've added a comment declaring the scope of the emulation (single
non-secure MSI register frame as Eric also suggested for the next
iteration).

-Christoffer

On 27 April 2015 at 14:41, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> Regarding adding support for the security extensions later, I assume the
> QEMU-specifics will be to add a flag to the device instantiation from
> the containing board activating security support, which would grow the
> IO region size of this device from 4K to 8K for an adjacent secure
> register frame and adjust the offsets.  I cannot think of a reason why
> that wouldn't work backwards-compatibly?

I think you'd probably want to add a second MMIO region rather
than making the first one double-size. I don't think there's anything
in the spec that mandates them being adjacent. You also need to allocate
more interrupt lines.

I think this should all be backwards-compatible, yes.

thanks
-- PMM

On Mon, Apr 27, 2015 at 02:43:17PM +0100, Peter Maydell wrote:
> On 27 April 2015 at 14:41, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> > Regarding adding support for the security extensions later, I assume the
> > QEMU-specifics will be to add a flag to the device instantiation from
> > the containing board activating security support, which would grow the
> > IO region size of this device from 4K to 8K for an adjacent secure
> > register frame and adjust the offsets.  I cannot think of a reason why
> > that wouldn't work backwards-compatibly?
> 
> I think you'd probably want to add a second MMIO region rather
> than making the first one double-size. I don't think there's anything
> in the spec that mandates them being adjacent. You also need to allocate
> more interrupt lines.

Right; the spec actually says they are NOT required to be adjacent.

> 
> I think this should all be backwards-compatible, yes.
> 

ok - thanks,
-Christoffer