This function returns the currently programmed guest physical
APIC ID of a vCPU in both xAPIC and x2APIC modes.
Suggested-by: Maxim Levitsky <[email protected]>
Signed-off-by: Suravee Suthikulpanit <[email protected]>
---
arch/x86/kvm/lapic.c | 23 +++++++++++++++++++++++
arch/x86/kvm/lapic.h | 5 +----
arch/x86/kvm/svm/avic.c | 21 +++++++++++++++++----
3 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 03d1b6325eb8..73a1e650a294 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -106,11 +106,34 @@ static inline int apic_enabled(struct kvm_lapic *apic)
(LVT_MASK | APIC_MODE_MASK | APIC_INPUT_POLARITY | \
APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER)
+static inline u8 kvm_xapic_id(struct kvm_lapic *apic)
+{
+ return kvm_lapic_get_reg(apic, APIC_ID) >> 24;
+}
+
static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)
{
return apic->vcpu->vcpu_id;
}
+int kvm_get_apic_id(struct kvm_vcpu *vcpu, u32 *id)
+{
+ if (!id)
+ return -EINVAL;
+
+ if (!apic_x2apic_mode(vcpu->arch.apic)) {
+ /* For xAPIC, APIC ID cannot be larger than 254. */
+ if (vcpu->vcpu_id >= APIC_BROADCAST)
+ return -EINVAL;
+
+ *id = kvm_xapic_id(vcpu->arch.apic);
+ } else {
+ *id = kvm_x2apic_id(vcpu->arch.apic);
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(kvm_get_apic_id);
+
static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
{
return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 2b44e533fc8d..2b9463da1528 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -254,9 +254,6 @@ static inline enum lapic_mode kvm_apic_mode(u64 apic_base)
return apic_base & (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE);
}
-static inline u8 kvm_xapic_id(struct kvm_lapic *apic)
-{
- return kvm_lapic_get_reg(apic, APIC_ID) >> 24;
-}
+int kvm_get_apic_id(struct kvm_vcpu *vcpu, u32 *id);
#endif
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 4d7a8743196e..7e5a39a8e698 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -441,14 +441,21 @@ static void avic_invalidate_logical_id_entry(struct kvm_vcpu *vcpu)
static int avic_handle_ldr_update(struct kvm_vcpu *vcpu)
{
- int ret = 0;
+ int ret;
struct vcpu_svm *svm = to_svm(vcpu);
u32 ldr = kvm_lapic_get_reg(vcpu->arch.apic, APIC_LDR);
- u32 id = kvm_xapic_id(vcpu->arch.apic);
+ u32 id;
+
+ ret = kvm_get_apic_id(vcpu, &id);
+ if (ret)
+ return ret;
if (ldr == svm->ldr_reg)
return 0;
+ if (id == X2APIC_BROADCAST)
+ return -EINVAL;
+
avic_invalidate_logical_id_entry(vcpu);
if (ldr)
@@ -464,7 +471,12 @@ static int avic_handle_apic_id_update(struct kvm_vcpu *vcpu)
{
u64 *old, *new;
struct vcpu_svm *svm = to_svm(vcpu);
- u32 id = kvm_xapic_id(vcpu->arch.apic);
+ u32 id;
+ int ret;
+
+ ret = kvm_get_apic_id(vcpu, &id);
+ if (ret)
+ return 1;
if (vcpu->vcpu_id == id)
return 0;
@@ -484,7 +496,8 @@ static int avic_handle_apic_id_update(struct kvm_vcpu *vcpu)
* APIC ID table entry if already setup the LDR.
*/
if (svm->ldr_reg)
- avic_handle_ldr_update(vcpu);
+ if (avic_handle_ldr_update(vcpu))
+ return 1;
return 0;
}
--
2.25.1
On Tue, 2022-03-08 at 10:39 -0600, Suravee Suthikulpanit wrote:
> This function returns the currently programmed guest physical
> APIC ID of a vCPU in both xAPIC and x2APIC modes.
>
> Suggested-by: Maxim Levitsky <[email protected]>
> Signed-off-by: Suravee Suthikulpanit <[email protected]>
> ---
> arch/x86/kvm/lapic.c | 23 +++++++++++++++++++++++
> arch/x86/kvm/lapic.h | 5 +----
> arch/x86/kvm/svm/avic.c | 21 +++++++++++++++++----
> 3 files changed, 41 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 03d1b6325eb8..73a1e650a294 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -106,11 +106,34 @@ static inline int apic_enabled(struct kvm_lapic *apic)
> (LVT_MASK | APIC_MODE_MASK | APIC_INPUT_POLARITY | \
> APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER)
>
> +static inline u8 kvm_xapic_id(struct kvm_lapic *apic)
> +{
> + return kvm_lapic_get_reg(apic, APIC_ID) >> 24;
> +}
> +
> static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)
> {
> return apic->vcpu->vcpu_id;
> }
>
> +int kvm_get_apic_id(struct kvm_vcpu *vcpu, u32 *id)
> +{
> + if (!id)
> + return -EINVAL;
> +
> + if (!apic_x2apic_mode(vcpu->arch.apic)) {
> + /* For xAPIC, APIC ID cannot be larger than 254. */
> + if (vcpu->vcpu_id >= APIC_BROADCAST)
> + return -EINVAL;
> +
> + *id = kvm_xapic_id(vcpu->arch.apic);
> + } else {
> + *id = kvm_x2apic_id(vcpu->arch.apic);
> + }
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(kvm_get_apic_id);
> +
> static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
> {
> return pi_inject_timer && kvm_vcpu_apicv_active(vcpu);
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 2b44e533fc8d..2b9463da1528 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -254,9 +254,6 @@ static inline enum lapic_mode kvm_apic_mode(u64 apic_base)
> return apic_base & (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE);
> }
>
> -static inline u8 kvm_xapic_id(struct kvm_lapic *apic)
> -{
> - return kvm_lapic_get_reg(apic, APIC_ID) >> 24;
> -}
> +int kvm_get_apic_id(struct kvm_vcpu *vcpu, u32 *id);
>
> #endif
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 4d7a8743196e..7e5a39a8e698 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -441,14 +441,21 @@ static void avic_invalidate_logical_id_entry(struct kvm_vcpu *vcpu)
>
> static int avic_handle_ldr_update(struct kvm_vcpu *vcpu)
> {
> - int ret = 0;
> + int ret;
> struct vcpu_svm *svm = to_svm(vcpu);
> u32 ldr = kvm_lapic_get_reg(vcpu->arch.apic, APIC_LDR);
> - u32 id = kvm_xapic_id(vcpu->arch.apic);
> + u32 id;
> +
> + ret = kvm_get_apic_id(vcpu, &id);
> + if (ret)
> + return ret;
What about apic_id == 0?
>
> if (ldr == svm->ldr_reg)
> return 0;
>
> + if (id == X2APIC_BROADCAST)
> + return -EINVAL;
> +
Why this is needed? avic_handle_ldr_update is called either
when guest writes to APIC_LDR (should not reach here),
or if LDR got changed while AVIC was inhibited (also
thankfully KVM doesn't allow it to be changed in x2APIC mode,
and it does reset it when enabling x2apic).
> avic_invalidate_logical_id_entry(vcpu);
>
> if (ldr)
> @@ -464,7 +471,12 @@ static int avic_handle_apic_id_update(struct kvm_vcpu *vcpu)
> {
> u64 *old, *new;
> struct vcpu_svm *svm = to_svm(vcpu);
> - u32 id = kvm_xapic_id(vcpu->arch.apic);
> + u32 id;
> + int ret;
> +
> + ret = kvm_get_apic_id(vcpu, &id);
> + if (ret)
> + return 1;
Well this function is totally broken anyway and I woudn't even bother touching it,
maximum, just stick 'return 0' in the very start of this function if the apic is
in x2apic mode now.
Oh well...
>
> if (vcpu->vcpu_id == id)
> return 0;
> @@ -484,7 +496,8 @@ static int avic_handle_apic_id_update(struct kvm_vcpu *vcpu)
> * APIC ID table entry if already setup the LDR.
> */
> if (svm->ldr_reg)
> - avic_handle_ldr_update(vcpu);
> + if (avic_handle_ldr_update(vcpu))
> + return 1;
>
> return 0;
> }
Best regards,
Maxim Levitsky
Maxim,
On 3/24/22 9:14 PM, Maxim Levitsky wrote:
> On Tue, 2022-03-08 at 10:39 -0600, Suravee Suthikulpanit wrote:
>> This function returns the currently programmed guest physical
>> APIC ID of a vCPU in both xAPIC and x2APIC modes.
>>
>> Suggested-by: Maxim Levitsky <[email protected]>
>> Signed-off-by: Suravee Suthikulpanit <[email protected]>
>> ---
>> arch/x86/kvm/lapic.c | 23 +++++++++++++++++++++++
>> arch/x86/kvm/lapic.h | 5 +----
>> arch/x86/kvm/svm/avic.c | 21 +++++++++++++++++----
>> 3 files changed, 41 insertions(+), 8 deletions(-)
>>
>> ...
>>
>> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
>> index 4d7a8743196e..7e5a39a8e698 100644
>> --- a/arch/x86/kvm/svm/avic.c
>> +++ b/arch/x86/kvm/svm/avic.c
>> @@ -441,14 +441,21 @@ static void avic_invalidate_logical_id_entry(struct kvm_vcpu *vcpu)
>>
>> static int avic_handle_ldr_update(struct kvm_vcpu *vcpu)
>> {
>> - int ret = 0;
>> + int ret;
>> struct vcpu_svm *svm = to_svm(vcpu);
>> u32 ldr = kvm_lapic_get_reg(vcpu->arch.apic, APIC_LDR);
>> - u32 id = kvm_xapic_id(vcpu->arch.apic);
>> + u32 id;
>> +
>> + ret = kvm_get_apic_id(vcpu, &id);
>> + if (ret)
>> + return ret;
> What about apic_id == 0?
The "id" is the returned apic ID. The "ret" is to let caller know if
the APIC look up is successful or not. "ret == 0" means success and
the value in "id" is valid.
>>
>> if (ldr == svm->ldr_reg)
>> return 0;
>>
>> + if (id == X2APIC_BROADCAST)
>> + return -EINVAL;
>> +
>
> Why this is needed? avic_handle_ldr_update is called either
> when guest writes to APIC_LDR (should not reach here),
> or if LDR got changed while AVIC was inhibited (also
> thankfully KVM doesn't allow it to be changed in x2APIC mode,
> and it does reset it when enabling x2apic).
At this point, we don't need to handle LDR update in x2APIC case.
I will add apic_x2apic_mode() check here.
>
>
>
>> avic_invalidate_logical_id_entry(vcpu);
>>
>> if (ldr)
>> @@ -464,7 +471,12 @@ static int avic_handle_apic_id_update(struct kvm_vcpu *vcpu)
>> {
>> u64 *old, *new;
>> struct vcpu_svm *svm = to_svm(vcpu);
>> - u32 id = kvm_xapic_id(vcpu->arch.apic);
>> + u32 id;
>> + int ret;
>> +
>> + ret = kvm_get_apic_id(vcpu, &id);
>> + if (ret)
>> + return 1;
>
> Well this function is totally broken anyway and I woudn't even bother touching it,
> maximum, just stick 'return 0' in the very start of this function if the apic is
> in x2apic mode now.
>
> Oh well...
>
Sorry, I'm not sure if I understand what you mean by "broken".
This function setup/update the AVIC physical APIC ID table, whenever the APIC ID is
initialize or updated. It is needed in both xAPIC and x2APIC cases.
Regards,
Suravee
Maxim
On 4/5/22 10:58 AM, Suravee Suthikulpanit wrote:
>>
>>
>>
>>> avic_invalidate_logical_id_entry(vcpu);
>>> if (ldr)
>>> @@ -464,7 +471,12 @@ static int avic_handle_apic_id_update(struct kvm_vcpu *vcpu)
>>> {
>>> u64 *old, *new;
>>> struct vcpu_svm *svm = to_svm(vcpu);
>>> - u32 id = kvm_xapic_id(vcpu->arch.apic);
>>> + u32 id;
>>> + int ret;
>>> +
>>> + ret = kvm_get_apic_id(vcpu, &id);
>>> + if (ret)
>>> + return 1;
>>
>> Well this function is totally broken anyway and I woudn't even bother touching it,
>> maximum, just stick 'return 0' in the very start of this function if the apic is
>> in x2apic mode now.
>>
>> Oh well...
>>
>
> Sorry, I'm not sure if I understand what you mean by "broken".
>
> This function setup/update the AVIC physical APIC ID table, whenever the APIC ID is
> initialize or updated. It is needed in both xAPIC and x2APIC cases.
Actually, I will rework this part and send out change in the next revision.
Thanks,
Suravee