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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan



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