Re: [Qemu-arm] [PATCH 11/23] hw/intc/arm_gicv3: Implement GICv3 distributor registers

[Shannon Zhao <shannon.zhao@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