Patchwork acpi: mcfg: Set MCFGMMIONotReserved error to low

login
register
mail settings
Submitter Alex Hung
Date Oct. 25, 2012, 5:42 a.m.
Message ID <1351143725-2781-1-git-send-email-alex.hung@canonical.com>
Download mbox | patch
Permalink /patch/194041/
State Accepted
Headers show

Comments

Alex Hung - Oct. 25, 2012, 5:42 a.m.
In PCI Firmware Specification, it claims "If the operating system does not natively
comprehend reserving the MMCFG region, the MMCFG region must be reserved by firmware."
However, Linux is an operating system that comprehend MMCFG, and therefore it is not
a problem for not declaring it.

Signed-off-by: Alex Hung <alex.hung@canonical.com>
---
 src/acpi/mcfg/mcfg.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Keng-Yu Lin - Oct. 25, 2012, 7:02 a.m.
On Thu, Oct 25, 2012 at 1:42 PM, Alex Hung <alex.hung@canonical.com> wrote:
> In PCI Firmware Specification, it claims "If the operating system does not natively
> comprehend reserving the MMCFG region, the MMCFG region must be reserved by firmware."
> However, Linux is an operating system that comprehend MMCFG, and therefore it is not
> a problem for not declaring it.
>
> Signed-off-by: Alex Hung <alex.hung@canonical.com>
> ---
>  src/acpi/mcfg/mcfg.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/acpi/mcfg/mcfg.c b/src/acpi/mcfg/mcfg.c
> index 9dfd9fd..878c0f1 100644
> --- a/src/acpi/mcfg/mcfg.c
> +++ b/src/acpi/mcfg/mcfg.c
> @@ -200,7 +200,7 @@ static int mcfg_test1(fwts_framework *fw)
>                 if ((memory_map_list != NULL) &&
>                     (!fwts_memory_map_is_reserved(memory_map_list, config->base_address))) {
>
> -                       fwts_failed(fw, LOG_LEVEL_MEDIUM, "MCFGMMIONotReserved",
> +                       fwts_failed(fw, LOG_LEVEL_LOW, "MCFGMMIONotReserved",
>                                 "MCFG mmio config space at 0x%" PRIx64
>                                 " is not reserved in the memory map table",
>                                 config->base_address);
> --
> 1.7.9.5
>
Acked-by: Keng-Yu Lin <kengyu@canonical.com>
Colin King - Oct. 25, 2012, 8:48 a.m.
On 25/10/12 06:42, Alex Hung wrote:
> In PCI Firmware Specification, it claims "If the operating system does not natively
> comprehend reserving the MMCFG region, the MMCFG region must be reserved by firmware."
> However, Linux is an operating system that comprehend MMCFG, and therefore it is not
> a problem for not declaring it.
>
> Signed-off-by: Alex Hung <alex.hung@canonical.com>
> ---
>   src/acpi/mcfg/mcfg.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/acpi/mcfg/mcfg.c b/src/acpi/mcfg/mcfg.c
> index 9dfd9fd..878c0f1 100644
> --- a/src/acpi/mcfg/mcfg.c
> +++ b/src/acpi/mcfg/mcfg.c
> @@ -200,7 +200,7 @@ static int mcfg_test1(fwts_framework *fw)
>   		if ((memory_map_list != NULL) &&
>   		    (!fwts_memory_map_is_reserved(memory_map_list, config->base_address))) {
>
> -			fwts_failed(fw, LOG_LEVEL_MEDIUM, "MCFGMMIONotReserved",
> +			fwts_failed(fw, LOG_LEVEL_LOW, "MCFGMMIONotReserved",
>   				"MCFG mmio config space at 0x%" PRIx64
>   				" is not reserved in the memory map table",
>   				config->base_address);
>

I'm not 100% sure how significant this is.  Can you take a look at what 
the kernel does if it isn't reserved by firmware?  I think the relevant 
code is in arch/x86/pci/*

Personally I don't mind dropping this to LOW, but I'd like to know what 
the kernel does nowadays for this issue.

Colin
Alex Hung - Oct. 25, 2012, 10:17 a.m.
On 10/25/2012 04:48 PM, Colin Ian King wrote:
> On 25/10/12 06:42, Alex Hung wrote:
>> In PCI Firmware Specification, it claims "If the operating system does
>> not natively
>> comprehend reserving the MMCFG region, the MMCFG region must be
>> reserved by firmware."
>> However, Linux is an operating system that comprehend MMCFG, and
>> therefore it is not
>> a problem for not declaring it.
>>
>> Signed-off-by: Alex Hung <alex.hung@canonical.com>
>> ---
>>   src/acpi/mcfg/mcfg.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/acpi/mcfg/mcfg.c b/src/acpi/mcfg/mcfg.c
>> index 9dfd9fd..878c0f1 100644
>> --- a/src/acpi/mcfg/mcfg.c
>> +++ b/src/acpi/mcfg/mcfg.c
>> @@ -200,7 +200,7 @@ static int mcfg_test1(fwts_framework *fw)
>>           if ((memory_map_list != NULL) &&
>>               (!fwts_memory_map_is_reserved(memory_map_list,
>> config->base_address))) {
>>
>> -            fwts_failed(fw, LOG_LEVEL_MEDIUM, "MCFGMMIONotReserved",
>> +            fwts_failed(fw, LOG_LEVEL_LOW, "MCFGMMIONotReserved",
>>                   "MCFG mmio config space at 0x%" PRIx64
>>                   " is not reserved in the memory map table",
>>                   config->base_address);
>>
>
> I'm not 100% sure how significant this is.  Can you take a look at what
> the kernel does if it isn't reserved by firmware?  I think the relevant
> code is in arch/x86/pci/*
>
> Personally I don't mind dropping this to LOW, but I'd like to know what
> the kernel does nowadays for this issue.
>
> Colin
>
>

Hi Colin,

Thanks for reminding.

I checked mmconfig-shared.c and there is a function 
"pci_mmconfig_insert" to insert (reserve) the memory address in MCFG. It 
seems safe to lower the error level.

Cheers,
Alex Hung
Colin King - Oct. 25, 2012, 10:22 a.m.
On 25/10/12 11:17, Alex Hung wrote:
> On 10/25/2012 04:48 PM, Colin Ian King wrote:
>> On 25/10/12 06:42, Alex Hung wrote:
>>> In PCI Firmware Specification, it claims "If the operating system does
>>> not natively
>>> comprehend reserving the MMCFG region, the MMCFG region must be
>>> reserved by firmware."
>>> However, Linux is an operating system that comprehend MMCFG, and
>>> therefore it is not
>>> a problem for not declaring it.
>>>
>>> Signed-off-by: Alex Hung <alex.hung@canonical.com>
>>> ---
>>>   src/acpi/mcfg/mcfg.c |    2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/src/acpi/mcfg/mcfg.c b/src/acpi/mcfg/mcfg.c
>>> index 9dfd9fd..878c0f1 100644
>>> --- a/src/acpi/mcfg/mcfg.c
>>> +++ b/src/acpi/mcfg/mcfg.c
>>> @@ -200,7 +200,7 @@ static int mcfg_test1(fwts_framework *fw)
>>>           if ((memory_map_list != NULL) &&
>>>               (!fwts_memory_map_is_reserved(memory_map_list,
>>> config->base_address))) {
>>>
>>> -            fwts_failed(fw, LOG_LEVEL_MEDIUM, "MCFGMMIONotReserved",
>>> +            fwts_failed(fw, LOG_LEVEL_LOW, "MCFGMMIONotReserved",
>>>                   "MCFG mmio config space at 0x%" PRIx64
>>>                   " is not reserved in the memory map table",
>>>                   config->base_address);
>>>
>>
>> I'm not 100% sure how significant this is.  Can you take a look at what
>> the kernel does if it isn't reserved by firmware?  I think the relevant
>> code is in arch/x86/pci/*
>>
>> Personally I don't mind dropping this to LOW, but I'd like to know what
>> the kernel does nowadays for this issue.
>>
>> Colin
>>
>>
>
> Hi Colin,
>
> Thanks for reminding.
>
> I checked mmconfig-shared.c and there is a function
> "pci_mmconfig_insert" to insert (reserve) the memory address in MCFG. It
> seems safe to lower the error level.
>
> Cheers,
> Alex Hung
>
Thanks Alex, I suspect I picked up this paranoid check from the original 
code derived from the linuxfirmwarekit. I'm happy to reduce this to a 
LOW failure.

Acked-by: Colin Ian King <colin.king@canonical.com>

Patch

diff --git a/src/acpi/mcfg/mcfg.c b/src/acpi/mcfg/mcfg.c
index 9dfd9fd..878c0f1 100644
--- a/src/acpi/mcfg/mcfg.c
+++ b/src/acpi/mcfg/mcfg.c
@@ -200,7 +200,7 @@  static int mcfg_test1(fwts_framework *fw)
 		if ((memory_map_list != NULL) &&
 		    (!fwts_memory_map_is_reserved(memory_map_list, config->base_address))) {
 
-			fwts_failed(fw, LOG_LEVEL_MEDIUM, "MCFGMMIONotReserved",
+			fwts_failed(fw, LOG_LEVEL_LOW, "MCFGMMIONotReserved",
 				"MCFG mmio config space at 0x%" PRIx64
 				" is not reserved in the memory map table",
 				config->base_address);