Re: [RFC PATCH v5 037/104] KVM: x86/mmu: Allow non-zero init value for shadow PTE

[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