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

From: "Jan Beulich" <JBeulich@suse.com>
To: Doug Goldstein <cardoe@cardoe.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: Thu, 18 Aug 2016 03:37:20 -0600	[thread overview]
Message-ID: <57B59DF00200007800106F6B@prv-mh.provo.novell.com> (raw)
In-Reply-To: <e0a5bb74-5131-cebf-4ff8-19f61f813108@cardoe.com>

>>> On 18.08.16 at 03:59, <cardoe@cardoe.com> wrote:
> 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)?

Well, that's what I've basically put up for discussion. My personal
preference would be to drop the hard coding of cpu_has_mtrr (and
hence keep it as well as mtrr_if).

>>> --- 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?

Either way is fine with me, all I wish is for it to be clear that you
don't stop half ways with the cleanup (which I'm glad you took a
stab on, btw).

Jan

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