On Mon, Mar 27, 2017 at 02:31:44PM +0100, Punit Agrawal wrote:
> Christoffer Dall <cdall@xxxxxxxxxx> writes:
>
> > On Mon, Mar 27, 2017 at 01:00:56PM +0100, James Morse wrote:
> >> Hi guys,
> >>
> >> On 27/03/17 12:20, Punit Agrawal wrote:
> >> > Christoffer Dall <cdall@xxxxxxxxxx> writes:
> >> >> On Wed, Mar 15, 2017 at 04:07:27PM +0000, James Morse wrote:
> >> >>> Once we enable ARCH_SUPPORTS_MEMORY_FAILURE on arm64[0], notifications for
> >> >>> broken memory can call memory_failure() in mm/memory-failure.c to deliver
> >> >>> SIGBUS to any user space process using the page, and notify all the
> >> >>> in-kernel users.
> >> >>>
> >> >>> If the page corresponded with guest memory, KVM will unmap this page
> >> >>> from its stage2 page tables. The user space process that allocated
> >> >>> this memory may have never touched this page in which case it may not
> >> >>> be mapped meaning SIGBUS won't be delivered.
> >> >>>
> >> >>> When this happens KVM discovers pfn == KVM_PFN_ERR_HWPOISON when it
> >> >>> comes to process the stage2 fault.
> >> >>>
> >> >>> Do as x86 does, and deliver the SIGBUS when we discover
> >> >>> KVM_PFN_ERR_HWPOISON. Use the stage2 mapping size as the si_addr_lsb
> >> >>> as this matches the user space mapping size.
> >>
> >> >>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> >> >>> index 962616fd4ddd..9d1aa294e88f 100644
> >> >>> --- a/arch/arm/kvm/mmu.c
> >> >>> +++ b/arch/arm/kvm/mmu.c
> >> >>> @@ -20,8 +20,10 @@
> >> >>> #include <linux/kvm_host.h>
> >> >>> #include <linux/io.h>
> >> >>> #include <linux/hugetlb.h>
> >> >>> +#include <linux/sched/signal.h>
> >> >>> #include <trace/events/kvm.h>
> >> >>> #include <asm/pgalloc.h>
> >> >>> +#include <asm/siginfo.h>
> >> >>> #include <asm/cacheflush.h>
> >> >>> #include <asm/kvm_arm.h>
> >> >>> #include <asm/kvm_mmu.h>
> >> >>> @@ -1237,6 +1239,23 @@ static void coherent_cache_guest_page(struct kvm_vcpu *vcpu, kvm_pfn_t pfn,
> >> >>> __coherent_cache_guest_page(vcpu, pfn, size);
> >> >>> }
> >> >>>
> >> >>> +static void kvm_send_hwpoison_signal(unsigned long address, bool hugetlb)
> >> >>> +{
> >> >>> + siginfo_t info;
> >> >>> +
> >> >>> + info.si_signo = SIGBUS;
> >> >>> + info.si_errno = 0;
> >> >>> + info.si_code = BUS_MCEERR_AR;
> >> >>> + info.si_addr = (void __user *)address;
> >> >>> +
> >> >>> + if (hugetlb)
> >> >>> + info.si_addr_lsb = PMD_SHIFT;
> >> >>> + else
> >> >>> + info.si_addr_lsb = PAGE_SHIFT;
> >> >>> +
> >> >>> + send_sig_info(SIGBUS, &info, current);
> >> >>> +}
> >> >>> +
> >> >>> static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> >> >>> struct kvm_memory_slot *memslot, unsigned long hva,
> >> >>> unsigned long fault_status)
> >> >>> @@ -1306,6 +1325,10 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> >> >>> smp_rmb();
> >> >>>
> >> >>> pfn = gfn_to_pfn_prot(kvm, gfn, write_fault, &writable);
> >> >>> + if (pfn == KVM_PFN_ERR_HWPOISON) {
> >> >>> + kvm_send_hwpoison_signal(hva, hugetlb);
> >> >>
> >> >> The way this is called means that we'll only notify userspace of a huge
> >> >> mapping if userspace is mapping hugetlbfs, and not because the stage2
> >> >> mapping may or may not have used transparent huge pages when the error
> >> >> was discovered. Is this the desired semantics?
> >>
> >> No,
> >>
> >>
> >> > I think so.
> >> >
> >> > AFAIUI, transparent hugepages are split before being poisoned while all
> >> > the underlying pages of a hugepage are poisoned together, i.e., no
> >> > splitting.
> >>
> >> In which case I need to look into this some more!
> >>
> >> My thinking was we should report the size that was knocked out of the stage2 to
> >> avoid the guest repeatedly faulting until it has touched every guest-page-size
> >> in the stage2 hole.
> >
> > By signaling something at the fault path, I think it's going to be very
> > hard to backtrack how the stage 2 page tables looked like when faults
> > started happening, because I think these are completely decoupled events
> > (the mmu notifier and the later fault).
> >
> >>
> >> Reading the code in that kvm/mmu.c it looked like the mapping sizes would always
> >> be the same as those used by userspace.
> >
> > I think the mapping sizes should be the same between userspace and KVM,
> > but the mapping size of a particular page (and associated pages) may
> > vary over time.
>
> Stage 1 and Stage 2 support different hugepage sizes. A larger size
> stage 1 page maps to multiple stage 2 page table entries. For stage 1,
> we support PUD_SIZE, CONT_PMD_SIZE, PMD_SIZE and CONT_PTE_SIZE while
> only PMD_SIZE is supported for Stage 2.
>
> >
> >>
> >> If the page was split before KVM could have taken this fault I assumed it would
> >> fault on the page-size mapping and hugetlb would be false.
> >
> > I think you could have a huge page, which gets unmapped as a result on
> > it getting split (perhaps because there was a failure on one page) and
> > later as you fault, you can discover a range which can be a hugetlbfs or
> > transparent huge pages.
> >
> > The question that I don't know is how Linux behaves if a page is marked
> > with hwpoison, in that case, if Linux never supports THP and always
> > marks an entire huge page in a hugetlbfs with the poison, then I think
> > we're mostly good here. If not, we should make sure we align with
> > whatever the rest of the kernel does.
>
> AFAICT, a hugetlbfs page is poisoned as a whole while thp is split
> before poisoning. Quoting comment near the top of memory_failure() in
> mm/memory_failure.c.
>
> /*
> * Currently errors on hugetlbfs pages are measured in hugepage units,
> * so nr_pages should be 1 << compound_order. OTOH when errors are on
> * transparent hugepages, they are supposed to be split and error
> * measurement is done in normal page units. So nr_pages should be one
> * in this case.
> */
>
> >
> >> (which is already
> >> wrong for another reason, looks like I grabbed the variable before
> >> transparent_hugepage_adjust() has had a go a it.).
> >>
> >
> > yes, which is why I asked if you only care about hugetlbfs.
> >
>
> Based on the comment above, we should never get a poisoned page that is
> part of a transparent hugepage.
>
> >>
> >> >> Also notice that the hva is not necessarily aligned to the beginning of
> >> >> the huge page, so can we be giving userspace wrong information by
> >> >> pointing in the middle of a huge page and telling it there was an
> >> >> address error in the size of the PMD ?
> >> >>
> >> >
> >> > I could be reading it wrong but I think we are fine here - the address
> >> > (hva) is the location that faulted. And the lsb indicates the least
> >> > significant bit of the faulting address (See man sigaction(2)). The
> >> > receiver of the signal is expected to use the address and lsb to workout
> >> > the extent of corruption.
> >>
> >> kill_proc() in mm/memory-failure.c does this too, but the address is set by
> >> page_address_in_vma() in add_to_kill() of the same file. (I'll chat with Punit
> >> off list.)
> >>
> >>
> >> > Though I missed a subtlety while reviewing the patch before. The
> >> > reported lsb should be for the userspace hugepage mapping (i.e., hva)
> >> > and not for the stage 2.
> >>
> >> I thought these were always supposed to be the same, and using hugetlb was a bug
> >> because I didn't look closely enough at what is_vm_hugetlb_page() does.
>
> See above.
>
> >>
> >>
> >> > In light of this, I'd like to retract my Reviewed-by tag for this
> >> > version of the patch as I believe we'll need to change the lsb
> >> > reporting.
> >>
> >> Sure, lets work out what this should be doing. I'm beginning to suspect x86's
> >> 'always page size' was correct to begin with!
> >>
> >
> > I had a sense of that too, but it would be good to understand how you
> > mark and individual page within a hugetlbfs huge page with hwpoison...
>
> I don't think it is possible to mark an individual page in a hugetlbfs
> page - it's all or nothing.
>
> AFAICT, the SIGBUS report is for user mappings and doesn't have to care
> whether it's Stage 2 hugetlb page or thp. And the lsb determination should
> take the Stage 1 hugepage size into account - something along the lines
> of the snippet from previous email.
>
I think the lsb should indicate the size of the memory region known to
be broken by the kernel - however that whole mechanism works.
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
