* Re: [PATCH 04/11] KVM: PPC: fix incorrect element_size for stxsiwx in analyse_instr
From: Paul Mackerras @ 2018-05-03 5:50 UTC (permalink / raw)
To: wei.guo.simon; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <1524657284-16706-5-git-send-email-wei.guo.simon@gmail.com>
On Wed, Apr 25, 2018 at 07:54:37PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
>
> stwsiwx will place contents of word element 1 of VSR into word
> storage of EA. So the element size of stwsiwx should be 4.
>
> This patch correct the size from 8 to 4.
>
> Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
> ---
> arch/powerpc/lib/sstep.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index 34d68f1..151d484 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -2178,7 +2178,7 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
> case 140: /* stxsiwx */
> op->reg = rd | ((instr & 1) << 5);
> op->type = MKOP(STORE_VSX, 0, 4);
> - op->element_size = 8;
> + op->element_size = 4;
I made the element_size be 8 deliberately because this way, with
size=4 but element_size=8, the code will naturally choose the correct
word (the least-significant word of the left half) of the register to
store into memory. With this change you then need the special case in
a later patch for stxsiwx, which you shouldn't need if you don't make
this change.
Paul.
^ permalink raw reply
* Re: [PATCH 07/11] KVM: PPC: reconstruct non-SIMD LOAD/STORE instruction mmio emulation with analyse_intr() input
From: Paul Mackerras @ 2018-05-03 6:03 UTC (permalink / raw)
To: wei.guo.simon; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <1524657284-16706-8-git-send-email-wei.guo.simon@gmail.com>
On Wed, Apr 25, 2018 at 07:54:40PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
>
> This patch reconstructs non-SIMD LOAD/STORE instruction MMIO emulation
> with analyse_intr() input. It utilizes the BYTEREV/UPDATE/SIGNEXT
> properties exported by analyse_instr() and invokes
> kvmppc_handle_load(s)/kvmppc_handle_store() accordingly.
>
> It also move CACHEOP type handling into the skeleton.
>
> instruction_type within sstep.h is renamed to avoid conflict with
> kvm_ppc.h.
I'd prefer to change the one in kvm_ppc.h, especially since that one
isn't exactly about the type of instruction, but more about the type
of interrupt led to us trying to fetch the instruction.
> Suggested-by: Paul Mackerras <paulus@ozlabs.org>
> Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
> ---
> arch/powerpc/include/asm/sstep.h | 2 +-
> arch/powerpc/kvm/emulate_loadstore.c | 282 +++++++----------------------------
> 2 files changed, 51 insertions(+), 233 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
> index ab9d849..0a1a312 100644
> --- a/arch/powerpc/include/asm/sstep.h
> +++ b/arch/powerpc/include/asm/sstep.h
> @@ -23,7 +23,7 @@
> #define IS_RFID(instr) (((instr) & 0xfc0007fe) == 0x4c000024)
> #define IS_RFI(instr) (((instr) & 0xfc0007fe) == 0x4c000064)
>
> -enum instruction_type {
> +enum analyse_instruction_type {
> COMPUTE, /* arith/logical/CR op, etc. */
> LOAD, /* load and store types need to be contiguous */
> LOAD_MULTI,
> diff --git a/arch/powerpc/kvm/emulate_loadstore.c b/arch/powerpc/kvm/emulate_loadstore.c
> index 90b9692..aaaf872 100644
> --- a/arch/powerpc/kvm/emulate_loadstore.c
> +++ b/arch/powerpc/kvm/emulate_loadstore.c
> @@ -31,9 +31,12 @@
> #include <asm/kvm_ppc.h>
> #include <asm/disassemble.h>
> #include <asm/ppc-opcode.h>
> +#include <asm/sstep.h>
> #include "timing.h"
> #include "trace.h"
>
> +int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
> + unsigned int instr);
You shouldn't need this prototype here, since there's one in sstep.h.
> #ifdef CONFIG_PPC_FPU
> static bool kvmppc_check_fp_disabled(struct kvm_vcpu *vcpu)
> {
> @@ -84,8 +87,9 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
> struct kvm_run *run = vcpu->run;
> u32 inst;
> int ra, rs, rt;
> - enum emulation_result emulated;
> + enum emulation_result emulated = EMULATE_FAIL;
> int advance = 1;
> + struct instruction_op op;
>
> /* this default type might be overwritten by subcategories */
> kvmppc_set_exit_type(vcpu, EMULATED_INST_EXITS);
> @@ -114,144 +118,64 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
> vcpu->arch.mmio_update_ra = 0;
> vcpu->arch.mmio_host_swabbed = 0;
>
> - switch (get_op(inst)) {
> - case 31:
> - switch (get_xop(inst)) {
> - case OP_31_XOP_LWZX:
> - emulated = kvmppc_handle_load(run, vcpu, rt, 4, 1);
> - break;
> -
> - case OP_31_XOP_LWZUX:
> - emulated = kvmppc_handle_load(run, vcpu, rt, 4, 1);
> - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> - break;
> -
> - case OP_31_XOP_LBZX:
> - emulated = kvmppc_handle_load(run, vcpu, rt, 1, 1);
> - break;
> + emulated = EMULATE_FAIL;
> + vcpu->arch.regs.msr = vcpu->arch.shared->msr;
> + vcpu->arch.regs.ccr = vcpu->arch.cr;
> + if (analyse_instr(&op, &vcpu->arch.regs, inst) == 0) {
> + int type = op.type & INSTR_TYPE_MASK;
> + int size = GETSIZE(op.type);
>
> - case OP_31_XOP_LBZUX:
> - emulated = kvmppc_handle_load(run, vcpu, rt, 1, 1);
> - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> - break;
> + switch (type) {
> + case LOAD: {
> + int instr_byte_swap = op.type & BYTEREV;
>
> - case OP_31_XOP_STDX:
> - emulated = kvmppc_handle_store(run, vcpu,
> - kvmppc_get_gpr(vcpu, rs), 8, 1);
> - break;
> + if (op.type & UPDATE) {
> + vcpu->arch.mmio_ra = op.update_reg;
> + vcpu->arch.mmio_update_ra = 1;
> + }
Any reason we can't just update RA here immediately?
>
> - case OP_31_XOP_STDUX:
> - emulated = kvmppc_handle_store(run, vcpu,
> - kvmppc_get_gpr(vcpu, rs), 8, 1);
> - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> - break;
> -
> - case OP_31_XOP_STWX:
> - emulated = kvmppc_handle_store(run, vcpu,
> - kvmppc_get_gpr(vcpu, rs), 4, 1);
> - break;
> -
> - case OP_31_XOP_STWUX:
> - emulated = kvmppc_handle_store(run, vcpu,
> - kvmppc_get_gpr(vcpu, rs), 4, 1);
> - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> - break;
> -
> - case OP_31_XOP_STBX:
> - emulated = kvmppc_handle_store(run, vcpu,
> - kvmppc_get_gpr(vcpu, rs), 1, 1);
> - break;
> -
> - case OP_31_XOP_STBUX:
> - emulated = kvmppc_handle_store(run, vcpu,
> - kvmppc_get_gpr(vcpu, rs), 1, 1);
> - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> - break;
> -
> - case OP_31_XOP_LHAX:
> - emulated = kvmppc_handle_loads(run, vcpu, rt, 2, 1);
> - break;
> -
> - case OP_31_XOP_LHAUX:
> - emulated = kvmppc_handle_loads(run, vcpu, rt, 2, 1);
> - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> - break;
> + if (op.type & SIGNEXT)
> + emulated = kvmppc_handle_loads(run, vcpu,
> + op.reg, size, !instr_byte_swap);
> + else
> + emulated = kvmppc_handle_load(run, vcpu,
> + op.reg, size, !instr_byte_swap);
>
> - case OP_31_XOP_LHZX:
> - emulated = kvmppc_handle_load(run, vcpu, rt, 2, 1);
> break;
> -
> - case OP_31_XOP_LHZUX:
> - emulated = kvmppc_handle_load(run, vcpu, rt, 2, 1);
> - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> - break;
> -
> - case OP_31_XOP_STHX:
> - emulated = kvmppc_handle_store(run, vcpu,
> - kvmppc_get_gpr(vcpu, rs), 2, 1);
> - break;
> -
> - case OP_31_XOP_STHUX:
> - emulated = kvmppc_handle_store(run, vcpu,
> - kvmppc_get_gpr(vcpu, rs), 2, 1);
> - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> - break;
> -
> - case OP_31_XOP_DCBST:
> - case OP_31_XOP_DCBF:
> - case OP_31_XOP_DCBI:
> + }
> + case STORE:
> + if (op.type & UPDATE) {
> + vcpu->arch.mmio_ra = op.update_reg;
> + vcpu->arch.mmio_update_ra = 1;
> + }
Same comment again about updating RA.
> +
> + /* if need byte reverse, op.val has been reverted by
"reversed" rather than "reverted". "Reverted" means put back to a
former state.
Paul.
^ permalink raw reply
* Re: [PATCH 03/11] KVM: PPC: Fix a mmio_host_swabbed uninitialized usage issue when VMX store
From: Paul Mackerras @ 2018-05-03 5:48 UTC (permalink / raw)
To: wei.guo.simon; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <1524657284-16706-4-git-send-email-wei.guo.simon@gmail.com>
On Wed, Apr 25, 2018 at 07:54:36PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
>
> When KVM emulates VMX store, it will invoke kvmppc_get_vmx_data() to
> retrieve VMX reg val. kvmppc_get_vmx_data() will check mmio_host_swabbed
> to decide which double word of vr[] to be used. But the
> mmio_host_swabbed can be uninitiazed during VMX store procedure:
>
> kvmppc_emulate_loadstore
> \- kvmppc_handle_store128_by2x64
> \- kvmppc_get_vmx_data
>
> This patch corrects this by using kvmppc_need_byteswap() to choose
> double word of vr[] and initialized mmio_host_swabbed to avoid invisble
> trouble.
>
> Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
The patch is correct, but I think the patch description needs to say
that vcpu->arch.mmio_host_swabbed is not meant to be used at all for
emulation of store instructions, and this patch makes that true for
VMX stores.
Paul.
^ permalink raw reply
* Re: [PATCH 01/11] KVM: PPC: add pt_regs into kvm_vcpu_arch and move vcpu->arch.gpr[] into it
From: Paul Mackerras @ 2018-05-03 5:34 UTC (permalink / raw)
To: wei.guo.simon; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <1524657284-16706-2-git-send-email-wei.guo.simon@gmail.com>
On Wed, Apr 25, 2018 at 07:54:34PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
>
> Current regs are scattered at kvm_vcpu_arch structure and it will
> be more neat to organize them into pt_regs structure.
>
> Also it will enable reconstruct MMIO emulation code with
"reimplement" would be clearer than "reconstruct" here, I think.
> @@ -438,7 +438,7 @@ int main(void)
> OFFSET(VCPU_TAR, kvm_vcpu, arch.tar);
> #endif
> OFFSET(VCPU_CR, kvm_vcpu, arch.cr);
> - OFFSET(VCPU_PC, kvm_vcpu, arch.pc);
> + OFFSET(VCPU_PC, kvm_vcpu, arch.nip);
This hunk shouldn't be in this patch.
Paul.
^ permalink raw reply
* Re: [PATCH 02/11] KVM: PPC: mov nip/ctr/lr/xer registers to pt_regs in kvm_vcpu_arch
From: Paul Mackerras @ 2018-05-03 5:46 UTC (permalink / raw)
To: wei.guo.simon; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <1524657284-16706-3-git-send-email-wei.guo.simon@gmail.com>
On Wed, Apr 25, 2018 at 07:54:35PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
>
> This patch moves nip/ctr/lr/xer registers from scattered places in
> kvm_vcpu_arch to pt_regs structure.
>
> cr register is "unsigned long" in pt_regs and u32 in vcpu->arch.
> It will need more consideration and may move in later patches.
>
> Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
Mostly looks fine; some nits below.
> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> index e8a78a5..731f7d4 100644
> --- a/arch/powerpc/kernel/asm-offsets.c
> +++ b/arch/powerpc/kernel/asm-offsets.c
> @@ -431,14 +431,14 @@ int main(void)
> #ifdef CONFIG_ALTIVEC
> OFFSET(VCPU_VRS, kvm_vcpu, arch.vr.vr);
> #endif
> - OFFSET(VCPU_XER, kvm_vcpu, arch.xer);
> - OFFSET(VCPU_CTR, kvm_vcpu, arch.ctr);
> - OFFSET(VCPU_LR, kvm_vcpu, arch.lr);
> + OFFSET(VCPU_XER, kvm_vcpu, arch.regs.xer);
> + OFFSET(VCPU_CTR, kvm_vcpu, arch.regs.ctr);
> + OFFSET(VCPU_LR, kvm_vcpu, arch.regs.link);
> #ifdef CONFIG_PPC_BOOK3S
> OFFSET(VCPU_TAR, kvm_vcpu, arch.tar);
> #endif
> - OFFSET(VCPU_CR, kvm_vcpu, arch.cr);
> - OFFSET(VCPU_PC, kvm_vcpu, arch.nip);
This should be arch.pc; arch.nip doesn't exist.
> + OFFSET(VCPU_CR, kvm_vcpu, arch.regs.ccr);
I thought the patch description said you weren't moving CR at this
stage?
> + OFFSET(VCPU_PC, kvm_vcpu, arch.regs.nip);
> #ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> OFFSET(VCPU_MSR, kvm_vcpu, arch.shregs.msr);
> OFFSET(VCPU_SRR0, kvm_vcpu, arch.shregs.srr0);
> @@ -693,11 +693,11 @@ int main(void)
> #endif /* CONFIG_PPC_BOOK3S_64 */
>
> #else /* CONFIG_PPC_BOOK3S */
> - OFFSET(VCPU_CR, kvm_vcpu, arch.cr);
> - OFFSET(VCPU_XER, kvm_vcpu, arch.xer);
> - OFFSET(VCPU_LR, kvm_vcpu, arch.lr);
> - OFFSET(VCPU_CTR, kvm_vcpu, arch.ctr);
> - OFFSET(VCPU_PC, kvm_vcpu, arch.pc);
> + OFFSET(VCPU_CR, kvm_vcpu, arch.regs.ccr);
Once again VCPU_CR should not be changed.
> + OFFSET(VCPU_XER, kvm_vcpu, arch.regs.xer);
> + OFFSET(VCPU_LR, kvm_vcpu, arch.regs.link);
> + OFFSET(VCPU_CTR, kvm_vcpu, arch.regs.ctr);
> + OFFSET(VCPU_PC, kvm_vcpu, arch.regs.nip);
> OFFSET(VCPU_SPRG9, kvm_vcpu, arch.sprg9);
> OFFSET(VCPU_LAST_INST, kvm_vcpu, arch.last_inst);
> OFFSET(VCPU_FAULT_DEAR, kvm_vcpu, arch.fault_dear);
Paul.
^ permalink raw reply
* Re: [PATCH 10/11] KVM: PPC: reconstruct LOAD_VMX/STORE_VMX instruction mmio emulation with analyse_intr() input
From: Paul Mackerras @ 2018-05-03 6:17 UTC (permalink / raw)
To: wei.guo.simon; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <1524657284-16706-11-git-send-email-wei.guo.simon@gmail.com>
On Wed, Apr 25, 2018 at 07:54:43PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
>
> This patch reconstructs LOAD_VMX/STORE_VMX instruction MMIO emulation with
> analyse_intr() input. When emulating the store, the VMX reg will need to
> be flushed so that the right reg val can be retrieved before writing to
> IO MEM.
>
> Suggested-by: Paul Mackerras <paulus@ozlabs.org>
> Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
This looks fine for lvx and stvx, but now we are also doing something
for the vector element loads and stores (lvebx, stvebx, lvehx, stvehx,
etc.) without having the logic to insert or extract the correct
element in the vector register image. We need either to generate an
error for the element load/store instructions, or handle them
correctly.
> diff --git a/arch/powerpc/kvm/emulate_loadstore.c b/arch/powerpc/kvm/emulate_loadstore.c
> index 2dbdf9a..0bfee2f 100644
> --- a/arch/powerpc/kvm/emulate_loadstore.c
> +++ b/arch/powerpc/kvm/emulate_loadstore.c
> @@ -160,6 +160,27 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
> KVM_MMIO_REG_FPR|op.reg, size, 1);
> break;
> #endif
> +#ifdef CONFIG_ALTIVEC
> + case LOAD_VMX:
> + if (kvmppc_check_altivec_disabled(vcpu))
> + return EMULATE_DONE;
> +
> + /* VMX access will need to be size aligned */
This comment isn't quite right; it isn't that the address needs to be
size-aligned, it's that the hardware forcibly aligns it. So I would
say something like /* Hardware enforces alignment of VMX accesses */.
> + vcpu->arch.vaddr_accessed &= ~((unsigned long)size - 1);
> + vcpu->arch.paddr_accessed &= ~((unsigned long)size - 1);
> +
> + if (size == 16) {
> + vcpu->arch.mmio_vmx_copy_nums = 2;
> + emulated = kvmppc_handle_load128_by2x64(run,
> + vcpu, KVM_MMIO_REG_VMX|op.reg,
> + 1);
> + } else if (size <= 8)
> + emulated = kvmppc_handle_load(run, vcpu,
> + KVM_MMIO_REG_VMX|op.reg,
> + size, 1);
> +
> + break;
> +#endif
> case STORE:
> if (op.type & UPDATE) {
> vcpu->arch.mmio_ra = op.update_reg;
> @@ -197,6 +218,36 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
> VCPU_FPR(vcpu, op.reg), size, 1);
> break;
> #endif
> +#ifdef CONFIG_ALTIVEC
> + case STORE_VMX:
> + if (kvmppc_check_altivec_disabled(vcpu))
> + return EMULATE_DONE;
> +
> + /* VMX access will need to be size aligned */
> + vcpu->arch.vaddr_accessed &= ~((unsigned long)size - 1);
> + vcpu->arch.paddr_accessed &= ~((unsigned long)size - 1);
> +
> + /* if it is PR KVM, the FP/VEC/VSX registers need to
> + * be flushed so that kvmppc_handle_store() can read
> + * actual VMX vals from vcpu->arch.
> + */
> + if (!is_kvmppc_hv_enabled(vcpu->kvm))
As before, I suggest just testing that the function pointer isn't
NULL.
> + vcpu->kvm->arch.kvm_ops->giveup_ext(vcpu,
> + MSR_VEC);
> +
> + if (size == 16) {
> + vcpu->arch.mmio_vmx_copy_nums = 2;
> + emulated = kvmppc_handle_store128_by2x64(run,
> + vcpu, op.reg, 1);
> + } else if (size <= 8) {
> + u64 val;
> +
> + kvmppc_get_vmx_data(vcpu, op.reg, &val);
> + emulated = kvmppc_handle_store(run, vcpu,
> + val, size, 1);
> + }
> + break;
> +#endif
> case CACHEOP:
> /* Do nothing. The guest is performing dcbi because
> * hardware DMA is not snooped by the dcache, but
> @@ -354,28 +405,6 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
> break;
> #endif /* CONFIG_VSX */
>
> -#ifdef CONFIG_ALTIVEC
> - case OP_31_XOP_LVX:
> - if (kvmppc_check_altivec_disabled(vcpu))
> - return EMULATE_DONE;
> - vcpu->arch.vaddr_accessed &= ~0xFULL;
> - vcpu->arch.paddr_accessed &= ~0xFULL;
> - vcpu->arch.mmio_vmx_copy_nums = 2;
> - emulated = kvmppc_handle_load128_by2x64(run, vcpu,
> - KVM_MMIO_REG_VMX|rt, 1);
> - break;
> -
> - case OP_31_XOP_STVX:
> - if (kvmppc_check_altivec_disabled(vcpu))
> - return EMULATE_DONE;
> - vcpu->arch.vaddr_accessed &= ~0xFULL;
> - vcpu->arch.paddr_accessed &= ~0xFULL;
> - vcpu->arch.mmio_vmx_copy_nums = 2;
> - emulated = kvmppc_handle_store128_by2x64(run, vcpu,
> - rs, 1);
> - break;
> -#endif /* CONFIG_ALTIVEC */
> -
> default:
> emulated = EMULATE_FAIL;
> break;
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index e724601..000182e 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -1408,7 +1408,7 @@ int kvmppc_handle_load128_by2x64(struct kvm_run *run, struct kvm_vcpu *vcpu,
> return emulated;
> }
>
> -static inline int kvmppc_get_vmx_data(struct kvm_vcpu *vcpu, int rs, u64 *val)
> +int kvmppc_get_vmx_data(struct kvm_vcpu *vcpu, int rs, u64 *val)
> {
> vector128 vrs = VCPU_VSX_VR(vcpu, rs);
> u32 di;
> --
> 1.8.3.1
Paul.
^ permalink raw reply
* Re: [PATCH 05/11] KVM: PPC: add GPR RA update skeleton for MMIO emulation
From: Paul Mackerras @ 2018-05-03 5:58 UTC (permalink / raw)
To: wei.guo.simon; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <1524657284-16706-6-git-send-email-wei.guo.simon@gmail.com>
On Wed, Apr 25, 2018 at 07:54:38PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
>
> To optimize kvm emulation code with analyse_instr, adds new
> mmio_update_ra flag to aid with GPR RA update.
>
> This patch arms RA update at load/store emulation path for both
> qemu mmio emulation or coalesced mmio emulation.
It's not clear to me why you need to do this. The existing code
writes RA at the point where the instruction is decoded. In later
patches, you change that so the RA update occurs after the MMIO
operation is performed. Is there a particular reason why you made
that change?
Paul.
^ permalink raw reply
* Re: [PATCH 09/11] KVM: PPC: reconstruct LOAD_FP/STORE_FP instruction mmio emulation with analyse_intr() input
From: Paul Mackerras @ 2018-05-03 6:10 UTC (permalink / raw)
To: wei.guo.simon; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <1524657284-16706-10-git-send-email-wei.guo.simon@gmail.com>
On Wed, Apr 25, 2018 at 07:54:42PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
>
> This patch reconstructs LOAD_FP/STORE_FP instruction MMIO emulation with
> analyse_intr() input. It utilizes the FPCONV/UPDATE properties exported by
> analyse_instr() and invokes kvmppc_handle_load(s)/kvmppc_handle_store()
> accordingly.
>
> The FP regs need to be flushed so that the right FP reg vals can be read
> from vcpu->arch.fpr.
This only applies for store instructions; it would be clearer if you
said that explicitly.
>
> Suggested-by: Paul Mackerras <paulus@ozlabs.org>
> Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
Same comment about updating RA as for the other patches. Otherwise
this looks fine.
Paul.
^ permalink raw reply
* Re: [PATCH 11/11] KVM: PPC: reconstruct LOAD_VSX/STORE_VSX instruction mmio emulation with analyse_intr() input
From: Paul Mackerras @ 2018-05-03 6:26 UTC (permalink / raw)
To: wei.guo.simon; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <1524657284-16706-12-git-send-email-wei.guo.simon@gmail.com>
On Wed, Apr 25, 2018 at 07:54:44PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
>
> This patch reconstructs LOAD_VSX/STORE_VSX instruction MMIO emulation with
> analyse_intr() input. It utilizes VSX_FPCONV/VSX_SPLAT/SIGNEXT exported
> by analyse_instr() and handle accordingly.
>
> When emulating VSX store, the VSX reg will need to be flushed so that
> the right reg val can be retrieved before writing to IO MEM.
>
> Suggested-by: Paul Mackerras <paulus@ozlabs.org>
> Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
Looks good, except that you shouldn't need the special case for
stxsiwx. With size=4 and element_size=8, kvmppc_handle_vsx_store
should just do the right thing, as far as I can see.
Paul.
^ permalink raw reply
* [PATCH kernel v2] powerpc/ioda: Use ibm, supported-tce-sizes for IOMMU page size mask
From: Alexey Kardashevskiy @ 2018-05-03 4:29 UTC (permalink / raw)
To: linuxppc-dev
Cc: Alexey Kardashevskiy, David Gibson, Michael Ellerman,
Russell Currey
At the moment we assume that IODA2 and newer PHBs can always do 4K/64K/16M
IOMMU pages, however this is not the case for POWER9 and now skiboot
advertises the supported sizes via the device so we use that instead
of hard coding the mask.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v2:
* added quirk for POWER8 to advertise 16M if skiboot has not provided info
---
arch/powerpc/platforms/powernv/pci-ioda.c | 26 +++++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 3f9c69d..891b4b6 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -2910,6 +2910,30 @@ static void pnv_pci_ioda2_table_free_pages(struct iommu_table *tbl)
tbl->it_indirect_levels);
}
+static unsigned long pnv_ioda_parse_tce_sizes(struct pnv_phb *phb)
+{
+ struct pci_controller *hose = phb->hose;
+ struct device_node *dn = hose->dn;
+ int i, len = 0;
+ const __be32 *r;
+ unsigned long mask = 0;
+
+ r = of_get_property(dn, "ibm,supported-tce-sizes", &len);
+ if (!r || !len) {
+ mask = SZ_4K | SZ_64K;
+ /* Add 16M for POWER8 by default */
+ if (cpu_has_feature(CPU_FTR_ARCH_207S) &&
+ !cpu_has_feature(CPU_FTR_ARCH_300))
+ mask |= SZ_16M;
+ return mask;
+ }
+
+ for (i = 0; i < len / sizeof(*r); ++i)
+ mask |= 1ULL << be32_to_cpu(r[i]);
+
+ return mask;
+}
+
static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb,
struct pnv_ioda_pe *pe)
{
@@ -2934,7 +2958,7 @@ static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb,
pe->table_group.max_dynamic_windows_supported =
IOMMU_TABLE_GROUP_MAX_TABLES;
pe->table_group.max_levels = POWERNV_IOMMU_MAX_LEVELS;
- pe->table_group.pgsizes = SZ_4K | SZ_64K | SZ_16M;
+ pe->table_group.pgsizes = pnv_ioda_parse_tce_sizes(phb);
#ifdef CONFIG_IOMMU_API
pe->table_group.ops = &pnv_pci_ioda2_ops;
#endif
--
2.11.0
^ permalink raw reply related
* Re: [PATCH RFC 1/1] KVM: PPC: Book3S HV: pack VCORE IDs to access full VCPU ID space
From: Sam Bobroff @ 2018-05-03 3:38 UTC (permalink / raw)
To: David Gibson
Cc: Sam Bobroff, kvm-ppc, paulus, linuxppc-dev, Cédric Le Goater,
kvm
In-Reply-To: <20180503031113.GM13229@umbus.fritz.box>
[-- Attachment #1: Type: text/plain, Size: 9018 bytes --]
On Thu, May 03, 2018 at 01:11:13PM +1000, David Gibson wrote:
> On Tue, May 01, 2018 at 02:52:21PM +1000, Sam Bobroff wrote:
> > On Tue, Apr 24, 2018 at 01:48:25PM +1000, David Gibson wrote:
> > > On Tue, Apr 24, 2018 at 01:19:15PM +1000, Sam Bobroff wrote:
> > > > On Mon, Apr 23, 2018 at 11:06:35AM +0200, Cédric Le Goater wrote:
> > > > > On 04/16/2018 06:09 AM, David Gibson wrote:
> [snip]
> > > > At the moment, kvm->vcores[] and xive->vp_base are both sized by NR_CPUS
> > > > (via KVM_MAX_VCPUS and KVM_MAX_VCORES which are both NR_CPUS). This is
> > > > enough space for the maximum number of VCPUs, and some space is wasted
> > > > when the guest uses less than this (but KVM doesn't know how many will
> > > > be created, so we can't do better easily). The problem is that the
> > > > indicies overflow before all of those VCPUs can be created, not that
> > > > more space is needed.
> > > >
> > > > We could fix the overflow by expanding these areas to KVM_MAX_VCPU_ID
> > > > but that will use 8x the space we use now, and we know that no more than
> > > > KVM_MAX_VCPUS will be used so all this new space is basically wasted.
> > > >
> > > > So remapping seems better if it will work. (Ben H. was strongly against
> > > > wasting more XIVE space if possible.)
> > >
> > > Hm, ok. Are the relevant arrays here per-VM, or global? Or some of both?
> >
> > Per-VM. They are the kvm->vcores[] array and the blocks of memory
> > pointed to by xive->vp_base.
>
> Hm. If it were global (where you can't know the size of a specific
> VM) I'd certainly see the concern about not expanding the size of the
> array.
>
> As it is, I'm a little perplexed that we care so much about the
> difference between KVM_MAX_VCPUS and KVM_MAX_VCPU_ID, a factor of 8,
> when we apparently don't care about the difference between the vm's
> actual number of cpus and KVM_MAX_VCPUS, a factor of maybe 2048 (for a
> 1vcpu guest and powernv_defconfig).
I agree, and we should do better (more because of the XIVE area than the
vcores array), but that will require a coordinated change to QEMU and
KVM to export the information KVM needs and that's going to be more
complicated. So basically, yes, this is only partially fixing it but
it's easy to do.
As for how we could solve the bigger problem; I've discussed with Paul
the idea of adding the guest's threading mode as a second parameter when
QEMU sets KVM_CAP_SMT to set the VSMT mode but that only helps with
packing; KVM still needs to know the maximum number of (hot pluggable)
CPUs so we'll need some other extension as well.
> > > > In short, remapping provides a way to allow the guest to create it's full set
> > > > of VCPUs without wasting any more space than we do currently, without
> > > > having to do something more complicated like tracking used IDs or adding
> > > > additional KVM CAPs.
> > > >
> > > > > >> +
> > > > > >> #endif /* __ASM_KVM_BOOK3S_H__ */
> > > > > >> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> > > > > >> index 9cb9448163c4..49165cc90051 100644
> > > > > >> --- a/arch/powerpc/kvm/book3s_hv.c
> > > > > >> +++ b/arch/powerpc/kvm/book3s_hv.c
> > > > > >> @@ -1762,7 +1762,7 @@ static int threads_per_vcore(struct kvm *kvm)
> > > > > >> return threads_per_subcore;
> > > > > >> }
> > > > > >>
> > > > > >> -static struct kvmppc_vcore *kvmppc_vcore_create(struct kvm *kvm, int core)
> > > > > >> +static struct kvmppc_vcore *kvmppc_vcore_create(struct kvm *kvm, int id)
> > > > > >> {
> > > > > >> struct kvmppc_vcore *vcore;
> > > > > >>
> > > > > >> @@ -1776,7 +1776,7 @@ static struct kvmppc_vcore *kvmppc_vcore_create(struct kvm *kvm, int core)
> > > > > >> init_swait_queue_head(&vcore->wq);
> > > > > >> vcore->preempt_tb = TB_NIL;
> > > > > >> vcore->lpcr = kvm->arch.lpcr;
> > > > > >> - vcore->first_vcpuid = core * kvm->arch.smt_mode;
> > > > > >> + vcore->first_vcpuid = id;
> > > > > >> vcore->kvm = kvm;
> > > > > >> INIT_LIST_HEAD(&vcore->preempt_list);
> > > > > >>
> > > > > >> @@ -1992,12 +1992,18 @@ static struct kvm_vcpu *kvmppc_core_vcpu_create_hv(struct kvm *kvm,
> > > > > >> mutex_lock(&kvm->lock);
> > > > > >> vcore = NULL;
> > > > > >> err = -EINVAL;
> > > > > >> - core = id / kvm->arch.smt_mode;
> > > > > >> + if (cpu_has_feature(CPU_FTR_ARCH_300)) {
> > > > > >> + BUG_ON(kvm->arch.smt_mode != 1);
> > > > > >> + core = kvmppc_pack_vcpu_id(kvm, id);
> > > > > >> + } else {
> > > > > >> + core = id / kvm->arch.smt_mode;
> > > > > >> + }
> > > > > >> if (core < KVM_MAX_VCORES) {
> > > > > >> vcore = kvm->arch.vcores[core];
> > > > > >> + BUG_ON(cpu_has_feature(CPU_FTR_ARCH_300) && vcore);
> > > > > >> if (!vcore) {
> > > > > >> err = -ENOMEM;
> > > > > >> - vcore = kvmppc_vcore_create(kvm, core);
> > > > > >> + vcore = kvmppc_vcore_create(kvm, id & ~(kvm->arch.smt_mode - 1));
> > > > > >> kvm->arch.vcores[core] = vcore;
> > > > > >> kvm->arch.online_vcores++;
> > > > > >> }
> > > > > >> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> > > > > >> index f9818d7d3381..681dfe12a5f3 100644
> > > > > >> --- a/arch/powerpc/kvm/book3s_xive.c
> > > > > >> +++ b/arch/powerpc/kvm/book3s_xive.c
> > > > > >> @@ -317,6 +317,11 @@ static int xive_select_target(struct kvm *kvm, u32 *server, u8 prio)
> > > > > >> return -EBUSY;
> > > > > >> }
> > > > > >>
> > > > > >> +static u32 xive_vp(struct kvmppc_xive *xive, u32 server)
> > > > > >> +{
> > > > > >> + return xive->vp_base + kvmppc_pack_vcpu_id(xive->kvm, server);
> > > > > >> +}
> > > > > >> +
> > > > > >
> > > > > > I'm finding the XIVE indexing really baffling. There are a bunch of
> > > > > > other places where the code uses (xive->vp_base + NUMBER) directly.
> > > >
> > > > Ugh, yes. It looks like I botched part of my final cleanup and all the
> > > > cases you saw in kvm/book3s_xive.c should have been replaced with a call to
> > > > xive_vp(). I'll fix it and sorry for the confusion.
> > >
> > > Ok.
> > >
> > > > > This links the QEMU vCPU server NUMBER to a XIVE virtual processor number
> > > > > in OPAL. So we need to check that all used NUMBERs are, first, consistent
> > > > > and then, in the correct range.
> > > >
> > > > Right. My approach was to allow XIVE to keep using server numbers that
> > > > are equal to VCPU IDs, and just pack down the ID before indexing into
> > > > the vp_base area.
> > > >
> > > > > > If those are host side references, I guess they don't need updates for
> > > > > > this.
> > > >
> > > > These are all guest side references.
> > > >
> > > > > > But if that's the case, then how does indexing into the same array
> > > > > > with both host and guest server numbers make sense?
> > > >
> > > > Right, it doesn't make sense to mix host and guest server numbers when
> > > > we're remapping only the guest ones, but in this case (without native
> > > > guest XIVE support) it's just guest ones.
> > >
> > > Right. Will this remapping be broken by guest-visible XIVE? That is
> > > for the guest visible XIVE are we going to need to expose un-remapped
> > > XIVE server IDs to the guest?
> >
> > I'm not sure, I'll start looking at that next.
> >
> > > > > yes. VPs are allocated with KVM_MAX_VCPUS :
> > > > >
> > > > > xive->vp_base = xive_native_alloc_vp_block(KVM_MAX_VCPUS);
> > > > >
> > > > > but
> > > > >
> > > > > #define KVM_MAX_VCPU_ID (threads_per_subcore * KVM_MAX_VCORES)
> > > > >
> > > > > WE would need to change the allocation of the VPs I guess.
> > > >
> > > > Yes, this is one of the structures that overflow if we don't pack the IDs.
> > > >
> > > > > >> static u8 xive_lock_and_mask(struct kvmppc_xive *xive,
> > > > > >> struct kvmppc_xive_src_block *sb,
> > > > > >> struct kvmppc_xive_irq_state *state)
> > > > > >> @@ -1084,7 +1089,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
> > > > > >> pr_devel("Duplicate !\n");
> > > > > >> return -EEXIST;
> > > > > >> }
> > > > > >> - if (cpu >= KVM_MAX_VCPUS) {
> > > > > >> + if (cpu >= KVM_MAX_VCPU_ID) {>>
> > > > > >> pr_devel("Out of bounds !\n");
> > > > > >> return -EINVAL;
> > > > > >> }
> > > > > >> @@ -1098,7 +1103,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
> > > > > >> xc->xive = xive;
> > > > > >> xc->vcpu = vcpu;
> > > > > >> xc->server_num = cpu;
> > > > > >> - xc->vp_id = xive->vp_base + cpu;
> > > > > >> + xc->vp_id = xive_vp(xive, cpu);
> > > > > >> xc->mfrr = 0xff;
> > > > > >> xc->valid = true;
> > > > > >>
> > > > > >
> > > > >
> > >
> > >
> > >
> >
> >
>
>
>
> --
> 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: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] powerpc/perf: Rearrange memory freeing in imc init
From: Madhavan Srinivasan @ 2018-05-03 3:33 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <1523264425-19544-2-git-send-email-anju@linux.vnet.ibm.com>
On Monday 09 April 2018 02:30 PM, Anju T Sudhakar wrote:
> When any of the IMC (In-Memory Collection counter) devices fail
> to initialize, imc_common_mem_free() frees set of memory. In doing so,
> pmu_ptr pointer is also freed. But pmu_ptr pointer is used in subsequent
> function (imc_common_cpuhp_mem_free()) which is wrong. Patch here reorders
> the code to avoid such access.
>
> Also free the memory which is dynamically allocated during imc initialization,
> wherever required.
Reviewed-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> Signed-off-by: Anju T Sudhakar <anju@linux.vnet.ibm.com>
> ---
> test matrix and static checker run details are updated in the cover letter
> patch is based on
> https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git (branch: merge)
>
> arch/powerpc/perf/imc-pmu.c | 32 ++++++++++++++++---------------
> arch/powerpc/platforms/powernv/opal-imc.c | 13 ++++++++++---
> 2 files changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
> index d7532e7..258b0f4 100644
> --- a/arch/powerpc/perf/imc-pmu.c
> +++ b/arch/powerpc/perf/imc-pmu.c
> @@ -1153,7 +1153,7 @@ static void cleanup_all_core_imc_memory(void)
> /* mem_info will never be NULL */
> for (i = 0; i < nr_cores; i++) {
> if (ptr[i].vbase)
> - free_pages((u64)ptr->vbase, get_order(size));
> + free_pages((u64)ptr[i].vbase, get_order(size));
> }
>
> kfree(ptr);
> @@ -1191,7 +1191,6 @@ static void imc_common_mem_free(struct imc_pmu *pmu_ptr)
> if (pmu_ptr->attr_groups[IMC_EVENT_ATTR])
> kfree(pmu_ptr->attr_groups[IMC_EVENT_ATTR]->attrs);
> kfree(pmu_ptr->attr_groups[IMC_EVENT_ATTR]);
> - kfree(pmu_ptr);
> }
>
> /*
> @@ -1208,6 +1207,7 @@ static void imc_common_cpuhp_mem_free(struct imc_pmu *pmu_ptr)
> cpuhp_remove_state(CPUHP_AP_PERF_POWERPC_NEST_IMC_ONLINE);
> kfree(nest_imc_refc);
> kfree(per_nest_pmu_arr);
> + per_nest_pmu_arr = NULL;
> }
>
> if (nest_pmus > 0)
> @@ -1319,10 +1319,8 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
> int ret;
>
> ret = imc_mem_init(pmu_ptr, parent, pmu_idx);
> - if (ret) {
> - imc_common_mem_free(pmu_ptr);
> - return ret;
> - }
> + if (ret)
> + goto err_free_mem;
>
> switch (pmu_ptr->domain) {
> case IMC_DOMAIN_NEST:
> @@ -1337,7 +1335,9 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
> ret = init_nest_pmu_ref();
> if (ret) {
> mutex_unlock(&nest_init_lock);
> - goto err_free;
> + kfree(per_nest_pmu_arr);
> + per_nest_pmu_arr = NULL;
> + goto err_free_mem;
> }
> /* Register for cpu hotplug notification. */
> ret = nest_pmu_cpumask_init();
> @@ -1345,7 +1345,8 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
> mutex_unlock(&nest_init_lock);
> kfree(nest_imc_refc);
> kfree(per_nest_pmu_arr);
> - goto err_free;
> + per_nest_pmu_arr = NULL;
> + goto err_free_mem;
> }
> }
> nest_pmus++;
> @@ -1355,7 +1356,7 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
> ret = core_imc_pmu_cpumask_init();
> if (ret) {
> cleanup_all_core_imc_memory();
> - return ret;
> + goto err_free_mem;
> }
>
> break;
> @@ -1363,7 +1364,7 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
> ret = thread_imc_cpu_init();
> if (ret) {
> cleanup_all_thread_imc_memory();
> - return ret;
> + goto err_free_mem;
> }
>
> break;
> @@ -1373,23 +1374,24 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
>
> ret = update_events_in_group(parent, pmu_ptr);
> if (ret)
> - goto err_free;
> + goto err_free_cpuhp_mem;
>
> ret = update_pmu_ops(pmu_ptr);
> if (ret)
> - goto err_free;
> + goto err_free_cpuhp_mem;
>
> ret = perf_pmu_register(&pmu_ptr->pmu, pmu_ptr->pmu.name, -1);
> if (ret)
> - goto err_free;
> + goto err_free_cpuhp_mem;
>
> pr_info("%s performance monitor hardware support registered\n",
> pmu_ptr->pmu.name);
>
> return 0;
>
> -err_free:
> - imc_common_mem_free(pmu_ptr);
> +err_free_cpuhp_mem:
> imc_common_cpuhp_mem_free(pmu_ptr);
> +err_free_mem:
> + imc_common_mem_free(pmu_ptr);
> return ret;
> }
> diff --git a/arch/powerpc/platforms/powernv/opal-imc.c b/arch/powerpc/platforms/powernv/opal-imc.c
> index 2a14fda..490bb72 100644
> --- a/arch/powerpc/platforms/powernv/opal-imc.c
> +++ b/arch/powerpc/platforms/powernv/opal-imc.c
> @@ -115,8 +115,10 @@ static int imc_get_mem_addr_nest(struct device_node *node,
> return -ENOMEM;
>
> chipid_arr = kcalloc(nr_chips, sizeof(*chipid_arr), GFP_KERNEL);
> - if (!chipid_arr)
> + if (!chipid_arr) {
> + kfree(base_addr_arr);
> return -ENOMEM;
> + }
>
> if (of_property_read_u32_array(node, "chip-id", chipid_arr, nr_chips))
> goto error;
> @@ -143,7 +145,6 @@ static int imc_get_mem_addr_nest(struct device_node *node,
> return 0;
>
> error:
> - kfree(pmu_ptr->mem_info);
> kfree(base_addr_arr);
> kfree(chipid_arr);
> return -1;
> @@ -183,8 +184,14 @@ static int imc_pmu_create(struct device_node *parent, int pmu_index, int domain)
>
> /* Function to register IMC pmu */
> ret = init_imc_pmu(parent, pmu_ptr, pmu_index);
> - if (ret)
> + if (ret) {
> pr_err("IMC PMU %s Register failed\n", pmu_ptr->pmu.name);
> + kfree(pmu_ptr->pmu.name);
> + if (pmu_ptr->domain == IMC_DOMAIN_NEST)
> + kfree(pmu_ptr->mem_info);
> + kfree(pmu_ptr);
> + return ret;
> + }
>
> return 0;
>
^ permalink raw reply
* Re: [PATCH 2/4] powerpc/perf: Replace the direct return with goto statement
From: Madhavan Srinivasan @ 2018-05-03 3:33 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <1523264425-19544-3-git-send-email-anju@linux.vnet.ibm.com>
On Monday 09 April 2018 02:30 PM, Anju T Sudhakar wrote:
> Replace the direct return statement in imc_mem_init() with goto,
> to adhere to the kernel coding style.
Reviewed-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> Signed-off-by: Anju T Sudhakar <anju@linux.vnet.ibm.com>
> ---
> arch/powerpc/perf/imc-pmu.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
> index 258b0f4..1b285cd 100644
> --- a/arch/powerpc/perf/imc-pmu.c
> +++ b/arch/powerpc/perf/imc-pmu.c
> @@ -1236,7 +1236,7 @@ static int imc_mem_init(struct imc_pmu *pmu_ptr, struct device_node *parent,
> int pmu_index)
> {
> const char *s;
> - int nr_cores, cpu, res;
> + int nr_cores, cpu, res = -ENOMEM;
>
> if (of_property_read_string(parent, "name", &s))
> return -ENODEV;
> @@ -1246,7 +1246,7 @@ static int imc_mem_init(struct imc_pmu *pmu_ptr, struct device_node *parent,
> /* Update the pmu name */
> pmu_ptr->pmu.name = kasprintf(GFP_KERNEL, "%s%s_imc", "nest_", s);
> if (!pmu_ptr->pmu.name)
> - return -ENOMEM;
> + goto err;
>
> /* Needed for hotplug/migration */
> if (!per_nest_pmu_arr) {
> @@ -1254,7 +1254,7 @@ static int imc_mem_init(struct imc_pmu *pmu_ptr, struct device_node *parent,
> sizeof(struct imc_pmu *),
> GFP_KERNEL);
> if (!per_nest_pmu_arr)
> - return -ENOMEM;
> + goto err;
> }
> per_nest_pmu_arr[pmu_index] = pmu_ptr;
> break;
> @@ -1262,21 +1262,21 @@ static int imc_mem_init(struct imc_pmu *pmu_ptr, struct device_node *parent,
> /* Update the pmu name */
> pmu_ptr->pmu.name = kasprintf(GFP_KERNEL, "%s%s", s, "_imc");
> if (!pmu_ptr->pmu.name)
> - return -ENOMEM;
> + goto err;
>
> nr_cores = DIV_ROUND_UP(num_present_cpus(), threads_per_core);
> pmu_ptr->mem_info = kcalloc(nr_cores, sizeof(struct imc_mem_info),
> GFP_KERNEL);
>
> if (!pmu_ptr->mem_info)
> - return -ENOMEM;
> + goto err;
>
> core_imc_refc = kcalloc(nr_cores, sizeof(struct imc_pmu_ref),
> GFP_KERNEL);
>
> if (!core_imc_refc) {
> kfree(pmu_ptr->mem_info);
> - return -ENOMEM;
> + goto err;
> }
>
> core_imc_pmu = pmu_ptr;
> @@ -1285,14 +1285,14 @@ static int imc_mem_init(struct imc_pmu *pmu_ptr, struct device_node *parent,
> /* Update the pmu name */
> pmu_ptr->pmu.name = kasprintf(GFP_KERNEL, "%s%s", s, "_imc");
> if (!pmu_ptr->pmu.name)
> - return -ENOMEM;
> + goto err;
>
> thread_imc_mem_size = pmu_ptr->counter_mem_size;
> for_each_online_cpu(cpu) {
> res = thread_imc_mem_alloc(cpu, pmu_ptr->counter_mem_size);
> if (res) {
> cleanup_all_thread_imc_memory();
> - return res;
> + goto err;
> }
> }
>
> @@ -1302,6 +1302,8 @@ static int imc_mem_init(struct imc_pmu *pmu_ptr, struct device_node *parent,
> }
>
> return 0;
> +err:
> + return res;
> }
>
> /*
^ permalink raw reply
* Re: [PATCH 3/4] powerpc/perf: Return appropriate value for unknown domain
From: Madhavan Srinivasan @ 2018-05-03 3:32 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <1523264425-19544-4-git-send-email-anju@linux.vnet.ibm.com>
On Monday 09 April 2018 02:30 PM, Anju T Sudhakar wrote:
> Return proper error code for unknown domain during IMC initialization.
Looks good to me.
Reviewed-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> Signed-off-by: Anju T Sudhakar <anju@linux.vnet.ibm.com>
> ---
> arch/powerpc/perf/imc-pmu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
> index 1b285cd..4b4ca83 100644
> --- a/arch/powerpc/perf/imc-pmu.c
> +++ b/arch/powerpc/perf/imc-pmu.c
> @@ -1371,7 +1371,7 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
>
> break;
> default:
> - return -1; /* Unknown domain */
> + return -EINVAL; /* Unknown domain */
> }
>
> ret = update_events_in_group(parent, pmu_ptr);
^ permalink raw reply
* Re: [PATCH 4/4] powerpc/perf: Unregister thread-imc if core-imc not supported
From: Madhavan Srinivasan @ 2018-05-03 3:31 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <1523264425-19544-5-git-send-email-anju@linux.vnet.ibm.com>
On Monday 09 April 2018 02:30 PM, Anju T Sudhakar wrote:
> Enable thread-imc in the kernel, only if core-imc is registered.
Can you add more info here? Why we need this and so on.
> Signed-off-by: Anju T Sudhakar <anju@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/imc-pmu.h | 1 +
> arch/powerpc/perf/imc-pmu.c | 12 ++++++++++++
> arch/powerpc/platforms/powernv/opal-imc.c | 9 +++++++++
> 3 files changed, 22 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/imc-pmu.h b/arch/powerpc/include/asm/imc-pmu.h
> index d76cb11..69f516e 100644
> --- a/arch/powerpc/include/asm/imc-pmu.h
> +++ b/arch/powerpc/include/asm/imc-pmu.h
> @@ -128,4 +128,5 @@ extern int init_imc_pmu(struct device_node *parent,
> struct imc_pmu *pmu_ptr, int pmu_id);
> extern void thread_imc_disable(void);
> extern int get_max_nest_dev(void);
> +extern void unregister_thread_imc(void);
> #endif /* __ASM_POWERPC_IMC_PMU_H */
> diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
> index 4b4ca83..fa88785 100644
> --- a/arch/powerpc/perf/imc-pmu.c
> +++ b/arch/powerpc/perf/imc-pmu.c
> @@ -40,6 +40,7 @@ static struct imc_pmu *core_imc_pmu;
> /* Thread IMC data structures and variables */
>
> static DEFINE_PER_CPU(u64 *, thread_imc_mem);
> +static struct imc_pmu *thread_imc_pmu;
> static int thread_imc_mem_size;
>
> struct imc_pmu *imc_event_to_pmu(struct perf_event *event)
> @@ -1228,6 +1229,16 @@ static void imc_common_cpuhp_mem_free(struct imc_pmu *pmu_ptr)
> }
> }
>
> +/*
> + * Function to unregister thread-imc if core-imc
> + * is not registered.
> + */
> +void unregister_thread_imc(void)
> +{
> + imc_common_cpuhp_mem_free(thread_imc_pmu);
> + imc_common_mem_free(thread_imc_pmu);
> + perf_pmu_unregister(&thread_imc_pmu->pmu);
> +}
>
> /*
> * imc_mem_init : Function to support memory allocation for core imc.
> @@ -1296,6 +1307,7 @@ static int imc_mem_init(struct imc_pmu *pmu_ptr, struct device_node *parent,
> }
> }
>
> + thread_imc_pmu = pmu_ptr;
> break;
> default:
> return -EINVAL;
> diff --git a/arch/powerpc/platforms/powernv/opal-imc.c b/arch/powerpc/platforms/powernv/opal-imc.c
> index 490bb72..58a0794 100644
> --- a/arch/powerpc/platforms/powernv/opal-imc.c
> +++ b/arch/powerpc/platforms/powernv/opal-imc.c
> @@ -255,6 +255,7 @@ static int opal_imc_counters_probe(struct platform_device *pdev)
> {
> struct device_node *imc_dev = pdev->dev.of_node;
> int pmu_count = 0, domain;
> + bool core_imc_reg = false, thread_imc_reg = false;
> u32 type;
>
> /*
> @@ -292,6 +293,10 @@ static int opal_imc_counters_probe(struct platform_device *pdev)
> if (!imc_pmu_create(imc_dev, pmu_count, domain)) {
> if (domain == IMC_DOMAIN_NEST)
> pmu_count++;
> + if (domain == IMC_DOMAIN_CORE)
> + core_imc_reg = true;
> + if (domain == IMC_DOMAIN_THREAD)
> + thread_imc_reg = true;
> }
> }
>
> @@ -299,6 +304,10 @@ static int opal_imc_counters_probe(struct platform_device *pdev)
> if (pmu_count == 0)
> debugfs_remove_recursive(imc_debugfs_parent);
>
> + /* If core imc is not registered, unregister thread-imc */
> + if (!core_imc_reg && thread_imc_reg)
> + unregister_thread_imc();
> +
> return 0;
> }
>
^ permalink raw reply
* Re: [PATCH v2 RFC 1/1] KVM: PPC: Book3S HV: pack VCORE IDs to access full VCPU ID space
From: David Gibson @ 2018-05-03 3:07 UTC (permalink / raw)
To: Sam Bobroff; +Cc: linuxppc-dev, kvm, kvm-ppc, paulus, clg
In-Reply-To: <aafa1e1ec2e331b0842855c2e5e46a0c2d011acc.1525150933.git.sbobroff@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 10696 bytes --]
On Tue, May 01, 2018 at 03:04:41PM +1000, Sam Bobroff wrote:
> From: Sam Bobroff <sam.bobroff@au1.ibm.com>
>
> It is not currently possible to create the full number of possible
> VCPUs (KVM_MAX_VCPUS) on Power9 with KVM-HV when the guest uses less
> threads per core than it's core stride (or "VSMT mode"). This is
> because the VCORE ID and XIVE offsets to grow beyond KVM_MAX_VCPUS
> even though the VCPU ID is less than KVM_MAX_VCPU_ID.
>
> To address this, "pack" the VCORE ID and XIVE offsets by using
> knowledge of the way the VCPU IDs will be used when there are less
> guest threads per core than the core stride. The primary thread of
> each core will always be used first. Then, if the guest uses more than
> one thread per core, these secondary threads will sequentially follow
> the primary in each core.
>
> So, the only way an ID above KVM_MAX_VCPUS can be seen, is if the
> VCPUs are being spaced apart, so at least half of each core is empty
> and IDs between KVM_MAX_VCPUS and (KVM_MAX_VCPUS * 2) can be mapped
> into the second half of each core (4..7, in an 8-thread core).
>
> Similarly, if IDs above KVM_MAX_VCPUS * 2 are seen, at least 3/4 of
> each core is being left empty, and we can map down into the second and
> third quarters of each core (2, 3 and 5, 6 in an 8-thread core).
>
> Lastly, if IDs above KVM_MAX_VCPUS * 4 are seen, only the primary
> threads are being used and 7/8 of the core is empty, allowing use of
> the 1, 3, 5 and 7 thread slots.
>
> (Strides less than 8 are handled similarly.)
>
> This allows the VCORE ID or offset to be calculated quickly from the
> VCPU ID or XIVE server numbers, without access to the VCPU structure.
>
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> ---
> Hello everyone,
>
> I've tested this on P8 and P9, in lots of combinations of host and guest
> threading modes and it has been fine but it does feel like a "tricky"
> approach, so I still feel somewhat wary about it.
That's understandable. My worry about it is substantially mitigated
by the fact that it's not visible to userspace or the guest, so we can
fix it without corresponding changes being needed there.
> I've posted it as an RFC because I have not tested it with guest native-XIVE,
> and I suspect that it will take some work to support it.
Yeah.. guest-native XIVE is still rather a ways off.
> ====== v1 -> v2: ======
>
> Patch 1/1: KVM: PPC: Book3S HV: pack VCORE IDs to access full VCPU ID space
> * Corrected places in kvm/book3s_xive.c where IDs weren't packed.
> * Because kvmppc_pack_vcpu_id() is only called on P9, there is no need to test "emul_smt_mode > 1", so remove it.
> * Re-ordered block_offsets[] to be more ascending.
> * Added more detailed description of the packing algorithm.
>
> ====== v1: ======
Excluding concerns about native XIVE,
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>
> Patch 1/1: KVM: PPC: Book3S HV: pack VCORE IDs to access full VCPU ID space
>
> arch/powerpc/include/asm/kvm_book3s.h | 44 +++++++++++++++++++++++++++++++++++
> arch/powerpc/kvm/book3s_hv.c | 14 +++++++----
> arch/powerpc/kvm/book3s_xive.c | 19 +++++++++------
> 3 files changed, 66 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
> index 376ae803b69c..a8d9d625e873 100644
> --- a/arch/powerpc/include/asm/kvm_book3s.h
> +++ b/arch/powerpc/include/asm/kvm_book3s.h
> @@ -368,4 +368,48 @@ extern int kvmppc_h_logical_ci_store(struct kvm_vcpu *vcpu);
> #define SPLIT_HACK_MASK 0xff000000
> #define SPLIT_HACK_OFFS 0xfb000000
>
> +/* Pack a VCPU ID from the [0..KVM_MAX_VCPU_ID) space down to the
> + * [0..KVM_MAX_VCPUS) space, while using knowledge of the guest's core stride
> + * (but not it's actual threading mode, which is not available) to avoid
> + * collisions.
> + *
> + * The implementation leaves VCPU IDs from the range [0..KVM_MAX_VCPUS) (block
> + * 0) unchanged: if the guest is filling each VCORE completely then it will be
> + * using consecutive IDs and it will fill the space without any packing.
> + *
> + * For higher VCPU IDs, the packed ID is based on the VCPU ID modulo
> + * KVM_MAX_VCPUS (effectively masking off the top bits) and then an offset is
> + * added to avoid collisions.
> + *
> + * VCPU IDs in the range [KVM_MAX_VCPUS..(KVM_MAX_VCPUS*2)) (block 1) are only
> + * possible if the guest is leaving at least 1/2 of each VCORE empty, so IDs
> + * can be safely packed into the second half of each VCORE by adding an offset
> + * of (stride / 2).
> + *
> + * Similarly, if VCPU IDs in the range [(KVM_MAX_VCPUS*2)..(KVM_MAX_VCPUS*4))
> + * (blocks 2 and 3) are seen, the guest must be leaving at least 3/4 of each
> + * VCORE empty so packed IDs can be offset by (stride / 4) and (stride * 3 / 4).
> + *
> + * Finally, VCPU IDs from blocks 5..7 will only be seen if the guest is using a
> + * stride of 8 and 1 thread per core so the remaining offsets of 1, 3, 5 and 7
> + * must be free to use.
> + *
> + * (The offsets for each block are stored in block_offsets[], indexed by the
> + * block number if the stride is 8. For cases where the guest's stride is less
> + * than 8, we can re-use the block_offsets array by multiplying the block
> + * number by (MAX_SMT_THREADS / stride) to reach the correct entry.)
> + */
> +static inline u32 kvmppc_pack_vcpu_id(struct kvm *kvm, u32 id)
> +{
> + const int block_offsets[MAX_SMT_THREADS] = {0, 4, 2, 6, 1, 3, 5, 7};
> + int stride = kvm->arch.emul_smt_mode;
> + int block = (id / KVM_MAX_VCPUS) * (MAX_SMT_THREADS / stride);
> + u32 packed_id;
> +
> + BUG_ON(block >= MAX_SMT_THREADS);
> + packed_id = (id % KVM_MAX_VCPUS) + block_offsets[block];
> + BUG_ON(packed_id >= KVM_MAX_VCPUS);
> + return packed_id;
> +}
> +
> #endif /* __ASM_KVM_BOOK3S_H__ */
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 9cb9448163c4..49165cc90051 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -1762,7 +1762,7 @@ static int threads_per_vcore(struct kvm *kvm)
> return threads_per_subcore;
> }
>
> -static struct kvmppc_vcore *kvmppc_vcore_create(struct kvm *kvm, int core)
> +static struct kvmppc_vcore *kvmppc_vcore_create(struct kvm *kvm, int id)
> {
> struct kvmppc_vcore *vcore;
>
> @@ -1776,7 +1776,7 @@ static struct kvmppc_vcore *kvmppc_vcore_create(struct kvm *kvm, int core)
> init_swait_queue_head(&vcore->wq);
> vcore->preempt_tb = TB_NIL;
> vcore->lpcr = kvm->arch.lpcr;
> - vcore->first_vcpuid = core * kvm->arch.smt_mode;
> + vcore->first_vcpuid = id;
> vcore->kvm = kvm;
> INIT_LIST_HEAD(&vcore->preempt_list);
>
> @@ -1992,12 +1992,18 @@ static struct kvm_vcpu *kvmppc_core_vcpu_create_hv(struct kvm *kvm,
> mutex_lock(&kvm->lock);
> vcore = NULL;
> err = -EINVAL;
> - core = id / kvm->arch.smt_mode;
> + if (cpu_has_feature(CPU_FTR_ARCH_300)) {
> + BUG_ON(kvm->arch.smt_mode != 1);
> + core = kvmppc_pack_vcpu_id(kvm, id);
> + } else {
> + core = id / kvm->arch.smt_mode;
> + }
> if (core < KVM_MAX_VCORES) {
> vcore = kvm->arch.vcores[core];
> + BUG_ON(cpu_has_feature(CPU_FTR_ARCH_300) && vcore);
> if (!vcore) {
> err = -ENOMEM;
> - vcore = kvmppc_vcore_create(kvm, core);
> + vcore = kvmppc_vcore_create(kvm, id & ~(kvm->arch.smt_mode - 1));
> kvm->arch.vcores[core] = vcore;
> kvm->arch.online_vcores++;
> }
> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> index f9818d7d3381..dbd5887daf4a 100644
> --- a/arch/powerpc/kvm/book3s_xive.c
> +++ b/arch/powerpc/kvm/book3s_xive.c
> @@ -317,6 +317,11 @@ static int xive_select_target(struct kvm *kvm, u32 *server, u8 prio)
> return -EBUSY;
> }
>
> +static u32 xive_vp(struct kvmppc_xive *xive, u32 server)
> +{
> + return xive->vp_base + kvmppc_pack_vcpu_id(xive->kvm, server);
> +}
> +
> static u8 xive_lock_and_mask(struct kvmppc_xive *xive,
> struct kvmppc_xive_src_block *sb,
> struct kvmppc_xive_irq_state *state)
> @@ -362,7 +367,7 @@ static u8 xive_lock_and_mask(struct kvmppc_xive *xive,
> */
> if (xd->flags & OPAL_XIVE_IRQ_MASK_VIA_FW) {
> xive_native_configure_irq(hw_num,
> - xive->vp_base + state->act_server,
> + xive_vp(xive, state->act_server),
> MASKED, state->number);
> /* set old_p so we can track if an H_EOI was done */
> state->old_p = true;
> @@ -418,7 +423,7 @@ static void xive_finish_unmask(struct kvmppc_xive *xive,
> */
> if (xd->flags & OPAL_XIVE_IRQ_MASK_VIA_FW) {
> xive_native_configure_irq(hw_num,
> - xive->vp_base + state->act_server,
> + xive_vp(xive, state->act_server),
> state->act_priority, state->number);
> /* If an EOI is needed, do it here */
> if (!state->old_p)
> @@ -495,7 +500,7 @@ static int xive_target_interrupt(struct kvm *kvm,
> kvmppc_xive_select_irq(state, &hw_num, NULL);
>
> return xive_native_configure_irq(hw_num,
> - xive->vp_base + server,
> + xive_vp(xive, server),
> prio, state->number);
> }
>
> @@ -883,7 +888,7 @@ int kvmppc_xive_set_mapped(struct kvm *kvm, unsigned long guest_irq,
> * which is fine for a never started interrupt.
> */
> xive_native_configure_irq(hw_irq,
> - xive->vp_base + state->act_server,
> + xive_vp(xive, state->act_server),
> state->act_priority, state->number);
>
> /*
> @@ -959,7 +964,7 @@ int kvmppc_xive_clr_mapped(struct kvm *kvm, unsigned long guest_irq,
>
> /* Reconfigure the IPI */
> xive_native_configure_irq(state->ipi_number,
> - xive->vp_base + state->act_server,
> + xive_vp(xive, state->act_server),
> state->act_priority, state->number);
>
> /*
> @@ -1084,7 +1089,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
> pr_devel("Duplicate !\n");
> return -EEXIST;
> }
> - if (cpu >= KVM_MAX_VCPUS) {
> + if (cpu >= KVM_MAX_VCPU_ID) {
> pr_devel("Out of bounds !\n");
> return -EINVAL;
> }
> @@ -1098,7 +1103,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
> xc->xive = xive;
> xc->vcpu = vcpu;
> xc->server_num = cpu;
> - xc->vp_id = xive->vp_base + cpu;
> + xc->vp_id = xive_vp(xive, cpu);
> xc->mfrr = 0xff;
> xc->valid = true;
>
--
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 RFC 1/1] KVM: PPC: Book3S HV: pack VCORE IDs to access full VCPU ID space
From: David Gibson @ 2018-05-03 3:11 UTC (permalink / raw)
To: Sam Bobroff; +Cc: kvm-ppc, paulus, linuxppc-dev, Cédric Le Goater, kvm
In-Reply-To: <20180501044206.GA8330@tungsten.ozlabs.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 7948 bytes --]
On Tue, May 01, 2018 at 02:52:21PM +1000, Sam Bobroff wrote:
> On Tue, Apr 24, 2018 at 01:48:25PM +1000, David Gibson wrote:
> > On Tue, Apr 24, 2018 at 01:19:15PM +1000, Sam Bobroff wrote:
> > > On Mon, Apr 23, 2018 at 11:06:35AM +0200, Cédric Le Goater wrote:
> > > > On 04/16/2018 06:09 AM, David Gibson wrote:
[snip]
> > > At the moment, kvm->vcores[] and xive->vp_base are both sized by NR_CPUS
> > > (via KVM_MAX_VCPUS and KVM_MAX_VCORES which are both NR_CPUS). This is
> > > enough space for the maximum number of VCPUs, and some space is wasted
> > > when the guest uses less than this (but KVM doesn't know how many will
> > > be created, so we can't do better easily). The problem is that the
> > > indicies overflow before all of those VCPUs can be created, not that
> > > more space is needed.
> > >
> > > We could fix the overflow by expanding these areas to KVM_MAX_VCPU_ID
> > > but that will use 8x the space we use now, and we know that no more than
> > > KVM_MAX_VCPUS will be used so all this new space is basically wasted.
> > >
> > > So remapping seems better if it will work. (Ben H. was strongly against
> > > wasting more XIVE space if possible.)
> >
> > Hm, ok. Are the relevant arrays here per-VM, or global? Or some of both?
>
> Per-VM. They are the kvm->vcores[] array and the blocks of memory
> pointed to by xive->vp_base.
Hm. If it were global (where you can't know the size of a specific
VM) I'd certainly see the concern about not expanding the size of the
array.
As it is, I'm a little perplexed that we care so much about the
difference between KVM_MAX_VCPUS and KVM_MAX_VCPU_ID, a factor of 8,
when we apparently don't care about the difference between the vm's
actual number of cpus and KVM_MAX_VCPUS, a factor of maybe 2048 (for a
1vcpu guest and powernv_defconfig).
>
> > > In short, remapping provides a way to allow the guest to create it's full set
> > > of VCPUs without wasting any more space than we do currently, without
> > > having to do something more complicated like tracking used IDs or adding
> > > additional KVM CAPs.
> > >
> > > > >> +
> > > > >> #endif /* __ASM_KVM_BOOK3S_H__ */
> > > > >> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> > > > >> index 9cb9448163c4..49165cc90051 100644
> > > > >> --- a/arch/powerpc/kvm/book3s_hv.c
> > > > >> +++ b/arch/powerpc/kvm/book3s_hv.c
> > > > >> @@ -1762,7 +1762,7 @@ static int threads_per_vcore(struct kvm *kvm)
> > > > >> return threads_per_subcore;
> > > > >> }
> > > > >>
> > > > >> -static struct kvmppc_vcore *kvmppc_vcore_create(struct kvm *kvm, int core)
> > > > >> +static struct kvmppc_vcore *kvmppc_vcore_create(struct kvm *kvm, int id)
> > > > >> {
> > > > >> struct kvmppc_vcore *vcore;
> > > > >>
> > > > >> @@ -1776,7 +1776,7 @@ static struct kvmppc_vcore *kvmppc_vcore_create(struct kvm *kvm, int core)
> > > > >> init_swait_queue_head(&vcore->wq);
> > > > >> vcore->preempt_tb = TB_NIL;
> > > > >> vcore->lpcr = kvm->arch.lpcr;
> > > > >> - vcore->first_vcpuid = core * kvm->arch.smt_mode;
> > > > >> + vcore->first_vcpuid = id;
> > > > >> vcore->kvm = kvm;
> > > > >> INIT_LIST_HEAD(&vcore->preempt_list);
> > > > >>
> > > > >> @@ -1992,12 +1992,18 @@ static struct kvm_vcpu *kvmppc_core_vcpu_create_hv(struct kvm *kvm,
> > > > >> mutex_lock(&kvm->lock);
> > > > >> vcore = NULL;
> > > > >> err = -EINVAL;
> > > > >> - core = id / kvm->arch.smt_mode;
> > > > >> + if (cpu_has_feature(CPU_FTR_ARCH_300)) {
> > > > >> + BUG_ON(kvm->arch.smt_mode != 1);
> > > > >> + core = kvmppc_pack_vcpu_id(kvm, id);
> > > > >> + } else {
> > > > >> + core = id / kvm->arch.smt_mode;
> > > > >> + }
> > > > >> if (core < KVM_MAX_VCORES) {
> > > > >> vcore = kvm->arch.vcores[core];
> > > > >> + BUG_ON(cpu_has_feature(CPU_FTR_ARCH_300) && vcore);
> > > > >> if (!vcore) {
> > > > >> err = -ENOMEM;
> > > > >> - vcore = kvmppc_vcore_create(kvm, core);
> > > > >> + vcore = kvmppc_vcore_create(kvm, id & ~(kvm->arch.smt_mode - 1));
> > > > >> kvm->arch.vcores[core] = vcore;
> > > > >> kvm->arch.online_vcores++;
> > > > >> }
> > > > >> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> > > > >> index f9818d7d3381..681dfe12a5f3 100644
> > > > >> --- a/arch/powerpc/kvm/book3s_xive.c
> > > > >> +++ b/arch/powerpc/kvm/book3s_xive.c
> > > > >> @@ -317,6 +317,11 @@ static int xive_select_target(struct kvm *kvm, u32 *server, u8 prio)
> > > > >> return -EBUSY;
> > > > >> }
> > > > >>
> > > > >> +static u32 xive_vp(struct kvmppc_xive *xive, u32 server)
> > > > >> +{
> > > > >> + return xive->vp_base + kvmppc_pack_vcpu_id(xive->kvm, server);
> > > > >> +}
> > > > >> +
> > > > >
> > > > > I'm finding the XIVE indexing really baffling. There are a bunch of
> > > > > other places where the code uses (xive->vp_base + NUMBER) directly.
> > >
> > > Ugh, yes. It looks like I botched part of my final cleanup and all the
> > > cases you saw in kvm/book3s_xive.c should have been replaced with a call to
> > > xive_vp(). I'll fix it and sorry for the confusion.
> >
> > Ok.
> >
> > > > This links the QEMU vCPU server NUMBER to a XIVE virtual processor number
> > > > in OPAL. So we need to check that all used NUMBERs are, first, consistent
> > > > and then, in the correct range.
> > >
> > > Right. My approach was to allow XIVE to keep using server numbers that
> > > are equal to VCPU IDs, and just pack down the ID before indexing into
> > > the vp_base area.
> > >
> > > > > If those are host side references, I guess they don't need updates for
> > > > > this.
> > >
> > > These are all guest side references.
> > >
> > > > > But if that's the case, then how does indexing into the same array
> > > > > with both host and guest server numbers make sense?
> > >
> > > Right, it doesn't make sense to mix host and guest server numbers when
> > > we're remapping only the guest ones, but in this case (without native
> > > guest XIVE support) it's just guest ones.
> >
> > Right. Will this remapping be broken by guest-visible XIVE? That is
> > for the guest visible XIVE are we going to need to expose un-remapped
> > XIVE server IDs to the guest?
>
> I'm not sure, I'll start looking at that next.
>
> > > > yes. VPs are allocated with KVM_MAX_VCPUS :
> > > >
> > > > xive->vp_base = xive_native_alloc_vp_block(KVM_MAX_VCPUS);
> > > >
> > > > but
> > > >
> > > > #define KVM_MAX_VCPU_ID (threads_per_subcore * KVM_MAX_VCORES)
> > > >
> > > > WE would need to change the allocation of the VPs I guess.
> > >
> > > Yes, this is one of the structures that overflow if we don't pack the IDs.
> > >
> > > > >> static u8 xive_lock_and_mask(struct kvmppc_xive *xive,
> > > > >> struct kvmppc_xive_src_block *sb,
> > > > >> struct kvmppc_xive_irq_state *state)
> > > > >> @@ -1084,7 +1089,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
> > > > >> pr_devel("Duplicate !\n");
> > > > >> return -EEXIST;
> > > > >> }
> > > > >> - if (cpu >= KVM_MAX_VCPUS) {
> > > > >> + if (cpu >= KVM_MAX_VCPU_ID) {>>
> > > > >> pr_devel("Out of bounds !\n");
> > > > >> return -EINVAL;
> > > > >> }
> > > > >> @@ -1098,7 +1103,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
> > > > >> xc->xive = xive;
> > > > >> xc->vcpu = vcpu;
> > > > >> xc->server_num = cpu;
> > > > >> - xc->vp_id = xive->vp_base + cpu;
> > > > >> + xc->vp_id = xive_vp(xive, cpu);
> > > > >> xc->mfrr = 0xff;
> > > > >> xc->valid = true;
> > > > >>
> > > > >
> > > >
> >
> >
> >
>
>
--
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 kernel] powerpc/ioda: Use ibm,supported-tce-sizes for IOMMU page size mask
From: Russell Currey @ 2018-05-03 0:53 UTC (permalink / raw)
To: Alexey Kardashevskiy, linuxppc-dev; +Cc: David Gibson
In-Reply-To: <20180502061239.36398-1-aik@ozlabs.ru>
On Wed, 2018-05-02 at 16:12 +1000, Alexey Kardashevskiy wrote:
> At the moment we assume that IODA2 and newer PHBs can always do
> 4K/64K/16M
> IOMMU pages, however this is not the case for POWER9 and now skiboot
> advertises the supported sizes via the device so we use that instead
> of hard coding the mask.
>
> This falls back to the default mask if no "ibm,supported-tce-sizes"
> is provided. This removes 16MB from the defaults as it is not
> supported
> everywhere; the downside of this is that hugepages backed POWER8
> guests
> will fall back to 64K IOMMU pages until skiboot is updated.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: Russell Currey <ruscur@russell.cc>
^ permalink raw reply
* Re: [PATCH] pkeys: Introduce PKEY_ALLOC_SIGNALINHERIT and change signal semantics
From: Ram Pai @ 2018-05-02 23:38 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Florian Weimer, Dave Hansen, Linux-MM, Linux API, linux-x86_64,
linux-arch, X86 ML, linuxppc-dev
In-Reply-To: <CALCETrUfO=vXg5rT-n=y8pLktcq5+ORvgpsOXCHG4GaugB3k2A@mail.gmail.com>
On Wed, May 02, 2018 at 09:18:11PM +0000, Andy Lutomirski wrote:
> On Wed, May 2, 2018 at 2:13 PM Ram Pai <linuxram@us.ibm.com> wrote:
>
>
> > > Ram, would you please comment?
>
> > on POWER the pkey behavior will remain the same at entry or at exit from
> > the signal handler. For eg: if a key is read-disabled on entry into
> > the signal handler, and gets read-enabled in the signal handler, than it
> > will continue to be read-enabled on return from the signal handler.
>
> > In other words, changes to key permissions persist across signal
> > boundaries.
>
> I don't know about POWER's ISA, but this is crappy behavior. If a thread
> temporarily grants itself access to a restrictive memory key and then gets
> a signal, the signal handler should *not* have access to that key.
This is a new requirement that I was not aware off. Its not documented
anywhere AFAICT. Regardless of how the ISA behaves, its still a kernel
behavior that needs to be clearly defined.
--
Ram Pai
^ permalink raw reply
* Re: [PATCH v10 00/25] Speculative page faults
From: Punit Agrawal @ 2018-05-02 15:50 UTC (permalink / raw)
To: Laurent Dufour
Cc: akpm, mhocko, peterz, kirill, ak, dave, jack, Matthew Wilcox,
benh, mpe, paulus, Thomas Gleixner, Ingo Molnar, hpa, Will Deacon,
Sergey Senozhatsky, Andrea Arcangeli, Alexei Starovoitov,
kemi.wang, sergey.senozhatsky.work, Daniel Jordan, David Rientjes,
Jerome Glisse, Ganesh Mahendran, linux-kernel, linux-mm, haren,
khandual, npiggin, bsingharora, paulmck, Tim Chen <tim.c.che
In-Reply-To: <eef94f4f-800e-9994-d926-a71b80552ebc@linux.vnet.ibm.com>
Hi Laurent,
Thanks for your reply.
Laurent Dufour <ldufour@linux.vnet.ibm.com> writes:
> On 02/05/2018 16:17, Punit Agrawal wrote:
>> Hi Laurent,
>>
>> One query below -
>>
>> Laurent Dufour <ldufour@linux.vnet.ibm.com> writes:
>>
>> [...]
>>
>>>
>>> Ebizzy:
>>> -------
>>> The test is counting the number of records per second it can manage, the
>>> higher is the best. I run it like this 'ebizzy -mTRp'. To get consistent
>>> result I repeated the test 100 times and measure the average result. The
>>> number is the record processes per second, the higher is the best.
>>>
>>> BASE SPF delta
>>> 16 CPUs x86 VM 12405.52 91104.52 634.39%
>>> 80 CPUs P8 node 37880.01 76201.05 101.16%
>>
>> How do you measure the number of records processed? Is there a specific
>> version of ebizzy that reports this? I couldn't find a way to get this
>> information with the ebizzy that's included in ltp.
>
> I'm using the original one : http://ebizzy.sourceforge.net/
Turns out I missed the records processed in the verbose output enabled
by "-vvv". Sorry for the noise.
[...]
>>
>> A trial run showed increased fault handling when SPF is enabled on an
>> 8-core ARM64 system running 4.17-rc3. I am using a port of your x86
>> patch to enable spf on arm64.
>>
>> SPF
>> ---
>>
>> Performance counter stats for './ebizzy -vvvmTRp':
>>
>> 1,322,736 faults
>> 1,299,241 software/config=11/
>>
>> 10.005348034 seconds time elapsed
>>
>> No SPF
>> -----
>>
>> Performance counter stats for './ebizzy -vvvmTRp':
>>
>> 708,916 faults
>> 0 software/config=11/
>>
>> 10.005807432 seconds time elapsed
>
> Thanks for sharing these good numbers !
A quick run showed 71041 (no-spf) vs 122306 (spf) records/s (~72%
improvement).
I'd like to do some runs on a slightly larger system (if I can get my
hands on one) to see how the patches behave. I'll also have a closer
look at your series - the previous comments were just somethings I
observed as part of trying the functionality on arm64.
Thanks,
Punit
^ permalink raw reply
* Re: [PATCH v10 00/25] Speculative page faults
From: Laurent Dufour @ 2018-05-02 14:45 UTC (permalink / raw)
To: Punit Agrawal
Cc: akpm, mhocko, peterz, kirill, ak, dave, jack, Matthew Wilcox,
benh, mpe, paulus, Thomas Gleixner, Ingo Molnar, hpa, Will Deacon,
Sergey Senozhatsky, Andrea Arcangeli, Alexei Starovoitov,
kemi.wang, sergey.senozhatsky.work, Daniel Jordan, David Rientjes,
Jerome Glisse, Ganesh Mahendran, linux-kernel, linux-mm, haren,
khandual, npiggin, bsingharora, paulmck, Tim Chen, linuxppc-dev,
x86
In-Reply-To: <87bmdynnv4.fsf@e105922-lin.cambridge.arm.com>
On 02/05/2018 16:17, Punit Agrawal wrote:
> Hi Laurent,
>
> One query below -
>
> Laurent Dufour <ldufour@linux.vnet.ibm.com> writes:
>
> [...]
>
>>
>> Ebizzy:
>> -------
>> The test is counting the number of records per second it can manage, the
>> higher is the best. I run it like this 'ebizzy -mTRp'. To get consistent
>> result I repeated the test 100 times and measure the average result. The
>> number is the record processes per second, the higher is the best.
>>
>> BASE SPF delta
>> 16 CPUs x86 VM 12405.52 91104.52 634.39%
>> 80 CPUs P8 node 37880.01 76201.05 101.16%
>
> How do you measure the number of records processed? Is there a specific
> version of ebizzy that reports this? I couldn't find a way to get this
> information with the ebizzy that's included in ltp.
I'm using the original one : http://ebizzy.sourceforge.net/
>
>>
>> Here are the performance counter read during a run on a 16 CPUs x86 VM:
>> Performance counter stats for './ebizzy -mRTp':
>> 860074 faults
>> 856866 spf
>> 285 pagefault:spf_pte_lock
>> 1506 pagefault:spf_vma_changed
>> 0 pagefault:spf_vma_noanon
>> 73 pagefault:spf_vma_notsup
>> 0 pagefault:spf_vma_access
>> 0 pagefault:spf_pmd_changed
>>
>> And the ones captured during a run on a 80 CPUs Power node:
>> Performance counter stats for './ebizzy -mRTp':
>> 722695 faults
>> 699402 spf
>> 16048 pagefault:spf_pte_lock
>> 6838 pagefault:spf_vma_changed
>> 0 pagefault:spf_vma_noanon
>> 277 pagefault:spf_vma_notsup
>> 0 pagefault:spf_vma_access
>> 0 pagefault:spf_pmd_changed
>>
>> In ebizzy's case most of the page fault were handled in a speculative way,
>> leading the ebizzy performance boost.
>
> A trial run showed increased fault handling when SPF is enabled on an
> 8-core ARM64 system running 4.17-rc3. I am using a port of your x86
> patch to enable spf on arm64.
>
> SPF
> ---
>
> Performance counter stats for './ebizzy -vvvmTRp':
>
> 1,322,736 faults
> 1,299,241 software/config=11/
>
> 10.005348034 seconds time elapsed
>
> No SPF
> -----
>
> Performance counter stats for './ebizzy -vvvmTRp':
>
> 708,916 faults
> 0 software/config=11/
>
> 10.005807432 seconds time elapsed
Thanks for sharing these good numbers !
> Thanks,
> Punit
>
> [...]
>
^ permalink raw reply
* Re: [PATCH v10 00/25] Speculative page faults
From: Punit Agrawal @ 2018-05-02 14:17 UTC (permalink / raw)
To: Laurent Dufour
Cc: akpm, mhocko, peterz, kirill, ak, dave, jack, Matthew Wilcox,
benh, mpe, paulus, Thomas Gleixner, Ingo Molnar, hpa, Will Deacon,
Sergey Senozhatsky, Andrea Arcangeli, Alexei Starovoitov,
kemi.wang, sergey.senozhatsky.work, Daniel Jordan, David Rientjes,
Jerome Glisse, Ganesh Mahendran, linux-kernel, linux-mm, haren,
khandual, npiggin, bsingharora, paulmck, Tim Chen, linuxppc-dev,
x86
In-Reply-To: <1523975611-15978-1-git-send-email-ldufour@linux.vnet.ibm.com>
Hi Laurent,
One query below -
Laurent Dufour <ldufour@linux.vnet.ibm.com> writes:
[...]
>
> Ebizzy:
> -------
> The test is counting the number of records per second it can manage, the
> higher is the best. I run it like this 'ebizzy -mTRp'. To get consistent
> result I repeated the test 100 times and measure the average result. The
> number is the record processes per second, the higher is the best.
>
> BASE SPF delta
> 16 CPUs x86 VM 12405.52 91104.52 634.39%
> 80 CPUs P8 node 37880.01 76201.05 101.16%
How do you measure the number of records processed? Is there a specific
version of ebizzy that reports this? I couldn't find a way to get this
information with the ebizzy that's included in ltp.
>
> Here are the performance counter read during a run on a 16 CPUs x86 VM:
> Performance counter stats for './ebizzy -mRTp':
> 860074 faults
> 856866 spf
> 285 pagefault:spf_pte_lock
> 1506 pagefault:spf_vma_changed
> 0 pagefault:spf_vma_noanon
> 73 pagefault:spf_vma_notsup
> 0 pagefault:spf_vma_access
> 0 pagefault:spf_pmd_changed
>
> And the ones captured during a run on a 80 CPUs Power node:
> Performance counter stats for './ebizzy -mRTp':
> 722695 faults
> 699402 spf
> 16048 pagefault:spf_pte_lock
> 6838 pagefault:spf_vma_changed
> 0 pagefault:spf_vma_noanon
> 277 pagefault:spf_vma_notsup
> 0 pagefault:spf_vma_access
> 0 pagefault:spf_pmd_changed
>
> In ebizzy's case most of the page fault were handled in a speculative way,
> leading the ebizzy performance boost.
A trial run showed increased fault handling when SPF is enabled on an
8-core ARM64 system running 4.17-rc3. I am using a port of your x86
patch to enable spf on arm64.
SPF
---
Performance counter stats for './ebizzy -vvvmTRp':
1,322,736 faults
1,299,241 software/config=11/
10.005348034 seconds time elapsed
No SPF
-----
Performance counter stats for './ebizzy -vvvmTRp':
708,916 faults
0 software/config=11/
10.005807432 seconds time elapsed
Thanks,
Punit
[...]
^ permalink raw reply
* [PATCH 6/6] powerpc/syscalls: timer_create can be handle by perfectly normal COMPAT_SYS_SPU
From: Michael Ellerman @ 2018-05-02 13:20 UTC (permalink / raw)
To: linuxppc-dev; +Cc: viro, linux-kernel
In-Reply-To: <20180502132051.28861-1-mpe@ellerman.id.au>
From: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/powerpc/include/asm/systbl.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/systbl.h b/arch/powerpc/include/asm/systbl.h
index cdf528089a63..79a3b47e4839 100644
--- a/arch/powerpc/include/asm/systbl.h
+++ b/arch/powerpc/include/asm/systbl.h
@@ -245,7 +245,7 @@ SYSCALL_SPU(epoll_create)
SYSCALL_SPU(epoll_ctl)
SYSCALL_SPU(epoll_wait)
SYSCALL_SPU(remap_file_pages)
-SYSX_SPU(sys_timer_create,compat_sys_timer_create,sys_timer_create)
+COMPAT_SYS_SPU(timer_create)
COMPAT_SYS_SPU(timer_settime)
COMPAT_SYS_SPU(timer_gettime)
SYSCALL_SPU(timer_getoverrun)
--
2.14.1
^ permalink raw reply related
* [PATCH 5/6] powerpc/syscalls: kill ppc32_select()
From: Michael Ellerman @ 2018-05-02 13:20 UTC (permalink / raw)
To: linuxppc-dev; +Cc: viro, linux-kernel
In-Reply-To: <20180502132051.28861-1-mpe@ellerman.id.au>
From: Al Viro <viro@zeniv.linux.org.uk>
it had always been pointless - compat_sys_select() sign-extends
the first argument just fine on its own.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
[mpe: Use COMPAT_SPU_NEW() to keep systbl_chk.sh happy]
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/include/asm/systbl.h | 2 +-
arch/powerpc/kernel/sys_ppc32.c | 9 ---------
2 files changed, 1 insertion(+), 10 deletions(-)
diff --git a/arch/powerpc/include/asm/systbl.h b/arch/powerpc/include/asm/systbl.h
index b91701c0711a..cdf528089a63 100644
--- a/arch/powerpc/include/asm/systbl.h
+++ b/arch/powerpc/include/asm/systbl.h
@@ -147,7 +147,7 @@ SYSCALL_SPU(setfsuid)
SYSCALL_SPU(setfsgid)
SYSCALL_SPU(llseek)
COMPAT_SYS_SPU(getdents)
-SYSX_SPU(sys_select,ppc32_select,sys_select)
+COMPAT_SPU_NEW(select)
SYSCALL_SPU(flock)
SYSCALL_SPU(msync)
COMPAT_SYS_SPU(readv)
diff --git a/arch/powerpc/kernel/sys_ppc32.c b/arch/powerpc/kernel/sys_ppc32.c
index c11c73373691..bdf58ba1a94b 100644
--- a/arch/powerpc/kernel/sys_ppc32.c
+++ b/arch/powerpc/kernel/sys_ppc32.c
@@ -52,15 +52,6 @@
#include <asm/syscalls.h>
#include <asm/switch_to.h>
-
-asmlinkage long ppc32_select(u32 n, compat_ulong_t __user *inp,
- compat_ulong_t __user *outp, compat_ulong_t __user *exp,
- compat_uptr_t tvp_x)
-{
- /* sign extend n */
- return compat_sys_select((int)n, inp, outp, exp, compat_ptr(tvp_x));
-}
-
unsigned long compat_sys_mmap2(unsigned long addr, size_t len,
unsigned long prot, unsigned long flags,
unsigned long fd, unsigned long pgoff)
--
2.14.1
^ permalink raw reply related
* [PATCH 4/6] powerpc/syscalls: Add COMPAT_SPU_NEW() macro
From: Michael Ellerman @ 2018-05-02 13:20 UTC (permalink / raw)
To: linuxppc-dev; +Cc: viro, linux-kernel
In-Reply-To: <20180502132051.28861-1-mpe@ellerman.id.au>
Currently the select system call is wired up with the SYSX_SPU()
macro. The SYSX_SPU() is not handled by systbl_chk.c, which means the
syscall number for select is not checked.
That hides the fact that the syscall number for select is actually
__NR__newselect not __NR_select.
In a following patch we'd like to drop ppc32_select() which means
select will become a regular COMPAT_SYS_SPU() syscall. But
COMPAT_SYS_SPU() can't deal with the fact that the syscall number is
actually __NR__newselect. We also can't just redefine __NR_select
because that's still used for the old select call.
So add a new COMPAT_NEW_SPU() that does the same thing as
COMPAT_SYS_SPU() except it encodes that we're using the new number.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/kernel/systbl.S | 1 +
arch/powerpc/kernel/systbl_chk.c | 1 +
arch/powerpc/platforms/cell/spu_callbacks.c | 1 +
3 files changed, 3 insertions(+)
diff --git a/arch/powerpc/kernel/systbl.S b/arch/powerpc/kernel/systbl.S
index c7d5216d91d7..919a32746ede 100644
--- a/arch/powerpc/kernel/systbl.S
+++ b/arch/powerpc/kernel/systbl.S
@@ -35,6 +35,7 @@
#endif
#define SYSCALL_SPU(func) SYSCALL(func)
#define COMPAT_SYS_SPU(func) COMPAT_SYS(func)
+#define COMPAT_SPU_NEW(func) COMPAT_SYS(func)
#define SYSX_SPU(f, f3264, f32) SYSX(f, f3264, f32)
.section .rodata,"a"
diff --git a/arch/powerpc/kernel/systbl_chk.c b/arch/powerpc/kernel/systbl_chk.c
index 28476e811644..4653258722ac 100644
--- a/arch/powerpc/kernel/systbl_chk.c
+++ b/arch/powerpc/kernel/systbl_chk.c
@@ -31,6 +31,7 @@
#define SYSCALL_SPU(func) SYSCALL(func)
#define COMPAT_SYS_SPU(func) COMPAT_SYS(func)
+#define COMPAT_SPU_NEW(func) COMPAT_SYS(_new##func)
#define SYSX_SPU(f, f3264, f32) SYSX(f, f3264, f32)
/* Just insert a marker for ni_syscalls */
diff --git a/arch/powerpc/platforms/cell/spu_callbacks.c b/arch/powerpc/platforms/cell/spu_callbacks.c
index d5bb8c8d769a..8ae86200ef6c 100644
--- a/arch/powerpc/platforms/cell/spu_callbacks.c
+++ b/arch/powerpc/platforms/cell/spu_callbacks.c
@@ -44,6 +44,7 @@ static void *spu_syscall_table[] = {
#define SYSCALL_SPU(func) sys_##func,
#define COMPAT_SYS_SPU(func) sys_##func,
+#define COMPAT_SPU_NEW(func) sys_##func,
#define SYSX_SPU(f, f3264, f32) f,
#include <asm/systbl.h>
--
2.14.1
^ permalink raw reply related
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