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

[Zhenzhong Duan <zhenzhong.duan@xxxxxxxxxx>]

On 2018/8/20 15:45, Jan Beulich wrote:
> > > > On 20.08.18 at 05:38, <zhenzhong.duan@xxxxxxxxxx> wrote:
> > On 2018/8/17 20:28, Jan Beulich wrote:
> > > > > > On 17.08.18 at 09:01, <zhenzhong.duan@xxxxxxxxxx> wrote:
> > > > pci_conf_read8() needs pci mmcfg mapping to work on multiple pci segments
> > > > system such as HPE Superdome-Flex.
> > > >
> > > > Move acpi_mmcfg_init() call in acpi_boot_init() before calling
> > > > acpi_parse_dmar() so that when pci_conf_read8() is called in
> > > > acpi_parse_dev_scope(), we already have the mapping set up.
> > > >
> > > > acpi_mmcfg_init() only setup mmcfg mapping and has some global variables
> > > > initialized so there is no hazard to move it ahead.
> > > I'm afraid this is a gross understatement. "Setup mappings"
> > > alone is already putting such movement under question. Have
> > > you checked explicitly that the initialization needed for this
> > > setting up of mappings, if any, has actually occurred before
> > > the new point the function gets called?
> > >
> > > In particular, mmio_ro_ranges looks to get set up only after
> > > the call to acpi_boot_init(). I guess you didn't see a crash
> > > solely because you also move the invocation across the call
> > > to zap_low_mappings().
> > >
> > > Similary pci_mmcfg_check_hostbridge() doesn't look all that
> > > trivial.
> > >
> > > Please may I ask that you be quite a bit more careful here,
> > > even more so when you've been warned already?
> > >
> > > > Meanwhile from its
> > > > name, acpi_boot_init() is a proper place to call it.
> > > >
> > > > The alternative is moving the acpi_parse_dev_scope() call to a later point
> > > > after acpi_mmcfg_init(). But acpi_parse_one_drhd(), acpi_parse_one_rmrr()
> > > > and acpi_parse_one_atsr() all called acpi_parse_dev_scope() as their main
> > > > job. Splitting those functions to two pieces looks less optimal and
> > > > meaningless.
> > > Certainly not meaningless; I'm also not sure why you consider
> > > the device scope parsing their main job. It's perhaps their
> > > most involved part, but the fact alone that for the purposes
> > > here we could probably get away without that part already
> > > suggests to me that this is a secondary (yet necessary) aspect.
> > >
> > > Furthermore MMCFG will continue to not work this early (or
> > > more precisely not at all until Dom0 boot has progressed far
> > > enough) if the range(s) isn't/aren't marked reserved in E820.
> > > It would be worthwhile saying half a sentence to this effect
> > > in the description.
> > I meet some difficulty moving dev scope parsing to later point. Please
> > suggest.
> >
> > First, acpi_parse_dev_scope() may fail and disable_all_dmar_units() is
> > called to free all dmar related allocations which is already used by
> > iommu_supports_eim().
> Hmm, right - iommu_supports_eim() indeed requires device scope parsing
> to have happened.
>
> > I'm thinking about moving below piece of code earlier too, and I checked
> > pci_mmcfg_check_hostbridge() carefully, it's secure, what do you think
> > about that?
> >
> >       mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges",
> >                                     RANGESETF_prettyprint_hex);
> That's a reasonable thing to do, and is (as pointed out) a necessary
> prereq. But to be very clear - you'll also have to prove it's sufficient,
> and for that it doesn't suffice to consider pci_mmcfg_check_hostbridge()
> alone.
Not sure how to prove, I checked over acpi_mmcfg_init() carefully, 
acpi_disabled and DMI info are used and they are initialized earlier 
than acpi_dmar_init() call, I only found mmio_ro_ranges need to be moved.
Thanks
Zhenzhong

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