diff mbox series

[3/3] cxl: Avoid to create dax regions for type2 accelerators

Message ID 20240729084611.502889-4-ying.huang@intel.com
State Superseded
Headers show
Series cxl: Preparation of type2 accelerators support | expand

Commit Message

Huang, Ying July 29, 2024, 8:46 a.m. UTC 
The memory range of a type2 accelerator should be managed by the type2
accelerator specific driver instead of the common dax region drivers,
as discussed in [1].

[1] https://lore.kernel.org/linux-cxl/66469ff1b8fbc_2c2629427@dwillia2-xfh.jf.intel.com.notmuch/

So, in this patch, we skip dax regions creation for type2 accelerator
device memory regions.

Based on: https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Co-developed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Alejandro Lucero <alucerop@amd.com>
---
 drivers/cxl/core/region.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Jonathan Cameron Aug. 4, 2024, 4:38 p.m. UTC | #1 
On Mon, 29 Jul 2024 16:46:11 +0800
Huang Ying <ying.huang@intel.com> wrote:

> The memory range of a type2 accelerator should be managed by the type2
> accelerator specific driver instead of the common dax region drivers,
> as discussed in [1].
> 
> [1] https://lore.kernel.org/linux-cxl/66469ff1b8fbc_2c2629427@dwillia2-xfh.jf.intel.com.notmuch/
> 
> So, in this patch, we skip dax regions creation for type2 accelerator
> device memory regions.
> 
> Based on: https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/
> 
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/cxl/core/region.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 9a483c8a32fd..b37e12bb4a35 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3435,6 +3435,14 @@ static int cxl_region_probe(struct device *dev)
>  					p->res->start, p->res->end, cxlr,
>  					is_system_ram) > 0)
>  			return 0;
> +		/*
> +		 * HDM-D[B] (device-memory) regions have accelerator
> +		 * specific usage, skip device-dax registration.
> +		 */
> +		if (cxlr->type == CXL_DECODER_DEVMEM)
> +			return 0;

As in previous need to be careful as that may not mean it's
an accelerator.

However, we do need to deal with BI setup for HDM-DB type 3 devices
etc and to check the HDM Decoder capability registers to make sure
Supported Coherence model is appropriate. (e.g. 11 for host only or
device coherency - HDM-H/HDM-DB)

> +
> +		/* HDM-H routes to device-dax */
>  		return devm_cxl_add_dax_region(cxlr);
>  	default:
>  		dev_dbg(&cxlr->dev, "unsupported region mode: %d\n",
Huang, Ying Aug. 6, 2024, 5:52 a.m. UTC | #2 
Jonathan Cameron <Jonathan.Cameron@Huawei.com> writes:

> On Mon, 29 Jul 2024 16:46:11 +0800
> Huang Ying <ying.huang@intel.com> wrote:
>
>> The memory range of a type2 accelerator should be managed by the type2
>> accelerator specific driver instead of the common dax region drivers,
>> as discussed in [1].
>> 
>> [1] https://lore.kernel.org/linux-cxl/66469ff1b8fbc_2c2629427@dwillia2-xfh.jf.intel.com.notmuch/
>> 
>> So, in this patch, we skip dax regions creation for type2 accelerator
>> device memory regions.
>> 
>> Based on: https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/
>> 
>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> Cc: Davidlohr Bueso <dave@stgolabs.net>
>> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
>> Cc: Dave Jiang <dave.jiang@intel.com>
>> Cc: Alison Schofield <alison.schofield@intel.com>
>> Cc: Vishal Verma <vishal.l.verma@intel.com>
>> Cc: Ira Weiny <ira.weiny@intel.com>
>> Cc: Alejandro Lucero <alucerop@amd.com>
>> ---
>>  drivers/cxl/core/region.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>> 
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index 9a483c8a32fd..b37e12bb4a35 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -3435,6 +3435,14 @@ static int cxl_region_probe(struct device *dev)
>>  					p->res->start, p->res->end, cxlr,
>>  					is_system_ram) > 0)
>>  			return 0;
>> +		/*
>> +		 * HDM-D[B] (device-memory) regions have accelerator
>> +		 * specific usage, skip device-dax registration.
>> +		 */
>> +		if (cxlr->type == CXL_DECODER_DEVMEM)
>> +			return 0;
>
> As in previous need to be careful as that may not mean it's
> an accelerator.

