Re: [PATCH 4/5] cxl: Set type of region to that of the first endpoint

[Alejandro Lucero Palau]

On 10/17/24 23:33, Dan Williams wrote:
> Huang Ying wrote:
> > Current kernel hard-codes the type of region to type 3 expander now,
> > because this is the only supported device type.  As a preparation to
> > support type 2 accelerators, the patch sets the type of region to that
> > of the first endpoint.  Then, the patch checks whether the type of
> > region is same as the type of other endpoints of the region.  Because
> > what we really need is to make sure the type of all endpoints of a
> > region is same.  And, the patch lets expander/accelerator device
> > drivers specify the target type of endpoint devices via struct
> > cxl_dev_state.
> >
> > Signed-off-by: "Huang, Ying" <ying.huang@xxxxxxxxx>
> > Reviewed-by: Gregory Price <gourry@xxxxxxxxxx>
> > Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> > Cc: Davidlohr Bueso <dave@xxxxxxxxxxxx>
> > Cc: Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>
> > Cc: Dave Jiang <dave.jiang@xxxxxxxxx>
> > Cc: Alison Schofield <alison.schofield@xxxxxxxxx>
> > Cc: Vishal Verma <vishal.l.verma@xxxxxxxxx>
> > Cc: Ira Weiny <ira.weiny@xxxxxxxxx>
> > Cc: Alejandro Lucero <alucerop@xxxxxxx>
> > Cc: Ben Cheatham <benjamin.cheatham@xxxxxxx>
> > ---
> >   drivers/cxl/acpi.c        |  1 -
> >   drivers/cxl/core/hdm.c    | 28 +++++++++++++---------------
> >   drivers/cxl/core/port.c   |  2 ++
> >   drivers/cxl/core/region.c | 13 +++++++------
> >   drivers/cxl/cxl.h         |  1 +
> >   5 files changed, 23 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index 21486e471305..29c2a44b122c 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -382,7 +382,6 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
> >   
> >   	cxld = &cxlrd->cxlsd.cxld;
> >   	cxld->flags = cfmws_to_decoder_flags(cfmws->restrictions);
> > -	cxld->target_type = CXL_DECODER_EXPANDER;
> >   	cxld->hpa_range = (struct range) {
> >   		.start = cfmws->base_hpa,
> >   		.end = cfmws->base_hpa + cfmws->window_size - 1,
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 478fb6691759..c9accf8be71f 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -841,18 +841,25 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >   		.end = base + size - 1,
> >   	};
> >   
> > +	if (cxled) {
> > +		struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > +		struct cxl_dev_state *cxlds = cxlmd->cxlds;
> > +
> > +		if (cxlds->type == CXL_DEVTYPE_CLASSMEM)
> > +			cxld->target_type = CXL_DECODER_EXPANDER;
> > +		else
> > +			cxld->target_type = CXL_DECODER_ACCEL;
> This looks broken there is no way to know the target type of the decoder
> from the cxl_dev_state. An "accelerator" can have HDM and an "expander"
> can have HDM-DB.
>
> > +	}
> > +
> >   	/* decoders are enabled if committed */
> >   	if (committed) {
> >   		cxld->flags |= CXL_DECODER_F_ENABLE;
> >   		if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
> >   			cxld->flags |= CXL_DECODER_F_LOCK;
> > -		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl)) {
> > -			cxld->target_type = CXL_DECODER_EXPANDER;
> > +		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl))
> >   			cxld->coherence = CXL_DECODER_HOSTONLYCOH;
> > -		} else {
> > -			cxld->target_type = CXL_DECODER_ACCEL;
> > +		else
> >   			cxld->coherence = CXL_DECODER_DEVCOH;
> > -		}
> >   
> >   		guard(rwsem_write)(&cxl_region_rwsem);
> >   		if (cxld->id != cxl_num_decoders_committed(port)) {
> > @@ -874,21 +881,12 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >   			struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> >   			struct cxl_dev_state *cxlds = cxlmd->cxlds;
> >   
> > -			/*
> > -			 * Default by devtype until a device arrives that needs
> > -			 * more precision.
> > -			 */
> > -			if (cxlds->type == CXL_DEVTYPE_CLASSMEM)
> > -				cxld->target_type = CXL_DECODER_EXPANDER;
> > -			else
> > -				cxld->target_type = CXL_DECODER_ACCEL;
> >   			if (cxlds->coherence == CXL_DEVCOH_HOSTONLY)
> >   				cxld->coherence = CXL_DECODER_HOSTONLYCOH;
> >   			else
> >   				cxld->coherence = CXL_DECODER_DEVCOH;
> >   		} else {
> > -			/* To be overridden by region type/coherence at commit time */
> > -			cxld->target_type = CXL_DECODER_EXPANDER;
> > +			/* To be overridden by region coherence at commit time */
> >   			cxld->coherence = CXL_DECODER_HOSTONLYCOH;
> >   		}
> >   
> > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > index 9ebbffcea26a..d1bc6aed6509 100644
> > --- a/drivers/cxl/core/port.c
> > +++ b/drivers/cxl/core/port.c
> > @@ -139,6 +139,8 @@ static ssize_t target_type_show(struct device *dev,
> >   		return sysfs_emit(buf, "accelerator\n");
> >   	case CXL_DECODER_EXPANDER:
> >   		return sysfs_emit(buf, "expander\n");
> > +	default:
> > +		break;
> >   	}
> >   	return -ENXIO;
> >   }
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 21b877d8582f..d709738ada61 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1926,7 +1926,10 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> >   		return -ENXIO;
> >   	}
> >   
> > -	if (cxled->cxld.target_type != cxlr->type) {
> > +	/* Set the type of region to that of the first endpoint */
> > +	if (cxlr->type == CXL_DECODER_INVALID) {
> > +		cxlr->type = cxled->cxld.target_type;
> > +	} else if (cxled->cxld.target_type != cxlr->type) {
> No, the type of the region is determined by the caller and should be
> gated by the region capability. For type-2 region creation I doubt
> userspace is going to be creating those vs the accelerator so this all
> seems backwards to me.


FWIW, path 19 of last type support patchset does add the type to be set 
at region creation time by the caller:

https://lore.kernel.org/linux-cxl/4ce8cc04-71fd-424a-9831-86f89fcd7d2f@xxxxxxx/T/#m420ea86f7b9193519e3226c377612ad3ea546633