WARN if KVM attempts to set vCPU affinity when posted interrupts aren't
enabled, as KVM shouldn't try to enable posting when they're unsupported,
and the IOMMU driver darn well should only advertise posting support when
AMD_IOMMU_GUEST_IR_VAPIC() is true.

Note, KVM consumes is_guest_mode only on success.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 drivers/iommu/amd/iommu.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index b3a01b7757ee..4f69a37cf143 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -3852,19 +3852,12 @@ static int amd_ir_set_vcpu_affinity(struct irq_data *data, void *vcpu_info)
 	if (!dev_data || !dev_data->use_vapic)
 		return -EINVAL;
 
+	if (WARN_ON_ONCE(!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)))
+		return -EINVAL;
+
 	ir_data->cfg = irqd_cfg(data);
 	pi_data->ir_data = ir_data;
 
-	/* Note:
-	 * SVM tries to set up for VAPIC mode, but we are in
-	 * legacy mode. So, we force legacy mode instead.
-	 */
-	if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)) {
-		pr_debug("%s: Fall back to using intr legacy remap\n",
-			 __func__);
-		pi_data->is_guest_mode = false;
-	}
-
 	pi_data->prev_ga_tag = ir_data->cached_ga_tag;
 	if (pi_data->is_guest_mode) {
 		ir_data->ga_root_ptr = (pi_data->base >> 12);
-- 
2.49.0.504.g3bcea36a83-goog

On 4/5/2025 1:08 AM, Sean Christopherson wrote:
> WARN if KVM attempts to set vCPU affinity when posted interrupts aren't
> enabled, as KVM shouldn't try to enable posting when they're unsupported,
> and the IOMMU driver darn well should only advertise posting support when
> AMD_IOMMU_GUEST_IR_VAPIC() is true.
> 
> Note, KVM consumes is_guest_mode only on success.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   drivers/iommu/amd/iommu.c | 13 +++----------
>   1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index b3a01b7757ee..4f69a37cf143 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -3852,19 +3852,12 @@ static int amd_ir_set_vcpu_affinity(struct irq_data *data, void *vcpu_info)
>   	if (!dev_data || !dev_data->use_vapic)
>   		return -EINVAL;
>   
> +	if (WARN_ON_ONCE(!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)))
> +		return -EINVAL;
> +

Hi Sean,
'dev_data->use_vapic' is always zero when AMD IOMMU uses legacy
interrupts i.e. when AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) is 0.
Hence you can remove this additional check.

Regards
Sairaj Kodilkar

>   	ir_data->cfg = irqd_cfg(data);
>   	pi_data->ir_data = ir_data;
>   
> -	/* Note:
> -	 * SVM tries to set up for VAPIC mode, but we are in
> -	 * legacy mode. So, we force legacy mode instead.
> -	 */
> -	if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)) {
> -		pr_debug("%s: Fall back to using intr legacy remap\n",
> -			 __func__);
> -		pi_data->is_guest_mode = false;
> -	}
> -
>   	pi_data->prev_ga_tag = ir_data->cached_ga_tag;
>   	if (pi_data->is_guest_mode) {
>   		ir_data->ga_root_ptr = (pi_data->base >> 12);

On Fri, Apr 11, 2025, Sairaj Kodilkar wrote:
> On 4/5/2025 1:08 AM, Sean Christopherson wrote:
> > WARN if KVM attempts to set vCPU affinity when posted interrupts aren't
> > enabled, as KVM shouldn't try to enable posting when they're unsupported,
> > and the IOMMU driver darn well should only advertise posting support when
> > AMD_IOMMU_GUEST_IR_VAPIC() is true.
> > 
> > Note, KVM consumes is_guest_mode only on success.
> > 
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >   drivers/iommu/amd/iommu.c | 13 +++----------
> >   1 file changed, 3 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> > index b3a01b7757ee..4f69a37cf143 100644
> > --- a/drivers/iommu/amd/iommu.c
> > +++ b/drivers/iommu/amd/iommu.c
> > @@ -3852,19 +3852,12 @@ static int amd_ir_set_vcpu_affinity(struct irq_data *data, void *vcpu_info)
> >   	if (!dev_data || !dev_data->use_vapic)
> >   		return -EINVAL;
> > +	if (WARN_ON_ONCE(!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)))
> > +		return -EINVAL;
> > +
> 
> Hi Sean,
> 'dev_data->use_vapic' is always zero when AMD IOMMU uses legacy
> interrupts i.e. when AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) is 0.
> Hence you can remove this additional check.

