[PATCH 00/14] address half the remaining Misra rule 11.8 violations

[PATCH 00/14] address half the remaining Misra rule 11.8 violations

[Jan Beulich]

Casting away const-ness (or volatile-ness) is generally bad practices,
so this is perhaps one of the more important rules to get into clean
state. Despite being bad practice in general, there are cases where
doing so is pretty much unavoidable; in some cases even the library
spec mandates doing so (without Misra having any provisions for that).
How to deal with most of those cases is still up for discussion, only
very few of them are actually dealt with here.

As per [1] this addresses about half the remaining violations in all
four analyze jobs we have in gitlab-CI (patch 06 wasn't included
there, yet).

01: lib: obey to Misra rule 11.8 where possible
02: x86/bitops: don't cast away volatile-ness
03: x86: have cmpxchg16b() obey to Misra rule 11.8
04: x86/boot: don't cast away const-ness
05: x86/altcall: hide casting away of const
06: Arm/alternative: hide casting away of const
07: Arm64/GICv3: have gicv3_its_find_quirk() not cast away const-ness
08: Arm/guestcopy: deviate copy_guest() uses just like their x86 counterparts
09: ACPI: address a Misra rule 11.8 violation in Arm code
10: ELF/notes: use pointer-to-const by default in ELFNOTE_...()
11: crypto/vmac: don't cast away const-ness in aes_key_setup()
12: gnttab: don't cast away constness
13: passthrough/PCI: rework (s,b,d,f) init of struct pci_dev
14: xhci-dbc: don't cast away const-ness in xhci_find_dbc()

Jan

[1] https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/2810122986

[PATCH 01/14] lib: obey to Misra rule 11.8 where possible

[Jan Beulich]

Casting away const-ness (or volatile-ness) is never a good idea, but
some library functions (e.g. strchr()) require doing so. Where not
required, remove / replace respective casts.

While there convert touched functions to Xen style.

