| Message ID | 1460988011-17758-5-git-send-email-quan.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> From: Quan Xu > Sent: Monday, April 18, 2016 10:00 PM > > While grant table is unmapping, the domain (with the exception of the unmapping -> unmapped. > hardware domain) may be crashed due to IOMMU mapping and unmapping > failures, and then it is unnecessary to flush specified CPUs' TLBs. Above description is not complete. You said "with the exception of the hardware domain". Then people will ask whether 'unnecessary' is also true for hardware domain (if not restriction for hardware domain to invoke those interfaces). > > Signed-off-by: Quan Xu <quan.xu@intel.com> > > CC: Ian Jackson <ian.jackson@eu.citrix.com> > CC: Jan Beulich <jbeulich@suse.com> > CC: Keir Fraser <keir@xen.org> > CC: Tim Deegan <tim@xen.org> > --- > xen/common/grant_table.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c > index 8b22299..1801fe9 100644 > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -1366,8 +1366,9 @@ gnttab_unmap_grant_ref( > > return 0; > > -fault: > - gnttab_flush_tlb(current->domain); > + fault: > + if ( current->domain->is_shut_down ) > + gnttab_flush_tlb(current->domain); > > for ( i = 0; i < partial_done; i++ ) > __gnttab_unmap_common_complete(&(common[i])); > @@ -1429,8 +1430,9 @@ gnttab_unmap_and_replace( > > return 0; > > -fault: > - gnttab_flush_tlb(current->domain); > + fault: > + if ( current->domain->is_shut_down ) > + gnttab_flush_tlb(current->domain); > > for ( i = 0; i < partial_done; i++ ) > __gnttab_unmap_common_complete(&(common[i])); > -- > 1.9.1 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On April 19, 2016 2:46pm, Tian, Kevin <kevin.tian@intel.com> wrote: > > From: Quan Xu > > Sent: Monday, April 18, 2016 10:00 PM > > > > While grant table is unmapping, the domain (with the exception of the > > unmapping -> unmapped. > A slightly different take, IMO the hypercall is not returned, so it is DOING. > > hardware domain) may be crashed due to IOMMU mapping and unmapping > > failures, and then it is unnecessary to flush specified CPUs' TLBs. > > Above description is not complete. You said "with the exception of the > hardware domain". Then people will ask whether 'unnecessary' > is also true for hardware domain (if not restriction for hardware domain to > invoke those interfaces). > Make sense. Could I only drop '(with the exception of the hardware domain)'? > > > > Signed-off-by: Quan Xu <quan.xu@intel.com> > > > > CC: Ian Jackson <ian.jackson@eu.citrix.com> > > CC: Jan Beulich <jbeulich@suse.com> > > CC: Keir Fraser <keir@xen.org> > > CC: Tim Deegan <tim@xen.org> > > --- > > xen/common/grant_table.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index > > 8b22299..1801fe9 100644 > > --- a/xen/common/grant_table.c > > +++ b/xen/common/grant_table.c > > @@ -1366,8 +1366,9 @@ gnttab_unmap_grant_ref( > > > > return 0; > > > > -fault: > > - gnttab_flush_tlb(current->domain); > > + fault: > > + if ( current->domain->is_shut_down ) > > + gnttab_flush_tlb(current->domain); > > > > for ( i = 0; i < partial_done; i++ ) > > __gnttab_unmap_common_complete(&(common[i])); > > @@ -1429,8 +1430,9 @@ gnttab_unmap_and_replace( > > > > return 0; > > > > -fault: > > - gnttab_flush_tlb(current->domain); > > + fault: > > + if ( current->domain->is_shut_down ) > > + gnttab_flush_tlb(current->domain); > > > > for ( i = 0; i < partial_done; i++ ) > > __gnttab_unmap_common_complete(&(common[i])); > > -- > > 1.9.1 > > > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > http://lists.xen.org/xen-devel
> From: Xu, Quan > Sent: Tuesday, April 19, 2016 9:28 PM > > On April 19, 2016 2:46pm, Tian, Kevin <kevin.tian@intel.com> wrote: > > > From: Quan Xu > > > Sent: Monday, April 18, 2016 10:00 PM > > > > > > While grant table is unmapping, the domain (with the exception of the > > > > unmapping -> unmapped. > > > > A slightly different take, IMO the hypercall is not returned, so it is DOING. More accurately you might want to say "When a granted page is being unmapped". > > > > > hardware domain) may be crashed due to IOMMU mapping and unmapping > > > failures, and then it is unnecessary to flush specified CPUs' TLBs. > > > > Above description is not complete. You said "with the exception of the > > hardware domain". Then people will ask whether 'unnecessary' > > is also true for hardware domain (if not restriction for hardware domain to > > invoke those interfaces). > > > > Make sense. > Could I only drop '(with the exception of the hardware domain)'? > Still didn't get the rationale here. After another look at the patch, actually I'm even not sure how the change is related to the purpose of this patch series. Also the change is opposite to your comment - you limit flush to crashed domain instead: > > -fault: > > - gnttab_flush_tlb(current->domain); > > + fault: > > + if ( current->domain->is_shut_down ) > > + gnttab_flush_tlb(current->domain); Thanks Kevin
>>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote: > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -1366,8 +1366,9 @@ gnttab_unmap_grant_ref( > > return 0; > > -fault: > - gnttab_flush_tlb(current->domain); > + fault: > + if ( current->domain->is_shut_down ) > + gnttab_flush_tlb(current->domain); First of all: Is avoiding this really all that useful? You don't avoid further work in any of the earlier patches. I.e. not the least for consistency reasons I'd suggest dropping this patch. And then, besides the condition being inverted as Kevin has already pointed out, I think you mean ->is_shutting_down, not ->is_shut_down. Jan
On April 25, 2016 5:56 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote: > > --- a/xen/common/grant_table.c > > +++ b/xen/common/grant_table.c > > @@ -1366,8 +1366,9 @@ gnttab_unmap_grant_ref( > > > > return 0; > > > > -fault: > > - gnttab_flush_tlb(current->domain); > > + fault: > > + if ( current->domain->is_shut_down ) > > + gnttab_flush_tlb(current->domain); > > First of all: Is avoiding this really all that useful? You don't avoid further work > in any of the earlier patches. I.e. not the least for consistency reasons I'd > suggest dropping this patch. > > And then, besides the condition being inverted as Kevin has already pointed > out, I think you mean ->is_shutting_down, not > ->is_shut_down. > I will drop this patch in next v3. Quan
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index 8b22299..1801fe9 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -1366,8 +1366,9 @@ gnttab_unmap_grant_ref( return 0; -fault: - gnttab_flush_tlb(current->domain); + fault: + if ( current->domain->is_shut_down ) + gnttab_flush_tlb(current->domain); for ( i = 0; i < partial_done; i++ ) __gnttab_unmap_common_complete(&(common[i])); @@ -1429,8 +1430,9 @@ gnttab_unmap_and_replace( return 0; -fault: - gnttab_flush_tlb(current->domain); + fault: + if ( current->domain->is_shut_down ) + gnttab_flush_tlb(current->domain); for ( i = 0; i < partial_done; i++ ) __gnttab_unmap_common_complete(&(common[i]));
While grant table is unmapping, the domain (with the exception of the hardware domain) may be crashed due to IOMMU mapping and unmapping failures, and then it is unnecessary to flush specified CPUs' TLBs. Signed-off-by: Quan Xu <quan.xu@intel.com> CC: Ian Jackson <ian.jackson@eu.citrix.com> CC: Jan Beulich <jbeulich@suse.com> CC: Keir Fraser <keir@xen.org> CC: Tim Deegan <tim@xen.org> --- xen/common/grant_table.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)