Re: [PATCH] arm,smmu: match start level of page table walk with P2M

[Laurentiu Tudor <laurentiu.tudor@xxxxxxx>]

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.tudor@xxxxxxx wrote:
>>> From: Laurentiu Tudor <laurentiu.tudor@xxxxxxx>
>>>
>>> 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.tudor@xxxxxxx>
>>
>> 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