Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230629205040.665834-1-axelrasmussen@google.com>
 <ZKSDLogLASaZgKCP@x1n> <CADrL8HXp-P44VxTXdJMkzSgPC8r_b0T21_cuPCTNy6Ub2PFBKA@mail.gmail.com>
 <ZKWXGnSKcOdnaeJw@x1n> <CADrL8HWO8g2-YdUtyLM6e+f1VJq6YV-b1_rj-beEh2C84kAgEw@mail.gmail.com>
 <ZKWcgD1hFFUxeQHg@x1n>
In-Reply-To: <ZKWcgD1hFFUxeQHg@x1n>
From:   Axel Rasmussen <axelrasmussen@google.com>
Date:   Wed, 5 Jul 2023 10:56:23 -0700
Message-ID: <CAJHvVcjNDazqnopjcqx2LLcmQf_4QUeGBSvSJxwGh40e78uhCg@mail.gmail.com>
Subject: Re: [PATCH v2 1/6] mm: userfaultfd: add new UFFDIO_POISON ioctl
To:     Peter Xu <peterx@redhat.com>
Cc:     James Houghton <jthoughton@google.com>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Andrew Morton <akpm@linux-foundation.org>,
        Christian Brauner <brauner@kernel.org>,
        David Hildenbrand <david@redhat.com>,
        Huang Ying <ying.huang@intel.com>,
        Hugh Dickins <hughd@google.com>,
        Jiaqi Yan <jiaqiyan@google.com>,
        Jonathan Corbet <corbet@lwn.net>,
        "Liam R. Howlett" <Liam.Howlett@oracle.com>,
        Miaohe Lin <linmiaohe@huawei.com>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        "Mike Rapoport (IBM)" <rppt@kernel.org>,
        Muchun Song <muchun.song@linux.dev>,
        Nadav Amit <namit@vmware.com>,
        Naoya Horiguchi <naoya.horiguchi@nec.com>,
        Shuah Khan <shuah@kernel.org>,
        ZhangPeng <zhangpeng362@huawei.com>, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-mm@kvack.org, linux-kselftest@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Jul 5, 2023 at 9:38=E2=80=AFAM Peter Xu <peterx@redhat.com> wrote:
>
> On Wed, Jul 05, 2023 at 09:27:15AM -0700, James Houghton wrote:
> > On Wed, Jul 5, 2023 at 9:15=E2=80=AFAM Peter Xu <peterx@redhat.com> wro=
te:
> > >
> > > 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 re=
name
> > > > > SWAPIN_ERROR and reuse it, could you explain why a new bit is sti=
ll 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, afai=
ct,
> > > > > 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 Axe=
l
> > > > has implemented). And I think that needs a separate PTE marker.
> > >
> > > My question was, should we also make SWAPIN_ERROR return VM_FAULT_HWP=
OISON
> > > always?
> > >
> > > Just to recap from what I already commented above - if a guest page g=
ot
> > > 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 wr=
ong
> > > when in kvm context and we should fix it first.
> >
> > Oh! Yes, I agree, though I'm not familiar enough with the users of
> > SWAPIN_ERROR to know if we can actually make this change.

Sorry I missed this, I'll take another pass looking at existing
SWAPIN_ERROR uses, and see if this can be done.

Thanks for the thorough review Peter, I'll address this comment and
the others in a v3. :)

>
> Miaohe initially proposed this swapin error facility, let's see whether h=
e
> can comment; he's already in the cc list.
>
> AFAICT that's the right thing to do, and it shouldn't affect any existing
> user of swapin error if there is.
>
> Or say, VM_FAULT_HWPOISON should be the same as VM_FAULT_SIGBUS when not =
in
> kvm context, so shouldn't change a thing in !kvm, while changing that
> should fix kvm from crashing a guest where we shouldn't need to.
>
> --
> Peter Xu
>