Received-SPF: pass (google.com: domain of linux-kernel+bounces-158723-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: "Wang, Wei W" <wei.w.wang@intel.com>
To: Sean Christopherson <seanjc@google.com>
CC: Paolo Bonzini <pbonzini@redhat.com>, "kvm@vger.kernel.org"
	<kvm@vger.kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 1/4] KVM: x86: Add a struct to consolidate host values,
 e.g. EFER, XCR0, etc...
Thread-Topic: [PATCH 1/4] KVM: x86: Add a struct to consolidate host values,
 e.g. EFER, XCR0, etc...
Thread-Index: AQHalcvRIagHuf/K/kmJgZMA6+igorF4N0TQgADRuICAAAC28A==
Date: Thu, 25 Apr 2024 14:37:14 +0000
Message-ID: <DS0PR11MB6373C82CB77354EFD779E0C3DC172@DS0PR11MB6373.namprd11.prod.outlook.com>
References: <20240423221521.2923759-1-seanjc@google.com>
 <20240423221521.2923759-2-seanjc@google.com>
 <DS0PR11MB6373B95FF222DD6939CFEFC6DC172@DS0PR11MB6373.namprd11.prod.outlook.com>
 <ZipjhYUIAQMMkXci@google.com>
In-Reply-To: <ZipjhYUIAQMMkXci@google.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?FllzNEDzBgNFXZ6KVlmnyDi/RhkivifkjPFJJFf1mM4VB5x544k8Mgv2C0+3?=
 =?us-ascii?Q?2nZtJUorPSRwSs1AdB04dT7/VFKXZ05QidnEcm6Va6UY4Wve+VTMEj8JiOW+?=
 =?us-ascii?Q?X5hA3bF4HmbJuV/z0HKrj4dXPWqSklSIl6qto8pzLH4spuNW148ZK2KC3W1S?=
 =?us-ascii?Q?vriyNi4o9sQG2X3hWpdxL3SM8fQGrijxB+iQmkrOSbOXNoBnqs/IVcohP2gu?=
 =?us-ascii?Q?N6o/iYTV7w416HAbn+MJOyfw6QaufACgYBsmKmNHxLqDvihA2H2oIhj+Kurv?=
 =?us-ascii?Q?aStzEaLP3kRQGFRdgVf0ZqwOcbee3z7vSz9o8T0DBCYU4Iaimhx0RIiCmbFe?=
 =?us-ascii?Q?44BxuqG4V5lQPwRHqF1AVB9nh1ALwGKIxx9Ylrn+TztaWas7IK5dxv/CKfc4?=
 =?us-ascii?Q?OSg/hn0tgjFPmdywOAdEyISxUkzvqGobF2um9FRCGp+iavyDB+SSFon30iRa?=
 =?us-ascii?Q?65Pdt9sNd42a+YxHen9PEq/vCHo/cwZyG9PykOt9Askjw9kxKlnYP2Z8wTj0?=
 =?us-ascii?Q?ahcuO7K0bHRcT8VhOcLUY+aQBLRcQyluVh79k9DNFtzN7mGQPUQN+JdVdli4?=
 =?us-ascii?Q?pKLTsxehBasuihs7hP2QzXRLAwC4ayqRY7/z+6O2iKNFL5P8nOV+85rcK935?=
 =?us-ascii?Q?UsYel+3ppYDaAm2gdvyN/GIadWaH1WpR+pOgQCBc3z2NH2sv8CrQOvA7nwIa?=
 =?us-ascii?Q?cUsdpxKM8sVnXDzCK9wdn+VWq+oHFIudq5zBEZssZ5cFQnuspRB0J2lEWMN+?=
 =?us-ascii?Q?QZa/mEhYeMv+F5m8RDETMJkQrTtib+6Iq8asFacckYzT1IrmbQwA5gVT7bpq?=
 =?us-ascii?Q?cGI9jVuLdhdqvgqbOj4I2DBiN4z/YVgTRlItnFoD/J83BsvKH/FfIMjb2AcS?=
 =?us-ascii?Q?iectcRM0dvJDGYvuWR6pAcdGZUeoHR76ou5KhhKvtH1drUWtbLGLa4zykVne?=
 =?us-ascii?Q?Y2dt3PJb7/QH7QHD6bzAkrhuuwjtA0GVgFwqDXviqyqdBVR1AE4QPDyd1d/N?=
 =?us-ascii?Q?vaaV2WWkhTm3UcjfGhGZOKvH0Ula+/JdMbFzGyLnNVDXD+UmTn/vyNorrkDI?=
 =?us-ascii?Q?V9QH5tQCsuaekgMdiwSMFFi4KWXAEvayE9kCLQo+sVKCbbPjAi9RY3JB8P9G?=
 =?us-ascii?Q?U6t4UPUB0kl+29ZDVYb95ZtEk8L8G+lurTA6XakUjQaCSyRgwTvtd16/wO6B?=
 =?us-ascii?Q?AI/aLASYX8hV8QAyW5CXHemC4L2vdZzLJxEnvK4NXqXdV5nrfuNn3k8+6OVD?=
 =?us-ascii?Q?jgc2w8ZBMrCiWKhtUm65yqUvjry5yCh7FBE4FB+RMK/Yku+vvGdYMz9/IO98?=
 =?us-ascii?Q?mW2HiRWYP1D//rVDbn9PUsDvdnY6/G6LbTb9bJiVHkKKBXlotFdyGHeb54iH?=
 =?us-ascii?Q?N8oSynTGadEgTYwmXBJYNBfJ3ttAHJczP7pg9118xpltOW39H1eZE/3H1SAj?=
 =?us-ascii?Q?b7IhgQbEF+KVblAeA+oeZrpj+IXutzGzPeGKFPl7c121nY9/5ckujVst8qZ8?=
 =?us-ascii?Q?DNbeE0VtL57kkbywxIzmn1uM55Ga0Swm6y7axKoDlmKxnm+kTzhpKsPHRxNi?=
 =?us-ascii?Q?t2008UeVmKttHrpj2uPGj6iuicMAYUm6uVps7XlU?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB6373.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b2d19ca6-e82c-41a4-8324-08dc653532e9
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Apr 2024 14:37:14.1004
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: cT6Kn+ip1ijB3auVNadMgrJkYZNl7Q3dOBPSWYUgoQcMZYtoYHgRuD7BNDOYLtB0dC3gcre6SRf2zhRjk9xW+g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB6102
X-OriginatorOrg: intel.com

On Thursday, April 25, 2024 10:10 PM, Sean Christopherson wrote:
> On Thu, Apr 25, 2024, Wei W Wang wrote:
> > On Wednesday, April 24, 2024 6:15 AM, Sean Christopherson wrote:
> > > @@ -403,7 +403,7 @@ static void vmx_update_fb_clear_dis(struct
> > > kvm_vcpu *vcpu, struct vcpu_vmx *vmx)
> > >  	 * and VM-Exit.
> > >  	 */
> > >  	vmx->disable_fb_clear
> > > =3D !cpu_feature_enabled(X86_FEATURE_CLEAR_CPU_BUF) &&
> > > -				(host_arch_capabilities &
> > > ARCH_CAP_FB_CLEAR_CTRL) &&
> > > +				(kvm_host.arch_capabilities &
> > > ARCH_CAP_FB_CLEAR_CTRL) &&
> >
> > The line of code appears to be lengthy. It would be preferable to
> > limit it to under
> > 80 columns per line.
>=20
> I agree that staying under 80 is generally preferred, but I find this
>=20
> 	vmx->disable_fb_clear =3D (kvm_host.arch_capabilities &
> ARCH_CAP_FB_CLEAR_CTRL) &&
> 				!boot_cpu_has_bug(X86_BUG_MDS) &&
> 				!boot_cpu_has_bug(X86_BUG_TAA);
>=20
> much more readable than this
>=20
> 	vmx->disable_fb_clear =3D (kvm_host.arch_capabilities &
> 			 	 ARCH_CAP_FB_CLEAR_CTRL) &&
> 				!boot_cpu_has_bug(X86_BUG_MDS) &&
> 				!boot_cpu_has_bug(X86_BUG_TAA);
>=20
> We should shorten the name to arch_caps, but I don't think that's a net
> positive, e.g. unless we do a bulk rename, it'd diverge from several othe=
r
> functions/variables, and IMO it would be less obvious that the field hold=
s
> MSR_IA32_ARCH_CAPABILITIES.

Yeah, the above isn't nice and no need to do bulk rename.
We could just shorten it here, e.g.:

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 4ed8c73f88e4..8d0ab5a6a515 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -393,6 +393,9 @@ static __always_inline void vmx_enable_fb_clear(struct =
vcpu_vmx *vmx)

 static void vmx_update_fb_clear_dis(struct kvm_vcpu *vcpu, struct vcpu_vmx=
 *vmx)
 {
+       u64 arch_cap =3D kvm_host.arch_capabilities;
+
        /*
         * Disable VERW's behavior of clearing CPU buffers for the guest if=
 the
         * CPU isn't affected by MDS/TAA, and the host hasn't forcefully en=
abled
@@ -402,7 +405,7 @@ static void vmx_update_fb_clear_dis(struct kvm_vcpu *vc=
pu, struct vcpu_vmx *vmx)
         * and VM-Exit.
         */
        vmx->disable_fb_clear =3D !cpu_feature_enabled(X86_FEATURE_CLEAR_CP=
U_BUF) &&
-                               (kvm_host.arch_capabilities & ARCH_CAP_FB_C=
LEAR_CTRL) &&
+                               (arch_cap & ARCH_CAP_FB_CLEAR_CTRL) &&
                                !boot_cpu_has_bug(X86_BUG_MDS) &&
                                !boot_cpu_has_bug(X86_BUG_TAA);


>=20
> > >  				!boot_cpu_has_bug(X86_BUG_MDS) &&
> > >  				!boot_cpu_has_bug(X86_BUG_TAA);
> > >
>=20
> > > @@ -325,11 +332,8 @@ int x86_emulate_instruction(struct kvm_vcpu
> > > *vcpu, gpa_t cr2_or_gpa,
> > >  			    int emulation_type, void *insn, int insn_len);
> fastpath_t
> > > handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu);
> > >
> > > -extern u64 host_xcr0;
> > > -extern u64 host_xss;
> > > -extern u64 host_arch_capabilities;
> > > -
> > >  extern struct kvm_caps kvm_caps;
> > > +extern struct kvm_host_values kvm_host;
> >
> > Have you considered merging the kvm_host_values and kvm_caps into one
> > unified structure?
>=20
> No really.  I don't see any benefit, only the downside of having to come =
up
> with a name that is intuitive when reading code related to both.

I thought the two structures perform quite similar jobs and most of the fie=
lds in
kvm_cap, e.g. has_tsc_control, supported_perf_cap, could also be interprete=
d
as host values?