Re: [Xen-devel] [PATCH] xen/arm: skip first page when RAM starts at 0x0

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 26 Apr 2019, Julien Grall wrote:
> On 4/26/19 4:48 PM, Jan Beulich wrote:
> > > > > On 26.04.19 at 17:38, <julien.grall@xxxxxxx> wrote:
> > > On 26/04/2019 10:42, Jan Beulich wrote:
> > > > > > > On 26.04.19 at 11:19, <Julien.Grall@xxxxxxx> wrote:
> > > > > So how does the PDX work for memory below 4GB? Do you still call
> > > > > pfn_to_pdx or those pages?
> > > > 
> > > > Of course. We merely never compress any of the low 32 address
> > > > bits, i.e. our choice is limited to address bits 32 ... 51.
> > > 
> > > Ah, the code makes a bit more sense on the x86 side. Is there any reason
> > > to
> > > limit to address bit 32 .. 51?
> > 
> > Well, there being various special things immediately below 4Gb
> > there's simply little point in trying to squash any of the lower bits.
> > 
> > > For Arm, we can't compress bits 0 ... 30 due to the buddy allocator (see
> > > pfn_pdx_hole_setup). So we could ignore the first 1GB of RAM.
> > 
> > Bits 0 ... 30 would be the first 2Gb afaict.
> 
> Sorry I messed us my maths. I meant 0 ... 29 (MAX_ORDER = 18).
> 
> @Stefano, do you think you can have a look at it?

After an extensive online debugging session with Julien, we found a fix
to the PDX code that makes the masks and nr_pdxs calculation correct.
See the appended patch below.

Mask is initialized to 0 to the let the algorithm compute the right mask
based on the RAM banks. pdx_init_mask doesn't cope with a start address
of 0x0 because it does `base_addr - 1' which causes a wrap around when
base_addr is 0x0. That works OK as far as I can tell and the resulting
values of `pfn_pdx_bottom_mask', `pfn_top_mask', and
`pfn_pdx_hole_shift' seems correct (they match the case where I force
ram_start = PAGE_SIZE).

The other change to nr_pdxs is less obvious. It is clear that nr_pdxs is
calculated wrongly in this case (memory 0-0x80000000,
0x800000000-0x880000000, ps=0, pe=0x880000000): nr_pdxs=524288 which is
half the number we need (the correct number is 1048575).

Taking a line from the x86 code xen/arch/x86/setup.c:setup_max_pdx
(Julien's suggestion):

  max_pdx = pfn_to_pdx(top_page - 1) + 1;

I changed nr_pdxs to
  
  nr_pdxs = pfn_to_pdx((pe >> PAGE_SHIFT) - 1) + 1;

and it works. I think the change is correct because looking at the
implementation of pfn_to_pdx it is certainly meant to operate on a pfn
masking bits on it to compensate for the holes. It is not meant to work
on a size.

Jan, does it look correct to you too?


diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 01ae2cc..f711eef 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -874,8 +874,7 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
 /* Map a frame table to cover physical addresses ps through pe */
 void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
 {
-    unsigned long nr_pages = (pe - ps) >> PAGE_SHIFT;
-    unsigned long nr_pdxs = pfn_to_pdx(nr_pages);
+    unsigned long nr_pdxs = pfn_to_pdx((pe >> PAGE_SHIFT) - 1) + 1;
     unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
     mfn_t base_mfn;
     const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) : 
MB(32);
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index ccb0f18..9e7da41 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -482,7 +482,7 @@ static void __init init_pdx(void)
 {
     paddr_t bank_start, bank_size, bank_end;
 
-    u64 mask = pdx_init_mask(bootinfo.mem.bank[0].start);
+    u64 mask = 0;
     int bank;
 
     for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ )

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