Received-SPF: pass (google.com: domain of linux-kernel+bounces-149046-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Wed, 17 Apr 2024 11:23:27 -0700
In-Reply-To: <399bbc9d-ae75-4754-8ce9-af563df15a38@intel.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240413044129.52812-1-xry111@xry111.site> <20240413044129.52812-2-xry111@xry111.site>
 <Zh8Oli5MIyekLrbu@google.com> <20240417172222.f4h2csf4xkffccqn@desk> <399bbc9d-ae75-4754-8ce9-af563df15a38@intel.com>
Message-ID: <ZiATnwBHJThDNpxk@google.com>
Subject: Re: [PATCH v8 2/2] x86/mm: Don't disable PCID if the kernel is
 running on a hypervisor
From: Sean Christopherson <seanjc@google.com>
To: Dave Hansen <dave.hansen@intel.com>
Cc: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>, Xi Ruoyao <xry111@xry111.site>, 
	Dave Hansen <dave.hansen@linux.intel.com>, Michael Kelley <mhklinux@outlook.com>, 
	Andy Lutomirski <luto@kernel.org>, Peter Zijlstra <peterz@infradead.org>, 
	Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, 
	"H. Peter Anvin" <hpa@zytor.com>, x86@kernel.org, linux-kernel@vger.kernel.org, 
	Andrew Cooper <andrew.cooper3@citrix.com>
Content-Type: text/plain; charset="us-ascii"

On Wed, Apr 17, 2024, Dave Hansen wrote:
> On 4/17/24 10:22, Pawan Gupta wrote:
> >>>  static const struct x86_cpu_id invlpg_miss_ids[] = {
> >>> +	/* Only bare-metal is affected.  PCIDs in guests are OK.  */
> >>> +	{
> >>> +	  .vendor	= X86_VENDOR_INTEL,
> >>> +	  .family	= 6,
> >>> +	  .model	= INTEL_FAM6_ANY,

Just in case we go this route (I hope we don't), this should probably be:

        /* Only bare-metal is affected.  PCIDs in guests are OK.  */
        {
          .vendor       = X86_VENDOR_ANY,
          .feature      = X86_FEATURE_HYPERVISOR,
          .driver_data  = 0,
        },

to make it clear that the goal is to match only the feature.  Matching Intel P6
suffices because that's what the other entries in the array all check, but it
makes subtle, confusing code even more subtle and confusing.

> >>> +	  .feature	= X86_FEATURE_HYPERVISOR,
> >> Isn't this inverted?  x86_match_cpu() will return NULL if the CPU doesn't have
> >> HYPERVISOR.  We want it to return NULL if the CPU *does* have HYPERVISOR.
> > I think the implementation is correct, x86_match_cpu() will not return
> > NULL if the CPU doesn't have HYPERVISOR feature *and* matches one of the
> > CPUs below. It will only return NULL if none of the entries match.

Oooh, and because it's the first entry it will always be found even if a different
entry would match the FMS.  Oof.

> I think I gave a crappy suggestion here.
> 
> Let's just do the X86_FEATURE_HYPERVISOR explicitly in the code instead
> of trying to cram it into the invlpg_miss_ids[] check.  It's way easier
> to understand with an explicit code check.

+1.  And it doesn't rely on the HYPERVISOR entry being the first entry, which
is doubly evil.