[PATCH v3 12/17] x86: Return mtrr_add_request() to its old purpose

[Bin Meng]

On Mon, May 8, 2023 at 10:48 PM Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Fri, May 5, 2023 at 6:51 AM Simon Glass <sjg at chromium.org> wrote:
> >
> > This function used to be for adding a list of requests to be actioned on
> > relocation. Revert it back to this purpose, to avoid problems with boards
> > which need control of their MTRRs (i.e. those which don't use FSP).
> >
> > The mtrr_set_next_var() function is available when the next free
> > variable-MTRR must be set, so this can be used instead.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > Fixes: 3bcd6cf89ef ("x86: mtrr: Skip MSRs that were already programmed..")
> > Fixes: 596bd0589ad ("x86: mtrr: Do not clear the unused ones..")
> > ---
> >
> > Changes in v3:
> > - Fix invalid commit IDs obtained from another branch
> >
> >  arch/x86/cpu/mtrr.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/cpu/mtrr.c b/arch/x86/cpu/mtrr.c
> > index e69dfb552b1..c174dd9b3ad 100644
> > --- a/arch/x86/cpu/mtrr.c
> > +++ b/arch/x86/cpu/mtrr.c
> > @@ -156,8 +156,12 @@ int mtrr_commit(bool do_caches)
> >         debug("open done\n");
> >         qsort(req, gd->arch.mtrr_req_count, sizeof(*req), h_comp_mtrr);
> >         for (i = 0; i < gd->arch.mtrr_req_count; i++, req++)
> > -               mtrr_set_next_var(req->type, req->start, req->size);
> > +               set_var_mtrr(i, req->type, req->start, req->size);
>
> This change actually reverts 3bcd6cf89ef ("x86: mtrr: Skip MSRs that
> were already programmed.."), but as 3bcd6cf89ef commit message says:
>
>     At present mtrr_commit() programs the MTRR MSRs starting from
>     index 0, which may overwrite MSRs that were already programmed
>     by previous boot stage or FSP.
>
> So this change will cause regression.
>
> >
> > +       /* Clear the ones that are unused */
> > +       debug("clear\n");
> > +       for (; i < mtrr_get_var_count(); i++)
> > +               wrmsrl(MTRR_PHYS_MASK_MSR(i), 0);
>
> This change actually reverts 596bd0589ad ("x86: mtrr: Do not clear the
> unused ones.."), which will also create regression unfortunately..
>
> >         debug("close\n");
> >         mtrr_close(&state, do_caches);
> >         debug("mtrr done\n");
> > --

The regression mentioned above will cause Intel Crown Bay fail to
boot. See https://lore.kernel.org/u-boot/20210731084529.7524-1-bmeng.cn@gmail.com/

Regards,
Bin