Christoph Egger
2010-Nov-12 18:45 UTC
[Xen-devel] [PATCH 13/13] Nested Virtualization: hap-on-hap
-- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Nov-16 16:58 UTC
Re: [Xen-devel] [PATCH 13/13] Nested Virtualization: hap-on-hap
Hi, This patch doesn''t address most of my concerns with the last version. My comments about callers of gva_to_gfn still stand -- basically, gva_to_gfn should return an N1 GFN, and most callers (including hvm_copy) should not need to know about N2 GFNs or multiple p2ms. Also, you''re still copying large parts of the existing HAP walker (e.g. the next_level function). Can you avoid the duplication by using a write_p2m_entry() callback, since that seems to be the part that''s different? Skipping a flush when p2m->cr3 == 0 is not safe. I suggested using -1 as the magic value last time. My comments on why p2m_flush_locked() isn''t enough to reclaim an in-use p2m for recycling still stand. I have one more comment on the new code: At 18:45 +0000 on 12 Nov (1289587557), Christoph Egger wrote:> hap_write_p2m_entry(struct vcpu *v, unsigned long gfn, l1_pgentry_t *p, > mfn_t table_mfn, l1_pgentry_t new, unsigned int level) > { > + struct domain *d = v->domain; > uint32_t old_flags; > > - hap_lock(v->domain); > + old_flags = l1e_get_flags(*p); > > - old_flags = l1e_get_flags(*p); > + /* We know always use the host p2m here, regardless if the vcpu > + * is in host or guest mode. The vcpu can be in guest mode by > + * a hypercall which passes a domain and chooses mostly the first > + * vcpu. > + * XXX This is the reason why this function can not be used re-used > + * for updating the nestedp2m. Otherwise, hypercalls would randomly > + * operate on host p2m and nested p2m. > + */ > + if ( nestedhvm_enabled(d) ) { > + mfn_t omfn = _mfn(l1e_get_pfn(*p)); > + p2m_type_t op2mt = p2m_flags_to_type(old_flags); > + > + if ( p2m_is_valid(op2mt) && mfn_valid(omfn) ) {I think you need to do this flush even if !mfn_valid(omfn) - for example if you''re removing a mapping of an MMIO area.> + mfn_t nmfn = _mfn(l1e_get_pfn(new)); > + p2m_type_t np2mt = p2m_flags_to_type(l1e_get_flags(new)); > + > + if ( p2m_is_valid(np2mt) > + && mfn_valid(nmfn) > + && !(l1e_get_flags(new) & _PAGE_PRESENT) )I don''t understand this test at all - you need to flush if you''re removing an old present entry regardless of what replaces it. The only case where you can skip the flush is if old == new. Cheers, Tim.> + { > + /* This GFN -> MFN is going to get removed. */ > + /* XXX There is a more efficient way to do that > + * but it works for now. > + * Note, p2m_flush_nestedp2m calls hap_lock() internally. > + */ > + p2m_flush_nestedp2m(d); > + } > + } > + } > + > + hap_lock(d); > +-- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Christoph Egger
2010-Dec-02 17:49 UTC
Re: [Xen-devel] [PATCH 13/13] Nested Virtualization: hap-on-hap
On Tuesday 16 November 2010 17:58:25 Tim Deegan wrote:> Hi, > > This patch doesn''t address most of my concerns with the last version. > > My comments about callers of gva_to_gfn still stand -- basically, > gva_to_gfn should return an N1 GFN, and most callers (including hvm_copy) > should not need to know about N2 GFNs or multiple p2ms.Done.> > Also, you''re still copying large parts of the existing HAP walker > (e.g. the next_level function). Can you avoid the duplication by > using a write_p2m_entry() callback, since that seems to be the part > that''s different?Yes, I did it with a callback.> > Skipping a flush when p2m->cr3 == 0 is not safe. I suggested using -1 > as the magic value last time.Done.> > My comments on why p2m_flush_locked() isn''t enough to reclaim an in-use > p2m for recycling still stand.Can you point me to the mail in the ML archive you refer to, please?> > I have one more comment on the new code: > > At 18:45 +0000 on 12 Nov (1289587557), Christoph Egger wrote: > > hap_write_p2m_entry(struct vcpu *v, unsigned long gfn, l1_pgentry_t *p, > > mfn_t table_mfn, l1_pgentry_t new, unsigned int > > level) { > > + struct domain *d = v->domain; > > uint32_t old_flags; > > > > - hap_lock(v->domain); > > + old_flags = l1e_get_flags(*p); > > > > - old_flags = l1e_get_flags(*p); > > + /* We know always use the host p2m here, regardless if the vcpu > > + * is in host or guest mode. The vcpu can be in guest mode by > > + * a hypercall which passes a domain and chooses mostly the first > > + * vcpu. > > + * XXX This is the reason why this function can not be used re-used > > + * for updating the nestedp2m. Otherwise, hypercalls would randomly > > + * operate on host p2m and nested p2m. > > + */ > > + if ( nestedhvm_enabled(d) ) { > > + mfn_t omfn = _mfn(l1e_get_pfn(*p)); > > + p2m_type_t op2mt = p2m_flags_to_type(old_flags); > > + > > + if ( p2m_is_valid(op2mt) && mfn_valid(omfn) ) { > > I think you need to do this flush even if !mfn_valid(omfn) - for example > if you''re removing a mapping of an MMIO area.Fixed.> > > + mfn_t nmfn = _mfn(l1e_get_pfn(new)); > > + p2m_type_t np2mt = p2m_flags_to_type(l1e_get_flags(new)); > > + > > + if ( p2m_is_valid(np2mt) > > + && mfn_valid(nmfn) > > + && !(l1e_get_flags(new) & _PAGE_PRESENT) ) > > I don''t understand this test at all - you need to flush if you''re > removing an old present entry regardless of what replaces it. The only > case where you can skip the flush is if old == new.Fixed.> Cheers, > > Tim. > > > + { > > + /* This GFN -> MFN is going to get removed. */ > > + /* XXX There is a more efficient way to do that > > + * but it works for now. > > + * Note, p2m_flush_nestedp2m calls hap_lock() > > internally. + */ > > + p2m_flush_nestedp2m(d); > > + } > > + } > > + } > > + > > + hap_lock(d); > > +-- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Dec-08 10:17 UTC
Re: [Xen-devel] [PATCH 13/13] Nested Virtualization: hap-on-hap
At 17:49 +0000 on 02 Dec (1291312156), Christoph Egger wrote:> > My comments on why p2m_flush_locked() isn''t enough to reclaim an in-use > > p2m for recycling still stand. > > Can you point me to the mail in the ML archive you refer to, please?It''s the discussion at the end of this email: http://lists.xensource.com/archives/html/xen-devel/2010-09/msg00624.html Cheers, Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Christoph Egger
2010-Dec-08 10:32 UTC
Re: [Xen-devel] [PATCH 13/13] Nested Virtualization: hap-on-hap
On Wednesday 08 December 2010 11:17:15 Tim Deegan wrote:> At 17:49 +0000 on 02 Dec (1291312156), Christoph Egger wrote: > > > My comments on why p2m_flush_locked() isn''t enough to reclaim an in-use > > > p2m for recycling still stand. > > > > Can you point me to the mail in the ML archive you refer to, please? > > It''s the discussion at the end of this email: > http://lists.xensource.com/archives/html/xen-devel/2010-09/msg00624.htmlTnx. I see it is related to your suggestion to check cr3 against -1 you already mentioned. This is already fixed in my local tree. Christoph -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Dec-08 10:55 UTC
Re: [Xen-devel] [PATCH 13/13] Nested Virtualization: hap-on-hap
At 10:32 +0000 on 08 Dec (1291804321), Christoph Egger wrote:> On Wednesday 08 December 2010 11:17:15 Tim Deegan wrote: > > At 17:49 +0000 on 02 Dec (1291312156), Christoph Egger wrote: > > > > My comments on why p2m_flush_locked() isn''t enough to reclaim an in-use > > > > p2m for recycling still stand. > > > > > > Can you point me to the mail in the ML archive you refer to, please? > > > > It''s the discussion at the end of this email: > > http://lists.xensource.com/archives/html/xen-devel/2010-09/msg00624.html > > Tnx. I see it is related to your suggestion to check cr3 against -1 you > already mentioned.It''s similar, but different. In particular, checking CR3 against -1 may not fix it. It''s possible that I''m just missing the path on vmentry that checks the p2m hasn''t been reassigned. Quoting my two concerns from before:> > Is this enough? If this p2m might be in host_vmcb->h_cr3 somewhere on > > another vcpu, then you need to make sure that vcpu gets reset not to use > > it any more. > > There are three possibilities: > An other vcpu is in VMRUN emulation before a nestedp2m is assigned. > In this case, it will get a new nestedp2m as it won''t find its ''old'' > nestedp2m anymore. > > An other vcpu is in VMRUN emulation after a nestedp2m is assigned. > It will VMEXIT with a nested page fault.Why?> An other vcpu already running l2 guest. > It will VMEXIT with a nested page fault immediately.Hmm. It will exit for the TLB shootdown IPI, but I think you need to clear vcpu_nestedhvm(v).nh_p2m on the other vcpu to make sure it doesn''t re-enter with the p2m you''ve just recycled. Cheers, Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Christoph Egger
2010-Dec-20 16:27 UTC
Re: [Xen-devel] [PATCH 13/13] Nested Virtualization: hap-on-hap
On Wednesday 08 December 2010 11:55:08 Tim Deegan wrote:> At 10:32 +0000 on 08 Dec (1291804321), Christoph Egger wrote: > > On Wednesday 08 December 2010 11:17:15 Tim Deegan wrote: > > > At 17:49 +0000 on 02 Dec (1291312156), Christoph Egger wrote: > > > > > My comments on why p2m_flush_locked() isn''t enough to reclaim an > > > > > in-use p2m for recycling still stand. > > > > > > > > Can you point me to the mail in the ML archive you refer to, please? > > > > > > It''s the discussion at the end of this email: > > > http://lists.xensource.com/archives/html/xen-devel/2010-09/msg00624.htm > > >l > > > > Tnx. I see it is related to your suggestion to check cr3 against -1 you > > already mentioned. > > It''s similar, but different. In particular, checking CR3 against -1 > may not fix it. It''s possible that I''m just missing the path on vmentry > that checks the p2m hasn''t been reassigned.My comments are based on the seventh patch series I just sent out. The assumption is that the p2m is *empty*. So in case it is reassigned the (v)cpu will fall out with a nested page fault since the MMU can''t do a page table walk.> > Quoting my two concerns from before: > > > Is this enough? If this p2m might be in host_vmcb->h_cr3 somewhere on > > > another vcpu, then you need to make sure that vcpu gets reset not to > > > use it any more. > > > > There are three possibilities: > > An other vcpu is in VMRUN emulation before a nestedp2m is assigned. > > In this case, it will get a new nestedp2m as it won''t find its ''old'' > > nestedp2m anymore. > > > > An other vcpu is in VMRUN emulation after a nestedp2m is assigned. > > It will VMEXIT with a nested page fault. > > Why?Because the p2m is empty. The MMU can not do a page table walk.> > An other vcpu already running l2 guest. > > It will VMEXIT with a nested page fault immediately. > > Hmm. It will exit for the TLB shootdown IPI, but I think you need to > clear vcpu_nestedhvm(v).nh_p2m on the other vcpu to make sure it doesn''t > re-enter with the p2m you''ve just recycled.The p2m is empty so I don''t see a problem when it gets recycled. Christoph -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Dec-20 16:35 UTC
Re: [Xen-devel] [PATCH 13/13] Nested Virtualization: hap-on-hap
At 16:27 +0000 on 20 Dec (1292862443), Christoph Egger wrote:> > > An other vcpu is in VMRUN emulation after a nestedp2m is assigned. > > > It will VMEXIT with a nested page fault. > > > > Why? > > Because the p2m is empty. The MMU can not do a page table walk. > > > > An other vcpu already running l2 guest. > > > It will VMEXIT with a nested page fault immediately. > > > > Hmm. It will exit for the TLB shootdown IPI, but I think you need to > > clear vcpu_nestedhvm(v).nh_p2m on the other vcpu to make sure it doesn''t > > re-enter with the p2m you''ve just recycled. > > The p2m is empty so I don''t see a problem when it gets recycled.It''s only empty very briefly. You''ve assigned it to a vcpu which is about to take a nested fault and fill it with entries, right? What happens if the other vcpu is handling an SMI or executing a tight loop of register arithmetic for a few thousand cycles? What stops it seeing the new contents of the p2m? Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel