diff mbox series

[01/16] x86/shadow: fix and improve sh_page_has_multiple_shadows()

Message ID 290c9058-9907-4c6d-3fe8-987868a3a843@suse.com (mailing list archive)
State New, archived
Headers show
Series x86: assorted (mostly) shadow mode adjustments | expand

Commit Message

Jan Beulich March 22, 2023, 9:29 a.m. UTC 
While no caller currently invokes the function without first making sure
there is at least one shadow [1], we'd better eliminate UB here:
find_first_set_bit() requires input to be non-zero to return a well-
defined result.

Further, using find_first_set_bit() isn't very efficient in the first
place for the intended purpose.

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

[1] The function has exactly two uses, and both are from OOS code, which
    is HVM-only. For HVM (but not for PV) sh_mfn_is_a_page_table(),
    guarding the call to sh_unsync(), guarantees at least one shadow.
    Hence even if sh_page_has_multiple_shadows() returned a bogus value
    when invoked for a PV domain, the subsequent is_hvm_vcpu() and
    oos_active checks (the former being redundant with the latter) will
    compensate. (Arguably that oos_active check should come first, for
    both clarity and efficiency reasons.)
---
Considering present uses, ASSERT(shadows) might be an option as well,
instead of making the null check part of the return value expression.

Comments

Andrew Cooper March 23, 2023, 11:55 a.m. UTC | #1 
On 22/03/2023 9:29 am, Jan Beulich wrote:
> While no caller currently invokes the function without first making sure
> there is at least one shadow [1], we'd better eliminate UB here:
> find_first_set_bit() requires input to be non-zero to return a well-
> defined result.
>
> Further, using find_first_set_bit() isn't very efficient in the first
> place for the intended purpose.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Patch

--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -332,7 +332,7 @@  static inline int sh_page_has_multiple_s
         return 0;
     shadows = pg->shadow_flags & SHF_page_type_mask;
     /* More than one type bit set in shadow-flags? */
-    return ( (shadows & ~(1UL << find_first_set_bit(shadows))) != 0 );
+    return shadows && (shadows & (shadows - 1));
 }
 
 #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)