[PATCH 0/2] VT-d: x2APIC + IR adjustments

[PATCH 0/2] VT-d: x2APIC + IR adjustments

[Jan Beulich]

1: use proper error codes in iommu_enable_x2apic_IR()
2: section placement and type adjustments

Signed-off-by: Jan Beulich <jbeulich@suse.com>

[PATCH 1/2] VT-d: use proper error codes in iommu_enable_x2apic_IR()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3465 bytes --]

... allowing to suppress a confusing messeage combination: When
ACPI_DMAR_X2APIC_OPT_OUT is set, so far we first logged a message
that IR could not be enabled (hence not using x2APIC), followed by
one indicating successful initialization of IR (if no other problems
prevented that).

Also adjust the return type of iommu_supports_eim() and fix some
broken indentation in the function.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -943,8 +943,18 @@ void __init x2apic_bsp_setup(void)
     mask_8259A();
     mask_IO_APIC_setup(ioapic_entries);
 
-    if ( iommu_enable_x2apic_IR() )
+    switch ( iommu_enable_x2apic_IR() )
     {
+    case 0:
+        break;
+    case -ENXIO: /* ACPI_DMAR_X2APIC_OPT_OUT set */
+        if ( !x2apic_enabled )
+        {
+            printk("Not enabling x2APIC (upon firmware request)\n");
+            goto restore_out;
+        }
+        /* fall through */
+    default:
         if ( x2apic_enabled )
             panic("Interrupt remapping could not be enabled while "
                   "x2APIC is already enabled by BIOS");
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -143,10 +143,10 @@ static void set_hpet_source_id(unsigned 
     set_ire_sid(ire, SVT_VERIFY_SID_SQ, SQ_13_IGNORE_3, hpetid_to_bdf(id));
 }
 
-int iommu_supports_eim(void)
+bool_t iommu_supports_eim(void)
 {
     struct acpi_drhd_unit *drhd;
-    int apic;
+    unsigned int apic;
 
     if ( !iommu_qinval || !iommu_intremap || list_empty(&acpi_drhd_units) )
         return 0;
@@ -154,12 +154,12 @@ int iommu_supports_eim(void)
     /* We MUST have a DRHD unit for each IOAPIC. */
     for ( apic = 0; apic < nr_ioapics; apic++ )
         if ( !ioapic_to_drhd(IO_APIC_ID(apic)) )
-    {
+        {
             dprintk(XENLOG_WARNING VTDPREFIX,
                     "There is not a DRHD for IOAPIC %#x (id: %#x)!\n",
                     apic, IO_APIC_ID(apic));
             return 0;
-    }
+        }
 
     for_each_drhd_unit ( drhd )
         if ( !ecap_queued_inval(drhd->iommu->ecap) ||
@@ -833,10 +833,10 @@ int iommu_enable_x2apic_IR(void)
     struct iommu *iommu;
 
     if ( !iommu_supports_eim() )
-        return -1;
+        return -EOPNOTSUPP;
 
     if ( !platform_supports_x2apic() )
-        return -1;
+        return -ENXIO;
 
     for_each_drhd_unit ( drhd )
     {
@@ -861,7 +861,7 @@ int iommu_enable_x2apic_IR(void)
         {
             dprintk(XENLOG_INFO VTDPREFIX,
                     "Failed to enable Queued Invalidation!\n");
-            return -1;
+            return -EIO;
         }
     }
 
@@ -873,7 +873,7 @@ int iommu_enable_x2apic_IR(void)
         {
             dprintk(XENLOG_INFO VTDPREFIX,
                     "Failed to enable Interrupt Remapping!\n");
-            return -1;
+            return -EIO;
         }
     }
 
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -27,7 +27,7 @@ int iommu_setup_hpet_msi(struct msi_desc
 /* While VT-d specific, this must get declared in a generic header. */
 int adjust_vtd_irq_affinities(void);
 void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int order, int present);
-int iommu_supports_eim(void);
+bool_t iommu_supports_eim(void);
 int iommu_enable_x2apic_IR(void);
 void iommu_disable_x2apic_IR(void);
 




[-- Attachment #2: x86-enable-x2APIC-IR-error-codes.patch --]
[-- Type: text/plain, Size: 3519 bytes --]

VT-d: use proper error codes in iommu_enable_x2apic_IR()

... allowing to suppress a confusing messeage combination: When
ACPI_DMAR_X2APIC_OPT_OUT is set, so far we first logged a message
that IR could not be enabled (hence not using x2APIC), followed by
one indicating successful initialization of IR (if no other problems
prevented that).

Also adjust the return type of iommu_supports_eim() and fix some
broken indentation in the function.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -943,8 +943,18 @@ void __init x2apic_bsp_setup(void)
     mask_8259A();
     mask_IO_APIC_setup(ioapic_entries);
 
-    if ( iommu_enable_x2apic_IR() )
+    switch ( iommu_enable_x2apic_IR() )
     {
+    case 0:
+        break;
+    case -ENXIO: /* ACPI_DMAR_X2APIC_OPT_OUT set */
+        if ( !x2apic_enabled )
+        {
+            printk("Not enabling x2APIC (upon firmware request)\n");
+            goto restore_out;
+        }
+        /* fall through */
+    default:
         if ( x2apic_enabled )
             panic("Interrupt remapping could not be enabled while "
                   "x2APIC is already enabled by BIOS");
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -143,10 +143,10 @@ static void set_hpet_source_id(unsigned 
     set_ire_sid(ire, SVT_VERIFY_SID_SQ, SQ_13_IGNORE_3, hpetid_to_bdf(id));
 }
 
-int iommu_supports_eim(void)
+bool_t iommu_supports_eim(void)
 {
     struct acpi_drhd_unit *drhd;
-    int apic;
+    unsigned int apic;
 
     if ( !iommu_qinval || !iommu_intremap || list_empty(&acpi_drhd_units) )
         return 0;
@@ -154,12 +154,12 @@ int iommu_supports_eim(void)
     /* We MUST have a DRHD unit for each IOAPIC. */
     for ( apic = 0; apic < nr_ioapics; apic++ )
         if ( !ioapic_to_drhd(IO_APIC_ID(apic)) )
-    {
+        {
             dprintk(XENLOG_WARNING VTDPREFIX,
                     "There is not a DRHD for IOAPIC %#x (id: %#x)!\n",
                     apic, IO_APIC_ID(apic));
             return 0;
-    }
+        }
 
     for_each_drhd_unit ( drhd )
         if ( !ecap_queued_inval(drhd->iommu->ecap) ||
@@ -833,10 +833,10 @@ int iommu_enable_x2apic_IR(void)
     struct iommu *iommu;
 
     if ( !iommu_supports_eim() )
-        return -1;
+        return -EOPNOTSUPP;
 
     if ( !platform_supports_x2apic() )
-        return -1;
+        return -ENXIO;
 
     for_each_drhd_unit ( drhd )
     {
@@ -861,7 +861,7 @@ int iommu_enable_x2apic_IR(void)
         {
             dprintk(XENLOG_INFO VTDPREFIX,
                     "Failed to enable Queued Invalidation!\n");
-            return -1;
+            return -EIO;
         }
     }
 
@@ -873,7 +873,7 @@ int iommu_enable_x2apic_IR(void)
         {
             dprintk(XENLOG_INFO VTDPREFIX,
                     "Failed to enable Interrupt Remapping!\n");
-            return -1;
+            return -EIO;
         }
     }
 
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -27,7 +27,7 @@ int iommu_setup_hpet_msi(struct msi_desc
 /* While VT-d specific, this must get declared in a generic header. */
 int adjust_vtd_irq_affinities(void);
 void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int order, int present);
-int iommu_supports_eim(void);
+bool_t iommu_supports_eim(void);
 int iommu_enable_x2apic_IR(void);
 void iommu_disable_x2apic_IR(void);
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/2] VT-d: use proper error codes in iommu_enable_x2apic_IR()

[Andrew Cooper]

On 29/09/15 12:44, Jan Beulich wrote:
> ... allowing to suppress a confusing messeage combination: When
> ACPI_DMAR_X2APIC_OPT_OUT is set, so far we first logged a message
> that IR could not be enabled (hence not using x2APIC), followed by
> one indicating successful initialization of IR (if no other problems
> prevented that).
>
> Also adjust the return type of iommu_supports_eim() and fix some
> broken indentation in the function.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 1/2] VT-d: use proper error codes in iommu_enable_x2apic_IR()

[Zhang, Yang Z]

Jan Beulich wrote on 2015-09-29:
> ... allowing to suppress a confusing messeage combination: When 
> ACPI_DMAR_X2APIC_OPT_OUT is set, so far we first logged a message that 
> IR could not be enabled (hence not using x2APIC), followed by one 
> indicating successful initialization of IR (if no other problems prevented that).
> 
> Also adjust the return type of iommu_supports_eim() and fix some 
> broken indentation in the function.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Yang Zhang <yang.z.zhang@intel.com>

Best regards,
Yang

[PATCH 2/2] VT-d: section placement and type adjustments

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2876 bytes --]

With x2APIC requiring iommu_supports_eim() to return true, we can
adjust a few conditonals such that both it and
platform_supports_x2apic() can be marked __init. For the latter as
well as for platform_supports_intremap() also change the return types
to bool_t.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -901,16 +901,17 @@ void acpi_dmar_zap(void)
         write_atomic((uint32_t*)&dmar_table->signature[0], sig);
 }
 
-int platform_supports_intremap(void)
+bool_t platform_supports_intremap(void)
 {
-    unsigned int mask = ACPI_DMAR_INTR_REMAP;
+    const unsigned int mask = ACPI_DMAR_INTR_REMAP;
 
     return (dmar_flags & mask) == ACPI_DMAR_INTR_REMAP;
 }
 
-int platform_supports_x2apic(void)
+bool_t __init platform_supports_x2apic(void)
 {
-    unsigned int mask = ACPI_DMAR_INTR_REMAP | ACPI_DMAR_X2APIC_OPT_OUT;
+    const unsigned int mask = ACPI_DMAR_INTR_REMAP | ACPI_DMAR_X2APIC_OPT_OUT;
+
     return cpu_has_x2apic && ((dmar_flags & mask) == ACPI_DMAR_INTR_REMAP);
 }
 
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -93,8 +93,8 @@ void vtd_ops_preamble_quirk(struct iommu
 void vtd_ops_postamble_quirk(struct iommu* iommu);
 void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map);
 void pci_vtd_quirk(const struct pci_dev *);
-int platform_supports_intremap(void);
-int platform_supports_x2apic(void);
+bool_t platform_supports_intremap(void);
+bool_t platform_supports_x2apic(void);
 
 void vtd_set_hwdom_mapping(struct domain *d);
 
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -143,7 +143,7 @@ static void set_hpet_source_id(unsigned 
     set_ire_sid(ire, SVT_VERIFY_SID_SQ, SQ_13_IGNORE_3, hpetid_to_bdf(id));
 }
 
-bool_t iommu_supports_eim(void)
+bool_t __init iommu_supports_eim(void)
 {
     struct acpi_drhd_unit *drhd;
     unsigned int apic;
@@ -832,11 +832,16 @@ int iommu_enable_x2apic_IR(void)
     struct acpi_drhd_unit *drhd;
     struct iommu *iommu;
 
-    if ( !iommu_supports_eim() )
-        return -EOPNOTSUPP;
+    if ( system_state < SYS_STATE_active )
+    {
+        if ( !iommu_supports_eim() )
+            return -EOPNOTSUPP;
 
-    if ( !platform_supports_x2apic() )
-        return -ENXIO;
+        if ( !platform_supports_x2apic() )
+            return -ENXIO;
+    }
+    else if ( !x2apic_enabled )
+        return -EOPNOTSUPP;
 
     for_each_drhd_unit ( drhd )
     {
@@ -888,7 +893,8 @@ void iommu_disable_x2apic_IR(void)
 {
     struct acpi_drhd_unit *drhd;
 
-    if ( !iommu_supports_eim() )
+    /* x2apic_enabled implies iommu_supports_eim(). */
+    if ( !x2apic_enabled )
         return;
 
     for_each_drhd_unit ( drhd )




[-- Attachment #2: x86-enable-x2APIC-IR-init.patch --]
[-- Type: text/plain, Size: 2918 bytes --]

VT-d: section placement and type adjustments

With x2APIC requiring iommu_supports_eim() to return true, we can
adjust a few conditonals such that both it and
platform_supports_x2apic() can be marked __init. For the latter as
well as for platform_supports_intremap() also change the return types
to bool_t.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -901,16 +901,17 @@ void acpi_dmar_zap(void)
         write_atomic((uint32_t*)&dmar_table->signature[0], sig);
 }
 
-int platform_supports_intremap(void)
+bool_t platform_supports_intremap(void)
 {
-    unsigned int mask = ACPI_DMAR_INTR_REMAP;
+    const unsigned int mask = ACPI_DMAR_INTR_REMAP;
 
     return (dmar_flags & mask) == ACPI_DMAR_INTR_REMAP;
 }
 
-int platform_supports_x2apic(void)
+bool_t __init platform_supports_x2apic(void)
 {
-    unsigned int mask = ACPI_DMAR_INTR_REMAP | ACPI_DMAR_X2APIC_OPT_OUT;
+    const unsigned int mask = ACPI_DMAR_INTR_REMAP | ACPI_DMAR_X2APIC_OPT_OUT;
+
     return cpu_has_x2apic && ((dmar_flags & mask) == ACPI_DMAR_INTR_REMAP);
 }
 
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -93,8 +93,8 @@ void vtd_ops_preamble_quirk(struct iommu
 void vtd_ops_postamble_quirk(struct iommu* iommu);
 void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map);
 void pci_vtd_quirk(const struct pci_dev *);
-int platform_supports_intremap(void);
-int platform_supports_x2apic(void);
+bool_t platform_supports_intremap(void);
+bool_t platform_supports_x2apic(void);
 
 void vtd_set_hwdom_mapping(struct domain *d);
 
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -143,7 +143,7 @@ static void set_hpet_source_id(unsigned 
     set_ire_sid(ire, SVT_VERIFY_SID_SQ, SQ_13_IGNORE_3, hpetid_to_bdf(id));
 }
 
-bool_t iommu_supports_eim(void)
+bool_t __init iommu_supports_eim(void)
 {
     struct acpi_drhd_unit *drhd;
     unsigned int apic;
@@ -832,11 +832,16 @@ int iommu_enable_x2apic_IR(void)
     struct acpi_drhd_unit *drhd;
     struct iommu *iommu;
 
-    if ( !iommu_supports_eim() )
-        return -EOPNOTSUPP;
+    if ( system_state < SYS_STATE_active )
+    {
+        if ( !iommu_supports_eim() )
+            return -EOPNOTSUPP;
 
-    if ( !platform_supports_x2apic() )
-        return -ENXIO;
+        if ( !platform_supports_x2apic() )
+            return -ENXIO;
+    }
+    else if ( !x2apic_enabled )
+        return -EOPNOTSUPP;
 
     for_each_drhd_unit ( drhd )
     {
@@ -888,7 +893,8 @@ void iommu_disable_x2apic_IR(void)
 {
     struct acpi_drhd_unit *drhd;
 
-    if ( !iommu_supports_eim() )
+    /* x2apic_enabled implies iommu_supports_eim(). */
+    if ( !x2apic_enabled )
         return;
 
     for_each_drhd_unit ( drhd )

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/2] VT-d: section placement and type adjustments

[Andrew Cooper]

On 29/09/15 12:45, Jan Beulich wrote:
> With x2APIC requiring iommu_supports_eim() to return true, we can
> adjust a few conditonals such that both it and
> platform_supports_x2apic() can be marked __init. For the latter as
> well as for platform_supports_intremap() also change the return types
> to bool_t.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 2/2] VT-d: section placement and type adjustments

[Zhang, Yang Z]

Jan Beulich wrote on 2015-09-29:
> With x2APIC requiring iommu_supports_eim() to return true, we can
> adjust a few conditonals such that both it and
> platform_supports_x2apic() can be marked __init. For the latter as
> well as for platform_supports_intremap() also change the return types
> to bool_t.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -901,16 +901,17 @@ void acpi_dmar_zap(void)
>          write_atomic((uint32_t*)&dmar_table->signature[0], sig);
>  }
> -int platform_supports_intremap(void)
> +bool_t platform_supports_intremap(void)
>  {
> -    unsigned int mask = ACPI_DMAR_INTR_REMAP;
> +    const unsigned int mask = ACPI_DMAR_INTR_REMAP;
> 
>      return (dmar_flags & mask) == ACPI_DMAR_INTR_REMAP;
>  }
> -int platform_supports_x2apic(void)
> +bool_t __init platform_supports_x2apic(void)
>  {
> -    unsigned int mask = ACPI_DMAR_INTR_REMAP |
> ACPI_DMAR_X2APIC_OPT_OUT; +    const unsigned int mask =
> ACPI_DMAR_INTR_REMAP | ACPI_DMAR_X2APIC_OPT_OUT; +
>      return cpu_has_x2apic && ((dmar_flags & mask) ==
> ACPI_DMAR_INTR_REMAP);
>  }
> --- a/xen/drivers/passthrough/vtd/extern.h
> +++ b/xen/drivers/passthrough/vtd/extern.h
> @@ -93,8 +93,8 @@ void vtd_ops_preamble_quirk(struct iommu
>  void vtd_ops_postamble_quirk(struct iommu* iommu);
>  void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map);
>  void pci_vtd_quirk(const struct pci_dev *);
> -int platform_supports_intremap(void);
> -int platform_supports_x2apic(void);
> +bool_t platform_supports_intremap(void);
> +bool_t platform_supports_x2apic(void);
> 
>  void vtd_set_hwdom_mapping(struct domain *d);
> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -143,7 +143,7 @@ static void set_hpet_source_id(unsigned
>      set_ire_sid(ire, SVT_VERIFY_SID_SQ, SQ_13_IGNORE_3,
> hpetid_to_bdf(id));
>  }
> -bool_t iommu_supports_eim(void)
> +bool_t __init iommu_supports_eim(void)
>  {
>      struct acpi_drhd_unit *drhd; unsigned int apic; @@ -832,11 +832,16
>      @@ int iommu_enable_x2apic_IR(void) struct acpi_drhd_unit *drhd;
>      struct iommu *iommu;
> -    if ( !iommu_supports_eim() )
> -        return -EOPNOTSUPP;
> +    if ( system_state < SYS_STATE_active )
> +    {
> +        if ( !iommu_supports_eim() )
> +            return -EOPNOTSUPP;
> 
> -    if ( !platform_supports_x2apic() )
> -        return -ENXIO;
> +        if ( !platform_supports_x2apic() )
> +            return -ENXIO;
> +    }
> +    else if ( !x2apic_enabled )
> +        return -EOPNOTSUPP;

Why need the last check here? From the code, this check is called only in resume_x2apic() which already has an assert there: ASSERT(x2apic_enabled) .

Best regards,
Yang

Re: [PATCH 2/2] VT-d: section placement and type adjustments

[Jan Beulich]

>>> On 10.10.15 at 08:30, <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2015-09-29:
>> --- a/xen/drivers/passthrough/vtd/intremap.c
>> +++ b/xen/drivers/passthrough/vtd/intremap.c
>> @@ -143,7 +143,7 @@ static void set_hpet_source_id(unsigned
>>      set_ire_sid(ire, SVT_VERIFY_SID_SQ, SQ_13_IGNORE_3,
>> hpetid_to_bdf(id));
>>  }
>> -bool_t iommu_supports_eim(void)
>> +bool_t __init iommu_supports_eim(void)
>>  {
>>      struct acpi_drhd_unit *drhd; unsigned int apic; @@ -832,11 +832,16
>>      @@ int iommu_enable_x2apic_IR(void) struct acpi_drhd_unit *drhd;
>>      struct iommu *iommu;
>> -    if ( !iommu_supports_eim() )
>> -        return -EOPNOTSUPP;
>> +    if ( system_state < SYS_STATE_active )
>> +    {
>> +        if ( !iommu_supports_eim() )
>> +            return -EOPNOTSUPP;
>> 
>> -    if ( !platform_supports_x2apic() )
>> -        return -ENXIO;
>> +        if ( !platform_supports_x2apic() )
>> +            return -ENXIO;
>> +    }
>> +    else if ( !x2apic_enabled )
>> +        return -EOPNOTSUPP;
> 
> Why need the last check here? From the code, this check is called only in 
> resume_x2apic() which already has an assert there: ASSERT(x2apic_enabled) .

Just to cover (theoretical) future callers. Plus I don't think a function
should make undue assumptions about ASSERT()s placed in far away
code, or misbehave in non-debug builds just because then there's no
guard in the caller anymore.

Jan

Re: [PATCH 2/2] VT-d: section placement and type adjustments

[Zhang, Yang Z]

Jan Beulich wrote on 2015-10-12:
>>>> On 10.10.15 at 08:30, <yang.z.zhang@intel.com> wrote:
>> Jan Beulich wrote on 2015-09-29:
>>> --- a/xen/drivers/passthrough/vtd/intremap.c
>>> +++ b/xen/drivers/passthrough/vtd/intremap.c
>>> @@ -143,7 +143,7 @@ static void set_hpet_source_id(unsigned
>>>      set_ire_sid(ire, SVT_VERIFY_SID_SQ, SQ_13_IGNORE_3,
>>> hpetid_to_bdf(id));  } -bool_t iommu_supports_eim(void)
>>> +bool_t __init iommu_supports_eim(void)
>>>  {
>>>      struct acpi_drhd_unit *drhd; unsigned int apic; @@ -832,11 +832,16
>>>      @@ int iommu_enable_x2apic_IR(void) struct acpi_drhd_unit *drhd;
>>>      struct iommu *iommu;
>>> -    if ( !iommu_supports_eim() )
>>> -        return -EOPNOTSUPP;
>>> +    if ( system_state < SYS_STATE_active )
>>> +    {
>>> +        if ( !iommu_supports_eim() )
>>> +            return -EOPNOTSUPP;
>>> 
>>> -    if ( !platform_supports_x2apic() )
>>> -        return -ENXIO;
>>> +        if ( !platform_supports_x2apic() )
>>> +            return -ENXIO;
>>> +    }
>>> +    else if ( !x2apic_enabled )
>>> +        return -EOPNOTSUPP;
>> 
>> Why need the last check here? From the code, this check is called
>> only in
>> resume_x2apic() which already has an assert there:
> ASSERT(x2apic_enabled) .
> 
> Just to cover (theoretical) future callers. Plus I don't think a
> function should make undue assumptions about ASSERT()s placed in far
> away code, or misbehave in non-debug builds just because then there's
> no guard in the caller anymore.

ok, it make sense.

Acked-by: Yang Zhang <yang.z.zhang@intel.com>

Best regards,
Yang

Re: [PATCH 2/2] VT-d: section placement and type adjustments

[Jan Beulich]

>>> On 13.10.15 at 07:28, <yang.z.zhang@intel.com> wrote:
> Acked-by: Yang Zhang <yang.z.zhang@intel.com>

Thanks. What about patch 1?

Jan

Re: [PATCH 2/2] VT-d: section placement and type adjustments

[Zhang, Yang Z]

Jan Beulich wrote on 2015-10-13:
>>>> On 13.10.15 at 07:28, <yang.z.zhang@intel.com> wrote:
>> Acked-by: Yang Zhang <yang.z.zhang@intel.com>
> 
> Thanks. What about patch 1?

Done!
I think I have acked it. But seems I forget to do it. 

Best regards,
Yang