| Message ID | 1251989512-22072-1-git-send-email-agraf@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Indeed. Good catch. Thanks :-) On Thu, Sep 03, 2009 at 04:51:52PM +0200, Alexander Graf wrote: > Thanks to Joerg's previous series of cleanups, we now have almost all > information we need to decide what to do on #VMEXIT because we get > the variables from the VMCB on VMRUN. > > Unfortunately there's one piece that slipped through the conversion, > namely the MSR intercept which still tries to map the nested VMCB > to find out if MSRs are intercepted. > > So let's use the cached value, removing the need for two atomic maps > (which breaks anyways) and fix an oops along the way. > > CC: Joerg Roedel <joerg.roedel@amd.com> > Signed-off-by: Alexander Graf <agraf@suse.de> Acked-by: Joerg Roedel <joerg.roedel@amd.com> > > --- > > v1 -> v2: > > - Don't break when MSR is not intercepted > --- > arch/x86/kvm/svm.c | 15 ++++++--------- > 1 files changed, 6 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index 2df9b45..a5f90c7 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -1427,19 +1427,17 @@ static bool nested_svm_exit_handled_msr(struct vcpu_svm *svm) > { > u32 param = svm->vmcb->control.exit_info_1 & 1; > u32 msr = svm->vcpu.arch.regs[VCPU_REGS_RCX]; > - struct vmcb *nested_vmcb; > bool ret = false; > u32 t0, t1; > u8 *msrpm; > > - nested_vmcb = nested_svm_map(svm, svm->nested.vmcb, KM_USER0); > - msrpm = nested_svm_map(svm, svm->nested.vmcb_msrpm, KM_USER1); > + if (!(svm->nested.intercept & (1ULL << INTERCEPT_MSR_PROT))) > + return false; > > - if (!nested_vmcb || !msrpm) > - goto out; > + msrpm = nested_svm_map(svm, svm->nested.vmcb_msrpm, KM_USER0); > > - if (!(nested_vmcb->control.intercept & (1ULL << INTERCEPT_MSR_PROT))) > - return 0; > + if (!msrpm) > + goto out; > > switch (msr) { > case 0 ... 0x1fff: > @@ -1464,8 +1462,7 @@ static bool nested_svm_exit_handled_msr(struct vcpu_svm *svm) > ret = msrpm[t1] & ((1 << param) << t0); > > out: > - nested_svm_unmap(nested_vmcb, KM_USER0); > - nested_svm_unmap(msrpm, KM_USER1); > + nested_svm_unmap(msrpm, KM_USER0); > > return ret; > } > -- > 1.6.0.2 > >
On Thu, Sep 03, 2009 at 04:51:52PM +0200, Alexander Graf wrote: > Thanks to Joerg's previous series of cleanups, we now have almost all > information we need to decide what to do on #VMEXIT because we get > the variables from the VMCB on VMRUN. > > Unfortunately there's one piece that slipped through the conversion, > namely the MSR intercept which still tries to map the nested VMCB > to find out if MSRs are intercepted. > > So let's use the cached value, removing the need for two atomic maps > (which breaks anyways) and fix an oops along the way. > > CC: Joerg Roedel <joerg.roedel@amd.com> > Signed-off-by: Alexander Graf <agraf@suse.de> Applied, thanks. BTW, why nested_svm_map takes mmap_sem? Thats looks wrong. > > --- > > v1 -> v2: > > - Don't break when MSR is not intercepted > --- > arch/x86/kvm/svm.c | 15 ++++++--------- > 1 files changed, 6 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index 2df9b45..a5f90c7 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -1427,19 +1427,17 @@ static bool nested_svm_exit_handled_msr(struct vcpu_svm *svm) > { > u32 param = svm->vmcb->control.exit_info_1 & 1; > u32 msr = svm->vcpu.arch.regs[VCPU_REGS_RCX]; > - struct vmcb *nested_vmcb; > bool ret = false; > u32 t0, t1; > u8 *msrpm; > > - nested_vmcb = nested_svm_map(svm, svm->nested.vmcb, KM_USER0); > - msrpm = nested_svm_map(svm, svm->nested.vmcb_msrpm, KM_USER1); > + if (!(svm->nested.intercept & (1ULL << INTERCEPT_MSR_PROT))) > + return false; > > - if (!nested_vmcb || !msrpm) > - goto out; > + msrpm = nested_svm_map(svm, svm->nested.vmcb_msrpm, KM_USER0); > > - if (!(nested_vmcb->control.intercept & (1ULL << INTERCEPT_MSR_PROT))) > - return 0; > + if (!msrpm) > + goto out; > > switch (msr) { > case 0 ... 0x1fff: > @@ -1464,8 +1462,7 @@ static bool nested_svm_exit_handled_msr(struct vcpu_svm *svm) > ret = msrpm[t1] & ((1 << param) << t0); > > out: > - nested_svm_unmap(nested_vmcb, KM_USER0); > - nested_svm_unmap(msrpm, KM_USER1); > + nested_svm_unmap(msrpm, KM_USER0); > > return ret; > } > -- > 1.6.0.2 > > -- > 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 -- 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
Am 04.09.2009 um 19:05 schrieb Marcelo Tosatti <mtosatti@redhat.com>: > On Thu, Sep 03, 2009 at 04:51:52PM +0200, Alexander Graf wrote: >> Thanks to Joerg's previous series of cleanups, we now have almost all >> information we need to decide what to do on #VMEXIT because we get >> the variables from the VMCB on VMRUN. >> >> Unfortunately there's one piece that slipped through the conversion, >> namely the MSR intercept which still tries to map the nested VMCB >> to find out if MSRs are intercepted. >> >> So let's use the cached value, removing the need for two atomic maps >> (which breaks anyways) and fix an oops along the way. >> >> CC: Joerg Roedel <joerg.roedel@amd.com> >> Signed-off-by: Alexander Graf <agraf@suse.de> > > Applied, thanks. > > BTW, why nested_svm_map takes mmap_sem? Thats looks wrong. Hm, good question. As long as it's an atomic section that only touches vcpu local data there's no need to lock at all, right? Alex > >> >> --- >> >> v1 -> v2: >> >> - Don't break when MSR is not intercepted >> --- >> arch/x86/kvm/svm.c | 15 ++++++--------- >> 1 files changed, 6 insertions(+), 9 deletions(-) >> >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >> index 2df9b45..a5f90c7 100644 >> --- a/arch/x86/kvm/svm.c >> +++ b/arch/x86/kvm/svm.c >> @@ -1427,19 +1427,17 @@ static bool nested_svm_exit_handled_msr >> (struct vcpu_svm *svm) >> { >> u32 param = svm->vmcb->control.exit_info_1 & 1; >> u32 msr = svm->vcpu.arch.regs[VCPU_REGS_RCX]; >> - struct vmcb *nested_vmcb; >> bool ret = false; >> u32 t0, t1; >> u8 *msrpm; >> >> - nested_vmcb = nested_svm_map(svm, svm->nested.vmcb, KM_USER0); >> - msrpm = nested_svm_map(svm, svm->nested.vmcb_msrpm, >> KM_USER1); >> + if (!(svm->nested.intercept & (1ULL << INTERCEPT_MSR_PROT))) >> + return false; >> >> - if (!nested_vmcb || !msrpm) >> - goto out; >> + msrpm = nested_svm_map(svm, svm->nested.vmcb_msrpm, KM_USER0); >> >> - if (!(nested_vmcb->control.intercept & (1ULL << >> INTERCEPT_MSR_PROT))) >> - return 0; >> + if (!msrpm) >> + goto out; >> >> switch (msr) { >> case 0 ... 0x1fff: >> @@ -1464,8 +1462,7 @@ static bool nested_svm_exit_handled_msr >> (struct vcpu_svm *svm) >> ret = msrpm[t1] & ((1 << param) << t0); >> >> out: >> - nested_svm_unmap(nested_vmcb, KM_USER0); >> - nested_svm_unmap(msrpm, KM_USER1); >> + nested_svm_unmap(msrpm, KM_USER0); >> >> return ret; >> } >> -- >> 1.6.0.2 >> >> -- >> 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 -- 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 Sat, Sep 05, 2009 at 12:59:26AM +0200, Alexander Graf wrote: > > Am 04.09.2009 um 19:05 schrieb Marcelo Tosatti <mtosatti@redhat.com>: > >> On Thu, Sep 03, 2009 at 04:51:52PM +0200, Alexander Graf wrote: >>> Thanks to Joerg's previous series of cleanups, we now have almost all >>> information we need to decide what to do on #VMEXIT because we get >>> the variables from the VMCB on VMRUN. >>> >>> Unfortunately there's one piece that slipped through the conversion, >>> namely the MSR intercept which still tries to map the nested VMCB >>> to find out if MSRs are intercepted. >>> >>> So let's use the cached value, removing the need for two atomic maps >>> (which breaks anyways) and fix an oops along the way. >>> >>> CC: Joerg Roedel <joerg.roedel@amd.com> >>> Signed-off-by: Alexander Graf <agraf@suse.de> >> >> Applied, thanks. >> >> BTW, why nested_svm_map takes mmap_sem? Thats looks wrong. > > Hm, good question. > > As long as it's an atomic section that only touches vcpu local data > there's no need to lock at all, right? Yes. gfn_to_page / get_user_pages will grab mmap_sem if required. Can you please send a patch to drop it from nested_svm_map? -- 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
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index 2df9b45..a5f90c7 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -1427,19 +1427,17 @@ static bool nested_svm_exit_handled_msr(struct vcpu_svm *svm) { u32 param = svm->vmcb->control.exit_info_1 & 1; u32 msr = svm->vcpu.arch.regs[VCPU_REGS_RCX]; - struct vmcb *nested_vmcb; bool ret = false; u32 t0, t1; u8 *msrpm; - nested_vmcb = nested_svm_map(svm, svm->nested.vmcb, KM_USER0); - msrpm = nested_svm_map(svm, svm->nested.vmcb_msrpm, KM_USER1); + if (!(svm->nested.intercept & (1ULL << INTERCEPT_MSR_PROT))) + return false; - if (!nested_vmcb || !msrpm) - goto out; + msrpm = nested_svm_map(svm, svm->nested.vmcb_msrpm, KM_USER0); - if (!(nested_vmcb->control.intercept & (1ULL << INTERCEPT_MSR_PROT))) - return 0; + if (!msrpm) + goto out; switch (msr) { case 0 ... 0x1fff: @@ -1464,8 +1462,7 @@ static bool nested_svm_exit_handled_msr(struct vcpu_svm *svm) ret = msrpm[t1] & ((1 << param) << t0); out: - nested_svm_unmap(nested_vmcb, KM_USER0); - nested_svm_unmap(msrpm, KM_USER1); + nested_svm_unmap(msrpm, KM_USER0); return ret; }
Thanks to Joerg's previous series of cleanups, we now have almost all information we need to decide what to do on #VMEXIT because we get the variables from the VMCB on VMRUN. Unfortunately there's one piece that slipped through the conversion, namely the MSR intercept which still tries to map the nested VMCB to find out if MSRs are intercepted. So let's use the cached value, removing the need for two atomic maps (which breaks anyways) and fix an oops along the way. CC: Joerg Roedel <joerg.roedel@amd.com> Signed-off-by: Alexander Graf <agraf@suse.de> --- v1 -> v2: - Don't break when MSR is not intercepted --- arch/x86/kvm/svm.c | 15 ++++++--------- 1 files changed, 6 insertions(+), 9 deletions(-)