On 23.08.18 14:43, Marc Zyngier wrote: > On 23/08/18 13:24, Alexander Graf wrote: >> On 08/23/2018 01:16 PM, Marc Zyngier wrote: >>> On 21/08/18 17:54, Alexander Graf wrote: >>>> On 08/21/2018 05:08 PM, Marc Zyngier wrote: >>>>> On 21/08/18 15:08, Alexander Graf wrote: >>>>>> On 08/21/2018 03:57 PM, Marc Zyngier wrote: >>>>>>> On 21/08/18 14:35, Alexander Graf wrote: >>>>>>>> On 10/23/2017 06:11 PM, Marc Zyngier wrote: >>>>>>>>> The only case where we actually need to perform a dcache maintenance >>>>>>>>> is when we map the page for the first time, and subsequent permission >>>>>>>>> faults do not require cache maintenance. Let's make it conditional >>>>>>>>> on not being a permission fault (and thus a translation fault). >>>>>>>>> >>>>>>>>> Reviewed-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx> >>>>>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx> >>>>>>>> This patch unfortunately breaks something on Hi1616 SoCs when running >>>>>>>> 32bit guests. With this patch applied (and thus with 4.18) I get random >>>>>>>> illegal instruction warnings from 32bit code inside VMs. I do not know >>>>>>>> at this point whether this affects other CPUs as well. >>>>>>> Can you please give a few more details? >>>>>>> >>>>>>> - what are the CPUs on this Hi1616? At least a /proc/cpuinfo would help >>>>>> These are A72s: >>>>>> >>>>>> processor : 0 >>>>>> BogoMIPS : 100.00 >>>>>> Features : fp asimd evtstrm aes pmull sha1 sha2 crc32 cpuid >>>>>> CPU implementer : 0x41 >>>>>> CPU architecture: 8 >>>>>> CPU variant : 0x0 >>>>>> CPU part : 0xd08 >>>>>> CPU revision : 2 >>>>>> >>>>>>> - an example of the crash? Is it within the decompressor? After? This >>>>>>> things do matter, given the number of crazy things the 32bit kernel does >>>>>> They are always in user space. My current reproducer is this: >>>>>> >>>>>> $ while rpm -qa > /dev/null; do :; done >>>>>> >>>>>> If I run this in parallel with something that just populates RAM (dd >>>>>> if=/dev/nvme0n1 of=/dev/null bs=10G) I get an illegal instruction fault >>>>>> within seconds: >>>>>> >>>>>> sh-4.4# while rpm -qa > /dev/null; do true; done >>>>>> Illegal instruction (core dumped) >>>>>> >>>>>> >>>>>>> - a host kernel configuration? >>>>>> Host kernel configuration is just the normal openSUSE one: >>>>>> >>>>>> https://kernel.opensuse.org/cgit/kernel-source/plain/config/arm64/default?h=stable >>>>>> >>>>>>>> If anyone is interested in a reproducer, I have something handy. But for >>>>>>>> now I believe we should just revert this patch. >>>>>>> Before we revert anything, I'd like to understand what is happening. >>>>>> Yeah, I didn't realize the commit is already in since 4.16, so I agree. >>>>>> I'll bisect a bit, but it may take a while. >>>>> Do you mind giving this a try? >>>>> >>>>> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c >>>>> index 1d90d79706bd..df8f3d5eaa22 100644 >>>>> --- a/virt/kvm/arm/mmu.c >>>>> +++ b/virt/kvm/arm/mmu.c >>>>> @@ -1531,7 +1536,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, >>>>> kvm_set_pfn_dirty(pfn); >>>>> } >>>>> >>>>> - if (fault_status != FSC_PERM) >>>>> + if (fault_status != FSC_PERM || write_fault) >>>>> clean_dcache_guest_page(pfn, PMD_SIZE); >>>>> >>>>> if (exec_fault) { >>>>> @@ -1553,7 +1558,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, >>>>> mark_page_dirty(kvm, gfn); >>>>> } >>>>> >>>>> - if (fault_status != FSC_PERM) >>>>> + if (fault_status != FSC_PERM || write_fault) >>>>> clean_dcache_guest_page(pfn, PAGE_SIZE); >>>>> >>>>> if (exec_fault) { >>>>> >>>>> >>>>> The missing logic is that a write from the guest could have triggered >>>>> a CoW, meaning we definitely need to flush it in that case too. It >>>>> fixes a kvm-unit-test regression here. >>>> This patch unfortunately does not fix the issue. I still see illegal >>>> instructions. >>> [+Dave] >>> >>> That's because what you're observing has nothing to do with caching, but >>> with FP/SIMD trapping instead. Thanks to the guest image you've provided, >>> I've been able to extract the following: >>> >>> [ 3.944273] systemd-udevd (266): undefined instruction: pc=(ptrval) >>> [ 3.945396] CPU: 0 PID: 266 Comm: systemd-udevd Not tainted 4.17.14-2-lpae #1 openSUSE Tumbleweed (unreleased) >>> [ 3.947130] Hardware name: Generic DT based system >>> [ 3.947976] PC is at 0xb6b9397a >>> [ 3.948547] LR is at 0xb6e9a1b0 >>> [ 3.958291] pc : [<b6b9397a>] lr : [<b6e9a1b0>] psr: 20070030 >>> [ 3.959664] sp : bebe36a0 ip : b6f6ad50 fp : 005e8784 >>> [ 3.960601] r10: bebe3814 r9 : bebe36c0 r8 : bebe36bc >>> [ 3.961522] r7 : bebe3814 r6 : 00000074 r5 : 00000000 r4 : 00000000 >>> [ 3.962661] r3 : 005f2b60 r2 : 00000074 r1 : 00000000 r0 : bebe3814 >>> [ 3.963801] Flags: nzCv IRQs on FIQs on Mode USER_32 ISA Thumb Segment user >>> [ 4.000606] Control: 30c5383d Table: 6a8e7400 DAC: fffffffd >>> [ 4.001659] Code: d1f9 f1a0 0001 4770 (eee0) 1b10 >>> >>> maz@flakes:~/armv7-build-fail$ (echo .thumb; echo .inst.w 0xeee01b10) > x.S && arm-linux-gnueabihf-gcc -c x.S && arm-linux-gnueabihf-objdump -d x.o >>> >>> x.o: file format elf32-littlearm >>> >>> >>> Disassembly of section .text: >>> >>> 00000000 <.text>: >>> 0: eee0 1b10 vdup.8 q0, r1 >>> >>> A VFP instruction. Given that you've reported that things worked in >>> 4.17 and broke in 4.18, I strongly suspect the new lazy FPSIMD code. >>> Upon inspection, the way we setup trapping for 32bit is a tiny bit >>> suspect. >>> >>> Could you please give this patch a go? My Seattle has been running >>> with it for 30 minutes now, and it is still running (instead of >>> failing with seconds). >>> >>> Thanks, >>> >>> M. >>> >>> From f2d1d43e2429f9269ab6c565440e095ab3a9be89 Mon Sep 17 00:00:00 2001 >>> From: Marc Zyngier <marc.zyngier@xxxxxxx> >>> Date: Thu, 23 Aug 2018 11:51:43 +0100 >>> Subject: [PATCH] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD >>> >>> If trapping FPSIMD in the context of an AArch32 guest, it is critical >>> to set FPEXC32_EL2.EN to 1 so that the trapping is taken to EL2 and >>> not EL1. >>> >>> Conversely, it is just as critical *not* to set FPEXC32_EL2.EN to 1 >>> if we're not going to trap FPSIMD, as we then corrupt the existing >>> VFP state. >>> >>> Moving the call to __activate_traps_fpsimd32 to the point where we >>> know for sure that we are going to trap ensures that we don't set that >>> bit spuriously. >>> >>> Fixes: e6b673b741ea ("KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing") >>> Cc: Dave Martin <dave.martin@xxxxxxx> >>> Reported-by: Alexander Graf <agraf@xxxxxxx> >>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx> >> >> Very good catch. This patch does indeed fix it. I'm still puzzled why >> reverting the dcache patch actually had any positive effect, but oh well >> ... :) > > Well, there is still a bug in that dcache patch, which I've fixed > separately (in a better way than what I sent before), so you may have > hit that as well, but that's not the most visible bug. > >> Tested-by: Alexander Graf <agraf@xxxxxxx> > > Thanks for that. Can I expect to see your patch in 4.19 and 4.18-stable? :) Alex
