From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Egger Subject: Re: [PATCH] x86: fix nested HVM initialization Date: Thu, 24 May 2012 14:44:28 +0200 Message-ID: <4FBE2D2C.7060704@amd.com> References: <4FBE424A0200007800085DDE@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <4FBE424A0200007800085DDE@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel List-Id: xen-devel@lists.xenproject.org On 05/24/12 14:14, Jan Beulich wrote: > - no need for calling nestedhvm_setup() explicitly (can be a normal > init-call, and can be __init) > - calling _xmalloc() for multi-page, page-aligned memory regions is > inefficient - use alloc_xenheap_pages() instead > - albeit an allocation error is unlikely here, add error handling > nevertheless (and have nestedhvm_vcpu_initialise() bail if an error > occurred during setup) > - nestedhvm_enabled() must no access d->arch.hvm_domain without first > checking that 'd' actually represents a HVM domain > > Signed-off-by: Jan Beulich Looks ok to me. How did you test it? Christoph > > --- a/xen/arch/x86/hvm/nestedhvm.c > +++ b/xen/arch/x86/hvm/nestedhvm.c > @@ -25,16 +25,14 @@ > #include /* for local_event_delivery_(en|dis)able */ > #include /* for paging_mode_hap() */ > > +static unsigned long *shadow_io_bitmap[3]; > + > /* Nested HVM on/off per domain */ > bool_t > nestedhvm_enabled(struct domain *d) > { > - bool_t enabled; > - > - enabled = !!(d->arch.hvm_domain.params[HVM_PARAM_NESTEDHVM]); > - BUG_ON(enabled && !is_hvm_domain(d)); > - > - return enabled; > + return is_hvm_domain(d) && > + d->arch.hvm_domain.params[HVM_PARAM_NESTEDHVM]; > } > > /* Nested VCPU */ > @@ -76,6 +74,9 @@ nestedhvm_vcpu_initialise(struct vcpu *v > { > int rc = -EOPNOTSUPP; > > + if ( !shadow_io_bitmap[0] ) > + return -ENOMEM; > + > if ( !hvm_funcs.nhvm_vcpu_initialise || > ((rc = hvm_funcs.nhvm_vcpu_initialise(v)) != 0) ) > return rc; > @@ -147,15 +148,15 @@ nestedhvm_is_n2(struct vcpu *v) > * iomap[2] set set > */ > > -static unsigned long *shadow_io_bitmap[3]; > - > -void > +static int __init > nestedhvm_setup(void) > { > /* Same format and size as hvm_io_bitmap (Intel needs only 2 pages). */ > - unsigned int sz = (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) > - ? 2*PAGE_SIZE : 3*PAGE_SIZE; > - unsigned int i; > + unsigned int nr = (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) ? 2 : 3; > + unsigned int i, order = get_order_from_pages(nr); > + > + if ( !hvm_funcs.name ) > + return 0; > > /* shadow_io_bitmaps can't be declared static because > * they must fulfill hw requirements (page aligned section) > @@ -169,13 +170,25 @@ nestedhvm_setup(void) > > for ( i = 0; i < ARRAY_SIZE(shadow_io_bitmap); i++ ) > { > - shadow_io_bitmap[i] = _xmalloc(sz, PAGE_SIZE); > - memset(shadow_io_bitmap[i], ~0U, sz); > + shadow_io_bitmap[i] = alloc_xenheap_pages(order, 0); > + if ( !shadow_io_bitmap[i] ) > + { > + while ( i-- ) > + { > + free_xenheap_pages(shadow_io_bitmap[i], order); > + shadow_io_bitmap[i] = NULL; > + } > + return -ENOMEM; > + } > + memset(shadow_io_bitmap[i], ~0U, nr << PAGE_SHIFT); > } > > __clear_bit(0x80, shadow_io_bitmap[0]); > __clear_bit(0xed, shadow_io_bitmap[1]); > + > + return 0; > } > +__initcall(nestedhvm_setup); > > unsigned long * > nestedhvm_vcpu_iomap_get(bool_t port_80, bool_t port_ed) > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -1312,8 +1312,6 @@ void __init __start_xen(unsigned long mb > if ( opt_watchdog ) > watchdog_setup(); > > - nestedhvm_setup(); > - > if ( !tboot_protect_mem_regions() ) > panic("Could not protect TXT memory regions\n"); > > --- a/xen/include/asm-x86/setup.h > +++ b/xen/include/asm-x86/setup.h > @@ -21,7 +21,6 @@ void set_nr_cpu_ids(unsigned int max_cpu > void numa_initmem_init(unsigned long start_pfn, unsigned long end_pfn); > void arch_init_memory(void); > void subarch_init_memory(void); > -void nestedhvm_setup(void); > > void init_IRQ(void); > void vesa_init(void); > > > -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85689 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632