Re: [Qemu-devel] [PATCH V7 0/2] Add option to configure guest vPMU

[Wei Huang <wei@redhat.com>]

On 10/25/2016 12:56 PM, Andrew Jones wrote:
<snip>
>>
>> Compared with V7, my proposed solution above isn't so different as we
>> thought. Details below. In V7,
>>
>> * has_pmu=off by default. It is turned on in target-arm/cpu.c (see
>> below). Apparently it is turned on only under KVM mode; UNDER TCG, IT
>> REMAINS OFF.
>>
>>     if (arm_feature(&cpu->env, ARM_FEATURE_PMU) && kvm_enabled()) {
>>         qdev_property_add_static(DEVICE(obj), &arm_cpu_has_pmu_property,
>>                                  &error_abort);
>>     }
>> * We then remove ARM_FEATURE_PMU if has_pmu=off. The rest of code will
>> know if PMU is on/off using arm_feature(&cpu->env, ARM_FEATURE_PMU).
>>
>> With this implementation, V7 can
>> * disable PMU under TCG mode
>> * print out an error when "pmu=on|off" property is specified under TCG.
>>
>> Drew didn't like it, he wanted a warning when "pmu=on", and didn't want
>> it when "pmu=off". This is fair. To do that, we need to add
>> arm_cpu_has_pmu_property for both KVM and TCG. But, in this case,
>> printing a warning message only for "pmu=on" isn't possible without
>> tri-state, because we can't tell if pmu is on by default OR turned on by
>> users using ",pmu=on".
>>
>> To solve this problem, in my proposed solution above, we still add
>> arm_cpu_has_pmu_property under TCG, but turned it off (see below).
>>
>>     if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
>>         qdev_property_add_static(DEVICE(obj),
>>             &arm_cpu_has_pmu_property, &error_abort);
>>         if (!kvm_enabled())
>>             object_property_set_bool(obj, false, "pmu", NULL);
>>     }
>>
>> Because has_pmu=off now, we remove ARM_FEATURE_PMU under TCG. THIS IS
>> SAME AS V7. If pmu=on is detected later, we know end-users turn it on
>> intentionally and can flag a warning message. With this approach, we can:
>> * disable PMU under TCG mode
>> * print a warning when ",pmu=on" is specified; it remains silent under
>> other cases.
>>
>> I think removing tri-state and printing out a warning only for ",pmu=on"
>> contradict each other. We need some trick to solve this problem. The
>> proposed solution didn't change the behavior underfoot as it might sound
>> like. If you have other suggestion, I want to know the details.
>>
> 
> I'm a bit lost as to which proposal is which now, but what I didn't like
> was QEMU failing to run with TCG when the same command line worked for
> KVM. If the property doesn't exist when using TCG then QEMU fails when a
> command line including ,pmu=on/off is used.
> 
> Peter says he's not worried about the PMU not working for TCG right now,
> and thus isn't worried about warning that it doesn't work. I'm not sure
> if he was proposing to keep your v7 - fail for tcg when the property was
> used, or if he was proposing to just not worry about the property for tcg.
> I believe the later case would be the proposed change to v7, but without
> any warning.

I think Peter means the later case, which requires a fix to v7. It will
keep the same command line for both TCG and KVM; but TCG will ignore
silently without a warning when ",pmu=on".

> 
> Anyway, whatever you guys work out is fine by me. I can live with changing
> the command line between KVM and TCG runs. Eventually I won't have to when
> PMU support comes to TCG. I'm also fine with ,pmu=on silently not actually
> providing a PMU when run under TCG. It's like Peter says, many things

I prefer the second 2nd one. If you are OK, I will send in a new spin (V8).

> under TCG don't work, but none of them warn about it. Only, in this case,
> I feel a warning for ,pmu=on would be better, because the feature would be
> getting asked for explicitly.
> 
> Thanks,
> drew
>