* [PATCH 1/2] KVM: x86: Store the microcode version in struct kvm_arch @ 2017-11-26 16:41 Filippo Sironi 2017-11-26 16:41 ` [PATCH 2/2] KVM: x86: Allow userspace to define what's the microcode version Filippo Sironi 0 siblings, 1 reply; 11+ messages in thread From: Filippo Sironi @ 2017-11-26 16:41 UTC (permalink / raw) To: pbonzini, rkrcmar, kvm, linux-kernel; +Cc: Filippo Sironi ... and read it from there when emulating accesses to MSR_IA32_UCODE_REV. This is the first step to allow userspace to define what's the microcode version that the guest should see. Signed-off-by: Filippo Sironi <sironi@amazon.de> --- arch/x86/include/asm/kvm_host.h | 2 ++ arch/x86/kvm/x86.c | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 1bfb99770c34..84b20139f4f1 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -776,6 +776,8 @@ struct kvm_arch { struct mutex apic_map_lock; struct kvm_apic_map *apic_map; + u32 microcode_version; + unsigned int tss_addr; bool apic_access_page_done; diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 34c85aa2e2d1..925c3e29cad3 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -2447,7 +2447,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) msr_info->data = 0; break; case MSR_IA32_UCODE_REV: - msr_info->data = 0x100000000ULL; + msr_info->data = (u64)vcpu->kvm->arch.microcode_version << 32; break; case MSR_MTRRcap: case 0x200 ... 0x2ff: @@ -8121,6 +8121,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) if (type) return -EINVAL; + kvm->arch.microcode_version = 0x1; + INIT_HLIST_HEAD(&kvm->arch.mask_notifier_list); INIT_LIST_HEAD(&kvm->arch.active_mmu_pages); INIT_LIST_HEAD(&kvm->arch.zapped_obsolete_pages); -- 2.7.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] KVM: x86: Allow userspace to define what's the microcode version 2017-11-26 16:41 [PATCH 1/2] KVM: x86: Store the microcode version in struct kvm_arch Filippo Sironi @ 2017-11-26 16:41 ` Filippo Sironi 2017-11-27 1:02 ` Wanpeng Li ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Filippo Sironi @ 2017-11-26 16:41 UTC (permalink / raw) To: pbonzini, rkrcmar, kvm, linux-kernel; +Cc: Filippo Sironi ... that the guest should see. Guest operating systems may check the microcode version to decide whether to disable certain features that are known to be buggy up to certain microcode versions. Address the issue by making the microcode version that the guest should see settable. The rationale for having userspace specifying the microcode version, rather than having the kernel picking it, is to ensure consistency for live-migrated instances; we don't want them to see a microcode version increase without a reset. Signed-off-by: Filippo Sironi <sironi@amazon.de> --- arch/x86/kvm/x86.c | 23 +++++++++++++++++++++++ include/uapi/linux/kvm.h | 3 +++ 2 files changed, 26 insertions(+) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 925c3e29cad3..741588f27ebc 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -4033,6 +4033,29 @@ long kvm_arch_vm_ioctl(struct file *filp, } u; switch (ioctl) { + case KVM_GET_MICROCODE_VERSION: { + r = -EFAULT; + if (copy_to_user(argp, + &kvm->arch.microcode_version, + sizeof(kvm->arch.microcode_version))) + goto out; + break; + } + case KVM_SET_MICROCODE_VERSION: { + u32 microcode_version; + + r = -EFAULT; + if (copy_from_user(µcode_version, + argp, + sizeof(microcode_version))) + goto out; + r = -EINVAL; + if (!microcode_version) + goto out; + kvm->arch.microcode_version = microcode_version; + r = 0; + break; + } case KVM_SET_TSS_ADDR: r = kvm_vm_ioctl_set_tss_addr(kvm, arg); break; diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index 282d7613fce8..e11887758e29 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -1192,6 +1192,9 @@ struct kvm_s390_ucas_mapping { #define KVM_S390_UCAS_UNMAP _IOW(KVMIO, 0x51, struct kvm_s390_ucas_mapping) #define KVM_S390_VCPU_FAULT _IOW(KVMIO, 0x52, unsigned long) +#define KVM_GET_MICROCODE_VERSION _IOR(KVMIO, 0x5e, __u32) +#define KVM_SET_MICROCODE_VERSION _IOW(KVMIO, 0x5f, __u32) + /* Device model IOC */ #define KVM_CREATE_IRQCHIP _IO(KVMIO, 0x60) #define KVM_IRQ_LINE _IOW(KVMIO, 0x61, struct kvm_irq_level) -- 2.7.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] KVM: x86: Allow userspace to define what's the microcode version 2017-11-26 16:41 ` [PATCH 2/2] KVM: x86: Allow userspace to define what's the microcode version Filippo Sironi @ 2017-11-27 1:02 ` Wanpeng Li 2017-12-09 7:21 ` Sironi, Filippo 2017-11-27 10:40 ` Paolo Bonzini 2017-11-27 11:58 ` Paolo Bonzini 2 siblings, 1 reply; 11+ messages in thread From: Wanpeng Li @ 2017-11-27 1:02 UTC (permalink / raw) To: Filippo Sironi; +Cc: Paolo Bonzini, Radim Krcmar, kvm, linux-kernel 2017-11-27 0:41 GMT+08:00 Filippo Sironi <sironi@amazon.de>: > ... that the guest should see. > Guest operating systems may check the microcode version to decide whether > to disable certain features that are known to be buggy up to certain > microcode versions. Address the issue by making the microcode version > that the guest should see settable. > The rationale for having userspace specifying the microcode version, rather > than having the kernel picking it, is to ensure consistency for live-migrated > instances; we don't want them to see a microcode version increase without a > reset. Is there a scenario which needs to refresh the microcode in the guest instead of on the host? Regards, Wanpeng Li > > Signed-off-by: Filippo Sironi <sironi@amazon.de> > --- > arch/x86/kvm/x86.c | 23 +++++++++++++++++++++++ > include/uapi/linux/kvm.h | 3 +++ > 2 files changed, 26 insertions(+) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 925c3e29cad3..741588f27ebc 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -4033,6 +4033,29 @@ long kvm_arch_vm_ioctl(struct file *filp, > } u; > > switch (ioctl) { > + case KVM_GET_MICROCODE_VERSION: { > + r = -EFAULT; > + if (copy_to_user(argp, > + &kvm->arch.microcode_version, > + sizeof(kvm->arch.microcode_version))) > + goto out; > + break; > + } > + case KVM_SET_MICROCODE_VERSION: { > + u32 microcode_version; > + > + r = -EFAULT; > + if (copy_from_user(µcode_version, > + argp, > + sizeof(microcode_version))) > + goto out; > + r = -EINVAL; > + if (!microcode_version) > + goto out; > + kvm->arch.microcode_version = microcode_version; > + r = 0; > + break; > + } > case KVM_SET_TSS_ADDR: > r = kvm_vm_ioctl_set_tss_addr(kvm, arg); > break; > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > index 282d7613fce8..e11887758e29 100644 > --- a/include/uapi/linux/kvm.h > +++ b/include/uapi/linux/kvm.h > @@ -1192,6 +1192,9 @@ struct kvm_s390_ucas_mapping { > #define KVM_S390_UCAS_UNMAP _IOW(KVMIO, 0x51, struct kvm_s390_ucas_mapping) > #define KVM_S390_VCPU_FAULT _IOW(KVMIO, 0x52, unsigned long) > > +#define KVM_GET_MICROCODE_VERSION _IOR(KVMIO, 0x5e, __u32) > +#define KVM_SET_MICROCODE_VERSION _IOW(KVMIO, 0x5f, __u32) > + > /* Device model IOC */ > #define KVM_CREATE_IRQCHIP _IO(KVMIO, 0x60) > #define KVM_IRQ_LINE _IOW(KVMIO, 0x61, struct kvm_irq_level) > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] KVM: x86: Allow userspace to define what's the microcode version 2017-11-27 1:02 ` Wanpeng Li @ 2017-12-09 7:21 ` Sironi, Filippo 0 siblings, 0 replies; 11+ messages in thread From: Sironi, Filippo @ 2017-12-09 7:21 UTC (permalink / raw) To: Wanpeng Li; +Cc: Paolo Bonzini, Radim Krcmar, kvm, linux-kernel > On 26. Nov 2017, at 17:02, Wanpeng Li <kernellwp@gmail.com> wrote: > > 2017-11-27 0:41 GMT+08:00 Filippo Sironi <sironi@amazon.de>: >> ... that the guest should see. >> Guest operating systems may check the microcode version to decide whether >> to disable certain features that are known to be buggy up to certain >> microcode versions. Address the issue by making the microcode version >> that the guest should see settable. >> The rationale for having userspace specifying the microcode version, rather >> than having the kernel picking it, is to ensure consistency for live-migrated >> instances; we don't want them to see a microcode version increase without a >> reset. > > Is there a scenario which needs to refresh the microcode in the guest > instead of on the host? > > Regards, > Wanpeng Li Not that I can think of. Today, we're picking up the host microcode version when launching an instance and making sure that the same version is exposed for the life time of the instance (i.e., across migrations that don't result in a reset). Filippo >> >> Signed-off-by: Filippo Sironi <sironi@amazon.de> >> --- >> arch/x86/kvm/x86.c | 23 +++++++++++++++++++++++ >> include/uapi/linux/kvm.h | 3 +++ >> 2 files changed, 26 insertions(+) >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 925c3e29cad3..741588f27ebc 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -4033,6 +4033,29 @@ long kvm_arch_vm_ioctl(struct file *filp, >> } u; >> >> switch (ioctl) { >> + case KVM_GET_MICROCODE_VERSION: { >> + r = -EFAULT; >> + if (copy_to_user(argp, >> + &kvm->arch.microcode_version, >> + sizeof(kvm->arch.microcode_version))) >> + goto out; >> + break; >> + } >> + case KVM_SET_MICROCODE_VERSION: { >> + u32 microcode_version; >> + >> + r = -EFAULT; >> + if (copy_from_user(µcode_version, >> + argp, >> + sizeof(microcode_version))) >> + goto out; >> + r = -EINVAL; >> + if (!microcode_version) >> + goto out; >> + kvm->arch.microcode_version = microcode_version; >> + r = 0; >> + break; >> + } >> case KVM_SET_TSS_ADDR: >> r = kvm_vm_ioctl_set_tss_addr(kvm, arg); >> break; >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h >> index 282d7613fce8..e11887758e29 100644 >> --- a/include/uapi/linux/kvm.h >> +++ b/include/uapi/linux/kvm.h >> @@ -1192,6 +1192,9 @@ struct kvm_s390_ucas_mapping { >> #define KVM_S390_UCAS_UNMAP _IOW(KVMIO, 0x51, struct kvm_s390_ucas_mapping) >> #define KVM_S390_VCPU_FAULT _IOW(KVMIO, 0x52, unsigned long) >> >> +#define KVM_GET_MICROCODE_VERSION _IOR(KVMIO, 0x5e, __u32) >> +#define KVM_SET_MICROCODE_VERSION _IOW(KVMIO, 0x5f, __u32) >> + >> /* Device model IOC */ >> #define KVM_CREATE_IRQCHIP _IO(KVMIO, 0x60) >> #define KVM_IRQ_LINE _IOW(KVMIO, 0x61, struct kvm_irq_level) >> -- >> 2.7.4 >> > Amazon Development Center Germany GmbH Berlin - Dresden - Aachen main office: Krausenstr. 38, 10117 Berlin Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger Ust-ID: DE289237879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] KVM: x86: Allow userspace to define what's the microcode version 2017-11-26 16:41 ` [PATCH 2/2] KVM: x86: Allow userspace to define what's the microcode version Filippo Sironi 2017-11-27 1:02 ` Wanpeng Li @ 2017-11-27 10:40 ` Paolo Bonzini 2017-12-09 7:33 ` Sironi, Filippo 2017-11-27 11:58 ` Paolo Bonzini 2 siblings, 1 reply; 11+ messages in thread From: Paolo Bonzini @ 2017-11-27 10:40 UTC (permalink / raw) To: Filippo Sironi, rkrcmar, kvm, linux-kernel On 26/11/2017 17:41, Filippo Sironi wrote: > ... that the guest should see. > Guest operating systems may check the microcode version to decide whether > to disable certain features that are known to be buggy up to certain > microcode versions. Address the issue by making the microcode version > that the guest should see settable. What's the advantage of specifying the microcode version, rather than relying on userspace to drop the CPUID bit for the buggy feature? old guest(*) new guest hide in CPUID good good use ucode rev BAD good (*) old guest = doesn't know that the feature is buggy until a given ucode revision Thanks, Paolo > The rationale for having userspace specifying the microcode version, rather > than having the kernel picking it, is to ensure consistency for live-migrated > instances; we don't want them to see a microcode version increase without a > reset. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] KVM: x86: Allow userspace to define what's the microcode version 2017-11-27 10:40 ` Paolo Bonzini @ 2017-12-09 7:33 ` Sironi, Filippo 0 siblings, 0 replies; 11+ messages in thread From: Sironi, Filippo @ 2017-12-09 7:33 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Radim Krcmar, kvm, linux-kernel > On 27. Nov 2017, at 02:40, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 26/11/2017 17:41, Filippo Sironi wrote: >> ... that the guest should see. >> Guest operating systems may check the microcode version to decide whether >> to disable certain features that are known to be buggy up to certain >> microcode versions. Address the issue by making the microcode version >> that the guest should see settable. > > What's the advantage of specifying the microcode version, rather than > relying on userspace to drop the CPUID bit for the buggy feature? > > old guest(*) new guest > > hide in CPUID good good > > use ucode rev BAD good > > > (*) old guest = doesn't know that the feature is buggy until a given > ucode revision > > Thanks, > > Paolo On C5 and M5 instances, we're basically exposing the host CPUID with few exceptions. Linux (among the others) has checks to make sure that certain features aren't enabled on a certain family/model/stepping if the microcode version isn't greater than or equal to a known good version. apic_check_deadline_errata() in arch/x86/kernel/apic/apic.c is the most recent example in Linux that I know of (by now you've updated it ;) but when we got the original bug report that triggered this patch, this wasn't the case). By exposing the real microcode version, we're preventing buggy guests that don't check that they are running virtualized (i.e., they should trust the hypervisor) from disabling features that are effectively not buggy. Filippo >> The rationale for having userspace specifying the microcode version, rather >> than having the kernel picking it, is to ensure consistency for live-migrated >> instances; we don't want them to see a microcode version increase without a >> reset. Amazon Development Center Germany GmbH Berlin - Dresden - Aachen main office: Krausenstr. 38, 10117 Berlin Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger Ust-ID: DE289237879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] KVM: x86: Allow userspace to define what's the microcode version 2017-11-26 16:41 ` [PATCH 2/2] KVM: x86: Allow userspace to define what's the microcode version Filippo Sironi 2017-11-27 1:02 ` Wanpeng Li 2017-11-27 10:40 ` Paolo Bonzini @ 2017-11-27 11:58 ` Paolo Bonzini 2017-11-27 22:09 ` Steve Rutherford 2017-12-09 7:42 ` Sironi, Filippo 2 siblings, 2 replies; 11+ messages in thread From: Paolo Bonzini @ 2017-11-27 11:58 UTC (permalink / raw) To: Filippo Sironi, rkrcmar, kvm, linux-kernel On 26/11/2017 17:41, Filippo Sironi wrote: > ... that the guest should see. > Guest operating systems may check the microcode version to decide whether > to disable certain features that are known to be buggy up to certain > microcode versions. Address the issue by making the microcode version > that the guest should see settable. > The rationale for having userspace specifying the microcode version, rather > than having the kernel picking it, is to ensure consistency for live-migrated > instances; we don't want them to see a microcode version increase without a > reset. > > Signed-off-by: Filippo Sironi <sironi@amazon.de> > --- > arch/x86/kvm/x86.c | 23 +++++++++++++++++++++++ > include/uapi/linux/kvm.h | 3 +++ > 2 files changed, 26 insertions(+) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 925c3e29cad3..741588f27ebc 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -4033,6 +4033,29 @@ long kvm_arch_vm_ioctl(struct file *filp, > } u; > > switch (ioctl) { > + case KVM_GET_MICROCODE_VERSION: { > + r = -EFAULT; > + if (copy_to_user(argp, > + &kvm->arch.microcode_version, > + sizeof(kvm->arch.microcode_version))) > + goto out; > + break; > + } > + case KVM_SET_MICROCODE_VERSION: { > + u32 microcode_version; > + > + r = -EFAULT; > + if (copy_from_user(µcode_version, > + argp, > + sizeof(microcode_version))) > + goto out; > + r = -EINVAL; > + if (!microcode_version) > + goto out; > + kvm->arch.microcode_version = microcode_version; > + r = 0; > + break; > + } Also, there's no need to define new ioctls, instead you can just place it in the vcpu and use KVM_GET_MSR/KVM_SET_MSR. I'd agree that's slightly less polished, but it matches what we do already for e.g. nested VMX model specific registers. And it spares you for writing the documentation that you didn't include in this patch. :) Paolo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] KVM: x86: Allow userspace to define what's the microcode version 2017-11-27 11:58 ` Paolo Bonzini @ 2017-11-27 22:09 ` Steve Rutherford 2017-12-09 7:51 ` Sironi, Filippo 2017-12-09 7:42 ` Sironi, Filippo 1 sibling, 1 reply; 11+ messages in thread From: Steve Rutherford @ 2017-11-27 22:09 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Filippo Sironi, Radim Krčmář, KVM list, LKML On Mon, Nov 27, 2017 at 3:58 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 26/11/2017 17:41, Filippo Sironi wrote: >> ... that the guest should see. >> Guest operating systems may check the microcode version to decide whether >> to disable certain features that are known to be buggy up to certain >> microcode versions. Address the issue by making the microcode version >> that the guest should see settable. >> The rationale for having userspace specifying the microcode version, rather >> than having the kernel picking it, is to ensure consistency for live-migrated >> instances; we don't want them to see a microcode version increase without a >> reset. >> >> Signed-off-by: Filippo Sironi <sironi@amazon.de> >> --- >> arch/x86/kvm/x86.c | 23 +++++++++++++++++++++++ >> include/uapi/linux/kvm.h | 3 +++ >> 2 files changed, 26 insertions(+) >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 925c3e29cad3..741588f27ebc 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -4033,6 +4033,29 @@ long kvm_arch_vm_ioctl(struct file *filp, >> } u; >> >> switch (ioctl) { >> + case KVM_GET_MICROCODE_VERSION: { >> + r = -EFAULT; >> + if (copy_to_user(argp, >> + &kvm->arch.microcode_version, >> + sizeof(kvm->arch.microcode_version))) >> + goto out; >> + break; >> + } >> + case KVM_SET_MICROCODE_VERSION: { >> + u32 microcode_version; >> + >> + r = -EFAULT; >> + if (copy_from_user(µcode_version, >> + argp, >> + sizeof(microcode_version))) >> + goto out; >> + r = -EINVAL; >> + if (!microcode_version) >> + goto out; >> + kvm->arch.microcode_version = microcode_version; >> + r = 0; >> + break; >> + } > > Also, there's no need to define new ioctls, instead you can just place > it in the vcpu and use KVM_GET_MSR/KVM_SET_MSR. I'd agree that's > slightly less polished, but it matches what we do already for e.g. > nested VMX model specific registers. And it spares you for writing the > documentation that you didn't include in this patch. :) > > Paolo This feels good time to mention Peter Hornyack's old MSR KVM_EXIT patches. With something like them, there would be no need to push this into the kernel at all. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] KVM: x86: Allow userspace to define what's the microcode version 2017-11-27 22:09 ` Steve Rutherford @ 2017-12-09 7:51 ` Sironi, Filippo 0 siblings, 0 replies; 11+ messages in thread From: Sironi, Filippo @ 2017-12-09 7:51 UTC (permalink / raw) To: Steve Rutherford Cc: Paolo Bonzini, Radim Krčmář, KVM list, LKML > On 27. Nov 2017, at 14:09, Steve Rutherford <srutherford@google.com> wrote: > > On Mon, Nov 27, 2017 at 3:58 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> On 26/11/2017 17:41, Filippo Sironi wrote: >>> ... that the guest should see. >>> Guest operating systems may check the microcode version to decide whether >>> to disable certain features that are known to be buggy up to certain >>> microcode versions. Address the issue by making the microcode version >>> that the guest should see settable. >>> The rationale for having userspace specifying the microcode version, rather >>> than having the kernel picking it, is to ensure consistency for live-migrated >>> instances; we don't want them to see a microcode version increase without a >>> reset. >>> >>> Signed-off-by: Filippo Sironi <sironi@amazon.de> >>> --- >>> arch/x86/kvm/x86.c | 23 +++++++++++++++++++++++ >>> include/uapi/linux/kvm.h | 3 +++ >>> 2 files changed, 26 insertions(+) >>> >>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>> index 925c3e29cad3..741588f27ebc 100644 >>> --- a/arch/x86/kvm/x86.c >>> +++ b/arch/x86/kvm/x86.c >>> @@ -4033,6 +4033,29 @@ long kvm_arch_vm_ioctl(struct file *filp, >>> } u; >>> >>> switch (ioctl) { >>> + case KVM_GET_MICROCODE_VERSION: { >>> + r = -EFAULT; >>> + if (copy_to_user(argp, >>> + &kvm->arch.microcode_version, >>> + sizeof(kvm->arch.microcode_version))) >>> + goto out; >>> + break; >>> + } >>> + case KVM_SET_MICROCODE_VERSION: { >>> + u32 microcode_version; >>> + >>> + r = -EFAULT; >>> + if (copy_from_user(µcode_version, >>> + argp, >>> + sizeof(microcode_version))) >>> + goto out; >>> + r = -EINVAL; >>> + if (!microcode_version) >>> + goto out; >>> + kvm->arch.microcode_version = microcode_version; >>> + r = 0; >>> + break; >>> + } >> >> Also, there's no need to define new ioctls, instead you can just place >> it in the vcpu and use KVM_GET_MSR/KVM_SET_MSR. I'd agree that's >> slightly less polished, but it matches what we do already for e.g. >> nested VMX model specific registers. And it spares you for writing the >> documentation that you didn't include in this patch. :) >> >> Paolo > > This feels good time to mention Peter Hornyack's old MSR KVM_EXIT > patches. With something like them, there would be no need to push this > into the kernel at all. That's one of the solution we discussed internally (at Amazon) but we didn't pursue yet given the need to release a quick fix for customers. I was thinking about implementing a mechanism to selectively go back to userspace to emulate MSRs; something that's not limited to KVM unhandled MSRs but that instead could even override KVM's handling. Filippo Amazon Development Center Germany GmbH Berlin - Dresden - Aachen main office: Krausenstr. 38, 10117 Berlin Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger Ust-ID: DE289237879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] KVM: x86: Allow userspace to define what's the microcode version 2017-11-27 11:58 ` Paolo Bonzini 2017-11-27 22:09 ` Steve Rutherford @ 2017-12-09 7:42 ` Sironi, Filippo 2017-12-11 16:03 ` Paolo Bonzini 1 sibling, 1 reply; 11+ messages in thread From: Sironi, Filippo @ 2017-12-09 7:42 UTC (permalink / raw) To: Paolo Bonzini; +Cc: rkrcmar, kvm, linux-kernel > On 27. Nov 2017, at 03:58, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 26/11/2017 17:41, Filippo Sironi wrote: >> ... that the guest should see. >> Guest operating systems may check the microcode version to decide whether >> to disable certain features that are known to be buggy up to certain >> microcode versions. Address the issue by making the microcode version >> that the guest should see settable. >> The rationale for having userspace specifying the microcode version, rather >> than having the kernel picking it, is to ensure consistency for live-migrated >> instances; we don't want them to see a microcode version increase without a >> reset. >> >> Signed-off-by: Filippo Sironi <sironi@amazon.de> >> --- >> arch/x86/kvm/x86.c | 23 +++++++++++++++++++++++ >> include/uapi/linux/kvm.h | 3 +++ >> 2 files changed, 26 insertions(+) >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 925c3e29cad3..741588f27ebc 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -4033,6 +4033,29 @@ long kvm_arch_vm_ioctl(struct file *filp, >> } u; >> >> switch (ioctl) { >> + case KVM_GET_MICROCODE_VERSION: { >> + r = -EFAULT; >> + if (copy_to_user(argp, >> + &kvm->arch.microcode_version, >> + sizeof(kvm->arch.microcode_version))) >> + goto out; >> + break; >> + } >> + case KVM_SET_MICROCODE_VERSION: { >> + u32 microcode_version; >> + >> + r = -EFAULT; >> + if (copy_from_user(µcode_version, >> + argp, >> + sizeof(microcode_version))) >> + goto out; >> + r = -EINVAL; >> + if (!microcode_version) >> + goto out; >> + kvm->arch.microcode_version = microcode_version; >> + r = 0; >> + break; >> + } > > Also, there's no need to define new ioctls, instead you can just place > it in the vcpu and use KVM_GET_MSR/KVM_SET_MSR. I'd agree that's > slightly less polished, but it matches what we do already for e.g. > nested VMX model specific registers. And it spares you for writing the > documentation that you didn't include in this patch. :) > > Paolo I wanted to do the work once rather than doing it per vCPU but using KVM_{GET|SET}_MSR and extending the list of MSRs that userspace can save/restore is certainly doable. I'll look into that and post a v2. Filippo Amazon Development Center Germany GmbH Berlin - Dresden - Aachen main office: Krausenstr. 38, 10117 Berlin Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger Ust-ID: DE289237879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] KVM: x86: Allow userspace to define what's the microcode version 2017-12-09 7:42 ` Sironi, Filippo @ 2017-12-11 16:03 ` Paolo Bonzini 0 siblings, 0 replies; 11+ messages in thread From: Paolo Bonzini @ 2017-12-11 16:03 UTC (permalink / raw) To: Sironi, Filippo; +Cc: rkrcmar, kvm, linux-kernel On 09/12/2017 08:42, Sironi, Filippo wrote: > I wanted to do the work once rather than doing it per vCPU but using > KVM_{GET|SET}_MSR and extending the list of MSRs that userspace can > save/restore is certainly doable. > > I'll look into that and post a v2. Thanks! Paolo ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-12-11 16:03 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-26 16:41 [PATCH 1/2] KVM: x86: Store the microcode version in struct kvm_arch Filippo Sironi 2017-11-26 16:41 ` [PATCH 2/2] KVM: x86: Allow userspace to define what's the microcode version Filippo Sironi 2017-11-27 1:02 ` Wanpeng Li 2017-12-09 7:21 ` Sironi, Filippo 2017-11-27 10:40 ` Paolo Bonzini 2017-12-09 7:33 ` Sironi, Filippo 2017-11-27 11:58 ` Paolo Bonzini 2017-11-27 22:09 ` Steve Rutherford 2017-12-09 7:51 ` Sironi, Filippo 2017-12-09 7:42 ` Sironi, Filippo 2017-12-11 16:03 ` Paolo Bonzini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.