| Message ID | 16bb514ac0a5fe0d6e9a2c95279a8200ff4495c6.1706886631.git.simone.ballarin@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | address violation of MISRA C:2012 Rule 13.1 | expand |
On 02.02.2024 16:16, Simone Ballarin wrote: > Rule 13.1: Initializer lists shall not contain persistent side effects > > This patch moves expressions with side-effects into new variables before > the initializer lists. > > No functional changes. > > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> To be honest, I don't like this. It's more code for no gain. Really its hampering clarity imo. I'm willing to be convinced otherwise, but for now this gets a nack from me. As an aside, ... > --- a/xen/arch/x86/io_apic.c > +++ b/xen/arch/x86/io_apic.c > @@ -2559,9 +2559,12 @@ integer_param("max_gsi_irqs", max_gsi_irqs); > > static __init bool bad_ioapic_register(unsigned int idx) > { > - union IO_APIC_reg_00 reg_00 = { .raw = io_apic_read(idx, 0) }; > - union IO_APIC_reg_01 reg_01 = { .raw = io_apic_read(idx, 1) }; > - union IO_APIC_reg_02 reg_02 = { .raw = io_apic_read(idx, 2) }; > + uint32_t reg_00_raw = io_apic_read(idx, 0); > + uint32_t reg_01_raw = io_apic_read(idx, 1); > + uint32_t reg_02_raw = io_apic_read(idx, 2); ... while you properly use uint32_t here, ... > + union IO_APIC_reg_00 reg_00 = { .raw = reg_00_raw }; > + union IO_APIC_reg_01 reg_01 = { .raw = reg_01_raw }; > + union IO_APIC_reg_02 reg_02 = { .raw = reg_02_raw }; > > if ( reg_00.raw == -1 && reg_01.raw == -1 && reg_02.raw == -1 ) > { > --- a/xen/arch/x86/mpparse.c > +++ b/xen/arch/x86/mpparse.c > @@ -798,11 +798,12 @@ void __init mp_register_lapic_address ( > > int mp_register_lapic(u32 id, bool enabled, bool hotplug) > { > + u32 apic = apic_read(APIC_LVR); ... why the being-phased-out u32 here? Jan > struct mpc_config_processor processor = { > .mpc_type = MP_PROCESSOR, > /* Note: We don't fill in fields not consumed anywhere. */ > .mpc_apicid = id, > - .mpc_apicver = GET_APIC_VERSION(apic_read(APIC_LVR)), > + .mpc_apicver = GET_APIC_VERSION(apic), > .mpc_cpuflag = (enabled ? CPU_ENABLED : 0) | > (id == boot_cpu_physical_apicid ? > CPU_BOOTPROCESSOR : 0), > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -885,13 +885,14 @@ static struct domain *__init create_dom0(const module_t *image, > { > static char __initdata cmdline[MAX_GUEST_CMDLINE]; > > + unsigned int max_vcpus = dom0_max_vcpus(); > struct xen_domctl_createdomain dom0_cfg = { > .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0, > .max_evtchn_port = -1, > .max_grant_frames = -1, > .max_maptrack_frames = -1, > .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version), > - .max_vcpus = dom0_max_vcpus(), > + .max_vcpus = max_vcpus, > .arch = { > .misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0, > },
On Tue, 6 Feb 2024, Jan Beulich wrote: > On 02.02.2024 16:16, Simone Ballarin wrote: > > Rule 13.1: Initializer lists shall not contain persistent side effects > > > > This patch moves expressions with side-effects into new variables before > > the initializer lists. > > > > No functional changes. > > > > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> > > To be honest, I don't like this. It's more code for no gain. Really its > hampering clarity imo. I'm willing to be convinced otherwise, but for > now this gets a nack from me. Which part of the patch you don't like? The changes to bad_ioapic_register? > As an aside, ... > > > --- a/xen/arch/x86/io_apic.c > > +++ b/xen/arch/x86/io_apic.c > > @@ -2559,9 +2559,12 @@ integer_param("max_gsi_irqs", max_gsi_irqs); > > > > static __init bool bad_ioapic_register(unsigned int idx) > > { > > - union IO_APIC_reg_00 reg_00 = { .raw = io_apic_read(idx, 0) }; > > - union IO_APIC_reg_01 reg_01 = { .raw = io_apic_read(idx, 1) }; > > - union IO_APIC_reg_02 reg_02 = { .raw = io_apic_read(idx, 2) }; > > + uint32_t reg_00_raw = io_apic_read(idx, 0); > > + uint32_t reg_01_raw = io_apic_read(idx, 1); > > + uint32_t reg_02_raw = io_apic_read(idx, 2); > > ... while you properly use uint32_t here, ... > > > + union IO_APIC_reg_00 reg_00 = { .raw = reg_00_raw }; > > + union IO_APIC_reg_01 reg_01 = { .raw = reg_01_raw }; > > + union IO_APIC_reg_02 reg_02 = { .raw = reg_02_raw }; > > > > if ( reg_00.raw == -1 && reg_01.raw == -1 && reg_02.raw == -1 ) > > { > > --- a/xen/arch/x86/mpparse.c > > +++ b/xen/arch/x86/mpparse.c > > @@ -798,11 +798,12 @@ void __init mp_register_lapic_address ( > > > > int mp_register_lapic(u32 id, bool enabled, bool hotplug) > > { > > + u32 apic = apic_read(APIC_LVR); > > ... why the being-phased-out u32 here? > > Jan > > > struct mpc_config_processor processor = { > > .mpc_type = MP_PROCESSOR, > > /* Note: We don't fill in fields not consumed anywhere. */ > > .mpc_apicid = id, > > - .mpc_apicver = GET_APIC_VERSION(apic_read(APIC_LVR)), > > + .mpc_apicver = GET_APIC_VERSION(apic), > > .mpc_cpuflag = (enabled ? CPU_ENABLED : 0) | > > (id == boot_cpu_physical_apicid ? > > CPU_BOOTPROCESSOR : 0), > > --- a/xen/arch/x86/setup.c > > +++ b/xen/arch/x86/setup.c > > @@ -885,13 +885,14 @@ static struct domain *__init create_dom0(const module_t *image, > > { > > static char __initdata cmdline[MAX_GUEST_CMDLINE]; > > > > + unsigned int max_vcpus = dom0_max_vcpus(); > > struct xen_domctl_createdomain dom0_cfg = { > > .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0, > > .max_evtchn_port = -1, > > .max_grant_frames = -1, > > .max_maptrack_frames = -1, > > .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version), > > - .max_vcpus = dom0_max_vcpus(), > > + .max_vcpus = max_vcpus, > > .arch = { > > .misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0, > > }, >
On 07.02.2024 01:56, Stefano Stabellini wrote: > On Tue, 6 Feb 2024, Jan Beulich wrote: >> On 02.02.2024 16:16, Simone Ballarin wrote: >>> Rule 13.1: Initializer lists shall not contain persistent side effects >>> >>> This patch moves expressions with side-effects into new variables before >>> the initializer lists. >>> >>> No functional changes. >>> >>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> >> >> To be honest, I don't like this. It's more code for no gain. Really its >> hampering clarity imo. I'm willing to be convinced otherwise, but for >> now this gets a nack from me. > > Which part of the patch you don't like? The changes to > bad_ioapic_register? Really all of them. bad_ioapic_register() is merely worst. Jan
On 06/02/24 14:13, Jan Beulich wrote: > On 02.02.2024 16:16, Simone Ballarin wrote: >> Rule 13.1: Initializer lists shall not contain persistent side effects >> >> This patch moves expressions with side-effects into new variables before >> the initializer lists. >> >> No functional changes. >> >> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> > > To be honest, I don't like this. It's more code for no gain. Really its > hampering clarity imo. I'm willing to be convinced otherwise, but for > now this gets a nack from me. > > As an aside, ... > >> --- a/xen/arch/x86/io_apic.c >> +++ b/xen/arch/x86/io_apic.c >> @@ -2559,9 +2559,12 @@ integer_param("max_gsi_irqs", max_gsi_irqs); >> >> static __init bool bad_ioapic_register(unsigned int idx) >> { >> - union IO_APIC_reg_00 reg_00 = { .raw = io_apic_read(idx, 0) }; >> - union IO_APIC_reg_01 reg_01 = { .raw = io_apic_read(idx, 1) }; >> - union IO_APIC_reg_02 reg_02 = { .raw = io_apic_read(idx, 2) }; >> + uint32_t reg_00_raw = io_apic_read(idx, 0); >> + uint32_t reg_01_raw = io_apic_read(idx, 1); >> + uint32_t reg_02_raw = io_apic_read(idx, 2); I'll propose a deviation for these single item initializers. > > ... while you properly use uint32_t here, ... > >> + union IO_APIC_reg_00 reg_00 = { .raw = reg_00_raw }; >> + union IO_APIC_reg_01 reg_01 = { .raw = reg_01_raw }; >> + union IO_APIC_reg_02 reg_02 = { .raw = reg_02_raw }; >> >> if ( reg_00.raw == -1 && reg_01.raw == -1 && reg_02.raw == -1 ) >> { >> --- a/xen/arch/x86/mpparse.c >> +++ b/xen/arch/x86/mpparse.c >> @@ -798,11 +798,12 @@ void __init mp_register_lapic_address ( >> >> int mp_register_lapic(u32 id, bool enabled, bool hotplug) >> { >> + u32 apic = apic_read(APIC_LVR);> > ... why the being-phased-out u32 here? > It was just to be coherent with the type of id. I will use uint32_t in the next submission. > Jan > >> struct mpc_config_processor processor = { >> .mpc_type = MP_PROCESSOR, >> /* Note: We don't fill in fields not consumed anywhere. */ >> .mpc_apicid = id, >> - .mpc_apicver = GET_APIC_VERSION(apic_read(APIC_LVR)), >> + .mpc_apicver = GET_APIC_VERSION(apic), >> .mpc_cpuflag = (enabled ? CPU_ENABLED : 0) | >> (id == boot_cpu_physical_apicid ? >> CPU_BOOTPROCESSOR : 0), >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -885,13 +885,14 @@ static struct domain *__init create_dom0(const module_t *image, >> { >> static char __initdata cmdline[MAX_GUEST_CMDLINE]; >> >> + unsigned int max_vcpus = dom0_max_vcpus(); >> struct xen_domctl_createdomain dom0_cfg = { >> .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0, >> .max_evtchn_port = -1, >> .max_grant_frames = -1, >> .max_maptrack_frames = -1, >> .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version), >> - .max_vcpus = dom0_max_vcpus(), >> + .max_vcpus = max_vcpus, >> .arch = { >> .misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0, >> }, >
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c index b48a642465..4a6ab85689 100644 --- a/xen/arch/x86/io_apic.c +++ b/xen/arch/x86/io_apic.c @@ -2559,9 +2559,12 @@ integer_param("max_gsi_irqs", max_gsi_irqs); static __init bool bad_ioapic_register(unsigned int idx) { - union IO_APIC_reg_00 reg_00 = { .raw = io_apic_read(idx, 0) }; - union IO_APIC_reg_01 reg_01 = { .raw = io_apic_read(idx, 1) }; - union IO_APIC_reg_02 reg_02 = { .raw = io_apic_read(idx, 2) }; + uint32_t reg_00_raw = io_apic_read(idx, 0); + uint32_t reg_01_raw = io_apic_read(idx, 1); + uint32_t reg_02_raw = io_apic_read(idx, 2); + union IO_APIC_reg_00 reg_00 = { .raw = reg_00_raw }; + union IO_APIC_reg_01 reg_01 = { .raw = reg_01_raw }; + union IO_APIC_reg_02 reg_02 = { .raw = reg_02_raw }; if ( reg_00.raw == -1 && reg_01.raw == -1 && reg_02.raw == -1 ) { diff --git a/xen/arch/x86/mpparse.c b/xen/arch/x86/mpparse.c index d8ccab2449..81a819403b 100644 --- a/xen/arch/x86/mpparse.c +++ b/xen/arch/x86/mpparse.c @@ -798,11 +798,12 @@ void __init mp_register_lapic_address ( int mp_register_lapic(u32 id, bool enabled, bool hotplug) { + u32 apic = apic_read(APIC_LVR); struct mpc_config_processor processor = { .mpc_type = MP_PROCESSOR, /* Note: We don't fill in fields not consumed anywhere. */ .mpc_apicid = id, - .mpc_apicver = GET_APIC_VERSION(apic_read(APIC_LVR)), + .mpc_apicver = GET_APIC_VERSION(apic), .mpc_cpuflag = (enabled ? CPU_ENABLED : 0) | (id == boot_cpu_physical_apicid ? CPU_BOOTPROCESSOR : 0), diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index ee682dd136..886031d86a 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -885,13 +885,14 @@ static struct domain *__init create_dom0(const module_t *image, { static char __initdata cmdline[MAX_GUEST_CMDLINE]; + unsigned int max_vcpus = dom0_max_vcpus(); struct xen_domctl_createdomain dom0_cfg = { .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0, .max_evtchn_port = -1, .max_grant_frames = -1, .max_maptrack_frames = -1, .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version), - .max_vcpus = dom0_max_vcpus(), + .max_vcpus = max_vcpus, .arch = { .misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0, },
Rule 13.1: Initializer lists shall not contain persistent side effects This patch moves expressions with side-effects into new variables before the initializer lists. No functional changes. Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> --- xen/arch/x86/io_apic.c | 9 ++++++--- xen/arch/x86/mpparse.c | 3 ++- xen/arch/x86/setup.c | 3 ++- 3 files changed, 10 insertions(+), 5 deletions(-)