No functional change intended.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/lib/memcpy.c
+++ b/xen/lib/memcpy.c
@@ -15,12 +15,13 @@
  */
 void *(memcpy)(void *dest, const void *src, size_t n)
 {
-	char *tmp = (char *) dest, *s = (char *) src;
+    char *tmp = dest;
+    const char *s = src;
 
-	while (n--)
-		*tmp++ = *s++;
+    while ( n-- )
+        *tmp++ = *s++;
 
-	return dest;
+    return dest;
 }
 
 /*
--- a/xen/lib/memmove.c
+++ b/xen/lib/memmove.c
@@ -14,21 +14,25 @@
  */
 void *(memmove)(void *dest, const void *src, size_t n)
 {
-	char *tmp, *s;
+    char *tmp;
+    const char *s;
 
-	if (dest <= src) {
-		tmp = (char *) dest;
-		s = (char *) src;
-		while (n--)
-			*tmp++ = *s++;
-	} else {
-		tmp = (char *) dest + n;
-		s = (char *) src + n;
-		while (n--)
-			*--tmp = *--s;
-	}
+    if ( dest <= src )
+    {
+        tmp = dest;
+        s = src;
+        while ( n-- )
+            *tmp++ = *s++;
+    }
+    else
+    {
+        tmp = dest + n;
+        s = src + n;
+        while ( n-- )
+            *--tmp = *--s;
+    }
 
-	return dest;
+    return dest;
 }
 
 /*
--- a/xen/lib/strcmp.c
+++ b/xen/lib/strcmp.c
@@ -11,16 +11,15 @@
  */
 int (strcmp)(const char *cs, const char *ct)
 {
-	unsigned char *csu = (unsigned char *)cs;
-	unsigned char *ctu = (unsigned char *)ct;
-	int res;
+    const unsigned char *csu = (const void *)cs;
+    const unsigned char *ctu = (const void *)ct;
+    int res;
 
-	while (1) {
-		if ((res = *csu - *ctu++) != 0 || !*csu++)
-			break;
-	}
+    for ( ; ; )
+        if ( (res = *csu - *ctu++) != 0 || !*csu++ )
+            break;
 
-	return res;
+    return res;
 }
 
 /*
--- a/xen/lib/strncmp.c
+++ b/xen/lib/strncmp.c
@@ -12,17 +12,18 @@
  */
 int (strncmp)(const char *cs, const char *ct, size_t count)
 {
-	unsigned char *csu = (unsigned char *)cs;
-	unsigned char *ctu = (unsigned char *)ct;
-	int res = 0;
+    const unsigned char *csu = (const void *)cs;
+    const unsigned char *ctu = (const void *)ct;
+    int res = 0;
 
-	while (count) {
-		if ((res = *csu - *ctu++) != 0 || !*csu++)
-			break;
-		count--;
-	}
+    while ( count )
+    {
+        if ( (res = *csu - *ctu++) != 0 || !*csu++ )
+            break;
+        count--;
+    }
 
-	return res;
+    return res;
 }
 
 /*

[PATCH 02/14] x86/bitops: don't cast away volatile-ness

[Jan Beulich]

Doing so, besides being a bad idea in general, violates Misra rule 11.8.
Use the helper macro we have available anyway.

No functional change intended.

Fixes: f1879b2c2584 ("xen: introduce generic non-atomic test_*bit()")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/include/asm/bitops.h
+++ b/xen/arch/x86/include/asm/bitops.h
@@ -188,7 +188,7 @@ static inline int arch__test_and_set_bit
     asm volatile ( "btsl %[nr], %[addr]\n\t"
                    ASM_FLAG_OUT(, "sbbl %[old], %[old]\n\t")
                    : [old] ASM_FLAG_OUT("=@ccc", "=r") (oldbit),
-                     [addr] "+m" (*(int *)addr) : [nr] "Ir" (nr) : "memory" );
+                     [addr] "+m" (ADDR) : [nr] "Ir" (nr) : "memory" );
 
     return oldbit;
 }
@@ -234,7 +234,7 @@ static inline int arch__test_and_clear_b
     asm volatile ( "btrl %[nr], %[addr]\n\t"
                    ASM_FLAG_OUT(, "sbbl %[old], %[old]\n\t")
                    : [old] ASM_FLAG_OUT("=@ccc", "=r") (oldbit),
-                     [addr] "+m" (*(int *)addr) : [nr] "Ir" (nr) : "memory" );
+                     [addr] "+m" (ADDR) : [nr] "Ir" (nr) : "memory" );
 
     return oldbit;
 }
@@ -248,7 +248,7 @@ static inline int arch__test_and_change_
     asm volatile ( "btcl %[nr], %[addr]\n\t"
                    ASM_FLAG_OUT(, "sbbl %[old], %[old]\n\t")
                    : [old] ASM_FLAG_OUT("=@ccc", "=r") (oldbit),
-                     [addr] "+m" (*(int *)addr) : [nr] "Ir" (nr) : "memory" );
+                     [addr] "+m" (ADDR) : [nr] "Ir" (nr) : "memory" );
 
     return oldbit;
 }

[PATCH 03/14] x86: have cmpxchg16b() obey to Misra rule 11.8

[Jan Beulich]

Casting away const-ness (or volatile-ness) is never a good idea, and
there's no need to in cmpxchg16b().

No functional change intended.

Fixes: e0116acbbd0b ("x86: add cmpxchg16b support")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/include/asm/x86_64/system.h
+++ b/xen/arch/x86/include/asm/x86_64/system.h
@@ -56,7 +56,7 @@ static always_inline __uint128_t cmpxchg
     ASSERT(!((unsigned long)_p & 0xf));                    \
     BUILD_BUG_ON(sizeof(*(o)) != sizeof(__uint128_t));     \
     BUILD_BUG_ON(sizeof(*(n)) != sizeof(__uint128_t));     \
-    __cmpxchg16b(_p, (void *)(o), (void *)(n));            \
+    __cmpxchg16b(_p, (const void *)(o), (const void *)(n));\
 })
 
 #endif /* __X86_64_SYSTEM_H__ */

[PATCH 04/14] x86/boot: don't cast away const-ness

[Jan Beulich]

While the general C library strchr() has to do so, the early boot cmdline
parsing has no such requirement.

No functional change.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/boot/cmdline.c
+++ b/xen/arch/x86/boot/cmdline.c
@@ -77,12 +77,12 @@ static int strncmp(const char *cs, const
     return 0;
 }
 
