Re: [PATCH 3/6] KVM: MMU: Add EPT support

Yang, Sheng
2008-04-18T15:16:37Z

On Friday 18 April 2008 21:30:14 Anthony Liguori wrote:
> Yang, Sheng wrote:
> > @@ -1048,17 +1071,18 @@ static void mmu_set_spte(struct kvm_vcpu *vcpu,
> > u64 *shadow_pte,
> > * whether the guest actually used the pte (in order to detect
> > * demand paging).
> > */
> > - spte = PT_PRESENT_MASK | PT_DIRTY_MASK;
> > + spte = shadow_base_present_pte | shadow_dirty_mask;
> > if (!speculative)
> > pte_access |= PT_ACCESSED_MASK;
> > if (!dirty)
> > pte_access &= ~ACC_WRITE_MASK;
> > - if (!(pte_access & ACC_EXEC_MASK))
> > - spte |= PT64_NX_MASK;
> > -
> > - spte |= PT_PRESENT_MASK;
> > + if (pte_access & ACC_EXEC_MASK) {
> > + if (shadow_x_mask)
> > + spte |= shadow_x_mask;
> > + } else if (shadow_nx_mask)
> > + spte |= shadow_nx_mask;
>
> This looks like it may be a bug. The old behavior sets NX if
> (pte_access & ACC_EXEC_MASK). The new behavior unconditionally sets NX
> and never sets PRESENT. Also, the if (shadow_x_mas k) checks are
> unnecessary. spte |= 0 is a nop.

Thanks for the comment! I realized two judgments of shadow_nx/x_mask is
unnecessary... In fact, the correct behavior is either set shadow_x_mask or
shadow_nx_mask, may be there is a better approach for this. The logic assured
by program itself is always safer. But I will remove the redundant code at
first.

But I don't think it's a bug. The old behavior set NX if (!(pte_access &
ACC_EXEC_MASK)), the same as the new one. And I also curious about the
PRESENT bit. You see, the PRESENT bit was set at the beginning of the code,
and I really don't know why the duplicate one exists there...

>
> > if (pte_access & ACC_USER_MASK)
> > - spte |= PT_USER_MASK;
> > + spte |= shadow_user_mask;
> > if (largepage)
> > spte |= PT_PAGE_SIZE_MASK;

--
Thanks
Yang, Sheng

>
> Regards,
>
> Anthony Liguori