[PATCH] Paging and memory sharing for HVM guests

[PATCH] Paging and memory sharing for HVM guests

[Grzegorz Milos]

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

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.

Thanks
Patrick & Gregor

[-- Attachment #2: mem_paging_and_sharing_final.tar.gz --]
[-- Type: application/x-gzip, Size: 85873 bytes --]

[-- 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] Paging and memory sharing for HVM guests

[Dan Magenheimer]

Hi Gregor and Patrick --

Congrats!

Could you provide instructions on how to apply them
to xen-unstable tip?  I tried applying them using
"patch -p1" in the order given in the file "series"
and most applied but I got a handful of failed hunks
and even one patch file missing altogether
(...pfec_page_paged.patch).

(Just xen patches, no Linux patches tried yet.)

Thanks,
Dan

> -----Original Message-----
> From: Grzegorz Milos [mailto:gm281@cam.ac.uk]
> Sent: Wednesday, December 16, 2009 4:15 PM
> To: xen-devel@lists.xensource.com; Patrick Colp; Andrew Peace; Keir
> Fraser
> Subject: [Xen-devel] [PATCH] Paging and memory sharing for HVM guests
> 
> 
> 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.
> 
> Thanks
> Patrick & Gregor
>

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

[Patrick Colp]

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

Oops! I've attached the missing patch.

The easiest way to apply them is to use a mercurial patch queue, but simply 
applying them in order of the series file works too. Did the patches that 
failed fail before or after this missing one? If after, then try again with 
this patch and see if there are still any problems. If before, which patches?

The queue was refreshed against revision 20683, pulled yesterday morning.


Patrick


Dan Magenheimer wrote:
> Hi Gregor and Patrick --
> 
> Congrats!
> 
> Could you provide instructions on how to apply them
> to xen-unstable tip?  I tried applying them using
> "patch -p1" in the order given in the file "series"
> and most applied but I got a handful of failed hunks
> and even one patch file missing altogether
> (...pfec_page_paged.patch).
> 
> (Just xen patches, no Linux patches tried yet.)
> 
> Thanks,
> Dan
> 
>> -----Original Message-----
>> From: Grzegorz Milos [mailto:gm281@cam.ac.uk]
>> Sent: Wednesday, December 16, 2009 4:15 PM
>> To: xen-devel@lists.xensource.com; Patrick Colp; Andrew Peace; Keir
>> Fraser
>> Subject: [Xen-devel] [PATCH] Paging and memory sharing for HVM guests
>>
>>
>> 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.
>>
>> Thanks
>> Patrick & Gregor
>>
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

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

hap_gva_to_gfn paging support. Return PFEC_page_paged when a paged
out page is found. Ensure top-level page table page and l1 entry
are paged in. If an intermediary page table page is paged out,
propogate error to caller.

Signed-off-by: Patrick Colp <Patrick.Colp@citrix.com>

diff -r 5c370b75a5b7 xen/arch/x86/mm/hap/guest_walk.c
--- a/xen/arch/x86/mm/hap/guest_walk.c	Wed Oct 14 10:33:40 2009 -0700
+++ b/xen/arch/x86/mm/hap/guest_walk.c	Wed Oct 14 10:33:44 2009 -0700
@@ -46,6 +46,14 @@
     /* Get the top-level table's MFN */
     cr3 = v->arch.hvm_vcpu.guest_cr[3];
     top_mfn = gfn_to_mfn(v->domain, _gfn(cr3 >> PAGE_SHIFT), &p2mt);
+    if ( p2m_is_paging(p2mt) )
+    {
+//        if ( p2m_is_paged(p2mt) )
+            p2m_mem_paging_populate(v->domain, cr3 >> PAGE_SHIFT);
+
+        pfec[0] = PFEC_page_paged;
+        return INVALID_GFN;
+    }
     if ( !p2m_is_ram(p2mt) )
     {
         pfec[0] &= ~PFEC_page_present;
@@ -62,12 +70,28 @@
     unmap_domain_page(top_map);
 
     /* Interpret the answer */
-    if ( missing == 0 ) 
-        return gfn_x(guest_l1e_get_gfn(gw.l1e));
-    
+    if ( missing == 0 )
+    {
+        gfn_t gfn = guest_l1e_get_gfn(gw.l1e);
+        gfn_to_mfn(v->domain, gfn, &p2mt);
+        if ( p2m_is_paging(p2mt) )
+        {
+//            if ( p2m_is_paged(p2mt) )
+                p2m_mem_paging_populate(v->domain, gfn_x(gfn));
+
+            pfec[0] = PFEC_page_paged;
+            return INVALID_GFN;
+        }
+
+        return gfn_x(gfn);
+    }
+
     if ( missing & _PAGE_PRESENT )
         pfec[0] &= ~PFEC_page_present;
-    
+
+    if ( missing & _PAGE_PAGED )
+        pfec[0] = PFEC_page_paged;
+
     return INVALID_GFN;
 }
 
diff -r 5c370b75a5b7 xen/include/asm-x86/processor.h
--- a/xen/include/asm-x86/processor.h	Wed Oct 14 10:33:40 2009 -0700
+++ b/xen/include/asm-x86/processor.h	Wed Oct 14 10:33:44 2009 -0700
@@ -132,6 +132,7 @@
 #define PFEC_user_mode      (1U<<2)
 #define PFEC_reserved_bit   (1U<<3)
 #define PFEC_insn_fetch     (1U<<4)
+#define PFEC_page_paged     (1U<<5)
 
 #ifndef __ASSEMBLY__
 

[-- 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] Paging and memory sharing for HVM guests

[Dan Magenheimer]

With the missing patch present, applying in the
order of series worked fine.  Against 20656 (tip as
of 117 minutes ago), there are about 14
hunks with an offset, but those are unlikely
to be any problem.

Thanks!
Dan

> -----Original Message-----
> From: Patrick Colp [mailto:pjcolp@cs.ubc.ca]
> Sent: Wednesday, December 16, 2009 5:00 PM
> To: Dan Magenheimer
> Cc: Grzegorz Milos; xen-devel@lists.xensource.com; Andrew Peace; Keir
> Fraser
> Subject: Re: [Xen-devel] [PATCH] Paging and memory sharing for HVM
> guests
> 
> 
> Oops! I've attached the missing patch.
> 
> The easiest way to apply them is to use a mercurial patch 
> queue, but simply 
> applying them in order of the series file works too. Did the 
> patches that 
> failed fail before or after this missing one? If after, then 
> try again with 
> this patch and see if there are still any problems. If 
> before, which patches?
> 
> The queue was refreshed against revision 20683, pulled 
> yesterday morning.
> 
> 
> Patrick
> 
> 
> Dan Magenheimer wrote:
> > Hi Gregor and Patrick --
> > 
> > Congrats!
> > 
> > Could you provide instructions on how to apply them
> > to xen-unstable tip?  I tried applying them using
> > "patch -p1" in the order given in the file "series"
> > and most applied but I got a handful of failed hunks
> > and even one patch file missing altogether
> > (...pfec_page_paged.patch).
> > 
> > (Just xen patches, no Linux patches tried yet.)
> > 
> > Thanks,
> > Dan
> > 
> >> -----Original Message-----
> >> From: Grzegorz Milos [mailto:gm281@cam.ac.uk]
> >> Sent: Wednesday, December 16, 2009 4:15 PM
> >> To: xen-devel@lists.xensource.com; Patrick Colp; Andrew Peace; Keir
> >> Fraser
> >> Subject: [Xen-devel] [PATCH] Paging and memory sharing for 
> HVM guests
> >>
> >>
> >> 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.
> >>
> >> Thanks
> >> Patrick & Gregor
> >>
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel
>

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

[Jan Beulich]

>>> 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.

An overview description of the design would be nice, to have a basic
understanding before looking at the individual patches. In particular,
from a first brief look, I'm having the impression that only HVM guests'
pages can be subject to paging.

On the Linux patches:

Introducing another bogus failure indicator for the mmap_batch
privcmd operations seems rather undesirable - we'll already need to
find a backwards-compatible solution to the current (broken) or-ing
in of 0xf0000000 (broken because MFNs can now be more than
28 bits wide).

Using msleep() with hard-coded values (in at least one case even
contradicting the accompanying comment) seems more like a hack
than a permanent solution. Can't there be some signaling done, or
can't there alternatively be a polling hypercall?

Removing support for IOCTL_PRIVCMD_MMAP from the pv-ops
implementation seems pretty unrelated, so should probably be a
separate patch.

Also, most of the patches seem to use blanks instead of tabs for
indentation, and occasionally other non-standard formatting.

Jan

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

[Jan Beulich]

Keir,

>>> 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.

as I just realized you already committed these patches - wouldn't it
make sense to allow for some reviewing/discussion at least for larger
batches like this? I would really have wanted the Linux side issues I
already pointed out fixed before they go in, and lacking a general
description of the approach taken I didn't even take a closer look at
the Xen side changes (but during the brief check I did, I think I
spotted at least one problem already).

Jan

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

[Grzegorz Milos]

> An overview description of the design would be nice, to have a basic
> understanding before looking at the individual patches. In particular,
> from a first brief look, I'm having the impression that only HVM guests'
> pages can be subject to paging.

Indeed. Paging and memory sharing is designed for HVM guests only. We
are compiling proper documentation for it, but we'll try to write a
short(er) overview too.

> On the Linux patches:
>
> Introducing another bogus failure indicator for the mmap_batch
> privcmd operations seems rather undesirable - we'll already need to
> find a backwards-compatible solution to the current (broken) or-ing
> in of 0xf0000000 (broken because MFNs can now be more than
> 28 bits wide).

Surly you mean > 30 bits wide?
Anyway, I'll let Patrick comment on that, since he is the author of
this bit of the code.

> Using msleep() with hard-coded values (in at least one case even
> contradicting the accompanying comment) seems more like a hack
> than a permanent solution. Can't there be some signaling done, or
> can't there alternatively be a polling hypercall?

We intent do use proper signalling for EAGAIN failed grant maps at
least. For example, you'll notice that I've separated the block
backend patch into two, with the
'mem_sharing_blkback_periodic_retry.patch' implementing a temporary
retry loop. Once we've got memory event interface ready, we'll replace
the loop with a mem event 'kick'. In some cases though, a simple retry
loop seem fine to me (e.g. grant maps of ring pages).

WRT to a catch-all retry loop for 'direct' foreign mappings, the idea
is that we'll provide a separate non-blocking call (which may fail
with EAGAIN) for the callers who care about performance. For the time
being a single retry loop was the quickest way to get the code out for
people to test/comment on it.

