Re: [PATCH 6 of 7] IOMMU: add crash_shutdown iommu_op

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

On 14/06/11 13:10, Keir Fraser wrote:
> On 13/06/2011 18:02, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:
>
>> The kdump kernel has problems booting with interrupt/dma
>> remapping enabled, so we need a new iommu_ops called
>> crash_shutdown which is basically suspend but doesn't
>> need to bother saving state.
>>
>> Make sure that crash_shutdown is called on the kexec
>> path.
> What's wrong with the existing .suspend iommu callbacks? They look very
> similar (identical in the AMD case in fact). The extra
> iommu_disable_x2apic_IR() could be avoided in favour of the eisting one in
> the lapic_suspend() path, perhaps?
>
>  -- Keir
>

For the Intel case, we don't care whether the user has forced iommu on
the comandline - we still need to disable it.  Also, saving state is
pointless in the case of a crash.  It also gives us a clean way to  set
iommu_enabled = 0; which prevents unhelpful panics later on when
disable_IO_APICs times out because it is polling the wrong register.

For AMD, that is simply because AMD doesn't actually save state.  (I
assume that the state is retained in the hardware, but I don't have any
AMD boxes I can test with)

>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> diff -r 2635774346c4 -r 3ad737eb0a8d xen/arch/x86/crash.c
>> --- a/xen/arch/x86/crash.c Mon Jun 13 17:45:43 2011 +0100
>> +++ b/xen/arch/x86/crash.c Mon Jun 13 17:45:43 2011 +0100
>> @@ -77,6 +77,10 @@ static void nmi_shootdown_cpus(void)
>>          msecs--;
>>      }
>>  
>> +    /* Crash shutdown any IOMMU functionality as the crashdump kernel is not
>> +     * happy when booting if interrupt/dma remapping is still enabled */
>> +    iommu_crash_shutdown();
>> +
>>      __stop_this_cpu();
>>      disable_IO_APIC();
>>  
>> diff -r 2635774346c4 -r 3ad737eb0a8d xen/drivers/passthrough/amd/iommu_init.c
>> --- a/xen/drivers/passthrough/amd/iommu_init.c Mon Jun 13 17:45:43 2011 +0100
>> +++ b/xen/drivers/passthrough/amd/iommu_init.c Mon Jun 13 17:45:43 2011 +0100
>> @@ -921,6 +921,14 @@ void amd_iommu_suspend(void)
>>          disable_iommu(iommu);
>>  }
>>  
>> +void amd_iommu_crash_shutdown(void)
>> +{
>> +    struct amd_iommu *iommu;
>> +
>> +    for_each_amd_iommu ( iommu )
>> +        disable_iommu(iommu);
>> +}
>> +
>>  void amd_iommu_resume(void)
>>  {
>>      struct amd_iommu *iommu;
>> diff -r 2635774346c4 -r 3ad737eb0a8d
>> xen/drivers/passthrough/amd/pci_amd_iommu.c
>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c Mon Jun 13 17:45:43 2011
>> +0100
>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c Mon Jun 13 17:45:43 2011
>> +0100
>> @@ -459,4 +459,5 @@ const struct iommu_ops amd_iommu_ops = {
>>      .suspend = amd_iommu_suspend,
>>      .resume = amd_iommu_resume,
>>      .share_p2m = amd_iommu_share_p2m,
>> +    .crash_shutdown = amd_iommu_crash_shutdown,
>>  };
>> diff -r 2635774346c4 -r 3ad737eb0a8d xen/drivers/passthrough/iommu.c
>> --- a/xen/drivers/passthrough/iommu.c Mon Jun 13 17:45:43 2011 +0100
>> +++ b/xen/drivers/passthrough/iommu.c Mon Jun 13 17:45:43 2011 +0100
>> @@ -429,6 +429,14 @@ void iommu_share_p2m_table(struct domain
>>          ops->share_p2m(d);
>>  }
>>  
>> +void iommu_crash_shutdown(void)
>> +{
>> +    const struct iommu_ops *ops = iommu_get_ops();
>> +    if ( ops && ops->crash_shutdown )
>> +        ops->crash_shutdown();
>> +    iommu_enabled = 0;
>> +}
>> +
>>  /*
>>   * Local variables:
>>   * mode: C
>> diff -r 2635774346c4 -r 3ad737eb0a8d xen/drivers/passthrough/vtd/iommu.c
>> --- a/xen/drivers/passthrough/vtd/iommu.c Mon Jun 13 17:45:43 2011 +0100
>> +++ b/xen/drivers/passthrough/vtd/iommu.c Mon Jun 13 17:45:43 2011 +0100
>> @@ -2238,6 +2238,25 @@ static void vtd_suspend(void)
>>      }
>>  }
>>  
>> +static void vtd_crash_shutdown(void)
>> +{
>> +    struct acpi_drhd_unit *drhd;
>> +    struct iommu *iommu;
>> +
>> +    if ( !iommu_enabled )
>> +        return;
>> +
>> +    iommu_flush_all();
>> +
>> +    for_each_drhd_unit ( drhd )
>> +    {
>> +        iommu = drhd->iommu;
>> +        iommu_disable_translation(iommu);
>> +    }
>> +
>> +    iommu_disable_x2apic_IR();
>> +}
>> +
>>  static void vtd_resume(void)
>>  {
>>      struct acpi_drhd_unit *drhd;
>> @@ -2289,6 +2308,7 @@ const struct iommu_ops intel_iommu_ops =
>>      .suspend = vtd_suspend,
>>      .resume = vtd_resume,
>>      .share_p2m = iommu_set_pgd,
>> +    .crash_shutdown = vtd_crash_shutdown,
>>  };
>>  
>>  /*
>> diff -r 2635774346c4 -r 3ad737eb0a8d
>> xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
>> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h Mon Jun 13 17:45:43 2011
>> +0100
>> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h Mon Jun 13 17:45:43 2011
>> +0100
>> @@ -98,6 +98,7 @@ unsigned int amd_iommu_read_ioapic_from_
>>  /* power management support */
>>  void amd_iommu_resume(void);
>>  void amd_iommu_suspend(void);
>> +void amd_iommu_crash_shutdown(void);
>>  
>>  static inline u32 get_field_from_reg_u32(u32 reg_value, u32 mask, u32 shift)
>>  {
>> diff -r 2635774346c4 -r 3ad737eb0a8d xen/include/xen/iommu.h
>> --- a/xen/include/xen/iommu.h Mon Jun 13 17:45:43 2011 +0100
>> +++ b/xen/include/xen/iommu.h Mon Jun 13 17:45:43 2011 +0100
>> @@ -133,6 +133,7 @@ struct iommu_ops {
>>      void (*suspend)(void);
>>      void (*resume)(void);
>>      void (*share_p2m)(struct domain *d);
>> +    void (*crash_shutdown)(void);
>>  };
>>  
>>  void iommu_update_ire_from_apic(unsigned int apic, unsigned int reg, unsigned
>> int value);
>> @@ -142,6 +143,7 @@ unsigned int iommu_read_apic_from_ire(un
>>  
>>  void iommu_suspend(void);
>>  void iommu_resume(void);
>> +void iommu_crash_shutdown(void);
>>  
>>  void iommu_set_dom0_mapping(struct domain *d);
>>  void iommu_share_p2m_table(struct domain *d);
>>
>> _______________________________________________
>> 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