Xen devel - Sep 2005 - RE: [PATCH][VT] Patch to allow VMX domainsto be destroyedor shut down cleanly

Keir Fraser wrote:
> On 19 Sep 2005, at 06:54, Jiang, Yunhong wrote:
> 
>> More exactly should be:
>> 	if (!vmx_paging_enabled(d)) {
>> 			get_page for new mfn
>> 			put_page for old mfn
>> 	}
> 
> What would old_mfn be in this case?
> 
> If we do any cr3 ref-counting on cr0 writes I would expect it to be:
>   * put_page() on clearing of CR0_PG
>   * get_page() on setting of CR0_PG
> 
> Given that writing cr3 currently does a get_page/put_page regardless
> of the setting of CR0_PG, is there any need to be doing get_page()
> within vmx_set_cr0()?
I''m not sure if the get_page() on vmx_set_cr0 has purpose of sanity
check , otherwise it can be removed. In fact, I think the put_page has the
effect to remove it.
> 
> I think it''s more correct to do get/put in vmx_set_cr0 (at that
point
> we can know what format the pagetables should be, for example
> (32/32p/64). But I think it''s a bigger cleanup patch.
Do you mean put all get/put on vmx_set_cr0 ? even for paging enabled case?

And there is one more issue .If the guest is destroyed before the kernel
enabling paging, how about the page pointed by cr3? It may cause one referece
count left. The reason is, currently when destroy domain, only
domain->guest_table is released. However, if the paging is not enabled, then
the get_page on vmx_set_cr0 has no corresponding put_page, which may cause
problem. Hope comments on this point.
> 
> Whatever you go for: please send a complete tested patch. :-)
Sure！
> 
>   -- Keir

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 19 Sep 2005, at 11:52, Jiang, Yunhong wrote:
>> I think it''s more correct to do get/put in vmx_set_cr0 (at
that point
>> we can know what format the pagetables should be, for example
>> (32/32p/64). But I think it''s a bigger cleanup patch.
>
> Do you mean put all get/put on vmx_set_cr0 ? even for paging enabled 
> case?
No, I mean that get/put on cr3 change (or guest destruction) should be 
gated on CR0_PG. Then, additionally, get or put is required when CR0_PG 
is toggled.
> And there is one more issue .If the guest is destroyed before the 
> kernel enabling paging, how about the page pointed by cr3? It may 
> cause one referece count left. The reason is, currently when destroy 
> domain, only domain->guest_table is released. However, if the paging 
> is not enabled, then the get_page on vmx_set_cr0 has no corresponding 
> put_page, which may cause problem. Hope comments on this point.
Doing what I describe above would fix this: CR0_PG would be clear, so 
put_page() would not happen.
>>
>> Whatever you go for: please send a complete tested patch. :-)
>
> Sure！
Thanks!

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser <Keir.Fraser@cl.cam.ac.uk> wrote on 09/19/2005 07:40:50 AM:
>
> On 19 Sep 2005, at 11:52, Jiang, Yunhong wrote:
>
> >> I think it''s more correct to do get/put in vmx_set_cr0
(at that point
> >> we can know what format the pagetables should be, for example
> >> (32/32p/64). But I think it''s a bigger cleanup patch.
> >
> > Do you mean put all get/put on vmx_set_cr0 ? even for paging enabled
> > case?
>
> No, I mean that get/put on cr3 change (or guest destruction) should be
> gated on CR0_PG. Then, additionally, get or put is required when CR0_PG
> is toggled.
>
> > And there is one more issue .If the guest is destroyed before the
> > kernel enabling paging, how about the page pointed by cr3? It may
> > cause one referece count left. The reason is, currently when destroy
> > domain, only domain->guest_table is released. However, if the
paging
> > is not enabled, then the get_page on vmx_set_cr0 has no corresponding
> > put_page, which may cause problem. Hope comments on this point.
>
> Doing what I describe above would fix this: CR0_PG would be clear, so
> put_page() would not happen.
>
> >>
> >> Whatever you go for: please send a complete tested patch. :-)
> >
> > Sure！
>
> Thanks!
>
>   -- Keir
>
If the shadow page reference counts are handled correctly now (I look
forward to receiving a complete patch!), do we really need
shadow_tainted_refcnts flag in the domain structure ?  What''s
the real purpose of this flag ?  Is it used to indicate that
shadow mode was enabled but now disabled ?
Thanks.

Khoa H.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 19 Sep 2005, at 18:26, Khoa Huynh wrote:
> If the shadow page reference counts are handled correctly now (I look
> forward to receiving a complete patch!), do we really need
> shadow_tainted_refcnts flag in the domain structure ?  What''s
> the real purpose of this flag ?  Is it used to indicate that
> shadow mode was enabled but now disabled ?
Although the general ref counts are okay, I think the type counts can 
be wrong. Certainly the only use of tainted_refcnts check is to avoid 
an assertion in common/page_alloc.c (it asserts that if the general 
count is zero, teh type count must also be zero).

Do a debug build of Xen, remove the tainted_refcnts check from 
page_alloc.c, and see if destroying a vmx domain kills Xen. :-)

If not, maybe we can remove that flag.

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel