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.
