| Message ID | 20240227232100.478238-7-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TDX/SNP part 1 of n, for 6.9 | expand |
On 2/28/2024 7:20 AM, Paolo Bonzini wrote: > From: Sean Christopherson <seanjc@google.com> > > TDX will use a different shadow PTE entry value for MMIO from VMX. Add > members to kvm_arch and track value for MMIO per-VM instead of global > variables. By using the per-VM EPT entry value for MMIO, the existing VMX > logic is kept working. Introduce a separate setter function so that guest > TD can override later. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > Message-Id: <229a18434e5d83f45b1fcd7bf1544d79db1becb6.1705965635.git.isaku.yamahata@intel.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > arch/x86/include/asm/kvm_host.h | 2 ++ > arch/x86/kvm/mmu.h | 1 + > arch/x86/kvm/mmu/mmu.c | 8 +++++--- > arch/x86/kvm/mmu/spte.c | 10 ++++++++-- > arch/x86/kvm/mmu/spte.h | 4 ++-- > arch/x86/kvm/mmu/tdp_mmu.c | 6 +++--- > 6 files changed, 21 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 85dc0f7d09e3..a4514c2ef0ec 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -1313,6 +1313,8 @@ struct kvm_arch { > */ > spinlock_t mmu_unsync_pages_lock; > > + u64 shadow_mmio_value; > + > struct iommu_domain *iommu_domain; > bool iommu_noncoherent; > #define __KVM_HAVE_ARCH_NONCOHERENT_DMA > diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h > index 60f21bb4c27b..2c54ba5b0a28 100644 > --- a/arch/x86/kvm/mmu.h > +++ b/arch/x86/kvm/mmu.h > @@ -101,6 +101,7 @@ static inline u8 kvm_get_shadow_phys_bits(void) > } > > void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 mmio_mask, u64 access_mask); > +void kvm_mmu_set_mmio_spte_value(struct kvm *kvm, u64 mmio_value); Now, the new added function along with above function mislead people on their names. It's easily to think - kvm_mmu_set_mmio_spte_mask() is to set shadow_mmio_mask, while - kvm_mmu_set_mmio_spte_value() is to set shadow_mmio_value we'd better come up with a better name. Other than it, the patch looks good to me. Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>
On 2/28/2024 7:20 AM, Paolo Bonzini wrote: > From: Sean Christopherson <seanjc@google.com> > > TDX will use a different shadow PTE entry value for MMIO from VMX. Add > members to kvm_arch and track value for MMIO per-VM instead of global Nit: members -> a member, since only 'shadow_mmio_value' is added. > variables. By using the per-VM EPT entry value for MMIO, the existing VMX > logic is kept working. Introduce a separate setter function so that guest > TD can override later. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > Message-Id: <229a18434e5d83f45b1fcd7bf1544d79db1becb6.1705965635.git.isaku.yamahata@intel.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com> > --- > arch/x86/include/asm/kvm_host.h | 2 ++ > arch/x86/kvm/mmu.h | 1 + > arch/x86/kvm/mmu/mmu.c | 8 +++++--- > arch/x86/kvm/mmu/spte.c | 10 ++++++++-- > arch/x86/kvm/mmu/spte.h | 4 ++-- > arch/x86/kvm/mmu/tdp_mmu.c | 6 +++--- > 6 files changed, 21 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 85dc0f7d09e3..a4514c2ef0ec 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -1313,6 +1313,8 @@ struct kvm_arch { > */ > spinlock_t mmu_unsync_pages_lock; > > + u64 shadow_mmio_value; > + > struct iommu_domain *iommu_domain; > bool iommu_noncoherent; > #define __KVM_HAVE_ARCH_NONCOHERENT_DMA > diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h > index 60f21bb4c27b..2c54ba5b0a28 100644 > --- a/arch/x86/kvm/mmu.h > +++ b/arch/x86/kvm/mmu.h > @@ -101,6 +101,7 @@ static inline u8 kvm_get_shadow_phys_bits(void) > } > > void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 mmio_mask, u64 access_mask); > +void kvm_mmu_set_mmio_spte_value(struct kvm *kvm, u64 mmio_value); > void kvm_mmu_set_me_spte_mask(u64 me_value, u64 me_mask); > void kvm_mmu_set_ept_masks(bool has_ad_bits, bool has_exec_only); > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index b5baf11359ad..195e46a1f00f 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -2515,7 +2515,7 @@ static int mmu_page_zap_pte(struct kvm *kvm, struct kvm_mmu_page *sp, > return kvm_mmu_prepare_zap_page(kvm, child, > invalid_list); > } > - } else if (is_mmio_spte(pte)) { > + } else if (is_mmio_spte(kvm, pte)) { > mmu_spte_clear_no_track(spte); > } > return 0; > @@ -4197,7 +4197,7 @@ static int handle_mmio_page_fault(struct kvm_vcpu *vcpu, u64 addr, bool direct) > if (WARN_ON_ONCE(reserved)) > return -EINVAL; > > - if (is_mmio_spte(spte)) { > + if (is_mmio_spte(vcpu->kvm, spte)) { > gfn_t gfn = get_mmio_spte_gfn(spte); > unsigned int access = get_mmio_spte_access(spte); > > @@ -4813,7 +4813,7 @@ EXPORT_SYMBOL_GPL(kvm_mmu_new_pgd); > static bool sync_mmio_spte(struct kvm_vcpu *vcpu, u64 *sptep, gfn_t gfn, > unsigned int access) > { > - if (unlikely(is_mmio_spte(*sptep))) { > + if (unlikely(is_mmio_spte(vcpu->kvm, *sptep))) { > if (gfn != get_mmio_spte_gfn(*sptep)) { > mmu_spte_clear_no_track(sptep); > return true; > @@ -6320,6 +6320,8 @@ static bool kvm_has_zapped_obsolete_pages(struct kvm *kvm) > > void kvm_mmu_init_vm(struct kvm *kvm) > { > + > + kvm->arch.shadow_mmio_value = shadow_mmio_value; > INIT_LIST_HEAD(&kvm->arch.active_mmu_pages); > INIT_LIST_HEAD(&kvm->arch.zapped_obsolete_pages); > INIT_LIST_HEAD(&kvm->arch.possible_nx_huge_pages); > diff --git a/arch/x86/kvm/mmu/spte.c b/arch/x86/kvm/mmu/spte.c > index 02a466de2991..318135daf685 100644 > --- a/arch/x86/kvm/mmu/spte.c > +++ b/arch/x86/kvm/mmu/spte.c > @@ -74,10 +74,10 @@ u64 make_mmio_spte(struct kvm_vcpu *vcpu, u64 gfn, unsigned int access) > u64 spte = generation_mmio_spte_mask(gen); > u64 gpa = gfn << PAGE_SHIFT; > > - WARN_ON_ONCE(!shadow_mmio_value); > + WARN_ON_ONCE(!vcpu->kvm->arch.shadow_mmio_value); > > access &= shadow_mmio_access_mask; > - spte |= shadow_mmio_value | access; > + spte |= vcpu->kvm->arch.shadow_mmio_value | access; > spte |= gpa | shadow_nonpresent_or_rsvd_mask; > spte |= (gpa & shadow_nonpresent_or_rsvd_mask) > << SHADOW_NONPRESENT_OR_RSVD_MASK_LEN; > @@ -411,6 +411,12 @@ void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 mmio_mask, u64 access_mask) > } > EXPORT_SYMBOL_GPL(kvm_mmu_set_mmio_spte_mask); > > +void kvm_mmu_set_mmio_spte_value(struct kvm *kvm, u64 mmio_value) > +{ > + kvm->arch.shadow_mmio_value = mmio_value; > +} > +EXPORT_SYMBOL_GPL(kvm_mmu_set_mmio_spte_value); > + > void kvm_mmu_set_me_spte_mask(u64 me_value, u64 me_mask) > { > /* shadow_me_value must be a subset of shadow_me_mask */ > diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h > index 26bc95bbc962..1a163aee9ec6 100644 > --- a/arch/x86/kvm/mmu/spte.h > +++ b/arch/x86/kvm/mmu/spte.h > @@ -264,9 +264,9 @@ static inline struct kvm_mmu_page *root_to_sp(hpa_t root) > return spte_to_child_sp(root); > } > > -static inline bool is_mmio_spte(u64 spte) > +static inline bool is_mmio_spte(struct kvm *kvm, u64 spte) > { > - return (spte & shadow_mmio_mask) == shadow_mmio_value && > + return (spte & shadow_mmio_mask) == kvm->arch.shadow_mmio_value && > likely(enable_mmio_caching); > } > > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c > index c8a4d92497b4..d15c44a8e123 100644 > --- a/arch/x86/kvm/mmu/tdp_mmu.c > +++ b/arch/x86/kvm/mmu/tdp_mmu.c > @@ -495,8 +495,8 @@ static void handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn, > * impact the guest since both the former and current SPTEs > * are nonpresent. > */ > - if (WARN_ON_ONCE(!is_mmio_spte(old_spte) && > - !is_mmio_spte(new_spte) && > + if (WARN_ON_ONCE(!is_mmio_spte(kvm, old_spte) && > + !is_mmio_spte(kvm, new_spte) && > !is_removed_spte(new_spte))) > pr_err("Unexpected SPTE change! Nonpresent SPTEs\n" > "should not be replaced with another,\n" > @@ -1028,7 +1028,7 @@ static int tdp_mmu_map_handle_target_level(struct kvm_vcpu *vcpu, > } > > /* If a MMIO SPTE is installed, the MMIO will need to be emulated. */ > - if (unlikely(is_mmio_spte(new_spte))) { > + if (unlikely(is_mmio_spte(vcpu->kvm, new_spte))) { > vcpu->stat.pf_mmio_spte_created++; > trace_mark_mmio_spte(rcu_dereference(iter->sptep), iter->gfn, > new_spte);
On 28/02/2024 12:20 pm, Paolo Bonzini wrote: > From: Sean Christopherson <seanjc@google.com> > > TDX will use a different shadow PTE entry value for MMIO from VMX. Add > members to kvm_arch and track value for MMIO per-VM instead of global "members" -> "member". > variables. "variables" -> "variable". By using the per-VM EPT entry value for MMIO, the existing VMX > logic is kept working. Introduce a separate setter function so that guest > TD can override later. "guest TD can override " -> "KVM can override for TDX guest". > > Signed-off-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > Message-Id: <229a18434e5d83f45b1fcd7bf1544d79db1becb6.1705965635.git.isaku.yamahata@intel.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > arch/x86/include/asm/kvm_host.h | 2 ++ > arch/x86/kvm/mmu.h | 1 + > arch/x86/kvm/mmu/mmu.c | 8 +++++--- > arch/x86/kvm/mmu/spte.c | 10 ++++++++-- > arch/x86/kvm/mmu/spte.h | 4 ++-- > arch/x86/kvm/mmu/tdp_mmu.c | 6 +++--- > 6 files changed, 21 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 85dc0f7d09e3..a4514c2ef0ec 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -1313,6 +1313,8 @@ struct kvm_arch { > */ > spinlock_t mmu_unsync_pages_lock; > > + u64 shadow_mmio_value; > + > struct iommu_domain *iommu_domain; > bool iommu_noncoherent; > #define __KVM_HAVE_ARCH_NONCOHERENT_DMA > diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h > index 60f21bb4c27b..2c54ba5b0a28 100644 > --- a/arch/x86/kvm/mmu.h > +++ b/arch/x86/kvm/mmu.h > @@ -101,6 +101,7 @@ static inline u8 kvm_get_shadow_phys_bits(void) > } > > void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 mmio_mask, u64 access_mask); > +void kvm_mmu_set_mmio_spte_value(struct kvm *kvm, u64 mmio_value); > void kvm_mmu_set_me_spte_mask(u64 me_value, u64 me_mask); > void kvm_mmu_set_ept_masks(bool has_ad_bits, bool has_exec_only); > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index b5baf11359ad..195e46a1f00f 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -2515,7 +2515,7 @@ static int mmu_page_zap_pte(struct kvm *kvm, struct kvm_mmu_page *sp, > return kvm_mmu_prepare_zap_page(kvm, child, > invalid_list); > } > - } else if (is_mmio_spte(pte)) { > + } else if (is_mmio_spte(kvm, pte)) { > mmu_spte_clear_no_track(spte); > } > return 0; > @@ -4197,7 +4197,7 @@ static int handle_mmio_page_fault(struct kvm_vcpu *vcpu, u64 addr, bool direct) > if (WARN_ON_ONCE(reserved)) > return -EINVAL; > > - if (is_mmio_spte(spte)) { > + if (is_mmio_spte(vcpu->kvm, spte)) { > gfn_t gfn = get_mmio_spte_gfn(spte); > unsigned int access = get_mmio_spte_access(spte); > > @@ -4813,7 +4813,7 @@ EXPORT_SYMBOL_GPL(kvm_mmu_new_pgd); > static bool sync_mmio_spte(struct kvm_vcpu *vcpu, u64 *sptep, gfn_t gfn, > unsigned int access) > { > - if (unlikely(is_mmio_spte(*sptep))) { > + if (unlikely(is_mmio_spte(vcpu->kvm, *sptep))) { > if (gfn != get_mmio_spte_gfn(*sptep)) { > mmu_spte_clear_no_track(sptep); > return true; > @@ -6320,6 +6320,8 @@ static bool kvm_has_zapped_obsolete_pages(struct kvm *kvm) > > void kvm_mmu_init_vm(struct kvm *kvm) > { > + > + kvm->arch.shadow_mmio_value = shadow_mmio_value; Unintended blank line. [...] Acked-by: Kai Huang <kai.huang@intel.com>
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 85dc0f7d09e3..a4514c2ef0ec 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1313,6 +1313,8 @@ struct kvm_arch { */ spinlock_t mmu_unsync_pages_lock; + u64 shadow_mmio_value; + struct iommu_domain *iommu_domain; bool iommu_noncoherent; #define __KVM_HAVE_ARCH_NONCOHERENT_DMA diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h index 60f21bb4c27b..2c54ba5b0a28 100644 --- a/arch/x86/kvm/mmu.h +++ b/arch/x86/kvm/mmu.h @@ -101,6 +101,7 @@ static inline u8 kvm_get_shadow_phys_bits(void) } void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 mmio_mask, u64 access_mask); +void kvm_mmu_set_mmio_spte_value(struct kvm *kvm, u64 mmio_value); void kvm_mmu_set_me_spte_mask(u64 me_value, u64 me_mask); void kvm_mmu_set_ept_masks(bool has_ad_bits, bool has_exec_only); diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index b5baf11359ad..195e46a1f00f 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -2515,7 +2515,7 @@ static int mmu_page_zap_pte(struct kvm *kvm, struct kvm_mmu_page *sp, return kvm_mmu_prepare_zap_page(kvm, child, invalid_list); } - } else if (is_mmio_spte(pte)) { + } else if (is_mmio_spte(kvm, pte)) { mmu_spte_clear_no_track(spte); } return 0; @@ -4197,7 +4197,7 @@ static int handle_mmio_page_fault(struct kvm_vcpu *vcpu, u64 addr, bool direct) if (WARN_ON_ONCE(reserved)) return -EINVAL; - if (is_mmio_spte(spte)) { + if (is_mmio_spte(vcpu->kvm, spte)) { gfn_t gfn = get_mmio_spte_gfn(spte); unsigned int access = get_mmio_spte_access(spte); @@ -4813,7 +4813,7 @@ EXPORT_SYMBOL_GPL(kvm_mmu_new_pgd); static bool sync_mmio_spte(struct kvm_vcpu *vcpu, u64 *sptep, gfn_t gfn, unsigned int access) { - if (unlikely(is_mmio_spte(*sptep))) { + if (unlikely(is_mmio_spte(vcpu->kvm, *sptep))) { if (gfn != get_mmio_spte_gfn(*sptep)) { mmu_spte_clear_no_track(sptep); return true; @@ -6320,6 +6320,8 @@ static bool kvm_has_zapped_obsolete_pages(struct kvm *kvm) void kvm_mmu_init_vm(struct kvm *kvm) { + + kvm->arch.shadow_mmio_value = shadow_mmio_value; INIT_LIST_HEAD(&kvm->arch.active_mmu_pages); INIT_LIST_HEAD(&kvm->arch.zapped_obsolete_pages); INIT_LIST_HEAD(&kvm->arch.possible_nx_huge_pages); diff --git a/arch/x86/kvm/mmu/spte.c b/arch/x86/kvm/mmu/spte.c index 02a466de2991..318135daf685 100644 --- a/arch/x86/kvm/mmu/spte.c +++ b/arch/x86/kvm/mmu/spte.c @@ -74,10 +74,10 @@ u64 make_mmio_spte(struct kvm_vcpu *vcpu, u64 gfn, unsigned int access) u64 spte = generation_mmio_spte_mask(gen); u64 gpa = gfn << PAGE_SHIFT; - WARN_ON_ONCE(!shadow_mmio_value); + WARN_ON_ONCE(!vcpu->kvm->arch.shadow_mmio_value); access &= shadow_mmio_access_mask; - spte |= shadow_mmio_value | access; + spte |= vcpu->kvm->arch.shadow_mmio_value | access; spte |= gpa | shadow_nonpresent_or_rsvd_mask; spte |= (gpa & shadow_nonpresent_or_rsvd_mask) << SHADOW_NONPRESENT_OR_RSVD_MASK_LEN; @@ -411,6 +411,12 @@ void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 mmio_mask, u64 access_mask) } EXPORT_SYMBOL_GPL(kvm_mmu_set_mmio_spte_mask); +void kvm_mmu_set_mmio_spte_value(struct kvm *kvm, u64 mmio_value) +{ + kvm->arch.shadow_mmio_value = mmio_value; +} +EXPORT_SYMBOL_GPL(kvm_mmu_set_mmio_spte_value); + void kvm_mmu_set_me_spte_mask(u64 me_value, u64 me_mask) { /* shadow_me_value must be a subset of shadow_me_mask */ diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h index 26bc95bbc962..1a163aee9ec6 100644 --- a/arch/x86/kvm/mmu/spte.h +++ b/arch/x86/kvm/mmu/spte.h @@ -264,9 +264,9 @@ static inline struct kvm_mmu_page *root_to_sp(hpa_t root) return spte_to_child_sp(root); } -static inline bool is_mmio_spte(u64 spte) +static inline bool is_mmio_spte(struct kvm *kvm, u64 spte) { - return (spte & shadow_mmio_mask) == shadow_mmio_value && + return (spte & shadow_mmio_mask) == kvm->arch.shadow_mmio_value && likely(enable_mmio_caching); } diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index c8a4d92497b4..d15c44a8e123 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -495,8 +495,8 @@ static void handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn, * impact the guest since both the former and current SPTEs * are nonpresent. */ - if (WARN_ON_ONCE(!is_mmio_spte(old_spte) && - !is_mmio_spte(new_spte) && + if (WARN_ON_ONCE(!is_mmio_spte(kvm, old_spte) && + !is_mmio_spte(kvm, new_spte) && !is_removed_spte(new_spte))) pr_err("Unexpected SPTE change! Nonpresent SPTEs\n" "should not be replaced with another,\n" @@ -1028,7 +1028,7 @@ static int tdp_mmu_map_handle_target_level(struct kvm_vcpu *vcpu, } /* If a MMIO SPTE is installed, the MMIO will need to be emulated. */ - if (unlikely(is_mmio_spte(new_spte))) { + if (unlikely(is_mmio_spte(vcpu->kvm, new_spte))) { vcpu->stat.pf_mmio_spte_created++; trace_mark_mmio_spte(rcu_dereference(iter->sptep), iter->gfn, new_spte);