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.

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.
---
v3: Adjust description to accont 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,7 +402,8 @@ void __init acpi_mmcfg_init(void)
 {
     bool valid = true;
 
-    pci_segments_init();
+    if ( pci_add_segment(0) )
+        panic("Could not initialize PCI segment 0\n");
 
     /* MMCONFIG disabled */
     if ((pci_probe & PCI_PROBE_MMCONF) == 0)
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -122,12 +122,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
@@ -214,7 +214,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 Tue, Feb 04, 2025 at 02:04:35PM +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.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

IMO I would rather not add the segment here, and just make sure that
all callers that add segments have proper error reporting when it
fails.  Maybe alloc_pseg() should gain a printk on failure?

Thanks, Roger.

On 06.02.2025 16:08, Roger Pau Monné wrote:
> On Tue, Feb 04, 2025 at 02:04:35PM +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.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> IMO I would rather not add the segment here, and just make sure that
> all callers that add segments have proper error reporting when it
> fails.

Maybe. Yet then things may (on x86) work fine with secondary segments not
properly working. A log from one of the few multi-segment systems that I
had seen data from suggested that none of the devices on the secondary
segment were actually used by anything. This was, if I'm not mistaken,
the underlying reason why (on x86) we demand segment 0 to have proper
representation, but do things best effort only for other segments. Which
isn't to say that we can't change things and do better.

>  Maybe alloc_pseg() should gain a printk on failure?

Not sure that would buy us much, especially if (on x86) it's seg 0 which
is affected.

For the patch at hand - do you then suggest to simply drop it? The plan
here wasn't to re-work what we do, just tidy slightly how we do things.

Jan

On Mon, Feb 10, 2025 at 11:01:04AM +0100, Jan Beulich wrote:
> On 06.02.2025 16:08, Roger Pau Monné wrote:
> > On Tue, Feb 04, 2025 at 02:04:35PM +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.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > IMO I would rather not add the segment here, and just make sure that
> > all callers that add segments have proper error reporting when it
> > fails.
> 
> Maybe. Yet then things may (on x86) work fine with secondary segments not
> properly working. A log from one of the few multi-segment systems that I
> had seen data from suggested that none of the devices on the secondary
> segment were actually used by anything. This was, if I'm not mistaken,
> the underlying reason why (on x86) we demand segment 0 to have proper
> representation, but do things best effort only for other segments. Which
> isn't to say that we can't change things and do better.
> 
> >  Maybe alloc_pseg() should gain a printk on failure?
> 
> Not sure that would buy us much, especially if (on x86) it's seg 0 which
> is affected.
> 
> For the patch at hand - do you then suggest to simply drop it? The plan
> here wasn't to re-work what we do, just tidy slightly how we do things.

I feel like acpi_mmcfg_init() is an obscure place to do this.  It
won't be strange to shuffle that call around and forgot it's also
adding segment 0.

I would prefer if such allocation of segment 0 was done in
__start_xen(), as that makes it much easier to identify dependencies
and prevent such kind of re-ordering mistakes.

Thanks, Roger.