[1/2] kvm: vmx: Introduce lapic_mode enumeration

Message ID	20180509205605.2161-2-krish.sadhukhan@oracle.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <kvm-owner@kernel.org> From: Krish Sadhukhan <krish.sadhukhan@oracle.com> To: kvm@vger.kernel.org Cc: pbonzini@redhat.com, rkrcmar@redhat.com, jmattson@google.com Subject: [PATCH 1/2] kvm: vmx: Introduce lapic_mode enumeration Date: Wed, 9 May 2018 16:56:04 -0400 Message-Id: <20180509205605.2161-2-krish.sadhukhan@oracle.com> In-Reply-To: <20180509205605.2161-1-krish.sadhukhan@oracle.com> References: <20180509205605.2161-1-krish.sadhukhan@oracle.com> Sender: kvm-owner@vger.kernel.org Precedence: bulk

Krish Sadhukhan May 9, 2018, 8:56 p.m. UTC

From: Jim Mattson <jmattson@google.com>

The local APIC can be in one of three modes: disabled, xAPIC or
x2APIC. (A fourth mode, "invalid," is included for completeness.)

Using the new enumeration can make some of the APIC mode logic easier
to read. In kvm_set_apic_base, for instance, it is clear that one
cannot transition directly from x2APIC mode to xAPIC mode or directly
from APIC disabled to x2APIC mode.

Signed-off-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 arch/x86/kvm/lapic.h | 14 ++++++++++++++
 arch/x86/kvm/x86.c   | 31 +++++++++++++++++++------------
 2 files changed, 33 insertions(+), 12 deletions(-)

Wanpeng Li May 12, 2018, 11:40 p.m. UTC | #1

