There's no need to save and restore the IO-APIC entries, the entries
prior to suspension have already been saved by ioapic_suspend, and
will be restored by ioapic_resume. Note that at the point where
resume_x2apic gets called the IO-APIC has not yet resumed, and hence
all entries should be masked.

Note this shouldn't introduce any functional change.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
I'm Ccing Marek since I think he usually tests suspend/resume. Could
you give this patch a try?
---
Cc: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Cc: Juergen Gross <jgross@suse.com>
---
Changes since v2:
 - New in this version.
---
 xen/arch/x86/apic.c | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index 6cdb50cf41..0607eb92a8 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -492,35 +492,8 @@ static void __enable_x2apic(void)
 
 static void resume_x2apic(void)
 {
-    struct IO_APIC_route_entry **ioapic_entries = NULL;
-
-    ASSERT(x2apic_enabled);
-
-    ioapic_entries = alloc_ioapic_entries();
-    if ( !ioapic_entries )
-    {
-        printk("Allocate ioapic_entries failed\n");
-        goto out;
-    }
-
-    if ( save_IO_APIC_setup(ioapic_entries) )
-    {
-        printk("Saving IO-APIC state failed\n");
-        goto out;
-    }
-
-    mask_8259A();
-    mask_IO_APIC_setup(ioapic_entries);
-
     iommu_enable_x2apic();
     __enable_x2apic();
-
-    restore_IO_APIC_setup(ioapic_entries);
-    unmask_8259A();
-
-out:
-    if ( ioapic_entries )
-        free_ioapic_entries(ioapic_entries);
 }
 
 void setup_local_APIC(void)
-- 
2.23.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On Tue, Oct 15, 2019 at 05:47:34PM +0200, Roger Pau Monne wrote:
> There's no need to save and restore the IO-APIC entries, the entries
> prior to suspension have already been saved by ioapic_suspend, and
> will be restored by ioapic_resume. Note that at the point where
> resume_x2apic gets called the IO-APIC has not yet resumed, and hence
> all entries should be masked.
> 
> Note this shouldn't introduce any functional change.
> 
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

I've tried host suspend without any domU running and it works. I've tested
just this patch without others in the series, does it matter?

Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

> ---
> I'm Ccing Marek since I think he usually tests suspend/resume. Could
> you give this patch a try?
> ---
> Cc: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Cc: Juergen Gross <jgross@suse.com>
> ---
> Changes since v2:
>  - New in this version.
> ---
>  xen/arch/x86/apic.c | 27 ---------------------------
>  1 file changed, 27 deletions(-)
> 
> diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
> index 6cdb50cf41..0607eb92a8 100644
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -492,35 +492,8 @@ static void __enable_x2apic(void)
>  
>  static void resume_x2apic(void)
>  {
> -    struct IO_APIC_route_entry **ioapic_entries = NULL;
> -
> -    ASSERT(x2apic_enabled);
> -
> -    ioapic_entries = alloc_ioapic_entries();
> -    if ( !ioapic_entries )
> -    {
> -        printk("Allocate ioapic_entries failed\n");
> -        goto out;
> -    }
> -
> -    if ( save_IO_APIC_setup(ioapic_entries) )
> -    {
> -        printk("Saving IO-APIC state failed\n");
> -        goto out;
> -    }
> -
> -    mask_8259A();
> -    mask_IO_APIC_setup(ioapic_entries);
> -
>      iommu_enable_x2apic();
>      __enable_x2apic();
> -
> -    restore_IO_APIC_setup(ioapic_entries);
> -    unmask_8259A();
> -
> -out:
> -    if ( ioapic_entries )
> -        free_ioapic_entries(ioapic_entries);
>  }
>  
>  void setup_local_APIC(void)

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On Thu, Oct 17, 2019 at 10:26:08AM +0200, Marek Marczykowski-Górecki wrote:
> On Tue, Oct 15, 2019 at 05:47:34PM +0200, Roger Pau Monne wrote:
> > There's no need to save and restore the IO-APIC entries, the entries
> > prior to suspension have already been saved by ioapic_suspend, and
> > will be restored by ioapic_resume. Note that at the point where
> > resume_x2apic gets called the IO-APIC has not yet resumed, and hence
> > all entries should be masked.
> > 
> > Note this shouldn't introduce any functional change.
> > 
> > Suggested-by: Jan Beulich <jbeulich@suse.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> I've tried host suspend without any domU running and it works. I've tested
> just this patch without others in the series, does it matter?

No that's fine.

> Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Thanks.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 15.10.2019 17:47, Roger Pau Monne wrote:
> There's no need to save and restore the IO-APIC entries, the entries
> prior to suspension have already been saved by ioapic_suspend, and
> will be restored by ioapic_resume. Note that at the point where
> resume_x2apic gets called the IO-APIC has not yet resumed, and hence
> all entries should be masked.
> 
> Note this shouldn't introduce any functional change.
> 
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel