[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 1/3] VT-d: Check VT-d Device-TLB flush error.
>>> On 23.12.15 at 09:25, <quan.xu@xxxxxxxxx> wrote: > @@ -182,7 +186,7 @@ static int enter_state(u32 state) > error = tboot_s3_resume(); > break; > case ACPI_STATE_S5: > - acpi_enter_sleep_state(ACPI_STATE_S5); > + error = acpi_enter_sleep_state(ACPI_STATE_S5); I can't see how this is related to the purpose of the patch. I don't mind such error checking being added, but not in this huge patch. It would anyway be nice if you could see about splitting this apart, to aid reviewing and - in case it would be needed after committing - bisection. > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -2443,11 +2443,18 @@ static int __get_page_type(struct page_info *page, > unsigned long type, > if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) ) > { > if ( (x & PGT_type_mask) == PGT_writable_page ) > - iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page))); > + { > + rc = iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page))); > + return rc; > + } > else if ( type == PGT_writable_page ) > - iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)), > - page_to_mfn(page), > - IOMMUF_readable|IOMMUF_writable); > + { > + rc = iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)), > + page_to_mfn(page), > + IOMMUF_readable|IOMMUF_writable); > + if ( rc ) > + return rc; > + } > } > } Again you can't simply return here, or else you leak the type reference, and you indefinitely stall any other CPU waiting for page validation to happen. > --- a/xen/arch/x86/mm/p2m-ept.c > +++ b/xen/arch/x86/mm/p2m-ept.c > @@ -829,15 +829,23 @@ out: > need_modify_vtd_table ) > { > if ( iommu_hap_pt_share ) > - iommu_pte_flush(d, gfn, &ept_entry->epte, order, > vtd_pte_present); > + rc = iommu_pte_flush(d, gfn, &ept_entry->epte, order, > vtd_pte_present); > else > { > if ( iommu_flags ) > for ( i = 0; i < (1 << order); i++ ) > - iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags); > + { > + rc = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, > iommu_flags); > + if ( rc ) > + break; > + } And the pattern repeats - you can't just exit without undoing what so far was done. > else > for ( i = 0; i < (1 << order); i++ ) > - iommu_unmap_page(d, gfn + i); > + { > + rc = iommu_unmap_page(d, gfn + i); > + if ( rc ) > + break; > + } As a special case, unmapping should perhaps continue despite an error, in an attempt to do best effort cleanup. I'm not going to continue further down, as I suspect I'll find more of the same class of issues. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |