linux-kernel - Re: [PATCH v3 2/2] KVM: SVM: Limit AVIC physical max index based on configured max_vcpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ap7276xfys3zweihirs6xnrvd53u3zlndqghju4sckysi53sli@r27e6jlo2kdw>
Date: Mon, 21 Jul 2025 19:42:24 +0530
From: Naveen N Rao <naveen@...nel.org>
To: Sean Christopherson <seanjc@...gle.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Paolo Bonzini <pbonzini@...hat.com>, Suravee Suthikulpanit <suravee.suthikulpanit@....com>, 
	Vasant Hegde <vasant.hegde@....com>
Subject: Re: [PATCH v3 2/2] KVM: SVM: Limit AVIC physical max index based on
 configured max_vcpu_ids

On Fri, Jul 18, 2025 at 08:17:02AM -0700, Sean Christopherson wrote:
> On Fri, Jul 18, 2025, Naveen N Rao wrote:
> > On Mon, Jun 23, 2025 at 05:38:23PM -0700, Sean Christopherson wrote:
> > > > @@ -103,7 +114,7 @@ static void avic_activate_vmcb(struct vcpu_svm *svm)
> > > >  	 */
> > > >  	if (x2avic_enabled && apic_x2apic_mode(svm->vcpu.arch.apic)) {
> > > >  		vmcb->control.int_ctl |= X2APIC_MODE_MASK;
> > > > -		vmcb->control.avic_physical_id |= x2avic_max_physical_id;
> > > > +		vmcb->control.avic_physical_id |= avic_get_max_physical_id(svm->vcpu.kvm, true);
> > > 
> > > Don't pass hardcoded booleans when it is at all possible to do something else.
> > > For this case, I would either do:
> > > 
> > >   static u32 avic_get_max_physical_id(struct kvm_vcpu *vcpu)
> > >   {
> > > 	u32 arch_max;
> > > 	
> > > 	if (x2avic_enabled && apic_x2apic_mode(vcpu->arch.apic))
> > 
> > This won't work since we want to use this during vCPU init and at that 
> > point, I don't think we can rely on the vCPU x2APIC mode to decide the 
> > size of the AVIC physical ID table.
> 
> Ah, I missed that this is used by avic_get_physical_id_table_order().  How about
> this?
> 
> static u32 __avic_get_max_physical_id(struct kvm *kvm, struct kvm_vcpu *vcpu)
> {
> 	u32 arch_max;
> 
> 	if (x2avic_enabled && (!vcpu || apic_x2apic_mode(vcpu->arch.apic)))
> 		arch_max = x2avic_max_physical_id;
> 	else
> 		arch_max = AVIC_MAX_PHYSICAL_ID;
> 
> 	return min(kvm->arch.max_vcpu_ids - 1, arch_max);
> }
> 
> static u32 avic_get_max_physical_id(struct kvm_vcpu *vcpu)
> {
> 	return __avic_get_max_physical_id(vcpu->kvm, vcpu);
> }
> 
> static void avic_activate_vmcb(struct vcpu_svm *svm)
> {
> 	struct vmcb *vmcb = svm->vmcb01.ptr;
> 	struct kvm_vcpu *vcpu = &svm->vcpu;
> 
> 	vmcb->control.int_ctl &= ~(AVIC_ENABLE_MASK | X2APIC_MODE_MASK);
> 
> 	vmcb->control.avic_physical_id &= ~AVIC_PHYSICAL_MAX_INDEX_MASK;
> 	vmcb->control.avic_physical_id |= avic_get_max_physical_id(vcpu);
> 
> 	vmcb->control.int_ctl |= AVIC_ENABLE_MASK;
> 
> 	/*
> 	 * Note: KVM supports hybrid-AVIC mode, where KVM emulates x2APIC MSR
> 	 * accesses, while interrupt injection to a running vCPU can be
> 	 * achieved using AVIC doorbell.  KVM disables the APIC access page
> 	 * (deletes the memslot) if any vCPU has x2APIC enabled, thus enabling
> 	 * AVIC in hybrid mode activates only the doorbell mechanism.
> 	 */
> 	if (x2avic_enabled && apic_x2apic_mode(vcpu->arch.apic)) {
> 		vmcb->control.int_ctl |= X2APIC_MODE_MASK;
> 		/* Disabling MSR intercept for x2APIC registers */
> 		svm_set_x2apic_msr_interception(svm, false);
> 	} else {
> 		/*
> 		 * Flush the TLB, the guest may have inserted a non-APIC
> 		 * mapping into the TLB while AVIC was disabled.
> 		 */
> 		kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
> 
> 		/* Enabling MSR intercept for x2APIC registers */
> 		svm_set_x2apic_msr_interception(svm, true);
> 	}
> }
> 
> static int avic_get_physical_id_table_order(struct kvm *kvm)
> {
> 	/* Limit to the maximum physical ID supported in x2avic mode */
> 	return get_order((__avic_get_max_physical_id(kvm, NULL) + 1) * sizeof(u64));
> }

This LGTM, I will incorporate this.

> 
> > > > +static int svm_vcpu_precreate(struct kvm *kvm)
> > > > +{
> > > > +	return avic_alloc_physical_id_table(kvm);
> > > 
> > > Why is allocation being moved to svm_vcpu_precreate()?
> > 
> > This is because we want KVM_CAP_MAX_VCPU_ID to have been invoked by the 
> > VMM, and that is guaranteed to be set by the time the first vCPU is 
> > created. We restrict the AVIC physical ID table based on the maximum 
> > number of vCPUs set by the VMM.
> > 
> > This mirrors how Intel VMX uses this capability. I will call this out 
> > explicitly in the commit log.
> 
> Do it as a separate patch.  Then you pretty much *have* to write a changelog,
> and the changes that are specific to 4k support are even more isolated.

Sure.


Thanks,
Naveen