* Re: [PATCH] ima: add a new CONFIG for loading arch-specific policies
From: Michael Ellerman @ 2020-03-02 23:23 UTC (permalink / raw)
To: Mimi Zohar, Ard Biesheuvel
Cc: linux-s390, linux-efi, Nayna Jain, Linux Kernel Mailing List,
Philipp Rudo, Martin Schwidefsky, linux-integrity, linuxppc-dev
In-Reply-To: <1583161018.8544.96.camel@linux.ibm.com>
Mimi Zohar <zohar@linux.ibm.com> writes:
> On Mon, 2020-03-02 at 15:52 +0100, Ard Biesheuvel wrote:
>> On Mon, 2 Mar 2020 at 15:48, Mimi Zohar <zohar@linux.ibm.com> wrote:
>> >
>> > On Wed, 2020-02-26 at 14:10 -0500, Nayna Jain wrote:
>> > > Every time a new architecture defines the IMA architecture specific
>> > > functions - arch_ima_get_secureboot() and arch_ima_get_policy(), the IMA
>> > > include file needs to be updated. To avoid this "noise", this patch
>> > > defines a new IMA Kconfig IMA_SECURE_AND_OR_TRUSTED_BOOT option, allowing
>> > > the different architectures to select it.
>> > >
>> > > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
>> > > Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
>> > > Cc: Ard Biesheuvel <ardb@kernel.org>
>> > > Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
>> > > Cc: Philipp Rudo <prudo@linux.ibm.com>
>> > > Cc: Michael Ellerman <mpe@ellerman.id.au>
>> > > ---
>> > > arch/powerpc/Kconfig | 2 +-
>> > > arch/s390/Kconfig | 1 +
>> > > arch/x86/Kconfig | 1 +
>> > > include/linux/ima.h | 3 +--
>> > > security/integrity/ima/Kconfig | 9 +++++++++
>> > > 5 files changed, 13 insertions(+), 3 deletions(-)
>> > >
>> > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> > > index 497b7d0b2d7e..b8ce1b995633 100644
>> > > --- a/arch/powerpc/Kconfig
>> > > +++ b/arch/powerpc/Kconfig
>> > > @@ -246,6 +246,7 @@ config PPC
>> > > select SYSCTL_EXCEPTION_TRACE
>> > > select THREAD_INFO_IN_TASK
>> > > select VIRT_TO_BUS if !PPC64
>> > > + select IMA_SECURE_AND_OR_TRUSTED_BOOT if PPC_SECURE_BOOT
>> > > #
>> > > # Please keep this list sorted alphabetically.
>> > > #
>> > > @@ -978,7 +979,6 @@ config PPC_SECURE_BOOT
>> > > prompt "Enable secure boot support"
>> > > bool
>> > > depends on PPC_POWERNV
>> > > - depends on IMA_ARCH_POLICY
>> > > help
>> > > Systems with firmware secure boot enabled need to define security
>> > > policies to extend secure boot to the OS. This config allows a user
>> > > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
>> > > index 8abe77536d9d..90ff3633ade6 100644
>> > > --- a/arch/s390/Kconfig
>> > > +++ b/arch/s390/Kconfig
>> > > @@ -195,6 +195,7 @@ config S390
>> > > select ARCH_HAS_FORCE_DMA_UNENCRYPTED
>> > > select SWIOTLB
>> > > select GENERIC_ALLOCATOR
>> > > + select IMA_SECURE_AND_OR_TRUSTED_BOOT
>> > >
>> > >
>> > > config SCHED_OMIT_FRAME_POINTER
>> > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> > > index beea77046f9b..cafa66313fe2 100644
>> > > --- a/arch/x86/Kconfig
>> > > +++ b/arch/x86/Kconfig
>> > > @@ -230,6 +230,7 @@ config X86
>> > > select VIRT_TO_BUS
>> > > select X86_FEATURE_NAMES if PROC_FS
>> > > select PROC_PID_ARCH_STATUS if PROC_FS
>> > > + select IMA_SECURE_AND_OR_TRUSTED_BOOT if EFI
>> >
>> > Not everyone is interested in enabling IMA or requiring IMA runtime
>> > policies. With this patch, enabling IMA_ARCH_POLICY is therefore
>> > still left up to the person building the kernel. As a result, I'm
>> > seeing the following warning, which is kind of cool.
>> >
>> > WARNING: unmet direct dependencies detected for
>> > IMA_SECURE_AND_OR_TRUSTED_BOOT
>> > Depends on [n]: INTEGRITY [=y] && IMA [=y] && IMA_ARCH_POLICY [=n]
>> > Selected by [y]:
>> > - X86 [=y] && EFI [=y]
>> >
>> > Ard, Michael, Martin, just making sure this type of warning is
>> > acceptable before upstreaming this patch. I would appreciate your
>> > tags.
>> >
>>
>> Ehm, no, warnings like these are not really acceptable. It means there
>> is an inconsistency in the way the Kconfig dependencies are defined.
>>
>> Does this help:
>>
>> select IMA_SECURE_AND_OR_TRUSTED_BOOT if EFI && IMA_ARCH_POLICY
>>
>> ?
>
> Yes, that's fine for x86. Michael, Martin, do you want something
> similar or would you prefer actually selecting IMA_ARCH_POLICY?
For powerpc this should be all we need:
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 497b7d0b2d7e..a5cfde432983 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -976,12 +976,13 @@ config PPC_MEM_KEYS
config PPC_SECURE_BOOT
prompt "Enable secure boot support"
bool
depends on PPC_POWERNV
depends on IMA_ARCH_POLICY
+ select IMA_SECURE_AND_OR_TRUSTED_BOOT
help
Systems with firmware secure boot enabled need to define security
policies to extend secure boot to the OS. This config allows a user
to enable OS secure boot on systems that have firmware support for
it. If in doubt say N.
cheers
^ permalink raw reply related
* Re: [RFC PATCH v1] powerpc/prom_init: disable XIVE in Secure VM.
From: Greg Kurz @ 2020-03-02 20:54 UTC (permalink / raw)
To: Ram Pai; +Cc: aik, andmike, kvm-ppc, clg, sukadev, linuxppc-dev, bauerman,
david
In-Reply-To: <1582962844-26333-1-git-send-email-linuxram@us.ibm.com>
On Fri, 28 Feb 2020 23:54:04 -0800
Ram Pai <linuxram@us.ibm.com> wrote:
> XIVE is not correctly enabled for Secure VM in the KVM Hypervisor yet.
>
What exactly is "not correctly enabled" ?
> Hence Secure VM, must always default to XICS interrupt controller.
>
So this is a temporary workaround until whatever isn't working with
XIVE and the Secure VM gets fixed. Maybe worth mentioning this in
some comment.
> If XIVE is requested through kernel command line option "xive=on",
> override and turn it off.
>
There's no such thing as requesting XIVE with "xive=on". XIVE is
on by default if the platform and CPU support it BUT it can be
disabled with "xive=off" in which case the guest wont request
XIVE except if it's the only available mode.
> If XIVE is the only supported platform interrupt controller; specified
> through qemu option "ic-mode=xive", simply abort. Otherwise default to
> XICS.
>
If XIVE is the only option and the guest requests XICS anyway, QEMU is
supposed to print an error message and terminate:
if (!spapr->irq->xics) {
error_report(
"Guest requested unavailable interrupt mode (XICS), either don't set the ic-mode machine property or try ic-mode=xics or ic-mode=dual");
exit(EXIT_FAILURE);
}
I think it would be better to end up there rather than aborting.
> Cc: kvm-ppc@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> Cc: Michael Anderson <andmike@linux.ibm.com>
> Cc: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> Cc: Paul Mackerras <paulus@ozlabs.org>
> Cc: Greg Kurz <groug@kaod.org>
> Cc: Cedric Le Goater <clg@fr.ibm.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
> arch/powerpc/kernel/prom_init.c | 43 ++++++++++++++++++++++++++++-------------
> 1 file changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index 5773453..dd96c82 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -805,6 +805,18 @@ static void __init early_cmdline_parse(void)
> #endif
> }
>
> +#ifdef CONFIG_PPC_SVM
> + opt = prom_strstr(prom_cmd_line, "svm=");
> + if (opt) {
> + bool val;
> +
> + opt += sizeof("svm=") - 1;
> + if (!prom_strtobool(opt, &val))
> + prom_svm_enable = val;
> + prom_printf("svm =%d\n", prom_svm_enable);
> + }
> +#endif /* CONFIG_PPC_SVM */
> +
> #ifdef CONFIG_PPC_PSERIES
> prom_radix_disable = !IS_ENABLED(CONFIG_PPC_RADIX_MMU_DEFAULT);
> opt = prom_strstr(prom_cmd_line, "disable_radix");
> @@ -823,23 +835,22 @@ static void __init early_cmdline_parse(void)
> if (prom_radix_disable)
> prom_debug("Radix disabled from cmdline\n");
>
> - opt = prom_strstr(prom_cmd_line, "xive=off");
> - if (opt) {
A comment to explain why we currently need to limit ourselves to using
XICS would be appreciated.
> +#ifdef CONFIG_PPC_SVM
> + if (prom_svm_enable) {
> prom_xive_disable = true;
> - prom_debug("XIVE disabled from cmdline\n");
> + prom_debug("XIVE disabled in Secure VM\n");
> }
> -#endif /* CONFIG_PPC_PSERIES */
> -
> -#ifdef CONFIG_PPC_SVM
> - opt = prom_strstr(prom_cmd_line, "svm=");
> - if (opt) {
> - bool val;
> +#endif /* CONFIG_PPC_SVM */
>
> - opt += sizeof("svm=") - 1;
> - if (!prom_strtobool(opt, &val))
> - prom_svm_enable = val;
> + if (!prom_xive_disable) {
> + opt = prom_strstr(prom_cmd_line, "xive=off");
> + if (opt) {
> + prom_xive_disable = true;
> + prom_debug("XIVE disabled from cmdline\n");
> + }
> }
> -#endif /* CONFIG_PPC_SVM */
> +
> +#endif /* CONFIG_PPC_PSERIES */
> }
>
> #ifdef CONFIG_PPC_PSERIES
> @@ -1251,6 +1262,12 @@ static void __init prom_parse_xive_model(u8 val,
> break;
> case OV5_FEAT(OV5_XIVE_EXPLOIT): /* Only Exploitation mode */
> prom_debug("XIVE - exploitation mode supported\n");
> +
> +#ifdef CONFIG_PPC_SVM
> + if (prom_svm_enable)
> + prom_panic("WARNING: xive unsupported in Secure VM\n");
Change the prom_panic() line into a break. The guest will ask XICS and QEMU
will terminate nicely. Maybe still print out a warning since QEMU won't mention
the Secure VM aspect of things.
> +#endif /* CONFIG_PPC_SVM */
> +
> if (prom_xive_disable) {
> /*
> * If we __have__ to do XIVE, we're better off ignoring
^ permalink raw reply
* Re: [PATCH] mm/debug: Add tests validating arch page table helpers for core features
From: Christophe Leroy @ 2020-03-02 21:24 UTC (permalink / raw)
To: Anshuman Khandual
Cc: Catalin Marinas, Heiko Carstens, linux-mm, Paul Mackerras,
H. Peter Anvin, linux-riscv, Will Deacon, linux-arch, linux-s390,
x86, Mike Rapoport, Christian Borntraeger, Ingo Molnar,
linux-snps-arc, Vasily Gorbik, Borislav Petkov, Paul Walmsley,
Kirill A . Shutemov, Thomas Gleixner, linux-arm-kernel,
Vineet Gupta, linux-kernel, Palmer Dabbelt, Andrew Morton,
linuxppc-dev
In-Reply-To: <4343eda9-7df2-a13c-0125-cf784c81ce14@arm.com>
Anshuman Khandual <anshuman.khandual@arm.com> a écrit :
> On 02/27/2020 04:59 PM, Christophe Leroy wrote:
>>
>>
>> Le 27/02/2020 à 11:33, Anshuman Khandual a écrit :
>>> This adds new tests validating arch page table helpers for these following
>>> core memory features. These tests create and test specific mapping types at
>>> various page table levels.
>>>
>>> * SPECIAL mapping
>>> * PROTNONE mapping
>>> * DEVMAP mapping
>>> * SOFTDIRTY mapping
>>> * SWAP mapping
>>> * MIGRATION mapping
>>> * HUGETLB mapping
>>> * THP mapping
>>>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Cc: Mike Rapoport <rppt@linux.ibm.com>
>>> Cc: Vineet Gupta <vgupta@synopsys.com>
>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>> Cc: Will Deacon <will@kernel.org>
>>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>> Cc: Paul Mackerras <paulus@samba.org>
>>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>>> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
>>> Cc: Vasily Gorbik <gor@linux.ibm.com>
>>> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Cc: Ingo Molnar <mingo@redhat.com>
>>> Cc: Borislav Petkov <bp@alien8.de>
>>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>>> Cc: Kirill A. Shutemov <kirill@shutemov.name>
>>> Cc: Paul Walmsley <paul.walmsley@sifive.com>
>>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
>>> Cc: linux-snps-arc@lists.infradead.org
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: linuxppc-dev@lists.ozlabs.org
>>> Cc: linux-s390@vger.kernel.org
>>> Cc: linux-riscv@lists.infradead.org
>>> Cc: x86@kernel.org
>>> Cc: linux-arch@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>> ---
>>> Tested on arm64 and x86 platforms without any test failures. But this has
>>> only been built tested on several other platforms. Individual tests need
>>> to be verified on all current enabling platforms for the test i.e s390,
>>> ppc32, arc etc.
>>>
>>> This patch must be applied on v5.6-rc3 after these patches
>>>
>>> 1. https://patchwork.kernel.org/patch/11385057/
>>> 2. https://patchwork.kernel.org/patch/11407715/
>>>
>>> OR
>>>
>>> This patch must be applied on linux-next (next-20200227) after this patch
>>>
>>> 2. https://patchwork.kernel.org/patch/11407715/
>>>
>>> mm/debug_vm_pgtable.c | 310 +++++++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 309 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>> index 96dd7d574cef..3fb90d5b604e 100644
>>> --- a/mm/debug_vm_pgtable.c
>>> +++ b/mm/debug_vm_pgtable.c
>>> @@ -41,6 +41,44 @@
>>> * wrprotect(entry) = A write protected and not a write entry
>>> * pxx_bad(entry) = A mapped and non-table entry
>>> * pxx_same(entry1, entry2) = Both entries hold the exact same value
>>> + *
>>> + * Specific feature operations
>>> + *
>>> + * pte_mkspecial(entry) = Creates a special entry at PTE level
>>> + * pte_special(entry) = Tests a special entry at PTE level
>>> + *
>>> + * pte_protnone(entry) = Tests a no access entry at PTE level
>>> + * pmd_protnone(entry) = Tests a no access entry at PMD level
>>> + *
>>> + * pte_mkdevmap(entry) = Creates a device entry at PTE level
>>> + * pmd_mkdevmap(entry) = Creates a device entry at PMD level
>>> + * pud_mkdevmap(entry) = Creates a device entry at PUD level
>>> + * pte_devmap(entry) = Tests a device entry at PTE level
>>> + * pmd_devmap(entry) = Tests a device entry at PMD level
>>> + * pud_devmap(entry) = Tests a device entry at PUD level
>>> + *
>>> + * pte_mksoft_dirty(entry) = Creates a soft dirty entry at PTE level
>>> + * pmd_mksoft_dirty(entry) = Creates a soft dirty entry at PMD level
>>> + * pte_swp_mksoft_dirty(entry) = Creates a soft dirty swap
>>> entry at PTE level
>>> + * pmd_swp_mksoft_dirty(entry) = Creates a soft dirty swap
>>> entry at PMD level
>>> + * pte_soft_dirty(entry) = Tests a soft dirty entry at PTE level
>>> + * pmd_soft_dirty(entry) = Tests a soft dirty entry at PMD level
>>> + * pte_swp_soft_dirty(entry) = Tests a soft dirty swap entry
>>> at PTE level
>>> + * pmd_swp_soft_dirty(entry) = Tests a soft dirty swap entry
>>> at PMD level
>>> + * pte_clear_soft_dirty(entry) = Clears a soft dirty entry
>>> at PTE level
>>> + * pmd_clear_soft_dirty(entry) = Clears a soft dirty entry
>>> at PMD level
>>> + * pte_swp_clear_soft_dirty(entry) = Clears a soft dirty swap
>>> entry at PTE level
>>> + * pmd_swp_clear_soft_dirty(entry) = Clears a soft dirty swap
>>> entry at PMD level
>>> + *
>>> + * pte_mkhuge(entry) = Creates a HugeTLB entry at given level
>>> + * pte_huge(entry) = Tests a HugeTLB entry at given level
>>> + *
>>> + * pmd_trans_huge(entry) = Tests a trans huge page at PMD level
>>> + * pud_trans_huge(entry) = Tests a trans huge page at PUD level
>>> + * pmd_present(entry) = Tests an entry points to memory at
>>> PMD level
>>> + * pud_present(entry) = Tests an entry points to memory at
>>> PUD level
>>> + * pmd_mknotpresent(entry) = Invalidates an PMD entry for MMU
>>> + * pud_mknotpresent(entry) = Invalidates an PUD entry for MMU
>>> */
>>> #define VMFLAGS (VM_READ|VM_WRITE|VM_EXEC)
>>> @@ -287,6 +325,233 @@ static void __init
>>> pmd_populate_tests(struct mm_struct *mm, pmd_t *pmdp,
>>> WARN_ON(pmd_bad(pmd));
>>> }
>>> +#ifdef CONFIG_ARCH_HAS_PTE_SPECIAL
>>
>> Can we avoid ifdefs unless necessary ?
>>
>> In mm/memory.c I see things like the following, it means
>> pte_special() always exist and a #ifdef is not necessary.
>
> True, #ifdef here can be dropped here, done.
>
>>
>> if (IS_ENABLED(CONFIG_ARCH_HAS_PTE_SPECIAL)) {
>> if (likely(!pte_special(pte)))
>> goto check_pfn;
>> if (vma->vm_ops && vma->vm_ops->find_special_page)
>> return vma->vm_ops->find_special_page(vma, addr);
>> if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
>> return NULL;
>> if (is_zero_pfn(pfn))
>> return NULL;
>> if (pte_devmap(pte))
>> return NULL;
>>
>> print_bad_pte(vma, addr, pte, NULL);
>> return NULL;
>> }
>>
>>> +static void __init pte_special_tests(unsigned long pfn, pgprot_t prot)
>>> +{
>>> + pte_t pte = pfn_pte(pfn, prot);
>>> +
>>> + WARN_ON(!pte_special(pte_mkspecial(pte)));
>>> +}
>>> +#else
>>> +static void __init pte_special_tests(unsigned long pfn, pgprot_t prot) { }
>>> +#endif
>>> +
>>> +#ifdef CONFIG_NUMA_BALANCING
>>
>> Same here, this ifdef shouldn't be necessary because in
>> /include/asm-generic/pgtable.h we have the following, so a if
>> (IS_ENABLED()) should be enough.
>>
>> #ifndef CONFIG_NUMA_BALANCING
>> /*
>> * Technically a PTE can be PROTNONE even when not doing NUMA balancing but
>> * the only case the kernel cares is for NUMA balancing and is only ever set
>> * when the VMA is accessible. For PROT_NONE VMAs, the PTEs are not marked
>> * _PAGE_PROTNONE so by by default, implement the helper as "always no". It
>> * is the responsibility of the caller to distinguish between PROT_NONE
>> * protections and NUMA hinting fault protections.
>> */
>> static inline int pte_protnone(pte_t pte)
>> {
>> return 0;
>> }
>>
>> static inline int pmd_protnone(pmd_t pmd)
>> {
>> return 0;
>> }
>> #endif /* CONFIG_NUMA_BALANCING */
>
> True, #ifdef here can be dropped, done. There is something I had missed
> before, pfn_pmd() requires #ifdef CONFIG_TRANSPARENT_HUGEPAGE instead. We
> need a pmd_t here with given prot. We cannot go via pfn_pte() followed by
> pte_pmd(), as the later is platform specific and not available in general.
As many things require CONFIG_TRANSPARENT_HUGEPAGE, maybe it would be
worth creating an additional C file with the related functions and
build it conditionnaly to CONFIG_TRANSPARENT_HUGEPAGE
Christophe
>
>>
>>> +static void __init pte_protnone_tests(unsigned long pfn, pgprot_t prot)
>>> +{
>>> + pte_t pte = pfn_pte(pfn, prot);
>>> +
>>> + WARN_ON(!pte_protnone(pte));
>>> + WARN_ON(!pte_present(pte));
>>> +}
>>> +
>>> +static void __init pmd_protnone_tests(unsigned long pfn, pgprot_t prot)
>>> +{
>>> + pmd_t pmd = pfn_pmd(pfn, prot);
>>> +
>>> + WARN_ON(!pmd_protnone(pmd));
>>> + WARN_ON(!pmd_present(pmd));
>>> +}
>>> +#else
>>> +static void __init pte_protnone_tests(unsigned long pfn, pgprot_t
>>> prot) { }
>>> +static void __init pmd_protnone_tests(unsigned long pfn, pgprot_t
>>> prot) { }
>>> +#endif
>>> +
>>> +#ifdef CONFIG_ARCH_HAS_PTE_DEVMAP
>>
>> Same here, in include/linux/mm.h we have:
>>
>> #ifndef CONFIG_ARCH_HAS_PTE_DEVMAP
>> static inline int pte_devmap(pte_t pte)
>> {
>> return 0;
>> }
>> #endif
>>
>>
>>> +static void __init pte_devmap_tests(unsigned long pfn, pgprot_t prot)
>>> +{
>>> + pte_t pte = pfn_pte(pfn, prot);
>>> +
>>> + WARN_ON(!pte_devmap(pte_mkdevmap(pte)));
>>> +}
>>> +
>>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>
>> Same. In inlude/asm-generic/pgtables.h you have:
>>
>> #if !defined(CONFIG_ARCH_HAS_PTE_DEVMAP) ||
>> !defined(CONFIG_TRANSPARENT_HUGEPAGE)
>> static inline int pmd_devmap(pmd_t pmd)
>> {
>> return 0;
>> }
>> static inline int pud_devmap(pud_t pud)
>> {
>> return 0;
>> }
>> static inline int pgd_devmap(pgd_t pgd)
>> {
>> return 0;
>> }
>> #endif
>>
>>> +static void __init pmd_devmap_tests(unsigned long pfn, pgprot_t prot)
>>> +{
>>> + pmd_t pmd = pfn_pmd(pfn, prot);
>>> +
>>> + WARN_ON(!pmd_devmap(pmd_mkdevmap(pmd)));
>>> +}
>>> +
>>> +#ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
>>
>> Same, see above
>
> Even though pxx_devmap() fallback definitions are present, pxx_mkdevmap()
> ones are still missing. We will have to add them first as a pre-requisite
> patch (which might not be popular without any non-debug use case) in order
> to drop these #ifdefs here.
>
>>
>>> +static void __init pud_devmap_tests(unsigned long pfn, pgprot_t prot)
>>> +{
>>> + pud_t pud = pfn_pud(pfn, prot);
>>> +
>>> + WARN_ON(!pud_devmap(pud_mkdevmap(pud)));
>>> +}
>>> +#else
>>> +static void __init pud_devmap_tests(unsigned long pfn, pgprot_t prot) { }
>>> +#endif
>>> +#else
>>> +static void __init pmd_devmap_tests(unsigned long pfn, pgprot_t prot) { }
>>> +static void __init pud_devmap_tests(unsigned long pfn, pgprot_t prot) { }
>>> +#endif
>>> +#else
>>> +static void __init pte_devmap_tests(unsigned long pfn, pgprot_t prot) { }
>>> +static void __init pmd_devmap_tests(unsigned long pfn, pgprot_t prot) { }
>>> +static void __init pud_devmap_tests(unsigned long pfn, pgprot_t prot) { }
>>> +#endif
>>> +
>>> +#ifdef CONFIG_MEM_SOFT_DIRTY
>>
>> Same, they always exist, see include/asm-generic/pgtable.h
>
> Yeah, this can be dropped. Though will have to again add TRANSPARENT_HUGEPAGE
> to protect pfn_pmd() as explained before.
>
>>
>>> +static void __init pte_soft_dirty_tests(unsigned long pfn, pgprot_t prot)
>>> +{
>>> + pte_t pte = pfn_pte(pfn, prot);
>>> +
>>> + WARN_ON(!pte_soft_dirty(pte_mksoft_dirty(pte)));
>>> + WARN_ON(pte_soft_dirty(pte_clear_soft_dirty(pte)));
>>> +}
>>> +
>>> +static void __init pte_swap_soft_dirty_tests(unsigned long pfn,
>>> pgprot_t prot)
>>> +{
>>> + pte_t pte = pfn_pte(pfn, prot);
>>> +
>>> + WARN_ON(!pte_swp_soft_dirty(pte_swp_mksoft_dirty(pte)));
>>> + WARN_ON(pte_swp_soft_dirty(pte_swp_clear_soft_dirty(pte)));
>>> +}
>>> +
>>> +#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
>>
>> Same
>
> True, #ifdef here can be dropped, done.
>
>>
>>> +static void __init pmd_soft_dirty_tests(unsigned long pfn, pgprot_t prot)
>>> +{
>>> + pmd_t pmd = pfn_pmd(pfn, prot);
>>> +
>>> + WARN_ON(!pmd_soft_dirty(pmd_mksoft_dirty(pmd)));
>>> + WARN_ON(pmd_soft_dirty(pmd_clear_soft_dirty(pmd)));
>>> +}
>>> +
>>> +static void __init pmd_swap_soft_dirty_tests(unsigned long pfn,
>>> pgprot_t prot)
>>> +{
>>> + pmd_t pmd = pfn_pmd(pfn, prot);
>>> +
>>> + WARN_ON(!pmd_swp_soft_dirty(pmd_swp_mksoft_dirty(pmd)));
>>> + WARN_ON(pmd_swp_soft_dirty(pmd_swp_clear_soft_dirty(pmd)));
>>> +}
>>> +#else
>>> +static void __init pmd_soft_dirty_tests(unsigned long pfn,
>>> pgprot_t prot) { }
>>> +static void __init pmd_swap_soft_dirty_tests(unsigned long pfn,
>>> pgprot_t prot)
>>> +{
>>> +}
>>> +#endif
>>> +#else
>>> +static void __init pte_soft_dirty_tests(unsigned long pfn,
>>> pgprot_t prot) { }
>>> +static void __init pmd_soft_dirty_tests(unsigned long pfn,
>>> pgprot_t prot) { }
>>> +static void __init pte_swap_soft_dirty_tests(unsigned long pfn,
>>> pgprot_t prot)
>>> +{
>>> +}
>>> +static void __init pmd_swap_soft_dirty_tests(unsigned long pfn,
>>> pgprot_t prot)
>>> +{
>>> +}
>>> +#endif
>>> +
>>> +static void __init pte_swap_tests(unsigned long pfn, pgprot_t prot)
>>> +{
>>> + swp_entry_t swp;
>>> + pte_t pte;
>>> +
>>> + pte = pfn_pte(pfn, prot);
>>> + swp = __pte_to_swp_entry(pte);
>>> + WARN_ON(!pte_same(pte, __swp_entry_to_pte(swp)));
>>> +}
>>> +
>>> +#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
>>> +static void __init pmd_swap_tests(unsigned long pfn, pgprot_t prot)
>>> +{
>>> + swp_entry_t swp;
>>> + pmd_t pmd;
>>> +
>>> + pmd = pfn_pmd(pfn, prot);
>>> + swp = __pmd_to_swp_entry(pmd);
>>> + WARN_ON(!pmd_same(pmd, __swp_entry_to_pmd(swp)));
>>> +}
>>> +#else
>>> +static void __init pmd_swap_tests(unsigned long pfn, pgprot_t prot) { }
>>> +#endif
>>> +
>>> +#ifdef CONFIG_MIGRATION
>>
>> Same. See include/linux/swapops.h
>
> True, #ifdef here can be dropped, done. Though will have to again add
> back TRANSPARENT_HUGEPAGE to protect pfn_pmd() as explained before.
>
>>
>>> +static void __init swap_migration_tests(struct page *page)
>>> +{
>>> + swp_entry_t swp;
>>> +
>>> + /*
>>> + * make_migration_entry() expects given page to be
>>> + * locked, otherwise it stumbles upon a BUG_ON().
>>> + */
>>> + __SetPageLocked(page);
>>> + swp = make_migration_entry(page, 1);
>>> + WARN_ON(!is_migration_entry(swp));
>>> + WARN_ON(!is_write_migration_entry(swp));
>>> +
>>> + make_migration_entry_read(&swp);
>>> + WARN_ON(!is_migration_entry(swp));
>>> + WARN_ON(is_write_migration_entry(swp));
>>> +
>>> + swp = make_migration_entry(page, 0);
>>> + WARN_ON(!is_migration_entry(swp));
>>> + WARN_ON(is_write_migration_entry(swp));
>>> + __ClearPageLocked(page);
>>> +}
>>> +#else
>>> +static void __init swap_migration_tests(struct page *page) { }
>>> +#endif
>>> +
>>> +#ifdef CONFIG_HUGETLB_PAGE
>>> +static void __init hugetlb_tests(unsigned long pfn, pgprot_t prot)
>>> +{
>>> +#ifdef CONFIG_ARCH_WANT_GENERAL_HUGETLB
>>> + pte_t pte = pfn_pte(pfn, prot);
>>> +
>>> + WARN_ON(!pte_huge(pte_mkhuge(pte)));
>>
>> We also need tests on hugepd stuff
>
> Sure, but lets discuss this on the other thread.
>
>>
>>> +#endif
>>> +}
>>> +#else
>>> +static void __init hugetlb_tests(unsigned long pfn, pgprot_t prot) { }
>>> +#endif
>>> +
>>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>
>> Same, see include/asm-generic/pgtable.h
>
> This is required to protect pxx_mknotpresent() which does not have a
> fall back and pfn_pmd()/pfn_pud() helpers have similar situation as
> well.
>
>>
>>> +static void __init pmd_thp_tests(unsigned long pfn, pgprot_t prot)
>>> +{
>>> + pmd_t pmd;
>>> +
>>> + /*
>>> + * pmd_trans_huge() and pmd_present() must return negative
>>> + * after MMU invalidation with pmd_mknotpresent().
>>> + */
>>> + pmd = pfn_pmd(pfn, prot);
>>> + WARN_ON(!pmd_trans_huge(pmd_mkhuge(pmd)));
>>> +
>>> + /*
>>> + * Though platform specific test exclusions are not ideal,
>>> + * in this case S390 does not define pmd_mknotpresent()
>>> + * which should be tested on other platforms enabling THP.
>>> + */
>>> +#ifndef CONFIG_S390
>>> + WARN_ON(pmd_trans_huge(pmd_mknotpresent(pmd)));
>>> + WARN_ON(pmd_present(pmd_mknotpresent(pmd)));
>>> +#endif
>>
>> Can we add a stub on S390 instead ?
>
> Actually we dont have to. pmd_mknotpresent() is required for platforms
> that do not have __HAVE_ARCH_PMDP_INVALIDATE. Hence can wrap this code
> with !__HAVE_ARCH_PMDP_INVALIDATE to prevent build failures on such
> platforms like s390.
>
>>
>>> +}
>>> +
>>> +#ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
>>
>> Same ?
>
> The problem is, neither pud_mknotpresent() nor pfn_pud() have a generic
> fallback definition. So will have to keep this #ifdef.
>
>>
>>> +static void __init pud_thp_tests(unsigned long pfn, pgprot_t prot)
>>> +{
>>> + pud_t pud;
>>> +
>>> + /*
>>> + * pud_trans_huge() and pud_present() must return negative
>>> + * after MMU invalidation with pud_mknotpresent().
>>> + */
>>> + pud = pfn_pud(pfn, prot);
>>> + WARN_ON(!pud_trans_huge(pud_mkhuge(pud)));
>>> + WARN_ON(pud_trans_huge(pud_mknotpresent(pud)));
>>> + WARN_ON(pud_present(pud_mknotpresent(pud)));
>>> +}
>>> +#else
>>> +static void __init pud_thp_tests(unsigned long pfn, pgprot_t prot) { }
>>> +#endif
>>> +#else
>>> +static void __init pmd_thp_tests(unsigned long pfn, pgprot_t prot) { }
>>> +static void __init pud_thp_tests(unsigned long pfn, pgprot_t prot) { }
>>> +#endif
>>> +
>>> static unsigned long __init get_random_vaddr(void)
>>> {
>>> unsigned long random_vaddr, random_pages, total_user_pages;
>>> @@ -302,13 +567,14 @@ static unsigned long __init get_random_vaddr(void)
>>> void __init debug_vm_pgtable(void)
>>> {
>>> struct mm_struct *mm;
>>> + struct page *page;
>>> pgd_t *pgdp;
>>> p4d_t *p4dp, *saved_p4dp;
>>> pud_t *pudp, *saved_pudp;
>>> pmd_t *pmdp, *saved_pmdp, pmd;
>>> pte_t *ptep;
>>> pgtable_t saved_ptep;
>>> - pgprot_t prot;
>>> + pgprot_t prot, protnone;
>>> phys_addr_t paddr;
>>> unsigned long vaddr, pte_aligned, pmd_aligned;
>>> unsigned long pud_aligned, p4d_aligned, pgd_aligned;
>>> @@ -322,6 +588,25 @@ void __init debug_vm_pgtable(void)
>>> return;
>>> }
>>> + /*
>>> + * swap_migration_tests() requires a dedicated page as it needs to
>>> + * be locked before creating a migration entry from it. Locking the
>>> + * page that actually maps kernel text ('start_kernel') can be real
>>> + * problematic. Lets allocate a dedicated page explicitly for this
>>> + * purpose that will be freed later.
>>> + */
>>> + page = alloc_page(GFP_KERNEL);
>>
>> Can we do the page allocation and freeing in swap_migration_tests()
>> instead ?
>
> Although all the resources used in the helpers have been allocated
> in the main
> function itself before being passed down and later freed. But may be just an
> exception could be made for swap_migration_tests() function as the allocated
> page is being exclusively used here. Later on if we need this page for some
> other future tests, then will have to move it back to debug_vm_pgtable().
^ permalink raw reply
* Re: [PATCH] ima: add a new CONFIG for loading arch-specific policies
From: Heiko Carstens @ 2020-03-02 21:21 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-s390, linux-efi, Nayna Jain, Linux Kernel Mailing List,
Philipp Rudo, Martin Schwidefsky, linux-integrity, linuxppc-dev,
Ard Biesheuvel
In-Reply-To: <1583161018.8544.96.camel@linux.ibm.com>
On Mon, Mar 02, 2020 at 09:56:58AM -0500, Mimi Zohar wrote:
> On Mon, 2020-03-02 at 15:52 +0100, Ard Biesheuvel wrote:
> > On Mon, 2 Mar 2020 at 15:48, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > > index beea77046f9b..cafa66313fe2 100644
> > > > --- a/arch/x86/Kconfig
> > > > +++ b/arch/x86/Kconfig
> > > > @@ -230,6 +230,7 @@ config X86
> > > > select VIRT_TO_BUS
> > > > select X86_FEATURE_NAMES if PROC_FS
> > > > select PROC_PID_ARCH_STATUS if PROC_FS
> > > > + select IMA_SECURE_AND_OR_TRUSTED_BOOT if EFI
> > >
> > > Not everyone is interested in enabling IMA or requiring IMA runtime
> > > policies. With this patch, enabling IMA_ARCH_POLICY is therefore
> > > still left up to the person building the kernel. As a result, I'm
> > > seeing the following warning, which is kind of cool.
> > >
> > > WARNING: unmet direct dependencies detected for
> > > IMA_SECURE_AND_OR_TRUSTED_BOOT
> > > Depends on [n]: INTEGRITY [=y] && IMA [=y] && IMA_ARCH_POLICY [=n]
> > > Selected by [y]:
> > > - X86 [=y] && EFI [=y]
> > >
> > > Ard, Michael, Martin, just making sure this type of warning is
> > > acceptable before upstreaming this patch. I would appreciate your
> > > tags.
> > >
> >
> > Ehm, no, warnings like these are not really acceptable. It means there
> > is an inconsistency in the way the Kconfig dependencies are defined.
> >
> > Does this help:
> >
> > select IMA_SECURE_AND_OR_TRUSTED_BOOT if EFI && IMA_ARCH_POLICY
> >
> > ?
>
> Yes, that's fine for x86. Michael, Martin, do you want something
> similar or would you prefer actually selecting IMA_ARCH_POLICY?
For s390 something like
select IMA_SECURE_AND_OR_TRUSTED_BOOT if IMA_ARCH_POLICY
should be fine.
Thanks,
Heiko
^ permalink raw reply
* Re: [PATCH v3 7/7] mm/memremap: Set caching mode for PCI P2PDMA memory to WC
From: Logan Gunthorpe @ 2020-03-02 21:20 UTC (permalink / raw)
To: Dan Williams
Cc: linux-ia64, Linux-sh, Peter Zijlstra, Catalin Marinas,
Dave Hansen, platform-driver-x86, Linux MM, Will Deacon,
Christoph Hellwig, linux-s390, David Hildenbrand, Jason Gunthorpe,
Ingo Molnar, Borislav Petkov, Andy Lutomirski, Thomas Gleixner,
Michal Hocko, Linux ARM, Eric Badger, Linux Kernel Mailing List,
Andrew Morton, linuxppc-dev
In-Reply-To: <CAPcyv4gNi3sesGnujShStoF8bi8kYg+MQkqhQRCT_1+wex5wbw@mail.gmail.com>
On 2020-02-29 3:47 p.m., Dan Williams wrote:
> On Fri, Feb 21, 2020 at 10:25 AM Logan Gunthorpe <logang@deltatee.com> wrote:
>>
>> PCI BAR IO memory should never be mapped as WB, however prior to this
>> the PAT bits were set WB and it was typically overridden by MTRR
>> registers set by the firmware.
>>
>> Set PCI P2PDMA memory to be WC (writecombining) as the only current
>> user (the NVMe CMB) was originally mapped WC before the P2PDMA code
>> replaced the mapping with devm_memremap_pages().
>
> Will the change to UC regress this existing use case?
I don't think so. They've been essentially mapped UC for a long time now
(since the P2PDMA patch set was merged) and nobody has complained.
^ permalink raw reply
* Re: [RFC 00/11] perf: Enhancing perf to export processor hazard information
From: Paul Clarke @ 2020-03-02 21:08 UTC (permalink / raw)
To: Ravi Bangoria, linuxppc-dev, linux-kernel
Cc: mark.rutland, ak, peterz, alexey.budankov, adrian.hunter, acme,
alexander.shishkin, yao.jin, maddy, paulus, eranian,
robert.richter, namhyung, kim.phillips, jolsa, mingo, kan.liang
In-Reply-To: <20200302052355.36365-1-ravi.bangoria@linux.ibm.com>
On 3/1/20 11:23 PM, Ravi Bangoria wrote:
> Most modern microprocessors employ complex instruction execution
> pipelines such that many instructions can be 'in flight' at any
> given point in time. Various factors affect this pipeline and
> hazards are the primary among them. Different types of hazards
> exist - Data hazards, Structural hazards and Control hazards.
> Data hazard is the case where data dependencies exist between
> instructions in different stages in the pipeline. Structural
> hazard is when the same processor hardware is needed by more
> than one instruction in flight at the same time. Control hazards
> are more the branch misprediction kinds.
>
> Information about these hazards are critical towards analyzing
> performance issues and also to tune software to overcome such
> issues. Modern processors export such hazard data in Performance
> Monitoring Unit (PMU) registers. Ex, 'Sampled Instruction Event
> Register' on IBM PowerPC[1][2] and 'Instruction-Based Sampling' on
> AMD[3] provides similar information.
>
> Implementation detail:
>
> A new sample_type called PERF_SAMPLE_PIPELINE_HAZ is introduced.
> If it's set, kernel converts arch specific hazard information
> into generic format:
>
> struct perf_pipeline_haz_data {
> /* Instruction/Opcode type: Load, Store, Branch .... */
> __u8 itype;
At the risk of bike-shedding (in an RFC, no less), "itype" doesn't convey enough meaning to me. "inst_type"? I see in 03/11, you use "perf_inst_type".
> /* Instruction Cache source */
> __u8 icache;
Possibly same here, and you use "perf_inst_cache" in 03/11.
> /* Instruction suffered hazard in pipeline stage */
> __u8 hazard_stage;
> /* Hazard reason */
> __u8 hazard_reason;
> /* Instruction suffered stall in pipeline stage */
> __u8 stall_stage;
> /* Stall reason */
> __u8 stall_reason;
> __u16 pad;
> };
>
> ... which can be read by user from mmap() ring buffer. With this
> approach, sample perf report in hazard mode looks like (On IBM
> PowerPC):
>
> # ./perf record --hazard ./ebizzy
> # ./perf report --hazard
> Overhead Symbol Shared Instruction Type Hazard Stage Hazard Reason Stall Stage Stall Reason ICache access
> 36.58% [.] thread_run ebizzy Load LSU Mispredict LSU Load fin L1 hit
> 9.46% [.] thread_run ebizzy Load LSU Mispredict LSU Dcache_miss L1 hit
> 1.76% [.] thread_run ebizzy Fixed point - - - - L1 hit
> 1.31% [.] thread_run ebizzy Load LSU ERAT Miss LSU Load fin L1 hit
> 1.27% [.] thread_run ebizzy Load LSU Mispredict - - L1 hit
> 1.16% [.] thread_run ebizzy Fixed point - - FXU Fixed cycle L1 hit
> 0.50% [.] thread_run ebizzy Fixed point ISU Source Unavailable FXU Fixed cycle L1 hit
> 0.30% [.] thread_run ebizzy Load LSU LMQ Full, DERAT Miss LSU Load fin L1 hit
> 0.24% [.] thread_run ebizzy Load LSU ERAT Miss - - L1 hit
> 0.08% [.] thread_run ebizzy - - - BRU Fixed cycle L1 hit
> 0.05% [.] thread_run ebizzy Branch - - BRU Fixed cycle L1 hit
> 0.04% [.] thread_run ebizzy Fixed point ISU Source Unavailable - - L1 hit
How are these to be interpreted? This is great information, but is it possible to make it more readable for non-experts? If each of these map 1:1 with hardware events, should you emit the name of the event here, so that can be used to look up further information? For example, does the first line map to PM_CMPLU_STALL_LSU_FIN?
What was "Mispredict[ed]"? (Is it different from a branch misprediction?) And how does this relate to "L1 hit"?
Can we emit "Load finish" instead of "Load fin" for easier reading? 03/11 also has "Marked fin before NTC".
Nit: why does "Dcache_miss" have an underscore and none of the others?
> Also perf annotate with hazard data:
> │ static int
> │ compare(const void *p1, const void *p2)
> │ {
> 33.23 │ std r31,-8(r1)
> │ {haz_stage: LSU, haz_reason: ERAT Miss, stall_stage: LSU, stall_reason: Store, icache: L1 hit}
> │ {haz_stage: LSU, haz_reason: ERAT Miss, stall_stage: LSU, stall_reason: Store, icache: L1 hit}
> │ {haz_stage: LSU, haz_reason: Load Hit Store, stall_stage: LSU, stall_reason: -, icache: L3 hit}
> │ {haz_stage: LSU, haz_reason: ERAT Miss, stall_stage: -, stall_reason: -, icache: L1 hit}
> │ {haz_stage: LSU, haz_reason: ERAT Miss, stall_stage: LSU, stall_reason: Store, icache: L1 hit}
> │ {haz_stage: LSU, haz_reason: ERAT Miss, stall_stage: LSU, stall_reason: Store, icache: L1 hit}
> 0.84 │ stdu r1,-64(r1)
> │ {haz_stage: LSU, haz_reason: ERAT Miss, stall_stage: -, stall_reason: -, icache: L1 hit}
> 0.24 │ mr r31,r1
> │ {haz_stage: -, haz_reason: -, stall_stage: -, stall_reason: -, icache: L1 hit}
> 21.18 │ std r3,32(r31)
> │ {haz_stage: LSU, haz_reason: ERAT Miss, stall_stage: LSU, stall_reason: Store, icache: L1 hit}
> │ {haz_stage: LSU, haz_reason: ERAT Miss, stall_stage: LSU, stall_reason: Store, icache: L1 hit}
> │ {haz_stage: LSU, haz_reason: ERAT Miss, stall_stage: LSU, stall_reason: Store, icache: L1 hit}
>
>
> Patches:
> - Patch #1 is a simple cleanup patch
> - Patch #2, #3, #4 implements generic and arch specific kernel
> infrastructure
> - Patch #5 enables perf record and script with hazard mode
> - Patch #6, #7, #8 enables perf report with hazard mode
> - Patch #9, #10, #11 enables perf annotate with hazard mode
>
> Note:
> - This series is based on the talk by Madhavan in LPC 2018[4]. This is
> just an early RFC to get comments about the approach and not intended
> to be merged yet.
> - I've prepared the series base on v5.6-rc3. But it depends on generic
> perf annotate fixes [5][6] which are already merged by Arnaldo in
> perf/urgent and perf/core.
>
> [1]: Book III, Section 9.4.10:
> https://openpowerfoundation.org/?resource_lib=power-isa-version-3-0
> [2]: https://wiki.raptorcs.com/w/images/6/6b/POWER9_PMU_UG_v12_28NOV2018_pub.pdf#G9.1106986
This document is also available from the "IBM Portal for OpenPOWER" under the "All IBM Material for OpenPOWER" https://www-355.ibm.com/systems/power/openpower/tgcmDocumentRepository.xhtml?aliasId=OpenPOWER, under each of the individual modules. (Well hidden, it might be said, and not a simple link like you have here.)
> [3]: https://www.amd.com/system/files/TechDocs/24593.pdf#G19.1089550
> [4]: https://linuxplumbersconf.org/event/2/contributions/76/
> [5]: http://lore.kernel.org/r/20200204045233.474937-1-ravi.bangoria@linux.ibm.com
> [6]: http://lore.kernel.org/r/20200213064306.160480-1-ravi.bangoria@linux.ibm.com
PC
^ permalink raw reply
* Re: [PATCH v3 6/7] mm/memory_hotplug: Add pgprot_t to mhp_params
From: Dan Williams @ 2020-03-02 20:26 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: Michal Hocko, linux-ia64, Linux-sh, Peter Zijlstra,
Catalin Marinas, Dave Hansen, platform-driver-x86, Linux MM,
Will Deacon, Christoph Hellwig, linux-s390, David Hildenbrand,
Ingo Molnar, Borislav Petkov, Andy Lutomirski, Thomas Gleixner,
Michal Hocko, Linux ARM, Eric Badger, Linux Kernel Mailing List,
Andrew Morton, linuxppc-dev
In-Reply-To: <8b13f6aa-77b7-a47d-1a49-b8e2f800ac9d@deltatee.com>
On Mon, Mar 2, 2020 at 10:55 AM Logan Gunthorpe <logang@deltatee.com> wrote:
>
>
>
> On 2020-02-29 3:44 p.m., Dan Williams wrote:
> > On Fri, Feb 21, 2020 at 10:25 AM Logan Gunthorpe <logang@deltatee.com> wrote:
> >>
> >> devm_memremap_pages() is currently used by the PCI P2PDMA code to create
> >> struct page mappings for IO memory. At present, these mappings are created
> >> with PAGE_KERNEL which implies setting the PAT bits to be WB. However, on
> >> x86, an mtrr register will typically override this and force the cache
> >> type to be UC-. In the case firmware doesn't set this register it is
> >> effectively WB and will typically result in a machine check exception
> >> when it's accessed.
> >>
> >> Other arches are not currently likely to function correctly seeing they
> >> don't have any MTRR registers to fall back on.
> >>
> >> To solve this, provide a way to specify the pgprot value explicitly to
> >> arch_add_memory().
> >>
> >> Of the arches that support MEMORY_HOTPLUG: x86_64, and arm64 need a simple
> >> change to pass the pgprot_t down to their respective functions which set
> >> up the page tables. For x86_32, set the page tables explicitly using
> >> _set_memory_prot() (seeing they are already mapped). For ia64, s390 and
> >> sh, reject anything but PAGE_KERNEL settings -- this should be fine,
> >> for now, seeing these architectures don't support ZONE_DEVICE.
> >>
> >> A check in __add_pages() is also added to ensure the pgprot parameter was
> >> set for all arches.
> >>
> >> Cc: Dan Williams <dan.j.williams@intel.com>
> >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> >> Acked-by: David Hildenbrand <david@redhat.com>
> >> Acked-by: Michal Hocko <mhocko@suse.com>
> >> ---
> >> arch/arm64/mm/mmu.c | 3 ++-
> >> arch/ia64/mm/init.c | 3 +++
> >> arch/powerpc/mm/mem.c | 3 ++-
> >> arch/s390/mm/init.c | 3 +++
> >> arch/sh/mm/init.c | 3 +++
> >> arch/x86/mm/init_32.c | 5 +++++
> >> arch/x86/mm/init_64.c | 2 +-
> >> include/linux/memory_hotplug.h | 2 ++
> >> mm/memory_hotplug.c | 5 ++++-
> >> mm/memremap.c | 6 +++---
> >> 10 files changed, 28 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >> index ee37bca8aba8..ea3fa844a8a2 100644
> >> --- a/arch/arm64/mm/mmu.c
> >> +++ b/arch/arm64/mm/mmu.c
> >> @@ -1058,7 +1058,8 @@ int arch_add_memory(int nid, u64 start, u64 size,
> >> flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
> >>
> >> __create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
> >> - size, PAGE_KERNEL, __pgd_pgtable_alloc, flags);
> >> + size, params->pgprot, __pgd_pgtable_alloc,
> >> + flags);
> >>
> >> memblock_clear_nomap(start, size);
> >>
> >> diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
> >> index 97bbc23ea1e3..d637b4ea3147 100644
> >> --- a/arch/ia64/mm/init.c
> >> +++ b/arch/ia64/mm/init.c
> >> @@ -676,6 +676,9 @@ int arch_add_memory(int nid, u64 start, u64 size,
> >> unsigned long nr_pages = size >> PAGE_SHIFT;
> >> int ret;
> >>
> >> + if (WARN_ON_ONCE(params->pgprot.pgprot != PAGE_KERNEL.pgprot))
> >> + return -EINVAL;
> >> +
> >> ret = __add_pages(nid, start_pfn, nr_pages, params);
> >> if (ret)
> >> printk("%s: Problem encountered in __add_pages() as ret=%d\n",
> >> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> >> index 19b1da5d7eca..832412bc7fad 100644
> >> --- a/arch/powerpc/mm/mem.c
> >> +++ b/arch/powerpc/mm/mem.c
> >> @@ -138,7 +138,8 @@ int __ref arch_add_memory(int nid, u64 start, u64 size,
> >> resize_hpt_for_hotplug(memblock_phys_mem_size());
> >>
> >> start = (unsigned long)__va(start);
> >> - rc = create_section_mapping(start, start + size, nid, PAGE_KERNEL);
> >> + rc = create_section_mapping(start, start + size, nid,
> >> + params->pgprot);
> >> if (rc) {
> >> pr_warn("Unable to create mapping for hot added memory 0x%llx..0x%llx: %d\n",
> >> start, start + size, rc);
> >> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> >> index e9e4a7abd0cc..87b2d024e75a 100644
> >> --- a/arch/s390/mm/init.c
> >> +++ b/arch/s390/mm/init.c
> >> @@ -277,6 +277,9 @@ int arch_add_memory(int nid, u64 start, u64 size,
> >> if (WARN_ON_ONCE(params->altmap))
> >> return -EINVAL;
> >>
> >> + if (WARN_ON_ONCE(params->pgprot.pgprot != PAGE_KERNEL.pgprot))
> >> + return -EINVAL;
> >> +
> >> rc = vmem_add_mapping(start, size);
> >> if (rc)
> >> return rc;
> >> diff --git a/arch/sh/mm/init.c b/arch/sh/mm/init.c
> >> index e5114c053364..b9de2d4fa57e 100644
> >> --- a/arch/sh/mm/init.c
> >> +++ b/arch/sh/mm/init.c
> >> @@ -412,6 +412,9 @@ int arch_add_memory(int nid, u64 start, u64 size,
> >> unsigned long nr_pages = size >> PAGE_SHIFT;
> >> int ret;
> >>
> >> + if (WARN_ON_ONCE(params->pgprot.pgprot != PAGE_KERNEL.pgprot)
> >> + return -EINVAL;
> >> +
> >> /* We only have ZONE_NORMAL, so this is easy.. */
> >> ret = __add_pages(nid, start_pfn, nr_pages, params);
> >> if (unlikely(ret))
> >> diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
> >> index e25a4218e6ff..96d8e4fb1cc8 100644
> >> --- a/arch/x86/mm/init_32.c
> >> +++ b/arch/x86/mm/init_32.c
> >> @@ -858,6 +858,11 @@ int arch_add_memory(int nid, u64 start, u64 size,
> >> {
> >> unsigned long start_pfn = start >> PAGE_SHIFT;
> >> unsigned long nr_pages = size >> PAGE_SHIFT;
> >> + int ret;
> >> +
> >> + ret = _set_memory_prot(start, nr_pages, params->pgprot);
> >
> > Perhaps a comment since it's not immediately obvious where the
> > PAGE_KERNEL prot was established, and perhaps add a conditional to
> > skip this call in the param->pgprot == PAGE_KERNEL case?
>
> Yes I can add the skip in the PAGE_KERNEL case. Though I'm not sure what
> you are asking for with regards to the comment. Just that pgprot is set
> by the caller usually to PAGE_KERNEL?
No, I'm reacting to this comment in the changelog "For x86_32, set the
page tables explicitly using _set_memory_prot() (seeing they are
already mapped)". You've done some investigation that
x86_32::arch_add_memory() expects the page tables to be already
established. I think that's worth capturing inline in the code for
other people doing cross-arch arch_add_memory() changes.
^ permalink raw reply
* Re: [RFC 00/11] perf: Enhancing perf to export processor hazard information
From: Stephane Eranian @ 2020-03-02 20:21 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Mark Rutland, Ravi Bangoria, Andi Kleen, Alexander Shishkin,
Jiri Olsa, LKML, Arnaldo Carvalho de Melo, Adrian Hunter,
Robert Richter, yao.jin, Ingo Molnar, Paul Mackerras, maddy,
Namhyung Kim, Phillips, Kim, linuxppc-dev, Alexey Budankov,
Liang, Kan
In-Reply-To: <20200302101332.GS18400@hirez.programming.kicks-ass.net>
On Mon, Mar 2, 2020 at 2:13 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Mar 02, 2020 at 10:53:44AM +0530, Ravi Bangoria wrote:
> > Modern processors export such hazard data in Performance
> > Monitoring Unit (PMU) registers. Ex, 'Sampled Instruction Event
> > Register' on IBM PowerPC[1][2] and 'Instruction-Based Sampling' on
> > AMD[3] provides similar information.
> >
> > Implementation detail:
> >
> > A new sample_type called PERF_SAMPLE_PIPELINE_HAZ is introduced.
> > If it's set, kernel converts arch specific hazard information
> > into generic format:
> >
> > struct perf_pipeline_haz_data {
> > /* Instruction/Opcode type: Load, Store, Branch .... */
> > __u8 itype;
> > /* Instruction Cache source */
> > __u8 icache;
> > /* Instruction suffered hazard in pipeline stage */
> > __u8 hazard_stage;
> > /* Hazard reason */
> > __u8 hazard_reason;
> > /* Instruction suffered stall in pipeline stage */
> > __u8 stall_stage;
> > /* Stall reason */
> > __u8 stall_reason;
> > __u16 pad;
> > };
>
> Kim, does this format indeed work for AMD IBS?
Personally, I don't like the term hazard. This is too IBM Power
specific. We need to find a better term, maybe stall or penalty.
Also worth considering is the support of ARM SPE (Statistical
Profiling Extension) which is their version of IBS.
Whatever gets added need to cover all three with no limitations.
^ permalink raw reply
* [RESEND PATCH] soc: fsl: Enable compile testing of FSL_RCPM
From: Krzysztof Kozlowski @ 2020-03-02 20:14 UTC (permalink / raw)
To: Li Yang, linuxppc-dev, linux-arm-kernel, linux-kernel; +Cc: Krzysztof Kozlowski
FSL_RCPM can be compile tested to increase build coverage.
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
drivers/soc/fsl/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig
index 4df32bc4c7a6..e142662d7c99 100644
--- a/drivers/soc/fsl/Kconfig
+++ b/drivers/soc/fsl/Kconfig
@@ -43,7 +43,7 @@ config DPAA2_CONSOLE
config FSL_RCPM
bool "Freescale RCPM support"
- depends on PM_SLEEP && (ARM || ARM64)
+ depends on PM_SLEEP && (ARM || ARM64 || COMPILE_TEST)
help
The NXP QorIQ Processors based on ARM Core have RCPM module
(Run Control and Power Management), which performs all device-level
--
2.17.1
^ permalink raw reply related
* Re: [Intel-gfx] [PATCH v7 00/12] Introduce CAP_PERFMON to secure system performance monitoring and observability
From: James Morris @ 2020-03-02 19:44 UTC (permalink / raw)
To: Serge Hallyn
Cc: linux-man, linux-doc@vger.kernel.org, Peter Zijlstra,
joonas.lahtinen@linux.intel.com, Alexei Starovoitov,
Stephane Eranian, Paul Mackerras, Will Deacon, Ingo Molnar,
Andi Kleen, Jiri Olsa, Alexey Budankov, Igor Lubashev,
oprofile-list, Stephen Smalley, selinux@vger.kernel.org,
intel-gfx@lists.freedesktop.org, Arnaldo Carvalho de Melo,
Thomas Gleixner, linux-arm-kernel, linux-parisc@vger.kernel.org,
linux-kernel, linux-security-module@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, Helge Deller
In-Reply-To: <20200302001913.GA21145@sl>
On Sun, 1 Mar 2020, Serge Hallyn wrote:
> Thanks, this looks good to me, in keeping with the CAP_SYSLOG break.
>
> Acked-by: Serge E. Hallyn <serge@hallyn.com>
>
> for the set.
>
> James/Ingo/Peter, if noone has remaining objections, whose branch
> should these go in through?
>
> thanks,
I was assuming via the perf tree, but I am happy to take them.
> -serge
>
> On Tue, Feb 25, 2020 at 12:55:54PM +0300, Alexey Budankov wrote:
> >
> > Hi,
> >
> > Is there anything else I could do in order to move the changes forward
> > or is something still missing from this patch set?
> > Could you please share you mind?
> >
> > Thanks,
> > Alexey
> >
> > On 17.02.2020 11:02, Alexey Budankov wrote:
> > >
> > > Currently access to perf_events, i915_perf and other performance
> > > monitoring and observability subsystems of the kernel is open only for
> > > a privileged process [1] with CAP_SYS_ADMIN capability enabled in the
> > > process effective set [2].
> > >
> > > This patch set introduces CAP_PERFMON capability designed to secure
> > > system performance monitoring and observability operations so that
> > > CAP_PERFMON would assist CAP_SYS_ADMIN capability in its governing role
> > > for performance monitoring and observability subsystems of the kernel.
> > >
> > > CAP_PERFMON intends to harden system security and integrity during
> > > performance monitoring and observability operations by decreasing attack
> > > surface that is available to a CAP_SYS_ADMIN privileged process [2].
> > > Providing the access to performance monitoring and observability
> > > operations under CAP_PERFMON capability singly, without the rest of
> > > CAP_SYS_ADMIN credentials, excludes chances to misuse the credentials
> > > and makes the operation more secure. Thus, CAP_PERFMON implements the
> > > principal of least privilege for performance monitoring and
> > > observability operations (POSIX IEEE 1003.1e: 2.2.2.39 principle of
> > > least privilege: A security design principle that states that a process
> > > or program be granted only those privileges (e.g., capabilities)
> > > necessary to accomplish its legitimate function, and only for the time
> > > that such privileges are actually required)
> > >
> > > CAP_PERFMON intends to meet the demand to secure system performance
> > > monitoring and observability operations for adoption in security
> > > sensitive, restricted, multiuser production environments (e.g. HPC
> > > clusters, cloud and virtual compute environments), where root or
> > > CAP_SYS_ADMIN credentials are not available to mass users of a system,
> > > and securely unblock accessibility of system performance monitoring and
> > > observability operations beyond root and CAP_SYS_ADMIN use cases.
> > >
> > > CAP_PERFMON intends to take over CAP_SYS_ADMIN credentials related to
> > > system performance monitoring and observability operations and balance
> > > amount of CAP_SYS_ADMIN credentials following the recommendations in
> > > the capabilities man page [2] for CAP_SYS_ADMIN: "Note: this capability
> > > is overloaded; see Notes to kernel developers, below." For backward
> > > compatibility reasons access to system performance monitoring and
> > > observability subsystems of the kernel remains open for CAP_SYS_ADMIN
> > > privileged processes but CAP_SYS_ADMIN capability usage for secure
> > > system performance monitoring and observability operations is
> > > discouraged with respect to the designed CAP_PERFMON capability.
> > >
> > > Possible alternative solution to this system security hardening,
> > > capabilities balancing task of making performance monitoring and
> > > observability operations more secure and accessible could be to use
> > > the existing CAP_SYS_PTRACE capability to govern system performance
> > > monitoring and observability subsystems. However CAP_SYS_PTRACE
> > > capability still provides users with more credentials than are
> > > required for secure performance monitoring and observability
> > > operations and this excess is avoided by the designed CAP_PERFMON.
> > >
> > > Although software running under CAP_PERFMON can not ensure avoidance of
> > > related hardware issues, the software can still mitigate those issues
> > > following the official hardware issues mitigation procedure [3]. The
> > > bugs in the software itself can be fixed following the standard kernel
> > > development process [4] to maintain and harden security of system
> > > performance monitoring and observability operations. Finally, the patch
> > > set is shaped in the way that simplifies backtracking procedure of
> > > possible induced issues [5] as much as possible.
> > >
> > > The patch set is for tip perf/core repository:
> > > git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip perf/core
> > > sha1: fdb64822443ec9fb8c3a74b598a74790ae8d2e22
> > >
> > > ---
> > > Changes in v7:
> > > - updated and extended kernel.rst and perf-security.rst documentation
> > > files with the information about CAP_PERFMON capability and its use cases
> > > - documented the case of double audit logging of CAP_PERFMON and CAP_SYS_ADMIN
> > > capabilities on a SELinux enabled system
> > > Changes in v6:
> > > - avoided noaudit checks in perfmon_capable() to explicitly advertise
> > > CAP_PERFMON usage thru audit logs to secure system performance
> > > monitoring and observability
> > > Changes in v5:
> > > - renamed CAP_SYS_PERFMON to CAP_PERFMON
> > > - extended perfmon_capable() with noaudit checks
> > > Changes in v4:
> > > - converted perfmon_capable() into an inline function
> > > - made perf_events kprobes, uprobes, hw breakpoints and namespaces data
> > > available to CAP_SYS_PERFMON privileged processes
> > > - applied perfmon_capable() to drivers/perf and drivers/oprofile
> > > - extended __cmd_ftrace() with support of CAP_SYS_PERFMON
> > > Changes in v3:
> > > - implemented perfmon_capable() macros aggregating required capabilities
> > > checks
> > > Changes in v2:
> > > - made perf_events trace points available to CAP_SYS_PERFMON privileged
> > > processes
> > > - made perf_event_paranoid_check() treat CAP_SYS_PERFMON equally to
> > > CAP_SYS_ADMIN
> > > - applied CAP_SYS_PERFMON to i915_perf, bpf_trace, powerpc and parisc
> > > system performance monitoring and observability related subsystems
> > >
> > > ---
> > > Alexey Budankov (12):
> > > capabilities: introduce CAP_PERFMON to kernel and user space
> > > perf/core: open access to the core for CAP_PERFMON privileged process
> > > perf/core: open access to probes for CAP_PERFMON privileged process
> > > perf tool: extend Perf tool with CAP_PERFMON capability support
> > > drm/i915/perf: open access for CAP_PERFMON privileged process
> > > trace/bpf_trace: open access for CAP_PERFMON privileged process
> > > powerpc/perf: open access for CAP_PERFMON privileged process
> > > parisc/perf: open access for CAP_PERFMON privileged process
> > > drivers/perf: open access for CAP_PERFMON privileged process
> > > drivers/oprofile: open access for CAP_PERFMON privileged process
> > > doc/admin-guide: update perf-security.rst with CAP_PERFMON information
> > > doc/admin-guide: update kernel.rst with CAP_PERFMON information
> > >
> > > Documentation/admin-guide/perf-security.rst | 65 +++++++++++++--------
> > > Documentation/admin-guide/sysctl/kernel.rst | 16 +++--
> > > arch/parisc/kernel/perf.c | 2 +-
> > > arch/powerpc/perf/imc-pmu.c | 4 +-
> > > drivers/gpu/drm/i915/i915_perf.c | 13 ++---
> > > drivers/oprofile/event_buffer.c | 2 +-
> > > drivers/perf/arm_spe_pmu.c | 4 +-
> > > include/linux/capability.h | 4 ++
> > > include/linux/perf_event.h | 6 +-
> > > include/uapi/linux/capability.h | 8 ++-
> > > kernel/events/core.c | 6 +-
> > > kernel/trace/bpf_trace.c | 2 +-
> > > security/selinux/include/classmap.h | 4 +-
> > > tools/perf/builtin-ftrace.c | 5 +-
> > > tools/perf/design.txt | 3 +-
> > > tools/perf/util/cap.h | 4 ++
> > > tools/perf/util/evsel.c | 10 ++--
> > > tools/perf/util/util.c | 1 +
> > > 18 files changed, 98 insertions(+), 61 deletions(-)
> > >
> > > ---
> > > Validation (Intel Skylake, 8 cores, Fedora 29, 5.5.0-rc3+, x86_64):
> > >
> > > libcap library [6], [7], [8] and Perf tool can be used to apply
> > > CAP_PERFMON capability for secure system performance monitoring and
> > > observability beyond the scope permitted by the system wide
> > > perf_event_paranoid kernel setting [9] and below are the steps for
> > > evaluation:
> > >
> > > - patch, build and boot the kernel
> > > - patch, build Perf tool e.g. to /home/user/perf
> > > ...
> > > # git clone git://git.kernel.org/pub/scm/libs/libcap/libcap.git libcap
> > > # pushd libcap
> > > # patch libcap/include/uapi/linux/capabilities.h with [PATCH 1]
> > > # make
> > > # pushd progs
> > > # ./setcap "cap_perfmon,cap_sys_ptrace,cap_syslog=ep" /home/user/perf
> > > # ./setcap -v "cap_perfmon,cap_sys_ptrace,cap_syslog=ep" /home/user/perf
> > > /home/user/perf: OK
> > > # ./getcap /home/user/perf
> > > /home/user/perf = cap_sys_ptrace,cap_syslog,cap_perfmon+ep
> > > # echo 2 > /proc/sys/kernel/perf_event_paranoid
> > > # cat /proc/sys/kernel/perf_event_paranoid
> > > 2
> > > ...
> > > $ /home/user/perf top
> > > ... works as expected ...
> > > $ cat /proc/`pidof perf`/status
> > > Name: perf
> > > Umask: 0002
> > > State: S (sleeping)
> > > Tgid: 2958
> > > Ngid: 0
> > > Pid: 2958
> > > PPid: 9847
> > > TracerPid: 0
> > > Uid: 500 500 500 500
> > > Gid: 500 500 500 500
> > > FDSize: 256
> > > ...
> > > CapInh: 0000000000000000
> > > CapPrm: 0000004400080000
> > > CapEff: 0000004400080000 => 01000100 00000000 00001000 00000000 00000000
> > > cap_perfmon,cap_sys_ptrace,cap_syslog
> > > CapBnd: 0000007fffffffff
> > > CapAmb: 0000000000000000
> > > NoNewPrivs: 0
> > > Seccomp: 0
> > > Speculation_Store_Bypass: thread vulnerable
> > > Cpus_allowed: ff
> > > Cpus_allowed_list: 0-7
> > > ...
> > >
> > > Usage of cap_perfmon effectively avoids unused credentials excess:
> > >
> > > - with cap_sys_admin:
> > > CapEff: 0000007fffffffff => 01111111 11111111 11111111 11111111 11111111
> > >
> > > - with cap_perfmon:
> > > CapEff: 0000004400080000 => 01000100 00000000 00001000 00000000 00000000
> > > 38 34 19
> > > perfmon syslog sys_ptrace
> > >
> > > ---
> > > [1] https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html
> > > [2] http://man7.org/linux/man-pages/man7/capabilities.7.html
> > > [3] https://www.kernel.org/doc/html/latest/process/embargoed-hardware-issues.html
> > > [4] https://www.kernel.org/doc/html/latest/admin-guide/security-bugs.html
> > > [5] https://www.kernel.org/doc/html/latest/process/management-style.html#decisions
> > > [6] http://man7.org/linux/man-pages/man8/setcap.8.html
> > > [7] https://git.kernel.org/pub/scm/libs/libcap/libcap.git
> > > [8] https://sites.google.com/site/fullycapable/, posix_1003.1e-990310.pdf
> > > [9] http://man7.org/linux/man-pages/man2/perf_event_open.2.html
> > >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH V14] mm/debug: Add tests validating architecture page table helpers
From: Qian Cai @ 2020-03-02 19:40 UTC (permalink / raw)
To: Christophe Leroy, Anshuman Khandual, linux-mm
Cc: Catalin Marinas, Heiko Carstens, Paul Mackerras, H. Peter Anvin,
linux-riscv, Will Deacon, linux-arch, linux-s390, x86,
Mike Rapoport, Christian Borntraeger, Ingo Molnar, linux-snps-arc,
Vasily Gorbik, Borislav Petkov, Paul Walmsley,
Kirill A . Shutemov, Thomas Gleixner, linux-arm-kernel,
Vineet Gupta, linux-kernel, Palmer Dabbelt, Andrew Morton,
linuxppc-dev
In-Reply-To: <1582732318.7365.129.camel@lca.pw>
On Wed, 2020-02-26 at 10:51 -0500, Qian Cai wrote:
> On Wed, 2020-02-26 at 15:45 +0100, Christophe Leroy wrote:
> >
> > Le 26/02/2020 à 15:09, Qian Cai a écrit :
> > > On Mon, 2020-02-17 at 08:47 +0530, Anshuman Khandual wrote:
> > > > This adds tests which will validate architecture page table helpers and
> > > > other accessors in their compliance with expected generic MM semantics.
> > > > This will help various architectures in validating changes to existing
> > > > page table helpers or addition of new ones.
> > > >
> > > > This test covers basic page table entry transformations including but not
> > > > limited to old, young, dirty, clean, write, write protect etc at various
> > > > level along with populating intermediate entries with next page table page
> > > > and validating them.
> > > >
> > > > Test page table pages are allocated from system memory with required size
> > > > and alignments. The mapped pfns at page table levels are derived from a
> > > > real pfn representing a valid kernel text symbol. This test gets called
> > > > inside kernel_init() right after async_synchronize_full().
> > > >
> > > > This test gets built and run when CONFIG_DEBUG_VM_PGTABLE is selected. Any
> > > > architecture, which is willing to subscribe this test will need to select
> > > > ARCH_HAS_DEBUG_VM_PGTABLE. For now this is limited to arc, arm64, x86, s390
> > > > and ppc32 platforms where the test is known to build and run successfully.
> > > > Going forward, other architectures too can subscribe the test after fixing
> > > > any build or runtime problems with their page table helpers. Meanwhile for
> > > > better platform coverage, the test can also be enabled with CONFIG_EXPERT
> > > > even without ARCH_HAS_DEBUG_VM_PGTABLE.
> > > >
> > > > Folks interested in making sure that a given platform's page table helpers
> > > > conform to expected generic MM semantics should enable the above config
> > > > which will just trigger this test during boot. Any non conformity here will
> > > > be reported as an warning which would need to be fixed. This test will help
> > > > catch any changes to the agreed upon semantics expected from generic MM and
> > > > enable platforms to accommodate it thereafter.
> > >
> > > How useful is this that straightly crash the powerpc?
> > >
> > > [ 23.263425][ T1] debug_vm_pgtable: debug_vm_pgtable: Validating
> > > architecture page table helpers
> > > [ 23.263625][ T1] ------------[ cut here ]------------
> > > [ 23.263649][ T1] kernel BUG at arch/powerpc/mm/pgtable.c:274!
> >
> > The problem on PPC64 is known and has to be investigated and fixed.
>
> It might be interesting to hear what powerpc64 maintainers would say about it
> and if it is actually worth "fixing" in the arch code, but that BUG_ON() was
> there since 2009 and had not been exposed until this patch comes alone?
This patch below makes it works on powerpc64 in order to dodge the BUG_ON()s in
assert_pte_locked() triggered by pte_clear_tests().
diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
index 96dd7d574cef..50b385233971 100644
--- a/mm/debug_vm_pgtable.c
+++ b/mm/debug_vm_pgtable.c
@@ -55,6 +55,8 @@
#define RANDOM_ORVALUE GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS)
#define RANDOM_NZVALUE GENMASK(7, 0)
+unsigned long vaddr;
+
static void __init pte_basic_tests(unsigned long pfn, pgprot_t prot)
{
pte_t pte = pfn_pte(pfn, prot);
@@ -256,7 +258,7 @@ static void __init pte_clear_tests(struct mm_struct *mm,
pte_t *ptep)
pte = __pte(pte_val(pte) | RANDOM_ORVALUE);
WRITE_ONCE(*ptep, pte);
- pte_clear(mm, 0, ptep);
+ pte_clear(mm, vaddr, ptep);
pte = READ_ONCE(*ptep);
WARN_ON(!pte_none(pte));
}
@@ -310,8 +312,9 @@ void __init debug_vm_pgtable(void)
pgtable_t saved_ptep;
pgprot_t prot;
phys_addr_t paddr;
- unsigned long vaddr, pte_aligned, pmd_aligned;
+ unsigned long pte_aligned, pmd_aligned;
unsigned long pud_aligned, p4d_aligned, pgd_aligned;
+ spinlock_t *ptl;
pr_info("Validating architecture page table helpers\n");
prot = vm_get_page_prot(VMFLAGS);
@@ -344,7 +347,7 @@ void __init debug_vm_pgtable(void)
p4dp = p4d_alloc(mm, pgdp, vaddr);
pudp = pud_alloc(mm, p4dp, vaddr);
pmdp = pmd_alloc(mm, pudp, vaddr);
- ptep = pte_alloc_map(mm, pmdp, vaddr);
+ ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
/*
* Save all the page table page addresses as the page table
@@ -370,7 +373,7 @@ void __init debug_vm_pgtable(void)
p4d_clear_tests(mm, p4dp);
pgd_clear_tests(mm, pgdp);
- pte_unmap(ptep);
+ pte_unmap_unlock(ptep, ptl);
pmd_populate_tests(mm, pmdp, saved_ptep);
pud_populate_tests(mm, pudp, saved_pmdp);
^ permalink raw reply related
* Re: [PATCH v3 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"
From: Jiri Olsa @ 2020-03-02 15:08 UTC (permalink / raw)
To: Kajol Jain
Cc: mark.rutland, maddy, peterz, yao.jin, mingo, kan.liang, ak,
alexander.shishkin, anju, mamatha4, sukadev, ravi.bangoria, acme,
jmario, namhyung, tglx, mpetlan, gregkh, linux-kernel,
linux-perf-users, jolsa, linuxppc-dev
In-Reply-To: <20200229094159.25573-7-kjain@linux.ibm.com>
On Sat, Feb 29, 2020 at 03:11:57PM +0530, Kajol Jain wrote:
SNIP
> #define PVR_VER(pvr) (((pvr) >> 16) & 0xFFFF) /* Version field */
> #define PVR_REV(pvr) (((pvr) >> 0) & 0xFFFF) /* Revison field */
>
> +#define SOCKETS_INFO_FILE_PATH "/devices/hv_24x7/interface/"
> +
> int
> get_cpuid(char *buffer, size_t sz)
> {
> @@ -44,3 +51,43 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
>
> return bufp;
> }
> +
> +int arch_get_runtimeparam(void)
> +{
> + int count = 0;
> + DIR *dir;
> + char path[PATH_MAX];
> + const char *sysfs = sysfs__mountpoint();
> + char filename[] = "sockets";
> + FILE *file;
> + char buf[16], *num;
> + int data;
> +
> + if (!sysfs)
> + goto out;
> +
> + snprintf(path, PATH_MAX,
> + "%s" SOCKETS_INFO_FILE_PATH, sysfs);
> + dir = opendir(path);
> +
> + if (!dir)
> + goto out;
> +
> + strcat(path, filename);
> + file = fopen(path, "r");
> +
> + if (!file)
> + goto out;
> +
> + data = fread(buf, 1, sizeof(buf), file);
> +
> + if (data == 0)
> + goto out;
> +
> + count = strtol(buf, &num, 10);
> +out:
> + if (!count)
> + count = 1;
> +
> + return count;
we have sysfs__read_ull for this
jirka
^ permalink raw reply
* Re: [PATCH v3 6/7] mm/memory_hotplug: Add pgprot_t to mhp_params
From: Logan Gunthorpe @ 2020-03-02 18:55 UTC (permalink / raw)
To: Dan Williams
Cc: Michal Hocko, linux-ia64, Linux-sh, Peter Zijlstra,
Catalin Marinas, Dave Hansen, platform-driver-x86, Linux MM,
Will Deacon, Christoph Hellwig, linux-s390, David Hildenbrand,
Ingo Molnar, Borislav Petkov, Andy Lutomirski, Thomas Gleixner,
Michal Hocko, Linux ARM, Eric Badger, Linux Kernel Mailing List,
Andrew Morton, linuxppc-dev
In-Reply-To: <CAPcyv4gR1+NaWzteqNKip=cYk89oEVW18HNao7Xv=JipzzDagw@mail.gmail.com>
On 2020-02-29 3:44 p.m., Dan Williams wrote:
> On Fri, Feb 21, 2020 at 10:25 AM Logan Gunthorpe <logang@deltatee.com> wrote:
>>
>> devm_memremap_pages() is currently used by the PCI P2PDMA code to create
>> struct page mappings for IO memory. At present, these mappings are created
>> with PAGE_KERNEL which implies setting the PAT bits to be WB. However, on
>> x86, an mtrr register will typically override this and force the cache
>> type to be UC-. In the case firmware doesn't set this register it is
>> effectively WB and will typically result in a machine check exception
>> when it's accessed.
>>
>> Other arches are not currently likely to function correctly seeing they
>> don't have any MTRR registers to fall back on.
>>
>> To solve this, provide a way to specify the pgprot value explicitly to
>> arch_add_memory().
>>
>> Of the arches that support MEMORY_HOTPLUG: x86_64, and arm64 need a simple
>> change to pass the pgprot_t down to their respective functions which set
>> up the page tables. For x86_32, set the page tables explicitly using
>> _set_memory_prot() (seeing they are already mapped). For ia64, s390 and
>> sh, reject anything but PAGE_KERNEL settings -- this should be fine,
>> for now, seeing these architectures don't support ZONE_DEVICE.
>>
>> A check in __add_pages() is also added to ensure the pgprot parameter was
>> set for all arches.
>>
>> Cc: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
>> Acked-by: David Hildenbrand <david@redhat.com>
>> Acked-by: Michal Hocko <mhocko@suse.com>
>> ---
>> arch/arm64/mm/mmu.c | 3 ++-
>> arch/ia64/mm/init.c | 3 +++
>> arch/powerpc/mm/mem.c | 3 ++-
>> arch/s390/mm/init.c | 3 +++
>> arch/sh/mm/init.c | 3 +++
>> arch/x86/mm/init_32.c | 5 +++++
>> arch/x86/mm/init_64.c | 2 +-
>> include/linux/memory_hotplug.h | 2 ++
>> mm/memory_hotplug.c | 5 ++++-
>> mm/memremap.c | 6 +++---
>> 10 files changed, 28 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index ee37bca8aba8..ea3fa844a8a2 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -1058,7 +1058,8 @@ int arch_add_memory(int nid, u64 start, u64 size,
>> flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>>
>> __create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
>> - size, PAGE_KERNEL, __pgd_pgtable_alloc, flags);
>> + size, params->pgprot, __pgd_pgtable_alloc,
>> + flags);
>>
>> memblock_clear_nomap(start, size);
>>
>> diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
>> index 97bbc23ea1e3..d637b4ea3147 100644
>> --- a/arch/ia64/mm/init.c
>> +++ b/arch/ia64/mm/init.c
>> @@ -676,6 +676,9 @@ int arch_add_memory(int nid, u64 start, u64 size,
>> unsigned long nr_pages = size >> PAGE_SHIFT;
>> int ret;
>>
>> + if (WARN_ON_ONCE(params->pgprot.pgprot != PAGE_KERNEL.pgprot))
>> + return -EINVAL;
>> +
>> ret = __add_pages(nid, start_pfn, nr_pages, params);
>> if (ret)
>> printk("%s: Problem encountered in __add_pages() as ret=%d\n",
>> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
>> index 19b1da5d7eca..832412bc7fad 100644
>> --- a/arch/powerpc/mm/mem.c
>> +++ b/arch/powerpc/mm/mem.c
>> @@ -138,7 +138,8 @@ int __ref arch_add_memory(int nid, u64 start, u64 size,
>> resize_hpt_for_hotplug(memblock_phys_mem_size());
>>
>> start = (unsigned long)__va(start);
>> - rc = create_section_mapping(start, start + size, nid, PAGE_KERNEL);
>> + rc = create_section_mapping(start, start + size, nid,
>> + params->pgprot);
>> if (rc) {
>> pr_warn("Unable to create mapping for hot added memory 0x%llx..0x%llx: %d\n",
>> start, start + size, rc);
>> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
>> index e9e4a7abd0cc..87b2d024e75a 100644
>> --- a/arch/s390/mm/init.c
>> +++ b/arch/s390/mm/init.c
>> @@ -277,6 +277,9 @@ int arch_add_memory(int nid, u64 start, u64 size,
>> if (WARN_ON_ONCE(params->altmap))
>> return -EINVAL;
>>
>> + if (WARN_ON_ONCE(params->pgprot.pgprot != PAGE_KERNEL.pgprot))
>> + return -EINVAL;
>> +
>> rc = vmem_add_mapping(start, size);
>> if (rc)
>> return rc;
>> diff --git a/arch/sh/mm/init.c b/arch/sh/mm/init.c
>> index e5114c053364..b9de2d4fa57e 100644
>> --- a/arch/sh/mm/init.c
>> +++ b/arch/sh/mm/init.c
>> @@ -412,6 +412,9 @@ int arch_add_memory(int nid, u64 start, u64 size,
>> unsigned long nr_pages = size >> PAGE_SHIFT;
>> int ret;
>>
>> + if (WARN_ON_ONCE(params->pgprot.pgprot != PAGE_KERNEL.pgprot)
>> + return -EINVAL;
>> +
>> /* We only have ZONE_NORMAL, so this is easy.. */
>> ret = __add_pages(nid, start_pfn, nr_pages, params);
>> if (unlikely(ret))
>> diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
>> index e25a4218e6ff..96d8e4fb1cc8 100644
>> --- a/arch/x86/mm/init_32.c
>> +++ b/arch/x86/mm/init_32.c
>> @@ -858,6 +858,11 @@ int arch_add_memory(int nid, u64 start, u64 size,
>> {
>> unsigned long start_pfn = start >> PAGE_SHIFT;
>> unsigned long nr_pages = size >> PAGE_SHIFT;
>> + int ret;
>> +
>> + ret = _set_memory_prot(start, nr_pages, params->pgprot);
>
> Perhaps a comment since it's not immediately obvious where the
> PAGE_KERNEL prot was established, and perhaps add a conditional to
> skip this call in the param->pgprot == PAGE_KERNEL case?
Yes I can add the skip in the PAGE_KERNEL case. Though I'm not sure what
you are asking for with regards to the comment. Just that pgprot is set
by the caller usually to PAGE_KERNEL?
> Other than that looks good to me, but only an ack since I'm only
> testing the x86 changes.
>
> Acked-by: Dan Williams <dan.j.williams@intel.com>
Thanks,
Logan
^ permalink raw reply
* Re: [PATCH v3 4/7] x86/mm: Introduce _set_memory_prot()
From: Logan Gunthorpe @ 2020-03-02 18:46 UTC (permalink / raw)
To: Dan Williams
Cc: X86 ML, linux-ia64, Linux-sh, Peter Zijlstra, Catalin Marinas,
Dave Hansen, platform-driver-x86, Linux MM, H. Peter Anvin,
Will Deacon, Christoph Hellwig, linux-s390, David Hildenbrand,
Ingo Molnar, Borislav Petkov, Andy Lutomirski, Thomas Gleixner,
Michal Hocko, Linux ARM, Eric Badger, Linux Kernel Mailing List,
Andrew Morton, linuxppc-dev
In-Reply-To: <CAPcyv4j=bZ5KBPp6PbViERdDe+HZpV_W6qbSJupTNAzyfiK6xg@mail.gmail.com>
On 2020-02-29 3:33 p.m., Dan Williams wrote:
> On Fri, Feb 21, 2020 at 10:25 AM Logan Gunthorpe <logang@deltatee.com> wrote:
>>
>> For use in the 32bit arch_add_memory() to set the pgprot type of the
>> memory to add.
>>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Borislav Petkov <bp@alien8.de>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: x86@kernel.org
>> Cc: Dave Hansen <dave.hansen@linux.intel.com>
>> Cc: Andy Lutomirski <luto@kernel.org>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
>> ---
>> arch/x86/include/asm/set_memory.h | 1 +
>> arch/x86/mm/pat/set_memory.c | 7 +++++++
>> 2 files changed, 8 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/set_memory.h b/arch/x86/include/asm/set_memory.h
>> index 64c3dce374e5..0aca959cf9a4 100644
>> --- a/arch/x86/include/asm/set_memory.h
>> +++ b/arch/x86/include/asm/set_memory.h
>> @@ -34,6 +34,7 @@
>> * The caller is required to take care of these.
>> */
>>
>> +int _set_memory_prot(unsigned long addr, int numpages, pgprot_t prot);
>
> I wonder if this should be separated from the naming convention of the
> other routines because this is only an internal helper for code paths
> where the prot was established by an upper layer. For example, I
> expect that the kernel does not want new usages to make the mistake of
> calling:
>
> _set_memory_prot(..., pgprot_writecombine(pgprot))
>
> ...instead of
>
> _set_memory_wc()
>
> I'm thinking just a double underscore rename (__set_memory_prot) and a
> kerneldoc comment for that pointing people to use the direct
> _set_memory_<cachemode> helpers.
Thanks! Will do. Note, though, that even _set_memory_wc() is an internal
x86-specific function. But the extra comment and underscore still make
sense.
> With that you can add:
>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
>
^ permalink raw reply
* Re: [PATCH v3 3/5] libnvdimm/namespace: Enforce memremap_compat_align()
From: Dan Williams @ 2020-03-02 18:45 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: linuxppc-dev, Linux Kernel Mailing List, linux-nvdimm
In-Reply-To: <87fterrmau.fsf@linux.ibm.com>
On Mon, Mar 2, 2020 at 4:09 AM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> Dan Williams <dan.j.williams@intel.com> writes:
>
> > The pmem driver on PowerPC crashes with the following signature when
> > instantiating misaligned namespaces that map their capacity via
> > memremap_pages().
> >
> > BUG: Unable to handle kernel data access at 0xc001000406000000
> > Faulting instruction address: 0xc000000000090790
> > NIP [c000000000090790] arch_add_memory+0xc0/0x130
> > LR [c000000000090744] arch_add_memory+0x74/0x130
> > Call Trace:
> > arch_add_memory+0x74/0x130 (unreliable)
> > memremap_pages+0x74c/0xa30
> > devm_memremap_pages+0x3c/0xa0
> > pmem_attach_disk+0x188/0x770
> > nvdimm_bus_probe+0xd8/0x470
> >
> > With the assumption that only memremap_pages() has alignment
> > constraints, enforce memremap_compat_align() for
> > pmem_should_map_pages(), nd_pfn, and nd_dax cases. This includes
> > preventing the creation of namespaces where the base address is
> > misaligned and cases there infoblock padding parameters are invalid.
> >
>
> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>
> > Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> > Cc: Jeff Moyer <jmoyer@redhat.com>
> > Fixes: a3619190d62e ("libnvdimm/pfn: stop padding pmem namespaces to section alignment")
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> > drivers/nvdimm/namespace_devs.c | 12 ++++++++++++
> > drivers/nvdimm/pfn_devs.c | 26 +++++++++++++++++++++++---
> > 2 files changed, 35 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> > index 032dc61725ff..68e89855f779 100644
> > --- a/drivers/nvdimm/namespace_devs.c
> > +++ b/drivers/nvdimm/namespace_devs.c
> > @@ -10,6 +10,7 @@
> > #include <linux/nd.h>
> > #include "nd-core.h"
> > #include "pmem.h"
> > +#include "pfn.h"
> > #include "nd.h"
> >
> > static void namespace_io_release(struct device *dev)
> > @@ -1739,6 +1740,17 @@ struct nd_namespace_common *nvdimm_namespace_common_probe(struct device *dev)
> > return ERR_PTR(-ENODEV);
> > }
>
> May be add a comment here that both dax/fsdax namespace details are
> checked in nd_pfn_validate() so that we look at start_pad and end_trunc
> while validating the namespace?
>
> >
> > + if (pmem_should_map_pages(dev)) {
> > + struct nd_namespace_io *nsio = to_nd_namespace_io(&ndns->dev);
> > + struct resource *res = &nsio->res;
> > +
> > + if (!IS_ALIGNED(res->start | (res->end + 1),
> > + memremap_compat_align())) {
> > + dev_err(&ndns->dev, "%pr misaligned, unable to map\n", res);
> > + return ERR_PTR(-EOPNOTSUPP);
> > + }
> > + }
> > +
> > if (is_namespace_pmem(&ndns->dev)) {
> > struct nd_namespace_pmem *nspm;
> >
> > diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
> > index 79fe02d6f657..3bdd4b883d05 100644
> > --- a/drivers/nvdimm/pfn_devs.c
> > +++ b/drivers/nvdimm/pfn_devs.c
> > @@ -446,6 +446,7 @@ static bool nd_supported_alignment(unsigned long align)
> > int nd_pfn_validate(struct nd_pfn *nd_pfn, const char *sig)
> > {
> > u64 checksum, offset;
> > + struct resource *res;
> > enum nd_pfn_mode mode;
> > struct nd_namespace_io *nsio;
> > unsigned long align, start_pad;
> > @@ -578,13 +579,14 @@ int nd_pfn_validate(struct nd_pfn *nd_pfn, const char *sig)
> > * established.
> > */
> > nsio = to_nd_namespace_io(&ndns->dev);
> > - if (offset >= resource_size(&nsio->res)) {
> > + res = &nsio->res;
> > + if (offset >= resource_size(res)) {
> > dev_err(&nd_pfn->dev, "pfn array size exceeds capacity of %s\n",
> > dev_name(&ndns->dev));
> > return -EOPNOTSUPP;
> > }
> >
> > - if ((align && !IS_ALIGNED(nsio->res.start + offset + start_pad, align))
> > + if ((align && !IS_ALIGNED(res->start + offset + start_pad, align))
> > || !IS_ALIGNED(offset, PAGE_SIZE)) {
> > dev_err(&nd_pfn->dev,
> > "bad offset: %#llx dax disabled align: %#lx\n",
> > @@ -592,6 +594,18 @@ int nd_pfn_validate(struct nd_pfn *nd_pfn, const char *sig)
> > return -EOPNOTSUPP;
> > }
> >
> > + if (!IS_ALIGNED(res->start + le32_to_cpu(pfn_sb->start_pad),
> > + memremap_compat_align())) {
> > + dev_err(&nd_pfn->dev, "resource start misaligned\n");
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + if (!IS_ALIGNED(res->end + 1 - le32_to_cpu(pfn_sb->end_trunc),
> > + memremap_compat_align())) {
> > + dev_err(&nd_pfn->dev, "resource end misaligned\n");
> > + return -EOPNOTSUPP;
> > + }
> > +
> > return 0;
> > }
> > EXPORT_SYMBOL(nd_pfn_validate);
> > @@ -750,7 +764,13 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn)
> > start = nsio->res.start;
> > size = resource_size(&nsio->res);
> > npfns = PHYS_PFN(size - SZ_8K);
> > - align = max(nd_pfn->align, SUBSECTION_SIZE);
> > + align = max(nd_pfn->align, memremap_compat_align());
> > + if (!IS_ALIGNED(start, memremap_compat_align())) {
> > + dev_err(&nd_pfn->dev, "%s: start %pa misaligned to %#lx\n",
> > + dev_name(&ndns->dev), &start,
> > + memremap_compat_align());
> > + return -EINVAL;
> > + }
>
> This validates start in case of a new namespace creation where the user
> updated nd_region->align value? A comment there would help when looking
> at the code later?
Yeah, sounds good will respin with those updates.
^ permalink raw reply
* Re: [PATCH v3 15/27] powerpc/powernv/pmem: Add support for near storage commands
From: Dan Williams @ 2020-03-02 18:42 UTC (permalink / raw)
To: Frederic Barrat
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Mahesh Salgaonkar,
Masahiro Yamada, Oliver O'Halloran, Mauro Carvalho Chehab,
Ira Weiny, Rob Herring, Dave Jiang, linux-nvdimm,
Aneesh Kumar K . V, Krzysztof Kozlowski, Anju T Sudhakar,
alastair, Andrew Donnellan, Arnd Bergmann, Greg Kurz,
Nicholas Piggin, Cédric Le Goater, Thomas Gleixner,
Hari Bathini, Alastair D'Silva, Linux MM, Greg Kroah-Hartman,
Linux Kernel Mailing List, Vishal Verma, Paul Mackerras,
Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <9e40ad40-6fa8-0fd2-a53a-8a3029a3639c@linux.ibm.com>
On Mon, Mar 2, 2020 at 9:59 AM Frederic Barrat <fbarrat@linux.ibm.com> wrote:
>
>
>
> Le 21/02/2020 à 04:27, Alastair D'Silva a écrit :
> > 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>
> > ---
>
>
> Is any of these new functions ever called?
This is my concern as well. The libnvdimm command support is limited
to the commands that Linux will use. Other passthrough commands are
supported through a passthrough interface. However, that passthrough
interface is explicitly limited to publicly documented command sets so
that the kernel has an opportunity to constrain and consolidate
command implementations across vendors.
^ permalink raw reply
* Re: [PATCH v3 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"
From: Jiri Olsa @ 2020-03-02 15:08 UTC (permalink / raw)
To: Kajol Jain
Cc: mark.rutland, maddy, peterz, yao.jin, mingo, kan.liang, ak,
alexander.shishkin, anju, mamatha4, sukadev, ravi.bangoria, acme,
jmario, namhyung, tglx, mpetlan, gregkh, linux-kernel,
linux-perf-users, jolsa, linuxppc-dev
In-Reply-To: <20200229094159.25573-7-kjain@linux.ibm.com>
On Sat, Feb 29, 2020 at 03:11:57PM +0530, Kajol Jain wrote:
SNIP
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 02aee946b6c1..f629828cc0de 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -399,6 +399,11 @@ void metricgroup__print(bool metrics, bool metricgroups, char *filter,
> strlist__delete(metriclist);
> }
>
> +int __weak arch_get_runtimeparam(void)
> +{
> + return 1;
> +}
> +
> static int metricgroup__add_metric(const char *metric, struct strbuf *events,
> struct list_head *group_list)
> {
> @@ -419,52 +424,77 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
> continue;
> if (match_metric(pe->metric_group, metric) ||
> match_metric(pe->metric_name, metric)) {
> - const char **ids;
> - int idnum;
> - struct egroup *eg;
> - bool no_group = false;
> + int k, count;
two things in here.. there's already ack-ed patchset from Kan Liang:
Support metric group constraint
>[PATCH V2 2/5] perf metricgroup: Factor out metricgroup__add_metric_weak_group()
that's changing this place, so you might want to synchronize with that
> +
> + if (strstr(pe->metric_expr, "?"))
> + count = arch_get_runtimeparam();
> + else
> + count = 1;
> +
> + /* This loop is added to create multiple
> + * events depend on count value and add
> + * those events to group_list.
> + */
> + for (k = 0; k < count; k++) {
> + const char **ids;
> + int idnum;
> + struct egroup *eg;
> + bool no_group = false;
> + char value[PATH_MAX];
> +
> + pr_debug("metric expr %s for %s\n",
> + pe->metric_expr, pe->metric_name);
> + expr__runtimeparam = k;
the other thing is that I don't really follow what's going on in here
you're setting expr__runtimeparam to the loop index,
which you get from some arch related file
we should do this in arch-specific way.. I think that Kan's change is
already moving some bits into separate function and that should make
all this more readable, but perhaps we might need more, so all the
'repeating' code will be in a function
please either separate this to arch code, or make it understandable
for people from other archs ;-)
jirka
> + if (expr__find_other(pe->metric_expr, NULL,
> + &ids, &idnum) < 0)
> + continue;
> + if (events->len > 0)
> + strbuf_addf(events, ",");
> + for (j = 0; j < idnum; j++) {
> + pr_debug("found event %s\n", ids[j]);
> + /*
> + * Duration time maps to a software
> + * event and can make groups not count.
> + * Always use it outside a group.
> + */
> + if (!strcmp(ids[j], "duration_time")) {
> + if (j > 0)
> + strbuf_addf(events,
> + "}:W,");
> + strbuf_addf(events,
> + "duration_time");
> + no_group = true;
> + continue;
> + }
> + strbuf_addf(events, "%s%s",
> + j == 0 || no_group ? "{" :
> + ",", ids[j]);
> + no_group = false;
> + }
> + if (!no_group)
> + strbuf_addf(events, "}:W");
>
> - pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
> + eg = malloc(sizeof(struct egroup));
> + if (!eg) {
> + ret = -ENOMEM;
> + break;
> + }
> + eg->ids = ids;
> + eg->idnum = idnum;
>
> - if (expr__find_other(pe->metric_expr,
> - NULL, &ids, &idnum) < 0)
> - continue;
> - if (events->len > 0)
> - strbuf_addf(events, ",");
> - for (j = 0; j < idnum; j++) {
> - pr_debug("found event %s\n", ids[j]);
> - /*
> - * Duration time maps to a software event and can make
> - * groups not count. Always use it outside a
> - * group.
> - */
> - if (!strcmp(ids[j], "duration_time")) {
> - if (j > 0)
> - strbuf_addf(events, "}:W,");
> - strbuf_addf(events, "duration_time");
> - no_group = true;
> - continue;
> + if (strstr(pe->metric_expr, "?")) {
> + sprintf(value, "%s%c%d",
> + pe->metric_name, '_', k);
> + } else {
> + strcpy(value, pe->metric_name);
> }
> - strbuf_addf(events, "%s%s",
> - j == 0 || no_group ? "{" : ",",
> - ids[j]);
> - no_group = false;
> - }
> - if (!no_group)
> - strbuf_addf(events, "}:W");
>
> - eg = malloc(sizeof(struct egroup));
> - if (!eg) {
> - ret = -ENOMEM;
> - break;
> + eg->metric_name = strdup(value);
> + eg->metric_expr = pe->metric_expr;
> + eg->metric_unit = pe->unit;
> + list_add_tail(&eg->nd, group_list);
> + ret = 0;
> }
> - eg->ids = ids;
> - eg->idnum = idnum;
> - eg->metric_name = pe->metric_name;
> - eg->metric_expr = pe->metric_expr;
> - eg->metric_unit = pe->unit;
> - list_add_tail(&eg->nd, group_list);
> - ret = 0;
> }
> }
> return ret;
SNIP
^ permalink raw reply
* Re: [PATCH v3 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"
From: Jiri Olsa @ 2020-03-02 15:09 UTC (permalink / raw)
To: Kajol Jain
Cc: mark.rutland, maddy, peterz, yao.jin, mingo, kan.liang, ak,
alexander.shishkin, anju, mamatha4, sukadev, ravi.bangoria, acme,
jmario, namhyung, tglx, mpetlan, gregkh, linux-kernel,
linux-perf-users, jolsa, linuxppc-dev
In-Reply-To: <20200229094159.25573-7-kjain@linux.ibm.com>
On Sat, Feb 29, 2020 at 03:11:57PM +0530, Kajol Jain wrote:
SNIP
> + *dst++ = paramval[i++];
> + free(paramval);
> + }
> + }
> else
> *dst++ = *str;
> str++;
> @@ -72,8 +86,8 @@ number [0-9]+
>
> sch [-,=]
> spec \\{sch}
> -sym [0-9a-zA-Z_\.:@]+
> -symbol {spec}*{sym}*{spec}*{sym}*
> +sym [0-9a-zA-Z_\.:@?]+
> +symbol {spec}*{sym}*{spec}*{sym}*{spec}*{sym}
>
> %%
> {
> diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
> index 4720cbe79357..0f3ef0f37bf4 100644
> --- a/tools/perf/util/expr.y
> +++ b/tools/perf/util/expr.y
> @@ -38,6 +38,8 @@
> %type <num> expr if_expr
>
> %{
> +int expr__runtimeparam;
we don't like global variables.. could this be part of the
contaxt struct?
jirka
^ permalink raw reply
* Re: [PATCH v3 15/27] powerpc/powernv/pmem: Add support for near storage commands
From: Frederic Barrat @ 2020-03-02 17:58 UTC (permalink / raw)
To: Alastair D'Silva, alastair
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Masahiro Yamada,
Oliver O'Halloran, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Aneesh Kumar K . V, Krzysztof Kozlowski, Anju T Sudhakar,
Mahesh Salgaonkar, Andrew Donnellan, Arnd Bergmann, Greg Kurz,
Nicholas Piggin, Cédric Le Goater, Dan Williams,
Hari Bathini, linux-mm, Greg Kroah-Hartman, linux-kernel,
Vishal Verma, Paul Mackerras, Andrew Morton, linuxppc-dev,
David S. Miller
In-Reply-To: <20200221032720.33893-16-alastair@au1.ibm.com>
Le 21/02/2020 à 04:27, Alastair D'Silva a écrit :
> 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>
> ---
Is any of these new functions ever called?
Fred
> arch/powerpc/platforms/powernv/pmem/ocxl.c | 6 +++
> .../platforms/powernv/pmem/ocxl_internal.c | 41 +++++++++++++++++++
> .../platforms/powernv/pmem/ocxl_internal.h | 37 +++++++++++++++++
> 3 files changed, 84 insertions(+)
>
> diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl.c b/arch/powerpc/platforms/powernv/pmem/ocxl.c
> index 4e782d22605b..b8bd7e703b19 100644
> --- a/arch/powerpc/platforms/powernv/pmem/ocxl.c
> +++ b/arch/powerpc/platforms/powernv/pmem/ocxl.c
> @@ -259,12 +259,18 @@ static int setup_command_metadata(struct ocxlpmem *ocxlpmem)
> int rc;
>
> mutex_init(&ocxlpmem->admin_command.lock);
> + mutex_init(&ocxlpmem->ns_command.lock);
>
> rc = extract_command_metadata(ocxlpmem, GLOBAL_MMIO_ACMA_CREQO,
> &ocxlpmem->admin_command);
> if (rc)
> return rc;
>
> + rc = extract_command_metadata(ocxlpmem, GLOBAL_MMIO_NSCMA_CREQO,
> + &ocxlpmem->ns_command);
> + if (rc)
> + return rc;
> +
> return 0;
> }
>
> diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl_internal.c b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.c
> index 583f48023025..3e0b133feddf 100644
> --- a/arch/powerpc/platforms/powernv/pmem/ocxl_internal.c
> +++ b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.c
> @@ -133,6 +133,47 @@ int admin_response_handled(const struct ocxlpmem *ocxlpmem)
> OCXL_LITTLE_ENDIAN, GLOBAL_MMIO_CHI_ACRA);
> }
>
> +int ns_command_request(struct ocxlpmem *ocxlpmem, u8 op_code)
> +{
> + u64 val;
> + int rc = ocxl_global_mmio_read64(ocxlpmem->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(ocxlpmem, &ocxlpmem->ns_command, op_code);
> +}
> +
> +int ns_response(const struct ocxlpmem *ocxlpmem)
> +{
> + return command_response(ocxlpmem, &ocxlpmem->ns_command);
> +}
> +
> +int ns_command_execute(const struct ocxlpmem *ocxlpmem)
> +{
> + return ocxl_global_mmio_set64(ocxlpmem->ocxl_afu, GLOBAL_MMIO_HCI,
> + OCXL_LITTLE_ENDIAN, GLOBAL_MMIO_HCI_NSCRW);
> +}
> +
> +bool ns_command_complete(const struct ocxlpmem *ocxlpmem)
> +{
> + u64 val = 0;
> + int rc = ocxlpmem_chi(ocxlpmem, &val);
> +
> + WARN_ON(rc);
> +
> + return (val & GLOBAL_MMIO_CHI_NSCRA) != 0;
> +}
> +
> +int ns_response_handled(const struct ocxlpmem *ocxlpmem)
> +{
> + return ocxl_global_mmio_set64(ocxlpmem->ocxl_afu, GLOBAL_MMIO_CHIC,
> + OCXL_LITTLE_ENDIAN, GLOBAL_MMIO_CHI_NSCRA);
> +}
> +
> void warn_status(const struct ocxlpmem *ocxlpmem, const char *message,
> u8 status)
> {
> diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h
> index 2fef68c71271..28e2020f6355 100644
> --- a/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h
> +++ b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h
> @@ -107,6 +107,7 @@ struct ocxlpmem {
> struct ocxl_context *ocxl_context;
> void *metadata_addr;
> struct command_metadata admin_command;
> + struct command_metadata ns_command;
> struct resource pmem_res;
> struct nd_region *nd_region;
> char fw_version[8+1];
> @@ -175,6 +176,42 @@ int admin_command_complete_timeout(const struct ocxlpmem *ocxlpmem,
> */
> int admin_response_handled(const struct ocxlpmem *ocxlpmem);
>
> +/**
> + * ns_command_request() - Issue a near storage command request
> + * @ocxlpmem: the device metadata
> + * @op_code: The op-code for the command
> + * Returns an identifier for the command, or negative on error
> + */
> +int ns_command_request(struct ocxlpmem *ocxlpmem, u8 op_code);
> +
> +/**
> + * ns_response() - Validate a near storage response
> + * @ocxlpmem: the device metadata
> + * Returns the status code of the command, or negative on error
> + */
> +int ns_response(const struct ocxlpmem *ocxlpmem);
> +
> +/**
> + * ns_command_execute() - Notify the controller to start processing a pending near storage command
> + * @ocxlpmem: the device metadata
> + * Returns 0 on success, negative on error
> + */
> +int ns_command_execute(const struct ocxlpmem *ocxlpmem);
> +
> +/**
> + * ns_command_complete() - Is a near storage command executing
> + * @ocxlpmem: the device metadata
> + * Returns true if the previous admin command has completed
> + */
> +bool ns_command_complete(const struct ocxlpmem *ocxlpmem);
> +
> +/**
> + * ns_response_handled() - Notify the controller that the near storage response has been handled
> + * @ocxlpmem: the device metadata
> + * Returns 0 on success, negative on failure
> + */
> +int ns_response_handled(const struct ocxlpmem *ocxlpmem);
> +
> /**
> * warn_status() - Emit a kernel warning showing a command status.
> * @ocxlpmem: the device metadata
>
^ permalink raw reply
* Re: [PATCH v3 13/27] powerpc/powernv/pmem: Read the capability registers & wait for device ready
From: Frederic Barrat @ 2020-03-02 17:51 UTC (permalink / raw)
To: Alastair D'Silva, alastair
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Masahiro Yamada,
Oliver O'Halloran, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Aneesh Kumar K . V, Krzysztof Kozlowski, Anju T Sudhakar,
Mahesh Salgaonkar, Andrew Donnellan, Arnd Bergmann, Greg Kurz,
Nicholas Piggin, Cédric Le Goater, Dan Williams,
Hari Bathini, linux-mm, Greg Kroah-Hartman, linux-kernel,
Vishal Verma, Paul Mackerras, Andrew Morton, linuxppc-dev,
David S. Miller
In-Reply-To: <20200221032720.33893-14-alastair@au1.ibm.com>
Le 21/02/2020 à 04:27, Alastair D'Silva a écrit :
> 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>
> ---
> arch/powerpc/platforms/powernv/pmem/Makefile | 2 +-
> arch/powerpc/platforms/powernv/pmem/ocxl.c | 92 +++++++++++++++++++
> .../platforms/powernv/pmem/ocxl_internal.c | 19 ++++
> .../platforms/powernv/pmem/ocxl_internal.h | 24 +++++
> 4 files changed, 136 insertions(+), 1 deletion(-)
> create mode 100644 arch/powerpc/platforms/powernv/pmem/ocxl_internal.c
>
> diff --git a/arch/powerpc/platforms/powernv/pmem/Makefile b/arch/powerpc/platforms/powernv/pmem/Makefile
> index 1c55c4193175..4ceda25907d4 100644
> --- a/arch/powerpc/platforms/powernv/pmem/Makefile
> +++ b/arch/powerpc/platforms/powernv/pmem/Makefile
> @@ -4,4 +4,4 @@ ccflags-$(CONFIG_PPC_WERROR) += -Werror
>
> obj-$(CONFIG_OCXL_PMEM) += ocxlpmem.o
>
> -ocxlpmem-y := ocxl.o
> +ocxlpmem-y := ocxl.o ocxl_internal.o
> diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl.c b/arch/powerpc/platforms/powernv/pmem/ocxl.c
> index 3c4eeb5dcc0f..431212c9f0cc 100644
> --- a/arch/powerpc/platforms/powernv/pmem/ocxl.c
> +++ b/arch/powerpc/platforms/powernv/pmem/ocxl.c
> @@ -8,6 +8,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>
> @@ -215,6 +216,36 @@ static int register_lpc_mem(struct ocxlpmem *ocxlpmem)
> return 0;
> }
>
> +/**
> + * is_usable() - Is a controller usable?
> + * @ocxlpmem: the device metadata
> + * @verbose: True to log errors
> + * Return: true if the controller is usable
> + */
> +static bool is_usable(const struct ocxlpmem *ocxlpmem, bool verbose)
> +{
> + u64 chi = 0;
> + int rc = ocxlpmem_chi(ocxlpmem, &chi);
> +
> + if (rc < 0)
> + return false;
> +
> + if (!(chi & GLOBAL_MMIO_CHI_CRDY)) {
> + if (verbose)
> + dev_err(&ocxlpmem->dev, "controller is not ready.\n");
> + return false;
> + }
> +
> + if (!(chi & GLOBAL_MMIO_CHI_MA)) {
> + if (verbose)
> + dev_err(&ocxlpmem->dev,
> + "controller does not have memory available.\n");
> + return false;
> + }
> +
> + return true;
> +}
> +
> /**
> * allocate_minor() - Allocate a minor number to use for an OpenCAPI pmem device
> * @ocxlpmem: the device metadata
> @@ -328,6 +359,48 @@ static void ocxlpmem_remove(struct pci_dev *pdev)
> }
> }
>
> +/**
> + * read_device_metadata() - Retrieve config information from the AFU and save it for future use
> + * @ocxlpmem: the device metadata
> + * Return: 0 on success, negative on failure
> + */
> +static int read_device_metadata(struct ocxlpmem *ocxlpmem)
> +{
> + u64 val;
> + int rc;
> +
> + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu, GLOBAL_MMIO_CCAP0,
> + OCXL_LITTLE_ENDIAN, &val);
> + if (rc)
> + return rc;
> +
> + ocxlpmem->scm_revision = val & 0xFFFF;
> + ocxlpmem->read_latency = (val >> 32) & 0xFF;
> + ocxlpmem->readiness_timeout = (val >> 48) & 0x0F;
> + ocxlpmem->memory_available_timeout = val >> 52;
> +
> + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu, GLOBAL_MMIO_CCAP1,
> + OCXL_LITTLE_ENDIAN, &val);
> + if (rc)
> + return rc;
> +
> + ocxlpmem->max_controller_dump_size = val & 0xFFFFFFFF;
> +
> + // Extract firmware version text
> + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu, GLOBAL_MMIO_FWVER,
> + OCXL_HOST_ENDIAN, (u64 *)ocxlpmem->fw_version);
> + if (rc)
> + return rc;
> +
> + ocxlpmem->fw_version[8] = '\0';
> +
> + dev_info(&ocxlpmem->dev,
> + "Firmware version '%s' SCM revision %d:%d\n", ocxlpmem->fw_version,
> + ocxlpmem->scm_revision >> 4, ocxlpmem->scm_revision & 0x0F);
> +
> + return 0;
> +}
> +
> /**
> * probe_function0() - Set up function 0 for an OpenCAPI persistent memory device
> * This is important as it enables templates higher than 0 across all other functions,
> @@ -368,6 +441,7 @@ static int probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> struct ocxlpmem *ocxlpmem;
> int rc;
> + u16 elapsed, timeout;
>
> if (PCI_FUNC(pdev->devfn) == 0)
> return probe_function0(pdev);
> @@ -422,6 +496,24 @@ static int probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> goto err;
> }
>
> + if (read_device_metadata(ocxlpmem)) {
> + dev_err(&pdev->dev, "Could not read metadata\n");
Need to set rc
> + goto err;
> + }
> +
> + elapsed = 0;
> + timeout = ocxlpmem->readiness_timeout + ocxlpmem->memory_available_timeout;
> + while (!is_usable(ocxlpmem, false)) {
> + if (elapsed++ > timeout) {
> + dev_warn(&ocxlpmem->dev, "OpenCAPI Persistent Memory ready timeout.\n");
> + (void)is_usable(ocxlpmem, true);
I guess that extra call to is_usable() is just to log the cause of the
error. However, with some bad luck, the call could now succeed.
Fred
> + rc = -ENXIO;
> + goto err;
> + }
> +
> + msleep(1000);
> + }
> +
> rc = register_lpc_mem(ocxlpmem);
> if (rc) {
> dev_err(&pdev->dev, "Could not register OpenCAPI persistent memory with libnvdimm\n");
> diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl_internal.c b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.c
> new file mode 100644
> index 000000000000..617ca943b1b8
> --- /dev/null
> +++ b/arch/powerpc/platforms/powernv/pmem/ocxl_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 "ocxl_internal.h"
> +
> +int ocxlpmem_chi(const struct ocxlpmem *ocxlpmem, u64 *chi)
> +{
> + u64 val;
> + int rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu, GLOBAL_MMIO_CHI,
> + OCXL_LITTLE_ENDIAN, &val);
> + if (rc)
> + return rc;
> +
> + *chi = val;
> +
> + return 0;
> +}
> diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h
> index 9cf3e42750e7..ba0301533d00 100644
> --- a/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h
> +++ b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h
> @@ -97,4 +97,28 @@ struct ocxlpmem {
> void *metadata_addr;
> struct resource pmem_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
> + */
> };
> +
> +/**
> + * ocxlpmem_chi() - Get the value of the CHI register
> + * @ocxlpmem: the device metadata
> + * @chi: returns the CHI value
> + *
> + * Returns 0 on success, negative on error
> + */
> +int ocxlpmem_chi(const struct ocxlpmem *ocxlpmem, u64 *chi);
>
^ permalink raw reply
* eh_frame confusion
From: Naveen N. Rao @ 2020-03-02 17:32 UTC (permalink / raw)
To: Linux Kbuild mailing list, LKML, linuxppc-dev@lists.ozlabs.org,
Rasmus Villemoes, Michael Ellerman
In-Reply-To: <1583168442.ovqnxu16tp.naveen@linux.ibm.com>
Naveen N. Rao wrote:
> Rasmus Villemoes wrote:
>> I'm building a ppc32 kernel, and noticed that after upgrading from gcc-7
>> to gcc-8 all object files now end up having .eh_frame section. For
>> vmlinux, that's not a problem, because they all get discarded in
>> arch/powerpc/kernel/vmlinux.lds.S . However, they stick around in
>> modules, which doesn't seem to be useful - given that everything worked
>> just fine with gcc-7, and I don't see anything in the module loader that
>> handles .eh_frame.
>>
>> The reason I care is that my target has a rather tight rootfs budget,
>> and the .eh_frame section seem to occupy 10-30% of the file size
>> (obviously very depending on the particular module).
>>
>> Comparing the .foo.o.cmd files, I don't see change in options that might
>> explain this (there's a bunch of new -Wno-*, and the -mspe=no spelling
>> is apparently no longer supported in gcc-8). Both before and after, there's
>>
>> -fno-dwarf2-cfi-asm
>>
>> about which gcc's documentation says
>>
>> '-fno-dwarf2-cfi-asm'
>> Emit DWARF unwind info as compiler generated '.eh_frame' section
>> instead of using GAS '.cfi_*' directives.
>>
>> Looking into where that comes from got me even more confused, because
>> both arm and unicore32 say
>>
>> # Never generate .eh_frame
>> KBUILD_CFLAGS += $(call cc-option,-fno-dwarf2-cfi-asm)
>>
>> while the ppc32 case at hand says
>>
>> # FIXME: the module load should be taught about the additional relocs
>> # generated by this.
>> # revert to pre-gcc-4.4 behaviour of .eh_frame
>
> Michael opened a task to look into this recently and I had spent some
> time last week on this. The original commit/discussion adding
> -fno-dwarf2-cfi-asm refers to R_PPC64_REL32 relocations not being
> handled by our module loader:
> http://lkml.kernel.org/r/20090224065112.GA6690@bombadil.infradead.org
>
> However, that is now handled thanks to commit 9f751b82b491d:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9f751b82b491d
>
> I did a test build and a simple module loaded fine, so I think
> -fno-dwarf2-cfi-asm is not required anymore, unless Michael has seen
> some breakages with it. Michael?
>
>>
>> but prior to gcc-8, .eh_frame didn't seem to get generated anyway.
>>
>> Can .eh_frame sections be discarded for modules (on ppc32 at least), or
>> is there some magic that makes them necessary when building with gcc-8?
>
> As Segher points out, it looks like we need to add
> -fno-asynchronous-unwind-tables. Most other architectures seem to use
> that too.
Can you check if the below patch works? I am yet to test this in more
detail, but would be good to know the implications for ppc32.
- Naveen
---
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index f35730548e42..5b5bf98b8217 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -239,10 +239,7 @@ KBUILD_CFLAGS += $(call cc-option,-mno-vsx)
KBUILD_CFLAGS += $(call cc-option,-mno-spe)
KBUILD_CFLAGS += $(call cc-option,-mspe=no)
-# FIXME: the module load should be taught about the additional relocs
-# generated by this.
-# revert to pre-gcc-4.4 behaviour of .eh_frame
-KBUILD_CFLAGS += $(call cc-option,-fno-dwarf2-cfi-asm)
+KBUILD_CFLAGS += $(call cc-option,-fno-asynchronous-unwind-tables)
# Never use string load/store instructions as they are
# often slow when they are implemented at all
diff --git a/arch/powerpc/kernel/vdso32/Makefile b/arch/powerpc/kernel/vdso32/Makefile
index e147bbdc12cd..d43b0b18137c 100644
--- a/arch/powerpc/kernel/vdso32/Makefile
+++ b/arch/powerpc/kernel/vdso32/Makefile
@@ -25,6 +25,7 @@ KCOV_INSTRUMENT := n
UBSAN_SANITIZE := n
ccflags-y := -shared -fno-common -fno-builtin -nostdlib \
+ -fasynchronous-unwind-tables \
-Wl,-soname=linux-vdso32.so.1 -Wl,--hash-style=both
asflags-y := -D__VDSO32__ -s
diff --git a/arch/powerpc/kernel/vdso64/Makefile b/arch/powerpc/kernel/vdso64/Makefile
index 32ebb3522ea1..b2cbb5c49bad 100644
--- a/arch/powerpc/kernel/vdso64/Makefile
+++ b/arch/powerpc/kernel/vdso64/Makefile
@@ -13,6 +13,7 @@ KCOV_INSTRUMENT := n
UBSAN_SANITIZE := n
ccflags-y := -shared -fno-common -fno-builtin -nostdlib \
+ -fasynchronous-unwind-tables \
-Wl,-soname=linux-vdso64.so.1 -Wl,--hash-style=both
asflags-y := -D__VDSO64__ -s
^ permalink raw reply related
* Re: eh_frame confusion
From: Naveen N. Rao @ 2020-03-02 17:17 UTC (permalink / raw)
To: Rasmus Villemoes, Segher Boessenkool, Michael Ellerman
Cc: linuxppc-dev@lists.ozlabs.org, LKML, Linux Kbuild mailing list
In-Reply-To: <20200302124442.GI22482@gate.crashing.org>
Segher Boessenkool wrote:
> On Mon, Mar 02, 2020 at 11:56:05AM +0100, Rasmus Villemoes wrote:
>> I'm building a ppc32 kernel, and noticed that after upgrading from gcc-7
>> to gcc-8 all object files now end up having .eh_frame section.
>
> Since GCC 8, we enable -fasynchronous-unwind-tables by default for
> PowerPC. See https://gcc.gnu.org/r259298 .
>
>> For
>> vmlinux, that's not a problem, because they all get discarded in
>> arch/powerpc/kernel/vmlinux.lds.S . However, they stick around in
>> modules, which doesn't seem to be useful - given that everything worked
>> just fine with gcc-7, and I don't see anything in the module loader that
>> handles .eh_frame.
>
> It is useful for debugging. Not many people debug the kernel like this,
> of course.
I'm trying to understand if we need that. Other architectures seems to
pass -fasynchronous-unwind-tables only for the vdso, but disable it for
the kernel build. I suppose we can do the same.
If using -fno-asynchronous-unwind-tables, would crash/perf have
problems?
- Naveen
^ permalink raw reply
* eh_frame confusion
From: Naveen N. Rao @ 2020-03-02 17:09 UTC (permalink / raw)
To: Linux Kbuild mailing list, LKML, linuxppc-dev@lists.ozlabs.org,
Rasmus Villemoes, Michael Ellerman
In-Reply-To: <3b00b45f-74b5-13e3-9a98-c3d6b3bb7286@rasmusvillemoes.dk>
Rasmus Villemoes wrote:
> I'm building a ppc32 kernel, and noticed that after upgrading from gcc-7
> to gcc-8 all object files now end up having .eh_frame section. For
> vmlinux, that's not a problem, because they all get discarded in
> arch/powerpc/kernel/vmlinux.lds.S . However, they stick around in
> modules, which doesn't seem to be useful - given that everything worked
> just fine with gcc-7, and I don't see anything in the module loader that
> handles .eh_frame.
>
> The reason I care is that my target has a rather tight rootfs budget,
> and the .eh_frame section seem to occupy 10-30% of the file size
> (obviously very depending on the particular module).
>
> Comparing the .foo.o.cmd files, I don't see change in options that might
> explain this (there's a bunch of new -Wno-*, and the -mspe=no spelling
> is apparently no longer supported in gcc-8). Both before and after, there's
>
> -fno-dwarf2-cfi-asm
>
> about which gcc's documentation says
>
> '-fno-dwarf2-cfi-asm'
> Emit DWARF unwind info as compiler generated '.eh_frame' section
> instead of using GAS '.cfi_*' directives.
>
> Looking into where that comes from got me even more confused, because
> both arm and unicore32 say
>
> # Never generate .eh_frame
> KBUILD_CFLAGS += $(call cc-option,-fno-dwarf2-cfi-asm)
>
> while the ppc32 case at hand says
>
> # FIXME: the module load should be taught about the additional relocs
> # generated by this.
> # revert to pre-gcc-4.4 behaviour of .eh_frame
Michael opened a task to look into this recently and I had spent some
time last week on this. The original commit/discussion adding
-fno-dwarf2-cfi-asm refers to R_PPC64_REL32 relocations not being
handled by our module loader:
http://lkml.kernel.org/r/20090224065112.GA6690@bombadil.infradead.org
However, that is now handled thanks to commit 9f751b82b491d:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9f751b82b491d
I did a test build and a simple module loaded fine, so I think
-fno-dwarf2-cfi-asm is not required anymore, unless Michael has seen
some breakages with it. Michael?
>
> but prior to gcc-8, .eh_frame didn't seem to get generated anyway.
>
> Can .eh_frame sections be discarded for modules (on ppc32 at least), or
> is there some magic that makes them necessary when building with gcc-8?
As Segher points out, it looks like we need to add
-fno-asynchronous-unwind-tables. Most other architectures seem to use
that too.
- Naveen
^ permalink raw reply
* [Bug 206733] i2c i2c-3: i2c-powermac: modalias failure on /uni-n@f8000000/i2c@f8001000/cereal@1c0
From: bugzilla-daemon @ 2020-03-02 16:58 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-206733-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=206733
--- Comment #1 from Erhard F. (erhard_f@mailbox.org) ---
Created attachment 287761
--> https://bugzilla.kernel.org/attachment.cgi?id=287761&action=edit
kernel .config (5.6-rc4, PowerMac G4 DP)
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* [Bug 206733] New: i2c i2c-3: i2c-powermac: modalias failure on /uni-n@f8000000/i2c@f8001000/cereal@1c0
From: bugzilla-daemon @ 2020-03-02 16:50 UTC (permalink / raw)
To: linuxppc-dev
https://bugzilla.kernel.org/show_bug.cgi?id=206733
Bug ID: 206733
Summary: i2c i2c-3: i2c-powermac: modalias failure on
/uni-n@f8000000/i2c@f8001000/cereal@1c0
Product: Platform Specific/Hardware
Version: 2.5
Kernel Version: 5.6-rc4
Hardware: PPC-32
OS: Linux
Tree: Mainline
Status: NEW
Severity: normal
Priority: P1
Component: PPC-32
Assignee: platform_ppc-32@kernel-bugs.osdl.org
Reporter: erhard_f@mailbox.org
Regression: No
Created attachment 287759
--> https://bugzilla.kernel.org/attachment.cgi?id=287759&action=edit
dmesg (5.6-rc4, PowerMac G4 DP)
The G4 MDD/DP can't quite pick up this device, despite it shows up in the
bootlog earlier.
[...]
Mär 02 17:23:45 T600 kernel: i2c-dev: adapter [uni-n 1] registered as minor 3
Mär 02 17:23:45 T600 kernel: i2c i2c-3: adapter [uni-n 1] registered
Mär 02 17:23:45 T600 kernel: PowerMac i2c bus uni-n 1 registered
Mär 02 17:23:45 T600 kernel: i2c i2c-3: i2c-powermac: register
/uni-n@f8000000/i2c@f8001000/cereal@1c0
Mär 02 17:23:45 T600 kernel: i2c i2c-3: i2c-powermac: modalias failure on
/uni-n@f8000000/i2c@f8001000/cereal@1c0
Mär 02 17:23:45 T600 kernel: i2c-dev: adapter [uni-n 0] registered as minor 4
[...]
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ 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