RE: [PATCH v2 3/4] xen/arm: Handle reserved heap pages in boot and heap allocator

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> > I agree we need to require 64KB alignment, and currently we are following
> > this because we are doing 32MB alignment.
> Hmmm... I think we are talking about two different things here. What I
> am referring to is the alignment of the start/end of the region provided
> by the admin.

Ahh sorry my bad, yeah I should add the alignment requirement in dt-binding.
Will do that in v3.

> 
> [...]
> 
> >> Note that to handle arm32, you will also need to exclude the xenheap
> area.
> >
> > When I implement the code, I found that the arm32 Xenheap excluding
> logic
> > somehow can be reused.
> >
> > So I think I tried to reuse as much as current code. Would below
> > populate_boot_allocator() seem ok to you?
> 
> I would prefer if they are separate because the logic can be simplified
> when using the static heap (the xenheap cannot across a region).

I think whether separate this logic or not is personal taste, I did miss
the "xenheap cannot across the bank" part so I agree your suggestion
is better, but I think...

> 
> Something like:
> 
> for ( i = 0; i < banks->nr_banks; i++ )
> {
> 
> #ifdef CONFIG_ARM_32
>      if ( (bank_start >= mfn_to_maddr(direct_mfn_start) &&
>             bank_end < mfn_to_maddr(direct_mfn_start) )

... this is probably wrong or I misunderstood? IMHO the xenheap
is always smaller (or equal) than the bank, based on the logic that
finding a contiguous xenheap in a bank.

So the code I propose would be:
```
@@ -712,12 +712,37 @@ static void __init populate_boot_allocator(void)
 {
     unsigned int i;
     const struct meminfo *banks = &bootinfo.mem;
+    paddr_t s, e;
+
+    if ( bootinfo.static_heap )
+    {
+        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
+        {
+            if ( bootinfo.reserved_mem.bank[i].type != MEMBANK_STATIC_HEAP )
+                continue;
+
+            s = bootinfo.reserved_mem.bank[i].start;
+            e = s + bootinfo.reserved_mem.bank[i].size;
+#ifdef CONFIG_ARM_32
+            /* Avoid the xenheap, note that the xenheap cannot across a bank */
+            if ( s <= mfn_to_maddr(directmap_mfn_start) &&
+                 e >= mfn_to_maddr(directmap_mfn_end) )
+            {
+                init_boot_pages(s, mfn_to_maddr(directmap_mfn_start));
+                init_boot_pages(mfn_to_maddr(directmap_mfn_end), e);
+            }
+            else
+#endif
+                init_boot_pages(s, e);
+        }
+
+        return;
+    }

     for ( i = 0; i < banks->nr_banks; i++ )
     /* The original logic in populate_boot_allocator()*/
```

Kind regards,
Henry