Hi David,
it is nice to see that you are still working on s390 related topics.
On Mon, 29 May 2017 18:32:00 +0200
David Hildenbrand <[email protected]> wrote:
> Having to enable vm.alloc_pgste globally might not be the best solution.
> 4k page tables are created for all processes and running QEMU KVM guests
> is more complicated than it should be.
To run KVM guests you need to issue a single sysctl to set vm.allocate_pgste,
this is the best solution we found so far.
> 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.
> However, we
> might be able to let 2k and 4k page tables co-exist. We only need
> 4k page tables whenever we want to expose such memory to a guest. So
> turning on 4k page table allocation at one point and only allowing such
> memory to go into our gmap (guest mapping) might be a solution.
> User space tools like QEMU that create the VM before mmap-ing any memory
> that will belong to the guest can simply use the new VM type. Proper 4k
> page tables will be created for any memory mmap-ed afterwards. And these
> can be used in the gmap without problems. Existing user space tools
> will work as before - having to enable vm.alloc_pgste explicitly.
I can not say that I like this approach. Right now a process either uses
2K page tables or 4K page tables. With your patch it is basically per page
table page. Memory areas that existed before the switch to allocate
4K page tables can not be mapped to the guests gmap anymore. There might
be hidden pitfalls e.g. with guest migration.
> This should play fine with vSIE, as vSIE code works completely on the gmap.
> So if only page tables with pgste go into our gmap, we should be fine.
>
> Not sure if this breaks important concepts, has some serious performance
> problems or I am missing important cases. If so, I guess there is really
> no way to avoid setting vm.alloc_pgste.
>
> Possible modifications:
> - Enable this option via an ioctl (like KVM_S390_ENABLE_SIE) instead of
> a new VM type
> - Remember if we have mixed pgtables. If !mixed, we can make maybe faster
> decisions (if that is really a problem).
What I do not like in particular is this function:
static inline int pgtable_has_pgste(struct mm_struct *mm, unsigned long addr)
{
struct page *page;
if (!mm_has_pgste(mm))
return 0;
page = pfn_to_page(addr >> PAGE_SHIFT);
return atomic_read(&page->_mapcount) & 0x4U;
}
The check for pgstes got more complicated, it used to be a test-under-mask
of a bit in the mm struct and a branch. Now we have an additional pfn_to_page,
an atomic_read and a bit test. That is done multiple times for every ptep_xxx
operation.
Is the operational simplification of not having to set vm.allocate_pgste really
that important ?
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On 06/01/2017 12:46 PM, Martin Schwidefsky wrote:
> Hi David,
>
> it is nice to see that you are still working on s390 related topics.
>
> On Mon, 29 May 2017 18:32:00 +0200
> David Hildenbrand <[email protected]> wrote:
>
>> Having to enable vm.alloc_pgste globally might not be the best solution.
>> 4k page tables are created for all processes and running QEMU KVM guests
>> is more complicated than it should be.
>
> To run KVM guests you need to issue a single sysctl to set vm.allocate_pgste,
> this is the best solution we found so far.
Suse and Ubuntu seem to have a sysctl.conf file in the qemu-kvm package that
does a global switch.
>
>> 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.
>
>> However, we
>> might be able to let 2k and 4k page tables co-exist. We only need
>> 4k page tables whenever we want to expose such memory to a guest. So
>> turning on 4k page table allocation at one point and only allowing such
>> memory to go into our gmap (guest mapping) might be a solution.
>> User space tools like QEMU that create the VM before mmap-ing any memory
>> that will belong to the guest can simply use the new VM type. Proper 4k
>> page tables will be created for any memory mmap-ed afterwards. And these
>> can be used in the gmap without problems. Existing user space tools
>> will work as before - having to enable vm.alloc_pgste explicitly.
>
> I can not say that I like this approach. Right now a process either uses
> 2K page tables or 4K page tables. With your patch it is basically per page
> table page. Memory areas that existed before the switch to allocate
> 4K page tables can not be mapped to the guests gmap anymore. There might
> be hidden pitfalls e.g. with guest migration.
>
>> This should play fine with vSIE, as vSIE code works completely on the gmap.
>> So if only page tables with pgste go into our gmap, we should be fine.
>>
>> Not sure if this breaks important concepts, has some serious performance
>> problems or I am missing important cases. If so, I guess there is really
>> no way to avoid setting vm.alloc_pgste.
>>
>> Possible modifications:
>> - Enable this option via an ioctl (like KVM_S390_ENABLE_SIE) instead of
>> a new VM type
>> - Remember if we have mixed pgtables. If !mixed, we can make maybe faster
>> decisions (if that is really a problem).
>
> What I do not like in particular is this function:
>
> static inline int pgtable_has_pgste(struct mm_struct *mm, unsigned long addr)
> {
> struct page *page;
>
> if (!mm_has_pgste(mm))
> return 0;
>
> page = pfn_to_page(addr >> PAGE_SHIFT);
> return atomic_read(&page->_mapcount) & 0x4U;
> }
The good thing with this approach is that the first condition will make non-KVM
processes as fast as before. In fact, given the sysctl thing being present everywhere,
this patch might actually move non-KVM processes back to 2k page tables so it
improve those.
>
> The check for pgstes got more complicated, it used to be a test-under-mask
> of a bit in the mm struct and a branch. Now we have an additional pfn_to_page,
> an atomic_read and a bit test. That is done multiple times for every ptep_xxx
> operation.
>
> Is the operational simplification of not having to set vm.allocate_pgste really
> that important ?
>
Hi Martin,
thanks for having a look at this!
>> However, we
>> might be able to let 2k and 4k page tables co-exist. We only need
>> 4k page tables whenever we want to expose such memory to a guest. So
>> turning on 4k page table allocation at one point and only allowing such
>> memory to go into our gmap (guest mapping) might be a solution.
>> User space tools like QEMU that create the VM before mmap-ing any memory
>> that will belong to the guest can simply use the new VM type. Proper 4k
>> page tables will be created for any memory mmap-ed afterwards. And these
>> can be used in the gmap without problems. Existing user space tools
>> will work as before - having to enable vm.alloc_pgste explicitly.
>
> I can not say that I like this approach. Right now a process either uses
> 2K page tables or 4K page tables. With your patch it is basically per page
> table page. Memory areas that existed before the switch to allocate
> 4K page tables can not be mapped to the guests gmap anymore. There might
> be hidden pitfalls e.g. with guest migration.
As long as QEMU knows what it is doing, I don't see problems with
migration. Even for migration, all memory is mmaped afterwards. But yes,
we have to think about this carefully. (that's why QEMU explicitly has
to switch this behavior on, to not break user space that mmaps it
differently).
>
>> This should play fine with vSIE, as vSIE code works completely on the gmap.
>> So if only page tables with pgste go into our gmap, we should be fine.
>>
>> Not sure if this breaks important concepts, has some serious performance
>> problems or I am missing important cases. If so, I guess there is really
>> no way to avoid setting vm.alloc_pgste.
>>
>> Possible modifications:
>> - Enable this option via an ioctl (like KVM_S390_ENABLE_SIE) instead of
>> a new VM type
>> - Remember if we have mixed pgtables. If !mixed, we can make maybe faster
>> decisions (if that is really a problem).
>
> What I do not like in particular is this function:
>
> static inline int pgtable_has_pgste(struct mm_struct *mm, unsigned long addr)
> {
> struct page *page;
>
> if (!mm_has_pgste(mm))
> return 0;
>
> page = pfn_to_page(addr >> PAGE_SHIFT);
> return atomic_read(&page->_mapcount) & 0x4U;
> }
>
> The check for pgstes got more complicated, it used to be a test-under-mask
> of a bit in the mm struct and a branch. Now we have an additional pfn_to_page,
> an atomic_read and a bit test. That is done multiple times for every ptep_xxx
> operation.
The "heavy overhead" only applies to processes using KVM, ordinary
processes only have one bit test (just as before !mm_has_pgste(mm)).
Can you judge how much overhead this could be in practice? (esp: can it
be noticed?)
We could of course "tune" the functions to reduce the number of
pgtable_has_pgste() but we won't get rid of at least one such
pfn_to_page. And the question is if already one such test degrades
performance noticeably. Unfortunately the z/VM test system I have is not
really the best fit for performance tests.
>
> Is the operational simplification of not having to set vm.allocate_pgste really
> that important ?
>
The problem is that once we have a package that installs QEMU, and we
don't want to completely confuse the user (let him care about
vm.allocate_pgste), we have to set this automatically.
E.g. see https://bugzilla.redhat.com/show_bug.cgi?id=1290589
a) install a config file that will set it on every boot (like fedora)
b) set vm.allocate_pgste=1 right away, so QEMU can be used without a reboot
Now, this is possible, but as you know this can implicitly influence
other workload (double size of page tables for everything). Esp.
thinking about systems that are not pure KVM hosts. So by simply
installing the QEMU package, we change system behavior. Even if QEMU
will only be used sometimes on that host.
So actually what we want is : PGSTES only for processes that actually
need it and handle the details internally.
I don't really see too many ways to do this differently. The more we can
hide "internal specialties", the better. E.g. think about guests being
based on huge pages completely someday in the future: the concept of
pgstes does not apply here. Still, pgste will have to be enabled for the
complete system and there isn't too much we can do about it
(s390_enable_sie can't know that QEMU will only use huge pages, so we
would need yet another VM type in QEMU to handle guests that can only
map huge pages).
An alternative: Have some process that enables PGSTE for all of its
future children. Fork+execv qemu. However, such a process in between
will most likely confuse tooling like libvirt when it comes to process ids.
Can you think of any other way to handle this? Setting
vm.allocate_pgste=1 should really be avoided if possible.
Thanks a lot again!
--
Thanks,
David
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.
At least I also don't like the next "hack", that is specifically designed
to only work with how QEMU is currently implemented. It might break with
future QEMU changes or the next user space implementation that drives the
kvm interface, but is doing everything differently.
Let's look for a "clean" solution that will always work. We had too many
hacks for this problem and *all* of them were broken.
On Thu, Jun 01, 2017 at 01:27:28PM +0200, David Hildenbrand wrote:
> An alternative: Have some process that enables PGSTE for all of its
> future children. Fork+execv qemu. However, such a process in between
> will most likely confuse tooling like libvirt when it comes to process ids.
That would be something like sys_personality() vs setarch, e.g. adding a
new s390 specific personality flag and then do something like
setarch s390x --kvm qemu...
However I'm afraid that this is also not acceptable from a usability point
of view.
On 06/02/2017 09:02 AM, 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.
>
> At least I also don't like the next "hack", that is specifically designed
> to only work with how QEMU is currently implemented. It might break with
> future QEMU changes or the next user space implementation that drives the
> kvm interface, but is doing everything differently.
> Let's look for a "clean" solution that will always work. We had too many
> hacks for this problem and *all* of them were broken.
The more I think about it, dropping 2k page tables and always allocate a full
page would simplify pgalloc. As far I can see this would also get rid of
the &mm->context.pgtable_lock.
On Fri, 2 Jun 2017 09:13:03 +0200
Christian Borntraeger <[email protected]> wrote:
> On 06/02/2017 09:02 AM, 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.
> >
> > At least I also don't like the next "hack", that is specifically designed
> > to only work with how QEMU is currently implemented. It might break with
> > future QEMU changes or the next user space implementation that drives the
> > kvm interface, but is doing everything differently.
> > Let's look for a "clean" solution that will always work. We had too many
> > hacks for this problem and *all* of them were broken.
>
>
> The more I think about it, dropping 2k page tables and always allocate a full
> page would simplify pgalloc. As far I can see this would also get rid of
> the &mm->context.pgtable_lock.
And it would waste twice the amount of memory for page tables. NAK.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On 06/02/2017 09:16 AM, Martin Schwidefsky wrote:
> On Fri, 2 Jun 2017 09:13:03 +0200
> Christian Borntraeger <[email protected]> wrote:
>
>> On 06/02/2017 09:02 AM, 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.
>>>
>>> At least I also don't like the next "hack", that is specifically designed
>>> to only work with how QEMU is currently implemented. It might break with
>>> future QEMU changes or the next user space implementation that drives the
>>> kvm interface, but is doing everything differently.
>>> Let's look for a "clean" solution that will always work. We had too many
>>> hacks for this problem and *all* of them were broken.
>>
>>
>> The more I think about it, dropping 2k page tables and always allocate a full
>> page would simplify pgalloc. As far I can see this would also get rid of
>> the &mm->context.pgtable_lock.
>
> And it would waste twice the amount of memory for page tables. NAK.
Yes and we spend the same amount of memory TODAY, because every distro on the
planet that uses KVM has sysctl.allocate_pgste set.
On 06/02/2017 09:18 AM, Christian Borntraeger wrote:
> On 06/02/2017 09:16 AM, Martin Schwidefsky wrote:
>> On Fri, 2 Jun 2017 09:13:03 +0200
>> Christian Borntraeger <[email protected]> wrote:
>>
>>> On 06/02/2017 09:02 AM, 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.
>>>>
>>>> At least I also don't like the next "hack", that is specifically designed
>>>> to only work with how QEMU is currently implemented. It might break with
>>>> future QEMU changes or the next user space implementation that drives the
>>>> kvm interface, but is doing everything differently.
>>>> Let's look for a "clean" solution that will always work. We had too many
>>>> hacks for this problem and *all* of them were broken.
>>>
>>>
>>> The more I think about it, dropping 2k page tables and always allocate a full
>>> page would simplify pgalloc. As far I can see this would also get rid of
>>> the &mm->context.pgtable_lock.
>>
>> And it would waste twice the amount of memory for page tables. NAK.
>
> Yes and we spend the same amount of memory TODAY, because every distro on the
> planet that uses KVM has sysctl.allocate_pgste set.
Maybe todays approach might be still the best. (if qemu is installed, its all
4k, if not its all 2k)
On Fri, 2 Jun 2017 09:25:54 +0200
Christian Borntraeger <[email protected]> wrote:
> On 06/02/2017 09:18 AM, Christian Borntraeger wrote:
> > On 06/02/2017 09:16 AM, Martin Schwidefsky wrote:
> >> On Fri, 2 Jun 2017 09:13:03 +0200
> >> Christian Borntraeger <[email protected]> wrote:
> >>
> >>> On 06/02/2017 09:02 AM, 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.
> >>>>
> >>>> At least I also don't like the next "hack", that is specifically designed
> >>>> to only work with how QEMU is currently implemented. It might break with
> >>>> future QEMU changes or the next user space implementation that drives the
> >>>> kvm interface, but is doing everything differently.
> >>>> Let's look for a "clean" solution that will always work. We had too many
> >>>> hacks for this problem and *all* of them were broken.
> >>>
> >>>
> >>> The more I think about it, dropping 2k page tables and always allocate a full
> >>> page would simplify pgalloc. As far I can see this would also get rid of
> >>> the &mm->context.pgtable_lock.
> >>
> >> And it would waste twice the amount of memory for page tables. NAK.
> >
> > Yes and we spend the same amount of memory TODAY, because every distro on the
> > planet that uses KVM has sysctl.allocate_pgste set.
>
> Maybe todays approach might be still the best. (if qemu is installed, its all
> 4k, if not its all 2k)
Exactly-
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Fri, 2 Jun 2017 09:02:10 +0200
Heiko Carstens <[email protected]> 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:
--
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..79911231f9e6 100644
--- a/arch/s390/include/asm/elf.h
+++ b/arch/s390/include/asm/elf.h
@@ -151,6 +151,28 @@ 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_elf_pt_proc(ehdr, phdr, elf, interp, state) (0)
+#define arch_check_elf(ehdr, interp, interp_ehdr, state) \
+({ \
+ struct elf64_hdr *hdr = (void*) ehdr; \
+ int _rc = 0; \
+ if (hdr->e_ident[EI_CLASS] == ELFCLASS64 && \
+ (hdr->e_flags & 0x00000002) && \
+ !page_table_allocate_pgste && \
+ !current->mm->context.alloc_pgste) { \
+ current->mm->context.alloc_pgste = 1; \
+ 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..268a5d22ce1b 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 ||
+ current->mm->context.alloc_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/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
#
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On 06/02/2017 11:46 AM, Martin Schwidefsky wrote:
> On Fri, 2 Jun 2017 09:02:10 +0200
> Heiko Carstens <[email protected]> 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.
> --
> 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..79911231f9e6 100644
> --- a/arch/s390/include/asm/elf.h
> +++ b/arch/s390/include/asm/elf.h
> @@ -151,6 +151,28 @@ 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_elf_pt_proc(ehdr, phdr, elf, interp, state) (0)
> +#define arch_check_elf(ehdr, interp, interp_ehdr, state) \
> +({ \
> + struct elf64_hdr *hdr = (void*) ehdr; \
> + int _rc = 0; \
> + if (hdr->e_ident[EI_CLASS] == ELFCLASS64 && \
> + (hdr->e_flags & 0x00000002) && \
shall we define some EF_S390_PGSTE flags in elf.h for this number?
What is 0x00000001 by the way?
On Fri, Jun 02, 2017 at 11:46:47AM +0200, Martin Schwidefsky wrote:
> On Fri, 2 Jun 2017 09:02:10 +0200
> Heiko Carstens <[email protected]> wrote:
> > Maybe this is a bit over-simplified, but might work.
> This is not over-simplified at all, that does work:
Good!
> +struct arch_elf_state {
> +};
> +
> +#define INIT_ARCH_ELF_STATE { }
> +
> +#define arch_elf_pt_proc(ehdr, phdr, elf, interp, state) (0)
> +#define arch_check_elf(ehdr, interp, interp_ehdr, state) \
> +({ \
> + struct elf64_hdr *hdr = (void*) ehdr; \
> + int _rc = 0; \
> + if (hdr->e_ident[EI_CLASS] == ELFCLASS64 && \
> + (hdr->e_flags & 0x00000002) && \
> + !page_table_allocate_pgste && \
> + !current->mm->context.alloc_pgste) { \
> + current->mm->context.alloc_pgste = 1; \
However, I think this is over-simplified, unless I'm mistaken.
If you set current->mm->context.alloc_pgste here, then that means that 4k
page tables will be freed when the original mm will be released, instead of
the correct 2k ones.
I think you need an additional intermediate context flag here. Something
like current->mm->context.request_pgste or whatever, no?
On Fri, 2 Jun 2017 12:28:48 +0200
Heiko Carstens <[email protected]> wrote:
> On Fri, Jun 02, 2017 at 11:46:47AM +0200, Martin Schwidefsky wrote:
> > On Fri, 2 Jun 2017 09:02:10 +0200
> > Heiko Carstens <[email protected]> wrote:
> > > Maybe this is a bit over-simplified, but might work.
> > This is not over-simplified at all, that does work:
>
> Good!
>
> > +struct arch_elf_state {
> > +};
> > +
> > +#define INIT_ARCH_ELF_STATE { }
> > +
> > +#define arch_elf_pt_proc(ehdr, phdr, elf, interp, state) (0)
> > +#define arch_check_elf(ehdr, interp, interp_ehdr, state) \
> > +({ \
> > + struct elf64_hdr *hdr = (void*) ehdr; \
> > + int _rc = 0; \
> > + if (hdr->e_ident[EI_CLASS] == ELFCLASS64 && \
> > + (hdr->e_flags & 0x00000002) && \
> > + !page_table_allocate_pgste && \
> > + !current->mm->context.alloc_pgste) { \
> > + current->mm->context.alloc_pgste = 1; \
>
> However, I think this is over-simplified, unless I'm mistaken.
>
> If you set current->mm->context.alloc_pgste here, then that means that 4k
> page tables will be freed when the original mm will be released, instead of
> the correct 2k ones.
>
> I think you need an additional intermediate context flag here. Something
> like current->mm->context.request_pgste or whatever, no?
Yes, the flags for the current mm and the next mm have to be different.
request_pgste is a nice name for the new flag, I'll use it.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Fri, 2 Jun 2017 12:19:19 +0200
Christian Borntraeger <[email protected]> wrote:
> On 06/02/2017 11:46 AM, Martin Schwidefsky wrote:
> > On Fri, 2 Jun 2017 09:02:10 +0200
> > Heiko Carstens <[email protected]> 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 <elf.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#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 <elf-file>\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;
}
> > --
> > 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..79911231f9e6 100644
> > --- a/arch/s390/include/asm/elf.h
> > +++ b/arch/s390/include/asm/elf.h
> > @@ -151,6 +151,28 @@ 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_elf_pt_proc(ehdr, phdr, elf, interp, state) (0)
> > +#define arch_check_elf(ehdr, interp, interp_ehdr, state) \
> > +({ \
> > + struct elf64_hdr *hdr = (void*) ehdr; \
> > + int _rc = 0; \
> > + if (hdr->e_ident[EI_CLASS] == ELFCLASS64 && \
> > + (hdr->e_flags & 0x00000002) && \
>
> shall we define some EF_S390_PGSTE flags in elf.h for this number?
> What is 0x00000001 by the way?
Yes, we have to give the bit a nice name, like EF_S390_REQUEST_PGSTE
0x00000001 is EF_S390_HIGH_GPRS
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On 02.06.2017 09:02, 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.
Cool, I thought that we would not be able to get around having to make a
decision at fork time, but this really looks promising.
Thanks for this idea Heiko!
--
Thanks,
David
On 06/02/2017 12:53 PM, Martin Schwidefsky wrote:
> On Fri, 2 Jun 2017 12:19:19 +0200
> Christian Borntraeger <[email protected]> wrote:
>
>> On 06/02/2017 11:46 AM, Martin Schwidefsky wrote:
>>> On Fri, 2 Jun 2017 09:02:10 +0200
>>> Heiko Carstens <[email protected]> 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 <elf.h>
> #include <fcntl.h>
> #include <stdio.h>
> #include <stdlib.h>
>
> #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 <elf-file>\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?
On Fri, 2 Jun 2017 15:20:33 +0200
Christian Borntraeger <[email protected]> wrote:
> On 06/02/2017 12:53 PM, Martin Schwidefsky wrote:
> > On Fri, 2 Jun 2017 12:19:19 +0200
> > Christian Borntraeger <[email protected]> wrote:
> >
> >> On 06/02/2017 11:46 AM, Martin Schwidefsky wrote:
> >>> On Fri, 2 Jun 2017 09:02:10 +0200
> >>> Heiko Carstens <[email protected]> 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 <elf.h>
> > #include <fcntl.h>
> > #include <stdio.h>
> > #include <stdlib.h>
> >
> > #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 <elf-file>\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 <[email protected]>
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 <[email protected]>
---
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.
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?
On Wed, 7 Jun 2017 22:47:56 +0200
Heiko Carstens <[email protected]> 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.
On Thu, Jun 08, 2017 at 07:35:28AM +0200, Martin Schwidefsky wrote:
> On Wed, 7 Jun 2017 22:47:56 +0200
> Heiko Carstens <[email protected]> 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?
On Thu, 8 Jun 2017 08:25:31 +0200
Heiko Carstens <[email protected]> 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 <[email protected]> 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.
On Thu, Jun 08, 2017 at 01:24:01PM +0200, Martin Schwidefsky wrote:
> On Thu, 8 Jun 2017 08:25:31 +0200
> Heiko Carstens <[email protected]> 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.