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

[Bin Meng]

Hi Simon,

On Tue, May 9, 2023 at 11:08 AM Simon Glass <sjg at chromium.org> wrote:
>
> 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()

mtrr_commit() is called by the following common places:

- arch/x86/lib/init_helpers.c::init_cache_f_r()
- arch/x86/lib/spl.c::board_init_f_r()
- arch/x86/lib/fsp/fsp_graphics.c::fsp_video_probe()
- drivers/video/vesa.c::vesa_video_probe()

> 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

Yes, FSPv1 sets up the MTRRs.

> what the newer FSPs do, but perhaps we could use that behaviour for
> FSPv1?

This mtrr_commit() API is overloaded. On some places it is called with
caller's intention to initialize MTRRs completely from scratch but on
some other places it is called with caller's intention to initialize
the next available MTRR. We should make this API usage clear. I will
see if I can make a patch.

Regards,
Bin