From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48230) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b1Exs-0008Cj-QL for qemu-devel@nongnu.org; Fri, 13 May 2016 11:25:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b1Exr-0007G6-7e for qemu-devel@nongnu.org; Fri, 13 May 2016 11:25:20 -0400 Received: from mail-vk0-x231.google.com ([2607:f8b0:400c:c05::231]:35686) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b1Exr-0007Fm-3T for qemu-devel@nongnu.org; Fri, 13 May 2016 11:25:19 -0400 Received: by mail-vk0-x231.google.com with SMTP id f66so141369571vkh.2 for ; Fri, 13 May 2016 08:25:17 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <5735ED3B.8010904@linaro.org> References: <1462814989-24360-1-git-send-email-peter.maydell@linaro.org> <1462814989-24360-12-git-send-email-peter.maydell@linaro.org> <5735ED3B.8010904@linaro.org> From: Peter Maydell Date: Fri, 13 May 2016 16:24:57 +0100 Message-ID: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 11/23] hw/intc/arm_gicv3: Implement GICv3 distributor registers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Shannon Zhao Cc: qemu-arm , QEMU Developers , Patch Tracking , Pavel Fedin , Shlomo Pongratz , Shlomo Pongratz , Christoffer Dall On 13 May 2016 at 16:05, Shannon Zhao wrote: > On 2016=E5=B9=B405=E6=9C=8810=E6=97=A5 01:29, Peter Maydell wrote: >> +static MemTxResult gicd_writeb(GICv3State *s, hwaddr offset, >> + uint64_t value, MemTxAttrs attrs) >> +{ >> + /* Most GICv3 distributor registers do not support byte accesses. *= / >> + switch (offset) { >> + case GICD_CPENDSGIR ... GICD_CPENDSGIR + 0xf: >> + case GICD_SPENDSGIR ... GICD_SPENDSGIR + 0xf: >> + case GICD_ITARGETSR ... GICD_ITARGETSR + 0x3ff: >> + /* This GIC implementation always has affinity routing enabled, >> + * so these registers are all RAZ/WI. >> + */ >> + return MEMTX_OK; >> + case GICD_IPRIORITYR ... GICD_IPRIORITYR + 0x3ff: >> + { >> + int irq =3D offset - GICD_IPRIORITYR; >> + >> + gicd_write_ipriorityr(s, attrs, irq, value); >> + gicv3_update(s, irq, 1); > The GICv3 SPEC says: > " > When affinity routing is enabled for the security state of an interrupt: > =E2=80=A2 GICR_IPRIORITYR is used instead of GICD_IPRIORITYR where = n =3D 0 > to 7 (that > is, for SGIs and PPIs). > =E2=80=A2 GICD_IPRIORITYR is RAZ/WI where n =3D 0 to 7. > " > > So I think it shouldn't call gicv3_update if attrs.secure is true and > irq < 32. And it should check the parameter irq in gicv3_update(). If irq < 32 then gicd_write_ipriority() will return without doing anything. We'll unnecessarily call gicv3_update(), but that does no harm, and I don't think being slightly inefficient for an access a correctly functioning guest will never make is a big problem. >> + switch (offset) { >> + case GICD_CTLR: >> + if (!attrs.secure && !(s->gicd_ctlr & GICD_CTLR_DS)) { >> + /* The NS view of the GICD_CTLR sees only certain bits: >> + * + bit [31] (RWP) is an alias of the Secure bit [31] >> + * + bit [4] (ARE_NS) is an alias of Secure bit [5] >> + * + bit [1] (EnableGrp1A) is an alias of Secure bit [1] if >> + * NS affinity routing is enabled, otherwise RES0 >> + * + bit [0] (EnableGrp1) is an alias of Secure bit [1] if >> + * NS affinity routing is not enabled, otherwise RES0 >> + * Since for QEMU affinity routing is always enabled >> + * for both S and NS this means that bits [4] and [5] are >> + * both always 1, and we can simply make the NS view >> + * be bits 31, 4 and 1 of the S view. >> + */ >> + *data =3D s->gicd_ctlr & (GICD_CTLR_ARE_NS | > As you said, we make the NS view be bit 4 of the S view. So the > GICD_CTLR_ARE_NS should be GICD_CTLR_ARE_S, right? Yes, you're right, this should be GICD_CTLR_ARE_S. >> + GICD_CTLR_EN_GRP1NS | >> + GICD_CTLR_RWP); >> + } else { >> + *data =3D s->gicd_ctlr; >> + } >> + return MEMTX_OK; thanks -- PMM