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: <aHplblJxJ-7FuaTH@google.com>
Date: Fri, 18 Jul 2025 08:17:02 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Naveen N Rao <naveen@...nel.org>
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, 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));
}

> > > +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.