| Message ID | 1473285866-6868-4-git-send-email-paul.c.lai@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 08.09.16 at 00:04, <paul.c.lai@intel.com> wrote: > @@ -65,6 +66,56 @@ altp2m_vcpu_destroy(struct vcpu *v) > vcpu_unpause(v); > } > > +int > +altp2m_domain_init(struct domain *d) > +{ > + int rc; > + unsigned int i; > + > + if ( !hvm_altp2m_supported() ) > + { > + rc = 0; > + goto out; > + } > + /* 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; > +} I don't follow: I did specifically ask to avoid goto when where the label is there's just a single statement (return in this case). > +void > +altp2m_domain_teardown(struct domain *d) > +{ > + unsigned int i; > + > + if ( !hvm_altp2m_supported() ) > + return; Bad tab indentation. > @@ -499,27 +500,7 @@ int hap_enable(struct domain *d, u32 mode) > goto out; > } > > - 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 = altp2m_domain_init(d); > > /* Now let other users see the new mode */ > d->arch.paging.mode = mode | PG_HAP_enable; I don't think you should reach the following statement(s) when your function returned an error. Even if this might be benign now, this is easy to overlook if later more code gets added near the end of the function. Also I wonder whether in an error case there's not a possibility for memory to be leaked. That wouldn't be a problem your patch introduces, but I think you could have noticed and fixed this as you touch the code anyway. > --- 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( struct domain *d, unsigned int i) Another instance of you not having corrected what was previously pointed out: There's a stray blank here. And even if I hadn't said there are multiple instances of this, you should still apply such comments to all of your series. And I only now notice that this e.g. also applies to the suggested re-ordering of operations in altp2m_domain_teardown(). I think I'm going to stop reviewing this series here: Please make sure you address all review comments (either verbally or by code adjustment) before submitting a new version (or in extreme cases, like due to lack of feedback, point out open issues right after the first --- separator). Jan
[Paul2] in-line -----Original Message----- From: Jan Beulich [mailto:JBeulich@suse.com] Sent: Thursday, September 8, 2016 8:02 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 Altp2m cleanup v4 3/4] Move altp2m specific functions to altp2m files. >>> On 08.09.16 at 00:04, <paul.c.lai@intel.com> wrote: > @@ -65,6 +66,56 @@ altp2m_vcpu_destroy(struct vcpu *v) > vcpu_unpause(v); > } > > +int > +altp2m_domain_init(struct domain *d) > +{ > + int rc; > + unsigned int i; > + > + if ( !hvm_altp2m_supported() ) > + { > + rc = 0; > + goto out; > + } > + /* 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; > +} I don't follow: I did specifically ask to avoid goto when where the label is there's just a single statement (return in this case). [Paul2] In the v3 2/3 patch series you said: [Jan] 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). [Paul2] I didn't get an answer to the question. After scratching my head, just realized you want the 'goto' to become a 'return'. I will make this change in order to pass your review (and the coding style of Xen). That said, I'm a believer in one exit per function -- to me, this is a much cleaner coding style. I'm not trying to start a debate, I'm just explaining how I was confused. > +void > +altp2m_domain_teardown(struct domain *d) { > + unsigned int i; > + > + if ( !hvm_altp2m_supported() ) > + return; Bad tab indentation. > @@ -499,27 +500,7 @@ int hap_enable(struct domain *d, u32 mode) > goto out; > } > > - 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 = altp2m_domain_init(d); > > /* Now let other users see the new mode */ > d->arch.paging.mode = mode | PG_HAP_enable; I don't think you should reach the following statement(s) when your function returned an error. Even if this might be benign now, this is easy to overlook if later more code gets added near the end of the function. Also I wonder whether in an error case there's not a possibility for memory to be leaked. That wouldn't be a problem your patch introduces, but I think you could have noticed and fixed this as you touch the code anyway. [Paul2] Good catch > --- 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( struct domain *d, unsigned int i) Another instance of you not having corrected what was previously pointed out: There's a stray blank here. And even if I hadn't said there are multiple instances of this, you should still apply such comments to all of your series. And I only now notice that this e.g. also applies to the suggested re-ordering of operations in altp2m_domain_teardown(). I think I'm going to stop reviewing this series here: Please make sure you address all review comments (either verbally or by code adjustment) before submitting a new version (or in extreme cases, like due to lack of feedback, point out open issues right after the first --- separator). [Paul2] I did do a sweep for the stray blanks (and other like typos/style issues), but your eyes are much better trained. Will do again. [Paul 2] Thanks for the quick feedback. Jan
diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c index 930bdc2..bf3f46e 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,56 @@ altp2m_vcpu_destroy(struct vcpu *v) vcpu_unpause(v); } +int +altp2m_domain_init(struct domain *d) +{ + int rc; + unsigned int i; + + if ( !hvm_altp2m_supported() ) + { + rc = 0; + goto out; + } + /* 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 +altp2m_domain_teardown(struct domain *d) +{ + unsigned int i; + + if ( !hvm_altp2m_supported() ) + return; + + d->arch.altp2m_active = 0; + + 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..283ec38 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> @@ -499,27 +500,7 @@ int hap_enable(struct domain *d, u32 mode) goto out; } - 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 = altp2m_domain_init(d); /* Now let other users see the new mode */ d->arch.paging.mode = mode | PG_HAP_enable; @@ -533,19 +514,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]); - } + altp2m_domain_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..7ae3168 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( 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 7d14c3b..9a97962 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; } } @@ -2261,33 +2261,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; @@ -2393,18 +2366,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; @@ -2416,7 +2377,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(d, idx); rc = 0; } @@ -2436,7 +2397,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(d, i); *idx = i; rc = 0; diff --git a/xen/include/asm-x86/altp2m.h b/xen/include/asm-x86/altp2m.h index 64c7618..0090c89 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 altp2m_domain_init(struct domain *d); +void altp2m_domain_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..4d247b5 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( 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 9fc9ead..c138522 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> @@ -784,9 +784,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); @@ -848,7 +845,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 | 51 +++++++++++++++++++++++++++++++++++++++ xen/arch/x86/mm/hap/hap.c | 37 +++------------------------- 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, 105 insertions(+), 83 deletions(-)