Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751514AbdFGMev convert rfc822-to-8bit (ORCPT ); Wed, 7 Jun 2017 08:34:51 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:52920 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751649AbdFGMeu (ORCPT ); Wed, 7 Jun 2017 08:34:50 -0400 Date: Wed, 7 Jun 2017 14:34:40 +0200 From: Martin Schwidefsky To: Christian Borntraeger Cc: Heiko Carstens , David Hildenbrand , kvm@vger.kernel.org, linux-kernel@vger.kernel.org, Thomas Huth , Andreas Krebbel Subject: Re: [PATCH RFC 0/2] KVM: s390: avoid having to enable vm.alloc_pgste In-Reply-To: <63b5c762-fbbb-b6dc-0f2d-6837dac5bb04@de.ibm.com> References: <20170529163202.13077-1-david@redhat.com> <20170601124651.3e7969ab@mschwideX1> <20170602070210.GA4221@osiris> <20170602114647.35e6d30f@mschwideX1> <2d45c9d5-2108-7408-e7cd-44b9f6d03b0f@de.ibm.com> <20170602125345.5ac9e12e@mschwideX1> <63b5c762-fbbb-b6dc-0f2d-6837dac5bb04@de.ibm.com> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 8BIT X-TM-AS-GCONF: 00 x-cbid: 17060712-0020-0000-0000-00000381E2C1 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17060712-0021-0000-0000-000041FCCE64 Message-Id: <20170607143440.7c86af85@mschwideX1> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-06-07_08:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1706070234 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10190 Lines: 273 On Fri, 2 Jun 2017 15:20:33 +0200 Christian Borntraeger wrote: > On 06/02/2017 12:53 PM, Martin Schwidefsky wrote: > > On Fri, 2 Jun 2017 12:19:19 +0200 > > Christian Borntraeger wrote: > > > >> On 06/02/2017 11:46 AM, Martin Schwidefsky wrote: > >>> On Fri, 2 Jun 2017 09:02:10 +0200 > >>> Heiko Carstens wrote: > >>> > >>>> On Thu, Jun 01, 2017 at 12:46:51PM +0200, Martin Schwidefsky wrote: > >>>>>> Unfortunately, converting all page tables to 4k pgste page tables is > >>>>>> not possible without provoking various race conditions. > >>>>> > >>>>> That is one approach we tried and was found to be buggy. The point is that > >>>>> you are not allowed to reallocate a page table while a VMA exists that is > >>>>> in the address range of that page table. > >>>>> > >>>>> Another approach we tried is to use an ELF flag on the qemu executable. > >>>>> That does not work either because fs/exec.c allocates and populates the > >>>>> new mm struct for the argument pages before fs/binfmt_elf.c comes into > >>>>> play. > >>>> > >>>> How about if you would fail the system call within arch_check_elf() if you > >>>> detect that the binary requires pgstes (as indicated by elf flags) and then > >>>> restart the system call? > >>>> > >>>> That is: arch_check_elf() e.g. would set a thread flag that future mm's > >>>> should be allocated with pgstes. Then do_execve() would cleanup everything > >>>> and return to entry.S. Upon return to userspace we detect this condition > >>>> and simply restart the system call, similar to signals vs -ERESTARTSYS. > >>>> > >>>> That would make do_execve() cleanup everything and upon reentering it would > >>>> allocate an mm with the pgste flag set. > >>>> > >>>> Maybe this is a bit over-simplified, but might work. > >>> > >>> This is not over-simplified at all, that does work: > >> > >> > >> Nice. Next question is how to integrate that elf flag into the qemu > >> build environment. > > > > So far I use a small C program to set the flag: > > > > #include > > #include > > #include > > #include > > > > #define ERREXIT(...) \ > > do { \ > > fprintf(stderr, __VA_ARGS__); \ > > exit(-1); \ > > } while (0) > > > > int main(int argc, char *argv[]) > > { > > Elf64_Ehdr ehdr; > > int fd, rc; > > > > if (argc != 2) > > ERREXIT("Usage: %s \n", argv[0]); > > > > fd = open(argv[1], O_RDWR); > > if (fd < 0) > > ERREXIT("Unable to open file %s\n", argv[1]); > > > > if (pread(fd, &ehdr, sizeof(ehdr), 0) != sizeof(ehdr) || > > memcmp(&ehdr.e_ident, ELFMAG, SELFMAG) != 0 || > > ehdr.e_ident[EI_CLASS] != ELFCLASS64 || > > ehdr.e_machine != EM_S390) > > ERREXIT("Invalid ELF file %s\n", argv[1]); > > > > ehdr.e_flags |= 0x00000002; > > > > if (pwrite(fd, &ehdr, sizeof(ehdr), 0) != sizeof(ehdr)) > > ERREXIT("Write to of file %s failed\n", argv[1]); > > > > close(fd); > > return 0; > > } > > > > Thanks for that. I assume this is mostly for testing and we want to have > toolchain support for that. Otherwise things like build-id (the sha variant) > might be wrong, no? Andreas Krebbel suggested to use a ELF segment type to mark the qemu binary, similar to GNU_STACK. For the kernel part of this solution see below, now we just need toolchain support to generate a qemu executable with the PT_S390_REQUEST_PGSTE segment. The -z linker option comes to mind, e.g. "-z request-pgste" -- >From 90c15b34c07644395d0bd96a632826740c539561 Mon Sep 17 00:00:00 2001 From: Martin Schwidefsky Date: Wed, 7 Jun 2017 14:10:24 +0200 Subject: [PATCH] s390/kvm: avoid global config of vm.alloc_pgste=1 The system control vm.alloc_pgste is used to control the size of the page tables, either 2K or 4K. The idea is that a KVM host sets the vm.alloc_pgste control to 1 which causes *all* new processes to run with 4K page tables. For a non-kvm system the control should stay off to save on memory used for page tables. Trouble is that distributions choose to set the control globally to be able to run KVM guests. This wastes memory on non-KVM systems. Introduce the PT_S390_REQUEST_PGSTE ELF segment type to "mark" the qemu executable with it. All executables with this (empty) segment in its ELF phdr array will be started with 4K page tables. Any executable without PT_S390_REQUEST_PGSTE will run with the default 2K page tables. This removes the need to set vm.alloc_pgste=1 for a KVM host and minimizes the waste of memory for page tables. Signed-off-by: Martin Schwidefsky --- arch/s390/Kconfig | 1 + arch/s390/include/asm/elf.h | 26 ++++++++++++++++++++++++++ arch/s390/include/asm/mmu_context.h | 3 ++- arch/s390/include/asm/ptrace.h | 2 ++ arch/s390/include/asm/thread_info.h | 1 + arch/s390/kernel/entry.S | 13 ++++++++++++- 6 files changed, 44 insertions(+), 2 deletions(-) diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig index 69a77eecaec1..7bd182676ddd 100644 --- a/arch/s390/Kconfig +++ b/arch/s390/Kconfig @@ -64,6 +64,7 @@ config ARCH_SUPPORTS_UPROBES config S390 def_bool y + select ARCH_BINFMT_ELF_STATE select ARCH_HAS_DEVMEM_IS_ALLOWED select ARCH_HAS_ELF_RANDOMIZE select ARCH_HAS_GCOV_PROFILE_ALL diff --git a/arch/s390/include/asm/elf.h b/arch/s390/include/asm/elf.h index e8f623041769..7811d1006642 100644 --- a/arch/s390/include/asm/elf.h +++ b/arch/s390/include/asm/elf.h @@ -117,6 +117,9 @@ #define ELF_DATA ELFDATA2MSB #define ELF_ARCH EM_S390 +/* s390 specific phdr types */ +#define PT_S390_REQUEST_PGSTE 0x70000000 + /* * ELF register definitions.. */ @@ -151,6 +154,29 @@ extern unsigned int vdso_enabled; && (x)->e_ident[EI_CLASS] == ELF_CLASS) #define compat_start_thread start_thread31 +struct arch_elf_state { +}; + +#define INIT_ARCH_ELF_STATE { } + +#define arch_check_elf(ehdr, interp, interp_ehdr, state) (0) +#define arch_elf_pt_proc(ehdr, phdr, elf, interp, state) \ +({ \ + struct elf64_hdr *_ehdr = (void *) ehdr; \ + struct elf64_phdr *_phdr = (void *) phdr; \ + int _rc = 0; \ + if (_ehdr->e_ident[EI_CLASS] == ELFCLASS64 && \ + _phdr->p_type == PT_S390_REQUEST_PGSTE && \ + !page_table_allocate_pgste && \ + !test_thread_flag(TIF_REQUEST_PGSTE)) { \ + set_thread_flag(TIF_REQUEST_PGSTE); \ + set_pt_regs_flag(task_pt_regs(current), \ + PIF_SYSCALL_RESTART); \ + _rc = -EAGAIN; \ + } \ + _rc; \ +}) + /* For SVR4/S390 the function pointer to be registered with `atexit` is passed in R14. */ #define ELF_PLAT_INIT(_r, load_addr) \ diff --git a/arch/s390/include/asm/mmu_context.h b/arch/s390/include/asm/mmu_context.h index c119d564d8f2..1201b18e817d 100644 --- a/arch/s390/include/asm/mmu_context.h +++ b/arch/s390/include/asm/mmu_context.h @@ -25,7 +25,8 @@ static inline int init_new_context(struct task_struct *tsk, mm->context.gmap_asce = 0; mm->context.flush_mm = 0; #ifdef CONFIG_PGSTE - mm->context.alloc_pgste = page_table_allocate_pgste; + mm->context.alloc_pgste = page_table_allocate_pgste || + test_thread_flag(TIF_REQUEST_PGSTE); mm->context.has_pgste = 0; mm->context.use_skey = 0; mm->context.use_cmma = 0; diff --git a/arch/s390/include/asm/ptrace.h b/arch/s390/include/asm/ptrace.h index 99bc456cc26a..24baa80f7af6 100644 --- a/arch/s390/include/asm/ptrace.h +++ b/arch/s390/include/asm/ptrace.h @@ -11,9 +11,11 @@ #define PIF_SYSCALL 0 /* inside a system call */ #define PIF_PER_TRAP 1 /* deliver sigtrap on return to user */ +#define PIF_SYSCALL_RESTART 2 /* restart the current system call */ #define _PIF_SYSCALL _BITUL(PIF_SYSCALL) #define _PIF_PER_TRAP _BITUL(PIF_PER_TRAP) +#define _PIF_SYSCALL_RESTART _BITUL(PIF_SYSCALL_RESTART) #ifndef __ASSEMBLY__ diff --git a/arch/s390/include/asm/thread_info.h b/arch/s390/include/asm/thread_info.h index f36e6e2b73f0..e7444d76cc63 100644 --- a/arch/s390/include/asm/thread_info.h +++ b/arch/s390/include/asm/thread_info.h @@ -56,6 +56,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src); #define TIF_NEED_RESCHED 2 /* rescheduling necessary */ #define TIF_UPROBE 3 /* breakpointed or single-stepping */ #define TIF_GUARDED_STORAGE 4 /* load guarded storage control block */ +#define TIF_REQUEST_PGSTE 5 /* New mm's will use 4K page tables */ #define TIF_SYSCALL_TRACE 8 /* syscall trace active */ #define TIF_SYSCALL_AUDIT 9 /* syscall auditing active */ #define TIF_SECCOMP 10 /* secure computing */ diff --git a/arch/s390/kernel/entry.S b/arch/s390/kernel/entry.S index 0c2c3b8bfc9a..8c824b32527a 100644 --- a/arch/s390/kernel/entry.S +++ b/arch/s390/kernel/entry.S @@ -52,7 +52,7 @@ _TIF_TRACE = (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | _TIF_SECCOMP | \ _TIF_SYSCALL_TRACEPOINT) _CIF_WORK = (_CIF_MCCK_PENDING | _CIF_ASCE_PRIMARY | \ _CIF_ASCE_SECONDARY | _CIF_FPU) -_PIF_WORK = (_PIF_PER_TRAP) +_PIF_WORK = (_PIF_PER_TRAP | _PIF_SYSCALL_RESTART) #define BASED(name) name-cleanup_critical(%r13) @@ -342,6 +342,8 @@ ENTRY(system_call) jo .Lsysc_guarded_storage TSTMSK __PT_FLAGS(%r11),_PIF_PER_TRAP jo .Lsysc_singlestep + TSTMSK __PT_FLAGS(%r11),_PIF_SYSCALL_RESTART + jo .Lsysc_syscall_restart TSTMSK __TI_flags(%r12),_TIF_SIGPENDING jo .Lsysc_sigpending TSTMSK __TI_flags(%r12),_TIF_NOTIFY_RESUME @@ -434,6 +436,15 @@ ENTRY(system_call) jg do_per_trap # +# _PIF_SYSCALL_RESTART is set, repeat the current system call +# +.Lsysc_syscall_restart: + ni __PT_FLAGS+7(%r11),255-_PIF_SYSCALL_RESTART + lmg %r1,%r7,__PT_R1(%r11) # load svc arguments + lg %r2,__PT_ORIG_GPR2(%r11) + j .Lsysc_do_svc + +# # call tracehook_report_syscall_entry/tracehook_report_syscall_exit before # and after the system call # -- 2.11.2 -- blue skies, Martin. "Reality continues to ruin my life." - Calvin.