Re: [Minios-devel] [UNIKRAFT PATCHv4 29/43] plat/kvm: Enable MMU for Arm64

[Julien Grall <julien.grall@xxxxxxx>]

Hi Wei,

On 18/07/18 09:14, Wei Chen wrote:
> > > +
> > > +#define TCR_T1SZ_SHIFT 16
> > > +#define TCR_T0SZ_SHIFT 0
> > > +#define TCR_T1SZ(x)    ((x) << TCR_T1SZ_SHIFT)
> > > +#define TCR_T0SZ(x)    ((x) << TCR_T0SZ_SHIFT)
> > > +#define TCR_TxSZ(x)    (TCR_T1SZ(x) | TCR_T0SZ(x))
> > > +
> > > +#define TCR_INIT_FLAGS (TCR_TxSZ(64 - VIRT_BITS) | TCR_ASID_16 | \
> > > +                       TCR_TG0_4K | TCR_CACHE_ATTRS | TCR_SMP_ATTRS)
> > > +
> > > +/* SCTLR_EL1 - System Control Register */
> > > +#define SCTLR_RES0     0xc8222400      /* Reserved ARMv8.0, write 0 */
> > > +#define SCTLR_RES1     0x30d00800      /* Reserved ARMv8.0, write 1 */
> > You don't seem to use those two defines. So I would drop them.
> Yes, currently, I haven't used them. I would drop them.
>
> > > +
> > > +#define SCTLR_M                (_AC(1, UL) << 0)
> > > +#define SCTLR_A                (_AC(1, UL) << 1)
> > > +#define SCTLR_C                (_AC(1, UL) << 2)
> > > +#define SCTLR_SA       (_AC(1, UL) << 3)
> > > +#define SCTLR_SA0      (_AC(1, UL) << 4)
> > > +#define SCTLR_CP15BEN  (_AC(1, UL) << 5)
> > > +#define SCTLR_THEE     (_AC(1, UL) << 6)
> > I can't find this bit in the latest ARM ARM (0487C.a).
> You can find it from here, Reserve0
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0500e/BABJAHDA.html
That's the technical reference for the Cortex-A53 and your link point to 
the 32-bit SCTLR. It seems that is also defined in 64-bit SCTLR.
However, this bit has no name in the ARM ARM, so I would rather not give 
a name here because it may be re-purposed in the future.
[...]

> > >         /* Load dtb address to x0 as a parameter */
> > >         ldr x0, =_dtb
> > > diff --git a/plat/kvm/arm/pagetable.S b/plat/kvm/arm/pagetable.S
> > > index 8de6305..c3bb85b 100644
> > > --- a/plat/kvm/arm/pagetable.S
> > > +++ b/plat/kvm/arm/pagetable.S
> > > @@ -181,6 +181,43 @@ ENTRY(create_pagetables)
> > >         ret
> > >    END(create_pagetables)
> > >
> > > +ENTRY(start_mmu)
> > > +       dsb sy
> > What's this DSB for?
> Guarantee the create_pagetables has been done before start mmu.
Can you add a comment on top explaining it?

Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel