Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <01d50e80-7d58-6dc8-73d4-bd8ad1d1bec0@zytor.com>
Date:   Mon, 5 Jun 2023 10:19:10 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v8 09/33] x86/cpu: add X86_CR4_FRED macro
Content-Language: en-US
To:     Thomas Gleixner <tglx@linutronix.de>, Xin Li <xin3.li@intel.com>,
        linux-kernel@vger.kernel.org, x86@kernel.org, kvm@vger.kernel.org
Cc:     mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
        peterz@infradead.org, andrew.cooper3@citrix.com, seanjc@google.com,
        pbonzini@redhat.com, ravi.v.shankar@intel.com,
        jiangshanlai@gmail.com, shan.kang@intel.com
References: <20230410081438.1750-1-xin3.li@intel.com>
 <20230410081438.1750-10-xin3.li@intel.com> <87r0qq6rtm.ffs@tglx>
From:   "H. Peter Anvin" <hpa@zytor.com>
In-Reply-To: <87r0qq6rtm.ffs@tglx>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 6/5/23 05:01, Thomas Gleixner wrote:
> On Mon, Apr 10 2023 at 01:14, Xin Li wrote:
> 
>> From: "H. Peter Anvin (Intel)" <hpa@zytor.com>
>>
>> Add X86_CR4_FRED macro for the FRED bit in %cr4. This bit should be a
> 
> s/should/must/ no?
> 
>> +/*
>> + * These bits should not change their value after CPU init is finished.
>> + * The explicit cast to unsigned long suppresses a warning on i386 for
>> + * x86-64 only feature bits >= 32.
>> + */
>>   static const unsigned long cr4_pinned_mask =
>> -	X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_UMIP |
>> -	X86_CR4_FSGSBASE | X86_CR4_CET;
>> +	(unsigned long)
> 
> That type cast is required because:
> 
> +#define X86_CR4_FRED		_BITULL(X86_CR4_FRED_BIT)
> 
> Bah. Fred is 64 bit only. So why defining this as 1ULL << 32
> unconditionally and stripping the bit off on 32bit via the type cast?
> 
> #ifdef CONFIG_X86_64
> #define X86_CR4_FRED		_BITUL(X86_CR4_FRED_BIT)
> #else
> #define X86_CR4_FRED		0
> #endif
> 
> would be too obvious, right?
> 

It also adds an #ifdef mess to avoid a simple typecast. Is that the 
right tradeoff?

I'm not saying it is or it isn't, it is an open question.

	-hpa