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

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

[Zhenzhong Duan]

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

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Tested-by: Gopalasetty, Manoj <manoj.gopalasetty@hpe.com>
---
 xen/arch/x86/acpi/boot.c |    2 ++
 xen/arch/x86/setup.c     |    2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c
index 8e6c96d..e89c2e9 100644
--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -724,6 +724,8 @@ int __init acpi_boot_init(void)
 
 	acpi_table_parse(ACPI_SIG_HPET, acpi_parse_hpet);
 
+	acpi_mmcfg_init();
+
 	acpi_dmar_init();
 
 	erst_init();
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index d5cc584..9d0cd3b 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1598,8 +1598,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     vesa_mtrr_init();
 
-    acpi_mmcfg_init();
-
     early_msi_init();
 
     iommu_setup();    /* setup iommu if available */
-- 
1.7.3

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

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

[Jan Beulich]

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



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

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

[Zhenzhong Duan]

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

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

[Jan Beulich]

>>> On 20.08.18 at 05:38, <zhenzhong.duan@oracle.com> wrote:
> 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.
> 
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Zhenzhong Duan]

On 2018/8/20 15:45, Jan Beulich wrote:
>>>> On 20.08.18 at 05:38, <zhenzhong.duan@oracle.com> wrote:
>> 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.
>>
>> 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.
Not sure how to prove, I checked over acpi_mmcfg_init() carefully, 
acpi_disabled and DMI info are used and they are initialized earlier 
than acpi_dmar_init() call, I only found mmio_ro_ranges need to be moved.

Thanks
Zhenzhong

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

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

[Jan Beulich]

>>> On 20.08.18 at 11:38, <zhenzhong.duan@oracle.com> wrote:
> On 2018/8/20 15:45, Jan Beulich wrote:
>>>>> On 20.08.18 at 05:38, <zhenzhong.duan@oracle.com> wrote:
>>> 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.
> Not sure how to prove, I checked over acpi_mmcfg_init() carefully, 
> acpi_disabled and DMI info are used and they are initialized earlier 
> than acpi_dmar_init() call, I only found mmio_ro_ranges need to be moved.

But that's only half of it: Checking just acpi_mmcfg_init() is insufficient.
You also need to check everything between the old and new call sites.
And the result of this checking wants to be summarized (read: in a
brief but nevertheless sufficient form) in the patch description.

Jan



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

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

[zhenzhong.duan]

[-- Attachment #1.1: Type: text/plain, Size: 2461 bytes --]

Ok, that make sense for me. Thanks for your detailed explanation, I will rewrite the patch next week.

Sent from mobile

2018年8月17日 20:28于 Jan Beulich <JBeulich@suse.com>写道：
>
> >>> 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 
>
>

[-- Attachment #1.2: Type: text/html, Size: 3073 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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