Hmm, or move it above?  KVM should never call amd_ir_set_vcpu_affinity() if
IRQ posting is unsupported, and that would make this consistent with the end
behavior of amd_iommu_update_ga() and amd_iommu_{de,}activate_guest_mode().

	if (WARN_ON_ONCE(!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)))
		return -EINVAL;

	if (ir_data->iommu == NULL)
		return -EINVAL;

	dev_data = search_dev_data(ir_data->iommu, irte_info->devid);

	/* Note:
	 * This device has never been set up for guest mode.
	 * we should not modify the IRTE
	 */
	if (!dev_data || !dev_data->use_vapic)
		return -EINVAL;

I'd like to keep the WARN so that someone will notice if KVM screws up.

On 4/11/2025 7:40 PM, Sean Christopherson wrote:
> On Fri, Apr 11, 2025, Sairaj Kodilkar wrote:
>> On 4/5/2025 1:08 AM, Sean Christopherson wrote:
>>> WARN if KVM attempts to set vCPU affinity when posted interrupts aren't
>>> enabled, as KVM shouldn't try to enable posting when they're unsupported,
>>> and the IOMMU driver darn well should only advertise posting support when
>>> AMD_IOMMU_GUEST_IR_VAPIC() is true.
>>>
>>> Note, KVM consumes is_guest_mode only on success.
>>>
>>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>>> ---
>>>   drivers/iommu/amd/iommu.c | 13 +++----------
>>>   1 file changed, 3 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>>> index b3a01b7757ee..4f69a37cf143 100644
>>> --- a/drivers/iommu/amd/iommu.c
>>> +++ b/drivers/iommu/amd/iommu.c
>>> @@ -3852,19 +3852,12 @@ static int amd_ir_set_vcpu_affinity(struct irq_data *data, void *vcpu_info)
>>>   	if (!dev_data || !dev_data->use_vapic)
>>>   		return -EINVAL;
>>> +	if (WARN_ON_ONCE(!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)))
>>> +		return -EINVAL;
>>> +
>>
>> Hi Sean,
>> 'dev_data->use_vapic' is always zero when AMD IOMMU uses legacy
>> interrupts i.e. when AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) is 0.
>> Hence you can remove this additional check.
> 
> Hmm, or move it above?  KVM should never call amd_ir_set_vcpu_affinity() if
> IRQ posting is unsupported, and that would make this consistent with the end
> behavior of amd_iommu_update_ga() and amd_iommu_{de,}activate_guest_mode().
> 
> 	if (WARN_ON_ONCE(!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)))

Note that this is global IOMMU level check while dev_data->use_vapic is per
device. We set per device thing while attaching device to domain based on IOMMU
domain type and IOMMU vapic support.

How about add WARN_ON based on dev_data->use_vapic .. so that we can catch if
something went wrong in IOMMU side as well?

-Vasant

On Tue, Apr 15, 2025, Vasant Hegde wrote:
> On 4/11/2025 7:40 PM, Sean Christopherson wrote:
> > On Fri, Apr 11, 2025, Sairaj Kodilkar wrote:
> >> On 4/5/2025 1:08 AM, Sean Christopherson wrote:
> >>> WARN if KVM attempts to set vCPU affinity when posted interrupts aren't
> >>> enabled, as KVM shouldn't try to enable posting when they're unsupported,
> >>> and the IOMMU driver darn well should only advertise posting support when
> >>> AMD_IOMMU_GUEST_IR_VAPIC() is true.
> >>>
> >>> Note, KVM consumes is_guest_mode only on success.
> >>>
> >>> Signed-off-by: Sean Christopherson <seanjc@google.com>
> >>> ---
> >>>   drivers/iommu/amd/iommu.c | 13 +++----------
> >>>   1 file changed, 3 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> >>> index b3a01b7757ee..4f69a37cf143 100644
> >>> --- a/drivers/iommu/amd/iommu.c
> >>> +++ b/drivers/iommu/amd/iommu.c
> >>> @@ -3852,19 +3852,12 @@ static int amd_ir_set_vcpu_affinity(struct irq_data *data, void *vcpu_info)
> >>>   	if (!dev_data || !dev_data->use_vapic)
> >>>   		return -EINVAL;
> >>> +	if (WARN_ON_ONCE(!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)))
> >>> +		return -EINVAL;
> >>> +
> >>
> >> Hi Sean,
> >> 'dev_data->use_vapic' is always zero when AMD IOMMU uses legacy
> >> interrupts i.e. when AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) is 0.
> >> Hence you can remove this additional check.
> > 
> > Hmm, or move it above?  KVM should never call amd_ir_set_vcpu_affinity() if
> > IRQ posting is unsupported, and that would make this consistent with the end
> > behavior of amd_iommu_update_ga() and amd_iommu_{de,}activate_guest_mode().
> > 
> > 	if (WARN_ON_ONCE(!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)))
> 
> Note that this is global IOMMU level check while dev_data->use_vapic is per
> device. We set per device thing while attaching device to domain based on IOMMU
> domain type and IOMMU vapic support.
> 
> How about add WARN_ON based on dev_data->use_vapic .. so that we can catch if
> something went wrong in IOMMU side as well?

