diff mbox series

[02/14] cxl/mem: Map memory device registers

Message ID 20210130002438.1872527-3-ben.widawsky@intel.com (mailing list archive)
State Not Applicable, archived
Headers show
Series CXL 2.0 Support | expand

Commit Message

Ben Widawsky Jan. 30, 2021, 12:24 a.m. UTC
All the necessary bits are initialized in order to find and map the
register space for CXL Memory Devices. This is accomplished by using the
Register Locator DVSEC (CXL 2.0 - 8.1.9.1) to determine which PCI BAR to
use, and how much of an offset from that BAR should be added.

If the memory device registers are found and mapped a new internal data
structure tracking device state is allocated.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/cxl.h | 17 ++++++++++
 drivers/cxl/mem.c | 83 +++++++++++++++++++++++++++++++++++++++++++++--
 drivers/cxl/pci.h | 14 ++++++++
 3 files changed, 112 insertions(+), 2 deletions(-)
 create mode 100644 drivers/cxl/cxl.h

Comments

David Rientjes Jan. 30, 2021, 11:51 p.m. UTC | #1
On Fri, 29 Jan 2021, Ben Widawsky wrote:

> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> new file mode 100644
> index 000000000000..d81d0ba4617c
> --- /dev/null
> +++ b/drivers/cxl/cxl.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright(c) 2020 Intel Corporation. */
> +
> +#ifndef __CXL_H__
> +#define __CXL_H__
> +
> +/**
> + * struct cxl_mem - A CXL memory device
> + * @pdev: The PCI device associated with this CXL device.
> + * @regs: IO mappings to the device's MMIO
> + */
> +struct cxl_mem {
> +	struct pci_dev *pdev;
> +	void __iomem *regs;
> +};
> +
> +#endif

Stupid question: can there be more than one CXL.mem capable logical 
device?  I only ask to determine if an ordinal is needed to enumerate 
multiple LDs.

> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index f4ee9a507ac9..a869c8dc24cc 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -4,6 +4,58 @@
>  #include <linux/pci.h>
>  #include <linux/io.h>
>  #include "pci.h"
> +#include "cxl.h"
> +
> +/**
> + * cxl_mem_create() - Create a new &struct cxl_mem.
> + * @pdev: The pci device associated with the new &struct cxl_mem.
> + * @reg_lo: Lower 32b of the register locator
> + * @reg_hi: Upper 32b of the register locator.
> + *
> + * Return: The new &struct cxl_mem on success, NULL on failure.
> + *
> + * Map the BAR for a CXL memory device. This BAR has the memory device's
> + * registers for the device as specified in CXL specification.
> + */
> +static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
> +				      u32 reg_hi)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cxl_mem *cxlm;
> +	void __iomem *regs;
> +	u64 offset;
> +	u8 bar;
> +	int rc;
> +
> +	offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK);
> +	bar = (reg_lo >> CXL_REGLOC_BIR_SHIFT) & CXL_REGLOC_BIR_MASK;
> +
> +	/* Basic sanity check that BAR is big enough */
> +	if (pci_resource_len(pdev, bar) < offset) {
> +		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
> +			&pdev->resource[bar], (unsigned long long)offset);
> +		return NULL;
> +	}
> +
> +	rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev));
> +	if (rc != 0) {
> +		dev_err(dev, "failed to map registers\n");
> +		return NULL;
> +	}
> +
> +	cxlm = devm_kzalloc(&pdev->dev, sizeof(*cxlm), GFP_KERNEL);
> +	if (!cxlm) {
> +		dev_err(dev, "No memory available\n");
> +		return NULL;
> +	}
> +
> +	regs = pcim_iomap_table(pdev)[bar];
> +	cxlm->pdev = pdev;
> +	cxlm->regs = regs + offset;
> +
> +	dev_dbg(dev, "Mapped CXL Memory Device resource\n");
> +	return cxlm;
> +}
>  
>  static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
>  {
> @@ -32,15 +84,42 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
>  static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
>  	struct device *dev = &pdev->dev;
> -	int regloc;
> +	struct cxl_mem *cxlm;
> +	int rc, regloc, i;
> +
> +	rc = pcim_enable_device(pdev);
> +	if (rc)
> +		return rc;
>  
>  	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC);
>  	if (!regloc) {
>  		dev_err(dev, "register location dvsec not found\n");
>  		return -ENXIO;
>  	}
> +	regloc += 0xc; /* Skip DVSEC + reserved fields */

