Re: [Xen-devel] [PATCH] x86/mmcfg/drhd: Move acpi_mmcfg_init() before calling acpi_parse_dmar()

[Zhenzhong Duan <zhenzhong.duan@xxxxxxxxxx>]

On 2018/8/16 15:10, Jan Beulich wrote:
> > > > On 16.08.18 at 07:10, <zhenzhong.duan@xxxxxxxxxx> wrote:
> > On a multiple pci segment system such as HPE Superdome-Flex, pci config space
> > from nonzero segment is accessed with mmcfg during acpi parsing DMAR region.
> First of all - can you please write a little more helpful (to reviewers)
> patch description. I had to hunt down the config space accesses
> instead of you clearly saying where they are (and why they are
> unavoidably there).
Sorry, I'll improve it in v2
> Furthermore you also move the invocation of pt_pci_init(), with no
> explanation at all. I did not want to invest the time to understand
> why that's needed.
I'll add it in v2. I moved pt_pci_init() ahead because pci_add_segment() 
depending on pci_segments radix tree being initialized.
acpi_mmcfg_init
  ->acpi_parse_mcfg
    ->pci_add_segment
> > We need to setup mmcfg mapping before that or else drhd isn't correctly setup
> > and devices under it fail to load in dom0.
> That's the improvement side of the change. Mind also saying a word
> on why the reordering won't break any other dependencies? After all
> you move up the functions across dozens of other ones, not the least
> far ahead of the point where IRQs get enabled the first time.
pt_pci_init() initialize pci_segments radix tree, acpi_mmcfg_init() 
initialize pci_mmcfg_virt[] and setup mmcfg mapping in 
pci_mmcfg_virt[idx].virt. I thought it's ok to just have these 
structures initialzed earlier.
> Have you investigated the alternative of deferring acpi_dmar_init()
> to a later point, or at least the part of it that needs to do PCI
> config space accesses? I'm not currently convinced the device scope
> parsing needs to happen that early: Neither iommu_supports_eim()
> nor iommu_enable_x2apic_IR() look to depend on that information
> at the first glance, and I think these are the routines that require
> (part of) the DMAR parsing to happen early.
I moved acpi_mmcfg_init() ahead of acpi_boot_init() because below code 
sequence depending on pci_mmcfg_virt being correctly setup.
acpi_dmar_init
  ->acpi_parse_dmar
    ->acpi_parse_one_drhd
      ->acpi_parse_dev_scope
        ->pci_conf_read8
          ->pci_mmcfg_read
            ->pci_dev_base
              ->get_virt
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -1493,6 +1493,10 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >   
> >       generic_apic_probe();
> >   
> > +    pt_pci_init();
> > +
> > +    acpi_mmcfg_init();
> > +
> >       acpi_boot_init();
> With the dependency being _in_ acpi_boot_init(), the invocation of
> acpi_mmcfg_init() should now be moved there.
Yes, I feel better to move pt_pci_init() and acpi_mmcfg_init() in 
acpi_boot_init() before acpi_mmcfg_init(). Any more comments?
Thanks
Zhenzhong

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel