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

[Peter Maydell <peter.maydell@linaro.org>]

On 13 May 2016 at 16:05, Shannon Zhao <shannon.zhao@linaro.org> wrote:
> 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().

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 = 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 = s->gicd_ctlr;
>> +        }
>> +        return MEMTX_OK;

thanks
-- PMM