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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 needWith 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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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.] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jiang, Yunhong
2009-Mar-04 16:01 UTC
RE: [Xen-devel] Re: [PATCH]Add a flag for shadow pages
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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Gianluca Guida
2009-Mar-04 16:32 UTC
Re: [Xen-devel] Re: [PATCH]Add a flag for shadow pages
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jiang, Yunhong
2009-Mar-05 02:41 UTC
RE: [Xen-devel] Re: [PATCH]Add a flag for shadow pages
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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel