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
>
