[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

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

Second, In acpi_parse_one_drhd(), acpi_register_drhd_unit() should not be moved later so that acpi_drhd_units is setup early, but it's called if pci_device_detect() return true, pci_device_detect() depends on mmcfg to work which isn't setup mapping yet.

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);


Thanks
Zhenzhong

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.