Virtualization - Jun 2020 - [PATCH v3 42/75] x86/sev-es: Setup GHCB based boot #VC handler

On Tue, Apr 28, 2020 at 05:16:52PM +0200, Joerg Roedel
wrote:
> diff --git a/arch/x86/include/asm/sev-es.h b/arch/x86/include/asm/sev-es.h
> index b2cbcd40b52e..e1ed963a57ec 100644
> --- a/arch/x86/include/asm/sev-es.h
> +++ b/arch/x86/include/asm/sev-es.h
> @@ -74,5 +74,6 @@ static inline u64 lower_bits(u64 val, unsigned int bits)
>  }
>  
>  extern void vc_no_ghcb(void);
> +extern bool vc_boot_ghcb(struct pt_regs *regs);
Those function names need verbs:

	handle_vc_no_ghcb
	handle_vc_boot_ghcb
> @@ -161,3 +176,104 @@ static enum es_result vc_read_mem(struct es_em_ctxt
*ctxt,
>  
>  /* Include code shared with pre-decompression boot stage */
>  #include "sev-es-shared.c"
> +
> +/*
> + * This function runs on the first #VC exception after the kernel
> + * switched to virtual addresses.
> + */
> +static bool __init sev_es_setup_ghcb(void)
There's already another sev_es_setup_ghcb() in compressed/. All those
functions with the same name are just confusion waiting to happen. Let's
prepend the ones in compressed/ with "early_" or so, so that their
names
are at least different even if they're in two different files with the
same name.

This way you know at least which function is used in which boot stages.
> +{
> +	/* First make sure the hypervisor talks a supported protocol. */
> +	if (!sev_es_negotiate_protocol())
> +		return false;
<---- newline here.
> +	/*
> +	 * Clear the boot_ghcb. The first exception comes in before the bss
> +	 * section is cleared.
> +	 */
> +	memset(&boot_ghcb_page, 0, PAGE_SIZE);
> +
> +	/* Alright - Make the boot-ghcb public */
> +	boot_ghcb = &boot_ghcb_page;
> +
> +	return true;
> +}
> +
> +static void __init vc_early_vc_forward_exception(struct es_em_ctxt *ctxt)
That second "vc" looks redundant.
> +{
> +	int trapnr = ctxt->fi.vector;
> +
> +	if (trapnr == X86_TRAP_PF)
> +		native_write_cr2(ctxt->fi.cr2);
> +
> +	ctxt->regs->orig_ax = ctxt->fi.error_code;
> +	do_early_exception(ctxt->regs, trapnr);
> +}
> +
> +static enum es_result vc_handle_exitcode(struct es_em_ctxt *ctxt,
> +					 struct ghcb *ghcb,
> +					 unsigned long exit_code)
> +{
> +	enum es_result result;
> +
> +	switch (exit_code) {
> +	default:
> +		/*
> +		 * Unexpected #VC exception
> +		 */
> +		result = ES_UNSUPPORTED;
> +	}
> +
> +	return result;
> +}
> +
> +bool __init vc_boot_ghcb(struct pt_regs *regs)
> +{
> +	unsigned long exit_code = regs->orig_ax;
> +	struct es_em_ctxt ctxt;
> +	enum es_result result;
> +
> +	/* Do initial setup or terminate the guest */
> +	if (unlikely(boot_ghcb == NULL && !sev_es_setup_ghcb()))
> +		sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST);
> +
> +	vc_ghcb_invalidate(boot_ghcb);
Newline here...
> +	result = vc_init_em_ctxt(&ctxt, regs, exit_code);
> +
... remove that one here.
> +	if (result == ES_OK)
> +		result = vc_handle_exitcode(&ctxt, boot_ghcb, exit_code);
...

-- 
Regards/Gruss,
    Boris.

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

On Wed, May 20, 2020 at 09:22:30PM +0200, Borislav Petkov
wrote:
> On Tue, Apr 28, 2020 at 05:16:52PM +0200, Joerg Roedel wrote:
> > diff --git a/arch/x86/include/asm/sev-es.h
b/arch/x86/include/asm/sev-es.h
> > index b2cbcd40b52e..e1ed963a57ec 100644
> > --- a/arch/x86/include/asm/sev-es.h
> > +++ b/arch/x86/include/asm/sev-es.h
> > @@ -74,5 +74,6 @@ static inline u64 lower_bits(u64 val, unsigned int
bits)
> >  }
> >  
> >  extern void vc_no_ghcb(void);
> > +extern bool vc_boot_ghcb(struct pt_regs *regs);
> 
> Those function names need verbs:
> 
> 	handle_vc_no_ghcb
> 	handle_vc_boot_ghcb
This are IDT entry points and the names above follow the convention for
them, like e.g. 'page_fault', 'nmi' or
'general_protection'. Should I
still add the verbs or just add a comment explaining what those symbols
are?
> There's already another sev_es_setup_ghcb() in compressed/. All those
> functions with the same name are just confusion waiting to happen.
Let's
> prepend the ones in compressed/ with "early_" or so, so that
their names
> are at least different even if they're in two different files with the
> same name.
> 
> This way you know at least which function is used in which boot stages.
Okay, will see what can be changed. Some functions are part of the
interface for sev-es-shared.c and need to have the same names, but
sev_es_setup_ghcb() can be named differently.
> > +static void __init vc_early_vc_forward_exception(struct es_em_ctxt
*ctxt)
> 
> That second "vc" looks redundant.
Heh, search and replace artifact :) Fixed now.


	Joerg

On Thu, Jun 04, 2020 at 02:07:49PM +0200, Joerg Roedel
wrote:
> This are IDT entry points and the names above follow the convention for
> them, like e.g. 'page_fault', 'nmi' or
'general_protection'. Should I
> still add the verbs or just add a comment explaining what those symbols
> are?
Hmmkay, I see vc_no_ghcb doing

call    do_vc_no_ghcb

and that's setup in early_idt_setup().

vc_boot_ghcb(), OTOH, is called by do_early_exception() only so that one
could be called handle_vc_boot_ghcb(), no? I.e., I don't see it being an
IDT entry point.

-- 
Regards/Gruss,
    Boris.

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