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@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). > I prefer the comment at the implementation. Thanks, drew
