Re: [Xen-devel] [PATCH v5] x86/mm: Clean up p2m_finish_type_change return value

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf Of 
> Alexandru Stefan ISAILA
> Sent: 10 April 2019 09:19
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx; George Dunlap <George.Dunlap@xxxxxxxxxx>; 
> Paul Durrant
> <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; 
> jbeulich@xxxxxxxx; Roger
> Pau Monne <roger.pau@xxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v5] x86/mm: Clean up p2m_finish_type_change 
> return value
> 
> Roger/Paul are you ok with the latest changes? Can this go in?
> 

Yes, I am happy for my R-b to be added if so desired.

  Paul

> Regards,
> Alex
> 
> On 29.03.2019 14:50, Alexandru Stefan ISAILA wrote:
> > In the case of any errors, finish_type_change() passes values returned
> > from p2m->recalc() up the stack (with some exceptions in the case where
> > an error is expected); this eventually ends up being returned to the
> > XEN_DOMOP_map_mem_type_to_ioreq_server hypercall.
> >
> > However, on Intel processors (but not on AMD processor), p2m->recalc()
> > can also return '1' as well as '0'.  This case is handled very
> > inconsistently: finish_type_change() will return the value of the final
> > entry it attempts, discarding results for other entries;
> > p2m_finish_type_change() will attempt to accumulate '1's, so that it
> > returns '1' if any of the calls to finish_type_change() returns '1'; and
> > dm_op() will again return '1' only if the very last call to
> > p2m_finish_type_change() returns '1'.  The result is that the
> > XEN_DMOP_map_mem_type_to_ioreq_server() hypercall will sometimes return
> > 0 and sometimes return 1 on success, in an unpredictable manner.
> >
> > The hypercall documentation doesn't mention return values; but it's not
> > clear what the caller could do with the information about whether
> > entries had been changed or not.  At the moment it's always 0 on AMD
> > boxes, and *usually* 1 on Intel boxes; so nothing can be relying on a
> > '1' return value for correctness (or if it is, it's broken).
> >
> > Make the return value on success consistently '0' by only returning
> > 0/-ERROR from finish_type_change().  Also remove the accumulation code
> > from p2m_finish_type_change().
> >
> > Suggested-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> > Signed-off-by: Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>
> >
> > ---
> > Changes since V4:
> >     - Undo changes in p2m->recalc() and have 1 turn into 0 in
> >     finish_type_change().
> > ---
> >   xen/arch/x86/mm/p2m.c | 16 +++++-----------
> >   1 file changed, 5 insertions(+), 11 deletions(-)
> >
> > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> > index b9bbb8f485..5508cedc55 100644
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -1158,7 +1158,7 @@ void p2m_change_type_range(struct domain *d,
> >    * Finish p2m type change for gfns which are marked as need_recalc in a 
> > range.
> >    * Uses the current p2m's max_mapped_pfn to further clip the invalidation
> >    * range for alternate p2ms.
> > - * Returns: 0/1 for success, negative for failure
> > + * Returns: 0 for success, negative for failure
> >    */
> >   static int finish_type_change(struct p2m_domain *p2m,
> >                                 gfn_t first_gfn, unsigned long max_nr)
> > @@ -1174,9 +1174,9 @@ static int finish_type_change(struct p2m_domain *p2m,
> >           /*
> >            * ept->recalc could return 0/1/-ENOMEM. pt->recalc could return
> >            * 0/-ENOMEM/-ENOENT, -ENOENT isn't an error as we are looping
> > -         * gfn here.
> > +         * gfn here. If rc is 1 we need to have it 0 for success.
> >            */
> > -        if ( rc == -ENOENT )
> > +        if ( rc == -ENOENT || rc == 1 )
> >               rc = 0;
> >           else if ( rc < 0 )
> >           {
> > @@ -1213,19 +1213,13 @@ int p2m_finish_type_change(struct domain *d,
> >               if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
> >               {
> >                   struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
> > -                int rc1;
> >
> >                   p2m_lock(altp2m);
> > -                rc1 = finish_type_change(altp2m, first_gfn, max_nr);
> > +                rc = finish_type_change(altp2m, first_gfn, max_nr);
> >                   p2m_unlock(altp2m);
> >
> > -                if ( rc1 < 0 )
> > -                {
> > -                    rc = rc1;
> > +                if ( rc < 0 )
> >                       goto out;
> > -                }
> > -
> > -                rc |= rc1;
> >               }
> >       }
> >   #endif
> >
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel