While the 2nd of the commits referenced below should have moved the call
to amd_iommu_msi_enable() instead of adding another one, the situation
wasn't quite right even before: It can't have done any good to enable
MSI when no IRQ was allocated for it, yet.
Fixes: 5f569f1ac50e ("AMD/IOMMU: allow enabling with IRQ not yet set up")
Fixes: d9e49d1afe2e ("AMD/IOMMU: adjust setup of internal interrupt for x2APIC mode")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -902,8 +902,6 @@ static void enable_iommu(struct amd_iomm
}
}
- amd_iommu_msi_enable(iommu, IOMMU_CONTROL_ENABLED);
-
set_iommu_ht_flags(iommu);
set_iommu_command_buffer_control(iommu, IOMMU_CONTROL_ENABLED);On 30/01/2025 11:11 am, Jan Beulich wrote:
> While the 2nd of the commits referenced below should have moved the call
> to amd_iommu_msi_enable() instead of adding another one, the situation
> wasn't quite right even before: It can't have done any good to enable
> MSI when no IRQ was allocated for it, yet.
>
> Fixes: 5f569f1ac50e ("AMD/IOMMU: allow enabling with IRQ not yet set up")
> Fixes: d9e49d1afe2e ("AMD/IOMMU: adjust setup of internal interrupt for x2APIC mode")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -902,8 +902,6 @@ static void enable_iommu(struct amd_iomm
There's a call to amd_iommu_msi_enable() just out of context here which
was added by the 2nd referenced commit.
Given that it's asymmetric in an if() condition regarding xt_en, and the
calls are only set_affinity() calls, why is this retained?
(I think I know, and if it is the reason I suspect, then you're missing
a very critical detail from the commit message.)
~Andrew
> }
> }
>
> - amd_iommu_msi_enable(iommu, IOMMU_CONTROL_ENABLED);
> -
> set_iommu_ht_flags(iommu);
> set_iommu_command_buffer_control(iommu, IOMMU_CONTROL_ENABLED);
>
>
On 02.02.2025 14:50, Andrew Cooper wrote:
> On 30/01/2025 11:11 am, Jan Beulich wrote:
>> While the 2nd of the commits referenced below should have moved the call
>> to amd_iommu_msi_enable() instead of adding another one, the situation
>> wasn't quite right even before: It can't have done any good to enable
>> MSI when no IRQ was allocated for it, yet.
>>
>> Fixes: 5f569f1ac50e ("AMD/IOMMU: allow enabling with IRQ not yet set up")
>> Fixes: d9e49d1afe2e ("AMD/IOMMU: adjust setup of internal interrupt for x2APIC mode")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>> @@ -902,8 +902,6 @@ static void enable_iommu(struct amd_iomm
>
> There's a call to amd_iommu_msi_enable() just out of context here which
> was added by the 2nd referenced commit.
>
> Given that it's asymmetric in an if() condition regarding xt_en, and the
> calls are only set_affinity() calls, why is this retained?
>
> (I think I know, and if it is the reason I suspect, then you're missing
> a very critical detail from the commit message.)
Hmm, you did read the commit message, didn't you? That commit should have
moved that call, rather than adding another one.
However, you have a point. It looks like 7a89f62dddee ("AMD IOMMU: make
interrupt work again") should already have removed that call. Prior to
that change request_irq()'s call (via setup_irq()) to iommu_msi_startup()
was in fact premature, as MSI address and data weren't set up yet (IOW
while still apparently redundant, the extra call served kind of a doc
purpose). Things apparently worked because the IOMMU itself wasn't
enabled yet, and hence shouldn't have raised any interrupts prior to MSI
being fully configured.
However, for S3 resume I think the call needs to stay there, as the
startup hook wouldn't be called in that case (which may be the detail
you're alluding to). Imo that wants solving differently though. Not sure
it's a good idea to do this right here, or perhaps better in a separate
change.
I've added
"The other call to amd_iommu_msi_enable(), just out of patch context,
needs to stay there until S3 resume is re-worked. For the boot path that
call should be unnecessary, as iommu{,_maskable}_msi_startup() will have
done it already (by way of invoking iommu_msi_unmask())."
as a 2nd paragraph to the description, in the hope that's what you're
after.
Jan
On 03/02/2025 8:41 am, Jan Beulich wrote:
> On 02.02.2025 14:50, Andrew Cooper wrote:
>> On 30/01/2025 11:11 am, Jan Beulich wrote:
>>> While the 2nd of the commits referenced below should have moved the call
>>> to amd_iommu_msi_enable() instead of adding another one, the situation
>>> wasn't quite right even before: It can't have done any good to enable
>>> MSI when no IRQ was allocated for it, yet.
>>>
>>> Fixes: 5f569f1ac50e ("AMD/IOMMU: allow enabling with IRQ not yet set up")
>>> Fixes: d9e49d1afe2e ("AMD/IOMMU: adjust setup of internal interrupt for x2APIC mode")
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>>> @@ -902,8 +902,6 @@ static void enable_iommu(struct amd_iomm
>> There's a call to amd_iommu_msi_enable() just out of context here which
>> was added by the 2nd referenced commit.
>>
>> Given that it's asymmetric in an if() condition regarding xt_en, and the
>> calls are only set_affinity() calls, why is this retained?
>>
>> (I think I know, and if it is the reason I suspect, then you're missing
>> a very critical detail from the commit message.)
> Hmm, you did read the commit message, didn't you? That commit should have
> moved that call, rather than adding another one.
>
> However, you have a point. It looks like 7a89f62dddee ("AMD IOMMU: make
> interrupt work again") should already have removed that call. Prior to
> that change request_irq()'s call (via setup_irq()) to iommu_msi_startup()
> was in fact premature, as MSI address and data weren't set up yet (IOW
> while still apparently redundant, the extra call served kind of a doc
> purpose). Things apparently worked because the IOMMU itself wasn't
> enabled yet, and hence shouldn't have raised any interrupts prior to MSI
> being fully configured.
>
> However, for S3 resume I think the call needs to stay there, as the
> startup hook wouldn't be called in that case (which may be the detail
> you're alluding to). Imo that wants solving differently though. Not sure
> it's a good idea to do this right here, or perhaps better in a separate
> change.
>
> I've added
>
> "The other call to amd_iommu_msi_enable(), just out of patch context,
> needs to stay there until S3 resume is re-worked. For the boot path that
> call should be unnecessary, as iommu{,_maskable}_msi_startup() will have
> done it already (by way of invoking iommu_msi_unmask())."
>
> as a 2nd paragraph to the description, in the hope that's what you're
> after.
Ok, not the reason I was thinking. I was thinking it was an x vs x2
APIC issue, and split setup path.
It is specifically weird to have:
if ( msi )
{
if ( cap_xt_en )
...
else
{
...
amd_iommu_msi_enable();
}
// should enable here ?
}
If this call really is only necessary for the S3 path, that explains
half the problem, but what activates MSIs for the xt_en case after S3?
~Andrew
On 03.02.2025 15:19, Andrew Cooper wrote:
> On 03/02/2025 8:41 am, Jan Beulich wrote:
>> On 02.02.2025 14:50, Andrew Cooper wrote:
>>> On 30/01/2025 11:11 am, Jan Beulich wrote:
>>>> While the 2nd of the commits referenced below should have moved the call
>>>> to amd_iommu_msi_enable() instead of adding another one, the situation
>>>> wasn't quite right even before: It can't have done any good to enable
>>>> MSI when no IRQ was allocated for it, yet.
>>>>
>>>> Fixes: 5f569f1ac50e ("AMD/IOMMU: allow enabling with IRQ not yet set up")
>>>> Fixes: d9e49d1afe2e ("AMD/IOMMU: adjust setup of internal interrupt for x2APIC mode")
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>>>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>>>> @@ -902,8 +902,6 @@ static void enable_iommu(struct amd_iomm
>>> There's a call to amd_iommu_msi_enable() just out of context here which
>>> was added by the 2nd referenced commit.
>>>
>>> Given that it's asymmetric in an if() condition regarding xt_en, and the
>>> calls are only set_affinity() calls, why is this retained?
>>>
>>> (I think I know, and if it is the reason I suspect, then you're missing
>>> a very critical detail from the commit message.)
>> Hmm, you did read the commit message, didn't you? That commit should have
>> moved that call, rather than adding another one.
>>
>> However, you have a point. It looks like 7a89f62dddee ("AMD IOMMU: make
>> interrupt work again") should already have removed that call. Prior to
>> that change request_irq()'s call (via setup_irq()) to iommu_msi_startup()
>> was in fact premature, as MSI address and data weren't set up yet (IOW
>> while still apparently redundant, the extra call served kind of a doc
>> purpose). Things apparently worked because the IOMMU itself wasn't
>> enabled yet, and hence shouldn't have raised any interrupts prior to MSI
>> being fully configured.
>>
>> However, for S3 resume I think the call needs to stay there, as the
>> startup hook wouldn't be called in that case (which may be the detail
>> you're alluding to). Imo that wants solving differently though. Not sure
>> it's a good idea to do this right here, or perhaps better in a separate
>> change.
>>
>> I've added
>>
>> "The other call to amd_iommu_msi_enable(), just out of patch context,
>> needs to stay there until S3 resume is re-worked. For the boot path that
>> call should be unnecessary, as iommu{,_maskable}_msi_startup() will have
>> done it already (by way of invoking iommu_msi_unmask())."
>>
>> as a 2nd paragraph to the description, in the hope that's what you're
>> after.
>
> Ok, not the reason I was thinking. I was thinking it was an x vs x2
> APIC issue, and split setup path.
>
> It is specifically weird to have:
>
> if ( msi )
> {
> if ( cap_xt_en )
> ...
> else
> {
> ...
> amd_iommu_msi_enable();
> }
> // should enable here ?
> }
>
> If this call really is only necessary for the S3 path, that explains
> half the problem, but what activates MSIs for the xt_en case after S3?
The write of the control register where the enable bit is. There's no
actual "MSI" anymore in that case.
Jan
On 03/02/2025 3:53 pm, Jan Beulich wrote:
> On 03.02.2025 15:19, Andrew Cooper wrote:
>> On 03/02/2025 8:41 am, Jan Beulich wrote:
>>> On 02.02.2025 14:50, Andrew Cooper wrote:
>>>> On 30/01/2025 11:11 am, Jan Beulich wrote:
>>>>> While the 2nd of the commits referenced below should have moved the call
>>>>> to amd_iommu_msi_enable() instead of adding another one, the situation
>>>>> wasn't quite right even before: It can't have done any good to enable
>>>>> MSI when no IRQ was allocated for it, yet.
>>>>>
>>>>> Fixes: 5f569f1ac50e ("AMD/IOMMU: allow enabling with IRQ not yet set up")
>>>>> Fixes: d9e49d1afe2e ("AMD/IOMMU: adjust setup of internal interrupt for x2APIC mode")
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>
>>>>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>>>>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>>>>> @@ -902,8 +902,6 @@ static void enable_iommu(struct amd_iomm
>>>> There's a call to amd_iommu_msi_enable() just out of context here which
>>>> was added by the 2nd referenced commit.
>>>>
>>>> Given that it's asymmetric in an if() condition regarding xt_en, and the
>>>> calls are only set_affinity() calls, why is this retained?
>>>>
>>>> (I think I know, and if it is the reason I suspect, then you're missing
>>>> a very critical detail from the commit message.)
>>> Hmm, you did read the commit message, didn't you? That commit should have
>>> moved that call, rather than adding another one.
>>>
>>> However, you have a point. It looks like 7a89f62dddee ("AMD IOMMU: make
>>> interrupt work again") should already have removed that call. Prior to
>>> that change request_irq()'s call (via setup_irq()) to iommu_msi_startup()
>>> was in fact premature, as MSI address and data weren't set up yet (IOW
>>> while still apparently redundant, the extra call served kind of a doc
>>> purpose). Things apparently worked because the IOMMU itself wasn't
>>> enabled yet, and hence shouldn't have raised any interrupts prior to MSI
>>> being fully configured.
>>>
>>> However, for S3 resume I think the call needs to stay there, as the
>>> startup hook wouldn't be called in that case (which may be the detail
>>> you're alluding to). Imo that wants solving differently though. Not sure
>>> it's a good idea to do this right here, or perhaps better in a separate
>>> change.
>>>
>>> I've added
>>>
>>> "The other call to amd_iommu_msi_enable(), just out of patch context,
>>> needs to stay there until S3 resume is re-worked. For the boot path that
>>> call should be unnecessary, as iommu{,_maskable}_msi_startup() will have
>>> done it already (by way of invoking iommu_msi_unmask())."
>>>
>>> as a 2nd paragraph to the description, in the hope that's what you're
>>> after.
>> Ok, not the reason I was thinking. I was thinking it was an x vs x2
>> APIC issue, and split setup path.
>>
>> It is specifically weird to have:
>>
>> if ( msi )
>> {
>> if ( cap_xt_en )
>> ...
>> else
>> {
>> ...
>> amd_iommu_msi_enable();
>> }
>> // should enable here ?
>> }
>>
>> If this call really is only necessary for the S3 path, that explains
>> half the problem, but what activates MSIs for the xt_en case after S3?
> The write of the control register where the enable bit is. There's no
> actual "MSI" anymore in that case.
Oh, right.
I definitely knew that at one point, and I've clearly forgotten.
I wonder if we want a /* Note, no MSI in this case */ inside the if(),
but that might be a stretch...
~Andrew
On 2025-01-30 06:11, Jan Beulich wrote:
> While the 2nd of the commits referenced below should have moved the call
> to amd_iommu_msi_enable() instead of adding another one, the situation
> wasn't quite right even before: It can't have done any good to enable
> MSI when no IRQ was allocated for it, yet.
>
> Fixes: 5f569f1ac50e ("AMD/IOMMU: allow enabling with IRQ not yet set up")
> Fixes: d9e49d1afe2e ("AMD/IOMMU: adjust setup of internal interrupt for x2APIC mode")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Tested-by: Jason Andryuk <jason.andryuk@amd.com>
dom0=pvh on CPU *without* x2apic support.
Thanks,
Jason
© 2016 - 2025 Red Hat, Inc.