On Wed, Jul 05, 2023 at 09:09:19AM -0700, James Houghton wrote:
diff --git a/include/linux/swapops.h b/include/linux/swapops.h index 4c932cb45e0b..8259fee32421 100644 --- a/include/linux/swapops.h +++ b/include/linux/swapops.h @@ -394,7 +394,8 @@ typedef unsigned long pte_marker;
#define PTE_MARKER_UFFD_WP BIT(0) #define PTE_MARKER_SWAPIN_ERROR BIT(1) -#define PTE_MARKER_MASK (BIT(2) - 1) +#define PTE_MARKER_UFFD_POISON BIT(2)
One more tab.
Though I remembered the last time we discussed IIRC we plan to rename SWAPIN_ERROR and reuse it, could you explain why a new bit is still needed?
I think I commented this but I'll do it again: IIUC any existing host swapin errors for guest pages should be reported as MCE too, afaict, happened in kvm context.
I think swapin errors are treated differently than poison. Swapin errors get VM_FAULT_SIGBUS, and poison gets VM_FAULT_HWPOISON, so UFFDIO_POISON should also get VM_FAULT_HWPOISON (so that's what Axel has implemented). And I think that needs a separate PTE marker.
My question was, should we also make SWAPIN_ERROR return VM_FAULT_HWPOISON always?
Just to recap from what I already commented above - if a guest page got error in swapin due to block sector failures, it should be treated as VM_FAULT_HWPOISON too, IMHO. IOW, I think current SWAPIN_ERROR is wrong when in kvm context and we should fix it first.
+#define PTE_MARKER_MASK (BIT(3) - 1)
static inline swp_entry_t make_pte_marker_entry(pte_marker marker) { diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h index ac7b0c96d351..ac8c6854097c 100644 --- a/include/linux/userfaultfd_k.h +++ b/include/linux/userfaultfd_k.h @@ -46,6 +46,7 @@ enum mfill_atomic_mode { MFILL_ATOMIC_COPY, MFILL_ATOMIC_ZEROPAGE, MFILL_ATOMIC_CONTINUE,
MFILL_ATOMIC_POISON, NR_MFILL_ATOMIC_MODES,};
@@ -83,6 +84,9 @@ extern ssize_t mfill_atomic_zeropage(struct mm_struct *dst_mm, extern ssize_t mfill_atomic_continue(struct mm_struct *dst_mm, unsigned long dst_start, unsigned long len, atomic_t *mmap_changing, uffd_flags_t flags); +extern ssize_t mfill_atomic_poison(struct mm_struct *dst_mm, unsigned long start,
unsigned long len, atomic_t *mmap_changing,uffd_flags_t flags);extern int mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start, unsigned long len, bool enable_wp, atomic_t *mmap_changing); diff --git a/include/uapi/linux/userfaultfd.h b/include/uapi/linux/userfaultfd.h index 66dd4cd277bd..62151706c5a3 100644 --- a/include/uapi/linux/userfaultfd.h +++ b/include/uapi/linux/userfaultfd.h @@ -39,7 +39,8 @@ UFFD_FEATURE_MINOR_SHMEM | \ UFFD_FEATURE_EXACT_ADDRESS | \ UFFD_FEATURE_WP_HUGETLBFS_SHMEM | \
UFFD_FEATURE_WP_UNPOPULATED)
UFFD_FEATURE_WP_UNPOPULATED | \UFFD_FEATURE_POISON)#define UFFD_API_IOCTLS \ ((__u64)1 << _UFFDIO_REGISTER | \ (__u64)1 << _UFFDIO_UNREGISTER | \ @@ -49,12 +50,14 @@ (__u64)1 << _UFFDIO_COPY | \ (__u64)1 << _UFFDIO_ZEROPAGE | \ (__u64)1 << _UFFDIO_WRITEPROTECT | \
(__u64)1 << _UFFDIO_CONTINUE)
(__u64)1 << _UFFDIO_CONTINUE | \(__u64)1 << _UFFDIO_POISON)#define UFFD_API_RANGE_IOCTLS_BASIC \ ((__u64)1 << _UFFDIO_WAKE | \ (__u64)1 << _UFFDIO_COPY | \
(__u64)1 << _UFFDIO_WRITEPROTECT | \ (__u64)1 << _UFFDIO_CONTINUE | \
(__u64)1 << _UFFDIO_WRITEPROTECT)
(__u64)1 << _UFFDIO_POISON)May not be a large deal, but it's still better to declare the feature & ioctls after all things implemented. Maybe make these few lines (UFFD_API*, and the new feature bit) as the last patch to enable the feature?
I agree. Another option would be to have a separate feature for UFFDIO_POISON for hugetlb, but I don't think we should do that. :)
Yeah let's make the features "memory-type-free" if possible. :)
Thanks,