I remembered that some portion of shadow table is per-domain property, which is why a domain shadow lock is used. Then at shadow_blow_tables, should target domain be paused before blowing all tables in case some entries are still in use in guest context on other cpus for a SMP guest. Or some delay unhook mechanism is used to check above condition? Thanks, Kevin _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi, At 10:49 +0800 on 27 Nov (1196160559), Tian, Kevin wrote:> I remembered that some portion of shadow table is per-domain property, > which is why a domain shadow lock is used.I''m not sure what you mean. The shadow lock is used to stop concurrent updates to a domain''s shadow pagetable state from different CPUs.> Then at shadow_blow_tables, should target domain be paused before > blowing all tables in case some entries are still in use in guest > context on other cpus for a SMP guest. Or some delay unhook mechanism > is used to check above condition?No, we just unhook them. Updates to shadow pagetables are safe against concurrent *reads* because we''re careful about the ordering of writes on PAE entries and writes are atomic on non-pae and 64-bit. Of course, other CPUs might have the old contents of the shadow pagetables in their TLBs. This is safe because we don''t overwrite old shadow pagetables until TLBs have been flushed (see shadow_alloc()) and we flush all the TLBs at the bottom of shadow_blow_tables(). Cheers, Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Citrix Systems. [Company #5334508: XenSource UK Ltd, reg''d c/o EC2Y 5EB, UK.] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>From: Tim Deegan [mailto:Tim.Deegan@citrix.com] >Sent: 2007年11月27日 17:58 > >Hi, > >At 10:49 +0800 on 27 Nov (1196160559), Tian, Kevin wrote: >> I remembered that some portion of shadow table is per-domain >property, >> which is why a domain shadow lock is used. > >I''m not sure what you mean. The shadow lock is used to stop concurrent >updates to a domain''s shadow pagetable state from different CPUs.I meant same to you, that some shadow entry is domain-wise and shared on multiple vcpus.> >> Then at shadow_blow_tables, should target domain be paused before >> blowing all tables in case some entries are still in use in guest >> context on other cpus for a SMP guest. Or some delay unhook mechanism >> is used to check above condition? > >No, we just unhook them. Updates to shadow pagetables are safe against >concurrent *reads* because we''re careful about the ordering of writes >on PAE entries and writes are atomic on non-pae and 64-bit.Maybe I made some misunderstanding here. By comment of shadow_blow_ tables: /* Deliberately free all the memory we can: this will tear down all of * this domain''s shadows */ The implicit here is that all shadow pages of this domain will be released as result. However when ''blow'' is on-going on one cpu, the ''blow-ed'' pages may be active on address translation on another cpu, if other vcpus are not paused. I think anyway hardware should be prevented from walking shadow pages which are torn down from another cpu... So my question is, whether all shadow pages are indeed free-ed as result of ''blow'' option? Or some IPI will be definitely triggered when free-ing one shadow page referenced by multiple VCPUs, before final TLB flush? Thanks, Kevin> >Of course, other CPUs might have the old contents of the shadow >pagetables in their TLBs. This is safe because we don''t overwrite old >shadow pagetables until TLBs have been flushed (see shadow_alloc()) and >we flush all the TLBs at the bottom of shadow_blow_tables(). > >Cheers, > >Tim. > >-- >Tim Deegan <Tim.Deegan@citrix.com> >Principal Software Engineer, Citrix Systems. >[Company #5334508: XenSource UK Ltd, reg''d c/o EC2Y 5EB, UK.] >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi, At 18:32 +0800 on 27 Nov (1196188342), Tian, Kevin wrote:> Maybe I made some misunderstanding here. By comment of shadow_blow_ > tables: > /* Deliberately free all the memory we can: this will tear down all of > * this domain''s shadows */In this comment, "free" means only freeing as far as the domain''s shadow free lists, not to domheap. Does that make more sense?> The implicit here is that all shadow pages of this domain will be released > as result. However when ''blow'' is on-going on one cpu, the ''blow-ed'' pages > may be active on address translation on another cpu, if other vcpus are > not paused. I think anyway hardware should be prevented from walking > shadow pages which are torn down from another cpu...As I said, it''s safe to do this concurrently with other CPUs reading the shadow pagetables, and we have the shadow lock to protect against concurrent writes. - other CPUs never see a half-written entry because of the logic in safe_write_entry(). - l1es in other CPUs'' TLBs are safe to leave until the final TLB flush because there''s no intermediate stage mid-operation that requires rights to have been relinquished. - higher-level entries in other CPUs'' TLBs are safe because we leave the contents of the shadow pagetables they point at alone until we''re sure all the TLBs are flushed. (We never write to pages on the shadow free list and we check the TLB flush timestamps when we allocate them from the free list again.) Can you be clearer about what you think the risk is? If we''ve missed something then it''s quite important, because it probably affects every other shadow operation as well.> So my question is, whether all shadow pages are indeed free-ed as result > of ''blow'' option?Only as far as the free list. We never free shadow pages back to domheap until the allocation is changed or shadow mode is disabled. But I think it would still be safe even if we freed to domheap because the deferred-TLB-flush logic in page_alloc.c would do the right thing.> Or some IPI will be definitely triggered when free-ing one > shadow page referenced by multiple VCPUs, before final TLB flush?No. We do no synchronisation until the TLB flush at the end. Cheers, Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Citrix Systems. [Company #5334508: XenSource UK Ltd, reg''d c/o EC2Y 5EB, UK.] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Thanks a lot for your clarification. Now I''m clear that current solution is safe. Actually in the start I also doubt my suspicion, since such hole is severe and unlikely nobody reports. So just post for your help on this part. :-) Thanks, Kevin>From: Tim Deegan [mailto:Tim.Deegan@citrix.com] >Sent: 2007年11月27日 18:51 > >Hi, > >At 18:32 +0800 on 27 Nov (1196188342), Tian, Kevin wrote: >> Maybe I made some misunderstanding here. By comment of shadow_blow_ >> tables: >> /* Deliberately free all the memory we can: this will tear >down all of >> * this domain''s shadows */ > >In this comment, "free" means only freeing as far as the >domain''s shadow >free lists, not to domheap. Does that make more sense? > >> The implicit here is that all shadow pages of this domain >will be released >> as result. However when ''blow'' is on-going on one cpu, the >''blow-ed'' pages >> may be active on address translation on another cpu, if >other vcpus are >> not paused. I think anyway hardware should be prevented from walking >> shadow pages which are torn down from another cpu... > >As I said, it''s safe to do this concurrently with other CPUs >reading the >shadow pagetables, and we have the shadow lock to protect against >concurrent writes. > >- other CPUs never see a half-written entry because of the logic in > safe_write_entry(). >- l1es in other CPUs'' TLBs are safe to leave until the final TLB flush > because there''s no intermediate stage mid-operation that >requires rights > to have been relinquished. >- higher-level entries in other CPUs'' TLBs are safe because we >leave the > contents of the shadow pagetables they point at alone until >we''re sure > all the TLBs are flushed. (We never write to pages on the >shadow free > list and we check the TLB flush timestamps when we allocate them > from the free list again.) > >Can you be clearer about what you think the risk is? If we''ve missed >something then it''s quite important, because it probably affects every >other shadow operation as well. > >> So my question is, whether all shadow pages are indeed >free-ed as result >> of ''blow'' option? > >Only as far as the free list. We never free shadow pages back to >domheap until the allocation is changed or shadow mode is >disabled. But >I think it would still be safe even if we freed to domheap because the >deferred-TLB-flush logic in page_alloc.c would do the right thing. > >> Or some IPI will be definitely triggered when free-ing one >> shadow page referenced by multiple VCPUs, before final TLB flush? > >No. We do no synchronisation until the TLB flush at the end. > >Cheers, > >Tim. > >-- >Tim Deegan <Tim.Deegan@citrix.com> >Principal Software Engineer, Citrix Systems. >[Company #5334508: XenSource UK Ltd, reg''d c/o EC2Y 5EB, UK.] >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel