Re: [PATCH v2 for-4.20? 6/6] PCI: drop pci_segments_init()

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Feb 04, 2025 at 08:51:10AM +0100, Jan Beulich wrote:
> On 04.02.2025 08:45, Jan Beulich wrote:
> > On 03.02.2025 18:18, Roger Pau Monné wrote:
> >> On Mon, Feb 03, 2025 at 05:27:24PM +0100, Jan Beulich wrote:
> >>> --- a/xen/arch/x86/x86_64/mmconfig-shared.c
> >>> +++ b/xen/arch/x86/x86_64/mmconfig-shared.c
> >>> @@ -402,6 +402,9 @@ void __init acpi_mmcfg_init(void)
> >>>  {
> >>>      bool valid = true;
> >>>  
> >>> +    if ( pci_add_segment(0) )
> >>> +        panic("Could not initialize PCI segment 0\n");
> >>
> >> Do you still need the pci_add_segment() call here?
> >>
> >> pci_add_device() will already add the segments as required, and
> >> acpi_parse_mcfg() does also add the segments described in the MCFG.
> > 
> > Well, in principle you're right. It's more the action in case of
> > failure that makes me want to keep it: We won't fare very well on
> > about every system if we can't register segment 0.

pci_add_segment() should only fail due to being out of memory, and I'm
quite sure if pci_add_segment() was to fail here due to out of memory
issues Xen won't be able to complete booting anyway.

Note the usage of "should only fail", as it's possible for
radix_tree_insert() to return -EEXIST, but that shouldn't be possible
given alloc_pseg() checks whether the segment is already added.

> Thinking about it: Your question may be more applicable on Arm, as I'm
> entirely uncertain whether there segment 0 is always going to be needed.
> I could well imagine system designers deliberately putting all the
> devices elsewhere. Segment 0 always being in use on x86 is more a
> heritage thing, after all.

I guess I would leave that one to the Arm maintainers, but from my
knowledge I got the impression is fairly common for Arm to have
multiple segments, not sure whether it's also common to start at
segment 0.

I'm not strongly opposed to leaving the pci_add_segment(0) call on
x86, but I would recommend prepending a small comment to note adding
the segment is not strictly required; it's just done for better error
reporting, as other callers that add PCI segments might silently
ignore the failure to add segment 0.

An unrelated question: looking at MCFG handling I've noticed that
calling PHYSDEVOP_pci_mmcfg_reserved doesn't seem to result in the
segment being added.  Is it on purpose that pci_mmcfg_reserved()
doesn't attempt to allocate the hardware domain discovered segment?

Thanks, Roger.