* [PATCH] Fix save/restore for HVM domains with viridian=1
@ 2011-11-25 14:50 Paul Durrant
2011-11-25 14:58 ` Jan Beulich
2011-11-25 15:00 ` Tim Deegan
0 siblings, 2 replies; 14+ messages in thread
From: Paul Durrant @ 2011-11-25 14:50 UTC (permalink / raw)
To: xen-devel; +Cc: paul.durrant
# HG changeset patch
# User Paul Durrant <paul.durrant@citrix.com>
# Date 1322232651 0
# Node ID a3c5e87c73a991861fcdd9a5a1eb8ebc635a2d09
# Parent 0a0c02a616768bfab16c072788cb76be1893c37f
Fix save/restore for HVM domains with viridian=1
xc_domain_save/restore currently pay no attention to HVM_PARAM_VIRIDIAN which
results in an HVM domain running a recent version on Windows (post-Vista)
locking up on a domain restore due to EOIs (done via a viridian MSR write)
being silently dropped.
This patch adds an extra save entry for the viridian parameter and also
adds code in the viridian kernel module to catch attempted use of viridian
functionality when the HVM parameter has not been set.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
diff -r 0a0c02a61676 -r a3c5e87c73a9 tools/libxc/xc_domain_restore.c
--- a/tools/libxc/xc_domain_restore.c Mon Nov 21 21:28:34 2011 +0000
+++ b/tools/libxc/xc_domain_restore.c Fri Nov 25 14:50:51 2011 +0000
@@ -675,6 +675,7 @@ typedef struct {
uint64_t vm86_tss;
uint64_t console_pfn;
uint64_t acpi_ioport_location;
+ uint64_t viridian;
} pagebuf_t;
static int pagebuf_init(pagebuf_t* buf)
@@ -809,6 +810,16 @@ static int pagebuf_get_one(xc_interface
}
return pagebuf_get_one(xch, ctx, buf, fd, dom);
+ case XC_SAVE_ID_HVM_VIRIDIAN:
+ /* Skip padding 4 bytes then read the acpi ioport location. */
+ if ( RDEXACT(fd, &buf->viridian, sizeof(uint32_t)) ||
+ RDEXACT(fd, &buf->viridian, sizeof(uint64_t)) )
+ {
+ PERROR("error read the viridian flag");
+ return -1;
+ }
+ return pagebuf_get_one(xch, ctx, buf, fd, dom);
+
default:
if ( (count > MAX_BATCH_SIZE) || (count < 0) ) {
ERROR("Max batch size exceeded (%d). Giving up.", count);
@@ -1440,6 +1451,9 @@ int xc_domain_restore(xc_interface *xch,
fcntl(io_fd, F_SETFL, orig_io_fd_flags | O_NONBLOCK);
}
+ if (pagebuf.viridian != 0)
+ xc_set_hvm_param(xch, dom, HVM_PARAM_VIRIDIAN, 1);
+
if (pagebuf.acpi_ioport_location == 1) {
DBGPRINTF("Use new firmware ioport from the checkpoint\n");
xc_set_hvm_param(xch, dom, HVM_PARAM_ACPI_IOPORTS_LOCATION, 1);
diff -r 0a0c02a61676 -r a3c5e87c73a9 tools/libxc/xc_domain_save.c
--- a/tools/libxc/xc_domain_save.c Mon Nov 21 21:28:34 2011 +0000
+++ b/tools/libxc/xc_domain_save.c Fri Nov 25 14:50:51 2011 +0000
@@ -1506,6 +1506,18 @@ int xc_domain_save(xc_interface *xch, in
PERROR("Error when writing the firmware ioport version");
goto out;
}
+
+ chunk.id = XC_SAVE_ID_HVM_VIRIDIAN;
+ chunk.data = 0;
+ xc_get_hvm_param(xch, dom, HVM_PARAM_VIRIDIAN,
+ (unsigned long *)&chunk.data);
+
+ if ( (chunk.data != 0) &&
+ wrexact(io_fd, &chunk, sizeof(chunk)) )
+ {
+ PERROR("Error when writing the viridian flag");
+ goto out;
+ }
}
if ( !callbacks->checkpoint )
diff -r 0a0c02a61676 -r a3c5e87c73a9 tools/libxc/xg_save_restore.h
--- a/tools/libxc/xg_save_restore.h Mon Nov 21 21:28:34 2011 +0000
+++ b/tools/libxc/xg_save_restore.h Fri Nov 25 14:50:51 2011 +0000
@@ -134,6 +134,7 @@
#define XC_SAVE_ID_HVM_CONSOLE_PFN -8 /* (HVM-only) */
#define XC_SAVE_ID_LAST_CHECKPOINT -9 /* Commit to restoring after completion of current iteration. */
#define XC_SAVE_ID_HVM_ACPI_IOPORTS_LOCATION -10
+#define XC_SAVE_ID_HVM_VIRIDIAN -11
/*
** We process save/restore/migrate in batches of pages; the below
diff -r 0a0c02a61676 -r a3c5e87c73a9 xen/arch/x86/hvm/viridian.c
--- a/xen/arch/x86/hvm/viridian.c Mon Nov 21 21:28:34 2011 +0000
+++ b/xen/arch/x86/hvm/viridian.c Fri Nov 25 14:50:51 2011 +0000
@@ -206,8 +206,11 @@ int wrmsr_viridian_regs(uint32_t idx, ui
struct vcpu *v = current;
struct domain *d = v->domain;
- if ( !is_viridian_domain(d) )
+ if ( !is_viridian_domain(d) ) {
+ gdprintk(XENLOG_WARNING, "%s: %d not a viridian domain\n", __func__,
+ d->domain_id);
return 0;
+ }
switch ( idx )
{
@@ -271,8 +274,11 @@ int rdmsr_viridian_regs(uint32_t idx, ui
struct vcpu *v = current;
struct domain *d = v->domain;
- if ( !is_viridian_domain(d) )
+ if ( !is_viridian_domain(d) ) {
+ gdprintk(XENLOG_WARNING, "%s: %d not a viridian domain\n", __func__,
+ d->domain_id);
return 0;
+ }
switch ( idx )
{
@@ -411,6 +417,8 @@ static int viridian_load_domain_ctxt(str
if ( hvm_load_entry(VIRIDIAN_DOMAIN, h, &ctxt) != 0 )
return -EINVAL;
+ ASSERT(is_viridian_domain(d));
+
d->arch.hvm_domain.viridian.hypercall_gpa.raw = ctxt.hypercall_gpa;
d->arch.hvm_domain.viridian.guest_os_id.raw = ctxt.guest_os_id;
@@ -455,6 +463,8 @@ static int viridian_load_vcpu_ctxt(struc
if ( hvm_load_entry(VIRIDIAN_VCPU, h, &ctxt) != 0 )
return -EINVAL;
+ ASSERT(is_viridian_domain(d));
+
v->arch.hvm_vcpu.viridian.apic_assist.raw = ctxt.apic_assist;
return 0;
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] Fix save/restore for HVM domains with viridian=1
2011-11-25 14:50 [PATCH] Fix save/restore for HVM domains with viridian=1 Paul Durrant
@ 2011-11-25 14:58 ` Jan Beulich
2011-11-25 15:16 ` Keir Fraser
2011-11-25 15:00 ` Tim Deegan
1 sibling, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2011-11-25 14:58 UTC (permalink / raw)
To: paul.durrant; +Cc: xen-devel
>>> On 25.11.11 at 15:50, Paul Durrant <paul.durrant@citrix.com> wrote:
> # HG changeset patch
> # User Paul Durrant <paul.durrant@citrix.com>
> # Date 1322232651 0
> # Node ID a3c5e87c73a991861fcdd9a5a1eb8ebc635a2d09
> # Parent 0a0c02a616768bfab16c072788cb76be1893c37f
> Fix save/restore for HVM domains with viridian=1
>
> xc_domain_save/restore currently pay no attention to HVM_PARAM_VIRIDIAN
> which
> results in an HVM domain running a recent version on Windows (post-Vista)
> locking up on a domain restore due to EOIs (done via a viridian MSR write)
> being silently dropped.
> This patch adds an extra save entry for the viridian parameter and also
> adds code in the viridian kernel module to catch attempted use of viridian
> functionality when the HVM parameter has not been set.
Assuming this means the changes to {rd,wr}msr_viridian_regs(), isn't
this going to needlessly spam the log? I.e. printing this just once (per
VM) would seem to suffice. Or alternatively I would think that
HVM_DBG_LOG() would be a better choice here.
Jan
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>
> diff -r 0a0c02a61676 -r a3c5e87c73a9 tools/libxc/xc_domain_restore.c
> --- a/tools/libxc/xc_domain_restore.c Mon Nov 21 21:28:34 2011 +0000
> +++ b/tools/libxc/xc_domain_restore.c Fri Nov 25 14:50:51 2011 +0000
> @@ -675,6 +675,7 @@ typedef struct {
> uint64_t vm86_tss;
> uint64_t console_pfn;
> uint64_t acpi_ioport_location;
> + uint64_t viridian;
> } pagebuf_t;
>
> static int pagebuf_init(pagebuf_t* buf)
> @@ -809,6 +810,16 @@ static int pagebuf_get_one(xc_interface
> }
> return pagebuf_get_one(xch, ctx, buf, fd, dom);
>
> + case XC_SAVE_ID_HVM_VIRIDIAN:
> + /* Skip padding 4 bytes then read the acpi ioport location. */
> + if ( RDEXACT(fd, &buf->viridian, sizeof(uint32_t)) ||
> + RDEXACT(fd, &buf->viridian, sizeof(uint64_t)) )
> + {
> + PERROR("error read the viridian flag");
> + return -1;
> + }
> + return pagebuf_get_one(xch, ctx, buf, fd, dom);
> +
> default:
> if ( (count > MAX_BATCH_SIZE) || (count < 0) ) {
> ERROR("Max batch size exceeded (%d). Giving up.", count);
> @@ -1440,6 +1451,9 @@ int xc_domain_restore(xc_interface *xch,
> fcntl(io_fd, F_SETFL, orig_io_fd_flags | O_NONBLOCK);
> }
>
> + if (pagebuf.viridian != 0)
> + xc_set_hvm_param(xch, dom, HVM_PARAM_VIRIDIAN, 1);
> +
> if (pagebuf.acpi_ioport_location == 1) {
> DBGPRINTF("Use new firmware ioport from the checkpoint\n");
> xc_set_hvm_param(xch, dom, HVM_PARAM_ACPI_IOPORTS_LOCATION, 1);
> diff -r 0a0c02a61676 -r a3c5e87c73a9 tools/libxc/xc_domain_save.c
> --- a/tools/libxc/xc_domain_save.c Mon Nov 21 21:28:34 2011 +0000
> +++ b/tools/libxc/xc_domain_save.c Fri Nov 25 14:50:51 2011 +0000
> @@ -1506,6 +1506,18 @@ int xc_domain_save(xc_interface *xch, in
> PERROR("Error when writing the firmware ioport version");
> goto out;
> }
> +
> + chunk.id = XC_SAVE_ID_HVM_VIRIDIAN;
> + chunk.data = 0;
> + xc_get_hvm_param(xch, dom, HVM_PARAM_VIRIDIAN,
> + (unsigned long *)&chunk.data);
> +
> + if ( (chunk.data != 0) &&
> + wrexact(io_fd, &chunk, sizeof(chunk)) )
> + {
> + PERROR("Error when writing the viridian flag");
> + goto out;
> + }
> }
>
> if ( !callbacks->checkpoint )
> diff -r 0a0c02a61676 -r a3c5e87c73a9 tools/libxc/xg_save_restore.h
> --- a/tools/libxc/xg_save_restore.h Mon Nov 21 21:28:34 2011 +0000
> +++ b/tools/libxc/xg_save_restore.h Fri Nov 25 14:50:51 2011 +0000
> @@ -134,6 +134,7 @@
> #define XC_SAVE_ID_HVM_CONSOLE_PFN -8 /* (HVM-only) */
> #define XC_SAVE_ID_LAST_CHECKPOINT -9 /* Commit to restoring after
> completion of current iteration. */
> #define XC_SAVE_ID_HVM_ACPI_IOPORTS_LOCATION -10
> +#define XC_SAVE_ID_HVM_VIRIDIAN -11
>
> /*
> ** We process save/restore/migrate in batches of pages; the below
> diff -r 0a0c02a61676 -r a3c5e87c73a9 xen/arch/x86/hvm/viridian.c
> --- a/xen/arch/x86/hvm/viridian.c Mon Nov 21 21:28:34 2011 +0000
> +++ b/xen/arch/x86/hvm/viridian.c Fri Nov 25 14:50:51 2011 +0000
> @@ -206,8 +206,11 @@ int wrmsr_viridian_regs(uint32_t idx, ui
> struct vcpu *v = current;
> struct domain *d = v->domain;
>
> - if ( !is_viridian_domain(d) )
> + if ( !is_viridian_domain(d) ) {
> + gdprintk(XENLOG_WARNING, "%s: %d not a viridian domain\n",
> __func__,
> + d->domain_id);
> return 0;
> + }
>
> switch ( idx )
> {
> @@ -271,8 +274,11 @@ int rdmsr_viridian_regs(uint32_t idx, ui
> struct vcpu *v = current;
> struct domain *d = v->domain;
>
> - if ( !is_viridian_domain(d) )
> + if ( !is_viridian_domain(d) ) {
> + gdprintk(XENLOG_WARNING, "%s: %d not a viridian domain\n",
> __func__,
> + d->domain_id);
> return 0;
> + }
>
> switch ( idx )
> {
> @@ -411,6 +417,8 @@ static int viridian_load_domain_ctxt(str
> if ( hvm_load_entry(VIRIDIAN_DOMAIN, h, &ctxt) != 0 )
> return -EINVAL;
>
> + ASSERT(is_viridian_domain(d));
> +
> d->arch.hvm_domain.viridian.hypercall_gpa.raw = ctxt.hypercall_gpa;
> d->arch.hvm_domain.viridian.guest_os_id.raw = ctxt.guest_os_id;
>
> @@ -455,6 +463,8 @@ static int viridian_load_vcpu_ctxt(struc
> if ( hvm_load_entry(VIRIDIAN_VCPU, h, &ctxt) != 0 )
> return -EINVAL;
>
> + ASSERT(is_viridian_domain(d));
> +
> v->arch.hvm_vcpu.viridian.apic_assist.raw = ctxt.apic_assist;
>
> return 0;
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] Fix save/restore for HVM domains with viridian=1
2011-11-25 14:58 ` Jan Beulich
@ 2011-11-25 15:16 ` Keir Fraser
0 siblings, 0 replies; 14+ messages in thread
From: Keir Fraser @ 2011-11-25 15:16 UTC (permalink / raw)
To: Jan Beulich, paul.durrant; +Cc: xen-devel
On 25/11/2011 14:58, "Jan Beulich" <JBeulich@suse.com> wrote:
>> xc_domain_save/restore currently pay no attention to HVM_PARAM_VIRIDIAN
>> which
>> results in an HVM domain running a recent version on Windows (post-Vista)
>> locking up on a domain restore due to EOIs (done via a viridian MSR write)
>> being silently dropped.
>> This patch adds an extra save entry for the viridian parameter and also
>> adds code in the viridian kernel module to catch attempted use of viridian
>> functionality when the HVM parameter has not been set.
>
> Assuming this means the changes to {rd,wr}msr_viridian_regs(), isn't
> this going to needlessly spam the log? I.e. printing this just once (per
> VM) would seem to suffice. Or alternatively I would think that
> HVM_DBG_LOG() would be a better choice here.
It's unlikely to print more than once before the VM wedges. We could go
further and inject #GP, which we ought to be doing anyway.
-- Keir
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Fix save/restore for HVM domains with viridian=1
2011-11-25 14:50 [PATCH] Fix save/restore for HVM domains with viridian=1 Paul Durrant
2011-11-25 14:58 ` Jan Beulich
@ 2011-11-25 15:00 ` Tim Deegan
2011-11-25 15:18 ` Keir Fraser
1 sibling, 1 reply; 14+ messages in thread
From: Tim Deegan @ 2011-11-25 15:00 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel
At 14:50 +0000 on 25 Nov (1322232658), Paul Durrant wrote:
> # HG changeset patch
> # User Paul Durrant <paul.durrant@citrix.com>
> # Date 1322232651 0
> # Node ID a3c5e87c73a991861fcdd9a5a1eb8ebc635a2d09
> # Parent 0a0c02a616768bfab16c072788cb76be1893c37f
> Fix save/restore for HVM domains with viridian=1
>
> xc_domain_save/restore currently pay no attention to HVM_PARAM_VIRIDIAN which
> results in an HVM domain running a recent version on Windows (post-Vista)
> locking up on a domain restore due to EOIs (done via a viridian MSR write)
> being silently dropped.
> This patch adds an extra save entry for the viridian parameter and also
> adds code in the viridian kernel module to catch attempted use of viridian
> functionality when the HVM parameter has not been set.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>
> diff -r 0a0c02a61676 -r a3c5e87c73a9 tools/libxc/xc_domain_restore.c
> --- a/tools/libxc/xc_domain_restore.c Mon Nov 21 21:28:34 2011 +0000
> +++ b/tools/libxc/xc_domain_restore.c Fri Nov 25 14:50:51 2011 +0000
> @@ -675,6 +675,7 @@ typedef struct {
> uint64_t vm86_tss;
> uint64_t console_pfn;
> uint64_t acpi_ioport_location;
> + uint64_t viridian;
> } pagebuf_t;
>
> static int pagebuf_init(pagebuf_t* buf)
> @@ -809,6 +810,16 @@ static int pagebuf_get_one(xc_interface
> }
> return pagebuf_get_one(xch, ctx, buf, fd, dom);
>
> + case XC_SAVE_ID_HVM_VIRIDIAN:
> + /* Skip padding 4 bytes then read the acpi ioport location. */
Ahem. ^^^^^^^^^^^^^^^^^^^^
> + if ( RDEXACT(fd, &buf->viridian, sizeof(uint32_t)) ||
> + RDEXACT(fd, &buf->viridian, sizeof(uint64_t)) )
> + {
> + PERROR("error read the viridian flag");
> + return -1;
> + }
> + return pagebuf_get_one(xch, ctx, buf, fd, dom);
> +
> default:
> if ( (count > MAX_BATCH_SIZE) || (count < 0) ) {
> ERROR("Max batch size exceeded (%d). Giving up.", count);
> @@ -1440,6 +1451,9 @@ int xc_domain_restore(xc_interface *xch,
> fcntl(io_fd, F_SETFL, orig_io_fd_flags | O_NONBLOCK);
> }
>
> + if (pagebuf.viridian != 0)
> + xc_set_hvm_param(xch, dom, HVM_PARAM_VIRIDIAN, 1);
> +
> if (pagebuf.acpi_ioport_location == 1) {
> DBGPRINTF("Use new firmware ioport from the checkpoint\n");
> xc_set_hvm_param(xch, dom, HVM_PARAM_ACPI_IOPORTS_LOCATION, 1);
> diff -r 0a0c02a61676 -r a3c5e87c73a9 tools/libxc/xc_domain_save.c
> --- a/tools/libxc/xc_domain_save.c Mon Nov 21 21:28:34 2011 +0000
> +++ b/tools/libxc/xc_domain_save.c Fri Nov 25 14:50:51 2011 +0000
> @@ -1506,6 +1506,18 @@ int xc_domain_save(xc_interface *xch, in
> PERROR("Error when writing the firmware ioport version");
> goto out;
> }
> +
> + chunk.id = XC_SAVE_ID_HVM_VIRIDIAN;
> + chunk.data = 0;
> + xc_get_hvm_param(xch, dom, HVM_PARAM_VIRIDIAN,
> + (unsigned long *)&chunk.data);
> +
> + if ( (chunk.data != 0) &&
> + wrexact(io_fd, &chunk, sizeof(chunk)) )
> + {
> + PERROR("Error when writing the viridian flag");
> + goto out;
> + }
> }
>
> if ( !callbacks->checkpoint )
> diff -r 0a0c02a61676 -r a3c5e87c73a9 tools/libxc/xg_save_restore.h
> --- a/tools/libxc/xg_save_restore.h Mon Nov 21 21:28:34 2011 +0000
> +++ b/tools/libxc/xg_save_restore.h Fri Nov 25 14:50:51 2011 +0000
> @@ -134,6 +134,7 @@
> #define XC_SAVE_ID_HVM_CONSOLE_PFN -8 /* (HVM-only) */
> #define XC_SAVE_ID_LAST_CHECKPOINT -9 /* Commit to restoring after completion of current iteration. */
> #define XC_SAVE_ID_HVM_ACPI_IOPORTS_LOCATION -10
> +#define XC_SAVE_ID_HVM_VIRIDIAN -11
>
> /*
> ** We process save/restore/migrate in batches of pages; the below
> diff -r 0a0c02a61676 -r a3c5e87c73a9 xen/arch/x86/hvm/viridian.c
> --- a/xen/arch/x86/hvm/viridian.c Mon Nov 21 21:28:34 2011 +0000
> +++ b/xen/arch/x86/hvm/viridian.c Fri Nov 25 14:50:51 2011 +0000
> @@ -206,8 +206,11 @@ int wrmsr_viridian_regs(uint32_t idx, ui
> struct vcpu *v = current;
> struct domain *d = v->domain;
>
> - if ( !is_viridian_domain(d) )
> + if ( !is_viridian_domain(d) ) {
> + gdprintk(XENLOG_WARNING, "%s: %d not a viridian domain\n", __func__,
> + d->domain_id);
This should probably be rate-limited; a confused domain could cause a
_lot_ of these. Might we want to pause/kill the domain as well, or
inject a fault?
> return 0;
> + }
>
> switch ( idx )
> {
> @@ -271,8 +274,11 @@ int rdmsr_viridian_regs(uint32_t idx, ui
> struct vcpu *v = current;
> struct domain *d = v->domain;
>
> - if ( !is_viridian_domain(d) )
> + if ( !is_viridian_domain(d) ) {
> + gdprintk(XENLOG_WARNING, "%s: %d not a viridian domain\n", __func__,
> + d->domain_id);
Likewise.
> return 0;
> + }
>
> switch ( idx )
> {
> @@ -411,6 +417,8 @@ static int viridian_load_domain_ctxt(str
> if ( hvm_load_entry(VIRIDIAN_DOMAIN, h, &ctxt) != 0 )
> return -EINVAL;
>
> + ASSERT(is_viridian_domain(d));
> +
I don't think it's appropriate to crash Xen if the save file is bogus.
> d->arch.hvm_domain.viridian.hypercall_gpa.raw = ctxt.hypercall_gpa;
> d->arch.hvm_domain.viridian.guest_os_id.raw = ctxt.guest_os_id;
>
> @@ -455,6 +463,8 @@ static int viridian_load_vcpu_ctxt(struc
> if ( hvm_load_entry(VIRIDIAN_VCPU, h, &ctxt) != 0 )
> return -EINVAL;
>
> + ASSERT(is_viridian_domain(d));
> +
Likewise.
Tim.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] Fix save/restore for HVM domains with viridian=1
2011-11-25 15:00 ` Tim Deegan
@ 2011-11-25 15:18 ` Keir Fraser
2011-11-25 15:26 ` Paul Durrant
0 siblings, 1 reply; 14+ messages in thread
From: Keir Fraser @ 2011-11-25 15:18 UTC (permalink / raw)
To: Tim Deegan, Paul Durrant; +Cc: xen-devel
On 25/11/2011 15:00, "Tim Deegan" <tim@xen.org> wrote:
>> - if ( !is_viridian_domain(d) )
>> + if ( !is_viridian_domain(d) ) {
>> + gdprintk(XENLOG_WARNING, "%s: %d not a viridian domain\n", __func__,
>> + d->domain_id);
>
> This should probably be rate-limited; a confused domain could cause a
> _lot_ of these. Might we want to pause/kill the domain as well, or
> inject a fault?
We do XENLOG_WARNING logging on various paths that silently fail (e.g.,
weird writes to the VIOAPIC). We should probably just #GP, that will silence
the guest pretty quickly. I can make that change.
-- Keir
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] Fix save/restore for HVM domains with viridian=1
2011-11-25 15:18 ` Keir Fraser
@ 2011-11-25 15:26 ` Paul Durrant
0 siblings, 0 replies; 14+ messages in thread
From: Paul Durrant @ 2011-11-25 15:26 UTC (permalink / raw)
To: Keir Fraser, Tim (Xen.org); +Cc: xen-devel@lists.xensource.com
Yes, I forgot about #GP. Windows 7 unfortunately handles and but doesn't complain. I'll ditch the logging.
Paul
> -----Original Message-----
> From: Keir Fraser [mailto:keir.xen@gmail.com]
> Sent: 25 November 2011 15:18
> To: Tim (Xen.org); Paul Durrant
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH] Fix save/restore for HVM domains
> with viridian=1
>
> On 25/11/2011 15:00, "Tim Deegan" <tim@xen.org> wrote:
>
> >> - if ( !is_viridian_domain(d) )
> >> + if ( !is_viridian_domain(d) ) {
> >> + gdprintk(XENLOG_WARNING, "%s: %d not a viridian
> domain\n", __func__,
> >> + d->domain_id);
> >
> > This should probably be rate-limited; a confused domain could
> cause a
> > _lot_ of these. Might we want to pause/kill the domain as well,
> or
> > inject a fault?
>
> We do XENLOG_WARNING logging on various paths that silently fail
> (e.g., weird writes to the VIOAPIC). We should probably just #GP,
> that will silence the guest pretty quickly. I can make that change.
>
> -- Keir
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Fix save/restore for HVM domains with viridian=1
@ 2011-11-25 15:14 Paul Durrant
2011-11-25 15:25 ` Tim Deegan
0 siblings, 1 reply; 14+ messages in thread
From: Paul Durrant @ 2011-11-25 15:14 UTC (permalink / raw)
To: Tim (Xen.org); +Cc: xen-devel@lists.xensource.com
> -----Original Message-----
> From: Tim Deegan [mailto:tim@xen.org]
> Sent: 25 November 2011 15:01
> To: Paul Durrant
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH] Fix save/restore for HVM domains
> with viridian=1
>
> At 14:50 +0000 on 25 Nov (1322232658), Paul Durrant wrote:
[snip]
> >
> > + case XC_SAVE_ID_HVM_VIRIDIAN:
> > + /* Skip padding 4 bytes then read the acpi ioport
> location.
> > + */
>
> Ahem.
> ^^^^^^^^^^^^^^^^^^^^
>
Yes, whoops.
[snip]
> > @@ -206,8 +206,11 @@ int wrmsr_viridian_regs(uint32_t idx, ui
> > struct vcpu *v = current;
> > struct domain *d = v->domain;
> >
> > - if ( !is_viridian_domain(d) )
> > + if ( !is_viridian_domain(d) ) {
> > + gdprintk(XENLOG_WARNING, "%s: %d not a viridian
> domain\n", __func__,
> > + d->domain_id);
>
> This should probably be rate-limited; a confused domain could cause
> a _lot_ of these. Might we want to pause/kill the domain as well,
> or inject a fault?
>
Yes, true.
> > return 0;
> > + }
> >
> > switch ( idx )
> > {
> > @@ -271,8 +274,11 @@ int rdmsr_viridian_regs(uint32_t idx, ui
> > struct vcpu *v = current;
> > struct domain *d = v->domain;
> >
> > - if ( !is_viridian_domain(d) )
> > + if ( !is_viridian_domain(d) ) {
> > + gdprintk(XENLOG_WARNING, "%s: %d not a viridian
> domain\n", __func__,
> > + d->domain_id);
>
> Likewise.
>
> > return 0;
> > + }
> >
> > switch ( idx )
> > {
> > @@ -411,6 +417,8 @@ static int viridian_load_domain_ctxt(str
> > if ( hvm_load_entry(VIRIDIAN_DOMAIN, h, &ctxt) != 0 )
> > return -EINVAL;
> >
> > + ASSERT(is_viridian_domain(d));
> > +
>
> I don't think it's appropriate to crash Xen if the save file is
> bogus.
>
There's a similar ASSERT in the hypercall function anyway; would you rather I turned that into a rate limited warning too?
Paul
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] Fix save/restore for HVM domains with viridian=1
2011-11-25 15:14 Paul Durrant
@ 2011-11-25 15:25 ` Tim Deegan
0 siblings, 0 replies; 14+ messages in thread
From: Tim Deegan @ 2011-11-25 15:25 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel@lists.xensource.com
At 15:14 +0000 on 25 Nov (1322234058), Paul Durrant wrote:
> > > @@ -411,6 +417,8 @@ static int viridian_load_domain_ctxt(str
> > > if ( hvm_load_entry(VIRIDIAN_DOMAIN, h, &ctxt) != 0 )
> > > return -EINVAL;
> > >
> > > + ASSERT(is_viridian_domain(d));
> > > +
> >
> > I don't think it's appropriate to crash Xen if the save file is
> > bogus.
> >
>
> There's a similar ASSERT in the hypercall function anyway; would you rather I turned that into a rate limited warning too?
>
If you mean the one in viridian_hypercall(), no - if that function is
called for a non-viridian VM that's a bug in Xen so the ASSERT() is
correct.
The viridian_load_*_ctxt() functions are called based on the HVM save
record the tools gave us, so we should at most return an error if they
lead us astray. And I think it should be OK to load the HVM state and then
set the HVM params, so the current (lack of) response is correct.
Cheers,
Tim.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] Fix save/restore for HVM domains with viridian=1
@ 2011-11-25 15:28 Paul Durrant
0 siblings, 0 replies; 14+ messages in thread
From: Paul Durrant @ 2011-11-25 15:28 UTC (permalink / raw)
To: xen-devel; +Cc: paul.durrant
# HG changeset patch
# User Paul Durrant <paul.durrant@citrix.com>
# Date 1322234894 0
# Node ID b50479d64deea72b11008fd444aa951737b2573d
# Parent 0a0c02a616768bfab16c072788cb76be1893c37f
Fix save/restore for HVM domains with viridian=1
xc_domain_save/restore currently pay no attention to HVM_PARAM_VIRIDIAN which
results in an HVM domain running a recent version on Windows (post-Vista)
locking up on a domain restore due to EOIs (done via a viridian MSR write)
being silently dropped.
This patch adds an extra save entry for the viridian parameter and also
adds code in the viridian kernel module to catch attempted use of viridian
functionality when the HVM parameter has not been set.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
diff -r 0a0c02a61676 -r b50479d64dee tools/libxc/xc_domain_restore.c
--- a/tools/libxc/xc_domain_restore.c Mon Nov 21 21:28:34 2011 +0000
+++ b/tools/libxc/xc_domain_restore.c Fri Nov 25 15:28:14 2011 +0000
@@ -675,6 +675,7 @@ typedef struct {
uint64_t vm86_tss;
uint64_t console_pfn;
uint64_t acpi_ioport_location;
+ uint64_t viridian;
} pagebuf_t;
static int pagebuf_init(pagebuf_t* buf)
@@ -809,6 +810,16 @@ static int pagebuf_get_one(xc_interface
}
return pagebuf_get_one(xch, ctx, buf, fd, dom);
+ case XC_SAVE_ID_HVM_VIRIDIAN:
+ /* Skip padding 4 bytes then read the viridian flag. */
+ if ( RDEXACT(fd, &buf->viridian, sizeof(uint32_t)) ||
+ RDEXACT(fd, &buf->viridian, sizeof(uint64_t)) )
+ {
+ PERROR("error read the viridian flag");
+ return -1;
+ }
+ return pagebuf_get_one(xch, ctx, buf, fd, dom);
+
default:
if ( (count > MAX_BATCH_SIZE) || (count < 0) ) {
ERROR("Max batch size exceeded (%d). Giving up.", count);
@@ -1440,6 +1451,9 @@ int xc_domain_restore(xc_interface *xch,
fcntl(io_fd, F_SETFL, orig_io_fd_flags | O_NONBLOCK);
}
+ if (pagebuf.viridian != 0)
+ xc_set_hvm_param(xch, dom, HVM_PARAM_VIRIDIAN, 1);
+
if (pagebuf.acpi_ioport_location == 1) {
DBGPRINTF("Use new firmware ioport from the checkpoint\n");
xc_set_hvm_param(xch, dom, HVM_PARAM_ACPI_IOPORTS_LOCATION, 1);
diff -r 0a0c02a61676 -r b50479d64dee tools/libxc/xc_domain_save.c
--- a/tools/libxc/xc_domain_save.c Mon Nov 21 21:28:34 2011 +0000
+++ b/tools/libxc/xc_domain_save.c Fri Nov 25 15:28:14 2011 +0000
@@ -1506,6 +1506,18 @@ int xc_domain_save(xc_interface *xch, in
PERROR("Error when writing the firmware ioport version");
goto out;
}
+
+ chunk.id = XC_SAVE_ID_HVM_VIRIDIAN;
+ chunk.data = 0;
+ xc_get_hvm_param(xch, dom, HVM_PARAM_VIRIDIAN,
+ (unsigned long *)&chunk.data);
+
+ if ( (chunk.data != 0) &&
+ wrexact(io_fd, &chunk, sizeof(chunk)) )
+ {
+ PERROR("Error when writing the viridian flag");
+ goto out;
+ }
}
if ( !callbacks->checkpoint )
diff -r 0a0c02a61676 -r b50479d64dee tools/libxc/xg_save_restore.h
--- a/tools/libxc/xg_save_restore.h Mon Nov 21 21:28:34 2011 +0000
+++ b/tools/libxc/xg_save_restore.h Fri Nov 25 15:28:14 2011 +0000
@@ -134,6 +134,7 @@
#define XC_SAVE_ID_HVM_CONSOLE_PFN -8 /* (HVM-only) */
#define XC_SAVE_ID_LAST_CHECKPOINT -9 /* Commit to restoring after completion of current iteration. */
#define XC_SAVE_ID_HVM_ACPI_IOPORTS_LOCATION -10
+#define XC_SAVE_ID_HVM_VIRIDIAN -11
/*
** We process save/restore/migrate in batches of pages; the below
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH] Fix save/restore for HVM domains with viridian=1
@ 2011-11-25 15:29 Paul Durrant
2011-11-25 15:30 ` Paul Durrant
0 siblings, 1 reply; 14+ messages in thread
From: Paul Durrant @ 2011-11-25 15:29 UTC (permalink / raw)
To: xen-devel; +Cc: paul.durrant
# HG changeset patch
# User Paul Durrant <paul.durrant@citrix.com>
# Date 1322234894 0
# Node ID b50479d64deea72b11008fd444aa951737b2573d
# Parent 0a0c02a616768bfab16c072788cb76be1893c37f
Fix save/restore for HVM domains with viridian=1
xc_domain_save/restore currently pay no attention to HVM_PARAM_VIRIDIAN which
results in an HVM domain running a recent version on Windows (post-Vista)
locking up on a domain restore due to EOIs (done via a viridian MSR write)
being silently dropped.
This patch adds an extra save entry for the viridian parameter and also
adds code in the viridian kernel module to catch attempted use of viridian
functionality when the HVM parameter has not been set.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
diff -r 0a0c02a61676 -r b50479d64dee tools/libxc/xc_domain_restore.c
--- a/tools/libxc/xc_domain_restore.c Mon Nov 21 21:28:34 2011 +0000
+++ b/tools/libxc/xc_domain_restore.c Fri Nov 25 15:28:14 2011 +0000
@@ -675,6 +675,7 @@ typedef struct {
uint64_t vm86_tss;
uint64_t console_pfn;
uint64_t acpi_ioport_location;
+ uint64_t viridian;
} pagebuf_t;
static int pagebuf_init(pagebuf_t* buf)
@@ -809,6 +810,16 @@ static int pagebuf_get_one(xc_interface
}
return pagebuf_get_one(xch, ctx, buf, fd, dom);
+ case XC_SAVE_ID_HVM_VIRIDIAN:
+ /* Skip padding 4 bytes then read the viridian flag. */
+ if ( RDEXACT(fd, &buf->viridian, sizeof(uint32_t)) ||
+ RDEXACT(fd, &buf->viridian, sizeof(uint64_t)) )
+ {
+ PERROR("error read the viridian flag");
+ return -1;
+ }
+ return pagebuf_get_one(xch, ctx, buf, fd, dom);
+
default:
if ( (count > MAX_BATCH_SIZE) || (count < 0) ) {
ERROR("Max batch size exceeded (%d). Giving up.", count);
@@ -1440,6 +1451,9 @@ int xc_domain_restore(xc_interface *xch,
fcntl(io_fd, F_SETFL, orig_io_fd_flags | O_NONBLOCK);
}
+ if (pagebuf.viridian != 0)
+ xc_set_hvm_param(xch, dom, HVM_PARAM_VIRIDIAN, 1);
+
if (pagebuf.acpi_ioport_location == 1) {
DBGPRINTF("Use new firmware ioport from the checkpoint\n");
xc_set_hvm_param(xch, dom, HVM_PARAM_ACPI_IOPORTS_LOCATION, 1);
diff -r 0a0c02a61676 -r b50479d64dee tools/libxc/xc_domain_save.c
--- a/tools/libxc/xc_domain_save.c Mon Nov 21 21:28:34 2011 +0000
+++ b/tools/libxc/xc_domain_save.c Fri Nov 25 15:28:14 2011 +0000
@@ -1506,6 +1506,18 @@ int xc_domain_save(xc_interface *xch, in
PERROR("Error when writing the firmware ioport version");
goto out;
}
+
+ chunk.id = XC_SAVE_ID_HVM_VIRIDIAN;
+ chunk.data = 0;
+ xc_get_hvm_param(xch, dom, HVM_PARAM_VIRIDIAN,
+ (unsigned long *)&chunk.data);
+
+ if ( (chunk.data != 0) &&
+ wrexact(io_fd, &chunk, sizeof(chunk)) )
+ {
+ PERROR("Error when writing the viridian flag");
+ goto out;
+ }
}
if ( !callbacks->checkpoint )
diff -r 0a0c02a61676 -r b50479d64dee tools/libxc/xg_save_restore.h
--- a/tools/libxc/xg_save_restore.h Mon Nov 21 21:28:34 2011 +0000
+++ b/tools/libxc/xg_save_restore.h Fri Nov 25 15:28:14 2011 +0000
@@ -134,6 +134,7 @@
#define XC_SAVE_ID_HVM_CONSOLE_PFN -8 /* (HVM-only) */
#define XC_SAVE_ID_LAST_CHECKPOINT -9 /* Commit to restoring after completion of current iteration. */
#define XC_SAVE_ID_HVM_ACPI_IOPORTS_LOCATION -10
+#define XC_SAVE_ID_HVM_VIRIDIAN -11
/*
** We process save/restore/migrate in batches of pages; the below
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] Fix save/restore for HVM domains with viridian=1
2011-11-25 15:29 Paul Durrant
@ 2011-11-25 15:30 ` Paul Durrant
0 siblings, 0 replies; 14+ messages in thread
From: Paul Durrant @ 2011-11-25 15:30 UTC (permalink / raw)
To: Paul Durrant, xen-devel@lists.xensource.com
Sorry, ignore this one. I need to fix the comment.
Paul
> -----Original Message-----
> From: Paul Durrant [mailto:paul.durrant@citrix.com]
> Sent: 25 November 2011 15:29
> To: xen-devel@lists.xensource.com
> Cc: Paul Durrant
> Subject: [PATCH] Fix save/restore for HVM domains with viridian=1
>
> # HG changeset patch
> # User Paul Durrant <paul.durrant@citrix.com> # Date 1322234894 0 #
> Node ID b50479d64deea72b11008fd444aa951737b2573d
> # Parent 0a0c02a616768bfab16c072788cb76be1893c37f
> Fix save/restore for HVM domains with viridian=1
>
> xc_domain_save/restore currently pay no attention to
> HVM_PARAM_VIRIDIAN which results in an HVM domain running a recent
> version on Windows (post-Vista) locking up on a domain restore due
> to EOIs (done via a viridian MSR write) being silently dropped.
> This patch adds an extra save entry for the viridian parameter and
> also adds code in the viridian kernel module to catch attempted use
> of viridian functionality when the HVM parameter has not been set.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>
> diff -r 0a0c02a61676 -r b50479d64dee tools/libxc/xc_domain_restore.c
> --- a/tools/libxc/xc_domain_restore.c Mon Nov 21 21:28:34 2011
> +0000
> +++ b/tools/libxc/xc_domain_restore.c Fri Nov 25 15:28:14 2011
> +0000
> @@ -675,6 +675,7 @@ typedef struct {
> uint64_t vm86_tss;
> uint64_t console_pfn;
> uint64_t acpi_ioport_location;
> + uint64_t viridian;
> } pagebuf_t;
>
> static int pagebuf_init(pagebuf_t* buf) @@ -809,6 +810,16 @@ static
> int pagebuf_get_one(xc_interface
> }
> return pagebuf_get_one(xch, ctx, buf, fd, dom);
>
> + case XC_SAVE_ID_HVM_VIRIDIAN:
> + /* Skip padding 4 bytes then read the viridian flag. */
> + if ( RDEXACT(fd, &buf->viridian, sizeof(uint32_t)) ||
> + RDEXACT(fd, &buf->viridian, sizeof(uint64_t)) )
> + {
> + PERROR("error read the viridian flag");
> + return -1;
> + }
> + return pagebuf_get_one(xch, ctx, buf, fd, dom);
> +
> default:
> if ( (count > MAX_BATCH_SIZE) || (count < 0) ) {
> ERROR("Max batch size exceeded (%d). Giving up.",
> count); @@ -1440,6 +1451,9 @@ int xc_domain_restore(xc_interface
> *xch,
> fcntl(io_fd, F_SETFL, orig_io_fd_flags | O_NONBLOCK);
> }
>
> + if (pagebuf.viridian != 0)
> + xc_set_hvm_param(xch, dom, HVM_PARAM_VIRIDIAN, 1);
> +
> if (pagebuf.acpi_ioport_location == 1) {
> DBGPRINTF("Use new firmware ioport from the checkpoint\n");
> xc_set_hvm_param(xch, dom, HVM_PARAM_ACPI_IOPORTS_LOCATION,
> 1); diff -r 0a0c02a61676 -r b50479d64dee
> tools/libxc/xc_domain_save.c
> --- a/tools/libxc/xc_domain_save.c Mon Nov 21 21:28:34 2011 +0000
> +++ b/tools/libxc/xc_domain_save.c Fri Nov 25 15:28:14 2011 +0000
> @@ -1506,6 +1506,18 @@ int xc_domain_save(xc_interface *xch, in
> PERROR("Error when writing the firmware ioport
> version");
> goto out;
> }
> +
> + chunk.id = XC_SAVE_ID_HVM_VIRIDIAN;
> + chunk.data = 0;
> + xc_get_hvm_param(xch, dom, HVM_PARAM_VIRIDIAN,
> + (unsigned long *)&chunk.data);
> +
> + if ( (chunk.data != 0) &&
> + wrexact(io_fd, &chunk, sizeof(chunk)) )
> + {
> + PERROR("Error when writing the viridian flag");
> + goto out;
> + }
> }
>
> if ( !callbacks->checkpoint )
> diff -r 0a0c02a61676 -r b50479d64dee tools/libxc/xg_save_restore.h
> --- a/tools/libxc/xg_save_restore.h Mon Nov 21 21:28:34 2011
> +0000
> +++ b/tools/libxc/xg_save_restore.h Fri Nov 25 15:28:14 2011
> +0000
> @@ -134,6 +134,7 @@
> #define XC_SAVE_ID_HVM_CONSOLE_PFN -8 /* (HVM-only) */
> #define XC_SAVE_ID_LAST_CHECKPOINT -9 /* Commit to restoring
> after completion of current iteration. */
> #define XC_SAVE_ID_HVM_ACPI_IOPORTS_LOCATION -10
> +#define XC_SAVE_ID_HVM_VIRIDIAN -11
>
> /*
> ** We process save/restore/migrate in batches of pages; the below
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] Fix save/restore for HVM domains with viridian=1
@ 2011-11-25 15:30 Paul Durrant
2011-11-25 15:39 ` Keir Fraser
0 siblings, 1 reply; 14+ messages in thread
From: Paul Durrant @ 2011-11-25 15:30 UTC (permalink / raw)
To: xen-devel; +Cc: paul.durrant
# HG changeset patch
# User Paul Durrant <paul.durrant@citrix.com>
# Date 1322235040 0
# Node ID 2a8ef49f60cfe5a6c4c9f434af472ab58a125a7a
# Parent 0a0c02a616768bfab16c072788cb76be1893c37f
Fix save/restore for HVM domains with viridian=1
xc_domain_save/restore currently pay no attention to HVM_PARAM_VIRIDIAN which
results in an HVM domain running a recent version on Windows (post-Vista)
locking up on a domain restore due to EOIs (done via a viridian MSR write)
being silently dropped.
This patch adds an extra save entry for the viridian parameter.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
diff -r 0a0c02a61676 -r 2a8ef49f60cf tools/libxc/xc_domain_restore.c
--- a/tools/libxc/xc_domain_restore.c Mon Nov 21 21:28:34 2011 +0000
+++ b/tools/libxc/xc_domain_restore.c Fri Nov 25 15:30:40 2011 +0000
@@ -675,6 +675,7 @@ typedef struct {
uint64_t vm86_tss;
uint64_t console_pfn;
uint64_t acpi_ioport_location;
+ uint64_t viridian;
} pagebuf_t;
static int pagebuf_init(pagebuf_t* buf)
@@ -809,6 +810,16 @@ static int pagebuf_get_one(xc_interface
}
return pagebuf_get_one(xch, ctx, buf, fd, dom);
+ case XC_SAVE_ID_HVM_VIRIDIAN:
+ /* Skip padding 4 bytes then read the viridian flag. */
+ if ( RDEXACT(fd, &buf->viridian, sizeof(uint32_t)) ||
+ RDEXACT(fd, &buf->viridian, sizeof(uint64_t)) )
+ {
+ PERROR("error read the viridian flag");
+ return -1;
+ }
+ return pagebuf_get_one(xch, ctx, buf, fd, dom);
+
default:
if ( (count > MAX_BATCH_SIZE) || (count < 0) ) {
ERROR("Max batch size exceeded (%d). Giving up.", count);
@@ -1440,6 +1451,9 @@ int xc_domain_restore(xc_interface *xch,
fcntl(io_fd, F_SETFL, orig_io_fd_flags | O_NONBLOCK);
}
+ if (pagebuf.viridian != 0)
+ xc_set_hvm_param(xch, dom, HVM_PARAM_VIRIDIAN, 1);
+
if (pagebuf.acpi_ioport_location == 1) {
DBGPRINTF("Use new firmware ioport from the checkpoint\n");
xc_set_hvm_param(xch, dom, HVM_PARAM_ACPI_IOPORTS_LOCATION, 1);
diff -r 0a0c02a61676 -r 2a8ef49f60cf tools/libxc/xc_domain_save.c
--- a/tools/libxc/xc_domain_save.c Mon Nov 21 21:28:34 2011 +0000
+++ b/tools/libxc/xc_domain_save.c Fri Nov 25 15:30:40 2011 +0000
@@ -1506,6 +1506,18 @@ int xc_domain_save(xc_interface *xch, in
PERROR("Error when writing the firmware ioport version");
goto out;
}
+
+ chunk.id = XC_SAVE_ID_HVM_VIRIDIAN;
+ chunk.data = 0;
+ xc_get_hvm_param(xch, dom, HVM_PARAM_VIRIDIAN,
+ (unsigned long *)&chunk.data);
+
+ if ( (chunk.data != 0) &&
+ wrexact(io_fd, &chunk, sizeof(chunk)) )
+ {
+ PERROR("Error when writing the viridian flag");
+ goto out;
+ }
}
if ( !callbacks->checkpoint )
diff -r 0a0c02a61676 -r 2a8ef49f60cf tools/libxc/xg_save_restore.h
--- a/tools/libxc/xg_save_restore.h Mon Nov 21 21:28:34 2011 +0000
+++ b/tools/libxc/xg_save_restore.h Fri Nov 25 15:30:40 2011 +0000
@@ -134,6 +134,7 @@
#define XC_SAVE_ID_HVM_CONSOLE_PFN -8 /* (HVM-only) */
#define XC_SAVE_ID_LAST_CHECKPOINT -9 /* Commit to restoring after completion of current iteration. */
#define XC_SAVE_ID_HVM_ACPI_IOPORTS_LOCATION -10
+#define XC_SAVE_ID_HVM_VIRIDIAN -11
/*
** We process save/restore/migrate in batches of pages; the below
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] Fix save/restore for HVM domains with viridian=1
2011-11-25 15:30 Paul Durrant
@ 2011-11-25 15:39 ` Keir Fraser
2011-11-25 15:42 ` Paul Durrant
0 siblings, 1 reply; 14+ messages in thread
From: Keir Fraser @ 2011-11-25 15:39 UTC (permalink / raw)
To: Paul Durrant, xen-devel
On 25/11/2011 15:30, "Paul Durrant" <paul.durrant@citrix.com> wrote:
> # HG changeset patch
> # User Paul Durrant <paul.durrant@citrix.com>
> # Date 1322235040 0
> # Node ID 2a8ef49f60cfe5a6c4c9f434af472ab58a125a7a
> # Parent 0a0c02a616768bfab16c072788cb76be1893c37f
> Fix save/restore for HVM domains with viridian=1
>
> xc_domain_save/restore currently pay no attention to HVM_PARAM_VIRIDIAN which
> results in an HVM domain running a recent version on Windows (post-Vista)
> locking up on a domain restore due to EOIs (done via a viridian MSR write)
> being silently dropped.
> This patch adds an extra save entry for the viridian parameter.
Ok, I did check in the previous with the intention of fixing up the Xen
bits. Is the best fix now agreed to just remove the Xen bits? :-) I can
check in a patch to do that if so.
-- Keir
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>
> diff -r 0a0c02a61676 -r 2a8ef49f60cf tools/libxc/xc_domain_restore.c
> --- a/tools/libxc/xc_domain_restore.c Mon Nov 21 21:28:34 2011 +0000
> +++ b/tools/libxc/xc_domain_restore.c Fri Nov 25 15:30:40 2011 +0000
> @@ -675,6 +675,7 @@ typedef struct {
> uint64_t vm86_tss;
> uint64_t console_pfn;
> uint64_t acpi_ioport_location;
> + uint64_t viridian;
> } pagebuf_t;
>
> static int pagebuf_init(pagebuf_t* buf)
> @@ -809,6 +810,16 @@ static int pagebuf_get_one(xc_interface
> }
> return pagebuf_get_one(xch, ctx, buf, fd, dom);
>
> + case XC_SAVE_ID_HVM_VIRIDIAN:
> + /* Skip padding 4 bytes then read the viridian flag. */
> + if ( RDEXACT(fd, &buf->viridian, sizeof(uint32_t)) ||
> + RDEXACT(fd, &buf->viridian, sizeof(uint64_t)) )
> + {
> + PERROR("error read the viridian flag");
> + return -1;
> + }
> + return pagebuf_get_one(xch, ctx, buf, fd, dom);
> +
> default:
> if ( (count > MAX_BATCH_SIZE) || (count < 0) ) {
> ERROR("Max batch size exceeded (%d). Giving up.", count);
> @@ -1440,6 +1451,9 @@ int xc_domain_restore(xc_interface *xch,
> fcntl(io_fd, F_SETFL, orig_io_fd_flags | O_NONBLOCK);
> }
>
> + if (pagebuf.viridian != 0)
> + xc_set_hvm_param(xch, dom, HVM_PARAM_VIRIDIAN, 1);
> +
> if (pagebuf.acpi_ioport_location == 1) {
> DBGPRINTF("Use new firmware ioport from the checkpoint\n");
> xc_set_hvm_param(xch, dom, HVM_PARAM_ACPI_IOPORTS_LOCATION, 1);
> diff -r 0a0c02a61676 -r 2a8ef49f60cf tools/libxc/xc_domain_save.c
> --- a/tools/libxc/xc_domain_save.c Mon Nov 21 21:28:34 2011 +0000
> +++ b/tools/libxc/xc_domain_save.c Fri Nov 25 15:30:40 2011 +0000
> @@ -1506,6 +1506,18 @@ int xc_domain_save(xc_interface *xch, in
> PERROR("Error when writing the firmware ioport version");
> goto out;
> }
> +
> + chunk.id = XC_SAVE_ID_HVM_VIRIDIAN;
> + chunk.data = 0;
> + xc_get_hvm_param(xch, dom, HVM_PARAM_VIRIDIAN,
> + (unsigned long *)&chunk.data);
> +
> + if ( (chunk.data != 0) &&
> + wrexact(io_fd, &chunk, sizeof(chunk)) )
> + {
> + PERROR("Error when writing the viridian flag");
> + goto out;
> + }
> }
>
> if ( !callbacks->checkpoint )
> diff -r 0a0c02a61676 -r 2a8ef49f60cf tools/libxc/xg_save_restore.h
> --- a/tools/libxc/xg_save_restore.h Mon Nov 21 21:28:34 2011 +0000
> +++ b/tools/libxc/xg_save_restore.h Fri Nov 25 15:30:40 2011 +0000
> @@ -134,6 +134,7 @@
> #define XC_SAVE_ID_HVM_CONSOLE_PFN -8 /* (HVM-only) */
> #define XC_SAVE_ID_LAST_CHECKPOINT -9 /* Commit to restoring after
> completion of current iteration. */
> #define XC_SAVE_ID_HVM_ACPI_IOPORTS_LOCATION -10
> +#define XC_SAVE_ID_HVM_VIRIDIAN -11
>
> /*
> ** We process save/restore/migrate in batches of pages; the below
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] Fix save/restore for HVM domains with viridian=1
2011-11-25 15:39 ` Keir Fraser
@ 2011-11-25 15:42 ` Paul Durrant
0 siblings, 0 replies; 14+ messages in thread
From: Paul Durrant @ 2011-11-25 15:42 UTC (permalink / raw)
To: Keir Fraser, xen-devel@lists.xensource.com
Keir,
Yes, just ditch the kernel change.
Paul
> -----Original Message-----
> From: Keir Fraser [mailto:keir.xen@gmail.com]
> Sent: 25 November 2011 15:40
> To: Paul Durrant; xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH] Fix save/restore for HVM domains
> with viridian=1
>
> On 25/11/2011 15:30, "Paul Durrant" <paul.durrant@citrix.com> wrote:
>
> > # HG changeset patch
> > # User Paul Durrant <paul.durrant@citrix.com> # Date 1322235040 0
> #
> > Node ID 2a8ef49f60cfe5a6c4c9f434af472ab58a125a7a
> > # Parent 0a0c02a616768bfab16c072788cb76be1893c37f
> > Fix save/restore for HVM domains with viridian=1
> >
> > xc_domain_save/restore currently pay no attention to
> > HVM_PARAM_VIRIDIAN which results in an HVM domain running a recent
> > version on Windows (post-Vista) locking up on a domain restore due
> to
> > EOIs (done via a viridian MSR write) being silently dropped.
> > This patch adds an extra save entry for the viridian parameter.
>
> Ok, I did check in the previous with the intention of fixing up the
> Xen bits. Is the best fix now agreed to just remove the Xen bits? :-
> ) I can check in a patch to do that if so.
>
> -- Keir
>
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> >
> > diff -r 0a0c02a61676 -r 2a8ef49f60cf
> tools/libxc/xc_domain_restore.c
> > --- a/tools/libxc/xc_domain_restore.c Mon Nov 21 21:28:34 2011
> +0000
> > +++ b/tools/libxc/xc_domain_restore.c Fri Nov 25 15:30:40 2011
> +0000
> > @@ -675,6 +675,7 @@ typedef struct {
> > uint64_t vm86_tss;
> > uint64_t console_pfn;
> > uint64_t acpi_ioport_location;
> > + uint64_t viridian;
> > } pagebuf_t;
> >
> > static int pagebuf_init(pagebuf_t* buf) @@ -809,6 +810,16 @@
> static
> > int pagebuf_get_one(xc_interface
> > }
> > return pagebuf_get_one(xch, ctx, buf, fd, dom);
> >
> > + case XC_SAVE_ID_HVM_VIRIDIAN:
> > + /* Skip padding 4 bytes then read the viridian flag. */
> > + if ( RDEXACT(fd, &buf->viridian, sizeof(uint32_t)) ||
> > + RDEXACT(fd, &buf->viridian, sizeof(uint64_t)) )
> > + {
> > + PERROR("error read the viridian flag");
> > + return -1;
> > + }
> > + return pagebuf_get_one(xch, ctx, buf, fd, dom);
> > +
> > default:
> > if ( (count > MAX_BATCH_SIZE) || (count < 0) ) {
> > ERROR("Max batch size exceeded (%d). Giving up.",
> count);
> > @@ -1440,6 +1451,9 @@ int xc_domain_restore(xc_interface *xch,
> > fcntl(io_fd, F_SETFL, orig_io_fd_flags | O_NONBLOCK);
> > }
> >
> > + if (pagebuf.viridian != 0)
> > + xc_set_hvm_param(xch, dom, HVM_PARAM_VIRIDIAN, 1);
> > +
> > if (pagebuf.acpi_ioport_location == 1) {
> > DBGPRINTF("Use new firmware ioport from the
> checkpoint\n");
> > xc_set_hvm_param(xch, dom,
> HVM_PARAM_ACPI_IOPORTS_LOCATION,
> > 1); diff -r 0a0c02a61676 -r 2a8ef49f60cf
> tools/libxc/xc_domain_save.c
> > --- a/tools/libxc/xc_domain_save.c Mon Nov 21 21:28:34 2011 +0000
> > +++ b/tools/libxc/xc_domain_save.c Fri Nov 25 15:30:40 2011 +0000
> > @@ -1506,6 +1506,18 @@ int xc_domain_save(xc_interface *xch, in
> > PERROR("Error when writing the firmware ioport
> version");
> > goto out;
> > }
> > +
> > + chunk.id = XC_SAVE_ID_HVM_VIRIDIAN;
> > + chunk.data = 0;
> > + xc_get_hvm_param(xch, dom, HVM_PARAM_VIRIDIAN,
> > + (unsigned long *)&chunk.data);
> > +
> > + if ( (chunk.data != 0) &&
> > + wrexact(io_fd, &chunk, sizeof(chunk)) )
> > + {
> > + PERROR("Error when writing the viridian flag");
> > + goto out;
> > + }
> > }
> >
> > if ( !callbacks->checkpoint )
> > diff -r 0a0c02a61676 -r 2a8ef49f60cf tools/libxc/xg_save_restore.h
> > --- a/tools/libxc/xg_save_restore.h Mon Nov 21 21:28:34 2011 +0000
> > +++ b/tools/libxc/xg_save_restore.h Fri Nov 25 15:30:40 2011 +0000
> > @@ -134,6 +134,7 @@
> > #define XC_SAVE_ID_HVM_CONSOLE_PFN -8 /* (HVM-only) */
> > #define XC_SAVE_ID_LAST_CHECKPOINT -9 /* Commit to restoring
> after
> > completion of current iteration. */
> > #define XC_SAVE_ID_HVM_ACPI_IOPORTS_LOCATION -10
> > +#define XC_SAVE_ID_HVM_VIRIDIAN -11
> >
> > /*
> > ** We process save/restore/migrate in batches of pages; the below
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2011-11-25 15:42 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-11-25 14:50 [PATCH] Fix save/restore for HVM domains with viridian=1 Paul Durrant
2011-11-25 14:58 ` Jan Beulich
2011-11-25 15:16 ` Keir Fraser
2011-11-25 15:00 ` Tim Deegan
2011-11-25 15:18 ` Keir Fraser
2011-11-25 15:26 ` Paul Durrant
-- strict thread matches above, loose matches on Subject: below --
2011-11-25 15:14 Paul Durrant
2011-11-25 15:25 ` Tim Deegan
2011-11-25 15:28 Paul Durrant
2011-11-25 15:29 Paul Durrant
2011-11-25 15:30 ` Paul Durrant
2011-11-25 15:30 Paul Durrant
2011-11-25 15:39 ` Keir Fraser
2011-11-25 15:42 ` Paul Durrant
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.