Re: [RFC/PATCH v2 19/22] s390/mm: Split huge pages if granular protection is needed

[Janosch Frank <frankja@linux.vnet.ibm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4446 bytes --]

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@linux.vnet.ibm.com>
>>> ---
>>>  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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]