| Message ID | 1461921917-48394-10-git-send-email-quan.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> 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.
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. Quan
>>> 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
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. > Got it, I will fix it in next v4. Quan
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; } static void device_power_up(void) diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c index 4536106..02588aa 100644 --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -1339,12 +1339,19 @@ static void invalidate_all_devices(void) iterate_ivrs_mappings(_invalidate_all_devices); } -void amd_iommu_suspend(void) +int amd_iommu_suspend(void) { struct amd_iommu *iommu; for_each_amd_iommu ( iommu ) disable_iommu(iommu); + + return 0; +} + +void amd_iommu_crash_shutdown(void) +{ + amd_iommu_suspend(); } void amd_iommu_resume(void) diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c index c8ee8dc..fb8e816 100644 --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c @@ -628,6 +628,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 839852f..c565c66 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -407,11 +407,14 @@ int iommu_do_domctl( return ret; } -void iommu_suspend() +int __must_check iommu_suspend() { const struct iommu_ops *ops = iommu_get_ops(); + if ( iommu_enabled ) - ops->suspend(); + return ops->suspend(); + + return 0; } void iommu_share_p2m_table(struct domain* d) diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index bf8c9c1..cf847ec 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -542,11 +542,12 @@ static int iommu_flush_iotlb_psi( return status; } -static void iommu_flush_all(void) +static int __must_check iommu_flush_all(void) { struct acpi_drhd_unit *drhd; struct iommu *iommu; int flush_dev_iotlb; + int rc = 0, ret; flush_all_cache(); for_each_drhd_unit ( drhd ) @@ -554,8 +555,13 @@ static void iommu_flush_all(void) iommu = drhd->iommu; iommu_flush_context_global(iommu, 0); flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0; - iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb); + ret = iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb); + + if ( ret ) + rc = ret; } + + return rc; } static int iommu_flush_iotlb(struct domain *d, unsigned long gfn, @@ -1272,7 +1278,9 @@ static void __hwdom_init intel_iommu_hwdom_init(struct domain *d) setup_hwdom_pci_devices(d, setup_hwdom_device); setup_hwdom_rmrr(d); - iommu_flush_all(); + if ( iommu_flush_all() ) + printk(XENLOG_WARNING VTDPREFIX + " intel_iommu_hwdom_init: IOMMU flush all failed.\n"); for_each_drhd_unit ( drhd ) { @@ -2134,8 +2142,8 @@ static int init_vtd_hw(void) return -EIO; } } - iommu_flush_all(); - return 0; + + return iommu_flush_all(); } static void __hwdom_init setup_hwdom_rmrr(struct domain *d) @@ -2424,16 +2432,25 @@ static int intel_iommu_group_id(u16 seg, u8 bus, u8 devfn) } static u32 iommu_state[MAX_IOMMUS][MAX_IOMMU_REGS]; -static void vtd_suspend(void) + +static int vtd_suspend(void) { struct acpi_drhd_unit *drhd; struct iommu *iommu; u32 i; + int rc; if ( !iommu_enabled ) - return; + return 0; - iommu_flush_all(); + rc = iommu_flush_all(); + + if ( rc ) + { + printk(XENLOG_WARNING VTDPREFIX + " vtd_suspend: IOMMU flush all failed.\n"); + return rc; + } for_each_drhd_unit ( drhd ) { @@ -2462,6 +2479,8 @@ static void vtd_suspend(void) if ( !iommu_intremap && iommu_qinval ) disable_qinval(iommu); } + + return 0; } static void vtd_crash_shutdown(void) @@ -2472,7 +2491,9 @@ static void vtd_crash_shutdown(void) if ( !iommu_enabled ) return; - iommu_flush_all(); + if ( iommu_flush_all() ) + printk(XENLOG_WARNING VTDPREFIX + " vtd_crash_shutdown: IOMMU flush all failed.\n"); for_each_drhd_unit ( drhd ) { diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h index 9c51172..f540fc8 100644 --- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h @@ -119,7 +119,7 @@ extern unsigned long *shared_intremap_inuse; /* power management support */ void amd_iommu_resume(void); -void amd_iommu_suspend(void); +int amd_iommu_suspend(void); void amd_iommu_crash_shutdown(void); /* guest iommu support */ diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index fe52428..dfa52ef 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -157,7 +157,7 @@ struct iommu_ops { unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int reg); int (*setup_hpet_msi)(struct msi_desc *); #endif /* CONFIG_X86 */ - void (*suspend)(void); + int (*suspend)(void); void (*resume)(void); void (*share_p2m)(struct domain *d); void (*crash_shutdown)(void); @@ -167,7 +167,7 @@ struct iommu_ops { void (*dump_p2m_table)(struct domain *d); }; -void iommu_suspend(void); +int __must_check iommu_suspend(void); void iommu_resume(void); void iommu_crash_shutdown(void); int iommu_get_reserved_device_memory(iommu_grdm_t *, void *);
Propagate the IOMMU Device-TLB flush error up to IOMMU suspending. Signed-off-by: Quan Xu <quan.xu@intel.com> CC: Jan Beulich <jbeulich@suse.com> CC: Liu Jinsong <jinsong.liu@alibaba-inc.com> CC: Keir Fraser <keir@xen.org> CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien.grall@arm.com> CC: Kevin Tian <kevin.tian@intel.com> CC: Feng Wu <feng.wu@intel.com> --- xen/arch/x86/acpi/power.c | 15 ++++++++++- xen/drivers/passthrough/amd/iommu_init.c | 9 ++++++- xen/drivers/passthrough/amd/pci_amd_iommu.c | 2 +- xen/drivers/passthrough/iommu.c | 7 +++-- xen/drivers/passthrough/vtd/iommu.c | 39 ++++++++++++++++++++------- xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 2 +- xen/include/xen/iommu.h | 4 +-- 7 files changed, 61 insertions(+), 17 deletions(-)