From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:44162)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <shannon.zhao@linaro.org>) id 1b1Eey-0005yL-27
	for qemu-devel@nongnu.org; Fri, 13 May 2016 11:05:49 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <shannon.zhao@linaro.org>) id 1b1Eer-0002NI-2V
	for qemu-devel@nongnu.org; Fri, 13 May 2016 11:05:46 -0400
Received: from mail-io0-x230.google.com ([2607:f8b0:4001:c06::230]:36066)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <shannon.zhao@linaro.org>) id 1b1Eeq-0002N5-Rg
	for qemu-devel@nongnu.org; Fri, 13 May 2016 11:05:40 -0400
Received: by mail-io0-x230.google.com with SMTP id i75so130803439ioa.3
	for <qemu-devel@nongnu.org>; Fri, 13 May 2016 08:05:39 -0700 (PDT)
References: <1462814989-24360-1-git-send-email-peter.maydell@linaro.org>
	<1462814989-24360-12-git-send-email-peter.maydell@linaro.org>
From: Shannon Zhao <shannon.zhao@linaro.org>
Message-ID: <5735ED3B.8010904@linaro.org>
Date: Fri, 13 May 2016 23:05:31 +0800
MIME-Version: 1.0
In-Reply-To: <1462814989-24360-12-git-send-email-peter.maydell@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Subject: Re: [Qemu-devel] [PATCH 11/23] hw/intc/arm_gicv3: Implement GICv3
 distributor registers
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel/>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Peter Maydell <peter.maydell@linaro.org>, qemu-arm@nongnu.org, qemu-devel@nongnu.org
Cc: patches@linaro.org, Pavel Fedin <p.fedin@samsung.com>, Shlomo Pongratz <shlomo.pongratz@huawei.com>, Shlomo Pongratz <shlomopongratz@gmail.com>, Christoffer Dall <christoffer.dall@linaro.org>

On 2016年05月10日 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 = 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:
• GICR_IPRIORITYR<n> is used instead of GICD_IPRIORITYR<n> where n = 0
to 7 (that
is, for SGIs and PPIs).
• GICD_IPRIORITYR<n> is RAZ/WI where n = 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().

> +        return MEMTX_OK;
> +    }
> +    default:
> +        return MEMTX_ERROR;
> +    }
> +}
> +
> +static MemTxResult gicd_readw(GICv3State *s, hwaddr offset,
> +                              uint64_t *data, MemTxAttrs attrs)
> +{
> +    /* Only GICD_SETSPI_NSR, GICD_CLRSPI_NSR, GICD_SETSPI_SR and GICD_SETSPI_NSR
> +     * support 16 bit accesses, and those registers are all part of the
> +     * optional message-based SPI feature which this GIC does not currently
> +     * implement (ie for us GICD_TYPER.MBIS == 0), so for us they are
> +     * reserved.
> +     */
> +    return MEMTX_ERROR;
> +}
> +
> +static MemTxResult gicd_writew(GICv3State *s, hwaddr offset,
> +                               uint64_t value, MemTxAttrs attrs)
> +{
> +    /* Only GICD_SETSPI_NSR, GICD_CLRSPI_NSR, GICD_SETSPI_SR and GICD_SETSPI_NSR
> +     * support 16 bit accesses, and those registers are all part of the
> +     * optional message-based SPI feature which this GIC does not currently
> +     * implement (ie for us GICD_TYPER.MBIS == 0), so for us they are
> +     * reserved.
> +     */
> +    return MEMTX_ERROR;
> +}
> +
> +static MemTxResult gicd_readl(GICv3State *s, hwaddr offset,
> +                              uint64_t *data, MemTxAttrs attrs)
> +{
> +    /* Almost all GICv3 distributor registers are 32-bit.
> +     * Note that WO registers must return an UNKNOWN value on reads,
> +     * not an abort.
> +     */
> +
> +    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 = 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?

> +                                    GICD_CTLR_EN_GRP1NS |
> +                                    GICD_CTLR_RWP);
> +        } else {
> +            *data = s->gicd_ctlr;
> +        }
> +        return MEMTX_OK;


-- 
Shannon