[PATCH] x86/p2m: also clear defer_nested_flush on error

[PATCH] x86/p2m: also clear defer_nested_flush on error

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]

While presumably benign (domain creation ought to fail when P2M setup
fails) let's be on the safe side and clear the flag as intended. And
actually, the code can be streamlined quite a bit by recognizing that
the only difference between the success and error cases is the message
printed in each case. With that, a stray spin_unlock() also goes away.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -435,18 +435,12 @@ int p2m_alloc_table(struct p2m_domain *p
     p2m->defer_nested_flush = 1;
     rc = p2m_set_entry(p2m, 0, _mfn(INVALID_MFN), PAGE_ORDER_4K,
                        p2m_invalid, p2m->default_access);
-    if ( rc )
-        goto error;
     p2m->defer_nested_flush = 0;
-
-    P2M_PRINTK("p2m table initialised for slot zero\n");
-    p2m_unlock(p2m);
-    return 0;
-
-    spin_unlock(&p2m->domain->page_alloc_lock);
- error:
-    P2M_PRINTK("failed to initialise p2m table for slot zero. rc:%d\n", rc);
     p2m_unlock(p2m);
+    if ( !rc )
+        P2M_PRINTK("p2m table initialised for slot zero\n");
+    else
+        P2M_PRINTK("failed to initialise p2m table for slot zero (%d)\n", rc);
     return rc;
 }
 




[-- Attachment #2: x86-p2m-clear-defer_nested_flush.patch --]
[-- Type: text/plain, Size: 1278 bytes --]

x86/p2m: also clear defer_nested_flush on error

While presumably benign (domain creation ought to fail when P2M setup
fails) let's be on the safe side and clear the flag as intended. And
actually, the code can be streamlined quite a bit by recognizing that
the only difference between the success and error cases is the message
printed in each case. With that, a stray spin_unlock() also goes away.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -435,18 +435,12 @@ int p2m_alloc_table(struct p2m_domain *p
     p2m->defer_nested_flush = 1;
     rc = p2m_set_entry(p2m, 0, _mfn(INVALID_MFN), PAGE_ORDER_4K,
                        p2m_invalid, p2m->default_access);
-    if ( rc )
-        goto error;
     p2m->defer_nested_flush = 0;
-
-    P2M_PRINTK("p2m table initialised for slot zero\n");
-    p2m_unlock(p2m);
-    return 0;
-
-    spin_unlock(&p2m->domain->page_alloc_lock);
- error:
-    P2M_PRINTK("failed to initialise p2m table for slot zero. rc:%d\n", rc);
     p2m_unlock(p2m);
+    if ( !rc )
+        P2M_PRINTK("p2m table initialised for slot zero\n");
+    else
+        P2M_PRINTK("failed to initialise p2m table for slot zero (%d)\n", rc);
     return rc;
 }
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] x86/p2m: also clear defer_nested_flush on error

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 1647 bytes --]

On 15/04/14 15:10, Jan Beulich wrote:
> While presumably benign (domain creation ought to fail when P2M setup
> fails) let's be on the safe side and clear the flag as intended. And
> actually, the code can be streamlined quite a bit by recognizing that
> the only difference between the success and error cases is the message
> printed in each case. With that, a stray spin_unlock() also goes away.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

I would like to hope that any optimising compiler would make that dead
spin_unlock() disappear, but it is certainly good to properly get rid of it.

>
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -435,18 +435,12 @@ int p2m_alloc_table(struct p2m_domain *p
>      p2m->defer_nested_flush = 1;
>      rc = p2m_set_entry(p2m, 0, _mfn(INVALID_MFN), PAGE_ORDER_4K,
>                         p2m_invalid, p2m->default_access);
> -    if ( rc )
> -        goto error;
>      p2m->defer_nested_flush = 0;
> -
> -    P2M_PRINTK("p2m table initialised for slot zero\n");
> -    p2m_unlock(p2m);
> -    return 0;
> -
> -    spin_unlock(&p2m->domain->page_alloc_lock);
> - error:
> -    P2M_PRINTK("failed to initialise p2m table for slot zero. rc:%d\n", rc);
>      p2m_unlock(p2m);
> +    if ( !rc )
> +        P2M_PRINTK("p2m table initialised for slot zero\n");
> +    else
> +        P2M_PRINTK("failed to initialise p2m table for slot zero (%d)\n", rc);
>      return rc;
>  }
>  
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 2548 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] x86/p2m: also clear defer_nested_flush on error

[Egger, Christoph]

On 15.04.14 16:10, Jan Beulich wrote:
> While presumably benign (domain creation ought to fail when P2M setup
> fails) let's be on the safe side and clear the flag as intended. And
> actually, the code can be streamlined quite a bit by recognizing that
> the only difference between the success and error cases is the message
> printed in each case. With that, a stray spin_unlock() also goes away.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Nice cleanup!

Reviewed-by: Christoph Egger <chegger@amazon.de>

> 
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -435,18 +435,12 @@ int p2m_alloc_table(struct p2m_domain *p
>      p2m->defer_nested_flush = 1;
>      rc = p2m_set_entry(p2m, 0, _mfn(INVALID_MFN), PAGE_ORDER_4K,
>                         p2m_invalid, p2m->default_access);
> -    if ( rc )
> -        goto error;
>      p2m->defer_nested_flush = 0;
> -
> -    P2M_PRINTK("p2m table initialised for slot zero\n");
> -    p2m_unlock(p2m);
> -    return 0;
> -
> -    spin_unlock(&p2m->domain->page_alloc_lock);
> - error:
> -    P2M_PRINTK("failed to initialise p2m table for slot zero. rc:%d\n", rc);
>      p2m_unlock(p2m);
> +    if ( !rc )
> +        P2M_PRINTK("p2m table initialised for slot zero\n");
> +    else
> +        P2M_PRINTK("failed to initialise p2m table for slot zero (%d)\n", rc);
>      return rc;
>  }

Re: [PATCH] x86/p2m: also clear defer_nested_flush on error

[Tim Deegan]

At 15:10 +0100 on 15 Apr (1397571007), Jan Beulich wrote:
> While presumably benign (domain creation ought to fail when P2M setup
> fails) let's be on the safe side and clear the flag as intended. And
> actually, the code can be streamlined quite a bit by recognizing that
> the only difference between the success and error cases is the message
> printed in each case. With that, a stray spin_unlock() also goes away.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Applied, thanks.

Tim.