i386: Ensure feature names are always defined

[PATCH 1/3] i386: Add missing "vmx-ept-wb" feature name

Posted by Eduardo Habkost 5 years ago

Not having a feature name in feature_word_info breaks error
reporting and query-cpu-model-expansion.  Add the missing feature
name to feature_word_info[FEAT_VMX_EPT_VPID_CAPS].feat_names[14].

Fixes: 0723cc8a5558 ("target/i386: add VMX features to named CPU models")
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target/i386/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index ae89024d366..2bf3ab78056 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1262,7 +1262,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             "vmx-ept-execonly", NULL, NULL, NULL,
             NULL, NULL, "vmx-page-walk-4", "vmx-page-walk-5",
             NULL, NULL, NULL, NULL,
-            NULL, NULL, NULL, NULL,
+            NULL, NULL, "vmx-ept-wb", NULL,
             "vmx-ept-2mb", "vmx-ept-1gb", NULL, NULL,
             "vmx-invept", "vmx-eptad", "vmx-ept-advanced-exitinfo", NULL,
             NULL, "vmx-invept-single-context", "vmx-invept-all-context", NULL,
-- 
2.28.0

Re: [PATCH 1/3] i386: Add missing "vmx-ept-wb" feature name

Posted by Paolo Bonzini 5 years ago

This is intentional, because there's no way that any hypervisor can run if
this feature is disabled.

Paolo

Il lun 1 feb 2021, 23:54 Eduardo Habkost <ehabkost@redhat.com> ha scritto:

