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

[Julien Grall <julien@xen.org>]

Hi,

On 02/10/2020 10:29, Laurentiu Tudor wrote:
> 
> 
> 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@nxp.com wrote:
>>>> From: Laurentiu Tudor <laurentiu.tudor@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.tudor@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.

I am fine with that.

Cheers,

-- 
Julien Grall