Assuming the DVSEC revision number is always 0x0 or there's no value in 
storing this in struct cxl_mem for the future.

Acked-by: David Rientjes <rientjes@google.com>
Ben Widawsky Feb. 1, 2021, 4:46 p.m. UTC | #2
On 21-01-30 15:51:42, David Rientjes wrote:
> On Fri, 29 Jan 2021, Ben Widawsky wrote:
> 
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > new file mode 100644
> > index 000000000000..d81d0ba4617c
> > --- /dev/null
> > +++ b/drivers/cxl/cxl.h
> > @@ -0,0 +1,17 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/* Copyright(c) 2020 Intel Corporation. */
> > +
> > +#ifndef __CXL_H__
> > +#define __CXL_H__
> > +
> > +/**
> > + * struct cxl_mem - A CXL memory device
> > + * @pdev: The PCI device associated with this CXL device.
> > + * @regs: IO mappings to the device's MMIO
> > + */
> > +struct cxl_mem {
> > +	struct pci_dev *pdev;
> > +	void __iomem *regs;
> > +};
> > +
> > +#endif
> 
> Stupid question: can there be more than one CXL.mem capable logical 
> device?  I only ask to determine if an ordinal is needed to enumerate 
> multiple LDs.

Not a stupid question at all. I admit, I haven't spent much time thinking about
MLDs. I don't have a solid answer to your question. As I understand it, the
devices in the virtual hierarchy will appear as individual CXL type 3 device
components (2.4 in the spec) and transparent to software. A few times I've
attempted to think about MLDs, get confused, and go do something else. The only
MLD specificity I know of is the MLD DVSEC (8.1.10), which seems not incredibly
interesting to me at present (basically, only supporting hot reset).

