Re: [PATCH v2 04/27] ARM: GICv3 ITS: map ITS command buffer

[Julien Grall <julien.grall@arm.com>]

Hi Andre,

On 22/03/17 16:31, André Przywara wrote:
> On 22/03/17 15:23, Julien Grall wrote:
>> On 16/03/17 11:20, Andre Przywara wrote:
>>> +
>>>  LIST_HEAD(host_its_list);
>>>
>>>  bool gicv3_its_host_has_its(void)
>>> @@ -56,6 +59,55 @@ static uint64_t encode_propbaser_phys_addr(paddr_t
>>> addr, unsigned int page_bits)
>>>      return ret | ((addr & GENMASK(51, 48)) >> (48 - 12));
>>>  }
>>>
>>> +static void *its_map_cbaser(struct host_its *its)
>>> +{
>>> +    void __iomem *cbasereg = its->its_base + GITS_CBASER;
>>> +    uint64_t reg;
>>> +    void *buffer;
>>> +    unsigned int order;
>>> +
>>> +    reg  = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
>>> +    reg |= GIC_BASER_CACHE_SameAsInner <<
>>> GITS_BASER_OUTER_CACHEABILITY_SHIFT;
>>> +    reg |= GIC_BASER_CACHE_RaWaWb <<
>>> GITS_BASER_INNER_CACHEABILITY_SHIFT;
>>> +
>>> +    /* The ITS command buffer needs to be 64K aligned. */
>>
>> Looking at the spec, the command buffer does not need to be 64K aligned.
>> On the previous version, you made it 4K aligned. So why this restriction?
>
> As you have already learnt, the GIC is more subtle sometimes ;-) Read
> the description at "Physical_Address, bits [51:12]" in the CBASER
> paragraph and tell me what you take from it. I decided to read it as
> "has to be 64K aligned".
> Happy to correct this otherwise.

Hmmm. I think you are right. This is a little bit weird, but fine. Lets 
keep like that.

>
>>> +    order = max(get_order_from_bytes(ITS_CMD_QUEUE_SZ), 16U -
>>> PAGE_SHIFT);
>>> +    buffer = alloc_xenheap_pages(order, 0);
>>
>> I am not sure to understand why you move from _zalloc to
>> alloc_xenheap_*. The resulting behavior will be exactly the same but the
>> former result to a simpler code.
>                                                   ^^^^^^^^^^^^^^^^
> Really?
> I checked and found that alloc_xenheap_pages gives me physically
> contiguous allocation in the given order, where *alloc just guarantees
> virtually contiguous pages.

Yes. Give a look at my answer on patch #3.

Regards,

-- 
Julien Grall

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