On Mon, Jul 29, 2024 at 12:30 PM Jan Beulich <jbeul...@suse.com> wrote:
> On 25.07.2024 10:27, Fouad Hilly wrote:
> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -2650,7 +2650,7 @@ performance.
> > Alternatively, selecting `tsx=1` will re-enable TSX at the users own
> risk.
> >
> > ### ucode
> > -> `= List of [ <integer> | scan=<bool>, nmi=<bool>, allow-same=<bool> ]`
> > +> `= List of [ <integer> | scan=<bool>, nmi=<bool> ]`
> >
> > Applicability: x86
> > Default: `nmi`
> > @@ -2682,11 +2682,6 @@ precedence over `scan`.
> > stop_machine context. In NMI handler, even NMIs are blocked, which is
> > considered safer. The default value is `true`.
> >
> > -'allow-same' alters the default acceptance policy for new microcode to
> permit
> > -trying to reload the same version. Many CPUs will actually reload
> microcode
> > -of the same version, and this allows for easy testing of the late
> microcode
> > -loading path.
>
> The removal of a command line (sub)option should come with a CHANGELOG.md
> entry.
>
Will be fixed in v7
>
> > --- a/xen/arch/x86/cpu/microcode/core.c
> > +++ b/xen/arch/x86/cpu/microcode/core.c
> > @@ -90,6 +90,11 @@ struct ucode_mod_blob {
> > size_t size;
> > };
> >
> > +struct patch_with_flags {
> > + unsigned int flags;
> > + struct microcode_patch *patch;
>
> Pointer-to-const? If the const was omitted here just because of
> microcode_free_patch(), then I think the issue should be taken care
> of there.
>
This struct is required as is, I initially added a similar struct with
const (which was removed in v6).
updated control_thread_fn()
-static int control_thread_fn(const struct microcode_patch *patch)
+static int control_thread_fn(struct microcode_patch *patch,
+ unsigned int flags)
> @@ -237,7 +238,11 @@ static DEFINE_PER_CPU(int, loading_err);
> > */
> > static cpumask_t cpu_callin_map;
> > static atomic_t cpu_out, cpu_updated;
> > -static const struct microcode_patch *nmi_patch = ZERO_BLOCK_PTR;
> > +static struct patch_with_flags nmi_patch_with_flags =
>
> Could the variable name perhaps continue to be "nmi_patch"? Or be
> simply "nmi_arg" or some such?
>
Sure, will keep it as nmi_patch
>
> > +{
> > + .flags = 0,
>
> Nit: This isn't really needed.
>
> > @@ -379,7 +386,8 @@ static int secondary_nmi_work(void)
> > return wait_for_state(LOADING_EXIT) ? 0 : -EBUSY;
> > }
> >
> > -static int primary_thread_work(const struct microcode_patch *patch)
> > +static int primary_thread_work(struct microcode_patch *patch,
> > + unsigned int flags)
> > {
>
> Why is this change needed?
>
This will be reverted in v7
>
> > @@ -446,7 +455,8 @@ static int secondary_thread_fn(void)
> > return this_cpu(loading_err);
> > }
> >
> > -static int primary_thread_fn(const struct microcode_patch *patch)
> > +static int primary_thread_fn(struct microcode_patch *patch,
> > + unsigned int flags)
>
> Same here.
>
This will be reverted in v7
>
> Jan
>
Thanks,
Fouad
