[PATCH] minios: don't rely on specific page table allocation scheme

[PATCH] minios: don't rely on specific page table allocation scheme

[Juergen Gross]

Today mini-os is making assumptions how the page tables it is started
with are being allocated. Especially it is using the number of page
table frames to calculate which is the first unmapped pfn.

Instead of relying on page table number assumptions just look into the
page tables to find the first pfn not already mapped.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/mm.c         | 47 ++++++++++++++++++++++++++++++++++++-----------
 include/x86/arch_mm.h |  7 -------
 2 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/arch/x86/mm.c b/arch/x86/mm.c
index 9c6d1b8..5d7c006 100644
--- a/arch/x86/mm.c
+++ b/arch/x86/mm.c
@@ -200,8 +200,8 @@ static void build_pagetable(unsigned long *start_pfn, unsigned long *max_pfn)
     int count = 0;
     int rc;
 
-    pfn_to_map = 
-        (start_info.nr_pt_frames - NOT_L1_FRAMES) * L1_PAGETABLE_ENTRIES;
+    pfn_to_map = (*start_pfn + L1_PAGETABLE_ENTRIES - 1) &
+                 ~(L1_PAGETABLE_ENTRIES - 1);
 
     if ( *max_pfn >= virt_to_pfn(HYPERVISOR_VIRT_START) )
     {
@@ -229,9 +229,15 @@ static void build_pagetable(unsigned long *start_pfn, unsigned long *max_pfn)
 #if defined(__x86_64__)
         offset = l4_table_offset(start_address);
         /* Need new L3 pt frame */
-        if ( !(start_address & L3_MASK) )
-            if ( need_pt_frame(start_address, L3_FRAME) ) 
-                new_pt_frame(&pt_pfn, pt_mfn, offset, L3_FRAME);
+        if ( !(tab[offset] & _PAGE_PRESENT) )
+        {
+            if ( !need_pt_frame(start_address, L3_FRAME) )
+            {
+                printk("ERROR: build_pagetable(): L3 frame not present\n");
+                do_exit();
+            }
+            new_pt_frame(&pt_pfn, pt_mfn, offset, L3_FRAME);
+        }
 
         page = tab[offset];
         pt_mfn = pte_to_mfn(page);
@@ -239,18 +245,37 @@ static void build_pagetable(unsigned long *start_pfn, unsigned long *max_pfn)
 #endif
         offset = l3_table_offset(start_address);
         /* Need new L2 pt frame */
-        if ( !(start_address & L2_MASK) )
-            if ( need_pt_frame(start_address, L2_FRAME) )
-                new_pt_frame(&pt_pfn, pt_mfn, offset, L2_FRAME);
+        if ( !(tab[offset] & _PAGE_PRESENT) )
+        {
+            if ( !need_pt_frame(start_address, L2_FRAME) )
+            {
+                printk("ERROR: build_pagetable(): L2 frame not present\n");
+                do_exit();
+            }
+            new_pt_frame(&pt_pfn, pt_mfn, offset, L2_FRAME);
+        }
 
         page = tab[offset];
         pt_mfn = pte_to_mfn(page);
         tab = to_virt(mfn_to_pfn(pt_mfn) << PAGE_SHIFT);
         offset = l2_table_offset(start_address);        
         /* Need new L1 pt frame */
-        if ( !(start_address & L1_MASK) )
-            if ( need_pt_frame(start_address, L1_FRAME) )
-                new_pt_frame(&pt_pfn, pt_mfn, offset, L1_FRAME);
+        if ( !(tab[offset] & _PAGE_PRESENT) )
+        {
+            if ( !need_pt_frame(start_address, L1_FRAME) )
+            {
+                printk("ERROR: build_pagetable(): L1 frame not present\n");
+                do_exit();
+            }
+            new_pt_frame(&pt_pfn, pt_mfn, offset, L1_FRAME);
+        }
+        else if ( !(start_address & L1_MASK) )
+        {
+            /* Already mapped, skip this L1 entry. */
+            start_address += L1_PAGETABLE_ENTRIES << PAGE_SHIFT;
+            pfn_to_map += L1_PAGETABLE_ENTRIES;
+            continue;
+        }
 
         page = tab[offset];
         pt_mfn = pte_to_mfn(page);
diff --git a/include/x86/arch_mm.h b/include/x86/arch_mm.h
index 23cfca7..58f29fc 100644
--- a/include/x86/arch_mm.h
+++ b/include/x86/arch_mm.h
@@ -56,12 +56,6 @@
 
 #define L2_MASK  ((1UL << L3_PAGETABLE_SHIFT) - 1)
 
-/*
- * If starting from virtual address greater than 0xc0000000,
- * this value will be 2 to account for final mid-level page
- * directory which is always mapped in at this location.
- */
-#define NOT_L1_FRAMES           3
 #define PRIpte "016llx"
 #ifndef __ASSEMBLY__
 typedef uint64_t pgentry_t;
@@ -87,7 +81,6 @@ typedef uint64_t pgentry_t;
 #define L2_MASK  ((1UL << L3_PAGETABLE_SHIFT) - 1)
 #define L3_MASK  ((1UL << L4_PAGETABLE_SHIFT) - 1)
 
-#define NOT_L1_FRAMES           3
 #define PRIpte "016lx"
 #ifndef __ASSEMBLY__
 typedef unsigned long pgentry_t;
-- 
2.6.2

Re: [PATCH] minios: don't rely on specific page table allocation scheme

[Wei Liu]

On Thu, Nov 19, 2015 at 05:05:16PM +0100, Juergen Gross wrote:
> Today mini-os is making assumptions how the page tables it is started
> with are being allocated. Especially it is using the number of page
> table frames to calculate which is the first unmapped pfn.
> 
> Instead of relying on page table number assumptions just look into the
> page tables to find the first pfn not already mapped.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

I ran both pv-grub-x86_{64,32} up to the point when grub menu was
shown -- That's beyond the point of the original crash.

I also ran mini-os 32 and 64 bit with this patch on toolstack without
large p2m series. They worked.

> ---
>  arch/x86/mm.c         | 47 ++++++++++++++++++++++++++++++++++++-----------
>  include/x86/arch_mm.h |  7 -------
>  2 files changed, 36 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/mm.c b/arch/x86/mm.c
> index 9c6d1b8..5d7c006 100644
> --- a/arch/x86/mm.c
> +++ b/arch/x86/mm.c
> @@ -200,8 +200,8 @@ static void build_pagetable(unsigned long *start_pfn, unsigned long *max_pfn)
>      int count = 0;
>      int rc;
>  
> -    pfn_to_map = 
> -        (start_info.nr_pt_frames - NOT_L1_FRAMES) * L1_PAGETABLE_ENTRIES;
> +    pfn_to_map = (*start_pfn + L1_PAGETABLE_ENTRIES - 1) &
> +                 ~(L1_PAGETABLE_ENTRIES - 1);
>  
>      if ( *max_pfn >= virt_to_pfn(HYPERVISOR_VIRT_START) )
>      {
> @@ -229,9 +229,15 @@ static void build_pagetable(unsigned long *start_pfn, unsigned long *max_pfn)
>  #if defined(__x86_64__)
>          offset = l4_table_offset(start_address);
>          /* Need new L3 pt frame */
> -        if ( !(start_address & L3_MASK) )
> -            if ( need_pt_frame(start_address, L3_FRAME) ) 
> -                new_pt_frame(&pt_pfn, pt_mfn, offset, L3_FRAME);
> +        if ( !(tab[offset] & _PAGE_PRESENT) )
> +        {
> +            if ( !need_pt_frame(start_address, L3_FRAME) )
> +            {
> +                printk("ERROR: build_pagetable(): L3 frame not present\n");
> +                do_exit();
> +            }
> +            new_pt_frame(&pt_pfn, pt_mfn, offset, L3_FRAME);

I'm not sure about this hunk and other similar hunks. Shouldn't this be

               if ( need_pt_frame(start_address, L3_FRAME) )
	           new_pt_frame(&pt_pfn, pt_mfn, offset, L3_FRAME);

That is, just like before.


Wei.

Re: [PATCH] minios: don't rely on specific page table allocation scheme

[Juergen Gross]

On 20/11/15 13:46, Wei Liu wrote:
> On Thu, Nov 19, 2015 at 05:05:16PM +0100, Juergen Gross wrote:
>> Today mini-os is making assumptions how the page tables it is started
>> with are being allocated. Especially it is using the number of page
>> table frames to calculate which is the first unmapped pfn.
>>
>> Instead of relying on page table number assumptions just look into the
>> page tables to find the first pfn not already mapped.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> I ran both pv-grub-x86_{64,32} up to the point when grub menu was
> shown -- That's beyond the point of the original crash.
> 
> I also ran mini-os 32 and 64 bit with this patch on toolstack without
> large p2m series. They worked.
> 
>> ---
>>  arch/x86/mm.c         | 47 ++++++++++++++++++++++++++++++++++++-----------
>>  include/x86/arch_mm.h |  7 -------
>>  2 files changed, 36 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/x86/mm.c b/arch/x86/mm.c
>> index 9c6d1b8..5d7c006 100644
>> --- a/arch/x86/mm.c
>> +++ b/arch/x86/mm.c
>> @@ -200,8 +200,8 @@ static void build_pagetable(unsigned long *start_pfn, unsigned long *max_pfn)
>>      int count = 0;
>>      int rc;
>>  
>> -    pfn_to_map = 
>> -        (start_info.nr_pt_frames - NOT_L1_FRAMES) * L1_PAGETABLE_ENTRIES;
>> +    pfn_to_map = (*start_pfn + L1_PAGETABLE_ENTRIES - 1) &
>> +                 ~(L1_PAGETABLE_ENTRIES - 1);
>>  
>>      if ( *max_pfn >= virt_to_pfn(HYPERVISOR_VIRT_START) )
>>      {
>> @@ -229,9 +229,15 @@ static void build_pagetable(unsigned long *start_pfn, unsigned long *max_pfn)
>>  #if defined(__x86_64__)
>>          offset = l4_table_offset(start_address);
>>          /* Need new L3 pt frame */
>> -        if ( !(start_address & L3_MASK) )
>> -            if ( need_pt_frame(start_address, L3_FRAME) ) 
>> -                new_pt_frame(&pt_pfn, pt_mfn, offset, L3_FRAME);
>> +        if ( !(tab[offset] & _PAGE_PRESENT) )
>> +        {
>> +            if ( !need_pt_frame(start_address, L3_FRAME) )
>> +            {
>> +                printk("ERROR: build_pagetable(): L3 frame not present\n");
>> +                do_exit();
>> +            }
>> +            new_pt_frame(&pt_pfn, pt_mfn, offset, L3_FRAME);
> 
> I'm not sure about this hunk and other similar hunks. Shouldn't this be
> 
>                if ( need_pt_frame(start_address, L3_FRAME) )
> 	           new_pt_frame(&pt_pfn, pt_mfn, offset, L3_FRAME);
> 
> That is, just like before.

I didn't want to keep it like before. What sense does it make to
find a page table is not existing and to decide it don't need to be
created as it should exist already and then to carry on as if it would
be there? The only viable alternative would have been to nuke the
need_pt_frame() call.

Juergen

Re: [PATCH] minios: don't rely on specific page table allocation scheme

[Wei Liu]

On Fri, Nov 20, 2015 at 01:55:29PM +0100, Juergen Gross wrote:
> On 20/11/15 13:46, Wei Liu wrote:
> > On Thu, Nov 19, 2015 at 05:05:16PM +0100, Juergen Gross wrote:
> >> Today mini-os is making assumptions how the page tables it is started
> >> with are being allocated. Especially it is using the number of page
> >> table frames to calculate which is the first unmapped pfn.
> >>
> >> Instead of relying on page table number assumptions just look into the
> >> page tables to find the first pfn not already mapped.
> >>
> >> Signed-off-by: Juergen Gross <jgross@suse.com>
> > 
> > I ran both pv-grub-x86_{64,32} up to the point when grub menu was
> > shown -- That's beyond the point of the original crash.
> > 
> > I also ran mini-os 32 and 64 bit with this patch on toolstack without
> > large p2m series. They worked.
> > 
> >> ---
> >>  arch/x86/mm.c         | 47 ++++++++++++++++++++++++++++++++++++-----------
> >>  include/x86/arch_mm.h |  7 -------
> >>  2 files changed, 36 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/arch/x86/mm.c b/arch/x86/mm.c
> >> index 9c6d1b8..5d7c006 100644
> >> --- a/arch/x86/mm.c
> >> +++ b/arch/x86/mm.c
> >> @@ -200,8 +200,8 @@ static void build_pagetable(unsigned long *start_pfn, unsigned long *max_pfn)
> >>      int count = 0;
> >>      int rc;
> >>  
> >> -    pfn_to_map = 
> >> -        (start_info.nr_pt_frames - NOT_L1_FRAMES) * L1_PAGETABLE_ENTRIES;
> >> +    pfn_to_map = (*start_pfn + L1_PAGETABLE_ENTRIES - 1) &
> >> +                 ~(L1_PAGETABLE_ENTRIES - 1);
> >>  
> >>      if ( *max_pfn >= virt_to_pfn(HYPERVISOR_VIRT_START) )
> >>      {
> >> @@ -229,9 +229,15 @@ static void build_pagetable(unsigned long *start_pfn, unsigned long *max_pfn)
> >>  #if defined(__x86_64__)
> >>          offset = l4_table_offset(start_address);
> >>          /* Need new L3 pt frame */
> >> -        if ( !(start_address & L3_MASK) )
> >> -            if ( need_pt_frame(start_address, L3_FRAME) ) 
> >> -                new_pt_frame(&pt_pfn, pt_mfn, offset, L3_FRAME);
> >> +        if ( !(tab[offset] & _PAGE_PRESENT) )
> >> +        {
> >> +            if ( !need_pt_frame(start_address, L3_FRAME) )
> >> +            {
> >> +                printk("ERROR: build_pagetable(): L3 frame not present\n");
> >> +                do_exit();
> >> +            }
> >> +            new_pt_frame(&pt_pfn, pt_mfn, offset, L3_FRAME);
> > 
> > I'm not sure about this hunk and other similar hunks. Shouldn't this be
> > 
> >                if ( need_pt_frame(start_address, L3_FRAME) )
> > 	           new_pt_frame(&pt_pfn, pt_mfn, offset, L3_FRAME);
> > 
> > That is, just like before.
> 
> I didn't want to keep it like before. What sense does it make to
> find a page table is not existing and to decide it don't need to be
> created as it should exist already and then to carry on as if it would
> be there? The only viable alternative would have been to nuke the
> need_pt_frame() call.
> 

Yeah, just nuking the need_pt_frame is fine. That's simpler.

Wei.


> Juergen
>