[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [XEN PATCH][for-4.19 v4 1/8] xen/include: add macro ISOLATE_LOW_BIT



On 2023-10-31 11:20, Jan Beulich wrote:
On 31.10.2023 11:03, Nicola Vetrini wrote:
On 2023-10-31 09:28, Nicola Vetrini wrote:
On 2023-10-31 08:43, Jan Beulich wrote:
On 30.10.2023 23:44, Stefano Stabellini wrote:
On Mon, 30 Oct 2023, Jan Beulich wrote:
On 27.10.2023 15:34, Nicola Vetrini wrote:
--- a/xen/include/xen/macros.h
+++ b/xen/include/xen/macros.h
@@ -8,8 +8,14 @@
 #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
 #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))

-#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
-#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
+/*
+ * Given an unsigned integer argument, expands to a mask where
just the least
+ * significant nonzero bit of the argument is set, or 0 if no bits
are set.
+ */
+#define ISOLATE_LOW_BIT(x) ((x) & -(x))

Not even considering future Misra changes (which aiui may require
that
anyway), this generalization of the macro imo demands that its
argument
now be evaluated only once.

Fur sure that would be an improvement, but I don't see a trivial way
to
do it and this issue is also present today before the patch.

This was an issue here for MASK_EXTR() and MASK_INSR(), yes, but the
new
macro has wider use, and there was no issue elsewhere so far.

I think it
would be better to avoid scope-creeping this patch as we are already
at
v4 for something that was expected to be a trivial mechanical change.
I
would rather review the fix as a separate patch, maybe sent by you as
you probably have a specific implementation in mind?

#define ISOLATE_LOW_BIT(x) ({ \
    typeof(x) x_ = (x); \
    x_ & -x_; \
})

Hard to see the scope creep here. What I would consider scope creep I
specifically didn't even ask for: I'd like this macro to be
overridable
by an arch. Specifically (see my earlier naming hint) I'd like to use x86's BMI insn BLSI in the context of "x86: allow Kconfig control over
psABI level", when ABI v2 or higher is in use.

I appreciate you suggesting an implementation; I'll send a v5
incorporating it.

There's an issue with this approach, though: since the macro is used
indirectly
in expressions that are e.g. case labels or array sizes, the build fails
(see [1] for instance).
Perhaps it's best to leave it as is?

Hmm. I'm afraid it's not an option to "leave as is", not the least because
- as said - I'm under the impression that another Misra rule requires
macro arguments to be evaluated exactly once. Best I can think of right
away is to have a macro for limited use (to address such build issues)
plus an inline function (for general use). But yes, maybe that then indeed
needs to be a 2nd step.

Jan

[1] https://gitlab.com/xen-project/people/bugseng/xen/-/jobs/5423693947


There is no such MISRA Rule afaik: R23.7 is similar, but only for C11 generic selections.

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.