Eduardo Habkost <ehabk...@redhat.com> writes:
> On Thu, Apr 22, 2021 at 06:11:16PM +0200, Vitaly Kuznetsov wrote:
>> Clean up hv_cpuid_check_and_set() by separating hyperv_feature_supported()
>> off it. No functional change intended.
>>
>> Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
>> ---
>> target/i386/kvm/kvm.c | 49 ++++++++++++++++++++++++++-----------------
>> 1 file changed, 30 insertions(+), 19 deletions(-)
>>
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index f791baa29acf..ba093dba4d23 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -1107,13 +1107,33 @@ static int hv_cpuid_get_fw(struct kvm_cpuid2 *cpuid,
>> int fw, uint32_t *r)
>> return 0;
>> }
>>
>> +static bool hyperv_feature_supported(struct kvm_cpuid2 *cpuid, int feature)
>> +{
>> + uint32_t r, fw, bits;
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(kvm_hyperv_properties[feature].flags); i++) {
>> + fw = kvm_hyperv_properties[feature].flags[i].fw;
>> + bits = kvm_hyperv_properties[feature].flags[i].bits;
>> +
>> + if (!fw) {
>> + continue;
>> + }
>> +
>> + if (hv_cpuid_get_fw(cpuid, fw, &r) || (r & bits) != bits) {
>> + return false;
>> + }
>> + }
>> +
>> + return true;
>> +}
>> +
>> static int hv_cpuid_check_and_set(CPUState *cs, struct kvm_cpuid2 *cpuid,
>> int feature)
>> {
>> X86CPU *cpu = X86_CPU(cs);
>> - uint32_t r, fw, bits;
>> uint64_t deps;
>> - int i, dep_feat;
>> + int dep_feat;
>>
>> if (!hyperv_feat_enabled(cpu, feature) && !cpu->hyperv_passthrough) {
>> return 0;
>> @@ -1132,23 +1152,14 @@ static int hv_cpuid_check_and_set(CPUState *cs,
>> struct kvm_cpuid2 *cpuid,
>> deps &= ~(1ull << dep_feat);
>> }
>>
>> - for (i = 0; i < ARRAY_SIZE(kvm_hyperv_properties[feature].flags); i++) {
>> - fw = kvm_hyperv_properties[feature].flags[i].fw;
>> - bits = kvm_hyperv_properties[feature].flags[i].bits;
>> -
>> - if (!fw) {
>> - continue;
>> - }
>> -
>> - if (hv_cpuid_get_fw(cpuid, fw, &r) || (r & bits) != bits) {
>> - if (hyperv_feat_enabled(cpu, feature)) {
>> - fprintf(stderr,
>> - "Hyper-V %s is not supported by kernel\n",
>> - kvm_hyperv_properties[feature].desc);
>> - return 1;
>> - } else {
>> - return 0;
>> - }
>> + if (!hyperv_feature_supported(cpuid, feature)) {
>> + if (hyperv_feat_enabled(cpu, feature)) {
>> + fprintf(stderr,
>> + "Hyper-V %s is not supported by kernel\n",
>> + kvm_hyperv_properties[feature].desc);
>> + return 1;
>> + } else {
>> + return 0;
>
> The reason for returning prematurely here when
> !hyperv_feat_enabled() is not clear to me
This hopefully gets much better at the end of the series where
hv_cpuid_check_and_set() is split into 'check' and 'set'. The reason to
return immediately is: if the feature was not requested explicitly and
we're not in 'hv-passthrough' mode, there's no need to check whether KVM
supports it, if we have all the dependencies,...
> but you are keeping the existing behavior, so:
>
> Reviewed-by: Eduardo Habkost <ehabk...@redhat.com>
>
Thanks!
--
Vitaly
