Re: [Xen-devel] [PATCH v5 03/12] arm/mem_access: Add defines supporting PTs with varying page sizes

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien,


On 07/05/2017 01:41 PM, Julien Grall wrote:
>
>
> On 04/07/17 22:33, Sergej Proskurin wrote:
>> Hi Julien,
>
> Hi Sergej,
>
>> On 07/04/2017 06:15 PM, Julien Grall wrote:
>>> Hi Sergej,
>>>
>>
>> [...]
>>
>>>> +
>>>> +#define GUEST_TABLE_OFFSET(offs, gran)          ((paddr_t)(offs) &
>>>> lpae_entry_mask(gran))
>>>> +#define
>>>> GUEST_TABLE_OFFSET_HELPERS(gran)
>>>> \
>>>> +static inline vaddr_t third_guest_table_offset_##gran##K(vaddr_t
>>>> gva)                   \
>>>
>>> Sorry I haven't spot it before. This is not going to work properly on
>>> 32-bit if you use vaddr_t. Indeed, input for stage-2 page-table (i.e
>>> IPA) will be 40-bit. But vaddr_t is 32-bit. So you to use paddr_t here
>>> and in all the helpers below.
>>>
>>
>> I agree that IPAs won't work properly on AArch32. However, we don't walk
>> the second stage translation tables with the introduced code (yet?). In
>> fact, second stage translation walks in software are not supported at
>> the moment. I understand why you would think in this direction, with
>> ARM's nested virtualization support coming up, where we might need to
>> walk the second stage translation tables in sw. Yet, with the current
>> implementation, we work on on GVAs (not IPAs) and hence the vaddr_t
>> should not present an issue (except that the now missing CONFIG_ARM_64
>> #ifdef's in the long-descriptor translation table walk create compile
>> issues as we need to support both different page granularities and
>> zeroeth-level offsets which work on gva's > 32bit on AArch64).
>>
>> If you wish to see the implementation extended in the future to support
>> walking the 2nd stage address translation, then I will gladly change
>> vaddr_t to paddr_t.
>
> Rather than justifying with: "We don't use like that today, so it is
> fine to keep the bug", you should think: "How can it be used in the
> future?". And when I see a cast from vaddr_t to paddr_t in the code,
> then I can directly tell something is wrong.
>
> We already have code to walk stage-2 page-table (see p2m_lookup) and
> are using similar macro a bit everywhere in the p2m code. I was
> actually thinking to make *_table_offset an alias to *_table_offset_4k
> because they are the same. Note I am not asking modify the p2m code...
>
> Even though the stage-2 code does not use those helpers today, I don't
> see any valid reasons to keep a known latent bug in the code. It will
> likely be forgotten and I wish good luck of the developer who will
> have the issue.
>
> BTW, I think you can drop "guest" in the name because those new helps
> are very generic.

Please don't understand me wrong. I am absolutely up for the change and
simply wanted to understand the reason behind your suggestion. The type
change will be part of the next version (including the naming
suggestion). Thank you.

Cheers,
~Sergej



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