> Removing support for IOCTL_PRIVCMD_MMAP from the pv-ops
> implementation seems pretty unrelated, so should probably be a
> separate patch.

Forwarding this Q to Patrick again.

> Also, most of the patches seem to use blanks instead of tabs for
> indentation, and occasionally other non-standard formatting.

Mea culpa. I tend to have 'tab expansion' set in my editor. I'll fix
whitespace in my linux kernel patches.

Thanks
Gregor

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

[Patrick Colp]

>> Introducing another bogus failure indicator for the mmap_batch
>> privcmd operations seems rather undesirable - we'll already need to
>> find a backwards-compatible solution to the current (broken) or-ing
>> in of 0xf0000000 (broken because MFNs can now be more than
>> 28 bits wide).
> 
> Surly you mean > 30 bits wide?
> Anyway, I'll let Patrick comment on that, since he is the author of
> this bit of the code.

I realise that this isn't the best approach. However, as you point out,
there already is 0xf0000000 in the code. I thought it easiest just to latch
on to that for now, although you're quite right that a better solution
needs to be found, preferably something that will solve both these problems.


>> Removing support for IOCTL_PRIVCMD_MMAP from the pv-ops
>> implementation seems pretty unrelated, so should probably be a
>> separate patch.
> 
> Forwarding this Q to Patrick again.

Yes, you're probably right. A while back I submitted a patch which changed
libxc to use only mmap_batch, which is now in the xen-unstable tree. This
was done with the idea of unifying the mmap interface (although ideally it
would be a nicer one that what is there currently, but that's an entirely
different discussion). This all came about while working on this project,
which is why it's been included in the patches here. It could easily be
resubmitted as an independent patch, if that is preferred.


Patrick

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

[Keir Fraser]

On 17/12/2009 11:19, "Jan Beulich" <JBeulich@novell.com> wrote:

>> 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.
> 
> as I just realized you already committed these patches - wouldn't it
> make sense to allow for some reviewing/discussion at least for larger
> batches like this? I would really have wanted the Linux side issues I
> already pointed out fixed before they go in, and lacking a general
> description of the approach taken I didn't even take a closer look at
> the Xen side changes (but during the brief check I did, I think I
> spotted at least one problem already).

Well, maybe I jumped the gun a bit. But I think this feature is inevitable
-- everyone wants it -- so it's a choice between fixing up in tree versus
out of tree. Perhaps it would have been better to get at least the first
round of fixes done before check in.

 -- Keir

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

[Dan Magenheimer]

Does live migration work (reliably) when pages have been
swapped out?  IIRC, this is a limitation of VMware's
implementation (or at least was a limitation at one
point).

> -----Original Message-----
> From: Grzegorz Milos [mailto:gm281@cam.ac.uk]
> Sent: Wednesday, December 16, 2009 4:15 PM
> To: xen-devel@lists.xensource.com; Patrick Colp; Andrew Peace; Keir
> Fraser
> Subject: [Xen-devel] [PATCH] Paging and memory sharing for HVM guests
> 
> 
> 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.
> 
> Thanks
> Patrick & Gregor
>

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

[Konrad Rzeszutek Wilk]

On Wed, Dec 16, 2009 at 11:14:47PM +0000, Grzegorz Milos wrote:
> 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.


Congrats!

And now to the tiny review I did:

1). mem_event_xen_core.patch

"Copyright (c) 2009 Citrix (R)&D) Ltd. (Patrick Colp)"

The lawyers might not like it the copyright being assigned to a non-existent
entity :-(


2).  mem_paging_xen_pfec_page_paged.patch:

15 +    if ( p2m_is_paging(p2mt) )
 16 +    {
 17 +//        if ( p2m_is_paged(p2mt) )

I think you can safely remove the commented out code.

3). mem_sharing_xen_mm.patch:

 27 +    /*
 28 +     * Initialise our DOMID_IO domain.
                                ^^->COW
 29 +     * This domain owns sharable pages.
 30 +     */
 31 +    dom_cow = domain_create(DOMID_COW, DOMCRF_dummy, 0);


