On 12.07.2018 11:04, frankja wrote:
> On Thu, 12 Jul 2018 10:35:00 +0200
> David Hildenbrand <david@xxxxxxxxxx> wrote:
>
>> On 12.07.2018 10:09, frankja wrote:
>>> On Thu, 12 Jul 2018 09:23:01 +0200
>>> David Hildenbrand <david@xxxxxxxxxx> wrote:
>>>>
>>>>> #include <linux/kernel.h>
>>>>> @@ -589,8 +591,8 @@ int __gmap_link(struct gmap *gmap, unsigned
>>>>> long gaddr, unsigned long vmaddr) return -EFAULT;
>>>>> pmd = pmd_offset(pud, vmaddr);
>>>>> VM_BUG_ON(pmd_none(*pmd));
>>>>> - /* large pmds cannot yet be handled */
>>>>> - if (pmd_large(*pmd))
>>>>> + /* Are we allowed to use huge pages? */
>>>>> + if (pmd_large(*pmd)
>>>>> && !gmap->mm->context.uses_gmap_hpage) return -EFAULT;
>>>>> /* Link gmap segment table entry location to page table.
>>>>> */ rc = radix_tree_preload(GFP_KERNEL);
>>>>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>>>>> index b6270a3b38e9..b955b986b341 100644
>>>>> --- a/include/uapi/linux/kvm.h
>>>>> +++ b/include/uapi/linux/kvm.h
>>>>> @@ -949,6 +949,7 @@ struct kvm_ppc_resize_hpt {
>>>>> #define KVM_CAP_GET_MSR_FEATURES 153
>>>>> #define KVM_CAP_HYPERV_EVENTFD 154
>>>>> #define KVM_CAP_HYPERV_TLBFLUSH 155
>>>>> +#define KVM_CAP_S390_HPAGE_1M 156
>>>>>
>>>>> #ifdef KVM_CAP_IRQ_ROUTING
>>>>>
>>>>>
>>>>
>>>> I was wondering if we should add some safety net for gmap shadows
>>>> when hitting a huge pmd. Or when trying to create a shadow for a
>>>> gmap with huge pmds enabled (add a check in gmap_shadow()). So the
>>>> GMAP kernel interface remains consistent.
>>>
>>> For now we don't have the sief2 so we don't end up in gmap_shadow()
>>> anyway. Huge l3 in small l2 is currently supported via the fake
>>> tables.
>>>
>>> So what do you want to keep consistent?
>>
>> gmap is just an internal kernel interface and KVM is the only user
>> right now. The logic that gmap shadow does not work with hpage gmaps
>> is right now buried in KVM, not in the GMAP. That's why I am asking
>> if it makes sense to also have safety nets in the gmap shadow code.
>>
>> If you added the other safety net I suggested (PMD protection with
>> SHADOW notifier bit set -> -EINVAL), we are maybe already on the safe
>> side that if gmap_shadow() would be used. We would maybe fail in a
>> nice fashion. But I didn't check all call paths. Prohibiting
>> gmap_shadow() when huge pages are allowed in the GMAP right from the
>> start would be the big hammer.
>>
>
> I'll add a BUG_ON(parent->mm->context.allow_gmap_hpage_1m); it's
> removed later anyway...
>
> For now I'll also add the notifier warning for the gmap invalidation, it
> should only slow us down with thp, as hpages are otherwise never
> touched after fault-in. And it will be useful for thp testing in a few
> months.
>
Sounds good to me! Looking forward to the next version of this series :)
--
Thanks,
David / dhildenb
