On 10/2/2020 11:18 AM, Julien Grall wrote:
> Hi,
>
> On 02/10/2020 00:52, Stefano Stabellini wrote:
>> On Mon, 28 Sep 2020, laurentiu.tu...@nxp.com wrote:
>>> From: Laurentiu Tudor <laurentiu.tu...@nxp.com>
>>>
>>> Don't hardcode the lookup start level of the page table walk to 1
>>> and instead match the one used in P2M. This should fix scenarios
>>> involving SMMU where the start level is different than 1.
>>>
>>> Signed-off-by: Laurentiu Tudor <laurentiu.tu...@nxp.com>
>>
>> Thank you for the patch, I think it is correct, except that smmu.c today
>> can be enabled even on arm32 builds, where p2m_root_level would be
>> uninitialized.
>>
>> We need to initialize p2m_root_level at the beginning of
>> setup_virt_paging under the #ifdef CONFIG_ARM_32. We can statically
>> initialize it to 1 in that case. Or...
>>
>>
>>> ---
>>> xen/arch/arm/p2m.c | 2 +-
>>> xen/drivers/passthrough/arm/smmu.c | 2 +-
>>> xen/include/asm-arm/p2m.h | 1 +
>>> 3 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>> index ce59f2b503..0181b09dc0 100644
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -18,7 +18,6 @@
>>> #ifdef CONFIG_ARM_64
>>> static unsigned int __read_mostly p2m_root_order;
>>> -static unsigned int __read_mostly p2m_root_level;
>>> #define P2M_ROOT_ORDER p2m_root_order
>>> #define P2M_ROOT_LEVEL p2m_root_level
>>> static unsigned int __read_mostly max_vmid = MAX_VMID_8_BIT;
>>> @@ -39,6 +38,7 @@ static unsigned int __read_mostly max_vmid =
>>> MAX_VMID_8_BIT;
>>> * restricted by external entity (e.g. IOMMU).
>>> */
>>> unsigned int __read_mostly p2m_ipa_bits = 64;
>>> +unsigned int __read_mostly p2m_root_level;
>>
>> ... we could p2m_root_level = 1; here
>
> IMHO, this is going to make the code quite confusing given that only the
> SMMU would use this variable for arm32.
>
> The P2M root level also cannot be changed by the SMMU (at least for
> now). So I would suggest to introduce a helper (maybe
> p2m_get_root_level()) and use it in the SMMU code.
>
> An alternative would be to move the definition of P2M_ROOT_{ORDER,
> LEVEL} in p2m.h
Alright, I'll go with this second option if that's ok with you.
---
Thanks & Best Regards, Laurentiu
