| Message ID | 20230504225101.2366414-13-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | x86: Various fixes for chromebooks | expand |
Hi Simon, On Fri, May 5, 2023 at 6:51 AM Simon Glass <sjg@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@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"); > -- Regards, Bin
On Mon, May 8, 2023 at 10:48 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > Hi Simon, > > On Fri, May 5, 2023 at 6:51 AM Simon Glass <sjg@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@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
Hi Bin, On Mon, 8 May 2023 at 08:51, Bin Meng <bmeng.cn@gmail.com> wrote: > > On Mon, May 8, 2023 at 10:48 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > > > Hi Simon, > > > > On Fri, May 5, 2023 at 6:51 AM Simon Glass <sjg@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@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
Hi Simon, On Tue, May 9, 2023 at 11:08 AM Simon Glass <sjg@chromium.org> wrote: > > Hi Bin, > > On Mon, 8 May 2023 at 08:51, Bin Meng <bmeng.cn@gmail.com> wrote: > > > > On Mon, May 8, 2023 at 10:48 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > > > > > Hi Simon, > > > > > > On Fri, May 5, 2023 at 6:51 AM Simon Glass <sjg@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@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
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); + /* Clear the ones that are unused */ + debug("clear\n"); + for (; i < mtrr_get_var_count(); i++) + wrmsrl(MTRR_PHYS_MASK_MSR(i), 0); debug("close\n"); mtrr_close(&state, do_caches); debug("mtrr done\n");
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@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(-)