| Message ID | 20200128125216.709-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/apic: Improve current_local_apic_mode() | expand |
On Tue, Jan 28, 2020 at 12:52:16PM +0000, Andrew Cooper wrote: > boot_cpu_has(X86_FEATURE_X2APIC) doesn't need checking to interpret > APIC_BASE_EXTD. > > Also take the opportunity to optimise the generated assembly by not using > rdmsrl(). GCC isn't clever enough to spot that it can drop the shift and or > to put %eax in the higher half of msr_contents. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Wei Liu <wl@xen.org>
On 28.01.2020 13:52, Andrew Cooper wrote: > boot_cpu_has(X86_FEATURE_X2APIC) doesn't need checking to interpret > APIC_BASE_EXTD. Hmm, the comment you remove ... > --- a/xen/arch/x86/apic.c > +++ b/xen/arch/x86/apic.c > @@ -1534,18 +1534,14 @@ void __init record_boot_APIC_mode(void) > /* Look at the bits in MSR_APIC_BASE and work out which APIC mode we are in */ > enum apic_mode current_local_apic_mode(void) > { > - u64 msr_contents; > + uint32_t high, low; > > - rdmsrl(MSR_APIC_BASE, msr_contents); > + rdmsr(MSR_APIC_BASE, low, high); > > - /* Reading EXTD bit from the MSR is only valid if CPUID > - * says so, else reserved */ ... states the situation correctly, I think. I guess there's no hardware allowing the bit to be set without the feature being there, but a virtual or emulated environment could go and set the bit without violating any specification, as long as the CPUID bit is clear. (Afaict we still allow PV guests to see the host MSR_APIC_BASE contents, yet such guests wouldn't see the CPUID flag set. We've had a customer inferring from the set bit in the MSR that the other x2APIC [host] MSRs can also be read. They wouldn't have run into their issue if they had followed its "reserved" semantics in such a case.) I guess I'm missing some further aspect here. Jan
On 28/01/2020 14:10, Jan Beulich wrote: > On 28.01.2020 13:52, Andrew Cooper wrote: >> boot_cpu_has(X86_FEATURE_X2APIC) doesn't need checking to interpret >> APIC_BASE_EXTD. > Hmm, the comment you remove ... > >> --- a/xen/arch/x86/apic.c >> +++ b/xen/arch/x86/apic.c >> @@ -1534,18 +1534,14 @@ void __init record_boot_APIC_mode(void) >> /* Look at the bits in MSR_APIC_BASE and work out which APIC mode we are in */ >> enum apic_mode current_local_apic_mode(void) >> { >> - u64 msr_contents; >> + uint32_t high, low; >> >> - rdmsrl(MSR_APIC_BASE, msr_contents); >> + rdmsr(MSR_APIC_BASE, low, high); >> >> - /* Reading EXTD bit from the MSR is only valid if CPUID >> - * says so, else reserved */ > ... states the situation correctly, I think. I guess there's no hardware > allowing the bit to be set without the feature being there, but a virtual > or emulated environment could go and set the bit without violating any > specification, as long as the CPUID bit is clear. It is unrealistic to expect that some emulated environment supports preserving of a reserved bit when real hardware uses #GP. > (Afaict we still allow > PV guests to see the host MSR_APIC_BASE contents, yet such guests > wouldn't see the CPUID flag set. I tried an experiment a few years ago to properly reject access to MSR_APIC_BASE for PV guests. Suffice it to say that Linux doesn't boot. This is ultimately a bug in Linux, stemming from broken MSR handling in Xen, and inappropriate leakage of state which shouldn't ever have been available to PV guests. PV guests cannot interact with the LAPIC at all. Their only interrupt controller is the event channel interface. > We've had a customer inferring from the > set bit in the MSR that the other x2APIC [host] MSRs can also be read. Right, but that is bugs stacked on top of bugs. Its not surprising there is a cascade set of failures. Also remember that PV guests get to see the host's x2apic setting, seemingly for topology reasons. I'm not convinced this is actually a clever idea, but given that noone has actually fixed guest topology handling yet, I've also gone out of my way not to make changes in this area while adjusting other aspects of CPUID handling. ~Andrew
On 14.02.2020 18:59, Andrew Cooper wrote: > On 28/01/2020 14:10, Jan Beulich wrote: >> On 28.01.2020 13:52, Andrew Cooper wrote: >>> boot_cpu_has(X86_FEATURE_X2APIC) doesn't need checking to interpret >>> APIC_BASE_EXTD. >> Hmm, the comment you remove ... >> >>> --- a/xen/arch/x86/apic.c >>> +++ b/xen/arch/x86/apic.c >>> @@ -1534,18 +1534,14 @@ void __init record_boot_APIC_mode(void) >>> /* Look at the bits in MSR_APIC_BASE and work out which APIC mode we are in */ >>> enum apic_mode current_local_apic_mode(void) >>> { >>> - u64 msr_contents; >>> + uint32_t high, low; >>> >>> - rdmsrl(MSR_APIC_BASE, msr_contents); >>> + rdmsr(MSR_APIC_BASE, low, high); >>> >>> - /* Reading EXTD bit from the MSR is only valid if CPUID >>> - * says so, else reserved */ >> ... states the situation correctly, I think. I guess there's no hardware >> allowing the bit to be set without the feature being there, but a virtual >> or emulated environment could go and set the bit without violating any >> specification, as long as the CPUID bit is clear. > > It is unrealistic to expect that some emulated environment supports > preserving of a reserved bit when real hardware uses #GP. However unrealistic it may be, don't you agree it to be best in questionable cases if we stay as closely to the spec as possible? Also I intentionally didn't talk about the bit being preserved, but the bit perhaps be uniformly set. Simplistic x2APIC virtualization could always set this bit (without violating the spec), relying on consumers to actually inspect the CPUID bit first. One less conditional wherever the value of the MSR gets calculated. Jan
diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c index a6a7754d77..0684c5d9c2 100644 --- a/xen/arch/x86/apic.c +++ b/xen/arch/x86/apic.c @@ -1534,18 +1534,14 @@ void __init record_boot_APIC_mode(void) /* Look at the bits in MSR_APIC_BASE and work out which APIC mode we are in */ enum apic_mode current_local_apic_mode(void) { - u64 msr_contents; + uint32_t high, low; - rdmsrl(MSR_APIC_BASE, msr_contents); + rdmsr(MSR_APIC_BASE, low, high); - /* Reading EXTD bit from the MSR is only valid if CPUID - * says so, else reserved */ - if ( boot_cpu_has(X86_FEATURE_X2APIC) && (msr_contents & APIC_BASE_EXTD) ) + if ( low & APIC_BASE_EXTD ) return APIC_MODE_X2APIC; - /* EN bit should always be valid as long as we can read the MSR - */ - if ( msr_contents & APIC_BASE_ENABLE ) + if ( low & APIC_BASE_ENABLE ) return APIC_MODE_XAPIC; return APIC_MODE_DISABLED;
boot_cpu_has(X86_FEATURE_X2APIC) doesn't need checking to interpret APIC_BASE_EXTD. Also take the opportunity to optimise the generated assembly by not using rdmsrl(). GCC isn't clever enough to spot that it can drop the shift and or to put %eax in the higher half of msr_contents. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> What can I say - the numpty who wrote that code was young and naive... --- xen/arch/x86/apic.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)