diff mbox series

[v5,11/10] hvmloader: use memory type constants

Message ID 24461a6b-b118-aad9-6407-d215d07a2924@suse.com (mailing list archive)
State Superseded
Headers show
Series Make PAT handling less brittle | expand

Commit Message

Jan Beulich Dec. 20, 2022, 4:13 p.m. UTC 
Now that we have them available in a header which is okay to use from
hvmloader sources, do away with respective literal numbers and silent
assumptions.

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

Comments

Andrew Cooper Dec. 20, 2022, 4:36 p.m. UTC | #1 
On 20/12/2022 4:13 pm, Jan Beulich wrote:
> Now that we have them available in a header which is okay to use from
> hvmloader sources, do away with respective literal numbers and silent
> assumptions.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/tools/firmware/hvmloader/cacheattr.c
> +++ b/tools/firmware/hvmloader/cacheattr.c
> @@ -22,6 +22,8 @@
>  #include "util.h"
>  #include "config.h"
>  
> +#include <xen/asm/x86-defns.h>
> +
>  #define MSR_MTRRphysBase(reg) (0x200 + 2 * (reg))
>  #define MSR_MTRRphysMask(reg) (0x200 + 2 * (reg) + 1)
>  #define MSR_MTRRcap          0x00fe
> @@ -71,23 +73,32 @@ void cacheattr_init(void)
>  
>      addr_mask = ((1ull << phys_bits) - 1) & ~((1ull << 12) - 1);

Does this want to be PAGE_SHIFT ?

>      mtrr_cap = rdmsr(MSR_MTRRcap);
> -    mtrr_def = (1u << 11) | 6; /* E, default type WB */
> +    mtrr_def = (1u << 11) | X86_MT_WB; /* E, default type WB */
>  
>      /* Fixed-range MTRRs supported? */
>      if ( mtrr_cap & (1u << 8) )
>      {
> +#define BCST2(mt) ((mt) | ((mt) << 8))
> +#define BCST4(mt) (BCST2(mt) | (BCST2(mt) << 16))
> +#define BCST8(mt) (BCST4(mt) | ((uint64_t)BCST4(mt) << 32))

IMO this is clearer as

#define BCST8(mt) ((mt) * 0x0101010101010101ULL)

which saves several macros.

>          /* 0x00000-0x9ffff: Write Back (WB) */
> -        content = 0x0606060606060606ull;
> +        content = BCST8(X86_MT_WB);
>          wrmsr(MSR_MTRRfix64K_00000, content);
>          wrmsr(MSR_MTRRfix16K_80000, content);
> +
>          /* 0xa0000-0xbffff: Write Combining (WC) */
>          if ( mtrr_cap & (1u << 10) ) /* WC supported? */
> -            content = 0x0101010101010101ull;
> +            content = BCST8(X86_MT_WC);
>          wrmsr(MSR_MTRRfix16K_A0000, content);

This looks like it's latently buggy.  We'll end up with WB if WC isn't
supported, when it ought to be UC.

I suppose it doesn't actually matter because if there is a VGA region
there, it will actually be holes in the P2M and trap for emulation.

Also, Xen being 64-bit, WC is always available.

> +
>          /* 0xc0000-0xfffff: Write Back (WB) */
> -        content = 0x0606060606060606ull;
> +        content = BCST8(X86_MT_WB);
>          for ( i = 0; i < 8; i++ )
>              wrmsr(MSR_MTRRfix4K_C0000 + i, content);
> +#undef BCST8
> +#undef BCST4
> +#undef BCST2
> +
>          mtrr_def |= 1u << 10; /* FE */
>          printf("fixed MTRRs ... ");
>      }
> @@ -106,7 +117,7 @@ void cacheattr_init(void)
>              while ( ((base + size) < base) || ((base + size) > pci_mem_end) )
>                  size >>= 1;
>  
> -            wrmsr(MSR_MTRRphysBase(i), base);
> +            wrmsr(MSR_MTRRphysBase(i), base | X86_MT_UC);
>              wrmsr(MSR_MTRRphysMask(i), (~(size - 1) & addr_mask) | (1u << 11));

If you're re-spinning for page_size or the macros, any chance of also
gaining some constants for E/FE to avoid these naked shifts?

~Andrew

