Re: [Xen-devel] [PATCH v7 07/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending (top level ones)

[Jan Beulich]

>>> On 12.06.16 at 09:42, <quan...@intel.com> wrote:

> 
>> -Original Message-
>> From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of Jan
>> Beulich
>> Sent: Wednesday, June 08, 2016 10:52 PM
>> To: Xu, Quan <quan...@intel.com>
>> Cc: Tian, Kevin <kevin.t...@intel.com>; Stefano Stabellini
>> <sstabell...@kernel.org>; Wu, Feng <feng...@intel.com>; Liu Jinsong
>> <jinsong@alibaba-inc.com>; dario.faggi...@citrix.com; xen-
>> de...@lists.xen.org; Julien Grall <julien.gr...@arm.com>; Suravee
>> Suthikulpanit <suravee.suthikulpa...@amd.com>; Andrew Cooper
>> <andrew.coop...@citrix.com>; Keir Fraser <k...@xen.org>
>> Subject: Re: [Xen-devel] [PATCH v7 07/11] IOMMU: propagate IOMMU
>> Device-TLB flush error up to IOMMU suspending (top level ones)
>> 
> 
> On 
>> >>> On 08.06.16 at 10:59, <quan...@intel.com> wrote:
>> > @@ -169,6 +203,7 @@ static int enter_state(u32 state)
>> 
>> Right above here we have
>> 
>> if ( (error = device_power_down()) )
>> 
>> which is now wrong as long as SAVED_ALL is not zero.
>> 
>> >  {
>> >  printk(XENLOG_ERR "Some devices failed to power down.");
>> >  system_state = SYS_STATE_resume;
>> > +device_power_up(error);
>> >  goto done;
>> 
>> For the goto you need to adjust "error", or else you return something
>> meaningless (a sort of random positive number) to your caller.
>> 
> 
> Yes, it is still not correct. Could I change it as following: 
> 
> 
> -if ( (error = device_power_down()) )
> +if ( (error = device_power_down()) != SAVED_ALL )
>  {
>  printk(XENLOG_ERR "Some devices failed to power down.");
>  system_state = SYS_STATE_resume;
> +device_power_up(error);
> +error = -EIO;
>  goto done;
>  }

This would address only part of the issue afaict - SAVED_ALL
not being zero would still result in the function returning a
positive value instead of zero in the success case. But to be
honest I don't see why this simple to solve an issue requires
any kind of discussion on how to deal with it.

Jan


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

Re: [Xen-devel] [PATCH v7 07/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending (top level ones)

[Xu, Quan]

> -Original Message-
> From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of Jan
> Beulich
> Sent: Wednesday, June 08, 2016 10:52 PM
> To: Xu, Quan <quan...@intel.com>
> Cc: Tian, Kevin <kevin.t...@intel.com>; Stefano Stabellini
> <sstabell...@kernel.org>; Wu, Feng <feng...@intel.com>; Liu Jinsong
> <jinsong@alibaba-inc.com>; dario.faggi...@citrix.com; xen-
> de...@lists.xen.org; Julien Grall <julien.gr...@arm.com>; Suravee
> Suthikulpanit <suravee.suthikulpa...@amd.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Keir Fraser <k...@xen.org>
> Subject: Re: [Xen-devel] [PATCH v7 07/11] IOMMU: propagate IOMMU
> Device-TLB flush error up to IOMMU suspending (top level ones)
> 

On 
> >>> On 08.06.16 at 10:59, <quan...@intel.com> wrote:
> > @@ -169,6 +203,7 @@ static int enter_state(u32 state)
> 
> Right above here we have
> 
> if ( (error = device_power_down()) )
> 
> which is now wrong as long as SAVED_ALL is not zero.
> 
> >  {
> >  printk(XENLOG_ERR "Some devices failed to power down.");
> >  system_state = SYS_STATE_resume;
> > +device_power_up(error);
> >  goto done;
> 
> For the goto you need to adjust "error", or else you return something
> meaningless (a sort of random positive number) to your caller.
> 

Yes, it is still not correct. Could I change it as following: 


-if ( (error = device_power_down()) )
+if ( (error = device_power_down()) != SAVED_ALL )
 {
 printk(XENLOG_ERR "Some devices failed to power down.");
 system_state = SYS_STATE_resume;
+device_power_up(error);
+error = -EIO;
 goto done;
 }

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

Re: [Xen-devel] [PATCH v7 07/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending (top level ones)

[Tian, Kevin]

> From: Xu, Quan
> Sent: Wednesday, June 08, 2016 4:59 PM
> 
> From: Quan Xu 
> 
> Signed-off-by: Quan Xu 
> 
> CC: Jan Beulich 
> CC: Liu Jinsong 
> CC: Keir Fraser 
> CC: Andrew Cooper 
> CC: Suravee Suthikulpanit 
> CC: Stefano Stabellini 
> CC: Julien Grall 
> CC: Kevin Tian 
> CC: Feng Wu 
> 

Acked-by: Kevin Tian 

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

Re: [Xen-devel] [PATCH v7 07/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending (top level ones)

[Suravee Suthikulanit]

On 6/8/2016 3:59 AM, Xu, Quan wrote:

From: Quan Xu 

Signed-off-by: Quan Xu 

CC: Jan Beulich 
CC: Liu Jinsong 
CC: Keir Fraser 
CC: Andrew Cooper 
CC: Suravee Suthikulpanit 
CC: Stefano Stabellini 
CC: Julien Grall 
CC: Kevin Tian 
CC: Feng Wu 

v7:
  1. return SAVED_ALL at the bottom of device_power_down(), instead
 of SAVED_NONE.
  2. drop the 'if ( error > 0 )', calling device_power_up(error)
 without any if().
  3. for vtd_suspend():
   - drop pointless initializer.
   - return 0 at the bottom to make obvious that no error path
 comes there.


Shouldn't the changes log for v7 probably go ...

---

... HERE instead so that we don't get this in the commit log.


For AMD part,
Acked-by: Suravee Suthikulpanit 

Thanks,
Suravee

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

Re: [Xen-devel] [PATCH v7 07/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending (top level ones)

[Jan Beulich]

>>> On 08.06.16 at 10:59,  wrote:
> @@ -169,6 +203,7 @@ static int enter_state(u32 state)

Right above here we have

if ( (error = device_power_down()) )

which is now wrong as long as SAVED_ALL is not zero.

>  {
>  printk(XENLOG_ERR "Some devices failed to power down.");
>  system_state = SYS_STATE_resume;
> +device_power_up(error);
>  goto done;

For the goto you need to adjust "error", or else you return
something meaningless (a sort of random positive number) to your
caller.

Jan


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

[Xen-devel] [PATCH v7 07/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending (top level ones)

[Xu, Quan]

From: Quan Xu 

Signed-off-by: Quan Xu 

CC: Jan Beulich 
CC: Liu Jinsong 
CC: Keir Fraser 
CC: Andrew Cooper 
CC: Suravee Suthikulpanit 
CC: Stefano Stabellini 
CC: Julien Grall 
CC: Kevin Tian 
CC: Feng Wu 

v7:
  1. return SAVED_ALL at the bottom of device_power_down(), instead
 of SAVED_NONE.
  2. drop the 'if ( error > 0 )', calling device_power_up(error)
 without any if().
  3. for vtd_suspend():
   - drop pointless initializer.
   - return 0 at the bottom to make obvious that no error path
 comes there.
---
 xen/arch/x86/acpi/power.c | 73 ---
 xen/drivers/passthrough/amd/iommu_init.c  |  9 +++-
 xen/drivers/passthrough/amd/pci_amd_iommu.c   |  2 +-
 xen/drivers/passthrough/iommu.c   |  6 ++-
 xen/drivers/passthrough/vtd/iommu.c   | 35 +
 xen/include/asm-x86/hvm/svm/amd-iommu-proto.h |  2 +-
 xen/include/xen/iommu.h   |  4 +-
 7 files changed, 96 insertions(+), 35 deletions(-)

diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index 2885e31..717a809 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -43,36 +43,70 @@ struct acpi_sleep_info acpi_sinfo;
 
 void do_suspend_lowlevel(void);
 
+enum dev_power_saved
+{
+SAVED_NONE,
+SAVED_CONSOLE,
+SAVED_TIME,
+SAVED_I8259A,
+SAVED_IOAPIC,
+SAVED_IOMMU,
+SAVED_LAPIC,
+SAVED_ALL,
+};
+
 static int device_power_down(void)
 {
-console_suspend();
+if ( console_suspend() )
+return SAVED_NONE;
 
-time_suspend();
+if ( time_suspend() )
+return SAVED_CONSOLE;
 
-i8259A_suspend();
+if ( i8259A_suspend() )
+return SAVED_TIME;
 
+/* ioapic_suspend cannot fail */
 ioapic_suspend();
 
-iommu_suspend();
+if ( iommu_suspend() )
+return SAVED_IOAPIC;
 
-lapic_suspend();
+if ( lapic_suspend() )
+return SAVED_IOMMU;
 
-return 0;
+return SAVED_ALL;
 }
 
-static void device_power_up(void)
+static void device_power_up(enum dev_power_saved saved)
 {
-lapic_resume();
-
-iommu_resume();
-
-ioapic_resume();
-
-i8259A_resume();
-
-time_resume();
-
-console_resume();
+switch ( saved )
+{
+case SAVED_ALL:
+case SAVED_LAPIC:
+lapic_resume();
+/* fall through */
+case SAVED_IOMMU:
+iommu_resume();
+/* fall through */
+case SAVED_IOAPIC:
+ioapic_resume();
+/* fall through */
+case SAVED_I8259A:
+i8259A_resume();
+/* fall through */
+case SAVED_TIME:
+time_resume();
+/* fall through */
+case SAVED_CONSOLE:
+console_resume();
+/* fall through */
+case SAVED_NONE:
+break;
+default:
+BUG();
+break;
+}
 }
 
 static void freeze_domains(void)
@@ -169,6 +203,7 @@ static int enter_state(u32 state)
 {
 printk(XENLOG_ERR "Some devices failed to power down.");
 system_state = SYS_STATE_resume;
+device_power_up(error);
 goto done;
 }
 
@@ -196,7 +231,7 @@ static int enter_state(u32 state)
 write_cr4(cr4 & ~X86_CR4_MCE);
 write_efer(read_efer());
 
-device_power_up();
+device_power_up(SAVED_ALL);
 
 mcheck_init(_cpu_data, 0);
 write_cr4(cr4);
diff --git a/xen/drivers/passthrough/amd/iommu_init.c 
b/xen/drivers/passthrough/amd/iommu_init.c
index 4536106..0b68596 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -1339,7 +1339,14 @@ static void invalidate_all_devices(void)
 iterate_ivrs_mappings(_invalidate_all_devices);
 }
 
-void amd_iommu_suspend(void)
+int amd_iommu_suspend(void)
+{
+amd_iommu_crash_shutdown();
+
+return 0;
+}
+
+void amd_iommu_crash_shutdown(void)
 {
 struct amd_iommu *iommu;
 
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c 
b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 4a860af..7761241 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -633,6 +633,6 @@ 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_suspend,
+.crash_shutdown = amd_iommu_crash_shutdown,
 .dump_p2m_table = amd_dump_p2m_table,
 };
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 3a73fab..a9898fc 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -379,10 +379,12 @@ int __init iommu_setup(void)
 return rc;
 }
 
-void iommu_suspend()
+int iommu_suspend()
 {
 if ( iommu_enabled )
-