Re: [Xen-devel] [PATCH 04/16] vmx: nest: nested control structure

[Christoph Egger <Christoph.Egger@xxxxxxx>]

On Wednesday 15 September 2010 15:17:42 Christoph Egger wrote:
> On Wednesday 15 September 2010 15:06:03 Dong, Eddie wrote:
> > Christoph Egger wrote:
> > > On Wednesday 08 September 2010 17:22:12 Qing He wrote:
> > >> v->arch.hvm_vmx.nest as control structure
> > >>
> > >> Signed-off-by: Qing He <qing.he@xxxxxxxxx>
> > >> Signed-off-by: Eddie Dong <eddie.dong@xxxxxxxxx>
> > >>
> > >> ---
> > >> diff -r fc4de5eedd1d xen/include/asm-x86/hvm/vmx/nest.h
> > >> --- /dev/null    Thu Jan 01 00:00:00 1970 +0000
> > >> +++ b/xen/include/asm-x86/hvm/vmx/nest.h Wed Sep 08 21:03:41 2010
> > >> +0800 @@ -0,0 +1,45 @@ +/*
> > >> + * nest.h: nested virtualization for VMX.
> > >> + *
> > >> + * Copyright (c) 2010, Intel Corporation.
> > >> + * Author: Qing He <qing.he@xxxxxxxxx>
> > >> + *
> > >> + * This program is free software; you can redistribute it and/or
> > >> modify it + * under the terms and conditions of the GNU General
> > >> Public License, + * version 2, as published by the Free Software
> > >> Foundation. + * + * This program is distributed in the hope it will
> > >> be useful, but WITHOUT + * ANY WARRANTY; without even the implied
> > >> warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE.
> > >> See the GNU General Public License for + * more details. + *
> > >> + * You should have received a copy of the GNU General Public
> > >> License along with + * this program; if not, write to the Free
> > >> Software Foundation, Inc., 59 Temple + * Place - Suite 330, Boston,
> > >> MA 02111-1307 USA. + * + */
> > >> +#ifndef __ASM_X86_HVM_NEST_H__
> > >> +#define __ASM_X86_HVM_NEST_H__
> > >> +
> > >> +struct vmcs_struct;
> > >> +
> > >> +struct vmx_nest_struct {
> > >
> > > Is it ok to name it 'struct nestedvmx' ?
> >
> > Fine, renamed to nested_vmx.
> >
> > >> +    paddr_t              guest_vmxon_pa;
> > >> +
> > >> +    /* Saved host vmcs for vcpu itself */
> > >> +    struct vmcs_struct  *hvmcs;
> > >> +
> > >> +    /*
> > >> +     * Guest's `current vmcs' of vcpu
> > >> +     *  - gvmcs_pa: guest VMCS region physical address
> > >> +     *  - vvmcs:    (guest) virtual vmcs
> > >> +     *  - svmcs:    effective vmcs for the guest of this vcpu
> > >> +     *  - valid:    launch state: invalid on clear, valid on ld +
> > >> */ +    paddr_t              gvmcs_pa;
> > >> +    void                *vvmcs;
> > >> +    struct vmcs_struct  *svmcs;
> > >> +    int                  vmcs_valid;
> > >> +};
> > >> +
> > >> +#endif /* __ASM_X86_HVM_NEST_H__ */
> > >> diff -r fc4de5eedd1d xen/include/asm-x86/hvm/vmx/vmcs.h
> > >> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h Wed Sep 08 21:00:00 2010
> > >> +0800 +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h   Wed Sep 08 21:03:41
> > >>  2010 +0800 @@ -22,6 +22,7 @@ #include <asm/config.h>
> > >>  #include <asm/hvm/io.h>
> > >>  #include <asm/hvm/vpmu.h>
> > >> +#include <asm/hvm/vmx/nest.h>
> > >>
> > >>  extern void vmcs_dump_vcpu(struct vcpu *v);
> > >>  extern void setup_vmcs_dump(void);
> > >> @@ -99,6 +100,9 @@
> > >>      u32                  secondary_exec_control;
> > >>      u32                  exception_bitmap;
> > >>
> > >> +    /* nested virtualization */
> > >> +    struct vmx_nest_struct nest;
> > >> +
> > >>  #ifdef __x86_64__
> > >>      struct vmx_msr_state msr_state;
> > >>      unsigned long        shadow_gs;
> > >
> > > I think, the structure should be allocated in the
> > > nestedhvm_vcpu_initialise() function hook and assigned to the nh_arch
> > > pointer in struct nestedhvm.
> >
> > Well, the structure itself is pretty small, so dynamic allocation is
> > really not a good idea.

It's not a question of size. The point is that it is opaque to non-vmx code.

> > Instead, the internal field such as vvmcs/svmcs 
> > are big and thus we use a pointer, but they are allocated on demand. This
> > follows the style of arch_vmx_struct in vcpu data structure.

That's fine though. However you can reuse some fields of struct nestedhvm:

gvmcs_pa  is functional the same as nh_vmaddr
svmcs is functional the same as nh_vm

> > I am fine with your nestedhvm_vcpu_initialise "design", but VMX doesn't
> > need to use the wrapper so far.
>
> It should be implemented good enough that vcpu creation doesn't fail.
>
> Christoph



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel