Re: [Xen-devel] [PATCH v4 2/4] p2m: change write_p2m_entry to return an error code

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.02.19 at 18:27, <roger.pau@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -184,6 +184,7 @@ p2m_next_level(struct p2m_domain *p2m, void **table,
>      l1_pgentry_t *p2m_entry, new_entry;
>      void *next;
>      unsigned int flags;
> +    int rc;
>  
>      if ( !(p2m_entry = p2m_find_entry(*table, gfn_remainder, gfn,
>                                        shift, max)) )
> @@ -202,7 +203,13 @@ p2m_next_level(struct p2m_domain *p2m, void **table,
>          new_entry = l1e_from_mfn(mfn, P2M_BASE_FLAGS | _PAGE_RW);
>  
>          p2m_add_iommu_flags(&new_entry, level, 
> IOMMUF_readable|IOMMUF_writable);
> -        p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level + 1);
> +        rc = p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level + 1);
> +        if ( rc )
> +        {
> +            ASSERT_UNREACHABLE();
> +            p2m_free_ptp(p2m, mfn_to_page(mfn));
> +            return rc;
> +        }
>      }
>      else if ( flags & _PAGE_PSE )
>      {
> @@ -250,14 +257,27 @@ p2m_next_level(struct p2m_domain *p2m, void **table,
>          {
>              new_entry = l1e_from_pfn(pfn | (i << ((level - 1) * 
> PAGETABLE_ORDER)),
>                                       flags);
> -            p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, level);
> +            rc = p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, 
> level);
> +            if ( rc )
> +            {
> +                ASSERT_UNREACHABLE();
> +                unmap_domain_page(l1_entry);
> +                p2m_free_ptp(p2m, mfn_to_page(mfn));
> +                return rc;
> +            }
>          }
>  
>          unmap_domain_page(l1_entry);
>  
>          new_entry = l1e_from_mfn(mfn, P2M_BASE_FLAGS | _PAGE_RW);
>          p2m_add_iommu_flags(&new_entry, level, 
> IOMMUF_readable|IOMMUF_writable);
> -        p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level + 1);
> +        rc = p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level + 1);
> +        if ( rc )
> +        {
> +            ASSERT_UNREACHABLE();
> +            p2m_free_ptp(p2m, mfn_to_page(mfn));
> +            return rc;
> +        }
>      }
>      else
>          ASSERT(flags & _PAGE_PRESENT);

Personally I would find it quite desirable if there was just one instance of
this error handling you add, which doesn't look overly difficult to arrange
for.

> @@ -321,7 +341,8 @@ static int p2m_pt_set_recalc_range(struct p2m_domain *p2m,
>              if ( (l1e_get_flags(e) & _PAGE_PRESENT) && !needs_recalc(l1, e) )
>              {
>                  set_recalc(l1, e);
> -                p2m->write_p2m_entry(p2m, first_gfn, pent, e, level);
> +                err = p2m->write_p2m_entry(p2m, first_gfn, pent, e, level);
> +                ASSERT(!err);
>              }
>              first_gfn += 1UL << (i * PAGETABLE_ORDER);
>          }

So on a release build what result the caller would observe in case
there is an (unexpected) error depends on whether this occurs on
the last iteration. I don't consider this very helpful behavior in
terms of debuggability. (Along these lines also again further down).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel