| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 20 Jun 2023 11:30:51 -0700
From: Sathyanarayanan Kuppuswamy
<sathyanarayanan.kuppuswamy@...ux.intel.com>
To: Dexuan Cui <decui@...rosoft.com>, ak@...ux.intel.com,
arnd@...db.de, bp@...en8.de, brijesh.singh@....com,
dan.j.williams@...el.com, dave.hansen@...el.com,
dave.hansen@...ux.intel.com, haiyangz@...rosoft.com, hpa@...or.com,
jane.chu@...cle.com, kirill.shutemov@...ux.intel.com,
kys@...rosoft.com, linux-arch@...r.kernel.org,
linux-hyperv@...r.kernel.org, luto@...nel.org, mingo@...hat.com,
peterz@...radead.org, rostedt@...dmis.org, seanjc@...gle.com,
tglx@...utronix.de, tony.luck@...el.com, wei.liu@...nel.org,
x86@...nel.org, mikelley@...rosoft.com
Cc: linux-kernel@...r.kernel.org, Tianyu.Lan@...rosoft.com,
rick.p.edgecombe@...el.com
Subject: Re: [PATCH v8 1/2] x86/tdx: Retry TDVMCALL_MAP_GPA() when needed
Hi,
On 6/20/23 8:48 AM, Dexuan Cui wrote:
> GHCI spec for TDX 1.0 says that the MapGPA call may fail with the R10
> error code = TDG.VP.VMCALL_RETRY (1), and the guest must retry this
> operation for the pages in the region starting at the GPA specified
> in R11.
>
> When a fully enlightened TDX guest runs on Hyper-V, Hyper-V can return
> the retry error when set_memory_decrypted() is called to decrypt up to
> 1GB of swiotlb bounce buffers.
>
> Acked-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> Reviewed-by: Michael Kelley <mikelley@...rosoft.com>
> Signed-off-by: Dexuan Cui <decui@...rosoft.com>
> ---
>
> arch/x86/coco/tdx/tdx.c | 63 +++++++++++++++++++++++++------
> arch/x86/include/asm/shared/tdx.h | 2 +
> 2 files changed, 53 insertions(+), 12 deletions(-)
>
> Changes in v2:
> Used __tdx_hypercall() directly in tdx_map_gpa().
> Added a max_retry_cnt of 1000.
> Renamed a few variables, e.g., r11 -> map_fail_paddr.
>
> Changes in v3:
> Changed max_retry_cnt from 1000 to 3.
>
> Changes in v4:
> __tdx_hypercall(&args, TDX_HCALL_HAS_OUTPUT) -> __tdx_hypercall_ret()
> Added Kirill's Acked-by.
>
> Changes in v5:
> Added Michael's Reviewed-by.
>
> Changes in v6: None.
>
> Changes in v7:
> Addressed Dave's comments:
> see https://lwn.net/ml/linux-kernel/SA1PR21MB1335736123C2BCBBFD7460C3BF46A@SA1PR21MB1335.namprd21.prod.outlook.com
>
> Changes in v8:
> Rebased to tip.git's master branch.
>
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 1d6b863c42b0..0c198ab73aa7 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -703,14 +703,16 @@ static bool tdx_cache_flush_required(void)
> }
>
> /*
> - * Inform the VMM of the guest's intent for this physical page: shared with
> - * the VMM or private to the guest. The VMM is expected to change its mapping
> - * of the page in response.
> + * Notify the VMM about page mapping conversion. More info about ABI
> + * can be found in TDX Guest-Host-Communication Interface (GHCI),
> + * section "TDG.VP.VMCALL<MapGPA>".
> */
> -static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
> +static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc)
> {
> - phys_addr_t start = __pa(vaddr);
> - phys_addr_t end = __pa(vaddr + numpages * PAGE_SIZE);
> + const int max_retries_per_page = 3;
Add some details about why you chose 3? Maybe you can also use macro for it.
> + struct tdx_hypercall_args args;
> + u64 map_fail_paddr, ret;
> + int retry_count = 0;
>
> if (!enc) {
> /* Set the shared (decrypted) bits: */
> @@ -718,12 +720,49 @@ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
> end |= cc_mkdec(0);
> }
>
> - /*
> - * Notify the VMM about page mapping conversion. More info about ABI
> - * can be found in TDX Guest-Host-Communication Interface (GHCI),
> - * section "TDG.VP.VMCALL<MapGPA>"
> - */
> - if (_tdx_hypercall(TDVMCALL_MAP_GPA, start, end - start, 0, 0))
> + while (retry_count < max_retries_per_page) {
> + memset(&args, 0, sizeof(args));
> + args.r10 = TDX_HYPERCALL_STANDARD;
> + args.r11 = TDVMCALL_MAP_GPA;
> + args.r12 = start;
> + args.r13 = end - start;
> +
> + ret = __tdx_hypercall_ret(&args);
> + if (ret != TDVMCALL_STATUS_RETRY)
> + return !ret;
> + /*
> + * The guest must retry the operation for the pages in the
> + * region starting at the GPA specified in R11. R11 comes
> + * from the untrusted VMM. Sanity check it.
> + */
> + map_fail_paddr = args.r11;
Do you really need map_fail_paddr? Why not directly use args.r11?
> + if (map_fail_paddr < start || map_fail_paddr >= end)
> + return false;
> +
> + /* "Consume" a retry without forward progress */
> + if (map_fail_paddr == start) {
> + retry_count++;
> + continue;
> + }
> +
> + start = map_fail_paddr;
> + retry_count = 0;
> + }
> +
> + return false;
> +}
> +
> +/*
> + * Inform the VMM of the guest's intent for this physical page: shared with
> + * the VMM or private to the guest. The VMM is expected to change its mapping
> + * of the page in response.
> + */
> +static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
> +{
> + phys_addr_t start = __pa(vaddr);
> + phys_addr_t end = __pa(vaddr + numpages * PAGE_SIZE);
> +
> + if (!tdx_map_gpa(start, end, enc))
> return false;
>
> /* shared->private conversion requires memory to be accepted before use */
> diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
> index 90ea813c4b99..9db89a99ae5b 100644
> --- a/arch/x86/include/asm/shared/tdx.h
> +++ b/arch/x86/include/asm/shared/tdx.h
> @@ -24,6 +24,8 @@
> #define TDVMCALL_MAP_GPA 0x10001
> #define TDVMCALL_REPORT_FATAL_ERROR 0x10003
>
> +#define TDVMCALL_STATUS_RETRY 1
> +
> #ifndef __ASSEMBLY__
>
> /*
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
Powered by blists - more mailing lists