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

[Simon Glass]

Hi Bin,

On Mon, 8 May 2023 at 08:51, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> 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/

Can that board perhaps use the other functoin to set MTRRs? It is
mtrr_set_next_var()

The change in the API has broken two of the Chromebooks which is why I
am trying to go back to the way it was. Does this board set up the
MTRRs in FSP? If so, perhaps we need a Kconfig for that? It is not
what the newer FSPs do, but perhaps we could use that behaviour for
FSPv1?

Regards,
Simon