> 
> > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > index f4ee9a507ac9..a869c8dc24cc 100644
> > --- a/drivers/cxl/mem.c
> > +++ b/drivers/cxl/mem.c
> > @@ -4,6 +4,58 @@
> >  #include <linux/pci.h>
> >  #include <linux/io.h>
> >  #include "pci.h"
> > +#include "cxl.h"
> > +
> > +/**
> > + * cxl_mem_create() - Create a new &struct cxl_mem.
> > + * @pdev: The pci device associated with the new &struct cxl_mem.
> > + * @reg_lo: Lower 32b of the register locator
> > + * @reg_hi: Upper 32b of the register locator.
> > + *
> > + * Return: The new &struct cxl_mem on success, NULL on failure.
> > + *
> > + * Map the BAR for a CXL memory device. This BAR has the memory device's
> > + * registers for the device as specified in CXL specification.
> > + */
> > +static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
> > +				      u32 reg_hi)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct cxl_mem *cxlm;
> > +	void __iomem *regs;
> > +	u64 offset;
> > +	u8 bar;
> > +	int rc;
> > +
> > +	offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK);
> > +	bar = (reg_lo >> CXL_REGLOC_BIR_SHIFT) & CXL_REGLOC_BIR_MASK;
> > +
> > +	/* Basic sanity check that BAR is big enough */
> > +	if (pci_resource_len(pdev, bar) < offset) {
> > +		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
> > +			&pdev->resource[bar], (unsigned long long)offset);
> > +		return NULL;
> > +	}
> > +
> > +	rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev));
> > +	if (rc != 0) {
> > +		dev_err(dev, "failed to map registers\n");
> > +		return NULL;
> > +	}
> > +
> > +	cxlm = devm_kzalloc(&pdev->dev, sizeof(*cxlm), GFP_KERNEL);
> > +	if (!cxlm) {
> > +		dev_err(dev, "No memory available\n");
> > +		return NULL;
> > +	}
> > +
> > +	regs = pcim_iomap_table(pdev)[bar];
> > +	cxlm->pdev = pdev;
> > +	cxlm->regs = regs + offset;
> > +
> > +	dev_dbg(dev, "Mapped CXL Memory Device resource\n");
> > +	return cxlm;
> > +}
> >  
> >  static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
> >  {
> > @@ -32,15 +84,42 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
> >  static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >  {
> >  	struct device *dev = &pdev->dev;
> > -	int regloc;
> > +	struct cxl_mem *cxlm;
> > +	int rc, regloc, i;
> > +
> > +	rc = pcim_enable_device(pdev);
> > +	if (rc)
> > +		return rc;
> >  
> >  	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC);
> >  	if (!regloc) {
> >  		dev_err(dev, "register location dvsec not found\n");
> >  		return -ENXIO;
> >  	}
> > +	regloc += 0xc; /* Skip DVSEC + reserved fields */
> 
> Assuming the DVSEC revision number is always 0x0 or there's no value in 
> storing this in struct cxl_mem for the future.

So this logic actually came from Dan originally, so don't take this necessarily
as the authoritative answer.

At some point revision id will need to be considered. However, the consortium
seems to be going to great lengths (kudos) to make all modifications backward
compatible. As such, we can consider this the driver for rev0 (the only such rev
in existence today), and when a new rev comes along, figure out how to best
handle it. However, the expectation is that this code will still work for revN.

> 
> Acked-by: David Rientjes <rientjes@google.com>

Thanks!
Konrad Rzeszutek Wilk Feb. 1, 2021, 5:36 p.m. UTC | #3
>  
> -	return 0;
> +	rc = -ENXIO;
> +	for (i = regloc; i < regloc + 0x24; i += 8) {

This 0x24, 8, and
> +		u32 reg_lo, reg_hi;
> +		u8 reg_type;
> +
> +		/* "register low and high" contain other bits */
> +		pci_read_config_dword(pdev, i, &reg_lo);
> +		pci_read_config_dword(pdev, i + 4, &reg_hi);

4

scream of #define's or sizeof()?
Jonathan Cameron Feb. 1, 2021, 6:19 p.m. UTC | #4
On Mon, 1 Feb 2021 08:46:24 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> On 21-01-30 15:51:42, David Rientjes wrote:
> > On Fri, 29 Jan 2021, Ben Widawsky wrote:
> >   
> > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > > new file mode 100644
> > > index 000000000000..d81d0ba4617c
> > > --- /dev/null
> > > +++ b/drivers/cxl/cxl.h
> > > @@ -0,0 +1,17 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +/* Copyright(c) 2020 Intel Corporation. */
> > > +
> > > +#ifndef __CXL_H__
> > > +#define __CXL_H__
> > > +
> > > +/**
> > > + * struct cxl_mem - A CXL memory device
> > > + * @pdev: The PCI device associated with this CXL device.
> > > + * @regs: IO mappings to the device's MMIO
> > > + */
> > > +struct cxl_mem {
> > > +	struct pci_dev *pdev;
> > > +	void __iomem *regs;
> > > +};
> > > +
> > > +#endif  
> > 
> > Stupid question: can there be more than one CXL.mem capable logical 
> > device?  I only ask to determine if an ordinal is needed to enumerate 
> > multiple LDs.  
> 
> Not a stupid question at all. I admit, I haven't spent much time thinking about
> MLDs. I don't have a solid answer to your question. As I understand it, the
> devices in the virtual hierarchy will appear as individual CXL type 3 device
> components (2.4 in the spec) and transparent to software. A few times I've
> attempted to think about MLDs, get confused, and go do something else. The only
> MLD specificity I know of is the MLD DVSEC (8.1.10), which seems not incredibly
> interesting to me at present (basically, only supporting hot reset).

That's my understanding as well.  If you have an MLD (and hence multiple logical memory
devices) the fact they have multiple logical devices will be nearly invisible to
any given host (will more or less look like an SLD).  Configuration via the
fabric manager API will be invisible to the host other than via hotplug
events when the configuration changes.
Note the MLD DVSEC is only in the fabric manager owned logical device (so Linux
won't see it in the PCI hierarchy).  Note the fabric manager is usually controlled by
a BMC or similar.

Various registers become hardwired to 0 and certain writes are ignored when it's
an MLD logical device rather than an physical device (SLD)

We might want to look at emulating this in QEMU to give us a
platform to verify the spec (particularly to make sure the various hardwired
registers don't cause any problems), but I'd not expect to see anything specific
in kernel support.

Jonathan



> 
> >   
> > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > > index f4ee9a507ac9..a869c8dc24cc 100644
> > > --- a/drivers/cxl/mem.c
> > > +++ b/drivers/cxl/mem.c
> > > @@ -4,6 +4,58 @@
> > >  #include <linux/pci.h>
> > >  #include <linux/io.h>
> > >  #include "pci.h"
> > > +#include "cxl.h"
> > > +
> > > +/**
> > > + * cxl_mem_create() - Create a new &struct cxl_mem.
> > > + * @pdev: The pci device associated with the new &struct cxl_mem.
> > > + * @reg_lo: Lower 32b of the register locator
> > > + * @reg_hi: Upper 32b of the register locator.
> > > + *
> > > + * Return: The new &struct cxl_mem on success, NULL on failure.
> > > + *
> > > + * Map the BAR for a CXL memory device. This BAR has the memory device's
> > > + * registers for the device as specified in CXL specification.
> > > + */
> > > +static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
> > > +				      u32 reg_hi)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct cxl_mem *cxlm;
> > > +	void __iomem *regs;
> > > +	u64 offset;
> > > +	u8 bar;
> > > +	int rc;
> > > +
> > > +	offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK);
> > > +	bar = (reg_lo >> CXL_REGLOC_BIR_SHIFT) & CXL_REGLOC_BIR_MASK;
> > > +
> > > +	/* Basic sanity check that BAR is big enough */
> > > +	if (pci_resource_len(pdev, bar) < offset) {
> > > +		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
> > > +			&pdev->resource[bar], (unsigned long long)offset);
> > > +		return NULL;
> > > +	}
> > > +
> > > +	rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev));
> > > +	if (rc != 0) {
> > > +		dev_err(dev, "failed to map registers\n");
> > > +		return NULL;
> > > +	}
> > > +
> > > +	cxlm = devm_kzalloc(&pdev->dev, sizeof(*cxlm), GFP_KERNEL);
> > > +	if (!cxlm) {
> > > +		dev_err(dev, "No memory available\n");
> > > +		return NULL;
> > > +	}
> > > +
> > > +	regs = pcim_iomap_table(pdev)[bar];
> > > +	cxlm->pdev = pdev;
> > > +	cxlm->regs = regs + offset;
> > > +
> > > +	dev_dbg(dev, "Mapped CXL Memory Device resource\n");
> > > +	return cxlm;
> > > +}
> > >  
> > >  static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
> > >  {
> > > @@ -32,15 +84,42 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
> > >  static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > >  {
> > >  	struct device *dev = &pdev->dev;
> > > -	int regloc;
> > > +	struct cxl_mem *cxlm;
> > > +	int rc, regloc, i;
> > > +
> > > +	rc = pcim_enable_device(pdev);
> > > +	if (rc)
> > > +		return rc;
> > >  
> > >  	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC);
> > >  	if (!regloc) {
> > >  		dev_err(dev, "register location dvsec not found\n");
> > >  		return -ENXIO;
> > >  	}
> > > +	regloc += 0xc; /* Skip DVSEC + reserved fields */  
> > 
> > Assuming the DVSEC revision number is always 0x0 or there's no value in 
> > storing this in struct cxl_mem for the future.  
> 
> So this logic actually came from Dan originally, so don't take this necessarily
> as the authoritative answer.
> 
> At some point revision id will need to be considered. However, the consortium
> seems to be going to great lengths (kudos) to make all modifications backward
> compatible. As such, we can consider this the driver for rev0 (the only such rev
> in existence today), and when a new rev comes along, figure out how to best
> handle it. However, the expectation is that this code will still work for revN.
> 
> > 
> > Acked-by: David Rientjes <rientjes@google.com>  
> 
> Thanks!
Christoph Hellwig Feb. 2, 2021, 6:04 p.m. UTC | #5
Any reason not to merge a bunch of patches?  Both this one and
the previous one are rather useless on their own, making review
harder than necessary.

> + * cxl_mem_create() - Create a new &struct cxl_mem.
> + * @pdev: The pci device associated with the new &struct cxl_mem.
> + * @reg_lo: Lower 32b of the register locator
> + * @reg_hi: Upper 32b of the register locator.
> + *
> + * Return: The new &struct cxl_mem on success, NULL on failure.
> + *
> + * Map the BAR for a CXL memory device. This BAR has the memory device's
> + * registers for the device as specified in CXL specification.
> + */

A lot of text with almost no value over just reading the function.
What's that fetish with kerneldoc comments for trivial static functions?

> +		reg_type =
> +			(reg_lo >> CXL_REGLOC_RBI_SHIFT) & CXL_REGLOC_RBI_MASK;

OTOH this screams for a helper that would make the code a lot more
self documenting.

> +		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
> +			rc = 0;
> +			cxlm = cxl_mem_create(pdev, reg_lo, reg_hi);
> +			if (!cxlm)
> +				rc = -ENODEV;
> +			break;

And given that we're going to grow more types eventually, why not start
out with a switch here?  Also why return the structure when nothing
uses it?
Ben Widawsky Feb. 2, 2021, 6:31 p.m. UTC | #6
On 21-02-02 18:04:41, Christoph Hellwig wrote:
> Any reason not to merge a bunch of patches?  Both this one and
> the previous one are rather useless on their own, making review
> harder than necessary.
> 

As this is an initial driver, there's obviously no functional/regression testing
value in separating the patches.

This was the way we brought up the driver and how we verified functionality
incrementally. I see value in both capturing that in the history, as well as
making review a bit easier (which apparently failed for you).

> > + * cxl_mem_create() - Create a new &struct cxl_mem.
> > + * @pdev: The pci device associated with the new &struct cxl_mem.
> > + * @reg_lo: Lower 32b of the register locator
> > + * @reg_hi: Upper 32b of the register locator.
> > + *
> > + * Return: The new &struct cxl_mem on success, NULL on failure.
> > + *
> > + * Map the BAR for a CXL memory device. This BAR has the memory device's
> > + * registers for the device as specified in CXL specification.
> > + */
> 
> A lot of text with almost no value over just reading the function.
> What's that fetish with kerneldoc comments for trivial static functions?
> 
> > +		reg_type =
> > +			(reg_lo >> CXL_REGLOC_RBI_SHIFT) & CXL_REGLOC_RBI_MASK;
> 
> OTOH this screams for a helper that would make the code a lot more
> self documenting.
> 

I agree, I'll change this.

> > +		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
> > +			rc = 0;
> > +			cxlm = cxl_mem_create(pdev, reg_lo, reg_hi);
> > +			if (!cxlm)
> > +				rc = -ENODEV;
> > +			break;
> 
> And given that we're going to grow more types eventually, why not start
> out with a switch here?  Also why return the structure when nothing
> uses it?

 We've (Intel) already started working on the libnvdimm integration which does
 change this around a bit. In order to go with what's best tested though, I've
 chosen to use this as is for merge. Many different people not just in Intel
 have tested these codepaths. The resulting code moves the check on register
 type out of this function entirely.

 If you'd like me to make it a switch, I can, but it's going to be extracted
 later anyway.
Christoph Hellwig Feb. 3, 2021, 5:12 p.m. UTC | #7
On Tue, Feb 02, 2021 at 10:31:51AM -0800, Ben Widawsky wrote:
> > > +		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
> > > +			rc = 0;
> > > +			cxlm = cxl_mem_create(pdev, reg_lo, reg_hi);
> > > +			if (!cxlm)
> > > +				rc = -ENODEV;
> > > +			break;
> > 
> > And given that we're going to grow more types eventually, why not start
> > out with a switch here?  Also why return the structure when nothing
> > uses it?
> 
>  We've (Intel) already started working on the libnvdimm integration which does
>  change this around a bit. In order to go with what's best tested though, I've
>  chosen to use this as is for merge. Many different people not just in Intel
>  have tested these codepaths. The resulting code moves the check on register
>  type out of this function entirely.
> 
>  If you'd like me to make it a switch, I can, but it's going to be extracted
>  later anyway.

This was just a suggestion.  No hard feelings, it's just that the code
looks a little odd to me.
diff mbox series

Patch

diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
new file mode 100644
index 000000000000..d81d0ba4617c
--- /dev/null
+++ b/drivers/cxl/cxl.h
@@ -0,0 +1,17 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright(c) 2020 Intel Corporation. */
+
+#ifndef __CXL_H__
+#define __CXL_H__
+
+/**
+ * struct cxl_mem - A CXL memory device
+ * @pdev: The PCI device associated with this CXL device.
+ * @regs: IO mappings to the device's MMIO
+ */
+struct cxl_mem {
+	struct pci_dev *pdev;
+	void __iomem *regs;
+};
+
+#endif
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index f4ee9a507ac9..a869c8dc24cc 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -4,6 +4,58 @@ 
 #include <linux/pci.h>
 #include <linux/io.h>
 #include "pci.h"
+#include "cxl.h"
+
+/**
+ * cxl_mem_create() - Create a new &struct cxl_mem.
+ * @pdev: The pci device associated with the new &struct cxl_mem.
+ * @reg_lo: Lower 32b of the register locator
+ * @reg_hi: Upper 32b of the register locator.
+ *
+ * Return: The new &struct cxl_mem on success, NULL on failure.
+ *
+ * Map the BAR for a CXL memory device. This BAR has the memory device's
+ * registers for the device as specified in CXL specification.
+ */
+static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
+				      u32 reg_hi)
+{
+	struct device *dev = &pdev->dev;
+	struct cxl_mem *cxlm;
+	void __iomem *regs;
+	u64 offset;
+	u8 bar;
+	int rc;
+
+	offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK);
+	bar = (reg_lo >> CXL_REGLOC_BIR_SHIFT) & CXL_REGLOC_BIR_MASK;
+
+	/* Basic sanity check that BAR is big enough */
+	if (pci_resource_len(pdev, bar) < offset) {
+		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
+			&pdev->resource[bar], (unsigned long long)offset);
+		return NULL;
+	}
+
+	rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev));
+	if (rc != 0) {
+		dev_err(dev, "failed to map registers\n");
+		return NULL;
+	}
+
+	cxlm = devm_kzalloc(&pdev->dev, sizeof(*cxlm), GFP_KERNEL);
+	if (!cxlm) {
+		dev_err(dev, "No memory available\n");
+		return NULL;
+	}
+
+	regs = pcim_iomap_table(pdev)[bar];
+	cxlm->pdev = pdev;
+	cxlm->regs = regs + offset;
+
+	dev_dbg(dev, "Mapped CXL Memory Device resource\n");
+	return cxlm;
+}
 
 static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
 {
@@ -32,15 +84,42 @@  static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
 static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	struct device *dev = &pdev->dev;
-	int regloc;
+	struct cxl_mem *cxlm;
+	int rc, regloc, i;
+
+	rc = pcim_enable_device(pdev);
+	if (rc)
+		return rc;
 
 	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC);
 	if (!regloc) {
 		dev_err(dev, "register location dvsec not found\n");
 		return -ENXIO;
 	}
