Re: [Minios-devel] [UNIKRAFT PATCHv5 15/46] plat/include: Define address offsets of boot stack and pagetable

["Wei Chen (Arm Technology China)" <Wei.Chen@xxxxxxx>]

Hi Simon,

> -----Original Message-----
> From: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> Sent: 2018年9月7日 17:29
> To: Wei Chen (Arm Technology China) <Wei.Chen@xxxxxxx>; minios-
> devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [Minios-devel] [UNIKRAFT PATCHv5 15/46] plat/include: Define
> address offsets of boot stack and pagetable
> 
> On 07.09.2018 07:14, Wei Chen (Arm Technology China) wrote:
> > Hi Simon,
> >
> >> -----Original Message-----
> >> From: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> >> Sent: 2018年9月6日 22:53
> >> To: Wei Chen (Arm Technology China) <Wei.Chen@xxxxxxx>; minios-
> >> devel@xxxxxxxxxxxxxxxxxxxx
> >> Cc: Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> >> Subject: Re: [Minios-devel] [UNIKRAFT PATCHv5 15/46] plat/include: Define
> >> address offsets of boot stack and pagetable
> >>
> >> Hey Wei,
> >>
> >> These defines for the memory layout are specific for KVM for now, right?
> >> Wouldn't it then make sense to place this files to plat/kvm/include?
> >> Or do you know if this is going to be the same for Xen?
> >>
> >
> > I want to use the same memory layout for KVM and Xen. I know that current
> > Code for Xen platform is ported from mini-os. But once, when code for
> > Arm/KVM becomes stable, I want reuse most of the code for these two
> > platforms.
> 
> Okay, sounds reasonable. Could you add this as one sentence in the
> commit message to eplain why you decided to place this to common/?
> 

Ok, I will add similar comment in the commit message.

> >
> >> On 10.08.2018 09:08, Wei Chen wrote:
> >>> From: Wei Chen <Wei.Chen@xxxxxxx>
> >>>
> >>> If we place the boot stack and pagetable in BSS section. These
> >>> areas are not easy to be reused after changing to new stack.
> >>> So, in Arm64, we want to place the pagetable and boot stack
> >>> after the end of image. In this case, once we change to new
> >>> stack or we have new pagetable, these two areas can be reclaimed
> >>> very easy.
> >>>
> >>> Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> >>> ---
> >>>    plat/common/include/arm/arm64/mm.h | 86 ++++++++++++++++++++++++++++++
> >>>    plat/common/include/arm/mm.h       | 44 +++++++++++++++
> >>>    plat/common/include/mm.h           | 44 +++++++++++++++
> >>>    3 files changed, 174 insertions(+)
> >>>    create mode 100644 plat/common/include/arm/arm64/mm.h
> >>>    create mode 100644 plat/common/include/arm/mm.h
> >>>    create mode 100644 plat/common/include/mm.h
> >>>
> >>> diff --git a/plat/common/include/arm/arm64/mm.h
> >> b/plat/common/include/arm/arm64/mm.h
> >>> new file mode 100644
> >>> index 0000000..3c1db27
> >>> --- /dev/null
> >>> +++ b/plat/common/include/arm/arm64/mm.h
> >>> @@ -0,0 +1,86 @@
> >>> +/* SPDX-License-Identifier: BSD-3-Clause */
> >>> +/*
> >>> + * Authors: Wei Chen <wei.chen@xxxxxxx>
> >>> + *
> >>> + * Copyright (c) 2018, Arm Ltd. All rights reserved.
> >>> + *
> >>> + * Redistribution and use in source and binary forms, with or without
> >>> + * modification, are permitted provided that the following conditions
> >>> + * are met:
> >>> + *
> >>> + * 1. Redistributions of source code must retain the above copyright
> >>> + *    notice, this list of conditions and the following disclaimer.
> >>> + * 2. Redistributions in binary form must reproduce the above copyright
> >>> + *    notice, this list of conditions and the following disclaimer in the
> >>> + *    documentation and/or other materials provided with the 
> >>> distribution.
> >>> + * 3. Neither the name of the copyright holder nor the names of its
> >>> + *    contributors may be used to endorse or promote products derived
> from
> >>> + *    this software without specific prior written permission.
> >>> + *
> >>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> "AS
> >> IS"
> >>> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> >> THE
> >>> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> >> PURPOSE
> >>> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS
> >> BE
> >>> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> >>> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> >>> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> BUSINESS
> >>> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER
> IN
> >>> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR 
> >>> OTHERWISE)
> >>> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
> >> THE
> >>> + * POSSIBILITY OF SUCH DAMAGE.
> >>> + *
> >>> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> >>> + */
> >>> +
> >>> +#ifndef __CPU_ARM_64_MM_H__
> >>> +#define __CPU_ARM_64_MM_H__
> >>> +
> >>> +/*
> >>> + * We will place the pagetable and boot stack after image area,
> >>> + * So we define the address offset of pagetable and boot stack
> >>> + * here.
> >>> + */
> >>
> >> This is a general comment, right? For now it looks like it would explain
> >> the following block. Could you add an empty line afterwards?
> >>
> >
> > Yes, I will add an empty line.
> >
> >>> +#define PAGE_SIZE  __PAGE_SIZE
> >>> +#define PAGE_SHIFT __PAGE_SHIFT
> >>> +#define STACK_SIZE __STACK_SIZE
> >>
> >> Do you require these renamings or could you use the ukarch __* variants
> >> directly?
> >>
> >
> > It's just a habit, I think I can use ukarch __* variants directly, if this
> > is uniform style of Unikraft : )
> >
> 
> It is actually just a little detail. I would take your patch with or
> without the change. I just do not like this renaming of the macros
> because it looks like a source for confusion later.
> 

