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

[Wei Huang]

On 10/25/2016 10:26 AM, Peter Maydell wrote:
> On 25 October 2016 at 08:33, Andrew Jones <address@hidden> wrote:
>> On Mon, Oct 24, 2016 at 11:39:59PM -0500, Wei Huang wrote:
>>>>> V6->V7:
>>>>>   * change has_pmu variable type from OnOffAuto to Boolean
>>>>>   * only add "pmu" property to CPU under kvm mode, default ON
>>>>
>>>> Hmm, if we don't allow the property with TCG then switching a guest from
>>>> KVM to TCG will require more than just an accelerator switch. That's a
>>>> bit annoying and I think we'd have to teach it to libvirt too. I'd prefer
>>>>
>>>>  -M virt-2.8,accel=tcg -cpu cortex-a57         NO     NO
>>>>  -M virt-2.8,accel=tcg -cpu cortex-a57,pmu=off NO     NO
>>>>  -M virt-2.8,accel=tcg -cpu cortex-a57,pmu=on  NO     "Warning: PMU not
>>>> yet supported with TCG" (or something)
>>>
>>> I am fine with this request. But note that, if we enforce
>>> pmu-default=ON, we can't tell "-cpu cortex-a57" apart from
>>> "cortex-a57,pmu=on", implying that we have to print the warning msg for
>>> the case of "cortex-a57" as well (this was why we switch to tri-state
>>> before). To solve this problem, we have to switch from pmu-default=ON to
>>> pmu-default=OFF under TCG mode, something like:
>>>
>>>     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);
>>>     }
>>>
>>> Then we do:
>>>     if (cpu->has_pmu && !kvm_enabled()) {
>>>         cpu->has_pmu = false;
>>>         if (!pmu_warned && !qtest_enabled()) {
>>>             error_report("warning: pmu not supported under TCG");
>>>             pmu_warned = true;
>>>         }
>>>     }
>>>
>>> This will work. Are you and Peter OK with this solution?
>>>
>>
>> Yes, I prefer it to not being able to easily switch a command line from
>> KVM to TCG. Granted, right now we mostly need to run with -cpu host for
>> KVM, which also needs to be switched for TCG. However that will hopefully
>> change with Shannon's work.
> 
> If the TCG default is different from the KVM default then you
> will have problems with switching it anyway, because it will
> silently change behaviour underfoot. I think we should avoid that...

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.

Thanks,
-Wei

> 
> thanks
> -- PMM
>