Re: [Xen-devel] [RFC v3 4/6] xen/arm: Add save/restore support for guest core registers

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxxx>
> Signed-off-by: Wei Huang <w1.huang@xxxxxxxxxxx>
> ---
>  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
>  



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel