| Message ID | 1471627333-4534-3-git-send-email-paul.c.lai@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 19.08.16 at 19:22, <paul.c.lai@intel.com> wrote: > @@ -65,6 +66,50 @@ altp2m_vcpu_destroy(struct vcpu *v) > vcpu_unpause(v); > } > > +int > +hvm_altp2m_init( struct domain *d) Stray blank (more elsewhere). Also I don't think hvm_ is a proper prefix for a function placed in this file, i.e. if altp2m_init() is used elsewhere already, then altp2m_hvm_init() or perhaps better altp2m_domain_init() please. > +{ > + int rc = 0; > + unsigned int i = 0; Pointless initializers. > + /* Init alternate p2m data. */ > + if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL ) > + { > + rc = -ENOMEM; > + goto out; > + } When the epilogue (after the target label) is just a return statement, please avoid using goto. > +void > +hvm_altp2m_teardown( struct domain *d) > +{ > + unsigned int i = 0; > + d->arch.altp2m_active = 0; > + > + if ( d->arch.altp2m_eptp ) > + { > + free_xenheap_page(d->arch.altp2m_eptp); > + d->arch.altp2m_eptp = NULL; > + } Please avoid the if() - free_xenheap_page() happily deals with a NULL pointer passed to it. > + for ( i = 0; i < MAX_ALTP2M; i++ ) > + p2m_teardown(d->arch.altp2m_p2m[i]); I realize it's been that way in the original code, but wouldn't swapping the two things be both more natural and more robust? > @@ -501,24 +502,9 @@ int hap_enable(struct domain *d, u32 mode) > > if ( hvm_altp2m_supported() ) > { > - /* Init alternate p2m data */ > - if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL ) > - { > - rv = -ENOMEM; > - goto out; > - } > - > - for ( i = 0; i < MAX_EPTP; i++ ) > - d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN); > - > - for ( i = 0; i < MAX_ALTP2M; i++ ) > - { > - rv = p2m_alloc_table(d->arch.altp2m_p2m[i]); > - if ( rv != 0 ) > - goto out; > - } > - > - d->arch.altp2m_active = 0; > + rv = hvm_altp2m_init(d); > + if ( rv != 0 ) > + goto out; > } > > /* Now let other users see the new mode */ > @@ -534,18 +520,7 @@ void hap_final_teardown(struct domain *d) > unsigned int i; > > if ( hvm_altp2m_supported() ) > - { > - d->arch.altp2m_active = 0; > - > - if ( d->arch.altp2m_eptp ) > - { > - free_xenheap_page(d->arch.altp2m_eptp); > - d->arch.altp2m_eptp = NULL; > - } > - > - for ( i = 0; i < MAX_ALTP2M; i++ ) > - p2m_teardown(d->arch.altp2m_p2m[i]); > - } > + hvm_altp2m_teardown(d); I wonder whether in both cases the hvm_altp2m_supported() would better also be moved into the functions. It looks like the parts above and below this point, except for header file adjustments, are completely independent. Please consider splitting into two patches. > --- a/xen/arch/x86/mm/p2m-ept.c > +++ b/xen/arch/x86/mm/p2m-ept.c > @@ -1329,6 +1329,45 @@ void setup_ept_dump(void) > register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0); > } > > +void p2m_init_altp2m_ept_helper( struct domain *d, unsigned int i) Please drop the _helper suffix now that there is _ept. Jan
[PAUL] in-line Ravi -- please comment on swap comment below. -----Original Message----- From: Jan Beulich [mailto:JBeulich@suse.com] Sent: Friday, September 2, 2016 6:31 AM To: Lai, Paul C <paul.c.lai@intel.com> Cc: george.dunlap@citrix.com; Sahita, Ravi <ravi.sahita@intel.com>; xen-devel <xen-devel@lists.xenproject.org> Subject: Re: [PATCH v2 Altp2m cleanup v3 2/3] Move altp2m specific functions to altp2m files. >>> On 19.08.16 at 19:22, <paul.c.lai@intel.com> wrote: > @@ -65,6 +66,50 @@ altp2m_vcpu_destroy(struct vcpu *v) > vcpu_unpause(v); > } > > +int > +hvm_altp2m_init( struct domain *d) Stray blank (more elsewhere). Also I don't think hvm_ is a proper prefix for a function placed in this file, i.e. if altp2m_init() is used elsewhere already, then altp2m_hvm_init() or perhaps better altp2m_domain_init() please. > +{ > + int rc = 0; > + unsigned int i = 0; Pointless initializers. > + /* Init alternate p2m data. */ > + if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL ) > + { > + rc = -ENOMEM; > + goto out; > + } When the epilogue (after the target label) is just a return statement, please avoid using goto. [PAUL] I don't see this code in an epilogue (after the target label). > +void > +hvm_altp2m_teardown( struct domain *d) { > + unsigned int i = 0; > + d->arch.altp2m_active = 0; > + > + if ( d->arch.altp2m_eptp ) > + { > + free_xenheap_page(d->arch.altp2m_eptp); > + d->arch.altp2m_eptp = NULL; > + } Please avoid the if() - free_xenheap_page() happily deals with a NULL pointer passed to it. > + for ( i = 0; i < MAX_ALTP2M; i++ ) > + p2m_teardown(d->arch.altp2m_p2m[i]); I realize it's been that way in the original code, but wouldn't swapping the two things be both more natural and more robust? [PAUL] Ravi ? > @@ -501,24 +502,9 @@ int hap_enable(struct domain *d, u32 mode) > > if ( hvm_altp2m_supported() ) > { > - /* Init alternate p2m data */ > - if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL ) > - { > - rv = -ENOMEM; > - goto out; > - } > - > - for ( i = 0; i < MAX_EPTP; i++ ) > - d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN); > - > - for ( i = 0; i < MAX_ALTP2M; i++ ) > - { > - rv = p2m_alloc_table(d->arch.altp2m_p2m[i]); > - if ( rv != 0 ) > - goto out; > - } > - > - d->arch.altp2m_active = 0; > + rv = hvm_altp2m_init(d); > + if ( rv != 0 ) > + goto out; > } > > /* Now let other users see the new mode */ @@ -534,18 +520,7 @@ > void hap_final_teardown(struct domain *d) > unsigned int i; > > if ( hvm_altp2m_supported() ) > - { > - d->arch.altp2m_active = 0; > - > - if ( d->arch.altp2m_eptp ) > - { > - free_xenheap_page(d->arch.altp2m_eptp); > - d->arch.altp2m_eptp = NULL; > - } > - > - for ( i = 0; i < MAX_ALTP2M; i++ ) > - p2m_teardown(d->arch.altp2m_p2m[i]); > - } > + hvm_altp2m_teardown(d); I wonder whether in both cases the hvm_altp2m_supported() would better also be moved into the functions. It looks like the parts above and below this point, except for header file adjustments, are completely independent. Please consider splitting into two patches. > --- a/xen/arch/x86/mm/p2m-ept.c > +++ b/xen/arch/x86/mm/p2m-ept.c > @@ -1329,6 +1329,45 @@ void setup_ept_dump(void) > register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT > tables", 0); } > > +void p2m_init_altp2m_ept_helper( struct domain *d, unsigned int i) Please drop the _helper suffix now that there is _ept. Jan
>>> On 02.09.16 at 19:56, <paul.c.lai@intel.com> wrote: > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Friday, September 2, 2016 6:31 AM >>>> On 19.08.16 at 19:22, <paul.c.lai@intel.com> wrote: >> + /* Init alternate p2m data. */ >> + if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL ) >> + { >> + rc = -ENOMEM; >> + goto out; >> + } > > When the epilogue (after the target label) is just a return statement, > please avoid using goto. > > [PAUL] I don't see this code in an epilogue (after the target label). I don't understand: The function ends like this out: return rc; } What is it that you don't see here? Again, all I'm asking for is that in a case like this you simply use "return" instead of the rc assignment and "goto". Jan
diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c index 930bdc2..cc9b0fc 100644 --- a/xen/arch/x86/mm/altp2m.c +++ b/xen/arch/x86/mm/altp2m.c @@ -17,6 +17,7 @@ #include <asm/hvm/support.h> #include <asm/hvm/hvm.h> +#include <asm/domain.h> #include <asm/p2m.h> #include <asm/altp2m.h> @@ -65,6 +66,50 @@ altp2m_vcpu_destroy(struct vcpu *v) vcpu_unpause(v); } +int +hvm_altp2m_init( struct domain *d) +{ + int rc = 0; + unsigned int i = 0; + + /* Init alternate p2m data. */ + if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL ) + { + rc = -ENOMEM; + goto out; + } + + for ( i = 0; i < MAX_EPTP; i++ ) + d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN); + + for ( i = 0; i < MAX_ALTP2M; i++ ) + { + rc = p2m_alloc_table(d->arch.altp2m_p2m[i]); + if ( rc != 0 ) + goto out; + } + + d->arch.altp2m_active = 0; + out: + return rc; +} + +void +hvm_altp2m_teardown( struct domain *d) +{ + unsigned int i = 0; + d->arch.altp2m_active = 0; + + if ( d->arch.altp2m_eptp ) + { + free_xenheap_page(d->arch.altp2m_eptp); + d->arch.altp2m_eptp = NULL; + } + + for ( i = 0; i < MAX_ALTP2M; i++ ) + p2m_teardown(d->arch.altp2m_p2m[i]); +} + /* * Local variables: * mode: C diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c index 3218fa2..6e1e9a5 100644 --- a/xen/arch/x86/mm/hap/hap.c +++ b/xen/arch/x86/mm/hap/hap.c @@ -37,6 +37,7 @@ #include <asm/hap.h> #include <asm/paging.h> #include <asm/p2m.h> +#include <asm/altp2m.h> #include <asm/domain.h> #include <xen/numa.h> #include <asm/hvm/nestedhvm.h> @@ -501,24 +502,9 @@ int hap_enable(struct domain *d, u32 mode) if ( hvm_altp2m_supported() ) { - /* Init alternate p2m data */ - if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL ) - { - rv = -ENOMEM; - goto out; - } - - for ( i = 0; i < MAX_EPTP; i++ ) - d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN); - - for ( i = 0; i < MAX_ALTP2M; i++ ) - { - rv = p2m_alloc_table(d->arch.altp2m_p2m[i]); - if ( rv != 0 ) - goto out; - } - - d->arch.altp2m_active = 0; + rv = hvm_altp2m_init(d); + if ( rv != 0 ) + goto out; } /* Now let other users see the new mode */ @@ -534,18 +520,7 @@ void hap_final_teardown(struct domain *d) unsigned int i; if ( hvm_altp2m_supported() ) - { - d->arch.altp2m_active = 0; - - if ( d->arch.altp2m_eptp ) - { - free_xenheap_page(d->arch.altp2m_eptp); - d->arch.altp2m_eptp = NULL; - } - - for ( i = 0; i < MAX_ALTP2M; i++ ) - p2m_teardown(d->arch.altp2m_p2m[i]); - } + hvm_altp2m_teardown(d); /* Destroy nestedp2m's first */ for (i = 0; i < MAX_NESTEDP2M; i++) { diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index 13cab24..8247a02 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -1329,6 +1329,45 @@ void setup_ept_dump(void) register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0); } +void p2m_init_altp2m_ept_helper( struct domain *d, unsigned int i) +{ + struct p2m_domain *p2m = d->arch.altp2m_p2m[i]; + struct ept_data *ept; + + p2m->min_remapped_gfn = gfn_x(INVALID_GFN); + p2m->max_remapped_gfn = 0; + ept = &p2m->ept; + ept->asr = pagetable_get_pfn(p2m_get_pagetable(p2m)); + d->arch.altp2m_eptp[i] = ept_get_eptp(ept); +} + +unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp) +{ + struct p2m_domain *p2m; + struct ept_data *ept; + unsigned int i; + + altp2m_list_lock(d); + + for ( i = 0; i < MAX_ALTP2M; i++ ) + { + if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) ) + continue; + + p2m = d->arch.altp2m_p2m[i]; + ept = &p2m->ept; + + if ( eptp == ept_get_eptp(ept) ) + goto out; + } + + i = INVALID_ALTP2M; + + out: + altp2m_list_unlock(d); + return i; +} + /* * Local variables: * mode: C diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index ff0cce8..7673663 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -196,8 +196,8 @@ static void p2m_teardown_altp2m(struct domain *d) if ( !d->arch.altp2m_p2m[i] ) continue; p2m = d->arch.altp2m_p2m[i]; - d->arch.altp2m_p2m[i] = NULL; p2m_free_one(p2m); + d->arch.altp2m_p2m[i] = NULL; } } @@ -2278,33 +2278,6 @@ int unmap_mmio_regions(struct domain *d, return i == nr ? 0 : i ?: ret; } -unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp) -{ - struct p2m_domain *p2m; - struct ept_data *ept; - unsigned int i; - - altp2m_list_lock(d); - - for ( i = 0; i < MAX_ALTP2M; i++ ) - { - if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) ) - continue; - - p2m = d->arch.altp2m_p2m[i]; - ept = &p2m->ept; - - if ( eptp == ept_get_eptp(ept) ) - goto out; - } - - i = INVALID_ALTP2M; - - out: - altp2m_list_unlock(d); - return i; -} - bool_t p2m_switch_vcpu_altp2m_by_id(struct vcpu *v, unsigned int idx) { struct domain *d = v->domain; @@ -2410,18 +2383,6 @@ void p2m_flush_altp2m(struct domain *d) altp2m_list_unlock(d); } -static void p2m_init_altp2m_helper(struct domain *d, unsigned int i) -{ - struct p2m_domain *p2m = d->arch.altp2m_p2m[i]; - struct ept_data *ept; - - p2m->min_remapped_gfn = gfn_x(INVALID_GFN); - p2m->max_remapped_gfn = 0; - ept = &p2m->ept; - ept->asr = pagetable_get_pfn(p2m_get_pagetable(p2m)); - d->arch.altp2m_eptp[i] = ept_get_eptp(ept); -} - int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx) { int rc = -EINVAL; @@ -2433,7 +2394,7 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx) if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) ) { - p2m_init_altp2m_helper(d, idx); + p2m_init_altp2m_ept_helper(d, idx); rc = 0; } @@ -2453,7 +2414,7 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx) if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) ) continue; - p2m_init_altp2m_helper(d, i); + p2m_init_altp2m_ept_helper(d, i); *idx = i; rc = 0; diff --git a/xen/include/asm-x86/altp2m.h b/xen/include/asm-x86/altp2m.h index 64c7618..a236ed7 100644 --- a/xen/include/asm-x86/altp2m.h +++ b/xen/include/asm-x86/altp2m.h @@ -18,7 +18,6 @@ #ifndef __ASM_X86_ALTP2M_H #define __ASM_X86_ALTP2M_H -#include <xen/types.h> #include <xen/sched.h> /* for struct vcpu, struct domain */ #include <asm/hvm/vcpu.h> /* for vcpu_altp2m */ @@ -38,4 +37,7 @@ static inline uint16_t altp2m_vcpu_idx(const struct vcpu *v) return vcpu_altp2m(v).p2midx; } +int hvm_altp2m_init(struct domain *d); +void hvm_altp2m_teardown(struct domain *d); + #endif /* __ASM_X86_ALTP2M_H */ diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h index 4cdd9b1..0383a9d 100644 --- a/xen/include/asm-x86/hvm/vmx/vmx.h +++ b/xen/include/asm-x86/hvm/vmx/vmx.h @@ -561,6 +561,9 @@ void ept_p2m_uninit(struct p2m_domain *p2m); void ept_walk_table(struct domain *d, unsigned long gfn); bool_t ept_handle_misconfig(uint64_t gpa); void setup_ept_dump(void); +void p2m_init_altp2m_ept_helper( struct domain *d, unsigned int i); +/* Locate an alternate p2m by its EPTP */ +unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp); void update_guest_eip(void); diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h index d5fd546..0c5b391 100644 --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -23,8 +23,8 @@ * along with this program; If not, see <http://www.gnu.org/licenses/>. */ -#ifndef _XEN_P2M_H -#define _XEN_P2M_H +#ifndef _XEN_ASM_X86_P2M_H +#define _XEN_ASM_X86_P2M_H #include <xen/config.h> #include <xen/paging.h> @@ -781,9 +781,6 @@ static inline struct p2m_domain *p2m_get_altp2m(struct vcpu *v) return v->domain->arch.altp2m_p2m[index]; } -/* Locate an alternate p2m by its EPTP */ -unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp); - /* Switch alternate p2m for a single vcpu */ bool_t p2m_switch_vcpu_altp2m_by_id(struct vcpu *v, unsigned int idx); @@ -845,7 +842,7 @@ static inline unsigned int p2m_get_iommu_flags(p2m_type_t p2mt) return flags; } -#endif /* _XEN_P2M_H */ +#endif /* _XEN_ASM_X86_P2M_H */ /* * Local variables:
Move altp2m specific functions to altp2m files. This makes the code a little easier to read. Also moving ept code to ept specific files as requested in: https://lists.xenproject.org/archives/html/xen-devel/2015-07/msg04323.html Signed-off-by: Paul Lai <paul.c.lai@intel.com> --- xen/arch/x86/mm/altp2m.c | 45 +++++++++++++++++++++++++++++++++++++++ xen/arch/x86/mm/hap/hap.c | 35 +++++------------------------- xen/arch/x86/mm/p2m-ept.c | 39 +++++++++++++++++++++++++++++++++ xen/arch/x86/mm/p2m.c | 45 +++------------------------------------ xen/include/asm-x86/altp2m.h | 4 +++- xen/include/asm-x86/hvm/vmx/vmx.h | 3 +++ xen/include/asm-x86/p2m.h | 9 +++----- 7 files changed, 101 insertions(+), 79 deletions(-)