* [PATCH 1/5] x86: Misra fixes for U/L suffixes
2025-12-10 18:30 [PATCH 0/5] x86: Misc MISRA fixes Andrew Cooper
@ 2025-12-10 18:30 ` Andrew Cooper
2025-12-10 20:09 ` Nicola Vetrini
2025-12-10 18:30 ` [PATCH 2/5] x86: Name parameters in function declarations Andrew Cooper
` (3 subsequent siblings)
4 siblings, 1 reply; 23+ messages in thread
From: Andrew Cooper @ 2025-12-10 18:30 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné,
Stefano Stabellini, consulting @ bugseng . com, Nicola Vetrini
With the wider testing, some more violations have been spotted. This
addresses violations of Rule 7.2 (suffixes required) and Rule 7.3 (L must be
uppercase).
For ELF64_R_TYPE(), cast to uint32_t matching the surrounding examples.
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/common.c | 4 ++--
xen/arch/x86/pv/descriptor-tables.c | 2 +-
xen/drivers/passthrough/vtd/iommu.h | 2 +-
xen/include/xen/elfstructs.h | 2 +-
xen/include/xen/sizes.h | 2 +-
5 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 0176e33bc9c7..423764a32653 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -1961,7 +1961,7 @@ int sh_remove_write_access(struct domain *d, mfn_t gmfn,
/* FreeBSD 64bit: linear map 0xffff800000000000 */
switch ( level )
{
- case 1: GUESS(0xffff800000000000
+ case 1: GUESS(0xffff800000000000UL
+ ((fault_addr & VADDR_MASK) >> 9), 6); break;
case 2: GUESS(0xffff804000000000UL
+ ((fault_addr & VADDR_MASK) >> 18), 6); break;
@@ -1969,7 +1969,7 @@ int sh_remove_write_access(struct domain *d, mfn_t gmfn,
+ ((fault_addr & VADDR_MASK) >> 27), 6); break;
}
/* FreeBSD 64bit: direct map at 0xffffff0000000000 */
- GUESS(0xffffff0000000000 + gfn_to_gaddr(gfn), 6);
+ GUESS(0xffffff0000000000UL + gfn_to_gaddr(gfn), 6);
}
#undef GUESS
diff --git a/xen/arch/x86/pv/descriptor-tables.c b/xen/arch/x86/pv/descriptor-tables.c
index 02647a2c5047..26f7d18b11b5 100644
--- a/xen/arch/x86/pv/descriptor-tables.c
+++ b/xen/arch/x86/pv/descriptor-tables.c
@@ -216,7 +216,7 @@ static bool check_descriptor(const struct domain *dom, seg_desc_t *d)
* 0xf6800000. Extend these to allow access to the larger read-only
* M2P table available in 32on64 mode.
*/
- base = (b & 0xff000000) | ((b & 0xff) << 16) | (a >> 16);
+ base = (b & 0xff000000U) | ((b & 0xff) << 16) | (a >> 16);
limit = (b & 0xf0000) | (a & 0xffff);
limit++; /* We add one because limit is inclusive. */
diff --git a/xen/drivers/passthrough/vtd/iommu.h b/xen/drivers/passthrough/vtd/iommu.h
index 29d350b23db6..4f41360c53c0 100644
--- a/xen/drivers/passthrough/vtd/iommu.h
+++ b/xen/drivers/passthrough/vtd/iommu.h
@@ -266,7 +266,7 @@ struct dma_pte {
#define DMA_PTE_PROT (DMA_PTE_READ | DMA_PTE_WRITE)
#define DMA_PTE_SP (1 << 7)
#define DMA_PTE_SNP (1 << 11)
-#define DMA_PTE_CONTIG_MASK (0xfull << PADDR_BITS)
+#define DMA_PTE_CONTIG_MASK (0xfULL << PADDR_BITS)
#define dma_clear_pte(p) do {(p).val = 0;} while(0)
#define dma_set_pte_readable(p) do {(p).val |= DMA_PTE_READ;} while(0)
#define dma_set_pte_writable(p) do {(p).val |= DMA_PTE_WRITE;} while(0)
diff --git a/xen/include/xen/elfstructs.h b/xen/include/xen/elfstructs.h
index eb6b87a823a8..8770e7454672 100644
--- a/xen/include/xen/elfstructs.h
+++ b/xen/include/xen/elfstructs.h
@@ -360,7 +360,7 @@ typedef struct {
} Elf64_Rela;
#define ELF64_R_SYM(info) ((info) >> 32)
-#define ELF64_R_TYPE(info) ((info) & 0xFFFFFFFF)
+#define ELF64_R_TYPE(info) ((uint32_t)(info))
#define ELF64_R_INFO(s,t) (((s) << 32) + (uint32_t)(t))
/*
diff --git a/xen/include/xen/sizes.h b/xen/include/xen/sizes.h
index f7b728ddab06..d309ebf04406 100644
--- a/xen/include/xen/sizes.h
+++ b/xen/include/xen/sizes.h
@@ -43,6 +43,6 @@
#define SZ_512M 0x20000000
#define SZ_1G 0x40000000
-#define SZ_2G 0x80000000
+#define SZ_2G 0x80000000U
#endif /* __XEN_SIZES_H__ */
--
2.39.5
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 1/5] x86: Misra fixes for U/L suffixes
2025-12-10 18:30 ` [PATCH 1/5] x86: Misra fixes for U/L suffixes Andrew Cooper
@ 2025-12-10 20:09 ` Nicola Vetrini
2025-12-10 20:31 ` Nicola Vetrini
0 siblings, 1 reply; 23+ messages in thread
From: Nicola Vetrini @ 2025-12-10 20:09 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Jan Beulich, Roger Pau Monné, Stefano Stabellini,
consulting @ bugseng . com
On 2025-12-10 19:30, Andrew Cooper wrote:
> With the wider testing, some more violations have been spotted. This
> addresses violations of Rule 7.2 (suffixes required) and Rule 7.3 (L
> must be
> uppercase).
>
> For ELF64_R_TYPE(), cast to uint32_t matching the surrounding examples.
>
> 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/common.c | 4 ++--
> xen/arch/x86/pv/descriptor-tables.c | 2 +-
> xen/drivers/passthrough/vtd/iommu.h | 2 +-
> xen/include/xen/elfstructs.h | 2 +-
> xen/include/xen/sizes.h | 2 +-
> 5 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/x86/mm/shadow/common.c
> b/xen/arch/x86/mm/shadow/common.c
> index 0176e33bc9c7..423764a32653 100644
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -1961,7 +1961,7 @@ int sh_remove_write_access(struct domain *d,
> mfn_t gmfn,
> /* FreeBSD 64bit: linear map 0xffff800000000000 */
> switch ( level )
> {
> - case 1: GUESS(0xffff800000000000
> + case 1: GUESS(0xffff800000000000UL
> + ((fault_addr & VADDR_MASK) >> 9), 6);
> break;
> case 2: GUESS(0xffff804000000000UL
> + ((fault_addr & VADDR_MASK) >> 18), 6);
> break;
> @@ -1969,7 +1969,7 @@ int sh_remove_write_access(struct domain *d,
> mfn_t gmfn,
> + ((fault_addr & VADDR_MASK) >> 27), 6);
> break;
> }
> /* FreeBSD 64bit: direct map at 0xffffff0000000000 */
> - GUESS(0xffffff0000000000 + gfn_to_gaddr(gfn), 6);
> + GUESS(0xffffff0000000000UL + gfn_to_gaddr(gfn), 6);
> }
>
> #undef GUESS
> diff --git a/xen/arch/x86/pv/descriptor-tables.c
> b/xen/arch/x86/pv/descriptor-tables.c
> index 02647a2c5047..26f7d18b11b5 100644
> --- a/xen/arch/x86/pv/descriptor-tables.c
> +++ b/xen/arch/x86/pv/descriptor-tables.c
> @@ -216,7 +216,7 @@ static bool check_descriptor(const struct domain
> *dom, seg_desc_t *d)
> * 0xf6800000. Extend these to allow access to the larger
> read-only
> * M2P table available in 32on64 mode.
> */
> - base = (b & 0xff000000) | ((b & 0xff) << 16) | (a >> 16);
> + base = (b & 0xff000000U) | ((b & 0xff) << 16) | (a >> 16);
>
> limit = (b & 0xf0000) | (a & 0xffff);
> limit++; /* We add one because limit is inclusive. */
> diff --git a/xen/drivers/passthrough/vtd/iommu.h
> b/xen/drivers/passthrough/vtd/iommu.h
> index 29d350b23db6..4f41360c53c0 100644
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -266,7 +266,7 @@ struct dma_pte {
> #define DMA_PTE_PROT (DMA_PTE_READ | DMA_PTE_WRITE)
> #define DMA_PTE_SP (1 << 7)
> #define DMA_PTE_SNP (1 << 11)
> -#define DMA_PTE_CONTIG_MASK (0xfull << PADDR_BITS)
> +#define DMA_PTE_CONTIG_MASK (0xfULL << PADDR_BITS)
> #define dma_clear_pte(p) do {(p).val = 0;} while(0)
> #define dma_set_pte_readable(p) do {(p).val |= DMA_PTE_READ;} while(0)
> #define dma_set_pte_writable(p) do {(p).val |= DMA_PTE_WRITE;}
> while(0)
> diff --git a/xen/include/xen/elfstructs.h
> b/xen/include/xen/elfstructs.h
> index eb6b87a823a8..8770e7454672 100644
> --- a/xen/include/xen/elfstructs.h
> +++ b/xen/include/xen/elfstructs.h
> @@ -360,7 +360,7 @@ typedef struct {
> } Elf64_Rela;
>
> #define ELF64_R_SYM(info) ((info) >> 32)
> -#define ELF64_R_TYPE(info) ((info) & 0xFFFFFFFF)
> +#define ELF64_R_TYPE(info) ((uint32_t)(info))
> #define ELF64_R_INFO(s,t) (((s) << 32) + (uint32_t)(t))
>
> /*
> diff --git a/xen/include/xen/sizes.h b/xen/include/xen/sizes.h
> index f7b728ddab06..d309ebf04406 100644
> --- a/xen/include/xen/sizes.h
> +++ b/xen/include/xen/sizes.h
> @@ -43,6 +43,6 @@
> #define SZ_512M 0x20000000
>
> #define SZ_1G 0x40000000
> -#define SZ_2G 0x80000000
> +#define SZ_2G 0x80000000U
>
> #endif /* __XEN_SIZES_H__ */
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 1/5] x86: Misra fixes for U/L suffixes
2025-12-10 20:09 ` Nicola Vetrini
@ 2025-12-10 20:31 ` Nicola Vetrini
2025-12-10 23:48 ` Andrew Cooper
0 siblings, 1 reply; 23+ messages in thread
From: Nicola Vetrini @ 2025-12-10 20:31 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Jan Beulich, Roger Pau Monné, Stefano Stabellini,
consulting @ bugseng . com
On 2025-12-10 21:09, 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 7.2 (suffixes required) and Rule 7.3 (L
>> must be
>> uppercase).
>>
>> For ELF64_R_TYPE(), cast to uint32_t matching the surrounding
>> examples.
>>
>> 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/common.c | 4 ++--
>> xen/arch/x86/pv/descriptor-tables.c | 2 +-
>> xen/drivers/passthrough/vtd/iommu.h | 2 +-
>> xen/include/xen/elfstructs.h | 2 +-
>> xen/include/xen/sizes.h | 2 +-
>> 5 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/shadow/common.c
>> b/xen/arch/x86/mm/shadow/common.c
>> index 0176e33bc9c7..423764a32653 100644
>> --- a/xen/arch/x86/mm/shadow/common.c
>> +++ b/xen/arch/x86/mm/shadow/common.c
>> @@ -1961,7 +1961,7 @@ int sh_remove_write_access(struct domain *d,
>> mfn_t gmfn,
>> /* FreeBSD 64bit: linear map 0xffff800000000000 */
>> switch ( level )
>> {
>> - case 1: GUESS(0xffff800000000000
>> + case 1: GUESS(0xffff800000000000UL
>> + ((fault_addr & VADDR_MASK) >> 9), 6);
>> break;
>> case 2: GUESS(0xffff804000000000UL
>> + ((fault_addr & VADDR_MASK) >> 18), 6);
>> break;
>> @@ -1969,7 +1969,7 @@ int sh_remove_write_access(struct domain *d,
>> mfn_t gmfn,
>> + ((fault_addr & VADDR_MASK) >> 27), 6);
>> break;
>> }
>> /* FreeBSD 64bit: direct map at 0xffffff0000000000 */
>> - GUESS(0xffffff0000000000 + gfn_to_gaddr(gfn), 6);
>> + GUESS(0xffffff0000000000UL + gfn_to_gaddr(gfn), 6);
>> }
>>
>> #undef GUESS
>> diff --git a/xen/arch/x86/pv/descriptor-tables.c
>> b/xen/arch/x86/pv/descriptor-tables.c
>> index 02647a2c5047..26f7d18b11b5 100644
>> --- a/xen/arch/x86/pv/descriptor-tables.c
>> +++ b/xen/arch/x86/pv/descriptor-tables.c
>> @@ -216,7 +216,7 @@ static bool check_descriptor(const struct domain
>> *dom, seg_desc_t *d)
>> * 0xf6800000. Extend these to allow access to the larger
>> read-only
>> * M2P table available in 32on64 mode.
>> */
>> - base = (b & 0xff000000) | ((b & 0xff) << 16) | (a >> 16);
>> + base = (b & 0xff000000U) | ((b & 0xff) << 16) | (a >>
>> 16);
>>
>> limit = (b & 0xf0000) | (a & 0xffff);
>> limit++; /* We add one because limit is inclusive. */
>> diff --git a/xen/drivers/passthrough/vtd/iommu.h
>> b/xen/drivers/passthrough/vtd/iommu.h
>> index 29d350b23db6..4f41360c53c0 100644
>> --- a/xen/drivers/passthrough/vtd/iommu.h
>> +++ b/xen/drivers/passthrough/vtd/iommu.h
>> @@ -266,7 +266,7 @@ struct dma_pte {
>> #define DMA_PTE_PROT (DMA_PTE_READ | DMA_PTE_WRITE)
>> #define DMA_PTE_SP (1 << 7)
>> #define DMA_PTE_SNP (1 << 11)
>> -#define DMA_PTE_CONTIG_MASK (0xfull << PADDR_BITS)
>> +#define DMA_PTE_CONTIG_MASK (0xfULL << PADDR_BITS)
>> #define dma_clear_pte(p) do {(p).val = 0;} while(0)
>> #define dma_set_pte_readable(p) do {(p).val |= DMA_PTE_READ;}
>> while(0)
>> #define dma_set_pte_writable(p) do {(p).val |= DMA_PTE_WRITE;}
>> while(0)
>> diff --git a/xen/include/xen/elfstructs.h
>> b/xen/include/xen/elfstructs.h
>> index eb6b87a823a8..8770e7454672 100644
>> --- a/xen/include/xen/elfstructs.h
>> +++ b/xen/include/xen/elfstructs.h
>> @@ -360,7 +360,7 @@ typedef struct {
>> } Elf64_Rela;
>>
>> #define ELF64_R_SYM(info) ((info) >> 32)
>> -#define ELF64_R_TYPE(info) ((info) & 0xFFFFFFFF)
>> +#define ELF64_R_TYPE(info) ((uint32_t)(info))
Actually I think this doesn't build:
arch/x86/livepatch.c: In function ‘arch_livepatch_perform_rela’:
././include/xen/config.h:55:24: error: format ‘%lu’ expects argument of
type ‘long unsigned int’, but argument 3 has type ‘unsigned int’
[-Werror=format=]
55 | #define XENLOG_ERR "<0>"
| ^~~~~
arch/x86/livepatch.c:332:20: note: in expansion of macro ‘XENLOG_ERR’
332 | printk(XENLOG_ERR LIVEPATCH "%s: Unhandled
relocation %lu\n",
| ^~~~~~~~~~
arch/x86/livepatch.c:332:69: note: format string is defined here
332 | printk(XENLOG_ERR LIVEPATCH "%s: Unhandled
relocation %lu\n",
|
~~^
|
|
|
long unsigned int
|
%u
the error location is a bit unclear, but the cast is the culprit
>> #define ELF64_R_INFO(s,t) (((s) << 32) + (uint32_t)(t))
>>
>> /*
>> diff --git a/xen/include/xen/sizes.h b/xen/include/xen/sizes.h
>> index f7b728ddab06..d309ebf04406 100644
>> --- a/xen/include/xen/sizes.h
>> +++ b/xen/include/xen/sizes.h
>> @@ -43,6 +43,6 @@
>> #define SZ_512M 0x20000000
>>
>> #define SZ_1G 0x40000000
>> -#define SZ_2G 0x80000000
>> +#define SZ_2G 0x80000000U
>>
>> #endif /* __XEN_SIZES_H__ */
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 1/5] x86: Misra fixes for U/L suffixes
2025-12-10 20:31 ` Nicola Vetrini
@ 2025-12-10 23:48 ` Andrew Cooper
2025-12-11 7:39 ` Nicola Vetrini
0 siblings, 1 reply; 23+ messages in thread
From: Andrew Cooper @ 2025-12-10 23:48 UTC (permalink / raw)
To: Nicola Vetrini
Cc: Andrew Cooper, Xen-devel, Jan Beulich, Roger Pau Monné,
Stefano Stabellini, consulting @ bugseng . com
On 10/12/2025 8:31 pm, Nicola Vetrini wrote:
> On 2025-12-10 21:09, Nicola Vetrini wrote:
>> On 2025-12-10 19:30, Andrew Cooper wrote:
>>> diff --git a/xen/include/xen/elfstructs.h
>>> b/xen/include/xen/elfstructs.h
>>> index eb6b87a823a8..8770e7454672 100644
>>> --- a/xen/include/xen/elfstructs.h
>>> +++ b/xen/include/xen/elfstructs.h
>>> @@ -360,7 +360,7 @@ typedef struct {
>>> } Elf64_Rela;
>>>
>>> #define ELF64_R_SYM(info) ((info) >> 32)
>>> -#define ELF64_R_TYPE(info) ((info) & 0xFFFFFFFF)
>>> +#define ELF64_R_TYPE(info) ((uint32_t)(info))
>
> Actually I think this doesn't build:
>
> arch/x86/livepatch.c: In function ‘arch_livepatch_perform_rela’:
> ././include/xen/config.h:55:24: error: format ‘%lu’ expects argument
> of type ‘long unsigned int’, but argument 3 has type ‘unsigned int’
> [-Werror=format=]
> 55 | #define XENLOG_ERR "<0>"
> | ^~~~~
> arch/x86/livepatch.c:332:20: note: in expansion of macro ‘XENLOG_ERR’
> 332 | printk(XENLOG_ERR LIVEPATCH "%s: Unhandled
> relocation %lu\n",
> | ^~~~~~~~~~
> arch/x86/livepatch.c:332:69: note: format string is defined here
> 332 | printk(XENLOG_ERR LIVEPATCH "%s: Unhandled
> relocation %lu\n",
>
> | ~~^
>
> | |
>
> |
> long unsigned int
>
> | %u
>
> the error location is a bit unclear, but the cast is the culprit
Yeah, I spotted that just as I heading out, and ran
https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/2207521982
instead.
I've swapped back to using 0xFFFFFFFFU. info ends up being long, and
the result of the expression needs to stay that way.
However, looking at the report for that, I still missed one. I've
folded in this hunk too.
diff --git a/xen/arch/x86/pv/emulate.c b/xen/arch/x86/pv/emulate.c
index 8c44dea12330..e741e686c1af 100644
--- a/xen/arch/x86/pv/emulate.c
+++ b/xen/arch/x86/pv/emulate.c
@@ -37,7 +37,7 @@ int pv_emul_read_descriptor(unsigned int sel, const struct vcpu *v,
if ( !(desc.b & _SEGMENT_L) )
{
*base = ((desc.a >> 16) + ((desc.b & 0xff) << 16) +
- (desc.b & 0xff000000));
+ (desc.b & 0xff000000U));
*limit = (desc.a & 0xffff) | (desc.b & 0x000f0000);
if ( desc.b & _SEGMENT_G )
*limit = ((*limit + 1) << 12) - 1;
~Andrew
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 1/5] x86: Misra fixes for U/L suffixes
2025-12-10 23:48 ` Andrew Cooper
@ 2025-12-11 7:39 ` Nicola Vetrini
0 siblings, 0 replies; 23+ messages in thread
From: Nicola Vetrini @ 2025-12-11 7:39 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Jan Beulich, Roger Pau Monné, Stefano Stabellini,
consulting @ bugseng . com
On 2025-12-11 00:48, Andrew Cooper wrote:
> On 10/12/2025 8:31 pm, Nicola Vetrini wrote:
>> On 2025-12-10 21:09, Nicola Vetrini wrote:
>>> On 2025-12-10 19:30, Andrew Cooper wrote:
>>>> diff --git a/xen/include/xen/elfstructs.h
>>>> b/xen/include/xen/elfstructs.h
>>>> index eb6b87a823a8..8770e7454672 100644
>>>> --- a/xen/include/xen/elfstructs.h
>>>> +++ b/xen/include/xen/elfstructs.h
>>>> @@ -360,7 +360,7 @@ typedef struct {
>>>> } Elf64_Rela;
>>>>
>>>> #define ELF64_R_SYM(info) ((info) >> 32)
>>>> -#define ELF64_R_TYPE(info) ((info) & 0xFFFFFFFF)
>>>> +#define ELF64_R_TYPE(info) ((uint32_t)(info))
>>
>> Actually I think this doesn't build:
>>
>> arch/x86/livepatch.c: In function ‘arch_livepatch_perform_rela’:
>> ././include/xen/config.h:55:24: error: format ‘%lu’ expects argument
>> of type ‘long unsigned int’, but argument 3 has type ‘unsigned int’
>> [-Werror=format=]
>> 55 | #define XENLOG_ERR "<0>"
>> | ^~~~~
>> arch/x86/livepatch.c:332:20: note: in expansion of macro ‘XENLOG_ERR’
>> 332 | printk(XENLOG_ERR LIVEPATCH "%s: Unhandled
>> relocation %lu\n",
>> | ^~~~~~~~~~
>> arch/x86/livepatch.c:332:69: note: format string is defined here
>> 332 | printk(XENLOG_ERR LIVEPATCH "%s: Unhandled
>> relocation %lu\n",
>>
>> |
>> ~~^
>>
>> |
>> |
>>
>> |
>> long unsigned int
>>
>> | %u
>>
>> the error location is a bit unclear, but the cast is the culprit
>
> Yeah, I spotted that just as I heading out, and ran
> https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/2207521982
> instead.
>
> I've swapped back to using 0xFFFFFFFFU. info ends up being long, and
> the result of the expression needs to stay that way.
>
> However, looking at the report for that, I still missed one. I've
> folded in this hunk too.
>
> diff --git a/xen/arch/x86/pv/emulate.c b/xen/arch/x86/pv/emulate.c
> index 8c44dea12330..e741e686c1af 100644
> --- a/xen/arch/x86/pv/emulate.c
> +++ b/xen/arch/x86/pv/emulate.c
> @@ -37,7 +37,7 @@ int pv_emul_read_descriptor(unsigned int sel, const
> struct vcpu *v,
> if ( !(desc.b & _SEGMENT_L) )
> {
> *base = ((desc.a >> 16) + ((desc.b & 0xff) << 16) +
> - (desc.b & 0xff000000));
> + (desc.b & 0xff000000U));
> *limit = (desc.a & 0xffff) | (desc.b & 0x000f0000);
> if ( desc.b & _SEGMENT_G )
> *limit = ((*limit + 1) << 12) - 1;
>
Makes sense, feel free to retain my R-by with these two changes.
>
> ~Andrew
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/5] x86: Name parameters in function declarations
2025-12-10 18:30 [PATCH 0/5] x86: Misc MISRA fixes Andrew Cooper
2025-12-10 18:30 ` [PATCH 1/5] x86: Misra fixes for U/L suffixes Andrew Cooper
@ 2025-12-10 18:30 ` Andrew Cooper
2025-12-10 20:15 ` Nicola Vetrini
2025-12-10 18:30 ` [PATCH 3/5] x86/ucode: Don't cast away const-ness in cmp_patch_id() Andrew Cooper
` (2 subsequent siblings)
4 siblings, 1 reply; 23+ messages in thread
From: Andrew Cooper @ 2025-12-10 18:30 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné,
Stefano Stabellini, consulting @ bugseng . com, Nicola Vetrini
With the wider testing, some more violations have been spotted. This
addresses violations of Rule 8.2 (parameters must be named).
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/common.c | 8 ++++----
xen/arch/x86/pv/emul-priv-op.c | 2 +-
xen/include/xen/livepatch.h | 2 +-
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 423764a32653..f2aee5be46a7 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -69,11 +69,11 @@ const uint8_t sh_type_to_size[] = {
DEFINE_PER_CPU(uint32_t,trace_shadow_path_flags);
-static int cf_check sh_enable_log_dirty(struct domain *);
-static int cf_check sh_disable_log_dirty(struct domain *);
-static void cf_check sh_clean_dirty_bitmap(struct domain *);
+static int cf_check sh_enable_log_dirty(struct domain *d);
+static int cf_check sh_disable_log_dirty(struct domain *d);
+static void cf_check sh_clean_dirty_bitmap(struct domain *d);
-static void cf_check shadow_update_paging_modes(struct vcpu *);
+static void cf_check shadow_update_paging_modes(struct vcpu *v);
/* Set up the shadow-specific parts of a domain struct at start of day.
* Called for every domain from arch_domain_create() */
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 225d4cff03c1..08dec9990e39 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -40,7 +40,7 @@ struct priv_op_ctxt {
};
/* I/O emulation helpers. Use non-standard calling conventions. */
-void nocall load_guest_gprs(struct cpu_user_regs *);
+void nocall load_guest_gprs(struct cpu_user_regs *regs);
void nocall save_guest_gprs(void);
typedef void io_emul_stub_t(struct cpu_user_regs *);
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index d074a5bebecc..3f5ad01f1bdd 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -62,7 +62,7 @@ struct livepatch_fstate {
uint8_t insn_buffer[LIVEPATCH_OPAQUE_SIZE];
};
-int livepatch_op(struct xen_sysctl_livepatch_op *);
+int livepatch_op(struct xen_sysctl_livepatch_op *op);
void check_for_livepatch_work(void);
unsigned long livepatch_symbols_lookup_by_name(const char *symname);
bool is_patch(const void *addr);
--
2.39.5
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 2/5] x86: Name parameters in function declarations
2025-12-10 18:30 ` [PATCH 2/5] x86: Name parameters in function declarations Andrew Cooper
@ 2025-12-10 20:15 ` Nicola Vetrini
2025-12-12 1:39 ` Andrew Cooper
0 siblings, 1 reply; 23+ messages in thread
From: Nicola Vetrini @ 2025-12-10 20:15 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Jan Beulich, Roger Pau Monné, Stefano Stabellini,
consulting @ bugseng . com
On 2025-12-10 19:30, Andrew Cooper wrote:
> With the wider testing, some more violations have been spotted. This
> addresses violations of Rule 8.2 (parameters must be named).
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
One nit below
> ---
> 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/common.c | 8 ++++----
> xen/arch/x86/pv/emul-priv-op.c | 2 +-
> xen/include/xen/livepatch.h | 2 +-
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/x86/mm/shadow/common.c
> b/xen/arch/x86/mm/shadow/common.c
> index 423764a32653..f2aee5be46a7 100644
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -69,11 +69,11 @@ const uint8_t sh_type_to_size[] = {
>
> DEFINE_PER_CPU(uint32_t,trace_shadow_path_flags);
>
> -static int cf_check sh_enable_log_dirty(struct domain *);
> -static int cf_check sh_disable_log_dirty(struct domain *);
> -static void cf_check sh_clean_dirty_bitmap(struct domain *);
> +static int cf_check sh_enable_log_dirty(struct domain *d);
> +static int cf_check sh_disable_log_dirty(struct domain *d);
> +static void cf_check sh_clean_dirty_bitmap(struct domain *d);
>
> -static void cf_check shadow_update_paging_modes(struct vcpu *);
> +static void cf_check shadow_update_paging_modes(struct vcpu *v);
>
> /* Set up the shadow-specific parts of a domain struct at start of
> day.
> * Called for every domain from arch_domain_create() */
> diff --git a/xen/arch/x86/pv/emul-priv-op.c
> b/xen/arch/x86/pv/emul-priv-op.c
> index 225d4cff03c1..08dec9990e39 100644
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -40,7 +40,7 @@ struct priv_op_ctxt {
> };
>
> /* I/O emulation helpers. Use non-standard calling conventions. */
> -void nocall load_guest_gprs(struct cpu_user_regs *);
> +void nocall load_guest_gprs(struct cpu_user_regs *regs);
> void nocall save_guest_gprs(void);
>
> typedef void io_emul_stub_t(struct cpu_user_regs *);
> diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
> index d074a5bebecc..3f5ad01f1bdd 100644
> --- a/xen/include/xen/livepatch.h
> +++ b/xen/include/xen/livepatch.h
> @@ -62,7 +62,7 @@ struct livepatch_fstate {
> uint8_t insn_buffer[LIVEPATCH_OPAQUE_SIZE];
> };
>
> -int livepatch_op(struct xen_sysctl_livepatch_op *);
> +int livepatch_op(struct xen_sysctl_livepatch_op *op);
xen/common/livepatch.c:int livepatch_op(struct xen_sysctl_livepatch_op
*livepatch)
Shouldn't this decl also use "*op" as well? Might not be triggered in
this configuration due to the absence of CONFIG_LIVEPATCH I think.
> void check_for_livepatch_work(void);
> unsigned long livepatch_symbols_lookup_by_name(const char *symname);
> bool is_patch(const void *addr);
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 2/5] x86: Name parameters in function declarations
2025-12-10 20:15 ` Nicola Vetrini
@ 2025-12-12 1:39 ` Andrew Cooper
0 siblings, 0 replies; 23+ messages in thread
From: Andrew Cooper @ 2025-12-12 1:39 UTC (permalink / raw)
To: Nicola Vetrini
Cc: Andrew Cooper, Xen-devel, Jan Beulich, Roger Pau Monné,
Stefano Stabellini, consulting @ bugseng . com
On 10/12/2025 8:15 pm, 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 8.2 (parameters must be named).
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Thanks.
>
> One nit below
>
>> ---
>> 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/common.c | 8 ++++----
>> xen/arch/x86/pv/emul-priv-op.c | 2 +-
>> xen/include/xen/livepatch.h | 2 +-
>> 3 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/shadow/common.c
>> b/xen/arch/x86/mm/shadow/common.c
>> index 423764a32653..f2aee5be46a7 100644
>> --- a/xen/arch/x86/mm/shadow/common.c
>> +++ b/xen/arch/x86/mm/shadow/common.c
>> @@ -69,11 +69,11 @@ const uint8_t sh_type_to_size[] = {
>>
>> DEFINE_PER_CPU(uint32_t,trace_shadow_path_flags);
>>
>> -static int cf_check sh_enable_log_dirty(struct domain *);
>> -static int cf_check sh_disable_log_dirty(struct domain *);
>> -static void cf_check sh_clean_dirty_bitmap(struct domain *);
>> +static int cf_check sh_enable_log_dirty(struct domain *d);
>> +static int cf_check sh_disable_log_dirty(struct domain *d);
>> +static void cf_check sh_clean_dirty_bitmap(struct domain *d);
>>
>> -static void cf_check shadow_update_paging_modes(struct vcpu *);
>> +static void cf_check shadow_update_paging_modes(struct vcpu *v);
>>
>> /* Set up the shadow-specific parts of a domain struct at start of day.
>> * Called for every domain from arch_domain_create() */
>> diff --git a/xen/arch/x86/pv/emul-priv-op.c
>> b/xen/arch/x86/pv/emul-priv-op.c
>> index 225d4cff03c1..08dec9990e39 100644
>> --- a/xen/arch/x86/pv/emul-priv-op.c
>> +++ b/xen/arch/x86/pv/emul-priv-op.c
>> @@ -40,7 +40,7 @@ struct priv_op_ctxt {
>> };
>>
>> /* I/O emulation helpers. Use non-standard calling conventions. */
>> -void nocall load_guest_gprs(struct cpu_user_regs *);
>> +void nocall load_guest_gprs(struct cpu_user_regs *regs);
>> void nocall save_guest_gprs(void);
>>
>> typedef void io_emul_stub_t(struct cpu_user_regs *);
>> diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
>> index d074a5bebecc..3f5ad01f1bdd 100644
>> --- a/xen/include/xen/livepatch.h
>> +++ b/xen/include/xen/livepatch.h
>> @@ -62,7 +62,7 @@ struct livepatch_fstate {
>> uint8_t insn_buffer[LIVEPATCH_OPAQUE_SIZE];
>> };
>>
>> -int livepatch_op(struct xen_sysctl_livepatch_op *);
>> +int livepatch_op(struct xen_sysctl_livepatch_op *op);
>
> xen/common/livepatch.c:int livepatch_op(struct xen_sysctl_livepatch_op
> *livepatch)
>
> Shouldn't this decl also use "*op" as well? Might not be triggered in
> this configuration due to the absence of CONFIG_LIVEPATCH I think.
Yes, I've converted the main function to use op too. It makes the patch
a bit larger, but it's a much better name to use in this context.
~Andrew
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/5] x86/ucode: Don't cast away const-ness in cmp_patch_id()
2025-12-10 18:30 [PATCH 0/5] x86: Misc MISRA fixes Andrew Cooper
2025-12-10 18:30 ` [PATCH 1/5] x86: Misra fixes for U/L suffixes Andrew Cooper
2025-12-10 18:30 ` [PATCH 2/5] x86: Name parameters in function declarations Andrew Cooper
@ 2025-12-10 18:30 ` Andrew Cooper
2025-12-10 20:37 ` Nicola Vetrini
2025-12-10 18:30 ` [PATCH 4/5] x86: Fix missing breaks Andrew Cooper
2025-12-10 18:30 ` [PATCH 5/5] x86: Fix missing brackets in macros Andrew Cooper
4 siblings, 1 reply; 23+ messages in thread
From: Andrew Cooper @ 2025-12-10 18:30 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné,
Stefano Stabellini, consulting @ bugseng . com, Nicola Vetrini
Fixes a volation of MISRA rule 11.8.
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/cpu/microcode/amd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index adabe6e6e838..2760ace92177 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -106,7 +106,7 @@ static bool __ro_after_init entrysign_mitigiated_in_firmware;
static int cf_check cmp_patch_id(const void *key, const void *elem)
{
const struct patch_digest *pd = elem;
- uint32_t patch_id = *(uint32_t *)key;
+ uint32_t patch_id = *(const uint32_t *)key;
if ( patch_id == pd->patch_id )
return 0;
--
2.39.5
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 3/5] x86/ucode: Don't cast away const-ness in cmp_patch_id()
2025-12-10 18:30 ` [PATCH 3/5] x86/ucode: Don't cast away const-ness in cmp_patch_id() Andrew Cooper
@ 2025-12-10 20:37 ` Nicola Vetrini
2025-12-11 0:06 ` Stefano Stabellini
0 siblings, 1 reply; 23+ messages in thread
From: Nicola Vetrini @ 2025-12-10 20:37 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Jan Beulich, Roger Pau Monné, Stefano Stabellini,
consulting @ bugseng . com
On 2025-12-10 19:30, Andrew Cooper wrote:
> Fixes a volation of MISRA rule 11.8.
>
> 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/cpu/microcode/amd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/cpu/microcode/amd.c
> b/xen/arch/x86/cpu/microcode/amd.c
> index adabe6e6e838..2760ace92177 100644
> --- a/xen/arch/x86/cpu/microcode/amd.c
> +++ b/xen/arch/x86/cpu/microcode/amd.c
> @@ -106,7 +106,7 @@ static bool __ro_after_init
> entrysign_mitigiated_in_firmware;
> static int cf_check cmp_patch_id(const void *key, const void *elem)
> {
> const struct patch_digest *pd = elem;
> - uint32_t patch_id = *(uint32_t *)key;
> + uint32_t patch_id = *(const uint32_t *)key;
>
> if ( patch_id == pd->patch_id )
> return 0;
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 3/5] x86/ucode: Don't cast away const-ness in cmp_patch_id()
2025-12-10 20:37 ` Nicola Vetrini
@ 2025-12-11 0:06 ` Stefano Stabellini
0 siblings, 0 replies; 23+ messages in thread
From: Stefano Stabellini @ 2025-12-11 0:06 UTC (permalink / raw)
To: Nicola Vetrini
Cc: Andrew Cooper, Xen-devel, Jan Beulich, Roger Pau Monné,
Stefano Stabellini, consulting @ bugseng . com
On Wed, 10 Dec 2025, Nicola Vetrini wrote:
> On 2025-12-10 19:30, Andrew Cooper wrote:
> > Fixes a volation of MISRA rule 11.8.
> >
> > 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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 4/5] x86: Fix missing breaks
2025-12-10 18:30 [PATCH 0/5] x86: Misc MISRA fixes Andrew Cooper
` (2 preceding siblings ...)
2025-12-10 18:30 ` [PATCH 3/5] x86/ucode: Don't cast away const-ness in cmp_patch_id() Andrew Cooper
@ 2025-12-10 18:30 ` Andrew Cooper
2025-12-10 21:04 ` Nicola Vetrini
2025-12-10 18:30 ` [PATCH 5/5] x86: Fix missing brackets in macros Andrew Cooper
4 siblings, 1 reply; 23+ messages in thread
From: Andrew Cooper @ 2025-12-10 18:30 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné,
Stefano Stabellini, consulting @ bugseng . com, Nicola Vetrini
With the wider testing, some more violations have been spotted. This
addresses violations of Rule 16.3 which requires all case statements to be
terminated with a break or other unconditional control flow change.
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/domain.c | 1 +
xen/arch/x86/mm/shadow/hvm.c | 1 +
xen/arch/x86/pv/emul-priv-op.c | 1 +
xen/arch/x86/pv/emulate.c | 1 +
xen/common/livepatch.c | 1 -
xen/common/livepatch_elf.c | 1 +
6 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 5e37bfbd17d6..b15120180993 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1517,6 +1517,7 @@ int arch_set_info_guest(
{
case -EINTR:
rc = -ERESTART;
+ fallthrough;
case -ERESTART:
break;
case 0:
diff --git a/xen/arch/x86/mm/shadow/hvm.c b/xen/arch/x86/mm/shadow/hvm.c
index 114957a3e1ec..69334c095608 100644
--- a/xen/arch/x86/mm/shadow/hvm.c
+++ b/xen/arch/x86/mm/shadow/hvm.c
@@ -268,6 +268,7 @@ hvm_emulate_cmpxchg(enum x86_segment seg,
default:
SHADOW_PRINTK("cmpxchg size %u is not supported\n", bytes);
prev = ~old;
+ break;
}
if ( prev != old )
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 08dec9990e39..fb6d57d6fbd3 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -407,6 +407,7 @@ static void _guest_io_write(unsigned int port, unsigned int bytes,
default:
ASSERT_UNREACHABLE();
+ break;
}
}
diff --git a/xen/arch/x86/pv/emulate.c b/xen/arch/x86/pv/emulate.c
index 8c44dea12330..b201ea1c6a97 100644
--- a/xen/arch/x86/pv/emulate.c
+++ b/xen/arch/x86/pv/emulate.c
@@ -120,6 +120,7 @@ void pv_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
printk(XENLOG_G_ERR "%s(%pv, 0x%08x, 0x%016"PRIx64") Bad register\n",
__func__, v, reg, val);
domain_crash(d);
+ break;
}
}
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 9285f88644f4..b39f8d7bfe20 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1924,7 +1924,6 @@ static void noinline do_livepatch_work(void)
p->name);
ASSERT_UNREACHABLE();
}
- default:
break;
}
}
diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
index 25ce1bd5a0ad..2e82f2cb8c46 100644
--- a/xen/common/livepatch_elf.c
+++ b/xen/common/livepatch_elf.c
@@ -347,6 +347,7 @@ int livepatch_elf_resolve_symbols(struct livepatch_elf *elf)
dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Symbol resolved: %s => %#"PRIxElfAddr" (%s)\n",
elf->name, elf->sym[i].name,
st_value, elf->sec[idx].name);
+ break;
}
if ( rc )
--
2.39.5
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 4/5] x86: Fix missing breaks
2025-12-10 18:30 ` [PATCH 4/5] x86: Fix missing breaks Andrew Cooper
@ 2025-12-10 21:04 ` Nicola Vetrini
0 siblings, 0 replies; 23+ messages in thread
From: Nicola Vetrini @ 2025-12-10 21:04 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Jan Beulich, Roger Pau Monné, Stefano Stabellini,
consulting @ bugseng . com
On 2025-12-10 19:30, Andrew Cooper wrote:
> With the wider testing, some more violations have been spotted. This
> addresses violations of Rule 16.3 which requires all case statements to
> be
> terminated with a break or other unconditional control flow change.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
with one nit below
> ---
> 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/domain.c | 1 +
> xen/arch/x86/mm/shadow/hvm.c | 1 +
> xen/arch/x86/pv/emul-priv-op.c | 1 +
> xen/arch/x86/pv/emulate.c | 1 +
> xen/common/livepatch.c | 1 -
> xen/common/livepatch_elf.c | 1 +
> 6 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 5e37bfbd17d6..b15120180993 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1517,6 +1517,7 @@ int arch_set_info_guest(
> {
> case -EINTR:
> rc = -ERESTART;
> + fallthrough;
> case -ERESTART:
> break;
> case 0:
> diff --git a/xen/arch/x86/mm/shadow/hvm.c
> b/xen/arch/x86/mm/shadow/hvm.c
> index 114957a3e1ec..69334c095608 100644
> --- a/xen/arch/x86/mm/shadow/hvm.c
> +++ b/xen/arch/x86/mm/shadow/hvm.c
> @@ -268,6 +268,7 @@ hvm_emulate_cmpxchg(enum x86_segment seg,
> default:
> SHADOW_PRINTK("cmpxchg size %u is not supported\n", bytes);
> prev = ~old;
> + break;
> }
>
> if ( prev != old )
> diff --git a/xen/arch/x86/pv/emul-priv-op.c
> b/xen/arch/x86/pv/emul-priv-op.c
> index 08dec9990e39..fb6d57d6fbd3 100644
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -407,6 +407,7 @@ static void _guest_io_write(unsigned int port,
> unsigned int bytes,
>
> default:
> ASSERT_UNREACHABLE();
> + break;
> }
> }
>
> diff --git a/xen/arch/x86/pv/emulate.c b/xen/arch/x86/pv/emulate.c
> index 8c44dea12330..b201ea1c6a97 100644
> --- a/xen/arch/x86/pv/emulate.c
> +++ b/xen/arch/x86/pv/emulate.c
> @@ -120,6 +120,7 @@ void pv_set_reg(struct vcpu *v, unsigned int reg,
> uint64_t val)
> printk(XENLOG_G_ERR "%s(%pv, 0x%08x, 0x%016"PRIx64") Bad
> register\n",
> __func__, v, reg, val);
> domain_crash(d);
> + break;
> }
> }
>
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 9285f88644f4..b39f8d7bfe20 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -1924,7 +1924,6 @@ static void noinline do_livepatch_work(void)
> p->name);
> ASSERT_UNREACHABLE();
> }
> - default:
> break;
could I talk you into changing to
fallthrough;
default:
break;
while it's clear that the meaning is the same, the LIVEPATCH_ACTION_*
constants being switched on are not in an enum, which is made more
obvious by the presence of a default clause.
> }
> }
> diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
> index 25ce1bd5a0ad..2e82f2cb8c46 100644
> --- a/xen/common/livepatch_elf.c
> +++ b/xen/common/livepatch_elf.c
> @@ -347,6 +347,7 @@ int livepatch_elf_resolve_symbols(struct
> livepatch_elf *elf)
> dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Symbol resolved:
> %s => %#"PRIxElfAddr" (%s)\n",
> elf->name, elf->sym[i].name,
> st_value, elf->sec[idx].name);
> + break;
> }
>
> if ( rc )
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 5/5] x86: Fix missing brackets in macros
2025-12-10 18:30 [PATCH 0/5] x86: Misc MISRA fixes Andrew Cooper
` (3 preceding siblings ...)
2025-12-10 18:30 ` [PATCH 4/5] x86: Fix missing breaks Andrew Cooper
@ 2025-12-10 18:30 ` Andrew Cooper
2025-12-10 21:11 ` Nicola Vetrini
2025-12-11 8:36 ` Jan Beulich
4 siblings, 2 replies; 23+ messages in thread
From: Andrew Cooper @ 2025-12-10 18:30 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné,
Stefano Stabellini, consulting @ bugseng . com, Nicola Vetrini
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 5/5] x86: Fix missing brackets in macros
2025-12-10 18:30 ` [PATCH 5/5] x86: Fix missing brackets in macros Andrew Cooper
@ 2025-12-10 21:11 ` Nicola Vetrini
2025-12-11 0:07 ` Stefano Stabellini
2025-12-11 8:36 ` Jan Beulich
1 sibling, 1 reply; 23+ messages in thread
From: Nicola Vetrini @ 2025-12-10 21:11 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Jan Beulich, Roger Pau Monné, Stefano Stabellini,
consulting @ bugseng . com
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 5/5] x86: Fix missing brackets in macros
2025-12-10 21:11 ` Nicola Vetrini
@ 2025-12-11 0:07 ` Stefano Stabellini
0 siblings, 0 replies; 23+ messages in thread
From: Stefano Stabellini @ 2025-12-11 0:07 UTC (permalink / raw)
To: Nicola Vetrini
Cc: Andrew Cooper, Xen-devel, Jan Beulich, Roger Pau Monné,
Stefano Stabellini, consulting @ bugseng . com
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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/5] x86: Fix missing brackets in macros
2025-12-10 18:30 ` [PATCH 5/5] x86: Fix missing brackets in macros Andrew Cooper
2025-12-10 21:11 ` Nicola Vetrini
@ 2025-12-11 8:36 ` Jan Beulich
2025-12-11 9:15 ` Nicola Vetrini
1 sibling, 1 reply; 23+ messages in thread
From: Jan Beulich @ 2025-12-11 8:36 UTC (permalink / raw)
To: Andrew Cooper
Cc: Roger Pau Monné, Stefano Stabellini,
consulting @ bugseng . com, Nicola Vetrini, Xen-devel
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 5/5] x86: Fix missing brackets in macros
2025-12-11 8:36 ` Jan Beulich
@ 2025-12-11 9:15 ` Nicola Vetrini
2025-12-11 9:30 ` Jan Beulich
0 siblings, 1 reply; 23+ messages in thread
From: Nicola Vetrini @ 2025-12-11 9:15 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Roger Pau Monné, Stefano Stabellini,
consulting @ bugseng . com, Xen-devel
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 5/5] x86: Fix missing brackets in macros
2025-12-11 9:15 ` Nicola Vetrini
@ 2025-12-11 9:30 ` Jan Beulich
2025-12-11 10:38 ` Nicola Vetrini
0 siblings, 1 reply; 23+ messages in thread
From: Jan Beulich @ 2025-12-11 9:30 UTC (permalink / raw)
To: Nicola Vetrini
Cc: Andrew Cooper, Roger Pau Monné, Stefano Stabellini,
consulting @ bugseng . com, Xen-devel
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 5/5] x86: Fix missing brackets in macros
2025-12-11 9:30 ` Jan Beulich
@ 2025-12-11 10:38 ` Nicola Vetrini
2025-12-11 12:28 ` Nicola Vetrini
0 siblings, 1 reply; 23+ messages in thread
From: Nicola Vetrini @ 2025-12-11 10:38 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Roger Pau Monné, Stefano Stabellini,
consulting @ bugseng . com, Xen-devel
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 5/5] x86: Fix missing brackets in macros
2025-12-11 10:38 ` Nicola Vetrini
@ 2025-12-11 12:28 ` Nicola Vetrini
2025-12-12 1:45 ` Andrew Cooper
0 siblings, 1 reply; 23+ messages in thread
From: Nicola Vetrini @ 2025-12-11 12:28 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper3
Cc: Roger Pau Monné, Stefano Stabellini,
consulting @ bugseng . com, Xen-devel
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 5/5] x86: Fix missing brackets in macros
2025-12-11 12:28 ` Nicola Vetrini
@ 2025-12-12 1:45 ` Andrew Cooper
0 siblings, 0 replies; 23+ messages in thread
From: Andrew Cooper @ 2025-12-12 1:45 UTC (permalink / raw)
To: Nicola Vetrini, Jan Beulich
Cc: Andrew Cooper, Roger Pau Monné, Stefano Stabellini,
consulting @ bugseng . com, Xen-devel
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
^ permalink raw reply [flat|nested] 23+ messages in thread