qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 2/3] arm_gicv2m: Add GICv2m widget to support MS


From: Christoffer Dall
Subject: Re: [Qemu-devel] [PATCH 2/3] arm_gicv2m: Add GICv2m widget to support MSIs
Date: Mon, 27 Apr 2015 06:41:17 -0700
User-agent: Mutt/1.5.21 (2010-09-15)

On Tue, Apr 21, 2015 at 03:40:51PM +0100, Peter Maydell wrote:
> On 8 April 2015 at 22:20, Christoffer Dall <address@hidden>
> 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 <address@hidden>
> > ---
> >  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 <address@hidden>
> > + *
> > + * 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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]