Re: [PATCH v5 03/15] x86/mtrr: replace some constants with defines

[Juergen Gross]

On 03.04.23 18:03, Borislav Petkov wrote:
> On Sat, Apr 01, 2023 at 08:36:40AM +0200, Juergen Gross wrote:
> > @@ -643,10 +646,12 @@ static bool set_mtrr_var_ranges(unsigned int index, struct mtrr_var_range *vr)
> >   	unsigned int lo, hi;
> >   	bool changed = false;
> >   
> > +#define BASE_MASK	(MTRR_BASE_TYPE_MASK | (size_and_mask << PAGE_SHIFT))
> > +#define MASK_MASK	(MTRR_MASK_VALID | (size_and_mask << PAGE_SHIFT))
>
> No, "MASK_MASK" is too much. :-)

Any better suggestion for the name? :-)

> Anyway, so I started looking at this and here's what I'm seeing on my
> machine even with the old code:
>
>          rdmsr(MTRRphysBase_MSR(index), lo, hi);
>          if ((vr->base_lo & 0xfffff0ffUL) != (lo & 0xfffff0ffUL)
>              || (vr->base_hi & (size_and_mask >> (32 - PAGE_SHIFT))) !=
>                  (hi & (size_and_mask >> (32 - PAGE_SHIFT)))) {
>
>
> size_and_mask:		0x0000000ffff00000
> the shifted version:	0x000000000000ffff
>
> which is bits [15:0]
>
> Now, looking at MTRRphysBase_MSR's definition, the high 32 bits are:
>
> [63 ... reserved ... ][ max_addr ... 32 ]
>
> and that second slice: from max_addr to the 32nd bit of the whole MSR is
> the second part of PhysBase.
>
> max_addr aka phys_addr comes from:
>
> 	phys_addr = cpuid_eax(0x80000008) & 0xff;
>
> on that machine, that value is 48.
>
> Which means, the physaddr slice should be [48 .. 32], i.e.,
>
> 	0x0001ffff00000000

No. The "48" is the _number_ of physical address bits, so the 64 bit address
mask will be 0000ffff.ffffffff (48 bits set).

> and when you shift that by 32 so that it can be ANDed with the high
> portion of the MSR, it becomes
>
> 0x000000000001ffff
>
> i.e., bit 16 is set too.
>
> And that max address can be up to 51:
>
> "Range Physical Base-Address (PhysBase)—Bits 51:12. The memory-range base-address in
> physical-address space. PhysBase is aligned on a 4-Kbyte (or greater) address in the 52-bit
> physical-address space supported by the AMD64 architecture. PhysBase represents the most-
> significant 40-address bits of the physical address. Physical-address
> bits 11:0 are assumed to be 0."
>
> Long story short, size_and_mask needs to be done from
>
> 	size_and_mask = ~size_or_mask & 0xfffff00000ULL;
>
> to
>
> 	size_and_mask = ~size_or_mask & GENMASK_ULL(phys_addr, 20);

This must be phys_addr - 1, as we start to count with bit 0.

> size_or_mask bits already took into consideration the phys_addr:
>
> 	size_or_mask = SIZE_OR_MASK_BITS(phys_addr);
>
> and size_and_mask needs to do it too.
>
> Right?
>
> I'll test this on the boxes here everywhere. I guess this gets hit only
> on boxes where the phys_addr of the variable MTRRs goes over the 16
> bits.
>
> As to this patch: please make all the bit and mask definitions you're
> adding as close to the MTRR bit and mask definition names in the
> APM+SDM. I've started this already (ontop of yours):

Okay.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature