| Message ID | 1460988011-17758-8-git-send-email-quan.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote: > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -678,8 +678,9 @@ static int xenmem_add_to_physmap(struct domain *d, > if ( need_iommu(d) ) > { > this_cpu(iommu_dont_flush_iotlb) = 0; > - iommu_iotlb_flush(d, xatp->idx - done, done); > - iommu_iotlb_flush(d, xatp->gpfn - done, done); > + rc = iommu_iotlb_flush(d, xatp->idx - done, done); > + if ( !rc ) > + rc = iommu_iotlb_flush(d, xatp->gpfn - done, done); And again - best effort flushing in all cases. I.e. you shouldn't bypass the second one if the first one failed, but instead properly accumulate the return value, and also again not clobber any negative value rc may already hold. What's worse (and makes me wonder whether this got tested) - you also clobber the positive return value this function may produce, even in the case the flushes succeed (and hence the function as a whole was successful). > --- a/xen/include/xen/iommu.h > +++ b/xen/include/xen/iommu.h > @@ -182,8 +182,8 @@ int iommu_do_pci_domctl(struct xen_domctl *, struct domain *d, > int iommu_do_domctl(struct xen_domctl *, struct domain *d, > XEN_GUEST_HANDLE_PARAM(xen_domctl_t)); > > -void iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count); > -void iommu_iotlb_flush_all(struct domain *d); > +int iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count); > +int iommu_iotlb_flush_all(struct domain *d); __must_check Jan
On April 25, 2016 6:19 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote: > > --- a/xen/common/memory.c > > +++ b/xen/common/memory.c > > @@ -678,8 +678,9 @@ static int xenmem_add_to_physmap(struct domain > *d, > > if ( need_iommu(d) ) > > { > > this_cpu(iommu_dont_flush_iotlb) = 0; > > - iommu_iotlb_flush(d, xatp->idx - done, done); > > - iommu_iotlb_flush(d, xatp->gpfn - done, done); > > + rc = iommu_iotlb_flush(d, xatp->idx - done, done); > > + if ( !rc ) > > + rc = iommu_iotlb_flush(d, xatp->gpfn - done, done); > > And again - best effort flushing in all cases. I.e. you shouldn't bypass the > second one if the first one failed, but instead properly accumulate the return > value, and also again not clobber any negative value rc may already hold. Thanks for correcting me. I did like accumulating the return value. :( I will follow your suggestion in next v3. > What's worse (and makes me wonder whether this got tested) - you also > clobber the positive return value this function may produce, even in the case > the flushes succeed (and hence the function as a whole was successful). > I have tested it with previous patches (i.e. 1st patch), launching a VM with PT ATS device to invoke this call tree. btw, I fix the positive issue in 1st patch. I know this is not strict, as I assume this patch set will be committed at the same time. > > --- a/xen/include/xen/iommu.h > > +++ b/xen/include/xen/iommu.h > > @@ -182,8 +182,8 @@ int iommu_do_pci_domctl(struct xen_domctl *, > > struct domain *d, int iommu_do_domctl(struct xen_domctl *, struct domain > *d, > > XEN_GUEST_HANDLE_PARAM(xen_domctl_t)); > > > > -void iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned > > int page_count); -void iommu_iotlb_flush_all(struct domain *d); > > +int iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned > > +int page_count); int iommu_iotlb_flush_all(struct domain *d); > > __must_check > Agreed. Quan
>>> On 26.04.16 at 14:23, <quan.xu@intel.com> wrote: > On April 25, 2016 6:19 PM, Jan Beulich <JBeulich@suse.com> wrote: >> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote: >> > --- a/xen/common/memory.c >> > +++ b/xen/common/memory.c >> > @@ -678,8 +678,9 @@ static int xenmem_add_to_physmap(struct domain >> *d, >> > if ( need_iommu(d) ) >> > { >> > this_cpu(iommu_dont_flush_iotlb) = 0; >> > - iommu_iotlb_flush(d, xatp->idx - done, done); >> > - iommu_iotlb_flush(d, xatp->gpfn - done, done); >> > + rc = iommu_iotlb_flush(d, xatp->idx - done, done); >> > + if ( !rc ) >> > + rc = iommu_iotlb_flush(d, xatp->gpfn - done, done); >> >> And again - best effort flushing in all cases. I.e. you shouldn't bypass the >> second one if the first one failed, but instead properly accumulate the return >> value, and also again not clobber any negative value rc may already hold. > > > Thanks for correcting me. I did like accumulating the return value. :( > I will follow your suggestion in next v3. > >> What's worse (and makes me wonder whether this got tested) - you also >> clobber the positive return value this function may produce, even in the case >> the flushes succeed (and hence the function as a whole was successful). >> > I have tested it with previous patches (i.e. 1st patch), launching a VM > with PT ATS device to invoke this call tree. > btw, I fix the positive issue in 1st patch. > I know this is not strict, as I assume this patch set will be committed at > the same time. Hmm, the "positive" here has nothing to do with the "positive" in patch 1. Please just have a look at xenmem_add_to_physmap() as a whole. Jan
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index a2a9c4b..11d2c02 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1177,7 +1177,10 @@ out: if ( flush ) { flush_tlb_domain(d); - iommu_iotlb_flush(d, sgfn, egfn - sgfn); + ret = iommu_iotlb_flush(d, sgfn, egfn - sgfn); + + if ( ret ) + rc = ret; } while ( (pg = page_list_remove_head(&free_pages)) ) diff --git a/xen/common/memory.c b/xen/common/memory.c index c7fca96..965bd51 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -678,8 +678,9 @@ static int xenmem_add_to_physmap(struct domain *d, if ( need_iommu(d) ) { this_cpu(iommu_dont_flush_iotlb) = 0; - iommu_iotlb_flush(d, xatp->idx - done, done); - iommu_iotlb_flush(d, xatp->gpfn - done, done); + rc = iommu_iotlb_flush(d, xatp->idx - done, done); + if ( !rc ) + rc = iommu_iotlb_flush(d, xatp->gpfn - done, done); } #endif diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index c351209..9254760 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -301,24 +301,28 @@ static void iommu_free_pagetables(unsigned long unused) cpumask_cycle(smp_processor_id(), &cpu_online_map)); } -void iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count) +int iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count) { struct hvm_iommu *hd = domain_hvm_iommu(d); if ( !iommu_enabled || !hd->platform_ops || !hd->platform_ops->iotlb_flush ) - return; + return 0; hd->platform_ops->iotlb_flush(d, gfn, page_count); + + return 0; } -void iommu_iotlb_flush_all(struct domain *d) +int iommu_iotlb_flush_all(struct domain *d) { struct hvm_iommu *hd = domain_hvm_iommu(d); if ( !iommu_enabled || !hd->platform_ops || !hd->platform_ops->iotlb_flush_all ) - return; + return 0; hd->platform_ops->iotlb_flush_all(d); + + return 0; } int __init iommu_setup(void) diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c index 8cbb655..c2c1ee3 100644 --- a/xen/drivers/passthrough/x86/iommu.c +++ b/xen/drivers/passthrough/x86/iommu.c @@ -104,8 +104,9 @@ int arch_iommu_populate_page_table(struct domain *d) this_cpu(iommu_dont_flush_iotlb) = 0; if ( !rc ) - iommu_iotlb_flush_all(d); - else if ( rc != -ERESTART ) + rc = iommu_iotlb_flush_all(d); + + if ( rc && rc != -ERESTART ) iommu_teardown(d); return rc; diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 8217cb7..ff4608d 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -182,8 +182,8 @@ int iommu_do_pci_domctl(struct xen_domctl *, struct domain *d, int iommu_do_domctl(struct xen_domctl *, struct domain *d, XEN_GUEST_HANDLE_PARAM(xen_domctl_t)); -void iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count); -void iommu_iotlb_flush_all(struct domain *d); +int iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count); +int iommu_iotlb_flush_all(struct domain *d); /* * The purpose of the iommu_dont_flush_iotlb optional cpu flag is to
While IOMMU Device-TLB flush timed out, xen calls panic() at present. However the existing panic() is going to be eliminated, so we must propagate the IOMMU Device-TLB flush error up to the iommu_iotlb_flush{,_all}. If IOMMU mapping and unmapping failed, the domain (with the exception of the hardware domain) is crashed, treated as a fatal error. Rollback can be lighter weight. This patch fixes the top level ones (this patch doesn't fix the leaf ones but the next patch does). Signed-off-by: Quan Xu <quan.xu@intel.com> CC: Stefano Stabellini <stefano.stabellini@citrix.com> CC: Julien Grall <julien.grall@arm.com> CC: Jan Beulich <jbeulich@suse.com> --- xen/arch/arm/p2m.c | 5 ++++- xen/common/memory.c | 5 +++-- xen/drivers/passthrough/iommu.c | 12 ++++++++---- xen/drivers/passthrough/x86/iommu.c | 5 +++-- xen/include/xen/iommu.h | 4 ++-- 5 files changed, 20 insertions(+), 11 deletions(-)