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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.02.2025 09:56, Roger Pau Monné wrote:
> 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.

Let's continue this on v3, where I'm extending remarks on this change.
An option is to simply leave out this patch altogether. Then a follow-
on option would be to purge the call to pci_segments_init() with an
entirely different justification (e.g. yours).

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

That hypercall was added solely for the purpose of reporting resource
reservation status. While we could decide to re-purpose it to also
record the segment, that wasn't the goal so far.

Jan