[kvm-unit-tests PATCH 0/2] arm: MMU extentions to enable litmus7

[kvm-unit-tests PATCH 0/2] arm: MMU extentions to enable litmus7

[Nikos Nikoleris]

Hi all,

litmus7 [1][2], a tool that we develop and use to test the memory
model on hardware, is building on kvm-unit-tests to encapsulate full
system tests and control address translation. This series extends the
kvm-unit-tests arm MMU API and adds two memory attributes to MAIR_EL1
to make them available to the litmus tests.

[1]: http://diy.inria.fr/doc/litmus.html
[2]: https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/posts/expanding-memory-model-tools-system-level-architecture

Thanks,

Nikos


Luc Maranget (1):
  arm: Add mmu_get_pte() to the MMU API

Nikos Nikoleris (1):
  arm: Add support for the DEVICE_nGRE and NORMAL_WT memory types

 lib/arm/asm/mmu-api.h         |  1 +
 lib/arm64/asm/pgtable-hwdef.h |  2 ++
 lib/arm/mmu.c                 | 23 ++++++++++++++---------
 arm/cstart64.S                |  6 +++++-
 4 files changed, 22 insertions(+), 10 deletions(-)

-- 
2.17.1

[kvm-unit-tests PATCH 1/2] arm: Add mmu_get_pte() to the MMU API

[Nikos Nikoleris]

From: Luc Maranget <Luc.Maranget@inria.fr>

Add the mmu_get_pte() function that allows a test to get a pointer to
the PTE for a valid virtual address. Return NULL if the MMU is off.

Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
---
 lib/arm/asm/mmu-api.h |  1 +
 lib/arm/mmu.c         | 23 ++++++++++++++---------
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/lib/arm/asm/mmu-api.h b/lib/arm/asm/mmu-api.h
index 2bbe1fa..3d04d03 100644
--- a/lib/arm/asm/mmu-api.h
+++ b/lib/arm/asm/mmu-api.h
@@ -22,5 +22,6 @@ extern void mmu_set_range_sect(pgd_t *pgtable, uintptr_t virt_offset,
 extern void mmu_set_range_ptes(pgd_t *pgtable, uintptr_t virt_offset,
 			       phys_addr_t phys_start, phys_addr_t phys_end,
 			       pgprot_t prot);
+extern pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr);
 extern void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr);
 #endif
diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
index 51fa745..2113604 100644
--- a/lib/arm/mmu.c
+++ b/lib/arm/mmu.c
@@ -210,7 +210,7 @@ unsigned long __phys_to_virt(phys_addr_t addr)
 	return addr;
 }
 
-void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
+pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr)
 {
 	pgd_t *pgd;
 	pud_t *pud;
@@ -218,7 +218,7 @@ void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
 	pte_t *pte;
 
 	if (!mmu_enabled())
-		return;
+		return NULL;
 
 	pgd = pgd_offset(pgtable, vaddr);
 	assert(pgd_valid(*pgd));
@@ -228,16 +228,21 @@ void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
 	assert(pmd_valid(*pmd));
 
 	if (pmd_huge(*pmd)) {
-		pmd_t entry = __pmd(pmd_val(*pmd) & ~PMD_SECT_USER);
-		WRITE_ONCE(*pmd, entry);
-		goto out_flush_tlb;
+		return &pmd_val(*pmd);
 	}
 
 	pte = pte_offset(pmd, vaddr);
 	assert(pte_valid(*pte));
-	pte_t entry = __pte(pte_val(*pte) & ~PTE_USER);
-	WRITE_ONCE(*pte, entry);
 
-out_flush_tlb:
-	flush_tlb_page(vaddr);
+        return &pte_val(*pte);
+}
+
+void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
+{
+	pteval_t *p_pte = mmu_get_pte(pgtable, vaddr);
+	if (p_pte) {
+		pteval_t entry = *p_pte & ~PTE_USER;
+		WRITE_ONCE(*p_pte, entry);
+		flush_tlb_page(vaddr);
+	}
 }
-- 
2.17.1

[kvm-unit-tests PATCH 2/2] arm: Add support for the DEVICE_nGRE and NORMAL_WT memory types

