* [PATCH v3] minios: don't rely on specific page table allocation scheme
@ 2015-11-20 18:32 Juergen Gross
2015-11-20 18:39 ` Samuel Thibault
0 siblings, 1 reply; 3+ messages in thread
From: Juergen Gross @ 2015-11-20 18:32 UTC (permalink / raw)
To: stefano.stabellini, samuel.thibault, xen-devel, Ian.Campbell,
ian.jackson, wei.liu2, minios-devel
Cc: 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>
---
Changes in V3:
- check for every pfn already mapped (suggested by Samuel Thibault)
Changes in V2:
- remove need_pt_frame() as it simplifies code (suggested by Wei Liu)
---
arch/x86/mm.c | 93 ++++++++++++---------------------------------------
include/x86/arch_mm.h | 7 ----
2 files changed, 21 insertions(+), 79 deletions(-)
diff --git a/arch/x86/mm.c b/arch/x86/mm.c
index 9c6d1b8..134a135 100644
--- a/arch/x86/mm.c
+++ b/arch/x86/mm.c
@@ -132,61 +132,6 @@ static void new_pt_frame(unsigned long *pt_pfn, unsigned long prev_l_mfn,
}
/*
- * Checks if a pagetable frame is needed at 'level' to map a given
- * address. Note, this function is specific to the initial page table
- * building.
- */
-static int need_pt_frame(unsigned long va, int level)
-{
- unsigned long hyp_virt_start = HYPERVISOR_VIRT_START;
-#if defined(__x86_64__)
- unsigned long hyp_virt_end = HYPERVISOR_VIRT_END;
-#else
- unsigned long hyp_virt_end = 0xffffffff;
-#endif
-
- /* In general frames will _not_ be needed if they were already
- allocated to map the hypervisor into our VA space */
-#if defined(__x86_64__)
- if ( level == L3_FRAME )
- {
- if ( l4_table_offset(va) >=
- l4_table_offset(hyp_virt_start) &&
- l4_table_offset(va) <=
- l4_table_offset(hyp_virt_end))
- return 0;
- return 1;
- }
- else
-#endif
-
- if ( level == L2_FRAME )
- {
-#if defined(__x86_64__)
- if ( l4_table_offset(va) >=
- l4_table_offset(hyp_virt_start) &&
- l4_table_offset(va) <=
- l4_table_offset(hyp_virt_end))
-#endif
- if ( l3_table_offset(va) >=
- l3_table_offset(hyp_virt_start) &&
- l3_table_offset(va) <=
- l3_table_offset(hyp_virt_end))
- return 0;
-
- return 1;
- }
- else
- /* Always need l1 frames */
- if ( level == L1_FRAME )
- return 1;
-
- printk("ERROR: Unknown frame level %d, hypervisor %llx,%llx\n",
- level, hyp_virt_start, hyp_virt_end);
- return -1;
-}
-
-/*
* Build the initial pagetable.
*/
static void build_pagetable(unsigned long *start_pfn, unsigned long *max_pfn)
@@ -200,8 +145,9 @@ 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;
+ /* Be conservative: even if we know there will be more pages already
+ mapped, start the loop at the very beginning. */
+ pfn_to_map = *start_pfn;
if ( *max_pfn >= virt_to_pfn(HYPERVISOR_VIRT_START) )
{
@@ -229,9 +175,8 @@ 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) )
+ new_pt_frame(&pt_pfn, pt_mfn, offset, L3_FRAME);
page = tab[offset];
pt_mfn = pte_to_mfn(page);
@@ -239,29 +184,33 @@ 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) )
+ 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) )
+ new_pt_frame(&pt_pfn, pt_mfn, offset, L1_FRAME);
page = tab[offset];
pt_mfn = pte_to_mfn(page);
+ tab = to_virt(mfn_to_pfn(pt_mfn) << PAGE_SHIFT);
offset = l1_table_offset(start_address);
- mmu_updates[count].ptr =
- ((pgentry_t)pt_mfn << PAGE_SHIFT) + sizeof(pgentry_t) * offset;
- mmu_updates[count].val =
- (pgentry_t)pfn_to_mfn(pfn_to_map++) << PAGE_SHIFT | L1_PROT;
- count++;
- if ( count == L1_PAGETABLE_ENTRIES || pfn_to_map == *max_pfn )
+ if ( !(tab[offset] & _PAGE_PRESENT) )
+ {
+ mmu_updates[count].ptr =
+ ((pgentry_t)pt_mfn << PAGE_SHIFT) + sizeof(pgentry_t) * offset;
+ mmu_updates[count].val =
+ (pgentry_t)pfn_to_mfn(pfn_to_map) << PAGE_SHIFT | L1_PROT;
+ count++;
+ }
+ pfn_to_map++;
+ if ( count == L1_PAGETABLE_ENTRIES ||
+ (count && pfn_to_map == *max_pfn) )
{
rc = HYPERVISOR_mmu_update(mmu_updates, count, NULL, DOMID_SELF);
if ( rc < 0 )
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3] minios: don't rely on specific page table allocation scheme
2015-11-20 18:32 [PATCH v3] minios: don't rely on specific page table allocation scheme Juergen Gross
@ 2015-11-20 18:39 ` Samuel Thibault
2015-11-23 9:55 ` Ian Campbell
0 siblings, 1 reply; 3+ messages in thread
From: Samuel Thibault @ 2015-11-20 18:39 UTC (permalink / raw)
To: Juergen Gross
Cc: wei.liu2, Ian.Campbell, stefano.stabellini, ian.jackson,
xen-devel, minios-devel
Juergen Gross, on Fri 20 Nov 2015 19:32:42 +0100, 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>
Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
> Changes in V3:
> - check for every pfn already mapped (suggested by Samuel Thibault)
>
> Changes in V2:
> - remove need_pt_frame() as it simplifies code (suggested by Wei Liu)
>
> ---
> arch/x86/mm.c | 93 ++++++++++++---------------------------------------
> include/x86/arch_mm.h | 7 ----
> 2 files changed, 21 insertions(+), 79 deletions(-)
>
> diff --git a/arch/x86/mm.c b/arch/x86/mm.c
> index 9c6d1b8..134a135 100644
> --- a/arch/x86/mm.c
> +++ b/arch/x86/mm.c
> @@ -132,61 +132,6 @@ static void new_pt_frame(unsigned long *pt_pfn, unsigned long prev_l_mfn,
> }
>
> /*
> - * Checks if a pagetable frame is needed at 'level' to map a given
> - * address. Note, this function is specific to the initial page table
> - * building.
> - */
> -static int need_pt_frame(unsigned long va, int level)
> -{
> - unsigned long hyp_virt_start = HYPERVISOR_VIRT_START;
> -#if defined(__x86_64__)
> - unsigned long hyp_virt_end = HYPERVISOR_VIRT_END;
> -#else
> - unsigned long hyp_virt_end = 0xffffffff;
> -#endif
> -
> - /* In general frames will _not_ be needed if they were already
> - allocated to map the hypervisor into our VA space */
> -#if defined(__x86_64__)
> - if ( level == L3_FRAME )
> - {
> - if ( l4_table_offset(va) >=
> - l4_table_offset(hyp_virt_start) &&
> - l4_table_offset(va) <=
> - l4_table_offset(hyp_virt_end))
> - return 0;
> - return 1;
> - }
> - else
> -#endif
> -
> - if ( level == L2_FRAME )
> - {
> -#if defined(__x86_64__)
> - if ( l4_table_offset(va) >=
> - l4_table_offset(hyp_virt_start) &&
> - l4_table_offset(va) <=
> - l4_table_offset(hyp_virt_end))
> -#endif
> - if ( l3_table_offset(va) >=
> - l3_table_offset(hyp_virt_start) &&
> - l3_table_offset(va) <=
> - l3_table_offset(hyp_virt_end))
> - return 0;
> -
> - return 1;
> - }
> - else
> - /* Always need l1 frames */
> - if ( level == L1_FRAME )
> - return 1;
> -
> - printk("ERROR: Unknown frame level %d, hypervisor %llx,%llx\n",
> - level, hyp_virt_start, hyp_virt_end);
> - return -1;
> -}
> -
> -/*
> * Build the initial pagetable.
> */
> static void build_pagetable(unsigned long *start_pfn, unsigned long *max_pfn)
> @@ -200,8 +145,9 @@ 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;
> + /* Be conservative: even if we know there will be more pages already
> + mapped, start the loop at the very beginning. */
> + pfn_to_map = *start_pfn;
>
> if ( *max_pfn >= virt_to_pfn(HYPERVISOR_VIRT_START) )
> {
> @@ -229,9 +175,8 @@ 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) )
> + new_pt_frame(&pt_pfn, pt_mfn, offset, L3_FRAME);
>
> page = tab[offset];
> pt_mfn = pte_to_mfn(page);
> @@ -239,29 +184,33 @@ 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) )
> + 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) )
> + new_pt_frame(&pt_pfn, pt_mfn, offset, L1_FRAME);
>
> page = tab[offset];
> pt_mfn = pte_to_mfn(page);
> + tab = to_virt(mfn_to_pfn(pt_mfn) << PAGE_SHIFT);
> offset = l1_table_offset(start_address);
>
> - mmu_updates[count].ptr =
> - ((pgentry_t)pt_mfn << PAGE_SHIFT) + sizeof(pgentry_t) * offset;
> - mmu_updates[count].val =
> - (pgentry_t)pfn_to_mfn(pfn_to_map++) << PAGE_SHIFT | L1_PROT;
> - count++;
> - if ( count == L1_PAGETABLE_ENTRIES || pfn_to_map == *max_pfn )
> + if ( !(tab[offset] & _PAGE_PRESENT) )
> + {
> + mmu_updates[count].ptr =
> + ((pgentry_t)pt_mfn << PAGE_SHIFT) + sizeof(pgentry_t) * offset;
> + mmu_updates[count].val =
> + (pgentry_t)pfn_to_mfn(pfn_to_map) << PAGE_SHIFT | L1_PROT;
> + count++;
> + }
> + pfn_to_map++;
> + if ( count == L1_PAGETABLE_ENTRIES ||
> + (count && pfn_to_map == *max_pfn) )
> {
> rc = HYPERVISOR_mmu_update(mmu_updates, count, NULL, DOMID_SELF);
> if ( rc < 0 )
> 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
>
--
Samuel
<N> sl - display animations aimed to correct users who accidentally enter
<N> sl instead of ls.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] minios: don't rely on specific page table allocation scheme
2015-11-20 18:39 ` Samuel Thibault
@ 2015-11-23 9:55 ` Ian Campbell
0 siblings, 0 replies; 3+ messages in thread
From: Ian Campbell @ 2015-11-23 9:55 UTC (permalink / raw)
To: Samuel Thibault, Juergen Gross
Cc: minios-devel, wei.liu2, xen-devel, ian.jackson, stefano.stabellini
On Fri, 2015-11-20 at 19:39 +0100, Samuel Thibault wrote:
> Juergen Gross, on Fri 20 Nov 2015 19:32:42 +0100, wrote:
This patch conflicted with 1855e8b2e884 "mini-os: enable compiler check for
printk format types" which was committed in June.
I have (hopefully correctly) resolved the conflict by simply removing the
entirety of need_pt_frame(), but in the future please base your patches on
something up to date.
> > 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>
>
> Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Applied, along with a corresponding update to MINIOS_UPSTREAM_REVISION in
xen.git:Config.mk.
Ian.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-11-23 9:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-20 18:32 [PATCH v3] minios: don't rely on specific page table allocation scheme Juergen Gross
2015-11-20 18:39 ` Samuel Thibault
2015-11-23 9:55 ` Ian Campbell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.