[PATCH 0/8] Make PAT handling less brittle

[PATCH 0/8] Make PAT handling less brittle

[Demi Marie Obenour]

While working on Qubes OS Marek found out that there were some PAT hacks
in the Linux i915 driver.  I decided to make Xen use Linux’s PAT to see
if it solved the graphics glitches that were observed; it did.  This
required a substantial amount of preliminary work that is useful even
without using Linux’s PAT.

Patches 1 through 7 are the preliminary work and I would like them to be
accepted into upstream Xen.  Patch 7 does technically break ABI by
rejecting the unused PAT entries in debug builds, but as release builds
are not impacted I suspect it is not a serious concern.  Patch 8
actually switches to Linux’s PAT and is NOT intended to be merged (at
least for now) as it would at a minimum break migration of PV guests
from hosts that do not have the patch.

Demi Marie Obenour (8):
  x86/mm: Avoid hard-coding PAT in get_page_from_l1e()
  p2m-pt: Avoid hard-coding Xen's PAT
  x86/mm/shadow: avoid assuming a specific Xen PAT
  efi: Avoid hard-coding the various PAT constants
  x86/mm/shadow: do not open-code PAGE_CACHE_ATTRS
  x86: Derive XEN_MSR_PAT from its individual entries
  x86/mm: make code robust to future PAT changes
  Use Linux's PAT

 xen/arch/x86/include/asm/page.h      |  4 +-
 xen/arch/x86/include/asm/processor.h | 22 +++++++++-
 xen/arch/x86/mm.c                    | 65 ++++++++++++++++++++++++----
 xen/arch/x86/mm/p2m-pt.c             |  6 +--
 xen/arch/x86/mm/shadow/multi.c       |  8 ++--
 xen/common/efi/boot.c                | 10 ++---
 6 files changed, 91 insertions(+), 24 deletions(-)

-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH 1/8] x86/mm: Avoid hard-coding PAT in get_page_from_l1e()

[Demi Marie Obenour]

This still hard-codes the assumption that the two spare values are
mapped to UC.  Removing this assumption would require a more complex
patch.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 xen/arch/x86/mm.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 78b1972e4170cacccc9c37c6e64e76e66a7da87f..5d05399c3a841bf03991a3bed63df9a815c1e891 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -961,13 +961,10 @@ get_page_from_l1e(
 
         switch ( l1f & PAGE_CACHE_ATTRS )
         {
-        case 0: /* WB */
-            flip |= _PAGE_PWT | _PAGE_PCD;
-            break;
-        case _PAGE_PWT: /* WT */
-        case _PAGE_PWT | _PAGE_PAT: /* WP */
-            flip |= _PAGE_PCD | (l1f & _PAGE_PAT);
-            break;
+        case _PAGE_WB:
+        case _PAGE_WT:
+        case _PAGE_WP:
+            flip |= (l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC;
         }
 
         return flip;
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH 2/8] p2m-pt: Avoid hard-coding Xen's PAT

[Demi Marie Obenour]

This makes the code much easier to understand.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 xen/arch/x86/mm/p2m-pt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index eaba2b0fb4e6830f52b7d112fba8175dfe6d2770..cd1af33b6772ab1016e8d4c3284a6bc5d282869d 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -99,13 +99,13 @@ static unsigned long p2m_type_to_flags(const struct p2m_domain *p2m,
         return flags | P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
     case p2m_mmio_direct:
         if ( !rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn)) )
-            flags |= _PAGE_RW;
+            flags |= _PAGE_RW | _PAGE_UCM;
         else
         {
-            flags |= _PAGE_PWT;
+            flags |= _PAGE_UC;
             ASSERT(!level);
         }
-        return flags | P2M_BASE_FLAGS | _PAGE_PCD;
+        return flags | P2M_BASE_FLAGS;
     }
 }
 
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH 3/8] x86/mm/shadow: avoid assuming a specific Xen PAT

[Demi Marie Obenour]

This makes the code easier to understand and more robust if Xen's PAT
ever changes.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 xen/arch/x86/mm/shadow/multi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 2370b3060285fee895f335f2a82d3d22ca5d31ed..4e94fec3d50cde0e5a26ecb62ff4d00dd00f759d 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -629,8 +629,8 @@ _sh_propagate(struct vcpu *v,
     else if ( p2mt == p2m_mmio_direct &&
               rangeset_contains_singleton(mmio_ro_ranges, mfn_x(target_mfn)) )
     {
-        sflags &= ~(_PAGE_RW | _PAGE_PAT);
-        sflags |= _PAGE_PCD | _PAGE_PWT;
+        sflags &= ~(_PAGE_RW | PAGE_CACHE_ATTRS);
+        sflags |= _PAGE_UC;
     }
 
     // protect guest page tables
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH 4/8] efi: Avoid hard-coding the various PAT constants

[Demi Marie Obenour]

This makes the code much easier to understand, and avoids problems if
Xen's PAT ever changes in the future.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 xen/common/efi/boot.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 8e880fe30c7541a202dec3e665300d6549953aa3..260997b251b09dae4b48c1b1db665778e02d760a 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1746,21 +1746,21 @@ void __init efi_init_memory(void)
         if ( desc->Attribute & EFI_MEMORY_WB )
             /* nothing */;
         else if ( desc->Attribute & EFI_MEMORY_WT )
-            prot |= _PAGE_PWT | MAP_SMALL_PAGES;
+            prot |= _PAGE_WT | MAP_SMALL_PAGES;
         else if ( desc->Attribute & EFI_MEMORY_WC )
-            prot |= _PAGE_PAT | MAP_SMALL_PAGES;
+            prot |= _PAGE_WC | MAP_SMALL_PAGES;
         else if ( desc->Attribute & (EFI_MEMORY_UC | EFI_MEMORY_UCE) )
-            prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
+            prot |= _PAGE_UC | MAP_SMALL_PAGES;
         else if ( efi_bs_revision >= EFI_REVISION(2, 5) &&
                   (desc->Attribute & EFI_MEMORY_WP) )
-            prot |= _PAGE_PAT | _PAGE_PWT | MAP_SMALL_PAGES;
+            prot |= _PAGE_WP | MAP_SMALL_PAGES;
         else
         {
             printk(XENLOG_ERR "Unknown cachability for MFNs %#lx-%#lx%s\n",
                    smfn, emfn - 1, efi_map_uc ? ", assuming UC" : "");
             if ( !efi_map_uc )
                 continue;
-            prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
+            prot |= _PAGE_UC | MAP_SMALL_PAGES;
         }
 
         if ( desc->Attribute & (efi_bs_revision < EFI_REVISION(2, 5)
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH 5/8] x86/mm/shadow: do not open-code PAGE_CACHE_ATTRS

[Demi Marie Obenour]

This makes the code easier to understand.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 xen/arch/x86/mm/shadow/multi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 4e94fec3d50cde0e5a26ecb62ff4d00dd00f759d..6bb564b0145285afc93b72a60b7797fcfe8696dc 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -535,7 +535,7 @@ _sh_propagate(struct vcpu *v,
     if ( guest_nx_enabled(v) )
         pass_thru_flags |= _PAGE_NX_BIT;
     if ( level == 1 && !shadow_mode_refcounts(d) && mmio_mfn )
-        pass_thru_flags |= _PAGE_PAT | _PAGE_PCD | _PAGE_PWT;
+        pass_thru_flags |= PAGE_CACHE_ATTRS;
     sflags = gflags & pass_thru_flags;
 
     /*
@@ -548,7 +548,7 @@ _sh_propagate(struct vcpu *v,
     {
         int type;
 
-        ASSERT(!(sflags & (_PAGE_PAT | _PAGE_PCD | _PAGE_PWT)));
+        ASSERT(!(sflags & PAGE_CACHE_ATTRS));
 
         /* compute the PAT index for shadow page entry when VT-d is enabled
          * and device assigned.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH 6/8] x86: Derive XEN_MSR_PAT from its individual entries

[Demi Marie Obenour]

This avoids it being a magic constant that is difficult for humans to
decode.  Use a _Static_assert to check that the old and new values are
identical.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 xen/arch/x86/include/asm/processor.h | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h
index 8e2816fae9b97bd4e153a30cc3802971fe0355af..64b75e444947c64e2e5eba457deec92a873d7a63 100644
--- a/xen/arch/x86/include/asm/processor.h
+++ b/xen/arch/x86/include/asm/processor.h
@@ -92,13 +92,33 @@
                           X86_EFLAGS_NT|X86_EFLAGS_DF|X86_EFLAGS_IF|    \
                           X86_EFLAGS_TF)
 
+/* Individual entries in IA32_CR_PAT */
+#define MSR_PAT_UC  _AC(0x00, ULL)
+#define MSR_PAT_WC  _AC(0x01, ULL)
+#define MSR_PAT_RESERVED_1  _AC(0x02, ULL)
+#define MSR_PAT_RESERVED_2  _AC(0x03, ULL)
+#define MSR_PAT_WT  _AC(0x04, ULL)
+#define MSR_PAT_WP  _AC(0x05, ULL)
+#define MSR_PAT_WB  _AC(0x06, ULL)
+#define MSR_PAT_UCM _AC(0x07, ULL)
+
 /*
  * Host IA32_CR_PAT value to cover all memory types.  This is not the default
  * MSR_PAT value, and is an ABI with PV guests.
  */
-#define XEN_MSR_PAT _AC(0x050100070406, ULL)
+#define XEN_MSR_PAT (MSR_PAT_WB  << 0x00 | \
+                     MSR_PAT_WT  << 0x08 | \
+                     MSR_PAT_UCM << 0x10 | \
+                     MSR_PAT_UC  << 0x18 | \
+                     MSR_PAT_WC  << 0x20 | \
+                     MSR_PAT_WP  << 0x28 | \
+                     MSR_PAT_UC  << 0x30 | \
+                     MSR_PAT_UC  << 0x38 | \
+                     0)
 
 #ifndef __ASSEMBLY__
+_Static_assert(XEN_MSR_PAT == 0x050100070406ULL,
+               "wrong XEN_MSR_PAT breaks PV guests");
 
 struct domain;
 struct vcpu;
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH 7/8] x86/mm: make code robust to future PAT changes

[Demi Marie Obenour]

It may be desirable to change Xen's PAT for various reasons.  This
requires changes to several _PAGE_* macros as well.  Add static
assertions to check that XEN_MSR_PAT is consistent with the _PAGE_*
macros.

Additionally, Xen has two unused entries in the PAT.  Currently these
are UC, but this will change if the hardware ever supports additional
memory types.  To avoid future problems, this adds a check in debug
builds that injects #GP into a guest that tries to use one of these
entries, along with returning -EINVAL from the hypercall.  Future
versions of Xen will refuse to use these entries even in release builds.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 xen/arch/x86/mm.c | 58 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 54 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 5d05399c3a841bf03991a3bed63df9a815c1e891..517fccee699b2a673ba537e47933aefc80017aa5 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -849,6 +849,45 @@ static int cf_check print_mmio_emul_range(
 }
 #endif
 
+static void __init __maybe_unused build_assertions(void)
+{
+    /* A bunch of static assertions to check that the XEN_MSR_PAT is valid
+     * and consistent with the _PAGE_* macros */
+#define PAT_VALUE(v) (0xFF & (XEN_MSR_PAT >> (8 * (v))))
+#define BAD_VALUE(v) ((v) < 0 || (v) > 7 ||                                    \
+                      (v) == MSR_PAT_RESERVED_1 || (v) == MSR_PAT_RESERVED_2)
+#define BAD_PAT_VALUE(v) BUILD_BUG_ON(BAD_VALUE(PAT_VALUE(v)))
+    BAD_PAT_VALUE(0);
+    BAD_PAT_VALUE(1);
+    BAD_PAT_VALUE(2);
+    BAD_PAT_VALUE(3);
+    BAD_PAT_VALUE(4);
+    BAD_PAT_VALUE(5);
+    BAD_PAT_VALUE(6);
+    BAD_PAT_VALUE(7);
+#undef BAD_PAT_VALUE
+#undef BAD_VALUE
+#define PAT_SHIFT(page_value) (((page_value) & _PAGE_PAT) >> 5 |               \
+                               ((page_value) & (_PAGE_PCD | _PAGE_PWT)) >> 3)
+#define CHECK_PAGE_VALUE(page_value) do {                                      \
+    /* Check that the _PAGE_* macros only use bits from PAGE_CACHE_ATTRS */    \
+    BUILD_BUG_ON(((_PAGE_##page_value) & PAGE_CACHE_ATTRS) !=                  \
+                  (_PAGE_##page_value));                                       \
+    /* Check that the _PAGE_* are consistent with XEN_MSR_PAT */               \
+    BUILD_BUG_ON(PAT_VALUE(PAT_SHIFT(_PAGE_##page_value)) !=                   \
+                 (MSR_PAT_##page_value));                                      \
+} while (0)
+    CHECK_PAGE_VALUE(WT);
+    CHECK_PAGE_VALUE(WB);
+    CHECK_PAGE_VALUE(WC);
+    CHECK_PAGE_VALUE(UC);
+    CHECK_PAGE_VALUE(UCM);
+    CHECK_PAGE_VALUE(WP);
+#undef CHECK_PAGE_VALUE
+#undef PAT_SHIFT
+#undef PAT_VALUE
+}
+
 /*
  * get_page_from_l1e returns:
  *   0  => success (page not present also counts as such)
@@ -961,13 +1000,24 @@ get_page_from_l1e(
 
         switch ( l1f & PAGE_CACHE_ATTRS )
         {
-        case _PAGE_WB:
+        default:
+#ifndef NDEBUG
+            printk(XENLOG_G_WARNING
+                   "d%d: Guest tried to use bad cachability attribute %u for MFN %lx\n",
+                   l1e_owner->domain_id, l1f & PAGE_CACHE_ATTRS, mfn);
+            pv_inject_hw_exception(TRAP_gp_fault, 0);
+            return -EINVAL;
+#endif
         case _PAGE_WT:
         case _PAGE_WP:
-            flip |= (l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC;
+        case _PAGE_WB:
+            /* Force this to be uncachable */
+            return flip | ( (l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC );
+        case _PAGE_WC:
+        case _PAGE_UC:
+        case _PAGE_UCM:
+            return flip;
         }
-
-        return flip;
     }
 
     if ( unlikely((real_pg_owner != pg_owner) &&
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[RFC PATCH 8/8] Use Linux's PAT

[Demi Marie Obenour]

This is purely for testing, to see if it works around a bug in i915.  It
is not intended to be merged.

NOT-signed-off-by: DO NOT MERGE
---
 xen/arch/x86/include/asm/page.h      |  4 ++--
 xen/arch/x86/include/asm/processor.h | 12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/include/asm/page.h b/xen/arch/x86/include/asm/page.h
index b585235d064a567082582c8e92a4e8283fd949ca..ab9b46f1d0901e50a83fd035ff28d1bda0b781a2 100644
--- a/xen/arch/x86/include/asm/page.h
+++ b/xen/arch/x86/include/asm/page.h
@@ -333,11 +333,11 @@ void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t);
 
 /* Memory types, encoded under Xen's choice of MSR_PAT. */
 #define _PAGE_WB         (                                0)
-#define _PAGE_WT         (                        _PAGE_PWT)
+#define _PAGE_WC         (                        _PAGE_PWT)
 #define _PAGE_UCM        (            _PAGE_PCD            )
 #define _PAGE_UC         (            _PAGE_PCD | _PAGE_PWT)
-#define _PAGE_WC         (_PAGE_PAT                        )
 #define _PAGE_WP         (_PAGE_PAT |             _PAGE_PWT)
+#define _PAGE_WT         (_PAGE_PAT | _PAGE_PCD | _PAGE_PWT)
 
 /*
  * Debug option: Ensure that granted mappings are not implicitly unmapped.
diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h
index 64b75e444947c64e2e5eba457deec92a873d7a63..7a27d19d1dc8aa9c102683985c30fb85864303fa 100644
--- a/xen/arch/x86/include/asm/processor.h
+++ b/xen/arch/x86/include/asm/processor.h
@@ -107,18 +107,18 @@
  * MSR_PAT value, and is an ABI with PV guests.
  */
 #define XEN_MSR_PAT (MSR_PAT_WB  << 0x00 | \
-                     MSR_PAT_WT  << 0x08 | \
+                     MSR_PAT_WC  << 0x08 | \
                      MSR_PAT_UCM << 0x10 | \
                      MSR_PAT_UC  << 0x18 | \
-                     MSR_PAT_WC  << 0x20 | \
+                     MSR_PAT_WB  << 0x20 | \
                      MSR_PAT_WP  << 0x28 | \
-                     MSR_PAT_UC  << 0x30 | \
-                     MSR_PAT_UC  << 0x38 | \
+                     MSR_PAT_UCM << 0x30 | \
+                     MSR_PAT_WT  << 0x38 | \
                      0)
 
 #ifndef __ASSEMBLY__
-_Static_assert(XEN_MSR_PAT == 0x050100070406ULL,
-               "wrong XEN_MSR_PAT breaks PV guests");
+_Static_assert(XEN_MSR_PAT == 0x0407050600070106ULL,
+               "wrong XEN_MSR_PAT breaks i915 driver on PV Linux");
 
 struct domain;
 struct vcpu;
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Re: [PATCH 1/8] x86/mm: Avoid hard-coding PAT in get_page_from_l1e()

[Andrew Cooper]

On 06/12/2022 04:33, Demi Marie Obenour wrote:
> This still hard-codes the assumption that the two spare values are
> mapped to UC.  Removing this assumption would require a more complex
> patch.
>
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  xen/arch/x86/mm.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 78b1972e4170cacccc9c37c6e64e76e66a7da87f..5d05399c3a841bf03991a3bed63df9a815c1e891 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -961,13 +961,10 @@ get_page_from_l1e(
>  

As you noted that this desperately needs a comment, how about

/* Force cacheable memtypes to UC, */

here?  Can fix up on commit.

~Andrew

>          switch ( l1f & PAGE_CACHE_ATTRS )
>          {
> -        case 0: /* WB */
> -            flip |= _PAGE_PWT | _PAGE_PCD;
> -            break;
> -        case _PAGE_PWT: /* WT */
> -        case _PAGE_PWT | _PAGE_PAT: /* WP */
> -            flip |= _PAGE_PCD | (l1f & _PAGE_PAT);
> -            break;
> +        case _PAGE_WB:
> +        case _PAGE_WT:
> +        case _PAGE_WP:
> +            flip |= (l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC;
>          }
>  
>          return flip;

Re: [PATCH 2/8] p2m-pt: Avoid hard-coding Xen's PAT

[Andrew Cooper]

On 06/12/2022 04:33, Demi Marie Obenour wrote:
> This makes the code much easier to understand.
>
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> ---
>  xen/arch/x86/mm/p2m-pt.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> index eaba2b0fb4e6830f52b7d112fba8175dfe6d2770..cd1af33b6772ab1016e8d4c3284a6bc5d282869d 100644
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -99,13 +99,13 @@ static unsigned long p2m_type_to_flags(const struct p2m_domain *p2m,
>          return flags | P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
>      case p2m_mmio_direct:
>          if ( !rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn)) )
> -            flags |= _PAGE_RW;
> +            flags |= _PAGE_RW | _PAGE_UCM;
>          else
>          {
> -            flags |= _PAGE_PWT;
> +            flags |= _PAGE_UC;
>              ASSERT(!level);
>          }
> -        return flags | P2M_BASE_FLAGS | _PAGE_PCD;
> +        return flags | P2M_BASE_FLAGS;

I agree that this is a correct transformation of the logic, but the
logic cannot possibly be correct in the first place.

The read-only-ness of the MMIO range has no legitimate bearing on UC vs
UC-.  I have a feeling this is another attempt to control mixed
cacheability - the HVM side cleanup from XSA-402 is still very much pending.

I'm tempted to R-by and commit the patch, with a note in the commit
message saying that the transformation is correct but that it highlights
that the pre-existing logic is suspect.

~Andrew

Re: [PATCH 3/8] x86/mm/shadow: avoid assuming a specific Xen PAT

[Andrew Cooper]

On 06/12/2022 04:33, Demi Marie Obenour wrote:
> This makes the code easier to understand and more robust if Xen's PAT
> ever changes.
>
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 1/8] x86/mm: Avoid hard-coding PAT in get_page_from_l1e()

[Jan Beulich]

On 06.12.2022 05:33, Demi Marie Obenour wrote:
> This still hard-codes the assumption that the two spare values are
> mapped to UC.  Removing this assumption would require a more complex
> patch.
> 
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>

You eliminate some assumptions at the price of introducing new ones.
With that I consider the description insufficient; really there isn't
any description here, just a statement on something you do _not_ do.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -961,13 +961,10 @@ get_page_from_l1e(
>  
>          switch ( l1f & PAGE_CACHE_ATTRS )
>          {
> -        case 0: /* WB */
> -            flip |= _PAGE_PWT | _PAGE_PCD;
> -            break;
> -        case _PAGE_PWT: /* WT */
> -        case _PAGE_PWT | _PAGE_PAT: /* WP */
> -            flip |= _PAGE_PCD | (l1f & _PAGE_PAT);
> -            break;
> +        case _PAGE_WB:
> +        case _PAGE_WT:
> +        case _PAGE_WP:
> +            flip |= (l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC;
>          }

Please never drop "break" from case blocks, even if it's (at a given
point in time) the last in its switch(). That's a recipe for someone
else then forgetting to add it when another case block is added.

Jan

Re: [PATCH 1/8] x86/mm: Avoid hard-coding PAT in get_page_from_l1e()

[Jan Beulich]

On 06.12.2022 11:42, Andrew Cooper wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -961,13 +961,10 @@ get_page_from_l1e(
>>  
> 
> As you noted that this desperately needs a comment, how about
> 
> /* Force cacheable memtypes to UC, */
> 
> here?

The comment you appear to be missing is actually there, just perhaps a
bit too far ahead:

            /* MMIO pages must not be mapped cachable unless requested so. */

In any event the comment you suggest to add merely re-states in different
words what this earlier one already says.

Jan

Re: [PATCH 2/8] p2m-pt: Avoid hard-coding Xen's PAT

[Jan Beulich]

On 06.12.2022 11:59, Andrew Cooper wrote:
> On 06/12/2022 04:33, Demi Marie Obenour wrote:
>> This makes the code much easier to understand.
>>
>> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
>> ---
>>  xen/arch/x86/mm/p2m-pt.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
>> index eaba2b0fb4e6830f52b7d112fba8175dfe6d2770..cd1af33b6772ab1016e8d4c3284a6bc5d282869d 100644
>> --- a/xen/arch/x86/mm/p2m-pt.c
>> +++ b/xen/arch/x86/mm/p2m-pt.c
>> @@ -99,13 +99,13 @@ static unsigned long p2m_type_to_flags(const struct p2m_domain *p2m,
>>          return flags | P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
>>      case p2m_mmio_direct:
>>          if ( !rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn)) )
>> -            flags |= _PAGE_RW;
>> +            flags |= _PAGE_RW | _PAGE_UCM;
>>          else
>>          {
>> -            flags |= _PAGE_PWT;
>> +            flags |= _PAGE_UC;
>>              ASSERT(!level);
>>          }
>> -        return flags | P2M_BASE_FLAGS | _PAGE_PCD;
>> +        return flags | P2M_BASE_FLAGS;
> 
> I agree that this is a correct transformation of the logic, but the
> logic cannot possibly be correct in the first place.
> 
> The read-only-ness of the MMIO range has no legitimate bearing on UC vs
> UC-.  I have a feeling this is another attempt to control mixed
> cacheability

I think so, yes - at the time the goal was to disallow cachability other
than the one used by Xen for those pages where we know Xen has a mapping.

Jan

> - the HVM side cleanup from XSA-402 is still very much pending.
> 
> I'm tempted to R-by and commit the patch, with a note in the commit
> message saying that the transformation is correct but that it highlights
> that the pre-existing logic is suspect.
> 
> ~Andrew

Re: [PATCH 4/8] efi: Avoid hard-coding the various PAT constants

[Jan Beulich]

On 06.12.2022 05:33, Demi Marie Obenour wrote:
> This makes the code much easier to understand, and avoids problems if
> Xen's PAT ever changes in the future.
> 
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH 4/8] efi: Avoid hard-coding the various PAT constants

[Andrew Cooper]

On 06/12/2022 04:33, Demi Marie Obenour wrote:
> This makes the code much easier to understand, and avoids problems if
> Xen's PAT ever changes in the future.
>
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> ---
>  xen/common/efi/boot.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 8e880fe30c7541a202dec3e665300d6549953aa3..260997b251b09dae4b48c1b1db665778e02d760a 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1746,21 +1746,21 @@ void __init efi_init_memory(void)
>          if ( desc->Attribute & EFI_MEMORY_WB )
>              /* nothing */;

This is an implicit 0 case which wants changing to _PAGE_WB.

>          else if ( desc->Attribute & EFI_MEMORY_WT )
> -            prot |= _PAGE_PWT | MAP_SMALL_PAGES;
> +            prot |= _PAGE_WT | MAP_SMALL_PAGES;
>          else if ( desc->Attribute & EFI_MEMORY_WC )
> -            prot |= _PAGE_PAT | MAP_SMALL_PAGES;
> +            prot |= _PAGE_WC | MAP_SMALL_PAGES;
>          else if ( desc->Attribute & (EFI_MEMORY_UC | EFI_MEMORY_UCE) )
> -            prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
> +            prot |= _PAGE_UC | MAP_SMALL_PAGES;
>          else if ( efi_bs_revision >= EFI_REVISION(2, 5) &&
>                    (desc->Attribute & EFI_MEMORY_WP) )
> -            prot |= _PAGE_PAT | _PAGE_PWT | MAP_SMALL_PAGES;
> +            prot |= _PAGE_WP | MAP_SMALL_PAGES;

Unrelated to the transformation. I'm unconvinced about the correctness
of using MAP_SMALL_PAGES here.  There's nothing wrong with large pages
of reduced cache-ability, and the framebuffer is going to live in one of
these regions, probably a WC one...

~Andrew

>          else
>          {
>              printk(XENLOG_ERR "Unknown cachability for MFNs %#lx-%#lx%s\n",
>                     smfn, emfn - 1, efi_map_uc ? ", assuming UC" : "");
>              if ( !efi_map_uc )
>                  continue;
> -            prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
> +            prot |= _PAGE_UC | MAP_SMALL_PAGES;
>          }
>  
>          if ( desc->Attribute & (efi_bs_revision < EFI_REVISION(2, 5)

Re: [PATCH 5/8] x86/mm/shadow: do not open-code PAGE_CACHE_ATTRS

[Jan Beulich]

On 06.12.2022 05:33, Demi Marie Obenour wrote:
> This makes the code easier to understand.

Personally I'm not entirely sure about this, but the elimination of
open-coding is worthwhile in any event.

> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH 6/8] x86: Derive XEN_MSR_PAT from its individual entries

[Andrew Cooper]

On 06/12/2022 04:33, Demi Marie Obenour wrote:
> This avoids it being a magic constant that is difficult for humans to
> decode.  Use a _Static_assert to check that the old and new values are
> identical.
>
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> ---
>  xen/arch/x86/include/asm/processor.h | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h
> index 8e2816fae9b97bd4e153a30cc3802971fe0355af..64b75e444947c64e2e5eba457deec92a873d7a63 100644
> --- a/xen/arch/x86/include/asm/processor.h
> +++ b/xen/arch/x86/include/asm/processor.h
> @@ -92,13 +92,33 @@
>                            X86_EFLAGS_NT|X86_EFLAGS_DF|X86_EFLAGS_IF|    \
>                            X86_EFLAGS_TF)
>  
> +/* Individual entries in IA32_CR_PAT */
> +#define MSR_PAT_UC  _AC(0x00, ULL)
> +#define MSR_PAT_WC  _AC(0x01, ULL)
> +#define MSR_PAT_RESERVED_1  _AC(0x02, ULL)
> +#define MSR_PAT_RESERVED_2  _AC(0x03, ULL)
> +#define MSR_PAT_WT  _AC(0x04, ULL)
> +#define MSR_PAT_WP  _AC(0x05, ULL)
> +#define MSR_PAT_WB  _AC(0x06, ULL)
> +#define MSR_PAT_UCM _AC(0x07, ULL)

This isn't really correct.  Constants for MSRs typically live in
msr-index.h, but these are architectural x86 memory types.

These ought be

#define X86_MT_$X ...  (skipping the two reserved values)

in x86-defns.h, and the PAT_TYPE_*, MTRR_TYPE_* and EPT_EMT_* constants
want removing.

There are two minor restrictions (EPT can't have UCM, MTRR can't have
WC), but they are all operating in terms of architectural memory type
values, and the code ought to reflect this.

> +
>  /*
>   * Host IA32_CR_PAT value to cover all memory types.  This is not the default
>   * MSR_PAT value, and is an ABI with PV guests.
>   */
> -#define XEN_MSR_PAT _AC(0x050100070406, ULL)
> +#define XEN_MSR_PAT (MSR_PAT_WB  << 0x00 | \
> +                     MSR_PAT_WT  << 0x08 | \
> +                     MSR_PAT_UCM << 0x10 | \
> +                     MSR_PAT_UC  << 0x18 | \
> +                     MSR_PAT_WC  << 0x20 | \
> +                     MSR_PAT_WP  << 0x28 | \
> +                     MSR_PAT_UC  << 0x30 | \
> +                     MSR_PAT_UC  << 0x38 | \
> +                     0)
>  
>  #ifndef __ASSEMBLY__
> +_Static_assert(XEN_MSR_PAT == 0x050100070406ULL,
> +               "wrong XEN_MSR_PAT breaks PV guests");

This wants to be in the build_assertions() that you introduce in the
next patch, and a BUILD_BUG_ON().  We still support compilers which
don't know _Static_assert().

~Andrew

Re: [PATCH 6/8] x86: Derive XEN_MSR_PAT from its individual entries

[Jan Beulich]

On 06.12.2022 05:33, Demi Marie Obenour wrote:
> --- a/xen/arch/x86/include/asm/processor.h
> +++ b/xen/arch/x86/include/asm/processor.h
> @@ -92,13 +92,33 @@
>                            X86_EFLAGS_NT|X86_EFLAGS_DF|X86_EFLAGS_IF|    \
>                            X86_EFLAGS_TF)
>  
> +/* Individual entries in IA32_CR_PAT */
> +#define MSR_PAT_UC  _AC(0x00, ULL)
> +#define MSR_PAT_WC  _AC(0x01, ULL)
> +#define MSR_PAT_RESERVED_1  _AC(0x02, ULL)
> +#define MSR_PAT_RESERVED_2  _AC(0x03, ULL)
> +#define MSR_PAT_WT  _AC(0x04, ULL)
> +#define MSR_PAT_WP  _AC(0x05, ULL)
> +#define MSR_PAT_WB  _AC(0x06, ULL)
> +#define MSR_PAT_UCM _AC(0x07, ULL)

This would be the at least 4th instance of these constants in our tree
(see xen/arch/x86/include/asm/mtrr.h, xen/include/public/domctl.h, and
xen/include/public/hvm/dm_op.h). Since they're part of the public
interface, I'm inclined to suggest to use those. If that faces
opposition, the next best approach would apparently be to move mtrr.h's
into x86-defns.h (including the largely identical MTRR values), thus
then also allowing hvmloader sources (which currently open-code the
MTRR values in cacheattr.c).

>  /*
>   * Host IA32_CR_PAT value to cover all memory types.  This is not the default
>   * MSR_PAT value, and is an ABI with PV guests.
>   */
> -#define XEN_MSR_PAT _AC(0x050100070406, ULL)
> +#define XEN_MSR_PAT (MSR_PAT_WB  << 0x00 | \
> +                     MSR_PAT_WT  << 0x08 | \
> +                     MSR_PAT_UCM << 0x10 | \
> +                     MSR_PAT_UC  << 0x18 | \
> +                     MSR_PAT_WC  << 0x20 | \
> +                     MSR_PAT_WP  << 0x28 | \
> +                     MSR_PAT_UC  << 0x30 | \
> +                     MSR_PAT_UC  << 0x38 | \
> +                     0)
>  
>  #ifndef __ASSEMBLY__
> +_Static_assert(XEN_MSR_PAT == 0x050100070406ULL,
> +               "wrong XEN_MSR_PAT breaks PV guests");

I'm afraid this won't compile on all gcc versions we support; see
xen/include/xen/lib.h's conditional use thereof for BUILD_BUG_ON().
You really want to use BUILD_BUG_ON() here instead of partly open-
coding it. (Yes, it requires to be put where a statement can be
put, not just a declaration, but the check doesn't strictly need
to live in a header file, so that's reasonably easy to accommodate
for.)

Jan

Re: [RFC PATCH 8/8] Use Linux's PAT

[Andrew Cooper]

On 06/12/2022 04:33, Demi Marie Obenour wrote:
> This is purely for testing, to see if it works around a bug in i915.  It
> is not intended to be merged.
>
> NOT-signed-off-by: DO NOT MERGE

Following up on Marek's report on IRC/Matrix, you're saying that this
change does actually fix screen corruption issues on AlderLake, and
something on TigerLake too?

If that is actually the case, then one of two things is happening.  Either,

1) Drivers in Linux are bypassing the regular caching APIs, or
2) The translation logic between Linux's idea of cacheability and Xen's
PAT values is buggy.

