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

[Nikos Nikoleris <nikos.nikoleris@arm.com>]

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.