hdata: Move 'HRMOR_BIT' macro to header file

Message ID 20180315074356.31473-1-hegdevasant@linux.vnet.ibm.com
State Changes Requested
Headers show
Series
  • hdata: Move 'HRMOR_BIT' macro to header file
Related show

Commit Message

Vasant Hegde March 15, 2018, 7:43 a.m.
Its already defined twice. Lets move it to header file.

Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
---
 hdata/memory.c | 2 --
 hdata/spira.c  | 5 ++---
 hdata/spira.h  | 6 ++++++
 3 files changed, 8 insertions(+), 5 deletions(-)

Comments

Oliver March 19, 2018, 2:29 a.m. | #1
On Thu, Mar 15, 2018 at 6:43 PM, Vasant Hegde
<hegdevasant@linux.vnet.ibm.com> wrote:
> Its already defined twice. Lets move it to header file.
>
> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> ---
>  hdata/memory.c | 2 --
>  hdata/spira.c  | 5 ++---
>  hdata/spira.h  | 6 ++++++
>  3 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/hdata/memory.c b/hdata/memory.c
> index 27dc559f5..a83898955 100644
> --- a/hdata/memory.c
> +++ b/hdata/memory.c
> @@ -602,8 +602,6 @@ static struct dt_node *add_hb_reserve_node(const char *name, u64 start, u64 end)
>         return node;
>  }
>
> -#define HRMOR_BIT (1ul << 63)
> -
>  static void get_hb_reserved_mem(struct HDIF_common_hdr *ms_vpd)
>  {
>         const struct msvpd_hb_reserved_mem *hb_resv_mem;
> diff --git a/hdata/spira.c b/hdata/spira.c
> index 0724dcc42..19f456a1e 100644
> --- a/hdata/spira.c
> +++ b/hdata/spira.c
> @@ -88,16 +88,15 @@ __section(".cpuctrl.data") struct cpu_ctl_init_data cpu_ctl_init_data = {
>   * To help the FSP distinguishing between TCE tokens and actual physical
>   * addresses, we set the top bit to 1 on physical addresses
>   */
> -#define ADDR_TOP_BIT   (1ul << 63)
>
>  __section(".mdst.data") struct dump_mdst_table init_mdst_table[2] = {
>         {
> -               .addr = CPU_TO_BE64(INMEM_CON_START | ADDR_TOP_BIT),
> +               .addr = CPU_TO_BE64(INMEM_CON_START | HRMOR_BIT),
>                 .type = CPU_TO_BE32(DUMP_REGION_CONSOLE),
>                 .size = CPU_TO_BE32(INMEM_CON_LEN),
>         },
>         {
> -               .addr = CPU_TO_BE64(HBRT_CON_START | ADDR_TOP_BIT),
> +               .addr = CPU_TO_BE64(HBRT_CON_START | HRMOR_BIT),
>                 .type = CPU_TO_BE32(DUMP_REGION_HBRT_LOG),
>                 .size = CPU_TO_BE32(HBRT_CON_LEN),
>         },
> diff --git a/hdata/spira.h b/hdata/spira.h
> index 39e0e3333..d8421dcb6 100644
> --- a/hdata/spira.h
> +++ b/hdata/spira.h
> @@ -19,6 +19,12 @@
>
>  #include "hdif.h"
>

> +/*
> + * To help the FSP/hostboot distinguishing between physical address and relative
> + * address/TCE tokens, we set the top bit to 1 on physical addresses.
> + */
> +#define HRMOR_BIT (1ul << 63)

I don't understand this comment at all. On the host having bit 63 (ppc
bit 0 set) causes
the core to ignore the HRMOR setting when in hypervisor real mode. As
far as I know this is
why hostboot sets it. In OPAL we always run with HRMOR set to zero so
it doesn't do anything
for us (hence removing it), but if the FSP treats it as some kind of
flag then that usage needs
to be more clearly documented here.


>  /*
>   * The SPIRA structure
>   *
> --
> 2.14.3
>
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
Vasant Hegde March 21, 2018, 8:03 a.m. | #2
On 03/19/2018 07:59 AM, Oliver wrote:
> On Thu, Mar 15, 2018 at 6:43 PM, Vasant Hegde
> <hegdevasant@linux.vnet.ibm.com> wrote:
>> Its already defined twice. Lets move it to header file.
>>
>> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
>> ---
>>   hdata/memory.c | 2 --
>>   hdata/spira.c  | 5 ++---
>>   hdata/spira.h  | 6 ++++++
>>   3 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/hdata/memory.c b/hdata/memory.c
>> index 27dc559f5..a83898955 100644
>> --- a/hdata/memory.c
>> +++ b/hdata/memory.c
>> @@ -602,8 +602,6 @@ static struct dt_node *add_hb_reserve_node(const char *name, u64 start, u64 end)
>>          return node;
>>   }
>>
>> -#define HRMOR_BIT (1ul << 63)
>> -
>>   static void get_hb_reserved_mem(struct HDIF_common_hdr *ms_vpd)
>>   {
>>          const struct msvpd_hb_reserved_mem *hb_resv_mem;
>> diff --git a/hdata/spira.c b/hdata/spira.c
>> index 0724dcc42..19f456a1e 100644
>> --- a/hdata/spira.c
>> +++ b/hdata/spira.c
>> @@ -88,16 +88,15 @@ __section(".cpuctrl.data") struct cpu_ctl_init_data cpu_ctl_init_data = {
>>    * To help the FSP distinguishing between TCE tokens and actual physical
>>    * addresses, we set the top bit to 1 on physical addresses
>>    */
>> -#define ADDR_TOP_BIT   (1ul << 63)
>>
>>   __section(".mdst.data") struct dump_mdst_table init_mdst_table[2] = {
>>          {
>> -               .addr = CPU_TO_BE64(INMEM_CON_START | ADDR_TOP_BIT),
>> +               .addr = CPU_TO_BE64(INMEM_CON_START | HRMOR_BIT),
>>                  .type = CPU_TO_BE32(DUMP_REGION_CONSOLE),
>>                  .size = CPU_TO_BE32(INMEM_CON_LEN),
>>          },
>>          {
>> -               .addr = CPU_TO_BE64(HBRT_CON_START | ADDR_TOP_BIT),
>> +               .addr = CPU_TO_BE64(HBRT_CON_START | HRMOR_BIT),
>>                  .type = CPU_TO_BE32(DUMP_REGION_HBRT_LOG),
>>                  .size = CPU_TO_BE32(HBRT_CON_LEN),
>>          },
>> diff --git a/hdata/spira.h b/hdata/spira.h
>> index 39e0e3333..d8421dcb6 100644
>> --- a/hdata/spira.h
>> +++ b/hdata/spira.h
>> @@ -19,6 +19,12 @@
>>
>>   #include "hdif.h"
>>
> 
>> +/*
>> + * To help the FSP/hostboot distinguishing between physical address and relative
>> + * address/TCE tokens, we set the top bit to 1 on physical addresses.
>> + */
>> +#define HRMOR_BIT (1ul << 63)
> 
> I don't understand this comment at all. On the host having bit 63 (ppc
> bit 0 set) causes
> the core to ignore the HRMOR setting when in hypervisor real mode. As
> far as I know this is
> why hostboot sets it.

AFAIK if we set this bit hostboot thinks its real address. Otherwise it uses 
HRMOR and payload_base to get actual address (At least that's how its working in 
dump collection path).

I want to move this macro to header file ... So that I can use this bit while 
updating MDST
and MDDT table for MPIPL case.


>  In OPAL we always run with HRMOR set to zero so
> it doesn't do anything
> for us (hence removing it), but if the FSP treats it as some kind of
> flag then that usage needs
> to be more clearly documented here.

IIRC if top bit is set, then FSP thinks its real address. Otherwise its TCE 
mapped address.

-Vasant
Vasant Hegde March 22, 2018, 8:52 a.m. | #3
On 03/21/2018 01:33 PM, Vasant Hegde wrote:
> On 03/19/2018 07:59 AM, Oliver wrote:
>> On Thu, Mar 15, 2018 at 6:43 PM, Vasant Hegde
>> <hegdevasant@linux.vnet.ibm.com> wrote:
>>> Its already defined twice. Lets move it to header file.
>>>
>>> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
>>> ---
>>>   hdata/memory.c | 2 --
>>>   hdata/spira.c  | 5 ++---
>>>   hdata/spira.h  | 6 ++++++
>>>   3 files changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hdata/memory.c b/hdata/memory.c
>>> index 27dc559f5..a83898955 100644
>>> --- a/hdata/memory.c
>>> +++ b/hdata/memory.c
>>> @@ -602,8 +602,6 @@ static struct dt_node *add_hb_reserve_node(const char 
>>> *name, u64 start, u64 end)
>>>          return node;
>>>   }
>>>
>>> -#define HRMOR_BIT (1ul << 63)
>>> -
>>>   static void get_hb_reserved_mem(struct HDIF_common_hdr *ms_vpd)
>>>   {
>>>          const struct msvpd_hb_reserved_mem *hb_resv_mem;
>>> diff --git a/hdata/spira.c b/hdata/spira.c
>>> index 0724dcc42..19f456a1e 100644
>>> --- a/hdata/spira.c
>>> +++ b/hdata/spira.c
>>> @@ -88,16 +88,15 @@ __section(".cpuctrl.data") struct cpu_ctl_init_data 
>>> cpu_ctl_init_data = {
>>>    * To help the FSP distinguishing between TCE tokens and actual physical
>>>    * addresses, we set the top bit to 1 on physical addresses
>>>    */
>>> -#define ADDR_TOP_BIT   (1ul << 63)
>>>
>>>   __section(".mdst.data") struct dump_mdst_table init_mdst_table[2] = {
>>>          {
>>> -               .addr = CPU_TO_BE64(INMEM_CON_START | ADDR_TOP_BIT),
>>> +               .addr = CPU_TO_BE64(INMEM_CON_START | HRMOR_BIT),
>>>                  .type = CPU_TO_BE32(DUMP_REGION_CONSOLE),
>>>                  .size = CPU_TO_BE32(INMEM_CON_LEN),
>>>          },
>>>          {
>>> -               .addr = CPU_TO_BE64(HBRT_CON_START | ADDR_TOP_BIT),
>>> +               .addr = CPU_TO_BE64(HBRT_CON_START | HRMOR_BIT),
>>>                  .type = CPU_TO_BE32(DUMP_REGION_HBRT_LOG),
>>>                  .size = CPU_TO_BE32(HBRT_CON_LEN),
>>>          },
>>> diff --git a/hdata/spira.h b/hdata/spira.h
>>> index 39e0e3333..d8421dcb6 100644
>>> --- a/hdata/spira.h
>>> +++ b/hdata/spira.h
>>> @@ -19,6 +19,12 @@
>>>
>>>   #include "hdif.h"
>>>
>>
>>> +/*
>>> + * To help the FSP/hostboot distinguishing between physical address and 
>>> relative
>>> + * address/TCE tokens, we set the top bit to 1 on physical addresses.
>>> + */
>>> +#define HRMOR_BIT (1ul << 63)
>>
>> I don't understand this comment at all. On the host having bit 63 (ppc
>> bit 0 set) causes
>> the core to ignore the HRMOR setting when in hypervisor real mode. As
>> far as I know this is
>> why hostboot sets it.
> 
> AFAIK if we set this bit hostboot thinks its real address. Otherwise it uses 
> HRMOR and payload_base to get actual address (At least that's how its working in 
> dump collection path).
> 
> I want to move this macro to header file ... So that I can use this bit while 
> updating MDST
> and MDDT table for MPIPL case.
> 
> 
>>  In OPAL we always run with HRMOR set to zero so
>> it doesn't do anything
>> for us (hence removing it), but if the FSP treats it as some kind of
>> flag then that usage needs
>> to be more clearly documented here.
> 
> IIRC if top bit is set, then FSP thinks its real address. Otherwise its TCE 
> mapped address.

Confirmed with FSP folks. What I mentioned here is correct.

May be I will update comment and resend the patch.

-Vasant

Patch

diff --git a/hdata/memory.c b/hdata/memory.c
index 27dc559f5..a83898955 100644
--- a/hdata/memory.c
+++ b/hdata/memory.c
@@ -602,8 +602,6 @@  static struct dt_node *add_hb_reserve_node(const char *name, u64 start, u64 end)
 	return node;
 }
 
-#define HRMOR_BIT (1ul << 63)
-
 static void get_hb_reserved_mem(struct HDIF_common_hdr *ms_vpd)
 {
 	const struct msvpd_hb_reserved_mem *hb_resv_mem;
diff --git a/hdata/spira.c b/hdata/spira.c
index 0724dcc42..19f456a1e 100644
--- a/hdata/spira.c
+++ b/hdata/spira.c
@@ -88,16 +88,15 @@  __section(".cpuctrl.data") struct cpu_ctl_init_data cpu_ctl_init_data = {
  * To help the FSP distinguishing between TCE tokens and actual physical
  * addresses, we set the top bit to 1 on physical addresses
  */
-#define ADDR_TOP_BIT	(1ul << 63)
 
 __section(".mdst.data") struct dump_mdst_table init_mdst_table[2] = {
 	{
-		.addr = CPU_TO_BE64(INMEM_CON_START | ADDR_TOP_BIT),
+		.addr = CPU_TO_BE64(INMEM_CON_START | HRMOR_BIT),
 		.type = CPU_TO_BE32(DUMP_REGION_CONSOLE),
 		.size = CPU_TO_BE32(INMEM_CON_LEN),
 	},
 	{
-		.addr = CPU_TO_BE64(HBRT_CON_START | ADDR_TOP_BIT),
+		.addr = CPU_TO_BE64(HBRT_CON_START | HRMOR_BIT),
 		.type = CPU_TO_BE32(DUMP_REGION_HBRT_LOG),
 		.size = CPU_TO_BE32(HBRT_CON_LEN),
 	},
diff --git a/hdata/spira.h b/hdata/spira.h
index 39e0e3333..d8421dcb6 100644
--- a/hdata/spira.h
+++ b/hdata/spira.h
@@ -19,6 +19,12 @@ 
 
 #include "hdif.h"
 
+/*
+ * To help the FSP/hostboot distinguishing between physical address and relative
+ * address/TCE tokens, we set the top bit to 1 on physical addresses.
+ */
+#define HRMOR_BIT (1ul << 63)
+
 /*
  * The SPIRA structure
  *