[Nikos Nikoleris]

Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
---
 lib/arm64/asm/pgtable-hwdef.h | 2 ++
 arm/cstart64.S                | 6 +++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/arm64/asm/pgtable-hwdef.h b/lib/arm64/asm/pgtable-hwdef.h
index 3b6b0d6..16b59ba 100644
--- a/lib/arm64/asm/pgtable-hwdef.h
+++ b/lib/arm64/asm/pgtable-hwdef.h
@@ -143,5 +143,7 @@
 #define MT_DEVICE_GRE		2
 #define MT_NORMAL_NC		3	/* writecombine */
 #define MT_NORMAL		4
+#define MT_NORMAL_WT		5
+#define MT_DEVICE_nGRE		6
 
 #endif /* _ASMARM64_PGTABLE_HWDEF_H_ */
diff --git a/arm/cstart64.S b/arm/cstart64.S
index cedc678..540994d 100644
--- a/arm/cstart64.S
+++ b/arm/cstart64.S
@@ -154,6 +154,8 @@ halt:
  *   DEVICE_GRE         010     00001100
  *   NORMAL_NC          011     01000100
  *   NORMAL             100     11111111
+ *   NORMAL_WT          101     10111011
+ *   DEVICE_nGRE        110     00001000
  */
 #define MAIR(attr, mt) ((attr) << ((mt) * 8))
 
@@ -184,7 +186,9 @@ asm_mmu_enable:
 		     MAIR(0x04, MT_DEVICE_nGnRE) |	\
 		     MAIR(0x0c, MT_DEVICE_GRE) |	\
 		     MAIR(0x44, MT_NORMAL_NC) |		\
-		     MAIR(0xff, MT_NORMAL)
+		     MAIR(0xff, MT_NORMAL) |	        \
+		     MAIR(0xbb, MT_NORMAL_WT) |         \
+		     MAIR(0x08, MT_DEVICE_nGRE)
 	msr	mair_el1, x1
 
 	/* TTBR0 */
-- 
2.17.1

Re: [kvm-unit-tests PATCH 0/2] arm: MMU extentions to enable litmus7

[Andrew Jones]

On Mon, Nov 02, 2020 at 11:53:09AM +0000, Nikos Nikoleris wrote:
> Hi all,
> 
> litmus7 [1][2], a tool that we develop and use to test the memory
> model on hardware, is building on kvm-unit-tests to encapsulate full
> system tests and control address translation. This series extends the
> kvm-unit-tests arm MMU API and adds two memory attributes to MAIR_EL1
> to make them available to the litmus tests.
> 
> [1]: http://diy.inria.fr/doc/litmus.html
> [2]: https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/posts/expanding-memory-model-tools-system-level-architecture

Hi Nikos,

I'm glad to see this application of kvm-unit-tests. It'd be nice to
extract some of the overview and howto from the blog [2] into a
markdown file that we can add to the kvm-unit-tests repository.

Thanks,
drew

Re: [kvm-unit-tests PATCH 1/2] arm: Add mmu_get_pte() to the MMU API

[Andrew Jones]

On Mon, Nov 02, 2020 at 11:53:10AM +0000, Nikos Nikoleris wrote:
> From: Luc Maranget <Luc.Maranget@inria.fr>
> 
> Add the mmu_get_pte() function that allows a test to get a pointer to
> the PTE for a valid virtual address. Return NULL if the MMU is off.
> 
> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [kvm-unit-tests PATCH 2/2] arm: Add support for the DEVICE_nGRE and NORMAL_WT memory types

[Andrew Jones]

