Re: [Xen-devel] [PATCH v7 17/28] xen/arm: ITS: Add GITS registers emulation

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 24/09/15 12:29, Ian Campbell wrote:
> On Thu, 2015-09-24 at 12:05 +0100, Julien Grall wrote:
>> On 24/09/15 06:07, Vijay Kilari wrote:
>>> On Tue, Sep 22, 2015 at 8:00 PM, Julien Grall <julien.grall@xxxxxxxxxx>
>>> wrote:
>>>> Hi Vijay,
>>>>
>>>> On 18/09/15 14:09, vijay.kilari@xxxxxxxxx wrote:
>>> [...]
>>>>> +    case 0x0010 ... 0x007c:
>>>>> +    case 0xc000 ... 0xffcc:
>>>>> +        /* Implementation defined -- write ignored */
>>>>> +        goto write_ignore;
>>>>> +    case GITS_CBASER:
>>>>> +        /* XXX: support 32-bit access */
>>>>
>>>> You haven't asked my question on v6... What is missing to support 32
>>>> -bt
>>>> support? AFAICT the register helpers should the job for you. Correct?
>>>
>>>    GITS_CBASER.Physical Address holds bits[47:12], Being at
>>> physical address at bits [47:12]  cannot be updated with single 32-bit
>>> access.
>>
>> I'd like you to explain why GITS_CBASER can't be updated with single
>> 32-bit access. Because, based on the spec it's possible (see 8.19.2 ARM
>> IHI 0069A):
>>
>> "Bits [63:32] and bits [31:0] are accessible separately."
> 
> You clearly can't update the entire 47:12 range atomically with a 32-bit
> access, which I suppose is what Vijay means.

The GITS_CBASER holds when address of the command queue which can only
be used when the ITS is enabled (i.e GITS_CTLR.Enable = 1).

As soon as GITS_CTLR.enable is set, it's not possible GITS_CBASER
becomes read-only. So we don't care about atomic access.

Although, the main problem is how the emulation for this register has
been written. As soon as the register is written, we are validating the
fields in the register and store in a convenient way. This give us
little possibility to correctly support 32-bit access.

Given that the register value is never used until GITS_CTLR.enable is
set, we could have validating GITS_CBASER and update our internal state
directly in GITS_CTLR.

Note that this remark is valid for any register relyiong on
GITS_CTLR.Enable and GICR_CTLR.Enable.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel