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

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

[Zhenzhong Duan]

On a multiple pci segment system such as HPE Superdome-Flex, pci config space
from nonzero segment is accessed with mmcfg during acpi parsing DMAR region.

We need to setup mmcfg mapping before that or else drhd isn't correctly setup
and devices under it fail to load in dom0.

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

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 8301de8..9af7426 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1493,6 +1493,10 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     generic_apic_probe();
 
+    pt_pci_init();
+
+    acpi_mmcfg_init();
+
     acpi_boot_init();
 
     if ( smp_found_config )
@@ -1596,12 +1600,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     local_irq_enable();
 
-    pt_pci_init();
-
     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] x86/mmcfg/drhd: Move acpi_mmcfg_init() before calling acpi_parse_dmar()

[Jan Beulich]

>>> On 16.08.18 at 07:10, <zhenzhong.duan@oracle.com> wrote:
> On a multiple pci segment system such as HPE Superdome-Flex, pci config space
> from nonzero segment is accessed with mmcfg during acpi parsing DMAR region.

First of all - can you please write a little more helpful (to reviewers)
patch description. I had to hunt down the config space accesses
instead of you clearly saying where they are (and why they are
unavoidably there).

Furthermore you also move the invocation of pt_pci_init(), with no
explanation at all. I did not want to invest the time to understand
why that's needed.

> We need to setup mmcfg mapping before that or else drhd isn't correctly setup
> and devices under it fail to load in dom0.

That's the improvement side of the change. Mind also saying a word
on why the reordering won't break any other dependencies? After all
you move up the functions across dozens of other ones, not the least
far ahead of the point where IRQs get enabled the first time.

Have you investigated the alternative of deferring acpi_dmar_init()
to a later point, or at least the part of it that needs to do PCI
config space accesses? I'm not currently convinced the device scope
parsing needs to happen that early: Neither iommu_supports_eim()
nor iommu_enable_x2apic_IR() look to depend on that information
at the first glance, and I think these are the routines that require
(part of) the DMAR parsing to happen early.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1493,6 +1493,10 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>      generic_apic_probe();
>  
> +    pt_pci_init();
> +
> +    acpi_mmcfg_init();
> +
>      acpi_boot_init();

With the dependency being _in_ acpi_boot_init(), the invocation of
acpi_mmcfg_init() should now be moved there.

Jan



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

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

[Zhenzhong Duan]

On 2018/8/16 15:10, Jan Beulich wrote:
>>>> On 16.08.18 at 07:10, <zhenzhong.duan@oracle.com> wrote:
>> On a multiple pci segment system such as HPE Superdome-Flex, pci config space
>> from nonzero segment is accessed with mmcfg during acpi parsing DMAR region.
> 
> First of all - can you please write a little more helpful (to reviewers)
> patch description. I had to hunt down the config space accesses
> instead of you clearly saying where they are (and why they are
> unavoidably there).
Sorry, I'll improve it in v2
> 
> Furthermore you also move the invocation of pt_pci_init(), with no
> explanation at all. I did not want to invest the time to understand
> why that's needed.
I'll add it in v2. I moved pt_pci_init() ahead because pci_add_segment() 
depending on pci_segments radix tree being initialized.
acpi_mmcfg_init
   ->acpi_parse_mcfg
     ->pci_add_segment
> 
>> We need to setup mmcfg mapping before that or else drhd isn't correctly setup
>> and devices under it fail to load in dom0.
> 
> That's the improvement side of the change. Mind also saying a word
> on why the reordering won't break any other dependencies? After all
> you move up the functions across dozens of other ones, not the least
> far ahead of the point where IRQs get enabled the first time.
pt_pci_init() initialize pci_segments radix tree, acpi_mmcfg_init() 
initialize pci_mmcfg_virt[] and setup mmcfg mapping in 
pci_mmcfg_virt[idx].virt. I thought it's ok to just have these 
structures initialzed earlier.
> 
> Have you investigated the alternative of deferring acpi_dmar_init()
> to a later point, or at least the part of it that needs to do PCI
> config space accesses? I'm not currently convinced the device scope
> parsing needs to happen that early: Neither iommu_supports_eim()
> nor iommu_enable_x2apic_IR() look to depend on that information
> at the first glance, and I think these are the routines that require
> (part of) the DMAR parsing to happen early.
I moved acpi_mmcfg_init() ahead of acpi_boot_init() because below code 
sequence depending on pci_mmcfg_virt being correctly setup.
acpi_dmar_init
   ->acpi_parse_dmar
     ->acpi_parse_one_drhd
       ->acpi_parse_dev_scope
         ->pci_conf_read8
           ->pci_mmcfg_read
             ->pci_dev_base
               ->get_virt
> 
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1493,6 +1493,10 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>   
>>       generic_apic_probe();
>>   
>> +    pt_pci_init();
>> +
>> +    acpi_mmcfg_init();
>> +
>>       acpi_boot_init();
> 
> With the dependency being _in_ acpi_boot_init(), the invocation of
> acpi_mmcfg_init() should now be moved there.
Yes, I feel better to move pt_pci_init() and acpi_mmcfg_init() in 
acpi_boot_init() before acpi_mmcfg_init(). Any more comments?

Thanks
Zhenzhong

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

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

[Zhenzhong Duan]

On 2018/8/16 17:13, Zhenzhong Duan wrote:
>>
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -1493,6 +1493,10 @@ void __init noreturn __start_xen(unsigned long 
>>> mbi_p)
>>>       generic_apic_probe();
>>> +    pt_pci_init();
>>> +
>>> +    acpi_mmcfg_init();
>>> +
>>>       acpi_boot_init();
>>
>> With the dependency being _in_ acpi_boot_init(), the invocation of
>> acpi_mmcfg_init() should now be moved there.
> Yes, I feel better to move pt_pci_init() and acpi_mmcfg_init() in 
> acpi_boot_init() before acpi_mmcfg_init(). Any more comments?
I see acpi_boot_init() is empty func when CONFIG_ACPI_BOOT isn't set. Do 
we support disabling this config option? If yes, I think above change 
will break non-acpi case.

Thanks
Zhenzhong

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

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

[Jan Beulich]

>>> On 16.08.18 at 11:13, <zhenzhong.duan@oracle.com> wrote:
> On 2018/8/16 15:10, Jan Beulich wrote:
>> Have you investigated the alternative of deferring acpi_dmar_init()
>> to a later point, or at least the part of it that needs to do PCI
>> config space accesses? I'm not currently convinced the device scope
>> parsing needs to happen that early: Neither iommu_supports_eim()
>> nor iommu_enable_x2apic_IR() look to depend on that information
>> at the first glance, and I think these are the routines that require
>> (part of) the DMAR parsing to happen early.
> I moved acpi_mmcfg_init() ahead of acpi_boot_init() because below code 
> sequence depending on pci_mmcfg_virt being correctly setup.
> acpi_dmar_init
>    ->acpi_parse_dmar
>      ->acpi_parse_one_drhd
>        ->acpi_parse_dev_scope
>          ->pci_conf_read8
>            ->pci_mmcfg_read
>              ->pci_dev_base
>                ->get_virt

Have you read my previous response in full? I'm specifically asking
whether the device scope parsing (i.e. acpi_parse_dev_scope())
really needs to happen as early as it does now. Without that, the
dependency on MMCFG accesses working would go away.

>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -1493,6 +1493,10 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>>   
>>>       generic_apic_probe();
>>>   
>>> +    pt_pci_init();
>>> +
>>> +    acpi_mmcfg_init();
>>> +
>>>       acpi_boot_init();
>> 
>> With the dependency being _in_ acpi_boot_init(), the invocation of
>> acpi_mmcfg_init() should now be moved there.
> Yes, I feel better to move pt_pci_init() and acpi_mmcfg_init() in 
> acpi_boot_init() before acpi_mmcfg_init().

I didn't say move both, did I?

That said, now having looked at what it actually does, I think you want
to rename it if the suggested alternative route can't be used, as in
particular the pt_ prefix is quite misleading then. Once that has
happened, moving the invocation perhaps even _into_ acpi_mcfg_init()
might be reasonable.

Jan



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

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

[Jan Beulich]

>>> On 16.08.18 at 11:30, <zhenzhong.duan@oracle.com> wrote:
> On 2018/8/16 17:13, Zhenzhong Duan wrote:
>>>> --- a/xen/arch/x86/setup.c
>>>> +++ b/xen/arch/x86/setup.c
>>>> @@ -1493,6 +1493,10 @@ void __init noreturn __start_xen(unsigned long 
>>>> mbi_p)
>>>>       generic_apic_probe();
>>>> +    pt_pci_init();
>>>> +
>>>> +    acpi_mmcfg_init();
>>>> +
>>>>       acpi_boot_init();
>>>
>>> With the dependency being _in_ acpi_boot_init(), the invocation of
>>> acpi_mmcfg_init() should now be moved there.
>> Yes, I feel better to move pt_pci_init() and acpi_mmcfg_init() in 
>> acpi_boot_init() before acpi_mmcfg_init(). Any more comments?
> I see acpi_boot_init() is empty func when CONFIG_ACPI_BOOT isn't set. Do 
> we support disabling this config option? If yes, I think above change 
> will break non-acpi case.

I'm sorry, but I'm lost: grep produces no single hit on my tree
when looking for ACPI_BOOT. Are you looking at some older tree?
Even when considering ACPI - that Kconfig option exists only for
ARM's purposes right now. If you were to make it user selectable,
I think you'd first have to fix a number of build issues in case it
got turned off.

