* [PATCH] x86/mmuext: Unify okay/rc error handling in do_mmuext_op()
@ 2015-12-21 17:16 Andrew Cooper
2015-12-21 17:50 ` Boris Ostrovsky
2015-12-22 8:57 ` Jan Beulich
0 siblings, 2 replies; 5+ messages in thread
From: Andrew Cooper @ 2015-12-21 17:16 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Boris Ostrovsky, Jan Beulich
c/s 506db90 "x86/HVM: merge HVM and PVH hypercall tables" introduced a path
whereby 'okay' was used uninitialised, with broke compilation on CentOS 7.
Splitting the error handling like this is fragile and unnecessary. Drop the
okay variable entirely and just use rc directly, substituting rc = -EINVAL/0
for okay = 0/1.
In addition, two error messages are updated to print rc, and some stray
whitespace is dropped.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
xen/arch/x86/mm.c | 65 +++++++++++++++++++++++++------------------------------
1 file changed, 30 insertions(+), 35 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 717546a..977b2f4 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2974,7 +2974,7 @@ long do_mmuext_op(
struct vcpu *curr = current;
struct domain *d = curr->domain;
struct domain *pg_owner;
- int okay, rc = put_old_guest_table(curr);
+ int rc = put_old_guest_table(curr);
if ( unlikely(rc) )
{
@@ -3053,7 +3053,7 @@ long do_mmuext_op(
}
}
- okay = 1;
+ rc = 0;
switch ( op.cmd )
{
@@ -3087,33 +3087,32 @@ long do_mmuext_op(
page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL, P2M_ALLOC);
if ( unlikely(!page) )
{
- okay = 0;
+ rc = -EINVAL;
break;
}
rc = get_page_type_preemptible(page, type);
- okay = !rc;
- if ( unlikely(!okay) )
+ if ( unlikely(rc) )
{
if ( rc == -EINTR )
rc = -ERESTART;
else if ( rc != -ERESTART )
- MEM_LOG("Error while pinning mfn %lx", page_to_mfn(page));
+ MEM_LOG("Error %d while pinning mfn %lx",
+ rc, page_to_mfn(page));
if ( page != curr->arch.old_guest_table )
put_page(page);
break;
}
- if ( (rc = xsm_memory_pin_page(XSM_HOOK, d, pg_owner, page)) != 0 )
- okay = 0;
- else if ( unlikely(test_and_set_bit(_PGT_pinned,
- &page->u.inuse.type_info)) )
+ rc = xsm_memory_pin_page(XSM_HOOK, d, pg_owner, page);
+ if ( !rc && unlikely(test_and_set_bit(_PGT_pinned,
+ &page->u.inuse.type_info)) )
{
MEM_LOG("Mfn %lx already pinned", page_to_mfn(page));
- okay = 0;
+ rc = -EINVAL;
}
- if ( unlikely(!okay) )
+ if ( unlikely(rc) )
goto pin_drop;
/* A page is dirtied when its pin status is set. */
@@ -3150,14 +3149,14 @@ long do_mmuext_op(
page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL, P2M_ALLOC);
if ( unlikely(!page) )
{
- okay = 0;
+ rc = -EINVAL;
MEM_LOG("Mfn %lx bad domain", op.arg1.mfn);
break;
}
if ( !test_and_clear_bit(_PGT_pinned, &page->u.inuse.type_info) )
{
- okay = 0;
+ rc = -EINVAL;
put_page(page);
MEM_LOG("Mfn %lx not pinned", op.arg1.mfn);
break;
@@ -3212,20 +3211,18 @@ long do_mmuext_op(
if ( op.arg1.mfn != 0 )
{
if ( paging_mode_refcounts(d) )
- okay = get_page_from_pagenr(op.arg1.mfn, d);
+ rc = get_page_from_pagenr(op.arg1.mfn, d) ? 0 : -EINVAL;
else
- {
rc = get_page_and_type_from_pagenr(
op.arg1.mfn, PGT_root_page_table, d, 0, 1);
- okay = !rc;
- }
- if ( unlikely(!okay) )
+
+ if ( unlikely(rc) )
{
if ( rc == -EINTR )
rc = -ERESTART;
else if ( rc != -ERESTART )
- MEM_LOG("Error while installing new mfn %lx",
- op.arg1.mfn);
+ MEM_LOG("Error %d while installing new mfn %lx",
+ rc, op.arg1.mfn);
break;
}
if ( VM_ASSIST(d, m2p_strict) && !paging_mode_refcounts(d) )
@@ -3248,7 +3245,6 @@ long do_mmuext_op(
/* fallthrough */
case -ERESTART:
curr->arch.old_guest_table = page;
- okay = 0;
break;
default:
BUG_ON(rc);
@@ -3258,14 +3254,14 @@ long do_mmuext_op(
break;
}
-
+
case MMUEXT_TLB_FLUSH_LOCAL:
if ( likely(d == pg_owner) )
flush_tlb_local();
else
rc = -EPERM;
break;
-
+
case MMUEXT_INVLPG_LOCAL:
if ( unlikely(d != pg_owner) )
rc = -EPERM;
@@ -3342,7 +3338,7 @@ long do_mmuext_op(
else
{
MEM_LOG("Non-physdev domain tried to FLUSH_CACHE_GLOBAL");
- okay = 0;
+ rc = -EINVAL;
}
break;
@@ -3356,12 +3352,12 @@ long do_mmuext_op(
else if ( paging_mode_external(d) )
{
MEM_LOG("ignoring SET_LDT hypercall from external domain");
- okay = 0;
+ rc = -EINVAL;
}
else if ( ((ptr & (PAGE_SIZE - 1)) != 0) || !__addr_ok(ptr) ||
(ents > 8192) )
{
- okay = 0;
+ rc = -EINVAL;
MEM_LOG("Bad args to SET_LDT: ptr=%lx, ents=%lx", ptr, ents);
}
else if ( (curr->arch.pv_vcpu.ldt_ents != ents) ||
@@ -3385,7 +3381,7 @@ long do_mmuext_op(
if ( page )
put_page(page);
MEM_LOG("Error while clearing mfn %lx", op.arg1.mfn);
- okay = 0;
+ rc = -EINVAL;
break;
}
@@ -3406,15 +3402,16 @@ long do_mmuext_op(
P2M_ALLOC);
if ( unlikely(!src_page) )
{
- okay = 0;
+ rc = -EINVAL;
MEM_LOG("Error while copying from mfn %lx", op.arg2.src_mfn);
break;
}
dst_page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL,
P2M_ALLOC);
- okay = (dst_page && get_page_type(dst_page, PGT_writable_page));
- if ( unlikely(!okay) )
+ rc = (dst_page &&
+ get_page_type(dst_page, PGT_writable_page)) ? 0 : -EINVAL;
+ if ( unlikely(rc) )
{
put_page(src_page);
if ( dst_page )
@@ -3443,7 +3440,7 @@ long do_mmuext_op(
else if ( mfn & (L1_PAGETABLE_ENTRIES-1) )
{
MEM_LOG("Unaligned superpage reference mfn %lx", mfn);
- okay = 0;
+ rc = -EINVAL;
}
else if ( !opt_allow_superpage )
{
@@ -3464,7 +3461,7 @@ long do_mmuext_op(
else if ( mfn & (L1_PAGETABLE_ENTRIES-1) )
{
MEM_LOG("Unaligned superpage reference mfn %lx", mfn);
- okay = 0;
+ rc = -EINVAL;
}
else if ( !opt_allow_superpage )
{
@@ -3483,8 +3480,6 @@ long do_mmuext_op(
}
done:
- if ( unlikely(!okay) && !rc )
- rc = -EINVAL;
if ( unlikely(rc) )
break;
--
2.1.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/mmuext: Unify okay/rc error handling in do_mmuext_op()
2015-12-21 17:16 [PATCH] x86/mmuext: Unify okay/rc error handling in do_mmuext_op() Andrew Cooper
@ 2015-12-21 17:50 ` Boris Ostrovsky
2015-12-22 9:56 ` Andrew Cooper
2015-12-22 8:57 ` Jan Beulich
1 sibling, 1 reply; 5+ messages in thread
From: Boris Ostrovsky @ 2015-12-21 17:50 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: Jan Beulich
On 12/21/2015 12:16 PM, Andrew Cooper wrote:
> c/s 506db90 "x86/HVM: merge HVM and PVH hypercall tables" introduced a path
> whereby 'okay' was used uninitialised, with broke compilation on CentOS 7.
>
> Splitting the error handling like this is fragile and unnecessary. Drop the
> okay variable entirely and just use rc directly, substituting rc = -EINVAL/0
> for okay = 0/1.
>
> In addition, two error messages are updated to print rc, and some stray
> whitespace is dropped.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
with a couple of style nits:
> @@ -3258,14 +3254,14 @@ long do_mmuext_op(
>
> break;
> }
> -
> +
> case MMUEXT_TLB_FLUSH_LOCAL:
> if ( likely(d == pg_owner) )
> flush_tlb_local();
> else
> rc = -EPERM;
> break;
> -
> +
> case MMUEXT_INVLPG_LOCAL:
> if ( unlikely(d != pg_owner) )
> rc = -EPERM;
These look like stray changes.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/mmuext: Unify okay/rc error handling in do_mmuext_op()
2015-12-21 17:16 [PATCH] x86/mmuext: Unify okay/rc error handling in do_mmuext_op() Andrew Cooper
2015-12-21 17:50 ` Boris Ostrovsky
@ 2015-12-22 8:57 ` Jan Beulich
2015-12-22 9:09 ` Andrew Cooper
1 sibling, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2015-12-22 8:57 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Boris Ostrovsky, Xen-devel
>>> On 21.12.15 at 18:16, <andrew.cooper3@citrix.com> wrote:
> c/s 506db90 "x86/HVM: merge HVM and PVH hypercall tables" introduced a path
> whereby 'okay' was used uninitialised, with broke compilation on CentOS 7.
It appeared to be used uninitialized, but wasn't in fact (i.e. the
outcome - the value rc gets set to - didn't depend on the value
due to
if ( unlikely(!okay) && !rc )
rc = -EINVAL;
being equivalent to
if ( !rc && unlikely(!okay) )
rc = -EINVAL;
(no side effects for the expressions on either side of the &&).
I'll re-word accordingly upon committing, to not give the false
impression of there having been other than a cosmetic problem.
> Splitting the error handling like this is fragile and unnecessary. Drop the
> okay variable entirely and just use rc directly, substituting rc = -EINVAL/0
> for okay = 0/1.
>
> In addition, two error messages are updated to print rc, and some stray
> whitespace is dropped.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/mmuext: Unify okay/rc error handling in do_mmuext_op()
2015-12-22 8:57 ` Jan Beulich
@ 2015-12-22 9:09 ` Andrew Cooper
0 siblings, 0 replies; 5+ messages in thread
From: Andrew Cooper @ 2015-12-22 9:09 UTC (permalink / raw)
To: Jan Beulich; +Cc: Boris Ostrovsky, Xen-devel
On 22/12/2015 08:57, Jan Beulich wrote:
>>>> On 21.12.15 at 18:16, <andrew.cooper3@citrix.com> wrote:
>> c/s 506db90 "x86/HVM: merge HVM and PVH hypercall tables" introduced a path
>> whereby 'okay' was used uninitialised, with broke compilation on CentOS 7.
> It appeared to be used uninitialized, but wasn't in fact (i.e. the
> outcome - the value rc gets set to - didn't depend on the value
> due to
>
> if ( unlikely(!okay) && !rc )
> rc = -EINVAL;
>
> being equivalent to
>
> if ( !rc && unlikely(!okay) )
> rc = -EINVAL;
>
> (no side effects for the expressions on either side of the &&).
> I'll re-word accordingly upon committing, to not give the false
> impression of there having been other than a cosmetic problem.
There is a real problem. Because the compiler is able to prove that
okay is genuinely read uninitialised in one case, the rules concerning
undefined behaviour permit it to do anything it wishes, including
omitting this if statement.
As far as practical problems go however, it is the build breakage which
is relevant, and it breaks because of a -Werror=maybe-uninitialised.
~Andrew
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/mmuext: Unify okay/rc error handling in do_mmuext_op()
2015-12-21 17:50 ` Boris Ostrovsky
@ 2015-12-22 9:56 ` Andrew Cooper
0 siblings, 0 replies; 5+ messages in thread
From: Andrew Cooper @ 2015-12-22 9:56 UTC (permalink / raw)
To: Boris Ostrovsky, Xen-devel; +Cc: Jan Beulich
On 21/12/2015 17:50, Boris Ostrovsky wrote:
> On 12/21/2015 12:16 PM, Andrew Cooper wrote:
>> c/s 506db90 "x86/HVM: merge HVM and PVH hypercall tables" introduced
>> a path
>> whereby 'okay' was used uninitialised, with broke compilation on
>> CentOS 7.
>>
>> Splitting the error handling like this is fragile and unnecessary.
>> Drop the
>> okay variable entirely and just use rc directly, substituting rc =
>> -EINVAL/0
>> for okay = 0/1.
>>
>> In addition, two error messages are updated to print rc, and some stray
>> whitespace is dropped.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>
> with a couple of style nits:
>
>> @@ -3258,14 +3254,14 @@ long do_mmuext_op(
>> break;
>> }
>> -
>> +
>> case MMUEXT_TLB_FLUSH_LOCAL:
>> if ( likely(d == pg_owner) )
>> flush_tlb_local();
>> else
>> rc = -EPERM;
>> break;
>> -
>> +
>> case MMUEXT_INVLPG_LOCAL:
>> if ( unlikely(d != pg_owner) )
>> rc = -EPERM;
>
> These look like stray changes.
They are specifically the stray bits of whitespace referred to in the
commit message.
~Andrew
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-12-22 9:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-21 17:16 [PATCH] x86/mmuext: Unify okay/rc error handling in do_mmuext_op() Andrew Cooper
2015-12-21 17:50 ` Boris Ostrovsky
2015-12-22 9:56 ` Andrew Cooper
2015-12-22 8:57 ` Jan Beulich
2015-12-22 9:09 ` Andrew Cooper
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.