netdev - Re: [PATCH v11 01/23] cxl: add type2 device basic support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bf26b669-860c-493e-8126-733615f47b13@amd.com>
Date: Tue, 11 Mar 2025 15:05:04 -0500
From: Ben Cheatham <benjamin.cheatham@....com>
To: <alejandro.lucero-palau@....com>
CC: <linux-cxl@...r.kernel.org>, <netdev@...r.kernel.org>,
	<dan.j.williams@...el.com>, <edward.cree@....com>, <davem@...emloft.net>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <edumazet@...gle.com>,
	<dave.jiang@...el.com>, "Cheatham, Benjamin" <benjamin.cheatham@....com>
Subject: Re: [PATCH v11 01/23] cxl: add type2 device basic support

On 3/10/25 4:03 PM, alejandro.lucero-palau@....com wrote:
> From: Alejandro Lucero <alucerop@....com>
> 
> Differentiate CXL memory expanders (type 3) from CXL device accelerators
> (type 2) with a new function for initializing cxl_dev_state and a macro
> for helping accel drivers to embed cxl_dev_state inside a private
> struct.
> 
> Move structs to include/cxl as the size of the accel driver private
> struct embedding cxl_dev_state needs to know the size of this struct.
> 
> Use same new initialization with the type3 pci driver.
> 
> Signed-off-by: Alejandro Lucero <alucerop@....com>
> ---
>  drivers/cxl/core/mbox.c   |  12 +--
>  drivers/cxl/core/memdev.c |  32 ++++++
>  drivers/cxl/core/pci.c    |   1 +
>  drivers/cxl/core/regs.c   |   1 +
>  drivers/cxl/cxl.h         |  97 +-----------------
>  drivers/cxl/cxlmem.h      |  88 ++--------------
>  drivers/cxl/cxlpci.h      |  21 ----
>  drivers/cxl/pci.c         |  17 ++--
>  include/cxl/cxl.h         | 206 ++++++++++++++++++++++++++++++++++++++
>  include/cxl/pci.h         |  23 +++++
>  10 files changed, 285 insertions(+), 213 deletions(-)
>  create mode 100644 include/cxl/cxl.h
>  create mode 100644 include/cxl/pci.h
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index d72764056ce6..20df6f78f148 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1484,23 +1484,21 @@ int cxl_mailbox_init(struct cxl_mailbox *cxl_mbox, struct device *host)
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_mailbox_init, "CXL");
>  
> -struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev)
> +struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev, u64 serial,
> +						 u16 dvsec)
>  {
>  	struct cxl_memdev_state *mds;
>  	int rc;
>  
> -	mds = devm_kzalloc(dev, sizeof(*mds), GFP_KERNEL);
> +	mds = (struct cxl_memdev_state *)
> +		_cxl_dev_state_create(dev, CXL_DEVTYPE_CLASSMEM, serial, dvsec,
> +				      sizeof(struct cxl_memdev_state), true);

I would use sizeof(*mds) instead of sizeof(struct cxl_memdev_state) above.

What's the reason to not use the cxl_dev_state_create() macro here instead? Based on the commit
message I'm assuming it's because it's meant for accelerator drivers and this is a type 3 driver,
but I'm going to suggest using it here anyway (and maybe amending the commit message).

>  	if (!mds) {
>  		dev_err(dev, "No memory available\n");
>  		return ERR_PTR(-ENOMEM);
>  	}
>  
>  	mutex_init(&mds->event.log_lock);
> -	mds->cxlds.dev = dev;
> -	mds->cxlds.reg_map.host = dev;
> -	mds->cxlds.cxl_mbox.host = dev;
> -	mds->cxlds.reg_map.resource = CXL_RESOURCE_NONE;
> -	mds->cxlds.type = CXL_DEVTYPE_CLASSMEM;
>  

[snip]

> +/**
> + * struct cxl_dev_state - The driver device state
> + *
> + * cxl_dev_state represents the CXL driver/device state.  It provides an
> + * interface to mailbox commands as well as some cached data about the device.
> + * Currently only memory devices are represented.
> + *
> + * @dev: The device associated with this CXL state
> + * @cxlmd: The device representing the CXL.mem capabilities of @dev
> + * @reg_map: component and ras register mapping parameters
> + * @regs: Parsed register blocks
> + * @cxl_dvsec: Offset to the PCIe device DVSEC
> + * @rcd: operating in RCD mode (CXL 3.0 9.11.8 CXL Devices Attached to an RCH)
> + * @media_ready: Indicate whether the device media is usable
> + * @dpa_res: Overall DPA resource tree for the device
> + * @part: DPA partition array
> + * @nr_partitions: Number of DPA partitions
> + * @serial: PCIe Device Serial Number
> + * @type: Generic Memory Class device or Vendor Specific Memory device
> + * @cxl_mbox: CXL mailbox context
> + * @cxlfs: CXL features context
> + */
> +struct cxl_dev_state {
> +	struct device *dev;
> +	struct cxl_memdev *cxlmd;
> +	struct cxl_register_map reg_map;
> +	struct cxl_regs regs;
> +	int cxl_dvsec;
> +	bool rcd;
> +	bool media_ready;
> +	struct resource dpa_res;
> +	struct cxl_dpa_partition part[CXL_NR_PARTITIONS_MAX];
> +	unsigned int nr_partitions;
> +	u64 serial;
> +	enum cxl_devtype type;
> +	struct cxl_mailbox cxl_mbox;
> +#ifdef CONFIG_CXL_FEATURES
> +	struct cxl_features_state *cxlfs;
> +#endif
> +};

What happened to the comments for private/public fields for this struct that Dan suggested in
your RFC? If you don't think they're needed that's fine, I'm just curious!