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

[Julien Grall <julien.grall@linaro.org>]

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@citrix.com>
> ---
> 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.

Here a patch to fix this commit

======================================================================================

commit ce9b426051d742e116d43cbd0e15dc47f5fab88b
Author: Julien Grall <julien.grall@linaro.org>
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.
    
    Signed-off-by: Julien Grall <julien.grall@linaro.org>

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 6e6d7d4..4598866 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1108,16 +1108,17 @@ static int xenmem_add_to_physmap_range(struct domain *d,
         rc = xenmem_add_to_physmap_one(d, xatpr->space,
                                        xatpr->foreign_domid,
                                        idx, gpfn);
-        if ( rc < 0 )
-            goto out;
 
         if ( unlikely(copy_to_guest_offset(xatpr->errs,
-                                           xatpr->size-1, &rc, 1)) );
+                                           xatpr->size-1, &rc, 1)) )
         {
             rc = -EFAULT;
             goto out;
         }
 
+        if ( rc < 0 )
+            goto out;
+
         xatpr->size--;
 
         /* Check for continuation if it's not the last interation */

-- 
Julien Grall