Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 13 Jan 2023 15:48:59 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Borislav Petkov <bp@alien8.de>
Cc:     Michael Roth <michael.roth@amd.com>, kvm@vger.kernel.org,
        linux-coco@lists.linux.dev, linux-mm@kvack.org,
        linux-crypto@vger.kernel.org, x86@kernel.org,
        linux-kernel@vger.kernel.org, tglx@linutronix.de, mingo@redhat.com,
        jroedel@suse.de, thomas.lendacky@amd.com, hpa@zytor.com,
        ardb@kernel.org, pbonzini@redhat.com, vkuznets@redhat.com,
        wanpengli@tencent.com, jmattson@google.com, luto@kernel.org,
        dave.hansen@linux.intel.com, slp@redhat.com, pgonda@google.com,
        peterz@infradead.org, srinivas.pandruvada@linux.intel.com,
        rientjes@google.com, dovmurik@linux.ibm.com, tobin@ibm.com,
        vbabka@suse.cz, kirill@shutemov.name, ak@linux.intel.com,
        tony.luck@intel.com, marcorr@google.com,
        sathyanarayanan.kuppuswamy@linux.intel.com, alpergun@google.com,
        dgilbert@redhat.com, jarkko@kernel.org, ashish.kalra@amd.com,
        harald@profian.com
Subject: Re: [PATCH RFC v7 04/64] KVM: x86: Add 'fault_is_private' x86 op
Message-ID: <Y8F9a2pmh13aIQZL@google.com>
References: <20221214194056.161492-1-michael.roth@amd.com>
 <20221214194056.161492-5-michael.roth@amd.com>
 <Y628y6hQK38+IAev@zn.tnic>
 <20230105024256.ptujtjgzcdmpakoa@amd.com>
 <Y8Fr/F1RV0B8CHq5@zn.tnic>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y8Fr/F1RV0B8CHq5@zn.tnic>
Precedence: bulk

On Fri, Jan 13, 2023, Borislav Petkov wrote:
> On Wed, Jan 04, 2023 at 08:42:56PM -0600, Michael Roth wrote:
> > Obviously I need to add some proper documentation for this, but a 1
> > return basically means 'private_fault' pass-by-ref arg has been set
> > with the appropriate value, whereas 0 means "there's no platform-specific
> > handling for this, so if you have some generic way to determine this
> > then use that instead".
> 
> Still binary, tho, and can be bool, right?
> 
> I.e., you can just as well do:
> 
>         if (static_call(kvm_x86_fault_is_private)(kvm, gpa, err, &private_fault))
>                 goto out;
> 
> at the call site.

Ya.  Don't spend too much time trying to make this look super pretty though, there
are subtle bugs inherited from the base UPM series that need to be sorted out and
will impact this code.  E.g. invoking kvm_mem_is_private() outside of the protection
of mmu_invalidate_seq means changes to the attributes may not be reflected in the
page tables.

I'm also hoping we can avoid a callback entirely, though that may prove to be
more pain than gain.  I'm poking at the UPM and testing series right now, will
circle back to this and TDX in a few weeks to see if there's a sane way to communicate
shared vs. private without having to resort to a callback, and without having
races between page faults, KVM_SET_MEMORY_ATTRIBUTES, and KVM_SET_USER_MEMORY_REGION2.

> > This is mainly to handle CONFIG_HAVE_KVM_PRIVATE_MEM_TESTING, which
> > just parrots whatever kvm_mem_is_private() returns to support running
> > KVM selftests without needed hardware/platform support. If we don't
> > take care to skip this check where the above fault_is_private() hook
> > returns 1, then it ends up breaking SNP in cases where the kernel has
> > been compiled with CONFIG_HAVE_KVM_PRIVATE_MEM_TESTING, since SNP
> > relies on the page fault flags to make this determination, not
> > kvm_mem_is_private(), which normally only tracks the memory attributes
> > set by userspace via KVM_SET_MEMORY_ATTRIBUTES ioctl.
> 
> Some of that explanation belongs into the commit message, which is a bit
> lacking...

I'll circle back to this too when I give this series (and TDX) a proper look,
there's got too be a better way to handle this.