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).
>
>>
>>> +}
>>> +
>>> 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.
--
Thanks,
David / dhildenb
