On 13 May 2016 at 16:05, Shannon Zhao <shannon.z...@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
