Re: [kvm-unit-tests PATCH v4 08/13] x86: efi: Provide percpu storage

[Sean Christopherson <seanjc@google.com>]

Please send a standalone patch (with a SOB), this series has already been merged.

On Mon, Aug 22, 2022, Vasant Karasulli wrote:
> Writing to MSR_IA32_APICBASE in reset_apic() is an
> intercepted operation

Is _typically_ an intercepted operation, architecturally there's nothing that
requires APICBASE to emulated.  

> and causes #VC exception when the test is launched as
> an SEV-ES guest.
>
> So calling reset_apic() before IDT is set up in setup_idt() and
> load_idt() might cause problems.

> Similarly if accessing _percpu_data array element in setup_segments64() results
> in a page fault, this will lead to a double fault.

Well, yeah, but loading the IDT isn't going to magically fix the #PF.  I suspect
you're actually referring to the emulated MMIO #NPF=>#VC when accessing the xAPIC
through MMIO?

> Hence move reset_apic() call and percpu data setup after
> setup_idt() and load_idt().
> ---
>  lib/x86/setup.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/x86/setup.c b/lib/x86/setup.c
> index 7df0256..b14e692 100644
> --- a/lib/x86/setup.c
> +++ b/lib/x86/setup.c
> @@ -192,8 +192,6 @@ static void setup_segments64(void)
>  	write_gs(KERNEL_DS);
>  	write_ss(KERNEL_DS);
> 
> -	/* Setup percpu base */
> -	wrmsr(MSR_GS_BASE, (u64)&__percpu_data[pre_boot_apic_id()]);
> 
>  	/*
>  	 * Update the code segment by putting it on the stack before the return
> @@ -322,7 +320,7 @@ efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo)
>  		}
>  		return status;
>  	}
> -
> +
>  	status = setup_rsdp(efi_bootinfo);
>  	if (status != EFI_SUCCESS) {
>  		printf("Cannot find RSDP in EFI system table\n");
> @@ -344,14 +342,15 @@ efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo)
>  	}
> 
>  	setup_gdt_tss();
> +	setup_segments64();
> +	setup_idt();
> +	load_idt();
>  	/*
>  	 * GS.base, which points at the per-vCPU data, must be configured prior
>  	 * to resetting the APIC, which sets the per-vCPU APIC ops.
>  	 */
> -	setup_segments64();
> +	wrmsr(MSR_GS_BASE, (u64)&__percpu_data[pre_boot_apic_id()]);

This absolutely needs a comment, otherwise someone will wonder why on earth GS.base
isn't configured during setup_segments64().  Easist thing is probalby to split and
reword the above comment, e.g.

	/*
	 * Load GS.base with the per-vCPU data.  This must be done after loading
	 * the IDT as reading the APIC ID may #VC when running as an SEV-ES guest.
	 */
	wrmsr(MSR_GS_BASE, (u64)&__percpu_data[pre_boot_apic_id()]);

	/*
	 * Resetting the APIC sets the per-vCPU APIC ops and so must be done
	 * after loading GS.base with the per-vCPU data.
	 */
	reset_apic();	
	
>  	reset_apic();
> -	setup_idt();
> -	load_idt();
>  	mask_pic_interrupts();
>  	setup_page_table();
>  	enable_apic();
> --
> 2.34.1
>