| Message ID | fba262641f8233b4b9856cffeeb7a3ad0bad086a.1575477921.git.hongyxia@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add alternative API for Xen PTEs | expand |
> There's no need for the map/unmap functions to have a _new > suffix, is there? I thought this was weird at first also, but what I find really useful is that we can just change all call sites to the new API in small steps without breaking. Otherwise we have to merge a huge batch of changes (around 40 patches) at once. Hongyan
On 04.12.2019 18:54, Xia, Hongyan wrote: >> There's no need for the map/unmap functions to have a _new >> suffix, is there? > > I thought this was weird at first also, but what I find really useful > is that we can just change all call sites to the new API in small steps > without breaking. Otherwise we have to merge a huge batch of > changes (around 40 patches) at once. But my comment was on functions _not_ having an "old" equivalent. Having the suffix there initially makes for more code churn than necessary when finally dropping the suffixes. Jan
Hi, On 04/12/2019 17:10, Hongyan Xia wrote: > From: Wei Liu <wei.liu2@citrix.com> > > We are going to switch to using domheap page for page tables. > A new set of APIs is introduced to allocate, map, unmap and free pages > for page tables. > > The allocation and deallocation work on mfn_t but not page_info, > because they are required to work even before frame table is set up. > > Implement the old functions with the new ones. We will rewrite, site > by site, other mm functions that manipulate page tables to use the new > APIs. > > Note these new APIs still use xenheap page underneath and no actual > map and unmap is done so that we don't break xen half way. They will > be switched to use domheap and dynamic mappings when usage of old APIs > is eliminated. > > No functional change intended in this patch. > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > Signed-off-by: Hongyan Xia <hongyxia@amazon.com> > > --- > Changed since v3: > - const qualify unmap_xen_pagetable_new(). > - remove redundant parentheses. > --- > xen/arch/x86/mm.c | 39 ++++++++++++++++++++++++++++++++++----- > xen/include/asm-x86/mm.h | 11 +++++++++++ > 2 files changed, 45 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c > index 7d4dd80a85..ca362ad638 100644 > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -119,6 +119,7 @@ > #include <xen/efi.h> > #include <xen/grant_table.h> > #include <xen/hypercall.h> > +#include <xen/mm.h> > #include <asm/paging.h> > #include <asm/shadow.h> > #include <asm/page.h> > @@ -5020,22 +5021,50 @@ int mmcfg_intercept_write( > } > > void *alloc_xen_pagetable(void) > +{ > + mfn_t mfn; > + > + mfn = alloc_xen_pagetable_new(); > + ASSERT(!mfn_eq(mfn, INVALID_MFN)); The current version of alloc_xen_pagetable() may return NULL. So I can't see why this would not happen with the new implementation (see more below). Furthermore, AFAIK, mfn_to_virt() is not able to deal with INVALID_MFN. While the ASSERT will catch such error in debug build, non-debug build will end up to undefined behavior (allocation failure is a real possibility). Regardless the discussion on whether the ASSERT() is warrant, I think you want to have: if ( mfn_eq(mfn, INVALID_MFN) ) return NULL; I am half tempted to suggest to put that in map_xen_pagetable_new() for hardening. > + > + return map_xen_pagetable_new(mfn); > +} > + > +void free_xen_pagetable(void *v) > +{ > + if ( system_state != SYS_STATE_early_boot ) > + free_xen_pagetable_new(virt_to_mfn(v)); > +} > + > +mfn_t alloc_xen_pagetable_new(void) > { > if ( system_state != SYS_STATE_early_boot ) > { > void *ptr = alloc_xenheap_page(); > > BUG_ON(!hardware_domain && !ptr); This BUG_ON() only catch memory exhaustion before the Hardware Domain (aka Dom0) is created. Afterwards, the pointer may be NULL. As ... > - return ptr; > + return virt_to_mfn(ptr); virt_to_mfn() requires a valid MFN, you will end up to undefined behavior. > } > > - return mfn_to_virt(mfn_x(alloc_boot_pages(1, 1))); > + return alloc_boot_pages(1, 1); > } > > -void free_xen_pagetable(void *v) > +void *map_xen_pagetable_new(mfn_t mfn) > { > - if ( system_state != SYS_STATE_early_boot ) > - free_xenheap_page(v); > + return mfn_to_virt(mfn_x(mfn)); > +} > + > +/* v can point to an entry within a table or be NULL */ > +void unmap_xen_pagetable_new(const void *v) > +{ > + /* XXX still using xenheap page, no need to do anything. */ > +} > + > +/* mfn can be INVALID_MFN */ > +void free_xen_pagetable_new(mfn_t mfn) > +{ > + if ( system_state != SYS_STATE_early_boot && !mfn_eq(mfn, INVALID_MFN) ) > + free_xenheap_page(mfn_to_virt(mfn_x(mfn))); > } > > static DEFINE_SPINLOCK(map_pgdir_lock); > diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h > index 9d2b833579..76593fe9e7 100644 > --- a/xen/include/asm-x86/mm.h > +++ b/xen/include/asm-x86/mm.h > @@ -582,6 +582,17 @@ void *do_page_walk(struct vcpu *v, unsigned long addr); > /* Allocator functions for Xen pagetables. */ > void *alloc_xen_pagetable(void); > void free_xen_pagetable(void *v); > +mfn_t alloc_xen_pagetable_new(void); > +void *map_xen_pagetable_new(mfn_t mfn); > +void unmap_xen_pagetable_new(const void *v); > +void free_xen_pagetable_new(mfn_t mfn); > + > +#define UNMAP_XEN_PAGETABLE_NEW(ptr) \ > + do { \ > + unmap_xen_pagetable_new(ptr); \ > + (ptr) = NULL; \ > + } while (0) > + > l1_pgentry_t *virt_to_xen_l1e(unsigned long v); > > int __sync_local_execstate(void); >
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 7d4dd80a85..ca362ad638 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -119,6 +119,7 @@ #include <xen/efi.h> #include <xen/grant_table.h> #include <xen/hypercall.h> +#include <xen/mm.h> #include <asm/paging.h> #include <asm/shadow.h> #include <asm/page.h> @@ -5020,22 +5021,50 @@ int mmcfg_intercept_write( } void *alloc_xen_pagetable(void) +{ + mfn_t mfn; + + mfn = alloc_xen_pagetable_new(); + ASSERT(!mfn_eq(mfn, INVALID_MFN)); + + return map_xen_pagetable_new(mfn); +} + +void free_xen_pagetable(void *v) +{ + if ( system_state != SYS_STATE_early_boot ) + free_xen_pagetable_new(virt_to_mfn(v)); +} + +mfn_t alloc_xen_pagetable_new(void) { if ( system_state != SYS_STATE_early_boot ) { void *ptr = alloc_xenheap_page(); BUG_ON(!hardware_domain && !ptr); - return ptr; + return virt_to_mfn(ptr); } - return mfn_to_virt(mfn_x(alloc_boot_pages(1, 1))); + return alloc_boot_pages(1, 1); } -void free_xen_pagetable(void *v) +void *map_xen_pagetable_new(mfn_t mfn) { - if ( system_state != SYS_STATE_early_boot ) - free_xenheap_page(v); + return mfn_to_virt(mfn_x(mfn)); +} + +/* v can point to an entry within a table or be NULL */ +void unmap_xen_pagetable_new(const void *v) +{ + /* XXX still using xenheap page, no need to do anything. */ +} + +/* mfn can be INVALID_MFN */ +void free_xen_pagetable_new(mfn_t mfn) +{ + if ( system_state != SYS_STATE_early_boot && !mfn_eq(mfn, INVALID_MFN) ) + free_xenheap_page(mfn_to_virt(mfn_x(mfn))); } static DEFINE_SPINLOCK(map_pgdir_lock); diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h index 9d2b833579..76593fe9e7 100644 --- a/xen/include/asm-x86/mm.h +++ b/xen/include/asm-x86/mm.h @@ -582,6 +582,17 @@ void *do_page_walk(struct vcpu *v, unsigned long addr); /* Allocator functions for Xen pagetables. */ void *alloc_xen_pagetable(void); void free_xen_pagetable(void *v); +mfn_t alloc_xen_pagetable_new(void); +void *map_xen_pagetable_new(mfn_t mfn); +void unmap_xen_pagetable_new(const void *v); +void free_xen_pagetable_new(mfn_t mfn); + +#define UNMAP_XEN_PAGETABLE_NEW(ptr) \ + do { \ + unmap_xen_pagetable_new(ptr); \ + (ptr) = NULL; \ + } while (0) + l1_pgentry_t *virt_to_xen_l1e(unsigned long v); int __sync_local_execstate(void);