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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Feb 19, 2019 at 02:05:37AM -0700, Jan Beulich wrote:
> >>> 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.

Sure. Would you be fine with me adding a label?

> > @@ -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).

Done, thanks for spotting those.

Roger.

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