Received-SPF: pass (google.com: domain of linux-crypto+bounces-3240-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
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, 1 Apr 2024 17:22:29 -0500
From: Michael Roth <michael.roth@amd.com>
To: Paolo Bonzini <pbonzini@redhat.com>
CC: <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>, <seanjc@google.com>, <vkuznets@redhat.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>, <bp@alien8.de>, <vbabka@suse.cz>,
	<kirill@shutemov.name>, <ak@linux.intel.com>, <tony.luck@intel.com>,
	<sathyanarayanan.kuppuswamy@linux.intel.com>, <alpergun@google.com>,
	<jarkko@kernel.org>, <ashish.kalra@amd.com>, <nikunj.dadhania@amd.com>,
	<pankaj.gupta@amd.com>, <liam.merwick@oracle.com>, Brijesh Singh
	<brijesh.singh@amd.com>, Xu Yilun <yilun.xu@linux.intel.com>, Binbin Wu
	<binbin.wu@linux.intel.com>, Xiaoyao Li <xiaoyao.li@intel.com>
Subject: Re: [PATCH v12 11/29] KVM: SEV: Add KVM_SEV_SNP_LAUNCH_UPDATE command
Message-ID: <20240401222229.qpnpozdsr6b2sntk@amd.com>
References: <20240329225835.400662-1-michael.roth@amd.com>
 <20240329225835.400662-12-michael.roth@amd.com>
 <8c3685a6-833c-4b3c-83f4-c0bd78bba36e@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <8c3685a6-833c-4b3c-83f4-c0bd78bba36e@redhat.com>
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Apr 2024 22:22:52.0061
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 3a43d095-3e4e-417d-b0d8-08dc529a455a
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:
	CH1PEPF0000AD7D.namprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB8222

On Sat, Mar 30, 2024 at 09:31:40PM +0100, Paolo Bonzini wrote:
> On 3/29/24 23:58, Michael Roth wrote:

Cc'ing some more TDX folks.

> > +	memslot = gfn_to_memslot(kvm, params.gfn_start);
> > +	if (!kvm_slot_can_be_private(memslot)) {
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> 
> This can be moved to kvm_gmem_populate.

That does seem nicer, but I hadn't really seen that pattern for
kvm_gmem_get_pfn()/etc. so wasn't sure if that was by design or not. I
suppose in those cases the memslot is already available at the main
KVM page-fault call-sites so maybe it was just unecessary to do the
lookup internally there.

> 
> > +	populate_args.src = u64_to_user_ptr(params.uaddr);
> 
> This is not used if !do_memcpy, and in fact src is redundant with do_memcpy.
> Overall the arguments can be "kvm, gfn, src, npages, post_populate, opaque"
> which are relatively few and do not need the struct.

This was actually a consideration for TDX that was discussed during the
"Finalizing internal guest_memfd APIs for SNP/TDX" PUCK call. In that
case, they have a TDH_MEM_PAGE_ADD seamcall that takes @src and encrypts
it, loads it into the destination page, and then maps it into SecureEPT
through a single call. So in that particular case, @src would be
initialized, but the memcpy() would be unecessary.

It's not actually clear TDX plans to use this interface. In v19 they still
used a KVM MMU hook (set_private_spte) that gets triggered through a call
to KVM_MAP_MEMORY->kvm_mmu_map_tdp_page() prior to starting the guest. But
more recent discussion[1] suggests that KVM_MAP_MEMORY->kvm_mmu_map_tdp_page()
would now only be used to create upper levels of SecureEPT, and the
actual mapping/encrypting of the leaf page would be handled by a
separate TDX-specific interface.

With that model, the potential for using kvm_gmem_populate() seemed
plausible to I was trying to make it immediately usable for that
purpose. But maybe the TDX folks can confirm whether this would be
usable for them or not. (kvm_gmem_populate was introduced here[2] for
reference/background)

-Mike

[1] https://lore.kernel.org/kvm/20240319155349.GE1645738@ls.amr.corp.intel.com/T/#m8580d8e39476be565534d6ff5f5afa295fe8d4f7
[2] https://lore.kernel.org/kvm/20240329212444.395559-3-michael.roth@amd.com/T/#m3aeba660fcc991602820d3703b1265722b871025)


> 
> I'll do that when posting the next version of the patches in kvm-coco-queue.
> 
> Paolo
>