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.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>

> ---
> v2: Fix xenmem_add_to_physmap_range, which was assuming -errno return codes.
>     Fix raw_copy_from_guest and raw_clear_guest too
> ---
>  xen/arch/arm/decode.c       |   22 ++++++++++++++--------
>  xen/arch/arm/domain_build.c |    8 ++++----
>  xen/arch/arm/guestcopy.c    |   20 +++++++-------------
>  xen/arch/arm/mm.c           |   23 +++++++++++++++++------
>  4 files changed, 42 insertions(+), 31 deletions(-)
> 
> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
> index abe9f26..8880c39 100644
> --- a/xen/arch/arm/decode.c
> +++ b/xen/arch/arm/decode.c
> @@ -36,12 +36,10 @@ static void update_dabt(struct hsr_dabt *dabt, int reg,
>  static int decode_thumb2(register_t pc, struct hsr_dabt *dabt, uint16_t hw1)
>  {
>      uint16_t hw2;
> -    int rc;
>      uint16_t rt;
>  
> -    rc = raw_copy_from_guest(&hw2, (void *__user)(pc + 2), sizeof (hw2));
> -    if ( rc )
> -        return rc;
> +    if ( raw_copy_from_guest(&hw2, (void *__user)(pc + 2), sizeof (hw2)) )
> +        return -EFAULT;
>  
>      rt = (hw2 >> 12) & 0x7;
>  
> @@ -88,11 +86,9 @@ bad_thumb2:
>  static int decode_thumb(register_t pc, struct hsr_dabt *dabt)
>  {
>      uint16_t instr;
> -    int rc;
>  
> -    rc = raw_copy_from_guest(&instr, (void * __user)pc, sizeof (instr));
> -    if ( rc )
> -        return rc;
> +    if ( raw_copy_from_guest(&instr, (void * __user)pc, sizeof (instr)) )
> +        return -EFAULT;
>  
>      switch ( instr >> 12 )
>      {
> @@ -163,3 +159,13 @@ int decode_instruction(const struct cpu_user_regs *regs, struct hsr_dabt *dabt)
>  
>      return 1;
>  }
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 0269294..73a7cff 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -907,15 +907,15 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>  static void dtb_load(struct kernel_info *kinfo)
>  {
>      void * __user dtb_virt = (void * __user)(register_t)kinfo->dtb_paddr;
> -    unsigned long rc;
> +    unsigned long left;
>  
>      printk("Loading dom0 DTB to 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
>             kinfo->dtb_paddr, kinfo->dtb_paddr + fdt_totalsize(kinfo->fdt));
>  
> -    rc = raw_copy_to_guest_flush_dcache(dtb_virt, kinfo->fdt,
> +    left = raw_copy_to_guest_flush_dcache(dtb_virt, kinfo->fdt,
>                                          fdt_totalsize(kinfo->fdt));
> -    if ( rc != 0 )
> -        panic("Unable to copy the DTB to dom0 memory (rc = %lu)", rc);
> +    if ( left != 0 )
> +        panic("Unable to copy the DTB to dom0 memory (left = %lu bytes)", left);
>      xfree(kinfo->fdt);
>  }
>  
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index 08800a4..bd0a355 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -6,21 +6,19 @@
>  #include <asm/guest_access.h>
>  
>  static unsigned long raw_copy_to_guest_helper(void *to, const void *from,
> -                                              unsigned len, unsigned flush_dcache)
> +                                              unsigned len, int flush_dcache)
>  {
>      /* XXX needs to handle faults */
>      unsigned offset = (vaddr_t)to & ~PAGE_MASK;
>  
>      while ( len )
>      {
> -        int rc;
>          paddr_t g;
>          void *p;
>          unsigned size = min(len, (unsigned)PAGE_SIZE - offset);
>  
> -        rc = gvirt_to_maddr((vaddr_t) to, &g);
> -        if ( rc )
> -            return rc;
> +        if ( gvirt_to_maddr((vaddr_t) to, &g) )
> +            return len;
>  
>          p = map_domain_page(g>>PAGE_SHIFT);
>          p += offset;
> @@ -56,14 +54,12 @@ unsigned long raw_clear_guest(void *to, unsigned len)
>  
>      while ( len )
>      {
> -        int rc;
>          paddr_t g;
>          void *p;
>          unsigned size = min(len, (unsigned)PAGE_SIZE - offset);
>  
> -        rc = gvirt_to_maddr((vaddr_t) to, &g);
> -        if ( rc )
> -            return rc;
> +        if ( gvirt_to_maddr((vaddr_t) to, &g) )
> +            return len;
>  
>          p = map_domain_page(g>>PAGE_SHIFT);
>          p += offset;
> @@ -84,14 +80,12 @@ unsigned long raw_copy_from_guest(void *to, const void __user *from, unsigned le
>  
>      while ( len )
>      {
> -        int rc;
>          paddr_t g;
>          void *p;
>          unsigned size = min(len, (unsigned)(PAGE_SIZE - offset));
>  
> -        rc = gvirt_to_maddr((vaddr_t) from & PAGE_MASK, &g);
> -        if ( rc )
> -            return rc;
> +        if ( gvirt_to_maddr((vaddr_t) from & PAGE_MASK, &g) )
> +            return len;
>  
>          p = map_domain_page(g>>PAGE_SHIFT);
>          p += ((vaddr_t)from & (~PAGE_MASK));
> 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;
>  
> +        if ( unlikely(copy_to_guest_offset(xatpr->errs,
> +                                           xatpr->size-1, &rc, 1)) );
> +        {
> +            rc = -EFAULT;
> +            goto out;
> +        }
> +
>          xatpr->size--;
>  
>          /* Check for continuation if it's not the last interation */
> 


-- 
Julien Grall