Re: [Xen-devel] [PATCH] Paging and memory sharing for HVM guests

[Patrick Colp <pjcolp@xxxxxxxxx>]

Jan Beulich wrote:
> > > > Grzegorz Milos <gm281@xxxxxxxxx> 17.12.09 00:14 >>>
> > The series of 46 patches attached to this email contain the initial
> > implementation of memory paging and sharing for Xen. Patrick Colp
> > leads the work on the pager, and I am mostly responsible for memory
> > sharing. We would be grateful for any comments/suggestions you might
> > have. Individual patches are labeled with comments describing their
> > purpose and a sign-off footnote. Of course we are happy to discuss
> > them in more detail, as required. Assuming that there are no major
> > objections against including them in the mainstream xen-unstable tree,
> > we would like to move future development to that tree.
> So as far as I can tell we now have to colliding values in domctl.h:
>
> #define XEN_DOMCTL_PFINFO_LPINTAB (0x1U<<31)
> #define XEN_DOMCTL_PFINFO_PAGEDTAB (0x8U<<28)
>
> Was it determined that this is not (going to be) a problem? It's just
> the tools interface, but it's a public header...
XEN_DOMCTL_PFINFO_LPINTAB is currently only used by suspend/restore and 
offline page (which says the domain should be suspended first). Paging 
hasn't been fully tested with suspend yet. However, in xc_domain_save(), 
xc_map_foreign_batch() is called before xc_get_pfn_type_batch(), and 
xc_map_foreign_batch() ensures that all the pages in the specified range 
are paged in. So as far as I can tell, there should be no conflict here 
right now. But, that doesn't mean this couldn't cause problems in the 
future. Coming up with a non-conflicting solution would ultimately be 
preferred.

> Also there are several places were gmfn_to_mfn() was replaced by
> mfn_x(gfn_to_mfn()), but the p2m_is_valid() check that the former
> does was lost. Again - has it been determined that this will never
> cause any problem?
It's been awhile since I made that decision, but I seem to recall it making 
sense at the time. That could have been for EPT only, though (which is what 
paging/mem_event was originally designed on). However, I can see no reason 
to not have the p2m_is_valid() check put in below the gfn_to_mfn() calls. 
I've attached a patch which does this (except for in hvm_set_ioreq_page, 
since there's already a check for !p2m_is_ram() which will cause the 
function to return an error).

Patrick

Add checks for p2m_is_valid() after calls to gfn_to_mfn() that replace calls
to gmfn_to_mfn(), which does the check internally.

Signed-off-by: Patrick Colp <Patrick.Colp@xxxxxxxxxx>

diff -r 1a911fd65e52 xen/arch/x86/mm.c
--- a/xen/arch/x86/mm.c Fri Dec 18 07:53:27 2009 +0000
+++ b/xen/arch/x86/mm.c Fri Dec 18 09:01:05 2009 -0800
@@ -3105,6 +3105,8 @@
             req.ptr -= cmd;
             gmfn = req.ptr >> PAGE_SHIFT;
             mfn = mfn_x(gfn_to_mfn(pt_owner, gmfn, &p2mt));
+            if ( !p2m_is_valid(p2mt) )
+              mfn = INVALID_MFN;
 
             if ( p2m_is_paged(p2mt) )
             {
diff -r 1a911fd65e52 xen/common/grant_table.c
--- a/xen/common/grant_table.c  Fri Dec 18 07:53:27 2009 +0000
+++ b/xen/common/grant_table.c  Fri Dec 18 09:01:05 2009 -0800
@@ -1888,6 +1888,8 @@
     {
         p2m_type_t p2mt;
         s_frame = mfn_x(gfn_to_mfn(sd, op->source.u.gmfn, &p2mt));
+        if ( !p2m_is_valid(p2mt) )
+          s_frame = INVALID_MFN;
         if ( p2m_is_paging(p2mt) )
         {
             p2m_mem_paging_populate(sd, op->source.u.gmfn);
@@ -1929,6 +1931,8 @@
     {
         p2m_type_t p2mt;
         d_frame = gfn_to_mfn_private(dd, op->dest.u.gmfn, &p2mt);
+        if ( !p2m_is_valid(p2mt) )
+          d_frame = INVALID_MFN;
         if ( p2m_is_paging(p2mt) )
         {
             p2m_mem_paging_populate(dd, op->dest.u.gmfn);

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