[Xen-devel] [PATCH] x86/apic: Improve current_local_apic_mode()

[Xen-devel] [PATCH] x86/apic: Improve current_local_apic_mode()

[Andrew Cooper]

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(-)

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;
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/apic: Improve current_local_apic_mode()

[Wei Liu]

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>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/apic: Improve current_local_apic_mode()

[Jan Beulich]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/apic: Improve current_local_apic_mode()

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/apic: Improve current_local_apic_mode()

[Jan Beulich]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel