diff mbox series

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

Message ID 20201102115311.103750-2-nikos.nikoleris@arm.com (mailing list archive)
State New, archived
Headers show
Series [kvm-unit-tests,1/2] arm: Add mmu_get_pte() to the MMU API | expand

Commit Message

Nikos Nikoleris Nov. 2, 2020, 11:53 a.m. UTC 
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(-)

Comments

Andrew Jones Nov. 3, 2020, 5:10 p.m. UTC | #1 
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>
Alexandru Elisei Nov. 5, 2020, 2:27 p.m. UTC | #2 
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);
> +	}
>  }
Nikos Nikoleris Nov. 7, 2020, 11:01 a.m. UTC | #3 
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.
Alexandru Elisei Nov. 9, 2020, 12:36 p.m. UTC | #4 
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
Andrew Jones Nov. 9, 2020, 12:49 p.m. UTC | #5 
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
diff mbox series

Patch

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);
+	}
 }