[PATCH for-4.20 0/3] AMD/IOMMU: assorted corrections

[PATCH for-4.20 0/3] AMD/IOMMU: assorted corrections

[Jan Beulich]

The three patches are functionally independent, and they're presented
here merely in the order I came to notice the respective issues. At least
patch 2 wants seriously considering for 4.20.

1: AMD/IOMMU: drop stray MSI enabling
2: x86/PCI: init segments earlier
3: AMD/IOMMU: log IVHD contents

Jan

[PATCH for-4.20? 1/3] AMD/IOMMU: drop stray MSI enabling

[Jan Beulich]

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);
 

[PATCH for-4.20 2/3] x86/PCI: init segments earlier

[Jan Beulich]

In order for amd_iommu_detect_one_acpi()'s call to pci_ro_device() to
have permanent effect, pci_segments_init() needs to be called ahead of
making it there. Without this we're losing segment 0's r/o map, and thus
we're losing write-protection of the PCI devices representing IOMMUs.
Which in turn means that half-way recent Linux Dom0 will, as it boots,
turn off MSI on these devices, thus preventing any IOMMU events (faults
in particular) from being reported on pre-x2APIC hardware.

As the acpi_iommu_init() invocation was moved ahead of
acpi_mmcfg_init()'s by the offending commit, move the call to
pci_segments_init() accordingly.

Fixes: 3950f2485bbc ("x86/x2APIC: defer probe until after IOMMU ACPI table parsing")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Of course it would have been quite a bit easier to notice this issue if
radix_tree_insert() wouldn't work fine ahead of radix_tree_init() being
invoked for a given radix tree, when the index inserted at is 0.

While hunting down various other dead paths to actually find the root
cause, it occurred to me that it's probably not a good idea to fully
disallow config space writes for r/o devices: Dom0 won't be able to size
their BARs (luckily the IOMMU "devices" don't have any, but e.g. serial
ones generally will have at least one), for example. Without being able
to size BARs it also will likely be unable to correctly account for the
address space taken by these BARs. However, outside of vPCI it's not
really clear to me how we could reasonably emulate such BAR sizing
writes - we can't, after all, allow Dom0 to actually write to the
underlying physical registers, yet we don't intercept reads (i.e. we
can't mimic expected behavior then).

