* [RFC PATCH 1/3] xen/hvm-save: Refactor HVM_REGISTER_SAVE_RESTORE
2013-12-16 2:17 [RFC PATCH 0/3] Xen hvm saving improvements Andrew Cooper
@ 2013-12-16 2:17 ` Andrew Cooper
2013-12-16 9:33 ` Jan Beulich
2013-12-16 2:17 ` [RFC PATCH 2/3] xen/hvm-save: Extend hvm_save_handler to take an instance parameter Andrew Cooper
` (3 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Andrew Cooper @ 2013-12-16 2:17 UTC (permalink / raw)
To: Xen-devel
Cc: George Dunlap, Andrew Cooper, Keir Fraser, Don Slutz, Jan Beulich
The instance id in a save record served two purposes. For a PER_VCPU record,
it was the VCPU id while for a PER_DOM it was just an index.
As the number of instances needs to be stored to help fix hvm_save_one() later
in this series, refactor HVM_REGISTER_SAVE_RESTORE to simplify the interface
and prevent the buggy case of registering a PER_VCPU record with multiple
instances. The 'kind' can now be inferred from the number of instances.
There is now HVM_REGISTER_SAVE_RESTORE_PER_DOM() and
HVM_REGISTER_SAVE_RESTORE_PER_VCPU() which both take fewer arguments, and only
PER_DOM() allows setting a number of instances.
There is no observable change as a result of this patch.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Don Slutz <dslutz@verizon.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
xen/arch/x86/cpu/mcheck/vmce.c | 4 ++--
xen/arch/x86/hvm/hpet.c | 2 +-
xen/arch/x86/hvm/hvm.c | 11 +++++------
xen/arch/x86/hvm/i8254.c | 2 +-
xen/arch/x86/hvm/irq.c | 9 +++------
xen/arch/x86/hvm/mtrr.c | 3 +--
xen/arch/x86/hvm/pmtimer.c | 3 +--
xen/arch/x86/hvm/rtc.c | 2 +-
xen/arch/x86/hvm/vioapic.c | 2 +-
xen/arch/x86/hvm/viridian.c | 8 ++++----
xen/arch/x86/hvm/vlapic.c | 6 ++----
xen/arch/x86/hvm/vpic.c | 2 +-
xen/common/hvm/save.c | 13 ++++++++-----
xen/include/xen/hvm/save.h | 35 ++++++++++++++++++++---------------
14 files changed, 51 insertions(+), 51 deletions(-)
diff --git a/xen/arch/x86/cpu/mcheck/vmce.c b/xen/arch/x86/cpu/mcheck/vmce.c
index f6c35db..a88368a 100644
--- a/xen/arch/x86/cpu/mcheck/vmce.c
+++ b/xen/arch/x86/cpu/mcheck/vmce.c
@@ -335,8 +335,8 @@ static int vmce_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
return err ?: vmce_restore_vcpu(v, &ctxt);
}
-HVM_REGISTER_SAVE_RESTORE(VMCE_VCPU, vmce_save_vcpu_ctxt,
- vmce_load_vcpu_ctxt, 1, HVMSR_PER_VCPU);
+HVM_REGISTER_SAVE_RESTORE_PER_VCPU(VMCE_VCPU, vmce_save_vcpu_ctxt,
+ vmce_load_vcpu_ctxt);
/*
* for Intel MCE, broadcast vMCE to all vcpus
diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 4324b52..fb2c098 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -569,7 +569,7 @@ static int hpet_load(struct domain *d, hvm_domain_context_t *h)
return 0;
}
-HVM_REGISTER_SAVE_RESTORE(HPET, hpet_save, hpet_load, 1, HVMSR_PER_DOM);
+HVM_REGISTER_SAVE_RESTORE_PER_DOM(HPET, hpet_save, hpet_load, 1);
void hpet_init(struct vcpu *v)
{
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 69f7e74..eb21fc4 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -699,8 +699,8 @@ static int hvm_load_tsc_adjust(struct domain *d, hvm_domain_context_t *h)
return 0;
}
-HVM_REGISTER_SAVE_RESTORE(TSC_ADJUST, hvm_save_tsc_adjust,
- hvm_load_tsc_adjust, 1, HVMSR_PER_VCPU);
+HVM_REGISTER_SAVE_RESTORE_PER_VCPU(TSC_ADJUST, hvm_save_tsc_adjust,
+ hvm_load_tsc_adjust);
static int hvm_save_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
{
@@ -999,8 +999,7 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
return 0;
}
-HVM_REGISTER_SAVE_RESTORE(CPU, hvm_save_cpu_ctxt, hvm_load_cpu_ctxt,
- 1, HVMSR_PER_VCPU);
+HVM_REGISTER_SAVE_RESTORE_PER_VCPU(CPU, hvm_save_cpu_ctxt, hvm_load_cpu_ctxt);
#define HVM_CPU_XSAVE_SIZE(xcr0) (offsetof(struct hvm_hw_cpu_xsave, \
save_area) + \
@@ -1136,9 +1135,9 @@ static int __init __hvm_register_CPU_XSAVE_save_and_restore(void)
"CPU_XSAVE",
hvm_save_cpu_xsave_states,
hvm_load_cpu_xsave_states,
+ 0,
HVM_CPU_XSAVE_SIZE(xfeature_mask) +
- sizeof(struct hvm_save_descriptor),
- HVMSR_PER_VCPU);
+ sizeof(struct hvm_save_descriptor));
return 0;
}
__initcall(__hvm_register_CPU_XSAVE_save_and_restore);
diff --git a/xen/arch/x86/hvm/i8254.c b/xen/arch/x86/hvm/i8254.c
index c0d6bc2..139812a 100644
--- a/xen/arch/x86/hvm/i8254.c
+++ b/xen/arch/x86/hvm/i8254.c
@@ -423,7 +423,7 @@ static int pit_load(struct domain *d, hvm_domain_context_t *h)
return 0;
}
-HVM_REGISTER_SAVE_RESTORE(PIT, pit_save, pit_load, 1, HVMSR_PER_DOM);
+HVM_REGISTER_SAVE_RESTORE_PER_DOM(PIT, pit_save, pit_load, 1);
void pit_reset(struct domain *d)
{
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index 677fbcd..04ce739 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -658,9 +658,6 @@ static int irq_load_link(struct domain *d, hvm_domain_context_t *h)
return 0;
}
-HVM_REGISTER_SAVE_RESTORE(PCI_IRQ, irq_save_pci, irq_load_pci,
- 1, HVMSR_PER_DOM);
-HVM_REGISTER_SAVE_RESTORE(ISA_IRQ, irq_save_isa, irq_load_isa,
- 1, HVMSR_PER_DOM);
-HVM_REGISTER_SAVE_RESTORE(PCI_LINK, irq_save_link, irq_load_link,
- 1, HVMSR_PER_DOM);
+HVM_REGISTER_SAVE_RESTORE_PER_DOM(PCI_IRQ, irq_save_pci, irq_load_pci, 1);
+HVM_REGISTER_SAVE_RESTORE_PER_DOM(ISA_IRQ, irq_save_isa, irq_load_isa, 1);
+HVM_REGISTER_SAVE_RESTORE_PER_DOM(PCI_LINK, irq_save_link, irq_load_link, 1);
diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 9937f5a..61c785c 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -677,8 +677,7 @@ static int hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
return 0;
}
-HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr, hvm_load_mtrr_msr,
- 1, HVMSR_PER_VCPU);
+HVM_REGISTER_SAVE_RESTORE_PER_VCPU(MTRR, hvm_save_mtrr_msr, hvm_load_mtrr_msr);
uint8_t epte_get_entry_emt(struct domain *d, unsigned long gfn, mfn_t mfn,
uint8_t *ipat, bool_t direct_mmio)
diff --git a/xen/arch/x86/hvm/pmtimer.c b/xen/arch/x86/hvm/pmtimer.c
index 01ae31d..282e8ee 100644
--- a/xen/arch/x86/hvm/pmtimer.c
+++ b/xen/arch/x86/hvm/pmtimer.c
@@ -292,8 +292,7 @@ static int pmtimer_load(struct domain *d, hvm_domain_context_t *h)
return 0;
}
-HVM_REGISTER_SAVE_RESTORE(PMTIMER, pmtimer_save, pmtimer_load,
- 1, HVMSR_PER_DOM);
+HVM_REGISTER_SAVE_RESTORE_PER_DOM(PMTIMER, pmtimer_save, pmtimer_load, 1);
int pmtimer_change_ioport(struct domain *d, unsigned int version)
{
diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index cdedefe..8d9d634 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -741,7 +741,7 @@ static int rtc_load(struct domain *d, hvm_domain_context_t *h)
return 0;
}
-HVM_REGISTER_SAVE_RESTORE(RTC, rtc_save, rtc_load, 1, HVMSR_PER_DOM);
+HVM_REGISTER_SAVE_RESTORE_PER_DOM(RTC, rtc_save, rtc_load, 1);
void rtc_reset(struct domain *d)
{
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index d3c681b..7c75192 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -428,7 +428,7 @@ static int ioapic_load(struct domain *d, hvm_domain_context_t *h)
return hvm_load_entry(IOAPIC, h, s);
}
-HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, ioapic_load, 1, HVMSR_PER_DOM);
+HVM_REGISTER_SAVE_RESTORE_PER_DOM(IOAPIC, ioapic_save, ioapic_load, 1);
void vioapic_reset(struct domain *d)
{
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index 2b86d66..0ba85b3 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -449,8 +449,8 @@ static int viridian_load_domain_ctxt(struct domain *d, hvm_domain_context_t *h)
return 0;
}
-HVM_REGISTER_SAVE_RESTORE(VIRIDIAN_DOMAIN, viridian_save_domain_ctxt,
- viridian_load_domain_ctxt, 1, HVMSR_PER_DOM);
+HVM_REGISTER_SAVE_RESTORE_PER_DOM(VIRIDIAN_DOMAIN, viridian_save_domain_ctxt,
+ viridian_load_domain_ctxt, 1);
static int viridian_save_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
{
@@ -493,5 +493,5 @@ static int viridian_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
return 0;
}
-HVM_REGISTER_SAVE_RESTORE(VIRIDIAN_VCPU, viridian_save_vcpu_ctxt,
- viridian_load_vcpu_ctxt, 1, HVMSR_PER_VCPU);
+HVM_REGISTER_SAVE_RESTORE_PER_VCPU(VIRIDIAN_VCPU, viridian_save_vcpu_ctxt,
+ viridian_load_vcpu_ctxt);
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index bc06010..b64b9ee 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1220,10 +1220,8 @@ static int lapic_load_regs(struct domain *d, hvm_domain_context_t *h)
return 0;
}
-HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_save_hidden, lapic_load_hidden,
- 1, HVMSR_PER_VCPU);
-HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs, lapic_load_regs,
- 1, HVMSR_PER_VCPU);
+HVM_REGISTER_SAVE_RESTORE_PER_VCPU(LAPIC, lapic_save_hidden, lapic_load_hidden);
+HVM_REGISTER_SAVE_RESTORE_PER_VCPU(LAPIC_REGS, lapic_save_regs, lapic_load_regs);
int vlapic_init(struct vcpu *v)
{
diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c
index fea3f68..e882fe1 100644
--- a/xen/arch/x86/hvm/vpic.c
+++ b/xen/arch/x86/hvm/vpic.c
@@ -398,7 +398,7 @@ static int vpic_load(struct domain *d, hvm_domain_context_t *h)
return 0;
}
-HVM_REGISTER_SAVE_RESTORE(PIC, vpic_save, vpic_load, 2, HVMSR_PER_DOM);
+HVM_REGISTER_SAVE_RESTORE_PER_DOM(PIC, vpic_save, vpic_load, 2);
void vpic_reset(struct domain *d)
{
diff --git a/xen/common/hvm/save.c b/xen/common/hvm/save.c
index de76ada..2800c5b 100644
--- a/xen/common/hvm/save.c
+++ b/xen/common/hvm/save.c
@@ -36,15 +36,18 @@ static struct {
hvm_load_handler load;
const char *name;
size_t size;
- int kind;
+ unsigned int num;
} hvm_sr_handlers [HVM_SAVE_CODE_MAX + 1] = {{NULL, NULL, "<?>"},};
+#define is_per_vcpu_handler(_h) ((_h).num == 0)
+
/* Init-time function to add entries to that list */
void __init hvm_register_savevm(uint16_t typecode,
const char *name,
hvm_save_handler save_state,
hvm_load_handler load_state,
- size_t size, int kind)
+ unsigned int num,
+ size_t size)
{
ASSERT(typecode <= HVM_SAVE_CODE_MAX);
ASSERT(hvm_sr_handlers[typecode].save == NULL);
@@ -53,7 +56,7 @@ void __init hvm_register_savevm(uint16_t typecode,
hvm_sr_handlers[typecode].load = load_state;
hvm_sr_handlers[typecode].name = name;
hvm_sr_handlers[typecode].size = size;
- hvm_sr_handlers[typecode].kind = kind;
+ hvm_sr_handlers[typecode].num = num;
}
size_t hvm_save_size(struct domain *d)
@@ -67,7 +70,7 @@ size_t hvm_save_size(struct domain *d)
/* Plus space for each thing we will be saving */
for ( i = 0; i <= HVM_SAVE_CODE_MAX; i++ )
- if ( hvm_sr_handlers[i].kind == HVMSR_PER_VCPU )
+ if ( is_per_vcpu_handler(hvm_sr_handlers[i]) )
for_each_vcpu(d, v)
sz += hvm_sr_handlers[i].size;
else
@@ -92,7 +95,7 @@ int hvm_save_one(struct domain *d, uint16_t typecode, uint16_t instance,
|| hvm_sr_handlers[typecode].save == NULL )
return -EINVAL;
- if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_VCPU )
+ if ( is_per_vcpu_handler(hvm_sr_handlers[typecode]) )
for_each_vcpu(d, v)
sz += hvm_sr_handlers[typecode].size;
else
diff --git a/xen/include/xen/hvm/save.h b/xen/include/xen/hvm/save.h
index ae6f0bb..0e3ef13 100644
--- a/xen/include/xen/hvm/save.h
+++ b/xen/include/xen/hvm/save.h
@@ -94,30 +94,35 @@ typedef int (*hvm_save_handler) (struct domain *d,
typedef int (*hvm_load_handler) (struct domain *d,
hvm_domain_context_t *h);
-/* Init-time function to declare a pair of handlers for a type,
- * and the maximum buffer space needed to save this type of state */
+/* Init-time function to declare a pair of handlers for a type, and the
+ * maximum buffer space needed to save this type of state. 'num' of 0
+ * indicates a per-vcpu record, while 'num' of >0 indicates a per-domain
+ * record. */
void hvm_register_savevm(uint16_t typecode,
const char *name,
hvm_save_handler save_state,
hvm_load_handler load_state,
- size_t size, int kind);
-
-/* The space needed for saving can be per-domain or per-vcpu: */
-#define HVMSR_PER_DOM 0
-#define HVMSR_PER_VCPU 1
+ unsigned int num, size_t size);
/* Syntactic sugar around that function: specify the max number of
* saves, and this calculates the size of buffer needed */
-#define HVM_REGISTER_SAVE_RESTORE(_x, _save, _load, _num, _k) \
+#define HVM_REGISTER_SAVE_RESTORE_PER_DOM(_x, _save, _load, _num) \
+static int __init __hvm_register_##_x##_save_and_restore(void) \
+{ \
+ hvm_register_savevm( \
+ HVM_SAVE_CODE(_x), #_x, &_save, &_load, _num, \
+ (_num) * (HVM_SAVE_LENGTH(_x) \
+ + sizeof (struct hvm_save_descriptor))); \
+ return 0; \
+} \
+__initcall(__hvm_register_##_x##_save_and_restore);
+
+#define HVM_REGISTER_SAVE_RESTORE_PER_VCPU(_x, _save, _load) \
static int __init __hvm_register_##_x##_save_and_restore(void) \
{ \
- hvm_register_savevm(HVM_SAVE_CODE(_x), \
- #_x, \
- &_save, \
- &_load, \
- (_num) * (HVM_SAVE_LENGTH(_x) \
- + sizeof (struct hvm_save_descriptor)), \
- _k); \
+ hvm_register_savevm( \
+ HVM_SAVE_CODE(_x), #_x, &_save, &_load, 0, \
+ HVM_SAVE_LENGTH(_x) + sizeof (struct hvm_save_descriptor)); \
return 0; \
} \
__initcall(__hvm_register_##_x##_save_and_restore);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [RFC PATCH 1/3] xen/hvm-save: Refactor HVM_REGISTER_SAVE_RESTORE
2013-12-16 2:17 ` [RFC PATCH 1/3] xen/hvm-save: Refactor HVM_REGISTER_SAVE_RESTORE Andrew Cooper
@ 2013-12-16 9:33 ` Jan Beulich
0 siblings, 0 replies; 9+ messages in thread
From: Jan Beulich @ 2013-12-16 9:33 UTC (permalink / raw)
To: Andrew Cooper; +Cc: George Dunlap, xen-devel, Keir Fraser, Don Slutz
>>> On 16.12.13 at 03:17, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> The instance id in a save record served two purposes. For a PER_VCPU record,
> it was the VCPU id while for a PER_DOM it was just an index.
>
> As the number of instances needs to be stored to help fix hvm_save_one()
> later
> in this series, refactor HVM_REGISTER_SAVE_RESTORE to simplify the interface
> and prevent the buggy case of registering a PER_VCPU record with multiple
> instances. The 'kind' can now be inferred from the number of instances.
>
> There is now HVM_REGISTER_SAVE_RESTORE_PER_DOM() and
> HVM_REGISTER_SAVE_RESTORE_PER_VCPU() which both take fewer arguments, and
> only
> PER_DOM() allows setting a number of instances.
>
> There is no observable change as a result of this patch.
I think this is removing flexibilty (even if not needed by current code)
is that it disallows multiple instances of per-vCPU records. After all
the current "instance == vCPU-ID" implication is a per-save-type one,
not one baked into the interface.
Jan
^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFC PATCH 2/3] xen/hvm-save: Extend hvm_save_handler to take an instance parameter
2013-12-16 2:17 [RFC PATCH 0/3] Xen hvm saving improvements Andrew Cooper
2013-12-16 2:17 ` [RFC PATCH 1/3] xen/hvm-save: Refactor HVM_REGISTER_SAVE_RESTORE Andrew Cooper
@ 2013-12-16 2:17 ` Andrew Cooper
2013-12-16 2:17 ` [RFC PATCH 3/3] xen/hvm-save: Adjust calling of multi-instance save handlers Andrew Cooper
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Andrew Cooper @ 2013-12-16 2:17 UTC (permalink / raw)
To: Xen-devel
Cc: George Dunlap, Andrew Cooper, Keir Fraser, Don Slutz, Jan Beulich
The per-domain save handlers take this new parameter and (other than the PIC
which is the special case), use it in their hvm_save_entry() calls.
The per-vcpu save handlers take this new parameter and currently ignore it.
The return value for hvm_save_handler has been redefined to be more useful.
All save handlers have been modified to return negative on failure. The
per-domain ones return the number of bytes written, but the per-vcpu ones
still return 0 on success. This involved tweaking hvm_save_entry() and
_hvm_{init,write}_entry(), and most uses of them.
The callsites for the save handlers currently have the instance parameter
hardwired to 0, and have their error detection code suitably altered.
There should be no observable difference as a result of this patch.
There are a few scattered style fixes in the save functions.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Suggested-by: Jan Beulich <JBeulich@suse.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Don Slutz <dslutz@verizon.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
xen/arch/x86/cpu/mcheck/vmce.c | 5 +++--
xen/arch/x86/hvm/hpet.c | 6 +++---
xen/arch/x86/hvm/hvm.c | 19 +++++++++++--------
xen/arch/x86/hvm/i8254.c | 6 +++---
xen/arch/x86/hvm/irq.c | 15 +++++++++------
xen/arch/x86/hvm/mtrr.c | 7 ++++---
xen/arch/x86/hvm/pmtimer.c | 5 +++--
xen/arch/x86/hvm/rtc.c | 6 ++++--
xen/arch/x86/hvm/vioapic.c | 5 +++--
xen/arch/x86/hvm/viridian.c | 12 +++++++-----
xen/arch/x86/hvm/vlapic.c | 10 ++++++----
xen/arch/x86/hvm/vpic.c | 7 ++++---
xen/common/hvm/save.c | 15 ++++++++-------
xen/include/xen/hvm/save.h | 30 ++++++++++++++++++++----------
14 files changed, 88 insertions(+), 60 deletions(-)
diff --git a/xen/arch/x86/cpu/mcheck/vmce.c b/xen/arch/x86/cpu/mcheck/vmce.c
index a88368a..8ef40c3 100644
--- a/xen/arch/x86/cpu/mcheck/vmce.c
+++ b/xen/arch/x86/cpu/mcheck/vmce.c
@@ -296,7 +296,8 @@ int vmce_wrmsr(uint32_t msr, uint64_t val)
return ret;
}
-static int vmce_save_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
+static int vmce_save_vcpu_ctxt(struct domain *d, uint16_t inst,
+ hvm_domain_context_t *h)
{
struct vcpu *v;
int err = 0;
@@ -309,7 +310,7 @@ static int vmce_save_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
};
err = hvm_save_entry(VMCE_VCPU, v->vcpu_id, h, &ctxt);
- if ( err )
+ if ( err < 0 )
break;
}
diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index fb2c098..fbde99d 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -468,7 +468,7 @@ const struct hvm_mmio_handler hpet_mmio_handler = {
};
-static int hpet_save(struct domain *d, hvm_domain_context_t *h)
+static int hpet_save(struct domain *d, uint16_t inst, hvm_domain_context_t *h)
{
HPETState *hp = domain_vhpet(d);
int rc;
@@ -479,8 +479,8 @@ static int hpet_save(struct domain *d, hvm_domain_context_t *h)
hp->hpet.mc64 = hp->mc_offset + guest_time_hpet(hp);
/* Save the HPET registers */
- rc = _hvm_init_entry(h, HVM_SAVE_CODE(HPET), 0, HVM_SAVE_LENGTH(HPET));
- if ( rc == 0 )
+ rc = _hvm_init_entry(h, HVM_SAVE_CODE(HPET), inst, HVM_SAVE_LENGTH(HPET));
+ if ( rc >= 0 )
{
struct hvm_hw_hpet *rec = (struct hvm_hw_hpet *)&h->data[h->cur];
h->cur += HVM_SAVE_LENGTH(HPET);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index eb21fc4..925e792 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -662,7 +662,8 @@ void hvm_domain_destroy(struct domain *d)
vioapic_deinit(d);
}
-static int hvm_save_tsc_adjust(struct domain *d, hvm_domain_context_t *h)
+static int hvm_save_tsc_adjust(struct domain *d, uint16_t inst,
+ hvm_domain_context_t *h)
{
struct vcpu *v;
struct hvm_tsc_adjust ctxt;
@@ -672,7 +673,7 @@ static int hvm_save_tsc_adjust(struct domain *d, hvm_domain_context_t *h)
{
ctxt.tsc_adjust = v->arch.hvm_vcpu.msr_tsc_adjust;
err = hvm_save_entry(TSC_ADJUST, v->vcpu_id, h, &ctxt);
- if ( err )
+ if ( err < 0 )
break;
}
@@ -702,7 +703,8 @@ static int hvm_load_tsc_adjust(struct domain *d, hvm_domain_context_t *h)
HVM_REGISTER_SAVE_RESTORE_PER_VCPU(TSC_ADJUST, hvm_save_tsc_adjust,
hvm_load_tsc_adjust);
-static int hvm_save_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
+static int hvm_save_cpu_ctxt(struct domain *d, uint16_t inst,
+ hvm_domain_context_t *h)
{
struct vcpu *v;
struct hvm_hw_cpu ctxt;
@@ -806,8 +808,8 @@ static int hvm_save_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
ctxt.dr6 = v->arch.debugreg[6];
ctxt.dr7 = v->arch.debugreg[7];
- if ( hvm_save_entry(CPU, v->vcpu_id, h, &ctxt) != 0 )
- return 1;
+ if ( hvm_save_entry(CPU, v->vcpu_id, h, &ctxt) < 0 )
+ return -ENOSPC;
}
return 0;
}
@@ -1005,7 +1007,8 @@ HVM_REGISTER_SAVE_RESTORE_PER_VCPU(CPU, hvm_save_cpu_ctxt, hvm_load_cpu_ctxt);
save_area) + \
xstate_ctxt_size(xcr0))
-static int hvm_save_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
+static int hvm_save_cpu_xsave_states(struct domain *d, uint16_t inst,
+ hvm_domain_context_t *h)
{
struct vcpu *v;
struct hvm_hw_cpu_xsave *ctxt;
@@ -1019,8 +1022,8 @@ static int hvm_save_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
if ( !xsave_enabled(v) )
continue;
- if ( _hvm_init_entry(h, CPU_XSAVE_CODE, v->vcpu_id, size) )
- return 1;
+ if ( _hvm_init_entry(h, CPU_XSAVE_CODE, v->vcpu_id, size) < 0 )
+ return -ENOSPC;
ctxt = (struct hvm_hw_cpu_xsave *)&h->data[h->cur];
h->cur += size;
diff --git a/xen/arch/x86/hvm/i8254.c b/xen/arch/x86/hvm/i8254.c
index 139812a..33e6990 100644
--- a/xen/arch/x86/hvm/i8254.c
+++ b/xen/arch/x86/hvm/i8254.c
@@ -382,14 +382,14 @@ void pit_stop_channel0_irq(PITState *pit)
spin_unlock(&pit->lock);
}
-static int pit_save(struct domain *d, hvm_domain_context_t *h)
+static int pit_save(struct domain *d, uint16_t inst, hvm_domain_context_t *h)
{
PITState *pit = domain_vpit(d);
int rc;
spin_lock(&pit->lock);
-
- rc = hvm_save_entry(PIT, 0, h, &pit->hw);
+
+ rc = hvm_save_entry(PIT, inst, h, &pit->hw);
spin_unlock(&pit->lock);
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index 04ce739..a23bd0a 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -524,7 +524,8 @@ static int __init dump_irq_info_key_init(void)
}
__initcall(dump_irq_info_key_init);
-static int irq_save_pci(struct domain *d, hvm_domain_context_t *h)
+static int irq_save_pci(struct domain *d, uint16_t inst,
+ hvm_domain_context_t *h)
{
struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
unsigned int asserted, pdev, pintx;
@@ -546,7 +547,7 @@ static int irq_save_pci(struct domain *d, hvm_domain_context_t *h)
__hvm_pci_intx_deassert(d, pdev, pintx);
/* Save PCI IRQ lines */
- rc = hvm_save_entry(PCI_IRQ, 0, h, &hvm_irq->pci_intx);
+ rc = hvm_save_entry(PCI_IRQ, inst, h, &hvm_irq->pci_intx);
if ( asserted )
__hvm_pci_intx_assert(d, pdev, pintx);
@@ -556,20 +557,22 @@ static int irq_save_pci(struct domain *d, hvm_domain_context_t *h)
return rc;
}
-static int irq_save_isa(struct domain *d, hvm_domain_context_t *h)
+static int irq_save_isa(struct domain *d, uint16_t inst,
+ hvm_domain_context_t *h)
{
struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
/* Save ISA IRQ lines */
- return ( hvm_save_entry(ISA_IRQ, 0, h, &hvm_irq->isa_irq) );
+ return hvm_save_entry(ISA_IRQ, inst, h, &hvm_irq->isa_irq);
}
-static int irq_save_link(struct domain *d, hvm_domain_context_t *h)
+static int irq_save_link(struct domain *d, uint16_t inst,
+ hvm_domain_context_t *h)
{
struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
/* Save PCI-ISA link state */
- return ( hvm_save_entry(PCI_LINK, 0, h, &hvm_irq->pci_link) );
+ return hvm_save_entry(PCI_LINK, inst, h, &hvm_irq->pci_link);
}
static int irq_load_pci(struct domain *d, hvm_domain_context_t *h)
diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 61c785c..40f58ed 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -600,7 +600,8 @@ int32_t hvm_set_mem_pinned_cacheattr(
return 0;
}
-static int hvm_save_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
+static int hvm_save_mtrr_msr(struct domain *d, uint16_t inst,
+ hvm_domain_context_t *h)
{
int i;
struct vcpu *v;
@@ -631,8 +632,8 @@ static int hvm_save_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
hw_mtrr.msr_mtrr_fixed[i] =
((uint64_t*)mtrr_state->fixed_ranges)[i];
- if ( hvm_save_entry(MTRR, v->vcpu_id, h, &hw_mtrr) != 0 )
- return 1;
+ if ( hvm_save_entry(MTRR, v->vcpu_id, h, &hw_mtrr) < 0 )
+ return -ENOSPC;
}
return 0;
}
diff --git a/xen/arch/x86/hvm/pmtimer.c b/xen/arch/x86/hvm/pmtimer.c
index 282e8ee..db27950 100644
--- a/xen/arch/x86/hvm/pmtimer.c
+++ b/xen/arch/x86/hvm/pmtimer.c
@@ -241,7 +241,8 @@ static int handle_pmt_io(
return X86EMUL_UNHANDLEABLE;
}
-static int pmtimer_save(struct domain *d, hvm_domain_context_t *h)
+static int pmtimer_save(struct domain *d, uint16_t inst,
+ hvm_domain_context_t *h)
{
PMTState *s = &d->arch.hvm_domain.pl_time.vpmt;
uint32_t x, msb = s->pm.tmr_val & TMR_VAL_MSB;
@@ -260,7 +261,7 @@ static int pmtimer_save(struct domain *d, hvm_domain_context_t *h)
/* No point in setting the SCI here because we'll already have saved the
* IRQ and *PIC state; we'll fix it up when we restore the domain */
- rc = hvm_save_entry(PMTIMER, 0, h, &s->pm);
+ rc = hvm_save_entry(PMTIMER, inst, h, &s->pm);
spin_unlock(&s->lock);
diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index 8d9d634..e3fe472 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -702,13 +702,15 @@ void rtc_migrate_timers(struct vcpu *v)
}
/* Save RTC hardware state */
-static int rtc_save(struct domain *d, hvm_domain_context_t *h)
+static int rtc_save(struct domain *d, uint16_t inst, hvm_domain_context_t *h)
{
RTCState *s = domain_vrtc(d);
int rc;
+
spin_lock(&s->lock);
- rc = hvm_save_entry(RTC, 0, h, &s->hw);
+ rc = hvm_save_entry(RTC, inst, h, &s->hw);
spin_unlock(&s->lock);
+
return rc;
}
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 7c75192..3efd2d1 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -416,10 +416,11 @@ void vioapic_update_EOI(struct domain *d, int vector)
spin_unlock(&d->arch.hvm_domain.irq_lock);
}
-static int ioapic_save(struct domain *d, hvm_domain_context_t *h)
+static int ioapic_save(struct domain *d, uint16_t inst, hvm_domain_context_t *h)
{
struct hvm_hw_vioapic *s = domain_vioapic(d);
- return hvm_save_entry(IOAPIC, 0, h, s);
+
+ return hvm_save_entry(IOAPIC, inst, h, s);
}
static int ioapic_load(struct domain *d, hvm_domain_context_t *h)
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index 0ba85b3..4f9186d 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -423,7 +423,8 @@ out:
return HVM_HCALL_completed;
}
-static int viridian_save_domain_ctxt(struct domain *d, hvm_domain_context_t *h)
+static int viridian_save_domain_ctxt(struct domain *d, uint16_t inst,
+ hvm_domain_context_t *h)
{
struct hvm_viridian_domain_context ctxt;
@@ -433,7 +434,7 @@ static int viridian_save_domain_ctxt(struct domain *d, hvm_domain_context_t *h)
ctxt.hypercall_gpa = d->arch.hvm_domain.viridian.hypercall_gpa.raw;
ctxt.guest_os_id = d->arch.hvm_domain.viridian.guest_os_id.raw;
- return (hvm_save_entry(VIRIDIAN_DOMAIN, 0, h, &ctxt) != 0);
+ return hvm_save_entry(VIRIDIAN_DOMAIN, inst, h, &ctxt);
}
static int viridian_load_domain_ctxt(struct domain *d, hvm_domain_context_t *h)
@@ -452,7 +453,8 @@ static int viridian_load_domain_ctxt(struct domain *d, hvm_domain_context_t *h)
HVM_REGISTER_SAVE_RESTORE_PER_DOM(VIRIDIAN_DOMAIN, viridian_save_domain_ctxt,
viridian_load_domain_ctxt, 1);
-static int viridian_save_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
+static int viridian_save_vcpu_ctxt(struct domain *d, uint16_t inst,
+ hvm_domain_context_t *h)
{
struct vcpu *v;
@@ -464,8 +466,8 @@ static int viridian_save_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
ctxt.apic_assist = v->arch.hvm_vcpu.viridian.apic_assist.raw;
- if ( hvm_save_entry(VIRIDIAN_VCPU, v->vcpu_id, h, &ctxt) != 0 )
- return 1;
+ if ( hvm_save_entry(VIRIDIAN_VCPU, v->vcpu_id, h, &ctxt) < 0 )
+ return -ENOSPC;
}
return 0;
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index b64b9ee..81dfd3f 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1137,7 +1137,8 @@ static void lapic_rearm(struct vlapic *s)
s->timer_last_update = s->pt.last_plt_gtime;
}
-static int lapic_save_hidden(struct domain *d, hvm_domain_context_t *h)
+static int lapic_save_hidden(struct domain *d, uint16_t inst,
+ hvm_domain_context_t *h)
{
struct vcpu *v;
struct vlapic *s;
@@ -1146,14 +1147,15 @@ static int lapic_save_hidden(struct domain *d, hvm_domain_context_t *h)
for_each_vcpu ( d, v )
{
s = vcpu_vlapic(v);
- if ( (rc = hvm_save_entry(LAPIC, v->vcpu_id, h, &s->hw)) != 0 )
+ if ( (rc = hvm_save_entry(LAPIC, v->vcpu_id, h, &s->hw)) < 0 )
break;
}
return rc;
}
-static int lapic_save_regs(struct domain *d, hvm_domain_context_t *h)
+static int lapic_save_regs(struct domain *d, uint16_t inst,
+ hvm_domain_context_t *h)
{
struct vcpu *v;
struct vlapic *s;
@@ -1162,7 +1164,7 @@ static int lapic_save_regs(struct domain *d, hvm_domain_context_t *h)
for_each_vcpu ( d, v )
{
s = vcpu_vlapic(v);
- if ( (rc = hvm_save_entry(LAPIC_REGS, v->vcpu_id, h, s->regs)) != 0 )
+ if ( (rc = hvm_save_entry(LAPIC_REGS, v->vcpu_id, h, s->regs)) < 0 )
break;
}
diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c
index e882fe1..7e4b64b 100644
--- a/xen/arch/x86/hvm/vpic.c
+++ b/xen/arch/x86/hvm/vpic.c
@@ -364,7 +364,8 @@ static int vpic_intercept_elcr_io(
return X86EMUL_OKAY;
}
-static int vpic_save(struct domain *d, hvm_domain_context_t *h)
+static int vpic_save(struct domain *d, uint16_t inst,
+ hvm_domain_context_t *h)
{
struct hvm_hw_vpic *s;
int i;
@@ -373,8 +374,8 @@ static int vpic_save(struct domain *d, hvm_domain_context_t *h)
for ( i = 0; i < 2 ; i++ )
{
s = &d->arch.hvm_domain.vpic[i];
- if ( hvm_save_entry(PIC, i, h, s) )
- return 1;
+ if ( hvm_save_entry(PIC, i, h, s) < 0 )
+ return -ENOSPC;
}
return 0;
diff --git a/xen/common/hvm/save.c b/xen/common/hvm/save.c
index 2800c5b..e9723e3 100644
--- a/xen/common/hvm/save.c
+++ b/xen/common/hvm/save.c
@@ -109,7 +109,7 @@ int hvm_save_one(struct domain *d, uint16_t typecode, uint16_t instance,
if ( !ctxt.data )
return -ENOMEM;
- if ( hvm_sr_handlers[typecode].save(d, &ctxt) != 0 )
+ if ( hvm_sr_handlers[typecode].save(d, 0, &ctxt) < 0 )
{
printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16"\n",
d->domain_id, typecode);
@@ -150,7 +150,7 @@ int hvm_save(struct domain *d, hvm_domain_context_t *h)
arch_hvm_save(d, &hdr);
- if ( hvm_save_entry(HEADER, 0, h, &hdr) != 0 )
+ if ( hvm_save_entry(HEADER, 0, h, &hdr) < 0 )
{
printk(XENLOG_G_ERR "HVM%d save: failed to write header\n",
d->domain_id);
@@ -165,7 +165,7 @@ int hvm_save(struct domain *d, hvm_domain_context_t *h)
{
printk(XENLOG_G_INFO "HVM%d save: %s\n",
d->domain_id, hvm_sr_handlers[i].name);
- if ( handler(d, h) != 0 )
+ if ( handler(d, 0, h) < 0 )
{
printk(XENLOG_G_ERR
"HVM%d save: failed to save type %"PRIu16"\n",
@@ -176,7 +176,7 @@ int hvm_save(struct domain *d, hvm_domain_context_t *h)
}
/* Save an end-of-file marker */
- if ( hvm_save_entry(END, 0, h, &end) != 0 )
+ if ( hvm_save_entry(END, 0, h, &end) < 0 )
{
/* Run out of data */
printk(XENLOG_G_ERR "HVM%d save: no room for end marker\n",
@@ -260,20 +260,21 @@ int _hvm_init_entry(struct hvm_domain_context *h,
printk(XENLOG_G_WARNING "HVM save: no room for"
" %"PRIu32" + %zu bytes for typecode %"PRIu16"\n",
len, sizeof(*d), tc);
- return -1;
+ return -ENOSPC;
}
d->typecode = tc;
d->instance = inst;
d->length = len;
h->cur += sizeof(*d);
- return 0;
+ return sizeof(*d);
}
-void _hvm_write_entry(struct hvm_domain_context *h,
+int _hvm_write_entry(struct hvm_domain_context *h,
void *src, uint32_t src_len)
{
memcpy(&h->data[h->cur], src, src_len);
h->cur += src_len;
+ return src_len;
}
int _hvm_check_entry(struct hvm_domain_context *h,
diff --git a/xen/include/xen/hvm/save.h b/xen/include/xen/hvm/save.h
index 0e3ef13..b2dca48 100644
--- a/xen/include/xen/hvm/save.h
+++ b/xen/include/xen/hvm/save.h
@@ -30,21 +30,23 @@ typedef struct hvm_domain_context {
uint8_t *data;
} hvm_domain_context_t;
-/* Marshalling an entry: check space and fill in the header */
+/* Marshalling an entry: check space and fill in the header. Returns the
+ * number of bytes written, or negative for an error. */
int _hvm_init_entry(struct hvm_domain_context *h,
uint16_t tc, uint16_t inst, uint32_t len);
-/* Marshalling: copy the contents in a type-safe way */
-void _hvm_write_entry(struct hvm_domain_context *h,
- void *src, uint32_t src_len);
+/* Marshalling: copy the contents in a type-safe way. Returns the number of
+ * bytes written. */
+int _hvm_write_entry(struct hvm_domain_context *h,
+ void *src, uint32_t src_len);
/* Marshalling: init and copy; evaluates to zero on success */
#define hvm_save_entry(_x, _inst, _h, _src) ({ \
int r; \
r = _hvm_init_entry((_h), HVM_SAVE_CODE(_x), \
(_inst), HVM_SAVE_LENGTH(_x)); \
- if ( r == 0 ) \
- _hvm_write_entry((_h), (_src), HVM_SAVE_LENGTH(_x)); \
+ if ( r >= 0 ) \
+ r += _hvm_write_entry((_h), (_src), HVM_SAVE_LENGTH(_x)); \
r; })
/* Unmarshalling: test an entry's size and typecode and record the instance */
@@ -85,11 +87,19 @@ static inline uint16_t hvm_load_instance(struct hvm_domain_context *h)
return d->instance;
}
-/* Handler types for different types of save-file entry.
- * The save handler may save multiple instances of a type into the buffer;
- * the load handler will be called once for each instance found when
- * restoring. Both return non-zero on error. */
+/* Handler types for different types of save-file entry.
+ *
+ * The save handler will be called once for each instance (either the number
+ * of per-domain instances, or for each vcpu). The caller ensures that the
+ * instance parameter is always valid in context (i.e. for a per-vcpu type,
+ * instance refers to a valid vcpu). It returns negative for an error, or the
+ * number of bytes written.
+ *
+ * The load handler will be called once for each instance found when
+ * restoring. It returns non-zero for an error.
+ */
typedef int (*hvm_save_handler) (struct domain *d,
+ uint16_t instance,
hvm_domain_context_t *h);
typedef int (*hvm_load_handler) (struct domain *d,
hvm_domain_context_t *h);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* [RFC PATCH 3/3] xen/hvm-save: Adjust calling of multi-instance save handlers.
2013-12-16 2:17 [RFC PATCH 0/3] Xen hvm saving improvements Andrew Cooper
2013-12-16 2:17 ` [RFC PATCH 1/3] xen/hvm-save: Refactor HVM_REGISTER_SAVE_RESTORE Andrew Cooper
2013-12-16 2:17 ` [RFC PATCH 2/3] xen/hvm-save: Extend hvm_save_handler to take an instance parameter Andrew Cooper
@ 2013-12-16 2:17 ` Andrew Cooper
2013-12-17 15:01 ` Tim Deegan
2013-12-16 9:44 ` [RFC PATCH 0/3] Xen hvm saving improvements Ian Campbell
2013-12-16 17:50 ` Don Slutz
4 siblings, 1 reply; 9+ messages in thread
From: Andrew Cooper @ 2013-12-16 2:17 UTC (permalink / raw)
To: Xen-devel
Cc: George Dunlap, Andrew Cooper, Keir Fraser, Don Slutz, Jan Beulich
Alter the calling logic so hvm_save and hvm_save_one are responsible for
calling the save handlers with appropriate instance IDs (and are responsible
for ensuring the validity of the instance parameter).
This involves fairly substantial changes to each of the save handler bodies
for records expecting to use multiple instances (all the per-vcpu ones, and
the PIC record).
Where sensible, refactoring has also involved changing the functions to write
directly into the context buffer, rather than writing to a context structure
on the stack and pointlessly copying.
The only observable change should be that hvm_save_one now extracts the
correct data in all cases, rather than being wrong for PIC records, variable
length records and per-vcpu records when one or more vcpus are offline. There
should be no difference whatsoever in the result from hvm_save.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Discovered-by: Don Slutz <dslutz@verizon.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Don Slutz <dslutz@verizon.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
xen/arch/x86/cpu/mcheck/vmce.c | 27 ++---
xen/arch/x86/hvm/hvm.c | 225 +++++++++++++++++++---------------------
xen/arch/x86/hvm/mtrr.c | 55 +++++-----
xen/arch/x86/hvm/viridian.c | 15 +--
xen/arch/x86/hvm/vlapic.c | 26 +----
xen/arch/x86/hvm/vpic.c | 13 +--
xen/common/hvm/save.c | 86 +++++++++------
7 files changed, 213 insertions(+), 234 deletions(-)
diff --git a/xen/arch/x86/cpu/mcheck/vmce.c b/xen/arch/x86/cpu/mcheck/vmce.c
index 8ef40c3..18ef18a 100644
--- a/xen/arch/x86/cpu/mcheck/vmce.c
+++ b/xen/arch/x86/cpu/mcheck/vmce.c
@@ -299,22 +299,23 @@ int vmce_wrmsr(uint32_t msr, uint64_t val)
static int vmce_save_vcpu_ctxt(struct domain *d, uint16_t inst,
hvm_domain_context_t *h)
{
- struct vcpu *v;
- int err = 0;
+ struct vcpu *v = d->vcpu[inst];
+ int rc = 0;
+ struct hvm_vmce_vcpu *rec;
- for_each_vcpu( d, v ) {
- struct hvm_vmce_vcpu ctxt = {
- .caps = v->arch.vmce.mcg_cap,
- .mci_ctl2_bank0 = v->arch.vmce.bank[0].mci_ctl2,
- .mci_ctl2_bank1 = v->arch.vmce.bank[1].mci_ctl2
- };
+ rc = _hvm_init_entry(h, HVM_SAVE_CODE(VMCE_VCPU),
+ inst, HVM_SAVE_LENGTH(VMCE_VCPU));
+ if ( rc < 0 )
+ return rc;
- err = hvm_save_entry(VMCE_VCPU, v->vcpu_id, h, &ctxt);
- if ( err < 0 )
- break;
- }
+ rec = (struct hvm_vmce_vcpu *)&h->data[h->cur];
+ h->cur += HVM_SAVE_LENGTH(VMCE_VCPU);
+
+ rec->caps = v->arch.vmce.mcg_cap;
+ rec->mci_ctl2_bank0 = v->arch.vmce.bank[0].mci_ctl2;
+ rec->mci_ctl2_bank1 = v->arch.vmce.bank[1].mci_ctl2;
- return err;
+ return rc + HVM_SAVE_LENGTH(VMCE_VCPU);
}
static int vmce_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 925e792..bf1901a 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -665,19 +665,8 @@ void hvm_domain_destroy(struct domain *d)
static int hvm_save_tsc_adjust(struct domain *d, uint16_t inst,
hvm_domain_context_t *h)
{
- struct vcpu *v;
- struct hvm_tsc_adjust ctxt;
- int err = 0;
-
- for_each_vcpu ( d, v )
- {
- ctxt.tsc_adjust = v->arch.hvm_vcpu.msr_tsc_adjust;
- err = hvm_save_entry(TSC_ADJUST, v->vcpu_id, h, &ctxt);
- if ( err < 0 )
- break;
- }
-
- return err;
+ return hvm_save_entry(TSC_ADJUST, inst, h,
+ &d->vcpu[inst]->arch.hvm_vcpu.msr_tsc_adjust);
}
static int hvm_load_tsc_adjust(struct domain *d, hvm_domain_context_t *h)
@@ -706,112 +695,116 @@ HVM_REGISTER_SAVE_RESTORE_PER_VCPU(TSC_ADJUST, hvm_save_tsc_adjust,
static int hvm_save_cpu_ctxt(struct domain *d, uint16_t inst,
hvm_domain_context_t *h)
{
- struct vcpu *v;
- struct hvm_hw_cpu ctxt;
+ struct vcpu *v = d->vcpu[inst];
+ struct hvm_hw_cpu *rec;
struct segment_register seg;
+ int rc = 0;
- for_each_vcpu ( d, v )
- {
- /* We don't need to save state for a vcpu that is down; the restore
- * code will leave it down if there is nothing saved. */
- if ( test_bit(_VPF_down, &v->pause_flags) )
- continue;
+ /* We don't need to save state for a vcpu that is down; the restore
+ * code will leave it down if there is nothing saved. */
+ if ( test_bit(_VPF_down, &v->pause_flags) )
+ return rc;
- /* Architecture-specific vmcs/vmcb bits */
- hvm_funcs.save_cpu_ctxt(v, &ctxt);
-
- ctxt.msr_tsc_aux = hvm_msr_tsc_aux(v);
-
- hvm_get_segment_register(v, x86_seg_idtr, &seg);
- ctxt.idtr_limit = seg.limit;
- ctxt.idtr_base = seg.base;
-
- hvm_get_segment_register(v, x86_seg_gdtr, &seg);
- ctxt.gdtr_limit = seg.limit;
- ctxt.gdtr_base = seg.base;
-
- hvm_get_segment_register(v, x86_seg_cs, &seg);
- ctxt.cs_sel = seg.sel;
- ctxt.cs_limit = seg.limit;
- ctxt.cs_base = seg.base;
- ctxt.cs_arbytes = seg.attr.bytes;
-
- hvm_get_segment_register(v, x86_seg_ds, &seg);
- ctxt.ds_sel = seg.sel;
- ctxt.ds_limit = seg.limit;
- ctxt.ds_base = seg.base;
- ctxt.ds_arbytes = seg.attr.bytes;
-
- hvm_get_segment_register(v, x86_seg_es, &seg);
- ctxt.es_sel = seg.sel;
- ctxt.es_limit = seg.limit;
- ctxt.es_base = seg.base;
- ctxt.es_arbytes = seg.attr.bytes;
-
- hvm_get_segment_register(v, x86_seg_ss, &seg);
- ctxt.ss_sel = seg.sel;
- ctxt.ss_limit = seg.limit;
- ctxt.ss_base = seg.base;
- ctxt.ss_arbytes = seg.attr.bytes;
-
- hvm_get_segment_register(v, x86_seg_fs, &seg);
- ctxt.fs_sel = seg.sel;
- ctxt.fs_limit = seg.limit;
- ctxt.fs_base = seg.base;
- ctxt.fs_arbytes = seg.attr.bytes;
-
- hvm_get_segment_register(v, x86_seg_gs, &seg);
- ctxt.gs_sel = seg.sel;
- ctxt.gs_limit = seg.limit;
- ctxt.gs_base = seg.base;
- ctxt.gs_arbytes = seg.attr.bytes;
-
- hvm_get_segment_register(v, x86_seg_tr, &seg);
- ctxt.tr_sel = seg.sel;
- ctxt.tr_limit = seg.limit;
- ctxt.tr_base = seg.base;
- ctxt.tr_arbytes = seg.attr.bytes;
-
- hvm_get_segment_register(v, x86_seg_ldtr, &seg);
- ctxt.ldtr_sel = seg.sel;
- ctxt.ldtr_limit = seg.limit;
- ctxt.ldtr_base = seg.base;
- ctxt.ldtr_arbytes = seg.attr.bytes;
-
- if ( v->fpu_initialised )
- memcpy(ctxt.fpu_regs, v->arch.fpu_ctxt, sizeof(ctxt.fpu_regs));
- else
- memset(ctxt.fpu_regs, 0, sizeof(ctxt.fpu_regs));
-
- ctxt.rax = v->arch.user_regs.eax;
- ctxt.rbx = v->arch.user_regs.ebx;
- ctxt.rcx = v->arch.user_regs.ecx;
- ctxt.rdx = v->arch.user_regs.edx;
- ctxt.rbp = v->arch.user_regs.ebp;
- ctxt.rsi = v->arch.user_regs.esi;
- ctxt.rdi = v->arch.user_regs.edi;
- ctxt.rsp = v->arch.user_regs.esp;
- ctxt.rip = v->arch.user_regs.eip;
- ctxt.rflags = v->arch.user_regs.eflags;
- ctxt.r8 = v->arch.user_regs.r8;
- ctxt.r9 = v->arch.user_regs.r9;
- ctxt.r10 = v->arch.user_regs.r10;
- ctxt.r11 = v->arch.user_regs.r11;
- ctxt.r12 = v->arch.user_regs.r12;
- ctxt.r13 = v->arch.user_regs.r13;
- ctxt.r14 = v->arch.user_regs.r14;
- ctxt.r15 = v->arch.user_regs.r15;
- ctxt.dr0 = v->arch.debugreg[0];
- ctxt.dr1 = v->arch.debugreg[1];
- ctxt.dr2 = v->arch.debugreg[2];
- ctxt.dr3 = v->arch.debugreg[3];
- ctxt.dr6 = v->arch.debugreg[6];
- ctxt.dr7 = v->arch.debugreg[7];
-
- if ( hvm_save_entry(CPU, v->vcpu_id, h, &ctxt) < 0 )
- return -ENOSPC;
- }
- return 0;
+ rc = _hvm_init_entry(h, HVM_SAVE_CODE(CPU),
+ inst, HVM_SAVE_LENGTH(CPU));
+ if ( rc < 0 )
+ return rc;
+
+ rec = (struct hvm_hw_cpu *)&h->data[h->cur];
+ h->cur += HVM_SAVE_LENGTH(CPU);
+
+ /* Architecture-specific vmcs/vmcb bits */
+ hvm_funcs.save_cpu_ctxt(v, rec);
+
+ rec->msr_tsc_aux = hvm_msr_tsc_aux(v);
+
+ hvm_get_segment_register(v, x86_seg_idtr, &seg);
+ rec->idtr_limit = seg.limit;
+ rec->idtr_base = seg.base;
+
+ hvm_get_segment_register(v, x86_seg_gdtr, &seg);
+ rec->gdtr_limit = seg.limit;
+ rec->gdtr_base = seg.base;
+
+ hvm_get_segment_register(v, x86_seg_cs, &seg);
+ rec->cs_sel = seg.sel;
+ rec->cs_limit = seg.limit;
+ rec->cs_base = seg.base;
+ rec->cs_arbytes = seg.attr.bytes;
+
+ hvm_get_segment_register(v, x86_seg_ds, &seg);
+ rec->ds_sel = seg.sel;
+ rec->ds_limit = seg.limit;
+ rec->ds_base = seg.base;
+ rec->ds_arbytes = seg.attr.bytes;
+
+ hvm_get_segment_register(v, x86_seg_es, &seg);
+ rec->es_sel = seg.sel;
+ rec->es_limit = seg.limit;
+ rec->es_base = seg.base;
+ rec->es_arbytes = seg.attr.bytes;
+
+ hvm_get_segment_register(v, x86_seg_ss, &seg);
+ rec->ss_sel = seg.sel;
+ rec->ss_limit = seg.limit;
+ rec->ss_base = seg.base;
+ rec->ss_arbytes = seg.attr.bytes;
+
+ hvm_get_segment_register(v, x86_seg_fs, &seg);
+ rec->fs_sel = seg.sel;
+ rec->fs_limit = seg.limit;
+ rec->fs_base = seg.base;
+ rec->fs_arbytes = seg.attr.bytes;
+
+ hvm_get_segment_register(v, x86_seg_gs, &seg);
+ rec->gs_sel = seg.sel;
+ rec->gs_limit = seg.limit;
+ rec->gs_base = seg.base;
+ rec->gs_arbytes = seg.attr.bytes;
+
+ hvm_get_segment_register(v, x86_seg_tr, &seg);
+ rec->tr_sel = seg.sel;
+ rec->tr_limit = seg.limit;
+ rec->tr_base = seg.base;
+ rec->tr_arbytes = seg.attr.bytes;
+
+ hvm_get_segment_register(v, x86_seg_ldtr, &seg);
+ rec->ldtr_sel = seg.sel;
+ rec->ldtr_limit = seg.limit;
+ rec->ldtr_base = seg.base;
+ rec->ldtr_arbytes = seg.attr.bytes;
+
+ if ( v->fpu_initialised )
+ memcpy(rec->fpu_regs, v->arch.fpu_ctxt, sizeof(rec->fpu_regs));
+ else
+ memset(rec->fpu_regs, 0, sizeof(rec->fpu_regs));
+
+ rec->rax = v->arch.user_regs.eax;
+ rec->rbx = v->arch.user_regs.ebx;
+ rec->rcx = v->arch.user_regs.ecx;
+ rec->rdx = v->arch.user_regs.edx;
+ rec->rbp = v->arch.user_regs.ebp;
+ rec->rsi = v->arch.user_regs.esi;
+ rec->rdi = v->arch.user_regs.edi;
+ rec->rsp = v->arch.user_regs.esp;
+ rec->rip = v->arch.user_regs.eip;
+ rec->rflags = v->arch.user_regs.eflags;
+ rec->r8 = v->arch.user_regs.r8;
+ rec->r9 = v->arch.user_regs.r9;
+ rec->r10 = v->arch.user_regs.r10;
+ rec->r11 = v->arch.user_regs.r11;
+ rec->r12 = v->arch.user_regs.r12;
+ rec->r13 = v->arch.user_regs.r13;
+ rec->r14 = v->arch.user_regs.r14;
+ rec->r15 = v->arch.user_regs.r15;
+ rec->dr0 = v->arch.debugreg[0];
+ rec->dr1 = v->arch.debugreg[1];
+ rec->dr2 = v->arch.debugreg[2];
+ rec->dr3 = v->arch.debugreg[3];
+ rec->dr6 = v->arch.debugreg[6];
+ rec->dr7 = v->arch.debugreg[7];
+
+ return rc + HVM_SAVE_LENGTH(CPU);
}
static bool_t hvm_efer_valid(struct domain *d,
diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 40f58ed..389138d 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -603,39 +603,40 @@ int32_t hvm_set_mem_pinned_cacheattr(
static int hvm_save_mtrr_msr(struct domain *d, uint16_t inst,
hvm_domain_context_t *h)
{
- int i;
- struct vcpu *v;
- struct hvm_hw_mtrr hw_mtrr;
- struct mtrr_state *mtrr_state;
- /* save mtrr&pat */
- for_each_vcpu(d, v)
- {
- mtrr_state = &v->arch.hvm_vcpu.mtrr;
+ int i, rc;
+ struct vcpu *v = d->vcpu[inst];
+ struct mtrr_state *mtrr_state = &v->arch.hvm_vcpu.mtrr;
+ struct hvm_hw_mtrr *rec;
- hvm_get_guest_pat(v, &hw_mtrr.msr_pat_cr);
+ rc = _hvm_init_entry(h, HVM_SAVE_CODE(MTRR),
+ inst, HVM_SAVE_LENGTH(MTRR));
+ if ( rc < 0 )
+ return rc;
- hw_mtrr.msr_mtrr_def_type = mtrr_state->def_type
- | (mtrr_state->enabled << 10);
- hw_mtrr.msr_mtrr_cap = mtrr_state->mtrr_cap;
+ rec = (struct hvm_hw_mtrr *)&h->data[h->cur];
+ h->cur += HVM_SAVE_LENGTH(MTRR);
- for ( i = 0; i < MTRR_VCNT; i++ )
- {
- /* save physbase */
- hw_mtrr.msr_mtrr_var[i*2] =
- ((uint64_t*)mtrr_state->var_ranges)[i*2];
- /* save physmask */
- hw_mtrr.msr_mtrr_var[i*2+1] =
- ((uint64_t*)mtrr_state->var_ranges)[i*2+1];
- }
+ hvm_get_guest_pat(v, &rec->msr_pat_cr);
- for ( i = 0; i < NUM_FIXED_MSR; i++ )
- hw_mtrr.msr_mtrr_fixed[i] =
- ((uint64_t*)mtrr_state->fixed_ranges)[i];
+ rec->msr_mtrr_def_type = mtrr_state->def_type
+ | (mtrr_state->enabled << 10);
+ rec->msr_mtrr_cap = mtrr_state->mtrr_cap;
- if ( hvm_save_entry(MTRR, v->vcpu_id, h, &hw_mtrr) < 0 )
- return -ENOSPC;
+ for ( i = 0; i < MTRR_VCNT; i++ )
+ {
+ /* save physbase */
+ rec->msr_mtrr_var[i*2] =
+ ((uint64_t*)mtrr_state->var_ranges)[i*2];
+ /* save physmask */
+ rec->msr_mtrr_var[i*2+1] =
+ ((uint64_t*)mtrr_state->var_ranges)[i*2+1];
}
- return 0;
+
+ for ( i = 0; i < NUM_FIXED_MSR; i++ )
+ rec->msr_mtrr_fixed[i] =
+ ((uint64_t*)mtrr_state->fixed_ranges)[i];
+
+ return rc + HVM_SAVE_LENGTH(MTRR);
}
static int hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index 4f9186d..dfb01d5 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -456,21 +456,12 @@ HVM_REGISTER_SAVE_RESTORE_PER_DOM(VIRIDIAN_DOMAIN, viridian_save_domain_ctxt,
static int viridian_save_vcpu_ctxt(struct domain *d, uint16_t inst,
hvm_domain_context_t *h)
{
- struct vcpu *v;
-
if ( !is_viridian_domain(d) )
return 0;
- for_each_vcpu( d, v ) {
- struct hvm_viridian_vcpu_context ctxt;
-
- ctxt.apic_assist = v->arch.hvm_vcpu.viridian.apic_assist.raw;
-
- if ( hvm_save_entry(VIRIDIAN_VCPU, v->vcpu_id, h, &ctxt) < 0 )
- return -ENOSPC;
- }
-
- return 0;
+ return hvm_save_entry(
+ VIRIDIAN_VCPU, inst, h,
+ &d->vcpu[inst]->arch.hvm_vcpu.viridian.apic_assist.raw);
}
static int viridian_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 81dfd3f..bb1438a 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1140,35 +1140,17 @@ static void lapic_rearm(struct vlapic *s)
static int lapic_save_hidden(struct domain *d, uint16_t inst,
hvm_domain_context_t *h)
{
- struct vcpu *v;
- struct vlapic *s;
- int rc = 0;
-
- for_each_vcpu ( d, v )
- {
- s = vcpu_vlapic(v);
- if ( (rc = hvm_save_entry(LAPIC, v->vcpu_id, h, &s->hw)) < 0 )
- break;
- }
+ struct vlapic *s = vcpu_vlapic(d->vcpu[inst]);
- return rc;
+ return hvm_save_entry(LAPIC, inst, h, &s->hw);
}
static int lapic_save_regs(struct domain *d, uint16_t inst,
hvm_domain_context_t *h)
{
- struct vcpu *v;
- struct vlapic *s;
- int rc = 0;
-
- for_each_vcpu ( d, v )
- {
- s = vcpu_vlapic(v);
- if ( (rc = hvm_save_entry(LAPIC_REGS, v->vcpu_id, h, s->regs)) < 0 )
- break;
- }
+ struct vlapic *s = vcpu_vlapic(d->vcpu[inst]);
- return rc;
+ return hvm_save_entry(LAPIC_REGS, inst, h, &s->regs);
}
static int lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c
index 7e4b64b..416e970 100644
--- a/xen/arch/x86/hvm/vpic.c
+++ b/xen/arch/x86/hvm/vpic.c
@@ -367,18 +367,9 @@ static int vpic_intercept_elcr_io(
static int vpic_save(struct domain *d, uint16_t inst,
hvm_domain_context_t *h)
{
- struct hvm_hw_vpic *s;
- int i;
+ struct hvm_hw_vpic *s = &d->arch.hvm_domain.vpic[inst];
- /* Save the state of both PICs */
- for ( i = 0; i < 2 ; i++ )
- {
- s = &d->arch.hvm_domain.vpic[i];
- if ( hvm_save_entry(PIC, i, h, s) < 0 )
- return -ENOSPC;
- }
-
- return 0;
+ return hvm_save_entry(PIC, inst, h, s);
}
static int vpic_load(struct domain *d, hvm_domain_context_t *h)
diff --git a/xen/common/hvm/save.c b/xen/common/hvm/save.c
index e9723e3..8eb9672 100644
--- a/xen/common/hvm/save.c
+++ b/xen/common/hvm/save.c
@@ -84,44 +84,42 @@ size_t hvm_save_size(struct domain *d)
int hvm_save_one(struct domain *d, uint16_t typecode, uint16_t instance,
XEN_GUEST_HANDLE_64(uint8) handle)
{
- int rv = 0;
- size_t sz = 0;
- struct vcpu *v;
- hvm_domain_context_t ctxt = { 0, };
+ int rv;
+ hvm_domain_context_t ctxt = { 0 };
- if ( d->is_dying
- || typecode > HVM_SAVE_CODE_MAX
- || hvm_sr_handlers[typecode].size < sizeof(struct hvm_save_descriptor)
- || hvm_sr_handlers[typecode].save == NULL )
+ if ( d->is_dying || typecode > HVM_SAVE_CODE_MAX )
return -EINVAL;
- if ( is_per_vcpu_handler(hvm_sr_handlers[typecode]) )
- for_each_vcpu(d, v)
- sz += hvm_sr_handlers[typecode].size;
- else
- sz = hvm_sr_handlers[typecode].size;
-
- if ( (instance + 1) * hvm_sr_handlers[typecode].size > sz )
+ if ( hvm_sr_handlers[typecode].size < sizeof(struct hvm_save_descriptor) ||
+ hvm_sr_handlers[typecode].save == NULL )
return -EINVAL;
- ctxt.size = sz;
- ctxt.data = xmalloc_bytes(sz);
+ if ( (is_per_vcpu_handler(hvm_sr_handlers[typecode]) &&
+ (instance >= d->max_vcpus || d->vcpu[instance] == NULL)) ||
+ (instance >= hvm_sr_handlers[typecode].num) )
+ return -EBADSLT;
+
+ ctxt.size = hvm_sr_handlers[typecode].size;
+ ctxt.data = xmalloc_bytes(hvm_sr_handlers[typecode].size);
if ( !ctxt.data )
return -ENOMEM;
- if ( hvm_sr_handlers[typecode].save(d, 0, &ctxt) < 0 )
+ rv = hvm_sr_handlers[typecode].save(d, instance, &ctxt);
+
+ if ( rv < 0 )
{
- printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16"\n",
- d->domain_id, typecode);
+ printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16
+ ", instance %"PRIu16"\n",
+ d->domain_id, typecode, instance);
rv = -EFAULT;
}
- else if ( copy_to_guest(handle,
- ctxt.data
- + (instance * hvm_sr_handlers[typecode].size)
- + sizeof (struct hvm_save_descriptor),
- hvm_sr_handlers[typecode].size
- - sizeof (struct hvm_save_descriptor)) )
- rv = -EFAULT;
+ else if ( rv <= sizeof (struct hvm_save_descriptor) )
+ rv = -ENODATA;
+ else
+ rv = copy_to_guest(handle,
+ ctxt.data + sizeof (struct hvm_save_descriptor),
+ rv - sizeof (struct hvm_save_descriptor))
+ ? -EFAULT : 0;
xfree(ctxt.data);
return rv;
@@ -165,13 +163,35 @@ int hvm_save(struct domain *d, hvm_domain_context_t *h)
{
printk(XENLOG_G_INFO "HVM%d save: %s\n",
d->domain_id, hvm_sr_handlers[i].name);
- if ( handler(d, 0, h) < 0 )
+
+ if ( is_per_vcpu_handler(hvm_sr_handlers[i]) )
+ {
+ struct vcpu *v;
+
+ for_each_vcpu( d, v )
+ if ( handler(d, v->vcpu_id, h) < 0 )
+ {
+ printk(XENLOG_G_ERR
+ "HVM%d save: failed to save type %"PRIu16
+ ", instance %"PRIu16"\n",
+ d->domain_id, i, v->vcpu_id);
+ return -EFAULT;
+ }
+ }
+ else
{
- printk(XENLOG_G_ERR
- "HVM%d save: failed to save type %"PRIu16"\n",
- d->domain_id, i);
- return -EFAULT;
- }
+ int j;
+
+ for ( j = 0; j < hvm_sr_handlers[i].num; ++j )
+ if ( handler(d, j, h) < 0 )
+ {
+ printk(XENLOG_G_ERR
+ "HVM%d save: failed to save type %"PRIu16
+ ", instance %"PRIu16"\n",
+ d->domain_id, i, j);
+ return -EFAULT;
+ }
+ }
}
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [RFC PATCH 3/3] xen/hvm-save: Adjust calling of multi-instance save handlers.
2013-12-16 2:17 ` [RFC PATCH 3/3] xen/hvm-save: Adjust calling of multi-instance save handlers Andrew Cooper
@ 2013-12-17 15:01 ` Tim Deegan
0 siblings, 0 replies; 9+ messages in thread
From: Tim Deegan @ 2013-12-17 15:01 UTC (permalink / raw)
To: Andrew Cooper
Cc: George Dunlap, Keir Fraser, Jan Beulich, Don Slutz, Xen-devel
At 02:17 +0000 on 16 Dec (1387156659), Andrew Cooper wrote:
> Alter the calling logic so hvm_save and hvm_save_one are responsible for
> calling the save handlers with appropriate instance IDs (and are responsible
> for ensuring the validity of the instance parameter).
>
> This involves fairly substantial changes to each of the save handler bodies
> for records expecting to use multiple instances (all the per-vcpu ones, and
> the PIC record).
>
> Where sensible, refactoring has also involved changing the functions to write
> directly into the context buffer, rather than writing to a context structure
> on the stack and pointlessly copying.
It wasn't pointlessly copying, it was copying to avoid having the HVM
buffer internals visible in the callers - i.e. you just called
hvm_save_entry() with an appropriate struct.
HVM save/restore is not performance-critical and I'd rather keep the
moving parts hidden, at the cost of copying a few bytes from a hot
cache line.
Tim.
> The only observable change should be that hvm_save_one now extracts the
> correct data in all cases, rather than being wrong for PIC records, variable
> length records and per-vcpu records when one or more vcpus are offline. There
> should be no difference whatsoever in the result from hvm_save.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Discovered-by: Don Slutz <dslutz@verizon.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Don Slutz <dslutz@verizon.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> xen/arch/x86/cpu/mcheck/vmce.c | 27 ++---
> xen/arch/x86/hvm/hvm.c | 225 +++++++++++++++++++---------------------
> xen/arch/x86/hvm/mtrr.c | 55 +++++-----
> xen/arch/x86/hvm/viridian.c | 15 +--
> xen/arch/x86/hvm/vlapic.c | 26 +----
> xen/arch/x86/hvm/vpic.c | 13 +--
> xen/common/hvm/save.c | 86 +++++++++------
> 7 files changed, 213 insertions(+), 234 deletions(-)
>
> diff --git a/xen/arch/x86/cpu/mcheck/vmce.c b/xen/arch/x86/cpu/mcheck/vmce.c
> index 8ef40c3..18ef18a 100644
> --- a/xen/arch/x86/cpu/mcheck/vmce.c
> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
> @@ -299,22 +299,23 @@ int vmce_wrmsr(uint32_t msr, uint64_t val)
> static int vmce_save_vcpu_ctxt(struct domain *d, uint16_t inst,
> hvm_domain_context_t *h)
> {
> - struct vcpu *v;
> - int err = 0;
> + struct vcpu *v = d->vcpu[inst];
> + int rc = 0;
> + struct hvm_vmce_vcpu *rec;
>
> - for_each_vcpu( d, v ) {
> - struct hvm_vmce_vcpu ctxt = {
> - .caps = v->arch.vmce.mcg_cap,
> - .mci_ctl2_bank0 = v->arch.vmce.bank[0].mci_ctl2,
> - .mci_ctl2_bank1 = v->arch.vmce.bank[1].mci_ctl2
> - };
> + rc = _hvm_init_entry(h, HVM_SAVE_CODE(VMCE_VCPU),
> + inst, HVM_SAVE_LENGTH(VMCE_VCPU));
> + if ( rc < 0 )
> + return rc;
>
> - err = hvm_save_entry(VMCE_VCPU, v->vcpu_id, h, &ctxt);
> - if ( err < 0 )
> - break;
> - }
> + rec = (struct hvm_vmce_vcpu *)&h->data[h->cur];
> + h->cur += HVM_SAVE_LENGTH(VMCE_VCPU);
> +
> + rec->caps = v->arch.vmce.mcg_cap;
> + rec->mci_ctl2_bank0 = v->arch.vmce.bank[0].mci_ctl2;
> + rec->mci_ctl2_bank1 = v->arch.vmce.bank[1].mci_ctl2;
>
> - return err;
> + return rc + HVM_SAVE_LENGTH(VMCE_VCPU);
> }
>
> static int vmce_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 925e792..bf1901a 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -665,19 +665,8 @@ void hvm_domain_destroy(struct domain *d)
> static int hvm_save_tsc_adjust(struct domain *d, uint16_t inst,
> hvm_domain_context_t *h)
> {
> - struct vcpu *v;
> - struct hvm_tsc_adjust ctxt;
> - int err = 0;
> -
> - for_each_vcpu ( d, v )
> - {
> - ctxt.tsc_adjust = v->arch.hvm_vcpu.msr_tsc_adjust;
> - err = hvm_save_entry(TSC_ADJUST, v->vcpu_id, h, &ctxt);
> - if ( err < 0 )
> - break;
> - }
> -
> - return err;
> + return hvm_save_entry(TSC_ADJUST, inst, h,
> + &d->vcpu[inst]->arch.hvm_vcpu.msr_tsc_adjust);
> }
>
> static int hvm_load_tsc_adjust(struct domain *d, hvm_domain_context_t *h)
> @@ -706,112 +695,116 @@ HVM_REGISTER_SAVE_RESTORE_PER_VCPU(TSC_ADJUST, hvm_save_tsc_adjust,
> static int hvm_save_cpu_ctxt(struct domain *d, uint16_t inst,
> hvm_domain_context_t *h)
> {
> - struct vcpu *v;
> - struct hvm_hw_cpu ctxt;
> + struct vcpu *v = d->vcpu[inst];
> + struct hvm_hw_cpu *rec;
> struct segment_register seg;
> + int rc = 0;
>
> - for_each_vcpu ( d, v )
> - {
> - /* We don't need to save state for a vcpu that is down; the restore
> - * code will leave it down if there is nothing saved. */
> - if ( test_bit(_VPF_down, &v->pause_flags) )
> - continue;
> + /* We don't need to save state for a vcpu that is down; the restore
> + * code will leave it down if there is nothing saved. */
> + if ( test_bit(_VPF_down, &v->pause_flags) )
> + return rc;
>
> - /* Architecture-specific vmcs/vmcb bits */
> - hvm_funcs.save_cpu_ctxt(v, &ctxt);
> -
> - ctxt.msr_tsc_aux = hvm_msr_tsc_aux(v);
> -
> - hvm_get_segment_register(v, x86_seg_idtr, &seg);
> - ctxt.idtr_limit = seg.limit;
> - ctxt.idtr_base = seg.base;
> -
> - hvm_get_segment_register(v, x86_seg_gdtr, &seg);
> - ctxt.gdtr_limit = seg.limit;
> - ctxt.gdtr_base = seg.base;
> -
> - hvm_get_segment_register(v, x86_seg_cs, &seg);
> - ctxt.cs_sel = seg.sel;
> - ctxt.cs_limit = seg.limit;
> - ctxt.cs_base = seg.base;
> - ctxt.cs_arbytes = seg.attr.bytes;
> -
> - hvm_get_segment_register(v, x86_seg_ds, &seg);
> - ctxt.ds_sel = seg.sel;
> - ctxt.ds_limit = seg.limit;
> - ctxt.ds_base = seg.base;
> - ctxt.ds_arbytes = seg.attr.bytes;
> -
> - hvm_get_segment_register(v, x86_seg_es, &seg);
> - ctxt.es_sel = seg.sel;
> - ctxt.es_limit = seg.limit;
> - ctxt.es_base = seg.base;
> - ctxt.es_arbytes = seg.attr.bytes;
> -
> - hvm_get_segment_register(v, x86_seg_ss, &seg);
> - ctxt.ss_sel = seg.sel;
> - ctxt.ss_limit = seg.limit;
> - ctxt.ss_base = seg.base;
> - ctxt.ss_arbytes = seg.attr.bytes;
> -
> - hvm_get_segment_register(v, x86_seg_fs, &seg);
> - ctxt.fs_sel = seg.sel;
> - ctxt.fs_limit = seg.limit;
> - ctxt.fs_base = seg.base;
> - ctxt.fs_arbytes = seg.attr.bytes;
> -
> - hvm_get_segment_register(v, x86_seg_gs, &seg);
> - ctxt.gs_sel = seg.sel;
> - ctxt.gs_limit = seg.limit;
> - ctxt.gs_base = seg.base;
> - ctxt.gs_arbytes = seg.attr.bytes;
> -
> - hvm_get_segment_register(v, x86_seg_tr, &seg);
> - ctxt.tr_sel = seg.sel;
> - ctxt.tr_limit = seg.limit;
> - ctxt.tr_base = seg.base;
> - ctxt.tr_arbytes = seg.attr.bytes;
> -
> - hvm_get_segment_register(v, x86_seg_ldtr, &seg);
> - ctxt.ldtr_sel = seg.sel;
> - ctxt.ldtr_limit = seg.limit;
> - ctxt.ldtr_base = seg.base;
> - ctxt.ldtr_arbytes = seg.attr.bytes;
> -
> - if ( v->fpu_initialised )
> - memcpy(ctxt.fpu_regs, v->arch.fpu_ctxt, sizeof(ctxt.fpu_regs));
> - else
> - memset(ctxt.fpu_regs, 0, sizeof(ctxt.fpu_regs));
> -
> - ctxt.rax = v->arch.user_regs.eax;
> - ctxt.rbx = v->arch.user_regs.ebx;
> - ctxt.rcx = v->arch.user_regs.ecx;
> - ctxt.rdx = v->arch.user_regs.edx;
> - ctxt.rbp = v->arch.user_regs.ebp;
> - ctxt.rsi = v->arch.user_regs.esi;
> - ctxt.rdi = v->arch.user_regs.edi;
> - ctxt.rsp = v->arch.user_regs.esp;
> - ctxt.rip = v->arch.user_regs.eip;
> - ctxt.rflags = v->arch.user_regs.eflags;
> - ctxt.r8 = v->arch.user_regs.r8;
> - ctxt.r9 = v->arch.user_regs.r9;
> - ctxt.r10 = v->arch.user_regs.r10;
> - ctxt.r11 = v->arch.user_regs.r11;
> - ctxt.r12 = v->arch.user_regs.r12;
> - ctxt.r13 = v->arch.user_regs.r13;
> - ctxt.r14 = v->arch.user_regs.r14;
> - ctxt.r15 = v->arch.user_regs.r15;
> - ctxt.dr0 = v->arch.debugreg[0];
> - ctxt.dr1 = v->arch.debugreg[1];
> - ctxt.dr2 = v->arch.debugreg[2];
> - ctxt.dr3 = v->arch.debugreg[3];
> - ctxt.dr6 = v->arch.debugreg[6];
> - ctxt.dr7 = v->arch.debugreg[7];
> -
> - if ( hvm_save_entry(CPU, v->vcpu_id, h, &ctxt) < 0 )
> - return -ENOSPC;
> - }
> - return 0;
> + rc = _hvm_init_entry(h, HVM_SAVE_CODE(CPU),
> + inst, HVM_SAVE_LENGTH(CPU));
> + if ( rc < 0 )
> + return rc;
> +
> + rec = (struct hvm_hw_cpu *)&h->data[h->cur];
> + h->cur += HVM_SAVE_LENGTH(CPU);
> +
> + /* Architecture-specific vmcs/vmcb bits */
> + hvm_funcs.save_cpu_ctxt(v, rec);
> +
> + rec->msr_tsc_aux = hvm_msr_tsc_aux(v);
> +
> + hvm_get_segment_register(v, x86_seg_idtr, &seg);
> + rec->idtr_limit = seg.limit;
> + rec->idtr_base = seg.base;
> +
> + hvm_get_segment_register(v, x86_seg_gdtr, &seg);
> + rec->gdtr_limit = seg.limit;
> + rec->gdtr_base = seg.base;
> +
> + hvm_get_segment_register(v, x86_seg_cs, &seg);
> + rec->cs_sel = seg.sel;
> + rec->cs_limit = seg.limit;
> + rec->cs_base = seg.base;
> + rec->cs_arbytes = seg.attr.bytes;
> +
> + hvm_get_segment_register(v, x86_seg_ds, &seg);
> + rec->ds_sel = seg.sel;
> + rec->ds_limit = seg.limit;
> + rec->ds_base = seg.base;
> + rec->ds_arbytes = seg.attr.bytes;
> +
> + hvm_get_segment_register(v, x86_seg_es, &seg);
> + rec->es_sel = seg.sel;
> + rec->es_limit = seg.limit;
> + rec->es_base = seg.base;
> + rec->es_arbytes = seg.attr.bytes;
> +
> + hvm_get_segment_register(v, x86_seg_ss, &seg);
> + rec->ss_sel = seg.sel;
> + rec->ss_limit = seg.limit;
> + rec->ss_base = seg.base;
> + rec->ss_arbytes = seg.attr.bytes;
> +
> + hvm_get_segment_register(v, x86_seg_fs, &seg);
> + rec->fs_sel = seg.sel;
> + rec->fs_limit = seg.limit;
> + rec->fs_base = seg.base;
> + rec->fs_arbytes = seg.attr.bytes;
> +
> + hvm_get_segment_register(v, x86_seg_gs, &seg);
> + rec->gs_sel = seg.sel;
> + rec->gs_limit = seg.limit;
> + rec->gs_base = seg.base;
> + rec->gs_arbytes = seg.attr.bytes;
> +
> + hvm_get_segment_register(v, x86_seg_tr, &seg);
> + rec->tr_sel = seg.sel;
> + rec->tr_limit = seg.limit;
> + rec->tr_base = seg.base;
> + rec->tr_arbytes = seg.attr.bytes;
> +
> + hvm_get_segment_register(v, x86_seg_ldtr, &seg);
> + rec->ldtr_sel = seg.sel;
> + rec->ldtr_limit = seg.limit;
> + rec->ldtr_base = seg.base;
> + rec->ldtr_arbytes = seg.attr.bytes;
> +
> + if ( v->fpu_initialised )
> + memcpy(rec->fpu_regs, v->arch.fpu_ctxt, sizeof(rec->fpu_regs));
> + else
> + memset(rec->fpu_regs, 0, sizeof(rec->fpu_regs));
> +
> + rec->rax = v->arch.user_regs.eax;
> + rec->rbx = v->arch.user_regs.ebx;
> + rec->rcx = v->arch.user_regs.ecx;
> + rec->rdx = v->arch.user_regs.edx;
> + rec->rbp = v->arch.user_regs.ebp;
> + rec->rsi = v->arch.user_regs.esi;
> + rec->rdi = v->arch.user_regs.edi;
> + rec->rsp = v->arch.user_regs.esp;
> + rec->rip = v->arch.user_regs.eip;
> + rec->rflags = v->arch.user_regs.eflags;
> + rec->r8 = v->arch.user_regs.r8;
> + rec->r9 = v->arch.user_regs.r9;
> + rec->r10 = v->arch.user_regs.r10;
> + rec->r11 = v->arch.user_regs.r11;
> + rec->r12 = v->arch.user_regs.r12;
> + rec->r13 = v->arch.user_regs.r13;
> + rec->r14 = v->arch.user_regs.r14;
> + rec->r15 = v->arch.user_regs.r15;
> + rec->dr0 = v->arch.debugreg[0];
> + rec->dr1 = v->arch.debugreg[1];
> + rec->dr2 = v->arch.debugreg[2];
> + rec->dr3 = v->arch.debugreg[3];
> + rec->dr6 = v->arch.debugreg[6];
> + rec->dr7 = v->arch.debugreg[7];
> +
> + return rc + HVM_SAVE_LENGTH(CPU);
> }
>
> static bool_t hvm_efer_valid(struct domain *d,
> diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
> index 40f58ed..389138d 100644
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -603,39 +603,40 @@ int32_t hvm_set_mem_pinned_cacheattr(
> static int hvm_save_mtrr_msr(struct domain *d, uint16_t inst,
> hvm_domain_context_t *h)
> {
> - int i;
> - struct vcpu *v;
> - struct hvm_hw_mtrr hw_mtrr;
> - struct mtrr_state *mtrr_state;
> - /* save mtrr&pat */
> - for_each_vcpu(d, v)
> - {
> - mtrr_state = &v->arch.hvm_vcpu.mtrr;
> + int i, rc;
> + struct vcpu *v = d->vcpu[inst];
> + struct mtrr_state *mtrr_state = &v->arch.hvm_vcpu.mtrr;
> + struct hvm_hw_mtrr *rec;
>
> - hvm_get_guest_pat(v, &hw_mtrr.msr_pat_cr);
> + rc = _hvm_init_entry(h, HVM_SAVE_CODE(MTRR),
> + inst, HVM_SAVE_LENGTH(MTRR));
> + if ( rc < 0 )
> + return rc;
>
> - hw_mtrr.msr_mtrr_def_type = mtrr_state->def_type
> - | (mtrr_state->enabled << 10);
> - hw_mtrr.msr_mtrr_cap = mtrr_state->mtrr_cap;
> + rec = (struct hvm_hw_mtrr *)&h->data[h->cur];
> + h->cur += HVM_SAVE_LENGTH(MTRR);
>
> - for ( i = 0; i < MTRR_VCNT; i++ )
> - {
> - /* save physbase */
> - hw_mtrr.msr_mtrr_var[i*2] =
> - ((uint64_t*)mtrr_state->var_ranges)[i*2];
> - /* save physmask */
> - hw_mtrr.msr_mtrr_var[i*2+1] =
> - ((uint64_t*)mtrr_state->var_ranges)[i*2+1];
> - }
> + hvm_get_guest_pat(v, &rec->msr_pat_cr);
>
> - for ( i = 0; i < NUM_FIXED_MSR; i++ )
> - hw_mtrr.msr_mtrr_fixed[i] =
> - ((uint64_t*)mtrr_state->fixed_ranges)[i];
> + rec->msr_mtrr_def_type = mtrr_state->def_type
> + | (mtrr_state->enabled << 10);
> + rec->msr_mtrr_cap = mtrr_state->mtrr_cap;
>
> - if ( hvm_save_entry(MTRR, v->vcpu_id, h, &hw_mtrr) < 0 )
> - return -ENOSPC;
> + for ( i = 0; i < MTRR_VCNT; i++ )
> + {
> + /* save physbase */
> + rec->msr_mtrr_var[i*2] =
> + ((uint64_t*)mtrr_state->var_ranges)[i*2];
> + /* save physmask */
> + rec->msr_mtrr_var[i*2+1] =
> + ((uint64_t*)mtrr_state->var_ranges)[i*2+1];
> }
> - return 0;
> +
> + for ( i = 0; i < NUM_FIXED_MSR; i++ )
> + rec->msr_mtrr_fixed[i] =
> + ((uint64_t*)mtrr_state->fixed_ranges)[i];
> +
> + return rc + HVM_SAVE_LENGTH(MTRR);
> }
>
> static int hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
> diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
> index 4f9186d..dfb01d5 100644
> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -456,21 +456,12 @@ HVM_REGISTER_SAVE_RESTORE_PER_DOM(VIRIDIAN_DOMAIN, viridian_save_domain_ctxt,
> static int viridian_save_vcpu_ctxt(struct domain *d, uint16_t inst,
> hvm_domain_context_t *h)
> {
> - struct vcpu *v;
> -
> if ( !is_viridian_domain(d) )
> return 0;
>
> - for_each_vcpu( d, v ) {
> - struct hvm_viridian_vcpu_context ctxt;
> -
> - ctxt.apic_assist = v->arch.hvm_vcpu.viridian.apic_assist.raw;
> -
> - if ( hvm_save_entry(VIRIDIAN_VCPU, v->vcpu_id, h, &ctxt) < 0 )
> - return -ENOSPC;
> - }
> -
> - return 0;
> + return hvm_save_entry(
> + VIRIDIAN_VCPU, inst, h,
> + &d->vcpu[inst]->arch.hvm_vcpu.viridian.apic_assist.raw);
> }
>
> static int viridian_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 81dfd3f..bb1438a 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -1140,35 +1140,17 @@ static void lapic_rearm(struct vlapic *s)
> static int lapic_save_hidden(struct domain *d, uint16_t inst,
> hvm_domain_context_t *h)
> {
> - struct vcpu *v;
> - struct vlapic *s;
> - int rc = 0;
> -
> - for_each_vcpu ( d, v )
> - {
> - s = vcpu_vlapic(v);
> - if ( (rc = hvm_save_entry(LAPIC, v->vcpu_id, h, &s->hw)) < 0 )
> - break;
> - }
> + struct vlapic *s = vcpu_vlapic(d->vcpu[inst]);
>
> - return rc;
> + return hvm_save_entry(LAPIC, inst, h, &s->hw);
> }
>
> static int lapic_save_regs(struct domain *d, uint16_t inst,
> hvm_domain_context_t *h)
> {
> - struct vcpu *v;
> - struct vlapic *s;
> - int rc = 0;
> -
> - for_each_vcpu ( d, v )
> - {
> - s = vcpu_vlapic(v);
> - if ( (rc = hvm_save_entry(LAPIC_REGS, v->vcpu_id, h, s->regs)) < 0 )
> - break;
> - }
> + struct vlapic *s = vcpu_vlapic(d->vcpu[inst]);
>
> - return rc;
> + return hvm_save_entry(LAPIC_REGS, inst, h, &s->regs);
> }
>
> static int lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
> diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c
> index 7e4b64b..416e970 100644
> --- a/xen/arch/x86/hvm/vpic.c
> +++ b/xen/arch/x86/hvm/vpic.c
> @@ -367,18 +367,9 @@ static int vpic_intercept_elcr_io(
> static int vpic_save(struct domain *d, uint16_t inst,
> hvm_domain_context_t *h)
> {
> - struct hvm_hw_vpic *s;
> - int i;
> + struct hvm_hw_vpic *s = &d->arch.hvm_domain.vpic[inst];
>
> - /* Save the state of both PICs */
> - for ( i = 0; i < 2 ; i++ )
> - {
> - s = &d->arch.hvm_domain.vpic[i];
> - if ( hvm_save_entry(PIC, i, h, s) < 0 )
> - return -ENOSPC;
> - }
> -
> - return 0;
> + return hvm_save_entry(PIC, inst, h, s);
> }
>
> static int vpic_load(struct domain *d, hvm_domain_context_t *h)
> diff --git a/xen/common/hvm/save.c b/xen/common/hvm/save.c
> index e9723e3..8eb9672 100644
> --- a/xen/common/hvm/save.c
> +++ b/xen/common/hvm/save.c
> @@ -84,44 +84,42 @@ size_t hvm_save_size(struct domain *d)
> int hvm_save_one(struct domain *d, uint16_t typecode, uint16_t instance,
> XEN_GUEST_HANDLE_64(uint8) handle)
> {
> - int rv = 0;
> - size_t sz = 0;
> - struct vcpu *v;
> - hvm_domain_context_t ctxt = { 0, };
> + int rv;
> + hvm_domain_context_t ctxt = { 0 };
>
> - if ( d->is_dying
> - || typecode > HVM_SAVE_CODE_MAX
> - || hvm_sr_handlers[typecode].size < sizeof(struct hvm_save_descriptor)
> - || hvm_sr_handlers[typecode].save == NULL )
> + if ( d->is_dying || typecode > HVM_SAVE_CODE_MAX )
> return -EINVAL;
>
> - if ( is_per_vcpu_handler(hvm_sr_handlers[typecode]) )
> - for_each_vcpu(d, v)
> - sz += hvm_sr_handlers[typecode].size;
> - else
> - sz = hvm_sr_handlers[typecode].size;
> -
> - if ( (instance + 1) * hvm_sr_handlers[typecode].size > sz )
> + if ( hvm_sr_handlers[typecode].size < sizeof(struct hvm_save_descriptor) ||
> + hvm_sr_handlers[typecode].save == NULL )
> return -EINVAL;
>
> - ctxt.size = sz;
> - ctxt.data = xmalloc_bytes(sz);
> + if ( (is_per_vcpu_handler(hvm_sr_handlers[typecode]) &&
> + (instance >= d->max_vcpus || d->vcpu[instance] == NULL)) ||
> + (instance >= hvm_sr_handlers[typecode].num) )
> + return -EBADSLT;
> +
> + ctxt.size = hvm_sr_handlers[typecode].size;
> + ctxt.data = xmalloc_bytes(hvm_sr_handlers[typecode].size);
> if ( !ctxt.data )
> return -ENOMEM;
>
> - if ( hvm_sr_handlers[typecode].save(d, 0, &ctxt) < 0 )
> + rv = hvm_sr_handlers[typecode].save(d, instance, &ctxt);
> +
> + if ( rv < 0 )
> {
> - printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16"\n",
> - d->domain_id, typecode);
> + printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16
> + ", instance %"PRIu16"\n",
> + d->domain_id, typecode, instance);
> rv = -EFAULT;
> }
> - else if ( copy_to_guest(handle,
> - ctxt.data
> - + (instance * hvm_sr_handlers[typecode].size)
> - + sizeof (struct hvm_save_descriptor),
> - hvm_sr_handlers[typecode].size
> - - sizeof (struct hvm_save_descriptor)) )
> - rv = -EFAULT;
> + else if ( rv <= sizeof (struct hvm_save_descriptor) )
> + rv = -ENODATA;
> + else
> + rv = copy_to_guest(handle,
> + ctxt.data + sizeof (struct hvm_save_descriptor),
> + rv - sizeof (struct hvm_save_descriptor))
> + ? -EFAULT : 0;
>
> xfree(ctxt.data);
> return rv;
> @@ -165,13 +163,35 @@ int hvm_save(struct domain *d, hvm_domain_context_t *h)
> {
> printk(XENLOG_G_INFO "HVM%d save: %s\n",
> d->domain_id, hvm_sr_handlers[i].name);
> - if ( handler(d, 0, h) < 0 )
> +
> + if ( is_per_vcpu_handler(hvm_sr_handlers[i]) )
> + {
> + struct vcpu *v;
> +
> + for_each_vcpu( d, v )
> + if ( handler(d, v->vcpu_id, h) < 0 )
> + {
> + printk(XENLOG_G_ERR
> + "HVM%d save: failed to save type %"PRIu16
> + ", instance %"PRIu16"\n",
> + d->domain_id, i, v->vcpu_id);
> + return -EFAULT;
> + }
> + }
> + else
> {
> - printk(XENLOG_G_ERR
> - "HVM%d save: failed to save type %"PRIu16"\n",
> - d->domain_id, i);
> - return -EFAULT;
> - }
> + int j;
> +
> + for ( j = 0; j < hvm_sr_handlers[i].num; ++j )
> + if ( handler(d, j, h) < 0 )
> + {
> + printk(XENLOG_G_ERR
> + "HVM%d save: failed to save type %"PRIu16
> + ", instance %"PRIu16"\n",
> + d->domain_id, i, j);
> + return -EFAULT;
> + }
> + }
> }
> }
>
> --
> 1.7.10.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH 0/3] Xen hvm saving improvements
2013-12-16 2:17 [RFC PATCH 0/3] Xen hvm saving improvements Andrew Cooper
` (2 preceding siblings ...)
2013-12-16 2:17 ` [RFC PATCH 3/3] xen/hvm-save: Adjust calling of multi-instance save handlers Andrew Cooper
@ 2013-12-16 9:44 ` Ian Campbell
2013-12-16 10:32 ` George Dunlap
2013-12-16 17:50 ` Don Slutz
4 siblings, 1 reply; 9+ messages in thread
From: Ian Campbell @ 2013-12-16 9:44 UTC (permalink / raw)
To: Andrew Cooper
Cc: George Dunlap, Keir Fraser, Jan Beulich, Don Slutz, Xen-devel
On Mon, 2013-12-16 at 02:17 +0000, Andrew Cooper wrote:
> This series is *very* RFC at the moment - it is only compile tested. I would
> very much appreciate review on the design. (and I shall see about sorting out
> some dev testing tomorrow)
>
> The ultimate purpose is to fix that the fact that
> XEN_DOMCTL_gethvmcontext_partial is very unreliable at providing the correct
> subset of information.
>
> All the changes are supposed to be internal to Xen. There should be no
> difference at all to the result of XEN_DOMCTL_gethvmcontext (which can be used
> as a useful dev test for the validity of the changes).
>
> George: As for whether this is intended for 4.5, I have no idea. On the one
> hand, it is fixing a hypercall which doesn't function correctly, but on the
> other hand, it is a very large and complicated set of changes relevant to
> migration. Then again, the majority of the changes were largely mechanical,
> and there is a simple test to prove whether there are bugs or not. I shall
> defer to others for comments on the risk/rewards of this series.
IMHO this is obviously not 4.4 material at this stage. Apart from
anything else we've been managing to release with these short comings
for many years.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC PATCH 0/3] Xen hvm saving improvements
2013-12-16 9:44 ` [RFC PATCH 0/3] Xen hvm saving improvements Ian Campbell
@ 2013-12-16 10:32 ` George Dunlap
0 siblings, 0 replies; 9+ messages in thread
From: George Dunlap @ 2013-12-16 10:32 UTC (permalink / raw)
To: Ian Campbell, Andrew Cooper
Cc: Keir Fraser, Jan Beulich, Don Slutz, Xen-devel
On 12/16/2013 09:44 AM, Ian Campbell wrote:
> On Mon, 2013-12-16 at 02:17 +0000, Andrew Cooper wrote:
>> This series is *very* RFC at the moment - it is only compile tested. I would
>> very much appreciate review on the design. (and I shall see about sorting out
>> some dev testing tomorrow)
>>
>> The ultimate purpose is to fix that the fact that
>> XEN_DOMCTL_gethvmcontext_partial is very unreliable at providing the correct
>> subset of information.
>>
>> All the changes are supposed to be internal to Xen. There should be no
>> difference at all to the result of XEN_DOMCTL_gethvmcontext (which can be used
>> as a useful dev test for the validity of the changes).
>>
>> George: As for whether this is intended for 4.5, I have no idea. On the one
>> hand, it is fixing a hypercall which doesn't function correctly, but on the
>> other hand, it is a very large and complicated set of changes relevant to
>> migration. Then again, the majority of the changes were largely mechanical,
>> and there is a simple test to prove whether there are bugs or not. I shall
>> defer to others for comments on the risk/rewards of this series.
> IMHO this is obviously not 4.4 material at this stage. Apart from
> anything else we've been managing to release with these short comings
> for many years.
Indeed.
-George
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH 0/3] Xen hvm saving improvements
2013-12-16 2:17 [RFC PATCH 0/3] Xen hvm saving improvements Andrew Cooper
` (3 preceding siblings ...)
2013-12-16 9:44 ` [RFC PATCH 0/3] Xen hvm saving improvements Ian Campbell
@ 2013-12-16 17:50 ` Don Slutz
4 siblings, 0 replies; 9+ messages in thread
From: Don Slutz @ 2013-12-16 17:50 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: George Dunlap, Keir Fraser, Don Slutz, Jan Beulich
On 12/15/13 21:17, Andrew Cooper wrote:
> This series is *very* RFC at the moment - it is only compile tested. I would
> very much appreciate review on the design. (and I shall see about sorting out
> some dev testing tomorrow)
>
> The ultimate purpose is to fix that the fact that
> XEN_DOMCTL_gethvmcontext_partial is very unreliable at providing the correct
> subset of information.
>
> All the changes are supposed to be internal to Xen. There should be no
> difference at all to the result of XEN_DOMCTL_gethvmcontext (which can be used
> as a useful dev test for the validity of the changes).
>
> George: As for whether this is intended for 4.5, I have no idea. On the one
> hand, it is fixing a hypercall which doesn't function correctly, but on the
> other hand, it is a very large and complicated set of changes relevant to
> migration. Then again, the majority of the changes were largely mechanical,
> and there is a simple test to prove whether there are bugs or not. I shall
> defer to others for comments on the risk/rewards of this series.
>
> Andrew Cooper (3):
> xen/hvm-save: Refactor HVM_REGISTER_SAVE_RESTORE
> xen/hvm-save: Extend hvm_save_handler to take an instance parameter
> xen/hvm-save: Adjust calling of multi-instance save handlers.
>
> xen/arch/x86/cpu/mcheck/vmce.c | 34 +++---
> xen/arch/x86/hvm/hpet.c | 8 +-
> xen/arch/x86/hvm/hvm.c | 249 ++++++++++++++++++++--------------------
> xen/arch/x86/hvm/i8254.c | 8 +-
> xen/arch/x86/hvm/irq.c | 24 ++--
> xen/arch/x86/hvm/mtrr.c | 61 +++++-----
> xen/arch/x86/hvm/pmtimer.c | 8 +-
> xen/arch/x86/hvm/rtc.c | 8 +-
> xen/arch/x86/hvm/vioapic.c | 7 +-
> xen/arch/x86/hvm/viridian.c | 31 ++---
> xen/arch/x86/hvm/vlapic.c | 38 ++----
> xen/arch/x86/hvm/vpic.c | 18 +--
> xen/common/hvm/save.c | 108 ++++++++++-------
> xen/include/xen/hvm/save.h | 65 +++++++----
> 14 files changed, 337 insertions(+), 330 deletions(-)
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Don Slutz <dslutz@verizon.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
>
> --
> 1.7.10.4
>
I think that with this size of a change, the issue that Jan talked about in the thread:
http://lists.xen.org/archives/html/xen-devel/2013-12/msg02246.html
about XEN_DOMCTL_gethvmcontext_partial not taking in a size, nor returning a size should also be fixed.
-Don Slutz
^ permalink raw reply [flat|nested] 9+ messages in thread