From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f201.google.com (mail-pf1-f201.google.com [209.85.210.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 27FE6377EA9 for ; Wed, 8 Jul 2026 13:36:30 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783517793; cv=none; b=OfNKIrDgc30aBky86E2xwcM6zPvrgjn78DdMle6NB1e+/D6BCXnWTpKxqzxx1Ueft1uBZfw0pVQpy4OCwa646eLxani2b9rQVzSTXV+1Sp5kHlHWGk1YQ71j3sfoEgygfngfjHyE1qloumlI+rbWATbIhGn357JUeualdHFAERI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783517793; c=relaxed/simple; bh=SC3W3f8odNqmLPQ5jk9l0PDzMsaA9uPQMtpPxXIHo10=; h=Date:In-Reply-To:Mime-Version:References:Message-ID:Subject:From: To:Cc:Content-Type; b=c654DhlVT8BgJaUjJQjAiWh1kC4yra8IN5/jEvf8H+zAt8245yfyD917S15SU1mrR+j0wKhddpAWJLDmdrpygX2hnwzArA8Q2wtHl7iWS7dOtSAo5aY5A8HHb279ITeOAXtMRjjN7Qv8oFiEQUnWGk3NbBMVcOASdmWzZqn3dxM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=flex--seanjc.bounces.google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=SEqIyxfY; arc=none smtp.client-ip=209.85.210.201 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=flex--seanjc.bounces.google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="SEqIyxfY" Received: by mail-pf1-f201.google.com with SMTP id d2e1a72fcca58-848452dd146so1803362b3a.2 for ; Wed, 08 Jul 2026 06:36:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20251104; t=1783517790; x=1784122590; darn=vger.kernel.org; h=content-type:cc:to:from:subject:message-id:references:mime-version :in-reply-to:date:from:to:cc:subject:date:message-id:reply-to :content-type; bh=BsuMXMKCfagFyuFxdoKczu47VXQMXKuH3Dpr7pHjR+w=; b=SEqIyxfYDx44zaMEheELIsRfB7Ck+g3lROg0RtzqAZCDAkZXdl2Azs8TMldfBRNQ+z 3fOEs6nQGoIMiUm/zgctLXwpPx64Kzp2RZwJ/5InDvmFa4Dz3ZnAdZ/ed11oEMrug44v WTv8X6b38NBU58r46QpzDDHI/z53vboNx6HXFwhtVsCoZ+YK4gdRLys3e2eVp1NzM51a hs4G3WSX9jEZjhUNyQQzuKseAAT26jgd55sXL5V53j/Wjntx5Tn1d4efnCN4Rgio9gK+ 1J1ZCtNyvlhiJyBsdQPxkxHCtfFL5Dszgn5ltp/BoqsBJraLNV//RJZGHwo8+Jhyw/nU soqQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1783517790; x=1784122590; h=content-type:cc:to:from:subject:message-id:references:mime-version :in-reply-to:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to:content-type; bh=BsuMXMKCfagFyuFxdoKczu47VXQMXKuH3Dpr7pHjR+w=; b=f2FkWqLD06kMwNQl4VaMGPYR9E7KbzT/RLrAKVbxOurCnpeZBdQjGBWJErrnwaFFIM DD7bzkUtL0xeqVhqdvUsY5reiFigx5iw+0xsf3qAfmH7XocJLMZirtHNElp0YKu5mnN8 wcvJXUkGsCFMgQmQlxohcva4xT6pKcGFpqxv9+F2+0ZapNLplGotBn7A9V5PcgECZHZp 6pfpNmh05kLLkkgNX1+1h7RyMp8ae/Nr0i73Bce/ipD3oZXa9KeRDWmiATzaOkRdpGVm HrirQef1x3JSfQF+Sq9auz9vkPGIFQwIswqp4rWAnagSAWL5/wf7zDH/m2OZhAfoFtRL aYyQ== X-Forwarded-Encrypted: i=1; AHgh+RqDKrO67UOffhjRL7YqWtEdZjDAXw8Zj7CGh4w8HLfeWqOAu7OwJU6zI4B67/o+50G5WtweOfHJBQO/wzE=@vger.kernel.org X-Gm-Message-State: AOJu0YzveGSxSi4YfpXGL45afYbfBKkXG2VVrtSgKQ1qLorFnPKJ4gif MXocMqatbglJ4IwVwMBJI3n4voDh/Ldu61gUOLiNYLEqRU0sVnqnoxVMcdvw0EEcUx6iCpuaNW7 gMSpm7Q== X-Received: from pfuw2.prod.google.com ([2002:a05:6a00:14c2:b0:847:7ffd:ce5e]) (user=seanjc job=prod-delivery.src-stubby-dispatcher) by 2002:a05:6a00:12d8:b0:845:cf86:9469 with SMTP id d2e1a72fcca58-8484327c8a2mr2415200b3a.29.1783517790268; Wed, 08 Jul 2026 06:36:30 -0700 (PDT) Date: Wed, 8 Jul 2026 06:36:29 -0700 In-Reply-To: <20260708091408.12106-2-sarunkod@amd.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 References: <20260708091408.12106-1-sarunkod@amd.com> <20260708091408.12106-2-sarunkod@amd.com> Message-ID: Subject: Re: [RFC PATCH v2 1/5] iommu/amd: kvm/svm: Improve API between SVM and AMD IOMMU From: Sean Christopherson To: Sairaj Kodilkar Cc: "H. Peter Anvin" , "Joerg Roedel (AMD)" , Borislav Petkov , Dave Hansen , Ingo Molnar , Paolo Bonzini , Robin Murphy , Suravee Suthikulpanit , Thomas Gleixner , Vasant Hegde , Will Deacon , iommu@lists.linux.dev, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, x86@kernel.org Content-Type: text/plain; charset="us-ascii" This should be two separate patches, one to rename the @cpu param/field, one to replace @ga_log_intr with flags. "Improve XYZ" is the equilavent of a do_work() function. And I'm having a hell of a time trying to review this because of the overlapping churn. Actually, if you insist on a "flags" field, three patches: rename, convert to flags, add the dedicated RUNNING flag. On Wed, Jul 08, 2026, Sairaj Kodilkar wrote: > The name "cpu" in the parameter is misleading as it represents the > physical apicid of the cpu. Hence rename it. Hrm, I was going to suggest having KVM pass in the actual CPU instead of the APIC ID, because the fact that the IRTE takes an APIC ID is an implementation detail that would ideally not bleed into the API. But unfortunately KVM uses the entry in the physical ID table when filling amd_iommu_pi_data.cpu, so taking the APIC ID probably is the least awful approach. :-/ > Introduce flags to determine the state of the vCPU (running or not) and > posted interrupts. > This is useful as following patch overloads the apicid (formerly cpu) field > to determine GAPPI destination when vCPU is not running and it can no longer > be used to determine if vCPU is running or not. Rewrite the GAPPI behavior with --verbose to explain *why*. The only reason I could suss out the why is because I already know how Intel posted interrupts work and so could intuit why GAPPI would want to keep the APIC ID valid. Without that knowledge, this is unreviewable. > Signed-off-by: Sairaj Kodilkar > --- > arch/x86/include/asm/irq_remapping.h | 4 ++-- > arch/x86/kvm/svm/avic.c | 22 ++++++++++++++-------- > drivers/iommu/amd/iommu.c | 24 ++++++++++++------------ > include/linux/amd-iommu.h | 14 ++++++++++---- > 4 files changed, 38 insertions(+), 26 deletions(-) > > diff --git a/arch/x86/include/asm/irq_remapping.h b/arch/x86/include/asm/irq_remapping.h > index 37b94f484ef3..40a2206a33c0 100644 > --- a/arch/x86/include/asm/irq_remapping.h > +++ b/arch/x86/include/asm/irq_remapping.h > @@ -35,8 +35,8 @@ struct amd_iommu_pi_data { > u64 vapic_addr; /* Physical address of the vCPU's vAPIC. */ > u32 ga_tag; > u32 vector; /* Guest vector of the interrupt */ > - int cpu; > - bool ga_log_intr; > + int apicid; > + int flags; Why use flags? This is on-stack data, packing booleans into flags adds no value IMO, and makes the code way harder to read. > bool is_guest_mode; > void *ir_data; > }; > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c > index cdd5a6dc646f..7862b13c5409 100644 > --- a/arch/x86/kvm/svm/avic.c > +++ b/arch/x86/kvm/svm/avic.c > @@ -932,6 +932,7 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm, > struct vcpu_svm *svm = to_svm(vcpu); > u64 entry; > int ret; > + int posted_intr; > > /* > * Prevent the vCPU from being scheduled out or migrated until > @@ -949,10 +950,11 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm, > */ > entry = svm->avic_physical_id_entry; > if (entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK) { > - pi_data.cpu = entry & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK; > + pi_data.apicid = entry & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK; > + pi_data.flags = AMD_IOMMU_FLAG_VCPU_RUNNING; > } else { > - pi_data.cpu = -1; > - pi_data.ga_log_intr = entry & AVIC_PHYSICAL_ID_ENTRY_GA_LOG_INTR; > + posted_intr = !!(entry & AVIC_PHYSICAL_ID_ENTRY_GA_LOG_INTR); "posted_intr" is misleading, and IMO flat out wrong. Even when a GA Log Intr and notification is logged/sent, the virtual interrupt is still posted to the vCPU's virtual APIC. I have no idea what you're trying to capture with this code. And that is yet another reason why this patch needs to be split up: so that you can explain the reasoning and logic behind this refactoring. > + pi_data.flags = posted_intr << AMD_IOMMU_FLAG_POSTED_INTR_SHIFT;