[PATCH] Do not set page's count_info directly

[PATCH] Do not set page's count_info directly

[Jiang, Yunhong]

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

Page offline patch add several flag to page_info->count_info. However, currently some code will try to set count_info after alloc_domheap_pages without using "&" or "|" operation, this may cause the new flags lost, since there are no protection. This patch try to make sure all write to count_info will only impact specific field.

Also currently shadow code assume count_info is 0 for shadow page, however, this is invalid after the new flags. Change some assert in shadow code.

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

Thanks
Yunhong Jiang

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

clean the count_info usage to make sure it will not overwrite offling information

diff -r 2c256b324e86 xen/arch/x86/mm/hap/hap.c
--- a/xen/arch/x86/mm/hap/hap.c	Thu Mar 05 02:21:47 2009 +0800
+++ b/xen/arch/x86/mm/hap/hap.c	Thu Mar 05 02:36:04 2009 +0800
@@ -152,7 +152,7 @@ static struct page_info *hap_alloc_p2m_p
         d->arch.paging.hap.total_pages--;
         d->arch.paging.hap.p2m_pages++;
         page_set_owner(pg, d);
-        pg->count_info = 1;
+        pg->count_info |= 1;
     }
 
     hap_unlock(d);
@@ -167,7 +167,7 @@ void hap_free_p2m_page(struct domain *d,
     if ( (pg->count_info & PGC_count_mask) != 1 )
         HAP_ERROR("Odd p2m page count c=%#lx t=%"PRtype_info"\n",
                   pg->count_info, pg->u.inuse.type_info);
-    pg->count_info = 0;
+    pg->count_info &= ~PGC_count_mask;
     /* Free should not decrement domain's total allocation, since
      * these pages were allocated without an owner. */
     page_set_owner(pg, NULL);
@@ -218,7 +218,6 @@ hap_set_allocation(struct domain *d, uns
             ASSERT(pg);
             d->arch.paging.hap.free_pages--;
             d->arch.paging.hap.total_pages--;
-            pg->count_info = 0;
             free_domheap_page(pg);
         }
 
diff -r 2c256b324e86 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c	Thu Mar 05 02:21:47 2009 +0800
+++ b/xen/arch/x86/mm/p2m.c	Thu Mar 05 02:36:04 2009 +0800
@@ -177,7 +177,7 @@ p2m_next_level(struct domain *d, mfn_t *
             return 0;
         page_list_add_tail(pg, &d->arch.p2m->pages);
         pg->u.inuse.type_info = type | 1 | PGT_validated;
-        pg->count_info = 1;
+        pg->count_info |= 1;
 
         new_entry = l1e_from_pfn(mfn_x(page_to_mfn(pg)),
                                  __PAGE_HYPERVISOR|_PAGE_USER);
@@ -216,7 +216,7 @@ p2m_next_level(struct domain *d, mfn_t *
             return 0;
         page_list_add_tail(pg, &d->arch.p2m->pages);
         pg->u.inuse.type_info = PGT_l1_page_table | 1 | PGT_validated;
-        pg->count_info = 1;
+        pg->count_info |= 1;
         
         /* New splintered mappings inherit the flags of the old superpage, 
          * with a little reorganisation for the _PAGE_PSE_PAT bit. */
diff -r 2c256b324e86 xen/arch/x86/mm/shadow/common.c
--- a/xen/arch/x86/mm/shadow/common.c	Thu Mar 05 02:21:47 2009 +0800
+++ b/xen/arch/x86/mm/shadow/common.c	Thu Mar 05 03:25:15 2009 +0800
@@ -1677,7 +1677,7 @@ sh_alloc_p2m_pages(struct domain *d)
          * believed to be a concern.
          */
         page_set_owner(&pg[i], d);
-        pg[i].count_info = 1;
+        pg[i].count_info |= 1;
         page_list_add_tail(&pg[i], &d->arch.paging.shadow.p2m_freelist);
     }
     return 1;
@@ -1721,7 +1721,7 @@ shadow_free_p2m_page(struct domain *d, s
         SHADOW_ERROR("Odd p2m page count c=%#lx t=%"PRtype_info"\n",
                      pg->count_info, pg->u.inuse.type_info);
     }
-    pg->count_info = 0;
+    pg->count_info &= ~PGC_count_mask;
     /* Free should not decrement domain's total allocation, since 
      * these pages were allocated without an owner. */
     page_set_owner(pg, NULL); 
@@ -1895,7 +1895,7 @@ static void sh_hash_audit_bucket(struct 
     while ( sp )
     {
         /* Not a shadow? */
-        BUG_ON( sp->count_info != 0 );
+        BUG_ON( (sp->count_info & PGC_count_mask )!= 0 ) ;
         /* Bogus type? */
         BUG_ON( sp->u.sh.type == 0 );
         BUG_ON( sp->u.sh.type > SH_type_max_shadow );
diff -r 2c256b324e86 xen/arch/x86/mm/shadow/multi.c
--- a/xen/arch/x86/mm/shadow/multi.c	Thu Mar 05 02:21:47 2009 +0800
+++ b/xen/arch/x86/mm/shadow/multi.c	Thu Mar 05 02:56:00 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_count_mask) != 0)
          || (sp->u.sh.type != SH_type_l1_shadow
              && sp->u.sh.type != SH_type_fl1_shadow) )
         goto fail;
diff -r 2c256b324e86 xen/arch/x86/mm/shadow/private.h
--- a/xen/arch/x86/mm/shadow/private.h	Thu Mar 05 02:21:47 2009 +0800
+++ b/xen/arch/x86/mm/shadow/private.h	Thu Mar 05 02:58:08 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_count_mask));
 
     /* If this is the entry in the up-pointer, remove it */
     if ( entry_pa != 0 

[-- 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] Do not set page's count_info directly

[Gianluca Guida]

On Thu, Mar 5, 2009 at 10:11 AM, Jiang, Yunhong <yunhong.jiang@intel.com> wrote:
> Page offline patch add several flag to page_info->count_info. However, currently some code will try to set count_info after alloc_domheap_pages without using "&" or "|" operation, this may cause the new flags lost, since there are no protection. This patch try to make sure all write to count_info will only impact specific field.

Hm, wouldn't be better to add some comments in mm.h or do this through
a macro? I think that one would normally tend to suppose, after you
allocate a page, that the count_info is all yours to set. It usually
is, since the offlining should be a rare event (I hope?), so catching
this kind of bug would be very hard, and make the whole mechanism very
fragile.

> Also currently shadow code assume count_info is 0 for shadow page, however, this is invalid after the new flags. Change some assert in shadow code.

Yes, that's correct. I find, though, (count_info & PGC_count_mask !=
0) as a check if the page is a shadow *very* confusing. Could you
define a macro with something like page_is_a_shadow_page() and hide
this mm.c dirty secret?