OK, I will used the __* variants directly ; )

> >>> +
> >>> +/*
> >>> + * Each entry in L0_TABLE can link to a L1_TABLE which supports 512GiB
> >>> + * memory mapping. One 4K page can provide 512 entries. In this case,
> >>> + * one page for L0_TABLE is enough for current stage.
> >>> + */
> >>> +#define L0_TABLE_OFFSET 0
> >>> +#define L0_TABLE_SIZE   PAGE_SIZE
> >>> +
> >>> +/*
> >>> + * Each entry in L1_TABLE can map to a 1GiB memory or link to a
> >>> + * L2_TABLE which supports 1GiB memory mapping. One 4K page can provide
> >>> + * 512 entries. We need at least 2 pages to support 1TB memory space
> >>> + * for platforms like KVM QEMU virtual machine.
> >>> + */
> >>> +#define L1_TABLE_OFFSET (L0_TABLE_OFFSET + L0_TABLE_SIZE)
> >>> +#define L1_TABLE_SIZE   (PAGE_SIZE * 2)
> >>> +
> >>> +/*
> >>> + * Each entry in L2_TABLE can map to a 2MiB block memory or link to a
> >>> + * L3_TABLE which supports 2MiB memory mapping. We need a L3_TABLE to
> >>> + * cover image area for us to manager different sections attributes.
> >>> + * So, we need one page for L2_TABLE to provide 511 enties for 2MiB
> >>> + * block mapping and 1 entry for L3_TABLE link.
> >>> + */
> >>> +#define L2_TABLE_OFFSET (L1_TABLE_OFFSET + L1_TABLE_SIZE)
> >>> +#define L2_TABLE_SIZE   PAGE_SIZE
> >>> +
> >>> +/*
> >>> + * As Unikraft image's size is very tiny, from tens to hundreds kilo
> >>> + * bytes. So one page for L3_TABLE is enough for us to manage section
> >>> + * attributes of image.
> >>> + */
> >>> +#define L3_TABLE_OFFSET (L2_TABLE_OFFSET + L2_TABLE_SIZE)
> >>> +#define L3_TABLE_SIZE   PAGE_SIZE
> >>> +
> >>> +/* Total memory size that will be used by pagetable */
> >>> +#define PAGE_TABLE_SIZE (L0_TABLE_SIZE + L1_TABLE_SIZE + \
> >>> +                         L2_TABLE_SIZE + L3_TABLE_SIZE)
> >>> +
> >>> +#endif /* __CPU_ARM_64_MM_H__ */
> >>> diff --git a/plat/common/include/arm/mm.h b/plat/common/include/arm/mm.h
> >>> new file mode 100644
> >>> index 0000000..0f5db19
> >>> --- /dev/null
> >>> +++ b/plat/common/include/arm/mm.h
> >>> @@ -0,0 +1,44 @@
> >>> +/* SPDX-License-Identifier: BSD-3-Clause */
> >>> +/*
> >>> + * Authors: Wei Chen <wei.chen@xxxxxxx>
> >>> + *
> >>> + * Copyright (c) 2018, Arm Ltd. All rights reserved.
> >>> + *
> >>> + * Redistribution and use in source and binary forms, with or without
> >>> + * modification, are permitted provided that the following conditions
> >>> + * are met:
> >>> + *
> >>> + * 1. Redistributions of source code must retain the above copyright
> >>> + *    notice, this list of conditions and the following disclaimer.
> >>> + * 2. Redistributions in binary form must reproduce the above copyright
> >>> + *    notice, this list of conditions and the following disclaimer in the
> >>> + *    documentation and/or other materials provided with the 
> >>> distribution.
> >>> + * 3. Neither the name of the copyright holder nor the names of its
> >>> + *    contributors may be used to endorse or promote products derived
> from
> >>> + *    this software without specific prior written permission.
> >>> + *
> >>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> "AS
> >> IS"
> >>> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> >> THE
> >>> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> >> PURPOSE
> >>> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS
> >> BE
> >>> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> >>> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> >>> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> BUSINESS
> >>> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER
> IN
> >>> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR 
> >>> OTHERWISE)
> >>> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
> >> THE
> >>> + * POSSIBILITY OF SUCH DAMAGE.
> >>> + *
> >>> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> >>> + */
> >>> +
> >>> +#ifndef __PLAT_CMN_ARM_MM_H__
> >>> +#define __PLAT_CMN_ARM_MM_H__
> >>> +
> >>> +#if defined(__ARM_64__)
> >>> +#include "arm64/mm.h"
> >>> +#else
> >>> +#error "Add cpu_defs.h for current architecture."
> >>
> >> I think it should be called "mm.h"
> >>
> >
> > Yes, it's a typo, I will fix it.
> >
> >>> +#endif
> >>> +
> >>> +#endif /* __PLAT_CMN_ARM_MM_H__ */
> >>> diff --git a/plat/common/include/mm.h b/plat/common/include/mm.h
> >>> new file mode 100644
> >>> index 0000000..c3a37f5
> >>> --- /dev/null
> >>> +++ b/plat/common/include/mm.h
> >>> @@ -0,0 +1,44 @@
> >>> +/* SPDX-License-Identifier: BSD-3-Clause */
> >>> +/*
> >>> + * Authors: Wei Chen <wei.chen@xxxxxxx>
> >>> + *
> >>> + * Copyright (c) 2018, Arm Ltd. All rights reserved.
> >>> + *
> >>> + * Redistribution and use in source and binary forms, with or without
> >>> + * modification, are permitted provided that the following conditions
> >>> + * are met:
> >>> + *
> >>> + * 1. Redistributions of source code must retain the above copyright
> >>> + *    notice, this list of conditions and the following disclaimer.
> >>> + * 2. Redistributions in binary form must reproduce the above copyright
> >>> + *    notice, this list of conditions and the following disclaimer in the
> >>> + *    documentation and/or other materials provided with the 
> >>> distribution.
> >>> + * 3. Neither the name of the copyright holder nor the names of its
> >>> + *    contributors may be used to endorse or promote products derived
> from
> >>> + *    this software without specific prior written permission.
> >>> + *
> >>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> "AS
> >> IS"
> >>> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> >> THE
> >>> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> >> PURPOSE
> >>> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS
> >> BE
> >>> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> >>> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> >>> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> BUSINESS
> >>> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER
> IN
> >>> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR 
> >>> OTHERWISE)
> >>> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
> >> THE
> >>> + * POSSIBILITY OF SUCH DAMAGE.
> >>> + *
> >>> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> >>> + */
> >>> +
> >>> +#ifndef __PLAT_CMN_MM_H__
> >>> +#define __PLAT_CMN_MM_H__
> >>> +
> >>> +#if defined(__ARM_64__)
> >>> +#include <arm/mm.h>
> >>> +#else
> >>> +#error "Add cpu.h for current architecture."
> >>
> >> Should also be called mm.h, probably.
> >>
> >
> > Yes.
> >
> >>> +#endif
> >>> +
> >>> +#endif /* __PLAT_CMN_MM_H__ */
> >>>
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel