| Message ID | e8ba0fb1451ef89c36b21a2063590baed2432031.1582799255.git.hongyxia@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] x86/mm: switch to new APIs in arch_init_memory | expand |
On Thu, Feb 27, 2020 at 10:27:39AM +0000, Hongyan Xia wrote: > From: Wei Liu <wei.liu2@citrix.com> > > The function will map and unmap pages on demand. > > Since we now map and unmap Xen PTE pages, we would like to track the > lifetime of mappings so that 1) we do not dereference memory through a > variable after it is unmapped, 2) we do not unmap more than once. > Therefore, we introduce the UNMAP_DOMAIN_PAGE macro to nullify the > variable after unmapping, and ignore NULL. > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > Signed-off-by: Hongyan Xia <hongyxia@amazon.com> > > --- > Changed in v2: > - let UNMAP_DOMAIN_PAGE itself check whether the input is NULL to avoid > adding the check in unmap_domain_page. > - reword the commit message. > --- > xen/arch/x86/mm.c | 14 ++++++++------ > xen/include/xen/domain_page.h | 7 +++++++ > 2 files changed, 15 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c > index 70b87c4830..9fcdcde5b7 100644 > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -356,19 +356,21 @@ void __init arch_init_memory(void) > ASSERT(root_pgt_pv_xen_slots < ROOT_PAGETABLE_PV_XEN_SLOTS); > if ( l4_table_offset(split_va) == l4_table_offset(split_va - 1) ) > { > - l3_pgentry_t *l3tab = alloc_xen_pagetable(); > + mfn_t l3mfn = alloc_xen_pagetable_new(); > > - if ( l3tab ) > + if ( !mfn_eq(l3mfn, INVALID_MFN) ) > { > - const l3_pgentry_t *l3idle = > - l4e_to_l3e(idle_pg_table[l4_table_offset(split_va)]); > + l3_pgentry_t *l3idle = map_l3t_from_l4e( > + idle_pg_table[l4_table_offset(split_va)]); > + l3_pgentry_t *l3tab = map_domain_page(l3mfn); > > for ( i = 0; i < l3_table_offset(split_va); ++i ) > l3tab[i] = l3idle[i]; > for ( ; i < L3_PAGETABLE_ENTRIES; ++i ) > l3tab[i] = l3e_empty(); > - split_l4e = l4e_from_mfn(virt_to_mfn(l3tab), > - __PAGE_HYPERVISOR_RW); > + split_l4e = l4e_from_mfn(l3mfn, __PAGE_HYPERVISOR_RW); > + UNMAP_DOMAIN_PAGE(l3idle); > + UNMAP_DOMAIN_PAGE(l3tab); > } > else > ++root_pgt_pv_xen_slots; > diff --git a/xen/include/xen/domain_page.h b/xen/include/xen/domain_page.h > index 32669a3339..bfc3bf6aeb 100644 > --- a/xen/include/xen/domain_page.h > +++ b/xen/include/xen/domain_page.h > @@ -72,4 +72,11 @@ static inline void unmap_domain_page_global(const void *va) {}; > > #endif /* !CONFIG_DOMAIN_PAGE */ > > +#define UNMAP_DOMAIN_PAGE(p) do { \ > + if ( p ) { \ Coding style mandates { should be placed in the next line. Other than this, the code looks correct to me. Wei. > + unmap_domain_page(p); \ > + (p) = NULL; \ > + } \ > +} while ( false ) > + > #endif /* __XEN_DOMAIN_PAGE_H__ */ > -- > 2.17.1 >
Hi Hongyan, On 27/02/2020 10:27, Hongyan Xia wrote: > From: Wei Liu <wei.liu2@citrix.com> > > The function will map and unmap pages on demand. > > Since we now map and unmap Xen PTE pages, we would like to track the > lifetime of mappings so that 1) we do not dereference memory through a > variable after it is unmapped, 2) we do not unmap more than once. > Therefore, we introduce the UNMAP_DOMAIN_PAGE macro to nullify the > variable after unmapping, and ignore NULL. > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > Signed-off-by: Hongyan Xia <hongyxia@amazon.com> > > --- > Changed in v2: > - let UNMAP_DOMAIN_PAGE itself check whether the input is NULL to avoid > adding the check in unmap_domain_page. > - reword the commit message. > --- > xen/arch/x86/mm.c | 14 ++++++++------ > xen/include/xen/domain_page.h | 7 +++++++ > 2 files changed, 15 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c > index 70b87c4830..9fcdcde5b7 100644 > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -356,19 +356,21 @@ void __init arch_init_memory(void) > ASSERT(root_pgt_pv_xen_slots < ROOT_PAGETABLE_PV_XEN_SLOTS); > if ( l4_table_offset(split_va) == l4_table_offset(split_va - 1) ) > { > - l3_pgentry_t *l3tab = alloc_xen_pagetable(); > + mfn_t l3mfn = alloc_xen_pagetable_new(); > > - if ( l3tab ) > + if ( !mfn_eq(l3mfn, INVALID_MFN) ) > { > - const l3_pgentry_t *l3idle = > - l4e_to_l3e(idle_pg_table[l4_table_offset(split_va)]); > + l3_pgentry_t *l3idle = map_l3t_from_l4e( > + idle_pg_table[l4_table_offset(split_va)]); > + l3_pgentry_t *l3tab = map_domain_page(l3mfn); > > for ( i = 0; i < l3_table_offset(split_va); ++i ) > l3tab[i] = l3idle[i]; > for ( ; i < L3_PAGETABLE_ENTRIES; ++i ) > l3tab[i] = l3e_empty(); > - split_l4e = l4e_from_mfn(virt_to_mfn(l3tab), > - __PAGE_HYPERVISOR_RW); > + split_l4e = l4e_from_mfn(l3mfn, __PAGE_HYPERVISOR_RW); > + UNMAP_DOMAIN_PAGE(l3idle); > + UNMAP_DOMAIN_PAGE(l3tab); > } > else > ++root_pgt_pv_xen_slots; > diff --git a/xen/include/xen/domain_page.h b/xen/include/xen/domain_page.h > index 32669a3339..bfc3bf6aeb 100644 > --- a/xen/include/xen/domain_page.h > +++ b/xen/include/xen/domain_page.h > @@ -72,4 +72,11 @@ static inline void unmap_domain_page_global(const void *va) {}; > > #endif /* !CONFIG_DOMAIN_PAGE */ > > +#define UNMAP_DOMAIN_PAGE(p) do { \ > + if ( p ) { \ > + unmap_domain_page(p); \ > + (p) = NULL; \ > + } \ > +} while ( false ) Do we need to keep the do {} while ()? Cheers,
On Thu, 2020-02-27 at 11:51 +0000, Julien Grall wrote: > Hi Hongyan, > > On 27/02/2020 10:27, Hongyan Xia wrote: > > ... > > diff --git a/xen/include/xen/domain_page.h > > b/xen/include/xen/domain_page.h > > index 32669a3339..bfc3bf6aeb 100644 > > --- a/xen/include/xen/domain_page.h > > +++ b/xen/include/xen/domain_page.h > > @@ -72,4 +72,11 @@ static inline void > > unmap_domain_page_global(const void *va) {}; > > > > #endif /* !CONFIG_DOMAIN_PAGE */ > > > > +#define UNMAP_DOMAIN_PAGE(p) do { \ > > + if ( p ) { \ > > + unmap_domain_page(p); \ > > + (p) = NULL; \ > > + } \ > > +} while ( false ) > > Do we need to keep the do {} while ()? I think we do. For example: if ( cond ) UNMAP_DOMAIN_PAGE(p); else blah_blah_blah(); If we remove the do-while, the else clause will be paired with the if in UNMAP_DOMAIN_PAGE(); Hongyan
Hi Hongyan, On 27/02/2020 11:59, Xia, Hongyan wrote: > On Thu, 2020-02-27 at 11:51 +0000, Julien Grall wrote: >> Hi Hongyan, >> >> On 27/02/2020 10:27, Hongyan Xia wrote: >>> ... >>> diff --git a/xen/include/xen/domain_page.h >>> b/xen/include/xen/domain_page.h >>> index 32669a3339..bfc3bf6aeb 100644 >>> --- a/xen/include/xen/domain_page.h >>> +++ b/xen/include/xen/domain_page.h >>> @@ -72,4 +72,11 @@ static inline void >>> unmap_domain_page_global(const void *va) {}; >>> >>> #endif /* !CONFIG_DOMAIN_PAGE */ >>> >>> +#define UNMAP_DOMAIN_PAGE(p) do { \ >>> + if ( p ) { \ >>> + unmap_domain_page(p); \ >>> + (p) = NULL; \ >>> + } \ >>> +} while ( false ) >> >> Do we need to keep the do {} while ()? > > I think we do. For example: > > if ( cond ) > UNMAP_DOMAIN_PAGE(p); > else > blah_blah_blah(); > > If we remove the do-while, the else clause will be paired with the if > in UNMAP_DOMAIN_PAGE(); GCC will actually throw a compiler error: test.c: In function ‘f’: test.c:13:5: error: ‘else’ without a previous ‘if’ else ^~~~ Anyway, yes we do need to keep do while {} to catch any use without the semicolon. Sorry for the noise. Cheers,
On 27.02.2020 11:27, Hongyan Xia wrote: > From: Wei Liu <wei.liu2@citrix.com> > > The function will map and unmap pages on demand. > > Since we now map and unmap Xen PTE pages, we would like to track the > lifetime of mappings so that 1) we do not dereference memory through a > variable after it is unmapped, 2) we do not unmap more than once. > Therefore, we introduce the UNMAP_DOMAIN_PAGE macro to nullify the > variable after unmapping, and ignore NULL. > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > Signed-off-by: Hongyan Xia <hongyxia@amazon.com> > > --- > Changed in v2: > - let UNMAP_DOMAIN_PAGE itself check whether the input is NULL to avoid > adding the check in unmap_domain_page. > - reword the commit message. > --- > xen/arch/x86/mm.c | 14 ++++++++------ > xen/include/xen/domain_page.h | 7 +++++++ > 2 files changed, 15 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c > index 70b87c4830..9fcdcde5b7 100644 > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -356,19 +356,21 @@ void __init arch_init_memory(void) > ASSERT(root_pgt_pv_xen_slots < ROOT_PAGETABLE_PV_XEN_SLOTS); > if ( l4_table_offset(split_va) == l4_table_offset(split_va - 1) ) > { > - l3_pgentry_t *l3tab = alloc_xen_pagetable(); > + mfn_t l3mfn = alloc_xen_pagetable_new(); > > - if ( l3tab ) > + if ( !mfn_eq(l3mfn, INVALID_MFN) ) > { > - const l3_pgentry_t *l3idle = > - l4e_to_l3e(idle_pg_table[l4_table_offset(split_va)]); > + l3_pgentry_t *l3idle = map_l3t_from_l4e( > + idle_pg_table[l4_table_offset(split_va)]); Somehow you've lost the const. I think both this and ... > --- a/xen/include/xen/domain_page.h > +++ b/xen/include/xen/domain_page.h > @@ -72,4 +72,11 @@ static inline void unmap_domain_page_global(const void *va) {}; > > #endif /* !CONFIG_DOMAIN_PAGE */ > > +#define UNMAP_DOMAIN_PAGE(p) do { \ > + if ( p ) { \ ... the brace placement here can be dealt with while committing. With both of them in place Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 70b87c4830..9fcdcde5b7 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -356,19 +356,21 @@ void __init arch_init_memory(void) ASSERT(root_pgt_pv_xen_slots < ROOT_PAGETABLE_PV_XEN_SLOTS); if ( l4_table_offset(split_va) == l4_table_offset(split_va - 1) ) { - l3_pgentry_t *l3tab = alloc_xen_pagetable(); + mfn_t l3mfn = alloc_xen_pagetable_new(); - if ( l3tab ) + if ( !mfn_eq(l3mfn, INVALID_MFN) ) { - const l3_pgentry_t *l3idle = - l4e_to_l3e(idle_pg_table[l4_table_offset(split_va)]); + l3_pgentry_t *l3idle = map_l3t_from_l4e( + idle_pg_table[l4_table_offset(split_va)]); + l3_pgentry_t *l3tab = map_domain_page(l3mfn); for ( i = 0; i < l3_table_offset(split_va); ++i ) l3tab[i] = l3idle[i]; for ( ; i < L3_PAGETABLE_ENTRIES; ++i ) l3tab[i] = l3e_empty(); - split_l4e = l4e_from_mfn(virt_to_mfn(l3tab), - __PAGE_HYPERVISOR_RW); + split_l4e = l4e_from_mfn(l3mfn, __PAGE_HYPERVISOR_RW); + UNMAP_DOMAIN_PAGE(l3idle); + UNMAP_DOMAIN_PAGE(l3tab); } else ++root_pgt_pv_xen_slots; diff --git a/xen/include/xen/domain_page.h b/xen/include/xen/domain_page.h index 32669a3339..bfc3bf6aeb 100644 --- a/xen/include/xen/domain_page.h +++ b/xen/include/xen/domain_page.h @@ -72,4 +72,11 @@ static inline void unmap_domain_page_global(const void *va) {}; #endif /* !CONFIG_DOMAIN_PAGE */ +#define UNMAP_DOMAIN_PAGE(p) do { \ + if ( p ) { \ + unmap_domain_page(p); \ + (p) = NULL; \ + } \ +} while ( false ) + #endif /* __XEN_DOMAIN_PAGE_H__ */