Thanks,
Gianluca

-- 
It was a type of people I did not know, I found them very strange and
they did not inspire confidence at all. Later I learned that I had been
introduced to electronic engineers.
                                                  E. W. Dijkstra

Re: [PATCH] Do not set page's count_info directly

[Tim Deegan]

At 12:32 +0000 on 05 Mar (1236256358), Gianluca Guida wrote:
> Yes, that's correct. I find, though, (count_info & PGC_count_mask !=
> 0) as a check if the page is a shadow *very* confusing. Could you
> define a macro with something like page_is_a_shadow_page() and hide
> this mm.c dirty secret?

The check doesn't tell you it's a shadow page -- it just tells you it's
*not* allocated in another way.  Checking with the mask is OK here,
perhaps with a comment change.

Tim.

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

Re: [PATCH] Do not set page's count_info directly

[Gianluca Guida]

On Thu, Mar 5, 2009 at 1:38 PM, Tim Deegan <Tim.Deegan@citrix.com> wrote:
> The check doesn't tell you it's a shadow page -- it just tells you it's
> *not* allocated in another way.  Checking with the mask is OK here,
> perhaps with a comment change.

Yes, but this is what the code (in the shadow code) intends to check.
My complete plan is to have this macro and restore the idea of having
a flag for shadow pages because, as for now, the fact that we can't
distinguish clearly a shadow page should be considered a sort of bug,
given the code assumptions. Or at least a good basis for future bugs.

Thanks,
Gianluca

-- 
It was a type of people I did not know, I found them very strange and
they did not inspire confidence at all. Later I learned that I had been
introduced to electronic engineers.
                                                  E. W. Dijkstra

Re: [PATCH] Do not set page's count_info directly

[Keir Fraser]

On 05/03/2009 12:45, "Gianluca Guida" <glguida@gmail.com> wrote:

> On Thu, Mar 5, 2009 at 1:38 PM, Tim Deegan <Tim.Deegan@citrix.com> wrote:
>> The check doesn't tell you it's a shadow page -- it just tells you it's
>> *not* allocated in another way.  Checking with the mask is OK here,
>> perhaps with a comment change.
> 
> Yes, but this is what the code (in the shadow code) intends to check.
> My complete plan is to have this macro and restore the idea of having
> a flag for shadow pages because, as for now, the fact that we can't
> distinguish clearly a shadow page should be considered a sort of bug,
> given the code assumptions. Or at least a good basis for future bugs.

