Document that AVIC is inhibited if any vCPU's APIC ID diverges from its
vCPU ID, i.e. that there's no need to check for a destination match in
the AVIC kick fast path.

Opportunistically tweak comments to remove "guest bug", as that suggests
KVM is punting on error handling, which is not the case.  Targeting a
non-existent vCPU or no vCPUs _may_ be a guest software bug, but whether
or not it's a guest bug is irrelevant.  Such behavior is architecturally
legal and thus needs to faithfully emulated by KVM (and it is).

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/avic.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 05a1cde8175c..3959d4766911 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -380,8 +380,8 @@ static int avic_kick_target_vcpus_fast(struct kvm *kvm, struct kvm_lapic *source
 			cluster = (dest >> 4) << 2;
 		}
 
+		/* Nothing to do if there are no destinations in the cluster. */
 		if (unlikely(!bitmap))
-			/* guest bug: nobody to send the logical interrupt to */
 			return 0;
 
 		if (!is_power_of_2(bitmap))
@@ -399,7 +399,7 @@ static int avic_kick_target_vcpus_fast(struct kvm *kvm, struct kvm_lapic *source
 			if (WARN_ON_ONCE(index != logid_index))
 				return -EINVAL;
 
-			/* guest bug: non existing/reserved logical destination */
+			/* Nothing to do if the logical destination is invalid. */
 			if (unlikely(!(logid_entry & AVIC_LOGICAL_ID_ENTRY_VALID_MASK)))
 				return 0;
 
@@ -418,9 +418,13 @@ static int avic_kick_target_vcpus_fast(struct kvm *kvm, struct kvm_lapic *source
 		}
 	}
 
+	/*
+	 * KVM inhibits AVIC if any vCPU ID diverges from the vCPUs APIC ID,
+	 * i.e. APIC ID == vCPU ID.  Once again, nothing to do if the target
+	 * vCPU doesn't exist.
+	 */
 	target_vcpu = kvm_get_vcpu_by_id(kvm, l1_physical_id);
 	if (unlikely(!target_vcpu))
-		/* guest bug: non existing vCPU is a target of this IPI*/
 		return 0;
 
 	target_vcpu->arch.apic->irr_pending = true;
-- 
2.37.2.672.g94769d06f0-goog

On Wed, 2022-08-31 at 00:34 +0000, Sean Christopherson wrote:
> Document that AVIC is inhibited if any vCPU's APIC ID diverges from its
> vCPU ID, i.e. that there's no need to check for a destination match in
> the AVIC kick fast path.
> 
> Opportunistically tweak comments to remove "guest bug", as that suggests
> KVM is punting on error handling, which is not the case.  Targeting a
> non-existent vCPU or no vCPUs _may_ be a guest software bug, but whether
> or not it's a guest bug is irrelevant.  Such behavior is architecturally
> legal and thus needs to faithfully emulated by KVM (and it is).

I don't want to pick a fight, but personally these things *are* guest bugs / improper usage of APIC,
and I don't think it is wrong to document them as such.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>


> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/svm/avic.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 05a1cde8175c..3959d4766911 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -380,8 +380,8 @@ static int avic_kick_target_vcpus_fast(struct kvm *kvm, struct kvm_lapic *source
>  			cluster = (dest >> 4) << 2;
>  		}
>  
> +		/* Nothing to do if there are no destinations in the cluster. */
>  		if (unlikely(!bitmap))
> -			/* guest bug: nobody to send the logical interrupt to */
>  			return 0;
>  
>  		if (!is_power_of_2(bitmap))
> @@ -399,7 +399,7 @@ static int avic_kick_target_vcpus_fast(struct kvm *kvm, struct kvm_lapic *source
>  			if (WARN_ON_ONCE(index != logid_index))
>  				return -EINVAL;
>  
> -			/* guest bug: non existing/reserved logical destination */
> +			/* Nothing to do if the logical destination is invalid. */
>  			if (unlikely(!(logid_entry & AVIC_LOGICAL_ID_ENTRY_VALID_MASK)))
>  				return 0;
>  
> @@ -418,9 +418,13 @@ static int avic_kick_target_vcpus_fast(struct kvm *kvm, struct kvm_lapic *source
>  		}
>  	}
>  
> +	/*
> +	 * KVM inhibits AVIC if any vCPU ID diverges from the vCPUs APIC ID,
> +	 * i.e. APIC ID == vCPU ID.  Once again, nothing to do if the target
> +	 * vCPU doesn't exist.
> +	 */
>  	target_vcpu = kvm_get_vcpu_by_id(kvm, l1_physical_id);
>  	if (unlikely(!target_vcpu))
> -		/* guest bug: non existing vCPU is a target of this IPI*/
>  		return 0;
>  
>  	target_vcpu->arch.apic->irr_pending = true;

