Re: [PATCH 07/59] KVM: arm64: nv: Add EL2 system registers to vcpu context
On 6/21/19 10:37 AM, Marc Zyngier wrote:
> From: Jintack Lim
>
> ARM v8.3 introduces a new bit in the HCR_EL2, which is the NV bit. When
> this bit is set, accessing EL2 registers in EL1 traps to EL2. In
> addition, executing the following instructions in EL1 will trap to EL2:
> tlbi, at, eret, and msr/mrs instructions to access SP_EL1. Most of the
> instructions that trap to EL2 with the NV bit were undef at EL1 prior to
> ARM v8.3. The only instruction that was not undef is eret.
>
> This patch sets up a handler for EL2 registers and SP_EL1 register
> accesses at EL1. The host hypervisor keeps those register values in
> memory, and will emulate their behavior.
>
> This patch doesn't set the NV bit yet. It will be set in a later patch
> once nested virtualization support is completed.
>
> Signed-off-by: Jintack Lim
> Signed-off-by: Marc Zyngier
> ---
> arch/arm64/include/asm/kvm_host.h | 37 +++-
> arch/arm64/include/asm/sysreg.h | 50 -
> arch/arm64/kvm/sys_regs.c | 74 ---
> 3 files changed, 154 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index 4bcd9c1291d5..2d4290d2513a 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -173,12 +173,47 @@ enum vcpu_sysreg {
> APGAKEYLO_EL1,
> APGAKEYHI_EL1,
>
> - /* 32bit specific registers. Keep them at the end of the range */
> + /* 32bit specific registers. */
> DACR32_EL2, /* Domain Access Control Register */
> IFSR32_EL2, /* Instruction Fault Status Register */
> FPEXC32_EL2,/* Floating-Point Exception Control Register */
> DBGVCR32_EL2, /* Debug Vector Catch Register */
>
> + /* EL2 registers sorted ascending by Op0, Op1, CRn, CRm, Op2 */
> + FIRST_EL2_SYSREG,
> + VPIDR_EL2 = FIRST_EL2_SYSREG,
> + /* Virtualization Processor ID Register */
> + VMPIDR_EL2, /* Virtualization Multiprocessor ID Register */
> + SCTLR_EL2, /* System Control Register (EL2) */
> + ACTLR_EL2, /* Auxiliary Control Register (EL2) */
> + HCR_EL2,/* Hypervisor Configuration Register */
> + MDCR_EL2, /* Monitor Debug Configuration Register (EL2) */
> + CPTR_EL2, /* Architectural Feature Trap Register (EL2) */
> + HSTR_EL2, /* Hypervisor System Trap Register */
> + HACR_EL2, /* Hypervisor Auxiliary Control Register */
> + TTBR0_EL2, /* Translation Table Base Register 0 (EL2) */
> + TTBR1_EL2, /* Translation Table Base Register 1 (EL2) */
> + TCR_EL2,/* Translation Control Register (EL2) */
> + VTTBR_EL2, /* Virtualization Translation Table Base Register */
> + VTCR_EL2, /* Virtualization Translation Control Register */
> + SPSR_EL2, /* EL2 saved program status register */
> + ELR_EL2,/* EL2 exception link register */
> + AFSR0_EL2, /* Auxiliary Fault Status Register 0 (EL2) */
> + AFSR1_EL2, /* Auxiliary Fault Status Register 1 (EL2) */
> + ESR_EL2,/* Exception Syndrome Register (EL2) */
> + FAR_EL2,/* Hypervisor IPA Fault Address Register */
> + HPFAR_EL2, /* Hypervisor IPA Fault Address Register */
> + MAIR_EL2, /* Memory Attribute Indirection Register (EL2) */
> + AMAIR_EL2, /* Auxiliary Memory Attribute Indirection Register
> (EL2) */
> + VBAR_EL2, /* Vector Base Address Register (EL2) */
> + RVBAR_EL2, /* Reset Vector Base Address Register */
> + RMR_EL2,/* Reset Management Register */
> + CONTEXTIDR_EL2, /* Context ID Register (EL2) */
> + TPIDR_EL2, /* EL2 Software Thread ID Register */
> + CNTVOFF_EL2,/* Counter-timer Virtual Offset register */
> + CNTHCTL_EL2,/* Counter-timer Hypervisor Control register */
> + SP_EL2, /* EL2 Stack Pointer */
> +
> NR_SYS_REGS /* Nothing after this line! */
> };
>
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index f3ca7e4796ab..8b95f2c42c3d 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -411,17 +411,49 @@
>
> #define SYS_PMCCFILTR_EL0sys_reg(3, 3, 14, 15, 7)
>
> +#define SYS_VPIDR_EL2sys_reg(3, 4, 0, 0, 0)
> +#define SYS_VMPIDR_EL2 sys_reg(3, 4, 0, 0, 5)
> +
> +#define SYS_SCTLR_EL2sys_reg(3, 4, 1, 0, 0)
> +#define SYS_ACTLR_EL2sys_reg(3, 4, 1, 0, 1)
> +#define SYS_HCR_EL2 sys_reg(3, 4, 1, 1, 0)
> +#define SYS_MDCR_EL2 sys_reg(3, 4, 1, 1, 1)
> +#define SYS_CPTR_EL2 sys_reg(3, 4, 1, 1, 2)
> +#define SYS_HSTR_EL2 sys_reg(3, 4, 1, 1, 3)
> +#define SYS_HACR_EL2 sys_reg(3, 4, 1, 1, 7)
> +
> #define SYS_
Re: [PATCH RFC 11/14] arm64: Move the ASID allocator code in a separate file
On Mon, Jun 24, 2019 at 12:35:35AM +0800, Guo Ren wrote:
> On Fri, Jun 21, 2019 at 10:16 PM Catalin Marinas
> wrote:
> > BTW, if you find the algorithm fairly straightforward ;), see this
> > bug-fix which took a formal model to identify: a8ffaaa060b8 ("arm64:
> > asid: Do not replace active_asids if already 0").
[...]
> Btw, Is this detected by arm's aisd allocator TLA+ model ? Or a real
> bug report ?
This specific bug was found by the TLA+ model checker (at the time we
were actually tracking down another bug with multi-threaded CPU sharing
the TLB, bug also confirmed by the formal model).
--
Catalin
Re: [PATCH 15/59] KVM: arm64: nv: Refactor vcpu_{read,write}_sys_reg
On 06/21/2019 10:37 AM, Marc Zyngier wrote:
> Extract the direct HW accessors for later reuse.
>
> Signed-off-by: Marc Zyngier
> ---
> arch/arm64/kvm/sys_regs.c | 247 +-
> 1 file changed, 139 insertions(+), 108 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 2b8734f75a09..e181359adadf 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -182,99 +182,161 @@ const struct el2_sysreg_map *find_el2_sysreg(const
> struct el2_sysreg_map *map,
> return entry;
> }
>
> +static bool __vcpu_read_sys_reg_from_cpu(int reg, u64 *val)
> +{
> + /*
> + * System registers listed in the switch are not saved on every
> + * exit from the guest but are only saved on vcpu_put.
> + *
> + * Note that MPIDR_EL1 for the guest is set by KVM via VMPIDR_EL2 but
> + * should never be listed below, because the guest cannot modify its
> + * own MPIDR_EL1 and MPIDR_EL1 is accessed for VCPU A from VCPU B's
> + * thread when emulating cross-VCPU communication.
> + */
> + switch (reg) {
> + case CSSELR_EL1:*val = read_sysreg_s(SYS_CSSELR_EL1); break;
> + case SCTLR_EL1: *val = read_sysreg_s(SYS_SCTLR_EL12); break;
> + case ACTLR_EL1: *val = read_sysreg_s(SYS_ACTLR_EL1);break;
> + case CPACR_EL1: *val = read_sysreg_s(SYS_CPACR_EL12); break;
> + case TTBR0_EL1: *val = read_sysreg_s(SYS_TTBR0_EL12); break;
> + case TTBR1_EL1: *val = read_sysreg_s(SYS_TTBR1_EL12); break;
> + case TCR_EL1: *val = read_sysreg_s(SYS_TCR_EL12); break;
> + case ESR_EL1: *val = read_sysreg_s(SYS_ESR_EL12); break;
> + case AFSR0_EL1: *val = read_sysreg_s(SYS_AFSR0_EL12); break;
> + case AFSR1_EL1: *val = read_sysreg_s(SYS_AFSR1_EL12); break;
> + case FAR_EL1: *val = read_sysreg_s(SYS_FAR_EL12); break;
> + case MAIR_EL1: *val = read_sysreg_s(SYS_MAIR_EL12);break;
> + case VBAR_EL1: *val = read_sysreg_s(SYS_VBAR_EL12);break;
> + case CONTEXTIDR_EL1:*val = read_sysreg_s(SYS_CONTEXTIDR_EL12);break;
> + case TPIDR_EL0: *val = read_sysreg_s(SYS_TPIDR_EL0);break;
> + case TPIDRRO_EL0: *val = read_sysreg_s(SYS_TPIDRRO_EL0); break;
> + case TPIDR_EL1: *val = read_sysreg_s(SYS_TPIDR_EL1);break;
> + case AMAIR_EL1: *val = read_sysreg_s(SYS_AMAIR_EL12); break;
> + case CNTKCTL_EL1: *val = read_sysreg_s(SYS_CNTKCTL_EL12); break;
> + case PAR_EL1: *val = read_sysreg_s(SYS_PAR_EL1); break;
> + case DACR32_EL2:*val = read_sysreg_s(SYS_DACR32_EL2); break;
> + case IFSR32_EL2:*val = read_sysreg_s(SYS_IFSR32_EL2); break;
> + case DBGVCR32_EL2: *val = read_sysreg_s(SYS_DBGVCR32_EL2); break;
> + default:return false;
> + }
> +
> + return true;
> +}
> +
> +static bool __vcpu_write_sys_reg_to_cpu(u64 val, int reg)
> +{
> + /*
> + * System registers listed in the switch are not restored on every
> + * entry to the guest but are only restored on vcpu_load.
> + *
> + * Note that MPIDR_EL1 for the guest is set by KVM via VMPIDR_EL2 but
> + * should never be listed below, because the the MPIDR should only be
> + * set once, before running the VCPU, and never changed later.
> + */
> + switch (reg) {
> + case CSSELR_EL1:write_sysreg_s(val, SYS_CSSELR_EL1);break;
> + case SCTLR_EL1: write_sysreg_s(val, SYS_SCTLR_EL12);break;
> + case ACTLR_EL1: write_sysreg_s(val, SYS_ACTLR_EL1); break;
> + case CPACR_EL1: write_sysreg_s(val, SYS_CPACR_EL12);break;
> + case TTBR0_EL1: write_sysreg_s(val, SYS_TTBR0_EL12);break;
> + case TTBR1_EL1: write_sysreg_s(val, SYS_TTBR1_EL12);break;
> + case TCR_EL1: write_sysreg_s(val, SYS_TCR_EL12); break;
> + case ESR_EL1: write_sysreg_s(val, SYS_ESR_EL12); break;
> + case AFSR0_EL1: write_sysreg_s(val, SYS_AFSR0_EL12);break;
> + case AFSR1_EL1: write_sysreg_s(val, SYS_AFSR1_EL12);break;
> + case FAR_EL1: write_sysreg_s(val, SYS_FAR_EL12); break;
> + case MAIR_EL1: write_sysreg_s(val, SYS_MAIR_EL12); break;
> + case VBAR_EL1: write_sysreg_s(val, SYS_VBAR_EL12); break;
> + case CONTEXTIDR_EL1:write_sysreg_s(val, SYS_CONTEXTIDR_EL12);break;
> + case TPIDR_EL0: write_sysreg_s(val, SYS_TPIDR_EL0); break;
> + case TPIDRRO_EL0: write_sysreg_s(val, SYS_TPIDRRO_EL0); break;
> + case TPIDR_EL1: write_sysreg_s(val, SYS_TPIDR_EL1); break;
> + case AMAIR_EL1: write_sysreg_s(val, SYS_AMAIR_EL12);break;
> + case CNTKCTL_EL1: write_s
Re: [PATCH 09/59] KVM: arm64: nv: Add nested virt VCPU primitives for vEL2 VCPU state
On Fri, Jun 21, 2019 at 10:37:53AM +0100, Marc Zyngier wrote:
> From: Christoffer Dall
>
> When running a nested hypervisor we commonly have to figure out if
> the VCPU mode is running in the context of a guest hypervisor or guest
> guest, or just a normal guest.
>
> Add convenient primitives for this.
>
> Signed-off-by: Christoffer Dall
> Signed-off-by: Marc Zyngier
> ---
> arch/arm64/include/asm/kvm_emulate.h | 55
> 1 file changed, 55 insertions(+)
>
> diff --git a/arch/arm64/include/asm/kvm_emulate.h
> b/arch/arm64/include/asm/kvm_emulate.h
> index 39ffe41855bc..8f201ea56f6e 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -191,6 +191,61 @@ static inline void vcpu_set_reg(struct kvm_vcpu *vcpu,
> u8 reg_num,
> vcpu_gp_regs(vcpu)->regs.regs[reg_num] = val;
> }
>
> +static inline bool vcpu_mode_el2_ctxt(const struct kvm_cpu_context *ctxt)
> +{
> + unsigned long cpsr = ctxt->gp_regs.regs.pstate;
> + u32 mode;
> +
> + if (cpsr & PSR_MODE32_BIT)
> + return false;
> +
> + mode = cpsr & PSR_MODE_MASK;
> +
> + return mode == PSR_MODE_EL2h || mode == PSR_MODE_EL2t;
We could also treat PSR_MODE32_BIT and PSR_MODE_MASK as a single field,
similarly as in the next patch, say:
switch (ctxt->gp_regs.regs.pstate & (PSR_MODE32_BIT | PSR_MODE_MASK)) {
case PSR_MODE_EL2h:
case PSR_MODE_EL2t:
return true;
}
return false;
(This is blatant bikeshedding...)
[...]
Cheers
---Dave
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 01/59] KVM: arm64: Migrate _elx sysreg accessors to msr_s/mrs_s
On 6/21/19 10:37 AM, Marc Zyngier wrote:
> From: Dave Martin
>
> Currently, the {read,write}_sysreg_el*() accessors for accessing
> particular ELs' sysregs in the presence of VHE rely on some local
> hacks and define their system register encodings in a way that is
> inconsistent with the core definitions in .
>
> As a result, it is necessary to add duplicate definitions for any
> system register that already needs a definition in sysreg.h for
> other reasons.
>
> This is a bit of a maintenance headache, and the reasons for the
> _el*() accessors working the way they do is a bit historical.
>
> This patch gets rid of the shadow sysreg definitions in
> , converts the _el*() accessors to use the core
> __msr_s/__mrs_s interface, and converts all call sites to use the
> standard sysreg #define names (i.e., upper case, with SYS_ prefix).
>
> This patch will conflict heavily anyway, so the opportunity taken
> to clean up some bad whitespace in the context of the changes is
> taken.
>
> The change exposes a few system registers that have no sysreg.h
> definition, due to msr_s/mrs_s being used in place of msr/mrs:
> additions are made in order to fill in the gaps.
>
> Signed-off-by: Dave Martin
> Cc: Catalin Marinas
> Cc: Christoffer Dall
> Cc: Mark Rutland
> Cc: Will Deacon
> Link: https://www.spinics.net/lists/kvm-arm/msg31717.html
> [Rebased to v4.21-rc1]
> Signed-off-by: Sudeep Holla
> [Rebased to v5.2-rc5, changelog updates]
> Signed-off-by: Marc Zyngier
> ---
> arch/arm/include/asm/kvm_hyp.h | 13 ++--
> arch/arm64/include/asm/kvm_emulate.h | 16 ++---
> arch/arm64/include/asm/kvm_hyp.h | 50 ++-
> arch/arm64/include/asm/sysreg.h | 35 ++-
> arch/arm64/kvm/hyp/switch.c | 14 ++---
> arch/arm64/kvm/hyp/sysreg-sr.c | 78
> arch/arm64/kvm/hyp/tlb.c | 12 ++--
> arch/arm64/kvm/hyp/vgic-v2-cpuif-proxy.c | 2 +-
> arch/arm64/kvm/regmap.c | 4 +-
> arch/arm64/kvm/sys_regs.c| 56 -
> virt/kvm/arm/arch_timer.c| 24
> 11 files changed, 148 insertions(+), 156 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_hyp.h b/arch/arm/include/asm/kvm_hyp.h
> index 87bcd18df8d5..059224fb14db 100644
> --- a/arch/arm/include/asm/kvm_hyp.h
> +++ b/arch/arm/include/asm/kvm_hyp.h
> @@ -93,13 +93,14 @@
> #define VFP_FPEXC__ACCESS_VFP(FPEXC)
>
> /* AArch64 compatibility macros, only for the timer so far */
> -#define read_sysreg_el0(r) read_sysreg(r##_el0)
> -#define write_sysreg_el0(v, r) write_sysreg(v, r##_el0)
> +#define read_sysreg_el0(r) read_sysreg(r##_EL0)
> +#define write_sysreg_el0(v, r) write_sysreg(v, r##_EL0)
> +
> +#define SYS_CNTP_CTL_EL0 CNTP_CTL
> +#define SYS_CNTP_CVAL_EL0CNTP_CVAL
> +#define SYS_CNTV_CTL_EL0 CNTV_CTL
> +#define SYS_CNTV_CVAL_EL0CNTV_CVAL
>
> -#define cntp_ctl_el0 CNTP_CTL
> -#define cntp_cval_el0CNTP_CVAL
> -#define cntv_ctl_el0 CNTV_CTL
> -#define cntv_cval_el0CNTV_CVAL
> #define cntvoff_el2 CNTVOFF
> #define cnthctl_el2 CNTHCTL
>
> diff --git a/arch/arm64/include/asm/kvm_emulate.h
> b/arch/arm64/include/asm/kvm_emulate.h
> index 613427fafff9..39ffe41855bc 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -137,7 +137,7 @@ static inline unsigned long *__vcpu_elr_el1(const struct
> kvm_vcpu *vcpu)
> static inline unsigned long vcpu_read_elr_el1(const struct kvm_vcpu *vcpu)
> {
> if (vcpu->arch.sysregs_loaded_on_cpu)
> - return read_sysreg_el1(elr);
> + return read_sysreg_el1(SYS_ELR);
> else
> return *__vcpu_elr_el1(vcpu);
> }
> @@ -145,7 +145,7 @@ static inline unsigned long vcpu_read_elr_el1(const
> struct kvm_vcpu *vcpu)
> static inline void vcpu_write_elr_el1(const struct kvm_vcpu *vcpu, unsigned
> long v)
> {
> if (vcpu->arch.sysregs_loaded_on_cpu)
> - write_sysreg_el1(v, elr);
> + write_sysreg_el1(v, SYS_ELR);
> else
> *__vcpu_elr_el1(vcpu) = v;
> }
> @@ -197,7 +197,7 @@ static inline unsigned long vcpu_read_spsr(const struct
> kvm_vcpu *vcpu)
> return vcpu_read_spsr32(vcpu);
>
> if (vcpu->arch.sysregs_loaded_on_cpu)
> - return read_sysreg_el1(spsr);
> + return read_sysreg_el1(SYS_SPSR);
> else
> return vcpu_gp_regs(vcpu)->spsr[KVM_SPSR_EL1];
> }
> @@ -210,7 +210,7 @@ static inline void vcpu_write_spsr(struct kvm_vcpu *vcpu,
> unsigned long v)
> }
>
> if (vcpu->arch.sysregs_loaded_on_cpu)
> - write_sysreg_el1(v, spsr);
> + write_sysreg_el1(v, SYS_SPSR);
> else
> vcpu_gp_regs(vcpu)->spsr[KVM_
Re: [PATCH 08/59] KVM: arm64: nv: Reset VMPIDR_EL2 and VPIDR_EL2 to sane values
On Fri, Jun 21, 2019 at 10:37:52AM +0100, Marc Zyngier wrote:
> The VMPIDR_EL2 and VPIDR_EL2 are architecturally UNKNOWN at reset, but
> let's be nice to a guest hypervisor behaving foolishly and reset these
> to something reasonable anyway.
Why be nice? Generally we do try to initialise UNKNOWN regs to garbage,
to help trip up badly-written guests.
Cheers
---Dave
>
> Signed-off-by: Christoffer Dall
> Signed-off-by: Marc Zyngier
> ---
> arch/arm64/kvm/sys_regs.c | 25 +
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index e81be6debe07..693dd063c9c2 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -624,7 +624,7 @@ static void reset_amair_el1(struct kvm_vcpu *vcpu, const
> struct sys_reg_desc *r)
> vcpu_write_sys_reg(vcpu, amair, AMAIR_EL1);
> }
>
> -static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> +static u64 compute_reset_mpidr(struct kvm_vcpu *vcpu)
> {
> u64 mpidr;
>
> @@ -638,7 +638,24 @@ static void reset_mpidr(struct kvm_vcpu *vcpu, const
> struct sys_reg_desc *r)
> mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
> mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
> mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
> - vcpu_write_sys_reg(vcpu, (1ULL << 31) | mpidr, MPIDR_EL1);
> + mpidr |= (1ULL << 31);
> +
> + return mpidr;
> +}
> +
> +static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> +{
> + vcpu_write_sys_reg(vcpu, compute_reset_mpidr(vcpu), MPIDR_EL1);
> +}
> +
> +static void reset_vmpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> +{
> + vcpu_write_sys_reg(vcpu, compute_reset_mpidr(vcpu), VMPIDR_EL2);
> +}
> +
> +static void reset_vpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> +{
> + vcpu_write_sys_reg(vcpu, read_cpuid_id(), VPIDR_EL2);
> }
>
> static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> @@ -1668,8 +1685,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>*/
> { SYS_DESC(SYS_PMCCFILTR_EL0), access_pmu_evtyper, reset_val,
> PMCCFILTR_EL0, 0 },
>
> - { SYS_DESC(SYS_VPIDR_EL2), access_rw, reset_val, VPIDR_EL2, 0 },
> - { SYS_DESC(SYS_VMPIDR_EL2), access_rw, reset_val, VMPIDR_EL2, 0 },
> + { SYS_DESC(SYS_VPIDR_EL2), access_rw, reset_vpidr, VPIDR_EL2 },
> + { SYS_DESC(SYS_VMPIDR_EL2), access_rw, reset_vmpidr, VMPIDR_EL2 },
>
> { SYS_DESC(SYS_SCTLR_EL2), access_rw, reset_val, SCTLR_EL2, 0 },
> { SYS_DESC(SYS_ACTLR_EL2), access_rw, reset_val, ACTLR_EL2, 0 },
> --
> 2.20.1
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 07/59] KVM: arm64: nv: Add EL2 system registers to vcpu context
On Fri, Jun 21, 2019 at 10:37:51AM +0100, Marc Zyngier wrote:
> From: Jintack Lim
>
> ARM v8.3 introduces a new bit in the HCR_EL2, which is the NV bit. When
> this bit is set, accessing EL2 registers in EL1 traps to EL2. In
> addition, executing the following instructions in EL1 will trap to EL2:
> tlbi, at, eret, and msr/mrs instructions to access SP_EL1. Most of the
> instructions that trap to EL2 with the NV bit were undef at EL1 prior to
> ARM v8.3. The only instruction that was not undef is eret.
>
> This patch sets up a handler for EL2 registers and SP_EL1 register
> accesses at EL1. The host hypervisor keeps those register values in
> memory, and will emulate their behavior.
>
> This patch doesn't set the NV bit yet. It will be set in a later patch
> once nested virtualization support is completed.
>
> Signed-off-by: Jintack Lim
> Signed-off-by: Marc Zyngier
> ---
> arch/arm64/include/asm/kvm_host.h | 37 +++-
> arch/arm64/include/asm/sysreg.h | 50 -
> arch/arm64/kvm/sys_regs.c | 74 ---
> 3 files changed, 154 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index 4bcd9c1291d5..2d4290d2513a 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -173,12 +173,47 @@ enum vcpu_sysreg {
> APGAKEYLO_EL1,
> APGAKEYHI_EL1,
>
> - /* 32bit specific registers. Keep them at the end of the range */
> + /* 32bit specific registers. */
Out of interest, why did we originally want these to be at the end?
Because they're not at the end any more...
> DACR32_EL2, /* Domain Access Control Register */
> IFSR32_EL2, /* Instruction Fault Status Register */
> FPEXC32_EL2,/* Floating-Point Exception Control Register */
> DBGVCR32_EL2, /* Debug Vector Catch Register */
>
> + /* EL2 registers sorted ascending by Op0, Op1, CRn, CRm, Op2 */
> + FIRST_EL2_SYSREG,
> + VPIDR_EL2 = FIRST_EL2_SYSREG,
> + /* Virtualization Processor ID Register */
> + VMPIDR_EL2, /* Virtualization Multiprocessor ID Register */
> + SCTLR_EL2, /* System Control Register (EL2) */
> + ACTLR_EL2, /* Auxiliary Control Register (EL2) */
> + HCR_EL2,/* Hypervisor Configuration Register */
> + MDCR_EL2, /* Monitor Debug Configuration Register (EL2) */
> + CPTR_EL2, /* Architectural Feature Trap Register (EL2) */
> + HSTR_EL2, /* Hypervisor System Trap Register */
> + HACR_EL2, /* Hypervisor Auxiliary Control Register */
> + TTBR0_EL2, /* Translation Table Base Register 0 (EL2) */
> + TTBR1_EL2, /* Translation Table Base Register 1 (EL2) */
> + TCR_EL2,/* Translation Control Register (EL2) */
> + VTTBR_EL2, /* Virtualization Translation Table Base Register */
> + VTCR_EL2, /* Virtualization Translation Control Register */
> + SPSR_EL2, /* EL2 saved program status register */
> + ELR_EL2,/* EL2 exception link register */
> + AFSR0_EL2, /* Auxiliary Fault Status Register 0 (EL2) */
> + AFSR1_EL2, /* Auxiliary Fault Status Register 1 (EL2) */
> + ESR_EL2,/* Exception Syndrome Register (EL2) */
> + FAR_EL2,/* Hypervisor IPA Fault Address Register */
> + HPFAR_EL2, /* Hypervisor IPA Fault Address Register */
> + MAIR_EL2, /* Memory Attribute Indirection Register (EL2) */
> + AMAIR_EL2, /* Auxiliary Memory Attribute Indirection Register
> (EL2) */
> + VBAR_EL2, /* Vector Base Address Register (EL2) */
> + RVBAR_EL2, /* Reset Vector Base Address Register */
> + RMR_EL2,/* Reset Management Register */
> + CONTEXTIDR_EL2, /* Context ID Register (EL2) */
> + TPIDR_EL2, /* EL2 Software Thread ID Register */
> + CNTVOFF_EL2,/* Counter-timer Virtual Offset register */
> + CNTHCTL_EL2,/* Counter-timer Hypervisor Control register */
> + SP_EL2, /* EL2 Stack Pointer */
> +
I wonder whether we could make these conditionally present somehow. Not
worth worrying about for now to save 200-odd bytes per vcpu though.
[...]
Cheers
---Dave
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 06/59] KVM: arm64: nv: Allow userspace to set PSR_MODE_EL2x
On Fri, Jun 21, 2019 at 02:50:08PM +0100, Marc Zyngier wrote: > On 21/06/2019 14:24, Julien Thierry wrote: > > > > > > On 21/06/2019 10:37, Marc Zyngier wrote: > >> From: Christoffer Dall > >> > >> We were not allowing userspace to set a more privileged mode for the VCPU > >> than EL1, but we should allow this when nested virtualization is enabled > >> for the VCPU. > >> > >> Signed-off-by: Christoffer Dall > >> Signed-off-by: Marc Zyngier > >> --- > >> arch/arm64/kvm/guest.c | 6 ++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > >> index 3ae2f82fca46..4c35b5d51e21 100644 > >> --- a/arch/arm64/kvm/guest.c > >> +++ b/arch/arm64/kvm/guest.c > >> @@ -37,6 +37,7 @@ > >> #include > >> #include > >> #include > >> +#include > >> #include > >> > >> #include "trace.h" > >> @@ -194,6 +195,11 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const > >> struct kvm_one_reg *reg) > >>if (vcpu_el1_is_32bit(vcpu)) > >>return -EINVAL; > >>break; > >> + case PSR_MODE_EL2h: > >> + case PSR_MODE_EL2t: > >> + if (vcpu_el1_is_32bit(vcpu) || > >> !nested_virt_in_use(vcpu)) > > > > This condition reads a bit weirdly. Why do we care about anything else > > than !nested_virt_in_use() ? > > > > If nested virt is not in use then obviously we return the error. > > > > If nested virt is in use then why do we care about EL1? Or should this > > test read as "highest_el_is_32bit" ? > > There are multiple things at play here: > > - MODE_EL2x is not a valid 32bit mode > - The architecture forbids nested virt with 32bit EL2 > > The code above is a simplification of these two conditions. But > certainly we can do a bit better, as kvm_reset_cpu() doesn't really > check that we don't create a vcpu with both 32bit+NV. These two bits > should really be exclusive. This code is safe for now because KVM_VCPU_MAX_FEATURES <= KVM_ARM_VCPU_NESTED_VIRT, right, i.e., nested_virt_in_use() cannot be true? This makes me a little uneasy, but I think that's paranoia talking: we want bisectably, but no sane person should ship with just half of this series. So I guess this is fine. We could stick something like if (WARN_ON(...)) return false; in nested_virt_in_use() and then remove it in the final patch, but it's probably overkill. Cheers ---Dave ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 13/59] KVM: arm64: nv: Handle virtual EL2 registers in vcpu_read/write_sys_reg()
On 06/21/2019 10:37 AM, Marc Zyngier wrote:
> From: Andre Przywara
>
> KVM internally uses accessor functions when reading or writing the
> guest's system registers. This takes care of accessing either the stored
> copy or using the "live" EL1 system registers when the host uses VHE.
>
> With the introduction of virtual EL2 we add a bunch of EL2 system
> registers, which now must also be taken care of:
> - If the guest is running in vEL2, and we access an EL1 sysreg, we must
> revert to the stored version of that, and not use the CPU's copy.
> - If the guest is running in vEL1, and we access an EL2 sysreg, we must
> also use the stored version, since the CPU carries the EL1 copy.
> - Some EL2 system registers are supposed to affect the current execution
> of the system, so we need to put them into their respective EL1
> counterparts. For this we need to define a mapping between the two.
> This is done using the newly introduced struct el2_sysreg_map.
> - Some EL2 system registers have a different format than their EL1
> counterpart, so we need to translate them before writing them to the
> CPU. This is done using an (optional) translate function in the map.
> - There are the three special registers SP_EL2, SPSR_EL2 and ELR_EL2,
> which need some separate handling.
>
> All of these cases are now wrapped into the existing accessor functions,
> so KVM users wouldn't need to care whether they access EL2 or EL1
> registers and also which state the guest is in.
>
> This handles what was formerly known as the "shadow state" dynamically,
> without requiring a separate copy for each vCPU EL.
>
> Signed-off-by: Andre Przywara
> Signed-off-by: Marc Zyngier
> ---
> arch/arm64/include/asm/kvm_emulate.h | 6 +
> arch/arm64/include/asm/kvm_host.h| 5 +
> arch/arm64/kvm/sys_regs.c| 163 +++
> 3 files changed, 174 insertions(+)
>
> diff --git a/arch/arm64/include/asm/kvm_emulate.h
> b/arch/arm64/include/asm/kvm_emulate.h
> index c43aac5fed69..f37006b6eec4 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -70,6 +70,12 @@ void kvm_emulate_nested_eret(struct kvm_vcpu *vcpu);
> int kvm_inject_nested_sync(struct kvm_vcpu *vcpu, u64 esr_el2);
> int kvm_inject_nested_irq(struct kvm_vcpu *vcpu);
>
> +u64 translate_tcr(u64 tcr);
> +u64 translate_cptr(u64 tcr);
> +u64 translate_sctlr(u64 tcr);
> +u64 translate_ttbr0(u64 tcr);
> +u64 translate_cnthctl(u64 tcr);
> +
> static inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu)
> {
> return !(vcpu->arch.hcr_el2 & HCR_RW);
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index 2d4290d2513a..dae9c42a7219 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -217,6 +217,11 @@ enum vcpu_sysreg {
> NR_SYS_REGS /* Nothing after this line! */
> };
>
> +static inline bool sysreg_is_el2(int reg)
> +{
> + return reg >= FIRST_EL2_SYSREG && reg < NR_SYS_REGS;
> +}
> +
> /* 32bit mapping */
> #define c0_MPIDR (MPIDR_EL1 * 2) /* MultiProcessor ID Register */
> #define c0_CSSELR(CSSELR_EL1 * 2)/* Cache Size Selection Register */
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 693dd063c9c2..d024114da162 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -76,11 +76,142 @@ static bool write_to_read_only(struct kvm_vcpu *vcpu,
> return false;
> }
>
> +static u64 tcr_el2_ips_to_tcr_el1_ps(u64 tcr_el2)
> +{
> + return ((tcr_el2 & TCR_EL2_PS_MASK) >> TCR_EL2_PS_SHIFT)
> + << TCR_IPS_SHIFT;
> +}
> +
> +u64 translate_tcr(u64 tcr)
> +{
> + return TCR_EPD1_MASK | /* disable TTBR1_EL1 */
> +((tcr & TCR_EL2_TBI) ? TCR_TBI0 : 0) |
> +tcr_el2_ips_to_tcr_el1_ps(tcr) |
> +(tcr & TCR_EL2_TG0_MASK) |
> +(tcr & TCR_EL2_ORGN0_MASK) |
> +(tcr & TCR_EL2_IRGN0_MASK) |
> +(tcr & TCR_EL2_T0SZ_MASK);
> +}
> +
> +u64 translate_cptr(u64 cptr_el2)
> +{
> + u64 cpacr_el1 = 0;
> +
> + if (!(cptr_el2 & CPTR_EL2_TFP))
> + cpacr_el1 |= CPACR_EL1_FPEN;
> + if (cptr_el2 & CPTR_EL2_TTA)
> + cpacr_el1 |= CPACR_EL1_TTA;
> + if (!(cptr_el2 & CPTR_EL2_TZ))
> + cpacr_el1 |= CPACR_EL1_ZEN;
> +
> + return cpacr_el1;
> +}
> +
> +u64 translate_sctlr(u64 sctlr)
> +{
> + /* Bit 20 is RES1 in SCTLR_EL1, but RES0 in SCTLR_EL2 */
> + return sctlr | BIT(20);
> +}
> +
> +u64 translate_ttbr0(u64 ttbr0)
> +{
> + /* Force ASID to 0 (ASID 0 or RES0) */
> + return ttbr0 & ~GENMASK_ULL(63, 48);
> +}
> +
> +u64 translate_cnthctl(u64 cnthctl)
> +{
> + return ((cnthctl & 0x3) << 10) | (cnthctl & 0xfc);
> +}
> +
> +#define EL2_SYSREG(el2, el1, translate) \
> + [el2 - FIRST_EL2_SYSREG] = { el2, el1, translate }
> +#define PURE_EL2_SYSREG(el2) \
> + [el
Re: [PATCH 04/59] KVM: arm64: nv: Introduce nested virtualization VCPU feature
On Fri, Jun 21, 2019 at 10:37:48AM +0100, Marc Zyngier wrote:
> From: Christoffer Dall
>
> Introduce the feature bit and a primitive that checks if the feature is
> set behind a static key check based on the cpus_have_const_cap check.
>
> Checking nested_virt_in_use() on systems without nested virt enabled
> should have neglgible overhead.
>
> We don't yet allow userspace to actually set this feature.
>
> Signed-off-by: Christoffer Dall
> Signed-off-by: Marc Zyngier
> ---
[...]
> diff --git a/arch/arm64/include/asm/kvm_nested.h
> b/arch/arm64/include/asm/kvm_nested.h
> new file mode 100644
> index ..8a3d121a0b42
> --- /dev/null
> +++ b/arch/arm64/include/asm/kvm_nested.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ARM64_KVM_NESTED_H
> +#define __ARM64_KVM_NESTED_H
> +
> +#include
> +
> +static inline bool nested_virt_in_use(const struct kvm_vcpu *vcpu)
> +{
> + return cpus_have_const_cap(ARM64_HAS_NESTED_VIRT) &&
> + test_bit(KVM_ARM_VCPU_NESTED_VIRT, vcpu->arch.features);
> +}
Also, is it worth having a vcpu->arch.flags flag for this, similarly to
SVE and ptrauth?
[...]
Cheers
---Dave
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 05/59] KVM: arm64: nv: Reset VCPU to EL2 registers if VCPU nested virt is set
On Fri, Jun 21, 2019 at 10:37:49AM +0100, Marc Zyngier wrote:
> From: Christoffer Dall
>
> Reset the VCPU with PSTATE.M = EL2h when the nested virtualization
> feature is enabled on the VCPU.
>
> Signed-off-by: Christoffer Dall
> Signed-off-by: Marc Zyngier
> ---
> arch/arm64/kvm/reset.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index 1140b4485575..675ca07dbb05 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -52,6 +52,11 @@ static const struct kvm_regs default_regs_reset = {
> PSR_F_BIT | PSR_D_BIT),
> };
>
> +static const struct kvm_regs default_regs_reset_el2 = {
> + .regs.pstate = (PSR_MODE_EL2h | PSR_A_BIT | PSR_I_BIT |
> + PSR_F_BIT | PSR_D_BIT),
> +};
> +
Is it worth having a #define for the common non-mode bits? It's a bit
weird for EL2 and EL1 to have indepedent DAIF defaults.
Putting a big block of zeros in the kernel text just to initialise one
register seems overkill. Now we're adding a third block of zeros,
maybe this is worth refactoring? We really just need a memset(0)
followed by config-dependent initialisation of regs.pstate AFAICT.
Not a big deal though: this doesn't look like a high risk for
maintainability.
Cheers
---Dave
> static const struct kvm_regs default_regs_reset32 = {
> .regs.pstate = (PSR_AA32_MODE_SVC | PSR_AA32_A_BIT |
> PSR_AA32_I_BIT | PSR_AA32_F_BIT),
> @@ -302,6 +307,8 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> if (!cpu_has_32bit_el1())
> goto out;
> cpu_reset = &default_regs_reset32;
> + } else if (test_bit(KVM_ARM_VCPU_NESTED_VIRT,
> vcpu->arch.features)) {
> + cpu_reset = &default_regs_reset_el2;
> } else {
> cpu_reset = &default_regs_reset;
> }
> --
> 2.20.1
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 04/59] KVM: arm64: nv: Introduce nested virtualization VCPU feature
On Fri, Jun 21, 2019 at 10:37:48AM +0100, Marc Zyngier wrote:
> From: Christoffer Dall
>
> Introduce the feature bit and a primitive that checks if the feature is
> set behind a static key check based on the cpus_have_const_cap check.
>
> Checking nested_virt_in_use() on systems without nested virt enabled
> should have neglgible overhead.
>
> We don't yet allow userspace to actually set this feature.
>
> Signed-off-by: Christoffer Dall
> Signed-off-by: Marc Zyngier
> ---
> arch/arm/include/asm/kvm_nested.h | 9 +
> arch/arm64/include/asm/kvm_nested.h | 13 +
> arch/arm64/include/uapi/asm/kvm.h | 1 +
> 3 files changed, 23 insertions(+)
> create mode 100644 arch/arm/include/asm/kvm_nested.h
> create mode 100644 arch/arm64/include/asm/kvm_nested.h
>
> diff --git a/arch/arm/include/asm/kvm_nested.h
> b/arch/arm/include/asm/kvm_nested.h
> new file mode 100644
> index ..124ff6445f8f
> --- /dev/null
> +++ b/arch/arm/include/asm/kvm_nested.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ARM_KVM_NESTED_H
> +#define __ARM_KVM_NESTED_H
> +
> +#include
> +
> +static inline bool nested_virt_in_use(const struct kvm_vcpu *vcpu) { return
> false; }
> +
> +#endif /* __ARM_KVM_NESTED_H */
> diff --git a/arch/arm64/include/asm/kvm_nested.h
> b/arch/arm64/include/asm/kvm_nested.h
> new file mode 100644
> index ..8a3d121a0b42
> --- /dev/null
> +++ b/arch/arm64/include/asm/kvm_nested.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ARM64_KVM_NESTED_H
> +#define __ARM64_KVM_NESTED_H
> +
> +#include
> +
> +static inline bool nested_virt_in_use(const struct kvm_vcpu *vcpu)
> +{
> + return cpus_have_const_cap(ARM64_HAS_NESTED_VIRT) &&
> + test_bit(KVM_ARM_VCPU_NESTED_VIRT, vcpu->arch.features);
> +}
> +
> +#endif /* __ARM64_KVM_NESTED_H */
> diff --git a/arch/arm64/include/uapi/asm/kvm.h
> b/arch/arm64/include/uapi/asm/kvm.h
> index d819a3e8b552..563e2a8bae93 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -106,6 +106,7 @@ struct kvm_regs {
> #define KVM_ARM_VCPU_SVE 4 /* enable SVE for this CPU */
> #define KVM_ARM_VCPU_PTRAUTH_ADDRESS 5 /* VCPU uses address authentication */
> #define KVM_ARM_VCPU_PTRAUTH_GENERIC 6 /* VCPU uses generic authentication */
> +#define KVM_ARM_VCPU_NESTED_VIRT 7 /* Support nested virtualization */
This seems weirdly named:
Isn't the feature we're exposing here really EL2? In that case, the
feature the guest gets with this flag enabled is plain virtualisation,
possibly with the option to nest further.
Does the guest also get nested virt (i.e., recursively nested virt from
the host's PoV) as a side effect, or would require an explicit extra
flag?
Cheers
---Dave
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 03/59] arm64: Add ARM64_HAS_NESTED_VIRT cpufeature
On Fri, Jun 21, 2019 at 10:37:47AM +0100, Marc Zyngier wrote: > From: Jintack Lim > > Add a new ARM64_HAS_NESTED_VIRT feature to indicate that the > CPU has the ARMv8.3 nested virtualization capability. > > This will be used to support nested virtualization in KVM. > > Signed-off-by: Jintack Lim > Signed-off-by: Andre Przywara > Signed-off-by: Christoffer Dall > Signed-off-by: Marc Zyngier > --- > .../admin-guide/kernel-parameters.txt | 4 +++ > arch/arm64/include/asm/cpucaps.h | 3 ++- > arch/arm64/include/asm/sysreg.h | 1 + > arch/arm64/kernel/cpufeature.c| 26 +++ > 4 files changed, 33 insertions(+), 1 deletion(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt > b/Documentation/admin-guide/kernel-parameters.txt > index 138f6664b2e2..202bb2115d83 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -2046,6 +2046,10 @@ > [KVM,ARM] Allow use of GICv4 for direct injection of > LPIs. > > + kvm-arm.nested= > + [KVM,ARM] Allow nested virtualization in KVM/ARM. > + Default is 0 (disabled) > + In light of the discussion on this patch, is it worth making 0 not guarantee that nested is allowed, rather than guaranteeing to disable nested? This would allow the option to be turned into a no-op later once the NV code is considered mature enough to rip out all the conditionality. [...] Cheers ---Dave ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH RFC 11/14] arm64: Move the ASID allocator code in a separate file
On Mon, Jun 24, 2019 at 12:35:35AM +0800, Guo Ren wrote: > On Fri, Jun 21, 2019 at 10:16 PM Catalin Marinas > wrote: > > > > On Wed, Jun 19, 2019 at 07:51:03PM +0800, Guo Ren wrote: > > > On Wed, Jun 19, 2019 at 4:54 PM Julien Grall wrote: > > > > On 6/19/19 9:07 AM, Guo Ren wrote: > > > > > Move arm asid allocator code in a generic one is a agood idea, I've > > > > > made a patchset for C-SKY and test is on processing, See: > > > > > https://lore.kernel.org/linux-csky/1560930553-26502-1-git-send-email-guo...@kernel.org/ > > > > > > > > > > If you plan to seperate it into generic one, I could co-work with you. > > > > > > > > Was the ASID allocator work out of box on C-Sky? > > > > > > Almost done, but one question: > > > arm64 remove the code in switch_mm: > > > cpumask_clear_cpu(cpu, mm_cpumask(prev)); > > > cpumask_set_cpu(cpu, mm_cpumask(next)); > > > > > > Why? Although arm64 cache operations could affect all harts with CTC > > > method of interconnect, I think we should keep these code for > > > primitive integrity in linux. Because cpu_bitmap is in mm_struct > > > instead of mm->context. > > > > We didn't have a use for this in the arm64 code, so no point in > > maintaining the mm_cpumask. On some arm32 systems (ARMv6) with no > > hardware broadcast of some TLB/cache operations, we use it to track > > where the task has run to issue IPI for TLB invalidation or some > > deferred I-cache invalidation. > The operation of set/clear mm_cpumask was removed in arm64 compared to > arm32. It seems no side effect on current arm64 system, but from > software meaning it's wrong. > I think we should keep mm_cpumask just like arm32. It was a while ago now, but I remember the atomic update of the mm_cpumask being quite expensive when I was profiling this stuff, so I removed it because we don't need it for arm64 (at least, it doesn't allow us to optimise our shootdowns in practice). I still think this is over-engineered for what you want on c-sky and making this code generic is a mistake. Will ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 02/59] KVM: arm64: Move __load_guest_stage2 to kvm_mmu.h
On Fri, Jun 21, 2019 at 10:37:46AM +0100, Marc Zyngier wrote: > Having __load_guest_stage2 in kvm_hyp.h is quickly going to trigger > a circular include problem. In order to avoid this, let's move > it to kvm_mmu.h, where it will be a better fit anyway. > > In the process, drop the __hyp_text annotation, which doesn't help > as the function is marked as __always_inline. Does GCC always inline things marked __always_inline? I seem to remember some gotchas in this area, but I may be being paranoid. If this still only called from hyp, I'd be tempted to heep the __hyp_text annotation just to be on the safe side. [...] Cheers ---Dave ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 01/59] KVM: arm64: Migrate _elx sysreg accessors to msr_s/mrs_s
On Fri, Jun 21, 2019 at 10:37:45AM +0100, Marc Zyngier wrote:
> From: Dave Martin
>
> Currently, the {read,write}_sysreg_el*() accessors for accessing
> particular ELs' sysregs in the presence of VHE rely on some local
> hacks and define their system register encodings in a way that is
> inconsistent with the core definitions in .
>
> As a result, it is necessary to add duplicate definitions for any
> system register that already needs a definition in sysreg.h for
> other reasons.
>
> This is a bit of a maintenance headache, and the reasons for the
> _el*() accessors working the way they do is a bit historical.
>
> This patch gets rid of the shadow sysreg definitions in
> , converts the _el*() accessors to use the core
> __msr_s/__mrs_s interface, and converts all call sites to use the
> standard sysreg #define names (i.e., upper case, with SYS_ prefix).
>
> This patch will conflict heavily anyway, so the opportunity taken
> to clean up some bad whitespace in the context of the changes is
> taken.
FWIW, "opportunity taken ... is taken".
Anway, Ack, thanks to you and Sudeep for keeping this alive.
Cheers
---Dave
> The change exposes a few system registers that have no sysreg.h
> definition, due to msr_s/mrs_s being used in place of msr/mrs:
> additions are made in order to fill in the gaps.
>
> Signed-off-by: Dave Martin
> Cc: Catalin Marinas
> Cc: Christoffer Dall
> Cc: Mark Rutland
> Cc: Will Deacon
> Link: https://www.spinics.net/lists/kvm-arm/msg31717.html
> [Rebased to v4.21-rc1]
> Signed-off-by: Sudeep Holla
> [Rebased to v5.2-rc5, changelog updates]
> Signed-off-by: Marc Zyngier
> ---
> arch/arm/include/asm/kvm_hyp.h | 13 ++--
> arch/arm64/include/asm/kvm_emulate.h | 16 ++---
> arch/arm64/include/asm/kvm_hyp.h | 50 ++-
> arch/arm64/include/asm/sysreg.h | 35 ++-
> arch/arm64/kvm/hyp/switch.c | 14 ++---
> arch/arm64/kvm/hyp/sysreg-sr.c | 78
> arch/arm64/kvm/hyp/tlb.c | 12 ++--
> arch/arm64/kvm/hyp/vgic-v2-cpuif-proxy.c | 2 +-
> arch/arm64/kvm/regmap.c | 4 +-
> arch/arm64/kvm/sys_regs.c| 56 -
> virt/kvm/arm/arch_timer.c| 24
> 11 files changed, 148 insertions(+), 156 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_hyp.h b/arch/arm/include/asm/kvm_hyp.h
> index 87bcd18df8d5..059224fb14db 100644
> --- a/arch/arm/include/asm/kvm_hyp.h
> +++ b/arch/arm/include/asm/kvm_hyp.h
> @@ -93,13 +93,14 @@
> #define VFP_FPEXC__ACCESS_VFP(FPEXC)
>
> /* AArch64 compatibility macros, only for the timer so far */
> -#define read_sysreg_el0(r) read_sysreg(r##_el0)
> -#define write_sysreg_el0(v, r) write_sysreg(v, r##_el0)
> +#define read_sysreg_el0(r) read_sysreg(r##_EL0)
> +#define write_sysreg_el0(v, r) write_sysreg(v, r##_EL0)
> +
> +#define SYS_CNTP_CTL_EL0 CNTP_CTL
> +#define SYS_CNTP_CVAL_EL0CNTP_CVAL
> +#define SYS_CNTV_CTL_EL0 CNTV_CTL
> +#define SYS_CNTV_CVAL_EL0CNTV_CVAL
>
> -#define cntp_ctl_el0 CNTP_CTL
> -#define cntp_cval_el0CNTP_CVAL
> -#define cntv_ctl_el0 CNTV_CTL
> -#define cntv_cval_el0CNTV_CVAL
> #define cntvoff_el2 CNTVOFF
> #define cnthctl_el2 CNTHCTL
>
> diff --git a/arch/arm64/include/asm/kvm_emulate.h
> b/arch/arm64/include/asm/kvm_emulate.h
> index 613427fafff9..39ffe41855bc 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -137,7 +137,7 @@ static inline unsigned long *__vcpu_elr_el1(const struct
> kvm_vcpu *vcpu)
> static inline unsigned long vcpu_read_elr_el1(const struct kvm_vcpu *vcpu)
> {
> if (vcpu->arch.sysregs_loaded_on_cpu)
> - return read_sysreg_el1(elr);
> + return read_sysreg_el1(SYS_ELR);
> else
> return *__vcpu_elr_el1(vcpu);
> }
> @@ -145,7 +145,7 @@ static inline unsigned long vcpu_read_elr_el1(const
> struct kvm_vcpu *vcpu)
> static inline void vcpu_write_elr_el1(const struct kvm_vcpu *vcpu, unsigned
> long v)
> {
> if (vcpu->arch.sysregs_loaded_on_cpu)
> - write_sysreg_el1(v, elr);
> + write_sysreg_el1(v, SYS_ELR);
> else
> *__vcpu_elr_el1(vcpu) = v;
> }
> @@ -197,7 +197,7 @@ static inline unsigned long vcpu_read_spsr(const struct
> kvm_vcpu *vcpu)
> return vcpu_read_spsr32(vcpu);
>
> if (vcpu->arch.sysregs_loaded_on_cpu)
> - return read_sysreg_el1(spsr);
> + return read_sysreg_el1(SYS_SPSR);
> else
> return vcpu_gp_regs(vcpu)->spsr[KVM_SPSR_EL1];
> }
> @@ -210,7 +210,7 @@ static inline void vcpu_write_spsr(struct kvm_vcpu *vcpu,
> unsigned long v)
> }
>
> if (vcpu->arch.sysregs_loaded_on_cpu)
>
Re: [PATCH RFC 11/14] arm64: Move the ASID allocator code in a separate file
On Thu, Jun 20, 2019 at 05:33:03PM +0800, Guo Ren wrote: > On Wed, Jun 19, 2019 at 8:39 PM Will Deacon wrote: > > > > On Wed, Jun 19, 2019 at 08:18:04PM +0800, Guo Ren wrote: > > > On Wed, Jun 19, 2019 at 5:12 PM Will Deacon wrote: > > > > This is one place where I'd actually prefer not to go down the route of > > > > making the code generic. Context-switching and low-level TLB management > > > > is deeply architecture-specific and I worry that by trying to make this > > > > code common, we run the real risk of introducing subtle bugs on some > > > > architecture every time it is changed. > > > "Add generic asid code" and "move arm's into generic" are two things. > > > We could do > > > first and let architecture's maintainer to choose. > > > > If I understand the proposal being discussed, it involves basing that > > generic ASID allocation code around the arm64 implementation which I don't > > necessarily think is a good starting point. > ... > > > > > > Furthermore, the algorithm we use > > > > on arm64 is designed to scale to large systems using DVM and may well be > > > > too complex and/or sub-optimal for architectures with different system > > > > topologies or TLB invalidation mechanisms. > > > It's just a asid algorithm not very complex and there is a callback > > > for architecture to define their > > > own local hart tlb flush. Seems it has nothing with DVM or tlb > > > broadcast mechanism. > > > > I'm pleased that you think the algorithm is not very complex, but I'm also > > worried that you might not have fully understood some of its finer details. > I understand your concern about my less understanding of asid > technology. Here is > my short-description of arm64 asid allocator: (If you find anything > wrong, please > correct me directly, thx :) The complexity mainly comes from the fact that this thing runs concurrently with itself without synchronization on the fast-path. Coupled with the need to use the same ASID for all threads of a task, you end up in fiddly situations where rollover can occur on one CPU whilst another CPU is trying to schedule a thread of a task that already has threads running in userspace. However, it's architecture-specific whether or not you care about that scenario. > > The reason I mention DVM and TLB broadcasting is because, depending on > > the mechanisms in your architecture relating to those, it may be strictly > > required that all concurrently running threads of a process have the same > > ASID at any given point in time, or it may be that you really don't care. > > > > If you don't care, then the arm64 allocator is over-engineered and likely > > inefficient for your system. If you do care, then it's worth considering > > whether a lock is sufficient around the allocator if you don't expect high > > core counts. Another possibility is that you end up using only one ASID and > > invalidating the local TLB on every context switch. Yet another design > > would be to manage per-cpu ASID pools. > I'll keep my system use the same ASID for SMP + IOMMU :P You will want a separate allocator for that: https://lkml.kernel.org/r/20190610184714.6786-2-jean-philippe.bruc...@arm.com > Yes, there are two styles of asid allocator: per-cpu ASID (MIPS) or > same ASID (ARM). > If the CPU couldn't support cache/tlb coherency maintian in hardware, > it should use > per-cpu ASID style because IPI is expensive and per-cpu ASID style > need more software > mechanism to improve performance (eg: delay cache flush). From software view > the > same ASID is clearer and easier to build bigger system with more TLB caches. > > I think the same ASID style is a more sensible choice for modern > processor and let it be > one of generic is reasonable. I'm not sure I agree. x86, for example, is better off using a different algorithm for allocating its PCIDs. > > So rather than blindly copying the arm64 code, I suggest sitting down and > > designing something that fits to your architecture instead. You may end up > > with something that is both simpler and more efficient. > In fact, riscv folks have discussed a lot about arm's asid allocator > and I learned > a lot from the discussion: > https://lore.kernel.org/linux-riscv/20190327100201.32220-1-anup.pa...@wdc.com/ If you require all threads of the same process to have the same ASID, then that patch looks broken to me. Will
Re: [PATCH 05/59] KVM: arm64: nv: Reset VCPU to EL2 registers if VCPU nested virt is set
On 21/06/2019 10:37, Marc Zyngier wrote:
From: Christoffer Dall
Reset the VCPU with PSTATE.M = EL2h when the nested virtualization
feature is enabled on the VCPU.
Signed-off-by: Christoffer Dall
Signed-off-by: Marc Zyngier
---
arch/arm64/kvm/reset.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index 1140b4485575..675ca07dbb05 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -52,6 +52,11 @@ static const struct kvm_regs default_regs_reset = {
PSR_F_BIT | PSR_D_BIT),
};
+static const struct kvm_regs default_regs_reset_el2 = {
+ .regs.pstate = (PSR_MODE_EL2h | PSR_A_BIT | PSR_I_BIT |
+ PSR_F_BIT | PSR_D_BIT),
+};
+
static const struct kvm_regs default_regs_reset32 = {
.regs.pstate = (PSR_AA32_MODE_SVC | PSR_AA32_A_BIT |
PSR_AA32_I_BIT | PSR_AA32_F_BIT),
@@ -302,6 +307,8 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
if (!cpu_has_32bit_el1())
goto out;
cpu_reset = &default_regs_reset32;
+ } else if (test_bit(KVM_ARM_VCPU_NESTED_VIRT,
vcpu->arch.features)) {
minor nit: "else if nested_virt_in_use(vcpu)" ?
Either ways:
Reviewed-by: Suzuki K Poulose
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