~Andrew

Re: [PATCH 4/8] efi: Avoid hard-coding the various PAT constants

[Jan Beulich]

On 06.12.2022 12:17, Andrew Cooper wrote:
> On 06/12/2022 04:33, Demi Marie Obenour wrote:
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -1746,21 +1746,21 @@ void __init efi_init_memory(void)
>>          if ( desc->Attribute & EFI_MEMORY_WB )
>>              /* nothing */;
> 
> This is an implicit 0 case which wants changing to _PAGE_WB.

Oh, yes. Demi, feel free to retain my R-b with the adjustment.

>>          else if ( desc->Attribute & EFI_MEMORY_WT )
>> -            prot |= _PAGE_PWT | MAP_SMALL_PAGES;
>> +            prot |= _PAGE_WT | MAP_SMALL_PAGES;
>>          else if ( desc->Attribute & EFI_MEMORY_WC )
>> -            prot |= _PAGE_PAT | MAP_SMALL_PAGES;
>> +            prot |= _PAGE_WC | MAP_SMALL_PAGES;
>>          else if ( desc->Attribute & (EFI_MEMORY_UC | EFI_MEMORY_UCE) )
>> -            prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
>> +            prot |= _PAGE_UC | MAP_SMALL_PAGES;
>>          else if ( efi_bs_revision >= EFI_REVISION(2, 5) &&
>>                    (desc->Attribute & EFI_MEMORY_WP) )
>> -            prot |= _PAGE_PAT | _PAGE_PWT | MAP_SMALL_PAGES;
>> +            prot |= _PAGE_WP | MAP_SMALL_PAGES;
> 
> Unrelated to the transformation. I'm unconvinced about the correctness
> of using MAP_SMALL_PAGES here.  There's nothing wrong with large pages
> of reduced cache-ability,

