Re: [PATCH 04/29] arm64: KVM: system register definitions for 64bit guests

Christopher Covington
2013-03-12T13:20:44Z

Hi Marc,

Here are a few minor questions and suggestions.

On 03/04/2013 10:47 PM, Marc Zyngier wrote:
> Define the saved/restored registers for 64bit guests.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> arch/arm64/include/asm/kvm_asm.h | 68 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 68 insertions(+)
> create mode 100644 arch/arm64/include/asm/kvm_asm.h

[...]

> +/*
> + * 0 is reserved as an invalid value.
> + * Order *must* be kept in sync with the hyp switch code.
> + */
> +#define MPIDR_EL1 1 /* MultiProcessor Affinity Register */
> +#define CSSELR_EL1 2 /* Cache Size Selection Register */
> +#define SCTLR_EL1 3 /* System Control Register */
> +#define ACTLR_EL1 4 /* Auxilliary Control Register */
> +#define CPACR_EL1 5 /* Coprocessor Access Control */
> +#define TTBR0_EL1 6 /* Translation Table Base Register 0 */
> +#define TTBR1_EL1 7 /* Translation Table Base Register 1 */
> +#define TCR_EL1 8 /* Translation Control Register */
> +#define ESR_EL1 9 /* Exception Syndrome Register */
> +#define AFSR0_EL1 10 /* Auxilary Fault Status Register 0 */
> +#define AFSR1_EL1 11 /* Auxilary Fault Status Register 1 */
> +#define FAR_EL1 12 /* Fault Address Register */
> +#define MAIR_EL1 13 /* Memory Attribute Indirection Register */
> +#define VBAR_EL1 14 /* Vector Base Address Register */
> +#define CONTEXTIDR_EL1 15 /* Context ID Register */
> +#define TPIDR_EL0 16 /* Thread ID, User R/W */
> +#define TPIDRRO_EL0 17 /* Thread ID, User R/O */
> +#define TPIDR_EL1 18 /* Thread ID, Privileged */
> +#define AMAIR_EL1 19 /* Aux Memory Attribute Indirection Register */
> +#define CNTKCTL_EL1 20 /* Timer Control Register (EL1) */

Any particular reason to have CNTKCTL_EL1 enumerated here and handled by
(dump|load)_sysregs, but all the other timer registers handled by
(save|restore)_timer_state in hyp.S?

> +#define NR_SYS_REGS 21

[...]

> +/* Hyp Syndrome Register (HSR) bits */
> +#define ESR_EL2_EC_SHIFT (26)
> +#define ESR_EL2_EC (0x3fU << ESR_EL2_EC_SHIFT)
> +#define ESR_EL2_IL (1U << 25)
> +#define ESR_EL2_ISS (ESR_EL2_IL - 1)
> +#define ESR_EL2_ISV_SHIFT (24)
> +#define ESR_EL2_ISV (1U << ESR_EL2_ISV_SHIFT)
> +#define ESR_EL2_SAS_SHIFT (22)
> +#define ESR_EL2_SAS (3U << ESR_EL2_SAS_SHIFT)
> +#define ESR_EL2_SSE (1 << 21)
> +#define ESR_EL2_SRT_SHIFT (16)
> +#define ESR_EL2_SRT_MASK (0x1f << ESR_EL2_SRT_SHIFT)
> +#define ESR_EL2_SF (1 << 15)
> +#define ESR_EL2_AR (1 << 14)
> +#define ESR_EL2_EA (1 << 9)
> +#define ESR_EL2_CM (1 << 8)
> +#define ESR_EL2_S1PTW (1 << 7)
> +#define ESR_EL2_WNR (1 << 6)
> +#define ESR_EL2_FSC (0x3f)
> +#define ESR_EL2_FSC_TYPE (0x3c)
> +
> +#define ESR_EL2_CV_SHIFT (24)
> +#define ESR_EL2_CV (1U << ESR_EL2_CV_SHIFT)
> +#define ESR_EL2_COND_SHIFT (20)
> +#define ESR_EL2_COND (0xfU << ESR_EL2_COND_SHIFT)

[...]

> +#define ESR_EL2_EC_UNKNOWN (0x00)
> +#define ESR_EL2_EC_WFI (0x01)
> +#define ESR_EL2_EC_CP15_32 (0x03)
> +#define ESR_EL2_EC_CP15_64 (0x04)
> +#define ESR_EL2_EC_CP14_MR (0x05)
> +#define ESR_EL2_EC_CP14_LS (0x06)
> +#define ESR_EL2_EC_SIMD_FP (0x07)
> +#define ESR_EL2_EC_CP10_ID (0x08)
> +#define ESR_EL2_EC_CP14_64 (0x0C)
> +#define ESR_EL2_EC_ILL_ISS (0x0E)
> +#define ESR_EL2_EC_SVC32 (0x11)
> +#define ESR_EL2_EC_HVC32 (0x12)
> +#define ESR_EL2_EC_SMC32 (0x13)
> +#define ESR_EL2_EC_SVC64 (0x14)
> +#define ESR_EL2_EC_HVC64 (0x16)
> +#define ESR_EL2_EC_SMC64 (0x17)
> +#define ESR_EL2_EC_SYS64 (0x18)
> +#define ESR_EL2_EC_IABT (0x20)
> +#define ESR_EL2_EC_IABT_HYP (0x21)
> +#define ESR_EL2_EC_PC_ALIGN (0x22)
> +#define ESR_EL2_EC_DABT (0x24)
> +#define ESR_EL2_EC_DABT_HYP (0x25)
> +#define ESR_EL2_EC_SP_ALIGN (0x26)
> +#define ESR_EL2_EC_FP32 (0x28)
> +#define ESR_EL2_EC_FP64 (0x2C)
> +#define ESR_EL2_EC_SERRROR (0x2F)
> +#define ESR_EL2_EC_BREAKPT (0x30)
> +#define ESR_EL2_EC_BREAKPT_HYP (0x31)
> +#define ESR_EL2_EC_SOFTSTP (0x32)
> +#define ESR_EL2_EC_SOFTSTP_HYP (0x33)
> +#define ESR_EL2_EC_WATCHPT (0x34)
> +#define ESR_EL2_EC_WATCHPT_HYP (0x35)
> +#define ESR_EL2_EC_BKPT32 (0x38)
> +#define ESR_EL2_EC_VECTOR32 (0x3A)
> +#define ESR_EL2_EC_BKPT64 (0x3C)

Is there any functional difference between these fields and bits at EL2 and
these fields at other exception levels? If not, you could consider omitting
"_EL2".

[...]

Regards,
Christopher

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
the Linux Foundation