* [PATCH] arm,smmu: match start level of page table walk with P2M
@ 2020-09-28 13:51 laurentiu.tudor
2020-10-01 23:52 ` Stefano Stabellini
0 siblings, 1 reply; 6+ messages in thread
From: laurentiu.tudor @ 2020-09-28 13:51 UTC (permalink / raw)
To: sstabellini, julien, xen-devel, Volodymyr_Babchuk, will
Cc: diana.craciun, anda-alexandra.dorneanu, Laurentiu Tudor
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>
---
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;
/* Helpers to lookup the properties of each level */
static const paddr_t level_masks[] =
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 94662a8501..85709a136f 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -1152,7 +1152,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
(TTBCR_RGN_WBWA << TTBCR_IRGN0_SHIFT);
if (!stage1)
- reg |= (TTBCR_SL0_LVL_1 << TTBCR_SL0_SHIFT);
+ reg |= (2 - p2m_root_level) << TTBCR_SL0_SHIFT;
writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR);
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 5fdb6e8183..97b5eada2b 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -12,6 +12,7 @@
/* Holds the bit size of IPAs in p2m tables. */
extern unsigned int p2m_ipa_bits;
+extern unsigned int p2m_root_level;
struct domain;
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] arm,smmu: match start level of page table walk with P2M
2020-09-28 13:51 [PATCH] arm,smmu: match start level of page table walk with P2M laurentiu.tudor
@ 2020-10-01 23:52 ` Stefano Stabellini
2020-10-02 7:55 ` Laurentiu Tudor
2020-10-02 8:18 ` Julien Grall
0 siblings, 2 replies; 6+ messages in thread
From: Stefano Stabellini @ 2020-10-01 23:52 UTC (permalink / raw)
To: Laurentiu Tudor
Cc: sstabellini, julien, xen-devel, Volodymyr_Babchuk, will,
diana.craciun, anda-alexandra.dorneanu
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
> /* Helpers to lookup the properties of each level */
> static const paddr_t level_masks[] =
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 94662a8501..85709a136f 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -1152,7 +1152,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
> (TTBCR_RGN_WBWA << TTBCR_IRGN0_SHIFT);
>
> if (!stage1)
> - reg |= (TTBCR_SL0_LVL_1 << TTBCR_SL0_SHIFT);
> + reg |= (2 - p2m_root_level) << TTBCR_SL0_SHIFT;
>
> writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR);
>
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 5fdb6e8183..97b5eada2b 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -12,6 +12,7 @@
>
> /* Holds the bit size of IPAs in p2m tables. */
> extern unsigned int p2m_ipa_bits;
> +extern unsigned int p2m_root_level;
>
> struct domain;
>
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm,smmu: match start level of page table walk with P2M
2020-10-01 23:52 ` Stefano Stabellini
@ 2020-10-02 7:55 ` Laurentiu Tudor
2020-10-02 8:18 ` Julien Grall
1 sibling, 0 replies; 6+ messages in thread
From: Laurentiu Tudor @ 2020-10-02 7:55 UTC (permalink / raw)
To: Stefano Stabellini
Cc: julien, xen-devel, Volodymyr_Babchuk, will, diana.craciun,
anda-alexandra.dorneanu
On 10/2/2020 2:52 AM, 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
>
This looks straight forward and in line with what we do with
p2m_ipa_bits, I'll send a v2 right away.
Thanks for the review.
---
Best Regards, Laurentiu
>> /* Helpers to lookup the properties of each level */
>> static const paddr_t level_masks[] =
>> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
>> index 94662a8501..85709a136f 100644
>> --- a/xen/drivers/passthrough/arm/smmu.c
>> +++ b/xen/drivers/passthrough/arm/smmu.c
>> @@ -1152,7 +1152,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
>> (TTBCR_RGN_WBWA << TTBCR_IRGN0_SHIFT);
>>
>> if (!stage1)
>> - reg |= (TTBCR_SL0_LVL_1 << TTBCR_SL0_SHIFT);
>> + reg |= (2 - p2m_root_level) << TTBCR_SL0_SHIFT;
>>
>> writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR);
>>
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index 5fdb6e8183..97b5eada2b 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -12,6 +12,7 @@
>>
>> /* Holds the bit size of IPAs in p2m tables. */
>> extern unsigned int p2m_ipa_bits;
>> +extern unsigned int p2m_root_level;
>>
>> struct domain;
>>
>> --
>> 2.17.1
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm,smmu: match start level of page table walk with P2M
2020-10-01 23:52 ` Stefano Stabellini
2020-10-02 7:55 ` Laurentiu Tudor
@ 2020-10-02 8:18 ` Julien Grall
2020-10-02 9:29 ` Laurentiu Tudor
1 sibling, 1 reply; 6+ messages in thread
From: Julien Grall @ 2020-10-02 8:18 UTC (permalink / raw)
To: Stefano Stabellini, Laurentiu Tudor
Cc: xen-devel, Volodymyr_Babchuk, will, diana.craciun,
anda-alexandra.dorneanu
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
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm,smmu: match start level of page table walk with P2M
2020-10-02 8:18 ` Julien Grall
@ 2020-10-02 9:29 ` Laurentiu Tudor
2020-10-02 9:35 ` Julien Grall
0 siblings, 1 reply; 6+ messages in thread
From: Laurentiu Tudor @ 2020-10-02 9:29 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini
Cc: xen-devel, Volodymyr_Babchuk, will, diana.craciun,
anda-alexandra.dorneanu
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.
---
Thanks & Best Regards, Laurentiu
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm,smmu: match start level of page table walk with P2M
2020-10-02 9:29 ` Laurentiu Tudor
@ 2020-10-02 9:35 ` Julien Grall
0 siblings, 0 replies; 6+ messages in thread
From: Julien Grall @ 2020-10-02 9:35 UTC (permalink / raw)
To: Laurentiu Tudor, Stefano Stabellini
Cc: xen-devel, Volodymyr_Babchuk, will, diana.craciun,
anda-alexandra.dorneanu
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-02 9:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-28 13:51 [PATCH] arm,smmu: match start level of page table walk with P2M laurentiu.tudor
2020-10-01 23:52 ` Stefano Stabellini
2020-10-02 7:55 ` Laurentiu Tudor
2020-10-02 8:18 ` Julien Grall
2020-10-02 9:29 ` Laurentiu Tudor
2020-10-02 9:35 ` Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.