I'm happy for shadow code to have another bit in count_info for its own use.
I don't think it's good to have other code (e.g., generic page offlining
code) peeking at it, is all. The particular case for a new flag described
above sounds fine.

 -- Keir

RE: [PATCH] Do not set page's count_info directly

[Jiang, Yunhong]

Keir Fraser <mailto:keir.fraser@eu.citrix.com> wrote:
> On 05/03/2009 12:45, "Gianluca Guida" <glguida@gmail.com> wrote:
> 
>> On Thu, Mar 5, 2009 at 1:38 PM, Tim Deegan
> <Tim.Deegan@citrix.com> wrote:
>>> The check doesn't tell you it's a shadow page -- it just tells you it's
>>> *not* allocated in another way.  Checking with the mask is OK here,
>>> perhaps with a comment change.
>> 
>> Yes, but this is what the code (in the shadow code) intends to check.
>> My complete plan is to have this macro and restore the idea of having
>> a flag for shadow pages because, as for now, the fact that we can't
>> distinguish clearly a shadow page should be considered a sort of bug,
>> given the code assumptions. Or at least a good basis for future bugs.
> 
> I'm happy for shadow code to have another bit in count_info
> for its own use.
> I don't think it's good to have other code (e.g., generic page offlining
> code) peeking at it, is all. The particular case for a new
> flag described
> above sounds fine.

The new code is not using that flag anymore, it is use the page_get_owner_and_reference() to get the owner.

Thanks
Yunhong Jiang

> 
> -- Keir

Re: [PATCH] Do not set page's count_info directly

[Gianluca Guida]

Jiang, Yunhong wrote:
> Keir Fraser <mailto:keir.fraser@eu.citrix.com> wrote:
>> On 05/03/2009 12:45, "Gianluca Guida" <glguida@gmail.com> wrote:
>>
>>> On Thu, Mar 5, 2009 at 1:38 PM, Tim Deegan
>> <Tim.Deegan@citrix.com> wrote:
>>>> The check doesn't tell you it's a shadow page -- it just tells you it's
>>>> *not* allocated in another way.  Checking with the mask is OK here,
>>>> perhaps with a comment change.
>>> Yes, but this is what the code (in the shadow code) intends to check.
>>> My complete plan is to have this macro and restore the idea of having
>>> a flag for shadow pages because, as for now, the fact that we can't
>>> distinguish clearly a shadow page should be considered a sort of bug,
>>> given the code assumptions. Or at least a good basis for future bugs.
>> I'm happy for shadow code to have another bit in count_info
>> for its own use.
>> I don't think it's good to have other code (e.g., generic page offlining
>> code) peeking at it, is all. The particular case for a new
>> flag described
>> above sounds fine.
> 
> The new code is not using that flag anymore, it is use the page_get_owner_and_reference() to get the owner.

Yes, I was actually thinking about adding it. Turns out that I realized 
that it's not the best way to do stuff, but I think that I'll do the 
following:

- add a sh_page_is_a_shadow_page() in shadow code to make the code less 
confusing (with a small caveat comment about not being perfectly precise).
- exercise my English skills by writing a comment in page_info that 
summarizes this thread findings about current use if count_info and 
shadow pages.

Thanks,
Gianluca

RE: [PATCH] Do not set page's count_info directly

[Jiang, Yunhong]

>> 
>> The new code is not using that flag anymore, it is use the
> page_get_owner_and_reference() to get the owner.
> 
> Yes, I was actually thinking about adding it. Turns out that I realized
> that it's not the best way to do stuff, but I think that I'll do the
> following: 

One clarification, the "new code" I'm talking about is my page-offline patch.


Thanks
Yunhong Jiang

Re: [PATCH] Do not set page's count_info directly

[Keir Fraser]

On 05/03/2009 12:32, "Gianluca Guida" <glguida@gmail.com> wrote:

>> Also currently shadow code assume count_info is 0 for shadow page, however,
>> this is invalid after the new flags. Change some assert in shadow code.
> 
> Yes, that's correct. I find, though, (count_info & PGC_count_mask !=
> 0) as a check if the page is a shadow *very* confusing. Could you
> define a macro with something like page_is_a_shadow_page() and hide
> this mm.c dirty secret?

This would be a shadow-code-specific macro. In general count_mask==0 can
also mean the page is free, for example.

 -- Keir

RE: [PATCH] Do not set page's count_info directly

[Jiang, Yunhong]

