[v2,1/4] cxl/core: Rename cxl_trace_hpa() to cxl_translate()

Commit Message

Alison Schofield May 8, 2024, 6:47 p.m. UTC

From: Alison Schofield <[e-mail]>

Although cxl_trace_hpa() is used to populate TRACE EVENTs with HPA
addresses, the work it performs is a translation (dpa->hpa), not a
trace. Rename it. Since this is the only translate work in CXL,
drop the _hpa suffix in the rename.

Suggested-by: Dan Williams <[e-mail]>
Signed-off-by: Alison Schofield <[e-mail]>
---
 drivers/cxl/core/core.h   | 4 ++--
 drivers/cxl/core/mbox.c   | 2 +-
 drivers/cxl/core/region.c | 2 +-
 drivers/cxl/core/trace.h  | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

Comments

Dan Williams May 30, 2024, 3:45 a.m. UTC | #1

alison.schofield@ wrote:
> From: Alison Schofield <[e-mail]>
> 
> Although cxl_trace_hpa() is used to populate TRACE EVENTs with HPA
> addresses, the work it performs is a translation (dpa->hpa), not a
> trace. Rename it. Since this is the only translate work in CXL,
> drop the _hpa suffix in the rename.
> 
> Suggested-by: Dan Williams <[e-mail]>
> Signed-off-by: Alison Schofield <[e-mail]>

LGTM

Reviewed-by: Dan Williams <[e-mail]>

Jonathan Cameron June 7, 2024, 2:40 p.m. UTC | #2

On Wed,  8 May 2024 11:47:50 -0700
[e-mail] wrote:

> From: Alison Schofield <[e-mail]>
> 
> Although cxl_trace_hpa() is used to populate TRACE EVENTs with HPA
> addresses, the work it performs is a translation (dpa->hpa), not a
> trace. Rename it. Since this is the only translate work in CXL,
> drop the _hpa suffix in the rename.
> 
> Suggested-by: Dan Williams <[e-mail]>
> Signed-off-by: Alison Schofield <[e-mail]>

Maybe makes some sense to keep the direction in the name?

I can see for things like PPR we may need to go HPA to DPA as
it'll be based on error counts that may be reported in HPA.

So Friday bikeshedding time. 
cxl_dpa_to_hpa()?


