Messages in this thread | | | Date | Tue, 5 Apr 2022 16:13:33 +0200 | Subject | Re: [RFC PATCH v5 037/104] KVM: x86/mmu: Allow non-zero init value for shadow PTE | From | Paolo Bonzini <> |
| |
On 4/1/22 07:13, Kai Huang wrote: >> @@ -617,9 +617,9 @@ static int mmu_spte_clear_track_bits(struct kvm *kvm, u64 *sptep) >> int level = sptep_to_sp(sptep)->role.level; >> >> if (!spte_has_volatile_bits(old_spte)) >> - __update_clear_spte_fast(sptep, 0ull); >> + __update_clear_spte_fast(sptep, shadow_init_value); >> else >> - old_spte = __update_clear_spte_slow(sptep, 0ull); >> + old_spte = __update_clear_spte_slow(sptep, shadow_init_value);
(FWIW this one should also assume that the init_value is zero, and WARN if not).
> I guess it's better to have some comment here. Allow non-zero init value for > shadow PTE doesn't necessarily mean the initial value should be used when one > PTE is zapped. I think mmu_spte_clear_track_bits() is only called for mapping > of normal (shared) memory but not MMIO? Then perhaps it's better to have a > comment to explain we want "suppress #VE" set to get a real EPT violation for > normal memory access from guest? > >> >> if (!is_shadow_present_pte(old_spte)) >> return old_spte; >> @@ -651,7 +651,7 @@ static int mmu_spte_clear_track_bits(struct kvm *kvm, u64 *sptep) >> */ >> static void mmu_spte_clear_no_track(u64 *sptep) >> { >> - __update_clear_spte_fast(sptep, 0ull); >> + __update_clear_spte_fast(sptep, shadow_init_value); >> } > Similar here. Seems mmu_spte_clear_no_track() is used to zap non-leaf PTE which > doesn't require state tracking, so theoretically it can be set to 0. But this > seems is also called to zap MMIO PTE so looks need to set to shadow_init_value. > Anyway looks deserve a comment? > > Btw, Above two changes to mmu_spte_clear_track_bits() and > mmu_spte_clear_track_bits() seems a little bit out-of-scope of what this patch > claims to do. Allow non-zero init value for shadow PTE doesn't necessarily mean > the initial value should be used when one PTE is zapped. Maybe we can further > improve the patch title and commit message a little bit. Such as: Allow non- > zero value for empty (or invalid?) PTE? Non-present seems doesn't fit here.
I think shadow_init_value is not named well. Let's rename it to shadow_nonpresent_value, and the commit to "Allow non-zero value for non-present SPTE". That explains why it's used for zapping.
Paolo
| |