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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxxxx>

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