| Message ID | 20190415183100.6827-1-nadav.amit@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] x86: Prevent APIC base address from changing in test_enable_x2apic() | expand |
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. > apic_write(APIC_SPIV, 0x1ff); > } else { > printf("x2apic not detected\n"); > -- > 2.17.1 > From 0905302c23f20f67483b281e33f1930135087e3a Mon Sep 17 00:00:00 2001 From: Sean Christopherson <sean.j.christopherson@intel.com> Date: Mon, 15 Apr 2019 11:53:00 -0700 Subject: [PATCH] x86: apic: Preserve APIC base and BSP bits during x2APIC tests ...and reset to xAPIC mode unless x2APIC was already enabled. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- x86/apic.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/x86/apic.c b/x86/apic.c index 51744cf..02b6128 100644 --- a/x86/apic.c +++ b/x86/apic.c @@ -69,33 +69,39 @@ static void do_write_apicbase(void *data) static bool test_write_apicbase_exception(u64 data) { - data |= APIC_DEFAULT_PHYS_BASE | APIC_BSP; return test_for_exception(GP_VECTOR, do_write_apicbase, &data); } static void test_enable_x2apic(void) { + u64 orig_apicbase = rdmsr(MSR_IA32_APICBASE); + u64 apicbase; + if (enable_x2apic()) { printf("x2apic enabled\n"); + apicbase = orig_apicbase & ~(APIC_EN | APIC_EXTD); report("x2apic enabled to invalid state", - test_write_apicbase_exception(APIC_EXTD)); + test_write_apicbase_exception(apicbase | APIC_EXTD)); report("x2apic enabled to apic enabled", - test_write_apicbase_exception(APIC_EN)); + test_write_apicbase_exception(apicbase | APIC_EN)); report("x2apic enabled to disabled state", - !test_write_apicbase_exception(0)); + !test_write_apicbase_exception(apicbase | 0)); report("disabled to invalid state", - test_write_apicbase_exception(APIC_EXTD)); + test_write_apicbase_exception(apicbase | APIC_EXTD)); report("disabled to x2apic enabled", - test_write_apicbase_exception(APIC_EN | APIC_EXTD)); + test_write_apicbase_exception(apicbase | APIC_EN | APIC_EXTD)); - wrmsr(MSR_IA32_APICBASE, APIC_EN); + wrmsr(MSR_IA32_APICBASE, apicbase | APIC_EN); report("apic enabled to invalid state", - test_write_apicbase_exception(APIC_EXTD)); + test_write_apicbase_exception(apicbase | APIC_EXTD)); - wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD); + wrmsr(MSR_IA32_APICBASE, apicbase | APIC_EN | APIC_EXTD); apic_write(APIC_SPIV, 0x1ff); + + if (!(orig_apicbase & APIC_EXTD)) + reset_apic(); } else { printf("x2apic not detected\n");
> 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).
> 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.
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.
> 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.
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.
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); apic_write(APIC_SPIV, 0x1ff); } else { printf("x2apic not detected\n");