[U-Boot] [PATCH 3/3] x86: Test mtrr support flag before accessing mtrr msr

[Simon Glass]

Hi Bin,

On 21 January 2015 at 09:19, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Thu, Jan 22, 2015 at 12:06 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Bin,
>>
>> On 19 January 2015 at 22:01, Bin Meng <bmeng.cn at 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 at 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