Re: [PATCH] x86/AMD: make HT range dynamic for Fam17 and up

[Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>]

On 18/06/2021 17:00, Jan Beulich wrote:
> At the time of d838ac2539cf ("x86: don't allow Dom0 access to the HT
> address range") documentation correctly stated that the range was
> completely fixed. For Fam17 and newer, it lives at the top of physical
> address space, though.
From "Open-Source Register Reference for AMD Family 17h Processors (PUB)":
https://developer.amd.com/wp-content/resources/56255_3_03.PDF

"The processor defines a reserved memory address region starting at
FFFD_0000_0000h and extending up to FFFF_FFFF_FFFFh."

It's still doesn't say that it's at the top of physical address space
although I understand that's how it's now implemented. The official
document doesn't confirm it will move along with physical address space
extension.

> To correctly determine the top of physical address space, we need to
> account for their physical address reduction, hence the calculation of
> paddr_bits also gets adjusted.
>
> While for paddr_bits < 40 the HT range is completely hidden, there's no
> need to suppress the range insertion in that case: It'll just have no
> real meaning.
>
> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -349,13 +349,17 @@ void __init early_cpu_init(void)
>   
>   	eax = cpuid_eax(0x80000000);
>         if ((eax >> 16) == 0x8000 && eax >= 0x80000008) {
> +               ebx = eax >= 0x8000001f ? cpuid_ebx(0x8000001f) : 0;
>                 eax = cpuid_eax(0x80000008);
> -               paddr_bits = eax & 0xff;
> +
I understand Andrew has some concerns regarding changing paddr_bits but
some comment explaining what's located at 0x8000001f:ebx[11:6] and why
we're doing this might be useful.

> +               paddr_bits = (eax & 0xff) - ((ebx >> 6) & 0x3f);
>                 if (paddr_bits > PADDR_BITS)
>                         paddr_bits = PADDR_BITS;
> +
>                 vaddr_bits = (eax >> 8) & 0xff;
>                 if (vaddr_bits > VADDR_BITS)
>                         vaddr_bits = VADDR_BITS;
> +
>                 hap_paddr_bits = ((eax >> 16) & 0xff) ?: paddr_bits;
>                 if (hap_paddr_bits > PADDR_BITS)
>                         hap_paddr_bits = PADDR_BITS;
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -524,8 +524,11 @@ int __init dom0_setup_permissions(struct
>                                            MSI_ADDR_DEST_ID_MASK));
>       /* HyperTransport range. */
>       if ( boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON) )
> -        rc |= iomem_deny_access(d, paddr_to_pfn(0xfdULL << 32),
> -                                paddr_to_pfn((1ULL << 40) - 1));
> +    {
> +        mfn = paddr_to_pfn(1UL <<
> +                           (boot_cpu_data.x86 < 0x17 ? 40 : paddr_bits));
That doesn't really follow what Andrew gave us, namely:

1) On parts with <40 bits, its fully hidden from software
2) Before Fam17h, it was always 12G just below 1T, even if there was more RAM 
above this location
3) On Fam17h and later, it is variable based on SME, and is either just below 
2^48 (no encryption) or 2^43 (encryption)

Do we need (1) to be coded here as well?

Igor