Re: [RFC PATCH v2 6/8] arm/mem_access: Add long-descriptor based gpt

[Sergej Proskurin <proskurin@sec.in.tum.de>]

Hi Julien,


On 06/12/2017 12:44 PM, Julien Grall wrote:
>
>
> On 12/06/17 11:12, Sergej Proskurin wrote:
>> Hi Julien,
>
> Hello Sergej,
>
>>>
>>>> +
>>>> +    const unsigned int strides[3] = {
>>>> +        LPAE_SHIFT_4K,
>>>> +        LPAE_SHIFT_16K,
>>>> +        LPAE_SHIFT_64K
>>>> +    };
>>>
>>> Also, the stride can be found from the page shift. So I am not
>>> convinced you need that.
>>
>> Sure, but don't you think it is cleaner doing it that way, than
>> subtracting a value from PAGE_SHIFT_* although we have already a
>> suitable define for that? Apart from that, we need to consider different
>> strides for different granularities, which would make it harder to
>> read/review if we don't use an array of strides at this point. For
>> instance, see the following formula to compute the starting level of the
>> translation tables:
>>
>> level = 4 - DIV_ROUND_UP((input_size - grainsizes[gran]),
>> strides[gran]);
>
> Looking at AArch64.TranslationTableWalk the stride is basically:
>
> stride = grainsize - 3; // Log2(page size / 8 bytes)
>
> So I still don't see how this would make the code less readable. This
> would also avoid to introduce yet another array on the stack (though
> it should really have been static) for limited purpose

Yeap, I have already made both of the arrays static. However, removing
the strides array completely is absolutely fine by me too.

Cheers,
~Sergej

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