Re: [Xen-devel] [PATCH v3 09/10] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending

["Xu, Quan" <quan.xu@xxxxxxxxx>]

On May 04, 2016 4:43 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 04.05.16 at 04:14, <quan.xu@xxxxxxxxx> wrote:
> > On May 04, 2016 10:00 AM, Tian, Kevin <kevin.tian@xxxxxxxxx> 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:


--- 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




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