| Message ID | 4ada4343-c65b-456d-b0c2-9ae59937aaff@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: drop pci_segments_init() | expand |
On Wed, Feb 26, 2025 at 12:38:21PM +0100, Jan Beulich wrote: > Have callers invoke pci_add_segment() directly instead: With radix tree > initialization moved out of the function, its name isn't quite > describing anymore what it actually does. > > On x86 move the logic into __start_xen() itself, to reduce the risk of > re-introducing ordering issues like the one which was addressed by > 26fe09e34566 ("radix-tree: introduce RADIX_TREE{,_INIT}()"). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
On 2/26/25 06:38, Jan Beulich wrote: > Have callers invoke pci_add_segment() directly instead: With radix tree > initialization moved out of the function, its name isn't quite > describing anymore what it actually does. > > On x86 move the logic into __start_xen() itself, to reduce the risk of > re-introducing ordering issues like the one which was addressed by > 26fe09e34566 ("radix-tree: introduce RADIX_TREE{,_INIT}()"). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > This is entirely optional and up for discussion. There certainly also is > an argument towards keeping the function. Otoh on Arm there is the still > open question whether segment 0 really is kind of special there (as it > is on x86, largely for historical reasons), or whether the code can be > dropped there altogether. Segment 0 is not special on Arm as far as I'm aware. You can have a perfectly functioning system with only, say, segment 1, for example: (XEN) ==== PCI devices ==== (XEN) ==== segment 0001 ==== (XEN) 0001:00:01.0 - d0 - node -1 (XEN) 0001:00:00.0 - d0 - node -1 Segment numbers can be arbitrarily chosen by specifying the linux,pci-domain device tree property. > --- > v4: Move x86 logic into __start_xen() itself. > v3: Adjust description to account for and re-base over dropped earlier > patch. > v2: New. > > --- a/xen/arch/arm/pci/pci.c > +++ b/xen/arch/arm/pci/pci.c > @@ -88,7 +88,8 @@ static int __init pci_init(void) > if ( !pci_passthrough_enabled ) > return 0; > > - pci_segments_init(); > + if ( pci_add_segment(0) ) > + panic("Could not initialize PCI segment 0\n"); IMO it's okay to remove the call here since there is already a call to pci_add_segment() in xen/arch/arm/pci/pci-host-common.c:pci_host_common_probe() If there happens to be an Arm SoC with segment number quirks, that could be worked out in a SoC-specific xen/arch/arm/pci/pci-host-*.c.
On 26.02.2025 20:57, Stewart Hildebrand wrote: > On 2/26/25 06:38, Jan Beulich wrote: >> Have callers invoke pci_add_segment() directly instead: With radix tree >> initialization moved out of the function, its name isn't quite >> describing anymore what it actually does. >> >> On x86 move the logic into __start_xen() itself, to reduce the risk of >> re-introducing ordering issues like the one which was addressed by >> 26fe09e34566 ("radix-tree: introduce RADIX_TREE{,_INIT}()"). >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> This is entirely optional and up for discussion. There certainly also is >> an argument towards keeping the function. Otoh on Arm there is the still >> open question whether segment 0 really is kind of special there (as it >> is on x86, largely for historical reasons), or whether the code can be >> dropped there altogether. > > Segment 0 is not special on Arm as far as I'm aware. You can have a > perfectly functioning system with only, say, segment 1, for example: > > (XEN) ==== PCI devices ==== > (XEN) ==== segment 0001 ==== > (XEN) 0001:00:01.0 - d0 - node -1 > (XEN) 0001:00:00.0 - d0 - node -1 > > Segment numbers can be arbitrarily chosen by specifying the > linux,pci-domain device tree property. Right, that was the vague understanding I had. >> --- a/xen/arch/arm/pci/pci.c >> +++ b/xen/arch/arm/pci/pci.c >> @@ -88,7 +88,8 @@ static int __init pci_init(void) >> if ( !pci_passthrough_enabled ) >> return 0; >> >> - pci_segments_init(); >> + if ( pci_add_segment(0) ) >> + panic("Could not initialize PCI segment 0\n"); > > IMO it's okay to remove the call here since there is already a call to > pci_add_segment() in > xen/arch/arm/pci/pci-host-common.c:pci_host_common_probe() Is there? I can't see one, so maybe you're working from a tree with extra patches applied? Jan
On 2/27/25 01:58, Jan Beulich wrote: > On 26.02.2025 20:57, Stewart Hildebrand wrote: >> On 2/26/25 06:38, Jan Beulich wrote: >>> Have callers invoke pci_add_segment() directly instead: With radix tree >>> initialization moved out of the function, its name isn't quite >>> describing anymore what it actually does. >>> >>> On x86 move the logic into __start_xen() itself, to reduce the risk of >>> re-introducing ordering issues like the one which was addressed by >>> 26fe09e34566 ("radix-tree: introduce RADIX_TREE{,_INIT}()"). >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> This is entirely optional and up for discussion. There certainly also is >>> an argument towards keeping the function. Otoh on Arm there is the still >>> open question whether segment 0 really is kind of special there (as it >>> is on x86, largely for historical reasons), or whether the code can be >>> dropped there altogether. >> >> Segment 0 is not special on Arm as far as I'm aware. You can have a >> perfectly functioning system with only, say, segment 1, for example: >> >> (XEN) ==== PCI devices ==== >> (XEN) ==== segment 0001 ==== >> (XEN) 0001:00:01.0 - d0 - node -1 >> (XEN) 0001:00:00.0 - d0 - node -1 >> >> Segment numbers can be arbitrarily chosen by specifying the >> linux,pci-domain device tree property. > > Right, that was the vague understanding I had. > >>> --- a/xen/arch/arm/pci/pci.c >>> +++ b/xen/arch/arm/pci/pci.c >>> @@ -88,7 +88,8 @@ static int __init pci_init(void) >>> if ( !pci_passthrough_enabled ) >>> return 0; >>> >>> - pci_segments_init(); >>> + if ( pci_add_segment(0) ) >>> + panic("Could not initialize PCI segment 0\n"); >> >> IMO it's okay to remove the call here since there is already a call to >> pci_add_segment() in >> xen/arch/arm/pci/pci-host-common.c:pci_host_common_probe() > > Is there? I can't see one, so maybe you're working from a tree with extra > patches applied? Ah, you're right, sorry, I was looking at a downstream tree. The associated segment would be added in Xen upon the first time Dom0 calls PHYSDEVOP_pci_device_add.
--- a/xen/arch/arm/pci/pci.c +++ b/xen/arch/arm/pci/pci.c @@ -88,7 +88,8 @@ static int __init pci_init(void) if ( !pci_passthrough_enabled ) return 0; - pci_segments_init(); + if ( pci_add_segment(0) ) + panic("Could not initialize PCI segment 0\n"); if ( acpi_disabled ) return dt_pci_init(); --- 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/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1898,6 +1898,13 @@ void asmlinkage __init noreturn __start_ setup_system_domains(); /* + * Ahead of any ACPI table parsing make sure we have control structures + * for PCI segment 0. + */ + if ( pci_add_segment(0) ) + panic("Could not initialize PCI segment 0\n"); + + /* * IOMMU-related ACPI table parsing has to happen before APIC probing, for * check_x2apic_preenabled() to be able to observe respective findings, in * particular iommu_intremap having got turned off. --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -127,12 +127,6 @@ static int pci_segments_iterate( return rc; } -void __init pci_segments_init(void) -{ - if ( !alloc_pseg(0) ) - panic("Could not initialize PCI segment 0\n"); -} - int __init pci_add_segment(u16 seg) { return alloc_pseg(seg) ? 0 : -ENOMEM; --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -219,7 +219,6 @@ void setup_hwdom_pci_devices(struct doma int (*handler)(uint8_t devfn, struct pci_dev *pdev)); int pci_release_devices(struct domain *d); -void pci_segments_init(void); int pci_add_segment(u16 seg); const unsigned long *pci_get_ro_map(u16 seg); int pci_add_device(u16 seg, u8 bus, u8 devfn,
Have callers invoke pci_add_segment() directly instead: With radix tree initialization moved out of the function, its name isn't quite describing anymore what it actually does. On x86 move the logic into __start_xen() itself, to reduce the risk of re-introducing ordering issues like the one which was addressed by 26fe09e34566 ("radix-tree: introduce RADIX_TREE{,_INIT}()"). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- This is entirely optional and up for discussion. There certainly also is an argument towards keeping the function. Otoh on Arm there is the still open question whether segment 0 really is kind of special there (as it is on x86, largely for historical reasons), or whether the code can be dropped there altogether. --- v4: Move x86 logic into __start_xen() itself. v3: Adjust description to account for and re-base over dropped earlier patch. v2: New.