Message-ID: <7dd1e7d1510f17f1140b7174dd42fed752eefc38.camel@redhat.com>
Subject: Re: [PATCH v2 3/3] KVM: SVM: Extend host physical APIC ID field to
 support more than 8-bit
From:   Maxim Levitsky <mlevitsk@redhat.com>
To:     Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org, x86@kernel.org
Cc:     pbonzini@redhat.com, joro@8bytes.org, seanjc@google.com,
        tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
        peterz@infradead.org, hpa@zytor.com, thomas.lendacky@amd.com,
        jon.grimm@amd.com
Date:   Fri, 03 Dec 2021 09:46:15 +0200
In-Reply-To: <20211202235825.12562-4-suravee.suthikulpanit@amd.com>
References: <20211202235825.12562-1-suravee.suthikulpanit@amd.com>
         <20211202235825.12562-4-suravee.suthikulpanit@amd.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 2021-12-02 at 17:58 -0600, Suravee Suthikulpanit wrote:
> The AVIC physical APIC ID table entry contains the host physical
> APIC ID field, which the hardware uses to keep track of where each
> vCPU is running. Originally, the field is an 8-bit value, which can
> only support physical APIC ID up to 255.
> 
> To support system with larger APIC ID, the AVIC hardware extends
> this field to support up to the largest possible physical APIC ID
> available on the system.
> 
> Therefore, replace the hard-coded mask value with the value
> calculated from the maximum possible physical APIC ID in the system.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/kvm/svm/avic.c | 28 ++++++++++++++++++++--------
>  arch/x86/kvm/svm/svm.h  |  1 -
>  2 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 6aca1682f4b7..2a0f58e6edf5 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -19,6 +19,7 @@
>  #include <linux/amd-iommu.h>
>  #include <linux/kvm_host.h>
>  
> +#include <asm/apic.h>
>  #include <asm/irq_remapping.h>
>  
>  #include "trace.h"
> @@ -63,6 +64,7 @@
>  static DEFINE_HASHTABLE(svm_vm_data_hash, SVM_VM_DATA_HASH_BITS);
>  static u32 next_vm_id = 0;
>  static bool next_vm_id_wrapped = 0;
> +static u64 avic_host_physical_id_mask;
>  static DEFINE_SPINLOCK(svm_vm_data_hash_lock);
>  
>  /*
> @@ -133,6 +135,20 @@ void avic_vm_destroy(struct kvm *kvm)
>  	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
>  }
>  
> +static void avic_init_host_physical_apicid_mask(void)
> +{
> +	if (!x2apic_mode) {
Wonder why this is a exported  global variable and not function.
Not the patch fault though.
> +		/* If host is in xAPIC mode, default to only 8-bit mask. */
> +		avic_host_physical_id_mask = 0xffULL;
> +	} else {
> +		u32 count = get_count_order(apic_get_max_phys_apicid());
> +
> +		avic_host_physical_id_mask = BIT(count) - 1;
I think that there were some complains about using this macro and instead encouraged
to use 1 << x directly, but I see it used already in other places in avic.c so I don't know.


> +	}
> +	pr_debug("Using AVIC host physical APIC ID mask %#0llx\n",
> +		 avic_host_physical_id_mask);
> +}
> +
>  int avic_vm_init(struct kvm *kvm)
>  {
>  	unsigned long flags;
> @@ -943,22 +959,17 @@ avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int cpu, bool r)
>  void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  {
>  	u64 entry;
> -	/* ID = 0xff (broadcast), ID > 0xff (reserved) */
>  	int h_physical_id = kvm_cpu_get_apicid(cpu);
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  
> -	/*
> -	 * Since the host physical APIC id is 8 bits,
> -	 * we can support host APIC ID upto 255.
> -	 */
> -	if (WARN_ON(h_physical_id > AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK))
> +	if (WARN_ON(h_physical_id > avic_host_physical_id_mask))
>  		return;
>  
>  	entry = READ_ONCE(*(svm->avic_physical_id_cache));
>  	WARN_ON(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK);
>  
> -	entry &= ~AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
> -	entry |= (h_physical_id & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK);
> +	entry &= ~avic_host_physical_id_mask;
> +	entry |= h_physical_id;
>  
>  	entry &= ~AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK;
>  	if (svm->avic_is_running)
> @@ -1018,6 +1029,7 @@ bool avic_hardware_setup(bool avic, bool npt)
>  		return false;
>  
>  	pr_info("AVIC enabled\n");
> +	avic_init_host_physical_apicid_mask();
>  	amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
>  	return true;
>  }
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 1d2d72e56dd1..b4cb71c538b3 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -497,7 +497,6 @@ extern struct kvm_x86_nested_ops svm_nested_ops;
>  #define AVIC_LOGICAL_ID_ENTRY_VALID_BIT			31
>  #define AVIC_LOGICAL_ID_ENTRY_VALID_MASK		(1 << 31)
>  
> -#define AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK	(0xFFULL)
>  #define AVIC_PHYSICAL_ID_ENTRY_BACKING_PAGE_MASK	(0xFFFFFFFFFFULL << 12)
>  #define AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK		(1ULL << 62)
>  #define AVIC_PHYSICAL_ID_ENTRY_VALID_MASK		(1ULL << 63)


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

Best regards,
	Maxim Levitsky