@@ -470,6 +470,7 @@ struct kvm_mmu {
int (*sync_spte)(struct kvm_vcpu *vcpu,
struct kvm_mmu_page *sp, int i);
struct kvm_mmu_root_info root;
+ hpa_t private_root_hpa;
Should we have
struct kvm_mmu_root_info private_root;
instead?
This is corresponds to:
mmu->root.hpa
We don't need the other fields, so I think better to not take space. It does
look asymmetric though...
Being symmetric is why I asked. Anyway no strong opinion.
[...]
@@ -4685,7 +4687,7 @@ int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct
kvm_page_fault *fault)
if (kvm_mmu_honors_guest_mtrrs(vcpu->kvm)) {
for ( ; fault->max_level > PG_LEVEL_4K; --fault->max_level)
{
int page_num = KVM_PAGES_PER_HPAGE(fault-
max_level);
- gfn_t base = gfn_round_for_level(fault->gfn,
+ gfn_t base = gfn_round_for_level(gpa_to_gfn(fault-
addr),
fault->max_level);
I thought by reaching here the shared bit has already been stripped away
by the caller?
We don't support MTRRs so this code wont be executed for TDX, but not clear what
you are asking.
fault->addr has the shared bit (if present)
fault->gfn has it stripped.
When I was looking at the code, I thought fault->gfn is still having the
shred bit, and gpa_to_gfn() internally strips aways the shared bit, but
sorry it is not true.
My question is why do we even need this change? Souldn't we pass the
actual GFN (which doesn't have the shared bit) to
kvm_mtrr_check_gfn_range_consistency()?
If so, looks we should use fault->gfn to get the base?
It doesn't make a lot sense to still have it here, given we have a
universal KVM-defined PFERR_PRIVATE_ACCESS flag:
https://lore.kernel.org/kvm/20240507155817.3951344-2-pbonzini@xxxxxxxxxx/T/#mb30987f31b431771b42dfa64dcaa2efbc10ada5e
IMHO we should just strip the shared bit in the TDX variant of
handle_ept_violation(), and pass the PFERR_PRIVATE_ACCESS (when GPA
doesn't hvae shared bit) to the common fault handler so it can correctly
set fault->is_private to true.
I'm not sure what you are seeing here, could elaborate?
See reply below.
[...]
Anyway, from common code's perspective, we need to have some
clarification why we design to do it here.
free_mmu_pages(&vcpu->arch.root_mmu);
free_mmu_pages(&vcpu->arch.guest_mmu);
mmu_free_memory_caches(vcpu);
diff --git a/arch/x86/kvm/mmu/mmu_internal.h
b/arch/x86/kvm/mmu/mmu_internal.h
index 0f1a9d733d9e..3a7fe9261e23 100644
--- a/arch/x86/kvm/mmu/mmu_internal.h
+++ b/arch/x86/kvm/mmu/mmu_internal.h
@@ -6,6 +6,8 @@
#include <linux/kvm_host.h>
#include <asm/kvm_host.h>
+#include "mmu.h"
+
#ifdef CONFIG_KVM_PROVE_MMU
#define KVM_MMU_WARN_ON(x) WARN_ON_ONCE(x)
#else
@@ -178,6 +180,16 @@ static inline void kvm_mmu_alloc_private_spt(struct
kvm_vcpu *vcpu, struct kvm_m
sp->private_spt = kvm_mmu_memory_cache_alloc(&vcpu-
arch.mmu_private_spt_cache);
}
+static inline gfn_t kvm_gfn_for_root(struct kvm *kvm, struct kvm_mmu_page
*root,
+ gfn_t gfn)
+{
+ gfn_t gfn_for_root = kvm_gfn_to_private(kvm, gfn);
+
+ /* Set shared bit if not private */
+ gfn_for_root |= -(gfn_t)!is_private_sp(root) &
kvm_gfn_shared_mask(kvm);
+ return gfn_for_root;
+}
+
static inline bool kvm_mmu_page_ad_need_write_protect(struct kvm_mmu_page
*sp)
{
/*
@@ -348,7 +360,12 @@ static inline int __kvm_mmu_do_page_fault(struct
kvm_vcpu *vcpu, gpa_t cr2_or_gp
int r;
if (vcpu->arch.mmu->root_role.direct) {
- fault.gfn = fault.addr >> PAGE_SHIFT;
+ /*
+ * Things like memslots don't understand the concept of a
shared
+ * bit. Strip it so that the GFN can be used like normal,
and the
+ * fault.addr can be used when the shared bit is needed.
+ */
+ fault.gfn = gpa_to_gfn(fault.addr) &
~kvm_gfn_shared_mask(vcpu->kvm);
fault.slot = kvm_vcpu_gfn_to_memslot(vcpu, fault.gfn);
Again, I don't think it's nessary for fault.gfn to still have the shared
bit here?
It's getting stripped as it's set for the first time... What do you mean still
have it?
Sorry, I meant fault->addr.
This kinda usage is pretty much the reason I want to get rid of
kvm_gfn_shared_mask().
I think you want to move it to an x86_op right? Not get rid of the concept of a
shared bit? I think KVM will have a hard time doing TDX without knowing about
the shared bit location.
Or maybe you are saying you think it should be stripped earlier and live as a PF
error code?
I meant it seems we should just strip shared bit away from the GPA in
handle_ept_violation() and pass it as 'cr2_or_gpa' here, so fault->addr
won't have the shared bit.
Do you see any problem of doing so?
}
diff --git a/arch/x86/kvm/mmu/tdp_iter.h b/arch/x86/kvm/mmu/tdp_iter.h
index fae559559a80..8a64bcef9deb 100644
--- a/arch/x86/kvm/mmu/tdp_iter.h
+++ b/arch/x86/kvm/mmu/tdp_iter.h
@@ -91,7 +91,7 @@ struct tdp_iter {
tdp_ptep_t pt_path[PT64_ROOT_MAX_LEVEL];
/* A pointer to the current SPTE */
tdp_ptep_t sptep;
- /* The lowest GFN mapped by the current SPTE */
+ /* The lowest GFN (shared bits included) mapped by the current SPTE
*/
gfn_t gfn;
IMHO we need more clarification of this design.
Have you seen the documentation patch? Where do you think it should be? You mean
in the tdp_iter struct?
My thinking:
Changelog should clarify why include shared bit to 'gfn' in tdp_iter.
And here around the 'gfn' we can have some simple sentence to explain
why to include the shared bit.