-static char *strchr(const char *s, int c)
+static const char *strchr(const char *s, int c)
 {
     for ( ; *s != (char)c; ++s )
         if ( *s == '\0' )
             return NULL;
-    return (char *)s;
+    return s;
 }
 
 static size_t strspn(const char *s, const char *accept)

[PATCH 05/14] x86/altcall: hide casting away of const

[Jan Beulich]

ALT_CALL_PTR() really means to get rid of const (when necessary, i.e. when
used from apply_alt_calls()): We want to patch the original call site,
after all, and memory-wise that's unrelated to the struct alt_call * that
we start from.

No functional change intended.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/include/asm/alternative-call.h
+++ b/xen/arch/x86/include/asm/alternative-call.h
@@ -9,7 +9,7 @@
 struct alt_call {
     int32_t offset;
 };
-#define ALT_CALL_PTR(a) ((void *)&(a)->offset + (a)->offset)
+#define ALT_CALL_PTR(a) ((void *)((long)&(a)->offset + (a)->offset))
 #define ALT_CALL_LEN(a) (6)
 
 /*

[PATCH 06/14] Arm/alternative: hide casting away of const

[Jan Beulich]

We want to patch the original call site, which memory-wise is unrelated
to the struct alt_instr * that we start from.

No functional change intended.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -134,7 +134,7 @@ static int __apply_alternatives(const st
             BUG_ON(alt->repl_len != alt->orig_len);
 
         origptr = ALT_ORIG_PTR(alt);
-        updptr = (void *)origptr + update_offset;
+        updptr = (void *)(long)origptr + update_offset;
 
         nr_inst = alt->orig_len / ARCH_PATCH_INSN_SIZE;
 

[PATCH 07/14] Arm64/GICv3: have gicv3_its_find_quirk() not cast away const-ness

[Jan Beulich]

Doing so violates Misra rule 11.8, while being entirely unnecessary here:
All callers already store the return value in pointer-to-const variables.

No functional change.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -84,14 +84,14 @@ static const struct its_quirk its_quirks
     }
 };
 
-static struct its_quirk* gicv3_its_find_quirk(uint32_t iidr)
+static const struct its_quirk *gicv3_its_find_quirk(uint32_t iidr)
 {
     const struct its_quirk *quirks = its_quirks;
 
     for ( ; quirks->desc; quirks++ )
     {
         if ( quirks->iidr == (quirks->mask & iidr) )
-            return (struct its_quirk *)quirks;
+            return quirks;
     }
 
     return NULL;

[PATCH 08/14] Arm/guestcopy: deviate copy_guest() uses just like their x86 counterparts

[Jan Beulich]

Like x86/HVM's __hvm_copy(), Arm's copy_guest() is used for both to-guest
and from-guest copying. Naturally in the latter case the hypervisor buffer
needs writing to, hence the function parameter cannot be pointer-to-const.

No functional change intended.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Of course for both the pre-existing x86 deviation and the new Arm one it
might be more robust if the deviation was also limited to the respective
source file. Can this be expressed together with the needed regex?

--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -433,6 +433,12 @@ Fixing this violation would require to i
 -config=MC3A2.R11.8,reports+={safe,"any_area(any_loc(any_exp(macro(^container_of$))))"}
 -doc_end
 
+-doc_begin="Function copy_guest() in xen/arch/arm/guestcopy.c is a double-use
+function, where the parameter needs to not be const because it can be set for
+write or not"
+-config=MC3A2.R11.8,reports+={safe,"any_area(any_loc(text(^.*copy_guest.*COPY_to_guest doesn't modify.*$)))"}
+-doc_end
+
 -doc_begin="Function __hvm_copy in xen/arch/x86/hvm/hvm.c is a double-use
 function, where the parameter needs to not be const because it can be set for
 write or not"
--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -109,14 +109,16 @@ static unsigned long copy_guest(void *bu
 
 unsigned long raw_copy_to_guest(void *to, const void *from, unsigned int len)
 {
-    return copy_guest((void *)from, (vaddr_t)to, len,
-                      GVA_INFO(current), COPY_to_guest | COPY_linear);
+    return copy_guest((void *)from, /* COPY_to_guest doesn't modify */
+                      (vaddr_t)to, len, GVA_INFO(current),
+                      COPY_to_guest | COPY_linear);
 }
 
 unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
                                              unsigned int len)
 {
-    return copy_guest((void *)from, (vaddr_t)to, len, GVA_INFO(current),
+    return copy_guest((void *)from, /* COPY_to_guest doesn't modify */
+                      (vaddr_t)to, len, GVA_INFO(current),
                       COPY_to_guest | COPY_flush_dcache | COPY_linear);
 }
 

[PATCH 09/14] ACPI: address a Misra rule 11.8 violation in Arm code

[Jan Beulich]

Outside of drivers/acpi/tables/ (which is excluded from Eclair reporting
for dubious reasons), arch/arm/acpi/domain_build.c is the only user of
ACPI_COMPARE_NAME(). Make the macro const correct, at the expense of
introducing a "const" variant of ACPI_CAST_PTR(), and at the expense of
altering an imported file.

No functional change intended.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/acpi/acmacros.h
+++ b/xen/include/acpi/acmacros.h
@@ -103,6 +103,7 @@
  * Pointer manipulation
  */
 #define ACPI_CAST_PTR(t, p)             ((t *) (acpi_uintptr_t) (p))
+#define ACPI_CAST_CPTR(t, p)            ((const t *) (const acpi_uintptr_t) (p))
 #define ACPI_CAST_INDIRECT_PTR(t, p)    ((t **) (acpi_uintptr_t) (p))
 #define ACPI_ADD_PTR(t,a,b)             ACPI_CAST_PTR (t, (ACPI_CAST_PTR (u8,(a)) + (acpi_native_uint)(b)))
 #define ACPI_PTR_DIFF(a,b)              (acpi_native_uint) (ACPI_CAST_PTR (u8,(a)) - ACPI_CAST_PTR (u8,(b)))
@@ -116,9 +117,12 @@
 #define ACPI_PTR_TO_PHYSADDR(i)         ACPI_TO_INTEGER(i)
 
 #ifndef ACPI_MISALIGNMENT_NOT_SUPPORTED
-#define ACPI_COMPARE_NAME(a,b)          (*ACPI_CAST_PTR (u32,(a)) == *ACPI_CAST_PTR (u32,(b)))
+#define ACPI_COMPARE_NAME(a, b)         (*ACPI_CAST_CPTR (u32, a) == \
+                                         *ACPI_CAST_CPTR (u32, b))
 #else
