Message-ID: <CA+EHjTyDPEY7B_a8GC7RS8gzfoT2q9kJqJPuHB58ZXQ_61NGkQ@mail.gmail.com>
Date:   Fri, 6 Oct 2023 13:47:01 +0100
From:   Fuad Tabba <tabba@...gle.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>, Marc Zyngier <maz@...nel.org>,
        Oliver Upton <oliver.upton@...ux.dev>,
        Huacai Chen <chenhuacai@...nel.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Anup Patel <anup@...infault.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        "Matthew Wilcox (Oracle)" <willy@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Paul Moore <paul@...l-moore.com>,
        James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>, KVM <kvm@...r.kernel.org>,
        "moderated list:ARM64 PORT (AARCH64 ARCHITECTURE)" 
        <linux-arm-kernel@...ts.infradead.org>,
        KVMARM <kvmarm@...ts.linux.dev>,
        LinuxMIPS <linux-mips@...r.kernel.org>,
        linuxppc-dev@...ts.ozlabs.org, kvm-riscv@...ts.infradead.org,
        linux-riscv@...ts.infradead.org, linux-fsdevel@...r.kernel.org,
        linux-mm@...ck.org, linux-security-module@...r.kernel.org,
        open list <linux-kernel@...r.kernel.org>,
        Chao Peng <chao.p.peng@...ux.intel.com>,
        Jarkko Sakkinen <jarkko@...nel.org>,
        Anish Moorthy <amoorthy@...gle.com>,
        Yu Zhang <yu.c.zhang@...ux.intel.com>,
        Isaku Yamahata <isaku.yamahata@...el.com>,
        Xu Yilun <yilun.xu@...el.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Vishal Annapurve <vannapurve@...gle.com>,
        Ackerley Tng <ackerleytng@...gle.com>,
        Maciej Szmigiero <mail@...iej.szmigiero.name>,
        David Hildenbrand <david@...hat.com>,
        Quentin Perret <qperret@...gle.com>,
        Michael Roth <michael.roth@....com>,
        Wang <wei.w.wang@...el.com>,
        Liam Merwick <liam.merwick@...cle.com>,
        Isaku Yamahata <isaku.yamahata@...il.com>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>
Subject: Re: [RFC PATCH v12 11/33] KVM: Introduce per-page memory attributes

Hi Sean,

On Fri, Oct 6, 2023 at 4:21 AM Sean Christopherson <seanjc@...gle.com> wrote:
>
> On Thu, Oct 05, 2023, Fuad Tabba wrote:
> > Hi Sean,
> >
> > On Tue, Oct 3, 2023 at 9:51 PM Sean Christopherson <seanjc@...gle.com> wrote:
> > > > Like I said, pKVM doesn't need a userspace ABI for managing PRIVATE/SHARED,
> > > > just a way of tracking in the host kernel of what is shared (as opposed to
> > > > the hypervisor, which already has the knowledge). The solution could simply
> > > > be that pKVM does not enable KVM_GENERIC_MEMORY_ATTRIBUTES, has its own
> > > > tracking of the status of the guest pages, and only selects KVM_PRIVATE_MEM.
> > >
> > > At the risk of overstepping my bounds, I think that effectively giving the guest
> > > full control over what is shared vs. private is a mistake.  It more or less locks
> > > pKVM into a single model, and even within that model, dealing with errors and/or
> > > misbehaving guests becomes unnecessarily problematic.
> > >
> > > Using KVM_SET_MEMORY_ATTRIBUTES may not provide value *today*, e.g. the userspace
> > > side of pKVM could simply "reflect" all conversion hypercalls, and terminate the
> > > VM on errors.  But the cost is very minimal, e.g. a single extra ioctl() per
> > > converion, and the upside is that pKVM won't be stuck if a use case comes along
> > > that wants to go beyond "all conversion requests either immediately succeed or
> > > terminate the guest".
> >
> > Now that I understand the purpose of KVM_SET_MEMORY_ATTRIBUTES, I
> > agree. However, pKVM needs to track at the host kernel (i.e., EL1)
> > whether guest memory is shared or private.
>
> Why does EL1 need it's own view/opinion?  E.g. is it to avoid a accessing data
> that is still private according to EL2 (on behalf of the guest)?
>
> Assuming that's the case, why can't EL1 wait until it gets confirmation from EL2
> that the data is fully shared before doing whatever it is that needs to be done?
>
> Ah, is the problem that whether or not .mmap() is allowed keys off of the state
> of the memory attributes?  If that's so, then yeah, an internal flag in attributes
> is probably the way to go.  It doesn't need to be a "host kernel private" flag
> though, e.g. an IN_FLUX flag to capture that the attributes aren't fully realized
> might be more intuitive for readers, and might have utility for other attributes
> in the future too.

Yes, it's because of mmap. I think that an IN_FLUX flag might work
here. I'll have a go at it and see how it turns out.

Thanks,
/fuad

>
> > One approach would be to add another flag to the attributes that
> > tracks the host kernel view. The way KVM_SET_MEMORY_ATTRIBUTES is
> > implemented now, userspace can zero it, so in that case, that
> > operation would need to be masked to avoid that.
> >
> > Another approach would be to have a pKVM-specific xarray (or similar)
> > to do the tracking, but since there is a structure that's already
> > doing something similar (i.e.,the attributes array), it seems like it
> > would be unnecessary overhead.
> >
> > Do you have any ideas or preferences?
> >
> > Cheers,
> > /fuad

Powered by blists - more mailing lists