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

[Christoffer Dall]

On Fri, Apr 10, 2015 at 12:34 PM, Eric Auger <address@hidden> 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 <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
>>> 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 <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/>.
>>>> + */
>>>> +
>>>> +#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