| Message ID | 1490714052-18902-3-git-send-email-clombard@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 29/03/17 02:14, Christophe Lombard wrote: > 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> Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.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. > */ Comment should explain that pid/tid are now reserved0/1 because we've decided not to use them at all.
Le 29/03/2017 à 02:21, Andrew Donnellan a écrit : > On 29/03/17 02:14, Christophe Lombard wrote: >> 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> > > Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.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. >> */ > > Comment should explain that pid/tid are now reserved0/1 because we've > decided not to use them at all. > ok, I will update the comment. thanks
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 > 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; >
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; >>
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 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;
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(-)