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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.08.16 at 01:28, <cardoe@xxxxxxxxxx> 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.

> @@ -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.

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

Jan


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