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

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

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;

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