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.