| 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: Tue, 27 Aug 2013 22:56:31 +0800
From: Lan Tianyu <lantianyu1986@...il.com>
To: Yinghai Lu <yinghai@...nel.org>
Cc: "Rafael J. Wysocki" <rjw@...k.pl>,
Bjorn Helgaas <bhelgaas@...gle.com>,
rui wang <ruiv.wang@...il.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...e.hu>, "H. Peter Anvin" <hpa@...or.com>,
Tony Luck <tony.luck@...el.com>,
Linux PCI <linux-pci@...r.kernel.org>,
"linux-kernel@...r kernel org" <linux-kernel@...r.kernel.org>,
"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>
Subject: Re: [PATCH v4 28/28] PCI, x86, ACPI: get ioapic address from acpi device
2013/8/27 Yinghai Lu <yinghai@...nel.org>:
> On Mon, Aug 26, 2013 at 8:46 AM, Lan Tianyu <lantianyu1986@...il.com> wrote:
>> 2013/8/24 Yinghai Lu <yinghai@...nel.org>:
>>> On Fri, Aug 23, 2013 at 12:04 PM, Yinghai Lu <yinghai@...nel.org> wrote:
>>>> On Fri, Aug 23, 2013 at 11:38 AM, Yinghai Lu <yinghai@...nel.org> wrote:
>>>>> On Fri, Aug 23, 2013 at 8:34 AM, Lan Tianyu <lantianyu1986@...il.com> wrote:
>>>>>>> I worked around the problem by replacing acpi_resource_to_address64()
>>>>>>> with resource_to_addr(). But resource_to_addr() is a static function
>>>>>>> in arch/x86/pci/acpi.c, not very convenient to use. Here's what I did:
>>>>>>>
>>>>>>
>>>>>> Hi Rui&Yinghai:
>>>>>> How about using the following code to translate struct
>>>>>> acpi_resource to struct resouce in this setup_res()?
>>>>>>
>>>>>> if (acpi_dev_resource_address_space(...)
>>>>>> || acpi_dev_resource_memory(..))
>>>>>> return AE_OK;
>>>>>
>>>>> Yest, that could be better, will update that.
>>>>>
>>>>> Also can you submit patch that will use that in res_to_addr of
>>>>> arch/x86/pci/acpi.c?
>>>>
>>>> looks acpi_dev_resource_address_space... does not handle
>>>> PREFTCH and translation offset.
>>>>
>>>> So now i have to use res_to_addr alike one.
>>>
>>> Raphael,
>>>
>>> Maybe we should move resource_to_addr to acpi generic.
>>>
>>> Please check if you are ok with attached.
>>>
>>> Thanks
>>>
>>> Yinghai
>>
>> Hi Rafael & Yinghai:
>> I wrote 4 proposal patches to try to make acpi resource
>> functions to replace
>> resource_to_addr() in the arch/x86/pci/acpi.c. From my opinion,
>> resource_to_addr()
>> does most work which acpi resource functions have done. So please have
>> a look. Thanks.
>> The following patches just passed through compiling test.
>
> Thanks for doing that.
>
> Overall it's Good to me.
>
> some comments inline.
>
>>
>> Patch 1
>> -------------------
>> commit 1800bc9dda7318314607fbae7afda8be38e056dc
>> Author: Lan Tianyu <tianyu.lan@...el.com>
>> Date: Mon Aug 26 14:40:39 2013 +0800
>>
>> ACPI/Resource: Add setting PREFETCH flag support
>>
>> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
>> index b7201fc..23a560b 100644
>> --- a/drivers/acpi/resource.c
>> +++ b/drivers/acpi/resource.c
>> @@ -35,7 +35,7 @@
>> #endif
>>
>> static unsigned long acpi_dev_memresource_flags(u64 len, u8 write_protect,
>> - bool window)
>> + bool window, bool prefetchable)
>> {
>> unsigned long flags = IORESOURCE_MEM;
>>
>> @@ -48,6 +48,9 @@ static unsigned long acpi_dev_memresource_flags(u64
>> len, u8 write_protect,
>> if (window)
>> flags |= IORESOURCE_WINDOW;
>>
>> + if (prefetchable)
>> + flags |= IORESOURCE_PREFETCH;
>> +
>> return flags;
>> }
>>
>> @@ -56,7 +59,8 @@ static void acpi_dev_get_memresource(struct resource
>> *res, u64 start, u64 len,
>> {
>> res->start = start;
>> res->end = start + len - 1;
>> - res->flags = acpi_dev_memresource_flags(len, write_protect, false);
>> + res->flags = acpi_dev_memresource_flags(len, write_protect,
>> + false, false);
>> }
>
> passing write_protect, window then pref looks silly.
>
> may use | IO_REOURCE_... etc outside of acpi_dev_memresource_flags directly.
>
Yes, this looks better.
>>
>> /**
>> @@ -175,7 +179,7 @@ bool acpi_dev_resource_address_space(struct
>> acpi_resource *ares,
>> {
>> acpi_status status;
>> struct acpi_resource_address64 addr;
>> - bool window;
>> + bool window, prefetchable;
>> u64 len;
>> u8 io_decode;
>>
>> @@ -199,9 +203,11 @@ bool acpi_dev_resource_address_space(struct
>> acpi_resource *ares,
>> switch(addr.resource_type) {
>> case ACPI_MEMORY_RANGE:
>> len = addr.maximum - addr.minimum + 1;
>> + prefetchable =
>> + addr.info.mem.caching == ACPI_PREFETCHABLE_MEMORY;
>> res->flags = acpi_dev_memresource_flags(len,
>> addr.info.mem.write_protect,
>> - window);
>> + window, prefetchable);
>> break;
>> case ACPI_IO_RANGE:
>> io_decode = addr.granularity == 0xfff ?
>> @@ -252,7 +258,7 @@ bool acpi_dev_resource_ext_address_space(struct
>> acpi_resource *ares,
>> len = ext_addr->maximum - ext_addr->minimum + 1;
>> res->flags = acpi_dev_memresource_flags(len,
>> ext_addr->info.mem.write_protect,
>> - window);
>> + window, false);
>> break;
>> case ACPI_IO_RANGE:
>> io_decode = ext_addr->granularity == 0xfff ?
>>
>> Patch 2
>> -----------
>> commit a3ff8345ffb51885b216ba0ae231252cd88d4e76
>> Author: Lan Tianyu <tianyu.lan@...el.com>
>> Date: Mon Aug 26 15:57:14 2013 +0800
>>
>> ACPI/Resource: Add address translation support
>>
>> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
>> index 23a560b..439ee44 100644
>> --- a/drivers/acpi/resource.c
>> +++ b/drivers/acpi/resource.c
>> @@ -196,8 +196,8 @@ bool acpi_dev_resource_address_space(struct
>> acpi_resource *ares,
>> if (ACPI_FAILURE(status))
>> return true;
>>
>> - res->start = addr.minimum;
>> - res->end = addr.maximum;
>> + res->start = addr.minimum + addr.translation_offset;
>> + res->end = addr.maximum + addr.translation_offset;
>> window = addr.producer_consumer == ACPI_PRODUCER;
>>
>> switch(addr.resource_type) {
>>
>>
>> Patch 3
>> ---------------
>> commit 68bfefe619b49baa5d372e0664a6cb6d5138fad1
>> Author: Lan Tianyu <tianyu.lan@...el.com>
>> Date: Mon Aug 26 21:45:32 2013 +0800
>>
>> ACPI: Add new acpi_dev_resource_address_space_with_addr() function
>>
>> Add new function which can return the converted struct
>> acpi_resource_address64.
>> This will be used to get transaction offset in the setup_resource().
>>
>> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
>> index 439ee44..98a6c35 100644
>> --- a/drivers/acpi/resource.c
>> +++ b/drivers/acpi/resource.c
>> @@ -174,11 +174,11 @@ EXPORT_SYMBOL_GPL(acpi_dev_resource_io);
>> * and if that's the case, use the information in it to populate the generic
>> * resource object pointed to by @res.
>> */
>> -bool acpi_dev_resource_address_space(struct acpi_resource *ares,
>> +bool acpi_dev_resource_address_space_with_addr(struct acpi_resource *ares,
>> + struct acpi_resource_address64 *addr,
>> struct resource *res)
>> {
>> acpi_status status;
>> - struct acpi_resource_address64 addr;
>> bool window, prefetchable;
>> u64 len;
>> u8 io_decode;
>> @@ -192,28 +192,28 @@ bool acpi_dev_resource_address_space(struct
>> acpi_resource *ares,
>> return false;
>> }
>>
>> - status = acpi_resource_to_address64(ares, &addr);
>> + status = acpi_resource_to_address64(ares, addr);
>> if (ACPI_FAILURE(status))
>> return true;
>>
>> - res->start = addr.minimum + addr.translation_offset;
>> - res->end = addr.maximum + addr.translation_offset;
>> - window = addr.producer_consumer == ACPI_PRODUCER;
>> + res->start = addr->minimum + addr->translation_offset;
>> + res->end = addr->maximum + addr->translation_offset;
>> + window = addr->producer_consumer == ACPI_PRODUCER;
>>
>> - switch(addr.resource_type) {
>> + switch (addr->resource_type) {
>> case ACPI_MEMORY_RANGE:
>> - len = addr.maximum - addr.minimum + 1;
>> + len = addr->maximum - addr->minimum + 1;
>> prefetchable =
>> - addr.info.mem.caching == ACPI_PREFETCHABLE_MEMORY;
>> + addr->info.mem.caching == ACPI_PREFETCHABLE_MEMORY;
>> res->flags = acpi_dev_memresource_flags(len,
>> - addr.info.mem.write_protect,
>> + addr->info.mem.write_protect,
>> window, prefetchable);
>> break;
>> case ACPI_IO_RANGE:
>> - io_decode = addr.granularity == 0xfff ?
>> + io_decode = addr->granularity == 0xfff ?
>> ACPI_DECODE_10 : ACPI_DECODE_16;
>> - res->flags = acpi_dev_ioresource_flags(addr.minimum,
>> - addr.maximum,
>> + res->flags = acpi_dev_ioresource_flags(addr->minimum,
>> + addr->maximum,
>> io_decode, window);
>> break;
>> case ACPI_BUS_NUMBER_RANGE:
>> @@ -225,6 +225,16 @@ bool acpi_dev_resource_address_space(struct
>> acpi_resource *ares,
>>
>> return true;
>> }
>> +EXPORT_SYMBOL_GPL(acpi_dev_resource_address_space_with_addr);
>> +
>> +bool acpi_dev_resource_address_space(struct acpi_resource *ares,
>> + struct resource *res)
>> +{
>> + struct acpi_resource_address64 addr;
>> +
>> + return acpi_dev_resource_address_space_with_addr(ares, &addr,
>> + res);
>> +}
>> EXPORT_SYMBOL_GPL(acpi_dev_resource_address_space);
>>
>> /**
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index a5db4ae..9f5c0d5 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -260,6 +260,9 @@ bool acpi_dev_resource_memory(struct acpi_resource
>> *ares, struct resource *res);
>> bool acpi_dev_resource_io(struct acpi_resource *ares, struct resource *res);
>> bool acpi_dev_resource_address_space(struct acpi_resource *ares,
>> struct resource *res);
>> +bool acpi_dev_resource_address_space_with_addr(struct acpi_resource *ares,
>> + struct acpi_resource_address64 *addr,
>> + struct resource *res);
>> bool acpi_dev_resource_ext_address_space(struct acpi_resource *ares,
>> struct resource *res);
>> unsigned long acpi_dev_irq_flags(u8 triggering, u8 polarity, u8 shareable);
>>
>> Patch 4
>> --------------
>> commit a8733a1da756a257d55e68994268174b84b33670
>> Author: Lan Tianyu <tianyu.lan@...el.com>
>> Date: Mon Aug 26 22:53:38 2013 +0800
>>
>> X86/PCI/ACPI: Rework setup_resource() via functions acpi resource functions
>>
>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
>> index d641897..d4f85a1 100644
>> --- a/arch/x86/pci/acpi.c
>> +++ b/arch/x86/pci/acpi.c
>> @@ -219,62 +219,15 @@ static void teardown_mcfg_map(struct pci_root_info *info)
>> #endif
>>
>> static acpi_status
>> -resource_to_addr(struct acpi_resource *resource,
>> - struct acpi_resource_address64 *addr)
>> -{
>> - acpi_status status;
>> - struct acpi_resource_memory24 *memory24;
>> - struct acpi_resource_memory32 *memory32;
>> - struct acpi_resource_fixed_memory32 *fixed_memory32;
>> -
>> - memset(addr, 0, sizeof(*addr));
>> - switch (resource->type) {
>> - case ACPI_RESOURCE_TYPE_MEMORY24:
>> - memory24 = &resource->data.memory24;
>> - addr->resource_type = ACPI_MEMORY_RANGE;
>> - addr->minimum = memory24->minimum;
>> - addr->address_length = memory24->address_length;
>> - addr->maximum = addr->minimum + addr->address_length - 1;
>> - return AE_OK;
>> - case ACPI_RESOURCE_TYPE_MEMORY32:
>> - memory32 = &resource->data.memory32;
>> - addr->resource_type = ACPI_MEMORY_RANGE;
>> - addr->minimum = memory32->minimum;
>> - addr->address_length = memory32->address_length;
>> - addr->maximum = addr->minimum + addr->address_length - 1;
>> - return AE_OK;
>> - case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
>> - fixed_memory32 = &resource->data.fixed_memory32;
>> - addr->resource_type = ACPI_MEMORY_RANGE;
>> - addr->minimum = fixed_memory32->address;
>> - addr->address_length = fixed_memory32->address_length;
>> - addr->maximum = addr->minimum + addr->address_length - 1;
>> - return AE_OK;
>> - case ACPI_RESOURCE_TYPE_ADDRESS16:
>> - case ACPI_RESOURCE_TYPE_ADDRESS32:
>> - case ACPI_RESOURCE_TYPE_ADDRESS64:
>> - status = acpi_resource_to_address64(resource, addr);
>> - if (ACPI_SUCCESS(status) &&
>> - (addr->resource_type == ACPI_MEMORY_RANGE ||
>> - addr->resource_type == ACPI_IO_RANGE) &&
>> - addr->address_length > 0) {
>> - return AE_OK;
>> - }
>> - break;
>> - }
>> - return AE_ERROR;
>> -}
>> -
>> -static acpi_status
>> count_resource(struct acpi_resource *acpi_res, void *data)
>> {
>> struct pci_root_info *info = data;
>> - struct acpi_resource_address64 addr;
>> - acpi_status status;
>> + struct resource res;
>>
>> - status = resource_to_addr(acpi_res, &addr);
>> - if (ACPI_SUCCESS(status))
>> + if (acpi_dev_resource_address_space(acpi_res, &res)
>> + || acpi_dev_resource_memory(acpi_res, &res))
>> info->res_num++;
>> +
>> return AE_OK;
>> }
>>
>> @@ -282,27 +235,18 @@ static acpi_status
>> setup_resource(struct acpi_resource *acpi_res, void *data)
>> {
>> struct pci_root_info *info = data;
>> - struct resource *res;
>> + struct resource *res = &info->res[info->res_num];
>> struct acpi_resource_address64 addr;
>> - acpi_status status;
>> - unsigned long flags;
>> u64 start, orig_end, end;
>>
>> - status = resource_to_addr(acpi_res, &addr);
>> - if (!ACPI_SUCCESS(status))
>> - return AE_OK;
>> + memset(&addr, 0x00, sizeof(addr));
>>
>> - if (addr.resource_type == ACPI_MEMORY_RANGE) {
>> - flags = IORESOURCE_MEM;
>> - if (addr.info.mem.caching == ACPI_PREFETCHABLE_MEMORY)
>> - flags |= IORESOURCE_PREFETCH;
>> - } else if (addr.resource_type == ACPI_IO_RANGE) {
>> - flags = IORESOURCE_IO;
>> - } else
>> + if (!(acpi_dev_resource_address_space_with_addr(acpi_res, &addr, res)
>> + || acpi_dev_resource_memory(acpi_res, res)))
>
> acpi_dev_resource_address_space_with_addr(acpi_res, &addr, res)
>
> passing extra addr, just for translation_offset?
>
For this case, it's "Yes", . For some other cases(e.g
arch/ia64/pci/pci.c add_window()),
whole addr maybe necessary. To be more general, I hope it can return more infos
> maybe pass res_offset directly?
>
>
>
>
>> return AE_OK;
>>
>> - start = addr.minimum + addr.translation_offset;
>> - orig_end = end = addr.maximum + addr.translation_offset;
>> + start = res->start;
>> + orig_end = end = res->end;
>
> Yinghai
--
Best regards
Tianyu Lan
--
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