On 18/06/2021 18:15, Igor Druzhinin wrote:
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.coop...@citrix.com> Signed-off-by: Jan Beulich <jbeul...@suse.com> --- 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?
Ignore this last paragraph - I lost your statement in comment description. Igor
