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 <[email protected]> > > --- 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 <[email protected]>
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
signature.asc
Description: PGP signature