-#define ACPI_COMPARE_NAME(a,b)          (!ACPI_STRNCMP (ACPI_CAST_PTR (char,(a)), ACPI_CAST_PTR (char,(b)), ACPI_NAME_SIZE))
+#define ACPI_COMPARE_NAME(a, b)         (!ACPI_STRNCMP (ACPI_CAST_CPTR (char, a), \
+                                                        ACPI_CAST_CPTR (char, b), \
+                                                        ACPI_NAME_SIZE))
 #endif
 
 /*

[PATCH 10/14] ELF/notes: use pointer-to-const by default in ELFNOTE_...()

[Jan Beulich]

Not doing so results in a number of Misra rule 11.8 (casting away of
const-ness) violations. We need to allow kexec to use pointer to non-
const though, so provide a means to override the default.

For ELFNOTE_NEXT() we can do better and simply re-apply the type of the
incoming pointer.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -6,6 +6,9 @@
  * - Magnus Damm <magnus@valinux.co.jp>
  */
 
+/* We're producing ELF notes here. */
+#define ELFNOTE_CONST
+
 #include <xen/acpi.h>
 #include <xen/console.h>
 #include <xen/cpu.h>
--- a/xen/include/xen/elf.h
+++ b/xen/include/xen/elf.h
@@ -29,9 +29,14 @@
 
 #include <xen/elfstructs.h>
 
+#ifndef ELFNOTE_CONST
+#define ELFNOTE_CONST const
+#endif
+
 #define ELFNOTE_ALIGN(_n_) (((_n_)+3)&~3)
-#define ELFNOTE_NAME(_n_) ((char*)(_n_) + sizeof(*(_n_)))
+#define ELFNOTE_NAME(_n_) ((ELFNOTE_CONST char *)(_n_) + sizeof(*(_n_)))
 #define ELFNOTE_DESC(_n_) (ELFNOTE_NAME(_n_) + ELFNOTE_ALIGN((_n_)->namesz))
-#define ELFNOTE_NEXT(_n_) ((Elf_Note *)(ELFNOTE_DESC(_n_) + ELFNOTE_ALIGN((_n_)->descsz)))
+#define ELFNOTE_NEXT(_n_) ((typeof(_n_))(ELFNOTE_DESC(_n_) + \
+                                         ELFNOTE_ALIGN((_n_)->descsz)))
 
 #endif /* __XEN_ELF_H__ */

[PATCH 11/14] crypto/vmac: don't cast away const-ness in aes_key_setup()

[Jan Beulich]

vmac_set_key() passes in a pointer-to-const, which has const-ness removed
despite rijndaelKeySetupEnc() properly taking pointer-to-const.

No functional change.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I was wondering whether I shouldn't adjust the adjacent aes_encryption()
right away as well.

--- a/xen/include/crypto/vmac.h
+++ b/xen/include/crypto/vmac.h
@@ -69,7 +69,7 @@ typedef u32 aes_int_key[4*(VMAC_KEY_LEN/
 	    				    (u8 *)(in), (u8 *)(out))
 #define aes_key_setup(user_key,int_key)                 \
 	    	rijndaelKeySetupEnc((u32 *)(int_key),       \
-	    	                    (u8 *)(user_key), \
+	    	                    (const u8 *)(user_key), \
 	    	                    VMAC_KEY_LEN)
 #endif
 

[PATCH 12/14] gnttab: don't cast away constness

[Jan Beulich]

While _set_status_v2() indeed doesn't alter the grant_entry_header_t it
is handed a pointer to, _set_status_v1() does. Drop the const from the
parameter of the latter (and then necessarily also from _set_status()'s),
while adding const to the local variable of the former.

No functional change.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Really I think it would be best if we did away with the raw_shah local
variables (which looks reasonably simple for at least _set_status_v2()).
I'm unconvinced that we really need to use ACCESS_ONCE() here. Torn reads
aren't a problem; what we require is that we look at a stable local copy,
and that can be achieved by putting barrier() after the reads.

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -740,7 +740,7 @@ static unsigned int nr_grant_entries(str
     return 0;
 }
 
-static int _set_status_v1(const grant_entry_header_t *shah,
+static int _set_status_v1(grant_entry_header_t *shah,
                           struct domain *rd,
                           struct active_grant_entry *act,
                           int readonly,
@@ -832,7 +832,7 @@ static int _set_status_v2(const grant_en
                           domid_t  ldomid)
 {
     int      rc    = GNTST_okay;
-    uint32_t *raw_shah = (uint32_t *)shah;
+    const uint32_t *raw_shah = (const uint32_t *)shah;
     union grant_combo scombo;
     uint16_t mask  = GTF_type_mask;
 
@@ -909,7 +909,7 @@ done:
 }
 
 
-static int _set_status(const grant_entry_header_t *shah,
+static int _set_status(grant_entry_header_t *shah,
                        grant_status_t *status,
                        struct domain *rd,
                        unsigned int rgt_version,

[PATCH 13/14] passthrough/PCI: rework (s,b,d,f) init of struct pci_dev

[Jan Beulich]

Right now we're casting away const-ness, to initialize the individual
elements despite the field(s) being declared const. Eclair validly
recognizes this as a Misra rule 11.8 violation. Hide this by switching to
the use of memcpy(), deriving the destination address from the (mutable)
struct pci_dev * which we hold in hands.

No functional change intended.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Subsequently we may want to further leverage the sbdf local variable we
now have in the function. Yet of course the primary question is: Is this
an okay game to play in the first place?

--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -318,6 +318,7 @@ static void apply_quirks(struct pci_dev
 static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
 {
     struct pci_dev *pdev;
+    pci_sbdf_t sbdf = { .seg = pseg->nr, .bus = bus, .devfn = devfn };
     unsigned int pos;
     int rc;
 
@@ -329,9 +330,15 @@ static struct pci_dev *alloc_pdev(struct
     if ( !pdev )
         return NULL;
 
-    *(u16*) &pdev->seg = pseg->nr;
-    *((u8*) &pdev->bus) = bus;
-    *((u8*) &pdev->devfn) = devfn;
+    /*
+     * pdev->sbdf is deliberately const, i.e. it can't be written by structure
+     * or field assignment.  Writing by memcpy() works, as long as it's not
+     * &pdev->sbdf which is passed.  Since memcpy() isn't type-safe, have an
+     * explicit type check first.
+     */
+    (void)(&pdev->sbdf != &sbdf);
+    memcpy((void *)pdev + offsetof(struct pci_dev, sbdf), &sbdf, sizeof(sbdf));
+
     pdev->domain = NULL;
 
     INIT_LIST_HEAD(&pdev->vf_list);
@@ -390,7 +397,6 @@ static struct pci_dev *alloc_pdev(struct
                          phantom_devs[i].slot == PCI_SLOT(devfn) &&
                          phantom_devs[i].stride > PCI_FUNC(devfn) )
                     {
-                        pci_sbdf_t sbdf = pdev->sbdf;
                         unsigned int stride = phantom_devs[i].stride;
 
                         while ( (sbdf.fn += stride) > PCI_FUNC(devfn) )

[PATCH 14/14] xhci-dbc: don't cast away const-ness in xhci_find_dbc()

[Jan Beulich]

Doing so (at the return statement), besides being a bad idea anyway, is a
violation of Misra rule 11.8.

Make "xcap" have an initializer while adjusting the code anyway.

No functional change intended.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -384,16 +384,15 @@ static bool __init dbc_init_xhc(struct d
  */
 static struct dbc_reg __iomem *xhci_find_dbc(struct dbc *dbc)
 {
-    const uint32_t __iomem *xcap;
     uint32_t xcap_val;
     uint32_t next;
     uint32_t id = 0;
-    const void __iomem *mmio = dbc->xhc_mmio;
+    void __iomem *mmio = dbc->xhc_mmio;
     const uint32_t __iomem *hccp1 = mmio + 0x10;
+    uint32_t __iomem *xcap = mmio;
     const uint32_t DBC_ID = 0xA;
     int ttl = 48;
 
-    xcap = mmio;
     /*
      * This is initially an offset to the first capability. All the offsets
      * (both in HCCP1 and then next capability pointer) are dword-based.

Re: [PATCH 14/14] xhci-dbc: don't cast away const-ness in xhci_find_dbc()

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]

On Wed, Sep 02, 2026 at 08:37:26AM +0200, Jan Beulich wrote:
> Doing so (at the return statement), besides being a bad idea anyway, is a
> violation of Misra rule 11.8.
> 
> Make "xcap" have an initializer while adjusting the code anyway.
> 
> No functional change intended.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

> --- a/xen/drivers/char/xhci-dbc.c
> +++ b/xen/drivers/char/xhci-dbc.c
> @@ -384,16 +384,15 @@ static bool __init dbc_init_xhc(struct d
>   */
>  static struct dbc_reg __iomem *xhci_find_dbc(struct dbc *dbc)
>  {
> -    const uint32_t __iomem *xcap;
>      uint32_t xcap_val;
>      uint32_t next;
>      uint32_t id = 0;
> -    const void __iomem *mmio = dbc->xhc_mmio;
> +    void __iomem *mmio = dbc->xhc_mmio;
>      const uint32_t __iomem *hccp1 = mmio + 0x10;
> +    uint32_t __iomem *xcap = mmio;
>      const uint32_t DBC_ID = 0xA;
>      int ttl = 48;
>  
> -    xcap = mmio;
>      /*
>       * This is initially an offset to the first capability. All the offsets
>       * (both in HCCP1 and then next capability pointer) are dword-based.
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 07/14] Arm64/GICv3: have gicv3_its_find_quirk() not cast away const-ness

[Orzel, Michal]

On 02-Sep-26 08:33, Jan Beulich wrote:
> Doing so violates Misra rule 11.8, while being entirely unnecessary here:
> All callers already store the return value in pointer-to-const variables.
> 
> No functional change.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH 02/14] x86/bitops: don't cast away volatile-ness

[Andrew Cooper]

On 02/09/2026 7:30 am, Jan Beulich wrote:
> Doing so, besides being a bad idea in general, violates Misra rule 11.8.
> Use the helper macro we have available anyway.
>
> No functional change intended.
>
> Fixes: f1879b2c2584 ("xen: introduce generic non-atomic test_*bit()")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I really want to kill ADDR, not expand it's use.

I'll do a series and pull this patch into it.

~Andrew

Re: [PATCH 06/14] Arm/alternative: hide casting away of const

[Orzel, Michal]

On 02-Sep-26 08:32, Jan Beulich wrote:
> We want to patch the original call site, which memory-wise is unrelated
> to the struct alt_instr * that we start from.
> 
> No functional change intended.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/arm/alternative.c
> +++ b/xen/arch/arm/alternative.c
> @@ -134,7 +134,7 @@ static int __apply_alternatives(const st
>              BUG_ON(alt->repl_len != alt->orig_len);
>  
>          origptr = ALT_ORIG_PTR(alt);
> -        updptr = (void *)origptr + update_offset;
> +        updptr = (void *)(long)origptr + update_offset;
NIT: Why long and not unsigned long like other places in this file?

Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH 06/14] Arm/alternative: hide casting away of const

[Jan Beulich]

On 02.09.2026 13:47, Orzel, Michal wrote:
> On 02-Sep-26 08:32, Jan Beulich wrote:
>> We want to patch the original call site, which memory-wise is unrelated
>> to the struct alt_instr * that we start from.
>>
>> No functional change intended.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/arm/alternative.c
>> +++ b/xen/arch/arm/alternative.c
>> @@ -134,7 +134,7 @@ static int __apply_alternatives(const st
>>              BUG_ON(alt->repl_len != alt->orig_len);
>>  
>>          origptr = ALT_ORIG_PTR(alt);
>> -        updptr = (void *)origptr + update_offset;
>> +        updptr = (void *)(long)origptr + update_offset;
> NIT: Why long and not unsigned long like other places in this file?

Hmm, that was inherited from "x86/altcall: hide casting away of const",
yet apparently wrongly so: update_offset is of unsigned type here. I've
switched locally.

> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Thanks.

Jan

Re: [PATCH 13/14] passthrough/PCI: rework (s,b,d,f) init of struct pci_dev

[Roger Pau Monné]

On Wed, Sep 02, 2026 at 08:36:27AM +0200, Jan Beulich wrote:
> Right now we're casting away const-ness, to initialize the individual
> elements despite the field(s) being declared const. Eclair validly
> recognizes this as a Misra rule 11.8 violation. Hide this by switching to
> the use of memcpy(), deriving the destination address from the (mutable)
> struct pci_dev * which we hold in hands.
> 
> No functional change intended.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Subsequently we may want to further leverage the sbdf local variable we
> now have in the function. Yet of course the primary question is: Is this
> an okay game to play in the first place?

I've wondered the same, while this might be obfuscated enough for
Eclair to complain, aren't we still violating the spirit of the rule?

Regards, Roger.

Re: [PATCH 13/14] passthrough/PCI: rework (s,b,d,f) init of struct pci_dev

[Jan Beulich]

On 02.09.2026 14:44, Roger Pau Monné wrote:
> On Wed, Sep 02, 2026 at 08:36:27AM +0200, Jan Beulich wrote:
>> Right now we're casting away const-ness, to initialize the individual
>> elements despite the field(s) being declared const. Eclair validly
>> recognizes this as a Misra rule 11.8 violation. Hide this by switching to
>> the use of memcpy(), deriving the destination address from the (mutable)
>> struct pci_dev * which we hold in hands.
>>
>> No functional change intended.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Subsequently we may want to further leverage the sbdf local variable we
>> now have in the function. Yet of course the primary question is: Is this
>> an okay game to play in the first place?
> 
> I've wondered the same, while this might be obfuscated enough for
> Eclair to complain, aren't we still violating the spirit of the rule?

We do, but what do you do without dropping that "const" (which I'd really
like to keep), and without C++ concepts of initialization?

Jan

Re: [PATCH 13/14] passthrough/PCI: rework (s,b,d,f) init of struct pci_dev

[Roger Pau Monné]

On Wed, Sep 02, 2026 at 02:53:58PM +0200, Jan Beulich wrote:
> On 02.09.2026 14:44, Roger Pau Monné wrote:
> > On Wed, Sep 02, 2026 at 08:36:27AM +0200, Jan Beulich wrote:
> >> Right now we're casting away const-ness, to initialize the individual
> >> elements despite the field(s) being declared const. Eclair validly
> >> recognizes this as a Misra rule 11.8 violation. Hide this by switching to
> >> the use of memcpy(), deriving the destination address from the (mutable)
> >> struct pci_dev * which we hold in hands.
> >>
> >> No functional change intended.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> Subsequently we may want to further leverage the sbdf local variable we
> >> now have in the function. Yet of course the primary question is: Is this
> >> an okay game to play in the first place?
> > 
> > I've wondered the same, while this might be obfuscated enough for
> > Eclair to complain, aren't we still violating the spirit of the rule?
> 
> We do, but what do you do without dropping that "const" (which I'd really
> like to keep), and without C++ concepts of initialization?

Is it possible to "tag" this with a comment?  Noting we are aware of
the MISRA violation, but the result of the field being const outweigh
the violation.

At the end the memcpy() is also a violation, and hence is likely to
also be flagged by Eclair in the future - it might be best to simply
come clean and accept we have an intentional violation here.

Thanks, Roger.

Re: [PATCH 13/14] passthrough/PCI: rework (s,b,d,f) init of struct pci_dev

[Jan Beulich]

On 02.09.2026 17:07, Roger Pau Monné wrote:
> On Wed, Sep 02, 2026 at 02:53:58PM +0200, Jan Beulich wrote:
>> On 02.09.2026 14:44, Roger Pau Monné wrote:
>>> On Wed, Sep 02, 2026 at 08:36:27AM +0200, Jan Beulich wrote:
>>>> Right now we're casting away const-ness, to initialize the individual
>>>> elements despite the field(s) being declared const. Eclair validly
>>>> recognizes this as a Misra rule 11.8 violation. Hide this by switching to
>>>> the use of memcpy(), deriving the destination address from the (mutable)
>>>> struct pci_dev * which we hold in hands.
>>>>
>>>> No functional change intended.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> Subsequently we may want to further leverage the sbdf local variable we
>>>> now have in the function. Yet of course the primary question is: Is this
>>>> an okay game to play in the first place?
>>>
>>> I've wondered the same, while this might be obfuscated enough for
>>> Eclair to complain, aren't we still violating the spirit of the rule?
>>
>> We do, but what do you do without dropping that "const" (which I'd really
>> like to keep), and without C++ concepts of initialization?
> 
> Is it possible to "tag" this with a comment?  Noting we are aware of
> the MISRA violation, but the result of the field being const outweigh
> the violation.

Surely we could make up a SAF comment for this, just that SAF comments
aren't liked by everyone either (personally I consider them ugly, but
kind of tolerable).

Jan

Re: [PATCH 08/14] Arm/guestcopy: deviate copy_guest() uses just like their x86 counterparts

[Orzel, Michal]

On 02-Sep-26 08:33, Jan Beulich wrote:
> Like x86/HVM's __hvm_copy(), Arm's copy_guest() is used for both to-guest
> and from-guest copying. Naturally in the latter case the hypervisor buffer
> needs writing to, hence the function parameter cannot be pointer-to-const.
> 
> No functional change intended.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH 05/14] x86/altcall: hide casting away of const

[Roger Pau Monné]

On Wed, Sep 02, 2026 at 08:32:09AM +0200, Jan Beulich wrote:
> ALT_CALL_PTR() really means to get rid of const (when necessary, i.e. when
> used from apply_alt_calls()): We want to patch the original call site,
> after all, and memory-wise that's unrelated to the struct alt_call * that
> we start from.
> 
> No functional change intended.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger@xenproject.org>

Thanks, Roger.

Re: [PATCH 04/14] x86/boot: don't cast away const-ness

[Roger Pau Monné]

On Wed, Sep 02, 2026 at 08:31:45AM +0200, Jan Beulich wrote:
> While the general C library strchr() has to do so, the early boot cmdline
> parsing has no such requirement.
> 
> No functional change.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger@xenproject.org>

I won't usually agree to changing the interface of C library
functions, but this is limited enough to early boot cmdline parsing
that's IMO OK.  It might be good to rename the function so it doesn't
alias with the C library namespace now, since it's a different
interface.

Thanks, Roger.