Re: [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending

From: "Xu, Quan" <quan.xu@intel.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: "Tian, Kevin" <kevin.tian@intel.com>,
	"Wu, Feng" <feng.wu@intel.com>,
	Liu Jinsong <jinsong.liu@alibaba-inc.com>,
	"dario.faggioli@citrix.com" <dario.faggioli@citrix.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>,
	Julien Grall <julien.grall@arm.com>,
	Stefano Stabellini <stefano.stabellini@citrix.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Keir Fraser <keir@xen.org>
Subject: Re: [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
Date: Tue, 26 Apr 2016 11:28:18 +0000	[thread overview]
Message-ID: <945CA011AD5F084CBEA3E851C0AB28894B8947B9@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <571E213802000078000E54E5@prv-mh.provo.novell.com>

On April 25, 2016 7:53 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> > --- a/xen/arch/x86/acpi/power.c
> > +++ b/xen/arch/x86/acpi/power.c
> > @@ -45,19 +45,31 @@ void do_suspend_lowlevel(void);
> >
> >  static int device_power_down(void)
> >  {
> > +    int err;
> > +
> >      console_suspend();
> >
> >      time_suspend();
> >
> >      i8259A_suspend();
> >
> > -    ioapic_suspend();
> > +    err = iommu_suspend();
> > +    if ( err )
> > +        goto iommu_suspend_error;
> >
> >      iommu_suspend();
> 
> A little more care please - this is definitely not what you meant.
> 

Very sorry for this..:(

> > + iommu_suspend_error:
> > +    ioapic_resume();
> > +    i8259A_resume();
> > +    time_resume();
> > +    console_resume();
> > +
> > +    return err;
> 
> Instead of mostly repeating what device_power_up() does I wonder whether
> it wouldn't be better to re-use that function for the error path here. Either by
> suitably making information available to device_power_up() to skip the call to
> lapic_resume() or by making lapic_resume() itself recognize it got called
> without
> lapic_suspend() having got called first.
> 

To be honest, now this modification is much more readable to me, however I will reconsider your suggestion.

Quan

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