Xen devel - Sep 2012 - [PATCH] x86/MSI: fix 2nd S3 resume with interrupt remapping enabled

The first resume from S3 was corrupting internal data structures (in
that pci_restore_msi_state() updated the globally stored MSI message
from traditional to interrupt remapped format, which would then be
translated a second time during the second resume, breaking interrupt
delivery).

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

--- 2012-08-08.orig/xen/arch/x86/msi.c	2012-09-07 13:39:02.000000000 +0200
+++ 2012-08-08/xen/arch/x86/msi.c	2012-09-06 12:04:11.000000000 +0200
@@ -210,7 +210,10 @@ static void write_msi_msg(struct msi_des
     entry->msg = *msg;
 
     if ( iommu_enabled )
+    {
+        ASSERT(msg != &entry->msg);
         iommu_update_ire_from_msi(entry, msg);
+    }
 
     switch ( entry->msi_attrib.type )
     {
@@ -996,6 +999,7 @@ int pci_restore_msi_state(struct pci_dev
     int ret;
     struct msi_desc *entry, *tmp;
     struct irq_desc *desc;
+    struct msi_msg msg;
 
     ASSERT(spin_is_locked(&pcidevs_lock));
 
@@ -1030,7 +1034,8 @@ int pci_restore_msi_state(struct pci_dev
         else if ( entry->msi_attrib.type == PCI_CAP_ID_MSIX )
             msix_set_enable(pdev, 0);
 
-        write_msi_msg(entry, &entry->msg);
+        msg = entry->msg;
+        write_msi_msg(entry, &msg);
 
         msi_set_mask_bit(desc, entry->msi_attrib.masked);
 





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

On 07/09/2012 13:49, "Jan Beulich" <JBeulich@suse.com> wrote:
> The first resume from S3 was corrupting internal data structures (in
> that pci_restore_msi_state() updated the globally stored MSI message
> from traditional to interrupt remapped format, which would then be
> translated a second time during the second resume, breaking interrupt
> delivery).
Doesn''t that mean write_msi_msg() has a bit of a hideous interface?
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Keir Fraser <keir@xen.org>

Looks pretty safe for 4.2.0.
> --- 2012-08-08.orig/xen/arch/x86/msi.c 2012-09-07 13:39:02.000000000 +0200
> +++ 2012-08-08/xen/arch/x86/msi.c 2012-09-06 12:04:11.000000000 +0200
> @@ -210,7 +210,10 @@ static void write_msi_msg(struct msi_des
>      entry->msg = *msg;
>  
>      if ( iommu_enabled )
> +    {
> +        ASSERT(msg != &entry->msg);
>          iommu_update_ire_from_msi(entry, msg);
> +    }
>  
>      switch ( entry->msi_attrib.type )
>      {
> @@ -996,6 +999,7 @@ int pci_restore_msi_state(struct pci_dev
>      int ret;
>      struct msi_desc *entry, *tmp;
>      struct irq_desc *desc;
> +    struct msi_msg msg;
>  
>      ASSERT(spin_is_locked(&pcidevs_lock));
>  
> @@ -1030,7 +1034,8 @@ int pci_restore_msi_state(struct pci_dev
>          else if ( entry->msi_attrib.type == PCI_CAP_ID_MSIX )
>              msix_set_enable(pdev, 0);
>  
> -        write_msi_msg(entry, &entry->msg);
> +        msg = entry->msg;
> +        write_msi_msg(entry, &msg);
>  
>          msi_set_mask_bit(desc, entry->msi_attrib.masked);
>  
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

>>> On 07.09.12 at 16:44, Keir Fraser <keir.xen@gmail.com> wrote:
> On 07/09/2012 13:49, "Jan Beulich" <JBeulich@suse.com>
wrote:
> 
>> The first resume from S3 was corrupting internal data structures (in
>> that pci_restore_msi_state() updated the globally stored MSI message
>> from traditional to interrupt remapped format, which would then be
>> translated a second time during the second resume, breaking interrupt
>> delivery).
> 
> Doesn''t that mean write_msi_msg() has a bit of a hideous
interface?
It does, and we may want to clean that up. But at least this won''t
go unnoticed in debug builds anymore with the added assertion,
should anyone re-add a bad use of it.

Jan