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.vetr...@bugseng.com>
---
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?
So we need either (int) or (unsigned int). The question is which one
between the two, and theoretically (unsigned int) is better but due to
the reasons above (int) is the simplest choice.
Yes, instead of the cast we can also add a SAF-x comment, which refers
to a deviation that says something along the lines "we could fix this
with a cast but we prefer a deviation because it makes the code easier
to read".
In general my personal preference would be to use a cast, because we
shouldn't implicitly convert enums to integers.
See above. I'd like to understand whether we are going to sprinkle the
code with cast. If so, I am afraid I am against this solution.
Cheers,
--
Julien Grall