Re: [PATCH] kexec: disable iommu and x2apic before jumping into the kdump kernel

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 13/05/11 15:22, Jan Beulich wrote:
>>>> On 13.05.11 at 14:47, Andrew Cooper<andrew.cooper3@citrix.com>  wrote:
>> This is a massive hack but is better than the code currently in place.
>> Some functions have been duped into a crash_* version to separate them
>> from the regular shutdown path, where disabling these subsystems is
>> irrelevant.
> Hmm, I'd rather not see this go in (in this form). A few comment below,
> but even with them addressed I'm not going to be in favor of it.
>
> Also did you compare all of this with the way Linux deals with the same
> situation?
Linux 2.6.32 which I was comparing against successfully reverts the 
lapic to boot state, but has no clue about iommu so will never enable it 
in the first place.  There is currently not enough information in apic.c 
to remember the boot state - the hack for xapic boot state is poor to 
say the least.  I will rejig this to properly record boot state in all 
cases for local apics.
>> Signed-off-by: Andrew Cooper<andrew.cooper3@citrix.com>
>>
>> diff -r 0c446850d85e -r efa733ebd00d xen/arch/x86/apic.c
>> --- a/xen/arch/x86/apic.c	Wed May 11 12:58:04 2011 +0100
>> +++ b/xen/arch/x86/apic.c	Fri May 13 13:45:05 2011 +0100
>> @@ -344,6 +344,42 @@
>>       }
>>   }
>>
>> +/* Customised version of disable_local_APIC which is only called on the kexec crash path.
>> + * It is a brutal hack and only here untill the APIC code is overhauled.  We NEED to swtich
>> + * out of x2apic mode whatever the case as the kdump kernel cant cope due to the MMIO<->  MSR
>> + * swap invovled with x2apic mode.
> Unless the system booted in x2apic mode. Otherwise you'll confuse
> the secondary kernel even more.
>
>> + */
>> +void crash_disable_local_APIC(void)
>> +{
>> +    uint64_t msr_content;
>> +    clear_local_APIC();
>> +
>> +    apic_write_around(APIC_SPIV,
>> +        apic_read(APIC_SPIV)&  ~APIC_SPIV_APIC_ENABLED);
>> +
>> +    rdmsrl(MSR_IA32_APICBASE, msr_content);
>> +
>> +    /* This is the only way to exit x2apic mode.  Trying to disable x2apic mode while staying
>> +     * xapic mode will result in a protection fault which leads to a general protection fault
>> +     * as this is on the fault handler codepath.
>> +     */
>> +    msr_content&= ~(MSR_IA32_APICBASE_ENABLE|MSR_IA32_APICBASE_EXTD);
>> +    wrmsrl(MSR_IA32_APICBASE, msr_content);
>> +
>> +    /* In most cases, we should leave the lapic in xapic mode as this is the default state on
>> +     * CPU reset.  However, some bioses specifically disable xapic mode for hilarity sake so
>> +     * we should follow suit so the kdump kernel does not get confused about mismatching ACPI
>> +     * tables.
>> +     */
>> +    if ( ! enabled_via_apicbase) {
>> +	    /* If we did not manually enable xapic mode, I.E. the bios left it enabled,
>> +	     * lets follow suit and leave it enabled for the kdump kernel.
>> +	     */
>> +        wrmsrl(MSR_IA32_APICBASE, msr_content | MSR_IA32_APICBASE_ENABLE);
>> +    }
>> +}
>> +
>> +
>>   /*
>>    * This is to verify that we're looking at a real local APIC.
>>    * Check these against your board if the CPUs aren't getting
>> diff -r 0c446850d85e -r efa733ebd00d xen/arch/x86/crash.c
>> --- a/xen/arch/x86/crash.c	Wed May 11 12:58:04 2011 +0100
>> +++ b/xen/arch/x86/crash.c	Fri May 13 13:45:05 2011 +0100
>> @@ -27,6 +27,8 @@
>>   #include<asm/hvm/support.h>
>>   #include<asm/apic.h>
>>   #include<asm/io_apic.h>
>> +#include<xen/iommu.h>
>> +#include<asm/hvm/iommu.h>
>>
>>   static atomic_t waiting_for_crash_ipi;
>>   static unsigned int crashing_cpu;
>> @@ -43,7 +45,12 @@
>>
>>       kexec_crash_save_cpu();
>>
>> -    __stop_this_cpu();
>> +    crash_disable_local_APIC();
>> +    hvm_cpu_down();
>> +    /* Clear FPU, zapping any pending exceptions. Needed for warm reset with
>> +     * some BIOSes.*/
>> +    clts();
>> +    asm volatile ( "fninit" );
>>
>>       atomic_dec(&waiting_for_crash_ipi);
>>
>> @@ -77,7 +84,15 @@
>>           msecs--;
>>       }
>>
>> -    __stop_this_cpu();
>> +    crash_disable_local_APIC();
>> +    hvm_cpu_down();
>> +    /* Clear FPU, zapping any pending exceptions. Needed for warm reset with
>> +     * some BIOSes.*/
>> +    clts();
>> +    asm volatile ( "fninit" );
>> +
>> +    x2apic_enabled = 0;
> Why? And if indeed needed here, why not above? (Your patch wasn't
> generated with -p, so it's a little difficult to understand what functions
> you modify here.)
>
>> +
>>       disable_IO_APIC();
>>
>>       local_irq_enable();
>> @@ -86,9 +101,24 @@
>>   void machine_crash_shutdown(void)
>>   {
>>       crash_xen_info_t *info;
>> +    const struct iommu_ops * ops;
>>
>>       nmi_shootdown_cpus();
>>
>> +    local_irq_disable();
> local_irq_save()
Fair enough in the general case but in this case it really doesnt 
matter.  Having said that, there are other areas on the crash/reboot 
path which need updating with this logic
>> +
>> +    /* Yes i know this is hacky but it is the easiest solution.  I should add an iommu_ops
>> +     * function called crash() or so which just disables the iommu 'fun' without saving state
>> +     */
>> +    ops = iommu_get_ops();
>      if ( ops )
>
>> +    ops->suspend();
>> +
>> +    /* Yes i know this is from driver/passthrough/vtd/ but it appears to be architecture
>> +     * independant, and also bears little/no relation to x2apic.  Needs cleaning up
>> +     */
>> +    iommu_disable_x2apic_IR();
>> +    local_irq_enable();
> local_irq_restore() - you absolutely want to avoid turning on
> interrupts if they weren't on entry to the function.
>
>> +
>>       info = kexec_crash_save_info();
>>       info->xen_phys_start = xen_phys_start;
>>       info->dom0_pfn_to_mfn_frame_list_list =
>> diff -r 0c446850d85e -r efa733ebd00d xen/arch/x86/hpet.c
>> --- a/xen/arch/x86/hpet.c	Wed May 11 12:58:04 2011 +0100
>> +++ b/xen/arch/x86/hpet.c	Fri May 13 13:45:05 2011 +0100
>> @@ -670,6 +670,34 @@
>>       smp_send_event_check_mask(&cpu_online_map);
>>   }
>>
>> +void crash_hpet_disable_legacy_broadcast(void)
>> +{
>> +    u32 cfg;
>> +
>> +    if ( !hpet_events || !(hpet_events->flags&  HPET_EVT_LEGACY) )
>> +        return;
>> +
>> +    /* By this point in the crash handler, we are the only CPU still going,
>> +     * so the lock is irrelevant
>> +     */
>> +    /* spin_lock_irqsave(&hpet_events->lock, flags); */
>> +
>> +    hpet_events->flags |= HPET_EVT_DISABLE;
>> +
>> +    /* disable HPET T0 */
>> +    cfg = hpet_read32(HPET_Tn_CFG(0));
>> +    cfg&= ~HPET_TN_ENABLE;
>> +    hpet_write32(cfg, HPET_Tn_CFG(0));
>> +
>> +    /* Stop HPET legacy interrupts */
>> +    cfg = hpet_read32(HPET_CFG);
>> +    cfg&= ~HPET_CFG_LEGACY;
>> +    hpet_write32(cfg, HPET_CFG);
>> +
>> +    /* spin_unlock_irqrestore(&hpet_events->lock, flags); */
>> +}
> This looks to be the same except for the skipping of the locks.
> Calls for adding a parameter to the original function.
>
> Jan
This function skips the IPI to all other processors, because at this 
point in the code, all other processors have been shot down.  I will 
comment that detail
>> +
>> +
>>   void hpet_broadcast_enter(void)
>>   {
>>       unsigned int cpu = smp_processor_id();
>> diff -r 0c446850d85e -r efa733ebd00d xen/arch/x86/machine_kexec.c
>> --- a/xen/arch/x86/machine_kexec.c	Wed May 11 12:58:04 2011 +0100
>> +++ b/xen/arch/x86/machine_kexec.c	Fri May 13 13:45:05 2011 +0100
>> @@ -97,7 +97,7 @@
>>       };
>>
>>       if ( hpet_broadcast_is_available() )
>> -        hpet_disable_legacy_broadcast();
>> +        crash_hpet_disable_legacy_broadcast();
>>
>>       /*
>>        * compat_machine_kexec() returns to idle pagetables, which requires us
>> diff -r 0c446850d85e -r efa733ebd00d xen/include/asm-x86/apic.h
>> --- a/xen/include/asm-x86/apic.h	Wed May 11 12:58:04 2011 +0100
>> +++ b/xen/include/asm-x86/apic.h	Fri May 13 13:45:05 2011 +0100
>> @@ -182,6 +182,7 @@
>>   extern void connect_bsp_APIC (void);
>>   extern void disconnect_bsp_APIC (int virt_wire_setup);
>>   extern void disable_local_APIC (void);
>> +extern void crash_disable_local_APIC (void);
>>   extern int verify_local_APIC (void);
>>   extern void cache_APIC_registers (void);
>>   extern void sync_Arb_IDs (void);
>> diff -r 0c446850d85e -r efa733ebd00d xen/include/asm-x86/hpet.h
>> --- a/xen/include/asm-x86/hpet.h	Wed May 11 12:58:04 2011 +0100
>> +++ b/xen/include/asm-x86/hpet.h	Fri May 13 13:45:05 2011 +0100
>> @@ -73,5 +73,6 @@
>>   void hpet_broadcast_exit(void);
>>   int hpet_broadcast_is_available(void);
>>   void hpet_disable_legacy_broadcast(void);
>> +void crash_hpet_disable_legacy_broadcast(void);
>>
>>   #endif /* __X86_HPET_H__ */
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>

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