Yes.  We need some other way to identify type2 devices.

> However, we do need to deal with BI setup for HDM-DB type 3 devices
> etc and to check the HDM Decoder capability registers to make sure
> Supported Coherence model is appropriate. (e.g. 11 for host only or
> device coherency - HDM-H/HDM-DB)

Yes.  We need to check BI configuration too.

>> +
>> +		/* HDM-H routes to device-dax */
>>  		return devm_cxl_add_dax_region(cxlr);
>>  	default:
>>  		dev_dbg(&cxlr->dev, "unsupported region mode: %d\n",

--
Best Regards,
Huang, Ying
Alejandro Lucero Palau Aug. 12, 2024, 11:50 a.m. UTC | #3 
I do not understand why you took this change from my patchset.

Maybe the other two patches have a pass, but this should not be removed 
from my patchset, not at least after discussing it publicly.

The reason you mentioned for doing it was for making things easier for 
the other changes in my larger patchset, but again, you should have 
discussed this first publicly, and second, I do not remember other large 
patchsets, far larger than mine, being partially picked up  by someone 
else but the patchset submitter, and sending those picked changes in 
another patchset.


On 8/6/24 06:52, Huang, Ying wrote:
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> writes:
>
>> On Mon, 29 Jul 2024 16:46:11 +0800
>> Huang Ying <ying.huang@intel.com> wrote:
>>
>>> The memory range of a type2 accelerator should be managed by the type2
>>> accelerator specific driver instead of the common dax region drivers,
>>> as discussed in [1].
>>>
>>> [1] https://lore.kernel.org/linux-cxl/66469ff1b8fbc_2c2629427@dwillia2-xfh.jf.intel.com.notmuch/
>>>
>>> So, in this patch, we skip dax regions creation for type2 accelerator
>>> device memory regions.
>>>
>>> Based on: https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/
>>>
>>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>>> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
>>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>>> Cc: Davidlohr Bueso <dave@stgolabs.net>
>>> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
>>> Cc: Dave Jiang <dave.jiang@intel.com>
>>> Cc: Alison Schofield <alison.schofield@intel.com>
>>> Cc: Vishal Verma <vishal.l.verma@intel.com>
>>> Cc: Ira Weiny <ira.weiny@intel.com>
>>> Cc: Alejandro Lucero <alucerop@amd.com>
>>> ---
>>>   drivers/cxl/core/region.c | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>> index 9a483c8a32fd..b37e12bb4a35 100644
>>> --- a/drivers/cxl/core/region.c
>>> +++ b/drivers/cxl/core/region.c
>>> @@ -3435,6 +3435,14 @@ static int cxl_region_probe(struct device *dev)
>>>   					p->res->start, p->res->end, cxlr,
>>>   					is_system_ram) > 0)
>>>   			return 0;
>>> +		/*
>>> +		 * HDM-D[B] (device-memory) regions have accelerator
>>> +		 * specific usage, skip device-dax registration.
>>> +		 */
>>> +		if (cxlr->type == CXL_DECODER_DEVMEM)
>>> +			return 0;
>> As in previous need to be careful as that may not mean it's
>> an accelerator.
> Yes.  We need some other way to identify type2 devices.
>
>> However, we do need to deal with BI setup for HDM-DB type 3 devices
>> etc and to check the HDM Decoder capability registers to make sure
>> Supported Coherence model is appropriate. (e.g. 11 for host only or
>> device coherency - HDM-H/HDM-DB)
> Yes.  We need to check BI configuration too.
>
>>> +
>>> +		/* HDM-H routes to device-dax */
>>>   		return devm_cxl_add_dax_region(cxlr);
>>>   	default:
>>>   		dev_dbg(&cxlr->dev, "unsupported region mode: %d\n",
> --
> Best Regards,
> Huang, Ying
Alejandro Lucero Palau Aug. 12, 2024, 11:54 a.m. UTC | #4 
On 8/6/24 06:52, Huang, Ying wrote:
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> writes:
>
>> On Mon, 29 Jul 2024 16:46:11 +0800
>> Huang Ying <ying.huang@intel.com> wrote:
>>
>>> The memory range of a type2 accelerator should be managed by the type2
>>> accelerator specific driver instead of the common dax region drivers,
>>> as discussed in [1].
>>>
>>> [1] https://lore.kernel.org/linux-cxl/66469ff1b8fbc_2c2629427@dwillia2-xfh.jf.intel.com.notmuch/
>>>
>>> So, in this patch, we skip dax regions creation for type2 accelerator
>>> device memory regions.
>>>
>>> Based on: https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/
>>>
>>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>>> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
>>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>>> Cc: Davidlohr Bueso <dave@stgolabs.net>
>>> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
>>> Cc: Dave Jiang <dave.jiang@intel.com>
>>> Cc: Alison Schofield <alison.schofield@intel.com>
>>> Cc: Vishal Verma <vishal.l.verma@intel.com>
>>> Cc: Ira Weiny <ira.weiny@intel.com>
>>> Cc: Alejandro Lucero <alucerop@amd.com>
>>> ---
>>>   drivers/cxl/core/region.c | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>> index 9a483c8a32fd..b37e12bb4a35 100644
>>> --- a/drivers/cxl/core/region.c
>>> +++ b/drivers/cxl/core/region.c
>>> @@ -3435,6 +3435,14 @@ static int cxl_region_probe(struct device *dev)
>>>   					p->res->start, p->res->end, cxlr,
>>>   					is_system_ram) > 0)
>>>   			return 0;
>>> +		/*
>>> +		 * HDM-D[B] (device-memory) regions have accelerator
>>> +		 * specific usage, skip device-dax registration.
>>> +		 */
>>> +		if (cxlr->type == CXL_DECODER_DEVMEM)
>>> +			return 0;
>> As in previous need to be careful as that may not mean it's
>> an accelerator.
> Yes.  We need some other way to identify type2 devices.


Maybe the easier option is the accel driver specifying if DAX should be 
used.

We are adding mailbox and hdm as optional for accel/type2, with the 
driver specifying what is supported. Another optional param could be 
this DAX usage.


>> However, we do need to deal with BI setup for HDM-DB type 3 devices
>> etc and to check the HDM Decoder capability registers to make sure
>> Supported Coherence model is appropriate. (e.g. 11 for host only or
>> device coherency - HDM-H/HDM-DB)
> Yes.  We need to check BI configuration too.
>
>>> +
>>> +		/* HDM-H routes to device-dax */
>>>   		return devm_cxl_add_dax_region(cxlr);
>>>   	default:
>>>   		dev_dbg(&cxlr->dev, "unsupported region mode: %d\n",
> --
> Best Regards,
> Huang, Ying
Huang, Ying Aug. 15, 2024, 1:10 a.m. UTC | #5 
Alejandro Lucero Palau <alucerop@amd.com> writes:

> On 8/6/24 06:52, Huang, Ying wrote:
>> Jonathan Cameron <Jonathan.Cameron@Huawei.com> writes:
>>
>>> On Mon, 29 Jul 2024 16:46:11 +0800
>>> Huang Ying <ying.huang@intel.com> wrote:
>>>
>>>> The memory range of a type2 accelerator should be managed by the type2
>>>> accelerator specific driver instead of the common dax region drivers,
>>>> as discussed in [1].
>>>>
>>>> [1] https://lore.kernel.org/linux-cxl/66469ff1b8fbc_2c2629427@dwillia2-xfh.jf.intel.com.notmuch/
>>>>
>>>> So, in this patch, we skip dax regions creation for type2 accelerator
>>>> device memory regions.
>>>>
>>>> Based on: https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/
>>>>
>>>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>>>> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
>>>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>>>> Cc: Davidlohr Bueso <dave@stgolabs.net>
>>>> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
>>>> Cc: Dave Jiang <dave.jiang@intel.com>
>>>> Cc: Alison Schofield <alison.schofield@intel.com>
>>>> Cc: Vishal Verma <vishal.l.verma@intel.com>
>>>> Cc: Ira Weiny <ira.weiny@intel.com>
>>>> Cc: Alejandro Lucero <alucerop@amd.com>
>>>> ---
>>>>   drivers/cxl/core/region.c | 8 ++++++++
>>>>   1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>>> index 9a483c8a32fd..b37e12bb4a35 100644
>>>> --- a/drivers/cxl/core/region.c
>>>> +++ b/drivers/cxl/core/region.c
>>>> @@ -3435,6 +3435,14 @@ static int cxl_region_probe(struct device *dev)
>>>>   					p->res->start, p->res->end, cxlr,
>>>>   					is_system_ram) > 0)
>>>>   			return 0;
>>>> +		/*
>>>> +		 * HDM-D[B] (device-memory) regions have accelerator
>>>> +		 * specific usage, skip device-dax registration.
>>>> +		 */
>>>> +		if (cxlr->type == CXL_DECODER_DEVMEM)
>>>> +			return 0;
>>> As in previous need to be careful as that may not mean it's
>>> an accelerator.
>> Yes.  We need some other way to identify type2 devices.
>
>
> Maybe the easier option is the accel driver specifying if DAX should
> be used.
>
> We are adding mailbox and hdm as optional for accel/type2, with the
> driver specifying what is supported. Another optional param could be
> this DAX usage.

