[PATCH 0/5] x86: Misc MISRA fixes

[PATCH 0/5] x86: Misc MISRA fixes

[Andrew Cooper]

With the wider testing, some more violations have been spotted.  Address some
of the easier ones.

https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/2207505665

Andrew Cooper (5):
  x86: Misra fixes for U/L suffixes
  x86: Name parameters in function declarations
  x86/ucode: Don't cast away const-ness in cmp_patch_id()
  x86: Fix missing breaks
  x86: Fix missing brackets in macros

 xen/arch/x86/cpu/microcode/amd.c    |  2 +-
 xen/arch/x86/domain.c               |  1 +
 xen/arch/x86/mm/shadow/common.c     | 12 ++++++------
 xen/arch/x86/mm/shadow/hvm.c        |  1 +
 xen/arch/x86/mm/shadow/multi.c      |  2 +-
 xen/arch/x86/mm/shadow/private.h    |  6 +++---
 xen/arch/x86/pv/descriptor-tables.c |  2 +-
 xen/arch/x86/pv/emul-priv-op.c      |  3 ++-
 xen/arch/x86/pv/emulate.c           |  1 +
 xen/common/livepatch.c              |  1 -
 xen/common/livepatch_elf.c          |  1 +
 xen/drivers/passthrough/vtd/dmar.h  |  2 +-
 xen/drivers/passthrough/vtd/iommu.h |  2 +-
 xen/include/xen/elfstructs.h        |  2 +-
 xen/include/xen/kexec.h             |  4 ++--
 xen/include/xen/livepatch.h         |  2 +-
 xen/include/xen/sizes.h             |  2 +-
 17 files changed, 25 insertions(+), 21 deletions(-)

-- 
2.39.5

[PATCH 1/5] x86: Misra fixes for U/L suffixes

[Andrew Cooper]

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

[PATCH 2/5] x86: Name parameters in function declarations

[Andrew Cooper]

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

[PATCH 3/5] x86/ucode: Don't cast away const-ness in cmp_patch_id()

[Andrew Cooper]

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

[PATCH 4/5] x86: Fix missing breaks

[Andrew Cooper]

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

[PATCH 5/5] x86: Fix missing brackets in macros

[Andrew Cooper]

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

Re: [PATCH 1/5] x86: Misra fixes for U/L suffixes

[Nicola Vetrini]

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

Re: [PATCH 2/5] x86: Name parameters in function declarations

[Nicola Vetrini]

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

Re: [PATCH 1/5] x86: Misra fixes for U/L suffixes

[Nicola Vetrini]

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

Re: [PATCH 3/5] x86/ucode: Don't cast away const-ness in cmp_patch_id()

[Nicola Vetrini]

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

Re: [PATCH 4/5] x86: Fix missing breaks

[Nicola Vetrini]

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

Re: [PATCH 5/5] x86: Fix missing brackets in macros

[Nicola Vetrini]

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

Re: [PATCH 1/5] x86: Misra fixes for U/L suffixes

[Andrew Cooper]

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

Re: [PATCH 3/5] x86/ucode: Don't cast away const-ness in cmp_patch_id()

[Stefano Stabellini]

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>

Re: [PATCH 5/5] x86: Fix missing brackets in macros

[Stefano Stabellini]

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>

Re: [PATCH 1/5] x86: Misra fixes for U/L suffixes

[Nicola Vetrini]

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

Re: [PATCH 5/5] x86: Fix missing brackets in macros

[Jan Beulich]

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

Re: [PATCH 5/5] x86: Fix missing brackets in macros

[Nicola Vetrini]

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

Re: [PATCH 5/5] x86: Fix missing brackets in macros

[Jan Beulich]

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

Re: [PATCH 5/5] x86: Fix missing brackets in macros

[Nicola Vetrini]

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

Re: [PATCH 5/5] x86: Fix missing brackets in macros

[Nicola Vetrini]

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

Re: [PATCH 2/5] x86: Name parameters in function declarations

[Andrew Cooper]

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

Re: [PATCH 5/5] x86: Fix missing brackets in macros

[Andrew Cooper]

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