Hi Marc,
Thanks for your quick review comments.
On 10/07/2017 04:58 AM, Marc Zyngier wrote:
> On Sat, Oct 07 2017 at 12:13:24 am BST, Shanker Donthineni
> <shank...@codeaurora.org> wrote:
>> The current ITS driver works fine as long as normal memory and GICR
>> regions are located within the lower 48bit (>=0 && <2^48) physical
>> address space. Some of the registers GICR_PEND/PROP, GICR_VPEND/VPROP
>> and GITS_CBASER are handled properly but not all when configuring
>> the hardware with 52bit physical address.
>>
>> This patch does the following changes to support 52bit PA.
>> -Handle 52bit PA in GITS_BASERn.
>> -Fix ITT_addr width to 52bits, bits[51:8].
>> -Fix RDbase width to 52bits, bits[51:16].
>> -Fix VPT_addr width to 52bits, bits[51:16].
>>
>> Definition of the GITS_BASERn register when ITS PageSize is 64KB:
>> -Bits[47:16] of the register provide bits[47:16] of the table PA.
>> -Bits[15:12] of the register provide bits[51:48] of the table PA.
>> -Bits[15:00] of the base physical address are 0.
>>
>> Signed-off-by: Shanker Donthineni <shank...@codeaurora.org>
>> ---
>> drivers/irqchip/irq-gic-v3-its.c | 24 +++++++++++++++++++-----
>> include/linux/irqchip/arm-gic-v3.h | 3 +++
>> 2 files changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>> b/drivers/irqchip/irq-gic-v3-its.c
>> index e8d8934..e52c0da 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -308,7 +308,7 @@ static void its_encode_size(struct its_cmd_block *cmd,
>> u8 size)
>>
>> static void its_encode_itt(struct its_cmd_block *cmd, u64 itt_addr)
>> {
>> - its_mask_encode(&cmd->raw_cmd[2], itt_addr >> 8, 50, 8);
>> + its_mask_encode(&cmd->raw_cmd[2], itt_addr >> 8, 51, 8);
>> }
>>
>> static void its_encode_valid(struct its_cmd_block *cmd, int valid)
>> @@ -318,7 +318,7 @@ static void its_encode_valid(struct its_cmd_block *cmd,
>> int valid)
>>
>> static void its_encode_target(struct its_cmd_block *cmd, u64 target_addr)
>> {
>> - its_mask_encode(&cmd->raw_cmd[2], target_addr >> 16, 50, 16);
>> + its_mask_encode(&cmd->raw_cmd[2], target_addr >> 16, 51, 16);
>> }
>>
>> static void its_encode_collection(struct its_cmd_block *cmd, u16 col)
>> @@ -358,7 +358,7 @@ static void its_encode_its_list(struct its_cmd_block
>> *cmd, u16 its_list)
>>
>> static void its_encode_vpt_addr(struct its_cmd_block *cmd, u64 vpt_pa)
>> {
>> - its_mask_encode(&cmd->raw_cmd[3], vpt_pa >> 16, 50, 16);
>> + its_mask_encode(&cmd->raw_cmd[3], vpt_pa >> 16, 51, 16);
>> }
>>
>> static void its_encode_vpt_size(struct its_cmd_block *cmd, u8 vpt_size)
>> @@ -1478,9 +1478,9 @@ static int its_setup_baser(struct its_node *its,
>> struct its_baser *baser,
>> u64 val = its_read_baser(its, baser);
>> u64 esz = GITS_BASER_ENTRY_SIZE(val);
>> u64 type = GITS_BASER_TYPE(val);
>> + u64 baser_phys, tmp;
>> u32 alloc_pages;
>> void *base;
>> - u64 tmp;
>>
>> retry_alloc_baser:
>> alloc_pages = (PAGE_ORDER_TO_SIZE(order) / psz);
>> @@ -1496,8 +1496,22 @@ static int its_setup_baser(struct its_node *its,
>> struct its_baser *baser,
>> if (!base)
>> return -ENOMEM;
>>
>> + baser_phys = virt_to_phys(base);
>> +
>> + /* Check if the physical address of the memory is above 48bits */
>> + if (baser_phys & (~GITS_BASER_PHYS_MASK)) {
>> + /* 52bit PA is supported only when PageSize=64K */
>> + if (psz != SZ_64K) {
>> + free_pages((unsigned long)base, order);
>> + return -EFAULT;
>> + }
>> +
>> + /* Convert 52bit PA to 48bit field */
>> + baser_phys = GITS_BASER_64K_PHYS(baser_phys);
>> + }
>
> I'm not sure this is completely right. What guarantees that the memory
> you get is 64kB aligned? Your initial masking will do the wrong thing on
> a system with 16kB pages, and everything will break from that point on,
> as you're confusing the ITS page size with the CPU page size.
>
The first is check right, and the intention was to find out the upper bits
of the PA (bits[51:12]) have non-zero value irrespective of PS 64K/16K/4KB.
#define GITS_BASER_PHYS_MASK GENMASK_ULL(47, 12)
(baser_phys & (~GITS_BASER_PHYS_MASK)) becomes (baser_phys & 0xFFFF 0000 0000
0000)
We program GITS_BASERn PageSize based on 'psz' value.
However the current code has a hidden bug, we assume memory allocation is
aligned
on 64K/16K when the ITS PageSize (variable of 'psz') is 64K/16K. We have two
options
to solve the problem.
-fall-baclk to a lower ITS PageSize if the memory size is lower than 'psz'.
-allocate minimum of 64K for all BASERn tables.
Something like using the second method.
retry_alloc_baser:
alloc_pages = (min(PAGE_ORDER_TO_SIZE(order), 64K) / psz);
....
baser_phys = virt_to_phys(base);
/* Check if the physical address of the memory is above 48bits */
if (baser_phys & (~GITS_BASER_PHYS_MASK)) {
/* 52bit PA is supported only when PageSize=64K */
if (psz != SZ_64K) {
free_pages((unsigned long)base, order);
return -EFAULT;
}
/* Convert 52bit PA to 48bit field */
baser_phys = GITS_BASER_64K_PHYS(baser_phys);
}
> This probably wants to be predicated with a 64kB CPU page size (which is
> the only valid configuration for 52bit PA anyway).
>
> Thanks,
>
> M.
>
--
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux
Foundation Collaborative Project.