4), mem_sharing_tools_eagain.patch

The code spins with a 1 second timeout forever. Would it make sense
to include a retry counter so that, say after an hour (or maybe something
much smaller) you give up?

In the pv-ops patch series:

1). The  "*mfnp |= 0x80000000U;" and "*mfnp |= 0xf0000000U;" should
    use a #define. Maybe copy over the #defines from the xen tree ?

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

[Jan Beulich]

>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 17.12.09 17:38 >>>
>1). The  "*mfnp |= 0x80000000U;" and "*mfnp |= 0xf0000000U;" should
>    use a #define. Maybe copy over the #defines from the xen tree ?

Did you find any defines in the tools sources for that? The only place I
found this condition being checked at all was in xc_map_foreign_pages(),
where it used hard-coded values. Or are you referring to the
XEN_DOMCTL_PFINFO_* values? I'd say they're being mis-used when
applied to the mfn array used by mmap-batch (including apparent
pre-existing uses).

Jan

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

[Patrick Colp]

> 1). mem_event_xen_core.patch
> 
> "Copyright (c) 2009 Citrix (R)&D) Ltd. (Patrick Colp)"
> 
> The lawyers might not like it the copyright being assigned to a non-existent
> entity :-(

Oops. That should be easy enough to fix.


> 2).  mem_paging_xen_pfec_page_paged.patch:
> 
> 15 +    if ( p2m_is_paging(p2mt) )
>  16 +    {
>  17 +//        if ( p2m_is_paged(p2mt) )
> 
> I think you can safely remove the commented out code.

Yes we can. I thought I had removed all of the lines like that.


> 3). mem_sharing_xen_mm.patch:
> 
>  27 +    /*
>  28 +     * Initialise our DOMID_IO domain.
>                                 ^^->COW
>  29 +     * This domain owns sharable pages.
>  30 +     */
>  31 +    dom_cow = domain_create(DOMID_COW, DOMCRF_dummy, 0);
> 
> 
> 4), mem_sharing_tools_eagain.patch
> 
> The code spins with a 1 second timeout forever. Would it make sense
> to include a retry counter so that, say after an hour (or maybe something
> much smaller) you give up?

I'll defer these to Grzegorz.


Patrick

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

[Patrick Colp]

Jan Beulich wrote:
>>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 17.12.09 17:38 >>>
>> 1). The  "*mfnp |= 0x80000000U;" and "*mfnp |= 0xf0000000U;" should
>>    use a #define. Maybe copy over the #defines from the xen tree ?
> 
> Did you find any defines in the tools sources for that? The only place I
> found this condition being checked at all was in xc_map_foreign_pages(),
> where it used hard-coded values. Or are you referring to the
> XEN_DOMCTL_PFINFO_* values? I'd say they're being mis-used when
> applied to the mfn array used by mmap-batch (including apparent
> pre-existing uses).

I think many people agree that this idea of using 0xf0000000 is a very 
inelegant solution (myself included). Maybe now would be a good time to 
hash out what the proper way to deal with this is. I think this discussion 
should extend to the privcmd/libxc mmap interfaces generally as well.


Patrick

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

[Patrick Colp]

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

I've attached two patches which fix up the copyright and remove the 
commented out code. They can be applied in any order.


Patrick


Patrick Colp wrote:
>> 1). mem_event_xen_core.patch
>>
>> "Copyright (c) 2009 Citrix (R)&D) Ltd. (Patrick Colp)"
>>
>> The lawyers might not like it the copyright being assigned to a 
>> non-existent
>> entity :-(
> 
> Oops. That should be easy enough to fix.
> 
> 
>> 2).  mem_paging_xen_pfec_page_paged.patch:
>>
>> 15 +    if ( p2m_is_paging(p2mt) )
>>  16 +    {
>>  17 +//        if ( p2m_is_paged(p2mt) )
>>
>> I think you can safely remove the commented out code.
> 
> Yes we can. I thought I had removed all of the lines like that.
> 
> 
>> 3). mem_sharing_xen_mm.patch:
>>
>>  27 +    /*
>>  28 +     * Initialise our DOMID_IO domain.
>>                                 ^^->COW
>>  29 +     * This domain owns sharable pages.
>>  30 +     */
>>  31 +    dom_cow = domain_create(DOMID_COW, DOMCRF_dummy, 0);
>>
>>
>> 4), mem_sharing_tools_eagain.patch
>>
>> The code spins with a 1 second timeout forever. Would it make sense
>> to include a retry counter so that, say after an hour (or maybe something
>> much smaller) you give up?
> 
> I'll defer these to Grzegorz.
> 
> 
> Patrick
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

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

Fix error in the copyright for mem_event.c.

Signed-off-by: Patrick Colp <Patrick.Colp@citrix.com>

diff -r 750a177eeca5 xen/arch/x86/mm/mem_event.c
--- a/xen/arch/x86/mm/mem_event.c	Thu Dec 17 16:09:19 2009 +0000
+++ b/xen/arch/x86/mm/mem_event.c	Thu Dec 17 09:17:11 2009 -0800
@@ -3,7 +3,7 @@
  *
  * Memory event support.
  *
- * Copyright (c) 2009 Citrix (R)&D) Ltd. (Patrick Colp)
+ * Copyright (c) 2009 Citrix (R&D) Ltd. (Patrick Colp)
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by

