Re: [PATCH v2 07/14] x86: Replace PAT_* with X86_MT_*

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.12.2022 23:26, Demi Marie Obenour wrote:
> This allows eliminating the former.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with a couple of small remarks (some asking for minor adjustments):

> @@ -72,8 +72,8 @@ static uint8_t __read_mostly 
> mtrr_epat_tbl[MTRR_NUM_TYPES][MEMORY_NUM_TYPES] =
>      };
>  
>  /* Lookup table for PAT entry of a given PAT value in host PAT. */
> -static uint8_t __read_mostly pat_entry_tbl[PAT_TYPE_NUMS] =
> -    { [0 ... PAT_TYPE_NUMS-1] = INVALID_MEM_TYPE };
> +static uint8_t __read_mostly pat_entry_tbl[X86_NUM_MT] =
> +    { [0 ... X86_NUM_MT-1] = INVALID_MEM_TYPE };

When touching code like this, please also correct style (here: missing
blanks around '-').

> @@ -145,14 +145,14 @@ int hvm_vcpu_cacheattr_init(struct vcpu *v)
>      m->mtrr_cap = (1u << 10) | (1u << 8) | num_var_ranges;
>  
>      v->arch.hvm.pat_cr =
> -        ((uint64_t)PAT_TYPE_WRBACK) |               /* PAT0: WB */
> -        ((uint64_t)PAT_TYPE_WRTHROUGH << 8) |       /* PAT1: WT */
> -        ((uint64_t)PAT_TYPE_UC_MINUS << 16) |       /* PAT2: UC- */
> -        ((uint64_t)PAT_TYPE_UNCACHABLE << 24) |     /* PAT3: UC */
> -        ((uint64_t)PAT_TYPE_WRBACK << 32) |         /* PAT4: WB */
> -        ((uint64_t)PAT_TYPE_WRTHROUGH << 40) |      /* PAT5: WT */
> -        ((uint64_t)PAT_TYPE_UC_MINUS << 48) |       /* PAT6: UC- */
> -        ((uint64_t)PAT_TYPE_UNCACHABLE << 56);      /* PAT7: UC */
> +        ((uint64_t)X86_MT_WB) |           /* PAT0: WB */
> +        ((uint64_t)X86_MT_WT << 8) |      /* PAT1: WT */
> +        ((uint64_t)X86_MT_UCM << 16) |    /* PAT2: UC- */
> +        ((uint64_t)X86_MT_UC << 24) |     /* PAT3: UC */
> +        ((uint64_t)X86_MT_WB << 32) |     /* PAT4: WB */
> +        ((uint64_t)X86_MT_WT << 40) |     /* PAT5: WT */
> +        ((uint64_t)X86_MT_UCM << 48) |    /* PAT6: UC- */
> +        ((uint64_t)X86_MT_UC << 56);      /* PAT7: UC */

As per my comment on the earlier patch the casts indeed want to stay, but
with how you had the earlier patch I wonder why you did keep them in this
version (and elsewhere below as well).

> @@ -356,7 +356,7 @@ uint32_t get_pat_flags(struct vcpu *v,
>       */
>      pat_entry_value = mtrr_epat_tbl[shadow_mtrr_type][guest_eff_mm_type];
>      /* If conflit occurs(e.g host MTRR is UC, guest memory type is
> -     * WB),set UC as effective memory. Here, returning PAT_TYPE_UNCACHABLE 
> will
> +     * WB),set UC as effective memory. Here, returning X86_MT_UC will

Would you mind at least adding the missing blank after the comma while
you touch the line?

> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -573,7 +573,7 @@ int epte_get_entry_emt(struct domain *d, gfn_t gfn, mfn_t 
> mfn,
>      if ( gmtrr_mtype >= 0 )
>      {
>          *ipat = true;
> -        return gmtrr_mtype != PAT_TYPE_UC_MINUS ? gmtrr_mtype
> +        return gmtrr_mtype != X86_MT_UCM ? gmtrr_mtype
>                                                  : MTRR_TYPE_UNCACHABLE;

Please adjust indentation on this line then as well.

Jan