On 10/25/19 10:04 AM, David Hildenbrand wrote:
> On 25.10.19 09:18, Janosch Frank wrote:
>> On 10/24/19 6:07 PM, David Hildenbrand wrote:
>>> On 24.10.19 13:40, Janosch Frank wrote:
>>>> KSM will not work on secure pages, because when the kernel reads a
>>>> secure page, it will be encrypted and hence no two pages will look the
>>>> same.
>>>>
>>>> Let's mark the guest pages as unmergeable when we transition to secure
>>>> mode.
>>>>
>>>> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
>>>> ---
>>>> arch/s390/include/asm/gmap.h | 1 +
>>>> arch/s390/kvm/kvm-s390.c | 6 ++++++
>>>> arch/s390/mm/gmap.c | 28 ++++++++++++++++++----------
>>>> 3 files changed, 25 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/arch/s390/include/asm/gmap.h b/arch/s390/include/asm/gmap.h
>>>> index 6efc0b501227..eab6a2ec3599 100644
>>>> --- a/arch/s390/include/asm/gmap.h
>>>> +++ b/arch/s390/include/asm/gmap.h
>>>> @@ -145,4 +145,5 @@ int gmap_mprotect_notify(struct gmap *, unsigned long start,
>>>>
>>>> void gmap_sync_dirty_log_pmd(struct gmap *gmap, unsigned long dirty_bitmap[4],
>>>> unsigned long gaddr, unsigned long vmaddr);
>>>> +int gmap_mark_unmergeable(void);
>>>> #endif /* _ASM_S390_GMAP_H */
>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>> index 924132d92782..d1ba12f857e7 100644
>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>> @@ -2176,6 +2176,12 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
>>>> if (r)
>>>> break;
>>>>
>>>> + down_write(¤t->mm->mmap_sem);
>>>> + r = gmap_mark_unmergeable();
>>>> + up_write(¤t->mm->mmap_sem);
>>>> + if (r)
>>>> + break;
>>>> +
>>>> mutex_lock(&kvm->lock);
>>>> kvm_s390_vcpu_block_all(kvm);
>>>> /* FMT 4 SIE needs esca */
>>>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>>>> index edcdca97e85e..bf365a09f900 100644
>>>> --- a/arch/s390/mm/gmap.c
>>>> +++ b/arch/s390/mm/gmap.c
>>>> @@ -2548,6 +2548,23 @@ int s390_enable_sie(void)
>>>> }
>>>> EXPORT_SYMBOL_GPL(s390_enable_sie);
>>>>
>>>> +int gmap_mark_unmergeable(void)
>>>> +{
>>>> + struct mm_struct *mm = current->mm;
>>>> + struct vm_area_struct *vma;
>>>> +
>>>> + for (vma = mm->mmap; vma; vma = vma->vm_next) {
>>>> + if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
>>>> + MADV_UNMERGEABLE, &vma->vm_flags)) {
>>>> + mm->context.uses_skeys = 0;
>>>
>>> That skey setting does not make too much sense when coming via
>>> kvm_s390_handle_pv(). handle that in the caller?
>>
>> Hmm, I think the name of that variable is just plain wrong.
>> It should be "can_use_skeys" or "uses_unmergeable" (which would fit
>> better into the mm context anyway) and then we could add a
>> kvm->arch.uses_skeys to tell that we actually used them for migration
>> checks, etc..
>>
>> I had long discussions with Martin over these variable names a long time
>> ago..
>
> uses_skeys is set during s390_enable_skey(). that is used when we
>
> a) Call an skey instruction
> b) Migrate skeys
>
> So it should match "uses" or what am I missing?
>
> If you look at the users of "mm_uses_skeys(mm)" I think
> "uses_unmergeable" would actually be misleading. (e.g.,
> pgste_set_key()). it really means "somebody used skeys". The unmergable
> is just a required side effect.
Hmm, we couldn't check struct kvm from pgtable.c anyway.
Oh well, I still don't like it very much but your arguments are better
:-) Let's fix this.
Attachment:
signature.asc
Description: OpenPGP digital signature