[-- Attachment #3: mem_paging_pfec_fix.patch --]
[-- Type: text/x-patch, Size: 1054 bytes --]

Remove commented out p2m paging type check code from guest_walk.c.

Signed-off-by: Patrick Colp <Patrick.Colp@citrix.com>

diff -r 568cd42c9156 xen/arch/x86/mm/hap/guest_walk.c
--- a/xen/arch/x86/mm/hap/guest_walk.c	Thu Dec 17 09:17:12 2009 -0800
+++ b/xen/arch/x86/mm/hap/guest_walk.c	Thu Dec 17 09:21:56 2009 -0800
@@ -49,8 +49,7 @@
     top_mfn = gfn_to_mfn_unshare(v->domain, cr3 >> PAGE_SHIFT, &p2mt, 0);
     if ( p2m_is_paging(p2mt) )
     {
-//        if ( p2m_is_paged(p2mt) )
-            p2m_mem_paging_populate(v->domain, cr3 >> PAGE_SHIFT);
+        p2m_mem_paging_populate(v->domain, cr3 >> PAGE_SHIFT);
 
         pfec[0] = PFEC_page_paged;
         return INVALID_GFN;
@@ -82,8 +81,7 @@
         gfn_to_mfn_unshare(v->domain, gfn_x(gfn), &p2mt, 0);
         if ( p2m_is_paging(p2mt) )
         {
-//            if ( p2m_is_paged(p2mt) )
-                p2m_mem_paging_populate(v->domain, gfn_x(gfn));
+            p2m_mem_paging_populate(v->domain, gfn_x(gfn));
 
             pfec[0] = PFEC_page_paged;
             return INVALID_GFN;

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

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

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

[Konrad Rzeszutek Wilk]

On Thu, Dec 17, 2009 at 04:59:58PM +0000, Jan Beulich wrote:
> >>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 17.12.09 17:38 >>>
> >1). The  "*mfnp |= 0x80000000U;" and "*mfnp |= 0xf0000000U;" should
> >    use a #define. Maybe copy over the #defines from the xen tree ?
> 
> Did you find any defines in the tools sources for that? The only place I
> found this condition being checked at all was in xc_map_foreign_pages(),
> where it used hard-coded values. Or are you referring to the
> XEN_DOMCTL_PFINFO_* values? I'd say they're being mis-used when

Yes, those are the ones that I've gotten it from. Granted, as you said
they aren't wide enough for this. And it does look a bit unhealthy
to be writting those values in. I was thinking that that mmu.c should
probably have some code for this too to check if those MFNs
are no good.

> applied to the mfn array used by mmap-batch (including apparent
> pre-existing uses).

I am not that familiar with the grant driver to make a good judgment
on that. But I do think that upstream Linux folks would gag on this code
as "hacky".

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

[Jeremy Fitzhardinge]

On 12/17/2009 07:59 AM, Keir Fraser wrote:
> Well, maybe I jumped the gun a bit. But I think this feature is inevitable
> -- everyone wants it -- so it's a choice between fixing up in tree versus
> out of tree. Perhaps it would have been better to get at least the first
> round of fixes done before check in.
>    

It might have been an excellent time to use - gasp - a branch.

     J

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

[Jeremy Fitzhardinge]

On 12/16/2009 03:14 PM, Grzegorz Milos wrote:
> 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.
>    

I'm getting compile errors:

gcc  -O1 -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -g -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wno-unused-value -Wdeclaration-after-statement  -D__XEN_TOOLS__ -MMD -MF .bidir-daemon.o.d  -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE  -Werror -Wno-unused -I../include -I../../tools/libxc -I../../tools/include -D_GNU_SOURCE -fPIC -g -Wp,-MD,.bidir-daemon.o.d -c -o bidir-daemon.o bidir-daemon.c
cc1: warnings being treated as errors
bidir-daemon.c: In function ‘bidir_daemon’:
bidir-daemon.c:74: error: implicit declaration of function ‘sleep’
make[3]: *** [bidir-daemon.o] Error 1
make[3]: Leaving directory `/home/jeremy/hg/xen/unstable/tools/memshr'

and

make[3]: Entering directory `/home/jeremy/hg/xen/unstable/tools/xenpaging'
gcc  -O1 -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -g -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wno-unused-value -Wdeclaration-after-statement  -D__XEN_TOOLS__ -MMD -MF .file_ops.o.d  -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE  -I ../../tools/python/xen/lowlevel/xc -I ./ -I../../tools/libxc -I../../tools/include -I../../tools/xenstore -I../../tools/include -Werror -Wno-unused -g -Wp,-MD,.file_ops.o.d -c -o file_ops.o file_ops.c
gcc  -O1 -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -g -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wno-unused-value -Wdeclaration-after-statement  -D__XEN_TOOLS__ -MMD -MF .xc.o.d  -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE  -I ../../tools/python/xen/lowlevel/xc -I ./ -I../../tools/libxc -I../../tools/include -I../../tools/xenstore -I../../tools/include -Werror -Wno-unused -g -Wp,-MD,.xc.o.d -c -o xc.o xc.c
In file included from xc.c:29:
xc.h:29:27: error: xen/mem_event.h: No such file or directory
make[3]: *** [xc.o] Error 1
make[3]: Leaving directory `/home/jeremy/hg/xen/unstable/tools/xenpaging'


The first is easily fixed by adding #include <unistd.h>, but I haven't 
looked into the second yet.

Thanks,
J

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

[Patrick Colp]

That's odd. I just pulled a fresh copy of xen-unstable and built xen and 
the tools. Both compiled with no errors or problems. Maybe this is some 
sort of distro-dependent problem?


Patrick


Jeremy Fitzhardinge wrote:
> On 12/16/2009 03:14 PM, Grzegorz Milos wrote:
>> 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.
>>    
> 
> I'm getting compile errors:
> 
> gcc  -O1 -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -g 
> -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes 
> -Wno-unused-value -Wdeclaration-after-statement  -D__XEN_TOOLS__ -MMD 
> -MF .bidir-daemon.o.d  -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE  
> -Werror -Wno-unused -I../include -I../../tools/libxc 
> -I../../tools/include -D_GNU_SOURCE -fPIC -g -Wp,-MD,.bidir-daemon.o.d 
> -c -o bidir-daemon.o bidir-daemon.c
> cc1: warnings being treated as errors
> bidir-daemon.c: In function ‘bidir_daemon’:
> bidir-daemon.c:74: error: implicit declaration of function ‘sleep’
> make[3]: *** [bidir-daemon.o] Error 1
> make[3]: Leaving directory `/home/jeremy/hg/xen/unstable/tools/memshr'
> 
> and
> 
> make[3]: Entering directory `/home/jeremy/hg/xen/unstable/tools/xenpaging'
> gcc  -O1 -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -g 
> -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes 
> -Wno-unused-value -Wdeclaration-after-statement  -D__XEN_TOOLS__ -MMD 
> -MF .file_ops.o.d  -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE  -I 
> ../../tools/python/xen/lowlevel/xc -I ./ -I../../tools/libxc 
> -I../../tools/include -I../../tools/xenstore -I../../tools/include 
> -Werror -Wno-unused -g -Wp,-MD,.file_ops.o.d -c -o file_ops.o file_ops.c
> gcc  -O1 -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -g 
> -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes 
> -Wno-unused-value -Wdeclaration-after-statement  -D__XEN_TOOLS__ -MMD 
> -MF .xc.o.d  -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE  -I 
> ../../tools/python/xen/lowlevel/xc -I ./ -I../../tools/libxc 
> -I../../tools/include -I../../tools/xenstore -I../../tools/include 
> -Werror -Wno-unused -g -Wp,-MD,.xc.o.d -c -o xc.o xc.c
> In file included from xc.c:29:
> xc.h:29:27: error: xen/mem_event.h: No such file or directory
> make[3]: *** [xc.o] Error 1
> make[3]: Leaving directory `/home/jeremy/hg/xen/unstable/tools/xenpaging'
> 
> 
> The first is easily fixed by adding #include <unistd.h>, but I haven't 
> looked into the second yet.
> 
> Thanks,
> J

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

[Jeremy Fitzhardinge]

On 12/17/2009 12:08 PM, Patrick Colp wrote:
> That's odd. I just pulled a fresh copy of xen-unstable and built xen 
> and the tools. Both compiled with no errors or problems. Maybe this is 
> some sort of distro-dependent problem?

Possibly.  I'm using Fedora 11 on this machine, and it also fails in 
Fedora 12.  For the sleep() prototype, is it possible your versions 
headers are implicitly including unistd.h, or otherwise have a prototype 
for it?

And for the xen/mem_event.h problem, might you have that header 
installed somewhere?

     J

>
>
> Patrick
>
>
> Jeremy Fitzhardinge wrote:
>> On 12/16/2009 03:14 PM, Grzegorz Milos wrote:
>>> 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.
>>
>> I'm getting compile errors:
>>
>> gcc  -O1 -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -g 
>> -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes 
>> -Wno-unused-value -Wdeclaration-after-statement  -D__XEN_TOOLS__ -MMD 
>> -MF .bidir-daemon.o.d  -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE  
>> -Werror -Wno-unused -I../include -I../../tools/libxc 
>> -I../../tools/include -D_GNU_SOURCE -fPIC -g 
>> -Wp,-MD,.bidir-daemon.o.d -c -o bidir-daemon.o bidir-daemon.c
>> cc1: warnings being treated as errors
>> bidir-daemon.c: In function ‘bidir_daemon’:
>> bidir-daemon.c:74: error: implicit declaration of function ‘sleep’
>> make[3]: *** [bidir-daemon.o] Error 1
>> make[3]: Leaving directory `/home/jeremy/hg/xen/unstable/tools/memshr'
>>
>> and
>>
>> make[3]: Entering directory 
>> `/home/jeremy/hg/xen/unstable/tools/xenpaging'
>> gcc  -O1 -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -g 
>> -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes 
>> -Wno-unused-value -Wdeclaration-after-statement  -D__XEN_TOOLS__ -MMD 
>> -MF .file_ops.o.d  -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE  -I 
>> ../../tools/python/xen/lowlevel/xc -I ./ -I../../tools/libxc 
>> -I../../tools/include -I../../tools/xenstore -I../../tools/include 
>> -Werror -Wno-unused -g -Wp,-MD,.file_ops.o.d -c -o file_ops.o file_ops.c
>> gcc  -O1 -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -g 
>> -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes 
>> -Wno-unused-value -Wdeclaration-after-statement  -D__XEN_TOOLS__ -MMD 
>> -MF .xc.o.d  -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE  -I 
>> ../../tools/python/xen/lowlevel/xc -I ./ -I../../tools/libxc 
>> -I../../tools/include -I../../tools/xenstore -I../../tools/include 
>> -Werror -Wno-unused -g -Wp,-MD,.xc.o.d -c -o xc.o xc.c
>> In file included from xc.c:29:
>> xc.h:29:27: error: xen/mem_event.h: No such file or directory
>> make[3]: *** [xc.o] Error 1
>> make[3]: Leaving directory 
>> `/home/jeremy/hg/xen/unstable/tools/xenpaging'
>>
>>
>> The first is easily fixed by adding #include <unistd.h>, but I 
>> haven't looked into the second yet.
>>
>> Thanks,
>> J
>

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

[Jeremy Fitzhardinge]

On 12/17/2009 12:15 PM, Jeremy Fitzhardinge wrote:
> On 12/17/2009 12:08 PM, Patrick Colp wrote:
>> That's odd. I just pulled a fresh copy of xen-unstable and built xen 
>> and the tools. Both compiled with no errors or problems. Maybe this 
>> is some sort of distro-dependent problem?
>
> Possibly.  I'm using Fedora 11 on this machine, and it also fails in 
> Fedora 12.  For the sleep() prototype, is it possible your versions 
> headers are implicitly including unistd.h, or otherwise have a 
> prototype for it?
>
> And for the xen/mem_event.h problem, might you have that header 
> installed somewhere?

Though I just did a clean+build and it found the header, so maybe it was 
something in the header symlink mess.

So just the sleep prototype problem.

     J

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

[Patrick Colp]

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

OK. I've attached a patch to fix the include problem. I'll leave the 
xen/mem_event.h problem for now unless other people have it too.


Patrick


Jeremy Fitzhardinge wrote:
> On 12/17/2009 12:15 PM, Jeremy Fitzhardinge wrote:
>> On 12/17/2009 12:08 PM, Patrick Colp wrote:
>>> That's odd. I just pulled a fresh copy of xen-unstable and built xen 
>>> and the tools. Both compiled with no errors or problems. Maybe this 
>>> is some sort of distro-dependent problem?
>>
>> Possibly.  I'm using Fedora 11 on this machine, and it also fails in 
>> Fedora 12.  For the sleep() prototype, is it possible your versions 
>> headers are implicitly including unistd.h, or otherwise have a 
>> prototype for it?
>>
>> And for the xen/mem_event.h problem, might you have that header 
>> installed somewhere?
> 
> Though I just did a clean+build and it found the header, so maybe it was 
> something in the header symlink mess.
> 
> So just the sleep prototype problem.
> 
>     J

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

Add missing unistd.h header file which causes the build to fail on some
systems.

Signed-off-by: Patrick Colp <Patrick.Colp@citrix.com>

diff -r b47dfc7f5918 tools/memshr/bidir-daemon.c
--- a/tools/memshr/bidir-daemon.c	Thu Dec 17 09:21:56 2009 -0800
+++ b/tools/memshr/bidir-daemon.c	Thu Dec 17 12:19:57 2009 -0800
@@ -18,6 +18,7 @@
  */
 #include <pthread.h>
 #include <inttypes.h>
+#include <unistd.h>
 
 #include "bidir-hash.h"
 #include "memshr-priv.h"

[-- 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] Paging and memory sharing for HVM guests

[Jan Beulich]

>>> Patrick Colp <pjcolp@cs.ubc.ca> 17.12.09 18:05 >>>
>Jan Beulich wrote:
>>>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 17.12.09 17:38 >>>
>>> 1). The  "*mfnp |= 0x80000000U;" and "*mfnp |= 0xf0000000U;" should
>>>    use a #define. Maybe copy over the #defines from the xen tree ?
>> 
>> Did you find any defines in the tools sources for that? The only place I
>> found this condition being checked at all was in xc_map_foreign_pages(),
>> where it used hard-coded values. Or are you referring to the
>> XEN_DOMCTL_PFINFO_* values? I'd say they're being mis-used when
>> applied to the mfn array used by mmap-batch (including apparent
>> pre-existing uses).
>
>I think many people agree that this idea of using 0xf0000000 is a very 
>inelegant solution (myself included). Maybe now would be a good time to 
>hash out what the proper way to deal with this is. I think this discussion 
>should extend to the privcmd/libxc mmap interfaces generally as well.

Yes, I fully agree. While I had cooked together a patch to make the
tools auto-detect whether the underlying (64-bit) kernel uses a
sufficiently wide mask as error indicator (and a Linux side patch to
actually make the kernel behaviorally match the pseudo-documentation
in privcmd.h ("array of mfns - top nibble set on err"), I meanwhile
realized that this would break older tools running on a fixed kernel
(a combination I suppose to be valid).

The only way I can see to solve this is a new ioctl, and if we need a
new ioctl anyway, we can as well design it properly. The question
however is what the best way for an error indication is: Fully over-
writing the passed in MFNs, an extra bit vector passed in (does user
mode have a sufficiently common interface for dealing with bit fields,
like the kernel's bitops.h?), or yet something else?

Jan

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

[Jan Beulich]

>>> 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...

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?

Jan

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

[Patrick Colp]

[-- 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

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

[Patrick Colp]

Jan Beulich wrote:
>>>> Patrick Colp <pjcolp@cs.ubc.ca> 17.12.09 18:05 >>>
>> Jan Beulich wrote:
>>>>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 17.12.09 17:38 >>>
>>>> 1). The  "*mfnp |= 0x80000000U;" and "*mfnp |= 0xf0000000U;" should
>>>>    use a #define. Maybe copy over the #defines from the xen tree ?
>>> Did you find any defines in the tools sources for that? The only place I
>>> found this condition being checked at all was in xc_map_foreign_pages(),
>>> where it used hard-coded values. Or are you referring to the
>>> XEN_DOMCTL_PFINFO_* values? I'd say they're being mis-used when
>>> applied to the mfn array used by mmap-batch (including apparent
>>> pre-existing uses).
>> I think many people agree that this idea of using 0xf0000000 is a very 
>> inelegant solution (myself included). Maybe now would be a good time to 
>> hash out what the proper way to deal with this is. I think this discussion 
>> should extend to the privcmd/libxc mmap interfaces generally as well.
> 
> Yes, I fully agree. While I had cooked together a patch to make the
> tools auto-detect whether the underlying (64-bit) kernel uses a
> sufficiently wide mask as error indicator (and a Linux side patch to
> actually make the kernel behaviorally match the pseudo-documentation
> in privcmd.h ("array of mfns - top nibble set on err"), I meanwhile
> realized that this would break older tools running on a fixed kernel
> (a combination I suppose to be valid).
> 
> The only way I can see to solve this is a new ioctl, and if we need a
> new ioctl anyway, we can as well design it properly. The question
> however is what the best way for an error indication is: Fully over-
> writing the passed in MFNs, an extra bit vector passed in (does user
> mode have a sufficiently common interface for dealing with bit fields,
> like the kernel's bitops.h?), or yet something else?

A new ioctl seems like the reasonable approach. And as you, say if we're 
going to have a new ioctl, then let's do it right. When calling 
xc_map_foreign_batch(), is there any requirement that the pfns you pass in 
are contiguous (ordering incrementally)? If not, then I think completely 
over-writing the MFNs is probably the wrong thing to do, as it requires 
callers to keep two copies of the array to find out which pages didn't map. 
I would be more in favour of returning a bit vector. As far as I know, 
there is currently no common userland bitops.h. However, in my paging tool 
I have one, so it is possible to have it. We could include it with libxc 
(or in some other common library).


Patrick

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

[Dan Magenheimer]

> Does live migration work (reliably) when pages have been
> swapped out?

Never got an answer on this.  If the answer is NO or
not yet (and not likely in time for 4.0), I'd suggest
that a domain that enables page sharing should also
automagically turn on the "no_migrate" flag.

> -----Original Message-----
> From: Dan Magenheimer 
> Sent: Thursday, December 17, 2009 9:20 AM
> To: Grzegorz Milos; xen-devel@lists.xensource.com; Patrick 
> Colp; Andrew
> Peace; Keir Fraser
> Subject: RE: [Xen-devel] [PATCH] Paging and memory sharing for HVM
> guests
> 
> 
> Does live migration work (reliably) when pages have been
> swapped out?  IIRC, this is a limitation of VMware's
> implementation (or at least was a limitation at one
> point).
> 
> > -----Original Message-----
> > From: Grzegorz Milos [mailto:gm281@cam.ac.uk]
> > Sent: Wednesday, December 16, 2009 4:15 PM
> > To: xen-devel@lists.xensource.com; Patrick Colp; Andrew Peace; Keir
> > Fraser
> > Subject: [Xen-devel] [PATCH] Paging and memory sharing for 
> HVM guests
> > 
> > 
> > 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.
> > 
> > Thanks
> > Patrick & Gregor
> >

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

[Konrad Rzeszutek Wilk]

> A new ioctl seems like the reasonable approach. And as you, say if we're  
> going to have a new ioctl, then let's do it right. When calling  
> xc_map_foreign_batch(), is there any requirement that the pfns you pass 
> in are contiguous (ordering incrementally)? If not, then I think 
> completely over-writing the MFNs is probably the wrong thing to do, as it 
> requires callers to keep two copies of the array to find out which pages 
> didn't map. I would be more in favour of returning a bit vector. As far 

I would suggest you look in xen_exchange_memory in the linux kernel. One
of its customers is xen_create_contiguous_region which replaces MFNs
with ones underneath the 4GB.

There is a requirement in making the PFNs contigous but that is b/c
the Xen side loops over it assuming incremental order.

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

[Patrick Colp]

Dan Magenheimer wrote:
>> Does live migration work (reliably) when pages have been
>> swapped out?
> 
> Never got an answer on this.  If the answer is NO or
> not yet (and not likely in time for 4.0), I'd suggest
> that a domain that enables page sharing should also
> automagically turn on the "no_migrate" flag.

This is probably the safe thing to do for now, until further testing can be 
done (and in the case of the swapper, a more mature tool developed).


Patrick

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

[Patrick Colp]

Konrad Rzeszutek Wilk wrote:
>> A new ioctl seems like the reasonable approach. And as you, say if we're  
>> going to have a new ioctl, then let's do it right. When calling  
>> xc_map_foreign_batch(), is there any requirement that the pfns you pass 
>> in are contiguous (ordering incrementally)? If not, then I think 
>> completely over-writing the MFNs is probably the wrong thing to do, as it 
>> requires callers to keep two copies of the array to find out which pages 
>> didn't map. I would be more in favour of returning a bit vector. As far 
> 
> I would suggest you look in xen_exchange_memory in the linux kernel. One
> of its customers is xen_create_contiguous_region which replaces MFNs
> with ones underneath the 4GB.
> 
> There is a requirement in making the PFNs contigous but that is b/c
> the Xen side loops over it assuming incremental order.

If we know that the PFNs have to be contiguous (and we're happy having this 
be a requirement), then completely overwriting the MFNs is probably easier 
than returning a bit vector (and wouldn't require extra support in libxc).


Patrick

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

[Grzegorz Milos]

Live migration is something we've got on our to-check lists. There is
no reason why it shouldn't work properly (albeit slow if many pages
have been swapped out). We'll test and report back :). In the meantime
I vote against setting "no_migrate" on, if only because we'll get more
bug reports , even if it works for us.

Thanks
Gregor

On Fri, Dec 18, 2009 at 6:05 PM, Dan Magenheimer
<dan.magenheimer@oracle.com> wrote:
>> Does live migration work (reliably) when pages have been
>> swapped out?
>
> Never got an answer on this.  If the answer is NO or
> not yet (and not likely in time for 4.0), I'd suggest
> that a domain that enables page sharing should also
> automagically turn on the "no_migrate" flag.
>
>> -----Original Message-----
>> From: Dan Magenheimer
>> Sent: Thursday, December 17, 2009 9:20 AM
>> To: Grzegorz Milos; xen-devel@lists.xensource.com; Patrick
>> Colp; Andrew
>> Peace; Keir Fraser
>> Subject: RE: [Xen-devel] [PATCH] Paging and memory sharing for HVM
>> guests
>>
>>
>> Does live migration work (reliably) when pages have been
>> swapped out?  IIRC, this is a limitation of VMware's
>> implementation (or at least was a limitation at one
>> point).
>>
>> > -----Original Message-----
>> > From: Grzegorz Milos [mailto:gm281@cam.ac.uk]
>> > Sent: Wednesday, December 16, 2009 4:15 PM
>> > To: xen-devel@lists.xensource.com; Patrick Colp; Andrew Peace; Keir
>> > Fraser
>> > Subject: [Xen-devel] [PATCH] Paging and memory sharing for
>> HVM guests
>> >
>> >
>> > 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.
>> >
>> > Thanks
>> > Patrick & Gregor
>> >
>

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

[Dan Magenheimer]

> (albeit slow if many pages have been swapped out)

That's one thing I'm worried about.  Even if the migration
eventually completes, it is my understanding that "bad
things" can happen if a live migration takes too long.

Another thing I'm worried about is a deadlock (or maybe
livelock) if memory is overcommitted, a live migration
is called for of a domain that has much of its memory
swapped out, and there is insufficient memory on the
source machine to swap the migrating domain's memory
back in from disk.

Seems like a lot of corner cases, and I don't think
these are things we want enterprise customers to
discover the hard way.

I'd suggest setting no_migrate=1 for 4.0, and turning
it off in xen-unstable immediately following 4.0.

> -----Original Message-----
> From: Grzegorz Milos [mailto:gm281@cam.ac.uk]
> Sent: Sunday, December 20, 2009 8:04 AM
> To: Dan Magenheimer
> Cc: xen-devel@lists.xensource.com; Patrick Colp; Andrew Peace; Keir
> Fraser
> Subject: Re: [Xen-devel] [PATCH] Paging and memory sharing for HVM
> guests
> 
> 
> Live migration is something we've got on our to-check lists. There is
> no reason why it shouldn't work properly (albeit slow if many pages
> have been swapped out). We'll test and report back :). In the meantime
> I vote against setting "no_migrate" on, if only because we'll get more
> bug reports , even if it works for us.
> 
> Thanks
> Gregor
> 
> On Fri, Dec 18, 2009 at 6:05 PM, Dan Magenheimer
> <dan.magenheimer@oracle.com> wrote:
> >> Does live migration work (reliably) when pages have been
> >> swapped out?
> >
> > Never got an answer on this.  If the answer is NO or
> > not yet (and not likely in time for 4.0), I'd suggest
> > that a domain that enables page sharing should also
> > automagically turn on the "no_migrate" flag.
> >
> >> -----Original Message-----
> >> From: Dan Magenheimer
> >> Sent: Thursday, December 17, 2009 9:20 AM
> >> To: Grzegorz Milos; xen-devel@lists.xensource.com; Patrick
> >> Colp; Andrew
> >> Peace; Keir Fraser
> >> Subject: RE: [Xen-devel] [PATCH] Paging and memory sharing for HVM
> >> guests
> >>
> >>
> >> Does live migration work (reliably) when pages have been
> >> swapped out?  IIRC, this is a limitation of VMware's
> >> implementation (or at least was a limitation at one
> >> point).
> >>
> >> > -----Original Message-----
> >> > From: Grzegorz Milos [mailto:gm281@cam.ac.uk]
> >> > Sent: Wednesday, December 16, 2009 4:15 PM
> >> > To: xen-devel@lists.xensource.com; Patrick Colp; Andrew 
> Peace; Keir
> >> > Fraser
> >> > Subject: [Xen-devel] [PATCH] Paging and memory sharing for
> >> HVM guests
> >> >
> >> >
> >> > 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.
> >> >
> >> > Thanks
> >> > Patrick & Gregor
> >> >
> >
>

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

[Jan Beulich]

>>> Grzegorz Milos <gm281@cam.ac.uk> 17.12.09 00:14 >>>
>--- a/xen/include/public/domctl.h	Tue Dec 15 10:47:36 2009 -0800
>+++ b/xen/include/public/domctl.h	Tue Dec 15 10:47:42 2009 -0800
>@@ -691,6 +691,31 @@
> 
> };
> 
>+/*
>+ * Memory event operations
>+ */
>+
>+#define XEN_DOMCTL_mem_event_op  56
>+
>+/* Add and remove memory handlers */
>+#define XEN_DOMCTL_MEM_EVENT_OP_ENABLE     0
>+#define XEN_DOMCTL_MEM_EVENT_OP_DISABLE    1
>+
>+struct xen_domctl_mem_event_op {
>+    uint32_t       op;           /* XEN_DOMCTL_MEM_EVENT_OP_* */
>+    uint32_t       mode;         /* XEN_DOMCTL_MEM_EVENT_ENABLE_* */
>+
>+    /* OP_ENABLE */
>+    unsigned long  shared_addr;  /* IN:  Virtual address of shared page */
>+    unsigned long  ring_addr;    /* IN:  Virtual address of ring page */
>+
>+    /* Other OPs */
>+    unsigned long  gfn;          /* IN:  gfn of page being operated on */
>+};
>+typedef struct xen_domctl_mem_event_op xen_domctl_mem_event_op_t;
>+DEFINE_XEN_GUEST_HANDLE(xen_domctl_mem_event_op_t);
>+
>+
> struct xen_domctl {
>     uint32_t cmd;
>     uint32_t interface_version; /* XEN_DOMCTL_INTERFACE_VERSION */

The use of (unsigned) long in domctl.h and sysctl.h is invalid, as it breaks
32-bit dom0 on 64-bit hv.

>--- a/xen/include/public/domctl.h	Tue Dec 15 13:05:35 2009 -0800
>+++ b/xen/include/public/domctl.h	Tue Dec 15 13:05:38 2009 -0800
>...
>@@ -103,6 +104,7 @@
>     uint32_t flags;              /* XEN_DOMINF_* */
>     uint64_aligned_t tot_pages;
>     uint64_aligned_t max_pages;
>+    uint64_aligned_t shr_pages;
>     uint64_aligned_t shared_info_frame; /* GMFN of shared_info struct */
>     uint64_aligned_t cpu_time;
>     uint32_t nr_online_vcpus;    /* Number of VCPUs currently online. */

This change is not valid without changing XEN_DOMCTL_INTERFACE_VERSION,
since tools outside the Xen tree (namely, libvirt, which is currently broken
because of this) rely on knowing all (valid) (version, structure-layout) pairs.

>@@ -727,6 +729,52 @@
> typedef struct xen_domctl_mem_event_op xen_domctl_mem_event_op_t;
> DEFINE_XEN_GUEST_HANDLE(xen_domctl_mem_event_op_t);
> 
>+/*
>+ * Memory sharing operations
>+ */
>+#define XEN_DOMCTL_mem_sharing_op  58
>+
>+#define XEN_DOMCTL_MEM_SHARING_OP_CONTROL        0
>+#define XEN_DOMCTL_MEM_SHARING_OP_NOMINATE_GFN   1
>+#define XEN_DOMCTL_MEM_SHARING_OP_NOMINATE_GREF  2
>+#define XEN_DOMCTL_MEM_SHARING_OP_SHARE          3
>+#define XEN_DOMCTL_MEM_SHARING_OP_RESUME         4
>+#define XEN_DOMCTL_MEM_SHARING_OP_DEBUG_GFN      5
>+#define XEN_DOMCTL_MEM_SHARING_OP_DEBUG_MFN      6
>+#define XEN_DOMCTL_MEM_SHARING_OP_DEBUG_GREF     7
>+
>+#define XEN_DOMCTL_MEM_SHARING_S_HANDLE_INVALID  (-10)
>+#define XEN_DOMCTL_MEM_SHARING_C_HANDLE_INVALID  (-9)
>+
>+struct xen_domctl_mem_sharing_op {
>+    uint8_t       op;            /* XEN_DOMCTL_MEM_EVENT_OP_* */
>+
>+    union {
>+        int enable;                       /* for OP_CONTROL            */
>+
>+        struct mem_sharing_op_nominate {  /* for OP_NOMINATE           */
>+            union {
>+                unsigned long gfn;        /* IN: gfn to nominate       */
>+                uint32_t      grant_ref;  /* IN: grant ref to nominate */
>+            };
>+            uint64_t       handle;        /* OUT: the handle           */
>+        } nominate;
>+        struct mem_sharing_op_share {
>+            uint64_t source_handle;       /* IN: handle to the source page */
>+            uint64_t client_handle;       /* IN: handle to the client page */
>+        } share; 
>+        struct mem_sharing_op_debug {
>+            union {
>+                unsigned long  gfn;        /* IN: gfn to debug          */
>+                unsigned long  mfn;        /* IN: mfn to debug          */
>+                grant_ref_t    gref;       /* IN: gref to debug         */
>+            };
>+        } debug;
>+    };
>+};
>+typedef struct xen_domctl_mem_sharing_op xen_domctl_mem_sharing_op_t;
>+DEFINE_XEN_GUEST_HANDLE(xen_domctl_mem_sharing_op_t);
>+
> 
> struct xen_domctl {
>     uint32_t cmd;

Like above, using unsigned long here is invalid. Using int doesn't seem
to be too good an idea either, for portability reasons. I think fixed-size
integer types should be used everywhere.

Jan

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

[Keir Fraser]

On 22/12/2009 10:49, "Jan Beulich" <JBeulich@novell.com> wrote:

> Like above, using unsigned long here is invalid. Using int doesn't seem
> to be too good an idea either, for portability reasons. I think fixed-size
> integer types should be used everywhere.

I've just done a general clean up of domctl/sysctl as changeset 20711.

 -- Keir

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

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 22.12.09 12:34 >>>
>On 22/12/2009 10:49, "Jan Beulich" <JBeulich@novell.com> wrote:
>
>> Like above, using unsigned long here is invalid. Using int doesn't seem
>> to be too good an idea either, for portability reasons. I think fixed-size
>> integer types should be used everywhere.
>
>I've just done a general clean up of domctl/sysctl as changeset 20711.

Thanks!

Any chance you could also sync the public headers over to the
2.6.18 tree now that they should be stable for 4.0?

Jan

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

[Keir Fraser]

On 22/12/2009 12:56, "Jan Beulich" <JBeulich@novell.com> wrote:

>> I've just done a general clean up of domctl/sysctl as changeset 20711.
> 
> Thanks!
> 
> Any chance you could also sync the public headers over to the
> 2.6.18 tree now that they should be stable for 4.0?

Done.

 K.

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

[Jan Beulich]

>>> Grzegorz Milos <gm281@cam.ac.uk> 17.12.09 00:14 >>>
One more issue I see with these changes:

>--- a/drivers/xen/privcmd/privcmd.c
>+++ b/drivers/xen/privcmd/privcmd.c
>...
>@@ -236,8 +237,14 @@ static long privcmd_ioctl(struct file *f
> 		    (m.addr != vma->vm_start) ||
> 		    ((m.addr + (nr_pages << PAGE_SHIFT)) != vma->vm_end) ||
> 		    !privcmd_enforce_singleshot_mapping(vma)) {
>-			up_write(&mm->mmap_sem);
>-			goto mmapbatch_out;
>+			if (!(vma &&
>+			      (m.addr >= vma->vm_start) &&
>+			      ((m.addr + (nr_pages << PAGE_SHIFT)) <= vma->vm_end) &&
>+			      (nr_pages == 1) &&
>+			      !privcmd_enforce_singleshot_mapping(vma))) {
>+				up_write(&mm->mmap_sem);
>+				goto mmapbatch_out;
>+			}
> 		}
> 
> 		p = m.arr;

Isn't this undermining the purpose of privcmd_enforce_singleshot_mapping()?
You don't check that the eventual single page re-mapping attempt is
really due to an earlier -ENOENT failure, and hence the whole single
shot mapping checks are now pointless (though other than possibly to
enforce some minimal security I don't really know what its purpose
is/was - Keir?).

Jan

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

[Keir Fraser]

On 04/01/2010 14:14, "Jan Beulich" <JBeulich@novell.com> wrote:

> Isn't this undermining the purpose of privcmd_enforce_singleshot_mapping()?
> You don't check that the eventual single page re-mapping attempt is
> really due to an earlier -ENOENT failure, and hence the whole single
> shot mapping checks are now pointless (though other than possibly to
> enforce some minimal security I don't really know what its purpose
> is/was - Keir?).

It was just to avoid the BUG_ON(!pte_none(*pte)) in
direct_remap_area_pte_fn(). We trust privcmd users enough we could just get
rid of privcmd_enforce_singleshot_mapping(). We could instead forcibly clear
ptes before calling direct_remap_area(); or just do nothing.

 -- Keir

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

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 04.01.10 16:32 >>>
>On 04/01/2010 14:14, "Jan Beulich" <JBeulich@novell.com> wrote:
>
>> Isn't this undermining the purpose of privcmd_enforce_singleshot_mapping()?
>> You don't check that the eventual single page re-mapping attempt is
>> really due to an earlier -ENOENT failure, and hence the whole single
>> shot mapping checks are now pointless (though other than possibly to
>> enforce some minimal security I don't really know what its purpose
>> is/was - Keir?).
>
>It was just to avoid the BUG_ON(!pte_none(*pte)) in
>direct_remap_area_pte_fn(). We trust privcmd users enough we could just get
>rid of privcmd_enforce_singleshot_mapping(). We could instead forcibly clear
>ptes before calling direct_remap_area(); or just do nothing.

I'm of the opinion that even privileged users shouldn't be able to oops
the kernel, and hence hitting the BUG_ON() you're mentioning should
be avoided. Clearing the old mappings seems like a reasonable thing
to do.

Jan

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

[Jan Beulich]

>>> "Jan Beulich" <JBeulich@novell.com> 04.01.10 17:30 >>>
>>>> Keir Fraser <keir.fraser@eu.citrix.com> 04.01.10 16:32 >>>
>>On 04/01/2010 14:14, "Jan Beulich" <JBeulich@novell.com> wrote:
>>
>>> Isn't this undermining the purpose of privcmd_enforce_singleshot_mapping()?
>>> You don't check that the eventual single page re-mapping attempt is
>>> really due to an earlier -ENOENT failure, and hence the whole single
>>> shot mapping checks are now pointless (though other than possibly to
>>> enforce some minimal security I don't really know what its purpose
>>> is/was - Keir?).
>>
>>It was just to avoid the BUG_ON(!pte_none(*pte)) in
>>direct_remap_area_pte_fn(). We trust privcmd users enough we could just get
>>rid of privcmd_enforce_singleshot_mapping(). We could instead forcibly clear
>>ptes before calling direct_remap_area(); or just do nothing.
>
>I'm of the opinion that even privileged users shouldn't be able to oops
>the kernel, and hence hitting the BUG_ON() you're mentioning should
>be avoided. Clearing the old mappings seems like a reasonable thing
>to do.

Actually, after some more thinking about this it seems to me that the
best approach would probably be a per-page single shot mechanism.
This could simply be done through apply_to_page_range(), checking
for pte_none() in the callback. The old mechanism could be removed
then.

Jan