It's not clear to me that a WARN_ON(dev_data->use_vapic) would be free of false
positives.  AFAICT, the producers (e.g. VFIO) don't check whether or not a device
supports posting interrupts, and KVM definitely doesn't check.  And KVM is also
tolerant of irq_set_vcpu_affinity() failures, specifically for this type of
situation, so unfortunately I don't know that the IOMMU side of the world can
safely WARN.

On 4/16/2025 3:34 AM, Sean Christopherson wrote:
> On Tue, Apr 15, 2025, Vasant Hegde wrote:
>> On 4/11/2025 7:40 PM, Sean Christopherson wrote:
>>> On Fri, Apr 11, 2025, Sairaj Kodilkar wrote:
>>>> On 4/5/2025 1:08 AM, Sean Christopherson wrote:
>>>>> WARN if KVM attempts to set vCPU affinity when posted interrupts aren't
>>>>> enabled, as KVM shouldn't try to enable posting when they're unsupported,
>>>>> and the IOMMU driver darn well should only advertise posting support when
>>>>> AMD_IOMMU_GUEST_IR_VAPIC() is true.
>>>>>
>>>>> Note, KVM consumes is_guest_mode only on success.
>>>>>
>>>>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>>>>> ---
>>>>>    drivers/iommu/amd/iommu.c | 13 +++----------
>>>>>    1 file changed, 3 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>>>>> index b3a01b7757ee..4f69a37cf143 100644
>>>>> --- a/drivers/iommu/amd/iommu.c
>>>>> +++ b/drivers/iommu/amd/iommu.c
>>>>> @@ -3852,19 +3852,12 @@ static int amd_ir_set_vcpu_affinity(struct irq_data *data, void *vcpu_info)
>>>>>    	if (!dev_data || !dev_data->use_vapic)
>>>>>    		return -EINVAL;
>>>>> +	if (WARN_ON_ONCE(!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)))
>>>>> +		return -EINVAL;
>>>>> +
>>>>
>>>> Hi Sean,
>>>> 'dev_data->use_vapic' is always zero when AMD IOMMU uses legacy
>>>> interrupts i.e. when AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) is 0.
>>>> Hence you can remove this additional check.
>>>
>>> Hmm, or move it above?  KVM should never call amd_ir_set_vcpu_affinity() if
>>> IRQ posting is unsupported, and that would make this consistent with the end
>>> behavior of amd_iommu_update_ga() and amd_iommu_{de,}activate_guest_mode().
>>>
>>> 	if (WARN_ON_ONCE(!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)))
>>
>> Note that this is global IOMMU level check while dev_data->use_vapic is per
>> device. We set per device thing while attaching device to domain based on IOMMU
>> domain type and IOMMU vapic support.
>>
>> How about add WARN_ON based on dev_data->use_vapic .. so that we can catch if
>> something went wrong in IOMMU side as well?
> 
> It's not clear to me that a WARN_ON(dev_data->use_vapic) would be free of false
> positives.  AFAICT, the producers (e.g. VFIO) don't check whether or not a device
> supports posting interrupts, and KVM definitely doesn't check.  And KVM is also
> tolerant of irq_set_vcpu_affinity() failures, specifically for this type of
> situation, so unfortunately I don't know that the IOMMU side of the world can
> safely WARN.

Hi sean,
I think it is safe to have this WARN_ON(!dev_data->use_vapic) without
any false positives. IOMMU driver sets the dev_data->use_vapic only when
the device is in UNMANAGE_DOMAIN and it is 0 if the device is in any
other domain (DMA, DMA_FQ, IDENTITY).

We have a bigger problem from the VFIO side if we hit this WARN_ON()
as device is not in a UNMANGED_DOMAIN.

Regards
Sairaj Kodilkar

On Wed, Apr 16, 2025 at 11:47 AM Sairaj Kodilkar <sarunkod@amd.com> wrote:
> I think it is safe to have this WARN_ON(!dev_data->use_vapic) without
> any false positives. IOMMU driver sets the dev_data->use_vapic only when
> the device is in UNMANAGE_DOMAIN and it is 0 if the device is in any
> other domain (DMA, DMA_FQ, IDENTITY).
>
> We have a bigger problem from the VFIO side if we hit this WARN_ON()
> as device is not in a UNMANGED_DOMAIN.

This does seem safe, but its more of a VFIO/iommu change than a KVM
one so I'm not too comfortable with merging it myself; please submit
it as a separate patch.

Paolo

