Re: [PATCH V3 2/7] cxl: Remove unused values in bare-metal environment.

[christophe lombard <clombard@linux.vnet.ibm.com>]

Le 03/04/2017 à 14:27, Frederic Barrat a écrit :
>
>
> Le 28/03/2017 à 17:14, Christophe Lombard a écrit :
>> The two fields pid and tid of the structure cxl_irq_info are only used
>> in the guest environment. To avoid confusion, it's not necessary
>> to fill the fields in the bare-metal environment. These two fields
>> are renamed to 'reserved' to avoid undefined behavior on bare-metal.
>> The PSL Process and Thread Identification Register (CXL_PSL_PID_TID_An)
>> is only used when attaching a dedicated process for PSL8 only. This
>> register goes away in CAIA2.
>>
>> Signed-off-by: Christophe Lombard <clombard@linux.vnet.ibm.com>
>> ---
>>  drivers/misc/cxl/cxl.h    | 13 +++++++------
>>  drivers/misc/cxl/hcalls.c |  4 ++--
>>  drivers/misc/cxl/native.c |  5 -----
>>  3 files changed, 9 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
>> index 79e60ec..2bbe077 100644
>> --- a/drivers/misc/cxl/cxl.h
>> +++ b/drivers/misc/cxl/cxl.h
>> @@ -895,19 +895,20 @@ int __detach_context(struct cxl_context *ctx);
>>   * plpar_hcall9() in hvCall.S
>>   * As a consequence:
>>   * - we don't need to do any endianness conversion
>> - * - the pid and tid are an exception. They are 32-bit values 
>> returned in
>> - *   the same 64-bit register. So we do need to worry about byte 
>> ordering.
>> + * - the pid (reserved0) and tid (reserved1) are an exception. They are
>> + *   32-bit values returned in the same 64-bit register. So we do need
>> + *   to worry about byte ordering.
>>   */
>>  struct cxl_irq_info {
>>      u64 dsisr;
>>      u64 dar;
>>      u64 dsr;
>>  #ifndef CONFIG_CPU_LITTLE_ENDIAN
>> -    u32 pid;
>> -    u32 tid;
>> +    u32 reserved0;
>> +    u32 reserved1;
>>  #else
>> -    u32 tid;
>> -    u32 pid;
>> +    u32 reserved1;
>> +    u32 reserved0;
>>  #endif
>
>
> The resulting structure looks weird/over-complicated. I think we could 
> get rid of the #ifdef about endianess by just declaring:
>
> struct cxl_irq_info {
>     u64 dsisr;
>     u64 dar;
>     u64 dsr;
>     u64 pid_tid
>     u64 afu_err;
> ...
>
> and just print the 64-bit value in hcalls.c and let the reader figure 
> out the order. It's just informational anyway.
>
>
>   Fred
>
>

You are right, I will update the code.


>
>>      u64 afu_err;
>>      u64 errstat;
>> diff --git a/drivers/misc/cxl/hcalls.c b/drivers/misc/cxl/hcalls.c
>> index d6d11f4..142a095 100644
>> --- a/drivers/misc/cxl/hcalls.c
>> +++ b/drivers/misc/cxl/hcalls.c
>> @@ -414,8 +414,8 @@ long cxl_h_collect_int_info(u64 unit_address, u64 
>> process_token,
>>      switch (rc) {
>>      case H_SUCCESS:     /* The interrupt info is returned in return 
>> registers. */
>>          pr_devel("dsisr:%#llx, dar:%#llx, dsr:%#llx, pid:%u, tid:%u, 
>> afu_err:%#llx, errstat:%#llx\n",
>> -            info->dsisr, info->dar, info->dsr, info->pid,
>> -            info->tid, info->afu_err, info->errstat);
>> +            info->dsisr, info->dar, info->dsr, info->reserved0,
>> +            info->reserved1, info->afu_err, info->errstat);
>>          return 0;
>>      case H_PARAMETER:   /* An incorrect parameter was supplied. */
>>          return -EINVAL;
>> diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
>> index 7ae7105..7257e8b 100644
>> --- a/drivers/misc/cxl/native.c
>> +++ b/drivers/misc/cxl/native.c
>> @@ -859,8 +859,6 @@ static int native_detach_process(struct 
>> cxl_context *ctx)
>>
>>  static int native_get_irq_info(struct cxl_afu *afu, struct 
>> cxl_irq_info *info)
>>  {
>> -    u64 pidtid;
>> -
>>      /* If the adapter has gone away, we can't get any meaningful
>>       * information.
>>       */
>> @@ -870,9 +868,6 @@ static int native_get_irq_info(struct cxl_afu 
>> *afu, struct cxl_irq_info *info)
>>      info->dsisr = cxl_p2n_read(afu, CXL_PSL_DSISR_An);
>>      info->dar = cxl_p2n_read(afu, CXL_PSL_DAR_An);
>>      info->dsr = cxl_p2n_read(afu, CXL_PSL_DSR_An);
>> -    pidtid = cxl_p2n_read(afu, CXL_PSL_PID_TID_An);
>> -    info->pid = pidtid >> 32;
>> -    info->tid = pidtid & 0xffffffff;
>>      info->afu_err = cxl_p2n_read(afu, CXL_AFU_ERR_An);
>>      info->errstat = cxl_p2n_read(afu, CXL_PSL_ErrStat_An);
>>      info->proc_handle = 0;
>>