Re: [PATCH v4 02/10] x86/mm: Avoid hard-coding PAT in get_page_from_l1e()

[Demi Marie Obenour <demi@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 3086 bytes --]

On Thu, Dec 15, 2022 at 06:57:44PM -0500, Demi Marie Obenour wrote:
> get_page_from_l1e() relied on Xen's choice of PAT, which is brittle in
> the face of future PAT changes.  Instead, compute the actual cacheability
> used by the CPU and switch on that, as this will work no matter what PAT
> Xen uses.
> 
> No functional change intended.
> 
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> ---
> Changes since v3:
> - Compute and use the actual cacheability as seen by the processor.
> 
> Changes since v2:
> - Improve commit message.
> ---
>  xen/arch/x86/include/asm/processor.h |  8 ++++++++
>  xen/arch/x86/mm.c                    | 19 +++++++++++++------
>  2 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h
> index 8e2816fae9b97bd4e153a30cc3802971fe0355af..c3205ed9e10c1a57d23c5ecead66bebd82d87d06 100644
> --- a/xen/arch/x86/include/asm/processor.h
> +++ b/xen/arch/x86/include/asm/processor.h
> @@ -100,6 +100,14 @@
>  
>  #ifndef __ASSEMBLY__
>  
> +/* Convert from PAT/PCD/PWT embedded in PTE flags to actual cacheability value */
> +static inline unsigned int pte_flags_to_cacheability(unsigned int flags)
> +{
> +    unsigned int pat_shift = ((flags & _PAGE_PAT) >> 2) |
> +                             (flags & (_PAGE_PCD|_PAGE_PWT));

This could also be written as

    unsigned int pat_shift = pte_flags_to_cacheattr(flags) << 3;

which might be easiser to read.

> +    return 0xFF & (XEN_MSR_PAT >> pat_shift);
> +}
> +
>  struct domain;
>  struct vcpu;
>  
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 78b1972e4170cacccc9c37c6e64e76e66a7da87f..802073a01c5cf4dc3cf1d58d28ea4d4e9e8149c7 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -959,15 +959,22 @@ get_page_from_l1e(
>              flip = _PAGE_RW;
>          }
>  
> -        switch ( l1f & PAGE_CACHE_ATTRS )
> +        /* Force cacheable memtypes to UC */
> +        switch ( pte_flags_to_cacheability(l1f) )
>          {
> -        case 0: /* WB */
> -            flip |= _PAGE_PWT | _PAGE_PCD;
> +        case X86_MT_UC:
> +        case X86_MT_UCM:
> +        case X86_MT_WC:
> +            /* not cached */
>              break;
> -        case _PAGE_PWT: /* WT */
> -        case _PAGE_PWT | _PAGE_PAT: /* WP */
> -            flip |= _PAGE_PCD | (l1f & _PAGE_PAT);
> +        case X86_MT_WB:
> +        case X86_MT_WT:
> +        case X86_MT_WP:
> +            /* cacheable, force to UC */
> +            flip |= (l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC;
>              break;
> +        default:
> +            BUG();

Question for future reference: is this an appropriate use of BUG()?  The
default case is clearly unreachable, and I actually considered using
unreachable() here, but this isn’t performance-critical code from what I
can tell.  If execution *does* reach here something is seriously wrong.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]