On 13.07.2018 13:58, David Hildenbrand wrote:
> On 13.07.2018 12:09, Janosch Frank wrote:
>> On 13.07.2018 10:53, David Hildenbrand wrote:
>>> On 13.07.2018 08:36, Janosch Frank wrote:
>>>> Similarly to the pte skey handling, where we set the storage key to
>>>> the default key for each newly mapped pte, we have to also do that for
>>>> huge pmds.
>>>>
>>>> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
>>>> ---
>>>> arch/s390/mm/pgtable.c | 26 ++++++++++++++++++++++++--
>>>> 1 file changed, 24 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/s390/mm/pgtable.c b/arch/s390/mm/pgtable.c
>>>> index c393a6b0f362..6550ed56fcdf 100644
>>>> --- a/arch/s390/mm/pgtable.c
>>>> +++ b/arch/s390/mm/pgtable.c
>>>> @@ -410,14 +410,34 @@ static inline pmd_t pmdp_flush_lazy(struct mm_struct *mm,
>>>> return old;
>>>> }
>>>>
>>>> +static void pmdp_clear_skeys(struct mm_struct *mm, pmd_t *pmdp, pmd_t new)
>>>> +{
>>>> + unsigned long paddr = pmd_val(new) & HPAGE_MASK;
>>>> +
>>>> + /*
>>>> + * After a guest has used the first storage key instruction,
>>>> + * we must ensure, that each newly mapped huge pmd has all
>>>> + * skeys cleared before mapping it.
>>>> + */
>>>> + if (!mm_uses_skeys(mm) ||
>>>> + !pmd_none(*pmdp) ||
>>>> + !pmd_large(new) ||
>>>> + (pmd_val(new) & _SEGMENT_ENTRY_INVALID))
>>>> + return;
>>>> +
>>>> + __storage_key_init_range(paddr, paddr + HPAGE_SIZE - 1);
>>>
>>> I am by far not an expert on this :)
>>>
>>> I wonder if it could happen that we zero-over already used keys (e.g.
>>> changing protection of a pmd entry ?). I hope Martin can review this one.
>>
>> Thats why the old entry has to be pmd_none.
>> But yes, I already pre-warned Martin.
>
> So this should also hold for THP I assume? (replacing page tables by PMD
> and the other way around).
It might not catch a merge of a page table where not all ptes are set to
!pte_none but that depends on the thp implementation..
>
>>
>>>
>>>> +}
>>>> +
>>>> pmd_t pmdp_xchg_direct(struct mm_struct *mm, unsigned long addr,
>>>> pmd_t *pmdp, pmd_t new)
>>>> {
>>>> pmd_t old;
>>>>
>>>> preempt_disable();
>>>> - if (mm_has_pgste(mm))
>>>> + if (mm_has_pgste(mm)) {
>>>> + pmdp_clear_skeys(mm, pmdp, new);
>>>> pmdp_notify(mm, addr);
>>>
>>> I wonder if mm_has_pgste(mm) is the right to check for here. AFAICS,
>>> s390_enable_skey() does not really rely on PGSTE to be around
>>> ("theoretically" in contrast to pgste_set_key()).
>>>
>>> Shouldn't this rather be
>>
>> mm_has_pgste is unfortunately something of a combination of mm_has_guest
>> and mm_allocates_4k_tables. I already looked into making that clear, but
>> it's a tangled mess right now considering ucontrol, the TIF bit and the
>> sysctl.
>>
>> We have to test against the pgstes, as we absolutely do not want to take
>> the performance impact on non-guest systems (sysctl) or non-guest
>> processes (TIF).
>
> Agreed that that should be cleaned up in the future.
>
>
