Re: [PATCH 2/2] KVM: SVM: Drop unnecessary avic_vm_destroy() call on init failure

[Naveen N Rao]

On Mon, Jun 29, 2026 at 05:17:17PM -0700, Sean Christopherson wrote:
> On Mon, Jun 29, 2026, Naveen N Rao wrote:
> > On Thu, Jun 25, 2026 at 03:09:33PM -0700, Sean Christopherson wrote:
> > > Don't bother calling avic_vm_destroy() when allocating the logical ID table
> > > fails, as there is nothing to clean up now that the physical ID table is
> > > allocated later, on-demand at first vCPU creation.
> > > 
> > > For all intents and purposes, no functional change intended, as calling
> > > avic_vm_destroy() is a big nop in this case.
> > > 
> > > Fixes: 54ffe74cc4ab ("KVM: SVM: Move AVIC Physical ID table allocation to vcpu_precreate()")
> > > Cc: Naveen N Rao (AMD) <naveen@xxxxxxxxxx>
> > > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > > ---
> > >  arch/x86/kvm/svm/avic.c | 7 +------
> > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > 
> > This LGTM, though I'm not sure this deserves a fixes tag.
> 
> Yeah, it's borderline.  Because KVM (x86) doesn't do AUTOSEL, i.e. requires an
> explicit "Cc: stable@xxxxxxxxxxxxxxx" for the vast majority of cases, we can be
> very liberal with Fixes.  And so I like to use Fixes to create a paper trail for
> anything where a commit did something that would have necessitate a new version
> of the patch, had it been caught in code review.

Ah, that's the reason for preferring Fixes: tag over Cc: stable, got it.

> 
> > FWIW, I have had a patch (part of a larger series I am going to post 
> > soon) to move the logical_id_table allocation alongside the physical ID 
> > table allocation. I'm fine if you want to queue your patch first, but 
> > this is what I have locally:
> 
> Ah shoot, I should have read this mail earlier.  While floundering around, trying
> to figure out how to deal with kvm_arch_pre_destroy_vm() not being called when
> VM creation fails after kvm_arch_init_vm(), I ended up with the same idea, though
> I moved *all* of avic_vm_init() to the vCPU-precreate phase.  Because what I
> needed was to defer adding the VM to the GA Log list until the VM is fully
> created.
> 
> > @@ -303,10 +303,16 @@ int avic_alloc_physical_id_table(struct kvm *kvm)
> >  	if (kvm_svm->avic_physical_id_table)
> >  		return 0;
> >  
> > +	kvm_svm->avic_logical_id_table = (void *)get_zeroed_page(GFP_KERNEL_ACCOUNT);
> > +	if (!kvm_svm->avic_logical_id_table)
> > +		return -ENOMEM;
> > +
> >  	kvm_svm->avic_physical_id_table = (void *)__get_free_pages(GFP_KERNEL_ACCOUNT | __GFP_ZERO,
> >  								   avic_get_physical_id_table_order(kvm));
> > -	if (!kvm_svm->avic_physical_id_table)
> > +	if (!kvm_svm->avic_physical_id_table) {
> > +		free_page((unsigned long)kvm_svm->avic_logical_id_table);
> 
> Freeing the page on failure is "wrong".  To keep vCPU creation idempotent from
> userspace's perspective (well, not truly idempotent, but close-ish), KVM
> deliberately doesn't unwind if .vcpu_precreate() fails.  E.g. so that if
> allocating the tables succeeds, but a later stage of vCPU creation fails,
> subsequent calls to create vCPUs won't fail at the earlier stage.
> 
> The nice thing is that since the VM has already been created, there's no need to
> every unwind on failure, because it's no different than if KVM had successfully
> allocate the assets during VM creation.

Understood - I see there is a comment about this in 
kvm_arch_vcpu_precreate() which I missed. Thanks for the details!


- Naveen