Re: [Xen-devel] [PATCH v2] xen: arm: correct return value of raw_copy_{to/from}_guest_*, raw_clear_guest

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-12-09 at 18:34 +0000, Julien Grall wrote:
> On 12/09/2013 12:13 PM, Ian Campbell wrote:
> > This is a generic interface which is supposed to return the number of bytes
> > which were not copied. Make it so.
> > 
> > Update the incorrect callers prepare_dtb, decode_thumb{2} and
> > xenmem_add_to_physmap_range.
> > 
> > In the xenmem_add_to_physmap_range case, observe that we are not propagating
> > errors from xenmem_add_to_physmap_one and do so.
> > 
> > In the decode_thumb case and an emacs magic block to decode.c
> > 
> > Make the flush_dcache parameter to the helper an int while at it.
> 
> Actually this patch is buggy, I can't anymore create a guest on midway. I get 
> lots of: 
> Failed to map pfn to mfn rc:-14:0 pfn:3efd8 mfn:802a3
> Failed to map pfn to mfn rc:-14:0 pfn:3e019 mfn:802a4
> Failed to map pfn to mfn rc:-14:0 pfn:3ed92 mfn:802a5
> ...
> 
> > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > ---
> > v2: Fix xenmem_add_to_physmap_range, which was assuming -errno return codes.
> >     Fix raw_copy_from_guest and raw_clear_guest too
> > ---
> 
> [..]
> 
> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > index 399e546..26ca588 100644
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -1067,22 +1067,33 @@ static int xenmem_add_to_physmap_range(struct 
> > domain *d,
> >          xen_ulong_t idx;
> >          xen_pfn_t gpfn;
> >  
> > -        rc = copy_from_guest_offset(&idx, xatpr->idxs, xatpr->size-1, 1);
> > -        if ( rc < 0 )
> > +        if ( unlikely(copy_from_guest_offset(&idx, xatpr->idxs,
> > +                                             xatpr->size-1, 1)) )
> > +        {
> > +            rc = -EFAULT;
> >              goto out;
> > +        }
> >  
> > -        rc = copy_from_guest_offset(&gpfn, xatpr->gpfns, xatpr->size-1, 1);
> > -        if ( rc < 0 )
> > +        if ( unlikely(copy_from_guest_offset(&gpfn, xatpr->gpfns,
> > +                                             xatpr->size-1, 1)) )
> > +        {
> > +            rc = -EFAULT;
> >              goto out;
> > +        }
> >  
> >          rc = xenmem_add_to_physmap_one(d, xatpr->space,
> >                                         xatpr->foreign_domid,
> >                                         idx, gpfn);
> > -
> > -        rc = copy_to_guest_offset(xatpr->errs, xatpr->size-1, &rc, 1);
> >          if ( rc < 0 )
> >              goto out;
> 
> copy_to_guest_offset was fine before the "if ( rc < 0 )" because we want to 
> copy the error in the xatpr->errs.
> 
> > +        if ( unlikely(copy_to_guest_offset(xatpr->errs,
> > +                                           xatpr->size-1, &rc, 1)) );
> 
> You forgot to remote the ';' at the end.

Damn, that's what I get for making a last minute change to the patch!

> 
> Here a patch to fix this commit
> 
> ======================================================================================
> 
> commit ce9b426051d742e116d43cbd0e15dc47f5fab88b
> Author: Julien Grall <julien.grall@xxxxxxxxxx>
> Date:   Mon Dec 9 18:29:50 2013 +0000
> 
>     xen/arm: Fix regression after commit d963923
>     
>     The commit d963923  "xen: arm: correct return value of
>     raw_copy_{to/from}_guest_*, raw_clear_guest" doesn't permit to boot guest
>     on Xen ARM.
>     
>     Also we want to get the right rc in the error arrays, so move back
>     copy_to_guest function to the previous place.

Ah, I missed that the error check was in the copy_to_guest_offset and
thought it wasn't being tested at all, which is why I changed it.

I tried to clarify what was going on by expanding the commit log to:

    Remove the stray semicolon from the end of the if statement.
    
    Also we want to get the right rc in the error arrays, so we need to do the
    copy_to_guest_offset before checking the rc returned by
    xenmem_add_to_physmap_one.
    
>     Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>

Acked + applied, thanks for catching this.

Ian.


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