Re: [PATCH 2 of 3] apic: remove 'enabled_via_apicbase' variable

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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?

> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> 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@lists.xensource.com
> http://lists.xensource.com/xen-devel