On Tue, Sep 09, 2025, Chao Gao wrote:
> +static int kvm_set_one_msr(struct kvm_vcpu *vcpu, u32 msr, u64 __user *value)
> +{
> + u64 val;
> +
> + if (get_user(val, value))
> + return -EFAULT;
> +
> + return do_set_msr(vcpu, msr, &val);
This needs to explicitly return -EINVAL on failure, otherwise KVM will return
semi-arbitrary positive values to userspace.
> +}
> +
> #ifdef CONFIG_X86_64
> struct pvclock_clock {
> int vclock_mode;
> @@ -4737,6 +4762,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> case KVM_CAP_IRQFD_RESAMPLE:
> case KVM_CAP_MEMORY_FAULT_INFO:
> case KVM_CAP_X86_GUEST_MODE:
> + case KVM_CAP_ONE_REG:
We should add (partial) support for KVM_GET_REG_LIST, otherwise the ABI for
handling GUEST_SSP is effectively undefined. And because the ioctl is per-vCPU,
utilizing KVM_GET_REG_LIST gives us the opportunity to avoid the horrors we
created with KVM_GET_MSR_INDEX_LIST, where KVM enumerates MSRs that aren't fully
supported by the vCPU.
If we don't enumerate GUEST_SSP via KVM_GET_REG_LIST, then trying to do
KVM_{G,S}ET_ONE_REG will "unexpectedly" fail if the vCPU doesn't have SHSTK.
By enumerating GUEST_SSP in KVM_GET_REG_LIST _if and only if_ it's fully supported,
we'll have a much more explicit ABI than we do for MSRs. And if we don't do that,
we'd have to special case MSR_KVM_INTERNAL_GUEST_SSP in kvm_is_advertised_msr().
As for MSRs, that's where "partial" support comes in. For MSRs, I think the least
awful option is to keep using KVM_GET_MSR_INDEX_LIST for enumerating MSRs, and
document that any MSRs that can be accessed via KVM_{G,S}ET_MSRS can be accessed
via KVM_{G,S}ET_ONE_REG. That avoids having to bake in different behavior for
MSR vs. ONE_REG accesses (and avoids having to add a pile of code to precisely
enumerate support for per-vCPU MSRs).
> r = 1;
> break;
> case KVM_CAP_PRE_FAULT_MEMORY:
> @@ -5915,6 +5941,20 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
> }
> }
>
> +struct kvm_x86_reg_id {
> + __u32 index;
> + __u8 type;
> + __u8 rsvd1;
> + __u8 rsvd2:4;
> + __u8 size:4;
> + __u8 x86;
> +};
> +
> +static int kvm_translate_kvm_reg(struct kvm_x86_reg_id *reg)
> +{
> + return -EINVAL;
> +}
> +
> long kvm_arch_vcpu_ioctl(struct file *filp,
> unsigned int ioctl, unsigned long arg)
> {
> @@ -6031,6 +6071,44 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> srcu_read_unlock(&vcpu->kvm->srcu, idx);
> break;
> }
> + case KVM_GET_ONE_REG:
> + case KVM_SET_ONE_REG: {
> + struct kvm_x86_reg_id *id;
> + struct kvm_one_reg reg;
> + u64 __user *value;
> +
> + r = -EFAULT;
> + if (copy_from_user(®, argp, sizeof(reg)))
> + break;
> +
> + r = -EINVAL;
> + if ((reg.id & KVM_REG_ARCH_MASK) != KVM_REG_X86)
> + break;
> +
> + id = (struct kvm_x86_reg_id *)®.id;
> + if (id->rsvd1 || id->rsvd2)
> + break;
> +
> + if (id->type == KVM_X86_REG_TYPE_KVM) {
> + r = kvm_translate_kvm_reg(id);
> + if (r)
> + break;
> + }
> +
> + r = -EINVAL;
> + if (id->type != KVM_X86_REG_TYPE_MSR)
> + break;
> +
> + if ((reg.id & KVM_REG_SIZE_MASK) != KVM_REG_SIZE_U64)
> + break;
> +
> + value = u64_to_user_ptr(reg.addr);
> + if (ioctl == KVM_GET_ONE_REG)
> + r = kvm_get_one_msr(vcpu, id->index, value);
> + else
> + r = kvm_set_one_msr(vcpu, id->index, value);
> + break;
> + }
I think it makes sense to put this in a separate helper, if only so that the
error returns are more obvious.
> case KVM_TPR_ACCESS_REPORTING: {
> struct kvm_tpr_access_ctl tac;
>
> --
> 2.47.3
>
