[Skiboot] [PATCH v7 21/26] opal-dump: MPIPL endan conversions

[Vasant Hegde]

On 11/13/19 8:17 AM, Nicholas Piggin wrote:
> Vasant Hegde's on November 13, 2019 2:08 am:
>> On 11/11/19 11:19 AM, Nicholas Piggin wrote:
>>> Signed-off-by: Nicholas Piggin <npiggin at gmail.com>
>>> ---
>>>    core/opal-dump.c   | 80 +++++++++++++++++++++++++---------------------
>>>    include/opal-api.h | 14 ++++----
>>>    2 files changed, 51 insertions(+), 43 deletions(-)
>>>
>>
>>
>> .../...
>>
>>> @@ -426,13 +434,13 @@ static void post_mpipl_arch_regs_data(void)
>>>
>>>    	/* Fill CPU register details */
>>>    	opal_mpipl_cpu_data->version = OPAL_MPIPL_VERSION;
>>> -	opal_mpipl_cpu_data->cpu_data_version = proc_dump->version;
>>> +	opal_mpipl_cpu_data->cpu_data_version = cpu_to_be32((u32)proc_dump->version);
>>>    	opal_mpipl_cpu_data->cpu_data_size = proc_dump->thread_size;
>>>    	opal_mpipl_cpu_data->region_cnt = cpu_to_be32(1);
>>>
>>> -	opal_mpipl_cpu_data->region[0].src  = proc_dump->dest_addr & ~(HRMOR_BIT);
>>> -	opal_mpipl_cpu_data->region[0].dest = proc_dump->dest_addr & ~(HRMOR_BIT);
>>> -	opal_mpipl_cpu_data->region[0].size = proc_dump->act_size;
>>> +	opal_mpipl_cpu_data->region[0].src  = proc_dump->dest_addr & ~(cpu_to_be64(HRMOR_BIT));
>>> +	opal_mpipl_cpu_data->region[0].dest = proc_dump->dest_addr & ~(cpu_to_be64(HRMOR_BIT));
>>> +	opal_mpipl_cpu_data->region[0].size = cpu_to_be64(be32_to_cpu(proc_dump->act_size));
>>>
>>>    	/* Update tag */
>>>    	opal_mpipl_tags[OPAL_MPIPL_TAG_CPU] = (u64)opal_mpipl_cpu_data;
>>
>> We should convert opal_mpipl_cpu_data to BE here.
> 
> Ah, missed that because I didn't annotate the opal call types.
> 
> What about just doing the conversion in the opal calls? That tends to
> be the usual pattern is we keep our internal data structures in native
> endian, and then do the conversion at the point where we need to load/
> store hardware and OS memory.

Agree. We should do it in OPAL API call. From endianess point of view 
opal_mpipl_query_tag() is
somewhat special call...because for kernel passed tags we should return as is, 
and OPAL generated
tags we should convert to BE before sending it to kernel.


> 
>>
>>> @@ -442,7 +450,7 @@ static void post_mpipl_get_opal_data(void)
>>>    {
>>>    	struct mdrt_table *mdrt = (void *)(MDRT_TABLE_BASE);
>>>    	int i, j = 0, count = 0;
>>> -	u32 mdrt_cnt = ntuple_mdrt->act_cnt;
>>> +	int mdrt_cnt = be16_to_cpu(ntuple_mdrt->act_cnt);
>>>    	struct opal_mpipl_region *region;
>>>
>>>    	/* Count OPAL dump regions */
>>> @@ -466,8 +474,8 @@ static void post_mpipl_get_opal_data(void)
>>>
>>>    	/* Fill OPAL dump details */
>>>    	opal_mpipl_data->version = OPAL_MPIPL_VERSION;
>>> -	opal_mpipl_data->crashing_pir = mpipl_metadata->crashing_pir;
>>> -	opal_mpipl_data->region_cnt = count;
>>> +	opal_mpipl_data->crashing_pir = cpu_to_be32(mpipl_metadata->crashing_pir);
>>> +	opal_mpipl_data->region_cnt = cpu_to_be32(count);
>>>    	region = opal_mpipl_data->region;
>>>
>>>    	mdrt = (void *)(MDRT_TABLE_BASE);
>>> @@ -477,9 +485,9 @@ static void post_mpipl_get_opal_data(void)
>>>    			continue;
>>>    		}
>>>
>>> -		region[j].src  = mdrt->src_addr  & ~(HRMOR_BIT);
>>> -		region[j].dest = mdrt->dest_addr & ~(HRMOR_BIT);
>>> -		region[j].size = mdrt->size;
>>> +		region[j].src  = mdrt->src_addr  & ~(cpu_to_be64(HRMOR_BIT));
>>> +		region[j].dest = mdrt->dest_addr & ~(cpu_to_be64(HRMOR_BIT));
>>> +		region[j].size = cpu_to_be64(be32_to_cpu(mdrt->size));
>>>
>>>    		prlog(PR_NOTICE, "OPAL reserved region %d - src : 0x%llx, "
>>>    		      "dest : 0x%llx, size : 0x%llx\n", j, region[j].src,
>>
>> Can you fix above prlog as well?
>>
>> Also we have to convert `opal_mpipl_data` to BE before adding that to
>> `opal_mpipl_tags`.
> 
> Yeah I'll take another look at these. I'll do another tree-wide pass
> over the opal call APIs to make sure I haven't missed any other type
> annotations.

Thanks!

-Vasant