Re: [Xen-devel] [PATCH] xen: arm: process XENMEM_add_to_physmap_range forwards not backwards.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-12-18 at 09:58 +0000, Jan Beulich wrote:
> >>> On 18.12.13 at 10:41, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:
> > Jan points out that processing the list backwards is rather counter 
> > intuitive
> > and that the effect of the hypercall can differ between forwards and 
> > backwards
> > processing (e.g. in the presence of duplicate idx or gpfn, which would be
> > unusualy but as Jan says, users are a creative bunch)
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > Cc: Jan Beulich <JBeulich@xxxxxxxx>
> > Cc: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> > ---
> > Release: subtle ABI change, should go in to 4.4 before people rely on it 
> > (they
> > are not relying on it today TTBOMK and it seems unlikely but lets not risk 
> > it)
> > ---
> >  xen/arch/arm/mm.c |   12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > index e235364..67af28f 100644
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -1062,21 +1062,18 @@ static int xenmem_add_to_physmap_range(struct 
> > domain 
> > *d,
> >  {
> >      int rc;
> >  
> > -    /* Process entries in reverse order to allow continuations */
> >      while ( xatpr->size > 0 )
> >      {
> >          xen_ulong_t idx;
> >          xen_pfn_t gpfn;
> >  
> > -        if ( unlikely(copy_from_guest_offset(&idx, xatpr->idxs,
> > -                                             xatpr->size-1, 1)) )
> > +        if ( unlikely(copy_from_guest_offset(&idx, xatpr->idxs, 0, 1)) )
> >          {
> >              rc = -EFAULT;
> >              goto out;
> >          }
> >  
> > -        if ( unlikely(copy_from_guest_offset(&gpfn, xatpr->gpfns,
> > -                                             xatpr->size-1, 1)) )
> > +        if ( unlikely(copy_from_guest_offset(&gpfn, xatpr->gpfns, 0, 1)) )
> >          {
> >              rc = -EFAULT;
> >              goto out;
> > @@ -1086,8 +1083,7 @@ static int xenmem_add_to_physmap_range(struct domain 
> > *d,
> >                                         xatpr->foreign_domid,
> >                                         idx, gpfn);
> >  
> > -        if ( unlikely(copy_to_guest_offset(xatpr->errs,
> > -                                           xatpr->size-1, &rc, 1)) )
> > +        if ( unlikely(copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) )
> >          {
> >              rc = -EFAULT;
> >              goto out;
> > @@ -1096,6 +1092,8 @@ static int xenmem_add_to_physmap_range(struct domain 
> > *d,
> >          if ( rc < 0 )
> >              goto out;
> >  
> > +        guest_handle_add_offset(xatpr->idxs, 1);
> > +        guest_handle_add_offset(xatpr->gpfns, 1);
> 
> What about ->errs?

Oops! Well spotted. I'll hold of on v2 until:

> And anyway - as I said in an earlier response to the original thread,
> _if_ we want to stay with modifying the interface structure here,
> we need to document that in the public header. Personally I'd favor
> this inconsistent behavior to get fixed.

I've only just seen that comment, I replied there...

By "fixed" you mean the API doc updated or using the upper bits?

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel