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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.02.19 at 12:56, <roger.pau@xxxxxxxxxx> wrote:
> 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?

Personally (again) I'd prefer if you got away without, which (as said before)
doesn't look overly difficult to arrange for.

Jan



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