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@xxxxxxxx> >>> >>> 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@xxxxxxx> >> >> 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
