Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Mon, 7 Feb 2022 22:03:52 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Joerg Roedel <joro@8bytes.org>
Cc:     x86@kernel.org, Joerg Roedel <jroedel@suse.de>,
        Eric Biederman <ebiederm@xmission.com>,
        kexec@lists.infradead.org, hpa@zytor.com,
        Andy Lutomirski <luto@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Jiri Slaby <jslaby@suse.cz>,
        Dan Williams <dan.j.williams@intel.com>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        Juergen Gross <jgross@suse.com>,
        Kees Cook <keescook@chromium.org>,
        David Rientjes <rientjes@google.com>,
        Cfir Cohen <cfir@google.com>,
        Erdem Aktas <erdemaktas@google.com>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Mike Stunes <mstunes@vmware.com>,
        Martin Radev <martin.b.radev@gmail.com>,
        Arvind Sankar <nivedita@alum.mit.edu>,
        linux-coco@lists.linux.dev, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org, virtualization@lists.linux-foundation.org
Subject: Re: [PATCH v3 04/10] x86/sev: Cache AP Jump Table Address
Message-ID: <YgGXSKAsi4aLxFMg@google.com>
References: <20220127101044.13803-1-joro@8bytes.org>
 <20220127101044.13803-5-joro@8bytes.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220127101044.13803-5-joro@8bytes.org>
Precedence: bulk

On Thu, Jan 27, 2022, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Store the physical address of the AP jump table in kernel memory so
> that it does not need to be fetched from the Hypervisor again.

This doesn't explain why the kernel would retrieve the jump table more than once,
e.g. at this point in the series, this can only ever be called once.

> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  arch/x86/kernel/sev.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index 8a4317fa699a..969ef9855bb5 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -43,6 +43,9 @@ static struct ghcb boot_ghcb_page __bss_decrypted __aligned(PAGE_SIZE);
>   */
>  static struct ghcb __initdata *boot_ghcb;
>  
> +/* Cached AP jump table Address */
> +static phys_addr_t jump_table_pa;
> +
>  /* #VC handler runtime per-CPU data */
>  struct sev_es_runtime_data {
>  	struct ghcb ghcb_page;
> @@ -523,12 +526,14 @@ void noinstr __sev_es_nmi_complete(void)
>  	__sev_put_ghcb(&state);
>  }
>  
> -static u64 get_jump_table_addr(void)
> +static phys_addr_t get_jump_table_addr(void)

Not new, but I believe this can be tagged __init.

>  {
>  	struct ghcb_state state;
>  	unsigned long flags;
>  	struct ghcb *ghcb;
> -	u64 ret = 0;
> +
> +	if (jump_table_pa)
> +		return jump_table_pa;
>  
>  	local_irq_save(flags);
>  
> @@ -544,39 +549,36 @@ static u64 get_jump_table_addr(void)
>  
>  	if (ghcb_sw_exit_info_1_is_valid(ghcb) &&
>  	    ghcb_sw_exit_info_2_is_valid(ghcb))
> -		ret = ghcb->save.sw_exit_info_2;
> +		jump_table_pa = (phys_addr_t)ghcb->save.sw_exit_info_2;
>  
>  	__sev_put_ghcb(&state);
>  
>  	local_irq_restore(flags);
>  
> -	return ret;
> +	return jump_table_pa;
>  }
>  
>  int sev_es_setup_ap_jump_table(struct real_mode_header *rmh)

__init here too.

>  {
>  	u16 startup_cs, startup_ip;
> -	phys_addr_t jump_table_pa;
> -	u64 jump_table_addr;
>  	u16 __iomem *jump_table;
> +	phys_addr_t pa;
>  
> -	jump_table_addr = get_jump_table_addr();
> +	pa = get_jump_table_addr();
>  
>  	/* On UP guests there is no jump table so this is not a failure */

Does anything actually check that the jump table is valid for SMP guests?

> -	if (!jump_table_addr)
> +	if (!pa)

Using '0' for "not valid" is funky because '0' isn't technically an illegal GPA,
and because it means the address (or lack thereof) isn't cached on a single-vCPU
guest.