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

[Jan Beulich <jbeulich@xxxxxxxx>]

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