| Message ID | 940ccd1b-9ad8-4b68-a035-36f45326872b@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | AMD/IOMMU: assorted corrections | expand |
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
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.
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
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.
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
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
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
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
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
--- 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;
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).