On Mon, Nov 02, 2020 at 11:53:11AM +0000, Nikos Nikoleris wrote:
> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
> ---
>  lib/arm64/asm/pgtable-hwdef.h | 2 ++
>  arm/cstart64.S                | 6 +++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/arm64/asm/pgtable-hwdef.h b/lib/arm64/asm/pgtable-hwdef.h
> index 3b6b0d6..16b59ba 100644
> --- a/lib/arm64/asm/pgtable-hwdef.h
> +++ b/lib/arm64/asm/pgtable-hwdef.h
> @@ -143,5 +143,7 @@
>  #define MT_DEVICE_GRE		2
>  #define MT_NORMAL_NC		3	/* writecombine */
>  #define MT_NORMAL		4
> +#define MT_NORMAL_WT		5
> +#define MT_DEVICE_nGRE		6
>  
>  #endif /* _ASMARM64_PGTABLE_HWDEF_H_ */
> diff --git a/arm/cstart64.S b/arm/cstart64.S
> index cedc678..540994d 100644
> --- a/arm/cstart64.S
> +++ b/arm/cstart64.S
> @@ -154,6 +154,8 @@ halt:
>   *   DEVICE_GRE         010     00001100
>   *   NORMAL_NC          011     01000100
>   *   NORMAL             100     11111111
> + *   NORMAL_WT          101     10111011
> + *   DEVICE_nGRE        110     00001000
>   */
>  #define MAIR(attr, mt) ((attr) << ((mt) * 8))
>  
> @@ -184,7 +186,9 @@ asm_mmu_enable:
>  		     MAIR(0x04, MT_DEVICE_nGnRE) |	\
>  		     MAIR(0x0c, MT_DEVICE_GRE) |	\
>  		     MAIR(0x44, MT_NORMAL_NC) |		\
> -		     MAIR(0xff, MT_NORMAL)
> +		     MAIR(0xff, MT_NORMAL) |	        \
> +		     MAIR(0xbb, MT_NORMAL_WT) |         \
> +		     MAIR(0x08, MT_DEVICE_nGRE)
>  	msr	mair_el1, x1
>  
>  	/* TTBR0 */
> -- 
> 2.17.1
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [kvm-unit-tests PATCH 0/2] arm: MMU extentions to enable litmus7

[Nikos Nikoleris]

On 03/11/2020 17:09, Andrew Jones wrote:
> On Mon, Nov 02, 2020 at 11:53:09AM +0000, Nikos Nikoleris wrote:
>> Hi all,
>>
>> litmus7 [1][2], a tool that we develop and use to test the memory
>> model on hardware, is building on kvm-unit-tests to encapsulate full
>> system tests and control address translation. This series extends the
>> kvm-unit-tests arm MMU API and adds two memory attributes to MAIR_EL1
>> to make them available to the litmus tests.
>>
>> [1]: http://diy.inria.fr/doc/litmus.html
>> [2]: https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/posts/expanding-memory-model-tools-system-level-architecture
> 
> Hi Nikos,
> 
> I'm glad to see this application of kvm-unit-tests. It'd be nice to
> extract some of the overview and howto from the blog [2] into a
> markdown file that we can add to the kvm-unit-tests repository.
> 

Hi Drew,

Thanks for the reviews!

Very happy to do that I will work with Jade and Luc to write a howto and
will post a patch soon.

Thanks,

Nikos

> Thanks,
> drew
> 

Re: [kvm-unit-tests PATCH 1/2] arm: Add mmu_get_pte() to the MMU API

[Alexandru Elisei]

Hi Nikos,

Very good idea! Minor comments below.

On 11/2/20 11:53 AM, Nikos Nikoleris wrote:
> From: Luc Maranget <Luc.Maranget@inria.fr>
>
> Add the mmu_get_pte() function that allows a test to get a pointer to
> the PTE for a valid virtual address. Return NULL if the MMU is off.
>
> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>

Missing Signed-off-by from Luc Maranget.

> ---
>  lib/arm/asm/mmu-api.h |  1 +
>  lib/arm/mmu.c         | 23 ++++++++++++++---------
>  2 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/lib/arm/asm/mmu-api.h b/lib/arm/asm/mmu-api.h
> index 2bbe1fa..3d04d03 100644
> --- a/lib/arm/asm/mmu-api.h
> +++ b/lib/arm/asm/mmu-api.h
> @@ -22,5 +22,6 @@ extern void mmu_set_range_sect(pgd_t *pgtable, uintptr_t virt_offset,
>  extern void mmu_set_range_ptes(pgd_t *pgtable, uintptr_t virt_offset,
>  			       phys_addr_t phys_start, phys_addr_t phys_end,
>  			       pgprot_t prot);
> +extern pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr);
>  extern void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr);
>  #endif
> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
> index 51fa745..2113604 100644
> --- a/lib/arm/mmu.c
> +++ b/lib/arm/mmu.c
> @@ -210,7 +210,7 @@ unsigned long __phys_to_virt(phys_addr_t addr)
>  	return addr;
>  }
>  
> -void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
> +pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr)

