Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1268861

Re: [PATCH 09/10 RFC] KVM: x86: MMU: Move parent_pte handling from kvm_mmu_get_page() to link_shadow_page()

Show all headers | View raw

On 13/11/2015 03:15, Takuya Yoshikawa wrote:
> Actually, I don't understand why this is named kvm_mmu_put_page() for
> just removing parent_pte pointer from the sp->parent_ptes pointer chain.

Because it undoes kvm_mmu_get_page, I guess. :)

> 
>> On to kvm_mmu_get_page...
>>
>>          if (!direct) {
>>                  if (rmap_write_protect(vcpu, gfn))
>>                          kvm_flush_remote_tlbs(vcpu->kvm);
>>                  if (level > PT_PAGE_TABLE_LEVEL && need_sync)
>>                          kvm_sync_pages(vcpu, gfn);
>>
>> This seems fishy.
>>
>> need_sync is set if sp->unsync, but then the parents have not been
>> unsynced yet.
> 
> Reaching here means that kvm_mmu_get_page() could not return sp
> from inside the for_each_gfn_sp() loop above, so even without
> this patch, mark_unsync() has not been called.

You're right.

> Here, sp holds the new page allocated by kvm_mmu_alloc_page().
> One confusing thing is that hlist_add_head() right before this
> "if (!direct)" line has already added the new sp to the hash
> list, so it will be found by for_each_gfn_indirect_valid_sp()
> in kvm_sync_pages().
> 
> Because this sp is new and sp->unsync is not set,  kvm_sync_pages()
> will just skip it and look for other sp's whose ->unsync were found
> to be set in the for_each_gfn_sp() loop.
> 
> I'm not 100% sure if the existence of the parent_pte pointer in the
> newly created sp->parent_ptes chain alone makes any difference:

No, I don't think so.  Nothing needs the parent_ptes at this point:

- kvm_mmu_mark_parents_unsync, even in the existing code, it's called
before the new SPTE is created.

- as you said, kvm_mmu_prepare_zap_page can be called by kvm_sync_pages
but it will not operate on this page because its ->unsync is zero.

> So, "bool accessed" needs to be passed to kvm_mmu_get_page(). 

The "bool accessed" parameter is not necessary, I think.  It is only
false in the nested EPT case, and there's no reason not to set the
accessed bit *in the shadow page* if the host supports EPT
accessed/dirty bits.  I'll test and send a patch to remove the argument.

> But any way, we need to understand if mmu_page_add_parent_pte()
> really needs to be placed before the "if (!direct)" block.

No, I don't think so anymore.

I think these patches are fine as a starting point for further cleanups,
I'll push them to kvm/queue very soon.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 00/10 V2] KVM: x86: MMU: Clean up x86's mmu code for future  work Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> - 2015-11-12 12:40 +0100
  [PATCH 04/10] KVM: x86: MMU: Remove is_rmap_spte() and use  is_shadow_present_pte() Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> - 2015-11-12 12:40 +0100
  [PATCH 01/10] KVM: x86: MMU: Remove unused parameter of  __direct_map() Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> - 2015-11-12 12:40 +0100
  [PATCH 02/10] KVM: x86: MMU: Add helper function to clear a bit in  unsync child bitmap Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> - 2015-11-12 12:40 +0100
  [PATCH 08/10] KVM: x86: MMU: Move initialization of parent_ptes out  from kvm_mmu_alloc_page() Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> - 2015-11-12 12:50 +0100
  [PATCH 09/10 RFC] KVM: x86: MMU: Move parent_pte handling from  kvm_mmu_get_page() to link_shadow_page() Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> - 2015-11-12 12:50 +0100
    Re: [PATCH 09/10 RFC] KVM: x86: MMU: Move parent_pte handling from  kvm_mmu_get_page() to link_shadow_page() Paolo Bonzini <pbonzini@redhat.com> - 2015-11-12 15:30 +0100
      Re: [PATCH 09/10 RFC] KVM: x86: MMU: Move parent_pte handling from  kvm_mmu_get_page() to link_shadow_page() Paolo Bonzini <pbonzini@redhat.com> - 2015-11-12 18:10 +0100
      Re: [PATCH 09/10 RFC] KVM: x86: MMU: Move parent_pte handling from  kvm_mmu_get_page() to link_shadow_page() Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> - 2015-11-13 03:20 +0100
        Re: [PATCH 09/10 RFC] KVM: x86: MMU: Move parent_pte handling from  kvm_mmu_get_page() to link_shadow_page() Paolo Bonzini <pbonzini@redhat.com> - 2015-11-13 12:00 +0100
  [PATCH 06/10] KVM: x86: MMU: Consolidate WARN_ON/BUG_ON checks for  reverse-mapped sptes Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> - 2015-11-12 12:50 +0100
    Re: [PATCH 06/10] KVM: x86: MMU: Consolidate WARN_ON/BUG_ON checks  for reverse-mapped sptes Marcelo Tosatti <mtosatti@redhat.com> - 2015-11-13 23:10 +0100
      Re: [PATCH 06/10] KVM: x86: MMU: Consolidate WARN_ON/BUG_ON checks  for reverse-mapped sptes Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> - 2015-11-16 04:40 +0100
  [PATCH 10/10] KVM: x86: MMU: Remove unused parameter parent_pte  from kvm_mmu_get_page() Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> - 2015-11-12 12:50 +0100
  [PATCH 07/10] KVM: x86: MMU: Encapsulate the type of rmap-chain  head in a new struct Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> - 2015-11-12 12:50 +0100
  Re: [PATCH 00/10 V2] KVM: x86: MMU: Clean up x86's mmu code for  future work Paolo Bonzini <pbonzini@redhat.com> - 2015-11-12 13:10 +0100

csiph-web