On 25.01.2018 15:39, David Hildenbrand wrote:
> On 25.01.2018 08:16, Janosch Frank wrote:
>> On 13.12.2017 13:53, Janosch Frank wrote:
>>> A guest can put DAT tables for a lower level guest in the same huge
>>> segment as one of its prefixes or a g3 page. This would make it
>>> necessary for the segment to be unprotected (because of the prefix)
>>> and protected (because of the shadowing) at the same time. This is not
>>> possible in this universe.
>>>
>>> Hence we split the affected huge segment, so we can protect on a
>>> per-page basis. Such gmap segments are special and get a new software
>>> bit, that helps us handling this edge case.
>>>
>>> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxxxxxxx>
>>> ---
>>> arch/s390/include/asm/gmap.h | 13 ++
>>> arch/s390/include/asm/pgtable.h | 7 +-
>>> arch/s390/mm/fault.c | 10 +-
>>> arch/s390/mm/gmap.c | 256 ++++++++++++++++++++++++++++++++++++----
>>> arch/s390/mm/pgtable.c | 51 ++++++++
>>> 5 files changed, 313 insertions(+), 24 deletions(-)
>>
>>> @@ -1081,20 +1189,27 @@ static int gmap_protect_range(struct gmap *gmap, unsigned long gaddr,
>>> spinlock_t *ptl;
>>> unsigned long vmaddr, dist;
>>> pmd_t *pmdp, *hpmdp;
>>> - int rc;
>>> + int rc = 0;
>>>
>>> while (len) {
>>> rc = -EAGAIN;
>>> vmaddr = __gmap_translate(gmap, gaddr);
>>> hpmdp = (pmd_t *)huge_pte_offset(gmap->mm, vmaddr, HPAGE_SIZE);
>>> + if (!hpmdp)
>>> + BUG();
>>> /* Do we need tests here? */
>>> ptl = pmd_lock(gmap->mm, hpmdp);
>>>
>>> pmdp = gmap_pmd_op_walk(gmap, gaddr);
>>> if (pmdp) {
>>> if (!pmd_large(*pmdp)) {
>>> - rc = gmap_protect_pte(gmap, gaddr, pmdp, prot,
>>> - bits);
>>> + if (gmap_pmd_is_split(pmdp) &&
>>> + (bits & GMAP_NOTIFY_MPROT)) {
>>> + pmd_val(*pmdp) |= _SEGMENT_ENTRY_GMAP_IN;
>>> + }
>>
>> @David:
>> This currently breaks my brain. There *was* a reason why I put this
>> there and I was quite insistent that we needed it. Something about
>> notification areas on splits, but I absolutely can't remember it. Sigh,
>> should've made a comment.
>>
>> This might be a leftover from earlier versions, but could also keep us
>> from doing mprot notification on pte's.
>>
>
> This is indeed confusing.
>
> Somebody wants to protect are certain memory (e.g. PREFIX) and get
> notified on changes to this subpart.
>
> We have a split pmd
> -> we have huge pages in our user process tables
> -> we have 4k pages in our GMAP tables
>
> Now, we protect a subpart of this huge page via PTEs. My assumption is,
> that your "mirroring code" might reduce access rights to the PMD in the
> user process tables.
Yes, it does at the end of gmap_protect_pte
>
> So e.g. via user space tables -> 1MB write protected
> Via GMAP: only 8K write protected
Yes and if userspace touches the pmd, we end up in pmdp_notify which
will call notify split and do pte notification.
>
>
> In addition, you are setting the _SEGMENT_ENTRY_GMAP_IN on the GMAP PMD.
> This means, that the pmdp_notify_gmap() calls all notifiers
> (gmap_call_notifier) in case the whole PMD is changed (e.g. by write
> protecting for migration)
>
> So if we get a gmap_pmdp_xchg(), we would see _SEGMENT_ENTRY_GMAP_IN and
> trigger a notification. The PTEs remain unchecked (bad!).
Split pmds have own handling functions for setting and clearing RO which
work on pte bases.
ptep_remove_dirty_protection_split
test_and_clear_guest_dirty_split
ptep_notify_gmap
>
>
> Now, If I understand this correctly, what you would have to do:
>
> gmap_pmdp_xchg() (or rather pmdp_notify_gmap()) has to check whether it
> is a real huge page or a split pmd. If split, it has to call the PTE
> invalidators of the page table accordingly.
If I didn't just confuse myself completely, we don't need that. We never
call xchg on a split pmd, except for setting up the split and
pmdp_notify_gmap is never called on a split pmd either as it's not a pmd
anymore (from the gmap sight, for mm we do ptep_notify_gmap_split).
>
> This makes this manual hack unnecessary.
>
> Hope this helps :)
A tiny bit.
Anyway, the change seems to be stable, I'll send the change as a reply
when I come from the next meeting. Also had no postcopy problems.
When we agree on the change I'll do a new version.
Attachment:
signature.asc
Description: OpenPGP digital signature
