Re: [PATCH] irqchip/gicv3-its: Add missing changes to support 52bit physical address

[Shanker Donthineni]

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 
>>  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 
>>> ---
>>>  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(>raw_cmd[2], itt_addr >> 8, 50, 8);
>>> +   its_mask_encode(>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(>raw_cmd[2], target_addr >> 16, 50, 16);
>>> +   its_mask_encode(>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(>raw_cmd[3], vpt_pa >> 16, 50, 16);
>>> +   its_mask_encode(>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 & 0x   
> )
> 
> 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 = 

Re: [PATCH] irqchip/gicv3-its: Add missing changes to support 52bit physical address

[Shanker Donthineni]

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 
>>  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 
>>> ---
>>>  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(>raw_cmd[2], itt_addr >> 8, 50, 8);
>>> +   its_mask_encode(>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(>raw_cmd[2], target_addr >> 16, 50, 16);
>>> +   its_mask_encode(>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(>raw_cmd[3], vpt_pa >> 16, 50, 16);
>>> +   its_mask_encode(>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 & 0x   
> )
> 
> 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);
> 
>   
> 

Re: [PATCH] irqchip/gicv3-its: Add missing changes to support 52bit physical address

[Shanker Donthineni]

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 
>  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 
>> ---
>>  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(>raw_cmd[2], itt_addr >> 8, 50, 8);
>> +its_mask_encode(>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(>raw_cmd[2], target_addr >> 16, 50, 16);
>> +its_mask_encode(>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(>raw_cmd[3], vpt_pa >> 16, 50, 16);
>> +its_mask_encode(>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_MASKGENMASK_ULL(47, 12)

(baser_phys & (~GITS_BASER_PHYS_MASK)) becomes (baser_phys & 0x   
)

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 & 

Re: [PATCH] irqchip/gicv3-its: Add missing changes to support 52bit physical address

[Shanker Donthineni]

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 
>  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 
>> ---
>>  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(>raw_cmd[2], itt_addr >> 8, 50, 8);
>> +its_mask_encode(>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(>raw_cmd[2], target_addr >> 16, 50, 16);
>> +its_mask_encode(>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(>raw_cmd[3], vpt_pa >> 16, 50, 16);
>> +its_mask_encode(>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_MASKGENMASK_ULL(47, 12)

(baser_phys & (~GITS_BASER_PHYS_MASK)) becomes (baser_phys & 0x   
)

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 

Re: [PATCH] irqchip/gicv3-its: Add missing changes to support 52bit physical address

[Marc Zyngier]

On Sat, Oct 07 2017 at 12:13:24 am BST, Shanker Donthineni 
 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 
> ---
>  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(>raw_cmd[2], itt_addr >> 8, 50, 8);
> + its_mask_encode(>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(>raw_cmd[2], target_addr >> 16, 50, 16);
> + its_mask_encode(>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(>raw_cmd[3], vpt_pa >> 16, 50, 16);
> + its_mask_encode(>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.

This probably wants to be predicated with a 64kB CPU page size (which is
the only valid configuration for 52bit PA anyway).

Thanks,

M.
-- 
Jazz is not dead. It just smells funny.

Re: [PATCH] irqchip/gicv3-its: Add missing changes to support 52bit physical address

[Marc Zyngier]

On Sat, Oct 07 2017 at 12:13:24 am BST, Shanker Donthineni 
 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 
> ---
>  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(>raw_cmd[2], itt_addr >> 8, 50, 8);
> + its_mask_encode(>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(>raw_cmd[2], target_addr >> 16, 50, 16);
> + its_mask_encode(>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(>raw_cmd[3], vpt_pa >> 16, 50, 16);
> + its_mask_encode(>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.

This probably wants to be predicated with a 64kB CPU page size (which is
the only valid configuration for 52bit PA anyway).

Thanks,

M.
-- 
Jazz is not dead. It just smells funny.

[PATCH] irqchip/gicv3-its: Add missing changes to support 52bit physical address

[Shanker Donthineni]

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 
---
 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(>raw_cmd[2], itt_addr >> 8, 50, 8);
+   its_mask_encode(>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(>raw_cmd[2], target_addr >> 16, 50, 16);
+   its_mask_encode(>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(>raw_cmd[3], vpt_pa >> 16, 50, 16);
+   its_mask_encode(>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);
+   }
+
 retry_baser:
-   val = (virt_to_phys(base)|
+   val = (baser_phys|
(type << GITS_BASER_TYPE_SHIFT)  |
((esz - 1) << GITS_BASER_ENTRY_SIZE_SHIFT)   |
((alloc_pages - 1) << GITS_BASER_PAGES_SHIFT)|
diff --git a/include/linux/irqchip/arm-gic-v3.h 
b/include/linux/irqchip/arm-gic-v3.h
index b8b5998..4e82f73 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -373,6 +373,9 @@
 #define GITS_BASER_ENTRY_SIZE_SHIFT(48)
 #define GITS_BASER_ENTRY_SIZE(r)   r) >> GITS_BASER_ENTRY_SIZE_SHIFT) 
& 0x1f) + 1)
 #define GITS_BASER_ENTRY_SIZE_MASK GENMASK_ULL(52, 48)
+#define GITS_BASER_PHYS_MASK   GENMASK_ULL(47, 12)
+#define GITS_BASER_64K_PHYS(phys)  \
+   (((phys) | phys) >> 48) & 0xF) << 12)) & GITS_BASER_PHYS_MASK)
 #define GITS_BASER_SHAREABILITY_SHIFT  (10)
 #define GITS_BASER_InnerShareable  \
GIC_BASER_SHAREABILITY(GITS_BASER, InnerShareable)
-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, 
Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

[PATCH] irqchip/gicv3-its: Add missing changes to support 52bit physical address

[Shanker Donthineni]

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 
---
 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(>raw_cmd[2], itt_addr >> 8, 50, 8);
+   its_mask_encode(>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(>raw_cmd[2], target_addr >> 16, 50, 16);
+   its_mask_encode(>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(>raw_cmd[3], vpt_pa >> 16, 50, 16);
+   its_mask_encode(>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);
+   }
+
 retry_baser:
-   val = (virt_to_phys(base)|
+   val = (baser_phys|
(type << GITS_BASER_TYPE_SHIFT)  |
((esz - 1) << GITS_BASER_ENTRY_SIZE_SHIFT)   |
((alloc_pages - 1) << GITS_BASER_PAGES_SHIFT)|
diff --git a/include/linux/irqchip/arm-gic-v3.h 
b/include/linux/irqchip/arm-gic-v3.h
index b8b5998..4e82f73 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -373,6 +373,9 @@
 #define GITS_BASER_ENTRY_SIZE_SHIFT(48)
 #define GITS_BASER_ENTRY_SIZE(r)   r) >> GITS_BASER_ENTRY_SIZE_SHIFT) 
& 0x1f) + 1)
 #define GITS_BASER_ENTRY_SIZE_MASK GENMASK_ULL(52, 48)
+#define GITS_BASER_PHYS_MASK   GENMASK_ULL(47, 12)
+#define GITS_BASER_64K_PHYS(phys)  \
+   (((phys) | phys) >> 48) & 0xF) << 12)) & GITS_BASER_PHYS_MASK)
 #define GITS_BASER_SHAREABILITY_SHIFT  (10)
 #define GITS_BASER_InnerShareable  \
GIC_BASER_SHAREABILITY(GITS_BASER, InnerShareable)
-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, 
Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.