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

[Zhenzhong Duan <zhenzhong.duan@oracle.com>]

On 2018/8/17 20:28, Jan Beulich wrote:
>>>> On 17.08.18 at 09:01, <zhenzhong.duan@oracle.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel