| 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: Fri, 6 Nov 2015 11:18:56 +0100
From: Tomasz Nowicki <tn@...ihalf.com>
To: Lorenzo Pieralisi <lorenzo.pieralisi@....com>
Cc: Jiang Liu <jiang.liu@...ux.intel.com>,
Bjorn Helgaas <bhelgaas@...gle.com>,
"Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
Marc Zyngier <marc.zyngier@....com>,
Hanjun Guo <hanjun.guo@...aro.org>,
Liviu Dudau <Liviu.Dudau@....com>, linux-acpi@...r.kernel.org,
linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
x86@...nel.org
Subject: Re: [Patch v7 4/7] PCI/ACPI: Add interface acpi_pci_root_create()
On 05.11.2015 19:19, Lorenzo Pieralisi wrote:
> On Thu, Nov 05, 2015 at 03:21:34PM +0100, Tomasz Nowicki wrote:
>> On 14.10.2015 08:29, Jiang Liu wrote:
>
> [...]
>
>>> +static void acpi_pci_root_validate_resources(struct device *dev,
>>> + struct list_head *resources,
>>> + unsigned long type)
>>> +{
>>> + LIST_HEAD(list);
>>> + struct resource *res1, *res2, *root = NULL;
>>> + struct resource_entry *tmp, *entry, *entry2;
>>> +
>>> + BUG_ON((type & (IORESOURCE_MEM | IORESOURCE_IO)) == 0);
>>> + root = (type & IORESOURCE_MEM) ? &iomem_resource : &ioport_resource;
>>> +
>>> + list_splice_init(resources, &list);
>>> + resource_list_for_each_entry_safe(entry, tmp, &list) {
>>> + bool free = false;
>>> + resource_size_t end;
>>> +
>>> + res1 = entry->res;
>>> + if (!(res1->flags & type))
>>> + goto next;
>>> +
>>> + /* Exclude non-addressable range or non-addressable portion */
>>> + end = min(res1->end, root->end);
>>> + if (end <= res1->start) {
>>> + dev_info(dev, "host bridge window %pR (ignored, not CPU addressable)\n",
>>> + res1);
>>> + free = true;
>>> + goto next;
>>> + } else if (res1->end != end) {
>>> + dev_info(dev, "host bridge window %pR ([%#llx-%#llx] ignored, not CPU addressable)\n",
>>> + res1, (unsigned long long)end + 1,
>>> + (unsigned long long)res1->end);
>>> + res1->end = end;
>>> + }
>>> +
>>> + resource_list_for_each_entry(entry2, resources) {
>>> + res2 = entry2->res;
>>> + if (!(res2->flags & type))
>>> + continue;
>>> +
>>> + /*
>>> + * I don't like throwing away windows because then
>>> + * our resources no longer match the ACPI _CRS, but
>>> + * the kernel resource tree doesn't allow overlaps.
>>> + */
>>> + if (resource_overlaps(res1, res2)) {
>>> + res2->start = min(res1->start, res2->start);
>>> + res2->end = max(res1->end, res2->end);
>>> + dev_info(dev, "host bridge window expanded to %pR; %pR ignored\n",
>>> + res2, res1);
>>> + free = true;
>>> + goto next;
>>> + }
>>> + }
>>> +
>>> +next:
>>> + resource_list_del(entry);
>>> + if (free)
>>> + resource_list_free_entry(entry);
>>> + else
>>> + resource_list_add_tail(entry, resources);
>>> + }
>>> +}
>>> +
>>> +int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
>>> +{
>>> + int ret;
>>> + struct list_head *list = &info->resources;
>>> + struct acpi_device *device = info->bridge;
>>> + struct resource_entry *entry, *tmp;
>>> + unsigned long flags;
>>> +
>>> + flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_MEM_8AND16BIT;
>>> + ret = acpi_dev_get_resources(device, list,
>>> + acpi_dev_filter_resource_type_cb,
>>> + (void *)flags);
>>> + if (ret < 0)
>>> + dev_warn(&device->dev,
>>> + "failed to parse _CRS method, error code %d\n", ret);
>>> + else if (ret == 0)
>>> + dev_dbg(&device->dev,
>>> + "no IO and memory resources present in _CRS\n");
>>> + else {
>>> + resource_list_for_each_entry_safe(entry, tmp, list) {
>>> + if (entry->res->flags & IORESOURCE_DISABLED)
>>> + resource_list_destroy_entry(entry);
>>> + else
>>> + entry->res->name = info->name;
>>> + }
>>> + acpi_pci_root_validate_resources(&device->dev, list,
>>> + IORESOURCE_MEM);
>>> + acpi_pci_root_validate_resources(&device->dev, list,
>>> + IORESOURCE_IO);
>>
>> It is not clear to me why we need these two calls above ^^^. We are
>> using pci_acpi_root_add_resources(info) later. Is it not enough?
>>
>> Also, I cannot use acpi_pci_probe_root_resources() in my ARM64 PCI
>> driver. It is because acpi_dev_get_resources is adding
>> translation_offset to IO ranges start address and then:
>> acpi_pci_root_validate_resources(&device->dev, list,
>> IORESOURCE_IO);
>> rejects that IO regions as it is out of my 0x0-SZ_16M window.
>>
>> Does acpi_pci_probe_root_resources meant to be x86 specific and I
>> should avoid using it?
>
> IIUC, you _have_ to have the proper translation_offset to map the bridge
> window into the IO address space:
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/348708.html
>
> Then, using the offset, you should do something ia64 does, namely,
> retrieve the CPU address corresponding to IO space (see arch/ia64/pci/pci.c
> - add_io_space()) and map it in the physical address space by using
> pci_remap_iospace(), it is similar to what we have to do with DT.
>
> It is extremely confusing and I am not sure I got it right myself,
> I am still grokking ia64 code to understand what it really does.
>
> So basically, the IO bridge window coming from acpi_dev_get_resource()
> should represent the IO space in 0 - 16M, IIUC.
Yes, you are right IMO.
>
> By using the offset (that was initialized using translation_offset) and
> the resource->start, you can retrieve the cpu address that you need to
> actually map the IO space, since that's what we do on ARM (ie the
> IO resource is an offset into the virtual address space set aside
> for IO).
>
Right, that is clear to me, below my current implementation example
which works for ARM64 now:
static struct acpi_pci_root_ops acpi_pci_root_ops = {
[...]
.prepare_resources = pci_acpi_root_prepare_resources,
};
static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
{
struct list_head *list = &ci->resources;
struct acpi_device *device = ci->bridge;
struct resource_entry *entry, *tmp;
unsigned long flags;
int ret;
flags = IORESOURCE_IO | IORESOURCE_MEM;
ret = acpi_dev_get_resources(device, list,
acpi_dev_filter_resource_type_cb,
(void *)flags);
if (ret < 0) {
dev_warn(&device->dev,
"failed to parse _CRS method, error code %d\n", ret);
return ret;
} else if (ret == 0)
dev_dbg(&device->dev,
"no IO and memory resources present in _CRS\n");
resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
struct resource *res = entry->res;
if (entry->res->flags & IORESOURCE_DISABLED)
resource_list_destroy_entry(entry);
else
res->name = ci->name;
/*
* TODO: need to move pci_register_io_range() function out
* of drivers/of/address.c for both used by DT and ACPI
*/
if (res->flags & IORESOURCE_IO) {
unsigned long port;
int err;
resource_size_t length = res->end - res->start;
resource_size_t start = res->start;
err = pci_register_io_range(res->start, length);
if (err) {
resource_list_destroy_entry(entry);
continue;
}
port = pci_address_to_pio(res->start);
if (port == (unsigned long)-1) {
resource_list_destroy_entry(entry);
continue;
}
res->start = port;
res->end = res->start + length;
entry->offset = port - (start - entry->offset);
if (pci_remap_iospace(res, start) < 0)
resource_list_destroy_entry(entry);
}
}
return ret;
}
As you see acpi_dev_get_resources returns:
res->start = acpi_des->start + acpi_des->translation_offset (CPU address)
which then must be adjusted as you described to get io port and call
pci_remap_iospace.
This is also why I can not use acpi_pci_probe_root_resources here. Lets
assume we have IO range like that DSDT table form QEMU:
DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,
0x00000000, // Granularity
0x00000000, // Range Minimum
0x0000FFFF, // Range Maximum
0x3EFF0000, // Translation Offset
0x00010000, // Length
,, , TypeStatic)
so see acpi_dev_get_resources returns res->start = acpi_des->start (0x0)
+ acpi_des->translation_offset(0x3EFF0000) = 0x3EFF0000. This will be
rejected in acpi_pci_root_validate_resources() as 0x3EFF0000 does not
fit within 0-16M.
My question is if acpi_pci_probe_root_resources is handling
translation_offset properly and if we have some silent assumption
specific for e.g. ia64 here.
Thanks for help in looking at it.
Tomasz
--
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