| Message ID | 20201109113233.9012-24-dbrazdil@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Opt-in always-on nVHE hypervisor | expand |
Hi David,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on dennis-percpu/for-next]
[also build test WARNING on linus/master v5.10-rc3 next-20201109]
[cannot apply to kvmarm/next arm64/for-next/core soc/for-next arm/for-next xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/David-Brazdil/Opt-in-always-on-nVHE-hypervisor/20201109-193833
base: https://git.kernel.org/pub/scm/linux/kernel/git/dennis/percpu.git for-next
config: arm64-randconfig-r022-20201109 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 09ec07827b1128504457a93dee80b2ceee1af600)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/0day-ci/linux/commit/a59ab708ed6039e83756720b1d5974e84db5a8f4
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review David-Brazdil/Opt-in-always-on-nVHE-hypervisor/20201109-193833
git checkout a59ab708ed6039e83756720b1d5974e84db5a8f4
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> arch/arm64/kvm/arm.c:1875:13: warning: no previous prototype for function 'kvm_patch_hcr_flags' [-Wmissing-prototypes]
void __init kvm_patch_hcr_flags(struct alt_instr *alt,
^
arch/arm64/kvm/arm.c:1875:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void __init kvm_patch_hcr_flags(struct alt_instr *alt,
^
static
1 warning generated.
vim +/kvm_patch_hcr_flags +1875 arch/arm64/kvm/arm.c
1874
> 1875 void __init kvm_patch_hcr_flags(struct alt_instr *alt,
1876 __le32 *origptr, __le32 *updptr, int nr_inst)
1877 {
1878 int i;
1879 u32 rd;
1880
1881 BUG_ON(nr_inst != 4);
1882
1883 /* Skip for VHE and unprotected nVHE modes. */
1884 if (!is_kvm_protected_mode())
1885 return;
1886
1887 rd = aarch64_insn_decode_register(AARCH64_INSN_REGTYPE_RD,
1888 le32_to_cpu(origptr[0]));
1889
1890 for (i = 0; i < nr_inst; i++) {
1891 u32 oinsn = __gen_mov_hcr_insn(HCR_HOST_NVHE_FLAGS, rd, i);
1892 u32 insn = __gen_mov_hcr_insn(HCR_HOST_NVHE_PROTECTED_FLAGS, rd, i);
1893
1894 BUG_ON(oinsn != le32_to_cpu(origptr[i]));
1895 updptr[i] = cpu_to_le32(insn);
1896 }
1897 }
1898
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 2020-11-09 11:32, David Brazdil wrote: > While protected nVHE KVM is installed, start trapping all host SMCs. > By default, these are simply forwarded to EL3, but PSCI SMCs are > validated first. > > Create new constant HCR_HOST_NVHE_PROTECTED_FLAGS with the new set of > HCR > flags to use while the nVHE vector is installed when the kernel was > booted with the protected flag enabled. Switch back to the default HCR > flags when switching back to the stub vector. > > Signed-off-by: David Brazdil <dbrazdil@google.com> > --- > arch/arm64/include/asm/kvm_arm.h | 1 + > arch/arm64/kernel/image-vars.h | 4 ++++ > arch/arm64/kvm/arm.c | 35 ++++++++++++++++++++++++++++++ > arch/arm64/kvm/hyp/nvhe/hyp-init.S | 8 +++++++ > arch/arm64/kvm/hyp/nvhe/switch.c | 5 ++++- > 5 files changed, 52 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/kvm_arm.h > b/arch/arm64/include/asm/kvm_arm.h > index 64ce29378467..4e90c2debf70 100644 > --- a/arch/arm64/include/asm/kvm_arm.h > +++ b/arch/arm64/include/asm/kvm_arm.h > @@ -80,6 +80,7 @@ > HCR_FMO | HCR_IMO | HCR_PTW ) > #define HCR_VIRT_EXCP_MASK (HCR_VSE | HCR_VI | HCR_VF) > #define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK | HCR_ATA) > +#define HCR_HOST_NVHE_PROTECTED_FLAGS (HCR_HOST_NVHE_FLAGS | HCR_TSC) > #define HCR_HOST_VHE_FLAGS (HCR_RW | HCR_TGE | HCR_E2H) > > /* TCR_EL2 Registers bits */ > diff --git a/arch/arm64/kernel/image-vars.h > b/arch/arm64/kernel/image-vars.h > index 78a42a7cdb72..75cda51674f4 100644 > --- a/arch/arm64/kernel/image-vars.h > +++ b/arch/arm64/kernel/image-vars.h > @@ -62,9 +62,13 @@ __efistub__ctype = _ctype; > */ > > /* Alternative callbacks for init-time patching of nVHE hyp code. */ > +KVM_NVHE_ALIAS(kvm_patch_hcr_flags); > KVM_NVHE_ALIAS(kvm_patch_vector_branch); > KVM_NVHE_ALIAS(kvm_update_va_mask); > > +/* Static key enabled when the user opted into nVHE protected mode. */ > +KVM_NVHE_ALIAS(kvm_protected_mode); > + > /* Global kernel state accessed by nVHE hyp code. */ > KVM_NVHE_ALIAS(kvm_vgic_global_state); > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index 574aa2d026e6..c09b95cfa00a 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -1861,6 +1861,41 @@ void kvm_arch_exit(void) > kvm_perf_teardown(); > } > > +static inline u32 __init __gen_mov_hcr_insn(u64 hcr, u32 rd, int i) > +{ > + int shift = 48 - (i * 16); > + u16 imm = (hcr >> shift) & GENMASK(16, 0); I really doubt you want to encode 17 bits. > + > + return aarch64_insn_gen_movewide(rd, imm, shift, > + AARCH64_INSN_VARIANT_64BIT, > + (i == 0) ? AARCH64_INSN_MOVEWIDE_ZERO > + : AARCH64_INSN_MOVEWIDE_KEEP); > +} I've added a generate_mov_q() helper as part of my host EL2 entry rework. We can probably share some stuff here. > + > +void __init kvm_patch_hcr_flags(struct alt_instr *alt, > + __le32 *origptr, __le32 *updptr, int nr_inst) > +{ > + int i; > + u32 rd; > + > + BUG_ON(nr_inst != 4); > + > + /* Skip for VHE and unprotected nVHE modes. */ > + if (!is_kvm_protected_mode()) > + return; > + > + rd = aarch64_insn_decode_register(AARCH64_INSN_REGTYPE_RD, > + le32_to_cpu(origptr[0])); > + > + for (i = 0; i < nr_inst; i++) { > + u32 oinsn = __gen_mov_hcr_insn(HCR_HOST_NVHE_FLAGS, rd, i); > + u32 insn = __gen_mov_hcr_insn(HCR_HOST_NVHE_PROTECTED_FLAGS, rd, i); > + > + BUG_ON(oinsn != le32_to_cpu(origptr[i])); > + updptr[i] = cpu_to_le32(insn); > + } > +} > + > static int __init early_kvm_protected_cfg(char *buf) > { > bool val; > diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-init.S > b/arch/arm64/kvm/hyp/nvhe/hyp-init.S > index f999a35b2c8c..bbe6c5f558e0 100644 > --- a/arch/arm64/kvm/hyp/nvhe/hyp-init.S > +++ b/arch/arm64/kvm/hyp/nvhe/hyp-init.S > @@ -88,6 +88,12 @@ SYM_CODE_END(__kvm_hyp_init) > * x0: struct kvm_nvhe_init_params PA > */ > SYM_CODE_START(___kvm_hyp_init) > +alternative_cb kvm_patch_hcr_flags > + mov_q x1, HCR_HOST_NVHE_FLAGS You really want to be careful here: the mov_q macro expands to 2, 3 or 4 instructions, depending on the input data... It is also odd that you have both a static key and a patching alternative. Why isn't "protected KVM" a capability that can be evaluated as a a non patching alternative? In general, I'd like to reserve patching alternatives to values that cannot be evaluated at compile time (VM offsets, for example). > +alternative_cb_end > + msr hcr_el2, x1 > + isb > + > ldr x1, [x0, #NVHE_INIT_TPIDR_EL2] > msr tpidr_el2, x1 > > @@ -220,6 +226,8 @@ reset: > bic x5, x5, x6 // Clear SCTL_M and etc > pre_disable_mmu_workaround > msr sctlr_el2, x5 > + mov_q x5, HCR_HOST_NVHE_FLAGS > + msr hcr_el2, x5 > isb > > /* Install stub vectors */ > diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c > b/arch/arm64/kvm/hyp/nvhe/switch.c > index 8ae8160bc93a..f605b25a9afc 100644 > --- a/arch/arm64/kvm/hyp/nvhe/switch.c > +++ b/arch/arm64/kvm/hyp/nvhe/switch.c > @@ -96,7 +96,10 @@ static void __deactivate_traps(struct kvm_vcpu > *vcpu) > mdcr_el2 |= MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT; > > write_sysreg(mdcr_el2, mdcr_el2); > - write_sysreg(HCR_HOST_NVHE_FLAGS, hcr_el2); > + if (is_kvm_protected_mode()) > + write_sysreg(HCR_HOST_NVHE_PROTECTED_FLAGS, hcr_el2); > + else > + write_sysreg(HCR_HOST_NVHE_FLAGS, hcr_el2); > write_sysreg(CPTR_EL2_DEFAULT, cptr_el2); > write_sysreg(__kvm_hyp_host_vector, vbar_el2); > } Thanks, M.
> > +++ b/arch/arm64/kvm/hyp/nvhe/hyp-init.S > > @@ -88,6 +88,12 @@ SYM_CODE_END(__kvm_hyp_init) > > * x0: struct kvm_nvhe_init_params PA > > */ > > SYM_CODE_START(___kvm_hyp_init) > > +alternative_cb kvm_patch_hcr_flags > > + mov_q x1, HCR_HOST_NVHE_FLAGS > > You really want to be careful here: the mov_q macro expands to 2, 3 or 4 > instructions, depending on the input data... > > It is also odd that you have both a static key and a patching alternative. > Why isn't "protected KVM" a capability that can be evaluated as a a non > patching alternative? In general, I'd like to reserve patching alternatives > to values that cannot be evaluated at compile time (VM offsets, for > example). Capability was my initial idea as well but it looked tied to CPU features. Looking at it again, you're right that there is precedent for setting them from kernel params. Alright, I'll change it and that will get rid of the custom patching.
diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h index 64ce29378467..4e90c2debf70 100644 --- a/arch/arm64/include/asm/kvm_arm.h +++ b/arch/arm64/include/asm/kvm_arm.h @@ -80,6 +80,7 @@ HCR_FMO | HCR_IMO | HCR_PTW ) #define HCR_VIRT_EXCP_MASK (HCR_VSE | HCR_VI | HCR_VF) #define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK | HCR_ATA) +#define HCR_HOST_NVHE_PROTECTED_FLAGS (HCR_HOST_NVHE_FLAGS | HCR_TSC) #define HCR_HOST_VHE_FLAGS (HCR_RW | HCR_TGE | HCR_E2H) /* TCR_EL2 Registers bits */ diff --git a/arch/arm64/kernel/image-vars.h b/arch/arm64/kernel/image-vars.h index 78a42a7cdb72..75cda51674f4 100644 --- a/arch/arm64/kernel/image-vars.h +++ b/arch/arm64/kernel/image-vars.h @@ -62,9 +62,13 @@ __efistub__ctype = _ctype; */ /* Alternative callbacks for init-time patching of nVHE hyp code. */ +KVM_NVHE_ALIAS(kvm_patch_hcr_flags); KVM_NVHE_ALIAS(kvm_patch_vector_branch); KVM_NVHE_ALIAS(kvm_update_va_mask); +/* Static key enabled when the user opted into nVHE protected mode. */ +KVM_NVHE_ALIAS(kvm_protected_mode); + /* Global kernel state accessed by nVHE hyp code. */ KVM_NVHE_ALIAS(kvm_vgic_global_state); diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 574aa2d026e6..c09b95cfa00a 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -1861,6 +1861,41 @@ void kvm_arch_exit(void) kvm_perf_teardown(); } +static inline u32 __init __gen_mov_hcr_insn(u64 hcr, u32 rd, int i) +{ + int shift = 48 - (i * 16); + u16 imm = (hcr >> shift) & GENMASK(16, 0); + + return aarch64_insn_gen_movewide(rd, imm, shift, + AARCH64_INSN_VARIANT_64BIT, + (i == 0) ? AARCH64_INSN_MOVEWIDE_ZERO + : AARCH64_INSN_MOVEWIDE_KEEP); +} + +void __init kvm_patch_hcr_flags(struct alt_instr *alt, + __le32 *origptr, __le32 *updptr, int nr_inst) +{ + int i; + u32 rd; + + BUG_ON(nr_inst != 4); + + /* Skip for VHE and unprotected nVHE modes. */ + if (!is_kvm_protected_mode()) + return; + + rd = aarch64_insn_decode_register(AARCH64_INSN_REGTYPE_RD, + le32_to_cpu(origptr[0])); + + for (i = 0; i < nr_inst; i++) { + u32 oinsn = __gen_mov_hcr_insn(HCR_HOST_NVHE_FLAGS, rd, i); + u32 insn = __gen_mov_hcr_insn(HCR_HOST_NVHE_PROTECTED_FLAGS, rd, i); + + BUG_ON(oinsn != le32_to_cpu(origptr[i])); + updptr[i] = cpu_to_le32(insn); + } +} + static int __init early_kvm_protected_cfg(char *buf) { bool val; diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-init.S b/arch/arm64/kvm/hyp/nvhe/hyp-init.S index f999a35b2c8c..bbe6c5f558e0 100644 --- a/arch/arm64/kvm/hyp/nvhe/hyp-init.S +++ b/arch/arm64/kvm/hyp/nvhe/hyp-init.S @@ -88,6 +88,12 @@ SYM_CODE_END(__kvm_hyp_init) * x0: struct kvm_nvhe_init_params PA */ SYM_CODE_START(___kvm_hyp_init) +alternative_cb kvm_patch_hcr_flags + mov_q x1, HCR_HOST_NVHE_FLAGS +alternative_cb_end + msr hcr_el2, x1 + isb + ldr x1, [x0, #NVHE_INIT_TPIDR_EL2] msr tpidr_el2, x1 @@ -220,6 +226,8 @@ reset: bic x5, x5, x6 // Clear SCTL_M and etc pre_disable_mmu_workaround msr sctlr_el2, x5 + mov_q x5, HCR_HOST_NVHE_FLAGS + msr hcr_el2, x5 isb /* Install stub vectors */ diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c index 8ae8160bc93a..f605b25a9afc 100644 --- a/arch/arm64/kvm/hyp/nvhe/switch.c +++ b/arch/arm64/kvm/hyp/nvhe/switch.c @@ -96,7 +96,10 @@ static void __deactivate_traps(struct kvm_vcpu *vcpu) mdcr_el2 |= MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT; write_sysreg(mdcr_el2, mdcr_el2); - write_sysreg(HCR_HOST_NVHE_FLAGS, hcr_el2); + if (is_kvm_protected_mode()) + write_sysreg(HCR_HOST_NVHE_PROTECTED_FLAGS, hcr_el2); + else + write_sysreg(HCR_HOST_NVHE_FLAGS, hcr_el2); write_sysreg(CPTR_EL2_DEFAULT, cptr_el2); write_sysreg(__kvm_hyp_host_vector, vbar_el2); }
While protected nVHE KVM is installed, start trapping all host SMCs. By default, these are simply forwarded to EL3, but PSCI SMCs are validated first. Create new constant HCR_HOST_NVHE_PROTECTED_FLAGS with the new set of HCR flags to use while the nVHE vector is installed when the kernel was booted with the protected flag enabled. Switch back to the default HCR flags when switching back to the stub vector. Signed-off-by: David Brazdil <dbrazdil@google.com> --- arch/arm64/include/asm/kvm_arm.h | 1 + arch/arm64/kernel/image-vars.h | 4 ++++ arch/arm64/kvm/arm.c | 35 ++++++++++++++++++++++++++++++ arch/arm64/kvm/hyp/nvhe/hyp-init.S | 8 +++++++ arch/arm64/kvm/hyp/nvhe/switch.c | 5 ++++- 5 files changed, 52 insertions(+), 1 deletion(-)