On 4/11/25 16:10, Sean Christopherson wrote:
> On Fri, Apr 11, 2025, Sairaj Kodilkar wrote:
>> On 4/5/2025 1:08 AM, Sean Christopherson wrote:
>>> WARN if KVM attempts to set vCPU affinity when posted interrupts aren't
>>> enabled, as KVM shouldn't try to enable posting when they're unsupported,
>>> and the IOMMU driver darn well should only advertise posting support when
>>> AMD_IOMMU_GUEST_IR_VAPIC() is true.
>>>
>>> Note, KVM consumes is_guest_mode only on success.
>>>
>>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>>> ---
>>>    drivers/iommu/amd/iommu.c | 13 +++----------
>>>    1 file changed, 3 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>>> index b3a01b7757ee..4f69a37cf143 100644
>>> --- a/drivers/iommu/amd/iommu.c
>>> +++ b/drivers/iommu/amd/iommu.c
>>> @@ -3852,19 +3852,12 @@ static int amd_ir_set_vcpu_affinity(struct irq_data *data, void *vcpu_info)
>>>    	if (!dev_data || !dev_data->use_vapic)
>>>    		return -EINVAL;
>>> +	if (WARN_ON_ONCE(!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)))
>>> +		return -EINVAL;
>>> +
>>
>> Hi Sean,
>> 'dev_data->use_vapic' is always zero when AMD IOMMU uses legacy
>> interrupts i.e. when AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) is 0.
>> Hence you can remove this additional check.
> 
> Hmm, or move it above?  KVM should never call amd_ir_set_vcpu_affinity() if
> IRQ posting is unsupported, and that would make this consistent with the end
> behavior of amd_iommu_update_ga() and amd_iommu_{de,}activate_guest_mode().
> 
> 	if (WARN_ON_ONCE(!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)))
> 		return -EINVAL;
> 
> 	if (ir_data->iommu == NULL)
> 		return -EINVAL;
> 
> 	dev_data = search_dev_data(ir_data->iommu, irte_info->devid);
> 
> 	/* Note:
> 	 * This device has never been set up for guest mode.
> 	 * we should not modify the IRTE
> 	 */
> 	if (!dev_data || !dev_data->use_vapic)
> 		return -EINVAL;
> 
> I'd like to keep the WARN so that someone will notice if KVM screws up.

Makes sense, avic_pi_update_irte() returns way before it has the 
occasion to call irq_set_vcpu_affinity().

Paolo

On 4/11/2025 7:40 PM, Sean Christopherson wrote:
> On Fri, Apr 11, 2025, Sairaj Kodilkar wrote:
>> On 4/5/2025 1:08 AM, Sean Christopherson wrote:
>>> WARN if KVM attempts to set vCPU affinity when posted interrupts aren't
>>> enabled, as KVM shouldn't try to enable posting when they're unsupported,
>>> and the IOMMU driver darn well should only advertise posting support when
>>> AMD_IOMMU_GUEST_IR_VAPIC() is true.
>>>
>>> Note, KVM consumes is_guest_mode only on success.
>>>
>>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>>> ---
>>>    drivers/iommu/amd/iommu.c | 13 +++----------
>>>    1 file changed, 3 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>>> index b3a01b7757ee..4f69a37cf143 100644
>>> --- a/drivers/iommu/amd/iommu.c
>>> +++ b/drivers/iommu/amd/iommu.c
>>> @@ -3852,19 +3852,12 @@ static int amd_ir_set_vcpu_affinity(struct irq_data *data, void *vcpu_info)
>>>    	if (!dev_data || !dev_data->use_vapic)
>>>    		return -EINVAL;
>>> +	if (WARN_ON_ONCE(!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)))
>>> +		return -EINVAL;
>>> +
>>
>> Hi Sean,
>> 'dev_data->use_vapic' is always zero when AMD IOMMU uses legacy
>> interrupts i.e. when AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) is 0.
>> Hence you can remove this additional check.
> 
> Hmm, or move it above?  KVM should never call amd_ir_set_vcpu_affinity() if
> IRQ posting is unsupported, and that would make this consistent with the end
> behavior of amd_iommu_update_ga() and amd_iommu_{de,}activate_guest_mode().
> 
> 	if (WARN_ON_ONCE(!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)))
> 		return -EINVAL;
> 
> 	if (ir_data->iommu == NULL)
> 		return -EINVAL;
> 
> 	dev_data = search_dev_data(ir_data->iommu, irte_info->devid);
> 
> 	/* Note:
> 	 * This device has never been set up for guest mode.
> 	 * we should not modify the IRTE
> 	 */
> 	if (!dev_data || !dev_data->use_vapic)
> 		return -EINVAL;
> 
> I'd like to keep the WARN so that someone will notice if KVM screws up.

Yeah makes sense. Lets move it above

Thanks
Sairaj