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
