Hi Oliver,
On Tue, Apr 25, 2023 at 12:52 AM Oliver Upton <oliver.upton@xxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Reiji Watanabe <reijiw@xxxxxxxxxx>
> > Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
> > ---
> > 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
