On Mo 22-08-22 17:30:00, Sean Christopherson wrote:
> Please send a standalone patch (with a SOB), this series has already been merged.
>
Sorry, I will send a new patch.
> 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.
Right, I will improve the wording.
>
> > 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?
Damn, I overlooked the pre_boot_apic_id() call, thought just accessing the array
element is the culprit. :(
>
> > 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.
Will do that. Thanks for the suggestion and the review.
>
> /*
> * 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
> >
Thanks,
Vasant Karasulli
Kernel generalist
www.suse.com<http://www.suse.com>
[https://www.suse.com/assets/img/social-platforms-suse-logo.png]<http://www.suse.com/>
SUSE - Open Source Solutions for Enterprise Servers & Cloud<http://www.suse.com/>
Modernize your infrastructure with SUSE Linux Enterprise servers, cloud technology for IaaS, and SUSE's software-defined storage.
www.suse.com
