| Message ID | 20230424234704.2571444-3-jingzhangos@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support writable CPU ID registers from userspace | expand |
Hi Jing,
kernel test robot noticed the following build errors:
[auto build test ERROR on 6a8f57ae2eb07ab39a6f0ccad60c760743051026]
url: https://github.com/intel-lab-lkp/linux/commits/Jing-Zhang/KVM-arm64-Move-CPU-ID-feature-registers-emulation-into-a-separate-file/20230425-074908
base: 6a8f57ae2eb07ab39a6f0ccad60c760743051026
patch link: https://lore.kernel.org/r/20230424234704.2571444-3-jingzhangos%40google.com
patch subject: [PATCH v7 2/6] KVM: arm64: Save ID registers' sanitized value per guest
config: arm64-randconfig-r012-20230425 (https://download.01.org/0day-ci/archive/20230425/202304251438.2kYTSPa2-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 437b7602e4a998220871de78afcb020b9c14a661)
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/intel-lab-lkp/linux/commit/cca571c516149d39bbaf1b5e916df617940458a6
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jing-Zhang/KVM-arm64-Move-CPU-ID-feature-registers-emulation-into-a-separate-file/20230425-074908
git checkout cca571c516149d39bbaf1b5e916df617940458a6
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 prepare
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304251438.2kYTSPa2-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from arch/arm64/kernel/asm-offsets.c:16:
In file included from include/linux/kvm_host.h:45:
>> arch/arm64/include/asm/kvm_host.h:1109:20: error: no member named 'config_lock' in 'struct kvm_arch'
mutex_lock(&arch->config_lock);
~~~~ ^
arch/arm64/include/asm/kvm_host.h:1111:22: error: no member named 'config_lock' in 'struct kvm_arch'
mutex_unlock(&arch->config_lock);
~~~~ ^
arch/arm64/include/asm/kvm_host.h:1118:20: error: no member named 'config_lock' in 'struct kvm_arch'
mutex_lock(&arch->config_lock);
~~~~ ^
arch/arm64/include/asm/kvm_host.h:1120:22: error: no member named 'config_lock' in 'struct kvm_arch'
mutex_unlock(&arch->config_lock);
~~~~ ^
4 errors generated.
make[2]: *** [scripts/Makefile.build:114: arch/arm64/kernel/asm-offsets.s] Error 1
make[2]: Target 'prepare' not remade because of errors.
make[1]: *** [Makefile:1286: prepare0] Error 2
make[1]: Target 'prepare' not remade because of errors.
make: *** [Makefile:226: __sub-make] Error 2
make: Target 'prepare' not remade because of errors.
vim +1109 arch/arm64/include/asm/kvm_host.h
1104
1105 static inline u64 idreg_read(struct kvm_arch *arch, u32 id)
1106 {
1107 u64 val;
1108
> 1109 mutex_lock(&arch->config_lock);
1110 val = _idreg_read(arch, id);
1111 mutex_unlock(&arch->config_lock);
1112
1113 return val;
1114 }
1115
Hi Jing,
kernel test robot noticed the following build errors:
[auto build test ERROR on 6a8f57ae2eb07ab39a6f0ccad60c760743051026]
url: https://github.com/intel-lab-lkp/linux/commits/Jing-Zhang/KVM-arm64-Move-CPU-ID-feature-registers-emulation-into-a-separate-file/20230425-074908
base: 6a8f57ae2eb07ab39a6f0ccad60c760743051026
patch link: https://lore.kernel.org/r/20230424234704.2571444-3-jingzhangos%40google.com
patch subject: [PATCH v7 2/6] KVM: arm64: Save ID registers' sanitized value per guest
config: arm64-randconfig-r034-20230425 (https://download.01.org/0day-ci/archive/20230425/202304251520.GoTtrhba-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 12.1.0
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
# https://github.com/intel-lab-lkp/linux/commit/cca571c516149d39bbaf1b5e916df617940458a6
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jing-Zhang/KVM-arm64-Move-CPU-ID-feature-registers-emulation-into-a-separate-file/20230425-074908
git checkout cca571c516149d39bbaf1b5e916df617940458a6
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm64 prepare
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304251520.GoTtrhba-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/linux/kvm_host.h:45,
from arch/arm64/kernel/asm-offsets.c:16:
arch/arm64/include/asm/kvm_host.h: In function 'idreg_read':
>> arch/arm64/include/asm/kvm_host.h:1109:25: error: 'struct kvm_arch' has no member named 'config_lock'
1109 | mutex_lock(&arch->config_lock);
| ^~
arch/arm64/include/asm/kvm_host.h:1111:27: error: 'struct kvm_arch' has no member named 'config_lock'
1111 | mutex_unlock(&arch->config_lock);
| ^~
arch/arm64/include/asm/kvm_host.h: In function 'idreg_write':
arch/arm64/include/asm/kvm_host.h:1118:25: error: 'struct kvm_arch' has no member named 'config_lock'
1118 | mutex_lock(&arch->config_lock);
| ^~
arch/arm64/include/asm/kvm_host.h:1120:27: error: 'struct kvm_arch' has no member named 'config_lock'
1120 | mutex_unlock(&arch->config_lock);
| ^~
make[2]: *** [scripts/Makefile.build:114: arch/arm64/kernel/asm-offsets.s] Error 1
make[2]: Target 'prepare' not remade because of errors.
make[1]: *** [Makefile:1286: prepare0] Error 2
make[1]: Target 'prepare' not remade because of errors.
make: *** [Makefile:226: __sub-make] Error 2
make: Target 'prepare' not remade because of errors.
vim +1109 arch/arm64/include/asm/kvm_host.h
1104
1105 static inline u64 idreg_read(struct kvm_arch *arch, u32 id)
1106 {
1107 u64 val;
1108
> 1109 mutex_lock(&arch->config_lock);
1110 val = _idreg_read(arch, id);
1111 mutex_unlock(&arch->config_lock);
1112
1113 return val;
1114 }
1115
Hi Jing, On Mon, Apr 24, 2023 at 11:47:00PM +0000, Jing Zhang wrote: > Introduce id_regs[] in kvm_arch as a storage of guest's ID registers, > and save ID registers' sanitized value in the array at KVM_CREATE_VM. > Use the saved ones when ID registers are read by the guest or > userspace (via KVM_GET_ONE_REG). > > No functional change intended. > > Co-developed-by: Reiji Watanabe <reijiw@google.com> > Signed-off-by: Reiji Watanabe <reijiw@google.com> > Signed-off-by: Jing Zhang <jingzhangos@google.com> > --- > arch/arm64/include/asm/kvm_host.h | 47 +++++++++++++++++++++++++++++ > arch/arm64/kvm/arm.c | 1 + > arch/arm64/kvm/id_regs.c | 49 +++++++++++++++++++++++++------ > arch/arm64/kvm/sys_regs.c | 2 +- > arch/arm64/kvm/sys_regs.h | 3 +- > 5 files changed, 90 insertions(+), 12 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index bcd774d74f34..2b1fe90a1790 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -177,6 +177,20 @@ struct kvm_smccc_features { > unsigned long vendor_hyp_bmap; > }; > > +/* > + * Emualted CPU ID registers per VM typo: emulated > + * (Op0, Op1, CRn, CRm, Op2) of the ID registers to be saved in it > + * is (3, 0, 0, crm, op2), where 1<=crm<8, 0<=op2<8. > + * > + * These emulated idregs are VM-wide, but accessed from the context of a vCPU. > + * Access to id regs are guarded by kvm_arch.config_lock. > + */ > +#define KVM_ARM_ID_REG_NUM 56 > +#define IDREG_IDX(id) (((sys_reg_CRm(id) - 1) << 3) | sys_reg_Op2(id)) > +struct kvm_idregs { > + u64 regs[KVM_ARM_ID_REG_NUM]; > +}; What is the purpose of declaring the register array as a separate structure? It has no meaning (nor use) outside of the context of a VM. I'd prefer the 'regs' array be embedded directly in kvm_arch, and just name it 'idregs'. You can move your macro definitions there as well to immediately precede the field. > typedef unsigned int pkvm_handle_t; > > struct kvm_protected_vm { > @@ -243,6 +257,9 @@ struct kvm_arch { > /* Hypercall features firmware registers' descriptor */ > struct kvm_smccc_features smccc_feat; > > + /* Emulated CPU ID registers */ > + struct kvm_idregs idregs; > + > /* > * For an untrusted host VM, 'pkvm.handle' is used to lookup > * the associated pKVM instance in the hypervisor. > @@ -1008,6 +1025,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, > long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, > struct kvm_arm_copy_mte_tags *copy_tags); > > +void kvm_arm_init_id_regs(struct kvm *kvm); > + > /* Guest/host FPSIMD coordination helpers */ > int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu); > void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu); > @@ -1073,4 +1092,32 @@ static inline void kvm_hyp_reserve(void) { } > void kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu); > bool kvm_arm_vcpu_stopped(struct kvm_vcpu *vcpu); > > +static inline u64 _idreg_read(struct kvm_arch *arch, u32 id) <bikeshed> Personally, I find passing 'kvm_arch' around to be a bit clunky. Almost all functions in KVM take 'struct kvm' as an argument, even if it only accesses the data in 'kvm_arch'. So, I'd prefer if all these helpers took 'struct kvm *'. </bikeshed> > +{ > + return arch->idregs.regs[IDREG_IDX(id)]; > +} > + > +static inline void _idreg_write(struct kvm_arch *arch, u32 id, u64 val) > +{ > + arch->idregs.regs[IDREG_IDX(id)] = val; > +} > + > +static inline u64 idreg_read(struct kvm_arch *arch, u32 id) > +{ > + u64 val; > + > + mutex_lock(&arch->config_lock); > + val = _idreg_read(arch, id); > + mutex_unlock(&arch->config_lock); What exactly are we protecting against by taking the config_lock here? While I do believe there is value in serializing writers to the shared data, there isn't a need to serialize reads from the guest. What about implementing the following: - Acquire the config_lock for handling writes. Only allow the value to change if !kvm_vm_has_ran_once(). Otherwise, permit identical writes (useful for hotplug, I imagine) or return EBUSY if userspace tried to change something after running the VM. - Acquire the config_lock for handling reads *from userspace* - Handle reads from the guest with a plain old load, avoiding the need to acquire any locks. This has the benefit of avoiding lock contention for guest reads w/o requiring the use of atomic loads/stores (i.e. {READ,WRITE}_ONCE()) to protect said readers. > + return val; > +} > + > +static inline void idreg_write(struct kvm_arch *arch, u32 id, u64 val) > +{ > + mutex_lock(&arch->config_lock); > + _idreg_write(arch, id, val); > + mutex_unlock(&arch->config_lock); > +} > + > #endif /* __ARM64_KVM_HOST_H__ */ > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index 4b2e16e696a8..e34744c36406 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -153,6 +153,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > > set_default_spectre(kvm); > kvm_arm_init_hypercalls(kvm); > + kvm_arm_init_id_regs(kvm); > > /* > * Initialise the default PMUver before there is a chance to > diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c > index 96b4c43a5100..d2fba2fde01c 100644 > --- a/arch/arm64/kvm/id_regs.c > +++ b/arch/arm64/kvm/id_regs.c > @@ -52,16 +52,9 @@ static u8 pmuver_to_perfmon(u8 pmuver) > } > } > > -/* Read a sanitised cpufeature ID register by sys_reg_desc */ > -static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r) > +u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > { > - u32 id = reg_to_encoding(r); > - u64 val; > - > - if (sysreg_visible_as_raz(vcpu, r)) > - return 0; > - > - val = read_sanitised_ftr_reg(id); > + u64 val = idreg_read(&vcpu->kvm->arch, id); > > switch (id) { > case SYS_ID_AA64PFR0_EL1: > @@ -126,6 +119,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r > return val; > } > > +static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r) > +{ > + if (sysreg_visible_as_raz(vcpu, r)) > + return 0; > + > + return kvm_arm_read_id_reg(vcpu, reg_to_encoding(r)); > +} > + > /* cpufeature ID register access trap handlers */ > > static bool access_id_reg(struct kvm_vcpu *vcpu, > @@ -458,3 +459,33 @@ int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params) > > return 1; > } > + > +/* > + * Set the guest's ID registers that are defined in id_reg_descs[] > + * with ID_SANITISED() to the host's sanitized value. > + */ > +void kvm_arm_init_id_regs(struct kvm *kvm) > +{ > + int i; > + u32 id; > + u64 val; nit: use reverse christmas/fir tree ordering for locals. > + for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) { > + id = reg_to_encoding(&id_reg_descs[i]); > + if (WARN_ON_ONCE(!is_id_reg(id))) > + /* Shouldn't happen */ > + continue; I'll make the suggestion once more. Please do not implement these sort of sanity checks on static data structures at the point userspace has gotten involved. Sanity checking on id_reg_descs[] should happen at the time KVM is initialized. If anything is wrong at that point we should return an error and outright refuse to run KVM.
Hi Oliver, On Tue, Apr 25, 2023 at 12:52 AM Oliver Upton <oliver.upton@linux.dev> wrote: > > Hi Jing, > > On Mon, Apr 24, 2023 at 11:47:00PM +0000, Jing Zhang wrote: > > Introduce id_regs[] in kvm_arch as a storage of guest's ID registers, > > and save ID registers' sanitized value in the array at KVM_CREATE_VM. > > Use the saved ones when ID registers are read by the guest or > > userspace (via KVM_GET_ONE_REG). > > > > No functional change intended. > > > > Co-developed-by: Reiji Watanabe <reijiw@google.com> > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > Signed-off-by: Jing Zhang <jingzhangos@google.com> > > --- > > arch/arm64/include/asm/kvm_host.h | 47 +++++++++++++++++++++++++++++ > > arch/arm64/kvm/arm.c | 1 + > > arch/arm64/kvm/id_regs.c | 49 +++++++++++++++++++++++++------ > > arch/arm64/kvm/sys_regs.c | 2 +- > > arch/arm64/kvm/sys_regs.h | 3 +- > > 5 files changed, 90 insertions(+), 12 deletions(-) > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index bcd774d74f34..2b1fe90a1790 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -177,6 +177,20 @@ struct kvm_smccc_features { > > unsigned long vendor_hyp_bmap; > > }; > > > > +/* > > + * Emualted CPU ID registers per VM > > typo: emulated Will fix it. > > > + * (Op0, Op1, CRn, CRm, Op2) of the ID registers to be saved in it > > + * is (3, 0, 0, crm, op2), where 1<=crm<8, 0<=op2<8. > > + * > > + * These emulated idregs are VM-wide, but accessed from the context of a vCPU. > > + * Access to id regs are guarded by kvm_arch.config_lock. > > + */ > > +#define KVM_ARM_ID_REG_NUM 56 > > +#define IDREG_IDX(id) (((sys_reg_CRm(id) - 1) << 3) | sys_reg_Op2(id)) > > +struct kvm_idregs { > > + u64 regs[KVM_ARM_ID_REG_NUM]; > > +}; > > What is the purpose of declaring the register array as a separate > structure? It has no meaning (nor use) outside of the context of a VM. > > I'd prefer the 'regs' array be embedded directly in kvm_arch, and just > name it 'idregs'. You can move your macro definitions there as well to > immediately precede the field. > It was declared as you suggested in the older series. The separate structure was suggested by Marc. > > typedef unsigned int pkvm_handle_t; > > > > struct kvm_protected_vm { > > @@ -243,6 +257,9 @@ struct kvm_arch { > > /* Hypercall features firmware registers' descriptor */ > > struct kvm_smccc_features smccc_feat; > > > > + /* Emulated CPU ID registers */ > > + struct kvm_idregs idregs; > > + > > /* > > * For an untrusted host VM, 'pkvm.handle' is used to lookup > > * the associated pKVM instance in the hypervisor. > > @@ -1008,6 +1025,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, > > long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, > > struct kvm_arm_copy_mte_tags *copy_tags); > > > > +void kvm_arm_init_id_regs(struct kvm *kvm); > > + > > /* Guest/host FPSIMD coordination helpers */ > > int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu); > > void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu); > > @@ -1073,4 +1092,32 @@ static inline void kvm_hyp_reserve(void) { } > > void kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu); > > bool kvm_arm_vcpu_stopped(struct kvm_vcpu *vcpu); > > > > +static inline u64 _idreg_read(struct kvm_arch *arch, u32 id) > > <bikeshed> > > Personally, I find passing 'kvm_arch' around to be a bit clunky. Almost > all functions in KVM take 'struct kvm' as an argument, even if it only > accesses the data in 'kvm_arch'. > > So, I'd prefer if all these helpers took 'struct kvm *'. > > </bikeshed> > There is a reason why 'struct kvm' was not used. Usually arch dependent headers don't include <linux/kvm_host.h> which declares the 'struct kvm'. But here we need to reference the fields in 'struct kvm', we need to include <linux/kvm_host.h>. But <linux/kvm_host.h> is dependent on arch dependent structures defined in <asm/kvm_host.h>. To use 'struct kvm', one way is to include <linux/kvm_host.h> in the middle of <asm/kvm_host.h> (after those arch dependent structures but before those idregs read/write inlines). Or we can move these idregs read/write functions to id_regs.c as non-static functions, since they are not only used by id_regs.c. > > +{ > > + return arch->idregs.regs[IDREG_IDX(id)]; > > +} > > + > > +static inline void _idreg_write(struct kvm_arch *arch, u32 id, u64 val) > > +{ > > + arch->idregs.regs[IDREG_IDX(id)] = val; > > +} > > + > > +static inline u64 idreg_read(struct kvm_arch *arch, u32 id) > > +{ > > + u64 val; > > + > > + mutex_lock(&arch->config_lock); > > + val = _idreg_read(arch, id); > > + mutex_unlock(&arch->config_lock); > > What exactly are we protecting against by taking the config_lock here? The intention is to use the config_lock to protect the whole id_regs[] array, all idregs, not on the granularity of a single idreg. IIUC, there might be some dependencies between feature fields in different idregs, like the PMUVer and PerfMon. If there is no lock for reads from the guest, the guest may get inconsistent values for PMUVer and PerfMon. But since no changes are allowed when the VM is running, the reads from the guests don't have to use the lock as you suggested below. > > While I do believe there is value in serializing writers to the shared > data, there isn't a need to serialize reads from the guest. > > What about implementing the following: > > - Acquire the config_lock for handling writes. Only allow the value to > change if !kvm_vm_has_ran_once(). Otherwise, permit identical writes > (useful for hotplug, I imagine) or return EBUSY if userspace tried to > change something after running the VM. Sure, will add this check for writes during VM running. > > - Acquire the config_lock for handling reads *from userspace* > > - Handle reads from the guest with a plain old load, avoiding the need > to acquire any locks. Sure, will use this since as long as VM is running, the userspace should not change the values of idregs. > > This has the benefit of avoiding lock contention for guest reads w/o > requiring the use of atomic loads/stores (i.e. {READ,WRITE}_ONCE()) to > protect said readers. > > > + return val; > > +} > > + > > +static inline void idreg_write(struct kvm_arch *arch, u32 id, u64 val) > > +{ > > + mutex_lock(&arch->config_lock); > > + _idreg_write(arch, id, val); > > + mutex_unlock(&arch->config_lock); > > +} > > + > > #endif /* __ARM64_KVM_HOST_H__ */ > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > > index 4b2e16e696a8..e34744c36406 100644 > > --- a/arch/arm64/kvm/arm.c > > +++ b/arch/arm64/kvm/arm.c > > @@ -153,6 +153,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > > > > set_default_spectre(kvm); > > kvm_arm_init_hypercalls(kvm); > > + kvm_arm_init_id_regs(kvm); > > > > /* > > * Initialise the default PMUver before there is a chance to > > diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c > > index 96b4c43a5100..d2fba2fde01c 100644 > > --- a/arch/arm64/kvm/id_regs.c > > +++ b/arch/arm64/kvm/id_regs.c > > @@ -52,16 +52,9 @@ static u8 pmuver_to_perfmon(u8 pmuver) > > } > > } > > > > -/* Read a sanitised cpufeature ID register by sys_reg_desc */ > > -static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r) > > +u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > > { > > - u32 id = reg_to_encoding(r); > > - u64 val; > > - > > - if (sysreg_visible_as_raz(vcpu, r)) > > - return 0; > > - > > - val = read_sanitised_ftr_reg(id); > > + u64 val = idreg_read(&vcpu->kvm->arch, id); > > > > switch (id) { > > case SYS_ID_AA64PFR0_EL1: > > @@ -126,6 +119,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r > > return val; > > } > > > > +static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r) > > +{ > > + if (sysreg_visible_as_raz(vcpu, r)) > > + return 0; > > + > > + return kvm_arm_read_id_reg(vcpu, reg_to_encoding(r)); > > +} > > + > > /* cpufeature ID register access trap handlers */ > > > > static bool access_id_reg(struct kvm_vcpu *vcpu, > > @@ -458,3 +459,33 @@ int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params) > > > > return 1; > > } > > + > > +/* > > + * Set the guest's ID registers that are defined in id_reg_descs[] > > + * with ID_SANITISED() to the host's sanitized value. > > + */ > > +void kvm_arm_init_id_regs(struct kvm *kvm) > > +{ > > + int i; > > + u32 id; > > + u64 val; > > nit: use reverse christmas/fir tree ordering for locals. > Will do. > > + for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) { > > + id = reg_to_encoding(&id_reg_descs[i]); > > + if (WARN_ON_ONCE(!is_id_reg(id))) > > + /* Shouldn't happen */ > > + continue; > > I'll make the suggestion once more. > > Please do not implement these sort of sanity checks on static data > structures at the point userspace has gotten involved. Sanity checking > on id_reg_descs[] should happen at the time KVM is initialized. If > anything is wrong at that point we should return an error and outright > refuse to run KVM. Sure, will move the check to kvm_sys_reg_table_init. > > -- > Thanks, > Oliver Thanks, Jing
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index bcd774d74f34..2b1fe90a1790 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -177,6 +177,20 @@ struct kvm_smccc_features { unsigned long vendor_hyp_bmap; }; +/* + * Emualted CPU ID registers per VM + * (Op0, Op1, CRn, CRm, Op2) of the ID registers to be saved in it + * is (3, 0, 0, crm, op2), where 1<=crm<8, 0<=op2<8. + * + * These emulated idregs are VM-wide, but accessed from the context of a vCPU. + * Access to id regs are guarded by kvm_arch.config_lock. + */ +#define KVM_ARM_ID_REG_NUM 56 +#define IDREG_IDX(id) (((sys_reg_CRm(id) - 1) << 3) | sys_reg_Op2(id)) +struct kvm_idregs { + u64 regs[KVM_ARM_ID_REG_NUM]; +}; + typedef unsigned int pkvm_handle_t; struct kvm_protected_vm { @@ -243,6 +257,9 @@ struct kvm_arch { /* Hypercall features firmware registers' descriptor */ struct kvm_smccc_features smccc_feat; + /* Emulated CPU ID registers */ + struct kvm_idregs idregs; + /* * For an untrusted host VM, 'pkvm.handle' is used to lookup * the associated pKVM instance in the hypervisor. @@ -1008,6 +1025,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, struct kvm_arm_copy_mte_tags *copy_tags); +void kvm_arm_init_id_regs(struct kvm *kvm); + /* Guest/host FPSIMD coordination helpers */ int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu); void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu); @@ -1073,4 +1092,32 @@ static inline void kvm_hyp_reserve(void) { } void kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu); bool kvm_arm_vcpu_stopped(struct kvm_vcpu *vcpu); +static inline u64 _idreg_read(struct kvm_arch *arch, u32 id) +{ + return arch->idregs.regs[IDREG_IDX(id)]; +} + +static inline void _idreg_write(struct kvm_arch *arch, u32 id, u64 val) +{ + arch->idregs.regs[IDREG_IDX(id)] = val; +} + +static inline u64 idreg_read(struct kvm_arch *arch, u32 id) +{ + u64 val; + + mutex_lock(&arch->config_lock); + val = _idreg_read(arch, id); + mutex_unlock(&arch->config_lock); + + return val; +} + +static inline void idreg_write(struct kvm_arch *arch, u32 id, u64 val) +{ + mutex_lock(&arch->config_lock); + _idreg_write(arch, id, val); + mutex_unlock(&arch->config_lock); +} + #endif /* __ARM64_KVM_HOST_H__ */ diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 4b2e16e696a8..e34744c36406 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -153,6 +153,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) set_default_spectre(kvm); kvm_arm_init_hypercalls(kvm); + kvm_arm_init_id_regs(kvm); /* * Initialise the default PMUver before there is a chance to diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c index 96b4c43a5100..d2fba2fde01c 100644 --- a/arch/arm64/kvm/id_regs.c +++ b/arch/arm64/kvm/id_regs.c @@ -52,16 +52,9 @@ static u8 pmuver_to_perfmon(u8 pmuver) } } -/* Read a sanitised cpufeature ID register by sys_reg_desc */ -static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r) +u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id) { - u32 id = reg_to_encoding(r); - u64 val; - - if (sysreg_visible_as_raz(vcpu, r)) - return 0; - - val = read_sanitised_ftr_reg(id); + u64 val = idreg_read(&vcpu->kvm->arch, id); switch (id) { case SYS_ID_AA64PFR0_EL1: @@ -126,6 +119,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r return val; } +static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r) +{ + if (sysreg_visible_as_raz(vcpu, r)) + return 0; + + return kvm_arm_read_id_reg(vcpu, reg_to_encoding(r)); +} + /* cpufeature ID register access trap handlers */ static bool access_id_reg(struct kvm_vcpu *vcpu, @@ -458,3 +459,33 @@ int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params) return 1; } + +/* + * Set the guest's ID registers that are defined in id_reg_descs[] + * with ID_SANITISED() to the host's sanitized value. + */ +void kvm_arm_init_id_regs(struct kvm *kvm) +{ + int i; + u32 id; + u64 val; + + for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) { + id = reg_to_encoding(&id_reg_descs[i]); + if (WARN_ON_ONCE(!is_id_reg(id))) + /* Shouldn't happen */ + continue; + + /* + * Some hidden ID registers which are not in arm64_ftr_regs[] + * would cause warnings from read_sanitised_ftr_reg(). + * Skip those ID registers to avoid the warnings. + */ + if (id_reg_descs[i].visibility == raz_visibility) + /* Hidden or reserved ID register */ + continue; + + val = read_sanitised_ftr_reg(id); + idreg_write(&kvm->arch, id, val); + } +} diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 8dba10696dac..6370a0db9d26 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -364,7 +364,7 @@ static bool trap_loregion(struct kvm_vcpu *vcpu, struct sys_reg_params *p, const struct sys_reg_desc *r) { - u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1); + u64 val = kvm_arm_read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1); u32 sr = reg_to_encoding(r); if (!(val & (0xfUL << ID_AA64MMFR1_EL1_LO_SHIFT))) { diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h index 7ce546a8be60..e88fd77309b2 100644 --- a/arch/arm64/kvm/sys_regs.h +++ b/arch/arm64/kvm/sys_regs.h @@ -237,6 +237,7 @@ bool write_to_read_only(struct kvm_vcpu *vcpu, struct sys_reg_params *params, const struct sys_reg_desc *r); unsigned int raz_visibility(const struct kvm_vcpu *vcpu, const struct sys_reg_desc *r); int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params); +u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id); #define AA32(_x) .aarch32_map = AA32_##_x #define Op0(_x) .Op0 = _x @@ -251,6 +252,4 @@ int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params); CRn(sys_reg_CRn(reg)), CRm(sys_reg_CRm(reg)), \ Op2(sys_reg_Op2(reg)) -#define KVM_ARM_ID_REG_NUM 56 - #endif /* __ARM64_KVM_SYS_REGS_LOCAL_H__ */