[PATCH] xen: arm: correct return value of raw_copy_to_guest_*

[PATCH] xen: arm: correct return value of raw_copy_to_guest_*

[Ian Campbell]

This is a generic interface which is supposed to return the number of bytes
which were not copied. Make it so and update the one incorrect caller.

Make the flush_dcache parameter to the helper an int while at it.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/domain_build.c |    8 ++++----
 xen/arch/arm/guestcopy.c    |    8 +++-----
 2 files changed, 7 insertions(+), 9 deletions(-)

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..f1b2dda 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;
-- 
1.7.10.4

Re: [PATCH] xen: arm: correct return value of raw_copy_to_guest_*

[Julien Grall]

On 12/06/2013 03:25 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 and update the one incorrect caller.

This is also the same for raw_clear_guest and raw_copy_from_guest.

It seems the most of ARM code assume that these functions (or macro that 
call them) will return a negative value if it's fails.

-- 
Julien Grall

Re: [PATCH] xen: arm: correct return value of raw_copy_to_guest_*

[Ian Campbell]

On Fri, 2013-12-06 at 16:00 +0000, Julien Grall wrote:
> 
> On 12/06/2013 03:25 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 and update the one incorrect caller.
> 
> This is also the same for raw_clear_guest and raw_copy_from_guest.

Oops, yes.

> It seems the most of ARM code assume that these functions (or macro that 
> call them) will return a negative value if it's fails.

Hrm, the ones I looked at all seemed to match the return the number of
bytes not copied pattern (or more often just cared about 0 on success
and !0 otherwise). Did you find some which aren't that way?

In any case these functions are used from common code so we probably
ought to match the interface.

Ian.

Re: [PATCH] xen: arm: correct return value of raw_copy_to_guest_*

[Julien Grall]

On 12/06/2013 05:03 PM, Ian Campbell wrote:
> On Fri, 2013-12-06 at 16:00 +0000, Julien Grall wrote:
>>
>> On 12/06/2013 03:25 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 and update the one incorrect caller.
>>
>> This is also the same for raw_clear_guest and raw_copy_from_guest.
>
> Oops, yes.
>
>> It seems the most of ARM code assume that these functions (or macro that
>> call them) will return a negative value if it's fails.
>
> Hrm, the ones I looked at all seemed to match the return the number of
> bytes not copied pattern (or more often just cared about 0 on success
> and !0 otherwise). Did you find some which aren't that way?
>

copy_from_guest_offset is a macro which use raw_copy_from_guest.
In xenmem_add_to_physmap_range (arch/arm/mm.c), we only check if the 
return is negative.

-- 
Julien Grall

Re: [PATCH] xen: arm: correct return value of raw_copy_to_guest_*

[Ian Campbell]

On Fri, 2013-12-06 at 17:42 +0000, Julien Grall wrote:
> 
> On 12/06/2013 05:03 PM, Ian Campbell wrote:
> > On Fri, 2013-12-06 at 16:00 +0000, Julien Grall wrote:
> >>
> >> On 12/06/2013 03:25 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 and update the one incorrect caller.
> >>
> >> This is also the same for raw_clear_guest and raw_copy_from_guest.
> >
> > Oops, yes.
> >
> >> It seems the most of ARM code assume that these functions (or macro that
> >> call them) will return a negative value if it's fails.
> >
> > Hrm, the ones I looked at all seemed to match the return the number of
> > bytes not copied pattern (or more often just cared about 0 on success
> > and !0 otherwise). Did you find some which aren't that way?
> >
> 
> copy_from_guest_offset is a macro which use raw_copy_from_guest.
> In xenmem_add_to_physmap_range (arch/arm/mm.c), we only check if the 
> return is negative.

Thanks I'll add the fix for this to v2.

Ian.