Ian Campbell
2013-Dec-09 12:13 UTC
[PATCH v2] xen: arm: correct return value of raw_copy_{to/from}_guest_*, raw_clear_guest
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> --- 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 */ -- 1.7.10.4
Julien Grall
2013-Dec-09 15:14 UTC
Re: [PATCH v2] xen: arm: correct return value of raw_copy_{to/from}_guest_*, raw_clear_guest
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
Ian Campbell
2013-Dec-09 15:48 UTC
Re: [PATCH v2] xen: arm: correct return value of raw_copy_{to/from}_guest_*, raw_clear_guest
On Mon, 2013-12-09 at 15:14 +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. > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > Acked-by: Julien Grall <julien.grall@linaro.org>Applied, thanks.
Julien Grall
2013-Dec-09 18:34 UTC
Re: [PATCH v2] xen: arm: correct return value of raw_copy_{to/from}_guest_*, raw_clear_guest
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
Ian Campbell
2013-Dec-10 09:29 UTC
Re: [PATCH v2] xen: arm: correct return value of raw_copy_{to/from}_guest_*, raw_clear_guest
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@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.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@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.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@linaro.org>Acked + applied, thanks for catching this. Ian.