diff mbox

[v3,09/10] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending

Message ID 945CA011AD5F084CBEA3E851C0AB28894B8A94D5@SHSMSX101.ccr.corp.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Quan Xu May 5, 2016, 10:18 a.m. UTC 
On May 04, 2016 4:43 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 04.05.16 at 04:14, <quan.xu@intel.com> wrote:
> > On May 04, 2016 10:00 AM, Tian, Kevin <kevin.tian@intel.com> wrote:
> >> > From: Xu, Quan
> >> > Sent: Friday, April 29, 2016 5:25 PM diff --git
> >> > a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c index
> >> > 2885e31..9097333 100644
> >> > --- a/xen/arch/x86/acpi/power.c
> >> > +++ b/xen/arch/x86/acpi/power.c
> >> > @@ -45,6 +45,8 @@ void do_suspend_lowlevel(void);
> >> >
> >> >  static int device_power_down(void)  {
> >> > +    int err;
> >> > +
> >> >      console_suspend();
> >> >
> >> >      time_suspend();
> >> > @@ -53,11 +55,22 @@ static int device_power_down(void)
> >> >
> >> >      ioapic_suspend();
> >> >
> >> > -    iommu_suspend();
> >> > +    err = iommu_suspend();
> >> > +
> >> > +    if ( err )
> >> > +        goto iommu_suspend_error;
> >> >
> >> >      lapic_suspend();
> >> >
> >> >      return 0;
> >> > +
> >> > + iommu_suspend_error:
> >> > +    ioapic_resume();
> >> > +    i8259A_resume();
> >> > +    time_resume();
> >> > +    console_resume();
> >> > +
> >> > +    return err;
> >> >  }
> >>
> >> Jan had comment to better reuse device_power_up... looks no change in
> >> this version.
> >
> > Yes,  __iiuc__, this may be an optimization, but not a must.
> > We can discuss this in detail In this version.
> 
> As an optimization it would indeed be quite pointless here. My request was
> more for maintainability: By re-using the function future changes don't need
> to go to two places, and hence there's no risk of one of them getting
> forgotten.
> 

Jan,
What about this one as below:



--
Quan

Comments

Jan Beulich May 6, 2016, 7:02 a.m. UTC | #1 
>>> On 05.05.16 at 12:18, <quan.xu@intel.com> wrote:
> What about this one as below:

If done properly (coding style, comments to silence Coverity,
consistency throughout not just your change, but its effects on
the rest of the code in that file), that's certainly an option.

Jan

> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -43,6 +43,17 @@ struct acpi_sleep_info acpi_sinfo;
> 
>  void do_suspend_lowlevel(void);
> 
> +enum dev_power_type {
> +    TYPE_ALL,
> +    TYPE_CONSOLE,
> +    TYPE_TIME,
> +    TYPE_I8259A,
> +    TYPE_IOAPIC,
> +    TYPE_IOMMU,
> +    TYPE_LAPIC,
> +    TYPE_UNKNOWN
> +};
> +
>  static int device_power_down(void)
>  {
>      console_suspend();
> @@ -53,26 +64,35 @@ static int device_power_down(void)
> 
>      ioapic_suspend();
> 
> -    iommu_suspend();
> +    if ( iommu_suspend() )
> +        return TYPE_IOMMU;
> 
>      lapic_suspend();
> 
>      return 0;
>  }
> 
> -static void device_power_up(void)
> +static void device_power_up(enum dev_power_type type)
>  {
> -    lapic_resume();
> -
> -    iommu_resume();
> -
> -    ioapic_resume();
> -
> -    i8259A_resume();
> -
> -    time_resume();
> -
> -    console_resume();
> +    switch ( type ) {
> +    case TYPE_ALL:
> +    case TYPE_LAPIC:
> +        lapic_resume();
> +    case TYPE_IOMMU:
> +        iommu_resume();
> +    case TYPE_IOAPIC:
> +        ioapic_resume();
> +    case TYPE_I8259A:
> +        i8259A_resume();
> +    case TYPE_TIME:
> +        time_resume();
> +    case TYPE_CONSOLE:
> +        console_resume();
> +        break;
> +    default:
> +        BUG();
> +        break;
> +    }
>  }
> 
>  static void freeze_domains(void)
> @@ -169,6 +189,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 +217,7 @@ static int enter_state(u32 state)
>      write_cr4(cr4 & ~X86_CR4_MCE);
>      write_efer(read_efer());
> 
> -    device_power_up();
> +    device_power_up(TYPE_ALL);
> 
>      mcheck_init(&boot_cpu_data, 0);
>      write_cr4(cr4);
> 
> --
> Quan
Quan Xu May 6, 2016, 7:20 a.m. UTC | #2 
On May 06, 2016 3:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 05.05.16 at 12:18, <quan.xu@intel.com> wrote:
> > What about this one as below:
> 
> If done properly (coding style, comments to silence Coverity, consistency
> throughout not just your change, but its effects on the rest of the code in that
> file), that's certainly an option.
> 

Thanks. I'll try it in next v4.

-Quan
diff mbox

Patch

--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -43,6 +43,17 @@  struct acpi_sleep_info acpi_sinfo;

 void do_suspend_lowlevel(void);

+enum dev_power_type {
+    TYPE_ALL,
+    TYPE_CONSOLE,
+    TYPE_TIME,
+    TYPE_I8259A,
+    TYPE_IOAPIC,
+    TYPE_IOMMU,
+    TYPE_LAPIC,
+    TYPE_UNKNOWN
+};
+
 static int device_power_down(void)
 {
     console_suspend();
@@ -53,26 +64,35 @@  static int device_power_down(void)

     ioapic_suspend();

-    iommu_suspend();
+    if ( iommu_suspend() )
+        return TYPE_IOMMU;

     lapic_suspend();

     return 0;
 }

-static void device_power_up(void)
+static void device_power_up(enum dev_power_type type)
 {
-    lapic_resume();
-
-    iommu_resume();
-
-    ioapic_resume();
-
-    i8259A_resume();
-
-    time_resume();
-
-    console_resume();
+    switch ( type ) {
+    case TYPE_ALL:
+    case TYPE_LAPIC:
+        lapic_resume();
+    case TYPE_IOMMU:
+        iommu_resume();
+    case TYPE_IOAPIC:
+        ioapic_resume();
+    case TYPE_I8259A:
+        i8259A_resume();
+    case TYPE_TIME:
+        time_resume();
+    case TYPE_CONSOLE:
+        console_resume();
+        break;
+    default:
+        BUG();
+        break;
+    }
 }

 static void freeze_domains(void)
@@ -169,6 +189,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 +217,7 @@  static int enter_state(u32 state)
     write_cr4(cr4 & ~X86_CR4_MCE);
     write_efer(read_efer());

-    device_power_up();
+    device_power_up(TYPE_ALL);

     mcheck_init(&boot_cpu_data, 0);
     write_cr4(cr4);