Re: [PATCH 7/9] Add VMRUN handler v3
Alexander Graf wrote: Is copying one page really that expensive? Is there any accelerated function available for that that copies it with SSE or so? :-) 'rep movs' is supposed to be accelerated, doing cacheline-by-cacheline copies (at least on Intel). In any case the kernel memcpy() should pick the fastest method. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/9] Add VMRUN handler v3
[EMAIL PROTECTED] wrote: Copying data in memory is always expensive because the accesses may miss in the caches and data must be fetched from memory. As far as I know this can be around 150 cycles per cache line. When the copy is sequential, the processor will prefetch the data ahead of time, so you don't get the full hit. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/9] Add VMRUN handler v3
Am 19.09.2008 um 17:59 schrieb Joerg Roedel [EMAIL PROTECTED]: On Wed, Sep 17, 2008 at 03:41:24PM +0200, Alexander Graf wrote: This patch implements VMRUN. VMRUN enters a virtual CPU and runs that in the same context as the normal guest CPU would run. So basically it is implemented the same way, a normal CPU would do it. We also prepare all intercepts that get OR'ed with the original intercepts, as we do not allow a level 2 guest to be intercepted less than the first level guest. v2 implements the following improvements: - fixes the CPL check - does not allocate iopm when not used - remembers the host's IF in the HIF bit in the hflags v3: - make use of the new permission checking - add support for V_INTR_MASKING_MASK Signed-off-by: Alexander Graf [EMAIL PROTECTED] --- arch/x86/kvm/kvm_svm.h |9 ++ arch/x86/kvm/svm.c | 198 ++ +- include/asm-x86/kvm_host.h |2 + 3 files changed, 207 insertions(+), 2 deletions(-) diff --git a/arch/x86/kvm/kvm_svm.h b/arch/x86/kvm/kvm_svm.h index 76ad107..2afe0ce 100644 --- a/arch/x86/kvm/kvm_svm.h +++ b/arch/x86/kvm/kvm_svm.h @@ -43,6 +43,15 @@ struct vcpu_svm { u32 *msrpm; u64 nested_hsave; +u64 nested_vmcb; + +/* These are the merged vectors */ +u32 *nested_msrpm; +u32 *nested_iopm; + +/* gpa pointers to the real vectors */ +u64 nested_vmcb_msrpm; +u64 nested_vmcb_iopm; }; #endif diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index 0aa22e5..3601e75 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -51,6 +51,9 @@ MODULE_LICENSE(GPL); /* Turn on to get debugging output*/ /* #define NESTED_DEBUG */ +/* Not needed until device passthrough */ +/* #define NESTED_KVM_MERGE_IOPM */ + #ifdef NESTED_DEBUG #define nsvm_printk(fmt, args...) printk(KERN_INFO fmt, ## args) #else @@ -76,6 +79,11 @@ static inline struct vcpu_svm *to_svm(struct kvm_vcpu *vcpu) return container_of(vcpu, struct vcpu_svm, vcpu); } +static inline bool is_nested(struct vcpu_svm *svm) +{ +return svm-nested_vmcb; +} + static unsigned long iopm_base; struct kvm_ldttss_desc { @@ -614,6 +622,7 @@ static void init_vmcb(struct vcpu_svm *svm) force_new_asid(svm-vcpu); svm-nested_hsave = 0; +svm-nested_vmcb = 0; svm-vcpu.arch.hflags = HF_GIF_MASK; } @@ -639,6 +648,10 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) struct vcpu_svm *svm; struct page *page; struct page *msrpm_pages; +struct page *nested_msrpm_pages; +#ifdef NESTED_KVM_MERGE_IOPM +struct page *nested_iopm_pages; +#endif int err; svm = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL); @@ -661,9 +674,25 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) msrpm_pages = alloc_pages(GFP_KERNEL, MSRPM_ALLOC_ORDER); if (!msrpm_pages) goto uninit; + +nested_msrpm_pages = alloc_pages(GFP_KERNEL, MSRPM_ALLOC_ORDER); +if (!nested_msrpm_pages) +goto uninit; + +#ifdef NESTED_KVM_MERGE_IOPM +nested_iopm_pages = alloc_pages(GFP_KERNEL, IOPM_ALLOC_ORDER); +if (!nested_iopm_pages) +goto uninit; +#endif + svm-msrpm = page_address(msrpm_pages); svm_vcpu_init_msrpm(svm-msrpm); +svm-nested_msrpm = page_address(nested_msrpm_pages); +#ifdef NESTED_KVM_MERGE_IOPM +svm-nested_iopm = page_address(nested_iopm_pages); +#endif + svm-vmcb = page_address(page); clear_page(svm-vmcb); svm-vmcb_pa = page_to_pfn(page) PAGE_SHIFT; @@ -693,6 +722,10 @@ static void svm_free_vcpu(struct kvm_vcpu *vcpu) __free_page(pfn_to_page(svm-vmcb_pa PAGE_SHIFT)); __free_pages(virt_to_page(svm-msrpm), MSRPM_ALLOC_ORDER); +__free_pages(virt_to_page(svm-nested_msrpm), MSRPM_ALLOC_ORDER); +#ifdef NESTED_KVM_MERGE_IOPM +__free_pages(virt_to_page(svm-nested_iopm), IOPM_ALLOC_ORDER); +#endif kvm_vcpu_uninit(vcpu); kmem_cache_free(kvm_vcpu_cache, svm); } @@ -1230,6 +1263,138 @@ static int nested_svm_do(struct vcpu_svm *svm, return retval; } + +static int nested_svm_vmrun_msrpm(struct vcpu_svm *svm, void *arg1, + void *arg2, void *opaque) +{ +int i; +u32 *nested_msrpm = (u32*)arg1; +for (i=0; i PAGE_SIZE * (1 MSRPM_ALLOC_ORDER) / 4; i++) +svm-nested_msrpm[i] = svm-msrpm[i] | nested_msrpm[i]; +svm-vmcb-control.msrpm_base_pa = __pa(svm-nested_msrpm); + +return 0; +} + +#ifdef NESTED_KVM_MERGE_IOPM +static int nested_svm_vmrun_iopm(struct vcpu_svm *svm, void *arg1, + void *arg2, void *opaque) +{ +int i; +u32 *nested_iopm = (u32*)arg1; +u32 *iopm = (u32*)__va(iopm_base); +for (i=0; i PAGE_SIZE * (1 IOPM_ALLOC_ORDER) / 4; i++) +svm-nested_iopm[i] = iopm[i] | nested_iopm[i]; +svm-vmcb-control.iopm_base_pa = __pa(svm-nested_iopm); + +return 0; +} +#endif + +static int nested_svm_vmrun(struct vcpu_svm *svm, void *arg1, +void *arg2, void *opaque) +{ +struct vmcb *nested_vmcb =
Re: [PATCH 7/9] Add VMRUN handler v3
On Thu, Sep 25, 2008 at 07:32:55PM +0200, Alexander Graf wrote: This is a big security hole. With this we give the guest access to its own VMCB. The guest can take over or crash the whole host machine by rewriting its VMCB. We should be more selective what we save in the hsave area. Oh, right. I didn't even think of a case where the nested guest would have acvess to the hsave of itself. Since the hsave can never be used twice on one vcpu, we could just allocate our own memory for the hsave in the vcpu context and leave the nested hsave empty. I think we could also gain performance by only saving the important parts of the VMCB and not the whole page. Joerg -- | AMD Saxony Limited Liability Company Co. KG Operating | Wilschdorfer Landstr. 101, 01109 Dresden, Germany System| Register Court Dresden: HRA 4896 Research | General Partner authorized to represent: Center| AMD Saxony LLC (Wilmington, Delaware, US) | General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/9] Add VMRUN handler v3
On Thu, Sep 25, 2008 at 10:00:17PM +0200, Alexander Graf wrote: On 25.09.2008, at 19:37, Joerg Roedel wrote: On Thu, Sep 25, 2008 at 07:32:55PM +0200, Alexander Graf wrote: This is a big security hole. With this we give the guest access to its own VMCB. The guest can take over or crash the whole host machine by rewriting its VMCB. We should be more selective what we save in the hsave area. Oh, right. I didn't even think of a case where the nested guest would have acvess to the hsave of itself. Since the hsave can never be used twice on one vcpu, we could just allocate our own memory for the hsave in the vcpu context and leave the nested hsave empty. I think we could also gain performance by only saving the important parts of the VMCB and not the whole page. Is copying one page really that expensive? Is there any accelerated function available for that that copies it with SSE or so? :-) Copying data in memory is always expensive because the accesses may miss in the caches and data must be fetched from memory. As far as I know this can be around 150 cycles per cache line. Joerg -- | AMD Saxony Limited Liability Company Co. KG Operating | Wilschdorfer Landstr. 101, 01109 Dresden, Germany System| Register Court Dresden: HRA 4896 Research | General Partner authorized to represent: Center| AMD Saxony LLC (Wilmington, Delaware, US) | General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/9] Add VMRUN handler v3
On Wed, Sep 17, 2008 at 03:41:24PM +0200, Alexander Graf wrote: This patch implements VMRUN. VMRUN enters a virtual CPU and runs that in the same context as the normal guest CPU would run. So basically it is implemented the same way, a normal CPU would do it. We also prepare all intercepts that get OR'ed with the original intercepts, as we do not allow a level 2 guest to be intercepted less than the first level guest. v2 implements the following improvements: - fixes the CPL check - does not allocate iopm when not used - remembers the host's IF in the HIF bit in the hflags v3: - make use of the new permission checking - add support for V_INTR_MASKING_MASK Signed-off-by: Alexander Graf [EMAIL PROTECTED] --- arch/x86/kvm/kvm_svm.h |9 ++ arch/x86/kvm/svm.c | 198 +++- include/asm-x86/kvm_host.h |2 + 3 files changed, 207 insertions(+), 2 deletions(-) diff --git a/arch/x86/kvm/kvm_svm.h b/arch/x86/kvm/kvm_svm.h index 76ad107..2afe0ce 100644 --- a/arch/x86/kvm/kvm_svm.h +++ b/arch/x86/kvm/kvm_svm.h @@ -43,6 +43,15 @@ struct vcpu_svm { u32 *msrpm; u64 nested_hsave; + u64 nested_vmcb; + + /* These are the merged vectors */ + u32 *nested_msrpm; + u32 *nested_iopm; + + /* gpa pointers to the real vectors */ + u64 nested_vmcb_msrpm; + u64 nested_vmcb_iopm; }; #endif diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index 0aa22e5..3601e75 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -51,6 +51,9 @@ MODULE_LICENSE(GPL); /* Turn on to get debugging output*/ /* #define NESTED_DEBUG */ +/* Not needed until device passthrough */ +/* #define NESTED_KVM_MERGE_IOPM */ + #ifdef NESTED_DEBUG #define nsvm_printk(fmt, args...) printk(KERN_INFO fmt, ## args) #else @@ -76,6 +79,11 @@ static inline struct vcpu_svm *to_svm(struct kvm_vcpu *vcpu) return container_of(vcpu, struct vcpu_svm, vcpu); } +static inline bool is_nested(struct vcpu_svm *svm) +{ + return svm-nested_vmcb; +} + static unsigned long iopm_base; struct kvm_ldttss_desc { @@ -614,6 +622,7 @@ static void init_vmcb(struct vcpu_svm *svm) force_new_asid(svm-vcpu); svm-nested_hsave = 0; + svm-nested_vmcb = 0; svm-vcpu.arch.hflags = HF_GIF_MASK; } @@ -639,6 +648,10 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) struct vcpu_svm *svm; struct page *page; struct page *msrpm_pages; + struct page *nested_msrpm_pages; +#ifdef NESTED_KVM_MERGE_IOPM + struct page *nested_iopm_pages; +#endif int err; svm = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL); @@ -661,9 +674,25 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) msrpm_pages = alloc_pages(GFP_KERNEL, MSRPM_ALLOC_ORDER); if (!msrpm_pages) goto uninit; + + nested_msrpm_pages = alloc_pages(GFP_KERNEL, MSRPM_ALLOC_ORDER); + if (!nested_msrpm_pages) + goto uninit; + +#ifdef NESTED_KVM_MERGE_IOPM + nested_iopm_pages = alloc_pages(GFP_KERNEL, IOPM_ALLOC_ORDER); + if (!nested_iopm_pages) + goto uninit; +#endif + svm-msrpm = page_address(msrpm_pages); svm_vcpu_init_msrpm(svm-msrpm); + svm-nested_msrpm = page_address(nested_msrpm_pages); +#ifdef NESTED_KVM_MERGE_IOPM + svm-nested_iopm = page_address(nested_iopm_pages); +#endif + svm-vmcb = page_address(page); clear_page(svm-vmcb); svm-vmcb_pa = page_to_pfn(page) PAGE_SHIFT; @@ -693,6 +722,10 @@ static void svm_free_vcpu(struct kvm_vcpu *vcpu) __free_page(pfn_to_page(svm-vmcb_pa PAGE_SHIFT)); __free_pages(virt_to_page(svm-msrpm), MSRPM_ALLOC_ORDER); + __free_pages(virt_to_page(svm-nested_msrpm), MSRPM_ALLOC_ORDER); +#ifdef NESTED_KVM_MERGE_IOPM + __free_pages(virt_to_page(svm-nested_iopm), IOPM_ALLOC_ORDER); +#endif kvm_vcpu_uninit(vcpu); kmem_cache_free(kvm_vcpu_cache, svm); } @@ -1230,6 +1263,138 @@ static int nested_svm_do(struct vcpu_svm *svm, return retval; } + +static int nested_svm_vmrun_msrpm(struct vcpu_svm *svm, void *arg1, + void *arg2, void *opaque) +{ + int i; + u32 *nested_msrpm = (u32*)arg1; + for (i=0; i PAGE_SIZE * (1 MSRPM_ALLOC_ORDER) / 4; i++) + svm-nested_msrpm[i] = svm-msrpm[i] | nested_msrpm[i]; + svm-vmcb-control.msrpm_base_pa = __pa(svm-nested_msrpm); + + return 0; +} + +#ifdef NESTED_KVM_MERGE_IOPM +static int nested_svm_vmrun_iopm(struct vcpu_svm *svm, void *arg1, + void *arg2, void *opaque) +{ + int i; + u32 *nested_iopm = (u32*)arg1; + u32 *iopm = (u32*)__va(iopm_base); + for (i=0; i PAGE_SIZE * (1 IOPM_ALLOC_ORDER) / 4; i++) + svm-nested_iopm[i] = iopm[i]