Re: [kvm-unit-tests PATCH] x86: Prevent APIC base address from changing in test_enable_x2apic()

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Mon, Apr 15, 2019 at 12:55:42PM -0700, Nadav Amit wrote:
> > On Apr 15, 2019, at 12:37 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> > 
> > On Mon, Apr 15, 2019 at 12:12:58PM -0700, Nadav Amit wrote:
> >>> On Apr 15, 2019, at 12:09 PM, Nadav Amit <nadav.amit@gmail.com> wrote:
> >>> 
> >>>> On Apr 15, 2019, at 12:00 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> >>>> 
> >>>> On Mon, Apr 15, 2019 at 11:31:00AM -0700, nadav.amit@gmail.com wrote:
> >>>>> From: Nadav Amit <nadav.amit@gmail.com>
> >>>>> 
> >>>>> test_enable_x2apic() unintentionally changes the APIC base address to
> >>>>> zero and resets the BSP indication. This actually causes the local APIC
> >>>>> to overlap the IDT area, which is wrong. Prevent it from happening by
> >>>>> keeping the APIC base address and BSP-bit as it was before.
> >>>>> 
> >>>>> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
> >>>>> ---
> >>>>> x86/apic.c | 4 ++--
> >>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>> 
> >>>>> diff --git a/x86/apic.c b/x86/apic.c
> >>>>> index 51744cf..0849f87 100644
> >>>>> --- a/x86/apic.c
> >>>>> +++ b/x86/apic.c
> >>>>> @@ -90,11 +90,11 @@ static void test_enable_x2apic(void)
> >>>>>       report("disabled to x2apic enabled",
> >>>>>              test_write_apicbase_exception(APIC_EN | APIC_EXTD));
> >>>>> 
> >>>>> -        wrmsr(MSR_IA32_APICBASE, APIC_EN);
> >>>>> +        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_DEFAULT_PHYS_BASE | APIC_BSP);
> >>>>>       report("apic enabled to invalid state",
> >>>>>              test_write_apicbase_exception(APIC_EXTD));
> >>>>> 
> >>>>> -        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD);
> >>>>> +        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD | APIC_DEFAULT_PHYS_BASE | APIC_BSP);
> >>>> 
> >>>> It probably doesn't matter since AFAIK kvm-unit-tests always uses the
> >>>> default base, but preserving the current base+BSP would be preferred.
> >>>> The #GP tests get away with using APIC_DEFAULT_PHYS_BASE because the
> >>>> WRMSR will never succeed, but even that is poor form.  And the test
> >>>> should also reset to xAPIC when it's done.
> >>>> 
> >>>> I think the attached patch covers everything.
> >>> 
> >>> Thanks. The base is indeed the default, so it should not matter, but your
> >>> change should make the code more robust.
> >>> 
> >>> One important comment regarding your patch:
> >>> 
> >>>> +        if (!(orig_apicbase & APIC_EXTD))
> >>>> +            reset_apic();
> >>> 
> >>> This is not needed, since enable_x2apic() will only return true if it
> >>> succeeded in enabling x2apic. In addition, this is wrong, since reset_apic()
> >>> should (usually, and specifically in this case) be followed with:
> >>> 
> >>> 	apic_write(APIC_SPIV, 0x1ff);
> >>> 
> >>> And not the other way around (there are actually a couple of missing writes
> >>> to software-enable the APIC after reset_apic(), which I’ll send later).
> >> 
> >> Correcting myself - I understand (now) that you want to reset back to xapic, so
> >> that’s fine. Just keep "apic_write(APIC_SPIV, 0x1ff);” after the reset, please.
> > 
> > Ah, will do, I glazed over the apic_write() and didn't consider its purpose.
> 
> Sorry for not being consistent, but actually the apic_reset() seems wrong in
> general.
> 
> enable_x2apic() is used in the context of test_enable_x2apic() to figure out
> whether x2apic was enabled before. It is actually initially enabled after
> boot (see cstart64.S). So disabling it here would not be appropriate.

I more or less noticed the same thing.  Actually, I noticed we end up in
legacy xAPIC and so ended up with:

        if (orig_apicbase & APIC_EXTD)
            enable_x2apic();
        else
            reset_apic();

Along with a blurb in the changelog stating that reset_apic() is overkill
since the vCPU is already in xAPIC.