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

[Sean Christopherson <seanjc@google.com>]

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 <sarunkod@amd.com>
> ---
>  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;