>  
>              base += size;
> @@ -121,7 +132,7 @@ void cacheattr_init(void)
>              while ( (base + size < base) || (base + size > pci_hi_mem_end) )
>                  size >>= 1;
>  
> -            wrmsr(MSR_MTRRphysBase(i), base);
> +            wrmsr(MSR_MTRRphysBase(i), base | X86_MT_UC);
>              wrmsr(MSR_MTRRphysMask(i), (~(size - 1) & addr_mask) | (1u << 11));
>  
>              base += size;
>
>
Jan Beulich Dec. 20, 2022, 4:50 p.m. UTC | #2 
On 20.12.2022 17:36, Andrew Cooper wrote:
> On 20/12/2022 4:13 pm, Jan Beulich wrote:
>> --- a/tools/firmware/hvmloader/cacheattr.c
>> +++ b/tools/firmware/hvmloader/cacheattr.c
>> @@ -22,6 +22,8 @@
>>  #include "util.h"
>>  #include "config.h"
>>  
>> +#include <xen/asm/x86-defns.h>
>> +
>>  #define MSR_MTRRphysBase(reg) (0x200 + 2 * (reg))
>>  #define MSR_MTRRphysMask(reg) (0x200 + 2 * (reg) + 1)
>>  #define MSR_MTRRcap          0x00fe
>> @@ -71,23 +73,32 @@ void cacheattr_init(void)
>>  
>>      addr_mask = ((1ull << phys_bits) - 1) & ~((1ull << 12) - 1);
> 
> Does this want to be PAGE_SHIFT ?

Not really (it's rather an MTRR layout related constant which we ought
to use here, which only happens to match PAGE_{SIZE,SHIFT}) and not in
this patch (see the title).