Hmm, back in the 32-bit days we needed to be afraid of hardware issues
in that area. Hence we had a global policy of never allowing non-WB
large pages. Maybe we don't need to be concerned anymore ...

> and the framebuffer is going to live in one of these regions, probably
> a WC one...

I very much hope it won't live anywhere there, unless you think of non-
PCI devices supplying framebuffers. As long as it's described by a BAR,
it better wouldn't be covered by a memory map entry.

Jan

Re: [PATCH 6/8] x86: Derive XEN_MSR_PAT from its individual entries

[Jan Beulich]

On 06.12.2022 12:32, Andrew Cooper wrote:
> On 06/12/2022 04:33, Demi Marie Obenour wrote:
>> --- a/xen/arch/x86/include/asm/processor.h
>> +++ b/xen/arch/x86/include/asm/processor.h
>> @@ -92,13 +92,33 @@
>>                            X86_EFLAGS_NT|X86_EFLAGS_DF|X86_EFLAGS_IF|    \
>>                            X86_EFLAGS_TF)
>>  
>> +/* Individual entries in IA32_CR_PAT */
>> +#define MSR_PAT_UC  _AC(0x00, ULL)
>> +#define MSR_PAT_WC  _AC(0x01, ULL)
>> +#define MSR_PAT_RESERVED_1  _AC(0x02, ULL)
>> +#define MSR_PAT_RESERVED_2  _AC(0x03, ULL)
>> +#define MSR_PAT_WT  _AC(0x04, ULL)
>> +#define MSR_PAT_WP  _AC(0x05, ULL)
>> +#define MSR_PAT_WB  _AC(0x06, ULL)
>> +#define MSR_PAT_UCM _AC(0x07, ULL)
> 
> This isn't really correct.  Constants for MSRs typically live in
> msr-index.h, but these are architectural x86 memory types.
> 
> These ought be
> 
> #define X86_MT_$X ...  (skipping the two reserved values)
> 
> in x86-defns.h, and the PAT_TYPE_*, MTRR_TYPE_* and EPT_EMT_* constants
> want removing.
> 
> There are two minor restrictions (EPT can't have UCM, MTRR can't have
> WC), but they are all operating in terms of architectural memory type
> values, and the code ought to reflect this.

