From: Vitaly Kuznetsov <vkuznets@redhat.com>
To: "Sean Christopherson" <sean.j.christopherson@intel.com>,
"Paolo Bonzini" <pbonzini@redhat.com>,
"Radim Krčmář" <rkrcmar@redhat.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>,
Wanpeng Li <wanpengli@tencent.com>,
Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
Reto Buerki <reet@codelabs.ch>,
Liran Alon <liran.alon@oracle.com>
Subject: Re: [PATCH v2 7/8] KVM: x86: Add helpers to test/mark reg availability and dirtiness
Date: Mon, 30 Sep 2019 11:32:05 +0200 [thread overview]
Message-ID: <87d0fi3zai.fsf@vitty.brq.redhat.com> (raw)
In-Reply-To: <20190927214523.3376-8-sean.j.christopherson@intel.com>
Sean Christopherson <sean.j.christopherson@intel.com> writes:
> Add helpers to prettify code that tests and/or marks whether or not a
> register is available and/or dirty.
>
> Suggested-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
> arch/x86/kvm/kvm_cache_regs.h | 45 +++++++++++++++++++++++++----------
> arch/x86/kvm/vmx/nested.c | 4 ++--
> arch/x86/kvm/vmx/vmx.c | 29 ++++++++++------------
> arch/x86/kvm/x86.c | 13 ++++------
> 4 files changed, 53 insertions(+), 38 deletions(-)
>
> diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h
> index b85fc4b4e04f..9c2bc528800b 100644
> --- a/arch/x86/kvm/kvm_cache_regs.h
> +++ b/arch/x86/kvm/kvm_cache_regs.h
> @@ -37,12 +37,37 @@ BUILD_KVM_GPR_ACCESSORS(r14, R14)
> BUILD_KVM_GPR_ACCESSORS(r15, R15)
> #endif
>
> +static inline bool kvm_register_is_available(struct kvm_vcpu *vcpu,
> + enum kvm_reg reg)
> +{
> + return test_bit(reg, (unsigned long *)&vcpu->arch.regs_avail);
> +}
> +
(Interestingly enough, all call sites use !kvm_register_is_available()
but kvm_register_is_unavailable() sounds weird. So I'd prefer to keep it
as-is).
> +static inline bool kvm_register_is_dirty(struct kvm_vcpu *vcpu,
> + enum kvm_reg reg)
> +{
> + return test_bit(reg, (unsigned long *)&vcpu->arch.regs_dirty);
> +}
> +
> +static inline void kvm_register_mark_available(struct kvm_vcpu *vcpu,
> + enum kvm_reg reg)
> +{
> + __set_bit(reg, (unsigned long *)&vcpu->arch.regs_avail);
> +}
> +
> +static inline void kvm_register_mark_dirty(struct kvm_vcpu *vcpu,
> + enum kvm_reg reg)
> +{
> + __set_bit(reg, (unsigned long *)&vcpu->arch.regs_avail);
> + __set_bit(reg, (unsigned long *)&vcpu->arch.regs_dirty);
> +}
> +
Personal preference again, but I would've named this
"kvm_register_mark_avail_dirty" to indicate what we're actually doing
(and maybe even shortened 'kvm_register_' to 'kvm_reg_' everywhere as I
can't see how 'reg' could be misread).
> static inline unsigned long kvm_register_read(struct kvm_vcpu *vcpu, int reg)
> {
> if (WARN_ON_ONCE((unsigned int)reg >= NR_VCPU_REGS))
> return 0;
>
> - if (!test_bit(reg, (unsigned long *)&vcpu->arch.regs_avail))
> + if (!kvm_register_is_available(vcpu, reg))
> kvm_x86_ops->cache_reg(vcpu, reg);
>
> return vcpu->arch.regs[reg];
> @@ -55,13 +80,12 @@ static inline void kvm_register_write(struct kvm_vcpu *vcpu, int reg,
> return;
>
> vcpu->arch.regs[reg] = val;
> - __set_bit(reg, (unsigned long *)&vcpu->arch.regs_dirty);
> - __set_bit(reg, (unsigned long *)&vcpu->arch.regs_avail);
> + kvm_register_mark_dirty(vcpu, reg);
> }
>
> static inline unsigned long kvm_rip_read(struct kvm_vcpu *vcpu)
> {
> - if (!test_bit(VCPU_REGS_RIP, (unsigned long *)&vcpu->arch.regs_avail))
> + if (!kvm_register_is_available(vcpu, VCPU_REGS_RIP))
> kvm_x86_ops->cache_reg(vcpu, VCPU_REGS_RIP);
>
> return vcpu->arch.regs[VCPU_REGS_RIP];
> @@ -70,13 +94,12 @@ static inline unsigned long kvm_rip_read(struct kvm_vcpu *vcpu)
> static inline void kvm_rip_write(struct kvm_vcpu *vcpu, unsigned long val)
> {
> vcpu->arch.regs[VCPU_REGS_RIP] = val;
> - __set_bit(VCPU_REGS_RIP, (unsigned long *)&vcpu->arch.regs_dirty);
> - __set_bit(VCPU_REGS_RIP, (unsigned long *)&vcpu->arch.regs_avail);
> + kvm_register_mark_dirty(vcpu, VCPU_REGS_RIP);
> }
>
> static inline unsigned long kvm_rsp_read(struct kvm_vcpu *vcpu)
> {
> - if (!test_bit(VCPU_REGS_RSP, (unsigned long *)&vcpu->arch.regs_avail))
> + if (!kvm_register_is_available(vcpu, VCPU_REGS_RSP))
> kvm_x86_ops->cache_reg(vcpu, VCPU_REGS_RSP);
>
> return vcpu->arch.regs[VCPU_REGS_RSP];
> @@ -85,16 +108,14 @@ static inline unsigned long kvm_rsp_read(struct kvm_vcpu *vcpu)
> static inline void kvm_rsp_write(struct kvm_vcpu *vcpu, unsigned long val)
> {
> vcpu->arch.regs[VCPU_REGS_RSP] = val;
> - __set_bit(VCPU_REGS_RSP, (unsigned long *)&vcpu->arch.regs_dirty);
> - __set_bit(VCPU_REGS_RSP, (unsigned long *)&vcpu->arch.regs_avail);
> + kvm_register_mark_dirty(vcpu, VCPU_REGS_RSP);
> }
>
> static inline u64 kvm_pdptr_read(struct kvm_vcpu *vcpu, int index)
> {
> might_sleep(); /* on svm */
>
> - if (!test_bit(VCPU_EXREG_PDPTR,
> - (unsigned long *)&vcpu->arch.regs_avail))
> + if (!kvm_register_is_available(vcpu, VCPU_EXREG_PDPTR))
> kvm_x86_ops->cache_reg(vcpu, VCPU_EXREG_PDPTR);
>
> return vcpu->arch.walk_mmu->pdptrs[index];
> @@ -123,7 +144,7 @@ static inline ulong kvm_read_cr4_bits(struct kvm_vcpu *vcpu, ulong mask)
>
> static inline ulong kvm_read_cr3(struct kvm_vcpu *vcpu)
> {
> - if (!test_bit(VCPU_EXREG_CR3, (ulong *)&vcpu->arch.regs_avail))
> + if (!kvm_register_is_available(vcpu, VCPU_EXREG_CR3))
> kvm_x86_ops->decache_cr3(vcpu);
> return vcpu->arch.cr3;
> }
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index b72a00b53e4a..8946f11c574c 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -1012,7 +1012,7 @@ static int nested_vmx_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3, bool ne
> kvm_mmu_new_cr3(vcpu, cr3, false);
>
> vcpu->arch.cr3 = cr3;
> - __set_bit(VCPU_EXREG_CR3, (ulong *)&vcpu->arch.regs_avail);
> + kvm_register_mark_available(vcpu, VCPU_EXREG_CR3);
>
> kvm_init_mmu(vcpu, false);
>
> @@ -3986,7 +3986,7 @@ static void nested_vmx_restore_host_state(struct kvm_vcpu *vcpu)
>
> nested_ept_uninit_mmu_context(vcpu);
> vcpu->arch.cr3 = vmcs_readl(GUEST_CR3);
> - __set_bit(VCPU_EXREG_CR3, (ulong *)&vcpu->arch.regs_avail);
> + kvm_register_mark_available(vcpu, VCPU_EXREG_CR3);
>
> /*
> * Use ept_save_pdptrs(vcpu) to load the MMU's cached PDPTRs
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 814d3e6d0264..ed03d0cd1cc8 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -721,8 +721,8 @@ static bool vmx_segment_cache_test_set(struct vcpu_vmx *vmx, unsigned seg,
> bool ret;
> u32 mask = 1 << (seg * SEG_FIELD_NR + field);
>
> - if (!(vmx->vcpu.arch.regs_avail & (1 << VCPU_EXREG_SEGMENTS))) {
> - vmx->vcpu.arch.regs_avail |= (1 << VCPU_EXREG_SEGMENTS);
> + if (!kvm_register_is_available(&vmx->vcpu, VCPU_EXREG_SEGMENTS)) {
> + kvm_register_mark_available(&vmx->vcpu, VCPU_EXREG_SEGMENTS);
> vmx->segment_cache.bitmask = 0;
> }
> ret = vmx->segment_cache.bitmask & mask;
> @@ -1410,8 +1410,8 @@ unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> unsigned long rflags, save_rflags;
>
> - if (!test_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail)) {
> - __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail);
> + if (!kvm_register_is_available(vcpu, VCPU_EXREG_RFLAGS)) {
> + kvm_register_mark_available(vcpu, VCPU_EXREG_RFLAGS);
> rflags = vmcs_readl(GUEST_RFLAGS);
> if (vmx->rmode.vm86_active) {
> rflags &= RMODE_GUEST_OWNED_EFLAGS_BITS;
> @@ -1429,7 +1429,7 @@ void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
> unsigned long old_rflags;
>
> if (enable_unrestricted_guest) {
> - __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail);
> + kvm_register_mark_available(vcpu, VCPU_EXREG_RFLAGS);
>
> vmx->rflags = rflags;
> vmcs_writel(GUEST_RFLAGS, rflags);
> @@ -2175,7 +2175,8 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>
> static void vmx_cache_reg(struct kvm_vcpu *vcpu, enum kvm_reg reg)
> {
> - __set_bit(reg, (unsigned long *)&vcpu->arch.regs_avail);
> + kvm_register_mark_available(vcpu, reg);
> +
> switch (reg) {
> case VCPU_REGS_RSP:
> vcpu->arch.regs[VCPU_REGS_RSP] = vmcs_readl(GUEST_RSP);
> @@ -2862,7 +2863,7 @@ static void vmx_decache_cr3(struct kvm_vcpu *vcpu)
> {
> if (enable_unrestricted_guest || (enable_ept && is_paging(vcpu)))
> vcpu->arch.cr3 = vmcs_readl(GUEST_CR3);
> - __set_bit(VCPU_EXREG_CR3, (ulong *)&vcpu->arch.regs_avail);
> + kvm_register_mark_available(vcpu, VCPU_EXREG_CR3);
> }
>
> static void vmx_decache_cr4_guest_bits(struct kvm_vcpu *vcpu)
> @@ -2877,8 +2878,7 @@ static void ept_load_pdptrs(struct kvm_vcpu *vcpu)
> {
> struct kvm_mmu *mmu = vcpu->arch.walk_mmu;
>
> - if (!test_bit(VCPU_EXREG_PDPTR,
> - (unsigned long *)&vcpu->arch.regs_dirty))
> + if (!kvm_register_is_dirty(vcpu, VCPU_EXREG_PDPTR))
> return;
>
> if (is_pae_paging(vcpu)) {
> @@ -2900,10 +2900,7 @@ void ept_save_pdptrs(struct kvm_vcpu *vcpu)
> mmu->pdptrs[3] = vmcs_read64(GUEST_PDPTR3);
> }
>
> - __set_bit(VCPU_EXREG_PDPTR,
> - (unsigned long *)&vcpu->arch.regs_avail);
> - __set_bit(VCPU_EXREG_PDPTR,
> - (unsigned long *)&vcpu->arch.regs_dirty);
> + kvm_register_mark_dirty(vcpu, VCPU_EXREG_PDPTR);
> }
>
> static void ept_update_paging_mode_cr0(unsigned long *hw_cr0,
> @@ -2912,7 +2909,7 @@ static void ept_update_paging_mode_cr0(unsigned long *hw_cr0,
> {
> struct vcpu_vmx *vmx = to_vmx(vcpu);
>
> - if (!test_bit(VCPU_EXREG_CR3, (ulong *)&vcpu->arch.regs_avail))
> + if (!kvm_register_is_available(vcpu, VCPU_EXREG_CR3))
> vmx_decache_cr3(vcpu);
> if (!(cr0 & X86_CR0_PG)) {
> /* From paging/starting to nonpaging */
> @@ -6528,9 +6525,9 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
> if (vmx->nested.need_vmcs12_to_shadow_sync)
> nested_sync_vmcs12_to_shadow(vcpu);
>
> - if (test_bit(VCPU_REGS_RSP, (unsigned long *)&vcpu->arch.regs_dirty))
> + if (kvm_register_is_dirty(vcpu, VCPU_REGS_RSP))
> vmcs_writel(GUEST_RSP, vcpu->arch.regs[VCPU_REGS_RSP]);
> - if (test_bit(VCPU_REGS_RIP, (unsigned long *)&vcpu->arch.regs_dirty))
> + if (kvm_register_is_dirty(vcpu, VCPU_REGS_RIP))
> vmcs_writel(GUEST_RIP, vcpu->arch.regs[VCPU_REGS_RIP]);
>
> cr3 = __get_current_cr3_fast();
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 0ed07d8d2caa..cd6bd7991c39 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -709,10 +709,8 @@ int load_pdptrs(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, unsigned long cr3)
> ret = 1;
>
> memcpy(mmu->pdptrs, pdpte, sizeof(mmu->pdptrs));
> - __set_bit(VCPU_EXREG_PDPTR,
> - (unsigned long *)&vcpu->arch.regs_avail);
> - __set_bit(VCPU_EXREG_PDPTR,
> - (unsigned long *)&vcpu->arch.regs_dirty);
> + kvm_register_mark_dirty(vcpu, VCPU_EXREG_PDPTR);
> +
> out:
>
> return ret;
> @@ -730,8 +728,7 @@ bool pdptrs_changed(struct kvm_vcpu *vcpu)
> if (!is_pae_paging(vcpu))
> return false;
>
> - if (!test_bit(VCPU_EXREG_PDPTR,
> - (unsigned long *)&vcpu->arch.regs_avail))
> + if (!kvm_register_is_available(vcpu, VCPU_EXREG_PDPTR))
> return true;
>
> gfn = (kvm_read_cr3(vcpu) & 0xffffffe0ul) >> PAGE_SHIFT;
> @@ -976,7 +973,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
>
> kvm_mmu_new_cr3(vcpu, cr3, skip_tlb_flush);
> vcpu->arch.cr3 = cr3;
> - __set_bit(VCPU_EXREG_CR3, (ulong *)&vcpu->arch.regs_avail);
> + kvm_register_mark_available(vcpu, VCPU_EXREG_CR3);
>
> return 0;
> }
> @@ -8766,7 +8763,7 @@ static int __set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs)
> vcpu->arch.cr2 = sregs->cr2;
> mmu_reset_needed |= kvm_read_cr3(vcpu) != sregs->cr3;
> vcpu->arch.cr3 = sregs->cr3;
> - __set_bit(VCPU_EXREG_CR3, (ulong *)&vcpu->arch.regs_avail);
> + kvm_register_mark_available(vcpu, VCPU_EXREG_CR3);
>
> kvm_set_cr8(vcpu, sregs->cr8);
--
Vitaly
next prev parent reply other threads:[~2019-09-30 9:32 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-27 21:45 [PATCH v2 0/8] KVM: x86: nVMX GUEST_CR3 bug fix, and then some Sean Christopherson
2019-09-27 21:45 ` [PATCH v2 1/8] KVM: nVMX: Always write vmcs02.GUEST_CR3 during nested VM-Enter Sean Christopherson
2019-09-27 23:37 ` Jim Mattson
2019-09-27 21:45 ` [PATCH v2 2/8] KVM: VMX: Skip GUEST_CR3 VMREAD+VMWRITE if the VMCS is up-to-date Sean Christopherson
2019-09-27 21:45 ` [PATCH v2 3/8] KVM: VMX: Consolidate to_vmx() usage in RFLAGS accessors Sean Christopherson
2019-09-30 8:48 ` Vitaly Kuznetsov
2019-09-27 21:45 ` [PATCH v2 4/8] KVM: VMX: Optimize vmx_set_rflags() for unrestricted guest Sean Christopherson
2019-09-30 8:57 ` Vitaly Kuznetsov
2019-09-30 15:19 ` Sean Christopherson
2019-09-30 15:55 ` Vitaly Kuznetsov
2019-10-09 10:40 ` Paolo Bonzini
2019-10-09 16:38 ` Sean Christopherson
2019-10-09 20:59 ` Paolo Bonzini
2019-10-09 21:30 ` Sean Christopherson
2019-09-27 21:45 ` [PATCH v2 5/8] KVM: x86: Add WARNs to detect out-of-bounds register indices Sean Christopherson
2019-09-30 9:19 ` Vitaly Kuznetsov
2019-10-09 10:50 ` Paolo Bonzini
2019-10-09 16:36 ` Sean Christopherson
2019-09-27 21:45 ` [PATCH v2 6/8] KVM: x86: Fold 'enum kvm_ex_reg' definitions into 'enum kvm_reg' Sean Christopherson
2019-09-30 9:25 ` Vitaly Kuznetsov
2019-10-09 10:52 ` Paolo Bonzini
2019-10-09 11:27 ` Vitaly Kuznetsov
2019-09-27 21:45 ` [PATCH v2 7/8] KVM: x86: Add helpers to test/mark reg availability and dirtiness Sean Christopherson
2019-09-30 9:32 ` Vitaly Kuznetsov [this message]
2019-10-09 11:00 ` Paolo Bonzini
2019-09-27 21:45 ` [PATCH v2 8/8] KVM: x86: Fold decache_cr3() into cache_reg() Sean Christopherson
2019-09-30 10:58 ` Vitaly Kuznetsov
2019-09-30 15:04 ` Sean Christopherson
2019-09-30 15:27 ` Vitaly Kuznetsov
2019-09-30 15:33 ` Sean Christopherson
2019-10-09 11:03 ` Paolo Bonzini
2019-09-30 10:42 ` [PATCH v2 0/8] KVM: x86: nVMX GUEST_CR3 bug fix, and then some Reto Buerki
2019-10-29 15:03 ` Martin Lucina
2019-10-30 9:09 ` Sean Christopherson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87d0fi3zai.fsf@vitty.brq.redhat.com \
--to=vkuznets@redhat.com \
--cc=jmattson@google.com \
--cc=joro@8bytes.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=liran.alon@oracle.com \
--cc=pbonzini@redhat.com \
--cc=reet@codelabs.ch \
--cc=rkrcmar@redhat.com \
--cc=sean.j.christopherson@intel.com \
--cc=wanpengli@tencent.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).