* [PATCH v2 1/7] KVM: X86: Reset vcpu->arch.cpuid_nent to 0 if SET_CPUID fails
2020-06-23 11:58 [PATCH v2 0/7] Refactor handling flow of SET_CPUID* Xiaoyao Li
@ 2020-06-23 11:58 ` Xiaoyao Li
2020-06-23 18:20 ` Jim Mattson
2020-06-23 11:58 ` [PATCH v2 2/7] KVM: X86: Go on updating other CPUID leaves when leaf 1 is absent Xiaoyao Li
` (5 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Xiaoyao Li @ 2020-06-23 11:58 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov
Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, Xiaoyao Li
It needs to invalidate CPUID configruations if usersapce provides
illegal input.
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
arch/x86/kvm/cpuid.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 8a294f9747aa..1d13bad42bf9 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -207,6 +207,8 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
kvm_apic_set_version(vcpu);
kvm_x86_ops.cpuid_update(vcpu);
r = kvm_update_cpuid(vcpu);
+ if (r)
+ vcpu->arch.cpuid_nent = 0;
kvfree(cpuid_entries);
out:
@@ -230,6 +232,8 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
kvm_apic_set_version(vcpu);
kvm_x86_ops.cpuid_update(vcpu);
r = kvm_update_cpuid(vcpu);
+ if (r)
+ vcpu->arch.cpuid_nent = 0;
out:
return r;
}
--
2.18.2
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 1/7] KVM: X86: Reset vcpu->arch.cpuid_nent to 0 if SET_CPUID fails
2020-06-23 11:58 ` [PATCH v2 1/7] KVM: X86: Reset vcpu->arch.cpuid_nent to 0 if SET_CPUID fails Xiaoyao Li
@ 2020-06-23 18:20 ` Jim Mattson
2020-06-24 0:40 ` Xiaoyao Li
0 siblings, 1 reply; 17+ messages in thread
From: Jim Mattson @ 2020-06-23 18:20 UTC (permalink / raw)
To: Xiaoyao Li
Cc: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
Joerg Roedel, kvm list, LKML
On Tue, Jun 23, 2020 at 4:58 AM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>
> It needs to invalidate CPUID configruations if usersapce provides
Nits: configurations, userspace
> illegal input.
>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> arch/x86/kvm/cpuid.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 8a294f9747aa..1d13bad42bf9 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -207,6 +207,8 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
> kvm_apic_set_version(vcpu);
> kvm_x86_ops.cpuid_update(vcpu);
> r = kvm_update_cpuid(vcpu);
> + if (r)
> + vcpu->arch.cpuid_nent = 0;
>
> kvfree(cpuid_entries);
> out:
> @@ -230,6 +232,8 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
> kvm_apic_set_version(vcpu);
> kvm_x86_ops.cpuid_update(vcpu);
> r = kvm_update_cpuid(vcpu);
> + if (r)
> + vcpu->arch.cpuid_nent = 0;
> out:
> return r;
> }
> --
> 2.18.2
What if vcpu->arch.cpuid_nent was greater than 0 before the ioctl in question?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/7] KVM: X86: Reset vcpu->arch.cpuid_nent to 0 if SET_CPUID fails
2020-06-23 18:20 ` Jim Mattson
@ 2020-06-24 0:40 ` Xiaoyao Li
0 siblings, 0 replies; 17+ messages in thread
From: Xiaoyao Li @ 2020-06-24 0:40 UTC (permalink / raw)
To: Jim Mattson
Cc: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
Joerg Roedel, kvm list, LKML
On 6/24/2020 2:20 AM, Jim Mattson wrote:
> On Tue, Jun 23, 2020 at 4:58 AM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>>
>> It needs to invalidate CPUID configruations if usersapce provides
>
> Nits: configurations, userspace
oh, I'll fix it.
>> illegal input.
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>> arch/x86/kvm/cpuid.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 8a294f9747aa..1d13bad42bf9 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -207,6 +207,8 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>> kvm_apic_set_version(vcpu);
>> kvm_x86_ops.cpuid_update(vcpu);
>> r = kvm_update_cpuid(vcpu);
>> + if (r)
>> + vcpu->arch.cpuid_nent = 0;
>>
>> kvfree(cpuid_entries);
>> out:
>> @@ -230,6 +232,8 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
>> kvm_apic_set_version(vcpu);
>> kvm_x86_ops.cpuid_update(vcpu);
>> r = kvm_update_cpuid(vcpu);
>> + if (r)
>> + vcpu->arch.cpuid_nent = 0;
>> out:
>> return r;
>> }
>> --
>> 2.18.2
>
> What if vcpu->arch.cpuid_nent was greater than 0 before the ioctl in question?
>
Nice catch!
If considering it, then we have to restore the old CPUID configuration.
So how about making it simpler to just add one line of comment in API doc:
If KVM_SET_CPUID{2} fails, the old valid configuration is cleared as a
side effect.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 2/7] KVM: X86: Go on updating other CPUID leaves when leaf 1 is absent
2020-06-23 11:58 [PATCH v2 0/7] Refactor handling flow of SET_CPUID* Xiaoyao Li
2020-06-23 11:58 ` [PATCH v2 1/7] KVM: X86: Reset vcpu->arch.cpuid_nent to 0 if SET_CPUID fails Xiaoyao Li
@ 2020-06-23 11:58 ` Xiaoyao Li
2020-07-02 18:54 ` Sean Christopherson
2020-06-23 11:58 ` [PATCH v2 3/7] KVM: X86: Introduce kvm_check_cpuid() Xiaoyao Li
` (4 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Xiaoyao Li @ 2020-06-23 11:58 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov
Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, Xiaoyao Li
As handling of bits other leaf 1 added over time, kvm_update_cpuid()
should not return directly if leaf 1 is absent, but should go on
updateing other CPUID leaves.
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
arch/x86/kvm/cpuid.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 1d13bad42bf9..0164dac95ef5 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -60,22 +60,21 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
struct kvm_lapic *apic = vcpu->arch.apic;
best = kvm_find_cpuid_entry(vcpu, 1, 0);
- if (!best)
- return 0;
-
- /* Update OSXSAVE bit */
- if (boot_cpu_has(X86_FEATURE_XSAVE) && best->function == 0x1)
- cpuid_entry_change(best, X86_FEATURE_OSXSAVE,
+ if (best) {
+ /* Update OSXSAVE bit */
+ if (boot_cpu_has(X86_FEATURE_XSAVE))
+ cpuid_entry_change(best, X86_FEATURE_OSXSAVE,
kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE));
- cpuid_entry_change(best, X86_FEATURE_APIC,
+ cpuid_entry_change(best, X86_FEATURE_APIC,
vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE);
- if (apic) {
- if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
- apic->lapic_timer.timer_mode_mask = 3 << 17;
- else
- apic->lapic_timer.timer_mode_mask = 1 << 17;
+ if (apic) {
+ if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
+ apic->lapic_timer.timer_mode_mask = 3 << 17;
+ else
+ apic->lapic_timer.timer_mode_mask = 1 << 17;
+ }
}
best = kvm_find_cpuid_entry(vcpu, 7, 0);
--
2.18.2
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 2/7] KVM: X86: Go on updating other CPUID leaves when leaf 1 is absent
2020-06-23 11:58 ` [PATCH v2 2/7] KVM: X86: Go on updating other CPUID leaves when leaf 1 is absent Xiaoyao Li
@ 2020-07-02 18:54 ` Sean Christopherson
2020-07-02 19:02 ` Sean Christopherson
0 siblings, 1 reply; 17+ messages in thread
From: Sean Christopherson @ 2020-07-02 18:54 UTC (permalink / raw)
To: Xiaoyao Li
Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
On Tue, Jun 23, 2020 at 07:58:11PM +0800, Xiaoyao Li wrote:
> As handling of bits other leaf 1 added over time, kvm_update_cpuid()
> should not return directly if leaf 1 is absent, but should go on
> updateing other CPUID leaves.
>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
This should probably be marked for stable.
> ---
> arch/x86/kvm/cpuid.c | 23 +++++++++++------------
> 1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 1d13bad42bf9..0164dac95ef5 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -60,22 +60,21 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
> struct kvm_lapic *apic = vcpu->arch.apic;
>
> best = kvm_find_cpuid_entry(vcpu, 1, 0);
> - if (!best)
> - return 0;
Rather than wrap the existing code, what about throwing it in a separate
helper? That generates an easier to read diff and also has the nice
property of getting 'apic' out of the common code.
> -
> - /* Update OSXSAVE bit */
> - if (boot_cpu_has(X86_FEATURE_XSAVE) && best->function == 0x1)
> - cpuid_entry_change(best, X86_FEATURE_OSXSAVE,
> + if (best) {
> + /* Update OSXSAVE bit */
> + if (boot_cpu_has(X86_FEATURE_XSAVE))
> + cpuid_entry_change(best, X86_FEATURE_OSXSAVE,
> kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE));
>
> - cpuid_entry_change(best, X86_FEATURE_APIC,
> + cpuid_entry_change(best, X86_FEATURE_APIC,
> vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE);
>
> - if (apic) {
> - if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
> - apic->lapic_timer.timer_mode_mask = 3 << 17;
> - else
> - apic->lapic_timer.timer_mode_mask = 1 << 17;
> + if (apic) {
> + if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
> + apic->lapic_timer.timer_mode_mask = 3 << 17;
> + else
> + apic->lapic_timer.timer_mode_mask = 1 << 17;
> + }
> }
>
> best = kvm_find_cpuid_entry(vcpu, 7, 0);
> --
> 2.18.2
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 2/7] KVM: X86: Go on updating other CPUID leaves when leaf 1 is absent
2020-07-02 18:54 ` Sean Christopherson
@ 2020-07-02 19:02 ` Sean Christopherson
2020-07-02 22:28 ` Xiaoyao Li
0 siblings, 1 reply; 17+ messages in thread
From: Sean Christopherson @ 2020-07-02 19:02 UTC (permalink / raw)
To: Xiaoyao Li
Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
On Thu, Jul 02, 2020 at 11:54:03AM -0700, Sean Christopherson wrote:
> On Tue, Jun 23, 2020 at 07:58:11PM +0800, Xiaoyao Li wrote:
> > As handling of bits other leaf 1 added over time, kvm_update_cpuid()
> > should not return directly if leaf 1 is absent, but should go on
> > updateing other CPUID leaves.
> >
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>
> This should probably be marked for stable.
>
> > ---
> > arch/x86/kvm/cpuid.c | 23 +++++++++++------------
> > 1 file changed, 11 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index 1d13bad42bf9..0164dac95ef5 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -60,22 +60,21 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
> > struct kvm_lapic *apic = vcpu->arch.apic;
> >
> > best = kvm_find_cpuid_entry(vcpu, 1, 0);
> > - if (!best)
> > - return 0;
>
> Rather than wrap the existing code, what about throwing it in a separate
> helper? That generates an easier to read diff and also has the nice
> property of getting 'apic' out of the common code.
Hrm, that'd be overkill once the apic code is moved in a few patches.
What if you keep the cpuid updates wrapped (as in this patch), but then
do
if (best && apic) {
}
for the apic path? That'll minimize churn for code that is disappearing,
e.g. will make future git archaeologists happy :-).
> > -
> > - /* Update OSXSAVE bit */
> > - if (boot_cpu_has(X86_FEATURE_XSAVE) && best->function == 0x1)
> > - cpuid_entry_change(best, X86_FEATURE_OSXSAVE,
> > + if (best) {
> > + /* Update OSXSAVE bit */
> > + if (boot_cpu_has(X86_FEATURE_XSAVE))
> > + cpuid_entry_change(best, X86_FEATURE_OSXSAVE,
> > kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE));
> >
> > - cpuid_entry_change(best, X86_FEATURE_APIC,
> > + cpuid_entry_change(best, X86_FEATURE_APIC,
> > vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE);
> >
> > - if (apic) {
> > - if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
> > - apic->lapic_timer.timer_mode_mask = 3 << 17;
> > - else
> > - apic->lapic_timer.timer_mode_mask = 1 << 17;
> > + if (apic) {
> > + if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
> > + apic->lapic_timer.timer_mode_mask = 3 << 17;
> > + else
> > + apic->lapic_timer.timer_mode_mask = 1 << 17;
> > + }
> > }
> >
> > best = kvm_find_cpuid_entry(vcpu, 7, 0);
> > --
> > 2.18.2
> >
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 2/7] KVM: X86: Go on updating other CPUID leaves when leaf 1 is absent
2020-07-02 19:02 ` Sean Christopherson
@ 2020-07-02 22:28 ` Xiaoyao Li
0 siblings, 0 replies; 17+ messages in thread
From: Xiaoyao Li @ 2020-07-02 22:28 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
On 7/3/2020 3:02 AM, Sean Christopherson wrote:
> On Thu, Jul 02, 2020 at 11:54:03AM -0700, Sean Christopherson wrote:
>> On Tue, Jun 23, 2020 at 07:58:11PM +0800, Xiaoyao Li wrote:
>>> As handling of bits other leaf 1 added over time, kvm_update_cpuid()
>>> should not return directly if leaf 1 is absent, but should go on
>>> updateing other CPUID leaves.
>>>
>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>
>> This should probably be marked for stable.
>>
>>> ---
>>> arch/x86/kvm/cpuid.c | 23 +++++++++++------------
>>> 1 file changed, 11 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>>> index 1d13bad42bf9..0164dac95ef5 100644
>>> --- a/arch/x86/kvm/cpuid.c
>>> +++ b/arch/x86/kvm/cpuid.c
>>> @@ -60,22 +60,21 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
>>> struct kvm_lapic *apic = vcpu->arch.apic;
>>>
>>> best = kvm_find_cpuid_entry(vcpu, 1, 0);
>>> - if (!best)
>>> - return 0;
>>
>> Rather than wrap the existing code, what about throwing it in a separate
>> helper? That generates an easier to read diff and also has the nice
>> property of getting 'apic' out of the common code.
>
> Hrm, that'd be overkill once the apic code is moved in a few patches.
> What if you keep the cpuid updates wrapped (as in this patch), but then
> do
>
> if (best && apic) {
> }
>
> for the apic path? That'll minimize churn for code that is disappearing,
> e.g. will make future git archaeologists happy :-).
Sure. I'll do it in next version.
>>> -
>>> - /* Update OSXSAVE bit */
>>> - if (boot_cpu_has(X86_FEATURE_XSAVE) && best->function == 0x1)
>>> - cpuid_entry_change(best, X86_FEATURE_OSXSAVE,
>>> + if (best) {
>>> + /* Update OSXSAVE bit */
>>> + if (boot_cpu_has(X86_FEATURE_XSAVE))
>>> + cpuid_entry_change(best, X86_FEATURE_OSXSAVE,
>>> kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE));
>>>
>>> - cpuid_entry_change(best, X86_FEATURE_APIC,
>>> + cpuid_entry_change(best, X86_FEATURE_APIC,
>>> vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE);
>>>
>>> - if (apic) {
>>> - if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
>>> - apic->lapic_timer.timer_mode_mask = 3 << 17;
>>> - else
>>> - apic->lapic_timer.timer_mode_mask = 1 << 17;
>>> + if (apic) {
>>> + if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
>>> + apic->lapic_timer.timer_mode_mask = 3 << 17;
>>> + else
>>> + apic->lapic_timer.timer_mode_mask = 1 << 17;
>>> + }
>>> }
>>>
>>> best = kvm_find_cpuid_entry(vcpu, 7, 0);
>>> --
>>> 2.18.2
>>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 3/7] KVM: X86: Introduce kvm_check_cpuid()
2020-06-23 11:58 [PATCH v2 0/7] Refactor handling flow of SET_CPUID* Xiaoyao Li
2020-06-23 11:58 ` [PATCH v2 1/7] KVM: X86: Reset vcpu->arch.cpuid_nent to 0 if SET_CPUID fails Xiaoyao Li
2020-06-23 11:58 ` [PATCH v2 2/7] KVM: X86: Go on updating other CPUID leaves when leaf 1 is absent Xiaoyao Li
@ 2020-06-23 11:58 ` Xiaoyao Li
2020-06-23 11:58 ` [PATCH v2 4/7] KVM: X86: Split kvm_update_cpuid() Xiaoyao Li
` (3 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Xiaoyao Li @ 2020-06-23 11:58 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov
Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, Xiaoyao Li
Use kvm_check_cpuid() to validate if userspace provides legal cpuid
settings and call it before KVM updates CPUID.
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
Is the check of virutal address width really necessary?
KVM doesn't check other bits at all. I guess the policy is that KVM allows
illegal CPUID settings as long as it doesn't break host kernel. Userspace
takes the consequences itself if it sets bogus CPUID settings that breaks
its guest.
But why vaddr_bits is special? It seems illegal vaddr_bits won't break host
kernel.
---
arch/x86/kvm/cpuid.c | 54 ++++++++++++++++++++++++++++----------------
arch/x86/kvm/cpuid.h | 2 +-
2 files changed, 35 insertions(+), 21 deletions(-)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 0164dac95ef5..67e5c68fdb45 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -54,7 +54,26 @@ static u32 xstate_required_size(u64 xstate_bv, bool compacted)
#define F feature_bit
-int kvm_update_cpuid(struct kvm_vcpu *vcpu)
+static int kvm_check_cpuid(struct kvm_vcpu *vcpu)
+{
+ struct kvm_cpuid_entry2 *best;
+
+ /*
+ * The existing code assumes virtual address is 48-bit or 57-bit in the
+ * canonical address checks; exit if it is ever changed.
+ */
+ best = kvm_find_cpuid_entry(vcpu, 0x80000008, 0);
+ if (best) {
+ int vaddr_bits = (best->eax & 0xff00) >> 8;
+
+ if (vaddr_bits != 48 && vaddr_bits != 57 && vaddr_bits != 0)
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+void kvm_update_cpuid(struct kvm_vcpu *vcpu)
{
struct kvm_cpuid_entry2 *best;
struct kvm_lapic *apic = vcpu->arch.apic;
@@ -96,18 +115,6 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
cpuid_entry_has(best, X86_FEATURE_XSAVEC)))
best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
- /*
- * The existing code assumes virtual address is 48-bit or 57-bit in the
- * canonical address checks; exit if it is ever changed.
- */
- best = kvm_find_cpuid_entry(vcpu, 0x80000008, 0);
- if (best) {
- int vaddr_bits = (best->eax & 0xff00) >> 8;
-
- if (vaddr_bits != 48 && vaddr_bits != 57 && vaddr_bits != 0)
- return -EINVAL;
- }
-
best = kvm_find_cpuid_entry(vcpu, KVM_CPUID_FEATURES, 0);
if (kvm_hlt_in_guest(vcpu->kvm) && best &&
(best->eax & (1 << KVM_FEATURE_PV_UNHALT)))
@@ -127,7 +134,6 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
kvm_mmu_reset_context(vcpu);
kvm_pmu_refresh(vcpu);
- return 0;
}
static int is_efer_nx(void)
@@ -202,12 +208,16 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
vcpu->arch.cpuid_entries[i].padding[2] = 0;
}
vcpu->arch.cpuid_nent = cpuid->nent;
+ r = kvm_check_cpuid(vcpu);
+ if (r) {
+ vcpu->arch.cpuid_nent = 0;
+ goto out;
+ }
+
cpuid_fix_nx_cap(vcpu);
kvm_apic_set_version(vcpu);
kvm_x86_ops.cpuid_update(vcpu);
- r = kvm_update_cpuid(vcpu);
- if (r)
- vcpu->arch.cpuid_nent = 0;
+ kvm_update_cpuid(vcpu);
kvfree(cpuid_entries);
out:
@@ -228,11 +238,15 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
cpuid->nent * sizeof(struct kvm_cpuid_entry2)))
goto out;
vcpu->arch.cpuid_nent = cpuid->nent;
+ r = kvm_check_cpuid(vcpu);
+ if (r) {
+ vcpu->arch.cpuid_nent = 0;
+ goto out;
+ }
+
kvm_apic_set_version(vcpu);
kvm_x86_ops.cpuid_update(vcpu);
- r = kvm_update_cpuid(vcpu);
- if (r)
- vcpu->arch.cpuid_nent = 0;
+ kvm_update_cpuid(vcpu);
out:
return r;
}
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 05434cd9342f..f136de1debad 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -9,7 +9,7 @@
extern u32 kvm_cpu_caps[NCAPINTS] __read_mostly;
void kvm_set_cpu_caps(void);
-int kvm_update_cpuid(struct kvm_vcpu *vcpu);
+void kvm_update_cpuid(struct kvm_vcpu *vcpu);
struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
u32 function, u32 index);
int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
--
2.18.2
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 4/7] KVM: X86: Split kvm_update_cpuid()
2020-06-23 11:58 [PATCH v2 0/7] Refactor handling flow of SET_CPUID* Xiaoyao Li
` (2 preceding siblings ...)
2020-06-23 11:58 ` [PATCH v2 3/7] KVM: X86: Introduce kvm_check_cpuid() Xiaoyao Li
@ 2020-06-23 11:58 ` Xiaoyao Li
2020-06-23 11:58 ` [PATCH v2 5/7] KVM: X86: Rename cpuid_update() to update_vcpu_model() Xiaoyao Li
` (2 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Xiaoyao Li @ 2020-06-23 11:58 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov
Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, Xiaoyao Li
Split the part of updating vcpu model out of kvm_update_cpuid(), and put
it into a new kvm_update_vcpu_model(). So it's more clear that
kvm_update_cpuid() is to update guest CPUID settings, while
kvm_update_vcpu_model() is to update vcpu model (settings) based on the
updated CPUID settings.
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
arch/x86/kvm/cpuid.c | 38 ++++++++++++++++++++++++--------------
arch/x86/kvm/cpuid.h | 1 +
arch/x86/kvm/x86.c | 1 +
3 files changed, 26 insertions(+), 14 deletions(-)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 67e5c68fdb45..001f5a94880e 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -76,7 +76,6 @@ static int kvm_check_cpuid(struct kvm_vcpu *vcpu)
void kvm_update_cpuid(struct kvm_vcpu *vcpu)
{
struct kvm_cpuid_entry2 *best;
- struct kvm_lapic *apic = vcpu->arch.apic;
best = kvm_find_cpuid_entry(vcpu, 1, 0);
if (best) {
@@ -87,13 +86,6 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
cpuid_entry_change(best, X86_FEATURE_APIC,
vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE);
-
- if (apic) {
- if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
- apic->lapic_timer.timer_mode_mask = 3 << 17;
- else
- apic->lapic_timer.timer_mode_mask = 1 << 17;
- }
}
best = kvm_find_cpuid_entry(vcpu, 7, 0);
@@ -102,13 +94,8 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
kvm_read_cr4_bits(vcpu, X86_CR4_PKE));
best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
- if (!best) {
- vcpu->arch.guest_supported_xcr0 = 0;
- } else {
- vcpu->arch.guest_supported_xcr0 =
- (best->eax | ((u64)best->edx << 32)) & supported_xcr0;
+ if (best)
best->ebx = xstate_required_size(vcpu->arch.xcr0, false);
- }
best = kvm_find_cpuid_entry(vcpu, 0xD, 1);
if (best && (cpuid_entry_has(best, X86_FEATURE_XSAVES) ||
@@ -127,6 +114,27 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
vcpu->arch.ia32_misc_enable_msr &
MSR_IA32_MISC_ENABLE_MWAIT);
}
+}
+
+void kvm_update_vcpu_model(struct kvm_vcpu *vcpu)
+{
+ struct kvm_lapic *apic = vcpu->arch.apic;
+ struct kvm_cpuid_entry2 *best;
+
+ best = kvm_find_cpuid_entry(vcpu, 1, 0);
+ if (best && apic) {
+ if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
+ apic->lapic_timer.timer_mode_mask = 3 << 17;
+ else
+ apic->lapic_timer.timer_mode_mask = 1 << 17;
+ }
+
+ best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
+ if (!best)
+ vcpu->arch.guest_supported_xcr0 = 0;
+ else
+ vcpu->arch.guest_supported_xcr0 =
+ (best->eax | ((u64)best->edx << 32)) & supported_xcr0;
/* Note, maxphyaddr must be updated before tdp_level. */
vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
@@ -218,6 +226,7 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
kvm_apic_set_version(vcpu);
kvm_x86_ops.cpuid_update(vcpu);
kvm_update_cpuid(vcpu);
+ kvm_update_vcpu_model(vcpu);
kvfree(cpuid_entries);
out:
@@ -247,6 +256,7 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
kvm_apic_set_version(vcpu);
kvm_x86_ops.cpuid_update(vcpu);
kvm_update_cpuid(vcpu);
+ kvm_update_vcpu_model(vcpu);
out:
return r;
}
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index f136de1debad..45e3643e2fba 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -10,6 +10,7 @@ extern u32 kvm_cpu_caps[NCAPINTS] __read_mostly;
void kvm_set_cpu_caps(void);
void kvm_update_cpuid(struct kvm_vcpu *vcpu);
+void kvm_update_vcpu_model(struct kvm_vcpu *vcpu);
struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
u32 function, u32 index);
int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 00c88c2f34e4..4dee28bbc087 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8149,6 +8149,7 @@ static void enter_smm(struct kvm_vcpu *vcpu)
#endif
kvm_update_cpuid(vcpu);
+ kvm_update_vcpu_model(vcpu);
kvm_mmu_reset_context(vcpu);
}
--
2.18.2
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 5/7] KVM: X86: Rename cpuid_update() to update_vcpu_model()
2020-06-23 11:58 [PATCH v2 0/7] Refactor handling flow of SET_CPUID* Xiaoyao Li
` (3 preceding siblings ...)
2020-06-23 11:58 ` [PATCH v2 4/7] KVM: X86: Split kvm_update_cpuid() Xiaoyao Li
@ 2020-06-23 11:58 ` Xiaoyao Li
2020-06-23 11:58 ` [PATCH v2 6/7] KVM: X86: Move kvm_x86_ops.update_vcpu_model() into kvm_update_vcpu_model() Xiaoyao Li
2020-06-23 11:58 ` [PATCH v2 7/7] KVM: X86: Move kvm_apic_set_version() to kvm_update_vcpu_model() Xiaoyao Li
6 siblings, 0 replies; 17+ messages in thread
From: Xiaoyao Li @ 2020-06-23 11:58 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov
Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, Xiaoyao Li
The name of callback cpuid_update() is misleading that it's not about
updating CPUID settings of vcpu but updating the configurations of vcpu
based on the CPUIDs. So rename it to update_vcpu_model().
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
arch/x86/include/asm/kvm_host.h | 2 +-
arch/x86/kvm/cpuid.c | 4 ++--
arch/x86/kvm/svm/svm.c | 4 ++--
arch/x86/kvm/vmx/nested.c | 2 +-
arch/x86/kvm/vmx/vmx.c | 4 ++--
5 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index f852ee350beb..e8ae89eef199 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1083,7 +1083,7 @@ struct kvm_x86_ops {
void (*hardware_unsetup)(void);
bool (*cpu_has_accelerated_tpr)(void);
bool (*has_emulated_msr)(u32 index);
- void (*cpuid_update)(struct kvm_vcpu *vcpu);
+ void (*update_vcpu_model)(struct kvm_vcpu *vcpu);
unsigned int vm_size;
int (*vm_init)(struct kvm *kvm);
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 001f5a94880e..d2f93823f9fd 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -224,7 +224,7 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
cpuid_fix_nx_cap(vcpu);
kvm_apic_set_version(vcpu);
- kvm_x86_ops.cpuid_update(vcpu);
+ kvm_x86_ops.update_vcpu_model(vcpu);
kvm_update_cpuid(vcpu);
kvm_update_vcpu_model(vcpu);
@@ -254,7 +254,7 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
}
kvm_apic_set_version(vcpu);
- kvm_x86_ops.cpuid_update(vcpu);
+ kvm_x86_ops.update_vcpu_model(vcpu);
kvm_update_cpuid(vcpu);
kvm_update_vcpu_model(vcpu);
out:
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index c0da4dd78ac5..480d0354910a 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3551,7 +3551,7 @@ static u64 svm_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
return 0;
}
-static void svm_cpuid_update(struct kvm_vcpu *vcpu)
+static void svm_update_vcpu_model(struct kvm_vcpu *vcpu)
{
struct vcpu_svm *svm = to_svm(vcpu);
@@ -4051,7 +4051,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
.get_exit_info = svm_get_exit_info,
- .cpuid_update = svm_cpuid_update,
+ .update_vcpu_model = svm_update_vcpu_model,
.has_wbinvd_exit = svm_has_wbinvd_exit,
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index d1af20b050a8..86ba7cd49c97 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -6322,7 +6322,7 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps)
/*
* secondary cpu-based controls. Do not include those that
- * depend on CPUID bits, they are added later by vmx_cpuid_update.
+ * depend on CPUID bits, they are added later by vmx_update_vcpu_model.
*/
if (msrs->procbased_ctls_high & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS)
rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2,
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index b1a23ad986ff..147c696d6445 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7251,7 +7251,7 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)
vmx->pt_desc.ctl_bitmask &= ~(0xfULL << (32 + i * 4));
}
-static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
+static void vmx_update_vcpu_model(struct kvm_vcpu *vcpu)
{
struct vcpu_vmx *vmx = to_vmx(vcpu);
@@ -7945,7 +7945,7 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = {
.get_exit_info = vmx_get_exit_info,
- .cpuid_update = vmx_cpuid_update,
+ .update_vcpu_model = vmx_update_vcpu_model,
.has_wbinvd_exit = cpu_has_vmx_wbinvd_exit,
--
2.18.2
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 6/7] KVM: X86: Move kvm_x86_ops.update_vcpu_model() into kvm_update_vcpu_model()
2020-06-23 11:58 [PATCH v2 0/7] Refactor handling flow of SET_CPUID* Xiaoyao Li
` (4 preceding siblings ...)
2020-06-23 11:58 ` [PATCH v2 5/7] KVM: X86: Rename cpuid_update() to update_vcpu_model() Xiaoyao Li
@ 2020-06-23 11:58 ` Xiaoyao Li
2020-07-02 18:59 ` Sean Christopherson
2020-06-23 11:58 ` [PATCH v2 7/7] KVM: X86: Move kvm_apic_set_version() to kvm_update_vcpu_model() Xiaoyao Li
6 siblings, 1 reply; 17+ messages in thread
From: Xiaoyao Li @ 2020-06-23 11:58 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov
Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, Xiaoyao Li
kvm_x86_ops.update_vcpu_model() is used to update vmx/svm vcpu settings
based on updated CPUID settings. So it's supposed to be called after
CPUIDs are fully updated, i.e., kvm_update_cpuid().
Move it in kvm_update_vcpu_model().
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
---
arch/x86/kvm/cpuid.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index d2f93823f9fd..5decc2dd5448 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -121,6 +121,8 @@ void kvm_update_vcpu_model(struct kvm_vcpu *vcpu)
struct kvm_lapic *apic = vcpu->arch.apic;
struct kvm_cpuid_entry2 *best;
+ kvm_x86_ops.update_vcpu_model(vcpu);
+
best = kvm_find_cpuid_entry(vcpu, 1, 0);
if (best && apic) {
if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
@@ -136,6 +138,7 @@ void kvm_update_vcpu_model(struct kvm_vcpu *vcpu)
vcpu->arch.guest_supported_xcr0 =
(best->eax | ((u64)best->edx << 32)) & supported_xcr0;
+
/* Note, maxphyaddr must be updated before tdp_level. */
vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
vcpu->arch.tdp_level = kvm_x86_ops.get_tdp_level(vcpu);
@@ -224,7 +227,6 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
cpuid_fix_nx_cap(vcpu);
kvm_apic_set_version(vcpu);
- kvm_x86_ops.update_vcpu_model(vcpu);
kvm_update_cpuid(vcpu);
kvm_update_vcpu_model(vcpu);
@@ -254,7 +256,6 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
}
kvm_apic_set_version(vcpu);
- kvm_x86_ops.update_vcpu_model(vcpu);
kvm_update_cpuid(vcpu);
kvm_update_vcpu_model(vcpu);
out:
--
2.18.2
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 6/7] KVM: X86: Move kvm_x86_ops.update_vcpu_model() into kvm_update_vcpu_model()
2020-06-23 11:58 ` [PATCH v2 6/7] KVM: X86: Move kvm_x86_ops.update_vcpu_model() into kvm_update_vcpu_model() Xiaoyao Li
@ 2020-07-02 18:59 ` Sean Christopherson
2020-07-02 22:29 ` Xiaoyao Li
0 siblings, 1 reply; 17+ messages in thread
From: Sean Christopherson @ 2020-07-02 18:59 UTC (permalink / raw)
To: Xiaoyao Li
Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
On Tue, Jun 23, 2020 at 07:58:15PM +0800, Xiaoyao Li wrote:
> kvm_x86_ops.update_vcpu_model() is used to update vmx/svm vcpu settings
> based on updated CPUID settings. So it's supposed to be called after
> CPUIDs are fully updated, i.e., kvm_update_cpuid().
>
> Move it in kvm_update_vcpu_model().
The changelog needs to provide an in-depth analysis of VMX and SVM to prove
that there are no existing dependencies in the ordering. I've done the
analysis a few times over the past few years for a similar chage I carried
in my SGX code, but dropped that code a while back and haven't done the
analysis since. Anyways, it should be documented.
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> ---
> arch/x86/kvm/cpuid.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index d2f93823f9fd..5decc2dd5448 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -121,6 +121,8 @@ void kvm_update_vcpu_model(struct kvm_vcpu *vcpu)
> struct kvm_lapic *apic = vcpu->arch.apic;
> struct kvm_cpuid_entry2 *best;
>
> + kvm_x86_ops.update_vcpu_model(vcpu);
> +
> best = kvm_find_cpuid_entry(vcpu, 1, 0);
> if (best && apic) {
> if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
> @@ -136,6 +138,7 @@ void kvm_update_vcpu_model(struct kvm_vcpu *vcpu)
> vcpu->arch.guest_supported_xcr0 =
> (best->eax | ((u64)best->edx << 32)) & supported_xcr0;
>
> +
Spurious whitespace.
> /* Note, maxphyaddr must be updated before tdp_level. */
> vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
> vcpu->arch.tdp_level = kvm_x86_ops.get_tdp_level(vcpu);
> @@ -224,7 +227,6 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>
> cpuid_fix_nx_cap(vcpu);
> kvm_apic_set_version(vcpu);
> - kvm_x86_ops.update_vcpu_model(vcpu);
> kvm_update_cpuid(vcpu);
> kvm_update_vcpu_model(vcpu);
>
> @@ -254,7 +256,6 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
> }
>
> kvm_apic_set_version(vcpu);
> - kvm_x86_ops.update_vcpu_model(vcpu);
> kvm_update_cpuid(vcpu);
> kvm_update_vcpu_model(vcpu);
> out:
> --
> 2.18.2
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 6/7] KVM: X86: Move kvm_x86_ops.update_vcpu_model() into kvm_update_vcpu_model()
2020-07-02 18:59 ` Sean Christopherson
@ 2020-07-02 22:29 ` Xiaoyao Li
0 siblings, 0 replies; 17+ messages in thread
From: Xiaoyao Li @ 2020-07-02 22:29 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
On 7/3/2020 2:59 AM, Sean Christopherson wrote:
> On Tue, Jun 23, 2020 at 07:58:15PM +0800, Xiaoyao Li wrote:
>> kvm_x86_ops.update_vcpu_model() is used to update vmx/svm vcpu settings
>> based on updated CPUID settings. So it's supposed to be called after
>> CPUIDs are fully updated, i.e., kvm_update_cpuid().
>>
>> Move it in kvm_update_vcpu_model().
>
> The changelog needs to provide an in-depth analysis of VMX and SVM to prove
> that there are no existing dependencies in the ordering. I've done the
> analysis a few times over the past few years for a similar chage I carried
> in my SGX code, but dropped that code a while back and haven't done the
> analysis since. Anyways, it should be documented.
No problem. Will add the analysis in next version.
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>> ---
>> arch/x86/kvm/cpuid.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index d2f93823f9fd..5decc2dd5448 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -121,6 +121,8 @@ void kvm_update_vcpu_model(struct kvm_vcpu *vcpu)
>> struct kvm_lapic *apic = vcpu->arch.apic;
>> struct kvm_cpuid_entry2 *best;
>>
>> + kvm_x86_ops.update_vcpu_model(vcpu);
>> +
>> best = kvm_find_cpuid_entry(vcpu, 1, 0);
>> if (best && apic) {
>> if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
>> @@ -136,6 +138,7 @@ void kvm_update_vcpu_model(struct kvm_vcpu *vcpu)
>> vcpu->arch.guest_supported_xcr0 =
>> (best->eax | ((u64)best->edx << 32)) & supported_xcr0;
>>
>> +
>
> Spurious whitespace.
>
>> /* Note, maxphyaddr must be updated before tdp_level. */
>> vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
>> vcpu->arch.tdp_level = kvm_x86_ops.get_tdp_level(vcpu);
>> @@ -224,7 +227,6 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>>
>> cpuid_fix_nx_cap(vcpu);
>> kvm_apic_set_version(vcpu);
>> - kvm_x86_ops.update_vcpu_model(vcpu);
>> kvm_update_cpuid(vcpu);
>> kvm_update_vcpu_model(vcpu);
>>
>> @@ -254,7 +256,6 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
>> }
>>
>> kvm_apic_set_version(vcpu);
>> - kvm_x86_ops.update_vcpu_model(vcpu);
>> kvm_update_cpuid(vcpu);
>> kvm_update_vcpu_model(vcpu);
>> out:
>> --
>> 2.18.2
>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 7/7] KVM: X86: Move kvm_apic_set_version() to kvm_update_vcpu_model()
2020-06-23 11:58 [PATCH v2 0/7] Refactor handling flow of SET_CPUID* Xiaoyao Li
` (5 preceding siblings ...)
2020-06-23 11:58 ` [PATCH v2 6/7] KVM: X86: Move kvm_x86_ops.update_vcpu_model() into kvm_update_vcpu_model() Xiaoyao Li
@ 2020-06-23 11:58 ` Xiaoyao Li
2020-07-02 19:00 ` Sean Christopherson
6 siblings, 1 reply; 17+ messages in thread
From: Xiaoyao Li @ 2020-06-23 11:58 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov
Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, Xiaoyao Li
Obviously, kvm_apic_set_version() fits well in kvm_update_vcpu_model().
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
arch/x86/kvm/cpuid.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 5decc2dd5448..3428f4d84b42 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -129,6 +129,8 @@ void kvm_update_vcpu_model(struct kvm_vcpu *vcpu)
apic->lapic_timer.timer_mode_mask = 3 << 17;
else
apic->lapic_timer.timer_mode_mask = 1 << 17;
+
+ kvm_apic_set_version(vcpu);
}
best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
@@ -226,7 +228,6 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
}
cpuid_fix_nx_cap(vcpu);
- kvm_apic_set_version(vcpu);
kvm_update_cpuid(vcpu);
kvm_update_vcpu_model(vcpu);
@@ -255,7 +256,6 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
goto out;
}
- kvm_apic_set_version(vcpu);
kvm_update_cpuid(vcpu);
kvm_update_vcpu_model(vcpu);
out:
--
2.18.2
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 7/7] KVM: X86: Move kvm_apic_set_version() to kvm_update_vcpu_model()
2020-06-23 11:58 ` [PATCH v2 7/7] KVM: X86: Move kvm_apic_set_version() to kvm_update_vcpu_model() Xiaoyao Li
@ 2020-07-02 19:00 ` Sean Christopherson
2020-07-02 22:30 ` Xiaoyao Li
0 siblings, 1 reply; 17+ messages in thread
From: Sean Christopherson @ 2020-07-02 19:00 UTC (permalink / raw)
To: Xiaoyao Li
Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
On Tue, Jun 23, 2020 at 07:58:16PM +0800, Xiaoyao Li wrote:
> Obviously, kvm_apic_set_version() fits well in kvm_update_vcpu_model().
Same as the last patch, it would be nice to explicitly document that there
are no dependencies between kvm_apic_set_version() and kvm_update_cpuid().
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> arch/x86/kvm/cpuid.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 5decc2dd5448..3428f4d84b42 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -129,6 +129,8 @@ void kvm_update_vcpu_model(struct kvm_vcpu *vcpu)
> apic->lapic_timer.timer_mode_mask = 3 << 17;
> else
> apic->lapic_timer.timer_mode_mask = 1 << 17;
> +
> + kvm_apic_set_version(vcpu);
> }
>
> best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
> @@ -226,7 +228,6 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
> }
>
> cpuid_fix_nx_cap(vcpu);
> - kvm_apic_set_version(vcpu);
> kvm_update_cpuid(vcpu);
> kvm_update_vcpu_model(vcpu);
>
> @@ -255,7 +256,6 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
> goto out;
> }
>
> - kvm_apic_set_version(vcpu);
> kvm_update_cpuid(vcpu);
> kvm_update_vcpu_model(vcpu);
> out:
> --
> 2.18.2
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 7/7] KVM: X86: Move kvm_apic_set_version() to kvm_update_vcpu_model()
2020-07-02 19:00 ` Sean Christopherson
@ 2020-07-02 22:30 ` Xiaoyao Li
0 siblings, 0 replies; 17+ messages in thread
From: Xiaoyao Li @ 2020-07-02 22:30 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
On 7/3/2020 3:00 AM, Sean Christopherson wrote:
> On Tue, Jun 23, 2020 at 07:58:16PM +0800, Xiaoyao Li wrote:
>> Obviously, kvm_apic_set_version() fits well in kvm_update_vcpu_model().
>
> Same as the last patch, it would be nice to explicitly document that there
> are no dependencies between kvm_apic_set_version() and kvm_update_cpuid().
Sure, will do.
Thanks!
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>> arch/x86/kvm/cpuid.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 5decc2dd5448..3428f4d84b42 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -129,6 +129,8 @@ void kvm_update_vcpu_model(struct kvm_vcpu *vcpu)
>> apic->lapic_timer.timer_mode_mask = 3 << 17;
>> else
>> apic->lapic_timer.timer_mode_mask = 1 << 17;
>> +
>> + kvm_apic_set_version(vcpu);
>> }
>>
>> best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
>> @@ -226,7 +228,6 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>> }
>>
>> cpuid_fix_nx_cap(vcpu);
>> - kvm_apic_set_version(vcpu);
>> kvm_update_cpuid(vcpu);
>> kvm_update_vcpu_model(vcpu);
>>
>> @@ -255,7 +256,6 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
>> goto out;
>> }
>>
>> - kvm_apic_set_version(vcpu);
>> kvm_update_cpuid(vcpu);
>> kvm_update_vcpu_model(vcpu);
>> out:
>> --
>> 2.18.2
>>
^ permalink raw reply [flat|nested] 17+ messages in thread