| 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
| ||
|
Message-ID: <56A8146B.4070601@codeaurora.org>
Date: Tue, 26 Jan 2016 18:50:51 -0600
From: Shanker Donthineni <shankerd@...eaurora.org>
To: Marc Zyngier <marc.zyngier@....com>, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org
Cc: Thomas Gleixner <tglx@...utronix.de>,
Jason Cooper <jason@...edaemon.net>,
Vikram Sethi <vikrams@...eaurora.org>,
Timur Tabi <timur@...eaurora.org>,
Christopher Covington <ccovingt@...cinc.com>,
Tomasz Nowicki <tn@...ihalf.com>
Subject: Re: [PATCH 1/1] [V2] irqchip: gicv3-its: Introduce indirect
device-ITT table support
Hi Marc,
On 01/26/2016 05:31 AM, Marc Zyngier wrote:
> Shanker,
>
> On 25/01/16 22:58, Shanker Donthineni wrote:
>> Current ITS driver implementation limits the device ID to a few
>> number of bits depending on memory that has been allocated to a
>> flat DEV-ITT table. Some of the devices are not usable when device
>> ID is spread out across a large range of 32 bit values.
> That's not really it. Indirect tables allow the system not to waste
> memory on very sparse tables (you could implement flat 32bit device ID
> tables by carving out memory out of the Linux managed memory).
I agree, I will change the commit text to reflect your comments in V3 patch.
>> This patch covers more DEVID bits by implementing the GIC-ITS indirect
>> device table. This feature is enabled automatically during driver
>> probe if the flat table is not adequate to hold all DEVID bits.
>>
>> Signed-off-by: Shanker Donthineni <shankerd@...eaurora.org>
>> ---
>> [v1]->[v2]:
>> -Removed the line "Change-Id: I9c6d005dexxx" from commit message.
>> -Fixed the its->max_devid field initialization.
>>
>> drivers/irqchip/irq-gic-v3-its.c | 79 +++++++++++++++++++++++++++++++++++---
>> include/linux/irqchip/arm-gic-v3.h | 1 +
>> 2 files changed, 75 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index e23d1d1..408a358 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -41,6 +41,8 @@
>>
>> #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING (1ULL << 0)
>> #define ITS_FLAGS_WORKAROUND_CAVIUM_22375 (1ULL << 1)
>> +#define ITS_FLAGS_DEVICE_NEEDS_FLUSHING (1ULL << 2)
>> +#define ITS_FLAGS_INDIRECT_DEVICE_TABLE (1ULL << 3)
>>
>> #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING (1 << 0)
>>
>> @@ -71,6 +73,10 @@ struct its_node {
>> struct list_head its_device_list;
>> u64 flags;
>> u32 ite_size;
>> + u32 dev_table_idx;
>> + u32 dev_table_shift;
>> + u32 max_devid;
>> + u32 order;
> What are these new fields for?
These four fields are used for handling level-2 device table memory
allocation.
dev_table_idx - which array index in itx->tables[] holds level-one
device table.
order - page order of level-2 device table, reflects
size field (7..0) of GITS_BASER_DEVICE register.
dev_table_shift - contains the number of DEVID bits occupied by
level-2 device table.
max_devid - highest device id that can be supported by ITS
driver (will refactor to reflect your other comments below) .
>> };
>>
>> #define ITS_ITT_ALIGN SZ_256
>> @@ -824,6 +830,8 @@ static int its_alloc_tables(const char *node_name, struct its_node *its)
>> u64 typer;
>> u32 ids;
>>
>> +#define ITS_DEVICE_MAX_ORDER min(MAX_ORDER, get_order(SZ_8M))
> Where is this 8MB value coming from?
Chose 8MB because It is an optimal value, It covers complete 32bit DEVID
space, with assumption device
table entry size is 8bytes and ITS hardware support 64K page size.
Example1 with ITS page size 64K + device table entry size 8bytes:
Level-1: ((page_order(8MB) – 1 ) * PAGE_SIZE) /level-one entry size =
4M/8 = 19bits
Level-2: 64K / 8 (device table entry size) = 13bits. (64K came from
default ITS page size)
with 8-Bytes device entry size = handles 32bit DEVID
with 16-Bytes device entry size = handles 31bit DEVID
with 32-Bytes device entry size = handles 30bit DEVID
Example2 with ITS page size 16K + device table entry size 8bytes:
Level-1: ((page_order(8MB) – 1 ) * PAGE_SIZE) /level-one entry size =
4M/8 = 19bits
Level-2: 16K / 8 (device table entry size) = 11bits. (for ITS hardware
that doesn't support 64K page size)
with 8-Bytes device entry size = handles 30bit DEVID
with 16-Bytes device entry size = handles 29bit DEVID
with 32-Bytes device entry size = handles 28bit DEVID
>> +
>> if (its->flags & ITS_FLAGS_WORKAROUND_CAVIUM_22375) {
>> /*
>> * erratum 22375: only alloc 8MB table size
>> @@ -867,11 +875,12 @@ static int its_alloc_tables(const char *node_name, struct its_node *its)
>> */
>> order = max(get_order((1UL << ids) * entry_size),
>> order);
>> - if (order >= MAX_ORDER) {
>> - order = MAX_ORDER - 1;
>> - pr_warn("%s: Device Table too large, reduce its page order to %u\n",
>> - node_name, order);
>> - }
>> + if (order >= ITS_DEVICE_MAX_ORDER) {
>> + /* Update flags for two-level setup */
>> + its->flags |= ITS_FLAGS_INDIRECT_DEVICE_TABLE;
>> + order = ITS_DEVICE_MAX_ORDER - 1;
>> + } else
>> + its->max_devid = (1 << ids) - 1;
> We deal with the ID space in number of bits, not in number of devices we
> can represent. Yes, there is a direct relationship between the two, but
> I'd like to keep it consistent with the architecture specification.
I agree, will stick to ID space and add macro for conversion if need
after refactoring code.
>> }
>>
>> alloc_size = (1 << order) * PAGE_SIZE;
>> @@ -911,6 +920,26 @@ retry_baser:
>> break;
>> }
>>
>> + /* Enable two-level (indirect) device table if it is required */
>> + if ((type == GITS_BASER_TYPE_DEVICE) &&
>> + (its->flags & ITS_FLAGS_INDIRECT_DEVICE_TABLE)) {
>> + u32 shift = ilog2(psz / entry_size);
>> + u32 max_ids = ilog2(alloc_size >> 3) + shift;
> Please explain this calculation.
shift = number of LSB DEVID bits that are covered by level-2
page table.
i.e. (Level-2 device table size ) / (device table
entry size)
max_ids = level-1 device table size + shift (level-2 device table size)
i.e. (level-1 size) / level_1_entry_size (8) + shift
>> +
>> + if (ids > max_ids) {
>> + pr_warn(
>> + "ITS: @%pa DEVID too large reduce %u->%u\n",
>> + &its->phys_base, ids, max_ids);
>> + }
>> + its->max_devid = (1UL << max_ids) - 1;
>> + its->order = get_order(psz);
>> + its->dev_table_idx = i;
>> + its->dev_table_shift = shift;
>> + if (!(val & GITS_BASER_SHAREABILITY_MASK))
>> + its->flags |= ITS_FLAGS_DEVICE_NEEDS_FLUSHING;
> You can only evaluate the shareability after having written something,
> and read it back. Also, the name of the flag is a bit wrong: it is not
> the device that needs flushing, bit the ITS Device table.
there is a “goto retry_baser” statement at the end of “for loop” that
triggers above code logic that is repeated
whenever the ITS hardware doesn’t stick shareability bit.
>> + val |= GITS_BASER_INDIRECT;
> Erm... Do you realize that this is an optional feature? What happens if
> the HW doesn't support it?
I will refactor code to fix the issue in V3 patch.
>> + }
>> +
>> val |= alloc_pages - 1;
>>
>> writeq_relaxed(val, its->base + GITS_BASER + i * 8);
>> @@ -1134,6 +1163,32 @@ static struct its_device *its_find_device(struct its_node *its, u32 dev_id)
>> return its_dev;
>> }
>>
>> +static int its_alloc_device_table(struct its_node *its, u32 dev_id)
>> +{
>> + u64 *devtbl = its->tables[its->dev_table_idx];
>> + u32 alloc_size = (1 << its->order) * PAGE_SIZE;
>> + u32 idx = dev_id >> its->dev_table_shift;
> All this should be explained. I don't want to have to refer to the spec
> to understand how you're computing things. It also means that you need
> to define some helpers.
I will add helper functions/macros for handy conversion.
>> + struct page *page;
>> +
>> + /* Do nothing if the level-2 DEV-ITT entry was mapped earlier */
> There is no such thing as a DEV-ITT. There is a Device table (what you
> are allocating here), and a per-device ITT, which is not dealt with here
> at all.
I will rename all references of DEV-ITT to device table.
>> + if (devtbl[idx])
>> + return 0;
>> +
>> + /* Allocate memory for level-2 device table & map to level-1 table */
>> + page = alloc_pages(GFP_KERNEL | __GFP_ZERO, its->order);
>> + if (!page)
>> + return -ENOMEM;
>> +
>> + devtbl[idx] = page_to_phys(page) | GITS_BASER_VALID;
>> +
>> + if (its->flags & ITS_FLAGS_DEVICE_NEEDS_FLUSHING) {
>> + __flush_dcache_area(page_address(page), alloc_size);
>> + __flush_dcache_area(devtbl + idx, sizeof(*devtbl));
>> + }
>> +
>> + return 0;
> It really time we convert this driver to use dma_alloc_coherent. This is
> getting out of control...
dma_alloc_coherent() API operates on “struct device”. We don’t have
such device object available in current ITS driver to call DMA API.
>> +}
>> +
>> static struct its_device *its_create_device(struct its_node *its, u32 dev_id,
>> int nvecs)
>> {
>> @@ -1147,6 +1202,20 @@ static struct its_device *its_create_device(struct its_node *its, u32 dev_id,
>> int nr_ites;
>> int sz;
>>
>> + if (dev_id > its->max_devid) {
>> + pr_err("ITS: dev_id too large %d\n", dev_id);
>> + return NULL;
>> + }
> Under which circumstances can this actually happen?
Current driver assumes the device memory has been allocated to cover
all possible DEVIDs but is not always true.
This is perfect as long as sparse DEVID is small and if we don’t see the
message “Device Table too large, reduce its page order to”
in its_alloc_tables() .
As per GICv3-ITS spec, the MAPD command with DEV ID exceeding maximum
value supported is an error. The current driver logic
doesn't protect against this error.
>> +
>> + /* Ensure memory for level-2 table is allocated for dev_id */
>> + if (its->flags & ITS_FLAGS_INDIRECT_DEVICE_TABLE) {
>> + int ret;
>> +
>> + ret = its_alloc_device_table(its, dev_id);
>> + if (ret)
>> + return NULL;
>> + }
>> +
>> dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>> /*
>> * At least one bit of EventID is being used, hence a minimum
>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>> index d5d798b..04dfe09 100644
>> --- a/include/linux/irqchip/arm-gic-v3.h
>> +++ b/include/linux/irqchip/arm-gic-v3.h
>> @@ -206,6 +206,7 @@
>> #define GITS_BASER_NR_REGS 8
>>
>> #define GITS_BASER_VALID (1UL << 63)
>> +#define GITS_BASER_INDIRECT (1UL << 62)
>> #define GITS_BASER_nCnB (0UL << 59)
>> #define GITS_BASER_nC (1UL << 59)
>> #define GITS_BASER_RaWt (2UL << 59)
>>
> Thanks,
>
> M.
Powered by blists - more mailing lists