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.
> +}
> +
> 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
if (mm_uses_skeys(mm))
pmdp_clear_skeys(mm, pmdp, new);
(so you can remove the inner check). So you'd even have one check less.
> + }
> old = pmdp_flush_direct(mm, addr, pmdp);
> *pmdp = new;
> preempt_enable();
> @@ -431,8 +451,10 @@ pmd_t pmdp_xchg_lazy(struct mm_struct *mm, unsigned long addr,
> 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);
> + }
> old = pmdp_flush_lazy(mm, addr, pmdp);
> *pmdp = new;
> preempt_enable();
>
--
Thanks,
David / dhildenb
