| Message ID | 20160616060621.30422-4-haozhong.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <pbonzini@redhat.com> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> > --- > 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 <pbonzini@redhat.com> Have you prepared a patch for SeaBIOS already? Thanks, Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <pbonzini@redhat.com> > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> > > --- > > 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 <pbonzini@redhat.com> > > 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. Thanks, Haozhong -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <pbonzini@redhat.com> >>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> >>> --- >>> 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 <pbonzini@redhat.com> >> >> 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 17, 2016 at 07:31:08PM +0200, Laszlo Ersek 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). > >>> I'm sorry i'm joining this discussion a bit late returning from vacation. In a real platform supporting LMCE, BIOS is responsible for setting the bits for IA32_FEATURE_CONTROL correctly. There are good reasons why we want the BIOS to play this role. in a virtualized environment, do we really have to push the same requirement or would it suffice to just emulate it as we did in the early patches. Not sure what exact problem is created by just simply supporting it within kvm/qemu and not needing the bios for the guest to also adapt these changes. Cheers, Ashok -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/17/16 22:21, Raj, Ashok wrote: > On Fri, Jun 17, 2016 at 07:31:08PM +0200, Laszlo Ersek 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). >>>>> > > I'm sorry i'm joining this discussion a bit late returning from vacation. > In a real platform supporting LMCE, BIOS is responsible for setting the bits > for IA32_FEATURE_CONTROL correctly. There are good reasons why we want the > BIOS to play this role. > > in a virtualized environment, do we really have to push the same requirement > or would it suffice to just emulate it as we did in the early patches. > > Not sure what exact problem is created by just simply supporting it within > kvm/qemu and not needing the bios for the guest to also adapt these changes. At the moment, my understanding of this feature is superficial, but the mechanisms involved in it don't seem complex. I don't expect difficulties implementing it, I just need the details that I asked for spelled out for me. As to why we should be doing this in the guest firmware(s) -- "because that's what happens on physical machines too" :) Following the phys world to the letter in virt is not always a goal, but it's never wrong. Thanks Laszlo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 17, 2016 at 10:48:17PM +0200, Laszlo Ersek wrote: > On 06/17/16 22:21, Raj, Ashok wrote: > > On Fri, Jun 17, 2016 at 07:31:08PM +0200, Laszlo Ersek 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). > >>>>> > > > > I'm sorry i'm joining this discussion a bit late returning from vacation. > > In a real platform supporting LMCE, BIOS is responsible for setting the bits > > for IA32_FEATURE_CONTROL correctly. There are good reasons why we want the > > BIOS to play this role. > > > > in a virtualized environment, do we really have to push the same requirement > > or would it suffice to just emulate it as we did in the early patches. > > > > Not sure what exact problem is created by just simply supporting it within > > kvm/qemu and not needing the bios for the guest to also adapt these changes. > > At the moment, my understanding of this feature is superficial, but the > mechanisms involved in it don't seem complex. I don't expect > difficulties implementing it, I just need the details that I asked for > spelled out for me. > > As to why we should be doing this in the guest firmware(s) -- "because > that's what happens on physical machines too" :) Following the phys > world to the letter in virt is not always a goal, but it's never wrong. But the guest bios does nothing like the BIOS in the real platform. for e.g. a real bios would have SMM handlers to work for implementing firmware first mechanisms before notifying the OS. None of these exist in the virtalized world. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/17/16 22:55, Raj, Ashok wrote: > On Fri, Jun 17, 2016 at 10:48:17PM +0200, Laszlo Ersek wrote: >> On 06/17/16 22:21, Raj, Ashok wrote: >>> On Fri, Jun 17, 2016 at 07:31:08PM +0200, Laszlo Ersek 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). >>>>>>> >>> >>> I'm sorry i'm joining this discussion a bit late returning from vacation. >>> In a real platform supporting LMCE, BIOS is responsible for setting the bits >>> for IA32_FEATURE_CONTROL correctly. There are good reasons why we want the >>> BIOS to play this role. >>> >>> in a virtualized environment, do we really have to push the same requirement >>> or would it suffice to just emulate it as we did in the early patches. >>> >>> Not sure what exact problem is created by just simply supporting it within >>> kvm/qemu and not needing the bios for the guest to also adapt these changes. >> >> At the moment, my understanding of this feature is superficial, but the >> mechanisms involved in it don't seem complex. I don't expect >> difficulties implementing it, I just need the details that I asked for >> spelled out for me. >> >> As to why we should be doing this in the guest firmware(s) -- "because >> that's what happens on physical machines too" :) Following the phys >> world to the letter in virt is not always a goal, but it's never wrong. > > But the guest bios does nothing like the BIOS in the real platform. That's overstated. The guest firmwares do a lot of things they also do on physical hardware. PCI enumeration / resource assignment, for example. > for e.g. a real bios would have SMM handlers to work for implementing firmware > first mechanisms before notifying the OS. None of these exist in the > virtalized world. Both SeaBIOS and OVMF utilize SMM, for various purposes. OVMF's goals with SMM are briefly documented here: <https://github.com/tianocore/edk2/blob/master/OvmfPkg/README#L121>. For SMM support, all of KVM, QEMU, and OVMF needed (many) patches. Anyway, I'm neutral on this. If the consensus is that the MSR at hand is none of the guest firmware's business, I won't object -- hey, it's only less work for me. OTOH, if the consensus is that SeaBIOS should be aware of the MSR, then it follows that so should OVMF. Thanks Laszlo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/17/16 13:21, Raj, Ashok wrote: > On Fri, Jun 17, 2016 at 07:31:08PM +0200, Laszlo Ersek 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). > > >>> > > I'm sorry i'm joining this discussion a bit late returning from vacation. > In a real platform supporting LMCE, BIOS is responsible for setting the bits > for IA32_FEATURE_CONTROL correctly. There are good reasons why we want the > BIOS to play this role. > > in a virtualized environment, do we really have to push the same requirement > or would it suffice to just emulate it as we did in the early patches. > > Not sure what exact problem is created by just simply supporting it within > kvm/qemu and not needing the bios for the guest to also adapt these changes. > In the current nested VMX implementation in QEMU, setup MSR_IA32_FEATURE_CONTROL is left to guest. So I think, for LMCE which is another feature involving MSR_IA32_FEATURE_CONTROL, we may follow the existing code. Paolo and Radim, is there any case that objects to setting MSR_IA32_FEATURE_CONTROL in QEMU? Thanks, Haozhong -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20/06/2016 05:09, Haozhong Zhang wrote: > In the current nested VMX implementation in QEMU, setup > MSR_IA32_FEATURE_CONTROL is left to guest. So I think, for LMCE which > is another feature involving MSR_IA32_FEATURE_CONTROL, we may follow > the existing code. > > Paolo and Radim, is there any case that objects to setting > MSR_IA32_FEATURE_CONTROL in QEMU? If the SDM says that the reset state of the MSR is zero, QEMU should initialize it to zero. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/20/16 08:56, Paolo Bonzini wrote: > > > On 20/06/2016 05:09, Haozhong Zhang wrote: > > In the current nested VMX implementation in QEMU, setup > > MSR_IA32_FEATURE_CONTROL is left to guest. So I think, for LMCE which > > is another feature involving MSR_IA32_FEATURE_CONTROL, we may follow > > the existing code. > > > > Paolo and Radim, is there any case that objects to setting > > MSR_IA32_FEATURE_CONTROL in QEMU? > > If the SDM says that the reset state of the MSR is zero, QEMU should > initialize it to zero. > Yes, SDM says "This MSR is cleared to zero when a logical processor is reset" (in section "Enabling and Entering VMX operation"), so we should do so in qemu as well. Thanks, Haozhong -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/17/16 19:31, Laszlo Ersek wrote: > Hi Haozhong, > > On 06/16/16 13:19, Haozhong Zhang wrote: > > On 06/16/16 11:52, Paolo Bonzini wrote: [..] > >> 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. > Done. Pls check https://github.com/tianocore/edk2/issues/97. Thanks, Haozhong -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/22/16 12:18, Haozhong Zhang wrote: > On 06/17/16 19:31, Laszlo Ersek wrote: >> Hi Haozhong, >> >> On 06/16/16 13:19, Haozhong Zhang wrote: >>> On 06/16/16 11:52, Paolo Bonzini wrote: > [..] >>>> 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. >> > > Done. Pls check https://github.com/tianocore/edk2/issues/97. Thanks. Looks good. I asked a few more questions in there. Especially if the APs are involved in this, then the feature is going to be significantly more complex than I expected. Thanks Laszlo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 <pbonzini@redhat.com> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> --- hw/i386/pc.c | 28 ++++++++++++++++++++++++++++ target-i386/cpu.h | 4 ++++ 2 files changed, 32 insertions(+)