Jan



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

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

[Zhenzhong Duan]

在 2018/8/16 18:42, Jan Beulich 写道:
>>>> On 16.08.18 at 11:30, <zhenzhong.duan@oracle.com> wrote:
>> On 2018/8/16 17:13, Zhenzhong Duan wrote:
>>>>> --- a/xen/arch/x86/setup.c
>>>>> +++ b/xen/arch/x86/setup.c
>>>>> @@ -1493,6 +1493,10 @@ void __init noreturn __start_xen(unsigned long
>>>>> mbi_p)
>>>>>        generic_apic_probe();
>>>>> +    pt_pci_init();
>>>>> +
>>>>> +    acpi_mmcfg_init();
>>>>> +
>>>>>        acpi_boot_init();
>>>>
>>>> With the dependency being _in_ acpi_boot_init(), the invocation of
>>>> acpi_mmcfg_init() should now be moved there.
>>> Yes, I feel better to move pt_pci_init() and acpi_mmcfg_init() in
>>> acpi_boot_init() before acpi_mmcfg_init(). Any more comments?
>> I see acpi_boot_init() is empty func when CONFIG_ACPI_BOOT isn't set. Do
>> we support disabling this config option? If yes, I think above change
>> will break non-acpi case.
> 
> I'm sorry, but I'm lost: grep produces no single hit on my tree
> when looking for ACPI_BOOT. Are you looking at some older tree?
> Even when considering ACPI - that Kconfig option exists only for
> ARM's purposes right now. If you were to make it user selectable,
> I think you'd first have to fix a number of build issues in case it
> got turned off.
Sorry, I wrongly looked into oracle internal branch.
In upstream, it's CONFIG_ACPI. It looks not an issue as you said 
CONFIG_ACPI is only for ARM.

Thanks
Zhenzhong

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

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

[Zhenzhong Duan]

在 2018/8/16 18:37, Jan Beulich 写道:
>>>> On 16.08.18 at 11:13, <zhenzhong.duan@oracle.com> wrote:
>> On 2018/8/16 15:10, Jan Beulich wrote:
>>> Have you investigated the alternative of deferring acpi_dmar_init()
>>> to a later point, or at least the part of it that needs to do PCI
>>> config space accesses? I'm not currently convinced the device scope
>>> parsing needs to happen that early: Neither iommu_supports_eim()
>>> nor iommu_enable_x2apic_IR() look to depend on that information
>>> at the first glance, and I think these are the routines that require
>>> (part of) the DMAR parsing to happen early.
>> I moved acpi_mmcfg_init() ahead of acpi_boot_init() because below code
>> sequence depending on pci_mmcfg_virt being correctly setup.
>> acpi_dmar_init
>>     ->acpi_parse_dmar
>>       ->acpi_parse_one_drhd
>>         ->acpi_parse_dev_scope
>>           ->pci_conf_read8
>>             ->pci_mmcfg_read
>>               ->pci_dev_base
>>                 ->get_virt
> 
> Have you read my previous response in full? I'm specifically asking
> whether the device scope parsing (i.e. acpi_parse_dev_scope())
> really needs to happen as early as it does now. Without that, the
> dependency on MMCFG accesses working would go away.
I'll move acpi_dmar_init() to later point to have a test as 
acpi_parse_one_drhd, acpi_parse_one_rmrr and acpi_parse_one_atsr all 
called acpi_parse_dev_scope.
> 
>>>> --- a/xen/arch/x86/setup.c
>>>> +++ b/xen/arch/x86/setup.c
>>>> @@ -1493,6 +1493,10 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>>>    
>>>>        generic_apic_probe();
>>>>    
>>>> +    pt_pci_init();
>>>> +
>>>> +    acpi_mmcfg_init();
>>>> +
>>>>        acpi_boot_init();
>>>
>>> With the dependency being _in_ acpi_boot_init(), the invocation of
>>> acpi_mmcfg_init() should now be moved there.
>> Yes, I feel better to move pt_pci_init() and acpi_mmcfg_init() in
>> acpi_boot_init() before acpi_mmcfg_init().
> 
> I didn't say move both, did I?
> 
> That said, now having looked at what it actually does, I think you want
> to rename it if the suggested alternative route can't be used, as in
> particular the pt_ prefix is quite misleading then. Once that has
> happened, moving the invocation perhaps even _into_ acpi_mcfg_init()
Understood.
Personly I like this way more as pt_pci_init() and acpi_mmcfg_init() 
only initialized some global variable and harmless to move ahead. Also 
acpi_mcfg_init from its name is suitable to move in acpi_boot_init()

Thanks
Zhenzhong

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