| Message ID | 3abd753b-b1f2-499a-8c02-6b6d64a78a17@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | AMD/IOMMU: assorted corrections | expand |
On 03/02/2025 4:27 pm, Jan Beulich wrote: > Have callers invoke pci_add_segment() directly instead. On x86 move the > invocation back to acpi_mmcfg_init(), where it was prior to ???????????? > ("x86/PCI: init segments earlier"). > --- Need a SoB. I definitely prefer this version of the fix, but I think it would be better if patch 2 were merged into this. I know it means backporting the cleanup, but it is more robust IMO. ~Andrew
On Mon, Feb 03, 2025 at 05:27:24PM +0100, Jan Beulich wrote: > Have callers invoke pci_add_segment() directly instead. On x86 move the > invocation back to acpi_mmcfg_init(), where it was prior to ???????????? > ("x86/PCI: init segments earlier"). > --- > 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,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. Thanks, Roger.
On 03.02.2025 18:04, Andrew Cooper wrote: > On 03/02/2025 4:27 pm, Jan Beulich wrote: >> Have callers invoke pci_add_segment() directly instead. On x86 move the >> invocation back to acpi_mmcfg_init(), where it was prior to ???????????? >> ("x86/PCI: init segments earlier"). >> --- > > Need a SoB. Oops. > I definitely prefer this version of the fix, but I think it would be > better if patch 2 were merged into this. See the reply on the cover letter sub-thread. I'll be dropping patch 2 altogether, with patch 5 (perhaps moved ahead of patch 4) then being the actual bugfix. Jan
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. Jan
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. 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. Jan
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.
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
--- 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,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"); + /* MMCONFIG disabled */ if ((pci_probe & PCI_PROBE_MMCONF) == 0) return; --- 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/drivers/passthrough/x86/iommu.c +++ b/xen/drivers/passthrough/x86/iommu.c @@ -55,8 +55,6 @@ void __init acpi_iommu_init(void) { int ret = -ENODEV; - pci_segments_init(); - if ( !iommu_enable && !iommu_intremap ) return; --- 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,
Have callers invoke pci_add_segment() directly instead. On x86 move the invocation back to acpi_mmcfg_init(), where it was prior to ???????????? ("x86/PCI: init segments earlier"). --- v2: New.