| Message ID | ea0fea03-6002-4fc6-86ac-19598c9d9ef6@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | AMD/IOMMU: assorted corrections | expand |
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
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
--- 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);
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>