[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Minios-devel] [PATCH 29/40] arm64: init the memory system before console/xenbus/gic



Hi Shijie,

On 09/11/17 05:33, Huang Shijie wrote:
On Wed, Nov 08, 2017 at 10:38:09AM +0000, Julien Grall wrote:
Hi Shijie,

On 08/11/17 08:54, Huang Shijie wrote:
On Mon, Nov 06, 2017 at 05:06:58PM +0000, Julien Grall wrote:
Hi Shijie,

On 03/11/17 03:12, Huang Shijie wrote:
The mappings for console/xenbus/gic need several pages to setup
the page tables.

So we init the memory system before initiating the console/xenbus/gic,
and then we can allocate the pages which are needed by
the console/xenbus/gic mapping.

we use the 2M memory block for the memory.

Why 2MB and not 1GB mapping when possible? Also, what if the guest is using
2MB is a proper size for most of the memory. For example, we can use 2M
for 512MB, it is not proper to use 1GB mapping.

And you can be clever and mixing 2MB/1GB mapping depending on what you need to 
do.
Yes, we can. but the code looks a little ugly.
I am sorry but avoiding using switch between 1GB/2MB because the code might look ugly is not an excuse.

Xen is able to decide what is the best page size to use (see p2m_set_entry) and the code is readable and fairly clean.

So maybe you should think a bit more how to design it nicely.

[...]


+                     unsigned long max_pfn, long mem_type,
+                     paddr_t (*new_page)(void), int level)
+{
+    paddr_t p_start;
+    unsigned long v_end, next;
+    lpae_t *pgd;
+
+    v_end = vaddr + max_pfn * PAGE_SIZE;
+    p_start = PFN_PHYS(start_pfn);
+
+    /* The boot_l1_pgtable can span 512G address space */
+    pgd = &boot_l1_pgtable[l1_pgt_idx(vaddr)];
+
+    do {
+        next = (vaddr + L1_SIZE);
+        if (next > v_end)
+            next = v_end;
+
+        if ((*pgd) == L1_INVAL) {
+            set_pgt_entry(pgd, (new_page()) | PT_PT);

What if new_page() fails?
It will call the BUG() when it fails, please see alloc_new_page()..

I looked at the only caller in this patch and there are no BUG()...

So your interface is already wrong (see below). But a BUG() in alloc_new_page()
seem pretty odd. You want to let the caller decides what to do if the allocation
fails? In fact even the x86 do return an error on allocation failure. So why not
Arm?
okay, I will add the return check for it...


+        }
+
+        build_pud(pgd, vaddr, next, p_start, mem_type, new_page, level);
+        p_start += next - vaddr;
+        vaddr = next;
+        pgd++;
+    } while (vaddr != v_end);
+}

The code you implemented for page-table looks far too simple. For instance,
I am surprised there are no TLB maintenance anywhere.
We can add the TLB maintenance in future in the places it needed.

When is that future? As I said previously, page-table are going to be used
in multiple difference places. Implementing the page-table correctly from
the beginning does not hurt and will save time later on for all of us later
on.
  1.) we flush the TLB in the _start.
  2.) We setup the table after 1.)
  3.) we will not change the table in future.

I still don't understand why you keep saying table will not change in the future when I gave you proof of the invert...

We are going to need to modify page-table in near future (for instance for PV drivers/libc).


  Why do we need the TLB maintainable?

Well, you already have mapping in the page-tables and may end up to replace them. For instance, you already have a bit of RAM already mapped and likely going to rewrite the PTE for them.

Are they going to be exactly the same value? Will you shatter some mappings?

If TLB maintainance is not necessary. Then documentation in the code explaining why is always helpful for developer looking at the code later on.


+
+static unsigned long first_free_pfn;

Likely this variable and get_new_page needs more explanation in the code.
okay, I stole this code from the x86 in mini-os.

Sorry but I can't find this code in x86... The variable is here but seem to
be used differently. Would you mind giving a pointer?
in arch/x86/mm.c, search first_free_pfn.

Have you read what I wrote? I am perfectly happy to explain if you don't understand something, but it is annoying me to see you don't seen to bother considering what I say. It is already the second time in this e-mail...

I know where the first_free_pfn is used in x86 code... My point was that the usage is completely different from here. It makes sense over there, here it looks quite hackish.

In any case, what works for x86 does not mean this is the best way to do it...




+static paddr_t get_new_page(void)

I think it would be better if you had early (or something similar) in the
name. To make clear that is only used for early boot.
What's about get_new_page_early or get_new_page_boot?

I would prefer early_alloc_page()
okay.



+{
+    paddr_t new_page;
+
+    memset(pfn_to_virt(first_free_pfn), 0, PAGE_SIZE);

This is quite confusing, get_new_page() allocates page-table that
will be used to map the RAM. So how do you bootstrap it?
sorry, I do not quite understand your meaning..

Do you worry about the virtual address of first_free_pfn?

Yes. Where does the mapping between first_free_pfn and its virtual address
is done?
In the __setup_initial_pgtable.

Really? AFAICT, __setup_initial_pgtable will only map the kernel and DT. You can't assume there are will be free RAM mapped.

Cheers,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.