From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-io1-f73.google.com (mail-io1-f73.google.com [209.85.166.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id EC7121FECCB for ; Wed, 27 Nov 2024 14:38:42 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.166.73 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1732718324; cv=none; b=YVLl4fYupHgxnLimAcYFCd2E9MNEUcnJ1hBs1JYugfFyp75ixI/8CzZ4wjIEWGddhCs66qo15QUBp+dDIT9D0OHJae+Dq6A/8hUy6eNgFrSxD+gViEQqf9AhNswsudD55hfqjvzGlp0CR0J0yj2CA1zXmTCzYQh5x4t/GDFCfOE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1732718324; c=relaxed/simple; bh=xT97KYgdluN7ebir3nQvf+4Hgac1dq5jbrG4z+OLnsc=; h=Date:In-Reply-To:Mime-Version:References:Message-ID:Subject:From: To:Cc:Content-Type; b=knGISypSdwQedYACVM5HeFNDcZW7qBfALRz2b4BUJz59SEXOdNEakZpXAwbTW9qfA9lvojcXRB9EhX9iHvWJf+AYSoLnYP52ut+8t3Z2dE/2WNE1CL/KDddqaqKommJzAgtqWfcvlcbbD0ht079cbJD0kcuNzORSx/gSrPygwQI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=flex--seanjc.bounces.google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=oHjNr/E7; arc=none smtp.client-ip=209.85.166.73 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=flex--seanjc.bounces.google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="oHjNr/E7" Received: by mail-io1-f73.google.com with SMTP id ca18e2360f4ac-841896ec108so375069639f.1 for ; Wed, 27 Nov 2024 06:38:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1732718322; x=1733323122; darn=vger.kernel.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=RcW151oQ5hondKtplla/DkVWCP6XFqg+OnT+IGOy13o=; b=oHjNr/E7Gqu5IU/mx96kzhrk3/6fzB+qcYaWvWCcpANsuzVAfDCK2vKdlVmG1ei+/q KYWM9uzyjkOJOiXoNiHz86igzgp69psGE2Sxm+QjpB4Y51iyqyiyxojxIfQU28EkaEpT 3CdXjCDxvjsRneJcfDf95BeEyb79g9kVThMFaAfGbIKsedXLwCANS1WVfJPQP+cNekDU f36aNtSxyXGNq296Lze/ihooLmE75K5OMmL5ISIDi0VQ43GkwE5/f5F2F+hVekV3jS3F 5wABWAkIoTSjrwocQAE6EeKMHmPtOV6eFl8O6EbFZYF80agVTXizR78vXR9D1z5saZ85 /asQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1732718322; x=1733323122; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=RcW151oQ5hondKtplla/DkVWCP6XFqg+OnT+IGOy13o=; b=Kq57svNlt2gmBFfZ2bcrjLmnTCA9dHT+UZKrtmUtblbrFXzz2/w6+GmMVh55OoLtT9 z1Q22/r80n8Jgkdq70Hbq702JkJBTl+jsZpR/OEZ+WuEWimDRTwYZ3a0ezREuujiU2lx 5hU+cVxjSRiuTN1/vohEJ9nJkJ8NRPHOpw4rAj0ugICRhZysOgxi+GJzihM1KfzmgIzt H3CV/Tg9eWMzT0Yi4ufXLa8tYAxkfdwUFTUDh1/Is5j5tAmwSbG6hiUOCWZZriO1XP2A bL+nGBg7YffgH2YVAze3jYxK+S8wqFUhzSYfN8X5CczkbKRVKyTkeH+HbI3ix8ltLmr4 NgbQ== X-Forwarded-Encrypted: i=1; AJvYcCUn2zdU174luVckVgDMXCVKBtsxGxFUYuq7B/gx0fyi+mYa5x71hwquILLAcO0QEluI26XCqVV7X63GTF4=@vger.kernel.org X-Gm-Message-State: AOJu0Yxez1pUekghSF6jTJ66G205GEFSmBXXyNGwyMw+VFgZNVJQuXaS zR2f8Fi/e9qp6d2t7Ywfs3qeGADLexpLqQShnyAhi8jTHORUtb08rHwsiiJd22PMkElklOBUVuh wAQ== X-Google-Smtp-Source: AGHT+IG7Clebz5+ikhZk/p3e2lBnUMggo0Vrm0EMc7kyDhz6gWIVGZ5y2tr41LsMOLzWgPahBG28XOb331w= X-Received: from pgg13.prod.google.com ([2002:a05:6a02:4d8d:b0:7fb:db54:f065]) (user=seanjc job=prod-delivery.src-stubby-dispatcher) by 2002:a05:6e02:1e01:b0:3a7:7ee3:108d with SMTP id e9e14a558f8ab-3a7c55f2783mr35486615ab.23.1732718322166; Wed, 27 Nov 2024 06:38:42 -0800 (PST) Date: Wed, 27 Nov 2024 06:38:41 -0800 In-Reply-To: Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 References: <20240517173926.965351-23-seanjc@google.com> <43ef06aca700528d956c8f51101715df86f32a91.camel@redhat.com> <3da2be9507058a15578b5f736bc179dc3b5e970f.camel@redhat.com> <8f35b524cda53aff29a9389c79742fc14f77ec68.camel@redhat.com> <44e7f9cba483bda99f8ddc0a2ad41d69687e1dbe.camel@redhat.com> Message-ID: Subject: Re: [PATCH v2 22/49] KVM: x86: Add a macro to precisely handle aliased 0x1.EDX CPUID features From: Sean Christopherson To: Maxim Levitsky Cc: Paolo Bonzini , Vitaly Kuznetsov , kvm@vger.kernel.org, linux-kernel@vger.kernel.org, Hou Wenlong , Kechen Lu , Oliver Upton , Binbin Wu , Yang Weijiang , Robert Hoo Content-Type: text/plain; charset="us-ascii" On Thu, Nov 21, 2024, Maxim Levitsky wrote: > On Wed, 2024-09-11 at 08:37 -0700, Sean Christopherson wrote: > > On Tue, Sep 10, 2024, Maxim Levitsky wrote: > > > On Mon, 2024-08-05 at 15:00 -0700, Sean Christopherson wrote: > > > > At that point, I'm ok with defining each alias, though I honestly still don't > > > > understand the motivation for defining single-use macros. > > > > > > > > > > The idea is that nobody will need to look at these macros > > > (e.g__X86_FEATURE_8000_0001_ALIAS() and its usages), because it's clear what > > > they do, they just define few extra CPUID features that nobody really cares > > > about. > > > > > > ALIASED_F() on the other hand is yet another _F macro() and we will need, > > > once again and again to figure out why it is there, what it does, etc. > > > > That seems easily solved by naming the macro ALIASED_8000_0001_F(). I don't see > > how that's any less clear than __X86_FEATURE_8000_0001_ALIAS(), and as above, > > there are several advantages to defining the alias in the context of the leaf > > builder. > > > > Hi! > > I am stating my point again: Treating 8000_0001 leaf aliases as regular CPUID > features means that we don't need common code to deal with this, and thus > when someone reads the common code (and this is the thing I care about the > most) that someone won't need to dig up the info about what these aliases > are. Ah, this is where we disagree, I think. I feel quite strongly that oddities such as aliased/duplicate CPUID feature bits need to be made as visible as possible, and well documented. Hiding architectural quirks might save some readers a few seconds of their time, but it can also confuse others, and more importantly, makes it more difficult for new readers/developers to learn about the quirks. This code _looks_ wrong, as there's no indication that CPUID_8000_0001_EDX is unique. I too wasn't aware of the aliases until this series, and I was very confused by KVM's code. The only clue that I was given was the "Don't duplicate feature flags which are redundant with Intel!" comment in cpufeatures.h; I still ended up digging through the APM to understand what was going on. kvm_cpu_cap_mask(CPUID_1_EDX, F(FPU) | F(VME) | F(DE) | F(PSE) | F(TSC) | F(MSR) | F(PAE) | F(MCE) | F(CX8) | F(APIC) | 0 /* Reserved */ | F(SEP) | F(MTRR) | F(PGE) | F(MCA) | F(CMOV) | F(PAT) | F(PSE36) | 0 /* PSN */ | F(CLFLUSH) | 0 /* Reserved, DS, ACPI */ | F(MMX) | F(FXSR) | F(XMM) | F(XMM2) | F(SELFSNOOP) | 0 /* HTT, TM, Reserved, PBE */ ); kvm_cpu_cap_mask(CPUID_8000_0001_EDX, F(FPU) | F(VME) | F(DE) | F(PSE) | F(TSC) | F(MSR) | F(PAE) | F(MCE) | F(CX8) | F(APIC) | 0 /* Reserved */ | F(SYSCALL) | F(MTRR) | F(PGE) | F(MCA) | F(CMOV) | F(PAT) | F(PSE36) | 0 /* Reserved */ | F(NX) | 0 /* Reserved */ | F(MMXEXT) | F(MMX) | F(FXSR) | F(FXSR_OPT) | f_gbpages | F(RDTSCP) | 0 /* Reserved */ | f_lm | F(3DNOWEXT) | F(3DNOW) ); Versus this code, which hopefully elicits a "huh!?" and prompts curious readers to go look at the definition of ALIASED_1_EDX_F() to understand why KVM is being weird. And if readers can't figure things out purely from ALIASED_1_EDX_F()'s comment, then that's effectively a KVM documentation issue and should be fixed. In other words, I want to make things like this stick out so that more developers are aware of such quirks, i.e. to to minimize the probability of such knowledge being lost. I don't want the next generation of KVM developers to have to re-discover things that can be solved by a moderately verbose comment. kvm_cpu_cap_init(CPUID_1_EDX, F(FPU), F(VME), F(DE), F(PSE), F(TSC), F(MSR), F(PAE), F(MCE), F(CX8), F(APIC), ... ); kvm_cpu_cap_init(CPUID_8000_0001_EDX, ALIASED_1_EDX_F(FPU), ALIASED_1_EDX_F(VME), ALIASED_1_EDX_F(DE), ALIASED_1_EDX_F(PSE), ALIASED_1_EDX_F(TSC), ALIASED_1_EDX_F(MSR), ALIASED_1_EDX_F(PAE), ALIASED_1_EDX_F(MCE), ALIASED_1_EDX_F(CX8), ALIASED_1_EDX_F(APIC), ... ); > I for example didn't knew about them because these aliases are basically a > result of AMD redoing some things in the spec their way when they just > released first 64-bit extensions. I didn't follow the x86 ISA closely back > then (I only had 32 bit systems to play with). > > Best regards, > Maxim Levitsky > >