The unavailability of UCM is common to MTRR and EPT's EMT. WC, at the
same time, is a valid type to use in both. Which makes sense - EMT
after all merely is replacing what otherwise is expressed by MTRRs.

Jan

Re: [PATCH 7/8] x86/mm: make code robust to future PAT changes

[Jan Beulich]

On 06.12.2022 05:33, Demi Marie Obenour wrote:
> It may be desirable to change Xen's PAT for various reasons.  This
> requires changes to several _PAGE_* macros as well.  Add static
> assertions to check that XEN_MSR_PAT is consistent with the _PAGE_*
> macros.
> 
> Additionally, Xen has two unused entries in the PAT.  Currently these
> are UC, but this will change if the hardware ever supports additional
> memory types.  To avoid future problems, this adds a check in debug
> builds that injects #GP into a guest that tries to use one of these

I realize we already have a case of injecting of #GP from a hypercall
handling path, but I'm afraid this isn't really sane. _If_ #GP was
possible at all for any hypercall, it should imo be raised on the
syscall instruction (or the int $0x82 for compat guests), not on the
instruction following it. Plus, as a major difference, in the
existing case there's no other means to return an error indication.

> entries, along with returning -EINVAL from the hypercall.  Future
> versions of Xen will refuse to use these entries even in release builds.

Mind me asking where you're taking "will" from? (I might view "may" as
appropriate here.)

> @@ -961,13 +1000,24 @@ get_page_from_l1e(
>  
>          switch ( l1f & PAGE_CACHE_ATTRS )
>          {
> -        case _PAGE_WB:
> +        default:
> +#ifndef NDEBUG
> +            printk(XENLOG_G_WARNING
> +                   "d%d: Guest tried to use bad cachability attribute %u for MFN %lx\n",
> +                   l1e_owner->domain_id, l1f & PAGE_CACHE_ATTRS, mfn);
> +            pv_inject_hw_exception(TRAP_gp_fault, 0);
> +            return -EINVAL;
> +#endif
>          case _PAGE_WT:
>          case _PAGE_WP:
> -            flip |= (l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC;
> +        case _PAGE_WB:
> +            /* Force this to be uncachable */
> +            return flip | ( (l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC );
> +        case _PAGE_WC:
> +        case _PAGE_UC:
> +        case _PAGE_UCM:
> +            return flip;
>          }
> -
> -        return flip;
>      }

May I ask that you leave the basic structure here as is, merely adding
the default block you care about and the three case labels which you
need to add to compensate? Also please use %pd in new code logging
domain IDs. I'm also not convinced l1e_owner is the most appropriate
domain to actually blame here, at least with the wording you've chosen
to use.

Jan

Re: [PATCH 7/8] x86/mm: make code robust to future PAT changes

[Andrew Cooper]

On 06/12/2022 04:33, Demi Marie Obenour wrote:
> It may be desirable to change Xen's PAT for various reasons.  This
> requires changes to several _PAGE_* macros as well.  Add static
> assertions to check that XEN_MSR_PAT is consistent with the _PAGE_*
> macros.
>
> Additionally, Xen has two unused entries in the PAT.  Currently these
> are UC, but this will change if the hardware ever supports additional
> memory types.  To avoid future problems, this adds a check in debug
> builds that injects #GP into a guest that tries to use one of these
> entries, along with returning -EINVAL from the hypercall.  Future
> versions of Xen will refuse to use these entries even in release builds.
>
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> ---
>  xen/arch/x86/mm.c | 58 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 5d05399c3a841bf03991a3bed63df9a815c1e891..517fccee699b2a673ba537e47933aefc80017aa5 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -849,6 +849,45 @@ static int cf_check print_mmio_emul_range(
>  }
>  #endif
>  
> +static void __init __maybe_unused build_assertions(void)

This wants to be at the very bottom of the file.  (And also in the
previous patch to remove the _Static_assert())

> +{
> +    /* A bunch of static assertions to check that the XEN_MSR_PAT is valid
> +     * and consistent with the _PAGE_* macros */
> +#define PAT_VALUE(v) (0xFF & (XEN_MSR_PAT >> (8 * (v))))
> +#define BAD_VALUE(v) ((v) < 0 || (v) > 7 ||                                    \
> +                      (v) == MSR_PAT_RESERVED_1 || (v) == MSR_PAT_RESERVED_2)
> +#define BAD_PAT_VALUE(v) BUILD_BUG_ON(BAD_VALUE(PAT_VALUE(v)))
> +    BAD_PAT_VALUE(0);
> +    BAD_PAT_VALUE(1);
> +    BAD_PAT_VALUE(2);
> +    BAD_PAT_VALUE(3);
> +    BAD_PAT_VALUE(4);
> +    BAD_PAT_VALUE(5);
> +    BAD_PAT_VALUE(6);
> +    BAD_PAT_VALUE(7);
> +#undef BAD_PAT_VALUE
> +#undef BAD_VALUE

Given that you've reworked the PAT declaration to be of the form (MT <<
shift), I'm not sure how much value this check is.

> +#define PAT_SHIFT(page_value) (((page_value) & _PAGE_PAT) >> 5 |               \
> +                               ((page_value) & (_PAGE_PCD | _PAGE_PWT)) >> 3)

pte_flags_to_cacheattr()

