Re: [Xen-devel] [PATCH v8 08/11] xen: arch-specific hooks for domain_soft_reset()
Hi Vitaly,
On 16/07/2015 13:36, Vitaly Kuznetsov wrote:
Julien Grall julien.gr...@citrix.com writes:
On 23/06/2015 18:11, Vitaly Kuznetsov wrote:
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 24b8938..c112afa 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -653,6 +653,11 @@ void arch_domain_unpause(struct domain *d)
{
}
+int arch_domain_soft_reset(struct domain *d)
+{
+return 0;
I suspect that it would needs some code in this function in order to
support soft reset on ARM. If so, should not you return -ENOSYS until
someone take care of the ARM support?
I was hoping kexec for ARM will magically work without any arch-specific
bits here :-)
I don't know how work kexec for Xen, but I think we will have at least
to do the same as x86 for the shared info page.
Sure, let's make it -ENOSYS.
Thank you!
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 08/11] xen: arch-specific hooks for domain_soft_reset()
Julien Grall julien.gr...@citrix.com writes:
Hi Vitaly,
On 23/06/2015 18:11, Vitaly Kuznetsov wrote:
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 24b8938..c112afa 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -653,6 +653,11 @@ void arch_domain_unpause(struct domain *d)
{
}
+int arch_domain_soft_reset(struct domain *d)
+{
+return 0;
I suspect that it would needs some code in this function in order to
support soft reset on ARM. If so, should not you return -ENOSYS until
someone take care of the ARM support?
I was hoping kexec for ARM will magically work without any arch-specific
bits here :-)
Sure, let's make it -ENOSYS.
+}
+
Regards,
--
Vitaly
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 08/11] xen: arch-specific hooks for domain_soft_reset()
Hi Vitaly,
On 23/06/2015 18:11, Vitaly Kuznetsov wrote:
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 24b8938..c112afa 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -653,6 +653,11 @@ void arch_domain_unpause(struct domain *d)
{
}
+int arch_domain_soft_reset(struct domain *d)
+{
+return 0;
I suspect that it would needs some code in this function in order to
support soft reset on ARM. If so, should not you return -ENOSYS until
someone take care of the ARM support?
+}
+
Regards,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 08/11] xen: arch-specific hooks for domain_soft_reset()
On Tue, Jul 14, 2015 at 05:52:44PM +0200, Vitaly Kuznetsov wrote:
Konrad Rzeszutek Wilk konrad.w...@oracle.com writes:
On Tue, Jun 23, 2015 at 06:11:50PM +0200, Vitaly Kuznetsov wrote:
...
+int arch_domain_soft_reset(struct domain *d)
+{
+struct page_info *page = virt_to_page(d-shared_info), *new_page;
+int ret = 0;
+struct domain *owner;
+unsigned long mfn, mfn_new, gfn;
+p2m_type_t p2mt;
+unsigned int i;
+
+/* Soft reset is supported for HVM domains only */
Missing full stop. But perhaps we could explain what would be needed
for an PV guest to make it work (not as something for you to do
of course but an victim^H^H^Hvolunteer!).
Oh, I seriously doubt we'll ever be doing soft reset for classic PV. I
don't see an easy way to preserve existent memory and without this soft
reset is pointless. PVH (or PVHVM-without-dm) looks much more
promising.
+if ( !is_hvm_domain(d) )
+return -EINVAL;
I suggest we leave it here with the comment above and decide something
later based on the chosen path for PVH.
+
+hvm_destroy_all_ioreq_servers(d);
+
+spin_lock(d-event_lock);
+for ( i = 1; i d-nr_pirqs ; i ++ )
Is pirq 0 special?
No, for some reason I though it is not a valid number for pirq. Will fix
in v9.
+if ( owner != d )
+goto exit_put_page;
+
+mfn = page_to_mfn(page);
+if ( !mfn_valid(mfn) )
+{
+printk(XENLOG_G_ERR Dom%d's shared_info page points to invalid
MFN\n,
+ d-domain_id);
Would it be good to print out the PFN of the shared page to help narrow the
cause?
I think this case is impossibe under normal circumstances and it's not
clear to me how to get the PFN (did you mean GFN?) in such case.
Yes.
shared_info is always allocated in arch_domain_create() from Xen heap
and page_to_mfn() will never return INVALID_MFN here. In case we'll ever
see this printed we'll have examine why this is not true anymore. This
piece of code will have to be updated.
Ok, One way it could be if the guest decided to expunge this GFN fro
the guest (I think). Thought I am not sure why it would do such a thing :-)
+if ( ret )
+printk(XENLOG_G_ERR Failed to add a page to replace
+Dom%d's shared_info frame %lx\n, d-domain_id, gfn);
Should we free the new_page in this case?
The new page is already in domain's page_list so we won't lose it on
domain destroy but there is also no point in keeping it there if we
failed to add it to physmap. Will free it in v9.
Thanks!
--
Vitaly
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 08/11] xen: arch-specific hooks for domain_soft_reset()
Konrad Rzeszutek Wilk konrad.w...@oracle.com writes:
On Tue, Jul 14, 2015 at 05:52:44PM +0200, Vitaly Kuznetsov wrote:
Konrad Rzeszutek Wilk konrad.w...@oracle.com writes:
On Tue, Jun 23, 2015 at 06:11:50PM +0200, Vitaly Kuznetsov wrote:
...
+int arch_domain_soft_reset(struct domain *d)
+{
+struct page_info *page = virt_to_page(d-shared_info), *new_page;
+int ret = 0;
+struct domain *owner;
+unsigned long mfn, mfn_new, gfn;
+p2m_type_t p2mt;
+unsigned int i;
+
...
+/* Soft reset is supported for HVM domains only */
+if ( owner != d )
+goto exit_put_page;
+
+mfn = page_to_mfn(page);
+if ( !mfn_valid(mfn) )
+{
+printk(XENLOG_G_ERR Dom%d's shared_info page points to invalid
MFN\n,
+ d-domain_id);
Would it be good to print out the PFN of the shared page to help narrow
the cause?
I think this case is impossibe under normal circumstances and it's not
clear to me how to get the PFN (did you mean GFN?) in such case.
Yes.
shared_info is always allocated in arch_domain_create() from Xen heap
and page_to_mfn() will never return INVALID_MFN here. In case we'll ever
see this printed we'll have examine why this is not true anymore. This
piece of code will have to be updated.
Ok, One way it could be if the guest decided to expunge this GFN fro
the guest (I think). Thought I am not sure why it would do such a thing :-)
I'm not sure as we do page_to_mfn(virt_to_page(d-shared_info)) here and
d-shared_info is a hypervisor-s va, guest domain can't do anything to
it.
--
Vitaly
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 08/11] xen: arch-specific hooks for domain_soft_reset()
On Tue, Jun 23, 2015 at 06:11:50PM +0200, Vitaly Kuznetsov wrote:
x86-specific hook cleans up the pirq-emuirq mappings, destroys all ioreq
servers and and replaces the shared_info frame with an empty page to support
subsequent XENMAPSPACE_shared_info call.
ARM-specific hook is a noop for now.
Signed-off-by: Vitaly Kuznetsov vkuzn...@redhat.com
---
Changes since 'PATCH RFC' of the 'reset everything' approach to PVHVM guest
kexec:
- Coding style, check get_gfn_query() return value, various minor fixes [Jan
Beulich]
- Do not unpause VCPUs on arch hook failure [Jan Beulich]
---
xen/arch/arm/domain.c | 5 +++
xen/arch/x86/domain.c | 79
+++
xen/arch/x86/hvm/hvm.c| 2 +-
xen/common/domain.c | 5 +++
xen/include/asm-x86/hvm/hvm.h | 2 ++
xen/include/xen/domain.h | 2 ++
6 files changed, 94 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 24b8938..c112afa 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -653,6 +653,11 @@ void arch_domain_unpause(struct domain *d)
{
}
+int arch_domain_soft_reset(struct domain *d)
+{
+return 0;
+}
+
static int is_guest_pv32_psr(uint32_t psr)
{
switch (psr PSR_MODE_MASK)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 0363650..2dd0e0d 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -682,6 +682,85 @@ void arch_domain_unpause(struct domain *d)
viridian_time_ref_count_thaw(d);
}
+int arch_domain_soft_reset(struct domain *d)
+{
+struct page_info *page = virt_to_page(d-shared_info), *new_page;
+int ret = 0;
+struct domain *owner;
+unsigned long mfn, mfn_new, gfn;
+p2m_type_t p2mt;
+unsigned int i;
+
+/* Soft reset is supported for HVM domains only */
Missing full stop. But perhaps we could explain what would be needed
for an PV guest to make it work (not as something for you to do
of course but an victim^H^H^Hvolunteer!).
+if ( !is_hvm_domain(d) )
+return -EINVAL;
+
+hvm_destroy_all_ioreq_servers(d);
+
+spin_lock(d-event_lock);
+for ( i = 1; i d-nr_pirqs ; i ++ )
Is pirq 0 special?
s/i ++/i++/
+if ( domain_pirq_to_emuirq(d, i) != IRQ_UNBOUND )
+{
+ret = unmap_domain_pirq_emuirq(d, i);
+if ( ret )
+break;
+}
Could you add outer {} to the loop please?
+spin_unlock(d-event_lock);
+
+if ( ret )
+return ret;
+
+/*
+ * shared_info page needs to be replaced with a new page, otherwise we
+ * will get a hole if the domain does XENMAPSPACE_shared_info
Full stop missing.
+ */
+
+owner = page_get_owner_and_reference(page);
+/* If shared_info page wasn't mounted, we do not need to replace it */
s/mounted/used/
Missing full stop.
+if ( owner != d )
+goto exit_put_page;
+
+mfn = page_to_mfn(page);
+if ( !mfn_valid(mfn) )
+{
+printk(XENLOG_G_ERR Dom%d's shared_info page points to invalid
MFN\n,
+ d-domain_id);
Would it be good to print out the PFN of the shared page to help narrow the
cause?
+ret = -EINVAL;
+goto exit_put_page;
+}
+
+gfn = mfn_to_gmfn(d, mfn);
+if ( mfn_x(get_gfn_query(d, gfn, p2mt)) == INVALID_MFN )
+{
+printk(XENLOG_G_ERR Failed to get Dom%d's shared_info GFN (%lx)\n,
+ d-domain_id, gfn);
+ret = -EINVAL;
+goto exit_put_page;
+}
+
+new_page = alloc_domheap_page(d, 0);
+if ( !new_page )
+{
+printk(XENLOG_G_ERR Failed to alloc a page to replace
+Dom%d's shared_info frame %lx\n, d-domain_id, gfn);
+ret = -ENOMEM;
+goto exit_put_gfn;
+}
+mfn_new = page_to_mfn(new_page);
+guest_physmap_remove_page(d, gfn, mfn, 0);
s/0/PAGE_ORDER_4K/
+
+ret = guest_physmap_add_page(d, gfn, mfn_new, 0);
s/0/PAGE_ORDER_4K/
+if ( ret )
+printk(XENLOG_G_ERR Failed to add a page to replace
+Dom%d's shared_info frame %lx\n, d-domain_id, gfn);
Should we free the new_page in this case?
+ exit_put_gfn:
+put_gfn(d, gfn);
+ exit_put_page:
+put_page(page);
+
+return ret;
+}
+
unsigned long pv_guest_cr4_fixup(const struct vcpu *v, unsigned long
guest_cr4)
{
unsigned long hv_cr4_mask, hv_cr4 = real_cr4_to_pv_guest_cr4(read_cr4());
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index d5e5242..506a7be 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1319,7 +1319,7 @@ static void hvm_all_ioreq_servers_remove_vcpu(struct
domain *d, struct vcpu *v)
spin_unlock(d-arch.hvm_domain.ioreq_server.lock);
}
-static void hvm_destroy_all_ioreq_servers(struct domain *d)
+void hvm_destroy_all_ioreq_servers(struct domain *d)
{
struct
[Xen-devel] [PATCH v8 08/11] xen: arch-specific hooks for domain_soft_reset()
x86-specific hook cleans up the pirq-emuirq mappings, destroys all ioreq
servers and and replaces the shared_info frame with an empty page to support
subsequent XENMAPSPACE_shared_info call.
ARM-specific hook is a noop for now.
Signed-off-by: Vitaly Kuznetsov vkuzn...@redhat.com
---
Changes since 'PATCH RFC' of the 'reset everything' approach to PVHVM guest
kexec:
- Coding style, check get_gfn_query() return value, various minor fixes [Jan
Beulich]
- Do not unpause VCPUs on arch hook failure [Jan Beulich]
---
xen/arch/arm/domain.c | 5 +++
xen/arch/x86/domain.c | 79 +++
xen/arch/x86/hvm/hvm.c| 2 +-
xen/common/domain.c | 5 +++
xen/include/asm-x86/hvm/hvm.h | 2 ++
xen/include/xen/domain.h | 2 ++
6 files changed, 94 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 24b8938..c112afa 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -653,6 +653,11 @@ void arch_domain_unpause(struct domain *d)
{
}
+int arch_domain_soft_reset(struct domain *d)
+{
+return 0;
+}
+
static int is_guest_pv32_psr(uint32_t psr)
{
switch (psr PSR_MODE_MASK)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 0363650..2dd0e0d 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -682,6 +682,85 @@ void arch_domain_unpause(struct domain *d)
viridian_time_ref_count_thaw(d);
}
+int arch_domain_soft_reset(struct domain *d)
+{
+struct page_info *page = virt_to_page(d-shared_info), *new_page;
+int ret = 0;
+struct domain *owner;
+unsigned long mfn, mfn_new, gfn;
+p2m_type_t p2mt;
+unsigned int i;
+
+/* Soft reset is supported for HVM domains only */
+if ( !is_hvm_domain(d) )
+return -EINVAL;
+
+hvm_destroy_all_ioreq_servers(d);
+
+spin_lock(d-event_lock);
+for ( i = 1; i d-nr_pirqs ; i ++ )
+if ( domain_pirq_to_emuirq(d, i) != IRQ_UNBOUND )
+{
+ret = unmap_domain_pirq_emuirq(d, i);
+if ( ret )
+break;
+}
+spin_unlock(d-event_lock);
+
+if ( ret )
+return ret;
+
+/*
+ * shared_info page needs to be replaced with a new page, otherwise we
+ * will get a hole if the domain does XENMAPSPACE_shared_info
+ */
+
+owner = page_get_owner_and_reference(page);
+/* If shared_info page wasn't mounted, we do not need to replace it */
+if ( owner != d )
+goto exit_put_page;
+
+mfn = page_to_mfn(page);
+if ( !mfn_valid(mfn) )
+{
+printk(XENLOG_G_ERR Dom%d's shared_info page points to invalid MFN\n,
+ d-domain_id);
+ret = -EINVAL;
+goto exit_put_page;
+}
+
+gfn = mfn_to_gmfn(d, mfn);
+if ( mfn_x(get_gfn_query(d, gfn, p2mt)) == INVALID_MFN )
+{
+printk(XENLOG_G_ERR Failed to get Dom%d's shared_info GFN (%lx)\n,
+ d-domain_id, gfn);
+ret = -EINVAL;
+goto exit_put_page;
+}
+
+new_page = alloc_domheap_page(d, 0);
+if ( !new_page )
+{
+printk(XENLOG_G_ERR Failed to alloc a page to replace
+Dom%d's shared_info frame %lx\n, d-domain_id, gfn);
+ret = -ENOMEM;
+goto exit_put_gfn;
+}
+mfn_new = page_to_mfn(new_page);
+guest_physmap_remove_page(d, gfn, mfn, 0);
+
+ret = guest_physmap_add_page(d, gfn, mfn_new, 0);
+if ( ret )
+printk(XENLOG_G_ERR Failed to add a page to replace
+Dom%d's shared_info frame %lx\n, d-domain_id, gfn);
+ exit_put_gfn:
+put_gfn(d, gfn);
+ exit_put_page:
+put_page(page);
+
+return ret;
+}
+
unsigned long pv_guest_cr4_fixup(const struct vcpu *v, unsigned long guest_cr4)
{
unsigned long hv_cr4_mask, hv_cr4 = real_cr4_to_pv_guest_cr4(read_cr4());
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index d5e5242..506a7be 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1319,7 +1319,7 @@ static void hvm_all_ioreq_servers_remove_vcpu(struct
domain *d, struct vcpu *v)
spin_unlock(d-arch.hvm_domain.ioreq_server.lock);
}
-static void hvm_destroy_all_ioreq_servers(struct domain *d)
+void hvm_destroy_all_ioreq_servers(struct domain *d)
{
struct hvm_ioreq_server *s, *next;
diff --git a/xen/common/domain.c b/xen/common/domain.c
index ade80ff..cd5ed42 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1013,6 +1013,7 @@ int domain_unpause_by_systemcontroller(struct domain *d)
int domain_soft_reset(struct domain *d)
{
struct vcpu *v;
+int ret;
for_each_vcpu ( d, v )
if ( !v-paused_for_shutdown )
@@ -1025,6 +1026,10 @@ int domain_soft_reset(struct domain *d)
for_each_vcpu ( d, v )
unmap_vcpu_info(v);
+ret = arch_domain_soft_reset(d);
+if (ret)
+return ret;
+
domain_resume(d);
return 0;
diff --git a/xen/include/asm-x86/hvm/hvm.h