I was thinking it might be nice to have a comment here reminding callers to use
break-before-make when necessary, with a reference to the pages in the Arm ARM
where the exact conditions can be found (D5-2669 for armv8, B3-1378 for armv7). It
might save someone a lot of time debugging a once in 100 runs bug because they
forgot to do break-before-make. And having the exact page number will make it much
easier to find the relevant section.

>  {
>  	pgd_t *pgd;
>  	pud_t *pud;
> @@ -218,7 +218,7 @@ void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
>  	pte_t *pte;
>  
>  	if (!mmu_enabled())
> -		return;
> +		return NULL;
>  
>  	pgd = pgd_offset(pgtable, vaddr);
>  	assert(pgd_valid(*pgd));
> @@ -228,16 +228,21 @@ void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
>  	assert(pmd_valid(*pmd));
>  
>  	if (pmd_huge(*pmd)) {
> -		pmd_t entry = __pmd(pmd_val(*pmd) & ~PMD_SECT_USER);
> -		WRITE_ONCE(*pmd, entry);
> -		goto out_flush_tlb;
> +		return &pmd_val(*pmd);
>  	}

The braces are unnecessary now.

With the comments above fixed:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,
Alex
>  
>  	pte = pte_offset(pmd, vaddr);
>  	assert(pte_valid(*pte));
> -	pte_t entry = __pte(pte_val(*pte) & ~PTE_USER);
> -	WRITE_ONCE(*pte, entry);
>  
> -out_flush_tlb:
> -	flush_tlb_page(vaddr);
> +        return &pte_val(*pte);
> +}
> +
> +void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
> +{
> +	pteval_t *p_pte = mmu_get_pte(pgtable, vaddr);
> +	if (p_pte) {
> +		pteval_t entry = *p_pte & ~PTE_USER;
> +		WRITE_ONCE(*p_pte, entry);
> +		flush_tlb_page(vaddr);
> +	}
>  }

Re: [kvm-unit-tests PATCH 1/2] arm: Add mmu_get_pte() to the MMU API

[Nikos Nikoleris]

Hi Alex,

On 05/11/2020 14:27, Alexandru Elisei wrote:
> Hi Nikos,
>
> Very good idea! Minor comments below.
>
> On 11/2/20 11:53 AM, Nikos Nikoleris wrote:
>> From: Luc Maranget <Luc.Maranget@inria.fr>
>>
>> Add the mmu_get_pte() function that allows a test to get a pointer to
>> the PTE for a valid virtual address. Return NULL if the MMU is off.
>>
>> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
>
> Missing Signed-off-by from Luc Maranget.
>
>> ---
>>   lib/arm/asm/mmu-api.h |  1 +
>>   lib/arm/mmu.c         | 23 ++++++++++++++---------
>>   2 files changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/arm/asm/mmu-api.h b/lib/arm/asm/mmu-api.h
>> index 2bbe1fa..3d04d03 100644
>> --- a/lib/arm/asm/mmu-api.h
>> +++ b/lib/arm/asm/mmu-api.h
>> @@ -22,5 +22,6 @@ extern void mmu_set_range_sect(pgd_t *pgtable, uintptr_t virt_offset,
>>   extern void mmu_set_range_ptes(pgd_t *pgtable, uintptr_t virt_offset,
>>                             phys_addr_t phys_start, phys_addr_t phys_end,
>>                             pgprot_t prot);
>> +extern pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr);
>>   extern void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr);
>>   #endif
>> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
>> index 51fa745..2113604 100644
>> --- a/lib/arm/mmu.c
>> +++ b/lib/arm/mmu.c
>> @@ -210,7 +210,7 @@ unsigned long __phys_to_virt(phys_addr_t addr)
>>      return addr;
>>   }
>>
>> -void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
>> +pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr)
>
> I was thinking it might be nice to have a comment here reminding callers to use
> break-before-make when necessary, with a reference to the pages in the Arm ARM
> where the exact conditions can be found (D5-2669 for armv8, B3-1378 for armv7). It
> might save someone a lot of time debugging a once in 100 runs bug because they
> forgot to do break-before-make. And having the exact page number will make it much
> easier to find the relevant section.

Good idea if this is part of the API, it would be good to have a
reference to break-before-make. I am thinking of adding it in
lib/arm/asm/mmu-api.h where the MMU API is, just before the declaration:

