From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller Date: Tue, 21 Oct 2014 15:26:14 +0100 Message-ID: <54466D06.5070000@linaro.org> References: <1412328051-20015-1-git-send-email-talex5@gmail.com> <1412328051-20015-3-git-send-email-talex5@gmail.com> <1413889218.23337.24.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XgaOJ-0008JO-B4 for xen-devel@lists.xenproject.org; Tue, 21 Oct 2014 14:26:27 +0000 Received: by mail-wi0-f180.google.com with SMTP id em10so2025592wid.1 for ; Tue, 21 Oct 2014 07:26:21 -0700 (PDT) In-Reply-To: <1413889218.23337.24.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell , Thomas Leonard Cc: samuel.thibault@ens-lyon.org, stefano.stabellini@eu.citrix.com, xen-devel@lists.xenproject.org, Dave.Scott@eu.citrix.com, anil@recoil.org List-Id: xen-devel@lists.xenproject.org Hi Ian and Thomas, On 10/21/2014 12:00 PM, Ian Campbell wrote: > On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote: >> +static void gic_set_priority(struct gic *gic, int irq_number, unsigned char priority) >> +{ >> + uint32_t value; >> + uint32_t *addr = REG(gicd(gic, GICD_IPRIORITYR)) + (irq_number >> 2); >> + value = REG_READ32(addr); >> + value &= ~(0xff << (8 * (irq_number & 0x3))); // clear old priority >> + value |= priority << (8 * (irq_number & 0x3)); // set new priority >> + REG_WRITE32(addr, value); > > I believe the IPRIORITYR registers are byte addressable, so you can just > do a strb of the new value without needing to read-modify-write. > >> +static void gic_route_interrupt(struct gic *gic, int irq_number, unsigned char cpu_set) >> +{ >> + uint32_t value; >> + uint32_t *addr = REG(gicd(gic, GICD_ITARGETSR)) + (irq_number >> 2); >> + value = REG_READ32(addr); >> + value &= ~(0xff << (8 * (irq_number & 0x3))); // clear old target >> + value |= cpu_set << (8 * (irq_number & 0x3)); // set new target >> + REG_WRITE32(addr, value); > > Same for ITARGETSR. Our implementation of ITARGETSR doesn't handle correctly read/write per byte. If the register is RO (such as for the SGIs and PPIs), our write ignore is checking that the guest is writing a word. Even though we need to fix it in Xen (I could send a patch for it). We need to keep this implementation if we want mini-os to run on Xen 4.4.x. >> +/* Note: not thread safe (but we only support one CPU for now anyway) */ >> +static void gic_enable_interrupt(struct gic *gic, int irq_number, >> + unsigned char cpu_set, unsigned char level_sensitive, unsigned char ppi) >> +{ >> + void *set_enable_reg; >> + void *cfg_reg; >> + >> + // set priority >> + gic_set_priority(gic, irq_number, 0x0); >> + >> + // set target cpus for this interrupt >> + gic_route_interrupt(gic, irq_number, cpu_set); >> + >> + // set level/edge triggered >> + cfg_reg = (void *)gicd(gic, GICD_ICFGR); >> + if (level_sensitive) { >> + clear_bit_non_atomic((irq_number * 2) + 1, cfg_reg); >> + } else { >> + set_bit_non_atomic((irq_number * 2) + 1, cfg_reg); >> + } >> + if (ppi) > > missing else? Or should be folded in above as level_sensitive||ppi I would even drop this check. I don't think, we need to have specific configuration for PPIs. [..] >> + gic_enable_interrupt(&gic, EVENTS_IRQ /* interrupt number */, 0x1 /*cpu_set*/, 1 /*level_sensitive*/, 0 /* ppi */); >> + gic_enable_interrupt(&gic, VIRTUALTIMER_IRQ /* interrupt number */, 0x1 /*cpu_set*/, 1 /*level_sensitive*/, 1 /* ppi */); > > BTW, ppi or not is implicit in the irq number. At the same time, both interrupt are PPIs, why the former as ppi = 0 and the latter 1? Regards, -- Julien Grall