Il giorno gio 6 lug 2023 alle ore 10:04 Jan Beulich <jbeul...@suse.com> ha
scritto:
> On 05.07.2023 17:26, Simone Ballarin wrote:
> > --- a/xen/arch/x86/hvm/vmx/vvmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> > @@ -257,14 +257,14 @@ uint64_t get_vvmcs_virtual(void *vvmcs, uint32_t
> vmcs_encoding)
> >
> > switch ( enc.width ) {
> > case VVMCS_WIDTH_16:
> > - res &= 0xffff;
> > + res &= 0xffffU;
>
> I don't think the suffix is needed in cases like this one, and ...
>
> > break;
> > case VVMCS_WIDTH_64:
> > if ( enc.access_type )
> > res >>= 32;
> > break;
> > case VVMCS_WIDTH_32:
> > - res &= 0xffffffff;
> > + res &= 0xffffffffU;
>
> ... while generally I'm suggesting to avoid casts I wonder whether
> casting to uint32_t here wouldn't make things more obviously match
> the purpose. (Same again further down then.)
>
Using the cast is fine. I will change the patch.
>
> > --- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> > +++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> > @@ -207,7 +207,7 @@ void vmx_vmcs_reload(struct vcpu *v);
> > #define CPU_BASED_ACTIVATE_MSR_BITMAP 0x10000000
> > #define CPU_BASED_MONITOR_EXITING 0x20000000
> > #define CPU_BASED_PAUSE_EXITING 0x40000000
> > -#define CPU_BASED_ACTIVATE_SECONDARY_CONTROLS 0x80000000
> > +#define CPU_BASED_ACTIVATE_SECONDARY_CONTROLS 0x80000000U
>
> Interesting - you don't change adjacent #define-s here, nor ...
>
> > @@ -257,7 +257,7 @@ extern u32 vmx_vmentry_control;
> > #define SECONDARY_EXEC_XSAVES 0x00100000
> > #define SECONDARY_EXEC_TSC_SCALING 0x02000000
> > #define SECONDARY_EXEC_BUS_LOCK_DETECTION 0x40000000
> > -#define SECONDARY_EXEC_NOTIFY_VM_EXITING 0x80000000
> > +#define SECONDARY_EXEC_NOTIFY_VM_EXITING 0x80000000U
>
> ... here. May I ask why that is? (I'm not opposed, but the
> description suggests otherwise.)
>
> > --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> > +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> > @@ -136,7 +136,7 @@ static inline void pi_clear_sn(struct pi_desc
> *pi_desc)
> > /*
> > * Exit Reasons
> > */
> > -#define VMX_EXIT_REASONS_FAILED_VMENTRY 0x80000000
> > +#define VMX_EXIT_REASONS_FAILED_VMENTRY 0x80000000U
> > #define VMX_EXIT_REASONS_BUS_LOCK (1u << 26)
>
> Along the lines of the latter, perhaps switch to 1u << 31?
>
> > @@ -246,15 +246,15 @@ typedef union cr_access_qual {
> > /*
> > * Access Rights
> > */
> > -#define X86_SEG_AR_SEG_TYPE 0xf /* 3:0, segment type */
> > -#define X86_SEG_AR_DESC_TYPE (1u << 4) /* 4, descriptor type */
> > -#define X86_SEG_AR_DPL 0x60 /* 6:5, descriptor privilege
> level */
> > -#define X86_SEG_AR_SEG_PRESENT (1u << 7) /* 7, segment present */
> > -#define X86_SEG_AR_AVL (1u << 12) /* 12, available for system
> software */
> > -#define X86_SEG_AR_CS_LM_ACTIVE (1u << 13) /* 13, long mode active (CS
> only) */
> > -#define X86_SEG_AR_DEF_OP_SIZE (1u << 14) /* 14, default operation
> size */
> > -#define X86_SEG_AR_GRANULARITY (1u << 15) /* 15, granularity */
> > -#define X86_SEG_AR_SEG_UNUSABLE (1u << 16) /* 16, segment unusable */
> > +#define X86_SEG_AR_SEG_TYPE 0xfU /* 3:0, segment type */
> > +#define X86_SEG_AR_DESC_TYPE (1U << 4) /* 4, descriptor type */
> > +#define X86_SEG_AR_DPL 0x60U /* 6:5, descriptor privilege
> level */
> > +#define X86_SEG_AR_SEG_PRESENT (1U << 7) /* 7, segment present */
> > +#define X86_SEG_AR_AVL (1U << 12) /* 12, available for system
> software */
> > +#define X86_SEG_AR_CS_LM_ACTIVE (1U << 13) /* 13, long mode active (CS
> only) */
> > +#define X86_SEG_AR_DEF_OP_SIZE (1U << 14) /* 14, default operation
> size */
> > +#define X86_SEG_AR_GRANULARITY (1U << 15) /* 15, granularity */
> > +#define X86_SEG_AR_SEG_UNUSABLE (1U << 16) /* 16, segment unusable */
>
> How is this change related to rule 7.2? There are u suffixes already where
> needed (and 0xf and 0x60 don't strictly need one), so there's no violation
> here afaict. A mere style change to switch from u to U imo doesn't belong
> here (and, as mentioned while discussing the rule, is imo hampering
> readability in cases like these ones).
>
> Jan
>
In general, I will remove all non-strictly required U's you have pointed
out.
I will also amend the commit messages in these cases.
--
Simone Ballarin, M.Sc.
Field Application Engineer, BUGSENG (https://bugseng.com
<http://bugseng.com>)