extern pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr)

or would you rather have it in lib/arm/mmu.c with the code?

Thanks,

Nikos

>
>>   {
>>      pgd_t *pgd;
>>      pud_t *pud;
>> @@ -218,7 +218,7 @@ void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
>>      pte_t *pte;
>>
>>      if (!mmu_enabled())
>> -            return;
>> +            return NULL;
>>
>>      pgd = pgd_offset(pgtable, vaddr);
>>      assert(pgd_valid(*pgd));
>> @@ -228,16 +228,21 @@ void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
>>      assert(pmd_valid(*pmd));
>>
>>      if (pmd_huge(*pmd)) {
>> -            pmd_t entry = __pmd(pmd_val(*pmd) & ~PMD_SECT_USER);
>> -            WRITE_ONCE(*pmd, entry);
>> -            goto out_flush_tlb;
>> +            return &pmd_val(*pmd);
>>      }
>
> The braces are unnecessary now.
>
> With the comments above fixed:
>
> Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
>
> Thanks,
> Alex
>>
>>      pte = pte_offset(pmd, vaddr);
>>      assert(pte_valid(*pte));
>> -    pte_t entry = __pte(pte_val(*pte) & ~PTE_USER);
>> -    WRITE_ONCE(*pte, entry);
>>
>> -out_flush_tlb:
>> -    flush_tlb_page(vaddr);
>> +        return &pte_val(*pte);
>> +}
>> +
>> +void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
>> +{
>> +    pteval_t *p_pte = mmu_get_pte(pgtable, vaddr);
>> +    if (p_pte) {
>> +            pteval_t entry = *p_pte & ~PTE_USER;
>> +            WRITE_ONCE(*p_pte, entry);
>> +            flush_tlb_page(vaddr);
>> +    }
>>   }
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [kvm-unit-tests PATCH 1/2] arm: Add mmu_get_pte() to the MMU API

[Alexandru Elisei]

Hi Nikos,

On 11/7/20 11:01 AM, Nikos Nikoleris wrote:
> Hi Alex,
>
> On 05/11/2020 14:27, Alexandru Elisei wrote:
>> Hi Nikos,
>>
>> Very good idea! Minor comments below.
>>
>> On 11/2/20 11:53 AM, Nikos Nikoleris wrote:
>>> From: Luc Maranget <Luc.Maranget@inria.fr>
>>>
>>> Add the mmu_get_pte() function that allows a test to get a pointer to
>>> the PTE for a valid virtual address. Return NULL if the MMU is off.
>>>
>>> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
>>
>> Missing Signed-off-by from Luc Maranget.
>>
>>> ---
>>>   lib/arm/asm/mmu-api.h |  1 +
>>>   lib/arm/mmu.c         | 23 ++++++++++++++---------
>>>   2 files changed, 15 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/lib/arm/asm/mmu-api.h b/lib/arm/asm/mmu-api.h
>>> index 2bbe1fa..3d04d03 100644
>>> --- a/lib/arm/asm/mmu-api.h
>>> +++ b/lib/arm/asm/mmu-api.h
>>> @@ -22,5 +22,6 @@ extern void mmu_set_range_sect(pgd_t *pgtable, uintptr_t
>>> virt_offset,
>>>   extern void mmu_set_range_ptes(pgd_t *pgtable, uintptr_t virt_offset,
>>>                      phys_addr_t phys_start, phys_addr_t phys_end,
>>>                      pgprot_t prot);
>>> +extern pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr);
>>>   extern void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr);
>>>   #endif
>>> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
>>> index 51fa745..2113604 100644
>>> --- a/lib/arm/mmu.c
>>> +++ b/lib/arm/mmu.c
>>> @@ -210,7 +210,7 @@ unsigned long __phys_to_virt(phys_addr_t addr)
>>>       return addr;
>>>   }
>>>   -void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
>>> +pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr)
>>
>> I was thinking it might be nice to have a comment here reminding callers to use
>> break-before-make when necessary, with a reference to the pages in the Arm ARM
>> where the exact conditions can be found (D5-2669 for armv8, B3-1378 for armv7). It
>> might save someone a lot of time debugging a once in 100 runs bug because they
>> forgot to do break-before-make. And having the exact page number will make it much
>> easier to find the relevant section.
>
> Good idea if this is part of the API, it would be good to have a reference to
> break-before-make. I am thinking of adding it in lib/arm/asm/mmu-api.h where the
> MMU API is, just before the declaration:
>
> extern pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr)
>
> or would you rather have it in lib/arm/mmu.c with the code?