>>      mtrr_cap = rdmsr(MSR_MTRRcap);
>> -    mtrr_def = (1u << 11) | 6; /* E, default type WB */
>> +    mtrr_def = (1u << 11) | X86_MT_WB; /* E, default type WB */
>>  
>>      /* Fixed-range MTRRs supported? */
>>      if ( mtrr_cap & (1u << 8) )
>>      {
>> +#define BCST2(mt) ((mt) | ((mt) << 8))
>> +#define BCST4(mt) (BCST2(mt) | (BCST2(mt) << 16))
>> +#define BCST8(mt) (BCST4(mt) | ((uint64_t)BCST4(mt) << 32))
> 
> IMO this is clearer as
> 
> #define BCST8(mt) ((mt) * 0x0101010101010101ULL)
> 
> which saves several macros.

Ah, yes, will switch (and then name the thing just BCST()).

>>          /* 0x00000-0x9ffff: Write Back (WB) */
>> -        content = 0x0606060606060606ull;
>> +        content = BCST8(X86_MT_WB);
>>          wrmsr(MSR_MTRRfix64K_00000, content);
>>          wrmsr(MSR_MTRRfix16K_80000, content);
>> +
>>          /* 0xa0000-0xbffff: Write Combining (WC) */
>>          if ( mtrr_cap & (1u << 10) ) /* WC supported? */
>> -            content = 0x0101010101010101ull;
>> +            content = BCST8(X86_MT_WC);
>>          wrmsr(MSR_MTRRfix16K_A0000, content);
> 
> This looks like it's latently buggy.  We'll end up with WB if WC isn't
> supported, when it ought to be UC.
> 
> I suppose it doesn't actually matter because if there is a VGA region
> there, it will actually be holes in the P2M and trap for emulation.
> 
> Also, Xen being 64-bit, WC is always available.

Right, but we (or the admin) may elect to not surface availability to
the guest.

>> @@ -106,7 +117,7 @@ void cacheattr_init(void)
>>              while ( ((base + size) < base) || ((base + size) > pci_mem_end) )
>>                  size >>= 1;
>>  
>> -            wrmsr(MSR_MTRRphysBase(i), base);
>> +            wrmsr(MSR_MTRRphysBase(i), base | X86_MT_UC);
>>              wrmsr(MSR_MTRRphysMask(i), (~(size - 1) & addr_mask) | (1u << 11));
> 
> If you're re-spinning for page_size or the macros, any chance of also
> gaining some constants for E/FE to avoid these naked shifts?

Again, yes in principle, but not in this patch. This requires shuffling
more stuff into x86-defns.h first.

Jan
Demi Marie Obenour Dec. 20, 2022, 5:45 p.m. UTC | #3 
On Tue, Dec 20, 2022 at 05:13:04PM +0100, Jan Beulich wrote:
> Now that we have them available in a header which is okay to use from
> hvmloader sources, do away with respective literal numbers and silent
> assumptions.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/tools/firmware/hvmloader/cacheattr.c
> +++ b/tools/firmware/hvmloader/cacheattr.c
> @@ -22,6 +22,8 @@
>  #include "util.h"
>  #include "config.h"
>  
> +#include <xen/asm/x86-defns.h>
> +
>  #define MSR_MTRRphysBase(reg) (0x200 + 2 * (reg))
>  #define MSR_MTRRphysMask(reg) (0x200 + 2 * (reg) + 1)
>  #define MSR_MTRRcap          0x00fe
> @@ -71,23 +73,32 @@ void cacheattr_init(void)
>  
>      addr_mask = ((1ull << phys_bits) - 1) & ~((1ull << 12) - 1);
>      mtrr_cap = rdmsr(MSR_MTRRcap);
> -    mtrr_def = (1u << 11) | 6; /* E, default type WB */
> +    mtrr_def = (1u << 11) | X86_MT_WB; /* E, default type WB */
>  
>      /* Fixed-range MTRRs supported? */
>      if ( mtrr_cap & (1u << 8) )
>      {
> +#define BCST2(mt) ((mt) | ((mt) << 8))
> +#define BCST4(mt) (BCST2(mt) | (BCST2(mt) << 16))

This should include a cast to uint32_t, just like BCST8 includes a cast
to uint64_t.  It doesn’t have any functional impact since none of the
memory types have the high bit set, but it makes the correctness of the
code much more obvious to readers.

> +#define BCST8(mt) (BCST4(mt) | ((uint64_t)BCST4(mt) << 32))
>          /* 0x00000-0x9ffff: Write Back (WB) */
> -        content = 0x0606060606060606ull;
> +        content = BCST8(X86_MT_WB);
>          wrmsr(MSR_MTRRfix64K_00000, content);
>          wrmsr(MSR_MTRRfix16K_80000, content);
> +
>          /* 0xa0000-0xbffff: Write Combining (WC) */
>          if ( mtrr_cap & (1u << 10) ) /* WC supported? */
> -            content = 0x0101010101010101ull;
> +            content = BCST8(X86_MT_WC);
>          wrmsr(MSR_MTRRfix16K_A0000, content);
> +
>          /* 0xc0000-0xfffff: Write Back (WB) */
> -        content = 0x0606060606060606ull;
> +        content = BCST8(X86_MT_WB);
>          for ( i = 0; i < 8; i++ )
>              wrmsr(MSR_MTRRfix4K_C0000 + i, content);
> +#undef BCST8
> +#undef BCST4
> +#undef BCST2
> +
>          mtrr_def |= 1u << 10; /* FE */
>          printf("fixed MTRRs ... ");
>      }
> @@ -106,7 +117,7 @@ void cacheattr_init(void)
>              while ( ((base + size) < base) || ((base + size) > pci_mem_end) )
>                  size >>= 1;
>  
> -            wrmsr(MSR_MTRRphysBase(i), base);
> +            wrmsr(MSR_MTRRphysBase(i), base | X86_MT_UC);
>              wrmsr(MSR_MTRRphysMask(i), (~(size - 1) & addr_mask) | (1u << 11));
>  
>              base += size;
> @@ -121,7 +132,7 @@ void cacheattr_init(void)
>              while ( (base + size < base) || (base + size > pci_hi_mem_end) )
>                  size >>= 1;
>  
> -            wrmsr(MSR_MTRRphysBase(i), base);
> +            wrmsr(MSR_MTRRphysBase(i), base | X86_MT_UC);
>              wrmsr(MSR_MTRRphysMask(i), (~(size - 1) & addr_mask) | (1u << 11));
>  
>              base += size;
> 

With the suggested change:

Acked-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Jan Beulich Dec. 21, 2022, 6:56 a.m. UTC | #4 
On 20.12.2022 18:45, Demi Marie Obenour wrote:
> On Tue, Dec 20, 2022 at 05:13:04PM +0100, Jan Beulich wrote:
>> --- a/tools/firmware/hvmloader/cacheattr.c
>> +++ b/tools/firmware/hvmloader/cacheattr.c
>> @@ -22,6 +22,8 @@
>>  #include "util.h"
>>  #include "config.h"
>>  
>> +#include <xen/asm/x86-defns.h>
>> +
>>  #define MSR_MTRRphysBase(reg) (0x200 + 2 * (reg))
>>  #define MSR_MTRRphysMask(reg) (0x200 + 2 * (reg) + 1)
>>  #define MSR_MTRRcap          0x00fe
>> @@ -71,23 +73,32 @@ void cacheattr_init(void)
>>  
>>      addr_mask = ((1ull << phys_bits) - 1) & ~((1ull << 12) - 1);
>>      mtrr_cap = rdmsr(MSR_MTRRcap);
>> -    mtrr_def = (1u << 11) | 6; /* E, default type WB */
>> +    mtrr_def = (1u << 11) | X86_MT_WB; /* E, default type WB */
>>  
>>      /* Fixed-range MTRRs supported? */
>>      if ( mtrr_cap & (1u << 8) )
>>      {
>> +#define BCST2(mt) ((mt) | ((mt) << 8))
>> +#define BCST4(mt) (BCST2(mt) | (BCST2(mt) << 16))
> 
> This should include a cast to uint32_t, just like BCST8 includes a cast
> to uint64_t.  It doesn’t have any functional impact since none of the
> memory types have the high bit set, but it makes the correctness of the
> code much more obvious to readers.

I've already followed Andrew's suggestion.

> With the suggested change:
> 
> Acked-by: Demi Marie Obenour <demi@invisiblethingslab.com>

Thanks. Since I've addressed this differently, I'll hold back applying
of this.

From a formal perspective I'd also like to point out that an Acked-by:
from a person not being maintainer of any code being changed by a patch
has no meaning at all. Only Reviewed-by: has a meaning (but of course
implies more thorough looking at the change than Acked-by: does).

Jan
diff mbox series

Patch

--- a/tools/firmware/hvmloader/cacheattr.c
+++ b/tools/firmware/hvmloader/cacheattr.c
@@ -22,6 +22,8 @@ 
 #include "util.h"
 #include "config.h"
 
+#include <xen/asm/x86-defns.h>
+
 #define MSR_MTRRphysBase(reg) (0x200 + 2 * (reg))
 #define MSR_MTRRphysMask(reg) (0x200 + 2 * (reg) + 1)
 #define MSR_MTRRcap          0x00fe
@@ -71,23 +73,32 @@  void cacheattr_init(void)
 
     addr_mask = ((1ull << phys_bits) - 1) & ~((1ull << 12) - 1);
     mtrr_cap = rdmsr(MSR_MTRRcap);
-    mtrr_def = (1u << 11) | 6; /* E, default type WB */
+    mtrr_def = (1u << 11) | X86_MT_WB; /* E, default type WB */
 
     /* Fixed-range MTRRs supported? */
     if ( mtrr_cap & (1u << 8) )
     {
+#define BCST2(mt) ((mt) | ((mt) << 8))
+#define BCST4(mt) (BCST2(mt) | (BCST2(mt) << 16))
+#define BCST8(mt) (BCST4(mt) | ((uint64_t)BCST4(mt) << 32))
         /* 0x00000-0x9ffff: Write Back (WB) */
-        content = 0x0606060606060606ull;
+        content = BCST8(X86_MT_WB);
         wrmsr(MSR_MTRRfix64K_00000, content);
         wrmsr(MSR_MTRRfix16K_80000, content);
+
         /* 0xa0000-0xbffff: Write Combining (WC) */
         if ( mtrr_cap & (1u << 10) ) /* WC supported? */
-            content = 0x0101010101010101ull;
+            content = BCST8(X86_MT_WC);
         wrmsr(MSR_MTRRfix16K_A0000, content);
+
         /* 0xc0000-0xfffff: Write Back (WB) */
-        content = 0x0606060606060606ull;
+        content = BCST8(X86_MT_WB);
         for ( i = 0; i < 8; i++ )
             wrmsr(MSR_MTRRfix4K_C0000 + i, content);
+#undef BCST8
+#undef BCST4
+#undef BCST2
+
         mtrr_def |= 1u << 10; /* FE */
         printf("fixed MTRRs ... ");
     }
@@ -106,7 +117,7 @@  void cacheattr_init(void)
             while ( ((base + size) < base) || ((base + size) > pci_mem_end) )
                 size >>= 1;
 
-            wrmsr(MSR_MTRRphysBase(i), base);
+            wrmsr(MSR_MTRRphysBase(i), base | X86_MT_UC);
             wrmsr(MSR_MTRRphysMask(i), (~(size - 1) & addr_mask) | (1u << 11));
 
             base += size;
@@ -121,7 +132,7 @@  void cacheattr_init(void)
             while ( (base + size < base) || (base + size > pci_hi_mem_end) )
                 size >>= 1;
 
-            wrmsr(MSR_MTRRphysBase(i), base);
+            wrmsr(MSR_MTRRphysBase(i), base | X86_MT_UC);
             wrmsr(MSR_MTRRphysMask(i), (~(size - 1) & addr_mask) | (1u << 11));
 
             base += size;