Xen project Mailing List

Il giorno gio 6 lug 2023 alle ore 10:26 Jan Beulich <jbeulich@xxxxxxxx> ha scritto:

On 05.07.2023 17:26, Simone Ballarin wrote:
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -1211,7 +1211,7 @@ static void __init calibrate_APIC_clock(void)
> * Setup the APIC counter to maximum. There is no way the lapic
> * can underflow in the 100ms detection time frame.
> */
> - __setup_APIC_LVTT(0xffffffff);
> + __setup_APIC_LVTT(0xffffffffU);

While making the change less mechanical, we want to consider to switch
to ~0 in this and similar cases.

Changing ~0U is more than not mechanical: it is possibly dangerous.

The resulting value could be different depending on the architecture,

I prefer to not make such kind of changes in a MISRA-related patch.

> @@ -378,9 +378,9 @@ 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,
> + p->extd.max_leaf = 0x80000000U | min_t(uint32_t, max_extd_leaf & 0xffffU,
> ARRAY_SIZE(p->extd.raw) - 1);

Adjustments like this or ...

> @@ -768,7 +768,7 @@ 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->extd.max_leaf = 0x80000000U | min(p->extd.max_leaf & 0xffff,
> ((p->x86_vendor & (X86_VENDOR_AMD |
> X86_VENDOR_HYGON))
> ? CPUID_GUEST_NR_EXTD_AMD

... this also need to adjust indentation of the following lines.

Ok.

> --- a/xen/arch/x86/cpu/mcheck/mce-apei.c
> +++ b/xen/arch/x86/cpu/mcheck/mce-apei.c
> @@ -37,11 +37,11 @@
> #include "mce.h"
>
> #define CPER_CREATOR_MCE \
> - UUID_LE(0x75a574e3, 0x5052, 0x4b29, 0x8a, 0x8e, 0xbe, 0x2c, \
> - 0x64, 0x90, 0xb8, 0x9d)
> + UUID_LE(0x75a574e3U, 0x5052U, 0x4b29U, 0x8aU, 0x8eU, 0xbeU, 0x2cU, \
> + 0x64U, 0x90U, 0xb8U, 0x9dU)
> #define CPER_SECTION_TYPE_MCE \
> - UUID_LE(0xfe08ffbe, 0x95e4, 0x4be7, 0xbc, 0x73, 0x40, 0x96, \
> - 0x04, 0x4a, 0x38, 0xfc)
> + UUID_LE(0xfe08ffbeU, 0x95e4U, 0x4be7U, 0xbcU, 0x73U, 0x40U, 0x96U, \
> + 0x04U, 0x4aU, 0x38U, 0xfcU)

See the earlier (EFI) comment regarding excessive use of suffixes here.

Ok.

> --- a/xen/arch/x86/hvm/stdvga.c
> +++ b/xen/arch/x86/hvm/stdvga.c
> @@ -39,46 +39,46 @@
>
> #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),
> };

While I agree here, ...

> /* force some bits to zero */
> static const uint8_t sr_mask[8] = {
> - (uint8_t)~0xfc,
> - (uint8_t)~0xc2,
> - (uint8_t)~0xf0,
> - (uint8_t)~0xc0,
> - (uint8_t)~0xf1,
> - (uint8_t)~0xff,
> - (uint8_t)~0xff,
> - (uint8_t)~0x00,
> + (uint8_t)~0xfcU,
> + (uint8_t)~0xc2U,
> + (uint8_t)~0xf0U,
> + (uint8_t)~0xc0U,
> + (uint8_t)~0xf1U,
> + (uint8_t)~0xffU,
> + (uint8_t)~0xffU,
> + (uint8_t)~0x00U,
> };
>
> 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)~0xf0U,
> + (uint8_t)~0xf0U,
> + (uint8_t)~0xf0U,
> + (uint8_t)~0xe0U,
> + (uint8_t)~0xfcU,
> + (uint8_t)~0x84U,
> + (uint8_t)~0xf0U,
> + (uint8_t)~0xf0U,
> + (uint8_t)~0x00U,
> };

I continue to question these changes. They don't fix anything, do they?

No, they do not. They were done just for uniformity here.

I will remove the changes in sr_mask and gr_mask.

> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -291,7 +291,7 @@ static void enable_hypercall_page(struct domain *d)
> * calling convention) to differentiate Xen and Viridian hypercalls.
> */
> *(u8 *)(p + 0) = 0x0d; /* orl $0x80000000, %eax */
> - *(u32 *)(p + 1) = 0x80000000;
> + *(u32 *)(p + 1) = 0x80000000U;
> *(u8 *)(p + 5) = 0x0f; /* vmcall/vmmcall */
> *(u8 *)(p + 6) = 0x01;
> *(u8 *)(p + 7) = (cpu_has_vmx ? 0xc1 : 0xd9);

Please can this and ...

> --- a/xen/arch/x86/include/asm/guest/hyperv-tlfs.h
> +++ b/xen/arch/x86/include/asm/guest/hyperv-tlfs.h
> @@ -471,30 +471,30 @@ typedef struct _HV_REFERENCE_TSC_PAGE {
>
> /* Define hypervisor message types. */
> enum hv_message_type {
> - HVMSG_NONE = 0x00000000,
> + HVMSG_NONE = 0x00000000U,
>
> /* Memory access messages. */
> - HVMSG_UNMAPPED_GPA = 0x80000000,
> - HVMSG_GPA_INTERCEPT = 0x80000001,
> + HVMSG_UNMAPPED_GPA = 0x80000000U,
> + HVMSG_GPA_INTERCEPT = 0x80000001U,
>
> /* Timer notification messages. */
> - HVMSG_TIMER_EXPIRED = 0x80000010,
> + HVMSG_TIMER_EXPIRED = 0x80000010U,
>
> /* Error messages. */
> - HVMSG_INVALID_VP_REGISTER_VALUE = 0x80000020,
> - HVMSG_UNRECOVERABLE_EXCEPTION = 0x80000021,
> - HVMSG_UNSUPPORTED_FEATURE = 0x80000022,
> + HVMSG_INVALID_VP_REGISTER_VALUE = 0x80000020U,
> + HVMSG_UNRECOVERABLE_EXCEPTION = 0x80000021U,
> + HVMSG_UNSUPPORTED_FEATURE = 0x80000022U,
>
> /* Trace buffer complete messages. */
> - HVMSG_EVENTLOG_BUFFERCOMPLETE = 0x80000040,
> + HVMSG_EVENTLOG_BUFFERCOMPLETE = 0x80000040U,
>
> /* Platform-specific processor intercept messages. */
> - HVMSG_X64_IOPORT_INTERCEPT = 0x80010000,
> - HVMSG_X64_MSR_INTERCEPT = 0x80010001,
> - HVMSG_X64_CPUID_INTERCEPT = 0x80010002,
> - HVMSG_X64_EXCEPTION_INTERCEPT = 0x80010003,
> - HVMSG_X64_APIC_EOI = 0x80010004,
> - HVMSG_X64_LEGACY_FP_ERROR = 0x80010005
> + HVMSG_X64_IOPORT_INTERCEPT = 0x80010000U,
> + HVMSG_X64_MSR_INTERCEPT = 0x80010001U,
> + HVMSG_X64_CPUID_INTERCEPT = 0x80010002U,
> + HVMSG_X64_EXCEPTION_INTERCEPT = 0x80010003U,
> + HVMSG_X64_APIC_EOI = 0x80010004U,
> + HVMSG_X64_LEGACY_FP_ERROR = 0x80010005U
> };

... this together be made a separate Viridian-specific change? (I
continue to be uncertain about touching the header file; the
Viridian maintainers will need to judge.)

Ok, I will split the patch.

> --- a/xen/arch/x86/include/asm/x86-defns.h
> +++ b/xen/arch/x86/include/asm/x86-defns.h
> @@ -103,7 +103,7 @@
> /*
> * Debug status flags in DR6.
> */
> -#define X86_DR6_DEFAULT 0xffff0ff0 /* Default %dr6 value. */
> +#define X86_DR6_DEFAULT 0xffff0ff0U /* Default %dr6 value. */

Considering the respective register is pointer-/long-size, wouldn't
this better use UL? But of course we'd want to check that this then
doesn't affect code in do_debug() in an undesirable way.

Jan

Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH v2 12/13] xen/x86: fix violations of MISRA C:2012 Rule 7.2