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