--- a/xen/arch/x86/x86_64/mmconfig-shared.c
+++ b/xen/arch/x86/x86_64/mmconfig-shared.c
@@ -402,8 +402,6 @@ void __init acpi_mmcfg_init(void)
 {
     bool valid = true;
 
-    pci_segments_init();
-
     /* MMCONFIG disabled */
     if ((pci_probe & PCI_PROBE_MMCONF) == 0)
         return;
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -55,6 +55,8 @@ void __init acpi_iommu_init(void)
 {
     int ret = -ENODEV;
 
+    pci_segments_init();
+
     if ( !iommu_enable && !iommu_intremap )
         return;
 

[PATCH for-4.20? 3/3] AMD/IOMMU: log IVHD contents

[Jan Beulich]

Despite all the verbosity with "iommu=debug", information on the IOMMUs
themselves was missing.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -911,6 +911,11 @@ static int __init parse_ivhd_block(const
         return -ENODEV;
     }
 
+    AMD_IOMMU_DEBUG("IVHD: IOMMU @ %#lx cap @ %#x seg 0x%04x info %#x attr %#x\n",
+                    ivhd_block->base_address, ivhd_block->capability_offset,
+                    ivhd_block->pci_segment_group, ivhd_block->info,
+                    ivhd_block->iommu_attr);
+
     iommu = find_iommu_from_bdf_cap(ivhd_block->pci_segment_group,
                                     ivhd_block->header.device_id,
                                     ivhd_block->capability_offset);

Re: [PATCH for-4.20 0/3] AMD/IOMMU: assorted corrections

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 636 bytes --]


On 1/30/25 12:10 PM, Jan Beulich wrote:
> The three patches are functionally independent, and they're presented
> here merely in the order I came to notice the respective issues. At least
> patch 2 wants seriously considering for 4.20.

Based on the commit message for patch 2 should be merged to 4.20, patch 1 could be merged too as it is pretty straightforward
and I don't really mind to merge some additional logs. With proper review:
R-Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

>
> 1: AMD/IOMMU: drop stray MSI enabling
> 2: x86/PCI: init segments earlier
> 3: AMD/IOMMU: log IVHD contents
>
> Jan

[-- Attachment #2: Type: text/html, Size: 1246 bytes --]

Re: [PATCH for-4.20? 1/3] AMD/IOMMU: drop stray MSI enabling

[Jason Andryuk]

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

Re: [PATCH for-4.20 2/3] x86/PCI: init segments earlier

[Jason Andryuk]

On 2025-01-30 06:12, Jan Beulich wrote:
> In order for amd_iommu_detect_one_acpi()'s call to pci_ro_device() to
> have permanent effect, pci_segments_init() needs to be called ahead of
> making it there. Without this we're losing segment 0's r/o map, and thus
> we're losing write-protection of the PCI devices representing IOMMUs.
> Which in turn means that half-way recent Linux Dom0 will, as it boots,
> turn off MSI on these devices, thus preventing any IOMMU events (faults
> in particular) from being reported on pre-x2APIC hardware.
> 
> As the acpi_iommu_init() invocation was moved ahead of
> acpi_mmcfg_init()'s by the offending commit, move the call to
> pci_segments_init() accordingly.
> 
> Fixes: 3950f2485bbc ("x86/x2APIC: defer probe until after IOMMU ACPI table parsing")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Tested-by: Jason Andryuk <jason.andryuk@amd.com>

Thanks,
Jason

Re: [PATCH for-4.20? 3/3] AMD/IOMMU: log IVHD contents

[Jason Andryuk]

On 2025-01-30 06:13, Jan Beulich wrote:
> Despite all the verbosity with "iommu=debug", information on the IOMMUs
> themselves was missing.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Tested-by: Jason Andryuk <jason.andryuk@amd.com>

Thanks,
Jason

Re: [PATCH for-4.20? 1/3] AMD/IOMMU: drop stray MSI enabling

[Andrew Cooper]

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);
>  
>

Re: [PATCH for-4.20? 3/3] AMD/IOMMU: log IVHD contents

[Andrew Cooper]

On 30/01/2025 11:13 am, Jan Beulich wrote:
> Despite all the verbosity with "iommu=debug", information on the IOMMUs
> themselves was missing.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH for-4.20 2/3] x86/PCI: init segments earlier

[Andrew Cooper]

On 30/01/2025 11:12 am, Jan Beulich wrote:
> In order for amd_iommu_detect_one_acpi()'s call to pci_ro_device() to
> have permanent effect, pci_segments_init() needs to be called ahead of
> making it there. Without this we're losing segment 0's r/o map, and thus
> we're losing write-protection of the PCI devices representing IOMMUs.
> Which in turn means that half-way recent Linux Dom0 will, as it boots,
> turn off MSI on these devices, thus preventing any IOMMU events (faults
> in particular) from being reported on pre-x2APIC hardware.
>
> As the acpi_iommu_init() invocation was moved ahead of
> acpi_mmcfg_init()'s by the offending commit, move the call to
> pci_segments_init() accordingly.
>
> Fixes: 3950f2485bbc ("x86/x2APIC: defer probe until after IOMMU ACPI table parsing")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Of course it would have been quite a bit easier to notice this issue if
> radix_tree_insert() wouldn't work fine ahead of radix_tree_init() being
> invoked for a given radix tree, when the index inserted at is 0.
>
> While hunting down various other dead paths to actually find the root
> cause, it occurred to me that it's probably not a good idea to fully
> disallow config space writes for r/o devices: Dom0 won't be able to size
> their BARs (luckily the IOMMU "devices" don't have any, but e.g. serial
> ones generally will have at least one), for example. Without being able
> to size BARs it also will likely be unable to correctly account for the
> address space taken by these BARs. However, outside of vPCI it's not
> really clear to me how we could reasonably emulate such BAR sizing
> writes - we can't, after all, allow Dom0 to actually write to the
> underlying physical registers, yet we don't intercept reads (i.e. we
> can't mimic expected behavior then).
>
> --- a/xen/arch/x86/x86_64/mmconfig-shared.c
> +++ b/xen/arch/x86/x86_64/mmconfig-shared.c
> @@ -402,8 +402,6 @@ void __init acpi_mmcfg_init(void)
>  {
>      bool valid = true;
>  
> -    pci_segments_init();
> -
>      /* MMCONFIG disabled */
>      if ((pci_probe & PCI_PROBE_MMCONF) == 0)
>          return;
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -55,6 +55,8 @@ void __init acpi_iommu_init(void)
>  {
>      int ret = -ENODEV;
>  
> +    pci_segments_init();
> +
>      if ( !iommu_enable && !iommu_intremap )
>          return;
>  
>

I can't help but feel this is taking a bad problem and not making it any
better.

pci_segments_init() is even less (obviously) relevant in
apci_iommu_init() than it is in acpi_mmcfg_init(), and given the
fine-grain Kconfig-ing going on, is only one small step from
accidentally being compiled out.

ARM is in a bad state too, with this initialisation even being behind
the PCI Passthrough cmdline option.

IMO there are two problems here; one as you pointed out
(radix_tree_insert() doesn't fail), and that PCI handling requires
explicit initialisation to begin with.

Looking through radix tree, it wouldn't be hard to create a
RADIX_TREE_INIT macro to allow initialisation at compile time for
suitable objects (pci_segments and acpi_ivrs currently).

That involves exporting rcu_node_{alloc,free}(), although the last
caller of radix_tree_set_alloc_callbacks() was dropped when TMEM went,
so we could reasonably remove that infrastructure too, at which point
radix_tree_init() is a simple zero of the structure.

Dealing with alloc_pseg(0) is harder.  As we never free the PCI
segments, we could just opencode the radix_tree_root of height=1 with a
static pseg0 structure, and that would drop the need for
pci_segemnts_init() completely.

This gets us into a far less fragile position, and one liable to survive
future refactoring too.

~Andrew

P.S. Yes AMD IOMMUs really do have BARs.  The BIOS programs them, then
sets a register in config space to hide the BAR registers.  You can
reprogram them if you know how.

Re: [PATCH for-4.20? 1/3] AMD/IOMMU: drop stray MSI enabling

[Jan Beulich]

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

Re: [PATCH for-4.20 2/3] x86/PCI: init segments earlier

[Jan Beulich]

On 02.02.2025 15:46, Andrew Cooper wrote:
> On 30/01/2025 11:12 am, Jan Beulich wrote:
>> In order for amd_iommu_detect_one_acpi()'s call to pci_ro_device() to
>> have permanent effect, pci_segments_init() needs to be called ahead of
>> making it there. Without this we're losing segment 0's r/o map, and thus
>> we're losing write-protection of the PCI devices representing IOMMUs.
>> Which in turn means that half-way recent Linux Dom0 will, as it boots,
>> turn off MSI on these devices, thus preventing any IOMMU events (faults
>> in particular) from being reported on pre-x2APIC hardware.
>>
>> As the acpi_iommu_init() invocation was moved ahead of
>> acpi_mmcfg_init()'s by the offending commit, move the call to
>> pci_segments_init() accordingly.
>>
>> Fixes: 3950f2485bbc ("x86/x2APIC: defer probe until after IOMMU ACPI table parsing")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Of course it would have been quite a bit easier to notice this issue if
>> radix_tree_insert() wouldn't work fine ahead of radix_tree_init() being
>> invoked for a given radix tree, when the index inserted at is 0.
>>
>> While hunting down various other dead paths to actually find the root
>> cause, it occurred to me that it's probably not a good idea to fully
>> disallow config space writes for r/o devices: Dom0 won't be able to size
>> their BARs (luckily the IOMMU "devices" don't have any, but e.g. serial
>> ones generally will have at least one), for example. Without being able
>> to size BARs it also will likely be unable to correctly account for the
>> address space taken by these BARs. However, outside of vPCI it's not
>> really clear to me how we could reasonably emulate such BAR sizing
>> writes - we can't, after all, allow Dom0 to actually write to the
>> underlying physical registers, yet we don't intercept reads (i.e. we
>> can't mimic expected behavior then).
>>
>> --- a/xen/arch/x86/x86_64/mmconfig-shared.c
>> +++ b/xen/arch/x86/x86_64/mmconfig-shared.c
>> @@ -402,8 +402,6 @@ void __init acpi_mmcfg_init(void)
>>  {
>>      bool valid = true;
>>  
>> -    pci_segments_init();
>> -
>>      /* MMCONFIG disabled */
>>      if ((pci_probe & PCI_PROBE_MMCONF) == 0)
>>          return;
>> --- a/xen/drivers/passthrough/x86/iommu.c
>> +++ b/xen/drivers/passthrough/x86/iommu.c
>> @@ -55,6 +55,8 @@ void __init acpi_iommu_init(void)
>>  {
>>      int ret = -ENODEV;
>>  
>> +    pci_segments_init();
>> +
>>      if ( !iommu_enable && !iommu_intremap )
>>          return;
>>  
>>
> 
> I can't help but feel this is taking a bad problem and not making it any
> better.
> 
> pci_segments_init() is even less (obviously) relevant in
> apci_iommu_init() than it is in acpi_mmcfg_init(), and given the
> fine-grain Kconfig-ing going on, is only one small step from
> accidentally being compiled out.

The alternative I did consider was to move the call into __start_xen()
itself. Anything going beyond that looks more intrusive than we'd like
it at this point of the release cycle.

> ARM is in a bad state too, with this initialisation even being behind
> the PCI Passthrough cmdline option.
> 
> IMO there are two problems here; one as you pointed out
> (radix_tree_insert() doesn't fail), and that PCI handling requires
> explicit initialisation to begin with.
> 
> Looking through radix tree, it wouldn't be hard to create a
> RADIX_TREE_INIT macro to allow initialisation at compile time for
> suitable objects (pci_segments and acpi_ivrs currently).
> 
> That involves exporting rcu_node_{alloc,free}(), although the last
> caller of radix_tree_set_alloc_callbacks() was dropped when TMEM went,
> so we could reasonably remove that infrastructure too, at which point
> radix_tree_init() is a simple zero of the structure.

Yes, seeing that this was even an extension of ours (i.e. Linux doesn't
have such), it's certainly worth getting rid of. If nothing else, then
for the two cf_check annotations that's we'd then be able to drop. I'll
make a patch.

> Dealing with alloc_pseg(0) is harder.  As we never free the PCI
> segments, we could just opencode the radix_tree_root of height=1 with a
> static pseg0 structure, and that would drop the need for
> pci_segemnts_init() completely.

I'm afraid this would end up being too much open-coding for my taste.

I'd put this differently: Unlike the radix tree initialization, the
setting up of segment 0 isn't a prereq to acpi_iommu_init(). We could
keep acpi_mmcfg_init() doing that, by way of calling pci_add_segment(0)
(and that would simply be a no-op if acpi_iommu_init() ended up
introducing segment 0 already).

Jan

Re: [PATCH for-4.20 2/3] x86/PCI: init segments earlier

[Roger Pau Monné]

On Thu, Jan 30, 2025 at 12:12:31PM +0100, Jan Beulich wrote:
> In order for amd_iommu_detect_one_acpi()'s call to pci_ro_device() to
> have permanent effect, pci_segments_init() needs to be called ahead of
> making it there. Without this we're losing segment 0's r/o map, and thus
> we're losing write-protection of the PCI devices representing IOMMUs.
> Which in turn means that half-way recent Linux Dom0 will, as it boots,
> turn off MSI on these devices, thus preventing any IOMMU events (faults
> in particular) from being reported on pre-x2APIC hardware.
> 
> As the acpi_iommu_init() invocation was moved ahead of
> acpi_mmcfg_init()'s by the offending commit, move the call to
> pci_segments_init() accordingly.
> 
> Fixes: 3950f2485bbc ("x86/x2APIC: defer probe until after IOMMU ACPI table parsing")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Of course it would have been quite a bit easier to notice this issue if
> radix_tree_insert() wouldn't work fine ahead of radix_tree_init() being
> invoked for a given radix tree, when the index inserted at is 0.
> 
> While hunting down various other dead paths to actually find the root
> cause, it occurred to me that it's probably not a good idea to fully
> disallow config space writes for r/o devices: Dom0 won't be able to size
> their BARs (luckily the IOMMU "devices" don't have any, but e.g. serial
> ones generally will have at least one), for example. Without being able
> to size BARs it also will likely be unable to correctly account for the
> address space taken by these BARs. However, outside of vPCI it's not
> really clear to me how we could reasonably emulate such BAR sizing
> writes - we can't, after all, allow Dom0 to actually write to the
> underlying physical registers, yet we don't intercept reads (i.e. we
> can't mimic expected behavior then).

For properly sizing the domain will also attempt to toggle the memory
decoding bit ahead of sizing the BARs, and letting that trough will
break the usage of the device from Xen.  IOW: we would likely need to
emulate a fair amount of device state to make the view coherent from a
guest PoV, but is it worth it for a device that the hardware domain
cannot interact with?

Would it make more sense to just hide those devices instead of
allowing read-only access to their PCI config space?

> --- a/xen/arch/x86/x86_64/mmconfig-shared.c
> +++ b/xen/arch/x86/x86_64/mmconfig-shared.c
> @@ -402,8 +402,6 @@ void __init acpi_mmcfg_init(void)
>  {
>      bool valid = true;
>  
> -    pci_segments_init();
> -
>      /* MMCONFIG disabled */
>      if ((pci_probe & PCI_PROBE_MMCONF) == 0)
>          return;
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -55,6 +55,8 @@ void __init acpi_iommu_init(void)
>  {
>      int ret = -ENODEV;
>  
> +    pci_segments_init();

My preference might be to just place the pci_segments_init() call in
__start_xen(), instead of hiding it again in what might look like an
unrelated function (there's no mention of PCI in acpi_iommu_init()
function name for example).

Thanks, Roger.

Re: [PATCH for-4.20 2/3] x86/PCI: init segments earlier

[Jan Beulich]

On 03.02.2025 13:45, Roger Pau Monné wrote:
> On Thu, Jan 30, 2025 at 12:12:31PM +0100, Jan Beulich wrote:
>> In order for amd_iommu_detect_one_acpi()'s call to pci_ro_device() to
>> have permanent effect, pci_segments_init() needs to be called ahead of
>> making it there. Without this we're losing segment 0's r/o map, and thus
>> we're losing write-protection of the PCI devices representing IOMMUs.
>> Which in turn means that half-way recent Linux Dom0 will, as it boots,
>> turn off MSI on these devices, thus preventing any IOMMU events (faults
>> in particular) from being reported on pre-x2APIC hardware.
>>
>> As the acpi_iommu_init() invocation was moved ahead of
>> acpi_mmcfg_init()'s by the offending commit, move the call to
>> pci_segments_init() accordingly.
>>
>> Fixes: 3950f2485bbc ("x86/x2APIC: defer probe until after IOMMU ACPI table parsing")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Of course it would have been quite a bit easier to notice this issue if
>> radix_tree_insert() wouldn't work fine ahead of radix_tree_init() being
>> invoked for a given radix tree, when the index inserted at is 0.
>>
>> While hunting down various other dead paths to actually find the root
>> cause, it occurred to me that it's probably not a good idea to fully
>> disallow config space writes for r/o devices: Dom0 won't be able to size
>> their BARs (luckily the IOMMU "devices" don't have any, but e.g. serial
>> ones generally will have at least one), for example. Without being able
>> to size BARs it also will likely be unable to correctly account for the
>> address space taken by these BARs. However, outside of vPCI it's not
>> really clear to me how we could reasonably emulate such BAR sizing
>> writes - we can't, after all, allow Dom0 to actually write to the
>> underlying physical registers, yet we don't intercept reads (i.e. we
>> can't mimic expected behavior then).
> 
> For properly sizing the domain will also attempt to toggle the memory
> decoding bit ahead of sizing the BARs, and letting that trough will
> break the usage of the device from Xen.  IOW: we would likely need to
> emulate a fair amount of device state to make the view coherent from a
> guest PoV, but is it worth it for a device that the hardware domain
> cannot interact with?
> 
> Would it make more sense to just hide those devices instead of
> allowing read-only access to their PCI config space?

No, I don't think so. The original reason is still valid: We want such
devices to be enumerable by Dom0. Consider just this one implication
from us not permitting that: What if such a device is part of a multi-
function one, at func 0? Then we'd effectively hide all other devices
at the same bus/dev, too.

>> --- a/xen/arch/x86/x86_64/mmconfig-shared.c
>> +++ b/xen/arch/x86/x86_64/mmconfig-shared.c
>> @@ -402,8 +402,6 @@ void __init acpi_mmcfg_init(void)
>>  {
>>      bool valid = true;
>>  
>> -    pci_segments_init();
>> -
>>      /* MMCONFIG disabled */
>>      if ((pci_probe & PCI_PROBE_MMCONF) == 0)
>>          return;
>> --- a/xen/drivers/passthrough/x86/iommu.c
>> +++ b/xen/drivers/passthrough/x86/iommu.c
>> @@ -55,6 +55,8 @@ void __init acpi_iommu_init(void)
>>  {
>>      int ret = -ENODEV;
>>  
>> +    pci_segments_init();
> 
> My preference might be to just place the pci_segments_init() call in
> __start_xen(),

As said in reply to Andrew - I was considering doing so as an alternative
to the moving done here. I can certainly do so, in case some non-negative
reply comes back from him.

> instead of hiding it again in what might look like an
> unrelated function (there's no mention of PCI in acpi_iommu_init()
> function name for example).

Nor is there in acpi_mmcfg_init(). Irrespective of their names, both are
firmly tied to PCI.

Jan

Re: [PATCH for-4.20 2/3] x86/PCI: init segments earlier

[Jan Beulich]

On 03.02.2025 14:00, Jan Beulich wrote:
> On 03.02.2025 13:45, Roger Pau Monné wrote:
>> On Thu, Jan 30, 2025 at 12:12:31PM +0100, Jan Beulich wrote:
>>> --- a/xen/arch/x86/x86_64/mmconfig-shared.c
>>> +++ b/xen/arch/x86/x86_64/mmconfig-shared.c
>>> @@ -402,8 +402,6 @@ void __init acpi_mmcfg_init(void)
>>>  {
>>>      bool valid = true;
>>>  
>>> -    pci_segments_init();
>>> -
>>>      /* MMCONFIG disabled */
>>>      if ((pci_probe & PCI_PROBE_MMCONF) == 0)
>>>          return;
>>> --- a/xen/drivers/passthrough/x86/iommu.c
>>> +++ b/xen/drivers/passthrough/x86/iommu.c
>>> @@ -55,6 +55,8 @@ void __init acpi_iommu_init(void)
>>>  {
>>>      int ret = -ENODEV;
>>>  
>>> +    pci_segments_init();
>>
>> My preference might be to just place the pci_segments_init() call in
>> __start_xen(),
> 
> As said in reply to Andrew - I was considering doing so as an alternative
> to the moving done here. I can certainly do so, in case some non-negative
> reply comes back from him.

Oh, and: With further adjustments following from what Andrew had outlined,
I'm actually moving the invocation of what was pci_segments_init() back to
where it's now. (Which doesn't mean that couldn't be done from
__start_xen(); just mentioning it.)

Jan

Re: [PATCH for-4.20? 1/3] AMD/IOMMU: drop stray MSI enabling

[Andrew Cooper]

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

Re: [PATCH for-4.20 2/3] x86/PCI: init segments earlier

[Andrew Cooper]

On 03/02/2025 1:03 pm, Jan Beulich wrote:
> On 03.02.2025 14:00, Jan Beulich wrote:
>> On 03.02.2025 13:45, Roger Pau Monné wrote:
>>> On Thu, Jan 30, 2025 at 12:12:31PM +0100, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/x86_64/mmconfig-shared.c
>>>> +++ b/xen/arch/x86/x86_64/mmconfig-shared.c
>>>> @@ -402,8 +402,6 @@ void __init acpi_mmcfg_init(void)
>>>>  {
>>>>      bool valid = true;
>>>>  
>>>> -    pci_segments_init();
>>>> -
>>>>      /* MMCONFIG disabled */
>>>>      if ((pci_probe & PCI_PROBE_MMCONF) == 0)
>>>>          return;
>>>> --- a/xen/drivers/passthrough/x86/iommu.c
>>>> +++ b/xen/drivers/passthrough/x86/iommu.c
>>>> @@ -55,6 +55,8 @@ void __init acpi_iommu_init(void)
>>>>  {
>>>>      int ret = -ENODEV;
>>>>  
>>>> +    pci_segments_init();
>>> My preference might be to just place the pci_segments_init() call in
>>> __start_xen(),
>> As said in reply to Andrew - I was considering doing so as an alternative
>> to the moving done here. I can certainly do so, in case some non-negative
>> reply comes back from him.
> Oh, and: With further adjustments following from what Andrew had outlined,
> I'm actually moving the invocation of what was pci_segments_init() back to
> where it's now. (Which doesn't mean that couldn't be done from
> __start_xen(); just mentioning it.)

The name acpi_mmcfg_init() at least has a PCI implication, given mmcfg.

I know it's late in 4.20, and moving pci_segments_init() would be
acceptable at this juncture.

However, if you're making progress with improving radix trees, I think
that would be a better approach and probably fine to be considered at
this point.

~Andrew

Re: [PATCH for-4.20 2/3] x86/PCI: init segments earlier

[Andrew Cooper]

On 03/02/2025 9:09 am, Jan Beulich wrote:
> On 02.02.2025 15:46, Andrew Cooper wrote:
>> On 30/01/2025 11:12 am, Jan Beulich wrote:
>>> In order for amd_iommu_detect_one_acpi()'s call to pci_ro_device() to
>>> have permanent effect, pci_segments_init() needs to be called ahead of
>>> making it there. Without this we're losing segment 0's r/o map, and thus
>>> we're losing write-protection of the PCI devices representing IOMMUs.
>>> Which in turn means that half-way recent Linux Dom0 will, as it boots,
>>> turn off MSI on these devices, thus preventing any IOMMU events (faults
>>> in particular) from being reported on pre-x2APIC hardware.
>>>
>>> As the acpi_iommu_init() invocation was moved ahead of
>>> acpi_mmcfg_init()'s by the offending commit, move the call to
>>> pci_segments_init() accordingly.
>>>
>>> Fixes: 3950f2485bbc ("x86/x2APIC: defer probe until after IOMMU ACPI table parsing")
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> Of course it would have been quite a bit easier to notice this issue if
>>> radix_tree_insert() wouldn't work fine ahead of radix_tree_init() being
>>> invoked for a given radix tree, when the index inserted at is 0.
>>>
>>> While hunting down various other dead paths to actually find the root
>>> cause, it occurred to me that it's probably not a good idea to fully
>>> disallow config space writes for r/o devices: Dom0 won't be able to size
>>> their BARs (luckily the IOMMU "devices" don't have any, but e.g. serial
>>> ones generally will have at least one), for example. Without being able
>>> to size BARs it also will likely be unable to correctly account for the
>>> address space taken by these BARs. However, outside of vPCI it's not
>>> really clear to me how we could reasonably emulate such BAR sizing
>>> writes - we can't, after all, allow Dom0 to actually write to the
>>> underlying physical registers, yet we don't intercept reads (i.e. we
>>> can't mimic expected behavior then).
>>>
>>> --- a/xen/arch/x86/x86_64/mmconfig-shared.c
>>> +++ b/xen/arch/x86/x86_64/mmconfig-shared.c
>>> @@ -402,8 +402,6 @@ void __init acpi_mmcfg_init(void)
>>>  {
>>>      bool valid = true;
>>>  
>>> -    pci_segments_init();
>>> -
>>>      /* MMCONFIG disabled */
>>>      if ((pci_probe & PCI_PROBE_MMCONF) == 0)
>>>          return;
>>> --- a/xen/drivers/passthrough/x86/iommu.c
>>> +++ b/xen/drivers/passthrough/x86/iommu.c
>>> @@ -55,6 +55,8 @@ void __init acpi_iommu_init(void)
>>>  {
>>>      int ret = -ENODEV;
>>>  
>>> +    pci_segments_init();
>>> +
>>>      if ( !iommu_enable && !iommu_intremap )
>>>          return;
>>>  
>>>
>> I can't help but feel this is taking a bad problem and not making it any
>> better.
>>
>> pci_segments_init() is even less (obviously) relevant in
>> apci_iommu_init() than it is in acpi_mmcfg_init(), and given the
>> fine-grain Kconfig-ing going on, is only one small step from
>> accidentally being compiled out.
> The alternative I did consider was to move the call into __start_xen()
> itself. Anything going beyond that looks more intrusive than we'd like
> it at this point of the release cycle.

Moving into __start_xen() would be ok if we think we're getting too
close to the release.  It makes it clearer that there is explicit
ordering necessary.

>
>> ARM is in a bad state too, with this initialisation even being behind
>> the PCI Passthrough cmdline option.
>>
>> IMO there are two problems here; one as you pointed out
>> (radix_tree_insert() doesn't fail), and that PCI handling requires
>> explicit initialisation to begin with.
>>
>> Looking through radix tree, it wouldn't be hard to create a
>> RADIX_TREE_INIT macro to allow initialisation at compile time for
>> suitable objects (pci_segments and acpi_ivrs currently).
>>
>> That involves exporting rcu_node_{alloc,free}(), although the last
>> caller of radix_tree_set_alloc_callbacks() was dropped when TMEM went,
>> so we could reasonably remove that infrastructure too, at which point
>> radix_tree_init() is a simple zero of the structure.
> Yes, seeing that this was even an extension of ours (i.e. Linux doesn't
> have such), it's certainly worth getting rid of. If nothing else, then
> for the two cf_check annotations that's we'd then be able to drop. I'll
> make a patch.

Oh, even better. 

>
>> Dealing with alloc_pseg(0) is harder.  As we never free the PCI
>> segments, we could just opencode the radix_tree_root of height=1 with a
>> static pseg0 structure, and that would drop the need for
>> pci_segemnts_init() completely.
> I'm afraid this would end up being too much open-coding for my taste.

I didn't much like the suggestion either.

> I'd put this differently: Unlike the radix tree initialization, the
> setting up of segment 0 isn't a prereq to acpi_iommu_init(). We could
> keep acpi_mmcfg_init() doing that, by way of calling pci_add_segment(0)
> (and that would simply be a no-op if acpi_iommu_init() ended up
> introducing segment 0 already).

That might be ok.

~Andrew

Re: [PATCH for-4.20? 1/3] AMD/IOMMU: drop stray MSI enabling

[Jan Beulich]

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

Re: [PATCH for-4.20 2/3] x86/PCI: init segments earlier

[Jan Beulich]

On 03.02.2025 15:23, Andrew Cooper wrote:
> On 03/02/2025 1:03 pm, Jan Beulich wrote:
>> On 03.02.2025 14:00, Jan Beulich wrote:
>>> On 03.02.2025 13:45, Roger Pau Monné wrote:
>>>> On Thu, Jan 30, 2025 at 12:12:31PM +0100, Jan Beulich wrote:
>>>>> --- a/xen/arch/x86/x86_64/mmconfig-shared.c
>>>>> +++ b/xen/arch/x86/x86_64/mmconfig-shared.c
>>>>> @@ -402,8 +402,6 @@ void __init acpi_mmcfg_init(void)
>>>>>  {
>>>>>      bool valid = true;
>>>>>  
>>>>> -    pci_segments_init();
>>>>> -
>>>>>      /* MMCONFIG disabled */
>>>>>      if ((pci_probe & PCI_PROBE_MMCONF) == 0)
>>>>>          return;
>>>>> --- a/xen/drivers/passthrough/x86/iommu.c
>>>>> +++ b/xen/drivers/passthrough/x86/iommu.c
>>>>> @@ -55,6 +55,8 @@ void __init acpi_iommu_init(void)
>>>>>  {
>>>>>      int ret = -ENODEV;
>>>>>  
>>>>> +    pci_segments_init();
>>>> My preference might be to just place the pci_segments_init() call in
>>>> __start_xen(),
>>> As said in reply to Andrew - I was considering doing so as an alternative
>>> to the moving done here. I can certainly do so, in case some non-negative
>>> reply comes back from him.
>> Oh, and: With further adjustments following from what Andrew had outlined,
>> I'm actually moving the invocation of what was pci_segments_init() back to
>> where it's now. (Which doesn't mean that couldn't be done from
>> __start_xen(); just mentioning it.)
> 
> The name acpi_mmcfg_init() at least has a PCI implication, given mmcfg.
> 
> I know it's late in 4.20, and moving pci_segments_init() would be
> acceptable at this juncture.
> 
> However, if you're making progress with improving radix trees, I think
> that would be a better approach and probably fine to be considered at
> this point.

Well, let me submit v2 then with all those new patches.

Jan

Re: [PATCH for-4.20? 1/3] AMD/IOMMU: drop stray MSI enabling

[Andrew Cooper]

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