From 8bc660ce8a25172309958a551ce84b083fc7e173 Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Thu, 9 Nov 2023 19:58:14 -0500 Subject: [PATCH] i#5725: Set consistent x86 vendor decoding default (#6438) If we initialize the proc_get_vendor() to VENDOR_UNKNOWN we get contradictory decoding results for x86 opcodes that vary between VENDOR_AMD and VENDOR_INTEL (because some of our decoding code checks one and some checks the other), so we pick one (VENDOR_INTEL) for a stable self-consistent default. We set this statically and we override the underlying hardware in proc_init(). The user can use proc_set_vendor() (after an explicit init) to override if desired. Fixes #5725 --- core/arch/proc_shared.c | 41 ++++++++++++++++++++++++++--------------- core/arch/x86/proc.c | 17 ++++++++++++++++- 2 files changed, 42 insertions(+), 16 deletions(-) diff --git a/core/arch/proc_shared.c b/core/arch/proc_shared.c index e74e83fbb48..f50e6092b91 100644 --- a/core/arch/proc_shared.c +++ b/core/arch/proc_shared.c @@ -1,5 +1,5 @@ /* ********************************************************** - * Copyright (c) 2013-2022 Google, Inc. All rights reserved. + * Copyright (c) 2013-2023 Google, Inc. All rights reserved. * Copyright (c) 2000-2008 VMware, Inc. All rights reserved. * **********************************************************/ @@ -63,25 +63,36 @@ */ size_t cache_line_size = 32; static ptr_uint_t mask; /* bits that should be 0 to be cache-line-aligned */ -cpu_info_t cpu_info = { VENDOR_UNKNOWN, +cpu_info_t cpu_info = { +#ifdef X86 + /* If we initialize to VENDOR_UNKNOWN we get contradictory decoding results + * for opcodes that vary between VENDOR_AMD and VENDOR_INTEL (because some + * of our decoding code checks one and some checks the other) so we pick one + * for a stable self-consistent default. + */ + VENDOR_INTEL, +#else + VENDOR_UNKNOWN, +#endif #ifdef AARCHXX - 0, - 0, + 0, + 0, #endif - 0, - 0, - 0, - 0, - CACHE_SIZE_UNKNOWN, - CACHE_SIZE_UNKNOWN, - CACHE_SIZE_UNKNOWN, + 0, + 0, + 0, + 0, + CACHE_SIZE_UNKNOWN, + CACHE_SIZE_UNKNOWN, + CACHE_SIZE_UNKNOWN, #if defined(RISCV64) - /* FIXME i#3544: Not implemented */ - { 0 }, + /* FIXME i#3544: Not implemented */ + { 0 }, #else - { 0, 0, 0, 0 }, + { 0, 0, 0, 0 }, #endif - { 0x6e6b6e75, 0x006e776f } }; + { 0x6e6b6e75, 0x006e776f } +}; void proc_set_cache_size(uint val, uint *dst) diff --git a/core/arch/x86/proc.c b/core/arch/x86/proc.c index a06479e569b..a421cebdf56 100644 --- a/core/arch/x86/proc.c +++ b/core/arch/x86/proc.c @@ -1,5 +1,5 @@ /* ********************************************************** - * Copyright (c) 2013-2022 Google, Inc. All rights reserved. + * Copyright (c) 2013-2023 Google, Inc. All rights reserved. * Copyright (c) 2000-2008 VMware, Inc. All rights reserved. * **********************************************************/ @@ -333,6 +333,21 @@ get_processor_specific_info(void) our_cpuid((int *)&cpu_info.brand_string[4], 0x80000003, 0); our_cpuid((int *)&cpu_info.brand_string[8], 0x80000004, 0); } + + if (standalone_library) { + /* For separate decoding, keep a base default and do not target the + * underlying processor (xref i#431, i#6420, i#5725). + * The user can use proc_set_vendor() to override. + * This affects various decoding corner cases and is important to set. + * We leave all the other cache info, etc. pointing to the current hardware. + * We set this at the end here to avoid errors in getting other + * cpuid values with the wrong magic parameters. + * XXX: This is still potentially fragile; should the decoder have + * a separate vendor value? + */ + LOG(GLOBAL, LOG_TOP, 1, "For standalone decoding, assuming Intel target.\n"); + cpu_info.vendor = VENDOR_INTEL; + } } /* arch specific proc info */