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.

--- 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,

Hi Jan,

On 26/02/2025 11: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.

Looking at the discussion between you and Stewart, it looks like we need 
to keep the call for now. Once we have the downstream patches merged, we 
can remove it. So:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

On 26.02.2025 12: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>

Arm maintainers: May I ask for an ack (or ...

> ---
> 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.

... otherwise) here please?

Jan

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

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.

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.