On Wed, Aug 31, 2022, Maxim Levitsky wrote:
> On Wed, 2022-08-31 at 00:34 +0000, Sean Christopherson wrote:
> > Document that AVIC is inhibited if any vCPU's APIC ID diverges from its
> > vCPU ID, i.e. that there's no need to check for a destination match in
> > the AVIC kick fast path.
> > 
> > Opportunistically tweak comments to remove "guest bug", as that suggests
> > KVM is punting on error handling, which is not the case.  Targeting a
> > non-existent vCPU or no vCPUs _may_ be a guest software bug, but whether
> > or not it's a guest bug is irrelevant.  Such behavior is architecturally
> > legal and thus needs to faithfully emulated by KVM (and it is).
> 
> I don't want to pick a fight,

Please don't hesitate to push back, I would much rather discuss points of contention
than have an ongoing, silent battle through patches.

> but personally these things *are* guest bugs / improper usage of APIC, and I
> don't think it is wrong to document them as such.

If the guest is intentionally exercising an edge case, e.g. KUT or selftests, then
from the guest's perspective its code/behavior isn't buggy.

I completely agree that abusing/aliasing logical IDs is improper usage and arguably
out of spec, but the scenarios here are very much in spec, e.g. a bitmap of '0'
isn't expressly forbidden and both Intel and AMD specs very clearly state that
APICs discard interrupt messages if they are not the destination.

But that's somewhat beside the point, as I have no objection to documenting scenarios
that are out-of-spec or undefined.  What I object to is documenting such scenarios as
"guest bugs".  If the CPU/APIC/whatever behavior is undefined, then document it
as undefined.  Saying "guest bug" doesn't help future readers understand what is
architecturally supposed to happen, whereas a comment like

	/*
	 * Logical IDs are architecturally "required" to be unique, i.e. this is
	 * technically undefined behavior.  Disable the optimized logical map so
	 * that KVM is consistent with itself, as the non-optimized matching
	 * logic with accept interrupts on all CPUs with the logical ID.
	 */

anchors KVM's behavior to the specs and explains why KVM does XYZ in response to
undefined behavior.

I feel very strongly about "guest bug" because KVM has a history of disregarding
architectural correctness and using a "good enough" approach.  Simply stating
"guest bug" makes it all the more difficult to differentiate between KVM handling
architecturally undefined behavior, versus KVM deviating from the spec because
someone decided that KVM's partial emulation/virtualziation was "good enough".

On Wed, 2022-08-31 at 16:16 +0000, Sean Christopherson wrote:
> On Wed, Aug 31, 2022, Maxim Levitsky wrote:
> > On Wed, 2022-08-31 at 00:34 +0000, Sean Christopherson wrote:
> > > Document that AVIC is inhibited if any vCPU's APIC ID diverges from its
> > > vCPU ID, i.e. that there's no need to check for a destination match in
> > > the AVIC kick fast path.
> > > 
> > > Opportunistically tweak comments to remove "guest bug", as that suggests
> > > KVM is punting on error handling, which is not the case.  Targeting a
> > > non-existent vCPU or no vCPUs _may_ be a guest software bug, but whether
> > > or not it's a guest bug is irrelevant.  Such behavior is architecturally
> > > legal and thus needs to faithfully emulated by KVM (and it is).
> > 
> > I don't want to pick a fight,
> 
> Please don't hesitate to push back, I would much rather discuss points of contention
> than have an ongoing, silent battle through patches.
> 
> > but personally these things *are* guest bugs / improper usage of APIC, and I
> > don't think it is wrong to document them as such.
> 
> If the guest is intentionally exercising an edge case, e.g. KUT or selftests, then
> from the guest's perspective its code/behavior isn't buggy.
> 
> I completely agree that abusing/aliasing logical IDs is improper usage and arguably
> out of spec, but the scenarios here are very much in spec, e.g. a bitmap of '0'
> isn't expressly forbidden and both Intel and AMD specs very clearly state that
> APICs discard interrupt messages if they are not the destination.
> 
> But that's somewhat beside the point, as I have no objection to documenting scenarios
> that are out-of-spec or undefined.  What I object to is documenting such scenarios as
> "guest bugs".  If the CPU/APIC/whatever behavior is undefined, then document it
> as undefined.  Saying "guest bug" doesn't help future readers understand what is
> architecturally supposed to happen, whereas a comment like
> 
> 	/*
> 	 * Logical IDs are architecturally "required" to be unique, i.e. this is
> 	 * technically undefined behavior.  Disable the optimized logical map so
> 	 * that KVM is consistent with itself, as the non-optimized matching
> 	 * logic with accept interrupts on all CPUs with the logical ID.
> 	 */
> 
> anchors KVM's behavior to the specs and explains why KVM does XYZ in response to
> undefined behavior.
> 
> I feel very strongly about "guest bug" because KVM has a history of disregarding
> architectural correctness and using a "good enough" approach.  Simply stating
> "guest bug" makes it all the more difficult to differentiate between KVM handling
> architecturally undefined behavior, versus KVM deviating from the spec because
> someone decided that KVM's partial emulation/virtualziation was "good enough".
> 

All right, I agree with you.

Best regards,
	Maxim Levitsky