Re: [Minios-devel] [UNIKRAFT PATCHv3 20/25] plat/kvm: arm64: Build L3 page table for range greater than 2MB

[Jia He <jiakernel2@xxxxxxxxx>]

Hi Sharan, sorry for the delay

I have to spend some time to understand Wei's whole mmu design.

Please see my comments below

On 2019/1/17 17:18, Sharan Santhanam wrote:
> Hello Wei Chen,
>
> Please find the comment inline
>
> Thanks & Regards
> Sharan
>
> On 12/13/18 10:15 AM, Wei Chen wrote:
> > Current L3 table build function only supports to build L3 table for
> > memory range not exceeded than 2MB. But as we mentioned in previous
> > patch, the image size will be greater than 2MB. So in this case, build
> > L3 table for bigger image is not convenient. We have improve the
> > function of building L3 table to support bigger memory range.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > ---
> >   plat/kvm/arm/pagetable64.S | 19 ++++++++++++++++---
> >   1 file changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/plat/kvm/arm/pagetable64.S b/plat/kvm/arm/pagetable64.S
> > index bd473bf..212f8e0 100644
> > --- a/plat/kvm/arm/pagetable64.S
> > +++ b/plat/kvm/arm/pagetable64.S
> > @@ -427,7 +427,13 @@ link_l2_pagetable:
> >    *  x11, x12 and x13 are trashed
> >    */
> >   build_l3_pagetable:
> > -    /* Find the table index */
> > +    /* Find the first L3 table to be updated */
> > +    lsr x11, x8, #L2_SHIFT
> > +    and x11, x11, #Ln_ADDR_MASK
> > +    lsl x11, x11, #L3_SHIFT
> Why are we shifting right by L2_SHIFT and left with L3_SHIFT? 
> Shouldn't it be L3_SHIFT in both cases?
IMO, L2_shift (21) here is correct.

In wei's design, it uses several continuous 2M memory blocks to create 
the image mapping (Text, bss, data...)
The whole image mapping is created by several L2 table entries, each 
entry links to a L3 table page.
Thus

1. step 1 we need to find the L2 entry index, where the fist L3 table 
locates
+    /* Find the first L3 table to be updated */
+    lsr x11, x8, #L2_SHIFT
+    and x11, x11, #Ln_ADDR_MASK

2.step 2, we need to find the exact L3 table start address by the index 
because L3 tables will occupy
several pages.

That is what below did:

+    lsl x11, x11, #L3_SHIFT

+    add x6, x11, x6

Do you think it is correct ?

---

Cheers,

Justin (Jia He)

> > +    add x6, x11, x6
> > +
> > +    /* Find the first entry index in L3 table */
> >       lsr x11, x8, #L3_SHIFT
> >       and x11, x11, #Ln_ADDR_MASK
> >   @@ -446,10 +452,17 @@ build_l3_pagetable:
> >         /* Store the entry */
> >       str x13, [x6, x11, lsl #3]
> > +    add x11, x11, #1
> >   +    /* Cross L3 Page? */
> > +    and x11, x11, #Ln_ADDR_MASK
> > +    cbnz x11, 2f
> > +
> > +    /* To next L3 table */
> > +    add x6, x6, #__PAGE_SIZE
> > +
> > +2:    add x9, x9, #1
> >       sub x10, x10, #1
> > -    add x11, x11, #1
> > -    add x9, x9, #1
> >       cbnz x10, 1b
> >         ret
> _______________________________________________
> Minios-devel mailing list
> Minios-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/minios-devel
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel