Il giorno gio 6 lug 2023 alle ore 10:26 Jan Beulich <jbeul...@suse.com> 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
<http://bugseng.com>)
