Re: [PATCH 2/9] x86/mtrr: drop mtrr_if indirection

From: Doug Goldstein <cardoe@cardoe.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>, xen-devel@lists.xen.org
Subject: Re: [PATCH 2/9] x86/mtrr: drop mtrr_if indirection
Date: Wed, 17 Aug 2016 20:59:42 -0500	[thread overview]
Message-ID: <e0a5bb74-5131-cebf-4ff8-19f61f813108@cardoe.com> (raw)
In-Reply-To: <57B4798B0200007800106AE5@prv-mh.provo.novell.com>

[-- Attachment #1.1.1: Type: text/plain, Size: 2205 bytes --]

On 8/17/16 7:49 AM, Jan Beulich wrote:
>>>> On 17.08.16 at 01:28, <cardoe@cardoe.com> wrote:
>> There can only ever be one mtrr_if now and that is the generic
>> implementation
> 
> This is only true when taking into consideration that cpu_has_mtrr
> is #define-d to 1 right now. I'm not sure that's actually a good
> assumption (especially when think about running Xen itself
> virtualized, or possibly adding a mode of operation where no MTRRs
> are to be used). But if we want to keep it that way, then I'd suggest
> this patch should include removing cpu_has_mtrr (which will then
> show to the reviewers that the checks of mtrr_if against NULL
> indeed are dead code.

Sure I can remove cpu_has_mtrr that would certainly make it cleaner. Is
it ok with you and Andrew to make the assumption that we'll always have
MTRRs (until the day we don't like you described)?

> 
>> @@ -569,22 +561,19 @@ struct mtrr_value {
>>  void __init mtrr_bp_init(void)
>>  {
>>  	if (cpu_has_mtrr) {
>> -		mtrr_if = &generic_mtrr_ops;
>>  		size_or_mask = ~((1ULL << (paddr_bits - PAGE_SHIFT)) - 1);
>>  		size_and_mask = ~size_or_mask & 0xfffff00000ULL;
>>  	}
>>  
>> -	if (mtrr_if) {
>> -		set_num_var_ranges();
>> -		init_table();
>> -		if (use_intel())
>> -			get_mtrr_state();
>> -	}
>> +    set_num_var_ranges();
>> +    init_table();
>> +    if (use_intel())
>> +        get_mtrr_state();
>>  }
> 
> Please don't break indentation style.

Sad panda. This file has tabs. Sorry I missed that.

> 
>> --- a/xen/arch/x86/cpu/mtrr/mtrr.h
>> +++ b/xen/arch/x86/cpu/mtrr/mtrr.h
>> @@ -63,8 +63,8 @@ extern void set_mtrr_ops(const struct mtrr_ops *);
>>  extern u64 size_or_mask, size_and_mask;
>>  extern const struct mtrr_ops *mtrr_if;
>>  
>> -#define is_cpu(vnd)	(mtrr_if && mtrr_if->vendor == X86_VENDOR_##vnd)
>> -#define use_intel()	(mtrr_if && mtrr_if->use_intel_if == 1)
>> +#define is_cpu(vnd)	(X86_VENDOR_INTEL == X86_VENDOR_##vnd)
>> +#define use_intel()	(1)
> 
> Is the latter really useful to keep then?

Would you like me to squash patch 4 into this or make a note in the
commit message that further clean ups are coming?

--
Doug Goldstein

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel