Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1660199 > unrolled thread

Re: [PATCH RFC 0/2] KVM: s390: avoid having to enable vm.alloc_pgste

Started byHeiko Carstens <heiko.carstens@de.ibm.com>
First post2017-06-07 22:50 +0200
Last post2017-06-08 15:20 +0200
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH RFC 0/2] KVM: s390: avoid having to enable vm.alloc_pgste Heiko Carstens <heiko.carstens@de.ibm.com> - 2017-06-07 22:50 +0200
    Re: [PATCH RFC 0/2] KVM: s390: avoid having to enable  vm.alloc_pgste Martin Schwidefsky <schwidefsky@de.ibm.com> - 2017-06-08 07:40 +0200
      Re: [PATCH RFC 0/2] KVM: s390: avoid having to enable vm.alloc_pgste Heiko Carstens <heiko.carstens@de.ibm.com> - 2017-06-08 08:30 +0200
        Re: [PATCH RFC 0/2] KVM: s390: avoid having to enable  vm.alloc_pgste Martin Schwidefsky <schwidefsky@de.ibm.com> - 2017-06-08 13:30 +0200
          Re: [PATCH RFC 0/2] KVM: s390: avoid having to enable vm.alloc_pgste Heiko Carstens <heiko.carstens@de.ibm.com> - 2017-06-08 15:20 +0200

#1660199 — Re: [PATCH RFC 0/2] KVM: s390: avoid having to enable vm.alloc_pgste

FromHeiko Carstens <heiko.carstens@de.ibm.com>
Date2017-06-07 22:50 +0200
SubjectRe: [PATCH RFC 0/2] KVM: s390: avoid having to enable vm.alloc_pgste
Message-ID<tPQ3U-34T-27@gated-at.bofh.it>
On Wed, Jun 07, 2017 at 02:34:40PM +0200, Martin Schwidefsky wrote:
> +#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;							\
> +})

I'm wondering if this should simply fail, if a PT_S390_REQUEST_PGSTE type
segment exists, but it is not ELFCLASS64?
It will fail later anyway on s390_enable_sie(), but...

> 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);

I think the alloc_pgste flag should be inherited on fork, no?

[toc] | [next] | [standalone]

#1660754 — Re: [PATCH RFC 0/2] KVM: s390: avoid having to enable vm.alloc_pgste

FromMartin Schwidefsky <schwidefsky@de.ibm.com>
Date2017-06-08 07:40 +0200
SubjectRe: [PATCH RFC 0/2] KVM: s390: avoid having to enable vm.alloc_pgste
Message-ID<tPYkO-6y-13@gated-at.bofh.it>
In reply to#1660199
On Wed, 7 Jun 2017 22:47:56 +0200
Heiko Carstens <heiko.carstens@de.ibm.com> wrote:

> On Wed, Jun 07, 2017 at 02:34:40PM +0200, Martin Schwidefsky wrote:
> > +#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;							\
> > +})  
> 
> I'm wondering if this should simply fail, if a PT_S390_REQUEST_PGSTE type
> segment exists, but it is not ELFCLASS64?
> It will fail later anyway on s390_enable_sie(), but...

Does it matter if it fails for a 32-bit ELF file? Just makes the code more
complex without benefit, no?

> > 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);  
> 
> I think the alloc_pgste flag should be inherited on fork, no?

Yes, that makes it more consistent. I'll add it.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

[toc] | [prev] | [next] | [standalone]

#1660784

FromHeiko Carstens <heiko.carstens@de.ibm.com>
Date2017-06-08 08:30 +0200
Message-ID<tPZ7b-BL-3@gated-at.bofh.it>
In reply to#1660754
On Thu, Jun 08, 2017 at 07:35:28AM +0200, Martin Schwidefsky wrote:
> On Wed, 7 Jun 2017 22:47:56 +0200
> Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> > On Wed, Jun 07, 2017 at 02:34:40PM +0200, Martin Schwidefsky wrote:
> > > +#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;							\
> > > +})  
> > 
> > I'm wondering if this should simply fail, if a PT_S390_REQUEST_PGSTE type
> > segment exists, but it is not ELFCLASS64?
> > It will fail later anyway on s390_enable_sie(), but...
> 
> Does it matter if it fails for a 32-bit ELF file? Just makes the code more
> complex without benefit, no?

It would be more consistent, since right now a 32-bit ELF file with
PT_S390_REQUEST_PGSTE will be exectuted, but the page tables won't have any
pgstes. That's sort of odd, isn't it? And that later on it won't be able to
create a virtual machine because our current implementation doesn't allow
that for compat tasks is sort of unrelated.
But anyway, I'll leave that up to you, it doesn't really matter.

> 
> > > 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);  
> > 
> > I think the alloc_pgste flag should be inherited on fork, no?
> 
> Yes, that makes it more consistent. I'll add it.

