| Message ID | 1421730101-29664-3-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Bin, On 19 January 2015 at 22:01, Bin Meng <bmeng.cn@gmail.com> wrote: > On some x86 processors (like Intel Quark) the MTRR registers are not > supported. This is reflected by the CPUID (EAX 01H) result EDX[12]. > Accessing the MTRR registers on such processors will cause #GP so we > must test the support flag before accessing MTRR MSRs. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > --- > > arch/x86/cpu/mtrr.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/arch/x86/cpu/mtrr.c b/arch/x86/cpu/mtrr.c > index ac8765f..68f1b04 100644 > --- a/arch/x86/cpu/mtrr.c > +++ b/arch/x86/cpu/mtrr.c > @@ -22,6 +22,9 @@ DECLARE_GLOBAL_DATA_PTR; > /* Prepare to adjust MTRRs */ > void mtrr_open(struct mtrr_state *state) > { > + if (!gd->arch.has_mtrr) > + return; > + > state->enable_cache = dcache_status(); > > if (state->enable_cache) > @@ -33,6 +36,9 @@ void mtrr_open(struct mtrr_state *state) > /* Clean up after adjusting MTRRs, and enable them */ > void mtrr_close(struct mtrr_state *state) > { > + if (!gd->arch.has_mtrr) > + return; > + > wrmsrl(MTRR_DEF_TYPE_MSR, state->deftype | MTRR_DEF_TYPE_EN); > if (state->enable_cache) > enable_caches(); > @@ -45,6 +51,9 @@ int mtrr_commit(bool do_caches) > uint64_t mask; > int i; > > + if (!gd->arch.has_mtrr) > + return 0; > + > mtrr_open(&state); > for (i = 0; i < gd->arch.mtrr_req_count; i++, req++) { > mask = ~(req->size - 1); > @@ -66,6 +75,9 @@ int mtrr_add_request(int type, uint64_t start, uint64_t size) > struct mtrr_request *req; > uint64_t mask; > > + if (!gd->arch.has_mtrr) > + return 0; Shouldn't this (and the above) return -ENOSYS? > + > if (gd->arch.mtrr_req_count == MAX_MTRR_REQUESTS) > return -ENOSPC; > req = &gd->arch.mtrr_req[gd->arch.mtrr_req_count++]; > -- > 1.8.2.1 > Regards, Simon
Hi Simon, On Thu, Jan 22, 2015 at 12:06 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > On 19 January 2015 at 22:01, Bin Meng <bmeng.cn@gmail.com> wrote: >> On some x86 processors (like Intel Quark) the MTRR registers are not >> supported. This is reflected by the CPUID (EAX 01H) result EDX[12]. >> Accessing the MTRR registers on such processors will cause #GP so we >> must test the support flag before accessing MTRR MSRs. >> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> --- >> >> arch/x86/cpu/mtrr.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/arch/x86/cpu/mtrr.c b/arch/x86/cpu/mtrr.c >> index ac8765f..68f1b04 100644 >> --- a/arch/x86/cpu/mtrr.c >> +++ b/arch/x86/cpu/mtrr.c >> @@ -22,6 +22,9 @@ DECLARE_GLOBAL_DATA_PTR; >> /* Prepare to adjust MTRRs */ >> void mtrr_open(struct mtrr_state *state) >> { >> + if (!gd->arch.has_mtrr) >> + return; >> + >> state->enable_cache = dcache_status(); >> >> if (state->enable_cache) >> @@ -33,6 +36,9 @@ void mtrr_open(struct mtrr_state *state) >> /* Clean up after adjusting MTRRs, and enable them */ >> void mtrr_close(struct mtrr_state *state) >> { >> + if (!gd->arch.has_mtrr) >> + return; >> + >> wrmsrl(MTRR_DEF_TYPE_MSR, state->deftype | MTRR_DEF_TYPE_EN); >> if (state->enable_cache) >> enable_caches(); >> @@ -45,6 +51,9 @@ int mtrr_commit(bool do_caches) >> uint64_t mask; >> int i; >> >> + if (!gd->arch.has_mtrr) >> + return 0; >> + >> mtrr_open(&state); >> for (i = 0; i < gd->arch.mtrr_req_count; i++, req++) { >> mask = ~(req->size - 1); >> @@ -66,6 +75,9 @@ int mtrr_add_request(int type, uint64_t start, uint64_t size) >> struct mtrr_request *req; >> uint64_t mask; >> >> + if (!gd->arch.has_mtrr) >> + return 0; > > Shouldn't this (and the above) return -ENOSYS? If returning non-zero, the init_cache_f_r() will fail as it checks the return value. Do you think we should ignore the return value there? But if ignored, there is no meaning of returning error codes here. >> + >> if (gd->arch.mtrr_req_count == MAX_MTRR_REQUESTS) >> return -ENOSPC; >> req = &gd->arch.mtrr_req[gd->arch.mtrr_req_count++]; >> -- >> 1.8.2.1 >> > > Regards, > Simon Regards, Bin
Hi Bin, On 21 January 2015 at 09:19, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Thu, Jan 22, 2015 at 12:06 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Bin, >> >> On 19 January 2015 at 22:01, Bin Meng <bmeng.cn@gmail.com> wrote: >>> On some x86 processors (like Intel Quark) the MTRR registers are not >>> supported. This is reflected by the CPUID (EAX 01H) result EDX[12]. >>> Accessing the MTRR registers on such processors will cause #GP so we >>> must test the support flag before accessing MTRR MSRs. >>> >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>> --- >>> >>> arch/x86/cpu/mtrr.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/arch/x86/cpu/mtrr.c b/arch/x86/cpu/mtrr.c >>> index ac8765f..68f1b04 100644 >>> --- a/arch/x86/cpu/mtrr.c >>> +++ b/arch/x86/cpu/mtrr.c >>> @@ -22,6 +22,9 @@ DECLARE_GLOBAL_DATA_PTR; >>> /* Prepare to adjust MTRRs */ >>> void mtrr_open(struct mtrr_state *state) >>> { >>> + if (!gd->arch.has_mtrr) >>> + return; >>> + >>> state->enable_cache = dcache_status(); >>> >>> if (state->enable_cache) >>> @@ -33,6 +36,9 @@ void mtrr_open(struct mtrr_state *state) >>> /* Clean up after adjusting MTRRs, and enable them */ >>> void mtrr_close(struct mtrr_state *state) >>> { >>> + if (!gd->arch.has_mtrr) >>> + return; >>> + >>> wrmsrl(MTRR_DEF_TYPE_MSR, state->deftype | MTRR_DEF_TYPE_EN); >>> if (state->enable_cache) >>> enable_caches(); >>> @@ -45,6 +51,9 @@ int mtrr_commit(bool do_caches) >>> uint64_t mask; >>> int i; >>> >>> + if (!gd->arch.has_mtrr) >>> + return 0; >>> + >>> mtrr_open(&state); >>> for (i = 0; i < gd->arch.mtrr_req_count; i++, req++) { >>> mask = ~(req->size - 1); >>> @@ -66,6 +75,9 @@ int mtrr_add_request(int type, uint64_t start, uint64_t size) >>> struct mtrr_request *req; >>> uint64_t mask; >>> >>> + if (!gd->arch.has_mtrr) >>> + return 0; >> >> Shouldn't this (and the above) return -ENOSYS? > > If returning non-zero, the init_cache_f_r() will fail as it checks the > return value. Do you think we should ignore the return value there? > But if ignored, there is no meaning of returning error codes here. Yes, I understand, but I think it is better to ignore (just the -ENOSYS) return value in the caller (add a comment in the code if you like) than return an incorrect value indicating that the action was taken. > >>> + >>> if (gd->arch.mtrr_req_count == MAX_MTRR_REQUESTS) >>> return -ENOSPC; >>> req = &gd->arch.mtrr_req[gd->arch.mtrr_req_count++]; >>> -- >>> 1.8.2.1 Regards, Simon
diff --git a/arch/x86/cpu/mtrr.c b/arch/x86/cpu/mtrr.c index ac8765f..68f1b04 100644 --- a/arch/x86/cpu/mtrr.c +++ b/arch/x86/cpu/mtrr.c @@ -22,6 +22,9 @@ DECLARE_GLOBAL_DATA_PTR; /* Prepare to adjust MTRRs */ void mtrr_open(struct mtrr_state *state) { + if (!gd->arch.has_mtrr) + return; + state->enable_cache = dcache_status(); if (state->enable_cache) @@ -33,6 +36,9 @@ void mtrr_open(struct mtrr_state *state) /* Clean up after adjusting MTRRs, and enable them */ void mtrr_close(struct mtrr_state *state) { + if (!gd->arch.has_mtrr) + return; + wrmsrl(MTRR_DEF_TYPE_MSR, state->deftype | MTRR_DEF_TYPE_EN); if (state->enable_cache) enable_caches(); @@ -45,6 +51,9 @@ int mtrr_commit(bool do_caches) uint64_t mask; int i; + if (!gd->arch.has_mtrr) + return 0; + mtrr_open(&state); for (i = 0; i < gd->arch.mtrr_req_count; i++, req++) { mask = ~(req->size - 1); @@ -66,6 +75,9 @@ int mtrr_add_request(int type, uint64_t start, uint64_t size) struct mtrr_request *req; uint64_t mask; + if (!gd->arch.has_mtrr) + return 0; + if (gd->arch.mtrr_req_count == MAX_MTRR_REQUESTS) return -ENOSPC; req = &gd->arch.mtrr_req[gd->arch.mtrr_req_count++];
On some x86 processors (like Intel Quark) the MTRR registers are not supported. This is reflected by the CPUID (EAX 01H) result EDX[12]. Accessing the MTRR registers on such processors will cause #GP so we must test the support flag before accessing MTRR MSRs. Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- arch/x86/cpu/mtrr.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)