[v3,11/14] AMD/IOMMU: adjust setup of internal interrupt for x2APIC mode
diff mbox series

Message ID f8421e6c-3553-7f98-ab20-a26f4dcf8ee1@suse.com
State New
Headers show
Series
  • [v3,01/14] AMD/IOMMU: free more memory when cleaning up after error
Related show

Commit Message

Jan Beulich July 16, 2019, 4:39 p.m. UTC
In order to be able to express all possible destinations we need to make
use of this non-MSI-capability based mechanism. The new IRQ controller
structure can re-use certain MSI functions, though.

For now general and PPR interrupts still share a single vector, IRQ, and
hence handler.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v3: Re-base.

Comments

Andrew Cooper July 19, 2019, 5:31 p.m. UTC | #1
On 16/07/2019 17:39, Jan Beulich wrote:
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> @@ -416,6 +416,25 @@ union amd_iommu_ext_features {
>       } flds;
>   };
>   
> +/* x2APIC Control Registers */
> +#define IOMMU_XT_INT_CTRL_MMIO_OFFSET		0x0170
> +#define IOMMU_XT_PPR_INT_CTRL_MMIO_OFFSET	0x0178
> +#define IOMMU_XT_GA_INT_CTRL_MMIO_OFFSET	0x0180
> +
> +union amd_iommu_x2apic_control {
> +    uint64_t raw;
> +    struct {
> +        unsigned int :2;
> +        unsigned int dest_mode:1;
> +        unsigned int :5;
> +        unsigned int dest_lo:24;
> +        unsigned int vector:8;
> +        unsigned int int_type:1; /* DM in IOMMU spec 3.04 */
> +        unsigned int :15;
> +        unsigned int dest_hi:8;

Bool bitfields like you've done elsewhere in v3?

My pre-existing R-by stands, but ideally with this chagned.

~Andrew
Woods, Brian July 19, 2019, 6:39 p.m. UTC | #2
On Tue, Jul 16, 2019 at 04:39:58PM +0000, Jan Beulich wrote:
> In order to be able to express all possible destinations we need to make
> use of this non-MSI-capability based mechanism. The new IRQ controller
> structure can re-use certain MSI functions, though.
> 
> For now general and PPR interrupts still share a single vector, IRQ, and
> hence handler.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Brian Woods <brian.woods@amd.com>

> ---
> v3: Re-base.
> 
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -472,6 +472,44 @@ static hw_irq_controller iommu_maskable_
>       .set_affinity = set_msi_affinity,
>   };
>   
> +static void set_x2apic_affinity(struct irq_desc *desc, const cpumask_t *mask)
> +{
> +    struct amd_iommu *iommu = desc->action->dev_id;
> +    unsigned int dest = set_desc_affinity(desc, mask);
> +    union amd_iommu_x2apic_control ctrl = {};
> +    unsigned long flags;
> +
> +    if ( dest == BAD_APICID )
> +        return;
> +
> +    msi_compose_msg(desc->arch.vector, NULL, &iommu->msi.msg);
> +    iommu->msi.msg.dest32 = dest;
> +
> +    ctrl.dest_mode = MASK_EXTR(iommu->msi.msg.address_lo,
> +                               MSI_ADDR_DESTMODE_MASK);
> +    ctrl.int_type = MASK_EXTR(iommu->msi.msg.data,
> +                              MSI_DATA_DELIVERY_MODE_MASK);
> +    ctrl.vector = desc->arch.vector;
> +    ctrl.dest_lo = dest;
> +    ctrl.dest_hi = dest >> 24;
> +
> +    spin_lock_irqsave(&iommu->lock, flags);
> +    writeq(ctrl.raw, iommu->mmio_base + IOMMU_XT_INT_CTRL_MMIO_OFFSET);
> +    writeq(ctrl.raw, iommu->mmio_base + IOMMU_XT_PPR_INT_CTRL_MMIO_OFFSET);
> +    spin_unlock_irqrestore(&iommu->lock, flags);
> +}
> +
> +static hw_irq_controller iommu_x2apic_type = {
> +    .typename     = "IOMMU-x2APIC",
> +    .startup      = irq_startup_none,
> +    .shutdown     = irq_shutdown_none,
> +    .enable       = irq_enable_none,
> +    .disable      = irq_disable_none,
> +    .ack          = ack_nonmaskable_msi_irq,
> +    .end          = end_nonmaskable_msi_irq,
> +    .set_affinity = set_x2apic_affinity,
> +};
> +
>   static void parse_event_log_entry(struct amd_iommu *iommu, u32 entry[])
>   {
>       u16 domain_id, device_id, flags;
> @@ -726,8 +764,6 @@ static void iommu_interrupt_handler(int
>   static bool_t __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
>   {
>       int irq, ret;
> -    hw_irq_controller *handler;
> -    u16 control;
>   
>       irq = create_irq(NUMA_NO_NODE);
>       if ( irq <= 0 )
> @@ -747,20 +783,43 @@ static bool_t __init set_iommu_interrupt
>                           PCI_SLOT(iommu->bdf), PCI_FUNC(iommu->bdf));
>           return 0;
>       }
> -    control = pci_conf_read16(iommu->seg, PCI_BUS(iommu->bdf),
> -                              PCI_SLOT(iommu->bdf), PCI_FUNC(iommu->bdf),
> -                              iommu->msi.msi_attrib.pos + PCI_MSI_FLAGS);
> -    iommu->msi.msi.nvec = 1;
> -    if ( is_mask_bit_support(control) )
> -    {
> -        iommu->msi.msi_attrib.maskbit = 1;
> -        iommu->msi.msi.mpos = msi_mask_bits_reg(iommu->msi.msi_attrib.pos,
> -                                                is_64bit_address(control));
> -        handler = &iommu_maskable_msi_type;
> +
> +    if ( iommu->ctrl.int_cap_xt_en )
> +    {
> +        struct irq_desc *desc = irq_to_desc(irq);
> +
> +        iommu->msi.msi_attrib.pos = MSI_TYPE_IOMMU;
> +        iommu->msi.msi_attrib.maskbit = 0;
> +        iommu->msi.msi_attrib.is_64 = 1;
> +
> +        desc->msi_desc = &iommu->msi;
> +        desc->handler = &iommu_x2apic_type;
> +
> +        ret = 0;
>       }
>       else
> -        handler = &iommu_msi_type;
> -    ret = __setup_msi_irq(irq_to_desc(irq), &iommu->msi, handler);
> +    {
> +        hw_irq_controller *handler;
> +        u16 control;
> +
> +        control = pci_conf_read16(iommu->seg, PCI_BUS(iommu->bdf),
> +                                  PCI_SLOT(iommu->bdf), PCI_FUNC(iommu->bdf),
> +                                  iommu->msi.msi_attrib.pos + PCI_MSI_FLAGS);
> +
> +        iommu->msi.msi.nvec = 1;
> +        if ( is_mask_bit_support(control) )
> +        {
> +            iommu->msi.msi_attrib.maskbit = 1;
> +            iommu->msi.msi.mpos = msi_mask_bits_reg(iommu->msi.msi_attrib.pos,
> +                                                    is_64bit_address(control));
> +            handler = &iommu_maskable_msi_type;
> +        }
> +        else
> +            handler = &iommu_msi_type;
> +
> +        ret = __setup_msi_irq(irq_to_desc(irq), &iommu->msi, handler);
> +    }
> +
>       if ( !ret )
>           ret = request_irq(irq, 0, iommu_interrupt_handler, "amd_iommu", iommu);
>       if ( ret )
> @@ -838,8 +897,19 @@ static void enable_iommu(struct amd_iomm
>           struct irq_desc *desc = irq_to_desc(iommu->msi.irq);
>   
>           spin_lock(&desc->lock);
> -        set_msi_affinity(desc, NULL);
> -        spin_unlock(&desc->lock);
> +
> +        if ( iommu->ctrl.int_cap_xt_en )
> +        {
> +            set_x2apic_affinity(desc, NULL);
> +            spin_unlock(&desc->lock);
> +        }
> +        else
> +        {
> +            set_msi_affinity(desc, NULL);
> +            spin_unlock(&desc->lock);
> +
> +            amd_iommu_msi_enable(iommu, IOMMU_CONTROL_ENABLED);
> +        }
>       }
>   
>       amd_iommu_msi_enable(iommu, IOMMU_CONTROL_ENABLED);
> @@ -879,7 +949,9 @@ static void disable_iommu(struct amd_iom
>           return;
>       }
>   
> -    amd_iommu_msi_enable(iommu, IOMMU_CONTROL_DISABLED);
> +    if ( !iommu->ctrl.int_cap_xt_en )
> +        amd_iommu_msi_enable(iommu, IOMMU_CONTROL_DISABLED);
> +
>       set_iommu_command_buffer_control(iommu, IOMMU_CONTROL_DISABLED);
>       set_iommu_event_log_control(iommu, IOMMU_CONTROL_DISABLED);
>   
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> @@ -416,6 +416,25 @@ union amd_iommu_ext_features {
>       } flds;
>   };
>   
> +/* x2APIC Control Registers */
> +#define IOMMU_XT_INT_CTRL_MMIO_OFFSET		0x0170
> +#define IOMMU_XT_PPR_INT_CTRL_MMIO_OFFSET	0x0178
> +#define IOMMU_XT_GA_INT_CTRL_MMIO_OFFSET	0x0180
> +
> +union amd_iommu_x2apic_control {
> +    uint64_t raw;
> +    struct {
> +        unsigned int :2;
> +        unsigned int dest_mode:1;
> +        unsigned int :5;
> +        unsigned int dest_lo:24;
> +        unsigned int vector:8;
> +        unsigned int int_type:1; /* DM in IOMMU spec 3.04 */
> +        unsigned int :15;
> +        unsigned int dest_hi:8;
> +    };
> +};
> +
>   /* Status Register*/
>   #define IOMMU_STATUS_MMIO_OFFSET		0x2020
>   #define IOMMU_STATUS_EVENT_OVERFLOW_MASK	0x00000001
>
Jan Beulich July 22, 2019, 8:43 a.m. UTC | #3
On 19.07.2019 19:31, Andrew Cooper wrote:
> On 16/07/2019 17:39, Jan Beulich wrote:
>> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
>> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
>> @@ -416,6 +416,25 @@ union amd_iommu_ext_features {
>>        } flds;
>>    };
>>    
>> +/* x2APIC Control Registers */
>> +#define IOMMU_XT_INT_CTRL_MMIO_OFFSET		0x0170
>> +#define IOMMU_XT_PPR_INT_CTRL_MMIO_OFFSET	0x0178
>> +#define IOMMU_XT_GA_INT_CTRL_MMIO_OFFSET	0x0180
>> +
>> +union amd_iommu_x2apic_control {
>> +    uint64_t raw;
>> +    struct {
>> +        unsigned int :2;
>> +        unsigned int dest_mode:1;
>> +        unsigned int :5;
>> +        unsigned int dest_lo:24;
>> +        unsigned int vector:8;
>> +        unsigned int int_type:1; /* DM in IOMMU spec 3.04 */
>> +        unsigned int :15;
>> +        unsigned int dest_hi:8;
> 
> Bool bitfields like you've done elsewhere in v3?

I'd been considering this, but decided against because of ...

+static void set_x2apic_affinity(struct irq_desc *desc, const cpumask_t *mask)
+{
+    struct amd_iommu *iommu = desc->action->dev_id;
+    unsigned int dest = set_desc_affinity(desc, mask);
+    union amd_iommu_x2apic_control ctrl = {};
+    unsigned long flags;
+
+    if ( dest == BAD_APICID )
+        return;
+
+    msi_compose_msg(desc->arch.vector, NULL, &iommu->msi.msg);
+    iommu->msi.msg.dest32 = dest;
+
+    ctrl.dest_mode = MASK_EXTR(iommu->msi.msg.address_lo,
+                               MSI_ADDR_DESTMODE_MASK);
+    ctrl.int_type = MASK_EXTR(iommu->msi.msg.data,
+                              MSI_DATA_DELIVERY_MODE_MASK);

... this: We really mean a value copy here, not an "is zero" or
"is non-zero" one. I also think that both fields are not suitably
named for being boolean. In the recent re-work of struct
IO_APIC_route_entry (ca9310b24e) similar fields similarly were
left as "unsigned int". MSI's struct msg_data also falls into the
same category. I think if we wanted to switch to bool here, we
should do so everywhere at the same time (along with suitably
renaming fields).

Jan
Andrew Cooper July 22, 2019, 1:45 p.m. UTC | #4
On 22/07/2019 09:43, Jan Beulich wrote:
> On 19.07.2019 19:31, Andrew Cooper wrote:
>> On 16/07/2019 17:39, Jan Beulich wrote:
>>> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
>>> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
>>> @@ -416,6 +416,25 @@ union amd_iommu_ext_features {
>>>        } flds;
>>>    };
>>>    
>>> +/* x2APIC Control Registers */
>>> +#define IOMMU_XT_INT_CTRL_MMIO_OFFSET		0x0170
>>> +#define IOMMU_XT_PPR_INT_CTRL_MMIO_OFFSET	0x0178
>>> +#define IOMMU_XT_GA_INT_CTRL_MMIO_OFFSET	0x0180
>>> +
>>> +union amd_iommu_x2apic_control {
>>> +    uint64_t raw;
>>> +    struct {
>>> +        unsigned int :2;
>>> +        unsigned int dest_mode:1;
>>> +        unsigned int :5;
>>> +        unsigned int dest_lo:24;
>>> +        unsigned int vector:8;
>>> +        unsigned int int_type:1; /* DM in IOMMU spec 3.04 */
>>> +        unsigned int :15;
>>> +        unsigned int dest_hi:8;
>> Bool bitfields like you've done elsewhere in v3?
> I'd been considering this, but decided against because of ...
>
> +static void set_x2apic_affinity(struct irq_desc *desc, const cpumask_t *mask)
> +{
> +    struct amd_iommu *iommu = desc->action->dev_id;
> +    unsigned int dest = set_desc_affinity(desc, mask);
> +    union amd_iommu_x2apic_control ctrl = {};
> +    unsigned long flags;
> +
> +    if ( dest == BAD_APICID )
> +        return;
> +
> +    msi_compose_msg(desc->arch.vector, NULL, &iommu->msi.msg);
> +    iommu->msi.msg.dest32 = dest;
> +
> +    ctrl.dest_mode = MASK_EXTR(iommu->msi.msg.address_lo,
> +                               MSI_ADDR_DESTMODE_MASK);
> +    ctrl.int_type = MASK_EXTR(iommu->msi.msg.data,
> +                              MSI_DATA_DELIVERY_MODE_MASK);
>
> ... this: We really mean a value copy here, not an "is zero" or
> "is non-zero" one. I also think that both fields are not suitably
> named for being boolean. In the recent re-work of struct
> IO_APIC_route_entry (ca9310b24e) similar fields similarly were
> left as "unsigned int". MSI's struct msg_data also falls into the
> same category. I think if we wanted to switch to bool here, we
> should do so everywhere at the same time (along with suitably
> renaming fields).

Architecturally, both of these are single-bit fields, no?

~Andrew
Jan Beulich July 22, 2019, 3:22 p.m. UTC | #5
On 22.07.2019 15:45, Andrew Cooper wrote:
> On 22/07/2019 09:43, Jan Beulich wrote:
>> On 19.07.2019 19:31, Andrew Cooper wrote:
>>> On 16/07/2019 17:39, Jan Beulich wrote:
>>>> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
>>>> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
>>>> @@ -416,6 +416,25 @@ union amd_iommu_ext_features {
>>>>         } flds;
>>>>     };
>>>>     
>>>> +/* x2APIC Control Registers */
>>>> +#define IOMMU_XT_INT_CTRL_MMIO_OFFSET		0x0170
>>>> +#define IOMMU_XT_PPR_INT_CTRL_MMIO_OFFSET	0x0178
>>>> +#define IOMMU_XT_GA_INT_CTRL_MMIO_OFFSET	0x0180
>>>> +
>>>> +union amd_iommu_x2apic_control {
>>>> +    uint64_t raw;
>>>> +    struct {
>>>> +        unsigned int :2;
>>>> +        unsigned int dest_mode:1;
>>>> +        unsigned int :5;
>>>> +        unsigned int dest_lo:24;
>>>> +        unsigned int vector:8;
>>>> +        unsigned int int_type:1; /* DM in IOMMU spec 3.04 */
>>>> +        unsigned int :15;
>>>> +        unsigned int dest_hi:8;
>>> Bool bitfields like you've done elsewhere in v3?
>> I'd been considering this, but decided against because of ...
>>
>> +static void set_x2apic_affinity(struct irq_desc *desc, const cpumask_t *mask)
>> +{
>> +    struct amd_iommu *iommu = desc->action->dev_id;
>> +    unsigned int dest = set_desc_affinity(desc, mask);
>> +    union amd_iommu_x2apic_control ctrl = {};
>> +    unsigned long flags;
>> +
>> +    if ( dest == BAD_APICID )
>> +        return;
>> +
>> +    msi_compose_msg(desc->arch.vector, NULL, &iommu->msi.msg);
>> +    iommu->msi.msg.dest32 = dest;
>> +
>> +    ctrl.dest_mode = MASK_EXTR(iommu->msi.msg.address_lo,
>> +                               MSI_ADDR_DESTMODE_MASK);
>> +    ctrl.int_type = MASK_EXTR(iommu->msi.msg.data,
>> +                              MSI_DATA_DELIVERY_MODE_MASK);
>>
>> ... this: We really mean a value copy here, not an "is zero" or
>> "is non-zero" one. I also think that both fields are not suitably
>> named for being boolean. In the recent re-work of struct
>> IO_APIC_route_entry (ca9310b24e) similar fields similarly were
>> left as "unsigned int". MSI's struct msg_data also falls into the
>> same category. I think if we wanted to switch to bool here, we
>> should do so everywhere at the same time (along with suitably
>> renaming fields).
> 
> Architecturally, both of these are single-bit fields, no?

Sure, but with the names we have there's no obvious indication
whether physical/logical are respectively true or false. Same
(or worse) for fixed/lowest priority, which in the LAPIC even
has further accompanying values (i.e. couldn't possibly be bool
there at all).

Jan

Patch
diff mbox series

--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -472,6 +472,44 @@  static hw_irq_controller iommu_maskable_
      .set_affinity = set_msi_affinity,
  };
  
+static void set_x2apic_affinity(struct irq_desc *desc, const cpumask_t *mask)
+{
+    struct amd_iommu *iommu = desc->action->dev_id;
+    unsigned int dest = set_desc_affinity(desc, mask);
+    union amd_iommu_x2apic_control ctrl = {};
+    unsigned long flags;
+
+    if ( dest == BAD_APICID )
+        return;
+
+    msi_compose_msg(desc->arch.vector, NULL, &iommu->msi.msg);
+    iommu->msi.msg.dest32 = dest;
+
+    ctrl.dest_mode = MASK_EXTR(iommu->msi.msg.address_lo,
+                               MSI_ADDR_DESTMODE_MASK);
+    ctrl.int_type = MASK_EXTR(iommu->msi.msg.data,
+                              MSI_DATA_DELIVERY_MODE_MASK);
+    ctrl.vector = desc->arch.vector;
+    ctrl.dest_lo = dest;
+    ctrl.dest_hi = dest >> 24;
+
+    spin_lock_irqsave(&iommu->lock, flags);
+    writeq(ctrl.raw, iommu->mmio_base + IOMMU_XT_INT_CTRL_MMIO_OFFSET);
+    writeq(ctrl.raw, iommu->mmio_base + IOMMU_XT_PPR_INT_CTRL_MMIO_OFFSET);
+    spin_unlock_irqrestore(&iommu->lock, flags);
+}
+
+static hw_irq_controller iommu_x2apic_type = {
+    .typename     = "IOMMU-x2APIC",
+    .startup      = irq_startup_none,
+    .shutdown     = irq_shutdown_none,
+    .enable       = irq_enable_none,
+    .disable      = irq_disable_none,
+    .ack          = ack_nonmaskable_msi_irq,
+    .end          = end_nonmaskable_msi_irq,
+    .set_affinity = set_x2apic_affinity,
+};
+
  static void parse_event_log_entry(struct amd_iommu *iommu, u32 entry[])
  {
      u16 domain_id, device_id, flags;
@@ -726,8 +764,6 @@  static void iommu_interrupt_handler(int
  static bool_t __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
  {
      int irq, ret;
-    hw_irq_controller *handler;
-    u16 control;
  
      irq = create_irq(NUMA_NO_NODE);
      if ( irq <= 0 )
@@ -747,20 +783,43 @@  static bool_t __init set_iommu_interrupt
                          PCI_SLOT(iommu->bdf), PCI_FUNC(iommu->bdf));
          return 0;
      }
-    control = pci_conf_read16(iommu->seg, PCI_BUS(iommu->bdf),
-                              PCI_SLOT(iommu->bdf), PCI_FUNC(iommu->bdf),
-                              iommu->msi.msi_attrib.pos + PCI_MSI_FLAGS);
-    iommu->msi.msi.nvec = 1;
-    if ( is_mask_bit_support(control) )
-    {
-        iommu->msi.msi_attrib.maskbit = 1;
-        iommu->msi.msi.mpos = msi_mask_bits_reg(iommu->msi.msi_attrib.pos,
-                                                is_64bit_address(control));
-        handler = &iommu_maskable_msi_type;
+
+    if ( iommu->ctrl.int_cap_xt_en )
+    {
+        struct irq_desc *desc = irq_to_desc(irq);
+
+        iommu->msi.msi_attrib.pos = MSI_TYPE_IOMMU;
+        iommu->msi.msi_attrib.maskbit = 0;
+        iommu->msi.msi_attrib.is_64 = 1;
+
+        desc->msi_desc = &iommu->msi;
+        desc->handler = &iommu_x2apic_type;
+
+        ret = 0;
      }
      else
-        handler = &iommu_msi_type;
-    ret = __setup_msi_irq(irq_to_desc(irq), &iommu->msi, handler);
+    {
+        hw_irq_controller *handler;
+        u16 control;
+
+        control = pci_conf_read16(iommu->seg, PCI_BUS(iommu->bdf),
+                                  PCI_SLOT(iommu->bdf), PCI_FUNC(iommu->bdf),
+                                  iommu->msi.msi_attrib.pos + PCI_MSI_FLAGS);
+
+        iommu->msi.msi.nvec = 1;
+        if ( is_mask_bit_support(control) )
+        {
+            iommu->msi.msi_attrib.maskbit = 1;
+            iommu->msi.msi.mpos = msi_mask_bits_reg(iommu->msi.msi_attrib.pos,
+                                                    is_64bit_address(control));
+            handler = &iommu_maskable_msi_type;
+        }
+        else
+            handler = &iommu_msi_type;
+
+        ret = __setup_msi_irq(irq_to_desc(irq), &iommu->msi, handler);
+    }
+
      if ( !ret )
          ret = request_irq(irq, 0, iommu_interrupt_handler, "amd_iommu", iommu);
      if ( ret )
@@ -838,8 +897,19 @@  static void enable_iommu(struct amd_iomm
          struct irq_desc *desc = irq_to_desc(iommu->msi.irq);
  
          spin_lock(&desc->lock);
-        set_msi_affinity(desc, NULL);
-        spin_unlock(&desc->lock);
+
+        if ( iommu->ctrl.int_cap_xt_en )
+        {
+            set_x2apic_affinity(desc, NULL);
+            spin_unlock(&desc->lock);
+        }
+        else
+        {
+            set_msi_affinity(desc, NULL);
+            spin_unlock(&desc->lock);
+
+            amd_iommu_msi_enable(iommu, IOMMU_CONTROL_ENABLED);
+        }
      }
  
      amd_iommu_msi_enable(iommu, IOMMU_CONTROL_ENABLED);
@@ -879,7 +949,9 @@  static void disable_iommu(struct amd_iom
          return;
      }
  
-    amd_iommu_msi_enable(iommu, IOMMU_CONTROL_DISABLED);
+    if ( !iommu->ctrl.int_cap_xt_en )
+        amd_iommu_msi_enable(iommu, IOMMU_CONTROL_DISABLED);
+
      set_iommu_command_buffer_control(iommu, IOMMU_CONTROL_DISABLED);
      set_iommu_event_log_control(iommu, IOMMU_CONTROL_DISABLED);
  
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
@@ -416,6 +416,25 @@  union amd_iommu_ext_features {
      } flds;
  };
  
+/* x2APIC Control Registers */
+#define IOMMU_XT_INT_CTRL_MMIO_OFFSET		0x0170
+#define IOMMU_XT_PPR_INT_CTRL_MMIO_OFFSET	0x0178
+#define IOMMU_XT_GA_INT_CTRL_MMIO_OFFSET	0x0180
+
+union amd_iommu_x2apic_control {
+    uint64_t raw;
+    struct {
+        unsigned int :2;
+        unsigned int dest_mode:1;
+        unsigned int :5;
+        unsigned int dest_lo:24;
+        unsigned int vector:8;
+        unsigned int int_type:1; /* DM in IOMMU spec 3.04 */
+        unsigned int :15;
+        unsigned int dest_hi:8;
+    };
+};
+
  /* Status Register*/
  #define IOMMU_STATUS_MMIO_OFFSET		0x2020
  #define IOMMU_STATUS_EVENT_OVERFLOW_MASK	0x00000001