Changes since v3:
- Swapped my "KVM: nSVM: stop dereferencing vcpu->arch.mmu to get the
context in kvm_init_shadow{,_npt}_mmu()" with Paolo's "KVM: MMU: stop
dereferencing vcpu->arch.mmu to get the context for MMU init".
- keeping nested_svm_init_mmu_context() in nested_prepare_vmcb_control()
as this is also used from svm_set_nested_state() [Paolo],
nested_svm_load_cr3() becomes a separate step in enter_svm_guest_mode().
- nested_prepare_vmcb_save() remains 'void' [Paolo]
Original description:
This is a successor of "[PATCH v2 0/3] KVM: nSVM: fix #TF from CR3 switch
when entering guest" and "[PATCH] KVM: x86: drop erroneous mmu_check_root()
from fast_pgd_switch()".
The snowball is growing fast! It all started with an intention to fix
the particular 'tripple fault' issue (now fixed by PATCH7) but now we
also get rid of unconditional kvm_mmu_reset_context() upon nested guest
entry/exit and make the code resemble nVMX. There is still a huge room
for further improvement (proper error propagation, removing unconditional
MMU sync/TLB flush,...) but at least we're making some progress.
Tested with kvm selftests/kvm-unit-tests and by running nested Hyper-V
on KVM. The series doesn't seem to introduce any new issues.
Paolo Bonzini (1):
KVM: MMU: stop dereferencing vcpu->arch.mmu to get the context for MMU
init
Vitaly Kuznetsov (8):
KVM: nSVM: split kvm_init_shadow_npt_mmu() from kvm_init_shadow_mmu()
KVM: nSVM: reset nested_run_pending upon nested_svm_vmrun_msrpm()
failure
KVM: nSVM: prepare to handle errors from enter_svm_guest_mode()
KVM: nSVM: introduce nested_svm_load_cr3()/nested_npt_enabled()
KVM: nSVM: move kvm_set_cr3() after nested_svm_uninit_mmu_context()
KVM: nSVM: implement nested_svm_load_cr3() and use it for host->guest
switch
KVM: nSVM: use nested_svm_load_cr3() on guest->host switch
KVM: x86: drop superfluous mmu_check_root() from fast_pgd_switch()
arch/x86/kvm/mmu.h | 3 +-
arch/x86/kvm/mmu/mmu.c | 45 ++++++++++++------
arch/x86/kvm/svm/nested.c | 97 ++++++++++++++++++++++++++++-----------
arch/x86/kvm/svm/svm.c | 6 ++-
arch/x86/kvm/svm/svm.h | 4 +-
5 files changed, 110 insertions(+), 45 deletions(-)
--
2.25.4
kvm_mmu_new_pgd() refers to arch.mmu and at this point it still references
arch.guest_mmu while arch.root_mmu is expected.
Note, the change is effectively a nop: when !npt_enabled,
nested_svm_uninit_mmu_context() does nothing (as we don't do
nested_svm_init_mmu_context()) and with npt_enabled we don't
do kvm_set_cr3() but we're about to switch to nested_svm_load_cr3().
No functional change intended.
Signed-off-by: Vitaly Kuznetsov <[email protected]>
---
arch/x86/kvm/svm/nested.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 180929f3dbef..2c308dfa5468 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -611,12 +611,6 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
svm_set_efer(&svm->vcpu, hsave->save.efer);
svm_set_cr0(&svm->vcpu, hsave->save.cr0 | X86_CR0_PE);
svm_set_cr4(&svm->vcpu, hsave->save.cr4);
- if (npt_enabled) {
- svm->vmcb->save.cr3 = hsave->save.cr3;
- svm->vcpu.arch.cr3 = hsave->save.cr3;
- } else {
- (void)kvm_set_cr3(&svm->vcpu, hsave->save.cr3);
- }
kvm_rax_write(&svm->vcpu, hsave->save.rax);
kvm_rsp_write(&svm->vcpu, hsave->save.rsp);
kvm_rip_write(&svm->vcpu, hsave->save.rip);
@@ -636,6 +630,14 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
kvm_vcpu_unmap(&svm->vcpu, &map, true);
nested_svm_uninit_mmu_context(&svm->vcpu);
+
+ if (npt_enabled) {
+ svm->vmcb->save.cr3 = hsave->save.cr3;
+ svm->vcpu.arch.cr3 = hsave->save.cr3;
+ } else {
+ (void)kvm_set_cr3(&svm->vcpu, hsave->save.cr3);
+ }
+
kvm_mmu_reset_context(&svm->vcpu);
kvm_mmu_load(&svm->vcpu);
--
2.25.4
From: Paolo Bonzini <[email protected]>
kvm_init_shadow_mmu() was actually the only function that could be called
with different vcpu->arch.mmu values. Now that kvm_init_shadow_npt_mmu()
is separated from kvm_init_shadow_mmu(), we always know the MMU context
we need to use and there is no need to dereference vcpu->arch.mmu pointer.
Based on a patch by Vitaly Kuznetsov <[email protected]>.
Signed-off-by: Paolo Bonzini <[email protected]>
Signed-off-by: Vitaly Kuznetsov <[email protected]>
---
arch/x86/kvm/mmu/mmu.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 93f18e5fa8b5..3a306ab1a9c9 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -4884,7 +4884,7 @@ kvm_calc_tdp_mmu_root_page_role(struct kvm_vcpu *vcpu, bool base_only)
static void init_kvm_tdp_mmu(struct kvm_vcpu *vcpu)
{
- struct kvm_mmu *context = vcpu->arch.mmu;
+ struct kvm_mmu *context = &vcpu->arch.root_mmu;
union kvm_mmu_role new_role =
kvm_calc_tdp_mmu_root_page_role(vcpu, false);
@@ -4952,11 +4952,10 @@ kvm_calc_shadow_mmu_root_page_role(struct kvm_vcpu *vcpu, bool base_only)
return role;
}
-static void shadow_mmu_init_context(struct kvm_vcpu *vcpu, u32 cr0, u32 cr4,
- u32 efer, union kvm_mmu_role new_role)
+static void shadow_mmu_init_context(struct kvm_vcpu *vcpu, struct kvm_mmu *context,
+ u32 cr0, u32 cr4, u32 efer,
+ union kvm_mmu_role new_role)
{
- struct kvm_mmu *context = vcpu->arch.mmu;
-
if (!(cr0 & X86_CR0_PG))
nonpaging_init_context(vcpu, context);
else if (efer & EFER_LMA)
@@ -4972,23 +4971,23 @@ static void shadow_mmu_init_context(struct kvm_vcpu *vcpu, u32 cr0, u32 cr4,
static void kvm_init_shadow_mmu(struct kvm_vcpu *vcpu, u32 cr0, u32 cr4, u32 efer)
{
- struct kvm_mmu *context = vcpu->arch.mmu;
+ struct kvm_mmu *context = &vcpu->arch.root_mmu;
union kvm_mmu_role new_role =
kvm_calc_shadow_mmu_root_page_role(vcpu, false);
if (new_role.as_u64 != context->mmu_role.as_u64)
- shadow_mmu_init_context(vcpu, cr0, cr4, efer, new_role);
+ shadow_mmu_init_context(vcpu, context, cr0, cr4, efer, new_role);
}
void kvm_init_shadow_npt_mmu(struct kvm_vcpu *vcpu, u32 cr0, u32 cr4, u32 efer,
gpa_t nested_cr3)
{
- struct kvm_mmu *context = vcpu->arch.mmu;
+ struct kvm_mmu *context = &vcpu->arch.guest_mmu;
union kvm_mmu_role new_role =
kvm_calc_shadow_mmu_root_page_role(vcpu, false);
if (new_role.as_u64 != context->mmu_role.as_u64)
- shadow_mmu_init_context(vcpu, cr0, cr4, efer, new_role);
+ shadow_mmu_init_context(vcpu, context, cr0, cr4, efer, new_role);
}
EXPORT_SYMBOL_GPL(kvm_init_shadow_npt_mmu);
@@ -5024,7 +5023,7 @@ kvm_calc_shadow_ept_root_page_role(struct kvm_vcpu *vcpu, bool accessed_dirty,
void kvm_init_shadow_ept_mmu(struct kvm_vcpu *vcpu, bool execonly,
bool accessed_dirty, gpa_t new_eptp)
{
- struct kvm_mmu *context = vcpu->arch.mmu;
+ struct kvm_mmu *context = &vcpu->arch.guest_mmu;
u8 level = vmx_eptp_page_walk_level(new_eptp);
union kvm_mmu_role new_role =
kvm_calc_shadow_ept_root_page_role(vcpu, accessed_dirty,
@@ -5058,7 +5057,7 @@ EXPORT_SYMBOL_GPL(kvm_init_shadow_ept_mmu);
static void init_kvm_softmmu(struct kvm_vcpu *vcpu)
{
- struct kvm_mmu *context = vcpu->arch.mmu;
+ struct kvm_mmu *context = &vcpu->arch.root_mmu;
kvm_init_shadow_mmu(vcpu,
kvm_read_cr0_bits(vcpu, X86_CR0_PG),
--
2.25.4
Some operations in enter_svm_guest_mode() may fail, e.g. currently
we suppress kvm_set_cr3() return value. Prepare the code to proparate
errors.
No functional change intended.
Signed-off-by: Vitaly Kuznetsov <[email protected]>
---
arch/x86/kvm/svm/nested.c | 26 ++++++++++++++++----------
arch/x86/kvm/svm/svm.c | 6 ++++--
arch/x86/kvm/svm/svm.h | 4 ++--
3 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 1cc8592b1820..5e6c988a4e6b 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -379,7 +379,7 @@ static void nested_prepare_vmcb_control(struct vcpu_svm *svm)
mark_all_dirty(svm->vmcb);
}
-void enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa,
+int enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa,
struct vmcb *nested_vmcb)
{
svm->nested.vmcb = vmcb_gpa;
@@ -388,6 +388,8 @@ void enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa,
nested_prepare_vmcb_control(svm);
svm_set_gif(svm, true);
+
+ return 0;
}
int nested_svm_vmrun(struct vcpu_svm *svm)
@@ -465,18 +467,22 @@ int nested_svm_vmrun(struct vcpu_svm *svm)
copy_vmcb_control_area(&hsave->control, &vmcb->control);
svm->nested.nested_run_pending = 1;
- enter_svm_guest_mode(svm, vmcb_gpa, nested_vmcb);
- if (!nested_svm_vmrun_msrpm(svm)) {
- svm->nested.nested_run_pending = 0;
+ if (enter_svm_guest_mode(svm, vmcb_gpa, nested_vmcb))
+ goto out_exit_err;
- svm->vmcb->control.exit_code = SVM_EXIT_ERR;
- svm->vmcb->control.exit_code_hi = 0;
- svm->vmcb->control.exit_info_1 = 0;
- svm->vmcb->control.exit_info_2 = 0;
+ if (nested_svm_vmrun_msrpm(svm))
+ goto out;
- nested_svm_vmexit(svm);
- }
+out_exit_err:
+ svm->nested.nested_run_pending = 0;
+
+ svm->vmcb->control.exit_code = SVM_EXIT_ERR;
+ svm->vmcb->control.exit_code_hi = 0;
+ svm->vmcb->control.exit_info_1 = 0;
+ svm->vmcb->control.exit_info_2 = 0;
+
+ nested_svm_vmexit(svm);
out:
kvm_vcpu_unmap(&svm->vcpu, &map, true);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index c0da4dd78ac5..b8ec56fe5478 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3843,6 +3843,7 @@ static int svm_pre_leave_smm(struct kvm_vcpu *vcpu, const char *smstate)
struct kvm_host_map map;
u64 guest;
u64 vmcb;
+ int ret = 0;
guest = GET_SMSTATE(u64, smstate, 0x7ed8);
vmcb = GET_SMSTATE(u64, smstate, 0x7ee0);
@@ -3851,10 +3852,11 @@ static int svm_pre_leave_smm(struct kvm_vcpu *vcpu, const char *smstate)
if (kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(vmcb), &map) == -EINVAL)
return 1;
nested_vmcb = map.hva;
- enter_svm_guest_mode(svm, vmcb, nested_vmcb);
+ ret = enter_svm_guest_mode(svm, vmcb, nested_vmcb);
kvm_vcpu_unmap(&svm->vcpu, &map, true);
}
- return 0;
+
+ return ret;
}
static void enable_smi_window(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 6ac4c00a5d82..f98649af11d1 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -387,8 +387,8 @@ static inline bool nested_exit_on_nmi(struct vcpu_svm *svm)
return (svm->nested.ctl.intercept & (1ULL << INTERCEPT_NMI));
}
-void enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa,
- struct vmcb *nested_vmcb);
+int enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa,
+ struct vmcb *nested_vmcb);
void svm_leave_nested(struct vcpu_svm *svm);
int nested_svm_vmrun(struct vcpu_svm *svm);
void nested_svm_vmloadsave(struct vmcb *from_vmcb, struct vmcb *to_vmcb);
--
2.25.4
As a preparatory change for implementing nested specifig PGD switch for
nSVM (following nVMX' nested_vmx_load_cr3()) instead of relying on
kvm_set_cr3() introduce nested_svm_load_cr3().
No functional change intended.
Signed-off-by: Vitaly Kuznetsov <[email protected]>
---
arch/x86/kvm/svm/nested.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 5e6c988a4e6b..180929f3dbef 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -311,6 +311,21 @@ static void nested_vmcb_save_pending_event(struct vcpu_svm *svm,
nested_vmcb->control.exit_int_info = exit_int_info;
}
+static inline bool nested_npt_enabled(struct vcpu_svm *svm)
+{
+ return svm->nested.ctl.nested_ctl & SVM_NESTED_CTL_NP_ENABLE;
+}
+
+/*
+ * Load guest's cr3 at nested entry. @nested_npt is true if we are
+ * emulating VM-Entry into a guest with NPT enabled.
+ */
+static int nested_svm_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3,
+ bool nested_npt)
+{
+ return kvm_set_cr3(vcpu, cr3);
+}
+
static void nested_prepare_vmcb_save(struct vcpu_svm *svm, struct vmcb *nested_vmcb)
{
/* Load the nested guest state */
@@ -324,7 +339,8 @@ static void nested_prepare_vmcb_save(struct vcpu_svm *svm, struct vmcb *nested_v
svm_set_efer(&svm->vcpu, nested_vmcb->save.efer);
svm_set_cr0(&svm->vcpu, nested_vmcb->save.cr0);
svm_set_cr4(&svm->vcpu, nested_vmcb->save.cr4);
- (void)kvm_set_cr3(&svm->vcpu, nested_vmcb->save.cr3);
+ (void)nested_svm_load_cr3(&svm->vcpu, nested_vmcb->save.cr3,
+ nested_npt_enabled(svm));
svm->vmcb->save.cr2 = svm->vcpu.arch.cr2 = nested_vmcb->save.cr2;
kvm_rax_write(&svm->vcpu, nested_vmcb->save.rax);
@@ -343,7 +359,8 @@ static void nested_prepare_vmcb_save(struct vcpu_svm *svm, struct vmcb *nested_v
static void nested_prepare_vmcb_control(struct vcpu_svm *svm)
{
const u32 mask = V_INTR_MASKING_MASK | V_GIF_ENABLE_MASK | V_GIF_MASK;
- if (svm->nested.ctl.nested_ctl & SVM_NESTED_CTL_NP_ENABLE)
+
+ if (nested_npt_enabled(svm))
nested_svm_init_mmu_context(&svm->vcpu);
/* Guest paging mode is active - reset mmu */
--
2.25.4
On 10/07/20 16:11, Vitaly Kuznetsov wrote:
> Changes since v3:
> - Swapped my "KVM: nSVM: stop dereferencing vcpu->arch.mmu to get the
> context in kvm_init_shadow{,_npt}_mmu()" with Paolo's "KVM: MMU: stop
> dereferencing vcpu->arch.mmu to get the context for MMU init".
> - keeping nested_svm_init_mmu_context() in nested_prepare_vmcb_control()
> as this is also used from svm_set_nested_state() [Paolo],
> nested_svm_load_cr3() becomes a separate step in enter_svm_guest_mode().
> - nested_prepare_vmcb_save() remains 'void' [Paolo]
>
> Original description:
>
> This is a successor of "[PATCH v2 0/3] KVM: nSVM: fix #TF from CR3 switch
> when entering guest" and "[PATCH] KVM: x86: drop erroneous mmu_check_root()
> from fast_pgd_switch()".
>
> The snowball is growing fast! It all started with an intention to fix
> the particular 'tripple fault' issue (now fixed by PATCH7) but now we
> also get rid of unconditional kvm_mmu_reset_context() upon nested guest
> entry/exit and make the code resemble nVMX. There is still a huge room
> for further improvement (proper error propagation, removing unconditional
> MMU sync/TLB flush,...) but at least we're making some progress.
>
> Tested with kvm selftests/kvm-unit-tests and by running nested Hyper-V
> on KVM. The series doesn't seem to introduce any new issues.
>
> Paolo Bonzini (1):
> KVM: MMU: stop dereferencing vcpu->arch.mmu to get the context for MMU
> init
>
> Vitaly Kuznetsov (8):
> KVM: nSVM: split kvm_init_shadow_npt_mmu() from kvm_init_shadow_mmu()
> KVM: nSVM: reset nested_run_pending upon nested_svm_vmrun_msrpm()
> failure
> KVM: nSVM: prepare to handle errors from enter_svm_guest_mode()
> KVM: nSVM: introduce nested_svm_load_cr3()/nested_npt_enabled()
> KVM: nSVM: move kvm_set_cr3() after nested_svm_uninit_mmu_context()
> KVM: nSVM: implement nested_svm_load_cr3() and use it for host->guest
> switch
> KVM: nSVM: use nested_svm_load_cr3() on guest->host switch
> KVM: x86: drop superfluous mmu_check_root() from fast_pgd_switch()
>
> arch/x86/kvm/mmu.h | 3 +-
> arch/x86/kvm/mmu/mmu.c | 45 ++++++++++++------
> arch/x86/kvm/svm/nested.c | 97 ++++++++++++++++++++++++++++-----------
> arch/x86/kvm/svm/svm.c | 6 ++-
> arch/x86/kvm/svm/svm.h | 4 +-
> 5 files changed, 110 insertions(+), 45 deletions(-)
>
Queued, thanks.
Paolo
On Fri, Jul 10, 2020 at 04:11:53PM +0200, Vitaly Kuznetsov wrote:
> As a preparatory change for implementing nested specifig PGD switch for
s/specifig/specific
> nSVM (following nVMX' nested_vmx_load_cr3()) instead of relying on
nVMX's
> kvm_set_cr3() introduce nested_svm_load_cr3().
The changelog isn't all that helpful to understanding the actual change.
All this is doing is wrapping kvm_set_cr3(), but that's not at all obvious
from reading the above.
E.g.
Add nested_svm_load_cr3() as a pass-through to kvm_set_cr3() as a
preparatory change for implementing nested specific PGD switch for nSVM,
(following nVMx's nested_vmx_load_cr3()).
> No functional change intended.
>
> Signed-off-by: Vitaly Kuznetsov <[email protected]>
> ---
> arch/x86/kvm/svm/nested.c | 21 +++++++++++++++++++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 5e6c988a4e6b..180929f3dbef 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -311,6 +311,21 @@ static void nested_vmcb_save_pending_event(struct vcpu_svm *svm,
> nested_vmcb->control.exit_int_info = exit_int_info;
> }
>
> +static inline bool nested_npt_enabled(struct vcpu_svm *svm)
> +{
> + return svm->nested.ctl.nested_ctl & SVM_NESTED_CTL_NP_ENABLE;
> +}
> +
> +/*
> + * Load guest's cr3 at nested entry. @nested_npt is true if we are
> + * emulating VM-Entry into a guest with NPT enabled.
> + */
> +static int nested_svm_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3,
> + bool nested_npt)
IMO the addition of nested_npt_enabled() should be a separate patch, and
the additoin of @nested_npt should be in patch 7.
Hypothetically speaking, if nested_npt_enabled() is inaccurate at the call
site in nested_prepare_vmcb_save(), then this patch is technically wrong
even though it doesn't introduce a bug. Given that the call site of
nested_svm_load_cr3() is moved in patch 7, I don't see any value in adding
the placeholder parameter early.
> +{
> + return kvm_set_cr3(vcpu, cr3);
> +}
> +
> static void nested_prepare_vmcb_save(struct vcpu_svm *svm, struct vmcb *nested_vmcb)
> {
> /* Load the nested guest state */
> @@ -324,7 +339,8 @@ static void nested_prepare_vmcb_save(struct vcpu_svm *svm, struct vmcb *nested_v
> svm_set_efer(&svm->vcpu, nested_vmcb->save.efer);
> svm_set_cr0(&svm->vcpu, nested_vmcb->save.cr0);
> svm_set_cr4(&svm->vcpu, nested_vmcb->save.cr4);
> - (void)kvm_set_cr3(&svm->vcpu, nested_vmcb->save.cr3);
> + (void)nested_svm_load_cr3(&svm->vcpu, nested_vmcb->save.cr3,
> + nested_npt_enabled(svm));
>
> svm->vmcb->save.cr2 = svm->vcpu.arch.cr2 = nested_vmcb->save.cr2;
> kvm_rax_write(&svm->vcpu, nested_vmcb->save.rax);
> @@ -343,7 +359,8 @@ static void nested_prepare_vmcb_save(struct vcpu_svm *svm, struct vmcb *nested_v
> static void nested_prepare_vmcb_control(struct vcpu_svm *svm)
> {
> const u32 mask = V_INTR_MASKING_MASK | V_GIF_ENABLE_MASK | V_GIF_MASK;
> - if (svm->nested.ctl.nested_ctl & SVM_NESTED_CTL_NP_ENABLE)
> +
> + if (nested_npt_enabled(svm))
> nested_svm_init_mmu_context(&svm->vcpu);
>
> /* Guest paging mode is active - reset mmu */
> --
> 2.25.4
>
Sean Christopherson <[email protected]> writes:
> On Fri, Jul 10, 2020 at 04:11:53PM +0200, Vitaly Kuznetsov wrote:
>> As a preparatory change for implementing nested specifig PGD switch for
>
> s/specifig/specific
>
>> nSVM (following nVMX' nested_vmx_load_cr3()) instead of relying on
>
> nVMX's
>
>> kvm_set_cr3() introduce nested_svm_load_cr3().
>
> The changelog isn't all that helpful to understanding the actual change.
> All this is doing is wrapping kvm_set_cr3(), but that's not at all obvious
> from reading the above.
>
> E.g.
>
> Add nested_svm_load_cr3() as a pass-through to kvm_set_cr3() as a
> preparatory change for implementing nested specific PGD switch for nSVM,
> (following nVMx's nested_vmx_load_cr3()).
>
Sounds better indeed, thanks!
>> No functional change intended.
>>
>> Signed-off-by: Vitaly Kuznetsov <[email protected]>
>> ---
>> arch/x86/kvm/svm/nested.c | 21 +++++++++++++++++++--
>> 1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index 5e6c988a4e6b..180929f3dbef 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
>> @@ -311,6 +311,21 @@ static void nested_vmcb_save_pending_event(struct vcpu_svm *svm,
>> nested_vmcb->control.exit_int_info = exit_int_info;
>> }
>>
>> +static inline bool nested_npt_enabled(struct vcpu_svm *svm)
>> +{
>> + return svm->nested.ctl.nested_ctl & SVM_NESTED_CTL_NP_ENABLE;
>> +}
>> +
>> +/*
>> + * Load guest's cr3 at nested entry. @nested_npt is true if we are
>> + * emulating VM-Entry into a guest with NPT enabled.
>> + */
>> +static int nested_svm_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3,
>> + bool nested_npt)
>
> IMO the addition of nested_npt_enabled() should be a separate patch, and
> the additoin of @nested_npt should be in patch 7.
>
> Hypothetically speaking, if nested_npt_enabled() is inaccurate at the call
> site in nested_prepare_vmcb_save(), then this patch is technically wrong
> even though it doesn't introduce a bug. Given that the call site of
> nested_svm_load_cr3() is moved in patch 7, I don't see any value in adding
> the placeholder parameter early.
>
I see and I mostly agree, I put it here to avoid the unneeded churn and
make it easier to review the whole thing: this patch is technically a
nop so it can be reviewed in "doesn't change anything" mode and patches
which actually change things are smaller.
Paolo already said 'queued' here and your comments can't be addressed in
a follow-up patch but I can certainly do v5 if needed.
Thanks for your review!
--
Vitaly
On Tue, Jul 14, 2020 at 01:26:24PM +0200, Vitaly Kuznetsov wrote:
> Sean Christopherson <[email protected]> writes:
> > IMO the addition of nested_npt_enabled() should be a separate patch, and
> > the additoin of @nested_npt should be in patch 7.
> >
> > Hypothetically speaking, if nested_npt_enabled() is inaccurate at the call
> > site in nested_prepare_vmcb_save(), then this patch is technically wrong
> > even though it doesn't introduce a bug. Given that the call site of
> > nested_svm_load_cr3() is moved in patch 7, I don't see any value in adding
> > the placeholder parameter early.
> >
>
> I see and I mostly agree, I put it here to avoid the unneeded churn and
> make it easier to review the whole thing: this patch is technically a
> nop so it can be reviewed in "doesn't change anything" mode and patches
> which actually change things are smaller.
>
> Paolo already said 'queued' here and your comments can't be addressed in
> a follow-up patch but I can certainly do v5 if needed.
Eh, not necessary, I didn't see that the series was in kvm/queue until after
I hit send. Thanks!