Linux Virtualization - May 2020 - [PATCH v3 23/75] x86/boot/compressed/64: Setup GHCB Based VC Exception handler

On Tue, Apr 28, 2020 at 05:16:33PM +0200, Joerg Roedel
wrote:
> From: Joerg Roedel <jroedel at suse.de>
> 
> Install an exception handler for #VC exception that uses a GHCB. Also
> add the infrastructure for handling different exit-codes by decoding
> the instruction that caused the exception and error handling.
> 
> Signed-off-by: Joerg Roedel <jroedel at suse.de>
> ---
>  arch/x86/Kconfig                           |   1 +
>  arch/x86/boot/compressed/Makefile          |   3 +
>  arch/x86/boot/compressed/idt_64.c          |   4 +
>  arch/x86/boot/compressed/idt_handlers_64.S |   3 +-
>  arch/x86/boot/compressed/misc.c            |   7 +
>  arch/x86/boot/compressed/misc.h            |   7 +
>  arch/x86/boot/compressed/sev-es.c          | 110 +++++++++++++++
>  arch/x86/include/asm/sev-es.h              |  39 ++++++
>  arch/x86/include/uapi/asm/svm.h            |   1 +
>  arch/x86/kernel/sev-es-shared.c            | 154 +++++++++++++++++++++
>  10 files changed, 328 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 1197b5596d5a..2ba5f74f186d 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1523,6 +1523,7 @@ config AMD_MEM_ENCRYPT
>  	select DYNAMIC_PHYSICAL_MASK
>  	select ARCH_USE_MEMREMAP_PROT
>  	select ARCH_HAS_FORCE_DMA_UNENCRYPTED
> +	select INSTRUCTION_DECODER
>  	---help---
>  	  Say yes to enable support for the encryption of system memory.
>  	  This requires an AMD processor that supports Secure Memory
> diff --git a/arch/x86/boot/compressed/Makefile
b/arch/x86/boot/compressed/Makefile
> index a7847a1ef63a..8372b85c9c0e 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -41,6 +41,9 @@ KBUILD_CFLAGS += -Wno-pointer-sign
>  KBUILD_CFLAGS += $(call cc-option,-fmacro-prefix-map=$(srctree)/=)
>  KBUILD_CFLAGS += -fno-asynchronous-unwind-tables
>  
> +# sev-es.c inludes generated $(objtree)/arch/x86/lib/inat-tables.c
	      "includes"
> +CFLAGS_sev-es.o += -I$(objtree)/arch/x86/lib/
Does it?

I see

#include "../../lib/inat.c"
#include "../../lib/insn.c"

only and with the above CFLAGS-line removed, it builds still.

Leftover from earlier?
> +
>  KBUILD_AFLAGS  := $(KBUILD_CFLAGS) -D__ASSEMBLY__
>  GCOV_PROFILE := n
>  UBSAN_SANITIZE :=n
> diff --git a/arch/x86/boot/compressed/idt_64.c
b/arch/x86/boot/compressed/idt_64.c
> index f8295d68b3e1..44d20c4f47c9 100644
> --- a/arch/x86/boot/compressed/idt_64.c
> +++ b/arch/x86/boot/compressed/idt_64.c
> @@ -45,5 +45,9 @@ void load_stage2_idt(void)
>  
>  	set_idt_entry(X86_TRAP_PF, boot_page_fault);
>  
> +#ifdef CONFIG_AMD_MEM_ENCRYPT
> +	set_idt_entry(X86_TRAP_VC, boot_stage2_vc);
> +#endif
if IS_ENABLED()...

...
> +static enum es_result vc_decode_insn(struct es_em_ctxt *ctxt)
> +{
> +	char buffer[MAX_INSN_SIZE];
> +	enum es_result ret;
> +
> +	memcpy(buffer, (unsigned char *)ctxt->regs->ip, MAX_INSN_SIZE);
> +
> +	insn_init(&ctxt->insn, buffer, MAX_INSN_SIZE, 1);
> +	insn_get_length(&ctxt->insn);
> +
> +	ret = ctxt->insn.immediate.got ? ES_OK : ES_DECODE_FAILED;
Why are we checking whether the immediate? insn_get_length() sets
insn->length unconditionally while insn_get_immediate() can error out
and not set ->got... ?
> +
> +	return ret;
> +}
...
> +static bool sev_es_setup_ghcb(void)
> +{
> +	if (!sev_es_negotiate_protocol())
> +		sev_es_terminate(GHCB_SEV_ES_REASON_PROTOCOL_UNSUPPORTED);
> +
> +	if (set_page_decrypted((unsigned long)&boot_ghcb_page))
> +		return false;
> +
> +	/* Page is now mapped decrypted, clear it */
> +	memset(&boot_ghcb_page, 0, sizeof(boot_ghcb_page));
> +
> +	boot_ghcb = &boot_ghcb_page;
> +
> +	/* Initialize lookup tables for the instruction decoder */
> +	inat_init_tables();
Yeah, that call doesn't logically belong in this function AFAICT as this
function should setup the GHCB only. You can move it to the caller.
> +
> +	return true;
> +}
> +
> +void sev_es_shutdown_ghcb(void)
> +{
> +	if (!boot_ghcb)
> +		return;
> +
> +	/*
> +	 * GHCB Page must be flushed from the cache and mapped encrypted again.
> +	 * Otherwise the running kernel will see strange cache effects when
> +	 * trying to use that page.
> +	 */
> +	if (set_page_encrypted((unsigned long)&boot_ghcb_page))
> +		error("Can't map GHCB page encrypted");
Is that error() call enough?

Shouldn't we BUG_ON() here or mark that page Reserved or so, so that
nothing uses it during the system lifetime and thus avoid the strange
cache effects?

...
> +static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb,
> +					  struct es_em_ctxt *ctxt,
> +					  u64 exit_code, u64 exit_info_1,
> +					  u64 exit_info_2)
> +{
> +	enum es_result ret;
> +
> +	/* Fill in protocol and format specifiers */
> +	ghcb->protocol_version = GHCB_PROTOCOL_MAX;
> +	ghcb->ghcb_usage       = GHCB_DEFAULT_USAGE;
> +
> +	ghcb_set_sw_exit_code(ghcb, exit_code);
> +	ghcb_set_sw_exit_info_1(ghcb, exit_info_1);
> +	ghcb_set_sw_exit_info_2(ghcb, exit_info_2);
> +
> +	sev_es_wr_ghcb_msr(__pa(ghcb));
> +	VMGEXIT();
> +
> +	if ((ghcb->save.sw_exit_info_1 & 0xffffffff) == 1) {
					^^^^^^^^^^^

(1UL << 32) - 1

I guess.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

On Tue, May 12, 2020 at 08:11:57PM +0200, Borislav Petkov
wrote:
> > +# sev-es.c inludes generated $(objtree)/arch/x86/lib/inat-tables.c
> 
> 	      "includes"
> 
> > +CFLAGS_sev-es.o += -I$(objtree)/arch/x86/lib/
> 
> Does it?
> 
> I see
> 
> #include "../../lib/inat.c"
> #include "../../lib/insn.c"
> 
> only and with the above CFLAGS-line removed, it builds still.
> 
> Leftover from earlier?
No, this is a recent addition, otherwise this breaks out-of-tree builds
(make O=/some/path ...) because inat-tables.c (included from inat.c) is
generated during build and ends up in the $(objtree).
> > +	insn_init(&ctxt->insn, buffer, MAX_INSN_SIZE, 1);
> > +	insn_get_length(&ctxt->insn);
> > +
> > +	ret = ctxt->insn.immediate.got ? ES_OK : ES_DECODE_FAILED;
> 
> Why are we checking whether the immediate? insn_get_length() sets
> insn->length unconditionally while insn_get_immediate() can error out
> and not set ->got... ?
Because the immediate is the last part of the instruction which is
decoded (even if there is no immediate). The .got field is set when
either the immediate was decoded successfully or, in case the
instruction has no immediate, when the rest of the instruction was
decoded successfully. So testing immediate.got is the indicator whether
decoding was successful.
> 
> > +
> > +	return ret;
> > +}
> 
> ...
> 
> > +static bool sev_es_setup_ghcb(void)
> > +{
> > +	if (!sev_es_negotiate_protocol())
> > +		sev_es_terminate(GHCB_SEV_ES_REASON_PROTOCOL_UNSUPPORTED);
> > +
> > +	if (set_page_decrypted((unsigned long)&boot_ghcb_page))
> > +		return false;
> > +
> > +	/* Page is now mapped decrypted, clear it */
> > +	memset(&boot_ghcb_page, 0, sizeof(boot_ghcb_page));
> > +
> > +	boot_ghcb = &boot_ghcb_page;
> > +
> > +	/* Initialize lookup tables for the instruction decoder */
> > +	inat_init_tables();
> 
> Yeah, that call doesn't logically belong in this function AFAICT as
this
> function should setup the GHCB only. You can move it to the caller.
Probably better rename the function, it also does the sev-es protocol
version negotiation and all other related setup tasks. Maybe
sev_es_setup() is a better name?
> > +	if (set_page_encrypted((unsigned long)&boot_ghcb_page))
> > +		error("Can't map GHCB page encrypted");
> 
> Is that error() call enough?
> 
> Shouldn't we BUG_ON() here or mark that page Reserved or so, so that
> nothing uses it during the system lifetime and thus avoid the strange
> cache effects?
If the above call fails its the end of the systems lifetime, because we
can't continue to boot an SEV-ES guest when we have no GHCB.

BUG_ON() and friends are also not available in the pre-decompression
boot stage.
> 
> ...
> 
> > +static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb,
> > +					  struct es_em_ctxt *ctxt,
> > +					  u64 exit_code, u64 exit_info_1,
> > +					  u64 exit_info_2)
> > +{
> > +	enum es_result ret;
> > +
> > +	/* Fill in protocol and format specifiers */
> > +	ghcb->protocol_version = GHCB_PROTOCOL_MAX;
> > +	ghcb->ghcb_usage       = GHCB_DEFAULT_USAGE;
> > +
> > +	ghcb_set_sw_exit_code(ghcb, exit_code);
> > +	ghcb_set_sw_exit_info_1(ghcb, exit_info_1);
> > +	ghcb_set_sw_exit_info_2(ghcb, exit_info_2);
> > +
> > +	sev_es_wr_ghcb_msr(__pa(ghcb));
> > +	VMGEXIT();
> > +
> > +	if ((ghcb->save.sw_exit_info_1 & 0xffffffff) == 1) {
> 					^^^^^^^^^^^
> 
> (1UL << 32) - 1
> 
> I guess.
Or lower_32_bits(), probably. I'll change it.

Thanks,

	Joerg

On Tue, May 12, 2020 at 11:08:12PM +0200, Joerg Roedel
wrote:
> No, this is a recent addition, otherwise this breaks out-of-tree builds
> (make O=/some/path ...) because inat-tables.c (included from inat.c) is
> generated during build and ends up in the $(objtree).
Please add a blurb about this above it otherwise no one would have a
clue why it is there.
> Because the immediate is the last part of the instruction which is
> decoded (even if there is no immediate). The .got field is set when
> either the immediate was decoded successfully or, in case the
> instruction has no immediate, when the rest of the instruction was
> decoded successfully. So testing immediate.got is the indicator whether
> decoding was successful.
Hm, whether the immediate was parsed correctly or it wasn't there and
using that as the sign whether the instruction was decoded successfully
sounds kinda arbitrary.

@Masami: shouldn't that insn_get_length() thing return a value to denote
whether the decode was successful or not instead of testing arbitrary
fields?

Pasting for you the code sequence again:

	...
        insn_get_length(&ctxt->insn);

        ret = ctxt->insn.immediate.got ? ES_OK : ES_DECODE_FAILED;
	...

I wonder if one would be able to do instead:

	if (insn_get_length(&ctxt->insn))
		return ES_OK;

	return ES_DECODE_FAILED;

to have it straight-forward...
> Probably better rename the function, it also does the sev-es protocol
> version negotiation and all other related setup tasks. Maybe
> sev_es_setup() is a better name?
Sure.
> If the above call fails its the end of the systems lifetime, because we
> can't continue to boot an SEV-ES guest when we have no GHCB.
> 
> BUG_ON() and friends are also not available in the pre-decompression
> boot stage.
Oh ok, error() does hlt the system.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette