On 12.07.2023 12:32, Simone Ballarin wrote:
> @@ -378,10 +378,10 @@ static void __init calculate_host_policy(void)
> * this information.
> */
> if ( cpu_has_lfence_dispatch )
> - max_extd_leaf = max(max_extd_leaf, 0x80000021);
> + max_extd_leaf = max(max_extd_leaf, 0x80000021U);
>
> - p->extd.max_leaf = 0x80000000 | min_t(uint32_t, max_extd_leaf & 0xffff,
> - ARRAY_SIZE(p->extd.raw) - 1);
> + p->extd.max_leaf = 0x80000000U | min_t(uint32_t, max_extd_leaf & 0xffffU,
> + ARRAY_SIZE(p->extd.raw) - 1);
Why the 2nd of the U additions? I ask in particular because ...
> @@ -768,11 +768,11 @@ void recalculate_cpuid_policy(struct domain *d)
>
> p->basic.max_leaf = min(p->basic.max_leaf, max->basic.max_leaf);
> p->feat.max_subleaf = min(p->feat.max_subleaf, max->feat.max_subleaf);
> - p->extd.max_leaf = 0x80000000 | min(p->extd.max_leaf & 0xffff,
> - ((p->x86_vendor & (X86_VENDOR_AMD
> |
> -
> X86_VENDOR_HYGON))
> - ? CPUID_GUEST_NR_EXTD_AMD
> - : CPUID_GUEST_NR_EXTD_INTEL) -
> 1);
> + p->extd.max_leaf = 0x80000000U | min(p->extd.max_leaf & 0xffff,
> + ((p->x86_vendor &
> (X86_VENDOR_AMD |
> +
> X86_VENDOR_HYGON))
> + ? CPUID_GUEST_NR_EXTD_AMD
> + : CPUID_GUEST_NR_EXTD_INTEL) -
> 1);
... you don't do the same here (or else you would have to further
switch to e.g. using 1u, to please min()'s type checking).
Just to mention it (I think that U wants dropping for now), in the
earlier case if you switched to UL, you/we would be able to switch
from min_t() to the type-safe min().
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -383,7 +383,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr,
> uint32_t data)
>
> if ( !vector )
> {
> - int pirq = ((addr >> 32) & 0xffffff00) | dest;
> + int pirq = ((addr >> 32) & 0xffffff00U) | dest;
This is fishy, not just because of the use of plain int, but even more
so ...
> if ( pirq > 0 )
> {
... with this following. It leaves unclear what negative values would
mean. I think pirq wants to be unsigned int (pirq_info() expects it
this way, albeit far from all of its invocations adhere to this rule),
but the situation isn't helped by XEN_DMOP_inject_msi not having any
comment whatsoever on the meaning of the upper half of the uint64_t
addr field that's being passed in.
I'm inclined to omit this hunk for now. I'll look around some more,
and if I can come to a sensible conclusion I may then submit a patch
just for this.
> --- a/xen/arch/x86/hvm/stdvga.c
> +++ b/xen/arch/x86/hvm/stdvga.c
> @@ -39,22 +39,22 @@
>
> #define PAT(x) (x)
> static const uint32_t mask16[16] = {
> - PAT(0x00000000),
> - PAT(0x000000ff),
> - PAT(0x0000ff00),
> - PAT(0x0000ffff),
> - PAT(0x00ff0000),
> - PAT(0x00ff00ff),
> - PAT(0x00ffff00),
> - PAT(0x00ffffff),
> - PAT(0xff000000),
> - PAT(0xff0000ff),
> - PAT(0xff00ff00),
> - PAT(0xff00ffff),
> - PAT(0xffff0000),
> - PAT(0xffff00ff),
> - PAT(0xffffff00),
> - PAT(0xffffffff),
> + PAT(0x00000000U),
> + PAT(0x000000ffU),
> + PAT(0x0000ff00U),
> + PAT(0x0000ffffU),
> + PAT(0x00ff0000U),
> + PAT(0x00ff00ffU),
> + PAT(0x00ffff00U),
> + PAT(0x00ffffffU),
> + PAT(0xff000000U),
> + PAT(0xff0000ffU),
> + PAT(0xff00ff00U),
> + PAT(0xff00ffffU),
> + PAT(0xffff0000U),
> + PAT(0xffff00ffU),
> + PAT(0xffffff00U),
> + PAT(0xffffffffU),
> };
>
> /* force some bits to zero */
> @@ -70,15 +70,15 @@ static const uint8_t sr_mask[8] = {
> };
>
> static const uint8_t gr_mask[9] = {
> - (uint8_t)~0xf0, /* 0x00 */
> - (uint8_t)~0xf0, /* 0x01 */
> - (uint8_t)~0xf0, /* 0x02 */
> - (uint8_t)~0xe0, /* 0x03 */
> - (uint8_t)~0xfc, /* 0x04 */
> - (uint8_t)~0x84, /* 0x05 */
> - (uint8_t)~0xf0, /* 0x06 */
> - (uint8_t)~0xf0, /* 0x07 */
> - (uint8_t)~0x00, /* 0x08 */
> + (uint8_t)~0xf0,
> + (uint8_t)~0xf0,
> + (uint8_t)~0xf0,
> + (uint8_t)~0xe0,
> + (uint8_t)~0xfc,
> + (uint8_t)~0x84,
> + (uint8_t)~0xf0,
> + (uint8_t)~0xf0,
> + (uint8_t)~0x00,
> };
The changelog for v3 says you did drop the changes to this array.
> --- a/xen/arch/x86/include/asm/hvm/trace.h
> +++ b/xen/arch/x86/include/asm/hvm/trace.h
> @@ -58,7 +58,7 @@
> #define DO_TRC_HVM_VLAPIC DEFAULT_HVM_MISC
>
>
> -#define TRC_PAR_LONG(par) ((par)&0xFFFFFFFF),((par)>>32)
> +#define TRC_PAR_LONG(par) ((par) & 0xFFFFFFFFU), ((par) >> 32)
Perhaps again better to switch to a uint32_t cast on the lhs of the comma.
> @@ -93,7 +93,7 @@
> HVMTRACE_ND(evt, 0, 0)
>
> #define HVMTRACE_LONG_1D(evt, d1) \
> - HVMTRACE_2D(evt ## 64, (d1) & 0xFFFFFFFF, (d1) >> 32)
> + HVMTRACE_2D(evt ## 64, (d1) & 0xFFFFFFFFU, (d1) >> 32)
Same here then.
> /* K6 MSRs */
> -#define MSR_K6_EFER 0xc0000080
> -#define MSR_K6_STAR 0xc0000081
> -#define MSR_K6_WHCR 0xc0000082
> -#define MSR_K6_UWCCR 0xc0000085
> -#define MSR_K6_EPMR 0xc0000086
> -#define MSR_K6_PSOR 0xc0000087
> -#define MSR_K6_PFIR 0xc0000088
> +#define MSR_K6_EFER 0xc0000080U
> +#define MSR_K6_STAR 0xc0000081U
> +#define MSR_K6_WHCR 0xc0000082U
> +#define MSR_K6_UWCCR 0xc0000085U
> +#define MSR_K6_EPMR 0xc0000086U
> +#define MSR_K6_PSOR 0xc0000087U
> +#define MSR_K6_PFIR 0xc0000088U
This entire block can be dropped rather than adjusted; there are no uses of
these constants. I expect they're a remnant of 32-bit Xen, which has been
gone for a decade.
> @@ -459,10 +459,10 @@
> #define MSR_VIA_BCR2 0x00001147
>
> /* Transmeta defined MSRs */
> -#define MSR_TMTA_LONGRUN_CTRL 0x80868010
> -#define MSR_TMTA_LONGRUN_FLAGS 0x80868011
> -#define MSR_TMTA_LRTI_READOUT 0x80868018
> -#define MSR_TMTA_LRTI_VOLT_MHZ 0x8086801a
> +#define MSR_TMTA_LONGRUN_CTRL 0x80868010U
> +#define MSR_TMTA_LONGRUN_FLAGS 0x80868011U
> +#define MSR_TMTA_LRTI_READOUT 0x80868018U
> +#define MSR_TMTA_LRTI_VOLT_MHZ 0x8086801aU
Same here.
> --- a/xen/arch/x86/x86_64/acpi_mmcfg.c
> +++ b/xen/arch/x86/x86_64/acpi_mmcfg.c
> @@ -50,7 +50,7 @@ static int __init acpi_mcfg_check_entry(struct
> acpi_table_mcfg *mcfg,
> {
> int year;
>
> - if (cfg->address < 0xFFFFFFFF)
> + if (cfg->address < 0xFFFFFFFFU)
> return 0;
This check is bogus and would better be corrected. Such a correction
would presumably deal with the Misra violation at the same time.
Jan