> +#define CHECK_PAGE_VALUE(page_value) do {                                      \
> +    /* Check that the _PAGE_* macros only use bits from PAGE_CACHE_ATTRS */    \
> +    BUILD_BUG_ON(((_PAGE_##page_value) & PAGE_CACHE_ATTRS) !=                  \
> +                  (_PAGE_##page_value));                                       \
> +    /* Check that the _PAGE_* are consistent with XEN_MSR_PAT */               \
> +    BUILD_BUG_ON(PAT_VALUE(PAT_SHIFT(_PAGE_##page_value)) !=                   \
> +                 (MSR_PAT_##page_value));                                      \
> +} while (0)
> +    CHECK_PAGE_VALUE(WT);
> +    CHECK_PAGE_VALUE(WB);
> +    CHECK_PAGE_VALUE(WC);
> +    CHECK_PAGE_VALUE(UC);
> +    CHECK_PAGE_VALUE(UCM);
> +    CHECK_PAGE_VALUE(WP);
> +#undef CHECK_PAGE_VALUE
> +#undef PAT_SHIFT
> +#undef PAT_VALUE
> +}
> +
>  /*
>   * get_page_from_l1e returns:
>   *   0  => success (page not present also counts as such)
> @@ -961,13 +1000,24 @@ get_page_from_l1e(
>  
>          switch ( l1f & PAGE_CACHE_ATTRS )
>          {
> -        case _PAGE_WB:
> +        default:
> +#ifndef NDEBUG
> +            printk(XENLOG_G_WARNING
> +                   "d%d: Guest tried to use bad cachability attribute %u for MFN %lx\n",
> +                   l1e_owner->domain_id, l1f & PAGE_CACHE_ATTRS, mfn);

%pd.  You absolutely want to convert the PTE bits to a PAT value before
priniting (decimal on a PTE value is useless), and %PRI_mfn.

> +            pv_inject_hw_exception(TRAP_gp_fault, 0);

As I said on IRC, we do want this, but I'm not certain if we can get
away with just enabling it in debug builds.  _PAGE_GNTTAB was ok because
it has always been like that, but there's a non-trivial chance that
there are existing dom0 kernels which violate this constraint.

> +            return -EINVAL;
> +#endif
>          case _PAGE_WT:
>          case _PAGE_WP:
> -            flip |= (l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC;
> +        case _PAGE_WB:
> +            /* Force this to be uncachable */
> +            return flip | ( (l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC );
> +        case _PAGE_WC:
> +        case _PAGE_UC:
> +        case _PAGE_UCM:
> +            return flip;
>          }
> -
> -        return flip;

This wants reworking over Jan's suggestion in patch 1, and modifying to
reduce churn.  (Keep _PAGE_WB in the same order WRT _PAGE_WT, the
uncached memory types should simply break, and default should be at the
end.)

~Andrew

Re: [PATCH 4/8] efi: Avoid hard-coding the various PAT constants

[Demi Marie Obenour]

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

On Tue, Dec 06, 2022 at 11:17:20AM +0000, Andrew Cooper wrote:
> On 06/12/2022 04:33, Demi Marie Obenour wrote:
> > This makes the code much easier to understand, and avoids problems if
> > Xen's PAT ever changes in the future.
> >
> > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > ---
> >  xen/common/efi/boot.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> > index 8e880fe30c7541a202dec3e665300d6549953aa3..260997b251b09dae4b48c1b1db665778e02d760a 100644
> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -1746,21 +1746,21 @@ void __init efi_init_memory(void)
> >          if ( desc->Attribute & EFI_MEMORY_WB )
> >              /* nothing */;
> 
> This is an implicit 0 case which wants changing to _PAGE_WB.

Good catch!  I will make this change in v2, but I also will add
BUILD_BUG_ON(_PAGE_WB), as at least Linux assumes that _PAGE_WB is 0.

> >          else if ( desc->Attribute & EFI_MEMORY_WT )
> > -            prot |= _PAGE_PWT | MAP_SMALL_PAGES;
> > +            prot |= _PAGE_WT | MAP_SMALL_PAGES;
> >          else if ( desc->Attribute & EFI_MEMORY_WC )
> > -            prot |= _PAGE_PAT | MAP_SMALL_PAGES;
> > +            prot |= _PAGE_WC | MAP_SMALL_PAGES;
> >          else if ( desc->Attribute & (EFI_MEMORY_UC | EFI_MEMORY_UCE) )
> > -            prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
> > +            prot |= _PAGE_UC | MAP_SMALL_PAGES;
> >          else if ( efi_bs_revision >= EFI_REVISION(2, 5) &&
> >                    (desc->Attribute & EFI_MEMORY_WP) )
> > -            prot |= _PAGE_PAT | _PAGE_PWT | MAP_SMALL_PAGES;
> > +            prot |= _PAGE_WP | MAP_SMALL_PAGES;
> 
> Unrelated to the transformation. I'm unconvinced about the correctness
> of using MAP_SMALL_PAGES here.  There's nothing wrong with large pages
> of reduced cache-ability, and the framebuffer is going to live in one of
> these regions, probably a WC one...

I can make that a separate patch.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

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

Re: [PATCH 6/8] x86: Derive XEN_MSR_PAT from its individual entries

[Demi Marie Obenour]

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

On Tue, Dec 06, 2022 at 11:32:16AM +0000, Andrew Cooper wrote:
> On 06/12/2022 04:33, Demi Marie Obenour wrote:
> > This avoids it being a magic constant that is difficult for humans to
> > decode.  Use a _Static_assert to check that the old and new values are
> > identical.
> >
> > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > ---
> >  xen/arch/x86/include/asm/processor.h | 22 +++++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h
> > index 8e2816fae9b97bd4e153a30cc3802971fe0355af..64b75e444947c64e2e5eba457deec92a873d7a63 100644
> > --- a/xen/arch/x86/include/asm/processor.h
> > +++ b/xen/arch/x86/include/asm/processor.h
> > @@ -92,13 +92,33 @@
> >                            X86_EFLAGS_NT|X86_EFLAGS_DF|X86_EFLAGS_IF|    \
> >                            X86_EFLAGS_TF)
> >  
> > +/* Individual entries in IA32_CR_PAT */
> > +#define MSR_PAT_UC  _AC(0x00, ULL)
> > +#define MSR_PAT_WC  _AC(0x01, ULL)
> > +#define MSR_PAT_RESERVED_1  _AC(0x02, ULL)
> > +#define MSR_PAT_RESERVED_2  _AC(0x03, ULL)
> > +#define MSR_PAT_WT  _AC(0x04, ULL)
> > +#define MSR_PAT_WP  _AC(0x05, ULL)
> > +#define MSR_PAT_WB  _AC(0x06, ULL)
> > +#define MSR_PAT_UCM _AC(0x07, ULL)
> 
> This isn't really correct.

Do you mean that this in and of itself is buggy, or that the code ought
to be structured differently?

> Constants for MSRs typically live in
> msr-index.h, but these are architectural x86 memory types.
> 
> These ought be
> 
> #define X86_MT_$X ...  (skipping the two reserved values)

I will use the reserved values in BUILD_BUG_ON()s, so I would prefer to
keep (possibly defined somewhere else) if that is okay.

> in x86-defns.h, and the PAT_TYPE_*, MTRR_TYPE_* and EPT_EMT_* constants
> want removing.

This seems like a larger refactor that might belong in a separate patch.

> There are two minor restrictions (EPT can't have UCM, MTRR can't have
> WC), but they are all operating in terms of architectural memory type
> values, and the code ought to reflect this.

That makes sense.

> > +
> >  /*
> >   * Host IA32_CR_PAT value to cover all memory types.  This is not the default
> >   * MSR_PAT value, and is an ABI with PV guests.
> >   */
> > -#define XEN_MSR_PAT _AC(0x050100070406, ULL)
> > +#define XEN_MSR_PAT (MSR_PAT_WB  << 0x00 | \
> > +                     MSR_PAT_WT  << 0x08 | \
> > +                     MSR_PAT_UCM << 0x10 | \
> > +                     MSR_PAT_UC  << 0x18 | \
> > +                     MSR_PAT_WC  << 0x20 | \
> > +                     MSR_PAT_WP  << 0x28 | \
> > +                     MSR_PAT_UC  << 0x30 | \
> > +                     MSR_PAT_UC  << 0x38 | \
> > +                     0)
> >  
> >  #ifndef __ASSEMBLY__
> > +_Static_assert(XEN_MSR_PAT == 0x050100070406ULL,
> > +               "wrong XEN_MSR_PAT breaks PV guests");
> 
> This wants to be in the build_assertions() that you introduce in the
> next patch, and a BUILD_BUG_ON().  We still support compilers which
> don't know _Static_assert().

That’s fair
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

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

Re: [PATCH 7/8] x86/mm: make code robust to future PAT changes

[Demi Marie Obenour]

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

On Tue, Dec 06, 2022 at 12:06:24PM +0000, Andrew Cooper wrote:
> On 06/12/2022 04:33, Demi Marie Obenour wrote:
> > It may be desirable to change Xen's PAT for various reasons.  This
> > requires changes to several _PAGE_* macros as well.  Add static
> > assertions to check that XEN_MSR_PAT is consistent with the _PAGE_*
> > macros.
> >
> > Additionally, Xen has two unused entries in the PAT.  Currently these
> > are UC, but this will change if the hardware ever supports additional
> > memory types.  To avoid future problems, this adds a check in debug
> > builds that injects #GP into a guest that tries to use one of these
> > entries, along with returning -EINVAL from the hypercall.  Future
> > versions of Xen will refuse to use these entries even in release builds.
> >
> > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > ---
> >  xen/arch/x86/mm.c | 58 +++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 54 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > index 5d05399c3a841bf03991a3bed63df9a815c1e891..517fccee699b2a673ba537e47933aefc80017aa5 100644
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -849,6 +849,45 @@ static int cf_check print_mmio_emul_range(
> >  }
> >  #endif
> >  
> > +static void __init __maybe_unused build_assertions(void)
> 
> This wants to be at the very bottom of the file.  (And also in the
> previous patch to remove the _Static_assert())
> 
> > +{
> > +    /* A bunch of static assertions to check that the XEN_MSR_PAT is valid
> > +     * and consistent with the _PAGE_* macros */
> > +#define PAT_VALUE(v) (0xFF & (XEN_MSR_PAT >> (8 * (v))))
> > +#define BAD_VALUE(v) ((v) < 0 || (v) > 7 ||                                    \
> > +                      (v) == MSR_PAT_RESERVED_1 || (v) == MSR_PAT_RESERVED_2)
> > +#define BAD_PAT_VALUE(v) BUILD_BUG_ON(BAD_VALUE(PAT_VALUE(v)))
> > +    BAD_PAT_VALUE(0);
> > +    BAD_PAT_VALUE(1);
> > +    BAD_PAT_VALUE(2);
> > +    BAD_PAT_VALUE(3);
> > +    BAD_PAT_VALUE(4);
> > +    BAD_PAT_VALUE(5);
> > +    BAD_PAT_VALUE(6);
> > +    BAD_PAT_VALUE(7);
> > +#undef BAD_PAT_VALUE
> > +#undef BAD_VALUE
> 
> Given that you've reworked the PAT declaration to be of the form (MT <<
> shift), I'm not sure how much value this check is.

One of my goals with this patch set is that it should be possible to
choose *any* value for XEN_MSR_PAT and for the PAT-related _PAGE_*, with
all bad values caught at compile-time.  This would allow making it a
Kconfig option.

> > +#define PAT_SHIFT(page_value) (((page_value) & _PAGE_PAT) >> 5 |               \
> > +                               ((page_value) & (_PAGE_PCD | _PAGE_PWT)) >> 3)
> 
> pte_flags_to_cacheattr()

That’s a function, not a macro (and for good reason), so it can’t be
used in BUILD_BUG_ON().

> > +#define CHECK_PAGE_VALUE(page_value) do {                                      \
> > +    /* Check that the _PAGE_* macros only use bits from PAGE_CACHE_ATTRS */    \
> > +    BUILD_BUG_ON(((_PAGE_##page_value) & PAGE_CACHE_ATTRS) !=                  \
> > +                  (_PAGE_##page_value));                                       \
> > +    /* Check that the _PAGE_* are consistent with XEN_MSR_PAT */               \
> > +    BUILD_BUG_ON(PAT_VALUE(PAT_SHIFT(_PAGE_##page_value)) !=                   \
> > +                 (MSR_PAT_##page_value));                                      \
> > +} while (0)
> > +    CHECK_PAGE_VALUE(WT);
> > +    CHECK_PAGE_VALUE(WB);
> > +    CHECK_PAGE_VALUE(WC);
> > +    CHECK_PAGE_VALUE(UC);
> > +    CHECK_PAGE_VALUE(UCM);
> > +    CHECK_PAGE_VALUE(WP);
> > +#undef CHECK_PAGE_VALUE
> > +#undef PAT_SHIFT
> > +#undef PAT_VALUE
> > +}
> > +
> >  /*
> >   * get_page_from_l1e returns:
> >   *   0  => success (page not present also counts as such)
> > @@ -961,13 +1000,24 @@ get_page_from_l1e(
> >  
> >          switch ( l1f & PAGE_CACHE_ATTRS )
> >          {
> > -        case _PAGE_WB:
> > +        default:
> > +#ifndef NDEBUG
> > +            printk(XENLOG_G_WARNING
> > +                   "d%d: Guest tried to use bad cachability attribute %u for MFN %lx\n",
> > +                   l1e_owner->domain_id, l1f & PAGE_CACHE_ATTRS, mfn);
> 
> %pd.  You absolutely want to convert the PTE bits to a PAT value before
> priniting (decimal on a PTE value is useless), and %PRI_mfn.

I’ll have to look at the rest of the Xen tree to see how to use this.

> > +            pv_inject_hw_exception(TRAP_gp_fault, 0);
> 
> As I said on IRC, we do want this, but I'm not certain if we can get
> away with just enabling it in debug builds.  _PAGE_GNTTAB was ok because
> it has always been like that, but there's a non-trivial chance that
> there are existing dom0 kernels which violate this constraint.

I chose this approach because it is very simple to implement.  Anything
more complex ought to be in a separate patch, IMO.

> > +            return -EINVAL;
> > +#endif
> >          case _PAGE_WT:
> >          case _PAGE_WP:
> > -            flip |= (l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC;
> > +        case _PAGE_WB:
> > +            /* Force this to be uncachable */
> > +            return flip | ( (l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC );
> > +        case _PAGE_WC:
> > +        case _PAGE_UC:
> > +        case _PAGE_UCM:
> > +            return flip;
> >          }
> > -
> > -        return flip;
> 
> This wants reworking over Jan's suggestion in patch 1, and modifying to
> reduce churn.  (Keep _PAGE_WB in the same order WRT _PAGE_WT, the
> uncached memory types should simply break, and default should be at the
> end.)

I put the default in the middle to keep the preprocessor conditionals
simple and avoid duplication.  I will have the default be treated as
cachable memory.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

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

Re: [RFC PATCH 8/8] Use Linux's PAT

[Demi Marie Obenour]

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

On Tue, Dec 06, 2022 at 11:38:03AM +0000, Andrew Cooper wrote:
> On 06/12/2022 04:33, Demi Marie Obenour wrote:
> > This is purely for testing, to see if it works around a bug in i915.  It
> > is not intended to be merged.
> >
> > NOT-signed-off-by: DO NOT MERGE
> 
> Following up on Marek's report on IRC/Matrix, you're saying that this
> change does actually fix screen corruption issues on AlderLake, and
> something on TigerLake too?

Correct

> If that is actually the case, then one of two things is happening.  Either,
> 
> 1) Drivers in Linux are bypassing the regular caching APIs, or

This would not surprise me at all.

> 2) The translation logic between Linux's idea of cacheability and Xen's
> PAT values is buggy.

How could I check for this?
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

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

Re: [RFC PATCH 8/8] Use Linux's PAT

[Marek Marczykowski-Górecki]

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

On Tue, Dec 06, 2022 at 01:01:41PM -0500, Demi Marie Obenour wrote:
> On Tue, Dec 06, 2022 at 11:38:03AM +0000, Andrew Cooper wrote:
> > On 06/12/2022 04:33, Demi Marie Obenour wrote:
> > > This is purely for testing, to see if it works around a bug in i915.  It
> > > is not intended to be merged.
> > >
> > > NOT-signed-off-by: DO NOT MERGE
> > 
> > Following up on Marek's report on IRC/Matrix, you're saying that this
> > change does actually fix screen corruption issues on AlderLake, and
> > something on TigerLake too?
> 
> Correct
> 
> > If that is actually the case, then one of two things is happening.  Either,
> > 
> > 1) Drivers in Linux are bypassing the regular caching APIs, or
> 
> This would not surprise me at all.
> 
> > 2) The translation logic between Linux's idea of cacheability and Xen's
> > PAT values is buggy.
> 
> How could I check for this?

See Andy's unit test idea on #xendevel:

    as a pretty simple "unit" test in dom0, it might be a good idea to
    have a module which watches the PTE in question, and cycles through
    various of the memremap_*() APIs and checks the raw PTE that gets
    written after Linux and Xen are done fighting with it

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

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

Re: [RFC PATCH 8/8] Use Linux's PAT

[Demi Marie Obenour]

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

On Tue, Dec 06, 2022 at 07:12:06PM +0100, Marek Marczykowski-Górecki wrote:
> On Tue, Dec 06, 2022 at 01:01:41PM -0500, Demi Marie Obenour wrote:
> > On Tue, Dec 06, 2022 at 11:38:03AM +0000, Andrew Cooper wrote:
> > > On 06/12/2022 04:33, Demi Marie Obenour wrote:
> > > > This is purely for testing, to see if it works around a bug in i915.  It
> > > > is not intended to be merged.
> > > >
> > > > NOT-signed-off-by: DO NOT MERGE
> > > 
> > > Following up on Marek's report on IRC/Matrix, you're saying that this
> > > change does actually fix screen corruption issues on AlderLake, and
> > > something on TigerLake too?
> > 
> > Correct
> > 
> > > If that is actually the case, then one of two things is happening.  Either,
> > > 
> > > 1) Drivers in Linux are bypassing the regular caching APIs, or
> > 
> > This would not surprise me at all.
> > 
> > > 2) The translation logic between Linux's idea of cacheability and Xen's
> > > PAT values is buggy.
> > 
> > How could I check for this?
> 
> See Andy's unit test idea on #xendevel:
> 
>     as a pretty simple "unit" test in dom0, it might be a good idea to
>     have a module which watches the PTE in question, and cycles through
>     various of the memremap_*() APIs and checks the raw PTE that gets
>     written after Linux and Xen are done fighting with it

Which PTEs should I check?
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

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

Re: [RFC PATCH 8/8] Use Linux's PAT

[Marek Marczykowski-Górecki]

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

On Tue, Dec 06, 2022 at 02:47:42PM -0500, Demi Marie Obenour wrote:
> On Tue, Dec 06, 2022 at 07:12:06PM +0100, Marek Marczykowski-Górecki wrote:
> > On Tue, Dec 06, 2022 at 01:01:41PM -0500, Demi Marie Obenour wrote:
> > > On Tue, Dec 06, 2022 at 11:38:03AM +0000, Andrew Cooper wrote:
> > > > 2) The translation logic between Linux's idea of cacheability and Xen's
> > > > PAT values is buggy.
> > > 
> > > How could I check for this?
> > 
> > See Andy's unit test idea on #xendevel:
> > 
> >     as a pretty simple "unit" test in dom0, it might be a good idea to
> >     have a module which watches the PTE in question, and cycles through
> >     various of the memremap_*() APIs and checks the raw PTE that gets
> >     written after Linux and Xen are done fighting with it
> 
> Which PTEs should I check?

The ones adjusted by the memremap_*() call.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

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

Re: [PATCH 6/8] x86: Derive XEN_MSR_PAT from its individual entries

[Demi Marie Obenour]

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

On Tue, Dec 06, 2022 at 11:32:16AM +0000, Andrew Cooper wrote:
> On 06/12/2022 04:33, Demi Marie Obenour wrote:
> > This avoids it being a magic constant that is difficult for humans to
> > decode.  Use a _Static_assert to check that the old and new values are
> > identical.
> >
> > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > ---
> >  xen/arch/x86/include/asm/processor.h | 22 +++++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h
> > index 8e2816fae9b97bd4e153a30cc3802971fe0355af..64b75e444947c64e2e5eba457deec92a873d7a63 100644
> > --- a/xen/arch/x86/include/asm/processor.h
> > +++ b/xen/arch/x86/include/asm/processor.h
> > @@ -92,13 +92,33 @@
> >                            X86_EFLAGS_NT|X86_EFLAGS_DF|X86_EFLAGS_IF|    \
> >                            X86_EFLAGS_TF)
> >  
> > +/* Individual entries in IA32_CR_PAT */
> > +#define MSR_PAT_UC  _AC(0x00, ULL)
> > +#define MSR_PAT_WC  _AC(0x01, ULL)
> > +#define MSR_PAT_RESERVED_1  _AC(0x02, ULL)
> > +#define MSR_PAT_RESERVED_2  _AC(0x03, ULL)
> > +#define MSR_PAT_WT  _AC(0x04, ULL)
> > +#define MSR_PAT_WP  _AC(0x05, ULL)
> > +#define MSR_PAT_WB  _AC(0x06, ULL)
> > +#define MSR_PAT_UCM _AC(0x07, ULL)
> 
> This isn't really correct.  Constants for MSRs typically live in
> msr-index.h, but these are architectural x86 memory types.
> 
> These ought be
> 
> #define X86_MT_$X ...  (skipping the two reserved values)
> 
> in x86-defns.h, and the PAT_TYPE_*, MTRR_TYPE_* and EPT_EMT_* constants
> want removing.
> 
> There are two minor restrictions (EPT can't have UCM, MTRR can't have
> WC),