> Not having a feature name in feature_word_info breaks error
> reporting and query-cpu-model-expansion.  Add the missing feature
> name to feature_word_info[FEAT_VMX_EPT_VPID_CAPS].feat_names[14].
>
> Fixes: 0723cc8a5558 ("target/i386: add VMX features to named CPU models")
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target/i386/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index ae89024d366..2bf3ab78056 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1262,7 +1262,7 @@ static FeatureWordInfo
> feature_word_info[FEATURE_WORDS] = {
>              "vmx-ept-execonly", NULL, NULL, NULL,
>              NULL, NULL, "vmx-page-walk-4", "vmx-page-walk-5",
>              NULL, NULL, NULL, NULL,
> -            NULL, NULL, NULL, NULL,
> +            NULL, NULL, "vmx-ept-wb", NULL,
>              "vmx-ept-2mb", "vmx-ept-1gb", NULL, NULL,
>              "vmx-invept", "vmx-eptad", "vmx-ept-advanced-exitinfo", NULL,
>              NULL, "vmx-invept-single-context", "vmx-invept-all-context",
> NULL,
> --
> 2.28.0
>
>

Re: [PATCH 1/3] i386: Add missing "vmx-ept-wb" feature name

Posted by Eduardo Habkost 5 years ago

On Mon, Feb 01, 2021 at 11:59:48PM +0100, Paolo Bonzini wrote:
> Il lun 1 feb 2021, 23:54 Eduardo Habkost <ehabkost@redhat.com> ha scritto:
> 
> > Not having a feature name in feature_word_info breaks error
> > reporting and query-cpu-model-expansion.  Add the missing feature
> > name to feature_word_info[FEAT_VMX_EPT_VPID_CAPS].feat_names[14].
> >
> This is intentional, because there's no way that any hypervisor can run if
> this feature is disabled.

If leaving the feature without name enables some desirable
behavior, that's by accident and not by design.  Which part of
the existing behavior is intentional?

-- 
Eduardo

Re: [PATCH 1/3] i386: Add missing "vmx-ept-wb" feature name

Posted by Paolo Bonzini 5 years ago

Il mar 2 feb 2021, 00:05 Eduardo Habkost <ehabkost@redhat.com> ha scritto:

> On Mon, Feb 01, 2021 at 11:59:48PM +0100, Paolo Bonzini wrote:
> > Il lun 1 feb 2021, 23:54 Eduardo Habkost <ehabkost@redhat.com> ha
> scritto:
> >
> > > Not having a feature name in feature_word_info breaks error
> > > reporting and query-cpu-model-expansion.  Add the missing feature
> > > name to feature_word_info[FEAT_VMX_EPT_VPID_CAPS].feat_names[14].
> > >
> > This is intentional, because there's no way that any hypervisor can run
> if
> > this feature is disabled.
>
> If leaving the feature without name enables some desirable
> behavior, that's by accident and not by design.  Which part of
> the existing behavior is intentional?
>

Not being able to disable it.

Paolo


> --
> Eduardo
>
>

Re: [PATCH 1/3] i386: Add missing "vmx-ept-wb" feature name

Posted by Eduardo Habkost 5 years ago

On Tue, Feb 02, 2021 at 12:28:38AM +0100, Paolo Bonzini wrote:
> Il mar 2 feb 2021, 00:05 Eduardo Habkost <ehabkost@redhat.com> ha scritto:
> 
> > On Mon, Feb 01, 2021 at 11:59:48PM +0100, Paolo Bonzini wrote:
> > > Il lun 1 feb 2021, 23:54 Eduardo Habkost <ehabkost@redhat.com> ha
> > scritto:
> > >
> > > > Not having a feature name in feature_word_info breaks error
> > > > reporting and query-cpu-model-expansion.  Add the missing feature
> > > > name to feature_word_info[FEAT_VMX_EPT_VPID_CAPS].feat_names[14].
> > > >
> > > This is intentional, because there's no way that any hypervisor can run
> > if
> > > this feature is disabled.
> >
> > If leaving the feature without name enables some desirable
> > behavior, that's by accident and not by design.  Which part of
> > the existing behavior is intentional?
> >
> 
> Not being able to disable it.

We can make it a hard dependency of vmx, then.  We shouldn't
leave it without a name, though.

-- 
Eduardo

Re: [PATCH 1/3] i386: Add missing "vmx-ept-wb" feature name

Posted by Paolo Bonzini 5 years ago

On 02/02/21 01:18, Eduardo Habkost wrote:
> On Tue, Feb 02, 2021 at 12:28:38AM +0100, Paolo Bonzini wrote:
>> Il mar 2 feb 2021, 00:05 Eduardo Habkost <ehabkost@redhat.com> ha scritto:
>>
>>> On Mon, Feb 01, 2021 at 11:59:48PM +0100, Paolo Bonzini wrote:
>>>> Il lun 1 feb 2021, 23:54 Eduardo Habkost <ehabkost@redhat.com> ha
>>> scritto:
>>>>
>>>>> Not having a feature name in feature_word_info breaks error
>>>>> reporting and query-cpu-model-expansion.  Add the missing feature
>>>>> name to feature_word_info[FEAT_VMX_EPT_VPID_CAPS].feat_names[14].
>>>>>
>>>> This is intentional, because there's no way that any hypervisor can run
>>> if
>>>> this feature is disabled.
>>>
>>> If leaving the feature without name enables some desirable
>>> behavior, that's by accident and not by design.  Which part of
>>> the existing behavior is intentional?
>>>
>>
>> Not being able to disable it.
> 
> We can make it a hard dependency of vmx, then.  We shouldn't
> leave it without a name, though.

The feature is already added to the MSRs unconditionally in 
kvm_msr_entry_add_vmx.  I think we can just remove it from the models 
instead.

Paolo

Re: [PATCH 1/3] i386: Add missing "vmx-ept-wb" feature name

Posted by Eduardo Habkost 5 years ago

On Tue, Feb 02, 2021 at 08:54:30AM +0100, Paolo Bonzini wrote:
> On 02/02/21 01:18, Eduardo Habkost wrote:
> > On Tue, Feb 02, 2021 at 12:28:38AM +0100, Paolo Bonzini wrote:
> > > Il mar 2 feb 2021, 00:05 Eduardo Habkost <ehabkost@redhat.com> ha scritto:
> > > 
> > > > On Mon, Feb 01, 2021 at 11:59:48PM +0100, Paolo Bonzini wrote:
> > > > > Il lun 1 feb 2021, 23:54 Eduardo Habkost <ehabkost@redhat.com> ha
> > > > scritto:
> > > > > 
> > > > > > Not having a feature name in feature_word_info breaks error
> > > > > > reporting and query-cpu-model-expansion.  Add the missing feature
> > > > > > name to feature_word_info[FEAT_VMX_EPT_VPID_CAPS].feat_names[14].
> > > > > > 
> > > > > This is intentional, because there's no way that any hypervisor can run
> > > > if
> > > > > this feature is disabled.
> > > > 
> > > > If leaving the feature without name enables some desirable
> > > > behavior, that's by accident and not by design.  Which part of
> > > > the existing behavior is intentional?
> > > > 
> > > 
> > > Not being able to disable it.
> > 
> > We can make it a hard dependency of vmx, then.  We shouldn't
> > leave it without a name, though.
> 
> The feature is already added to the MSRs unconditionally in
> kvm_msr_entry_add_vmx.  I think we can just remove it from the models
> instead.

Sounds even simpler, and better.  I'll do it in v2.

-- 
Eduardo