[PATCH]Add a flag for shadow pages

[PATCH]Add a flag for shadow pages

[Jiang, Yunhong]

[-- Attachment #1: Type: text/plain, Size: 744 bytes --]

Currently we don't know that a page is a shadow page unless we are in shadow handler. This cause error when we try to get the page owner for the shadow page, this snippet add a flag to it.

signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>

I'm not quite sure if the sh_put_ref() and sh_rm_write_access_from_sl1p() is try to checking a page is shadow page (I assume so), because when a anonymous page is allocated, the count_info is also 0 (like HVM's vlapic page), so I change it like this patch (checking PGC_count_mask is 0). Since comments in sh_hash_audit_bucket() has stated clearly it is to check if it is shdow, so I replace it with test_bit().

Also, do we need checking in page_get_owner() also?

Thanks
Yunhong Jiang

[-- Attachment #2: new_shadow_flag.patch --]
[-- Type: application/octet-stream, Size: 3570 bytes --]

Add a special flag for shadow page

diff -r 02ea044972b7 xen/arch/x86/mm/shadow/common.c
--- a/xen/arch/x86/mm/shadow/common.c	Wed Mar 04 02:12:22 2009 +0800
+++ b/xen/arch/x86/mm/shadow/common.c	Wed Mar 04 02:53:46 2009 +0800
@@ -1820,6 +1820,7 @@ static unsigned int sh_set_allocation(st
                 sp[j].u.sh.pinned = 0;
                 sp[j].u.sh.count = 0;
                 sp[j].tlbflush_timestamp = 0; /* Not in any TLB */
+                sp[j].count_info |= PGC_shadow;
             }
             sp->v.free.order = order;
             page_list_add_tail(sp, &d->arch.paging.shadow.freelists[order]);
@@ -1835,7 +1836,10 @@ static unsigned int sh_set_allocation(st
              * gets overwritten normally, so need to clear it here.
              */
             for ( j = 0; j < 1U << order; j++ )
+            {
                 page_set_owner(&((struct page_info *)sp)[j], NULL);
+                sp[j].count_info &= ~PGC_shadow;
+            }
             d->arch.paging.shadow.free_pages -= 1 << order;
             d->arch.paging.shadow.total_pages -= 1 << order;
             free_domheap_pages((struct page_info *)sp, order);
@@ -1895,7 +1899,7 @@ static void sh_hash_audit_bucket(struct 
     while ( sp )
     {
         /* Not a shadow? */
-        BUG_ON( sp->count_info != 0 );
+        BUG_ON( !test_bit(_PGC_shadow, &sp->count_info) );
         /* Bogus type? */
         BUG_ON( sp->u.sh.type == 0 );
         BUG_ON( sp->u.sh.type > SH_type_max_shadow );
diff -r 02ea044972b7 xen/arch/x86/mm/shadow/multi.c
--- a/xen/arch/x86/mm/shadow/multi.c	Wed Mar 04 02:12:22 2009 +0800
+++ b/xen/arch/x86/mm/shadow/multi.c	Wed Mar 04 02:12:24 2009 +0800
@@ -4281,7 +4281,7 @@ int sh_rm_write_access_from_sl1p(struct 
 
     sp = mfn_to_page(smfn);
 
-    if ( sp->count_info != 0
+    if ( ((sp->count_info & (PGC_page_table | PGC_count_mask)) != 0)
          || (sp->u.sh.type != SH_type_l1_shadow
              && sp->u.sh.type != SH_type_fl1_shadow) )
         goto fail;
diff -r 02ea044972b7 xen/arch/x86/mm/shadow/private.h
--- a/xen/arch/x86/mm/shadow/private.h	Wed Mar 04 02:12:22 2009 +0800
+++ b/xen/arch/x86/mm/shadow/private.h	Wed Mar 04 02:12:24 2009 +0800
@@ -647,7 +647,7 @@ static inline void sh_put_ref(struct vcp
     struct page_info *sp = mfn_to_page(smfn);
 
     ASSERT(mfn_valid(smfn));
-    ASSERT(sp->count_info == 0);
+    ASSERT( (sp->count_info & (PGC_page_table | PGC_count_mask) ) == 0);
 
     /* If this is the entry in the up-pointer, remove it */
     if ( entry_pa != 0 
diff -r 02ea044972b7 xen/include/asm-x86/mm.h
--- a/xen/include/asm-x86/mm.h	Wed Mar 04 02:12:22 2009 +0800
+++ b/xen/include/asm-x86/mm.h	Wed Mar 04 02:48:09 2009 +0800
@@ -59,7 +59,7 @@ struct page_info
             unsigned long type_info;
         } inuse;
 
-        /* Page is in use as a shadow: count_info == 0. */
+        /* Page is in use as a shadow: count_info should be PGC_shadow. */
         struct {
             unsigned long type:5;   /* What kind of shadow is this? */
             unsigned long pinned:1; /* Is the shadow pinned? */
@@ -198,8 +198,12 @@ struct page_info
  /* 3-bit PAT/PCD/PWT cache-attribute hint. */
 #define PGC_cacheattr_base PG_shift(6)
 #define PGC_cacheattr_mask PG_mask(7, 6)
+/* 1-bit flag for shadow page */
+#define _PGC_shadow        PG_shift(7)
+#define PGC_shadow         PG_mask(1, 7)
+
  /* Count of references to this frame. */
-#define PGC_count_width   PG_shift(6)
+#define PGC_count_width   PG_shift(7)
 #define PGC_count_mask    ((1UL<<PGC_count_width)-1)
 
 #if defined(__i386__)

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH]Add a flag for shadow pages

[Keir Fraser]

On 04/03/2009 08:46, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:

> Currently we don't know that a page is a shadow page unless we are in shadow
> handler. This cause error when we try to get the page owner for the shadow
> page, this snippet add a flag to it.
> 
> signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>

You only actually use your new flag in a BUG_ON, where it might avoid false
negatives but hardly looks essential. Your other changes limit count_info
checks to PGC_page_table|PGC_count_info which might be an improvement (I'm
sure Tim or Gianluca can say) but they're existing fields.

> I'm not quite sure if the sh_put_ref() and sh_rm_write_access_from_sl1p() is
> try to checking a page is shadow page (I assume so), because when a anonymous
> page is allocated, the count_info is also 0 (like HVM's vlapic page), so I
> change it like this patch (checking PGC_count_mask is 0). Since comments in
> sh_hash_audit_bucket() has stated clearly it is to check if it is shdow, so I
> replace it with test_bit().
>
> Also, do we need checking in page_get_owner() also?

Usually gets used where we already get_page()ed or are about to get_page().
Most Xen code knows that page uses can change under its feet unless it grabs
a reference. So extra checks in page_get_owner() are probably rather
pointless.

 -- Keir

RE: [PATCH]Add a flag for shadow pages

[Jiang, Yunhong]

This is mainly for page offline. When we offline a page, we we need turn to the caller the page's owner but we can't distinguish easily if it is a assigned page or a shadow page.

Thanks
Yunhong Jiang

Keir Fraser <mailto:keir.fraser@eu.citrix.com> wrote:
> On 04/03/2009 08:46, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:
> 
>> Currently we don't know that a page is a shadow page unless we are in
>> shadow handler. This cause error when we try to get the page owner for the
>> shadow page, this snippet add a flag to it.
>> 
>> signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
> 
> You only actually use your new flag in a BUG_ON, where it
> might avoid false
> negatives but hardly looks essential. Your other changes limit count_info
> checks to PGC_page_table|PGC_count_info which might be an
> improvement (I'm
> sure Tim or Gianluca can say) but they're existing fields.
> 
>> I'm not quite sure if the sh_put_ref() and
> sh_rm_write_access_from_sl1p() is
>> try to checking a page is shadow page (I assume so), because when a
>> anonymous page is allocated, the count_info is also 0 (like HVM's vlapic
>> page), so I change it like this patch (checking PGC_count_mask is 0).
>> Since comments in sh_hash_audit_bucket() has stated clearly it is to check
>> if it is shdow, so I replace it with test_bit(). 
>> 
>> Also, do we need checking in page_get_owner() also?
> 
> Usually gets used where we already get_page()ed or are about
> to get_page().
> Most Xen code knows that page uses can change under its feet
> unless it grabs
> a reference. So extra checks in page_get_owner() are probably rather
> pointless. 
> 
> -- Keir

Re: [PATCH]Add a flag for shadow pages

[Keir Fraser]

On 04/03/2009 08:46, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:

> Currently we don't know that a page is a shadow page unless we are in shadow
> handler. This cause error when we try to get the page owner for the shadow
> page, this snippet add a flag to it.

Ah... Is this for your new page-offlining magic? You don't need it. If you
can't get_page(get_page_owner()) then that tells you the page has no owner,
or the owner changed under your feet. Pretty simple.

 -- Keir

Re: [PATCH]Add a flag for shadow pages

[Keir Fraser]

I just realised that. You use get_page() to lock down a page's owner.
Otherwise it can change under your feet anyway. You don't need a new flag.

 -- Keir

On 04/03/2009 09:17, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:

> This is mainly for page offline. When we offline a page, we we need turn to
> the caller the page's owner but we can't distinguish easily if it is a
> assigned page or a shadow page.
> 
> Thanks
> Yunhong Jiang
> 
> Keir Fraser <mailto:keir.fraser@eu.citrix.com> wrote:
>> On 04/03/2009 08:46, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:
>> 
>>> Currently we don't know that a page is a shadow page unless we are in
>>> shadow handler. This cause error when we try to get the page owner for the
>>> shadow page, this snippet add a flag to it.
>>> 
>>> signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
>> 
>> You only actually use your new flag in a BUG_ON, where it
>> might avoid false
>> negatives but hardly looks essential. Your other changes limit count_info
>> checks to PGC_page_table|PGC_count_info which might be an
>> improvement (I'm
>> sure Tim or Gianluca can say) but they're existing fields.
>> 
>>> I'm not quite sure if the sh_put_ref() and
>> sh_rm_write_access_from_sl1p() is
>>> try to checking a page is shadow page (I assume so), because when a
>>> anonymous page is allocated, the count_info is also 0 (like HVM's vlapic
>>> page), so I change it like this patch (checking PGC_count_mask is 0).
>>> Since comments in sh_hash_audit_bucket() has stated clearly it is to check
>>> if it is shdow, so I replace it with test_bit().
>>> 
>>> Also, do we need checking in page_get_owner() also?
>> 
>> Usually gets used where we already get_page()ed or are about
>> to get_page().
>> Most Xen code knows that page uses can change under its feet
>> unless it grabs
>> a reference. So extra checks in page_get_owner() are probably rather
>> pointless. 
>> 
>> -- Keir

RE: [PATCH]Add a flag for shadow pages

[Jiang, Yunhong]

Keir Fraser <mailto:keir.fraser@eu.citrix.com> wrote:
> I just realised that. You use get_page() to lock down a page's owner.
> Otherwise it can change under your feet anyway. You don't need

With get_page_owner() in get_page() will cause fault if it is a shadow page. Or you mean use exception table to protect it?

The page owner will not be changed anymore, since when a page freed, it will not be allocated anymore if marked page offlining. The worst situation is the page is assigned, but the owner firled is still not set (that action usually not protected by lock), then we return it is anonymous, that should be harmless as stated in the patch, since page_offline can't handle anonymous page.

Thanks
Yunhong Jiang

> a new flag.
> 
> -- Keir
> 
> On 04/03/2009 09:17, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:
> 
>> This is mainly for page offline. When we offline a page, we we need turn to
>> the caller the page's owner but we can't distinguish easily if it is a
>> assigned page or a shadow page.
>> 
>> Thanks
>> Yunhong Jiang
>> 
>> Keir Fraser <mailto:keir.fraser@eu.citrix.com> wrote:
>>> On 04/03/2009 08:46, "Jiang, Yunhong"
> <yunhong.jiang@intel.com> wrote:
>>> 
>>>> Currently we don't know that a page is a shadow page unless we are in
>>>> shadow handler. This cause error when we try to get the page owner for
>>>> the shadow page, this snippet add a flag to it.
>>>> 
>>>> signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
>>> 
>>> You only actually use your new flag in a BUG_ON, where it
>>> might avoid false
>>> negatives but hardly looks essential. Your other changes limit count_info
>>> checks to PGC_page_table|PGC_count_info which might be an improvement (I'm
>>> sure Tim or Gianluca can say) but they're existing fields.
>>> 
>>>> I'm not quite sure if the sh_put_ref() and
>>> sh_rm_write_access_from_sl1p() is
>>>> try to checking a page is shadow page (I assume so), because when a
>>>> anonymous page is allocated, the count_info is also 0 (like HVM's vlapic
>>>> page), so I change it like this patch (checking PGC_count_mask is 0).
>>>> Since comments in sh_hash_audit_bucket() has stated clearly it is to
>>>> check if it is shdow, so I replace it with test_bit().
>>>> 
>>>> Also, do we need checking in page_get_owner() also?
>>> 
>>> Usually gets used where we already get_page()ed or are about
>>> to get_page().
>>> Most Xen code knows that page uses can change under its feet
>>> unless it grabs
>>> a reference. So extra checks in page_get_owner() are probably rather
>>> pointless. 
>>> 
>>> -- Keir

Re: [PATCH]Add a flag for shadow pages

[Keir Fraser]

On 04/03/2009 09:28, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:

> Keir Fraser <mailto:keir.fraser@eu.citrix.com> wrote:
>> I just realised that. You use get_page() to lock down a page's owner.
>> Otherwise it can change under your feet anyway. You don't need
> 
> With get_page_owner() in get_page() will cause fault if it is a shadow page.
> Or you mean use exception table to protect it?

There are a few solutions. One would be to remove the debug printk from
get_page() since it is the only thing which dereferences the bogus 'domain
pointer'.

Another would be to create a new function page_get_reference_and_owner()
which obtains a reference on a guest page and *returns* the (now known
valid) domain pointer. Probably this is nicer actually. Then all existing
users of page_get_owner() need checking to ensure they don't need to use the
new more expensive function -- I think some are probably actually unsafe now
that shadow pages clobber the domain field.

 -- Keir

Re: Re: [PATCH]Add a flag for shadow pages

[Keir Fraser]

On 04/03/2009 09:56, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

> Another would be to create a new function page_get_reference_and_owner()
> which obtains a reference on a guest page and *returns* the (now known
> valid) domain pointer. Probably this is nicer actually. Then all existing
> users of page_get_owner() need checking to ensure they don't need to use the
> new more expensive function -- I think some are probably actually unsafe now
> that shadow pages clobber the domain field.

I'm working on this by the way. I'll clean up everything except shadow uses
of page_get_owner(). The only two possibly suspect uses I can see (most are
just ASSERT/BUG_ON uses I think are okay):
 * sh_mfn_is_a_pagetable()
 * shadow_get_page_from_l1e()

It'd be good if Tim or Gianluca would check whether these need to be more
careful -- could page_get_owner() return a duff non-NULL value in either of
these functions? This could only happen if the pages they work on could
possibly actually be shadow pages with clobbered page owner field.

 -- Keir

Re: Re: [PATCH]Add a flag for shadow pages

[Tim Deegan]

At 11:57 +0000 on 04 Mar (1236167835), Keir Fraser wrote:
> I'm working on this by the way. I'll clean up everything except shadow uses
> of page_get_owner(). The only two possibly suspect uses I can see (most are
> just ASSERT/BUG_ON uses I think are okay):
>  * sh_mfn_is_a_pagetable()
>  * shadow_get_page_from_l1e()
> 
> It'd be good if Tim or Gianluca would check whether these need to be more
> careful -- could page_get_owner() return a duff non-NULL value in either of
> these functions? This could only happen if the pages they work on could
> possibly actually be shadow pages with clobbered page owner field.

shadow_get_page_from_l1e() should never be handling a pointer to a
shadow -- if it does that then we've let the guest see the shadows and
all invariants go out the window. 

sh_mfn_is_a_pagetable() looks OK too; it only gets called based on the
contents of shadow PTEs or the MFNs that guests are writing to, both of
which should be safe.

It all feels a bit fragile to me though, compared to the old layout
where we always knew the owner field would be NULL.

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Citrix Systems (R&D) Ltd.
[Company #02300071, SL9 0DZ, UK.]

Re: Re: [PATCH]Add a flag for shadow pages

[Keir Fraser]

On 04/03/2009 12:13, "Tim Deegan" <Tim.Deegan@eu.citrix.com> wrote:

> shadow_get_page_from_l1e() should never be handling a pointer to a
> shadow -- if it does that then we've let the guest see the shadows and
> all invariants go out the window.
> 
> sh_mfn_is_a_pagetable() looks OK too; it only gets called based on the
> contents of shadow PTEs or the MFNs that guests are writing to, both of
> which should be safe.
> 
> It all feels a bit fragile to me though, compared to the old layout
> where we always knew the owner field would be NULL.

This is the tradeoff for the new get_page() implementation and wider
reference counts. We just need to be careful and use
page_get_owner_and_reference() in the few cases it seems to be needed.

 -- Keir

Re: Re: [PATCH]Add a flag for shadow pages

[Keir Fraser]

On 04/03/2009 11:57, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

> On 04/03/2009 09:56, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:
> 
>> Another would be to create a new function page_get_reference_and_owner()
>> which obtains a reference on a guest page and *returns* the (now known
>> valid) domain pointer. Probably this is nicer actually. Then all existing
>> users of page_get_owner() need checking to ensure they don't need to use the
>> new more expensive function -- I think some are probably actually unsafe now
>> that shadow pages clobber the domain field.
> 
> I'm working on this by the way. I'll clean up everything except shadow uses
> of page_get_owner().

Changeset 19268. See get_page_from_l1e() for an example safe usage of new
page_get_owner_and_reference() function.

 -- Keir

RE: Re: [PATCH]Add a flag for shadow pages

[Jiang, Yunhong]

Keir Fraser <mailto:keir.fraser@eu.citrix.com> wrote:
> On 04/03/2009 11:57, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:
> 
>> On 04/03/2009 09:56, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:
>> 
>>> Another would be to create a new function page_get_reference_and_owner()
>>> which obtains a reference on a guest page and *returns* the (now known
>>> valid) domain pointer. Probably this is nicer actually. Then all existing
>>> users of page_get_owner() need checking to ensure they don't need to use
>>> the new more expensive function -- I think some are probably actually
>>> unsafe now that shadow pages clobber the domain field.
>> 
>> I'm working on this by the way. I'll clean up everything except shadow uses
>> of page_get_owner().
> 
> Changeset 19268. See get_page_from_l1e() for an example safe
> usage of new
> page_get_owner_and_reference() function.

Thanks for your really quick-hand implementation. I will update my another patch accordingly tomorrow.

So still one question to the two assertion (or to Tim??) in sh_rm_write_access_from_sl1p()/sh_put_ref(). What's the potential error to be protected by this checking? If page is a shadow, it's count_info will always be 0, right? Or it is just a sanity checking?

I need change this is because, if we mark a page offline, then the count_info is not 0, even for shadow page. Can I just checking the count_mask here?

Thanks
Yunhong Jiang

> 
> -- Keir

Re: Re: [PATCH]Add a flag for shadow pages

[Gianluca Guida]

Jiang, Yunhong wrote:
> Thanks for your really quick-hand implementation. I will update my another patch accordingly tomorrow.
> 
> So still one question to the two assertion (or to Tim??) in sh_rm_write_access_from_sl1p()/sh_put_ref(). What's the potential error to be protected by this checking? If page is a shadow, it's count_info will always be 0, right? Or it is just a sanity checking?

They're sanity checking. Checks are there to be sure that the page is a 
shadow page. It used to be "->mbz == 0", which was clearer, in the old 
page_info model.

In the sh_rm_write_access_from_sl1p(), the check is there to ensure that 
the page is *still* a shadow page. As for current code, though, that 
check should be useless.

I actually think that having a more precise way of knowing when a page 
is a shadow page is definitely needed, for debugging or even in case the 
shadow allocation becomes more dynamic in the future.

> I need change this is because, if we mark a page offline, then the count_info is not 0, even for shadow page. Can I just checking the count_mask here?

Ehr, I am not following the discussions about your patch, but I suppose 
you set the page off-line after all references and uses of the page, 
even inside shadows, are removed. Or the count_info is != 0 to signal 
that the page is going offline?

Thanks,
Gianluca

Re: Re: [PATCH]Add a flag for shadow pages

[Keir Fraser]

On 04/03/2009 16:01, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:

> Thanks for your really quick-hand implementation. I will update my another
> patch accordingly tomorrow.
> 
> So still one question to the two assertion (or to Tim??) in
> sh_rm_write_access_from_sl1p()/sh_put_ref(). What's the potential error to be
> protected by this checking? If page is a shadow, it's count_info will always
> be 0, right? Or it is just a sanity checking?
> 
> I need change this is because, if we mark a page offline, then the count_info
> is not 0, even for shadow page. Can I just checking the count_mask here?

Current shadow pages always have count_info == 0. If you add in extra flags
that you want preserved in shadow pages, you'll need some modifications like
in your last patch.

 -- Keir

RE: Re: [PATCH]Add a flag for shadow pages

[Jiang, Yunhong]

Gianluca Guida <mailto:gianluca.guida@eu.citrix.com> wrote:
> Jiang, Yunhong wrote:
>> Thanks for your really quick-hand implementation. I will
> update my another patch accordingly tomorrow.
>> 
>> So still one question to the two assertion (or to Tim??) in
> sh_rm_write_access_from_sl1p()/sh_put_ref(). What's the
> potential error to be protected by this checking? If page is a
> shadow, it's count_info will always be 0, right? Or it is just
> a sanity checking?
> 
> They're sanity checking. Checks are there to be sure that the
> page is a
> shadow page. It used to be "->mbz == 0", which was clearer, in the old
> page_info model. 
> 
> In the sh_rm_write_access_from_sl1p(), the check is there to
> ensure that
> the page is *still* a shadow page. As for current code, though, that check
> should be useless. 
> 
> I actually think that having a more precise way of knowing when a page
> is a shadow page is definitely needed, for debugging or even
> in case the
> shadow allocation becomes more dynamic in the future.

I think a shadow page has always count_info == 0, while count_info==0 does not always mean a shadow pages.

> 
>> I need change this is because, if we mark a page offline,
> then the count_info is not 0, even for shadow page. Can I just checking the
> count_mask here? 
> 
> Ehr, I am not following the discussions about your patch, but
> I suppose
> you set the page off-line after all references and uses of the page,
> even inside shadows, are removed. Or the count_info is != 0 to signal
> that the page is going offline?

The latter. We will mark the page offline pending through a new flag. When we free the page, we will check the flag to decide the action. So the checking is shadow is not valid anymore. I will update it according to Keir's feedback.

Thanks
Yunhong Jiang

> 
> Thanks,
> Gianluca