2018-05-10 4:56 GMT+08:00 Krish Sadhukhan <krish.sadhukhan@oracle.com>:
> From: Jim Mattson <jmattson@google.com>
>
> The local APIC can be in one of three modes: disabled, xAPIC or
> x2APIC. (A fourth mode, "invalid," is included for completeness.)
>
> Using the new enumeration can make some of the APIC mode logic easier
> to read. In kvm_set_apic_base, for instance, it is clear that one
> cannot transition directly from x2APIC mode to xAPIC mode or directly
> from APIC disabled to x2APIC mode.
>
> Signed-off-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> ---
>  arch/x86/kvm/lapic.h | 14 ++++++++++++++
>  arch/x86/kvm/x86.c   | 31 +++++++++++++++++++------------
>  2 files changed, 33 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index edce055..ed0ed39 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -16,6 +16,13 @@
>  #define APIC_BUS_CYCLE_NS       1
>  #define APIC_BUS_FREQUENCY      (1000000000ULL / APIC_BUS_CYCLE_NS)
>
> +enum lapic_mode {
> +       LAPIC_MODE_DISABLED = 0,
> +       LAPIC_MODE_INVALID = X2APIC_ENABLE,
> +       LAPIC_MODE_XAPIC = MSR_IA32_APICBASE_ENABLE,
> +       LAPIC_MODE_X2APIC = MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE,
> +};
> +
>  struct kvm_timer {
>         struct hrtimer timer;
>         s64 period;                             /* unit: ns */
> @@ -89,6 +96,7 @@ u64 kvm_get_apic_base(struct kvm_vcpu *vcpu);
>  int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
>  int kvm_apic_get_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s);
>  int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s);
> +enum lapic_mode kvm_get_apic_mode(struct kvm_vcpu *vcpu);
>  int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu);
>
>  u64 kvm_get_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu);
> @@ -220,4 +228,10 @@ void kvm_lapic_switch_to_hv_timer(struct kvm_vcpu *vcpu);
>  void kvm_lapic_expired_hv_timer(struct kvm_vcpu *vcpu);
>  bool kvm_lapic_hv_timer_in_use(struct kvm_vcpu *vcpu);
>  void kvm_lapic_restart_hv_timer(struct kvm_vcpu *vcpu);
> +
> +static inline enum lapic_mode kvm_apic_mode(u64 apic_base)
> +{
> +       return apic_base & (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE);
> +}
> +
>  #endif
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 51ecd38..5f5874c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -318,23 +318,30 @@ u64 kvm_get_apic_base(struct kvm_vcpu *vcpu)
>  }
>  EXPORT_SYMBOL_GPL(kvm_get_apic_base);
>
> +enum lapic_mode kvm_get_apic_mode(struct kvm_vcpu *vcpu)
> +{
> +       return kvm_apic_mode(kvm_get_apic_base(vcpu));
> +}
> +EXPORT_SYMBOL_GPL(kvm_get_apic_mode);
> +
>  int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  {
> -       u64 old_state = vcpu->arch.apic_base &
> -               (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE);
> -       u64 new_state = msr_info->data &
> -               (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE);
> +       enum lapic_mode old_mode = kvm_get_apic_mode(vcpu);
> +       enum lapic_mode new_mode = kvm_apic_mode(msr_info->data);
>         u64 reserved_bits = ((~0ULL) << cpuid_maxphyaddr(vcpu)) | 0x2ff |
>                 (guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE);
>
> -       if ((msr_info->data & reserved_bits) || new_state == X2APIC_ENABLE)
> -               return 1;
> -       if (!msr_info->host_initiated &&
> -           ((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)))
> -               return 1;
> +       if (!msr_info->host_initiated) {
> +               if ((msr_info->data & reserved_bits) != 0 ||
> +                   new_mode == LAPIC_MODE_INVALID)
> +                       return 1;

This is opposite to commit d3802286fa (kvm: x86: Disallow illegal
IA32_APIC_BASE MSR values), any good reason this time?

Regards,
Wanpeng Li

> +               if (old_mode == LAPIC_MODE_X2APIC &&
> +                   new_mode == LAPIC_MODE_XAPIC)
> +                       return 1;
> +               if (old_mode == LAPIC_MODE_DISABLED &&
> +                   new_mode == LAPIC_MODE_X2APIC)
> +                       return 1;
> +       }
>
>         kvm_lapic_set_base(vcpu, msr_info->data);
>         return 0;
> --
> 2.9.5
>

Paolo Bonzini May 14, 2018, 3:48 p.m. UTC | #2

On 13/05/2018 01:40, Wanpeng Li wrote:
>> -       if ((msr_info->data & reserved_bits) || new_state == X2APIC_ENABLE)
>> -               return 1;
>> -       if (!msr_info->host_initiated &&
>> -           ((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)))
>> -               return 1;
>> +       if (!msr_info->host_initiated) {
>> +               if ((msr_info->data & reserved_bits) != 0 ||
>> +                   new_mode == LAPIC_MODE_INVALID)
>> +                       return 1;
> This is opposite to commit d3802286fa (kvm: x86: Disallow illegal
> IA32_APIC_BASE MSR values), any good reason this time?

I agree, msr_info->host_initiated does not matter for the invalid check.

Paolo

Jim Mattson May 14, 2018, 5:47 p.m. UTC | #3

This is a result of the patches being applied out of order. We'll fix it up.



On Sat, May 12, 2018 at 4:40 PM, Wanpeng Li <kernellwp@gmail.com> wrote:
> 2018-05-10 4:56 GMT+08:00 Krish Sadhukhan <krish.sadhukhan@oracle.com>:
>> From: Jim Mattson <jmattson@google.com>
>>
>> The local APIC can be in one of three modes: disabled, xAPIC or
>> x2APIC. (A fourth mode, "invalid," is included for completeness.)
>>
>> Using the new enumeration can make some of the APIC mode logic easier
>> to read. In kvm_set_apic_base, for instance, it is clear that one
>> cannot transition directly from x2APIC mode to xAPIC mode or directly
>> from APIC disabled to x2APIC mode.
>>
>> Signed-off-by: Jim Mattson <jmattson@google.com>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> ---
>>  arch/x86/kvm/lapic.h | 14 ++++++++++++++
>>  arch/x86/kvm/x86.c   | 31 +++++++++++++++++++------------
>>  2 files changed, 33 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
>> index edce055..ed0ed39 100644
>> --- a/arch/x86/kvm/lapic.h
>> +++ b/arch/x86/kvm/lapic.h
>> @@ -16,6 +16,13 @@
>>  #define APIC_BUS_CYCLE_NS       1
>>  #define APIC_BUS_FREQUENCY      (1000000000ULL / APIC_BUS_CYCLE_NS)
>>
>> +enum lapic_mode {
>> +       LAPIC_MODE_DISABLED = 0,
>> +       LAPIC_MODE_INVALID = X2APIC_ENABLE,
>> +       LAPIC_MODE_XAPIC = MSR_IA32_APICBASE_ENABLE,
>> +       LAPIC_MODE_X2APIC = MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE,
>> +};
>> +
>>  struct kvm_timer {
>>         struct hrtimer timer;
>>         s64 period;                             /* unit: ns */
>> @@ -89,6 +96,7 @@ u64 kvm_get_apic_base(struct kvm_vcpu *vcpu);
>>  int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
>>  int kvm_apic_get_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s);
>>  int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s);
>> +enum lapic_mode kvm_get_apic_mode(struct kvm_vcpu *vcpu);
>>  int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu);
>>
>>  u64 kvm_get_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu);
>> @@ -220,4 +228,10 @@ void kvm_lapic_switch_to_hv_timer(struct kvm_vcpu *vcpu);
>>  void kvm_lapic_expired_hv_timer(struct kvm_vcpu *vcpu);
>>  bool kvm_lapic_hv_timer_in_use(struct kvm_vcpu *vcpu);
>>  void kvm_lapic_restart_hv_timer(struct kvm_vcpu *vcpu);
>> +
>> +static inline enum lapic_mode kvm_apic_mode(u64 apic_base)
>> +{
>> +       return apic_base & (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE);
>> +}
>> +
>>  #endif
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 51ecd38..5f5874c 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -318,23 +318,30 @@ u64 kvm_get_apic_base(struct kvm_vcpu *vcpu)
>>  }
>>  EXPORT_SYMBOL_GPL(kvm_get_apic_base);
>>
>> +enum lapic_mode kvm_get_apic_mode(struct kvm_vcpu *vcpu)
>> +{
>> +       return kvm_apic_mode(kvm_get_apic_base(vcpu));
>> +}
>> +EXPORT_SYMBOL_GPL(kvm_get_apic_mode);
>> +
>>  int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>  {
>> -       u64 old_state = vcpu->arch.apic_base &
>> -               (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE);
>> -       u64 new_state = msr_info->data &
>> -               (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE);
>> +       enum lapic_mode old_mode = kvm_get_apic_mode(vcpu);
>> +       enum lapic_mode new_mode = kvm_apic_mode(msr_info->data);
>>         u64 reserved_bits = ((~0ULL) << cpuid_maxphyaddr(vcpu)) | 0x2ff |
>>                 (guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE);
>>
>> -       if ((msr_info->data & reserved_bits) || new_state == X2APIC_ENABLE)
>> -               return 1;
>> -       if (!msr_info->host_initiated &&
>> -           ((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)))
>> -               return 1;
>> +       if (!msr_info->host_initiated) {
>> +               if ((msr_info->data & reserved_bits) != 0 ||
>> +                   new_mode == LAPIC_MODE_INVALID)
>> +                       return 1;
>
> This is opposite to commit d3802286fa (kvm: x86: Disallow illegal
> IA32_APIC_BASE MSR values), any good reason this time?
>
> Regards,
> Wanpeng Li
>
>> +               if (old_mode == LAPIC_MODE_X2APIC &&
>> +                   new_mode == LAPIC_MODE_XAPIC)
>> +                       return 1;
>> +               if (old_mode == LAPIC_MODE_DISABLED &&
>> +                   new_mode == LAPIC_MODE_X2APIC)
>> +                       return 1;
>> +       }
>>
>>         kvm_lapic_set_base(vcpu, msr_info->data);
>>         return 0;
>> --
>> 2.9.5
>>

Paolo Bonzini May 15, 2018, 8:47 a.m. UTC | #4

On 14/05/2018 19:47, Jim Mattson wrote:
> This is a result of the patches being applied out of order. We'll fix it up.

No problem, I've fixed it locally.

Paolo

> 
> 
> On Sat, May 12, 2018 at 4:40 PM, Wanpeng Li <kernellwp@gmail.com> wrote:
>> 2018-05-10 4:56 GMT+08:00 Krish Sadhukhan <krish.sadhukhan@oracle.com>:
>>> From: Jim Mattson <jmattson@google.com>
>>>
>>> The local APIC can be in one of three modes: disabled, xAPIC or
>>> x2APIC. (A fourth mode, "invalid," is included for completeness.)
>>>
>>> Using the new enumeration can make some of the APIC mode logic easier
>>> to read. In kvm_set_apic_base, for instance, it is clear that one
>>> cannot transition directly from x2APIC mode to xAPIC mode or directly
>>> from APIC disabled to x2APIC mode.
>>>
>>> Signed-off-by: Jim Mattson <jmattson@google.com>
>>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>>> ---
>>>  arch/x86/kvm/lapic.h | 14 ++++++++++++++
>>>  arch/x86/kvm/x86.c   | 31 +++++++++++++++++++------------
>>>  2 files changed, 33 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
>>> index edce055..ed0ed39 100644
>>> --- a/arch/x86/kvm/lapic.h
>>> +++ b/arch/x86/kvm/lapic.h
>>> @@ -16,6 +16,13 @@
>>>  #define APIC_BUS_CYCLE_NS       1
>>>  #define APIC_BUS_FREQUENCY      (1000000000ULL / APIC_BUS_CYCLE_NS)
>>>
>>> +enum lapic_mode {
>>> +       LAPIC_MODE_DISABLED = 0,
>>> +       LAPIC_MODE_INVALID = X2APIC_ENABLE,
>>> +       LAPIC_MODE_XAPIC = MSR_IA32_APICBASE_ENABLE,
>>> +       LAPIC_MODE_X2APIC = MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE,
>>> +};
>>> +
>>>  struct kvm_timer {
>>>         struct hrtimer timer;
>>>         s64 period;                             /* unit: ns */
>>> @@ -89,6 +96,7 @@ u64 kvm_get_apic_base(struct kvm_vcpu *vcpu);
>>>  int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
>>>  int kvm_apic_get_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s);
>>>  int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s);
>>> +enum lapic_mode kvm_get_apic_mode(struct kvm_vcpu *vcpu);
>>>  int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu);
>>>
>>>  u64 kvm_get_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu);
>>> @@ -220,4 +228,10 @@ void kvm_lapic_switch_to_hv_timer(struct kvm_vcpu *vcpu);
>>>  void kvm_lapic_expired_hv_timer(struct kvm_vcpu *vcpu);
>>>  bool kvm_lapic_hv_timer_in_use(struct kvm_vcpu *vcpu);
>>>  void kvm_lapic_restart_hv_timer(struct kvm_vcpu *vcpu);
>>> +
>>> +static inline enum lapic_mode kvm_apic_mode(u64 apic_base)
>>> +{
>>> +       return apic_base & (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE);
>>> +}
>>> +
>>>  #endif
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 51ecd38..5f5874c 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -318,23 +318,30 @@ u64 kvm_get_apic_base(struct kvm_vcpu *vcpu)
>>>  }
>>>  EXPORT_SYMBOL_GPL(kvm_get_apic_base);
>>>
>>> +enum lapic_mode kvm_get_apic_mode(struct kvm_vcpu *vcpu)
>>> +{
>>> +       return kvm_apic_mode(kvm_get_apic_base(vcpu));
>>> +}
>>> +EXPORT_SYMBOL_GPL(kvm_get_apic_mode);
>>> +
>>>  int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>>  {
>>> -       u64 old_state = vcpu->arch.apic_base &
>>> -               (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE);
>>> -       u64 new_state = msr_info->data &
>>> -               (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE);
>>> +       enum lapic_mode old_mode = kvm_get_apic_mode(vcpu);
>>> +       enum lapic_mode new_mode = kvm_apic_mode(msr_info->data);
>>>         u64 reserved_bits = ((~0ULL) << cpuid_maxphyaddr(vcpu)) | 0x2ff |
>>>                 (guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE);
>>>
>>> -       if ((msr_info->data & reserved_bits) || new_state == X2APIC_ENABLE)
>>> -               return 1;
>>> -       if (!msr_info->host_initiated &&
>>> -           ((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)))
>>> -               return 1;
>>> +       if (!msr_info->host_initiated) {
>>> +               if ((msr_info->data & reserved_bits) != 0 ||
>>> +                   new_mode == LAPIC_MODE_INVALID)
>>> +                       return 1;
>>
>> This is opposite to commit d3802286fa (kvm: x86: Disallow illegal
>> IA32_APIC_BASE MSR values), any good reason this time?
>>
>> Regards,
>> Wanpeng Li
>>
>>> +               if (old_mode == LAPIC_MODE_X2APIC &&
>>> +                   new_mode == LAPIC_MODE_XAPIC)
>>> +                       return 1;
>>> +               if (old_mode == LAPIC_MODE_DISABLED &&
>>> +                   new_mode == LAPIC_MODE_X2APIC)
>>> +                       return 1;
>>> +       }
>>>
>>>         kvm_lapic_set_base(vcpu, msr_info->data);
>>>         return 0;
>>> --
>>> 2.9.5
>>>

Jim Mattson May 15, 2018, 4:17 p.m. UTC | #5

Hi Paolo,

Does it look something like this?

if ((msr_info->data & reserved_bits) || new_mode == LAPIC_MODE_INVALID)
        return 1;
if (!msr_info->host_initiated) {
        if (old_mode == LAPIC_MODE_X2APIC &&
            new_mode == LAPIC_MODE_XAPIC)
                return 1;
        if (old_mode == LAPIC_MODE_DISABLED &&
            new_mode == LAPIC_MODE_X2APIC)
                return 1;
}

On Tue, May 15, 2018 at 1:47 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 14/05/2018 19:47, Jim Mattson wrote:
>> This is a result of the patches being applied out of order. We'll fix it up.
>
> No problem, I've fixed it locally.
>
> Paolo
>
>>
>>
>> On Sat, May 12, 2018 at 4:40 PM, Wanpeng Li <kernellwp@gmail.com> wrote:
>>> 2018-05-10 4:56 GMT+08:00 Krish Sadhukhan <krish.sadhukhan@oracle.com>:
>>>> From: Jim Mattson <jmattson@google.com>
>>>>
>>>> The local APIC can be in one of three modes: disabled, xAPIC or
>>>> x2APIC. (A fourth mode, "invalid," is included for completeness.)
>>>>
>>>> Using the new enumeration can make some of the APIC mode logic easier
>>>> to read. In kvm_set_apic_base, for instance, it is clear that one
>>>> cannot transition directly from x2APIC mode to xAPIC mode or directly
>>>> from APIC disabled to x2APIC mode.
>>>>
>>>> Signed-off-by: Jim Mattson <jmattson@google.com>
>>>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>>>> ---
>>>>  arch/x86/kvm/lapic.h | 14 ++++++++++++++
>>>>  arch/x86/kvm/x86.c   | 31 +++++++++++++++++++------------
>>>>  2 files changed, 33 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
>>>> index edce055..ed0ed39 100644
>>>> --- a/arch/x86/kvm/lapic.h
>>>> +++ b/arch/x86/kvm/lapic.h
>>>> @@ -16,6 +16,13 @@
>>>>  #define APIC_BUS_CYCLE_NS       1
>>>>  #define APIC_BUS_FREQUENCY      (1000000000ULL / APIC_BUS_CYCLE_NS)
>>>>
>>>> +enum lapic_mode {
>>>> +       LAPIC_MODE_DISABLED = 0,
>>>> +       LAPIC_MODE_INVALID = X2APIC_ENABLE,
>>>> +       LAPIC_MODE_XAPIC = MSR_IA32_APICBASE_ENABLE,
>>>> +       LAPIC_MODE_X2APIC = MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE,
>>>> +};
>>>> +
>>>>  struct kvm_timer {
>>>>         struct hrtimer timer;
>>>>         s64 period;                             /* unit: ns */
>>>> @@ -89,6 +96,7 @@ u64 kvm_get_apic_base(struct kvm_vcpu *vcpu);
>>>>  int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
>>>>  int kvm_apic_get_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s);
>>>>  int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s);
>>>> +enum lapic_mode kvm_get_apic_mode(struct kvm_vcpu *vcpu);
>>>>  int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu);
>>>>
>>>>  u64 kvm_get_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu);
>>>> @@ -220,4 +228,10 @@ void kvm_lapic_switch_to_hv_timer(struct kvm_vcpu *vcpu);
>>>>  void kvm_lapic_expired_hv_timer(struct kvm_vcpu *vcpu);
>>>>  bool kvm_lapic_hv_timer_in_use(struct kvm_vcpu *vcpu);
>>>>  void kvm_lapic_restart_hv_timer(struct kvm_vcpu *vcpu);
>>>> +
>>>> +static inline enum lapic_mode kvm_apic_mode(u64 apic_base)
>>>> +{
>>>> +       return apic_base & (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE);
>>>> +}
>>>> +
>>>>  #endif
>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>>> index 51ecd38..5f5874c 100644
>>>> --- a/arch/x86/kvm/x86.c
>>>> +++ b/arch/x86/kvm/x86.c
>>>> @@ -318,23 +318,30 @@ u64 kvm_get_apic_base(struct kvm_vcpu *vcpu)
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(kvm_get_apic_base);
>>>>
>>>> +enum lapic_mode kvm_get_apic_mode(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +       return kvm_apic_mode(kvm_get_apic_base(vcpu));
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(kvm_get_apic_mode);
>>>> +
>>>>  int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>>>  {
>>>> -       u64 old_state = vcpu->arch.apic_base &
>>>> -               (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE);
>>>> -       u64 new_state = msr_info->data &
>>>> -               (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE);
>>>> +       enum lapic_mode old_mode = kvm_get_apic_mode(vcpu);
>>>> +       enum lapic_mode new_mode = kvm_apic_mode(msr_info->data);
>>>>         u64 reserved_bits = ((~0ULL) << cpuid_maxphyaddr(vcpu)) | 0x2ff |
>>>>                 (guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE);
>>>>
>>>> -       if ((msr_info->data & reserved_bits) || new_state == X2APIC_ENABLE)
>>>> -               return 1;
>>>> -       if (!msr_info->host_initiated &&
>>>> -           ((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)))
>>>> -               return 1;
>>>> +       if (!msr_info->host_initiated) {
>>>> +               if ((msr_info->data & reserved_bits) != 0 ||
>>>> +                   new_mode == LAPIC_MODE_INVALID)
>>>> +                       return 1;
>>>
>>> This is opposite to commit d3802286fa (kvm: x86: Disallow illegal
>>> IA32_APIC_BASE MSR values), any good reason this time?
>>>
>>> Regards,
>>> Wanpeng Li
>>>
>>>> +               if (old_mode == LAPIC_MODE_X2APIC &&
>>>> +                   new_mode == LAPIC_MODE_XAPIC)
>>>> +                       return 1;
>>>> +               if (old_mode == LAPIC_MODE_DISABLED &&
>>>> +                   new_mode == LAPIC_MODE_X2APIC)
>>>> +                       return 1;
>>>> +       }
>>>>
>>>>         kvm_lapic_set_base(vcpu, msr_info->data);
>>>>         return 0;
>>>> --
>>>> 2.9.5
>>>>
>

Paolo Bonzini May 15, 2018, 4:42 p.m. UTC | #6

On 15/05/2018 18:17, Jim Mattson wrote:
> Hi Paolo,
> 
> Does it look something like this?
> 
> if ((msr_info->data & reserved_bits) || new_mode == LAPIC_MODE_INVALID)
>         return 1;
> if (!msr_info->host_initiated) {
>         if (old_mode == LAPIC_MODE_X2APIC &&
>             new_mode == LAPIC_MODE_XAPIC)
>                 return 1;
>         if (old_mode == LAPIC_MODE_DISABLED &&
>             new_mode == LAPIC_MODE_X2APIC)
>                 return 1;
> }

Yes, entirely.  You can check here:

https://git.kernel.org/pub/scm/virt/kvm/kvm.git/commit/?h=next&id=588716494258899389206fa50426e78cc9df89b9

(look at all those digits at the beginning of the commit SHA1).

Paolo

[1/2] kvm: vmx: Introduce lapic_mode enumeration

Commit Message

Comments

Patch