Hi Marc,
On 10/07/2017 07:59 AM, Shanker Donthineni wrote:
> 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);
> }
>
>
I've included your suggestion in v2 patch
https://patchwork.kernel.org/patch/9991405/ even though
it's not really required. But it helps to understand the code changes better
and I also added a
warning message in case of not supporting 52bit PA. Please comment on v2 patch
if any thing I missed.
--
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.
