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

[Andrew Jones <drjones@redhat.com>]

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