From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48189) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDxby-0000GL-JN for qemu-devel@nongnu.org; Fri, 17 Jun 2016 13:31:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bDxbu-0007N3-As for qemu-devel@nongnu.org; Fri, 17 Jun 2016 13:31:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57978) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDxbu-0007Mq-2d for qemu-devel@nongnu.org; Fri, 17 Jun 2016 13:31:14 -0400 References: <20160616060621.30422-1-haozhong.zhang@intel.com> <20160616060621.30422-4-haozhong.zhang@intel.com> <20160616111951.yvmr6tznzw6tyxlx@hz-desktop> From: Laszlo Ersek Message-ID: <603c703c-5142-4721-e5a3-2d147c32ccd1@redhat.com> Date: Fri, 17 Jun 2016 19:31:08 +0200 MIME-Version: 1.0 In-Reply-To: <20160616111951.yvmr6tznzw6tyxlx@hz-desktop> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Haozhong Zhang Cc: Paolo Bonzini , qemu-devel@nongnu.org, Richard Henderson , Eduardo Habkost , "Michael S . Tsirkin" , Marcelo Tosatti , kvm@vger.kernel.org, Boris Petkov , Tony Luck , Andi Kleen , rkrcmar@redhat.com, Ashok Raj Hi Haozhong, On 06/16/16 13:19, Haozhong Zhang wrote: > On 06/16/16 11:52, Paolo Bonzini wrote: >> >> >> On 16/06/2016 08:06, Haozhong Zhang wrote: >>> It's a prerequisite that certain bits of MSR_IA32_FEATURE_CONTROL should >>> be set before some features (e.g. VMX and LMCE) can be used, which is >>> usually done by the firmware. This patch adds a fw_cfg file >>> "etc/msr_feature_control" which contains the advised value of >>> MSR_IA32_FEATURE_CONTROL and can be used by guest firmware (e.g. SeaBIOS). >>> >>> Suggested-by: Paolo Bonzini >>> Signed-off-by: Haozhong Zhang >>> --- >>> hw/i386/pc.c | 28 ++++++++++++++++++++++++++++ >>> target-i386/cpu.h | 4 ++++ >>> 2 files changed, 32 insertions(+) >>> >>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >>> index 7198ed5..d8178a5 100644 >>> --- a/hw/i386/pc.c >>> +++ b/hw/i386/pc.c >>> @@ -1147,6 +1147,33 @@ void pc_cpus_init(PCMachineState *pcms) >>> smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]); >>> } >>> >>> +static void pc_build_feature_control_file(PCMachineState *pcms) >>> +{ >>> + X86CPU *cpu = X86_CPU(pcms->possible_cpus->cpus[0].cpu); >>> + CPUX86State *env = &cpu->env; >>> + uint32_t unused, ecx, edx, feature_control_bits = 0; >>> + uint32_t *val; >>> + >>> + cpu_x86_cpuid(env, 1, 0, &unused, &unused, &ecx, &edx); >>> + if (ecx & CPUID_EXT_VMX) { >>> + feature_control_bits |= FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX; >>> + } >>> + >>> + if ((edx & (CPUID_EXT2_MCE | CPUID_EXT2_MCA)) == >>> + (CPUID_EXT2_MCE | CPUID_EXT2_MCA) && >>> + (env->mcg_cap & MCG_LMCE_P)) { >>> + feature_control_bits |= FEATURE_CONTROL_LMCE; >>> + } >>> + >>> + if (!feature_control_bits) { >>> + return; >>> + } >>> + >>> + val = g_malloc(sizeof(*val)); >>> + *val = feature_control_bits | FEATURE_CONTROL_LOCKED; >>> + fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val)); >>> +} >>> + >>> static >>> void pc_machine_done(Notifier *notifier, void *data) >>> { >>> @@ -1174,6 +1201,7 @@ void pc_machine_done(Notifier *notifier, void *data) >>> acpi_setup(); >>> if (pcms->fw_cfg) { >>> pc_build_smbios(pcms->fw_cfg); >>> + pc_build_feature_control_file(pcms); >>> } >>> } >>> >>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h >>> index f0cb04f..5e07c7a 100644 >>> --- a/target-i386/cpu.h >>> +++ b/target-i386/cpu.h >>> @@ -332,6 +332,10 @@ >>> #define MSR_TSC_ADJUST 0x0000003b >>> #define MSR_IA32_TSCDEADLINE 0x6e0 >>> >>> +#define FEATURE_CONTROL_LOCKED (1<<0) >>> +#define FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX (1<<2) >>> +#define FEATURE_CONTROL_LMCE (1<<20) >>> + >>> #define MSR_P6_PERFCTR0 0xc1 >>> >>> #define MSR_IA32_SMBASE 0x9e >>> >> >> Reviewed-by: Paolo Bonzini >> >> Have you prepared a patch for SeaBIOS already? > > Yes, I'll send it after I fix the type error (uint32_t => uint64_t) in > next version. This should be supported by OVMF as well (thanks Paolo for the heads-up). I'm glad to code that up, but I'd like to ask you to file an RFE (Request For Enhancement) in the upstream edk2 tracker for OVMF: https://github.com/tianocore/edk2/issues/new In the RFE, - the subject line should include "OvmfPkg", - please describe what the feature is good for (going into the details of a specific use case is welcome), - please specify the pathname and the internal format of the fw_cfg file, - please clarify if this MSR is considered part of the "chipset state" that the firmware is responsible for at *every* boot. In particular whether you expect that the firmware program this MSR at S3 resume as well. Thanks! Laszlo