On 8 April 2015 at 22:20, Christoffer Dall <christoffer.d...@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.d...@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.d...@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

Reply via email to