[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [XEN PATCH][for-4.19 8/9] xen/types: address Rule 10.1 for DECLARE_BITMAP use
- To: Julien Grall <julien@xxxxxxx>
- From: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
- Date: Tue, 10 Oct 2023 16:20:33 +0200
- Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx, michal.orzel@xxxxxxx, xenia.ragiadakou@xxxxxxx, ayan.kumar.halder@xxxxxxx, consulting@xxxxxxxxxxx, jbeulich@xxxxxxxx, andrew.cooper3@xxxxxxxxxx, roger.pau@xxxxxxxxxx, Paul Durrant <paul@xxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>
- Delivery-date: Tue, 10 Oct 2023 14:20:51 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
On 10/10/2023 12:53, Julien Grall wrote:
On 10/10/2023 02:09, Stefano Stabellini wrote:
On Mon, 9 Oct 2023, Julien Grall wrote:
On 07/10/2023 02:04, Stefano Stabellini wrote:
On Fri, 6 Oct 2023, Julien Grall wrote:
Hi Nicola,
On 06/10/2023 11:10, Nicola Vetrini wrote:
On 06/10/2023 11:34, Julien Grall wrote:
Hi Nicola,
On 06/10/2023 09:26, Nicola Vetrini wrote:
Given its use in the declaration
'DECLARE_BITMAP(features, IOMMU_FEAT_count)' the argument
'bits' has essential type 'enum iommu_feature', which is not
allowed by the Rule as an operand to the addition operator
in macro 'BITS_TO_LONGS'.
A comment in BITS_TO_LONGS is added to make it clear that
values passed are meant to be positive.
I am confused. If the value is meant to be positive. Then...
Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
---
xen/include/xen/iommu.h | 2 +-
xen/include/xen/types.h | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 0e747b0bbc1c..34aa0b9b5b81 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -360,7 +360,7 @@ struct domain_iommu {
#endif
/* Features supported by the IOMMU */
- DECLARE_BITMAP(features, IOMMU_FEAT_count);
+ DECLARE_BITMAP(features, (int)IOMMU_FEAT_count);
... why do we cast to (int) rather than (unsigned int)? Also, I
think
this cast deserve a comment on top because this is not a very
obvious
one.
/* Does the guest share HAP mapping with the IOMMU? */
bool hap_pt_share;
diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
index aea259db1ef2..936e83d333a0 100644
--- a/xen/include/xen/types.h
+++ b/xen/include/xen/types.h
@@ -22,6 +22,7 @@ typedef signed long ssize_t;
typedef __PTRDIFF_TYPE__ ptrdiff_t;
+/* Users of this macro are expected to pass a positive
value */
#define BITS_TO_LONGS(bits) \
(((bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
#define DECLARE_BITMAP(name,bits) \
Cheers,
See [1] for the reason why I did so. I should have mentioned that
in the
commit notes, sorry.
In short, making BITS_TO_LONGS essentially unsigned would cause a
cascade of
build errors and
possibly other essential type violations.
Can you share some of the errors?
If this is to be fixed that way, the effort required
is far greater. Either way, a comment on top of can be added,
along the
lines of:
Leaving this as an enum would violate MISRA C:2012 Rule 10.1
I read this as you are simply trying to silence your tool. I think
this
the
wrong approach as you are now making the code confusing for the
reader
(you
pass a signed int to a function that is supposed to take a positive
number).
I appreciate that this will result to more violations at the
beginning.
But
the whole point of MISRA is to make the code better.
If this is too complex to solve now, then a possible option is to
deviate
for
the time being.
I agree on everything Julien's wrote and I was about to suggest to
use a
SAF comment to suppress the warning because it is clearer than a int
cast.
But then I realized that even if BITS_TO_LONGS was fixed, wouldn't
still
we have a problem because IOMMU_FEAT_count is an enum?
Is it the case that IOMMU_FEAT_count would have to be cast
regardless,
either to int or unsigned int or whatever to be used in
DECLARE_BITMAP?
So we have 2 problems here: one problem is DECLARE_BITMAP taking int
instead of unsigned int, and another problem is IOMMU_FEAT_count
being
an enum.
If I got it right, then I would go with the cast to int (like done
in
this patch) with a decent comment on top of it.
I might be missing something here. But why should we use cast rather
than /*
SAF-X */ just above? I would have expected we wanted to highlight the
violation rather than hiding it.
My understanding is that the cast is required when converting an enum
type to an integer type and vice versa. The idea is that we shouldn't
do
implicit conversions as they are error prone, only explicit
conversions
are allowed between enum and integers.
We have a lot of places in Xen using implicit conversion from enum to
integer (for instance in the P2M code for p2m_type_t). Does ECLAIR
report all of them? If not, then why?
Re-replying here, since in the other reply I didn't address your concern
fully:
yes, there are more than a few places where this comes up for Rule 10.1,
especially
in x86 code. In theory a cast is not the only option to bring the code
into compliance,
but the specific solution should be checked on a case-by-case basis.
The main aim of the series on R10.1 is to deviate or fix the main
offenders in terms
of violations with as little effort as possible, to have a more
manageable analysis
result (in my branch, with some patches yet to be submitted I'm down to
a few violations
on ARM and ~100 on x86).
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
|
