| Message ID | 20170808232726.118570-1-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09.08.2017 01:27, Jim Mattson wrote: > Host-initiated writes to the IA32_APIC_BASE MSR do not have to follow > local APIC state transition constraints, but the value written must be > valid. > > Signed-off-by: Jim Mattson <jmattson@google.com> > --- > arch/x86/kvm/x86.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index d734aa8c5b4f..fb786d9feef1 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -313,10 +313,11 @@ int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > u64 reserved_bits = ((~0ULL) << cpuid_maxphyaddr(vcpu)) | > 0x2ff | (guest_cpuid_has_x2apic(vcpu) ? 0 : X2APIC_ENABLE); > > + if ((msr_info->data & reserved_bits) != 0 || > + new_state == X2APIC_ENABLE) I'd drop the != 0 here and fit it into one line. > + return 1; > if (!msr_info->host_initiated && > - ((msr_info->data & reserved_bits) != 0 || > - new_state == X2APIC_ENABLE || > - (new_state == MSR_IA32_APICBASE_ENABLE && > + ((new_state == MSR_IA32_APICBASE_ENABLE && > old_state == (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE)) || > (new_state == (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE) && > old_state == 0))) > @@ -7444,7 +7445,8 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, > kvm_x86_ops->set_efer(vcpu, sregs->efer); > apic_base_msr.data = sregs->apic_base; > apic_base_msr.host_initiated = true; > - kvm_set_apic_base(vcpu, &apic_base_msr); > + if (kvm_set_apic_base(vcpu, &apic_base_msr)) > + return -EINVAL; I assume we don't have to document this new behavior. > > mmu_reset_needed |= kvm_read_cr0(vcpu) != sregs->cr0; > kvm_x86_ops->set_cr0(vcpu, sregs->cr0); > From what I can tell, this looks good to me.
The only thing that makes me unhappy about this is that the KVM_SET_SREGS ioctl may modify some VCPU state before returning -EINVAL. I could hoist the call to kvm_set_apic_base, but that only works for one mutator. If this doesn't bother anyone else, I'll just leave it where it is. On Wed, Aug 9, 2017 at 2:13 AM, David Hildenbrand <david@redhat.com> wrote: > On 09.08.2017 01:27, Jim Mattson wrote: >> Host-initiated writes to the IA32_APIC_BASE MSR do not have to follow >> local APIC state transition constraints, but the value written must be >> valid. >> >> Signed-off-by: Jim Mattson <jmattson@google.com> >> --- >> arch/x86/kvm/x86.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index d734aa8c5b4f..fb786d9feef1 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -313,10 +313,11 @@ int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info) >> u64 reserved_bits = ((~0ULL) << cpuid_maxphyaddr(vcpu)) | >> 0x2ff | (guest_cpuid_has_x2apic(vcpu) ? 0 : X2APIC_ENABLE); >> >> + if ((msr_info->data & reserved_bits) != 0 || >> + new_state == X2APIC_ENABLE) > > I'd drop the != 0 here and fit it into one line. > >> + return 1; >> if (!msr_info->host_initiated && >> - ((msr_info->data & reserved_bits) != 0 || >> - new_state == X2APIC_ENABLE || >> - (new_state == MSR_IA32_APICBASE_ENABLE && >> + ((new_state == MSR_IA32_APICBASE_ENABLE && >> old_state == (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE)) || >> (new_state == (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE) && >> old_state == 0))) >> @@ -7444,7 +7445,8 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, >> kvm_x86_ops->set_efer(vcpu, sregs->efer); >> apic_base_msr.data = sregs->apic_base; >> apic_base_msr.host_initiated = true; >> - kvm_set_apic_base(vcpu, &apic_base_msr); >> + if (kvm_set_apic_base(vcpu, &apic_base_msr)) >> + return -EINVAL; > > I assume we don't have to document this new behavior. > >> >> mmu_reset_needed |= kvm_read_cr0(vcpu) != sregs->cr0; >> kvm_x86_ops->set_cr0(vcpu, sregs->cr0); >> > > From what I can tell, this looks good to me. > > -- > > Thanks, > > David
On 09.08.2017 17:14, Jim Mattson wrote: > The only thing that makes me unhappy about this is that the > KVM_SET_SREGS ioctl may modify some VCPU state before returning > -EINVAL. I could hoist the call to kvm_set_apic_base, but that only > works for one mutator. If this doesn't bother anyone else, I'll just > leave it where it is. Good point, but the question is if the caller is even able to recover from this failure? If we care, you might have to move kvm_set_apic_base() to the very top in kvm_arch_vcpu_ioctl_set_sregs. Or just do the check at that point. Guess Paolo knows the answer to our question, just as always :)
On 09/08/2017 22:31, David Hildenbrand wrote: > On 09.08.2017 17:14, Jim Mattson wrote: >> The only thing that makes me unhappy about this is that the >> KVM_SET_SREGS ioctl may modify some VCPU state before returning >> -EINVAL. I could hoist the call to kvm_set_apic_base, but that only >> works for one mutator. If this doesn't bother anyone else, I'll just >> leave it where it is. > > Good point, but the question is if the caller is even able to recover > from this failure? Likely not, but being cleaner is usually better... > If we care, you might have to move kvm_set_apic_base() to the very top > in kvm_arch_vcpu_ioctl_set_sregs. Or just do the check at that point. > > Guess Paolo knows the answer to our question, just as always :) Not sure I do, but I am (though only slightly) worried about not doing the kvm_mmu_reset_context if EFER as changed and also about missing update_cr8_intercept. Moving kvm_set_apic_base early is harmless, so why not move that to the beginning. Paolo
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index d734aa8c5b4f..fb786d9feef1 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -313,10 +313,11 @@ int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info) u64 reserved_bits = ((~0ULL) << cpuid_maxphyaddr(vcpu)) | 0x2ff | (guest_cpuid_has_x2apic(vcpu) ? 0 : X2APIC_ENABLE); + if ((msr_info->data & reserved_bits) != 0 || + new_state == X2APIC_ENABLE) + return 1; if (!msr_info->host_initiated && - ((msr_info->data & reserved_bits) != 0 || - new_state == X2APIC_ENABLE || - (new_state == MSR_IA32_APICBASE_ENABLE && + ((new_state == MSR_IA32_APICBASE_ENABLE && old_state == (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE)) || (new_state == (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE) && old_state == 0))) @@ -7444,7 +7445,8 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, kvm_x86_ops->set_efer(vcpu, sregs->efer); apic_base_msr.data = sregs->apic_base; apic_base_msr.host_initiated = true; - kvm_set_apic_base(vcpu, &apic_base_msr); + if (kvm_set_apic_base(vcpu, &apic_base_msr)) + return -EINVAL; mmu_reset_needed |= kvm_read_cr0(vcpu) != sregs->cr0; kvm_x86_ops->set_cr0(vcpu, sregs->cr0);
Host-initiated writes to the IA32_APIC_BASE MSR do not have to follow local APIC state transition constraints, but the value written must be valid. Signed-off-by: Jim Mattson <jmattson@google.com> --- arch/x86/kvm/x86.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)