From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55392) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YgWL3-0005U3-Su for qemu-devel@nongnu.org; Fri, 10 Apr 2015 06:39:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YgWKz-0005Dl-Ij for qemu-devel@nongnu.org; Fri, 10 Apr 2015 06:39:05 -0400 Received: from mail-ie0-f172.google.com ([209.85.223.172]:35575) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YgWKz-0005Df-Db for qemu-devel@nongnu.org; Fri, 10 Apr 2015 06:39:01 -0400 Received: by iejt8 with SMTP id t8so13837653iej.2 for ; Fri, 10 Apr 2015 03:39:00 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <5527A748.7000400@linaro.org> References: <1428528060-17896-1-git-send-email-christoffer.dall@linaro.org> <1428528060-17896-3-git-send-email-christoffer.dall@linaro.org> <55279509.1040402@linaro.org> <20150410095818.GE6186@cbox> <5527A748.7000400@linaro.org> Date: Fri, 10 Apr 2015 12:39:00 +0200 Message-ID: From: Christoffer Dall Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH 2/3] arm_gicv2m: Add GICv2m widget to support MSIs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Auger Cc: QEMU Developers On Fri, Apr 10, 2015 at 12:34 PM, Eric Auger 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 >>>> --- >>>> 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 >>>> + * >>>> + * 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 . >>>> + */ >>>> + >>>> +#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