* Re: [PATCH v2 06/16] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
From: David Gibson @ 2019-03-13 4:03 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <9e732140-b2c0-dfb7-d753-ba0ec7f3b930@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 1680 bytes --]
On Tue, Mar 12, 2019 at 06:00:38PM +0100, Cédric Le Goater wrote:
> On 2/25/19 3:39 AM, David Gibson wrote:
> > On Fri, Feb 22, 2019 at 12:28:30PM +0100, Cédric Le Goater wrote:
> >> These controls will be used by the H_INT_SET_QUEUE_CONFIG and
> >> H_INT_GET_QUEUE_CONFIG hcalls from QEMU. They will also be used to
> >> restore the configuration of the XIVE EQs in the KVM device and to
> >> capture the internal runtime state of the EQs. Both 'get' and 'set'
> >> rely on an OPAL call to access from the XIVE interrupt controller the
> >> EQ toggle bit and EQ index which are updated by the HW when event
> >> notifications are enqueued in the EQ.
> >>
> >> The value of the guest physical address of the event queue is saved in
> >> the XIVE internal xive_q structure for later use. That is when
> >> migration needs to mark the EQ pages dirty to capture a consistent
> >> memory state of the VM.
> >>
> >> To be noted that H_INT_SET_QUEUE_CONFIG does not require the extra
> >> OPAL call setting the EQ toggle bit and EQ index to configure the EQ,
> >> but restoring the EQ state will.
>
> I think we need to add some kind of flags to differentiate the hcall
> H_INT_SET_QUEUE_CONFIG from the restore of the EQ. The hcall does
> not need OPAL support call and this could help in the code
> transition.
Hrm. What's the actual difference in the semantics between the two
cases. The guest shouldn't have awareness of whether or not OPAL is
involved.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug()
From: David Gibson @ 2019-03-13 4:27 UTC (permalink / raw)
To: Laurent Vivier; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20190308105413.4302-1-lvivier@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 4526 bytes --]
On Fri, Mar 08, 2019 at 11:54:13AM +0100, Laurent Vivier wrote:
> resize_hpt_for_hotplug() reports a warning when it cannot
> resize the hash page table ("Unable to resize hash page
> table to target order") but in some cases it's not a problem
> and can make user thinks something has not worked properly.
>
> This patch moves the warning to arch_remove_memory() to
> only report the problem when it is needed.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> arch/powerpc/include/asm/sparsemem.h | 4 ++--
> arch/powerpc/mm/hash_utils_64.c | 17 ++++++-----------
> arch/powerpc/mm/mem.c | 3 ++-
> arch/powerpc/platforms/pseries/lpar.c | 3 ++-
> 4 files changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/sparsemem.h b/arch/powerpc/include/asm/sparsemem.h
> index 68da49320592..3192d454a733 100644
> --- a/arch/powerpc/include/asm/sparsemem.h
> +++ b/arch/powerpc/include/asm/sparsemem.h
> @@ -17,9 +17,9 @@ extern int create_section_mapping(unsigned long start, unsigned long end, int ni
> extern int remove_section_mapping(unsigned long start, unsigned long end);
>
> #ifdef CONFIG_PPC_BOOK3S_64
> -extern void resize_hpt_for_hotplug(unsigned long new_mem_size);
> +extern int resize_hpt_for_hotplug(unsigned long new_mem_size);
> #else
> -static inline void resize_hpt_for_hotplug(unsigned long new_mem_size) { }
> +static inline int resize_hpt_for_hotplug(unsigned long new_mem_size) { return 0; }
> #endif
>
> #ifdef CONFIG_NUMA
> diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
> index 0cc7fbc3bd1c..40bb2a8326bb 100644
> --- a/arch/powerpc/mm/hash_utils_64.c
> +++ b/arch/powerpc/mm/hash_utils_64.c
> @@ -755,12 +755,12 @@ static unsigned long __init htab_get_table_size(void)
> }
>
> #ifdef CONFIG_MEMORY_HOTPLUG
> -void resize_hpt_for_hotplug(unsigned long new_mem_size)
> +int resize_hpt_for_hotplug(unsigned long new_mem_size)
> {
> unsigned target_hpt_shift;
>
> if (!mmu_hash_ops.resize_hpt)
> - return;
> + return 0;
>
> target_hpt_shift = htab_shift_for_mem_size(new_mem_size);
>
> @@ -773,15 +773,10 @@ void resize_hpt_for_hotplug(unsigned long new_mem_size)
> * current shift
> */
> if ((target_hpt_shift > ppc64_pft_size)
> - || (target_hpt_shift < (ppc64_pft_size - 1))) {
> - int rc;
> -
> - rc = mmu_hash_ops.resize_hpt(target_hpt_shift);
> - if (rc && (rc != -ENODEV))
> - printk(KERN_WARNING
> - "Unable to resize hash page table to target order %d: %d\n",
> - target_hpt_shift, rc);
> - }
> + || (target_hpt_shift < (ppc64_pft_size - 1)))
> + return mmu_hash_ops.resize_hpt(target_hpt_shift);
> +
> + return 0;
> }
>
> int hash__create_section_mapping(unsigned long start, unsigned long end, int nid)
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index 33cc6f676fa6..0d40d970cf4a 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -169,7 +169,8 @@ int __meminit arch_remove_memory(int nid, u64 start, u64 size,
> */
> vm_unmap_aliases();
>
> - resize_hpt_for_hotplug(memblock_phys_mem_size());
> + if (resize_hpt_for_hotplug(memblock_phys_mem_size()) == -ENOSPC)
> + pr_warn("Hash collision while resizing HPT\n");
>
> return ret;
> }
> diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
> index f2a9f0adc2d3..1034ef1fe2b4 100644
> --- a/arch/powerpc/platforms/pseries/lpar.c
> +++ b/arch/powerpc/platforms/pseries/lpar.c
> @@ -901,8 +901,10 @@ static int pseries_lpar_resize_hpt(unsigned long shift)
> break;
>
> case H_PARAMETER:
> + pr_warn("Invalid argument from H_RESIZE_HPT_PREPARE\n");
> return -EINVAL;
> case H_RESOURCE:
> + pr_warn("Operation not permitted from H_RESIZE_HPT_PREPARE\n");
> return -EPERM;
> default:
> pr_warn("Unexpected error %d from H_RESIZE_HPT_PREPARE\n", rc);
> @@ -918,7 +920,6 @@ static int pseries_lpar_resize_hpt(unsigned long shift)
> if (rc != 0) {
> switch (state.commit_rc) {
> case H_PTEG_FULL:
> - pr_warn("Hash collision while resizing HPT\n");
> return -ENOSPC;
>
> default:
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2] fs/dax: deposit pagetable even when installing zero page
From: Aneesh Kumar K.V @ 2019-03-13 4:47 UTC (permalink / raw)
To: dan.j.williams, Ross Zwisler, Jan Kara, akpm
Cc: linux-fsdevel, linux-mm, linuxppc-dev, Alexander Viro,
linux-nvdimm
In-Reply-To: <20190309120721.21416-1-aneesh.kumar@linux.ibm.com>
Hi Dan/Andrew/Jan,
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> Architectures like ppc64 use the deposited page table to store hardware
> page table slot information. Make sure we deposit a page table when
> using zero page at the pmd level for hash.
>
> Without this we hit
>
> Unable to handle kernel paging request for data at address 0x00000000
> Faulting instruction address: 0xc000000000082a74
> Oops: Kernel access of bad area, sig: 11 [#1]
> ....
>
> NIP [c000000000082a74] __hash_page_thp+0x224/0x5b0
> LR [c0000000000829a4] __hash_page_thp+0x154/0x5b0
> Call Trace:
> hash_page_mm+0x43c/0x740
> do_hash_page+0x2c/0x3c
> copy_from_iter_flushcache+0xa4/0x4a0
> pmem_copy_from_iter+0x2c/0x50 [nd_pmem]
> dax_copy_from_iter+0x40/0x70
> dax_iomap_actor+0x134/0x360
> iomap_apply+0xfc/0x1b0
> dax_iomap_rw+0xac/0x130
> ext4_file_write_iter+0x254/0x460 [ext4]
> __vfs_write+0x120/0x1e0
> vfs_write+0xd8/0x220
> SyS_write+0x6c/0x110
> system_call+0x3c/0x130
>
> Fixes: b5beae5e224f ("powerpc/pseries: Add driver for PAPR SCM regions")
> Reviewed-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Any suggestion on which tree this patch should got to? Also since this
fix a kernel crash, we may want to get this to 5.1?
> ---
> Changes from v1:
> * Add reviewed-by:
> * Add Fixes:
>
> fs/dax.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/fs/dax.c b/fs/dax.c
> index 6959837cc465..01bfb2ac34f9 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -33,6 +33,7 @@
> #include <linux/sizes.h>
> #include <linux/mmu_notifier.h>
> #include <linux/iomap.h>
> +#include <asm/pgalloc.h>
> #include "internal.h"
>
> #define CREATE_TRACE_POINTS
> @@ -1410,7 +1411,9 @@ static vm_fault_t dax_pmd_load_hole(struct xa_state *xas, struct vm_fault *vmf,
> {
> struct address_space *mapping = vmf->vma->vm_file->f_mapping;
> unsigned long pmd_addr = vmf->address & PMD_MASK;
> + struct vm_area_struct *vma = vmf->vma;
> struct inode *inode = mapping->host;
> + pgtable_t pgtable = NULL;
> struct page *zero_page;
> spinlock_t *ptl;
> pmd_t pmd_entry;
> @@ -1425,12 +1428,22 @@ static vm_fault_t dax_pmd_load_hole(struct xa_state *xas, struct vm_fault *vmf,
> *entry = dax_insert_entry(xas, mapping, vmf, *entry, pfn,
> DAX_PMD | DAX_ZERO_PAGE, false);
>
> + if (arch_needs_pgtable_deposit()) {
> + pgtable = pte_alloc_one(vma->vm_mm);
> + if (!pgtable)
> + return VM_FAULT_OOM;
> + }
> +
> ptl = pmd_lock(vmf->vma->vm_mm, vmf->pmd);
> if (!pmd_none(*(vmf->pmd))) {
> spin_unlock(ptl);
> goto fallback;
> }
>
> + if (pgtable) {
> + pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
> + mm_inc_nr_ptes(vma->vm_mm);
> + }
> pmd_entry = mk_pmd(zero_page, vmf->vma->vm_page_prot);
> pmd_entry = pmd_mkhuge(pmd_entry);
> set_pmd_at(vmf->vma->vm_mm, pmd_addr, vmf->pmd, pmd_entry);
> @@ -1439,6 +1452,8 @@ static vm_fault_t dax_pmd_load_hole(struct xa_state *xas, struct vm_fault *vmf,
> return VM_FAULT_NOPAGE;
>
> fallback:
> + if (pgtable)
> + pte_free(vma->vm_mm, pgtable);
> trace_dax_pmd_load_hole_fallback(inode, vmf, zero_page, *entry);
> return VM_FAULT_FALLBACK;
> }
> --
> 2.20.1
-aneesh
^ permalink raw reply
* Re: [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug()
From: Christophe Leroy @ 2019-03-13 6:03 UTC (permalink / raw)
To: Laurent Vivier, linux-kernel; +Cc: linuxppc-dev, David Gibson
In-Reply-To: <20190308105413.4302-1-lvivier@redhat.com>
Le 08/03/2019 à 11:54, Laurent Vivier a écrit :
> resize_hpt_for_hotplug() reports a warning when it cannot
> resize the hash page table ("Unable to resize hash page
> table to target order") but in some cases it's not a problem
> and can make user thinks something has not worked properly.
>
> This patch moves the warning to arch_remove_memory() to
> only report the problem when it is needed.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
> arch/powerpc/include/asm/sparsemem.h | 4 ++--
> arch/powerpc/mm/hash_utils_64.c | 17 ++++++-----------
> arch/powerpc/mm/mem.c | 3 ++-
> arch/powerpc/platforms/pseries/lpar.c | 3 ++-
> 4 files changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/sparsemem.h b/arch/powerpc/include/asm/sparsemem.h
> index 68da49320592..3192d454a733 100644
> --- a/arch/powerpc/include/asm/sparsemem.h
> +++ b/arch/powerpc/include/asm/sparsemem.h
> @@ -17,9 +17,9 @@ extern int create_section_mapping(unsigned long start, unsigned long end, int ni
> extern int remove_section_mapping(unsigned long start, unsigned long end);
>
> #ifdef CONFIG_PPC_BOOK3S_64
> -extern void resize_hpt_for_hotplug(unsigned long new_mem_size);
> +extern int resize_hpt_for_hotplug(unsigned long new_mem_size);
> #else
> -static inline void resize_hpt_for_hotplug(unsigned long new_mem_size) { }
> +static inline int resize_hpt_for_hotplug(unsigned long new_mem_size) { return 0; }
> #endif
>
> #ifdef CONFIG_NUMA
> diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
> index 0cc7fbc3bd1c..40bb2a8326bb 100644
> --- a/arch/powerpc/mm/hash_utils_64.c
> +++ b/arch/powerpc/mm/hash_utils_64.c
> @@ -755,12 +755,12 @@ static unsigned long __init htab_get_table_size(void)
> }
>
> #ifdef CONFIG_MEMORY_HOTPLUG
> -void resize_hpt_for_hotplug(unsigned long new_mem_size)
> +int resize_hpt_for_hotplug(unsigned long new_mem_size)
> {
> unsigned target_hpt_shift;
>
> if (!mmu_hash_ops.resize_hpt)
> - return;
> + return 0;
>
> target_hpt_shift = htab_shift_for_mem_size(new_mem_size);
>
> @@ -773,15 +773,10 @@ void resize_hpt_for_hotplug(unsigned long new_mem_size)
> * current shift
> */
> if ((target_hpt_shift > ppc64_pft_size)
> - || (target_hpt_shift < (ppc64_pft_size - 1))) {
> - int rc;
> -
> - rc = mmu_hash_ops.resize_hpt(target_hpt_shift);
> - if (rc && (rc != -ENODEV))
> - printk(KERN_WARNING
> - "Unable to resize hash page table to target order %d: %d\n",
> - target_hpt_shift, rc);
> - }
> + || (target_hpt_shift < (ppc64_pft_size - 1)))
The || should go on the line above and the two (target_hpt... should be
aligned, and the () after the < are superflous.
And indeed, we should (in another patch) rename 'target_hpt_shift' with
a shorter name, this would avoid multiple lines.
Ref
https://www.kernel.org/doc/html/latest/process/coding-style.html#naming :
LOCAL variable names should be short, and to the point. If you have some
random integer loop counter, it should probably be called i. Calling it
loop_counter is non-productive, if there is no chance of it being
mis-understood. Similarly, tmp can be just about any type of variable
that is used to hold a temporary value.
If you are afraid to mix up your local variable names, you have another
problem, which is called the function-growth-hormone-imbalance syndrome.
See chapter 6 (Functions).
Christophe
> + return mmu_hash_ops.resize_hpt(target_hpt_shift);
> +
> + return 0;
> }
>
> int hash__create_section_mapping(unsigned long start, unsigned long end, int nid)
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index 33cc6f676fa6..0d40d970cf4a 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -169,7 +169,8 @@ int __meminit arch_remove_memory(int nid, u64 start, u64 size,
> */
> vm_unmap_aliases();
>
> - resize_hpt_for_hotplug(memblock_phys_mem_size());
> + if (resize_hpt_for_hotplug(memblock_phys_mem_size()) == -ENOSPC)
> + pr_warn("Hash collision while resizing HPT\n");
>
> return ret;
> }
> diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
> index f2a9f0adc2d3..1034ef1fe2b4 100644
> --- a/arch/powerpc/platforms/pseries/lpar.c
> +++ b/arch/powerpc/platforms/pseries/lpar.c
> @@ -901,8 +901,10 @@ static int pseries_lpar_resize_hpt(unsigned long shift)
> break;
>
> case H_PARAMETER:
> + pr_warn("Invalid argument from H_RESIZE_HPT_PREPARE\n");
> return -EINVAL;
> case H_RESOURCE:
> + pr_warn("Operation not permitted from H_RESIZE_HPT_PREPARE\n");
> return -EPERM;
> default:
> pr_warn("Unexpected error %d from H_RESIZE_HPT_PREPARE\n", rc);
> @@ -918,7 +920,6 @@ static int pseries_lpar_resize_hpt(unsigned long shift)
> if (rc != 0) {
> switch (state.commit_rc) {
> case H_PTEG_FULL:
> - pr_warn("Hash collision while resizing HPT\n");
> return -ENOSPC;
>
> default:
>
^ permalink raw reply
* Re: [PATCH v4 2/9] powerpc/powernv/idle: Restore AMR/UAMOR/AMOR after idle
From: Akshay Adiga @ 2019-03-13 6:11 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, npiggin
In-Reply-To: <20190228144917.16876-2-mpe@ellerman.id.au>
On Fri, Mar 01, 2019 at 01:49:10AM +1100, Michael Ellerman wrote:
> In order to implement KUAP (Kernel Userspace Access Protection) on
> Power9 we will be using the AMR, and therefore indirectly the
> UAMOR/AMOR.
>
> So save/restore these regs in the idle code.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> arch/powerpc/kernel/idle_book3s.S | 27 +++++++++++++++++++++++----
> 1 file changed, 23 insertions(+), 4 deletions(-)
>
> v4: New.
>
> diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
> index 36178000a2f2..4a860d3b9229 100644
> --- a/arch/powerpc/kernel/idle_book3s.S
> +++ b/arch/powerpc/kernel/idle_book3s.S
> @@ -170,8 +170,11 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_300)
> bne- core_idle_lock_held
> blr
>
> -/* Reuse an unused pt_regs slot for IAMR */
> +/* Reuse some unused pt_regs slots for AMR/IAMR/UAMOR/UAMOR */
> +#define PNV_POWERSAVE_AMR _TRAP
> #define PNV_POWERSAVE_IAMR _DAR
> +#define PNV_POWERSAVE_UAMOR _DSISR
> +#define PNV_POWERSAVE_AMOR RESULT
>
> /*
> * Pass requested state in r3:
> @@ -205,8 +208,16 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_300)
> SAVE_NVGPRS(r1)
>
> BEGIN_FTR_SECTION
> + mfspr r4, SPRN_AMR
> mfspr r5, SPRN_IAMR
> + mfspr r6, SPRN_UAMOR
> + std r4, PNV_POWERSAVE_AMR(r1)
> std r5, PNV_POWERSAVE_IAMR(r1)
> + std r6, PNV_POWERSAVE_UAMOR(r1)
> +BEGIN_FTR_SECTION_NESTED(42)
> + mfspr r7, SPRN_AMOR
> + std r7, PNV_POWERSAVE_AMOR(r1)
> +END_FTR_SECTION_NESTED_IFSET(CPU_FTR_HVMODE, 42)
> END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
Can we save/restore for ARCH_300S only ?
I checked the user manual "POWER8 Processor User’s Manual for the
Single-Chip Module" v1.3 dated 16 March 2016. In section 9.7.2.2,
It mentions that AMOR and UAMOR will be preserved by hardware.
Excrept :
" When an SMT thread switch is enabled, the napping thread’s
resources can be given to other threads to improve core
performance. Software must restore the architected state
of the dormant thread upon exiting nap mode except for
the following architected facilities, which are preserved
by the hardware:
- SLB State
- All Hypervisor Special Registers (includes PURR, SPURR, AMOR,
UAMOR, AMR, ACOP)
"
Keeping 207S has no harm, its just that we may spend a few
more cycles saving and restoring the sprs.
>
> mfcr r5
> @@ -935,12 +946,20 @@ END_FTR_SECTION_IFSET(CPU_FTR_HVMODE)
> REST_GPR(2, r1)
>
> BEGIN_FTR_SECTION
> - /* IAMR was saved in pnv_powersave_common() */
> + /* These regs were saved in pnv_powersave_common() */
> + ld r4, PNV_POWERSAVE_AMR(r1)
> ld r5, PNV_POWERSAVE_IAMR(r1)
> + ld r6, PNV_POWERSAVE_UAMOR(r1)
> + mtspr SPRN_AMR, r4
> mtspr SPRN_IAMR, r5
> + mtspr SPRN_UAMOR, r6
> +BEGIN_FTR_SECTION_NESTED(42)
> + ld r7, PNV_POWERSAVE_AMOR(r1)
> + mtspr SPRN_AMOR, r7
> +END_FTR_SECTION_NESTED_IFSET(CPU_FTR_HVMODE, 42)
> /*
> - * We don't need an isync here because the upcoming mtmsrd is
> - * execution synchronizing.
> + * We don't need an isync here after restoring IAMR because the upcoming
> + * mtmsrd is execution synchronizing.
> */
> END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
>
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH RFC v3 18/18] powerpc: KASAN for 64bit Book3E
From: Christophe Leroy @ 2019-03-13 7:02 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Nicholas Piggin, Aneesh Kumar K.V, Andrey Ryabinin,
Alexander Potapenko, Dmitry Vyukov, Daniel Axtens
Cc: linux-mm, linuxppc-dev, linux-kernel, kasan-dev
In-Reply-To: <3e97aba429c769bd99ccd8d6f16eda98f7d378a7.1552428161.git.christophe.leroy@c-s.fr>
Why does snowpatch reports not being able to apply it to any branch ?
I built the serie on top of merge branch, but it also cleanly applies on
next branch.
Could it be because the begining of the series is names 'v10' while the
end of it is 'RFC v3' as it comes from Daniel's RFC v2 ?
Christophe
Le 12/03/2019 à 23:16, Christophe Leroy a écrit :
> From: Daniel Axtens <dja@axtens.net>
>
> Wire up KASAN. Only outline instrumentation is supported.
>
> The KASAN shadow area is mapped into vmemmap space:
> 0x8000 0400 0000 0000 to 0x8000 0600 0000 0000.
> To do this we require that vmemmap be disabled. (This is the default
> in the kernel config that QorIQ provides for the machine in their
> SDK anyway - they use flat memory.)
>
> Only the kernel linear mapping (0xc000...) is checked. The vmalloc and
> ioremap areas (also in 0x800...) are all mapped to the zero page. As
> with the Book3S hash series, this requires overriding the memory <->
> shadow mapping.
>
> Also, as with both previous 64-bit series, early instrumentation is not
> supported. It would allow us to drop the check_return_arch_not_ready()
> hook in the KASAN core, but it's tricky to get it set up early enough:
> we need it setup before the first call to instrumented code like printk().
> Perhaps in the future.
>
> Only KASAN_MINIMAL works.
>
> Tested on e6500. KVM, kexec and xmon have not been tested.
>
> The test_kasan module fires warnings as expected, except for the
> following tests:
>
> - Expected/by design:
> kasan test: memcg_accounted_kmem_cache allocate memcg accounted object
>
> - Due to only supporting KASAN_MINIMAL:
> kasan test: kasan_stack_oob out-of-bounds on stack
> kasan test: kasan_global_oob out-of-bounds global variable
> kasan test: kasan_alloca_oob_left out-of-bounds to left on alloca
> kasan test: kasan_alloca_oob_right out-of-bounds to right on alloca
> kasan test: use_after_scope_test use-after-scope on int
> kasan test: use_after_scope_test use-after-scope on array
>
> Thanks to those who have done the heavy lifting over the past several
> years:
> - Christophe's 32 bit series: https://lists.ozlabs.org/pipermail/linuxppc-dev/2019-February/185379.html
> - Aneesh's Book3S hash series: https://lwn.net/Articles/655642/
> - Balbir's Book3S radix series: https://patchwork.ozlabs.org/patch/795211/
>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> Cc: Balbir Singh <bsingharora@gmail.com>
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> [- Removed EXPORT_SYMBOL of the static key
> - Fixed most checkpatch problems
> - Replaced kasan_zero_page[] by kasan_early_shadow_page[]
> - Reduced casting mess by using intermediate locals
> - Fixed build failure on pmac32_defconfig]
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/Kconfig.debug | 2 +-
> arch/powerpc/include/asm/kasan.h | 71 ++++++++++++++++++++++++++++
> arch/powerpc/mm/Makefile | 2 +
> arch/powerpc/mm/kasan/Makefile | 1 +
> arch/powerpc/mm/kasan/kasan_init_book3e_64.c | 50 ++++++++++++++++++++
> 6 files changed, 126 insertions(+), 1 deletion(-)
> create mode 100644 arch/powerpc/mm/kasan/kasan_init_book3e_64.c
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index d9364368329b..51ef9fac6c5d 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -174,6 +174,7 @@ config PPC
> select HAVE_ARCH_AUDITSYSCALL
> select HAVE_ARCH_JUMP_LABEL
> select HAVE_ARCH_KASAN if PPC32
> + select HAVE_ARCH_KASAN if PPC_BOOK3E_64 && !SPARSEMEM_VMEMMAP
> select HAVE_ARCH_KGDB
> select HAVE_ARCH_MMAP_RND_BITS
> select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
> diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
> index 61febbbdd02b..fc1f5fa7554e 100644
> --- a/arch/powerpc/Kconfig.debug
> +++ b/arch/powerpc/Kconfig.debug
> @@ -369,5 +369,5 @@ config PPC_FAST_ENDIAN_SWITCH
>
> config KASAN_SHADOW_OFFSET
> hex
> - depends on KASAN
> + depends on KASAN && PPC32
> default 0xe0000000
> diff --git a/arch/powerpc/include/asm/kasan.h b/arch/powerpc/include/asm/kasan.h
> index 296e51c2f066..ae410f0e060d 100644
> --- a/arch/powerpc/include/asm/kasan.h
> +++ b/arch/powerpc/include/asm/kasan.h
> @@ -21,12 +21,15 @@
> #define KASAN_SHADOW_START (KASAN_SHADOW_OFFSET + \
> (PAGE_OFFSET >> KASAN_SHADOW_SCALE_SHIFT))
>
> +#ifdef CONFIG_PPC32
> #define KASAN_SHADOW_OFFSET ASM_CONST(CONFIG_KASAN_SHADOW_OFFSET)
>
> #define KASAN_SHADOW_END 0UL
>
> #define KASAN_SHADOW_SIZE (KASAN_SHADOW_END - KASAN_SHADOW_START)
>
> +#endif /* CONFIG_PPC32 */
> +
> #ifdef CONFIG_KASAN
> void kasan_early_init(void);
> void kasan_mmu_init(void);
> @@ -36,5 +39,73 @@ static inline void kasan_init(void) { }
> static inline void kasan_mmu_init(void) { }
> #endif
>
> +#ifdef CONFIG_PPC_BOOK3E_64
> +#include <asm/pgtable.h>
> +#include <linux/jump_label.h>
> +
> +/*
> + * We don't put this in Kconfig as we only support KASAN_MINIMAL, and
> + * that will be disabled if the symbol is available in Kconfig
> + */
> +#define KASAN_SHADOW_OFFSET ASM_CONST(0x6800040000000000)
> +
> +#define KASAN_SHADOW_SIZE (KERN_VIRT_SIZE >> KASAN_SHADOW_SCALE_SHIFT)
> +
> +extern struct static_key_false powerpc_kasan_enabled_key;
> +extern unsigned char kasan_early_shadow_page[];
> +
> +static inline bool kasan_arch_is_ready_book3e(void)
> +{
> + if (static_branch_likely(&powerpc_kasan_enabled_key))
> + return true;
> + return false;
> +}
> +#define kasan_arch_is_ready kasan_arch_is_ready_book3e
> +
> +static inline void *kasan_mem_to_shadow_book3e(const void *ptr)
> +{
> + unsigned long addr = (unsigned long)ptr;
> +
> + if (addr >= KERN_VIRT_START && addr < KERN_VIRT_START + KERN_VIRT_SIZE)
> + return kasan_early_shadow_page;
> +
> + return (void *)(addr >> KASAN_SHADOW_SCALE_SHIFT) + KASAN_SHADOW_OFFSET;
> +}
> +#define kasan_mem_to_shadow kasan_mem_to_shadow_book3e
> +
> +static inline void *kasan_shadow_to_mem_book3e(const void *shadow_addr)
> +{
> + /*
> + * We map the entire non-linear virtual mapping onto the zero page so if
> + * we are asked to map the zero page back just pick the beginning of that
> + * area.
> + */
> + if (shadow_addr >= (void *)kasan_early_shadow_page &&
> + shadow_addr < (void *)(kasan_early_shadow_page + PAGE_SIZE))
> + return (void *)KERN_VIRT_START;
> +
> + return (void *)(((unsigned long)shadow_addr - KASAN_SHADOW_OFFSET) <<
> + KASAN_SHADOW_SCALE_SHIFT);
> +}
> +#define kasan_shadow_to_mem kasan_shadow_to_mem_book3e
> +
> +static inline bool kasan_addr_has_shadow_book3e(const void *ptr)
> +{
> + unsigned long addr = (unsigned long)ptr;
> +
> + /*
> + * We want to specifically assert that the addresses in the 0x8000...
> + * region have a shadow, otherwise they are considered by the kasan
> + * core to be wild pointers
> + */
> + if (addr >= KERN_VIRT_START && addr < (KERN_VIRT_START + KERN_VIRT_SIZE))
> + return true;
> +
> + return (ptr >= kasan_shadow_to_mem((void *)KASAN_SHADOW_START));
> +}
> +#define kasan_addr_has_shadow kasan_addr_has_shadow_book3e
> +
> +#endif /* CONFIG_PPC_BOOK3E_64 */
> +
> #endif /* __ASSEMBLY */
> #endif
> diff --git a/arch/powerpc/mm/Makefile b/arch/powerpc/mm/Makefile
> index 80382a2d169b..fc49231f807c 100644
> --- a/arch/powerpc/mm/Makefile
> +++ b/arch/powerpc/mm/Makefile
> @@ -8,9 +8,11 @@ ccflags-$(CONFIG_PPC64) := $(NO_MINIMAL_TOC)
> CFLAGS_REMOVE_slb.o = $(CC_FLAGS_FTRACE)
>
> KASAN_SANITIZE_ppc_mmu_32.o := n
> +KASAN_SANITIZE_fsl_booke_mmu.o := n
>
> ifdef CONFIG_KASAN
> CFLAGS_ppc_mmu_32.o += -DDISABLE_BRANCH_PROFILING
> +CFLAGS_fsl_booke_mmu.o += -DDISABLE_BRANCH_PROFILING
> endif
>
> obj-y := fault.o mem.o pgtable.o mmap.o \
> diff --git a/arch/powerpc/mm/kasan/Makefile b/arch/powerpc/mm/kasan/Makefile
> index 6577897673dd..f8f164ad8ade 100644
> --- a/arch/powerpc/mm/kasan/Makefile
> +++ b/arch/powerpc/mm/kasan/Makefile
> @@ -3,3 +3,4 @@
> KASAN_SANITIZE := n
>
> obj-$(CONFIG_PPC32) += kasan_init_32.o
> +obj-$(CONFIG_PPC_BOOK3E_64) += kasan_init_book3e_64.o
> diff --git a/arch/powerpc/mm/kasan/kasan_init_book3e_64.c b/arch/powerpc/mm/kasan/kasan_init_book3e_64.c
> new file mode 100644
> index 000000000000..f116c211d83c
> --- /dev/null
> +++ b/arch/powerpc/mm/kasan/kasan_init_book3e_64.c
> @@ -0,0 +1,50 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#define DISABLE_BRANCH_PROFILING
> +
> +#include <linux/kasan.h>
> +#include <linux/printk.h>
> +#include <linux/memblock.h>
> +#include <linux/sched/task.h>
> +#include <asm/pgalloc.h>
> +
> +DEFINE_STATIC_KEY_FALSE(powerpc_kasan_enabled_key);
> +
> +static void __init kasan_init_region(struct memblock_region *reg)
> +{
> + void *start = __va(reg->base);
> + void *end = __va(reg->base + reg->size);
> + unsigned long k_start, k_end, k_cur;
> +
> + if (start >= end)
> + return;
> +
> + k_start = (unsigned long)kasan_mem_to_shadow(start);
> + k_end = (unsigned long)kasan_mem_to_shadow(end);
> +
> + for (k_cur = k_start; k_cur < k_end; k_cur += PAGE_SIZE) {
> + void *va = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> +
> + map_kernel_page(k_cur, __pa(va), PAGE_KERNEL);
> + }
> + flush_tlb_kernel_range(k_start, k_end);
> +}
> +
> +void __init kasan_init(void)
> +{
> + struct memblock_region *reg;
> +
> + for_each_memblock(memory, reg)
> + kasan_init_region(reg);
> +
> + /* map the zero page RO */
> + map_kernel_page((unsigned long)kasan_early_shadow_page,
> + __pa(kasan_early_shadow_page), PAGE_KERNEL_RO);
> +
> + /* Turn on checking */
> + static_branch_inc(&powerpc_kasan_enabled_key);
> +
> + /* Enable error messages */
> + init_task.kasan_depth = 0;
> + pr_info("KASAN init done (64-bit Book3E)\n");
> +}
>
^ permalink raw reply
* Re: [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug()
From: Laurent Vivier @ 2019-03-13 8:01 UTC (permalink / raw)
To: Christophe Leroy, linux-kernel; +Cc: linuxppc-dev, David Gibson
In-Reply-To: <3db6b64d-02d3-8aa7-80ca-3e469ea743ff@c-s.fr>
On 13/03/2019 07:03, Christophe Leroy wrote:
>
>
> Le 08/03/2019 à 11:54, Laurent Vivier a écrit :
>> resize_hpt_for_hotplug() reports a warning when it cannot
>> resize the hash page table ("Unable to resize hash page
>> table to target order") but in some cases it's not a problem
>> and can make user thinks something has not worked properly.
>>
>> This patch moves the warning to arch_remove_memory() to
>> only report the problem when it is needed.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>> arch/powerpc/include/asm/sparsemem.h | 4 ++--
>> arch/powerpc/mm/hash_utils_64.c | 17 ++++++-----------
>> arch/powerpc/mm/mem.c | 3 ++-
>> arch/powerpc/platforms/pseries/lpar.c | 3 ++-
>> 4 files changed, 12 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/sparsemem.h
>> b/arch/powerpc/include/asm/sparsemem.h
>> index 68da49320592..3192d454a733 100644
>> --- a/arch/powerpc/include/asm/sparsemem.h
>> +++ b/arch/powerpc/include/asm/sparsemem.h
>> @@ -17,9 +17,9 @@ extern int create_section_mapping(unsigned long
>> start, unsigned long end, int ni
>> extern int remove_section_mapping(unsigned long start, unsigned long
>> end);
>> #ifdef CONFIG_PPC_BOOK3S_64
>> -extern void resize_hpt_for_hotplug(unsigned long new_mem_size);
>> +extern int resize_hpt_for_hotplug(unsigned long new_mem_size);
>> #else
>> -static inline void resize_hpt_for_hotplug(unsigned long new_mem_size)
>> { }
>> +static inline int resize_hpt_for_hotplug(unsigned long new_mem_size)
>> { return 0; }
>> #endif
>> #ifdef CONFIG_NUMA
>> diff --git a/arch/powerpc/mm/hash_utils_64.c
>> b/arch/powerpc/mm/hash_utils_64.c
>> index 0cc7fbc3bd1c..40bb2a8326bb 100644
>> --- a/arch/powerpc/mm/hash_utils_64.c
>> +++ b/arch/powerpc/mm/hash_utils_64.c
>> @@ -755,12 +755,12 @@ static unsigned long __init
>> htab_get_table_size(void)
>> }
>> #ifdef CONFIG_MEMORY_HOTPLUG
>> -void resize_hpt_for_hotplug(unsigned long new_mem_size)
>> +int resize_hpt_for_hotplug(unsigned long new_mem_size)
>> {
>> unsigned target_hpt_shift;
>> if (!mmu_hash_ops.resize_hpt)
>> - return;
>> + return 0;
>> target_hpt_shift = htab_shift_for_mem_size(new_mem_size);
>> @@ -773,15 +773,10 @@ void resize_hpt_for_hotplug(unsigned long
>> new_mem_size)
>> * current shift
>> */
>> if ((target_hpt_shift > ppc64_pft_size)
>> - || (target_hpt_shift < (ppc64_pft_size - 1))) {
>> - int rc;
>> -
>> - rc = mmu_hash_ops.resize_hpt(target_hpt_shift);
>> - if (rc && (rc != -ENODEV))
>> - printk(KERN_WARNING
>> - "Unable to resize hash page table to target order
>> %d: %d\n",
>> - target_hpt_shift, rc);
>> - }
>> + || (target_hpt_shift < (ppc64_pft_size - 1)))
>
> The || should go on the line above and the two (target_hpt... should be
> aligned, and the () after the < are superflous.
>
> And indeed, we should (in another patch) rename 'target_hpt_shift' with
> a shorter name, this would avoid multiple lines.
>
> Ref
> https://www.kernel.org/doc/html/latest/process/coding-style.html#naming :
>
> LOCAL variable names should be short, and to the point. If you have some
> random integer loop counter, it should probably be called i. Calling it
> loop_counter is non-productive, if there is no chance of it being
> mis-understood. Similarly, tmp can be just about any type of variable
> that is used to hold a temporary value.
>
> If you are afraid to mix up your local variable names, you have another
> problem, which is called the function-growth-hormone-imbalance syndrome.
> See chapter 6 (Functions).
>
I'm only removing a warning. Do we really need to rewrite all the code
around for that?
Thanks,
Laurent
^ permalink raw reply
* Re: [PATCH v2 15/16] KVM: introduce a KVM_DESTROY_DEVICE ioctl
From: Cédric Le Goater @ 2019-03-13 8:02 UTC (permalink / raw)
To: David Gibson; +Cc: kvm, kvm-ppc, Paul Mackerras, Paolo Bonzini, linuxppc-dev
In-Reply-To: <20190225041507.GS7668@umbus.fritz.box>
On 2/25/19 5:15 AM, David Gibson wrote:
> On Fri, Feb 22, 2019 at 12:28:39PM +0100, Cédric Le Goater wrote:
>> The 'destroy' method is currently used to destroy all devices when the
>> VM is destroyed after the vCPUs have been freed.
>>
>> This new KVM ioctl exposes the same KVM device method. It acts as a
>> software reset of the VM to 'destroy' selected devices when necessary
>> and perform the required cleanups on the vCPUs. Called with the
>> kvm->lock.
>>
>> The 'destroy' method could be improved by returning an error code.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>
> Again, has this been discussed with Paolo and/or other KVM core
> people?
Not yet. Adding Paolo for feedback.
>> ---
>> include/uapi/linux/kvm.h | 7 ++++++
>> virt/kvm/kvm_main.c | 38 +++++++++++++++++++++++++++++++
>> Documentation/virtual/kvm/api.txt | 19 ++++++++++++++++
>> 3 files changed, 64 insertions(+)
>>
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index 52bf74a1616e..d78fafa54274 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1183,6 +1183,11 @@ struct kvm_create_device {
>> __u32 flags; /* in: KVM_CREATE_DEVICE_xxx */
>> };
>>
>> +struct kvm_destroy_device {
>> + __u32 fd; /* in: device handle */
>> + __u32 flags; /* in: unused */
>> +};
>> +
>> struct kvm_device_attr {
>> __u32 flags; /* no flags currently defined */
>> __u32 group; /* device-defined */
>> @@ -1331,6 +1336,8 @@ struct kvm_s390_ucas_mapping {
>> #define KVM_GET_DEVICE_ATTR _IOW(KVMIO, 0xe2, struct kvm_device_attr)
>> #define KVM_HAS_DEVICE_ATTR _IOW(KVMIO, 0xe3, struct kvm_device_attr)
>>
>> +#define KVM_DESTROY_DEVICE _IOWR(KVMIO, 0xf0, struct kvm_destroy_device)
>> +
>> /*
>> * ioctls for vcpu fds
>> */
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 84717d8cb5e4..983d5c37f710 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -3026,6 +3026,30 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
>> return 0;
>> }
>>
>> +static int kvm_ioctl_destroy_device(struct kvm *kvm,
>> + struct kvm_destroy_device *dd)
>> +{
>> + struct fd f;
>> + struct kvm_device *dev;
>> +
>> + f = fdget(dd->fd);
>> + if (!f.file)
>> + return -EBADF;
>> +
>> + dev = kvm_device_from_filp(f.file);
>> + fdput(f);
>> +
>> + if (!dev)
>> + return -ENODEV;
>
> Don't you need to verify that the device belongs to this KVM instance?
ah yes. I should at least check : dev->kvm == kvm
>> + mutex_lock(&kvm->lock);
>> + list_del(&dev->vm_node);
>> + dev->ops->destroy(dev);
>> + mutex_unlock(&kvm->lock);
And there, I should problably drop a ref count on the VM. I am not sure
of that.
Thanks,
C.
>> + return 0;
>> +}
>> +
>> static long kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg)
>> {
>> switch (arg) {
>> @@ -3270,6 +3294,20 @@ static long kvm_vm_ioctl(struct file *filp,
>> r = 0;
>> break;
>> }
>> + case KVM_DESTROY_DEVICE: {
>> + struct kvm_destroy_device dd;
>> +
>> + r = -EFAULT;
>> + if (copy_from_user(&dd, argp, sizeof(dd)))
>> + goto out;
>> +
>> + r = kvm_ioctl_destroy_device(kvm, &dd);
>> + if (r)
>> + goto out;
>> +
>> + r = 0;
>> + break;
>> + }
>> case KVM_CHECK_EXTENSION:
>> r = kvm_vm_ioctl_check_extension_generic(kvm, arg);
>> break;
>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>> index 1db1435769b4..c2ba18279298 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -3857,6 +3857,25 @@ number of valid entries in the 'entries' array, which is then filled.
>> 'index' and 'flags' fields in 'struct kvm_cpuid_entry2' are currently reserved,
>> userspace should not expect to get any particular value there.
>>
>> +4.119 KVM_DESTROY_DEVICE
>> +
>> +Capability: KVM_CAP_DEVICE_CTRL
>> +Type: vm ioctl
>> +Parameters: struct kvm_destroy_device (in)
>> +Returns: 0 on success, -1 on error
>> +Errors:
>> + ENODEV: The device type is unknown or unsupported
>> +
>> + Other error conditions may be defined by individual device types or
>> + have their standard meanings.
>> +
>> +Destroys an emulated device in the kernel.
>> +
>> +struct kvm_destroy_device {
>> + __u32 fd; /* in: device handle */
>> + __u32 flags; /* unused */
>> +};
>> +
>> 5. The kvm_run structure
>> ------------------------
>>
>
^ permalink raw reply
* Re: [PATCH v5 02/10] powerpc/powernv/idle: Restore AMR/UAMOR/AMOR after idle
From: Akshay Adiga @ 2019-03-13 8:16 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, npiggin
In-Reply-To: <20190308011619.22402-2-mpe@ellerman.id.au>
On Fri, Mar 08, 2019 at 12:16:11PM +1100, Michael Ellerman wrote:
> In order to implement KUAP (Kernel Userspace Access Protection) on
> Power9 we will be using the AMR, and therefore indirectly the
> UAMOR/AMOR.
>
> So save/restore these regs in the idle code.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> v5: Unchanged.
> v4: New.
>
> arch/powerpc/kernel/idle_book3s.S | 27 +++++++++++++++++++++++----
> 1 file changed, 23 insertions(+), 4 deletions(-)
Opps.. i posted a comment on the v4.
It would be good if we can make AMOR/UAMOR/AMR save-restore
code power9 only.
^ permalink raw reply
* Re: [PATCH v2 16/16] KVM: PPC: Book3S HV: XIVE: clear the vCPU interrupt presenters
From: Cédric Le Goater @ 2019-03-13 8:17 UTC (permalink / raw)
To: David Gibson; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <20190225041858.GT7668@umbus.fritz.box>
On 2/25/19 5:18 AM, David Gibson wrote:
> On Fri, Feb 22, 2019 at 12:28:40PM +0100, Cédric Le Goater wrote:
>> When the VM boots, the CAS negotiation process determines which
>> interrupt mode to use and invokes a machine reset. At that time, the
>> previous KVM interrupt device is 'destroyed' before the chosen one is
>> created. Upon destruction, the vCPU interrupt presenters using the KVM
>> device should be cleared first, the machine will reconnect them later
>> to the new device after it is created.
>>
>> When using the KVM device, there is still a race window with the early
>> checks in kvmppc_native_connect_vcpu(). Yet to be fixed.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> arch/powerpc/kvm/book3s_xics.c | 19 +++++++++++++
>> arch/powerpc/kvm/book3s_xive.c | 39 +++++++++++++++++++++++++--
>> arch/powerpc/kvm/book3s_xive_native.c | 16 +++++++++++
>> 3 files changed, 72 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/kvm/book3s_xics.c b/arch/powerpc/kvm/book3s_xics.c
>> index f27ee57ab46e..81cdabf4295f 100644
>> --- a/arch/powerpc/kvm/book3s_xics.c
>> +++ b/arch/powerpc/kvm/book3s_xics.c
>> @@ -1342,6 +1342,25 @@ static void kvmppc_xics_free(struct kvm_device *dev)
>> struct kvmppc_xics *xics = dev->private;
>> int i;
>> struct kvm *kvm = xics->kvm;
>> + struct kvm_vcpu *vcpu;
>> +
>> + /*
>> + * When destroying the VM, the vCPUs are destroyed first and
>> + * the vCPU list should be empty. If this is not the case,
>> + * then we are simply destroying the device and we should
>> + * clean up the vCPU interrupt presenters first.
>> + */
>> + if (atomic_read(&kvm->online_vcpus) != 0) {
>> + /*
>> + * call kick_all_cpus_sync() to ensure that all CPUs
>> + * have executed any pending interrupts
>> + */
>> + if (is_kvmppc_hv_enabled(kvm))
>> + kick_all_cpus_sync();
>> +
>> + kvm_for_each_vcpu(i, vcpu, kvm)
>> + kvmppc_xics_free_icp(vcpu);
>> + }
>>
>> debugfs_remove(xics->dentry);
>>
>> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
>> index 7a14512b8944..0a1c11d6881c 100644
>> --- a/arch/powerpc/kvm/book3s_xive.c
>> +++ b/arch/powerpc/kvm/book3s_xive.c
>> @@ -1105,11 +1105,19 @@ void kvmppc_xive_disable_vcpu_interrupts(struct kvm_vcpu *vcpu)
>> void kvmppc_xive_cleanup_vcpu(struct kvm_vcpu *vcpu)
>> {
>> struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
>> - struct kvmppc_xive *xive = xc->xive;
>> + struct kvmppc_xive *xive;
>> int i;
>>
>> + if (!kvmppc_xics_enabled(vcpu))
>
> This should be kvmppc_xive_enabled(), no?
This is the KVM XICS-on-XIVE device and its IRQ type is KVMPPC_IRQ_XICS.
So this is correct :/
May be we should introduce a KVMPPC_IRQ_XICS_ON_XIVE macro to clarify.
>
>> + return;
>> +
>> + if (!xc)
>> + return;
>> +
>> pr_devel("cleanup_vcpu(cpu=%d)\n", xc->server_num);
>>
>> + xive = xc->xive;
>> +
>> /* Ensure no interrupt is still routed to that VP */
>> xc->valid = false;
>> kvmppc_xive_disable_vcpu_interrupts(vcpu);
>> @@ -1146,6 +1154,10 @@ void kvmppc_xive_cleanup_vcpu(struct kvm_vcpu *vcpu)
>> }
>> /* Free the VP */
>> kfree(xc);
>> +
>> + /* Cleanup the vcpu */
>> + vcpu->arch.irq_type = KVMPPC_IRQ_DEFAULT;
>> + vcpu->arch.xive_vcpu = NULL;
>> }
>>
>> int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
>> @@ -1163,7 +1175,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
>> }
>> if (xive->kvm != vcpu->kvm)
>> return -EPERM;
>> - if (vcpu->arch.irq_type)
>> + if (vcpu->arch.irq_type != KVMPPC_IRQ_DEFAULT)
>> return -EBUSY;
>> if (kvmppc_xive_find_server(vcpu->kvm, cpu)) {
>> pr_devel("Duplicate !\n");
>> @@ -1833,8 +1845,31 @@ static void kvmppc_xive_free(struct kvm_device *dev)
>> {
>> struct kvmppc_xive *xive = dev->private;
>> struct kvm *kvm = xive->kvm;
>> + struct kvm_vcpu *vcpu;
>> int i;
>>
>> + /*
>> + * When destroying the VM, the vCPUs are destroyed first and
>> + * the vCPU list should be empty. If this is not the case,
>> + * then we are simply destroying the device and we should
>> + * clean up the vCPU interrupt presenters first.
>> + */
>> + if (atomic_read(&kvm->online_vcpus) != 0) {
>> + /*
>> + * call kick_all_cpus_sync() to ensure that all CPUs
>> + * have executed any pending interrupts
>> + */
>> + if (is_kvmppc_hv_enabled(kvm))
>> + kick_all_cpus_sync();
>> +
>> + /*
>> + * TODO: There is still a race window with the early
>> + * checks in kvmppc_native_connect_vcpu()
>> + */
>> + kvm_for_each_vcpu(i, vcpu, kvm)
>> + kvmppc_xive_cleanup_vcpu(vcpu);
>> + }
>> +
>> debugfs_remove(xive->dentry);
>>
>> if (kvm)
>> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
>> index bf60870144f1..c0655164d9af 100644
>> --- a/arch/powerpc/kvm/book3s_xive_native.c
>> +++ b/arch/powerpc/kvm/book3s_xive_native.c
>> @@ -909,8 +909,24 @@ static void kvmppc_xive_native_free(struct kvm_device *dev)
>> {
>> struct kvmppc_xive *xive = dev->private;
>> struct kvm *kvm = xive->kvm;
>> + struct kvm_vcpu *vcpu;
>> int i;
>>
>> + /*
>> + * When destroying the VM, the vCPUs are destroyed first and
>> + * the vCPU list should be empty. If this is not the case,
>> + * then we are simply destroying the device and we should
>> + * clean up the vCPU interrupt presenters first.
>> + */
>> + if (atomic_read(&kvm->online_vcpus) != 0) {
>> + /*
>> + * TODO: There is still a race window with the early
>> + * checks in kvmppc_xive_native_connect_vcpu()
>> + */
>> + kvm_for_each_vcpu(i, vcpu, kvm)
>> + kvmppc_xive_native_cleanup_vcpu(vcpu);
>> + }
>> +
>> debugfs_remove(xive->dentry);
>>
>> pr_devel("Destroying xive native device\n");
>
^ permalink raw reply
* Re: [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug()
From: Christophe Leroy @ 2019-03-13 8:28 UTC (permalink / raw)
To: Laurent Vivier, linux-kernel; +Cc: linuxppc-dev, David Gibson
In-Reply-To: <1dfe1a2e-f6f9-2d5d-a38e-ba4517794bad@redhat.com>
Le 13/03/2019 à 09:01, Laurent Vivier a écrit :
> On 13/03/2019 07:03, Christophe Leroy wrote:
>>
>>
>> Le 08/03/2019 à 11:54, Laurent Vivier a écrit :
>>> resize_hpt_for_hotplug() reports a warning when it cannot
>>> resize the hash page table ("Unable to resize hash page
>>> table to target order") but in some cases it's not a problem
>>> and can make user thinks something has not worked properly.
>>>
>>> This patch moves the warning to arch_remove_memory() to
>>> only report the problem when it is needed.
>>>
>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>> ---
>>> arch/powerpc/include/asm/sparsemem.h | 4 ++--
>>> arch/powerpc/mm/hash_utils_64.c | 17 ++++++-----------
>>> arch/powerpc/mm/mem.c | 3 ++-
>>> arch/powerpc/platforms/pseries/lpar.c | 3 ++-
>>> 4 files changed, 12 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/sparsemem.h
>>> b/arch/powerpc/include/asm/sparsemem.h
>>> index 68da49320592..3192d454a733 100644
>>> --- a/arch/powerpc/include/asm/sparsemem.h
>>> +++ b/arch/powerpc/include/asm/sparsemem.h
>>> @@ -17,9 +17,9 @@ extern int create_section_mapping(unsigned long
>>> start, unsigned long end, int ni
>>> extern int remove_section_mapping(unsigned long start, unsigned long
>>> end);
>>> #ifdef CONFIG_PPC_BOOK3S_64
>>> -extern void resize_hpt_for_hotplug(unsigned long new_mem_size);
>>> +extern int resize_hpt_for_hotplug(unsigned long new_mem_size);
>>> #else
>>> -static inline void resize_hpt_for_hotplug(unsigned long new_mem_size)
>>> { }
>>> +static inline int resize_hpt_for_hotplug(unsigned long new_mem_size)
>>> { return 0; }
>>> #endif
>>> #ifdef CONFIG_NUMA
>>> diff --git a/arch/powerpc/mm/hash_utils_64.c
>>> b/arch/powerpc/mm/hash_utils_64.c
>>> index 0cc7fbc3bd1c..40bb2a8326bb 100644
>>> --- a/arch/powerpc/mm/hash_utils_64.c
>>> +++ b/arch/powerpc/mm/hash_utils_64.c
>>> @@ -755,12 +755,12 @@ static unsigned long __init
>>> htab_get_table_size(void)
>>> }
>>> #ifdef CONFIG_MEMORY_HOTPLUG
>>> -void resize_hpt_for_hotplug(unsigned long new_mem_size)
>>> +int resize_hpt_for_hotplug(unsigned long new_mem_size)
>>> {
>>> unsigned target_hpt_shift;
>>> if (!mmu_hash_ops.resize_hpt)
>>> - return;
>>> + return 0;
>>> target_hpt_shift = htab_shift_for_mem_size(new_mem_size);
>>> @@ -773,15 +773,10 @@ void resize_hpt_for_hotplug(unsigned long
>>> new_mem_size)
>>> * current shift
>>> */
>>> if ((target_hpt_shift > ppc64_pft_size)
>>> - || (target_hpt_shift < (ppc64_pft_size - 1))) {
>>> - int rc;
>>> -
>>> - rc = mmu_hash_ops.resize_hpt(target_hpt_shift);
>>> - if (rc && (rc != -ENODEV))
>>> - printk(KERN_WARNING
>>> - "Unable to resize hash page table to target order
>>> %d: %d\n",
>>> - target_hpt_shift, rc);
>>> - }
>>> + || (target_hpt_shift < (ppc64_pft_size - 1)))
>>
>> The || should go on the line above and the two (target_hpt... should be
>> aligned, and the () after the < are superflous.
>>
>> And indeed, we should (in another patch) rename 'target_hpt_shift' with
>> a shorter name, this would avoid multiple lines.
>>
>> Ref
>> https://www.kernel.org/doc/html/latest/process/coding-style.html#naming :
>>
>> LOCAL variable names should be short, and to the point. If you have some
>> random integer loop counter, it should probably be called i. Calling it
>> loop_counter is non-productive, if there is no chance of it being
>> mis-understood. Similarly, tmp can be just about any type of variable
>> that is used to hold a temporary value.
>>
>> If you are afraid to mix up your local variable names, you have another
>> problem, which is called the function-growth-hormone-imbalance syndrome.
>> See chapter 6 (Functions).
>>
>
> I'm only removing a warning. Do we really need to rewrite all the code
> around for that?
No, and that's the reason why I said it could be done in another
(future) patch.
Anyway, your patch should be clean regarding checkpatch
See https://patchwork.ozlabs.org/patch/1052984/
And
https://openpower.xyz/job/snowpatch/job/snowpatch-linux-checkpatch/3298//artifact/linux/checkpatch.log
CHECK:AVOID_EXTERNS: extern prototypes should be avoided in .h files
#31: FILE: arch/powerpc/include/asm/sparsemem.h:20:
+extern int resize_hpt_for_hotplug(unsigned long new_mem_size);
CHECK:LOGICAL_CONTINUATIONS: Logical continuations should be on the
previous line
#70: FILE: arch/powerpc/mm/hash_utils_64.c:776:
if ((target_hpt_shift > ppc64_pft_size)
+ || (target_hpt_shift < (ppc64_pft_size - 1)))
total: 0 errors, 0 warnings, 2 checks, 60 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or
--fix-inplace.
the.patch has style problems, please review.
NOTE: Ignored message types: ARCH_INCLUDE_LINUX BIT_MACRO
COMPARISON_TO_NULL DT_SPLIT_BINDING_PATCH EMAIL_SUBJECT
FILE_PATH_CHANGES GLOBAL_INITIALISERS LINE_SPACING MULTIPLE_ASSIGNMENTS
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
Christophe
>
> Thanks,
> Laurent
>
^ permalink raw reply
* Re: [PATCH RFC v3 18/18] powerpc: KASAN for 64bit Book3E
From: Christophe Leroy @ 2019-03-13 8:30 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Nicholas Piggin, Aneesh Kumar K.V, Andrey Ryabinin,
Alexander Potapenko, Dmitry Vyukov, Daniel Axtens
Cc: linux-mm, linuxppc-dev, linux-kernel, kasan-dev
In-Reply-To: <62ba1b83-cc75-82a4-a1c0-5a54464c963b@c-s.fr>
Any, the build is clean, see
http://kisskb.ellerman.id.au/kisskb/head/3e97aba429c769bd99ccd8d6f16eda98f7d378a7/
Only s390 defconfig and powerpc randconfig failed for unrelated reasons.
Christophe
Le 13/03/2019 à 08:02, Christophe Leroy a écrit :
> Why does snowpatch reports not being able to apply it to any branch ?
>
> I built the serie on top of merge branch, but it also cleanly applies on
> next branch.
>
> Could it be because the begining of the series is names 'v10' while the
> end of it is 'RFC v3' as it comes from Daniel's RFC v2 ?
>
> Christophe
>
> Le 12/03/2019 à 23:16, Christophe Leroy a écrit :
>> From: Daniel Axtens <dja@axtens.net>
>>
>> Wire up KASAN. Only outline instrumentation is supported.
>>
>> The KASAN shadow area is mapped into vmemmap space:
>> 0x8000 0400 0000 0000 to 0x8000 0600 0000 0000.
>> To do this we require that vmemmap be disabled. (This is the default
>> in the kernel config that QorIQ provides for the machine in their
>> SDK anyway - they use flat memory.)
>>
>> Only the kernel linear mapping (0xc000...) is checked. The vmalloc and
>> ioremap areas (also in 0x800...) are all mapped to the zero page. As
>> with the Book3S hash series, this requires overriding the memory <->
>> shadow mapping.
>>
>> Also, as with both previous 64-bit series, early instrumentation is not
>> supported. It would allow us to drop the check_return_arch_not_ready()
>> hook in the KASAN core, but it's tricky to get it set up early enough:
>> we need it setup before the first call to instrumented code like
>> printk().
>> Perhaps in the future.
>>
>> Only KASAN_MINIMAL works.
>>
>> Tested on e6500. KVM, kexec and xmon have not been tested.
>>
>> The test_kasan module fires warnings as expected, except for the
>> following tests:
>>
>> - Expected/by design:
>> kasan test: memcg_accounted_kmem_cache allocate memcg accounted object
>>
>> - Due to only supporting KASAN_MINIMAL:
>> kasan test: kasan_stack_oob out-of-bounds on stack
>> kasan test: kasan_global_oob out-of-bounds global variable
>> kasan test: kasan_alloca_oob_left out-of-bounds to left on alloca
>> kasan test: kasan_alloca_oob_right out-of-bounds to right on alloca
>> kasan test: use_after_scope_test use-after-scope on int
>> kasan test: use_after_scope_test use-after-scope on array
>>
>> Thanks to those who have done the heavy lifting over the past several
>> years:
>> - Christophe's 32 bit series:
>> https://lists.ozlabs.org/pipermail/linuxppc-dev/2019-February/185379.html
>> - Aneesh's Book3S hash series: https://lwn.net/Articles/655642/
>> - Balbir's Book3S radix series:
>> https://patchwork.ozlabs.org/patch/795211/
>>
>> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
>> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> Cc: Balbir Singh <bsingharora@gmail.com>
>> Signed-off-by: Daniel Axtens <dja@axtens.net>
>> [- Removed EXPORT_SYMBOL of the static key
>> - Fixed most checkpatch problems
>> - Replaced kasan_zero_page[] by kasan_early_shadow_page[]
>> - Reduced casting mess by using intermediate locals
>> - Fixed build failure on pmac32_defconfig]
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>> arch/powerpc/Kconfig | 1 +
>> arch/powerpc/Kconfig.debug | 2 +-
>> arch/powerpc/include/asm/kasan.h | 71
>> ++++++++++++++++++++++++++++
>> arch/powerpc/mm/Makefile | 2 +
>> arch/powerpc/mm/kasan/Makefile | 1 +
>> arch/powerpc/mm/kasan/kasan_init_book3e_64.c | 50 ++++++++++++++++++++
>> 6 files changed, 126 insertions(+), 1 deletion(-)
>> create mode 100644 arch/powerpc/mm/kasan/kasan_init_book3e_64.c
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index d9364368329b..51ef9fac6c5d 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -174,6 +174,7 @@ config PPC
>> select HAVE_ARCH_AUDITSYSCALL
>> select HAVE_ARCH_JUMP_LABEL
>> select HAVE_ARCH_KASAN if PPC32
>> + select HAVE_ARCH_KASAN if PPC_BOOK3E_64 &&
>> !SPARSEMEM_VMEMMAP
>> select HAVE_ARCH_KGDB
>> select HAVE_ARCH_MMAP_RND_BITS
>> select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
>> diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
>> index 61febbbdd02b..fc1f5fa7554e 100644
>> --- a/arch/powerpc/Kconfig.debug
>> +++ b/arch/powerpc/Kconfig.debug
>> @@ -369,5 +369,5 @@ config PPC_FAST_ENDIAN_SWITCH
>> config KASAN_SHADOW_OFFSET
>> hex
>> - depends on KASAN
>> + depends on KASAN && PPC32
>> default 0xe0000000
>> diff --git a/arch/powerpc/include/asm/kasan.h
>> b/arch/powerpc/include/asm/kasan.h
>> index 296e51c2f066..ae410f0e060d 100644
>> --- a/arch/powerpc/include/asm/kasan.h
>> +++ b/arch/powerpc/include/asm/kasan.h
>> @@ -21,12 +21,15 @@
>> #define KASAN_SHADOW_START (KASAN_SHADOW_OFFSET + \
>> (PAGE_OFFSET >> KASAN_SHADOW_SCALE_SHIFT))
>> +#ifdef CONFIG_PPC32
>> #define KASAN_SHADOW_OFFSET ASM_CONST(CONFIG_KASAN_SHADOW_OFFSET)
>> #define KASAN_SHADOW_END 0UL
>> #define KASAN_SHADOW_SIZE (KASAN_SHADOW_END - KASAN_SHADOW_START)
>> +#endif /* CONFIG_PPC32 */
>> +
>> #ifdef CONFIG_KASAN
>> void kasan_early_init(void);
>> void kasan_mmu_init(void);
>> @@ -36,5 +39,73 @@ static inline void kasan_init(void) { }
>> static inline void kasan_mmu_init(void) { }
>> #endif
>> +#ifdef CONFIG_PPC_BOOK3E_64
>> +#include <asm/pgtable.h>
>> +#include <linux/jump_label.h>
>> +
>> +/*
>> + * We don't put this in Kconfig as we only support KASAN_MINIMAL, and
>> + * that will be disabled if the symbol is available in Kconfig
>> + */
>> +#define KASAN_SHADOW_OFFSET ASM_CONST(0x6800040000000000)
>> +
>> +#define KASAN_SHADOW_SIZE (KERN_VIRT_SIZE >>
>> KASAN_SHADOW_SCALE_SHIFT)
>> +
>> +extern struct static_key_false powerpc_kasan_enabled_key;
>> +extern unsigned char kasan_early_shadow_page[];
>> +
>> +static inline bool kasan_arch_is_ready_book3e(void)
>> +{
>> + if (static_branch_likely(&powerpc_kasan_enabled_key))
>> + return true;
>> + return false;
>> +}
>> +#define kasan_arch_is_ready kasan_arch_is_ready_book3e
>> +
>> +static inline void *kasan_mem_to_shadow_book3e(const void *ptr)
>> +{
>> + unsigned long addr = (unsigned long)ptr;
>> +
>> + if (addr >= KERN_VIRT_START && addr < KERN_VIRT_START +
>> KERN_VIRT_SIZE)
>> + return kasan_early_shadow_page;
>> +
>> + return (void *)(addr >> KASAN_SHADOW_SCALE_SHIFT) +
>> KASAN_SHADOW_OFFSET;
>> +}
>> +#define kasan_mem_to_shadow kasan_mem_to_shadow_book3e
>> +
>> +static inline void *kasan_shadow_to_mem_book3e(const void *shadow_addr)
>> +{
>> + /*
>> + * We map the entire non-linear virtual mapping onto the zero
>> page so if
>> + * we are asked to map the zero page back just pick the beginning
>> of that
>> + * area.
>> + */
>> + if (shadow_addr >= (void *)kasan_early_shadow_page &&
>> + shadow_addr < (void *)(kasan_early_shadow_page + PAGE_SIZE))
>> + return (void *)KERN_VIRT_START;
>> +
>> + return (void *)(((unsigned long)shadow_addr -
>> KASAN_SHADOW_OFFSET) <<
>> + KASAN_SHADOW_SCALE_SHIFT);
>> +}
>> +#define kasan_shadow_to_mem kasan_shadow_to_mem_book3e
>> +
>> +static inline bool kasan_addr_has_shadow_book3e(const void *ptr)
>> +{
>> + unsigned long addr = (unsigned long)ptr;
>> +
>> + /*
>> + * We want to specifically assert that the addresses in the
>> 0x8000...
>> + * region have a shadow, otherwise they are considered by the kasan
>> + * core to be wild pointers
>> + */
>> + if (addr >= KERN_VIRT_START && addr < (KERN_VIRT_START +
>> KERN_VIRT_SIZE))
>> + return true;
>> +
>> + return (ptr >= kasan_shadow_to_mem((void *)KASAN_SHADOW_START));
>> +}
>> +#define kasan_addr_has_shadow kasan_addr_has_shadow_book3e
>> +
>> +#endif /* CONFIG_PPC_BOOK3E_64 */
>> +
>> #endif /* __ASSEMBLY */
>> #endif
>> diff --git a/arch/powerpc/mm/Makefile b/arch/powerpc/mm/Makefile
>> index 80382a2d169b..fc49231f807c 100644
>> --- a/arch/powerpc/mm/Makefile
>> +++ b/arch/powerpc/mm/Makefile
>> @@ -8,9 +8,11 @@ ccflags-$(CONFIG_PPC64) := $(NO_MINIMAL_TOC)
>> CFLAGS_REMOVE_slb.o = $(CC_FLAGS_FTRACE)
>> KASAN_SANITIZE_ppc_mmu_32.o := n
>> +KASAN_SANITIZE_fsl_booke_mmu.o := n
>> ifdef CONFIG_KASAN
>> CFLAGS_ppc_mmu_32.o += -DDISABLE_BRANCH_PROFILING
>> +CFLAGS_fsl_booke_mmu.o += -DDISABLE_BRANCH_PROFILING
>> endif
>> obj-y := fault.o mem.o pgtable.o mmap.o \
>> diff --git a/arch/powerpc/mm/kasan/Makefile
>> b/arch/powerpc/mm/kasan/Makefile
>> index 6577897673dd..f8f164ad8ade 100644
>> --- a/arch/powerpc/mm/kasan/Makefile
>> +++ b/arch/powerpc/mm/kasan/Makefile
>> @@ -3,3 +3,4 @@
>> KASAN_SANITIZE := n
>> obj-$(CONFIG_PPC32) += kasan_init_32.o
>> +obj-$(CONFIG_PPC_BOOK3E_64) += kasan_init_book3e_64.o
>> diff --git a/arch/powerpc/mm/kasan/kasan_init_book3e_64.c
>> b/arch/powerpc/mm/kasan/kasan_init_book3e_64.c
>> new file mode 100644
>> index 000000000000..f116c211d83c
>> --- /dev/null
>> +++ b/arch/powerpc/mm/kasan/kasan_init_book3e_64.c
>> @@ -0,0 +1,50 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#define DISABLE_BRANCH_PROFILING
>> +
>> +#include <linux/kasan.h>
>> +#include <linux/printk.h>
>> +#include <linux/memblock.h>
>> +#include <linux/sched/task.h>
>> +#include <asm/pgalloc.h>
>> +
>> +DEFINE_STATIC_KEY_FALSE(powerpc_kasan_enabled_key);
>> +
>> +static void __init kasan_init_region(struct memblock_region *reg)
>> +{
>> + void *start = __va(reg->base);
>> + void *end = __va(reg->base + reg->size);
>> + unsigned long k_start, k_end, k_cur;
>> +
>> + if (start >= end)
>> + return;
>> +
>> + k_start = (unsigned long)kasan_mem_to_shadow(start);
>> + k_end = (unsigned long)kasan_mem_to_shadow(end);
>> +
>> + for (k_cur = k_start; k_cur < k_end; k_cur += PAGE_SIZE) {
>> + void *va = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
>> +
>> + map_kernel_page(k_cur, __pa(va), PAGE_KERNEL);
>> + }
>> + flush_tlb_kernel_range(k_start, k_end);
>> +}
>> +
>> +void __init kasan_init(void)
>> +{
>> + struct memblock_region *reg;
>> +
>> + for_each_memblock(memory, reg)
>> + kasan_init_region(reg);
>> +
>> + /* map the zero page RO */
>> + map_kernel_page((unsigned long)kasan_early_shadow_page,
>> + __pa(kasan_early_shadow_page), PAGE_KERNEL_RO);
>> +
>> + /* Turn on checking */
>> + static_branch_inc(&powerpc_kasan_enabled_key);
>> +
>> + /* Enable error messages */
>> + init_task.kasan_depth = 0;
>> + pr_info("KASAN init done (64-bit Book3E)\n");
>> +}
>>
^ permalink raw reply
* Re: [PATCH 4/5] ocxl: Remove superfluous 'extern' from headers
From: Greg Kurz @ 2019-03-13 8:28 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, Andrew Donnellan,
Alastair D'Silva, Frederic Barrat, linuxppc-dev
In-Reply-To: <20190313040702.14276-5-alastair@au1.ibm.com>
On Wed, 13 Mar 2019 15:07:00 +1100
"Alastair D'Silva" <alastair@au1.ibm.com> wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> The 'extern' keyword adds no value here.
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> drivers/misc/ocxl/ocxl_internal.h | 54 +++++++++++++++----------------
> include/misc/ocxl.h | 36 ++++++++++-----------
> 2 files changed, 44 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/misc/ocxl/ocxl_internal.h b/drivers/misc/ocxl/ocxl_internal.h
> index a32f2151029f..321b29e77f45 100644
> --- a/drivers/misc/ocxl/ocxl_internal.h
> +++ b/drivers/misc/ocxl/ocxl_internal.h
> @@ -16,7 +16,6 @@
>
> extern struct pci_driver ocxl_pci_driver;
>
> -
> struct ocxl_fn {
> struct device dev;
> int bar_used[3];
> @@ -92,41 +91,40 @@ struct ocxl_process_element {
> __be32 software_state;
> };
>
> +struct ocxl_afu *ocxl_afu_get(struct ocxl_afu *afu);
> +void ocxl_afu_put(struct ocxl_afu *afu);
>
> -extern struct ocxl_afu *ocxl_afu_get(struct ocxl_afu *afu);
> -extern void ocxl_afu_put(struct ocxl_afu *afu);
> -
> -extern int ocxl_create_cdev(struct ocxl_afu *afu);
> -extern void ocxl_destroy_cdev(struct ocxl_afu *afu);
> -extern int ocxl_register_afu(struct ocxl_afu *afu);
> -extern void ocxl_unregister_afu(struct ocxl_afu *afu);
> +int ocxl_create_cdev(struct ocxl_afu *afu);
> +void ocxl_destroy_cdev(struct ocxl_afu *afu);
> +int ocxl_register_afu(struct ocxl_afu *afu);
> +void ocxl_unregister_afu(struct ocxl_afu *afu);
>
> -extern int ocxl_file_init(void);
> -extern void ocxl_file_exit(void);
> +int ocxl_file_init(void);
> +void ocxl_file_exit(void);
>
> -extern int ocxl_pasid_afu_alloc(struct ocxl_fn *fn, u32 size);
> -extern void ocxl_pasid_afu_free(struct ocxl_fn *fn, u32 start, u32 size);
> -extern int ocxl_actag_afu_alloc(struct ocxl_fn *fn, u32 size);
> -extern void ocxl_actag_afu_free(struct ocxl_fn *fn, u32 start, u32 size);
> +int ocxl_pasid_afu_alloc(struct ocxl_fn *fn, u32 size);
> +void ocxl_pasid_afu_free(struct ocxl_fn *fn, u32 start, u32 size);
> +int ocxl_actag_afu_alloc(struct ocxl_fn *fn, u32 size);
> +void ocxl_actag_afu_free(struct ocxl_fn *fn, u32 start, u32 size);
>
> -extern struct ocxl_context *ocxl_context_alloc(void);
> -extern int ocxl_context_init(struct ocxl_context *ctx, struct ocxl_afu *afu,
> +struct ocxl_context *ocxl_context_alloc(void);
> +int ocxl_context_init(struct ocxl_context *ctx, struct ocxl_afu *afu,
> struct address_space *mapping);
> -extern int ocxl_context_attach(struct ocxl_context *ctx, u64 amr);
> -extern int ocxl_context_mmap(struct ocxl_context *ctx,
> +int ocxl_context_attach(struct ocxl_context *ctx, u64 amr);
> +int ocxl_context_mmap(struct ocxl_context *ctx,
> struct vm_area_struct *vma);
> -extern int ocxl_context_detach(struct ocxl_context *ctx);
> -extern void ocxl_context_detach_all(struct ocxl_afu *afu);
> -extern void ocxl_context_free(struct ocxl_context *ctx);
> +int ocxl_context_detach(struct ocxl_context *ctx);
> +void ocxl_context_detach_all(struct ocxl_afu *afu);
> +void ocxl_context_free(struct ocxl_context *ctx);
>
> -extern int ocxl_sysfs_add_afu(struct ocxl_afu *afu);
> -extern void ocxl_sysfs_remove_afu(struct ocxl_afu *afu);
> +int ocxl_sysfs_add_afu(struct ocxl_afu *afu);
> +void ocxl_sysfs_remove_afu(struct ocxl_afu *afu);
>
> -extern int ocxl_afu_irq_alloc(struct ocxl_context *ctx, u64 *irq_offset);
> -extern int ocxl_afu_irq_free(struct ocxl_context *ctx, u64 irq_offset);
> -extern void ocxl_afu_irq_free_all(struct ocxl_context *ctx);
> -extern int ocxl_afu_irq_set_fd(struct ocxl_context *ctx, u64 irq_offset,
> +int ocxl_afu_irq_alloc(struct ocxl_context *ctx, u64 *irq_offset);
> +int ocxl_afu_irq_free(struct ocxl_context *ctx, u64 irq_offset);
> +void ocxl_afu_irq_free_all(struct ocxl_context *ctx);
> +int ocxl_afu_irq_set_fd(struct ocxl_context *ctx, u64 irq_offset,
> int eventfd);
> -extern u64 ocxl_afu_irq_get_addr(struct ocxl_context *ctx, u64 irq_offset);
> +u64 ocxl_afu_irq_get_addr(struct ocxl_context *ctx, u64 irq_offset);
>
> #endif /* _OCXL_INTERNAL_H_ */
> diff --git a/include/misc/ocxl.h b/include/misc/ocxl.h
> index 9ff6ddc28e22..4544573cc93c 100644
> --- a/include/misc/ocxl.h
> +++ b/include/misc/ocxl.h
> @@ -53,7 +53,7 @@ struct ocxl_fn_config {
> * Read the configuration space of a function and fill in a
> * ocxl_fn_config structure with all the function details
> */
> -extern int ocxl_config_read_function(struct pci_dev *dev,
> +int ocxl_config_read_function(struct pci_dev *dev,
> struct ocxl_fn_config *fn);
>
> /*
> @@ -62,14 +62,14 @@ extern int ocxl_config_read_function(struct pci_dev *dev,
> * AFU indexes can be sparse, so a driver should check all indexes up
> * to the maximum found in the function description
> */
> -extern int ocxl_config_check_afu_index(struct pci_dev *dev,
> +int ocxl_config_check_afu_index(struct pci_dev *dev,
> struct ocxl_fn_config *fn, int afu_idx);
>
> /*
> * Read the configuration space of a function for the AFU specified by
> * the index 'afu_idx'. Fills in a ocxl_afu_config structure
> */
> -extern int ocxl_config_read_afu(struct pci_dev *dev,
> +int ocxl_config_read_afu(struct pci_dev *dev,
> struct ocxl_fn_config *fn,
> struct ocxl_afu_config *afu,
> u8 afu_idx);
> @@ -77,7 +77,7 @@ extern int ocxl_config_read_afu(struct pci_dev *dev,
> /*
> * Get the max PASID value that can be used by the function
> */
> -extern int ocxl_config_get_pasid_info(struct pci_dev *dev, int *count);
> +int ocxl_config_get_pasid_info(struct pci_dev *dev, int *count);
>
> /*
> * Tell an AFU, by writing in the configuration space, the PASIDs that
> @@ -87,7 +87,7 @@ extern int ocxl_config_get_pasid_info(struct pci_dev *dev, int *count);
> * 'afu_control_offset' is the offset of the AFU control DVSEC which
> * can be found in the function configuration
> */
> -extern void ocxl_config_set_afu_pasid(struct pci_dev *dev,
> +void ocxl_config_set_afu_pasid(struct pci_dev *dev,
> int afu_control_offset,
> int pasid_base, u32 pasid_count_log);
>
> @@ -98,7 +98,7 @@ extern void ocxl_config_set_afu_pasid(struct pci_dev *dev,
> * 'supported' is the total number of actags desired by all the AFUs
> * of the function.
> */
> -extern int ocxl_config_get_actag_info(struct pci_dev *dev,
> +int ocxl_config_get_actag_info(struct pci_dev *dev,
> u16 *base, u16 *enabled, u16 *supported);
>
> /*
> @@ -108,7 +108,7 @@ extern int ocxl_config_get_actag_info(struct pci_dev *dev,
> * 'func_offset' is the offset of the Function DVSEC that can found in
> * the function configuration
> */
> -extern void ocxl_config_set_actag(struct pci_dev *dev, int func_offset,
> +void ocxl_config_set_actag(struct pci_dev *dev, int func_offset,
> u32 actag_base, u32 actag_count);
>
> /*
> @@ -118,7 +118,7 @@ extern void ocxl_config_set_actag(struct pci_dev *dev, int func_offset,
> * 'afu_control_offset' is the offset of the AFU control DVSEC for the
> * desired AFU. It can be found in the AFU configuration
> */
> -extern void ocxl_config_set_afu_actag(struct pci_dev *dev,
> +void ocxl_config_set_afu_actag(struct pci_dev *dev,
> int afu_control_offset,
> int actag_base, int actag_count);
>
> @@ -128,7 +128,7 @@ extern void ocxl_config_set_afu_actag(struct pci_dev *dev,
> * 'afu_control_offset' is the offset of the AFU control DVSEC for the
> * desired AFU. It can be found in the AFU configuration
> */
> -extern void ocxl_config_set_afu_state(struct pci_dev *dev,
> +void ocxl_config_set_afu_state(struct pci_dev *dev,
> int afu_control_offset, int enable);
>
> /*
> @@ -139,7 +139,7 @@ extern void ocxl_config_set_afu_state(struct pci_dev *dev,
> * between the host and device, and set the Transaction Layer on both
> * accordingly.
> */
> -extern int ocxl_config_set_TL(struct pci_dev *dev, int tl_dvsec);
> +int ocxl_config_set_TL(struct pci_dev *dev, int tl_dvsec);
>
> /*
> * Request an AFU to terminate a PASID.
> @@ -152,7 +152,7 @@ extern int ocxl_config_set_TL(struct pci_dev *dev, int tl_dvsec);
> * 'afu_control_offset' is the offset of the AFU control DVSEC for the
> * desired AFU. It can be found in the AFU configuration
> */
> -extern int ocxl_config_terminate_pasid(struct pci_dev *dev,
> +int ocxl_config_terminate_pasid(struct pci_dev *dev,
> int afu_control_offset, int pasid);
>
> /*
> @@ -165,13 +165,13 @@ extern int ocxl_config_terminate_pasid(struct pci_dev *dev,
> * Returns a 'link handle' that should be used for further calls for
> * the link
> */
> -extern int ocxl_link_setup(struct pci_dev *dev, int PE_mask,
> +int ocxl_link_setup(struct pci_dev *dev, int PE_mask,
> void **link_handle);
>
> /*
> * Remove the association between the function and its link.
> */
> -extern void ocxl_link_release(struct pci_dev *dev, void *link_handle);
> +void ocxl_link_release(struct pci_dev *dev, void *link_handle);
>
> /*
> * Add a Process Element to the Shared Process Area for a link.
> @@ -183,7 +183,7 @@ extern void ocxl_link_release(struct pci_dev *dev, void *link_handle);
> * 'xsl_err_data' is an argument passed to the above callback, if
> * defined
> */
> -extern int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr,
> +int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr,
> u64 amr, struct mm_struct *mm,
> void (*xsl_err_cb)(void *data, u64 addr, u64 dsisr),
> void *xsl_err_data);
> @@ -195,12 +195,12 @@ extern int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr,
> * pasid: the PASID for the AFU context
> * tid: the new thread id for the process element
> */
> -extern int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid);
> +int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid);
>
> /*
> * Remove a Process Element from the Shared Process Area for a link
> */
> -extern int ocxl_link_remove_pe(void *link_handle, int pasid);
> +int ocxl_link_remove_pe(void *link_handle, int pasid);
>
> /*
> * Allocate an AFU interrupt associated to the link.
> @@ -212,12 +212,12 @@ extern int ocxl_link_remove_pe(void *link_handle, int pasid);
> * interrupt. It is an MMIO address which needs to be remapped (one
> * page).
> */
> -extern int ocxl_link_irq_alloc(void *link_handle, int *hw_irq,
> +int ocxl_link_irq_alloc(void *link_handle, int *hw_irq,
> u64 *obj_handle);
>
> /*
> * Free a previously allocated AFU interrupt
> */
> -extern void ocxl_link_free_irq(void *link_handle, int hw_irq);
> +void ocxl_link_free_irq(void *link_handle, int hw_irq);
>
> #endif /* _MISC_OCXL_H_ */
^ permalink raw reply
* Re: [PATCH v2 06/16] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
From: Cédric Le Goater @ 2019-03-13 8:46 UTC (permalink / raw)
To: David Gibson; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <20190313040327.GK9881@umbus.fritz.box>
On 3/13/19 5:03 AM, David Gibson wrote:
> On Tue, Mar 12, 2019 at 06:00:38PM +0100, Cédric Le Goater wrote:
>> On 2/25/19 3:39 AM, David Gibson wrote:
>>> On Fri, Feb 22, 2019 at 12:28:30PM +0100, Cédric Le Goater wrote:
>>>> These controls will be used by the H_INT_SET_QUEUE_CONFIG and
>>>> H_INT_GET_QUEUE_CONFIG hcalls from QEMU. They will also be used to
>>>> restore the configuration of the XIVE EQs in the KVM device and to
>>>> capture the internal runtime state of the EQs. Both 'get' and 'set'
>>>> rely on an OPAL call to access from the XIVE interrupt controller the
>>>> EQ toggle bit and EQ index which are updated by the HW when event
>>>> notifications are enqueued in the EQ.
>>>>
>>>> The value of the guest physical address of the event queue is saved in
>>>> the XIVE internal xive_q structure for later use. That is when
>>>> migration needs to mark the EQ pages dirty to capture a consistent
>>>> memory state of the VM.
>>>>
>>>> To be noted that H_INT_SET_QUEUE_CONFIG does not require the extra
>>>> OPAL call setting the EQ toggle bit and EQ index to configure the EQ,
>>>> but restoring the EQ state will.
>>
>> I think we need to add some kind of flags to differentiate the hcall
>> H_INT_SET_QUEUE_CONFIG from the restore of the EQ. The hcall does
>> not need OPAL support call and this could help in the code
>> transition.
>
> Hrm. What's the actual difference in the semantics between the two
> cases.
None.
But we don't need to set the EQ state in the case of the HCALL and it's
(very) practical to run guests with XIVE enabled without the OPAL support.
The latter is the main reason clearly.
Thinking of it, I could test the EQ toggle bit and index passed to KVM
and skip the OPAL call which restores the EQ state if they are zero.
This is because I know that the OPAL call configuring the EQ resets them.
That will do. No need for a flag.
> The guest shouldn't have awareness of whether or not OPAL is involved.
yes.
Thanks,
C.
^ permalink raw reply
* Re: [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug()
From: Laurent Vivier @ 2019-03-13 8:50 UTC (permalink / raw)
To: Christophe Leroy, linux-kernel; +Cc: linuxppc-dev, David Gibson
In-Reply-To: <6f089371-29e6-6fa9-379d-76d3a87e2b8b@c-s.fr>
On 13/03/2019 09:28, Christophe Leroy wrote:
>
>
> Le 13/03/2019 à 09:01, Laurent Vivier a écrit :
>> On 13/03/2019 07:03, Christophe Leroy wrote:
>>>
>>>
>>> Le 08/03/2019 à 11:54, Laurent Vivier a écrit :
>>>> resize_hpt_for_hotplug() reports a warning when it cannot
>>>> resize the hash page table ("Unable to resize hash page
>>>> table to target order") but in some cases it's not a problem
>>>> and can make user thinks something has not worked properly.
>>>>
>>>> This patch moves the warning to arch_remove_memory() to
>>>> only report the problem when it is needed.
>>>>
>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>> ---
>>>> arch/powerpc/include/asm/sparsemem.h | 4 ++--
>>>> arch/powerpc/mm/hash_utils_64.c | 17 ++++++-----------
>>>> arch/powerpc/mm/mem.c | 3 ++-
>>>> arch/powerpc/platforms/pseries/lpar.c | 3 ++-
>>>> 4 files changed, 12 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/include/asm/sparsemem.h
>>>> b/arch/powerpc/include/asm/sparsemem.h
>>>> index 68da49320592..3192d454a733 100644
>>>> --- a/arch/powerpc/include/asm/sparsemem.h
>>>> +++ b/arch/powerpc/include/asm/sparsemem.h
>>>> @@ -17,9 +17,9 @@ extern int create_section_mapping(unsigned long
>>>> start, unsigned long end, int ni
>>>> extern int remove_section_mapping(unsigned long start, unsigned long
>>>> end);
>>>> #ifdef CONFIG_PPC_BOOK3S_64
>>>> -extern void resize_hpt_for_hotplug(unsigned long new_mem_size);
>>>> +extern int resize_hpt_for_hotplug(unsigned long new_mem_size);
>>>> #else
>>>> -static inline void resize_hpt_for_hotplug(unsigned long new_mem_size)
>>>> { }
>>>> +static inline int resize_hpt_for_hotplug(unsigned long new_mem_size)
>>>> { return 0; }
>>>> #endif
>>>> #ifdef CONFIG_NUMA
>>>> diff --git a/arch/powerpc/mm/hash_utils_64.c
>>>> b/arch/powerpc/mm/hash_utils_64.c
>>>> index 0cc7fbc3bd1c..40bb2a8326bb 100644
>>>> --- a/arch/powerpc/mm/hash_utils_64.c
>>>> +++ b/arch/powerpc/mm/hash_utils_64.c
>>>> @@ -755,12 +755,12 @@ static unsigned long __init
>>>> htab_get_table_size(void)
>>>> }
>>>> #ifdef CONFIG_MEMORY_HOTPLUG
>>>> -void resize_hpt_for_hotplug(unsigned long new_mem_size)
>>>> +int resize_hpt_for_hotplug(unsigned long new_mem_size)
>>>> {
>>>> unsigned target_hpt_shift;
>>>> if (!mmu_hash_ops.resize_hpt)
>>>> - return;
>>>> + return 0;
>>>> target_hpt_shift = htab_shift_for_mem_size(new_mem_size);
>>>> @@ -773,15 +773,10 @@ void resize_hpt_for_hotplug(unsigned long
>>>> new_mem_size)
>>>> * current shift
>>>> */
>>>> if ((target_hpt_shift > ppc64_pft_size)
>>>> - || (target_hpt_shift < (ppc64_pft_size - 1))) {
>>>> - int rc;
>>>> -
>>>> - rc = mmu_hash_ops.resize_hpt(target_hpt_shift);
>>>> - if (rc && (rc != -ENODEV))
>>>> - printk(KERN_WARNING
>>>> - "Unable to resize hash page table to target order
>>>> %d: %d\n",
>>>> - target_hpt_shift, rc);
>>>> - }
>>>> + || (target_hpt_shift < (ppc64_pft_size - 1)))
>>>
>>> The || should go on the line above and the two (target_hpt... should be
>>> aligned, and the () after the < are superflous.
>>>
>>> And indeed, we should (in another patch) rename 'target_hpt_shift' with
>>> a shorter name, this would avoid multiple lines.
>>>
>>> Ref
>>> https://www.kernel.org/doc/html/latest/process/coding-style.html#naming
>>> :
>>>
>>> LOCAL variable names should be short, and to the point. If you have some
>>> random integer loop counter, it should probably be called i. Calling it
>>> loop_counter is non-productive, if there is no chance of it being
>>> mis-understood. Similarly, tmp can be just about any type of variable
>>> that is used to hold a temporary value.
>>>
>>> If you are afraid to mix up your local variable names, you have another
>>> problem, which is called the function-growth-hormone-imbalance syndrome.
>>> See chapter 6 (Functions).
>>>
>>
>> I'm only removing a warning. Do we really need to rewrite all the code
>> around for that?
>
> No, and that's the reason why I said it could be done in another
> (future) patch.
>
> Anyway, your patch should be clean regarding checkpatch
>
> See https://patchwork.ozlabs.org/patch/1052984/
> And
> https://openpower.xyz/job/snowpatch/job/snowpatch-linux-checkpatch/3298//artifact/linux/checkpatch.log
>
>
> CHECK:AVOID_EXTERNS: extern prototypes should be avoided in .h files
> #31: FILE: arch/powerpc/include/asm/sparsemem.h:20:
> +extern int resize_hpt_for_hotplug(unsigned long new_mem_size);
>
> CHECK:LOGICAL_CONTINUATIONS: Logical continuations should be on the
> previous line
> #70: FILE: arch/powerpc/mm/hash_utils_64.c:776:
> if ((target_hpt_shift > ppc64_pft_size)
> + || (target_hpt_shift < (ppc64_pft_size - 1)))
>
It's really strange, from linux directory:
./scripts/checkpatch.pl 0001-powerpc-mm-move-warning-from-resize_hpt_for_hotplug.patch
doesn't output this error [1]. Why linux-ppc doesn't use the same script as in the kernel directory?
Anyway, I send a v3.
Thanks,
Laurent
[1] only:
WARNING: line over 80 characters
#34: FILE: arch/powerpc/include/asm/sparsemem.h:22:
+static inline int resize_hpt_for_hotplug(unsigned long new_mem_size) { return 0; }
total: 0 errors, 1 warnings, 70 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
0001-powerpc-mm-move-warning-from-resize_hpt_for_hotplug.patch has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
But I think it's cleaner to keep the over 80 characters line for the inline.
^ permalink raw reply
* Re: [PATCH 1/1] arch/powerpc: Rework local_paca to avoid LTO warnings
From: Christophe Leroy @ 2019-03-13 9:06 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Michal Hocko, Mahesh Salgaonkar, Alastair D'Silva,
linux-kernel, Nicholas Piggin, Mike Rapoport, Paul Mackerras,
Naveen N. Rao, linuxppc-dev, Andrew Morton
In-Reply-To: <20190313034208.13134-1-alastair@au1.ibm.com>
Hello,
Le 13/03/2019 à 04:42, Alastair D'Silva a écrit :
> From: Alastair D'Silva <alastair@d-silva.org>
>
> When building an LTO kernel, the existing code generates warnings:
> ./arch/powerpc/include/asm/paca.h:37:30: warning: register of
> ‘local_paca’ used for multiple global register variables
> register struct paca_struct *local_paca asm("r13");
> ^
> ./arch/powerpc/include/asm/paca.h:37:30: note: conflicts with
> ‘local_paca’
How do you build a LTO kernel ?
>
> This patch reworks local_paca into an inline getter & setter function,
> which addresses the warning.
This patch adds sparse warnings, see
https://patchwork.ozlabs.org/patch/1055875/
>
> Generated ASM from this patch is broadly similar (addresses have
> changed and the compiler uses different GPRs in some places).
Your text might be confusion. When I read it the first time I thought
you were saying that the compiler was now using another GPR than r13.
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
I guess the same has to be done for current, see
arch/powerpc/include/asm/current.h :
/*
* We keep `current' in r2 for speed.
*/
register struct task_struct *current asm ("r2");
> ---
> arch/powerpc/include/asm/paca.h | 44 +++++++++++++++++++++++----------
> arch/powerpc/kernel/paca.c | 2 +-
> 2 files changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
> index e843bc5d1a0f..9c9e2dea0f9b 100644
> --- a/arch/powerpc/include/asm/paca.h
> +++ b/arch/powerpc/include/asm/paca.h
> @@ -34,19 +34,6 @@
> #include <asm/cpuidle.h>
> #include <asm/atomic.h>
>
> -register struct paca_struct *local_paca asm("r13");
> -
> -#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
> -extern unsigned int debug_smp_processor_id(void); /* from linux/smp.h */
> -/*
> - * Add standard checks that preemption cannot occur when using get_paca():
> - * otherwise the paca_struct it points to may be the wrong one just after.
> - */
> -#define get_paca() ((void) debug_smp_processor_id(), local_paca)
> -#else
> -#define get_paca() local_paca
> -#endif
> -
> #ifdef CONFIG_PPC_PSERIES
> #define get_lppaca() (get_paca()->lppaca_ptr)
> #endif
> @@ -266,6 +253,37 @@ struct paca_struct {
> #endif
> } ____cacheline_aligned;
>
> +#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
> +extern unsigned int debug_smp_processor_id(void); /* from linux/smp.h */
> +#endif
Why moving this down, why not leaving at the same place as before ?
If you really need to move it, you should remove the 'extern' at the
same time to make checkpatch happy.
> +
> +static inline struct paca_struct *get_paca_no_preempt_check(void)
> +{
> + register struct paca_struct *paca asm("r13");
Should be a blank line there.
> + return paca;
> +}
> +
> +static inline struct paca_struct *get_paca(void)
> +{
> +#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
> + /*
> + * Add standard checks that preemption cannot occur when using get_paca():
> + * otherwise the paca_struct it points to may be the wrong one just after.
> + */
> + debug_smp_processor_id();
> +#endif
> + return get_paca_no_preempt_check();
> +}
> +
> +#define local_paca get_paca_no_preempt_check()
> +
> +static inline void set_paca(struct paca_struct *new)
> +{
> + register struct paca_struct *paca asm("r13");
Blank line should be added here.
> + paca = new;
> +}
> +
> +
> extern void copy_mm_to_paca(struct mm_struct *mm);
> extern struct paca_struct **paca_ptrs;
> extern void initialise_paca(struct paca_struct *new_paca, int cpu);
> diff --git a/arch/powerpc/kernel/paca.c b/arch/powerpc/kernel/paca.c
> index 913bfca09c4f..ae5c243f9d5a 100644
> --- a/arch/powerpc/kernel/paca.c
> +++ b/arch/powerpc/kernel/paca.c
> @@ -172,7 +172,7 @@ void __init initialise_paca(struct paca_struct *new_paca, int cpu)
> void setup_paca(struct paca_struct *new_paca)
> {
> /* Setup r13 */
> - local_paca = new_paca;
> + set_paca(new_paca);
>
> #ifdef CONFIG_PPC_BOOK3E
> /* On Book3E, initialize the TLB miss exception frames */
>
Christophe
^ permalink raw reply
* Re: [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug()
From: Christophe Leroy @ 2019-03-13 9:10 UTC (permalink / raw)
To: Laurent Vivier, linux-kernel; +Cc: linuxppc-dev, David Gibson
In-Reply-To: <8a22d3a7-f574-19f1-46d0-a54a5343dec8@redhat.com>
Le 13/03/2019 à 09:50, Laurent Vivier a écrit :
> On 13/03/2019 09:28, Christophe Leroy wrote:
>>
>>
>> Le 13/03/2019 à 09:01, Laurent Vivier a écrit :
>>> On 13/03/2019 07:03, Christophe Leroy wrote:
>>>>
>>>>
>>>> Le 08/03/2019 à 11:54, Laurent Vivier a écrit :
>>>>> resize_hpt_for_hotplug() reports a warning when it cannot
>>>>> resize the hash page table ("Unable to resize hash page
>>>>> table to target order") but in some cases it's not a problem
>>>>> and can make user thinks something has not worked properly.
>>>>>
>>>>> This patch moves the warning to arch_remove_memory() to
>>>>> only report the problem when it is needed.
>>>>>
>>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>>> ---
>>>>> arch/powerpc/include/asm/sparsemem.h | 4 ++--
>>>>> arch/powerpc/mm/hash_utils_64.c | 17 ++++++-----------
>>>>> arch/powerpc/mm/mem.c | 3 ++-
>>>>> arch/powerpc/platforms/pseries/lpar.c | 3 ++-
>>>>> 4 files changed, 12 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/arch/powerpc/include/asm/sparsemem.h
>>>>> b/arch/powerpc/include/asm/sparsemem.h
>>>>> index 68da49320592..3192d454a733 100644
>>>>> --- a/arch/powerpc/include/asm/sparsemem.h
>>>>> +++ b/arch/powerpc/include/asm/sparsemem.h
>>>>> @@ -17,9 +17,9 @@ extern int create_section_mapping(unsigned long
>>>>> start, unsigned long end, int ni
>>>>> extern int remove_section_mapping(unsigned long start, unsigned long
>>>>> end);
>>>>> #ifdef CONFIG_PPC_BOOK3S_64
>>>>> -extern void resize_hpt_for_hotplug(unsigned long new_mem_size);
>>>>> +extern int resize_hpt_for_hotplug(unsigned long new_mem_size);
>>>>> #else
>>>>> -static inline void resize_hpt_for_hotplug(unsigned long new_mem_size)
>>>>> { }
>>>>> +static inline int resize_hpt_for_hotplug(unsigned long new_mem_size)
>>>>> { return 0; }
>>>>> #endif
>>>>> #ifdef CONFIG_NUMA
>>>>> diff --git a/arch/powerpc/mm/hash_utils_64.c
>>>>> b/arch/powerpc/mm/hash_utils_64.c
>>>>> index 0cc7fbc3bd1c..40bb2a8326bb 100644
>>>>> --- a/arch/powerpc/mm/hash_utils_64.c
>>>>> +++ b/arch/powerpc/mm/hash_utils_64.c
>>>>> @@ -755,12 +755,12 @@ static unsigned long __init
>>>>> htab_get_table_size(void)
>>>>> }
>>>>> #ifdef CONFIG_MEMORY_HOTPLUG
>>>>> -void resize_hpt_for_hotplug(unsigned long new_mem_size)
>>>>> +int resize_hpt_for_hotplug(unsigned long new_mem_size)
>>>>> {
>>>>> unsigned target_hpt_shift;
>>>>> if (!mmu_hash_ops.resize_hpt)
>>>>> - return;
>>>>> + return 0;
>>>>> target_hpt_shift = htab_shift_for_mem_size(new_mem_size);
>>>>> @@ -773,15 +773,10 @@ void resize_hpt_for_hotplug(unsigned long
>>>>> new_mem_size)
>>>>> * current shift
>>>>> */
>>>>> if ((target_hpt_shift > ppc64_pft_size)
>>>>> - || (target_hpt_shift < (ppc64_pft_size - 1))) {
>>>>> - int rc;
>>>>> -
>>>>> - rc = mmu_hash_ops.resize_hpt(target_hpt_shift);
>>>>> - if (rc && (rc != -ENODEV))
>>>>> - printk(KERN_WARNING
>>>>> - "Unable to resize hash page table to target order
>>>>> %d: %d\n",
>>>>> - target_hpt_shift, rc);
>>>>> - }
>>>>> + || (target_hpt_shift < (ppc64_pft_size - 1)))
>>>>
>>>> The || should go on the line above and the two (target_hpt... should be
>>>> aligned, and the () after the < are superflous.
>>>>
>>>> And indeed, we should (in another patch) rename 'target_hpt_shift' with
>>>> a shorter name, this would avoid multiple lines.
>>>>
>>>> Ref
>>>> https://www.kernel.org/doc/html/latest/process/coding-style.html#naming
>>>> :
>>>>
>>>> LOCAL variable names should be short, and to the point. If you have some
>>>> random integer loop counter, it should probably be called i. Calling it
>>>> loop_counter is non-productive, if there is no chance of it being
>>>> mis-understood. Similarly, tmp can be just about any type of variable
>>>> that is used to hold a temporary value.
>>>>
>>>> If you are afraid to mix up your local variable names, you have another
>>>> problem, which is called the function-growth-hormone-imbalance syndrome.
>>>> See chapter 6 (Functions).
>>>>
>>>
>>> I'm only removing a warning. Do we really need to rewrite all the code
>>> around for that?
>>
>> No, and that's the reason why I said it could be done in another
>> (future) patch.
>>
>> Anyway, your patch should be clean regarding checkpatch
>>
>> See https://patchwork.ozlabs.org/patch/1052984/
>> And
>> https://openpower.xyz/job/snowpatch/job/snowpatch-linux-checkpatch/3298//artifact/linux/checkpatch.log
>>
>>
>> CHECK:AVOID_EXTERNS: extern prototypes should be avoided in .h files
>> #31: FILE: arch/powerpc/include/asm/sparsemem.h:20:
>> +extern int resize_hpt_for_hotplug(unsigned long new_mem_size);
>>
>> CHECK:LOGICAL_CONTINUATIONS: Logical continuations should be on the
>> previous line
>> #70: FILE: arch/powerpc/mm/hash_utils_64.c:776:
>> if ((target_hpt_shift > ppc64_pft_size)
>> + || (target_hpt_shift < (ppc64_pft_size - 1)))
>>
>
> It's really strange, from linux directory:
>
> ./scripts/checkpatch.pl 0001-powerpc-mm-move-warning-from-resize_hpt_for_hotplug.patch
Try with --strict
>
> doesn't output this error [1]. Why linux-ppc doesn't use the same script as in the kernel directory?
linux-ppc used it but with dedicated options, see
arch/powerpc/tools/checkpatch.sh
>
> Anyway, I send a v3.
>
> Thanks,
> Laurent
>
> [1] only:
>
> WARNING: line over 80 characters
> #34: FILE: arch/powerpc/include/asm/sparsemem.h:22:
> +static inline int resize_hpt_for_hotplug(unsigned long new_mem_size) { return 0; }
linux-ppc allows 90 characters.
>
> total: 0 errors, 1 warnings, 70 lines checked
>
> NOTE: For some of the reported defects, checkpatch may be able to
> mechanically convert to the typical style using --fix or --fix-inplace.
>
> 0001-powerpc-mm-move-warning-from-resize_hpt_for_hotplug.patch has style problems, please review.
>
> NOTE: If any of the errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
>
>
> But I think it's cleaner to keep the over 80 characters line for the inline.
>
Christophe
^ permalink raw reply
* Re: [PATCH 5/5] ocxl: Remove some unused exported symbols
From: Greg Kurz @ 2019-03-13 9:10 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, Andrew Donnellan,
Alastair D'Silva, Frederic Barrat, linuxppc-dev
In-Reply-To: <20190313040702.14276-6-alastair@au1.ibm.com>
On Wed, 13 Mar 2019 15:07:01 +1100
"Alastair D'Silva" <alastair@au1.ibm.com> wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> Remove some unused exported symbols.
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
> drivers/misc/ocxl/config.c | 2 --
> drivers/misc/ocxl/ocxl_internal.h | 23 +++++++++++++++++++++++
> include/misc/ocxl.h | 23 -----------------------
> 3 files changed, 23 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/misc/ocxl/config.c b/drivers/misc/ocxl/config.c
> index 026ac2ac4f9c..c90c2e4875bf 100644
> --- a/drivers/misc/ocxl/config.c
> +++ b/drivers/misc/ocxl/config.c
> @@ -299,7 +299,6 @@ int ocxl_config_check_afu_index(struct pci_dev *dev,
> }
> return 1;
> }
> -EXPORT_SYMBOL_GPL(ocxl_config_check_afu_index);
>
> static int read_afu_name(struct pci_dev *dev, struct ocxl_fn_config *fn,
> struct ocxl_afu_config *afu)
> @@ -535,7 +534,6 @@ int ocxl_config_get_pasid_info(struct pci_dev *dev, int *count)
> {
> return pnv_ocxl_get_pasid_count(dev, count);
> }
> -EXPORT_SYMBOL_GPL(ocxl_config_get_pasid_info);
>
> void ocxl_config_set_afu_pasid(struct pci_dev *dev, int pos, int pasid_base,
> u32 pasid_count_log)
> diff --git a/drivers/misc/ocxl/ocxl_internal.h b/drivers/misc/ocxl/ocxl_internal.h
> index 321b29e77f45..06fd98c989c8 100644
> --- a/drivers/misc/ocxl/ocxl_internal.h
> +++ b/drivers/misc/ocxl/ocxl_internal.h
> @@ -107,6 +107,29 @@ void ocxl_pasid_afu_free(struct ocxl_fn *fn, u32 start, u32 size);
> int ocxl_actag_afu_alloc(struct ocxl_fn *fn, u32 size);
> void ocxl_actag_afu_free(struct ocxl_fn *fn, u32 start, u32 size);
>
> +/*
> + * Get the max PASID value that can be used by the function
> + */
> +int ocxl_config_get_pasid_info(struct pci_dev *dev, int *count);
> +
> +/*
> + * Check if an AFU index is valid for the given function.
> + *
> + * AFU indexes can be sparse, so a driver should check all indexes up
> + * to the maximum found in the function description
> + */
> +int ocxl_config_check_afu_index(struct pci_dev *dev,
> + struct ocxl_fn_config *fn, int afu_idx);
> +
> +/**
Two *s ?
Also, this results in an ocxl_internal.h header file where only these
three functions are documented... which looks a bit weird IMHO. Since
these are ocxl internals, do we _really_ need to keep the comments ?
> + * Update values within a Process Element
> + *
> + * link_handle: the link handle associated with the process element
> + * pasid: the PASID for the AFU context
> + * tid: the new thread id for the process element
> + */
> +int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid);
> +
> struct ocxl_context *ocxl_context_alloc(void);
> int ocxl_context_init(struct ocxl_context *ctx, struct ocxl_afu *afu,
> struct address_space *mapping);
> diff --git a/include/misc/ocxl.h b/include/misc/ocxl.h
> index 4544573cc93c..9530d3be1b30 100644
> --- a/include/misc/ocxl.h
> +++ b/include/misc/ocxl.h
> @@ -56,15 +56,6 @@ struct ocxl_fn_config {
> int ocxl_config_read_function(struct pci_dev *dev,
> struct ocxl_fn_config *fn);
>
> -/*
> - * Check if an AFU index is valid for the given function.
> - *
> - * AFU indexes can be sparse, so a driver should check all indexes up
> - * to the maximum found in the function description
> - */
> -int ocxl_config_check_afu_index(struct pci_dev *dev,
> - struct ocxl_fn_config *fn, int afu_idx);
> -
> /*
> * Read the configuration space of a function for the AFU specified by
> * the index 'afu_idx'. Fills in a ocxl_afu_config structure
> @@ -74,11 +65,6 @@ int ocxl_config_read_afu(struct pci_dev *dev,
> struct ocxl_afu_config *afu,
> u8 afu_idx);
>
> -/*
> - * Get the max PASID value that can be used by the function
> - */
> -int ocxl_config_get_pasid_info(struct pci_dev *dev, int *count);
> -
> /*
> * Tell an AFU, by writing in the configuration space, the PASIDs that
> * it can use. Range starts at 'pasid_base' and its size is a multiple
> @@ -188,15 +174,6 @@ int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr,
> void (*xsl_err_cb)(void *data, u64 addr, u64 dsisr),
> void *xsl_err_data);
>
> -/**
> - * Update values within a Process Element
> - *
> - * link_handle: the link handle associated with the process element
> - * pasid: the PASID for the AFU context
> - * tid: the new thread id for the process element
> - */
> -int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid);
> -
> /*
> * Remove a Process Element from the Shared Process Area for a link
> */
^ permalink raw reply
* Re: [PATCH v2 06/16] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
From: Cédric Le Goater @ 2019-03-13 9:40 UTC (permalink / raw)
To: Paul Mackerras; +Cc: kvm, kvm-ppc, linuxppc-dev, David Gibson
In-Reply-To: <20190226052429.GC28015@blackberry>
On 2/26/19 6:24 AM, Paul Mackerras wrote:
> On Fri, Feb 22, 2019 at 12:28:30PM +0100, Cédric Le Goater wrote:
>> These controls will be used by the H_INT_SET_QUEUE_CONFIG and
>> H_INT_GET_QUEUE_CONFIG hcalls from QEMU. They will also be used to
>> restore the configuration of the XIVE EQs in the KVM device and to
>> capture the internal runtime state of the EQs. Both 'get' and 'set'
>> rely on an OPAL call to access from the XIVE interrupt controller the
>> EQ toggle bit and EQ index which are updated by the HW when event
>> notifications are enqueued in the EQ.
>>
>> The value of the guest physical address of the event queue is saved in
>> the XIVE internal xive_q structure for later use. That is when
>> migration needs to mark the EQ pages dirty to capture a consistent
>> memory state of the VM.
>>
>> To be noted that H_INT_SET_QUEUE_CONFIG does not require the extra
>> OPAL call setting the EQ toggle bit and EQ index to configure the EQ,
>> but restoring the EQ state will.
>
> [snip]
>
>> +/* Layout of 64-bit eq attribute */
>> +#define KVM_XIVE_EQ_PRIORITY_SHIFT 0
>> +#define KVM_XIVE_EQ_PRIORITY_MASK 0x7
>> +#define KVM_XIVE_EQ_SERVER_SHIFT 3
>> +#define KVM_XIVE_EQ_SERVER_MASK 0xfffffff8ULL
>> +
>> +/* Layout of 64-bit eq attribute values */
>> +struct kvm_ppc_xive_eq {
>> + __u32 flags;
>> + __u32 qsize;
>> + __u64 qpage;
>> + __u32 qtoggle;
>> + __u32 qindex;
>> + __u8 pad[40];
>> +};
>
> This is confusing. What's the difference between an "eq attribute"
> and an "eq attribute value"? Is the first actually a queue index or
> a queue identifier?
The "attribute" qualifier comes from the {get,set,has}_addr methods
of the KVM device. But it is not a well chosen name for the group
KVM_DEV_XIVE_GRP_EQ_CONFIG.
I should be using "eq identifier" and "eq values" or "eq state".
> Also, the kvm_ppc_xive_eq is not 64 bits, so the comment above it is
> wrong. Maybe you meant "64-byte"?
That was a bad copy paste. I have padded the structure to twice the size
of the XIVE END (the XIVE EQ descriptor in HW) which size is 32 bytes.
I thought that one extra u64 was not enough room for future.
>
> [snip]
>
>> + page = gfn_to_page(kvm, gpa_to_gfn(kvm_eq.qpage));
>> + if (is_error_page(page)) {
>> + pr_warn("Couldn't get guest page for %llx!\n", kvm_eq.qpage);
>> + return -ENOMEM;
>> + }
>> + qaddr = page_to_virt(page) + (kvm_eq.qpage & ~PAGE_MASK);
>
> Isn't this assuming that we can map the whole queue with a single
> gfn_to_page? That would only be true if kvm_eq.qsize <= PAGE_SHIFT.
> What happens if kvm_eq.qsize > PAGE_SHIFT?
Ah yes. Theoretically, it should not happen because we only advertise
64K in the DT for the moment. I should at least add a check. So I will
change the helper xive_native_validate_queue_size() to return -EINVAL
for other page sizes.
Do you think it would be complex to support XIVE EQs using a page larger
than the default one on the guest ?
Thanks,
C.
>
> Paul.
>
^ permalink raw reply
* Re: [PATCH v2] fs/dax: deposit pagetable even when installing zero page
From: Jan Kara @ 2019-03-13 9:58 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: Jan Kara, linux-nvdimm, linux-mm, Alexander Viro, Ross Zwisler,
linux-fsdevel, dan.j.williams, linuxppc-dev, akpm
In-Reply-To: <8736nrnzxm.fsf@linux.ibm.com>
On Wed 13-03-19 10:17:17, Aneesh Kumar K.V wrote:
>
> Hi Dan/Andrew/Jan,
>
> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>
> > Architectures like ppc64 use the deposited page table to store hardware
> > page table slot information. Make sure we deposit a page table when
> > using zero page at the pmd level for hash.
> >
> > Without this we hit
> >
> > Unable to handle kernel paging request for data at address 0x00000000
> > Faulting instruction address: 0xc000000000082a74
> > Oops: Kernel access of bad area, sig: 11 [#1]
> > ....
> >
> > NIP [c000000000082a74] __hash_page_thp+0x224/0x5b0
> > LR [c0000000000829a4] __hash_page_thp+0x154/0x5b0
> > Call Trace:
> > hash_page_mm+0x43c/0x740
> > do_hash_page+0x2c/0x3c
> > copy_from_iter_flushcache+0xa4/0x4a0
> > pmem_copy_from_iter+0x2c/0x50 [nd_pmem]
> > dax_copy_from_iter+0x40/0x70
> > dax_iomap_actor+0x134/0x360
> > iomap_apply+0xfc/0x1b0
> > dax_iomap_rw+0xac/0x130
> > ext4_file_write_iter+0x254/0x460 [ext4]
> > __vfs_write+0x120/0x1e0
> > vfs_write+0xd8/0x220
> > SyS_write+0x6c/0x110
> > system_call+0x3c/0x130
> >
> > Fixes: b5beae5e224f ("powerpc/pseries: Add driver for PAPR SCM regions")
> > Reviewed-by: Jan Kara <jack@suse.cz>
> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>
> Any suggestion on which tree this patch should got to? Also since this
> fix a kernel crash, we may want to get this to 5.1?
I think this should go through Dan's tree...
Honza
> > ---
> > Changes from v1:
> > * Add reviewed-by:
> > * Add Fixes:
> >
> > fs/dax.c | 15 +++++++++++++++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/fs/dax.c b/fs/dax.c
> > index 6959837cc465..01bfb2ac34f9 100644
> > --- a/fs/dax.c
> > +++ b/fs/dax.c
> > @@ -33,6 +33,7 @@
> > #include <linux/sizes.h>
> > #include <linux/mmu_notifier.h>
> > #include <linux/iomap.h>
> > +#include <asm/pgalloc.h>
> > #include "internal.h"
> >
> > #define CREATE_TRACE_POINTS
> > @@ -1410,7 +1411,9 @@ static vm_fault_t dax_pmd_load_hole(struct xa_state *xas, struct vm_fault *vmf,
> > {
> > struct address_space *mapping = vmf->vma->vm_file->f_mapping;
> > unsigned long pmd_addr = vmf->address & PMD_MASK;
> > + struct vm_area_struct *vma = vmf->vma;
> > struct inode *inode = mapping->host;
> > + pgtable_t pgtable = NULL;
> > struct page *zero_page;
> > spinlock_t *ptl;
> > pmd_t pmd_entry;
> > @@ -1425,12 +1428,22 @@ static vm_fault_t dax_pmd_load_hole(struct xa_state *xas, struct vm_fault *vmf,
> > *entry = dax_insert_entry(xas, mapping, vmf, *entry, pfn,
> > DAX_PMD | DAX_ZERO_PAGE, false);
> >
> > + if (arch_needs_pgtable_deposit()) {
> > + pgtable = pte_alloc_one(vma->vm_mm);
> > + if (!pgtable)
> > + return VM_FAULT_OOM;
> > + }
> > +
> > ptl = pmd_lock(vmf->vma->vm_mm, vmf->pmd);
> > if (!pmd_none(*(vmf->pmd))) {
> > spin_unlock(ptl);
> > goto fallback;
> > }
> >
> > + if (pgtable) {
> > + pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
> > + mm_inc_nr_ptes(vma->vm_mm);
> > + }
> > pmd_entry = mk_pmd(zero_page, vmf->vma->vm_page_prot);
> > pmd_entry = pmd_mkhuge(pmd_entry);
> > set_pmd_at(vmf->vma->vm_mm, pmd_addr, vmf->pmd, pmd_entry);
> > @@ -1439,6 +1452,8 @@ static vm_fault_t dax_pmd_load_hole(struct xa_state *xas, struct vm_fault *vmf,
> > return VM_FAULT_NOPAGE;
> >
> > fallback:
> > + if (pgtable)
> > + pte_free(vma->vm_mm, pgtable);
> > trace_dax_pmd_load_hole_fallback(inode, vmf, zero_page, *entry);
> > return VM_FAULT_FALLBACK;
> > }
> > --
> > 2.20.1
>
> -aneesh
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* [PATCH v3] powerpc/mm: move warning from resize_hpt_for_hotplug()
From: Laurent Vivier @ 2019-03-13 10:25 UTC (permalink / raw)
To: linux-kernel; +Cc: linuxppc-dev, Laurent Vivier, David Gibson
resize_hpt_for_hotplug() reports a warning when it cannot
resize the hash page table ("Unable to resize hash page
table to target order") but in some cases it's not a problem
and can make user thinks something has not worked properly.
This patch moves the warning to arch_remove_memory() to
only report the problem when it is needed.
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
Notes:
v3: move "||" to above line and remove parenthesis
v2: add warning messages for H_PARAMETER and H_RESOURCE
arch/powerpc/include/asm/sparsemem.h | 4 ++--
arch/powerpc/mm/hash_utils_64.c | 19 +++++++------------
arch/powerpc/mm/mem.c | 3 ++-
arch/powerpc/platforms/pseries/lpar.c | 3 ++-
4 files changed, 13 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/include/asm/sparsemem.h b/arch/powerpc/include/asm/sparsemem.h
index 68da49320592..3192d454a733 100644
--- a/arch/powerpc/include/asm/sparsemem.h
+++ b/arch/powerpc/include/asm/sparsemem.h
@@ -17,9 +17,9 @@ extern int create_section_mapping(unsigned long start, unsigned long end, int ni
extern int remove_section_mapping(unsigned long start, unsigned long end);
#ifdef CONFIG_PPC_BOOK3S_64
-extern void resize_hpt_for_hotplug(unsigned long new_mem_size);
+extern int resize_hpt_for_hotplug(unsigned long new_mem_size);
#else
-static inline void resize_hpt_for_hotplug(unsigned long new_mem_size) { }
+static inline int resize_hpt_for_hotplug(unsigned long new_mem_size) { return 0; }
#endif
#ifdef CONFIG_NUMA
diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
index 0cc7fbc3bd1c..5aa7594ee71b 100644
--- a/arch/powerpc/mm/hash_utils_64.c
+++ b/arch/powerpc/mm/hash_utils_64.c
@@ -755,12 +755,12 @@ static unsigned long __init htab_get_table_size(void)
}
#ifdef CONFIG_MEMORY_HOTPLUG
-void resize_hpt_for_hotplug(unsigned long new_mem_size)
+int resize_hpt_for_hotplug(unsigned long new_mem_size)
{
unsigned target_hpt_shift;
if (!mmu_hash_ops.resize_hpt)
- return;
+ return 0;
target_hpt_shift = htab_shift_for_mem_size(new_mem_size);
@@ -772,16 +772,11 @@ void resize_hpt_for_hotplug(unsigned long new_mem_size)
* reduce unless the target shift is at least 2 below the
* current shift
*/
- if ((target_hpt_shift > ppc64_pft_size)
- || (target_hpt_shift < (ppc64_pft_size - 1))) {
- int rc;
-
- rc = mmu_hash_ops.resize_hpt(target_hpt_shift);
- if (rc && (rc != -ENODEV))
- printk(KERN_WARNING
- "Unable to resize hash page table to target order %d: %d\n",
- target_hpt_shift, rc);
- }
+ if (target_hpt_shift > ppc64_pft_size ||
+ target_hpt_shift < ppc64_pft_size - 1)
+ return mmu_hash_ops.resize_hpt(target_hpt_shift);
+
+ return 0;
}
int hash__create_section_mapping(unsigned long start, unsigned long end, int nid)
diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index 33cc6f676fa6..0d40d970cf4a 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -169,7 +169,8 @@ int __meminit arch_remove_memory(int nid, u64 start, u64 size,
*/
vm_unmap_aliases();
- resize_hpt_for_hotplug(memblock_phys_mem_size());
+ if (resize_hpt_for_hotplug(memblock_phys_mem_size()) == -ENOSPC)
+ pr_warn("Hash collision while resizing HPT\n");
return ret;
}
diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
index f2a9f0adc2d3..1034ef1fe2b4 100644
--- a/arch/powerpc/platforms/pseries/lpar.c
+++ b/arch/powerpc/platforms/pseries/lpar.c
@@ -901,8 +901,10 @@ static int pseries_lpar_resize_hpt(unsigned long shift)
break;
case H_PARAMETER:
+ pr_warn("Invalid argument from H_RESIZE_HPT_PREPARE\n");
return -EINVAL;
case H_RESOURCE:
+ pr_warn("Operation not permitted from H_RESIZE_HPT_PREPARE\n");
return -EPERM;
default:
pr_warn("Unexpected error %d from H_RESIZE_HPT_PREPARE\n", rc);
@@ -918,7 +920,6 @@ static int pseries_lpar_resize_hpt(unsigned long shift)
if (rc != 0) {
switch (state.commit_rc) {
case H_PTEG_FULL:
- pr_warn("Hash collision while resizing HPT\n");
return -ENOSPC;
default:
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v2 03/16] KVM: PPC: Book3S HV: XIVE: introduce a new capability KVM_CAP_PPC_IRQ_XIVE
From: Cédric Le Goater @ 2019-03-13 8:34 UTC (permalink / raw)
To: Paul Mackerras; +Cc: kvm, kvm-ppc, linuxppc-dev, David Gibson
In-Reply-To: <20190225043551.GA20501@blackberry>
On 2/25/19 5:35 AM, Paul Mackerras wrote:
> On Fri, Feb 22, 2019 at 12:28:27PM +0100, Cédric Le Goater wrote:
>> The user interface exposes a new capability to let QEMU connect the
>> vCPU to the XIVE KVM device if required. The capability is only
>> advertised on a PowerNV Hypervisor as support for nested guests
>> (pseries KVM Hypervisor) is not yet available.
>
> If a bisection happened to land on this commit, we would have KVM
> saying it had the ability to support guests using XIVE natively, but
> it wouldn't actually work since we don't have all the code that is in
> the following patches.
OK. I didn't think migration was a must-have for bisection. I will move
the enablement at end.
> Thus, in order to avoid breaking bisection, you should either add the
> capability now but have it always return false until the rest of the
> code is in place, or else defer the addition of the capability until
> the end of the patch series.
I will introduce the capability early in the patchset and return false
as you are proposing. It seems to be the best approach.
>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>> index 8c69af10f91d..a38a643a24dd 100644
>> --- a/arch/powerpc/kvm/powerpc.c
>> +++ b/arch/powerpc/kvm/powerpc.c
>> @@ -570,6 +570,12 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>> case KVM_CAP_PPC_GET_CPU_CHAR:
>> r = 1;
>> break;
>> +#ifdef CONFIG_KVM_XIVE
>> + case KVM_CAP_PPC_IRQ_XIVE:
>> + /* only for PowerNV */
>> + r = !!cpu_has_feature(CPU_FTR_HVMODE);
>
> Shouldn't this be r = xive_enabled() && !!cpu_has_feature(CPU_FTR_HVMODE)
yes. we need the '__xive_enabled' toggle to be set also :/
It can set to off with the "xive=off" on the command line and on old P9
skiboot. That could be simplified one day.
> (or alternatively r = xics_on_xive(), though that would be confusing
> to the reader)?
This is correct. I didn't want to use the xics_on_xive() which is not
the capability we are activating.
I will keep the open-coded version.
> As it stands this would report true on POWER8, unless I'm missing
> something.
Ah yes. I forgot this combination also.
This should not be too complex to fix.
Thanks,
C.
^ permalink raw reply
* Re: [PATCH 2/5] ocxl: Clean up printf formats
From: Greg Kurz @ 2019-03-13 8:24 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, Andrew Donnellan,
Alastair D'Silva, Frederic Barrat, linuxppc-dev
In-Reply-To: <20190313040702.14276-3-alastair@au1.ibm.com>
On Wed, 13 Mar 2019 15:06:58 +1100
"Alastair D'Silva" <alastair@au1.ibm.com> wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> Use %# instead of using a literal '0x'
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> drivers/misc/ocxl/config.c | 6 +++---
> drivers/misc/ocxl/context.c | 2 +-
> drivers/misc/ocxl/trace.h | 10 +++++-----
> 3 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/misc/ocxl/config.c b/drivers/misc/ocxl/config.c
> index 8f2c5d8bd2ee..0ee7856b033d 100644
> --- a/drivers/misc/ocxl/config.c
> +++ b/drivers/misc/ocxl/config.c
> @@ -178,9 +178,9 @@ static int read_dvsec_vendor(struct pci_dev *dev)
> pci_read_config_dword(dev, pos + OCXL_DVSEC_VENDOR_DLX_VERS, &dlx);
>
> dev_dbg(&dev->dev, "Vendor specific DVSEC:\n");
> - dev_dbg(&dev->dev, " CFG version = 0x%x\n", cfg);
> - dev_dbg(&dev->dev, " TLX version = 0x%x\n", tlx);
> - dev_dbg(&dev->dev, " DLX version = 0x%x\n", dlx);
> + dev_dbg(&dev->dev, " CFG version = %#x\n", cfg);
> + dev_dbg(&dev->dev, " TLX version = %#x\n", tlx);
> + dev_dbg(&dev->dev, " DLX version = %#x\n", dlx);
> return 0;
> }
>
> diff --git a/drivers/misc/ocxl/context.c b/drivers/misc/ocxl/context.c
> index c10a940e3b38..3498a0199bde 100644
> --- a/drivers/misc/ocxl/context.c
> +++ b/drivers/misc/ocxl/context.c
> @@ -134,7 +134,7 @@ static vm_fault_t ocxl_mmap_fault(struct vm_fault *vmf)
> vm_fault_t ret;
>
> offset = vmf->pgoff << PAGE_SHIFT;
> - pr_debug("%s: pasid %d address 0x%lx offset 0x%llx\n", __func__,
> + pr_debug("%s: pasid %d address %#lx offset %#llx\n", __func__,
> ctx->pasid, vmf->address, offset);
>
> if (offset < ctx->afu->irq_base_offset)
> diff --git a/drivers/misc/ocxl/trace.h b/drivers/misc/ocxl/trace.h
> index bcb7ff330c1e..68bf2f173a1a 100644
> --- a/drivers/misc/ocxl/trace.h
> +++ b/drivers/misc/ocxl/trace.h
> @@ -28,7 +28,7 @@ DECLARE_EVENT_CLASS(ocxl_context,
> __entry->tidr = tidr;
> ),
>
> - TP_printk("linux pid=%d spa=0x%p pasid=0x%x pidr=0x%x tidr=0x%x",
> + TP_printk("linux pid=%d spa=%p pasid=%#x pidr=%#x tidr=%#x",
> __entry->pid,
> __entry->spa,
> __entry->pasid,
> @@ -61,7 +61,7 @@ TRACE_EVENT(ocxl_terminate_pasid,
> __entry->rc = rc;
> ),
>
> - TP_printk("pasid=0x%x rc=%d",
> + TP_printk("pasid=%#x rc=%d",
> __entry->pasid,
> __entry->rc
> )
> @@ -87,7 +87,7 @@ DECLARE_EVENT_CLASS(ocxl_fault_handler,
> __entry->tfc = tfc;
> ),
>
> - TP_printk("spa=%p pe=0x%llx dsisr=0x%llx dar=0x%llx tfc=0x%llx",
> + TP_printk("spa=%p pe=%#llx dsisr=%#llx dar=%#llx tfc=%#llx",
> __entry->spa,
> __entry->pe,
> __entry->dsisr,
> @@ -127,7 +127,7 @@ TRACE_EVENT(ocxl_afu_irq_alloc,
> __entry->irq_offset = irq_offset;
> ),
>
> - TP_printk("pasid=0x%x irq_id=%d virq=%u hw_irq=%d irq_offset=0x%llx",
> + TP_printk("pasid=%#x irq_id=%d virq=%u hw_irq=%d irq_offset=%#llx",
> __entry->pasid,
> __entry->irq_id,
> __entry->virq,
> @@ -150,7 +150,7 @@ TRACE_EVENT(ocxl_afu_irq_free,
> __entry->irq_id = irq_id;
> ),
>
> - TP_printk("pasid=0x%x irq_id=%d",
> + TP_printk("pasid=%#x irq_id=%d",
> __entry->pasid,
> __entry->irq_id
> )
^ permalink raw reply
* Re: [PATCH v2 09/16] KVM: PPC: Book3S HV: XIVE: add a control to dirty the XIVE EQ pages
From: Cédric Le Goater @ 2019-03-13 11:48 UTC (permalink / raw)
To: David Gibson; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <20190225025329.GM7668@umbus.fritz.box>
On 2/25/19 3:53 AM, David Gibson wrote:
> On Fri, Feb 22, 2019 at 12:28:33PM +0100, Cédric Le Goater wrote:
>> When migration of a VM is initiated, a first copy of the RAM is
>> transferred to the destination before the VM is stopped, but there is
>> no guarantee that the EQ pages in which the event notification are
>> queued have not been modified.
>>
>> To make sure migration will capture a consistent memory state, the
>> XIVE device should perform a XIVE quiesce sequence to stop the flow of
>> event notifications and stabilize the EQs. This is the purpose of the
>> KVM_DEV_XIVE_EQ_SYNC control which will also marks the EQ pages dirty
>> to force their transfer.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> arch/powerpc/include/uapi/asm/kvm.h | 1 +
>> arch/powerpc/kvm/book3s_xive_native.c | 67 ++++++++++++++++++++++
>> Documentation/virtual/kvm/devices/xive.txt | 29 ++++++++++
>> 3 files changed, 97 insertions(+)
>>
>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
>> index 289c504b7c1d..cd78ad1020fe 100644
>> --- a/arch/powerpc/include/uapi/asm/kvm.h
>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
>> @@ -678,6 +678,7 @@ struct kvm_ppc_cpu_char {
>> /* POWER9 XIVE Native Interrupt Controller */
>> #define KVM_DEV_XIVE_GRP_CTRL 1
>> #define KVM_DEV_XIVE_RESET 1
>> +#define KVM_DEV_XIVE_EQ_SYNC 2
>> #define KVM_DEV_XIVE_GRP_SOURCE 2 /* 64-bit source attributes */
>> #define KVM_DEV_XIVE_GRP_SOURCE_CONFIG 3 /* 64-bit source attributes */
>> #define KVM_DEV_XIVE_GRP_EQ_CONFIG 4 /* 64-bit eq attributes */
>> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
>> index dd2a9d411fe7..3debc876d5a0 100644
>> --- a/arch/powerpc/kvm/book3s_xive_native.c
>> +++ b/arch/powerpc/kvm/book3s_xive_native.c
>> @@ -640,6 +640,70 @@ static int kvmppc_xive_reset(struct kvmppc_xive *xive)
>> return 0;
>> }
>>
>> +static void kvmppc_xive_native_sync_sources(struct kvmppc_xive_src_block *sb)
>> +{
>> + int j;
>> +
>> + for (j = 0; j < KVMPPC_XICS_IRQ_PER_ICS; j++) {
>> + struct kvmppc_xive_irq_state *state = &sb->irq_state[j];
>> + struct xive_irq_data *xd;
>> + u32 hw_num;
>> +
>> + if (!state->valid)
>> + continue;
>> + if (state->act_priority == MASKED)
>
> Is this correct? If you masked an irq, then immediately did a sync,
> couldn't there still be some of the irqs in flight? I thought the
> reason we needed a sync was that masking and other such operations
> _didn't_ implicitly synchronize.
The struct kvmppc_xive_irq_state reflects the state of the EAS
configuration and not the state of the source. The source is masked
setting the PQ bits to '-Q', which is what is being done before calling
the KVM_DEV_XIVE_EQ_SYNC control.
If a source EAS is configured, OPAL syncs the XIVE IC of the source and
the XIVE IC of the previous target if any.
So I think we are fine.
C.
>> + continue;
>> +
>> + arch_spin_lock(&sb->lock);
>> + kvmppc_xive_select_irq(state, &hw_num, &xd);
>> + xive_native_sync_source(hw_num);
>> + xive_native_sync_queue(hw_num);
>> + arch_spin_unlock(&sb->lock);
>> + }
>> +}
>> +
>> +static int kvmppc_xive_native_vcpu_eq_sync(struct kvm_vcpu *vcpu)
>> +{
>> + struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
>> + unsigned int prio;
>> +
>> + if (!xc)
>> + return -ENOENT;
>> +
>> + for (prio = 0; prio < KVMPPC_XIVE_Q_COUNT; prio++) {
>> + struct xive_q *q = &xc->queues[prio];
>> +
>> + if (!q->qpage)
>> + continue;
>> +
>> + /* Mark EQ page dirty for migration */
>> + mark_page_dirty(vcpu->kvm, gpa_to_gfn(q->guest_qpage));
>> + }
>> + return 0;
>> +}
>> +
>> +static int kvmppc_xive_native_eq_sync(struct kvmppc_xive *xive)
>> +{
>> + struct kvm *kvm = xive->kvm;
>> + struct kvm_vcpu *vcpu;
>> + unsigned int i;
>> +
>> + pr_devel("%s\n", __func__);
>> +
>> + for (i = 0; i <= xive->max_sbid; i++) {
>> + if (xive->src_blocks[i])
>> + kvmppc_xive_native_sync_sources(xive->src_blocks[i]);
>> + }
>> +
>> + mutex_lock(&kvm->lock);
>> + kvm_for_each_vcpu(i, vcpu, kvm) {
>> + kvmppc_xive_native_vcpu_eq_sync(vcpu);
>> + }
>> + mutex_unlock(&kvm->lock);
>> +
>> + return 0;
>> +}
>> +
>> static int kvmppc_xive_native_set_attr(struct kvm_device *dev,
>> struct kvm_device_attr *attr)
>> {
>> @@ -650,6 +714,8 @@ static int kvmppc_xive_native_set_attr(struct kvm_device *dev,
>> switch (attr->attr) {
>> case KVM_DEV_XIVE_RESET:
>> return kvmppc_xive_reset(xive);
>> + case KVM_DEV_XIVE_EQ_SYNC:
>> + return kvmppc_xive_native_eq_sync(xive);
>> }
>> break;
>> case KVM_DEV_XIVE_GRP_SOURCE:
>> @@ -688,6 +754,7 @@ static int kvmppc_xive_native_has_attr(struct kvm_device *dev,
>> case KVM_DEV_XIVE_GRP_CTRL:
>> switch (attr->attr) {
>> case KVM_DEV_XIVE_RESET:
>> + case KVM_DEV_XIVE_EQ_SYNC:
>> return 0;
>> }
>> break;
>> diff --git a/Documentation/virtual/kvm/devices/xive.txt b/Documentation/virtual/kvm/devices/xive.txt
>> index 267634eae9e0..a26be635cff9 100644
>> --- a/Documentation/virtual/kvm/devices/xive.txt
>> +++ b/Documentation/virtual/kvm/devices/xive.txt
>> @@ -23,6 +23,12 @@ the legacy interrupt mode, referred as XICS (POWER7/8).
>> queues. To be used by kexec and kdump.
>> Errors: none
>>
>> + 1.2 KVM_DEV_XIVE_EQ_SYNC (write only)
>> + Sync all the sources and queues and mark the EQ pages dirty. This
>> + to make sure that a consistent memory state is captured when
>> + migrating the VM.
>> + Errors: none
>> +
>> 2. KVM_DEV_XIVE_GRP_SOURCE (write only)
>> Initializes a new source in the XIVE device and mask it.
>> Attributes:
>> @@ -95,3 +101,26 @@ the legacy interrupt mode, referred as XICS (POWER7/8).
>> -ENOENT: Unknown source number
>> -EINVAL: Not initialized source number, invalid priority or
>> invalid CPU number.
>> +
>> +* Migration:
>> +
>> + Saving the state of a VM using the XIVE native exploitation mode
>> + should follow a specific sequence. When the VM is stopped :
>> +
>> + 1. Mask all sources (PQ=01) to stop the flow of events.
>> +
>> + 2. Sync the XIVE device with the KVM control KVM_DEV_XIVE_EQ_SYNC to
>> + flush any in-flight event notification and to stabilize the EQs. At
>> + this stage, the EQ pages are marked dirty to make sure they are
>> + transferred in the migration sequence.
>> +
>> + 3. Capture the state of the source targeting, the EQs configuration
>> + and the state of thread interrupt context registers.
>> +
>> + Restore is similar :
>> +
>> + 1. Restore the EQ configuration. As targeting depends on it.
>> + 2. Restore targeting
>> + 3. Restore the thread interrupt contexts
>> + 4. Restore the source states
>> + 5. Let the vCPU run
>
^ permalink raw reply
* BUG: p8_aes_ctr randomly returns wrong results
From: Ondrej Mosnáček @ 2019-03-13 12:37 UTC (permalink / raw)
To: linux-crypto, Herbert Xu
Cc: marcelo.cerri, Stephan Mueller, leo.barbosa, linuxppc-dev,
Paulo Flabiano Smorigo
Hi,
FYI, the p8_aes_ctr crypto driver (drivers/crypto/vmx/aes_ctr.c) seems
to be seriously broken. When I do repeated encryption using libkcapi
multiple times in a row, I sometimes get a wrong result. This happens
more often with long messages (e.g. at 16 KiB it already happens very
frequently).
To reproduce:
1. Install or locally build libkcapi [1] (you will need the kcapi-enc
binary in PATH) on a ppc64le system.
2. Run the following in bash:
for i in {1..100}; do head -c $((16*1024)) /dev/zero | kcapi-enc -e -c
'ctr(aes)' -p test -s test --pbkdfiter 1 2>/dev/null | sha256sum; done
| sort -u
Expected result:
All invocations produce output with identical checksum.
Actual result:
Multiple different checksums are produced.
When I run 'rmmod vmx_crypto' before running the reproducer, I get
only one (correct) checksum, so this is definitely a bug in the
driver. Other ciphers (cbc(aes), xts(aes)) are not affected, even
though the glue code is very similar. That leads me to believe the
problem is somewhere in the assembly code.
[1] http://github.com/smuellerDD/libkcapi
Cheers,
Ondrej
^ 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