* [PATCH 0/4] Map register blocks individually
@ 2021-05-06 22:36 ira.weiny
2021-05-06 22:36 ` [PATCH 1/4] cxl/mem: Fully decode device capability header ira.weiny
` (4 more replies)
0 siblings, 5 replies; 10+ messages in thread
From: ira.weiny @ 2021-05-06 22:36 UTC (permalink / raw)
To: Ben Widawsky, Dan Williams
Cc: Ira Weiny, Alison Schofield, Vishal Verma, Jonathan Cameron,
linux-cxl, linux-kernel
From: Ira Weiny <ira.weiny@intel.com>
User space will want to map some register blocks. Currently BARs are mapped in
their entirety and pointers to the register blocks are created into those
mappings. This will prevent mappings from user space.
This series has 3 clean up patches followed by a patch to mapping the register
blocks individually.
Unfortunately, the information for the register blocks is contained inside the
BARs themselves. Which means the BAR must be mapped, probed, and unmapped
prior to the registers being mapped individually.
The probe stage creates list of register maps which is then iterated to map
the individual register blocks.
Ira Weiny (4):
cxl/mem: Fully decode device capability header
cxl/mem: Reserve all device regions at once
cxl/mem: Introduce cxl_decode_register_block()
cxl/mem: Map registers based on capabilities
drivers/cxl/core.c | 84 ++++++++++++++++++++------
drivers/cxl/cxl.h | 34 +++++++++--
drivers/cxl/pci.c | 147 +++++++++++++++++++++++++++++++++++----------
3 files changed, 211 insertions(+), 54 deletions(-)
--
2.28.0.rc0.12.gb6a658bd00c9
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/4] cxl/mem: Fully decode device capability header 2021-05-06 22:36 [PATCH 0/4] Map register blocks individually ira.weiny @ 2021-05-06 22:36 ` ira.weiny 2021-05-20 0:50 ` Dan Williams 2021-05-06 22:36 ` [PATCH 2/4] cxl/mem: Reserve all device regions at once ira.weiny ` (3 subsequent siblings) 4 siblings, 1 reply; 10+ messages in thread From: ira.weiny @ 2021-05-06 22:36 UTC (permalink / raw) To: Ben Widawsky, Dan Williams Cc: Ira Weiny, Alison Schofield, Vishal Verma, Jonathan Cameron, linux-cxl, linux-kernel From: Ira Weiny <ira.weiny@intel.com> Previously only the capability ID and offset were decoded. Create a version MASK and decode the additional version and length fields of the header. Signed-off-by: Ira Weiny <ira.weiny@intel.com> --- drivers/cxl/core.c | 15 ++++++++++++--- drivers/cxl/cxl.h | 1 + 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c index b3c3532b53f7..21553386e218 100644 --- a/drivers/cxl/core.c +++ b/drivers/cxl/core.c @@ -501,12 +501,21 @@ void cxl_setup_device_regs(struct device *dev, void __iomem *base, for (cap = 1; cap <= cap_count; cap++) { void __iomem *register_block; - u32 offset; + u32 hdr, offset, __maybe_unused length; u16 cap_id; + u8 version; + + hdr = readl(base + cap * 0x10); + + cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK, hdr); + version = FIELD_GET(CXLDEV_CAP_HDR_VERSION_MASK, hdr); + if (version != 1) + dev_err(dev, "Vendor cap ID: %x incorrect version (0x%x)\n", + cap_id, version); - cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK, - readl(base + cap * 0x10)); offset = readl(base + cap * 0x10 + 0x4); + length = readl(base + cap * 0x10 + 0x8); + register_block = base + offset; switch (cap_id) { diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 0211f44c95a2..9b315c069557 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -15,6 +15,7 @@ #define CXLDEV_CAP_ARRAY_COUNT_MASK GENMASK_ULL(47, 32) /* CXL 2.0 8.2.8.2 CXL Device Capability Header Register */ #define CXLDEV_CAP_HDR_CAP_ID_MASK GENMASK(15, 0) +#define CXLDEV_CAP_HDR_VERSION_MASK GENMASK(23, 16) /* CXL 2.0 8.2.8.2.1 CXL Device Capabilities */ #define CXLDEV_CAP_CAP_ID_DEVICE_STATUS 0x1 #define CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX 0x2 -- 2.28.0.rc0.12.gb6a658bd00c9 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/4] cxl/mem: Fully decode device capability header 2021-05-06 22:36 ` [PATCH 1/4] cxl/mem: Fully decode device capability header ira.weiny @ 2021-05-20 0:50 ` Dan Williams 2021-05-20 17:42 ` Ira Weiny 0 siblings, 1 reply; 10+ messages in thread From: Dan Williams @ 2021-05-20 0:50 UTC (permalink / raw) To: Weiny, Ira Cc: Ben Widawsky, Alison Schofield, Vishal Verma, Jonathan Cameron, linux-cxl, Linux Kernel Mailing List On Thu, May 6, 2021 at 3:37 PM <ira.weiny@intel.com> wrote: > > From: Ira Weiny <ira.weiny@intel.com> > > Previously only the capability ID and offset were decoded. > > Create a version MASK and decode the additional version and length > fields of the header. > I'm not seeing a justification for why Linux would want this patch? > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > --- > drivers/cxl/core.c | 15 ++++++++++++--- > drivers/cxl/cxl.h | 1 + > 2 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c > index b3c3532b53f7..21553386e218 100644 > --- a/drivers/cxl/core.c > +++ b/drivers/cxl/core.c > @@ -501,12 +501,21 @@ void cxl_setup_device_regs(struct device *dev, void __iomem *base, > > for (cap = 1; cap <= cap_count; cap++) { > void __iomem *register_block; > - u32 offset; > + u32 hdr, offset, __maybe_unused length; What's the point of reading the length and not using it? If this is used in a future patch then wait until then to add it. > u16 cap_id; > + u8 version; > + > + hdr = readl(base + cap * 0x10); > + > + cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK, hdr); > + version = FIELD_GET(CXLDEV_CAP_HDR_VERSION_MASK, hdr); > + if (version != 1) > + dev_err(dev, "Vendor cap ID: %x incorrect version (0x%x)\n", > + cap_id, version); It's not an error. Any future version needs to be backwards compatible. All this is doing is ensuring that when hardware is updated old kernels will start spamming the log. > > - cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK, > - readl(base + cap * 0x10)); > offset = readl(base + cap * 0x10 + 0x4); > + length = readl(base + cap * 0x10 + 0x8); > + > register_block = base + offset; > > switch (cap_id) { > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 0211f44c95a2..9b315c069557 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -15,6 +15,7 @@ > #define CXLDEV_CAP_ARRAY_COUNT_MASK GENMASK_ULL(47, 32) > /* CXL 2.0 8.2.8.2 CXL Device Capability Header Register */ > #define CXLDEV_CAP_HDR_CAP_ID_MASK GENMASK(15, 0) > +#define CXLDEV_CAP_HDR_VERSION_MASK GENMASK(23, 16) > /* CXL 2.0 8.2.8.2.1 CXL Device Capabilities */ > #define CXLDEV_CAP_CAP_ID_DEVICE_STATUS 0x1 > #define CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX 0x2 > -- > 2.28.0.rc0.12.gb6a658bd00c9 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/4] cxl/mem: Fully decode device capability header 2021-05-20 0:50 ` Dan Williams @ 2021-05-20 17:42 ` Ira Weiny 0 siblings, 0 replies; 10+ messages in thread From: Ira Weiny @ 2021-05-20 17:42 UTC (permalink / raw) To: Dan Williams Cc: Ben Widawsky, Alison Schofield, Vishal Verma, Jonathan Cameron, linux-cxl, Linux Kernel Mailing List On Wed, May 19, 2021 at 05:50:44PM -0700, Dan Williams wrote: > On Thu, May 6, 2021 at 3:37 PM <ira.weiny@intel.com> wrote: > > > > From: Ira Weiny <ira.weiny@intel.com> > > > > Previously only the capability ID and offset were decoded. > > > > Create a version MASK and decode the additional version and length > > fields of the header. > > > > I'm not seeing a justification for why Linux would want this patch? > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > --- > > drivers/cxl/core.c | 15 ++++++++++++--- > > drivers/cxl/cxl.h | 1 + > > 2 files changed, 13 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c > > index b3c3532b53f7..21553386e218 100644 > > --- a/drivers/cxl/core.c > > +++ b/drivers/cxl/core.c > > @@ -501,12 +501,21 @@ void cxl_setup_device_regs(struct device *dev, void __iomem *base, > > > > for (cap = 1; cap <= cap_count; cap++) { > > void __iomem *register_block; > > - u32 offset; > > + u32 hdr, offset, __maybe_unused length; > > What's the point of reading the length and not using it? Then length is needed in a future patch and it seemed easier to split by saying we are just going to decode the entire header as a patch. Ben caught this too because sparse found the unused variable... Let me see about squashing this into the future patch. > If this is > used in a future patch then wait until then to add it. ok. > > > u16 cap_id; > > + u8 version; > > + > > + hdr = readl(base + cap * 0x10); > > + > > + cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK, hdr); > > + version = FIELD_GET(CXLDEV_CAP_HDR_VERSION_MASK, hdr); > > + if (version != 1) > > + dev_err(dev, "Vendor cap ID: %x incorrect version (0x%x)\n", > > + cap_id, version); > > It's not an error. Any future version needs to be backwards > compatible. All this is doing is ensuring that when hardware is > updated old kernels will start spamming the log. Yep... After deleting this I think I should just 'forward squash' the length into the patch where I need it. Ira > > > > > - cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK, > > - readl(base + cap * 0x10)); > > offset = readl(base + cap * 0x10 + 0x4); > > + length = readl(base + cap * 0x10 + 0x8); > > + > > register_block = base + offset; > > > > switch (cap_id) { > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > > index 0211f44c95a2..9b315c069557 100644 > > --- a/drivers/cxl/cxl.h > > +++ b/drivers/cxl/cxl.h > > @@ -15,6 +15,7 @@ > > #define CXLDEV_CAP_ARRAY_COUNT_MASK GENMASK_ULL(47, 32) > > /* CXL 2.0 8.2.8.2 CXL Device Capability Header Register */ > > #define CXLDEV_CAP_HDR_CAP_ID_MASK GENMASK(15, 0) > > +#define CXLDEV_CAP_HDR_VERSION_MASK GENMASK(23, 16) > > /* CXL 2.0 8.2.8.2.1 CXL Device Capabilities */ > > #define CXLDEV_CAP_CAP_ID_DEVICE_STATUS 0x1 > > #define CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX 0x2 > > -- > > 2.28.0.rc0.12.gb6a658bd00c9 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/4] cxl/mem: Reserve all device regions at once 2021-05-06 22:36 [PATCH 0/4] Map register blocks individually ira.weiny 2021-05-06 22:36 ` [PATCH 1/4] cxl/mem: Fully decode device capability header ira.weiny @ 2021-05-06 22:36 ` ira.weiny 2021-05-20 1:00 ` Dan Williams 2021-05-06 22:36 ` [PATCH 3/4] cxl/mem: Introduce cxl_decode_register_block() ira.weiny ` (2 subsequent siblings) 4 siblings, 1 reply; 10+ messages in thread From: ira.weiny @ 2021-05-06 22:36 UTC (permalink / raw) To: Ben Widawsky, Dan Williams Cc: Ira Weiny, Alison Schofield, Vishal Verma, Jonathan Cameron, linux-cxl, linux-kernel From: Ira Weiny <ira.weiny@intel.com> In order to remap individual register sets each bar region must be reserved prior to mapping. Because the details of individual register sets are contained within the BARs themselves, the bar must be mapped 2 times, once to extract this information and a second time for each register set. Rather than attempt to reserve each BAR individually and track if that bar has been reserved. Open code pcim_iomap_regions() by first reserving all memory regions on the device and then mapping the bars individually as needed. Signed-off-by: Ira Weiny <ira.weiny@intel.com> --- drivers/cxl/pci.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index 191603b4e10b..40016709b310 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -926,9 +926,9 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 { struct pci_dev *pdev = cxlm->pdev; struct device *dev = &pdev->dev; + void __iomem *rc; u64 offset; u8 bar; - int rc; offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK); bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo); @@ -940,13 +940,14 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 return (void __iomem *)ERR_PTR(-ENXIO); } - rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev)); - if (rc) { + rc = pcim_iomap(pdev, bar, 0); + if (!rc) { dev_err(dev, "failed to map registers\n"); - return (void __iomem *)ERR_PTR(rc); + return (void __iomem *)ERR_PTR(-ENOMEM); } - dev_dbg(dev, "Mapped CXL Memory Device resource\n"); + dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ 0x%llx\n", + bar, offset); return pcim_iomap_table(pdev)[bar] + offset; } @@ -999,6 +1000,9 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) return -ENXIO; } + if (pci_request_mem_regions(pdev, pci_name(pdev))) + return -ENODEV; + /* Get the size of the Register Locator DVSEC */ pci_read_config_dword(pdev, regloc + PCI_DVSEC_HEADER1, ®loc_size); regloc_size = FIELD_GET(PCI_DVSEC_HEADER1_LENGTH_MASK, regloc_size); -- 2.28.0.rc0.12.gb6a658bd00c9 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] cxl/mem: Reserve all device regions at once 2021-05-06 22:36 ` [PATCH 2/4] cxl/mem: Reserve all device regions at once ira.weiny @ 2021-05-20 1:00 ` Dan Williams 2021-05-20 19:44 ` Ira Weiny 0 siblings, 1 reply; 10+ messages in thread From: Dan Williams @ 2021-05-20 1:00 UTC (permalink / raw) To: Weiny, Ira Cc: Ben Widawsky, Alison Schofield, Vishal Verma, Jonathan Cameron, linux-cxl, Linux Kernel Mailing List On Thu, May 6, 2021 at 3:37 PM <ira.weiny@intel.com> wrote: > > From: Ira Weiny <ira.weiny@intel.com> > > In order to remap individual register sets each bar region must be > reserved prior to mapping. Because the details of individual register > sets are contained within the BARs themselves, the bar must be mapped 2 > times, once to extract this information and a second time for each > register set. > > Rather than attempt to reserve each BAR individually and track if that > bar has been reserved. Open code pcim_iomap_regions() by first > reserving all memory regions on the device and then mapping the bars > individually as needed. > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > --- > drivers/cxl/pci.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 191603b4e10b..40016709b310 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -926,9 +926,9 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 > { > struct pci_dev *pdev = cxlm->pdev; > struct device *dev = &pdev->dev; > + void __iomem *rc; > u64 offset; > u8 bar; > - int rc; > > offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK); > bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo); > @@ -940,13 +940,14 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 > return (void __iomem *)ERR_PTR(-ENXIO); > } > > - rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev)); > - if (rc) { > + rc = pcim_iomap(pdev, bar, 0); It is odd that the temporary region pinning uses non-pcim, but the temporary mapping using pcim. > + if (!rc) { > dev_err(dev, "failed to map registers\n"); > - return (void __iomem *)ERR_PTR(rc); > + return (void __iomem *)ERR_PTR(-ENOMEM); I think since this support is a bug/compatibility fix it should probably be rebased to current cxl.git#next. If you still end up needing to return an __iomem ERR_PTR() then use IOMEM_ERR_PTR. > } > > - dev_dbg(dev, "Mapped CXL Memory Device resource\n"); > + dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ 0x%llx\n", s/0x%llx/%#llx/ > + bar, offset); > > return pcim_iomap_table(pdev)[bar] + offset; > } > @@ -999,6 +1000,9 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) > return -ENXIO; > } > > + if (pci_request_mem_regions(pdev, pci_name(pdev))) This tells me this patch is too fine grained and can't stand alone because it is missing the corresponding call to pci_release_mem_regions(). Ideally this would be kept in the same scope as the temporary io mapping. > + return -ENODEV; > + > /* Get the size of the Register Locator DVSEC */ > pci_read_config_dword(pdev, regloc + PCI_DVSEC_HEADER1, ®loc_size); > regloc_size = FIELD_GET(PCI_DVSEC_HEADER1_LENGTH_MASK, regloc_size); > -- > 2.28.0.rc0.12.gb6a658bd00c9 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] cxl/mem: Reserve all device regions at once 2021-05-20 1:00 ` Dan Williams @ 2021-05-20 19:44 ` Ira Weiny 0 siblings, 0 replies; 10+ messages in thread From: Ira Weiny @ 2021-05-20 19:44 UTC (permalink / raw) To: Dan Williams Cc: Ben Widawsky, Alison Schofield, Vishal Verma, Jonathan Cameron, linux-cxl, Linux Kernel Mailing List On Wed, May 19, 2021 at 06:00:51PM -0700, Dan Williams wrote: > On Thu, May 6, 2021 at 3:37 PM <ira.weiny@intel.com> wrote: > > > > From: Ira Weiny <ira.weiny@intel.com> > > > > In order to remap individual register sets each bar region must be > > reserved prior to mapping. Because the details of individual register > > sets are contained within the BARs themselves, the bar must be mapped 2 > > times, once to extract this information and a second time for each > > register set. > > > > Rather than attempt to reserve each BAR individually and track if that > > bar has been reserved. Open code pcim_iomap_regions() by first > > reserving all memory regions on the device and then mapping the bars > > individually as needed. > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > --- > > drivers/cxl/pci.c | 14 +++++++++----- > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > > index 191603b4e10b..40016709b310 100644 > > --- a/drivers/cxl/pci.c > > +++ b/drivers/cxl/pci.c > > @@ -926,9 +926,9 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 > > { > > struct pci_dev *pdev = cxlm->pdev; > > struct device *dev = &pdev->dev; > > + void __iomem *rc; > > u64 offset; > > u8 bar; > > - int rc; > > > > offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK); > > bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo); > > @@ -940,13 +940,14 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 > > return (void __iomem *)ERR_PTR(-ENXIO); > > } > > > > - rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev)); > > - if (rc) { > > + rc = pcim_iomap(pdev, bar, 0); > > It is odd that the temporary region pinning uses non-pcim, but the > temporary mapping using pcim. The idea of this series was to have only the final mappings be managed. But this particular patch does not make that change. Therefore I continue using pcim_iomap() for this patch. > > > + if (!rc) { > > dev_err(dev, "failed to map registers\n"); > > - return (void __iomem *)ERR_PTR(rc); > > + return (void __iomem *)ERR_PTR(-ENOMEM); > > I think since this support is a bug/compatibility fix it should > probably be rebased to current cxl.git#next. If you still end up > needing to return an __iomem ERR_PTR() then use IOMEM_ERR_PTR. Actually the type of rc has changed and I think it is best to just 'return rc;' here. Also, in a side conversation with Dan it seems the name would be more clear. So I will change it. > > > } > > > > - dev_dbg(dev, "Mapped CXL Memory Device resource\n"); > > + dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ 0x%llx\n", > > s/0x%llx/%#llx/ ok > > > + bar, offset); > > > > return pcim_iomap_table(pdev)[bar] + offset; > > } > > @@ -999,6 +1000,9 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) > > return -ENXIO; > > } > > > > + if (pci_request_mem_regions(pdev, pci_name(pdev))) > > This tells me this patch is too fine grained and can't stand alone > because it is missing the corresponding call to > pci_release_mem_regions(). Ideally this would be kept in the same > scope as the temporary io mapping. Ok, I was a bit confused why I did not need this but I don't. Dan and I discussed this offline and because pcim_enable_device() is called all pci functions are automatically managed. So the core will call pci_release_mem_regions() automatically for me. It took a bit of time for us to understand where the release call was. I'll add a comment to this to make this more clear. Ira > > > + return -ENODEV; > > + > > /* Get the size of the Register Locator DVSEC */ > > pci_read_config_dword(pdev, regloc + PCI_DVSEC_HEADER1, ®loc_size); > > regloc_size = FIELD_GET(PCI_DVSEC_HEADER1_LENGTH_MASK, regloc_size); > > -- > > 2.28.0.rc0.12.gb6a658bd00c9 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/4] cxl/mem: Introduce cxl_decode_register_block() 2021-05-06 22:36 [PATCH 0/4] Map register blocks individually ira.weiny 2021-05-06 22:36 ` [PATCH 1/4] cxl/mem: Fully decode device capability header ira.weiny 2021-05-06 22:36 ` [PATCH 2/4] cxl/mem: Reserve all device regions at once ira.weiny @ 2021-05-06 22:36 ` ira.weiny 2021-05-06 22:36 ` [PATCH 4/4] cxl/mem: Map registers based on capabilities ira.weiny 2021-05-20 0:24 ` [PATCH 0/4] Map register blocks individually Dan Williams 4 siblings, 0 replies; 10+ messages in thread From: ira.weiny @ 2021-05-06 22:36 UTC (permalink / raw) To: Ben Widawsky, Dan Williams Cc: Ira Weiny, Alison Schofield, Vishal Verma, Jonathan Cameron, linux-cxl, linux-kernel From: Ira Weiny <ira.weiny@intel.com> Each register block located in the DVSEC needs to be decoded from 2 words, 'register offset high' and 'register offset low'. Create a function, cxl_decode_register_block() to perform this decode and return the bar, offset, and register type of the register block. Then use the values decoded in cxl_mem_map_regblock() instead of passing the raw registers. Signed-off-by: Ira Weiny <ira.weiny@intel.com> --- drivers/cxl/pci.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index 40016709b310..cee14de0f251 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -922,16 +922,12 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev) return cxlm; } -static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi) +static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, + u8 bar, u64 offset) { struct pci_dev *pdev = cxlm->pdev; struct device *dev = &pdev->dev; void __iomem *rc; - u64 offset; - u8 bar; - - offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK); - bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo); /* Basic sanity check that BAR is big enough */ if (pci_resource_len(pdev, bar) < offset) { @@ -975,6 +971,14 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec) return 0; } +static void cxl_decode_register_block(u32 reg_lo, u32 reg_hi, + u8 *bar, u64 *offset, u8 *reg_type) +{ + *offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK); + *bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo); + *reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo); +} + /** * cxl_mem_setup_regs() - Setup necessary MMIO. * @cxlm: The CXL memory device to communicate with. @@ -1013,15 +1017,21 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) for (i = 0; i < regblocks; i++, regloc += 8) { u32 reg_lo, reg_hi; u8 reg_type; + u64 offset; + u8 bar; /* "register low and high" contain other bits */ pci_read_config_dword(pdev, regloc, ®_lo); pci_read_config_dword(pdev, regloc + 4, ®_hi); - reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo); + cxl_decode_register_block(reg_lo, reg_hi, &bar, &offset, + ®_type); + + dev_dbg(dev, "Found register block in bar %u @ 0x%llx of type %u\n", + bar, offset, reg_type); if (reg_type == CXL_REGLOC_RBI_MEMDEV) { - base = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi); + base = cxl_mem_map_regblock(cxlm, bar, offset); if (IS_ERR(base)) return PTR_ERR(base); break; -- 2.28.0.rc0.12.gb6a658bd00c9 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/4] cxl/mem: Map registers based on capabilities 2021-05-06 22:36 [PATCH 0/4] Map register blocks individually ira.weiny ` (2 preceding siblings ...) 2021-05-06 22:36 ` [PATCH 3/4] cxl/mem: Introduce cxl_decode_register_block() ira.weiny @ 2021-05-06 22:36 ` ira.weiny 2021-05-20 0:24 ` [PATCH 0/4] Map register blocks individually Dan Williams 4 siblings, 0 replies; 10+ messages in thread From: ira.weiny @ 2021-05-06 22:36 UTC (permalink / raw) To: Ben Widawsky, Dan Williams Cc: Ira Weiny, Alison Schofield, Vishal Verma, Jonathan Cameron, linux-cxl, linux-kernel From: Ira Weiny <ira.weiny@intel.com> The information required to map registers based on capabilities is contained within the bars themselves. This means the bar must be mapped to read the information needed and then unmapped to map the individual parts of the BAR based on capabilities. Change cxl_setup_device_regs() to return a register map, change the name to cxl_probe_device_regs(). Allocate and place these register maps on a list while processing each register block. After probing all blocks go back to map the individual registers blocks as specified in the map and free the maps created. Signed-off-by: Ira Weiny <ira.weiny@intel.com> --- drivers/cxl/core.c | 73 ++++++++++++++++++++++------- drivers/cxl/cxl.h | 33 ++++++++++++-- drivers/cxl/pci.c | 111 ++++++++++++++++++++++++++++++++++++--------- 3 files changed, 175 insertions(+), 42 deletions(-) diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c index 21553386e218..b8c7ca9d3203 100644 --- a/drivers/cxl/core.c +++ b/drivers/cxl/core.c @@ -4,6 +4,7 @@ #include <linux/module.h> #include <linux/slab.h> #include <linux/idr.h> +#include <linux/pci.h> #include "cxl.h" /** @@ -479,18 +480,13 @@ struct cxl_port *devm_cxl_add_port(struct device *host, } EXPORT_SYMBOL_GPL(devm_cxl_add_port); -/* - * cxl_setup_device_regs() - Detect CXL Device register blocks - * @dev: Host device of the @base mapping - * @base: mapping of CXL 2.0 8.2.8 CXL Device Register Interface - */ -void cxl_setup_device_regs(struct device *dev, void __iomem *base, - struct cxl_device_regs *regs) +void cxl_probe_device_regs(struct device *dev, void __iomem *base, + struct cxl_device_reg_map *map) { int cap, cap_count; u64 cap_array; - *regs = (struct cxl_device_regs) { 0 }; + *map = (struct cxl_device_reg_map){ 0 }; cap_array = readq(base + CXLDEV_CAP_ARRAY_OFFSET); if (FIELD_GET(CXLDEV_CAP_ARRAY_ID_MASK, cap_array) != @@ -500,8 +496,7 @@ void cxl_setup_device_regs(struct device *dev, void __iomem *base, cap_count = FIELD_GET(CXLDEV_CAP_ARRAY_COUNT_MASK, cap_array); for (cap = 1; cap <= cap_count; cap++) { - void __iomem *register_block; - u32 hdr, offset, __maybe_unused length; + u32 hdr, offset, length; u16 cap_id; u8 version; @@ -516,23 +511,28 @@ void cxl_setup_device_regs(struct device *dev, void __iomem *base, offset = readl(base + cap * 0x10 + 0x4); length = readl(base + cap * 0x10 + 0x8); - register_block = base + offset; - switch (cap_id) { case CXLDEV_CAP_CAP_ID_DEVICE_STATUS: dev_dbg(dev, "found Status capability (0x%x)\n", offset); - regs->status = register_block; + + map->status.valid = true; + map->status.offset = offset; + map->status.size = length; break; case CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX: dev_dbg(dev, "found Mailbox capability (0x%x)\n", offset); - regs->mbox = register_block; + map->mbox.valid = true; + map->mbox.offset = offset; + map->mbox.size = length; break; case CXLDEV_CAP_CAP_ID_SECONDARY_MAILBOX: dev_dbg(dev, "found Secondary Mailbox capability (0x%x)\n", offset); break; case CXLDEV_CAP_CAP_ID_MEMDEV: dev_dbg(dev, "found Memory Device capability (0x%x)\n", offset); - regs->memdev = register_block; + map->memdev.valid = true; + map->memdev.offset = offset; + map->memdev.size = length; break; default: if (cap_id > 0x8000) @@ -543,7 +543,48 @@ void cxl_setup_device_regs(struct device *dev, void __iomem *base, } } } -EXPORT_SYMBOL_GPL(cxl_setup_device_regs); +EXPORT_SYMBOL_GPL(cxl_probe_device_regs); + +int cxl_map_device_regs(struct pci_dev *pdev, + struct cxl_device_regs *regs, + struct cxl_register_map *map) +{ + struct device *dev = &pdev->dev; + resource_size_t phys_addr; + + phys_addr = pci_resource_start(pdev, map->barno); + phys_addr += map->block_offset; + + if (map->device_map.status.valid) { + resource_size_t addr; + resource_size_t length; + + addr = phys_addr + map->device_map.status.offset; + length = map->device_map.status.size; + regs->status = devm_ioremap(dev, addr, length); + } + + if (map->device_map.mbox.valid) { + resource_size_t addr; + resource_size_t length; + + addr = phys_addr + map->device_map.mbox.offset; + length = map->device_map.mbox.size; + regs->mbox = devm_ioremap(dev, addr, length); + } + + if (map->device_map.memdev.valid) { + resource_size_t addr; + resource_size_t length; + + addr = phys_addr + map->device_map.memdev.offset; + length = map->device_map.memdev.size; + regs->memdev = devm_ioremap(dev, addr, length); + } + + return 0; +} +EXPORT_SYMBOL_GPL(cxl_map_device_regs); struct bus_type cxl_bus_type = { .name = "cxl", diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 9b315c069557..afc18ee89795 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -55,9 +55,7 @@ struct cxl_device_regs { /* * Note, the anonymous union organization allows for per * register-block-type helper routines, without requiring block-type - * agnostic code to include the prefix. I.e. - * cxl_setup_device_regs(&cxlm->regs.dev) vs readl(cxlm->regs.mbox). - * The specificity reads naturally from left-to-right. + * agnostic code to include the prefix. */ struct cxl_regs { union { @@ -68,8 +66,33 @@ struct cxl_regs { }; }; -void cxl_setup_device_regs(struct device *dev, void __iomem *base, - struct cxl_device_regs *regs); +struct cxl_reg_map { + bool valid; + unsigned long offset; + unsigned long size; +}; + +struct cxl_device_reg_map { + struct cxl_reg_map status; + struct cxl_reg_map mbox; + struct cxl_reg_map memdev; +}; + +struct cxl_register_map { + struct list_head list; + u64 block_offset; + u8 reg_type; + u8 barno; + union { + struct cxl_device_reg_map device_map; + }; +}; + +void cxl_probe_device_regs(struct device *dev, void __iomem *base, + struct cxl_device_reg_map *map); +int cxl_map_device_regs(struct pci_dev *pdev, + struct cxl_device_regs *regs, + struct cxl_register_map *map); /* * Address space properties derived from: diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index cee14de0f251..97361c0e8a32 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -6,6 +6,7 @@ #include <linux/module.h> #include <linux/sizes.h> #include <linux/mutex.h> +#include <linux/list.h> #include <linux/cdev.h> #include <linux/idr.h> #include <linux/pci.h> @@ -936,7 +937,7 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, return (void __iomem *)ERR_PTR(-ENXIO); } - rc = pcim_iomap(pdev, bar, 0); + rc = pci_iomap(pdev, bar, 0); if (!rc) { dev_err(dev, "failed to map registers\n"); return (void __iomem *)ERR_PTR(-ENOMEM); @@ -945,7 +946,12 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ 0x%llx\n", bar, offset); - return pcim_iomap_table(pdev)[bar] + offset; + return rc; +} + +static void cxl_mem_unmap_regblock(struct cxl_mem *cxlm, void __iomem *base) +{ + pci_iounmap(cxlm->pdev, base); } static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec) @@ -971,6 +977,52 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec) return 0; } +static int cxl_probe_regs(struct cxl_mem *cxlm, void __iomem *base, + struct cxl_register_map *map) +{ + struct pci_dev *pdev = cxlm->pdev; + struct device *dev = &pdev->dev; + struct cxl_device_reg_map *dev_map; + + switch (map->reg_type) { + case CXL_REGLOC_RBI_MEMDEV: + dev_map = &map->device_map; + cxl_probe_device_regs(dev, base, dev_map); + if (!dev_map->status.valid || !dev_map->mbox.valid || + !dev_map->memdev.valid) { + dev_err(dev, "registers not found: %s%s%s\n", + !dev_map->status.valid ? "status " : "", + !dev_map->mbox.valid ? "status " : "", + !dev_map->memdev.valid ? "status " : ""); + return -ENXIO; + } + + dev_dbg(dev, "Probing device registers...\n"); + break; + default: + break; + } + + return 0; +} + +static int cxl_map_regs(struct cxl_mem *cxlm, struct cxl_register_map *map) +{ + struct pci_dev *pdev = cxlm->pdev; + struct device *dev = &pdev->dev; + + switch (map->reg_type) { + case CXL_REGLOC_RBI_MEMDEV: + cxl_map_device_regs(pdev, &cxlm->regs.device_regs, map); + dev_dbg(dev, "Probing device registers...\n"); + break; + default: + break; + } + + return 0; +} + static void cxl_decode_register_block(u32 reg_lo, u32 reg_hi, u8 *bar, u64 *offset, u8 *reg_type) { @@ -991,12 +1043,14 @@ static void cxl_decode_register_block(u32 reg_lo, u32 reg_hi, */ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) { - struct cxl_regs *regs = &cxlm->regs; struct pci_dev *pdev = cxlm->pdev; struct device *dev = &pdev->dev; u32 regloc_size, regblocks; void __iomem *base; int regloc, i; + struct cxl_register_map *map, *n; + LIST_HEAD(register_maps); + int ret = 0; regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_OFFSET); if (!regloc) { @@ -1020,7 +1074,14 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) u64 offset; u8 bar; - /* "register low and high" contain other bits */ + map = kzalloc(sizeof(*map), GFP_KERNEL); + if (!map) { + ret = -ENOMEM; + goto free_maps; + } + + list_add(&map->list, ®ister_maps); + pci_read_config_dword(pdev, regloc, ®_lo); pci_read_config_dword(pdev, regloc + 4, ®_hi); @@ -1030,30 +1091,38 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) dev_dbg(dev, "Found register block in bar %u @ 0x%llx of type %u\n", bar, offset, reg_type); - if (reg_type == CXL_REGLOC_RBI_MEMDEV) { - base = cxl_mem_map_regblock(cxlm, bar, offset); - if (IS_ERR(base)) - return PTR_ERR(base); - break; + base = cxl_mem_map_regblock(cxlm, bar, offset); + if (IS_ERR(base)) { + ret = PTR_ERR(base); + goto free_maps; } - } - if (i == regblocks) { - dev_err(dev, "Missing register locator for device registers\n"); - return -ENXIO; + map->barno = bar; + map->block_offset = offset; + map->reg_type = reg_type; + + ret = cxl_probe_regs(cxlm, base + offset, map); + + /* Always unmap the regblock regardless of probe success */ + cxl_mem_unmap_regblock(cxlm, base); + + if (ret) + goto free_maps; } - cxl_setup_device_regs(dev, base, ®s->device_regs); + list_for_each_entry(map, ®ister_maps, list) { + ret = cxl_map_regs(cxlm, map); + if (ret) + goto free_maps; + } - if (!regs->status || !regs->mbox || !regs->memdev) { - dev_err(dev, "registers not found: %s%s%s\n", - !regs->status ? "status " : "", - !regs->mbox ? "mbox " : "", - !regs->memdev ? "memdev" : ""); - return -ENXIO; +free_maps: + list_for_each_entry_safe(map, n, ®ister_maps, list) { + list_del(&map->list); + kfree(map); } - return 0; + return ret; } static struct cxl_memdev *to_cxl_memdev(struct device *dev) -- 2.28.0.rc0.12.gb6a658bd00c9 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/4] Map register blocks individually 2021-05-06 22:36 [PATCH 0/4] Map register blocks individually ira.weiny ` (3 preceding siblings ...) 2021-05-06 22:36 ` [PATCH 4/4] cxl/mem: Map registers based on capabilities ira.weiny @ 2021-05-20 0:24 ` Dan Williams 4 siblings, 0 replies; 10+ messages in thread From: Dan Williams @ 2021-05-20 0:24 UTC (permalink / raw) To: Weiny, Ira Cc: Ben Widawsky, Alison Schofield, Vishal Verma, Jonathan Cameron, linux-cxl, Linux Kernel Mailing List On Thu, May 6, 2021 at 3:37 PM <ira.weiny@intel.com> wrote: > > From: Ira Weiny <ira.weiny@intel.com> > > User space will want to map some register blocks. The motivation is not to allow userspace access. The motivation is a bug fix for hardware implementations that mix component and device registers into the same BAR and the fact that the driver stack has independent mapping implementations for those 2 cases. It is a happy side-effect that this also allows finer grained pci-mmap exclusion. > Currently BARs are mapped in > their entirety and pointers to the register blocks are created into those > mappings. This will prevent mappings from user space. > > This series has 3 clean up patches followed by a patch to mapping the register > blocks individually. > > Unfortunately, the information for the register blocks is contained inside the > BARs themselves. Which means the BAR must be mapped, probed, and unmapped > prior to the registers being mapped individually. > > The probe stage creates list of register maps which is then iterated to map > the individual register blocks. > > Ira Weiny (4): > cxl/mem: Fully decode device capability header > cxl/mem: Reserve all device regions at once > cxl/mem: Introduce cxl_decode_register_block() > cxl/mem: Map registers based on capabilities > > drivers/cxl/core.c | 84 ++++++++++++++++++++------ > drivers/cxl/cxl.h | 34 +++++++++-- > drivers/cxl/pci.c | 147 +++++++++++++++++++++++++++++++++++---------- > 3 files changed, 211 insertions(+), 54 deletions(-) > > -- > 2.28.0.rc0.12.gb6a658bd00c9 > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-05-20 19:44 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-06 22:36 [PATCH 0/4] Map register blocks individually ira.weiny 2021-05-06 22:36 ` [PATCH 1/4] cxl/mem: Fully decode device capability header ira.weiny 2021-05-20 0:50 ` Dan Williams 2021-05-20 17:42 ` Ira Weiny 2021-05-06 22:36 ` [PATCH 2/4] cxl/mem: Reserve all device regions at once ira.weiny 2021-05-20 1:00 ` Dan Williams 2021-05-20 19:44 ` Ira Weiny 2021-05-06 22:36 ` [PATCH 3/4] cxl/mem: Introduce cxl_decode_register_block() ira.weiny 2021-05-06 22:36 ` [PATCH 4/4] cxl/mem: Map registers based on capabilities ira.weiny 2021-05-20 0:24 ` [PATCH 0/4] Map register blocks individually Dan Williams
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).