Re: [Xen-devel] [PATCH v4 1/3] VT-d: Check VT-d Device-TLB flush error.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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