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

[Patrick Colp <pjcolp@cs.ubc.ca>]

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

Jan Beulich wrote:
>>>> Grzegorz Milos <gm281@cam.ac.uk> 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

[-- Attachment #2: mem_paging_gfn_to_mfn_check.patch --]
[-- Type: text/x-patch, Size: 1387 bytes --]

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@citrix.com>

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);

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

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