Re: [Xen-devel] [PATCH v7 04/10] xen: Introduce XEN_DOMCTL_soft_reset
Hi,
At 17:25 +0200 on 27 May (1432747540), Vitaly Kuznetsov wrote:
New operation reassigns all memory pages from source domain to the destination
domain mapping them at exactly the same GFNs. Pages mapped more than once
(e.g.
grants) are being copied.
Signed-off-by: Vitaly Kuznetsov vkuzn...@redhat.com
---
Changes in v7:
- is_soft_reset flag added to struct domain to preserve destination domain's
paused state across possible hypercall preemption.
[Jan Beulich]
- XENMEM_soft_reset - XEN_DOMCTL_soft_reset
- domain_soft_reset() returns int
- no locking for is_dying as it is now proteced by domctl_lock
- print a warning on !mfn_valid case
- check PGC_allocated for pages
- no print on assign_pages failure (it prints error messages on both its
failure paths)
- don't re-read page-count_info, use copy_page flag
- minor stucturing change
- pause both source and destination domain while processing the hypercall
- remove nr_transferred from public interface
[Tim Deegan]
- use get_gfn_type_access() in PoD case (x86-only)
---
xen/common/domain.c | 186
xen/common/domctl.c | 72 +
xen/include/public/domctl.h | 28 +++
xen/include/xen/sched.h | 6 ++
4 files changed, 292 insertions(+)
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 7825c56..824f325 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1006,6 +1006,192 @@ int domain_unpause_by_systemcontroller(struct domain
*d)
return 0;
}
+int domain_soft_reset(struct domain *source_d, struct domain *dest_d,
+ xen_pfn_t *gmfn_start)
+{
+int rc = 0;
+unsigned long mfn, mfn_new, gmfn, last_gmfn, count;
+unsigned int order;
+p2m_type_t p2mt;
+struct page_info *page, *new_page, *next_page;
+int drop_dom_ref, copy_page;
+
+last_gmfn = domain_get_maximum_gpfn(source_d);
+gmfn = *gmfn_start;
+while ( gmfn = last_gmfn )
+{
+page = get_page_from_gfn(source_d, gmfn, p2mt, 0);
In order to be safe against p2m changes, you should use
get_gfn_type_access() _here_, and put_gfn() when you're finished with the
gfn. You'll also need to get_page() the returned MFN, of course, to
make sure that it isn't freed before you reassign it.
+if ( unlikely(page == NULL) )
+{
+#ifdef CONFIG_X86
+struct p2m_domain *p2m = p2m_get_hostp2m(source_d);
+p2m_access_t p2ma;
+mfn_t mfn_p2m;
+
+order = 0;
+mfn_p2m = get_gfn_type_access(p2m, gmfn,
+ p2mt, p2ma, 0, order);
+if ( p2m_is_pod(p2mt) )
+{
+rc = guest_physmap_mark_populate_on_demand(dest_d, gmfn,
+ order);
+if ( unlikely(rc) )
+{
+printk(XENLOG_G_ERR Failed to mark Dom%d's GFN %lx
+(order: %d) as PoD\n, source_d-domain_id,
+ gmfn, order);
+goto fail;
+}
+}
+put_gfn(source_d, gmfn);
+gmfn += 1ul order;
+#else
+gmfn++;
+#endif
+goto preempt_check;
+}
+
+mfn = page_to_mfn(page);
+if ( unlikely(!mfn_valid(mfn)) )
+{
+printk(XENLOG_G_WARNING Dom%d's GFN %lx points to invalid
MFN\n,
+ source_d-domain_id, gmfn);
+put_page(page);
+gmfn++;
+goto preempt_check;
+}
+
+next_page = page;
+count = 0;
+copy_page = 0;
+while ( next_page !is_xen_heap_page(next_page)
+page_to_mfn(next_page) == mfn + count )
What's the purpose of this second loop? It doesn't seem to be doing
anything that the outer loop couldn't.
Cheers,
Tim.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7 04/10] xen: Introduce XEN_DOMCTL_soft_reset
Tim Deegan t...@xen.org writes:
Hi,
At 17:25 +0200 on 27 May (1432747540), Vitaly Kuznetsov wrote:
New operation reassigns all memory pages from source domain to the
destination
domain mapping them at exactly the same GFNs. Pages mapped more than once
(e.g.
grants) are being copied.
Signed-off-by: Vitaly Kuznetsov vkuzn...@redhat.com
---
Changes in v7:
- is_soft_reset flag added to struct domain to preserve destination domain's
paused state across possible hypercall preemption.
[Jan Beulich]
- XENMEM_soft_reset - XEN_DOMCTL_soft_reset
- domain_soft_reset() returns int
- no locking for is_dying as it is now proteced by domctl_lock
- print a warning on !mfn_valid case
- check PGC_allocated for pages
- no print on assign_pages failure (it prints error messages on both its
failure paths)
- don't re-read page-count_info, use copy_page flag
- minor stucturing change
- pause both source and destination domain while processing the hypercall
- remove nr_transferred from public interface
[Tim Deegan]
- use get_gfn_type_access() in PoD case (x86-only)
---
xen/common/domain.c | 186
xen/common/domctl.c | 72 +
xen/include/public/domctl.h | 28 +++
xen/include/xen/sched.h | 6 ++
4 files changed, 292 insertions(+)
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 7825c56..824f325 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1006,6 +1006,192 @@ int domain_unpause_by_systemcontroller(struct domain
*d)
return 0;
}
+int domain_soft_reset(struct domain *source_d, struct domain *dest_d,
+ xen_pfn_t *gmfn_start)
+{
+int rc = 0;
+unsigned long mfn, mfn_new, gmfn, last_gmfn, count;
+unsigned int order;
+p2m_type_t p2mt;
+struct page_info *page, *new_page, *next_page;
+int drop_dom_ref, copy_page;
+
+last_gmfn = domain_get_maximum_gpfn(source_d);
+gmfn = *gmfn_start;
+while ( gmfn = last_gmfn )
+{
+page = get_page_from_gfn(source_d, gmfn, p2mt, 0);
In order to be safe against p2m changes, you should use
get_gfn_type_access() _here_, and put_gfn() when you're finished with the
gfn. You'll also need to get_page() the returned MFN, of course, to
make sure that it isn't freed before you reassign it.
The only problem I see is the fact that get_gfn_type_access() is
x86-specific. Actually, I don't see why we can't have
get_gfn_type_access() for ARM: it locks the whole p2m with gfn_lock
(which is just p2m_lock() on x86) and then resolves the gfn. I'm not
sure what should be the right approach for this series: make this
hypercall x86-specific (temporary before we have all the required bits
in ARM) or try to make something up for ARM...
+if ( unlikely(page == NULL) )
+{
+#ifdef CONFIG_X86
+struct p2m_domain *p2m = p2m_get_hostp2m(source_d);
+p2m_access_t p2ma;
+mfn_t mfn_p2m;
+
+order = 0;
+mfn_p2m = get_gfn_type_access(p2m, gmfn,
+ p2mt, p2ma, 0, order);
+if ( p2m_is_pod(p2mt) )
+{
+rc = guest_physmap_mark_populate_on_demand(dest_d, gmfn,
+ order);
+if ( unlikely(rc) )
+{
+printk(XENLOG_G_ERR Failed to mark Dom%d's GFN %lx
+(order: %d) as PoD\n, source_d-domain_id,
+ gmfn, order);
+goto fail;
+}
+}
+put_gfn(source_d, gmfn);
+gmfn += 1ul order;
+#else
+gmfn++;
+#endif
+goto preempt_check;
+}
+
+mfn = page_to_mfn(page);
+if ( unlikely(!mfn_valid(mfn)) )
+{
+printk(XENLOG_G_WARNING Dom%d's GFN %lx points to invalid
MFN\n,
+ source_d-domain_id, gmfn);
+put_page(page);
+gmfn++;
+goto preempt_check;
+}
+
+next_page = page;
+count = 0;
+copy_page = 0;
+while ( next_page !is_xen_heap_page(next_page)
+page_to_mfn(next_page) == mfn + count )
What's the purpose of this second loop? It doesn't seem to be doing
anything that the outer loop couldn't.
True. This second loops searches for a continuous region to preserve the
order of mappings (when possible) but as it advances the gmfn it doesn't
bring any performance penalty. I was under an impression it makes this
code easier to read but if you think it doesn't I won't object.
Thanks,
--
Vitaly
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7 04/10] xen: Introduce XEN_DOMCTL_soft_reset
Tim Deegan t...@xen.org writes:
At 13:56 +0200 on 28 May (1432821360), Vitaly Kuznetsov wrote:
Tim Deegan t...@xen.org writes:
+last_gmfn = domain_get_maximum_gpfn(source_d);
+gmfn = *gmfn_start;
+while ( gmfn = last_gmfn )
+{
+page = get_page_from_gfn(source_d, gmfn, p2mt, 0);
In order to be safe against p2m changes, you should use
get_gfn_type_access() _here_, and put_gfn() when you're finished with the
gfn. You'll also need to get_page() the returned MFN, of course, to
make sure that it isn't freed before you reassign it.
The only problem I see is the fact that get_gfn_type_access() is
x86-specific. Actually, I don't see why we can't have
get_gfn_type_access() for ARM: it locks the whole p2m with gfn_lock
(which is just p2m_lock() on x86) and then resolves the gfn. I'm not
sure what should be the right approach for this series: make this
hypercall x86-specific (temporary before we have all the required bits
in ARM) or try to make something up for ARM...
I think it would be best to add this call to ARM.
+while ( next_page !is_xen_heap_page(next_page)
+page_to_mfn(next_page) == mfn + count )
What's the purpose of this second loop? It doesn't seem to be doing
anything that the outer loop couldn't.
True. This second loops searches for a continuous region to preserve the
order of mappings (when possible)
Ah; I think this, like the PoD case, should the more detailed p2m
lookup to get the actual order of the current mapping. That should be
shorter and more readable, and probably more correct.
If we bring get_gfn_type_access() call to the top level it becomes
possible (and easy) but (if I'm not mistaken) we still need to walk
through all pages of the mapping checking that each of them has the
reqiuired count_info (so it is not mapped from other domain or xen
itself). In case we meet a 'bad' page we'll have to split the mapping
(and copy the page itself).
--
Vitaly
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7 04/10] xen: Introduce XEN_DOMCTL_soft_reset
At 13:56 +0200 on 28 May (1432821360), Vitaly Kuznetsov wrote:
Tim Deegan t...@xen.org writes:
+last_gmfn = domain_get_maximum_gpfn(source_d);
+gmfn = *gmfn_start;
+while ( gmfn = last_gmfn )
+{
+page = get_page_from_gfn(source_d, gmfn, p2mt, 0);
In order to be safe against p2m changes, you should use
get_gfn_type_access() _here_, and put_gfn() when you're finished with the
gfn. You'll also need to get_page() the returned MFN, of course, to
make sure that it isn't freed before you reassign it.
The only problem I see is the fact that get_gfn_type_access() is
x86-specific. Actually, I don't see why we can't have
get_gfn_type_access() for ARM: it locks the whole p2m with gfn_lock
(which is just p2m_lock() on x86) and then resolves the gfn. I'm not
sure what should be the right approach for this series: make this
hypercall x86-specific (temporary before we have all the required bits
in ARM) or try to make something up for ARM...
I think it would be best to add this call to ARM.
+while ( next_page !is_xen_heap_page(next_page)
+page_to_mfn(next_page) == mfn + count )
What's the purpose of this second loop? It doesn't seem to be doing
anything that the outer loop couldn't.
True. This second loops searches for a continuous region to preserve the
order of mappings (when possible)
Ah; I think this, like the PoD case, should the more detailed p2m
lookup to get the actual order of the current mapping. That should be
shorter and more readable, and probably more correct.
Cheers,
Tim.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7 04/10] xen: Introduce XEN_DOMCTL_soft_reset
At 15:11 +0200 on 28 May (1432825919), Vitaly Kuznetsov wrote: Tim Deegan t...@xen.org writes: At 13:56 +0200 on 28 May (1432821360), Vitaly Kuznetsov wrote: Tim Deegan t...@xen.org writes: +while ( next_page !is_xen_heap_page(next_page) +page_to_mfn(next_page) == mfn + count ) What's the purpose of this second loop? It doesn't seem to be doing anything that the outer loop couldn't. True. This second loops searches for a continuous region to preserve the order of mappings (when possible) Ah; I think this, like the PoD case, should the more detailed p2m lookup to get the actual order of the current mapping. That should be shorter and more readable, and probably more correct. If we bring get_gfn_type_access() call to the top level it becomes possible (and easy) but (if I'm not mistaken) we still need to walk through all pages of the mapping checking that each of them has the reqiuired count_info (so it is not mapped from other domain or xen itself). In case we meet a 'bad' page we'll have to split the mapping (and copy the page itself). Hmmm. Yes, we can only reassign one page at a time. I think that will look cleaner if you split out the reassign-or-copy into a separate function that takes a start + order and DTRT, and then having the loop in this function handle one p2m entry (of whatever order) per iteration. BTW having looked at how messy this is ending up, and how it's still incomplete, I'd agree with Jan that resetting the domain state might be a better approach. Tim. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7 04/10] xen: Introduce XEN_DOMCTL_soft_reset
Tim Deegan t...@xen.org writes: At 15:11 +0200 on 28 May (1432825919), Vitaly Kuznetsov wrote: Tim Deegan t...@xen.org writes: At 13:56 +0200 on 28 May (1432821360), Vitaly Kuznetsov wrote: Tim Deegan t...@xen.org writes: +while ( next_page !is_xen_heap_page(next_page) +page_to_mfn(next_page) == mfn + count ) What's the purpose of this second loop? It doesn't seem to be doing anything that the outer loop couldn't. True. This second loops searches for a continuous region to preserve the order of mappings (when possible) Ah; I think this, like the PoD case, should the more detailed p2m lookup to get the actual order of the current mapping. That should be shorter and more readable, and probably more correct. If we bring get_gfn_type_access() call to the top level it becomes possible (and easy) but (if I'm not mistaken) we still need to walk through all pages of the mapping checking that each of them has the reqiuired count_info (so it is not mapped from other domain or xen itself). In case we meet a 'bad' page we'll have to split the mapping (and copy the page itself). Hmmm. Yes, we can only reassign one page at a time. I think that will look cleaner if you split out the reassign-or-copy into a separate function that takes a start + order and DTRT, and then having the loop in this function handle one p2m entry (of whatever order) per iteration. BTW having looked at how messy this is ending up, and how it's still incomplete, I'd agree with Jan that resetting the domain state might be a better approach. Even with the 'reset-everything' approach the function from this patch will still be required in some form as we'll still have to walk the p2m and each individual page checking it's count_info and replacing in case of need. At the same time we'll have lots of other hypervisor-related implications (tearing down everything) so I seriously doubt it's going to end up less messy (toolstack-related changes might go away though). -- Vitaly ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7 04/10] xen: Introduce XEN_DOMCTL_soft_reset
On 28.05.15 at 15:53, vkuzn...@redhat.com wrote: Tim Deegan t...@xen.org writes: BTW having looked at how messy this is ending up, and how it's still incomplete, I'd agree with Jan that resetting the domain state might be a better approach. Even with the 'reset-everything' approach the function from this patch will still be required in some form as we'll still have to walk the p2m and each individual page checking it's count_info and replacing in case of need. At the same time we'll have lots of other hypervisor-related implications (tearing down everything) so I seriously doubt it's going to end up less messy (toolstack-related changes might go away though). That would remain to be seen. I'm particularly unconvinced that iterating over all pages an copying is going to be needed: Iirc it was only grants mapped by another domain that we really need to hunt down, and we should be able to do that without iterating over the whole GFN space of the domain (or all of its assigned pages). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
