| Message ID | 1310571145-28930-3-git-send-email-joerg.roedel@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/13/2011 06:32 PM, Joerg Roedel wrote: > This saves copying over the vmload/vmsave switched part from > the host to the guest vmcb later. > > Signed-off-by: Joerg Roedel<joerg.roedel@amd.com> > --- > arch/x86/kvm/svm.c | 7 ++++++- > 1 files changed, 6 insertions(+), 1 deletions(-) > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index 3d5990f..dc703ac 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -3704,9 +3704,13 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu) > > /* Enter guest mode */ > "push %%"R"ax \n\t" > - "mov %c[vmcb](%[svm]), %%"R"ax \n\t" > + "mov %c[host_vmcb](%[svm]), %%"R"ax \n\t" > __ex(SVM_VMLOAD) "\n\t" > + "mov (%%"R"sp), %%"R"ax\n\t" > + "mov %c[vmcb](%[svm]), %%"R"ax \n\t" > __ex(SVM_VMRUN) "\n\t" > + "mov (%%"R"sp), %%"R"ax\n\t" > + "mov %c[host_vmcb](%[svm]), %%"R"ax \n\t" > __ex(SVM_VMSAVE) "\n\t" > "pop %%"R"ax \n\t" > Okay, so the plan is to split L2 state between ->vmcb and ->host_vmcb? In that case my suggestion for patch 1 doesn't apply. But the name still is confusing. If we don't find a better one, I want a fat comment explaining how state is split. (would be good to have documentation for the overall strategy of nsvm, like we have for nvmx and nmmu).
On Thu, Jul 14, 2011 at 02:29:36PM +0300, Avi Kivity wrote: > On 07/13/2011 06:32 PM, Joerg Roedel wrote: >> This saves copying over the vmload/vmsave switched part from >> the host to the guest vmcb later. >> >> Signed-off-by: Joerg Roedel<joerg.roedel@amd.com> >> --- >> arch/x86/kvm/svm.c | 7 ++++++- >> 1 files changed, 6 insertions(+), 1 deletions(-) >> >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >> index 3d5990f..dc703ac 100644 >> --- a/arch/x86/kvm/svm.c >> +++ b/arch/x86/kvm/svm.c >> @@ -3704,9 +3704,13 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu) >> >> /* Enter guest mode */ >> "push %%"R"ax \n\t" >> - "mov %c[vmcb](%[svm]), %%"R"ax \n\t" >> + "mov %c[host_vmcb](%[svm]), %%"R"ax \n\t" >> __ex(SVM_VMLOAD) "\n\t" >> + "mov (%%"R"sp), %%"R"ax\n\t" >> + "mov %c[vmcb](%[svm]), %%"R"ax \n\t" >> __ex(SVM_VMRUN) "\n\t" >> + "mov (%%"R"sp), %%"R"ax\n\t" >> + "mov %c[host_vmcb](%[svm]), %%"R"ax \n\t" >> __ex(SVM_VMSAVE) "\n\t" >> "pop %%"R"ax \n\t" >> > > Okay, so the plan is to split L2 state between ->vmcb and ->host_vmcb? Yes, otherwise we need to copy the vmload/vmsave switched state back and forth between both VMCBs which is a waste of cycles. > In that case my suggestion for patch 1 doesn't apply. But the name > still is confusing. If we don't find a better one, I want a fat comment > explaining how state is split. Hmm, how about naming them l1_vmcb and l2_vmcb? The comment explaining why vmload/vmsave always happens on l1_vmcb is needed anyway then. > (would be good to have documentation for the overall strategy of nsvm, > like we have for nvmx and nmmu). There is not much to document about future plans for nested-svm. At the moment I try to add emulation code for new SVM features when there is some time left. Live migration support is also on the list. The long-term plan is certainly to merge code with nested-vmx where possible and move logic into generic KVM code. The first item that comes to mind here is to create a single place where a vmexit is emulated and let all other place which do that today just signal that it is required. Joerg -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/14/2011 04:10 PM, Joerg Roedel wrote: > On Thu, Jul 14, 2011 at 02:29:36PM +0300, Avi Kivity wrote: > > On 07/13/2011 06:32 PM, Joerg Roedel wrote: > >> This saves copying over the vmload/vmsave switched part from > >> the host to the guest vmcb later. > >> > >> Signed-off-by: Joerg Roedel<joerg.roedel@amd.com> > >> --- > >> arch/x86/kvm/svm.c | 7 ++++++- > >> 1 files changed, 6 insertions(+), 1 deletions(-) > >> > >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > >> index 3d5990f..dc703ac 100644 > >> --- a/arch/x86/kvm/svm.c > >> +++ b/arch/x86/kvm/svm.c > >> @@ -3704,9 +3704,13 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu) > >> > >> /* Enter guest mode */ > >> "push %%"R"ax \n\t" > >> - "mov %c[vmcb](%[svm]), %%"R"ax \n\t" > >> + "mov %c[host_vmcb](%[svm]), %%"R"ax \n\t" > >> __ex(SVM_VMLOAD) "\n\t" > >> + "mov (%%"R"sp), %%"R"ax\n\t" > >> + "mov %c[vmcb](%[svm]), %%"R"ax \n\t" > >> __ex(SVM_VMRUN) "\n\t" > >> + "mov (%%"R"sp), %%"R"ax\n\t" > >> + "mov %c[host_vmcb](%[svm]), %%"R"ax \n\t" > >> __ex(SVM_VMSAVE) "\n\t" > >> "pop %%"R"ax \n\t" > >> > > > > Okay, so the plan is to split L2 state between ->vmcb and ->host_vmcb? > > Yes, otherwise we need to copy the vmload/vmsave switched state back and > forth between both VMCBs which is a waste of cycles. Just to be sure I understand this: the root cause is because VMRUN doesn't actually switch this state. So we have to copy the state. Okay. What about an L2 guest executing VMLOAD or VMSAVE which isn't intercepted? Don't we have to redirect it's reads and writes to host_vmcb? > > In that case my suggestion for patch 1 doesn't apply. But the name > > still is confusing. If we don't find a better one, I want a fat comment > > explaining how state is split. > > Hmm, how about naming them l1_vmcb and l2_vmcb? The comment explaining > why vmload/vmsave always happens on l1_vmcb is needed anyway then. In a later patch you introduce n_vmcb. I think it makes sense to name that vmcb02? > > (would be good to have documentation for the overall strategy of nsvm, > > like we have for nvmx and nmmu). > > There is not much to document about future plans for nested-svm. At the > moment I try to add emulation code for new SVM features when there is > some time left. Live migration support is also on the list. > Even the exising code would be good to document. So when a reader sees some bit, they can compare it to the document and see why it's that way. > The long-term plan is certainly to merge code with nested-vmx where > possible and move logic into generic KVM code. The first item that comes > to mind here is to create a single place where a vmexit is emulated and > let all other place which do that today just signal that it is required. I'm not very concerned about reuse with nvmx except for architectural code like interrupts. Of course, if it turns out simple I'm all for it, but if it's hard or uglifies the code, let it be.
On Thu, Jul 14, 2011 at 04:20:03PM +0300, Avi Kivity wrote: > On 07/14/2011 04:10 PM, Joerg Roedel wrote: >> Yes, otherwise we need to copy the vmload/vmsave switched state back and >> forth between both VMCBs which is a waste of cycles. > > Just to be sure I understand this: the root cause is because VMRUN > doesn't actually switch this state. So we have to copy the state. Okay. Right. > What about an L2 guest executing VMLOAD or VMSAVE which isn't > intercepted? Don't we have to redirect it's reads and writes to > host_vmcb? Yes, that needs to target the host_vmcb then. This is buggy in the patch-set. Thanks for pointing this out :) >> Hmm, how about naming them l1_vmcb and l2_vmcb? The comment explaining >> why vmload/vmsave always happens on l1_vmcb is needed anyway then. > > In a later patch you introduce n_vmcb. I think it makes sense to name > that vmcb02? Just for my understanding, what stands the first '0' for? The '1' and '2' make sense, but the '0' seems to be redundant? > Even the exising code would be good to document. So when a reader sees > some bit, they can compare it to the document and see why it's that way. I tried to put comments into the code to document the most complicated parts. But there is certainly room for improvement. Overall, I think the best place is to keep those comments in the code and not open another document for it. >> The long-term plan is certainly to merge code with nested-vmx where >> possible and move logic into generic KVM code. The first item that comes >> to mind here is to create a single place where a vmexit is emulated and >> let all other place which do that today just signal that it is required. > > I'm not very concerned about reuse with nvmx except for architectural > code like interrupts. Of course, if it turns out simple I'm all for it, > but if it's hard or uglifies the code, let it be. Yes, the interrupt code is another part that probably can be made generic. The nested-mmu code is already generic. Nested-VMX should be able to make use of it with only minor modifications. Joerg -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/14/2011 04:52 PM, Joerg Roedel wrote: > > What about an L2 guest executing VMLOAD or VMSAVE which isn't > > intercepted? Don't we have to redirect it's reads and writes to > > host_vmcb? > > Yes, that needs to target the host_vmcb then. This is buggy in the > patch-set. Thanks for pointing this out :) For the low price of an additional test to svm.flat. > >> Hmm, how about naming them l1_vmcb and l2_vmcb? The comment explaining > >> why vmload/vmsave always happens on l1_vmcb is needed anyway then. > > > > In a later patch you introduce n_vmcb. I think it makes sense to name > > that vmcb02? > > Just for my understanding, what stands the first '0' for? The '1' and > '2' make sense, but the '0' seems to be redundant? The first number is the level running in host mode, the second is the level running guest mode. vmcb01: host running guest vmcb02: host running nested guest vmcb12: guest running nested guest (i.e. the virtual vmcb in guest physical address space) > > Even the exising code would be good to document. So when a reader sees > > some bit, they can compare it to the document and see why it's that way. > > I tried to put comments into the code to document the most complicated > parts. But there is certainly room for improvement. Overall, I think the > best place is to keep those comments in the code and not open another > document for it. Those are good for the details, but not so good for the master plan. Like mmu.txt. > >> The long-term plan is certainly to merge code with nested-vmx where > >> possible and move logic into generic KVM code. The first item that comes > >> to mind here is to create a single place where a vmexit is emulated and > >> let all other place which do that today just signal that it is required. > > > > I'm not very concerned about reuse with nvmx except for architectural > > code like interrupts. Of course, if it turns out simple I'm all for it, > > but if it's hard or uglifies the code, let it be. > > Yes, the interrupt code is another part that probably can be made > generic. Yes. > The nested-mmu code is already generic. Nested-VMX should be able to > make use of it with only minor modifications. Yup, just need support for parsing the EPT PTE format.
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index 3d5990f..dc703ac 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -3704,9 +3704,13 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu) /* Enter guest mode */ "push %%"R"ax \n\t" - "mov %c[vmcb](%[svm]), %%"R"ax \n\t" + "mov %c[host_vmcb](%[svm]), %%"R"ax \n\t" __ex(SVM_VMLOAD) "\n\t" + "mov (%%"R"sp), %%"R"ax\n\t" + "mov %c[vmcb](%[svm]), %%"R"ax \n\t" __ex(SVM_VMRUN) "\n\t" + "mov (%%"R"sp), %%"R"ax\n\t" + "mov %c[host_vmcb](%[svm]), %%"R"ax \n\t" __ex(SVM_VMSAVE) "\n\t" "pop %%"R"ax \n\t" @@ -3731,6 +3735,7 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu) : : [svm]"a"(svm), [vmcb]"i"(offsetof(struct vcpu_svm, vmcb_pa)), + [host_vmcb]"i"(offsetof(struct vcpu_svm, host_vmcb_pa)), [rbx]"i"(offsetof(struct vcpu_svm, vcpu.arch.regs[VCPU_REGS_RBX])), [rcx]"i"(offsetof(struct vcpu_svm, vcpu.arch.regs[VCPU_REGS_RCX])), [rdx]"i"(offsetof(struct vcpu_svm, vcpu.arch.regs[VCPU_REGS_RDX])),
This saves copying over the vmload/vmsave switched part from the host to the guest vmcb later. Signed-off-by: Joerg Roedel <joerg.roedel@amd.com> --- arch/x86/kvm/svm.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)