* 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
* Re: BUG: p8_aes_ctr randomly returns wrong results
From: Ondrej Mosnáček @ 2019-03-13 12:48 UTC (permalink / raw)
To: linux-crypto, Herbert Xu
Cc: leo.barbosa, Stephan Mueller, nayna, marcelo.cerri, pfsmorigo,
leitao, linuxppc-dev
In-Reply-To: <CAAUqJDtsx6Sdv=6xAb1VWWjDF08y8wh2nsAypR8_x_e99a1Uwg@mail.gmail.com>
st 13. 3. 2019 o 13:37 Ondrej Mosnáček <omosnacek@gmail.com> napísal(a):
> 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
(Ah, forgot to compare email addresses with MAINTAINERS... let me try these)
^ permalink raw reply
* [PATCH] powerpc/vdso64: Fix CLOCK_MONOTONIC inconsistencies across Y2038
From: Michael Ellerman @ 2019-03-13 13:14 UTC (permalink / raw)
To: linuxppc-dev; +Cc: arnd, sboyd, linux-kernel, john.stultz, tglx, jaydee
Jakub Drnec reported:
Setting the realtime clock can sometimes make the monotonic clock go
back by over a hundred years. Decreasing the realtime clock across
the y2k38 threshold is one reliable way to reproduce. Allegedly this
can also happen just by running ntpd, I have not managed to
reproduce that other than booting with rtc at >2038 and then running
ntp. When this happens, anything with timers (e.g. openjdk) breaks
rather badly.
And included a test case (slightly edited for brevity):
#define _POSIX_C_SOURCE 199309L
#include <stdio.h>
#include <time.h>
#include <stdlib.h>
#include <unistd.h>
long get_time(void) {
struct timespec tp;
clock_gettime(CLOCK_MONOTONIC, &tp);
return tp.tv_sec + tp.tv_nsec / 1000000000;
}
int main(void) {
long last = get_time();
while(1) {
long now = get_time();
if (now < last) {
printf("clock went backwards by %ld seconds!\n", last - now);
}
last = now;
sleep(1);
}
return 0;
}
Which when run concurrently with:
# date -s 2040-1-1
# date -s 2037-1-1
Will detect the clock going backward.
The root cause is that wtom_clock_sec in struct vdso_data is only a
32-bit signed value, even though we set its value to be equal to
tk->wall_to_monotonic.tv_sec which is 64-bits.
Because the monotonic clock starts at zero when the system boots the
wall_to_montonic.tv_sec offset is negative for current and future
dates. Currently on a freshly booted system the offset will be in the
vicinity of negative 1.5 billion seconds.
However if the wall clock is set past the Y2038 boundary, the offset
from wall to monotonic becomes less than negative 2^31, and no longer
fits in 32-bits. When that value is assigned to wtom_clock_sec it is
truncated and becomes positive, causing the VDSO assembly code to
calculate CLOCK_MONOTONIC incorrectly.
That causes CLOCK_MONOTONIC to jump ahead by ~4 billion seconds which
it is not meant to do. Worse, if the time is then set back before the
Y2038 boundary CLOCK_MONOTONIC will jump backward.
We can fix it simply by storing the full 64-bit offset in the
vdso_data, and using that in the VDSO assembly code. We also shuffle
some of the fields in vdso_data to avoid creating a hole.
The original commit that added the CLOCK_MONOTONIC support to the VDSO
did actually use a 64-bit value for wtom_clock_sec, see commit
a7f290dad32e ("[PATCH] powerpc: Merge vdso's and add vdso support to
32 bits kernel") (Nov 2005). However just 3 days later it was
converted to 32-bits in commit 0c37ec2aa88b ("[PATCH] powerpc: vdso
fixes (take #2)"), and the bug has existed since then AFAICS.
Fixes: 0c37ec2aa88b ("[PATCH] powerpc: vdso fixes (take #2)")
Cc: stable@vger.kernel.org # v2.6.15+
Link: http://lkml.kernel.org/r/HaC.ZfES.62bwlnvAvMP.1STMMj@seznam.cz
Reported-by: Jakub Drnec <jaydee@email.cz>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/include/asm/vdso_datapage.h | 8 ++++----
arch/powerpc/kernel/vdso64/gettimeofday.S | 4 ++--
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/include/asm/vdso_datapage.h b/arch/powerpc/include/asm/vdso_datapage.h
index 1afe90ade595..bbc06bd72b1f 100644
--- a/arch/powerpc/include/asm/vdso_datapage.h
+++ b/arch/powerpc/include/asm/vdso_datapage.h
@@ -82,10 +82,10 @@ struct vdso_data {
__u32 icache_block_size; /* L1 i-cache block size */
__u32 dcache_log_block_size; /* L1 d-cache log block size */
__u32 icache_log_block_size; /* L1 i-cache log block size */
- __s32 wtom_clock_sec; /* Wall to monotonic clock */
- __s32 wtom_clock_nsec;
- struct timespec stamp_xtime; /* xtime as at tb_orig_stamp */
- __u32 stamp_sec_fraction; /* fractional seconds of stamp_xtime */
+ __u32 stamp_sec_fraction; /* fractional seconds of stamp_xtime */
+ __s32 wtom_clock_nsec; /* Wall to monotonic clock nsec */
+ __s64 wtom_clock_sec; /* Wall to monotonic clock sec */
+ struct timespec stamp_xtime; /* xtime as at tb_orig_stamp */
__u32 syscall_map_64[SYSCALL_MAP_SIZE]; /* map of syscalls */
__u32 syscall_map_32[SYSCALL_MAP_SIZE]; /* map of syscalls */
};
diff --git a/arch/powerpc/kernel/vdso64/gettimeofday.S b/arch/powerpc/kernel/vdso64/gettimeofday.S
index a4ed9edfd5f0..1f324c28705b 100644
--- a/arch/powerpc/kernel/vdso64/gettimeofday.S
+++ b/arch/powerpc/kernel/vdso64/gettimeofday.S
@@ -92,7 +92,7 @@ V_FUNCTION_BEGIN(__kernel_clock_gettime)
* At this point, r4,r5 contain our sec/nsec values.
*/
- lwa r6,WTOM_CLOCK_SEC(r3)
+ ld r6,WTOM_CLOCK_SEC(r3)
lwa r9,WTOM_CLOCK_NSEC(r3)
/* We now have our result in r6,r9. We create a fake dependency
@@ -125,7 +125,7 @@ V_FUNCTION_BEGIN(__kernel_clock_gettime)
bne cr6,75f
/* CLOCK_MONOTONIC_COARSE */
- lwa r6,WTOM_CLOCK_SEC(r3)
+ ld r6,WTOM_CLOCK_SEC(r3)
lwa r9,WTOM_CLOCK_NSEC(r3)
/* check if counter has updated */
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v2 10/16] KVM: PPC: Book3S HV: XIVE: add get/set accessors for the VP XIVE state
From: Cédric Le Goater @ 2019-03-13 13:19 UTC (permalink / raw)
To: David Gibson; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <20190225033144.GN7668@umbus.fritz.box>
On 2/25/19 4:31 AM, David Gibson wrote:
> On Fri, Feb 22, 2019 at 12:28:34PM +0100, Cédric Le Goater wrote:
>> At a VCPU level, the state of the thread interrupt management
>> registers needs to be collected. These registers are cached under the
>> 'xive_saved_state.w01' field of the VCPU when the VPCU context is
>> pulled from the HW thread. An OPAL call retrieves the backup of the
>> IPB register in the underlying XIVE NVT structure and merges it in the
>> KVM state.
>>
>> The structures of the interface between QEMU and KVM provisions some
>> extra room (two u64) for further extensions if more state needs to be
>> transferred back to QEMU.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> arch/powerpc/include/asm/kvm_ppc.h | 11 +++
>> arch/powerpc/include/uapi/asm/kvm.h | 2 +
>> arch/powerpc/kvm/book3s.c | 24 +++++++
>> arch/powerpc/kvm/book3s_xive_native.c | 82 ++++++++++++++++++++++
>> Documentation/virtual/kvm/devices/xive.txt | 19 +++++
>> 5 files changed, 138 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
>> index 1e61877fe147..664c65051612 100644
>> --- a/arch/powerpc/include/asm/kvm_ppc.h
>> +++ b/arch/powerpc/include/asm/kvm_ppc.h
>> @@ -272,6 +272,7 @@ union kvmppc_one_reg {
>> u64 addr;
>> u64 length;
>> } vpaval;
>> + u64 xive_timaval[4];
>
> This is doubling the size of the userspace visible one_reg union. Is
> that safe?
'safe' as in compatibility on an older KVM which would still use the old
kvmppc_one_reg definition ?
It should be fine as KVM_REG_PPC_VP_STATE would not be handled. Am I wrong ?
>> };
>>
>> struct kvmppc_ops {
>> @@ -604,6 +605,10 @@ extern int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,
>> extern void kvmppc_xive_native_cleanup_vcpu(struct kvm_vcpu *vcpu);
>> extern void kvmppc_xive_native_init_module(void);
>> extern void kvmppc_xive_native_exit_module(void);
>> +extern int kvmppc_xive_native_get_vp(struct kvm_vcpu *vcpu,
>> + union kvmppc_one_reg *val);
>> +extern int kvmppc_xive_native_set_vp(struct kvm_vcpu *vcpu,
>> + union kvmppc_one_reg *val);
>>
>> #else
>> static inline int kvmppc_xive_set_xive(struct kvm *kvm, u32 irq, u32 server,
>> @@ -636,6 +641,12 @@ static inline int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,
>> static inline void kvmppc_xive_native_cleanup_vcpu(struct kvm_vcpu *vcpu) { }
>> static inline void kvmppc_xive_native_init_module(void) { }
>> static inline void kvmppc_xive_native_exit_module(void) { }
>> +static inline int kvmppc_xive_native_get_vp(struct kvm_vcpu *vcpu,
>> + union kvmppc_one_reg *val)
>> +{ return 0; }
>> +static inline int kvmppc_xive_native_set_vp(struct kvm_vcpu *vcpu,
>> + union kvmppc_one_reg *val)
>> +{ return -ENOENT; }
>>
>> #endif /* CONFIG_KVM_XIVE */
>>
>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
>> index cd78ad1020fe..42d4ef93ec2d 100644
>> --- a/arch/powerpc/include/uapi/asm/kvm.h
>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
>> @@ -480,6 +480,8 @@ struct kvm_ppc_cpu_char {
>> #define KVM_REG_PPC_ICP_PPRI_SHIFT 16 /* pending irq priority */
>> #define KVM_REG_PPC_ICP_PPRI_MASK 0xff
>>
>> +#define KVM_REG_PPC_VP_STATE (KVM_REG_PPC | KVM_REG_SIZE_U256 | 0x8d)
>> +
>> /* Device control API: PPC-specific devices */
>> #define KVM_DEV_MPIC_GRP_MISC 1
>> #define KVM_DEV_MPIC_BASE_ADDR 0 /* 64-bit */
>> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
>> index 96d43f091255..f85a9211f30c 100644
>> --- a/arch/powerpc/kvm/book3s.c
>> +++ b/arch/powerpc/kvm/book3s.c
>> @@ -641,6 +641,18 @@ int kvmppc_get_one_reg(struct kvm_vcpu *vcpu, u64 id,
>> *val = get_reg_val(id, kvmppc_xics_get_icp(vcpu));
>> break;
>> #endif /* CONFIG_KVM_XICS */
>> +#ifdef CONFIG_KVM_XIVE
>> + case KVM_REG_PPC_VP_STATE:
>> + if (!vcpu->arch.xive_vcpu) {
>> + r = -ENXIO;
>> + break;
>> + }
>> + if (xive_enabled())
>> + r = kvmppc_xive_native_get_vp(vcpu, val);
>> + else
>> + r = -ENXIO;
>> + break;
>> +#endif /* CONFIG_KVM_XIVE */
>> case KVM_REG_PPC_FSCR:
>> *val = get_reg_val(id, vcpu->arch.fscr);
>> break;
>> @@ -714,6 +726,18 @@ int kvmppc_set_one_reg(struct kvm_vcpu *vcpu, u64 id,
>> r = kvmppc_xics_set_icp(vcpu, set_reg_val(id, *val));
>> break;
>> #endif /* CONFIG_KVM_XICS */
>> +#ifdef CONFIG_KVM_XIVE
>> + case KVM_REG_PPC_VP_STATE:
>> + if (!vcpu->arch.xive_vcpu) {
>> + r = -ENXIO;
>> + break;
>> + }
>> + if (xive_enabled())
>> + r = kvmppc_xive_native_set_vp(vcpu, val);
>> + else
>> + r = -ENXIO;
>> + break;
>> +#endif /* CONFIG_KVM_XIVE */
>> case KVM_REG_PPC_FSCR:
>> vcpu->arch.fscr = set_reg_val(id, *val);
>> break;
>> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
>> index 3debc876d5a0..132bff52d70a 100644
>> --- a/arch/powerpc/kvm/book3s_xive_native.c
>> +++ b/arch/powerpc/kvm/book3s_xive_native.c
>> @@ -845,6 +845,88 @@ static int kvmppc_xive_native_create(struct kvm_device *dev, u32 type)
>> return ret;
>> }
>>
>> +/*
>> + * Interrupt Pending Buffer (IPB) offset
>> + */
>> +#define TM_IPB_SHIFT 40
>> +#define TM_IPB_MASK (((u64) 0xFF) << TM_IPB_SHIFT)
>> +
>> +int kvmppc_xive_native_get_vp(struct kvm_vcpu *vcpu, union kvmppc_one_reg *val)
>> +{
>> + struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
>> + u64 opal_state;
>> + int rc;
>> +
>> + if (!kvmppc_xive_enabled(vcpu))
>> + return -EPERM;
>> +
>> + if (!xc)
>> + return -ENOENT;
>> +
>> + /* Thread context registers. We only care about IPB and CPPR */
>> + val->xive_timaval[0] = vcpu->arch.xive_saved_state.w01;
>> +
>> + /*
>> + * Return the OS CAM line to print out the VP identifier in
>> + * the QEMU monitor. This is not restored.
>> + */
>> + val->xive_timaval[1] = vcpu->arch.xive_cam_word;
>
> I'm pretty dubious about this mixing of vital state information with
> what's basically debug information.
I think QEMU deserves to know about the OS CAM line value. I was even
thinking about adding the POOL CAM line value for future use (nested)
> Doubly so since it requires changing the ABI to increase
> the one_reg union's size.
OK. That's one argument.
> Might be better to have this control only return the 0th and 2nd u64s
> from the TIMA, with the CAM debug information returned via some other
> mechanism.
Like an extra reg : KVM_REG_PPC_VP_CAM ?
>> +
>> + /* Get the VP state from OPAL */
>> + rc = xive_native_get_vp_state(xc->vp_id, &opal_state);
>> + if (rc)
>> + return rc;
>> +
>> + /*
>> + * Capture the backup of IPB register in the NVT structure and
>> + * merge it in our KVM VP state.
>> + */
>> + val->xive_timaval[0] |= cpu_to_be64(opal_state & TM_IPB_MASK);
>> +
>> + pr_devel("%s NSR=%02x CPPR=%02x IBP=%02x PIPR=%02x w01=%016llx w2=%08x opal=%016llx\n",
>> + __func__,
>> + vcpu->arch.xive_saved_state.nsr,
>> + vcpu->arch.xive_saved_state.cppr,
>> + vcpu->arch.xive_saved_state.ipb,
>> + vcpu->arch.xive_saved_state.pipr,
>> + vcpu->arch.xive_saved_state.w01,
>> + (u32) vcpu->arch.xive_cam_word, opal_state);
>
> Hrm.. except you don't seem to be using the last half of the timaval
> field anyway.
Yes. The two u64 are extras. We can do without.
Would that be ok if I stored the w01 regs in the first u64, the CAM line(s)
in the second and remove the extra two u64 ?
>> +
>> + return 0;
>> +}
>> +
>> +int kvmppc_xive_native_set_vp(struct kvm_vcpu *vcpu, union kvmppc_one_reg *val)
>> +{
>> + struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
>> + struct kvmppc_xive *xive = vcpu->kvm->arch.xive;
>> +
>> + pr_devel("%s w01=%016llx vp=%016llx\n", __func__,
>> + val->xive_timaval[0], val->xive_timaval[1]);
>> +
>> + if (!kvmppc_xive_enabled(vcpu))
>> + return -EPERM;
>> +
>> + if (!xc || !xive)
>> + return -ENOENT;
>> +
>> + /* We can't update the state of a "pushed" VCPU */
>> + if (WARN_ON(vcpu->arch.xive_pushed))
>
> What prevents userspace from tripping this WARN_ON()?
if the vCPU is executing a vCPU ioctl, it means that it exited the guest
and that its interrupt context has been pulled out of XIVE.
>> + return -EIO;
>
> EBUSY might be more appropriate here.
OK.
Thanks,
C.
>
>> +
>> + /*
>> + * Restore the thread context registers. IPB and CPPR should
>> + * be the only ones that matter.
>> + */
>> + vcpu->arch.xive_saved_state.w01 = val->xive_timaval[0];
>> +
>> + /*
>> + * There is no need to restore the XIVE internal state (IPB
>> + * stored in the NVT) as the IPB register was merged in KVM VP
>> + * state when captured.
>> + */
>> + return 0;
>> +}
>> +
>> static int xive_native_debug_show(struct seq_file *m, void *private)
>> {
>> struct kvmppc_xive *xive = m->private;
>> diff --git a/Documentation/virtual/kvm/devices/xive.txt b/Documentation/virtual/kvm/devices/xive.txt
>> index a26be635cff9..1b8957c50c53 100644
>> --- a/Documentation/virtual/kvm/devices/xive.txt
>> +++ b/Documentation/virtual/kvm/devices/xive.txt
>> @@ -102,6 +102,25 @@ the legacy interrupt mode, referred as XICS (POWER7/8).
>> -EINVAL: Not initialized source number, invalid priority or
>> invalid CPU number.
>>
>> +* VCPU state
>> +
>> + The XIVE IC maintains VP interrupt state in an internal structure
>> + called the NVT. When a VP is not dispatched on a HW processor
>> + thread, this structure can be updated by HW if the VP is the target
>> + of an event notification.
>> +
>> + It is important for migration to capture the cached IPB from the NVT
>> + as it synthesizes the priorities of the pending interrupts. We
>> + capture a bit more to report debug information.
>> +
>> + KVM_REG_PPC_VP_STATE (4 * 64bits)
>> + bits: | 63 .... 32 | 31 .... 0 |
>> + values: | TIMA word0 | TIMA word1 |
>> + bits: | 127 .......... 64 |
>> + values: | VP CAM Line |
>> + bits: | 255 .......... 128 |
>> + values: | unused |
>> +
>> * Migration:
>>
>> Saving the state of a VM using the XIVE native exploitation mode
>
^ permalink raw reply
* Re: [PATCH] powerpc/vdso64: Fix CLOCK_MONOTONIC inconsistencies across Y2038
From: Arnd Bergmann @ 2019-03-13 13:24 UTC (permalink / raw)
To: Michael Ellerman
Cc: Stephen Boyd, Linux Kernel Mailing List, linuxppc-dev,
John Stultz, Thomas Gleixner, Vincenzo Frascino, jaydee
In-Reply-To: <20190313131438.8212-1-mpe@ellerman.id.au>
On Wed, Mar 13, 2019 at 2:14 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
> That causes CLOCK_MONOTONIC to jump ahead by ~4 billion seconds which
> it is not meant to do. Worse, if the time is then set back before the
> Y2038 boundary CLOCK_MONOTONIC will jump backward.
>
> We can fix it simply by storing the full 64-bit offset in the
> vdso_data, and using that in the VDSO assembly code. We also shuffle
> some of the fields in vdso_data to avoid creating a hole.
I see nothing wrong with your patch, but I would point out that there is
a patch series [1] from Vincenzo Frascino to unify the vdso implementation
across architectures that I hope can make it into linux-5.2, and that will
resolve this issue, as well as allow 32-bit architectures to provide
a working interface with 64-bit time_t.
Arnd
[1] https://lore.kernel.org/linux-arm-kernel/20190222122430.21180-1-vincenzo.frascino@arm.com/
^ permalink raw reply
* Re: bpf jit PPC64 (BE) test_verifier PTR_TO_STACK store/load failure
From: Naveen N. Rao @ 2019-03-13 13:51 UTC (permalink / raw)
To: Daniel Borkmann, Michael Ellerman, Sandipan Das, Yauheni Kaliuta
Cc: linuxppc-dev, bpf, Jiri Olsa, netdev
In-Reply-To: <xuny4l87qc2v.fsf@redhat.com>
Hi,
Yauheni Kaliuta wrote:
> Hi!
>
> I found a failure:
>
> ```
> # ./test_verifier 722
> #722/u PTR_TO_STACK store/load FAIL retval -1 != -87117812
> 0: (bf) r1 = r10
> 1: (07) r1 += -10
> 2: (7a) *(u64 *)(r1 +2) = -87117812
> 3: (79) r0 = *(u64 *)(r1 +2)
> 4: (95) exit
> processed 5 insns (limit 131072), stack depth 8
> #722/p PTR_TO_STACK store/load FAIL retval -1 != -87117812
> 0: (bf) r1 = r10
> 1: (07) r1 += -10
> 2: (7a) *(u64 *)(r1 +2) = -87117812
> 3: (79) r0 = *(u64 *)(r1 +2)
> 4: (95) exit
> processed 5 insns (limit 131072), stack depth 8
> Summary: 0 PASSED, 0 SKIPPED, 2 FAILED
> ```
>
> The reason is in the JIT. The code is jitted into:
>
> [...]
> d00000000580e7f8: f9 23 00 00 std r9,0(r3)
> d00000000580e7fc: e9 03 00 02 lwa r8,0(r3)
> [...]
>
> so, it stores DW to the location r3, but loads W, i.e. in BE it is:
>
> saves
> r3: FF FF FF FF FA CE B0 0C
> loads
> r3: FF FF FF FF
>
> (in LE it works semicorretly, saves 0C B0 CE FA FF FF FF FF, loads 0C B0 CE FA)
>
> This is because of the handling of the +2 offset. For stores it is:
>
>
> #define PPC_STD(r, base, i) EMIT(PPC_INST_STD | ___PPC_RS(r) |
> \
> ___PPC_RA(base) | ((i) & 0xfffc))
>
> and for loads
> #define PPC_LD(r, base, i) EMIT(PPC_INST_LD | ___PPC_RT(r) | \
> ___PPC_RA(base) | IMM_L(i))
> #define IMM_L(i) ((uintptr_t)(i) & 0xffff)
>
> So, in the load case the offset +2 (immediate value) is not
> masked and turns the instruction to lwa instead of ld.
Indeed -- good catch and analysis!
>
>
> Would it be correct to & 0xfffc the immediate value as well?
Yes, I think that would be the right fix.
>
> BTW, the full run on big endian:
>
> Summary: 1190 PASSED, 125 SKIPPED, 4 FAILED
Thanks for pointing that out, I'll look into these failures.
- Naveen
^ permalink raw reply
* [PATCH v2] kmemleak: skip scanning holes in the .bss section
From: Qian Cai @ 2019-03-13 14:57 UTC (permalink / raw)
To: akpm
Cc: catalin.marinas, linux-kernel, kvm-ppc, linux-mm, Qian Cai,
linuxppc-dev
The commit 2d4f567103ff ("KVM: PPC: Introduce kvm_tmp framework") adds
kvm_tmp[] into the .bss section and then free the rest of unused spaces
back to the page allocator.
kernel_init
kvm_guest_init
kvm_free_tmp
free_reserved_area
free_unref_page
free_unref_page_prepare
With DEBUG_PAGEALLOC=y, it will unmap those pages from kernel. As the
result, kmemleak scan will trigger a panic below when it scans the .bss
section with unmapped pages.
Since this is done way before the first kmemleak_scan(), just go
lockless to make the implementation simple and skip those pages when
scanning the .bss section. Later, those pages could be tracked by
kmemleak again once allocated by the page allocator. Overall, this is
such a special case, so no need to make it a generic to let kmemleak
gain an ability to skip blocks in scan_large_block() for now.
BUG: Unable to handle kernel data access at 0xc000000001610000
Faulting instruction address: 0xc0000000003cc178
Oops: Kernel access of bad area, sig: 11 [#1]
LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=256 DEBUG_PAGEALLOC NUMA pSeries
CPU: 3 PID: 130 Comm: kmemleak Kdump: loaded Not tainted 5.0.0+ #9
REGS: c0000004b05bf940 TRAP: 0300 Not tainted (5.0.0+)
NIP [c0000000003cc178] scan_block+0xa8/0x190
LR [c0000000003cc170] scan_block+0xa0/0x190
Call Trace:
[c0000004b05bfbd0] [c0000000003cc170] scan_block+0xa0/0x190 (unreliable)
[c0000004b05bfc30] [c0000000003cc2c0] scan_large_block+0x60/0xa0
[c0000004b05bfc70] [c0000000003ccc64] kmemleak_scan+0x254/0x960
[c0000004b05bfd40] [c0000000003cdd50] kmemleak_scan_thread+0xec/0x12c
[c0000004b05bfdb0] [c000000000104388] kthread+0x1b8/0x1c0
[c0000004b05bfe20] [c00000000000b364] ret_from_kernel_thread+0x5c/0x78
Instruction dump:
7fa3eb78 4844667d 60000000 60000000 60000000 60000000 3bff0008 7fbcf840
409d00b8 4bfffeed 2fa30000 409e00ac <e87f0000> e93e0128 7fa91840
419dffdc
Signed-off-by: Qian Cai <cai@lca.pw>
---
v2: make the function __init per Andrew.
arch/powerpc/kernel/kvm.c | 3 +++
include/linux/kmemleak.h | 4 ++++
mm/kmemleak.c | 25 ++++++++++++++++++++++++-
3 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/kvm.c b/arch/powerpc/kernel/kvm.c
index 683b5b3805bd..5cddc8fc56bb 100644
--- a/arch/powerpc/kernel/kvm.c
+++ b/arch/powerpc/kernel/kvm.c
@@ -26,6 +26,7 @@
#include <linux/slab.h>
#include <linux/of.h>
#include <linux/pagemap.h>
+#include <linux/kmemleak.h>
#include <asm/reg.h>
#include <asm/sections.h>
@@ -712,6 +713,8 @@ static void kvm_use_magic_page(void)
static __init void kvm_free_tmp(void)
{
+ kmemleak_bss_hole(&kvm_tmp[kvm_tmp_index],
+ &kvm_tmp[ARRAY_SIZE(kvm_tmp)]);
free_reserved_area(&kvm_tmp[kvm_tmp_index],
&kvm_tmp[ARRAY_SIZE(kvm_tmp)], -1, NULL);
}
diff --git a/include/linux/kmemleak.h b/include/linux/kmemleak.h
index 5ac416e2d339..17d3684e81ab 100644
--- a/include/linux/kmemleak.h
+++ b/include/linux/kmemleak.h
@@ -46,6 +46,7 @@ extern void kmemleak_alloc_phys(phys_addr_t phys, size_t size, int min_count,
extern void kmemleak_free_part_phys(phys_addr_t phys, size_t size) __ref;
extern void kmemleak_not_leak_phys(phys_addr_t phys) __ref;
extern void kmemleak_ignore_phys(phys_addr_t phys) __ref;
+extern void kmemleak_bss_hole(void *start, void *stop) __init;
static inline void kmemleak_alloc_recursive(const void *ptr, size_t size,
int min_count, slab_flags_t flags,
@@ -131,6 +132,9 @@ static inline void kmemleak_not_leak_phys(phys_addr_t phys)
static inline void kmemleak_ignore_phys(phys_addr_t phys)
{
}
+static inline void kmemleak_bss_hole(void *start, void *stop)
+{
+}
#endif /* CONFIG_DEBUG_KMEMLEAK */
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 707fa5579f66..a2d894d3de07 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -237,6 +237,10 @@ static int kmemleak_skip_disable;
/* If there are leaks that can be reported */
static bool kmemleak_found_leaks;
+/* Skip scanning of a range in the .bss section. */
+static void *bss_hole_start;
+static void *bss_hole_stop;
+
static bool kmemleak_verbose;
module_param_named(verbose, kmemleak_verbose, bool, 0600);
@@ -1265,6 +1269,18 @@ void __ref kmemleak_ignore_phys(phys_addr_t phys)
}
EXPORT_SYMBOL(kmemleak_ignore_phys);
+/**
+ * kmemleak_bss_hole - skip scanning a range in the .bss section
+ *
+ * @start: start of the range
+ * @stop: end of the range
+ */
+void __init kmemleak_bss_hole(void *start, void *stop)
+{
+ bss_hole_start = start;
+ bss_hole_stop = stop;
+}
+
/*
* Update an object's checksum and return true if it was modified.
*/
@@ -1531,7 +1547,14 @@ static void kmemleak_scan(void)
/* data/bss scanning */
scan_large_block(_sdata, _edata);
- scan_large_block(__bss_start, __bss_stop);
+
+ if (bss_hole_start) {
+ scan_large_block(__bss_start, bss_hole_start);
+ scan_large_block(bss_hole_stop, __bss_stop);
+ } else {
+ scan_large_block(__bss_start, __bss_stop);
+ }
+
scan_large_block(__start_ro_after_init, __end_ro_after_init);
#ifdef CONFIG_SMP
--
2.17.2 (Apple Git-113)
^ permalink raw reply related
* Re: [PATCH v2] fs/dax: deposit pagetable even when installing zero page
From: Dan Williams @ 2019-03-13 15:46 UTC (permalink / raw)
To: Jan Kara
Cc: linux-nvdimm, Aneesh Kumar K.V, Linux MM, Alexander Viro,
Ross Zwisler, linux-fsdevel, Andrew Morton, linuxppc-dev
In-Reply-To: <20190313095834.GF32521@quack2.suse.cz>
On Wed, Mar 13, 2019 at 2:58 AM Jan Kara <jack@suse.cz> wrote:
>
> 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...
I'll merge this and let it soak in -next for a week and then submit for 5.1-rc2.
^ permalink raw reply
* Re: [PATCH 2/2] mm/dax: Don't enable huge dax mapping by default
From: Dan Williams @ 2019-03-13 16:02 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: Jan Kara, linux-nvdimm, Linux Kernel Mailing List, Linux MM,
Oliver, Ross Zwisler, Andrew Morton, linuxppc-dev,
Kirill A . Shutemov
In-Reply-To: <87k1hc8iqa.fsf@linux.ibm.com>
On Wed, Mar 6, 2019 at 1:18 AM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> Dan Williams <dan.j.williams@intel.com> writes:
>
> > On Thu, Feb 28, 2019 at 1:40 AM Oliver <oohall@gmail.com> wrote:
> >>
> >> On Thu, Feb 28, 2019 at 7:35 PM Aneesh Kumar K.V
> >> <aneesh.kumar@linux.ibm.com> wrote:
> >> >
> >> > Add a flag to indicate the ability to do huge page dax mapping. On architecture
> >> > like ppc64, the hypervisor can disable huge page support in the guest. In
> >> > such a case, we should not enable huge page dax mapping. This patch adds
> >> > a flag which the architecture code will update to indicate huge page
> >> > dax mapping support.
> >>
> >> *groan*
> >>
> >> > Architectures mostly do transparent_hugepage_flag = 0; if they can't
> >> > do hugepages. That also takes care of disabling dax hugepage mapping
> >> > with this change.
> >> >
> >> > Without this patch we get the below error with kvm on ppc64.
> >> >
> >> > [ 118.849975] lpar: Failed hash pte insert with error -4
> >> >
> >> > NOTE: The patch also use
> >> >
> >> > echo never > /sys/kernel/mm/transparent_hugepage/enabled
> >> > to disable dax huge page mapping.
> >> >
> >> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> >> > ---
> >> > TODO:
> >> > * Add Fixes: tag
> >> >
> >> > include/linux/huge_mm.h | 4 +++-
> >> > mm/huge_memory.c | 4 ++++
> >> > 2 files changed, 7 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> >> > index 381e872bfde0..01ad5258545e 100644
> >> > --- a/include/linux/huge_mm.h
> >> > +++ b/include/linux/huge_mm.h
> >> > @@ -53,6 +53,7 @@ vm_fault_t vmf_insert_pfn_pud(struct vm_area_struct *vma, unsigned long addr,
> >> > pud_t *pud, pfn_t pfn, bool write);
> >> > enum transparent_hugepage_flag {
> >> > TRANSPARENT_HUGEPAGE_FLAG,
> >> > + TRANSPARENT_HUGEPAGE_DAX_FLAG,
> >> > TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG,
> >> > TRANSPARENT_HUGEPAGE_DEFRAG_DIRECT_FLAG,
> >> > TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_FLAG,
> >> > @@ -111,7 +112,8 @@ static inline bool __transparent_hugepage_enabled(struct vm_area_struct *vma)
> >> > if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_FLAG))
> >> > return true;
> >> >
> >> > - if (vma_is_dax(vma))
> >> > + if (vma_is_dax(vma) &&
> >> > + (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_DAX_FLAG)))
> >> > return true;
> >>
> >> Forcing PTE sized faults should be fine for fsdax, but it'll break
> >> devdax. The devdax driver requires the fault size be >= the namespace
> >> alignment since devdax tries to guarantee hugepage mappings will be
> >> used and PMD alignment is the default. We can probably have devdax
> >> fall back to the largest size the hypervisor has made available, but
> >> it does run contrary to the design. Ah well, I suppose it's better off
> >> being degraded rather than unusable.
> >
> > Given this is an explicit setting I think device-dax should explicitly
> > fail to enable in the presence of this flag to preserve the
> > application visible behavior.
> >
> > I.e. if device-dax was enabled after this setting was made then I
> > think future faults should fail as well.
>
> Not sure I understood that. Now we are disabling the ability to map
> pages as huge pages. I am now considering that this should not be
> user configurable. Ie, this is something that platform can use to avoid
> dax forcing huge page mapping, but if the architecture can enable huge
> dax mapping, we should always default to using that.
No, that's an application visible behavior regression. The side effect
of this setting is that all huge-page configured device-dax instances
must be disabled.
> Now w.r.t to failures, can device-dax do an opportunistic huge page
> usage?
device-dax explicitly disclaims the ability to do opportunistic mappings.
> I haven't looked at the device-dax details fully yet. Do we make the
> assumption of the mapping page size as a format w.r.t device-dax? Is that
> derived from nd_pfn->align value?
Correct.
>
> Here is what I am working on:
> 1) If the platform doesn't support huge page and if the device superblock
> indicated that it was created with huge page support, we fail the device
> init.
Ok.
> 2) Now if we are creating a new namespace without huge page support in
> the platform, then we force the align details to PAGE_SIZE. In such a
> configuration when handling dax fault even with THP enabled during
> the build, we should not try to use hugepage. This I think we can
> achieve by using TRANSPARENT_HUGEPAEG_DAX_FLAG.
How is this dynamic property communicated to the guest?
>
> Also even if the user decided to not use THP, by
> echo "never" > transparent_hugepage/enabled , we should continue to map
> dax fault using huge page on platforms that can support huge pages.
>
> This still doesn't cover the details of a device-dax created with
> PAGE_SIZE align later booted with a kernel that can do hugepage dax.How
> should we handle that? That makes me think, this should be a VMA flag
> which got derived from device config? May be use VM_HUGEPAGE to indicate
> if device should use a hugepage mapping or not?
device-dax configured with PAGE_SIZE always gets PAGE_SIZE mappings.
^ permalink raw reply
* Re: [PATCH 2/2] mm/dax: Don't enable huge dax mapping by default
From: Dan Williams @ 2019-03-13 16:07 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: Jan Kara, linux-nvdimm, Linux Kernel Mailing List, Linux MM,
Oliver, Ross Zwisler, Andrew Morton, Michal Suchánek,
linuxppc-dev, Kirill A . Shutemov
In-Reply-To: <df01bf6e-84a1-53fb-bf0c-0957af2f79e1@linux.ibm.com>
On Wed, Mar 6, 2019 at 4:46 AM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> On 3/6/19 5:14 PM, Michal Suchánek wrote:
> > On Wed, 06 Mar 2019 14:47:33 +0530
> > "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> wrote:
> >
> >> Dan Williams <dan.j.williams@intel.com> writes:
> >>
> >>> On Thu, Feb 28, 2019 at 1:40 AM Oliver <oohall@gmail.com> wrote:
> >>>>
> >>>> On Thu, Feb 28, 2019 at 7:35 PM Aneesh Kumar K.V
> >>>> <aneesh.kumar@linux.ibm.com> wrote:
> >
> >> Also even if the user decided to not use THP, by
> >> echo "never" > transparent_hugepage/enabled , we should continue to map
> >> dax fault using huge page on platforms that can support huge pages.
> >
> > Is this a good idea?
> >
> > This knob is there for a reason. In some situations having huge pages
> > can severely impact performance of the system (due to host-guest
> > interaction or whatever) and the ability to really turn off all THP
> > would be important in those cases, right?
> >
>
> My understanding was that is not true for dax pages? These are not
> regular memory that got allocated. They are allocated out of /dev/dax/
> or /dev/pmem*. Do we have a reason not to use hugepages for mapping
> pages in that case?
The problem with the transparent_hugepage/enabled interface is that it
conflates performing compaction work to produce THP-pages with the
ability to map huge pages at all. The compaction is a nop for dax
because the memory is already statically allocated. If the
administrator does not want dax to consume huge TLB entries then don't
configure huge-page dax. If a hypervisor wants to force disable
huge-page-configured device-dax instances after the fact it seems we
need an explicit interface for that and not overload
transparent_hugepage/enabled.
^ permalink raw reply
* Re: [PATCH v6 0/4] Fix free/allocation of runtime gigantic pages
From: Dave Hansen @ 2019-03-13 16:41 UTC (permalink / raw)
To: Alexandre Ghiti, Andrew Morton, Vlastimil Babka, Catalin Marinas,
Will Deacon, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Martin Schwidefsky, Heiko Carstens,
Yoshinori Sato, Rich Felker, David S . Miller, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H . Peter Anvin, x86, Dave Hansen,
Andy Lutomirski, Peter Zijlstra, Mike Kravetz, linux-arm-kernel,
linux-kernel, linuxppc-dev, linux-s390, linux-sh, sparclinux,
linux-mm
In-Reply-To: <20190307132015.26970-1-alex@ghiti.fr>
On 3/7/19 5:20 AM, Alexandre Ghiti wrote:
> This series fixes sh and sparc that did not advertise their gigantic page
> support and then were not able to allocate and free those pages at runtime.
> It renames MEMORY_ISOLATION && COMPACTION || CMA condition into the more
> accurate CONTIG_ALLOC, since it allows the definition of alloc_contig_range
> function.
> Finally, it then fixes the wrong definition of ARCH_HAS_GIGANTIC_PAGE config
> that, without MEMORY_ISOLATION && COMPACTION || CMA defined, did not allow
> architectures to free boottime allocated gigantic pages although unrelated.
Looks good, thanks for all the changes. For everything generic in the
set, plus the x86 bits:
Acked-by: Dave Hansen <dave.hansen@intel.com>
^ permalink raw reply
* Re: [PATCH v6 0/4] Fix free/allocation of runtime gigantic pages
From: Alexandre Ghiti @ 2019-03-13 16:48 UTC (permalink / raw)
To: Dave Hansen, Andrew Morton, Vlastimil Babka, Catalin Marinas,
Will Deacon, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Martin Schwidefsky, Heiko Carstens,
Yoshinori Sato, Rich Felker, David S . Miller, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H . Peter Anvin, x86, Dave Hansen,
Andy Lutomirski, Peter Zijlstra, Mike Kravetz, linux-arm-kernel,
linux-kernel, linuxppc-dev, linux-s390, linux-sh, sparclinux,
linux-mm
In-Reply-To: <875e6287-9528-45ec-788c-9c785e548942@intel.com>
On 03/13/2019 05:41 PM, Dave Hansen wrote:
> On 3/7/19 5:20 AM, Alexandre Ghiti wrote:
>> This series fixes sh and sparc that did not advertise their gigantic page
>> support and then were not able to allocate and free those pages at runtime.
>> It renames MEMORY_ISOLATION && COMPACTION || CMA condition into the more
>> accurate CONTIG_ALLOC, since it allows the definition of alloc_contig_range
>> function.
>> Finally, it then fixes the wrong definition of ARCH_HAS_GIGANTIC_PAGE config
>> that, without MEMORY_ISOLATION && COMPACTION || CMA defined, did not allow
>> architectures to free boottime allocated gigantic pages although unrelated.
> Looks good, thanks for all the changes. For everything generic in the
> set, plus the x86 bits:
>
> Acked-by: Dave Hansen <dave.hansen@intel.com>
Thanks Dave,
Alex
^ permalink raw reply
* Re: [PATCH 1/7] ocxl: Provide global MMIO accessors for external drivers
From: Frederic Barrat @ 2019-03-13 18:06 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, Greg Kurz,
Andrew Donnellan, Alastair D'Silva, linuxppc-dev
In-Reply-To: <20190313041524.14644-2-alastair@au1.ibm.com>
Hi Alastair,
I only realize now that this patch looks a bit out of place and would
fit better later in the series: it assumes an external driver can get an
ocxl_afu handle, which can only happen after patch 5.
And since I'm nitpicking, the year at the top of the new mmio file could
be adjusted.
The mmio accessors look ok to me.
Fred
Le 13/03/2019 à 05:15, Alastair D'Silva a écrit :
> From: Alastair D'Silva <alastair@d-silva.org>
>
> External drivers that communicate via OpenCAPI will need to make
> MMIO calls to interact with the devices.
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> ---
> drivers/misc/ocxl/Makefile | 2 +-
> drivers/misc/ocxl/mmio.c | 234 +++++++++++++++++++++++++++++++++++++
> include/misc/ocxl.h | 113 ++++++++++++++++++
> 3 files changed, 348 insertions(+), 1 deletion(-)
> create mode 100644 drivers/misc/ocxl/mmio.c
>
> diff --git a/drivers/misc/ocxl/Makefile b/drivers/misc/ocxl/Makefile
> index 5229dcda8297..922e47cd4f0d 100644
> --- a/drivers/misc/ocxl/Makefile
> +++ b/drivers/misc/ocxl/Makefile
> @@ -1,7 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0+
> ccflags-$(CONFIG_PPC_WERROR) += -Werror
>
> -ocxl-y += main.o pci.o config.o file.o pasid.o
> +ocxl-y += main.o pci.o config.o file.o pasid.o mmio.o
> ocxl-y += link.o context.o afu_irq.o sysfs.o trace.o
> obj-$(CONFIG_OCXL) += ocxl.o
>
> diff --git a/drivers/misc/ocxl/mmio.c b/drivers/misc/ocxl/mmio.c
> new file mode 100644
> index 000000000000..7f6ebae1c6c7
> --- /dev/null
> +++ b/drivers/misc/ocxl/mmio.c
> @@ -0,0 +1,234 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// Copyright 2017 IBM Corp.
> +#include <linux/sched/mm.h>
> +#include "trace.h"
> +#include "ocxl_internal.h"
> +
> +int ocxl_global_mmio_read32(struct ocxl_afu *afu, size_t offset,
> + enum ocxl_endian endian, u32 *val)
> +{
> + if (offset > afu->config.global_mmio_size - 4)
> + return -EINVAL;
> +
> +#ifdef __BIG_ENDIAN__
> + if (endian == OCXL_HOST_ENDIAN)
> + endian = OCXL_BIG_ENDIAN;
> +#endif
> +
> + switch (endian) {
> + case OCXL_BIG_ENDIAN:
> + *val = readl_be((char *)afu->global_mmio_ptr + offset);
> + break;
> +
> + default:
> + *val = readl((char *)afu->global_mmio_ptr + offset);
> + break;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ocxl_global_mmio_read32);
> +
> +int ocxl_global_mmio_read64(struct ocxl_afu *afu, size_t offset,
> + enum ocxl_endian endian, u64 *val)
> +{
> + if (offset > afu->config.global_mmio_size - 8)
> + return -EINVAL;
> +
> +#ifdef __BIG_ENDIAN__
> + if (endian == OCXL_HOST_ENDIAN)
> + endian = OCXL_BIG_ENDIAN;
> +#endif
> +
> + switch (endian) {
> + case OCXL_BIG_ENDIAN:
> + *val = readq_be((char *)afu->global_mmio_ptr + offset);
> + break;
> +
> + default:
> + *val = readq((char *)afu->global_mmio_ptr + offset);
> + break;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ocxl_global_mmio_read64);
> +
> +int ocxl_global_mmio_write32(struct ocxl_afu *afu, size_t offset,
> + enum ocxl_endian endian, u32 val)
> +{
> + if (offset > afu->config.global_mmio_size - 4)
> + return -EINVAL;
> +
> +#ifdef __BIG_ENDIAN__
> + if (endian == OCXL_HOST_ENDIAN)
> + endian = OCXL_BIG_ENDIAN;
> +#endif
> +
> + switch (endian) {
> + case OCXL_BIG_ENDIAN:
> + writel_be(val, (char *)afu->global_mmio_ptr + offset);
> + break;
> +
> + default:
> + writel(val, (char *)afu->global_mmio_ptr + offset);
> + break;
> + }
> +
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ocxl_global_mmio_write32);
> +
> +int ocxl_global_mmio_write64(struct ocxl_afu *afu, size_t offset,
> + enum ocxl_endian endian, u64 val)
> +{
> + if (offset > afu->config.global_mmio_size - 8)
> + return -EINVAL;
> +
> +#ifdef __BIG_ENDIAN__
> + if (endian == OCXL_HOST_ENDIAN)
> + endian = OCXL_BIG_ENDIAN;
> +#endif
> +
> + switch (endian) {
> + case OCXL_BIG_ENDIAN:
> + writeq_be(val, (char *)afu->global_mmio_ptr + offset);
> + break;
> +
> + default:
> + writeq(val, (char *)afu->global_mmio_ptr + offset);
> + break;
> + }
> +
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ocxl_global_mmio_write64);
> +
> +int ocxl_global_mmio_set32(struct ocxl_afu *afu, size_t offset,
> + enum ocxl_endian endian, u32 mask)
> +{
> + u32 tmp;
> +
> + if (offset > afu->config.global_mmio_size - 4)
> + return -EINVAL;
> +
> +#ifdef __BIG_ENDIAN__
> + if (endian == OCXL_HOST_ENDIAN)
> + endian = OCXL_BIG_ENDIAN;
> +#endif
> +
> + switch (endian) {
> + case OCXL_BIG_ENDIAN:
> + tmp = readl_be((char *)afu->global_mmio_ptr + offset);
> + tmp |= mask;
> + writel_be(tmp, (char *)afu->global_mmio_ptr + offset);
> + break;
> +
> + default:
> + tmp = readl((char *)afu->global_mmio_ptr + offset);
> + tmp |= mask;
> + writel(tmp, (char *)afu->global_mmio_ptr + offset);
> + break;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ocxl_global_mmio_set32);
> +
> +int ocxl_global_mmio_set64(struct ocxl_afu *afu, size_t offset,
> + enum ocxl_endian endian, u64 mask)
> +{
> + u64 tmp;
> +
> + if (offset > afu->config.global_mmio_size - 8)
> + return -EINVAL;
> +
> +#ifdef __BIG_ENDIAN__
> + if (endian == OCXL_HOST_ENDIAN)
> + endian = OCXL_BIG_ENDIAN;
> +#endif
> +
> + switch (endian) {
> + case OCXL_BIG_ENDIAN:
> + tmp = readq_be((char *)afu->global_mmio_ptr + offset);
> + tmp |= mask;
> + writeq_be(tmp, (char *)afu->global_mmio_ptr + offset);
> + break;
> +
> + default:
> + tmp = readq((char *)afu->global_mmio_ptr + offset);
> + tmp |= mask;
> + writeq(tmp, (char *)afu->global_mmio_ptr + offset);
> + break;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ocxl_global_mmio_set64);
> +
> +int ocxl_global_mmio_clear32(struct ocxl_afu *afu, size_t offset,
> + enum ocxl_endian endian, u32 mask)
> +{
> + u32 tmp;
> +
> + if (offset > afu->config.global_mmio_size - 4)
> + return -EINVAL;
> +
> +#ifdef __BIG_ENDIAN__
> + if (endian == OCXL_HOST_ENDIAN)
> + endian = OCXL_BIG_ENDIAN;
> +#endif
> +
> + switch (endian) {
> + case OCXL_BIG_ENDIAN:
> + tmp = readl_be((char *)afu->global_mmio_ptr + offset);
> + tmp &= ~mask;
> + writel_be(tmp, (char *)afu->global_mmio_ptr + offset);
> + break;
> +
> + default:
> + tmp = readl((char *)afu->global_mmio_ptr + offset);
> + tmp &= ~mask;
> + writel(tmp, (char *)afu->global_mmio_ptr + offset);
> + break;
> + }
> +
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ocxl_global_mmio_clear32);
> +
> +int ocxl_global_mmio_clear64(struct ocxl_afu *afu, size_t offset,
> + enum ocxl_endian endian, u64 mask)
> +{
> + u64 tmp;
> +
> + if (offset > afu->config.global_mmio_size - 8)
> + return -EINVAL;
> +
> +#ifdef __BIG_ENDIAN__
> + if (endian == OCXL_HOST_ENDIAN)
> + endian = OCXL_BIG_ENDIAN;
> +#endif
> +
> + switch (endian) {
> + case OCXL_BIG_ENDIAN:
> + tmp = readq_be((char *)afu->global_mmio_ptr + offset);
> + tmp &= ~mask;
> + writeq_be(tmp, (char *)afu->global_mmio_ptr + offset);
> + break;
> +
> + default:
> + tmp = readq((char *)afu->global_mmio_ptr + offset);
> + tmp &= ~mask;
> + writeq(tmp, (char *)afu->global_mmio_ptr + offset);
> + break;
> + }
> +
> + writeq(tmp, (char *)afu->global_mmio_ptr + offset);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ocxl_global_mmio_clear64);
> diff --git a/include/misc/ocxl.h b/include/misc/ocxl.h
> index 9530d3be1b30..3b320c39f0af 100644
> --- a/include/misc/ocxl.h
> +++ b/include/misc/ocxl.h
> @@ -49,6 +49,119 @@ struct ocxl_fn_config {
> s8 max_afu_index;
> };
>
> +// These are opaque outside the ocxl driver
> +struct ocxl_afu;
> +
> +enum ocxl_endian {
> + OCXL_BIG_ENDIAN = 0, /**< AFU data is big-endian */
> + OCXL_LITTLE_ENDIAN = 1, /**< AFU data is little-endian */
> + OCXL_HOST_ENDIAN = 2, /**< AFU data is the same endianness as the host */
> +};
> +
> +/**
> + * Read a 32 bit value from global MMIO
> + *
> + * @afu: The AFU
> + * @offset: The Offset from the start of MMIO
> + * @endian: the endianness that the MMIO data is in
> + * @val: returns the value
> + *
> + * Returns 0 for success, negative on error
> + */
> +int ocxl_global_mmio_read32(struct ocxl_afu *afu, size_t offset,
> + enum ocxl_endian endian, u32 *val);
> +
> +/**
> + * Read a 64 bit value from global MMIO
> + *
> + * @afu: The AFU
> + * @offset: The Offset from the start of MMIO
> + * @endian: the endianness that the MMIO data is in
> + * @val: returns the value
> + *
> + * Returns 0 for success, negative on error
> + */
> +int ocxl_global_mmio_read64(struct ocxl_afu *afu, size_t offset,
> + enum ocxl_endian endian, u64 *val);
> +
> +/**
> + * Write a 32 bit value to global MMIO
> + *
> + * @afu: The AFU
> + * @offset: The Offset from the start of MMIO
> + * @endian: the endianness that the MMIO data is in
> + * @val: The value to write
> + *
> + * Returns 0 for success, negative on error
> + */
> +int ocxl_global_mmio_write32(struct ocxl_afu *afu, size_t offset,
> + enum ocxl_endian endian, u32 val);
> +
> +/**
> + * Write a 64 bit value to global MMIO
> + *
> + * @afu: The AFU
> + * @offset: The Offset from the start of MMIO
> + * @endian: the endianness that the MMIO data is in
> + * @val: The value to write
> + *
> + * Returns 0 for success, negative on error
> + */
> +int ocxl_global_mmio_write64(struct ocxl_afu *afu, size_t offset,
> + enum ocxl_endian endian, u64 val);
> +
> +/**
> + * Set bits in a 32 bit global MMIO register
> + *
> + * @afu: The AFU
> + * @offset: The Offset from the start of MMIO
> + * @endian: the endianness that the MMIO data is in
> + * @mask: a mask of the bits to set
> + *
> + * Returns 0 for success, negative on error
> + */
> +int ocxl_global_mmio_set32(struct ocxl_afu *afu, size_t offset,
> + enum ocxl_endian endian, u32 mask);
> +
> +/**
> + * Set bits in a 64 bit global MMIO register
> + *
> + * @afu: The AFU
> + * @offset: The Offset from the start of MMIO
> + * @endian: the endianness that the MMIO data is in
> + * @mask: a mask of the bits to set
> + *
> + * Returns 0 for success, negative on error
> + */
> +int ocxl_global_mmio_set64(struct ocxl_afu *afu, size_t offset,
> + enum ocxl_endian endian, u64 mask);
> +
> +/**
> + * Set bits in a 32 bit global MMIO register
> + *
> + * @afu: The AFU
> + * @offset: The Offset from the start of MMIO
> + * @endian: the endianness that the MMIO data is in
> + * @mask: a mask of the bits to set
> + *
> + * Returns 0 for success, negative on error
> + */
> +int ocxl_global_mmio_clear32(struct ocxl_afu *afu, size_t offset,
> + enum ocxl_endian endian, u32 mask);
> +
> +/**
> + * Set bits in a 64 bit global MMIO register
> + *
> + * @afu: The AFU
> + * @offset: The Offset from the start of MMIO
> + * @endian: the endianness that the MMIO data is in
> + * @mask: a mask of the bits to set
> + *
> + * Returns 0 for success, negative on error
> + */
> +int ocxl_global_mmio_clear64(struct ocxl_afu *afu, size_t offset,
> + enum ocxl_endian endian, u64 mask);
> +
> /*
> * Read the configuration space of a function and fill in a
> * ocxl_fn_config structure with all the function details
>
^ permalink raw reply
* Re: [PATCH 2/7] ocxl: Allow external drivers to use OpenCAPI contexts
From: Frederic Barrat @ 2019-03-13 18:20 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, Greg Kurz,
Andrew Donnellan, Alastair D'Silva, linuxppc-dev
In-Reply-To: <20190313041524.14644-3-alastair@au1.ibm.com>
Le 13/03/2019 à 05:15, Alastair D'Silva a écrit :
> From: Alastair D'Silva <alastair@d-silva.org>
>
> Most OpenCAPI operations require a valid context, so
> exposing these functions to external drivers is necessary.
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> ---
Patch could also be moved later in the series (same reason as before), but:
Acked-by: Frederic Barrat <fbarrat@linux.ibm.com>
> drivers/misc/ocxl/context.c | 9 +++++--
> drivers/misc/ocxl/file.c | 2 +-
> drivers/misc/ocxl/ocxl_internal.h | 6 -----
> include/misc/ocxl.h | 44 +++++++++++++++++++++++++++++++
> 4 files changed, 52 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/misc/ocxl/context.c b/drivers/misc/ocxl/context.c
> index 3498a0199bde..1534b56f1db1 100644
> --- a/drivers/misc/ocxl/context.c
> +++ b/drivers/misc/ocxl/context.c
> @@ -8,6 +8,7 @@ struct ocxl_context *ocxl_context_alloc(void)
> {
> return kzalloc(sizeof(struct ocxl_context), GFP_KERNEL);
> }
> +EXPORT_SYMBOL_GPL(ocxl_context_alloc);
>
> int ocxl_context_init(struct ocxl_context *ctx, struct ocxl_afu *afu,
> struct address_space *mapping)
> @@ -43,6 +44,7 @@ int ocxl_context_init(struct ocxl_context *ctx, struct ocxl_afu *afu,
> ocxl_afu_get(afu);
> return 0;
> }
> +EXPORT_SYMBOL_GPL(ocxl_context_init);
>
> /*
> * Callback for when a translation fault triggers an error
> @@ -63,7 +65,7 @@ static void xsl_fault_error(void *data, u64 addr, u64 dsisr)
> wake_up_all(&ctx->events_wq);
> }
>
> -int ocxl_context_attach(struct ocxl_context *ctx, u64 amr)
> +int ocxl_context_attach(struct ocxl_context *ctx, u64 amr, struct mm_struct *mm)
> {
> int rc;
>
> @@ -75,7 +77,7 @@ int ocxl_context_attach(struct ocxl_context *ctx, u64 amr)
> }
>
> rc = ocxl_link_add_pe(ctx->afu->fn->link, ctx->pasid,
> - current->mm->context.id, ctx->tidr, amr, current->mm,
> + mm->context.id, ctx->tidr, amr, mm,
> xsl_fault_error, ctx);
> if (rc)
> goto out;
> @@ -85,6 +87,7 @@ int ocxl_context_attach(struct ocxl_context *ctx, u64 amr)
> mutex_unlock(&ctx->status_mutex);
> return rc;
> }
> +EXPORT_SYMBOL_GPL(ocxl_context_attach);
>
> static vm_fault_t map_afu_irq(struct vm_area_struct *vma, unsigned long address,
> u64 offset, struct ocxl_context *ctx)
> @@ -243,6 +246,7 @@ int ocxl_context_detach(struct ocxl_context *ctx)
> }
> return 0;
> }
> +EXPORT_SYMBOL_GPL(ocxl_context_detach);
>
> void ocxl_context_detach_all(struct ocxl_afu *afu)
> {
> @@ -280,3 +284,4 @@ void ocxl_context_free(struct ocxl_context *ctx)
> ocxl_afu_put(ctx->afu);
> kfree(ctx);
> }
> +EXPORT_SYMBOL_GPL(ocxl_context_free);
> diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
> index 16eb8a60d5c7..865b3d176431 100644
> --- a/drivers/misc/ocxl/file.c
> +++ b/drivers/misc/ocxl/file.c
> @@ -100,7 +100,7 @@ static long afu_ioctl_attach(struct ocxl_context *ctx,
> return -EINVAL;
>
> amr = arg.amr & mfspr(SPRN_UAMOR);
> - rc = ocxl_context_attach(ctx, amr);
> + rc = ocxl_context_attach(ctx, amr, current->mm);
> return rc;
> }
>
> diff --git a/drivers/misc/ocxl/ocxl_internal.h b/drivers/misc/ocxl/ocxl_internal.h
> index 06fd98c989c8..779d15ef60b5 100644
> --- a/drivers/misc/ocxl/ocxl_internal.h
> +++ b/drivers/misc/ocxl/ocxl_internal.h
> @@ -130,15 +130,9 @@ int ocxl_config_check_afu_index(struct pci_dev *dev,
> */
> 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);
> -int ocxl_context_attach(struct ocxl_context *ctx, u64 amr);
> int ocxl_context_mmap(struct ocxl_context *ctx,
> struct vm_area_struct *vma);
> -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);
>
> int ocxl_sysfs_add_afu(struct ocxl_afu *afu);
> void ocxl_sysfs_remove_afu(struct ocxl_afu *afu);
> diff --git a/include/misc/ocxl.h b/include/misc/ocxl.h
> index 3b320c39f0af..ebbfe83cd5a6 100644
> --- a/include/misc/ocxl.h
> +++ b/include/misc/ocxl.h
> @@ -51,6 +51,7 @@ struct ocxl_fn_config {
>
> // These are opaque outside the ocxl driver
> struct ocxl_afu;
> +struct ocxl_context;
>
> enum ocxl_endian {
> OCXL_BIG_ENDIAN = 0, /**< AFU data is big-endian */
> @@ -58,6 +59,49 @@ enum ocxl_endian {
> OCXL_HOST_ENDIAN = 2, /**< AFU data is the same endianness as the host */
> };
>
> +/**
> + * Allocate space for a new OpenCAPI context
> + *
> + * Returns NULL on failure
> + */
> +struct ocxl_context *ocxl_context_alloc(void);
> +
> +/**
> + * Initialize an OpenCAPI context
> + *
> + * @ctx: The OpenCAPI context to initialize
> + * @afu: The AFU the context belongs to
> + * @mapping: The mapping to unmap when the context is closed (may be NULL)
> + */
> +int ocxl_context_init(struct ocxl_context *ctx, struct ocxl_afu *afu,
> + struct address_space *mapping);
> +
> +/**
> + * Free an OpenCAPI context
> + *
> + * @ctx: The OpenCAPI context to free
> + */
> +void ocxl_context_free(struct ocxl_context *ctx);
> +
> +/**
> + * Grant access to an MM to an OpenCAPI context
> + * @ctx: The OpenCAPI context to attach
> + * @amr: The value of the AMR register to restrict access
> + * @mm: The mm to attach to the context
> + *
> + * Returns 0 on success, negative on failure
> + */
> +int ocxl_context_attach(struct ocxl_context *ctx, u64 amr,
> + struct mm_struct *mm);
> +
> +/**
> + * Detach an MM from an OpenCAPI context
> + * @ctx: The OpenCAPI context to attach
> + *
> + * Returns 0 on success, negative on failure
> + */
> +int ocxl_context_detach(struct ocxl_context *ctx);
> +
> /**
> * Read a 32 bit value from global MMIO
> *
>
^ permalink raw reply
* Re: [PATCH v3] powerpc/mm: move warning from resize_hpt_for_hotplug()
From: Christophe Leroy @ 2019-03-13 19:16 UTC (permalink / raw)
To: Laurent Vivier, linux-kernel; +Cc: linuxppc-dev, David Gibson
In-Reply-To: <20190313102528.4632-1-lvivier@redhat.com>
Le 13/03/2019 à 11:25, 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.
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
>
> 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:
>
^ permalink raw reply
* [PATCH v2] powerpc/64s: Remove 'dummy_copy_buffer'
From: Mathieu Malaterre @ 2019-03-13 20:00 UTC (permalink / raw)
To: Michael Ellerman
Cc: Mathieu Malaterre, linux-kernel, Nicholas Piggin, Paul Mackerras,
linuxppc-dev
In-Reply-To: <20190312202921.31331-1-malat@debian.org>
In commit 2bf1071a8d50 ("powerpc/64s: Remove POWER9 DD1 support") the
function __switch_to remove usage for 'dummy_copy_buffer'. Since it is
not used anywhere else, remove it completely.
This remove the following warning:
arch/powerpc/kernel/process.c:1156:17: error: 'dummy_copy_buffer' defined but not used [-Werror=unused-const-variable=]
Cc: Nicholas Piggin <npiggin@gmail.com>
Suggested-by: Christophe Leroy <christophe.leroy@c-s.fr>
Signed-off-by: Mathieu Malaterre <malat@debian.org>
---
v2: complely remove dummy_copy_buffer instead of marking it as used (since not anymore)
arch/powerpc/kernel/process.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 77e44275d025..b2f0daa2fcf1 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1151,11 +1151,6 @@ static inline void restore_sprs(struct thread_struct *old_thread,
thread_pkey_regs_restore(new_thread, old_thread);
}
-#ifdef CONFIG_PPC_BOOK3S_64
-#define CP_SIZE 128
-static const u8 dummy_copy_buffer[CP_SIZE] __attribute__((aligned(CP_SIZE)));
-#endif
-
struct task_struct *__switch_to(struct task_struct *prev,
struct task_struct *new)
{
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] powerpc: sstep: Mark variable `rc` as unused in function 'analyse_instr'
From: Mathieu Malaterre @ 2019-03-13 20:12 UTC (permalink / raw)
To: Christophe Leroy; +Cc: Paul Mackerras, linuxppc-dev, LKML
In-Reply-To: <94d90740-c970-01b0-c430-bafc2e9eecd3@c-s.fr>
On Tue, Mar 12, 2019 at 10:26 PM Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
>
>
>
> Le 12/03/2019 à 22:12, Mathieu Malaterre a écrit :
> > On Tue, Mar 12, 2019 at 9:56 PM Christophe Leroy
> > <christophe.leroy@c-s.fr> wrote:
> >>
> >>
> >>
> >> Le 12/03/2019 à 21:20, Mathieu Malaterre a écrit :
> >>> Add gcc attribute unused for `rc` variable.
> >>>
> >>> Fix warnings treated as errors with W=1:
> >>>
> >>> arch/powerpc/lib/sstep.c:1172:31: error: variable 'rc' set but not used [-Werror=unused-but-set-variable]
> >>>
> >>> Signed-off-by: Mathieu Malaterre <malat@debian.org>
> >>> ---
> >>> 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 3d33fb509ef4..32d092f62ae0 100644
> >>> --- a/arch/powerpc/lib/sstep.c
> >>> +++ b/arch/powerpc/lib/sstep.c
> >>> @@ -1169,7 +1169,7 @@ static nokprobe_inline int trap_compare(long v1, long v2)
> >>> int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
> >>> unsigned int instr)
> >>> {
> >>> - unsigned int opcode, ra, rb, rc, rd, spr, u;
> >>> + unsigned int opcode, ra, rb, rc __maybe_unused, rd, spr, u;
> >>
> >> I think it would be better to enclose 'rc' inside a #ifdef CONFIG_PPC64
> >
> > Hum odd, I would have bet you would have suggested me to use
> > IS_ENABLED with some crazy scheme (I was not able to mix it with the
> > switch case nicely).
>
> Well I guess yes, you could also get rid of the #ifdef __powerpc64__ and
> instead add the following just after the 'case 4:'
>
> if (!IS_ENABLED(CONFIG_64))
> break;
>
> That's less uggly than adding two #ifdef/#endif
So you mean changing:
#ifdef __powerpc64__
case 4:
if (!cpu_has_feature(CPU_FTR_ARCH_300))
return -1;
into:
case 4:
if (!IS_ENABLED(CONFIG_PPC64))
break;
if (!cpu_has_feature(CPU_FTR_ARCH_300))
return -1;
So suddenly case label '4' becomes visible for ppc32, is that really
what you wanted ?
> Christophe
>
> >
> > Anyway I'll try your suggestion and post a v2.
> >
> >> Christophe
> >>
> >>> unsigned long int imm;
> >>> unsigned long int val, val2;
> >>> unsigned int mb, me, sh;
> >>>
^ permalink raw reply
* Re: [PATCH] powerpc: Make some functions static
From: Mathieu Malaterre @ 2019-03-13 20:12 UTC (permalink / raw)
To: Christophe Leroy; +Cc: Paul Mackerras, linuxppc-dev, LKML
In-Reply-To: <607b412d-e5a2-5d08-7fb9-53a24e6c76d4@c-s.fr>
On Tue, Mar 12, 2019 at 10:14 PM Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
>
>
>
> Le 12/03/2019 à 21:31, Mathieu Malaterre a écrit :
> > In commit cb9e4d10c448 ("[POWERPC] Add support for 750CL Holly board")
> > new functions were added. Since these functions can be made static,
> > make it so. While doing so, it turns out that holly_power_off and
> > holly_halt are unused, so remove them.
>
> I would have said 'since these functions are only used in this C file,
> make them static'.
>
> I think this could be split in two patches:
> 1/ Remove unused functions, ie holly_halt() and holly_power_off().
> 2/ Make the other ones static.
Michael do you want two patches ?
> Christophe
>
> >
> > Silence the following warnings triggered using W=1:
> >
> > arch/powerpc/platforms/embedded6xx/holly.c:47:5: error: no previous prototype for 'holly_exclude_device' [-Werror=missing-prototypes]
> > arch/powerpc/platforms/embedded6xx/holly.c:190:6: error: no previous prototype for 'holly_show_cpuinfo' [-Werror=missing-prototypes]
> > arch/powerpc/platforms/embedded6xx/holly.c:196:17: error: no previous prototype for 'holly_restart' [-Werror=missing-prototypes]
> > arch/powerpc/platforms/embedded6xx/holly.c:236:6: error: no previous prototype for 'holly_power_off' [-Werror=missing-prototypes]
> > arch/powerpc/platforms/embedded6xx/holly.c:243:6: error: no previous prototype for 'holly_halt' [-Werror=missing-prototypes]
> >
> > Signed-off-by: Mathieu Malaterre <malat@debian.org>
> > ---
> > arch/powerpc/platforms/embedded6xx/holly.c | 19 ++++---------------
> > 1 file changed, 4 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/embedded6xx/holly.c b/arch/powerpc/platforms/embedded6xx/holly.c
> > index 0409714e8070..829bf3697dc9 100644
> > --- a/arch/powerpc/platforms/embedded6xx/holly.c
> > +++ b/arch/powerpc/platforms/embedded6xx/holly.c
> > @@ -44,7 +44,8 @@
> >
> > #define HOLLY_PCI_CFG_PHYS 0x7c000000
> >
> > -int holly_exclude_device(struct pci_controller *hose, u_char bus, u_char devfn)
> > +static int holly_exclude_device(struct pci_controller *hose, u_char bus,
> > + u_char devfn)
> > {
> > if (bus == 0 && PCI_SLOT(devfn) == 0)
> > return PCIBIOS_DEVICE_NOT_FOUND;
> > @@ -187,13 +188,13 @@ static void __init holly_init_IRQ(void)
> > tsi108_write_reg(TSI108_MPIC_OFFSET + 0x30c, 0);
> > }
> >
> > -void holly_show_cpuinfo(struct seq_file *m)
> > +static void holly_show_cpuinfo(struct seq_file *m)
> > {
> > seq_printf(m, "vendor\t\t: IBM\n");
> > seq_printf(m, "machine\t\t: PPC750 GX/CL\n");
> > }
> >
> > -void __noreturn holly_restart(char *cmd)
> > +static void __noreturn holly_restart(char *cmd)
> > {
> > __be32 __iomem *ocn_bar1 = NULL;
> > unsigned long bar;
> > @@ -233,18 +234,6 @@ void __noreturn holly_restart(char *cmd)
> > for (;;) ;
> > }
> >
> > -void holly_power_off(void)
> > -{
> > - local_irq_disable();
> > - /* No way to shut power off with software */
> > - for (;;) ;
> > -}
> > -
> > -void holly_halt(void)
> > -{
> > - holly_power_off();
> > -}
> > -
> > /*
> > * Called very early, device-tree isn't unflattened
> > */
> >
^ permalink raw reply
* Re: [PATCH] powerpc: use $(origin ARCH) to select KBUILD_DEFCONFIG
From: Mathieu Malaterre @ 2019-03-13 20:17 UTC (permalink / raw)
To: Michael Ellerman; +Cc: Masahiro Yamada, linuxppc-dev, LKML, Paul Mackerras
In-Reply-To: <CAK7LNAQUEivnhFnmkP+5uCHRJcYWDtK_fm2gB0rsbzGv1taBCg@mail.gmail.com>
On Sat, Feb 16, 2019 at 3:26 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> On Sat, Feb 16, 2019 at 1:11 AM Mathieu Malaterre <malat@debian.org> wrote:
> >
> > On Fri, Feb 15, 2019 at 10:41 AM Masahiro Yamada
> > <yamada.masahiro@socionext.com> wrote:
> > >
> > > I often test all Kconfig commands for all architectures. To ease my
> > > workflow, I want 'make defconfig' at least working without any cross
> > > compiler.
> > >
> > > Currently, arch/powerpc/Makefile checks CROSS_COMPILE to decide the
> > > default defconfig source.
> > >
> > > If CROSS_COMPILE is unset, it is likely to be the native build, so
> > > 'uname -m' is useful to choose the defconfig. If CROSS_COMPILE is set,
> > > the user is cross-building (i.e. 'uname -m' is probably x86_64), so
> > > it falls back to ppc64_defconfig. Yup, make sense.
> > >
> > > However, I want to run 'make ARCH=* defconfig' without setting
> > > CROSS_COMPILE for each architecture.
> > >
> > > My suggestion is to check $(origin ARCH).
> > >
> > > When you cross-compile the kernel, you need to set ARCH from your
> > > environment or from the command line.
> > >
> > > For the native build, you do not need to set ARCH. The default in
> > > the top Makefile is used:
> > >
> > > ARCH ?= $(SUBARCH)
> > >
> > > Hence, $(origin ARCH) returns 'file'.
> > >
> > > Before this commit, 'make ARCH=powerpc defconfig' failed:
> >
> > In case you have not seen it, please check:
> >
> > http://patchwork.ozlabs.org/patch/1037835/
>
>
> I did not know that because I do not subscribe to ppc ML.
>
>
> Michael's patch looks good to me.
OK
>
> If you mimic x86, the following will work:
>
Nice! Michael do you have a preference ?
>
>
> diff --git a/Makefile b/Makefile
> index 86cf35d..eb9552d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -356,6 +356,11 @@ ifeq ($(ARCH),sh64)
> SRCARCH := sh
> endif
>
> +# Additional ARCH settings for powerpc
> +ifneq ($(filter ppc%,$(ARCH)),)
> + SRCARCH := powerpc
> +endif
> +
> KCONFIG_CONFIG ?= .config
> export KCONFIG_CONFIG
>
> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
> index 488c9ed..ff01fef 100644
> --- a/arch/powerpc/Makefile
> +++ b/arch/powerpc/Makefile
> @@ -34,10 +34,10 @@ ifdef CONFIG_PPC_BOOK3S_32
> KBUILD_CFLAGS += -mcpu=powerpc
> endif
>
> -ifeq ($(CROSS_COMPILE),)
> -KBUILD_DEFCONFIG := $(shell uname -m)_defconfig
> -else
> +ifeq ($(ARCH),powerpc)
> KBUILD_DEFCONFIG := ppc64_defconfig
> +else
> +KBUILD_DEFCONFIG := $(ARCH)_defconfig
> endif
>
> ifdef CONFIG_PPC64
> diff --git a/scripts/subarch.include b/scripts/subarch.include
> index 6506828..c98323f 100644
> --- a/scripts/subarch.include
> +++ b/scripts/subarch.include
> @@ -8,6 +8,6 @@ SUBARCH := $(shell uname -m | sed -e s/i.86/x86/ -e
> s/x86_64/x86/ \
> -e s/sun4u/sparc64/ \
> -e s/arm.*/arm/ -e s/sa110/arm/ \
> -e s/s390x/s390/ -e s/parisc64/parisc/ \
> - -e s/ppc.*/powerpc/ -e s/mips.*/mips/ \
> + -e s/mips.*/mips/ \
> -e s/sh[234].*/sh/ -e s/aarch64.*/arm64/ \
> -e s/riscv.*/riscv/)
>
>
>
>
>
>
>
>
> --
> Best Regards
> Masahiro Yamada
^ permalink raw reply
* bpf jit PPC64 (BE) test_verifier PTR_TO_STACK store/load failure
From: Yauheni Kaliuta @ 2019-03-13 10:54 UTC (permalink / raw)
To: Naveen N. Rao, Sandipan Das, Daniel Borkmann, Michael Ellerman
Cc: netdev, bpf, linuxppc-dev, Jiri Olsa
Hi!
I found a failure:
```
# ./test_verifier 722
#722/u PTR_TO_STACK store/load FAIL retval -1 != -87117812
0: (bf) r1 = r10
1: (07) r1 += -10
2: (7a) *(u64 *)(r1 +2) = -87117812
3: (79) r0 = *(u64 *)(r1 +2)
4: (95) exit
processed 5 insns (limit 131072), stack depth 8
#722/p PTR_TO_STACK store/load FAIL retval -1 != -87117812
0: (bf) r1 = r10
1: (07) r1 += -10
2: (7a) *(u64 *)(r1 +2) = -87117812
3: (79) r0 = *(u64 *)(r1 +2)
4: (95) exit
processed 5 insns (limit 131072), stack depth 8
Summary: 0 PASSED, 0 SKIPPED, 2 FAILED
```
The reason is in the JIT. The code is jitted into:
[...]
d00000000580e7f8: f9 23 00 00 std r9,0(r3)
d00000000580e7fc: e9 03 00 02 lwa r8,0(r3)
[...]
so, it stores DW to the location r3, but loads W, i.e. in BE it is:
saves
r3: FF FF FF FF FA CE B0 0C
loads
r3: FF FF FF FF
(in LE it works semicorretly, saves 0C B0 CE FA FF FF FF FF, loads 0C B0 CE FA)
This is because of the handling of the +2 offset. For stores it is:
#define PPC_STD(r, base, i) EMIT(PPC_INST_STD | ___PPC_RS(r) | \
___PPC_RA(base) | ((i) & 0xfffc))
and for loads
#define PPC_LD(r, base, i) EMIT(PPC_INST_LD | ___PPC_RT(r) | \
___PPC_RA(base) | IMM_L(i))
#define IMM_L(i) ((uintptr_t)(i) & 0xffff)
So, in the load case the offset +2 (immediate value) is not
masked and turns the instruction to lwa instead of ld.
Would it be correct to & 0xfffc the immediate value as well?
BTW, the full run on big endian:
Summary: 1190 PASSED, 125 SKIPPED, 4 FAILED
--
WBR,
Yauheni Kaliuta
^ permalink raw reply
* Re: bpf jit PPC64 (BE) test_verifier PTR_TO_STACK store/load failure
From: Segher Boessenkool @ 2019-03-13 22:14 UTC (permalink / raw)
To: Yauheni Kaliuta
Cc: Daniel Borkmann, Jiri Olsa, Sandipan Das, netdev, Naveen N. Rao,
bpf, linuxppc-dev
In-Reply-To: <xuny4l87qc2v.fsf@redhat.com>
Hi!
On Wed, Mar 13, 2019 at 12:54:16PM +0200, Yauheni Kaliuta wrote:
> This is because of the handling of the +2 offset.
The low two bits of instructions with primary opcodes 58 and 62 are part
of the opcode, not the offset. These instructions can not have offsets
with the low two bits non-zero.
> For stores it is:
> #define PPC_STD(r, base, i) EMIT(PPC_INST_STD | ___PPC_RS(r) | \
> ___PPC_RA(base) | ((i) & 0xfffc))
>
> and for loads
> #define PPC_LD(r, base, i) EMIT(PPC_INST_LD | ___PPC_RT(r) | \
> ___PPC_RA(base) | IMM_L(i))
> #define IMM_L(i) ((uintptr_t)(i) & 0xffff)
>
> So, in the load case the offset +2 (immediate value) is not
> masked and turns the instruction to lwa instead of ld.
>
> Would it be correct to & 0xfffc the immediate value as well?
That is only part of it. The other thing is you have to make sure those
low bits are zero *already* (and then you do not need the mask anymore).
For example, if the low two bits are not zero load the offset into a
register instead (and then do ldx or lwax).
Segher
^ permalink raw reply
* Re: [PATCH 1/1] arch/powerpc: Rework local_paca to avoid LTO warnings
From: Daniel Axtens @ 2019-03-13 23:54 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>
"Alastair D'Silva" <alastair@au1.ibm.com> writes:
> 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’
>
> This patch reworks local_paca into an inline getter & setter function,
> which addresses the warning.
>
> Generated ASM from this patch is broadly similar (addresses have
> changed and the compiler uses different GPRs in some places).
Ditto to Christophe's comment; I'd love to know how to build this so I
can actually see the differences. Perhaps you could bundle up all the
required changes and send it as a patch series with a cover letter
explaining this?
> +static inline struct paca_struct *get_paca_no_preempt_check(void)
> +{
> + register struct paca_struct *paca asm("r13");
> + return paca;
> +}
Isn't the convention to have the { on the same line as the function, or
am I horrible mis-remembering things?
Should these functions be __always_inline?
Regards,
Daniel
> +
> +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");
> + 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 */
> --
> 2.20.1
^ permalink raw reply
* Re: [PATCH 1/1] arch/powerpc: Rework local_paca to avoid LTO warnings
From: Andrew Donnellan @ 2019-03-14 0:09 UTC (permalink / raw)
To: Daniel Axtens, Alastair D'Silva
Cc: Michal Hocko, Mahesh Salgaonkar, linux-kernel, Nicholas Piggin,
Mike Rapoport, Paul Mackerras, Alastair D'Silva,
Naveen N. Rao, linuxppc-dev, Andrew Morton
In-Reply-To: <87ef7atjnk.fsf@dja-thinkpad.axtens.net>
On 14/3/19 10:54 am, Daniel Axtens wrote:
>> +static inline struct paca_struct *get_paca_no_preempt_check(void)
>> +{
>> + register struct paca_struct *paca asm("r13");
>> + return paca;
>> +}
>
> Isn't the convention to have the { on the same line as the function, or
> am I horrible mis-remembering things?
However, there is one special case, namely functions: they have the
opening brace at the beginning of the next line, thus:
int function(int x)
{
body of function
}
Heretic people all over the world have claimed that this inconsistency
is ... well ... inconsistent, but all right-thinking people know that
(a) K&R are **right** and (b) K&R are right. Besides, functions are
special anyway (you can't nest them in C).
--
Andrew Donnellan OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com IBM Australia Limited
^ permalink raw reply
* Re: BUG: p8_aes_ctr randomly returns wrong results
From: Daniel Axtens @ 2019-03-14 0:53 UTC (permalink / raw)
To: Ondrej Mosnáček, linux-crypto, Herbert Xu
Cc: leo.barbosa, Stephan Mueller, nayna, leitao, pfsmorigo,
marcelo.cerri, linuxppc-dev
In-Reply-To: <CAAUqJDuegy=zw3SwQ1UsV881DjLSvZ9X3joFt89HkBy7y9=7pg@mail.gmail.com>
Hi Ondrej,
I can confirm this on a ppc64le VM, at least as far back as 4.18, and am
investigating. Thanks for the reproducer.
Regards,
Daniel
^ permalink raw reply
* Re: [PATCH 1/1] arch/powerpc: Rework local_paca to avoid LTO warnings
From: Alastair D'Silva @ 2019-03-14 1:39 UTC (permalink / raw)
To: Christophe Leroy
Cc: Michal Hocko, Mahesh Salgaonkar, linux-kernel, Nicholas Piggin,
Mike Rapoport, Paul Mackerras, Naveen N. Rao, linuxppc-dev,
Andrew Morton
In-Reply-To: <bd858b12-7fab-057d-0065-2ccb98cbf648@c-s.fr>
On Wed, 2019-03-13 at 10:06 +0100, Christophe Leroy wrote:
> Hello,
Thanks for reviewing :)
>
> 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 ?
I'm using Andi Kleen's LTO tree:
https://github.com/andikleen/linux-misc/tree/lto-420-1
with a few other patches:
https://github.com/andikleen/linux-misc/pull/27
You'll need to add the following to your .config:
CONFIG_LTO_MENU=y
CONFIG_LTO=y
>
> > 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/
These warnings are bogus, they replace warnings that flagged against
spinlock.h.
> > 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.
>
I'll see if I can improve it.
> > 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");
Hmm, I didn't see problems on PPC64 as that already uses an inline
function. I'll address this in another patch for the PPC32 case.
> > ---
> > 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.
I moved it to keep it close to the usage of it.
I suppose the new implementation should be in the same place though.
> > +
> > +static inline struct paca_struct *get_paca_no_preempt_check(void)
> > +{
> > + register struct paca_struct *paca asm("r13");
>
> Should be a blank line there.
Whoops, I thought I ran checkpatch, but clearly, I forgot. I'll
resubmit.
> > + 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
>
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
^ 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