Re: [PATCH v5 4/4] x86/ucode: Utilize ucode_force and remove opt_ucode_allow_same

[Fouad Hilly <fouad.hilly@xxxxxxxxx>]

On Tue, Jul 16, 2024 at 3:59 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 12.07.2024 15:07, Fouad Hilly wrote:
> > --- a/xen/arch/x86/cpu/microcode/core.c
> > +++ b/xen/arch/x86/cpu/microcode/core.c
> > @@ -90,6 +90,16 @@ struct ucode_mod_blob {
> >      size_t size;
> >  };
> >
> > +struct microcode_patch_with_flags {
> > +    unsigned int flags;
> > +    struct microcode_patch *patch;
> > +};
> > +
> > +struct microcode_nmi_patch_with_flags {
> > +    unsigned int flags;
> > +    const struct microcode_patch *patch;
> > +};
>
> Why two different structures? I have to admit that I can't spot where the
> difference (const or not) would matter?
I will remove the struct with const and update:
static int control_thread_fn(const struct microcode_patch *patch,
                             unsigned int flags)
>
> Also for an internal struct I don't think you need the microcode_ prefixes.

I will remove microcode_prefixes.
>
> > @@ -284,21 +286,22 @@ static enum microcode_match_result cf_check compare_patch(
> >      return compare_revisions(old->rev, new->rev);
> >  }
> >
> > -static int cf_check apply_microcode(const struct microcode_patch *patch)
> > +static int cf_check apply_microcode(const struct microcode_patch *patch,
> > +                                    unsigned int flags)
> >  {
> >      uint64_t msr_content;
> >      unsigned int cpu = smp_processor_id();
> >      struct cpu_signature *sig = &this_cpu(cpu_sig);
> >      uint32_t rev, old_rev = sig->rev;
> >      enum microcode_match_result result;
> > +    bool ucode_force = flags == XENPF_UCODE_FORCE;
>
> Why == ? The term "flags" usually stands for there being multiple boolean
> indicators in a single value. That would demand use of & here.

Will be fixed in v6
>
> Jan

Thanks,

Fouad