Another way is let accel/cxl_pci driver specify the device type (type2
vs. type3).  cxl_pci_probe()->cxl_memdev_state_create() will set
cxl_dev_state.type = CXL_DEVTYPE_CLASSMEM. And
accel_X_probe()->cxl_accel_state_create() can set cxl_dev_state.type =
CXL_DEVTYPE_DEVMEM.  This can be passed to cxl_region.  Then we can
create cxl_dax_region for type3 devices only by default.

>
>>> However, we do need to deal with BI setup for HDM-DB type 3 devices
>>> etc and to check the HDM Decoder capability registers to make sure
>>> Supported Coherence model is appropriate. (e.g. 11 for host only or
>>> device coherency - HDM-H/HDM-DB)
>> Yes.  We need to check BI configuration too.
>>
>>>> +
>>>> +		/* HDM-H routes to device-dax */
>>>>   		return devm_cxl_add_dax_region(cxlr);
>>>>   	default:
>>>>   		dev_dbg(&cxlr->dev, "unsupported region mode: %d\n",

--
Best Regards,
Huang, Ying
diff mbox series

Patch

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 9a483c8a32fd..b37e12bb4a35 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3435,6 +3435,14 @@  static int cxl_region_probe(struct device *dev)
 					p->res->start, p->res->end, cxlr,
 					is_system_ram) > 0)
 			return 0;
+		/*
+		 * HDM-D[B] (device-memory) regions have accelerator
+		 * specific usage, skip device-dax registration.
+		 */
+		if (cxlr->type == CXL_DECODER_DEVMEM)
+			return 0;
+
+		/* HDM-H routes to device-dax */
 		return devm_cxl_add_dax_region(cxlr);
 	default:
 		dev_dbg(&cxlr->dev, "unsupported region mode: %d\n",