Received-SPF: pass (google.com: domain of linux-kernel+bounces-167905-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Michael Kelley <mhklinux@outlook.com>
To: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>, Thomas Gleixner
	<tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov
	<bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>, "x86@kernel.org"
	<x86@kernel.org>
CC: "Rafael J. Wysocki" <rafael@kernel.org>, Peter Zijlstra
	<peterz@infradead.org>, Adrian Hunter <adrian.hunter@intel.com>, Kuppuswamy
 Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>, Elena Reshetova
	<elena.reshetova@intel.com>, Jun Nakajima <jun.nakajima@intel.com>, Rick
 Edgecombe <rick.p.edgecombe@intel.com>, Tom Lendacky
	<thomas.lendacky@amd.com>, "Kalra, Ashish" <ashish.kalra@amd.com>, Sean
 Christopherson <seanjc@google.com>, "Huang, Kai" <kai.huang@intel.com>,
	Baoquan He <bhe@redhat.com>, "kexec@lists.infradead.org"
	<kexec@lists.infradead.org>, "linux-coco@lists.linux.dev"
	<linux-coco@lists.linux.dev>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, Dave Hansen <dave.hansen@intel.com>, Tao Liu
	<ltao@redhat.com>, "K. Y. Srinivasan" <kys@microsoft.com>, Haiyang Zhang
	<haiyangz@microsoft.com>, Wei Liu <wei.liu@kernel.org>, Dexuan Cui
	<decui@microsoft.com>, "linux-hyperv@vger.kernel.org"
	<linux-hyperv@vger.kernel.org>
Subject: RE: [PATCHv10 06/18] x86/mm: Make
 x86_platform.guest.enc_status_change_*() return errno
Thread-Topic: [PATCHv10 06/18] x86/mm: Make
 x86_platform.guest.enc_status_change_*() return errno
Thread-Index: AQHainGOGgLZb+RBzEGqD3vxVAH8/rGF1L6A
Date: Fri, 3 May 2024 16:29:00 +0000
Message-ID:
 <SN6PR02MB41574C2859020B4FD2769703D41F2@SN6PR02MB4157.namprd02.prod.outlook.com>
References: <20240409113010.465412-1-kirill.shutemov@linux.intel.com>
 <20240409113010.465412-7-kirill.shutemov@linux.intel.com>
In-Reply-To: <20240409113010.465412-7-kirill.shutemov@linux.intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?us-ascii?Q?1d8QWYCm2HA4i3QEeqXV1q+pj3A4ShmQ2vu42AGp1T67a1aywmk0yPwZpiAq?=
 =?us-ascii?Q?DpZin0pDlA5G6lVKUN4Jic5UW7GqpMJCZS3nZXp4ho6B5M/UqySI5MCq6a+I?=
 =?us-ascii?Q?JjWVSExHX1JvsDpOsxg++1xl2IHBhQzASqPnQlv1ZCoeKiyVDcPUETlyV0JN?=
 =?us-ascii?Q?6Pu9zf/apzGd7eYyVPFaEv7D4dc/Bc8RwVteZJrvB5BvbXM4bIlLuPQNI3gl?=
 =?us-ascii?Q?8BA/fsTF/CQAQGid+ZRSB6SOtCrbWWsnl+tmNtK0UUlgNlnmIgCDTy4xEqxn?=
 =?us-ascii?Q?35qGs0z6pYJi/K+sf/UecWTcUKZgw8rJD/9XtxzcAkvF9trGr5ZjxVGGCX9B?=
 =?us-ascii?Q?8j2cTRav7v5lMWe/4J2UdfMVQUdYe2r08mHysUEnixwzL+pN3dnAYC/sRMU5?=
 =?us-ascii?Q?/kn8blGLIFG9y/VTIHtm7uC3GQD9Y4JwHDQiDweSxxURmvLhTGQt0F8oyCIY?=
 =?us-ascii?Q?+20/vIUfaa4+NwrLLUEzUiqZN4qUUNRGT3/T9ngPbVK22ypX1g7DrgX4xLEm?=
 =?us-ascii?Q?0a3m7DLNumuhNQqI7uQaYaTmOei/10LfJpKlfefsrbU82+0Fn4lMY3NdV11x?=
 =?us-ascii?Q?R07vTSh+5LFHoyc1BHNR8lFYojUGP9Zgyu9rFyh/j4AV+d0GrOJ8cVneWr4B?=
 =?us-ascii?Q?rYn4/EiWDNN2vpE9BOVbdj6fLLwsP+SsuxJldalZ6nMul48vL3WFE4zXCfEi?=
 =?us-ascii?Q?G0g1eglZHThcVqVB6D0gxiXVbuDcrMFGSZzUZs0i4X2nQXYLN2amUkNTbDhL?=
 =?us-ascii?Q?4XEKVA25H2ht+RaOJH8jLeRMimBDl6trhZp8lP84zj2c413tCxkR3bOp44JO?=
 =?us-ascii?Q?F4dBkD4GqYjzd1JGEJnCmcpvh7ocpT5CkyAs4rkkqz8/EcR+jlYF3JakB9Xz?=
 =?us-ascii?Q?6I8WpJyze82BguRcByEEsf236RuIIsOm5GB/eiKIlaveE9aqnp7f1YQpxgLb?=
 =?us-ascii?Q?tuu8EnIx1P9OKEYxqfgsIslEyjuGPPpoznJd2cxQfWlyqjWa9K1i47uJchto?=
 =?us-ascii?Q?BG8u6YiS8n006vV4zpbUnvcYD421mE7JaM5xcfFoeqpgoM/7g12RJiNDoi2B?=
 =?us-ascii?Q?YWdVBncCGtBdq0VQNRl+emJmUZS+jukxbcUqURojTpE6EdxgRvhAaZtNsx5B?=
 =?us-ascii?Q?C50eLrCtDxM2+esTIXGXO9fy8g6hPVBODloINLcU0MiSPxZrPcgSzPQJqjQl?=
 =?us-ascii?Q?JQmXmARpkhpu1TMKV+mB3vtfPxeGxkhO335725Ru9BbEKYOaUOUtLNv173I?=
 =?us-ascii?Q?=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SN6PR02MB4157.namprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000
X-MS-Exchange-CrossTenant-Network-Message-Id: 22955027-b143-44c5-aea9-08dc6b8e2375
X-MS-Exchange-CrossTenant-rms-persistedconsumerorg: 00000000-0000-0000-0000-000000000000
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 May 2024 16:29:00.4168
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR02MB7485

From: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> Sent: Tuesday, A=
pril 9, 2024 4:30 AM
>=20
> TDX is going to have more than one reason to fail
> enc_status_change_prepare().
>=20
> Change the callback to return errno instead of assuming -EIO;
> enc_status_change_finish() changed too to keep the interface symmetric.
>=20
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Reviewed-by: Dave Hansen <dave.hansen@intel.com>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> Tested-by: Tao Liu <ltao@redhat.com>
> ---
>  arch/x86/coco/tdx/tdx.c         | 20 +++++++++++---------
>  arch/x86/hyperv/ivm.c           | 22 ++++++++++------------
>  arch/x86/include/asm/x86_init.h |  4 ++--
>  arch/x86/kernel/x86_init.c      |  4 ++--
>  arch/x86/mm/mem_encrypt_amd.c   |  8 ++++----
>  arch/x86/mm/pat/set_memory.c    |  8 +++++---
>  6 files changed, 34 insertions(+), 32 deletions(-)
>=20
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index c1cb90369915..26fa47db5782 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -798,28 +798,30 @@ static bool tdx_enc_status_changed(unsigned long va=
ddr, int numpages, bool enc)
>  	return true;
>  }
>=20
> -static bool tdx_enc_status_change_prepare(unsigned long vaddr, int numpa=
ges,
> -					  bool enc)
> +static int tdx_enc_status_change_prepare(unsigned long vaddr, int numpag=
es,
> +					 bool enc)
>  {
>  	/*
>  	 * Only handle shared->private conversion here.
>  	 * See the comment in tdx_early_init().
>  	 */
> -	if (enc)
> -		return tdx_enc_status_changed(vaddr, numpages, enc);
> -	return true;
> +	if (enc && !tdx_enc_status_changed(vaddr, numpages, enc))
> +		return -EIO;
> +
> +	return 0;
>  }
>=20
> -static bool tdx_enc_status_change_finish(unsigned long vaddr, int numpag=
es,
> +static int tdx_enc_status_change_finish(unsigned long vaddr, int numpage=
s,
>  					 bool enc)
>  {
>  	/*
>  	 * Only handle private->shared conversion here.
>  	 * See the comment in tdx_early_init().
>  	 */
> -	if (!enc)
> -		return tdx_enc_status_changed(vaddr, numpages, enc);
> -	return true;
> +	if (!enc && !tdx_enc_status_changed(vaddr, numpages, enc))
> +		return -EIO;
> +
> +	return 0;
>  }
>=20
>  void __init tdx_early_init(void)
> diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c
> index 768d73de0d09..b4a851d27c7c 100644
> --- a/arch/x86/hyperv/ivm.c
> +++ b/arch/x86/hyperv/ivm.c
> @@ -523,9 +523,9 @@ static int hv_mark_gpa_visibility(u16 count, const u6=
4 pfn[],
>   * transition is complete, hv_vtom_set_host_visibility() marks the pages
>   * as "present" again.
>   */
> -static bool hv_vtom_clear_present(unsigned long kbuffer, int pagecount, =
bool enc)
> +static int hv_vtom_clear_present(unsigned long kbuffer, int pagecount, b=
ool enc)
>  {
> -	return !set_memory_np(kbuffer, pagecount);
> +	return set_memory_np(kbuffer, pagecount);
>  }
>=20
>  /*
> @@ -536,20 +536,19 @@ static bool hv_vtom_clear_present(unsigned long kbu=
ffer, int pagecount, bool enc
>   * with host. This function works as wrap of hv_mark_gpa_visibility()
>   * with memory base and size.
>   */
> -static bool hv_vtom_set_host_visibility(unsigned long kbuffer, int pagec=
ount, bool enc)
> +static int hv_vtom_set_host_visibility(unsigned long kbuffer, int pageco=
unt, bool enc)
>  {
>  	enum hv_mem_host_visibility visibility =3D enc ?
>  			VMBUS_PAGE_NOT_VISIBLE : VMBUS_PAGE_VISIBLE_READ_WRITE;
>  	u64 *pfn_array;
>  	phys_addr_t paddr;
> +	int i, pfn, err;
>  	void *vaddr;
>  	int ret =3D 0;
> -	bool result =3D true;
> -	int i, pfn;
>=20
>  	pfn_array =3D kmalloc(HV_HYP_PAGE_SIZE, GFP_KERNEL);
>  	if (!pfn_array) {
> -		result =3D false;
> +		ret =3D -ENOMEM;
>  		goto err_set_memory_p;
>  	}
>=20
> @@ -568,10 +567,8 @@ static bool hv_vtom_set_host_visibility(unsigned lon=
g kbuffer, int pagecount, bo
>  		if (pfn =3D=3D HV_MAX_MODIFY_GPA_REP_COUNT || i =3D=3D pagecount - 1) =
{
>  			ret =3D hv_mark_gpa_visibility(pfn, pfn_array,
>  						     visibility);
> -			if (ret) {
> -				result =3D false;
> +			if (ret)
>  				goto err_free_pfn_array;
> -			}
>  			pfn =3D 0;
>  		}
>  	}
> @@ -586,10 +583,11 @@ static bool hv_vtom_set_host_visibility(unsigned lo=
ng kbuffer, int pagecount, bo
>  	 * order to avoid leaving the memory range in a "broken" state. Setting
>  	 * the PRESENT bits shouldn't fail, but return an error if it does.
>  	 */
> -	if (set_memory_p(kbuffer, pagecount))
> -		result =3D false;
> +	err =3D set_memory_p(kbuffer, pagecount);
> +	if (err && !ret)
> +		ret =3D err;
>=20
> -	return result;
> +	return ret;
>  }
>=20
>  static bool hv_vtom_tlb_flush_required(bool private)
> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_i=
nit.h
> index 6149eabe200f..28ac3cb9b987 100644
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -151,8 +151,8 @@ struct x86_init_acpi {
>   * @enc_cache_flush_required	Returns true if a cache flush is needed bef=
ore changing page encryption status
>   */
>  struct x86_guest {
> -	bool (*enc_status_change_prepare)(unsigned long vaddr, int npages, bool=
 enc);
> -	bool (*enc_status_change_finish)(unsigned long vaddr, int npages, bool =
enc);
> +	int (*enc_status_change_prepare)(unsigned long vaddr, int npages, bool =
enc);
> +	int (*enc_status_change_finish)(unsigned long vaddr, int npages, bool e=
nc);
>  	bool (*enc_tlb_flush_required)(bool enc);
>  	bool (*enc_cache_flush_required)(void);
>  };
> diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
> index d5dc5a92635a..a7143bb7dd93 100644
> --- a/arch/x86/kernel/x86_init.c
> +++ b/arch/x86/kernel/x86_init.c
> @@ -134,8 +134,8 @@ struct x86_cpuinit_ops x86_cpuinit =3D {
>=20
>  static void default_nmi_init(void) { };
>=20
> -static bool enc_status_change_prepare_noop(unsigned long vaddr, int npag=
es, bool enc) { return true; }
> -static bool enc_status_change_finish_noop(unsigned long vaddr, int npage=
s, bool enc) { return true; }
> +static int enc_status_change_prepare_noop(unsigned long vaddr, int npage=
s, bool enc) { return 0; }
> +static int enc_status_change_finish_noop(unsigned long vaddr, int npages=
, bool enc) { return 0; }
>  static bool enc_tlb_flush_required_noop(bool enc) { return false; }
>  static bool enc_cache_flush_required_noop(void) { return false; }
>  static bool is_private_mmio_noop(u64 addr) {return false; }
> diff --git a/arch/x86/mm/mem_encrypt_amd.c b/arch/x86/mm/mem_encrypt_amd.=
c
> index 422602f6039b..e7b67519ddb5 100644
> --- a/arch/x86/mm/mem_encrypt_amd.c
> +++ b/arch/x86/mm/mem_encrypt_amd.c
> @@ -283,7 +283,7 @@ static void enc_dec_hypercall(unsigned long vaddr, un=
signed long size, bool enc)
>  #endif
>  }
>=20
> -static bool amd_enc_status_change_prepare(unsigned long vaddr, int npage=
s, bool enc)
> +static int amd_enc_status_change_prepare(unsigned long vaddr, int npages=
, bool enc)
>  {
>  	/*
>  	 * To maintain the security guarantees of SEV-SNP guests, make sure
> @@ -292,11 +292,11 @@ static bool amd_enc_status_change_prepare(unsigned =
long vaddr, int npages, bool
>  	if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP) && !enc)
>  		snp_set_memory_shared(vaddr, npages);
>=20
> -	return true;
> +	return 0;
>  }
>=20
>  /* Return true unconditionally: return value doesn't matter for the SEV =
side */
> -static bool amd_enc_status_change_finish(unsigned long vaddr, int npages=
, bool enc)
> +static int amd_enc_status_change_finish(unsigned long vaddr, int npages,=
 bool enc)
>  {
>  	/*
>  	 * After memory is mapped encrypted in the page table, validate it
> @@ -308,7 +308,7 @@ static bool amd_enc_status_change_finish(unsigned lon=
g vaddr, int npages, bool e
>  	if (!cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT))
>  		enc_dec_hypercall(vaddr, npages << PAGE_SHIFT, enc);
>=20
> -	return true;
> +	return 0;
>  }
>=20
>  static void __init __set_clr_pte_enc(pte_t *kpte, int level, bool enc)
> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> index 80c9037ffadf..e5b454036bf3 100644
> --- a/arch/x86/mm/pat/set_memory.c
> +++ b/arch/x86/mm/pat/set_memory.c
> @@ -2156,7 +2156,8 @@ static int __set_memory_enc_pgtable(unsigned long a=
ddr, int numpages, bool enc)
>  		cpa_flush(&cpa, x86_platform.guest.enc_cache_flush_required());
>=20
>  	/* Notify hypervisor that we are about to set/clr encryption attribute.=
 */
> -	if (!x86_platform.guest.enc_status_change_prepare(addr, numpages, enc))
> +	ret =3D x86_platform.guest.enc_status_change_prepare(addr, numpages, en=
c);
> +	if (ret)
>  		goto vmm_fail;
>=20
>  	ret =3D __change_page_attr_set_clr(&cpa, 1);
> @@ -2174,7 +2175,8 @@ static int __set_memory_enc_pgtable(unsigned long a=
ddr, int numpages, bool enc)
>  		return ret;
>=20
>  	/* Notify hypervisor that we have successfully set/clr encryption attri=
bute. */
> -	if (!x86_platform.guest.enc_status_change_finish(addr, numpages, enc))
> +	ret =3D x86_platform.guest.enc_status_change_finish(addr, numpages, enc=
);
> +	if (ret)
>  		goto vmm_fail;
>=20
>  	return 0;
> @@ -2183,7 +2185,7 @@ static int __set_memory_enc_pgtable(unsigned long a=
ddr, int numpages, bool enc)
>  	WARN_ONCE(1, "CPA VMM failure to convert memory (addr=3D%p, numpages=3D=
%d) to %s.\n",
>  		  (void *)addr, numpages, enc ? "private" : "shared");

Nit: Now that there's an error code instead of just a boolean, it would be =
nice
to have this warning message include the error code.  Some of the callers o=
f
set_memory_decrypted()/encrypted() also output a message on failure that
includes the error code, in which case this message will be redundant.  But
many callers do not.

>=20
> -	return -EIO;
> +	return ret;
>  }
>=20
>  static int __set_memory_enc_dec(unsigned long addr, int numpages, bool e=
nc)
> --
> 2.43.0
>=20

My nit notwithstanding,

Reviewed-by: Michael Kelley <mhklinux@outlook.com>