+	regloc += 0xc; /* Skip DVSEC + reserved fields */
 
-	return 0;
+	rc = -ENXIO;
+	for (i = regloc; i < regloc + 0x24; i += 8) {
+		u32 reg_lo, reg_hi;
+		u8 reg_type;
+
+		/* "register low and high" contain other bits */
+		pci_read_config_dword(pdev, i, &reg_lo);
+		pci_read_config_dword(pdev, i + 4, &reg_hi);
+
+		reg_type =
+			(reg_lo >> CXL_REGLOC_RBI_SHIFT) & CXL_REGLOC_RBI_MASK;
+
+		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
+			rc = 0;
+			cxlm = cxl_mem_create(pdev, reg_lo, reg_hi);
+			if (!cxlm)
+				rc = -ENODEV;
+			break;
+		}
+	}
+
+	return rc;
 }
 
 static const struct pci_device_id cxl_mem_pci_tbl[] = {
diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h
index a8a9935fa90b..df222edb6ac3 100644
--- a/drivers/cxl/pci.h
+++ b/drivers/cxl/pci.h
@@ -17,4 +17,18 @@ 
 
 #define PCI_DVSEC_ID_CXL_REGLOC		0x8
 
+/* BAR Indicator Register (BIR) */
+#define CXL_REGLOC_BIR_SHIFT 0
+#define CXL_REGLOC_BIR_MASK 0x7
+
+/* Register Block Identifier (RBI) */
+#define CXL_REGLOC_RBI_SHIFT 8
+#define CXL_REGLOC_RBI_MASK 0xff
+#define CXL_REGLOC_RBI_EMPTY 0
+#define CXL_REGLOC_RBI_COMPONENT 1
+#define CXL_REGLOC_RBI_VIRT 2
+#define CXL_REGLOC_RBI_MEMDEV 3
+
+#define CXL_REGLOC_ADDR_MASK 0xffff0000
+
 #endif /* __CXL_PCI_H__ */