With the wider testing, some more violations have been spotted. This
addresses violations of Rule 20.7 which requires macro parameters to be
bracketed.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: consulting@bugseng.com <consulting@bugseng.com>
CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
xen/arch/x86/mm/shadow/multi.c | 2 +-
xen/arch/x86/mm/shadow/private.h | 6 +++---
xen/drivers/passthrough/vtd/dmar.h | 2 +-
xen/include/xen/kexec.h | 4 ++--
4 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 03be61e225c0..36ee6554b4c4 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -781,7 +781,7 @@ do { \
(_sl1e) = _sp + _i; \
if ( shadow_l1e_get_flags(*(_sl1e)) & _PAGE_PRESENT ) \
{_code} \
- if ( _done ) break; \
+ if ( (_done) ) break; \
increment_ptr_to_guest_entry(_gl1p); \
} \
unmap_domain_page(_sp); \
diff --git a/xen/arch/x86/mm/shadow/private.h b/xen/arch/x86/mm/shadow/private.h
index cef9dbef2e77..93834ec55c42 100644
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -636,9 +636,9 @@ prev_pinned_shadow(struct page_info *page,
}
#define foreach_pinned_shadow(dom, pos, tmp) \
- for ( pos = prev_pinned_shadow(NULL, (dom)); \
- pos ? (tmp = prev_pinned_shadow(pos, (dom)), 1) : 0; \
- pos = tmp )
+ for ( (pos) = prev_pinned_shadow(NULL, dom); \
+ (pos) ? (tmp = prev_pinned_shadow(pos, dom), 1) : 0; \
+ (pos) = tmp )
/*
* Pin a shadow page: take an extra refcount, set the pin bit,
diff --git a/xen/drivers/passthrough/vtd/dmar.h b/xen/drivers/passthrough/vtd/dmar.h
index 0ff4f365351f..11590f71a828 100644
--- a/xen/drivers/passthrough/vtd/dmar.h
+++ b/xen/drivers/passthrough/vtd/dmar.h
@@ -124,7 +124,7 @@ struct acpi_atsr_unit *acpi_find_matched_atsr_unit(const struct pci_dev *);
do { \
s_time_t start_time = NOW(); \
while (1) { \
- sts = op(iommu->reg, offset); \
+ sts = op((iommu)->reg, offset); \
if ( cond ) \
break; \
if ( NOW() > start_time + DMAR_OPERATION_TIMEOUT ) { \
diff --git a/xen/include/xen/kexec.h b/xen/include/xen/kexec.h
index e66eb6a8e593..5dd288d1a50e 100644
--- a/xen/include/xen/kexec.h
+++ b/xen/include/xen/kexec.h
@@ -66,9 +66,9 @@ void vmcoreinfo_append_str(const char *fmt, ...)
#define VMCOREINFO_PAGESIZE(value) \
vmcoreinfo_append_str("PAGESIZE=%ld\n", value)
#define VMCOREINFO_SYMBOL(name) \
- vmcoreinfo_append_str("SYMBOL(%s)=%lx\n", #name, (unsigned long)&name)
+ vmcoreinfo_append_str("SYMBOL(%s)=%lx\n", #name, (unsigned long)&(name))
#define VMCOREINFO_SYMBOL_ALIAS(alias, name) \
- vmcoreinfo_append_str("SYMBOL(%s)=%lx\n", #alias, (unsigned long)&name)
+ vmcoreinfo_append_str("SYMBOL(%s)=%lx\n", #alias, (unsigned long)&(name))
#define VMCOREINFO_STRUCT_SIZE(name) \
vmcoreinfo_append_str("SIZE(%s)=%zu\n", #name, sizeof(struct name))
#define VMCOREINFO_OFFSET(name, field) \
--
2.39.5
On 10.12.2025 19:30, Andrew Cooper wrote:
> With the wider testing, some more violations have been spotted. This
> addresses violations of Rule 20.7 which requires macro parameters to be
> bracketed.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: consulting@bugseng.com <consulting@bugseng.com>
> CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> xen/arch/x86/mm/shadow/multi.c | 2 +-
> xen/arch/x86/mm/shadow/private.h | 6 +++---
> xen/drivers/passthrough/vtd/dmar.h | 2 +-
> xen/include/xen/kexec.h | 4 ++--
> 4 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
> index 03be61e225c0..36ee6554b4c4 100644
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -781,7 +781,7 @@ do { \
> (_sl1e) = _sp + _i; \
> if ( shadow_l1e_get_flags(*(_sl1e)) & _PAGE_PRESENT ) \
> {_code} \
> - if ( _done ) break; \
> + if ( (_done) ) break; \
I don't understand this: There are parentheses already from if() itself.
> --- a/xen/arch/x86/mm/shadow/private.h
> +++ b/xen/arch/x86/mm/shadow/private.h
> @@ -636,9 +636,9 @@ prev_pinned_shadow(struct page_info *page,
> }
>
> #define foreach_pinned_shadow(dom, pos, tmp) \
> - for ( pos = prev_pinned_shadow(NULL, (dom)); \
> - pos ? (tmp = prev_pinned_shadow(pos, (dom)), 1) : 0; \
> - pos = tmp )
> + for ( (pos) = prev_pinned_shadow(NULL, dom); \
> + (pos) ? (tmp = prev_pinned_shadow(pos, dom), 1) : 0; \
> + (pos) = tmp )
What about tmp (twice)?
Jan
On 2025-12-11 09:36, Jan Beulich wrote:
> On 10.12.2025 19:30, Andrew Cooper wrote:
>> With the wider testing, some more violations have been spotted. This
>> addresses violations of Rule 20.7 which requires macro parameters to
>> be
>> bracketed.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: consulting@bugseng.com <consulting@bugseng.com>
>> CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> xen/arch/x86/mm/shadow/multi.c | 2 +-
>> xen/arch/x86/mm/shadow/private.h | 6 +++---
>> xen/drivers/passthrough/vtd/dmar.h | 2 +-
>> xen/include/xen/kexec.h | 4 ++--
>> 4 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/shadow/multi.c
>> b/xen/arch/x86/mm/shadow/multi.c
>> index 03be61e225c0..36ee6554b4c4 100644
>> --- a/xen/arch/x86/mm/shadow/multi.c
>> +++ b/xen/arch/x86/mm/shadow/multi.c
>> @@ -781,7 +781,7 @@ do {
>> \
>> (_sl1e) = _sp + _i;
>> \
>> if ( shadow_l1e_get_flags(*(_sl1e)) & _PAGE_PRESENT )
>> \
>> {_code}
>> \
>> - if ( _done ) break;
>> \
>> + if ( (_done) ) break;
>> \
>
> I don't understand this: There are parentheses already from if()
> itself.
>
Yeah, syntactically there are, but those are parsed as part of the if,
rather than its condition; the AST node contained within does not have
parentheses around it.
>> --- a/xen/arch/x86/mm/shadow/private.h
>> +++ b/xen/arch/x86/mm/shadow/private.h
>> @@ -636,9 +636,9 @@ prev_pinned_shadow(struct page_info *page,
>> }
>>
>> #define foreach_pinned_shadow(dom, pos, tmp) \
>> - for ( pos = prev_pinned_shadow(NULL, (dom)); \
>> - pos ? (tmp = prev_pinned_shadow(pos, (dom)), 1) : 0; \
>> - pos = tmp )
>> + for ( (pos) = prev_pinned_shadow(NULL, dom); \
>> + (pos) ? (tmp = prev_pinned_shadow(pos, dom), 1) : 0; \
>> + (pos) = tmp )
>
> What about tmp (twice)?
>
> Jan
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
On 11.12.2025 10:15, Nicola Vetrini wrote:
> On 2025-12-11 09:36, Jan Beulich wrote:
>> On 10.12.2025 19:30, Andrew Cooper wrote:
>>> With the wider testing, some more violations have been spotted. This
>>> addresses violations of Rule 20.7 which requires macro parameters to
>>> be
>>> bracketed.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>> CC: consulting@bugseng.com <consulting@bugseng.com>
>>> CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>> ---
>>> xen/arch/x86/mm/shadow/multi.c | 2 +-
>>> xen/arch/x86/mm/shadow/private.h | 6 +++---
>>> xen/drivers/passthrough/vtd/dmar.h | 2 +-
>>> xen/include/xen/kexec.h | 4 ++--
>>> 4 files changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/mm/shadow/multi.c
>>> b/xen/arch/x86/mm/shadow/multi.c
>>> index 03be61e225c0..36ee6554b4c4 100644
>>> --- a/xen/arch/x86/mm/shadow/multi.c
>>> +++ b/xen/arch/x86/mm/shadow/multi.c
>>> @@ -781,7 +781,7 @@ do {
>>> \
>>> (_sl1e) = _sp + _i;
>>> \
>>> if ( shadow_l1e_get_flags(*(_sl1e)) & _PAGE_PRESENT )
>>> \
>>> {_code}
>>> \
>>> - if ( _done ) break;
>>> \
>>> + if ( (_done) ) break;
>>> \
>>
>> I don't understand this: There are parentheses already from if()
>> itself.
>
> Yeah, syntactically there are, but those are parsed as part of the if,
> rather than its condition; the AST node contained within does not have
> parentheses around it.
I fear I don't follow. Besides us not using parentheses elsewhere when
if() is used like this macros, the point of requiring parentheses is (aiui)
to make precedence explicit. There already is no ambiguity here due to the
syntactically require parentheses in if(). Why would a rule and/or the
tool require redundant ones?
Jan
On 2025-12-11 10:30, Jan Beulich wrote:
> On 11.12.2025 10:15, Nicola Vetrini wrote:
>> On 2025-12-11 09:36, Jan Beulich wrote:
>>> On 10.12.2025 19:30, Andrew Cooper wrote:
>>>> With the wider testing, some more violations have been spotted.
>>>> This
>>>> addresses violations of Rule 20.7 which requires macro parameters to
>>>> be
>>>> bracketed.
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>>> CC: consulting@bugseng.com <consulting@bugseng.com>
>>>> CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>> ---
>>>> xen/arch/x86/mm/shadow/multi.c | 2 +-
>>>> xen/arch/x86/mm/shadow/private.h | 6 +++---
>>>> xen/drivers/passthrough/vtd/dmar.h | 2 +-
>>>> xen/include/xen/kexec.h | 4 ++--
>>>> 4 files changed, 7 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/mm/shadow/multi.c
>>>> b/xen/arch/x86/mm/shadow/multi.c
>>>> index 03be61e225c0..36ee6554b4c4 100644
>>>> --- a/xen/arch/x86/mm/shadow/multi.c
>>>> +++ b/xen/arch/x86/mm/shadow/multi.c
>>>> @@ -781,7 +781,7 @@ do {
>>>> \
>>>> (_sl1e) = _sp + _i;
>>>> \
>>>> if ( shadow_l1e_get_flags(*(_sl1e)) & _PAGE_PRESENT )
>>>> \
>>>> {_code}
>>>> \
>>>> - if ( _done ) break;
>>>> \
>>>> + if ( (_done) ) break;
>>>> \
>>>
>>> I don't understand this: There are parentheses already from if()
>>> itself.
>>
>> Yeah, syntactically there are, but those are parsed as part of the if,
>> rather than its condition; the AST node contained within does not have
>> parentheses around it.
>
> I fear I don't follow. Besides us not using parentheses elsewhere when
> if() is used like this macros, the point of requiring parentheses is
> (aiui)
> to make precedence explicit. There already is no ambiguity here due to
> the
> syntactically require parentheses in if(). Why would a rule and/or the
> tool require redundant ones?
>
this is parsed as (more or less) "if_stmt(integer_literal(0))" rather
than "if_stmt(paren_expr(integer_literal(0)))" when the macro is invoked
with 0 as parameter _done. Now, syntactically the parentheses are in the
source code, so the letter of the rule is satisfied (as long as there is
a single condition in the if condition), but the presence of those
parentheses is lost when parsing. I see how this can be seen as a false
positive, and we will definitely add some special handling so that cases
like this are properly recognized, but for simplicity here I would add
some extra parentheses, at least until the false positive is not
resolved
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
On 2025-12-11 11:38, Nicola Vetrini wrote:
> On 2025-12-11 10:30, Jan Beulich wrote:
>> On 11.12.2025 10:15, Nicola Vetrini wrote:
>>> On 2025-12-11 09:36, Jan Beulich wrote:
>>>> On 10.12.2025 19:30, Andrew Cooper wrote:
>>>>> With the wider testing, some more violations have been spotted.
>>>>> This
>>>>> addresses violations of Rule 20.7 which requires macro parameters
>>>>> to
>>>>> be
>>>>> bracketed.
>>>>>
>>>>> No functional change.
>>>>>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> ---
>>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>>>> CC: consulting@bugseng.com <consulting@bugseng.com>
>>>>> CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>> ---
>>>>> xen/arch/x86/mm/shadow/multi.c | 2 +-
>>>>> xen/arch/x86/mm/shadow/private.h | 6 +++---
>>>>> xen/drivers/passthrough/vtd/dmar.h | 2 +-
>>>>> xen/include/xen/kexec.h | 4 ++--
>>>>> 4 files changed, 7 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/mm/shadow/multi.c
>>>>> b/xen/arch/x86/mm/shadow/multi.c
>>>>> index 03be61e225c0..36ee6554b4c4 100644
>>>>> --- a/xen/arch/x86/mm/shadow/multi.c
>>>>> +++ b/xen/arch/x86/mm/shadow/multi.c
>>>>> @@ -781,7 +781,7 @@ do {
>>>>> \
>>>>> (_sl1e) = _sp + _i;
>>>>> \
>>>>> if ( shadow_l1e_get_flags(*(_sl1e)) & _PAGE_PRESENT )
>>>>> \
>>>>> {_code}
>>>>> \
>>>>> - if ( _done ) break;
>>>>> \
>>>>> + if ( (_done) ) break;
>>>>> \
>>>>
>>>> I don't understand this: There are parentheses already from if()
>>>> itself.
>>>
>>> Yeah, syntactically there are, but those are parsed as part of the
>>> if,
>>> rather than its condition; the AST node contained within does not
>>> have
>>> parentheses around it.
>>
>> I fear I don't follow. Besides us not using parentheses elsewhere when
>> if() is used like this macros, the point of requiring parentheses is
>> (aiui)
>> to make precedence explicit. There already is no ambiguity here due to
>> the
>> syntactically require parentheses in if(). Why would a rule and/or the
>> tool require redundant ones?
>>
>
> this is parsed as (more or less) "if_stmt(integer_literal(0))" rather
> than "if_stmt(paren_expr(integer_literal(0)))" when the macro is
> invoked with 0 > as parameter _done. Now, syntactically the parentheses
> are in the source code, so the letter of the rule is satisfied (as long
> as there is a single
> condition in the if condition), but the presence of those parentheses
> is lost when parsing. I see how this can be seen as a false positive,
> and we will
> definitely add some special handling so that cases like this are
> properly recognized, but for simplicity here I would add some extra
> parentheses, at
> least until the false positive is not resolved
Actually giving this a closer look the tool is correct: the fully
expanded code is:
19562 }} if ( ({ (__done = done); }) ) break;
increment_ptr_to_guest_entry(((void*)0)); } unmap_domain_page(_sp); }
while
<~~>
so the "done" argument ends up being expanded without parentheses, hence
the report is correct and the extra parentheses are needed, but should
be put into
/* 32-bit l1, on PAE or 64-bit shadows: need to walk both pages of
shadow */
791 #if GUEST_PAGING_LEVELS == 2 && SHADOW_PAGING_LEVELS > 2
792 #define FOREACH_PRESENT_L1E(_sl1mfn, _sl1e, _gl1p, _done, _code)
\
793 do {
\
794 int __done = 0;
\
795 _FOREACH_PRESENT_L1E(_sl1mfn, _sl1e, _gl1p,
\
<~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
796 ({ (__done = _done); }), _code);
\
rather than at the level of the if, I think
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
On 11/12/2025 12:28 pm, Nicola Vetrini wrote:
> On 2025-12-11 11:38, Nicola Vetrini wrote:
>> On 2025-12-11 10:30, Jan Beulich wrote:
>>> On 11.12.2025 10:15, Nicola Vetrini wrote:
>>>> On 2025-12-11 09:36, Jan Beulich wrote:
>>>>> On 10.12.2025 19:30, Andrew Cooper wrote:
>>>>>> With the wider testing, some more violations have been spotted.
>>>>>> This
>>>>>> addresses violations of Rule 20.7 which requires macro parameters to
>>>>>> be
>>>>>> bracketed.
>>>>>>
>>>>>> No functional change.
>>>>>>
>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>> ---
>>>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>>>>> CC: consulting@bugseng.com <consulting@bugseng.com>
>>>>>> CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>>> ---
>>>>>> xen/arch/x86/mm/shadow/multi.c | 2 +-
>>>>>> xen/arch/x86/mm/shadow/private.h | 6 +++---
>>>>>> xen/drivers/passthrough/vtd/dmar.h | 2 +-
>>>>>> xen/include/xen/kexec.h | 4 ++--
>>>>>> 4 files changed, 7 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/xen/arch/x86/mm/shadow/multi.c
>>>>>> b/xen/arch/x86/mm/shadow/multi.c
>>>>>> index 03be61e225c0..36ee6554b4c4 100644
>>>>>> --- a/xen/arch/x86/mm/shadow/multi.c
>>>>>> +++ b/xen/arch/x86/mm/shadow/multi.c
>>>>>> @@ -781,7 +781,7 @@ do {
>>>>>> \
>>>>>> (_sl1e) = _sp + _i;
>>>>>> \
>>>>>> if ( shadow_l1e_get_flags(*(_sl1e)) & _PAGE_PRESENT )
>>>>>> \
>>>>>> {_code}
>>>>>> \
>>>>>> - if ( _done ) break;
>>>>>> \
>>>>>> + if ( (_done) ) break;
>>>>>> \
>>>>>
>>>>> I don't understand this: There are parentheses already from if()
>>>>> itself.
>>>>
>>>> Yeah, syntactically there are, but those are parsed as part of the if,
>>>> rather than its condition; the AST node contained within does not have
>>>> parentheses around it.
>>>
>>> I fear I don't follow. Besides us not using parentheses elsewhere when
>>> if() is used like this macros, the point of requiring parentheses is
>>> (aiui)
>>> to make precedence explicit. There already is no ambiguity here due
>>> to the
>>> syntactically require parentheses in if(). Why would a rule and/or the
>>> tool require redundant ones?
>>>
>>
>> this is parsed as (more or less) "if_stmt(integer_literal(0))" rather
>> than "if_stmt(paren_expr(integer_literal(0)))" when the macro is
>> invoked with 0 > as parameter _done. Now, syntactically the
>> parentheses are in the source code, so the letter of the rule is
>> satisfied (as long as there is a single
>> condition in the if condition), but the presence of those parentheses
>> is lost when parsing. I see how this can be seen as a false positive,
>> and we will
>> definitely add some special handling so that cases like this are
>> properly recognized, but for simplicity here I would add some extra
>> parentheses, at
>> least until the false positive is not resolved
>
> Actually giving this a closer look the tool is correct:
Ah, and this adjustment wont fix the violation either.
> the fully expanded code is:
>
> 19562 }} if ( ({ (__done = done); }) ) break;
> increment_ptr_to_guest_entry(((void*)0)); } unmap_domain_page(_sp); }
> while
> <~~>
>
> so the "done" argument ends up being expanded without parentheses,
> hence the report is correct and the extra parentheses are needed, but
> should be put into
>
> /* 32-bit l1, on PAE or 64-bit shadows: need to walk both pages of
> shadow */
> 791 #if GUEST_PAGING_LEVELS == 2 && SHADOW_PAGING_LEVELS > 2
> 792 #define FOREACH_PRESENT_L1E(_sl1mfn, _sl1e, _gl1p, _done,
> _code) \
> 793 do
> { \
> 794 int __done =
> 0; \
> 795 _FOREACH_PRESENT_L1E(_sl1mfn, _sl1e,
> _gl1p, \
>
> <~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 796 ({ (__done = _done); }),
> _code); \
>
> rather than at the level of the if, I think
>
I hate these constructs with a passion, and from Matrix there's a
separate violation which has no viable fix with the construct staying
like this.
I experimented turning them into syntactically correct foreach_$FOO ( )
loops. I gave up because it got unwieldy, but now it's the only way I
can see to fix the violation, so I guess I should try again.
I'll drop this one hunk from the patch and commit the rest of the fixes.
~Andrew
On 2025-12-10 19:30, Andrew Cooper wrote:
> With the wider testing, some more violations have been spotted. This
> addresses violations of Rule 20.7 which requires macro parameters to be
> bracketed.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: consulting@bugseng.com <consulting@bugseng.com>
> CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> xen/arch/x86/mm/shadow/multi.c | 2 +-
> xen/arch/x86/mm/shadow/private.h | 6 +++---
> xen/drivers/passthrough/vtd/dmar.h | 2 +-
> xen/include/xen/kexec.h | 4 ++--
> 4 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/x86/mm/shadow/multi.c
> b/xen/arch/x86/mm/shadow/multi.c
> index 03be61e225c0..36ee6554b4c4 100644
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -781,7 +781,7 @@ do {
> \
> (_sl1e) = _sp + _i;
> \
> if ( shadow_l1e_get_flags(*(_sl1e)) & _PAGE_PRESENT )
> \
> {_code}
> \
> - if ( _done ) break;
> \
> + if ( (_done) ) break;
> \
> increment_ptr_to_guest_entry(_gl1p);
> \
> }
> \
> unmap_domain_page(_sp);
> \
> diff --git a/xen/arch/x86/mm/shadow/private.h
> b/xen/arch/x86/mm/shadow/private.h
> index cef9dbef2e77..93834ec55c42 100644
> --- a/xen/arch/x86/mm/shadow/private.h
> +++ b/xen/arch/x86/mm/shadow/private.h
> @@ -636,9 +636,9 @@ prev_pinned_shadow(struct page_info *page,
> }
>
> #define foreach_pinned_shadow(dom, pos, tmp) \
> - for ( pos = prev_pinned_shadow(NULL, (dom)); \
> - pos ? (tmp = prev_pinned_shadow(pos, (dom)), 1) : 0; \
> - pos = tmp )
> + for ( (pos) = prev_pinned_shadow(NULL, dom); \
> + (pos) ? (tmp = prev_pinned_shadow(pos, dom), 1) : 0; \
> + (pos) = tmp )
>
> /*
> * Pin a shadow page: take an extra refcount, set the pin bit,
> diff --git a/xen/drivers/passthrough/vtd/dmar.h
> b/xen/drivers/passthrough/vtd/dmar.h
> index 0ff4f365351f..11590f71a828 100644
> --- a/xen/drivers/passthrough/vtd/dmar.h
> +++ b/xen/drivers/passthrough/vtd/dmar.h
> @@ -124,7 +124,7 @@ struct acpi_atsr_unit
> *acpi_find_matched_atsr_unit(const struct pci_dev *);
> do { \
> s_time_t start_time = NOW(); \
> while (1) { \
> - sts = op(iommu->reg, offset); \
> + sts = op((iommu)->reg, offset); \
> if ( cond ) \
> break; \
> if ( NOW() > start_time + DMAR_OPERATION_TIMEOUT ) { \
> diff --git a/xen/include/xen/kexec.h b/xen/include/xen/kexec.h
> index e66eb6a8e593..5dd288d1a50e 100644
> --- a/xen/include/xen/kexec.h
> +++ b/xen/include/xen/kexec.h
> @@ -66,9 +66,9 @@ void vmcoreinfo_append_str(const char *fmt, ...)
> #define VMCOREINFO_PAGESIZE(value) \
> vmcoreinfo_append_str("PAGESIZE=%ld\n", value)
> #define VMCOREINFO_SYMBOL(name) \
> - vmcoreinfo_append_str("SYMBOL(%s)=%lx\n", #name, (unsigned
> long)&name)
> + vmcoreinfo_append_str("SYMBOL(%s)=%lx\n", #name, (unsigned
> long)&(name))
> #define VMCOREINFO_SYMBOL_ALIAS(alias, name) \
> - vmcoreinfo_append_str("SYMBOL(%s)=%lx\n", #alias, (unsigned
> long)&name)
> + vmcoreinfo_append_str("SYMBOL(%s)=%lx\n", #alias, (unsigned
> long)&(name))
> #define VMCOREINFO_STRUCT_SIZE(name) \
> vmcoreinfo_append_str("SIZE(%s)=%zu\n", #name, sizeof(struct
> name))
> #define VMCOREINFO_OFFSET(name, field) \
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
On Wed, 10 Dec 2025, Nicola Vetrini wrote: > On 2025-12-10 19:30, Andrew Cooper wrote: > > With the wider testing, some more violations have been spotted. This > > addresses violations of Rule 20.7 which requires macro parameters to be > > bracketed. > > > > No functional change. > > > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Nicola Vetrini <nicola.vetrini@bugseng.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
© 2016 - 2025 Red Hat, Inc.