netdev - Re: [PATCH v10 01/26] cxl: make memdev creation type agnostic

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: <67a50f7fbdfb_2d2c29447@dwillia2-xfh.jf.intel.com.notmuch>
Date: Thu, 6 Feb 2025 11:37:35 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: <alucerop@....com>, <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>
CC: Alejandro Lucero <alucerop@....com>
Subject: Re: [PATCH v10 01/26] cxl: make memdev creation type agnostic

alucerop@ wrote:
> From: Alejandro Lucero <alucerop@....com>
> 
> In preparation for Type2 support, change memdev creation making
> type based on argument.
> 
> Integrate initialization of dvsec and serial fields in the related
> cxl_dev_state within same function creating the memdev.
> 
> Move the code from mbox file to memdev file.
> 
> Add new header files with type2 required definitions for memdev
> state creation.
> 
> Signed-off-by: Alejandro Lucero <alucerop@....com>
> ---
>  drivers/cxl/core/mbox.c   | 20 --------------------
>  drivers/cxl/core/memdev.c | 23 +++++++++++++++++++++++
>  drivers/cxl/cxlmem.h      | 18 +++---------------
>  drivers/cxl/cxlpci.h      | 17 +----------------
>  drivers/cxl/pci.c         | 16 +++++++++-------
>  include/cxl/cxl.h         | 26 ++++++++++++++++++++++++++
>  include/cxl/pci.h         | 23 +++++++++++++++++++++++
>  7 files changed, 85 insertions(+), 58 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 4d22bb731177..96155b8af535 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1435,26 +1435,6 @@ 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 *mds;
> -
> -	mds = devm_kzalloc(dev, sizeof(*mds), GFP_KERNEL);
> -	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.reg_map.resource = CXL_RESOURCE_NONE;
> -	mds->cxlds.type = CXL_DEVTYPE_CLASSMEM;
> -
> -	return mds;
> -}
> -EXPORT_SYMBOL_NS_GPL(cxl_memdev_state_create, "CXL");
> -
>  void __init cxl_mbox_init(void)
>  {
>  	struct dentry *mbox_debugfs;
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 63c6c681125d..456d505f1bc8 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -632,6 +632,29 @@ static void detach_memdev(struct work_struct *work)
>  
>  static struct lock_class_key cxl_memdev_key;
>  
> +struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev, u64 serial,
> +						 u16 dvsec, enum cxl_devtype type)
> +{
> +	struct cxl_memdev_state *mds;
> +
> +	mds = devm_kzalloc(dev, sizeof(*mds), GFP_KERNEL);
> +	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.reg_map.resource = CXL_RESOURCE_NONE;
> +	mds->cxlds.cxl_dvsec = dvsec;
> +	mds->cxlds.serial = serial;
> +	mds->cxlds.type = type;
> +
> +	return mds;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_memdev_state_create, "CXL");

I was envisioning that accelerators only consider 'struct cxl_dev_state'
and that 'struct cxl_memdev_state' is exclusively for
CXL_DEVTYPE_CLASSMEM memory expander use case. Something roughly like
the below. Note, this borrows from the fwctl_alloc_device() example
which captures the spirit of registering a core object wrapped by an end
driver provided structure).

#define cxl_dev_state_create(parent, serial, dvsec, type, drv_struct, member)  \
        ({                                                                     \
                static_assert(__same_type(struct cxl_dev_state,                \
                                          ((drv_struct *)NULL)->member));      \
                static_assert(offsetof(drv_struct, member) == 0);              \
                (drv_struct *)_cxl_dev_state_create(parent, serial, dvsec,     \
                                                    type, sizeof(drv_struct)); \
        })

struct cxl_memdev_state *cxl_memdev_state_create(parent, serial, dvsec)
{
        struct cxl_memdev_state *mds = cxl_dev_state_create(
                parent, serial, dvsec, CXL_DEVTYPE_CLASSMEM,
                struct cxl_memdev_state, cxlds);

        if (IS_ERR(mds))
                return mds;
        
        mutex_init(&mds->event.log_lock);
        mds->cxlds.dev = dev;
        mds->cxlds.reg_map.host = dev;
        mds->cxlds.reg_map.resource = CXL_RESOURCE_NONE;
        mds->cxlds.cxl_dvsec = dvsec;
        mds->cxlds.serial = serial;
        mds->cxlds.type = type;

        return mds;
}

If an accelerator wants to share infrastructure that is currently housed
in 'struct cxl_memdev_state', then that functionality should first move
to 'struct cxl_dev_state'.