Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
From:   "Compostella, Jeremy" <jeremy.compostella@intel.com>
To:     Adam Dunlap <acdunlap@google.com>
Cc:     Ingo Molnar <mingo@kernel.org>, <kirill.shutemov@linux.intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "x86@kernel.org" <x86@kernel.org>,
        Felix Held <felix-coreboot@felixheld.de>
Subject: Re: Reserved bits and commit x86/sev-es: Set x86_virt_bits to the
 correct value straight away, instead of a two-phase approach
References: <87r0lry3bs.fsf@jcompost-mobl.amr.corp.intel.com>
        <CAMBK9=ZAzKyxuxdsDXNeA8xyPufwFoMxJaHwuh-z+eOmrGD1qQ@mail.gmail.com>
        <87fs27xzxz.fsf@jcompost-mobl.amr.corp.intel.com>
Date:   Thu, 19 Oct 2023 13:01:16 -0700
Message-ID: <87jzribcvn.fsf@jcompost-mobl.amr.corp.intel.com>
Organization: Intel Corporation - 2200 Mission College Blvd. Santa Clara,
        CA 95052. USA
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Precedence: bulk

--=-=-=
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

"Compostella, Jeremy" <jeremy.compostella@intel.com> writes:

> Adam Dunlap <acdunlap@google.com> writes:
>
>> On Wed, Oct 18, 2023 at 3:27=E2=80=AFPM Compostella, Jeremy
>> <jeremy.compostella@intel.com> wrote:
>>> In the light of commit fbf6449f84bf I am wondering what is the right
>>> approach to fix the regression for AMD and then fix the MTRR check for
>>> Intel. Should we introduce a new cpu_dev callback to read the number
>>> of reserved bits and take it into account in get_cpu_address_sizes() ?
>>
>> I think this approach makes sense. It seems better to have one
>> function that simply sets it to the right thing rather than setting
>> it to one value and then adjusting it (fbf6449f84bf did that for
>> x86_virt_bits, although it caused some other problems). However, I'm
>> not sure it would solve the problem your original patch tried to
>> fix, since x86_phys_bits would still be set after intel_init, which
>> apparently uses the value.
>
> Using cscope, I don't see any evidence of any vendor init code using
> `x86_phys_bits'. To my knowledge, they seem to be only setting
> x86_phys_bits or adjusting it.
>
>
>> Would it work to move the call to get_cpu_address_sizes() to nearer
>> the start of early_identify_cpu()?  We could also add a cpu_dev
>> callback so it doesn't need the 2-phase approach, but this would at
>> least bring it back into parity with v6.6-rc6.
>
> Such a change should resolve the issue I reported on this thread. I
> can run a quick smoke test later tonight or tomorrow.

It turns out that your suggestion does not work because
`get_cpu_address_sizes()' relies on `c->extended_cpuid_level' (set by
`get_cpu_cap(c)') and the `X86_FEATURE_CPUID' cpu capability (set by
`setup_force_cpu_cap(X86_FEATURE_CPUID)').

The following change works perfectly well for me:

,----
| @@ -1589,6 +1591,7 @@ static void __init early_identify_cpu(struct cpuinf=
o_x86 *c)
| 		get_cpu_vendor(c);
| 		get_cpu_cap(c);
| 		setup_force_cpu_cap(X86_FEATURE_CPUID);
| +		get_cpu_address_sizes(c);
| 		cpu_parse_early_param();
|=20
| 		if (this_cpu->c_early_init)
| @@ -1603,7 +1606,6 @@ static void __init early_identify_cpu(struct cpuinf=
o_x86 *c)
| 		setup_clear_cpu_cap(X86_FEATURE_CPUID);
| 	}
|=20
| -	get_cpu_address_sizes(c);
|=20
| 	setup_force_cpu_cap(X86_FEATURE_ALWAYS);
`----

Looking at fbf6449f84bf I am under the impression it should not hurt
it either but I'll let you verify.

--=20
*Jeremy*
/One Emacs to rule them all/

--=-=-=--