Date:   Tue, 7 Feb 2023 09:33:50 +0100
From:   Igor Mammedov <imammedo@redhat.com>
To:     Sean Christopherson <seanjc@google.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Alejandro Jimenez <alejandro.j.jimenez@oracle.com>,
        Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Subject: Re: [PATCH v2 2/3] KVM: SVM: Modify AVIC GATag to support max
 number of 512 vCPUs
Message-ID: <20230207093350.5db155ca@imammedo.users.ipa.redhat.com>
In-Reply-To: <20230207002156.521736-3-seanjc@google.com>
References: <20230207002156.521736-1-seanjc@google.com>
        <20230207002156.521736-3-seanjc@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue,  7 Feb 2023 00:21:55 +0000
Sean Christopherson <seanjc@google.com> wrote:

> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> Define AVIC_VCPU_ID_MASK based on AVIC_PHYSICAL_MAX_INDEX, i.e. the mask
> that effectively controls the largest guest physical APIC ID supported by
> x2AVIC, instead of hardcoding the number of bits to 8 (and the number of
> VM bits to 24).

Is there any particular reason not to tie it to max supported by KVM
KVM_MAX_VCPU_IDS?

Another question:
 will guest fail to start when configured with more than 512 vCPUs
 or it will start broken?

> 
> The AVIC GATag is programmed into the AMD IOMMU IRTE to provide a
> reference back to KVM in case the IOMMU cannot inject an interrupt into a
> non-running vCPU.  In such a case, the IOMMU notifies software by creating
> a GALog entry with the corresponded GATag, and KVM then uses the GATag to
> find the correct VM+vCPU to kick.  Dropping bit 8 from the GATag results
> in kicking the wrong vCPU when targeting vCPUs with x2APIC ID > 255.
> 
> Fixes: 4d1d7942e36a ("KVM: SVM: Introduce logic to (de)activate x2AVIC mode")
> Cc: stable@vger.kernel.org
> Reported-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Co-developed-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/svm/avic.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index ca684979e90d..326341a22153 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -27,19 +27,29 @@
>  #include "irq.h"
>  #include "svm.h"
>  
> -/* AVIC GATAG is encoded using VM and VCPU IDs */
> -#define AVIC_VCPU_ID_BITS		8
> -#define AVIC_VCPU_ID_MASK		((1 << AVIC_VCPU_ID_BITS) - 1)
> +/*
> + * Encode the arbitrary VM ID and the vCPU's default APIC ID, i.e the vCPU ID,
> + * into the GATag so that KVM can retrieve the correct vCPU from a GALog entry
> + * if an interrupt can't be delivered, e.g. because the vCPU isn't running.
> + *
> + * For the vCPU ID, use however many bits are currently allowed for the max
> + * guest physical APIC ID (limited by the size of the physical ID table), and
> + * use whatever bits remain to assign arbitrary AVIC IDs to VMs.  Note, the
> + * size of the GATag is defined by hardware (32 bits), but is an opaque value
> + * as far as hardware is concerned.
> + */
> +#define AVIC_VCPU_ID_MASK		AVIC_PHYSICAL_MAX_INDEX_MASK
>  
> -#define AVIC_VM_ID_BITS			24
> -#define AVIC_VM_ID_NR			(1 << AVIC_VM_ID_BITS)
> -#define AVIC_VM_ID_MASK			((1 << AVIC_VM_ID_BITS) - 1)
> +#define AVIC_VM_ID_SHIFT		HWEIGHT32(AVIC_PHYSICAL_MAX_INDEX_MASK)
> +#define AVIC_VM_ID_MASK			(GENMASK(31, AVIC_VM_ID_SHIFT) >> AVIC_VM_ID_SHIFT)
>  
> -#define AVIC_GATAG(x, y)		(((x & AVIC_VM_ID_MASK) << AVIC_VCPU_ID_BITS) | \
> +#define AVIC_GATAG(x, y)		(((x & AVIC_VM_ID_MASK) << AVIC_VM_ID_SHIFT) | \
>  						(y & AVIC_VCPU_ID_MASK))
> -#define AVIC_GATAG_TO_VMID(x)		((x >> AVIC_VCPU_ID_BITS) & AVIC_VM_ID_MASK)
> +#define AVIC_GATAG_TO_VMID(x)		((x >> AVIC_VM_ID_SHIFT) & AVIC_VM_ID_MASK)
>  #define AVIC_GATAG_TO_VCPUID(x)		(x & AVIC_VCPU_ID_MASK)
>  
> +static_assert(AVIC_GATAG(AVIC_VM_ID_MASK, AVIC_VCPU_ID_MASK) == -1u);
> +
>  static bool force_avic;
>  module_param_unsafe(force_avic, bool, 0444);
>