| Message ID | 1399583908-21755-5-git-send-email-w1.huang@samsung.com |
|---|---|
| State | New |
| Headers | show |
On 08/05/2014 22:18, Wei Huang wrote: > This patch implements a save/resore support for ARM guest core > registers. > > Signed-off-by: Evgeny Fedotov <e.fedotov@samsung.com> > Signed-off-by: Wei Huang <w1.huang@samsung.com> > --- > xen/arch/arm/hvm.c | 263 +++++++++++++++++++++++++++++++- > xen/include/public/arch-arm/hvm/save.h | 121 ++++++++++++++- > 2 files changed, 382 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c > index 471c4cd..7bfa547 100644 > --- a/xen/arch/arm/hvm.c > +++ b/xen/arch/arm/hvm.c > @@ -7,14 +7,15 @@ > > #include <xsm/xsm.h> > > +#include <xen/hvm/save.h> > #include <public/xen.h> > #include <public/hvm/params.h> > #include <public/hvm/hvm_op.h> > > #include <asm/hypercall.h> > +#include <asm/gic.h> > > long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg) > - Spurious whitespace change. > { > long rc = 0; > > @@ -65,3 +66,263 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg) > > return rc; > } > + > +static int hvm_save_cpu_ctxt(struct domain *d, hvm_domain_context_t *h) > +{ > + struct hvm_arm_cpu ctxt; > + struct vcpu_guest_core_regs c; > + struct vcpu *v; > + > + /* Save the state of CPU */ > + for_each_vcpu( d, v ) > + { > + memset(&ctxt, 0, sizeof(ctxt)); > + > + ctxt.sctlr = v->arch.sctlr; > + ctxt.ttbr0 = v->arch.ttbr0; > + ctxt.ttbr1 = v->arch.ttbr1; > + ctxt.ttbcr = v->arch.ttbcr; > + > + ctxt.dacr = v->arch.dacr; > + ctxt.ifsr = v->arch.ifsr; > +#ifdef CONFIG_ARM_32 > + ctxt.ifar = v->arch.ifar; > + ctxt.dfar = v->arch.dfar; > + ctxt.dfsr = v->arch.dfsr; > +#else > + ctxt.far = v->arch.far; > + ctxt.esr = v->arch.esr; > +#endif > + > +#ifdef CONFIG_ARM_32 > + ctxt.mair0 = v->arch.mair0; > + ctxt.mair1 = v->arch.mair1; > +#else > + ctxt.mair0 = v->arch.mair; > +#endif > + /* Control Registers */ > + ctxt.actlr = v->arch.actlr; > + ctxt.sctlr = v->arch.sctlr; > + ctxt.cpacr = v->arch.cpacr; > + > + ctxt.contextidr = v->arch.contextidr; > + ctxt.tpidr_el0 = v->arch.tpidr_el0; > + ctxt.tpidr_el1 = v->arch.tpidr_el1; > + ctxt.tpidrro_el0 = v->arch.tpidrro_el0; > + > + /* CP 15 */ > + ctxt.csselr = v->arch.csselr; > + > + ctxt.afsr0 = v->arch.afsr0; > + ctxt.afsr1 = v->arch.afsr1; > + ctxt.vbar = v->arch.vbar; > + ctxt.par = v->arch.par; > + ctxt.teecr = v->arch.teecr; > + ctxt.teehbr = v->arch.teehbr; > + > +#ifdef CONFIG_ARM_32 > + ctxt.joscr = v->arch.joscr; > + ctxt.jmcr = v->arch.jmcr; > +#endif > + > + memset(&c, 0, sizeof(c)); > + > + /* get guest core registers */ > + vcpu_regs_hyp_to_user(v, &c); > + > + ctxt.x0 = c.x0; > + ctxt.x1 = c.x1; > + ctxt.x2 = c.x2; > + ctxt.x3 = c.x3; > + ctxt.x4 = c.x4; > + ctxt.x5 = c.x5; > + ctxt.x6 = c.x6; > + ctxt.x7 = c.x7; > + ctxt.x8 = c.x8; > + ctxt.x9 = c.x9; > + ctxt.x10 = c.x10; > + ctxt.x11 = c.x11; > + ctxt.x12 = c.x12; > + ctxt.x13 = c.x13; > + ctxt.x14 = c.x14; > + ctxt.x15 = c.x15; > + ctxt.x16 = c.x16; > + ctxt.x17 = c.x17; > + ctxt.x18 = c.x18; > + ctxt.x19 = c.x19; > + ctxt.x20 = c.x20; > + ctxt.x21 = c.x21; > + ctxt.x22 = c.x22; > + ctxt.x23 = c.x23; > + ctxt.x24 = c.x24; > + ctxt.x25 = c.x25; > + ctxt.x26 = c.x26; > + ctxt.x27 = c.x27; > + ctxt.x28 = c.x28; > + ctxt.x29 = c.x29; > + ctxt.x30 = c.x30; > + ctxt.pc64 = c.pc64; > + ctxt.cpsr = c.cpsr; > + ctxt.spsr_el1 = c.spsr_el1; /* spsr_svc */ > + > +#ifdef CONFIG_ARM_32 > + ctxt.spsr_fiq = c.spsr_fiq; > + ctxt.spsr_irq = c.spsr_irq; > + ctxt.spsr_und = c.spsr_und; > + ctxt.spsr_abt = c.spsr_abt; > +#endif > +#ifdef CONFIG_ARM_64 > + ctxt.sp_el0 = c.sp_el0; > + ctxt.sp_el1 = c.sp_el1; > + ctxt.elr_el1 = c.elr_el1; > +#endif > + > + /* check VFP state size before dumping */ > + BUILD_BUG_ON(sizeof(v->arch.vfp) > sizeof (ctxt.vfp_state)); > + memcpy((void*) &ctxt.vfp_state, (void*) &v->arch.vfp, > + sizeof(v->arch.vfp)); > + > + if ( hvm_save_entry(VCPU, v->vcpu_id, h, &ctxt) != 0 ) > + return 1; > + } > + > + return 0; > +} > + > +static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h) > +{ > + int vcpuid; > + struct hvm_arm_cpu ctxt; > + struct vcpu *v; > + struct vcpu_guest_core_regs c; > + > + /* Which vcpu is this? */ > + vcpuid = hvm_load_instance(h); > + if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL ) > + { > + dprintk(XENLOG_G_ERR, "HVM restore: dom%u has no vcpu%u\n", > + d->domain_id, vcpuid); > + return -EINVAL; > + } > + > + if ( hvm_load_entry(VCPU, h, &ctxt) != 0 ) > + return -EINVAL; > + > + v->arch.sctlr = ctxt.sctlr; > + v->arch.ttbr0 = ctxt.ttbr0; > + v->arch.ttbr1 = ctxt.ttbr1; > + v->arch.ttbcr = ctxt.ttbcr; > + > + v->arch.dacr = ctxt.dacr; > + v->arch.ifsr = ctxt.ifsr; > +#ifdef CONFIG_ARM_32 > + v->arch.ifar = ctxt.ifar; > + v->arch.dfar = ctxt.dfar; > + v->arch.dfsr = ctxt.dfsr; > +#else > + v->arch.far = ctxt.far; > + v->arch.esr = ctxt.esr; > +#endif Where you have code like this, please use a union in the structure to reduce its size. > + > +#ifdef CONFIG_ARM_32 > + v->arch.mair0 = ctxt.mair0; > + v->arch.mair1 = ctxt.mair1; > +#else > + v->arch.mair = ctxt.mair0; > +#endif > + > + /* Control Registers */ > + v->arch.actlr = ctxt.actlr; > + v->arch.cpacr = ctxt.cpacr; > + v->arch.contextidr = ctxt.contextidr; > + v->arch.tpidr_el0 = ctxt.tpidr_el0; > + v->arch.tpidr_el1 = ctxt.tpidr_el1; > + v->arch.tpidrro_el0 = ctxt.tpidrro_el0; > + > + /* CP 15 */ > + v->arch.csselr = ctxt.csselr; > + > + v->arch.afsr0 = ctxt.afsr0; > + v->arch.afsr1 = ctxt.afsr1; > + v->arch.vbar = ctxt.vbar; > + v->arch.par = ctxt.par; > + v->arch.teecr = ctxt.teecr; > + v->arch.teehbr = ctxt.teehbr; > +#ifdef CONFIG_ARM_32 > + v->arch.joscr = ctxt.joscr; > + v->arch.jmcr = ctxt.jmcr; > +#endif > + > + /* fill guest core registers */ > + memset(&c, 0, sizeof(c)); > + c.x0 = ctxt.x0; > + c.x1 = ctxt.x1; > + c.x2 = ctxt.x2; > + c.x3 = ctxt.x3; > + c.x4 = ctxt.x4; > + c.x5 = ctxt.x5; > + c.x6 = ctxt.x6; > + c.x7 = ctxt.x7; > + c.x8 = ctxt.x8; > + c.x9 = ctxt.x9; > + c.x10 = ctxt.x10; > + c.x11 = ctxt.x11; > + c.x12 = ctxt.x12; > + c.x13 = ctxt.x13; > + c.x14 = ctxt.x14; > + c.x15 = ctxt.x15; > + c.x16 = ctxt.x16; > + c.x17 = ctxt.x17; > + c.x18 = ctxt.x18; > + c.x19 = ctxt.x19; > + c.x20 = ctxt.x20; > + c.x21 = ctxt.x21; > + c.x22 = ctxt.x22; > + c.x23 = ctxt.x23; > + c.x24 = ctxt.x24; > + c.x25 = ctxt.x25; > + c.x26 = ctxt.x26; > + c.x27 = ctxt.x27; > + c.x28 = ctxt.x28; > + c.x29 = ctxt.x29; > + c.x30 = ctxt.x30; > + c.pc64 = ctxt.pc64; > + c.cpsr = ctxt.cpsr; > + c.spsr_el1 = ctxt.spsr_el1; /* spsr_svc */ > + > +#ifdef CONFIG_ARM_32 > + c.spsr_fiq = ctxt.spsr_fiq; > + c.spsr_irq = ctxt.spsr_irq; > + c.spsr_und = ctxt.spsr_und; > + c.spsr_abt = ctxt.spsr_abt; > +#endif > +#ifdef CONFIG_ARM_64 > + c.sp_el0 = ctxt.sp_el0; > + c.sp_el1 = ctxt.sp_el1; > + c.elr_el1 = ctxt.elr_el1; > +#endif > + > + /* set guest core registers */ > + vcpu_regs_user_to_hyp(v, &c); > + > + BUILD_BUG_ON(sizeof(v->arch.vfp) > sizeof (ctxt.vfp_state)); > + memcpy(&v->arch.vfp, &ctxt, sizeof(v->arch.vfp)); > + > + v->is_initialised = 1; > + clear_bit(_VPF_down, &v->pause_flags); > + > + return 0; > +} > + > +HVM_REGISTER_SAVE_RESTORE(VCPU, hvm_save_cpu_ctxt, hvm_load_cpu_ctxt, 1, > + HVMSR_PER_VCPU); > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * tab-width: 4 > + * indent-tabs-mode: nil > + * End: > + */ > diff --git a/xen/include/public/arch-arm/hvm/save.h b/xen/include/public/arch-arm/hvm/save.h > index 8679bfd..18e5899 100644 > --- a/xen/include/public/arch-arm/hvm/save.h > +++ b/xen/include/public/arch-arm/hvm/save.h > @@ -86,10 +86,129 @@ struct hvm_arm_timer > }; > DECLARE_HVM_SAVE_TYPE(TIMER, 4, struct hvm_arm_timer); > > +/* ARM core hardware info */ > +struct hvm_arm_cpu > +{ > + /* ======= Guest VFP State ======= > + * - 34 8-bytes required for AArch32 guests > + * - 66 8-bytes required for AArch64 guests > + */ > + uint64_t vfp_state[66]; > + > + /* ======= Guest Core Registers ======= > + * - Each reg is multiplexed for AArch64 and AArch32 guests, if possible > + * - Each comments, /AArch64_reg, AArch32_reg/, describes its > + * corresponding 64- and 32-bit register name. "NA" means > + * "Not Applicable". > + * - Check "struct vcpu_guest_core_regs" for details. > + */ > + uint64_t x0; /* x0, r0_usr */ > + uint64_t x1; /* x1, r1_usr */ > + uint64_t x2; /* x2, r2_usr */ > + uint64_t x3; /* x3, r3_usr */ > + uint64_t x4; /* x4, r4_usr */ > + uint64_t x5; /* x5, r5_usr */ > + uint64_t x6; /* x6, r6_usr */ > + uint64_t x7; /* x7, r7_usr */ > + uint64_t x8; /* x8, r8_usr */ > + uint64_t x9; /* x9, r9_usr */ > + uint64_t x10; /* x10, r10_usr */ > + uint64_t x11; /* x11, r11_usr */ > + uint64_t x12; /* x12, r12_usr */ > + uint64_t x13; /* x13, sp_usr */ > + uint64_t x14; /* x14, lr_usr; */ > + uint64_t x15; /* x15, __unused_sp_hyp */ > + uint64_t x16; /* x16, lr_irq */ > + uint64_t x17; /* x17, sp_irq */ > + uint64_t x18; /* x18, lr_svc */ > + uint64_t x19; /* x19, sp_svc */ > + uint64_t x20; /* x20, lr_abt */ > + uint64_t x21; /* x21, sp_abt */ > + uint64_t x22; /* x22, lr_und */ > + uint64_t x23; /* x23, sp_und */ > + uint64_t x24; /* x24, r8_fiq */ > + uint64_t x25; /* x25, r9_fiq */ > + uint64_t x26; /* x26, r10_fiq */ > + uint64_t x27; /* x27, r11_fiq */ > + uint64_t x28; /* x28, r12_fiq */ > + uint64_t x29; /* fp, sp_fiq */ > + uint64_t x30; /* lr, lr_fiq */ Please use "uint64_t x[31];" and some loops. > + > + /* return address (EL1 ==> EL0) */ > + uint64_t elr_el1; /* elr_el1, NA */ > + /* return address (EL2 ==> EL1) */ > + uint64_t pc64; /* elr_el2, elr_el2 */ > + > + /* spsr registers */ > + uint32_t spsr_el1; /* spsr_el1, spsr_svc */ > + uint32_t spsr_fiq; /* NA, spsr_fiq */ > + uint32_t spsr_irq; /* NA, spsr_irq */ > + uint32_t spsr_und; /* NA, spsr_und */ > + uint32_t spsr_abt; /* NA, spsr_abt */ > + > + /* stack pointers */ > + uint64_t sp_el0; /* sp_el0, NA */ > + uint64_t sp_el1; /* sp_el1, NA */ > + > + /* guest mode */ > + uint32_t cpsr; /* spsr_el2, spsr_el2 */ > + > + /* ======= Guest System Registers ======= > + * - multiplexed for AArch32 and AArch64 guests > + * - 64-bit preferred if needed (for 64-bit guests) > + * - architecture specific registers are noted specifically > + */ > + /* exception */ > + uint64_t vbar; /* vbar, vbar */ > + > + /* mmu related */ > + uint64_t ttbcr; /* ttbcr, ttbcr */ > + uint64_t ttbr0; /* ttbr0, ttbr0 */ > + uint64_t ttbr1; /* ttbr1, ttbr1 */ > + uint32_t dacr; /* NA, dacr32 */ > + > + uint64_t par; /* par, par */ > + uint64_t mair0; /* mair, mair0 */ > + uint64_t mair1; /* NA, mair1 */ > + > + /* fault status */ > + uint32_t ifar; /* ifar, ifar */ > + uint32_t ifsr; /* ifsr, ifsr */ > + uint32_t dfar; /* dfar, dfar */ > + uint32_t dfsr; /* dfsr, dfsr */ > + > + uint64_t far; /* far, far */ > + uint64_t esr; /* esr, esr */ > + > + uint32_t afsr0; /* afsr0, afsr0 */ > + uint32_t afsr1; /* afsr1, afsr1 */ > + > + /* thumbee and jazelle */ > + uint32_t teecr; /* NA, teecr */ > + uint32_t teehbr; /* NA, teehbr */ > + > + uint32_t joscr; /* NA, joscr */ > + uint32_t jmcr; /* NA, jmcr */ > + > + /* control registers */ > + uint32_t sctlr; /* sctlr, sctlr */ > + uint32_t actlr; /* actlr, actlr */ > + uint32_t cpacr; /* cpacr, cpacr */ > + > + uint32_t csselr; /* csselr, csselr */ > + > + /* software management related */ > + uint32_t contextidr; /* contextidr, contextidr */ > + uint64_t tpidr_el0; /* tpidr_el0, tpidr_el0 */ > + uint64_t tpidr_el1; /* tpidr_el1, tpidr_el1 */ > + uint64_t tpidrro_el0; /* tpidrro_el0, tdidrro_el0 */ > +}; Again - 32/64bit alignment issues. ~Andrew > +DECLARE_HVM_SAVE_TYPE(VCPU, 5, struct hvm_arm_cpu); > + > /* > * Largest type-code in use > */ > -#define HVM_SAVE_CODE_MAX 4 > +#define HVM_SAVE_CODE_MAX 5 > > #endif >
On 05/08/2014 06:10 PM, Andrew Cooper wrote: > On 08/05/2014 22:18, Wei Huang wrote: >> +#ifdef CONFIG_ARM_32 >> + v->arch.ifar = ctxt.ifar; >> + v->arch.dfar = ctxt.dfar; >> + v->arch.dfsr = ctxt.dfsr; >> +#else >> + v->arch.far = ctxt.far; >> + v->arch.esr = ctxt.esr; >> +#endif > > Where you have code like this, please use a union in the structure to > reduce its size. I thought about it from your comments. We can only combine ifar and dfar into the far register. But it became a bit confusing. Will try again. > >> + /* ======= Guest Core Registers ======= >> + * - Each reg is multiplexed for AArch64 and AArch32 guests, if possible >> + * - Each comments, /AArch64_reg, AArch32_reg/, describes its >> + * corresponding 64- and 32-bit register name. "NA" means >> + * "Not Applicable". >> + * - Check "struct vcpu_guest_core_regs" for details. >> + */ >> + uint64_t x0; /* x0, r0_usr */ >> + uint64_t x1; /* x1, r1_usr */ >> + uint64_t x2; /* x2, r2_usr */ >> + uint64_t x3; /* x3, r3_usr */ >> + uint64_t x4; /* x4, r4_usr */ >> + uint64_t x5; /* x5, r5_usr */ >> + uint64_t x6; /* x6, r6_usr */ >> + uint64_t x7; /* x7, r7_usr */ >> + uint64_t x8; /* x8, r8_usr */ >> + uint64_t x9; /* x9, r9_usr */ >> + uint64_t x10; /* x10, r10_usr */ >> + uint64_t x11; /* x11, r11_usr */ >> + uint64_t x12; /* x12, r12_usr */ >> + uint64_t x13; /* x13, sp_usr */ >> + uint64_t x14; /* x14, lr_usr; */ >> + uint64_t x15; /* x15, __unused_sp_hyp */ >> + uint64_t x16; /* x16, lr_irq */ >> + uint64_t x17; /* x17, sp_irq */ >> + uint64_t x18; /* x18, lr_svc */ >> + uint64_t x19; /* x19, sp_svc */ >> + uint64_t x20; /* x20, lr_abt */ >> + uint64_t x21; /* x21, sp_abt */ >> + uint64_t x22; /* x22, lr_und */ >> + uint64_t x23; /* x23, sp_und */ >> + uint64_t x24; /* x24, r8_fiq */ >> + uint64_t x25; /* x25, r9_fiq */ >> + uint64_t x26; /* x26, r10_fiq */ >> + uint64_t x27; /* x27, r11_fiq */ >> + uint64_t x28; /* x28, r12_fiq */ >> + uint64_t x29; /* fp, sp_fiq */ >> + uint64_t x30; /* lr, lr_fiq */ > > Please use "uint64_t x[31];" and some loops. > Register multiplexing is very confusing when 32-bit VM is running on a 64-bit Xen. Even though your request is reasonable, I still consider expanding the register names here helpful. At least it gives me space to add comments to show the register mapping layout. I believe others will find it useful as well.
On Fri, 2014-05-09 at 11:35 -0500, Wei Huang wrote: > On 05/08/2014 06:10 PM, Andrew Cooper wrote: > > On 08/05/2014 22:18, Wei Huang wrote: > >> +#ifdef CONFIG_ARM_32 > >> + v->arch.ifar = ctxt.ifar; > >> + v->arch.dfar = ctxt.dfar; > >> + v->arch.dfsr = ctxt.dfsr; > >> +#else > >> + v->arch.far = ctxt.far; > >> + v->arch.esr = ctxt.esr; > >> +#endif > > > > Where you have code like this, please use a union in the structure to > > reduce its size. > I thought about it from your comments. We can only combine ifar and dfar > into the far register. But it became a bit confusing. Will try again. We use an ifdef for these particular register everywhere internally already, because they are quite annoyingly subtley different between arm32 and arm64. There is no requirement for the save/restore code to do any different/better. For the public API half we can't have this sort of ifdef though, so a union or just having all of the field is probably required. Don't forget about the 32-bit guest on 64-bit hypervisor case (caveat: I've only looked at the above quoted snippet, not the whole context) > >> + /* ======= Guest Core Registers ======= > >> + * - Each reg is multiplexed for AArch64 and AArch32 guests, if possible > >> + * - Each comments, /AArch64_reg, AArch32_reg/, describes its > >> + * corresponding 64- and 32-bit register name. "NA" means > >> + * "Not Applicable". > >> + * - Check "struct vcpu_guest_core_regs" for details. > >> + */ > >> + uint64_t x0; /* x0, r0_usr */ > >> + uint64_t x1; /* x1, r1_usr */ > >> + uint64_t x2; /* x2, r2_usr */ > >> + uint64_t x3; /* x3, r3_usr */ > >> + uint64_t x4; /* x4, r4_usr */ > >> + uint64_t x5; /* x5, r5_usr */ > >> + uint64_t x6; /* x6, r6_usr */ > >> + uint64_t x7; /* x7, r7_usr */ > >> + uint64_t x8; /* x8, r8_usr */ > >> + uint64_t x9; /* x9, r9_usr */ > >> + uint64_t x10; /* x10, r10_usr */ > >> + uint64_t x11; /* x11, r11_usr */ > >> + uint64_t x12; /* x12, r12_usr */ > >> + uint64_t x13; /* x13, sp_usr */ > >> + uint64_t x14; /* x14, lr_usr; */ > >> + uint64_t x15; /* x15, __unused_sp_hyp */ > >> + uint64_t x16; /* x16, lr_irq */ > >> + uint64_t x17; /* x17, sp_irq */ > >> + uint64_t x18; /* x18, lr_svc */ > >> + uint64_t x19; /* x19, sp_svc */ > >> + uint64_t x20; /* x20, lr_abt */ > >> + uint64_t x21; /* x21, sp_abt */ > >> + uint64_t x22; /* x22, lr_und */ > >> + uint64_t x23; /* x23, sp_und */ > >> + uint64_t x24; /* x24, r8_fiq */ > >> + uint64_t x25; /* x25, r9_fiq */ > >> + uint64_t x26; /* x26, r10_fiq */ > >> + uint64_t x27; /* x27, r11_fiq */ > >> + uint64_t x28; /* x28, r12_fiq */ > >> + uint64_t x29; /* fp, sp_fiq */ > >> + uint64_t x30; /* lr, lr_fiq */ > > > > Please use "uint64_t x[31];" and some loops. > > > Register multiplexing is very confusing when 32-bit VM is running on a > 64-bit Xen. Even though your request is reasonable, I still consider > expanding the register names here helpful. At least it gives me space to > add comments to show the register mapping layout. I believe others will > find it useful as well. I am fine with what you've got, although matching the names used in the internal struct cpu_user_regs for x29 and x30 would be nice perhaps. Ian.
Hi Wei, Thank you for the patch. On 08/05/14 22:18, Wei Huang wrote: > This patch implements a save/resore support for ARM guest core > registers. The commit 893256f "xen/arm: Correctly save/restore CNTKCTL_EL1" save/restore a new register during the context switch. I think you forgot to add it in this patch. Regards,
On Sun, 2014-05-11 at 10:06 +0100, Julien Grall wrote: > Hi Wei, > > Thank you for the patch. > > On 08/05/14 22:18, Wei Huang wrote: > > This patch implements a save/resore support for ARM guest core > > registers. > > > The commit 893256f "xen/arm: Correctly save/restore CNTKCTL_EL1" > save/restore a new register during the context switch. > > I think you forgot to add it in this patch. I think it would belong in the previous patch with the timer state. Ian.
On Thu, 2014-05-08 at 16:18 -0500, Wei Huang wrote: > This patch implements a save/resore support for ARM guest core > registers. > > Signed-off-by: Evgeny Fedotov <e.fedotov@samsung.com> > Signed-off-by: Wei Huang <w1.huang@samsung.com> > --- > xen/arch/arm/hvm.c | 263 +++++++++++++++++++++++++++++++- > xen/include/public/arch-arm/hvm/save.h | 121 ++++++++++++++- > 2 files changed, 382 insertions(+), 2 deletions(-) > > diff --git a/xen/include/public/arch-arm/hvm/save.h b/xen/include/public/arch-arm/hvm/save.h > index 8679bfd..18e5899 100644 > --- a/xen/include/public/arch-arm/hvm/save.h > +++ b/xen/include/public/arch-arm/hvm/save.h [...] > + /* return address (EL1 ==> EL0) */ > + uint64_t elr_el1; /* elr_el1, NA */ > + /* return address (EL2 ==> EL1) */ > + uint64_t pc64; /* elr_el2, elr_el2 */ The guest name of this register would be pc or r15, not elr_el2 which is the hyp mode exception return address. (I think the _el2 suffix should never appear in these structs) > + /* spsr registers */ > + uint32_t spsr_el1; /* spsr_el1, spsr_svc */ > + uint32_t spsr_fiq; /* NA, spsr_fiq */ > + uint32_t spsr_irq; /* NA, spsr_irq */ > + uint32_t spsr_und; /* NA, spsr_und */ > + uint32_t spsr_abt; /* NA, spsr_abt */ I think in general you can replace /* NA, .* */ with "32-bit only" and /* .*, NA */ with "64-bit only". and omit the comment in cases where everything matches. e.g. uint64_t vbar /* vbar, vbar */ > + > + /* stack pointers */ > + uint64_t sp_el0; /* sp_el0, NA */ > + uint64_t sp_el1; /* sp_el1, NA */ > + > + /* guest mode */ > + uint32_t cpsr; /* spsr_el2, spsr_el2 */ The guest register is just cpsr I think, spsr_el2 is a hypervisor mode ism. > + /* ======= Guest System Registers ======= > + * - multiplexed for AArch32 and AArch64 guests > + * - 64-bit preferred if needed (for 64-bit guests) > + * - architecture specific registers are noted specifically > + */ > + /* exception */ > + uint64_t vbar; /* vbar, vbar */ There's a bit of a mixture of v7 and v8 naming here. Can we try to consistently use the v8 naming (foo_el1/el0) for anything which is v8 only or common and only use v7 naming for 32-bit only registers. Or maybe we should be trying to use the "General names" as defined in section J or the ARMv8 ARM? Whatever we do it should be consistent. (I know Xen internally is a bit confused about this, but please lets try not to leak that into the public API). > + /* mmu related */ > + uint64_t ttbcr; /* ttbcr, ttbcr */ > + uint64_t ttbr0; /* ttbr0, ttbr0 */ > + uint64_t ttbr1; /* ttbr1, ttbr1 */ > + uint32_t dacr; /* NA, dacr32 */ > + > + uint64_t par; /* par, par */ > + uint64_t mair0; /* mair, mair0 */ > + uint64_t mair1; /* NA, mair1 */ mair0 and mair1 are only 32-bit. But actually I think the mapping is MAIR0==MAIR_EL1[31:0] and MAIR1==MAIR_EL1[63:32], so you should use that. > + /* fault status */ > + uint32_t ifar; /* ifar, ifar */ > + uint32_t ifsr; /* ifsr, ifsr */ > + uint32_t dfar; /* dfar, dfar */ > + uint32_t dfsr; /* dfsr, dfsr */ > + > + uint64_t far; /* far, far */ > + uint64_t esr; /* esr, esr */ I don't think there is a 32-bit ESR, since 32-bit has all the banked register CPSR.MODE stuff instead. I think the 32-bit ifar/dfar ifsr/dfsr have a mapping onto the 64-bit far etc. Table D1-81 in the ARMv8 ARM spells all that out. > + > + uint32_t afsr0; /* afsr0, afsr0 */ > + uint32_t afsr1; /* afsr1, afsr1 */ > + > + /* thumbee and jazelle */ > + uint32_t teecr; /* NA, teecr */ > + uint32_t teehbr; /* NA, teehbr */ > + > + uint32_t joscr; /* NA, joscr */ > + uint32_t jmcr; /* NA, jmcr */ > + > + /* control registers */ > + uint32_t sctlr; /* sctlr, sctlr */ > + uint32_t actlr; /* actlr, actlr */ > + uint32_t cpacr; /* cpacr, cpacr */ > + > + uint32_t csselr; /* csselr, csselr */ > + > + /* software management related */ > + uint32_t contextidr; /* contextidr, contextidr */ > + uint64_t tpidr_el0; /* tpidr_el0, tpidr_el0 */ > + uint64_t tpidr_el1; /* tpidr_el1, tpidr_el1 */ > + uint64_t tpidrro_el0; /* tpidrro_el0, tdidrro_el0 */ > +}; > +DECLARE_HVM_SAVE_TYPE(VCPU, 5, struct hvm_arm_cpu); > + > /* > * Largest type-code in use > */ > -#define HVM_SAVE_CODE_MAX 4 > +#define HVM_SAVE_CODE_MAX 5 > > #endif >
On 05/14/2014 12:16 PM, Ian Campbell wrote: > On Sun, 2014-05-11 at 10:06 +0100, Julien Grall wrote: >> Hi Wei, >> >> Thank you for the patch. >> >> On 08/05/14 22:18, Wei Huang wrote: >>> This patch implements a save/resore support for ARM guest core >>> registers. >> >> >> The commit 893256f "xen/arm: Correctly save/restore CNTKCTL_EL1" >> save/restore a new register during the context switch. >> >> I think you forgot to add it in this patch. > > I think it would belong in the previous patch with the timer state. If Wei plans to use his new structure (see [1]), I'm fine with save/restore on the previous patch. Otherwise it seems stupid to save twice the same value. Regards, [1] http://lists.xen.org/archives/html/xen-devel/2014-05/msg01642.html
On Wed, 2014-05-14 at 13:23 +0100, Julien Grall wrote: > On 05/14/2014 12:16 PM, Ian Campbell wrote: > > On Sun, 2014-05-11 at 10:06 +0100, Julien Grall wrote: > >> Hi Wei, > >> > >> Thank you for the patch. > >> > >> On 08/05/14 22:18, Wei Huang wrote: > >>> This patch implements a save/resore support for ARM guest core > >>> registers. > >> > >> > >> The commit 893256f "xen/arm: Correctly save/restore CNTKCTL_EL1" > >> save/restore a new register during the context switch. > >> > >> I think you forgot to add it in this patch. > > > > I think it would belong in the previous patch with the timer state. > > If Wei plans to use his new structure (see [1]), I'm fine with > save/restore on the previous patch. > > Otherwise it seems stupid to save twice the same value. Nobody has suggested saving it twice. > > Regards, > > [1] http://lists.xen.org/archives/html/xen-devel/2014-05/msg01642.html >
On 05/14/2014 02:25 PM, Ian Campbell wrote: > On Wed, 2014-05-14 at 13:23 +0100, Julien Grall wrote: >> On 05/14/2014 12:16 PM, Ian Campbell wrote: >>> On Sun, 2014-05-11 at 10:06 +0100, Julien Grall wrote: >>>> Hi Wei, >>>> >>>> Thank you for the patch. >>>> >>>> On 08/05/14 22:18, Wei Huang wrote: >>>>> This patch implements a save/resore support for ARM guest core >>>>> registers. >>>> >>>> >>>> The commit 893256f "xen/arm: Correctly save/restore CNTKCTL_EL1" >>>> save/restore a new register during the context switch. >>>> >>>> I think you forgot to add it in this patch. >>> >>> I think it would belong in the previous patch with the timer state. >> >> If Wei plans to use his new structure (see [1]), I'm fine with >> save/restore on the previous patch. >> >> Otherwise it seems stupid to save twice the same value. > > Nobody has suggested saving it twice. When I commented the code, there was 2 timer structure (one for the phys timer and the other for the virt timer). Anyway, it seems the new approach will be with a single structure. Regards,
diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c index 471c4cd..7bfa547 100644 --- a/xen/arch/arm/hvm.c +++ b/xen/arch/arm/hvm.c @@ -7,14 +7,15 @@ #include <xsm/xsm.h> +#include <xen/hvm/save.h> #include <public/xen.h> #include <public/hvm/params.h> #include <public/hvm/hvm_op.h> #include <asm/hypercall.h> +#include <asm/gic.h> long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg) - { long rc = 0; @@ -65,3 +66,263 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg) return rc; } + +static int hvm_save_cpu_ctxt(struct domain *d, hvm_domain_context_t *h) +{ + struct hvm_arm_cpu ctxt; + struct vcpu_guest_core_regs c; + struct vcpu *v; + + /* Save the state of CPU */ + for_each_vcpu( d, v ) + { + memset(&ctxt, 0, sizeof(ctxt)); + + ctxt.sctlr = v->arch.sctlr; + ctxt.ttbr0 = v->arch.ttbr0; + ctxt.ttbr1 = v->arch.ttbr1; + ctxt.ttbcr = v->arch.ttbcr; + + ctxt.dacr = v->arch.dacr; + ctxt.ifsr = v->arch.ifsr; +#ifdef CONFIG_ARM_32 + ctxt.ifar = v->arch.ifar; + ctxt.dfar = v->arch.dfar; + ctxt.dfsr = v->arch.dfsr; +#else + ctxt.far = v->arch.far; + ctxt.esr = v->arch.esr; +#endif + +#ifdef CONFIG_ARM_32 + ctxt.mair0 = v->arch.mair0; + ctxt.mair1 = v->arch.mair1; +#else + ctxt.mair0 = v->arch.mair; +#endif + /* Control Registers */ + ctxt.actlr = v->arch.actlr; + ctxt.sctlr = v->arch.sctlr; + ctxt.cpacr = v->arch.cpacr; + + ctxt.contextidr = v->arch.contextidr; + ctxt.tpidr_el0 = v->arch.tpidr_el0; + ctxt.tpidr_el1 = v->arch.tpidr_el1; + ctxt.tpidrro_el0 = v->arch.tpidrro_el0; + + /* CP 15 */ + ctxt.csselr = v->arch.csselr; + + ctxt.afsr0 = v->arch.afsr0; + ctxt.afsr1 = v->arch.afsr1; + ctxt.vbar = v->arch.vbar; + ctxt.par = v->arch.par; + ctxt.teecr = v->arch.teecr; + ctxt.teehbr = v->arch.teehbr; + +#ifdef CONFIG_ARM_32 + ctxt.joscr = v->arch.joscr; + ctxt.jmcr = v->arch.jmcr; +#endif + + memset(&c, 0, sizeof(c)); + + /* get guest core registers */ + vcpu_regs_hyp_to_user(v, &c); + + ctxt.x0 = c.x0; + ctxt.x1 = c.x1; + ctxt.x2 = c.x2; + ctxt.x3 = c.x3; + ctxt.x4 = c.x4; + ctxt.x5 = c.x5; + ctxt.x6 = c.x6; + ctxt.x7 = c.x7; + ctxt.x8 = c.x8; + ctxt.x9 = c.x9; + ctxt.x10 = c.x10; + ctxt.x11 = c.x11; + ctxt.x12 = c.x12; + ctxt.x13 = c.x13; + ctxt.x14 = c.x14; + ctxt.x15 = c.x15; + ctxt.x16 = c.x16; + ctxt.x17 = c.x17; + ctxt.x18 = c.x18; + ctxt.x19 = c.x19; + ctxt.x20 = c.x20; + ctxt.x21 = c.x21; + ctxt.x22 = c.x22; + ctxt.x23 = c.x23; + ctxt.x24 = c.x24; + ctxt.x25 = c.x25; + ctxt.x26 = c.x26; + ctxt.x27 = c.x27; + ctxt.x28 = c.x28; + ctxt.x29 = c.x29; + ctxt.x30 = c.x30; + ctxt.pc64 = c.pc64; + ctxt.cpsr = c.cpsr; + ctxt.spsr_el1 = c.spsr_el1; /* spsr_svc */ + +#ifdef CONFIG_ARM_32 + ctxt.spsr_fiq = c.spsr_fiq; + ctxt.spsr_irq = c.spsr_irq; + ctxt.spsr_und = c.spsr_und; + ctxt.spsr_abt = c.spsr_abt; +#endif +#ifdef CONFIG_ARM_64 + ctxt.sp_el0 = c.sp_el0; + ctxt.sp_el1 = c.sp_el1; + ctxt.elr_el1 = c.elr_el1; +#endif + + /* check VFP state size before dumping */ + BUILD_BUG_ON(sizeof(v->arch.vfp) > sizeof (ctxt.vfp_state)); + memcpy((void*) &ctxt.vfp_state, (void*) &v->arch.vfp, + sizeof(v->arch.vfp)); + + if ( hvm_save_entry(VCPU, v->vcpu_id, h, &ctxt) != 0 ) + return 1; + } + + return 0; +} + +static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h) +{ + int vcpuid; + struct hvm_arm_cpu ctxt; + struct vcpu *v; + struct vcpu_guest_core_regs c; + + /* Which vcpu is this? */ + vcpuid = hvm_load_instance(h); + if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL ) + { + dprintk(XENLOG_G_ERR, "HVM restore: dom%u has no vcpu%u\n", + d->domain_id, vcpuid); + return -EINVAL; + } + + if ( hvm_load_entry(VCPU, h, &ctxt) != 0 ) + return -EINVAL; + + v->arch.sctlr = ctxt.sctlr; + v->arch.ttbr0 = ctxt.ttbr0; + v->arch.ttbr1 = ctxt.ttbr1; + v->arch.ttbcr = ctxt.ttbcr; + + v->arch.dacr = ctxt.dacr; + v->arch.ifsr = ctxt.ifsr; +#ifdef CONFIG_ARM_32 + v->arch.ifar = ctxt.ifar; + v->arch.dfar = ctxt.dfar; + v->arch.dfsr = ctxt.dfsr; +#else + v->arch.far = ctxt.far; + v->arch.esr = ctxt.esr; +#endif + +#ifdef CONFIG_ARM_32 + v->arch.mair0 = ctxt.mair0; + v->arch.mair1 = ctxt.mair1; +#else + v->arch.mair = ctxt.mair0; +#endif + + /* Control Registers */ + v->arch.actlr = ctxt.actlr; + v->arch.cpacr = ctxt.cpacr; + v->arch.contextidr = ctxt.contextidr; + v->arch.tpidr_el0 = ctxt.tpidr_el0; + v->arch.tpidr_el1 = ctxt.tpidr_el1; + v->arch.tpidrro_el0 = ctxt.tpidrro_el0; + + /* CP 15 */ + v->arch.csselr = ctxt.csselr; + + v->arch.afsr0 = ctxt.afsr0; + v->arch.afsr1 = ctxt.afsr1; + v->arch.vbar = ctxt.vbar; + v->arch.par = ctxt.par; + v->arch.teecr = ctxt.teecr; + v->arch.teehbr = ctxt.teehbr; +#ifdef CONFIG_ARM_32 + v->arch.joscr = ctxt.joscr; + v->arch.jmcr = ctxt.jmcr; +#endif + + /* fill guest core registers */ + memset(&c, 0, sizeof(c)); + c.x0 = ctxt.x0; + c.x1 = ctxt.x1; + c.x2 = ctxt.x2; + c.x3 = ctxt.x3; + c.x4 = ctxt.x4; + c.x5 = ctxt.x5; + c.x6 = ctxt.x6; + c.x7 = ctxt.x7; + c.x8 = ctxt.x8; + c.x9 = ctxt.x9; + c.x10 = ctxt.x10; + c.x11 = ctxt.x11; + c.x12 = ctxt.x12; + c.x13 = ctxt.x13; + c.x14 = ctxt.x14; + c.x15 = ctxt.x15; + c.x16 = ctxt.x16; + c.x17 = ctxt.x17; + c.x18 = ctxt.x18; + c.x19 = ctxt.x19; + c.x20 = ctxt.x20; + c.x21 = ctxt.x21; + c.x22 = ctxt.x22; + c.x23 = ctxt.x23; + c.x24 = ctxt.x24; + c.x25 = ctxt.x25; + c.x26 = ctxt.x26; + c.x27 = ctxt.x27; + c.x28 = ctxt.x28; + c.x29 = ctxt.x29; + c.x30 = ctxt.x30; + c.pc64 = ctxt.pc64; + c.cpsr = ctxt.cpsr; + c.spsr_el1 = ctxt.spsr_el1; /* spsr_svc */ + +#ifdef CONFIG_ARM_32 + c.spsr_fiq = ctxt.spsr_fiq; + c.spsr_irq = ctxt.spsr_irq; + c.spsr_und = ctxt.spsr_und; + c.spsr_abt = ctxt.spsr_abt; +#endif +#ifdef CONFIG_ARM_64 + c.sp_el0 = ctxt.sp_el0; + c.sp_el1 = ctxt.sp_el1; + c.elr_el1 = ctxt.elr_el1; +#endif + + /* set guest core registers */ + vcpu_regs_user_to_hyp(v, &c); + + BUILD_BUG_ON(sizeof(v->arch.vfp) > sizeof (ctxt.vfp_state)); + memcpy(&v->arch.vfp, &ctxt, sizeof(v->arch.vfp)); + + v->is_initialised = 1; + clear_bit(_VPF_down, &v->pause_flags); + + return 0; +} + +HVM_REGISTER_SAVE_RESTORE(VCPU, hvm_save_cpu_ctxt, hvm_load_cpu_ctxt, 1, + HVMSR_PER_VCPU); + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/include/public/arch-arm/hvm/save.h b/xen/include/public/arch-arm/hvm/save.h index 8679bfd..18e5899 100644 --- a/xen/include/public/arch-arm/hvm/save.h +++ b/xen/include/public/arch-arm/hvm/save.h @@ -86,10 +86,129 @@ struct hvm_arm_timer }; DECLARE_HVM_SAVE_TYPE(TIMER, 4, struct hvm_arm_timer); +/* ARM core hardware info */ +struct hvm_arm_cpu +{ + /* ======= Guest VFP State ======= + * - 34 8-bytes required for AArch32 guests + * - 66 8-bytes required for AArch64 guests + */ + uint64_t vfp_state[66]; + + /* ======= Guest Core Registers ======= + * - Each reg is multiplexed for AArch64 and AArch32 guests, if possible + * - Each comments, /AArch64_reg, AArch32_reg/, describes its + * corresponding 64- and 32-bit register name. "NA" means + * "Not Applicable". + * - Check "struct vcpu_guest_core_regs" for details. + */ + uint64_t x0; /* x0, r0_usr */ + uint64_t x1; /* x1, r1_usr */ + uint64_t x2; /* x2, r2_usr */ + uint64_t x3; /* x3, r3_usr */ + uint64_t x4; /* x4, r4_usr */ + uint64_t x5; /* x5, r5_usr */ + uint64_t x6; /* x6, r6_usr */ + uint64_t x7; /* x7, r7_usr */ + uint64_t x8; /* x8, r8_usr */ + uint64_t x9; /* x9, r9_usr */ + uint64_t x10; /* x10, r10_usr */ + uint64_t x11; /* x11, r11_usr */ + uint64_t x12; /* x12, r12_usr */ + uint64_t x13; /* x13, sp_usr */ + uint64_t x14; /* x14, lr_usr; */ + uint64_t x15; /* x15, __unused_sp_hyp */ + uint64_t x16; /* x16, lr_irq */ + uint64_t x17; /* x17, sp_irq */ + uint64_t x18; /* x18, lr_svc */ + uint64_t x19; /* x19, sp_svc */ + uint64_t x20; /* x20, lr_abt */ + uint64_t x21; /* x21, sp_abt */ + uint64_t x22; /* x22, lr_und */ + uint64_t x23; /* x23, sp_und */ + uint64_t x24; /* x24, r8_fiq */ + uint64_t x25; /* x25, r9_fiq */ + uint64_t x26; /* x26, r10_fiq */ + uint64_t x27; /* x27, r11_fiq */ + uint64_t x28; /* x28, r12_fiq */ + uint64_t x29; /* fp, sp_fiq */ + uint64_t x30; /* lr, lr_fiq */ + + /* return address (EL1 ==> EL0) */ + uint64_t elr_el1; /* elr_el1, NA */ + /* return address (EL2 ==> EL1) */ + uint64_t pc64; /* elr_el2, elr_el2 */ + + /* spsr registers */ + uint32_t spsr_el1; /* spsr_el1, spsr_svc */ + uint32_t spsr_fiq; /* NA, spsr_fiq */ + uint32_t spsr_irq; /* NA, spsr_irq */ + uint32_t spsr_und; /* NA, spsr_und */ + uint32_t spsr_abt; /* NA, spsr_abt */ + + /* stack pointers */ + uint64_t sp_el0; /* sp_el0, NA */ + uint64_t sp_el1; /* sp_el1, NA */ + + /* guest mode */ + uint32_t cpsr; /* spsr_el2, spsr_el2 */ + + /* ======= Guest System Registers ======= + * - multiplexed for AArch32 and AArch64 guests + * - 64-bit preferred if needed (for 64-bit guests) + * - architecture specific registers are noted specifically + */ + /* exception */ + uint64_t vbar; /* vbar, vbar */ + + /* mmu related */ + uint64_t ttbcr; /* ttbcr, ttbcr */ + uint64_t ttbr0; /* ttbr0, ttbr0 */ + uint64_t ttbr1; /* ttbr1, ttbr1 */ + uint32_t dacr; /* NA, dacr32 */ + + uint64_t par; /* par, par */ + uint64_t mair0; /* mair, mair0 */ + uint64_t mair1; /* NA, mair1 */ + + /* fault status */ + uint32_t ifar; /* ifar, ifar */ + uint32_t ifsr; /* ifsr, ifsr */ + uint32_t dfar; /* dfar, dfar */ + uint32_t dfsr; /* dfsr, dfsr */ + + uint64_t far; /* far, far */ + uint64_t esr; /* esr, esr */ + + uint32_t afsr0; /* afsr0, afsr0 */ + uint32_t afsr1; /* afsr1, afsr1 */ + + /* thumbee and jazelle */ + uint32_t teecr; /* NA, teecr */ + uint32_t teehbr; /* NA, teehbr */ + + uint32_t joscr; /* NA, joscr */ + uint32_t jmcr; /* NA, jmcr */ + + /* control registers */ + uint32_t sctlr; /* sctlr, sctlr */ + uint32_t actlr; /* actlr, actlr */ + uint32_t cpacr; /* cpacr, cpacr */ + + uint32_t csselr; /* csselr, csselr */ + + /* software management related */ + uint32_t contextidr; /* contextidr, contextidr */ + uint64_t tpidr_el0; /* tpidr_el0, tpidr_el0 */ + uint64_t tpidr_el1; /* tpidr_el1, tpidr_el1 */ + uint64_t tpidrro_el0; /* tpidrro_el0, tdidrro_el0 */ +}; +DECLARE_HVM_SAVE_TYPE(VCPU, 5, struct hvm_arm_cpu); + /* * Largest type-code in use */ -#define HVM_SAVE_CODE_MAX 4 +#define HVM_SAVE_CODE_MAX 5 #endif