* [PATCH v3 3/7] powerpc/mm/ptdump: debugfs handler for W+X checks at runtime
From: Christophe Leroy @ 2020-02-03 7:11 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, ruscur
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <80ebd9075cd7c8b412c6d5d05f7542f9026642ef.1580713729.git.christophe.leroy@c-s.fr>
From: Russell Currey <ruscur@russell.cc>
Very rudimentary, just
echo 1 > [debugfs]/check_wx_pages
and check the kernel log. Useful for testing strict module RWX.
Updated the Kconfig entry to reflect this.
Also fixed a typo.
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
v3: no change
v2: no change
---
arch/powerpc/Kconfig.debug | 6 ++++--
arch/powerpc/mm/ptdump/ptdump.c | 21 ++++++++++++++++++++-
2 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
index 0b063830eea8..e37960ef68c6 100644
--- a/arch/powerpc/Kconfig.debug
+++ b/arch/powerpc/Kconfig.debug
@@ -370,7 +370,7 @@ config PPC_PTDUMP
If you are unsure, say N.
config PPC_DEBUG_WX
- bool "Warn on W+X mappings at boot"
+ bool "Warn on W+X mappings at boot & enable manual checks at runtime"
depends on PPC_PTDUMP && STRICT_KERNEL_RWX
help
Generate a warning if any W+X mappings are found at boot.
@@ -384,7 +384,9 @@ config PPC_DEBUG_WX
of other unfixed kernel bugs easier.
There is no runtime or memory usage effect of this option
- once the kernel has booted up - it's a one time check.
+ once the kernel has booted up, it only automatically checks once.
+
+ Enables the "check_wx_pages" debugfs entry for checking at runtime.
If in doubt, say "Y".
diff --git a/arch/powerpc/mm/ptdump/ptdump.c b/arch/powerpc/mm/ptdump/ptdump.c
index 206156255247..a15e19a3b14e 100644
--- a/arch/powerpc/mm/ptdump/ptdump.c
+++ b/arch/powerpc/mm/ptdump/ptdump.c
@@ -4,7 +4,7 @@
*
* This traverses the kernel pagetables and dumps the
* information about the used sections of memory to
- * /sys/kernel/debug/kernel_pagetables.
+ * /sys/kernel/debug/kernel_page_tables.
*
* Derived from the arm64 implementation:
* Copyright (c) 2014, The Linux Foundation, Laura Abbott.
@@ -413,6 +413,25 @@ void ptdump_check_wx(void)
else
pr_info("Checked W+X mappings: passed, no W+X pages found\n");
}
+
+static int check_wx_debugfs_set(void *data, u64 val)
+{
+ if (val != 1ULL)
+ return -EINVAL;
+
+ ptdump_check_wx();
+
+ return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(check_wx_fops, NULL, check_wx_debugfs_set, "%llu\n");
+
+static int ptdump_check_wx_init(void)
+{
+ return debugfs_create_file("check_wx_pages", 0200, NULL,
+ NULL, &check_wx_fops) ? 0 : -ENOMEM;
+}
+device_initcall(ptdump_check_wx_init);
#endif
static int ptdump_init(void)
--
2.25.0
^ permalink raw reply related
* [PATCH v3 4/7] powerpc: Set ARCH_HAS_STRICT_MODULE_RWX
From: Christophe Leroy @ 2020-02-03 7:11 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, ruscur
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <80ebd9075cd7c8b412c6d5d05f7542f9026642ef.1580713729.git.christophe.leroy@c-s.fr>
From: Russell Currey <ruscur@russell.cc>
To enable strict module RWX on powerpc, set:
CONFIG_STRICT_MODULE_RWX=y
You should also have CONFIG_STRICT_KERNEL_RWX=y set to have any real
security benefit.
ARCH_HAS_STRICT_MODULE_RWX is set to require ARCH_HAS_STRICT_KERNEL_RWX.
This is due to a quirk in arch/Kconfig and arch/powerpc/Kconfig that
makes STRICT_MODULE_RWX *on by default* in configurations where
STRICT_KERNEL_RWX is *unavailable*.
Since this doesn't make much sense, and module RWX without kernel RWX
doesn't make much sense, having the same dependencies as kernel RWX
works around this problem.
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
v3: no change
v2: no change
---
arch/powerpc/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index d0c6e7b7a62d..1a26b8526be0 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -131,6 +131,7 @@ config PPC
select ARCH_HAS_SCALED_CPUTIME if VIRT_CPU_ACCOUNTING_NATIVE && PPC_BOOK3S_64
select ARCH_HAS_SET_MEMORY
select ARCH_HAS_STRICT_KERNEL_RWX if ((PPC_BOOK3S_64 || PPC32) && !HIBERNATION)
+ select ARCH_HAS_STRICT_MODULE_RWX if ARCH_HAS_STRICT_KERNEL_RWX
select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
select ARCH_HAS_UACCESS_FLUSHCACHE
select ARCH_HAS_UACCESS_MCSAFE if PPC64
--
2.25.0
^ permalink raw reply related
* [PATCH v3 5/7] powerpc/configs: Enable STRICT_MODULE_RWX in skiroot_defconfig
From: Christophe Leroy @ 2020-02-03 7:12 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, ruscur
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <80ebd9075cd7c8b412c6d5d05f7542f9026642ef.1580713729.git.christophe.leroy@c-s.fr>
From: Russell Currey <ruscur@russell.cc>
skiroot_defconfig is the only powerpc defconfig with STRICT_KERNEL_RWX
enabled, and if you want memory protection for kernel text you'd want it
for modules too, so enable STRICT_MODULE_RWX there.
Acked-by: Joel Stanley <joel@joel.id.au>
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
v3: no change
v2: no change
---
arch/powerpc/configs/skiroot_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/configs/skiroot_defconfig b/arch/powerpc/configs/skiroot_defconfig
index 1b6bdad36b13..66d20dbe67b7 100644
--- a/arch/powerpc/configs/skiroot_defconfig
+++ b/arch/powerpc/configs/skiroot_defconfig
@@ -51,6 +51,7 @@ CONFIG_CMDLINE="console=tty0 console=hvc0 ipr.fast_reboot=1 quiet"
# CONFIG_PPC_MEM_KEYS is not set
CONFIG_JUMP_LABEL=y
CONFIG_STRICT_KERNEL_RWX=y
+CONFIG_STRICT_MODULE_RWX=y
CONFIG_MODULES=y
CONFIG_MODULE_UNLOAD=y
CONFIG_MODULE_SIG_FORCE=y
--
2.25.0
^ permalink raw reply related
* [PATCH v3 7/7] powerpc/32: use set_memory_attr()
From: Christophe Leroy @ 2020-02-03 7:12 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, ruscur
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <80ebd9075cd7c8b412c6d5d05f7542f9026642ef.1580713729.git.christophe.leroy@c-s.fr>
Use set_memory_attr() instead of the PPC32 specific change_page_attr()
change_page_attr() was checking that the address was not mapped by
blocks and was handling highmem, but that's unneeded because the
affected pages can't be in highmem and block mapping verification
is already done by the callers.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v3: no change
v2: new
---
arch/powerpc/mm/pgtable_32.c | 95 ++++--------------------------------
1 file changed, 10 insertions(+), 85 deletions(-)
diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
index 5fb90edd865e..3d92eaf3ee2f 100644
--- a/arch/powerpc/mm/pgtable_32.c
+++ b/arch/powerpc/mm/pgtable_32.c
@@ -23,6 +23,7 @@
#include <linux/highmem.h>
#include <linux/memblock.h>
#include <linux/slab.h>
+#include <linux/set_memory.h>
#include <asm/pgtable.h>
#include <asm/pgalloc.h>
@@ -121,99 +122,20 @@ void __init mapin_ram(void)
}
}
-/* Scan the real Linux page tables and return a PTE pointer for
- * a virtual address in a context.
- * Returns true (1) if PTE was found, zero otherwise. The pointer to
- * the PTE pointer is unmodified if PTE is not found.
- */
-static int
-get_pteptr(struct mm_struct *mm, unsigned long addr, pte_t **ptep, pmd_t **pmdp)
-{
- pgd_t *pgd;
- pud_t *pud;
- pmd_t *pmd;
- pte_t *pte;
- int retval = 0;
-
- pgd = pgd_offset(mm, addr & PAGE_MASK);
- if (pgd) {
- pud = pud_offset(pgd, addr & PAGE_MASK);
- if (pud && pud_present(*pud)) {
- pmd = pmd_offset(pud, addr & PAGE_MASK);
- if (pmd_present(*pmd)) {
- pte = pte_offset_map(pmd, addr & PAGE_MASK);
- if (pte) {
- retval = 1;
- *ptep = pte;
- if (pmdp)
- *pmdp = pmd;
- /* XXX caller needs to do pte_unmap, yuck */
- }
- }
- }
- }
- return(retval);
-}
-
-static int __change_page_attr_noflush(struct page *page, pgprot_t prot)
-{
- pte_t *kpte;
- pmd_t *kpmd;
- unsigned long address;
-
- BUG_ON(PageHighMem(page));
- address = (unsigned long)page_address(page);
-
- if (v_block_mapped(address))
- return 0;
- if (!get_pteptr(&init_mm, address, &kpte, &kpmd))
- return -EINVAL;
- __set_pte_at(&init_mm, address, kpte, mk_pte(page, prot), 0);
- pte_unmap(kpte);
-
- return 0;
-}
-
-/*
- * Change the page attributes of an page in the linear mapping.
- *
- * THIS DOES NOTHING WITH BAT MAPPINGS, DEBUG USE ONLY
- */
-static int change_page_attr(struct page *page, int numpages, pgprot_t prot)
-{
- int i, err = 0;
- unsigned long flags;
- struct page *start = page;
-
- local_irq_save(flags);
- for (i = 0; i < numpages; i++, page++) {
- err = __change_page_attr_noflush(page, prot);
- if (err)
- break;
- }
- wmb();
- local_irq_restore(flags);
- flush_tlb_kernel_range((unsigned long)page_address(start),
- (unsigned long)page_address(page));
- return err;
-}
-
void mark_initmem_nx(void)
{
- struct page *page = virt_to_page(_sinittext);
unsigned long numpages = PFN_UP((unsigned long)_einittext) -
PFN_DOWN((unsigned long)_sinittext);
if (v_block_mapped((unsigned long)_stext + 1))
mmu_mark_initmem_nx();
else
- change_page_attr(page, numpages, PAGE_KERNEL);
+ set_memory_attr((unsigned long)_sinittext, numpages, PAGE_KERNEL);
}
#ifdef CONFIG_STRICT_KERNEL_RWX
void mark_rodata_ro(void)
{
- struct page *page;
unsigned long numpages;
if (v_block_mapped((unsigned long)_sinittext)) {
@@ -222,20 +144,18 @@ void mark_rodata_ro(void)
return;
}
- page = virt_to_page(_stext);
numpages = PFN_UP((unsigned long)_etext) -
PFN_DOWN((unsigned long)_stext);
- change_page_attr(page, numpages, PAGE_KERNEL_ROX);
+ set_memory_attr((unsigned long)_stext, numpages, PAGE_KERNEL_ROX);
/*
* mark .rodata as read only. Use __init_begin rather than __end_rodata
* to cover NOTES and EXCEPTION_TABLE.
*/
- page = virt_to_page(__start_rodata);
numpages = PFN_UP((unsigned long)__init_begin) -
PFN_DOWN((unsigned long)__start_rodata);
- change_page_attr(page, numpages, PAGE_KERNEL_RO);
+ set_memory_attr((unsigned long)__start_rodata, numpages, PAGE_KERNEL_RO);
// mark_initmem_nx() should have already run by now
ptdump_check_wx();
@@ -245,9 +165,14 @@ void mark_rodata_ro(void)
#ifdef CONFIG_DEBUG_PAGEALLOC
void __kernel_map_pages(struct page *page, int numpages, int enable)
{
+ unsigned long addr = (unsigned long)page_address(page);
+
if (PageHighMem(page))
return;
- change_page_attr(page, numpages, enable ? PAGE_KERNEL : __pgprot(0));
+ if (enable)
+ set_memory_attr(addr, numpages, PAGE_KERNEL);
+ else
+ set_memory_attr(addr, numpages, __pgprot(0));
}
#endif /* CONFIG_DEBUG_PAGEALLOC */
--
2.25.0
^ permalink raw reply related
* [PATCH v3 6/7] powerpc/mm: implement set_memory_attr()
From: Christophe Leroy @ 2020-02-03 7:12 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, ruscur
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <80ebd9075cd7c8b412c6d5d05f7542f9026642ef.1580713729.git.christophe.leroy@c-s.fr>
In addition to the set_memory_xx() functions which allows to change
the memory attributes of not (yet) used memory regions, implement a
set_memory_attr() function to:
- set the final memory protection after init on currently used
kernel regions.
- enable/disable kernel memory regions in the scope of DEBUG_PAGEALLOC.
Unlike the set_memory_xx() which can act in three step as the regions
are unused, this function must modify 'on the fly' as the kernel is
executing from them. At the moment only PPC32 will use it and changing
page attributes on the fly is not an issue.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v3: no change
v2: new
---
arch/powerpc/include/asm/set_memory.h | 2 ++
arch/powerpc/mm/pageattr.c | 33 +++++++++++++++++++++++++++
2 files changed, 35 insertions(+)
diff --git a/arch/powerpc/include/asm/set_memory.h b/arch/powerpc/include/asm/set_memory.h
index 64011ea444b4..b040094f7920 100644
--- a/arch/powerpc/include/asm/set_memory.h
+++ b/arch/powerpc/include/asm/set_memory.h
@@ -29,4 +29,6 @@ static inline int set_memory_x(unsigned long addr, int numpages)
return change_memory_attr(addr, numpages, SET_MEMORY_X);
}
+int set_memory_attr(unsigned long addr, int numpages, pgprot_t prot);
+
#endif
diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
index 2b573768a7f7..b78a2a656dea 100644
--- a/arch/powerpc/mm/pageattr.c
+++ b/arch/powerpc/mm/pageattr.c
@@ -72,3 +72,36 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
return apply_to_page_range(&init_mm, start, sz, change_page_attr, (void *)action);
}
+
+/*
+ * Set the attributes of a page:
+ *
+ * This function is used by PPC32 at the end of init to set final kernel memory
+ * protection. It includes changing the maping of the page it is executing from
+ * and data pages it is using.
+ */
+static int set_page_attr(pte_t *ptep, unsigned long addr, void *data)
+{
+ pgprot_t prot = __pgprot((int)data);
+
+ spin_lock(&init_mm.page_table_lock);
+
+ set_pte_at(&init_mm, addr, ptep, pte_modify(*ptep, prot));
+ flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
+
+ spin_unlock(&init_mm.page_table_lock);
+
+ return 0;
+}
+
+int set_memory_attr(unsigned long addr, int numpages, pgprot_t prot)
+{
+ unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE);
+ unsigned long sz = numpages * PAGE_SIZE;
+
+ if (!numpages)
+ return 0;
+
+ return apply_to_page_range(&init_mm, start, sz, set_page_attr,
+ (void *)pgprot_val(prot));
+}
--
2.25.0
^ permalink raw reply related
* EEH init cleanup
From: Oliver O'Halloran @ 2020-02-03 8:35 UTC (permalink / raw)
To: linuxppc-dev; +Cc: sbobroff, tyreld
This series reworks how EEH devices are initialised. This mainly affects
pseries since it moves the "early" EEH probe out of shared code and into
pseries platform code. The goal here is to make the platform
dependencies more explicit and to allow PowerNV to implement its own
pci_dev <-> eeh_dev mapping that doesn't require a PCI_DN.
^ permalink raw reply
* [PATCH 1/6] powerpc/eeh: Add sysfs files in late probe
From: Oliver O'Halloran @ 2020-02-03 8:35 UTC (permalink / raw)
To: linuxppc-dev; +Cc: sbobroff, tyreld, Oliver O'Halloran
In-Reply-To: <20200203083521.16549-1-oohall@gmail.com>
Move creating the EEH specific sysfs files into eeh_add_device_late()
rather than being open-coded all over the place. Calling the function is
generally done immediately after calling eeh_add_device_late() anyway. The
two cases where it's not done there (OF based PCI probing and the pseries
VFs) don't seem to have any issues with the re-ordering.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
arch/powerpc/include/asm/eeh.h | 3 ---
arch/powerpc/kernel/eeh.c | 24 +-----------------------
arch/powerpc/kernel/of_platform.c | 3 ---
arch/powerpc/kernel/pci-common.c | 3 ---
arch/powerpc/platforms/powernv/eeh-powernv.c | 1 -
arch/powerpc/platforms/pseries/eeh_pseries.c | 3 +--
6 files changed, 2 insertions(+), 35 deletions(-)
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 6f9b2a1..5a34907 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -305,7 +305,6 @@ void eeh_add_device_early(struct pci_dn *);
void eeh_add_device_tree_early(struct pci_dn *);
void eeh_add_device_late(struct pci_dev *);
void eeh_add_device_tree_late(struct pci_bus *);
-void eeh_add_sysfs_files(struct pci_bus *);
void eeh_remove_device(struct pci_dev *);
int eeh_unfreeze_pe(struct eeh_pe *pe);
int eeh_pe_reset_and_recover(struct eeh_pe *pe);
@@ -368,8 +367,6 @@ static inline void eeh_add_device_late(struct pci_dev *dev) { }
static inline void eeh_add_device_tree_late(struct pci_bus *bus) { }
-static inline void eeh_add_sysfs_files(struct pci_bus *bus) { }
-
static inline void eeh_remove_device(struct pci_dev *dev) { }
#define EEH_POSSIBLE_ERROR(val, type) (0)
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 17cb3e9..0878912 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1210,6 +1210,7 @@ void eeh_add_device_late(struct pci_dev *dev)
dev->dev.archdata.edev = edev;
eeh_addr_cache_insert_dev(dev);
+ eeh_sysfs_add_device(dev);
}
/**
@@ -1238,29 +1239,6 @@ void eeh_add_device_tree_late(struct pci_bus *bus)
EXPORT_SYMBOL_GPL(eeh_add_device_tree_late);
/**
- * eeh_add_sysfs_files - Add EEH sysfs files for the indicated PCI bus
- * @bus: PCI bus
- *
- * This routine must be used to add EEH sysfs files for PCI
- * devices which are attached to the indicated PCI bus. The PCI bus
- * is added after system boot through hotplug or dlpar.
- */
-void eeh_add_sysfs_files(struct pci_bus *bus)
-{
- struct pci_dev *dev;
-
- list_for_each_entry(dev, &bus->devices, bus_list) {
- eeh_sysfs_add_device(dev);
- if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
- struct pci_bus *subbus = dev->subordinate;
- if (subbus)
- eeh_add_sysfs_files(subbus);
- }
- }
-}
-EXPORT_SYMBOL_GPL(eeh_add_sysfs_files);
-
-/**
* eeh_remove_device - Undo EEH setup for the indicated pci device
* @dev: pci device to be removed
*
diff --git a/arch/powerpc/kernel/of_platform.c b/arch/powerpc/kernel/of_platform.c
index 427fc22..cb68800 100644
--- a/arch/powerpc/kernel/of_platform.c
+++ b/arch/powerpc/kernel/of_platform.c
@@ -86,9 +86,6 @@ static int of_pci_phb_probe(struct platform_device *dev)
/* Add probed PCI devices to the device model */
pci_bus_add_devices(phb->bus);
- /* sysfs files should only be added after devices are added */
- eeh_add_sysfs_files(phb->bus);
-
return 0;
}
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index c6c0341..3d2b1cf 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1404,9 +1404,6 @@ void pcibios_finish_adding_to_bus(struct pci_bus *bus)
/* Add new devices to global lists. Register in proc, sysfs. */
pci_bus_add_devices(bus);
-
- /* sysfs files should only be added after devices are added */
- eeh_add_sysfs_files(bus);
}
EXPORT_SYMBOL_GPL(pcibios_finish_adding_to_bus);
diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
index 6f300ab..ef727ec 100644
--- a/arch/powerpc/platforms/powernv/eeh-powernv.c
+++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
@@ -48,7 +48,6 @@ void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
dev_dbg(&pdev->dev, "EEH: Setting up device\n");
eeh_add_device_early(pdn);
eeh_add_device_late(pdev);
- eeh_sysfs_add_device(pdev);
}
static int pnv_eeh_init(void)
diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
index 893ba3f..95bbf91 100644
--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
@@ -68,7 +68,6 @@ void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
}
#endif
eeh_add_device_early(pdn);
- eeh_add_device_late(pdev);
#ifdef CONFIG_PCI_IOV
if (pdev->is_virtfn) {
struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
@@ -78,7 +77,7 @@ void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
eeh_add_to_parent_pe(edev); /* Add as VF PE type */
}
#endif
- eeh_sysfs_add_device(pdev);
+ eeh_add_device_late(pdev);
}
/*
--
2.9.5
^ permalink raw reply related
* [PATCH 2/6] powerpc/eeh: Remove eeh_add_device_tree_late()
From: Oliver O'Halloran @ 2020-02-03 8:35 UTC (permalink / raw)
To: linuxppc-dev; +Cc: sbobroff, tyreld, Oliver O'Halloran
In-Reply-To: <20200203083521.16549-1-oohall@gmail.com>
On pseries and PowerNV pcibios_bus_add_device() calls eeh_add_device_late()
so there's no need to do a separate tree traversal to bind the eeh_dev and
pci_dev together setting up the PHB at boot. As a result we can remove
eeh_add_device_tree_late().
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
arch/powerpc/include/asm/eeh.h | 3 ---
arch/powerpc/kernel/eeh.c | 25 -------------------------
arch/powerpc/kernel/of_platform.c | 3 ---
arch/powerpc/kernel/pci-common.c | 3 ---
4 files changed, 34 deletions(-)
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 5a34907..5d10781 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -304,7 +304,6 @@ void eeh_addr_cache_init(void);
void eeh_add_device_early(struct pci_dn *);
void eeh_add_device_tree_early(struct pci_dn *);
void eeh_add_device_late(struct pci_dev *);
-void eeh_add_device_tree_late(struct pci_bus *);
void eeh_remove_device(struct pci_dev *);
int eeh_unfreeze_pe(struct eeh_pe *pe);
int eeh_pe_reset_and_recover(struct eeh_pe *pe);
@@ -365,8 +364,6 @@ static inline void eeh_add_device_tree_early(struct pci_dn *pdn) { }
static inline void eeh_add_device_late(struct pci_dev *dev) { }
-static inline void eeh_add_device_tree_late(struct pci_bus *bus) { }
-
static inline void eeh_remove_device(struct pci_dev *dev) { }
#define EEH_POSSIBLE_ERROR(val, type) (0)
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 0878912..9cb3370 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1214,31 +1214,6 @@ void eeh_add_device_late(struct pci_dev *dev)
}
/**
- * eeh_add_device_tree_late - Perform EEH initialization for the indicated PCI bus
- * @bus: PCI bus
- *
- * This routine must be used to perform EEH initialization for PCI
- * devices which are attached to the indicated PCI bus. The PCI bus
- * is added after system boot through hotplug or dlpar.
- */
-void eeh_add_device_tree_late(struct pci_bus *bus)
-{
- struct pci_dev *dev;
-
- if (eeh_has_flag(EEH_FORCE_DISABLED))
- return;
- list_for_each_entry(dev, &bus->devices, bus_list) {
- eeh_add_device_late(dev);
- if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
- struct pci_bus *subbus = dev->subordinate;
- if (subbus)
- eeh_add_device_tree_late(subbus);
- }
- }
-}
-EXPORT_SYMBOL_GPL(eeh_add_device_tree_late);
-
-/**
* eeh_remove_device - Undo EEH setup for the indicated pci device
* @dev: pci device to be removed
*
diff --git a/arch/powerpc/kernel/of_platform.c b/arch/powerpc/kernel/of_platform.c
index cb68800..64edac81 100644
--- a/arch/powerpc/kernel/of_platform.c
+++ b/arch/powerpc/kernel/of_platform.c
@@ -80,9 +80,6 @@ static int of_pci_phb_probe(struct platform_device *dev)
*/
pcibios_claim_one_bus(phb->bus);
- /* Finish EEH setup */
- eeh_add_device_tree_late(phb->bus);
-
/* Add probed PCI devices to the device model */
pci_bus_add_devices(phb->bus);
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 3d2b1cf..8983afa 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1399,9 +1399,6 @@ void pcibios_finish_adding_to_bus(struct pci_bus *bus)
pci_assign_unassigned_bus_resources(bus);
}
- /* Fixup EEH */
- eeh_add_device_tree_late(bus);
-
/* Add new devices to global lists. Register in proc, sysfs. */
pci_bus_add_devices(bus);
}
--
2.9.5
^ permalink raw reply related
* [PATCH 3/6] powerpc/eeh: Do early EEH init only when required
From: Oliver O'Halloran @ 2020-02-03 8:35 UTC (permalink / raw)
To: linuxppc-dev; +Cc: sbobroff, tyreld, Oliver O'Halloran
In-Reply-To: <20200203083521.16549-1-oohall@gmail.com>
The pci hotplug helper (pci_hp_add_devices()) calls
eeh_add_device_tree_early() to scan the device-tree for new PCI devices and
do the early EEH probe before the device is scanned. This early probe is a
no-op in a lot of cases because:
a) The early init is only required to satisfy a PAPR requirement that EEH
be configured before we start doing config accesses. On PowerNV it is
a no-op.
b) It's a no-op for devices that have already had their eeh_dev
initialised.
There are four callers of pci_hp_add_devices():
1. arch/powerpc/kernel/eeh_driver.c
Here the hotplug helper is called when re-scanning pci_devs that
were removed during an EEH recovery pass. The EEH stat for each
removed device (the eeh_dev) is retained across a recovery pass
so the early init is a no-op in this case.
2. drivers/pci/hotplug/pnv_php.c
This is also a no-op since the PowerNV hotplug driver is, suprisingly,
PowerNV specific.
3. drivers/pci/hotplug/rpaphp_core.c
4. drivers/pci/hotplug/rpaphp_pci.c
In these two cases new devices have been hotplugged and FW has
provided new DT nodes for each. These are the only two cases where
the EEH we might have new PCI device nodes in the DT so these are
the only two cases where the early EEH probe needs to be done.
We can move the calls to eeh_add_device_tree_early() to the locations where
it's needed and remove it from the generic path. This is preparation for
making the early EEH probe pseries specific.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
arch/powerpc/kernel/pci-hotplug.c | 2 --
drivers/pci/hotplug/rpaphp_core.c | 2 ++
drivers/pci/hotplug/rpaphp_pci.c | 4 +++-
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/pci-hotplug.c b/arch/powerpc/kernel/pci-hotplug.c
index d6a67f8..bf83f76 100644
--- a/arch/powerpc/kernel/pci-hotplug.c
+++ b/arch/powerpc/kernel/pci-hotplug.c
@@ -112,8 +112,6 @@ void pci_hp_add_devices(struct pci_bus *bus)
struct pci_controller *phb;
struct device_node *dn = pci_bus_to_OF_node(bus);
- eeh_add_device_tree_early(PCI_DN(dn));
-
phb = pci_bus_to_host(bus);
mode = PCI_PROBE_NORMAL;
diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
index e408e40..9c1e43e 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -494,6 +494,8 @@ static int enable_slot(struct hotplug_slot *hotplug_slot)
return retval;
if (state == PRESENT) {
+ eeh_add_device_tree_early(PCI_DN(slot->dn));
+
pci_lock_rescan_remove();
pci_hp_add_devices(slot->bus);
pci_unlock_rescan_remove();
diff --git a/drivers/pci/hotplug/rpaphp_pci.c b/drivers/pci/hotplug/rpaphp_pci.c
index beca61b..61ebbd8 100644
--- a/drivers/pci/hotplug/rpaphp_pci.c
+++ b/drivers/pci/hotplug/rpaphp_pci.c
@@ -95,8 +95,10 @@ int rpaphp_enable_slot(struct slot *slot)
return -EINVAL;
}
- if (list_empty(&bus->devices))
+ if (list_empty(&bus->devices)) {
+ eeh_add_device_tree_early(PCI_DN(slot->dn));
pci_hp_add_devices(bus);
+ }
if (!list_empty(&bus->devices)) {
slot->state = CONFIGURED;
--
2.9.5
^ permalink raw reply related
* [PATCH 4/6] powerpc/eeh: Remove PHB check in probe
From: Oliver O'Halloran @ 2020-02-03 8:35 UTC (permalink / raw)
To: linuxppc-dev; +Cc: sbobroff, tyreld, Oliver O'Halloran
In-Reply-To: <20200203083521.16549-1-oohall@gmail.com>
This check for a missing PHB has existing in various forms since the
initial PPC64 port was upstreamed in 2002. The idea seems to be that we
need to guard against creating pci-specific data structures for the non-pci
children of a PCI device tree node (e.g. USB devices). However, we only
create pci_dn structures for DT nodes that correspond to PCI devices so
there's not much point in doing this check in the eeh_probe path.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
arch/powerpc/kernel/eeh.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 9cb3370..a9e4ca7 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1120,7 +1120,6 @@ core_initcall_sync(eeh_init);
*/
void eeh_add_device_early(struct pci_dn *pdn)
{
- struct pci_controller *phb = pdn ? pdn->phb : NULL;
struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
if (!edev)
@@ -1129,11 +1128,6 @@ void eeh_add_device_early(struct pci_dn *pdn)
if (!eeh_has_flag(EEH_PROBE_MODE_DEVTREE))
return;
- /* USB Bus children of PCI devices will not have BUID's */
- if (NULL == phb ||
- (eeh_has_flag(EEH_PROBE_MODE_DEVTREE) && 0 == phb->buid))
- return;
-
eeh_ops->probe(pdn, NULL);
}
--
2.9.5
^ permalink raw reply related
* [PATCH 5/6] powerpc/eeh: Make early EEH init pseries specific
From: Oliver O'Halloran @ 2020-02-03 8:35 UTC (permalink / raw)
To: linuxppc-dev; +Cc: sbobroff, tyreld, Oliver O'Halloran
In-Reply-To: <20200203083521.16549-1-oohall@gmail.com>
The eeh_ops->probe() function is called from two different contexts:
1. On pseries, where set set EEH_PROBE_MODE_DEVTREE, it's called in
eeh_add_device_early() which is supposed to run before we create
a pci_dev.
2. On PowerNV, where we set EEH_PROBE_MODE_DEV, it's called in
eeh_device_add_late() which is supposed to run *after* the
pci_dev is created.
The "early" probe is required because PAPR requires that we perform an RTAS
call to enable EEH support on a device before we start interacting with it
via config space or MMIO. This requirement doesn't exist on PowerNV and
shoehorning two completely separate initialisation paths into a common
interface just results in a convoluted code everywhere.
Additionally the early probe requires the probe function to take an pci_dn
rather than a pci_dev argument. We'd like to make pci_dn a pseries specific
data structure since there's no real requirement for them on PowerNV. To
help both goals move the early probe into the pseries containment zone
so the platform depedence is more explicit.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
arch/powerpc/include/asm/eeh.h | 14 +++---
arch/powerpc/kernel/eeh.c | 46 --------------------
arch/powerpc/kernel/of_platform.c | 6 +--
arch/powerpc/platforms/powernv/eeh-powernv.c | 6 ---
arch/powerpc/platforms/pseries/eeh_pseries.c | 65 ++++++++++++++++++++++------
arch/powerpc/platforms/pseries/pci_dlpar.c | 2 +-
drivers/pci/hotplug/rpadlpar_core.c | 2 +-
drivers/pci/hotplug/rpaphp_core.c | 2 +-
drivers/pci/hotplug/rpaphp_pci.c | 3 +-
9 files changed, 65 insertions(+), 81 deletions(-)
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 5d10781..8580238 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -301,8 +301,6 @@ int __exit eeh_ops_unregister(const char *name);
int eeh_check_failure(const volatile void __iomem *token);
int eeh_dev_check_failure(struct eeh_dev *edev);
void eeh_addr_cache_init(void);
-void eeh_add_device_early(struct pci_dn *);
-void eeh_add_device_tree_early(struct pci_dn *);
void eeh_add_device_late(struct pci_dev *);
void eeh_remove_device(struct pci_dev *);
int eeh_unfreeze_pe(struct eeh_pe *pe);
@@ -358,10 +356,6 @@ static inline int eeh_check_failure(const volatile void __iomem *token)
static inline void eeh_addr_cache_init(void) { }
-static inline void eeh_add_device_early(struct pci_dn *pdn) { }
-
-static inline void eeh_add_device_tree_early(struct pci_dn *pdn) { }
-
static inline void eeh_add_device_late(struct pci_dev *dev) { }
static inline void eeh_remove_device(struct pci_dev *dev) { }
@@ -370,6 +364,14 @@ static inline void eeh_remove_device(struct pci_dev *dev) { }
#define EEH_IO_ERROR_VALUE(size) (-1UL)
#endif /* CONFIG_EEH */
+#if defined(CONFIG_PPC_PSERIES) && defined(CONFIG_EEH)
+void pseries_eeh_init_edev(struct pci_dn *pdn);
+void pseries_eeh_init_edev_recursive(struct pci_dn *pdn);
+#else
+static inline void pseries_eeh_add_device_early(struct pci_dn *pdn) { }
+static inline void pseries_eeh_add_device_tree_early(struct pci_dn *pdn) { }
+#endif
+
#ifdef CONFIG_PPC64
/*
* MMIO read/write operations with EEH support.
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index a9e4ca7..55d3ef6 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1107,52 +1107,6 @@ static int eeh_init(void)
core_initcall_sync(eeh_init);
/**
- * eeh_add_device_early - Enable EEH for the indicated device node
- * @pdn: PCI device node for which to set up EEH
- *
- * This routine must be used to perform EEH initialization for PCI
- * devices that were added after system boot (e.g. hotplug, dlpar).
- * This routine must be called before any i/o is performed to the
- * adapter (inluding any config-space i/o).
- * Whether this actually enables EEH or not for this device depends
- * on the CEC architecture, type of the device, on earlier boot
- * command-line arguments & etc.
- */
-void eeh_add_device_early(struct pci_dn *pdn)
-{
- struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
-
- if (!edev)
- return;
-
- if (!eeh_has_flag(EEH_PROBE_MODE_DEVTREE))
- return;
-
- eeh_ops->probe(pdn, NULL);
-}
-
-/**
- * eeh_add_device_tree_early - Enable EEH for the indicated device
- * @pdn: PCI device node
- *
- * This routine must be used to perform EEH initialization for the
- * indicated PCI device that was added after system boot (e.g.
- * hotplug, dlpar).
- */
-void eeh_add_device_tree_early(struct pci_dn *pdn)
-{
- struct pci_dn *n;
-
- if (!pdn)
- return;
-
- list_for_each_entry(n, &pdn->child_list, list)
- eeh_add_device_tree_early(n);
- eeh_add_device_early(pdn);
-}
-EXPORT_SYMBOL_GPL(eeh_add_device_tree_early);
-
-/**
* eeh_add_device_late - Perform EEH initialization for the indicated pci device
* @dev: pci device for which to set up EEH
*
diff --git a/arch/powerpc/kernel/of_platform.c b/arch/powerpc/kernel/of_platform.c
index 64edac81..71a3f97 100644
--- a/arch/powerpc/kernel/of_platform.c
+++ b/arch/powerpc/kernel/of_platform.c
@@ -62,13 +62,9 @@ static int of_pci_phb_probe(struct platform_device *dev)
/* Init pci_dn data structures */
pci_devs_phb_init_dynamic(phb);
- /* Create EEH devices for the PHB */
+ /* Create EEH PEs for the PHB */
eeh_dev_phb_init_dynamic(phb);
- /* Register devices with EEH */
- if (dev->dev.of_node->child)
- eeh_add_device_tree_early(PCI_DN(dev->dev.of_node));
-
/* Scan the bus */
pcibios_scan_phb(phb);
if (phb->bus == NULL)
diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
index ef727ec..eaa8dfe 100644
--- a/arch/powerpc/platforms/powernv/eeh-powernv.c
+++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
@@ -40,13 +40,7 @@ static int eeh_event_irq = -EINVAL;
void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
{
- struct pci_dn *pdn = pci_get_pdn(pdev);
-
- if (!pdn || eeh_has_flag(EEH_FORCE_DISABLED))
- return;
-
dev_dbg(&pdev->dev, "EEH: Setting up device\n");
- eeh_add_device_early(pdn);
eeh_add_device_late(pdev);
}
diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
index 95bbf91..1ca7cf0 100644
--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
@@ -67,7 +67,7 @@ void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
pdn->pe_number = physfn_pdn->pe_num_map[pdn->vf_index];
}
#endif
- eeh_add_device_early(pdn);
+ pseries_eeh_init_edev(pdn);
#ifdef CONFIG_PCI_IOV
if (pdev->is_virtfn) {
struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
@@ -221,15 +221,16 @@ static int pseries_eeh_find_ecap(struct pci_dn *pdn, int cap)
}
/**
- * pseries_eeh_probe - EEH probe on the given device
+ * pseries_eeh_init_edev - initialise the eeh_dev and eeh_pe for a pci_dn
+ *
* @pdn: PCI device node
- * @data: Unused
*
- * When EEH module is installed during system boot, all PCI devices
- * are checked one by one to see if it supports EEH. The function
- * is introduced for the purpose.
+ * When we discover a new PCI device via the device-tree we create a
+ * corresponding pci_dn and we allocate, but don't initialise, an eeh_dev.
+ * This function takes care of the initialisation and inserts the eeh_dev
+ * into the correct eeh_pe. If no eeh_pe exists we'll allocate one.
*/
-static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
+void pseries_eeh_init_edev(struct pci_dn *pdn)
{
struct eeh_dev *edev;
struct eeh_pe pe;
@@ -237,18 +238,35 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
int enable = 0;
int ret;
- /* Retrieve OF node and eeh device */
+ if (WARN_ON_ONCE(!eeh_has_flag(EEH_PROBE_MODE_DEVTREE)))
+ return;
+
+ /*
+ * Find the eeh_dev for this pdn. The storage for the eeh_dev was
+ * allocated at the same time as the pci_dn.
+ *
+ * XXX: We should probably re-visit that.
+ */
edev = pdn_to_eeh_dev(pdn);
- if (!edev || edev->pe)
- return NULL;
+ if (!edev)
+ return;
+
+ /*
+ * If ->pe is set then we've already probed this device. We hit
+ * this path when a pci_dev is removed and rescanned while recovering
+ * a PE (i.e. for devices where the driver doesn't support error
+ * recovery).
+ */
+ if (edev->pe)
+ return;
/* Check class/vendor/device IDs */
if (!pdn->vendor_id || !pdn->device_id || !pdn->class_code)
- return NULL;
+ return;
/* Skip for PCI-ISA bridge */
if ((pdn->class_code >> 8) == PCI_CLASS_BRIDGE_ISA)
- return NULL;
+ return;
eeh_edev_dbg(edev, "Probing device\n");
@@ -315,9 +333,29 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
/* Save memory bars */
eeh_save_bars(edev);
+}
+
+/**
+ * pseries_eeh_init_edev_recursive - Enable EEH for the indicated device
+ * @pdn: PCI device node
+ *
+ * This routine must be used to perform EEH initialization for the
+ * indicated PCI device that was added after system boot (e.g.
+ * hotplug, dlpar).
+ */
+void pseries_eeh_init_edev_recursive(struct pci_dn *pdn)
+{
+ struct pci_dn *n;
+
+ if (!pdn)
+ return;
+
+ list_for_each_entry(n, &pdn->child_list, list)
+ pseries_eeh_init_edev_recursive(n);
- return NULL;
+ pseries_eeh_init_edev(pdn);
}
+EXPORT_SYMBOL_GPL(pseries_eeh_init_edev_recursive);
/**
* pseries_eeh_set_option - Initialize EEH or MMIO/DMA reenable
@@ -775,7 +813,6 @@ static int pseries_notify_resume(struct pci_dn *pdn)
static struct eeh_ops pseries_eeh_ops = {
.name = "pseries",
.init = pseries_eeh_init,
- .probe = pseries_eeh_probe,
.set_option = pseries_eeh_set_option,
.get_pe_addr = pseries_eeh_get_pe_addr,
.get_state = pseries_eeh_get_state,
diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/platforms/pseries/pci_dlpar.c
index 361986e..b3a38f5 100644
--- a/arch/powerpc/platforms/pseries/pci_dlpar.c
+++ b/arch/powerpc/platforms/pseries/pci_dlpar.c
@@ -37,7 +37,7 @@ struct pci_controller *init_phb_dynamic(struct device_node *dn)
eeh_dev_phb_init_dynamic(phb);
if (dn->child)
- eeh_add_device_tree_early(PCI_DN(dn));
+ pseries_eeh_init_edev_recursive(PCI_DN(dn));
pcibios_scan_phb(phb);
pcibios_finish_adding_to_bus(phb->bus);
diff --git a/drivers/pci/hotplug/rpadlpar_core.c b/drivers/pci/hotplug/rpadlpar_core.c
index 977946e..c5eb509 100644
--- a/drivers/pci/hotplug/rpadlpar_core.c
+++ b/drivers/pci/hotplug/rpadlpar_core.c
@@ -140,7 +140,7 @@ static void dlpar_pci_add_bus(struct device_node *dn)
struct pci_controller *phb = pdn->phb;
struct pci_dev *dev = NULL;
- eeh_add_device_tree_early(pdn);
+ pseries_eeh_init_edev_recursive(pdn);
/* Add EADS device to PHB bus, adding new entry to bus->devices */
dev = of_create_pci_dev(dn, phb->bus, pdn->devfn);
diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
index 9c1e43e..b89d5ff 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -494,7 +494,7 @@ static int enable_slot(struct hotplug_slot *hotplug_slot)
return retval;
if (state == PRESENT) {
- eeh_add_device_tree_early(PCI_DN(slot->dn));
+ pseries_eeh_init_edev_recursive(PCI_DN(slot->dn));
pci_lock_rescan_remove();
pci_hp_add_devices(slot->bus);
diff --git a/drivers/pci/hotplug/rpaphp_pci.c b/drivers/pci/hotplug/rpaphp_pci.c
index 61ebbd8..e116ffe 100644
--- a/drivers/pci/hotplug/rpaphp_pci.c
+++ b/drivers/pci/hotplug/rpaphp_pci.c
@@ -96,7 +96,8 @@ int rpaphp_enable_slot(struct slot *slot)
}
if (list_empty(&bus->devices)) {
- eeh_add_device_tree_early(PCI_DN(slot->dn));
+ // XXX: uh, do we have the rescan lock held here?
+ pseries_eeh_init_edev_recursive(PCI_DN(slot->dn));
pci_hp_add_devices(bus);
}
--
2.9.5
^ permalink raw reply related
* [PATCH 6/6] powerpc/eeh: Rework eeh_ops->probe()
From: Oliver O'Halloran @ 2020-02-03 8:35 UTC (permalink / raw)
To: linuxppc-dev; +Cc: sbobroff, tyreld, Oliver O'Halloran
In-Reply-To: <20200203083521.16549-1-oohall@gmail.com>
With the EEH early probe now being pseries specific there's no need for
eeh_ops->probe() to take a pci_dn. Instead, we can make it take a pci_dev
and use the probe function to map a pci_dev to an eeh_dev. This allows
the platform to implement it's own method for finding (or creating) an
eeh_dev for a given pci_dev which also removes a use of pci_dn in
generic EEH code.
This patch also renames eeh_device_add_late() to eeh_device_probe(). This
better reflects what it does does and removes the last vestiges of the
early/late EEH probe split.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
arch/powerpc/include/asm/eeh.h | 6 ++--
arch/powerpc/kernel/eeh.c | 42 +++++++++++++++-------------
arch/powerpc/platforms/powernv/eeh-powernv.c | 30 ++++++++++----------
arch/powerpc/platforms/pseries/eeh_pseries.c | 23 ++++++++++++++-
4 files changed, 61 insertions(+), 40 deletions(-)
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 8580238..964a542 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -215,7 +215,7 @@ enum {
struct eeh_ops {
char *name;
int (*init)(void);
- void* (*probe)(struct pci_dn *pdn, void *data);
+ struct eeh_dev *(*probe)(struct pci_dev *pdev);
int (*set_option)(struct eeh_pe *pe, int option);
int (*get_pe_addr)(struct eeh_pe *pe);
int (*get_state)(struct eeh_pe *pe, int *delay);
@@ -301,7 +301,7 @@ int __exit eeh_ops_unregister(const char *name);
int eeh_check_failure(const volatile void __iomem *token);
int eeh_dev_check_failure(struct eeh_dev *edev);
void eeh_addr_cache_init(void);
-void eeh_add_device_late(struct pci_dev *);
+void eeh_probe_device(struct pci_dev *pdev);
void eeh_remove_device(struct pci_dev *);
int eeh_unfreeze_pe(struct eeh_pe *pe);
int eeh_pe_reset_and_recover(struct eeh_pe *pe);
@@ -356,7 +356,7 @@ static inline int eeh_check_failure(const volatile void __iomem *token)
static inline void eeh_addr_cache_init(void) { }
-static inline void eeh_add_device_late(struct pci_dev *dev) { }
+static inline void eeh_probe_device(struct pci_dev *dev) { }
static inline void eeh_remove_device(struct pci_dev *dev) { }
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 55d3ef6..2c5f7a6 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1113,29 +1113,37 @@ core_initcall_sync(eeh_init);
* This routine must be used to complete EEH initialization for PCI
* devices that were added after system boot (e.g. hotplug, dlpar).
*/
-void eeh_add_device_late(struct pci_dev *dev)
+void eeh_probe_device(struct pci_dev *dev)
{
- struct pci_dn *pdn;
struct eeh_dev *edev;
- if (!dev)
+ pr_debug("EEH: Adding device %s\n", pci_name(dev));
+
+ /*
+ * pci_dev_to_eeh_dev() can only work if eeh_probe_dev() was
+ * already called for this device.
+ */
+ if (WARN_ON_ONCE(pci_dev_to_eeh_dev(dev))) {
+ eeh_edev_dbg(edev, "Already bound to an eeh_dev!\n");
return;
+ }
- pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn);
- edev = pdn_to_eeh_dev(pdn);
- eeh_edev_dbg(edev, "Adding device\n");
- if (edev->pdev == dev) {
- eeh_edev_dbg(edev, "Device already referenced!\n");
+ edev = eeh_ops->probe(dev);
+ if (!edev) {
+ pr_debug("EEH: Adding device failed\n");
return;
}
/*
- * The EEH cache might not be removed correctly because of
- * unbalanced kref to the device during unplug time, which
- * relies on pcibios_release_device(). So we have to remove
- * that here explicitly.
+ * FIXME: We rely on pcibios_release_device() to remove the
+ * existing EEH state. The release function is only called if
+ * the pci_dev's refcount drops to zero so if something is
+ * keeping a ref to a device (e.g. a filesystem) we need to
+ * remove the old EEH state.
+ *
+ * FIXME: HEY MA, LOOK AT ME, NO LOCKING!
*/
- if (edev->pdev) {
+ if (edev->pdev && edev->pdev != dev) {
eeh_rmv_from_parent_pe(edev);
eeh_addr_cache_rmv_dev(edev->pdev);
eeh_sysfs_remove_device(edev->pdev);
@@ -1146,17 +1154,11 @@ void eeh_add_device_late(struct pci_dev *dev)
* into error handler afterwards.
*/
edev->mode |= EEH_DEV_NO_HANDLER;
-
- edev->pdev = NULL;
- dev->dev.archdata.edev = NULL;
}
- if (eeh_has_flag(EEH_PROBE_MODE_DEV))
- eeh_ops->probe(pdn, NULL);
-
+ /* bind the pdev and the edev together */
edev->pdev = dev;
dev->dev.archdata.edev = edev;
-
eeh_addr_cache_insert_dev(dev);
eeh_sysfs_add_device(dev);
}
diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
index eaa8dfe..79409e0 100644
--- a/arch/powerpc/platforms/powernv/eeh-powernv.c
+++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
@@ -41,7 +41,7 @@ static int eeh_event_irq = -EINVAL;
void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
{
dev_dbg(&pdev->dev, "EEH: Setting up device\n");
- eeh_add_device_late(pdev);
+ eeh_probe_device(pdev);
}
static int pnv_eeh_init(void)
@@ -340,23 +340,13 @@ static int pnv_eeh_find_ecap(struct pci_dn *pdn, int cap)
/**
* pnv_eeh_probe - Do probe on PCI device
- * @pdn: PCI device node
- * @data: unused
+ * @pdev: pci_dev to probe
*
- * When EEH module is installed during system boot, all PCI devices
- * are checked one by one to see if it supports EEH. The function
- * is introduced for the purpose. By default, EEH has been enabled
- * on all PCI devices. That's to say, we only need do necessary
- * initialization on the corresponding eeh device and create PE
- * accordingly.
- *
- * It's notable that's unsafe to retrieve the EEH device through
- * the corresponding PCI device. During the PCI device hotplug, which
- * was possiblly triggered by EEH core, the binding between EEH device
- * and the PCI device isn't built yet.
+ * Create, or find the existing, eeh_dev for this pci_dev.
*/
-static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
+static struct eeh_dev *pnv_eeh_probe(struct pci_dev *pdev)
{
+ struct pci_dn *pdn = pci_get_pdn(pdev);
struct pci_controller *hose = pdn->phb;
struct pnv_phb *phb = hose->private_data;
struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
@@ -373,6 +363,14 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
if (!edev || edev->pe)
return NULL;
+ /* already configured? */
+ if (edev->pdev) {
+ pr_debug("%s: found existing edev for %04x:%02x:%02x.%01x\n",
+ __func__, hose->global_number, config_addr >> 8,
+ PCI_SLOT(config_addr), PCI_FUNC(config_addr));
+ return edev;
+ }
+
/* Skip for PCI-ISA bridge */
if ((pdn->class_code >> 8) == PCI_CLASS_BRIDGE_ISA)
return NULL;
@@ -464,7 +462,7 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
eeh_edev_dbg(edev, "EEH enabled on device\n");
- return NULL;
+ return edev;
}
/**
diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
index 1ca7cf0..8453428 100644
--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
@@ -77,7 +77,7 @@ void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
eeh_add_to_parent_pe(edev); /* Add as VF PE type */
}
#endif
- eeh_add_device_late(pdev);
+ eeh_probe_device(pdev);
}
/*
@@ -335,6 +335,26 @@ void pseries_eeh_init_edev(struct pci_dn *pdn)
eeh_save_bars(edev);
}
+static struct eeh_dev *pseries_eeh_probe(struct pci_dev *pdev)
+{
+ struct eeh_dev *edev;
+ struct pci_dn *pdn;
+
+ pdn = pci_get_pdn_by_devfn(pdev->bus, pdev->devfn);
+ if (!pdn)
+ return NULL;
+
+ /*
+ * If the system supports EEH on this device then the eeh_dev was
+ * configured and inserted into a PE in pseries_eeh_init_edev()
+ */
+ edev = pdn_to_eeh_dev(pdn);
+ if (!edev || !edev->pe)
+ return NULL;
+
+ return edev;
+}
+
/**
* pseries_eeh_init_edev_recursive - Enable EEH for the indicated device
* @pdn: PCI device node
@@ -813,6 +833,7 @@ static int pseries_notify_resume(struct pci_dn *pdn)
static struct eeh_ops pseries_eeh_ops = {
.name = "pseries",
.init = pseries_eeh_init,
+ .probe = pseries_eeh_probe,
.set_option = pseries_eeh_set_option,
.get_pe_addr = pseries_eeh_get_pe_addr,
.get_state = pseries_eeh_get_state,
--
2.9.5
^ permalink raw reply related
* Re: [linuxppc-dev] Patch notification: 1 patch updated
From: Segher Boessenkool @ 2020-02-03 10:51 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <68a42fb3-1abc-d2f1-73eb-562f0dfe723f@c-s.fr>
On Mon, Feb 03, 2020 at 07:21:52AM +0100, Christophe Leroy wrote:
> Le 03/02/2020 à 02:00, Patchwork a écrit :
> >The following patch (submitted by you) has been updated in Patchwork:
> >
> > * linuxppc-dev: powerpc/nohash: Don't flush all TLBs when flushing one
> > page
> > - http://patchwork.ozlabs.org/patch/1231983/
> > - for: Linux PPC development
> > was: New
> > now: Superseded
>
> Superseded ? By what ?
>
> I sent a v2 for book3s/32, but this one is for nohash.
Other than the prefix, those patches have an identical subject. It
looks like something automated (maybe Michael himself ;-) ) was confused
by that :-)
Segher
^ permalink raw reply
* Re: [PATCH v5 6/6] powerpc/vdso: Switch VDSO to generic C implementation.
From: kbuild test robot @ 2020-02-03 11:27 UTC (permalink / raw)
To: Christophe Leroy
Cc: nathanl, kbuild-all, arnd, x86, linux-kernel, linux-mips,
Paul Mackerras, luto, tglx, vincenzo.frascino, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <231ec2e1fd2470ef7a9b8b2c766ff8e4095b6dd3.1580399657.git.christophe.leroy@c-s.fr>
[-- Attachment #1: Type: text/plain, Size: 2292 bytes --]
Hi Christophe,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on next-20200130]
[cannot apply to powerpc/next tip/timers/vdso mpe/next v5.5 v5.5-rc7 v5.5-rc6 v5.5]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-switch-VDSO-to-C-implementation/20200202-175410
base: c32e1d01a152aee976763ccf7c7ced53ca45b78f
config: powerpc-defconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=powerpc
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> arch/powerpc/kernel/vdso32/vgettimeofday.c:26:1: error: unknown type name 'time_t'; did you mean 'ktime_t'?
time_t __c_kernel_time(time_t *time, const struct vdso_data *vd)
^~~~~~
ktime_t
arch/powerpc/kernel/vdso32/vgettimeofday.c:26:24: error: unknown type name 'time_t'; did you mean 'ktime_t'?
time_t __c_kernel_time(time_t *time, const struct vdso_data *vd)
^~~~~~
ktime_t
--
>> arch/powerpc/kernel/vdso64/vgettimeofday.c:26:1: error: unknown type name 'time_t'; did you mean 'ktime_t'?
time_t __c_kernel_time(time_t *time, const struct vdso_data *vd)
^~~~~~
ktime_t
arch/powerpc/kernel/vdso64/vgettimeofday.c:26:24: error: unknown type name 'time_t'; did you mean 'ktime_t'?
time_t __c_kernel_time(time_t *time, const struct vdso_data *vd)
^~~~~~
ktime_t
vim +26 arch/powerpc/kernel/vdso32/vgettimeofday.c
25
> 26 time_t __c_kernel_time(time_t *time, const struct vdso_data *vd)
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25883 bytes --]
^ permalink raw reply
* Re: [PATCH v5 6/6] powerpc/vdso: Switch VDSO to generic C implementation.
From: Arnd Bergmann @ 2020-02-03 11:59 UTC (permalink / raw)
To: kbuild test robot
Cc: Nathan Lynch, kbuild-all, the arch/x86 maintainers,
linux-kernel@vger.kernel.org, open list:BROADCOM NVRAM DRIVER,
Paul Mackerras, Andy Lutomirski, Thomas Gleixner,
Vincenzo Frascino, linuxppc-dev, Linux ARM
In-Reply-To: <202002031426.QLrslGif%lkp@intel.com>
On Mon, Feb 3, 2020 at 12:28 PM kbuild test robot <lkp@intel.com> wrote:
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
>
> All errors (new ones prefixed by >>):
>
> >> arch/powerpc/kernel/vdso32/vgettimeofday.c:26:1: error: unknown type name 'time_t'; did you mean 'ktime_t'?
> time_t __c_kernel_time(time_t *time, const struct vdso_data *vd)
> ^~~~~~
> ktime_t
time_t is no longer available in linux-next, so this has to use
__kernel_old_time_t.
Arnd
^ permalink raw reply
* Re: [PATCH v2 06/27] ocxl: Tally up the LPC memory on a link & allow it to be mapped
From: Jonathan Cameron @ 2020-02-03 12:37 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Keith Busch,
Masahiro Yamada, Paul Mackerras, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Vishal Verma, Mahesh Salgaonkar, Krzysztof Kozlowski,
Anju T Sudhakar, alastair, Andrew Donnellan, Arnd Bergmann,
Greg Kurz, Nicholas Piggin, Cédric Le Goater, Dan Williams,
Hari Bathini, linux-mm, Greg Kroah-Hartman, linux-kernel,
Frederic Barrat, Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <20191203034655.51561-7-alastair@au1.ibm.com>
On Tue, 3 Dec 2019 14:46:34 +1100
Alastair D'Silva <alastair@au1.ibm.com> wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> Tally up the LPC memory on an OpenCAPI link & allow it to be mapped
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
Hi Alastair,
A few trivial comments inline.
Jonathan
> ---
> drivers/misc/ocxl/core.c | 10 ++++++
> drivers/misc/ocxl/link.c | 60 +++++++++++++++++++++++++++++++
> drivers/misc/ocxl/ocxl_internal.h | 33 +++++++++++++++++
> 3 files changed, 103 insertions(+)
>
> diff --git a/drivers/misc/ocxl/core.c b/drivers/misc/ocxl/core.c
> index b7a09b21ab36..2531c6cf19a0 100644
> --- a/drivers/misc/ocxl/core.c
> +++ b/drivers/misc/ocxl/core.c
> @@ -230,8 +230,18 @@ static int configure_afu(struct ocxl_afu *afu, u8 afu_idx, struct pci_dev *dev)
> if (rc)
> goto err_free_pasid;
>
> + if (afu->config.lpc_mem_size || afu->config.special_purpose_mem_size) {
> + rc = ocxl_link_add_lpc_mem(afu->fn->link, afu->config.lpc_mem_offset,
> + afu->config.lpc_mem_size +
> + afu->config.special_purpose_mem_size);
> + if (rc)
> + goto err_free_mmio;
> + }
> +
> return 0;
>
> +err_free_mmio:
> + unmap_mmio_areas(afu);
> err_free_pasid:
> reclaim_afu_pasid(afu);
> err_free_actag:
> diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c
> index 58d111afd9f6..d8503f0dc6ec 100644
> --- a/drivers/misc/ocxl/link.c
> +++ b/drivers/misc/ocxl/link.c
> @@ -84,6 +84,11 @@ struct ocxl_link {
> int dev;
> atomic_t irq_available;
> struct spa *spa;
> + struct mutex lpc_mem_lock;
Always a good idea to explicitly document what a lock is intended to protect.
> + u64 lpc_mem_sz; /* Total amount of LPC memory presented on the link */
> + u64 lpc_mem;
> + int lpc_consumers;
> +
> void *platform_data;
> };
> static struct list_head links_list = LIST_HEAD_INIT(links_list);
> @@ -396,6 +401,8 @@ static int alloc_link(struct pci_dev *dev, int PE_mask, struct ocxl_link **out_l
> if (rc)
> goto err_spa;
>
> + mutex_init(&link->lpc_mem_lock);
> +
> /* platform specific hook */
> rc = pnv_ocxl_spa_setup(dev, link->spa->spa_mem, PE_mask,
> &link->platform_data);
> @@ -711,3 +718,56 @@ void ocxl_link_free_irq(void *link_handle, int hw_irq)
> atomic_inc(&link->irq_available);
> }
> EXPORT_SYMBOL_GPL(ocxl_link_free_irq);
> +
> +int ocxl_link_add_lpc_mem(void *link_handle, u64 offset, u64 size)
> +{
> + struct ocxl_link *link = (struct ocxl_link *) link_handle;
> +
> + // Check for overflow
Stray c++ style comment.
> + if (offset > (offset + size))
> + return -EINVAL;
> +
> + mutex_lock(&link->lpc_mem_lock);
> + link->lpc_mem_sz = max(link->lpc_mem_sz, offset + size);
> +
> + mutex_unlock(&link->lpc_mem_lock);
> +
> + return 0;
> +}
> +
> +u64 ocxl_link_lpc_map(void *link_handle, struct pci_dev *pdev)
> +{
> + struct ocxl_link *link = (struct ocxl_link *) link_handle;
> + u64 lpc_mem;
> +
> + mutex_lock(&link->lpc_mem_lock);
> + if (link->lpc_mem) {
If you don't modify this later in the series (I haven't read it all yet :),
it rather feels like it would be more compact and just as readable as
something like...
if (!link->lpc_mem)
link->lpc_mem = pnv_ocxl...
if (link->lpc_mem)
link->lpc_consumers++;
mutex_unlock(&link->lpc_mem_lock);
return link->lpc_mem;
> + lpc_mem = link->lpc_mem;
> +
> + link->lpc_consumers++;
> + mutex_unlock(&link->lpc_mem_lock);
> + return lpc_mem;
> + }
> +
> + link->lpc_mem = pnv_ocxl_platform_lpc_setup(pdev, link->lpc_mem_sz);
> + if (link->lpc_mem)
> + link->lpc_consumers++;
> + lpc_mem = link->lpc_mem;
> + mutex_unlock(&link->lpc_mem_lock);
> +
> + return lpc_mem;
> +}
> +
> +void ocxl_link_lpc_release(void *link_handle, struct pci_dev *pdev)
> +{
> + struct ocxl_link *link = (struct ocxl_link *) link_handle;
> +
> + mutex_lock(&link->lpc_mem_lock);
> + WARN_ON(--link->lpc_consumers < 0);
> + if (link->lpc_consumers == 0) {
> + pnv_ocxl_platform_lpc_release(pdev);
> + link->lpc_mem = 0;
> + }
> +
> + mutex_unlock(&link->lpc_mem_lock);
> +}
> diff --git a/drivers/misc/ocxl/ocxl_internal.h b/drivers/misc/ocxl/ocxl_internal.h
> index 97415afd79f3..20b417e00949 100644
> --- a/drivers/misc/ocxl/ocxl_internal.h
> +++ b/drivers/misc/ocxl/ocxl_internal.h
> @@ -141,4 +141,37 @@ int ocxl_irq_offset_to_id(struct ocxl_context *ctx, u64 offset);
> u64 ocxl_irq_id_to_offset(struct ocxl_context *ctx, int irq_id);
> void ocxl_afu_irq_free_all(struct ocxl_context *ctx);
>
> +/**
> + * ocxl_link_add_lpc_mem() - Increment the amount of memory required by an OpenCAPI link
> + *
> + * @link_handle: The OpenCAPI link handle
> + * @offset: The offset of the memory to add
> + * @size: The amount of memory to increment by
> + *
> + * Return 0 on success, negative on overflow
> + */
> +int ocxl_link_add_lpc_mem(void *link_handle, u64 offset, u64 size);
> +
> +/**
> + * ocxl_link_lpc_map() - Map the LPC memory for an OpenCAPI device
> + *
> + * Since LPC memory belongs to a link, the whole LPC memory available
> + * on the link bust be mapped in order to make it accessible to a device.
must
> + *
> + * @link_handle: The OpenCAPI link handle
> + * @pdev: A device that is on the link
> + */
> +u64 ocxl_link_lpc_map(void *link_handle, struct pci_dev *pdev);
> +
> +/**
> + * ocxl_link_lpc_release() - Release the LPC memory device for an OpenCAPI device
> + *
> + * Offlines LPC memory on an OpenCAPI link for a device. If this is the
> + * last device on the link to release the memory, unmap it from the link.
> + *
> + * @link_handle: The OpenCAPI link handle
> + * @pdev: A device that is on the link
> + */
> +void ocxl_link_lpc_release(void *link_handle, struct pci_dev *pdev);
> +
> #endif /* _OCXL_INTERNAL_H_ */
^ permalink raw reply
* Re: [PATCH v2 07/27] ocxl: Add functions to map/unmap LPC memory
From: Jonathan Cameron @ 2020-02-03 12:49 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Keith Busch,
Masahiro Yamada, Paul Mackerras, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Vishal Verma, Mahesh Salgaonkar, Krzysztof Kozlowski,
Anju T Sudhakar, alastair, Andrew Donnellan, Arnd Bergmann,
Greg Kurz, Nicholas Piggin, Cédric Le Goater, Dan Williams,
Hari Bathini, linux-mm, Greg Kroah-Hartman, linux-kernel,
Frederic Barrat, Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <20191203034655.51561-8-alastair@au1.ibm.com>
On Tue, 3 Dec 2019 14:46:35 +1100
Alastair D'Silva <alastair@au1.ibm.com> wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> Add functions to map/unmap LPC memory
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
> drivers/misc/ocxl/config.c | 4 +++
> drivers/misc/ocxl/core.c | 50 +++++++++++++++++++++++++++++++
> drivers/misc/ocxl/ocxl_internal.h | 3 ++
> include/misc/ocxl.h | 18 +++++++++++
> 4 files changed, 75 insertions(+)
>
> diff --git a/drivers/misc/ocxl/config.c b/drivers/misc/ocxl/config.c
> index c8e19bfb5ef9..fb0c3b6f8312 100644
> --- a/drivers/misc/ocxl/config.c
> +++ b/drivers/misc/ocxl/config.c
> @@ -568,6 +568,10 @@ static int read_afu_lpc_memory_info(struct pci_dev *dev,
> afu->special_purpose_mem_size =
> total_mem_size - lpc_mem_size;
> }
> +
> + dev_info(&dev->dev, "Probed LPC memory of %#llx bytes and special purpose memory of %#llx bytes\n",
> + afu->lpc_mem_size, afu->special_purpose_mem_size);
> +
If we are being fussy, this block has nothing todo with the rest of the patch
so we should be seeing it here.
> return 0;
> }
>
> diff --git a/drivers/misc/ocxl/core.c b/drivers/misc/ocxl/core.c
> index 2531c6cf19a0..98611faea219 100644
> --- a/drivers/misc/ocxl/core.c
> +++ b/drivers/misc/ocxl/core.c
> @@ -210,6 +210,55 @@ static void unmap_mmio_areas(struct ocxl_afu *afu)
> release_fn_bar(afu->fn, afu->config.global_mmio_bar);
> }
>
> +int ocxl_afu_map_lpc_mem(struct ocxl_afu *afu)
> +{
> + struct pci_dev *dev = to_pci_dev(afu->fn->dev.parent);
> +
> + if ((afu->config.lpc_mem_size + afu->config.special_purpose_mem_size) == 0)
> + return 0;
> +
> + afu->lpc_base_addr = ocxl_link_lpc_map(afu->fn->link, dev);
> + if (afu->lpc_base_addr == 0)
> + return -EINVAL;
> +
> + if (afu->config.lpc_mem_size) {
I was happy with the explicit check on 0 above, but we should be consistent. Either
we make use of 0 == false, or we don't and explicitly check vs 0.
Hence
if (afu->config.pc_mem_size != 0) {
here or
if (!(afu->config.pc_mem_size + afu->config.special_purpose_mem_size))
return 0;
above.
> + afu->lpc_res.start = afu->lpc_base_addr + afu->config.lpc_mem_offset;
> + afu->lpc_res.end = afu->lpc_res.start + afu->config.lpc_mem_size - 1;
> + }
> +
> + if (afu->config.special_purpose_mem_size) {
> + afu->special_purpose_res.start = afu->lpc_base_addr +
> + afu->config.special_purpose_mem_offset;
> + afu->special_purpose_res.end = afu->special_purpose_res.start +
> + afu->config.special_purpose_mem_size - 1;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ocxl_afu_map_lpc_mem);
> +
> +struct resource *ocxl_afu_lpc_mem(struct ocxl_afu *afu)
> +{
> + return &afu->lpc_res;
> +}
> +EXPORT_SYMBOL_GPL(ocxl_afu_lpc_mem);
> +
> +static void unmap_lpc_mem(struct ocxl_afu *afu)
> +{
> + struct pci_dev *dev = to_pci_dev(afu->fn->dev.parent);
> +
> + if (afu->lpc_res.start || afu->special_purpose_res.start) {
> + void *link = afu->fn->link;
> +
> + ocxl_link_lpc_release(link, dev);
> +
> + afu->lpc_res.start = 0;
> + afu->lpc_res.end = 0;
> + afu->special_purpose_res.start = 0;
> + afu->special_purpose_res.end = 0;
> + }
> +}
> +
> static int configure_afu(struct ocxl_afu *afu, u8 afu_idx, struct pci_dev *dev)
> {
> int rc;
> @@ -251,6 +300,7 @@ static int configure_afu(struct ocxl_afu *afu, u8 afu_idx, struct pci_dev *dev)
>
> static void deconfigure_afu(struct ocxl_afu *afu)
> {
> + unmap_lpc_mem(afu);
Hmm. This breaks the existing balance between configure_afu and deconfigure_afu.
Given comments below on why we don't do map_lpc_mem in the afu bring up
(as it's a shared operation) it seems to me that we should be doing this
outside of the afu deconfigure. Perhaps ocxl_function_close is appropriate?
I don't know this infrastructure well enough to be sure.
If it does need to be here, then a comment to give more info on
why would be great!
> unmap_mmio_areas(afu);
> reclaim_afu_pasid(afu);
> reclaim_afu_actag(afu);
> diff --git a/drivers/misc/ocxl/ocxl_internal.h b/drivers/misc/ocxl/ocxl_internal.h
> index 20b417e00949..9f4b47900e62 100644
> --- a/drivers/misc/ocxl/ocxl_internal.h
> +++ b/drivers/misc/ocxl/ocxl_internal.h
> @@ -52,6 +52,9 @@ struct ocxl_afu {
> void __iomem *global_mmio_ptr;
> u64 pp_mmio_start;
> void *private;
> + u64 lpc_base_addr; /* Covers both LPC & special purpose memory */
> + struct resource lpc_res;
> + struct resource special_purpose_res;
> };
>
> enum ocxl_context_status {
> diff --git a/include/misc/ocxl.h b/include/misc/ocxl.h
> index 06dd5839e438..6f7c02f0d5e3 100644
> --- a/include/misc/ocxl.h
> +++ b/include/misc/ocxl.h
> @@ -212,6 +212,24 @@ int ocxl_irq_set_handler(struct ocxl_context *ctx, int irq_id,
>
> // AFU Metadata
>
> +/**
> + * Map the LPC system & special purpose memory for an AFU
> + *
> + * Do not call this during device discovery, as there may me multiple
> + * devices on a link, and the memory is mapped for the whole link, not
> + * just one device. It should only be called after all devices have
> + * registered their memory on the link.
> + *
> + * afu: The AFU that has the LPC memory to map
Run kernel-doc over these files and fix all the errors + warnings.
@afu: ..
and missing function name etc.
> + */
> +extern int ocxl_afu_map_lpc_mem(struct ocxl_afu *afu);
> +
> +/**
> + * Get the physical address range of LPC memory for an AFU
> + * afu: The AFU associated with the LPC memory
> + */
> +extern struct resource *ocxl_afu_lpc_mem(struct ocxl_afu *afu);
> +
> /**
> * Get a pointer to the config for an AFU
> *
^ permalink raw reply
* Re: [PATCH v2 08/27] ocxl: Save the device serial number in ocxl_fn
From: Jonathan Cameron @ 2020-02-03 12:53 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Keith Busch,
Masahiro Yamada, Paul Mackerras, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Vishal Verma, Mahesh Salgaonkar, Krzysztof Kozlowski,
Anju T Sudhakar, alastair, Andrew Donnellan, Arnd Bergmann,
Greg Kurz, Nicholas Piggin, Cédric Le Goater, Dan Williams,
Hari Bathini, linux-mm, Greg Kroah-Hartman, linux-kernel,
Frederic Barrat, Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <20191203034655.51561-9-alastair@au1.ibm.com>
On Tue, 3 Dec 2019 14:46:36 +1100
Alastair D'Silva <alastair@au1.ibm.com> wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> This patch retrieves the serial number of the card and makes it available
> to consumers of the ocxl driver via the ocxl_fn struct.
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> Acked-by: Frederic Barrat <fbarrat@linux.ibm.com>
> Acked-by: Andrew Donnellan <ajd@linux.ibm.com>
> ---
> drivers/misc/ocxl/config.c | 46 ++++++++++++++++++++++++++++++++++++++
> include/misc/ocxl.h | 1 +
> 2 files changed, 47 insertions(+)
>
> diff --git a/drivers/misc/ocxl/config.c b/drivers/misc/ocxl/config.c
> index fb0c3b6f8312..a9203c309365 100644
> --- a/drivers/misc/ocxl/config.c
> +++ b/drivers/misc/ocxl/config.c
> @@ -71,6 +71,51 @@ static int find_dvsec_afu_ctrl(struct pci_dev *dev, u8 afu_idx)
> return 0;
> }
>
> +/**
Make sure anything you mark as kernel doc with /** is valid
kernel-doc.
> + * Find a related PCI device (function 0)
> + * @device: PCI device to match
> + *
> + * Returns a pointer to the related device, or null if not found
> + */
> +static struct pci_dev *get_function_0(struct pci_dev *dev)
> +{
> + unsigned int devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0); // Look for function 0
Not sure the trailing comment adds much.
I'd personally not bother with this wrapper at all and just call
the pci functions directly where needed.
> +
> + return pci_get_domain_bus_and_slot(pci_domain_nr(dev->bus),
> + dev->bus->number, devfn);
> +}
> +
> +static void read_serial(struct pci_dev *dev, struct ocxl_fn_config *fn)
> +{
> + u32 low, high;
> + int pos;
> +
> + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DSN);
> + if (pos) {
> + pci_read_config_dword(dev, pos + 0x04, &low);
> + pci_read_config_dword(dev, pos + 0x08, &high);
> +
> + fn->serial = low | ((u64)high) << 32;
> +
> + return;
> + }
> +
> + if (PCI_FUNC(dev->devfn) != 0) {
> + struct pci_dev *related = get_function_0(dev);
> +
> + if (!related) {
> + fn->serial = 0;
> + return;
> + }
> +
> + read_serial(related, fn);
> + pci_dev_put(related);
> + return;
> + }
> +
> + fn->serial = 0;
> +}
> +
> static void read_pasid(struct pci_dev *dev, struct ocxl_fn_config *fn)
> {
> u16 val;
> @@ -208,6 +253,7 @@ int ocxl_config_read_function(struct pci_dev *dev, struct ocxl_fn_config *fn)
> int rc;
>
> read_pasid(dev, fn);
> + read_serial(dev, fn);
>
> rc = read_dvsec_tl(dev, fn);
> if (rc) {
> diff --git a/include/misc/ocxl.h b/include/misc/ocxl.h
> index 6f7c02f0d5e3..9843051c3c5b 100644
> --- a/include/misc/ocxl.h
> +++ b/include/misc/ocxl.h
> @@ -46,6 +46,7 @@ struct ocxl_fn_config {
> int dvsec_afu_info_pos; /* offset of the AFU information DVSEC */
> s8 max_pasid_log;
> s8 max_afu_index;
> + u64 serial;
> };
>
> enum ocxl_endian {
^ permalink raw reply
* Re: [PATCH v2 10/27] nvdimm: Add driver for OpenCAPI Storage Class Memory
From: Jonathan Cameron @ 2020-02-03 13:20 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Keith Busch,
Masahiro Yamada, Paul Mackerras, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Vishal Verma, Mahesh Salgaonkar, Krzysztof Kozlowski,
Anju T Sudhakar, alastair, Andrew Donnellan, Arnd Bergmann,
Greg Kurz, Nicholas Piggin, Cédric Le Goater, Dan Williams,
Hari Bathini, linux-mm, Greg Kroah-Hartman, linux-kernel,
Frederic Barrat, Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <20191203034655.51561-11-alastair@au1.ibm.com>
On Tue, 3 Dec 2019 14:46:38 +1100
Alastair D'Silva <alastair@au1.ibm.com> wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> This driver exposes LPC memory on OpenCAPI SCM cards
> as an NVDIMM, allowing the existing nvram infrastructure
> to be used.
>
> Namespace metadata is stored on the media itself, so
> scm_reserve_metadata() maps 1 section's worth of PMEM storage
> at the start to hold this. The rest of the PMEM range is registered
> with libnvdimm as an nvdimm. scm_ndctl_config_read/write/size() provide
> callbacks to libnvdimm to access the metadata.
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
Hi Alastair,
A few bits and bobs inline.
Thanks,
Jonathan
> ---
> drivers/nvdimm/Kconfig | 2 +
> drivers/nvdimm/Makefile | 2 +-
> drivers/nvdimm/ocxl/Kconfig | 15 +
> drivers/nvdimm/ocxl/Makefile | 7 +
> drivers/nvdimm/ocxl/scm.c | 519 +++++++++++++++++++++++++++++
> drivers/nvdimm/ocxl/scm_internal.h | 28 ++
> 6 files changed, 572 insertions(+), 1 deletion(-)
> create mode 100644 drivers/nvdimm/ocxl/Kconfig
> create mode 100644 drivers/nvdimm/ocxl/Makefile
> create mode 100644 drivers/nvdimm/ocxl/scm.c
> create mode 100644 drivers/nvdimm/ocxl/scm_internal.h
>
> diff --git a/drivers/nvdimm/Kconfig b/drivers/nvdimm/Kconfig
> index 36af7af6b7cf..d1bab36da61c 100644
> --- a/drivers/nvdimm/Kconfig
> +++ b/drivers/nvdimm/Kconfig
> @@ -130,4 +130,6 @@ config NVDIMM_TEST_BUILD
> core devm_memremap_pages() implementation and other
> infrastructure.
>
> +source "drivers/nvdimm/ocxl/Kconfig"
> +
> endif
> diff --git a/drivers/nvdimm/Makefile b/drivers/nvdimm/Makefile
> index 29203f3d3069..e33492128042 100644
> --- a/drivers/nvdimm/Makefile
> +++ b/drivers/nvdimm/Makefile
> @@ -1,5 +1,5 @@
> # SPDX-License-Identifier: GPL-2.0
> -obj-$(CONFIG_LIBNVDIMM) += libnvdimm.o
> +obj-$(CONFIG_LIBNVDIMM) += libnvdimm.o ocxl/
> obj-$(CONFIG_BLK_DEV_PMEM) += nd_pmem.o
> obj-$(CONFIG_ND_BTT) += nd_btt.o
> obj-$(CONFIG_ND_BLK) += nd_blk.o
> diff --git a/drivers/nvdimm/ocxl/Kconfig b/drivers/nvdimm/ocxl/Kconfig
> new file mode 100644
> index 000000000000..24099b300f5e
> --- /dev/null
> +++ b/drivers/nvdimm/ocxl/Kconfig
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +if LIBNVDIMM
> +
> +config OCXL_SCM
> + tristate "OpenCAPI Storage Class Memory"
> + depends on LIBNVDIMM && PPC_POWERNV && PCI && EEH
> + select ZONE_DEVICE
> + select OCXL
> + help
> + Exposes devices that implement the OpenCAPI Storage Class Memory
> + specification as persistent memory regions.
> +
> + Select N if unsure.
> +
> +endif
> diff --git a/drivers/nvdimm/ocxl/Makefile b/drivers/nvdimm/ocxl/Makefile
> new file mode 100644
> index 000000000000..74a1bd98848e
> --- /dev/null
> +++ b/drivers/nvdimm/ocxl/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +ccflags-$(CONFIG_PPC_WERROR) += -Werror
> +
> +obj-$(CONFIG_OCXL_SCM) += ocxlscm.o
> +
> +ocxlscm-y := scm.o
> diff --git a/drivers/nvdimm/ocxl/scm.c b/drivers/nvdimm/ocxl/scm.c
> new file mode 100644
> index 000000000000..571058a9e7b8
> --- /dev/null
> +++ b/drivers/nvdimm/ocxl/scm.c
> @@ -0,0 +1,519 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// Copyright 2019 IBM Corp.
> +
> +/*
> + * A driver for Storage Class Memory, connected via OpenCAPI
> + */
> +
> +#include <linux/module.h>
> +#include <misc/ocxl.h>
> +#include <linux/ndctl.h>
> +#include <linux/mm_types.h>
> +#include <linux/memory_hotplug.h>
> +#include "scm_internal.h"
> +
> +
> +static const struct pci_device_id scm_pci_tbl[] = {
> + { PCI_DEVICE(PCI_VENDOR_ID_IBM, 0x0625), },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(pci, scm_pci_tbl);
> +
> +#define SCM_NUM_MINORS 256 // Total to reserve
> +
> +static dev_t scm_dev;
> +static struct class *scm_class;
> +static struct mutex minors_idr_lock;
> +static struct idr minors_idr;
> +
> +static const struct attribute_group *scm_pmem_attribute_groups[] = {
> + &nvdimm_bus_attribute_group,
> + NULL,
> +};
> +
> +static const struct attribute_group *scm_pmem_region_attribute_groups[] = {
> + &nd_region_attribute_group,
> + &nd_device_attribute_group,
> + &nd_mapping_attribute_group,
> + &nd_numa_attribute_group,
> + NULL,
> +};
> +
> +/**
> + * scm_ndctl_config_write() - Handle a ND_CMD_SET_CONFIG_DATA command from ndctl
> + * @scm_data: the SCM metadata
> + * @command: the incoming data to write
> + * Return: 0 on success, negative on failure
> + */
> +static int scm_ndctl_config_write(struct scm_data *scm_data,
> + struct nd_cmd_set_config_hdr *command)
> +{
> + if (command->in_offset + command->in_length > SCM_LABEL_AREA_SIZE)
> + return -EINVAL;
> +
> + memcpy_flushcache(scm_data->metadata_addr + command->in_offset, command->in_buf,
> + command->in_length);
> +
> + return 0;
> +}
> +
> +/**
> + * scm_ndctl_config_read() - Handle a ND_CMD_GET_CONFIG_DATA command from ndctl
> + * @scm_data: the SCM metadata
> + * @command: the read request
> + * Return: 0 on success, negative on failure
> + */
> +static int scm_ndctl_config_read(struct scm_data *scm_data,
> + struct nd_cmd_get_config_data_hdr *command)
> +{
> + if (command->in_offset + command->in_length > SCM_LABEL_AREA_SIZE)
> + return -EINVAL;
> +
> + memcpy_mcsafe(command->out_buf, scm_data->metadata_addr + command->in_offset,
> + command->in_length);
> +
> + return 0;
> +}
> +
> +/**
> + * scm_ndctl_config_size() - Handle a ND_CMD_GET_CONFIG_SIZE command from ndctl
> + * @scm_data: the SCM metadata
> + * @command: the read request
> + * Return: 0 on success, negative on failure
> + */
> +static int scm_ndctl_config_size(struct nd_cmd_get_config_size *command)
> +{
> + command->status = 0;
> + command->config_size = SCM_LABEL_AREA_SIZE;
> + command->max_xfer = PAGE_SIZE;
> +
> + return 0;
> +}
> +
> +static int scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
> + struct nvdimm *nvdimm,
> + unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc)
> +{
> + struct scm_data *scm_data = container_of(nd_desc, struct scm_data, bus_desc);
> +
> + switch (cmd) {
> + case ND_CMD_GET_CONFIG_SIZE:
> + *cmd_rc = scm_ndctl_config_size(buf);
> + return 0;
> +
> + case ND_CMD_GET_CONFIG_DATA:
> + *cmd_rc = scm_ndctl_config_read(scm_data, buf);
> + return 0;
> +
> + case ND_CMD_SET_CONFIG_DATA:
> + *cmd_rc = scm_ndctl_config_write(scm_data, buf);
> + return 0;
> +
> + default:
> + return -ENOTTY;
> + }
> +}
> +
> +static ssize_t serial_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct nvdimm *nvdimm = to_nvdimm(dev);
> + struct scm_data *scm_data = nvdimm_provider_data(nvdimm);
> + const struct ocxl_fn_config *config = ocxl_function_config(scm_data->ocxl_fn);
> +
> + return sprintf(buf, "0x%llx\n", config->serial);
> +}
> +static DEVICE_ATTR_RO(serial);
> +
> +static struct attribute *scm_dimm_attributes[] = {
> + &dev_attr_serial.attr,
> + NULL,
> +};
> +
> +static umode_t scm_dimm_attr_visible(struct kobject *kobj,
> + struct attribute *a, int n)
> +{
> + return a->mode;
> +}
> +
> +static const struct attribute_group scm_dimm_attribute_group = {
> + .name = "ocxl",
> + .attrs = scm_dimm_attributes,
> + .is_visible = scm_dimm_attr_visible,
> +};
> +
> +static const struct attribute_group *scm_dimm_attribute_groups[] = {
> + &nvdimm_attribute_group,
> + &nd_device_attribute_group,
> + &scm_dimm_attribute_group,
> + NULL,
> +};
> +
> +/**
> + * scm_reserve_metadata() - Reserve space for nvdimm metadata
> + * @scm_data: The SCM device data
> + * @lpc_mem: The resource representing the LPC memory of the SCM device
> + */
> +static int scm_reserve_metadata(struct scm_data *scm_data,
> + struct resource *lpc_mem)
> +{
> + scm_data->metadata_addr = devm_memremap(&scm_data->dev, lpc_mem->start,
> + SCM_LABEL_AREA_SIZE, MEMREMAP_WB);
> + if (IS_ERR(scm_data->metadata_addr))
> + return PTR_ERR(scm_data->metadata_addr);
> +
> + return 0;
> +}
> +
> +/**
> + * scm_register_lpc_mem() - Discover persistent memory on a device and register it with the NVDIMM subsystem
> + * @scm_data: The SCM device data
> + * Return: 0 on success
> + */
> +static int scm_register_lpc_mem(struct scm_data *scm_data)
> +{
> + struct nd_region_desc region_desc;
> + struct nd_mapping_desc nd_mapping_desc;
> + struct resource *lpc_mem;
> + const struct ocxl_afu_config *config;
> + const struct ocxl_fn_config *fn_config;
> + int rc;
> + unsigned long nvdimm_cmd_mask = 0;
> + unsigned long nvdimm_flags = 0;
> + int target_node;
> + char serial[16+1];
> +
> + // Set up the reserved metadata area
> + rc = ocxl_afu_map_lpc_mem(scm_data->ocxl_afu);
> + if (rc < 0)
> + return rc;
> +
> + lpc_mem = ocxl_afu_lpc_mem(scm_data->ocxl_afu);
> + if (lpc_mem == NULL || lpc_mem->start == 0)
> + return -EINVAL;
> +
> + config = ocxl_afu_config(scm_data->ocxl_afu);
> + fn_config = ocxl_function_config(scm_data->ocxl_fn);
> +
> + rc = scm_reserve_metadata(scm_data, lpc_mem);
> + if (rc)
> + return rc;
> +
> + scm_data->bus_desc.attr_groups = scm_pmem_attribute_groups;
> + scm_data->bus_desc.provider_name = "ocxl-scm";
> + scm_data->bus_desc.ndctl = scm_ndctl;
> + scm_data->bus_desc.module = THIS_MODULE;
> +
> + scm_data->nvdimm_bus = nvdimm_bus_register(&scm_data->dev,
> + &scm_data->bus_desc);
odd alignment.
> + if (!scm_data->nvdimm_bus)
> + return -EINVAL;
> +
> + scm_data->scm_res.start = (u64)lpc_mem->start + SCM_LABEL_AREA_SIZE;
> + scm_data->scm_res.end = (u64)lpc_mem->start + config->lpc_mem_size - 1;
> + scm_data->scm_res.name = "SCM persistent memory";
> +
> + set_bit(ND_CMD_GET_CONFIG_SIZE, &nvdimm_cmd_mask);
> + set_bit(ND_CMD_GET_CONFIG_DATA, &nvdimm_cmd_mask);
> + set_bit(ND_CMD_SET_CONFIG_DATA, &nvdimm_cmd_mask);
> +
> + set_bit(NDD_ALIASING, &nvdimm_flags);
> +
> + snprintf(serial, sizeof(serial), "%llx", fn_config->serial);
> + nd_mapping_desc.nvdimm = nvdimm_create(scm_data->nvdimm_bus, scm_data,
> + scm_dimm_attribute_groups,
> + nvdimm_flags, nvdimm_cmd_mask,
> + 0, NULL);
> + if (!nd_mapping_desc.nvdimm)
> + return -ENOMEM;
> +
> + if (nvdimm_bus_check_dimm_count(scm_data->nvdimm_bus, 1))
> + return -EINVAL;
> +
> + nd_mapping_desc.start = scm_data->scm_res.start;
> + nd_mapping_desc.size = resource_size(&scm_data->scm_res);
> + nd_mapping_desc.position = 0;
> +
> + scm_data->nd_set.cookie1 = fn_config->serial + 1; // allow for empty serial
> + scm_data->nd_set.cookie2 = fn_config->serial + 1;
> +
> + target_node = of_node_to_nid(scm_data->pdev->dev.of_node);
> +
> + memset(®ion_desc, 0, sizeof(region_desc));
> + region_desc.res = &scm_data->scm_res;
> + region_desc.attr_groups = scm_pmem_region_attribute_groups;
> + region_desc.numa_node = NUMA_NO_NODE;
> + region_desc.target_node = target_node;
> + region_desc.num_mappings = 1;
> + region_desc.mapping = &nd_mapping_desc;
> + region_desc.nd_set = &scm_data->nd_set;
> +
> + set_bit(ND_REGION_PAGEMAP, ®ion_desc.flags);
> + /*
> + * NB: libnvdimm copies the data from ndr_desc into it's own
> + * structures so passing a stack pointer is fine.
> + */
> + scm_data->nd_region = nvdimm_pmem_region_create(scm_data->nvdimm_bus,
> + ®ion_desc);
> + if (!scm_data->nd_region)
> + return -EINVAL;
> +
> + dev_info(&scm_data->dev,
> + "Onlining %lluMB of persistent memory\n",
> + nd_mapping_desc.size / SZ_1M);
> +
> + return 0;
> +}
> +
> +/**
> + * allocate_scm_minor() - Allocate a minor number to use for an SCM device
> + * @scm_data: The SCM device to associate the minor with
> + * Return: the allocated minor number
> + */
> +static int allocate_scm_minor(struct scm_data *scm_data)
> +{
> + int minor;
> +
> + mutex_lock(&minors_idr_lock);
> + minor = idr_alloc(&minors_idr, scm_data, 0, SCM_NUM_MINORS, GFP_KERNEL);
> + mutex_unlock(&minors_idr_lock);
> + return minor;
> +}
> +
> +static void free_scm_minor(struct scm_data *scm_data)
> +{
> + mutex_lock(&minors_idr_lock);
> + idr_remove(&minors_idr, MINOR(scm_data->dev.devt));
> + mutex_unlock(&minors_idr_lock);
> +}
> +
> +/**
> + * free_scm() - Free all members of an SCM struct
> + * @scm_data: the SCM metadata to clear
> + */
> +static void free_scm(struct scm_data *scm_data)
> +{
> + int rc;
> +
> + if (scm_data->nvdimm_bus)
> + nvdimm_bus_unregister(scm_data->nvdimm_bus);
> +
> + free_scm_minor(scm_data);
> +
> + if (scm_data->metadata_addr)
> + devm_memunmap(&scm_data->dev, scm_data->metadata_addr);
> +
> + if (scm_data->ocxl_context) {
> + rc = ocxl_context_detach(scm_data->ocxl_context);
> + if (rc == -EBUSY)
> + dev_warn(&scm_data->dev, "Timeout detaching ocxl context\n");
> + else
> + ocxl_context_free(scm_data->ocxl_context);
> +
> + }
> +
> + if (scm_data->ocxl_afu)
> + ocxl_afu_put(scm_data->ocxl_afu);
> +
> + if (scm_data->ocxl_fn)
> + ocxl_function_close(scm_data->ocxl_fn);
> +
> + kfree(scm_data);
> +}
> +
> +/**
> + * free_scm_dev - Free an SCM device
> + * @dev: The device struct
> + */
> +static void free_scm_dev(struct device *dev)
> +{
> + struct scm_data *scm_data = container_of(dev, struct scm_data, dev);
> +
> + free_scm(scm_data);
> +}
> +
> +/**
> + * scm_register - Register an SCM device with the kernel
> + * @scm_data: the SCM metadata
> + * Return: 0 on success, negative on failure
> + */
> +static int scm_register(struct scm_data *scm_data)
> +{
> + int rc;
> + int minor = allocate_scm_minor(scm_data);
> +
> + if (minor < 0)
> + return minor;
> +
> + scm_data->dev.release = free_scm_dev;
> + rc = dev_set_name(&scm_data->dev, "ocxl-scm%d", minor);
> + if (rc < 0)
> + return rc;
> +
> + scm_data->dev.devt = MKDEV(MAJOR(scm_dev), minor);
> + scm_data->dev.class = scm_class;
> + scm_data->dev.parent = &scm_data->pdev->dev;
> +
> + rc = device_register(&scm_data->dev);
> + return rc;
return device_register(&scm_data->dev);
Assuming nothing else is added inbetween in later patches...
If it is then ignore this one.
> +}
> +
> +/**
> + * scm_remove() - Free an OpenCAPI Storage Class Memory device
> + * @pdev: the PCI device information struct
> + */
> +static void scm_remove(struct pci_dev *pdev)
> +{
> + if (PCI_FUNC(pdev->devfn) == 0) {
> + struct scm_function_0 *scm_func_0 = pci_get_drvdata(pdev);
> +
> + if (scm_func_0) {
> + ocxl_function_close(scm_func_0->ocxl_fn);
> + scm_func_0->ocxl_fn = NULL;
> + }
> + } else {
> + struct scm_data *scm_data = pci_get_drvdata(pdev);
> +
> + if (scm_data)
> + device_unregister(&scm_data->dev);
> + }
> +}
> +
> +/**
> + * scm_probe_function_0 - Set up function 0 for an OpenCAPI Storage Class Memory device
Overly long line + not consistent on () after function name.
IIRC either () or not is fine, but should be consistent in a gven file.
> + * This is important as it enables templates higher than 0 across all other functions,
> + * which in turn enables higher bandwidth accesses
> + * which in turn enables higher bandwidth accesses
Repeated line.
> + * @pdev: the PCI device information struct
> + * Return: 0 on success, negative on failure
> + */
> +static int scm_probe_function_0(struct pci_dev *pdev)
> +{
> + struct scm_function_0 *scm_func_0 = NULL;
> + struct ocxl_fn *fn;
> +
> + scm_func_0 = kzalloc(sizeof(*scm_func_0), GFP_KERNEL);
> + if (!scm_func_0)
> + return -ENOMEM;
> +
> + scm_func_0->pdev = pdev;
> + fn = ocxl_function_open(pdev);
> + if (IS_ERR(fn)) {
> + kfree(scm_func_0);
> + dev_err(&pdev->dev, "failed to open OCXL function\n");
> + return PTR_ERR(fn);
> + }
> + scm_func_0->ocxl_fn = fn;
> +
> + pci_set_drvdata(pdev, scm_func_0);
> +
> + return 0;
> +}
> +
> +/**
> + * scm_probe - Init an OpenCAPI Storage Class Memory device
> + * @pdev: the PCI device information struct
> + * @ent: The entry from scm_pci_tbl
> + * Return: 0 on success, negative on failure
> + */
> +static int scm_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +{
> + struct scm_data *scm_data = NULL;
Always set in paths that use it.
> +
> + if (PCI_FUNC(pdev->devfn) == 0)
> + return scm_probe_function_0(pdev);
> + else if (PCI_FUNC(pdev->devfn) != 1)
> + return 0;
> +
> + scm_data = kzalloc(sizeof(*scm_data), GFP_KERNEL);
> + if (!scm_data) {
> + dev_err(&pdev->dev, "Could not allocate SCM metadata\n");
> + goto err;
> + }
> + scm_data->pdev = pdev;
> +
> + pci_set_drvdata(pdev, scm_data);
> +
> + scm_data->ocxl_fn = ocxl_function_open(pdev);
> + if (IS_ERR(scm_data->ocxl_fn)) {
> + kfree(scm_data);
> + scm_data = NULL;
Doesn't seem like scm_data is used anywhere in the rror path..
> + pci_set_drvdata(pdev, NULL);
> + dev_err(&pdev->dev, "failed to open OCXL function\n");
> + goto err;
> + }
> +
> + scm_data->ocxl_afu = ocxl_function_fetch_afu(scm_data->ocxl_fn, 0);
> + if (scm_data->ocxl_afu == NULL) {
> + dev_err(&pdev->dev, "Could not get OCXL AFU from function\n");
> + goto err;
The comment below suggests to me that free_scm will only be called if we succeed
in scm_register? If so isn't there more error handling to be done until that
happens?
> + }
> +
> + ocxl_afu_get(scm_data->ocxl_afu);
> +
> + if (scm_register(scm_data) < 0) {
> + dev_err(&pdev->dev, "Could not register SCM device with the kernel\n");
> + goto err;
> + }
> +
> + // Resources allocated below here are cleaned up in the release handler
> +
> + if (ocxl_context_alloc(&scm_data->ocxl_context, scm_data->ocxl_afu, NULL)) {
> + dev_err(&pdev->dev, "Could not allocate OCXL context\n");
> + goto err;
> + }
> +
> + if (ocxl_context_attach(scm_data->ocxl_context, 0, NULL)) {
> + dev_err(&pdev->dev, "Could not attach ocxl context\n");
> + goto err;
> + }
> +
> + if (scm_register_lpc_mem(scm_data)) {
> + dev_err(&pdev->dev, "Could not register OCXL SCM memory with libnvdimm\n");
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + /*
> + * Further cleanup is done in the release handler via free_scm()
> + * This allows us to keep the character device live to handle IOCTLs to
> + * investigate issues if the card has an error
> + */
> +
> + dev_err(&pdev->dev,
> + "Error detected, will not register storage class memory\n");
> + return -ENXIO;
Probably better to return more specific errors from the various error paths.
-ENOMEM etc.
> +}
> +
> +static struct pci_driver scm_pci_driver = {
> + .name = "ocxl-scm",
> + .id_table = scm_pci_tbl,
> + .probe = scm_probe,
> + .remove = scm_remove,
> + .shutdown = scm_remove,
> +};
> +
> +static int __init scm_init(void)
> +{
> + int rc = 0;
> +
> + rc = pci_register_driver(&scm_pci_driver);
> + if (rc)
> + return rc;
> +
> + return 0;
> +}
> +
> +static void scm_exit(void)
> +{
> + pci_unregister_driver(&scm_pci_driver);
> +}
> +
> +module_init(scm_init);
> +module_exit(scm_exit);
> +
> +MODULE_DESCRIPTION("Storage Class Memory");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/nvdimm/ocxl/scm_internal.h b/drivers/nvdimm/ocxl/scm_internal.h
> new file mode 100644
> index 000000000000..6340012e0f8a
> --- /dev/null
> +++ b/drivers/nvdimm/ocxl/scm_internal.h
> @@ -0,0 +1,28 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// Copyright 2019 IBM Corp.
> +
> +#include <linux/pci.h>
> +#include <misc/ocxl.h>
> +#include <linux/libnvdimm.h>
> +#include <linux/mm.h>
> +
> +#define SCM_LABEL_AREA_SIZE (1UL << PA_SECTION_SHIFT)
> +
> +struct scm_function_0 {
> + struct pci_dev *pdev;
> + struct ocxl_fn *ocxl_fn;
> +};
> +
> +struct scm_data {
> + struct device dev;
> + struct pci_dev *pdev;
> + struct ocxl_fn *ocxl_fn;
> + struct nd_interleave_set nd_set;
> + struct nvdimm_bus_descriptor bus_desc;
> + struct nvdimm_bus *nvdimm_bus;
> + struct ocxl_afu *ocxl_afu;
> + struct ocxl_context *ocxl_context;
> + void *metadata_addr;
> + struct resource scm_res;
> + struct nd_region *nd_region;
> +};
^ permalink raw reply
* Re: [PATCH v2 12/27] nvdimm/ocxl: Read the capability registers & wait for device ready
From: Jonathan Cameron @ 2020-02-03 13:23 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Keith Busch,
Masahiro Yamada, Paul Mackerras, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Vishal Verma, Mahesh Salgaonkar, Krzysztof Kozlowski,
Anju T Sudhakar, alastair, Andrew Donnellan, Arnd Bergmann,
Greg Kurz, Nicholas Piggin, Cédric Le Goater, Dan Williams,
Hari Bathini, linux-mm, Greg Kroah-Hartman, linux-kernel,
Frederic Barrat, Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <20191203034655.51561-13-alastair@au1.ibm.com>
On Tue, 3 Dec 2019 14:46:40 +1100
Alastair D'Silva <alastair@au1.ibm.com> wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> This patch reads timeouts & firmware version from the controller, and
> uses those timeouts to wait for the controller to report that it is ready
> before handing the memory over to libnvdimm.
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
> drivers/nvdimm/ocxl/Makefile | 2 +-
> drivers/nvdimm/ocxl/scm.c | 84 ++++++++++++++++++++++++++++++
> drivers/nvdimm/ocxl/scm_internal.c | 19 +++++++
> drivers/nvdimm/ocxl/scm_internal.h | 24 +++++++++
> 4 files changed, 128 insertions(+), 1 deletion(-)
> create mode 100644 drivers/nvdimm/ocxl/scm_internal.c
>
> diff --git a/drivers/nvdimm/ocxl/Makefile b/drivers/nvdimm/ocxl/Makefile
> index 74a1bd98848e..9b6e31f0eb3e 100644
> --- a/drivers/nvdimm/ocxl/Makefile
> +++ b/drivers/nvdimm/ocxl/Makefile
> @@ -4,4 +4,4 @@ ccflags-$(CONFIG_PPC_WERROR) += -Werror
>
> obj-$(CONFIG_OCXL_SCM) += ocxlscm.o
>
> -ocxlscm-y := scm.o
> +ocxlscm-y := scm.o scm_internal.o
> diff --git a/drivers/nvdimm/ocxl/scm.c b/drivers/nvdimm/ocxl/scm.c
> index 571058a9e7b8..8088f65c289e 100644
> --- a/drivers/nvdimm/ocxl/scm.c
> +++ b/drivers/nvdimm/ocxl/scm.c
> @@ -7,6 +7,7 @@
>
> #include <linux/module.h>
> #include <misc/ocxl.h>
> +#include <linux/delay.h>
> #include <linux/ndctl.h>
> #include <linux/mm_types.h>
> #include <linux/memory_hotplug.h>
> @@ -266,6 +267,30 @@ static int scm_register_lpc_mem(struct scm_data *scm_data)
> return 0;
> }
>
> +/**
> + * scm_is_usable() - Is a controller usable?
> + * @scm_data: a pointer to the SCM device data
> + * Return: true if the controller is usable
> + */
> +static bool scm_is_usable(const struct scm_data *scm_data)
> +{
> + u64 chi = 0;
> + int rc = scm_chi(scm_data, &chi);
> +
> + if (!(chi & GLOBAL_MMIO_CHI_CRDY)) {
> + dev_err(&scm_data->dev, "SCM controller is not ready.\n");
> + return false;
> + }
> +
> + if (!(chi & GLOBAL_MMIO_CHI_MA)) {
> + dev_err(&scm_data->dev,
> + "SCM controller does not have memory available.\n");
> + return false;
> + }
> +
> + return true;
> +}
> +
> /**
> * allocate_scm_minor() - Allocate a minor number to use for an SCM device
> * @scm_data: The SCM device to associate the minor with
> @@ -380,6 +405,48 @@ static void scm_remove(struct pci_dev *pdev)
> }
> }
>
> +/**
> + * read_device_metadata() - Retrieve config information from the AFU and save it for future use
> + * @scm_data: the SCM metadata
> + * Return: 0 on success, negative on failure
> + */
> +static int read_device_metadata(struct scm_data *scm_data)
> +{
> + u64 val;
> + int rc;
> +
> + rc = ocxl_global_mmio_read64(scm_data->ocxl_afu, GLOBAL_MMIO_CCAP0,
> + OCXL_LITTLE_ENDIAN, &val);
> + if (rc)
> + return rc;
> +
> + scm_data->scm_revision = val & 0xFFFF;
> + scm_data->read_latency = (val >> 32) & 0xFF;
> + scm_data->readiness_timeout = (val >> 48) & 0xff;
> + scm_data->memory_available_timeout = val >> 52;
This overlaps with the masked region for readiness_timeout. I'll guess the maks
on that should be 0xF.
> +
> + rc = ocxl_global_mmio_read64(scm_data->ocxl_afu, GLOBAL_MMIO_CCAP1,
> + OCXL_LITTLE_ENDIAN, &val);
> + if (rc)
> + return rc;
> +
> + scm_data->max_controller_dump_size = val & 0xFFFFFFFF;
> +
> + // Extract firmware version text
> + rc = ocxl_global_mmio_read64(scm_data->ocxl_afu, GLOBAL_MMIO_FWVER,
> + OCXL_HOST_ENDIAN, (u64 *)scm_data->fw_version);
> + if (rc)
> + return rc;
> +
> + scm_data->fw_version[8] = '\0';
> +
> + dev_info(&scm_data->dev,
> + "Firmware version '%s' SCM revision %d:%d\n", scm_data->fw_version,
> + scm_data->scm_revision >> 4, scm_data->scm_revision & 0x0F);
> +
> + return 0;
> +}
> +
> /**
> * scm_probe_function_0 - Set up function 0 for an OpenCAPI Storage Class Memory device
> * This is important as it enables templates higher than 0 across all other functions,
> @@ -420,6 +487,8 @@ static int scm_probe_function_0(struct pci_dev *pdev)
> static int scm_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> struct scm_data *scm_data = NULL;
> + int elapsed;
> + u16 timeout;
>
> if (PCI_FUNC(pdev->devfn) == 0)
> return scm_probe_function_0(pdev);
> @@ -469,6 +538,21 @@ static int scm_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> goto err;
> }
>
> + if (read_device_metadata(scm_data)) {
> + dev_err(&pdev->dev, "Could not read SCM device metadata\n");
> + goto err;
> + }
> +
> + elapsed = 0;
> + timeout = scm_data->readiness_timeout + scm_data->memory_available_timeout;
> + while (!scm_is_usable(scm_data)) {
> + if (elapsed++ > timeout) {
> + dev_warn(&scm_data->dev, "SCM ready timeout.\n");
> + goto err;
> + }
> +
> + msleep(1000);
> + }
> if (scm_register_lpc_mem(scm_data)) {
> dev_err(&pdev->dev, "Could not register OCXL SCM memory with libnvdimm\n");
> goto err;
> diff --git a/drivers/nvdimm/ocxl/scm_internal.c b/drivers/nvdimm/ocxl/scm_internal.c
> new file mode 100644
> index 000000000000..72d3c0e7d846
> --- /dev/null
> +++ b/drivers/nvdimm/ocxl/scm_internal.c
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// Copyright 2019 IBM Corp.
> +
> +#include <misc/ocxl.h>
> +#include <linux/delay.h>
> +#include "scm_internal.h"
> +
> +int scm_chi(const struct scm_data *scm_data, u64 *chi)
> +{
> + u64 val;
> + int rc = ocxl_global_mmio_read64(scm_data->ocxl_afu, GLOBAL_MMIO_CHI,
> + OCXL_LITTLE_ENDIAN, &val);
> + if (rc)
> + return rc;
> +
> + *chi = val;
> +
> + return 0;
> +}
> diff --git a/drivers/nvdimm/ocxl/scm_internal.h b/drivers/nvdimm/ocxl/scm_internal.h
> index d6ab361f5de9..584450f55e30 100644
> --- a/drivers/nvdimm/ocxl/scm_internal.h
> +++ b/drivers/nvdimm/ocxl/scm_internal.h
> @@ -97,4 +97,28 @@ struct scm_data {
> void *metadata_addr;
> struct resource scm_res;
> struct nd_region *nd_region;
> + char fw_version[8+1];
> +
> + u32 max_controller_dump_size;
> + u16 scm_revision; // major/minor
> + u8 readiness_timeout; /* The worst case time (in seconds) that the host shall
> + * wait for the controller to become operational following a reset (CHI.CRDY).
> + */
> + u8 memory_available_timeout; /* The worst case time (in seconds) that the host shall
> + * wait for memory to become available following a reset (CHI.MA).
> + */
> +
> + u16 read_latency; /* The nominal measure of latency (in nanoseconds)
> + * associated with an unassisted read of a memory block.
> + * This represents the capability of the raw media technology without assistance
> + */
> };
> +
> +/**
> + * scm_chi() - Get the value of the CHI register
> + * @scm_data: The SCM metadata
> + * @chi: returns the CHI value
> + *
> + * Returns 0 on success, negative on error
> + */
> +int scm_chi(const struct scm_data *scm_data, u64 *chi);
^ permalink raw reply
* Re: [PATCH v2 13/27] nvdimm/ocxl: Add support for Admin commands
From: Jonathan Cameron @ 2020-02-03 14:18 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Keith Busch,
Masahiro Yamada, Paul Mackerras, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Vishal Verma, Mahesh Salgaonkar, Krzysztof Kozlowski,
Anju T Sudhakar, alastair, Andrew Donnellan, Arnd Bergmann,
Greg Kurz, Nicholas Piggin, Cédric Le Goater, Dan Williams,
Hari Bathini, linux-mm, Greg Kroah-Hartman, linux-kernel,
Frederic Barrat, Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <20191203034655.51561-14-alastair@au1.ibm.com>
On Tue, 3 Dec 2019 14:46:41 +1100
Alastair D'Silva <alastair@au1.ibm.com> wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> This patch requests the metadata required to issue admin commands, as well
> as some helper functions to construct and check the completion of the
> commands.
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
A few trivial bits inline.
Jonathan
> ---
> drivers/nvdimm/ocxl/scm.c | 67 +++++++++++++
> drivers/nvdimm/ocxl/scm_internal.c | 152 +++++++++++++++++++++++++++++
> drivers/nvdimm/ocxl/scm_internal.h | 62 ++++++++++++
> 3 files changed, 281 insertions(+)
>
> diff --git a/drivers/nvdimm/ocxl/scm.c b/drivers/nvdimm/ocxl/scm.c
> index 8088f65c289e..1e175f3c3cf2 100644
> --- a/drivers/nvdimm/ocxl/scm.c
> +++ b/drivers/nvdimm/ocxl/scm.c
> @@ -267,6 +267,58 @@ static int scm_register_lpc_mem(struct scm_data *scm_data)
> return 0;
> }
>
> +/**
> + * scm_extract_command_metadata() - Extract command data from MMIO & save it for further use
> + * @scm_data: a pointer to the SCM device data
> + * @offset: The base address of the command data structures (address of CREQO)
> + * @command_metadata: A pointer to the command metadata to populate
> + * Return: 0 on success, negative on failure
> + */
> +static int scm_extract_command_metadata(struct scm_data *scm_data, u32 offset,
> + struct command_metadata *command_metadata)
> +{
> + int rc;
> + u64 tmp;
> +
> + rc = ocxl_global_mmio_read64(scm_data->ocxl_afu, offset, OCXL_LITTLE_ENDIAN,
> + &tmp);
> + if (rc)
> + return rc;
> +
> + command_metadata->request_offset = tmp >> 32;
> + command_metadata->response_offset = tmp & 0xFFFFFFFF;
> +
> + rc = ocxl_global_mmio_read64(scm_data->ocxl_afu, offset + 8, OCXL_LITTLE_ENDIAN,
> + &tmp);
> + if (rc)
> + return rc;
> +
> + command_metadata->data_offset = tmp >> 32;
> + command_metadata->data_size = tmp & 0xFFFFFFFF;
> +
> + command_metadata->id = 0;
> +
> + return 0;
> +}
> +
> +/**
> + * scm_setup_command_metadata() - Set up the command metadata
> + * @scm_data: a pointer to the SCM device data
> + */
> +static int scm_setup_command_metadata(struct scm_data *scm_data)
> +{
> + int rc;
> +
> + mutex_init(&scm_data->admin_command.lock);
> +
> + rc = scm_extract_command_metadata(scm_data, GLOBAL_MMIO_ACMA_CREQO,
> + &scm_data->admin_command);
> + if (rc)
> + return rc;
Unless you are adding to this later in the series.
return scm_extract_command_metadata(scm_data,...)
> +
> + return 0;
> +}
> +
> /**
> * scm_is_usable() - Is a controller usable?
> * @scm_data: a pointer to the SCM device data
> @@ -276,6 +328,8 @@ static bool scm_is_usable(const struct scm_data *scm_data)
> {
> u64 chi = 0;
> int rc = scm_chi(scm_data, &chi);
> + if (rc)
> + return false;
>
> if (!(chi & GLOBAL_MMIO_CHI_CRDY)) {
> dev_err(&scm_data->dev, "SCM controller is not ready.\n");
> @@ -502,6 +556,14 @@ static int scm_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> }
> scm_data->pdev = pdev;
>
> + scm_data->timeouts[ADMIN_COMMAND_ERRLOG] = 2000; // ms
> + scm_data->timeouts[ADMIN_COMMAND_HEARTBEAT] = 100; // ms
> + scm_data->timeouts[ADMIN_COMMAND_SMART] = 100; // ms
> + scm_data->timeouts[ADMIN_COMMAND_CONTROLLER_DUMP] = 1000; // ms
> + scm_data->timeouts[ADMIN_COMMAND_CONTROLLER_STATS] = 100; // ms
> + scm_data->timeouts[ADMIN_COMMAND_SHUTDOWN] = 1000; // ms
> + scm_data->timeouts[ADMIN_COMMAND_FW_UPDATE] = 16000; // ms
> +
> pci_set_drvdata(pdev, scm_data);
>
> scm_data->ocxl_fn = ocxl_function_open(pdev);
> @@ -543,6 +605,11 @@ static int scm_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> goto err;
> }
>
> + if (scm_setup_command_metadata(scm_data)) {
> + dev_err(&pdev->dev, "Could not read OCXL command matada\n");
> + goto err;
> + }
> +
> elapsed = 0;
> timeout = scm_data->readiness_timeout + scm_data->memory_available_timeout;
> while (!scm_is_usable(scm_data)) {
> diff --git a/drivers/nvdimm/ocxl/scm_internal.c b/drivers/nvdimm/ocxl/scm_internal.c
> index 72d3c0e7d846..7b11b56863fb 100644
> --- a/drivers/nvdimm/ocxl/scm_internal.c
> +++ b/drivers/nvdimm/ocxl/scm_internal.c
> @@ -17,3 +17,155 @@ int scm_chi(const struct scm_data *scm_data, u64 *chi)
>
> return 0;
> }
> +
> +static int scm_command_request(const struct scm_data *scm_data,
> + struct command_metadata *cmd, u8 op_code)
> +{
> + u64 val = op_code;
> + int rc;
> + u8 i;
> +
> + cmd->op_code = op_code;
> + cmd->id++;
> +
> + val |= ((u64)cmd->id) << 16;
> +
> + rc = ocxl_global_mmio_write64(scm_data->ocxl_afu, cmd->request_offset,
> + OCXL_LITTLE_ENDIAN, val);
> + if (rc)
> + return rc;
> +
> + for (i = 0x08; i <= 0x38; i += 0x08) {
perhaps use sizeof(u64) to explain where the 0x08s come from.
For the 0x38, might be worth a define.
> + rc = ocxl_global_mmio_write64(scm_data->ocxl_afu,
> + cmd->request_offset + i,
> + OCXL_LITTLE_ENDIAN, 0);
> + if (rc)
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> +int scm_admin_command_request(struct scm_data *scm_data, u8 op_code)
> +{
> + u64 val;
> + int rc = ocxl_global_mmio_read64(scm_data->ocxl_afu, GLOBAL_MMIO_CHI,
> + OCXL_LITTLE_ENDIAN, &val);
> + if (rc)
> + return rc;
> +
> + return scm_command_request(scm_data, &scm_data->admin_command, op_code);
> +}
> +
> +static int scm_command_response(const struct scm_data *scm_data,
> + const struct command_metadata *cmd)
> +{
> + u64 val;
> + u16 id;
> + u8 status;
> + int rc = ocxl_global_mmio_read64(scm_data->ocxl_afu,
> + cmd->response_offset,
> + OCXL_LITTLE_ENDIAN, &val);
> + if (rc)
> + return rc;
> +
> + status = val & 0xff;
> + id = (val >> 16) & 0xffff;
> +
> + if (id != cmd->id) {
> + dev_warn(&scm_data->dev,
> + "Expected response for command %d, but received response for command %d instead.\n",
> + cmd->id, id);
> + }
> +
> + return status;
> +}
> +
> +int scm_admin_response(const struct scm_data *scm_data)
> +{
> + return scm_command_response(scm_data, &scm_data->admin_command);
> +}
> +
> +
> +int scm_admin_command_execute(const struct scm_data *scm_data)
> +{
> + return ocxl_global_mmio_set64(scm_data->ocxl_afu, GLOBAL_MMIO_HCI,
> + OCXL_LITTLE_ENDIAN, GLOBAL_MMIO_HCI_ACRW);
> +}
> +
> +static bool scm_admin_command_complete(const struct scm_data *scm_data)
> +{
> + u64 val = 0;
> +
> + int rc = scm_chi(scm_data, &val);
> +
> + WARN_ON(rc);
> +
> + return (val & GLOBAL_MMIO_CHI_ACRA) != 0;
> +}
> +
> +int scm_admin_command_complete_timeout(const struct scm_data *scm_data,
> + int command)
> +{
> + u32 timeout = scm_data->timeouts[command];
> + // 32 is the next power of 2 greater than the 20ms minimum for msleep
> +#define TIMEOUT_SLEEP_MILLIS 32
> + timeout /= TIMEOUT_SLEEP_MILLIS;
> + if (!timeout)
> + timeout = SCM_DEFAULT_TIMEOUT / TIMEOUT_SLEEP_MILLIS;
> +
> + while (timeout-- > 0) {
> + if (scm_admin_command_complete(scm_data))
> + return 0;
> + msleep(TIMEOUT_SLEEP_MILLIS);
> + }
> +
> + if (scm_admin_command_complete(scm_data))
> + return 0;
> +
> + return -EBUSY;
> +}
> +
> +int scm_admin_response_handled(const struct scm_data *scm_data)
> +{
> + return ocxl_global_mmio_set64(scm_data->ocxl_afu, GLOBAL_MMIO_CHIC,
> + OCXL_LITTLE_ENDIAN, GLOBAL_MMIO_CHI_ACRA);
> +}
> +
> +void scm_warn_status(const struct scm_data *scm_data, const char *message,
> + u8 status)
> +{
> + const char *text = "Unknown";
> +
> + switch (status) {
> + case STATUS_SUCCESS:
> + text = "Success";
> + break;
> +
> + case STATUS_MEM_UNAVAILABLE:
> + text = "Persistent memory unavailable";
> + break;
> +
> + case STATUS_BAD_OPCODE:
> + text = "Bad opcode";
> + break;
> +
> + case STATUS_BAD_REQUEST_PARM:
> + text = "Bad request parameter";
> + break;
> +
> + case STATUS_BAD_DATA_PARM:
> + text = "Bad data parameter";
> + break;
> +
> + case STATUS_DEBUG_BLOCKED:
> + text = "Debug action blocked";
> + break;
> +
> + case STATUS_FAIL:
> + text = "Failed";
> + break;
> + }
> +
> + dev_warn(&scm_data->dev, "%s: %s (%x)\n", message, text, status);
> +}
> diff --git a/drivers/nvdimm/ocxl/scm_internal.h b/drivers/nvdimm/ocxl/scm_internal.h
> index 584450f55e30..9bff684cd069 100644
> --- a/drivers/nvdimm/ocxl/scm_internal.h
> +++ b/drivers/nvdimm/ocxl/scm_internal.h
> @@ -6,6 +6,8 @@
> #include <linux/libnvdimm.h>
> #include <linux/mm.h>
>
> +#define SCM_DEFAULT_TIMEOUT 100
> +
> #define GLOBAL_MMIO_CHI 0x000
> #define GLOBAL_MMIO_CHIC 0x008
> #define GLOBAL_MMIO_CHIE 0x010
> @@ -80,6 +82,16 @@
>
> #define SCM_LABEL_AREA_SIZE (1UL << PA_SECTION_SHIFT)
>
> +struct command_metadata {
> + u32 request_offset;
> + u32 response_offset;
> + u32 data_offset;
> + u32 data_size;
> + struct mutex lock;
> + u16 id;
> + u8 op_code;
> +};
> +
> struct scm_function_0 {
> struct pci_dev *pdev;
> struct ocxl_fn *ocxl_fn;
> @@ -95,9 +107,11 @@ struct scm_data {
> struct ocxl_afu *ocxl_afu;
> struct ocxl_context *ocxl_context;
> void *metadata_addr;
> + struct command_metadata admin_command;
> struct resource scm_res;
> struct nd_region *nd_region;
> char fw_version[8+1];
> + u32 timeouts[ADMIN_COMMAND_MAX+1];
>
> u32 max_controller_dump_size;
> u16 scm_revision; // major/minor
> @@ -122,3 +136,51 @@ struct scm_data {
> * Returns 0 on success, negative on error
> */
> int scm_chi(const struct scm_data *scm_data, u64 *chi);
> +
> +/**
> + * scm_admin_command_request() - Issue an admin command request
> + * @scm_data: a pointer to the SCM device data
> + * @op_code: The op-code for the command
> + *
> + * Returns an identifier for the command, or negative on error
> + */
> +int scm_admin_command_request(struct scm_data *scm_data, u8 op_code);
> +
> +/**
> + * scm_admin_response() - Validate an admin response
> + * @scm_data: a pointer to the SCM device data
> + * Returns the status code of the command, or negative on error
> + */
> +int scm_admin_response(const struct scm_data *scm_data);
> +
> +/**
> + * scm_admin_command_execute() - Notify the controller to start processing a pending admin command
> + * @scm_data: a pointer to the SCM device data
> + * Returns 0 on success, negative on error
> + */
> +int scm_admin_command_execute(const struct scm_data *scm_data);
> +
> +/**
> + * scm_admin_command_complete_timeout() - Wait for an admin command to finish executing
> + * @scm_data: a pointer to the SCM device data
> + * @command: the admin command to wait for completion (determines the timeout)
> + * Returns 0 on success, -EBUSY on timeout
> + */
> +int scm_admin_command_complete_timeout(const struct scm_data *scm_data,
> + int command);
> +
> +/**
> + * scm_admin_response_handled() - Notify the controller that the admin response has been handled
> + * @scm_data: a pointer to the SCM device data
> + * Returns 0 on success, negative on failure
> + */
> +int scm_admin_response_handled(const struct scm_data *scm_data);
> +
> +/**
> + * scm_warn_status() - Emit a kernel warning showing a command status.
> + * @scm_data: a pointer to the SCM device data
> + * @message: A message to accompany the warning
> + * @status: The command status
> + */
> +void scm_warn_status(const struct scm_data *scm_data, const char *message,
> + u8 status);
^ permalink raw reply
* Re: [PATCH v2 14/27] nvdimm/ocxl: Add support for near storage commands
From: Jonathan Cameron @ 2020-02-03 14:22 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Keith Busch,
Masahiro Yamada, Paul Mackerras, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Vishal Verma, Mahesh Salgaonkar, Krzysztof Kozlowski,
Anju T Sudhakar, alastair, Andrew Donnellan, Arnd Bergmann,
Greg Kurz, Nicholas Piggin, Cédric Le Goater, Dan Williams,
Hari Bathini, linux-mm, Greg Kroah-Hartman, linux-kernel,
Frederic Barrat, Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <20191203034655.51561-15-alastair@au1.ibm.com>
On Tue, 3 Dec 2019 14:46:42 +1100
Alastair D'Silva <alastair@au1.ibm.com> wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> Similar to the previous patch, this adds support for near storage commands.
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
> drivers/nvdimm/ocxl/scm.c | 6 +++++
> drivers/nvdimm/ocxl/scm_internal.c | 41 ++++++++++++++++++++++++++++++
> drivers/nvdimm/ocxl/scm_internal.h | 38 +++++++++++++++++++++++++++
> 3 files changed, 85 insertions(+)
>
> diff --git a/drivers/nvdimm/ocxl/scm.c b/drivers/nvdimm/ocxl/scm.c
> index 1e175f3c3cf2..6c16ca7fabfa 100644
> --- a/drivers/nvdimm/ocxl/scm.c
> +++ b/drivers/nvdimm/ocxl/scm.c
> @@ -310,12 +310,18 @@ static int scm_setup_command_metadata(struct scm_data *scm_data)
> int rc;
>
> mutex_init(&scm_data->admin_command.lock);
> + mutex_init(&scm_data->ns_command.lock);
>
> rc = scm_extract_command_metadata(scm_data, GLOBAL_MMIO_ACMA_CREQO,
> &scm_data->admin_command);
> if (rc)
> return rc;
>
> + rc = scm_extract_command_metadata(scm_data, GLOBAL_MMIO_NSCMA_CREQO,
> + &scm_data->ns_command);
> + if (rc)
> + return rc;
> +
Ah. So much for my comment in previous patch. Ignore that...
> return 0;
> }
>
> diff --git a/drivers/nvdimm/ocxl/scm_internal.c b/drivers/nvdimm/ocxl/scm_internal.c
> index 7b11b56863fb..c405f1d8afb8 100644
> --- a/drivers/nvdimm/ocxl/scm_internal.c
> +++ b/drivers/nvdimm/ocxl/scm_internal.c
> @@ -132,6 +132,47 @@ int scm_admin_response_handled(const struct scm_data *scm_data)
> OCXL_LITTLE_ENDIAN, GLOBAL_MMIO_CHI_ACRA);
> }
>
> +int scm_ns_command_request(struct scm_data *scm_data, u8 op_code)
> +{
> + u64 val;
> + int rc = ocxl_global_mmio_read64(scm_data->ocxl_afu, GLOBAL_MMIO_CHI,
> + OCXL_LITTLE_ENDIAN, &val);
> + if (rc)
> + return rc;
> +
> + if (!(val & GLOBAL_MMIO_CHI_NSCRA))
> + return -EBUSY;
> +
> + return scm_command_request(scm_data, &scm_data->ns_command, op_code);
> +}
> +
> +int scm_ns_response(const struct scm_data *scm_data)
> +{
> + return scm_command_response(scm_data, &scm_data->ns_command);
> +}
> +
> +int scm_ns_command_execute(const struct scm_data *scm_data)
> +{
> + return ocxl_global_mmio_set64(scm_data->ocxl_afu, GLOBAL_MMIO_HCI,
> + OCXL_LITTLE_ENDIAN, GLOBAL_MMIO_HCI_NSCRW);
> +}
> +
> +bool scm_ns_command_complete(const struct scm_data *scm_data)
> +{
> + u64 val = 0;
> + int rc = scm_chi(scm_data, &val);
> +
> + WARN_ON(rc);
> +
> + return (val & GLOBAL_MMIO_CHI_NSCRA) != 0;
> +}
> +
> +int scm_ns_response_handled(const struct scm_data *scm_data)
> +{
> + return ocxl_global_mmio_set64(scm_data->ocxl_afu, GLOBAL_MMIO_CHIC,
> + OCXL_LITTLE_ENDIAN, GLOBAL_MMIO_CHI_NSCRA);
> +}
> +
> void scm_warn_status(const struct scm_data *scm_data, const char *message,
> u8 status)
> {
> diff --git a/drivers/nvdimm/ocxl/scm_internal.h b/drivers/nvdimm/ocxl/scm_internal.h
> index 9bff684cd069..9575996a89e7 100644
> --- a/drivers/nvdimm/ocxl/scm_internal.h
> +++ b/drivers/nvdimm/ocxl/scm_internal.h
> @@ -108,6 +108,7 @@ struct scm_data {
> struct ocxl_context *ocxl_context;
> void *metadata_addr;
> struct command_metadata admin_command;
> + struct command_metadata ns_command;
> struct resource scm_res;
> struct nd_region *nd_region;
> char fw_version[8+1];
> @@ -176,6 +177,42 @@ int scm_admin_command_complete_timeout(const struct scm_data *scm_data,
> */
> int scm_admin_response_handled(const struct scm_data *scm_data);
>
> +/**
> + * scm_ns_command_request() - Issue a near storage command request
> + * @scm_data: a pointer to the SCM device data
> + * @op_code: The op-code for the command
> + * Returns an identifier for the command, or negative on error
> + */
> +int scm_ns_command_request(struct scm_data *scm_data, u8 op_code);
> +
> +/**
> + * scm_ns_response() - Validate a near storage response
> + * @scm_data: a pointer to the SCM device data
> + * Returns the status code of the command, or negative on error
> + */
> +int scm_ns_response(const struct scm_data *scm_data);
> +
> +/**
> + * scm_ns_command_execute() - Notify the controller to start processing a pending near storage command
> + * @scm_data: a pointer to the SCM device data
> + * Returns 0 on success, negative on error
> + */
> +int scm_ns_command_execute(const struct scm_data *scm_data);
> +
> +/**
> + * scm_ns_command_complete() - Is a near storage command executing
> + * scm_data: a pointer to the SCM device data
> + * Returns true if the previous admin command has completed
> + */
> +bool scm_ns_command_complete(const struct scm_data *scm_data);
> +
> +/**
> + * scm_ns_response_handled() - Notify the controller that the near storage response has been handled
> + * scm_data: a pointer to the SCM device data
> + * Returns 0 on success, negative on failure
> + */
> +int scm_ns_response_handled(const struct scm_data *scm_data);
> +
> /**
> * scm_warn_status() - Emit a kernel warning showing a command status.
> * @scm_data: a pointer to the SCM device data
> @@ -184,3 +221,4 @@ int scm_admin_response_handled(const struct scm_data *scm_data);
> */
> void scm_warn_status(const struct scm_data *scm_data, const char *message,
> u8 status);
> +
Stray blank line!
Now we are into the real nitpicks. Not enough coffee.
Jonathan
^ permalink raw reply
* Re: [PATCH v2 24/27] nvdimm/ocxl: Implement Overwrite
From: Jonathan Cameron @ 2020-02-03 15:10 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Keith Busch,
Masahiro Yamada, Paul Mackerras, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Vishal Verma, Mahesh Salgaonkar, Krzysztof Kozlowski,
Anju T Sudhakar, alastair, Andrew Donnellan, Arnd Bergmann,
Greg Kurz, Nicholas Piggin, Cédric Le Goater, Dan Williams,
Hari Bathini, linux-mm, Greg Kroah-Hartman, linux-kernel,
Frederic Barrat, Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <20191203034655.51561-25-alastair@au1.ibm.com>
On Tue, 3 Dec 2019 14:46:52 +1100
Alastair D'Silva <alastair@au1.ibm.com> wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> The near storage command 'Secure Erase' overwrites all data on the
> media.
>
> This patch hooks it up to the security function 'overwrite'.
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
A few things to tidy up in here.
Thanks,
Jonathan
> ---
> drivers/nvdimm/ocxl/scm.c | 164 ++++++++++++++++++++++++++++-
> drivers/nvdimm/ocxl/scm_internal.c | 1 +
> drivers/nvdimm/ocxl/scm_internal.h | 17 +++
> 3 files changed, 180 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvdimm/ocxl/scm.c b/drivers/nvdimm/ocxl/scm.c
> index a81eb5916eb3..8deb7862793c 100644
> --- a/drivers/nvdimm/ocxl/scm.c
> +++ b/drivers/nvdimm/ocxl/scm.c
> @@ -169,6 +169,86 @@ static int scm_reserve_metadata(struct scm_data *scm_data,
> return 0;
> }
>
> +/**
> + * scm_overwrite() - Overwrite all data on the card
> + * @scm_data: The SCM device data
I would mention in here that this exists with the lock held and
where that is unlocked again.
> + * Return: 0 on success
> + */
> +int scm_overwrite(struct scm_data *scm_data)
> +{
> + int rc;
> +
> + mutex_lock(&scm_data->ns_command.lock);
> +
> + rc = scm_ns_command_request(scm_data, NS_COMMAND_SECURE_ERASE);
> + if (rc)
Perhaps change that goto label to reflect it is the error path rather
than a shared exit route.
> + goto out;
> +
> + rc = scm_ns_command_execute(scm_data);
> + if (rc)
> + goto out;
> +
> + scm_data->overwrite_state = SCM_OVERWRITE_BUSY;
> +
> + return 0;
> +
> +out:
> + mutex_unlock(&scm_data->ns_command.lock);
> + return rc;
> +}
> +
> +/**
> + * scm_secop_overwrite() - Overwrite all data on the card
> + * @nvdimm: The nvdimm representation of the SCM device to start the overwrite on
> + * @key_data: Unused (no security key implementation)
> + * Return: 0 on success
> + */
> +static int scm_secop_overwrite(struct nvdimm *nvdimm,
> + const struct nvdimm_key_data *key_data)
> +{
> + struct scm_data *scm_data = nvdimm_provider_data(nvdimm);
> +
> + return scm_overwrite(scm_data);
> +}
> +
> +/**
> + * scm_secop_query_overwrite() - Get the current overwrite state
> + * @nvdimm: The nvdimm representation of the SCM device to start the overwrite on
> + * Return: 0 if successful or idle, -EBUSY if busy, -EFAULT if failed
> + */
> +static int scm_secop_query_overwrite(struct nvdimm *nvdimm)
> +{
> + struct scm_data *scm_data = nvdimm_provider_data(nvdimm);
> +
> + if (scm_data->overwrite_state == SCM_OVERWRITE_BUSY)
> + return -EBUSY;
> +
> + if (scm_data->overwrite_state == SCM_OVERWRITE_FAILED)
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +/**
> + * scm_secop_get_flags() - return the security flags for the SCM device
All params need to documented in kernel-doc comments.
> + */
> +static unsigned long scm_secop_get_flags(struct nvdimm *nvdimm,
> + enum nvdimm_passphrase_type ptype)
> +{
> + struct scm_data *scm_data = nvdimm_provider_data(nvdimm);
> +
> + if (scm_data->overwrite_state == SCM_OVERWRITE_BUSY)
> + return BIT(NVDIMM_SECURITY_OVERWRITE);
> +
> + return BIT(NVDIMM_SECURITY_DISABLED);
> +}
> +
> +static const struct nvdimm_security_ops sec_ops = {
> + .get_flags = scm_secop_get_flags,
> + .overwrite = scm_secop_overwrite,
> + .query_overwrite = scm_secop_query_overwrite,
> +};
> +
> /**
> * scm_register_lpc_mem() - Discover persistent memory on a device and register it with the NVDIMM subsystem
> * @scm_data: The SCM device data
> @@ -224,10 +304,10 @@ static int scm_register_lpc_mem(struct scm_data *scm_data)
> set_bit(NDD_ALIASING, &nvdimm_flags);
>
> snprintf(serial, sizeof(serial), "%llx", fn_config->serial);
> - nd_mapping_desc.nvdimm = nvdimm_create(scm_data->nvdimm_bus, scm_data,
> + nd_mapping_desc.nvdimm = __nvdimm_create(scm_data->nvdimm_bus, scm_data,
> scm_dimm_attribute_groups,
> nvdimm_flags, nvdimm_cmd_mask,
> - 0, NULL);
> + 0, NULL, serial, &sec_ops);
> if (!nd_mapping_desc.nvdimm)
> return -ENOMEM;
>
> @@ -1530,6 +1610,83 @@ static void scm_dump_error_log(struct scm_data *scm_data)
> kfree(buf);
> }
>
> +static void scm_handle_nscra_doorbell(struct scm_data *scm_data)
> +{
> + int rc;
> +
> + if (scm_data->ns_command.op_code == NS_COMMAND_SECURE_ERASE) {
Feels likely that we are going to end up with quite a few blocks like this as
the driver is extended. Perhaps just start out with a switch statement and
separate functions that it calls?
> + u64 success, attempted;
> +
One is enough here.
> +
> + rc = scm_ns_response(scm_data);
> + if (rc < 0) {
> + scm_data->overwrite_state = SCM_OVERWRITE_FAILED;
If this were a separate function as suggested above, I'd use a goto to ensure we
unlock in all paths.
> + mutex_unlock(&scm_data->ns_command.lock);
> + return;
> + }
> + if (rc != STATUS_SUCCESS)
> + scm_warn_status(scm_data, "Unexpected status from overwrite", rc);
> +
> + rc = ocxl_global_mmio_read64(scm_data->ocxl_afu,
> + scm_data->ns_command.response_offset +
> + NS_RESPONSE_SECURE_ERASE_ACCESSIBLE_SUCCESS,
> + OCXL_HOST_ENDIAN, &success);
> + if (rc) {
> + scm_data->overwrite_state = SCM_OVERWRITE_FAILED;
> + mutex_unlock(&scm_data->ns_command.lock);
> + return;
> + }
> +
> + rc = ocxl_global_mmio_read64(scm_data->ocxl_afu,
> + scm_data->ns_command.response_offset +
> + NS_RESPONSE_SECURE_ERASE_ACCESSIBLE_ATTEMPTED,
> + OCXL_HOST_ENDIAN, &attempted);
> + if (rc) {
> + scm_data->overwrite_state = SCM_OVERWRITE_FAILED;
> + mutex_unlock(&scm_data->ns_command.lock);
> + return;
> + }
> +
> + scm_data->overwrite_state = SCM_OVERWRITE_SUCCESS;
> + if (success != attempted)
> + scm_data->overwrite_state = SCM_OVERWRITE_FAILED;
> +
> + dev_info(&scm_data->dev,
> + "Overwritten %llu/%llu accessible pages", success, attempted);
Do we want to spam the log? Feels like dev_dbg maybe?
> +
> + rc = ocxl_global_mmio_read64(scm_data->ocxl_afu,
> + scm_data->ns_command.response_offset +
> + NS_RESPONSE_SECURE_ERASE_DEFECTIVE_SUCCESS,
> + OCXL_HOST_ENDIAN, &success);
> + if (rc) {
> + scm_data->overwrite_state = SCM_OVERWRITE_FAILED;
> + mutex_unlock(&scm_data->ns_command.lock);
> + return;
> + }
> +
> + rc = ocxl_global_mmio_read64(scm_data->ocxl_afu,
> + scm_data->ns_command.response_offset +
> + NS_RESPONSE_SECURE_ERASE_DEFECTIVE_ATTEMPTED,
> + OCXL_HOST_ENDIAN, &attempted);
> + if (rc) {
> + scm_data->overwrite_state = SCM_OVERWRITE_FAILED;
> + mutex_unlock(&scm_data->ns_command.lock);
> + return;
> + }
> +
> + if (success != attempted)
> + scm_data->overwrite_state = SCM_OVERWRITE_FAILED;
> +
> + dev_info(&scm_data->dev,
> + "Overwritten %llu/%llu defective pages", success, attempted);
Again, maybe dev_dbg?
> +
> + scm_ns_response_handled(scm_data);
> +
> + mutex_unlock(&scm_data->ns_command.lock);
> + return;
> + }
> +}
> +
> static irqreturn_t scm_imn0_handler(void *private)
> {
> struct scm_data *scm_data = private;
> @@ -1537,6 +1694,9 @@ static irqreturn_t scm_imn0_handler(void *private)
>
> (void)scm_chi(scm_data, &chi);
>
> + if (chi & GLOBAL_MMIO_CHI_NSCRA)
> + scm_handle_nscra_doorbell(scm_data);
> +
> if (chi & GLOBAL_MMIO_CHI_ELA) {
> dev_warn(&scm_data->dev, "Error log is available\n");
>
> diff --git a/drivers/nvdimm/ocxl/scm_internal.c b/drivers/nvdimm/ocxl/scm_internal.c
> index 8fc849610eaa..db919a23c69b 100644
> --- a/drivers/nvdimm/ocxl/scm_internal.c
> +++ b/drivers/nvdimm/ocxl/scm_internal.c
> @@ -173,6 +173,7 @@ int scm_ns_response_handled(const struct scm_data *scm_data)
> OCXL_LITTLE_ENDIAN, GLOBAL_MMIO_CHI_NSCRA);
> }
>
> +
Stray blank line..
> void scm_warn_status(const struct scm_data *scm_data, const char *message,
> u8 status)
> {
> diff --git a/drivers/nvdimm/ocxl/scm_internal.h b/drivers/nvdimm/ocxl/scm_internal.h
> index af19813a7f75..4a29088612a9 100644
> --- a/drivers/nvdimm/ocxl/scm_internal.h
> +++ b/drivers/nvdimm/ocxl/scm_internal.h
> @@ -70,6 +70,15 @@
> #define ADMIN_COMMAND_CMD_CAPS 0x08u
> #define ADMIN_COMMAND_MAX 0x08u
>
> +#define NS_COMMAND_SECURE_ERASE 0x20ull
> +
> +#define NS_RESPONSE_SECURE_ERASE_ACCESSIBLE_SUCCESS 0x20
> +#define NS_RESPONSE_SECURE_ERASE_ACCESSIBLE_ATTEMPTED 0x28
> +#define NS_RESPONSE_SECURE_ERASE_DEFECTIVE_SUCCESS 0x30
> +#define NS_RESPONSE_SECURE_ERASE_DEFECTIVE_ATTEMPTED 0x38
> +
Lot of blank lines...
> +
> +
> #define STATUS_SUCCESS 0x00
> #define STATUS_MEM_UNAVAILABLE 0x20
> #define STATUS_BAD_OPCODE 0x50
> @@ -99,6 +108,13 @@ struct scm_function_0 {
> struct ocxl_fn *ocxl_fn;
> };
>
> +enum overwrite_state {
> + SCM_OVERWRITE_IDLE = 0,
> + SCM_OVERWRITE_BUSY,
> + SCM_OVERWRITE_SUCCESS,
> + SCM_OVERWRITE_FAILED
> +};
> +
> struct scm_data {
> struct device dev;
> struct pci_dev *pdev;
> @@ -116,6 +132,7 @@ struct scm_data {
> void *metadata_addr;
> struct command_metadata admin_command;
> struct command_metadata ns_command;
> + enum overwrite_state overwrite_state;
> struct resource scm_res;
> struct nd_region *nd_region;
> struct eventfd_ctx *ev_ctx;
^ permalink raw reply
* Re: [PATCH v2 22/27] nvdimm/ocxl: Implement the heartbeat command
From: Jonathan Cameron @ 2020-02-03 15:11 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Keith Busch,
Masahiro Yamada, Paul Mackerras, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Vishal Verma, Mahesh Salgaonkar, Krzysztof Kozlowski,
Anju T Sudhakar, alastair, Andrew Donnellan, Arnd Bergmann,
Greg Kurz, Nicholas Piggin, Cédric Le Goater, Dan Williams,
Hari Bathini, linux-mm, Greg Kroah-Hartman, linux-kernel,
Frederic Barrat, Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <20191203034655.51561-23-alastair@au1.ibm.com>
On Tue, 3 Dec 2019 14:46:50 +1100
Alastair D'Silva <alastair@au1.ibm.com> wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> The heartbeat admin command is a simple admin command that exercises
> the communication mechanisms within the controller.
>
> This patch issues a heartbeat command to the card during init to ensure
> we can communicate with the card's crontroller.
controller
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
> drivers/nvdimm/ocxl/scm.c | 43 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 43 insertions(+)
>
> diff --git a/drivers/nvdimm/ocxl/scm.c b/drivers/nvdimm/ocxl/scm.c
> index 8a30c887b5ed..e8b34262f397 100644
> --- a/drivers/nvdimm/ocxl/scm.c
> +++ b/drivers/nvdimm/ocxl/scm.c
> @@ -353,6 +353,44 @@ static bool scm_is_usable(const struct scm_data *scm_data)
> return true;
> }
>
> +/**
> + * scm_heartbeat() - Issue a heartbeat command to the controller
> + * @scm_data: a pointer to the SCM device data
> + * Return: 0 if the controller responded correctly, negative on error
> + */
> +static int scm_heartbeat(struct scm_data *scm_data)
> +{
> + int rc;
> +
> + mutex_lock(&scm_data->admin_command.lock);
> +
> + rc = scm_admin_command_request(scm_data, ADMIN_COMMAND_HEARTBEAT);
> + if (rc)
> + goto out;
> +
> + rc = scm_admin_command_execute(scm_data);
> + if (rc)
> + goto out;
> +
> + rc = scm_admin_command_complete_timeout(scm_data, ADMIN_COMMAND_HEARTBEAT);
> + if (rc < 0) {
> + dev_err(&scm_data->dev, "Heartbeat timeout\n");
> + goto out;
> + }
> +
> + rc = scm_admin_response(scm_data);
> + if (rc < 0)
> + goto out;
> + if (rc != STATUS_SUCCESS)
> + scm_warn_status(scm_data, "Unexpected status from heartbeat", rc);
> +
> + rc = scm_admin_response_handled(scm_data);
> +
> +out:
> + mutex_unlock(&scm_data->admin_command.lock);
> + return rc;
> +}
> +
> /**
> * allocate_scm_minor() - Allocate a minor number to use for an SCM device
> * @scm_data: The SCM device to associate the minor with
> @@ -1508,6 +1546,11 @@ static int scm_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> goto err;
> }
>
> + if (scm_heartbeat(scm_data)) {
> + dev_err(&pdev->dev, "SCM Heartbeat failed\n");
> + goto err;
> + }
> +
> elapsed = 0;
> timeout = scm_data->readiness_timeout + scm_data->memory_available_timeout;
> while (!scm_is_usable(scm_data)) {
^ permalink raw reply
* Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
From: Christophe Leroy @ 2020-02-03 15:14 UTC (permalink / raw)
To: Qian Cai
Cc: Mark Rutland, linux-ia64, linux-sh, Peter Zijlstra, James Hogan,
Tetsuo Handa, Heiko Carstens, Michal Hocko, linux-mm,
Paul Mackerras, kasan-dev, sparclinux, Thomas Gleixner,
linux-s390, x86, Russell King - ARM Linux, Matthew Wilcox,
Steven Price, Jason Gunthorpe, Gerald Schaefer, linux-snps-arc,
Ingo Molnar, Kees Cook, Anshuman Khandual, Masahiro Yamada,
Mark Brown, Kirill A . Shutemov, Dan Williams, Vlastimil Babka,
linux-arm-kernel, Sri Krishna chowdary, Ard Biesheuvel,
Greg Kroah-Hartman, Dave Hansen, linux-mips, Ralf Baechle,
linux-kernel, Paul Burton, Mike Rapoport, Vineet Gupta,
Martin Schwidefsky, Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <2C4ADFAE-7BB4-42B7-8F54-F036EA7A4316@lca.pw>
Le 02/02/2020 à 12:26, Qian Cai a écrit :
>
>
>> On Jan 30, 2020, at 9:13 AM, Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>
>> config DEBUG_VM_PGTABLE
>> bool "Debug arch page table for semantics compliance" if ARCH_HAS_DEBUG_VM_PGTABLE || EXPERT
>> depends on MMU
>> default 'n' if !ARCH_HAS_DEBUG_VM_PGTABLE
>> default 'y' if DEBUG_VM
>
> Does it really necessary to potentially force all bots to run this? Syzbot, kernel test robot etc? Does it ever pay off for all their machine times there?
>
Machine time ?
On a 32 bits powerpc running at 132 MHz, the tests takes less than 10ms.
Is it worth taking the risk of not detecting faults by not selecting it
by default ?
[ 5.656916] debug_vm_pgtable: debug_vm_pgtable: Validating
architecture page table helpers
[ 5.665661] debug_vm_pgtable: debug_vm_pgtable: Validated
architecture page table helpers
Christophe
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox