Eduardo Habkost <ehabk...@redhat.com> writes:
> On Thu, Apr 22, 2021 at 06:11:27PM +0200, Vitaly Kuznetsov wrote:
>> hv_cpuid_check_and_set() does too much:
>> - Checks if the feature is supported by KVM;
>> - Checks if all dependencies are enabled;
>> - Sets the feature bit in cpu->hyperv_features for 'passthrough' mode.
>>
>> To reduce the complexity, move all the logic except for dependencies
>> check out of it. Also, in 'passthrough' mode we don't really need to
>> check dependencies because KVM is supposed to provide a consistent
>> set anyway.
>>
>> Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
>> ---
>> target/i386/kvm/kvm.c | 105 +++++++++++++++---------------------------
>> 1 file changed, 36 insertions(+), 69 deletions(-)
>>
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index d5551c4ab5cf..2c1a77f9b00f 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -1144,16 +1144,12 @@ static bool hyperv_feature_supported(CPUState *cs,
>> int feature)
>> return true;
>> }
>>
>> -static int hv_cpuid_check_and_set(CPUState *cs, int feature, Error **errp)
>> +/* Checks that all feature dependencies are enabled */
>> +static void hv_feature_check_deps(X86CPU *cpu, int feature, Error **errp)
>
> Same suggestion as in patch 11/19: also returning a value to
> indicate error is preferred. I would return a boolean.
>
Sure, can do.
> (I don't think this alone should block the series, though)
>
>
>> {
>> - X86CPU *cpu = X86_CPU(cs);
>> uint64_t deps;
>> int dep_feat;
>>
>> - if (!hyperv_feat_enabled(cpu, feature) && !cpu->hyperv_passthrough) {
>> - return 0;
>> - }
>> -
>> deps = kvm_hyperv_properties[feature].dependencies;
>> while (deps) {
>> dep_feat = ctz64(deps);
>> @@ -1161,26 +1157,10 @@ static int hv_cpuid_check_and_set(CPUState *cs, int
>> feature, Error **errp)
>> error_setg(errp, "Hyper-V %s requires Hyper-V %s",
>> kvm_hyperv_properties[feature].desc,
>> kvm_hyperv_properties[dep_feat].desc);
>> - return 1;
>> + return;
>> }
>> deps &= ~(1ull << dep_feat);
>> }
>> -
>> - if (!hyperv_feature_supported(cs, feature)) {
>> - if (hyperv_feat_enabled(cpu, feature)) {
>> - error_setg(errp, "Hyper-V %s is not supported by kernel",
>> - kvm_hyperv_properties[feature].desc);
>> - return 1;
>> - } else {
>> - return 0;
>> - }
>> - }
>> -
>> - if (cpu->hyperv_passthrough) {
>> - cpu->hyperv_features |= BIT(feature);
>> - }
>> -
>> - return 0;
>> }
>>
>> static uint32_t hv_build_cpuid_leaf(CPUState *cs, uint32_t func, int reg)
>> @@ -1219,6 +1199,8 @@ static uint32_t hv_build_cpuid_leaf(CPUState *cs,
>> uint32_t func, int reg)
>> void kvm_hyperv_expand_features(X86CPU *cpu, Error **errp)
>> {
>> CPUState *cs = CPU(cpu);
>> + Error *local_err = NULL;
>> + int feat;
>>
>> if (!hyperv_enabled(cpu))
>> return;
>> @@ -1274,53 +1256,38 @@ void kvm_hyperv_expand_features(X86CPU *cpu, Error
>> **errp)
>>
>> cpu->hyperv_spinlock_attempts =
>> hv_cpuid_get_host(cs, HV_CPUID_ENLIGHTMENT_INFO, R_EBX);
>> - }
>>
>> - /* Features */
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RELAXED, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_VAPIC, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_TIME, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_CRASH, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RESET, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_VPINDEX, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RUNTIME, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_SYNIC, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_FREQUENCIES, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_REENLIGHTENMENT, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_TLBFLUSH, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_EVMCS, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_IPI, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER_DIRECT, errp)) {
>> - return;
>> + /*
>> + * Mark feature as enabled in 'cpu->hyperv_features' as
>> + * hv_build_cpuid_leaf() uses this info to build guest CPUIDs.
>> + */
>> + for (feat = 0; feat < ARRAY_SIZE(kvm_hyperv_properties); feat++) {
>> + if (hyperv_feature_supported(cs, feat)) {
>> + cpu->hyperv_features |= BIT(feat);
>> + }
>> + }
>> + } else {
>> + /* Check features availability and dependencies */
>> + for (feat = 0; feat < ARRAY_SIZE(kvm_hyperv_properties); feat++) {
>> + /* If the feature was not requested skip it. */
>> + if (!hyperv_feat_enabled(cpu, feat)) {
>> + continue;
>> + }
>
> That's the loop I suggested in patch 11/19. Nice. :)
>
:-)
> Reviewed-by: Eduardo Habkost <ehabk...@redhat.com>
Thanks!
>
>> +
>> + /* Check if the feature is supported by KVM */
>> + if (!hyperv_feature_supported(cs, feat)) {
>> + error_setg(errp, "Hyper-V %s is not supported by kernel",
>> + kvm_hyperv_properties[feat].desc);
>> + return;
>> + }
>> +
>> + /* Check dependencies */
>> + hv_feature_check_deps(cpu, feat, &local_err);
>> + if (local_err != NULL) {
>> + error_propagate(errp, local_err);
>> + return;
>> + }
>> + }
>> }
>>
>> /* Additional dependencies not covered by kvm_hyperv_properties[] */
>> --
>> 2.30.2
>>
--
Vitaly