> ---
>  drivers/cxl/core/core.h   | 4 ++--
>  drivers/cxl/core/mbox.c   | 2 +-
>  drivers/cxl/core/region.c | 2 +-
>  drivers/cxl/core/trace.h  | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 625394486459..8ceebd3b51d2 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -28,12 +28,12 @@ int cxl_region_init(void);
>  void cxl_region_exit(void);
>  int cxl_get_poison_by_endpoint(struct cxl_port *port);
>  struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa);
> -u64 cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> +u64 cxl_translate(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
>  		  u64 dpa);
>  
>  #else
>  static inline u64
> -cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd, u64 dpa)
> +cxl_translate(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd, u64 dpa)
>  {
>  	return ULLONG_MAX;
>  }
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 2626f3fff201..edc54a1ca298 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -878,7 +878,7 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
>  		dpa = le64_to_cpu(evt->common.phys_addr) & CXL_DPA_MASK;
>  		cxlr = cxl_dpa_to_region(cxlmd, dpa);
>  		if (cxlr)
> -			hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
> +			hpa = cxl_translate(cxlr, cxlmd, dpa);
>  
>  		if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
>  			trace_cxl_general_media(cxlmd, type, cxlr, hpa,
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 00a9f0eef8dd..245edf748906 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2797,7 +2797,7 @@ static u64 cxl_dpa_to_hpa(u64 dpa,  struct cxl_region *cxlr,
>  	return hpa;
>  }
>  
> -u64 cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> +u64 cxl_translate(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
>  		  u64 dpa)
>  {
>  	struct cxl_region_params *p = &cxlr->params;
> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> index 07a0394b1d99..6925a6a31f01 100644
> --- a/drivers/cxl/core/trace.h
> +++ b/drivers/cxl/core/trace.h
> @@ -704,7 +704,7 @@ TRACE_EVENT(cxl_poison,
>  		if (cxlr) {
>  			__assign_str(region, dev_name(&cxlr->dev));
>  			memcpy(__entry->uuid, &cxlr->params.uuid, 16);
> -			__entry->hpa = cxl_trace_hpa(cxlr, cxlmd,
> +			__entry->hpa = cxl_translate(cxlr, cxlmd,
>  						     __entry->dpa);
>  		} else {
>  			__assign_str(region, "");

Alison Schofield June 7, 2024, 5:45 p.m. UTC | #3

On Fri, Jun 07, 2024 at 03:40:25PM +0100, Jonathan Cameron wrote:
> On Wed,  8 May 2024 11:47:50 -0700
> [e-mail] wrote:
> 
> > From: Alison Schofield <[e-mail]>
> > 
> > Although cxl_trace_hpa() is used to populate TRACE EVENTs with HPA
> > addresses, the work it performs is a translation (dpa->hpa), not a
> > trace. Rename it. Since this is the only translate work in CXL,
> > drop the _hpa suffix in the rename.
> > 
> > Suggested-by: Dan Williams <[e-mail]>
> > Signed-off-by: Alison Schofield <[e-mail]>
> 
> Maybe makes some sense to keep the direction in the name?
> 
> I can see for things like PPR we may need to go HPA to DPA as
> it'll be based on error counts that may be reported in HPA.
> 
> So Friday bikeshedding time. 
> cxl_dpa_to_hpa()?

Thanks for the review Jonathan!

Let's sync on the possible dpa->hpa->dpa use cases and then I'll
weigh in on your bikeshedding.

Agree we need HPA->DPA support for userspace. User needs ability
to find which device maps an HPA.

wrt PPR, do you think that is an in kernel need?
If the PPR maintenance was done in response to a CXL event, the DPA
is already in the event trace along with the HPA so I wouldn't expect
that a lookup is needed, at least not within the kernel.

Your explicit cxl_dpa_to_hpa() name is good.

--Alison



> 
> 
> > ---
> >  drivers/cxl/core/core.h   | 4 ++--
> >  drivers/cxl/core/mbox.c   | 2 +-
> >  drivers/cxl/core/region.c | 2 +-
> >  drivers/cxl/core/trace.h  | 2 +-
> >  4 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> > index 625394486459..8ceebd3b51d2 100644
> > --- a/drivers/cxl/core/core.h
> > +++ b/drivers/cxl/core/core.h
> > @@ -28,12 +28,12 @@ int cxl_region_init(void);
> >  void cxl_region_exit(void);
> >  int cxl_get_poison_by_endpoint(struct cxl_port *port);
> >  struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa);
> > -u64 cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> > +u64 cxl_translate(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> >  		  u64 dpa);
> >  
> >  #else
> >  static inline u64
> > -cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd, u64 dpa)
> > +cxl_translate(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd, u64 dpa)
> >  {
> >  	return ULLONG_MAX;
> >  }
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index 2626f3fff201..edc54a1ca298 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -878,7 +878,7 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> >  		dpa = le64_to_cpu(evt->common.phys_addr) & CXL_DPA_MASK;
> >  		cxlr = cxl_dpa_to_region(cxlmd, dpa);
> >  		if (cxlr)
> > -			hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
> > +			hpa = cxl_translate(cxlr, cxlmd, dpa);
> >  
> >  		if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
> >  			trace_cxl_general_media(cxlmd, type, cxlr, hpa,
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 00a9f0eef8dd..245edf748906 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -2797,7 +2797,7 @@ static u64 cxl_dpa_to_hpa(u64 dpa,  struct cxl_region *cxlr,
> >  	return hpa;
> >  }
> >  
> > -u64 cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> > +u64 cxl_translate(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> >  		  u64 dpa)
> >  {
> >  	struct cxl_region_params *p = &cxlr->params;
> > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> > index 07a0394b1d99..6925a6a31f01 100644
> > --- a/drivers/cxl/core/trace.h
> > +++ b/drivers/cxl/core/trace.h
> > @@ -704,7 +704,7 @@ TRACE_EVENT(cxl_poison,
> >  		if (cxlr) {
> >  			__assign_str(region, dev_name(&cxlr->dev));
> >  			memcpy(__entry->uuid, &cxlr->params.uuid, 16);
> > -			__entry->hpa = cxl_trace_hpa(cxlr, cxlmd,
> > +			__entry->hpa = cxl_translate(cxlr, cxlmd,
> >  						     __entry->dpa);
> >  		} else {
> >  			__assign_str(region, "");
>

Jonathan Cameron June 7, 2024, 5:55 p.m. UTC | #4

On Fri, 7 Jun 2024 10:45:42 -0700
Alison Schofield <[e-mail]> wrote:

> On Fri, Jun 07, 2024 at 03:40:25PM +0100, Jonathan Cameron wrote:
> > On Wed,  8 May 2024 11:47:50 -0700
> > [e-mail] wrote:
> >   
> > > From: Alison Schofield <[e-mail]>
> > > 
> > > Although cxl_trace_hpa() is used to populate TRACE EVENTs with HPA
> > > addresses, the work it performs is a translation (dpa->hpa), not a
> > > trace. Rename it. Since this is the only translate work in CXL,
> > > drop the _hpa suffix in the rename.
> > > 
> > > Suggested-by: Dan Williams <[e-mail]>
> > > Signed-off-by: Alison Schofield <[e-mail]>  
> > 
> > Maybe makes some sense to keep the direction in the name?
> > 
> > I can see for things like PPR we may need to go HPA to DPA as
> > it'll be based on error counts that may be reported in HPA.
> > 
> > So Friday bikeshedding time. 
> > cxl_dpa_to_hpa()?  
> 
> Thanks for the review Jonathan!
> 
> Let's sync on the possible dpa->hpa->dpa use cases and then I'll
> weigh in on your bikeshedding.
> 
> Agree we need HPA->DPA support for userspace. User needs ability
> to find which device maps an HPA.
> 
> wrt PPR, do you think that is an in kernel need?
> If the PPR maintenance was done in response to a CXL event, the DPA
> is already in the event trace along with the HPA so I wouldn't expect
> that a lookup is needed, at least not within the kernel.

Might be a synchronous poison event that ends up
triggering thing. And for that we'll be lucky to get any useful
info beyond an HPA.

I've only started thinking about this in the last few days, so
not 100% sure yet.

Given PPR can I think interrupt data flow we need to be very
careful to mediate any attempt so to enable it at all will
need kernel support.  Whether the interface takes a DPA or
an HPA is an open question.  We'll see!

Jonathan

> 
> Your explicit cxl_dpa_to_hpa() name is good.
> 
> --Alison
> 
> 
> 
> > 
> >   
> > > ---
> > >  drivers/cxl/core/core.h   | 4 ++--
> > >  drivers/cxl/core/mbox.c   | 2 +-
> > >  drivers/cxl/core/region.c | 2 +-
> > >  drivers/cxl/core/trace.h  | 2 +-
> > >  4 files changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> > > index 625394486459..8ceebd3b51d2 100644
> > > --- a/drivers/cxl/core/core.h
> > > +++ b/drivers/cxl/core/core.h
> > > @@ -28,12 +28,12 @@ int cxl_region_init(void);
> > >  void cxl_region_exit(void);
> > >  int cxl_get_poison_by_endpoint(struct cxl_port *port);
> > >  struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa);
> > > -u64 cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> > > +u64 cxl_translate(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> > >  		  u64 dpa);
> > >  
> > >  #else
> > >  static inline u64
> > > -cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd, u64 dpa)
> > > +cxl_translate(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd, u64 dpa)
> > >  {
> > >  	return ULLONG_MAX;
> > >  }
> > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > > index 2626f3fff201..edc54a1ca298 100644
> > > --- a/drivers/cxl/core/mbox.c
> > > +++ b/drivers/cxl/core/mbox.c
> > > @@ -878,7 +878,7 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> > >  		dpa = le64_to_cpu(evt->common.phys_addr) & CXL_DPA_MASK;
> > >  		cxlr = cxl_dpa_to_region(cxlmd, dpa);
> > >  		if (cxlr)
> > > -			hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
> > > +			hpa = cxl_translate(cxlr, cxlmd, dpa);
> > >  
> > >  		if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
> > >  			trace_cxl_general_media(cxlmd, type, cxlr, hpa,
> > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > > index 00a9f0eef8dd..245edf748906 100644
> > > --- a/drivers/cxl/core/region.c
> > > +++ b/drivers/cxl/core/region.c
> > > @@ -2797,7 +2797,7 @@ static u64 cxl_dpa_to_hpa(u64 dpa,  struct cxl_region *cxlr,
> > >  	return hpa;
> > >  }
> > >  
> > > -u64 cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> > > +u64 cxl_translate(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> > >  		  u64 dpa)
> > >  {
> > >  	struct cxl_region_params *p = &cxlr->params;
> > > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> > > index 07a0394b1d99..6925a6a31f01 100644
> > > --- a/drivers/cxl/core/trace.h
> > > +++ b/drivers/cxl/core/trace.h
> > > @@ -704,7 +704,7 @@ TRACE_EVENT(cxl_poison,
> > >  		if (cxlr) {
> > >  			__assign_str(region, dev_name(&cxlr->dev));
> > >  			memcpy(__entry->uuid, &cxlr->params.uuid, 16);
> > > -			__entry->hpa = cxl_trace_hpa(cxlr, cxlmd,
> > > +			__entry->hpa = cxl_translate(cxlr, cxlmd,
> > >  						     __entry->dpa);
> > >  		} else {
> > >  			__assign_str(region, "");  
> >   
>

Patch

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 625394486459..8ceebd3b51d2 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -28,12 +28,12 @@  int cxl_region_init(void);
 void cxl_region_exit(void);
 int cxl_get_poison_by_endpoint(struct cxl_port *port);
 struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa);
-u64 cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
+u64 cxl_translate(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
 		  u64 dpa);
 
 #else
 static inline u64
-cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd, u64 dpa)
+cxl_translate(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd, u64 dpa)
 {
 	return ULLONG_MAX;
 }
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 2626f3fff201..edc54a1ca298 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -878,7 +878,7 @@  void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
 		dpa = le64_to_cpu(evt->common.phys_addr) & CXL_DPA_MASK;
 		cxlr = cxl_dpa_to_region(cxlmd, dpa);
 		if (cxlr)
-			hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
+			hpa = cxl_translate(cxlr, cxlmd, dpa);
 
 		if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
 			trace_cxl_general_media(cxlmd, type, cxlr, hpa,
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 00a9f0eef8dd..245edf748906 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2797,7 +2797,7 @@  static u64 cxl_dpa_to_hpa(u64 dpa,  struct cxl_region *cxlr,
 	return hpa;
 }
 
-u64 cxl_trace_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
+u64 cxl_translate(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
 		  u64 dpa)
 {
 	struct cxl_region_params *p = &cxlr->params;
diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
index 07a0394b1d99..6925a6a31f01 100644
--- a/drivers/cxl/core/trace.h
+++ b/drivers/cxl/core/trace.h
@@ -704,7 +704,7 @@  TRACE_EVENT(cxl_poison,
 		if (cxlr) {
 			__assign_str(region, dev_name(&cxlr->dev));
 			memcpy(__entry->uuid, &cxlr->params.uuid, 16);
-			__entry->hpa = cxl_trace_hpa(cxlr, cxlmd,
+			__entry->hpa = cxl_translate(cxlr, cxlmd,
 						     __entry->dpa);
 		} else {
 			__assign_str(region, "");