On 12/03/13 13:20, Christopher Covington wrote:
Hi Christopher,
> 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@xxxxxxx>
>> ---
>> 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?
This one is a bit of an odd one, as it controls the guest kernel
decision to expose its timer/counter to its own userspace. It is not
directly involved into the timekeeping itself.
As such, my choice was to make it part of the CPU state rather than the
timer state. But I admit this may not be the most obvious choice.
>> +#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".
>
> [...]
A few values here are EL2 specific (ESR_EL2_EC_*_HYP,
ESR_EL2_EC_HVC{32,64}, ESR_EL2_EC_CP1*...).
The fields themselves should be broadly compatible (I should go and
verify this though). Now, what I want to avoid is any sort of confusion
between exception levels, and make clear which level we're operating on.
If I can be sure we always operate in a non-ambiguous context, then _EL2
can go indeed.
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