The code seems to indicate otherwise: it behaves as if both EPT and MTRR
cannot have UCM.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

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

Re: [PATCH 7/8] x86/mm: make code robust to future PAT changes

[Jan Beulich]

On 06.12.2022 18:55, Demi Marie Obenour wrote:
> On Tue, Dec 06, 2022 at 12:06:24PM +0000, Andrew Cooper wrote:
>> On 06/12/2022 04:33, Demi Marie Obenour wrote:
>>> @@ -961,13 +1000,24 @@ get_page_from_l1e(
>>>  
>>>          switch ( l1f & PAGE_CACHE_ATTRS )
>>>          {
>>> -        case _PAGE_WB:
>>> +        default:
>>> +#ifndef NDEBUG
>>> +            printk(XENLOG_G_WARNING
>>> +                   "d%d: Guest tried to use bad cachability attribute %u for MFN %lx\n",
>>> +                   l1e_owner->domain_id, l1f & PAGE_CACHE_ATTRS, mfn);
>>
>> %pd.  You absolutely want to convert the PTE bits to a PAT value before
>> priniting (decimal on a PTE value is useless), and %PRI_mfn.
> 
> I’ll have to look at the rest of the Xen tree to see how to use this.
> 
>>> +            pv_inject_hw_exception(TRAP_gp_fault, 0);
>>
>> As I said on IRC, we do want this, but I'm not certain if we can get
>> away with just enabling it in debug builds.  _PAGE_GNTTAB was ok because
>> it has always been like that, but there's a non-trivial chance that
>> there are existing dom0 kernels which violate this constraint.
> 
> I chose this approach because it is very simple to implement.  Anything
> more complex ought to be in a separate patch, IMO.
> 
>>> +            return -EINVAL;
>>> +#endif

As to "complex": Just the "return -EINVAL" would be yet less complex.
Irrespective of the remark my understanding of Andrew's response is that
the concern extends to the error return as well.

Jan

Re: [PATCH 7/8] x86/mm: make code robust to future PAT changes

[Andrew Cooper]

On 07/12/2022 09:41, Jan Beulich wrote:
> On 06.12.2022 18:55, Demi Marie Obenour wrote:
>> On Tue, Dec 06, 2022 at 12:06:24PM +0000, Andrew Cooper wrote:
>>> On 06/12/2022 04:33, Demi Marie Obenour wrote:
>>>> @@ -961,13 +1000,24 @@ get_page_from_l1e(
>>>>  
>>>>          switch ( l1f & PAGE_CACHE_ATTRS )
>>>>          {
>>>> -        case _PAGE_WB:
>>>> +        default:
>>>> +#ifndef NDEBUG
>>>> +            printk(XENLOG_G_WARNING
>>>> +                   "d%d: Guest tried to use bad cachability attribute %u for MFN %lx\n",
>>>> +                   l1e_owner->domain_id, l1f & PAGE_CACHE_ATTRS, mfn);
>>> %pd.  You absolutely want to convert the PTE bits to a PAT value before
>>> priniting (decimal on a PTE value is useless), and %PRI_mfn.
>> I’ll have to look at the rest of the Xen tree to see how to use this.
>>
>>>> +            pv_inject_hw_exception(TRAP_gp_fault, 0);
>>> As I said on IRC, we do want this, but I'm not certain if we can get
>>> away with just enabling it in debug builds.  _PAGE_GNTTAB was ok because
>>> it has always been like that, but there's a non-trivial chance that
>>> there are existing dom0 kernels which violate this constraint.
>> I chose this approach because it is very simple to implement.  Anything
>> more complex ought to be in a separate patch, IMO.
>>
>>>> +            return -EINVAL;
>>>> +#endif
> As to "complex": Just the "return -EINVAL" would be yet less complex.
> Irrespective of the remark my understanding of Andrew's response is that
> the concern extends to the error return as well.

It's a tradeoff.

From a debugging point of view, #GP is absolutely the way to go, because
you get a full backtrace out in Linux.  It doesn't matter in the
slightest which side of the SYSCALL instruction it points at, because
that's not the interesting information to look at.

Returning -EINVAL stops the problematic cache attributes from being
used, but is far more variable in terms of noticeable change inside
Linux.  It ranges from hitting a BUG(), to having the return code lost.


In this case, I'd err on the side of a #GP fault because it is a
definite error in the guest needing debugging and fixing.  But as I said
originally, it probably needs to be on-by-default with a knob to disable.

~Andrew

Re: [RFC PATCH 8/8] Use Linux's PAT

[Demi Marie Obenour]

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

On Tue, Dec 06, 2022 at 07:12:06PM +0100, Marek Marczykowski-Górecki wrote:
> On Tue, Dec 06, 2022 at 01:01:41PM -0500, Demi Marie Obenour wrote:
> > On Tue, Dec 06, 2022 at 11:38:03AM +0000, Andrew Cooper wrote:
> > > On 06/12/2022 04:33, Demi Marie Obenour wrote:
> > > > This is purely for testing, to see if it works around a bug in i915.  It
> > > > is not intended to be merged.
> > > >
> > > > NOT-signed-off-by: DO NOT MERGE
> > > 
> > > Following up on Marek's report on IRC/Matrix, you're saying that this
> > > change does actually fix screen corruption issues on AlderLake, and
> > > something on TigerLake too?
> > 
> > Correct
> > 
> > > If that is actually the case, then one of two things is happening.  Either,
> > > 
> > > 1) Drivers in Linux are bypassing the regular caching APIs, or
> > 
> > This would not surprise me at all.
> > 
> > > 2) The translation logic between Linux's idea of cacheability and Xen's
> > > PAT values is buggy.
> > 
> > How could I check for this?
> 
> See Andy's unit test idea on #xendevel:
> 
>     as a pretty simple "unit" test in dom0, it might be a good idea to
>     have a module which watches the PTE in question, and cycles through
>     various of the memremap_*() APIs and checks the raw PTE that gets
>     written after Linux and Xen are done fighting with it

This confirmed that the translation logic is correct, which means that
i195 is buggy.  I filed https://gitlab.freedesktop.org/drm/intel/-/issues/7648
for that.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

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