| Message ID | 20200221032720.33893-1-alastair@au1.ibm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add support for OpenCAPI Persistent Memory devices | expand |
On Thu, Feb 20, 2020 at 7:28 PM Alastair D'Silva <alastair@au1.ibm.com> wrote: > > From: Alastair D'Silva <alastair@d-silva.org> > > This series adds support for OpenCAPI Persistent Memory devices, exposing > them as nvdimms so that we can make use of the existing infrastructure. A single sentence to introduce: 24 files changed, 3029 insertions(+), 97 deletions(-) ...is inadequate. What are OpenCAPI Persistent Memory devices? How do they compare, in terms relevant to libnvdimm, to other persistent memory devices? What challenges do they pose to the existing enabling? What is the overall approach taken with this 27 patch break down? What are the changes since v2, v1? If you incorporated someone's review feedback note it in the cover letter changelog, if you didn't incorporate someone's feedback note that too with an explanation. In short, provide a bridge document for someone familiar with the upstream infrastructure, but not necessarily steeped in powernv / OpenCAPI platform details, to get started with this code. For now, no need to resend the whole series, just reply to this message with a fleshed out cover letter and then incorporate it going forward for v4+.
On Fri, Feb 21, 2020 at 8:21 AM Dan Williams <dan.j.williams@intel.com> wrote: > > On Thu, Feb 20, 2020 at 7:28 PM Alastair D'Silva <alastair@au1.ibm.com> wrote: > > > > From: Alastair D'Silva <alastair@d-silva.org> > > > > This series adds support for OpenCAPI Persistent Memory devices, exposing > > them as nvdimms so that we can make use of the existing infrastructure. > > A single sentence to introduce: > > 24 files changed, 3029 insertions(+), 97 deletions(-) > > ...is inadequate. What are OpenCAPI Persistent Memory devices? How do > they compare, in terms relevant to libnvdimm, to other persistent > memory devices? What challenges do they pose to the existing enabling? > What is the overall approach taken with this 27 patch break down? What > are the changes since v2, v1? If you incorporated someone's review > feedback note it in the cover letter changelog, if you didn't Assumptions and tradeoffs the implementation considered are also critical for reviewing the approach.
On Fri, 2020-02-21 at 08:21 -0800, Dan Williams wrote: > On Thu, Feb 20, 2020 at 7:28 PM Alastair D'Silva < > alastair@au1.ibm.com> wrote: > > From: Alastair D'Silva <alastair@d-silva.org> > > > > This series adds support for OpenCAPI Persistent Memory devices, > > exposing > > them as nvdimms so that we can make use of the existing > > infrastructure. > > A single sentence to introduce: > > 24 files changed, 3029 insertions(+), 97 deletions(-) > > ...is inadequate. What are OpenCAPI Persistent Memory devices? How do > they compare, in terms relevant to libnvdimm, to other persistent > memory devices? What challenges do they pose to the existing > enabling? > What is the overall approach taken with this 27 patch break down? > What > are the changes since v2, v1? If you incorporated someone's review > feedback note it in the cover letter changelog, if you didn't > incorporate someone's feedback note that too with an explanation. > > In short, provide a bridge document for someone familiar with the > upstream infrastructure, but not necessarily steeped in powernv / > OpenCAPI platform details, to get started with this code. > > For now, no need to resend the whole series, just reply to this > message with a fleshed out cover letter and then incorporate it going > forward for v4+. Apologies, I was maintaining a changelog, and forgot to include it. I'll flesh out the cover letter too: This series adds support for OpenCAPI Persistent Memory devices on bare metal (arch/powernv), exposing them as nvdimms so that we can make use of the existing infrastructure. There already exists a driver for the same devices abstracted through PowerVM (arch/pseries): arch/powerpc/platforms/pseries/papr_scm.c These devices are connected via OpenCAPI, and present as LPC (lowest coherence point) memory to the system, practically, that means that memory on these cards could be treated as conventional, cache-coherent memory. Since the devices are connected via OpenCAPI, they are not enumerated via ACPI. Instead, OpenCAPI links present as pseudo-PCI bridges, with devices below them. This series introduces a driver that exposes the memory on these cards as nvdimms, with each card getting it's own bus. This is somewhat complicated by the fact that the cards do not have out of band persistent storage for metadata, so 1 SECTION_SIZE's (see SPARSEMEM) worth of storage is carved out of the top of the card storage to implement the ndctl_config_* calls. The driver is not responsible for configuring the NPU (NVLink Processing Unit) BARs to map the LPC memory from the card into the system's physical address space, instead, it requests this to be done via OPAL calls (typically implemented by Skiboot). The series is structured as follows: - Required infrastructure changes & cleanup - A minimal driver implementation - Implementing additional features within the driver V3: - Rebase against next/next-20200220 - Move driver to arch/powerpc/platforms/powernv, we now expect this driver to go upstream via the powerpc tree - "nvdimm/ocxl: Implement the Read Error Log command" - Fix bad header path - "nvdimm/ocxl: Read the capability registers & wait for device ready" - Fix overlapping masks between readiness_timeout & memory_available_timeout - "nvdimm: Add driver for OpenCAPI Storage Class Memory" - Address minor review comments from Jonathan Cameron - Remove attributes - Default to module if building LIBNVDIMM - Propogate errors up from called functions in probe() - "nvdimm/ocxl: Expose SMART data via ndctl" - Pack attributes in struct - Support different size SMART buffers for compatibility with newer ndctls that may want more SMART attribs than we provide - Rework to to use ND_CMD_CALL instead of ND_CMD_SMART - drop "ocxl: Free detached contexts in ocxl_context_detach_all()" - "powerpc: Map & release OpenCAPI LPC memory" - Remove 'extern' - Only available with CONFIG_MEMORY_HOTPLUG_SPARSE - "ocxl: Tally up the LPC memory on a link & allow it to be mapped" - Address minor review comments from Jonathan Cameron - "ocxl: Add functions to map/unmap LPC memory" - Split detected memory message into a separate patch - Address minor review comments from Jonathan Cameron - Add a comment explaining why unmap_lpc_mem is in deconfigure_afu - "nvdimm/ocxl: Add support for Admin commands" - use sizeof(u64) rather than 0x08 when iterating u64s - "nvdimm/ocxl: Implement the heartbeat command" - Fix typo in blurb - Address kernel doc issues - Ensure all uapi headers use C89 compatible comments - Drop patches for firmware update & overwrite, these will be submitted later once patches are available for ndctl - Rename SCM to OpenCAPI Persistent Memory V2: - "powerpc: Map & release OpenCAPI LPC memory" - Fix #if -> #ifdef - use pci_dev_id to get the bdfn - use __be64 to hold be data - indent check_hotplug_memory_addressable correctly - Remove export of check_hotplug_memory_addressable - "ocxl: Conditionally bind SCM devices to the generic OCXL driver" - Improve patch description and remove redundant default - "nvdimm: Add driver for OpenCAPI Storage Class Memory" - Mark a few funcs as static as identified by the 0day bot - Add OCXL dependancies to OCXL_SCM - Use memcpy_mcsafe in scm_ndctl_config_read - Rename scm_foo_offset_0x00 to scm_foo_header_parse & add docs - Name DIMM attribs "ocxl" rather than "scm" - Split out into base + many feature patches - "powerpc: Enable OpenCAPI Storage Class Memory driver on bare metal" - Build DEV_DAX & friends as modules - "ocxl: Conditionally bind SCM devices to the generic OCXL driver" - Patch dropped (easy enough to maintain this out of tree for development) - "ocxl: Tally up the LPC memory on a link & allow it to be mapped" - Add a warning if an unmatched lpc_release is called - "ocxl: Add functions to map/unmap LPC memory" - Use EXPORT_SYMBOL_GPL
On Mon, Feb 24, 2020 at 03:34:07PM +1100, Alastair D'Silva wrote: > V3: > - Rebase against next/next-20200220 > - Move driver to arch/powerpc/platforms/powernv, we now expect this > driver to go upstream via the powerpc tree That's rather the opposite direction of normal; mostly drivers live under drivers/ and not in arch/. It's easier for drivers to get overlooked when doing tree-wide changes if they're hiding.
On Sun, 2020-02-23 at 20:37 -0800, Matthew Wilcox wrote: > On Mon, Feb 24, 2020 at 03:34:07PM +1100, Alastair D'Silva wrote: > > V3: > > - Rebase against next/next-20200220 > > - Move driver to arch/powerpc/platforms/powernv, we now expect > > this > > driver to go upstream via the powerpc tree > > That's rather the opposite direction of normal; mostly drivers live > under > drivers/ and not in arch/. It's easier for drivers to get overlooked > when doing tree-wide changes if they're hiding. This is true, however, given that it was not all that desirable to have it under drivers/nvdimm, it's sister driver (for the same hardware) is also under arch, and that we don't expect this driver to be used on any platform other than powernv, we think this was the most reasonable place to put it.
On Mon, Feb 24, 2020 at 3:43 PM Alastair D'Silva <alastair@au1.ibm.com> wrote: > > On Sun, 2020-02-23 at 20:37 -0800, Matthew Wilcox wrote: > > On Mon, Feb 24, 2020 at 03:34:07PM +1100, Alastair D'Silva wrote: > > > V3: > > > - Rebase against next/next-20200220 > > > - Move driver to arch/powerpc/platforms/powernv, we now expect > > > this > > > driver to go upstream via the powerpc tree > > > > That's rather the opposite direction of normal; mostly drivers live > > under > > drivers/ and not in arch/. It's easier for drivers to get overlooked > > when doing tree-wide changes if they're hiding. > > This is true, however, given that it was not all that desirable to have > it under drivers/nvdimm, it's sister driver (for the same hardware) is > also under arch, and that we don't expect this driver to be used on any > platform other than powernv, we think this was the most reasonable > place to put it. Historically powernv specific platform drivers go in their respective subsystem trees rather than in arch/ and I'd prefer we kept it that way. When I added the papr_scm driver I put it in the pseries platform directory because most of the pseries paravirt code lives there for some reason; I don't know why. Luckily for me that followed the same model that Dan used when he put the NFIT driver in drivers/acpi/ and the libnvdimm core in drivers/nvdimm/ so we didn't have anything to argue about. However, as Matthew pointed out, it is at odds with how most subsystems operate. Is there any particular reason we're doing things this way or should we think about moving libnvdimm users to drivers/nvdimm/? Oliver
On Mon, 2020-02-24 at 17:51 +1100, Oliver O'Halloran wrote: > On Mon, Feb 24, 2020 at 3:43 PM Alastair D'Silva < > alastair@au1.ibm.com> wrote: > > On Sun, 2020-02-23 at 20:37 -0800, Matthew Wilcox wrote: > > > On Mon, Feb 24, 2020 at 03:34:07PM +1100, Alastair D'Silva wrote: > > > > V3: > > > > - Rebase against next/next-20200220 > > > > - Move driver to arch/powerpc/platforms/powernv, we now > > > > expect > > > > this > > > > driver to go upstream via the powerpc tree > > > > > > That's rather the opposite direction of normal; mostly drivers > > > live > > > under > > > drivers/ and not in arch/. It's easier for drivers to get > > > overlooked > > > when doing tree-wide changes if they're hiding. > > > > This is true, however, given that it was not all that desirable to > > have > > it under drivers/nvdimm, it's sister driver (for the same hardware) > > is > > also under arch, and that we don't expect this driver to be used on > > any > > platform other than powernv, we think this was the most reasonable > > place to put it. > > Historically powernv specific platform drivers go in their respective > subsystem trees rather than in arch/ and I'd prefer we kept it that > way. When I added the papr_scm driver I put it in the pseries > platform > directory because most of the pseries paravirt code lives there for > some reason; I don't know why. Luckily for me that followed the same > model that Dan used when he put the NFIT driver in drivers/acpi/ and > the libnvdimm core in drivers/nvdimm/ so we didn't have anything to > argue about. However, as Matthew pointed out, it is at odds with how > most subsystems operate. Is there any particular reason we're doing > things this way or should we think about moving libnvdimm users to > drivers/nvdimm/? > > Oliver I'm not too fussed where it ends up, as long as it ends up somewhere :) From what I can tell, the issue is that we have both "infrastructure" drivers, and end-device drivers. To me, it feels like drivers/nvdimm should contain both, and I think this feels like the right approach. I could move it back to drivers/nvdimm/ocxl, but I felt that it was only tolerated there, not desired. This could be cleared up with a response from Dan Williams, and if it is indeed dersired, this is my preferred location. I think a case could also be made for drivers/ocxl, simply because we don't expect more than a handful of drivers to ever live there (I expect most users will drive their devices from userspace via libocxl). In defence of keeping it in arch/powerpc/powernv, I highly doubt this driver will end up being used on any platform other than this. Even though OpenCAPI was engineered as an open standard, there is some competition from industry giants with a competing standard on a much more popular platform.
On Tue, Feb 25, 2020 at 4:14 PM Alastair D'Silva <alastair@au1.ibm.com> wrote: > > On Mon, 2020-02-24 at 17:51 +1100, Oliver O'Halloran wrote: > > On Mon, Feb 24, 2020 at 3:43 PM Alastair D'Silva < > > alastair@au1.ibm.com> wrote: > > > On Sun, 2020-02-23 at 20:37 -0800, Matthew Wilcox wrote: > > > > On Mon, Feb 24, 2020 at 03:34:07PM +1100, Alastair D'Silva wrote: > > > > > V3: > > > > > - Rebase against next/next-20200220 > > > > > - Move driver to arch/powerpc/platforms/powernv, we now > > > > > expect > > > > > this > > > > > driver to go upstream via the powerpc tree > > > > > > > > That's rather the opposite direction of normal; mostly drivers > > > > live > > > > under > > > > drivers/ and not in arch/. It's easier for drivers to get > > > > overlooked > > > > when doing tree-wide changes if they're hiding. > > > > > > This is true, however, given that it was not all that desirable to > > > have > > > it under drivers/nvdimm, it's sister driver (for the same hardware) > > > is > > > also under arch, and that we don't expect this driver to be used on > > > any > > > platform other than powernv, we think this was the most reasonable > > > place to put it. > > > > Historically powernv specific platform drivers go in their respective > > subsystem trees rather than in arch/ and I'd prefer we kept it that > > way. When I added the papr_scm driver I put it in the pseries > > platform > > directory because most of the pseries paravirt code lives there for > > some reason; I don't know why. Luckily for me that followed the same > > model that Dan used when he put the NFIT driver in drivers/acpi/ and > > the libnvdimm core in drivers/nvdimm/ so we didn't have anything to > > argue about. However, as Matthew pointed out, it is at odds with how > > most subsystems operate. Is there any particular reason we're doing > > things this way or should we think about moving libnvdimm users to > > drivers/nvdimm/? > > > > Oliver > > > I'm not too fussed where it ends up, as long as it ends up somewhere :) > > From what I can tell, the issue is that we have both "infrastructure" > drivers, and end-device drivers. To me, it feels like drivers/nvdimm > should contain both, and I think this feels like the right approach. > > I could move it back to drivers/nvdimm/ocxl, but I felt that it was > only tolerated there, not desired. This could be cleared up with a > response from Dan Williams, and if it is indeed dersired, this is my > preferred location. Apologies if I gave the impression it was only tolerated. I'm ok with drivers/nvdimm/ocxl/, and to the larger point I'd also be ok with a drivers/{acpi => nvdimm}/nfit and {arch/powerpc/platforms/pseries => drivers/nvdimm}/papr_scm.c move as well to keep all the consumers of the nvdimm related code together with the core.
On Tue, 2020-02-25 at 16:32 -0800, Dan Williams wrote: > On Tue, Feb 25, 2020 at 4:14 PM Alastair D'Silva < > alastair@au1.ibm.com> wrote: > > On Mon, 2020-02-24 at 17:51 +1100, Oliver O'Halloran wrote: > > > On Mon, Feb 24, 2020 at 3:43 PM Alastair D'Silva < > > > alastair@au1.ibm.com> wrote: > > > > On Sun, 2020-02-23 at 20:37 -0800, Matthew Wilcox wrote: > > > > > On Mon, Feb 24, 2020 at 03:34:07PM +1100, Alastair D'Silva > > > > > wrote: > > > > > > V3: > > > > > > - Rebase against next/next-20200220 > > > > > > - Move driver to arch/powerpc/platforms/powernv, we now > > > > > > expect > > > > > > this > > > > > > driver to go upstream via the powerpc tree > > > > > > > > > > That's rather the opposite direction of normal; mostly > > > > > drivers > > > > > live > > > > > under > > > > > drivers/ and not in arch/. It's easier for drivers to get > > > > > overlooked > > > > > when doing tree-wide changes if they're hiding. > > > > > > > > This is true, however, given that it was not all that desirable > > > > to > > > > have > > > > it under drivers/nvdimm, it's sister driver (for the same > > > > hardware) > > > > is > > > > also under arch, and that we don't expect this driver to be > > > > used on > > > > any > > > > platform other than powernv, we think this was the most > > > > reasonable > > > > place to put it. > > > > > > Historically powernv specific platform drivers go in their > > > respective > > > subsystem trees rather than in arch/ and I'd prefer we kept it > > > that > > > way. When I added the papr_scm driver I put it in the pseries > > > platform > > > directory because most of the pseries paravirt code lives there > > > for > > > some reason; I don't know why. Luckily for me that followed the > > > same > > > model that Dan used when he put the NFIT driver in drivers/acpi/ > > > and > > > the libnvdimm core in drivers/nvdimm/ so we didn't have anything > > > to > > > argue about. However, as Matthew pointed out, it is at odds with > > > how > > > most subsystems operate. Is there any particular reason we're > > > doing > > > things this way or should we think about moving libnvdimm users > > > to > > > drivers/nvdimm/? > > > > > > Oliver > > > > I'm not too fussed where it ends up, as long as it ends up > > somewhere :) > > > > From what I can tell, the issue is that we have both > > "infrastructure" > > drivers, and end-device drivers. To me, it feels like > > drivers/nvdimm > > should contain both, and I think this feels like the right > > approach. > > > > I could move it back to drivers/nvdimm/ocxl, but I felt that it was > > only tolerated there, not desired. This could be cleared up with a > > response from Dan Williams, and if it is indeed dersired, this is > > my > > preferred location. > > Apologies if I gave the impression it was only tolerated. I'm ok with > drivers/nvdimm/ocxl/, and to the larger point I'd also be ok with a > drivers/{acpi => nvdimm}/nfit and {arch/powerpc/platforms/pseries => > drivers/nvdimm}/papr_scm.c move as well to keep all the consumers of > the nvdimm related code together with the core. Great, thanks for clarifying, text is so imprecise when it comes to nuance :) I'll move ti back to drivers/nvdimm/ocxl then.
From: Alastair D'Silva <alastair@d-silva.org> This series adds support for OpenCAPI Persistent Memory devices, exposing them as nvdimms so that we can make use of the existing infrastructure. Alastair D'Silva (27): powerpc: Add OPAL calls for LPC memory alloc/release mm/memory_hotplug: Allow check_hotplug_memory_addressable to be called from drivers powerpc: Map & release OpenCAPI LPC memory ocxl: Remove unnecessary externs ocxl: Address kernel doc errors & warnings ocxl: Tally up the LPC memory on a link & allow it to be mapped ocxl: Add functions to map/unmap LPC memory ocxl: Emit a log message showing how much LPC memory was detected ocxl: Save the device serial number in ocxl_fn powerpc: Add driver for OpenCAPI Persistent Memory powerpc: Enable the OpenCAPI Persistent Memory driver for powernv_defconfig powerpc/powernv/pmem: Add register addresses & status values to the header powerpc/powernv/pmem: Read the capability registers & wait for device ready powerpc/powernv/pmem: Add support for Admin commands powerpc/powernv/pmem: Add support for near storage commands powerpc/powernv/pmem: Register a character device for userspace to interact with powerpc/powernv/pmem: Implement the Read Error Log command powerpc/powernv/pmem: Add controller dump IOCTLs powerpc/powernv/pmem: Add an IOCTL to report controller statistics powerpc/powernv/pmem: Forward events to userspace powerpc/powernv/pmem: Add an IOCTL to request controller health & perf data powerpc/powernv/pmem: Implement the heartbeat command powerpc/powernv/pmem: Add debug IOCTLs powerpc/powernv/pmem: Expose SMART data via ndctl powerpc/powernv/pmem: Expose the serial number in sysfs powerpc/powernv/pmem: Expose the firmware version in sysfs MAINTAINERS: Add myself & nvdimm/ocxl to ocxl MAINTAINERS | 3 + arch/powerpc/configs/powernv_defconfig | 5 + arch/powerpc/include/asm/opal-api.h | 2 + arch/powerpc/include/asm/opal.h | 3 + arch/powerpc/include/asm/pnv-ocxl.h | 40 +- arch/powerpc/platforms/powernv/Kconfig | 3 + arch/powerpc/platforms/powernv/Makefile | 1 + arch/powerpc/platforms/powernv/ocxl.c | 43 + arch/powerpc/platforms/powernv/opal-call.c | 2 + arch/powerpc/platforms/powernv/pmem/Kconfig | 21 + arch/powerpc/platforms/powernv/pmem/Makefile | 7 + arch/powerpc/platforms/powernv/pmem/ocxl.c | 1991 +++++++++++++++++ .../platforms/powernv/pmem/ocxl_internal.c | 213 ++ .../platforms/powernv/pmem/ocxl_internal.h | 254 +++ .../platforms/powernv/pmem/ocxl_sysfs.c | 46 + drivers/misc/ocxl/config.c | 74 +- drivers/misc/ocxl/core.c | 61 + drivers/misc/ocxl/link.c | 53 + drivers/misc/ocxl/ocxl_internal.h | 45 +- include/linux/memory_hotplug.h | 5 + include/misc/ocxl.h | 122 +- include/uapi/linux/ndctl.h | 1 + include/uapi/nvdimm/ocxl-pmem.h | 127 ++ mm/memory_hotplug.c | 4 +- 24 files changed, 3029 insertions(+), 97 deletions(-) create mode 100644 arch/powerpc/platforms/powernv/pmem/Kconfig create mode 100644 arch/powerpc/platforms/powernv/pmem/Makefile create mode 100644 arch/powerpc/platforms/powernv/pmem/ocxl.c create mode 100644 arch/powerpc/platforms/powernv/pmem/ocxl_internal.c create mode 100644 arch/powerpc/platforms/powernv/pmem/ocxl_internal.h create mode 100644 arch/powerpc/platforms/powernv/pmem/ocxl_sysfs.c create mode 100644 include/uapi/nvdimm/ocxl-pmem.h