Yang, Sheng wrote:
> 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.
The new behavior sets NX regardless of whether (pte_access &
ACC_EXEC_MASK). Is the desired change to unconditionally set NX?
> 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...
>
Looking at the code, you appear to be right. In the future, I think you
should separate any cleanups (like removing the redundant setting of
PRESENT) into a separate patch and stick to just programmatic changes of
PT_USER_MASK => shadow_user_mask, etc. in this patch. That makes it a
lot easier to review correctness.
Regards,
Anthony Liguori
>>> if (pte_access & ACC_USER_MASK)
>>> - spte |= PT_USER_MASK;
>>> + spte |= shadow_user_mask;
>>> if (largepage)
>>> spte |= PT_PAGE_SIZE_MASK;
>>>
>
>
-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference
Don't miss this year's exciting event. There's still time to save $100.
Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
