Re: [PATCH v2 06/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Dan Williams]

Dan Williams wrote:
> Robert Richter wrote:
> > On 20.10.22 22:17:07, Dan Williams wrote:
> > > Robert Richter wrote:
> > > > A downstream port must be connected to a component register block.
> > > > For restricted hosts the base address is determined from the RCRB. The
> > > > RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
> > > > get the RCRB and add code to extract the component register block from
> > > > it.
> > > > 
> > > > RCRB's BAR[0..1] point to the component block containing CXL subsystem
> > > > component registers. MEMBAR extraction follows the PCI base spec here,
> > > > esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1).
> > > > 
> > > > Note: Right now the component register block is used for HDM decoder
> > > > capability only which is optional for RCDs. If unsupported by the RCD,
> > > > the HDM init will fail. It is future work to bypass it in this case.
> > > > 
> > > > Signed-off-by: Terry Bowman <terry.bowman@xxxxxxx>
> > > > Signed-off-by: Robert Richter <rrichter@xxxxxxx>
> > > > ---
> > > >  drivers/cxl/acpi.c | 79 ++++++++++++++++++++++++++++++++++++++++------
> > > >  1 file changed, 69 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > > > index fb9f72813067..a92d5d7b7a92 100644
> > > > --- a/drivers/cxl/acpi.c
> > > > +++ b/drivers/cxl/acpi.c
> > > > @@ -9,6 +9,8 @@
> > > >  #include "cxlpci.h"
> > > >  #include "cxl.h"
> > > >  
> > > > +#define CXL_RCRB_SIZE	SZ_8K
> > > > +
> > > >  static unsigned long cfmws_to_decoder_flags(int restrictions)
> > > >  {
> > > >  	unsigned long flags = CXL_DECODER_F_ENABLE;
> > > > @@ -229,27 +231,82 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> > > >  struct cxl_chbs_context {
> > > >  	struct device *dev;
> > > >  	unsigned long long uid;
> > > > -	resource_size_t chbcr;
> > > > +	struct acpi_cedt_chbs chbs;
> > > >  };
> > > >  
> > > > -static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
> > > > -			 const unsigned long end)
> > > > +static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
> > > > +			const unsigned long end)
> > > >  {
> > > >  	struct cxl_chbs_context *ctx = arg;
> > > >  	struct acpi_cedt_chbs *chbs;
> > > >  
> > > > -	if (ctx->chbcr)
> > > > +	if (ctx->chbs.base)
> > > >  		return 0;
> > > >  
> > > >  	chbs = (struct acpi_cedt_chbs *) header;
> > > >  
> > > >  	if (ctx->uid != chbs->uid)
> > > >  		return 0;
> > > > -	ctx->chbcr = chbs->base;
> > > > +	ctx->chbs = *chbs;
> > > >  
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static resource_size_t cxl_get_chbcr(struct cxl_chbs_context *ctx)
> > > > +{
> > > 
> > > The core logic of this looks good, but this wants to be shared with the
> > > upstream port component register discovery.
> > > 
> > > Full disclosure I am reconciling these patches with an attempt that Dave
> > > Jiang made at this topic. Since your series hit the list first I will
> > > let it take the lead, but then fill it in with comments and learnings
> > > from Dave's effort.
> > > 
> > > So in this case Dave moved this into the drivers/cxl/core/regs.c with a
> > > function signature like:
> > > 
> > > enum cxl_rcrb {
> > >        CXL_RCRB_DOWNSTREAM,
> > >        CXL_RCRB_UPSTREAM,
> > > };
> > > 
> > > resource_size_t cxl_rcrb_to_component(struct device *dev,
> > >                                       resource_size_t rcrb_base, int len,
> > >                                       enum cxl_rcrb which);
> > > 
> > > ...where @which alternates when called by cxl_acpi for the downstream
> > > case, or cxl_mem for the upstream case.
> > 
> > Ok, I see where to go here. Could you point me to Dave's postings you
> > are referring to? I checked linux-cxl and could not find anything
> > related to RCRB or that changes regs.c.
> 
> He was in the middle of tidying them when you posted your series, but I
> think it would not hurt to push them to a git tree so you can grab the
> bits and pieces you want.
> 
> Dave?

Looks like the list delivery is backed up, so I added Dave to the Cc:.

He pushed:

https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-rch

...which was his original attempt and:

https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-rch-robert

...which was an attempt to rebase on top of your bits.

The common RCRB mapping function is here:

https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/commit/?h=cxl-rch-robert&id=5be44cad37972517dae6a79001080ccfbdb67c49

I think the path forward is to build on that common RCRB code, fix
cxl_acpi to register the pci host bridge device instead of the APCI
device as the dport device, and then rely on a flag to skip over
devm_enumerate_cxl_ports() in favor of just calling cxl_mem_find_port()
directly in the RCIEP / RCH case. Then we can figure out what to do
about RCDs that choose not to implement the HDM decoder capability which
was forbidden in CXL 2.0, but now allowed in CXL 3.0.