By the way, what prevents with the _current_ code a scenario like:

- set allocate_pgste sysctl to 1
- create kvm guest
- s390_enable_sie
- run vcpu
- set allocate_pgste sysctl to 0
- clone(... CLONE_FILES ...) (that is: new mm without pgstes, but shared fds)
- [child] run vcpu

Is there anything that makes sure we cannot execute the sie instruction in
the child process?

[toc] | [prev] | [next] | [standalone]

#1661069 — Re: [PATCH RFC 0/2] KVM: s390: avoid having to enable vm.alloc_pgste

FromMartin Schwidefsky <schwidefsky@de.ibm.com>
Date2017-06-08 13:30 +0200
SubjectRe: [PATCH RFC 0/2] KVM: s390: avoid having to enable vm.alloc_pgste
Message-ID<tQ3Nw-3Dr-13@gated-at.bofh.it>
In reply to#1660784
On Thu, 8 Jun 2017 08:25:31 +0200
Heiko Carstens <heiko.carstens@de.ibm.com> wrote:

> On Thu, Jun 08, 2017 at 07:35:28AM +0200, Martin Schwidefsky wrote:
> > On Wed, 7 Jun 2017 22:47:56 +0200
> > Heiko Carstens <heiko.carstens@de.ibm.com> wrote:  
> > > On Wed, Jun 07, 2017 at 02:34:40PM +0200, Martin Schwidefsky wrote:  
> > > > +#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;							\
> > > > +})    
> > > 
> > > I'm wondering if this should simply fail, if a PT_S390_REQUEST_PGSTE type
> > > segment exists, but it is not ELFCLASS64?
> > > It will fail later anyway on s390_enable_sie(), but...  
> > 
> > Does it matter if it fails for a 32-bit ELF file? Just makes the code more
> > complex without benefit, no?  
> 
> It would be more consistent, since right now a 32-bit ELF file with
> PT_S390_REQUEST_PGSTE will be exectuted, but the page tables won't have any
> pgstes. That's sort of odd, isn't it? And that later on it won't be able to
> create a virtual machine because our current implementation doesn't allow
> that for compat tasks is sort of unrelated.
> But anyway, I'll leave that up to you, it doesn't really matter.

Actually the code will be less complex if we add PT_S390_REQUEST_PGSTE for
32-bit ELF files as well. It does not make sense to define the segment for
a compat process as KVM won't work but you get what you ask for..

This looks like this:

#define arch_elf_pt_proc(ehdr, phdr, elf, interp, state)        \
({                                                              \
        int _rc = 0;                                            \
        if (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;                                                    \
})

phdr is a (struct elf_phd *) which is either define to a a (struct elf64_phdr *)
or a (struct elf32_phdr *). The check works in both cases.
 
> >   
> > > > 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);    
> > > 
> > > I think the alloc_pgste flag should be inherited on fork, no?  
> > 
> > Yes, that makes it more consistent. I'll add it.  
> 
> By the way, what prevents with the _current_ code a scenario like:
> 
> - set allocate_pgste sysctl to 1
> - create kvm guest
> - s390_enable_sie
> - run vcpu
> - set allocate_pgste sysctl to 0
> - clone(... CLONE_FILES ...) (that is: new mm without pgstes, but shared fds)
> - [child] run vcpu
> 
> Is there anything that makes sure we cannot execute the sie instruction in
> the child process?

Yes, that looks like a loop-hole.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

[toc] | [prev] | [next] | [standalone]

#1661165

FromHeiko Carstens <heiko.carstens@de.ibm.com>
Date2017-06-08 15:20 +0200
Message-ID<tQ5vY-4Kf-15@gated-at.bofh.it>
In reply to#1661069
On Thu, Jun 08, 2017 at 01:24:01PM +0200, Martin Schwidefsky wrote:
> On Thu, 8 Jun 2017 08:25:31 +0200
> Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> > It would be more consistent, since right now a 32-bit ELF file with
> > PT_S390_REQUEST_PGSTE will be exectuted, but the page tables won't have any
> > pgstes. That's sort of odd, isn't it? And that later on it won't be able to
> > create a virtual machine because our current implementation doesn't allow
> > that for compat tasks is sort of unrelated.
> > But anyway, I'll leave that up to you, it doesn't really matter.
> 
> Actually the code will be less complex if we add PT_S390_REQUEST_PGSTE for
> 32-bit ELF files as well. It does not make sense to define the segment for
> a compat process as KVM won't work but you get what you ask for..
> 
> This looks like this:
> 
> #define arch_elf_pt_proc(ehdr, phdr, elf, interp, state)        \
> ({                                                              \
>         int _rc = 0;                                            \
>         if (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;                                                    \
> })
> 
> phdr is a (struct elf_phd *) which is either define to a a (struct elf64_phdr *)
> or a (struct elf32_phdr *). The check works in both cases.

Yes, that makes sense.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web