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

Jan



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