There are no function comments anywhere in mmu-api.h or mmu.c, so I guess it's up
to you where you want to put it. I believe the usual convention is to put the
comments where the function is implemented because it's easier to understand what
the function does if the comment is right above it, and it makes it easier if you
have one prototype in a header file and multiple implementations in different .c
files. I see some comments in io.c and gic.c which seem to follow this convention
(unless Drew prefers otherwise).

Thanks,
Alex

Re: [kvm-unit-tests PATCH 1/2] arm: Add mmu_get_pte() to the MMU API

[Andrew Jones]

On Mon, Nov 09, 2020 at 12:36:36PM +0000, Alexandru Elisei wrote:
> Hi Nikos,
> 
> On 11/7/20 11:01 AM, Nikos Nikoleris wrote:
> > Hi Alex,
> >
> > On 05/11/2020 14:27, Alexandru Elisei wrote:
> >> Hi Nikos,
> >>
> >> Very good idea! Minor comments below.
> >>
> >> On 11/2/20 11:53 AM, Nikos Nikoleris wrote:
> >>> From: Luc Maranget <Luc.Maranget@inria.fr>
> >>>
> >>> Add the mmu_get_pte() function that allows a test to get a pointer to
> >>> the PTE for a valid virtual address. Return NULL if the MMU is off.
> >>>
> >>> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
> >>
> >> Missing Signed-off-by from Luc Maranget.
> >>
> >>> ---
> >>>   lib/arm/asm/mmu-api.h |  1 +
> >>>   lib/arm/mmu.c         | 23 ++++++++++++++---------
> >>>   2 files changed, 15 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/lib/arm/asm/mmu-api.h b/lib/arm/asm/mmu-api.h
> >>> index 2bbe1fa..3d04d03 100644
> >>> --- a/lib/arm/asm/mmu-api.h
> >>> +++ b/lib/arm/asm/mmu-api.h
> >>> @@ -22,5 +22,6 @@ extern void mmu_set_range_sect(pgd_t *pgtable, uintptr_t
> >>> virt_offset,
> >>>   extern void mmu_set_range_ptes(pgd_t *pgtable, uintptr_t virt_offset,
> >>>                      phys_addr_t phys_start, phys_addr_t phys_end,
> >>>                      pgprot_t prot);
> >>> +extern pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr);
> >>>   extern void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr);
> >>>   #endif
> >>> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
> >>> index 51fa745..2113604 100644
> >>> --- a/lib/arm/mmu.c
> >>> +++ b/lib/arm/mmu.c
> >>> @@ -210,7 +210,7 @@ unsigned long __phys_to_virt(phys_addr_t addr)
> >>>       return addr;
> >>>   }
> >>>   -void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
> >>> +pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr)
> >>
> >> I was thinking it might be nice to have a comment here reminding callers to use
> >> break-before-make when necessary, with a reference to the pages in the Arm ARM
> >> where the exact conditions can be found (D5-2669 for armv8, B3-1378 for armv7). It
> >> might save someone a lot of time debugging a once in 100 runs bug because they
> >> forgot to do break-before-make. And having the exact page number will make it much
> >> easier to find the relevant section.
> >
> > Good idea if this is part of the API, it would be good to have a reference to
> > break-before-make. I am thinking of adding it in lib/arm/asm/mmu-api.h where the
> > MMU API is, just before the declaration:
> >
> > extern pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr)
> >
> > or would you rather have it in lib/arm/mmu.c with the code?
> 
> There are no function comments anywhere in mmu-api.h or mmu.c, so I guess it's up
> to you where you want to put it. I believe the usual convention is to put the
> comments where the function is implemented because it's easier to understand what
> the function does if the comment is right above it, and it makes it easier if you
> have one prototype in a header file and multiple implementations in different .c
> files. I see some comments in io.c and gic.c which seem to follow this convention
> (unless Drew prefers otherwise).
>

I prefer the comment at the implementation.

Thanks,
drew