Keir Fraser <mailto:keir.fraser@eu.citrix.com> wrote:
> On 05/03/2009 12:32, "Gianluca Guida" <glguida@gmail.com> wrote:
> 
>>> Also currently shadow code assume count_info is 0 for shadow page,
>>> however, this is invalid after the new flags. Change some assert in
>>> shadow code. 
>> 
>> Yes, that's correct. I find, though, (count_info & PGC_count_mask !=
>> 0) as a check if the page is a shadow *very* confusing. Could you
>> define a macro with something like page_is_a_shadow_page() and hide
>> this mm.c dirty secret?
> 
> This would be a shadow-code-specific macro. In general count_mask==0 can
> also mean the page is free, for example.

Maybe I'm wrong, but anonymous page will have count_mask==0 while they are allocated.

Currently the only way that we can make sure the guest is free is through the boot allocator bitmap. When the bit is clear, the page is free.
When the bit is set, and the count_mask!=0, then it is owned by someone like guest ram, p2m table etc. if the count_mask==0, then it is either a shadow page or a anoynymous page.

Or do I missed anything?

Thanks
Yunhong Jiang


> 
> -- Keir

Re: [PATCH] Do not set page's count_info directly

[Keir Fraser]

On 05/03/2009 14:33, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:

>> This would be a shadow-code-specific macro. In general count_mask==0 can
>> also mean the page is free, for example.
> 
> Maybe I'm wrong, but anonymous page will have count_mask==0 while they are
> allocated.
> 
> Currently the only way that we can make sure the guest is free is through the
> boot allocator bitmap. When the bit is clear, the page is free.
> When the bit is set, and the count_mask!=0, then it is owned by someone like
> guest ram, p2m table etc. if the count_mask==0, then it is either a shadow
> page or a anoynymous page.
> 
> Or do I missed anything?

You are correct. I only gave one obvious example where count_info==0.

 -- Keir

RE: [PATCH] Do not set page's count_info directly

[Jiang, Yunhong]

xen-devel-bounces@lists.xensource.com <> wrote:
> On 05/03/2009 14:33, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:
> 
>>> This would be a shadow-code-specific macro. In general count_mask==0 can
>>> also mean the page is free, for example.
>> 
>> Maybe I'm wrong, but anonymous page will have count_mask==0 while they are
>> allocated. 
>> 
>> Currently the only way that we can make sure the guest is free is through
>> the boot allocator bitmap. When the bit is clear, the page is free.
>> When the bit is set, and the count_mask!=0, then it is owned by someone
>> like guest ram, p2m table etc. if the count_mask==0, then it is either a
>> shadow page or a anoynymous page. 
>> 
>> Or do I missed anything?
> 
> You are correct. I only gave one obvious example where count_info==0.

Thanks for your clarification.

-- Yunhong Jiang

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

RE: [PATCH] Do not set page's count_info directly

[Jiang, Yunhong]

Gianluca Guida <mailto:glguida@gmail.com> wrote:
> On Thu, Mar 5, 2009 at 10:11 AM, Jiang, Yunhong
> <yunhong.jiang@intel.com> wrote:
>> Page offline patch add several flag to
> page_info->count_info. However, currently some code will try
> to set count_info after alloc_domheap_pages without using "&"
> or "|" operation, this may cause the new flags lost, since
> there are no protection. This patch try to make sure all write
> to count_info will only impact specific field.
> 
> Hm, wouldn't be better to add some comments in mm.h or do this through
> a macro? I think that one would normally tend to suppose, after you
> allocate a page, that the count_info is all yours to set. It usually
> is, since the offlining should be a rare event (I hope?), so catching
> this kind of bug would be very hard, and make the whole mechanism very
> fragile. 

Yes, I considered how to prevent this kind of mis-use and in the end find it is impossible. The caller can always set the count_info if they like it.

One option is to use another bitmap to keep the offline information instead of page_info, but that will wast too many memory, since page offline is rare case as you pointed out.
Another option is to enable this bitmap only in debug version. When free page, we will check the bitmap, if the bitmap is set while the count_info is not set, it means something wrong happens and raise a BUG().

Any suggestion?

Thanks
Yunhong Jiang

> 
>> Also currently shadow code assume count_info is 0 for shadow
> page, however, this is invalid after the new flags. Change
> some assert in shadow code.
> 
> Yes, that's correct. I find, though, (count_info & PGC_count_mask !=
> 0) as a check if the page is a shadow *very* confusing. Could you
> define a macro with something like page_is_a_shadow_page() and hide this
> mm.c dirty secret? 
> 
> Thanks,
> Gianluca
> 
> --
> It was a type of people I did not know, I found them very strange and
> they did not inspire confidence at all. Later I learned that I had been
> introduced to electronic engineers.
>                                                  E. W. Dijkstra