Xen devel - Oct 2012 - [PATCH] ACPI/APEI: fix ERST MOVE_DATA instruction implementation

The src_base and dst_base fields in apei_exec_context are physical
address, so they should be ioremaped before being used in ERST
MOVE_DATA instruction.

Reported-by: Javier Martinez Canillas <martinez.javier@gmail.com>
Reported-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Huang Ying <ying.huang@intel.com>

Replace use of ioremap() by __acpi_map_table()/set_fixmap(). Fix error
handling.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Note that I have no way to test this code other than by having it go
through the tester, since I have no hardware available to reproduce
the regression found there (in other words, this is just a blind
attempt at addressing the problem uncovered by 26060:4fc87c2f31a0).

--- a/xen/drivers/acpi/apei/erst.c
+++ b/xen/drivers/acpi/apei/erst.c
@@ -247,15 +247,64 @@ static int erst_exec_move_data(struct ap
 {
 	int rc;
 	u64 offset;
+#ifdef CONFIG_X86
+	enum fixed_addresses idx;
+#endif
+	void *src, *dst;
+
+	/* ioremap does not work in interrupt context */
+	if (in_irq()) {
+		printk(KERN_WARNING
+		       "MOVE_DATA cannot be used in interrupt context\n");
+		return -EBUSY;
+	}
 
 	rc = __apei_exec_read_register(entry, &offset);
 	if (rc)
 		return rc;
-	memmove((void *)(unsigned long)(ctx->dst_base + offset),
-		(void *)(unsigned long)(ctx->src_base + offset),
-		ctx->var2);
 
-	return 0;
+#ifdef CONFIG_X86
+	switch (ctx->var2) {
+	case 0:
+		return 0;
+	case 1 ... PAGE_SIZE:
+		break;
+	default:
+		printk(KERN_WARNING
+		       "MOVE_DATA cannot be used for %#"PRIx64" bytes of
data\n",
+		       ctx->var2);
+		return -EOPNOTSUPP;
+	}
+
+	src = __acpi_map_table(ctx->src_base + offset, ctx->var2);
+#else
+	src = ioremap(ctx->src_base + offset, ctx->var2);
+#endif
+	if (!src)
+		return -ENOMEM;
+
+#ifdef CONFIG_X86
+	BUILD_BUG_ON(FIX_ACPI_PAGES < 4);
+	idx = virt_to_fix((unsigned long)src + 2 * PAGE_SIZE);
+	offset += ctx->dst_base;
+	dst = (void *)fix_to_virt(idx) + (offset & ~PAGE_MASK);
+	set_fixmap(idx, offset);
+	if (PFN_DOWN(offset) != PFN_DOWN(offset + ctx->var2 - 1)) {
+		idx = virt_to_fix((unsigned long)dst + PAGE_SIZE);
+		set_fixmap(idx, offset + PAGE_SIZE);
+	}
+#else
+	dst = ioremap(ctx->dst_base + offset, ctx->var2);
+#endif
+	if (dst) {
+		memmove(dst, src, ctx->var2);
+		iounmap(dst);
+	} else
+		rc = -ENOMEM;
+
+	iounmap(src);
+
+	return rc;
 }
 
 static struct apei_exec_ins_type erst_ins_type[] = {





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On 17/10/2012 12:36, "Jan Beulich" <JBeulich@suse.com> wrote:
> The src_base and dst_base fields in apei_exec_context are physical
> address, so they should be ioremaped before being used in ERST
> MOVE_DATA instruction.
> 
> Reported-by: Javier Martinez Canillas <martinez.javier@gmail.com>
> Reported-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Huang Ying <ying.huang@intel.com>
> 
> Replace use of ioremap() by __acpi_map_table()/set_fixmap(). Fix error
> handling.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Keir Fraser <keir@xen.org>
> ---
> Note that I have no way to test this code other than by having it go
> through the tester, since I have no hardware available to reproduce
> the regression found there (in other words, this is just a blind
> attempt at addressing the problem uncovered by 26060:4fc87c2f31a0).
> 
> --- a/xen/drivers/acpi/apei/erst.c
> +++ b/xen/drivers/acpi/apei/erst.c
> @@ -247,15 +247,64 @@ static int erst_exec_move_data(struct ap
>  {
> int rc;
> u64 offset;
> +#ifdef CONFIG_X86
> + enum fixed_addresses idx;
> +#endif
> + void *src, *dst;
> +
> + /* ioremap does not work in interrupt context */
> + if (in_irq()) {
> +  printk(KERN_WARNING
> +         "MOVE_DATA cannot be used in interrupt context\n");
> +  return -EBUSY;
> + }
>  
> rc = __apei_exec_read_register(entry, &offset);
> if (rc)
> return rc;
> - memmove((void *)(unsigned long)(ctx->dst_base + offset),
> -  (void *)(unsigned long)(ctx->src_base + offset),
> -  ctx->var2);
>  
> - return 0;
> +#ifdef CONFIG_X86
> + switch (ctx->var2) {
> + case 0:
> +  return 0;
> + case 1 ... PAGE_SIZE:
> +  break;
> + default:
> +  printk(KERN_WARNING
> +         "MOVE_DATA cannot be used for %#"PRIx64" bytes of
data\n",
> +         ctx->var2);
> +  return -EOPNOTSUPP;
> + }
> +
> + src = __acpi_map_table(ctx->src_base + offset, ctx->var2);
> +#else
> + src = ioremap(ctx->src_base + offset, ctx->var2);
> +#endif
> + if (!src)
> +  return -ENOMEM;
> +
> +#ifdef CONFIG_X86
> + BUILD_BUG_ON(FIX_ACPI_PAGES < 4);
> + idx = virt_to_fix((unsigned long)src + 2 * PAGE_SIZE);
> + offset += ctx->dst_base;
> + dst = (void *)fix_to_virt(idx) + (offset & ~PAGE_MASK);
> + set_fixmap(idx, offset);
> + if (PFN_DOWN(offset) != PFN_DOWN(offset + ctx->var2 - 1)) {
> +  idx = virt_to_fix((unsigned long)dst + PAGE_SIZE);
> +  set_fixmap(idx, offset + PAGE_SIZE);
> + }
> +#else
> + dst = ioremap(ctx->dst_base + offset, ctx->var2);
> +#endif
> + if (dst) {
> +  memmove(dst, src, ctx->var2);
> +  iounmap(dst);
> + } else
> +  rc = -ENOMEM;
> +
> + iounmap(src);
> +
> + return rc;
>  }
>  
>  static struct apei_exec_ins_type erst_ins_type[] = {
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

>>> On 17.10.12 at 13:36, "Jan Beulich"
<JBeulich@suse.com> wrote:
> --- a/xen/drivers/acpi/apei/erst.c
> +++ b/xen/drivers/acpi/apei/erst.c
> @@ -247,15 +247,64 @@ static int erst_exec_move_data(struct ap
>  {
>  	int rc;
>  	u64 offset;
> +#ifdef CONFIG_X86
> +	enum fixed_addresses idx;
> +#endif
> +	void *src, *dst;
> +
> +	/* ioremap does not work in interrupt context */
> +	if (in_irq()) {
> +		printk(KERN_WARNING
> +		       "MOVE_DATA cannot be used in interrupt context\n");
> +		return -EBUSY;
> +	}
>  
>  	rc = __apei_exec_read_register(entry, &offset);
>  	if (rc)
>  		return rc;
> -	memmove((void *)(unsigned long)(ctx->dst_base + offset),
> -		(void *)(unsigned long)(ctx->src_base + offset),
> -		ctx->var2);
>  
> -	return 0;
> +#ifdef CONFIG_X86
> +	switch (ctx->var2) {
> +	case 0:
> +		return 0;
> +	case 1 ... PAGE_SIZE:
> +		break;
> +	default:
> +		printk(KERN_WARNING
> +		       "MOVE_DATA cannot be used for %#"PRIx64" bytes of
data\n",
> +		       ctx->var2);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	src = __acpi_map_table(ctx->src_base + offset, ctx->var2);
> +#else
> +	src = ioremap(ctx->src_base + offset, ctx->var2);
> +#endif
Before anyone asks - yes, I have added to my todo list to put in
place a proper ioremap() for x86. But since I''m under some time
pressure right now, this was the faster route to something that
can be committed to see whether the regression goes away.

Jan
> +	if (!src)
> +		return -ENOMEM;
> +
> +#ifdef CONFIG_X86
> +	BUILD_BUG_ON(FIX_ACPI_PAGES < 4);
> +	idx = virt_to_fix((unsigned long)src + 2 * PAGE_SIZE);
> +	offset += ctx->dst_base;
> +	dst = (void *)fix_to_virt(idx) + (offset & ~PAGE_MASK);
> +	set_fixmap(idx, offset);
> +	if (PFN_DOWN(offset) != PFN_DOWN(offset + ctx->var2 - 1)) {
> +		idx = virt_to_fix((unsigned long)dst + PAGE_SIZE);
> +		set_fixmap(idx, offset + PAGE_SIZE);
> +	}
> +#else
> +	dst = ioremap(ctx->dst_base + offset, ctx->var2);
> +#endif
> +	if (dst) {
> +		memmove(dst, src, ctx->var2);
> +		iounmap(dst);
> +	} else
> +		rc = -ENOMEM;
> +
> +	iounmap(src);
> +
> +	return rc;
>  }
>  
>  static struct apei_exec_ins_type erst_ins_type[] = {

On 17/10/2012 12:53, "Jan Beulich" <JBeulich@suse.com> wrote:
> Before anyone asks - yes, I have added to my todo list to put in
> place a proper ioremap() for x86. But since I''m under some time
> pressure right now, this was the faster route to something that
> can be committed to see whether the regression goes away.
That''s good as we''ll need it for >4kB per-VCPU stacks (even
if we only need
4kB VCPU stacks, we''ll want a guard page too).

 -- Keir

>>> On 17.10.12 at 14:07, Keir Fraser <keir@xen.org> wrote:
> On 17/10/2012 12:53, "Jan Beulich" <JBeulich@suse.com>
wrote:
> 
>> Before anyone asks - yes, I have added to my todo list to put in
>> place a proper ioremap() for x86. But since I''m under some
time
>> pressure right now, this was the faster route to something that
>> can be committed to see whether the regression goes away.
> 
> That''s good as we''ll need it for >4kB per-VCPU stacks
(even if we only need
> 4kB VCPU stacks, we''ll want a guard page too).
But why would you want to do that via ioremap()?

Jan

On 17/10/2012 13:15, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> On 17.10.12 at 14:07, Keir Fraser <keir@xen.org> wrote:
>> On 17/10/2012 12:53, "Jan Beulich" <JBeulich@suse.com>
wrote:
>> 
>>> Before anyone asks - yes, I have added to my todo list to put in
>>> place a proper ioremap() for x86. But since I''m under some
time
>>> pressure right now, this was the faster route to something that
>>> can be committed to see whether the regression goes away.
>> 
>> That''s good as we''ll need it for >4kB per-VCPU
stacks (even if we only need
>> 4kB VCPU stacks, we''ll want a guard page too).
> 
> But why would you want to do that via ioremap()?
Well the required missing mechanism is the same, right? The ability to
allocate some unused virtual address space. We already have
map_pages_to_xen(), so it''s just the allocation part that is missing
for
either ioremap() or vcpu stacks.

 -- Keir
> Jan
>