Received-SPF: Pass (protection.outlook.com: domain of amd.com designates
 165.204.84.17 as permitted sender) receiver=protection.outlook.com;
 client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C
Date:   Mon, 20 Feb 2023 10:22:10 -0600
From:   Michael Roth <michael.roth@amd.com>
To:     Sean Christopherson <seanjc@google.com>
CC:     Borislav Petkov <bp@alien8.de>, <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: <20230220162210.42rjdgbdwbjiextz@amd.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>
 <Y8F9a2pmh13aIQZL@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Y8F9a2pmh13aIQZL@google.com>
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Feb 2023 16:41:36.7959
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: b6c2e143-74b1-423a-2241-08db1361556b
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com]
X-MS-Exchange-CrossTenant-AuthSource: BN8NAM11FT082.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB6233
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org

On Fri, Jan 13, 2023 at 03:48:59PM +0000, Sean Christopherson wrote:
> 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.

Can circle back on this, but for v8 at least I've kept the callback, but
simplified SVM implementation of it so that it's only needed for SNP. For
protected-SEV it will fall through to the same generic handling used by UPM
self-tests.

It seems like it's safe to have a callback of that sort here for TDX/SNP (or
whatever we end up replacing the callback with), since the #NPF flags
themselves won't change based on attribute updates, and the subsequent
comparison to kvm_mem_is_private() will happen after mmu_invalidate_seq
is logged.

But for protected-SEV and UPM selftests the initial kvm_mem_is_private()
can become stale vs. the one in __kvm_faultin_pfn(), but it seems like ATM
it would only lead to a spurious KVM_EXIT_MEMORY_FAULT, which SEV at least
should treat at an implicit page-state change and be able to recover from.
But yah, not ideal, and maybe for self-tests that makes it difficult to tell
if things are working as expected or not.

Maybe we should just skip setting fault->is_private here in the
non-TDX/non-SNP cases, and just have some other indicator so it's
initialized/ignored in kvm_mem_is_private() later. I think some iterations
of UPM did it this way prior to 'is_private' becoming const.

> 
> > > 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.
> 

It seems like for SNP/TDX we just need to register the shared/encrypted
bits with KVM MMU and let it handle checking the #NPF flags, but can
iterate on that for the next spin when we have a better idea what it
should look like.

-Mike