Re: [XEN PATCH v4 3/4] xen/x86: address violations of MISRA C:2012 Rule 13.1

[Simone Ballarin <simone.ballarin@xxxxxxxxxxx>]

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@xxxxxxxxxxx>
> 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,
> >           },
--
Simone Ballarin, M.Sc.

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