| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <YQLuBDZ2MlNlIoH4@google.com>
Date: Thu, 29 Jul 2021 18:05:56 +0000
From: Sean Christopherson <seanjc@...gle.com>
To: Lai Jiangshan <jiangshanlai@...il.com>
Cc: linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
Paolo Bonzini <pbonzini@...hat.com>,
Lai Jiangshan <laijs@...ux.alibaba.com>,
Vitaly Kuznetsov <vkuznets@...hat.com>,
Wanpeng Li <wanpengli@...cent.com>,
Jim Mattson <jmattson@...gle.com>
Subject: Re: [RFC PATCH] kvm/x86: Keep root hpa in prev_roots as much as
possible
On Wed, May 26, 2021, Lai Jiangshan wrote:
> From: Lai Jiangshan <laijs@...ux.alibaba.com>
>
> Pagetable roots in prev_roots[] are likely to be reused soon and
> there is no much overhead to keep it with a new need_sync field
> introduced.
>
> With the help of the new need_sync field, pagetable roots are
> kept as much as possible, and they will be re-synced before reused
> instead of being dropped.
>
> Signed-off-by: Lai Jiangshan <laijs@...ux.alibaba.com>
> ---
>
> This patch is just for RFC.
> Is the idea Ok?
Yes, the idea is definitely a good one.
> If the idea is Ok, we need to reused one bit from pgd or hpa
> as need_sync to save memory. Which one is better?
Ha, we can do this without increasing the memory footprint and without co-opting
a bit from pgd or hpa. Because of compiler alignment/padding, the u8s and bools
between mmu_role and prev_roots already occupy 8 bytes, even though the actual
size is 4 bytes. In total, we need room for 4 roots (3 previous + current), i.e.
4 bytes. If a separate array is used, no additional memory is consumed and no
masking is needed when reading/writing e.g. pgd.
The cost is an extra swap() when updating the prev_roots LRU, but that's peanuts
and would likely be offset by masking anyways.
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 99f37781a6fc..13bb3c3a60b4 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -424,10 +424,12 @@ struct kvm_mmu {
hpa_t root_hpa;
gpa_t root_pgd;
union kvm_mmu_role mmu_role;
+ bool root_unsync;
u8 root_level;
u8 shadow_root_level;
u8 ept_ad;
bool direct_map;
+ bool unsync_roots[KVM_MMU_NUM_PREV_ROOTS];
struct kvm_mmu_root_info prev_roots[KVM_MMU_NUM_PREV_ROOTS];
/*
> arch/x86/include/asm/kvm_host.h | 3 ++-
> arch/x86/kvm/mmu/mmu.c | 6 ++++++
> arch/x86/kvm/vmx/nested.c | 12 ++++--------
> arch/x86/kvm/x86.c | 9 +++++----
> 4 files changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 55efbacfc244..19a337cf7aa6 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -354,10 +354,11 @@ struct rsvd_bits_validate {
> struct kvm_mmu_root_info {
> gpa_t pgd;
> hpa_t hpa;
> + bool need_sync;
Hmm, use "unsync" instead of "need_sync", purely to match the existing terminology
in KVM's MMU for this sort of behavior.
> };
>
> #define KVM_MMU_ROOT_INFO_INVALID \
> - ((struct kvm_mmu_root_info) { .pgd = INVALID_PAGE, .hpa = INVALID_PAGE })
> + ((struct kvm_mmu_root_info) { .pgd = INVALID_PAGE, .hpa = INVALID_PAGE, .need_sync = true})
>
> #define KVM_MMU_NUM_PREV_ROOTS 3
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 5e60b00e8e50..147827135549 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -3878,6 +3878,7 @@ static bool cached_root_available(struct kvm_vcpu *vcpu, gpa_t new_pgd,
>
> root.pgd = mmu->root_pgd;
> root.hpa = mmu->root_hpa;
> + root.need_sync = false;
>
> if (is_root_usable(&root, new_pgd, new_role))
> return true;
> @@ -3892,6 +3893,11 @@ static bool cached_root_available(struct kvm_vcpu *vcpu, gpa_t new_pgd,
> mmu->root_hpa = root.hpa;
> mmu->root_pgd = root.pgd;
>
> + if (i < KVM_MMU_NUM_PREV_ROOTS && root.need_sync) {
Probably makes sense to write this as:
if (i >= KVM_MMU_NUM_PREV_ROOTS)
return false;
if (root.need_sync) {
kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
}
return true;
The "i < KVM_MMU_NUM_PREV_ROOTS == success" logic is just confusing enough that
it'd be nice to write it only once.
And that would also play nicely with deferring a sync for the "current" root
(see below), e.g.
...
unsync = mmu->root_unsync;
if (is_root_usable(&root, new_pgd, new_role))
goto found_root;
for (i = 0; i < KVM_MMU_NUM_PREV_ROOTS; i++) {
swap(root, mmu->prev_roots[i]);
swap(unsync, mmu->unsync_roots[i]);
if (is_root_usable(&root, new_pgd, new_role))
break;
}
if (i >= KVM_MMU_NUM_PREV_ROOTS)
return false;
found_root:
if (unsync) {
kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
}
return true;
> + kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
> + kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
> + }
> +
> return i < KVM_MMU_NUM_PREV_ROOTS;
> }
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 6058a65a6ede..ab7069ac6dc5 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -5312,7 +5312,7 @@ static int handle_invept(struct kvm_vcpu *vcpu)
> {
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> u32 vmx_instruction_info, types;
> - unsigned long type, roots_to_free;
> + unsigned long type;
> struct kvm_mmu *mmu;
> gva_t gva;
> struct x86_exception e;
> @@ -5361,29 +5361,25 @@ static int handle_invept(struct kvm_vcpu *vcpu)
> return nested_vmx_fail(vcpu,
> VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
>
> - roots_to_free = 0;
> if (nested_ept_root_matches(mmu->root_hpa, mmu->root_pgd,
> operand.eptp))
> - roots_to_free |= KVM_MMU_ROOT_CURRENT;
> + kvm_mmu_free_roots(vcpu, mmu, KVM_MMU_ROOT_CURRENT);
For a non-RFC series, I think this should do two things:
1. Separate INVEPT from INVPCID, i.e. do only INVPCID first.
2. Enhance INVEPT to SYNC+FLUSH the current root instead of freeing it
As alluded to above, this can be done by deferring the sync+flush (which can't
be done right away because INVEPT runs in L1 context, whereas KVM needs to sync+flush
L2 EPT context).
> for (i = 0; i < KVM_MMU_NUM_PREV_ROOTS; i++) {
> if (nested_ept_root_matches(mmu->prev_roots[i].hpa,
> mmu->prev_roots[i].pgd,
> operand.eptp))
> - roots_to_free |= KVM_MMU_ROOT_PREVIOUS(i);
> + mmu->prev_roots[i].need_sync = true;
> }
> break;
> case VMX_EPT_EXTENT_GLOBAL:
> - roots_to_free = KVM_MMU_ROOTS_ALL;
> + kvm_mmu_free_roots(vcpu, mmu, KVM_MMU_ROOTS_ALL);
> break;
> default:
> BUG();
> break;
> }
>
> - if (roots_to_free)
> - kvm_mmu_free_roots(vcpu, mmu, roots_to_free);
> -
> return nested_vmx_succeed(vcpu);
> }
Powered by blists - more mailing lists