| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 09 Apr 2015 10:50:02 +0800
From: Jiang Liu <jiang.liu@...ux.intel.com>
To: "Rafael J. Wysocki" <rjw@...ysocki.net>
CC: Bjorn Helgaas <bhelgaas@...gle.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>,
"H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
Len Brown <lenb@...nel.org>, Lv Zheng <lv.zheng@...el.com>,
LKML <linux-kernel@...r.kernel.org>, linux-pci@...r.kernel.org,
linux-acpi@...r.kernel.org
Subject: Re: [Bugfix v3] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716
On 2015/4/9 7:44, Rafael J. Wysocki wrote:
> On Wednesday, April 08, 2015 01:48:46 PM Jiang Liu wrote:
>> On 2015/4/7 8:28, Rafael J. Wysocki wrote:
>>> On Friday, April 03, 2015 10:04:11 PM Bjorn Helgaas wrote:
>>>> Hi Jiang,
>> <snip>
>>>>> Currently acpi_dev_filter_resource_type() is only used by ACPI pci
>>>>> host bridge and IOAPIC driver, so it shouldn't affect other drivers.
>>>>
>>>> We should assume it will eventually be used for all ACPI devices,
>>>> shouldn't we?
>>>
>>> I'm not sure about that, really. In fact, I'd restrict its use to devices
>>> types that actually can "produce" resources (ie. do not require the resources
>>> to be provided by their ancestors or to be available from a global pool).
>>>
>>> Otherwise we're pretty much guaranteed to get into trouble.
>>>
>>> And all of the above rules need to be documented in the kernel source tree
>>> or people will get confused.
>> Hi Rafael,
>> How about following comments for acpi_dev_filter_resource_type()?
>> Thanks!
>> Gerry
>> --------------------------------------------------------------------
>> /**
>> * According to ACPI specifications, Consumer/Producer flag in ACPI resource
>> * descriptor means:
>> * 1(CONSUMER): This device consumes this resource
>> * 0(PRODUCER): This device produces and consumes this resource
>> * But for ACPI PCI host bridge, it is interpreted in another way:
>
> So first of all, this leads to a question: Why is it interpreted for ACPI PCI
> host bridges differently?
>
> Is it something we've figured out from experience, or is there a standard
> mandating that?
Hi Rafael,
I think we got this knowledge by experiences. PCI FW spec only
states _CRS reports resources assigned to the host bridge by firmware.
There's no statement about whether the resource is consumed by host
bridge itself or provided to it's child bus/devices. On x86/IA64 side,
the main resource consumed by PCI host bridge is IOPORT 0xCF8-0xCFF,
but not sure about ARM64 side. So how about:
PCI Firmware specification states that _CRS reports resources
assigned to the host bridge, but there's no way to tell whether
the resource is consumed by host bridge itself or provided to
its child bus/devices. So according to experiences, PCI host bridge
interprets Consumer/Producer flag as below to tell whether the resource
is consumed by host bridge itself.
>
>> * 1(CONSUMER): PCI host bridge itself consumes the resource, such as
>> * IOPORT 0xCF8-0xCFF to access PCI configuraiton space.
>> * 0(PRODUCER): PCI host bridge provides this resource to its child
>> * bus and devices.
>> *
>> * So this is a specially designed helper function to support ACPI PCI host
>> * bridge and ACPI IOAPIC, and its usage should be limited to those specific
>
> And this will make the reader wonder why the IOAPIC should be treated the same
> way as a PCI host bridge in that respect.
If a hot-pluggable IOAPIC is represented as an ACPI device, its _CRS
reports MMIO address assigned to the IOAPIC. And an IOAPIC device
won't produce MMIO resources by itself. So we could reuse
acpi_dev_filter_resource_type() here.
How about:
* So this is a specially designed helper function to support:
* 1) ACPI PCI host bridge, as explained above
* 2) ACPI IOAPIC, its _CRS reports only one MMIO resource and
* it won't produce MMIO resources by itself.
Thanks!
Gerry
>
>> * scenarioso only. It filters ACPI resource descriptors as below:
>> * 1) If flag IORESOURCE_WINDOW is not specified, it's querying resources
>> * consumed by device. That is to return:
>> * a) ACPI resources without producer_consumer flag
>> * b) ACPI resources with producer_consumer flag setting to CONSUMER.
>> * 2) If flag IORESOURCE_WINDOW is specified, it's querying resources
>> provided
>> * by device. That is to return:
>> * a) ACPI resources with producer_consumer flag setting to PRODUCER.
>> * 3) But there's an exception. Some platforms, such as PC Engines APU.1C,
>> * report PCI host bridge resource provision by Memory32Fixed().
>> * Please refer to https://bugzilla.kernel.org/show_bug.cgi?id=94221
>> * So a special flag IORESOURCE_MEM_8AND16BIT is used to work around this
>> * BIOS issue.
>> */
>>
>>>
>>>>> Another possible fix is to only ignore IO resource consumed by host
>>>>> bridge and keep IOMEM resource consumed by host bridge, please refer to:
>>>>> http://www.spinics.net/lists/linux-pci/msg39706.html
>>>>>
>>>>> Sample ACPI table are archived at:
>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=94221
>>>>>
>>>>> V2->V3:
>>>>> Refine function acpi_dev_match_producer_consumer() as suggested by Rafael
>>>>>
>>>>> Fixes: 63f1789ec716("Ignore resources consumed by host bridge itself")
>>>>> Reported-and-Tested-by: Bernhard Thaler <bernhard.thaler@...et.at>
>>>>> Signed-off-by: Jiang Liu <jiang.liu@...ux.intel.com>
>>>>> ---
>>>>> arch/x86/pci/acpi.c | 5 ++---
>>>>> drivers/acpi/resource.c | 33 +++++++++++++++++++++++++++++----
>>>>> 2 files changed, 31 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
>>>>> index e4695985f9de..8c4b1201f340 100644
>>>>> --- a/arch/x86/pci/acpi.c
>>>>> +++ b/arch/x86/pci/acpi.c
>>>>> @@ -337,7 +337,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
>>>>> info->bridge = device;
>>>>> ret = acpi_dev_get_resources(device, list,
>>>>> acpi_dev_filter_resource_type_cb,
>>>>> - (void *)(IORESOURCE_IO | IORESOURCE_MEM));
>>>>> + (void *)(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_WINDOW));
>>>>> if (ret < 0)
>>>>> dev_warn(&device->dev,
>>>>> "failed to parse _CRS method, error code %d\n", ret);
>>>>> @@ -346,8 +346,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
>>>>> "no IO and memory resources present in _CRS\n");
>>>>> else
>>>>> resource_list_for_each_entry_safe(entry, tmp, list) {
>>>>> - if ((entry->res->flags & IORESOURCE_WINDOW) == 0 ||
>>>>> - (entry->res->flags & IORESOURCE_DISABLED))
>>>>> + if (entry->res->flags & IORESOURCE_DISABLED)
>>>>> resource_list_destroy_entry(entry);
>>>>> else
>>>>> entry->res->name = info->name;
>>>>> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
>>>>> index 5589a6e2a023..e761a868bdba 100644
>>>>> --- a/drivers/acpi/resource.c
>>>>> +++ b/drivers/acpi/resource.c
>>>>> @@ -567,6 +567,12 @@ int acpi_dev_get_resources(struct acpi_device *adev, struct list_head *list,
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(acpi_dev_get_resources);
>>>>>
>>>>> +static bool acpi_dev_match_producer_consumer(unsigned long types, int producer)
>>>>> +{
>>>>> + return ((types & IORESOURCE_WINDOW) && producer == ACPI_PRODUCER) ||
>>>>> + ((types & IORESOURCE_WINDOW) == 0 && producer == ACPI_CONSUMER);
>>>>> +}
>>>>> +
>>>>> /**
>>>>> * acpi_dev_filter_resource_type - Filter ACPI resource according to resource
>>>>> * types
>>>>> @@ -585,27 +591,46 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
>>>>> case ACPI_RESOURCE_TYPE_MEMORY24:
>>>>> case ACPI_RESOURCE_TYPE_MEMORY32:
>>>>> case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
>>>>> + /*
>>>>> + * These types of resource descriptor should be used to
>>>>> + * describe resource consumption instead of resource provision.
>>>>> + * But some platforms, such as PC Engines APU.1C, reports
>>>>> + * resource provision by Memory32Fixed(). Please refer to:
>>>>> + * https://bugzilla.kernel.org/show_bug.cgi?id=94221
>>>>> + * So accept it no matter IORESOURCE_WINDOW is specified or not.
>>>>> + */
>>>>> type = IORESOURCE_MEM;
>>>>
>>>> I think this means these resources will be accepted regardless of whether
>>>> the caller is looking for Consumer or Producer resources. To preserve the
>>>> behavior I added with 66528fdd45b0, we might be forced to do that for PCI
>>>> host bridges (or maybe we could just add a quirk for the PC Engines BIOS).
>>>>
>>>> But I don't think it matches the ACPI spec intent, so I'm not sure it's
>>>> right to do it for all devices.
>>>
>>> No, it isn't, which is why acpi_dev_filter_resource_type() should not be used
>>> for all devices.
>>>
>>>>> break;
>>>>> case ACPI_RESOURCE_TYPE_IO:
>>>>> case ACPI_RESOURCE_TYPE_FIXED_IO:
>>>>> - type = IORESOURCE_IO;
>>>>> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
>>>>> + type = IORESOURCE_IO;
>>>>> break;
>>>>> case ACPI_RESOURCE_TYPE_IRQ:
>>>>> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
>>>>> + type = IORESOURCE_IRQ;
>>>>> + break;
>>>>> case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
>>>>> - type = IORESOURCE_IRQ;
>>>>> + if (acpi_dev_match_producer_consumer(types,
>>>>> + ares->data.extended_irq.producer_consumer))
>>>>> + type = IORESOURCE_IRQ;
>>>>> break;
>>>>> case ACPI_RESOURCE_TYPE_DMA:
>>>>> case ACPI_RESOURCE_TYPE_FIXED_DMA:
>>>>> - type = IORESOURCE_DMA;
>>>>> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
>>>>> + type = IORESOURCE_DMA;
>>>>> break;
>>>>> case ACPI_RESOURCE_TYPE_GENERIC_REGISTER:
>>>>> - type = IORESOURCE_REG;
>>>>> + if (acpi_dev_match_producer_consumer(types, ACPI_CONSUMER))
>>>>> + type = IORESOURCE_REG;
>>>>> break;
>>>>> case ACPI_RESOURCE_TYPE_ADDRESS16:
>>>>> case ACPI_RESOURCE_TYPE_ADDRESS32:
>>>>> case ACPI_RESOURCE_TYPE_ADDRESS64:
>>>>> case ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64:
>>>>> + if (!acpi_dev_match_producer_consumer(types,
>>>>> + ares->data.address.producer_consumer))
>>>>> + break;
>>>>> if (ares->data.address.resource_type == ACPI_MEMORY_RANGE)
>>>>> type = IORESOURCE_MEM;
>>>>> else if (ares->data.address.resource_type == ACPI_IO_RANGE)
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>>>> the body of a message to majordomo@...r.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists