WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH 2 of 3] apic: remove 'enabled_via_apicbase' varia

To: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH 2 of 3] apic: remove 'enabled_via_apicbase' variable
From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Date: Wed, 18 May 2011 21:35:42 +0100
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Wed, 18 May 2011 13:39:12 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <20110518185339.GD14013@xxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <patchbomb.1305742093@andrewcoop> <e80b5280fe2fb8653204.1305742095@andrewcoop> <20110518185339.GD14013@xxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.17) Gecko/20110424 Lightning/1.0b2 Thunderbird/3.1.10


On 18/05/11 19:53, Konrad Rzeszutek Wilk wrote:
On Wed, May 18, 2011 at 07:08:15PM +0100, Andrew Cooper wrote:
The use of this varable was only sensible when the choice for lapic mode
variable
was between enabled or disabled.  Now that x2apic is about, it is wrong,
and causes a protection fault in certain cases when trying to tare down
tare?

x2apic mode.

The only place where its use is relevent in the code is in disable_local_APIC
                                           ^- is         ^^^ take that out.

which has been changed to correctly tare down the local APIC without a
teardown?
protection fault (which leads to a general protection fault).
So if you don't have x2apic, then it is wrong to disable the LAPIC mode?
What about older hardware?
I guess I wasn't very clear in my description. In older hardware without x2apic, it is correct to simply twiddle the ENABLE bit in the APICBASE MSR. However, with x2apic mode enabled, setting the ENABLE bit from 1 to 0 while leaving the EXTD bit set will result in a protection fault which will propagate to a general protection fault the same codepath will be called in the fault handler. As a result, the current code in disable_local_APIC will result in a GPF if the BIOS boots with LAPICs disabled (fine as per the spec for compatibility) and xen decided to take advantage of x2apic mode.
Signed-off-by: Andrew Cooper<andrew.cooper3@xxxxxxxxxx>

diff -r 62a8ce6595ad -r e80b5280fe2f xen/arch/x86/apic.c
--- a/xen/arch/x86/apic.c       Wed May 18 19:00:13 2011 +0100
+++ b/xen/arch/x86/apic.c       Wed May 18 19:00:13 2011 +0100
@@ -165,8 +165,6 @@ void __init apic_intr_init(void)
  /* Using APIC to generate smp_local_timer_interrupt? */
  static bool_t __read_mostly using_apic_timer;

-static bool_t __read_mostly enabled_via_apicbase;
-
  int get_physical_broadcast(void)
  {
      if (modern_apic())
@@ -330,6 +328,9 @@ void disconnect_bsp_APIC(int virt_wire_s

  void disable_local_APIC(void)
  {
+    u64 msr_contents;
+    enum apic_mode current_mode;
+
      clear_local_APIC();

      /*
@@ -339,10 +340,54 @@ void disable_local_APIC(void)
      apic_write_around(APIC_SPIV,
          apic_read(APIC_SPIV)&  ~APIC_SPIV_APIC_ENABLED);

-    if (enabled_via_apicbase) {
-        uint64_t msr_content;
-        rdmsrl(MSR_IA32_APICBASE, msr_content);
-        wrmsrl(MSR_IA32_APICBASE, msr_content&  ~MSR_IA32_APICBASE_ENABLE);
+    /* Work out what apic mode we are in */
+    rdmsrl(MSR_IA32_APICBASE, msr_contents);
+
+    /* Reading EXTD bit from the MSR is only valid if CPUID says so, else 
reserved */
+    if ( cpu_has(&current_cpu_data, X86_FEATURE_X2APIC)
+&&  (msr_contents&  MSR_IA32_APICBASE_EXTD) )
+        current_mode = APIC_MODE_X2APIC;
+    else
+        {
+            /* EN bit should always be valid as long as we can read the MSR
+             * Can anyone confirm this?
Might want to email Vivek Goyal..
+             */
+            if ( msr_contents&  MSR_IA32_APICBASE_ENABLE )
+                current_mode = APIC_MODE_XAPIC;
+            else
+                current_mode = APIC_MODE_DISABLED;
+        }
+
+    /* See what (if anything) we need to do to revert to boot mode */
+    if( current_mode != this_cpu(apic_boot_mode) )
+    {
+        rdmsrl(MSR_IA32_APICBASE, msr_contents);
+        msr_contents&= ~ ( MSR_IA32_APICBASE_ENABLE | MSR_IA32_APICBASE_EXTD );
+        wrmsrl(MSR_IA32_APICBASE, msr_contents);
+
+        switch(this_cpu(apic_boot_mode))
+        {
+        case APIC_MODE_DISABLED:
+            break; /* Nothing to do - we did this above */
+        case APIC_MODE_XAPIC:
+        {
+            msr_contents |= MSR_IA32_APICBASE_ENABLE;
+            wrmsrl(MSR_IA32_APICBASE, msr_contents);
+            break;
+        }
+        case APIC_MODE_X2APIC:
+        {
+            msr_contents |= ( MSR_IA32_APICBASE_ENABLE | 
MSR_IA32_APICBASE_EXTD );
+            wrmsrl(MSR_IA32_APICBASE, msr_contents);
+            break;
+        }
+        default:
+        {
+            printk("Hit default case when reverting lapic to boot state on core 
#%d\n",
+                   smp_processor_id());
+            break;
+        }
+        }
      }
  }

@@ -874,7 +919,6 @@ static int __init detect_init_APIC (void
              wrmsrl(MSR_IA32_APICBASE,
                  msr_content | MSR_IA32_APICBASE_ENABLE
                  | APIC_DEFAULT_PHYS_BASE);
-            enabled_via_apicbase = 1;
          }
      }
      /*

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

--
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel