[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Minios-devel] [PATCH v3 28/43] arm64: init the memory system
- To: Huang Shijie <shijie.huang@xxxxxxx>
- From: Julien Grall <julien.grall@xxxxxxx>
- Date: Mon, 30 Apr 2018 11:00:04 +0100
- Cc: jgross@xxxxxxxx, wei.liu2@xxxxxxxxxx, wei.chen@xxxxxxx, steve.capper@xxxxxxx, vlad.babchuk@xxxxxxxxx, minios-devel@xxxxxxxxxxxxxxxxxxxx, kaly.xin@xxxxxxx, samuel.thibault@xxxxxxxxxxxx, baozich@xxxxxxxxx, nd@xxxxxxx
- Delivery-date: Mon, 30 Apr 2018 10:00:15 +0000
- List-id: Mini-os development list <minios-devel.lists.xenproject.org>
Hi Shijie,
On 28/04/18 10:33, Huang Shijie wrote:
On Thu, Apr 26, 2018 at 02:10:01PM +0100, Julien Grall wrote:
Hi,
On 16/04/18 07:32, Huang Shijie wrote:
This patch do followings to initialize the memory system:
0.) Map extra 2M for the first_free_pfn.
What guarantees you there will be free space after the kernel? And why only
2MB?
I add a limit to mini-os (arm64), if the memory is less then 4M, the mini-os
will not run.
2MB is enough to provide pages for setting up the page table.
This does not answer my question. You can have 100MB of memory but still
no free space after the kernel because you don't know what is living in
the memory. So you may end up to write on the DTB or else because you
can't rely of the position of the binary in the memory if that was not
stated in the boot ABI (see linux/Documentation/arm64/booting.txt).
Looking at it: "At least image_size bytes from the start of the image
must be free for use by the kernel.", so you need to find another way to
allocate your page-tables.
As I have already said before, most likely you want to include a couple of
more page-table in the image directly.
I tried, but I donot know how many pages should be preserved in the image
directly.
Depending of the size of the mapping, you only need memory for the
page-tables to map that section. If you use 2MB mapping, only 2 pages
should be sufficient (allocating the L1 and L2 page-table). Then you
will have 2MB worth of memory to use afterwards.
[...]
diff --git a/arch/arm/mm.c b/arch/arm/mm.c
index d98fad8..ed59159 100644
--- a/arch/arm/mm.c
+++ b/arch/arm/mm.c
@@ -6,6 +6,7 @@
#include <mini-os/posix/limits.h>
#include <libfdt.h>
#include <lib.h>
+#include <arm64/pagetable.h>
paddr_t physical_address_offset;
unsigned mem_blocks = 1;
@@ -23,6 +24,236 @@ unsigned long allocate_ondemand(unsigned long n, unsigned
long alignment)
BUG();
}
+extern lpae_t boot_l0_pgtable[512];
+
+static inline void set_pgt_entry(lpae_t *ptr, lpae_t val)
+{
+ *ptr = val;
+ dsb(ishst);
+ isb();
+}
+
+static void build_pte(lpae_t *pmd, unsigned long vaddr, unsigned long vend,
+ paddr_t phys, uint64_t mem_type)
+{
+ lpae_t *pte;
+
+ pte = (lpae_t *)to_virt((*pmd) & ~ATTR_MASK_L) + l3_pgt_idx(vaddr);
+ do {
+ set_pgt_entry(pte, (phys & L3_MASK) | mem_type | L3_PAGE);
+
+ vaddr += L3_SIZE;
+ phys += L3_SIZE;
+ pte++;
+ } while (vaddr < vend);
+}
+
+static int build_pmd(lpae_t *pud, unsigned long vaddr, unsigned long vend,
You are using the term pte, pmd, pud (which basically looks very
Linuxism...) but I have not idea what you are refer to in Mini-OS context.
Please explain it.
sorry, what is the "Mini-OS context" stand for?
I meant what does PMD, PTE, PUD means in Mini-OS?
[...]
+{
+ lpae_t *pud;
+ unsigned long next;
+ int ret;
+
+ pud = (lpae_t *)to_virt((*pgd) & ~ATTR_MASK_L) + l1_pgt_idx(vaddr);
+ do {
+ if (level == 1) {
+ set_pgt_entry(pud, (phys & L1_MASK) | mem_type | L1_BLOCK);
+ } else {
+ next = vaddr + L1_SIZE;
+ if (next > vend)
+ next = vend;
+
+ if ((*pud) == L1_INVAL) {
+ paddr_t newpage = new_page();
+ if (!newpage)
+ return -ENOMEM;
+ set_pgt_entry(pud, newpage | PT_PT);
+ }
+
+ ret = build_pmd(pud, vaddr, next, phys, mem_type, new_page,
level);
+ if (ret)
+ return ret;
+ }
+
+ vaddr += L1_SIZE;
+ phys += L1_SIZE;
+ pud++;
+ } while (vaddr < vend);
+
+ return 0;
+}
+
+static int build_pagetable(unsigned long vaddr, unsigned long start_pfn,
+ unsigned long max_pfn, uint64_t mem_type,
+ paddr_t (*new_page)(void), int level)
Indentation.
Also, this is a bit weird to impose the caller to know the level where it
stops. How do you ensure the virtual and physical address are going to be
aligned properly?
The one who uses build_pagetable() should pay attention to the alignment.
Well, most of your caller does not look at the alignment... and you
don't even have documentation for your function... IHMO, you should have
a wrapper that will do the alignment for you and choose the size of the
section.
[...]
+/*
+ * This function will setup the page table for the memory system.
+ */
+void init_pagetable(unsigned long *start_pfn, unsigned long base,
+ unsigned long size)
+{
+ unsigned long vaddr = (unsigned long)to_virt(base);
+ paddr_t phys = base;
+ paddr_t sz = L1_SIZE;
+ lpae_t *pgd;
+ lpae_t *pud;
+ int level;
+
+ do {
+ /*
+ * We cannot set block mapping for PGD(level 0),
+ * but we can set block mapping for PUD(level 1) and PMD(level 2).
+ * Get the proper level for build_pagetable().
Your API looks wrong, a caller of the PT mapping should not need to know the
The build_pagetable() is only used internally, it is not a API function.
Whether your function is internal or not, the way to use it is very
unintuitive. Someone reading the function have no clue how to use it.
level it is going to map. The only thing necessary is the size of the
mapping.
Please see the init_pagetable()/map_frames_virt()/ioremap, they are API
function.
+ */
+ if (size >= L1_SIZE) {
+ pgd = &boot_l0_pgtable[l0_pgt_idx(vaddr)];
+ if ((*pgd) == L0_INVAL) {
I don't understand this code. Why do you need to check the boot table in
order to now the level to map?
The (*pgd) maybe empty, so we need to check it.
This does not answer why you need to know that when choosing the level.
And that raise the question why this is necessary here and not in the
other place...
diff --git a/arch/arm/setup.c b/arch/arm/setup.c
index 27bea4a..ab82eda 100644
--- a/arch/arm/setup.c
+++ b/arch/arm/setup.c
@@ -29,6 +29,9 @@ void arch_init(void *dtb_pointer, paddr_t physical_offset)
xprintk("Virtual -> physical offset = %"PRIpaddr" \n",
physical_address_offset);
+ /* Do the preparations */
+ arch_mm_preinit(&dtb_pointer);
+
xprintk("Checking DTB at %p...\n", dtb_pointer);
if ((r = fdt_check_header(dtb_pointer))) {
diff --git a/include/arm/arch_mm.h b/include/arm/arch_mm.h
index f77a210..db6e781 100644
--- a/include/arm/arch_mm.h
+++ b/include/arm/arch_mm.h
@@ -3,6 +3,10 @@
typedef uint64_t paddr_t;
#define PRIpaddr "lx"
+#define MIN_MEM_SIZE (0x400000)
Where does this value come from?
I added for the memory limit.
If the memory size is less then 4M, the mini-os will not run.
Why 4MB?
+#define MAX_MEM_SIZE (1UL << 39)
Same here?
Refer to the x86.
Well, I looked at x86 and I can't find MIN_MEM_SIZE and the value for
MAX_MEM_SIZE is 512 << 30. So clearly you need some explanation because
I can't refer to x86 easily nor know why you chose those values.
Cheers,
--
Julien Grall
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
