Re: [RFC PATCH v2 1/5] iommu/amd: kvm/svm: Improve API between SVM and AMD IOMMU

[Sairaj Kodilkar]

On 7/8/2026 7:06 PM, Sean Christopherson wrote:
> 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.
> 

Sorry for inconvenience, will split the patch and improve the verbosity
of all the commits.

> 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 <sarunkod@xxxxxxx>
>> ---
>>  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.
> 

I needed an generic name for boolean that is being used by both GAPPI
and GALOG, hence I replaced ga_log_intr with posted_intr. But I can
understand that it can lead to confusion. How about replacing
ga_log_intr with wakeup_intr and AVIC_PHYSICAL_ID_ENTRY_GA_LOG_INTR with
AVIC_PHYSICAL_ID_ENTRY_WAKEUP_INTR

Thanks
Sairaj

>> +			pi_data.flags = posted_intr << AMD_IOMMU_FLAG_POSTED_INTR_SHIFT;